[PATCH net-next 0/4] net: phy: move getting (R)MII refclock to phylib

[PATCH net-next 0/4] net: phy: move getting (R)MII refclock to phylib

[Heiner Kallweit]

>From c578be6534254bfc3fd627d9d7be07b1bb46f92c Mon Sep 17 00:00:00 2001
Few PHY drivers (smsc, bcm7xxx, micrel) get and enable the (R)MII
reference clock in their probe() callback. Move this common
functionality to phylib, this allows to remove it from drivers.

Heiner Kallweit (4):
  net: phylib: add getting reference clock
  net: phy: smsc: remove getting reference clock
  net: phy: micrel: remove getting reference clock
  net: phy: bcm7xxx: remove getting reference clock

 drivers/net/phy/bcm7xxx.c    | 20 --------------------
 drivers/net/phy/micrel.c     |  7 ++-----
 drivers/net/phy/phy_device.c |  6 ++++++
 drivers/net/phy/smsc.c       |  9 +--------
 include/linux/phy.h          |  5 +++++
 5 files changed, 14 insertions(+), 33 deletions(-)

-- 
2.40.0

[PATCH net-next 1/4] net: phylib: add getting reference clock

[Heiner Kallweit]

Few PHY drivers (smsc, bcm7xxx, micrel) get and enable the (R)MII
reference clock in their probe() callback. Move this common
functionality to phylib, this allows to remove it from the drivers
in a follow-up.

Note that we now enable the reference clock before deasserting the
PHY reset signal. Maybe this even allows us to get rid of
phy_reset_after_clk_enable().

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/phy/phy_device.c | 6 ++++++
 include/linux/phy.h          | 5 +++++
 2 files changed, 11 insertions(+)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index c0760cbf5..6668487e2 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -3096,6 +3096,12 @@ static int phy_probe(struct device *dev)
 	if (phydrv->flags & PHY_IS_INTERNAL)
 		phydev->is_internal = true;
 
+	phydev->refclk = devm_clk_get_optional_enabled(dev, NULL);
+	if (IS_ERR(phydev->refclk)) {
+		err = PTR_ERR(phydev->refclk);
+		goto out;
+	}
+
 	/* Deassert the reset signal */
 	phy_device_reset(phydev, 0);
 
diff --git a/include/linux/phy.h b/include/linux/phy.h
index fefd5091b..6d6129674 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -11,6 +11,7 @@
 #ifndef __PHY_H
 #define __PHY_H
 
+#include <linux/clk.h>
 #include <linux/compiler.h>
 #include <linux/spinlock.h>
 #include <linux/ethtool.h>
@@ -595,6 +596,7 @@ struct macsec_ops;
  * @interface: enum phy_interface_t value
  * @skb: Netlink message for cable diagnostics
  * @nest: Netlink nest used for cable diagnostics
+ * @refclk: External (R)MII reference clock
  * @ehdr: nNtlink header for cable diagnostics
  * @phy_led_triggers: Array of LED triggers
  * @phy_num_led_triggers: Number of triggers in @phy_led_triggers
