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

[Marek Vasut]

On 8/16/23 18:39, Abdellatif El Khlifi wrote:
> Hi Marek,

Hi,

> 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.

No, it does not do one thing, else I would not ask you to split this up 
into a series. It does multiple things and squashes those in single commit.

> 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.

This is not a new requirement, that requirement has been around since 
forever, see:

https://docs.kernel.org/process/submitting-patches.html#split-changes

U-Boot follows the same rule set.

>>>    	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.

Have you tested the entire test suite ? Like e.g. the DM/UT 'host' test 
? That one fails on sandbox64 iirc .

>>>    	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.

This fixes a print of phys_addr_t and is a good example for others to 
follow if they need to print such an address. If this is a separate 
patch with proper commit message explaining the change, then others can 
be pointed to that commit as an example to follow. If this is buried in 
a mega-patch, this benefit is lost.

>>>    	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.

If this is split up into proper patches with proper commit messages, the 
resulting change would be identical, so the "tested and works" argument 
still applies, even if this is split into proper series.