| Message ID | 1434123545-13549-1-git-send-email-andrew@lunn.ch |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Andrew Lunn <andrew@lunn.ch> Sent: Friday, June 12, 2015 11:39 PM > To: David Miller > Cc: Duan Fugang-B38611; Cory Tusar; netdev; Andrew Lunn > Subject: [PATCH] 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 clocks are not enabled, MDIO > reads/writes will simply time out. So enable the clocks before starting a > transaction, and disable them afterwards. The CCF performs reference > counting so the clocks will only be disabled if there are no other users. > > Signed-off-by: Andrew Lunn <andrew@lunn.ch> > --- NAK the patch. i.MX series MDIO bus is a part of ENET controller. If the eth interface is not open, all clocks including MDIO bus clock are not enabled for power saving. In general, if you want to use mdio bus net interface must be running status. > 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..122186b90cdb 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 = fec_enet_clk_enable(fep->netdev, true); > + if (ret) > + return 0xffff; > > 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"); > + fec_enet_clk_enable(fep->netdev, false); > return -ETIMEDOUT; > } > > - /* return value */ > - return FEC_MMFR_DATA(readl(fep->hwp + FEC_MII_DATA)); > + ret = FEC_MMFR_DATA(readl(fep->hwp + FEC_MII_DATA)); > + fec_enet_clk_enable(fep->netdev, false); > + > + 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 = fec_enet_clk_enable(fep->netdev, true); > + if (ret) > + netdev_err(fep->netdev, "Unable to enable clks\n"); > + > /* 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"); > + fec_enet_clk_enable(fep->netdev, false); > return -ETIMEDOUT; > } > > + fec_enet_clk_enable(fep->netdev, false); > + > return 0; > } > > -- > 2.1.4 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Jun 14, 2015 at 08:07:12AM +0000, Duan Andy wrote: > From: Andrew Lunn <andrew@lunn.ch> Sent: Friday, June 12, 2015 11:39 PM > > To: David Miller > > Cc: Duan Fugang-B38611; Cory Tusar; netdev; Andrew Lunn > > Subject: [PATCH] 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 clocks are not enabled, MDIO > > reads/writes will simply time out. So enable the clocks before starting a > > transaction, and disable them afterwards. The CCF performs reference > > counting so the clocks will only be disabled if there are no other users. > > > > Signed-off-by: Andrew Lunn <andrew@lunn.ch> > > --- > > NAK the patch. > i.MX series MDIO bus is a part of ENET controller. If the eth > interface is not open, all clocks including MDIO bus clock are not > enabled for power saving. Where do you see a power saving regression in this code? It is not as if i just unconditionally turn the clocks on. As the comment says, at the start of an MDIO transaction, the clocks are enabled. At the end of a transaction, they are disabled again. If you don't have a switch connected, there will be no transactions, hence no change to power savings. > In general, if you want to use mdio bus net interface must be > running status. This is not true for a number of Ethernet devices. All those currently used with DSA allow MDIO transactions at any time. Andrew -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Le 06/14/15 07:41, Andrew Lunn a écrit : > On Sun, Jun 14, 2015 at 08:07:12AM +0000, Duan Andy wrote: >> From: Andrew Lunn <andrew@lunn.ch> Sent: Friday, June 12, 2015 11:39 PM >>> To: David Miller >>> Cc: Duan Fugang-B38611; Cory Tusar; netdev; Andrew Lunn >>> Subject: [PATCH] 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 clocks are not enabled, MDIO >>> reads/writes will simply time out. So enable the clocks before starting a >>> transaction, and disable them afterwards. The CCF performs reference >>> counting so the clocks will only be disabled if there are no other users. >>> >>> Signed-off-by: Andrew Lunn <andrew@lunn.ch> >>> --- >> >> NAK the patch. > >> i.MX series MDIO bus is a part of ENET controller. If the eth >> interface is not open, all clocks including MDIO bus clock are not >> enabled for power saving. Which is fine, but is an assumption you can only make when your ENET controller is connected to an Ethernet PHY, with a switch, things get a little different see below. > > Where do you see a power saving regression in this code? It is not as > if i just unconditionally turn the clocks on. As the comment says, at > the start of an MDIO transaction, the clocks are enabled. At the end > of a transaction, they are disabled again. If you don't have a switch > connected, there will be no transactions, hence no change to power > savings. > >> In general, if you want to use mdio bus net interface must be >> running status. > > This is not true for a number of Ethernet devices. All those currently > used with DSA allow MDIO transactions at any time. Right, the typical sequence looks like this: - ethernet device is probed and registered first, for power savings purposes, the probe function might turn off clocks since we do not know whether the interface will be used at all, so until ndo_open is called, such clocks can be disabled - DSA is probed second, and probes the switches using MDIO bus accesses, and if a switch is found, will configure it via MDIO bus reads/writes, by then, the network interface is guaranteed to be down - last, you could disable the parent network device, but still issue MDIO reads/writes towards the switch to collect statistics, make it run in an unmanaged mode with the CPU interface down/powered off Since clocks are reference counted, I really do not see much problem with Andrew's approach here. If you are running without a switch and just a PHY, you will get an occasional clock reference count > 1 during MDIO reads/writes, and past your ndo_close() if the clock reference count is 1, aka still turned on, then that means you are not using the PHY library properly and you have dangling MDIO accesses.
From: Florian Fainelli <f.fainelli@gmail.com> Sent: Monday, June 15, 2015 3:32 AM > To: Andrew Lunn; Duan Fugang-B38611 > Cc: David Miller; Cory Tusar; netdev > Subject: Re: [PATCH] net: fec: Ensure clocks are enabled while using mdio > bus > > Le 06/14/15 07:41, Andrew Lunn a écrit : > > On Sun, Jun 14, 2015 at 08:07:12AM +0000, Duan Andy wrote: > >> From: Andrew Lunn <andrew@lunn.ch> Sent: Friday, June 12, 2015 11:39 > >> PM > >>> To: David Miller > >>> Cc: Duan Fugang-B38611; Cory Tusar; netdev; Andrew Lunn > >>> Subject: [PATCH] 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 clocks are not enabled, MDIO > >>> reads/writes will simply time out. So enable the clocks before > >>> starting a transaction, and disable them afterwards. The CCF > >>> performs reference counting so the clocks will only be disabled if > there are no other users. > >>> > >>> Signed-off-by: Andrew Lunn <andrew@lunn.ch> > >>> --- > >> > >> NAK the patch. > > > >> i.MX series MDIO bus is a part of ENET controller. If the eth > >> interface is not open, all clocks including MDIO bus clock are not > >> enabled for power saving. > > Which is fine, but is an assumption you can only make when your ENET > controller is connected to an Ethernet PHY, with a switch, things get a > little different see below. > > > > > Where do you see a power saving regression in this code? It is not as > > if i just unconditionally turn the clocks on. As the comment says, at > > the start of an MDIO transaction, the clocks are enabled. At the end > > of a transaction, they are disabled again. If you don't have a switch > > connected, there will be no transactions, hence no change to power > > savings. > > > >> In general, if you want to use mdio bus net interface must be running > >> status. > > > > This is not true for a number of Ethernet devices. All those currently > > used with DSA allow MDIO transactions at any time. > As I said, the MDIO bus is just one function of ENET controller, not dependent HW/IP. > Right, the typical sequence looks like this: > > - ethernet device is probed and registered first, for power savings > purposes, the probe function might turn off clocks since we do not know > whether the interface will be used at all, so until ndo_open is called, > such clocks can be disabled > > - DSA is probed second, and probes the switches using MDIO bus accesses, > and if a switch is found, will configure it via MDIO bus reads/writes, by > then, the network interface is guaranteed to be down > > - last, you could disable the parent network device, but still issue MDIO > reads/writes towards the switch to collect statistics, make it run in an > unmanaged mode with the CPU interface down/powered off > > Since clocks are reference counted, I really do not see much problem with > Andrew's approach here. > > If you are running without a switch and just a PHY, you will get an > occasional clock reference count > 1 during MDIO reads/writes, and past > your ndo_close() if the clock reference count is 1, aka still turned on, > then that means you are not using the PHY library properly and you have > dangling MDIO accesses. > -- > Florian Thanks for your detail explain why do you want the change. If so, it just need to enable ENET ipg clock for your cases, not all enet clocks. Ipg clock is for MDIO bus read/write, for ENET registers accessing. (fep->clk_ipg) Which switch phy do you use ? I tried BCM54220 switch on the driver that works fine.
Hi Andy > If so, it just need to enable ENET ipg clock for your cases, not all enet clocks. > Ipg clock is for MDIO bus read/write, for ENET registers accessing. (fep->clk_ipg) O.K, thanks. I will respin the patch with just the ipg clock. > Which switch phy do you use ? I tried BCM54220 switch on the driver that works fine. I'm using two mv88e6352 and a mv88e6185 in cascade. Andrew -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c index bf4cf3fbb5f2..122186b90cdb 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 = fec_enet_clk_enable(fep->netdev, true); + if (ret) + return 0xffff; 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"); + fec_enet_clk_enable(fep->netdev, false); return -ETIMEDOUT; } - /* return value */ - return FEC_MMFR_DATA(readl(fep->hwp + FEC_MII_DATA)); + ret = FEC_MMFR_DATA(readl(fep->hwp + FEC_MII_DATA)); + fec_enet_clk_enable(fep->netdev, false); + + 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 = fec_enet_clk_enable(fep->netdev, true); + if (ret) + netdev_err(fep->netdev, "Unable to enable clks\n"); + /* 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"); + fec_enet_clk_enable(fep->netdev, false); return -ETIMEDOUT; } + fec_enet_clk_enable(fep->netdev, false); + 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 clocks are not enabled, MDIO reads/writes will simply time out. So enable the clocks before starting a transaction, and disable them afterwards. The CCF performs reference counting so the clocks will only be disabled if there are no other users. Signed-off-by: Andrew Lunn <andrew@lunn.ch> --- drivers/net/ethernet/freescale/fec_main.c | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-)