Re: [PATCH 3/4] Revert "lpc32xx: cpu: add support for soft reset"
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
> Signed-off-by: Harald Seiler
> Signed-off-by: Harald Seiler
Applied to u-boot/next, thanks!
--
Tom
signature.asc
Description: PGP signature
RE: [PATCH 3/4] Revert "lpc32xx: cpu: add support for soft reset"
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
Sent: Thursday, December 17, 2020 5:09 AM
To: Sylvain Lemieux ; u-boot@lists.denx.de
Cc: Tom Rini ; Simon Glass
Subject: Re: [PATCH 3/4] Revert "lpc32xx: cpu: add support for soft reset"
Hello Sylvain,
On Tue, 2020-12-15 at 17:29 +, 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
> Sent: Tuesday, December 15, 2020 10:48 AM
> To: u-boot@lists.denx.de
> Cc: Harald Seiler ; Tom Rini ; Simon
> Glass ; Sylvain Lemieux
> 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
> Signed-off-by: Harald Seiler
> ---
> 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(>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, >pulse);
> + /* Reset pulse length is 13005 peripheral clock frames */
> + writel(13000, >pulse);
>
> - /* Force WDOG_RESET2 and RESOUT_N signal active */
> - writel(WDTIM_MCTRL_RESFRC2 | WDTIM_MCTRL_RESFRC1
> - | WDTIM_MCTRL_M_RES2, >mctrl);
> - } else {
> - /* Force match output active */
> - writel(0x01, >emr);
> -
> - /* Internal reset on match output (no pulse on
> "RESOUT_N") */
> - writel(WDTIM_MCTRL_M_RES1, >mctrl);
> - }
> + /* Force WDOG_RESET2 and RESOUT_N signal active */
> + writel(WDTIM_MCTRL_RESFRC2 | WDTIM_MCTRL_RESFRC1 |
> +WDTIM_MCTRL_M_RES2,
> + >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: h...@denx.de
Re: [PATCH 3/4] Revert "lpc32xx: cpu: add support for soft reset"
Hello Sylvain,
On Tue, 2020-12-15 at 17:29 +, 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
> Sent: Tuesday, December 15, 2020 10:48 AM
> To: u-boot@lists.denx.de
> Cc: Harald Seiler ; Tom Rini ; Simon Glass
> ; Sylvain Lemieux
> 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
> Signed-off-by: Harald Seiler
> ---
> 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(>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, >pulse);
> + /* Reset pulse length is 13005 peripheral clock frames */
> + writel(13000, >pulse);
>
> - /* Force WDOG_RESET2 and RESOUT_N signal active */
> - writel(WDTIM_MCTRL_RESFRC2 | WDTIM_MCTRL_RESFRC1
> - | WDTIM_MCTRL_M_RES2, >mctrl);
> - } else {
> - /* Force match output active */
> - writel(0x01, >emr);
> -
> - /* Internal reset on match output (no pulse on "RESOUT_N") */
> - writel(WDTIM_MCTRL_M_RES1, >mctrl);
> - }
> + /* Force WDOG_RESET2 and RESOUT_N signal active */
> + writel(WDTIM_MCTRL_RESFRC2 | WDTIM_MCTRL_RESFRC1 | WDTIM_MCTRL_M_RES2,
> + >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: h...@denx.de
RE: [PATCH 3/4] Revert "lpc32xx: cpu: add support for soft reset"
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
Sent: Tuesday, December 15, 2020 10:48 AM
To: u-boot@lists.denx.de
Cc: Harald Seiler ; Tom Rini ; Simon Glass
; Sylvain Lemieux
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
Signed-off-by: Harald Seiler
---
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(>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, >pulse);
+ /* Reset pulse length is 13005 peripheral clock frames */
+ writel(13000, >pulse);
- /* Force WDOG_RESET2 and RESOUT_N signal active */
- writel(WDTIM_MCTRL_RESFRC2 | WDTIM_MCTRL_RESFRC1
- | WDTIM_MCTRL_M_RES2, >mctrl);
- } else {
- /* Force match output active */
- writel(0x01, >emr);
-
- /* Internal reset on match output (no pulse on "RESOUT_N") */
- writel(WDTIM_MCTRL_M_RES1, >mctrl);
- }
+ /* Force WDOG_RESET2 and RESOUT_N signal active */
+ writel(WDTIM_MCTRL_RESFRC2 | WDTIM_MCTRL_RESFRC1 | WDTIM_MCTRL_M_RES2,
+ >mctrl);
while (1)
/* NOP */;
--
2.26.2
[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
Signed-off-by: Harald Seiler
---
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(>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, >pulse);
+ /* Reset pulse length is 13005 peripheral clock frames */
+ writel(13000, >pulse);
- /* Force WDOG_RESET2 and RESOUT_N signal active */
- writel(WDTIM_MCTRL_RESFRC2 | WDTIM_MCTRL_RESFRC1
- | WDTIM_MCTRL_M_RES2, >mctrl);
- } else {
- /* Force match output active */
- writel(0x01, >emr);
-
- /* Internal reset on match output (no pulse on "RESOUT_N") */
- writel(WDTIM_MCTRL_M_RES1, >mctrl);
- }
+ /* Force WDOG_RESET2 and RESOUT_N signal active */
+ writel(WDTIM_MCTRL_RESFRC2 | WDTIM_MCTRL_RESFRC1 | WDTIM_MCTRL_M_RES2,
+ >mctrl);
while (1)
/* NOP */;
--
2.26.2
