public inbox for u-boot@lists.denx.de
 help / color / mirror / Atom feed
* [PATCH 0/3] ARM: Fix reset in SPL if SYSRESET is not used
@ 2020-03-04 14:23 Harald Seiler
  2020-03-04 14:23 ` [PATCH 1/3] ARM: reset: use do_reset in SPL/TPL if SYSRESET was not enabled for them Harald Seiler
                   ` (3 more replies)
  0 siblings, 4 replies; 13+ messages in thread
From: Harald Seiler @ 2020-03-04 14:23 UTC (permalink / raw)
  To: u-boot

Hello,

continuing on the discussion around Claudius' patch for fixing reset in SPL [1]
we have taken a closer look at the issue.  To quickly summarize the situation:

    The original patch was to enable the generic ARM implementation of
    `do_reset` if CONFIG_SYSRESET is not enabled in SPL.  This would break
    compilation for some boards which define their own `do_reset` in
    `board/*/spl.c`.

    To be more specific, the following 4 boards have this custom `do_reset`:

        - toradex/verdin-imx8mm
        - freescale/imx8mn_evk
        - freescale/imx8mm_evk
        - freescale/imx8mp_evk

I hope we can all agree that `do_reset` is not at all meant to be implemented
in board files.  From looking at the related code for imx8m, it feels like this
was just a workaround hack to archieve the same thing which Claudius has fixed.
So this patch series reverts the addition of `do_reset` implementations in
imx8m board files and instead switches to using the proper fix provided by
Claudius.


Additionally, the custom do_reset implementations were passing an address
(WDOG1_BASE_ADDR) to `reset_cpu()` instead of 0.  This is the only place in the
entire U-Boot tree where this happens.  Instead, all other implementations
simply ignore the parameter and 0 is passed by callers.  It looks a lot like
this is a legacy left-over which makes me think that using it for a (hard-coded)
watchdog address is not a good idea as it breaks convention with the rest of
U-Boot.

[1]: https://patchwork.ozlabs.org/patch/1201959 

Claudius Heine (3):
  ARM: reset: use do_reset in SPL/TPL if SYSRESET was not enabled for
    them
  imx: imx8m*: Remove do_reset from board files
  imx: imx8m: Don't use the addr parameter of reset_cpu

 arch/arm/lib/Makefile             | 2 +-
 arch/arm/mach-imx/imx8m/soc.c     | 5 +----
 board/freescale/imx8mm_evk/spl.c  | 9 ---------
 board/freescale/imx8mn_evk/spl.c  | 9 ---------
 board/freescale/imx8mp_evk/spl.c  | 9 ---------
 board/toradex/verdin-imx8mm/spl.c | 9 ---------
 6 files changed, 2 insertions(+), 41 deletions(-)

-- 
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 at denx.de

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 1/3] ARM: reset: use do_reset in SPL/TPL if SYSRESET was not enabled for them
  2020-03-04 14:23 [PATCH 0/3] ARM: Fix reset in SPL if SYSRESET is not used Harald Seiler
@ 2020-03-04 14:23 ` Harald Seiler
  2020-03-04 14:26   ` Marek Vasut
  2020-05-01 16:31   ` sbabic at denx.de
  2020-03-04 14:23 ` [PATCH 2/3] imx: imx8m*: Remove do_reset from board files Harald Seiler
                   ` (2 subsequent siblings)
  3 siblings, 2 replies; 13+ messages in thread
From: Harald Seiler @ 2020-03-04 14:23 UTC (permalink / raw)
  To: u-boot

From: Claudius Heine <ch@denx.de>

In case CONFIG_SYSRESET is set, do_reset from reset.c will not be available
anywere, even if SYSRESET is disabled for SPL/TPL.

'do_reset' is called from SPL for instance from the panic handler and
PANIC_HANG is not set

Signed-off-by: Claudius Heine <ch@denx.de>
---
 arch/arm/lib/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/lib/Makefile b/arch/arm/lib/Makefile
index 8482f5446c5c..b839aa7a5096 100644
--- a/arch/arm/lib/Makefile
+++ b/arch/arm/lib/Makefile
@@ -57,7 +57,7 @@ obj-y	+= interrupts_64.o
 else
 obj-y	+= interrupts.o
 endif
-ifndef CONFIG_SYSRESET
+ifndef CONFIG_$(SPL_TPL_)SYSRESET
 obj-y	+= reset.o
 endif
 
-- 
2.24.1

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 2/3] imx: imx8m*: Remove do_reset from board files
  2020-03-04 14:23 [PATCH 0/3] ARM: Fix reset in SPL if SYSRESET is not used Harald Seiler
  2020-03-04 14:23 ` [PATCH 1/3] ARM: reset: use do_reset in SPL/TPL if SYSRESET was not enabled for them Harald Seiler
