Re: [PATCH v1 3/6] sandbox64: add support for NVMXIP QSPI

[Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>]

On Tue, Feb 07, 2023 at 11:38:45AM -0700, Simon Glass wrote:

Hi Simon,

> Hi,
> 
> On Tue, 7 Feb 2023 at 09:30, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Mon, Feb 06, 2023 at 09:02:38PM -0700, Simon Glass wrote:
> > > On Mon, 16 Jan 2023 at 10:28, <abdellatif.elkhlifi@arm.com> wrote:
> > > >
> > > > From: Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
> > > >
> > > > enable NVMXIP QSPI for sandbox 64-bit
> > > >
> > > > Adding two NVM XIP QSPI storage devices.
> > > >
> > > > Signed-off-by: Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
> > > > ---
> > > >  arch/sandbox/dts/sandbox64.dts | 13 +++++++++++++
> > > >  arch/sandbox/dts/test.dts      | 14 ++++++++++++++
> > > >  configs/sandbox64_defconfig    |  1 +
> > > >  drivers/nvmxip/nvmxip.c        |  4 ++++
> > > >  drivers/nvmxip/nvmxip.h        |  3 +++
> > > >  5 files changed, 35 insertions(+)
> > >
> > > Reviewed-by: Simon Glass <sjg@chromium.org>
> > >
> > > Nit below
> > >
> > > >
> > > > diff --git a/arch/sandbox/dts/sandbox64.dts b/arch/sandbox/dts/sandbox64.dts
> > > > index a9cd7908f8..aed3801af8 100644
> > > > --- a/arch/sandbox/dts/sandbox64.dts
> > > > +++ b/arch/sandbox/dts/sandbox64.dts
> > > > @@ -89,6 +89,19 @@
> > > >                 cs-gpios = <0>, <&gpio_a 0>;
> > > >         };
> > > >
> > > > +       nvmxip-qspi1@08000000 {
> > > > +               compatible = "nvmxip,qspi";
> > > > +               reg = <0x0 0x08000000 0x0 0x00200000>;
> > > > +               lba_shift = <9>;
> > > > +               lba = <4096>;
> > > > +       };
> > > > +
> > > > +       nvmxip-qspi2@08200000 {
> > > > +               compatible = "nvmxip,qspi";
> > > > +               reg = <0x0 0x08200000 0x0 0x00100000>;
> > > > +               lba_shift = <9>;
> > > > +               lba = <2048>;
> > > > +       };
> > > >  };
> > > >
> > > >  #include "sandbox.dtsi"
> > > > diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts
> > > > index dffe10adbf..c3ba0a225e 100644
> > > > --- a/arch/sandbox/dts/test.dts
> > > > +++ b/arch/sandbox/dts/test.dts
> > > > @@ -1745,6 +1745,20 @@
> > > >                 compatible = "u-boot,fwu-mdata-gpt";
> > > >                 fwu-mdata-store = <&mmc0>;
> > > >         };
> > > > +
> > > > +       nvmxip-qspi1@08000000 {
> > > > +               compatible = "nvmxip,qspi";
> > > > +               reg = <0x08000000 0x00200000>;
> > > > +               lba_shift = <9>;
> > > > +               lba = <4096>;
> > > > +       };
> > > > +
> > > > +       nvmxip-qspi2@08200000 {
> > > > +               compatible = "nvmxip,qspi";
> > > > +               reg = <0x08200000 0x00100000>;
> > > > +               lba_shift = <9>;
> > > > +               lba = <2048>;
> > > > +       };
> > > >  };
> > > >
> > > >  #include "sandbox_pmic.dtsi"
> > > > diff --git a/configs/sandbox64_defconfig b/configs/sandbox64_defconfig
> > > > index ba45ac0b71..cd4dadb190 100644
> > > > --- a/configs/sandbox64_defconfig
> > > > +++ b/configs/sandbox64_defconfig
> > > > @@ -259,3 +259,4 @@ CONFIG_FWU_MULTI_BANK_UPDATE=y
> > > >  CONFIG_UNIT_TEST=y
> > > >  CONFIG_UT_TIME=y
> > > >  CONFIG_UT_DM=y
> > > > +CONFIG_NVMXIP_QSPI=y
> > > > \ No newline at end of file
> > > > diff --git a/drivers/nvmxip/nvmxip.c b/drivers/nvmxip/nvmxip.c
> > > > index c276fb147e..ea9b9bfa48 100644
> > > > --- a/drivers/nvmxip/nvmxip.c
> > > > +++ b/drivers/nvmxip/nvmxip.c
> > > > @@ -87,6 +87,10 @@ int nvmxip_init(struct udevice *udev)
> > > >         priv_data->udev = udev;
> > > >         priv_data->plat_data = plat_data;
> > > >
> > > > +#if CONFIG_IS_ENABLED(SANDBOX64)
> > >
> > > if (IS_ENABLED(CONFIG_SANDBOX64))
> > >
> > >
> > > > +       sandbox_set_enable_memio(true);
> > > > +#endif
> >
> > Isn't that just going to introduce a warning about undeclared function
> > on non-sandbox64?
> 
> Well, the header file should always be included, so it shouldn't!

I tried what you suggested by including #include <asm/test.h> without #ifdef.

When compiling for Arm, the compilation fails:

drivers/mtd/nvmxip/nvmxip-uclass.c:12:10: fatal error: asm/test.h: No such
 file or directory                                                       
   12 | #include <asm/test.h>
      |          ^~~~~~~~~~~~
compilation terminated.

Reason of failure:

sandbox_set_enable_memio() prototype is declared in sandbox/include/asm/test.h, which doesn't work for platforms other than sandbox.

Even when including include/test/test.h instead of sandbox/include/asm/test.h, it will fail because there is an #ifdef CONFIG_SANDBOX in include/test/test.h:

#ifdef CONFIG_SANDBOX
#include <asm/state.h>
#include <asm/test.h>
#endif

I think better to keep the #if CONFIG_IS_ENABLED(SANDBOX64) in nvmxip-uclass.c.

Cheers,
Abdellatif

> 
> Please also add a newline to the defconfig.
> 
> Regards,
> Simon