[PATCH] arch: arm: kirkwood: support nvmem mac address

[PATCH] arch: arm: kirkwood: support nvmem mac address

[Rosen Penev]

of_get_ethdev_address gets called too early for nvmem. If EPROBE_DEFER
gets called, skip so that the ethernet driver can adjust the MAC address
through nvmem.

Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 arch/arm/mach-mvebu/kirkwood.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/arch/arm/mach-mvebu/kirkwood.c b/arch/arm/mach-mvebu/kirkwood.c
index 73b2a86d6489..da347f66900b 100644
--- a/arch/arm/mach-mvebu/kirkwood.c
+++ b/arch/arm/mach-mvebu/kirkwood.c
@@ -86,13 +86,18 @@ static void __init kirkwood_dt_eth_fixup(void)
 		void __iomem *io;
 		u8 *macaddr;
 		u32 reg;
+		int err;
 
 		if (!pnp)
 			continue;
 
-		/* skip disabled nodes or nodes with valid MAC address*/
-		if (!of_device_is_available(pnp) ||
-		    !of_get_mac_address(np, tmpmac))
+		/* skip disabled nodes */
+		if (!of_device_is_available(pnp))
+			goto eth_fixup_skip;
+
+		/* skip nodes with valid MAC address*/
+		err = of_get_mac_address(np, tmpmac);
+		if (err == -EPROBE_DEFER || !err)
 			goto eth_fixup_skip;
 
 		clk = of_clk_get(pnp, 0);
-- 
2.46.2

Re: [PATCH] arch: arm: kirkwood: support nvmem mac address

[Andrew Lunn]

On Mon, Sep 30, 2024 at 02:59:34PM -0700, Rosen Penev wrote:
> of_get_ethdev_address gets called too early for nvmem. If EPROBE_DEFER
> gets called, skip so that the ethernet driver can adjust the MAC address
> through nvmem.

Is this from code analysis or do you have a board with real issues? Do
we want to add a Fixed: so it gets back ported in stable?

> 
> Signed-off-by: Rosen Penev <rosenp@gmail.com>
> ---
>  arch/arm/mach-mvebu/kirkwood.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm/mach-mvebu/kirkwood.c b/arch/arm/mach-mvebu/kirkwood.c
> index 73b2a86d6489..da347f66900b 100644
> --- a/arch/arm/mach-mvebu/kirkwood.c
> +++ b/arch/arm/mach-mvebu/kirkwood.c
> @@ -86,13 +86,18 @@ static void __init kirkwood_dt_eth_fixup(void)
>  		void __iomem *io;
>  		u8 *macaddr;
>  		u32 reg;
> +		int err;
>  
>  		if (!pnp)
>  			continue;
>  
> -		/* skip disabled nodes or nodes with valid MAC address*/
> -		if (!of_device_is_available(pnp) ||
> -		    !of_get_mac_address(np, tmpmac))
> +		/* skip disabled nodes */
> +		if (!of_device_is_available(pnp))
> +			goto eth_fixup_skip;
> +
> +		/* skip nodes with valid MAC address*/
> +		err = of_get_mac_address(np, tmpmac);
> +		if (err == -EPROBE_DEFER || !err)
>  			goto eth_fixup_skip;

I'm wondering about ordering here. What exactly does EPROBE_DEFER
mean? Does it mean we know there is a MAC address in nvmem, but the
nvmem has not probed yet? Or can it mean, the nvmem has not probed
yet, and maybe there is a MAC address in it, maybe not?

In the maybe not case, we should still be trying to read the MAC from
the hardware and storing it way safe for later use.

	Andrew

Re: [PATCH] arch: arm: kirkwood: support nvmem mac address

[Rosen Penev]

On Mon, Sep 30, 2024 at 6:53 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Mon, Sep 30, 2024 at 02:59:34PM -0700, Rosen Penev wrote:
> > of_get_ethdev_address gets called too early for nvmem. If EPROBE_DEFER
> > gets called, skip so that the ethernet driver can adjust the MAC address
> > through nvmem.
>
> Is this from code analysis or do you have a board with real issues? Do
> we want to add a Fixed: so it gets back ported in stable?
Working with a guy that does. This device has not been upstreamed, so
not quite valid.
>
> >
> > Signed-off-by: Rosen Penev <rosenp@gmail.com>
> > ---
> >  arch/arm/mach-mvebu/kirkwood.c | 11 ++++++++---
> >  1 file changed, 8 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/arm/mach-mvebu/kirkwood.c b/arch/arm/mach-mvebu/kirkwood.c
> > index 73b2a86d6489..da347f66900b 100644
> > --- a/arch/arm/mach-mvebu/kirkwood.c
> > +++ b/arch/arm/mach-mvebu/kirkwood.c
> > @@ -86,13 +86,18 @@ static void __init kirkwood_dt_eth_fixup(void)
> >               void __iomem *io;
> >               u8 *macaddr;
> >               u32 reg;
> > +             int err;
> >
> >               if (!pnp)
> >                       continue;
> >
> > -             /* skip disabled nodes or nodes with valid MAC address*/
> > -             if (!of_device_is_available(pnp) ||
> > -                 !of_get_mac_address(np, tmpmac))
> > +             /* skip disabled nodes */
> > +             if (!of_device_is_available(pnp))
> > +                     goto eth_fixup_skip;
> > +
> > +             /* skip nodes with valid MAC address*/
> > +             err = of_get_mac_address(np, tmpmac);
> > +             if (err == -EPROBE_DEFER || !err)
> >                       goto eth_fixup_skip;
>
> I'm wondering about ordering here. What exactly does EPROBE_DEFER
> mean? Does it mean we know there is a MAC address in nvmem, but the
> nvmem has not probed yet? Or can it mean, the nvmem has not probed
> yet, and maybe there is a MAC address in it, maybe not?
It only means NVMEM has not loaded. NVMEM loads after MTD, meaning quite late.
>
> In the maybe not case, we should still be trying to read the MAC from
> the hardware and storing it way safe for later use.
Not sure how to go about that. OTOH it's not very common to have
CONFIG_NVMEM where not needed.

