diff mbox series

[RFC] imx8m: Drop env_get_location for imx8mn and imx8mp

Message ID 20211118135807.325871-1-michael@amarulasolutions.com
State New
Headers
Series [RFC] imx8m: Drop env_get_location for imx8mn and imx8mp |

Commit Message

Michael Trimarchi Nov. 18, 2021, 1:58 p.m. UTC 
This function defined for two architecture is not really clean
and can be generate problem when people add a new board

Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com>
---
 arch/arm/mach-imx/imx8m/soc.c | 42 -----------------------------------
 1 file changed, 42 deletions(-)

Comments

'Abel Vesa' via Amarula Linux Nov. 18, 2021, 6:53 p.m. UTC | #1 
Hello Michael,

> -----Original Message-----
> From: U-Boot <u-boot-bounces@lists.denx.de> On Behalf Of Michael Trimarchi
> Sent: Thursday, November 18, 2021 2:58 PM
> To: Ye Li <ye.li@nxp.com>; Stefano Babic <sbabic@denx.de>; Fabio Estevam
> <festevam@gmail.com>
> Cc: u-boot@lists.denx.de; Ariel D'Alessandro <ariel.dalessandro@collabora.com>;
> linux-amarula@amarulasolutions.com; Anthony Brandon
> <anthony@amarulasolutions.com>
> Subject: [RFC PATCH] imx8m: Drop env_get_location for imx8mn and imx8mp
> 
> 
> This function defined for two architecture is not really clean
> and can be generate problem when people add a new board

Not sure what you mean by "not clean" and "can generate problems" here,
perhaps you can elaborate that more in commit message?

> 
> Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com>
> ---
>  arch/arm/mach-imx/imx8m/soc.c | 42 -----------------------------------

Quick git grep shows that there are boards that have it implemented,
but only two CPU families have them:
- arch/arm/cpu/armv8/fsl-layerscape/cpu.c
- arch/arm/mach-imx/imx8m/soc.c

- board/aristainetos/aristainetos.c
- board/engicam/stm32mp1/stm32mp1.c
 -board/renesas/alt/alt.c
- board/renesas/gose/gose.c
- board/renesas/koelsch/koelsch.c
- board/renesas/lager/lager.c
- board/renesas/porter/porter.c
- board/renesas/silk/silk.c
- board/renesas/stout/stout.c
- board/sandbox/sandbox.c
- board/st/stm32mp1/stm32mp1.c
- board/sunxi/board.c
- board/xilinx/versal/board.c
- board/xilinx/zynq/board.c
- board/xilinx/zynqmp/zynqmp.c

If it does interfere with certain things you do - why not moving
it to imx8mn-evk and imx8mp-evk instead? At least this would give
adopters a hint, as EVKs are normally a reference target.

-- andrey
Michael Trimarchi Nov. 18, 2021, 9:48 p.m. UTC | #2 
Hi ZHIZHIKIN

On Thu, Nov 18, 2021 at 7:53 PM ZHIZHIKIN Andrey
<andrey.zhizhikin@leica-geosystems.com> wrote:
>
> Hello Michael,
>
> > -----Original Message-----
> > From: U-Boot <u-boot-bounces@lists.denx.de> On Behalf Of Michael Trimarchi
> > Sent: Thursday, November 18, 2021 2:58 PM
> > To: Ye Li <ye.li@nxp.com>; Stefano Babic <sbabic@denx.de>; Fabio Estevam
> > <festevam@gmail.com>
> > Cc: u-boot@lists.denx.de; Ariel D'Alessandro <ariel.dalessandro@collabora.com>;
> > linux-amarula@amarulasolutions.com; Anthony Brandon
> > <anthony@amarulasolutions.com>
> > Subject: [RFC PATCH] imx8m: Drop env_get_location for imx8mn and imx8mp
> >
> >
> > This function defined for two architecture is not really clean
> > and can be generate problem when people add a new board
>
> Not sure what you mean by "not clean" and "can generate problems" here,
> perhaps you can elaborate that more in commit message?
>

