| Message ID | 20210215070218.1188903-1-nathan@nathanrossi.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | of: of_mdio: Handle properties for non-phy mdio devices | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Guessed tree name to be net-next |
| netdev/subject_prefix | warning | Target tree name not specified in the subject |
| netdev/cc_maintainers | warning | 1 maintainers not CCed: linux@armlinux.org.uk |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 40 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
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
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 >
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
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.
> > 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
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
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 --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. */