Re: [PATCH 14/15] net: phy: add PHY regulator support

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:

quoted

wt., 23 cze 2020 o 11:56 Russell King - ARM Linux admin
[off-list ref] napisał(a):

quoted

On Tue, Jun 23, 2020 at 11:46:15AM +0200, Bartosz Golaszewski wrote:

quoted

wt., 23 cze 2020 o 11:43 Russell King - ARM Linux admin
[off-list ref] napisał(a):

quoted

On Tue, Jun 23, 2020 at 11:41:11AM +0200, Bartosz Golaszewski wrote:

quoted

pon., 22 cze 2020 o 15:29 Russell King - ARM Linux admin
[off-list ref] napisał(a):

quoted

[snip!]

quoted

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.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel