diff mbox series

[RFC] imx: imx8mm-evk/imx8mn-evk: enable sysreset driver for SPL

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

Commit Message

Claudius Heine Jan. 15, 2020, 2:49 p.m. UTC
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(-)

Comments

Simon Glass Jan. 16, 2020, 12:10 a.m. UTC | #1
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>
Peng Fan Jan. 16, 2020, 2:21 a.m. UTC | #2
> 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
Marek Vasut Jan. 16, 2020, 9:27 a.m. UTC | #3
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.
Peng Fan Jan. 17, 2020, 2:33 a.m. UTC | #4
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.
Marek Vasut Jan. 17, 2020, 9:32 a.m. UTC | #5
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 ?
Peng Fan Jan. 19, 2020, 7:48 a.m. UTC | #6
> 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.
Marek Vasut Jan. 19, 2020, 2:10 p.m. UTC | #7
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 mbox series

Patch

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