Message-ID: <aXeuR_YLoAFYEAVi@shell.armlinux.org.uk>
Date: Mon, 26 Jan 2026 18:11:19 +0000
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Prabhakar <prabhakar.csengg@...il.com>
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

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.


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.


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.

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.

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Powered by blists - more mailing lists