[PATCH v2] drivers: net: fsl_enetc: Pass on primary MAC address to Linux

[PATCH v2] drivers: net: fsl_enetc: Pass on primary MAC address to Linux

[Alex Marginean]

Passes on the primary address used by u-boot to Linux.  The code does a DT
fix-up for ENETC PFs and sets the primary MAC address in IERB.  The address
in IERB is restored on ENETC PCI functions at FLR.

Signed-off-by: Alex Marginean <alexandru.marginean@nxp.com>
---

The code is called fom ft_board_setup in board/freescale/ls1028a/ls1028a.c
mostly for consistency with other LS parts.  I'm open to suggestions though.

Changes in v2:
 - fixed warning for fdt_fixup_enetc_mac being implicitly declared

 board/freescale/ls1028a/ls1028a.c |  5 +++
 drivers/net/fsl_enetc.c           | 65 ++++++++++++++++++++++++++++++-
 drivers/net/fsl_enetc.h           |  3 ++
 3 files changed, 72 insertions(+), 1 deletion(-)

diff --git a/board/freescale/ls1028a/ls1028a.c b/board/freescale/ls1028a/ls1028a.c
index a9606b8865..1a82c95402 100644
--- a/board/freescale/ls1028a/ls1028a.c
+++ b/board/freescale/ls1028a/ls1028a.c
@@ -25,6 +25,7 @@
 #include <fdtdec.h>
 #include <miiphy.h>
 #include "../common/qixis.h"
+#include "../drivers/net/fsl_enetc.h"
 
 DECLARE_GLOBAL_DATA_PTR;
 
@@ -150,6 +151,10 @@ int ft_board_setup(void *blob, bd_t *bd)
 
 	fdt_fixup_icid(blob);
 
+#ifdef CONFIG_FSL_ENETC
+	fdt_fixup_enetc_mac(blob);
+#endif
+
 	return 0;
 }
 #endif
diff --git a/drivers/net/fsl_enetc.c b/drivers/net/fsl_enetc.c
index 0ca7e838a8..3c043888db 100644
--- a/drivers/net/fsl_enetc.c
+++ b/drivers/net/fsl_enetc.c
@@ -14,6 +14,69 @@
 
 #include "fsl_enetc.h"
 
+#define ENETC_DRIVER_NAME	"enetc_eth"
+
+/*
+ * sets the MAC address in IERB registers, this setting is persistent and
+ * carried over to Linux.
+ */
+static void enetc_set_ierb_primary_mac(struct udevice *dev, int devfn,
+				       const u8 *enetaddr)
+{
+#ifdef CONFIG_ARCH_LS1028A
+/*
+ * LS1028A is the only part with IERB at this time and there are plans to change
+ * its structure, keep this LS1028A specific for now
+ */
+#define IERB_BASE		0x1f0800000ULL
+#define IERB_PFMAC(pf, vf, n)	(IERB_BASE + 0x8000 + (pf) * 0x100 + (vf) * 8 \
+				 + (n) * 4)
+
+static int ierb_fn_to_pf[] = {0, 1, 2, -1, -1, -1, 3};
+
+	u16 lower = *(const u16 *)(enetaddr + 4);
+	u32 upper = *(const u32 *)enetaddr;
+
+	if (ierb_fn_to_pf[devfn] < 0)
+		return;
+
+	out_le32(IERB_PFMAC(ierb_fn_to_pf[devfn], 0, 0), upper);
+	out_le32(IERB_PFMAC(ierb_fn_to_pf[devfn], 0, 1), (u32)lower);
+#endif
+}
+
+/* sets up primary MAC addresses in DT/IERB */
+void fdt_fixup_enetc_mac(void *blob)
+{
+	struct pci_child_platdata *ppdata;
+	struct eth_pdata *pdata;
+	struct udevice *dev;
+	struct uclass *uc;
+	char path[256];
+	int offset;
+	int devfn;
+
+	uclass_get(UCLASS_ETH, &uc);
+	uclass_foreach_dev(dev, uc) {
+		if (!dev->driver || !dev->driver->name ||
+		    strcmp(dev->driver->name, ENETC_DRIVER_NAME))
+			continue;
+
+		pdata = dev_get_platdata(dev);
+		ppdata = dev_get_parent_platdata(dev);
+		devfn = PCI_FUNC(ppdata->devfn);
+
+		enetc_set_ierb_primary_mac(dev, devfn, pdata->enetaddr);
+
+		snprintf(path, 256, "/soc/pcie at 1f0000000/ethernet@%x,%x",
+			 PCI_DEV(ppdata->devfn), PCI_FUNC(ppdata->devfn));
+		offset = fdt_path_offset(blob, path);
+		if (offset < 0)
+			continue;
+		fdt_setprop(blob, offset, "mac-address", pdata->enetaddr, 6);
+	}
+}
+
 /*
  * Bind the device:
  * - set a more explicit name on the interface
@@ -583,7 +646,7 @@ static const struct eth_ops enetc_ops = {
 };
 
 U_BOOT_DRIVER(eth_enetc) = {
-	.name	= "enetc_eth",
+	.name	= ENETC_DRIVER_NAME,
 	.id	= UCLASS_ETH,
 	.bind	= enetc_bind,
 	.probe	= enetc_probe,
diff --git a/drivers/net/fsl_enetc.h b/drivers/net/fsl_enetc.h
index 0bb4cdff47..e441891468 100644
--- a/drivers/net/fsl_enetc.h
+++ b/drivers/net/fsl_enetc.h
@@ -226,4 +226,7 @@ int enetc_mdio_read_priv(struct enetc_mdio_priv *priv, int addr, int devad,
 int enetc_mdio_write_priv(struct enetc_mdio_priv *priv, int addr, int devad,
 			  int reg, u16 val);
 
+/* sets up primary MAC addresses in DT/IERB */
+void fdt_fixup_enetc_mac(void *blob);
+
 #endif /* _ENETC_H */
-- 
2.17.1

[PATCH v2] drivers: net: fsl_enetc: Pass on primary MAC address to Linux

[Michael Walle]

Am 2019-12-10 15:55, schrieb Alex Marginean:
> Passes on the primary address used by u-boot to Linux.  The code does a 
> DT
> fix-up for ENETC PFs and sets the primary MAC address in IERB.  The 
> address
> in IERB is restored on ENETC PCI functions at FLR.


I don't get the reason why this is done in a proprietary way. What is 
the
difference between any other network interface whose hardware address is
set up in the generic u-boot code.

Also, shouldn't write_hwaddr callback be implemented instead of the
enetc_set_ierb_primary_mac()?

-michael

> Signed-off-by: Alex Marginean <alexandru.marginean@nxp.com>
> ---
> 
> The code is called fom ft_board_setup in 
> board/freescale/ls1028a/ls1028a.c
> mostly for consistency with other LS parts.  I'm open to suggestions 
> though.
> 
> Changes in v2:
>  - fixed warning for fdt_fixup_enetc_mac being implicitly declared
> 
>  board/freescale/ls1028a/ls1028a.c |  5 +++
>  drivers/net/fsl_enetc.c           | 65 ++++++++++++++++++++++++++++++-
>  drivers/net/fsl_enetc.h           |  3 ++
>  3 files changed, 72 insertions(+), 1 deletion(-)
> 
> diff --git a/board/freescale/ls1028a/ls1028a.c
> b/board/freescale/ls1028a/ls1028a.c
> index a9606b8865..1a82c95402 100644
> --- a/board/freescale/ls1028a/ls1028a.c
> +++ b/board/freescale/ls1028a/ls1028a.c
> @@ -25,6 +25,7 @@
>  #include <fdtdec.h>
>  #include <miiphy.h>
>  #include "../common/qixis.h"
> +#include "../drivers/net/fsl_enetc.h"
> 
>  DECLARE_GLOBAL_DATA_PTR;
> 
> @@ -150,6 +151,10 @@ int ft_board_setup(void *blob, bd_t *bd)
> 
>  	fdt_fixup_icid(blob);
> 
> +#ifdef CONFIG_FSL_ENETC
> +	fdt_fixup_enetc_mac(blob);
> +#endif
> +
>  	return 0;
>  }
>  #endif
> diff --git a/drivers/net/fsl_enetc.c b/drivers/net/fsl_enetc.c
> index 0ca7e838a8..3c043888db 100644
> --- a/drivers/net/fsl_enetc.c
> +++ b/drivers/net/fsl_enetc.c
> @@ -14,6 +14,69 @@
> 
>  #include "fsl_enetc.h"
> 
> +#define ENETC_DRIVER_NAME	"enetc_eth"
> +
> +/*
> + * sets the MAC address in IERB registers, this setting is persistent 
> and
> + * carried over to Linux.
> + */
> +static void enetc_set_ierb_primary_mac(struct udevice *dev, int devfn,
> +				       const u8 *enetaddr)
> +{
> +#ifdef CONFIG_ARCH_LS1028A
> +/*
> + * LS1028A is the only part with IERB at this time and there are
> plans to change
> + * its structure, keep this LS1028A specific for now
> + */
> +#define IERB_BASE		0x1f0800000ULL
> +#define IERB_PFMAC(pf, vf, n)	(IERB_BASE + 0x8000 + (pf) * 0x100 + 
> (vf) * 8 \
> +				 + (n) * 4)
> +
> +static int ierb_fn_to_pf[] = {0, 1, 2, -1, -1, -1, 3};
wrong indendation

> +
> +	u16 lower = *(const u16 *)(enetaddr + 4);
> +	u32 upper = *(const u32 *)enetaddr;
> +
> +	if (ierb_fn_to_pf[devfn] < 0)
> +		return;
it would be easier to read if ierb_fn_to_pf[devfn] would be assigned to 
a local variable.

