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

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

[Nathan Rossi]

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(-)

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

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

[Andrew Lunn]

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.

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

     Andrew

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

[Florian Fainelli]

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
> 

-- 
Florian

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

[Nathan Rossi]

On Wed, 17 Feb 2021 at 03:50, Florian Fainelli <f.fainelli@gmail.com> wrote:
>
>
>
> 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

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

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

[Florian Fainelli]

On 2/16/2021 2:57 PM, Nathan Rossi wrote:
> On Wed, 17 Feb 2021 at 03:50, Florian Fainelli <f.fainelli@gmail.com> wrote:
>>
>>
>>
>> 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
> 
> 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.
>>
>>>
>>> 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

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

[Andrew Lunn]

> > 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

[PATCH v2] of: of_mdio: Handle broken-turn-around for non-phy mdio devices

[Nathan Rossi]

From: Nathan Rossi <nathan.rossi@digi.com>

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 <nathan.rossi@digi.com>
---
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

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

[Nathan Rossi]

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

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

[Andrew Lunn]

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