Hi,
On Mon, 16 Jan 2023 at 10:28, <abdellatif.elkhl...@arm.com> wrote:
>
> From: Abdellatif El Khlifi <abdellatif.elkhl...@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.elkhl...@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.elkhl...@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 <sean...@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.elkhl...@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?
> 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-off...@arm.com>
> +# Authors:
> +# Abdellatif El Khlifi <abdellatif.elkhl...@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-off...@arm.com>
> +# Authors:
> +# Abdellatif El Khlifi <abdellatif.elkhl...@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-off...@arm.com>
> + *
> + * Authors:
> + * Abdellatif El Khlifi <abdellatif.elkhl...@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-off...@arm.com>
> + *
> + * Authors:
> + * Abdellatif El Khlifi <abdellatif.elkhl...@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-off...@arm.com>
> + *
> + * Authors:
> + * Abdellatif El Khlifi <abdellatif.elkhl...@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.
> +
> +#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-off...@arm.com>
> + *
> + * Authors:
> + * Abdellatif El Khlifi <abdellatif.elkhl...@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