> +
> +	out_le32(IERB_PFMAC(ierb_fn_to_pf[devfn], 0, 0), upper);
> +	out_le32(IERB_PFMAC(ierb_fn_to_pf[devfn], 0, 1), (u32)lower);
> +#endif
> +}
> +
> +/* sets up primary MAC addresses in DT/IERB */
> +void fdt_fixup_enetc_mac(void *blob)
> +{
> +	struct pci_child_platdata *ppdata;
> +	struct eth_pdata *pdata;
> +	struct udevice *dev;
> +	struct uclass *uc;
> +	char path[256];
> +	int offset;
> +	int devfn;
> +
> +	uclass_get(UCLASS_ETH, &uc);
> +	uclass_foreach_dev(dev, uc) {
> +		if (!dev->driver || !dev->driver->name ||
> +		    strcmp(dev->driver->name, ENETC_DRIVER_NAME))
> +			continue;
> +
> +		pdata = dev_get_platdata(dev);
> +		ppdata = dev_get_parent_platdata(dev);
> +		devfn = PCI_FUNC(ppdata->devfn);
> +
> +		enetc_set_ierb_primary_mac(dev, devfn, pdata->enetaddr);
> +
> +		snprintf(path, 256, "/soc/pcie at 1f0000000/ethernet@%x,%x",
> +			 PCI_DEV(ppdata->devfn), PCI_FUNC(ppdata->devfn));
> +		offset = fdt_path_offset(blob, path);
> +		if (offset < 0)
> +			continue;
> +		fdt_setprop(blob, offset, "mac-address", pdata->enetaddr, 6);
> +	}
> +}
> +
>  /*
>   * Bind the device:
>   * - set a more explicit name on the interface
> @@ -583,7 +646,7 @@ static const struct eth_ops enetc_ops = {
>  };
> 
>  U_BOOT_DRIVER(eth_enetc) = {
> -	.name	= "enetc_eth",
> +	.name	= ENETC_DRIVER_NAME,
>  	.id	= UCLASS_ETH,
>  	.bind	= enetc_bind,
>  	.probe	= enetc_probe,
> diff --git a/drivers/net/fsl_enetc.h b/drivers/net/fsl_enetc.h
> index 0bb4cdff47..e441891468 100644
> --- a/drivers/net/fsl_enetc.h
> +++ b/drivers/net/fsl_enetc.h
> @@ -226,4 +226,7 @@ int enetc_mdio_read_priv(struct enetc_mdio_priv
> *priv, int addr, int devad,
>  int enetc_mdio_write_priv(struct enetc_mdio_priv *priv, int addr, int 
> devad,
>  			  int reg, u16 val);
> 
> +/* sets up primary MAC addresses in DT/IERB */
> +void fdt_fixup_enetc_mac(void *blob);
> +
>  #endif /* _ENETC_H */

[PATCH v2] drivers: net: fsl_enetc: Pass on primary MAC address to Linux

[Vladimir Oltean]

Hi Michael,

On Wed, 11 Dec 2019 at 00:48, Michael Walle <michael@walle.cc> wrote:
>
> Am 2019-12-10 15:55, schrieb Alex Marginean:
> > Passes on the primary address used by u-boot to Linux.  The code does a
> > DT
> > fix-up for ENETC PFs and sets the primary MAC address in IERB.  The
> > address
> > in IERB is restored on ENETC PCI functions at FLR.
>
>
> I don't get the reason why this is done in a proprietary way. What is
> the
> difference between any other network interface whose hardware address is
> set up in the generic u-boot code.
>
> Also, shouldn't write_hwaddr callback be implemented instead of the
> enetc_set_ierb_primary_mac()?
>

At the moment, the Linux driver ignored the device tree and only reads
the POR values of the SIPMAR registers. The reset value of those comes
from the IERB space, which U-Boot is configuring. So it would be good
if that behavior keeps working.

It would also be good if the Linux driver called of_get_mac_address,
so it needs the device tree binding for that. That's why both fixups
are performed, and why the generic function is not used.

As for the write_hwaddr callback instead of
enetc_set_primary_mac_addr, that is valid but I suppose it is outside
the scope of this patch. That function is related to the runtime MAC
address and not to the MAC passed to Linux.

> -michael
>
> > Signed-off-by: Alex Marginean <alexandru.marginean@nxp.com>
> > ---
> >
> > The code is called fom ft_board_setup in
> > board/freescale/ls1028a/ls1028a.c
> > mostly for consistency with other LS parts.  I'm open to suggestions
> > though.
> >
> > Changes in v2:
> >  - fixed warning for fdt_fixup_enetc_mac being implicitly declared
> >
> >  board/freescale/ls1028a/ls1028a.c |  5 +++
> >  drivers/net/fsl_enetc.c           | 65 ++++++++++++++++++++++++++++++-
> >  drivers/net/fsl_enetc.h           |  3 ++
> >  3 files changed, 72 insertions(+), 1 deletion(-)
> >
> > diff --git a/board/freescale/ls1028a/ls1028a.c
> > b/board/freescale/ls1028a/ls1028a.c
> > index a9606b8865..1a82c95402 100644
> > --- a/board/freescale/ls1028a/ls1028a.c
> > +++ b/board/freescale/ls1028a/ls1028a.c
> > @@ -25,6 +25,7 @@
> >  #include <fdtdec.h>
> >  #include <miiphy.h>
> >  #include "../common/qixis.h"
> > +#include "../drivers/net/fsl_enetc.h"
> >
> >  DECLARE_GLOBAL_DATA_PTR;
> >
> > @@ -150,6 +151,10 @@ int ft_board_setup(void *blob, bd_t *bd)
> >
> >       fdt_fixup_icid(blob);
> >
> > +#ifdef CONFIG_FSL_ENETC
> > +     fdt_fixup_enetc_mac(blob);
> > +#endif
> > +
> >       return 0;
> >  }
> >  #endif
> > diff --git a/drivers/net/fsl_enetc.c b/drivers/net/fsl_enetc.c
> > index 0ca7e838a8..3c043888db 100644
> > --- a/drivers/net/fsl_enetc.c
> > +++ b/drivers/net/fsl_enetc.c
> > @@ -14,6 +14,69 @@
> >
> >  #include "fsl_enetc.h"
> >
> > +#define ENETC_DRIVER_NAME    "enetc_eth"
> > +
> > +/*
> > + * sets the MAC address in IERB registers, this setting is persistent
> > and
> > + * carried over to Linux.
> > + */
> > +static void enetc_set_ierb_primary_mac(struct udevice *dev, int devfn,
> > +                                    const u8 *enetaddr)
> > +{
> > +#ifdef CONFIG_ARCH_LS1028A
> > +/*
> > + * LS1028A is the only part with IERB at this time and there are
> > plans to change
> > + * its structure, keep this LS1028A specific for now
> > + */
> > +#define IERB_BASE            0x1f0800000ULL
> > +#define IERB_PFMAC(pf, vf, n)        (IERB_BASE + 0x8000 + (pf) * 0x100 +
> > (vf) * 8 \
> > +                              + (n) * 4)
> > +
> > +static int ierb_fn_to_pf[] = {0, 1, 2, -1, -1, -1, 3};
> wrong indendation
>
> > +
> > +     u16 lower = *(const u16 *)(enetaddr + 4);
> > +     u32 upper = *(const u32 *)enetaddr;
> > +
> > +     if (ierb_fn_to_pf[devfn] < 0)
> > +             return;
> it would be easier to read if ierb_fn_to_pf[devfn] would be assigned to
> a local variable.
>

Alex, after you address this feedback from Michael, you can add my:

Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>

> > +
> > +     out_le32(IERB_PFMAC(ierb_fn_to_pf[devfn], 0, 0), upper);
> > +     out_le32(IERB_PFMAC(ierb_fn_to_pf[devfn], 0, 1), (u32)lower);
> > +#endif
> > +}
> > +
> > +/* sets up primary MAC addresses in DT/IERB */
> > +void fdt_fixup_enetc_mac(void *blob)
> > +{
> > +     struct pci_child_platdata *ppdata;
> > +     struct eth_pdata *pdata;
> > +     struct udevice *dev;
> > +     struct uclass *uc;
> > +     char path[256];
> > +     int offset;
> > +     int devfn;
> > +
> > +     uclass_get(UCLASS_ETH, &uc);
> > +     uclass_foreach_dev(dev, uc) {
> > +             if (!dev->driver || !dev->driver->name ||
> > +                 strcmp(dev->driver->name, ENETC_DRIVER_NAME))
> > +                     continue;
> > +
> > +             pdata = dev_get_platdata(dev);
> > +             ppdata = dev_get_parent_platdata(dev);
> > +             devfn = PCI_FUNC(ppdata->devfn);
> > +
> > +             enetc_set_ierb_primary_mac(dev, devfn, pdata->enetaddr);
> > +
> > +             snprintf(path, 256, "/soc/pcie at 1f0000000/ethernet@%x,%x",
> > +                      PCI_DEV(ppdata->devfn), PCI_FUNC(ppdata->devfn));
> > +             offset = fdt_path_offset(blob, path);
> > +             if (offset < 0)
> > +                     continue;
> > +             fdt_setprop(blob, offset, "mac-address", pdata->enetaddr, 6);
> > +     }
> > +}
> > +
> >  /*
> >   * Bind the device:
> >   * - set a more explicit name on the interface
> > @@ -583,7 +646,7 @@ static const struct eth_ops enetc_ops = {
> >  };
> >
> >  U_BOOT_DRIVER(eth_enetc) = {
> > -     .name   = "enetc_eth",
> > +     .name   = ENETC_DRIVER_NAME,
> >       .id     = UCLASS_ETH,
> >       .bind   = enetc_bind,
> >       .probe  = enetc_probe,
> > diff --git a/drivers/net/fsl_enetc.h b/drivers/net/fsl_enetc.h
> > index 0bb4cdff47..e441891468 100644
> > --- a/drivers/net/fsl_enetc.h
> > +++ b/drivers/net/fsl_enetc.h
> > @@ -226,4 +226,7 @@ int enetc_mdio_read_priv(struct enetc_mdio_priv
> > *priv, int addr, int devad,
> >  int enetc_mdio_write_priv(struct enetc_mdio_priv *priv, int addr, int
> > devad,
> >                         int reg, u16 val);
> >
> > +/* sets up primary MAC addresses in DT/IERB */
> > +void fdt_fixup_enetc_mac(void *blob);
> > +
> >  #endif /* _ENETC_H */

