FS#1702 - ath79: ag71xx (ar7240/ar934x builtin) switch does not detect link status change on all ports #6681

openwrt-bot · 2018-07-28T11:59:50Z

cshoredaniel:

Supply the following if possible:

Device problem occurs on
Software versions of OpenWrt/LEDE release, packages, etc.
Steps to reproduce

I'm adding ath79 arch support for some devices, and discovered the following:

At least on Atheros 9341 SoC with phy4-mii-enable enabled (i.e. fifth port acts as a separate ethX device (rather than being configured as a VLAN)), the link status of the the switch (i.e. the ports acting as a 'normal' ar7240 switch) is only detected on a single port of the switch and all other ports are ignored for cable plug/unplug.

This seems to be because the default is to use a 'Generic PHY' device and the SoC only responds to a single port on the switch.

I have a hack (not submitted as a patch to the list yet because to be done properly I really need to create an ag71xx-phy device (which means also ag71xx-mdio, etc for the drivers to work properly) to replace certain functions (which you'll see below) in the 'Generic PHY' so that link detection works correctly) which I include here for reference, which gets this working for me.

.../net/ethernet/atheros/ag71xx/ag71xx.h | 5 +
.../ethernet/atheros/ag71xx/ag71xx_ar7240.c | 180 ++++++++++++++++++
.../net/ethernet/atheros/ag71xx/ag71xx_main.c | 16 ++
3 files changed, 201 insertions(+)

diff --git a/target/linux/ath79/files/drivers/net/ethernet/atheros/ag71xx/ag71xx.h b/target/linux/ath79/files/drivers/net/ethernet/atheros/ag71xx/ag71xx.h
index 22b22522a3..2bebec4ac5 100644
--- a/target/linux/ath79/files/drivers/net/ethernet/atheros/ag71xx/ag71xx.h
+++ b/target/linux/ath79/files/drivers/net/ethernet/atheros/ag71xx/ag71xx.h
@@ -179,6 +179,7 @@ struct ag71xx {
struct phy_device *phy_dev;
void *phy_priv;
int phy_if_mode;

bool is_switch;

unsigned int link;
unsigned int speed;
@@ -450,4 +451,8 @@ int ag71xx_mdio_mii_write(struct mii_bus *bus, int addr, int reg, u16 val);
int ar7240sw_phy_read(struct mii_bus *mii, int addr, int reg);
int ar7240sw_phy_write(struct mii_bus *mii, int addr, int reg, u16 val);

+int ar7240sw_update_link(struct phy_device *phydev);
+int ar7240sw_read_status(struct phy_device *phydev);
+int ar7240sw_aneg_done(struct phy_device phydev);
+
#endif / _AG71XX_H */
diff --git a/target/linux/ath79/files/drivers/net/ethernet/atheros/ag71xx/ag71xx_ar7240.c b/target/linux/ath79/files/drivers/net/ethernet/atheros/ag71xx/ag71xx_ar7240.c
index 52c0f964cf..12c62660b4 100644
--- a/target/linux/ath79/files/drivers/net/ethernet/atheros/ag71xx/ag71xx_ar7240.c
+++ b/target/linux/ath79/files/drivers/net/ethernet/atheros/ag71xx/ag71xx_ar7240.c
@@ -194,6 +194,7 @@
#define AR7240_PORT_CPU 0
#define AR7240_NUM_PORTS 6
#define AR7240_NUM_PHYS 5
+#define AR7240_PHY_POLL_MASK 0x3e

#define AR7240_PHY_ID1 0x004d
#define AR7240_PHY_ID2 0xd041
@@ -294,6 +295,7 @@ struct ar7240sw {
struct ag71xx_switch_platform_data *swdata;
struct switch_dev swdev;
int num_ports;

u32 phy_poll_mask;
u8 ver;
bool vlan;
u16 vlan_id[AR7240_MAX_VLANS];
@@ -1245,10 +1247,20 @@ ar7240_probe(struct ag71xx *ag, struct device_node *np)
as->ver = (ctrl >> AR7240_MASK_CTRL_VERSION_S) &
AR7240_MASK_CTRL_VERSION_M;
if (of_property_read_u32(np, "phy-poll-mask", &as->phy_poll_mask))

  as->phy_poll_mask = AR7240_PHY_POLL_MASK;

else

  as->phy_poll_mask &= AR7240_PHY_POLL_MASK;

pr_info("phy-poll-mask %02x", as->phy_poll_mask);
ag->is_switch = false;
if (sw_is_ar7240(as)) {
swdev->name = "AR7240/AR9330 built-in switch";
swdev->ports = AR7240_NUM_PORTS - 1;
```
  ag->is_switch = true;
```
} else if (sw_is_ar934x(as)) {

  ag->is_switch = true;
  swdev->name = "AR934X built-in switch";

  if (ag->phy_if_mode == PHY_INTERFACE_MODE_GMII) {

@@ -1267,6 +1279,8 @@ ar7240_probe(struct ag71xx *ag, struct device_node *np)
ar7240sw_reg_set(mii, AR934X_REG_OPER_MODE1,
AR934X_REG_OPER_MODE1_PHY4_MII_EN);
swdev->ports = AR7240_NUM_PORTS - 1;

  	/* Don't include port 5 as it's now a separate PHY */

  	as->phy_poll_mask &= 0x1f;
  } else {
  	swdev->ports = AR7240_NUM_PORTS;
  }

@@ -1305,6 +1319,9 @@ void ag71xx_ar7240_start(struct ag71xx *ag)
if (!as)
return;

if (ag->phy_dev)
```
  ag->phy_dev->priv = ag->phy_priv;
```
ar7240sw_reset(as);

ar7240_set_addr(as, ag->dev->dev_addr);
@@ -1322,6 +1339,9 @@ int ag71xx_ar7240_init(struct ag71xx *ag, struct device_node *np)
ag->phy_priv = as;
ar7240sw_reset(as);
if (ag->phy_dev)
```
  ag->phy_dev->priv = ag->phy_priv;
```
rwlock_init(&as->stats_lock);

return 0;
@@ -1330,6 +1350,7 @@ int ag71xx_ar7240_init(struct ag71xx *ag, struct device_node *np)
void ag71xx_ar7240_cleanup(struct ag71xx *ag)
{
struct ar7240sw *as = ag->phy_priv;
ag->phy_dev->priv = NULL;

if (!as)
return;
@@ -1338,3 +1359,162 @@ void ag71xx_ar7240_cleanup(struct ag71xx *ag)
kfree(as);
ag->phy_priv = NULL;
}

+/* Copy of genphy_update_link without the call to driver hook for update_link

- else we get an infinite loop; from kernel drivers/net/phy/phy_device.c
*/
+int ar7240sw_copy_genphy_update_link(struct phy_device *phydev) {
int status;
/* Do a fake read */
status = phy_read(phydev, MII_BMSR);
if (status < 0)
```
  return status;
```
/* Read link and autonegotiation status */
status = phy_read(phydev, MII_BMSR);
if (status < 0)
```
  return status;
```
if ((status & BMSR_LSTATUS) == 0)
```
  phydev->link = 0;
```
else
```
  phydev->link = 1;
```
return 0;
+}

+int ar7240sw_update_link (struct phy_device *phydev) {

struct ar7240sw *as = NULL;
struct switch_dev *swdev = NULL;
int i;
int status = 0;
struct switch_port_link link;
if (!phydev->priv) {

  return ar7240sw_copy_genphy_update_link(phydev);

}
as = phydev->priv;
swdev = &(as->swdev);
if (!swdev) {

  return ar7240sw_copy_genphy_update_link(phydev);

}
/* Don't touch link state until we actully know the state */
for (i = 0; i < swdev->ports; i++) {
```
  if (i == swdev->cpu_port)
```
```
  	continue;
```
```
  if (!(as->phy_poll_mask & BIT(i))) {
```
```
  	continue;
```
```
  }
```

  status = ar7240_get_port_link(swdev, i, &link);

```
  if (status < 0) {
```
```
  	return status;
```
```
  }
```

  /* If any non-masked port is active, consider

```
   * device to be 'up'
```
```
   */
```
```
  if (link.link) {
```
```
  	phydev->link = 1;
```
```
  	return 0;
```
```
  }
```
}
/* otherwise, device is 'down' */
phydev->link = 0;
return 0;
+}

+int ar7240sw_read_status(struct phy_device *phydev) {

struct ar7240sw *as = NULL;
struct switch_dev *swdev = NULL;
int status;
int i;
struct switch_port_link link;
if (!phydev->priv)
```
  return genphy_read_status(phydev);
```
status = ar7240sw_update_link(phydev);
if (status)
```
  return status;
```
as = phydev->priv;
swdev = &(as->swdev);
if (!swdev)
```
  return genphy_read_status(phydev);
```
/* Set link status for base device */
if (phydev->link) {

  /* While down set link speed duplex lower, but valid */

```
  phydev->speed = SPEED_10;
```
```
  phydev->duplex = DUPLEX_HALF;
```
```
  for (i = 0; i < swdev->ports; i++) {
```

  	/* We report the speed and duplex as the

```
  	 * highest any non-masked port has
```
```
  	 * negotiated.
```
```
  	 */
```
```
  	if (i == swdev->cpu_port)
```
```
  		continue;
```

  	if (!(as->phy_poll_mask & BIT(i))) {

```
  		continue;
```
```
  	}
```

  	status = ar7240_get_port_link(swdev, i, &link);

```
  	if (status < 0) {
```
```
  		return status;
```
```
  	}
```
```
  	switch(link.speed) {
```
```
  	case SPEED_100:
```
```
  		if (phydev->speed != SPEED_1000)
```
```
  			phydev->speed = SPEED_100;
```
```
  		break;
```
```
  	case SPEED_1000:
```
```
  			phydev->speed = SPEED_1000;
```
```
  		break;
```
```
  	default:
```

  		/* We never reduce detected speed */

```
  		break;
```
```
  	}
```
```
  	switch(link.duplex) {
```
```
  	case DUPLEX_FULL:
```
```
  		phydev->duplex = DUPLEX_FULL;
```
```
  		break;
```
```
  	default:
```

  		/* We never reduce detected duplex */

```
  		break;
```
```
  	}
```
```
  }
```
```
  phydev->pause = 0;
```
```
  phydev->asym_pause = 0;
```
}
return 0;
+}

+int ar7240sw_aneg_done (struct phy_device *phydev) {

int status;
if (!phydev->priv) {
```
  return genphy_aneg_done(phydev);
```
}
status = ar7240sw_read_status(phydev);
if (status)
```
  return status;
```
/* If we were able to get a status we've
- autonegotiated on the port(s) already.
*/
return 1;
+}
diff --git a/target/linux/ath79/files/drivers/net/ethernet/atheros/ag71xx/ag71xx_main.c b/target/linux/ath79/files/drivers/net/ethernet/atheros/ag71xx/ag71xx_main.c
index 635c3b9127..f33e148e2e 100644
--- a/target/linux/ath79/files/drivers/net/ethernet/atheros/ag71xx/ag71xx_main.c
+++ b/target/linux/ath79/files/drivers/net/ethernet/atheros/ag71xx/ag71xx_main.c
@@ -1509,6 +1509,22 @@ static int ag71xx_probe(struct platform_device *pdev)

platform_set_drvdata(pdev, dev);
ag->phy_dev->priv = NULL;
if (ag->phy_priv)
```
  if (ag->is_switch)
```
```
  	ag->phy_dev->priv = ag->phy_priv;
```
/* NB: This replaces update_link for all instances of the
- the driver (so all 'Generic PHY' instances are affected)
*/
if (ag->phy_dev->drv) {

  ag->phy_dev->drv->update_link = ar7240sw_update_link;

  ag->phy_dev->drv->read_status = ar7240sw_read_status;

  ag->phy_dev->drv->aneg_done = ar7240sw_aneg_done;

}
err = register_netdev(dev);
if (err) {
dev_err(&pdev->dev, "unable to register net device\n");

The text was updated successfully, but these errors were encountered:

openwrt-bot · 2018-07-28T16:52:45Z

mkresin:

Please give #1146 a try. It is a complete overhaul of the ag71xx/buildin siwtch driver. Might be your issue is fixed already. Haven't had a look at your actual code yet, but the PR would be the best place to discuss your issue.

openwrt-bot · 2018-07-29T03:32:06Z

cshoredaniel:

Thanks, am looking at it now.

openwrt-bot · 2018-07-29T05:27:19Z

cshoredaniel:

Apparently I was overcomplicating things by not using a fixed link on the switch; while that worked for other ath79 devices I converted it appears not be be 'The ag71xxx Way'. So I guess having non-fixed link on ag71xx switches is actually a feature request.

Now to deal with the problem that I have a peculiar device that uses a reversed eth0 and eth1 compared to what is normal for ag71xx devices (although I did find at least one other device using the same type of board config, but it has not yet been converted).

It seems trying reverse probe order is what got me in trouble, so rather than that I'm going to try renaming the devices early in preinit using ip link set ethX name ethY (with an intermediate on one of them since I'm swapping eth0 and eth1).

after confirming that works with a fixed link, I'll come back and look at this patch (which might still be useful as a new feature).

This device does still have some particular that require the new PR, but the probe order is really what did me in. I'll mention it on the PR, but I think it's better to rename the devices (since AFAIK you don't get to name ethernet devices in the DT, rather it happens by probe order (although it would be nice if hotplug would allow to name the device, but I couldn't see any ethernet/netdev add events when I tried debugging messages from hotplug), probably because they don't get device nodes.

openwrt-bot · 2018-07-29T09:26:23Z

cshoredaniel:

https://github.com/981213 has said they would prefer to not add more functions to the built-in switch but rather move built-in support into ar8216 switch and move it out of ag71xx driver.

openwrt-bot · 2018-08-01T05:25:56Z

cshoredaniel:

@blogic has also expressed that it'd be better if the built-in switch were in ar8216 driver. Therefore should close this bug.

openwrt-bot closed this as completed Aug 1, 2018

chilinkiot mentioned this issue Apr 10, 2023

ath79: [openwrt-22.03 & openwrt-1907] PC machine will not update IP address when Lan IP address changed #12364

Open

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FS#1702 - ath79: ag71xx (ar7240/ar934x builtin) switch does not detect link status change on all ports #6681

FS#1702 - ath79: ag71xx (ar7240/ar934x builtin) switch does not detect link status change on all ports #6681

openwrt-bot commented Jul 28, 2018

openwrt-bot commented Jul 28, 2018

openwrt-bot commented Jul 29, 2018

openwrt-bot commented Jul 29, 2018

openwrt-bot commented Jul 29, 2018

openwrt-bot commented Aug 1, 2018

FS#1702 - ath79: ag71xx (ar7240/ar934x builtin) switch does not detect link status change on all ports #6681

FS#1702 - ath79: ag71xx (ar7240/ar934x builtin) switch does not detect link status change on all ports #6681

Comments

openwrt-bot commented Jul 28, 2018

openwrt-bot commented Jul 28, 2018

openwrt-bot commented Jul 29, 2018

openwrt-bot commented Jul 29, 2018

openwrt-bot commented Jul 29, 2018

openwrt-bot commented Aug 1, 2018