| Message ID | 20200622093744.13685-10-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:38AM +0200, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > Currently the PHY ID is read without taking the PHY out of reset. This > can only work if no resets are defined. This change delays the ID read > until we're actually registering the PHY device - this is needed because > earlier (when creating the device) we don't have a struct device yet > with resets already configured. > > While we could use the of_ helpers for GPIO and resets, we will be adding > PHY regulator support layer on and there are no regulator APIs that work > without struct device. The PHY subsystem cannot be the first to run into this problem, that you need a device structure to make use of the regulator API, but you need the regulator API to probe the device. How do other subsystems work around this? Maybe it is time to add a lower level API to the regulator framework? Andrew
On Mon, Jun 22, 2020 at 03:39:40PM +0200, Andrew Lunn wrote: > The PHY subsystem cannot be the first to run into this problem, that > you need a device structure to make use of the regulator API, but you > need the regulator API to probe the device. How do other subsystems > work around this? If the bus includes power management for the devices on the bus the controller is generally responsible for that rather than the devices, the devices access this via facilities provided by the bus if needed. If the device is enumerated by firmware prior to being physically enumerable then the bus will generally instantiate the device model device and then arrange to wait for the physical device to appear and get joined up with the device model device, typically in such situations the physical device might appear and disappear dynamically at runtime based on what the driver is doing anyway. > Maybe it is time to add a lower level API to the regulator framework? I don't see any need for that here, this is far from the only thing that's keyed off a struct device and having the device appear and disappear at runtime can make things like runtime PM look really messy to userspace. We could use a pre-probe stage in the device model for hotpluggable buses in embedded contexts where you might need to bring things out of reset or power them up before they'll appear on the bus for enumeration but buses have mostly handled that at their level.
On 6/22/20 6:51 AM, Mark Brown wrote: > On Mon, Jun 22, 2020 at 03:39:40PM +0200, Andrew Lunn wrote: > >> The PHY subsystem cannot be the first to run into this problem, that >> you need a device structure to make use of the regulator API, but you >> need the regulator API to probe the device. How do other subsystems >> work around this? > > If the bus includes power management for the devices on the bus the > controller is generally responsible for that rather than the devices, > the devices access this via facilities provided by the bus if needed. > If the device is enumerated by firmware prior to being physically > enumerable then the bus will generally instantiate the device model > device and then arrange to wait for the physical device to appear and > get joined up with the device model device, typically in such situations > the physical device might appear and disappear dynamically at runtime > based on what the driver is doing anyway. In premise there is nothing that prevents the MDIO bus from taking care of the regulators, resets, prior to probing the PHY driver, what is complicated here is that we do need to issue a read of the actual PHY to know its 32-bit unique identifier and match it with an appropriate driver. The way that we have worked around this with if you do not wish such a hardware access to be made, is to provide an Ethernet PHY node compatible string that encodes that 32-bit OUI directly. In premise the same challenges exist with PCI devices/endpoints as well as USB, would they have reset or regulator typically attached to them. > >> Maybe it is time to add a lower level API to the regulator framework? > > I don't see any need for that here, this is far from the only thing > that's keyed off a struct device and having the device appear and > disappear at runtime can make things like runtime PM look really messy > to userspace. > > We could use a pre-probe stage in the device model for hotpluggable > buses in embedded contexts where you might need to bring things out of > reset or power them up before they'll appear on the bus for enumeration > but buses have mostly handled that at their level. > That sounds like a better solution, are there any subsystems currently implementing that, or would this be a generic Linux device driver model addition that needs to be done?
On Tue, Jun 23, 2020 at 12:49:15PM -0700, Florian Fainelli wrote: > On 6/22/20 6:51 AM, Mark Brown wrote: > > If the bus includes power management for the devices on the bus the > > controller is generally responsible for that rather than the devices, > > the devices access this via facilities provided by the bus if needed. > > If the device is enumerated by firmware prior to being physically > > enumerable then the bus will generally instantiate the device model > > device and then arrange to wait for the physical device to appear and > > get joined up with the device model device, typically in such situations > > the physical device might appear and disappear dynamically at runtime > > based on what the driver is doing anyway. > In premise there is nothing that prevents the MDIO bus from taking care > of the regulators, resets, prior to probing the PHY driver, what is > complicated here is that we do need to issue a read of the actual PHY to > know its 32-bit unique identifier and match it with an appropriate > driver. The way that we have worked around this with if you do not wish > such a hardware access to be made, is to provide an Ethernet PHY node > compatible string that encodes that 32-bit OUI directly. In premise the > same challenges exist with PCI devices/endpoints as well as USB, would > they have reset or regulator typically attached to them. That all sounds very normal and is covered by both cases I describe? > > We could use a pre-probe stage in the device model for hotpluggable > > buses in embedded contexts where you might need to bring things out of > > reset or power them up before they'll appear on the bus for enumeration > > but buses have mostly handled that at their level. > That sounds like a better solution, are there any subsystems currently > implementing that, or would this be a generic Linux device driver model > addition that needs to be done? Like I say I'm suggesting doing something at the device model level.
śr., 24 cze 2020 o 11:43 Mark Brown <broonie@kernel.org> napisał(a): > > On Tue, Jun 23, 2020 at 12:49:15PM -0700, Florian Fainelli wrote: > > On 6/22/20 6:51 AM, Mark Brown wrote: > > > > If the bus includes power management for the devices on the bus the > > > controller is generally responsible for that rather than the devices, > > > the devices access this via facilities provided by the bus if needed. > > > If the device is enumerated by firmware prior to being physically > > > enumerable then the bus will generally instantiate the device model > > > device and then arrange to wait for the physical device to appear and > > > get joined up with the device model device, typically in such situations > > > the physical device might appear and disappear dynamically at runtime > > > based on what the driver is doing anyway. > > > In premise there is nothing that prevents the MDIO bus from taking care > > of the regulators, resets, prior to probing the PHY driver, what is > > complicated here is that we do need to issue a read of the actual PHY to > > know its 32-bit unique identifier and match it with an appropriate > > driver. The way that we have worked around this with if you do not wish > > such a hardware access to be made, is to provide an Ethernet PHY node > > compatible string that encodes that 32-bit OUI directly. In premise the > > same challenges exist with PCI devices/endpoints as well as USB, would > > they have reset or regulator typically attached to them. > > That all sounds very normal and is covered by both cases I describe? > > > > We could use a pre-probe stage in the device model for hotpluggable > > > buses in embedded contexts where you might need to bring things out of > > > reset or power them up before they'll appear on the bus for enumeration > > > but buses have mostly handled that at their level. > > > That sounds like a better solution, are there any subsystems currently > > implementing that, or would this be a generic Linux device driver model > > addition that needs to be done? > > Like I say I'm suggesting doing something at the device model level. I didn't expect to open such a can of worms... This has evolved into several new concepts being proposed vs my use-case which is relatively simple. The former will probably take several months of development, reviews and discussions and it will block supporting the phy supply on pumpkin boards upstream. I would prefer not to redo what other MAC drivers do (phy-supply property on the MAC node, controlling it from the MAC driver itself) if we've already established it's wrong. Is there any compromise we could reach to add support for a basic, common use-case of a single regulator supplying a PHY that needs enabling before reading its ID short-term (just like we currently support a single reset or reset-gpios property for PHYs) and introducing a whole new concept to the device model for more advanced (but currently mostly hypothetical) cases long-term? Bart
On 6/24/2020 6:48 AM, Bartosz Golaszewski wrote: > śr., 24 cze 2020 o 11:43 Mark Brown <broonie@kernel.org> napisał(a): >> >> On Tue, Jun 23, 2020 at 12:49:15PM -0700, Florian Fainelli wrote: >>> On 6/22/20 6:51 AM, Mark Brown wrote: >> >>>> If the bus includes power management for the devices on the bus the >>>> controller is generally responsible for that rather than the devices, >>>> the devices access this via facilities provided by the bus if needed. >>>> If the device is enumerated by firmware prior to being physically >>>> enumerable then the bus will generally instantiate the device model >>>> device and then arrange to wait for the physical device to appear and >>>> get joined up with the device model device, typically in such situations >>>> the physical device might appear and disappear dynamically at runtime >>>> based on what the driver is doing anyway. >> >>> In premise there is nothing that prevents the MDIO bus from taking care >>> of the regulators, resets, prior to probing the PHY driver, what is >>> complicated here is that we do need to issue a read of the actual PHY to >>> know its 32-bit unique identifier and match it with an appropriate >>> driver. The way that we have worked around this with if you do not wish >>> such a hardware access to be made, is to provide an Ethernet PHY node >>> compatible string that encodes that 32-bit OUI directly. In premise the >>> same challenges exist with PCI devices/endpoints as well as USB, would >>> they have reset or regulator typically attached to them. >> >> That all sounds very normal and is covered by both cases I describe? >> >>>> We could use a pre-probe stage in the device model for hotpluggable >>>> buses in embedded contexts where you might need to bring things out of >>>> reset or power them up before they'll appear on the bus for enumeration >>>> but buses have mostly handled that at their level. >> >>> That sounds like a better solution, are there any subsystems currently >>> implementing that, or would this be a generic Linux device driver model >>> addition that needs to be done? >> >> Like I say I'm suggesting doing something at the device model level. > > I didn't expect to open such a can of worms... > > This has evolved into several new concepts being proposed vs my > use-case which is relatively simple. The former will probably take > several months of development, reviews and discussions and it will > block supporting the phy supply on pumpkin boards upstream. I would > prefer not to redo what other MAC drivers do (phy-supply property on > the MAC node, controlling it from the MAC driver itself) if we've > already established it's wrong. You are not new to Linux development, so none of this should come as a surprise to you. Your proposed solution has clearly short comings and is a hack, especially around the PHY_ID_NONE business to get a phy_device only then to have the real PHY device ID. You should also now that "I need it now because my product deliverable depends on it" has never been received as a valid argument to coerce people into accepting a solution for which there are at review time known deficiencies to the proposed approach. > > Is there any compromise we could reach to add support for a basic, > common use-case of a single regulator supplying a PHY that needs > enabling before reading its ID short-term (just like we currently > support a single reset or reset-gpios property for PHYs) and > introducing a whole new concept to the device model for more advanced > (but currently mostly hypothetical) cases long-term? The pre-probe use case is absolutely not hypothetical, and I would need it for pcie-brcmstb.c at some point which is a PCIe root complex driver with multiple regulators that need to be turned on *prior* to enumerating the PCIe bus and creating pci_device instances. It is literally the same thing as what you are trying to do, just in a different subsystem, therefore I am happy to test and review your patches.
On Wed, Jun 24, 2020 at 6:06 PM Florian Fainelli <f.fainelli@gmail.com> wrote: > [snip!] > > > > This has evolved into several new concepts being proposed vs my > > use-case which is relatively simple. The former will probably take > > several months of development, reviews and discussions and it will > > block supporting the phy supply on pumpkin boards upstream. I would > > prefer not to redo what other MAC drivers do (phy-supply property on > > the MAC node, controlling it from the MAC driver itself) if we've > > already established it's wrong. > > You are not new to Linux development, so none of this should come as a > surprise to you. Your proposed solution has clearly short comings and is > a hack, especially around the PHY_ID_NONE business to get a phy_device > only then to have the real PHY device ID. You should also now that "I > need it now because my product deliverable depends on it" has never been > received as a valid argument to coerce people into accepting a solution > for which there are at review time known deficiencies to the proposed > approach. > Don't get me wrong, I understand that full well. On the other hand a couple years ago I put a significant amount of work into the concept of early platform device drivers for linux clocksource, clock and interrupt drivers. Every reviewer had his own preferred approach and after something like three completely different submissions and several conversations at conferences I simply gave up due to all the bikeshedding. It just wasn't moving forward and frankly: I expect any changes to the core driver model to follow a similar path of most resistance. I will give it a shot but at some point getting the job done is better than not getting it done just because the solution isn't perfect. IMO this approach is still slightly more correct than controlling the PHY's supply from the MAC driver. Bartosz
On Wed, Jun 24, 2020 at 09:06:28AM -0700, Florian Fainelli wrote: > On 6/24/2020 6:48 AM, Bartosz Golaszewski wrote: > > I didn't expect to open such a can of worms... > > > > This has evolved into several new concepts being proposed vs my > > use-case which is relatively simple. The former will probably take > > several months of development, reviews and discussions and it will > > block supporting the phy supply on pumpkin boards upstream. I would > > prefer not to redo what other MAC drivers do (phy-supply property on > > the MAC node, controlling it from the MAC driver itself) if we've > > already established it's wrong. > > You are not new to Linux development, so none of this should come as a > surprise to you. Your proposed solution has clearly short comings and is > a hack, especially around the PHY_ID_NONE business to get a phy_device > only then to have the real PHY device ID. You should also now that "I > need it now because my product deliverable depends on it" has never been > received as a valid argument to coerce people into accepting a solution > for which there are at review time known deficiencies to the proposed > approach. It /is/ a generic issue. The same problem exists for AMBA Primecell devices, and that code has an internal deferred device list that it manages. See drivers/amba/bus.c, amba_deferred_retry_func(), amba_device_try_add(), and amba_device_add(). As we see more devices gain this property, it needs to be addressed in a generic way, rather than coming up with multiple bus specific implementations. Maybe struct bus_type needs a method to do the preparation to add a device (such as reading IDs etc), which is called by device_add(). If that method returns -EPROBE_DEFER, the device gets added to a deferred list, which gets retried when drivers are successfully probed. Possible maybe?
On 2020-06-24 17:50, Russell King - ARM Linux admin wrote: > On Wed, Jun 24, 2020 at 09:06:28AM -0700, Florian Fainelli wrote: >> On 6/24/2020 6:48 AM, Bartosz Golaszewski wrote: >>> I didn't expect to open such a can of worms... >>> >>> This has evolved into several new concepts being proposed vs my >>> use-case which is relatively simple. The former will probably take >>> several months of development, reviews and discussions and it will >>> block supporting the phy supply on pumpkin boards upstream. I would >>> prefer not to redo what other MAC drivers do (phy-supply property on >>> the MAC node, controlling it from the MAC driver itself) if we've >>> already established it's wrong. >> >> You are not new to Linux development, so none of this should come as a >> surprise to you. Your proposed solution has clearly short comings and is >> a hack, especially around the PHY_ID_NONE business to get a phy_device >> only then to have the real PHY device ID. You should also now that "I >> need it now because my product deliverable depends on it" has never been >> received as a valid argument to coerce people into accepting a solution >> for which there are at review time known deficiencies to the proposed >> approach. > > It /is/ a generic issue. The same problem exists for AMBA Primecell > devices, and that code has an internal deferred device list that it > manages. See drivers/amba/bus.c, amba_deferred_retry_func(), > amba_device_try_add(), and amba_device_add(). > > As we see more devices gain this property, it needs to be addressed > in a generic way, rather than coming up with multiple bus specific > implementations. > > Maybe struct bus_type needs a method to do the preparation to add > a device (such as reading IDs etc), which is called by device_add(). > If that method returns -EPROBE_DEFER, the device gets added to a > deferred list, which gets retried when drivers are successfully > probed. Possible maybe? FWIW that would be ideal for solving an ordering a problem we have in the IOMMU subsystem too (which we currently sort-of-handle by deferring driver probe from dma_configure(), but it really needs to be done earlier and not depend on drivers being present at all). Robin.
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c index eccbf6aea63d..94944fffa9bb 100644 --- a/drivers/net/phy/phy_device.c +++ b/drivers/net/phy/phy_device.c @@ -658,12 +658,6 @@ struct phy_device *phy_device_create(struct mii_bus *bus, int addr, u32 phy_id, INIT_DELAYED_WORK(&dev->state_queue, phy_state_machine); device_initialize(&mdiodev->dev); - ret = phy_request_driver_module(dev); - if (ret) { - phy_device_free(dev); - dev = ERR_PTR(ret); - } - return dev; } EXPORT_SYMBOL(phy_device_create); @@ -813,30 +807,29 @@ static int get_phy_id(struct mii_bus *bus, int addr, u32 *phy_id, return 0; } +static int phy_device_read_id(struct phy_device *phydev) +{ + struct mdio_device *mdiodev = &phydev->mdio; + + phydev->c45_ids.devices_in_package = 0; + memset(phydev->c45_ids.device_ids, 0xff, + sizeof(phydev->c45_ids.device_ids)); + + return get_phy_id(mdiodev->bus, mdiodev->addr, &phydev->phy_id, + phydev->is_c45, &phydev->c45_ids); +} + /** - * get_phy_device - reads the specified PHY device and returns its @phy_device - * struct + * get_phy_device - create a phy_device withoug PHY ID * @bus: the target MII bus * @addr: PHY address on the MII bus * @is_c45: If true the PHY uses the 802.3 clause 45 protocol * - * Description: Reads the ID registers of the PHY at @addr on the - * @bus, then allocates and returns the phy_device to represent it. + * Allocates a new phy_device for @addr on the @bus. */ struct phy_device *get_phy_device(struct mii_bus *bus, int addr, bool is_c45) { - struct phy_c45_device_ids c45_ids; - u32 phy_id = 0; - int r; - - c45_ids.devices_in_package = 0; - memset(c45_ids.device_ids, 0xff, sizeof(c45_ids.device_ids)); - - r = get_phy_id(bus, addr, &phy_id, is_c45, &c45_ids); - if (r) - return ERR_PTR(r); - - return phy_device_create(bus, addr, phy_id, is_c45, &c45_ids); + return phy_device_create(bus, addr, PHY_ID_NONE, is_c45, NULL); } EXPORT_SYMBOL(get_phy_device); @@ -855,6 +848,16 @@ int phy_device_register(struct phy_device *phydev) /* Deassert the reset signal */ phy_device_reset(phydev, 0); + if (phydev->phy_id == PHY_ID_NONE) { + err = phy_device_read_id(phydev); + if (err) + goto err_unregister_mdio; + } + + err = phy_request_driver_module(phydev); + if (err) + goto err_unregister_mdio; + /* Run all of the fixups for this PHY */ err = phy_scan_fixups(phydev); if (err) { diff --git a/include/linux/phy.h b/include/linux/phy.h index 8c05d0fb5c00..2a695cd90c7c 100644 --- a/include/linux/phy.h +++ b/include/linux/phy.h @@ -742,6 +742,7 @@ struct phy_driver { #define PHY_ANY_ID "MATCH ANY PHY" #define PHY_ANY_UID 0xffffffff +#define PHY_ID_NONE 0 #define PHY_ID_MATCH_EXACT(id) .phy_id = (id), .phy_id_mask = GENMASK(31, 0) #define PHY_ID_MATCH_MODEL(id) .phy_id = (id), .phy_id_mask = GENMASK(31, 4)