diff mbox series

[U-Boot,1/4] ARM: dts: dh-imx6: add wdt-reboot node for sysreset driver

Message ID 20191128120638.2457-2-ch@denx.de
State Superseded
Delegated to: Stefano Babic
Headers show
Series ARM: imx6: DHCOM i.MX6 PDK: Fixing reset | expand

Commit Message

Claudius Heine Nov. 28, 2019, 12:06 p.m. UTC 
Signed-off-by: Claudius Heine <ch@denx.de>
---
 arch/arm/dts/imx6qdl-dhcom-pdk2.dtsi | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Marek Vasut Nov. 28, 2019, 12:14 p.m. UTC | #1 
On 11/28/19 1:06 PM, Claudius Heine wrote:
> Signed-off-by: Claudius Heine <ch@denx.de>
> ---
>  arch/arm/dts/imx6qdl-dhcom-pdk2.dtsi | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/arch/arm/dts/imx6qdl-dhcom-pdk2.dtsi b/arch/arm/dts/imx6qdl-dhcom-pdk2.dtsi
> index af4719aaeb..572bcbf8f0 100644
> --- a/arch/arm/dts/imx6qdl-dhcom-pdk2.dtsi
> +++ b/arch/arm/dts/imx6qdl-dhcom-pdk2.dtsi
> @@ -30,6 +30,11 @@
>  		mux-int-port = <1>;
>  		mux-ext-port = <3>;
>  	};
> +
> +	wdt-reboot {
> +		compatible = "wdt-reboot";
> +		wdt = <&wdog1>;
> +	};
>  };
>  
>  &audmux {

This should go into separate -u-boot.dtsi , so the base DT can be easily
synced with Linux one.
Claudius Heine Nov. 28, 2019, 12:44 p.m. UTC | #2 
On 28/11/2019 13.14, Marek Vasut wrote:
> On 11/28/19 1:06 PM, Claudius Heine wrote:
>> Signed-off-by: Claudius Heine <ch@denx.de>
>> ---
>>  arch/arm/dts/imx6qdl-dhcom-pdk2.dtsi | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/arch/arm/dts/imx6qdl-dhcom-pdk2.dtsi b/arch/arm/dts/imx6qdl-dhcom-pdk2.dtsi
>> index af4719aaeb..572bcbf8f0 100644
>> --- a/arch/arm/dts/imx6qdl-dhcom-pdk2.dtsi
>> +++ b/arch/arm/dts/imx6qdl-dhcom-pdk2.dtsi
>> @@ -30,6 +30,11 @@
>>  		mux-int-port = <1>;
>>  		mux-ext-port = <3>;
>>  	};
>> +
>> +	wdt-reboot {
>> +		compatible = "wdt-reboot";
>> +		wdt = <&wdog1>;
>> +	};
>>  };
>>  
>>  &audmux {
> 
> This should go into separate -u-boot.dtsi , so the base DT can be easily
> synced with Linux one.
> 

Ok. Will do that in v2. Thanks!
Fabio Estevam Nov. 28, 2019, 12:49 p.m. UTC | #3 
Hi Claudius,

On Thu, Nov 28, 2019 at 9:07 AM Claudius Heine <ch@denx.de> wrote:
>
> Signed-off-by: Claudius Heine <ch@denx.de>
> ---
>  arch/arm/dts/imx6qdl-dhcom-pdk2.dtsi | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/arch/arm/dts/imx6qdl-dhcom-pdk2.dtsi b/arch/arm/dts/imx6qdl-dhcom-pdk2.dtsi
> index af4719aaeb..572bcbf8f0 100644
> --- a/arch/arm/dts/imx6qdl-dhcom-pdk2.dtsi
> +++ b/arch/arm/dts/imx6qdl-dhcom-pdk2.dtsi
> @@ -30,6 +30,11 @@
>                 mux-int-port = <1>;
>                 mux-ext-port = <3>;
>         };
> +
> +       wdt-reboot {
> +               compatible = "wdt-reboot";
> +               wdt = <&wdog1>;
> +       };
>  };

Could you use the the same way that Linux handles the imx2_wdt?

Please see the commit below:

