mdio-gpio: Optionally ignore TA errors

Some PHYs do not always drive TA low despite returning valid data. Allow ignoring the TA value if we know we are connected to one of these.

This is based on a patch by Jonas Gorski.

---
This fixes MDIO communication with the second AR8316 on the Mikrotik RB493G. The board has an AR7161 SoC, which has only one MDIO interface, but there are two AR8316 switch chips. The first is handled by the SoC's MDIO pins, and enumerates fine. The second is wired into some GPIO pins, and thus uses the mdio-gpio driver.

Evidently, the AR8316 doesn't drive TA low when mdiobb_read() expects it, and the driver discards the data as a result. OpenWrt includes a simplified version of this patch: the check is removed altogether, and things just work.

A few problems with this patch:

- It's overly broad. As it turns out, this missing TA-low only happens once, on the first read, which is done by get_phy_device() to read the PHY id. Putting a discarded read in there before the real one ALSO fixes the problem:
the second and subsequent reads are fine. Unfortunately I can't force a dummy read on this board without smashing through a bunch of layers.

- I'm not entirely convinced that the PHY's bringing TA low needs checking to begin with. The standard clearly says that the PHY "shall" do it, but it's equally clear that the MDIO interface on the AR7161 doesn't have a problem talking to the AR8316. Presumably there are other SoCs in the wild talking to AR8316 MDIO interfaces.

- It could also be a timing problem in mdiobb_read(), but I haven't been able to find it if so. Unfortunately my board doesn't really allow me to stick a logic analyzer on there and figure out what's different between the two MDIO channels.

Advice, anyone?

 drivers/net/phy/mdio-bitbang.c | 2 +-
 drivers/net/phy/mdio-gpio.c    | 1 +
 include/linux/mdio-bitbang.h   | 3 +++
 include/linux/mdio-gpio.h      | 2 ++
 4 files changed, 7 insertions(+), 1 deletion(-)

index daec9b0..306fb74 100644
@@ -166,7 +166,7 @@ static int mdiobb_read(struct mii_bus *bus, int phy, int reg)
 	ctrl->ops->set_mdio_dir(ctrl, 0);
 
 	/* check the turnaround bit: the PHY should be driving it to zero */
