| Message ID | IA1PR20MB4953F9D77FFC76A9D236922DBBB6A@IA1PR20MB4953.namprd20.prod.outlook.com |
|---|---|
| State | New |
| Headers | show |
| Series | Change the sg2042 timer layout to fit aclint format | expand |
On Sat, Nov 18, 2023 at 12:39 PM Inochi Amaoto <inochiama@outlook.com> wrote: > > The timer registers of aclint don't follow the clint layout and can > be mapped on any different offset. As sg2042 uses separated timer > and mswi for its clint, it should follow the aclint spec and have > separated registers. > > The previous patch introduced a new type of T-HEAD aclint timer which > has clint timer layout. Although it has the clint timer layout, it > should follow the aclint spec and uses the separated mtime and mtimecmp > regs. So a ABI change is needed to make the timer fit the aclint spec. > > To make T-HEAD aclint timer more closer to the aclint spec, use > regs-names to represent the mtimecmp register, which can avoid hack > for unsupport mtime register of T-HEAD aclint timer. > > Signed-off-by: Inochi Amaoto <inochiama@outlook.com> > Fixes: 4734449f7311 ("dt-bindings: timer: Add Sophgo sg2042 CLINT timer") > Link: https://lists.infradead.org/pipermail/opensbi/2023-October/005693.html > Link: https://github.com/riscv/riscv-aclint/blob/main/riscv-aclint.adoc The ratified Priv v1.12 specification defines platform specific M-mode timer registers without defining any layout of mtime and mtimecmp registers. (Refer, "3.2.1 Machine Timer Registers (mtime and mtimecmp)") The "thead,c900-aclint-mtimer" can be thought of as is one possible implementation of "riscv,mtimer" defined by the Priv v1.12 specificaiton. If it is not too late then I suggest making this binding into generic "riscv,mtimer" binding. Regards, Anup > --- > .../timer/thead,c900-aclint-mtimer.yaml | 42 ++++++++++++++++++- > 1 file changed, 41 insertions(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/timer/thead,c900-aclint-mtimer.yaml b/Documentation/devicetree/bindings/timer/thead,c900-aclint-mtimer.yaml > index fbd235650e52..053488fb1286 100644 > --- a/Documentation/devicetree/bindings/timer/thead,c900-aclint-mtimer.yaml > +++ b/Documentation/devicetree/bindings/timer/thead,c900-aclint-mtimer.yaml > @@ -17,7 +17,20 @@ properties: > - const: thead,c900-aclint-mtimer > > reg: > - maxItems: 1 > + oneOf: > + - items: > + - description: MTIME Registers > + - description: MTIMECMP Registers > + - items: > + - description: MTIMECMP Registers > + > + reg-names: > + oneOf: > + - items: > + - const: mtime > + - const: mtimecmp > + - items: > + - const: mtimecmp > > interrupts-extended: > minItems: 1 > @@ -28,8 +41,34 @@ additionalProperties: false > required: > - compatible > - reg > + - reg-names > - interrupts-extended > > +allOf: > + - if: > + properties: > + compatible: > + contains: > + const: thead,c900-aclint-mtimer > + then: > + properties: > + reg: > + items: > + - description: MTIMECMP Registers > + reg-names: > + items: > + - const: mtimecmp > + else: > + properties: > + reg: > + items: > + - description: MTIME Registers > + - description: MTIMECMP Registers > + reg-names: > + items: > + - const: mtime > + - const: mtimecmp > + > examples: > - | > timer@ac000000 { > @@ -39,5 +78,6 @@ examples: > <&cpu3intc 7>, > <&cpu4intc 7>; > reg = <0xac000000 0x00010000>; > + reg-names = "mtimecmp"; > }; > ... > -- > 2.42.1 > >
On Thu, Nov 30, 2023 at 03:01:24PM +0530, Anup Patel wrote: > On Sat, Nov 18, 2023 at 12:39 PM Inochi Amaoto <inochiama@outlook.com> wrote: > > > > The timer registers of aclint don't follow the clint layout and can > > be mapped on any different offset. As sg2042 uses separated timer > > and mswi for its clint, it should follow the aclint spec and have > > separated registers. > > > > The previous patch introduced a new type of T-HEAD aclint timer which > > has clint timer layout. Although it has the clint timer layout, it > > should follow the aclint spec and uses the separated mtime and mtimecmp > > regs. So a ABI change is needed to make the timer fit the aclint spec. > > > > To make T-HEAD aclint timer more closer to the aclint spec, use > > regs-names to represent the mtimecmp register, which can avoid hack > > for unsupport mtime register of T-HEAD aclint timer. > > > > Signed-off-by: Inochi Amaoto <inochiama@outlook.com> > > Fixes: 4734449f7311 ("dt-bindings: timer: Add Sophgo sg2042 CLINT timer") > > Link: https://lists.infradead.org/pipermail/opensbi/2023-October/005693.html > > Link: https://github.com/riscv/riscv-aclint/blob/main/riscv-aclint.adoc > > The ratified Priv v1.12 specification defines platform specific M-mode timer > registers without defining any layout of mtime and mtimecmp registers. > (Refer, "3.2.1 Machine Timer Registers (mtime and mtimecmp)") > > The "thead,c900-aclint-mtimer" can be thought of as is one possible > implementation of "riscv,mtimer" defined by the Priv v1.12 specificaiton. > > If it is not too late then I suggest making this binding into generic > "riscv,mtimer" binding. We could definitely reorganise things, it's not too late for that as implementation specific compatibles would be needed regardless, so software that would've matched on those will continue to do so. That said, does this platform actually implement the 1.12 priv spec if there is no mtime register? The section you reference says: "Platforms provide a real-time counter, exposed as a memory-mapped machine-mode read-write register, mtime." It seems to me like this hardware is not suitable for a generic "riscv,mtimer" fallback. Am I missing something there Anup? It doesn't even implement the draft aclint spec, given that that says: "The MTIMER device provides machine-level timer functionality for a set of HARTs on a RISC-V platform. It has a single fixed-frequency monotonic time counter (MTIME) register and a time compare register (MTIMECMP) for each HART connected to the MTIMER device." But I already said no to having a generic, "riscv" prefixed, compatible for that, given it is in draft form. Cheers, Conor. > > --- > > .../timer/thead,c900-aclint-mtimer.yaml | 42 ++++++++++++++++++- > > 1 file changed, 41 insertions(+), 1 deletion(-) > > > > diff --git a/Documentation/devicetree/bindings/timer/thead,c900-aclint-mtimer.yaml b/Documentation/devicetree/bindings/timer/thead,c900-aclint-mtimer.yaml > > index fbd235650e52..053488fb1286 100644 > > --- a/Documentation/devicetree/bindings/timer/thead,c900-aclint-mtimer.yaml > > +++ b/Documentation/devicetree/bindings/timer/thead,c900-aclint-mtimer.yaml > > @@ -17,7 +17,20 @@ properties: > > - const: thead,c900-aclint-mtimer > > > > reg: > > - maxItems: 1 > > + oneOf: > > + - items: > > + - description: MTIME Registers > > + - description: MTIMECMP Registers > > + - items: > > + - description: MTIMECMP Registers > > + > > + reg-names: > > + oneOf: > > + - items: > > + - const: mtime > > + - const: mtimecmp > > + - items: > > + - const: mtimecmp > > > > interrupts-extended: > > minItems: 1 > > @@ -28,8 +41,34 @@ additionalProperties: false > > required: > > - compatible > > - reg > > + - reg-names > > - interrupts-extended > > > > +allOf: > > + - if: > > + properties: > > + compatible: > > + contains: > > + const: thead,c900-aclint-mtimer > > + then: > > + properties: > > + reg: > > + items: > > + - description: MTIMECMP Registers > > + reg-names: > > + items: > > + - const: mtimecmp > > + else: > > + properties: > > + reg: > > + items: > > + - description: MTIME Registers > > + - description: MTIMECMP Registers > > + reg-names: > > + items: > > + - const: mtime > > + - const: mtimecmp > > + > > examples: > > - | > > timer@ac000000 { > > @@ -39,5 +78,6 @@ examples: > > <&cpu3intc 7>, > > <&cpu4intc 7>; > > reg = <0xac000000 0x00010000>; > > + reg-names = "mtimecmp"; > > }; > > ... > > -- > > 2.42.1 > > > >
On Thu, Nov 30, 2023 at 3:27 PM Conor Dooley <conor@kernel.org> wrote: > > On Thu, Nov 30, 2023 at 03:01:24PM +0530, Anup Patel wrote: > > On Sat, Nov 18, 2023 at 12:39 PM Inochi Amaoto <inochiama@outlook.com> wrote: > > > > > > The timer registers of aclint don't follow the clint layout and can > > > be mapped on any different offset. As sg2042 uses separated timer > > > and mswi for its clint, it should follow the aclint spec and have > > > separated registers. > > > > > > The previous patch introduced a new type of T-HEAD aclint timer which > > > has clint timer layout. Although it has the clint timer layout, it > > > should follow the aclint spec and uses the separated mtime and mtimecmp > > > regs. So a ABI change is needed to make the timer fit the aclint spec. > > > > > > To make T-HEAD aclint timer more closer to the aclint spec, use > > > regs-names to represent the mtimecmp register, which can avoid hack > > > for unsupport mtime register of T-HEAD aclint timer. > > > > > > Signed-off-by: Inochi Amaoto <inochiama@outlook.com> > > > Fixes: 4734449f7311 ("dt-bindings: timer: Add Sophgo sg2042 CLINT timer") > > > Link: https://lists.infradead.org/pipermail/opensbi/2023-October/005693.html > > > Link: https://github.com/riscv/riscv-aclint/blob/main/riscv-aclint.adoc > > > > The ratified Priv v1.12 specification defines platform specific M-mode timer > > registers without defining any layout of mtime and mtimecmp registers. > > (Refer, "3.2.1 Machine Timer Registers (mtime and mtimecmp)") > > > > The "thead,c900-aclint-mtimer" can be thought of as is one possible > > implementation of "riscv,mtimer" defined by the Priv v1.12 specificaiton. > > > > If it is not too late then I suggest making this binding into generic > > "riscv,mtimer" binding. > > We could definitely reorganise things, it's not too late for that as > implementation specific compatibles would be needed regardless, so > software that would've matched on those will continue to do so. > > That said, does this platform actually implement the 1.12 priv spec if > there is no mtime register? The section you reference says: > "Platforms provide a real-time counter, exposed as a memory-mapped > machine-mode read-write register, mtime." It seems to me like this > hardware is not suitable for a generic "riscv,mtimer" fallback. Yes, the T-Head mtimer does not implement both mtime and mtimecmp so technically it only implements a portion of the ratified RISC-V mtimer chapter. > > Am I missing something there Anup? > > It doesn't even implement the draft aclint spec, given that that says: > "The MTIMER device provides machine-level timer functionality for a set > of HARTs on a RISC-V platform. It has a single fixed-frequency monotonic > time counter (MTIME) register and a time compare register (MTIMECMP) for > each HART connected to the MTIMER device." > > But I already said no to having a generic, "riscv" prefixed, compatible > for that, given it is in draft form. I am not suggesting T-Head timer implements aclint spec. Also, since aclint spec is in draft state it is out of the question. My suggestion is to treat "3.2.1 Machine Timer Registers (mtime and mtimecmp)" as RISC-V mtimer defined by the RISC-V privileged specification and define "riscv" prefixed DT binding for this. This binding defines two possible values for "reg" property: 1) contains two items: a) mtime register address and, b) base address of mtimecmp registers 2) contain one item: a) base address of mtimecmp registers The t-head mtimer seems to implement #2 whereas the RISC-V mtimer (Priv spec) aligns with #1. If we want to keep this DT binding t-head specific then we should remove option #1 (above) from this DT binding and add separate "riscv" prefixed DT binding for RISC-V mtimer. Regards, Anup > > Cheers, > Conor. > > > > --- > > > .../timer/thead,c900-aclint-mtimer.yaml | 42 ++++++++++++++++++- > > > 1 file changed, 41 insertions(+), 1 deletion(-) > > > > > > diff --git a/Documentation/devicetree/bindings/timer/thead,c900-aclint-mtimer.yaml b/Documentation/devicetree/bindings/timer/thead,c900-aclint-mtimer.yaml > > > index fbd235650e52..053488fb1286 100644 > > > --- a/Documentation/devicetree/bindings/timer/thead,c900-aclint-mtimer.yaml > > > +++ b/Documentation/devicetree/bindings/timer/thead,c900-aclint-mtimer.yaml > > > @@ -17,7 +17,20 @@ properties: > > > - const: thead,c900-aclint-mtimer > > > > > > reg: > > > - maxItems: 1 > > > + oneOf: > > > + - items: > > > + - description: MTIME Registers > > > + - description: MTIMECMP Registers > > > + - items: > > > + - description: MTIMECMP Registers > > > + > > > + reg-names: > > > + oneOf: > > > + - items: > > > + - const: mtime > > > + - const: mtimecmp > > > + - items: > > > + - const: mtimecmp > > > > > > interrupts-extended: > > > minItems: 1 > > > @@ -28,8 +41,34 @@ additionalProperties: false > > > required: > > > - compatible > > > - reg > > > + - reg-names > > > - interrupts-extended > > > > > > +allOf: > > > + - if: > > > + properties: > > > + compatible: > > > + contains: > > > + const: thead,c900-aclint-mtimer > > > + then: > > > + properties: > > > + reg: > > > + items: > > > + - description: MTIMECMP Registers > > > + reg-names: > > > + items: > > > + - const: mtimecmp > > > + else: > > > + properties: > > > + reg: > > > + items: > > > + - description: MTIME Registers > > > + - description: MTIMECMP Registers > > > + reg-names: > > > + items: > > > + - const: mtime > > > + - const: mtimecmp > > > + > > > examples: > > > - | > > > timer@ac000000 { > > > @@ -39,5 +78,6 @@ examples: > > > <&cpu3intc 7>, > > > <&cpu4intc 7>; > > > reg = <0xac000000 0x00010000>; > > > + reg-names = "mtimecmp"; > > > }; > > > ... > > > -- > > > 2.42.1 > > > > > >
On Thu, Nov 30, 2023 at 04:51:32PM +0530, Anup Patel wrote: > On Thu, Nov 30, 2023 at 3:27 PM Conor Dooley <conor@kernel.org> wrote: > > > > On Thu, Nov 30, 2023 at 03:01:24PM +0530, Anup Patel wrote: > > > On Sat, Nov 18, 2023 at 12:39 PM Inochi Amaoto <inochiama@outlook.com> wrote: > > > > > > > > The timer registers of aclint don't follow the clint layout and can > > > > be mapped on any different offset. As sg2042 uses separated timer > > > > and mswi for its clint, it should follow the aclint spec and have > > > > separated registers. > > > > > > > > The previous patch introduced a new type of T-HEAD aclint timer which > > > > has clint timer layout. Although it has the clint timer layout, it > > > > should follow the aclint spec and uses the separated mtime and mtimecmp > > > > regs. So a ABI change is needed to make the timer fit the aclint spec. > > > > > > > > To make T-HEAD aclint timer more closer to the aclint spec, use > > > > regs-names to represent the mtimecmp register, which can avoid hack > > > > for unsupport mtime register of T-HEAD aclint timer. > > > > > > > > Signed-off-by: Inochi Amaoto <inochiama@outlook.com> > > > > Fixes: 4734449f7311 ("dt-bindings: timer: Add Sophgo sg2042 CLINT timer") > > > > Link: https://lists.infradead.org/pipermail/opensbi/2023-October/005693.html > > > > Link: https://github.com/riscv/riscv-aclint/blob/main/riscv-aclint.adoc > > > > > > The ratified Priv v1.12 specification defines platform specific M-mode timer > > > registers without defining any layout of mtime and mtimecmp registers. > > > (Refer, "3.2.1 Machine Timer Registers (mtime and mtimecmp)") > > > > > > The "thead,c900-aclint-mtimer" can be thought of as is one possible > > > implementation of "riscv,mtimer" defined by the Priv v1.12 specificaiton. > > > > > > If it is not too late then I suggest making this binding into generic > > > "riscv,mtimer" binding. > > > > We could definitely reorganise things, it's not too late for that as > > implementation specific compatibles would be needed regardless, so > > software that would've matched on those will continue to do so. > > > > That said, does this platform actually implement the 1.12 priv spec if > > there is no mtime register? The section you reference says: > > "Platforms provide a real-time counter, exposed as a memory-mapped > > machine-mode read-write register, mtime." It seems to me like this > > hardware is not suitable for a generic "riscv,mtimer" fallback. > > Yes, the T-Head mtimer does not implement both mtime and mtimecmp > so technically it only implements a portion of the ratified RISC-V mtimer > chapter. > > > > > Am I missing something there Anup? > > > > It doesn't even implement the draft aclint spec, given that that says: > > "The MTIMER device provides machine-level timer functionality for a set > > of HARTs on a RISC-V platform. It has a single fixed-frequency monotonic > > time counter (MTIME) register and a time compare register (MTIMECMP) for > > each HART connected to the MTIMER device." > > > > But I already said no to having a generic, "riscv" prefixed, compatible > > for that, given it is in draft form. > > I am not suggesting T-Head timer implements aclint spec. Also, > since aclint spec is in draft state it is out of the question. I did not intend to imply that you were suggesting that there should be one. I was just trying to clarify that I was not trying to bring back the topic of a generic aclint binding applying here. > My suggestion is to treat "3.2.1 Machine Timer Registers (mtime > and mtimecmp)" as RISC-V mtimer defined by the RISC-V privileged > specification and define "riscv" prefixed DT binding for this. I'm not against a binding for that at all. > This binding defines two possible values for "reg" property: > 1) contains two items: a) mtime register address and, > b) base address of mtimecmp registers > 2) contain one item: a) base address of mtimecmp registers > > The t-head mtimer seems to implement #2 whereas the RISC-V > mtimer (Priv spec) aligns with #1. > > If we want to keep this DT binding t-head specific then > we should remove option #1 (above) from this DT binding This part is already the conclusion of one of the other "branches" of this thread and is (AFAIU) Inochi's plan for the next version. > and add separate "riscv" prefixed DT binding for RISC-V mtimer. Do you know of any users for a "riscv,mtimer" binding that are not covered by existing bindings for the clint? Cheers, Conor.
On Thu, Nov 30, 2023 at 05:18:15PM +0530, Anup Patel wrote: > On Thu, Nov 30, 2023 at 5:15 PM Conor Dooley <conor@kernel.org> wrote: > > > and add separate "riscv" prefixed DT binding for RISC-V mtimer. > > > > Do you know of any users for a "riscv,mtimer" binding that are not > > covered by existing bindings for the clint? > > Ventana Veyron-v1 implements a mtimer per-cluster (or chiplet) > which is compatible to "riscv,mtimer" (i.e. we have both mtime > and mtimecmp MMIO registers). Okay, thanks. I guess iff veyron-v1 DT support shows up (or other similar devices) we can go ahead with a "riscv,mtimer" binding then. I had thought that you guys were going to be using ACPI though, so I guess the "other similar devices" applies.
On Thu, Nov 30, 2023 at 5:40 PM Conor Dooley <conor@kernel.org> wrote: > > On Thu, Nov 30, 2023 at 05:18:15PM +0530, Anup Patel wrote: > > On Thu, Nov 30, 2023 at 5:15 PM Conor Dooley <conor@kernel.org> wrote: > > > > > and add separate "riscv" prefixed DT binding for RISC-V mtimer. > > > > > > Do you know of any users for a "riscv,mtimer" binding that are not > > > covered by existing bindings for the clint? > > > > Ventana Veyron-v1 implements a mtimer per-cluster (or chiplet) > > which is compatible to "riscv,mtimer" (i.e. we have both mtime > > and mtimecmp MMIO registers). > > Okay, thanks. I guess iff veyron-v1 DT support shows up (or other > similar devices) we can go ahead with a "riscv,mtimer" binding then. > I had thought that you guys were going to be using ACPI though, so > I guess the "other similar devices" applies. We use ACPI from EDK2 onwards in our boot-flow. The booting stages prior to EDK2 (such as OpenSBI) use DT. In fact, EDK2 also uses information in DT to populate static parts of the ACPI table. Regards, Anup
diff --git a/Documentation/devicetree/bindings/timer/thead,c900-aclint-mtimer.yaml b/Documentation/devicetree/bindings/timer/thead,c900-aclint-mtimer.yaml index fbd235650e52..053488fb1286 100644 --- a/Documentation/devicetree/bindings/timer/thead,c900-aclint-mtimer.yaml +++ b/Documentation/devicetree/bindings/timer/thead,c900-aclint-mtimer.yaml @@ -17,7 +17,20 @@ properties: - const: thead,c900-aclint-mtimer reg: - maxItems: 1 + oneOf: + - items: + - description: MTIME Registers + - description: MTIMECMP Registers + - items: + - description: MTIMECMP Registers + + reg-names: + oneOf: + - items: + - const: mtime + - const: mtimecmp + - items: + - const: mtimecmp interrupts-extended: minItems: 1 @@ -28,8 +41,34 @@ additionalProperties: false required: - compatible - reg + - reg-names - interrupts-extended +allOf: + - if: + properties: + compatible: + contains: + const: thead,c900-aclint-mtimer + then: + properties: + reg: + items: + - description: MTIMECMP Registers + reg-names: + items: + - const: mtimecmp + else: + properties: + reg: + items: + - description: MTIME Registers + - description: MTIMECMP Registers + reg-names: + items: + - const: mtime + - const: mtimecmp + examples: - | timer@ac000000 { @@ -39,5 +78,6 @@ examples: <&cpu3intc 7>, <&cpu4intc 7>; reg = <0xac000000 0x00010000>; + reg-names = "mtimecmp"; }; ...
The timer registers of aclint don't follow the clint layout and can be mapped on any different offset. As sg2042 uses separated timer and mswi for its clint, it should follow the aclint spec and have separated registers. The previous patch introduced a new type of T-HEAD aclint timer which has clint timer layout. Although it has the clint timer layout, it should follow the aclint spec and uses the separated mtime and mtimecmp regs. So a ABI change is needed to make the timer fit the aclint spec. To make T-HEAD aclint timer more closer to the aclint spec, use regs-names to represent the mtimecmp register, which can avoid hack for unsupport mtime register of T-HEAD aclint timer. Signed-off-by: Inochi Amaoto <inochiama@outlook.com> Fixes: 4734449f7311 ("dt-bindings: timer: Add Sophgo sg2042 CLINT timer") Link: https://lists.infradead.org/pipermail/opensbi/2023-October/005693.html Link: https://github.com/riscv/riscv-aclint/blob/main/riscv-aclint.adoc --- .../timer/thead,c900-aclint-mtimer.yaml | 42 ++++++++++++++++++- 1 file changed, 41 insertions(+), 1 deletion(-) -- 2.42.1