diff mbox series

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

Message ID 20230531150340.522994-2-detlev.casanova@collabora.com
State New
Headers
Series [1/2] dt-bindings: net: phy: Support external PHY xtal |

Commit Message

Detlev Casanova May 31, 2023, 3:03 p.m. UTC 
  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(+)

Comments

Heiner Kallweit May 31, 2023, 7:08 p.m. UTC | #1 
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?

> +	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;
Detlev Casanova June 1, 2023, 6:53 p.m. UTC | #2 
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;
Andrew Lunn June 1, 2023, 7:37 p.m. UTC | #3 
> 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
Florian Fainelli June 1, 2023, 8:14 p.m. UTC | #4 
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.
diff mbox series

Patch

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;