[PATCH 0/2] Indirect phy access for mv88e6171

These two patches allow the mv88e6171 driver to access the port phys
using indirect addressing. Depending on pin strapping, the switch
either uses a single address on the host MDIO bus, requiring the port
phys are accessed indirectly, or the switch uses a number of addresses
on the host bus and the phys can be directly accessed.

The 370RD, the first supported platform to use the 6171 uses multiple
addresses, so this indirect mode was not required. However the
WRT1900AC has the switch configured to use a single address, and so
indirect access is needed.

The mv88e6352 already has all the needed code. Refactor it into the
shared mv88e6xxx and then use it in the mv88e6171 driver.

Tested on the 370RD and WRT1900AC.

It would be good if Guenter Roeck could test on his platform to ensure
i've not broken anything for the mv88e6352.

Andrew Lunn (2):
  net: dsa: mv88e6352: Refactor shareable code
  net: dsa: mv88e6171: Enable access to phys via internal mdio bus

 drivers/net/dsa/mv88e6171.c | 18 ++++++-----
 drivers/net/dsa/mv88e6352.c | 77 +++++++--------------------------------------
 drivers/net/dsa/mv88e6xxx.c | 53 +++++++++++++++++++++++++++++++
 drivers/net/dsa/mv88e6xxx.h |  6 ++++
 4 files changed, 81 insertions(+), 73 deletions(-)

-- 
2.1.4

The mv88e6352 allows access to the port phys via an internal mdio bus
which is accessed using registers in the GLOBAL 2 range. The mv88e6171
and probably other devices use the same mechanism. Move this code into
the shared mv88e6xxx.c library.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/dsa/mv88e6352.c | 77 +++++++--------------------------------------
 drivers/net/dsa/mv88e6xxx.c | 53 +++++++++++++++++++++++++++++++
 drivers/net/dsa/mv88e6xxx.h |  6 ++++
 3 files changed, 71 insertions(+), 65 deletions(-)

diff --git a/drivers/net/dsa/mv88e6352.c b/drivers/net/dsa/mv88e6352.c
index e13adc7b3dda..1ebd8f96072a 100644
--- a/drivers/net/dsa/mv88e6352.c
+++ b/drivers/net/dsa/mv88e6352.c

@@ -22,59 +22,6 @@
 #include <net/dsa.h>
 #include "mv88e6xxx.h"
 
