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

[Lad, Prabhakar]

Hi Krzysztof,

On Thu, Dec 19, 2024 at 4:01 PM Krzysztof Kozlowski <krzk@xxxxxxxxxx> wrote:
>
> 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@xxxxxxxxxxxxxx/
>
> 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@xxxxxxxxxxxxxx/

    /* Do not error out to maintain old DT compatibility */
    syscon = syscon_regmap_lookup_by_phandle(np,
"renesas,syscon-cpg-error-rst");
    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;

        ret = regmap_read(syscon, args.args[0], &reg);
        if (ret)
            return -EINVAL;

        if (reg & CPG_ERROR_RST2(args.args[1])) {
            ret = regmap_write(syscon, args.args[0],
                       CPG_ERROR_RST2(args.args[1]) |
                       CPG_ERROR_RST2_WEN(args.args[1]));
            if (ret)
                return -EINVAL;
        }
        cardreset = true;
        bootstatus = reg & CPG_ERROR_RST2(args.args[1]) ? WDIOF_CARDRESET : 0;
        regmap_read(syscon, args.args[0], &reg);
    }

Case 1: "renesas,syscon-cpg-error-rst" is missing in the device tree (DT).
In this case, syscon_regmap_lookup_by_phandle() will return an error
code. Since the condition (!IS_ERR(syscon)) checks for a success case,
the code does not enter the if block when an error is returned.

Case 2: "renesas,syscon-cpg-error-rst" is present in the DT.
Here, syscon_regmap_lookup_by_phandle() will return 0, allowing the
code to enter the if block. Once inside the if block, we can confirm
that "renesas,syscon-cpg-error-rst" is present in the DT. At this
point, we validate the property and use
of_parse_phandle_with_fixed_args(). If the property does not match our
requirements, we return an error. Does returning an error when
"renesas,syscon-cpg-error-rst" is present but with unexpected values
in the DT break the ABI in this scenario?

> >
> >>>
> >>> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> >>> ---
> >>>  .../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.
>
OK, I'll rename the property to "renesas,syscon-cpg-error-rst".

> >
> >> 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.
>
Sure i'll explain this in the commit message.

Cheers,
Prabhakar