[PATCH 0/5] SMSC: Cleanups and clock setup

[PATCH 0/5] SMSC: Cleanups and clock setup

[Marco Felsch]

Hi,

this small series cleans the smsc-phy code a bit and adds the support to
specify the phy clock source. Adding the phy clock source support is
also the main purpose of this series.

Regards,
  Marco

Marco Felsch (5):
  net: phy: smsc: skip ENERGYON interrupt if disabled
  net: phy: smsc: simplify config_init callback
  dt-bindings: net: phy: smsc: document reference clock
  net: phy: smsc: add phy refclk in support
  net: phy: smsc: LAN8710/LAN8720: remove PHY_RST_AFTER_CLK_EN flag

 .../devicetree/bindings/net/smsc-lan87xx.txt  |  4 ++
 drivers/net/phy/smsc.c                        | 64 +++++++++++++++----
 2 files changed, 55 insertions(+), 13 deletions(-)

-- 
2.20.1

[PATCH 1/5] net: phy: smsc: skip ENERGYON interrupt if disabled

[Marco Felsch]

Don't enable the interrupt if the platform disable the energy detection
by "smsc,disable-energy-detect".

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 drivers/net/phy/smsc.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/net/phy/smsc.c b/drivers/net/phy/smsc.c
index 74568ae16125..fa539a867de6 100644
--- a/drivers/net/phy/smsc.c
+++ b/drivers/net/phy/smsc.c
@@ -37,10 +37,17 @@ struct smsc_phy_priv {
 
 static int smsc_phy_config_intr(struct phy_device *phydev)
 {
-	int rc = phy_write (phydev, MII_LAN83C185_IM,
-			((PHY_INTERRUPT_ENABLED == phydev->interrupts)
-			? MII_LAN83C185_ISF_INT_PHYLIB_EVENTS
-			: 0));
+	struct smsc_phy_priv *priv = phydev->priv;
+	u16 intmask = 0;
+	int rc;
+
+	if (phydev->interrupts) {
+		intmask = MII_LAN83C185_ISF_INT4 | MII_LAN83C185_ISF_INT6;
+		if (priv->energy_enable)
+			intmask |= MII_LAN83C185_ISF_INT7;
+	}
+
+	rc = phy_write(phydev, MII_LAN83C185_IM, intmask);
 
 	return rc < 0 ? rc : 0;
 }
-- 
2.20.1

[PATCH 2/5] net: phy: smsc: simplify config_init callback

[Marco Felsch]

Exit the driver specific config_init hook early if energy detection is
disabled. We can do this because we don't need to clear the interrupt
status here. Clearing the status should be removed anyway since this is
handled by the phy_enable_interrupts().

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 drivers/net/phy/smsc.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/net/phy/smsc.c b/drivers/net/phy/smsc.c
index fa539a867de6..79574fcbd880 100644
--- a/drivers/net/phy/smsc.c
+++ b/drivers/net/phy/smsc.c
@@ -62,19 +62,21 @@ static int smsc_phy_ack_interrupt(struct phy_device *phydev)
 static int smsc_phy_config_init(struct phy_device *phydev)
 {
 	struct smsc_phy_priv *priv = phydev->priv;
+	int rc;
+
+	if (!priv->energy_enable)
+		return 0;
 
-	int rc = phy_read(phydev, MII_LAN83C185_CTRL_STATUS);
+	rc = phy_read(phydev, MII_LAN83C185_CTRL_STATUS);
 
 	if (rc < 0)
 		return rc;
 
-	if (priv->energy_enable) {
-		/* Enable energy detect mode for this SMSC Transceivers */
-		rc = phy_write(phydev, MII_LAN83C185_CTRL_STATUS,
-			       rc | MII_LAN83C185_EDPWRDOWN);
-		if (rc < 0)
-			return rc;
-	}
+	/* Enable energy detect mode for this SMSC Transceivers */
+	rc = phy_write(phydev, MII_LAN83C185_CTRL_STATUS,
+		       rc | MII_LAN83C185_EDPWRDOWN);
+	if (rc < 0)
+		return rc;
 
 	return smsc_phy_ack_interrupt(phydev);
 }
-- 
2.20.1

[PATCH 3/5] dt-bindings: net: phy: smsc: document reference clock

[Marco Felsch]

Add support to specify the reference clock for the phy.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 Documentation/devicetree/bindings/net/smsc-lan87xx.txt | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/smsc-lan87xx.txt b/Documentation/devicetree/bindings/net/smsc-lan87xx.txt
index 8b7c719b0bb9..a8d0dc9a8c0e 100644
--- a/Documentation/devicetree/bindings/net/smsc-lan87xx.txt
+++ b/Documentation/devicetree/bindings/net/smsc-lan87xx.txt
@@ -5,6 +5,10 @@ through an Ethernet OF device node.
 
 Optional properties:
 
+- clocks:
+  The clock used as phy reference clock and is connected to phy
+  pin XTAL1/CLKIN.
+
 - smsc,disable-energy-detect:
   If set, do not enable energy detect mode for the SMSC phy.
   default: enable energy detect mode
-- 
2.20.1

[PATCH 4/5] net: phy: smsc: add phy refclk in support

[Marco Felsch]

Add support to specify the clock provider for the phy refclk and don't
rely on 'magic' host clock setup. [1] tried to address this by
introducing a flag and fixing the corresponding host. But this commit
breaks the IRQ support since the irq setup during .config_intr() is
thrown away because the reset comes from the side without respecting the
current phy-state within the phy-state-machine. Furthermore the commit
fixed the problem only for FEC based hosts other hosts acting like the
FEC are not covered.

This commit goes the other way around to address the bug fixed by [1].
Instead of resetting the device from the side every time the refclk gets
(re-)enabled it requests and enables the clock till the device gets
removed. The phy is still rest but now within the phylib and  with
respect to the phy-state-machine.

[1] commit 7f64e5b18ebb ("net: phy: smsc: LAN8710/20: add
    PHY_RST_AFTER_CLK_EN flag")

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 drivers/net/phy/smsc.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/drivers/net/phy/smsc.c b/drivers/net/phy/smsc.c
index 79574fcbd880..b98a7845681f 100644
--- a/drivers/net/phy/smsc.c
+++ b/drivers/net/phy/smsc.c
@@ -12,6 +12,7 @@
  *
  */
 
+#include <linux/clk.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/mii.h>
@@ -33,6 +34,7 @@ static struct smsc_hw_stat smsc_hw_stats[] = {
 
 struct smsc_phy_priv {
 	bool energy_enable;
+	struct clk *refclk;
 };
 
 static int smsc_phy_config_intr(struct phy_device *phydev)
@@ -194,11 +196,19 @@ static void smsc_get_stats(struct phy_device *phydev,
 		data[i] = smsc_get_stat(phydev, i);
 }
 
+static void smsc_clk_disable_action(void *data)
+{
+	struct smsc_phy_priv *priv = data;
+
+	clk_disable_unprepare(priv->refclk);
+}
+
 static int smsc_phy_probe(struct phy_device *phydev)
 {
 	struct device *dev = &phydev->mdio.dev;
 	struct device_node *of_node = dev->of_node;
 	struct smsc_phy_priv *priv;
+	int ret;
 
 	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
@@ -211,6 +221,26 @@ static int smsc_phy_probe(struct phy_device *phydev)
 
 	phydev->priv = priv;
 
+	priv->refclk = devm_clk_get_optional(dev, NULL);
+	if (IS_ERR(priv->refclk)) {
+		if (PTR_ERR(priv->refclk) == -EPROBE_DEFER)
+			return -EPROBE_DEFER;
+
+		/* Clocks are optional all errors should be ignored here */
+		return 0;
+	}
+
+	/* Starting from here errors should not be ignored anymore */
+	ret = clk_set_rate(priv->refclk, 50 * 1000 * 1000);
+	if (ret)
+		return ret;
+
+	ret = clk_prepare_enable(priv->refclk);
+	if (ret)
+		return ret;
+
+	devm_add_action_or_reset(dev, smsc_clk_disable_action, priv);
+
 	return 0;
 }
 
-- 
2.20.1

[PATCH 5/5] net: phy: smsc: LAN8710/LAN8720: remove PHY_RST_AFTER_CLK_EN flag

[Marco Felsch]

Don't reset the phy without respect to the phy-state-machine because
this breaks the phy IRQ mode. We can archive the same behaviour if the
refclk in is specified.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 drivers/net/phy/smsc.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/net/phy/smsc.c b/drivers/net/phy/smsc.c
index b98a7845681f..67adf11ef958 100644
--- a/drivers/net/phy/smsc.c
+++ b/drivers/net/phy/smsc.c
@@ -337,7 +337,6 @@ static struct phy_driver smsc_phy_driver[] = {
 	.name		= "SMSC LAN8710/LAN8720",
 
 	/* PHY_BASIC_FEATURES */
-	.flags		= PHY_RST_AFTER_CLK_EN,
 
 	.probe		= smsc_phy_probe,
 
-- 
2.20.1

Re: [PATCH 1/5] net: phy: smsc: skip ENERGYON interrupt if disabled

[Andrew Lunn]

On Mon, Aug 31, 2020 at 03:48:32PM +0200, Marco Felsch wrote:
> Don't enable the interrupt if the platform disable the energy detection
> by "smsc,disable-energy-detect".
> 
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
>  drivers/net/phy/smsc.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/phy/smsc.c b/drivers/net/phy/smsc.c
> index 74568ae16125..fa539a867de6 100644
> --- a/drivers/net/phy/smsc.c
> +++ b/drivers/net/phy/smsc.c
> @@ -37,10 +37,17 @@ struct smsc_phy_priv {
>  
>  static int smsc_phy_config_intr(struct phy_device *phydev)
>  {
> -	int rc = phy_write (phydev, MII_LAN83C185_IM,
> -			((PHY_INTERRUPT_ENABLED == phydev->interrupts)
> -			? MII_LAN83C185_ISF_INT_PHYLIB_EVENTS
> -			: 0));
> +	struct smsc_phy_priv *priv = phydev->priv;
> +	u16 intmask = 0;
> +	int rc;
> +
> +	if (phydev->interrupts) {
> +		intmask = MII_LAN83C185_ISF_INT4 | MII_LAN83C185_ISF_INT6;
> +		if (priv->energy_enable)
> +			intmask |= MII_LAN83C185_ISF_INT7;

Hi Marco

These names are not particularly helpful. The include file does
actually document the bits.

#define MII_LAN83C185_ISF_INT1 (1<<1) /* Auto-Negotiation Page Received */
#define MII_LAN83C185_ISF_INT2 (1<<2) /* Parallel Detection Fault */
#define MII_LAN83C185_ISF_INT3 (1<<3) /* Auto-Negotiation LP Ack */
#define MII_LAN83C185_ISF_INT4 (1<<4) /* Link Down */
#define MII_LAN83C185_ISF_INT5 (1<<5) /* Remote Fault Detected */
#define MII_LAN83C185_ISF_INT6 (1<<6) /* Auto-Negotiation complete */
#define MII_LAN83C185_ISF_INT7 (1<<7) /* ENERGYON */

If you feel like it, maybe add another patch which renames these to
something better. MII_LAN83C185_ISF_DOWN, MII_LAN83C185_ISF_ENERGY_ON,
etc?

For this patch:

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

    Andrew

Re: [PATCH 4/5] net: phy: smsc: add phy refclk in support

[Andrew Lunn]

> +	priv->refclk = devm_clk_get_optional(dev, NULL);
> +	if (IS_ERR(priv->refclk)) {
> +		if (PTR_ERR(priv->refclk) == -EPROBE_DEFER)
> +			return -EPROBE_DEFER;
> +
> +		/* Clocks are optional all errors should be ignored here */
> +		return 0;

Since you are calling devm_clk_get_optional() isn't an error a real
error, not that the clock is missing? It probably should be returned
as an error code.

   Andrew

Re: [PATCH 5/5] net: phy: smsc: LAN8710/LAN8720: remove PHY_RST_AFTER_CLK_EN flag

[Andrew Lunn]

On Mon, Aug 31, 2020 at 03:48:36PM +0200, Marco Felsch wrote:
> Don't reset the phy without respect to the phy-state-machine because
> this breaks the phy IRQ mode. We can archive the same behaviour if the
> refclk in is specified.
> 
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
>  drivers/net/phy/smsc.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/net/phy/smsc.c b/drivers/net/phy/smsc.c
> index b98a7845681f..67adf11ef958 100644
> --- a/drivers/net/phy/smsc.c
> +++ b/drivers/net/phy/smsc.c
> @@ -337,7 +337,6 @@ static struct phy_driver smsc_phy_driver[] = {
>  	.name		= "SMSC LAN8710/LAN8720",
>  
>  	/* PHY_BASIC_FEATURES */
> -	.flags		= PHY_RST_AFTER_CLK_EN,
>  
>  	.probe		= smsc_phy_probe,

Hi Marco

There are two PHYs using PHY_RST_AFTER_CLK_EN. What about the other
one?

	Andrew

Re: [PATCH 4/5] net: phy: smsc: add phy refclk in support

[Florian Fainelli]

On 8/31/2020 6:48 AM, Marco Felsch wrote:
> Add support to specify the clock provider for the phy refclk and don't
> rely on 'magic' host clock setup. [1] tried to address this by
> introducing a flag and fixing the corresponding host. But this commit
> breaks the IRQ support since the irq setup during .config_intr() is
> thrown away because the reset comes from the side without respecting the
> current phy-state within the phy-state-machine. Furthermore the commit
> fixed the problem only for FEC based hosts other hosts acting like the
> FEC are not covered.
> 
> This commit goes the other way around to address the bug fixed by [1].
> Instead of resetting the device from the side every time the refclk gets
> (re-)enabled it requests and enables the clock till the device gets
> removed. The phy is still rest but now within the phylib and  with
> respect to the phy-state-machine.
> 
> [1] commit 7f64e5b18ebb ("net: phy: smsc: LAN8710/20: add
>      PHY_RST_AFTER_CLK_EN flag")
> 
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
>   drivers/net/phy/smsc.c | 30 ++++++++++++++++++++++++++++++
>   1 file changed, 30 insertions(+)
> 
> diff --git a/drivers/net/phy/smsc.c b/drivers/net/phy/smsc.c
> index 79574fcbd880..b98a7845681f 100644
> --- a/drivers/net/phy/smsc.c
> +++ b/drivers/net/phy/smsc.c
> @@ -12,6 +12,7 @@
>    *
>    */
>   
> +#include <linux/clk.h>
>   #include <linux/kernel.h>
>   #include <linux/module.h>
>   #include <linux/mii.h>
> @@ -33,6 +34,7 @@ static struct smsc_hw_stat smsc_hw_stats[] = {
>   
>   struct smsc_phy_priv {
>   	bool energy_enable;
> +	struct clk *refclk;
>   };
>   
>   static int smsc_phy_config_intr(struct phy_device *phydev)
> @@ -194,11 +196,19 @@ static void smsc_get_stats(struct phy_device *phydev,
>   		data[i] = smsc_get_stat(phydev, i);
>   }
>   
> +static void smsc_clk_disable_action(void *data)
> +{
> +	struct smsc_phy_priv *priv = data;
> +
> +	clk_disable_unprepare(priv->refclk);
> +}
> +
>   static int smsc_phy_probe(struct phy_device *phydev)
>   {
>   	struct device *dev = &phydev->mdio.dev;
>   	struct device_node *of_node = dev->of_node;
>   	struct smsc_phy_priv *priv;
> +	int ret;
>   
>   	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>   	if (!priv)
> @@ -211,6 +221,26 @@ static int smsc_phy_probe(struct phy_device *phydev)
>   
>   	phydev->priv = priv;
>   
> +	priv->refclk = devm_clk_get_optional(dev, NULL);
> +	if (IS_ERR(priv->refclk)) {
> +		if (PTR_ERR(priv->refclk) == -EPROBE_DEFER)
> +			return -EPROBE_DEFER;
> +
> +		/* Clocks are optional all errors should be ignored here */
> +		return 0;
> +	}
> +
> +	/* Starting from here errors should not be ignored anymore */
> +	ret = clk_set_rate(priv->refclk, 50 * 1000 * 1000);
> +	if (ret)
> +		return ret;

The clock should be enabled first before attempting a rate change, and 
this also causes a more fundamental question: what is the sate of the 
clock when the PHY driver is probed, and is the reference clock feeding 
into the MDIO logic of the PHY.

By that I mean that if the reference clock was disabled, would the PHY 
still respond to MDIO reads such that you would be able to probe and 
identify it?

If not, your demv_clk_get_optional() is either too late, or assuming a 
prior state, or you are working around this in Device Tree by using a 
compatible string with the form 
"^ethernet-phy-id[a-f0-9]{4}\\.[a-f0-9]{4}$" in which case, this is a 
making assumptions about how the OF MDIO layer works which is not ideal.

I am preparing some patches that aim at enabling a given MDIO device's 
clock prior to probing it and should be able to post them by today.

> +
> +	ret = clk_prepare_enable(priv->refclk);
> +	if (ret)
> +		return ret;
> +
> +	devm_add_action_or_reset(dev, smsc_clk_disable_action, priv);
> +
>   	return 0;
>   }
>   
> 

-- 
Florian

Re: [PATCH 1/5] net: phy: smsc: skip ENERGYON interrupt if disabled

[Marco Felsch]

Hi Andrew,

thanks for your fast response :)

On 20-08-31 16:02, Andrew Lunn wrote:
> On Mon, Aug 31, 2020 at 03:48:32PM +0200, Marco Felsch wrote:
> > Don't enable the interrupt if the platform disable the energy detection
> > by "smsc,disable-energy-detect".
> > 
> > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > ---
> >  drivers/net/phy/smsc.c | 15 +++++++++++----
> >  1 file changed, 11 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/net/phy/smsc.c b/drivers/net/phy/smsc.c
> > index 74568ae16125..fa539a867de6 100644
> > --- a/drivers/net/phy/smsc.c
> > +++ b/drivers/net/phy/smsc.c
> > @@ -37,10 +37,17 @@ struct smsc_phy_priv {
> >  
> >  static int smsc_phy_config_intr(struct phy_device *phydev)
> >  {
> > -	int rc = phy_write (phydev, MII_LAN83C185_IM,
> > -			((PHY_INTERRUPT_ENABLED == phydev->interrupts)
> > -			? MII_LAN83C185_ISF_INT_PHYLIB_EVENTS
> > -			: 0));
> > +	struct smsc_phy_priv *priv = phydev->priv;
> > +	u16 intmask = 0;
> > +	int rc;
> > +
> > +	if (phydev->interrupts) {
> > +		intmask = MII_LAN83C185_ISF_INT4 | MII_LAN83C185_ISF_INT6;
> > +		if (priv->energy_enable)
> > +			intmask |= MII_LAN83C185_ISF_INT7;
> 
> Hi Marco
> 
> These names are not particularly helpful. The include file does
> actually document the bits.
> 
> #define MII_LAN83C185_ISF_INT1 (1<<1) /* Auto-Negotiation Page Received */
> #define MII_LAN83C185_ISF_INT2 (1<<2) /* Parallel Detection Fault */
> #define MII_LAN83C185_ISF_INT3 (1<<3) /* Auto-Negotiation LP Ack */
> #define MII_LAN83C185_ISF_INT4 (1<<4) /* Link Down */
> #define MII_LAN83C185_ISF_INT5 (1<<5) /* Remote Fault Detected */
> #define MII_LAN83C185_ISF_INT6 (1<<6) /* Auto-Negotiation complete */
> #define MII_LAN83C185_ISF_INT7 (1<<7) /* ENERGYON */
> 
> If you feel like it, maybe add another patch which renames these to
> something better. MII_LAN83C185_ISF_DOWN, MII_LAN83C185_ISF_ENERGY_ON,
> etc?

I know.. I will add a patch to change this after we get the clock
discussion done.

Regards,
  Marco

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

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 5/5] net: phy: smsc: LAN8710/LAN8720: remove PHY_RST_AFTER_CLK_EN flag

[Marco Felsch]

Hi Andrew,

On 20-08-31 16:11, Andrew Lunn wrote:
> On Mon, Aug 31, 2020 at 03:48:36PM +0200, Marco Felsch wrote:
> > Don't reset the phy without respect to the phy-state-machine because
> > this breaks the phy IRQ mode. We can archive the same behaviour if the
> > refclk in is specified.
> > 
> > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > ---
> >  drivers/net/phy/smsc.c | 1 -
> >  1 file changed, 1 deletion(-)
> > 
> > diff --git a/drivers/net/phy/smsc.c b/drivers/net/phy/smsc.c
> > index b98a7845681f..67adf11ef958 100644
> > --- a/drivers/net/phy/smsc.c
> > +++ b/drivers/net/phy/smsc.c
> > @@ -337,7 +337,6 @@ static struct phy_driver smsc_phy_driver[] = {
> >  	.name		= "SMSC LAN8710/LAN8720",
> >  
> >  	/* PHY_BASIC_FEATURES */
> > -	.flags		= PHY_RST_AFTER_CLK_EN,
> >  
> >  	.probe		= smsc_phy_probe,
> 
> Hi Marco
> 
> There are two PHYs using PHY_RST_AFTER_CLK_EN. What about the other
> one?

I think that they are broken too but I can't verify this therefore I
left them out.

Regards,
  Marco

> 
> 	Andrew
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 4/5] net: phy: smsc: add phy refclk in support

[Marco Felsch]

On 20-08-31 16:08, Andrew Lunn wrote:
> > +	priv->refclk = devm_clk_get_optional(dev, NULL);
> > +	if (IS_ERR(priv->refclk)) {
> > +		if (PTR_ERR(priv->refclk) == -EPROBE_DEFER)
> > +			return -EPROBE_DEFER;
> > +
> > +		/* Clocks are optional all errors should be ignored here */
> > +		return 0;
> 
> Since you are calling devm_clk_get_optional() isn't an error a real
> error, not that the clock is missing? It probably should be returned
> as an error code.

Yes you're right. Actually I can't remember why went this way... I will
change this to dev_err_probe() and this gets a oneliner.

Regards,
  Marco

> 
>    Andrew
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 4/5] net: phy: smsc: add phy refclk in support

[Marco Felsch]

On 20-08-31 09:32, Florian Fainelli wrote:
> 
> 
> On 8/31/2020 6:48 AM, Marco Felsch wrote:
> > Add support to specify the clock provider for the phy refclk and don't
> > rely on 'magic' host clock setup. [1] tried to address this by
> > introducing a flag and fixing the corresponding host. But this commit
> > breaks the IRQ support since the irq setup during .config_intr() is
> > thrown away because the reset comes from the side without respecting the
> > current phy-state within the phy-state-machine. Furthermore the commit
> > fixed the problem only for FEC based hosts other hosts acting like the
> > FEC are not covered.
> > 
> > This commit goes the other way around to address the bug fixed by [1].
> > Instead of resetting the device from the side every time the refclk gets
> > (re-)enabled it requests and enables the clock till the device gets
> > removed. The phy is still rest but now within the phylib and  with
> > respect to the phy-state-machine.
> > 
> > [1] commit 7f64e5b18ebb ("net: phy: smsc: LAN8710/20: add
> >      PHY_RST_AFTER_CLK_EN flag")
> > 
> > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > ---
> >   drivers/net/phy/smsc.c | 30 ++++++++++++++++++++++++++++++
> >   1 file changed, 30 insertions(+)
> > 
> > diff --git a/drivers/net/phy/smsc.c b/drivers/net/phy/smsc.c
> > index 79574fcbd880..b98a7845681f 100644
> > --- a/drivers/net/phy/smsc.c
> > +++ b/drivers/net/phy/smsc.c
> > @@ -12,6 +12,7 @@
> >    *
> >    */
> > +#include <linux/clk.h>
> >   #include <linux/kernel.h>
> >   #include <linux/module.h>
> >   #include <linux/mii.h>
> > @@ -33,6 +34,7 @@ static struct smsc_hw_stat smsc_hw_stats[] = {
> >   struct smsc_phy_priv {
> >   	bool energy_enable;
> > +	struct clk *refclk;
> >   };
> >   static int smsc_phy_config_intr(struct phy_device *phydev)
> > @@ -194,11 +196,19 @@ static void smsc_get_stats(struct phy_device *phydev,
> >   		data[i] = smsc_get_stat(phydev, i);
> >   }
> > +static void smsc_clk_disable_action(void *data)
> > +{
> > +	struct smsc_phy_priv *priv = data;
> > +
> > +	clk_disable_unprepare(priv->refclk);
> > +}
> > +
> >   static int smsc_phy_probe(struct phy_device *phydev)
> >   {
> >   	struct device *dev = &phydev->mdio.dev;
> >   	struct device_node *of_node = dev->of_node;
> >   	struct smsc_phy_priv *priv;
> > +	int ret;
> >   	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> >   	if (!priv)
> > @@ -211,6 +221,26 @@ static int smsc_phy_probe(struct phy_device *phydev)
> >   	phydev->priv = priv;
> > +	priv->refclk = devm_clk_get_optional(dev, NULL);
> > +	if (IS_ERR(priv->refclk)) {
> > +		if (PTR_ERR(priv->refclk) == -EPROBE_DEFER)
> > +			return -EPROBE_DEFER;
> > +
> > +		/* Clocks are optional all errors should be ignored here */
> > +		return 0;
> > +	}
> > +
> > +	/* Starting from here errors should not be ignored anymore */
> > +	ret = clk_set_rate(priv->refclk, 50 * 1000 * 1000);
> > +	if (ret)
> > +		return ret;
> 
> The clock should be enabled first before attempting a rate change

Is this the way to use the API? My understanding was to set the correct
clk value before we enable the clk value can be out-of-range for the
phy. But you have a point, we should:

ret = clk_prepare(priv->refclk);
if (ret)
	return ret;

ret = clk_set_rate(priv->refclk, 50 * 1000 * 1000);
if (ret)
	return ret;

ret = clk_enable(priv->refclk);
if (ret)
	return ret;

to avoide the usage of unprepared clocks.

>, and this
> also causes a more fundamental question: what is the sate of the clock when
> the PHY driver is probed, and is the reference clock feeding into the MDIO
> logic of the PHY.

Currently this state is defined by the bootloader if the clk is provided
by the host.

> By that I mean that if the reference clock was disabled, would the PHY still
> respond to MDIO reads such that you would be able to probe and identify it?

Pls correct me if I'm wrong but currently all phy drivers relying on the
settings made by the bootloader/firmware.

> If not, your demv_clk_get_optional() is either too late

Yes, I got this.

> , or assuming a prior state,

This is the our case. Isn't it the purpose of the bootloader to setup
the HW?

> or you are working around this in Device Tree by using a compatible
> string with the form "^ethernet-phy-id[a-f0-9]{4}\\.[a-f0-9]{4}$" in which
> case, this is a making assumptions about how the OF MDIO layer works which
> is not ideal.

Nope, I'm using "ethernet-phy-ieee802.3-c22".

> I am preparing some patches that aim at enabling a given MDIO device's clock
> prior to probing it and should be able to post them by today.

Create :) Can you provide me a link? What I can say for now is: This
solution was used by the micrel driver too and it seems to work. I
wanted to keep the change smaller/more local because the current
upstream state is: SMSC-Phy <-> FEC-Host ==> IRQ broken. If your patch
fixes this too in a more general matter I'm fine with it and we can drop
this patch but we should fix this as soon as possible.

Regards,
  Marco

> > +
> > +	ret = clk_prepare_enable(priv->refclk);
> > +	if (ret)
> > +		return ret;
> > +
> > +	devm_add_action_or_reset(dev, smsc_clk_disable_action, priv);
> > +
> >   	return 0;
> >   }
> > 
> 
> -- 
> Florian
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 4/5] net: phy: smsc: add phy refclk in support

[Andrew Lunn]

> Yes, I got this.
> 
> > , or assuming a prior state,
> 
> This is the our case. Isn't it the purpose of the bootloader to setup
> the HW?

This is a bit of a philosophical discussion. For PCs developers would
definitely agree, the firmware should be setting up most of the
hardware. And the firmware is involved in driving the hardware, via
ACPI. That works because you mostly cannot replaces the firmware.

In the ARM world we tend to take the opposite view. The bootloader
does the minimum to get the OS running, and the OS then setups up
everything. Often there are a choice of bootloaders, you have no idea
if the vendor bootload has been replaced by a mainline one with extra
features, etc. And we have no idea what the bootloader is actually
doing, so we try to assume nothing.

       Andrew

Re: [PATCH 4/5] net: phy: smsc: add phy refclk in support

[Florian Fainelli]

On 9/1/2020 1:24 AM, Marco Felsch wrote:
> Create :) Can you provide me a link? What I can say for now is: This
> solution was used by the micrel driver too and it seems to work. I
> wanted to keep the change smaller/more local because the current
> upstream state is: SMSC-Phy <-> FEC-Host ==> IRQ broken. If your patch
> fixes this too in a more general matter I'm fine with it and we can drop
> this patch but we should fix this as soon as possible.

Still working on it, I found a platform where there were some timing 
problems and am looking into it.

If your SMSC changes are so important and critical, get them applied to 
the "net" tree, your patch posting makes no mention of which netdev tree 
you are targeting.
-- 
Florian