Message-ID: <CA+V-a8t5RKY9vyFDg0V3AWcBovBdWbcvqdPeiPYmHRA8v2=UGQ@mail.gmail.com>
Date: Tue, 27 Jan 2026 13:39:29 +0000
From: "Lad, Prabhakar" <prabhakar.csengg@...il.com>
To: "Russell King (Oracle)" <linux@...linux.org.uk>
Cc: Andrew Lunn <andrew+netdev@...n.ch>, "David S. Miller" <davem@...emloft.net>, 
	Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, 
	Maxime Coquelin <mcoquelin.stm32@...il.com>, Alexandre Torgue <alexandre.torgue@...s.st.com>, 
	Philipp Zabel <p.zabel@...gutronix.de>, Geert Uytterhoeven <geert+renesas@...der.be>, netdev@...r.kernel.org, 
	linux-stm32@...md-mailman.stormreply.com, 
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org, 
	linux-renesas-soc@...r.kernel.org, Biju Das <biju.das.jz@...renesas.com>, 
	Fabrizio Castro <fabrizio.castro.jz@...esas.com>, 
	Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
Subject: Re: [RFC PATCH net-next] net: stmmac: Preserve bootloader MAC address
 across unconditional reset

Hi Russell,

On Mon, Jan 26, 2026 at 6:11 PM Russell King (Oracle)
<linux@...linux.org.uk> wrote:
>
> On Mon, Jan 26, 2026 at 05:25:03PM +0000, Prabhakar wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
> >
> > Commit 90f522a20e3d1 ("NET: dwmac: Make dwmac reset unconditional")
> > asserts a reset in probe when a reset controller is present. This reset
> > clears the MAC address registers, so a valid address programmed by the
> > bootloader gets lost and the driver falls back to a random address.
> >
> > Read the MAC address from the hardware registers before resetting the
> > hardware. Keep the existing address selection logic when no valid
> > address is found, and program the selected address back into the MAC
> > after probe so it remains consistent in hardware.
> >
> > Export stmmac_bus_clks_config() so the early read path can enable the
> > bus clocks before accessing the MAC registers.
>
> I don't think this is a good idea. stmmac_bus_clks_config() is specific
> to using platform devices, but the core stmmac driver also supports
> PCI that doesn't use stmmac_bus_clks_config().
>
> stmmac_bus_clks_config() handles:
>
>  - plat_dat->stmmac_clk
>  - plat_dat->pclk
>  - any clock handled by the plat_dat->clks_config() method
>
> For platform devices, stmmac_probe_config_dt() gets these two clocks
> from DT, and prepares and enables them both. So, by the time the
> probe function is called, these clocks are already running.
>
> For those handled by the platform glue, the glues that populate
> this function:
>
> eic7700: eic7700_clks_config() - this is called from the init/exit
> handlers. Will be invoked to enable the clocks by stmmac_dvr_probe().
>
> imx: imx_dwmac_clks_config() - called by imx_dwmac_probe() to enable
> clocks prior to stmmac_dvr_probe() being invoked.
>
> mediaktek: mediatek_dwmac_clks_config() - called by
> mediatek_dwmac_probe() to enable clocks prior to stmmac_dvr_probe()
> being invoked.
>
> qcom-ethqos: ethqos_clks_config() - called by qcom_ethqos_probe() to
> enable clocks prior to stmmac_dvr_probe() being invoked.
>
> So, I can confidently say that all clocks should be running by the
> time __stmmac_dvr_probe() is called, and thus there should be no
> requirement to call stmmac_bus_clks_config() in this code.
>
Agreed.

>
> The next problem: you place this code to read registers from stmmac
> before:
>
>         ret = reset_control_deassert(priv->plat->stmmac_ahb_rst);
>
> Sadly, the binding documentation is too vague to pin down what this
> is, as dwmac can have AHB master (which generates bus cycles for
> accessing memory) and AHB slave (which would be the target for
> register accesses) interfaces.
>
> The problem here is that if some platform glue has wired this reset
> such that it resets the AHB slave side, that will prevent register
> access, and thus your attempt to read the MAC across all devices
> will fail.
>
Ok, got you.

>
> The next question that comes up is that we have a perfectly good way
> that's been around for years to pass a MAC address from the boot
> loader into the kernel for any network interface. I notice that it
> isn't mentioned in the DT bindings, presumably to prevent people
> from adding it to their in-kernel DT files.
>
>         mac-address =
>         local-mac-address =
>
> The old documentation in ethernet.txt was:
>
> - mac-address: array of 6 bytes, specifies the MAC address that was last used by
>   the boot program; should be used in cases where the MAC address assigned to
>   the device by the boot program is different from the "local-mac-address"
>   property;
> - local-mac-address: array of 6 bytes, specifies the MAC address that was
>   assigned to the network device;
>
> Given that these are interfaces between the boot loader and the kernel,
> they can't be deprecated, as platforms will rely upon these properties
> to pass the MAC address from the boot loader to the kernel. For example
> on one of my systems:
>
> $ vdir /sys/class/net/eth0/of_node/
> total 0
> -r--r--r-- 1 root root  4 Jan 26 18:08 gop-port-id
> -r--r--r-- 1 root root 50 Jan 26 18:08 interrupt-names
> -r--r--r-- 1 root root 80 Jan 26 18:08 interrupts
> -r--r--r-- 1 root root  6 Jan 26 18:08 local-mac-address
> -r--r--r-- 1 root root 14 Jan 26 18:08 name
> -r--r--r-- 1 root root  4 Jan 26 18:08 phy
> -r--r--r-- 1 root root 10 Jan 26 18:08 phy-mode
> -r--r--r-- 1 root root  8 Jan 26 18:08 phys
> -r--r--r-- 1 root root  4 Jan 26 18:08 port-id
> -r--r--r-- 1 root root  4 Jan 26 18:08 reg
> -r--r--r-- 1 root root  5 Jan 26 18:08 status
>
> where "local-mac-address" states the MAC address to be used for eth0,
> as specified by the boot loader.
>
Thanks for the pointer, I will use the above-mentioned method with
which we won't be needing this patch on the RZ/T2H platform.

> I don't think stmmac needs this extra complication provided platforms
> make use of mechanisms that already exist... and I feel it's time to
> start saying no to platform specific quirks that can be handled by
> those mechanisms.
>
Ok.

Cheers,
Prabhakar

Powered by blists - more mailing lists