[U-Boot] [PATCH V2] imx: mx7: psci: add system reset support

[U-Boot] [PATCH V2] imx: mx7: psci: add system reset support

[Anson Huang]

Add i.MX7 PSCI system reset support, linux
kernel now can use "reboot" command to reset
system.

Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
changes since V1:
	write WDOG register directly instead of calling reset_cpu
	which is NOT safe since it is NOT in secure section.
 arch/arm/mach-imx/mx7/psci-mx7.c | 5 +++++
 arch/arm/mach-imx/mx7/psci.S     | 7 +++++++
 2 files changed, 12 insertions(+)

diff --git a/arch/arm/mach-imx/mx7/psci-mx7.c b/arch/arm/mach-imx/mx7/psci-mx7.c
index 7f429b0..66f6db6 100644
--- a/arch/arm/mach-imx/mx7/psci-mx7.c
+++ b/arch/arm/mach-imx/mx7/psci-mx7.c
@@ -74,3 +74,8 @@ __secure int imx_cpu_off(int cpu)
 	writel(0, SRC_BASE_ADDR + cpu * 8 + SRC_GPR1_MX7D + 4);
 	return 0;
 }
+
+__secure void imx_system_reset(void)
+{
+	writew(1 << 2, WDOG1_BASE_ADDR);
+}
diff --git a/arch/arm/mach-imx/mx7/psci.S b/arch/arm/mach-imx/mx7/psci.S
index fc5eb34..59f98cd 100644
--- a/arch/arm/mach-imx/mx7/psci.S
+++ b/arch/arm/mach-imx/mx7/psci.S
@@ -43,4 +43,11 @@ psci_cpu_off:
 1: 	wfi
 	b 1b
 
+.globl psci_system_reset
+psci_system_reset:
+	b	imx_system_reset
+
+2: 	wfi
+	b 2b
+
 	.popsection
-- 
1.9.1

[U-Boot] [PATCH V2] imx: mx7: psci: add system reset support

[Peng Fan]

Hi Anson,

> -----Original Message-----
> From: Anson Huang [mailto:Anson.Huang at nxp.com]
> Sent: Thursday, January 04, 2018 5:04 PM
> To: sbabic at denx.de; Fabio Estevam <fabio.estevam@nxp.com>;
> albert.u.boot at aribaud.net; christian.gmeiner at gmail.com; Peng Fan
> <peng.fan@nxp.com>; u-boot at lists.denx.de
> Subject: [PATCH V2] imx: mx7: psci: add system reset support
> 
> Add i.MX7 PSCI system reset support, linux kernel now can use "reboot"
> command to reset system.
> 
> Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> ---
> changes since V1:
> 	write WDOG register directly instead of calling reset_cpu
> 	which is NOT safe since it is NOT in secure section.
>  arch/arm/mach-imx/mx7/psci-mx7.c | 5 +++++
>  arch/arm/mach-imx/mx7/psci.S     | 7 +++++++
>  2 files changed, 12 insertions(+)
> 
> diff --git a/arch/arm/mach-imx/mx7/psci-mx7.c b/arch/arm/mach-
> imx/mx7/psci-mx7.c
> index 7f429b0..66f6db6 100644
> --- a/arch/arm/mach-imx/mx7/psci-mx7.c
> +++ b/arch/arm/mach-imx/mx7/psci-mx7.c
> @@ -74,3 +74,8 @@ __secure int imx_cpu_off(int cpu)
>  	writel(0, SRC_BASE_ADDR + cpu * 8 + SRC_GPR1_MX7D + 4);
>  	return 0;
>  }
> +
> +__secure void imx_system_reset(void)
> +{
> +	writew(1 << 2, WDOG1_BASE_ADDR);

I think this should be "writew((WCR_WDE | WCR_SRS), WDOG1_BASE_ADDR);"?

Regards,
Peng.

> +}
> diff --git a/arch/arm/mach-imx/mx7/psci.S b/arch/arm/mach-imx/mx7/psci.S
> index fc5eb34..59f98cd 100644
> --- a/arch/arm/mach-imx/mx7/psci.S
> +++ b/arch/arm/mach-imx/mx7/psci.S
> @@ -43,4 +43,11 @@ psci_cpu_off:
>  1: 	wfi
>  	b 1b
> 
> +.globl psci_system_reset
> +psci_system_reset:
> +	b	imx_system_reset
> +
> +2: 	wfi
> +	b 2b
> +
>  	.popsection
> --
> 1.9.1

