Re: [PATCH v4 1/2] riscv-platform-spec: PLIC and CLINT for Linux-2022 platform


Alistair Francis
 

On Mon, 2021-05-24 at 14:52 +0800, Abner Chang wrote:


Alistair Francis <Alistair.Francis@...> 於 2021年5月24日 週一
上午11:42寫道:
On Mon, 2021-05-24 at 11:03 +0800, Abner Chang wrote:
From: Abner Chang <renba.chang@...>

Initial description of PLIC  CLINT section of Linux-2022
platform.

Is this what we want to see of CLINT/Machine mode timer in the
platform spec?

On v4 commit,
- PLIC section with [DEPRECATED] in Linux- 2022 chapter
- CLINT section in Linux- 2022 chapter for M-mode timer. We don't
mention
  IPI because AIA already supported it.
- In Embedded-2022 Machine mode timer section, CLINT is not
mandatory.
- Separate section in appendix for the Machine mode timer
registers

On v3 commit,
- Address review comments.

On v2 commit,
- CLINT is not deprecated.

- Add a standalone section for Machine Mode Timer in System
Peripherals.
  Do you think this is a good place for Machine Mode Timer?
  @Mayuresh, please check if you are ok with this change, not
sure
if
this
  overlaps with your text or not (The timer setion). I can remove
this
  if you prefer to put this with your patch.

- In Embedded-2022, refer to Machine Mode Timer in System
Peripherals
  section and CLINT in Linux-2022 Platform.
  @Alistair, is this ok?

On v1 commit,
- Not sure where to put the [DEPRECATED].
- Change the reference of PLIC in section 2.2.2. Interrupt
Controller
to
  1.1.3.2 PLIC + CLINT section.

Signed-off-by: Abner Chang <renba.chang@...>
Cc: Alistair Francis <alistair.francis@...>
Cc: Sunil V L <sunilvl@...>
Cc: Mayuresh Chitale <mchitale@...>
---
 riscv-platform-spec.adoc | 104 +++++++++++++++++++++++----------
--
--
--
 1 file changed, 62 insertions(+), 42 deletions(-)

diff --git a/riscv-platform-spec.adoc b/riscv-platform-spec.adoc
index 160c74a..f8838ab 100644
--- a/riscv-platform-spec.adoc
+++ b/riscv-platform-spec.adoc
@@ -49,15 +49,37 @@ include::profiles.adoc[]
 * Start Address
 
 ==== Interrupt Controller
-* AIA
-* PLIC + CLINT
-* Interrupt Assignments
+===== AIA[[AIA]]
+===== PLIC[DEPRECATED][[PLIC]]
+The Platform Level Interrupt Controller (PLIC) provides
facilities
to route
+the non-local interrupts to the external interrupt of a hart
context
+with a given privilege mode in a given hart. The number of non-
local
interrupt
+sources supported by PLIC and how does each of them connect to
the
hart
+context is PLIC core implementation-specific. +
+(Refer to   
https://github.com/riscv/riscv-plic-spec/blob/master/riscv-plic.adoc[RISC-V
 PLIC Specification]
+for the implementation reference of PLIC operation parameters)
+
+===== CLINT[[CLINT]]
+On the contrast to PLIC, the Core Local Interrupt (CLINT)
provides
I don't think this is a contrast to the PLIC, it's just a different
functionality.
Can rephase it. 

I'm also not sure the CLINT should be in the interrupt controller
section. It is the Core Local INTerruptor (CLINT), not an interrupt
controller.
Hmm.. CLINT is not a sort of interrupt controller but seems to me it
is unnecessary to create another section apart from the Interrupt
Controller section.
It probably makes more sense to add a timer section and put the CLNIT
there instead of forcing it into the interrupt controller section, when
it isn't really an interrupt controller.


+facilities to trigger local interrupt of
<<MachineModeTimer,Machine
mode timer>> to hart.
+
+===== Interrupt Assignments
 
 ==== System Peripherals
-* UART/Serial Console
-* Clocks
-* Timers
-* Watchdog Timers
+===== UART/Serial Console
+===== Clocks
+===== Timers
+
+===== Machine Mode Timer[[MachineModeTimer]]
+Machine mode timer is required for Linux-2022 platform and
incorporate with
+<<CLINT,CLINT>> to trigger the timer event to hart. The format
of
Machine mode timer operation parameters
+(`mtime` and `mtimecmp` registers) must compliant with RISC-V
s/must compliant/must be compliant/g
will correct this 

Privilege specification section 3.1.10.
+The base address of the memory map registers of Machine mode
timer
is platform implementation-specific,
+however the offset of `mtime` and `mtimecmp` registers are fixed
as
+<<machineModeTimerRegs,Machine mode timer registers>>.
+
+
+===== Watchdog Timers
 
 ==== Boot Process
 * Firmware
@@ -289,9 +311,8 @@ Any RISC-V system that uses at least RV32/64G
can
meet the Embedded-2022
 specification.
 
 ==== Interrupt Controller
