[3/7] net: smc911x: Properly handle EEPROM MAC address

Message ID 20200611110321.9574-4-andre.przywara@arm.com
State Accepted
Commit 117a52e2387bdfcf7f74c5ec6c4985cdd7c5403e
Headers show
Series
  • arm: Juno board updates and PCIe/SATA enablement
Related show

Commit Message

Andre Przywara June 11, 2020, 11:03 a.m.
When compiled as a DM_ETH driver, the scm911x driver was reading the MAC
address from the optional EEPROM storage, but failed to copy this to the
platdata struct. Since it was also missing a definition of the
read_rom_hwaddr() function, the generic Ethernet code was dismissing
this MAC address, falling back to a random address or denying to start
at all.

Add an implementation of .read_rom_hwaddr, and refactor the function
reading the ROM address to be called by all interested parties.

This fixes MAC address issues when using the driver in DM_ETH "mode".

Signed-off-by: Andre Przywara <andre.przywara at arm.com>
---
 drivers/net/smc911x.c | 60 ++++++++++++++++++++++++++-----------------
 1 file changed, 36 insertions(+), 24 deletions(-)

Comments

Ramon Fried June 11, 2020, 11:10 a.m. | #1
On Thu, Jun 11, 2020 at 2:05 PM Andre Przywara <andre.przywara at arm.com> wrote:
>
> When compiled as a DM_ETH driver, the scm911x driver was reading the MAC
> address from the optional EEPROM storage, but failed to copy this to the
> platdata struct. Since it was also missing a definition of the
> read_rom_hwaddr() function, the generic Ethernet code was dismissing
> this MAC address, falling back to a random address or denying to start
> at all.
>
> Add an implementation of .read_rom_hwaddr, and refactor the function
> reading the ROM address to be called by all interested parties.
>
> This fixes MAC address issues when using the driver in DM_ETH "mode".
>
> Signed-off-by: Andre Przywara <andre.przywara at arm.com>
> ---
>  drivers/net/smc911x.c | 60 ++++++++++++++++++++++++++-----------------
>  1 file changed, 36 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/net/smc911x.c b/drivers/net/smc911x.c
> index 9d2790e561..053ff9f4ff 100644
> --- a/drivers/net/smc911x.c
> +++ b/drivers/net/smc911x.c
> @@ -187,6 +187,26 @@ static void smc911x_handle_mac_address(struct smc911x_priv *priv)
>         printf(DRIVERNAME ": MAC %pM\n", m);
>  }
>
> +static bool smc911x_read_mac_address(struct smc911x_priv *priv)
> +{
> +       u32 addrh, addrl;
> +
> +       /* address is obtained from optional eeprom */
> +       addrh = smc911x_get_mac_csr(priv, ADDRH);
> +       addrl = smc911x_get_mac_csr(priv, ADDRL);
> +       if (addrl == 0xffffffff && addrh == 0x0000ffff)
> +               return false;
> +
> +       priv->enetaddr[0] = addrl;
> +       priv->enetaddr[1] = addrl >>  8;
> +       priv->enetaddr[2] = addrl >> 16;
> +       priv->enetaddr[3] = addrl >> 24;
> +       priv->enetaddr[4] = addrh;
> +       priv->enetaddr[5] = addrh >> 8;
> +
> +       return true;
> +}
> +
>  static int smc911x_eth_phy_read(struct smc911x_priv *priv,
>                                 u8 phy, u8 reg, u16 *val)
>  {
> @@ -471,7 +491,6 @@ static int smc911x_recv(struct eth_device *dev)
>
>  int smc911x_initialize(u8 dev_num, int base_addr)
>  {
> -       unsigned long addrl, addrh;
>         struct smc911x_priv *priv;
>         int ret;
>
> @@ -489,18 +508,8 @@ int smc911x_initialize(u8 dev_num, int base_addr)
>                 goto err_detect;
>         }
>
> -       addrh = smc911x_get_mac_csr(priv, ADDRH);
> -       addrl = smc911x_get_mac_csr(priv, ADDRL);
> -       if (!(addrl == 0xffffffff && addrh == 0x0000ffff)) {
> -               /* address is obtained from optional eeprom */
> -               priv->enetaddr[0] = addrl;
> -               priv->enetaddr[1] = addrl >>  8;
> -               priv->enetaddr[2] = addrl >> 16;
> -               priv->enetaddr[3] = addrl >> 24;
> -               priv->enetaddr[4] = addrh;
> -               priv->enetaddr[5] = addrh >> 8;
> +       if (smc911x_read_mac_address(priv))
>                 memcpy(priv->dev.enetaddr, priv->enetaddr, 6);
> -       }
>
>         priv->dev.init = smc911x_init;
>         priv->dev.halt = smc911x_halt;
> @@ -565,6 +574,19 @@ static int smc911x_recv(struct udevice *dev, int flags, uchar **packetp)
>         return ret ? ret : -EAGAIN;
>  }
>
> +static int smc911x_read_rom_hwaddr(struct udevice *dev)
> +{
> +       struct smc911x_priv *priv = dev_get_priv(dev);
> +       struct eth_pdata *pdata = dev_get_platdata(dev);
> +
> +       if (!smc911x_read_mac_address(priv))
> +               return -ENODEV;
> +
> +       memcpy(pdata->enetaddr, priv->enetaddr, sizeof(pdata->enetaddr));
> +
> +       return 0;
> +}
> +
>  static int smc911x_bind(struct udevice *dev)
>  {
>         return device_set_name(dev, dev->name);
> @@ -573,7 +595,6 @@ static int smc911x_bind(struct udevice *dev)
>  static int smc911x_probe(struct udevice *dev)
>  {
>         struct smc911x_priv *priv = dev_get_priv(dev);
> -       unsigned long addrh, addrl;
>         int ret;
>
>         /* Try to detect chip. Will fail if not present. */
> @@ -581,17 +602,7 @@ static int smc911x_probe(struct udevice *dev)
>         if (ret)
>                 return ret;
>
> -       addrh = smc911x_get_mac_csr(priv, ADDRH);
> -       addrl = smc911x_get_mac_csr(priv, ADDRL);
> -       if (!(addrl == 0xffffffff && addrh == 0x0000ffff)) {
> -               /* address is obtained from optional eeprom */
> -               priv->enetaddr[0] = addrl;
> -               priv->enetaddr[1] = addrl >>  8;
> -               priv->enetaddr[2] = addrl >> 16;
> -               priv->enetaddr[3] = addrl >> 24;
> -               priv->enetaddr[4] = addrh;
> -               priv->enetaddr[5] = addrh >> 8;
> -       }
> +       smc911x_read_rom_hwaddr(dev);
>
>         return 0;
>  }
> @@ -612,6 +623,7 @@ static const struct eth_ops smc911x_ops = {
>         .send   = smc911x_send,
>         .recv   = smc911x_recv,
>         .stop   = smc911x_stop,
> +       .read_rom_hwaddr = smc911x_read_rom_hwaddr,
>  };
>
>  static const struct udevice_id smc911x_ids[] = {
> --
> 2.17.5
>
Reviewed-By: Ramon Fried <rfried.dev at gmail.com>
Linus Walleij June 20, 2020, 8:25 p.m. | #2
On Thu, Jun 11, 2020 at 1:04 PM Andre Przywara <andre.przywara at arm.com> wrote:

> When compiled as a DM_ETH driver, the scm911x driver was reading the MAC
> address from the optional EEPROM storage, but failed to copy this to the
> platdata struct. Since it was also missing a definition of the
> read_rom_hwaddr() function, the generic Ethernet code was dismissing
> this MAC address, falling back to a random address or denying to start
> at all.
>
> Add an implementation of .read_rom_hwaddr, and refactor the function
> reading the ROM address to be called by all interested parties.
>
> This fixes MAC address issues when using the driver in DM_ETH "mode".
>
> Signed-off-by: Andre Przywara <andre.przywara at arm.com>

Oups. That explains why the ethernet addresses were looking
so funny at times.
Reviewed-by: Linus Walleij <linus.walleij at linaro.org>

Yours,
Linus Walleij

Patch

diff --git a/drivers/net/smc911x.c b/drivers/net/smc911x.c
index 9d2790e561..053ff9f4ff 100644
--- a/drivers/net/smc911x.c
+++ b/drivers/net/smc911x.c
@@ -187,6 +187,26 @@  static void smc911x_handle_mac_address(struct smc911x_priv *priv)
 	printf(DRIVERNAME ": MAC %pM\n", m);
 }
 
+static bool smc911x_read_mac_address(struct smc911x_priv *priv)
+{
+	u32 addrh, addrl;
+
+	/* address is obtained from optional eeprom */
+	addrh = smc911x_get_mac_csr(priv, ADDRH);
+	addrl = smc911x_get_mac_csr(priv, ADDRL);
+	if (addrl == 0xffffffff && addrh == 0x0000ffff)
+		return false;
+
+	priv->enetaddr[0] = addrl;
+	priv->enetaddr[1] = addrl >>  8;
+	priv->enetaddr[2] = addrl >> 16;
+	priv->enetaddr[3] = addrl >> 24;
+	priv->enetaddr[4] = addrh;
+	priv->enetaddr[5] = addrh >> 8;
+
+	return true;
+}
+
 static int smc911x_eth_phy_read(struct smc911x_priv *priv,
 				u8 phy, u8 reg, u16 *val)
 {
@@ -471,7 +491,6 @@  static int smc911x_recv(struct eth_device *dev)
 
 int smc911x_initialize(u8 dev_num, int base_addr)
 {
-	unsigned long addrl, addrh;
 	struct smc911x_priv *priv;
 	int ret;
 
@@ -489,18 +508,8 @@  int smc911x_initialize(u8 dev_num, int base_addr)
 		goto err_detect;
 	}
 
-	addrh = smc911x_get_mac_csr(priv, ADDRH);
-	addrl = smc911x_get_mac_csr(priv, ADDRL);
-	if (!(addrl == 0xffffffff && addrh == 0x0000ffff)) {
-		/* address is obtained from optional eeprom */
-		priv->enetaddr[0] = addrl;
-		priv->enetaddr[1] = addrl >>  8;
-		priv->enetaddr[2] = addrl >> 16;
-		priv->enetaddr[3] = addrl >> 24;
-		priv->enetaddr[4] = addrh;
-		priv->enetaddr[5] = addrh >> 8;
+	if (smc911x_read_mac_address(priv))
 		memcpy(priv->dev.enetaddr, priv->enetaddr, 6);
-	}
 
 	priv->dev.init = smc911x_init;
 	priv->dev.halt = smc911x_halt;
@@ -565,6 +574,19 @@  static int smc911x_recv(struct udevice *dev, int flags, uchar **packetp)
 	return ret ? ret : -EAGAIN;
 }
 
+static int smc911x_read_rom_hwaddr(struct udevice *dev)
+{
+	struct smc911x_priv *priv = dev_get_priv(dev);
+	struct eth_pdata *pdata = dev_get_platdata(dev);
+
+	if (!smc911x_read_mac_address(priv))
+		return -ENODEV;
+
+	memcpy(pdata->enetaddr, priv->enetaddr, sizeof(pdata->enetaddr));
+
+	return 0;
+}
+
 static int smc911x_bind(struct udevice *dev)
 {
 	return device_set_name(dev, dev->name);
@@ -573,7 +595,6 @@  static int smc911x_bind(struct udevice *dev)
 static int smc911x_probe(struct udevice *dev)
 {
 	struct smc911x_priv *priv = dev_get_priv(dev);
-	unsigned long addrh, addrl;
 	int ret;
 
 	/* Try to detect chip. Will fail if not present. */
@@ -581,17 +602,7 @@  static int smc911x_probe(struct udevice *dev)
 	if (ret)
 		return ret;
 
-	addrh = smc911x_get_mac_csr(priv, ADDRH);
-	addrl = smc911x_get_mac_csr(priv, ADDRL);
-	if (!(addrl == 0xffffffff && addrh == 0x0000ffff)) {
-		/* address is obtained from optional eeprom */
-		priv->enetaddr[0] = addrl;
-		priv->enetaddr[1] = addrl >>  8;
-		priv->enetaddr[2] = addrl >> 16;
-		priv->enetaddr[3] = addrl >> 24;
-		priv->enetaddr[4] = addrh;
-		priv->enetaddr[5] = addrh >> 8;
-	}
+	smc911x_read_rom_hwaddr(dev);
 
 	return 0;
 }
@@ -612,6 +623,7 @@  static const struct eth_ops smc911x_ops = {
 	.send	= smc911x_send,
 	.recv	= smc911x_recv,
 	.stop	= smc911x_stop,
+	.read_rom_hwaddr = smc911x_read_rom_hwaddr,
 };
 
 static const struct udevice_id smc911x_ids[] = {