Re: [PATCH v2 3/3] ARM: dts: microchip: sama5d29_curiosity: Add nvmem-layout in QSPI for EUI48 MAC Address

[Andrew Lunn <andrew@lunn.ch>]

On Thu, Mar 27, 2025 at 06:03:05AM +0000, Manikandan.M@microchip.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@microchip.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?

> > 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 ....

	Andrew