| Message ID | daa29a99-0a17-d316-57eb-3e6162938d8d@ti.com |
|---|---|
| State | New |
| Headers | show |
On Thu, Apr 20, 2017 at 05:11:53PM +0300, Roger Quadros wrote: > Some boards [1] leave the PHYs at an invalid state > during system power-up or reset thus causing unreliability > issues with the PHY which manifests as PHY not being detected > or link not functional. To fix this, these PHYs need to be RESET > via a GPIO connected to the PHY's RESET pin. > > Some boards have a single GPIO controlling the PHY RESET pin of all > PHYs on the bus whereas some others have separate GPIOs controlling > individual PHY RESETs. > > In both cases, the RESET de-assertion cannot be done in the PHY driver > as the PHY will not probe till its reset is de-asserted. > So do the RESET de-assertion in the MDIO bus driver. > > [1] - am572x-idk, am571x-idk, a437x-idk > > Signed-off-by: Roger Quadros <rogerq@ti.com> Hi Roger Thanks for doing a generic solutions and the MDIO DT documentation. Reviewed-by: Andrew Lunn <andrew@lunn.ch> Andrew -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Roger, On 04/20/2017 07:11 AM, Roger Quadros wrote: > Some boards [1] leave the PHYs at an invalid state > during system power-up or reset thus causing unreliability > issues with the PHY which manifests as PHY not being detected > or link not functional. To fix this, these PHYs need to be RESET > via a GPIO connected to the PHY's RESET pin. > > Some boards have a single GPIO controlling the PHY RESET pin of all > PHYs on the bus whereas some others have separate GPIOs controlling > individual PHY RESETs. > > In both cases, the RESET de-assertion cannot be done in the PHY driver > as the PHY will not probe till its reset is de-asserted. > So do the RESET de-assertion in the MDIO bus driver. > > [1] - am572x-idk, am571x-idk, a437x-idk > > Signed-off-by: Roger Quadros <rogerq@ti.com> A few comments on the binding and the code, sorry for this late review. > +Example : > +This example shows these optional properties, plus other properties > +required for the TI Davinci MDIO driver. > + > + davinci_mdio: ethernet@0x5c030000 { > + compatible = "ti,davinci_mdio"; > + reg = <0x5c030000 0x1000>; > + #address-cells = <1>; > + #size-cells = <0>; > + > + reset-gpios = <&gpio2 5 GPIO_ACTIVE_LOW>; > + reset-delay-us = <2>; /* PHY datasheet states 1uS min */ us is micro seconds, uS is micro siemens. > + > + ethphy0: ethernet-phy@1 { > + reg = <1>; > + }; > + > + ethphy1: ethernet-phy@3 { > + reg = <3>; > + }; > + }; > > + /* de-assert bus level PHY GPIO resets */ > + for (i = 0; i < bus->num_reset_gpios; i++) { > + gpiod = devm_gpiod_get_index(&bus->dev, "reset", i, > + GPIOD_OUT_LOW); > + if (IS_ERR(gpiod)) { > + err = PTR_ERR(gpiod); > + if (err != -ENOENT) { > + pr_err("mii_bus %s couldn't get reset GPIO\n", > + bus->id); Could we use dev_err(&bus->dev) here to better identify which MDIO bus is returning the problem? > + return err; Should we somehow "unwind" the reset lines we were able to successfully take out of reset and therefore put back into reset state? How about mdiobus_unregister()? Should we have similar code there, if not for correctness to be more power efficient? > + } > + } else { > + gpiod_set_value_cansleep(gpiod, 1); > + udelay(bus->reset_delay_us); > + gpiod_set_value_cansleep(gpiod, 0); Does that work even if the polarity of the reset line is active low? Thanks! -- Florian -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Apr 21, 2017 at 03:31:09PM +0200, Lars-Peter Clausen wrote: > On 04/21/2017 03:15 PM, Roger Quadros wrote: > > diff --git a/Documentation/devicetree/bindings/net/mdio.txt b/Documentation/devicetree/bindings/net/mdio.txt > > new file mode 100644 > > index 0000000..4ffbbac > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/net/mdio.txt > > @@ -0,0 +1,33 @@ > > +Common MDIO bus properties. > > + > > +These are generic properties that can apply to any MDIO bus. > > + > > +Optional properties: > > +- reset-gpios: List of one or more GPIOs that control the RESET lines > > + of the PHYs on that MDIO bus. > > +- reset-delay-us: RESET pulse width in microseconds as per PHY datasheet. > > + > > +A list of child nodes, one per device on the bus is expected. These > > +should follow the generic phy.txt, or a device specific binding document. > > + > > +Example : > > +This example shows these optional properties, plus other properties > > +required for the TI Davinci MDIO driver. > > + > > + davinci_mdio: ethernet@0x5c030000 { > > + compatible = "ti,davinci_mdio"; > > + reg = <0x5c030000 0x1000>; > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + reset-gpios = <&gpio2 5 GPIO_ACTIVE_LOW>; > > + reset-delay-us = <2>; /* PHY datasheet states 1us min */ > > If this is the reset line of the PHY shouldn't it be a property of the PHY > node rather than of the MDIO controller node (which might have a reset on > its own)? > > + > > + ethphy0: ethernet-phy@1 { > > + reg = <1>; > > + }; > > + > > + ethphy1: ethernet-phy@3 { > > + reg = <3>; > > + }; Hi Lars-Peter We discussed this when the first proposal was made. There are two cases, to consider. 1) Here, one GPIO line resets all PHYs on the same MDIO bus. In this example, two PHYs. 2) There is one GPIO line per PHY. That is a separate case, and as you say, the reset line should probably be considered a PHY property, not an MDIO property. However, it can be messy, since in order to probe the MDIO bus, you probably need to take the PHY out of reset. Anyway, this patch addresses the first case, so should be accepted. If anybody wants to address the second case, they are free to do so. Andrew -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 25/04/17 19:22, Lars-Peter Clausen wrote: > On 04/24/2017 11:04 AM, Roger Quadros wrote: >> On 24/04/17 02:35, Andrew Lunn wrote: >>> On Fri, Apr 21, 2017 at 03:31:09PM +0200, Lars-Peter Clausen wrote: >>>> On 04/21/2017 03:15 PM, Roger Quadros wrote: >>>>> diff --git a/Documentation/devicetree/bindings/net/mdio.txt b/Documentation/devicetree/bindings/net/mdio.txt >>>>> new file mode 100644 >>>>> index 0000000..4ffbbac >>>>> --- /dev/null >>>>> +++ b/Documentation/devicetree/bindings/net/mdio.txt >>>>> @@ -0,0 +1,33 @@ >>>>> +Common MDIO bus properties. >>>>> + >>>>> +These are generic properties that can apply to any MDIO bus. >>>>> + >>>>> +Optional properties: >>>>> +- reset-gpios: List of one or more GPIOs that control the RESET lines >>>>> + of the PHYs on that MDIO bus. >>>>> +- reset-delay-us: RESET pulse width in microseconds as per PHY datasheet. >>>>> + >>>>> +A list of child nodes, one per device on the bus is expected. These >>>>> +should follow the generic phy.txt, or a device specific binding document. >>>>> + >>>>> +Example : >>>>> +This example shows these optional properties, plus other properties >>>>> +required for the TI Davinci MDIO driver. >>>>> + >>>>> + davinci_mdio: ethernet@0x5c030000 { >>>>> + compatible = "ti,davinci_mdio"; >>>>> + reg = <0x5c030000 0x1000>; >>>>> + #address-cells = <1>; >>>>> + #size-cells = <0>; >>>>> + >>>>> + reset-gpios = <&gpio2 5 GPIO_ACTIVE_LOW>; >>>>> + reset-delay-us = <2>; /* PHY datasheet states 1us min */ >>>> >>>> If this is the reset line of the PHY shouldn't it be a property of the PHY >>>> node rather than of the MDIO controller node (which might have a reset on >>>> its own)? >>>>> + >>>>> + ethphy0: ethernet-phy@1 { >>>>> + reg = <1>; >>>>> + }; >>>>> + >>>>> + ethphy1: ethernet-phy@3 { >>>>> + reg = <3>; >>>>> + }; >>> >>> Hi Lars-Peter >>> >>> We discussed this when the first proposal was made. There are two >>> cases, to consider. >>> >>> 1) Here, one GPIO line resets all PHYs on the same MDIO bus. In this >>> example, two PHYs. >>> >>> 2) There is one GPIO line per PHY. That is a separate case, and as you >>> say, the reset line should probably be considered a PHY property, not >>> an MDIO property. However, it can be messy, since in order to probe >>> the MDIO bus, you probably need to take the PHY out of reset. >>> > > But the DT binding documentation says something else "List of one or more > GPIOs that control the RESET lines of the PHYs on that MDIO bus". > >>> Anyway, this patch addresses the first case, so should be accepted. If >>> anybody wants to address the second case, they are free to do so. > > I think we all know that that's not going to happen. Once there is a working > kludge there is no incentive to do a proper implementation anymore. > > >> Thanks for the explanation Andrew. >> >> For the second case, even if the RESET GPIO property is specified >> in the PHY node, the RESET *will* have to be done by the MDIO bus driver >> else the PHY might not be probed at all. > > I'm not arguing with that, just that the hardware description should be > truthful to the hardware topology and not to the software topology, i.e. the > implementation details of the Linux kernel in this case. Reset GPIOs are not > the only resource that is connected to the PHY that needs to be enabled > before they can be enumerated. E.g. clocks and regulators fall into the same > realm. And while you might argue that with a on-SoC phy controller node > there wont be any conflicts in regard to the reset-gpios property, this not > so very true for the clocks property. > > And MDIO is not really special in this regard, other discoverable buses > (like USB, SDIO, ULPI) have the very same issue. Having a standardized > binding approach where the resources are declared as part as the child child > is preferable in my opinion. Good point. I agree now that if PHYs have individual RESET lines, they should be part of the PHY node. > >> >> Whether we need additional code to just to make the DT look prettier is >> questionable and if required can come as a separate patch. > > Unfortunately not, once it is merged it can't be changed anymore. > cheers, -roger -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> Patch is already in net-next. How can we get this fixed? Should I send a v5?
Hi Roger
See the followup patch Florian submitted. Send your Reviewed-by: if
you agree with it.
Andrew
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/net/mdio.txt b/Documentation/devicetree/bindings/net/mdio.txt new file mode 100644 index 0000000..6e703d7 --- /dev/null +++ b/Documentation/devicetree/bindings/net/mdio.txt @@ -0,0 +1,20 @@ +Common MDIO bus properties. + +These are generic properties that can apply to any MDIO bus. + +Optional properties: +- reset-gpios: List of one or more GPIOs that control the RESET lines + of the PHYs on that MDIO bus. +- reset-delay-us: RESET pulse width as per PHY datasheet. + +Example : + + davinci_mdio: ethernet@0x5c030000 { + compatible = "ti,davinci_mdio"; + ti,hwmods = "davinci_mdio"; + reg = <0x5c030000 0x1000>; + #address-cells = <1>; + #size-cells = <0>; + reset-gpios = <&gpio2 5 GPIO_ACTIVE_LOW>; + reset-delay-us = <2>; /* PHY datasheet states 1uS min */ + }; diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c index fa7d51f..b353d99 100644 --- a/drivers/net/phy/mdio_bus.c +++ b/drivers/net/phy/mdio_bus.c @@ -22,8 +22,11 @@ #include <linux/init.h> #include <linux/delay.h> #include <linux/device.h> +#include <linux/gpio.h> +#include <linux/gpio/consumer.h> #include <linux/of_device.h> #include <linux/of_mdio.h> +#include <linux/of_gpio.h> #include <linux/netdevice.h> #include <linux/etherdevice.h> #include <linux/skbuff.h> @@ -307,6 +310,7 @@ int __mdiobus_register(struct mii_bus *bus, struct module *owner) { struct mdio_device *mdiodev; int i, err; + struct gpio_desc *gpiod; if (NULL == bus || NULL == bus->name || NULL == bus->read || NULL == bus->write) @@ -333,6 +337,24 @@ int __mdiobus_register(struct mii_bus *bus, struct module *owner) if (bus->reset) bus->reset(bus); + /* de-assert bus level PHY GPIO resets */ + for (i = 0; i < bus->num_reset_gpios; i++) { + gpiod = devm_gpiod_get_index(&bus->dev, "reset", i, + GPIOD_OUT_LOW); + if (IS_ERR(gpiod)) { + err = PTR_ERR(gpiod); + if (err != -ENOENT) { + pr_err("mii_bus %s couldn't get reset GPIO\n", + bus->id); + return err; + } + } else { + gpiod_set_value_cansleep(gpiod, 1); + udelay(bus->reset_delay_us); + gpiod_set_value_cansleep(gpiod, 0); + } + } + for (i = 0; i < PHY_MAX_ADDR; i++) { if ((bus->phy_mask & (1 << i)) == 0) { struct phy_device *phydev; diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c index 0b29798..7e4c80f 100644 --- a/drivers/of/of_mdio.c +++ b/drivers/of/of_mdio.c @@ -22,6 +22,8 @@ #include <linux/of_net.h> #include <linux/module.h> +#define DEFAULT_GPIO_RESET_DELAY 10 /* in microseconds */ + MODULE_AUTHOR("Grant Likely <grant.likely@secretlab.ca>"); MODULE_LICENSE("GPL"); @@ -221,6 +223,11 @@ int of_mdiobus_register(struct mii_bus *mdio, struct device_node *np) mdio->dev.of_node = np; + /* Get bus level PHY reset GPIO details */ + mdio->reset_delay_us = DEFAULT_GPIO_RESET_DELAY; + of_property_read_u32(np, "reset-delay-us", &mdio->reset_delay_us); + mdio->num_reset_gpios = of_gpio_named_count(np, "reset-gpios"); + /* Register the MDIO bus */ rc = mdiobus_register(mdio); if (rc) diff --git a/include/linux/phy.h b/include/linux/phy.h index 43a7748..80a6574 100644 --- a/include/linux/phy.h +++ b/include/linux/phy.h @@ -217,6 +217,11 @@ struct mii_bus { * matching its address */ int irq[PHY_MAX_ADDR]; + + /* GPIO reset pulse width in uS */ + int reset_delay_us; + /* Number of reset GPIOs */ + int num_reset_gpios; }; #define to_mii_bus(d) container_of(d, struct mii_bus, dev)
Some boards [1] leave the PHYs at an invalid state during system power-up or reset thus causing unreliability issues with the PHY which manifests as PHY not being detected or link not functional. To fix this, these PHYs need to be RESET via a GPIO connected to the PHY's RESET pin. Some boards have a single GPIO controlling the PHY RESET pin of all PHYs on the bus whereas some others have separate GPIOs controlling individual PHY RESETs. In both cases, the RESET de-assertion cannot be done in the PHY driver as the PHY will not probe till its reset is de-asserted. So do the RESET de-assertion in the MDIO bus driver. [1] - am572x-idk, am571x-idk, a437x-idk Signed-off-by: Roger Quadros <rogerq@ti.com> --- v2: - add device tree binding document (mdio.txt) - specify default reset delay in of_mdio.c instead of mdio_bus.c Documentation/devicetree/bindings/net/mdio.txt | 20 ++++++++++++++++++++ drivers/net/phy/mdio_bus.c | 22 ++++++++++++++++++++++ drivers/of/of_mdio.c | 7 +++++++ include/linux/phy.h | 5 +++++ 4 files changed, 54 insertions(+) create mode 100644 Documentation/devicetree/bindings/net/mdio.txt -- 2.7.4 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html