Re: [PATCH v1 6/6] sandbox64: add a test case for UCLASS_NVMXIP
On Mon, Feb 06, 2023 at 09:02:40PM -0700, Simon Glass wrote:
> Hi,
>
> On Mon, 16 Jan 2023 at 10:29, wrote:
> >
> > From: Abdellatif El Khlifi
> >
> > provide a test for NVM XIP devices
> >
> > The test case allows to make sure of the following:
> >
> > - The NVM XIP QSPI devices are probed
> > - The DT entries are read correctly
> > - the data read from the flash by the NVMXIP block driver is correct
> >
> > Signed-off-by: Abdellatif El Khlifi
> > ---
> > MAINTAINERS | 1 +
> > test/dm/Makefile | 5 ++
> > test/dm/nvmxip.c | 117 +++
> > 3 files changed, 123 insertions(+)
> > create mode 100644 test/dm/nvmxip.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 539d01f68c..e92898e462 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -1199,6 +1199,7 @@ S:Maintained
> > F: doc/develop/driver-model/nvmxip.rst
> > F: doc/device-tree-bindings/nvmxip/nvmxip.txt
> > F: drivers/nvmxip/
> > +F: test/dm/nvmxip.c
> >
> > NVMEM
> > M: Sean Anderson
> > diff --git a/test/dm/Makefile b/test/dm/Makefile
> > index 7a79b6e1a2..7f9d0b3c38 100644
> > --- a/test/dm/Makefile
> > +++ b/test/dm/Makefile
> > @@ -1,6 +1,7 @@
> > # SPDX-License-Identifier: GPL-2.0+
> > #
> > # Copyright (c) 2013 Google, Inc
> > +# Copyright 2023 Arm Limited and/or its affiliates
> >
> >
> > obj-$(CONFIG_UT_DM) += test-dm.o
> >
> > @@ -17,6 +18,10 @@ 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)
> > +obj-y += nvmxip.o
> > +endif
> > +
> > ifneq ($(CONFIG_SANDBOX),)
> > ifeq ($(CONFIG_ACPIGEN),y)
> > obj-y += acpi.o
> > diff --git a/test/dm/nvmxip.c b/test/dm/nvmxip.c
> > new file mode 100644
> > index 00..b65154d125
> > --- /dev/null
> > +++ b/test/dm/nvmxip.c
> > @@ -0,0 +1,117 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Functional tests for UCLASS_FFA class
> > + *
> > + * Copyright 2023 Arm Limited and/or its affiliates
> >
> > + *
> > + * Authors:
> > + * Abdellatif El Khlifi
> > + */
> > +
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include "../../drivers/nvmxip/nvmxip.h"
>
> move to end
>
> > +#include
> > +#include
> > +
> > +/* NVMXIP devices described in the device tree */
> > +#define SANDBOX_NVMXIP_DEVICES 2
> > +
> > +/* reference device tree data for the probed devices */
> > +static struct nvmxip_plat nvmqspi_refdata[SANDBOX_NVMXIP_DEVICES] = {
> > + {0x0800, 9, 4096}, {0x0820, 9, 2048}
> > +};
> > +
> > +#define NVMXIP_BLK_START_PATTERN 0x1122334455667788ULL
> > +#define NVMXIP_BLK_END_PATTERN 0xa1a2a3a4a5a6a7a8ULL
> > +
> > +static int dm_nvmxip_flash_sanity(u8 device_idx, void *buffer)
> > +{
> > + int i;
> > + u64 *ptr = NULL;
> > + u8 *base = NULL;
> > + unsigned long blksz;
> > +
> > + blksz = 1 << nvmqspi_refdata[device_idx].lba_shift;
>
> BIT() or BITUL() ?
>
> > +
> > + /* if buffer not NULL, init the flash with the pattern data*/
>
> space before */ (please fix globally)
>
> can you explain why, in the comment?
>
> > + if (!buffer)
> > + base = map_sysmem(nvmqspi_refdata[device_idx].phys_base, 0);
> > + else
> > + base = buffer;
> > +
> > + for (i = 0; i < nvmqspi_refdata[device_idx].lba ; i++) {
> > + ptr = (u64 *)(base + i * blksz);
> > +
> > + /* write an 8 bytes pattern at the start of the current
> > block*/
> > + if (!buffer)
> > + *ptr = NVMXIP_BLK_START_PATTERN;
> > + else if (*ptr != NVMXIP_BLK_START_PATTERN)
> > + return -EINVAL;
> > +
> > + ptr = (u64 *)((u8 *)ptr + blksz - sizeof(u64));
> > +
> > + /* write an 8 bytes pattern at the end of the current
> > block*/
> > + if (!buffer)
> > + *ptr = NVMXIP_BLK_END_PATTERN;
> > + else if (*ptr != NVMXIP_BLK_END_PATTERN)
> > + return -EINVAL;
>
> You can use ut_assert...() here so that people can see which line failed
>
> > + }
> > +
> > + if (!buffer)
> > + unmap_sysmem(base);
> > +
> > + return 0;
> > +}
> > +
> > +static int dm_test_nvmxip(struct unit_test_state *uts)
> > +{
> > + struct nvmxip_plat *plat_data = NULL;
> > + struct udevice *dev = NULL, *bdev = NULL;
> > + u8 device_idx;
> > + void *buffer = NULL;
> > + unsigned long flashsz;
> > +
> > + /* set the flash content first for both devices */
> > + dm_nvmxip_flash_sanity(0, NULL);
> > + dm_nvmxip_flash_sanity(1, NULL);
> > +
> > + /* probing all NVM XIP QSPI devices */
> > + for (device_idx = 0, uclass_first_device(UCLASS_NVMXIP, &dev);
> > +dev;
> > +u
Re: [PATCH v1 6/6] sandbox64: add a test case for UCLASS_NVMXIP
Hi,
On Mon, 16 Jan 2023 at 10:29, wrote:
>
> From: Abdellatif El Khlifi
>
> provide a test for NVM XIP devices
>
> The test case allows to make sure of the following:
>
> - The NVM XIP QSPI devices are probed
> - The DT entries are read correctly
> - the data read from the flash by the NVMXIP block driver is correct
>
> Signed-off-by: Abdellatif El Khlifi
> ---
> MAINTAINERS | 1 +
> test/dm/Makefile | 5 ++
> test/dm/nvmxip.c | 117 +++
> 3 files changed, 123 insertions(+)
> create mode 100644 test/dm/nvmxip.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 539d01f68c..e92898e462 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1199,6 +1199,7 @@ S:Maintained
> F: doc/develop/driver-model/nvmxip.rst
> F: doc/device-tree-bindings/nvmxip/nvmxip.txt
> F: drivers/nvmxip/
> +F: test/dm/nvmxip.c
>
> NVMEM
> M: Sean Anderson
> diff --git a/test/dm/Makefile b/test/dm/Makefile
> index 7a79b6e1a2..7f9d0b3c38 100644
> --- a/test/dm/Makefile
> +++ b/test/dm/Makefile
> @@ -1,6 +1,7 @@
> # SPDX-License-Identifier: GPL-2.0+
> #
> # Copyright (c) 2013 Google, Inc
> +# Copyright 2023 Arm Limited and/or its affiliates
>
>
> obj-$(CONFIG_UT_DM) += test-dm.o
>
> @@ -17,6 +18,10 @@ 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)
> +obj-y += nvmxip.o
> +endif
> +
> ifneq ($(CONFIG_SANDBOX),)
> ifeq ($(CONFIG_ACPIGEN),y)
> obj-y += acpi.o
> diff --git a/test/dm/nvmxip.c b/test/dm/nvmxip.c
> new file mode 100644
> index 00..b65154d125
> --- /dev/null
> +++ b/test/dm/nvmxip.c
> @@ -0,0 +1,117 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Functional tests for UCLASS_FFA class
> + *
> + * Copyright 2023 Arm Limited and/or its affiliates
>
> + *
> + * Authors:
> + * Abdellatif El Khlifi
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include "../../drivers/nvmxip/nvmxip.h"
move to end
> +#include
> +#include
> +
> +/* NVMXIP devices described in the device tree */
> +#define SANDBOX_NVMXIP_DEVICES 2
> +
> +/* reference device tree data for the probed devices */
> +static struct nvmxip_plat nvmqspi_refdata[SANDBOX_NVMXIP_DEVICES] = {
> + {0x0800, 9, 4096}, {0x0820, 9, 2048}
> +};
> +
> +#define NVMXIP_BLK_START_PATTERN 0x1122334455667788ULL
> +#define NVMXIP_BLK_END_PATTERN 0xa1a2a3a4a5a6a7a8ULL
> +
> +static int dm_nvmxip_flash_sanity(u8 device_idx, void *buffer)
> +{
> + int i;
> + u64 *ptr = NULL;
> + u8 *base = NULL;
> + unsigned long blksz;
> +
> + blksz = 1 << nvmqspi_refdata[device_idx].lba_shift;
BIT() or BITUL() ?
> +
> + /* if buffer not NULL, init the flash with the pattern data*/
space before */ (please fix globally)
can you explain why, in the comment?
> + if (!buffer)
> + base = map_sysmem(nvmqspi_refdata[device_idx].phys_base, 0);
> + else
> + base = buffer;
> +
> + for (i = 0; i < nvmqspi_refdata[device_idx].lba ; i++) {
> + ptr = (u64 *)(base + i * blksz);
> +
> + /* write an 8 bytes pattern at the start of the current
> block*/
> + if (!buffer)
> + *ptr = NVMXIP_BLK_START_PATTERN;
> + else if (*ptr != NVMXIP_BLK_START_PATTERN)
> + return -EINVAL;
> +
> + ptr = (u64 *)((u8 *)ptr + blksz - sizeof(u64));
> +
> + /* write an 8 bytes pattern at the end of the current block*/
> + if (!buffer)
> + *ptr = NVMXIP_BLK_END_PATTERN;
> + else if (*ptr != NVMXIP_BLK_END_PATTERN)
> + return -EINVAL;
You can use ut_assert...() here so that people can see which line failed
> + }
> +
> + if (!buffer)
> + unmap_sysmem(base);
> +
> + return 0;
> +}
> +
> +static int dm_test_nvmxip(struct unit_test_state *uts)
> +{
> + struct nvmxip_plat *plat_data = NULL;
> + struct udevice *dev = NULL, *bdev = NULL;
> + u8 device_idx;
> + void *buffer = NULL;
> + unsigned long flashsz;
> +
> + /* set the flash content first for both devices */
> + dm_nvmxip_flash_sanity(0, NULL);
> + dm_nvmxip_flash_sanity(1, NULL);
> +
> + /* probing all NVM XIP QSPI devices */
> + for (device_idx = 0, uclass_first_device(UCLASS_NVMXIP, &dev);
> +dev;
> +uclass_next_device(&dev), device_idx++) {
> + plat_data = dev_get_plat(dev);
> +
> + /* device tree entries checks */
> + ut_assertok(nvmqspi_refdata[device_idx].phys_base !=
> plat_data->phys_base);
> + ut_assertok(nvmqspi_refdata[device_idx].lba_shift !=
> plat_data->lba_shift);
> + ut_assertok(nvmqspi_refdata[device_idx]
