Re: [PATCH 3/5] dt-bindings: watchdog: renesas,rzn1-wdt: Document the reset line

[Krzysztof Kozlowski]

On 10/03/2026 19:12, Herve Codina wrote:
> Hi Krzysztof,
> 
> On Tue, 10 Mar 2026 18:38:50 +0100
> Krzysztof Kozlowski <krzk@xxxxxxxxxx> wrote:
> 
>> On 10/03/2026 18:32, Herve Codina (Schneider Electric) wrote:
>>> Watchdogs available in the RZ/N1 SoC can use their specific hardware
>>> reset line to reset the system on watchdog timeout.
>>>
>>> This line is not documented in the current binding.
>>>
>>> Fill this lack and describe this per watchdog reset line.
>>>
>>> Signed-off-by: Herve Codina (Schneider Electric) <herve.codina@xxxxxxxxxxx>
>>> ---
>>>  .../bindings/watchdog/renesas,rzn1-wdt.yaml   | 22 +++++++++++++++++++
>>>  .../dt-bindings/watchdog/renesas,rzn1-wdt.h   | 16 ++++++++++++++
>>>  2 files changed, 38 insertions(+)
>>>  create mode 100644 include/dt-bindings/watchdog/renesas,rzn1-wdt.h
>>>
>>> diff --git a/Documentation/devicetree/bindings/watchdog/renesas,rzn1-wdt.yaml b/Documentation/devicetree/bindings/watchdog/renesas,rzn1-wdt.yaml
>>> index 7e3ee533cd56..40a9a4ebc716 100644
>>> --- a/Documentation/devicetree/bindings/watchdog/renesas,rzn1-wdt.yaml
>>> +++ b/Documentation/devicetree/bindings/watchdog/renesas,rzn1-wdt.yaml
>>> @@ -26,6 +26,26 @@ properties:
>>>  
>>>    timeout-sec: true
>>>  
>>> +  renesas,reset-line:
>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>> +    enum: [0, 1]
>>> +    description: |
>>> +      The watchdog reset line (dt-bindings/watchdog/renesas,rzn1-wdt.h defines
>>> +      these values). A wachdog timeout asserts this reset line to perform a
>>> +      hardware system reset. Two watchdogs are present in the RZ/N1 SoC and
>>> +      each of them has a dedicated reset line.
>>> +
>>> +        - 0: RZN1_WDT_A7_0
>>> +          This reset line can be asserted only by the A7 0 watchdog. This
>>> +          watchdog is the one mapped at 0x40008000 on RZ/N1 SoCs.
>>> +
>>> +        - 1: RZN1_WDT_A7_1
>>> +          This reset line can be asserted only by the A7 1 watchdog. This
>>> +          watchdog is the one mapped at 0x40009000 on RZ/N1 SoCs.
>>> +
>>> +      If the renesas,reset-line property is not present, the watchdog timeout
>>> +      only triggers an interrupt.  
>>
>> I don't understand. You have two watchdogs (0x40008000 and 0x40009000)
>> so why you would tell each of them that they can reset line associated
>> with them? Can a watchdog reset other watchdog's line? No, thus code like:
>>
>> watchdog@40008000 {
>> 	renesas,reset-line = <RZN1_WDT_A7_1>;
>> };
>>
>> makes no sense and thus is pointless to specify in DT.
>>
>> What's more, if reset line is always wired (and how could it be since it
>> is fully within the soc), why would this be board-level property?
> 
> This is the exact same for interrupts and clocks.
> 
> Interrupts dedicated to IPs and hardwired, as well as clocks. Those resources
> are described in DT.
> 
> Why not this reset line?

How is it the same? clocks and interrupts represent wiring between
modules, so that other module (provider) will be properly configured. So
where is here phandle to point to the other module which you are
configuring? How single number <0, 1> can be a phandle?


> 
>>
>>
>>
>>> +
>>>  required:
>>>    - compatible
>>>    - reg
>>> @@ -41,10 +61,12 @@ examples:
>>>    - |
>>>      #include <dt-bindings/clock/r9a06g032-sysctrl.h>
>>>      #include <dt-bindings/interrupt-controller/arm-gic.h>
>>> +    #include <dt-bindings/watchdog/renesas,rzn1-wdt.h>
>>>  
>>>      watchdog@40008000 {
>>>              compatible = "renesas,r9a06g032-wdt", "renesas,rzn1-wdt";
>>>              reg = <0x40008000 0x1000>;
>>>              interrupts = <GIC_SPI 73 IRQ_TYPE_EDGE_RISING>;
>>>              clocks = <&sysctrl R9A06G032_CLK_WATCHDOG>;
>>> +            renesas,reset-line = <RZN1_WDT_A7_0>;
>>>      };
>>> diff --git a/include/dt-bindings/watchdog/renesas,rzn1-wdt.h b/include/dt-bindings/watchdog/renesas,rzn1-wdt.h
>>> new file mode 100644
>>> index 000000000000..fe534aff0609
>>> --- /dev/null
>>> +++ b/include/dt-bindings/watchdog/renesas,rzn1-wdt.h
>>> @@ -0,0 +1,16 @@
>>> +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
>>> +/*
>>> + * RZ/N1 watchdog reset lines
>>> + *
>>> + * Copyright (C) 2026 Bootlin
>>> + *
>>> + * Herve Codina <herve.codina@xxxxxxxxxxx>
>>> + */
>>> +
>>> +#ifndef __DT_BINDINGS_RZN1_WDT_H__
>>> +#define __DT_BINDINGS_RZN1_WDT_H__
>>> +
>>> +#define RZN1_WDT_A7_0		0
>>> +#define RZN1_WDT_A7_1		1  
>>
>> I also see little value of the binding, but probably because I don't
>> understand the point of this patch.
> 
> I mentioned 0 and 1 for those lines in the binding and referred to this
> header. What's wrong with that ?
> 
> Clocks use the same kind of description.
> A bunch of defines in header file to avoid a direct number.

So instead of answering my questions why you are doing this, why do you
need it, you use arguments "I saw some code looking like that, so I can
do that". No, you can not do that.

We don't copy blindly some code just because it looks similar. And we
avoid answering "I saw it somewhere" when someone asks you why you are
doing this.

Best regards,
Krzysztof