Re: [PATCH v2 2/2] nvmxip: add sandbox support
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.
Re: [PATCH v2 2/2] nvmxip: add sandbox support
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
Re: [PATCH v2 2/2] nvmxip: add sandbox support
On 8/16/23 13:05, Abdellatif El Khlifi wrote:
enable the 32-bit version of sandbox
Initially NVMXIP came with sandbox64 support.
Let's enable sandbox support as well.
Signed-off-by: Abdellatif El Khlifi
Cc: Tom Rini
Cc: Simon Glass
Cc: Marek Vasut
---
Changelog:
===
v2:
* split into 2 commits: one for sandbox 32-bit support,
and one for moving PHYS_ADDR_LN* to log.h
v1: introduce the sandbox 32-bit support
arch/sandbox/dts/sandbox.dts | 14 ++
Please separate DT change
configs/sandbox_defconfig | 4 ++--
Config change too, separate patch that goes last.
drivers/mtd/nvmxip/nvmxip-uclass.c | 6 +++---
drivers/mtd/nvmxip/nvmxip.c| 13 +++--
drivers/mtd/nvmxip/nvmxip_qspi.c | 2 +-
test/dm/Makefile | 2 +-
6 files changed, 32 insertions(+), 9 deletions(-)
[...]
--- a/drivers/mtd/nvmxip/nvmxip-uclass.c
+++ b/drivers/mtd/nvmxip/nvmxip-uclass.c
@@ -9,7 +9,7 @@
#include
#include
#include
-#if CONFIG_IS_ENABLED(SANDBOX64)
+#if CONFIG_IS_ENABLED(SANDBOX)
#include
#endif
#include
@@ -39,7 +39,7 @@ static int nvmxip_post_bind(struct udevice *udev)
char bdev_name[NVMXIP_BLKDEV_NAME_SZ + 1];
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
#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.
return 0;
}
diff --git a/drivers/mtd/nvmxip/nvmxip.c b/drivers/mtd/nvmxip/nvmxip.c
index a359e3b482..9191e69a40 100644
--- a/drivers/mtd/nvmxip/nvmxip.c
+++ b/drivers/mtd/nvmxip/nvmxip.c
@@ -11,6 +11,7 @@
#include
#include
#include
+#include
#include
#include
#include "nvmxip.h"
@@ -26,9 +27,17 @@
*
* Always return 0.
*/
-static int nvmxip_mmio_rawread(const phys_addr_t address, u64 *value)
+static int nvmxip_mmio_rawread(const u64 *address, u64 *value)
{
+#if CONFIG_IS_ENABLED(PHYS_64BIT)
*value = readq(address);
+#else
+ u32 h_word, l_word;
+
+ 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
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.
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
