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

[Claudiu Beznea <claudiu.beznea@tuxon.dev>]

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;


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/