[PATCH 1/2] dt-bindings: net: phy: Support external PHY xtal

[PATCH 1/2] dt-bindings: net: phy: Support external PHY xtal

[Detlev Casanova]

Ethernet PHYs can have external an clock that needs to be activated before
probing the PHY.

Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
---
 .../devicetree/bindings/net/ethernet-phy.yaml          | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/ethernet-phy.yaml b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
index 4f574532ee13..e83a33c2aa59 100644
--- a/Documentation/devicetree/bindings/net/ethernet-phy.yaml
+++ b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
@@ -93,6 +93,16 @@ properties:
       the turn around line low at end of the control phase of the
       MDIO transaction.
 
+  clock-names:
+    items:
+      - const: xtal
+
+  clocks:
+    maxItems: 1
+    description:
+      External clock connected to the PHY. If not specified it is assumed
+      that the PHY uses a fixed crystal or an internal oscillator.
+
   enet-phy-lane-swap:
     $ref: /schemas/types.yaml#/definitions/flag
     description:
-- 
2.39.3

[PATCH 2/2] net: phy: realtek: Add optional external PHY clock

[Detlev Casanova]

In some cases, the PHY can use an external clock source instead of a
crystal.

Add an optional clock in the phy node to make sure that the clock source
is enabled, if specified, before probing.

Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
---
 drivers/net/phy/realtek.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
index 3d99fd6664d7..70c75dbbf799 100644
--- a/drivers/net/phy/realtek.c
+++ b/drivers/net/phy/realtek.c
@@ -12,6 +12,7 @@
 #include <linux/phy.h>
 #include <linux/module.h>
 #include <linux/delay.h>
+#include <linux/clk.h>
 
 #define RTL821x_PHYSR				0x11
 #define RTL821x_PHYSR_DUPLEX			BIT(13)
@@ -80,6 +81,7 @@ struct rtl821x_priv {
 	u16 phycr1;
 	u16 phycr2;
 	bool has_phycr2;
+	struct clk *clk;
 };
 
 static int rtl821x_read_page(struct phy_device *phydev)
@@ -103,6 +105,11 @@ static int rtl821x_probe(struct phy_device *phydev)
 	if (!priv)
 		return -ENOMEM;
 
+	priv->clk = devm_clk_get_optional_enabled(dev, "xtal");
+	if (IS_ERR(priv->clk))
+		return dev_err_probe(dev, PTR_ERR(priv->clk),
+				     "failed to get phy xtal clock\n");
+
 	ret = phy_read_paged(phydev, 0xa43, RTL8211F_PHYCR1);
 	if (ret < 0)
 		return ret;
-- 
2.39.3

Re: [PATCH 1/2] dt-bindings: net: phy: Support external PHY xtal

[Andrew Lunn]

On Wed, May 31, 2023 at 11:03:39AM -0400, Detlev Casanova wrote:
> Ethernet PHYs can have external an clock that needs to be activated before
> probing the PHY.
> 
> Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
> ---
>  .../devicetree/bindings/net/ethernet-phy.yaml          | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/ethernet-phy.yaml b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
> index 4f574532ee13..e83a33c2aa59 100644
> --- a/Documentation/devicetree/bindings/net/ethernet-phy.yaml
> +++ b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
> @@ -93,6 +93,16 @@ properties:
>        the turn around line low at end of the control phase of the
>        MDIO transaction.
>  
> +  clock-names:
> +    items:
> +      - const: xtal

I don't think xtal is the best of names here. It generally is used as
an abbreviation for crystal. And the commit message is about there not
being a crystal, but an actual clock.

How is this clock named on the datasheet?

    Andrew

Re: [PATCH 1/2] dt-bindings: net: phy: Support external PHY xtal

[Detlev Casanova]

On Wednesday, May 31, 2023 11:16:46 A.M. EDT Andrew Lunn wrote:
> On Wed, May 31, 2023 at 11:03:39AM -0400, Detlev Casanova wrote:
> > Ethernet PHYs can have external an clock that needs to be activated before
> > probing the PHY.
> > 
> > Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
> > ---
> > 
> >  .../devicetree/bindings/net/ethernet-phy.yaml          | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/net/ethernet-phy.yaml
> > b/Documentation/devicetree/bindings/net/ethernet-phy.yaml index
> > 4f574532ee13..e83a33c2aa59 100644
> > --- a/Documentation/devicetree/bindings/net/ethernet-phy.yaml
> > +++ b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
> > 
> > @@ -93,6 +93,16 @@ properties:
> >        the turn around line low at end of the control phase of the
> >        MDIO transaction.
> > 
> > +  clock-names:
> > +    items:
> > +      - const: xtal
> 
> I don't think xtal is the best of names here. It generally is used as
> an abbreviation for crystal. And the commit message is about there not
> being a crystal, but an actual clock.
> 
> How is this clock named on the datasheet?

