net: phy: realtek: Support external PHY clock

net: phy: realtek: Support external PHY clock

[Detlev Casanova]

Hi, thank you for your comments ! HEre are the changes that were
discussed.

Some PHYs can use an external clock that must be enabled before probing
it.

Changes since v1:
 * Remove the clock name as it is not guaranteed to be identical across
   different PHYs
 * Disable/Enable the clock when suspending/resuming

[PATCH v2 1/3] 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..b13dd0b3c99e 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, NULL);
+	if (IS_ERR(priv->clk))
+		return dev_err_probe(dev, PTR_ERR(priv->clk),
+				     "failed to get phy clock\n");
+
 	ret = phy_read_paged(phydev, 0xa43, RTL8211F_PHYCR1);
 	if (ret < 0)
 		return ret;
-- 
2.39.3

[PATCH v2 2/3] dt-bindings: net: phy: Document support for external PHY clk

[Detlev Casanova]

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

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

diff --git a/Documentation/devicetree/bindings/net/ethernet-phy.yaml b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
index 4f574532ee13..c1241c8a3b77 100644
--- a/Documentation/devicetree/bindings/net/ethernet-phy.yaml
+++ b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
@@ -93,6 +93,12 @@ properties:
       the turn around line low at end of the control phase of the
       MDIO transaction.
 
+  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 v2 3/3] net: phy: realtek: Disable clock on suspend

[Detlev Casanova]

For PHYs that call rtl821x_probe() where an external clock can be
configured, make sure that the clock is disabled
when ->suspend() is called and enabled on resume.

The PHY_ALWAYS_CALL_SUSPEND is added to ensure that the suspend function
is actually always called.

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

diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
index b13dd0b3c99e..62eac4835def 100644
--- a/drivers/net/phy/realtek.c
+++ b/drivers/net/phy/realtek.c
@@ -426,10 +426,28 @@ static int rtl8211f_config_init(struct phy_device *phydev)
 	return genphy_soft_reset(phydev);
 }
 
