Message-ID: <CAK9=C2Vsx8ddpYiqUhvqnJpdb-FKeNhLz3PsVpSeEz4TeQHiEg@mail.gmail.com>
Date:   Thu, 30 Nov 2023 16:51:32 +0530
From:   Anup Patel <apatel@...tanamicro.com>
To:     Conor Dooley <conor@...nel.org>
Cc:     Inochi Amaoto <inochiama@...look.com>,
        Daniel Lezcano <daniel.lezcano@...aro.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley <conor+dt@...nel.org>,
        Paul Walmsley <paul.walmsley@...ive.com>,
        Palmer Dabbelt <palmer@...belt.com>,
        Albert Ou <aou@...s.berkeley.edu>,
        Chen Wang <unicorn_wang@...look.com>,
        Anup Patel <anup@...infault.org>,
        Samuel Holland <samuel.holland@...ive.com>,
        Guo Ren <guoren@...nel.org>,
        Jisheng Zhang <jszhang@...nel.org>,
        linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
        linux-riscv@...ts.infradead.org
Subject: Re: [PATCH v4 1/2] dt-bindings: timer: thead,c900-aclint-mtimer:
 separate mtime and mtimecmp regs

On Thu, Nov 30, 2023 at 3:27 PM Conor Dooley <conor@...nel.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@...look.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@...look.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@...00000 {
> > > @@ -39,5 +78,6 @@ examples:
> > >                              <&cpu3intc 7>,
> > >                              <&cpu4intc 7>;
> > >        reg = <0xac000000 0x00010000>;
> > > +      reg-names = "mtimecmp";
> > >      };
> > >  ...
> > > --
> > > 2.42.1
> > >
> > >

Powered by blists - more mailing lists