Re: [PATCH v1 6/6] sandbox64: add a test case for UCLASS_NVMXIP

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

On Mon, Feb 06, 2023 at 09:02:40PM -0700, Simon Glass wrote:
> Hi,
> 
> On Mon, 16 Jan 2023 at 10:29, <abdellatif.elkhlifi@arm.com> wrote:
> >
> > From: Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
> >
> > 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 <abdellatif.elkhlifi@arm.com>
> > ---
> >  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 <seanga2@gmail.com>
> > 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 <open-source-office@arm.com>
> >
> >  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 0000000000..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 <open-source-office@arm.com>
> > + *
> > + * Authors:
> > + *   Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
> > + */
> > +
> > +#include <common.h>
> > +#include <console.h>
> > +#include <blk.h>
> > +#include <dm.h>
> > +#include <dm/test.h>
> > +#include "../../drivers/nvmxip/nvmxip.h"
> 
> move to end
> 
> > +#include <test/test.h>
> > +#include <test/ut.h>
> > +
> > +/* 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] = {
> > +       {0x08000000, 9, 4096}, {0x08200000, 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].lba != plat_data->lba);
> > +
> > +               /* before reading all the flash blocks, let's calculate the flash size */
> > +               flashsz = plat_data->lba << plat_data->lba_shift;
> > +
> > +               /* allocate the user buffer where to copy the blocks data to */
> > +               buffer = calloc(flashsz, 1);
> > +               ut_assertok(!buffer);
> > +
> > +               /* the block device is the child of the parent device probed with DT*/
> > +               ut_assertok(device_find_first_child(dev, &bdev));
> > +
> > +               /* reading all the flash blocks*/
> > +               ut_asserteq(plat_data->lba, blk_read(bdev, 0, plat_data->lba, buffer));
> > +
> > +               /* compare the data read from flash with the expected data */
> > +               ut_assertok(dm_nvmxip_flash_sanity(device_idx, buffer));
> > +
> > +               free(buffer);
> > +       }
> > +
> > +       ut_assertok(device_idx != SANDBOX_NVMXIP_DEVICES);
> > +
> > +       return CMD_RET_SUCCESS;
> > +}
> > +
> > +DM_TEST(dm_test_nvmxip, UT_TESTF_SCAN_FDT | UT_TESTF_CONSOLE_REC);
> > --
> > 2.17.1
> >
> 
> It seems like you are using the same driver for sandbox as for the
> real hardware. Is that right? There is nothing really wrong with that,
> it's just unusual.

Yes, the driver is exactly the same for real HW and sandbox because the logic is the same. No need to duplicate it.

Cheers,
Abdellatif