Message-ID: <c33d5c3d-61b9-41c9-b553-dec8087a60a4@microchip.com>
Date: Fri, 28 Mar 2025 10:54:02 +0000
From: <Manikandan.M@...rochip.com>
To: <andrew@...n.ch>
CC: <robh@...nel.org>, <krzk+dt@...nel.org>, <conor+dt@...nel.org>,
	<Nicolas.Ferre@...rochip.com>, <alexandre.belloni@...tlin.com>,
	<claudiu.beznea@...on.dev>, <tudor.ambarus@...aro.org>,
	<pratyush@...nel.org>, <mwalle@...nel.org>, <miquel.raynal@...tlin.com>,
	<richard@....at>, <vigneshr@...com>, <devicetree@...r.kernel.org>,
	<linux-arm-kernel@...ts.infradead.org>, <linux-kernel@...r.kernel.org>,
	<linux-mtd@...ts.infradead.org>
Subject: Re: [PATCH v2 3/3] ARM: dts: microchip: sama5d29_curiosity: Add
 nvmem-layout in QSPI for EUI48 MAC Address

Hi Andrew,

On 27/03/25 6:44 pm, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Thu, Mar 27, 2025 at 06:03:05AM +0000, Manikandan.M@...rochip.com wrote:
>> Hi Andrew Lunn,
>>
>> On 26/03/25 6:48 pm, Andrew Lunn wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> On Wed, Mar 26, 2025 at 12:51:40PM +0530, Manikandan Muralidharan wrote:
>>>> Add nvmem-layout in QSPI to read the EUI48 Mac address by the
>>>> net drivers using the nvmem property.The offset is set to 0x0
>>>> since the factory programmed address is available in the
>>>> resource managed space and the size determine if the requested
>>>> address is of EUI48 (0x6) or EUI-64 (0x8) type.
>>>> This is useful for cases where U-Boot is skipped and the Ethernet
>>>> MAC address is needed to be configured by the kernel
>>>>
>>>> Signed-off-by: Manikandan Muralidharan <manikandan.m@...rochip.com>
>>>> ---
>>>>    .../arm/boot/dts/microchip/at91-sama5d29_curiosity.dts | 10 ++++++++++
>>>>    1 file changed, 10 insertions(+)
>>>>
>>>> diff --git a/arch/arm/boot/dts/microchip/at91-sama5d29_curiosity.dts b/arch/arm/boot/dts/microchip/at91-sama5d29_curiosity.dts
>>>> index 35756cc01e68..6c5ff08f0b3f 100644
>>>> --- a/arch/arm/boot/dts/microchip/at91-sama5d29_curiosity.dts
>>>> +++ b/arch/arm/boot/dts/microchip/at91-sama5d29_curiosity.dts
>>>> @@ -478,6 +478,16 @@ flash@0 {
>>>>                 label = "atmel_qspi1";
>>>>                 status = "okay";
>>>>
>>>> +             nvmem-layout {
>>>> +                     compatible = "fixed-layout";
>>>> +                     #address-cells = <1>;
>>>> +                     #size-cells = <1>;
>>>> +
>>>> +                     mac_address_eui48: mac-address@0 {
>>>> +                             reg = <0x0 0x6>;
>>>> +                     };
>>>> +             };
>>>> +
>>>
>>> I've not looked too deeply how this all works. Don't you need a
>>> reference in the ethernet node pointing to this?
>> Yes we need a reference to 'mac_address_eui48' using nvmem-cells in the
>> Ethernet node, since the sama5d29_curiosity uses a daughter card for PHY
>> [1], the DTS properties are defined in overlay files. Here is the quick
>> usage of the nvmem ref in the ethernet node:
>>        macb0 {
>>                nvmem-cells = <&mac_address_eui48>;
>>                nvmem-cell-names = "mac-address";
>>
>>                phy {
>>
>>                };
>>        };
> 
> So why are you not adding this as part of this patch?
As mentioned, we maintain the DT nodes and properties of all daughter 
cards and modules which are not part of the on-board peripherals in a 
separate repo's as overlay [1].In the next version I will also include 
the DT changes of the board which actually has an on-board PHY with 
mac-address programmed in QSPI SFDP vendor area to convey the changes 
better- SAMA5D27 WLSOM1

[1] --> 
https://github.com/linux4microchip/dt-overlay-mchp/blob/master/sama5d29_curiosity/sama5d29_curiosity_ksz8091.dtso

> 
>>> And are there ordering issues? Boards used to use the MAC address from
>>> somewhere else now start using this address, causing a change in
>>> behaviour. I would expect somewhere a comment that this MAC address
>>> will be used last, after all other options have been tried, in order
>>> to avoid regressions.
>>>
>> The order of search is documented in of_get_mac_address() in
>> net/core/of_net.c file
>>
>> The driver attempts to retrieve the MAC address through a hierarchical
>> approach: first checking device tree properties, then exploring NVMEM
>> cells, followed by the U-Boot 'ethaddr' environment variable. If no
>> valid MAC address is found through these methods, the driver will
>> generate a random but valid MAC address as a final fallback mechanism.
> 
> This is not quite correct. macb first uses
> of_get_ethdev_address()->of_get_mac_address() which looks for DT
> properties:
> 
>          ret = of_get_mac_addr(np, "mac-address", addr);
>          if (!ret)
>                  return 0;
> 
>          ret = of_get_mac_addr(np, "local-mac-address", addr);
>          if (!ret)
>                  return 0;
> 
>          ret = of_get_mac_addr(np, "address", addr);
>          if (!ret)
>                  return 0;
> 
> And then it looks in nvram.
> 
>          return of_get_mac_address_nvmem(np, addr);
> 
> If they all fail, it uses macb_get_hwaddr() which looks in 4 different
> locations within the macb register set.
> 
> Then lastly it uses a random MAC address.
> 
> So with your proposed change, anybody using the curiosity board and
> this last mechanism to set the MAC address sees a change in behaviour,
> it will start using nvram instead. You should at least document this,
> and if possible, argue that nobody is using this last mechanism
> because ....

Retrieving MAC Address from NVRAM is practiced in sama7g5ek but with 
EEPROM memory
For the next version, I will add a note in the commit message explaining 
the hierarchical order and the possibilities of hitting 
of_get_mac_address_nvmem()

> 
>          Andrew

-- 
Thanks and Regards,
Manikandan M.

Powered by blists - more mailing lists