Regards,
-Vladimir

[PATCH v2] drivers: net: fsl_enetc: Pass on primary MAC address to Linux

[Alexandru Marginean]

On 12/10/2019 11:47 PM, Michael Walle wrote:
> Am 2019-12-10 15:55, schrieb Alex Marginean:
>> Passes on the primary address used by u-boot to Linux.  The code does 
>> a DT
>> fix-up for ENETC PFs and sets the primary MAC address in IERB.  The 
>> address
>> in IERB is restored on ENETC PCI functions at FLR.
> 
> 
> I don't get the reason why this is done in a proprietary way. What is the
> difference between any other network interface whose hardware address is
> set up in the generic u-boot code.

By proprietary do you mean IERB?

Network cards normally have a ROM which comes preset from the factory 
with default MAC addresses.  At run-time drivers can use the default 
address or replace it.  The MAC address in IERB is the default MAC 
address for ENETC interfaces at run-time, this address is available to 
the driver/stack after issuing an FLR and in the absence of a DT node 
for the PCI function.
We can pass MAC addresses to Linux through DT alone, but that's more of 
a hassle if Linux decides to assign the PCI function to a guest or 
user-space app.  Using the IERB values doesn't require any fix-up for 
guest DTs.

IERB is not actually a ROM though and we need U-Boot to set factory MAC 
addresses into IERB some time before Linux boots.

> Also, shouldn't write_hwaddr callback be implemented instead of the
> enetc_set_ierb_primary_mac()?

OK, makes sense, I will move the code there.

I'll send a v3,
Thanks!
Alex

> 
> -michael
> 
>> Signed-off-by: Alex Marginean <alexandru.marginean@nxp.com>
>> ---
>>
>> The code is called fom ft_board_setup in 
>> board/freescale/ls1028a/ls1028a.c
>> mostly for consistency with other LS parts.  I'm open to suggestions 
>> though.
>>
>> Changes in v2:
>>  - fixed warning for fdt_fixup_enetc_mac being implicitly declared
>>
>>  board/freescale/ls1028a/ls1028a.c |  5 +++
>>  drivers/net/fsl_enetc.c           | 65 ++++++++++++++++++++++++++++++-
>>  drivers/net/fsl_enetc.h           |  3 ++
>>  3 files changed, 72 insertions(+), 1 deletion(-)
>>
>> diff --git a/board/freescale/ls1028a/ls1028a.c
>> b/board/freescale/ls1028a/ls1028a.c
>> index a9606b8865..1a82c95402 100644
>> --- a/board/freescale/ls1028a/ls1028a.c
>> +++ b/board/freescale/ls1028a/ls1028a.c
>> @@ -25,6 +25,7 @@
>>  #include <fdtdec.h>
>>  #include <miiphy.h>
>>  #include "../common/qixis.h"
>> +#include "../drivers/net/fsl_enetc.h"
>>
>>  DECLARE_GLOBAL_DATA_PTR;
>>
>> @@ -150,6 +151,10 @@ int ft_board_setup(void *blob, bd_t *bd)
>>
>>      fdt_fixup_icid(blob);
>>
>> +#ifdef CONFIG_FSL_ENETC
>> +    fdt_fixup_enetc_mac(blob);
>> +#endif
>> +
>>      return 0;
>>  }
>>  #endif
>> diff --git a/drivers/net/fsl_enetc.c b/drivers/net/fsl_enetc.c
>> index 0ca7e838a8..3c043888db 100644
>> --- a/drivers/net/fsl_enetc.c
>> +++ b/drivers/net/fsl_enetc.c
>> @@ -14,6 +14,69 @@
>>
>>  #include "fsl_enetc.h"
>>
>> +#define ENETC_DRIVER_NAME    "enetc_eth"
>> +
>> +/*
>> + * sets the MAC address in IERB registers, this setting is persistent 
>> and
>> + * carried over to Linux.
>> + */
>> +static void enetc_set_ierb_primary_mac(struct udevice *dev, int devfn,
>> +                       const u8 *enetaddr)
>> +{
>> +#ifdef CONFIG_ARCH_LS1028A
>> +/*
>> + * LS1028A is the only part with IERB at this time and there are
>> plans to change
>> + * its structure, keep this LS1028A specific for now
>> + */
>> +#define IERB_BASE        0x1f0800000ULL
>> +#define IERB_PFMAC(pf, vf, n)    (IERB_BASE + 0x8000 + (pf) * 0x100 + 
>> (vf) * 8 \
>> +                 + (n) * 4)
>> +
>> +static int ierb_fn_to_pf[] = {0, 1, 2, -1, -1, -1, 3};
> wrong indendation
> 
>> +
>> +    u16 lower = *(const u16 *)(enetaddr + 4);
>> +    u32 upper = *(const u32 *)enetaddr;
>> +
>> +    if (ierb_fn_to_pf[devfn] < 0)
>> +        return;
> it would be easier to read if ierb_fn_to_pf[devfn] would be assigned to 
> a local variable.
> 
>> +
>> +    out_le32(IERB_PFMAC(ierb_fn_to_pf[devfn], 0, 0), upper);
>> +    out_le32(IERB_PFMAC(ierb_fn_to_pf[devfn], 0, 1), (u32)lower);
>> +#endif
>> +}
>> +
>> +/* sets up primary MAC addresses in DT/IERB */
>> +void fdt_fixup_enetc_mac(void *blob)
>> +{
>> +    struct pci_child_platdata *ppdata;
>> +    struct eth_pdata *pdata;
>> +    struct udevice *dev;
>> +    struct uclass *uc;
>> +    char path[256];
>> +    int offset;
>> +    int devfn;
>> +
>> +    uclass_get(UCLASS_ETH, &uc);
>> +    uclass_foreach_dev(dev, uc) {
>> +        if (!dev->driver || !dev->driver->name ||
>> +            strcmp(dev->driver->name, ENETC_DRIVER_NAME))
>> +            continue;
>> +
>> +        pdata = dev_get_platdata(dev);
>> +        ppdata = dev_get_parent_platdata(dev);
>> +        devfn = PCI_FUNC(ppdata->devfn);
>> +
>> +        enetc_set_ierb_primary_mac(dev, devfn, pdata->enetaddr);
>> +
>> +        snprintf(path, 256, "/soc/pcie at 1f0000000/ethernet@%x,%x",
>> +             PCI_DEV(ppdata->devfn), PCI_FUNC(ppdata->devfn));
>> +        offset = fdt_path_offset(blob, path);
>> +        if (offset < 0)
>> +            continue;
>> +        fdt_setprop(blob, offset, "mac-address", pdata->enetaddr, 6);
>> +    }
>> +}
>> +
>>  /*
>>   * Bind the device:
>>   * - set a more explicit name on the interface
>> @@ -583,7 +646,7 @@ static const struct eth_ops enetc_ops = {
>>  };
>>
>>  U_BOOT_DRIVER(eth_enetc) = {
>> -    .name    = "enetc_eth",
>> +    .name    = ENETC_DRIVER_NAME,
>>      .id    = UCLASS_ETH,
>>      .bind    = enetc_bind,
>>      .probe    = enetc_probe,
>> diff --git a/drivers/net/fsl_enetc.h b/drivers/net/fsl_enetc.h
>> index 0bb4cdff47..e441891468 100644
>> --- a/drivers/net/fsl_enetc.h
>> +++ b/drivers/net/fsl_enetc.h
>> @@ -226,4 +226,7 @@ int enetc_mdio_read_priv(struct enetc_mdio_priv
>> *priv, int addr, int devad,
>>  int enetc_mdio_write_priv(struct enetc_mdio_priv *priv, int addr, int 
>> devad,
>>                int reg, u16 val);
>>
>> +/* sets up primary MAC addresses in DT/IERB */
>> +void fdt_fixup_enetc_mac(void *blob);
>> +
>>  #endif /* _ENETC_H */

[PATCH v2] drivers: net: fsl_enetc: Pass on primary MAC address to Linux

[Michael Walle]

Hi Vladimir,

Am 2019-12-11 13:46, schrieb Vladimir Oltean:
> Hi Michael,
> 
> On Wed, 11 Dec 2019 at 00:48, Michael Walle <michael@walle.cc> wrote:
>> 
>> Am 2019-12-10 15:55, schrieb Alex Marginean:
>> > Passes on the primary address used by u-boot to Linux.  The code does a
>> > DT
>> > fix-up for ENETC PFs and sets the primary MAC address in IERB.  The
>> > address
>> > in IERB is restored on ENETC PCI functions at FLR.
>> 
>> 
>> I don't get the reason why this is done in a proprietary way. What is
>> the
>> difference between any other network interface whose hardware address 
>> is
>> set up in the generic u-boot code.
>> 
>> Also, shouldn't write_hwaddr callback be implemented instead of the
>> enetc_set_ierb_primary_mac()?
>> 
> 
> At the moment, the Linux driver ignored the device tree and only reads
> the POR values of the SIPMAR registers. The reset value of those comes
> from the IERB space, which U-Boot is configuring. So it would be good
> if that behavior keeps working.
> 
> It would also be good if the Linux driver called of_get_mac_address,
> so it needs the device tree binding for that. That's why both fixups
> are performed, and why the generic function is not used.

