Re: [PATCH v2 2/2] nvmxip: add sandbox support

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

Hi Marek,

Please kindly find my comments below.

> > arch/sandbox/dts/sandbox.dts       | 14 ++++++++++++++
> 
> Please separate DT change
> 
> >   configs/sandbox_defconfig          |  4 ++--
> 
> Config change too, separate patch that goes last.

This commit is doing 1 thing: adding 32-bit sandbox support.

The DT change comes into the same context.
It makes sense to keep it in this same commit.

In previous contributions I made, it was accepted that DT
and defconfig  are part of the same commit as the code.

Let's see what Simon thinks.

I'm happy to split if that has becone a new requirement.

> >   	int devnum;
> > -#if CONFIG_IS_ENABLED(SANDBOX64)
> > +#if CONFIG_IS_ENABLED(SANDBOX)
> >   	sandbox_set_enable_memio(true);
> 
> This should not be in drivers at all, this should be in tests/ , see
> 
> https://source.denx.de/u-boot/custodians/u-boot-sh/-/commit/b07772226b405d6212d4faa9329d64a09708b188

Thanks, I'm happy to improve that one. I'll move it to tests in a seperate commit :)

> 
> >   #endif
> > @@ -62,7 +62,7 @@ static int nvmxip_post_bind(struct udevice *udev)
> >   		return ret;
> >   	}
> > -	log_info("[%s]: the block device %s ready for use\n", udev->name, bdev_name);
> > +	log_debug("[%s]: the block device %s ready for use\n", udev->name, bdev_name);
> 
> Unrelated change -> separate patch please.

Valid point, I'll do thanks.

> > +	l_word = readl(address);
> > +	h_word = readl((u8 *)address + sizeof(u32));
> > +	*value = FIELD_PREP(GENMASK_ULL(63, 32), h_word) | l_word;
> > +#endif
> >   	return 0;
> >   }
> > @@ -67,7 +76,7 @@ static ulong nvmxip_blk_read(struct udevice *dev, lbaint_t blknr, lbaint_t blkcn
> >   	/* assumption: the data is virtually contiguous */
> >   	for (qdata_idx = 0 ; qdata_idx < qwords ; qdata_idx++)
> > -		nvmxip_mmio_rawread((phys_addr_t)(virt_blkaddr + qdata_idx), pdst++);
> > +		nvmxip_mmio_rawread(virt_blkaddr + qdata_idx, pdst++);
> 
> Separate patch please, or just use this commit as part of this series:
> 
> https://source.denx.de/u-boot/custodians/u-boot-sh/-/commit/85a662e98c44921a0f5652fa4b170625424ef9a9

This is part of the 32-bit support work.
Before this commit, it works fine on sandbox64.

> >   	log_debug("[%s]:     src[0]: 0x%llx , dst[0]: 0x%llx , src[-1]: 0x%llx , dst[-1]: 0x%llx\n",
> >   		  dev->name,
> > diff --git a/drivers/mtd/nvmxip/nvmxip_qspi.c b/drivers/mtd/nvmxip/nvmxip_qspi.c
> > index 7221fd1cb4..faeb99b4ad 100644
> > --- a/drivers/mtd/nvmxip/nvmxip_qspi.c
> > +++ b/drivers/mtd/nvmxip/nvmxip_qspi.c
> > @@ -50,7 +50,7 @@ static int nvmxip_qspi_of_to_plat(struct udevice *dev)
> >   		return -EINVAL;
> >   	}
> > -	log_debug("[%s]: XIP device base addr: 0x%llx , lba_shift: %d , lbas: %lu\n",
> > +	log_debug("[%s]: XIP device base addr: " PHYS_ADDR_LN " , lba_shift: %d , lbas: %lu\n",
> >   		  dev->name, plat->phys_base, plat->lba_shift, plat->lba);
> 
> Another separate patch.

This is part of the 32-bit support work.

> 
> >   	return 0;
> > diff --git a/test/dm/Makefile b/test/dm/Makefile
> > index 7ed00733c1..77172d9012 100644
> > --- a/test/dm/Makefile
> > +++ b/test/dm/Makefile
> > @@ -18,7 +18,7 @@ obj-$(CONFIG_UT_DM) += test-uclass.o
> >   obj-$(CONFIG_UT_DM) += core.o
> >   obj-$(CONFIG_UT_DM) += read.o
> >   obj-$(CONFIG_UT_DM) += phys2bus.o
> > -ifeq ($(CONFIG_NVMXIP_QSPI)$(CONFIG_SANDBOX64),yy)
> > +ifeq ($(CONFIG_NVMXIP_QSPI),y)
> 
> Separate patch.
> 
> Look here for some ideas:
> 
> https://source.denx.de/u-boot/custodians/u-boot-sh/-/commits/ci%2Ftest-sandbox64?ref_type=heads

SANDBOX applies for both sandbox64 and sandbox.
This is part of enabling sandbox alongside sandbox64.

This has been tested and works.

Kind regards
Abdellatif