[PATCH v4 0/1] Add StarFive JH8100 watchdog

[PATCH v4 0/1] Add StarFive JH8100 watchdog

[Ji Sheng Teoh]

Changes since v3:
- Drop items in compatible field.
- Replace items with maxItems in reset field.
- Replace maxItems with items: -description in if else reset field

Changes since v2:
- Express JH8100 compatibility to JH7110 in dt-bindings.
- Rework min/maxItems constraint for JH8100 resets property.

Changes since v1:
- Drop "starfive,jh8100-wdt" compatible field in starfive-wdt.c,
  and express them in dt-bindings.
- Use minItems in resets field to cater for single reset signal
  in JH8100.
- Reword Watchdog reset to Core reset for JH8100.

StarFive's JH8100 watchdog reuses JH7100 register mapping.
DT-binding of JH7100 watchdog is extended to support JH8100.
Since JH8100 only uses 1 reset signal, update the binding to
support one reset for "starfive,jh8100-wdt" compatible.

Ji Sheng Teoh (1):
  dt-bindings: watchdog: starfive,jh7100-wdt: Add compatible for JH8100

 .../watchdog/starfive,jh7100-wdt.yaml         | 29 +++++++++++++++----
 1 file changed, 23 insertions(+), 6 deletions(-)

-- 
2.25.1

[PATCH v4 1/1] dt-bindings: watchdog: starfive,jh7100-wdt: Add compatible for JH8100

[Ji Sheng Teoh]

Add "starfive,jh8100-wdt" compatible string for StarFive's JH8100
watchdog.
Since JH8100 watchdog only has 1 reset signal, update binding
document to support one reset for "starfive,jh8100-wdt" compatible.

Signed-off-by: Ley Foon Tan <leyfoon.tan@starfivetech.com>
Signed-off-by: Ji Sheng Teoh <jisheng.teoh@starfivetech.com>
---
 .../watchdog/starfive,jh7100-wdt.yaml         | 29 +++++++++++++++----
 1 file changed, 23 insertions(+), 6 deletions(-)

diff --git a/Documentation/devicetree/bindings/watchdog/starfive,jh7100-wdt.yaml b/Documentation/devicetree/bindings/watchdog/starfive,jh7100-wdt.yaml
index 68f3f6fd08a6..ab077f64a83e 100644
--- a/Documentation/devicetree/bindings/watchdog/starfive,jh7100-wdt.yaml
+++ b/Documentation/devicetree/bindings/watchdog/starfive,jh7100-wdt.yaml
@@ -19,14 +19,12 @@ description:
   isn't cleared, the watchdog will reset the system unless the watchdog
   reset is disabled.
 
-allOf:
-  - $ref: watchdog.yaml#
-
 properties:
   compatible:
     enum:
       - starfive,jh7100-wdt
       - starfive,jh7110-wdt
+      - starfive,jh8100-wdt
 
   reg:
     maxItems: 1
@@ -45,9 +43,7 @@ properties:
       - const: core
 
   resets:
-    items:
-      - description: APB reset
-      - description: Core reset
+    maxItems: 2
 
 required:
   - compatible
@@ -56,6 +52,27 @@ required:
   - clock-names
   - resets
 
+allOf:
+  - $ref: watchdog.yaml#
+
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - starfive,jh8100-wdt
+    then:
+      properties:
+        resets:
+          items:
+            - description: Core reset
+    else:
+      properties:
+        resets:
+          items:
+            - description: APB reset
+            - description: Core reset
+
 unevaluatedProperties: false
 
 examples:
-- 
2.25.1

Re: [PATCH v4 1/1] dt-bindings: watchdog: starfive,jh7100-wdt: Add compatible for JH8100

[Krzysztof Kozlowski]

