On Mon, Nov 29, 2021 at 01:17:44PM +0100, Marek Behún wrote:
> On Sun, 28 Nov 2021 18:47:11 +0100
> Tommaso Merciai <tomm.merc...@gmail.com> wrote:
>
> > On Fri, Nov 26, 2021 at 07:00:21PM +0100, Marek Behún wrote:
> > > On Fri, 26 Nov 2021 18:43:31 +0100
> > > Tommaso Merciai <tomm.merc...@gmail.com> wrote:
> > >
> > > > Override env_get_location function at board level, previously dropped
> > > > down from soc.c
> > > >
> > > > References:
> > > > - commit f1575f23df1ef704051f218d5bc4aeeb20c2c542
> > > >
> > > > Signed-off-by: Tommaso Merciai <tomm.merc...@gmail.com>
> > > > ---
> > > > Changes since v1:
> > > > - Remove wrong env_get_offset function from commit
> > > >
> > > > board/freescale/imx8mp_evk/imx8mp_evk.c | 41 +++++++++++++++++++++++++
> > > > 1 file changed, 41 insertions(+)
> > > >
> > > > diff --git a/board/freescale/imx8mp_evk/imx8mp_evk.c
> > > > b/board/freescale/imx8mp_evk/imx8mp_evk.c
> > > > index 62096c24fb..6b2eeaf152 100644
> > > > --- a/board/freescale/imx8mp_evk/imx8mp_evk.c
> > > > +++ b/board/freescale/imx8mp_evk/imx8mp_evk.c
> > > > @@ -5,6 +5,7 @@
> > > >
> > > > #include <common.h>
> > > > #include <env.h>
> > > > +#include <env_internal.h>
> > > > #include <errno.h>
> > > > #include <init.h>
> > > > #include <miiphy.h>
> > > > @@ -17,6 +18,7 @@
> > > > #include <asm/arch/clock.h>
> > > > #include <asm/arch/sys_proto.h>
> > > > #include <asm/mach-imx/gpio.h>
> > > > +#include <asm/mach-imx/boot_mode.h>
> > > >
> > > > DECLARE_GLOBAL_DATA_PTR;
> > > >
> > > > @@ -32,6 +34,45 @@ static iomux_v3_cfg_t const wdog_pads[] = {
> > > > MX8MP_PAD_GPIO1_IO02__WDOG1_WDOG_B |
> > > > MUX_PAD_CTRL(WDOG_PAD_CTRL),
> > > > };
> > > >
> > > > +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
> > >
> > > Using
> > > if (IS_ENABLED(CONFIG_ENV_IS_NOWHERE))
> > > instead of #ifdefs is preferred because the compiler then warns about
> > > bugs even in the disabled codepaths (that's why checkpatch complains
> > > about #ifdefs).
> > >
> > > I know that this cannot be combined with switch() in a simple way,
> > > though.
> > >
> > > Do you need to use switch? Can't you rewrite it to use if()s and use
> > > IS_ENABLED()?
> > >
> > > Marek
> >
> > Hi Marek,
> > Thanks for your review. What do you think about this solution?
> >
> > 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;
> >
> > if (IS_ENABLED(CONFIG_ENV_IS_IN_SPI_FLASH) && dev == QSPI_BOOT) {
> > env_loc = ENVL_SPI_FLASH;
> > }
> > else if (IS_ENABLED(CONFIG_ENV_IS_IN_NAND) && dev == NAND_BOOT) {
> > env_loc = ENVL_NAND;
> > }
> > else if (IS_ENABLED(CONFIG_ENV_IS_IN_MMC)) {
> > switch (dev) {
> > case SD1_BOOT:
> > case SD2_BOOT:
> > case SD3_BOOT:
> > case MMC1_BOOT:
> > case MMC2_BOOT:
> > case MMC3_BOOT:
> > env_loc = ENVL_MMC;
> > break;
> > default:
> > break;
> > }
> > }
> > else if (IS_ENABLED(CONFIG_ENV_IS_NOWHERE)) {
> > env_loc = ENVL_MMC;
> > }
> >
> > return env_loc;
> > }
>
> Thanks, this looks ok, just run it through checkpatch to correct the
> indentation of 'case' statements and put 'else if' on the same line as
> '}':
>
> if () {
> } else if () {
> } ...
>
> Marek
Thanks Marek for your review. Sent v2.
Let me know,
Tommaso
