Re: [PATCH 1/2] dt-bindings: timer: add a binding for LiteX Timer

[Icenowy Zheng]

在 2022-11-21星期一的 11:19 +0100，Krzysztof Kozlowski写道：
> On 21/11/2022 05:25, Icenowy Zheng wrote:
> > The LiteX SoC generator has a timer core, which by default only
> > generates a simple down counter.
> 
> Subject: drop second, redundant "bindings".
> 
> > 
> > Add a DT binding for it.
> > 
> > Signed-off-by: Icenowy Zheng <uwu@xxxxxxxxxx>
> > ---
> >  .../bindings/timer/litex,timer.yaml           | 52
> > +++++++++++++++++++
> >  1 file changed, 52 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/timer/litex,timer.yaml
> > 
> > diff --git
> > a/Documentation/devicetree/bindings/timer/litex,timer.yaml
> > b/Documentation/devicetree/bindings/timer/litex,timer.yaml
> > new file mode 100644
> > index 000000000000..bece07586c6b
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/timer/litex,timer.yaml
> > @@ -0,0 +1,52 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/timer/litex,timer.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: LiteX Timer
> > +
> > +maintainers:
> > +  - Icenowy Zheng <uwu@xxxxxxxxxx>
> > +
> > +description: |
> > +  The LiteX Timer is a count-down timer that is defaultly embedded
> > +  into all LiteX SoCs, unless explicitly disabled. It's fed
> > directly
> > +  by the system clock like other LiteX peripherals.
> > +
> > +properties:
> > +  compatible:
> > +    const: litex,timer
> 
> No model name/number? If it is part of Soc, then a SoC specific
> number
> is expected usually.

Ah it's part of a SoC generator, as a default core. [1]

If you like, I think the version of LiteX SoC generator itself could be
added, like `litex,timer-22.08`.

> 
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    maxItems: 1
> > +
> > +  litex,width:
> > +    description:
> > +      The width of the timer's value, specified as the width
> > argument
> > +      when creating an instance of litex.soc.cores.Timer.
> 
> This lacks type ($ref) and units in description, but more important -
> why this is not part of compatible? Is it a width of register(s)?
> 
> And what is "instance of litex.soc.cores.Timer"? Is it configurable,
> soft-core?

Yes, it is a configurable soft core, although the configuration of a
non-32-bit counter is only available when directly generating this core
w/o using the full SoC generator (the full SoC generator defaults to
32-bit).

> 
> BTW, there is reg-io-width property.

This is not register I/O width, it's only the width of the counter.

And because of the LiteX CSR bus nature, all registers after this will
be automatically moved if this register's size goes beyond the
alignment (current all LiteX support in Linux mainline assumes 32-bit
alignment, which is also the default configuration).

> 
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - interrupts
> > +  - clocks
> > +  - litex,width
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +     timer@a0006000 {
> 
> Use 4 spaces for example indentation.

Thanks, will change this in the next iteration.

> 
> > +        compatible = "litex,timer";
> > +        reg = <0xa0006000 0x20>;
> > +        clocks = <&sys_clk>;
> > +        interrupts = <17>;
> > +        litex,width = <32>;
> > +     };
> 
> Best regards,
> Krzysztof
> 

[1]
https://github.com/enjoy-digital/litex/blob/master/litex/soc/cores/timer.py