[4/6] dt-bindings: watchdog: renesas: Document `renesas,r9a09g057-syscon-wdt-errorrst` property

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

The RZ/V2H(P) CPG block includes Error Reset Registers (CPG_ERROR_RSTm).
A system reset is triggered in response to error interrupt factors, and
the corresponding bit is set in the CPG_ERROR_RSTm register. These
registers can be utilized by various IP blocks as needed.

In the event of a watchdog overflow or underflow, a system reset is issued,
and the CPG_ERROR_RST2[0/1/2/3] bits are set depending on the watchdog in
use: CM33 = 0, CA55 = 1, CR8_0 = 2, CR8_1 = 3. For the watchdog driver to
determine and report the current boot status, it needs to read the
CPG_ERROR_RST2[0/1/2/3]bits and provide this information to the user upon
request.

To facilitate this operation, add `renesas,r9a09g057-syscon-wdt-errorrst`
property to the WDT node, which maps to the `syscon` CPG node, enabling
retrieval of the necessary information. For example:

    wdt1: watchdog@14400000 {
        compatible = "renesas,r9a09g057-wdt";
        renesas,r9a09g057-syscon-wdt-errorrst = <&cpg 0xb40 1>;
        ...
    };

The `renesas,r9a09g057-syscon-wdt-errorrst` property consists of three
cells:
1. The first cell is a phandle to the CPG node.
2. The second cell specifies the offset of the CPG_ERROR_RSTm register
   within the SYSCON.
3. The third cell indicates the specific bit within the CPG_ERROR_RSTm
   register.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 .../bindings/watchdog/renesas,wdt.yaml          | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

On Wed, Dec 18, 2024 at 12:34:12AM +0000, Prabhakar wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> The RZ/V2H(P) CPG block includes Error Reset Registers (CPG_ERROR_RSTm).
> A system reset is triggered in response to error interrupt factors, and
> the corresponding bit is set in the CPG_ERROR_RSTm register. These
> registers can be utilized by various IP blocks as needed.
> 
> In the event of a watchdog overflow or underflow, a system reset is issued,
> and the CPG_ERROR_RST2[0/1/2/3] bits are set depending on the watchdog in
> use: CM33 = 0, CA55 = 1, CR8_0 = 2, CR8_1 = 3. For the watchdog driver to
> determine and report the current boot status, it needs to read the
> CPG_ERROR_RST2[0/1/2/3]bits and provide this information to the user upon
> request.
> 
> To facilitate this operation, add `renesas,r9a09g057-syscon-wdt-errorrst`
> property to the WDT node, which maps to the `syscon` CPG node, enabling
> retrieval of the necessary information. For example:
> 
>     wdt1: watchdog@14400000 {
>         compatible = "renesas,r9a09g057-wdt";
>         renesas,r9a09g057-syscon-wdt-errorrst = <&cpg 0xb40 1>;
>         ...

Drop, obvious.

>     };
> 
> The `renesas,r9a09g057-syscon-wdt-errorrst` property consists of three
> cells:
> 1. The first cell is a phandle to the CPG node.
> 2. The second cell specifies the offset of the CPG_ERROR_RSTm register
>    within the SYSCON.
> 3. The third cell indicates the specific bit within the CPG_ERROR_RSTm
>    register.

Don't describe the contents of patch.  Drop paragraph. There is no need
to make commit msg unnecessary long. Focus on explaining unknown parts
of commit: why? or who is affected by ABI break? why breaking ABI?
instead of repeating diff.

> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
>  .../bindings/watchdog/renesas,wdt.yaml          | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/watchdog/renesas,wdt.yaml b/Documentation/devicetree/bindings/watchdog/renesas,wdt.yaml
> index 29ada89fdcdc..8d29f5f1be7e 100644
> --- a/Documentation/devicetree/bindings/watchdog/renesas,wdt.yaml
> +++ b/Documentation/devicetree/bindings/watchdog/renesas,wdt.yaml
> @@ -112,6 +112,19 @@ properties:
>  
>    timeout-sec: true
>  
> +  renesas,r9a09g057-syscon-wdt-errorrst:

There are never, *never* SoC names in property names, because we want
properties to be re-usable.

Make the property generic for all your devices and be sure to disallow
it everywhere the CPG_ERROR_RSTm *does not* exist (which is different
from "where CPG_ERROR_RSTm is not used by watchdog driver").

> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> +    description:
> +      The first cell is a phandle to the SYSCON entry required to obtain
> +      the current boot status. The second cell specifies the CPG_ERROR_RSTm
> +      register offset within the SYSCON, and the third cell indicates the
> +      bit within the CPG_ERROR_RSTm register.
> +    items:
> +      - items:
> +          - description: Phandle to the CPG node
> +          - description: The CPG_ERROR_RSTm register offset
> +          - description: The bit within CPG_ERROR_RSTm register of interest
> +
>  required:
>    - compatible
>    - reg
> @@ -182,7 +195,11 @@ allOf:
>        properties:
>          interrupts: false
>          interrupt-names: false
> +      required:
> +        - renesas,r9a09g057-syscon-wdt-errorrst

No, ABI break.

Best regards,
Krzysztof

