| Message ID | 20220607135619.174110-2-phil.edworthy@renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | arm64: renesas: Add RZ/V2M watchdog support | expand |
On 07/06/2022 15:56, Phil Edworthy wrote: > Add the documentation for the r9a09g011 SoC, but in doing so also > reorganise the doc to make it easier to read. > Additionally, make the binding require an interrupt to be specified. > Whilst the driver does not need an interrupt, all of the SoCs that use > this binding actually provide one. > > Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com> > Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com> > --- > .../bindings/watchdog/renesas,wdt.yaml | 63 ++++++++++++------- > 1 file changed, 42 insertions(+), 21 deletions(-) > > diff --git a/Documentation/devicetree/bindings/watchdog/renesas,wdt.yaml b/Documentation/devicetree/bindings/watchdog/renesas,wdt.yaml > index a8d7dde5271b..6473734921e3 100644 > --- a/Documentation/devicetree/bindings/watchdog/renesas,wdt.yaml > +++ b/Documentation/devicetree/bindings/watchdog/renesas,wdt.yaml > @@ -31,6 +31,11 @@ properties: > - renesas,r9a07g054-wdt # RZ/V2L > - const: renesas,rzg2l-wdt > > + - items: > + - enum: > + - renesas,r9a09g011-wdt # RZ/V2M > + - const: renesas,rzv2m-wdt # RZ/V2M > + > - items: > - enum: > - renesas,r8a7742-wdt # RZ/G1H > @@ -70,13 +75,27 @@ properties: > reg: > maxItems: 1 > > - interrupts: true > + interrupts: > + minItems: 1 > + items: > + - description: Timeout > + - description: Parity error > > - interrupt-names: true > + interrupt-names: This also needs minItems > + items: > + - const: wdt > + - const: perrout > > - clocks: true > + clocks: > + minItems: 1 > + items: > + - description: Register access clock > + - description: Main clock > > - clock-names: true > + clock-names: Ditto > + items: > + - const: pclk > + - const: oscclk > Best regards, Krzysztof
Hi Krzysztof, Thanks for your review. On 08 June 2022 11:52 Krzysztof Kozlowski wrote: > On 07/06/2022 15:56, Phil Edworthy wrote: > > Add the documentation for the r9a09g011 SoC, but in doing so also > > reorganise the doc to make it easier to read. > > Additionally, make the binding require an interrupt to be specified. > > Whilst the driver does not need an interrupt, all of the SoCs that use > > this binding actually provide one. > > > > Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com> > > Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com> > > --- > > .../bindings/watchdog/renesas,wdt.yaml | 63 ++++++++++++------- > > 1 file changed, 42 insertions(+), 21 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/watchdog/renesas,wdt.yaml > b/Documentation/devicetree/bindings/watchdog/renesas,wdt.yaml > > index a8d7dde5271b..6473734921e3 100644 > > --- a/Documentation/devicetree/bindings/watchdog/renesas,wdt.yaml > > +++ b/Documentation/devicetree/bindings/watchdog/renesas,wdt.yaml > > @@ -31,6 +31,11 @@ properties: > > - renesas,r9a07g054-wdt # RZ/V2L > > - const: renesas,rzg2l-wdt > > > > + - items: > > + - enum: > > + - renesas,r9a09g011-wdt # RZ/V2M > > + - const: renesas,rzv2m-wdt # RZ/V2M > > + > > - items: > > - enum: > > - renesas,r8a7742-wdt # RZ/G1H > > @@ -70,13 +75,27 @@ properties: > > reg: > > maxItems: 1 > > > > - interrupts: true > > + interrupts: > > + minItems: 1 > > + items: > > + - description: Timeout > > + - description: Parity error > > > > - interrupt-names: true > > + interrupt-names: > > This also needs minItems I left minItems off for interrupt-names and clock-names on the basis that they are only needed if you have more than one interrupt or clock. After adding the lines you suggested (minItems: 1), I find that 'make dtbs_check' passes even if there are no interrupt-names or clock-names specified. Is this expected? minItems: 0 makes more sense to me, but it is required to be greater than or equal 1 Thanks Phil
Hi Krzysztof, On 10 June 2022 15:38 Phil Edworthy wrote: > On 08 June 2022 11:52 Krzysztof Kozlowski wrote: > > On 07/06/2022 15:56, Phil Edworthy wrote: > > > Add the documentation for the r9a09g011 SoC, but in doing so also > > > reorganise the doc to make it easier to read. > > > Additionally, make the binding require an interrupt to be specified. > > > Whilst the driver does not need an interrupt, all of the SoCs that use > > > this binding actually provide one. > > > > > > Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com> > > > Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com> > > > --- > > > .../bindings/watchdog/renesas,wdt.yaml | 63 ++++++++++++------ > - > > > 1 file changed, 42 insertions(+), 21 deletions(-) > > > > > > diff --git > a/Documentation/devicetree/bindings/watchdog/renesas,wdt.yaml > > b/Documentation/devicetree/bindings/watchdog/renesas,wdt.yaml > > > index a8d7dde5271b..6473734921e3 100644 > > > --- a/Documentation/devicetree/bindings/watchdog/renesas,wdt.yaml > > > +++ b/Documentation/devicetree/bindings/watchdog/renesas,wdt.yaml > > > @@ -31,6 +31,11 @@ properties: > > > - renesas,r9a07g054-wdt # RZ/V2L > > > - const: renesas,rzg2l-wdt > > > > > > + - items: > > > + - enum: > > > + - renesas,r9a09g011-wdt # RZ/V2M > > > + - const: renesas,rzv2m-wdt # RZ/V2M > > > + > > > - items: > > > - enum: > > > - renesas,r8a7742-wdt # RZ/G1H > > > @@ -70,13 +75,27 @@ properties: > > > reg: > > > maxItems: 1 > > > > > > - interrupts: true > > > + interrupts: > > > + minItems: 1 > > > + items: > > > + - description: Timeout > > > + - description: Parity error > > > > > > - interrupt-names: true > > > + interrupt-names: > > > > This also needs minItems > I left minItems off for interrupt-names and clock-names on the basis that > they are only needed if you have more than one interrupt or clock. > > After adding the lines you suggested (minItems: 1), I find that > 'make dtbs_check' passes even if there are no interrupt-names or > clock-names specified. Is this expected? > > minItems: 0 makes more sense to me, but it is required to be greater than > or equal 1 Immediately after sending this I realised that minItems: 1 is correct as interrupt-names and clock-names are _not_ in required. So when they are specified, the minimum is 1. Sorry for the noise. Phil
On 10/06/2022 16:38, Phil Edworthy wrote: > Hi Krzysztof, > > Thanks for your review. > > On 08 June 2022 11:52 Krzysztof Kozlowski wrote: >> On 07/06/2022 15:56, Phil Edworthy wrote: >>> Add the documentation for the r9a09g011 SoC, but in doing so also >>> reorganise the doc to make it easier to read. >>> Additionally, make the binding require an interrupt to be specified. >>> Whilst the driver does not need an interrupt, all of the SoCs that use >>> this binding actually provide one. >>> >>> Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com> >>> Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com> >>> --- >>> .../bindings/watchdog/renesas,wdt.yaml | 63 ++++++++++++------- >>> 1 file changed, 42 insertions(+), 21 deletions(-) >>> >>> diff --git a/Documentation/devicetree/bindings/watchdog/renesas,wdt.yaml >> b/Documentation/devicetree/bindings/watchdog/renesas,wdt.yaml >>> index a8d7dde5271b..6473734921e3 100644 >>> --- a/Documentation/devicetree/bindings/watchdog/renesas,wdt.yaml >>> +++ b/Documentation/devicetree/bindings/watchdog/renesas,wdt.yaml >>> @@ -31,6 +31,11 @@ properties: >>> - renesas,r9a07g054-wdt # RZ/V2L >>> - const: renesas,rzg2l-wdt >>> >>> + - items: >>> + - enum: >>> + - renesas,r9a09g011-wdt # RZ/V2M >>> + - const: renesas,rzv2m-wdt # RZ/V2M >>> + >>> - items: >>> - enum: >>> - renesas,r8a7742-wdt # RZ/G1H >>> @@ -70,13 +75,27 @@ properties: >>> reg: >>> maxItems: 1 >>> >>> - interrupts: true >>> + interrupts: >>> + minItems: 1 >>> + items: >>> + - description: Timeout >>> + - description: Parity error >>> >>> - interrupt-names: true >>> + interrupt-names: >> >> This also needs minItems > I left minItems off for interrupt-names and clock-names on the basis that > they are only needed if you have more than one interrupt or clock. True, but now you disallow them for one clock/interrupt cases in other variants. Although after looking at existing bindings - it's even messier there. For certain variants it is just ":true" which is not correct. In general, the properties in "properties:" section should have constraints - the most wide. These are narrowed for specific variants or even disallowed for some. Old bindings allowed anything for some variants, like 20 interrupt names so clearly wrong. > > After adding the lines you suggested (minItems: 1), I find that > 'make dtbs_check' passes even if there are no interrupt-names or > clock-names specified. Is this expected? These are not required, aren't they? If they are not required, they can be missing... > > minItems: 0 makes more sense to me, but it is required to be greater than > or equal 1 > > Thanks > Phil Best regards, Krzysztof
diff --git a/Documentation/devicetree/bindings/watchdog/renesas,wdt.yaml b/Documentation/devicetree/bindings/watchdog/renesas,wdt.yaml index a8d7dde5271b..6473734921e3 100644 --- a/Documentation/devicetree/bindings/watchdog/renesas,wdt.yaml +++ b/Documentation/devicetree/bindings/watchdog/renesas,wdt.yaml @@ -31,6 +31,11 @@ properties: - renesas,r9a07g054-wdt # RZ/V2L - const: renesas,rzg2l-wdt + - items: + - enum: + - renesas,r9a09g011-wdt # RZ/V2M + - const: renesas,rzv2m-wdt # RZ/V2M + - items: - enum: - renesas,r8a7742-wdt # RZ/G1H @@ -70,13 +75,27 @@ properties: reg: maxItems: 1 - interrupts: true + interrupts: + minItems: 1 + items: + - description: Timeout + - description: Parity error - interrupt-names: true + interrupt-names: + items: + - const: wdt + - const: perrout - clocks: true + clocks: + minItems: 1 + items: + - description: Register access clock + - description: Main clock - clock-names: true + clock-names: + items: + - const: pclk + - const: oscclk power-domains: maxItems: 1 @@ -89,6 +108,7 @@ properties: required: - compatible - reg + - interrupts - clocks allOf: @@ -112,32 +132,31 @@ allOf: compatible: contains: enum: + - renesas,rzv2m-wdt - renesas,rzg2l-wdt then: properties: - interrupts: - maxItems: 2 - interrupt-names: - items: - - const: wdt - - const: perrout clocks: - items: - - description: Register access clock - - description: Main clock + minItems: 2 clock-names: - items: - - const: pclk - - const: oscclk + minItems: 2 required: - clock-names - - interrupt-names - else: + + - if: + properties: + compatible: + contains: + enum: + - renesas,rzg2l-wdt + then: properties: interrupts: - maxItems: 1 - clocks: - maxItems: 1 + minItems: 2 + interrupt-names: + minItems: 2 + required: + - interrupt-names additionalProperties: false @@ -145,9 +164,11 @@ examples: - | #include <dt-bindings/clock/r8a7795-cpg-mssr.h> #include <dt-bindings/power/r8a7795-sysc.h> + #include <dt-bindings/interrupt-controller/arm-gic.h> wdt0: watchdog@e6020000 { compatible = "renesas,r8a7795-wdt", "renesas,rcar-gen3-wdt"; reg = <0xe6020000 0x0c>; + interrupts = <GIC_SPI 140 IRQ_TYPE_LEVEL_HIGH>; clocks = <&cpg CPG_MOD 402>; power-domains = <&sysc R8A7795_PD_ALWAYS_ON>; resets = <&cpg 402>;