[v5,06/11] dt-bindings: watchdog: Add HPE GXP Watchdog timer binding

Commit Message

Hawkins, Nick April 21, 2022, 7:21 p.m. UTC

From: Nick Hawkins <[e-mail]>

Add the hpe gxp watchdog timer binding hpe,gxp-wdt.
This will enable support for the HPE GXP Watchdog.

Signed-off-by: Nick Hawkins <[e-mail]>

---
v5:
* Fixed version log
v4:
* Made watchdog a child of timer because of same register
  area based on review feedback
* Simplified the watchdog yaml as it will get information
  from parent device
v3:
* Used proper patchset format.
v2:
* Converted from txt to yaml
---
 .../bindings/watchdog/hpe,gxp-wdt.yaml        | 30 +++++++++++++++++++
 1 file changed, 30 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/watchdog/hpe,gxp-wdt.yaml

Comments

Krzysztof Kozlowski April 23, 2022, 10:52 a.m. UTC | #1

On 21/04/2022 21:21, [e-mail] wrote:
> ---
>  .../bindings/watchdog/hpe,gxp-wdt.yaml        | 30 +++++++++++++++++++
>  1 file changed, 30 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/watchdog/hpe,gxp-wdt.yaml
> 
> diff --git a/Documentation/devicetree/bindings/watchdog/hpe,gxp-wdt.yaml b/Documentation/devicetree/bindings/watchdog/hpe,gxp-wdt.yaml
> new file mode 100644
> index 000000000000..c20da146352f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/watchdog/hpe,gxp-wdt.yaml
> @@ -0,0 +1,30 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/watchdog/hpe,gxp-wdt.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: HPE GXP Controlled Watchdog
> +
> +allOf:
> +  - $ref: "watchdog.yaml#"

allOf goes after maintainers, before properties.

> +
> +maintainers:
> +  - Nick Hawkins <[e-mail]>
> +  - Jean-Marie Verdun <[e-mail]>
> +
> +properties:
> +  compatible:
> +    const: hpe,gxp-wdt
> +
> +required:
> +  - compatible
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    watchdog0:  watchdog {

Doubled space.

> +      compatible = "hpe,gxp-wdt";
> +    };
> +


Best regards,
Krzysztof

Rob Herring (Arm) April 25, 2022, 10:04 p.m. UTC | #2