-Embedded systems are recommended to use a spec compliant
-https://github.com/riscv/riscv-plic-spec[PLIC], a spec compliant
-       
https://github.com/riscv/riscv-fast-interrupt/blob/master/clic.adoc[CLIC]
+Embedded systems are recommended to use a spec compliant
<<PLIC,PLIC>>, a spec
+compliant         
https://github.com/riscv/riscv-fast-interrupt/blob/master/clic.adoc[CLIC]
 or both a CLIC and and PLIC.
 
 If using just a PLIC the system must continue to use the
original
basic
@@ -303,38 +324,9 @@ must be supported.
 Embedded systems cannot use a non-compliant interrupt controller
and
still
 call it a PLIC or CLIC.
 
-==== Machine Timer
-The RISC-V machine timer (controlled via `mtime` and `mtimecmp`)
must be
-implemented. The two registers must be memory mapped as required
by
the RISC-V
-specification.
-
-The Embedded-2022 specification requires that the registers be
mapped
-adjacent to each other with the `mtime` region at the lower
address.
-
-The starting address of this region can be located anywhere in
-memory, including inside other peripherals, as long as the start
address is
-4 byte aligned.
-
-An example of the memory layout for a 32-bit system with a
single
hart is below
-
--------------------------
-=========================
-| 0x00 |  mtime low     |
-| 0x04 |  mtime high    |
-| 0x08 |  mtimecmp low  |
-| 0x0C |  mtimecmp high |
-=========================
--------------------------
-
-and for a 64-bit system with 2 harts
-
----------------------------
-===========================
-| 0x00 |  mtime           |
-| 0x08 |  mtimecmp hart 1 |
-| 0x10 |  mtimecmp hart 2 |
-===========================
----------------------------
+==== Machine Mode Timer
+The Embedded-2022 specification requires RISC-V
<<MachineModeTimer,Machine mode timer>> to be implemented.
+The Machine mode timer could be incorporated with the Core Local
Interrupt (<<CLINT, CLINT>>) or the specific event trigger
mechanism

CLINT is INTerruptor not just Interrupt.
yes, will correct it

according to the platform design.
What specific trigger mechanism?
platform implementation-specific trigger mechanism?  or just remove
I meant you didn't say what the mechanism is. It should be explicit, so
that there are no ambiguities.

this? I put this just because the previous feedback says a small
system may not have CLINT for the m-mode timer.
I would be in favour of saying either do what we already have written
here OR a CLINT.

If we go with something else that's ok, we just need to be clear about
what it is.


 
 ==== Memory Map
 It is recommended that main memory and loadable code (not ROM)
start
at
@@ -349,5 +341,33 @@ When PMP is supported it is recommended to
include at least 4 regions, although
 if possible more should be supported to allow more flexibility.
Hardware
 implementations should aim for supporting at least 16 PMP
regions.
 
+// Appendix sections
+== Appendix
+=== Machine Mode Timer Registers [[MachineModeTimerRegs]]
+The base address of Machine mode timer operation parameters is
platform implementation-specific. The Machine mode timer compare
registers (`mtimecmp`) for each hart is located at offset 0 and
the
layout is organized as below table.
Can we maintain what we have for the embedded spec?
Do you mean to maintain the register layout(offset) originally
defined in the 2.2.3. for embedded systems? 
Yes. I worry that mandating a CLINT layout is too restrictive and that
no one will end up following the embedded-2022 spec then. I would be in
favour of saying CLINT or the memory layout we already have.

I think we have the register layout as below to be compliant with the
m-mode timer registers defined in SiFive CLINT.
I don't think that's flexible enough and that vendors won't want to
follow it as they already have other IP they are using for timers.

Alistair

Or do you mean something else?

 It allows the
memory maps to be inside of a MMIO device. For example they can be
registers inside of a general system controller.


Also it looks like the lines are a little long here, can be bring
them
back to 80 charecters?
sure. 

+
+.Registers layout of mtimecmp
+[width="60%",cols="1,^3,^3"]
+|=======
+|*Offset*|*Register (8-byte) for RV64*|*Register (4-byte) for
RV32*
+|`0x0000` |mtimecmp for hart 0 |mtimecmp low for hart 0
+|`0x0004` ||mtimecmp high for hart 0
+|`0x0008` |mtimecmp for hart 1 |mtimecmp low for hart 1
+|`0x000c` ||mtimecmp high for hart 1
+|... ||
+|`0x7ff0` |mtimecmp for hart 4094|mtimecmp low for hart 4094
+|`0x7ff4` ||mtimecmp high for hart 4094
+|=======
+
+The Machine mode timer counter (`mtime`) is located at offset
0x7ff8
from the base address of operation parameters.
+
+.Registers layout of mtime
+[width="60%",cols="1,^3,^3"]
+|=======
+|*Offset*|*Register (8-byte) for RV64*|*Register (4-byte) for
RV32*
+|`0x7ff8` |mtime|mtime low
+|`0x7ffc` ||mtime high
+|=======
Are you saying for a small embedded single core CPU we need to put
mtimecmp at offset 0x00 and mtime at 0x7ff8? What about all the
addresses between them?

Alistair

+
 // acknowledge all of the contributors
 include::contributors.adoc[]

Join tech-unixplatformspec@lists.riscv.org to automatically receive all group messages.