@@ -719,6 +721,9 @@ struct phy_device {
 	void *ehdr;
 	struct nlattr *nest;
 
+	/* external (R)MII reference clock */
+	struct clk *refclk;
+
 	/* Interrupt and Polling infrastructure */
 	struct delayed_work state_queue;
 
-- 
2.40.0

Re: [PATCH net-next 1/4] net: phylib: add getting reference clock

[Florian Fainelli]

On 3/24/23 11:03, Heiner Kallweit wrote:
> Few PHY drivers (smsc, bcm7xxx, micrel) get and enable the (R)MII
> reference clock in their probe() callback. Move this common
> functionality to phylib, this allows to remove it from the drivers
> in a follow-up.
> 
> Note that we now enable the reference clock before deasserting the
> PHY reset signal. Maybe this even allows us to get rid of
> phy_reset_after_clk_enable().
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>   drivers/net/phy/phy_device.c | 6 ++++++
>   include/linux/phy.h          | 5 +++++
>   2 files changed, 11 insertions(+)
> 
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index c0760cbf5..6668487e2 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -3096,6 +3096,12 @@ static int phy_probe(struct device *dev)
>   	if (phydrv->flags & PHY_IS_INTERNAL)
>   		phydev->is_internal = true;
>   
> +	phydev->refclk = devm_clk_get_optional_enabled(dev, NULL);
> +	if (IS_ERR(phydev->refclk)) {
> +		err = PTR_ERR(phydev->refclk);
> +		goto out;
> +	}

My comment in patch 2 should have been there, I would add a flag that 
the PHY driver can set that tells the core that it is OK to fetch the 
clock. In the case of bcm7xxx.c is it not the reference clock, so while 
we can use phydev->refclk for the same purpose, it could be a tad confusing.
-- 
Florian

[PATCH net-next 2/4] net: phy: smsc: remove getting reference clock

[Heiner Kallweit]

Now that getting the reference clock has been moved to phylib,
we can remove it here.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/phy/smsc.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/drivers/net/phy/smsc.c b/drivers/net/phy/smsc.c
index 730964b85..48654c684 100644
--- a/drivers/net/phy/smsc.c
+++ b/drivers/net/phy/smsc.c
@@ -278,7 +278,6 @@ int smsc_phy_probe(struct phy_device *phydev)
 {
 	struct device *dev = &phydev->mdio.dev;
 	struct smsc_phy_priv *priv;
-	struct clk *refclk;
 
 	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
@@ -291,13 +290,7 @@ int smsc_phy_probe(struct phy_device *phydev)
 
 	phydev->priv = priv;
 
-	/* Make clk optional to keep DTB backward compatibility. */
-	refclk = devm_clk_get_optional_enabled(dev, NULL);
-	if (IS_ERR(refclk))
-		return dev_err_probe(dev, PTR_ERR(refclk),
-				     "Failed to request clock\n");
-
-	return clk_set_rate(refclk, 50 * 1000 * 1000);
+	return clk_set_rate(phydev->refclk, 50 * 1000 * 1000);
 }
 EXPORT_SYMBOL_GPL(smsc_phy_probe);
 
-- 
2.40.0

Re: [PATCH net-next 2/4] net: phy: smsc: remove getting reference clock

[Florian Fainelli]

On 3/24/23 11:03, Heiner Kallweit wrote:
> Now that getting the reference clock has been moved to phylib,
> we can remove it here.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>   drivers/net/phy/smsc.c | 9 +--------
>   1 file changed, 1 insertion(+), 8 deletions(-)
> 
> diff --git a/drivers/net/phy/smsc.c b/drivers/net/phy/smsc.c
> index 730964b85..48654c684 100644
> --- a/drivers/net/phy/smsc.c
> +++ b/drivers/net/phy/smsc.c
> @@ -278,7 +278,6 @@ int smsc_phy_probe(struct phy_device *phydev)
>   {
>   	struct device *dev = &phydev->mdio.dev;
>   	struct smsc_phy_priv *priv;
> -	struct clk *refclk;
>   
>   	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>   	if (!priv)
> @@ -291,13 +290,7 @@ int smsc_phy_probe(struct phy_device *phydev)
>   
>   	phydev->priv = priv;
>   
> -	/* Make clk optional to keep DTB backward compatibility. */
> -	refclk = devm_clk_get_optional_enabled(dev, NULL);
> -	if (IS_ERR(refclk))
> -		return dev_err_probe(dev, PTR_ERR(refclk),
> -				     "Failed to request clock\n");
> -
> -	return clk_set_rate(refclk, 50 * 1000 * 1000);
> +	return clk_set_rate(phydev->refclk, 50 * 1000 * 1000);

AFAIR one should be calling clk_prepare_enable() before clk_set_rate(), 
which neither smsc.c nor micrel.c do.

If we insist on moving this code to the PHY library which I have no 
strong objections against, we might provide a PHY_REQUIRES_REFCLK flag 
that the generic code can key off, or in the same of bcm7xx.c: 
PHY_LET_ME_MANAGED_MY_CLOCK?
-- 
Florian

Re: [PATCH net-next 2/4] net: phy: smsc: remove getting reference clock

[Florian Fainelli]

On 3/24/23 12:07, Florian Fainelli wrote:
> On 3/24/23 11:03, Heiner Kallweit wrote:
>> Now that getting the reference clock has been moved to phylib,
>> we can remove it here.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>>   drivers/net/phy/smsc.c | 9 +--------
>>   1 file changed, 1 insertion(+), 8 deletions(-)
>>
>> diff --git a/drivers/net/phy/smsc.c b/drivers/net/phy/smsc.c
>> index 730964b85..48654c684 100644
>> --- a/drivers/net/phy/smsc.c
>> +++ b/drivers/net/phy/smsc.c
>> @@ -278,7 +278,6 @@ int smsc_phy_probe(struct phy_device *phydev)
>>   {
>>       struct device *dev = &phydev->mdio.dev;
>>       struct smsc_phy_priv *priv;
>> -    struct clk *refclk;
>>       priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>>       if (!priv)
>> @@ -291,13 +290,7 @@ int smsc_phy_probe(struct phy_device *phydev)
>>       phydev->priv = priv;
>> -    /* Make clk optional to keep DTB backward compatibility. */
>> -    refclk = devm_clk_get_optional_enabled(dev, NULL);
>> -    if (IS_ERR(refclk))
>> -        return dev_err_probe(dev, PTR_ERR(refclk),
>> -                     "Failed to request clock\n");
>> -
>> -    return clk_set_rate(refclk, 50 * 1000 * 1000);
>> +    return clk_set_rate(phydev->refclk, 50 * 1000 * 1000);
> 
> AFAIR one should be calling clk_prepare_enable() before clk_set_rate(), 
> which neither smsc.c nor micrel.c do.

Which is implied by the devm_clk_get_optional_enabled().
-- 
Florian

Re: [PATCH net-next 2/4] net: phy: smsc: remove getting reference clock

[Florian Fainelli]

On 3/24/23 11:03, Heiner Kallweit wrote:
> Now that getting the reference clock has been moved to phylib,
> we can remove it here.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>   drivers/net/phy/smsc.c | 9 +--------
>   1 file changed, 1 insertion(+), 8 deletions(-)
> 
> diff --git a/drivers/net/phy/smsc.c b/drivers/net/phy/smsc.c
> index 730964b85..48654c684 100644
> --- a/drivers/net/phy/smsc.c
> +++ b/drivers/net/phy/smsc.c
> @@ -278,7 +278,6 @@ int smsc_phy_probe(struct phy_device *phydev)
>   {
>   	struct device *dev = &phydev->mdio.dev;
>   	struct smsc_phy_priv *priv;
> -	struct clk *refclk;
>   
>   	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>   	if (!priv)
> @@ -291,13 +290,7 @@ int smsc_phy_probe(struct phy_device *phydev)
>   
>   	phydev->priv = priv;
>   
> -	/* Make clk optional to keep DTB backward compatibility. */
> -	refclk = devm_clk_get_optional_enabled(dev, NULL);
> -	if (IS_ERR(refclk))
> -		return dev_err_probe(dev, PTR_ERR(refclk),
> -				     "Failed to request clock\n");
> -
> -	return clk_set_rate(refclk, 50 * 1000 * 1000);
> +	return clk_set_rate(phydev->refclk, 50 * 1000 * 1000);

AFAIR one should be calling clk_prepare_enable() before clk_set_rate(), 
which neither smsc.c nor micrel.c do.

If we insist on moving this code to the PHY library which I have no 
strong objections against, we might provide a PHY_REQUIRES_REFCLK flag 
that the generic code can key off, or in the same of bcm7xx.c: 
PHY_LET_ME_MANAGED_MY_CLOCK?
-- 
Florian

[PATCH net-next 3/4] net: phy: micrel: remove getting reference clock

[Heiner Kallweit]

Now that getting the reference clock has been moved to phylib, we can
remove it here.

Only one clock is supported by the PHY, therefore it's ok that we now use
the first clock instead of the named one.

Note that currently devm_clk_get is used instead of devm_clk_get_optional,
but nevertheless the clock is treated as optional.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/phy/micrel.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
index e26c6723c..dfd2c1d0f 100644
--- a/drivers/net/phy/micrel.c
+++ b/drivers/net/phy/micrel.c
@@ -1884,7 +1884,6 @@ static int kszphy_probe(struct phy_device *phydev)
 	const struct kszphy_type *type = phydev->drv->driver_data;
 	const struct device_node *np = phydev->mdio.dev.of_node;
 	struct kszphy_priv *priv;
-	struct clk *clk;
 
 	priv = devm_kzalloc(&phydev->mdio.dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
@@ -1896,10 +1895,8 @@ static int kszphy_probe(struct phy_device *phydev)
 
 	kszphy_parse_led_mode(phydev);
 
-	clk = devm_clk_get(&phydev->mdio.dev, "rmii-ref");
-	/* NOTE: clk may be NULL if building without CONFIG_HAVE_CLK */
-	if (!IS_ERR_OR_NULL(clk)) {
-		unsigned long rate = clk_get_rate(clk);
+	if (phydev->refclk) {
+		unsigned long rate = clk_get_rate(phydev->refclk);
 		bool rmii_ref_clk_sel_25_mhz;
 
 		if (type)
-- 
2.40.0

[PATCH net-next 4/4] net: phy: bcm7xxx: remove getting reference clock

[Heiner Kallweit]

Now that getting the reference clock has been moved to phylib,
we can remove it here.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/phy/bcm7xxx.c | 20 --------------------
 1 file changed, 20 deletions(-)

diff --git a/drivers/net/phy/bcm7xxx.c b/drivers/net/phy/bcm7xxx.c
index 75593e7d1..c608e0439 100644
--- a/drivers/net/phy/bcm7xxx.c
+++ b/drivers/net/phy/bcm7xxx.c
@@ -11,7 +11,6 @@
 #include "bcm-phy-lib.h"
 #include <linux/bitops.h>
 #include <linux/brcmphy.h>
-#include <linux/clk.h>
 #include <linux/mdio.h>
 
 /* Broadcom BCM7xxx internal PHY registers */
@@ -45,7 +44,6 @@
 
 struct bcm7xxx_phy_priv {
 	u64	*stats;
-	struct clk *clk;
 };
 
 static int bcm7xxx_28nm_d0_afe_config_init(struct phy_device *phydev)
@@ -825,14 +823,6 @@ static int bcm7xxx_28nm_probe(struct phy_device *phydev)
 	if (!priv->stats)
 		return -ENOMEM;
 
-	priv->clk = devm_clk_get_optional(&phydev->mdio.dev, NULL);
-	if (IS_ERR(priv->clk))
-		return PTR_ERR(priv->clk);
-
-	ret = clk_prepare_enable(priv->clk);
-	if (ret)
-		return ret;
-
 	/* Dummy read to a register to workaround an issue upon reset where the
 	 * internal inverter may not allow the first MDIO transaction to pass
 	 * the MDIO management controller and make us return 0xffff for such
@@ -844,13 +834,6 @@ static int bcm7xxx_28nm_probe(struct phy_device *phydev)
 	return ret;
 }
 
-static void bcm7xxx_28nm_remove(struct phy_device *phydev)
-{
-	struct bcm7xxx_phy_priv *priv = phydev->priv;
-
-	clk_disable_unprepare(priv->clk);
-}
-
 #define BCM7XXX_28NM_GPHY(_oui, _name)					\
 {									\
 	.phy_id		= (_oui),					\
@@ -866,7 +849,6 @@ static void bcm7xxx_28nm_remove(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)					\
@@ -882,7 +864,6 @@ static void bcm7xxx_28nm_remove(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,				\
 	.read_mmd	= bcm7xxx_28nm_ephy_read_mmd,			\
 	.write_mmd	= bcm7xxx_28nm_ephy_write_mmd,			\
 }
@@ -908,7 +889,6 @@ static void bcm7xxx_28nm_remove(struct phy_device *phydev)
 	/* PHY_BASIC_FEATURES */					\
 	.flags		= PHY_IS_INTERNAL,				\
 	.probe		= bcm7xxx_28nm_probe,				\
-	.remove		= bcm7xxx_28nm_remove,				\
 	.config_init	= bcm7xxx_16nm_ephy_config_init,		\
 	.config_aneg	= genphy_config_aneg,				\
 	.read_status	= genphy_read_status,				\
-- 
2.40.0

Re: [PATCH net-next 4/4] net: phy: bcm7xxx: remove getting reference clock

[Florian Fainelli]

On 3/24/23 11:05, Heiner Kallweit wrote:
> Now that getting the reference clock has been moved to phylib,
> we can remove it here.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

This is not the reference clock for bcm7xxx this is the SoC internal 
clock that feeds the Soc internal PHY.
-- 
Florian

Re: [PATCH net-next 4/4] net: phy: bcm7xxx: remove getting reference clock

[Heiner Kallweit]

On 24.03.2023 20:03, Florian Fainelli wrote:
> On 3/24/23 11:05, Heiner Kallweit wrote:
>> Now that getting the reference clock has been moved to phylib,
>> we can remove it here.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> 
> This is not the reference clock for bcm7xxx this is the SoC internal clock that feeds the Soc internal PHY.

Ah, good to know. Then indeed we may have to allow drivers to disable this feature.

Another aspect: When looking at ba4ee3c05365 "net: phy: bcm7xxx: request and manage GPHY clock"
I stumbled across statement "PHY driver can be probed with the clocks turned off".
I interpret this in a way that dynamic PHY detection doesn't work because the PHY ID
registers aren't accessible before the PHY driver has been loaded and probed. Is this right?
Should the MDIO bus driver enable the clock for the PHY?

Re: [PATCH net-next 4/4] net: phy: bcm7xxx: remove getting reference clock

[Florian Fainelli]

On 3/24/23 12:50, Heiner Kallweit wrote:
> On 24.03.2023 20:03, Florian Fainelli wrote:
>> On 3/24/23 11:05, Heiner Kallweit wrote:
>>> Now that getting the reference clock has been moved to phylib,
>>> we can remove it here.
>>>
>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>>
>> This is not the reference clock for bcm7xxx this is the SoC internal clock that feeds the Soc internal PHY.
> 
> Ah, good to know. Then indeed we may have to allow drivers to disable this feature.
> 
> Another aspect: When looking at ba4ee3c05365 "net: phy: bcm7xxx: request and manage GPHY clock"
> I stumbled across statement "PHY driver can be probed with the clocks turned off".
> I interpret this in a way that dynamic PHY detection doesn't work because the PHY ID
> registers aren't accessible before the PHY driver has been loaded and probed. Is this right?

Yes this is correct we actually probe with the clock turned off as we 
try to run as low power as possible upon boot.

> Should the MDIO bus driver enable the clock for the PHY?
> 

This is what I had done in our downstream MDIO bus driver initially and 
this was fine because we were guaranteed to use a specific MDIO bus 
driver since the PHY is integrated.

Eventually when this landed upstream I went with specifying the Ethernet 
PHY compatible with the "ethernet-phy-idAAAA.BBBB" notation which forces 
the PHY library to match the compatible with the driver directly without 
requiring to read from MII_PHYSID1/2.

The problems I saw with the MDIO bus approach was that:

- you would have to enable the clock prior to scanning the bus which 
could be done in mii_bus::reset for a driver specific way of doing it, 
or directly in mdiobus_scan() and then you would have to balance the 
clock enable count within the PHY driver's probe function which required 
using __clk_is_enabled() to ensure the clock could be disabled later on 
when you unbind the PHY device from its driver or during remove, or 
suspend/resume

- if the PHY device tree node specified multiple clocks, you would not 
necessarily know which one(s) to enable and which one(s) not to. 
Enabling all of them would be a waste of power and could also possibly 
create sequencing issues if we have a situation similar to the reference 
clock you are trying to address. Not enabling any would obviously not 
work at all.

Using the "ethernet-phy-idAAAA.BBBB" ensured that the PHY driver could 
enable the clock(s) it needs and ensure that probe() and remove() would 
have balanced clock enable/disable calls.
-- 
Florian

Re: [PATCH net-next 4/4] net: phy: bcm7xxx: remove getting reference clock

[Heiner Kallweit]

On 24.03.2023 21:11, Florian Fainelli wrote:
> On 3/24/23 12:50, Heiner Kallweit wrote:
>> On 24.03.2023 20:03, Florian Fainelli wrote:
>>> On 3/24/23 11:05, Heiner Kallweit wrote:
>>>> Now that getting the reference clock has been moved to phylib,
>>>> we can remove it here.
>>>>
>>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>>>
>>> This is not the reference clock for bcm7xxx this is the SoC internal clock that feeds the Soc internal PHY.
>>
>> Ah, good to know. Then indeed we may have to allow drivers to disable this feature.
>>
>> Another aspect: When looking at ba4ee3c05365 "net: phy: bcm7xxx: request and manage GPHY clock"
>> I stumbled across statement "PHY driver can be probed with the clocks turned off".
>> I interpret this in a way that dynamic PHY detection doesn't work because the PHY ID
>> registers aren't accessible before the PHY driver has been loaded and probed. Is this right?
> 
> Yes this is correct we actually probe with the clock turned off as we try to run as low power as possible upon boot.
> 
>> Should the MDIO bus driver enable the clock for the PHY?
>>
> 
> This is what I had done in our downstream MDIO bus driver initially and this was fine because we were guaranteed to use a specific MDIO bus driver since the PHY is integrated.
> 
> Eventually when this landed upstream I went with specifying the Ethernet PHY compatible with the "ethernet-phy-idAAAA.BBBB" notation which forces the PHY library to match the compatible with the driver directly without requiring to read from MII_PHYSID1/2.
> 
> The problems I saw with the MDIO bus approach was that:
> 
> - you would have to enable the clock prior to scanning the bus which could be done in mii_bus::reset for a driver specific way of doing it, or directly in mdiobus_scan() and then you would have to balance the clock enable count within the PHY driver's probe function which required using __clk_is_enabled() to ensure the clock could be disabled later on when you unbind the PHY device from its driver or during remove, or suspend/resume
> 
> - if the PHY device tree node specified multiple clocks, you would not necessarily know which one(s) to enable and which one(s) not to. Enabling all of them would be a waste of power and could also possibly create sequencing issues if we have a situation similar to the reference clock you are trying to address. Not enabling any would obviously not work at all.
> 
> Using the "ethernet-phy-idAAAA.BBBB" ensured that the PHY driver could enable the clock(s) it needs and ensure that probe() and remove() would have balanced clock enable/disable calls.

I see, thanks for the comprehensive explanation. If we need an additional
PHY driver flag or other measures, then I wonder whether it's worth it
to add refclock handling to phylib for two drivers. Maybe not.

Re: [PATCH net-next 4/4] net: phy: bcm7xxx: remove getting reference clock

[Florian Fainelli]

On 3/24/23 14:16, Heiner Kallweit wrote:
> On 24.03.2023 21:11, Florian Fainelli wrote:
>> On 3/24/23 12:50, Heiner Kallweit wrote:
>>> On 24.03.2023 20:03, Florian Fainelli wrote:
>>>> On 3/24/23 11:05, Heiner Kallweit wrote:
>>>>> Now that getting the reference clock has been moved to phylib,
>>>>> we can remove it here.
>>>>>
>>>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>>>>
>>>> This is not the reference clock for bcm7xxx this is the SoC internal clock that feeds the Soc internal PHY.
>>>
>>> Ah, good to know. Then indeed we may have to allow drivers to disable this feature.
>>>
>>> Another aspect: When looking at ba4ee3c05365 "net: phy: bcm7xxx: request and manage GPHY clock"
>>> I stumbled across statement "PHY driver can be probed with the clocks turned off".
>>> I interpret this in a way that dynamic PHY detection doesn't work because the PHY ID
>>> registers aren't accessible before the PHY driver has been loaded and probed. Is this right?
>>
>> Yes this is correct we actually probe with the clock turned off as we try to run as low power as possible upon boot.
>>
>>> Should the MDIO bus driver enable the clock for the PHY?
>>>
>>
>> This is what I had done in our downstream MDIO bus driver initially and this was fine because we were guaranteed to use a specific MDIO bus driver since the PHY is integrated.
>>
>> Eventually when this landed upstream I went with specifying the Ethernet PHY compatible with the "ethernet-phy-idAAAA.BBBB" notation which forces the PHY library to match the compatible with the driver directly without requiring to read from MII_PHYSID1/2.
>>
>> The problems I saw with the MDIO bus approach was that:
>>
>> - you would have to enable the clock prior to scanning the bus which could be done in mii_bus::reset for a driver specific way of doing it, or directly in mdiobus_scan() and then you would have to balance the clock enable count within the PHY driver's probe function which required using __clk_is_enabled() to ensure the clock could be disabled later on when you unbind the PHY device from its driver or during remove, or suspend/resume
>>
>> - if the PHY device tree node specified multiple clocks, you would not necessarily know which one(s) to enable and which one(s) not to. Enabling all of them would be a waste of power and could also possibly create sequencing issues if we have a situation similar to the reference clock you are trying to address. Not enabling any would obviously not work at all.
>>
>> Using the "ethernet-phy-idAAAA.BBBB" ensured that the PHY driver could enable the clock(s) it needs and ensure that probe() and remove() would have balanced clock enable/disable calls.
> 
> I see, thanks for the comprehensive explanation. If we need an additional
> PHY driver flag or other measures, then I wonder whether it's worth it
> to add refclock handling to phylib for two drivers. Maybe not.

Agreed that two drivers may not be that many. Situations like stmmac or 
other drivers where there may be a need for the PHY clock to run for the 
MAC's RX path to complete initializing might be solved using existing 
PHY library routines.
-- 
Florian