Re: [PATCH v1 1/6] drivers/nvmxip: introduce NVM XIP block storage emulation

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

On Tue, Feb 07, 2023 at 11:38:57AM -0700, Simon Glass wrote:
> Hi,
> 
> On Mon, 16 Jan 2023 at 10:28, <abdellatif.elkhlifi@arm.com> wrote:
> >
> > From: Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
> >
> > add block storage emulation for NVM XIP flash devices
> >
> > Some paltforms such as Corstone-1000 need to see NVM XIP raw flash
> > as a block storage device with read only capability.
> 
> As mentioned I didn't see this patch originally. It generally looks OK
> but have made some comments below.
> 
> >
> > Here NVM flash devices are devices with addressable
> > memory (e.g: QSPI NOR flash).
> >
> > The implementation is generic and can be used by different platforms.
> >
> > Two drivers are provided as follows.
> >
> >   nvmxip-blk :
> >
> >     a generic block driver allowing to read from the XIP flash
> >
> >   nvmxip_qspi :
> >
> >     The driver probed with the DT and parent of the nvmxip-blk device.
> >     nvmxip_qspi can be reused by other platforms. If the platform
> >     has custom settings to apply before using the flash, then the platform
> >     can provide its own parent driver belonging to UCLASS_NVMXIP and reuse
> >     nvmxip-blk. The custom driver can be implmented like nvmxip_qspi in
> >     addition to the platform custom settings.
> >
> > Platforms can use multiple NVM XIP devices at the same time by defining a
> > DT node for each one of them.
> >
> > For more details please refer to doc/develop/driver-model/nvmxip.rst
> >
> > Signed-off-by: Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
> > ---
> >  MAINTAINERS                                |   7 ++
> >  doc/develop/driver-model/index.rst         |   1 +
> >  doc/develop/driver-model/nvmxip.rst        |  70 +++++++++++
> >  doc/device-tree-bindings/nvmxip/nvmxip.txt |  56 +++++++++
> >  drivers/Kconfig                            |   2 +
> >  drivers/Makefile                           |   1 +
> >  drivers/block/blk-uclass.c                 |   1 +
> >  drivers/nvmxip/Kconfig                     |  19 +++
> >  drivers/nvmxip/Makefile                    |   8 ++
> >  drivers/nvmxip/nvmxip-uclass.c             |  15 +++
> >  drivers/nvmxip/nvmxip.c                    | 129 +++++++++++++++++++++
> >  drivers/nvmxip/nvmxip.h                    |  48 ++++++++
> >  drivers/nvmxip/nvmxip_qspi.c               |  67 +++++++++++
> >  include/dm/uclass-id.h                     |   1 +
> >  14 files changed, 425 insertions(+)
> >  create mode 100644 doc/develop/driver-model/nvmxip.rst
> >  create mode 100644 doc/device-tree-bindings/nvmxip/nvmxip.txt
> >  create mode 100644 drivers/nvmxip/Kconfig
> >  create mode 100644 drivers/nvmxip/Makefile
> >  create mode 100644 drivers/nvmxip/nvmxip-uclass.c
> >  create mode 100644 drivers/nvmxip/nvmxip.c
> >  create mode 100644 drivers/nvmxip/nvmxip.h
> >  create mode 100644 drivers/nvmxip/nvmxip_qspi.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index b2de50ccfc..539d01f68c 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -1193,6 +1193,13 @@ F:       cmd/nvme.c
> >  F:     include/nvme.h
> >  F:     doc/develop/driver-model/nvme.rst
> >
> > +NVMXIP
> > +M:     Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
> > +S:     Maintained
> > +F:     doc/develop/driver-model/nvmxip.rst
> > +F:     doc/device-tree-bindings/nvmxip/nvmxip.txt
> > +F:     drivers/nvmxip/
> > +
> >  NVMEM
> >  M:     Sean Anderson <seanga2@gmail.com>
> >  S:     Maintained
> > diff --git a/doc/develop/driver-model/index.rst b/doc/develop/driver-model/index.rst
> > index 7366ef818c..8e12bbd936 100644
> > --- a/doc/develop/driver-model/index.rst
> > +++ b/doc/develop/driver-model/index.rst
> > @@ -20,6 +20,7 @@ subsystems
> >     livetree
> >     migration
> >     nvme
> > +   nvmxip
> >     of-plat
> >     pci-info
> >     pmic-framework
> > diff --git a/doc/develop/driver-model/nvmxip.rst b/doc/develop/driver-model/nvmxip.rst
> > new file mode 100644
> > index 0000000000..91b24e4e50
> > --- /dev/null
> > +++ b/doc/develop/driver-model/nvmxip.rst
> > @@ -0,0 +1,70 @@
> > +.. SPDX-License-Identifier: GPL-2.0+
> > +
> > +NVM XIP Block Storage Emulation Driver
> > +=======================================
> > +
> > +Summary
> > +-------
> > +
> > +Non-Volatile Memory devices with addressable memory (e.g: QSPI NOR flash) could
> > +be used for block storage needs (e.g: parsing a GPT layout in a raw QSPI NOR flash).
> > +
> > +The NVMXIP class provides this functionality and can be used for any 64-bit platform.
> > +
> > +The NVMXIP class provides the following drivers:
> > +
> > +      nvmxip-blk :
> > +
> > +        A generic block driver allowing to read from the XIP flash.
> > +       The driver belongs to UCLASS_BLK.
> > +       The driver implemented by drivers/nvmxip/nvmxip.c
> > +
> > +      nvmxip_qspi :
> > +
> > +        The driver probed with the DT and parent of the nvmxip-blk device.
> > +        nvmxip_qspi can be reused by other platforms. If the platform
> > +        has custom settings to apply before using the flash, then the platform
> > +        can provide its own parent driver belonging to UCLASS_NVMXIP and reuse
> > +        nvmxip-blk. The custom driver can be implmented like nvmxip_qspi in
> > +        addition to the platform custom settings.
> > +       The nvmxip_qspi driver belongs to UCLASS_NVMXIP.
> > +       The driver implemented by drivers/nvmxip/nvmxip_qspi.c
> > +
> > +    The implementation is generic and can be used by different platforms.
> > +
> > +Supported hardware
> > +--------------------------------
> > +
> > +Any 64-bit plaform.
> 
> platform
> 
> Why not 64-bit ? Please mention that.
> 
> > +
> > +Configuration
> > +----------------------
> > +
> > +config NVMXIP
> > +         This option allows the emulation of a block storage device
> > +         on top of a direct access non volatile memory XIP flash devices.
> > +         This support provides the read operation.
> > +         This option provides the block storage driver nvmxip-blk which
> > +         handles the read operation. This driver is HW agnostic and can support
> > +         multiple flash devices at the same time.
> > +
> > +config NVMXIP_QSPI
> > +         This option allows the emulation of a block storage device on top of a QSPI XIP flash.
> > +         Any platform that needs to emulate one or multiple XIP flash devices can turn this
> > +         option on to enable the functionality. NVMXIP config is selected automatically.
> > +         Platforms that need to add custom treatments before accessing to the flash, can
> > +         write their own driver (same as nvmxip_qspi in addition to the custom settings).
> > +
> > +Device Tree nodes
> > +--------------------
> > +
> > +Multiple XIP flash devices can be used at the same time by describing them through DT
> > +nodes.
> > +
> > +Please refer to the documentation of the DT binding at:
> > +
> > +doc/device-tree-bindings/nvmxip/nvmxip.txt
> > +
> > +Contributors
> > +------------
> > +   * Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
> > diff --git a/doc/device-tree-bindings/nvmxip/nvmxip.txt b/doc/device-tree-bindings/nvmxip/nvmxip.txt
> > new file mode 100644
> > index 0000000000..7c4b03f66b
> > --- /dev/null
> > +++ b/doc/device-tree-bindings/nvmxip/nvmxip.txt
> > @@ -0,0 +1,56 @@
> > +Specifying NVMXIP information for devices
> > +======================================
> > +
> > +NVM XIP flash device nodes
> > +---------------------------
> > +
> > +Each flash device should have its own node.
> > +
> > +Each node must specify the following fields:
> > +
> > +1)
> > +               compatible = "nvmxip,qspi";
> > +
> > +This allows to bind the flash device with the nvmxip_qspi driver
> > +If a platform has its own driver, please provide your own compatible
> > +string.
> > +
> > +2)
> > +               reg = <0x0 0x08000000 0x0 0x00200000>;
> 
> Do we not use regs = /bits/ 64 <...? ?
> 
> > +
> > +The start address and size of the flash device. The values give here are an
> > +example (when the cell size is 2).
> > +
> > +When cell size is 1, the reg field looks like this:
> > +
> > +               reg = <0x08000000 0x00200000>;
> > +
> > +3)
> > +
> > +               lba_shift = <9>;
> > +
> > +The number of bit shifts used to calculate the size in bytes of one block.
> > +In this example the block size is 1 << 9 = 2 ^ 9 = 512 bytes
> > +
> > +4)
> > +
> > +               lba = <4096>;
> > +
> > +The number of blocks.
> > +
> > +Example of multiple flash devices
> > +----------------------------------------------------
> > +
> > +       nvmxip-qspi1@08000000 {
> > +               compatible = "nvmxip,qspi";
> > +               reg = <0x0 0x08000000 0x0 0x00200000>;
> > +               lba_shift = <9>;
> > +               lba = <4096>;
> > +       };
> > +
> > +       nvmxip-qspi2@08200000 {
> > +               compatible = "nvmxip,qspi";
> > +               reg = <0x0 0x08200000 0x0 0x00100000>;
> > +               lba_shift = <9>;
> > +               lba = <2048>;
> > +       };
> > diff --git a/drivers/Kconfig b/drivers/Kconfig
> > index 9101e538b0..ef321bee94 100644
> > --- a/drivers/Kconfig
> > +++ b/drivers/Kconfig
> > @@ -76,6 +76,8 @@ source "drivers/net/Kconfig"
> >
> >  source "drivers/nvme/Kconfig"
> >
> > +source "drivers/nvmxip/Kconfig"
> > +
> >  source "drivers/pci/Kconfig"
> >
> >  source "drivers/pci_endpoint/Kconfig"
> > diff --git a/drivers/Makefile b/drivers/Makefile
> > index 83b14ef1fd..5e7fd6dab5 100644
> > --- a/drivers/Makefile
> > +++ b/drivers/Makefile
> > @@ -89,6 +89,7 @@ obj-$(CONFIG_FWU_MDATA) += fwu-mdata/
> >  obj-y += misc/
> >  obj-$(CONFIG_MMC) += mmc/
> >  obj-$(CONFIG_NVME) += nvme/
> > +obj-$(CONFIG_NVMXIP) += nvmxip/
> 
> Do you think this justifies its own directory? Do we expect more
> drivers? Another option would be drivers/mtd
> 
> Is there an equivalent Linux driver?

