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], ®);
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], ®);
}
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