-static int mv88e6352_wait(struct dsa_switch *ds, int reg, int offset, u16 mask)
-{
-	unsigned long timeout = jiffies + HZ / 10;
-
-	while (time_before(jiffies, timeout)) {
-		int ret;
-
-		ret = REG_READ(reg, offset);
-		if (!(ret & mask))
-			return 0;
-
-		usleep_range(1000, 2000);
-	}
-	return -ETIMEDOUT;
-}
-
-static inline int mv88e6352_phy_wait(struct dsa_switch *ds)
-{
-	return mv88e6352_wait(ds, REG_GLOBAL2, 0x18, 0x8000);
-}
-
-static inline int mv88e6352_eeprom_load_wait(struct dsa_switch *ds)
-{
-	return mv88e6352_wait(ds, REG_GLOBAL2, 0x14, 0x0800);
-}
-
-static inline int mv88e6352_eeprom_busy_wait(struct dsa_switch *ds)
-{
-	return mv88e6352_wait(ds, REG_GLOBAL2, 0x14, 0x8000);
-}
-
-static int __mv88e6352_phy_read(struct dsa_switch *ds, int addr, int regnum)
-{
-	int ret;
-
-	REG_WRITE(REG_GLOBAL2, 0x18, 0x9800 | (addr << 5) | regnum);
-
-	ret = mv88e6352_phy_wait(ds);
-	if (ret < 0)
-		return ret;
-
-	return REG_READ(REG_GLOBAL2, 0x19);
-}
-
-static int __mv88e6352_phy_write(struct dsa_switch *ds, int addr, int regnum,
-				 u16 val)
-{
-	REG_WRITE(REG_GLOBAL2, 0x19, val);
-	REG_WRITE(REG_GLOBAL2, 0x18, 0x9400 | (addr << 5) | regnum);
-
-	return mv88e6352_phy_wait(ds);
-}
-
 static char *mv88e6352_probe(struct device *host_dev, int sw_addr)
 {
 	struct mii_bus *bus = dsa_host_dev_to_mii_bus(host_dev);

@@ -346,12 +293,12 @@ static int mv88e6352_phy_page_read(struct dsa_switch *ds,
 	int ret;
 
 	mutex_lock(&ps->phy_mutex);
-	ret = __mv88e6352_phy_write(ds, port, 0x16, page);
+	ret = mv88e6xxx_phy_write_indirect(ds, port, 0x16, page);
 	if (ret < 0)
 		goto error;
-	ret = __mv88e6352_phy_read(ds, port, reg);
+	ret = mv88e6xxx_phy_read_indirect(ds, port, reg);
 error:
-	__mv88e6352_phy_write(ds, port, 0x16, 0x0);
+	mv88e6xxx_phy_write_indirect(ds, port, 0x16, 0x0);
 	mutex_unlock(&ps->phy_mutex);
 	return ret;
 }

@@ -363,13 +310,13 @@ static int mv88e6352_phy_page_write(struct dsa_switch *ds,
 	int ret;
 
 	mutex_lock(&ps->phy_mutex);
-	ret = __mv88e6352_phy_write(ds, port, 0x16, page);
+	ret = mv88e6xxx_phy_write_indirect(ds, port, 0x16, page);
 	if (ret < 0)
 		goto error;
 
-	ret = __mv88e6352_phy_write(ds, port, reg, val);
+	ret = mv88e6xxx_phy_write_indirect(ds, port, reg, val);
 error:
-	__mv88e6352_phy_write(ds, port, 0x16, 0x0);
+	mv88e6xxx_phy_write_indirect(ds, port, 0x16, 0x0);
 	mutex_unlock(&ps->phy_mutex);
 	return ret;
 }

@@ -482,7 +429,7 @@ mv88e6352_phy_read(struct dsa_switch *ds, int port, int regnum)
 		return addr;
 
 	mutex_lock(&ps->phy_mutex);
-	ret = __mv88e6352_phy_read(ds, addr, regnum);
+	ret = mv88e6xxx_phy_read_indirect(ds, addr, regnum);
 	mutex_unlock(&ps->phy_mutex);
 
 	return ret;

@@ -499,7 +446,7 @@ mv88e6352_phy_write(struct dsa_switch *ds, int port, int regnum, u16 val)
 		return addr;
 
 	mutex_lock(&ps->phy_mutex);
-	ret = __mv88e6352_phy_write(ds, addr, regnum, val);
+	ret = mv88e6xxx_phy_write_indirect(ds, addr, regnum, val);
 	mutex_unlock(&ps->phy_mutex);
 
 	return ret;

@@ -553,7 +500,7 @@ static int mv88e6352_read_eeprom_word(struct dsa_switch *ds, int addr)
 	if (ret < 0)
 		goto error;
 
-	ret = mv88e6352_eeprom_busy_wait(ds);
+	ret = mv88e6xxx_eeprom_busy_wait(ds);
 	if (ret < 0)
 		goto error;
 

@@ -576,7 +523,7 @@ static int mv88e6352_get_eeprom(struct dsa_switch *ds,
 
 	eeprom->magic = 0xc3ec4951;
 
-	ret = mv88e6352_eeprom_load_wait(ds);
+	ret = mv88e6xxx_eeprom_load_wait(ds);
 	if (ret < 0)
 		return ret;
 

@@ -657,7 +604,7 @@ static int mv88e6352_write_eeprom_word(struct dsa_switch *ds, int addr,
 	if (ret < 0)
 		goto error;
 
-	ret = mv88e6352_eeprom_busy_wait(ds);
+	ret = mv88e6xxx_eeprom_busy_wait(ds);
 error:
 	mutex_unlock(&ps->eeprom_mutex);
 	return ret;

@@ -681,7 +628,7 @@ static int mv88e6352_set_eeprom(struct dsa_switch *ds,
 	len = eeprom->len;
 	eeprom->len = 0;
 
-	ret = mv88e6352_eeprom_load_wait(ds);
+	ret = mv88e6xxx_eeprom_load_wait(ds);
 	if (ret < 0)
 		return ret;
 

diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
index 3e7e31a6abb7..a83ace0803e7 100644
--- a/drivers/net/dsa/mv88e6xxx.c
+++ b/drivers/net/dsa/mv88e6xxx.c

@@ -596,6 +596,59 @@ error:
 }
 #endif /* CONFIG_NET_DSA_HWMON */
 
+static int mv88e6xxx_wait(struct dsa_switch *ds, int reg, int offset, u16 mask)
+{
+	unsigned long timeout = jiffies + HZ / 10;
+
+	while (time_before(jiffies, timeout)) {
+		int ret;
+
+		ret = REG_READ(reg, offset);
+		if (!(ret & mask))
+			return 0;
+
+		usleep_range(1000, 2000);
+	}
+	return -ETIMEDOUT;
+}
+
+int mv88e6xxx_phy_wait(struct dsa_switch *ds)
+{
+	return mv88e6xxx_wait(ds, REG_GLOBAL2, 0x18, 0x8000);
+}
+
+int mv88e6xxx_eeprom_load_wait(struct dsa_switch *ds)
+{
+	return mv88e6xxx_wait(ds, REG_GLOBAL2, 0x14, 0x0800);
+}
+
+int mv88e6xxx_eeprom_busy_wait(struct dsa_switch *ds)
+{
+	return mv88e6xxx_wait(ds, REG_GLOBAL2, 0x14, 0x8000);
+}
+
+int mv88e6xxx_phy_read_indirect(struct dsa_switch *ds, int addr, int regnum)
+{
+	int ret;
+
+	REG_WRITE(REG_GLOBAL2, 0x18, 0x9800 | (addr << 5) | regnum);
+
+	ret = mv88e6xxx_phy_wait(ds);
+	if (ret < 0)
+		return ret;
+
+	return REG_READ(REG_GLOBAL2, 0x19);
+}
+
+int mv88e6xxx_phy_write_indirect(struct dsa_switch *ds, int addr, int regnum,
+				 u16 val)
+{
+	REG_WRITE(REG_GLOBAL2, 0x19, val);
+	REG_WRITE(REG_GLOBAL2, 0x18, 0x9400 | (addr << 5) | regnum);
+
+	return mv88e6xxx_phy_wait(ds);
+}
+
 static int __init mv88e6xxx_init(void)
 {
 #if IS_ENABLED(CONFIG_NET_DSA_MV88E6131)

diff --git a/drivers/net/dsa/mv88e6xxx.h b/drivers/net/dsa/mv88e6xxx.h
index 03e397efde36..72942271bb67 100644
--- a/drivers/net/dsa/mv88e6xxx.h
+++ b/drivers/net/dsa/mv88e6xxx.h

@@ -82,6 +82,12 @@ int mv88e6xxx_get_regs_len(struct dsa_switch *ds, int port);
 void mv88e6xxx_get_regs(struct dsa_switch *ds, int port,
 			struct ethtool_regs *regs, void *_p);
 int  mv88e6xxx_get_temp(struct dsa_switch *ds, int *temp);
+int mv88e6xxx_phy_wait(struct dsa_switch *ds);
+int mv88e6xxx_eeprom_load_wait(struct dsa_switch *ds);
+int mv88e6xxx_eeprom_busy_wait(struct dsa_switch *ds);
+int mv88e6xxx_phy_read_indirect(struct dsa_switch *ds, int addr, int regnum);
+int mv88e6xxx_phy_write_indirect(struct dsa_switch *ds, int addr, int regnum,
+				 u16 val);
 
 extern struct dsa_switch_driver mv88e6131_switch_driver;
 extern struct dsa_switch_driver mv88e6123_61_65_switch_driver;

-- 
2.1.4

When the device is configured to use single chip addressing mode, the
phy devices of the port are not accessible on the host MDIO
bus. Instead the switch internal MDIO bus must be used. For this to
work, the phy polling unit must be enabled.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/dsa/mv88e6171.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/net/dsa/mv88e6171.c b/drivers/net/dsa/mv88e6171.c
index aa33d16f2e22..9808c860a797 100644
--- a/drivers/net/dsa/mv88e6171.c
+++ b/drivers/net/dsa/mv88e6171.c

@@ -51,8 +51,11 @@ static int mv88e6171_switch_reset(struct dsa_switch *ds)
 	/* Wait for transmit queues to drain. */
 	usleep_range(2000, 4000);
 
-	/* Reset the switch. */
-	REG_WRITE(REG_GLOBAL, 0x04, 0xc400);
+	/* Reset the switch. Keep PPU active.  The PPU needs to be
+	 * active to support indirect phy register accesses through
+	 * global registers 0x18 and 0x19.
+	 */
+	REG_WRITE(REG_GLOBAL, 0x04, 0xc000);
 
 	/* Wait up to one second for reset to complete. */
 	timeout = jiffies + 1 * HZ;

@@ -83,11 +86,10 @@ static int mv88e6171_setup_global(struct dsa_switch *ds)
 	int ret;
 	int i;
 
-	/* Disable the PHY polling unit (since there won't be any
-	 * external PHYs to poll), don't discard packets with
-	 * excessive collisions, and mask all interrupt sources.
+	/* Discard packets with excessive collisions, mask all
+	 * interrupt sources, enable PPU.
 	 */
-	REG_WRITE(REG_GLOBAL, 0x04, 0x0000);
+	REG_WRITE(REG_GLOBAL, 0x04, 0x6000);
 
 	/* Set the default address aging time to 5 minutes, and
 	 * enable address learn messages to be sent to all message

@@ -336,7 +338,7 @@ mv88e6171_phy_read(struct dsa_switch *ds, int port, int regnum)
 	int ret;
 
 	mutex_lock(&ps->phy_mutex);
-	ret = mv88e6xxx_phy_read(ds, addr, regnum);
+	ret = mv88e6xxx_phy_read_indirect(ds, addr, regnum);
 	mutex_unlock(&ps->phy_mutex);
 	return ret;
 }

@@ -350,7 +352,7 @@ mv88e6171_phy_write(struct dsa_switch *ds,
 	int ret;
 
 	mutex_lock(&ps->phy_mutex);
-	ret = mv88e6xxx_phy_write(ds, addr, regnum, val);
+	ret = mv88e6xxx_phy_write_indirect(ds, addr, regnum, val);
 	mutex_unlock(&ps->phy_mutex);
 	return ret;
 }

-- 
2.1.4

On 02/14/2015 10:17 AM, Andrew Lunn wrote:

These two patches allow the mv88e6171 driver to access the port phys
using indirect addressing. Depending on pin strapping, the switch
either uses a single address on the host MDIO bus, requiring the port
phys are accessed indirectly, or the switch uses a number of addresses
on the host bus and the phys can be directly accessed.

The 370RD, the first supported platform to use the 6171 uses multiple
addresses, so this indirect mode was not required. However the
WRT1900AC has the switch configured to use a single address, and so
indirect access is needed.

The mv88e6352 already has all the needed code. Refactor it into the
shared mv88e6xxx and then use it in the mv88e6171 driver.

Tested on the 370RD and WRT1900AC.

It would be good if Guenter Roeck could test on his platform to ensure
i've not broken anything for the mv88e6352.

I'll try. My primary problem right now is that I run Lennert Buytenhek's
patch set to support bridging offload (aka hardware bridging) on top of
the upstream dsa code, and the upstream code now supports a competing /
alternate means  to support bridging/switching offload (NET_SWITCHDEV)
which doesn't work with dsa (at least not yet). So I'll have to figure
out if / how I can run your patches with my code base, or how I can add
add support for NET_SWITCHDEV into dsa.

Do you know if there are any efforts going on in that direction ?

Thanks,
Guenter

I'll try. My primary problem right now is that I run Lennert Buytenhek's
patch set to support bridging offload (aka hardware bridging) on top of
the upstream dsa code, and the upstream code now supports a competing /
alternate means  to support bridging/switching offload (NET_SWITCHDEV)
which doesn't work with dsa (at least not yet). So I'll have to figure
out if / how I can run your patches with my code base, or how I can add
add support for NET_SWITCHDEV into dsa.

We should be adding switchdev support to DSA for hardware
bridging. The concepts in Lennert Buytenhek's should be a good
starting point for this.
 

Do you know if there are any efforts going on in that direction ?

Florian has expressed an interest in getting hardware bridging
working. I've no idea if he has looked at it from the perspective of
switchdev.

	Andrew

2015-02-15 12:25 GMT-08:00 Andrew Lunn [off-list ref]:

quoted

I'll try. My primary problem right now is that I run Lennert Buytenhek's
patch set to support bridging offload (aka hardware bridging) on top of
the upstream dsa code, and the upstream code now supports a competing /
alternate means  to support bridging/switching offload (NET_SWITCHDEV)
which doesn't work with dsa (at least not yet). So I'll have to figure
out if / how I can run your patches with my code base, or how I can add
add support for NET_SWITCHDEV into dsa.

We should be adding switchdev support to DSA for hardware
bridging. The concepts in Lennert Buytenhek's should be a good
starting point for this.

In fact, there is not much to be implemented in DSA, since we already
have everything in place in net-next now:

- net_device notifier to learn which ports are leaving/joining the bridge
- hook an abstraction for ndo_switch_port_stp_update
- hook an abstraction for ndo_fdb_{add,del,dump}

quoted

Do you know if there are any efforts going on in that direction ?

Florian has expressed an interest in getting hardware bridging
working. I've no idea if he has looked at it from the perspective of
switchdev.

I have some patches that leverage the switchdev-related patches and
bring an abstraction to DSA, they should not be fundamentally
different at the DSA driver level, since most of the abstraction is
done in net/dsa/slave.c, I plan on posting these patches tomorrow once
I am back home. I tested these with the bcm_sf2 driver, so testing
with Marvell hardware would be more than welcome.

Thanks!
-- 
Florian

On 02/15/2015 01:05 PM, Florian Fainelli wrote:

2015-02-15 12:25 GMT-08:00 Andrew Lunn [off-list ref]:

quoted

quoted

I'll try. My primary problem right now is that I run Lennert Buytenhek's
patch set to support bridging offload (aka hardware bridging) on top of
the upstream dsa code, and the upstream code now supports a competing /
alternate means  to support bridging/switching offload (NET_SWITCHDEV)
which doesn't work with dsa (at least not yet). So I'll have to figure
out if / how I can run your patches with my code base, or how I can add
add support for NET_SWITCHDEV into dsa.

We should be adding switchdev support to DSA for hardware
bridging. The concepts in Lennert Buytenhek's should be a good
starting point for this.

In fact, there is not much to be implemented in DSA, since we already
have everything in place in net-next now:

- net_device notifier to learn which ports are leaving/joining the bridge
- hook an abstraction for ndo_switch_port_stp_update
- hook an abstraction for ndo_fdb_{add,del,dump}

quoted

quoted

Do you know if there are any efforts going on in that direction ?

Florian has expressed an interest in getting hardware bridging
working. I've no idea if he has looked at it from the perspective of
switchdev.

I have some patches that leverage the switchdev-related patches and
bring an abstraction to DSA, they should not be fundamentally
different at the DSA driver level, since most of the abstraction is
done in net/dsa/slave.c, I plan on posting these patches tomorrow once
I am back home. I tested these with the bcm_sf2 driver, so testing
with Marvell hardware would be more than welcome.

Sounds good. I would be more than happy to test the code with the
Marvell hardware.

My current status is that I got Lennert's patches running on top of
the latest version of the dsa code, including the switchdev changes.
It currently uses ndo_switch_port_stp_update, but I still have
ndo_bridge_join, ndo_bridge_leave, and ndo_bridge_port_flush
as added callbacks.

Guenter

On 02/14/2015 10:17 AM, Andrew Lunn wrote:

The mv88e6352 allows access to the port phys via an internal mdio bus
which is accessed using registers in the GLOBAL 2 range. The mv88e6171
and probably other devices use the same mechanism. Move this code into
the shared mv88e6xxx.c library.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>

With mv88e6352:

Tested-by: Guenter Roeck <linux@roeck-us.net>

Guenter

From: Andrew Lunn <andrew@lunn.ch>
Date: Sat, 14 Feb 2015 19:17:49 +0100

These two patches allow the mv88e6171 driver to access the port phys
using indirect addressing. Depending on pin strapping, the switch
either uses a single address on the host MDIO bus, requiring the port
phys are accessed indirectly, or the switch uses a number of addresses
on the host bus and the phys can be directly accessed.

The 370RD, the first supported platform to use the 6171 uses multiple
addresses, so this indirect mode was not required. However the
WRT1900AC has the switch configured to use a single address, and so
indirect access is needed.

The mv88e6352 already has all the needed code. Refactor it into the
shared mv88e6xxx and then use it in the mv88e6171 driver.

Tested on the 370RD and WRT1900AC.

It would be good if Guenter Roeck could test on his platform to ensure
i've not broken anything for the mv88e6352.

Series applied, thanks.