On 9/2/2020 3:20 PM, Andrew Lunn wrote:
+ priv->clk = devm_clk_get_optional(&phydev->mdio.dev, "sw_gphy");
+ if (IS_ERR(priv->clk))
+ return PTR_ERR(priv->clk);
+
+ /* To get there, the mdiobus registration logic already enabled our
+ * clock otherwise we would not have probed this device since we would
+ * not be able to read its ID. To avoid artificially bumping up the
+ * clock reference count, only do the clock enable from a phy_remove ->
+ * phy_probe path (driver unbind, then rebind).
+ */
+ if (!__clk_is_enabled(priv->clk))
+ ret = clk_prepare_enable(priv->clk);
This i don't get. The clock subsystem does reference counting. So what
i would expect to happen is that during scanning of the bus, phylib
enables the clock and keeps it enabled until after probe. To keep
things balanced, phylib would disable the clock after probe.
That would be fine, although it assumes that the individual PHY drivers
have obtained the clocks and called clk_prepare_enable(), which is a
fair assumption I suppose.
If the driver wants the clock enabled all the time, it can enable it
in the probe method. The common clock framework will then have two
reference counts for the clock, so that when the probe exists, and
phylib disables the clock, the CCF keeps the clock ticking. The PHY
driver can then disable the clock in .remove.
But then the lowest count you will have is 1, which will lead to the
clock being left on despite having unbound the PHY driver from the
device (->remove was called). This does not allow saving any power
unfortunately.
There are some PHYs which will enumerate with the clock disabled. They
only need it ticking for packet transfer. Such PHY drivers can enable
the clock only when needed in order to save some power when the
interface is administratively down.
Then the best approach would be for the OF scanning code to enable all
clocks reference by the Ethernet PHY node (like it does in the proposed
patch), since there is no knowledge of which clock is necessary and all
must be assumed to be critical for MDIO bus scanning. Right before
drv->probe() we drop all resources reference counts, and from there on
->probe() is assumed to manage the necessary clocks.
It looks like another solution may be to use the assigned-clocks
property which will take care of assigning clock references to devices
and having those applied as soon as the clock provider is available.
--
Florian
