* [PATCH net-next 0/4] net: phy: move getting (R)MII refclock to phylib
@ 2023-03-24 18:01 Heiner Kallweit
2023-03-24 18:03 ` [PATCH net-next 1/4] net: phylib: add getting reference clock Heiner Kallweit
` (3 more replies)
0 siblings, 4 replies; 14+ messages in thread
From: Heiner Kallweit @ 2023-03-24 18:01 UTC (permalink / raw)
To: Andrew Lunn, Russell King - ARM Linux, Jakub Kicinski,
David Miller, Paolo Abeni, Eric Dumazet, Florian Fainelli,
Broadcom internal kernel review list
Cc: netdev
>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
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH net-next 1/4] net: phylib: add getting reference clock
2023-03-24 18:01 [PATCH net-next 0/4] net: phy: move getting (R)MII refclock to phylib Heiner Kallweit
@ 2023-03-24 18:03 ` Heiner Kallweit
2023-03-24 19:10 ` Florian Fainelli
2023-03-24 18:03 ` [PATCH net-next 2/4] net: phy: smsc: remove " Heiner Kallweit
` (2 subsequent siblings)
3 siblings, 1 reply; 14+ messages in thread
From: Heiner Kallweit @ 2023-03-24 18:03 UTC (permalink / raw)
To: Andrew Lunn, Russell King - ARM Linux, Jakub Kicinski,
David Miller, Paolo Abeni, Eric Dumazet, Florian Fainelli,
Broadcom internal kernel review list
Cc: netdev
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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH net-next 2/4] net: phy: smsc: remove getting reference clock
2023-03-24 18:01 [PATCH net-next 0/4] net: phy: move getting (R)MII refclock to phylib Heiner Kallweit
2023-03-24 18:03 ` [PATCH net-next 1/4] net: phylib: add getting reference clock Heiner Kallweit
@ 2023-03-24 18:03 ` Heiner Kallweit
2023-03-24 19:07 ` Florian Fainelli
2023-03-24 19:07 ` Florian Fainelli
2023-03-24 18:04 ` [PATCH net-next 3/4] net: phy: micrel: " Heiner Kallweit
2023-03-24 18:05 ` [PATCH net-next 4/4] net: phy: bcm7xxx: " Heiner Kallweit
3 siblings, 2 replies; 14+ messages in thread
From: Heiner Kallweit @ 2023-03-24 18:03 UTC (permalink / raw)
To: Andrew Lunn, Russell King - ARM Linux, Jakub Kicinski,
David Miller, Paolo Abeni, Eric Dumazet, Florian Fainelli,
Broadcom internal kernel review list
Cc: netdev
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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH net-next 3/4] net: phy: micrel: remove getting reference clock
2023-03-24 18:01 [PATCH net-next 0/4] net: phy: move getting (R)MII refclock to phylib Heiner Kallweit
2023-03-24 18:03 ` [PATCH net-next 1/4] net: phylib: add getting reference clock Heiner Kallweit
2023-03-24 18:03 ` [PATCH net-next 2/4] net: phy: smsc: remove " Heiner Kallweit
@ 2023-03-24 18:04 ` Heiner Kallweit
2023-03-24 18:05 ` [PATCH net-next 4/4] net: phy: bcm7xxx: " Heiner Kallweit
3 siblings, 0 replies; 14+ messages in thread
From: Heiner Kallweit @ 2023-03-24 18:04 UTC (permalink / raw)
To: Andrew Lunn, Russell King - ARM Linux, Jakub Kicinski,
David Miller, Paolo Abeni, Eric Dumazet, Florian Fainelli,
Broadcom internal kernel review list
Cc: netdev
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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH net-next 4/4] net: phy: bcm7xxx: remove getting reference clock
2023-03-24 18:01 [PATCH net-next 0/4] net: phy: move getting (R)MII refclock to phylib Heiner Kallweit
` (2 preceding siblings ...)
2023-03-24 18:04 ` [PATCH net-next 3/4] net: phy: micrel: " Heiner Kallweit
@ 2023-03-24 18:05 ` Heiner Kallweit
2023-03-24 19:03 ` Florian Fainelli
3 siblings, 1 reply; 14+ messages in thread
From: Heiner Kallweit @ 2023-03-24 18:05 UTC (permalink / raw)
To: Andrew Lunn, Russell King - ARM Linux, Jakub Kicinski,
David Miller, Paolo Abeni, Eric Dumazet, Florian Fainelli,
Broadcom internal kernel review list
Cc: netdev
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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH net-next 4/4] net: phy: bcm7xxx: remove getting reference clock
2023-03-24 18:05 ` [PATCH net-next 4/4] net: phy: bcm7xxx: " Heiner Kallweit
@ 2023-03-24 19:03 ` Florian Fainelli
2023-03-24 19:50 ` Heiner Kallweit
0 siblings, 1 reply; 14+ messages in thread
From: Florian Fainelli @ 2023-03-24 19:03 UTC (permalink / raw)
To: Heiner Kallweit, Andrew Lunn, Russell King - ARM Linux,
Jakub Kicinski, David Miller, Paolo Abeni, Eric Dumazet,
Florian Fainelli, Broadcom internal kernel review list
Cc: netdev
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net-next 2/4] net: phy: smsc: remove getting reference clock
2023-03-24 18:03 ` [PATCH net-next 2/4] net: phy: smsc: remove " Heiner Kallweit
@ 2023-03-24 19:07 ` Florian Fainelli
2023-03-24 19:07 ` Florian Fainelli
1 sibling, 0 replies; 14+ messages in thread
From: Florian Fainelli @ 2023-03-24 19:07 UTC (permalink / raw)
To: Heiner Kallweit, Andrew Lunn, Russell King - ARM Linux,
Jakub Kicinski, David Miller, Paolo Abeni, Eric Dumazet,
Florian Fainelli, Broadcom internal kernel review list
Cc: netdev
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net-next 2/4] net: phy: smsc: remove getting reference clock
2023-03-24 18:03 ` [PATCH net-next 2/4] net: phy: smsc: remove " Heiner Kallweit
2023-03-24 19:07 ` Florian Fainelli
@ 2023-03-24 19:07 ` Florian Fainelli
2023-03-24 19:09 ` Florian Fainelli
1 sibling, 1 reply; 14+ messages in thread
From: Florian Fainelli @ 2023-03-24 19:07 UTC (permalink / raw)
To: Heiner Kallweit, Andrew Lunn, Russell King - ARM Linux,
Jakub Kicinski, David Miller, Paolo Abeni, Eric Dumazet,
Florian Fainelli, Broadcom internal kernel review list
Cc: netdev
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net-next 2/4] net: phy: smsc: remove getting reference clock
2023-03-24 19:07 ` Florian Fainelli
@ 2023-03-24 19:09 ` Florian Fainelli
0 siblings, 0 replies; 14+ messages in thread
From: Florian Fainelli @ 2023-03-24 19:09 UTC (permalink / raw)
To: Florian Fainelli, Heiner Kallweit, Andrew Lunn,
Russell King - ARM Linux, Jakub Kicinski, David Miller,
Paolo Abeni, Eric Dumazet, Broadcom internal kernel review list
Cc: netdev
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net-next 1/4] net: phylib: add getting reference clock
2023-03-24 18:03 ` [PATCH net-next 1/4] net: phylib: add getting reference clock Heiner Kallweit
@ 2023-03-24 19:10 ` Florian Fainelli
0 siblings, 0 replies; 14+ messages in thread
From: Florian Fainelli @ 2023-03-24 19:10 UTC (permalink / raw)
To: Heiner Kallweit, Andrew Lunn, Russell King - ARM Linux,
Jakub Kicinski, David Miller, Paolo Abeni, Eric Dumazet,
Florian Fainelli, Broadcom internal kernel review list
Cc: netdev
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net-next 4/4] net: phy: bcm7xxx: remove getting reference clock
2023-03-24 19:03 ` Florian Fainelli
@ 2023-03-24 19:50 ` Heiner Kallweit
2023-03-24 20:11 ` Florian Fainelli
0 siblings, 1 reply; 14+ messages in thread
From: Heiner Kallweit @ 2023-03-24 19:50 UTC (permalink / raw)
To: Florian Fainelli, Andrew Lunn, Russell King - ARM Linux,
Jakub Kicinski, David Miller, Paolo Abeni, Eric Dumazet,
Broadcom internal kernel review list
Cc: netdev
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?
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net-next 4/4] net: phy: bcm7xxx: remove getting reference clock
2023-03-24 19:50 ` Heiner Kallweit
@ 2023-03-24 20:11 ` Florian Fainelli
2023-03-24 21:16 ` Heiner Kallweit
0 siblings, 1 reply; 14+ messages in thread
From: Florian Fainelli @ 2023-03-24 20:11 UTC (permalink / raw)
To: Heiner Kallweit, Florian Fainelli, Andrew Lunn,
Russell King - ARM Linux, Jakub Kicinski, David Miller,
Paolo Abeni, Eric Dumazet, Broadcom internal kernel review list
Cc: netdev
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net-next 4/4] net: phy: bcm7xxx: remove getting reference clock
2023-03-24 20:11 ` Florian Fainelli
@ 2023-03-24 21:16 ` Heiner Kallweit
2023-03-24 21:19 ` Florian Fainelli
0 siblings, 1 reply; 14+ messages in thread
From: Heiner Kallweit @ 2023-03-24 21:16 UTC (permalink / raw)
To: Florian Fainelli, Andrew Lunn, Russell King - ARM Linux,
Jakub Kicinski, David Miller, Paolo Abeni, Eric Dumazet,
Broadcom internal kernel review list
Cc: netdev
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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net-next 4/4] net: phy: bcm7xxx: remove getting reference clock
2023-03-24 21:16 ` Heiner Kallweit
@ 2023-03-24 21:19 ` Florian Fainelli
0 siblings, 0 replies; 14+ messages in thread
From: Florian Fainelli @ 2023-03-24 21:19 UTC (permalink / raw)
To: Heiner Kallweit, Florian Fainelli, Andrew Lunn,
Russell King - ARM Linux, Jakub Kicinski, David Miller,
Paolo Abeni, Eric Dumazet, Broadcom internal kernel review list
Cc: netdev
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
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2023-03-24 21:19 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-03-24 18:01 [PATCH net-next 0/4] net: phy: move getting (R)MII refclock to phylib Heiner Kallweit
2023-03-24 18:03 ` [PATCH net-next 1/4] net: phylib: add getting reference clock Heiner Kallweit
2023-03-24 19:10 ` Florian Fainelli
2023-03-24 18:03 ` [PATCH net-next 2/4] net: phy: smsc: remove " Heiner Kallweit
2023-03-24 19:07 ` Florian Fainelli
2023-03-24 19:07 ` Florian Fainelli
2023-03-24 19:09 ` Florian Fainelli
2023-03-24 18:04 ` [PATCH net-next 3/4] net: phy: micrel: " Heiner Kallweit
2023-03-24 18:05 ` [PATCH net-next 4/4] net: phy: bcm7xxx: " Heiner Kallweit
2023-03-24 19:03 ` Florian Fainelli
2023-03-24 19:50 ` Heiner Kallweit
2023-03-24 20:11 ` Florian Fainelli
2023-03-24 21:16 ` Heiner Kallweit
2023-03-24 21:19 ` Florian Fainelli
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.