[PATCH 0/3] ARM: Fix reset in SPL if SYSRESET is not used
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(-)
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
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
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
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; -}
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
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
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);
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
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
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
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_evkI 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.
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,
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_evkI 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.
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,
participants (4)
-
Harald Seiler -
Marek Vasut -
sbabic＠denx.de -
Stefano Babic