| Message ID | 20200115144938.730627-1-ch@denx.de |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] imx: imx8mm-evk/imx8mn-evk: enable sysreset driver for SPL | expand |
On Thu, 16 Jan 2020 at 03:50, Claudius Heine <ch at denx.de> wrote: > > Hi, > > I have only tested compiling, but if the reset in the SPL on i.MX8MM and > i.MX8MN still works with this patch applied, then we don't need board > specific 'do_reset' function and special configurations flags for this > case. > > I currently don't have access to the hardware to test this. > > regards, > Claudius > > -- >8 -- > Subject: [RFC PATCH] imx: imx8mm-evk/imx8mn-evk: enable sysreset driver for > SPL > > Instead of implementing a custom reset function for the SPL, it can > directly use the sysreset DM driver. > > Signed-off-by: Claudius Heine <ch at denx.de> > --- > arch/arm/mach-imx/imx8m/soc.c | 19 ------------------- > board/freescale/imx8mm_evk/spl.c | 9 --------- > board/freescale/imx8mn_evk/spl.c | 9 --------- > configs/imx8mm_evk_defconfig | 1 + > configs/imx8mn_ddr4_evk_defconfig | 1 + > 5 files changed, 2 insertions(+), 37 deletions(-) Reviewed-by: Simon Glass <sjg at chromium.org>
> Subject: [RFC PATCH] imx: imx8mm-evk/imx8mn-evk: enable sysreset driver > for SPL > NAK, this will not work on i.MX8MM/N. Currently sysreset psci is enabled, however psci will not work for SPL, because SPL boots before BL31. Regards, Peng. > Hi, > > I have only tested compiling, but if the reset in the SPL on i.MX8MM and > i.MX8MN still works with this patch applied, then we don't need board > specific 'do_reset' function and special configurations flags for this case. > > I currently don't have access to the hardware to test this. > > regards, > Claudius > > -- >8 -- > Subject: [RFC PATCH] imx: imx8mm-evk/imx8mn-evk: enable sysreset driver > for SPL > > Instead of implementing a custom reset function for the SPL, it can directly > use the sysreset DM driver. > > Signed-off-by: Claudius Heine <ch at denx.de> > --- > arch/arm/mach-imx/imx8m/soc.c | 19 ------------------- > board/freescale/imx8mm_evk/spl.c | 9 --------- > board/freescale/imx8mn_evk/spl.c | 9 --------- > configs/imx8mm_evk_defconfig | 1 + > configs/imx8mn_ddr4_evk_defconfig | 1 + > 5 files changed, 2 insertions(+), 37 deletions(-) > > diff --git a/arch/arm/mach-imx/imx8m/soc.c > b/arch/arm/mach-imx/imx8m/soc.c index 5ce5a180e8..519108d4c9 100644 > --- a/arch/arm/mach-imx/imx8m/soc.c > +++ b/arch/arm/mach-imx/imx8m/soc.c > @@ -378,22 +378,3 @@ int ft_system_setup(void *blob, bd_t *bd) > return 0; > } > #endif > - > -#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; > - > - /* Clear WDA to trigger WDOG_B immediately */ > - writew((WCR_WDE | WCR_SRS), &wdog->wcr); > - > - while (1) { > - /* > - * spin for .5 seconds before reset > - */ > - } > -} > -#endif > diff --git a/board/freescale/imx8mm_evk/spl.c > b/board/freescale/imx8mm_evk/spl.c > index 2d08f9a563..e8dec452ea 100644 > --- a/board/freescale/imx8mm_evk/spl.c > +++ b/board/freescale/imx8mm_evk/spl.c > @@ -159,12 +159,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 cbde9f6b3c..0c70fbb155 100644 > --- a/board/freescale/imx8mn_evk/spl.c > +++ b/board/freescale/imx8mn_evk/spl.c > @@ -112,12 +112,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/configs/imx8mm_evk_defconfig b/configs/imx8mm_evk_defconfig > index 87560ef989..c07f4104f9 100644 > --- a/configs/imx8mm_evk_defconfig > +++ b/configs/imx8mm_evk_defconfig > @@ -83,5 +83,6 @@ CONFIG_DM_REGULATOR_FIXED=y > CONFIG_DM_REGULATOR_GPIO=y CONFIG_MXC_UART=y > CONFIG_SYSRESET=y > +CONFIG_SPL_SYSRESET=y > CONFIG_SYSRESET_PSCI=y > CONFIG_DM_THERMAL=y > diff --git a/configs/imx8mn_ddr4_evk_defconfig > b/configs/imx8mn_ddr4_evk_defconfig > index 50b03d0763..c55998da4c 100644 > --- a/configs/imx8mn_ddr4_evk_defconfig > +++ b/configs/imx8mn_ddr4_evk_defconfig > @@ -75,5 +75,6 @@ CONFIG_DM_REGULATOR_FIXED=y > CONFIG_DM_REGULATOR_GPIO=y CONFIG_MXC_UART=y > CONFIG_SYSRESET=y > +CONFIG_SPL_SYSRESET=y > CONFIG_SYSRESET_PSCI=y > CONFIG_DM_THERMAL=y > -- > 2.24.1
On 1/16/20 3:21 AM, Peng Fan wrote: Hello Peng, >> Subject: [RFC PATCH] imx: imx8mm-evk/imx8mn-evk: enable sysreset driver >> for SPL >> > > NAK, this will not work on i.MX8MM/N. > > Currently sysreset psci is enabled, however psci will not work for SPL, because > SPL boots before BL31. Thank you for the constructive feedback. So basically , what we need is a real sysreset driver for iMX8MM , which can work with bare-bones hardware interface, that is, poking some register, correct ? So, either you or Claudius needs to implement a driver in drivers/sysreset, which will bind in SPL and if needed, do some writel() into some registers to reset the board, correct ? Because, I believe we can both agree that dumping such ad-hoc code which implements reset in board code is not the right way, esp. nowadays with all the DM/DT stuff in.
Hi Marek, > Subject: Re: [RFC PATCH] imx: imx8mm-evk/imx8mn-evk: enable sysreset > driver for SPL > > On 1/16/20 3:21 AM, Peng Fan wrote: > > Hello Peng, > > >> Subject: [RFC PATCH] imx: imx8mm-evk/imx8mn-evk: enable sysreset > >> driver for SPL > >> > > > > NAK, this will not work on i.MX8MM/N. > > > > Currently sysreset psci is enabled, however psci will not work for > > SPL, because SPL boots before BL31. > > Thank you for the constructive feedback. > > So basically , what we need is a real sysreset driver for iMX8MM , which can > work with bare-bones hardware interface, that is, poking some register, > correct ? Yes. > > So, either you or Claudius needs to implement a driver in drivers/sysreset, > which will bind in SPL and if needed, do some writel() into some registers to > reset the board, correct ? Yes. > > Because, I believe we can both agree that dumping such ad-hoc code which > implements reset in board code is not the right way, esp. nowadays with all > the DM/DT stuff in. Alought we still have ocram space, but our SPL is huge now, 100KB+ Regards, Peng.
On 1/17/20 3:33 AM, Peng Fan wrote: > Hi Marek, Hi, >> Subject: Re: [RFC PATCH] imx: imx8mm-evk/imx8mn-evk: enable sysreset >> driver for SPL >> >> On 1/16/20 3:21 AM, Peng Fan wrote: >> >> Hello Peng, >> >>>> Subject: [RFC PATCH] imx: imx8mm-evk/imx8mn-evk: enable sysreset >>>> driver for SPL >>>> >>> >>> NAK, this will not work on i.MX8MM/N. >>> >>> Currently sysreset psci is enabled, however psci will not work for >>> SPL, because SPL boots before BL31. >> >> Thank you for the constructive feedback. >> >> So basically , what we need is a real sysreset driver for iMX8MM , which can >> work with bare-bones hardware interface, that is, poking some register, >> correct ? > > Yes. > >> >> So, either you or Claudius needs to implement a driver in drivers/sysreset, >> which will bind in SPL and if needed, do some writel() into some registers to >> reset the board, correct ? > > Yes. > >> >> Because, I believe we can both agree that dumping such ad-hoc code which >> implements reset in board code is not the right way, esp. nowadays with all >> the DM/DT stuff in. > > Alought we still have ocram space, but our SPL is huge now, 100KB+ How so ? How much does DM sysreset add to that ?
> Subject: Re: [RFC PATCH] imx: imx8mm-evk/imx8mn-evk: enable sysreset > driver for SPL > > On 1/17/20 3:33 AM, Peng Fan wrote: > > Hi Marek, > > Hi, > > >> Subject: Re: [RFC PATCH] imx: imx8mm-evk/imx8mn-evk: enable sysreset > >> driver for SPL > >> > >> On 1/16/20 3:21 AM, Peng Fan wrote: > >> > >> Hello Peng, > >> > >>>> Subject: [RFC PATCH] imx: imx8mm-evk/imx8mn-evk: enable sysreset > >>>> driver for SPL > >>>> > >>> > >>> NAK, this will not work on i.MX8MM/N. > >>> > >>> Currently sysreset psci is enabled, however psci will not work for > >>> SPL, because SPL boots before BL31. > >> > >> Thank you for the constructive feedback. > >> > >> So basically , what we need is a real sysreset driver for iMX8MM , > >> which can work with bare-bones hardware interface, that is, poking > >> some register, correct ? > > > > Yes. > > > >> > >> So, either you or Claudius needs to implement a driver in > >> drivers/sysreset, which will bind in SPL and if needed, do some > >> writel() into some registers to reset the board, correct ? > > > > Yes. > > > >> > >> Because, I believe we can both agree that dumping such ad-hoc code > >> which implements reset in board code is not the right way, esp. > >> nowadays with all the DM/DT stuff in. > > > > Alought we still have ocram space, but our SPL is huge now, 100KB+ > > How so ? > > How much does DM sysreset add to that ? currently I am a bit hesitated about DM SPL, but it let us not to maintain two drivers for one module. Just some complaining words:) I am fine to add DM sysreset currently. Not sure how much it will add after DM usb. Regards, Peng.
On 1/19/20 8:48 AM, Peng Fan wrote: [...] >>>> Because, I believe we can both agree that dumping such ad-hoc code >>>> which implements reset in board code is not the right way, esp. >>>> nowadays with all the DM/DT stuff in. >>> >>> Alought we still have ocram space, but our SPL is huge now, 100KB+ >> >> How so ? >> >> How much does DM sysreset add to that ? > > currently I am a bit hesitated about DM SPL, > but it let us not to maintain two drivers for one module. > Just some complaining words:) > I am fine to add DM sysreset currently. > Not sure how much it will add after DM usb. I have two things to say about this. 1) I share your concern about DM SPL, we have a massive size problem. Thus far, we don't have a solution. Patches welcome. One option might be to optimize out frameworks which only have one driver instance in SPL, like MMC_TINY does and have subsystem API directly call that one driver instance. 2) Hacking ad-hoc drivers in board/ files is broken design, so we need a solution, which does not do that. If we want to disregard DM sysreset in SPL for this board, we can only do that based on hard numbers, that is, measure the size impact and determine if that's an issue. If so, we need another solution. Otherwise, we enable DM sysreset and go back to solving 1) as a separate topic.
diff --git a/arch/arm/mach-imx/imx8m/soc.c b/arch/arm/mach-imx/imx8m/soc.c index 5ce5a180e8..519108d4c9 100644 --- a/arch/arm/mach-imx/imx8m/soc.c +++ b/arch/arm/mach-imx/imx8m/soc.c @@ -378,22 +378,3 @@ int ft_system_setup(void *blob, bd_t *bd) return 0; } #endif - -#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; - - /* Clear WDA to trigger WDOG_B immediately */ - writew((WCR_WDE | WCR_SRS), &wdog->wcr); - - while (1) { - /* - * spin for .5 seconds before reset - */ - } -} -#endif diff --git a/board/freescale/imx8mm_evk/spl.c b/board/freescale/imx8mm_evk/spl.c index 2d08f9a563..e8dec452ea 100644 --- a/board/freescale/imx8mm_evk/spl.c +++ b/board/freescale/imx8mm_evk/spl.c @@ -159,12 +159,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 cbde9f6b3c..0c70fbb155 100644 --- a/board/freescale/imx8mn_evk/spl.c +++ b/board/freescale/imx8mn_evk/spl.c @@ -112,12 +112,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/configs/imx8mm_evk_defconfig b/configs/imx8mm_evk_defconfig index 87560ef989..c07f4104f9 100644 --- a/configs/imx8mm_evk_defconfig +++ b/configs/imx8mm_evk_defconfig @@ -83,5 +83,6 @@ CONFIG_DM_REGULATOR_FIXED=y CONFIG_DM_REGULATOR_GPIO=y CONFIG_MXC_UART=y CONFIG_SYSRESET=y +CONFIG_SPL_SYSRESET=y CONFIG_SYSRESET_PSCI=y CONFIG_DM_THERMAL=y diff --git a/configs/imx8mn_ddr4_evk_defconfig b/configs/imx8mn_ddr4_evk_defconfig index 50b03d0763..c55998da4c 100644 --- a/configs/imx8mn_ddr4_evk_defconfig +++ b/configs/imx8mn_ddr4_evk_defconfig @@ -75,5 +75,6 @@ CONFIG_DM_REGULATOR_FIXED=y CONFIG_DM_REGULATOR_GPIO=y CONFIG_MXC_UART=y CONFIG_SYSRESET=y +CONFIG_SPL_SYSRESET=y CONFIG_SYSRESET_PSCI=y CONFIG_DM_THERMAL=y
Hi, I have only tested compiling, but if the reset in the SPL on i.MX8MM and i.MX8MN still works with this patch applied, then we don't need board specific 'do_reset' function and special configurations flags for this case. I currently don't have access to the hardware to test this. regards, Claudius -- >8 -- Subject: [RFC PATCH] imx: imx8mm-evk/imx8mn-evk: enable sysreset driver for SPL Instead of implementing a custom reset function for the SPL, it can directly use the sysreset DM driver. Signed-off-by: Claudius Heine <ch at denx.de> --- arch/arm/mach-imx/imx8m/soc.c | 19 ------------------- board/freescale/imx8mm_evk/spl.c | 9 --------- board/freescale/imx8mn_evk/spl.c | 9 --------- configs/imx8mm_evk_defconfig | 1 + configs/imx8mn_ddr4_evk_defconfig | 1 + 5 files changed, 2 insertions(+), 37 deletions(-)