[PATCH 0/2] arm64: renesas: Add RZ/V2M watchdog support

[PATCH 0/2] arm64: renesas: Add RZ/V2M watchdog support

[Phil Edworthy]

Hello all,

This patch series adds support for the Watchdog Timer (WDT) in the
RZ/V2M SoC.

Phil Edworthy (2):
  dt-bindings: watchdog: renesas,wdt: Add r9a09g011 (RZ/V2M) support
  watchdog: rzg2l_wdt: Add rzv2m compatible string

 .../bindings/watchdog/renesas,wdt.yaml        | 63 ++++++++++++-------
 drivers/watchdog/rzg2l_wdt.c                  |  1 +
 2 files changed, 43 insertions(+), 21 deletions(-)

-- 
2.34.1

[PATCH 1/2] dt-bindings: watchdog: renesas,wdt: Add r9a09g011 (RZ/V2M) support

[Phil Edworthy]

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:
+    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>;
-- 
2.34.1

[PATCH 2/2] watchdog: rzg2l_wdt: Add rzv2m compatible string

[Phil Edworthy]

The WDT on RZ/V2M devices is basically the same as RZ/G2L, with the
exception that the RZ/V2M has a single combined interrupt, whereas the
RZ/G2L has time out and error interrupts.

Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
---
 drivers/watchdog/rzg2l_wdt.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/watchdog/rzg2l_wdt.c b/drivers/watchdog/rzg2l_wdt.c
index 6eea0ee4af49..cee3eef7cc47 100644
--- a/drivers/watchdog/rzg2l_wdt.c
+++ b/drivers/watchdog/rzg2l_wdt.c
@@ -256,6 +256,7 @@ static int rzg2l_wdt_probe(struct platform_device *pdev)
 
 static const struct of_device_id rzg2l_wdt_ids[] = {
 	{ .compatible = "renesas,rzg2l-wdt", },
+	{ .compatible = "renesas,rzv2m-wdt", },
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(of, rzg2l_wdt_ids);
-- 
2.34.1

Re: [PATCH 1/2] dt-bindings: watchdog: renesas,wdt: Add r9a09g011 (RZ/V2M) support

[Krzysztof Kozlowski]

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

Re: [PATCH 2/2] watchdog: rzg2l_wdt: Add rzv2m compatible string

[Geert Uytterhoeven]

Hi Phil,

Thanks for your patch!

On Tue, Jun 7, 2022 at 3:59 PM Phil Edworthy <phil.edworthy@renesas.com> wrote:
> The WDT on RZ/V2M devices is basically the same as RZ/G2L, with the
> exception that the RZ/V2M has a single combined interrupt, whereas the
> RZ/G2L has time out and error interrupts.

... and that the RZ/V2M watchdog does not have the parity error
handling.

> Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
> Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>

> --- a/drivers/watchdog/rzg2l_wdt.c
> +++ b/drivers/watchdog/rzg2l_wdt.c
> @@ -256,6 +256,7 @@ static int rzg2l_wdt_probe(struct platform_device *pdev)
>
>  static const struct of_device_id rzg2l_wdt_ids[] = {
>         { .compatible = "renesas,rzg2l-wdt", },
> +       { .compatible = "renesas,rzv2m-wdt", },

Hence this is not sufficient, as you need to prevent writing to the
parity registers on RZ/V2M.

>         { /* sentinel */ }
>  };

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

RE: [PATCH 2/2] watchdog: rzg2l_wdt: Add rzv2m compatible string

[Phil Edworthy]

Hi Geert,

On 09 June 2022 11:11 Geert Uytterhoeven wrote:
> On Tue, Jun 7, 2022 at 3:59 PM Phil wrote:
> > The WDT on RZ/V2M devices is basically the same as RZ/G2L, with the
> > exception that the RZ/V2M has a single combined interrupt, whereas the
> > RZ/G2L has time out and error interrupts.
> 
> ... and that the RZ/V2M watchdog does not have the parity error
> handling.
> 
> > Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
> > Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
> 
> > --- a/drivers/watchdog/rzg2l_wdt.c
> > +++ b/drivers/watchdog/rzg2l_wdt.c
> > @@ -256,6 +256,7 @@ static int rzg2l_wdt_probe(struct platform_device
> *pdev)
> >
> >  static const struct of_device_id rzg2l_wdt_ids[] = {
> >         { .compatible = "renesas,rzg2l-wdt", },
> > +       { .compatible = "renesas,rzv2m-wdt", },
> 
> Hence this is not sufficient, as you need to prevent writing to the
> parity registers on RZ/V2M.

Oops, thanks for the review and noticing this.

Phil

RE: [PATCH 1/2] dt-bindings: watchdog: renesas,wdt: Add r9a09g011 (RZ/V2M) support

[Phil Edworthy]

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

RE: [PATCH 1/2] dt-bindings: watchdog: renesas,wdt: Add r9a09g011 (RZ/V2M) support

[Phil Edworthy]

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

Re: [PATCH 1/2] dt-bindings: watchdog: renesas,wdt: Add r9a09g011 (RZ/V2M) support

[Krzysztof Kozlowski]

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