commit ceea0c145d0c38badfcfc5443138e94ab094dc4a
Author: Robert Hancock <hancock@sedsystems.ca>
Date:   Tue Aug 6 11:05:29 2019 -0600

    watchdog: imx: Add DT ext-reset handling

    The Linux imx2_wdt driver uses a fsl,ext-reset-output boolean in the
    device tree to specify whether the board design should use the external
    reset instead of the internal reset. Use this boolean to determine which
    mode to use rather than using external reset unconditionally.

    For the legacy non-DM mode, the external reset is always used in order
    to maintain the previous behavior.

    Signed-off-by: Robert Hancock <hancock@sedsystems.ca>
Harald Seiler Nov. 28, 2019, 1:07 p.m. UTC | #4 
Hello Claudius, Fabio,

On Thu, 2019-11-28 at 09:49 -0300, Fabio Estevam wrote:
> Hi Claudius,
> 
> On Thu, Nov 28, 2019 at 9:07 AM Claudius Heine <ch@denx.de> wrote:
> > Signed-off-by: Claudius Heine <ch@denx.de>
> > ---
> >  arch/arm/dts/imx6qdl-dhcom-pdk2.dtsi | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/arch/arm/dts/imx6qdl-dhcom-pdk2.dtsi b/arch/arm/dts/imx6qdl-dhcom-pdk2.dtsi
> > index af4719aaeb..572bcbf8f0 100644
> > --- a/arch/arm/dts/imx6qdl-dhcom-pdk2.dtsi
> > +++ b/arch/arm/dts/imx6qdl-dhcom-pdk2.dtsi
> > @@ -30,6 +30,11 @@
> >                 mux-int-port = <1>;
> >                 mux-ext-port = <3>;
> >         };
> > +
> > +       wdt-reboot {
> > +               compatible = "wdt-reboot";
> > +               wdt = <&wdog1>;
> > +       };
> >  };
> 
> Could you use the the same way that Linux handles the imx2_wdt?
> 
> Please see the commit below:
> 
> commit ceea0c145d0c38badfcfc5443138e94ab094dc4a
> Author: Robert Hancock <hancock@sedsystems.ca>
> Date:   Tue Aug 6 11:05:29 2019 -0600
> 
>     watchdog: imx: Add DT ext-reset handling
> 
>     The Linux imx2_wdt driver uses a fsl,ext-reset-output boolean in the
>     device tree to specify whether the board design should use the external
>     reset instead of the internal reset. Use this boolean to determine which
>     mode to use rather than using external reset unconditionally.
> 
>     For the legacy non-DM mode, the external reset is always used in order
>     to maintain the previous behavior.

I think the source of the problem lies within this:  The old behavior
(before commit f2929d11a639 ("watchdog: imx: Use immediate reset bits
for expire_now")) was asserting *both* external and internal reset.  The
datasheet mentions the following for the bit to enable external reset:

 | There is no effect on [internal reset] upon writing on this bit.
 | [External reset] gets asserted along with [internal reset] if this bit
 | is set.

If the intention was to keep the old behavior, the
imx_watchdog_expire_now() function needs to be changed to either

- (ext_reset == false) write WCR_WDE | WCR_WDA (ie. assert only internal), or
- (ext_reset == true)  write WCR_WDE (ie. assert both internal and external).
Claudius Heine Nov. 28, 2019, 1:15 p.m. UTC | #5 
Hi Fabio,

On 28/11/2019 13.49, Fabio Estevam wrote:
> Hi Claudius,
> 
> On Thu, Nov 28, 2019 at 9:07 AM Claudius Heine <ch@denx.de> wrote:
>>
>> Signed-off-by: Claudius Heine <ch@denx.de>
>> ---
>>  arch/arm/dts/imx6qdl-dhcom-pdk2.dtsi | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/arch/arm/dts/imx6qdl-dhcom-pdk2.dtsi b/arch/arm/dts/imx6qdl-dhcom-pdk2.dtsi
>> index af4719aaeb..572bcbf8f0 100644
>> --- a/arch/arm/dts/imx6qdl-dhcom-pdk2.dtsi
>> +++ b/arch/arm/dts/imx6qdl-dhcom-pdk2.dtsi
>> @@ -30,6 +30,11 @@
>>                 mux-int-port = <1>;
>>                 mux-ext-port = <3>;
>>         };
>> +
>> +       wdt-reboot {
>> +               compatible = "wdt-reboot";
>> +               wdt = <&wdog1>;
>> +       };
>>  };
> 
> Could you use the the same way that Linux handles the imx2_wdt?

