Read MAC address from SST vendor specific SFDP region

nvmem_register() stores config->of_node (or config->dev->of_node) into nvmem->dev.of_node without taking its own reference. Callers that get the node via of_get_compatible_child() etc. and drop their reference right after registering leave nvmem->dev.of_node backed by no reference at all.

Take an of_node_get() when assigning nvmem->dev.of_node and release it in nvmem_release(). Existing callers that hold their own get/put pair around the registration call need no changes, since get/put on a device_node is per-holder and independent of core now also holding one.

Suggested-by Michael Walle <[email protected]>

Signed-off-by Manikandan Muralidharan <[email protected]>

nvmem_register() stores config->of_node (or config->dev->of_node) into nvmem->dev.of_node without taking its own reference. Callers that get the node via of_get_compatible_child() etc. and drop their reference right after registering leave nvmem->dev.of_node backed by no reference at all.

Take an of_node_get() when assigning nvmem->dev.of_node and release it in nvmem_release(). Existing callers that hold their own get/put pair around the registration call need no changes, since get/put on a device_node is per-holder and independent of core now also holding one.

Suggested-by Michael Walle <[email protected]>

Signed-off-by Manikandan Muralidharan <[email protected]>

---
 drivers/nvmem/core.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

drivers/nvmem/core.c +1 -0

index 0556d140170a..98192597b935 100644
@@ -541,6 +541,7 @@ static void nvmem_release(struct device *dev)
 
 	ida_free(&nvmem_ida, nvmem->id);
 	gpiod_put(nvmem->wp_gpio);
+	of_node_put(nvmem->dev.of_node);
 	kfree(nvmem->ops);
 	kfree(nvmem);
 }

@@ -950,9 +951,9 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
 	nvmem->keepout = config->keepout;

Quoted 12 lines · Manikandan Muralidharan

nvmem_register() stores config->of_node (or config->dev->of_node) into nvmem->dev.of_node without taking its own reference. Callers that get the node via of_get_compatible_child() etc. and drop their reference right after registering leave nvmem->dev.of_node backed by no reference at all.

Take an of_node_get() when assigning nvmem->dev.of_node and release it in nvmem_release(). Existing callers that hold their own get/put pair around the registration call need no changes, since get/put on a device_node is per-holder and independent of core now also holding one.

Quoted 2 lines

Suggested-by Michael Walle <[email protected]>

Signed-off-by Manikandan Muralidharan <[email protected]>

Nice!

Did you check if any callers are also taking a reference, which might not be needed anymore now.

Reviewed-by Michael Walle <[email protected]>

-michael

Quoted 28 lines

---
  drivers/nvmem/core.c | 5 +++--
  1 file changed, 3 insertions(+), 2 deletions(-)

drivers/nvmem/core.c +3 -2

index 0556d140170a..98192597b935 100644
@@ -541,6 +541,7 @@ static void nvmem_release(struct device *dev)
 
  	ida_free(&nvmem_ida, nvmem->id);
  	gpiod_put(nvmem->wp_gpio);
+	of_node_put(nvmem->dev.of_node);
  	kfree(nvmem->ops);
  	kfree(nvmem);
  }

@@ -950,9 +951,9 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
  	nvmem->keepout = config->keepout;
  	nvmem->nkeepout = config->nkeepout;
  	if (config->of_node)
-		nvmem->dev.of_node = config->of_node;
+		nvmem->dev.of_node = of_node_get(config->of_node);
  	else