On 16/12/2023 02:48, Ji Sheng Teoh wrote:
> Add "starfive,jh8100-wdt" compatible string for StarFive's JH8100
> watchdog.
> Since JH8100 watchdog only has 1 reset signal, update binding
> document to support one reset for "starfive,jh8100-wdt" compatible.
> 
> Signed-off-by: Ley Foon Tan <leyfoon.tan@starfivetech.com>
> Signed-off-by: Ji Sheng Teoh <jisheng.teoh@starfivetech.com>
> ---
>  .../watchdog/starfive,jh7100-wdt.yaml         | 29 +++++++++++++++----
>  1 file changed, 23 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/watchdog/starfive,jh7100-wdt.yaml b/Documentation/devicetree/bindings/watchdog/starfive,jh7100-wdt.yaml
> index 68f3f6fd08a6..ab077f64a83e 100644
> --- a/Documentation/devicetree/bindings/watchdog/starfive,jh7100-wdt.yaml
> +++ b/Documentation/devicetree/bindings/watchdog/starfive,jh7100-wdt.yaml
> @@ -19,14 +19,12 @@ description:
>    isn't cleared, the watchdog will reset the system unless the watchdog
>    reset is disabled.
>  
> -allOf:
> -  - $ref: watchdog.yaml#
> -
>  properties:
>    compatible:
>      enum:
>        - starfive,jh7100-wdt
>        - starfive,jh7110-wdt
> +      - starfive,jh8100-wdt

What is happening with this patchset? I asked about one specific items.
you know - comment is written under specific inline quopte.
You wrote in changelog "Drop items in compatible field.", but I see
oneOf gone!

I have real doubts that you ever tested your entire solution with this
binding. Where is the DTS?

Best regards,
Krzysztof

Re: [PATCH v4 1/1] dt-bindings: watchdog: starfive,jh7100-wdt: Add compatible for JH8100

[Ji Sheng Teoh]

On Mon, 18 Dec 2023 09:41:07 +0100
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:

> On 16/12/2023 02:48, Ji Sheng Teoh wrote:
> > Add "starfive,jh8100-wdt" compatible string for StarFive's JH8100
> > watchdog.
> > Since JH8100 watchdog only has 1 reset signal, update binding
> > document to support one reset for "starfive,jh8100-wdt" compatible.
> > 
> > Signed-off-by: Ley Foon Tan <leyfoon.tan@starfivetech.com>
> > Signed-off-by: Ji Sheng Teoh <jisheng.teoh@starfivetech.com>
> > ---
> >  .../watchdog/starfive,jh7100-wdt.yaml         | 29
> > +++++++++++++++---- 1 file changed, 23 insertions(+), 6 deletions(-)
> > 
> > diff --git
> > a/Documentation/devicetree/bindings/watchdog/starfive,jh7100-wdt.yaml
> > b/Documentation/devicetree/bindings/watchdog/starfive,jh7100-wdt.yaml
> > index 68f3f6fd08a6..ab077f64a83e 100644 ---
> > a/Documentation/devicetree/bindings/watchdog/starfive,jh7100-wdt.yaml
> > +++
> > b/Documentation/devicetree/bindings/watchdog/starfive,jh7100-wdt.yaml
> > @@ -19,14 +19,12 @@ description: isn't cleared, the watchdog will
> > reset the system unless the watchdog reset is disabled. -allOf:
> > -  - $ref: watchdog.yaml#
> > -
> >  properties:
> >    compatible:
> >      enum:
> >        - starfive,jh7100-wdt
> >        - starfive,jh7110-wdt
> > +      - starfive,jh8100-wdt  
> 
> What is happening with this patchset? I asked about one specific
> items. you know - comment is written under specific inline quopte.
> You wrote in changelog "Drop items in compatible field.", but I see
> oneOf gone!

My bad, I've interpreted it wrongly. 
Will rework the compatible field to this instead:

  compatible:
    oneOf:
      - enum:
          - starfive,jh7100-wdt
          - starfive,jh7110-wdt
      - items:
          - enum:
              - starfive,jh8100-wdt
          - const: starfive,jh7110-wdt


While reworking this, I've noticed reset field with maxItems alone will
cause dt_binding_check to fail with following error:
'watchdog@12270000: resets: [[4294967295, 15]] is too short'
This is fixed by defining minItems and maxItems as follow:

  resets:
    minItems: 1
    maxItems: 2

> 
> I have real doubts that you ever tested your entire solution with this
> binding. Where is the DTS?
> 

Currently, the DTS is still in internal and yet to upstream as it depends
on [1].
[1]
https://lore.kernel.org/all/20231201121410.95298-1-jeeheng.sia@starfivetech.com/