@ 2020-03-04 14:23 ` Harald Seiler
  2020-03-04 14:29   ` Marek Vasut
  2020-05-01 16:33   ` sbabic at denx.de
  2020-03-04 14:23 ` [PATCH 3/3] imx: imx8m: Don't use the addr parameter of reset_cpu Harald Seiler
  2020-04-23 11:18 ` [PATCH 0/3] ARM: Fix reset in SPL if SYSRESET is not used Harald Seiler
  3 siblings, 2 replies; 13+ messages in thread
From: Harald Seiler @ 2020-03-04 14:23 UTC (permalink / raw)
  To: u-boot

From: Claudius Heine <ch@denx.de>

Use the `do_reset` implementation of `arch/arm/lib/reset.c` in SPL
instead.  It is very close to what is done here, anyway, and plays
more nicely with the rest of U-Boot than adding a custom `do_reset`
implementation into board files.

`do_reset` from `arch/arm/lib/reset.c` calls `reset_cpu` with 0 as the
addr parameter while the boards are passing WDOG1_BASE_ADDR.  This is
ok because the `reset_cpu` implementation uses WDOG1_BASE_ADDR by
default if 0 is passed in.

Co-Authored-by: Harald Seiler <hws@denx.de>
Signed-off-by: Claudius Heine <ch@denx.de>
Signed-off-by: Harald Seiler <hws@denx.de>
---
 board/freescale/imx8mm_evk/spl.c  | 9 ---------
 board/freescale/imx8mn_evk/spl.c  | 9 ---------
 board/freescale/imx8mp_evk/spl.c  | 9 ---------
 board/toradex/verdin-imx8mm/spl.c | 9 ---------
 4 files changed, 36 deletions(-)

diff --git a/board/freescale/imx8mm_evk/spl.c b/board/freescale/imx8mm_evk/spl.c
index 5d17f397cb68..4d34622465b3 100644
--- a/board/freescale/imx8mm_evk/spl.c
+++ b/board/freescale/imx8mm_evk/spl.c
@@ -161,12 +161,3 @@ void board_init_f(ulong dummy)
 
 	board_init_r(NULL, 0);
 }
-
-int do_reset(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
-{
-	puts ("resetting ...\n");
-
-	reset_cpu(WDOG1_BASE_ADDR);
-
-	return 0;
-}
diff --git a/board/freescale/imx8mn_evk/spl.c b/board/freescale/imx8mn_evk/spl.c
index 7aed14c52b68..45417b24464d 100644
--- a/board/freescale/imx8mn_evk/spl.c
+++ b/board/freescale/imx8mn_evk/spl.c
@@ -114,12 +114,3 @@ void board_init_f(ulong dummy)
 
 	board_init_r(NULL, 0);
 }
-
-int do_reset(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
-{
-	puts("resetting ...\n");
-
-	reset_cpu(WDOG1_BASE_ADDR);
-
-	return 0;
-}
diff --git a/board/freescale/imx8mp_evk/spl.c b/board/freescale/imx8mp_evk/spl.c
index 0b20668e2b30..39c1dae684ac 100644
--- a/board/freescale/imx8mp_evk/spl.c
+++ b/board/freescale/imx8mp_evk/spl.c
@@ -149,12 +149,3 @@ void board_init_f(ulong dummy)
 
 	board_init_r(NULL, 0);
 }
-
-int do_reset(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
-{
-	puts("resetting ...\n");
-
-	reset_cpu(WDOG1_BASE_ADDR);
-
-	return 0;
-}
diff --git a/board/toradex/verdin-imx8mm/spl.c b/board/toradex/verdin-imx8mm/spl.c
index a5dc54082054..dc5bd84f332e 100644
--- a/board/toradex/verdin-imx8mm/spl.c
+++ b/board/toradex/verdin-imx8mm/spl.c
@@ -169,12 +169,3 @@ void board_init_f(ulong dummy)
 
 	board_init_r(NULL, 0);
 }
-
-int do_reset(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
-{
-	puts("resetting ...\n");
-
-	reset_cpu(WDOG1_BASE_ADDR);
-
-	return 0;
-}
-- 
2.24.1

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 3/3] imx: imx8m: Don't use the addr parameter of reset_cpu
  2020-03-04 14:23 [PATCH 0/3] ARM: Fix reset in SPL if SYSRESET is not used Harald Seiler
  2020-03-04 14:23 ` [PATCH 1/3] ARM: reset: use do_reset in SPL/TPL if SYSRESET was not enabled for them Harald Seiler
  2020-03-04 14:23 ` [PATCH 2/3] imx: imx8m*: Remove do_reset from board files Harald Seiler
@ 2020-03-04 14:23 ` Harald Seiler
  2020-03-04 14:30   ` Marek Vasut
  2020-05-01 16:32   ` sbabic at denx.de
  2020-04-23 11:18 ` [PATCH 0/3] ARM: Fix reset in SPL if SYSRESET is not used Harald Seiler
  3 siblings, 2 replies; 13+ messages in thread
From: Harald Seiler @ 2020-03-04 14:23 UTC (permalink / raw)
  To: u-boot

From: Claudius Heine <ch@denx.de>

imx8m has the only implementation of `reset_cpu` which does not ignore
the addr parameter and instead gives it some meaning as the base address
of watchdog registers.  This breaks convention with the rest of U-Boot
where the parameter is ignored and callers are passing in 0.

Fixes: d2041725e84b ("imx8m: restrict reset_cpu")
Co-Authored-by: Harald Seiler <hws@denx.de>
Signed-off-by: Claudius Heine <ch@denx.de>
Signed-off-by: Harald Seiler <hws@denx.de>
---
 arch/arm/mach-imx/imx8m/soc.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/arch/arm/mach-imx/imx8m/soc.c b/arch/arm/mach-imx/imx8m/soc.c
index 7fcbd53f3020..2d3afc61a452 100644
--- a/arch/arm/mach-imx/imx8m/soc.c
+++ b/arch/arm/mach-imx/imx8m/soc.c
@@ -385,10 +385,7 @@ int ft_system_setup(void *blob, bd_t *bd)
 #if defined(CONFIG_SPL_BUILD) || !defined(CONFIG_SYSRESET)
 void reset_cpu(ulong addr)
 {
-       struct watchdog_regs *wdog = (struct watchdog_regs *)addr;
-
-       if (!addr)
-	       wdog = (struct watchdog_regs *)WDOG1_BASE_ADDR;
+       struct watchdog_regs *wdog = (struct watchdog_regs *)WDOG1_BASE_ADDR;
 
        /* Clear WDA to trigger WDOG_B immediately */
        writew((WCR_WDE | WCR_SRS), &wdog->wcr);
-- 
2.24.1

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 1/3] ARM: reset: use do_reset in SPL/TPL if SYSRESET was not enabled for them
  2020-03-04 14:23 ` [PATCH 1/3] ARM: reset: use do_reset in SPL/TPL if SYSRESET was not enabled for them Harald Seiler
