[U-Boot] [PATCH 0/4] ARM: imx6: DHCOM i.MX6 PDK: Fixing reset

[U-Boot] [PATCH 0/4] ARM: imx6: DHCOM i.MX6 PDK: Fixing reset

[Claudius Heine]

Hi,

currently the reset on the DHCOM i.MX6 board is brocken in u-boot.

This patchset fixes that by integrating the sysreset and watchdog dm driver.

regards,
Claudius

Claudius Heine (4):
  ARM: dts: dh-imx6: add wdt-reboot node for sysreset driver
  ARM: imx6: DHCOM i.MX6 PDK: Enable sysreset driver
  ARM: imx6: DHCOM i.MX6 PDK: Enable wdt command
  ARM: imx6: DHCOM i.MX6 PDK: Use HW_WATCHDOG in SPL

 arch/arm/dts/imx6qdl-dhcom-pdk2.dtsi | 5 +++++
 configs/dh_imx6_defconfig            | 3 +++
 include/configs/dh_imx6.h            | 5 +++++
 3 files changed, 13 insertions(+)

-- 
2.21.0

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

[Claudius Heine]

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 {
-- 
2.21.0

[U-Boot] [PATCH 2/4] ARM: imx6: DHCOM i.MX6 PDK: Enable sysreset driver

[Claudius Heine]

Signed-off-by: Claudius Heine <ch@denx.de>
---
 configs/dh_imx6_defconfig | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/configs/dh_imx6_defconfig b/configs/dh_imx6_defconfig
index 4055812007..68dc3b0d1f 100644
--- a/configs/dh_imx6_defconfig
+++ b/configs/dh_imx6_defconfig
@@ -77,6 +77,8 @@ CONFIG_DM_SCSI=y
 CONFIG_SPI=y
 CONFIG_DM_SPI=y
 CONFIG_MXC_SPI=y
+CONFIG_SYSRESET=y
+CONFIG_SYSRESET_WATCHDOG=y
 CONFIG_USB=y
 CONFIG_DM_USB=y
 CONFIG_USB_GADGET=y
-- 
2.21.0

[U-Boot] [PATCH 3/4] ARM: imx6: DHCOM i.MX6 PDK: Enable wdt command

[Claudius Heine]

Signed-off-by: Claudius Heine <ch@denx.de>
---
 configs/dh_imx6_defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/configs/dh_imx6_defconfig b/configs/dh_imx6_defconfig
index 68dc3b0d1f..e5c44381b2 100644
--- a/configs/dh_imx6_defconfig
+++ b/configs/dh_imx6_defconfig
@@ -37,6 +37,7 @@ CONFIG_CMD_MMC=y
 CONFIG_CMD_SATA=y
 CONFIG_CMD_USB=y
 CONFIG_CMD_USB_MASS_STORAGE=y
+CONFIG_CMD_WDT=y
 CONFIG_CMD_CACHE=y
 CONFIG_CMD_TIME=y
 CONFIG_CMD_EXT4_WRITE=y
-- 
2.21.0

[U-Boot] [PATCH 4/4] ARM: imx6: DHCOM i.MX6 PDK: Use HW_WATCHDOG in SPL

[Claudius Heine]

The SPL does not have DM enabled and therefor needs to use the hardware
watchdog interface provided by the imx-watchdog driver.

Signed-off-by: Claudius Heine <ch@denx.de>
---
 include/configs/dh_imx6.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/configs/dh_imx6.h b/include/configs/dh_imx6.h
index d4bd88f511..77074aaa06 100644
--- a/include/configs/dh_imx6.h
+++ b/include/configs/dh_imx6.h
@@ -87,6 +87,11 @@
 #endif
 
 /* Watchdog */
+#if defined(CONFIG_SPL_BUILD)
+#undef CONFIG_WDT
+#undef CONFIG_WATCHDOG
+#define CONFIG_HW_WATCHDOG
+#endif
 
 /* allow to overwrite serial and ethaddr */
 #define CONFIG_ENV_OVERWRITE
-- 
2.21.0

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

[Marek Vasut]

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.

[U-Boot] [PATCH 0/4] ARM: imx6: DHCOM i.MX6 PDK: Fixing reset

[Harald Seiler]

Hello Claudius,

On Thu, 2019-11-28 at 13:06 +0100, Claudius Heine wrote:
> Hi,
> 
> currently the reset on the DHCOM i.MX6 board is brocken in u-boot.
> 
> This patchset fixes that by integrating the sysreset and watchdog dm driver.

I think you should clarify that reset was broken by commit f2929d11a639
("watchdog: imx: Use immediate reset bits for expire_now") which changed
reset to, by default, only assert the external reset [1].  DHCOM i.MX6
needs the internal reset though, which previously was asserted as as
well.  Maybe you can add a `Fixes:` line to one of your commits?

Additionally, I am still unsure whether the current default behavior is
correct.  I'd rather assert both external and internal reset, which is
what the i.MX watchdog does on timeout as well (as long as WDT bit is
set, which we do by default [2]).  There is also an inconsistency
between the non-DM implementation (external by default) and DM
implementation (internal by default).

> regards,
> Claudius
> 
> Claudius Heine (4):
>   ARM: dts: dh-imx6: add wdt-reboot node for sysreset driver
>   ARM: imx6: DHCOM i.MX6 PDK: Enable sysreset driver
>   ARM: imx6: DHCOM i.MX6 PDK: Enable wdt command
>   ARM: imx6: DHCOM i.MX6 PDK: Use HW_WATCHDOG in SPL
> 
>  arch/arm/dts/imx6qdl-dhcom-pdk2.dtsi | 5 +++++
>  configs/dh_imx6_defconfig            | 3 +++
>  include/configs/dh_imx6.h            | 5 +++++
>  3 files changed, 13 insertions(+)
> 

[1]: https://gitlab.denx.de/u-boot/u-boot/blob/master/drivers/watchdog/imx_watchdog.c#L45
[2]: https://gitlab.denx.de/u-boot/u-boot/blob/master/drivers/watchdog/imx_watchdog.c#L96
-- 
Harald

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

[Claudius Heine]

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!

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

[Fabio Estevam]

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>

[U-Boot] [PATCH 0/4] ARM: imx6: DHCOM i.MX6 PDK: Fixing reset

[Claudius Heine]

Hi Harald,

On 28/11/2019 13.34, Harald Seiler wrote:
> Hello Claudius,
> 
> On Thu, 2019-11-28 at 13:06 +0100, Claudius Heine wrote:
>> Hi,
>>
>> currently the reset on the DHCOM i.MX6 board is brocken in u-boot.
>>
>> This patchset fixes that by integrating the sysreset and watchdog dm driver.
> 
> I think you should clarify that reset was broken by commit f2929d11a639
> ("watchdog: imx: Use immediate reset bits for expire_now") which changed
> reset to, by default, only assert the external reset [1].  DHCOM i.MX6
> needs the internal reset though, which previously was asserted as as
> well.  Maybe you can add a `Fixes:` line to one of your commits?

Hmm... Not sure which of the commit should have the 'Fixes' line though,
since each does some part of fixing it. Maybe I should squash them?

> 
> Additionally, I am still unsure whether the current default behavior is
> correct.  I'd rather assert both external and internal reset, which is
> what the i.MX watchdog does on timeout as well (as long as WDT bit is
> set, which we do by default [2]).  There is also an inconsistency
> between the non-DM implementation (external by default) and DM
> implementation (internal by default).

Yes, but that is a separate discussion IMO. This patch just addresses
the stuff for DHCOM does not touch the imx-watchdog driver. Maybe an RFC
patch that fixes the inconsistency might be useful to start it.

regards,
Claudius

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

[Harald Seiler]

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).

