| Message ID | cover.1583328917.git.hws@denx.de |
|---|---|
| Headers | show |
| Series | ARM: Fix reset in SPL if SYSRESET is not used | expand |
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,
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, >
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(-)