Sorry, I will adjust the commit message. This should be done at board
level, that
would like to define the enviroment from it's own config or from some specific
setup.

> >
> > Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com>
> > ---
> >  arch/arm/mach-imx/imx8m/soc.c | 42 -----------------------------------
>
> Quick git grep shows that there are boards that have it implemented,
> but only two CPU families have them:
> - arch/arm/cpu/armv8/fsl-layerscape/cpu.c
> - arch/arm/mach-imx/imx8m/soc.c
>
> - board/aristainetos/aristainetos.c
> - board/engicam/stm32mp1/stm32mp1.c
>  -board/renesas/alt/alt.c
> - board/renesas/gose/gose.c
> - board/renesas/koelsch/koelsch.c
> - board/renesas/lager/lager.c
> - board/renesas/porter/porter.c
> - board/renesas/silk/silk.c
> - board/renesas/stout/stout.c
> - board/sandbox/sandbox.c
> - board/st/stm32mp1/stm32mp1.c
> - board/sunxi/board.c
> - board/xilinx/versal/board.c
> - board/xilinx/zynq/board.c
> - board/xilinx/zynqmp/zynqmp.c
>
> If it does interfere with certain things you do - why not moving
> it to imx8mn-evk and imx8mp-evk instead? At least this would give
> adopters a hint, as EVKs are normally a reference target.

Yes, the patch is only involve the imx8mp and imx8mn.
For the problem is that make no sense to define an unique way for multiple
board. One board can boot from emmc and having env on spi flash etc

Michael
>
> -- andrey
'Abel Vesa' via Amarula Linux Nov. 19, 2021, 12:57 p.m. UTC | #3 
Hello Michael,

> -----Original Message-----
> From: Michael Nazzareno Trimarchi <michael@amarulasolutions.com>
> Sent: Thursday, November 18, 2021 10:48 PM
> To: ZHIZHIKIN Andrey <andrey.zhizhikin@leica-geosystems.com>
> Cc: Ye Li <ye.li@nxp.com>; Stefano Babic <sbabic@denx.de>; Fabio Estevam
> <festevam@gmail.com>; u-boot@lists.denx.de; Ariel D'Alessandro
> <ariel.dalessandro@collabora.com>; linux-amarula@amarulasolutions.com; Anthony
> Brandon <anthony@amarulasolutions.com>
> Subject: Re: [RFC PATCH] imx8m: Drop env_get_location for imx8mn and imx8mp
> 
> 
> Hi ZHIZHIKIN

It's Andrey, mail client swaps last and first names. :)

> 
> On Thu, Nov 18, 2021 at 7:53 PM ZHIZHIKIN Andrey
> <andrey.zhizhikin@leica-geosystems.com> wrote:
> >
> > Hello Michael,
> >
> > > -----Original Message-----
> > > From: U-Boot <u-boot-bounces@lists.denx.de> On Behalf Of Michael Trimarchi
> > > Sent: Thursday, November 18, 2021 2:58 PM
> > > To: Ye Li <ye.li@nxp.com>; Stefano Babic <sbabic@denx.de>; Fabio Estevam
> > > <festevam@gmail.com>
> > > Cc: u-boot@lists.denx.de; Ariel D'Alessandro
> <ariel.dalessandro@collabora.com>;
> > > linux-amarula@amarulasolutions.com; Anthony Brandon
> > > <anthony@amarulasolutions.com>
> > > Subject: [RFC PATCH] imx8m: Drop env_get_location for imx8mn and imx8mp
> > >
> > >
> > > This function defined for two architecture is not really clean
> > > and can be generate problem when people add a new board
> >
> > Not sure what you mean by "not clean" and "can generate problems" here,
> > perhaps you can elaborate that more in commit message?
> >
> 
> Sorry, I will adjust the commit message. This should be done at board
> level, that
> would like to define the enviroment from it's own config or from some specific
> setup.

From your other patch I've seen on ML [1], combined with this one - it looks
like you're trying to revert 2707faf01f ("imx8mn/imx8mp: override env_get_offset
and env_get_location"). If you go ahead with both patches, then I suggest you
submit a revert instead.