Actually I have no idea what this whole function even does. For these
devices, uboot usually reads the ethaddr variable and passes it to the
kernel. Something like that can be handled entirely by nvmem.
>
>         Andrew

Re: [PATCH] arch: arm: kirkwood: support nvmem mac address

[Andrew Lunn]

On Mon, Sep 30, 2024 at 07:50:56PM -0700, Rosen Penev wrote:
> On Mon, Sep 30, 2024 at 6:53 PM Andrew Lunn <andrew@lunn.ch> wrote:
> >
> > On Mon, Sep 30, 2024 at 02:59:34PM -0700, Rosen Penev wrote:
> > > of_get_ethdev_address gets called too early for nvmem. If EPROBE_DEFER
> > > gets called, skip so that the ethernet driver can adjust the MAC address
> > > through nvmem.
> >
> > Is this from code analysis or do you have a board with real issues? Do
> > we want to add a Fixed: so it gets back ported in stable?

> Working with a guy that does. This device has not been upstreamed, so
> not quite valid.

As the MVEBU Maitnainer, i'm always happy to see new kirkwood
boards. It has been a number of years since the last one was
contributed.

> > > Signed-off-by: Rosen Penev <rosenp@gmail.com>
> > > ---
> > >  arch/arm/mach-mvebu/kirkwood.c | 11 ++++++++---
> > >  1 file changed, 8 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/arch/arm/mach-mvebu/kirkwood.c b/arch/arm/mach-mvebu/kirkwood.c
> > > index 73b2a86d6489..da347f66900b 100644
> > > --- a/arch/arm/mach-mvebu/kirkwood.c
> > > +++ b/arch/arm/mach-mvebu/kirkwood.c
> > > @@ -86,13 +86,18 @@ static void __init kirkwood_dt_eth_fixup(void)
> > >               void __iomem *io;
> > >               u8 *macaddr;
> > >               u32 reg;
> > > +             int err;
> > >
> > >               if (!pnp)
> > >                       continue;
> > >
> > > -             /* skip disabled nodes or nodes with valid MAC address*/
> > > -             if (!of_device_is_available(pnp) ||
> > > -                 !of_get_mac_address(np, tmpmac))
> > > +             /* skip disabled nodes */
> > > +             if (!of_device_is_available(pnp))
> > > +                     goto eth_fixup_skip;
> > > +
> > > +             /* skip nodes with valid MAC address*/
> > > +             err = of_get_mac_address(np, tmpmac);
> > > +             if (err == -EPROBE_DEFER || !err)
> > >                       goto eth_fixup_skip;
> >
> > I'm wondering about ordering here. What exactly does EPROBE_DEFER
> > mean? Does it mean we know there is a MAC address in nvmem, but the
> > nvmem has not probed yet? Or can it mean, the nvmem has not probed
> > yet, and maybe there is a MAC address in it, maybe not?

> It only means NVMEM has not loaded. NVMEM loads after MTD, meaning quite late.
> >
> > In the maybe not case, we should still be trying to read the MAC from
> > the hardware and storing it way safe for later use.
> Not sure how to go about that. OTOH it's not very common to have
> CONFIG_NVMEM where not needed.

Yes, it is. Think about openwrt! It will have one kernel configuration
for all kirkwood boards. If one device needs NVMEM, they all get
NVMEM.

> 
> Actually I have no idea what this whole function even does. For these
> devices, uboot usually reads the ethaddr variable and passes it to the
> kernel. Something like that can be handled entirely by nvmem.

Well, no not really. What you say is true for 'modern' systems,
anything which is less than 10 years old. But kirkwood is very old. It
was one of the SoCs which pushed the adaptation of DT. I did a lot of
work adding DT bindings to all the Marvell drivers, so that kirkwood
and orion5x could make use of DT, rather than board files. But back
then u-boot had no support for DT. u-boot had no idea about DT
blobs. The blob was just appended to the end of the kernel
bZimage. u-boot just loaded the 'kernel' into memory, and the kernel
knew to look for it at the end of the image.

As you say, u-boot knows the MAC addresses. But Marvell decided on a
unique way to pass them to Linux. The Marvell u-boot programmed the
MAC addresses into the Ethernet controllers. So when Linux booted, the
MAC addresses are already set.

But part of DT was clock support, using the common clock
framework. Towards the end of the boot, CCF turns off all clocks which
have not been claimed. If the Ethernet driver has not loaded yet, the
clock is not claimed, and so it got turned off. And when the clock is
turned off, the ethernet controller forgets its MAC address :-(

We hacked around this. During early boot, this bit of code looked in
the Ethernet controller and reads out the MAC address, and modifies
the DT blob, adding the MAC addresses in the usual place. When the MAC
driver probes, it then uses the normal API to get the MAC address.

We need to ensure this change does not break this. If we 100% know
there is a MAC address in nvmem, we can skip reading the MAC out of
the hardware. But if there is an nvmem, but not MAC addresses in it,
we need to read the MAC addresses out from the controllers. Hence the
exact meaning of EPRODE_DEFER is important.

	Andrew