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

[Manikandan.M]

Hi Micheal,

On 05/03/25 3:54 pm, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Wed Mar 5, 2025 at 11:01 AM CET, Manikandan Muralidharan wrote:
>> From: Varshini Rajendran <varshini.rajendran@xxxxxxxxxxxxx>
>>
>> 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@xxxxxxxxxxxxx>
>> [manikandan.m@xxxxxxxxxxxxx: Integrate the nvmem->read callback framework]
>> Signed-off-by: Manikandan Muralidharan <manikandan.m@xxxxxxxxxxxxx>
>> ---
>>   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?
> 
Since the offset is passed from the nvmem-layout in DT I thought that 
could help and I now realize that the SFDP Table of contents for other 
part numbers starting with sst26vf064b ends at 0x25F, going forward I 
will add a check to validate the number of DWORDS in the parameter table 
to differentiate and proceed further for SST26VF064BEUI flash.
>> +
>> +     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.
> 

Because the EUI identifier within the SFDP is unique to the 
SST26VF064BEUI flash, I opted to handle it here rather than in the core.

Also here the MAC address data resides within the 0x260-0x26F range, I 
will resize the nvmem_config.size to 0x10 instead of registering the 
full SFDP region as NVMEM.

> -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[] = {
> 

-- 
Thanks and Regards,
Manikandan M.