@ 2020-03-04 14:26   ` Marek Vasut
  2020-05-01 16:31   ` sbabic at denx.de
  1 sibling, 0 replies; 13+ messages in thread
From: Marek Vasut @ 2020-03-04 14:26 UTC (permalink / raw)
  To: u-boot

On 3/4/20 3:23 PM, Harald Seiler wrote:
> From: Claudius Heine <ch@denx.de>
> 
> In case CONFIG_SYSRESET is set, do_reset from reset.c will not be available
> anywere, even if SYSRESET is disabled for SPL/TPL.
> 
> 'do_reset' is called from SPL for instance from the panic handler and
> PANIC_HANG is not set
> 
> Signed-off-by: Claudius Heine <ch@denx.de>

Reviewed-by: Marek Vasut <marex@denx.de>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 2/3] imx: imx8m*: Remove do_reset from board files
  2020-03-04 14:23 ` [PATCH 2/3] imx: imx8m*: Remove do_reset from board files Harald Seiler
@ 2020-03-04 14:29   ` Marek Vasut
  2020-05-01 16:33   ` sbabic at denx.de
  1 sibling, 0 replies; 13+ messages in thread
From: Marek Vasut @ 2020-03-04 14:29 UTC (permalink / raw)
  To: u-boot

On 3/4/20 3:23 PM, Harald Seiler wrote:
> From: Claudius Heine <ch@denx.de>
> 
> Use the `do_reset` implementation of `arch/arm/lib/reset.c` in SPL
> instead.  It is very close to what is done here, anyway, and plays
> more nicely with the rest of U-Boot than adding a custom `do_reset`
> implementation into board files.
> 
> `do_reset` from `arch/arm/lib/reset.c` calls `reset_cpu` with 0 as the
> addr parameter while the boards are passing WDOG1_BASE_ADDR.  This is
> ok because the `reset_cpu` implementation uses WDOG1_BASE_ADDR by
> default if 0 is passed in.
> 
> Co-Authored-by: Harald Seiler <hws@denx.de>
> Signed-off-by: Claudius Heine <ch@denx.de>
> Signed-off-by: Harald Seiler <hws@denx.de>
> ---
>  board/freescale/imx8mm_evk/spl.c  | 9 ---------
>  board/freescale/imx8mn_evk/spl.c  | 9 ---------
>  board/freescale/imx8mp_evk/spl.c  | 9 ---------
>  board/toradex/verdin-imx8mm/spl.c | 9 ---------
>  4 files changed, 36 deletions(-)

Reviewed-by: Marek Vasut <marex@denx.de>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 3/3] imx: imx8m: Don't use the addr parameter of reset_cpu
  2020-03-04 14:23 ` [PATCH 3/3] imx: imx8m: Don't use the addr parameter of reset_cpu Harald Seiler
