| Message ID | 20200903043947.3272453-1-f.fainelli@gmail.com |
|---|---|
| Headers | show |
| Series | net: phy: Support enabling clocks prior to bus probe | expand |
On Wed, Sep 02, 2020 at 09:39:45PM -0700, Florian Fainelli wrote: > Some Ethernet PHYs may require that their clock, which typically drives > their logic to respond to reads on the MDIO bus be enabled before > issusing a MDIO bus scan. > > We have a chicken and egg problem though which is that we cannot enable > a given Ethernet PHY's device clock until we have a phy_device instance > create and called the driver's probe function. This will not happen > unless we are successful in probing the PHY device, which requires its > clock(s) to be turned on. > > For DT based systems we can solve this by using of_clk_get() which > operates on a device_node reference, and make sure that all clocks > associaed with the node are enabled prior to doing any reads towards the > device. In order to avoid drivers having to know the a priori reference > count of the resources, we drop them back to 0 right before calling > ->probe() which is then supposed to manage the resources normally. Hi Florian This seems asymmetric. The resources are enabled in of_mdiobus_phy_device_register() but disabled in phy_probe(). What we are really interested in is having the clock ticking while trying to read the ID registers. Could the enable/disable be placed around the call to get_phy_id()? Andrew
On 9/3/2020 2:28 PM, Rob Herring wrote: > On Wed, Sep 2, 2020 at 10:39 PM Florian Fainelli <f.fainelli@gmail.com> wrote: >> >> Some Ethernet PHYs may require that their clock, which typically drives >> their logic to respond to reads on the MDIO bus be enabled before >> issusing a MDIO bus scan. >> >> We have a chicken and egg problem though which is that we cannot enable >> a given Ethernet PHY's device clock until we have a phy_device instance >> create and called the driver's probe function. This will not happen >> unless we are successful in probing the PHY device, which requires its >> clock(s) to be turned on. >> >> For DT based systems we can solve this by using of_clk_get() which >> operates on a device_node reference, and make sure that all clocks >> associaed with the node are enabled prior to doing any reads towards the >> device. In order to avoid drivers having to know the a priori reference >> count of the resources, we drop them back to 0 right before calling >> ->probe() which is then supposed to manage the resources normally. > > What if a device requires clocks enabled in a certain order or timing? > It's not just clocks, you could have some GPIOs or a regulator that > need enabling first. It's device specific, so really needs a per > device solution. This is not just an issue with MDIO. I think we > really need some sort of pre-probe hook in the driver model in order > to do any non-discoverable init for discoverable buses. Or perhaps > forcing probe when there are devices defined in DT if they're not > discovered by normal means. I like the pre-probe hook idea, and there are other devices that I have access to that would benefit from that, like PCIe end-points that require regulators to be turned on in order for them to be discoverable. For MDIO we might actually be able to create the backing device reference without having read the device ID yet, provided that we know its address on the bus, which DT can tell us. Bartosz attempted to do that not so long ago and we sort of stalled there, too: https://lkml.org/lkml/2020/6/22/253 Let me see if I can just add a pre-probe hook, make use of it in the MDIO layer, and we see how we can apply it to other subsystems?
On 9/2/2020 9:39 PM, Florian Fainelli wrote: > Hi all, > > This patch series takes care of enabling the Ethernet PHY clocks in > DT-based systems (we have no way to do it for ACPI, and ACPI would > likely keep all of this hardware enabled anyway). > > Please test on your respective platforms, mine still seems to have > a race condition that I am tracking down as it looks like we are not > waiting long enough post clock enable. > > The check on the clock reference count is necessary to avoid an > artificial bump of the clock reference count and to support the unbind > -> bind of the PHY driver. We could solve it in different ways. > > Comments and test results welcome! Andrew, while we figure out a proper way to support this with the Linux device driver model, would you be opposed in a single patch to drivers/net/mdio/mdio-bcm-unimac.c which takes care of enabling the PHY's clock during bus->reset just for the sake of getting those systems to work, and later on we move over to the pre-probe mechanism? That would allow me to continue working with upstream kernels on these systems without carrying a big pile of patches.
On 20-09-02 21:39, Florian Fainelli wrote: > The internal Gigabit PHY on Broadcom STB chips has a digital clock which > drives its MDIO interface among other things, the driver now requests > and manage that clock during .probe() and .remove() accordingly. Hi Florian, Seems like you added the same support here like I did for the smsc driver. So should I go with my proposed patch which can be adapted later after you guys figured out who to enable the required resources? Regards, Marco
> Just a bunch of questions. > > Actually, why is it necessary to have a full MDIO bus scan already during > probing peripherals? That is the Linux bus model. It does not matter what sort of bus it is, PCI, USB, MDIO, etc. When the bus driver is loaded, the bus is enumerated and drivers probe for each device found on the bus. > I'd say that it is not necessary to have a PHY getting found before it is > needed to setup the complete interface. It is like saying, we don't need to probe the keyboard until the first time the "Press Enter" prompt is given? Andrew
W dniu 2020-09-04 o 15:45, Andrew Lunn pisze: >> Just a bunch of questions. >> >> Actually, why is it necessary to have a full MDIO bus scan already during >> probing peripherals? > That is the Linux bus model. It does not matter what sort of bus it > is, PCI, USB, MDIO, etc. When the bus driver is loaded, the bus is > enumerated and drivers probe for each device found on the bus. OK. But is it always expected to find all the devices on the bus in the first run? Does the bus model ever allow to just add any more devices? Kind of, "hotplug". :) >> I'd say that it is not necessary to have a PHY getting found before it is >> needed to setup the complete interface. > It is like saying, we don't need to probe the keyboard until the first > time the "Press Enter" prompt is given? I'm not sure what you mean. It's like saying that we don't need to care if we even have the keyboard until we are interested in any interaction with it. (This might be reading a key, an autotest, ..., or not using, but avoiding a conflict - depends on application.) Best regards, Adam
On 9/3/2020 11:18 PM, Marco Felsch wrote: > On 20-09-02 21:39, Florian Fainelli wrote: >> The internal Gigabit PHY on Broadcom STB chips has a digital clock which >> drives its MDIO interface among other things, the driver now requests >> and manage that clock during .probe() and .remove() accordingly. > > Hi Florian, > > Seems like you added the same support here like I did for the smsc > driver. So should I go with my proposed patch which can be adapted later > after you guys figured out who to enable the required resources? That seems fine to me, on your platform there appears to be an assumption that we will be able to probe the SMSC PHY because everything we need is already enabled, right? If so, this patch series does not change that state.
On 20-09-04 08:37, Florian Fainelli wrote: ... > > Is this really necessary? The devm_clk_get_optional() function already > > registers the devm_clk_release() hook. > > Yes, because you can unbind the PHY driver from sysfs, and if you want to > bind that driver again, which will call .probe() again, you must undo > strictly everything that .probe() did. The embedded mdio_device does not go > away, so there will be no automatic freeing of resources. Using devm_* may > be confusing, so using just the plain clk_get() and clk_put() may be clearer > here. Hi Florian, sorry for asking again... I'm getting a bit confused during applying your comments to my smsc-phy patchset. A few drivers are using the devm_kzalloc() (including your bcm7xxx.c and my smsc.c). Does this mean that those drivers have a memory leak since the mdio_device does not disappear and so the memory allocated during probe() isn't freed? Regards, Marco