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.
