| Message ID | 20240627043317.3751996-6-chris.packham@alliedtelesis.co.nz |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | mips: Support for RTL9302C | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
On 27/06/2024 06:33, Chris Packham wrote: > Add the devicetree schema for the realtek,otto-timer present on a number > of Realtek SoCs. > > Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Best regards, Krzysztof
Hi Chris, Thanks for submitting these patches! On Thu, 2024-06-27 at 16:33 +1200, Chris Packham wrote: > Add the devicetree schema for the realtek,otto-timer present on a number > of Realtek SoCs. > > Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz> > --- [...] > + > + reg: > + items: > + - description: timer0 registers > + - description: timer1 registers > + - description: timer2 registers > + - description: timer3 registers > + - description: timer4 registers > + > + clocks: > + maxItems: 1 > + > + interrupts: > + items: > + - description: timer0 interrupt > + - description: timer1 interrupt > + - description: timer2 interrupt > + - description: timer3 interrupt > + - description: timer4 interrupt Instead of providing a (SoC dependent) number of reg and interrupt items, can't we just provide one reg+interrupt per timer and instantiate one block for however many timers the SoC has? > + > +additionalProperties: false > + > +examples: > + - | > + timer@3200 { > + compatible = "realtek,rtl9302-timer", "realtek,otto-timer"; > + reg = <0x3200 0x10>, <0x3210 0x10>, <0x3220 0x10>, > + <0x3230 0x10>, <0x3240 0x10>; > + > + interrupt-parent = <&intc>; > + interrupts = <7>, <8>, <9>, <10>, <11>; > + clocks = <&lx_clk>; > + }; So this would become: timer@3200 { compatible = ... reg = <0x3200 0x10>; interrupt-parent = <&intc>; interrupts = <7>; ... }; timer@3210 { compatible = ... reg = <0x3210 0x10>; interrupt-parent = <&intc>; interrupts = <8>; ... }; ... More verbose, but it also makes the binding a bit simpler. The driver can then still do whatever it wants with all the timers that are registered, although some more resource locking might be required. Best, Sander
On 30/06/24 08:40, Sander Vanheule wrote: > Hi Chris, > > Thanks for submitting these patches! > > On Thu, 2024-06-27 at 16:33 +1200, Chris Packham wrote: >> Add the devicetree schema for the realtek,otto-timer present on a number >> of Realtek SoCs. >> >> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz> >> --- > [...] > >> + >> + reg: >> + items: >> + - description: timer0 registers >> + - description: timer1 registers >> + - description: timer2 registers >> + - description: timer3 registers >> + - description: timer4 registers >> + >> + clocks: >> + maxItems: 1 >> + >> + interrupts: >> + items: >> + - description: timer0 interrupt >> + - description: timer1 interrupt >> + - description: timer2 interrupt >> + - description: timer3 interrupt >> + - description: timer4 interrupt > Instead of providing a (SoC dependent) number of reg and interrupt items, can't we just > provide one reg+interrupt per timer and instantiate one block for however many timers the > SoC has? > >> + >> +additionalProperties: false >> + >> +examples: >> + - | >> + timer@3200 { >> + compatible = "realtek,rtl9302-timer", "realtek,otto-timer"; >> + reg = <0x3200 0x10>, <0x3210 0x10>, <0x3220 0x10>, >> + <0x3230 0x10>, <0x3240 0x10>; >> + >> + interrupt-parent = <&intc>; >> + interrupts = <7>, <8>, <9>, <10>, <11>; >> + clocks = <&lx_clk>; >> + }; > So this would become: > timer@3200 { > compatible = ... > reg = <0x3200 0x10>; > interrupt-parent = <&intc>; > interrupts = <7>; > ... > }; > timer@3210 { > compatible = ... > reg = <0x3210 0x10>; > interrupt-parent = <&intc>; > interrupts = <8>; > ... > }; > ... > > More verbose, but it also makes the binding a bit simpler. The driver can then still do > whatever it wants with all the timers that are registered, although some more resource > locking might be required. I kind of prefer the single entry for the whole TCU. If we were to fold the watchdog into this then we could have a single larger range that covered all the timers similar to the ingenic,tcu. But that would technically be a breaking change at this point.
diff --git a/Documentation/devicetree/bindings/timer/realtek,otto-timer.yaml b/Documentation/devicetree/bindings/timer/realtek,otto-timer.yaml new file mode 100644 index 000000000000..7b6ec2c69484 --- /dev/null +++ b/Documentation/devicetree/bindings/timer/realtek,otto-timer.yaml @@ -0,0 +1,63 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/timer/realtek,otto-timer.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Realtek Otto SoCs Timer/Counter + +description: + Realtek SoCs support a number of timers/counters. These are used + as a per CPU clock event generator and an overall CPU clocksource. + +maintainers: + - Chris Packham <chris.packham@alliedtelesis.co.nz> + +properties: + $nodename: + pattern: "^timer@[0-9a-f]+$" + + compatible: + items: + - enum: + - realtek,rtl9302-timer + - const: realtek,otto-timer + + reg: + items: + - description: timer0 registers + - description: timer1 registers + - description: timer2 registers + - description: timer3 registers + - description: timer4 registers + + clocks: + maxItems: 1 + + interrupts: + items: + - description: timer0 interrupt + - description: timer1 interrupt + - description: timer2 interrupt + - description: timer3 interrupt + - description: timer4 interrupt + +required: + - compatible + - reg + - clocks + - interrupts + +additionalProperties: false + +examples: + - | + timer@3200 { + compatible = "realtek,rtl9302-timer", "realtek,otto-timer"; + reg = <0x3200 0x10>, <0x3210 0x10>, <0x3220 0x10>, + <0x3230 0x10>, <0x3240 0x10>; + + interrupt-parent = <&intc>; + interrupts = <7>, <8>, <9>, <10>, <11>; + clocks = <&lx_clk>; + };
Add the devicetree schema for the realtek,otto-timer present on a number of Realtek SoCs. Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz> --- Notes: Changes in v3: - Use items to describe regs and interrupt properties - Remove minItems condition Changes in v2: - Use specific compatible (rtl9302-timer instead of rtl930x-timer) - Remove unnecessary label - Remove unused irq flags (interrupt controller is one-cell) - Set minItems for reg and interrupts based on compatible .../bindings/timer/realtek,otto-timer.yaml | 63 +++++++++++++++++++ 1 file changed, 63 insertions(+) create mode 100644 Documentation/devicetree/bindings/timer/realtek,otto-timer.yaml