| Message ID | 1434825509-11241-1-git-send-email-andrew@lunn.ch |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in
> > 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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in
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.
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in
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; }
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(-)