-- 
Harald

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-62  Fax: +49-8142-66989-80   Email: hws at denx.de

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

[Claudius Heine]

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?

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

[Fabio Estevam]

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

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

[Claudius Heine]

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).

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

[Fabio Estevam]

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.

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

[Claudius Heine]

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

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

[Fabio Estevam]

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

[U-Boot] [PATCH 0/4] ARM: imx6: DHCOM i.MX6 PDK: Fixing reset

[Robert Hancock]

On 2019-11-28 6:34 a.m., Harald Seiler wrote:
> Hello Claudius,
> 
> On Thu, 2019-11-28 at 13:06 +0100, Claudius Heine wrote:
>> Hi,
>>
>> currently the reset on the DHCOM i.MX6 board is brocken in u-boot.
>>
>> This patchset fixes that by integrating the sysreset and watchdog dm driver.
> 
> I think you should clarify that reset was broken by commit f2929d11a639
> ("watchdog: imx: Use immediate reset bits for expire_now") which changed
> reset to, by default, only assert the external reset [1].  DHCOM i.MX6
> needs the internal reset though, which previously was asserted as as
> well.  Maybe you can add a `Fixes:` line to one of your commits?

The behavior of the driver in the DM mode should match what the Linux
IMX watchdog driver is doing for both the watchdog timeout and reset
operations. The reset operation there explicitly uses either internal
reset or external reset, not both. For the actual watchdog timeout, they
both set the WDT bit or not depending on whether ext-reset is set, which
it seems would result in either internal+external reset or just internal
reset (it doesn't look like you can trigger only an external reset on
timeout).

> 
> Additionally, I am still unsure whether the current default behavior is
> correct.  I'd rather assert both external and internal reset, which is
> what the i.MX watchdog does on timeout as well (as long as WDT bit is
> set, which we do by default [2]).  There is also an inconsistency
> between the non-DM implementation (external by default) and DM
> implementation (internal by default).