[U-Boot] [PATCH V2] imx: mx7: psci: add system reset support

[Anson Huang]

Hi, Peng

Best Regards!
Anson Huang


> -----Original Message-----
> From: Peng Fan
> Sent: 2018-01-04 5:48 PM
> To: Anson Huang <anson.huang@nxp.com>; sbabic at denx.de; Fabio Estevam
> <fabio.estevam@nxp.com>; albert.u.boot at aribaud.net;
> christian.gmeiner at gmail.com; u-boot at lists.denx.de
> Subject: RE: [PATCH V2] imx: mx7: psci: add system reset support
> 
> Hi Anson,
> 
> > -----Original Message-----
> > From: Anson Huang [mailto:Anson.Huang at nxp.com]
> > Sent: Thursday, January 04, 2018 5:04 PM
> > To: sbabic at denx.de; Fabio Estevam <fabio.estevam@nxp.com>;
> > albert.u.boot at aribaud.net; christian.gmeiner at gmail.com; Peng Fan
> > <peng.fan@nxp.com>; u-boot at lists.denx.de
> > Subject: [PATCH V2] imx: mx7: psci: add system reset support
> >
> > Add i.MX7 PSCI system reset support, linux kernel now can use "reboot"
> > command to reset system.
> >
> > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> > ---
> > changes since V1:
> > 	write WDOG register directly instead of calling reset_cpu
> > 	which is NOT safe since it is NOT in secure section.
> >  arch/arm/mach-imx/mx7/psci-mx7.c | 5 +++++
> >  arch/arm/mach-imx/mx7/psci.S     | 7 +++++++
> >  2 files changed, 12 insertions(+)
> >
> > diff --git a/arch/arm/mach-imx/mx7/psci-mx7.c b/arch/arm/mach-
> > imx/mx7/psci-mx7.c index 7f429b0..66f6db6 100644
> > --- a/arch/arm/mach-imx/mx7/psci-mx7.c
> > +++ b/arch/arm/mach-imx/mx7/psci-mx7.c
> > @@ -74,3 +74,8 @@ __secure int imx_cpu_off(int cpu)
> >  	writel(0, SRC_BASE_ADDR + cpu * 8 + SRC_GPR1_MX7D + 4);
> >  	return 0;
> >  }
> > +
> > +__secure void imx_system_reset(void)
> > +{
> > +	writew(1 << 2, WDOG1_BASE_ADDR);
> 
> I think this should be "writew((WCR_WDE | WCR_SRS),
> WDOG1_BASE_ADDR);"?

I use SRS reset which will assert reset signal immediately, no need to wait
Wdog timeout.


Anson.

> 
> Regards,
> Peng.
> 
> > +}
> > diff --git a/arch/arm/mach-imx/mx7/psci.S
> > b/arch/arm/mach-imx/mx7/psci.S index fc5eb34..59f98cd 100644
> > --- a/arch/arm/mach-imx/mx7/psci.S
> > +++ b/arch/arm/mach-imx/mx7/psci.S
> > @@ -43,4 +43,11 @@ psci_cpu_off:
> >  1: 	wfi
> >  	b 1b
> >
> > +.globl psci_system_reset
> > +psci_system_reset:
> > +	b	imx_system_reset
> > +
> > +2: 	wfi
> > +	b 2b
> > +
> >  	.popsection
> > --
> > 1.9.1

[U-Boot] [PATCH V2] imx: mx7: psci: add system reset support

[Fabio Estevam]

Hi Anson,

On Thu, Jan 4, 2018 at 7:03 AM, Anson Huang <Anson.Huang@nxp.com> wrote:
> Add i.MX7 PSCI system reset support, linux
> kernel now can use "reboot" command to reset
> system.

Reading this commit message gives me the impression that the 'reboot'
command does not work currently.

However, the 'reboot' command works provided the boards connects the
WDOG output pin to the PMIC  due to the MX7 erratum.

What exactly are you fixing here? Please clarify.

[U-Boot] [PATCH V2] imx: mx7: psci: add system reset support

[Anson Huang]

Hi, Fabio

Best Regards!
Anson Huang


