| Message ID | 20230531150340.522994-2-detlev.casanova@collabora.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [1/2] dt-bindings: net: phy: Support external PHY xtal | expand |
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;
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;
> 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
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 --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;
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(+)