@ 2020-03-04 14:30   ` Marek Vasut
  2020-03-04 14:35     ` Harald Seiler
  2020-05-01 16:32   ` sbabic at denx.de
  1 sibling, 1 reply; 13+ messages in thread
From: Marek Vasut @ 2020-03-04 14:30 UTC (permalink / raw)
  To: u-boot

On 3/4/20 3:23 PM, Harald Seiler wrote:
> From: Claudius Heine <ch@denx.de>
> 
> imx8m has the only implementation of `reset_cpu` which does not ignore
> the addr parameter and instead gives it some meaning as the base address
> of watchdog registers.  This breaks convention with the rest of U-Boot
> where the parameter is ignored and callers are passing in 0.
> 
> Fixes: d2041725e84b ("imx8m: restrict reset_cpu")
> Co-Authored-by: Harald Seiler <hws@denx.de>
> Signed-off-by: Claudius Heine <ch@denx.de>
> Signed-off-by: Harald Seiler <hws@denx.de>
> ---
>  arch/arm/mach-imx/imx8m/soc.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/arch/arm/mach-imx/imx8m/soc.c b/arch/arm/mach-imx/imx8m/soc.c
> index 7fcbd53f3020..2d3afc61a452 100644
> --- a/arch/arm/mach-imx/imx8m/soc.c
> +++ b/arch/arm/mach-imx/imx8m/soc.c
> @@ -385,10 +385,7 @@ int ft_system_setup(void *blob, bd_t *bd)
>  #if defined(CONFIG_SPL_BUILD) || !defined(CONFIG_SYSRESET)
>  void reset_cpu(ulong addr)
>  {
> -       struct watchdog_regs *wdog = (struct watchdog_regs *)addr;
> -
> -       if (!addr)
> -	       wdog = (struct watchdog_regs *)WDOG1_BASE_ADDR;
> +       struct watchdog_regs *wdog = (struct watchdog_regs *)WDOG1_BASE_ADDR;
>  
>         /* Clear WDA to trigger WDOG_B immediately */
>         writew((WCR_WDE | WCR_SRS), &wdog->wcr);

Can't we remove the param altogether ?

Otherwise
Reviewed-by: Marek Vasut <marex@denx.de>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 3/3] imx: imx8m: Don't use the addr parameter of reset_cpu
  2020-03-04 14:30   ` Marek Vasut