> -----Original Message-----
> From: Fabio Estevam [mailto:festevam at gmail.com]
> Sent: 2018-01-04 8:57 PM
> To: Anson Huang <anson.huang@nxp.com>
> Cc: Stefano Babic <sbabic@denx.de>; Fabio Estevam
> <fabio.estevam@nxp.com>; Albert ARIBAUD <albert.u.boot@aribaud.net>;
> Christian Gmeiner <christian.gmeiner@gmail.com>; Peng Fan
> <peng.fan@nxp.com>; U-Boot-Denx <u-boot@lists.denx.de>
> Subject: Re: [U-Boot] [PATCH V2] imx: mx7: psci: add system reset support
> 
> Hi Anson,
> 
> On Thu, Jan 4, 2018 at 7:03 AM, Anson Huang <Anson.Huang@nxp.com> wrote:
> > Add i.MX7 PSCI system reset support, linux kernel now can use "reboot"
> > command to reset system.
> 
> Reading this commit message gives me the impression that the 'reboot'
> command does not work currently.
> 
> However, the 'reboot' command works provided the boards connects the
> WDOG output pin to the PMIC  due to the MX7 erratum.
> 
> What exactly are you fixing here? Please clarify.

Thanks for the comments, the current reboot command works is just
because wdog driver is enabled and when system reboot, the reboot notification
callback in wdog driver will do reset. But if wdog is disabled in dtb, the reboot
command will fail.

As now we use PSCI, the reboot operation will eventually call into u-boot's psci
reset callback, we should support this feature, the reboot feature in Linux kernel
should NOT depends on wdog's existence.

But I did notice that I miss a change to enable the psci reset feature, we should
Use psci 1.0 instead of 0.1, psci 0.1 only supports very limited features which does
NOT include system reset, power off etc..

So I re-send a patch series to do these changes, to support PSCI system reset, system
power off, please help review, thanks.

Anson.

[U-Boot] [PATCH V2] imx: mx7: psci: add system reset support

[Fabio Estevam]

Hi Anson,

On Fri, Jan 5, 2018 at 4:25 AM, Anson Huang <anson.huang@nxp.com> wrote:

> Thanks for the comments, the current reboot command works is just
> because wdog driver is enabled and when system reboot, the reboot notification
> callback in wdog driver will do reset. But if wdog is disabled in dtb, the reboot
> command will fail.
>
> As now we use PSCI, the reboot operation will eventually call into u-boot's psci
> reset callback, we should support this feature, the reboot feature in Linux kernel
> should NOT depends on wdog's existence.

Ok, but what about erratum e10574 (Watchdog: A watchdog timeout or
software trigger will not reset the SOC)?

According to this erratum WDOG_B pin needs to generate a system power on.

[U-Boot] [PATCH V2] imx: mx7: psci: add system reset support

[Anson Huang]

Best Regards!
Anson Huang


> -----Original Message-----
> From: Fabio Estevam [mailto:festevam at gmail.com]
> Sent: 2018-01-07 6:48 AM
> To: Anson Huang <anson.huang@nxp.com>
> Cc: Stefano Babic <sbabic@denx.de>; Fabio Estevam
> <fabio.estevam@nxp.com>; Albert ARIBAUD <albert.u.boot@aribaud.net>;
> Christian Gmeiner <christian.gmeiner@gmail.com>; Peng Fan
> <peng.fan@nxp.com>; U-Boot-Denx <u-boot@lists.denx.de>
> Subject: Re: [U-Boot] [PATCH V2] imx: mx7: psci: add system reset support
> 
> Hi Anson,
> 
> On Fri, Jan 5, 2018 at 4:25 AM, Anson Huang <anson.huang@nxp.com> wrote:
> 
> > Thanks for the comments, the current reboot command works is just
> > because wdog driver is enabled and when system reboot, the reboot
> > notification callback in wdog driver will do reset. But if wdog is
> > disabled in dtb, the reboot command will fail.
> >
> > As now we use PSCI, the reboot operation will eventually call into
> > u-boot's psci reset callback, we should support this feature, the
> > reboot feature in Linux kernel should NOT depends on wdog's existence.
> 
> Ok, but what about erratum e10574 (Watchdog: A watchdog timeout or
> software trigger will not reset the SOC)?
> 
> According to this erratum WDOG_B pin needs to generate a system power on.

Correct, we need to enable WDA to toggle WDOG_B for reset, thanks.
Will send a V3 patch set.

Anson.
 

[U-Boot] [PATCH V2] imx: mx7: psci: add system reset support

[Anson Huang]

Best Regards!
Anson Huang