That is the sysreset device node, not the imx2_wdt one. (I will move
that into a '*-u-boot.dtsi' in v2)

Or am I misunderstanding you?
Fabio Estevam Nov. 28, 2019, 1:18 p.m. UTC | #6 
Hi Claudius,

On Thu, Nov 28, 2019 at 10:15 AM Claudius Heine <ch@denx.de> wrote:

> That is the sysreset device node, not the imx2_wdt one. (I will move
> that into a '*-u-boot.dtsi' in v2)
>
> Or am I misunderstanding you?

What I am asking is: why do we need a specific sysreset node for U-Boot?

Can't we just add the wdog node the same way we do in the kernel (from
the imx2_wdt binding) and get it to work?

Thanks
Claudius Heine Nov. 28, 2019, 1:42 p.m. UTC | #7 
On 28/11/2019 14.18, Fabio Estevam wrote:
> Hi Claudius,
> 
> On Thu, Nov 28, 2019 at 10:15 AM Claudius Heine <ch@denx.de> wrote:
> 
>> That is the sysreset device node, not the imx2_wdt one. (I will move
>> that into a '*-u-boot.dtsi' in v2)
>>
>> Or am I misunderstanding you?
> 
> What I am asking is: why do we need a specific sysreset node for U-Boot?

Good question. But I don't know a better answer to that than just saying
that this is currently the way the reset is implemented in u-boot with
DM AFAIK.

> Can't we just add the wdog node the same way we do in the kernel (from
> the imx2_wdt binding) and get it to work?

Probably. But I currently don't know a way to do it that doesn't involve
implementing new code.

I am currently more or less just doing what others have done before for
similar boards (display5).
Fabio Estevam Nov. 28, 2019, 1:55 p.m. UTC | #8 
On Thu, Nov 28, 2019 at 10:42 AM Claudius Heine <ch@denx.de> wrote:
>
> On 28/11/2019 14.18, Fabio Estevam wrote:
> > Hi Claudius,
> >
> > On Thu, Nov 28, 2019 at 10:15 AM Claudius Heine <ch@denx.de> wrote:
> >
> >> That is the sysreset device node, not the imx2_wdt one. (I will move
> >> that into a '*-u-boot.dtsi' in v2)
> >>
> >> Or am I misunderstanding you?
> >
> > What I am asking is: why do we need a specific sysreset node for U-Boot?
>
> Good question. But I don't know a better answer to that than just saying
> that this is currently the way the reset is implemented in u-boot with
> DM AFAIK.
>
> > Can't we just add the wdog node the same way we do in the kernel (from
> > the imx2_wdt binding) and get it to work?
>
> Probably. But I currently don't know a way to do it that doesn't involve
> implementing new code.

Could you please try passing the wdog node the same we do in the kernel?

I prefer we have a single way to deal with watchdog instead of having
a specific U-Boot method.
Claudius Heine Nov. 28, 2019, 3:31 p.m. UTC | #9 
On 28/11/2019 14.55, Fabio Estevam wrote:
> On Thu, Nov 28, 2019 at 10:42 AM Claudius Heine <ch@denx.de> wrote:
>>
>> On 28/11/2019 14.18, Fabio Estevam wrote:
>>> Hi Claudius,
>>>
>>> On Thu, Nov 28, 2019 at 10:15 AM Claudius Heine <ch@denx.de> wrote:
>>>
>>>> That is the sysreset device node, not the imx2_wdt one. (I will move
>>>> that into a '*-u-boot.dtsi' in v2)
>>>>
>>>> Or am I misunderstanding you?
>>>
>>> What I am asking is: why do we need a specific sysreset node for U-Boot?
>>
>> Good question. But I don't know a better answer to that than just saying
>> that this is currently the way the reset is implemented in u-boot with
>> DM AFAIK.
>>
>>> Can't we just add the wdog node the same way we do in the kernel (from
>>> the imx2_wdt binding) and get it to work?
>>
>> Probably. But I currently don't know a way to do it that doesn't involve
>> implementing new code.
> 
> Could you please try passing the wdog node the same we do in the kernel?
> 
> I prefer we have a single way to deal with watchdog instead of having
> a specific U-Boot method.