> Best regards,
> Krzysztof
> 

Re: [PATCH v4 1/1] dt-bindings: watchdog: starfive,jh7100-wdt: Add compatible for JH8100

[Krzysztof Kozlowski]

On 18/12/2023 15:27, Ji Sheng Teoh wrote:
>>
>> I have real doubts that you ever tested your entire solution with this
>> binding. Where is the DTS?
>>
> 
> Currently, the DTS is still in internal and yet to upstream as it depends
> on [1].

Yeah, so you send untested code which cannot work or pass tests.  If you
do not test your code, we need to be able to at least verify it, so send
your DTS. Otherwise I cannot trust that this works at all.

Best regards,
Krzysztof

Re: [PATCH v4 1/1] dt-bindings: watchdog: starfive,jh7100-wdt: Add compatible for JH8100

[Ji Sheng Teoh]

On Mon, 18 Dec 2023 15:41:37 +0100
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:

> On 18/12/2023 15:27, Ji Sheng Teoh wrote:
> >>
> >> I have real doubts that you ever tested your entire solution with
> >> this binding. Where is the DTS?
> >>  
> > 
> > Currently, the DTS is still in internal and yet to upstream as it
> > depends on [1].  
> 
> Yeah, so you send untested code which cannot work or pass tests.  If
> you do not test your code, we need to be able to at least verify it,
> so send your DTS. Otherwise I cannot trust that this works at all.
>
Will submit it with DTS once things have cleared up.
Thanks for the comment. 

> Best regards,
> Krzysztof
> 

Re: [PATCH v4 1/1] dt-bindings: watchdog: starfive,jh7100-wdt: Add compatible for JH8100

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1054 bytes --]

On Mon, Dec 18, 2023 at 11:37:38PM +0800, Ji Sheng Teoh wrote:
> On Mon, 18 Dec 2023 15:41:37 +0100
> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
> 
> > On 18/12/2023 15:27, Ji Sheng Teoh wrote:
> > >>
> > >> I have real doubts that you ever tested your entire solution with
> > >> this binding. Where is the DTS?
> > >>  
> > > 
> > > Currently, the DTS is still in internal and yet to upstream as it
> > > depends on [1].  
> > 
> > Yeah, so you send untested code which cannot work or pass tests.  If
> > you do not test your code, we need to be able to at least verify it,
> > so send your DTS. Otherwise I cannot trust that this works at all.
> >
> Will submit it with DTS once things have cleared up.
> Thanks for the comment. 

[1] is not going to applied for a while since the SoC doesn't actually
exist yet and is pre-tapeout on an FPGA. I would just send the dts patch
adding the watchdog alongside the series, or else you'll be waiting for
quite a while. Or even link to the node on github or whatever.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v4 1/1] dt-bindings: watchdog: starfive,jh7100-wdt: Add compatible for JH8100

[Ji Sheng Teoh]

On Tue, 19 Dec 2023 15:43:07 +0000
Conor Dooley <conor@kernel.org> wrote:

> On Mon, Dec 18, 2023 at 11:37:38PM +0800, Ji Sheng Teoh wrote:
> > On Mon, 18 Dec 2023 15:41:37 +0100
> > Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
> >   
> > > On 18/12/2023 15:27, Ji Sheng Teoh wrote:  
> > > >>
> > > >> I have real doubts that you ever tested your entire solution
> > > >> with this binding. Where is the DTS?
> > > >>    
> > > > 
> > > > Currently, the DTS is still in internal and yet to upstream as
> > > > it depends on [1].    
> > > 
> > > Yeah, so you send untested code which cannot work or pass tests.
> > > If you do not test your code, we need to be able to at least
> > > verify it, so send your DTS. Otherwise I cannot trust that this
> > > works at all. 
> > Will submit it with DTS once things have cleared up.
> > Thanks for the comment.   
> 
> [1] is not going to applied for a while since the SoC doesn't actually
> exist yet and is pre-tapeout on an FPGA. I would just send the dts
> patch adding the watchdog alongside the series, or else you'll be
> waiting for quite a while. Or even link to the node on github or
> whatever.
> 

Ok, will add the watchdog dts alongside this series and mention its
dependency on [1]. Thanks.