-		nvmem->dev.of_node = config->dev->of_node;
+		nvmem->dev.of_node = of_node_get(config->dev->of_node);
 
  	switch (config->id) {
  	case NVMEM_DEVID_NONE:

The SPI NOR core already reads the SFDP tables during enumeration and caches them in nor->sfdp->dwords (see spi_nor_parse_sfdp()). Re-expose that cached data as a read-only NVMEM device, in on-flash byte order, rooted at the flash's SFDP child node (compatible "jedec,sfdp").

This lets NVMEM cells reference any SFDP data: a fixed-layout for parameters at a known offset, or an nvmem-layout parser for vendor data whose location must be discovered at runtime. The device is only registered when an "sfdp" node is present in the device tree.

Signed-off-by Manikandan Muralidharan <[email protected]>

The SPI NOR core already reads the SFDP tables during enumeration and caches them in nor->sfdp->dwords (see spi_nor_parse_sfdp()). Re-expose that cached data as a read-only NVMEM device, in on-flash byte order, rooted at the flash's SFDP child node (compatible "jedec,sfdp").

This lets NVMEM cells reference any SFDP data: a fixed-layout for parameters at a known offset, or an nvmem-layout parser for vendor data whose location must be discovered at runtime. The device is only registered when an "sfdp" node is present in the device tree.

Signed-off-by Manikandan Muralidharan <[email protected]>

---
 drivers/mtd/spi-nor/core.c | 78 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 78 insertions(+)

drivers/mtd/spi-nor/core.c +32 -0

index 8bc117b46e02..4e47459b1679 100644
@@ -17,6 +17,7 @@
 #include <linux/mtd/mtd.h>
 #include <linux/mtd/spi-nor.h>
 #include <linux/mutex.h>
+#include <linux/nvmem-provider.h>
 #include <linux/of.h>
 #include <linux/regulator/consumer.h>
 #include <linux/sched/task_stack.h>

@@ -3001,6 +3002,75 @@ static void spi_nor_init_fixup_flags(struct spi_nor *nor)
 		nor->flags |= SNOR_F_IO_MODE_EN_VOLATILE;
 }
 
+static int spi_nor_sfdp_reg_read(void *priv, unsigned int offset,
+				 void *val, size_t bytes)
+{
+	struct spi_nor *nor = priv;
+	struct sfdp *sfdp = nor->sfdp;
+	size_t sfdp_size = sfdp->num_dwords * sizeof(*sfdp->dwords);
+
+	if (offset >= sfdp_size || bytes > sfdp_size - offset)
+		return -EINVAL;
+
+	/* The cached SFDP is kept in on-flash (little-endian) byte order. */
+	memcpy(val, (u8 *)sfdp->dwords + offset, bytes);
+
+	return 0;
+}
+
+/**
+ * spi_nor_register_sfdp_nvmem() - expose the SFDP as a read-only NVMEM device
+ * @nor:	pointer to a 'struct spi_nor'
+ *
+ * Expose the whole SFDP, in on-flash byte order, as a read-only NVMEM device
+ * rooted at the flash's SFDP child node (compatible "jedec,sfdp"). This lets
+ * generic (fixed-layout) or vendor (nvmem-layout) cells reference any SFDP
+ * data. The device is only registered when a child node with the "jedec,sfdp"
+ * compatible is described in the device tree.
+ *
+ * Return: 0 on success or if there is nothing to do, -errno otherwise.
+ */
+static int spi_nor_register_sfdp_nvmem(struct spi_nor *nor)
+{
+	struct device *dev = nor->dev;

drivers/mtd/spi-nor/core.c +36 -0

+
+	config.dev = dev;
+	config.of_node = np;
+	config.name = "sfdp";
+	config.id = NVMEM_DEVID_AUTO;
+	config.owner = THIS_MODULE;
+	config.read_only = true;
+	config.word_size = 1;
+	config.stride = 1;
+	config.size = (int)(nor->sfdp->num_dwords * sizeof(*nor->sfdp->dwords));
+	config.reg_read = spi_nor_sfdp_reg_read;
+	config.priv = nor;
+
+	nvmem = devm_nvmem_register(dev, &config);
+	of_node_put(np);
+	if (IS_ERR(nvmem)) {
+		/* NVMEM support is optional. */
+		if (PTR_ERR(nvmem) == -EOPNOTSUPP)
+			return 0;
+		return dev_err_probe(dev, PTR_ERR(nvmem),
+				     "failed to register SFDP NVMEM device\n");
+	}
+
+	dev_dbg(dev, "exposed %d-byte SFDP as an NVMEM device\n", config.size);
+
+	return 0;
+}
+
 /**
  * spi_nor_late_init_params() - Late initialization of default flash parameters.
  * @nor:	pointer to a 'struct spi_nor'

@@ -3204,6 +3274,14 @@ static int spi_nor_init_params(struct spi_nor *nor)
 		spi_nor_init_params_deprecated(nor);
 	}
 
+	/*
+	 * Expose the SFDP table as an NVMEM device only when
+	 * the flash actually provides one
+	 */
+	ret = spi_nor_register_sfdp_nvmem(nor);
+	if (ret)
+		return ret;
+
 	ret = spi_nor_late_init_params(nor);
 	if (ret)
 		return ret;

