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 |
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/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
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 >
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 {
Signed-off-by: Claudius Heine <ch@denx.de> --- arch/arm/dts/imx6qdl-dhcom-pdk2.dtsi | 5 +++++ 1 file changed, 5 insertions(+)