[PATCHv3 net-next] net: fec: Ensure clocks are enabled while using mdio bus

[PATCHv3 net-next] net: fec: Ensure clocks are enabled while using mdio bus

[Andrew Lunn]

When a switch is attached to the mdio bus, the mdio bus can be used
while the interface is not open. If the IPG clock are not enabled,
MDIO reads/writes will simply time out. So enable the clock before
starting a transaction, and disable it afterwards. The CCF performs
reference counting so the clock will only be disabled if there are no
other users.

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

v3:
 Return the error code from clk_prepare_enable()
v2:
 Only enable the IGP clock.

 drivers/net/ethernet/freescale/fec_main.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index bf4cf3fbb5f2..8d9b1fd175f7 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -65,6 +65,7 @@
 
 static void set_multicast_list(struct net_device *ndev);
 static void fec_enet_itr_coal_init(struct net_device *ndev);
+static int fec_enet_clk_enable(struct net_device *ndev, bool enable);
 
 #define DRIVER_NAME	"fec"
 
@@ -1764,6 +1765,11 @@ static int fec_enet_mdio_read(struct mii_bus *bus, int mii_id, int regnum)
 {
 	struct fec_enet_private *fep = bus->priv;
 	unsigned long time_left;
+	int ret;
+
+	ret = clk_prepare_enable(fep->clk_ipg);
+	if (ret)
+		return ret;
 
 	fep->mii_timeout = 0;
 	init_completion(&fep->mdio_done);
@@ -1779,11 +1785,14 @@ static int fec_enet_mdio_read(struct mii_bus *bus, int mii_id, int regnum)
 	if (time_left == 0) {
 		fep->mii_timeout = 1;
 		netdev_err(fep->netdev, "MDIO read timeout\n");
+		clk_disable_unprepare(fep->clk_ipg);
 		return -ETIMEDOUT;
 	}
 
-	/* return value */
-	return FEC_MMFR_DATA(readl(fep->hwp + FEC_MII_DATA));
+	ret = FEC_MMFR_DATA(readl(fep->hwp + FEC_MII_DATA));
+	clk_disable_unprepare(fep->clk_ipg);
+
+	return ret;
 }
 
 static int fec_enet_mdio_write(struct mii_bus *bus, int mii_id, int regnum,
@@ -1791,10 +1800,15 @@ static int fec_enet_mdio_write(struct mii_bus *bus, int mii_id, int regnum,
 {
 	struct fec_enet_private *fep = bus->priv;
 	unsigned long time_left;
+	int ret;
 
 	fep->mii_timeout = 0;
 	init_completion(&fep->mdio_done);
 
+	ret = clk_prepare_enable(fep->clk_ipg);
+	if (ret)
+		return ret;
+
 	/* start a write op */
 	writel(FEC_MMFR_ST | FEC_MMFR_OP_WRITE |
 		FEC_MMFR_PA(mii_id) | FEC_MMFR_RA(regnum) |
@@ -1807,9 +1821,12 @@ static int fec_enet_mdio_write(struct mii_bus *bus, int mii_id, int regnum,
 	if (time_left == 0) {
 		fep->mii_timeout = 1;
 		netdev_err(fep->netdev, "MDIO write timeout\n");
+		clk_disable_unprepare(fep->clk_ipg);
 		return -ETIMEDOUT;
 	}
 
+	clk_disable_unprepare(fep->clk_ipg);
+
 	return 0;
 }
 
-- 
2.1.4

RE: [PATCHv3 net-next] net: fec: Ensure clocks are enabled while using mdio bus

[Duan Andy]

From: Andrew Lunn <andrew@lunn.ch> Sent: Sunday, June 21, 2015 2:38 AM
> To: David Miller
> Cc: Duan Fugang-B38611; Duan Fugang-B38611; Cory Tusar; netdev; Andrew
> Lunn
> Subject: [PATCHv3 net-next] net: fec: Ensure clocks are enabled while
> using mdio bus
> 
> When a switch is attached to the mdio bus, the mdio bus can be used while
> the interface is not open. If the IPG clock are not enabled, MDIO
> reads/writes will simply time out. So enable the clock before starting a
> transaction, and disable it afterwards. The CCF performs reference
> counting so the clock will only be disabled if there are no other users.
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> ---
> 
> v3:
>  Return the error code from clk_prepare_enable()
> v2:
>  Only enable the IGP clock.
> 
>  drivers/net/ethernet/freescale/fec_main.c | 21 +++++++++++++++++++--
>  1 file changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/fec_main.c
> b/drivers/net/ethernet/freescale/fec_main.c
> index bf4cf3fbb5f2..8d9b1fd175f7 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -65,6 +65,7 @@
> 
>  static void set_multicast_list(struct net_device *ndev);  static void
> fec_enet_itr_coal_init(struct net_device *ndev);
> +static int fec_enet_clk_enable(struct net_device *ndev, bool enable);
> 
>  #define DRIVER_NAME	"fec"
> 
> @@ -1764,6 +1765,11 @@ static int fec_enet_mdio_read(struct mii_bus *bus,
> int mii_id, int regnum)  {
>  	struct fec_enet_private *fep = bus->priv;
>  	unsigned long time_left;
> +	int ret;
> +
> +	ret = clk_prepare_enable(fep->clk_ipg);
> +	if (ret)
> +		return ret;
> 
>  	fep->mii_timeout = 0;
>  	init_completion(&fep->mdio_done);
> @@ -1779,11 +1785,14 @@ static int fec_enet_mdio_read(struct mii_bus *bus,
> int mii_id, int regnum)
>  	if (time_left == 0) {
>  		fep->mii_timeout = 1;
>  		netdev_err(fep->netdev, "MDIO read timeout\n");
> +		clk_disable_unprepare(fep->clk_ipg);
>  		return -ETIMEDOUT;
>  	}
> 
> -	/* return value */
> -	return FEC_MMFR_DATA(readl(fep->hwp + FEC_MII_DATA));
> +	ret = FEC_MMFR_DATA(readl(fep->hwp + FEC_MII_DATA));
> +	clk_disable_unprepare(fep->clk_ipg);
> +
> +	return ret;
>  }
> 

I suggest you use runtime pm to enable/disable clock for performance consideration. Not every time for mdio bus access needs to enable/disable clock.


>  static int fec_enet_mdio_write(struct mii_bus *bus, int mii_id, int
> regnum, @@ -1791,10 +1800,15 @@ static int fec_enet_mdio_write(struct
> mii_bus *bus, int mii_id, int regnum,  {
>  	struct fec_enet_private *fep = bus->priv;
>  	unsigned long time_left;
> +	int ret;
> 
>  	fep->mii_timeout = 0;
>  	init_completion(&fep->mdio_done);
> 
> +	ret = clk_prepare_enable(fep->clk_ipg);
> +	if (ret)
> +		return ret;
> +
>  	/* start a write op */
>  	writel(FEC_MMFR_ST | FEC_MMFR_OP_WRITE |
>  		FEC_MMFR_PA(mii_id) | FEC_MMFR_RA(regnum) | @@ -1807,9
> +1821,12 @@ static int fec_enet_mdio_write(struct mii_bus *bus, int
> mii_id, int regnum,
>  	if (time_left == 0) {
>  		fep->mii_timeout = 1;
>  		netdev_err(fep->netdev, "MDIO write timeout\n");
> +		clk_disable_unprepare(fep->clk_ipg);
>  		return -ETIMEDOUT;
>  	}
> 
> +	clk_disable_unprepare(fep->clk_ipg);
> +
>  	return 0;
>  }
> 
> --
> 2.1.4

Re: [PATCHv3 net-next] net: fec: Ensure clocks are enabled while using mdio bus

[Andrew Lunn]

> > int mii_id, int regnum)  {
> >  	struct fec_enet_private *fep = bus->priv;
> >  	unsigned long time_left;
> > +	int ret;
> > +
> > +	ret = clk_prepare_enable(fep->clk_ipg);
> > +	if (ret)
> > +		return ret;
> > 
> >  	fep->mii_timeout = 0;
> >  	init_completion(&fep->mdio_done);
> > @@ -1779,11 +1785,14 @@ static int fec_enet_mdio_read(struct mii_bus *bus,
> > int mii_id, int regnum)
> >  	if (time_left == 0) {
> >  		fep->mii_timeout = 1;
> >  		netdev_err(fep->netdev, "MDIO read timeout\n");
> > +		clk_disable_unprepare(fep->clk_ipg);
> >  		return -ETIMEDOUT;
> >  	}
> > 
> > -	/* return value */
> > -	return FEC_MMFR_DATA(readl(fep->hwp + FEC_MII_DATA));
> > +	ret = FEC_MMFR_DATA(readl(fep->hwp + FEC_MII_DATA));
> > +	clk_disable_unprepare(fep->clk_ipg);
> > +
> > +	return ret;
> >  }
> > 
> 

> I suggest you use runtime pm to enable/disable clock for performance
> consideration. Not every time for mdio bus access needs to
> enable/disable clock.

Can you explain that some more. When are you suggesting doing a
runtime enable/disable? Given the current DSA architecture, i would
probably do a runtime enable as soon as i lookup the mdio bus, and
never do a runtime disable. Also, how would you include this runtime
pm into the phy state machine which is going to be polling your mdio
bus around once per second for normal phys?

Florian, as Maintainer of the phy state machine, what do you think
about adding runtime PM to it?

  Thanks
	Andrew

RE: [PATCHv3 net-next] net: fec: Ensure clocks are enabled while using mdio bus

[Duan Andy]

From: Andrew Lunn <andrew@lunn.ch> Sent: Tuesday, June 23, 2015 10:52 AM
> To: Duan Fugang-B38611; Florian Fainelli
> Cc: David Miller; Cory Tusar; netdev
> Subject: Re: [PATCHv3 net-next] net: fec: Ensure clocks are enabled while
> using mdio bus
> 
> > > int mii_id, int regnum)  {
> > >  	struct fec_enet_private *fep = bus->priv;
> > >  	unsigned long time_left;
> > > +	int ret;
> > > +
> > > +	ret = clk_prepare_enable(fep->clk_ipg);
> > > +	if (ret)
> > > +		return ret;
> > >
> > >  	fep->mii_timeout = 0;
> > >  	init_completion(&fep->mdio_done);
> > > @@ -1779,11 +1785,14 @@ static int fec_enet_mdio_read(struct mii_bus
> > > *bus, int mii_id, int regnum)
> > >  	if (time_left == 0) {
> > >  		fep->mii_timeout = 1;
> > >  		netdev_err(fep->netdev, "MDIO read timeout\n");
> > > +		clk_disable_unprepare(fep->clk_ipg);
> > >  		return -ETIMEDOUT;
> > >  	}
> > >
> > > -	/* return value */
> > > -	return FEC_MMFR_DATA(readl(fep->hwp + FEC_MII_DATA));
> > > +	ret = FEC_MMFR_DATA(readl(fep->hwp + FEC_MII_DATA));
> > > +	clk_disable_unprepare(fep->clk_ipg);
> > > +
> > > +	return ret;
> > >  }
> > >
> >
> 
> > I suggest you use runtime pm to enable/disable clock for performance
> > consideration. Not every time for mdio bus access needs to
> > enable/disable clock.
> 
> Can you explain that some more. When are you suggesting doing a runtime
> enable/disable? Given the current DSA architecture, i would probably do a
> runtime enable as soon as i lookup the mdio bus, and never do a runtime
> disable. Also, how would you include this runtime pm into the phy state
> machine which is going to be polling your mdio bus around once per second
> for normal phys?
> 

You can do it like this:

#define FEC_MDIO_PM_TIMEOUT  100 /* ms */

static int fec_probe(struct platform_device *pdev)
{
	...
	pm_runtime_enable(&pdev->dev);
	pm_runtime_set_autosuspend_delay(&pdev->dev, FEC_MDIO_PM_TIMEOUT);
	pm_runtime_use_autosuspend(&pdev->dev);
	...
}


static int fec_enet_mdio_read(struct mii_bus *bus, int mii_id, int regnum)
{
	...
	pm_runtime_get_sync(dev);

	...

	pm_runtime_mark_last_busy(dev);
	pm_runtime_put_autosuspend(dev);
}


> Florian, as Maintainer of the phy state machine, what do you think about
> adding runtime PM to it?
> 
>   Thanks
> 	Andrew

Re: [PATCHv3 net-next] net: fec: Ensure clocks are enabled while using mdio bus

[Andrew Lunn]

On Tue, Jun 23, 2015 at 03:12:15AM +0000, Duan Andy wrote:
> From: Andrew Lunn <andrew@lunn.ch> Sent: Tuesday, June 23, 2015 10:52 AM
> > To: Duan Fugang-B38611; Florian Fainelli
> > Cc: David Miller; Cory Tusar; netdev
> > Subject: Re: [PATCHv3 net-next] net: fec: Ensure clocks are enabled while
> > using mdio bus
> > 
> > > > int mii_id, int regnum)  {
> > > >  	struct fec_enet_private *fep = bus->priv;
> > > >  	unsigned long time_left;
> > > > +	int ret;
> > > > +
> > > > +	ret = clk_prepare_enable(fep->clk_ipg);
> > > > +	if (ret)
> > > > +		return ret;
> > > >
> > > >  	fep->mii_timeout = 0;
> > > >  	init_completion(&fep->mdio_done);
> > > > @@ -1779,11 +1785,14 @@ static int fec_enet_mdio_read(struct mii_bus
> > > > *bus, int mii_id, int regnum)
> > > >  	if (time_left == 0) {
> > > >  		fep->mii_timeout = 1;
> > > >  		netdev_err(fep->netdev, "MDIO read timeout\n");
> > > > +		clk_disable_unprepare(fep->clk_ipg);
> > > >  		return -ETIMEDOUT;
> > > >  	}
> > > >
> > > > -	/* return value */
> > > > -	return FEC_MMFR_DATA(readl(fep->hwp + FEC_MII_DATA));
> > > > +	ret = FEC_MMFR_DATA(readl(fep->hwp + FEC_MII_DATA));
> > > > +	clk_disable_unprepare(fep->clk_ipg);
> > > > +
> > > > +	return ret;
> > > >  }
> > > >
> > >
> > 
> > > I suggest you use runtime pm to enable/disable clock for performance
> > > consideration. Not every time for mdio bus access needs to
> > > enable/disable clock.
> > 
> > Can you explain that some more. When are you suggesting doing a runtime
> > enable/disable? Given the current DSA architecture, i would probably do a
> > runtime enable as soon as i lookup the mdio bus, and never do a runtime
> > disable. Also, how would you include this runtime pm into the phy state
> > machine which is going to be polling your mdio bus around once per second
> > for normal phys?
> > 
> 
> You can do it like this:
> 
> #define FEC_MDIO_PM_TIMEOUT  100 /* ms */
> 
> static int fec_probe(struct platform_device *pdev)
> {
> 	...
> 	pm_runtime_enable(&pdev->dev);
> 	pm_runtime_set_autosuspend_delay(&pdev->dev, FEC_MDIO_PM_TIMEOUT);
> 	pm_runtime_use_autosuspend(&pdev->dev);
> 	...
> }
> 
> 
> static int fec_enet_mdio_read(struct mii_bus *bus, int mii_id, int regnum)
> {
> 	...
> 	pm_runtime_get_sync(dev);
> 
> 	...
> 
> 	pm_runtime_mark_last_busy(dev);
> 	pm_runtime_put_autosuspend(dev);
> }

Isn't this heavier than clk_prepare_enable()/clk_disable_unprepare()?

The clock is only going to be enabled/disabled before the interface is
opened(). Once it is open, the IGP clock is always running, so these
clock operations just become simple reference counts. But the runtime
operations you are suggesting will be doing lots of stuff after open
as well as before open.

   Andrew

Re: [PATCHv3 net-next] net: fec: Ensure clocks are enabled while using mdio bus

[Florian Fainelli]

2015-06-22 19:52 GMT-07:00 Andrew Lunn <andrew@lunn.ch>:
>> > int mii_id, int regnum)  {
>> >     struct fec_enet_private *fep = bus->priv;
>> >     unsigned long time_left;
>> > +   int ret;
>> > +
>> > +   ret = clk_prepare_enable(fep->clk_ipg);
>> > +   if (ret)
>> > +           return ret;
>> >
>> >     fep->mii_timeout = 0;
>> >     init_completion(&fep->mdio_done);
>> > @@ -1779,11 +1785,14 @@ static int fec_enet_mdio_read(struct mii_bus *bus,
>> > int mii_id, int regnum)
>> >     if (time_left == 0) {
>> >             fep->mii_timeout = 1;
>> >             netdev_err(fep->netdev, "MDIO read timeout\n");
>> > +           clk_disable_unprepare(fep->clk_ipg);
>> >             return -ETIMEDOUT;
>> >     }
>> >
>> > -   /* return value */
>> > -   return FEC_MMFR_DATA(readl(fep->hwp + FEC_MII_DATA));
>> > +   ret = FEC_MMFR_DATA(readl(fep->hwp + FEC_MII_DATA));
>> > +   clk_disable_unprepare(fep->clk_ipg);
>> > +
>> > +   return ret;
>> >  }
>> >
>>
>
>> I suggest you use runtime pm to enable/disable clock for performance
>> consideration. Not every time for mdio bus access needs to
>> enable/disable clock.
>
> Can you explain that some more. When are you suggesting doing a
> runtime enable/disable? Given the current DSA architecture, i would
> probably do a runtime enable as soon as i lookup the mdio bus, and
> never do a runtime disable. Also, how would you include this runtime
> pm into the phy state machine which is going to be polling your mdio
> bus around once per second for normal phys?

I believe the sh_eth driver is an in-tree example of a driver doing
runtime PM for its MDIO bus controller implementation.

>
> Florian, as Maintainer of the phy state machine, what do you think
> about adding runtime PM to it?

I have no objection to adding runtime PM awareness into the PHY
library, runtime PM should work well here since we are doing host
initiated reads, at least for polled PHYs. For interrupt driven PHYs,
I suppose we could also get something to work, although one needs to
be more careful. One potential issue is that runtime PM does not seem
to have some sort of latency built-in into it, so you do not really
know how long it takes to transition from low-power to functional
state where you can issue a MDIO read/write  with success.

You could argue that as long as the sum of the times needed to
wake-up, perform the operation and go back to sleep is below the
polling frequency, you are safe, but a) this does not work for all
device if we ever lower the poll frequency and b) this starts putting
near real-time requirements on doing all of these operations.

I would suspect that unless the clock feeding the MDIO bus controller
feeds over power hungry digital logic, you would get most power
savings from enabling link power management features such as EEE.
Anything else dealing with digital logic might just be noise compared
to doing this. Technically, even between an Ethernet switch and the
CPU port you should be able to do it, provided that the switch
supports it.

My 2 cents.
-- 
Florian

RE: [PATCHv3 net-next] net: fec: Ensure clocks are enabled while using mdio bus

[Duan Andy]

From: Andrew Lunn <andrew@lunn.ch> Sent: Tuesday, June 23, 2015 11:44 AM
> To: Duan Fugang-B38611
> Cc: Florian Fainelli; David Miller; Cory Tusar; netdev
> Subject: Re: [PATCHv3 net-next] net: fec: Ensure clocks are enabled while
> using mdio bus
> 
> On Tue, Jun 23, 2015 at 03:12:15AM +0000, Duan Andy wrote:
> > From: Andrew Lunn <andrew@lunn.ch> Sent: Tuesday, June 23, 2015 10:52
> > AM
> > > To: Duan Fugang-B38611; Florian Fainelli
> > > Cc: David Miller; Cory Tusar; netdev
> > > Subject: Re: [PATCHv3 net-next] net: fec: Ensure clocks are enabled
> > > while using mdio bus
> > >
> > > > > int mii_id, int regnum)  {
> > > > >  	struct fec_enet_private *fep = bus->priv;
> > > > >  	unsigned long time_left;
> > > > > +	int ret;
> > > > > +
> > > > > +	ret = clk_prepare_enable(fep->clk_ipg);
> > > > > +	if (ret)
> > > > > +		return ret;
> > > > >
> > > > >  	fep->mii_timeout = 0;
> > > > >  	init_completion(&fep->mdio_done); @@ -1779,11 +1785,14 @@
> > > > > static int fec_enet_mdio_read(struct mii_bus *bus, int mii_id,
> > > > > int regnum)
> > > > >  	if (time_left == 0) {
> > > > >  		fep->mii_timeout = 1;
> > > > >  		netdev_err(fep->netdev, "MDIO read timeout\n");
> > > > > +		clk_disable_unprepare(fep->clk_ipg);
> > > > >  		return -ETIMEDOUT;
> > > > >  	}
> > > > >
> > > > > -	/* return value */
> > > > > -	return FEC_MMFR_DATA(readl(fep->hwp + FEC_MII_DATA));
> > > > > +	ret = FEC_MMFR_DATA(readl(fep->hwp + FEC_MII_DATA));
> > > > > +	clk_disable_unprepare(fep->clk_ipg);
> > > > > +
> > > > > +	return ret;
> > > > >  }
> > > > >
> > > >
> > >
> > > > I suggest you use runtime pm to enable/disable clock for
> > > > performance consideration. Not every time for mdio bus access
> > > > needs to enable/disable clock.
> > >
> > > Can you explain that some more. When are you suggesting doing a
> > > runtime enable/disable? Given the current DSA architecture, i would
> > > probably do a runtime enable as soon as i lookup the mdio bus, and
> > > never do a runtime disable. Also, how would you include this runtime
> > > pm into the phy state machine which is going to be polling your mdio
> > > bus around once per second for normal phys?
> > >
> >
> > You can do it like this:
> >
> > #define FEC_MDIO_PM_TIMEOUT  100 /* ms */
> >
> > static int fec_probe(struct platform_device *pdev) {
> > 	...
> > 	pm_runtime_enable(&pdev->dev);
> > 	pm_runtime_set_autosuspend_delay(&pdev->dev, FEC_MDIO_PM_TIMEOUT);
> > 	pm_runtime_use_autosuspend(&pdev->dev);
> > 	...
> > }
> >
> >
> > static int fec_enet_mdio_read(struct mii_bus *bus, int mii_id, int
> > regnum) {
> > 	...
> > 	pm_runtime_get_sync(dev);
> >
> > 	...
> >
> > 	pm_runtime_mark_last_busy(dev);
> > 	pm_runtime_put_autosuspend(dev);
> > }
> 
> Isn't this heavier than clk_prepare_enable()/clk_disable_unprepare()?
> 
No, I don't think so. Move clock enable/disable() to runtime suspend/resume is similar to your patch, but
Can reduce the times during mii bus stress busy access like mii stress test case.


static int fec_runtime_suspend(struct device *dev)
{
	...
	clk_disable(fep->ipg_clk);

	return 0;
}

static int fec_runtime_resume(struct device *dev)
{
	return clk_enable(fep->ipg_clk);
}

> The clock is only going to be enabled/disabled before the interface is
> opened(). Once it is open, the IGP clock is always running, so these
> clock operations just become simple reference counts. But the runtime
> operations you are suggesting will be doing lots of stuff after open as
> well as before open.
> 
No, no any lots of stuff comparing to your current implementation.

>    Andrew