Add an NVMEM layout that exposes the factory-programmed EUI-48 identifier from the Microchip/SST vendor SFDP parameter table (e.g. SST26VF064BEUI) as a "mac-address" cell, for use as a network MAC address. The vendor table is located at runtime via the SFDP NVMEM device (no offset in DT), and a read_post_process callback reverses the LSB-first bytes into canonical MAC order. Binds to an "nvmem-layout" node with compatible "microchip,sst26vf-sfdp-eui".

Signed-off-by Manikandan Muralidharan <[email protected]>

drivers/nvmem/layouts/Makefile +14 -0

+/* SFDP header and parameter header, as laid out on the flash. */
+struct sfdp_header {
+	u8 signature[4];
+	u8 minor;
+	u8 major;
+	u8 nph;
+	u8 unused;
+};
+
+struct sfdp_parameter_header {
+	u8 id_lsb;
+	u8 minor;
+	u8 major;
+	u8 length;

drivers/nvmem/layouts/Makefile +10 -0

+		return -EINVAL;
+
+	return 0;
+}
+
+static int sfdp_eui_find_vendor_table(struct nvmem_device *nvmem, u32 *ptp)
+{
+	struct sfdp_parameter_header ph;
+	struct sfdp_header hdr;
+	int nph, i, ret;

drivers/nvmem/layouts/Makefile +14 -0

+		ret = nvmem_device_read(nvmem, sizeof(hdr) + i * sizeof(ph),
+					sizeof(ph), &ph);
+		if (ret < 0)
+			return ret;
+
+		if (SFDP_PARAM_HEADER_ID(&ph) != SFDP_MCHP_VENDOR_ID)
+			continue;
+
+		*ptp = SFDP_PARAM_HEADER_PTP(&ph);
+		return 0;
+	}
+
+	return -ENOENT;
+}

EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe

Hi,

Quoted 9 lines · Manikandan Muralidharan

Add an NVMEM layout that exposes the factory-programmed EUI-48 identifier from the Microchip/SST vendor SFDP parameter table (e.g. SST26VF064BEUI) as a "mac-address" cell, for use as a network MAC address. The vendor table is located at runtime via the SFDP NVMEM device (no offset in DT), and a read_post_process callback reverses the LSB-first bytes into canonical MAC order. Binds to an "nvmem-layout" node with compatible "microchip,sst26vf-sfdp-eui".

Signed-off-by Manikandan Muralidharan <[email protected]>

Mostly minor nits, see below.

drivers/nvmem/layouts/Makefile line 10

Quoted 8 lines

drivers/nvmem/layouts/Makefile +1 -0

@@ -6,4 +6,5 @@
  obj-$(CONFIG_NVMEM_LAYOUTS) += fixed-layout.o
  obj-$(CONFIG_NVMEM_LAYOUT_SL28_VPD) += sl28vpd.o
  obj-$(CONFIG_NVMEM_LAYOUT_ONIE_TLV) += onie-tlv.o
+obj-$(CONFIG_NVMEM_LAYOUT_SST26VF_SFDP_EUI) += sst26vf-sfdp-eui.o
  obj-$(CONFIG_NVMEM_LAYOUT_U_BOOT_ENV) += u-boot-env.o

Not sure this is ordered alphabetically, so in general add it at the end.

Quoted 14 lines

drivers/nvmem/layouts/Makefile +14 -0

+/* SFDP header and parameter header, as laid out on the flash. */
+struct sfdp_header {
+     u8 signature[4];
+     u8 minor;
+     u8 major;
+     u8 nph;
+     u8 unused;
+};
+
+struct sfdp_parameter_header {
+     u8 id_lsb;
+     u8 minor;
+     u8 major;
+     u8 length;

drivers/nvmem/layouts/Makefile

Quoted 10 lines

drivers/nvmem/layouts/Makefile +10 -0

+     u8 parameter_table_pointer[3];
+     u8 id_msb;
+};
+
+#define SFDP_SIGNATURE                       0x50444653U
+
+#define SFDP_PARAM_HEADER_ID(h)              (((h)->id_msb << 8) | (h)->id_lsb)
+#define SFDP_PARAM_HEADER_PTP(h)     get_unaligned_le24((h)->parameter_table_pointer)
+
+/* Microchip (vendor) parameter table identifier: id_msb << 8 | id_lsb. */

