diff mbox series

of: of_mdio: Handle properties for non-phy mdio devices

Message ID 20210215070218.1188903-1-nathan@nathanrossi.com
State New
Headers show
Series of: of_mdio: Handle properties for non-phy mdio devices | expand

Commit Message

Nathan Rossi Feb. 15, 2021, 7:02 a.m. UTC 
From: Nathan Rossi <nathan.rossi@digi.com>

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 <nathan.rossi@digi.com>
---
 drivers/net/mdio/of_mdio.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

---
2.30.0

Comments

Florian Fainelli Feb. 16, 2021, 5:50 p.m. UTC | #1 
On 2/16/2021 5:06 AM, Andrew Lunn wrote:
> On Mon, Feb 15, 2021 at 07:02:18AM +0000, Nathan Rossi wrote:
>> From: Nathan Rossi <nathan.rossi@digi.com>
>>
>> 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
>
Andrew Lunn Feb. 17, 2021, 3:19 a.m. UTC | #2 
> > 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
Nathan Rossi Feb. 17, 2021, 4:48 a.m. UTC | #3 
On Wed, 17 Feb 2021 at 13:19, Andrew Lunn <andrew@lunn.ch> wrote:
>

> > > 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
Andrew Lunn Feb. 17, 2021, 1:25 p.m. UTC | #4 
On Wed, Feb 17, 2021 at 02:48:30PM +1000, Nathan Rossi wrote:
> On Wed, 17 Feb 2021 at 13:19, Andrew Lunn <andrew@lunn.ch> wrote:

> >

> > > > 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
diff mbox series

Patch

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.
 	 */