Skip to contentSkip to conversation

net: Add ag7xxx driver for Atheros MIPS

PATCH New Marek Vasut 3 participants 4 messages May 24, 2016 – Jun 9, 2017 Network drivers

lore
Share & tools
lore

Marek Vasut

author
#0 lore raw

PATCH 1/1 net: Add ag7xxx driver for Atheros MIPS

Add ethernet driver for the AR933x and AR934x Atheros MIPS machines. The driver could be easily extended to other WiSoCs.

Signed-off-by Marek Vasut <[email protected]>
Cc Daniel Schwierzeck <[email protected]>
Cc Joe Hershberger <[email protected]>
Cc Wills Wang <[email protected]>

---
V2: - Drop the printf() in case malloc fails, it's pointless to try
and print something if we cannot allocate memory, since printf also allocates memory.
V3: - Replace magic 0x11 with MII_MIPSCR register

---
 drivers/net/Kconfig  |   9 +
 drivers/net/Makefile |   1 +
 drivers/net/ag7xxx.c | 980 +++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 990 insertions(+)
 create mode 100644 drivers/net/ag7xxx.c
drivers/net/Kconfig +9 -0
index 91b7690..00f151f 100644
@@ -46,6 +46,15 @@ menuconfig NETDEVICES
 
 if NETDEVICES
 
+config AG7XXX
+	bool "Atheros AG7xxx Ethernet MAC support"
+	depends on DM_ETH
+	select PHYLIB
+	help
+	  This driver supports the Atheros AG7xxx Ethernet MAC. This MAC is
+	  present in the Atheros AR7xxx, AR9xxx and QCA9xxx MIPS chips.
+
+
 config ALTERA_TSE
 	bool "Altera Triple-Speed Ethernet MAC support"
 	depends on DM_ETH
drivers/net/Makefile +1 -0
index fbedd04..55d56a0 100644
@@ -7,6 +7,7 @@
 
 obj-$(CONFIG_PPC4xx_EMAC) += 4xx_enet.o
 obj-$(CONFIG_ALTERA_TSE) += altera_tse.o
+obj-$(CONFIG_AG7XXX) += ag7xxx.o
 obj-$(CONFIG_ARMADA100_FEC) += armada100_fec.o
 obj-$(CONFIG_DRIVER_AT91EMAC) += at91_emac.o
 obj-$(CONFIG_DRIVER_AX88180) += ax88180.o
drivers/net/ag7xxx.c new file +980 -0

980 lines not on this page · raw message

Signature · 1 line
2.7.0
#1 lore raw

PATCH 1/1 net: Add ag7xxx driver for Atheros MIPS

Am 24.05.2016 um 23:29 schrieb Marek Vasut:

Quoted 12 lines

Add ethernet driver for the AR933x and AR934x Atheros MIPS machines. The driver could be easily extended to other WiSoCs.

Signed-off-by Marek Vasut <[email protected]>
Cc Daniel Schwierzeck <[email protected]>
Cc Joe Hershberger <[email protected]>
Cc Wills Wang <[email protected]>

---
V2: - Drop the printf() in case malloc fails, it's pointless to try
and print something if we cannot allocate memory, since printf also allocates memory.
V3: - Replace magic 0x11 with MII_MIPSCR register

Quoted 7 lines
---
 drivers/net/Kconfig  |   9 +
 drivers/net/Makefile |   1 +
 drivers/net/ag7xxx.c | 980 +++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 990 insertions(+)
 create mode 100644 drivers/net/ag7xxx.c

applied to u-boot-mips, thanks.

Signature · 9 lines
- Daniel

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160530/97cad8f4/attachment.sig>
#2 lore raw

PATCH 1/1 net: Add ag7xxx driver for Atheros MIPS

In reply to Marek Vasut

Hi Marek,

I was looking at something else and noticed what looks like an issue with this code you submitted.

Quoted 7 lines · Marek Vasut

Add ethernet driver for the AR933x and AR934x Atheros MIPS machines. The driver could be easily extended to other WiSoCs.

Signed-off-by Marek Vasut <[email protected]>
Cc Daniel Schwierzeck <[email protected]>
Cc Joe Hershberger <[email protected]>
Cc Wills Wang <[email protected]>
Quoted 6 lines

---
V2: - Drop the printf() in case malloc fails, it's pointless to try
and print something if we cannot allocate memory, since printf also allocates memory.
V3: - Replace magic 0x11 with MII_MIPSCR register
---

[...] SNIP

Quoted 1 line

+static u16 ag7xxx_mdio_rw(struct mii_dev *bus, int addr, int reg, u32 val)