In the case of the PHY I used (RTL8211F), it is EXT_CLK. But this must be 
generic to any (ethernet) PHY, so using ext_clk to match it would not be
good either.

Now this is about having an external clock, so the ext_clk name makes sense in 
this case.

I'm not pushing one name or another, let's use what you feel is more natural.

Detlev.

Re: [PATCH 1/2] dt-bindings: net: phy: Support external PHY xtal

[Florian Fainelli]

On 5/31/23 11:00, Detlev Casanova wrote:
> On Wednesday, May 31, 2023 11:16:46 A.M. EDT Andrew Lunn wrote:
>> On Wed, May 31, 2023 at 11:03:39AM -0400, Detlev Casanova wrote:
>>> Ethernet PHYs can have external an clock that needs to be activated before
>>> probing the PHY.
>>>
>>> Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
>>> ---
>>>
>>>   .../devicetree/bindings/net/ethernet-phy.yaml          | 10 ++++++++++
>>>   1 file changed, 10 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/net/ethernet-phy.yaml
>>> b/Documentation/devicetree/bindings/net/ethernet-phy.yaml index
>>> 4f574532ee13..e83a33c2aa59 100644
>>> --- a/Documentation/devicetree/bindings/net/ethernet-phy.yaml
>>> +++ b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
>>>
>>> @@ -93,6 +93,16 @@ properties:
>>>         the turn around line low at end of the control phase of the
>>>         MDIO transaction.
>>>
>>> +  clock-names:
>>> +    items:
>>> +      - const: xtal
>>
>> I don't think xtal is the best of names here. It generally is used as
>> an abbreviation for crystal. And the commit message is about there not
>> being a crystal, but an actual clock.
>>
>> How is this clock named on the datasheet?
> 
> In the case of the PHY I used (RTL8211F), it is EXT_CLK. But this must be
> generic to any (ethernet) PHY, so using ext_clk to match it would not be
> good either.
> 
> Now this is about having an external clock, so the ext_clk name makes sense in
> this case.
> 
> I'm not pushing one name or another, let's use what you feel is more natural.

You can look up clocks by positional index, maybe this is a case where 
there are just too many names that PHY vendors will use that we should 
not be using one specific name in particular, but just define the order 
in which clocks should be specified.
-- 
Florian

Re: [PATCH 1/2] dt-bindings: net: phy: Support external PHY xtal

[Krzysztof Kozlowski]

On 31/05/2023 20:00, Detlev Casanova wrote:
>>> +  clock-names:
>>> +    items:
>>> +      - const: xtal
>>
>> I don't think xtal is the best of names here. It generally is used as
>> an abbreviation for crystal. And the commit message is about there not
>> being a crystal, but an actual clock.
>>
>> How is this clock named on the datasheet?
> 
> In the case of the PHY I used (RTL8211F), it is EXT_CLK. But this must be 
> generic to any (ethernet) PHY, so using ext_clk to match it would not be
> good either.
> 
> Now this is about having an external clock, so the ext_clk name makes sense in 
> this case.
> 
> I'm not pushing one name or another, let's use what you feel is more natural.

Just drop the name.

Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: net: phy: Support external PHY xtal

[Detlev Casanova]

On Thursday, June 1, 2023 12:52:18 P.M. EDT Krzysztof Kozlowski wrote:
> On 31/05/2023 20:00, Detlev Casanova wrote:
> >>> +  clock-names:
> >>> +    items:
> >>> +      - const: xtal
> >> 
> >> I don't think xtal is the best of names here. It generally is used as
> >> an abbreviation for crystal. And the commit message is about there not
> >> being a crystal, but an actual clock.
> >> 
> >> How is this clock named on the datasheet?
> > 
> > In the case of the PHY I used (RTL8211F), it is EXT_CLK. But this must be
> > generic to any (ethernet) PHY, so using ext_clk to match it would not be
> > good either.
> > 
> > Now this is about having an external clock, so the ext_clk name makes
> > sense in this case.
> > 
> > I'm not pushing one name or another, let's use what you feel is more
> > natural.
> Just drop the name.