This is not relevant, we saw it just above ^^^^^^^^^^^^^^^^^^^^^^^

drivers/nvmem/layouts/Makefile

Quoted 13 lines

drivers/nvmem/layouts/Makefile +13 -0

+#define SFDP_MCHP_VENDOR_ID          0x01bf
+
+#define SFDP_MCHP_EUI48_MARKER_OFFSET        0x60
+#define SFDP_MCHP_EUI48_MARKER               0x30
+#define SFDP_MCHP_EUI48_OFFSET               0x61
+
+static int sfdp_eui_read_post_process(void *priv, const char *id, int index,
+                                   unsigned int offset, void *buf,
+                                   size_t bytes)
+{
+     u8 *data = buf;
+     int i;
+

I believe the if against the length is wrong and should be inverted and placed here:

         if (bytes != ETH_ALEN)
                 return -EINVAL;

Since lenght does not change, prefer to bail out early.

drivers/nvmem/layouts/Makefile

Quoted 5 lines

drivers/nvmem/layouts/Makefile +5 -0

+     /* SFDP stores the address least-significant octet first; reverse it. */
+     for (i = 0; i < bytes / 2; i++)
+             swap(data[i], data[bytes - 1 - i]);
+
+     if (bytes == ETH_ALEN && !is_valid_ether_addr(buf))

Is bytes != ETH_ALEN a valid use case?

Quoted 10 lines

drivers/nvmem/layouts/Makefile +10 -0

+             return -EINVAL;
+
+     return 0;
+}
+
+static int sfdp_eui_find_vendor_table(struct nvmem_device *nvmem, u32 *ptp)
+{
+     struct sfdp_parameter_header ph;
+     struct sfdp_header hdr;
+     int nph, i, ret;

drivers/nvmem/layouts/Makefile

Quoted 10 lines

drivers/nvmem/layouts/Makefile +10 -0

+
+     ret = nvmem_device_read(nvmem, 0, sizeof(hdr), &hdr);
+     if (ret < 0)
+             return ret;
+
+     if (get_unaligned_le32(hdr.signature) != SFDP_SIGNATURE)
+             return -EINVAL;
+
+     /* The number of parameter headers (NPH) field is zero-based. */
+     nph = hdr.nph;

nph = hdr.nph + 1 ?

drivers/nvmem/layouts/Makefile

Quoted 2 lines

drivers/nvmem/layouts/Makefile +2 -0

+
+     for (i = 0; i <= nph; i++) {

and here the usual i < nph?

Quoted 14 lines

drivers/nvmem/layouts/Makefile +14 -0

+             ret = nvmem_device_read(nvmem, sizeof(hdr) + i * sizeof(ph),
+                                     sizeof(ph), &ph);
+             if (ret < 0)
+                     return ret;
+
+             if (SFDP_PARAM_HEADER_ID(&ph) != SFDP_MCHP_VENDOR_ID)
+                     continue;
+
+             *ptp = SFDP_PARAM_HEADER_PTP(&ph);
+             return 0;
+     }
+
+     return -ENOENT;
+}

...

drivers/nvmem/layouts/Makefile

Quoted 11 lines

drivers/nvmem/layouts/Makefile +11 -0

+     info.name = "mac-address";
+     info.offset = base + SFDP_MCHP_EUI48_OFFSET;
+     info.bytes = ETH_ALEN;
+     info.np = of_get_child_by_name(layout_np, "mac-address");
+     info.read_post_process = sfdp_eui_read_post_process;
+
+     ret = nvmem_add_one_cell(nvmem, &info);
+     if (ret)
+             of_node_put(info.np);
+     else
+             dev_dbg(dev, "exposed EUI-48 at SFDP offset 0x%x\n", info.offset);

This one is not really useful anymore.

drivers/nvmem/layouts/Makefile

Quoted 6 lines

drivers/nvmem/layouts/Makefile +6 -0

+
+     of_node_put(layout_np);
+
+     return ret;
+}
+

Thanks,
Miquèl