| Message ID | 3641492.klKRrvS8tr@wasted.cogentembedded.com |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
On Fri, Apr 29, 2016 at 01:12:54AM +0300, 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 as it made use of the reset GPIO for its own purposes... > > Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> > > --- > Changes in version 2: > - reformatted the changelog; > - resolved rejects, refreshed the patch. > > Documentation/devicetree/bindings/net/phy.txt | 2 + > Documentation/devicetree/bindings/net/phy.txt | 2 + Acked-by: Rob Herring <robh@kernel.org> > drivers/net/phy/at803x.c | 19 ++------------ > drivers/net/phy/mdio_bus.c | 4 +++ > drivers/net/phy/mdio_device.c | 27 +++++++++++++++++++-- > drivers/net/phy/phy_device.c | 33 ++++++++++++++++++++++++-- > drivers/of/of_mdio.c | 16 ++++++++++++ > include/linux/mdio.h | 3 ++ > include/linux/phy.h | 5 +++ > 8 files changed, 89 insertions(+), 20 deletions(-) > > Index: net-next/Documentation/devicetree/bindings/net/phy.txt > =================================================================== > --- net-next.orig/Documentation/devicetree/bindings/net/phy.txt > +++ net-next/Documentation/devicetree/bindings/net/phy.txt > @@ -35,6 +35,8 @@ Optional Properties: > - broken-turn-around: If set, indicates the PHY device does not correctly > release the turn around line low at the end of a MDIO transaction. > > +- reset-gpios: The GPIO phandle and specifier for the PHY reset signal. > + > Example: > > ethernet-phy@0 { > Index: net-next/drivers/net/phy/at803x.c > =================================================================== > --- net-next.orig/drivers/net/phy/at803x.c > +++ net-next/drivers/net/phy/at803x.c > @@ -65,7 +65,6 @@ MODULE_LICENSE("GPL"); > > struct at803x_priv { > bool phy_reset:1; > - struct gpio_desc *gpiod_reset; > }; > > struct at803x_context { > @@ -271,22 +270,10 @@ static int at803x_probe(struct phy_devic > { > 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; > @@ -361,14 +348,14 @@ static void at803x_link_change_notify(st > */ > if (phydev->drv->phy_id == ATH8030_PHY_ID) { > 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); > Index: net-next/drivers/net/phy/mdio_bus.c > =================================================================== > --- net-next.orig/drivers/net/phy/mdio_bus.c > +++ net-next/drivers/net/phy/mdio_bus.c > @@ -35,6 +35,7 @@ > #include <linux/phy.h> > #include <linux/io.h> > #include <linux/uaccess.h> > +#include <linux/gpio/consumer.h> > > #include <asm/irq.h> > > @@ -371,6 +372,9 @@ void mdiobus_unregister(struct mii_bus * > if (!mdiodev) > continue; > > + if (mdiodev->reset) > + gpiod_put(mdiodev->reset); > + > mdiodev->device_remove(mdiodev); > mdiodev->device_free(mdiodev); > } > Index: net-next/drivers/net/phy/mdio_device.c > =================================================================== > --- net-next.orig/drivers/net/phy/mdio_device.c > +++ net-next/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> > @@ -103,6 +105,13 @@ void mdio_device_remove(struct mdio_devi > } > 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 > @@ -117,9 +126,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); > > + /* Assert the reset signal */ > + mdio_device_reset(mdiodev, 1); > + } > + > return err; > } > > @@ -129,9 +145,16 @@ static int mdio_remove(struct device *de > struct device_driver *drv = mdiodev->dev.driver; > struct mdio_driver *mdiodrv = to_mdio_driver(drv); > > - if (mdiodrv->remove) > + if (mdiodrv->remove) { > + /* Deassert the reset signal */ > + mdio_device_reset(mdiodev, 0); > + > mdiodrv->remove(mdiodev); > > + /* Assert the reset signal */ > + mdio_device_reset(mdiodev, 1); > + } > + > return 0; > } > > Index: net-next/drivers/net/phy/phy_device.c > =================================================================== > --- net-next.orig/drivers/net/phy/phy_device.c > +++ net-next/drivers/net/phy/phy_device.c > @@ -589,6 +589,9 @@ int phy_device_register(struct phy_devic > 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) { > @@ -604,9 +607,15 @@ int phy_device_register(struct phy_devic > goto out; > } > > + /* Assert the reset signal */ > + phy_device_reset(phydev, 1); > + > return 0; > > out: > + /* Assert the reset signal */ > + phy_device_reset(phydev, 1); > + > mdiobus_unregister_device(&phydev->mdio); > return err; > } > @@ -792,6 +801,9 @@ int phy_init_hw(struct phy_device *phyde > { > int ret = 0; > > + /* Deassert the reset signal */ > + phy_device_reset(phydev, 0); > + > if (!phydev->drv || !phydev->drv->config_init) > return 0; > > @@ -997,6 +1009,9 @@ void phy_detach(struct phy_device *phyde > > put_device(&phydev->mdio.dev); > module_put(bus->owner); > + > + /* Assert the reset signal */ > + phy_device_reset(phydev, 1); > } > EXPORT_SYMBOL(phy_detach); > > @@ -1596,9 +1611,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); > > + /* Assert the reset signal */ > + phy_device_reset(phydev, 1); > + } > + > mutex_unlock(&phydev->lock); > > return err; > @@ -1612,8 +1634,15 @@ static int phy_remove(struct device *dev > phydev->state = PHY_DOWN; > mutex_unlock(&phydev->lock); > > - if (phydev->drv->remove) > + if (phydev->drv->remove) { > + /* Deassert the reset signal */ > + phy_device_reset(phydev, 0); > + > phydev->drv->remove(phydev); > + > + /* Assert the reset signal */ > + phy_device_reset(phydev, 1); > + } > phydev->drv = NULL; > > return 0; > Index: net-next/drivers/of/of_mdio.c > =================================================================== > --- net-next.orig/drivers/of/of_mdio.c > +++ net-next/drivers/of/of_mdio.c > @@ -44,6 +44,7 @@ static int of_get_phy_id(struct device_n > static void of_mdiobus_register_phy(struct mii_bus *mdio, > struct device_node *child, u32 addr) > { > + struct gpio_desc *gpiod; > struct phy_device *phy; > bool is_c45; > int rc; > @@ -52,10 +53,17 @@ static void of_mdiobus_register_phy(stru > is_c45 = of_device_is_compatible(child, > "ethernet-phy-ieee802.3-c45"); > > + gpiod = fwnode_get_named_gpiod(&child->fwnode, "reset-gpios"); > + /* Deassert the reset signal */ > + if (!IS_ERR(gpiod)) > + gpiod_direction_output(gpiod, 0); > if (!is_c45 && !of_get_phy_id(child, &phy_id)) > phy = phy_device_create(mdio, addr, phy_id, 0, NULL); > else > phy = get_phy_device(mdio, addr, is_c45); > + /* Assert the reset signal again */ > + if (!IS_ERR(gpiod)) > + gpiod_set_value(gpiod, 1); > if (IS_ERR(phy)) > return; > > @@ -75,6 +83,9 @@ static void of_mdiobus_register_phy(stru > of_node_get(child); > phy->mdio.dev.of_node = child; > > + if (!IS_ERR(gpiod)) > + phy->mdio.reset = gpiod; > + > /* All data is now stored in the phy struct; > * register it */ > rc = phy_device_register(phy); > @@ -92,6 +103,7 @@ static void of_mdiobus_register_device(s > struct device_node *child, u32 addr) > { > struct mdio_device *mdiodev; > + struct gpio_desc *gpiod; > int rc; > > mdiodev = mdio_device_create(mdio, addr); > @@ -104,6 +116,10 @@ static void of_mdiobus_register_device(s > of_node_get(child); > mdiodev->dev.of_node = child; > > + gpiod = fwnode_get_named_gpiod(&child->fwnode, "reset-gpios"); > + if (!IS_ERR(gpiod)) > + mdiodev->reset = gpiod; > + > /* All data is now stored in the mdiodev struct; register it. */ > rc = mdio_device_register(mdiodev); > if (rc) { > Index: net-next/include/linux/mdio.h > =================================================================== > --- net-next.orig/include/linux/mdio.h > +++ net-next/include/linux/mdio.h > @@ -11,6 +11,7 @@ > > #include <uapi/linux/mdio.h> > > +struct gpio_desc; > struct mii_bus; > > /* Multiple levels of nesting are possible. However typically this is > @@ -37,6 +38,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) > > @@ -69,6 +71,7 @@ void mdio_device_free(struct mdio_device > 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); > > Index: net-next/include/linux/phy.h > =================================================================== > --- net-next.orig/include/linux/phy.h > +++ net-next/include/linux/phy.h > @@ -769,6 +769,11 @@ static inline int phy_read_status(struct > return phydev->drv->read_status(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) > >
On 04/28/2016 03:12 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 as it made use of the reset GPIO for its own purposes... > > Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> This looks good to me: Acked-by: Florian Fainelli <f.fainelli@gmail.com> Can you follow up with changes in phy_{suspend,resume} if that is also an use case that you have?
Hello. On 05/10/2016 09:32 PM, Florian Fainelli 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 as it made use of the reset GPIO for its own purposes... >> >> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> > > This looks good to me: > > Acked-by: Florian Fainelli <f.fainelli@gmail.com> Thank you! I'll send v3 without [RFT] then. > Can you follow up with changes in phy_{suspend,resume} I'm not sure what changes you mean -- powering down the PHYs? > if that is also > an use case that you have? No, I'm not into power management. MBR, Sergei
On 05/10/2016 12:11 PM, Sergei Shtylyov wrote: > Hello. > > On 05/10/2016 09:32 PM, Florian Fainelli 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 as it made use of the reset GPIO for its own purposes... >>> >>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> >> >> This looks good to me: >> >> Acked-by: Florian Fainelli <f.fainelli@gmail.com> > > Thank you! I'll send v3 without [RFT] then. > >> Can you follow up with changes in phy_{suspend,resume} > > I'm not sure what changes you mean -- powering down the PHYs? Yes, powering down, conversely up the PHY. The whole point of putting this in PHYLIB is to be able to perform things like that. We do not need this right now, but it would be nice if we saw that materialize at some point.
Hello Sergei, [we already talked about this patch in #armlinux, I'm now just forwarding my comments on the list. Background was that I sent an easier and less complete patch with the same idea. See http://patchwork.ozlabs.org/patch/621418/] [added Linus Walleij to Cc, there is a question for you/him below] On Fri, Apr 29, 2016 at 01:12:54AM +0300, Sergei Shtylyov wrote: > --- net-next.orig/Documentation/devicetree/bindings/net/phy.txt > +++ net-next/Documentation/devicetree/bindings/net/phy.txt > @@ -35,6 +35,8 @@ Optional Properties: > - broken-turn-around: If set, indicates the PHY device does not correctly > release the turn around line low at the end of a MDIO transaction. > > +- reset-gpios: The GPIO phandle and specifier for the PHY reset signal. > + > Example: > > ethernet-phy@0 { This is great. > Index: net-next/drivers/net/phy/at803x.c > =================================================================== > --- net-next.orig/drivers/net/phy/at803x.c > +++ net-next/drivers/net/phy/at803x.c > @@ -65,7 +65,6 @@ MODULE_LICENSE("GPL"); > [...] My patch breaks this driver. I wasn't aware of it. > [...] > Index: net-next/drivers/net/phy/mdio_device.c > =================================================================== > --- net-next.orig/drivers/net/phy/mdio_device.c > +++ net-next/drivers/net/phy/mdio_device.c > [...] > @@ -103,6 +105,13 @@ void mdio_device_remove(struct mdio_devi > } > EXPORT_SYMBOL(mdio_device_remove); > > +void mdio_device_reset(struct mdio_device *mdiodev, int value) > +{ > + if (mdiodev->reset) > + gpiod_set_value(mdiodev->reset, value); Before v4.6-rc1~108^2~91 it was not necessary to check for the first parameter being non-NULL before calling gpiod_set_value. Linus, did you change this on purpose? > +} > +EXPORT_SYMBOL(mdio_device_reset); > + > /** > * mdio_probe - probe an MDIO device > * @dev: device to probe > @@ -117,9 +126,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); > > + /* Assert the reset signal */ > + mdio_device_reset(mdiodev, 1); I wonder if it's safe to do this in general. What if ->probe does something with the phy that is lost by resetting but that is relied on later? > + } > + > return err; > } > [...] > Index: net-next/drivers/of/of_mdio.c > =================================================================== > --- net-next.orig/drivers/of/of_mdio.c > +++ net-next/drivers/of/of_mdio.c > @@ -44,6 +44,7 @@ static int of_get_phy_id(struct device_n > static void of_mdiobus_register_phy(struct mii_bus *mdio, > struct device_node *child, u32 addr) > { > + struct gpio_desc *gpiod; > struct phy_device *phy; > bool is_c45; > int rc; > @@ -52,10 +53,17 @@ static void of_mdiobus_register_phy(stru > is_c45 = of_device_is_compatible(child, > "ethernet-phy-ieee802.3-c45"); > > + gpiod = fwnode_get_named_gpiod(&child->fwnode, "reset-gpios"); > + /* Deassert the reset signal */ > + if (!IS_ERR(gpiod)) > + gpiod_direction_output(gpiod, 0); This is wrong I think. You must only ignore -ENODEV, all other error codes should be passed to the caller. (I see that's not trivial because of_mdiobus_register_phy returns void.) In my patch I used devm_gpiod_get_array which has the nice property that I can already pass GPIOD_OUT_LOW in flags. Also this binds the lifetime of the gpio to the device which is nice and IMHO the right direction for the phylib (i.e. better embracing of the device model). This cannot be used here easily however because there is no struct device yet and this is only created after the phy id is determined. The phy id is either read from the device tree or must be read from the phy which might fail if reset is not deasserted. Principally there is no reason however that the phy_id must be known before the struct device is created however. Best regards Uwe
Hello. On 05/12/2016 09:42 PM, Uwe Kleine-König wrote: > [we already talked about this patch in #armlinux, I'm now just > forwarding my comments on the list. Background was that I sent an easier > and less complete patch with the same idea. See > http://patchwork.ozlabs.org/patch/621418/] > > [added Linus Walleij to Cc, there is a question for you/him below] > > On Fri, Apr 29, 2016 at 01:12:54AM +0300, Sergei Shtylyov wrote: >> --- net-next.orig/Documentation/devicetree/bindings/net/phy.txt >> +++ net-next/Documentation/devicetree/bindings/net/phy.txt >> @@ -35,6 +35,8 @@ Optional Properties: >> - broken-turn-around: If set, indicates the PHY device does not correctly >> release the turn around line low at the end of a MDIO transaction. >> >> +- reset-gpios: The GPIO phandle and specifier for the PHY reset signal. >> + >> Example: >> >> ethernet-phy@0 { > > This is great. > >> Index: net-next/drivers/net/phy/at803x.c >> =================================================================== >> --- net-next.orig/drivers/net/phy/at803x.c >> +++ net-next/drivers/net/phy/at803x.c >> @@ -65,7 +65,6 @@ MODULE_LICENSE("GPL"); >> [...] > > My patch breaks this driver. I wasn't aware of it. I tried to be as careful as I could but still it looks that I didn't succeed at that too... [...] >> Index: net-next/drivers/net/phy/mdio_device.c >> =================================================================== >> --- net-next.orig/drivers/net/phy/mdio_device.c >> +++ net-next/drivers/net/phy/mdio_device.c [...] >> @@ -117,9 +126,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); >> >> + /* Assert the reset signal */ >> + mdio_device_reset(mdiodev, 1); > > I wonder if it's safe to do this in general. What if ->probe does > something with the phy that is lost by resetting but that is relied on > later? Well, I thought that config_init() method is designed for that but indeed the LXT driver writes to BMCR in its probe() method and hence is broken. Thank you for noticing... [...] >> Index: net-next/drivers/of/of_mdio.c >> =================================================================== >> --- net-next.orig/drivers/of/of_mdio.c >> +++ net-next/drivers/of/of_mdio.c >> @@ -44,6 +44,7 @@ static int of_get_phy_id(struct device_n >> static void of_mdiobus_register_phy(struct mii_bus *mdio, >> struct device_node *child, u32 addr) >> { >> + struct gpio_desc *gpiod; >> struct phy_device *phy; >> bool is_c45; >> int rc; >> @@ -52,10 +53,17 @@ static void of_mdiobus_register_phy(stru >> is_c45 = of_device_is_compatible(child, >> "ethernet-phy-ieee802.3-c45"); >> >> + gpiod = fwnode_get_named_gpiod(&child->fwnode, "reset-gpios"); >> + /* Deassert the reset signal */ >> + if (!IS_ERR(gpiod)) >> + gpiod_direction_output(gpiod, 0); > > This is wrong I think. You must only ignore -ENODEV, all other error At least -ENOSYS should also be ignored (it's returned when gpiolib is not configured), right? When does -ENODEV gets returned (it's not easy to follow)? > codes should be passed to the caller. The caller doesn't care anyway... > (I see that's not trivial because > of_mdiobus_register_phy returns void.) I've made this function *void* in net-next. > In my patch I used devm_gpiod_get_array which has the nice property that > I can already pass GPIOD_OUT_LOW in flags. Also this binds the lifetime > of the gpio to the device which is nice and IMHO the right direction for > the phylib (i.e. better embracing of the device model). > > This cannot be used here easily however because there is no struct > device yet and this is only created after the phy id is determined. Your last patch [1] didn't make use of the managed device API (devm) either, I didn't quite get to the bottom of that... > The > phy id is either read from the device tree or must be read from the phy > which might fail if reset is not deasserted. > Principally there is no reason however that the phy_id must be known > before the struct device is created however. It's just that the code is cleaner that way... [1] http://paste.debian.net/683630/ > Best regards > Uwe MBR, Sergei
> >>+ gpiod = fwnode_get_named_gpiod(&child->fwnode, "reset-gpios"); > >>+ /* Deassert the reset signal */ > >>+ if (!IS_ERR(gpiod)) > >>+ gpiod_direction_output(gpiod, 0); > > > >This is wrong I think. You must only ignore -ENODEV, all other error > > At least -ENOSYS should also be ignored (it's returned when > gpiolib is not configured), right? When does -ENODEV gets returned > (it's not easy to follow)? > > >codes should be passed to the caller. > > The caller doesn't care anyway... It should do. What if fwnode_get_named_gpiod() returns -EPROBE_DEFER because the GPIO driver has not been loaded yet? Andrew
Hello Sergei, On Fri, May 13, 2016 at 12:35:50AM +0300, Sergei Shtylyov wrote: > On 05/12/2016 09:42 PM, Uwe Kleine-König wrote: > >>Index: net-next/drivers/net/phy/mdio_device.c > >>=================================================================== > >>--- net-next.orig/drivers/net/phy/mdio_device.c > >>+++ net-next/drivers/net/phy/mdio_device.c > [...] > >>@@ -117,9 +126,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); > >> > >>+ /* Assert the reset signal */ > >>+ mdio_device_reset(mdiodev, 1); > > > >I wonder if it's safe to do this in general. What if ->probe does > >something with the phy that is lost by resetting but that is relied on > >later? > > Well, I thought that config_init() method is designed for that but indeed > the LXT driver writes to BMCR in its probe() method and hence is broken. > Thank you for noticing... > > [...] > >>Index: net-next/drivers/of/of_mdio.c > >>=================================================================== > >>--- net-next.orig/drivers/of/of_mdio.c > >>+++ net-next/drivers/of/of_mdio.c > >>@@ -44,6 +44,7 @@ static int of_get_phy_id(struct device_n > >> static void of_mdiobus_register_phy(struct mii_bus *mdio, > >> struct device_node *child, u32 addr) > >> { > >>+ struct gpio_desc *gpiod; > >> struct phy_device *phy; > >> bool is_c45; > >> int rc; > >>@@ -52,10 +53,17 @@ static void of_mdiobus_register_phy(stru > >> is_c45 = of_device_is_compatible(child, > >> "ethernet-phy-ieee802.3-c45"); > >> > >>+ gpiod = fwnode_get_named_gpiod(&child->fwnode, "reset-gpios"); > >>+ /* Deassert the reset signal */ > >>+ if (!IS_ERR(gpiod)) > >>+ gpiod_direction_output(gpiod, 0); > > > >This is wrong I think. You must only ignore -ENODEV, all other error > > At least -ENOSYS should also be ignored (it's returned when gpiolib is > not configured), right? No, that's a common misconception. If GPIOLIB is off you cannot determine if dt specified a reset gpio. So you have the choice between: a) ignore -ENOSYS and so don't handle the reset line in the cases where it's necessary probably yielding an "Error: phy not found" message. b) fail to probe even if a reset handling isn't necessary, yielding "Error: failed to get hold of reset gpio". I say b) is the better one. It's easier to debug because the error message is better, GPIOLIB is enabled in most cases anyhow (still maybe select it?) and it's ensured that we're operating in the limits of the hardware specs (maybe a phy returns a random value when a register is read while reset is applied?). > When does -ENODEV gets returned (it's not easy to follow)? I don't know for sure for fwnode_get_named_gpiod, but the gpiod_get*() family returns -ENODEV if the node doesn't have a reset-gpio property. > >codes should be passed to the caller. > > The caller doesn't care anyway... > > >(I see that's not trivial because > >of_mdiobus_register_phy returns void.) > > I've made this function *void* in net-next. I'd say this is a step in the wrong direction. For example this makes it impossible to handle the case where the phy should be probed, the gpio driver isn't available yet, though. Normally you'd want to return -EPROBE_DEFER in this case and retry probing later. > >In my patch I used devm_gpiod_get_array which has the nice property that > >I can already pass GPIOD_OUT_LOW in flags. Also this binds the lifetime > >of the gpio to the device which is nice and IMHO the right direction for > >the phylib (i.e. better embracing of the device model). > > > >This cannot be used here easily however because there is no struct > >device yet and this is only created after the phy id is determined. > > Your last patch [1] didn't make use of the managed device API (devm) > either, I didn't quite get to the bottom of that... Right, devm didn't work. My patch was a prototype for a way that allowed to bind the lifetime of the gpio to the device. This might be longer than the call to mdiobus_unregister. See the problems that i2c handles because it doesn't handle lifetimes correctly in drivers/i2c/i2c-core.c at the end of i2c_del_adapter. > >The phy id is either read from the device tree or must be read from > >the phy which might fail if reset is not deasserted. > > >Principally there is no reason however that the phy_id must be known > >before the struct device is created however. > > It's just that the code is cleaner that way... I don't agree, I don't see that determine_phyid() allocate_device() is better than allocate_device() determine_phyid() . The former is maybe easier because that's the status quo and it doesn't need patching. But IMHO the result is on a similar level of cleanliness. Best regards Uwe > [1] http://paste.debian.net/683630/
Hi Sergei, On 12/05/16 21:42, Uwe Kleine-König wrote: > Hello Sergei, > > [we already talked about this patch in #armlinux, I'm now just > forwarding my comments on the list. Background was that I sent an easier > and less complete patch with the same idea. See > http://patchwork.ozlabs.org/patch/621418/] > > [added Linus Walleij to Cc, there is a question for you/him below] > > On Fri, Apr 29, 2016 at 01:12:54AM +0300, Sergei Shtylyov wrote: >> --- net-next.orig/Documentation/devicetree/bindings/net/phy.txt >> +++ net-next/Documentation/devicetree/bindings/net/phy.txt >> @@ -35,6 +35,8 @@ Optional Properties: >> - broken-turn-around: If set, indicates the PHY device does not correctly >> release the turn around line low at the end of a MDIO transaction. >> >> +- reset-gpios: The GPIO phandle and specifier for the PHY reset signal. >> + >> Example: >> >> ethernet-phy@0 { > > This is great. > >> Index: net-next/drivers/net/phy/at803x.c >> =================================================================== >> --- net-next.orig/drivers/net/phy/at803x.c >> +++ net-next/drivers/net/phy/at803x.c >> @@ -65,7 +65,6 @@ MODULE_LICENSE("GPL"); >> [...] > > My patch breaks this driver. I wasn't aware of it. > >> [...] >> Index: net-next/drivers/net/phy/mdio_device.c >> =================================================================== >> --- net-next.orig/drivers/net/phy/mdio_device.c >> +++ net-next/drivers/net/phy/mdio_device.c >> [...] >> @@ -103,6 +105,13 @@ void mdio_device_remove(struct mdio_devi >> } >> EXPORT_SYMBOL(mdio_device_remove); >> >> +void mdio_device_reset(struct mdio_device *mdiodev, int value) >> +{ >> + if (mdiodev->reset) >> + gpiod_set_value(mdiodev->reset, value); > > Before v4.6-rc1~108^2~91 it was not necessary to check for the first > parameter being non-NULL before calling gpiod_set_value. Linus, did you > change this on purpose? > >> +} >> +EXPORT_SYMBOL(mdio_device_reset); >> + >> /** >> * mdio_probe - probe an MDIO device >> * @dev: device to probe >> @@ -117,9 +126,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); >> >> + /* Assert the reset signal */ >> + mdio_device_reset(mdiodev, 1); > > I wonder if it's safe to do this in general. What if ->probe does > something with the phy that is lost by resetting but that is relied on > later? mdio_probe is called for non PHY devices only, right? I'm a bit lost as to why we're de-asserting reset at multiple places. i.e. mdio_probe(), phy_device_register(), phy_init_hw(), phy_probe(), of_mdiobus_register_phy(). Isn't it simpler to just de-assert it once at the topmost level? i.e. of_mdiobus_register_device() f and of_mdiobus_register_phy()? Also, how about issuing a reset pulse instead of just de-asserting it. This would tackle the case where PHY is in invalid state with reset already de-asserted. Another issue is that on some boards we have one reset line tied to multiple PHYs. How do we prevent multiple resets being taking place when each of the PHYs are registered? Do we just specify the reset line only for one PHY in the DT or can we have the reset gpio in the mdio_bus node for such case? cheers, -roger > >> + } >> + >> return err; >> } >> [...] >> Index: net-next/drivers/of/of_mdio.c >> =================================================================== >> --- net-next.orig/drivers/of/of_mdio.c >> +++ net-next/drivers/of/of_mdio.c >> @@ -44,6 +44,7 @@ static int of_get_phy_id(struct device_n >> static void of_mdiobus_register_phy(struct mii_bus *mdio, >> struct device_node *child, u32 addr) >> { >> + struct gpio_desc *gpiod; >> struct phy_device *phy; >> bool is_c45; >> int rc; >> @@ -52,10 +53,17 @@ static void of_mdiobus_register_phy(stru >> is_c45 = of_device_is_compatible(child, >> "ethernet-phy-ieee802.3-c45"); >> >> + gpiod = fwnode_get_named_gpiod(&child->fwnode, "reset-gpios"); >> + /* Deassert the reset signal */ >> + if (!IS_ERR(gpiod)) >> + gpiod_direction_output(gpiod, 0); > > This is wrong I think. You must only ignore -ENODEV, all other error > codes should be passed to the caller. (I see that's not trivial because > of_mdiobus_register_phy returns void.) > > In my patch I used devm_gpiod_get_array which has the nice property that > I can already pass GPIOD_OUT_LOW in flags. Also this binds the lifetime > of the gpio to the device which is nice and IMHO the right direction for > the phylib (i.e. better embracing of the device model). > > This cannot be used here easily however because there is no struct > device yet and this is only created after the phy id is determined. The > phy id is either read from the device tree or must be read from the phy > which might fail if reset is not deasserted. > > Principally there is no reason however that the phy_id must be known > before the struct device is created however. > > Best regards > Uwe >
Hello. On 05/13/2016 12:35 AM, Sergei Shtylyov wrote: >> [we already talked about this patch in #armlinux, I'm now just >> forwarding my comments on the list. Background was that I sent an easier >> and less complete patch with the same idea. See >> http://patchwork.ozlabs.org/patch/621418/] >> >> [added Linus Walleij to Cc, there is a question for you/him below] >> >> On Fri, Apr 29, 2016 at 01:12:54AM +0300, Sergei Shtylyov wrote: >>> --- net-next.orig/Documentation/devicetree/bindings/net/phy.txt >>> +++ net-next/Documentation/devicetree/bindings/net/phy.txt >>> @@ -35,6 +35,8 @@ Optional Properties: >>> - broken-turn-around: If set, indicates the PHY device does not correctly >>> release the turn around line low at the end of a MDIO transaction. >>> >>> +- reset-gpios: The GPIO phandle and specifier for the PHY reset signal. >>> + >>> Example: >>> >>> ethernet-phy@0 { >> >> This is great. >> >>> Index: net-next/drivers/net/phy/at803x.c >>> =================================================================== >>> --- net-next.orig/drivers/net/phy/at803x.c >>> +++ net-next/drivers/net/phy/at803x.c >>> @@ -65,7 +65,6 @@ MODULE_LICENSE("GPL"); >>> [...] >> >> My patch breaks this driver. I wasn't aware of it. > > I tried to be as careful as I could but still it looks that I didn't > succeed at that too... Hm, I'm starting to forget the vital details about my patch... > [...] >>> Index: net-next/drivers/net/phy/mdio_device.c >>> =================================================================== >>> --- net-next.orig/drivers/net/phy/mdio_device.c >>> +++ net-next/drivers/net/phy/mdio_device.c > [...] >>> @@ -117,9 +126,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); >>> >>> + /* Assert the reset signal */ >>> + mdio_device_reset(mdiodev, 1); >> >> I wonder if it's safe to do this in general. What if ->probe does >> something with the phy that is lost by resetting but that is relied on >> later? > > Well, I thought that config_init() method is designed for that but indeed > the LXT driver writes to BMCR in its probe() method and hence is broken. > Thank you for noticing... It's broken even without my patch. The phylib will cause a PHY soft reset when attaching to the PHY device, so all BMCR programming dpone in the probe() method will be lost. My patch does make sense as is. Looks like I should also look into fixing lxt.c. Florian, what do you think? [...] MBR, Sergei
Hello. On 05/13/2016 12:07 PM, Roger Quadros wrote: [...] >>> +} >>> +EXPORT_SYMBOL(mdio_device_reset); >>> + >>> /** >>> * mdio_probe - probe an MDIO device >>> * @dev: device to probe >>> @@ -117,9 +126,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); >>> >>> + /* Assert the reset signal */ >>> + mdio_device_reset(mdiodev, 1); >> >> I wonder if it's safe to do this in general. What if ->probe does >> something with the phy that is lost by resetting but that is relied on >> later? > > mdio_probe is called for non PHY devices only, right? Yes, those also can have a reset signal. > I'm a bit lost as to why we're de-asserting reset at multiple places. i.e. > mdio_probe(), phy_device_register(), phy_init_hw(), phy_probe(), of_mdiobus_register_phy(). > Isn't it simpler to just de-assert it once at the topmost level? I wasn't after simplicity here. The intent was to save some power putting the device at reset when it's not needed. Florian Fainelly (the phylib maintainer) actually wanted me to go even further with that and assert reset in phy_suspend() but it was too much for me. > i.e. of_mdiobus_register_device() f and of_mdiobus_register_phy()? > > Also, how about issuing a reset pulse instead of just de-asserting it. If it's already held at reset, my assumption is that it's enough time passed already. > This would tackle the case where PHY is in invalid state with reset already > de-asserted. Good question. I haven't yet met such cases though. > Another issue is that on some boards we have one reset line tied to > multiple PHYs.How do we prevent multiple resets being taking place when each of > the PHYs are registered? My patch just doesn't address this case -- it's about the individual resets only. > Do we just specify the reset line only for one PHY in > the DT or can we have the reset gpio in the mdio_bus node for such case? I think there's mii_bus::reset() method for that case. Some Ethernet drivers even use it (mostly instead of the code being suggested here). > cheers, > -roger MBR, Sergei
> >Another issue is that on some boards we have one reset line tied to > >multiple PHYs.How do we prevent multiple resets being taking place when each of > >the PHYs are registered? > > My patch just doesn't address this case -- it's about the > individual resets only. This actually needs to be addresses a layer above. What you have is a bus reset, not a device reset. So the gpio line is associated to the mdio bus, not a PHY. Either your MDIO driver needs to handle the gpio line, or in __mdio_register(), before it starts looking at the children. Andrew
On 05/13/2016 11:44 PM, Andrew Lunn wrote: >>> Another issue is that on some boards we have one reset line tied to >>> multiple PHYs.How do we prevent multiple resets being taking place when each of >>> the PHYs are registered? >> >> My patch just doesn't address this case -- it's about the >> individual resets only. > > This actually needs to be addresses a layer above. What you have is a > bus reset, not a device reset. No. There's simply no such thing as a bus reset for the xMII/MDIO busses, there's simply no reset signaling on them. Every device has its own reset signal and its own timing requirements. > So the gpio line is associated to the mdio bus, not a PHY. No. > Either your MDIO driver needs to handle the gpio > line, or in __mdio_register(), __mdiobus_register(), you mean? > before it starts looking at the > children. It's basically the same thing. The MDIO bus reset is a misconception. > > Andrew MBR, Sergei
Hello. On 05/13/2016 07:06 AM, Andrew Lunn wrote: >>>> + gpiod = fwnode_get_named_gpiod(&child->fwnode, "reset-gpios"); >>>> + /* Deassert the reset signal */ >>>> + if (!IS_ERR(gpiod)) >>>> + gpiod_direction_output(gpiod, 0); >>> >>> This is wrong I think. You must only ignore -ENODEV, all other error >> >> At least -ENOSYS should also be ignored (it's returned when >> gpiolib is not configured), right? When does -ENODEV gets returned >> (it's not easy to follow)? >> >>> codes should be passed to the caller. >> >> The caller doesn't care anyway... > > It should do. Tell that to Florian. So far, everybody has been happy with of_mdiobus_register(). Until I had to touch this code. :-) > What if fwnode_get_named_gpiod() returns -EPROBE_DEFER > because the GPIO driver has not been loaded yet? Bad luck. :-) Seriously, I'll see what I can do but it's not a trivial case. > Andrew MBR, Sergei
Hello. On 05/13/2016 10:06 AM, Uwe Kleine-König wrote: [...] >>>> Index: net-next/drivers/of/of_mdio.c >>>> =================================================================== >>>> --- net-next.orig/drivers/of/of_mdio.c >>>> +++ net-next/drivers/of/of_mdio.c >>>> @@ -44,6 +44,7 @@ static int of_get_phy_id(struct device_n >>>> static void of_mdiobus_register_phy(struct mii_bus *mdio, >>>> struct device_node *child, u32 addr) >>>> { >>>> + struct gpio_desc *gpiod; >>>> struct phy_device *phy; >>>> bool is_c45; >>>> int rc; >>>> @@ -52,10 +53,17 @@ static void of_mdiobus_register_phy(stru >>>> is_c45 = of_device_is_compatible(child, >>>> "ethernet-phy-ieee802.3-c45"); >>>> >>>> + gpiod = fwnode_get_named_gpiod(&child->fwnode, "reset-gpios"); >>>> + /* Deassert the reset signal */ >>>> + if (!IS_ERR(gpiod)) >>>> + gpiod_direction_output(gpiod, 0); >>> >>> This is wrong I think. You must only ignore -ENODEV, all other error >> >> At least -ENOSYS should also be ignored (it's returned when gpiolib is >> not configured), right? > > No, that's a common misconception. If GPIOLIB is off you cannot > determine if dt specified a reset gpio. So you have the choice between: > > a) ignore -ENOSYS and so don't handle the reset line in the cases where > it's necessary probably yielding an "Error: phy not found" message. > b) fail to probe even if a reset handling isn't necessary, yielding > "Error: failed to get hold of reset gpio". > > I say b) is the better one. It's easier to debug because the error > message is better, GPIOLIB is enabled in most cases anyhow (still maybe > select it?) and it's ensured that we're operating in the limits of the > hardware specs (maybe a phy returns a random value when a register is > read while reset is applied?). It returns all ones in my case. >> When does -ENODEV gets returned (it's not easy to follow)? > > I don't know for sure for fwnode_get_named_gpiod, but the gpiod_get*() > family returns -ENODEV if the node doesn't have a reset-gpio property. Are you sure it's not -ENOENT? >>> codes should be passed to the caller. >> >> The caller doesn't care anyway... >> >>> (I see that's not trivial because >>> of_mdiobus_register_phy returns void.) >> >> I've made this function *void* in net-next. > > I'd say this is a step in the wrong direction. For example this makes it > impossible to handle the case where the phy should be probed, the gpio > driver isn't available yet, though. Normally you'd want to return > -EPROBE_DEFER in this case and retry probing later. Well, of_mdiobus_register() is not an easy function to add the checks whiuch were never there (and undo the done stuff on failure). I'll see what I can do but no promises... >>> In my patch I used devm_gpiod_get_array which has the nice property that >>> I can already pass GPIOD_OUT_LOW in flags. Also this binds the lifetime >>> of the gpio to the device which is nice and IMHO the right direction for >>> the phylib (i.e. better embracing of the device model). >>> >>> This cannot be used here easily however because there is no struct >>> device yet and this is only created after the phy id is determined. >> >> Your last patch [1] didn't make use of the managed device API (devm) >> either, I didn't quite get to the bottom of that... > > Right, devm didn't work. My patch was a prototype for a way that allowed > to bind the lifetime of the gpio to the device. This might be longer > than the call to mdiobus_unregister. Ah, that was the reason... Well, then you hardly achieved anything with rehashing the code... > See the problems that i2c handles > because it doesn't handle lifetimes correctly in drivers/i2c/i2c-core.c > at the end of i2c_del_adapter. > >>> The phy id is either read from the device tree or must be read from >>> the phy which might fail if reset is not deasserted. >> >>> Principally there is no reason however that the phy_id must be known >>> before the struct device is created however. >> >> It's just that the code is cleaner that way... > > I don't agree, I don't see that > > determine_phyid() > allocate_device() > > is better than > > allocate_device() > determine_phyid() This is an oversimplified view. Actually, it is: error = determine_phyid() if (error) return allocate_device() versus allocate_device() error = determine_phyid() if (error) free_device() > . The former is maybe easier because that's the status quo and it > doesn't need patching. But IMHO the result is on a similar level of > cleanliness. I disagree. And I don't see why it's necessary at all. Just to use another gpiolib API? > Best regards > Uwe MBR, Sergei
On Fri, May 13, 2016 at 11:56:12PM +0300, Sergei Shtylyov wrote: > On 05/13/2016 11:44 PM, Andrew Lunn wrote: > > >>>Another issue is that on some boards we have one reset line tied to > >>>multiple PHYs.How do we prevent multiple resets being taking place when each of > >>>the PHYs are registered? > >> > >> My patch just doesn't address this case -- it's about the > >>individual resets only. > > > >This actually needs to be addresses a layer above. What you have is a > >bus reset, not a device reset. > > No. > There's simply no such thing as a bus reset for the xMII/MDIO > busses, there's simply no reset signaling on them. Every device has > its own reset signal and its own timing requirements. Except in the case above, where two phys are sharing the same reset signal. So although it is not part of the mdio standard to have a bus reset, this is in effect what the gpio line is doing, resetting all devices on the bus. If you don't model that as a bus reset, how do you model it? Andrew
Hello. On 05/14/2016 02:44 AM, Andrew Lunn wrote: >>>>> Another issue is that on some boards we have one reset line tied to >>>>> multiple PHYs.How do we prevent multiple resets being taking place when each of >>>>> the PHYs are registered? >>>> >>>> My patch just doesn't address this case -- it's about the >>>> individual resets only. >>> >>> This actually needs to be addresses a layer above. What you have is a >>> bus reset, not a device reset. >> >> No. >> There's simply no such thing as a bus reset for the xMII/MDIO >> busses, there's simply no reset signaling on them. Every device has >> its own reset signal and its own timing requirements. > > Except in the case above, where two phys are sharing the same reset > signal. So although it is not part of the mdio standard to have a bus > reset, this is in effect what the gpio line is doing, resetting all > devices on the bus. If you don't model that as a bus reset, how do you > model it? I'm not suggesting that the shared reset should be handled by my patch. Contrariwise, I suggested to use the mii_bus::reset() method -- I see it as a necessary evil. However, in the more common case of a single PHY, this method simply doesn't scale -- you'd have to teach each and every individual MAC/ MDIO driver to do the GPIO reset trick. > Andrew MBR, Sergei
On Sat, May 14, 2016 at 10:36:38PM +0300, Sergei Shtylyov wrote: > Hello. > > On 05/14/2016 02:44 AM, Andrew Lunn wrote: > > >>>>>Another issue is that on some boards we have one reset line tied to > >>>>>multiple PHYs.How do we prevent multiple resets being taking place when each of > >>>>>the PHYs are registered? > >>>> > >>>> My patch just doesn't address this case -- it's about the > >>>>individual resets only. > >>> > >>>This actually needs to be addresses a layer above. What you have is a > >>>bus reset, not a device reset. > >> > >> No. > >> There's simply no such thing as a bus reset for the xMII/MDIO > >>busses, there's simply no reset signaling on them. Every device has > >>its own reset signal and its own timing requirements. > > > >Except in the case above, where two phys are sharing the same reset > >signal. So although it is not part of the mdio standard to have a bus > >reset, this is in effect what the gpio line is doing, resetting all > >devices on the bus. If you don't model that as a bus reset, how do you > >model it? > > I'm not suggesting that the shared reset should be handled by my > patch. Contrariwise, I suggested to use the mii_bus::reset() method I think we miss understood each other somewhere. Your code is great for one gpio reset line for one phy. I think there could be similar code one layer above to handle one gpio line for multiple phys. Andrew
Hello. On 05/13/2016 10:18 PM, Sergei Shtylyov wrote: >>> [we already talked about this patch in #armlinux, I'm now just >>> forwarding my comments on the list. Background was that I sent an easier >>> and less complete patch with the same idea. See >>> http://patchwork.ozlabs.org/patch/621418/] >>> >>> [added Linus Walleij to Cc, there is a question for you/him below] >>> >>> On Fri, Apr 29, 2016 at 01:12:54AM +0300, Sergei Shtylyov wrote: >>>> --- net-next.orig/Documentation/devicetree/bindings/net/phy.txt >>>> +++ net-next/Documentation/devicetree/bindings/net/phy.txt >>>> @@ -35,6 +35,8 @@ Optional Properties: >>>> - broken-turn-around: If set, indicates the PHY device does not correctly >>>> release the turn around line low at the end of a MDIO transaction. >>>> >>>> +- reset-gpios: The GPIO phandle and specifier for the PHY reset signal. >>>> + >>>> Example: >>>> >>>> ethernet-phy@0 { >>> >>> This is great. >>> >>>> Index: net-next/drivers/net/phy/at803x.c >>>> =================================================================== >>>> --- net-next.orig/drivers/net/phy/at803x.c >>>> +++ net-next/drivers/net/phy/at803x.c >>>> @@ -65,7 +65,6 @@ MODULE_LICENSE("GPL"); >>>> [...] >>> >>> My patch breaks this driver. I wasn't aware of it. >> >> I tried to be as careful as I could but still it looks that I didn't >> succeed at that too... > > Hm, I'm starting to forget the vital details about my patch... > >> [...] >>>> Index: net-next/drivers/net/phy/mdio_device.c >>>> =================================================================== >>>> --- net-next.orig/drivers/net/phy/mdio_device.c >>>> +++ net-next/drivers/net/phy/mdio_device.c >> [...] >>>> @@ -117,9 +126,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); >>>> >>>> + /* Assert the reset signal */ >>>> + mdio_device_reset(mdiodev, 1); >>> >>> I wonder if it's safe to do this in general. What if ->probe does >>> something with the phy that is lost by resetting but that is relied on >>> later? >> >> Well, I thought that config_init() method is designed for that but indeed >> the LXT driver writes to BMCR in its probe() method and hence is broken. >> Thank you for noticing... > > It's broken even without my patch. The phylib will cause a PHY soft reset Only iff the config_init() method exists in the PHY driver... > when attaching to the PHY device, so all BMCR programming dpone in the probe() > method will be lost. My patch does make sense as is. No, actually it doesn't. :-( > Looks like I should alsolook into fixing lxt.c. It took me to actually do a patch to understand my fault. Sigh... :-/ > Florian, what do you think? Florian, is phy_init_hw() logic correct? MBR, Sergei
Hello. On 05/14/2016 10:50 PM, Andrew Lunn wrote: >>>>>>> Another issue is that on some boards we have one reset line tied to >>>>>>> multiple PHYs.How do we prevent multiple resets being taking place when each of >>>>>>> the PHYs are registered? >>>>>> >>>>>> My patch just doesn't address this case -- it's about the >>>>>> individual resets only. >>>>> >>>>> This actually needs to be addresses a layer above. What you have is a >>>>> bus reset, not a device reset. >>>> >>>> No. >>>> There's simply no such thing as a bus reset for the xMII/MDIO >>>> busses, there's simply no reset signaling on them. Every device has >>>> its own reset signal and its own timing requirements. >>> >>> Except in the case above, where two phys are sharing the same reset >>> signal. So although it is not part of the mdio standard to have a bus >>> reset, this is in effect what the gpio line is doing, resetting all >>> devices on the bus. If you don't model that as a bus reset, how do you >>> model it? >> >> I'm not suggesting that the shared reset should be handled by my >> patch. Contrariwise, I suggested to use the mii_bus::reset() method > > I think we miss understood each other somewhere. > > Your code is great for one gpio reset line for one phy. > > I think there could be similar code one layer above to handle one gpio > line for multiple phys. Ah, you want me to recognize some MAC/MDIO bound prop (e.g. "mdio-reset-gpios") in of_mdiobus_register()? I'll think about it now that my patch needs fixing anyway... > Andrew MBR, Sergei
> >I think there could be similar code one layer above to handle one gpio > >line for multiple phys. > > Ah, you want me to recognize some MAC/MDIO bound prop (e.g. > "mdio-reset-gpios") in of_mdiobus_register()? I'll think about it > now that my patch needs fixing anyway... Hi Sergi It does not need to be you implementing this, your hardware does not need it. However, if you do feel like doing it, great. Andrew
On 13/05/16 22:36, Sergei Shtylyov wrote: > Hello. > > On 05/13/2016 12:07 PM, Roger Quadros wrote: > > [...] > >>>> +} >>>> +EXPORT_SYMBOL(mdio_device_reset); >>>> + >>>> /** >>>> * mdio_probe - probe an MDIO device >>>> * @dev: device to probe >>>> @@ -117,9 +126,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); >>>> >>>> + /* Assert the reset signal */ >>>> + mdio_device_reset(mdiodev, 1); >>> >>> I wonder if it's safe to do this in general. What if ->probe does >>> something with the phy that is lost by resetting but that is relied on >>> later? >> >> mdio_probe is called for non PHY devices only, right? > > Yes, those also can have a reset signal. > >> I'm a bit lost as to why we're de-asserting reset at multiple places. i.e. >> mdio_probe(), phy_device_register(), phy_init_hw(), phy_probe(), of_mdiobus_register_phy(). > >> Isn't it simpler to just de-assert it once at the topmost level? > > I wasn't after simplicity here. The intent was to save some power putting the device at reset when it's not needed. Florian Fainelly (the phylib maintainer) actually wanted me to go even further with that and assert reset in phy_suspend() but it was too much for me. Is using RESET for power saving a standard practice? Isn't there some register interface for power saving? My concern here is that RESET does a number of things that might be undesired for normal operation. e.g. PHY's will use bootstrap settings on RESET and we need to ensure that bootstrap pins are always in the right setting before issuing a RESET. What happens to the PHY link? Is it lost while PHY RESET is asserted? Is this really desirable? > >> i.e. of_mdiobus_register_device() f and of_mdiobus_register_phy()? >> >> Also, how about issuing a reset pulse instead of just de-asserting it. > > If it's already held at reset, my assumption is that it's enough time passed already. > >> This would tackle the case where PHY is in invalid state with reset already >> de-asserted. > > Good question. I haven't yet met such cases though. > >> Another issue is that on some boards we have one reset line tied to >> multiple PHYs.How do we prevent multiple resets being taking place when each of >> the PHYs are registered? > > My patch just doesn't address this case -- it's about the individual resets only. > >> Do we just specify the reset line only for one PHY in >> the DT or can we have the reset gpio in the mdio_bus node for such case? > > I think there's mii_bus::reset() method for that case. Some Ethernet drivers even use it > (mostly instead of the code being suggested here). > -- cheers, -roger
Hello. On 5/15/2016 6:23 PM, Andrew Lunn wrote: >>> I think there could be similar code one layer above to handle one gpio >>> line for multiple phys. >> >> Ah, you want me to recognize some MAC/MDIO bound prop (e.g. >> "mdio-reset-gpios") in of_mdiobus_register()? I'll think about it >> now that my patch needs fixing anyway... > > Hi Sergi > > It does not need to be you implementing this, your hardware does not > need it. However, if you do feel like doing it, great. It would cover my "single PHY" case anyway. > Andrew MBR, Sergei
On Thu, May 12, 2016 at 8:42 PM, Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > [added Linus Walleij to Cc, there is a question for you/him below] (...) >> +void mdio_device_reset(struct mdio_device *mdiodev, int value) >> +{ >> + if (mdiodev->reset) >> + gpiod_set_value(mdiodev->reset, value); > > Before v4.6-rc1~108^2~91 it was not necessary to check for the first > parameter being non-NULL before calling gpiod_set_value. Linus, did you > change this on purpose? Not really. And AFAICT it is still not necessary: what changed is that an error message will be printed by VALIDATE_DESC() if you do that. And that is proper I guess? I think it's sloppy code to randomly pass in NULL to a call and just expect it to bail out, it seems more like exercising the error path than something you'd normally rely on. Or am I getting things wrong? Yours, Linus Walleij
On Thu, May 26, 2016 at 11:00:55AM +0200, Linus Walleij wrote: > On Thu, May 12, 2016 at 8:42 PM, Uwe Kleine-König > <u.kleine-koenig@pengutronix.de> wrote: > > > [added Linus Walleij to Cc, there is a question for you/him below] > (...) > >> +void mdio_device_reset(struct mdio_device *mdiodev, int value) > >> +{ > >> + if (mdiodev->reset) > >> + gpiod_set_value(mdiodev->reset, value); > > > > Before v4.6-rc1~108^2~91 it was not necessary to check for the first > > parameter being non-NULL before calling gpiod_set_value. Linus, did you > > change this on purpose? > > Not really. And AFAICT it is still not necessary: what changed is that > an error message will be printed by VALIDATE_DESC() if you do that. > And that is proper I guess? I think it's sloppy code to randomly pass in > NULL to a call and just expect it to bail out, it seems more like > exercising the error path than something you'd normally rely on. > > Or am I getting things wrong? is the following sloppy?: somegpio = gpiod_get_optional(dev, "some", GPIOD_OUT_LOW); if (IS_ERR(somegpio)) return PTR_ERR(somegpio); gpiod_set_value(somegpio, 1); If not (as I assume) you really changed something as this might trigger the warning. Best regards Uwe
On Thu, May 26, 2016 at 9:00 PM, Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > On Thu, May 26, 2016 at 11:00:55AM +0200, Linus Walleij wrote: >> On Thu, May 12, 2016 at 8:42 PM, Uwe Kleine-König >> <u.kleine-koenig@pengutronix.de> wrote: >> >> > [added Linus Walleij to Cc, there is a question for you/him below] >> (...) >> >> +void mdio_device_reset(struct mdio_device *mdiodev, int value) >> >> +{ >> >> + if (mdiodev->reset) >> >> + gpiod_set_value(mdiodev->reset, value); >> > >> > Before v4.6-rc1~108^2~91 it was not necessary to check for the first >> > parameter being non-NULL before calling gpiod_set_value. Linus, did you >> > change this on purpose? >> >> Not really. And AFAICT it is still not necessary: what changed is that >> an error message will be printed by VALIDATE_DESC() if you do that. >> And that is proper I guess? I think it's sloppy code to randomly pass in >> NULL to a call and just expect it to bail out, it seems more like >> exercising the error path than something you'd normally rely on. >> >> Or am I getting things wrong? > > is the following sloppy?: > > somegpio = gpiod_get_optional(dev, "some", GPIOD_OUT_LOW); > if (IS_ERR(somegpio)) > return PTR_ERR(somegpio); > gpiod_set_value(somegpio, 1); Grrr OK I see, it's explicit from the _optional() call that it may be NULL and then it should be ignored. So subsequent functions should ignore that and bail out. My bad, sorry. > If not (as I assume) you really changed something as this might trigger > the warning. Making a patch. Yours, Linus Walleij
Index: net-next/Documentation/devicetree/bindings/net/phy.txt =================================================================== --- net-next.orig/Documentation/devicetree/bindings/net/phy.txt +++ net-next/Documentation/devicetree/bindings/net/phy.txt @@ -35,6 +35,8 @@ Optional Properties: - broken-turn-around: If set, indicates the PHY device does not correctly release the turn around line low at the end of a MDIO transaction. +- reset-gpios: The GPIO phandle and specifier for the PHY reset signal. + Example: ethernet-phy@0 { Index: net-next/drivers/net/phy/at803x.c =================================================================== --- net-next.orig/drivers/net/phy/at803x.c +++ net-next/drivers/net/phy/at803x.c @@ -65,7 +65,6 @@ MODULE_LICENSE("GPL"); struct at803x_priv { bool phy_reset:1; - struct gpio_desc *gpiod_reset; }; struct at803x_context { @@ -271,22 +270,10 @@ static int at803x_probe(struct phy_devic { 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; @@ -361,14 +348,14 @@ static void at803x_link_change_notify(st */ if (phydev->drv->phy_id == ATH8030_PHY_ID) { 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); Index: net-next/drivers/net/phy/mdio_bus.c =================================================================== --- net-next.orig/drivers/net/phy/mdio_bus.c +++ net-next/drivers/net/phy/mdio_bus.c @@ -35,6 +35,7 @@ #include <linux/phy.h> #include <linux/io.h> #include <linux/uaccess.h> +#include <linux/gpio/consumer.h> #include <asm/irq.h> @@ -371,6 +372,9 @@ void mdiobus_unregister(struct mii_bus * if (!mdiodev) continue; + if (mdiodev->reset) + gpiod_put(mdiodev->reset); + mdiodev->device_remove(mdiodev); mdiodev->device_free(mdiodev); } Index: net-next/drivers/net/phy/mdio_device.c =================================================================== --- net-next.orig/drivers/net/phy/mdio_device.c +++ net-next/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> @@ -103,6 +105,13 @@ void mdio_device_remove(struct mdio_devi } 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 @@ -117,9 +126,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); + /* Assert the reset signal */ + mdio_device_reset(mdiodev, 1); + } + return err; } @@ -129,9 +145,16 @@ static int mdio_remove(struct device *de struct device_driver *drv = mdiodev->dev.driver; struct mdio_driver *mdiodrv = to_mdio_driver(drv); - if (mdiodrv->remove) + if (mdiodrv->remove) { + /* Deassert the reset signal */ + mdio_device_reset(mdiodev, 0); + mdiodrv->remove(mdiodev); + /* Assert the reset signal */ + mdio_device_reset(mdiodev, 1); + } + return 0; } Index: net-next/drivers/net/phy/phy_device.c =================================================================== --- net-next.orig/drivers/net/phy/phy_device.c +++ net-next/drivers/net/phy/phy_device.c @@ -589,6 +589,9 @@ int phy_device_register(struct phy_devic 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) { @@ -604,9 +607,15 @@ int phy_device_register(struct phy_devic goto out; } + /* Assert the reset signal */ + phy_device_reset(phydev, 1); + return 0; out: + /* Assert the reset signal */ + phy_device_reset(phydev, 1); + mdiobus_unregister_device(&phydev->mdio); return err; } @@ -792,6 +801,9 @@ int phy_init_hw(struct phy_device *phyde { int ret = 0; + /* Deassert the reset signal */ + phy_device_reset(phydev, 0); + if (!phydev->drv || !phydev->drv->config_init) return 0; @@ -997,6 +1009,9 @@ void phy_detach(struct phy_device *phyde put_device(&phydev->mdio.dev); module_put(bus->owner); + + /* Assert the reset signal */ + phy_device_reset(phydev, 1); } EXPORT_SYMBOL(phy_detach); @@ -1596,9 +1611,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); + /* Assert the reset signal */ + phy_device_reset(phydev, 1); + } + mutex_unlock(&phydev->lock); return err; @@ -1612,8 +1634,15 @@ static int phy_remove(struct device *dev phydev->state = PHY_DOWN; mutex_unlock(&phydev->lock); - if (phydev->drv->remove) + if (phydev->drv->remove) { + /* Deassert the reset signal */ + phy_device_reset(phydev, 0); + phydev->drv->remove(phydev); + + /* Assert the reset signal */ + phy_device_reset(phydev, 1); + } phydev->drv = NULL; return 0; Index: net-next/drivers/of/of_mdio.c =================================================================== --- net-next.orig/drivers/of/of_mdio.c +++ net-next/drivers/of/of_mdio.c @@ -44,6 +44,7 @@ static int of_get_phy_id(struct device_n static void of_mdiobus_register_phy(struct mii_bus *mdio, struct device_node *child, u32 addr) { + struct gpio_desc *gpiod; struct phy_device *phy; bool is_c45; int rc; @@ -52,10 +53,17 @@ static void of_mdiobus_register_phy(stru is_c45 = of_device_is_compatible(child, "ethernet-phy-ieee802.3-c45"); + gpiod = fwnode_get_named_gpiod(&child->fwnode, "reset-gpios"); + /* Deassert the reset signal */ + if (!IS_ERR(gpiod)) + gpiod_direction_output(gpiod, 0); if (!is_c45 && !of_get_phy_id(child, &phy_id)) phy = phy_device_create(mdio, addr, phy_id, 0, NULL); else phy = get_phy_device(mdio, addr, is_c45); + /* Assert the reset signal again */ + if (!IS_ERR(gpiod)) + gpiod_set_value(gpiod, 1); if (IS_ERR(phy)) return; @@ -75,6 +83,9 @@ static void of_mdiobus_register_phy(stru of_node_get(child); phy->mdio.dev.of_node = child; + if (!IS_ERR(gpiod)) + phy->mdio.reset = gpiod; + /* All data is now stored in the phy struct; * register it */ rc = phy_device_register(phy); @@ -92,6 +103,7 @@ static void of_mdiobus_register_device(s struct device_node *child, u32 addr) { struct mdio_device *mdiodev; + struct gpio_desc *gpiod; int rc; mdiodev = mdio_device_create(mdio, addr); @@ -104,6 +116,10 @@ static void of_mdiobus_register_device(s of_node_get(child); mdiodev->dev.of_node = child; + gpiod = fwnode_get_named_gpiod(&child->fwnode, "reset-gpios"); + if (!IS_ERR(gpiod)) + mdiodev->reset = gpiod; + /* All data is now stored in the mdiodev struct; register it. */ rc = mdio_device_register(mdiodev); if (rc) { Index: net-next/include/linux/mdio.h =================================================================== --- net-next.orig/include/linux/mdio.h +++ net-next/include/linux/mdio.h @@ -11,6 +11,7 @@ #include <uapi/linux/mdio.h> +struct gpio_desc; struct mii_bus; /* Multiple levels of nesting are possible. However typically this is @@ -37,6 +38,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) @@ -69,6 +71,7 @@ void mdio_device_free(struct mdio_device 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); Index: net-next/include/linux/phy.h =================================================================== --- net-next.orig/include/linux/phy.h +++ net-next/include/linux/phy.h @@ -769,6 +769,11 @@ static inline int phy_read_status(struct return phydev->drv->read_status(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)
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 as it made use of the reset GPIO for its own purposes... Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> --- Changes in version 2: - reformatted the changelog; - resolved rejects, refreshed the patch. Documentation/devicetree/bindings/net/phy.txt | 2 + Documentation/devicetree/bindings/net/phy.txt | 2 + drivers/net/phy/at803x.c | 19 ++------------ drivers/net/phy/mdio_bus.c | 4 +++ drivers/net/phy/mdio_device.c | 27 +++++++++++++++++++-- drivers/net/phy/phy_device.c | 33 ++++++++++++++++++++++++-- drivers/of/of_mdio.c | 16 ++++++++++++ include/linux/mdio.h | 3 ++ include/linux/phy.h | 5 +++ 8 files changed, 89 insertions(+), 20 deletions(-)