Message ID | f372db02-a919-55b4-9200-20726cd13482@ti.com |
---|---|
State | Changes Requested, archived |
Delegated to: | David Miller |
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
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!
> > + 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? Hi Florian Yes, it does. The gpiod_ API takes care of that, if you set the flag GPIO_ACTIVE_LOW in the device tree blob. This is one of the improvements over the gpio_ API. Andrew
Hi Florian, On 21/04/17 04:23, Florian Fainelli wrote: > 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. No problem at all. > >> +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. OK. > >> + >> + 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? Sure. > >> + 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? Al right. > >> + } >> + } 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? > Yes. The polarity needs to be specified at DT as explained by Andrew already. cheers, -roger
diff --git a/Documentation/devicetree/bindings/net/mdio.txt b/Documentation/devicetree/bindings/net/mdio.txt new file mode 100644 index 0000000..3517369 --- /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 */ + + ethphy0: ethernet-phy@1 { + reg = <1>; + }; + + ethphy1: ethernet-phy@3 { + reg = <3>; + }; + }; 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> --- v3: - added more information in the DT binding document. 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 | 33 ++++++++++++++++++++++++++ drivers/net/phy/mdio_bus.c | 22 +++++++++++++++++ drivers/of/of_mdio.c | 7 ++++++ include/linux/phy.h | 5 ++++ 4 files changed, 67 insertions(+) create mode 100644 Documentation/devicetree/bindings/net/mdio.txt