| Message ID | 3545645b5481c72123bfc450269f14007ee8154a.1608045173.git.hws@denx.de |
|---|---|
| State | Accepted |
| Commit | 3394f398b5f37d930b9ae1b6638fe26b0cc735d8 |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | Remove addr parameter from reset_cpu() | expand |
Hi, This functionality (soft vs hard reset) is used in multiple LPC32xx products with our custom hardware. If this support is remove from upstream, we will have to maintain this patch locally (out of tree). Sylvain Lemieux -----Original Message----- From: Harald Seiler <hws@denx.de> Sent: Tuesday, December 15, 2020 10:48 AM To: u-boot@lists.denx.de Cc: Harald Seiler <hws@denx.de>; Tom Rini <trini@konsulko.com>; Simon Glass <sjg@chromium.org>; Sylvain Lemieux <sylvain.lemieux@jci.com> Subject: [PATCH 3/4] Revert "lpc32xx: cpu: add support for soft reset" This reverts commit 576007aec9a4a5f4f3dd1f690fb26a8c05ceb75f. The paramter passed to reset_cpu() no longer holds a meaning as all call-sites now pass the value 0. Thus, branching on it is essentially dead code and will just confuse future readers. Revert soft-reset support and just always perform a hard-reset for now. This is a preparation for removal of the reset_cpu() parameter across the entire tree in a later patch. Fixes: 576007aec9a4 ("lpc32xx: cpu: add support for soft reset") Cc: Sylvain Lemieux <slemieux@tycoint.com> Signed-off-by: Harald Seiler <hws@denx.de> --- arch/arm/mach-lpc32xx/cpu.c | 21 +++++---------------- 1 file changed, 5 insertions(+), 16 deletions(-) diff --git a/arch/arm/mach-lpc32xx/cpu.c b/arch/arm/mach-lpc32xx/cpu.c index 32af6206056b..7378192a33c2 100644 --- a/arch/arm/mach-lpc32xx/cpu.c +++ b/arch/arm/mach-lpc32xx/cpu.c @@ -22,23 +22,12 @@ void reset_cpu(ulong addr) /* Enable watchdog clock */ setbits_le32(&clk->timclk_ctrl, CLK_TIMCLK_WATCHDOG); - /* To be compatible with the original U-Boot code: - * addr: - 0: perform hard reset. - * - !=0: perform a soft reset; i.e. "RESOUT_N" not asserted). */ - if (addr == 0) { - /* Reset pulse length is 13005 peripheral clock frames */ - writel(13000, &wdt->pulse); + /* Reset pulse length is 13005 peripheral clock frames */ + writel(13000, &wdt->pulse); - /* Force WDOG_RESET2 and RESOUT_N signal active */ - writel(WDTIM_MCTRL_RESFRC2 | WDTIM_MCTRL_RESFRC1 - | WDTIM_MCTRL_M_RES2, &wdt->mctrl); - } else { - /* Force match output active */ - writel(0x01, &wdt->emr); - - /* Internal reset on match output (no pulse on "RESOUT_N") */ - writel(WDTIM_MCTRL_M_RES1, &wdt->mctrl); - } + /* Force WDOG_RESET2 and RESOUT_N signal active */ + writel(WDTIM_MCTRL_RESFRC2 | WDTIM_MCTRL_RESFRC1 | WDTIM_MCTRL_M_RES2, + &wdt->mctrl); while (1) /* NOP */; -- 2.26.2
Hello Sylvain, On Tue, 2020-12-15 at 17:29 +0000, Sylvain Lemieux wrote: > Hi, > > This functionality (soft vs hard reset) is used in multiple LPC32xx > products with our custom hardware. > > If this support is remove from upstream, we will have to maintain this > patch locally (out of tree). My intention with this series is of course not to break existing and useful functionality. Can you elaborate how you are making use of this differentiation in its current form? Does some board code call reset_cpu() directly, with different parameter values? I would argue that at this time, the proper way to support both soft-reset and hard-reset is via a sysreset driver, see `drivers/sysreset/sysreset_psci.c` for a simple example. As I laid out in the cover letter of this series, the `addr` parameter to reset_cpu() does not really fit this purpose. Regards,
Hi Harald, Yes, we have board code that will call this function with a different value to generate a soft reset (i.e. no pulse on "RESOUT_N"). We have a need to be able to reset the processor (soft or hard) based on the need to generate or not a pulse on RESOUT_N signal. We will be able to keep this patch local until somebody in the team look at a sysreset driver implementation. Regards, Sylvain Lemieux -----Original Message----- From: Harald Seiler <hws@denx.de> Sent: Thursday, December 17, 2020 5:09 AM To: Sylvain Lemieux <sylvain.lemieux@jci.com>; u-boot@lists.denx.de Cc: Tom Rini <trini@konsulko.com>; Simon Glass <sjg@chromium.org> Subject: Re: [PATCH 3/4] Revert "lpc32xx: cpu: add support for soft reset" Hello Sylvain, On Tue, 2020-12-15 at 17:29 +0000, Sylvain Lemieux wrote: > Hi, > > This functionality (soft vs hard reset) is used in multiple LPC32xx > products with our custom hardware. > > If this support is remove from upstream, we will have to maintain this > patch locally (out of tree). My intention with this series is of course not to break existing and useful functionality. Can you elaborate how you are making use of this differentiation in its current form? Does some board code call reset_cpu() directly, with different parameter values? I would argue that at this time, the proper way to support both soft-reset and hard-reset is via a sysreset driver, see `drivers/sysreset/sysreset_psci.c` for a simple example. As I laid out in the cover letter of this series, the `addr` parameter to reset_cpu() does not really fit this purpose. Regards, -- Harald > Sylvain Lemieux > > -----Original Message----- > From: Harald Seiler <hws@denx.de> > Sent: Tuesday, December 15, 2020 10:48 AM > To: u-boot@lists.denx.de > Cc: Harald Seiler <hws@denx.de>; Tom Rini <trini@konsulko.com>; Simon > Glass <sjg@chromium.org>; Sylvain Lemieux <sylvain.lemieux@jci.com> > Subject: [PATCH 3/4] Revert "lpc32xx: cpu: add support for soft reset" > > This reverts commit 576007aec9a4a5f4f3dd1f690fb26a8c05ceb75f. > > The paramter passed to reset_cpu() no longer holds a meaning as all > call-sites now pass the value 0. Thus, branching on it is essentially > dead code and will just confuse future readers. > > Revert soft-reset support and just always perform a hard-reset for now. > This is a preparation for removal of the reset_cpu() parameter across > the entire tree in a later patch. > > Fixes: 576007aec9a4 ("lpc32xx: cpu: add support for soft reset") > Cc: Sylvain Lemieux <slemieux@tycoint.com> > Signed-off-by: Harald Seiler <hws@denx.de> > --- > arch/arm/mach-lpc32xx/cpu.c | 21 +++++---------------- > 1 file changed, 5 insertions(+), 16 deletions(-) > > diff --git a/arch/arm/mach-lpc32xx/cpu.c b/arch/arm/mach-lpc32xx/cpu.c > index 32af6206056b..7378192a33c2 100644 > --- a/arch/arm/mach-lpc32xx/cpu.c > +++ b/arch/arm/mach-lpc32xx/cpu.c > @@ -22,23 +22,12 @@ void reset_cpu(ulong addr) > /* Enable watchdog clock */ > setbits_le32(&clk->timclk_ctrl, CLK_TIMCLK_WATCHDOG); > > - /* To be compatible with the original U-Boot code: > - * addr: - 0: perform hard reset. > - * - !=0: perform a soft reset; i.e. "RESOUT_N" not > asserted). */ > - if (addr == 0) { > - /* Reset pulse length is 13005 peripheral clock frames > */ > - writel(13000, &wdt->pulse); > + /* Reset pulse length is 13005 peripheral clock frames */ > + writel(13000, &wdt->pulse); > > - /* Force WDOG_RESET2 and RESOUT_N signal active */ > - writel(WDTIM_MCTRL_RESFRC2 | WDTIM_MCTRL_RESFRC1 > - | WDTIM_MCTRL_M_RES2, &wdt->mctrl); > - } else { > - /* Force match output active */ > - writel(0x01, &wdt->emr); > - > - /* Internal reset on match output (no pulse on > "RESOUT_N") */ > - writel(WDTIM_MCTRL_M_RES1, &wdt->mctrl); > - } > + /* Force WDOG_RESET2 and RESOUT_N signal active */ > + writel(WDTIM_MCTRL_RESFRC2 | WDTIM_MCTRL_RESFRC1 | > +WDTIM_MCTRL_M_RES2, > + &wdt->mctrl); > > while (1) > /* NOP */; > -- > 2.26.2 > -- 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@denx.de
On Tue, Dec 15, 2020 at 04:47:51PM +0100, Harald Seiler wrote: > This reverts commit 576007aec9a4a5f4f3dd1f690fb26a8c05ceb75f. > > The paramter passed to reset_cpu() no longer holds a meaning as all > call-sites now pass the value 0. Thus, branching on it is essentially > dead code and will just confuse future readers. > > Revert soft-reset support and just always perform a hard-reset for now. > This is a preparation for removal of the reset_cpu() parameter across > the entire tree in a later patch. > > Fixes: 576007aec9a4 ("lpc32xx: cpu: add support for soft reset") > Cc: Sylvain Lemieux <slemieux@tycoint.com> > Signed-off-by: Harald Seiler <hws@denx.de> > Signed-off-by: Harald Seiler <hws@denx.de> Applied to u-boot/next, thanks!
diff --git a/arch/arm/mach-lpc32xx/cpu.c b/arch/arm/mach-lpc32xx/cpu.c index 32af6206056b..7378192a33c2 100644 --- a/arch/arm/mach-lpc32xx/cpu.c +++ b/arch/arm/mach-lpc32xx/cpu.c @@ -22,23 +22,12 @@ void reset_cpu(ulong addr) /* Enable watchdog clock */ setbits_le32(&clk->timclk_ctrl, CLK_TIMCLK_WATCHDOG); - /* To be compatible with the original U-Boot code: - * addr: - 0: perform hard reset. - * - !=0: perform a soft reset; i.e. "RESOUT_N" not asserted). */ - if (addr == 0) { - /* Reset pulse length is 13005 peripheral clock frames */ - writel(13000, &wdt->pulse); + /* Reset pulse length is 13005 peripheral clock frames */ + writel(13000, &wdt->pulse); - /* Force WDOG_RESET2 and RESOUT_N signal active */ - writel(WDTIM_MCTRL_RESFRC2 | WDTIM_MCTRL_RESFRC1 - | WDTIM_MCTRL_M_RES2, &wdt->mctrl); - } else { - /* Force match output active */ - writel(0x01, &wdt->emr); - - /* Internal reset on match output (no pulse on "RESOUT_N") */ - writel(WDTIM_MCTRL_M_RES1, &wdt->mctrl); - } + /* Force WDOG_RESET2 and RESOUT_N signal active */ + writel(WDTIM_MCTRL_RESFRC2 | WDTIM_MCTRL_RESFRC1 | WDTIM_MCTRL_M_RES2, + &wdt->mctrl); while (1) /* NOP */;
This reverts commit 576007aec9a4a5f4f3dd1f690fb26a8c05ceb75f. The paramter passed to reset_cpu() no longer holds a meaning as all call-sites now pass the value 0. Thus, branching on it is essentially dead code and will just confuse future readers. Revert soft-reset support and just always perform a hard-reset for now. This is a preparation for removal of the reset_cpu() parameter across the entire tree in a later patch. Fixes: 576007aec9a4 ("lpc32xx: cpu: add support for soft reset") Cc: Sylvain Lemieux <slemieux@tycoint.com> Signed-off-by: Harald Seiler <hws@denx.de> --- arch/arm/mach-lpc32xx/cpu.c | 21 +++++---------------- 1 file changed, 5 insertions(+), 16 deletions(-)