On 19/12/2024 11:06, Lad, Prabhakar wrote:
>>> To facilitate this operation, add `renesas,r9a09g057-syscon-wdt-errorrst`
>>> property to the WDT node, which maps to the `syscon` CPG node, enabling
>>> retrieval of the necessary information. For example:
>>>
>>>     wdt1: watchdog@14400000 {
>>>         compatible = "renesas,r9a09g057-wdt";
>>>         renesas,r9a09g057-syscon-wdt-errorrst = <&cpg 0xb40 1>;
>>>         ...
>>
>> Drop, obvious.
>>
> Ok.
> 
>>>     };
>>>
>>> The `renesas,r9a09g057-syscon-wdt-errorrst` property consists of three
>>> cells:
>>> 1. The first cell is a phandle to the CPG node.
>>> 2. The second cell specifies the offset of the CPG_ERROR_RSTm register
>>>    within the SYSCON.
>>> 3. The third cell indicates the specific bit within the CPG_ERROR_RSTm
>>>    register.
>>
>> Don't describe the contents of patch.  Drop paragraph. There is no need
>> to make commit msg unnecessary long. Focus on explaining unknown parts
>> of commit: why? or who is affected by ABI break? why breaking ABI?
>> instead of repeating diff.
>>
> Ok, I'll drop the para. There isnt any ABI breakage as the driver
> patch [0] will skip supporting watchdog bootstatus if this property is
> not present.
> 
> [0] https://lore.kernel.org/all/20241218003414.490498-6-prabhakar.mahadev-lad.rj@bp.renesas.com/

Really? I see in rzv2h_wdt_probe():

+		if (ret)
+			return ret;

so to me you are failing the probe, not skipping anything.

> 
>>>
>>> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>>> ---
>>>  .../bindings/watchdog/renesas,wdt.yaml          | 17 +++++++++++++++++
>>>  1 file changed, 17 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/watchdog/renesas,wdt.yaml b/Documentation/devicetree/bindings/watchdog/renesas,wdt.yaml
>>> index 29ada89fdcdc..8d29f5f1be7e 100644
>>> --- a/Documentation/devicetree/bindings/watchdog/renesas,wdt.yaml
>>> +++ b/Documentation/devicetree/bindings/watchdog/renesas,wdt.yaml
>>> @@ -112,6 +112,19 @@ properties:
>>>
>>>    timeout-sec: true
>>>
>>> +  renesas,r9a09g057-syscon-wdt-errorrst:
>>
>> There are never, *never* SoC names in property names, because we want
>> properties to be re-usable.
>>
> I should have mentioned this in my commit message (my bad). The
> renesas,wdt.yaml binding file is being used for all the SoCs
> currently. To avoid any conflicts by just having vendor specific
> property I added SoC name to the preoperty.

I know what you did and I replied: that's a no go for reasons I stated.

> 
> @Geert/Wolfram - Maybe we need to split the binding on per SoC bases?

You can but I don't understand why exactly.

> 
>> Make the property generic for all your devices and be sure to disallow
>> it everywhere the CPG_ERROR_RSTm *does not* exist (which is different
>> from "where CPG_ERROR_RSTm is not used by watchdog driver").
>>
> This patch already disallows `renesas,r9a09g057-syscon-wdt-errorrst`
> for the rest of the SoCs and only allows for RZ/V2H(P) SoC or am I
> missing something?

No, that's fine, but to avoid disallowing it for others you named it per
SoC.

> 
>>> +    $ref: /schemas/types.yaml#/definitions/phandle-array
>>> +    description:
>>> +      The first cell is a phandle to the SYSCON entry required to obtain
>>> +      the current boot status. The second cell specifies the CPG_ERROR_RSTm
>>> +      register offset within the SYSCON, and the third cell indicates the
>>> +      bit within the CPG_ERROR_RSTm register.
>>> +    items:
>>> +      - items:
>>> +          - description: Phandle to the CPG node
>>> +          - description: The CPG_ERROR_RSTm register offset
>>> +          - description: The bit within CPG_ERROR_RSTm register of interest
>>> +
>>>  required:
>>>    - compatible
>>>    - reg
>>> @@ -182,7 +195,11 @@ allOf:
>>>        properties:
>>>          interrupts: false
>>>          interrupt-names: false
>>> +      required:
>>> +        - renesas,r9a09g057-syscon-wdt-errorrst
>>
>> No, ABI break.
>>
> As mentioned above we won't break ABI, this required flag is for future changes.

Then why is this required? Or at least explain this in the commit msg.

Best regards,
Krzysztof

On 22/12/2024 12:11, Lad, Prabhakar wrote:
>>> [0] https://lore.kernel.org/all/20241218003414.490498-6-prabhakar.mahadev-lad.rj@bp.renesas.com/
>>
>> Really? I see in rzv2h_wdt_probe():
>>
>> +               if (ret)
>> +                       return ret;
>>
>> so to me you are failing the probe, not skipping anything.
>>
> Yes really this wont break any ABI. From patch [0] we have the below:
> 
> [0] https://lore.kernel.org/all/20241218003414.490498-6-prabhakar.mahadev-lad.rj@bp.renesas.com/
> 
>     /* Do not error out to maintain old DT compatibility */
>     syscon = syscon_regmap_lookup_by_phandle(np,
> "renesas,syscon-cpg-error-rst");

Right, somehow I missed that part.

>     if (!IS_ERR(syscon)) {
>         struct of_phandle_args args;
>         u32 reg;
> 
>         ret = of_parse_phandle_with_fixed_args(np,
> "renesas,syscon-cpg-error-rst",
>                                2, 0, &args);
>         if (ret)
>             return ret;
Best regards,
Krzysztof