Re: [PATCH v3 1/3] mtd: spi-nor: sfdp: parse SFDP SST vendor map and register EUI addresses into NVMEM framework

Claudiu Beznea
2025-06-07T11:07:37Z

Hi, Manikandan,

On 21.05.2025 10:03, Manikandan Muralidharan wrote:
> Some SST flash like SST26VF064BEUI serial quad flash memory is programmed
> at the factory with a globally unique EUI-48 and EUI-64 identifiers stored
> in the SFDP vendor parameter table and it is permanently write-protected.
>
> Add SST Vendor table SFDP parser to read the EUI-48 and EUI-64
> Mac Addresses and allocate them using resource-managed devm_kcalloc
> which will be freed on driver detach.
>
> Regitser the Addresses into NVMEM framework and parse them when
> requested using the nvmem properties in the DT by the net drivers.
> In kernel the Ethernet MAC address relied on U-Boot env variables or
> generated a random address, which posed challenges for boards without
> on-board EEPROMs or with multiple Ethernet ports.
> This change ensures consistent and reliable MAC address retrieval
> from QSPI benefiting boards like the sama5d27-wlsom1-ek, sama5d29 curiosity
> and sam9x75 curiosity.
>
> Signed-off-by: Manikandan Muralidharan <manikandan.m@microchip.com>
> ---
> drivers/mtd/spi-nor/sfdp.c | 161 ++++++++++++++++++++++++++++++++++++
> include/linux/mtd/spi-nor.h | 7 ++
> 2 files changed, 168 insertions(+)
>
> diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
> index 21727f9a4ac6..920708ae928a 100644
> --- a/drivers/mtd/spi-nor/sfdp.c
> +++ b/drivers/mtd/spi-nor/sfdp.c
> @@ -31,6 +31,7 @@
> * Register Map Offsets for Multi-Chip
> * SPI Memory Devices.
> */
> +#define SFDP_MCHP_SST_ID 0x01bf
>
> #define SFDP_SIGNATURE 0x50444653U
>
> @@ -1344,6 +1345,163 @@ static int spi_nor_parse_sccr_mc(struct spi_nor *nor,
> return ret;
> }
>
> +#define SFDP_MCHP_PARAM_TABLE_LEN 28
> +#define SFDP_SST26VF064BEUI_ID 0xFF4326BFU
> +
> +#define SFDP_MCHP_EUI48 0x30
> +#define SFDP_MCHP_EUI48_MASK GENMASK(7, 0)
> +#define SFDP_MCHP_EUI48_MAC_LEN 6
> +
> +#define SFDP_MCHP_EUI64 0x40
> +#define SFDP_MCHP_EUI64_MASK GENMASK(31, 24)
> +#define SFDP_MCHP_EUI64_MAC_LEN 8
> +
> +/**
> + * spi_nor_mchp_sfdp_read_addr()- read callback to copy the EUI-48 or EUI-68
> + * Addresses for device that request via NVMEM
> + *
> + * @priv: User context passed to read callbacks.
> + * @offset: Offset within the NVMEM device.
> + * @val: pointer where to fill the ethernet address
> + * @bytes: Length of the NVMEM cell
> + *
> + * Return: 0 on success, -EINVAL otherwise.
> + */
> +static int spi_nor_mchp_sfdp_read_addr(void *priv, unsigned int off,
> + void *val, size_t bytes)
> +{
> + struct spi_nor *nor = priv;
> +
> + if (SFDP_MCHP_PARAM_TABLE_LEN == nor->mchp_eui->vendor_param_length) {

From checkpatch.pl:

[Checkpatch] WARNING: Comparisons should place the constant on the
right side of the test
#71: FILE: drivers/mtd/spi-nor/sfdp.c:1375:
+ if (SFDP_MCHP_PARAM_TABLE_LEN == nor->mchp_eui->vendor_param_length) {

Also, to avoid indenting the above block you can reverse the check here,
and return, e.g.:

if (nor->mchp_eui->vendor_param_length != SFDP_MCHP_PARAM_TABLE_LEN)
return 0;

> + switch (bytes) {
> + case SFDP_MCHP_EUI48_MAC_LEN:
> + memcpy(val, nor->mchp_eui->ethaddr_eui48, SFDP_MCHP_EUI48_MAC_LEN);
> + break;
> + case SFDP_MCHP_EUI64_MAC_LEN:
> + memcpy(val, nor->mchp_eui->ethaddr_eui64, SFDP_MCHP_EUI64_MAC_LEN);
> + break;
> + default:
> + return -EINVAL;
> + }
> + }
> +
> + return 0;
> +}
> +
> +/**
> + * spi_nor_parse_mchp_sfdp() - Parse the Microchip vendor specific parameter table
> + * Read and store the EUI-48 and EUI-64 address to
> + * struct spi_nor_sst_mchp_eui_info if the addresses are
> + * programmed in the SST26VF064BEUI sst flag
> + *
> + * @nor: pointer to a 'struct spi_nor'
> + * @sccr_header: pointer to the 'struct sfdp_parameter_header' describing
> + * the Microchip vendor parameter header length and version.
> + *
> + * Return: 0 on success of if addresses are not programmed, -errno otherwise.
> + */
> +static int spi_nor_parse_mchp_sfdp(struct spi_nor *nor,
> + const struct sfdp_parameter_header *mchp_header)
> +{
> + struct nvmem_device *nvmem;
> + struct nvmem_config nvmem_config = { };
> + struct spi_nor_sst_mchp_eui_info *mchp_eui;
> + u32 *dwords, addr, sst_flash_id;
> + size_t len;
> + int ret = 0, size = 0;
> +
> + if (SFDP_MCHP_PARAM_TABLE_LEN != mchp_header->length)

From checkpatch.pl:

WARNING: Comparisons should place the constant on the right side of the test
#109: FILE: drivers/mtd/spi-nor/sfdp.c:1413:
+ if (SFDP_MCHP_PARAM_TABLE_LEN != mchp_header->length)

> + return -EINVAL;
> +
> + addr = SFDP_PARAM_HEADER_PTP(mchp_header);
> + /* Get the SST SPI NOR FLASH ID */
> + ret = spi_nor_read_sfdp_dma_unsafe(nor, addr, sizeof(sst_flash_id),
> + &sst_flash_id);
> + if (ret < 0)
> + return ret;
> +
> + /* Check the SPI NOR FLASH ID */
> + if (le32_to_cpu(sst_flash_id) != SFDP_SST26VF064BEUI_ID)
> + return -EINVAL;
> +
> + len = mchp_header->length * sizeof(*dwords);
> + dwords = kmalloc(len, GFP_KERNEL);

I think this can be replaced by:

u32 *dwords __free(kfree) = kmalloc(...);

> + if (!dwords)
> + return -ENOMEM;
> +
> + ret = spi_nor_read_sfdp(nor, addr, len, dwords);
> + if (ret)
> + goto out;
> +
> + le32_to_cpu_array(dwords, mchp_header->length);
> +
> + mchp_eui = devm_kzalloc(nor->dev, sizeof(*mchp_eui), GFP_KERNEL);
> + if (!mchp_eui) {
> + ret = -ENOMEM;
> + goto out;
> + }
> +
> + if (SFDP_MCHP_EUI48 == FIELD_GET(SFDP_MCHP_EUI48_MASK,
> + dwords[SFDP_DWORD(25)])) {
> + mchp_eui->ethaddr_eui48 = devm_kcalloc(nor->dev,
> + SFDP_MCHP_EUI48_MAC_LEN,
> + sizeof(u8), GFP_KERNEL);
> + if (!mchp_eui->ethaddr_eui48) {
> + ret = -ENOMEM;
> + devm_kfree(nor->dev, mchp_eui);
> + goto out;
> + }
> + memcpy(mchp_eui->ethaddr_eui48, (u8 *)&dwords[SFDP_DWORD(25)] + 1,
> + SFDP_MCHP_EUI48_MAC_LEN);
> + size = SFDP_MCHP_EUI48_MAC_LEN;
> + }
> +
> + if (SFDP_MCHP_EUI64 == FIELD_GET(SFDP_MCHP_EUI64_MASK,
> + dwords[SFDP_DWORD(26)])) {
> + mchp_eui->ethaddr_eui64 = devm_kcalloc(nor->dev,
> + SFDP_MCHP_EUI64_MAC_LEN,
> + sizeof(u8), GFP_KERNEL);
> + if (!mchp_eui->ethaddr_eui64) {
> + ret = -ENOMEM;
> + devm_kfree(nor->dev, mchp_eui->ethaddr_eui48);
> + devm_kfree(nor->dev, mchp_eui);
> + goto out;
> + }
> + memcpy(mchp_eui->ethaddr_eui64, (u8 *)&dwords[SFDP_DWORD(27)],
> + SFDP_MCHP_EUI64_MAC_LEN);
> + size += SFDP_MCHP_EUI64_MAC_LEN;
> + }
> +
> + /*
> + * Return if SST26VF064BEUI sst flash is not programmed
> + * with EUI-48 or EUI-64 information
> + */
> + if (!size) {
> + devm_kfree(nor->dev, mchp_eui);
> + goto out;
> + }
> +
> + mchp_eui->vendor_param_length = mchp_header->length;
> + nor->mchp_eui = mchp_eui;
> + nvmem_config.word_size = 1;
> + nvmem_config.stride = 1;
> + nvmem_config.dev = nor->dev;
> + nvmem_config.size = size;
> + nvmem_config.priv = nor;
> + nvmem_config.reg_read = spi_nor_mchp_sfdp_read_addr;
> +
> + nvmem = devm_nvmem_register(nor->dev, &nvmem_config);
> + if (IS_ERR(nvmem)) {
> + dev_err(nor->dev, "failed to register NVMEM device: %ld\n",
> + PTR_ERR(nvmem));
> + ret = PTR_ERR(nvmem);
> + }
> +
> +out:
> + kfree(dwords);
> + return ret;
> +}
> +
> /**
> * spi_nor_post_sfdp_fixups() - Updates the flash's parameters and settings
> * after SFDP has been parsed. Called only for flashes that define JESD216 SFDP
> @@ -1564,6 +1722,9 @@ int spi_nor_parse_sfdp(struct spi_nor *nor)
> err = spi_nor_parse_sccr_mc(nor, param_header);
> break;
>
> + case SFDP_MCHP_SST_ID:
> + err = spi_nor_parse_mchp_sfdp(nor, param_header);
> + break;
> default:
> break;
> }
> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
> index cdcfe0fd2e7d..051078d23ea1 100644
> --- a/include/linux/mtd/spi-nor.h
> +++ b/include/linux/mtd/spi-nor.h
> @@ -339,6 +339,12 @@ struct flash_info;
> struct spi_nor_manufacturer;
> struct spi_nor_flash_parameter;
>
> +struct spi_nor_sst_mchp_eui_info {
> + u8 vendor_param_length;
> + u8 *ethaddr_eui48;
> + u8 *ethaddr_eui64;

you may want to keep pointer first to avoid padding, if any.

Thank you,
Claudiu

> +};
> +
> /**
> * struct spi_nor - Structure for defining the SPI NOR layer
> * @mtd: an mtd_info structure
> @@ -408,6 +414,7 @@ struct spi_nor {
> u32 flags;
> enum spi_nor_cmd_ext cmd_ext_type;
> struct sfdp *sfdp;
> + struct spi_nor_sst_mchp_eui_info *mchp_eui;
> struct dentry *debugfs_root;
>
> const struct spi_nor_controller_ops *controller_ops;