yes, but u-boot already sets the mac-address/local-mac-address property
in the device tree already in a generic way, see fdt_fixup_ethernet().

> As for the write_hwaddr callback instead of
> enetc_set_primary_mac_addr, that is valid but I suppose it is outside
> the scope of this patch. That function is related to the runtime MAC
> address and not to the MAC passed to Linux.

according to the comment in eth-uclass.c this is not for (u-boot) 
runtime:

  /*
   * Devices need to write the hwaddr even if not started so that Linux
   * will have access to the hwaddr that u-boot stored for the device.
   * This is accomplished by attempting to probe each device and calling
   * their write_hwaddr() operation.
   */

and the runtime mac address for u-boot is already set enetc_start().

-michael

[PATCH v2] drivers: net: fsl_enetc: Pass on primary MAC address to Linux

[Michael Walle]

Am 2019-12-11 14:04, schrieb Alexandru Marginean:
> On 12/10/2019 11:47 PM, Michael Walle wrote:
>> Am 2019-12-10 15:55, schrieb Alex Marginean:
>>> Passes on the primary address used by u-boot to Linux.  The code does 
>>> a DT
>>> fix-up for ENETC PFs and sets the primary MAC address in IERB.  The 
>>> address
>>> in IERB is restored on ENETC PCI functions at FLR.
>> 
>> 
>> I don't get the reason why this is done in a proprietary way. What is 
>> the
>> difference between any other network interface whose hardware address 
>> is
>> set up in the generic u-boot code.
> 
> By proprietary do you mean IERB?

I meant the fdt fixups for mac-address/local-mac-address which is not
handled in the generic u-boot code, see my previous mail.

Because if it would, then there would be no need for the board specific
code below (just the one function call actually).

> Network cards normally have a ROM which comes preset from the factory
> with default MAC addresses.  At run-time drivers can use the default
> address or replace it.  The MAC address in IERB is the default MAC
> address for ENETC interfaces at run-time, this address is available to
> the driver/stack after issuing an FLR and in the absence of a DT node
> for the PCI function.
> We can pass MAC addresses to Linux through DT alone, but that's more
> of a hassle if Linux decides to assign the PCI function to a guest or
> user-space app.  Using the IERB values doesn't require any fix-up for
> guest DTs.

Understood.

> IERB is not actually a ROM though and we need U-Boot to set factory
> MAC addresses into IERB some time before Linux boots.
> 
>> Also, shouldn't write_hwaddr callback be implemented instead of the
>> enetc_set_ierb_primary_mac()?
> 
> OK, makes sense, I will move the code there.
> 

-michael

[PATCH v2] drivers: net: fsl_enetc: Pass on primary MAC address to Linux

[Alexandru Marginean]

On 12/11/2019 2:16 PM, Michael Walle wrote:
> Hi Vladimir,
> 
> Am 2019-12-11 13:46, schrieb Vladimir Oltean:
>> Hi Michael,
>>
>> On Wed, 11 Dec 2019 at 00:48, Michael Walle <michael@walle.cc> wrote:
>>>
>>> Am 2019-12-10 15:55, schrieb Alex Marginean:
>>> > Passes on the primary address used by u-boot to Linux.  The code 
>>> does a
>>> > DT
>>> > fix-up for ENETC PFs and sets the primary MAC address in IERB.  The
>>> > address
>>> > in IERB is restored on ENETC PCI functions at FLR.
>>>
>>>
>>> I don't get the reason why this is done in a proprietary way. What is
>>> the
>>> difference between any other network interface whose hardware address is
>>> set up in the generic u-boot code.
>>>
>>> Also, shouldn't write_hwaddr callback be implemented instead of the
>>> enetc_set_ierb_primary_mac()?
>>>
>>
>> At the moment, the Linux driver ignored the device tree and only reads
>> the POR values of the SIPMAR registers. The reset value of those comes
>> from the IERB space, which U-Boot is configuring. So it would be good
>> if that behavior keeps working.
>>
>> It would also be good if the Linux driver called of_get_mac_address,
>> so it needs the device tree binding for that. That's why both fixups
>> are performed, and why the generic function is not used.
> 
> yes, but u-boot already sets the mac-address/local-mac-address property
> in the device tree already in a generic way, see fdt_fixup_ethernet().

I think fdt_fixup_ethernet is not a good choice for us.
The issue is that it ties Linux DT to device indexes in U-Boot.  That's 
a problem if we plug an Eth PCI card in, we would need to change Linux 
DT, which is definitely not desirable.
We actually do use PCI Eth cards with some of our boards and U-Boot does 
pick those up and indexes shift.

Also U-Boot and Linux DTs have to be in sync all the time, if we disable 
one port in U-Boot but not in Linux we would mix up MAC addresses.

So as far as using generic fix-up code I'm all for it, but in this case 
we would need some platform specific rules to match Linux DT nodes to 
U-Boot eth addresses.

>> As for the write_hwaddr callback instead of
>> enetc_set_primary_mac_addr, that is valid but I suppose it is outside
>> the scope of this patch. That function is related to the runtime MAC
>> address and not to the MAC passed to Linux.
> 
> according to the comment in eth-uclass.c this is not for (u-boot) runtime:
> 
>   /*
>    * Devices need to write the hwaddr even if not started so that Linux
>    * will have access to the hwaddr that u-boot stored for the device.
>    * This is accomplished by attempting to probe each device and calling
>    * their write_hwaddr() operation.
>    */
> 
> and the runtime mac address for u-boot is already set enetc_start().
> 
> -michael

This is fine, I'll move the IERB code to write_hwaddr.

Alex

[PATCH v2] drivers: net: fsl_enetc: Pass on primary MAC address to Linux

[Michael Walle]

Hi Alex,

Am 2019-12-11 16:37, schrieb Alexandru Marginean:
> On 12/11/2019 2:16 PM, Michael Walle wrote:
>> Hi Vladimir,
>> 
>> Am 2019-12-11 13:46, schrieb Vladimir Oltean:
>>> Hi Michael,
>>> 
>>> On Wed, 11 Dec 2019 at 00:48, Michael Walle <michael@walle.cc> wrote:
>>>> 
>>>> Am 2019-12-10 15:55, schrieb Alex Marginean:
>>>> > Passes on the primary address used by u-boot to Linux.  The code does a
>>>> > DT
>>>> > fix-up for ENETC PFs and sets the primary MAC address in IERB.  The
>>>> > address
>>>> > in IERB is restored on ENETC PCI functions at FLR.
>>>> 
>>>> 
>>>> I don't get the reason why this is done in a proprietary way. What 
>>>> is
>>>> the
>>>> difference between any other network interface whose hardware 
>>>> address is
>>>> set up in the generic u-boot code.
>>>> 
>>>> Also, shouldn't write_hwaddr callback be implemented instead of the
>>>> enetc_set_ierb_primary_mac()?
>>>> 
>>> 
>>> At the moment, the Linux driver ignored the device tree and only 
>>> reads
>>> the POR values of the SIPMAR registers. The reset value of those 
>>> comes
>>> from the IERB space, which U-Boot is configuring. So it would be good
>>> if that behavior keeps working.
>>> 
>>> It would also be good if the Linux driver called of_get_mac_address,
>>> so it needs the device tree binding for that. That's why both fixups
>>> are performed, and why the generic function is not used.
>> 
>> yes, but u-boot already sets the mac-address/local-mac-address 
>> property
>> in the device tree already in a generic way, see fdt_fixup_ethernet().
> 
> I think fdt_fixup_ethernet is not a good choice for us.
> The issue is that it ties Linux DT to device indexes in U-Boot.
> That's a problem if we plug an Eth PCI card in, we would need to
> change Linux DT, which is definitely not desirable.
> We actually do use PCI Eth cards with some of our boards and U-Boot
> does pick those up and indexes shift.

are you sure? afaik it works by reading the /alias/ethernetN phandles
which gets ethNaddr assigned if you set the FDT_SEQ_MACADDR_FROM_ENV
config option. I've just tried it, here is my linux dts diff

--- a/arch/arm64/boot/dts/freescale/fsl-ls1028a.dtsi
+++ b/arch/arm64/boot/dts/freescale/fsl-ls1028a.dtsi
@@ -17,6 +17,11 @@
         #address-cells = <2>;
         #size-cells = <2>;

