[RFC net-next 0/2] net: phy: Support enabling clocks prior to bus probe

[RFC net-next 0/2] net: phy: Support enabling clocks prior to bus probe

[Florian Fainelli]

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!

Florian Fainelli (2):
  net: phy: Support enabling clocks prior to bus probe
  net: phy: bcm7xxx: request and manage GPHY clock

 drivers/net/phy/bcm7xxx.c    | 29 ++++++++++++-
 drivers/net/phy/phy_device.c |  6 ++-
 drivers/of/of_mdio.c         | 84 ++++++++++++++++++++++++++++++++++++
 include/linux/of_mdio.h      |  7 +++
 include/linux/phy.h          | 12 ++++++
 5 files changed, 136 insertions(+), 2 deletions(-)

-- 
2.25.1

[RFC net-next 1/2] net: phy: Support enabling clocks prior to bus probe

[Florian Fainelli]

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.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/phy/phy_device.c |  6 ++-
 drivers/of/of_mdio.c         | 84 ++++++++++++++++++++++++++++++++++++
 include/linux/of_mdio.h      |  7 +++
 include/linux/phy.h          | 12 ++++++
 4 files changed, 108 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 57d44648c8dd..f3f67dee0309 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -23,6 +23,7 @@
 #include <linux/mm.h>
 #include <linux/module.h>
 #include <linux/netdevice.h>
+#include <linux/of_mdio.h>
 #include <linux/phy.h>
 #include <linux/phy_led_triggers.h>
 #include <linux/property.h>
@@ -2914,8 +2915,11 @@ static int phy_probe(struct device *dev)
 
 out:
 	/* Assert the reset signal */
-	if (err)
+	if (err) {
 		phy_device_reset(phydev, 1);
+		of_mdiobus_device_disable_resources(phydev->mdio.bus,
+						    phydev->mdio.addr);
+	}
 
 	mutex_unlock(&phydev->lock);
 
diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
index cb32d7ef4938..de5e3388b581 100644
--- a/drivers/of/of_mdio.c
+++ b/drivers/of/of_mdio.c
@@ -19,6 +19,7 @@
 #include <linux/of_mdio.h>
 #include <linux/of_net.h>
 #include <linux/module.h>
+#include <linux/clk.h>
 
 #define DEFAULT_GPIO_RESET_DELAY	10	/* in microseconds */
 
@@ -60,6 +61,81 @@ static struct mii_timestamper *of_find_mii_timestamper(struct device_node *node)
 	return register_mii_timestamper(arg.np, arg.args[0]);
 }
 