> -----Original Message-----
> From: Fabio Estevam [mailto:festevam at gmail.com]
> Sent: 2018-01-07 6:48 AM
> To: Anson Huang <anson.huang@nxp.com>
> Cc: Stefano Babic <sbabic@denx.de>; Fabio Estevam
> <fabio.estevam@nxp.com>; Albert ARIBAUD <albert.u.boot@aribaud.net>;
> Christian Gmeiner <christian.gmeiner@gmail.com>; Peng Fan
> <peng.fan@nxp.com>; U-Boot-Denx <u-boot@lists.denx.de>
> Subject: Re: [U-Boot] [PATCH V2] imx: mx7: psci: add system reset support
> 
> Hi Anson,
> 
> On Fri, Jan 5, 2018 at 4:25 AM, Anson Huang <anson.huang@nxp.com> wrote:
> 
> > Thanks for the comments, the current reboot command works is just
> > because wdog driver is enabled and when system reboot, the reboot
> > notification callback in wdog driver will do reset. But if wdog is
> > disabled in dtb, the reboot command will fail.
> >
> > As now we use PSCI, the reboot operation will eventually call into
> > u-boot's psci reset callback, we should support this feature, the
> > reboot feature in Linux kernel should NOT depends on wdog's existence.
> 
> Ok, but what about erratum e10574 (Watchdog: A watchdog timeout or
> software trigger will not reset the SOC)?
> 
> According to this erratum WDOG_B pin needs to generate a system power on.

What should we do here? Writing 0x4 into WCR register will cause WDOG reset
and WDOG_B will be asserted, if the board has WDOG_B connected to PMIC, then
everything is right. Do you mean we need to handle the case of WDOG_B pin NOT
connected to PMIC case? If the board has no such design, then the reset will be
NOT safe using wdog.

Anson 

[U-Boot] [U-Boot,V2] imx: mx7: psci: add system reset support

[Trent Piepho]

On Thu, 2018-01-04 at 17:03 +0800, Anson Huang wrote:
> Add i.MX7 PSCI system reset support, linux
> kernel now can use "reboot" command to reset
> system.


> +__secure void imx_system_reset(void)
> +{
> +	writew(1 << 2, WDOG1_BASE_ADDR);
> +}

This does not work properly on our board.

Due to an erratum in iMX7d it is necessary to wire the external WDOG_B
signal to the pmic to cycle power in order to reset the board.  The
Linux IMX watchdog driver works when it does a reboot via the watchdog,
but this code does not.

When the Linux drivers is configured for an external wdog signal, using
a DT property, it sets WCR_SRS to prevent the internal system reset in
response to watchdog triggering.  When that is not done, as in this
patch, the internal reset appears to reset the wdog module or iomux or
something, which causes the imx7d to stop asserting the external wdog
signal.

In my tests, this takes only about 2.4 µs.  Such a short wdog_b pulse
does not appear to be sufficient to trigger the pmic to cycle power.

When SRS is set, then the WDOG_B signal will remain asserted until the
POR_B input signal to the imx7d is asserted.  I.e., until pmic responds
to the watchdog signal.

[U-Boot] [U-Boot,V2] imx: mx7: psci: add system reset support

[Peng Fan]

> -----Original Message-----
> From: Trent Piepho [mailto:tpiepho at impinj.com]
> Sent: 2018年5月8日 6:07
> To: christian.gmeiner at gmail.com; Peng Fan <peng.fan@nxp.com>; Anson
> Huang <anson.huang@nxp.com>; u-boot at lists.denx.de;
> albert.u.boot at aribaud.net; sbabic at denx.de
> Cc: Fabio Estevam <fabio.estevam@nxp.com>
> Subject: Re: [U-Boot,V2] imx: mx7: psci: add system reset support
> 
> On Thu, 2018-01-04 at 17:03 +0800, Anson Huang wrote:
> > Add i.MX7 PSCI system reset support, linux kernel now can use "reboot"
> > command to reset system.
> 
> 
> > +__secure void imx_system_reset(void)
> > +{
> > +	writew(1 << 2, WDOG1_BASE_ADDR);
> > +}
> 
> This does not work properly on our board.

You could try write 0x14 to WDOG1_BASE_ADDR to see whether it works.
Here using (1 << 2) triggers both SRS and WDOG_B which seems wrong.

-Peng.

