| Message ID | 20191128120638.2457-1-ch@denx.de |
|---|---|
| Headers | show |
| Series | ARM: imx6: DHCOM i.MX6 PDK: Fixing reset | expand |
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
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(+) >> > > [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 >
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