Re: [PATCH 1/2] mtd: spi-nor: sst: register SFDP region into NVMEM framework to read MAC Address

["Michael Walle" <mwalle@kernel.org>]

On Wed Mar 5, 2025 at 11:01 AM CET, Manikandan Muralidharan wrote:
> From: Varshini Rajendran <varshini.rajendran@microchip.com>
>
> EUI identifier and the MAC Address of the Ethernet Interface is stored
> after the SFDP table of contents starting at address 0x260 in the
> QSPI memory.
> Register the entire SFDP region read by the spi-nor (nor->sfdp) into the
> NVMEM framework and read the MAC Address 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 sama5d29 curiosity and sam9x75 curiosity.
>
> Signed-off-by: Varshini Rajendran <varshini.rajendran@microchip.com>
> [manikandan.m@microchip.com: Integrate the nvmem->read callback framework]
> Signed-off-by: Manikandan Muralidharan <manikandan.m@microchip.com>
> ---
>  drivers/mtd/spi-nor/sst.c | 62 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 62 insertions(+)
>
> diff --git a/drivers/mtd/spi-nor/sst.c b/drivers/mtd/spi-nor/sst.c
> index 175211fe6a5e..a0abf201ad41 100644
> --- a/drivers/mtd/spi-nor/sst.c
> +++ b/drivers/mtd/spi-nor/sst.c
> @@ -5,6 +5,7 @@
>   */
>  
>  #include <linux/mtd/spi-nor.h>
> +#include <linux/nvmem-provider.h>
>  
>  #include "core.h"
>  
> @@ -13,6 +14,8 @@
>  
>  #define SST26VF_CR_BPNV		BIT(3)
>  
> +#define SST26VF_SFDP_EUI48	0x30
> +
>  static int sst26vf_nor_lock(struct spi_nor *nor, loff_t ofs, u64 len)
>  {
>  	return -EOPNOTSUPP;
> @@ -56,8 +59,67 @@ static int sst26vf_nor_late_init(struct spi_nor *nor)
>  	return 0;
>  }
>  
> +/**
> + * sst26vf_sfdp_mac_addr_read() - check if the EUI-48 MAC Address is programmed
> + * and read the data from the prestored SFDP data
> + *
> + * @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 sst26vf_sfdp_mac_addr_read(void *priv, unsigned int off,
> +				      void *val, size_t bytes)
> +{
> +	struct spi_nor *nor = priv;
> +	struct sfdp *sfdp = nor->sfdp;
> +	loff_t offset = off;
> +	size_t sfdp_size;
> +
> +	/*
> +	 * Check if the EUI-48 MAC address is programmed in the next six address
> +	 * locations.
> +	 * @off is programmed in the DT and stores the start of MAC Address
> +	 * byte, (off - 1) stores the bit length of the Extended Unique
> +	 * Identifier
> +	 */
> +	if (SST26VF_SFDP_EUI48 != *((u8 *)sfdp->dwords + (offset - 1)))
> +		return -EINVAL;

What happens if you read at a different offset? You're exposing
the entire SFDP region. What happens if there is a 0x30 at a
different location?

> +
> +	sfdp_size = sfdp->num_dwords * sizeof(*sfdp->dwords);
> +	memory_read_from_buffer(val, bytes, &offset, sfdp->dwords,
> +				sfdp_size);
> +	return 0;
> +}
> +
> +static struct nvmem_config sst26vf_sfdp_nvmem_config = {
> +	.word_size = 1,
> +	.stride = 1,
> +};
> +
> +static int sst26vf_nor_post_sfdp(struct spi_nor *nor)
> +{
> +	struct nvmem_device *nvmem;
> +
> +	sst26vf_sfdp_nvmem_config.dev = nor->dev;
> +	sst26vf_sfdp_nvmem_config.size = nor->sfdp->num_dwords * sizeof(*nor->sfdp->dwords);
> +	sst26vf_sfdp_nvmem_config.priv = nor;
> +	sst26vf_sfdp_nvmem_config.reg_read = sst26vf_sfdp_mac_addr_read;
> +
> +	nvmem = devm_nvmem_register(nor->dev, &sst26vf_sfdp_nvmem_config);
> +	if (IS_ERR(nvmem)) {
> +		dev_err(nor->dev, "failed to register NVMEM device: %ld\n", PTR_ERR(nvmem));
> +		return PTR_ERR(nvmem);

I don't think it makes sense to have this one-off in a particular
driver. If at all, this should be handled in the core. Sorry, but
this really looks like an ugly hack.

-michael

> +	}
> +
> +	return 0;
> +}
> +
>  static const struct spi_nor_fixups sst26vf_nor_fixups = {
>  	.late_init = sst26vf_nor_late_init,
> +	.post_sfdp = sst26vf_nor_post_sfdp,
>  };
>  
>  static const struct flash_info sst_nor_parts[] = {