The legacy non-DM implementation kept the settings for timeout the same
as they were before. For the reset, it appears that it used to do
internal+external reset whereas now it does external only, so it's
possible that might cause an issue on some boards. However, they should
really be switching to DM and setting the ext-reset attribute properly
depending on which reset they actually want, as that's needed to get
proper watchdog timeout/restart behavior in Linux as well. I doubt any
board actually needs both internal and external resets since then Linux
would be unable to reboot properly.

> 
>> regards,
>> Claudius
>>
>> Claudius Heine (4):
>>   ARM: dts: dh-imx6: add wdt-reboot node for sysreset driver
>>   ARM: imx6: DHCOM i.MX6 PDK: Enable sysreset driver
>>   ARM: imx6: DHCOM i.MX6 PDK: Enable wdt command
>>   ARM: imx6: DHCOM i.MX6 PDK: Use HW_WATCHDOG in SPL
>>
>>  arch/arm/dts/imx6qdl-dhcom-pdk2.dtsi | 5 +++++
>>  configs/dh_imx6_defconfig            | 3 +++
>>  include/configs/dh_imx6.h            | 5 +++++
>>  3 files changed, 13 insertions(+)
>>
> 
> [1]: https://gitlab.denx.de/u-boot/u-boot/blob/master/drivers/watchdog/imx_watchdog.c#L45
> [2]: https://gitlab.denx.de/u-boot/u-boot/blob/master/drivers/watchdog/imx_watchdog.c#L96
> 

-- 
Robert Hancock
Senior Software Developer
SED Systems, a division of Calian Ltd.
Email: hancock at sedsystems.ca

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

[Robert Hancock]

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
> 

-- 
Robert Hancock
Senior Software Developer
SED Systems, a division of Calian Ltd.
Email: hancock at sedsystems.ca

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

[Fabio Estevam]

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!

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

[Claudius Heine]

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
> 

[U-Boot] [PATCH 0/4] ARM: imx6: DHCOM i.MX6 PDK: Fixing reset

[Claudius Heine]

Hi Robert,

On 28/11/2019 17.17, Robert Hancock wrote:
> On 2019-11-28 6:34 a.m., Harald Seiler wrote:
>> Hello Claudius,
>>
>> On Thu, 2019-11-28 at 13:06 +0100, Claudius Heine wrote:
>>> Hi,
>>>
>>> currently the reset on the DHCOM i.MX6 board is brocken in u-boot.
>>>
>>> This patchset fixes that by integrating the sysreset and watchdog dm driver.
>>
>> I think you should clarify that reset was broken by commit f2929d11a639
>> ("watchdog: imx: Use immediate reset bits for expire_now") which changed
>> reset to, by default, only assert the external reset [1].  DHCOM i.MX6
>> needs the internal reset though, which previously was asserted as as
>> well.  Maybe you can add a `Fixes:` line to one of your commits?
> 
> The behavior of the driver in the DM mode should match what the Linux
> IMX watchdog driver is doing for both the watchdog timeout and reset
> operations. The reset operation there explicitly uses either internal
> reset or external reset, not both. For the actual watchdog timeout, they
> both set the WDT bit or not depending on whether ext-reset is set, which
> it seems would result in either internal+external reset or just internal
> reset (it doesn't look like you can trigger only an external reset on
> timeout).
> 
>>
>> Additionally, I am still unsure whether the current default behavior is
>> correct.  I'd rather assert both external and internal reset, which is
>> what the i.MX watchdog does on timeout as well (as long as WDT bit is
>> set, which we do by default [2]).  There is also an inconsistency
>> between the non-DM implementation (external by default) and DM
>> implementation (internal by default).
> 
> The legacy non-DM implementation kept the settings for timeout the same
> as they were before. For the reset, it appears that it used to do
> internal+external reset whereas now it does external only, so it's
> possible that might cause an issue on some boards. However, they should
> really be switching to DM and setting the ext-reset attribute properly
> depending on which reset they actually want, as that's needed to get
> proper watchdog timeout/restart behavior in Linux as well. I doubt any
> board actually needs both internal and external resets since then Linux
> would be unable to reboot properly.

Thx, for the explanation! An issue I could think of is in the SPL, where
DM is not possible because of size limitations. If that SPL wants to
trigger a reset, will that not cause only an external reset and boards
that need a internal one will just hang?

If that is the case, then there probably should be a way to configure
that or let it trigger both like it did before.

regards,
Claudius

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-54 Fax: (+49)-8142-66989-80 Email: ch at denx.de

           PGP key: 6FF2 E59F 00C6 BC28 31D8 64C1 1173 CB19 9808 B153
                             Keyserver: hkp://pool.sks-keyservers.net