@ 2020-03-04 14:35     ` Harald Seiler
  0 siblings, 0 replies; 13+ messages in thread
From: Harald Seiler @ 2020-03-04 14:35 UTC (permalink / raw)
  To: u-boot

Hello Marek,

On Wed, 2020-03-04 at 15:30 +0100, Marek Vasut wrote:
> On 3/4/20 3:23 PM, Harald Seiler wrote:
> > From: Claudius Heine <ch@denx.de>
> > 
> > imx8m has the only implementation of `reset_cpu` which does not ignore
> > the addr parameter and instead gives it some meaning as the base address
> > of watchdog registers.  This breaks convention with the rest of U-Boot
> > where the parameter is ignored and callers are passing in 0.
> > 
> > Fixes: d2041725e84b ("imx8m: restrict reset_cpu")
> > Co-Authored-by: Harald Seiler <hws@denx.de>
> > Signed-off-by: Claudius Heine <ch@denx.de>
> > Signed-off-by: Harald Seiler <hws@denx.de>
> > ---
> >  arch/arm/mach-imx/imx8m/soc.c | 5 +----
> >  1 file changed, 1 insertion(+), 4 deletions(-)
> > 
> > diff --git a/arch/arm/mach-imx/imx8m/soc.c b/arch/arm/mach-imx/imx8m/soc.c
> > index 7fcbd53f3020..2d3afc61a452 100644
> > --- a/arch/arm/mach-imx/imx8m/soc.c
> > +++ b/arch/arm/mach-imx/imx8m/soc.c
> > @@ -385,10 +385,7 @@ int ft_system_setup(void *blob, bd_t *bd)
> >  #if defined(CONFIG_SPL_BUILD) || !defined(CONFIG_SYSRESET)
> >  void reset_cpu(ulong addr)
> >  {
> > -       struct watchdog_regs *wdog = (struct watchdog_regs *)addr;
> > -
> > -       if (!addr)
> > -	       wdog = (struct watchdog_regs *)WDOG1_BASE_ADDR;
> > +       struct watchdog_regs *wdog = (struct watchdog_regs *)WDOG1_BASE_ADDR;
> >  
> >         /* Clear WDA to trigger WDOG_B immediately */
> >         writew((WCR_WDE | WCR_SRS), &wdog->wcr);
> 
> Can't we remove the param altogether ?

Yes, that is what I would suggest as well.  Although I'd do that as
a separate follow up because it is not ARM nor i.MX specific.

> Otherwise
> Reviewed-by: Marek Vasut <marex@denx.de>
-- 
Harald

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 0/3] ARM: Fix reset in SPL if SYSRESET is not used
  2020-03-04 14:23 [PATCH 0/3] ARM: Fix reset in SPL if SYSRESET is not used Harald Seiler
                   ` (2 preceding siblings ...)
  2020-03-04 14:23 ` [PATCH 3/3] imx: imx8m: Don't use the addr parameter of reset_cpu Harald Seiler
@ 2020-04-23 11:18 ` Harald Seiler
  2020-04-23 11:23   ` Stefano Babic
  3 siblings, 1 reply; 13+ messages in thread
From: Harald Seiler @ 2020-04-23 11:18 UTC (permalink / raw)
  To: u-boot

Hello,

On Wed, 2020-03-04 at 15:23 +0100, Harald Seiler wrote:
> Hello,
> 
> continuing on the discussion around Claudius' patch for fixing reset in SPL [1]
> we have taken a closer look at the issue.  To quickly summarize the situation:
> 
>     The original patch was to enable the generic ARM implementation of
>     `do_reset` if CONFIG_SYSRESET is not enabled in SPL.  This would break
>     compilation for some boards which define their own `do_reset` in
>     `board/*/spl.c`.
> 
>     To be more specific, the following 4 boards have this custom `do_reset`:
> 
>         - toradex/verdin-imx8mm
>         - freescale/imx8mn_evk
>         - freescale/imx8mm_evk
>         - freescale/imx8mp_evk
> 
> I hope we can all agree that `do_reset` is not at all meant to be implemented
> in board files.  From looking at the related code for imx8m, it feels like this
> was just a workaround hack to archieve the same thing which Claudius has fixed.
> So this patch series reverts the addition of `do_reset` implementations in
> imx8m board files and instead switches to using the proper fix provided by
> Claudius.
> 
> 
> Additionally, the custom do_reset implementations were passing an address
> (WDOG1_BASE_ADDR) to `reset_cpu()` instead of 0.  This is the only place in the
> entire U-Boot tree where this happens.  Instead, all other implementations
> simply ignore the parameter and 0 is passed by callers.  It looks a lot like
> this is a legacy left-over which makes me think that using it for a (hard-coded)
> watchdog address is not a good idea as it breaks convention with the rest of
> U-Boot.
> 
> [1]: https://patchwork.ozlabs.org/patch/1201959 
> 
> Claudius Heine (3):
>   ARM: reset: use do_reset in SPL/TPL if SYSRESET was not enabled for
>     them
>   imx: imx8m*: Remove do_reset from board files
>   imx: imx8m: Don't use the addr parameter of reset_cpu
> 
>  arch/arm/lib/Makefile             | 2 +-
>  arch/arm/mach-imx/imx8m/soc.c     | 5 +----
>  board/freescale/imx8mm_evk/spl.c  | 9 ---------
>  board/freescale/imx8mn_evk/spl.c  | 9 ---------
>  board/freescale/imx8mp_evk/spl.c  | 9 ---------
>  board/toradex/verdin-imx8mm/spl.c | 9 ---------
>  6 files changed, 2 insertions(+), 41 deletions(-)
> 

Quick question, what is the situation with this series?  Is there anything
which needs to be addressed?

Without it, CONFIG_SPL_USB_SDP_SUPPORT is broken on the imx6q hardware
I am working with and I guess the same issue might exist on many other
boards as well (The USB stack needs a do_reset() implementation in SPL).

Regards,
-- 
Harald

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 0/3] ARM: Fix reset in SPL if SYSRESET is not used
  2020-04-23 11:18 ` [PATCH 0/3] ARM: Fix reset in SPL if SYSRESET is not used Harald Seiler
@ 2020-04-23 11:23   ` Stefano Babic
  0 siblings, 0 replies; 13+ messages in thread
From: Stefano Babic @ 2020-04-23 11:23 UTC (permalink / raw)
  To: u-boot

On 23.04.20 13:18, Harald Seiler wrote:
> Hello,
> 
> On Wed, 2020-03-04 at 15:23 +0100, Harald Seiler wrote:
>> Hello,
>>
>> continuing on the discussion around Claudius' patch for fixing reset in SPL [1]
>> we have taken a closer look at the issue.  To quickly summarize the situation:
>>
>>     The original patch was to enable the generic ARM implementation of
>>     `do_reset` if CONFIG_SYSRESET is not enabled in SPL.  This would break
>>     compilation for some boards which define their own `do_reset` in
>>     `board/*/spl.c`.
>>
>>     To be more specific, the following 4 boards have this custom `do_reset`:
>>
>>         - toradex/verdin-imx8mm
>>         - freescale/imx8mn_evk
>>         - freescale/imx8mm_evk
>>         - freescale/imx8mp_evk
>>
>> I hope we can all agree that `do_reset` is not at all meant to be implemented
>> in board files.  From looking at the related code for imx8m, it feels like this
>> was just a workaround hack to archieve the same thing which Claudius has fixed.
>> So this patch series reverts the addition of `do_reset` implementations in
>> imx8m board files and instead switches to using the proper fix provided by
>> Claudius.
>>
>>
>> Additionally, the custom do_reset implementations were passing an address
>> (WDOG1_BASE_ADDR) to `reset_cpu()` instead of 0.  This is the only place in the
>> entire U-Boot tree where this happens.  Instead, all other implementations
>> simply ignore the parameter and 0 is passed by callers.  It looks a lot like
>> this is a legacy left-over which makes me think that using it for a (hard-coded)
>> watchdog address is not a good idea as it breaks convention with the rest of
>> U-Boot.
>>
>> [1]: https://patchwork.ozlabs.org/patch/1201959 
>>
>> Claudius Heine (3):
>>   ARM: reset: use do_reset in SPL/TPL if SYSRESET was not enabled for
>>     them
>>   imx: imx8m*: Remove do_reset from board files
>>   imx: imx8m: Don't use the addr parameter of reset_cpu
>>
>>  arch/arm/lib/Makefile             | 2 +-
>>  arch/arm/mach-imx/imx8m/soc.c     | 5 +----
>>  board/freescale/imx8mm_evk/spl.c  | 9 ---------
>>  board/freescale/imx8mn_evk/spl.c  | 9 ---------
>>  board/freescale/imx8mp_evk/spl.c  | 9 ---------
>>  board/toradex/verdin-imx8mm/spl.c | 9 ---------
>>  6 files changed, 2 insertions(+), 41 deletions(-)
>>
> 
> Quick question, what is the situation with this series?  Is there anything
> which needs to be addressed?

Patches are fine IMHO - I will merge them soon.

Stefano

> 
> Without it, CONFIG_SPL_USB_SDP_SUPPORT is broken on the imx6q hardware
> I am working with and I guess the same issue might exist on many other
> boards as well (The USB stack needs a do_reset() implementation in SPL).
> 
> Regards,
> 


-- 
=====================================================================
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 1/3] ARM: reset: use do_reset in SPL/TPL if SYSRESET was not enabled for them
  2020-03-04 14:23 ` [PATCH 1/3] ARM: reset: use do_reset in SPL/TPL if SYSRESET was not enabled for them Harald Seiler
  2020-03-04 14:26   ` Marek Vasut
@ 2020-05-01 16:31   ` sbabic at denx.de
  1 sibling, 0 replies; 13+ messages in thread