+static int of_mdiobus_device_enable_resources(struct mii_bus *bus,
+					      struct device_node *child,
+					      u32 addr)
+{
+	struct mdio_device_resource *res = &bus->mdio_resources[addr];
+	unsigned int i;
+	int rc;
+
+	rc = of_count_phandle_with_args(child, "clocks", "#clock-cells");
+	if (rc < 0) {
+		if (rc == -ENOENT)
+			rc = 0;
+		else
+			return rc;
+	}
+
+	res->num_clks = rc;
+	if (rc == 0)
+		return rc;
+
+	dev_dbg(&bus->dev, "Found %d clocks for child at %d\n", rc, addr);
+
+	res->clks = devm_kcalloc(&bus->dev, res->num_clks,
+				 sizeof(struct clk *), GFP_KERNEL);
+	if (!res->clks)
+		return -ENOMEM;
+
+	for (i = 0; i < res->num_clks; i++) {
+		res->clks[i] = of_clk_get(child, i);
+		if (IS_ERR(res->clks[i])) {
+			if (PTR_ERR(res->clks[i]) == -ENOENT)
+				continue;
+
+			return PTR_ERR(res->clks[i]);
+		}
+
+		rc = clk_prepare_enable(res->clks[i]);
+		if (rc) {
+			dev_err(&bus->dev,
+				"Failed to enabled clock for %d (rc: %d)\n",
+				addr, rc);
+			goto out_clk_disable;
+		}
+
+		dev_dbg(&bus->dev,
+			"Enable clock %d for child at %d\n",
+			i, addr);
+	}
+
+	return 0;
+
+out_clk_disable:
+	while (i-- > 0) {
+		clk_disable_unprepare(res->clks[i]);
+		clk_put(res->clks[i]);
+	}
+	return rc;
+}
+
+void of_mdiobus_device_disable_resources(struct mii_bus *bus, u32 addr)
+{
+	struct mdio_device_resource *res = &bus->mdio_resources[addr];
+	unsigned int i;
+
+	if (res->num_clks == 0)
+		return;
+
+	for (i = 0; i < res->num_clks; i++) {
+		clk_disable_unprepare(res->clks[i]);
+		clk_put(res->clks[i]);
+		dev_dbg(&bus->dev, "Disabled clk %d for %d\n", i, addr);
+	}
+}
+EXPORT_SYMBOL(of_mdiobus_device_disable_resources);
+
 int of_mdiobus_phy_device_register(struct mii_bus *mdio, struct phy_device *phy,
 			      struct device_node *child, u32 addr)
 {
@@ -117,6 +193,12 @@ static int of_mdiobus_register_phy(struct mii_bus *mdio,
 	if (IS_ERR(mii_ts))
 		return PTR_ERR(mii_ts);
 
+	rc = of_mdiobus_device_enable_resources(mdio, child, addr);
+	if (rc) {
+		dev_err(&mdio->dev, "enable resources: %d\n", rc);
+		return rc;
+	}
+
 	is_c45 = of_device_is_compatible(child,
 					 "ethernet-phy-ieee802.3-c45");
 
@@ -125,6 +207,7 @@ static int of_mdiobus_register_phy(struct mii_bus *mdio,
 	else
 		phy = get_phy_device(mdio, addr, is_c45);
 	if (IS_ERR(phy)) {
+		of_mdiobus_device_disable_resources(mdio, addr);
 		if (mii_ts)
 			unregister_mii_timestamper(mii_ts);
 		return PTR_ERR(phy);
@@ -132,6 +215,7 @@ static int of_mdiobus_register_phy(struct mii_bus *mdio,
 
 	rc = of_mdiobus_phy_device_register(mdio, phy, child, addr);
 	if (rc) {
+		of_mdiobus_device_disable_resources(mdio, addr);
 		if (mii_ts)
 			unregister_mii_timestamper(mii_ts);
 		phy_device_free(phy);
diff --git a/include/linux/of_mdio.h b/include/linux/of_mdio.h
index 1efb88d9f892..c0041d2afb0e 100644
--- a/include/linux/of_mdio.h
+++ b/include/linux/of_mdio.h
@@ -58,6 +58,8 @@ static inline int of_mdio_parse_addr(struct device *dev,
 	return addr;
 }
 
+void of_mdiobus_device_disable_resources(struct mii_bus *mdio, u32 addr);
+
 #else /* CONFIG_OF_MDIO */
 static inline bool of_mdiobus_child_is_phy(struct device_node *child)
 {
@@ -129,6 +131,11 @@ static inline int of_mdiobus_phy_device_register(struct mii_bus *mdio,
 {
 	return -ENOSYS;
 }
+
+static inline void of_mdiobus_device_disable_resources(struct mii_bus *mdio,
+						       u32 addr)
+{
+}
 #endif
 
 
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 3a09d2bf69ea..78e64acfc42e 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -247,6 +247,13 @@ struct phy_package_shared {
 #define PHY_SHARED_F_INIT_DONE  0
 #define PHY_SHARED_F_PROBE_DONE 1
 
+struct clk;
+
+struct mdio_device_resource {
+	unsigned int num_clks;
+	struct clk **clks;
+};
+
 /*
  * The Bus class for PHYs.  Devices which provide access to
  * PHYs should register using this structure
@@ -291,6 +298,11 @@ struct mii_bus {
 	 */
 	int irq[PHY_MAX_ADDR];
 
+	/* An array of MDIO device resources that must be enabled
+	 * during probe for identification to succeed.
+	 */
+	struct mdio_device_resource mdio_resources[PHY_MAX_ADDR];
+
 	/* GPIO reset pulse width in microseconds */
 	int reset_delay_us;
 	/* GPIO reset deassert delay in microseconds */
-- 
2.25.1

Re: [RFC net-next 1/2] net: phy: Support enabling clocks prior to bus probe

[Florian Fainelli]

On 9/2/2020 2:33 PM, 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.

That last part is actually not in this patch series, I had it in an 
intermediate version, but after chasing a clock enabling/disabling race 
that appears specific to the platforms I am using, I eventually removed 
it but left that part in the commit message.
-- 
Florian

Re: [RFC net-next 1/2] net: phy: Support enabling clocks prior to bus probe

[Andrew Lunn]

Hi Florian

> 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.

issuing

> 
> 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

associated

Not fully looked at the code yet.

    Andrew

[RFC net-next 2/2] net: phy: bcm7xxx: request and manage GPHY clock

[Florian Fainelli]

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.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/phy/bcm7xxx.c | 29 ++++++++++++++++++++++++++++-
 1 file changed, 28 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/bcm7xxx.c b/drivers/net/phy/bcm7xxx.c
index 692048d86ab1..2b81a11e3167 100644
--- a/drivers/net/phy/bcm7xxx.c
+++ b/drivers/net/phy/bcm7xxx.c
@@ -11,6 +11,8 @@
 #include "bcm-phy-lib.h"
 #include <linux/bitops.h>
 #include <linux/brcmphy.h>
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
 #include <linux/mdio.h>
 
 /* Broadcom BCM7xxx internal PHY registers */
@@ -39,6 +41,7 @@
 
 struct bcm7xxx_phy_priv {
 	u64	*stats;
+	struct clk *clk;
 };
 
 static int bcm7xxx_28nm_d0_afe_config_init(struct phy_device *phydev)
@@ -521,6 +524,7 @@ static void bcm7xxx_28nm_get_phy_stats(struct phy_device *phydev,
 static int bcm7xxx_28nm_probe(struct phy_device *phydev)
 {
 	struct bcm7xxx_phy_priv *priv;
+	int ret = 0;
 
 	priv = devm_kzalloc(&phydev->mdio.dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
@@ -534,7 +538,28 @@ static int bcm7xxx_28nm_probe(struct phy_device *phydev)
 	if (!priv->stats)
 		return -ENOMEM;
 
-	return 0;
+	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);
+
+	return ret;
+}
+
+static void bcm7xxx_28nm_remove(struct phy_device *phydev)
+{
+	struct bcm7xxx_phy_priv *priv = phydev->priv;
+
+	clk_disable_unprepare(priv->clk);
+	devm_clk_put(&phydev->mdio.dev, priv->clk);
 }
 
 #define BCM7XXX_28NM_GPHY(_oui, _name)					\
@@ -552,6 +577,7 @@ static int bcm7xxx_28nm_probe(struct phy_device *phydev)
 	.get_strings	= bcm_phy_get_strings,				\
 	.get_stats	= bcm7xxx_28nm_get_phy_stats,			\
 	.probe		= bcm7xxx_28nm_probe,				\
+	.remove		= bcm7xxx_28nm_remove,				\
 }
 
 #define BCM7XXX_28NM_EPHY(_oui, _name)					\
@@ -567,6 +593,7 @@ static int bcm7xxx_28nm_probe(struct phy_device *phydev)
 	.get_strings	= bcm_phy_get_strings,				\
 	.get_stats	= bcm7xxx_28nm_get_phy_stats,			\
 	.probe		= bcm7xxx_28nm_probe,				\
+	.remove		= bcm7xxx_28nm_remove,				\
 }
 
 #define BCM7XXX_40NM_EPHY(_oui, _name)					\
-- 
2.25.1

Re: [RFC net-next 2/2] net: phy: bcm7xxx: request and manage GPHY clock

[Andrew Lunn]

> +	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.

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.

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.

	  Andrew

Re: [RFC net-next 2/2] net: phy: bcm7xxx: request and manage GPHY clock

[Florian Fainelli]

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

Re: [RFC net-next 2/2] net: phy: bcm7xxx: request and manage GPHY clock

[Adam Rudziński]

W dniu 2020-09-03 o 04:13, Florian Fainelli pisze:
>
>
> 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.

Hi Guys,

I've just realized that a PHY may also have a reset signal connected. 
The reset signal may be controlled by the dedicated peripheral or by GPIO.

In general terms, there might be a set of control signals needed to 
enable the PHY. It seems that the clock and the reset would be the 
typical useful options.

Going further with my imagination of how evil the hardware design could 
be, in general the signals for the PHY may have some relations to other 
control signals.

I think that from the software point of view this comes down to 
assumption that the PHY is to be controlled "driver only knows how".

Best regards,
Adam

Re: [RFC net-next 2/2] net: phy: bcm7xxx: request and manage GPHY clock

[Florian Fainelli]

On 9/2/2020 11:00 PM, Adam Rudziński wrote:
> 
> W dniu 2020-09-03 o 04:13, Florian Fainelli pisze:
>>
>>
>> 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.
> 
> Hi Guys,
> 
> I've just realized that a PHY may also have a reset signal connected. 
> The reset signal may be controlled by the dedicated peripheral or by GPIO.

There is already support for such a thing within 
drivers/net/phy/mdio_bus.c though it assumes we could bind the PHY 
device to its driver already.

> 
> In general terms, there might be a set of control signals needed to 
> enable the PHY. It seems that the clock and the reset would be the 
> typical useful options.
> 
> Going further with my imagination of how evil the hardware design could 
> be, in general the signals for the PHY may have some relations to other 
> control signals.
> 
> I think that from the software point of view this comes down to 
> assumption that the PHY is to be controlled "driver only knows how".

That is all well and good as long as we can actually bind the PHY device 
which its driver, and right now this means that we either have:

- a compatible string in Device Tree which is of the form 
ethernet-phy-id%4x.%4x (see of_get_phy_id) which means that we *know* 
already which PHY we have and we avoid doing reads of MII_PHYSID1 and 
MII_PHYSID2. This is a Linux implementation detail that should not have 
to be known to systems designer IMHO

- a successful read of MII_PHYSID1 and MII_PHYSID2 (or an equivalent for 
the clause 45 PHYs) that allows us to know what PHY device we have, 
which is something that needs to happen eventually.

The problem is when there are essential resources such as clocks, 
regulators, reset signals that must be enabled, respectively de-asserted 
in order for a successful MDIO read of MII_PHYSID1 and MII_PHYSID2 to 
succeed.

There is no driver involvement at that stage because we have no 
phy_device to bind it to *yet*. Depending on what we read from 
MII_PHYSID1/PHY_ID2 we will either successfully bind to the Generic PHY 
driver (assuming we did not read all Fs) or not and we will return 
-ENODEV and then it is game over.

This is the chicken and egg problem that this patch series is 
addressing, for clocks, because we can retrieve clock devices with just 
a device_node reference.

It is absolutely reasonable to design systems whereby the boot loader is 
not initializing or using the Ethernet PHY devices, or if it did, put 
them back into reset/low power state after use and before transitioning 
to the OS. This is not only a clean transition, it may also be the only 
way to meet a certain power target as you cannot know or assume that the 
OS will also make use of these resources. This is even more true if 
there is dynamic power negotiation (MHL or USB Type-C for instance).
-- 
Florian

Re: [RFC net-next 2/2] net: phy: bcm7xxx: request and manage GPHY clock

[Adam Rudziński]

W dniu 2020-09-03 o 17:21, Florian Fainelli pisze:
>
>
> On 9/2/2020 11:00 PM, Adam Rudziński wrote:
>>
>> W dniu 2020-09-03 o 04:13, Florian Fainelli pisze:
>>>
>>>
>>> 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.
>>
>> Hi Guys,
>>
>> I've just realized that a PHY may also have a reset signal connected. 
>> The reset signal may be controlled by the dedicated peripheral or by 
>> GPIO.
>
> There is already support for such a thing within 
> drivers/net/phy/mdio_bus.c though it assumes we could bind the PHY 
> device to its driver already.
>
>>
>> In general terms, there might be a set of control signals needed to 
>> enable the PHY. It seems that the clock and the reset would be the 
>> typical useful options.
>>
>> Going further with my imagination of how evil the hardware design 
>> could be, in general the signals for the PHY may have some relations 
>> to other control signals.
>>
>> I think that from the software point of view this comes down to 
>> assumption that the PHY is to be controlled "driver only knows how".
>
> That is all well and good as long as we can actually bind the PHY 
> device which its driver, and right now this means that we either have:
>
> - a compatible string in Device Tree which is of the form 
> ethernet-phy-id%4x.%4x (see of_get_phy_id) which means that we *know* 
> already which PHY we have and we avoid doing reads of MII_PHYSID1 and 
> MII_PHYSID2. This is a Linux implementation detail that should not 
> have to be known to systems designer IMHO
>
> - a successful read of MII_PHYSID1 and MII_PHYSID2 (or an equivalent 
> for the clause 45 PHYs) that allows us to know what PHY device we 
> have, which is something that needs to happen eventually.
>
> The problem is when there are essential resources such as clocks, 
> regulators, reset signals that must be enabled, respectively 
> de-asserted in order for a successful MDIO read of MII_PHYSID1 and 
> MII_PHYSID2 to succeed.
>
> There is no driver involvement at that stage because we have no 
> phy_device to bind it to *yet*. Depending on what we read from 
> MII_PHYSID1/PHY_ID2 we will either successfully bind to the Generic 
> PHY driver (assuming we did not read all Fs) or not and we will return 
> -ENODEV and then it is game over.
>
> This is the chicken and egg problem that this patch series is 
> addressing, for clocks, because we can retrieve clock devices with 
> just a device_node reference.

I have an impression that here the effort goes in the wrong direction. 
If I understand correctly, the goal is to have the kernel find out what 
will the driver need to use the phy. But, the kernel anyway calls a 
probe function of the driver, doesn't it? To me it looks as if you were 
trying to do something that the driver will/would/might do later, and 
possibly "undo" it in the meantime. In this regard, this becomes kind of 
a workaround, not solution of the problem. Also, having taken a glance 
at your previous messages, I can tell that this is all becoming even 
more complex.

I think that the effort should be to allow any node in the device tree 
to take care about its child nodes by itself, and just "report" to the 
kernel, or "install" in the kernel whatever is necessary, but without 
any initiative of the kernel. Let the drivers be as complicated as 
necessary, not the kernel.

Best regards,
Adam

Re: [RFC net-next 2/2] net: phy: bcm7xxx: request and manage GPHY clock

[Florian Fainelli]

On 9/3/2020 10:13 AM, Adam Rudziński wrote:
> 
> W dniu 2020-09-03 o 17:21, Florian Fainelli pisze:
>>
>>
>> On 9/2/2020 11:00 PM, Adam Rudziński wrote:
>>>
>>> W dniu 2020-09-03 o 04:13, Florian Fainelli pisze:
>>>>
>>>>
>>>> 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.
>>>
>>> Hi Guys,
>>>
>>> I've just realized that a PHY may also have a reset signal connected. 
>>> The reset signal may be controlled by the dedicated peripheral or by 
>>> GPIO.
>>
>> There is already support for such a thing within 
>> drivers/net/phy/mdio_bus.c though it assumes we could bind the PHY 
>> device to its driver already.
>>
>>>
>>> In general terms, there might be a set of control signals needed to 
>>> enable the PHY. It seems that the clock and the reset would be the 
>>> typical useful options.
>>>
>>> Going further with my imagination of how evil the hardware design 
>>> could be, in general the signals for the PHY may have some relations 
>>> to other control signals.
>>>
>>> I think that from the software point of view this comes down to 
>>> assumption that the PHY is to be controlled "driver only knows how".
>>
>> That is all well and good as long as we can actually bind the PHY 
>> device which its driver, and right now this means that we either have:
>>
>> - a compatible string in Device Tree which is of the form 
>> ethernet-phy-id%4x.%4x (see of_get_phy_id) which means that we *know* 
>> already which PHY we have and we avoid doing reads of MII_PHYSID1 and 
>> MII_PHYSID2. This is a Linux implementation detail that should not 
>> have to be known to systems designer IMHO
>>
>> - a successful read of MII_PHYSID1 and MII_PHYSID2 (or an equivalent 
>> for the clause 45 PHYs) that allows us to know what PHY device we 
>> have, which is something that needs to happen eventually.
>>
>> The problem is when there are essential resources such as clocks, 
>> regulators, reset signals that must be enabled, respectively 
>> de-asserted in order for a successful MDIO read of MII_PHYSID1 and 
>> MII_PHYSID2 to succeed.
>>
>> There is no driver involvement at that stage because we have no 
>> phy_device to bind it to *yet*. Depending on what we read from 
>> MII_PHYSID1/PHY_ID2 we will either successfully bind to the Generic 
>> PHY driver (assuming we did not read all Fs) or not and we will return 
>> -ENODEV and then it is game over.
>>
>> This is the chicken and egg problem that this patch series is 
>> addressing, for clocks, because we can retrieve clock devices with 
>> just a device_node reference.
> 
> I have an impression that here the effort goes in the wrong direction. 
> If I understand correctly, the goal is to have the kernel find out what 
> will the driver need to use the phy. But, the kernel anyway calls a 
> probe function of the driver, doesn't it? To me it looks as if you were 
> trying to do something that the driver will/would/might do later, and 
> possibly "undo" it in the meantime. In this regard, this becomes kind of 
> a workaround, not solution of the problem. Also, having taken a glance 
> at your previous messages, I can tell that this is all becoming even 
> more complex.

What is the problem according to you, and what would an acceptable 
solution look like then?

> 
> I think that the effort should be to allow any node in the device tree 
> to take care about its child nodes by itself, and just "report" to the 
> kernel, or "install" in the kernel whatever is necessary, but without 
> any initiative of the kernel. Let the drivers be as complicated as 
> necessary, not the kernel.

The Device Tree representation is already correct in that it lists 
clocks/regulators/reset signals that are needed by the PHY. The problem 
is its implementation with the Linux device driver model. Please read 
again what I wrote.
-- 
Florian

Re: [RFC net-next 2/2] net: phy: bcm7xxx: request and manage GPHY clock

[Adam Rudziński]

W dniu 2020-09-03 o 19:17, Florian Fainelli pisze:
>
>
> On 9/3/2020 10:13 AM, Adam Rudziński wrote:
>>
>> W dniu 2020-09-03 o 17:21, Florian Fainelli pisze:
>>>
>>>
>>> On 9/2/2020 11:00 PM, Adam Rudziński wrote:
>>>>
>>>> W dniu 2020-09-03 o 04:13, Florian Fainelli pisze:
>>>>>
>>>>>
>>>>> 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.
>>>>
>>>> Hi Guys,
>>>>
>>>> I've just realized that a PHY may also have a reset signal 
>>>> connected. The reset signal may be controlled by the dedicated 
>>>> peripheral or by GPIO.
>>>
>>> There is already support for such a thing within 
>>> drivers/net/phy/mdio_bus.c though it assumes we could bind the PHY 
>>> device to its driver already.
>>>
>>>>
>>>> In general terms, there might be a set of control signals needed to 
>>>> enable the PHY. It seems that the clock and the reset would be the 
>>>> typical useful options.
>>>>
>>>> Going further with my imagination of how evil the hardware design 
>>>> could be, in general the signals for the PHY may have some 
>>>> relations to other control signals.
>>>>
>>>> I think that from the software point of view this comes down to 
>>>> assumption that the PHY is to be controlled "driver only knows how".
>>>
>>> That is all well and good as long as we can actually bind the PHY 
>>> device which its driver, and right now this means that we either have:
>>>
>>> - a compatible string in Device Tree which is of the form 
>>> ethernet-phy-id%4x.%4x (see of_get_phy_id) which means that we 
>>> *know* already which PHY we have and we avoid doing reads of 
>>> MII_PHYSID1 and MII_PHYSID2. This is a Linux implementation detail 
>>> that should not have to be known to systems designer IMHO
>>>
>>> - a successful read of MII_PHYSID1 and MII_PHYSID2 (or an equivalent 
>>> for the clause 45 PHYs) that allows us to know what PHY device we 
>>> have, which is something that needs to happen eventually.
>>>
>>> The problem is when there are essential resources such as clocks, 
>>> regulators, reset signals that must be enabled, respectively 
>>> de-asserted in order for a successful MDIO read of MII_PHYSID1 and 
>>> MII_PHYSID2 to succeed.
>>>
>>> There is no driver involvement at that stage because we have no 
>>> phy_device to bind it to *yet*. Depending on what we read from 
>>> MII_PHYSID1/PHY_ID2 we will either successfully bind to the Generic 
>>> PHY driver (assuming we did not read all Fs) or not and we will 
>>> return -ENODEV and then it is game over.
>>>
>>> This is the chicken and egg problem that this patch series is 
>>> addressing, for clocks, because we can retrieve clock devices with 
>>> just a device_node reference.
>>
>> I have an impression that here the effort goes in the wrong 
>> direction. If I understand correctly, the goal is to have the kernel 
>> find out what will the driver need to use the phy. But, the kernel 
>> anyway calls a probe function of the driver, doesn't it? To me it 
>> looks as if you were trying to do something that the driver 
>> will/would/might do later, and possibly "undo" it in the meantime. In 
>> this regard, this becomes kind of a workaround, not solution of the 
>> problem. Also, having taken a glance at your previous messages, I can 
>> tell that this is all becoming even more complex.
>
> What is the problem according to you, and what would an acceptable 
> solution look like then?
>
>>
>> I think that the effort should be to allow any node in the device 
>> tree to take care about its child nodes by itself, and just "report" 
>> to the kernel, or "install" in the kernel whatever is necessary, but 
>> without any initiative of the kernel. Let the drivers be as 
>> complicated as necessary, not the kernel.
>
> The Device Tree representation is already correct in that it lists 
> clocks/regulators/reset signals that are needed by the PHY. The 
> problem is its implementation with the Linux device driver model. 
> Please read again what I wrote.

The problem which I had, was that kernel was unable to read ID of second 
PHY, because it needed to have the clock enabled, and during probing it 
still didn't have. I don't know what problems are addressed by the 
discussed patches - only the same one, or if something else too.
In my solution, of my problem, which now works well for me, instead of 
teaching kernel any new tricks, I've taught the kernel to listen to the 
driver, by adding a function allowing the driver to register its PHY on 
the MDIO bus at any time. Then, each driver instance (for a particular 
interface) configures whatever is necessary, finds its PHY when probing 
the shared MDIO bus, and tells the kernel(?) to add it to the shared 
MDIO bus. Since each one does that, when the time comes, all PHYs are 
known and all interfaces are up.
This is just an example. It doesn't need the kernel to mimic the driver. 
But, it requires a bit different structure of the device tree, and I 
guess I'm not aware of tons of reasons for which it's not "the good 
way". Sorry, I can't be more constructive here, I don't have that much 
experience with the kernel. The idea is simple: the driver does the job, 
not the kernel.
You know much better than me how Linux works, so you decide if this 
makes sense, or not, and what does this actually mean in the context of 
the system.

Concerning invalid PHY ID, all Fs are read when the MDIO bus works, but 
the PHY is inactive. Another invalid ID is all 0s. I have seen this 
value when the MDIO bus didn't have the pins assigned, so its signals 
were "trapped" in the processor. Another cause could be a physical fault 
(short?) on the bus. Maybe this case should end up in returning -ENODEV too.

Best regards,
Adam

Re: [RFC net-next 2/2] net: phy: bcm7xxx: request and manage GPHY clock

[Florian Fainelli]

On 9/3/2020 12:21 PM, Adam Rudziński wrote:
> 
> W dniu 2020-09-03 o 19:17, Florian Fainelli pisze:
>>
>>
>> On 9/3/2020 10:13 AM, Adam Rudziński wrote:
>>>
>>> W dniu 2020-09-03 o 17:21, Florian Fainelli pisze:
>>>>
>>>>
>>>> On 9/2/2020 11:00 PM, Adam Rudziński wrote:
>>>>>
>>>>> W dniu 2020-09-03 o 04:13, Florian Fainelli pisze:
>>>>>>
>>>>>>
>>>>>> 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.
>>>>>
>>>>> Hi Guys,
>>>>>
>>>>> I've just realized that a PHY may also have a reset signal 
>>>>> connected. The reset signal may be controlled by the dedicated 
>>>>> peripheral or by GPIO.
>>>>
>>>> There is already support for such a thing within 
>>>> drivers/net/phy/mdio_bus.c though it assumes we could bind the PHY 
>>>> device to its driver already.
>>>>
>>>>>
>>>>> In general terms, there might be a set of control signals needed to 
>>>>> enable the PHY. It seems that the clock and the reset would be the 
>>>>> typical useful options.
>>>>>
>>>>> Going further with my imagination of how evil the hardware design 
>>>>> could be, in general the signals for the PHY may have some 
>>>>> relations to other control signals.
>>>>>
>>>>> I think that from the software point of view this comes down to 
>>>>> assumption that the PHY is to be controlled "driver only knows how".
>>>>
>>>> That is all well and good as long as we can actually bind the PHY 
>>>> device which its driver, and right now this means that we either have:
>>>>
>>>> - a compatible string in Device Tree which is of the form 
>>>> ethernet-phy-id%4x.%4x (see of_get_phy_id) which means that we 
>>>> *know* already which PHY we have and we avoid doing reads of 
>>>> MII_PHYSID1 and MII_PHYSID2. This is a Linux implementation detail 
>>>> that should not have to be known to systems designer IMHO
>>>>
>>>> - a successful read of MII_PHYSID1 and MII_PHYSID2 (or an equivalent 
>>>> for the clause 45 PHYs) that allows us to know what PHY device we 
>>>> have, which is something that needs to happen eventually.
>>>>
>>>> The problem is when there are essential resources such as clocks, 
>>>> regulators, reset signals that must be enabled, respectively 
>>>> de-asserted in order for a successful MDIO read of MII_PHYSID1 and 
>>>> MII_PHYSID2 to succeed.
>>>>
>>>> There is no driver involvement at that stage because we have no 
>>>> phy_device to bind it to *yet*. Depending on what we read from 
>>>> MII_PHYSID1/PHY_ID2 we will either successfully bind to the Generic 
>>>> PHY driver (assuming we did not read all Fs) or not and we will 
>>>> return -ENODEV and then it is game over.
>>>>
>>>> This is the chicken and egg problem that this patch series is 
>>>> addressing, for clocks, because we can retrieve clock devices with 
>>>> just a device_node reference.
>>>
>>> I have an impression that here the effort goes in the wrong 
>>> direction. If I understand correctly, the goal is to have the kernel 
>>> find out what will the driver need to use the phy. But, the kernel 
>>> anyway calls a probe function of the driver, doesn't it? To me it 
>>> looks as if you were trying to do something that the driver 
>>> will/would/might do later, and possibly "undo" it in the meantime. In 
>>> this regard, this becomes kind of a workaround, not solution of the 
>>> problem. Also, having taken a glance at your previous messages, I can 
>>> tell that this is all becoming even more complex.
>>
>> What is the problem according to you, and what would an acceptable 
>> solution look like then?
>>
>>>
>>> I think that the effort should be to allow any node in the device 
>>> tree to take care about its child nodes by itself, and just "report" 
>>> to the kernel, or "install" in the kernel whatever is necessary, but 
>>> without any initiative of the kernel. Let the drivers be as 
>>> complicated as necessary, not the kernel.
>>
>> The Device Tree representation is already correct in that it lists 
>> clocks/regulators/reset signals that are needed by the PHY. The 
>> problem is its implementation with the Linux device driver model. 
>> Please read again what I wrote.
> 
> The problem which I had, was that kernel was unable to read ID of second 
> PHY, because it needed to have the clock enabled, and during probing it 
> still didn't have. I don't know what problems are addressed by the 
> discussed patches - only the same one, or if something else too.

That is precisely the problem being addressed here, an essential clock 
to the PHY must be enabled for the PHY to be accessible on the MDIO bus.

> In my solution, of my problem, which now works well for me, instead of 
> teaching kernel any new tricks, I've taught the kernel to listen to the 
> driver, by adding a function allowing the driver to register its PHY on 
> the MDIO bus at any time. Then, each driver instance (for a particular 
> interface) configures whatever is necessary, finds its PHY when probing 
> the shared MDIO bus, and tells the kernel(?) to add it to the shared 
> MDIO bus. Since each one does that, when the time comes, all PHYs are 
> known and all interfaces are up.

Except this is bending the Linux driver model backwards, a driver does 
not register devices, a bus does, and then it binds the driver to that 
device object. For the bus to scan a hardware device, said hardware 
device must be in a functional state to be discovered.

> This is just an example. It doesn't need the kernel to mimic the driver. 
> But, it requires a bit different structure of the device tree, and I 
> guess I'm not aware of tons of reasons for which it's not "the good 
> way". Sorry, I can't be more constructive here, I don't have that much 
> experience with the kernel. The idea is simple: the driver does the job, 
> not the kernel.
> You know much better than me how Linux works, so you decide if this 
> makes sense, or not, and what does this actually mean in the context of 
> the system.
> 
> Concerning invalid PHY ID, all Fs are read when the MDIO bus works, but 
> the PHY is inactive. Another invalid ID is all 0s. I have seen this 
> value when the MDIO bus didn't have the pins assigned, so its signals 
> were "trapped" in the processor. Another cause could be a physical fault 
> (short?) on the bus. Maybe this case should end up in returning -ENODEV 
> too.

Read failures are already treated as such, there is no need to change 
anything here.
-- 
Florian

Re: [RFC net-next 2/2] net: phy: bcm7xxx: request and manage GPHY clock

[Adam Rudziński]

W dniu 2020-09-03 o 21:35, Florian Fainelli pisze:
>
>
> On 9/3/2020 12:21 PM, Adam Rudziński wrote:
>>
>> W dniu 2020-09-03 o 19:17, Florian Fainelli pisze:
>>>
>>>
>>> On 9/3/2020 10:13 AM, Adam Rudziński wrote:
>>>>
>>>> W dniu 2020-09-03 o 17:21, Florian Fainelli pisze:
>>>>>
>>>>>
>>>>> On 9/2/2020 11:00 PM, Adam Rudziński wrote:
>>>>>>
>>>>>> W dniu 2020-09-03 o 04:13, Florian Fainelli pisze:
>>>>>>>
>>>>>>>
>>>>>>> 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.
>>>>>>
>>>>>> Hi Guys,
>>>>>>
>>>>>> I've just realized that a PHY may also have a reset signal 
>>>>>> connected. The reset signal may be controlled by the dedicated 
>>>>>> peripheral or by GPIO.
>>>>>
>>>>> There is already support for such a thing within 
>>>>> drivers/net/phy/mdio_bus.c though it assumes we could bind the PHY 
>>>>> device to its driver already.
>>>>>
>>>>>>
>>>>>> In general terms, there might be a set of control signals needed 
>>>>>> to enable the PHY. It seems that the clock and the reset would be 
>>>>>> the typical useful options.
>>>>>>
>>>>>> Going further with my imagination of how evil the hardware design 
>>>>>> could be, in general the signals for the PHY may have some 
>>>>>> relations to other control signals.
>>>>>>
>>>>>> I think that from the software point of view this comes down to 
>>>>>> assumption that the PHY is to be controlled "driver only knows how".
>>>>>
>>>>> That is all well and good as long as we can actually bind the PHY 
>>>>> device which its driver, and right now this means that we either 
>>>>> have:
>>>>>
>>>>> - a compatible string in Device Tree which is of the form 
>>>>> ethernet-phy-id%4x.%4x (see of_get_phy_id) which means that we 
>>>>> *know* already which PHY we have and we avoid doing reads of 
>>>>> MII_PHYSID1 and MII_PHYSID2. This is a Linux implementation detail 
>>>>> that should not have to be known to systems designer IMHO
>>>>>
>>>>> - a successful read of MII_PHYSID1 and MII_PHYSID2 (or an 
>>>>> equivalent for the clause 45 PHYs) that allows us to know what PHY 
>>>>> device we have, which is something that needs to happen eventually.
>>>>>
>>>>> The problem is when there are essential resources such as clocks, 
>>>>> regulators, reset signals that must be enabled, respectively 
>>>>> de-asserted in order for a successful MDIO read of MII_PHYSID1 and 
>>>>> MII_PHYSID2 to succeed.
>>>>>
>>>>> There is no driver involvement at that stage because we have no 
>>>>> phy_device to bind it to *yet*. Depending on what we read from 
>>>>> MII_PHYSID1/PHY_ID2 we will either successfully bind to the 
>>>>> Generic PHY driver (assuming we did not read all Fs) or not and we 
>>>>> will return -ENODEV and then it is game over.
>>>>>
>>>>> This is the chicken and egg problem that this patch series is 
>>>>> addressing, for clocks, because we can retrieve clock devices with 
>>>>> just a device_node reference.
>>>>
>>>> I have an impression that here the effort goes in the wrong 
>>>> direction. If I understand correctly, the goal is to have the 
>>>> kernel find out what will the driver need to use the phy. But, the 
>>>> kernel anyway calls a probe function of the driver, doesn't it? To 
>>>> me it looks as if you were trying to do something that the driver 
>>>> will/would/might do later, and possibly "undo" it in the meantime. 
>>>> In this regard, this becomes kind of a workaround, not solution of 
>>>> the problem. Also, having taken a glance at your previous messages, 
>>>> I can tell that this is all becoming even more complex.
>>>
>>> What is the problem according to you, and what would an acceptable 
>>> solution look like then?
>>>
>>>>
>>>> I think that the effort should be to allow any node in the device 
>>>> tree to take care about its child nodes by itself, and just 
>>>> "report" to the kernel, or "install" in the kernel whatever is 
>>>> necessary, but without any initiative of the kernel. Let the 
>>>> drivers be as complicated as necessary, not the kernel.
>>>
>>> The Device Tree representation is already correct in that it lists 
>>> clocks/regulators/reset signals that are needed by the PHY. The 
>>> problem is its implementation with the Linux device driver model. 
>>> Please read again what I wrote.
>>
>> The problem which I had, was that kernel was unable to read ID of 
>> second PHY, because it needed to have the clock enabled, and during 
>> probing it still didn't have. I don't know what problems are 
>> addressed by the discussed patches - only the same one, or if 
>> something else too.
>
> That is precisely the problem being addressed here, an essential clock 
> to the PHY must be enabled for the PHY to be accessible on the MDIO bus.
>
>> In my solution, of my problem, which now works well for me, instead 
>> of teaching kernel any new tricks, I've taught the kernel to listen 
>> to the driver, by adding a function allowing the driver to register 
>> its PHY on the MDIO bus at any time. Then, each driver instance (for 
>> a particular interface) configures whatever is necessary, finds its 
>> PHY when probing the shared MDIO bus, and tells the kernel(?) to add 
>> it to the shared MDIO bus. Since each one does that, when the time 
>> comes, all PHYs are known and all interfaces are up.
>
> Except this is bending the Linux driver model backwards, a driver does 
> not register devices, a bus does, and then it binds the driver to that 
> device object. For the bus to scan a hardware device, said hardware 
> device must be in a functional state to be discovered.

OK. Probably I'd better just have said that all the mentioned actions 
were a consequence of calling the driver's probe function (fec_probe() 
in the example of IMX FEC driver) and were not relying on any 
pre-configured features. My description in fact was very... imprecise.

>> This is just an example. It doesn't need the kernel to mimic the 
>> driver. But, it requires a bit different structure of the device 
>> tree, and I guess I'm not aware of tons of reasons for which it's not 
>> "the good way". Sorry, I can't be more constructive here, I don't 
>> have that much experience with the kernel. The idea is simple: the 
>> driver does the job, not the kernel.
>> You know much better than me how Linux works, so you decide if this 
>> makes sense, or not, and what does this actually mean in the context 
>> of the system.

Best regards,
Adam

Re: [RFC net-next 2/2] net: phy: bcm7xxx: request and manage GPHY clock

[Florian Fainelli]

On 9/3/2020 1:09 PM, Adam Rudziński wrote:
> 
> W dniu 2020-09-03 o 21:35, Florian Fainelli pisze:
>>
>>
>> On 9/3/2020 12:21 PM, Adam Rudziński wrote:
>>>
>>> W dniu 2020-09-03 o 19:17, Florian Fainelli pisze:
>>>>
>>>>
>>>> On 9/3/2020 10:13 AM, Adam Rudziński wrote:
>>>>>
>>>>> W dniu 2020-09-03 o 17:21, Florian Fainelli pisze:
>>>>>>
>>>>>>
>>>>>> On 9/2/2020 11:00 PM, Adam Rudziński wrote:
>>>>>>>
>>>>>>> W dniu 2020-09-03 o 04:13, Florian Fainelli pisze:
>>>>>>>>
>>>>>>>>
>>>>>>>> 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.
>>>>>>>
>>>>>>> Hi Guys,
>>>>>>>
>>>>>>> I've just realized that a PHY may also have a reset signal 
>>>>>>> connected. The reset signal may be controlled by the dedicated 
>>>>>>> peripheral or by GPIO.
>>>>>>
>>>>>> There is already support for such a thing within 
>>>>>> drivers/net/phy/mdio_bus.c though it assumes we could bind the PHY 
>>>>>> device to its driver already.
>>>>>>
>>>>>>>
>>>>>>> In general terms, there might be a set of control signals needed 
>>>>>>> to enable the PHY. It seems that the clock and the reset would be 
>>>>>>> the typical useful options.
>>>>>>>
>>>>>>> Going further with my imagination of how evil the hardware design 
>>>>>>> could be, in general the signals for the PHY may have some 
>>>>>>> relations to other control signals.
>>>>>>>
>>>>>>> I think that from the software point of view this comes down to 
>>>>>>> assumption that the PHY is to be controlled "driver only knows how".
>>>>>>
>>>>>> That is all well and good as long as we can actually bind the PHY 
>>>>>> device which its driver, and right now this means that we either 
>>>>>> have:
>>>>>>
>>>>>> - a compatible string in Device Tree which is of the form 
>>>>>> ethernet-phy-id%4x.%4x (see of_get_phy_id) which means that we 
>>>>>> *know* already which PHY we have and we avoid doing reads of 
>>>>>> MII_PHYSID1 and MII_PHYSID2. This is a Linux implementation detail 
>>>>>> that should not have to be known to systems designer IMHO
>>>>>>
>>>>>> - a successful read of MII_PHYSID1 and MII_PHYSID2 (or an 
>>>>>> equivalent for the clause 45 PHYs) that allows us to know what PHY 
>>>>>> device we have, which is something that needs to happen eventually.
>>>>>>
>>>>>> The problem is when there are essential resources such as clocks, 
>>>>>> regulators, reset signals that must be enabled, respectively 
>>>>>> de-asserted in order for a successful MDIO read of MII_PHYSID1 and 
>>>>>> MII_PHYSID2 to succeed.
>>>>>>
>>>>>> There is no driver involvement at that stage because we have no 
>>>>>> phy_device to bind it to *yet*. Depending on what we read from 
>>>>>> MII_PHYSID1/PHY_ID2 we will either successfully bind to the 
>>>>>> Generic PHY driver (assuming we did not read all Fs) or not and we 
>>>>>> will return -ENODEV and then it is game over.
>>>>>>
>>>>>> This is the chicken and egg problem that this patch series is 
>>>>>> addressing, for clocks, because we can retrieve clock devices with 
>>>>>> just a device_node reference.
>>>>>
>>>>> I have an impression that here the effort goes in the wrong 
>>>>> direction. If I understand correctly, the goal is to have the 
>>>>> kernel find out what will the driver need to use the phy. But, the 
>>>>> kernel anyway calls a probe function of the driver, doesn't it? To 
>>>>> me it looks as if you were trying to do something that the driver 
>>>>> will/would/might do later, and possibly "undo" it in the meantime. 
>>>>> In this regard, this becomes kind of a workaround, not solution of 
>>>>> the problem. Also, having taken a glance at your previous messages, 
>>>>> I can tell that this is all becoming even more complex.
>>>>
>>>> What is the problem according to you, and what would an acceptable 
>>>> solution look like then?
>>>>
>>>>>
>>>>> I think that the effort should be to allow any node in the device 
>>>>> tree to take care about its child nodes by itself, and just 
>>>>> "report" to the kernel, or "install" in the kernel whatever is 
>>>>> necessary, but without any initiative of the kernel. Let the 
>>>>> drivers be as complicated as necessary, not the kernel.
>>>>
>>>> The Device Tree representation is already correct in that it lists 
>>>> clocks/regulators/reset signals that are needed by the PHY. The 
>>>> problem is its implementation with the Linux device driver model. 
>>>> Please read again what I wrote.
>>>
>>> The problem which I had, was that kernel was unable to read ID of 
>>> second PHY, because it needed to have the clock enabled, and during 
>>> probing it still didn't have. I don't know what problems are 
>>> addressed by the discussed patches - only the same one, or if 
>>> something else too.
>>
>> That is precisely the problem being addressed here, an essential clock 
>> to the PHY must be enabled for the PHY to be accessible on the MDIO bus.
>>
>>> In my solution, of my problem, which now works well for me, instead 
>>> of teaching kernel any new tricks, I've taught the kernel to listen 
>>> to the driver, by adding a function allowing the driver to register 
>>> its PHY on the MDIO bus at any time. Then, each driver instance (for 
>>> a particular interface) configures whatever is necessary, finds its 
>>> PHY when probing the shared MDIO bus, and tells the kernel(?) to add 
>>> it to the shared MDIO bus. Since each one does that, when the time 
>>> comes, all PHYs are known and all interfaces are up.
>>
>> Except this is bending the Linux driver model backwards, a driver does 
>> not register devices, a bus does, and then it binds the driver to that 
>> device object. For the bus to scan a hardware device, said hardware 
>> device must be in a functional state to be discovered.
> 
> OK. Probably I'd better just have said that all the mentioned actions 
> were a consequence of calling the driver's probe function (fec_probe() 
> in the example of IMX FEC driver) and were not relying on any 
> pre-configured features. My description in fact was very... imprecise.

Yes it was unfortunately, and it is still a complete layering violation, 
the FEC should provide a MDIO bus instance with read/write operations, 
and that is all that should be needed. There should be no poking into 
its Device Tree nodes as much as possible.
-- 
Florian