On Thu, Apr 21, 2022 at 02:21:27PM -0500, [e-mail] wrote:
> From: Nick Hawkins <[e-mail]>
> 
> Add the hpe gxp watchdog timer binding hpe,gxp-wdt.
> This will enable support for the HPE GXP Watchdog.
> 
> Signed-off-by: Nick Hawkins <[e-mail]>
> 
> ---
> v5:
> * Fixed version log
> v4:
> * Made watchdog a child of timer because of same register
>   area based on review feedback
> * Simplified the watchdog yaml as it will get information
>   from parent device
> v3:
> * Used proper patchset format.
> v2:
> * Converted from txt to yaml
> ---
>  .../bindings/watchdog/hpe,gxp-wdt.yaml        | 30 +++++++++++++++++++
>  1 file changed, 30 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/watchdog/hpe,gxp-wdt.yaml
> 
> diff --git a/Documentation/devicetree/bindings/watchdog/hpe,gxp-wdt.yaml b/Documentation/devicetree/bindings/watchdog/hpe,gxp-wdt.yaml
> new file mode 100644
> index 000000000000..c20da146352f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/watchdog/hpe,gxp-wdt.yaml
> @@ -0,0 +1,30 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/watchdog/hpe,gxp-wdt.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: HPE GXP Controlled Watchdog
> +
> +allOf:
> +  - $ref: "watchdog.yaml#"
> +
> +maintainers:
> +  - Nick Hawkins <[e-mail]>
> +  - Jean-Marie Verdun <[e-mail]>
> +
> +properties:
> +  compatible:
> +    const: hpe,gxp-wdt
> +
> +required:
> +  - compatible
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    watchdog0:  watchdog {
> +      compatible = "hpe,gxp-wdt";

How is this h/w controlled? I'm guessing it's part of the timer? If so, 
you don't need this node. A single node can implement multiple 
functions.

Rob

Hawkins, Nick April 26, 2022, 1:21 p.m. UTC | #3

-----Original Message-----
From: Rob Herring [mailto:[e-mail]] 
Sent: Monday, April 25, 2022 5:05 PM
To: Hawkins, Nick <[e-mail]>
Cc: Verdun, Jean-Marie <[e-mail]>; [e-mail]; [e-mail]; [e-mail]; Wim Van Sebroeck <[e-mail]>; Guenter Roeck <[e-mail]>; Krzysztof Kozlowski <[e-mail]>; [e-mail]; [e-mail]; [e-mail]
Subject: Re: [PATCH v5 06/11] dt-bindings: watchdog: Add HPE GXP Watchdog timer binding

(...)

> How is this h/w controlled? I'm guessing it's part of the timer? If so, you don't need this node. A single node can implement multiple functions.

It is associated with the timer because of the shared register set. Based on feedback from Krzysztof I need to create a child node for gxp-timer. I therefore will remove this file and move gxp-wdt to the hpe,gxp-timer.yaml as a child node.

Thank you for the feedback.

-Nick Hawkins

Krzysztof Kozlowski April 26, 2022, 1:34 p.m. UTC | #4

On 26/04/2022 15:21, Hawkins, Nick wrote:
>> How is this h/w controlled? I'm guessing it's part of the timer? If so, you don't need this node. A single node can implement multiple functions.
> 
> It is associated with the timer because of the shared register set. Based on feedback from Krzysztof I need to create a child node for gxp-timer. I therefore will remove this file and move gxp-wdt to the hpe,gxp-timer.yaml as a child node.

I have impression my feedback was about mapping entire address space,
not few registers of watchdog:
https://lore.kernel.org/all/[e-mail]/

However later during talks it turned out that the address space is
heavily shared.

Nick,
BTW, do you see my comments in the email I linked above? This v5 makes
the same mistake. We talked about this already, so please make note of
comments you receive and implement them.

Best regards,
Krzysztof

Hawkins, Nick April 26, 2022, 1:52 p.m. UTC | #5

-----Original Message-----
From: Krzysztof Kozlowski [mailto:[e-mail]] 
Sent: Tuesday, April 26, 2022 8:35 AM
To: Hawkins, Nick <[e-mail]>; Rob Herring <[e-mail]>
Cc: Verdun, Jean-Marie <[e-mail]>; [e-mail]; [e-mail]; [e-mail]; Wim Van Sebroeck <[e-mail]>; Guenter Roeck <[e-mail]>; Krzysztof Kozlowski <[e-mail]>; [e-mail]; [e-mail]; [e-mail]
Subject: Re: [PATCH v5 06/11] dt-bindings: watchdog: Add HPE GXP Watchdog timer binding

On 26/04/2022 15:21, Hawkins, Nick wrote:
>>> How is this h/w controlled? I'm guessing it's part of the timer? If so, you don't need this node. A single node can implement multiple functions.
>> 
>> It is associated with the timer because of the shared register set. Based on feedback from Krzysztof I need to create a child node for gxp-timer. I therefore will remove this file and move gxp-wdt to the hpe,gxp-timer.yaml as a child node.

>I have impression my feedback was about mapping entire address space, not few registers of watchdog:
>https://lore.kernel.org/all/[e-mail]/

>However later during talks it turned out that the address space is heavily shared.

>Nick,
>BTW, do you see my comments in the email I linked above? This v5 makes the same mistake. We talked about this already, so please make note of comments you receive and implement them.

Krzysztof,

Apologies, I did miss the comment about the double spacing around the label and the label not being necessary. I will not make this mistake again. I became focused about the comment of mapping an entire register space which indirectly lead me on to the path which I am now having the gxp-timer have the gxp-wdt as a child. To be specific the feedback I was speaking of above was about the gxp-timer which is here: https://lore.kernel.org/all/[e-mail]/ That is the children must be defined for a simple-mfd device. Hence the plan I have now is to remove the hpe,gxp-wdt.yaml entirely and include it in the hpe,gxp-timer.yaml. I assume that is the correct thing to do?

Thank you for your time and feedback,

-Nick

Krzysztof Kozlowski April 26, 2022, 3:44 p.m. UTC | #6

On 26/04/2022 15:52, Hawkins, Nick wrote:
> Apologies, I did miss the comment about the double spacing around the label and the label not being necessary. I will not make this mistake again. I became focused about the comment of mapping an entire register space which indirectly lead me on to the path which I am now having the gxp-timer have the gxp-wdt as a child. To be specific the feedback I was speaking of above was about the gxp-timer which is here: https://lore.kernel.org/all/[e-mail]/ That is the children must be defined for a simple-mfd device. 

This was comment for this v5, not for previous patches. In this v5, you
have a child of timer, so it has to be defined in timer schema.

This was not a comment whether a child should exist or should not. It
was made under the assumption that you want to have a child node.

> Hence the plan I have now is to remove the hpe,gxp-wdt.yaml entirely and include it in the hpe,gxp-timer.yaml. I assume that is the correct thing to do?

I would follow here the advice from Rob, so since the blocks are mixed
significantly (same address space), then let's assume it's actually one
device with two functions. In such case Rob pointed out that child node
is not necessary.

The implementation might differ, depending how the features are mixed-up
with each other. It might be one driver having timer and watchdog, or
several drivers (usually bound together with a MFD driver which serves
as parents and binds to the OF node).

Best regards,
Krzysztof

Patch

diff --git a/Documentation/devicetree/bindings/watchdog/hpe,gxp-wdt.yaml b/Documentation/devicetree/bindings/watchdog/hpe,gxp-wdt.yaml
new file mode 100644
index 000000000000..c20da146352f
--- /dev/null
+++ b/Documentation/devicetree/bindings/watchdog/hpe,gxp-wdt.yaml
@@ -0,0 +1,30 @@ 
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/watchdog/hpe,gxp-wdt.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: HPE GXP Controlled Watchdog
+
+allOf:
+  - $ref: "watchdog.yaml#"
+
+maintainers:
+  - Nick Hawkins <[e-mail]>
+  - Jean-Marie Verdun <[e-mail]>
+
+properties:
+  compatible:
+    const: hpe,gxp-wdt
+
+required:
+  - compatible
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    watchdog0:  watchdog {
+      compatible = "hpe,gxp-wdt";
+    };
+