Returns a u16

Quoted 9 lines
drivers/net/ag7xxx.c new file +9 -0
+{
+       u32 data;
+
+       /* Dummy read followed by PHY read/write command. */
+       ag7xxx_switch_reg_read(bus, 0x98, &data);
+       data = val | (reg << 16) | (addr << 21) | BIT(30) | BIT(31);
+       ag7xxx_switch_reg_write(bus, 0x98, data);
+
+       /* Wait for operation to finish */

drivers/net/ag7xxx.c line 291

Quoted 10 lines
drivers/net/ag7xxx.c new file +10 -0
+       do {
+               ag7xxx_switch_reg_read(bus, 0x98, &data);
+       } while (data & BIT(31));
+
+       return data & 0xffff;
+}
+
+static int ag7xxx_mdio_read(struct mii_dev *bus, int addr, int devad, int reg)
+{
+       return ag7xxx_mdio_rw(bus, addr, reg, BIT(27));

Directly returns said u16 as an int.

drivers/net/ag7xxx.c line 299

Quoted 8 lines
drivers/net/ag7xxx.c new file +8 -0
+}
+
+static int ag7xxx_mdio_write(struct mii_dev *bus, int addr, int devad, int reg,
+                            u16 val)
+{
+       ag7xxx_mdio_rw(bus, addr, reg, val);
+       return 0;
+}

[...] SNIP

Quoted 13 lines
drivers/net/ag7xxx.c new file +13 -0
+static int ag933x_phy_setup_common(struct udevice *dev)
+{
+       struct ar7xxx_eth_priv *priv = dev_get_priv(dev);
+       int i, ret, phymax;
+
+       if (priv->model == AG7XXX_MODEL_AG933X)
+               phymax = 4;
+       else if (priv->model == AG7XXX_MODEL_AG934X)
+               phymax = 5;
+       else
+               return -EINVAL;
+
+       if (priv->interface == PHY_INTERFACE_MODE_RMII) {

drivers/net/ag7xxx.c line 726

Quoted 10 lines
drivers/net/ag7xxx.c new file +10 -0
+               ret = ag933x_phy_setup_reset_set(dev, phymax);
+               if (ret)
+                       return ret;
+
+               ret = ag933x_phy_setup_reset_fin(dev, phymax);
+               if (ret)
+                       return ret;
+
+               /* Read out link status */
+               ret = ag7xxx_mdio_read(priv->bus, phymax, 0, MII_MIPSCR);

Read the link status, which can never be negative.

Another issue: Is MII_MIPSCR really the register name? It's not better than "17" - constants should mean something, just just be a random name with the right value.

drivers/net/ag7xxx.c line 215

Quoted 4 lines
drivers/net/ag7xxx.c new file +4 -0
+               if (ret < 0)
+                       return ret;
+
+               return 0;

Return 0 unconditionally. Presumably you want to actually check the link status to be something specific if you bother to read it out.

Quoted 8 lines
drivers/net/ag7xxx.c new file +8 -0
+       }
+
+       /* Switch ports */
+       for (i = 0; i < phymax; i++) {
+               ret = ag933x_phy_setup_reset_set(dev, i);
+               if (ret)
+                       return ret;
+       }

drivers/net/ag7xxx.c line 748

Quoted 10 lines
drivers/net/ag7xxx.c new file +10 -0
+
+       for (i = 0; i < phymax; i++) {
+               ret = ag933x_phy_setup_reset_fin(dev, i);
+               if (ret)
+                       return ret;
+       }
+
+       for (i = 0; i < phymax; i++) {
+               /* Read out link status */
+               ret = ag7xxx_mdio_read(priv->bus, i, 0, MII_MIPSCR);

Same thing here.

Quoted 6 lines
drivers/net/ag7xxx.c new file +6 -0
+               if (ret < 0)
+                       return ret;
+       }
+
+       return 0;
+}
author
#3 lore raw

PATCH 1/1 net: Add ag7xxx driver for Atheros MIPS

Quoted 1 line · Joe Hershberger

Hi Marek,

Hi!

Quoted 74 lines

I was looking at something else and noticed what looks like an issue with this code you submitted.

Quoted 7 lines · Marek Vasut

Add ethernet driver for the AR933x and AR934x Atheros MIPS machines. The driver could be easily extended to other WiSoCs.

Signed-off-by Marek Vasut <[email protected]>
Cc Daniel Schwierzeck <[email protected]>
Cc Joe Hershberger <[email protected]>
Cc Wills Wang <[email protected]>
Quoted 6 lines