Sorry, but we are probably misunderstanding each other here. So I try to
go step by step to show how I think about this. Maybe that way we figure
out where our understanding differs. Please bear with me, its a bit
verbose :)

My patch adds the 'wdt-reboot' device node which is needed by u-boot to
reset the device but doesn't exist in the linux kernel (since there is
no 'wdt-reboot' driver):

+       wdt-reboot {
+               compatible = "wdt-reboot";
+               wdt = <&wdog1>;
+       };

This 'wdt-reboot' node instruments its driver
(drivers/sysreset/sysreset_watchdog.c [1]) to use the watchdog node
(arch/arm/dts/imx6qdl.dtsi [2]) and its driver
(drivers/watchdog/imx_watchdog.c [3]).

Neither the watchdog node in imx6qdl.dtsi [2], that is nearly identical
to the one in the linux kernel [4], nor its driver [3] was touched by
the changes in this patchset. The thing that has changed however is that
'CONFIG_SYSRESET_WATCHDOG' was enabled, which in turn enabled the
'CONFIG_WDT' switch. 'CONFIG_WDT' in turn disabled the 'hw_watchdog*'
function definitions in the imx-watchdog driver and enabled
'CONFIG_WATCHDOG' and disabled 'CONFIG_HW_WATCHDOG'.

The 'CONFIG_WATCHDOG' and 'CONFIG_HW_WATCHDOG' changes cause the
'WATCHDOG_RESET*' defines in [5] to no longer refer to the
'hw_watchdog*' but instead to the 'watchdog*' functions. Those functions
are handled by the watchdog DM class driver [6].


[1]
https://gitlab.denx.de/u-boot/u-boot/blob/4b19b89ca4a866b7baa642533e6dbd67cd832d27/drivers/sysreset/sysreset_watchdog.c#L48
[2]
https://gitlab.denx.de/u-boot/u-boot/blob/4b19b89ca4a866b7baa642533e6dbd67cd832d27/arch/arm/dts/imx6qdl.dtsi#L659
[3]
https://gitlab.denx.de/u-boot/u-boot/blob/4b19b89ca4a866b7baa642533e6dbd67cd832d27/drivers/watchdog/imx_watchdog.c
[4]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/imx6qdl.dtsi?h=v5.4#n672
[5]
https://gitlab.denx.de/u-boot/u-boot/blob/4b19b89ca4a866b7baa642533e6dbd67cd832d27/include/watchdog.h#L38
[6]
https://gitlab.denx.de/u-boot/u-boot/blob/4b19b89ca4a866b7baa642533e6dbd67cd832d27/drivers/watchdog/wdt-uclass.c#L74
Fabio Estevam Nov. 28, 2019, 3:43 p.m. UTC | #10 
Hi Claudius,

On Thu, Nov 28, 2019 at 12:31 PM Claudius Heine <ch@denx.de> wrote:

> Sorry, but we are probably misunderstanding each other here. So I try to
> go step by step to show how I think about this. Maybe that way we figure
> out where our understanding differs. Please bear with me, its a bit
> verbose :)

What I don't understand is why do we need "wdt-reboot" for i.MX in U-Boot?

The i.MX watchdog driver is capable of resetting via internal watchdog
or via a WDOG_B pin when the fsl,ext-reset-output property is passed.

> My patch adds the 'wdt-reboot' device node which is needed by u-boot to
> reset the device but doesn't exist in the linux kernel (since there is

Right. This is the point I don't understand: why do we need a U-Boot
wdt-reboot implementation in the first place?

The imx watchdog works in the kernel with DT. Why can't you just use
the same binding in U-Boot?

