[PATCH] of: of_mdio: Handle properties for non-phy mdio devices

From: Nathan Rossi <redacted>

The documentation for MDIO bindings describes the "broken-turn-around",
"reset-assert-us", and "reset-deassert-us" properties such that any MDIO
device can define them. Other MDIO devices may require these properties
in order to correctly function on the MDIO bus.

Enable the parsing and configuration associated with these properties by
moving the associated OF parsing to a common function
of_mdiobus_child_parse and use it to apply these properties for both
PHYs and other MDIO devices.

Signed-off-by: Nathan Rossi <redacted>
---
 drivers/net/mdio/of_mdio.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/drivers/net/mdio/of_mdio.c b/drivers/net/mdio/of_mdio.c
index 4daf94bb56..c28db49b72 100644
--- a/drivers/net/mdio/of_mdio.c
+++ b/drivers/net/mdio/of_mdio.c

@@ -42,6 +42,18 @@ static int of_get_phy_id(struct device_node *device, u32 *phy_id)
 	return -EINVAL;
 }
 
+static void of_mdiobus_child_parse(struct mii_bus *mdio, struct mdio_device
+				   *mdiodev, struct device_node *node, u32 addr)
+{
+	if (of_property_read_bool(node, "broken-turn-around"))
+		mdio->phy_ignore_ta_mask |= 1 << addr;
+
+	of_property_read_u32(node, "reset-assert-us",
+			     &mdiodev->reset_assert_delay);
+	of_property_read_u32(node, "reset-deassert-us",
+			     &mdiodev->reset_deassert_delay);
+}
+
 static struct mii_timestamper *of_find_mii_timestamper(struct device_node *node)
 {
 	struct of_phandle_args arg;

@@ -76,13 +88,7 @@ int of_mdiobus_phy_device_register(struct mii_bus *mdio, struct phy_device *phy,
 		phy->irq = mdio->irq[addr];
 	}
 
-	if (of_property_read_bool(child, "broken-turn-around"))
-		mdio->phy_ignore_ta_mask |= 1 << addr;
-
-	of_property_read_u32(child, "reset-assert-us",
-			     &phy->mdio.reset_assert_delay);
-	of_property_read_u32(child, "reset-deassert-us",
-			     &phy->mdio.reset_deassert_delay);
+	of_mdiobus_child_parse(mdio, &phy->mdio, child, addr);
 
 	/* Associate the OF node with the device structure so it
 	 * can be looked up later */

@@ -158,6 +164,8 @@ static int of_mdiobus_register_device(struct mii_bus *mdio,
 	if (IS_ERR(mdiodev))
 		return PTR_ERR(mdiodev);
 
+	of_mdiobus_child_parse(mdio, mdiodev, child, addr);
+
 	/* Associate the OF node with the device structure so it
 	 * can be looked up later.
 	 */

---
2.30.0

On Mon, Feb 15, 2021 at 07:02:18AM +0000, Nathan Rossi wrote:

From: Nathan Rossi <redacted>

The documentation for MDIO bindings describes the "broken-turn-around",
"reset-assert-us", and "reset-deassert-us" properties such that any MDIO
device can define them. Other MDIO devices may require these properties
in order to correctly function on the MDIO bus.

Enable the parsing and configuration associated with these properties by
moving the associated OF parsing to a common function
of_mdiobus_child_parse and use it to apply these properties for both
PHYs and other MDIO devices.

Hi Nathan

What device are you using this with?

The Marvell Switch driver does its own GPIO reset handling. It has a
better idea when a hardware reset should be applied than what the
phylib core has. It will also poll the EEPROM busy bit after a
reset. How long a pause you need after the reset depends on how full
the EEPROM is.

And i've never had problems with broken-turn-around with Marvell
switches.

Given the complexity of an Ethernet switch, it is probably better if
it handles its own reset.

     Andrew

On 2/16/2021 5:06 AM, Andrew Lunn wrote:

On Mon, Feb 15, 2021 at 07:02:18AM +0000, Nathan Rossi wrote:

quoted

From: Nathan Rossi <redacted>

The documentation for MDIO bindings describes the "broken-turn-around",
"reset-assert-us", and "reset-deassert-us" properties such that any MDIO
device can define them. Other MDIO devices may require these properties
in order to correctly function on the MDIO bus.

Enable the parsing and configuration associated with these properties by
moving the associated OF parsing to a common function
of_mdiobus_child_parse and use it to apply these properties for both
PHYs and other MDIO devices.

Hi Nathan

What device are you using this with?

The Marvell Switch driver does its own GPIO reset handling. It has a
better idea when a hardware reset should be applied than what the
phylib core has. It will also poll the EEPROM busy bit after a
reset. How long a pause you need after the reset depends on how full
the EEPROM is.

And i've never had problems with broken-turn-around with Marvell
switches.

The patch does make sense though, Broadcom 53125 switches have a broken
turn around and are mdio_device instances, the broken behavior may not
show up with all MDIO controllers used to interface though. For the
reset, I would agree with you this is better delegated to the switch
driver, given that unlike PHY devices, we have no need to know the
mdio_device ID prior to binding the device and the driver together.

Given the complexity of an Ethernet switch, it is probably better if
it handles its own reset.

     Andrew

-- 
Florian

On Wed, 17 Feb 2021 at 03:50, Florian Fainelli [off-list ref] wrote:

On 2/16/2021 5:06 AM, Andrew Lunn wrote:

quoted

On Mon, Feb 15, 2021 at 07:02:18AM +0000, Nathan Rossi wrote:

quoted

From: Nathan Rossi <redacted>

The documentation for MDIO bindings describes the "broken-turn-around",
"reset-assert-us", and "reset-deassert-us" properties such that any MDIO
device can define them. Other MDIO devices may require these properties
in order to correctly function on the MDIO bus.

Enable the parsing and configuration associated with these properties by
moving the associated OF parsing to a common function
of_mdiobus_child_parse and use it to apply these properties for both
PHYs and other MDIO devices.

Hi Nathan

What device are you using this with?

The Marvell Switch driver does its own GPIO reset handling. It has a
better idea when a hardware reset should be applied than what the
phylib core has. It will also poll the EEPROM busy bit after a
reset. How long a pause you need after the reset depends on how full
the EEPROM is.

And i've never had problems with broken-turn-around with Marvell
switches.

The patch does make sense though, Broadcom 53125 switches have a broken
turn around and are mdio_device instances, the broken behavior may not
show up with all MDIO controllers used to interface though. For the

Yes the reason we needed this change was to enable broken turn around,
specifically with a Marvell 88E6390.

reset, I would agree with you this is better delegated to the switch
driver, given that unlike PHY devices, we have no need to know the
mdio_device ID prior to binding the device and the driver together.

quoted

Given the complexity of an Ethernet switch, it is probably better if
it handles its own reset.

We are not using the reset assert, I included this as part of the
change to match the existing phy parsing behavior. I can update this
change to only handle broken turn around, or is it also preferred that
broken turn around is handled by the e.g. switch driver?

Thanks,
Nathan

On 2/16/2021 2:57 PM, Nathan Rossi wrote:

On Wed, 17 Feb 2021 at 03:50, Florian Fainelli [off-list ref] wrote:

quoted

On 2/16/2021 5:06 AM, Andrew Lunn wrote:

quoted

On Mon, Feb 15, 2021 at 07:02:18AM +0000, Nathan Rossi wrote:

quoted

From: Nathan Rossi <redacted>

The documentation for MDIO bindings describes the "broken-turn-around",
"reset-assert-us", and "reset-deassert-us" properties such that any MDIO
device can define them. Other MDIO devices may require these properties
in order to correctly function on the MDIO bus.

Enable the parsing and configuration associated with these properties by
moving the associated OF parsing to a common function
of_mdiobus_child_parse and use it to apply these properties for both
PHYs and other MDIO devices.

Hi Nathan

What device are you using this with?

The Marvell Switch driver does its own GPIO reset handling. It has a
better idea when a hardware reset should be applied than what the
phylib core has. It will also poll the EEPROM busy bit after a
reset. How long a pause you need after the reset depends on how full
the EEPROM is.

And i've never had problems with broken-turn-around with Marvell
switches.

The patch does make sense though, Broadcom 53125 switches have a broken
turn around and are mdio_device instances, the broken behavior may not
show up with all MDIO controllers used to interface though. For the

Yes the reason we needed this change was to enable broken turn around,
specifically with a Marvell 88E6390.

quoted

reset, I would agree with you this is better delegated to the switch
driver, given that unlike PHY devices, we have no need to know the
mdio_device ID prior to binding the device and the driver together.

quoted

Given the complexity of an Ethernet switch, it is probably better if
it handles its own reset.

We are not using the reset assert, I included this as part of the
change to match the existing phy parsing behavior. I can update this
change to only handle broken turn around, or is it also preferred that
broken turn around is handled by the e.g. switch driver?

Broken turn around needs to be handled by the core MDIO layer since it
affects what happens to the mdiobus_read() operation, so it is
appropriate to be common to all MDIO and PHY devices. Basically take
your patch but leave the reset handling to the PHY device handling.
-- 
Florian

quoted

The patch does make sense though, Broadcom 53125 switches have a broken
turn around and are mdio_device instances, the broken behavior may not
show up with all MDIO controllers used to interface though. For the

Yes the reason we needed this change was to enable broken turn around,
specifically with a Marvell 88E6390.

Ah, odd. I've never had problems with the 6390, either connected to a
Freecale FEC, or the Linux bit banging MDIO bus.

What are you using for an MDIO bus controller? Did it already support
broken turn around, or did you need to add it?

       Andrew

From: Nathan Rossi <redacted>

The documentation for MDIO bindings describes the "broken-turn-around",
property such that any MDIO device can define it. Other MDIO devices may
require this property in order to correctly function on the MDIO bus.

Enable the parsing and configuration associated with this property for
non-phy MDIO devices.

Signed-off-by: Nathan Rossi <redacted>
---
Changes in v2:
- Only handle broken-turn-around for non-phy devices
- No need for of_mdiobus_child_parse function
---
 drivers/net/mdio/of_mdio.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/mdio/of_mdio.c b/drivers/net/mdio/of_mdio.c
index 4daf94bb56..9796f259a8 100644
--- a/drivers/net/mdio/of_mdio.c
+++ b/drivers/net/mdio/of_mdio.c

@@ -158,6 +158,9 @@ static int of_mdiobus_register_device(struct mii_bus *mdio,
 	if (IS_ERR(mdiodev))
 		return PTR_ERR(mdiodev);
 
+	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.
 	 */

---
2.30.0

On Wed, 17 Feb 2021 at 13:19, Andrew Lunn [off-list ref] wrote:

quoted

quoted

The patch does make sense though, Broadcom 53125 switches have a broken
turn around and are mdio_device instances, the broken behavior may not
show up with all MDIO controllers used to interface though. For the

Yes the reason we needed this change was to enable broken turn around,
specifically with a Marvell 88E6390.

Ah, odd. I've never had problems with the 6390, either connected to a
Freecale FEC, or the Linux bit banging MDIO bus.

What are you using for an MDIO bus controller? Did it already support
broken turn around, or did you need to add it?

Using bit bang MDIO to access the 88e6390. I suspect the issue is
specific to the board design, another similar design we have uses bit
bang MDIO but a 88e6193x switch and does not have any issue with turn
around.

Regards,
Nathan

On Wed, Feb 17, 2021 at 02:48:30PM +1000, Nathan Rossi wrote:

On Wed, 17 Feb 2021 at 13:19, Andrew Lunn [off-list ref] wrote:

quoted

quoted

quoted

The patch does make sense though, Broadcom 53125 switches have a broken
turn around and are mdio_device instances, the broken behavior may not
show up with all MDIO controllers used to interface though. For the

Yes the reason we needed this change was to enable broken turn around,
specifically with a Marvell 88E6390.

Ah, odd. I've never had problems with the 6390, either connected to a
Freecale FEC, or the Linux bit banging MDIO bus.

What are you using for an MDIO bus controller? Did it already support
broken turn around, or did you need to add it?

Using bit bang MDIO to access the 88e6390. I suspect the issue is
specific to the board design, another similar design we have uses bit
bang MDIO but a 88e6193x switch and does not have any issue with turn
around.

So to me, it sounds like changing the data pin, by the host, from
being driven to high impedance, is taking too long. So this is a bus
problem, not a per device on the bus problem. You need to indicate to
the bus controller that all addresses on the bus have broken turn
around, not just one. If you look at mdio-bitbang.c  it has:

        /* 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.
                 */
                for (i = 0; i < 32; i++)
                        mdiobb_get_bit(ctrl);

                return 0xffff;
        }

So the property it specific to one address. And the mv88e6xxx normally
takes up multiple addresses on the bus.

So i would do this differently. Add a new property to "mdio-gpio" to
indicate the host has broken turn around, and it needs to set all 32
bits of bus->phy_ignore_ta_mask.

     Andrew