---
V2: - Drop the printf() in case malloc fails, it's pointless to try
and print something if we cannot allocate memory, since printf also allocates memory.
V3: - Replace magic 0x11 with MII_MIPSCR register
---

[...] SNIP

Quoted 1 line

+static u16 ag7xxx_mdio_rw(struct mii_dev *bus, int addr, int reg, u32 val)

Returns a u16

Quoted 9 lines
drivers/net/ag7xxx.c new file +9 -0
+{
+       u32 data;
+
+       /* Dummy read followed by PHY read/write command. */
+       ag7xxx_switch_reg_read(bus, 0x98, &data);
+       data = val | (reg << 16) | (addr << 21) | BIT(30) | BIT(31);
+       ag7xxx_switch_reg_write(bus, 0x98, data);
+
+       /* Wait for operation to finish */

drivers/net/ag7xxx.c line 291

Quoted 10 lines
drivers/net/ag7xxx.c new file +10 -0
+       do {
+               ag7xxx_switch_reg_read(bus, 0x98, &data);
+       } while (data & BIT(31));
+
+       return data & 0xffff;
+}
+
+static int ag7xxx_mdio_read(struct mii_dev *bus, int addr, int devad, int reg)
+{
+       return ag7xxx_mdio_rw(bus, addr, reg, BIT(27));

Directly returns said u16 as an int.

drivers/net/ag7xxx.c line 299

Quoted 8 lines
drivers/net/ag7xxx.c new file +8 -0
+}
+
+static int ag7xxx_mdio_write(struct mii_dev *bus, int addr, int devad, int reg,
+                            u16 val)
+{
+       ag7xxx_mdio_rw(bus, addr, reg, val);
+       return 0;
+}

[...] SNIP

Quoted 17 lines
drivers/net/ag7xxx.c new file +17 -0
+static int ag933x_phy_setup_common(struct udevice *dev)
+{
+       struct ar7xxx_eth_priv *priv = dev_get_priv(dev);
+       int i, ret, phymax;
+
+       if (priv->model == AG7XXX_MODEL_AG933X)
+               phymax = 4;
+       else if (priv->model == AG7XXX_MODEL_AG934X)
+               phymax = 5;
+       else
+               return -EINVAL;
+
+       if (priv->interface == PHY_INTERFACE_MODE_RMII) {
+               ret = ag933x_phy_setup_reset_set(dev, phymax);
+               if (ret)
+                       return ret;
+
Quoted 8 lines

drivers/net/ag7xxx.c line 726

Quoted 6 lines
drivers/net/ag7xxx.c new file +6 -0
+               ret = ag933x_phy_setup_reset_fin(dev, phymax);
+               if (ret)
+                       return ret;
+
+               /* Read out link status */
+               ret = ag7xxx_mdio_read(priv->bus, phymax, 0, MII_MIPSCR);

Read the link status, which can never be negative.

Can you send a patch for this ?

Quoted 3 lines

Another issue: Is MII_MIPSCR really the register name? It's not better than "17" - constants should mean something, just just be a random name with the right value.

Can you check the AR9331 or AR9344 datasheet ? It should be there, although they tend to be cryptic.

Quoted 7 lines

drivers/net/ag7xxx.c line 215

Quoted 4 lines
drivers/net/ag7xxx.c new file +4 -0
+               if (ret < 0)
+                       return ret;
+
+               return 0;

Return 0 unconditionally. Presumably you want to actually check the link status to be something specific if you bother to read it out.

Actually, I think this is only for the switch ports, so we don't care about the link status.

Quoted 27 lines
Quoted 8 lines
drivers/net/ag7xxx.c new file +8 -0
+       }
+
+       /* Switch ports */
+       for (i = 0; i < phymax; i++) {
+               ret = ag933x_phy_setup_reset_set(dev, i);
+               if (ret)
+                       return ret;
+       }

drivers/net/ag7xxx.c line 748

Quoted 10 lines
drivers/net/ag7xxx.c new file +10 -0
+
+       for (i = 0; i < phymax; i++) {
+               ret = ag933x_phy_setup_reset_fin(dev, i);
+               if (ret)
+                       return ret;
+       }
+
+       for (i = 0; i < phymax; i++) {
+               /* Read out link status */
+               ret = ag7xxx_mdio_read(priv->bus, i, 0, MII_MIPSCR);

Same thing here.

Quoted 6 lines
drivers/net/ag7xxx.c new file +6 -0
+               if (ret < 0)
+                       return ret;
+       }
+
+       return 0;
+}
Signature · 2 lines
Best regards,
Marek Vasut