[U-Boot] [PATCH 0/4] ARM: imx6: DHCOM i.MX6 PDK: Fixing reset
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(+)
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 {
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.
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!
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
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).
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?
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
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).
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.
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/4b19b89ca4a866b7baa642533e6dbd67cd... [2] https://gitlab.denx.de/u-boot/u-boot/blob/4b19b89ca4a866b7baa642533e6dbd67cd... [3] https://gitlab.denx.de/u-boot/u-boot/blob/4b19b89ca4a866b7baa642533e6dbd67cd... [4] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch... [5] https://gitlab.denx.de/u-boot/u-boot/blob/4b19b89ca4a866b7baa642533e6dbd67cd... [6] https://gitlab.denx.de/u-boot/u-boot/blob/4b19b89ca4a866b7baa642533e6dbd67cd...
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
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
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!
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
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
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
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
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_watchd... [2]: https://gitlab.denx.de/u-boot/u-boot/blob/master/drivers/watchdog/imx_watchd...
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
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(+)
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
participants (5)
-
Claudius Heine -
Fabio Estevam -
Harald Seiler -
Marek Vasut -
Robert Hancock