> 
> > >
> > > Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com>
> > > ---
> > >  arch/arm/mach-imx/imx8m/soc.c | 42 -----------------------------------
> >
> > Quick git grep shows that there are boards that have it implemented,
> > but only two CPU families have them:
> > - arch/arm/cpu/armv8/fsl-layerscape/cpu.c
> > - arch/arm/mach-imx/imx8m/soc.c
> >
> > - board/aristainetos/aristainetos.c
> > - board/engicam/stm32mp1/stm32mp1.c
> >  -board/renesas/alt/alt.c
> > - board/renesas/gose/gose.c
> > - board/renesas/koelsch/koelsch.c
> > - board/renesas/lager/lager.c
> > - board/renesas/porter/porter.c
> > - board/renesas/silk/silk.c
> > - board/renesas/stout/stout.c
> > - board/sandbox/sandbox.c
> > - board/st/stm32mp1/stm32mp1.c
> > - board/sunxi/board.c
> > - board/xilinx/versal/board.c
> > - board/xilinx/zynq/board.c
> > - board/xilinx/zynqmp/zynqmp.c
> >
> > If it does interfere with certain things you do - why not moving
> > it to imx8mn-evk and imx8mp-evk instead? At least this would give
> > adopters a hint, as EVKs are normally a reference target.
> 
> Yes, the patch is only involve the imx8mp and imx8mn.
> For the problem is that make no sense to define an unique way for multiple
> board. One board can boot from emmc and having env on spi flash etc

I see, but this is rather a specialization to me. How can one ensure
that the env deployed on another storage device is valid for the media
you're booting from?

Anyways, I suggest that this function is kept in both imx8mn and imx8mp
evk boards instead of being completely dropped.

> 
> Michael
> >
> > -- andrey
> 
> 
> 
> --
> Michael Nazzareno Trimarchi
> Co-Founder & Chief Executive Officer
> M. +39 347 913 2170
> michael@amarulasolutions.com
> __________________________________
> 
> Amarula Solutions BV
> Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
> T. +31 (0)85 111 9172
> info@amarulasolutions.com

Link: [1]: http://patchwork.ozlabs.org/project/uboot/patch/20211117143456.34441-1-michael@amarulasolutions.com/

-- andrey
diff mbox series

Patch

diff --git a/arch/arm/mach-imx/imx8m/soc.c b/arch/arm/mach-imx/imx8m/soc.c
index e75d2fa255..83a515c973 100644
--- a/arch/arm/mach-imx/imx8m/soc.c
+++ b/arch/arm/mach-imx/imx8m/soc.c
@@ -1311,45 +1311,3 @@  void do_error(struct pt_regs *pt_regs, unsigned int esr)
 }
 #endif
 #endif
-
-#if defined(CONFIG_IMX8MN) || defined(CONFIG_IMX8MP)
-enum env_location env_get_location(enum env_operation op, int prio)
-{
-	enum boot_device dev = get_boot_device();
-	enum env_location env_loc = ENVL_UNKNOWN;
-
-	if (prio)
-		return env_loc;
-
-	switch (dev) {
-#ifdef CONFIG_ENV_IS_IN_SPI_FLASH
-	case QSPI_BOOT:
-		env_loc = ENVL_SPI_FLASH;
-		break;
-#endif
-#ifdef CONFIG_ENV_IS_IN_NAND
-	case NAND_BOOT:
-		env_loc = ENVL_NAND;
-		break;
-#endif
-#ifdef CONFIG_ENV_IS_IN_MMC
-	case SD1_BOOT:
-	case SD2_BOOT:
-	case SD3_BOOT:
-	case MMC1_BOOT:
-	case MMC2_BOOT:
-	case MMC3_BOOT:
-		env_loc =  ENVL_MMC;
-		break;
-#endif
-	default:
-#if defined(CONFIG_ENV_IS_NOWHERE)
-		env_loc = ENVL_NOWHERE;
-#endif
-		break;
-	}
-
-	return env_loc;
-}
-
-#endif