So I can just use devm_clk_get_optional_enabled(dev, NULL) and I'll get the 
first clock defines in the device tree ?

Detlev.

Re: [PATCH 1/2] dt-bindings: net: phy: Support external PHY xtal

[Andrew Lunn]

> So I can just use devm_clk_get_optional_enabled(dev, NULL) and I'll get the 
> first clock defines in the device tree ?

Yes:

bcm7xxx.c:	clk = devm_clk_get_optional_enabled(&phydev->mdio.dev, NULL);
micrel.c:	clk = devm_clk_get(&phydev->mdio.dev, "rmii-ref");
smsc.c:		refclk = devm_clk_get_optional_enabled(dev, NULL);

	Andrew

Re: [PATCH 2/2] net: phy: realtek: Add optional external PHY clock

[Detlev Casanova]

On Wednesday, May 31, 2023 3:08:53 P.M. EDT Heiner Kallweit wrote:
> On 31.05.2023 17:03, Detlev Casanova wrote:
> > In some cases, the PHY can use an external clock source instead of a
> > crystal.
> > 
> > Add an optional clock in the phy node to make sure that the clock source
> > is enabled, if specified, before probing.
> > 
> > Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
> > ---
> > 
> >  drivers/net/phy/realtek.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
> > index 3d99fd6664d7..70c75dbbf799 100644
> > --- a/drivers/net/phy/realtek.c
> > +++ b/drivers/net/phy/realtek.c
> > @@ -12,6 +12,7 @@
> > 
> >  #include <linux/phy.h>
> >  #include <linux/module.h>
> >  #include <linux/delay.h>
> > 
> > +#include <linux/clk.h>
> > 
> >  #define RTL821x_PHYSR				0x11
> >  #define RTL821x_PHYSR_DUPLEX			BIT(13)
> > 
> > @@ -80,6 +81,7 @@ struct rtl821x_priv {
> > 
> >  	u16 phycr1;
> >  	u16 phycr2;
> >  	bool has_phycr2;
> > 
> > +	struct clk *clk;
> > 
> >  };
> >  
> >  static int rtl821x_read_page(struct phy_device *phydev)
> > 
> > @@ -103,6 +105,11 @@ static int rtl821x_probe(struct phy_device *phydev)
> > 
> >  	if (!priv)
> >  	
> >  		return -ENOMEM;
> > 
> > +	priv->clk = devm_clk_get_optional_enabled(dev, "xtal");
> 
> Why add priv->clk if it isn't used outside probe()?
> 
> How about suspend/resume? Would it make sense to stop the clock
> whilst PHY is suspended?

I'm not sure about this. Isn't the clock still necessary when suspended for 
things like wake on lan ?

> > +	if (IS_ERR(priv->clk))
> > +		return dev_err_probe(dev, PTR_ERR(priv->clk),
> > +				     "failed to get phy xtal 
clock\n");
> > +
> > 
> >  	ret = phy_read_paged(phydev, 0xa43, RTL8211F_PHYCR1);
> >  	if (ret < 0)
> >  	
> >  		return ret;

Re: [PATCH 2/2] net: phy: realtek: Add optional external PHY clock

[Andrew Lunn]

> I'm not sure about this. Isn't the clock still necessary when suspended for 
> things like wake on lan ?

Yes, but the PHY should know if its a WoL source, and not disable its
own clock. There is some support for this in phylib, and Florian has
also reworked it recently for Broadcom PHYs.

     Andrew

Re: [PATCH 2/2] net: phy: realtek: Add optional external PHY clock

[Florian Fainelli]

On 6/1/23 12:37, Andrew Lunn wrote:
>> I'm not sure about this. Isn't the clock still necessary when suspended for
>> things like wake on lan ?
> 
> Yes, but the PHY should know if its a WoL source, and not disable its
> own clock. There is some support for this in phylib, and Florian has
> also reworked it recently for Broadcom PHYs.

If you want to have the PHY driver have a chance to disable the clock if 
Wake-on-LAN is disabled and therefore conserve power, you should set 
PHY_ALWAYS_CALL_SUSPEND in the phy_driver::flags and in the 
suspend/resume functions do something like:

suspend:
	/* last step after all registers are accessed */
	if (!phydev->wol_enabled)
		clk_disable_unprepare()
resume:
	/* first step before registers are accessed */
	if (!phydev->wol_enabled)
		clk_prepare_enable()

The flag is necessary to ensure that the PHY driver's suspend function 
will be called. The resume will be called regardless of Wake-on-LAN 
being enabled or not.
-- 
Florian