From: sbabic at denx.de @ 2020-05-01 16:31 UTC (permalink / raw)
  To: u-boot

> From: Claudius Heine <ch@denx.de>
> In case CONFIG_SYSRESET is set, do_reset from reset.c will not be available
> anywere, even if SYSRESET is disabled for SPL/TPL.
> 'do_reset' is called from SPL for instance from the panic handler and
> PANIC_HANG is not set
> Signed-off-by: Claudius Heine <ch@denx.de>
> Reviewed-by: Marek Vasut <marex@denx.de>
Applied to u-boot-imx, master, thanks !

Best regards,
Stefano Babic

-- 
=====================================================================
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 3/3] imx: imx8m: Don't use the addr parameter of reset_cpu
  2020-03-04 14:23 ` [PATCH 3/3] imx: imx8m: Don't use the addr parameter of reset_cpu Harald Seiler
  2020-03-04 14:30   ` Marek Vasut
@ 2020-05-01 16:32   ` sbabic at denx.de
  1 sibling, 0 replies; 13+ messages in thread
From: sbabic at denx.de @ 2020-05-01 16:32 UTC (permalink / raw)
  To: u-boot

> From: Claudius Heine <ch@denx.de>
> imx8m has the only implementation of `reset_cpu` which does not ignore
> the addr parameter and instead gives it some meaning as the base address
> of watchdog registers.  This breaks convention with the rest of U-Boot
> where the parameter is ignored and callers are passing in 0.
> Fixes: d2041725e84b ("imx8m: restrict reset_cpu")
> Co-Authored-by: Harald Seiler <hws@denx.de>
> Signed-off-by: Claudius Heine <ch@denx.de>
> Signed-off-by: Harald Seiler <hws@denx.de>
> Reviewed-by: Marek Vasut <marex@denx.de>
Applied to u-boot-imx, master, thanks !

Best regards,
Stefano Babic

-- 
=====================================================================
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 2/3] imx: imx8m*: Remove do_reset from board files
  2020-03-04 14:23 ` [PATCH 2/3] imx: imx8m*: Remove do_reset from board files Harald Seiler
  2020-03-04 14:29   ` Marek Vasut
@ 2020-05-01 16:33   ` sbabic at denx.de
  1 sibling, 0 replies; 13+ messages in thread
