[PATCH 1/2] drivers/mtd/nvmxip: Rework the read accessor to support 32bit systems
Get rid of nvmxip_mmio_rawread() and just implement the readl()/readq() reader loop within nvmxip_blk_read(). Cast the destination buffer as needed and increment the read by either 4 or 8 bytes depending on if this is systemd with 32bit or 64bit physical address.
Signed-off-by: Marek Vasut marek.vasut+renesas@mailbox.org --- Cc: Abdellatif El Khlifi abdellatif.elkhlifi@arm.com Cc: Simon Glass sjg@chromium.org --- drivers/mtd/nvmxip/nvmxip.c | 38 ++++++++++++------------------------- 1 file changed, 12 insertions(+), 26 deletions(-)
diff --git a/drivers/mtd/nvmxip/nvmxip.c b/drivers/mtd/nvmxip/nvmxip.c index a359e3b4822..0bd98d64275 100644 --- a/drivers/mtd/nvmxip/nvmxip.c +++ b/drivers/mtd/nvmxip/nvmxip.c @@ -15,23 +15,6 @@ #include <linux/errno.h> #include "nvmxip.h"
-/** - * nvmxip_mmio_rawread() - read from the XIP flash - * @address: address of the data - * @value: pointer to where storing the value read - * - * Read raw data from the XIP flash. - * - * Return: - * - * Always return 0. - */ -static int nvmxip_mmio_rawread(const phys_addr_t address, u64 *value) -{ - *value = readq(address); - return 0; -} - /** * nvmxip_blk_read() - block device read operation * @dev: the block device @@ -49,15 +32,14 @@ static ulong nvmxip_blk_read(struct udevice *dev, lbaint_t blknr, lbaint_t blkcn { struct nvmxip_plat *plat = dev_get_plat(dev->parent); struct blk_desc *desc = dev_get_uclass_plat(dev); - /* number of the u64 words to read */ - u32 qwords = (blkcnt * desc->blksz) / sizeof(u64); + /* number of bytes to read */ + u32 size = blkcnt * desc->blksz; /* physical address of the first block to read */ phys_addr_t blkaddr = plat->phys_base + blknr * desc->blksz; - u64 *virt_blkaddr; - u64 *pdst = buffer; + void *virt_blkaddr; uint qdata_idx;
- if (!pdst) + if (!buffer) return -EINVAL;
log_debug("[%s]: reading from blknr: %lu , blkcnt: %lu\n", dev->name, blknr, blkcnt); @@ -66,12 +48,16 @@ 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++); - +#if IS_ENABLED(CONFIG_PHYS_64BIT) + for (qdata_idx = 0 ; qdata_idx < size; qdata_idx += sizeof(u64)) + *(u64 *)(buffer + qdata_idx) = readq(virt_blkaddr + qdata_idx); +#else + for (qdata_idx = 0 ; qdata_idx < size; qdata_idx += sizeof(u32)) + *(u32 *)(buffer + qdata_idx) = readl(virt_blkaddr + qdata_idx); +#endif log_debug("[%s]: src[0]: 0x%llx , dst[0]: 0x%llx , src[-1]: 0x%llx , dst[-1]: 0x%llx\n", dev->name, - *virt_blkaddr, + *(u64 *)virt_blkaddr, *(u64 *)buffer, *(u64 *)((u8 *)virt_blkaddr + desc->blksz * blkcnt - sizeof(u64)), *(u64 *)((u8 *)buffer + desc->blksz * blkcnt - sizeof(u64)));
Enable NVMXIP QSPI driver on sandbox, since it is already enabled on sandbox64. Update blk tests to match.
Signed-off-by: Marek Vasut marek.vasut+renesas@mailbox.org --- Cc: Abdellatif El Khlifi abdellatif.elkhlifi@arm.com Cc: Simon Glass sjg@chromium.org --- configs/sandbox_defconfig | 1 + test/dm/blk.c | 63 +++++++++++++++++++++++++++++++-------- 2 files changed, 52 insertions(+), 12 deletions(-)
diff --git a/configs/sandbox_defconfig b/configs/sandbox_defconfig index 1cd1c2ed7cd..f451a94362e 100644 --- a/configs/sandbox_defconfig +++ b/configs/sandbox_defconfig @@ -227,6 +227,7 @@ CONFIG_SPI_FLASH_SPANSION=y CONFIG_SPI_FLASH_STMICRO=y CONFIG_SPI_FLASH_SST=y CONFIG_SPI_FLASH_WINBOND=y +CONFIG_NVMXIP_QSPI=y CONFIG_MULTIPLEXER=y CONFIG_MUX_MMIO=y CONFIG_NVME_PCI=y diff --git a/test/dm/blk.c b/test/dm/blk.c index 446c4423e6f..d268a441c99 100644 --- a/test/dm/blk.c +++ b/test/dm/blk.c @@ -82,12 +82,12 @@ static int dm_test_blk_usb(struct unit_test_state *uts) ut_asserteq_ptr(usb_dev, dev_get_parent(dev));
/* Check we have one block device for each mass storage device */ - ut_asserteq(6, count_blk_devices()); + ut_asserteq(8, count_blk_devices());
/* Now go around again, making sure the old devices were unbound */ ut_assertok(usb_stop()); ut_assertok(usb_init()); - ut_asserteq(6, count_blk_devices()); + ut_asserteq(8, count_blk_devices()); ut_assertok(usb_stop());
return 0; @@ -184,6 +184,10 @@ static int dm_test_blk_iter(struct unit_test_state *uts) */ ut_assertok(blk_first_device_err(BLKF_FIXED, &dev)); ut_asserteq_str("mmc2.blk", dev->name); + ut_assertok(blk_next_device_err(BLKF_FIXED, &dev)); + ut_asserteq_str("nvmxip-qspi1@08000000.blk#1", dev->name); + ut_assertok(blk_next_device_err(BLKF_FIXED, &dev)); + ut_asserteq_str("nvmxip-qspi2@08200000.blk#2", dev->name); ut_asserteq(-ENODEV, blk_next_device_err(BLKF_FIXED, &dev));
ut_assertok(blk_first_device_err(BLKF_REMOVABLE, &dev)); @@ -198,16 +202,23 @@ static int dm_test_blk_iter(struct unit_test_state *uts) ut_asserteq_str("mmc1.blk", dev->name); ut_assertok(blk_next_device_err(BLKF_BOTH, &dev)); ut_asserteq_str("mmc0.blk", dev->name); + ut_assertok(blk_next_device_err(BLKF_BOTH, &dev)); + ut_asserteq_str("nvmxip-qspi1@08000000.blk#1", dev->name); + ut_assertok(blk_next_device_err(BLKF_BOTH, &dev)); + ut_asserteq_str("nvmxip-qspi2@08200000.blk#2", dev->name); ut_asserteq(-ENODEV, blk_next_device_err(BLKF_FIXED, &dev));
- ut_asserteq(1, blk_count_devices(BLKF_FIXED)); + ut_asserteq(3, blk_count_devices(BLKF_FIXED)); ut_asserteq(2, blk_count_devices(BLKF_REMOVABLE)); - ut_asserteq(3, blk_count_devices(BLKF_BOTH)); + ut_asserteq(5, blk_count_devices(BLKF_BOTH));
i = 0; blk_foreach_probe(BLKF_FIXED, dev) - ut_asserteq_str((i++, "mmc2.blk"), dev->name); - ut_asserteq(1, i); + ut_asserteq_str((++i == 1 ? "mmc2.blk" : + i == 2 ? "nvmxip-qspi1@08000000.blk#1" : + "nvmxip-qspi2@08200000.blk#2"), + dev->name); + ut_asserteq(3, i);
i = 0; blk_foreach_probe(BLKF_REMOVABLE, dev) @@ -216,9 +227,13 @@ static int dm_test_blk_iter(struct unit_test_state *uts)
i = 0; blk_foreach_probe(BLKF_BOTH, dev) - ut_asserteq_str((++i == 1 ? "mmc2.blk" : i == 2 ? - "mmc1.blk" : "mmc0.blk"), dev->name); - ut_asserteq(3, i); + ut_asserteq_str((++i == 1 ? "mmc2.blk" : + i == 2 ? "mmc1.blk" : + i == 3 ? "mmc0.blk" : + i == 4 ? "nvmxip-qspi1@08000000.blk#1" : + "nvmxip-qspi2@08200000.blk#2"), + dev->name); + ut_asserteq(5, i);
return 0; } @@ -242,6 +257,14 @@ static int dm_test_blk_flags(struct unit_test_state *uts) ut_assertnonnull(dev); ut_asserteq_str("mmc0.blk", dev->name);
+ ut_assertok(blk_find_next(BLKF_BOTH, &dev)); + ut_assertnonnull(dev); + ut_asserteq_str("nvmxip-qspi1@08000000.blk#1", dev->name); + + ut_assertok(blk_find_next(BLKF_BOTH, &dev)); + ut_assertnonnull(dev); + ut_asserteq_str("nvmxip-qspi2@08200000.blk#2", dev->name); + ut_asserteq(-ENODEV, blk_find_next(BLKF_BOTH, &dev)); ut_assertnull(dev);
@@ -265,6 +288,14 @@ static int dm_test_blk_flags(struct unit_test_state *uts) ut_assertnonnull(dev); ut_asserteq_str("mmc0.blk", dev->name);
+ ut_assertok(blk_next_device_err(BLKF_BOTH, &dev)); + ut_assertnonnull(dev); + ut_asserteq_str("nvmxip-qspi1@08000000.blk#1", dev->name); + + ut_assertok(blk_next_device_err(BLKF_BOTH, &dev)); + ut_assertnonnull(dev); + ut_asserteq_str("nvmxip-qspi2@08200000.blk#2", dev->name); + ut_asserteq(-ENODEV, blk_next_device_err(BLKF_BOTH, &dev));
/* Look only for fixed devices */ @@ -272,6 +303,14 @@ static int dm_test_blk_flags(struct unit_test_state *uts) ut_assertnonnull(dev); ut_asserteq_str("mmc2.blk", dev->name);
+ ut_assertok(blk_next_device_err(BLKF_FIXED, &dev)); + ut_assertnonnull(dev); + ut_asserteq_str("nvmxip-qspi1@08000000.blk#1", dev->name); + + ut_assertok(blk_next_device_err(BLKF_FIXED, &dev)); + ut_assertnonnull(dev); + ut_asserteq_str("nvmxip-qspi2@08200000.blk#2", dev->name); + ut_asserteq(-ENODEV, blk_next_device_err(BLKF_FIXED, &dev));
/* Look only for removable devices */ @@ -317,13 +356,13 @@ static int dm_test_blk_foreach(struct unit_test_state *uts) blk_foreach_probe(BLKF_BOTH, dev) found |= 1 << dectoul(&dev->name[3], NULL); ut_asserteq(7, found); - ut_asserteq(3, blk_count_devices(BLKF_BOTH)); + ut_asserteq(5, blk_count_devices(BLKF_BOTH));
found = 0; blk_foreach_probe(BLKF_FIXED, dev) found |= 1 << dectoul(&dev->name[3], NULL); - ut_asserteq(4, found); - ut_asserteq(1, blk_count_devices(BLKF_FIXED)); + ut_asserteq(5, found); + ut_asserteq(3, blk_count_devices(BLKF_FIXED));
found = 0; blk_foreach_probe(BLKF_REMOVABLE, dev)
On Sun, 13 Aug 2023 at 15:47, Marek Vasut marek.vasut+renesas@mailbox.org wrote:
Enable NVMXIP QSPI driver on sandbox, since it is already enabled on sandbox64. Update blk tests to match.
Signed-off-by: Marek Vasut marek.vasut+renesas@mailbox.org
Cc: Abdellatif El Khlifi abdellatif.elkhlifi@arm.com Cc: Simon Glass sjg@chromium.org
configs/sandbox_defconfig | 1 + test/dm/blk.c | 63 +++++++++++++++++++++++++++++++-------- 2 files changed, 52 insertions(+), 12 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
On 8/15/23 00:42, Simon Glass wrote:
On Sun, 13 Aug 2023 at 15:47, Marek Vasut marek.vasut+renesas@mailbox.org wrote:
Enable NVMXIP QSPI driver on sandbox, since it is already enabled on sandbox64. Update blk tests to match.
Signed-off-by: Marek Vasut marek.vasut+renesas@mailbox.org
Cc: Abdellatif El Khlifi abdellatif.elkhlifi@arm.com Cc: Simon Glass sjg@chromium.org
configs/sandbox_defconfig | 1 + test/dm/blk.c | 63 +++++++++++++++++++++++++++++++-------- 2 files changed, 52 insertions(+), 12 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
NAK, this one has to wait. All of this NVMXIP stuff needs closer look, I think it is really weirdly broken and doesn't even pass sandbox and sandbox64 tests. Can Abdellatif try and check whether ALL sandbox and sandbox64 tests pass with this enabled ?
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 abdellatif.elkhlifi@arm.com Cc: Tom Rini trini@konsulko.com Cc: Simon Glass sjg@chromium.org --- arch/sandbox/dts/sandbox.dts | 14 ++++++++++++++ configs/sandbox_defconfig | 4 ++-- drivers/mtd/nvmxip/nvmxip-uclass.c | 6 +++--- drivers/mtd/nvmxip/nvmxip.c | 13 +++++++++++-- drivers/mtd/nvmxip/nvmxip_qspi.c | 10 +++++++++- test/dm/Makefile | 2 +- 6 files changed, 40 insertions(+), 9 deletions(-)
diff --git a/arch/sandbox/dts/sandbox.dts b/arch/sandbox/dts/sandbox.dts index 12d3eff5fa..1d38a939c4 100644 --- a/arch/sandbox/dts/sandbox.dts +++ b/arch/sandbox/dts/sandbox.dts @@ -97,6 +97,20 @@ compatible = "sandbox,spi"; cs-gpios = <0>, <&gpio_a 0>; }; + + 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.dtsi" diff --git a/configs/sandbox_defconfig b/configs/sandbox_defconfig index 1cd1c2ed7c..296f9d7326 100644 --- a/configs/sandbox_defconfig +++ b/configs/sandbox_defconfig @@ -145,7 +145,6 @@ CONFIG_IP_DEFRAG=y CONFIG_BOOTP_SERVERIP=y CONFIG_IPV6=y CONFIG_DM_DMA=y -CONFIG_DEVRES=y CONFIG_DEBUG_DEVRES=y CONFIG_SIMPLE_PM_BUS=y CONFIG_ADC=y @@ -184,6 +183,7 @@ CONFIG_DMA_CHANNELS=y CONFIG_SANDBOX_DMA=y CONFIG_FASTBOOT_FLASH=y CONFIG_FASTBOOT_FLASH_MMC_DEV=0 +CONFIG_ARM_FFA_TRANSPORT=y CONFIG_GPIO_HOG=y CONFIG_DM_GPIO_LOOKUP_LABEL=y CONFIG_QCOM_PMIC_GPIO=y @@ -227,6 +227,7 @@ CONFIG_SPI_FLASH_SPANSION=y CONFIG_SPI_FLASH_STMICRO=y CONFIG_SPI_FLASH_SST=y CONFIG_SPI_FLASH_WINBOND=y +CONFIG_NVMXIP_QSPI=y CONFIG_MULTIPLEXER=y CONFIG_MUX_MMIO=y CONFIG_NVME_PCI=y @@ -345,4 +346,3 @@ CONFIG_TEST_FDTDEC=y CONFIG_UNIT_TEST=y CONFIG_UT_TIME=y CONFIG_UT_DM=y -CONFIG_ARM_FFA_TRANSPORT=y diff --git a/drivers/mtd/nvmxip/nvmxip-uclass.c b/drivers/mtd/nvmxip/nvmxip-uclass.c index 6d8eb177b5..7177f8f314 100644 --- a/drivers/mtd/nvmxip/nvmxip-uclass.c +++ b/drivers/mtd/nvmxip/nvmxip-uclass.c @@ -9,7 +9,7 @@ #include <common.h> #include <dm.h> #include <log.h> -#if CONFIG_IS_ENABLED(SANDBOX64) +#if CONFIG_IS_ENABLED(SANDBOX) #include <asm/test.h> #endif #include <linux/bitops.h> @@ -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); #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);
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 <log.h> #include <mapmem.h> #include <asm/io.h> +#include <linux/bitfield.h> #include <linux/bitops.h> #include <linux/errno.h> #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++);
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..b6b94ca57f 100644 --- a/drivers/mtd/nvmxip/nvmxip_qspi.c +++ b/drivers/mtd/nvmxip/nvmxip_qspi.c @@ -17,6 +17,14 @@ DECLARE_GLOBAL_DATA_PTR;
#define NVMXIP_QSPI_DRV_NAME "nvmxip_qspi"
+/* Select the right physical address formatting according to the platform */ +#ifdef CONFIG_PHYS_64BIT +#define PhysAddrLength "ll" +#else +#define PhysAddrLength "" +#endif +#define PHYS_ADDR_LN "%" PhysAddrLength "x" + /** * nvmxip_qspi_of_to_plat() -read from DT * @dev: the NVMXIP device @@ -50,7 +58,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);
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) obj-y += nvmxip.o endif
On Tue, 15 Aug 2023 at 10:43, Abdellatif El Khlifi abdellatif.elkhlifi@arm.com 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 abdellatif.elkhlifi@arm.com Cc: Tom Rini trini@konsulko.com Cc: Simon Glass sjg@chromium.org
arch/sandbox/dts/sandbox.dts | 14 ++++++++++++++ configs/sandbox_defconfig | 4 ++-- drivers/mtd/nvmxip/nvmxip-uclass.c | 6 +++--- drivers/mtd/nvmxip/nvmxip.c | 13 +++++++++++-- drivers/mtd/nvmxip/nvmxip_qspi.c | 10 +++++++++- test/dm/Makefile | 2 +- 6 files changed, 40 insertions(+), 9 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
diff --git a/arch/sandbox/dts/sandbox.dts b/arch/sandbox/dts/sandbox.dts index 12d3eff5fa..1d38a939c4 100644 --- a/arch/sandbox/dts/sandbox.dts +++ b/arch/sandbox/dts/sandbox.dts @@ -97,6 +97,20 @@ compatible = "sandbox,spi"; cs-gpios = <0>, <&gpio_a 0>; };
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.dtsi" diff --git a/configs/sandbox_defconfig b/configs/sandbox_defconfig index 1cd1c2ed7c..296f9d7326 100644 --- a/configs/sandbox_defconfig +++ b/configs/sandbox_defconfig @@ -145,7 +145,6 @@ CONFIG_IP_DEFRAG=y CONFIG_BOOTP_SERVERIP=y CONFIG_IPV6=y CONFIG_DM_DMA=y -CONFIG_DEVRES=y CONFIG_DEBUG_DEVRES=y CONFIG_SIMPLE_PM_BUS=y CONFIG_ADC=y @@ -184,6 +183,7 @@ CONFIG_DMA_CHANNELS=y CONFIG_SANDBOX_DMA=y CONFIG_FASTBOOT_FLASH=y CONFIG_FASTBOOT_FLASH_MMC_DEV=0 +CONFIG_ARM_FFA_TRANSPORT=y CONFIG_GPIO_HOG=y CONFIG_DM_GPIO_LOOKUP_LABEL=y CONFIG_QCOM_PMIC_GPIO=y @@ -227,6 +227,7 @@ CONFIG_SPI_FLASH_SPANSION=y CONFIG_SPI_FLASH_STMICRO=y CONFIG_SPI_FLASH_SST=y CONFIG_SPI_FLASH_WINBOND=y +CONFIG_NVMXIP_QSPI=y CONFIG_MULTIPLEXER=y CONFIG_MUX_MMIO=y CONFIG_NVME_PCI=y @@ -345,4 +346,3 @@ CONFIG_TEST_FDTDEC=y CONFIG_UNIT_TEST=y CONFIG_UT_TIME=y CONFIG_UT_DM=y -CONFIG_ARM_FFA_TRANSPORT=y diff --git a/drivers/mtd/nvmxip/nvmxip-uclass.c b/drivers/mtd/nvmxip/nvmxip-uclass.c index 6d8eb177b5..7177f8f314 100644 --- a/drivers/mtd/nvmxip/nvmxip-uclass.c +++ b/drivers/mtd/nvmxip/nvmxip-uclass.c @@ -9,7 +9,7 @@ #include <common.h> #include <dm.h> #include <log.h> -#if CONFIG_IS_ENABLED(SANDBOX64) +#if CONFIG_IS_ENABLED(SANDBOX) #include <asm/test.h> #endif #include <linux/bitops.h> @@ -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); #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); 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 <log.h> #include <mapmem.h> #include <asm/io.h> +#include <linux/bitfield.h> #include <linux/bitops.h> #include <linux/errno.h> #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++); 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..b6b94ca57f 100644 --- a/drivers/mtd/nvmxip/nvmxip_qspi.c +++ b/drivers/mtd/nvmxip/nvmxip_qspi.c @@ -17,6 +17,14 @@ DECLARE_GLOBAL_DATA_PTR;
#define NVMXIP_QSPI_DRV_NAME "nvmxip_qspi"
+/* Select the right physical address formatting according to the platform */ +#ifdef CONFIG_PHYS_64BIT +#define PhysAddrLength "ll" +#else +#define PhysAddrLength "" +#endif +#define PHYS_ADDR_LN "%" PhysAddrLength "x"
Could this go in a standard header?
/**
- nvmxip_qspi_of_to_plat() -read from DT
- @dev: the NVMXIP device
@@ -50,7 +58,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); 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) obj-y += nvmxip.o endif
-- 2.25.1
Regars, Simon
sets the log formatting according to the platform (64-bit vs 32-bit)
Signed-off-by: Abdellatif El Khlifi abdellatif.elkhlifi@arm.com Cc: Simon Glass sjg@chromium.org --- cmd/armffa.c | 8 -------- include/log.h | 9 +++++++++ 2 files changed, 9 insertions(+), 8 deletions(-)
diff --git a/cmd/armffa.c b/cmd/armffa.c index 7e6eafc03a..ab0fd54d97 100644 --- a/cmd/armffa.c +++ b/cmd/armffa.c @@ -13,14 +13,6 @@ #include <stdlib.h> #include <asm/io.h>
-/* Select the right physical address formatting according to the platform */ -#ifdef CONFIG_PHYS_64BIT -#define PhysAddrLength "ll" -#else -#define PhysAddrLength "" -#endif -#define PHYS_ADDR_LN "%" PhysAddrLength "x" - /** * ffa_get_dev() - Return the FF-A device * @devp: pointer to the FF-A device diff --git a/include/log.h b/include/log.h index 6e84f080ef..3f631d6816 100644 --- a/include/log.h +++ b/include/log.h @@ -370,6 +370,15 @@ void __assert_fail(const char *assertion, const char *file, unsigned int line, #define log_msg_retz(_msg, _ret) ((void)(_msg), _ret) #endif
+/* Select the right physical address formatting according to the platform */ +#ifdef CONFIG_PHYS_64BIT +#define PhysAddrLength "ll" +#else +#define PhysAddrLength "" +#endif +#define PHYS_ADDR_LN "%" PhysAddrLength "x" +#define PHYS_ADDR_LNU "%" PhysAddrLength "u" + /** * enum log_rec_flags - Flags for a log record */ enum log_rec_flags { /** @LOGRECF_FORCE_DEBUG: Force output of debug record */
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 abdellatif.elkhlifi@arm.com Cc: Tom Rini trini@konsulko.com Cc: Simon Glass sjg@chromium.org Cc: Marek Vasut marek.vasut@mailbox.org
---
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 ++++++++++++++ configs/sandbox_defconfig | 4 ++-- 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(-)
diff --git a/arch/sandbox/dts/sandbox.dts b/arch/sandbox/dts/sandbox.dts index 12d3eff5fa..1d38a939c4 100644 --- a/arch/sandbox/dts/sandbox.dts +++ b/arch/sandbox/dts/sandbox.dts @@ -97,6 +97,20 @@ compatible = "sandbox,spi"; cs-gpios = <0>, <&gpio_a 0>; }; + + 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.dtsi" diff --git a/configs/sandbox_defconfig b/configs/sandbox_defconfig index 1cd1c2ed7c..296f9d7326 100644 --- a/configs/sandbox_defconfig +++ b/configs/sandbox_defconfig @@ -145,7 +145,6 @@ CONFIG_IP_DEFRAG=y CONFIG_BOOTP_SERVERIP=y CONFIG_IPV6=y CONFIG_DM_DMA=y -CONFIG_DEVRES=y CONFIG_DEBUG_DEVRES=y CONFIG_SIMPLE_PM_BUS=y CONFIG_ADC=y @@ -184,6 +183,7 @@ CONFIG_DMA_CHANNELS=y CONFIG_SANDBOX_DMA=y CONFIG_FASTBOOT_FLASH=y CONFIG_FASTBOOT_FLASH_MMC_DEV=0 +CONFIG_ARM_FFA_TRANSPORT=y CONFIG_GPIO_HOG=y CONFIG_DM_GPIO_LOOKUP_LABEL=y CONFIG_QCOM_PMIC_GPIO=y @@ -227,6 +227,7 @@ CONFIG_SPI_FLASH_SPANSION=y CONFIG_SPI_FLASH_STMICRO=y CONFIG_SPI_FLASH_SST=y CONFIG_SPI_FLASH_WINBOND=y +CONFIG_NVMXIP_QSPI=y CONFIG_MULTIPLEXER=y CONFIG_MUX_MMIO=y CONFIG_NVME_PCI=y @@ -345,4 +346,3 @@ CONFIG_TEST_FDTDEC=y CONFIG_UNIT_TEST=y CONFIG_UT_TIME=y CONFIG_UT_DM=y -CONFIG_ARM_FFA_TRANSPORT=y diff --git a/drivers/mtd/nvmxip/nvmxip-uclass.c b/drivers/mtd/nvmxip/nvmxip-uclass.c index 6d8eb177b5..7177f8f314 100644 --- a/drivers/mtd/nvmxip/nvmxip-uclass.c +++ b/drivers/mtd/nvmxip/nvmxip-uclass.c @@ -9,7 +9,7 @@ #include <common.h> #include <dm.h> #include <log.h> -#if CONFIG_IS_ENABLED(SANDBOX64) +#if CONFIG_IS_ENABLED(SANDBOX) #include <asm/test.h> #endif #include <linux/bitops.h> @@ -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); #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);
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 <log.h> #include <mapmem.h> #include <asm/io.h> +#include <linux/bitfield.h> #include <linux/bitops.h> #include <linux/errno.h> #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++);
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);
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) obj-y += nvmxip.o endif
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 abdellatif.elkhlifi@arm.com Cc: Tom Rini trini@konsulko.com Cc: Simon Glass sjg@chromium.org Cc: Marek Vasut marek.vasut@mailbox.org
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 <common.h> #include <dm.h> #include <log.h> -#if CONFIG_IS_ENABLED(SANDBOX64) +#if CONFIG_IS_ENABLED(SANDBOX) #include <asm/test.h> #endif #include <linux/bitops.h> @@ -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/b07772226b405d62...
#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 <log.h> #include <mapmem.h> #include <asm/io.h> +#include <linux/bitfield.h> #include <linux/bitops.h> #include <linux/errno.h> #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/85a662e98c44921a...
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-sandb...
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/b07772226b405d62...
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/85a662e98c44921a...
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-sandb...
SANDBOX applies for both sandbox64 and sandbox. This is part of enabling sandbox alongside sandbox64.
This has been tested and works.
Kind regards Abdellatif
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/b07772226b405d62...
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/85a662e98c44921a...
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-sandb...
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.
On 8/16/23 13:05, Abdellatif El Khlifi wrote:
sets the log formatting according to the platform (64-bit vs 32-bit)
Use imperative mood please, see https://www.kernel.org/doc/html/latest/process/submitting-patches.html#descr...
Signed-off-by: Abdellatif El Khlifi abdellatif.elkhlifi@arm.com Cc: Simon Glass sjg@chromium.org
cmd/armffa.c | 8 -------- include/log.h | 9 +++++++++ 2 files changed, 9 insertions(+), 8 deletions(-)
diff --git a/cmd/armffa.c b/cmd/armffa.c index 7e6eafc03a..ab0fd54d97 100644 --- a/cmd/armffa.c +++ b/cmd/armffa.c @@ -13,14 +13,6 @@ #include <stdlib.h> #include <asm/io.h>
-/* Select the right physical address formatting according to the platform */ -#ifdef CONFIG_PHYS_64BIT -#define PhysAddrLength "ll" -#else -#define PhysAddrLength "" -#endif -#define PHYS_ADDR_LN "%" PhysAddrLength "x"
Would it make sense to implement printf '%pa' formatting string instead ?
Hi Marek,
On Wed, 16 Aug 2023 at 05:13, Marek Vasut marek.vasut@mailbox.org wrote:
On 8/16/23 13:05, Abdellatif El Khlifi wrote:
sets the log formatting according to the platform (64-bit vs 32-bit)
Use imperative mood please, see https://www.kernel.org/doc/html/latest/process/submitting-patches.html#descr...
Signed-off-by: Abdellatif El Khlifi abdellatif.elkhlifi@arm.com Cc: Simon Glass sjg@chromium.org
cmd/armffa.c | 8 -------- include/log.h | 9 +++++++++ 2 files changed, 9 insertions(+), 8 deletions(-)
diff --git a/cmd/armffa.c b/cmd/armffa.c index 7e6eafc03a..ab0fd54d97 100644 --- a/cmd/armffa.c +++ b/cmd/armffa.c @@ -13,14 +13,6 @@ #include <stdlib.h> #include <asm/io.h>
-/* Select the right physical address formatting according to the platform */ -#ifdef CONFIG_PHYS_64BIT -#define PhysAddrLength "ll" -#else -#define PhysAddrLength "" -#endif -#define PHYS_ADDR_LN "%" PhysAddrLength "x"
Would it make sense to implement printf '%pa' formatting string instead ?
The problem is that it is not a pointer. Also we want the compiler to check the size.
Regards, Simon
On 8/16/23 16:39, Simon Glass wrote:
Hi Marek,
On Wed, 16 Aug 2023 at 05:13, Marek Vasut marek.vasut@mailbox.org wrote:
On 8/16/23 13:05, Abdellatif El Khlifi wrote:
sets the log formatting according to the platform (64-bit vs 32-bit)
Use imperative mood please, see https://www.kernel.org/doc/html/latest/process/submitting-patches.html#descr...
Signed-off-by: Abdellatif El Khlifi abdellatif.elkhlifi@arm.com Cc: Simon Glass sjg@chromium.org
cmd/armffa.c | 8 -------- include/log.h | 9 +++++++++ 2 files changed, 9 insertions(+), 8 deletions(-)
diff --git a/cmd/armffa.c b/cmd/armffa.c index 7e6eafc03a..ab0fd54d97 100644 --- a/cmd/armffa.c +++ b/cmd/armffa.c @@ -13,14 +13,6 @@ #include <stdlib.h> #include <asm/io.h>
-/* Select the right physical address formatting according to the platform */ -#ifdef CONFIG_PHYS_64BIT -#define PhysAddrLength "ll" -#else -#define PhysAddrLength "" -#endif -#define PHYS_ADDR_LN "%" PhysAddrLength "x"
Would it make sense to implement printf '%pa' formatting string instead ?
The problem is that it is not a pointer. Also we want the compiler to check the size.
What does Linux use ?
Hi Simon,
sets the log formatting according to the platform (64-bit vs 32-bit)
Signed-off-by: Abdellatif El Khlifi abdellatif.elkhlifi@arm.com Cc: Simon Glass sjg@chromium.org
cmd/armffa.c | 8 -------- include/log.h | 9 +++++++++ 2 files changed, 9 insertions(+), 8 deletions(-)
diff --git a/cmd/armffa.c b/cmd/armffa.c index 7e6eafc03a..ab0fd54d97 100644 --- a/cmd/armffa.c +++ b/cmd/armffa.c @@ -13,14 +13,6 @@ #include <stdlib.h> #include <asm/io.h>
-/* Select the right physical address formatting according to the platform */ -#ifdef CONFIG_PHYS_64BIT -#define PhysAddrLength "ll" -#else -#define PhysAddrLength "" -#endif -#define PHYS_ADDR_LN "%" PhysAddrLength "x"
/**
- ffa_get_dev() - Return the FF-A device
- @devp: pointer to the FF-A device
diff --git a/include/log.h b/include/log.h index 6e84f080ef..3f631d6816 100644 --- a/include/log.h +++ b/include/log.h @@ -370,6 +370,15 @@ void __assert_fail(const char *assertion, const char *file, unsigned int line, #define log_msg_retz(_msg, _ret) ((void)(_msg), _ret) #endif
+/* Select the right physical address formatting according to the platform */ +#ifdef CONFIG_PHYS_64BIT +#define PhysAddrLength "ll" +#else +#define PhysAddrLength "" +#endif +#define PHYS_ADDR_LN "%" PhysAddrLength "x" +#define PHYS_ADDR_LNU "%" PhysAddrLength "u"
/** * enum log_rec_flags - Flags for a log record */ enum log_rec_flags { /** @LOGRECF_FORCE_DEBUG: Force output of debug record */ -- 2.25.1
Any feedback about this specific commit please ?
A gentle reminder about the context: While I was working on the FF-A support, you suggested that I move PHYS_ADDR_LN macros to a common location so it can be used in a generic way.
Cheers, Abdellatif
On 8/15/23 18:43, 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 abdellatif.elkhlifi@arm.com Cc: Tom Rini trini@konsulko.com Cc: Simon Glass sjg@chromium.org
Can you please split this into multiple patches and add proper commit message description to each one ? Thanks.
Hi,
On Tue, Aug 15, 2023 at 12:45:57AM +0200, Marek Vasut wrote:
On 8/15/23 00:42, Simon Glass wrote:
On Sun, 13 Aug 2023 at 15:47, Marek Vasut marek.vasut+renesas@mailbox.org wrote:
Enable NVMXIP QSPI driver on sandbox, since it is already enabled on sandbox64. Update blk tests to match.
Signed-off-by: Marek Vasut marek.vasut+renesas@mailbox.org
Cc: Abdellatif El Khlifi abdellatif.elkhlifi@arm.com Cc: Simon Glass sjg@chromium.org
configs/sandbox_defconfig | 1 + test/dm/blk.c | 63 +++++++++++++++++++++++++++++++-------- 2 files changed, 52 insertions(+), 12 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
NAK, this one has to wait. All of this NVMXIP stuff needs closer look, I think it is really weirdly broken and doesn't even pass sandbox and sandbox64 tests. Can Abdellatif try and check whether ALL sandbox and sandbox64 tests pass with this enabled ?
Initially the NVMXIP feature has been introduced with sandbox64 support.
The test case runs fine on sandbox64.
I've just sent a commit enabling NVMXIP for sandbox [1].
Tested on both sandbox and sandbox64.
I hope this helps.
[1]: https://lore.kernel.org/all/20230815164300.444701-1-abdellatif.elkhlifi@arm....
Cheers Abdellatif
On 8/13/23 23:46, Marek Vasut wrote:
Get rid of nvmxip_mmio_rawread() and just implement the readl()/readq() reader loop within nvmxip_blk_read(). Cast the destination buffer as needed and increment the read by either 4 or 8 bytes depending on if this is systemd with 32bit or 64bit physical address.
Signed-off-by: Marek Vasut marek.vasut+renesas@mailbox.org
These two patches need a bit more work, skip for now.
On 8/13/23 23:46, Marek Vasut wrote:
Get rid of nvmxip_mmio_rawread() and just implement the readl()/readq() reader loop within nvmxip_blk_read(). Cast the destination buffer as needed and increment the read by either 4 or 8 bytes depending on if this is systemd with 32bit or 64bit physical address.
Signed-off-by: Marek Vasut marek.vasut+renesas@mailbox.org
Cc: Abdellatif El Khlifi abdellatif.elkhlifi@arm.com Cc: Simon Glass sjg@chromium.org
Sandbox CI test fails with this series:
https://source.denx.de/u-boot/custodians/u-boot-sh/-/pipelines/17427
https://u-boot.source-pages.denx.de/-/custodians/u-boot-sh/-/jobs/679256/art...
Hi,
On Sat, Aug 19, 2023 at 02:23:02AM +0200, Marek Vasut wrote:
On 8/13/23 23:46, Marek Vasut wrote:
Get rid of nvmxip_mmio_rawread() and just implement the readl()/readq() reader loop within nvmxip_blk_read(). Cast the destination buffer as needed and increment the read by either 4 or 8 bytes depending on if this is systemd with 32bit or 64bit physical address.
Signed-off-by: Marek Vasut marek.vasut+renesas@mailbox.org
Cc: Abdellatif El Khlifi abdellatif.elkhlifi@arm.com Cc: Simon Glass sjg@chromium.org
Sandbox CI test fails with this series:
https://source.denx.de/u-boot/custodians/u-boot-sh/-/pipelines/17427
https://u-boot.source-pages.denx.de/-/custodians/u-boot-sh/-/jobs/679256/art...
I run the {host , blk_usb, blk_iter, blk_foreach, blk_flags} tests using the commit [1] preceding the NVMXIP patchset. The tests fail as shown below [3]. I think these issues are not related to NVMXIP.
Regarding your comments for [2], I'll address them as soon as I can.
[1]: b197f1f05dee730e173a0756cb1a5f2be5d3ba5b [2]: https://lore.kernel.org/all/20230816110551.86930-2-abdellatif.elkhlifi@arm.c... [3]: tests results:
host:
test/dm/host.c:44, dm_test_host(): 0 == host_attach_file(dev, filename): Expected 0x0 (0), got 0xfffffffe (-2) Segmentation fault (core dumped)
blk_usb:
test/dm/blk.c:88, dm_test_blk_usb(): 6 == count_blk_devices(): Expected 0x6 (6), got 0x1 (1) Bus usb@1: scanning bus usb@1 for devices... 2 USB Device(s) found test/dm/blk.c:93, dm_test_blk_usb(): 6 == count_blk_devices(): Expected 0x6 (6), got 0x1 (1) Test blk_usb failed 4 times
blk_iter:
test/dm/blk.c:188, dm_test_blk_iter(): 0 == blk_first_device_err(BLKF_FIXED, &dev): Expected 0x0 (0), got 0xffffffed (-19) Segmentation fault (core dumped)
blk_foreach:
test/dm/blk.c:305, dm_test_blk_foreach(): 7 == found: Expected 0x7 (7), got 0x0 (0) test/dm/blk.c:316, dm_test_blk_foreach(): 7 == found: Expected 0x7 (7), got 0x0 (0) test/dm/blk.c:322, dm_test_blk_foreach(): 7 == found: Expected 0x7 (7), got 0x0 (0) test/dm/blk.c:323, dm_test_blk_foreach(): 3 == blk_count_devices(BLKF_BOTH): Expected 0x3 (3), got 0x0 (0) test/dm/blk.c:328, dm_test_blk_foreach(): 4 == found: Expected 0x4 (4), got 0x0 (0) test/dm/blk.c:329, dm_test_blk_foreach(): 1 == blk_count_devices(BLKF_FIXED): Expected 0x1 (1), got 0x0 (0) test/dm/blk.c:334, dm_test_blk_foreach(): 3 == found: Expected 0x3 (3), got 0x0 (0) test/dm/blk.c:335, dm_test_blk_foreach(): 2 == blk_count_devices(BLKF_REMOVABLE): Expected 0x2 (2), got 0x0 (0) ...
blk_flags:
test/dm/blk.c:236, dm_test_blk_flags(): 0 == blk_find_first(BLKF_BOTH, &dev): Expected 0x0 (0), got 0xffffffed (-19) test/dm/blk.c:237, dm_test_blk_flags(): dev = NULL: Expected non-null, got NULL Segmentation fault (core dumped)
Cheers, Abdellatif
participants (4)
-
Abdellatif El Khlifi -
Marek Vasut -
Marek Vasut -
Simon Glass