| Message ID | 1512390905-28094-1-git-send-email-geert+renesas@glider.be |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | [v4.1] phylib: Add device reset GPIO support | expand |
On 12/04/2017 01:35 PM, Geert Uytterhoeven wrote: > From: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> > > The PHY devices sometimes do have their reset signal (maybe even power > supply?) tied to some GPIO and sometimes it also does happen that a boot > loader does not leave it deasserted. So far this issue has been attacked > from (as I believe) a wrong angle: by teaching the MAC driver to manipulate > the GPIO in question; that solution, when applied to the device trees, led > to adding the PHY reset GPIO properties to the MAC device node, with one > exception: Cadence MACB driver which could handle the "reset-gpios" prop > in a PHY device subnode. I believe that the correct approach is to teach > the 'phylib' to get the MDIO device reset GPIO from the device tree node > corresponding to this device -- which this patch is doing... > > Note that I had to modify the AT803x PHY driver as it would stop working > otherwise -- it made use of the reset GPIO for its own purposes... > > Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> > Acked-by: Rob Herring <robh@kernel.org> > [geert: Propagate actual errors from fwnode_get_named_gpiod()] > [geert: Avoid destroying initial setup] > [geert: Consolidate GPIO descriptor acquiring code] > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > --- Successfully tested this patch on a i.MX6SOLO based board containing a LAN8710 PHY: Tested-by: Richard Leitner <richard.leitner@skidata.com>
Hello! On 12/04/2017 03:35 PM, Geert Uytterhoeven wrote: > From: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> > > The PHY devices sometimes do have their reset signal (maybe even power > supply?) tied to some GPIO and sometimes it also does happen that a boot > loader does not leave it deasserted. So far this issue has been attacked > from (as I believe) a wrong angle: by teaching the MAC driver to manipulate > the GPIO in question; that solution, when applied to the device trees, led > to adding the PHY reset GPIO properties to the MAC device node, with one > exception: Cadence MACB driver which could handle the "reset-gpios" prop > in a PHY device subnode. I believe that the correct approach is to teach > the 'phylib' to get the MDIO device reset GPIO from the device tree node > corresponding to this device -- which this patch is doing... > > Note that I had to modify the AT803x PHY driver as it would stop working > otherwise -- it made use of the reset GPIO for its own purposes... > > Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> > Acked-by: Rob Herring <robh@kernel.org> > [geert: Propagate actual errors from fwnode_get_named_gpiod()] > [geert: Avoid destroying initial setup] > [geert: Consolidate GPIO descriptor acquiring code] > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> [...] > diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c > index 2df7b62c1a36811e..8f8b7747c54bc478 100644 > --- a/drivers/net/phy/mdio_bus.c > +++ b/drivers/net/phy/mdio_bus.c [...] > @@ -48,9 +49,26 @@ > > int mdiobus_register_device(struct mdio_device *mdiodev) > { > + struct gpio_desc *gpiod = NULL; > + > if (mdiodev->bus->mdio_map[mdiodev->addr]) > return -EBUSY; > > + /* Deassert the optional reset signal */ Umm, but why deassert it here for such a short time? > + if (mdiodev->dev.of_node) > + gpiod = fwnode_get_named_gpiod(&mdiodev->dev.of_node->fwnode, > + "reset-gpios", 0, GPIOD_OUT_LOW, > + "PHY reset"); > + if (PTR_ERR(gpiod) == -ENOENT) > + gpiod = NULL; > + else if (IS_ERR(gpiod)) > + return PTR_ERR(gpiod); Hm, returning on error with reset deasserted? > + > + mdiodev->reset = gpiod; > + > + /* Assert the reset signal again */ > + mdio_device_reset(mdiodev, 1); > + > mdiodev->bus->mdio_map[mdiodev->addr] = mdiodev; > > return 0; [...] MBR, Sergei
On 12/07/2017 08:20 PM, Sergei Shtylyov wrote: >> The PHY devices sometimes do have their reset signal (maybe even power >> supply?) tied to some GPIO and sometimes it also does happen that a boot >> loader does not leave it deasserted. So far this issue has been attacked >> from (as I believe) a wrong angle: by teaching the MAC driver to manipulate >> the GPIO in question; that solution, when applied to the device trees, led >> to adding the PHY reset GPIO properties to the MAC device node, with one >> exception: Cadence MACB driver which could handle the "reset-gpios" prop >> in a PHY device subnode. I believe that the correct approach is to teach >> the 'phylib' to get the MDIO device reset GPIO from the device tree node >> corresponding to this device -- which this patch is doing... >> >> Note that I had to modify the AT803x PHY driver as it would stop working >> otherwise -- it made use of the reset GPIO for its own purposes... >> >> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> >> Acked-by: Rob Herring <robh@kernel.org> >> [geert: Propagate actual errors from fwnode_get_named_gpiod()] >> [geert: Avoid destroying initial setup] >> [geert: Consolidate GPIO descriptor acquiring code] >> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > [...] >> diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c >> index 2df7b62c1a36811e..8f8b7747c54bc478 100644 >> --- a/drivers/net/phy/mdio_bus.c >> +++ b/drivers/net/phy/mdio_bus.c > [...] >> @@ -48,9 +49,26 @@ >> int mdiobus_register_device(struct mdio_device *mdiodev) >> { >> + struct gpio_desc *gpiod = NULL; >> + >> if (mdiodev->bus->mdio_map[mdiodev->addr]) >> return -EBUSY; >> + /* Deassert the optional reset signal */ > > Umm, but why deassert it here for such a short time? > >> + if (mdiodev->dev.of_node) >> + gpiod = fwnode_get_named_gpiod(&mdiodev->dev.of_node->fwnode, >> + "reset-gpios", 0, GPIOD_OUT_LOW, >> + "PHY reset"); >> + if (PTR_ERR(gpiod) == -ENOENT) >> + gpiod = NULL; >> + else if (IS_ERR(gpiod)) >> + return PTR_ERR(gpiod); > > Hm, returning on error with reset deasserted? Oops, error means we couldn't drive the GPIO at all... The 1st question remains though... [...] MBR, Sergei
Hi Sergei, On Thu, Dec 7, 2017 at 6:20 PM, Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> wrote: > On 12/04/2017 03:35 PM, Geert Uytterhoeven wrote: >> From: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> >> The PHY devices sometimes do have their reset signal (maybe even power >> supply?) tied to some GPIO and sometimes it also does happen that a boot >> loader does not leave it deasserted. So far this issue has been attacked >> from (as I believe) a wrong angle: by teaching the MAC driver to >> manipulate >> the GPIO in question; that solution, when applied to the device trees, led >> to adding the PHY reset GPIO properties to the MAC device node, with one >> exception: Cadence MACB driver which could handle the "reset-gpios" prop >> in a PHY device subnode. I believe that the correct approach is to teach >> the 'phylib' to get the MDIO device reset GPIO from the device tree node >> corresponding to this device -- which this patch is doing... >> >> Note that I had to modify the AT803x PHY driver as it would stop working >> otherwise -- it made use of the reset GPIO for its own purposes... >> >> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> >> Acked-by: Rob Herring <robh@kernel.org> >> [geert: Propagate actual errors from fwnode_get_named_gpiod()] >> [geert: Avoid destroying initial setup] >> [geert: Consolidate GPIO descriptor acquiring code] >> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > > [...] >> >> diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c >> index 2df7b62c1a36811e..8f8b7747c54bc478 100644 >> --- a/drivers/net/phy/mdio_bus.c >> +++ b/drivers/net/phy/mdio_bus.c > > [...] >> >> @@ -48,9 +49,26 @@ >> int mdiobus_register_device(struct mdio_device *mdiodev) >> { >> + struct gpio_desc *gpiod = NULL; >> + >> if (mdiodev->bus->mdio_map[mdiodev->addr]) >> return -EBUSY; >> + /* Deassert the optional reset signal */ > > > Umm, but why deassert it here for such a short time? That's a consequence of moving it from drivers/of/of_mdio.c to here. Not that it was deasserted that much longer in drivers/of/of_mdio.c, though... Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Hello! On 12/08/2017 12:53 PM, Geert Uytterhoeven wrote: >> On 12/04/2017 03:35 PM, Geert Uytterhoeven wrote: >>> From: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> >>> The PHY devices sometimes do have their reset signal (maybe even power >>> supply?) tied to some GPIO and sometimes it also does happen that a boot >>> loader does not leave it deasserted. So far this issue has been attacked >>> from (as I believe) a wrong angle: by teaching the MAC driver to >>> manipulate >>> the GPIO in question; that solution, when applied to the device trees, led >>> to adding the PHY reset GPIO properties to the MAC device node, with one >>> exception: Cadence MACB driver which could handle the "reset-gpios" prop >>> in a PHY device subnode. I believe that the correct approach is to teach >>> the 'phylib' to get the MDIO device reset GPIO from the device tree node >>> corresponding to this device -- which this patch is doing... >>> >>> Note that I had to modify the AT803x PHY driver as it would stop working >>> otherwise -- it made use of the reset GPIO for its own purposes... >>> >>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> >>> Acked-by: Rob Herring <robh@kernel.org> >>> [geert: Propagate actual errors from fwnode_get_named_gpiod()] >>> [geert: Avoid destroying initial setup] >>> [geert: Consolidate GPIO descriptor acquiring code] >>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> >> >> [...] >>> >>> diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c >>> index 2df7b62c1a36811e..8f8b7747c54bc478 100644 >>> --- a/drivers/net/phy/mdio_bus.c >>> +++ b/drivers/net/phy/mdio_bus.c >> >> [...] >>> >>> @@ -48,9 +49,26 @@ >>> int mdiobus_register_device(struct mdio_device *mdiodev) >>> { >>> + struct gpio_desc *gpiod = NULL; >>> + >>> if (mdiodev->bus->mdio_map[mdiodev->addr]) >>> return -EBUSY; >>> + /* Deassert the optional reset signal */ >> >> >> Umm, but why deassert it here for such a short time? > > That's a consequence of moving it from drivers/of/of_mdio.c to here. Well, you shouldn't do code moves without some thinking. ;-) > Not that it was deasserted that much longer in drivers/of/of_mdio.c, though... There it had a reason, here I'm not seeing one. Perhaps using GPIOD_ASIS (or GPIOD_OUT_HIGH) instead of GPIOD_OUT_LOW and dropping mdio_device_reset(mdiodev, 1) afterwards would make more sense here? > Gr{oetje,eeting}s, > > Geert MBR, Sergei
diff --git a/Documentation/devicetree/bindings/net/phy.txt b/Documentation/devicetree/bindings/net/phy.txt index 77d0b2a61ffa96fc..c05479f5ac7cc837 100644 --- a/Documentation/devicetree/bindings/net/phy.txt +++ b/Documentation/devicetree/bindings/net/phy.txt @@ -53,6 +53,8 @@ Optional Properties: to ensure the integrated PHY is used. The absence of this property indicates the muxers should be configured so that the external PHY is used. +- reset-gpios: The GPIO phandle and specifier for the PHY reset signal. + Example: ethernet-phy@0 { diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c index de7dd6566df7a364..29da7a3c7a3761c0 100644 --- a/drivers/net/phy/at803x.c +++ b/drivers/net/phy/at803x.c @@ -71,7 +71,6 @@ MODULE_LICENSE("GPL"); struct at803x_priv { bool phy_reset:1; - struct gpio_desc *gpiod_reset; }; struct at803x_context { @@ -254,22 +253,11 @@ static int at803x_probe(struct phy_device *phydev) { struct device *dev = &phydev->mdio.dev; struct at803x_priv *priv; - struct gpio_desc *gpiod_reset; priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); if (!priv) return -ENOMEM; - if (phydev->drv->phy_id != ATH8030_PHY_ID) - goto does_not_require_reset_workaround; - - gpiod_reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW); - if (IS_ERR(gpiod_reset)) - return PTR_ERR(gpiod_reset); - - priv->gpiod_reset = gpiod_reset; - -does_not_require_reset_workaround: phydev->priv = priv; return 0; @@ -343,14 +331,14 @@ static void at803x_link_change_notify(struct phy_device *phydev) * cannot recover from by software. */ if (phydev->state == PHY_NOLINK) { - if (priv->gpiod_reset && !priv->phy_reset) { + if (phydev->mdio.reset && !priv->phy_reset) { struct at803x_context context; at803x_context_save(phydev, &context); - gpiod_set_value(priv->gpiod_reset, 1); + phy_device_reset(phydev, 1); msleep(1); - gpiod_set_value(priv->gpiod_reset, 0); + phy_device_reset(phydev, 0); msleep(1); at803x_context_restore(phydev, &context); diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c index 2df7b62c1a36811e..8f8b7747c54bc478 100644 --- a/drivers/net/phy/mdio_bus.c +++ b/drivers/net/phy/mdio_bus.c @@ -38,6 +38,7 @@ #include <linux/phy.h> #include <linux/io.h> #include <linux/uaccess.h> +#include <linux/gpio/consumer.h> #include <asm/irq.h> @@ -48,9 +49,26 @@ int mdiobus_register_device(struct mdio_device *mdiodev) { + struct gpio_desc *gpiod = NULL; + if (mdiodev->bus->mdio_map[mdiodev->addr]) return -EBUSY; + /* Deassert the optional reset signal */ + if (mdiodev->dev.of_node) + gpiod = fwnode_get_named_gpiod(&mdiodev->dev.of_node->fwnode, + "reset-gpios", 0, GPIOD_OUT_LOW, + "PHY reset"); + if (PTR_ERR(gpiod) == -ENOENT) + gpiod = NULL; + else if (IS_ERR(gpiod)) + return PTR_ERR(gpiod); + + mdiodev->reset = gpiod; + + /* Assert the reset signal again */ + mdio_device_reset(mdiodev, 1); + mdiodev->bus->mdio_map[mdiodev->addr] = mdiodev; return 0; @@ -420,6 +438,9 @@ void mdiobus_unregister(struct mii_bus *bus) if (!mdiodev) continue; + if (mdiodev->reset) + gpiod_put(mdiodev->reset); + mdiodev->device_remove(mdiodev); mdiodev->device_free(mdiodev); } diff --git a/drivers/net/phy/mdio_device.c b/drivers/net/phy/mdio_device.c index e24f28924af8953d..75d97dd9fb281704 100644 --- a/drivers/net/phy/mdio_device.c +++ b/drivers/net/phy/mdio_device.c @@ -12,6 +12,8 @@ #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt #include <linux/errno.h> +#include <linux/gpio.h> +#include <linux/gpio/consumer.h> #include <linux/init.h> #include <linux/interrupt.h> #include <linux/kernel.h> @@ -114,6 +116,13 @@ void mdio_device_remove(struct mdio_device *mdiodev) } EXPORT_SYMBOL(mdio_device_remove); +void mdio_device_reset(struct mdio_device *mdiodev, int value) +{ + if (mdiodev->reset) + gpiod_set_value(mdiodev->reset, value); +} +EXPORT_SYMBOL(mdio_device_reset); + /** * mdio_probe - probe an MDIO device * @dev: device to probe @@ -128,8 +137,16 @@ static int mdio_probe(struct device *dev) struct mdio_driver *mdiodrv = to_mdio_driver(drv); int err = 0; - if (mdiodrv->probe) + if (mdiodrv->probe) { + /* Deassert the reset signal */ + mdio_device_reset(mdiodev, 0); + err = mdiodrv->probe(mdiodev); + if (err) { + /* Assert the reset signal */ + mdio_device_reset(mdiodev, 1); + } + } return err; } @@ -140,9 +157,13 @@ static int mdio_remove(struct device *dev) struct device_driver *drv = mdiodev->dev.driver; struct mdio_driver *mdiodrv = to_mdio_driver(drv); - if (mdiodrv->remove) + if (mdiodrv->remove) { mdiodrv->remove(mdiodev); + /* Assert the reset signal */ + mdio_device_reset(mdiodev, 1); + } + return 0; } diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c index 8154fb706751e8ad..1de5e242b8b4cda3 100644 --- a/drivers/net/phy/phy_device.c +++ b/drivers/net/phy/phy_device.c @@ -632,6 +632,9 @@ int phy_device_register(struct phy_device *phydev) if (err) return err; + /* Deassert the reset signal */ + phy_device_reset(phydev, 0); + /* Run all of the fixups for this PHY */ err = phy_scan_fixups(phydev); if (err) { @@ -650,6 +653,9 @@ int phy_device_register(struct phy_device *phydev) return 0; out: + /* Assert the reset signal */ + phy_device_reset(phydev, 1); + mdiobus_unregister_device(&phydev->mdio); return err; } @@ -666,6 +672,10 @@ EXPORT_SYMBOL(phy_device_register); void phy_device_remove(struct phy_device *phydev) { device_del(&phydev->mdio.dev); + + /* Assert the reset signal */ + phy_device_reset(phydev, 1); + mdiobus_unregister_device(&phydev->mdio); } EXPORT_SYMBOL(phy_device_remove); @@ -849,6 +859,9 @@ int phy_init_hw(struct phy_device *phydev) { int ret = 0; + /* Deassert the reset signal */ + phy_device_reset(phydev, 0); + if (!phydev->drv || !phydev->drv->config_init) return 0; @@ -1126,6 +1139,9 @@ void phy_detach(struct phy_device *phydev) put_device(&phydev->mdio.dev); if (ndev_owner != bus->owner) module_put(bus->owner); + + /* Assert the reset signal */ + phy_device_reset(phydev, 1); } EXPORT_SYMBOL(phy_detach); @@ -1811,8 +1827,16 @@ static int phy_probe(struct device *dev) /* Set the state to READY by default */ phydev->state = PHY_READY; - if (phydev->drv->probe) + if (phydev->drv->probe) { + /* Deassert the reset signal */ + phy_device_reset(phydev, 0); + err = phydev->drv->probe(phydev); + if (err) { + /* Assert the reset signal */ + phy_device_reset(phydev, 1); + } + } mutex_unlock(&phydev->lock); @@ -1829,8 +1853,12 @@ static int phy_remove(struct device *dev) phydev->state = PHY_DOWN; mutex_unlock(&phydev->lock); - if (phydev->drv && phydev->drv->remove) + if (phydev->drv && phydev->drv->remove) { phydev->drv->remove(phydev); + + /* Assert the reset signal */ + phy_device_reset(phydev, 1); + } phydev->drv = NULL; return 0; diff --git a/include/linux/mdio.h b/include/linux/mdio.h index ca08ab16ecdc9b78..92d4e55ffe675637 100644 --- a/include/linux/mdio.h +++ b/include/linux/mdio.h @@ -12,6 +12,7 @@ #include <uapi/linux/mdio.h> #include <linux/mod_devicetable.h> +struct gpio_desc; struct mii_bus; /* Multiple levels of nesting are possible. However typically this is @@ -39,6 +40,7 @@ struct mdio_device { /* Bus address of the MDIO device (0-31) */ int addr; int flags; + struct gpio_desc *reset; }; #define to_mdio_device(d) container_of(d, struct mdio_device, dev) @@ -71,6 +73,7 @@ void mdio_device_free(struct mdio_device *mdiodev); struct mdio_device *mdio_device_create(struct mii_bus *bus, int addr); int mdio_device_register(struct mdio_device *mdiodev); void mdio_device_remove(struct mdio_device *mdiodev); +void mdio_device_reset(struct mdio_device *mdiodev, int value); int mdio_driver_register(struct mdio_driver *drv); void mdio_driver_unregister(struct mdio_driver *drv); int mdio_device_bus_match(struct device *dev, struct device_driver *drv); diff --git a/include/linux/phy.h b/include/linux/phy.h index 4962af37722ac2ea..6106ed915841c97e 100644 --- a/include/linux/phy.h +++ b/include/linux/phy.h @@ -841,6 +841,11 @@ int phy_aneg_done(struct phy_device *phydev); int phy_stop_interrupts(struct phy_device *phydev); int phy_restart_aneg(struct phy_device *phydev); +static inline void phy_device_reset(struct phy_device *phydev, int value) +{ + mdio_device_reset(&phydev->mdio, value); +} + #define phydev_err(_phydev, format, args...) \ dev_err(&_phydev->mdio.dev, format, ##args)