| Message ID | 20200622093744.13685-15-brgl@bgdev.pl (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | net: phy: correctly model the PHY voltage supply in DT | expand |
On Mon, Jun 22, 2020 at 11:37:43AM +0200, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > The MDIO sub-system now supports PHY regulators. Let's reuse the code > to extend this support over to the PHY device. > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> > --- > drivers/net/phy/phy_device.c | 49 ++++++++++++++++++++++++++++-------- > include/linux/phy.h | 10 ++++++++ > 2 files changed, 48 insertions(+), 11 deletions(-) > > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c > index 58923826838b..d755adb748a5 100644 > --- a/drivers/net/phy/phy_device.c > +++ b/drivers/net/phy/phy_device.c > @@ -827,7 +827,12 @@ int phy_device_register(struct phy_device *phydev) > > err = mdiobus_register_device(&phydev->mdio); > if (err) > - return err; > + goto err_out; > + > + /* Enable the PHY regulator */ > + err = phy_device_power_on(phydev); > + if (err) > + goto err_unregister_mdio; > > /* Deassert the reset signal */ > phy_device_reset(phydev, 0); > @@ -846,22 +851,25 @@ int phy_device_register(struct phy_device *phydev) > err = phy_scan_fixups(phydev); > if (err) { > phydev_err(phydev, "failed to initialize\n"); > - goto out; > + goto err_reset; > } > > err = device_add(&phydev->mdio.dev); > if (err) { > phydev_err(phydev, "failed to add\n"); > - goto out; > + goto err_reset; > } > > return 0; > > - out: > +err_reset: > /* Assert the reset signal */ > phy_device_reset(phydev, 1); > - > + /* Disable the PHY regulator */ > + phy_device_power_off(phydev); > +err_unregister_mdio: > mdiobus_unregister_device(&phydev->mdio); > +err_out: > return err; > } > EXPORT_SYMBOL(phy_device_register); > @@ -883,6 +891,8 @@ void phy_device_remove(struct phy_device *phydev) > > /* Assert the reset signal */ > phy_device_reset(phydev, 1); > + /* Disable the PHY regulator */ > + phy_device_power_off(phydev); > > mdiobus_unregister_device(&phydev->mdio); > } > @@ -1064,6 +1074,11 @@ int phy_init_hw(struct phy_device *phydev) > { > int ret = 0; > > + /* Enable the PHY regulator */ > + ret = phy_device_power_on(phydev); > + if (ret) > + return ret; > + > /* Deassert the reset signal */ > phy_device_reset(phydev, 0); > > @@ -1644,6 +1659,8 @@ void phy_detach(struct phy_device *phydev) > > /* Assert the reset signal */ > phy_device_reset(phydev, 1); > + /* Disable the PHY regulator */ > + phy_device_power_off(phydev); This is likely to cause issues for some PHY drivers. Note that we have some PHY drivers which register a temperature sensor in the probe function, which means they can be accessed independently of the lifetime of the PHY bound to the network driver (which may only be while the network device is "up".) We certainly do not want hwmon failing just because the network device is down. That's kind of worked around for the reset stuff, because there are two layers to that: the mdio device layer reset support which knows nothing of the PHY binding state to the network driver, and the phylib reset support, but it is not nice.
pon., 22 cze 2020 o 15:29 Russell King - ARM Linux admin <linux@armlinux.org.uk> napisał(a): > [snip!] > > This is likely to cause issues for some PHY drivers. Note that we have > some PHY drivers which register a temperature sensor in the probe > function, which means they can be accessed independently of the lifetime > of the PHY bound to the network driver (which may only be while the > network device is "up".) We certainly do not want hwmon failing just > because the network device is down. > > That's kind of worked around for the reset stuff, because there are two > layers to that: the mdio device layer reset support which knows nothing > of the PHY binding state to the network driver, and the phylib reset > support, but it is not nice. > Regulators are reference counted so if the hwmon driver enables it using mdio_device_power_on() it will stay on even after the PHY driver calls phy_device_power_off(), right? Am I missing something? Bart
On Tue, Jun 23, 2020 at 11:41:11AM +0200, Bartosz Golaszewski wrote: > pon., 22 cze 2020 o 15:29 Russell King - ARM Linux admin > <linux@armlinux.org.uk> napisał(a): > > > > [snip!] > > > > > This is likely to cause issues for some PHY drivers. Note that we have > > some PHY drivers which register a temperature sensor in the probe > > function, which means they can be accessed independently of the lifetime > > of the PHY bound to the network driver (which may only be while the > > network device is "up".) We certainly do not want hwmon failing just > > because the network device is down. > > > > That's kind of worked around for the reset stuff, because there are two > > layers to that: the mdio device layer reset support which knows nothing > > of the PHY binding state to the network driver, and the phylib reset > > support, but it is not nice. > > > > Regulators are reference counted so if the hwmon driver enables it > using mdio_device_power_on() it will stay on even after the PHY driver > calls phy_device_power_off(), right? Am I missing something? If that is true, you will need to audit the PHY drivers to add that.
wt., 23 cze 2020 o 11:43 Russell King - ARM Linux admin <linux@armlinux.org.uk> napisał(a): > > On Tue, Jun 23, 2020 at 11:41:11AM +0200, Bartosz Golaszewski wrote: > > pon., 22 cze 2020 o 15:29 Russell King - ARM Linux admin > > <linux@armlinux.org.uk> napisał(a): > > > > > > > [snip!] > > > > > > > > This is likely to cause issues for some PHY drivers. Note that we have > > > some PHY drivers which register a temperature sensor in the probe > > > function, which means they can be accessed independently of the lifetime > > > of the PHY bound to the network driver (which may only be while the > > > network device is "up".) We certainly do not want hwmon failing just > > > because the network device is down. > > > > > > That's kind of worked around for the reset stuff, because there are two > > > layers to that: the mdio device layer reset support which knows nothing > > > of the PHY binding state to the network driver, and the phylib reset > > > support, but it is not nice. > > > > > > > Regulators are reference counted so if the hwmon driver enables it > > using mdio_device_power_on() it will stay on even after the PHY driver > > calls phy_device_power_off(), right? Am I missing something? > > If that is true, you will need to audit the PHY drivers to add that. > This change doesn't have any effect on devices which don't have a regulator assigned in DT though. The one I'm adding in the last patch is the first to use this. Bart
On Tue, Jun 23, 2020 at 11:46:15AM +0200, Bartosz Golaszewski wrote: > wt., 23 cze 2020 o 11:43 Russell King - ARM Linux admin > <linux@armlinux.org.uk> napisał(a): > > > > On Tue, Jun 23, 2020 at 11:41:11AM +0200, Bartosz Golaszewski wrote: > > > pon., 22 cze 2020 o 15:29 Russell King - ARM Linux admin > > > <linux@armlinux.org.uk> napisał(a): > > > > > > > > > > [snip!] > > > > > > > > > > > This is likely to cause issues for some PHY drivers. Note that we have > > > > some PHY drivers which register a temperature sensor in the probe > > > > function, which means they can be accessed independently of the lifetime > > > > of the PHY bound to the network driver (which may only be while the > > > > network device is "up".) We certainly do not want hwmon failing just > > > > because the network device is down. > > > > > > > > That's kind of worked around for the reset stuff, because there are two > > > > layers to that: the mdio device layer reset support which knows nothing > > > > of the PHY binding state to the network driver, and the phylib reset > > > > support, but it is not nice. > > > > > > > > > > Regulators are reference counted so if the hwmon driver enables it > > > using mdio_device_power_on() it will stay on even after the PHY driver > > > calls phy_device_power_off(), right? Am I missing something? > > > > If that is true, you will need to audit the PHY drivers to add that. > > > > This change doesn't have any effect on devices which don't have a > regulator assigned in DT though. The one I'm adding in the last patch > is the first to use this. It's quality of implementation. Should we wait for someone else to make use of the new regulator support that has been added with a PHY that uses hwmon, and they don't realise that it breaks hwmon on it, and several kernel versions go by without it being noticed. It will only be a noticable issue when the associated network device is down, and that network device driver detaches from the PHY, so _is_ likely not to be noticed. Or should we do a small amount of work now to properly implement regulator support, which includes a trivial grep for "hwmon" amongst the PHY drivers, and add the necessary call to avoid the regulator being shut off.
wt., 23 cze 2020 o 11:56 Russell King - ARM Linux admin <linux@armlinux.org.uk> napisał(a): > > On Tue, Jun 23, 2020 at 11:46:15AM +0200, Bartosz Golaszewski wrote: > > wt., 23 cze 2020 o 11:43 Russell King - ARM Linux admin > > <linux@armlinux.org.uk> napisał(a): > > > > > > On Tue, Jun 23, 2020 at 11:41:11AM +0200, Bartosz Golaszewski wrote: > > > > pon., 22 cze 2020 o 15:29 Russell King - ARM Linux admin > > > > <linux@armlinux.org.uk> napisał(a): > > > > > > > > > > > > > [snip!] > > > > > > > > > > > > > > This is likely to cause issues for some PHY drivers. Note that we have > > > > > some PHY drivers which register a temperature sensor in the probe > > > > > function, which means they can be accessed independently of the lifetime > > > > > of the PHY bound to the network driver (which may only be while the > > > > > network device is "up".) We certainly do not want hwmon failing just > > > > > because the network device is down. > > > > > > > > > > That's kind of worked around for the reset stuff, because there are two > > > > > layers to that: the mdio device layer reset support which knows nothing > > > > > of the PHY binding state to the network driver, and the phylib reset > > > > > support, but it is not nice. > > > > > > > > > > > > > Regulators are reference counted so if the hwmon driver enables it > > > > using mdio_device_power_on() it will stay on even after the PHY driver > > > > calls phy_device_power_off(), right? Am I missing something? > > > > > > If that is true, you will need to audit the PHY drivers to add that. > > > > > > > This change doesn't have any effect on devices which don't have a > > regulator assigned in DT though. The one I'm adding in the last patch > > is the first to use this. > > It's quality of implementation. > > Should we wait for someone else to make use of the new regulator > support that has been added with a PHY that uses hwmon, and they > don't realise that it breaks hwmon on it, and several kernel versions > go by without it being noticed. It will only be a noticable issue > when the associated network device is down, and that network device > driver detaches from the PHY, so _is_ likely not to be noticed. > > Or should we do a small amount of work now to properly implement > regulator support, which includes a trivial grep for "hwmon" amongst > the PHY drivers, and add the necessary call to avoid the regulator > being shut off. > I'm not sure what the correct approach is here. Provide some helper that, when called, would increase the regulator's reference count even more to keep it enabled from the moment hwmon is registered to when the driver is detached? Bart
On Tue, Jun 23, 2020 at 06:27:06PM +0200, Bartosz Golaszewski wrote: > wt., 23 cze 2020 o 11:56 Russell King - ARM Linux admin > <linux@armlinux.org.uk> napisał(a): > > > > On Tue, Jun 23, 2020 at 11:46:15AM +0200, Bartosz Golaszewski wrote: > > > wt., 23 cze 2020 o 11:43 Russell King - ARM Linux admin > > > <linux@armlinux.org.uk> napisał(a): > > > > > > > > On Tue, Jun 23, 2020 at 11:41:11AM +0200, Bartosz Golaszewski wrote: > > > > > pon., 22 cze 2020 o 15:29 Russell King - ARM Linux admin > > > > > <linux@armlinux.org.uk> napisał(a): > > > > > > > > > > > > > > > > [snip!] > > > > > > > > > > > > > > > > > This is likely to cause issues for some PHY drivers. Note that we have > > > > > > some PHY drivers which register a temperature sensor in the probe > > > > > > function, which means they can be accessed independently of the lifetime > > > > > > of the PHY bound to the network driver (which may only be while the > > > > > > network device is "up".) We certainly do not want hwmon failing just > > > > > > because the network device is down. > > > > > > > > > > > > That's kind of worked around for the reset stuff, because there are two > > > > > > layers to that: the mdio device layer reset support which knows nothing > > > > > > of the PHY binding state to the network driver, and the phylib reset > > > > > > support, but it is not nice. > > > > > > > > > > > > > > > > Regulators are reference counted so if the hwmon driver enables it > > > > > using mdio_device_power_on() it will stay on even after the PHY driver > > > > > calls phy_device_power_off(), right? Am I missing something? > > > > > > > > If that is true, you will need to audit the PHY drivers to add that. > > > > > > > > > > This change doesn't have any effect on devices which don't have a > > > regulator assigned in DT though. The one I'm adding in the last patch > > > is the first to use this. > > > > It's quality of implementation. > > > > Should we wait for someone else to make use of the new regulator > > support that has been added with a PHY that uses hwmon, and they > > don't realise that it breaks hwmon on it, and several kernel versions > > go by without it being noticed. It will only be a noticable issue > > when the associated network device is down, and that network device > > driver detaches from the PHY, so _is_ likely not to be noticed. > > > > Or should we do a small amount of work now to properly implement > > regulator support, which includes a trivial grep for "hwmon" amongst > > the PHY drivers, and add the necessary call to avoid the regulator > > being shut off. > > > > I'm not sure what the correct approach is here. Provide some helper > that, when called, would increase the regulator's reference count even > more to keep it enabled from the moment hwmon is registered to when > the driver is detached? I think a PHY driver needs the utility to control this. We need to be careful here with naming, because phylib is not the only code in the kernel that uses the phy_ prefix. If we had runtime PM support for PHYs, with regulator support hooked into runtime PM, then we already have standard interfaces that drivers can use to control whether the device gets powered down.
On Wed, Jun 24, 2020 at 05:57:19PM +0100, Russell King - ARM Linux admin wrote: > On Tue, Jun 23, 2020 at 06:27:06PM +0200, Bartosz Golaszewski wrote: > > wt., 23 cze 2020 o 11:56 Russell King - ARM Linux admin > > <linux@armlinux.org.uk> napisał(a): > > > > > > On Tue, Jun 23, 2020 at 11:46:15AM +0200, Bartosz Golaszewski wrote: > > > > wt., 23 cze 2020 o 11:43 Russell King - ARM Linux admin > > > > <linux@armlinux.org.uk> napisał(a): > > > > > > > > > > On Tue, Jun 23, 2020 at 11:41:11AM +0200, Bartosz Golaszewski wrote: > > > > > > pon., 22 cze 2020 o 15:29 Russell King - ARM Linux admin > > > > > > <linux@armlinux.org.uk> napisał(a): > > > > > > > > > > > > > > > > > > > [snip!] > > > > > > > > > > > > > > > > > > > > This is likely to cause issues for some PHY drivers. Note that we have > > > > > > > some PHY drivers which register a temperature sensor in the probe > > > > > > > function, which means they can be accessed independently of the lifetime > > > > > > > of the PHY bound to the network driver (which may only be while the > > > > > > > network device is "up".) We certainly do not want hwmon failing just > > > > > > > because the network device is down. > > > > > > > > > > > > > > That's kind of worked around for the reset stuff, because there are two > > > > > > > layers to that: the mdio device layer reset support which knows nothing > > > > > > > of the PHY binding state to the network driver, and the phylib reset > > > > > > > support, but it is not nice. > > > > > > > > > > > > > > > > > > > Regulators are reference counted so if the hwmon driver enables it > > > > > > using mdio_device_power_on() it will stay on even after the PHY driver > > > > > > calls phy_device_power_off(), right? Am I missing something? > > > > > > > > > > If that is true, you will need to audit the PHY drivers to add that. > > > > > > > > > > > > > This change doesn't have any effect on devices which don't have a > > > > regulator assigned in DT though. The one I'm adding in the last patch > > > > is the first to use this. > > > > > > It's quality of implementation. > > > > > > Should we wait for someone else to make use of the new regulator > > > support that has been added with a PHY that uses hwmon, and they > > > don't realise that it breaks hwmon on it, and several kernel versions > > > go by without it being noticed. It will only be a noticable issue > > > when the associated network device is down, and that network device > > > driver detaches from the PHY, so _is_ likely not to be noticed. > > > > > > Or should we do a small amount of work now to properly implement > > > regulator support, which includes a trivial grep for "hwmon" amongst > > > the PHY drivers, and add the necessary call to avoid the regulator > > > being shut off. > > > > > > > I'm not sure what the correct approach is here. Provide some helper > > that, when called, would increase the regulator's reference count even > > more to keep it enabled from the moment hwmon is registered to when > > the driver is detached? > > I think a PHY driver needs the utility to control this. We need to be > careful here with naming, because phylib is not the only code in the > kernel that uses the phy_ prefix. > > If we had runtime PM support for PHYs, with regulator support hooked > into runtime PM, then we already have standard interfaces that drivers > can use to control whether the device gets powered down. Other ideas: - using genpd outside of the SoC to provide power domain management. This is already hooked into runtime PM, but would need their agreement, a genpd provider written, and runtime PM added to phylib. - if we're going for some core driver model approach, then the driver model only knows when devices are bound and unbound to their driver, it knows nothing of phylib's attach/detach from the network interface. If we want to shut off power when a PHY is not attached, we would likely need some kind of interface to do that.
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c index 58923826838b..d755adb748a5 100644 --- a/drivers/net/phy/phy_device.c +++ b/drivers/net/phy/phy_device.c @@ -827,7 +827,12 @@ int phy_device_register(struct phy_device *phydev) err = mdiobus_register_device(&phydev->mdio); if (err) - return err; + goto err_out; + + /* Enable the PHY regulator */ + err = phy_device_power_on(phydev); + if (err) + goto err_unregister_mdio; /* Deassert the reset signal */ phy_device_reset(phydev, 0); @@ -846,22 +851,25 @@ int phy_device_register(struct phy_device *phydev) err = phy_scan_fixups(phydev); if (err) { phydev_err(phydev, "failed to initialize\n"); - goto out; + goto err_reset; } err = device_add(&phydev->mdio.dev); if (err) { phydev_err(phydev, "failed to add\n"); - goto out; + goto err_reset; } return 0; - out: +err_reset: /* Assert the reset signal */ phy_device_reset(phydev, 1); - + /* Disable the PHY regulator */ + phy_device_power_off(phydev); +err_unregister_mdio: mdiobus_unregister_device(&phydev->mdio); +err_out: return err; } EXPORT_SYMBOL(phy_device_register); @@ -883,6 +891,8 @@ void phy_device_remove(struct phy_device *phydev) /* Assert the reset signal */ phy_device_reset(phydev, 1); + /* Disable the PHY regulator */ + phy_device_power_off(phydev); mdiobus_unregister_device(&phydev->mdio); } @@ -1064,6 +1074,11 @@ int phy_init_hw(struct phy_device *phydev) { int ret = 0; + /* Enable the PHY regulator */ + ret = phy_device_power_on(phydev); + if (ret) + return ret; + /* Deassert the reset signal */ phy_device_reset(phydev, 0); @@ -1644,6 +1659,8 @@ void phy_detach(struct phy_device *phydev) /* Assert the reset signal */ phy_device_reset(phydev, 1); + /* Disable the PHY regulator */ + phy_device_power_off(phydev); } EXPORT_SYMBOL(phy_detach); @@ -2684,13 +2701,18 @@ static int phy_probe(struct device *dev) mutex_lock(&phydev->lock); + /* Enable the PHY regulator */ + err = phy_device_power_on(phydev); + if (err) + goto out; + /* Deassert the reset signal */ phy_device_reset(phydev, 0); if (phydev->drv->probe) { err = phydev->drv->probe(phydev); if (err) - goto out; + goto out_reset; } /* Start out supporting everything. Eventually, @@ -2708,7 +2730,7 @@ static int phy_probe(struct device *dev) } if (err) - goto out; + goto out_reset; if (!linkmode_test_bit(ETHTOOL_LINK_MODE_Autoneg_BIT, phydev->supported)) @@ -2751,11 +2773,16 @@ static int phy_probe(struct device *dev) /* Set the state to READY by default */ phydev->state = PHY_READY; -out: - /* Assert the reset signal */ - if (err) - phy_device_reset(phydev, 1); + mutex_unlock(&phydev->lock); + + return 0; +out_reset: + /* Assert the reset signal */ + phy_device_reset(phydev, 1); + /* Disable the PHY regulator */ + phy_device_power_off(phydev); +out: mutex_unlock(&phydev->lock); return err; diff --git a/include/linux/phy.h b/include/linux/phy.h index 01d24a934ad1..585ce8db32cf 100644 --- a/include/linux/phy.h +++ b/include/linux/phy.h @@ -1291,6 +1291,16 @@ static inline void phy_device_reset(struct phy_device *phydev, int value) mdio_device_reset(&phydev->mdio, value); } +static inline int phy_device_power_on(struct phy_device *phydev) +{ + return mdio_device_power_on(&phydev->mdio); +} + +static inline int phy_device_power_off(struct phy_device *phydev) +{ + return mdio_device_power_off(&phydev->mdio); +} + #define phydev_err(_phydev, format, args...) \ dev_err(&_phydev->mdio.dev, format, ##args)