Re: [PATCH v4 1/2] dt-bindings: timer: thead,c900-aclint-mtimer: separate mtime and mtimecmp regs

[Conor Dooley]

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@xxxxxxxxxxx> 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@xxxxxxxxxxx>
> > 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
> >
> >

Attachment: signature.asc
Description: PGP signature