> 
> Due to an erratum in iMX7d it is necessary to wire the external WDOG_B
> signal to the pmic to cycle power in order to reset the board.  The
> Linux IMX watchdog driver works when it does a reboot via the watchdog,
> but this code does not.
> 
> When the Linux drivers is configured for an external wdog signal, using
> a DT property, it sets WCR_SRS to prevent the internal system reset in
> response to watchdog triggering.  When that is not done, as in this
> patch, the internal reset appears to reset the wdog module or iomux or
> something, which causes the imx7d to stop asserting the external wdog
> signal.
> 
> In my tests, this takes only about 2.4 µs.  Such a short wdog_b pulse
> does not appear to be sufficient to trigger the pmic to cycle power.
> 
> When SRS is set, then the WDOG_B signal will remain asserted until the
> POR_B input signal to the imx7d is asserted.  I.e., until pmic responds
> to the watchdog signal.

[U-Boot] [U-Boot,V2] imx: mx7: psci: add system reset support

[Trent Piepho]

On Tue, 2018-05-08 at 05:29 +0000, Peng Fan wrote:
> > -----Original Message-----
> > From: Trent Piepho [mailto:tpiepho at impinj.com]
> > Sent: 2018年5月8日 6:07
> > To: christian.gmeiner at gmail.com; Peng Fan <peng.fan@nxp.com>; Anson
> > Huang <anson.huang@nxp.com>; u-boot at lists.denx.de;
> > albert.u.boot at aribaud.net; sbabic at denx.de
> > Cc: Fabio Estevam <fabio.estevam@nxp.com>
> > Subject: Re: [U-Boot,V2] imx: mx7: psci: add system reset support
> > 
> > On Thu, 2018-01-04 at 17:03 +0800, Anson Huang wrote:
> > > Add i.MX7 PSCI system reset support, linux kernel now can use "reboot"
> > > command to reset system.
> > 
> > 
> > > +__secure void imx_system_reset(void)
> > > +{
> > > +	writew(1 << 2, WDOG1_BASE_ADDR);
> > > +}
> > 
> > This does not work properly on our board.
> 
> You could try write 0x14 to WDOG1_BASE_ADDR to see whether it works.
> Here using (1 << 2) triggers both SRS and WDOG_B which seems wrong.

It works when the Linux driver does that and appears to work
identically when uboot does it that way.

Triggering both SRS and WDOG_B does not work as it causes a very short
wdog_b assertion.

There is a little difficulty here in just changing the code to use
WCR_WDE|WCR_SRS, as that would be wrong if the board does not use an
external signal.  The Linux driver uses the device tree to determine
what to do, but this will not work for PSCI.

> > Due to an erratum in iMX7d it is necessary to wire the external WDOG_B
> > signal to the pmic to cycle power in order to reset the board.  The
> > Linux IMX watchdog driver works when it does a reboot via the watchdog,
> > but this code does not.
> > 
> > When the Linux drivers is configured for an external wdog signal, using
> > a DT property, it sets WCR_SRS to prevent the internal system reset in
> > response to watchdog triggering.  When that is not done, as in this
> > patch, the internal reset appears to reset the wdog module or iomux or
> > something, which causes the imx7d to stop asserting the external wdog
> > signal.
> > 
> > In my tests, this takes only about 2.4 µs.  Such a short wdog_b pulse
> > does not appear to be sufficient to trigger the pmic to cycle power.
> > 
> > When SRS is set, then the WDOG_B signal will remain asserted until the
> > POR_B input signal to the imx7d is asserted.  I.e., until pmic responds
> > to the watchdog signal.

[U-Boot] [U-Boot,V2] imx: mx7: psci: add system reset support

[Peng Fan]