Yes we can have more drivers using the NVMXIP Uclass
(devices that need to run board specific treatment before accessing the NVM XIP flash).
Anyway, I moved the NVMXIP folder under mtd.
Regarding Linux, I'm not aware of any driver doing the same thing.

> 
> >  obj-$(CONFIG_PCI_ENDPOINT) += pci_endpoint/
> >  obj-y += dfu/
> >  obj-$(CONFIG_PCH) += pch/
> > diff --git a/drivers/block/blk-uclass.c b/drivers/block/blk-uclass.c
> > index c69fc4d518..e8ab576c32 100644
> > --- a/drivers/block/blk-uclass.c
> > +++ b/drivers/block/blk-uclass.c
> > @@ -28,6 +28,7 @@ static struct {
> >         { UCLASS_AHCI, "sata" },
> >         { UCLASS_HOST, "host" },
> >         { UCLASS_NVME, "nvme" },
> > +       { UCLASS_NVMXIP, "nvmxip" },
> >         { UCLASS_EFI_MEDIA, "efi" },
> >         { UCLASS_EFI_LOADER, "efiloader" },
> >         { UCLASS_VIRTIO, "virtio" },
> > diff --git a/drivers/nvmxip/Kconfig b/drivers/nvmxip/Kconfig
> > new file mode 100644
> > index 0000000000..3ef7105026
> > --- /dev/null
> > +++ b/drivers/nvmxip/Kconfig
> > @@ -0,0 +1,19 @@
> > +# SPDX-License-Identifier: GPL-2.0+
> > +#
> > +# Copyright 2023 Arm Limited and/or its affiliates <open-source-office@arm.com>
> > +# Authors:
> > +#   Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
> > +
> > +config NVMXIP
> > +       bool "NVM XIP devices support"
> > +       select BLK
> > +       help
> > +         This option allows the emulation of a block storage device
> > +         on top of a direct access non volatile memory XIP flash devices.
> > +         This support provides the read operation.
> > +
> > +config NVMXIP_QSPI
> > +       bool "QSPI XIP  support"
> > +       select NVMXIP
> > +       help
> > +         This option allows the emulation of a block storage device on top of a QSPI XIP flash
> > diff --git a/drivers/nvmxip/Makefile b/drivers/nvmxip/Makefile
> > new file mode 100644
> > index 0000000000..54eacc102e
> > --- /dev/null
> > +++ b/drivers/nvmxip/Makefile
> > @@ -0,0 +1,8 @@
> > +# SPDX-License-Identifier: GPL-2.0+
> > +#
> > +# Copyright 2023 Arm Limited and/or its affiliates <open-source-office@arm.com>
> > +# Authors:
> > +#   Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
> > +
> > +obj-y += nvmxip-uclass.o nvmxip.o
> > +obj-$(CONFIG_NVMXIP_QSPI) += nvmxip_qspi.o
> > diff --git a/drivers/nvmxip/nvmxip-uclass.c b/drivers/nvmxip/nvmxip-uclass.c
> > new file mode 100644
> > index 0000000000..aed8e163d2
> > --- /dev/null
> > +++ b/drivers/nvmxip/nvmxip-uclass.c
> > @@ -0,0 +1,15 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * 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 <dm.h>
> > +
> > +UCLASS_DRIVER(nvmxip) = {
> > +       .name   = "nvmxip",
> > +       .id     = UCLASS_NVMXIP,
> > +};
> > diff --git a/drivers/nvmxip/nvmxip.c b/drivers/nvmxip/nvmxip.c
> > new file mode 100644
> > index 0000000000..c276fb147e
> > --- /dev/null
> > +++ b/drivers/nvmxip/nvmxip.c
> > @@ -0,0 +1,129 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * 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 <dm.h>
> > +#include <dm/device-internal.h>
> > +#include "nvmxip.h"
> > +
> > +static u32 nvmxip_bdev_max_devs;
> 
> Please put that in the uclass-private data. We don't want statics,
> etc. with driver model.
> 
> > +
> > +static int nvmxip_mmio_rawread(const phys_addr_t address, u64 *value)
> > +{
> > +       *value = readq(address);
> > +       return 0;
> > +}
> > +
> > +static ulong nvmxip_blk_read(struct udevice *udev, lbaint_t blknr, lbaint_t blkcnt, void *buffer)
> > +{
> > +       struct nvmxip_blk_priv *bpriv_data = dev_get_priv(udev);
> > +       struct blk_desc *desc = dev_get_uclass_plat(udev);
> > +
> 
> drop blank line, and can you combine the two following comments:
> 
> > +       /* size of 1 block */
> > +       /* number of the u64 words to read */
> > +       u32 qwords = (blkcnt * desc->blksz) / sizeof(u64);
> > +       /* physical address of the first block to read */
> > +       phys_addr_t blkaddr = bpriv_data->pplat_data->phys_base + blknr * desc->blksz;
> > +       u64 *virt_blkaddr;
> > +       u64 *pdst = buffer;
> > +       u32 qdata_idx;
> 
> Why is this u32? It should be uint. Try to use natural types for loops
> and function parameters unless there is a very good reason.
> 
> > +
> > +       if (!pdst)
> > +               return -EINVAL;
> > +
> > +       pr_debug("[%s]: reading from blknr: %lu , blkcnt: %lu\n", udev->name, blknr, blkcnt);
> > +
> > +       virt_blkaddr = map_sysmem(blkaddr, 0);
> > +
> > +       /* assumption: the data is virtually contiguous */
> > +
> [..]
> 
> > +
> > +int nvmxip_init(struct udevice *udev)
> 
> Why is this function exported? It should probably be called
> nvmxip_post_bind() and be called by the uclass when a new device is
> bound?
> 
> > +{
> > +       struct nvmxip_plat *plat_data = dev_get_plat(udev);
> 
> plat
> 
> > +       struct nvmxip_priv *priv_data = dev_get_priv(udev);
> 
> priv
> 
> (please use these names throughout as that is the convention; it makes
> it easier for people to understand your code)
> 
> > +       int ret;
> > +       struct udevice *bdev = NULL;
> > +       char bdev_name[NVMXIP_BLKDEV_NAME_SZ + 1] = {0};
> 
> You don't need to init it as you write to it below.
> 
> > +
> > +       priv_data->udev = udev;
> > +       priv_data->plat_data = plat_data;
> > +
> > +       nvmxip_bdev_max_devs++;
> > +
> > +       snprintf(bdev_name, NVMXIP_BLKDEV_NAME_SZ, "nvmxip-blk#%d", nvmxip_bdev_max_devs);
> > +
> > +       ret = blk_create_devicef(udev, NVMXIP_BLKDRV_NAME, bdev_name, UCLASS_NVMXIP,
> > +                                nvmxip_bdev_max_devs, NVMXIP_DEFAULT_LBA_SZ,
> > +                                NVMXIP_DEFAULT_LBA_COUNT, &bdev);
> > +       if (ret) {
> > +               pr_err("[%s]: failure during creation of the block device %s, error %d\n",
> > +                      udev->name, bdev_name, ret);
> > +               goto blkdev_setup_error;
> > +       }
> > +
> > +       ret = blk_probe_or_unbind(bdev);
> > +       if (ret) {
> > +               pr_err("[%s]: failure during probing the block device %s, error %d\n",
> > +                      udev->name, bdev_name, ret);
> > +               goto blkdev_setup_error;
> > +       }
> > +
> > +       pr_info("[%s]: the block device %s ready for use\n", udev->name, bdev_name);
> > +
> > +       return 0;
> > +
> > +blkdev_setup_error:
> > +       nvmxip_bdev_max_devs--;
> 
> I'm not sure if it helps, but there is a uclass_id_count() you can use.
> 
> > +       return ret;
> > +}
> > +
> > +static const struct blk_ops nvmxip_blk_ops = {
> > +       .read   = nvmxip_blk_read,
> > +};
> > +
> > +U_BOOT_DRIVER(nvmxip_blk) = {
> > +       .name   = NVMXIP_BLKDRV_NAME,
> > +       .id     = UCLASS_BLK,
> > +       .probe  = nvmxip_blk_probe,
> > +       .ops    = &nvmxip_blk_ops,
> > +       .priv_auto      = sizeof(struct nvmxip_blk_priv),
> > +};
> > diff --git a/drivers/nvmxip/nvmxip.h b/drivers/nvmxip/nvmxip.h
> > new file mode 100644
> > index 0000000000..47ae3bd8ab
> > --- /dev/null
> > +++ b/drivers/nvmxip/nvmxip.h
> > @@ -0,0 +1,48 @@
> > +/* SPDX-License-Identifier: GPL-2.0+ */
> > +/*
> > + * Copyright 2023 Arm Limited and/or its affiliates <open-source-office@arm.com>
> > + *
> > + * Authors:
> > + *   Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
> > + */
> > +
> > +#ifndef __DRIVER_NVMXIP_H__
> > +#define __DRIVER_NVMXIP_H__
> > +
> > +#include <asm/io.h>
> > +#include <blk.h>
> > +#include <linux/bitops.h>
> > +#include <linux/compat.h>
> > +#include <mapmem.h>
> 
> Please put blk and mapmem in the C file, at least. Check if you need
> the others here, too.

Done in patchset v2, except for blk which is needed by nvmxip.h (struct nvmxip_plat uses lbaint_t type )

Cheers,
Abdellatif

> 
> 
> > +
> > +#define NVMXIP_BLKDRV_NAME    "nvmxip-blk"
> > +
> > +#define NVMXIP_BLKDEV_NAME_SZ 20
> > +
> > +#define NVMXIP_DEFAULT_LBA_SHIFT 10 /* 1024 bytes per block */
> > +#define NVMXIP_DEFAULT_LBA_COUNT 1024 /* block count */
> > +
> > +#define NVMXIP_DEFAULT_LBA_SZ BIT(NVMXIP_DEFAULT_LBA_SHIFT)
> 
> This is a private header so you don't need the long names - e.g. drop
> the NVMXIP_
> > +
> > +/* NVM XIP device platform data */
> 
> I suggest using sphinx style right from the start, since this is a new file.
> 
> > +struct nvmxip_plat {
> > +       phys_addr_t phys_base; /* NVM XIP device base address */
> > +       u32 lba_shift; /* block size shift count (read from device tree) */
> > +       lbaint_t lba; /* number of blocks (read from device tree) */
> > +};
> > +
> > +/* NVM XIP device private data */
> > +struct nvmxip_priv {
> > +       struct udevice *udev;
> > +       struct nvmxip_plat *plat_data;
> 
> Please don't store private data from devices. You can use
> dev_get_plat() or whatever on a stored device. It just adds confusion.
> 
> > +};
> > +
> > +/* Private data of the block device associated with the NVM XIP device (the parent) */
> > +struct nvmxip_blk_priv {
> > +       struct udevice *bdev;
> 
> What is that?
> 
> > +       struct nvmxip_plat *pplat_data; /* parent device platform data */
> 
> same here
> 
> > +};
> > +
> > +int nvmxip_init(struct udevice *udev);
> 
> should not be exported
> 
> > +
> > +#endif /* __DRIVER_NVMXIP_H__ */
> > diff --git a/drivers/nvmxip/nvmxip_qspi.c b/drivers/nvmxip/nvmxip_qspi.c
> > new file mode 100644
> > index 0000000000..d0ce4c6633
> > --- /dev/null
> > +++ b/drivers/nvmxip/nvmxip_qspi.c
> 
> Can you put the driver in a separate patch? It is a good idea to have
> the uclass in a patch by itself.
> 
> > @@ -0,0 +1,67 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * 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 <dm.h>
> > +#include <fdt_support.h>
> > +#include "nvmxip.h"
> > +
> > +#include <asm/global_data.h>
> > +DECLARE_GLOBAL_DATA_PTR;
> > +
> > +#define NVMXIP_QSPI_DRV_NAME "nvmxip_qspi"
> > +
> > +static int nvmxip_qspi_probe(struct udevice *dev)
> > +{
> > +       pr_debug("[%s][%s]\n", __func__, dev->name);
> > +       return nvmxip_init(dev);
> > +}
> > +
> > +static int nvmxip_qspi_of_to_plat(struct udevice *dev)
> > +{
> > +       struct nvmxip_plat *plat_data = dev_get_plat(dev);
> > +       int ret;
> > +
> > +       plat_data->phys_base = (phys_addr_t)dev_read_addr(dev);
> > +       if (plat_data->phys_base == FDT_ADDR_T_NONE) {
> > +               pr_err("[%s]: can not get base address from device tree\n", dev->name);
> 
> This is adding a lot to code size...do you want to use debug instead?
> 
> > +               return -EINVAL;
> > +       }
> > +
> > +       ret = dev_read_u32(dev, "lba_shift", &plat_data->lba_shift);
> > +       if (ret) {
> > +               pr_err("[%s]: can not get lba_shift from device tree\n", dev->name);
> > +               return -EINVAL;
> > +       }
> > +
> > +       ret = dev_read_u32(dev, "lba", (u32 *)&plat_data->lba);
> > +       if (ret) {
> > +               pr_err("[%s]: can not get lba from device tree\n", dev->name);
> > +               return -EINVAL;
> > +       }
> > +
> > +       pr_debug("[%s]: XIP device base addr: 0x%llx , lba_shift: %d , lbas: %lu\n",
> > +                dev->name, plat_data->phys_base, plat_data->lba_shift, plat_data->lba);
> > +
> > +       return 0;
> > +}
> > +
> > +static const struct udevice_id nvmxip_qspi_ids[] = {
> > +       { .compatible = "nvmxip,qspi" },
> > +       { /* sentinel */ }
> > +};
> > +
> > +U_BOOT_DRIVER(nvmxip_qspi) = {
> > +       .name = NVMXIP_QSPI_DRV_NAME,
> > +       .id = UCLASS_NVMXIP,
> > +       .of_match = nvmxip_qspi_ids,
> > +       .of_to_plat = nvmxip_qspi_of_to_plat,
> > +       .priv_auto = sizeof(struct nvmxip_priv),
> > +       .plat_auto = sizeof(struct nvmxip_plat),
> > +       .probe = nvmxip_qspi_probe,
> > +};
> > diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h
> > index 376f741cc2..dcfe8cfe2c 100644
> > --- a/include/dm/uclass-id.h
> > +++ b/include/dm/uclass-id.h
> > @@ -88,6 +88,7 @@ enum uclass_id {
> >         UCLASS_NOP,             /* No-op devices */
> >         UCLASS_NORTHBRIDGE,     /* Intel Northbridge / SDRAM controller */
> >         UCLASS_NVME,            /* NVM Express device */
> > +       UCLASS_NVMXIP,          /* NVM XIP devices */
> >         UCLASS_P2SB,            /* (x86) Primary-to-Sideband Bus */
> >         UCLASS_PANEL,           /* Display panel, such as an LCD */
> >         UCLASS_PANEL_BACKLIGHT, /* Backlight controller for panel */
> > --
> > 2.17.1
> >
> 
> Regards,
> Simon