Re: [PATCH v2 3/3] ARM: dts: microchip: sama5d29_curiosity: Add nvmem-layout in QSPI for EUI48 MAC Address
From: Andrew Lunn
Date: Thu Mar 27 2025 - 09:16:51 EST
On Thu, Mar 27, 2025 at 06:03:05AM +0000, Manikandan.M@xxxxxxxxxxxxx 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@xxxxxxxxxxxxx>
> >> ---
> >> .../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