-	if (mdiobb_get_bit(ctrl) != 0) {
+	if (mdiobb_get_bit(ctrl) != 0 && !ctrl->ignore_ta_errors) {
 		/* PHY didn't drive TA low -- flush any bits it
 		 * may be trying to send.
 		 */

index 0a0578a..f5cddf5 100644
@@ -138,6 +138,7 @@ static struct mii_bus *mdio_gpio_bus_init(struct device *dev,
 	if (!bitbang)
 		goto out;
 
+	bitbang->ctrl.ignore_ta_errors = pdata->ignore_ta_errors;
 	bitbang->ctrl.ops = &mdio_gpio_ops;
 	bitbang->ctrl.reset = pdata->reset;
 	bitbang->mdc = pdata->mdc;

index 76f52bb..3469bf2 100644
@@ -31,6 +31,9 @@ struct mdiobb_ops {
 };
 
 struct mdiobb_ctrl {
+	/* Some PHYs don't drive TA low even though they completed successfully. */
+	unsigned int ignore_ta_errors:1;
+
 	const struct mdiobb_ops *ops;
 	/* reset callback */
 	int (*reset)(struct mii_bus *bus);

index 66c30a7..cfe6d1b 100644
@@ -19,6 +19,8 @@ struct mdio_gpio_platform_data {
 	unsigned int mdio;
 	unsigned int mdo;
 
+	unsigned int ignore_ta_errors:1;
+
 	bool mdc_active_low;
 	bool mdio_active_low;
 	bool mdo_active_low;

1.9.1

I think we need a more generic solution to this problem, not localized only to the mdio-gpio driver as this also affects other systems using non-bit banged MDIO bus controllers, and something that limits the TA workaround to particular PHY addresses, not globally.

What I was thinking about is something along these lines:

- add a phy_ignore_ta_mask field to struct mii_bus

- when a MDIO bus is probed via Device Tree, scan for a "ignore-ta" boolean property and set the phy_ignore_ta_mask bitmask accordingly

- when a MDIO bus is probed via regular platform data it can set the phy_ignore_ta_mask directly while probing/connecting the PHY devices

- update the relevant MDIO bus drivers to check for this bit if they are known to be interfaced with pathological PHY devices/switches that do not release the drive TA low when expected

Does that sound reasonable to you?

Florian

Hi all,

This is an attempt at solving the broken turn-around problem in a way that is not specific to the mdio-gpio driver, since it affects different kinds of platforms.

We cannot make that localized to PHY device drivers because probing the PHY device which has a broken turn-around can fail as early as in get_phy_id(), therefore we need a bit of help from Device Tree/platform_data.

Florian Fainelli (3):
net: phy: Add phy_ignore_ta_mask to account for broken turn-around
of: mdio: Add a "broken-turn-around" property
net: phy: mdio-gpio: Handle phy_ignore_ta_mask

 Documentation/devicetree/bindings/net/phy.txt | 3 +++
 drivers/net/phy/mdio-bitbang.c                | 7 +++++--
 drivers/of/of_mdio.c                          | 3 +++
 include/linux/phy.h                           | 3 +++
 4 files changed, 14 insertions(+), 2 deletions(-)

2.1.0

Some PHY devices/switches will not release the turn-around line as they should do at the end of a MDIO transaction. To help with such situations, allow MDIO bus drivers to be made aware of such restrictions.

---
 include/linux/phy.h | 3 +++
 1 file changed, 3 insertions(+)

index 685809835b5c..701c7a3946e0 100644
@@ -181,6 +181,9 @@ struct mii_bus {
 	/* PHY addresses to be ignored when probing */
 	u32 phy_mask;
 
+	/* PHY addresses to ignore the TA/read failure */
+	u32 phy_ignore_ta_mask;
+
 	/*
 	 * Pointer to an array of interrupts, each PHY's
 	 * interrupt at the index matching its address

2.1.0

Some Ethernet PHY devices/switches may not properly release the MDIO bus during turn-around time, and fail to drive it low, which can be seen by some controllers as a read failure, while the data clocked in is still correct.

Add a boolean property "broken-turn-around" which is parsed by the generic MDIO bus probing code and will set the corresponding bit in the MDIO bus phy_ignore_ta_mask bitmask for MDIO bus drivers to utilize that information.

---
 Documentation/devicetree/bindings/net/phy.txt | 3 +++
 drivers/of/of_mdio.c                          | 3 +++
 2 files changed, 6 insertions(+)

index 40831fbaff72..525e1658f2da 100644
@@ -30,6 +30,9 @@ Optional Properties:
 
 - max-speed: Maximum PHY supported speed (10, 100, 1000...)
 
+- broken-turn-around: If set, indicates the PHY device does not correctly
+  release the turn around line low at the end of a MDIO transaction.
+
 Example:
 
 ethernet-phy@0 {

index 0c064485d1c2..fdc60db60829 100644
@@ -68,6 +68,9 @@ static int of_mdiobus_register_phy(struct mii_bus *mdio, struct device_node *chi
 			phy->irq = mdio->irq[addr];
 	}
 
+	if (of_property_read_bool(child, "broken-turn-around"))
+		mdio->phy_ignore_ta_mask |= 1 << addr;
+
 	/* Associate the OF node with the device structure so it
 	 * can be looked up later */
 	of_node_get(child);

2.1.0

Update mdiobb_read() to read whether the PHY has a broken turn-around, and if it does, ignore it to make the read succeeed.

---
 drivers/net/phy/mdio-bitbang.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

index daec9b05d168..61a543c788cc 100644
@@ -165,8 +165,11 @@ static int mdiobb_read(struct mii_bus *bus, int phy, int reg)
 
 	ctrl->ops->set_mdio_dir(ctrl, 0);
 
-	/* check the turnaround bit: the PHY should be driving it to zero */
-	if (mdiobb_get_bit(ctrl) != 0) {
+	/* check the turnaround bit: the PHY should be driving it to zero, if this
+	 * PHY is listed in phy_ignore_ta_mask as having broken TA, skip that
+	 */
+	if (mdiobb_get_bit(ctrl) != 0 &&
+	    !(bus->phy_ignore_ta_mask & (1 << phy))) {
 		/* PHY didn't drive TA low -- flush any bits it
 		 * may be trying to send.
 		 */

2.1.0

That is indeed a better place to put that ignore-ta flag, and the device tree support is of course a big improvement over the previous patch as well.

But it's still too big a hammer to hit the AR8316 problem with, I think. Just like the previous patch, this disables checking TA on _all_ reads, but I really only need it disabled on the first read.

Are you saying you've seen other systems with this TA problem? Did those have problems after more than just the first read?

Bert Vermeulen        [email protected]          email/xmpp

Yes, I have a bunch of boards here that use a Broadcom BCM53125 switch which is known not to release the line during turn-around time, and that is for all reads. A workaround is to discard the read failure indication from the MDIO controller, but to selectively do that, I would be keen on using the patches I just posted.

Florian

From: Florian Fainelli <[email protected]>
Date: Tue, 12 May 2015 10:33:23 -0700

Series applied, thanks Florian.