> -----Original Message-----
> From: Trent Piepho [mailto:tpiepho at impinj.com]
> Sent: 2018年5月9日 4:11
> To: Peng Fan <peng.fan@nxp.com>; christian.gmeiner at gmail.com; Anson
> Huang <anson.huang@nxp.com>; u-boot at lists.denx.de;
> albert.u.boot at aribaud.net; sbabic at denx.de
> Cc: Fabio Estevam <fabio.estevam@nxp.com>
> Subject: Re: [U-Boot,V2] imx: mx7: psci: add system reset support
> 
> On Tue, 2018-05-08 at 05:29 +0000, Peng Fan wrote:
> > > -----Original Message-----
> > > From: Trent Piepho [mailto:tpiepho at impinj.com]
> > > Sent: 2018年5月8日 6:07
> > > To: christian.gmeiner at gmail.com; Peng Fan <peng.fan@nxp.com>; Anson
> > > Huang <anson.huang@nxp.com>; u-boot at lists.denx.de;
> > > albert.u.boot at aribaud.net; sbabic at denx.de
> > > Cc: Fabio Estevam <fabio.estevam@nxp.com>
> > > Subject: Re: [U-Boot,V2] imx: mx7: psci: add system reset support
> > >
> > > On Thu, 2018-01-04 at 17:03 +0800, Anson Huang wrote:
> > > > Add i.MX7 PSCI system reset support, linux kernel now can use "reboot"
> > > > command to reset system.
> > >
> > >
> > > > +__secure void imx_system_reset(void) {
> > > > +	writew(1 << 2, WDOG1_BASE_ADDR); }
> > >
> > > This does not work properly on our board.
> >
> > You could try write 0x14 to WDOG1_BASE_ADDR to see whether it works.
> > Here using (1 << 2) triggers both SRS and WDOG_B which seems wrong.
> 
> It works when the Linux driver does that and appears to work identically when
> uboot does it that way.
> 
> Triggering both SRS and WDOG_B does not work as it causes a very short
> wdog_b assertion.

Yes.

> 
> There is a little difficulty here in just changing the code to use
> WCR_WDE|WCR_SRS, as that would be wrong if the board does not use an
> external signal.  The Linux driver uses the device tree to determine what to do,
> but this will not work for PSCI.

U-Boot need parse the device tree and configure the WDOG accordingly.

-Peng

> 
> > > Due to an erratum in iMX7d it is necessary to wire the external
> > > WDOG_B signal to the pmic to cycle power in order to reset the
> > > board.  The Linux IMX watchdog driver works when it does a reboot
> > > via the watchdog, but this code does not.
> > >
> > > When the Linux drivers is configured for an external wdog signal,
> > > using a DT property, it sets WCR_SRS to prevent the internal system
> > > reset in response to watchdog triggering.  When that is not done, as
> > > in this patch, the internal reset appears to reset the wdog module
> > > or iomux or something, which causes the imx7d to stop asserting the
> > > external wdog signal.
> > >
> > > In my tests, this takes only about 2.4 µs.  Such a short wdog_b
> > > pulse does not appear to be sufficient to trigger the pmic to cycle power.
> > >
> > > When SRS is set, then the WDOG_B signal will remain asserted until
> > > the POR_B input signal to the imx7d is asserted.  I.e., until pmic
> > > responds to the watchdog signal.

[U-Boot] [U-Boot,V2] imx: mx7: psci: add system reset support

[Trent Piepho]

On Wed, 2018-05-09 at 01:13 +0000, Peng Fan wrote:
> > +0800, Anson Huang wrote:
> > > > > Add i.MX7 PSCI system reset support, linux kernel now can use "reboot"
> > > > > command to reset system.
> > > > 
> > > > 
> > > > > +__secure void imx_system_reset(void) {
> > > > > +	writew(1 << 2, WDOG1_BASE_ADDR); }
> > > > 
> > > > This does not work properly on our board.
> > > 
> > > You could try write 0x14 to WDOG1_BASE_ADDR to see whether it works.
> > > Here using (1 << 2) triggers both SRS and WDOG_B which seems wrong.
> > 
> > It works when the Linux driver does that and appears to work identically when
> > uboot does it that way.
> > 
> > Triggering both SRS and WDOG_B does not work as it causes a very short
> > wdog_b assertion.
> 
> Yes.

In case it wasn't clear, this code is in the new 2018.05 u-boot release
and I think will cause a problem for any imx7d board using the typical
reset erratum option of connecting wdog_b to a pfuze3000 pmic's pwrwon
pin.

An external watchdog monitor chip, I think as used on sabreD for
instance, might still respond to the short pulse and allow it work.

> > There is a little difficulty here in just changing the code to use
> > WCR_WDE|WCR_SRS, as that would be wrong if the board does not use an
> > external signal.  The Linux driver uses the device tree to determine what to do,
> > but this will not work for PSCI.
> 
> U-Boot need parse the device tree and configure the WDOG accordingly.

Yes, but consider the difficulties:

There could be multiple Linux device trees, e.g. in a FIT image and
chosen based on board revision.  Should the PSCI code use the u-boot
device tree?  Which would then need to also have this board revision
support.  Or should it use the Linux DT, which it would need to get
information from when the kernel DT is prepared before boot?