+static int rtl821x_suspend(struct phy_device *phydev)
+{
+	struct rtl821x_priv *priv = phydev->priv;
+	int ret = genphy_suspend(phydev);
+
+	if (ret)
+		return ret;
+
+	if (!phydev->wol_enabled)
+		clk_disable_unprepare(priv->clk);
+
+	return ret;
+}
+
 static int rtl821x_resume(struct phy_device *phydev)
 {
+	struct rtl821x_priv *priv = phydev->priv;
 	int ret;
 
+	if (!phydev->wol_enabled)
+		clk_prepare_enable(priv->clk);
+
 	ret = genphy_resume(phydev);
 	if (ret < 0)
 		return ret;
@@ -934,10 +952,11 @@ static struct phy_driver realtek_drvs[] = {
 		.read_status	= rtlgen_read_status,
 		.config_intr	= &rtl8211f_config_intr,
 		.handle_interrupt = rtl8211f_handle_interrupt,
-		.suspend	= genphy_suspend,
+		.suspend	= rtl821x_suspend,
 		.resume		= rtl821x_resume,
 		.read_page	= rtl821x_read_page,
 		.write_page	= rtl821x_write_page,
+		.flags		= PHY_ALWAYS_CALL_SUSPEND,
 	}, {
 		PHY_ID_MATCH_EXACT(RTL_8211FVD_PHYID),
 		.name		= "RTL8211F-VD Gigabit Ethernet",
@@ -946,10 +965,11 @@ static struct phy_driver realtek_drvs[] = {
 		.read_status	= rtlgen_read_status,
 		.config_intr	= &rtl8211f_config_intr,
 		.handle_interrupt = rtl8211f_handle_interrupt,
-		.suspend	= genphy_suspend,
+		.suspend	= rtl821x_suspend,
 		.resume		= rtl821x_resume,
 		.read_page	= rtl821x_read_page,
 		.write_page	= rtl821x_write_page,
+		.flags		= PHY_ALWAYS_CALL_SUSPEND,
 	}, {
 		.name		= "Generic FE-GE Realtek PHY",
 		.match_phy_device = rtlgen_match_phy_device,
-- 
2.39.3

Re: [PATCH v2 2/3] dt-bindings: net: phy: Document support for external PHY clk

[Andrew Lunn]

On Fri, Jun 02, 2023 at 02:26:58PM -0400, Detlev Casanova wrote:
> Ethern PHYs can have external an clock that needs to be activated before
> probing the PHY.

`Ethernet PHYs can have an external clock.`

We need to be careful with 'activated before probing the PHY'. phylib
itself will not activate the clock. You must be putting the IDs into
the compatible string, so the correct driver is loaded, and its probe
function is called. The probe itself enables the clock, so it is not
before probe, but during probe.

I'm picky about this because we have issues with enumerating the MDIO
bus to find PHYs. Some boards needs the PHY taking out of reset,
regulators enabled, clocks enabled etc, before the PHY will respond on
the bus. It is hard for the core to do this, before the probe. So we
recommend putting IDs in the compatible, so the driver probe function
to do any additional setup needed.

> Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
> ---
>  Documentation/devicetree/bindings/net/ethernet-phy.yaml | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/ethernet-phy.yaml b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
> index 4f574532ee13..c1241c8a3b77 100644
> --- a/Documentation/devicetree/bindings/net/ethernet-phy.yaml
> +++ b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
> @@ -93,6 +93,12 @@ properties:
>        the turn around line low at end of the control phase of the
>        MDIO transaction.
>  
> +  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.

This text is good.

    Andrew

---
pw-bot: cr

Re: [PATCH v2 1/3] net: phy: realtek: Add optional external PHY clock

[Andrew Lunn]

On Fri, Jun 02, 2023 at 02:26:57PM -0400, 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>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH v2 1/3] net: phy: realtek: Add optional external PHY clock

[Florian Fainelli]

On 6/2/23 11:26, 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>

Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
-- 
Florian

Re: [PATCH v2 3/3] net: phy: realtek: Disable clock on suspend

[Florian Fainelli]

On 6/2/23 11:26, Detlev Casanova wrote:
> For PHYs that call rtl821x_probe() where an external clock can be
> configured, make sure that the clock is disabled
> when ->suspend() is called and enabled on resume.
> 
> The PHY_ALWAYS_CALL_SUSPEND is added to ensure that the suspend function
> is actually always called.
> 
> Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>

Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
-- 
Florian

Re: [PATCH v2 2/3] dt-bindings: net: phy: Document support for external PHY clk

[Detlev Casanova]

On Friday, June 2, 2023 2:42:38 P.M. EDT Andrew Lunn wrote:
> On Fri, Jun 02, 2023 at 02:26:58PM -0400, Detlev Casanova wrote:
> > Ethern PHYs can have external an clock that needs to be activated before
> > probing the PHY.
> 
> `Ethernet PHYs can have an external clock.`
> 
> We need to be careful with 'activated before probing the PHY'. phylib
> itself will not activate the clock. You must be putting the IDs into
> the compatible string, so the correct driver is loaded, and its probe
> function is called. The probe itself enables the clock, so it is not
> before probe, but during probe.
> 
> I'm picky about this because we have issues with enumerating the MDIO
> bus to find PHYs. Some boards needs the PHY taking out of reset,
> regulators enabled, clocks enabled etc, before the PHY will respond on
> the bus. It is hard for the core to do this, before the probe. So we
> recommend putting IDs in the compatible, so the driver probe function
> to do any additional setup needed.

That makes sense, In my head, "probing" == calling phy_write/read() functions. 
But I get how this could be confused with the _probe() function. (And I just 
realised that there are typos)

What about "Ethernet PHYs can have an external clock that needs to be 
activated before communicating with the PHY" ?

> > Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
> > ---
> > 
> >  Documentation/devicetree/bindings/net/ethernet-phy.yaml | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/net/ethernet-phy.yaml
> > b/Documentation/devicetree/bindings/net/ethernet-phy.yaml index
> > 4f574532ee13..c1241c8a3b77 100644
> > --- a/Documentation/devicetree/bindings/net/ethernet-phy.yaml
> > +++ b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
> > 
> > @@ -93,6 +93,12 @@ properties:
> >        the turn around line low at end of the control phase of the
> >        MDIO transaction.
> > 
> > +  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.
> 
> This text is good.

Detlev

Re: [PATCH v2 2/3] dt-bindings: net: phy: Document support for external PHY clk

[Andrew Lunn]

> What about "Ethernet PHYs can have an external clock that needs to be 
> activated before communicating with the PHY" ?

That good.

Thanks
	Andrew

Re: [PATCH v2 2/3] dt-bindings: net: phy: Document support for external PHY clk

[Krzysztof Kozlowski]

On 02/06/2023 20:26, Detlev Casanova wrote:
> Ethern PHYs can have external an clock that needs to be activated before
> probing the PHY.
> 
> Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
> ---
>  Documentation/devicetree/bindings/net/ethernet-phy.yaml | 6 ++++++

With fixes from Andrew:

Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: net: phy: realtek: Support external PHY clock

[Jakub Kicinski]

On Mon, 5 Jun 2023 14:45:01 -0700 Jakub Kicinski wrote:
> On Mon,  5 Jun 2023 11:19:50 -0400 Detlev Casanova wrote:
> > Some PHYs can use an external clock that must be enabled before
> > communicating with them.  
> 
> Please add a [PATCH v4 0/0] prefix to the subject of the cover letter
> (or use --cover-letter with git format-patch to generate the template).
> Otherwise patchwork doesn't realize this is a cover letter.

And so it didn't realize you already sent a v4...
FWIW we ask people not to repost within 24h, confuses reviewers.

Re: net: phy: realtek: Support external PHY clock

[Jakub Kicinski]

On Mon,  5 Jun 2023 11:19:50 -0400 Detlev Casanova wrote:
> Some PHYs can use an external clock that must be enabled before
> communicating with them.

Please add a [PATCH v4 0/0] prefix to the subject of the cover letter
(or use --cover-letter with git format-patch to generate the template).
Otherwise patchwork doesn't realize this is a cover letter.

net: phy: realtek: Support external PHY clock

[Detlev Casanova]

Some PHYs can use an external clock that must be enabled before
communicating with them.

Changes since v2:
 * Reword documentation commit message

Changes since v1:
 * Remove the clock name as it is not guaranteed to be identical across
   different PHYs
 * Disable/Enable the clock when suspending/resuming