Thanks
Robert Hancock Nov. 28, 2019, 4:21 p.m. UTC | #11 
On 2019-11-28 9:43 a.m., Fabio Estevam wrote:
> Hi Claudius,
> 
> On Thu, Nov 28, 2019 at 12:31 PM Claudius Heine <ch@denx.de> wrote:
> 
>> Sorry, but we are probably misunderstanding each other here. So I try to
>> go step by step to show how I think about this. Maybe that way we figure
>> out where our understanding differs. Please bear with me, its a bit
>> verbose :)
> 
> What I don't understand is why do we need "wdt-reboot" for i.MX in U-Boot?
> 
> The i.MX watchdog driver is capable of resetting via internal watchdog
> or via a WDOG_B pin when the fsl,ext-reset-output property is passed.
> 
>> My patch adds the 'wdt-reboot' device node which is needed by u-boot to
>> reset the device but doesn't exist in the linux kernel (since there is
> 
> Right. This is the point I don't understand: why do we need a U-Boot
> wdt-reboot implementation in the first place?
> 
> The imx watchdog works in the kernel with DT. Why can't you just use
> the same binding in U-Boot?

I ended up needing to add this node for our board as well to be able to
reset from U-Boot using DM. The watchdog itself is set up just from its
own device tree entry, but there's nothing to tie the sysreset code into
using the watchdog (and which one to use, since iMX has two of them)
without that node being present.

In Linux this works differently - the watchdog drivers that are capable
of triggering an immediate reset will register themselves as a reset
handler automatically so the system will try to use that functionality
in order to reboot the system. To avoid the need for that explicit
wdt-reboot node, something like this would need to be implemented in U-Boot.

> 
> Thanks
>
Fabio Estevam Nov. 28, 2019, 7:44 p.m. UTC | #12 
Hi Robert,

On Thu, Nov 28, 2019 at 1:22 PM Robert Hancock <hancock@sedsystems.ca> wrote:

> I ended up needing to add this node for our board as well to be able to
> reset from U-Boot using DM. The watchdog itself is set up just from its
> own device tree entry, but there's nothing to tie the sysreset code into
> using the watchdog (and which one to use, since iMX has two of them)
> without that node being present.
>
> In Linux this works differently - the watchdog drivers that are capable
> of triggering an immediate reset will register themselves as a reset
> handler automatically so the system will try to use that functionality
> in order to reboot the system. To avoid the need for that explicit
> wdt-reboot node, something like this would need to be implemented in U-Boot.

Thanks for the clarification!
Claudius Heine Nov. 29, 2019, 7:36 a.m. UTC | #13 
Hi Fabio,

On 28/11/2019 16.43, Fabio Estevam wrote:
> Hi Claudius,
> 
> On Thu, Nov 28, 2019 at 12:31 PM Claudius Heine <ch@denx.de> wrote:
> 
>> Sorry, but we are probably misunderstanding each other here. So I try to
>> go step by step to show how I think about this. Maybe that way we figure
>> out where our understanding differs. Please bear with me, its a bit
>> verbose :)
> 
> What I don't understand is why do we need "wdt-reboot" for i.MX in U-Boot?
> 
> The i.MX watchdog driver is capable of resetting via internal watchdog
> or via a WDOG_B pin when the fsl,ext-reset-output property is passed.

Ok, right. Well my patches fixes the 'reset' command in u-boot and for
that we need a sysreset driver in u-boot currently.

The watchdog itself works fine IIRC.

> 
>> My patch adds the 'wdt-reboot' device node which is needed by u-boot to
>> reset the device but doesn't exist in the linux kernel (since there is
> 
> Right. This is the point I don't understand: why do we need a U-Boot
> wdt-reboot implementation in the first place?
> 
> The imx watchdog works in the kernel with DT. Why can't you just use
> the same binding in U-Boot?
> 
> Thanks
>
diff mbox series

Patch

diff --git a/arch/arm/dts/imx6qdl-dhcom-pdk2.dtsi b/arch/arm/dts/imx6qdl-dhcom-pdk2.dtsi
index af4719aaeb..572bcbf8f0 100644
--- a/arch/arm/dts/imx6qdl-dhcom-pdk2.dtsi
+++ b/arch/arm/dts/imx6qdl-dhcom-pdk2.dtsi
@@ -30,6 +30,11 @@ 
 		mux-int-port = <1>;
 		mux-ext-port = <3>;
 	};
+
+	wdt-reboot {
+		compatible = "wdt-reboot";
+		wdt = <&wdog1>;
+	};
 };
 
 &audmux {