Of the five external watchdog signals from the iMX7d, which one should
be used?  I believe each imx watchdog configured in Linux will register
a reset handler that are called in turn, so Linux should handle this
ok.  The uboot PSCI currently assumes wdog1 is the correct one to use.

The Linux driver interfaces with Linux pinctl to iomux the watchdog
signal.  The PSCI code is assuming the wdog signal is correctly muxed.
Default iomux values doesn't have any wdog signal routed to a pin.

Not impossible, but it is not so simple as the original three lines of
code might have made it seem at first glance.

If one has the Linux imx watchdog driver working, to use as watchdog,
then one gets a wdog system reset for free with all the above taken
care of.  Which makes me wonder if disabling the PSCI reset might not
be a better option in that situration.

[U-Boot] [U-Boot,V2] imx: mx7: psci: add system reset support

[Peng Fan]

> -----Original Message-----
> From: Trent Piepho [mailto:tpiepho at impinj.com]
> Sent: 2018年5月10日 3:22
> To: Peng Fan <peng.fan@nxp.com>; christian.gmeiner at gmail.com; Anson
> Huang <anson.huang@nxp.com>; u-boot at lists.denx.de;
> albert.u.boot at aribaud.net; sbabic at denx.de
> Cc: Fabio Estevam <fabio.estevam@nxp.com>
> Subject: Re: [U-Boot,V2] imx: mx7: psci: add system reset support
> 
> On Wed, 2018-05-09 at 01:13 +0000, Peng Fan wrote:
> > > +0800, Anson Huang wrote:
> > > > > > Add i.MX7 PSCI system reset support, linux kernel now can use
> "reboot"
> > > > > > command to reset system.
> > > > >
> > > > >
> > > > > > +__secure void imx_system_reset(void) {
> > > > > > +	writew(1 << 2, WDOG1_BASE_ADDR); }
> > > > >
> > > > > This does not work properly on our board.
> > > >
> > > > You could try write 0x14 to WDOG1_BASE_ADDR to see whether it works.
> > > > Here using (1 << 2) triggers both SRS and WDOG_B which seems wrong.
> > >
> > > It works when the Linux driver does that and appears to work
> > > identically when uboot does it that way.
> > >
> > > Triggering both SRS and WDOG_B does not work as it causes a very
> > > short wdog_b assertion.
> >
> > Yes.
> 
> In case it wasn't clear, this code is in the new 2018.05 u-boot release and I think
> will cause a problem for any imx7d board using the typical reset erratum option
> of connecting wdog_b to a pfuze3000 pmic's pwrwon pin.
> 
> An external watchdog monitor chip, I think as used on sabreD for instance,
> might still respond to the short pulse and allow it work.

I understand this. 

> 
> > > There is a little difficulty here in just changing the code to use
> > > WCR_WDE|WCR_SRS, as that would be wrong if the board does not use an
> > > external signal.  The Linux driver uses the device tree to determine
> > > what to do, but this will not work for PSCI.
> >
> > U-Boot need parse the device tree and configure the WDOG accordingly.
> 
> Yes, but consider the difficulties:
> 
> There could be multiple Linux device trees, e.g. in a FIT image and chosen based
> on board revision.  Should the PSCI code use the u-boot device tree?  Which
> would then need to also have this board revision support.  Or should it use the
> Linux DT, which it would need to get information from when the kernel DT is
> prepared before boot?
> 
> Of the five external watchdog signals from the iMX7d, which one should be used?
> I believe each imx watchdog configured in Linux will register a reset handler
> that are called in turn, so Linux should handle this ok.  The uboot PSCI currently
> assumes wdog1 is the correct one to use.
> 
> The Linux driver interfaces with Linux pinctl to iomux the watchdog signal.  The
> PSCI code is assuming the wdog signal is correctly muxed.
> Default iomux values doesn't have any wdog signal routed to a pin.
> 
> Not impossible, but it is not so simple as the original three lines of code might
> have made it seem at first glance.

The imx_system_reset might could be moved to per board specific. Then the board
owner could have their own imeplementation.

Stefano, what do you think?

> 
> If one has the Linux imx watchdog driver working, to use as watchdog, then one
> gets a wdog system reset for free with all the above taken care of.  Which
> makes me wonder if disabling the PSCI reset might not be a better option in that
> situration.

Just wonder, since you are using psci, you have the power related code ready?

-Peng.