Hi Krzysztof,
Thank you for the review.
On Thu, Dec 19, 2024 at 9:02 AM Krzysztof Kozlowski <krzk@xxxxxxxxxx> wrote:
>
> On Wed, Dec 18, 2024 at 12:34:12AM +0000, Prabhakar wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> >
> > 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.
>
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/
> >
> > 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.
@Geert/Wolfram - Maybe we need to split the binding on per SoC bases?
> 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?
> > + $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.
Cheers,
Prabhakar