+       aliases {
+               ethernet0 = &enetc_port0;
+               ethernet1 = &enetc_port1;
+       };
+
         cpus {
                 #address-cells = <1>;
                 #size-cells = <0>;
@@ -761,10 +766,12 @@
                         enetc_port0: ethernet at 0,0 {
                                 compatible = "fsl,enetc";
                                 reg = <0x000000 0 0 0 0>;
+                               local-mac-address = [ 00 00 00 00 00 00 
];
                         };
                         enetc_port1: ethernet at 0,1 {
                                 compatible = "fsl,enetc";
                                 reg = <0x000100 0 0 0 0>;
+                               local-mac-address = [ 00 00 00 00 00 00 
];
                         };
                         enetc_mdio_pf3: mdio at 0,3 {
                                 compatible = "fsl,enetc-mdio";

That way the mapping between ethNaddr and the network device can also
be changed by the user by changing the linux device tree.

also, uboot should respect the /aliases/ethernetN, too.

BTW what will be the source of the network addresses, the u-boot
environment variables? (which might be set either by the user or some
kind of board specific code).

> Also U-Boot and Linux DTs have to be in sync all the time, if we
> disable one port in U-Boot but not in Linux we would mix up MAC
> addresses.

I see. But that shouldn't happen with the code above. But are you sure
that this

> +       uclass_get(UCLASS_ETH, &uc);
> +       uclass_foreach_dev(dev, uc) {

will work then? in my config (just enetc-0) there is only one eth device

# dm tree
[snip]
  pci           2  [ + ]   pci_generic_ecam      |-- pcie at 1f0000000
  eth           0  [ + ]   enetc_eth             |   |-- enetc-0
  mdio          0  [ + ]   enetc_mdio            |   |-- emdio-3
  pci_generi    0  [   ]   pci_generic_drv       |   |-- pci_4:0.4
  dsa           0  [   ]   felix-switch          |   |-- felix-switch
  pci_generi    1  [   ]   pci_generic_drv       |   `-- pci_4:1f.0
[snip]

> 
> So as far as using generic fix-up code I'm all for it, but in this
> case we would need some platform specific rules to match Linux DT
> nodes to U-Boot eth addresses.
> 
>>> As for the write_hwaddr callback instead of
>>> enetc_set_primary_mac_addr, that is valid but I suppose it is outside
>>> the scope of this patch. That function is related to the runtime MAC
>>> address and not to the MAC passed to Linux.
>> 
>> according to the comment in eth-uclass.c this is not for (u-boot) 
>> runtime:
>> 
>>   /*
>>    * Devices need to write the hwaddr even if not started so that 
>> Linux
>>    * will have access to the hwaddr that u-boot stored for the device.
>>    * This is accomplished by attempting to probe each device and 
>> calling
>>    * their write_hwaddr() operation.
>>    */
>> 
>> and the runtime mac address for u-boot is already set enetc_start().
>> 
>> -michael
> 
> This is fine, I'll move the IERB code to write_hwaddr.

This will also only be called if the device is not disabled in u-boot,
from what I see. So it might be better to "fix" this.

I bet you are not the first/only one which needs this kind of "how do we
propagate the hardware address to linux" where the devices might be 
unused
in u-boot. There must be a better way to do this ;)


-michael

[PATCH v2] drivers: net: fsl_enetc: Pass on primary MAC address to Linux

[Alexandru Marginean]

Hi Michael,

On 12/11/2019 6:03 PM, Michael Walle wrote:
> Hi Alex,
> 
> Am 2019-12-11 16:37, schrieb Alexandru Marginean:
>> On 12/11/2019 2:16 PM, Michael Walle wrote:
>>> Hi Vladimir,
>>>
>>> Am 2019-12-11 13:46, schrieb Vladimir Oltean:
>>>> Hi Michael,
>>>>
>>>> On Wed, 11 Dec 2019 at 00:48, Michael Walle <michael@walle.cc> wrote:
>>>>>
>>>>> Am 2019-12-10 15:55, schrieb Alex Marginean:
>>>>> > Passes on the primary address used by u-boot to Linux.  The code 
>>>>> does a
>>>>> > DT
>>>>> > fix-up for ENETC PFs and sets the primary MAC address in IERB.  The
>>>>> > address
>>>>> > in IERB is restored on ENETC PCI functions at FLR.
>>>>>
>>>>>
>>>>> I don't get the reason why this is done in a proprietary way. What is
>>>>> the
>>>>> difference between any other network interface whose hardware 
>>>>> address is
>>>>> set up in the generic u-boot code.
>>>>>
>>>>> Also, shouldn't write_hwaddr callback be implemented instead of the
>>>>> enetc_set_ierb_primary_mac()?
>>>>>
>>>>
>>>> At the moment, the Linux driver ignored the device tree and only reads
>>>> the POR values of the SIPMAR registers. The reset value of those comes
>>>> from the IERB space, which U-Boot is configuring. So it would be good
>>>> if that behavior keeps working.
>>>>
>>>> It would also be good if the Linux driver called of_get_mac_address,
>>>> so it needs the device tree binding for that. That's why both fixups
>>>> are performed, and why the generic function is not used.
>>>
>>> yes, but u-boot already sets the mac-address/local-mac-address property
>>> in the device tree already in a generic way, see fdt_fixup_ethernet().
>>
>> I think fdt_fixup_ethernet is not a good choice for us.
>> The issue is that it ties Linux DT to device indexes in U-Boot.
>> That's a problem if we plug an Eth PCI card in, we would need to
>> change Linux DT, which is definitely not desirable.
>> We actually do use PCI Eth cards with some of our boards and U-Boot
>> does pick those up and indexes shift.
> 
> are you sure? afaik it works by reading the /alias/ethernetN phandles
> which gets ethNaddr assigned if you set the FDT_SEQ_MACADDR_FROM_ENV
> config option. I've just tried it, here is my linux dts diff
> 
> --- a/arch/arm64/boot/dts/freescale/fsl-ls1028a.dtsi
> +++ b/arch/arm64/boot/dts/freescale/fsl-ls1028a.dtsi
> @@ -17,6 +17,11 @@
>          #address-cells = <2>;
>          #size-cells = <2>;
> 
> +       aliases {
> +               ethernet0 = &enetc_port0;
> +               ethernet1 = &enetc_port1;
> +       };
> +
>          cpus {
>                  #address-cells = <1>;
>                  #size-cells = <0>;
> @@ -761,10 +766,12 @@
>                          enetc_port0: ethernet at 0,0 {
>                                  compatible = "fsl,enetc";
>                                  reg = <0x000000 0 0 0 0>;
> +                               local-mac-address = [ 00 00 00 00 00 00 ];
>                          };
>                          enetc_port1: ethernet at 0,1 {
>                                  compatible = "fsl,enetc";
>                                  reg = <0x000100 0 0 0 0>;
> +                               local-mac-address = [ 00 00 00 00 00 00 ];
>                          };
>                          enetc_mdio_pf3: mdio at 0,3 {
>                                  compatible = "fsl,enetc-mdio";
> 
> That way the mapping between ethNaddr and the network device can also
> be changed by the user by changing the linux device tree.
> 
> also, uboot should respect the /aliases/ethernetN, too.

I don't disagree with any of that, but that's not the issue I mentioned. 
  I meant actual PCI cards being plugged in, I didn't mean having 
disabled ECAM functions.

In your example DT  enetc port 0 is tied to /aliases/ethernet0 and to 
ethaddr, enetc port 1 is tied to /aliases/ethernet1 and to eth1addr.

A LS1028 board with a PCI Eth card plugged in shows this:

Net:   e1000: 68:05:ca:66:bf:bd
        eth0: e1000#0 [PRIME], eth1: enetc-0, eth2: enetc-2, eth3: swp0, 
eth4: swp1, eth5: swp2, eth6: swp3

In this case eth0 is the e1000 card and it uses ethaddr, enetc port 0 is 
eth1 and uses eth1addr.  The fix-up to /aliases/ethernet0 in your 
example makes enetc port 0 get the MAC address of the PCI card.  If the 
PCI card is removed then enetc port 0 ends up being eth0 in U-Boot and 
and actually use ethaddr, not eth1addr.
To make this work with a PCI card plugged in one would need to change 
the aliases in Linux DT, which is not a fun thing to do.

> BTW what will be the source of the network addresses, the u-boot
> environment variables? (which might be set either by the user or some
> kind of board specific code).

Yes, the environment variables.  As far as I know these come preset from 
the factory.  The reference boards usually come with stickers too, 
listing the preset MAC addresses.

>> Also U-Boot and Linux DTs have to be in sync all the time, if we
>> disable one port in U-Boot but not in Linux we would mix up MAC
>> addresses.
> 
> I see. But that shouldn't happen with the code above. But are you sure
> that this
> 
>> +       uclass_get(UCLASS_ETH, &uc);
>> +       uclass_foreach_dev(dev, uc) {
> 
> will work then? in my config (just enetc-0) there is only one eth device
> 
> # dm tree
> [snip]
>   pci           2  [ + ]   pci_generic_ecam      |-- pcie at 1f0000000
>   eth           0  [ + ]   enetc_eth             |   |-- enetc-0
>   mdio          0  [ + ]   enetc_mdio            |   |-- emdio-3
>   pci_generi    0  [   ]   pci_generic_drv       |   |-- pci_4:0.4
>   dsa           0  [   ]   felix-switch          |   |-- felix-switch
>   pci_generi    1  [   ]   pci_generic_drv       |   `-- pci_4:1f.0
> [snip]

Yeah, I guess that's no better either way.  U-Boot wouldn't be able to 
fix-up addresses for interfaces it didn't know about, yes.  It works if 
Linux doesn't use some interfaces that U-Boot does, the MAC addresses 
per port would match between U-Boot and Linux.  That case isn't too 
useful though, if they differ it's probably the other way around, and 
U-Boot has fewer interfaces.

There is no simple solution to satisfy all cases really.
Using an index encoded in the DT to identify a given interface isn't OK, 
in my example there's a PCI card that gets probed first and then all IDs 
shift up.
Fixing up addresses for interfaces U-Boot doesn't know about is also a 
problem, as the fix-up code should stay clear of eth addresses used by 
live interfaces, on the assumption that those should continue using 
them.  Assume the device has two interfaces, first one is disabled in 
U-Boot but enabled in Linux.  Should it use ethaddr in Linux, or 
eth1addr?  Ethaddr is associated with 2nd interface in U-Boot and it 
makes sense to maintain that association in Linux.

>>
>> So as far as using generic fix-up code I'm all for it, but in this
>> case we would need some platform specific rules to match Linux DT
>> nodes to U-Boot eth addresses.
>>
>>>> As for the write_hwaddr callback instead of
>>>> enetc_set_primary_mac_addr, that is valid but I suppose it is outside
>>>> the scope of this patch. That function is related to the runtime MAC
>>>> address and not to the MAC passed to Linux.
>>>
>>> according to the comment in eth-uclass.c this is not for (u-boot) 
>>> runtime:
>>>
>>>   /*
>>>    * Devices need to write the hwaddr even if not started so that Linux
>>>    * will have access to the hwaddr that u-boot stored for the device.
>>>    * This is accomplished by attempting to probe each device and calling
>>>    * their write_hwaddr() operation.
>>>    */
>>>
>>> and the runtime mac address for u-boot is already set enetc_start().
>>>
>>> -michael
>>
>> This is fine, I'll move the IERB code to write_hwaddr.
> 
> This will also only be called if the device is not disabled in u-boot,
> from what I see. So it might be better to "fix" this.
> 
> I bet you are not the first/only one which needs this kind of "how do we
> propagate the hardware address to linux" where the devices might be unused
> in u-boot. There must be a better way to do this ;)
> 
> 
> -michael

Yeah, I'm sure others bumped into this before.
For sure fdt_fixup_ethernet as defined now can't cover all cases.  It 
can only rely on the U-Boot interface index which is volatile with 
plug-in PCI cards.  We could introduce some new index for integrated 
interfaces that's set by the driver, maybe improve on existing req_seq. 
In current code req_seq is not too useful, as dev->seq is allocated in 
order of discovery, so it doesn't help if 2nd found device requests seq 
0, that's already in use.  But that could probably be improved.

For now I think I'll stick to current code, proprietary as it is and 
with the limitation that U-Boot has to enable interfaces that Linux uses.
I am open to any better suggestions though.

Thanks!
Alex

[PATCH v2] drivers: net: fsl_enetc: Pass on primary MAC address to Linux

[Michael Walle]

Hi Alex,

Am 2019-12-11 22:01, schrieb Alexandru Marginean:
> Hi Michael,
> 
> On 12/11/2019 6:03 PM, Michael Walle wrote:
>> Hi Alex,
>> 
>> Am 2019-12-11 16:37, schrieb Alexandru Marginean:
>>> On 12/11/2019 2:16 PM, Michael Walle wrote:
>>>> Hi Vladimir,
>>>> 
>>>> Am 2019-12-11 13:46, schrieb Vladimir Oltean:
>>>>> Hi Michael,
>>>>> 
>>>>> On Wed, 11 Dec 2019 at 00:48, Michael Walle <michael@walle.cc> 
>>>>> wrote:
>>>>>> 
>>>>>> Am 2019-12-10 15:55, schrieb Alex Marginean:
>>>>>> > Passes on the primary address used by u-boot to Linux.  The code does a
>>>>>> > DT
>>>>>> > fix-up for ENETC PFs and sets the primary MAC address in IERB.  The
>>>>>> > address
>>>>>> > in IERB is restored on ENETC PCI functions at FLR.
>>>>>> 
>>>>>> 
>>>>>> I don't get the reason why this is done in a proprietary way. What 
>>>>>> is
>>>>>> the
>>>>>> difference between any other network interface whose hardware 
>>>>>> address is
>>>>>> set up in the generic u-boot code.
>>>>>> 
>>>>>> Also, shouldn't write_hwaddr callback be implemented instead of 
>>>>>> the
>>>>>> enetc_set_ierb_primary_mac()?
>>>>>> 
>>>>> 
>>>>> At the moment, the Linux driver ignored the device tree and only 
>>>>> reads
>>>>> the POR values of the SIPMAR registers. The reset value of those 
>>>>> comes
>>>>> from the IERB space, which U-Boot is configuring. So it would be 
>>>>> good
>>>>> if that behavior keeps working.
>>>>> 
>>>>> It would also be good if the Linux driver called 
>>>>> of_get_mac_address,
>>>>> so it needs the device tree binding for that. That's why both 
>>>>> fixups
>>>>> are performed, and why the generic function is not used.
>>>> 
>>>> yes, but u-boot already sets the mac-address/local-mac-address 
>>>> property
>>>> in the device tree already in a generic way, see 
>>>> fdt_fixup_ethernet().
>>> 
>>> I think fdt_fixup_ethernet is not a good choice for us.
>>> The issue is that it ties Linux DT to device indexes in U-Boot.
>>> That's a problem if we plug an Eth PCI card in, we would need to
>>> change Linux DT, which is definitely not desirable.
>>> We actually do use PCI Eth cards with some of our boards and U-Boot
>>> does pick those up and indexes shift.
>> 
>> are you sure? afaik it works by reading the /alias/ethernetN phandles
>> which gets ethNaddr assigned if you set the FDT_SEQ_MACADDR_FROM_ENV
>> config option. I've just tried it, here is my linux dts diff
>> 
>> --- a/arch/arm64/boot/dts/freescale/fsl-ls1028a.dtsi
>> +++ b/arch/arm64/boot/dts/freescale/fsl-ls1028a.dtsi
>> @@ -17,6 +17,11 @@
>>          #address-cells = <2>;
>>          #size-cells = <2>;
>> 
>> +       aliases {
>> +               ethernet0 = &enetc_port0;
>> +               ethernet1 = &enetc_port1;
>> +       };
>> +
>>          cpus {
>>                  #address-cells = <1>;
>>                  #size-cells = <0>;
>> @@ -761,10 +766,12 @@
>>                          enetc_port0: ethernet at 0,0 {
>>                                  compatible = "fsl,enetc";
>>                                  reg = <0x000000 0 0 0 0>;
>> +                               local-mac-address = [ 00 00 00 00 00 
>> 00 ];
>>                          };
>>                          enetc_port1: ethernet at 0,1 {
>>                                  compatible = "fsl,enetc";
>>                                  reg = <0x000100 0 0 0 0>;
>> +                               local-mac-address = [ 00 00 00 00 00 
>> 00 ];
>>                          };
>>                          enetc_mdio_pf3: mdio at 0,3 {
>>                                  compatible = "fsl,enetc-mdio";
>> 
>> That way the mapping between ethNaddr and the network device can also
>> be changed by the user by changing the linux device tree.
>> 
>> also, uboot should respect the /aliases/ethernetN, too.
> 
> I don't disagree with any of that, but that's not the issue I
> mentioned.  I meant actual PCI cards being plugged in, I didn't mean
> having disabled ECAM functions.
> 
> In your example DT  enetc port 0 is tied to /aliases/ethernet0 and to
> ethaddr, enetc port 1 is tied to /aliases/ethernet1 and to eth1addr.
> 
> A LS1028 board with a PCI Eth card plugged in shows this:
> 
> Net:   e1000: 68:05:ca:66:bf:bd
>        eth0: e1000#0 [PRIME], eth1: enetc-0, eth2: enetc-2, eth3:
> swp0, eth4: swp1, eth5: swp2, eth6: swp3
> 
> In this case eth0 is the e1000 card and it uses ethaddr, enetc port 0
> is eth1 and uses eth1addr.  The fix-up to /aliases/ethernet0 in your
> example makes enetc port 0 get the MAC address of the PCI card.  If
> the PCI card is removed then enetc port 0 ends up being eth0 in U-Boot
> and and actually use ethaddr, not eth1addr.

Mh, I've just tried this and it seems that this is even worse. Because
if you set the ethaddr, uboot will complain about mismatching ethernet
addresses.

> To make this work with a PCI card plugged in one would need to change
> the aliases in Linux DT, which is not a fun thing to do.
> 
>> BTW what will be the source of the network addresses, the u-boot
>> environment variables? (which might be set either by the user or some
>> kind of board specific code).
> 
> Yes, the environment variables.  As far as I know these come preset
> from the factory.  The reference boards usually come with stickers
> too, listing the preset MAC addresses.

so here is already the first problem, see above. Just to be sure, I
don't argue against having the environment as the source (actually
we do import the mac addresses from our eeprom into the environment),
just to make you aware that there is an underlying problem.

I've tried to use the aliases in u-boot, too. Unfortunately, the
e1000 will get the first slot, regardless that /aliases/ethernet0 is
a phandle to enetc-0. I guess that is the actual problem here. Because
if that would work then everything else would fall into place. (except
the write_hwaddr()).


>>> Also U-Boot and Linux DTs have to be in sync all the time, if we
>>> disable one port in U-Boot but not in Linux we would mix up MAC
>>> addresses.
>> 
>> I see. But that shouldn't happen with the code above. But are you sure
>> that this
>> 
>>> +       uclass_get(UCLASS_ETH, &uc);
>>> +       uclass_foreach_dev(dev, uc) {
>> 
>> will work then? in my config (just enetc-0) there is only one eth 
>> device
>> 
>> # dm tree
>> [snip]
>>   pci           2  [ + ]   pci_generic_ecam      |-- pcie at 1f0000000
>>   eth           0  [ + ]   enetc_eth             |   |-- enetc-0
>>   mdio          0  [ + ]   enetc_mdio            |   |-- emdio-3
>>   pci_generi    0  [   ]   pci_generic_drv       |   |-- pci_4:0.4
>>   dsa           0  [   ]   felix-switch          |   |-- felix-switch
>>   pci_generi    1  [   ]   pci_generic_drv       |   `-- pci_4:1f.0
>> [snip]
> 
> Yeah, I guess that's no better either way.  U-Boot wouldn't be able to
> fix-up addresses for interfaces it didn't know about, yes.  It works
> if Linux doesn't use some interfaces that U-Boot does, the MAC
> addresses per port would match between U-Boot and Linux.  That case
> isn't too useful though, if they differ it's probably the other way
> around, and U-Boot has fewer interfaces.
> 
> There is no simple solution to satisfy all cases really.

mhh just to let you know some cases we have:
  (1) enetc-0, enetc-1 in u-boot
      enetc-0, enetc-1, enetc-2 (and switch) in linux
  (2) enetc-1 in u-boot
      all enetc's and switch in linux
  (3a) well the "standard" ones where linux == uboot
  (3b) "standard one" but just enetc-1, skipping enetc-0

> Using an index encoded in the DT to identify a given interface isn't
> OK, in my example there's a PCI card that gets probed first and then
> all IDs shift up.

correct, and i guess that is the actual problem here. If it would be
appended to a pre-defined list (aka /aliases in u-boot) then there 
wouldn't
be any shift.

> Fixing up addresses for interfaces U-Boot doesn't know about is also a
> problem, as the fix-up code should stay clear of eth addresses used by
> live interfaces, on the assumption that those should continue using
> them.  Assume the device has two interfaces, first one is disabled in
> U-Boot but enabled in Linux.  Should it use ethaddr in Linux, or
> eth1addr?  Ethaddr is associated with 2nd interface in U-Boot and it
> makes sense to maintain that association in Linux.

If you say ethNaddr always maps to /alias/ethernetN, you won't have a
problem, will you? Other than ethXaddr might be unused.

Also consider the following:
  - you have consecutive mac addresses
  - u-boot uses enetc-1, enetc-0 is disabled, it would use ethaddr
    lets say (its  ..:00)
  - linux uses enetc-0 and enetc-1, then if it is not shifted,
    enetc-0 will have (..:01) and enetc-1 stays with (..:00).

Although it doesn't impact the function, it still looks bad.


>>> 
>>> So as far as using generic fix-up code I'm all for it, but in this
>>> case we would need some platform specific rules to match Linux DT
>>> nodes to U-Boot eth addresses.
>>> 
>>>>> As for the write_hwaddr callback instead of
>>>>> enetc_set_primary_mac_addr, that is valid but I suppose it is 
>>>>> outside
>>>>> the scope of this patch. That function is related to the runtime 
>>>>> MAC
>>>>> address and not to the MAC passed to Linux.
>>>> 
>>>> according to the comment in eth-uclass.c this is not for (u-boot) 
>>>> runtime:
>>>> 
>>>>   /*
>>>>    * Devices need to write the hwaddr even if not started so that 
>>>> Linux
>>>>    * will have access to the hwaddr that u-boot stored for the 
>>>> device.
>>>>    * This is accomplished by attempting to probe each device and 
>>>> calling
>>>>    * their write_hwaddr() operation.
>>>>    */
>>>> 
>>>> and the runtime mac address for u-boot is already set enetc_start().
>>>> 
>>>> -michael
>>> 
>>> This is fine, I'll move the IERB code to write_hwaddr.
>> 
>> This will also only be called if the device is not disabled in u-boot,
>> from what I see. So it might be better to "fix" this.
>> 
>> I bet you are not the first/only one which needs this kind of "how do 
>> we
>> propagate the hardware address to linux" where the devices might be 
>> unused
>> in u-boot. There must be a better way to do this ;)
>> 
>> 
>> -michael
> 
> Yeah, I'm sure others bumped into this before.
> For sure fdt_fixup_ethernet as defined now can't cover all cases.  It
> can only rely on the U-Boot interface index which is volatile with
> plug-in PCI cards.  We could introduce some new index for integrated
> interfaces that's set by the driver, maybe improve on existing
> req_seq. In current code req_seq is not too useful, as dev->seq is
> allocated in order of discovery, so it doesn't help if 2nd found
> device requests seq 0, that's already in use.  But that could probably
> be improved.
> 
> For now I think I'll stick to current code, proprietary as it is and
> with the limitation that U-Boot has to enable interfaces that Linux
> uses.

Please don't take that shortcut because it will be a pain to get it
cleaned-up and retain backwards compatibility :( Also like explained
above, it already falls short if you have ethaddr defined.

> I am open to any better suggestions though.

see above and like you've described it. append any plugable interfaces
at the end of the predefined list (with probably empty slots in used
ethNaddr).

-michael

[PATCH v2] drivers: net: fsl_enetc: Pass on primary MAC address to Linux

[Alexandru Marginean]

On 12/12/2019 1:12 PM, Michael Walle wrote:
> Hi Alex,
> 
> Am 2019-12-11 22:01, schrieb Alexandru Marginean:
>> Hi Michael,
>>
>> On 12/11/2019 6:03 PM, Michael Walle wrote:
>>> Hi Alex,
>>>
>>> Am 2019-12-11 16:37, schrieb Alexandru Marginean:
>>>> On 12/11/2019 2:16 PM, Michael Walle wrote:
>>>>> Hi Vladimir,
>>>>>
>>>>> Am 2019-12-11 13:46, schrieb Vladimir Oltean:
>>>>>> Hi Michael,
>>>>>>
>>>>>> On Wed, 11 Dec 2019 at 00:48, Michael Walle <michael@walle.cc> wrote:
>>>>>>>
>>>>>>> Am 2019-12-10 15:55, schrieb Alex Marginean:
>>>>>>> > Passes on the primary address used by u-boot to Linux.  The 
>>>>>>> code does a
>>>>>>> > DT
>>>>>>> > fix-up for ENETC PFs and sets the primary MAC address in IERB.  
>>>>>>> The
>>>>>>> > address
>>>>>>> > in IERB is restored on ENETC PCI functions at FLR.
>>>>>>>
>>>>>>>
>>>>>>> I don't get the reason why this is done in a proprietary way. 
>>>>>>> What is
>>>>>>> the
>>>>>>> difference between any other network interface whose hardware 
>>>>>>> address is
>>>>>>> set up in the generic u-boot code.
>>>>>>>
>>>>>>> Also, shouldn't write_hwaddr callback be implemented instead of the
>>>>>>> enetc_set_ierb_primary_mac()?
>>>>>>>
>>>>>>
>>>>>> At the moment, the Linux driver ignored the device tree and only 
>>>>>> reads
>>>>>> the POR values of the SIPMAR registers. The reset value of those 
>>>>>> comes
>>>>>> from the IERB space, which U-Boot is configuring. So it would be good
>>>>>> if that behavior keeps working.
>>>>>>
>>>>>> It would also be good if the Linux driver called of_get_mac_address,
>>>>>> so it needs the device tree binding for that. That's why both fixups
>>>>>> are performed, and why the generic function is not used.
>>>>>
>>>>> yes, but u-boot already sets the mac-address/local-mac-address 
>>>>> property
>>>>> in the device tree already in a generic way, see fdt_fixup_ethernet().
>>>>
>>>> I think fdt_fixup_ethernet is not a good choice for us.
>>>> The issue is that it ties Linux DT to device indexes in U-Boot.
>>>> That's a problem if we plug an Eth PCI card in, we would need to
>>>> change Linux DT, which is definitely not desirable.
>>>> We actually do use PCI Eth cards with some of our boards and U-Boot
>>>> does pick those up and indexes shift.
>>>
>>> are you sure? afaik it works by reading the /alias/ethernetN phandles
>>> which gets ethNaddr assigned if you set the FDT_SEQ_MACADDR_FROM_ENV
>>> config option. I've just tried it, here is my linux dts diff
>>>
>>> --- a/arch/arm64/boot/dts/freescale/fsl-ls1028a.dtsi
>>> +++ b/arch/arm64/boot/dts/freescale/fsl-ls1028a.dtsi
>>> @@ -17,6 +17,11 @@
>>>          #address-cells = <2>;
>>>          #size-cells = <2>;
>>>
>>> +       aliases {
>>> +               ethernet0 = &enetc_port0;
>>> +               ethernet1 = &enetc_port1;
>>> +       };
>>> +
>>>          cpus {
>>>                  #address-cells = <1>;
>>>                  #size-cells = <0>;
>>> @@ -761,10 +766,12 @@
>>>                          enetc_port0: ethernet at 0,0 {
>>>                                  compatible = "fsl,enetc";
>>>                                  reg = <0x000000 0 0 0 0>;
>>> +                               local-mac-address = [ 00 00 00 00 00 
>>> 00 ];
>>>                          };
>>>                          enetc_port1: ethernet at 0,1 {
>>>                                  compatible = "fsl,enetc";
>>>                                  reg = <0x000100 0 0 0 0>;
>>> +                               local-mac-address = [ 00 00 00 00 00 
>>> 00 ];
>>>                          };
>>>                          enetc_mdio_pf3: mdio at 0,3 {
>>>                                  compatible = "fsl,enetc-mdio";
>>>
>>> That way the mapping between ethNaddr and the network device can also
>>> be changed by the user by changing the linux device tree.
>>>
>>> also, uboot should respect the /aliases/ethernetN, too.
>>
>> I don't disagree with any of that, but that's not the issue I
>> mentioned.  I meant actual PCI cards being plugged in, I didn't mean
>> having disabled ECAM functions.
>>
>> In your example DT  enetc port 0 is tied to /aliases/ethernet0 and to
>> ethaddr, enetc port 1 is tied to /aliases/ethernet1 and to eth1addr.
>>
>> A LS1028 board with a PCI Eth card plugged in shows this:
>>
>> Net:   e1000: 68:05:ca:66:bf:bd
>>        eth0: e1000#0 [PRIME], eth1: enetc-0, eth2: enetc-2, eth3:
>> swp0, eth4: swp1, eth5: swp2, eth6: swp3
>>
>> In this case eth0 is the e1000 card and it uses ethaddr, enetc port 0
>> is eth1 and uses eth1addr.  The fix-up to /aliases/ethernet0 in your
>> example makes enetc port 0 get the MAC address of the PCI card.  If
>> the PCI card is removed then enetc port 0 ends up being eth0 in U-Boot
>> and and actually use ethaddr, not eth1addr.
> 
> Mh, I've just tried this and it seems that this is even worse. Because
> if you set the ethaddr, uboot will complain about mismatching ethernet
> addresses.

Between ethaddr from eeprom and e1000 ROM address?  Yes, it will complain.

>> To make this work with a PCI card plugged in one would need to change
>> the aliases in Linux DT, which is not a fun thing to do.
>>
>>> BTW what will be the source of the network addresses, the u-boot
>>> environment variables? (which might be set either by the user or some
>>> kind of board specific code).
>>
>> Yes, the environment variables.  As far as I know these come preset
>> from the factory.  The reference boards usually come with stickers
>> too, listing the preset MAC addresses.
> 
> so here is already the first problem, see above. Just to be sure, I
> don't argue against having the environment as the source (actually
> we do import the mac addresses from our eeprom into the environment),
> just to make you aware that there is an underlying problem.

I know, we waste the 1st address from environment if there's a card 
plugged in, and having more cards makes it worse.

> I've tried to use the aliases in u-boot, too. Unfortunately, the
> e1000 will get the first slot, regardless that /aliases/ethernet0 is
> a phandle to enetc-0. I guess that is the actual problem here. Because
> if that would work then everything else would fall into place. (except
> the write_hwaddr()).

I don't think aliases in U-Boot DT help really, these are all PCI 
devices, the card and enetc/ECAM.  They are probed on the PCI bus, not 
from DT.

>>>> Also U-Boot and Linux DTs have to be in sync all the time, if we
>>>> disable one port in U-Boot but not in Linux we would mix up MAC
>>>> addresses.
>>>
>>> I see. But that shouldn't happen with the code above. But are you sure
>>> that this
>>>
>>>> +       uclass_get(UCLASS_ETH, &uc);
>>>> +       uclass_foreach_dev(dev, uc) {
>>>
>>> will work then? in my config (just enetc-0) there is only one eth device
>>>
>>> # dm tree
>>> [snip]
>>>   pci           2  [ + ]   pci_generic_ecam      |-- pcie at 1f0000000
>>>   eth           0  [ + ]   enetc_eth             |   |-- enetc-0
>>>   mdio          0  [ + ]   enetc_mdio            |   |-- emdio-3
>>>   pci_generi    0  [   ]   pci_generic_drv       |   |-- pci_4:0.4
>>>   dsa           0  [   ]   felix-switch          |   |-- felix-switch
>>>   pci_generi    1  [   ]   pci_generic_drv       |   `-- pci_4:1f.0
>>> [snip]
>>
>> Yeah, I guess that's no better either way.  U-Boot wouldn't be able to
>> fix-up addresses for interfaces it didn't know about, yes.  It works
>> if Linux doesn't use some interfaces that U-Boot does, the MAC
>> addresses per port would match between U-Boot and Linux.  That case
>> isn't too useful though, if they differ it's probably the other way
>> around, and U-Boot has fewer interfaces.
>>
>> There is no simple solution to satisfy all cases really.
> 
> mhh just to let you know some cases we have:
>   (1) enetc-0, enetc-1 in u-boot
>       enetc-0, enetc-1, enetc-2 (and switch) in linux
>   (2) enetc-1 in u-boot
>       all enetc's and switch in linux
>   (3a) well the "standard" ones where linux == uboot
>   (3b) "standard one" but just enetc-1, skipping enetc-0
> 

OK, these look reasonable to me, but we need a new solution to make them 
work.

>> Using an index encoded in the DT to identify a given interface isn't
>> OK, in my example there's a PCI card that gets probed first and then
>> all IDs shift up.
> 
> correct, and i guess that is the actual problem here. If it would be
> appended to a pre-defined list (aka /aliases in u-boot) then there wouldn't
> be any shift.

These interfaces have to be probed from the PCI bus.  Using /aliases to 
fix ip dev->seq may work for devices used by U-Boot, it's not so obvious 
how U-Boot would keep track of indexes of devices it doesn't know about 
though.  I hope we can come up with something simpler than that.

>> Fixing up addresses for interfaces U-Boot doesn't know about is also a
>> problem, as the fix-up code should stay clear of eth addresses used by
>> live interfaces, on the assumption that those should continue using
>> them.  Assume the device has two interfaces, first one is disabled in
>> U-Boot but enabled in Linux.  Should it use ethaddr in Linux, or
>> eth1addr?  Ethaddr is associated with 2nd interface in U-Boot and it
>> makes sense to maintain that association in Linux.
> 
> If you say ethNaddr always maps to /alias/ethernetN, you won't have a
> problem, will you? Other than ethXaddr might be unused.
> 
> Also consider the following:
>   - you have consecutive mac addresses
>   - u-boot uses enetc-1, enetc-0 is disabled, it would use ethaddr
>     lets say (its  ..:00)
>   - linux uses enetc-0 and enetc-1, then if it is not shifted,
>     enetc-0 will have (..:01) and enetc-1 stays with (..:00).
> 
> Although it doesn't impact the function, it still looks bad.

It doesn't look neat, but it's workable.

>>>> So as far as using generic fix-up code I'm all for it, but in this
>>>> case we would need some platform specific rules to match Linux DT
>>>> nodes to U-Boot eth addresses.
>>>>
>>>>>> As for the write_hwaddr callback instead of
>>>>>> enetc_set_primary_mac_addr, that is valid but I suppose it is outside
>>>>>> the scope of this patch. That function is related to the runtime MAC
>>>>>> address and not to the MAC passed to Linux.
>>>>>
>>>>> according to the comment in eth-uclass.c this is not for (u-boot) 
>>>>> runtime:
>>>>>
>>>>>   /*
>>>>>    * Devices need to write the hwaddr even if not started so that 
>>>>> Linux
>>>>>    * will have access to the hwaddr that u-boot stored for the device.
>>>>>    * This is accomplished by attempting to probe each device and 
>>>>> calling
>>>>>    * their write_hwaddr() operation.
>>>>>    */
>>>>>
>>>>> and the runtime mac address for u-boot is already set enetc_start().
>>>>>
>>>>> -michael
>>>>
>>>> This is fine, I'll move the IERB code to write_hwaddr.
>>>
>>> This will also only be called if the device is not disabled in u-boot,
>>> from what I see. So it might be better to "fix" this.
>>>
>>> I bet you are not the first/only one which needs this kind of "how do we
>>> propagate the hardware address to linux" where the devices might be 
>>> unused
>>> in u-boot. There must be a better way to do this ;)
>>>
>>>
>>> -michael
>>
>> Yeah, I'm sure others bumped into this before.
>> For sure fdt_fixup_ethernet as defined now can't cover all cases.  It
>> can only rely on the U-Boot interface index which is volatile with
>> plug-in PCI cards.  We could introduce some new index for integrated
>> interfaces that's set by the driver, maybe improve on existing
>> req_seq. In current code req_seq is not too useful, as dev->seq is
>> allocated in order of discovery, so it doesn't help if 2nd found
>> device requests seq 0, that's already in use.  But that could probably
>> be improved.
>>
>> For now I think I'll stick to current code, proprietary as it is and
>> with the limitation that U-Boot has to enable interfaces that Linux
>> uses.
> 
> Please don't take that shortcut because it will be a pain to get it
> cleaned-up and retain backwards compatibility :( Also like explained
> above, it already falls short if you have ethaddr defined.
> 
>> I am open to any better suggestions though.
> 
> see above and like you've described it. append any plugable interfaces
> at the end of the predefined list (with probably empty slots in used
> ethNaddr).
> 
> -michael

All solutions based on the interface index fall short, the index is 
really volatile if PCI cards are involved.

So here's two alternatives I can think of:

- have the index nailed in place for relevant interfaces, including the 
ones U-Boot doesn't even use.  That could be done introducing a static 
range of indexes, which is not used for plug-in cards.  Since this 
should be backward compatible we don't want to mess with IDs in use on 
current platforms, so we would need to move the static IDs up.
Say there's a

	#define ETH_STATIC_INDEX_BASE  0x10

enetc could do something like:

	dev->req_seq = ETH_STATIC_INDEX_BASE + fno.

meaning PF0, 1, 2, 6 get eth indexes 0x10, 0x11, 0x12, 0x16.

The DT fix-up code could use this base when processing the Linux DT, so 
/aliases/ethernet0 gets eth10addr, /aliases/ethernet1 gets eth11addr and 
so on.

Cards like e1000 are left with 0 based index, so they don't interfere 
with this ID range if they are not too many, and MAC addresses between 
U-Boot and Linux are preserved even if U-Boot doesn't use all the 
interfaces.
that, or

- move away for using the index to make this association.
How about having an allocation scheme that associates the device, not 
its probing index with the MAC address.  It could be based on the name, 
like 'enetc-0-addr=..:00' or device topology, bus/function for pci, like 
'eth-pci:4:00.0-addr=..:00'.  These kind of schemes would not depend on 
what is enabled in U-Boot either, but they would require some more 
specific support for matching enetc-0 or pci:4:00.0 to a Linux DT node.

Thoughts, any better ideas?

Thanks!
Alex

[PATCH v2] drivers: net: fsl_enetc: Pass on primary MAC address to Linux

[Priyanka Jain]

>-----Original Message-----
>From: U-Boot <u-boot-bounces@lists.denx.de> On Behalf Of Alex Marginean
>Sent: Tuesday, December 10, 2019 8:26 PM
>To: u-boot at lists.denx.de
>Cc: Joe Hershberger <joe.hershberger@ni.com>; Claudiu Manoil
><claudiu.manoil@nxp.com>; Vladimir Oltean <vladimir.oltean@nxp.com>
>Subject: [PATCH v2] drivers: net: fsl_enetc: Pass on primary MAC address to
>Linux
>
>Passes on the primary address used by u-boot to Linux.  The code does a DT
>fix-up for ENETC PFs and sets the primary MAC address in IERB.  The address
>in IERB is restored on ENETC PCI functions at FLR.
>
>Signed-off-by: Alex Marginean <alexandru.marginean@nxp.com>
>---
Patch applied (after removing extra spaces in description) in u-boot-fsl-qoriq/master
-priyankajain