From: sbabic at denx.de @ 2020-05-01 16:33 UTC (permalink / raw)
  To: u-boot

> From: Claudius Heine <ch@denx.de>
> Use the `do_reset` implementation of `arch/arm/lib/reset.c` in SPL
> instead.  It is very close to what is done here, anyway, and plays
> more nicely with the rest of U-Boot than adding a custom `do_reset`
> implementation into board files.
> `do_reset` from `arch/arm/lib/reset.c` calls `reset_cpu` with 0 as the
> addr parameter while the boards are passing WDOG1_BASE_ADDR.  This is
> ok because the `reset_cpu` implementation uses WDOG1_BASE_ADDR by
> default if 0 is passed in.
> Co-Authored-by: Harald Seiler <hws@denx.de>
> Signed-off-by: Claudius Heine <ch@denx.de>
> Signed-off-by: Harald Seiler <hws@denx.de>
> Reviewed-by: Marek Vasut <marex@denx.de>
Applied to u-boot-imx, master, thanks !

Best regards,
Stefano Babic

-- 
=====================================================================
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2020-05-01 16:33 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-03-04 14:23 [PATCH 0/3] ARM: Fix reset in SPL if SYSRESET is not used Harald Seiler
2020-03-04 14:23 ` [PATCH 1/3] ARM: reset: use do_reset in SPL/TPL if SYSRESET was not enabled for them Harald Seiler
2020-03-04 14:26   ` Marek Vasut
2020-05-01 16:31   ` sbabic at denx.de
2020-03-04 14:23 ` [PATCH 2/3] imx: imx8m*: Remove do_reset from board files Harald Seiler
2020-03-04 14:29   ` Marek Vasut
2020-05-01 16:33   ` sbabic at denx.de
2020-03-04 14:23 ` [PATCH 3/3] imx: imx8m: Don't use the addr parameter of reset_cpu Harald Seiler
2020-03-04 14:30   ` Marek Vasut
2020-03-04 14:35     ` Harald Seiler
2020-05-01 16:32   ` sbabic at denx.de
2020-04-23 11:18 ` [PATCH 0/3] ARM: Fix reset in SPL if SYSRESET is not used Harald Seiler
2020-04-23 11:23   ` Stefano Babic

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox