| Message ID | 20170606220017.24807-2-manoj.iyer@canonical.com |
|---|---|
| State | New |
| Headers | show |
On Tue, Jun 06, 2017 at 05:00:17PM -0500, Manoj Iyer wrote: > From: Timur Tabi <timur@codeaurora.org> > > Use software polling (PHY_POLL) to check for link state changes instead > of relying on the EMAC's hardware polling feature. Some PHY drivers > are unable to get a functioning link because the HW polling is not > robust enough. > > The EMAC is able to poll the PHY on the MDIO bus looking for link state > changes (via the Link Status bit in the Status Register at address 0x1). > When the link state changes, the EMAC triggers an interrupt and tells the > driver what the new state is. The feature eliminates the need for > software to poll the MDIO bus. > > Unfortunately, this feature is incompatible with phylib, because it > ignores everything that the PHY core and PHY drivers are trying to do. > In particular: > > 1. It assumes a compatible register set, so PHYs with different registers > may not work. > > 2. It doesn't allow for hardware errata that have work-arounds implemented > in the PHY driver. > > 3. It doesn't support multiple register pages. If the PHY core switches > the register set to another page, the EMAC won't know the page has > changed and will still attempt to read the same PHY register. > > 4. It only checks the copper side of the link, not the SGMII side. Some > PHY drivers (e.g. at803x) may also check the SGMII side, and > report the link as not ready during autonegotiation if the SGMII link > is still down. Phylib then waits for another interrupt to query > the PHY again, but the EMAC won't send another interrupt because it > thinks the link is up. > > BugLink: https://bugs.launchpad.net/bugs/1696143 > > Cc: stable@vger.kernel.org # 4.11.x > Tested-by: Manoj Iyer <manoj.iyer@canonical.com> > Signed-off-by: Timur Tabi <timur@codeaurora.org> > Signed-off-by: David S. Miller <davem@davemloft.net> > (cherry picked from commit 246096690be0742d9bb5f3456d2cb95b68f7b46d) > Signed-off-by: Manoj Iyer <manoj.iyer@canonical.com> Clean cherry pick, isolated to a single driver, and also an upstream stable patch. Acked-by: Seth Forshee <seth.forshee@canonical.com> Note that this has already been applied to artful/master-next as part of 4.11.5.
On 07.06.2017 00:00, Manoj Iyer wrote: > From: Timur Tabi <timur@codeaurora.org> > > Use software polling (PHY_POLL) to check for link state changes instead > of relying on the EMAC's hardware polling feature. Some PHY drivers > are unable to get a functioning link because the HW polling is not > robust enough. > > The EMAC is able to poll the PHY on the MDIO bus looking for link state > changes (via the Link Status bit in the Status Register at address 0x1). > When the link state changes, the EMAC triggers an interrupt and tells the > driver what the new state is. The feature eliminates the need for > software to poll the MDIO bus. > > Unfortunately, this feature is incompatible with phylib, because it > ignores everything that the PHY core and PHY drivers are trying to do. > In particular: > > 1. It assumes a compatible register set, so PHYs with different registers > may not work. > > 2. It doesn't allow for hardware errata that have work-arounds implemented > in the PHY driver. > > 3. It doesn't support multiple register pages. If the PHY core switches > the register set to another page, the EMAC won't know the page has > changed and will still attempt to read the same PHY register. > > 4. It only checks the copper side of the link, not the SGMII side. Some > PHY drivers (e.g. at803x) may also check the SGMII side, and > report the link as not ready during autonegotiation if the SGMII link > is still down. Phylib then waits for another interrupt to query > the PHY again, but the EMAC won't send another interrupt because it > thinks the link is up. > > BugLink: https://bugs.launchpad.net/bugs/1696143 > > Cc: stable@vger.kernel.org # 4.11.x > Tested-by: Manoj Iyer <manoj.iyer@canonical.com> > Signed-off-by: Timur Tabi <timur@codeaurora.org> > Signed-off-by: David S. Miller <davem@davemloft.net> > (cherry picked from commit 246096690be0742d9bb5f3456d2cb95b68f7b46d) > Signed-off-by: Manoj Iyer <manoj.iyer@canonical.com> Acked-by: Stefan Bader <stefan.bader@canonical.com> > --- Appears to be upstream with the same sha1 (at least by now. -Stefan > drivers/net/ethernet/qualcomm/emac/emac-mac.c | 2 +- > drivers/net/ethernet/qualcomm/emac/emac-phy.c | 75 ++------------------------- > drivers/net/ethernet/qualcomm/emac/emac.c | 22 +------- > 3 files changed, 6 insertions(+), 93 deletions(-) > > diff --git a/drivers/net/ethernet/qualcomm/emac/emac-mac.c b/drivers/net/ethernet/qualcomm/emac/emac-mac.c > index 824aa97e055d..ee6f84efe942 100644 > --- a/drivers/net/ethernet/qualcomm/emac/emac-mac.c > +++ b/drivers/net/ethernet/qualcomm/emac/emac-mac.c > @@ -931,7 +931,7 @@ int emac_mac_up(struct emac_adapter *adpt) > emac_mac_config(adpt); > emac_mac_rx_descs_refill(adpt, &adpt->rx_q); > > - adpt->phydev->irq = PHY_IGNORE_INTERRUPT; > + adpt->phydev->irq = PHY_POLL; > ret = phy_connect_direct(netdev, adpt->phydev, emac_adjust_link, > PHY_INTERFACE_MODE_SGMII); > if (ret) { > diff --git a/drivers/net/ethernet/qualcomm/emac/emac-phy.c b/drivers/net/ethernet/qualcomm/emac/emac-phy.c > index 441c19366489..18461fcb9815 100644 > --- a/drivers/net/ethernet/qualcomm/emac/emac-phy.c > +++ b/drivers/net/ethernet/qualcomm/emac/emac-phy.c > @@ -13,15 +13,11 @@ > /* Qualcomm Technologies, Inc. EMAC PHY Controller driver. > */ > > -#include <linux/module.h> > -#include <linux/of.h> > -#include <linux/of_net.h> > #include <linux/of_mdio.h> > #include <linux/phy.h> > #include <linux/iopoll.h> > #include <linux/acpi.h> > #include "emac.h" > -#include "emac-mac.h" > > /* EMAC base register offsets */ > #define EMAC_MDIO_CTRL 0x001414 > @@ -52,62 +48,10 @@ > > #define MDIO_WAIT_TIMES 1000 > > -#define EMAC_LINK_SPEED_DEFAULT (\ > - EMAC_LINK_SPEED_10_HALF |\ > - EMAC_LINK_SPEED_10_FULL |\ > - EMAC_LINK_SPEED_100_HALF |\ > - EMAC_LINK_SPEED_100_FULL |\ > - EMAC_LINK_SPEED_1GB_FULL) > - > -/** > - * emac_phy_mdio_autopoll_disable() - disable mdio autopoll > - * @adpt: the emac adapter > - * > - * The autopoll feature takes over the MDIO bus. In order for > - * the PHY driver to be able to talk to the PHY over the MDIO > - * bus, we need to temporarily disable the autopoll feature. > - */ > -static int emac_phy_mdio_autopoll_disable(struct emac_adapter *adpt) > -{ > - u32 val; > - > - /* disable autopoll */ > - emac_reg_update32(adpt->base + EMAC_MDIO_CTRL, MDIO_AP_EN, 0); > - > - /* wait for any mdio polling to complete */ > - if (!readl_poll_timeout(adpt->base + EMAC_MDIO_CTRL, val, > - !(val & MDIO_BUSY), 100, MDIO_WAIT_TIMES * 100)) > - return 0; > - > - /* failed to disable; ensure it is enabled before returning */ > - emac_reg_update32(adpt->base + EMAC_MDIO_CTRL, 0, MDIO_AP_EN); > - > - return -EBUSY; > -} > - > -/** > - * emac_phy_mdio_autopoll_disable() - disable mdio autopoll > - * @adpt: the emac adapter > - * > - * The EMAC has the ability to poll the external PHY on the MDIO > - * bus for link state changes. This eliminates the need for the > - * driver to poll the phy. If if the link state does change, > - * the EMAC issues an interrupt on behalf of the PHY. > - */ > -static void emac_phy_mdio_autopoll_enable(struct emac_adapter *adpt) > -{ > - emac_reg_update32(adpt->base + EMAC_MDIO_CTRL, 0, MDIO_AP_EN); > -} > - > static int emac_mdio_read(struct mii_bus *bus, int addr, int regnum) > { > struct emac_adapter *adpt = bus->priv; > u32 reg; > - int ret; > - > - ret = emac_phy_mdio_autopoll_disable(adpt); > - if (ret) > - return ret; > > emac_reg_update32(adpt->base + EMAC_PHY_STS, PHY_ADDR_BMSK, > (addr << PHY_ADDR_SHFT)); > @@ -122,24 +66,15 @@ static int emac_mdio_read(struct mii_bus *bus, int addr, int regnum) > if (readl_poll_timeout(adpt->base + EMAC_MDIO_CTRL, reg, > !(reg & (MDIO_START | MDIO_BUSY)), > 100, MDIO_WAIT_TIMES * 100)) > - ret = -EIO; > - else > - ret = (reg >> MDIO_DATA_SHFT) & MDIO_DATA_BMSK; > + return -EIO; > > - emac_phy_mdio_autopoll_enable(adpt); > - > - return ret; > + return (reg >> MDIO_DATA_SHFT) & MDIO_DATA_BMSK; > } > > static int emac_mdio_write(struct mii_bus *bus, int addr, int regnum, u16 val) > { > struct emac_adapter *adpt = bus->priv; > u32 reg; > - int ret; > - > - ret = emac_phy_mdio_autopoll_disable(adpt); > - if (ret) > - return ret; > > emac_reg_update32(adpt->base + EMAC_PHY_STS, PHY_ADDR_BMSK, > (addr << PHY_ADDR_SHFT)); > @@ -155,11 +90,9 @@ static int emac_mdio_write(struct mii_bus *bus, int addr, int regnum, u16 val) > if (readl_poll_timeout(adpt->base + EMAC_MDIO_CTRL, reg, > !(reg & (MDIO_START | MDIO_BUSY)), 100, > MDIO_WAIT_TIMES * 100)) > - ret = -EIO; > + return -EIO; > > - emac_phy_mdio_autopoll_enable(adpt); > - > - return ret; > + return 0; > } > > /* Configure the MDIO bus and connect the external PHY */ > diff --git a/drivers/net/ethernet/qualcomm/emac/emac.c b/drivers/net/ethernet/qualcomm/emac/emac.c > index 350d2362ddae..977aa31b6371 100644 > --- a/drivers/net/ethernet/qualcomm/emac/emac.c > +++ b/drivers/net/ethernet/qualcomm/emac/emac.c > @@ -50,19 +50,7 @@ > #define DMAR_DLY_CNT_DEF 15 > #define DMAW_DLY_CNT_DEF 4 > > -#define IMR_NORMAL_MASK (\ > - ISR_ERROR |\ > - ISR_GPHY_LINK |\ > - ISR_TX_PKT |\ > - GPHY_WAKEUP_INT) > - > -#define IMR_EXTENDED_MASK (\ > - SW_MAN_INT |\ > - ISR_OVER |\ > - ISR_ERROR |\ > - ISR_GPHY_LINK |\ > - ISR_TX_PKT |\ > - GPHY_WAKEUP_INT) > +#define IMR_NORMAL_MASK (ISR_ERROR | ISR_OVER | ISR_TX_PKT) > > #define ISR_TX_PKT (\ > TX_PKT_INT |\ > @@ -70,10 +58,6 @@ > TX_PKT_INT2 |\ > TX_PKT_INT3) > > -#define ISR_GPHY_LINK (\ > - GPHY_LINK_UP_INT |\ > - GPHY_LINK_DOWN_INT) > - > #define ISR_OVER (\ > RFD0_UR_INT |\ > RFD1_UR_INT |\ > @@ -187,10 +171,6 @@ irqreturn_t emac_isr(int _irq, void *data) > if (status & ISR_OVER) > net_warn_ratelimited("warning: TX/RX overflow\n"); > > - /* link event */ > - if (status & ISR_GPHY_LINK) > - phy_mac_interrupt(adpt->phydev, !!(status & GPHY_LINK_UP_INT)); > - > exit: > /* enable the interrupt */ > writel(irq->mask, adpt->base + EMAC_INT_MASK); >
Applied to Zesty master-next. Thanks, -Stefan
diff --git a/drivers/net/ethernet/qualcomm/emac/emac-mac.c b/drivers/net/ethernet/qualcomm/emac/emac-mac.c index 824aa97e055d..ee6f84efe942 100644 --- a/drivers/net/ethernet/qualcomm/emac/emac-mac.c +++ b/drivers/net/ethernet/qualcomm/emac/emac-mac.c @@ -931,7 +931,7 @@ int emac_mac_up(struct emac_adapter *adpt) emac_mac_config(adpt); emac_mac_rx_descs_refill(adpt, &adpt->rx_q); - adpt->phydev->irq = PHY_IGNORE_INTERRUPT; + adpt->phydev->irq = PHY_POLL; ret = phy_connect_direct(netdev, adpt->phydev, emac_adjust_link, PHY_INTERFACE_MODE_SGMII); if (ret) { diff --git a/drivers/net/ethernet/qualcomm/emac/emac-phy.c b/drivers/net/ethernet/qualcomm/emac/emac-phy.c index 441c19366489..18461fcb9815 100644 --- a/drivers/net/ethernet/qualcomm/emac/emac-phy.c +++ b/drivers/net/ethernet/qualcomm/emac/emac-phy.c @@ -13,15 +13,11 @@ /* Qualcomm Technologies, Inc. EMAC PHY Controller driver. */ -#include <linux/module.h> -#include <linux/of.h> -#include <linux/of_net.h> #include <linux/of_mdio.h> #include <linux/phy.h> #include <linux/iopoll.h> #include <linux/acpi.h> #include "emac.h" -#include "emac-mac.h" /* EMAC base register offsets */ #define EMAC_MDIO_CTRL 0x001414 @@ -52,62 +48,10 @@ #define MDIO_WAIT_TIMES 1000 -#define EMAC_LINK_SPEED_DEFAULT (\ - EMAC_LINK_SPEED_10_HALF |\ - EMAC_LINK_SPEED_10_FULL |\ - EMAC_LINK_SPEED_100_HALF |\ - EMAC_LINK_SPEED_100_FULL |\ - EMAC_LINK_SPEED_1GB_FULL) - -/** - * emac_phy_mdio_autopoll_disable() - disable mdio autopoll - * @adpt: the emac adapter - * - * The autopoll feature takes over the MDIO bus. In order for - * the PHY driver to be able to talk to the PHY over the MDIO - * bus, we need to temporarily disable the autopoll feature. - */ -static int emac_phy_mdio_autopoll_disable(struct emac_adapter *adpt) -{ - u32 val; - - /* disable autopoll */ - emac_reg_update32(adpt->base + EMAC_MDIO_CTRL, MDIO_AP_EN, 0); - - /* wait for any mdio polling to complete */ - if (!readl_poll_timeout(adpt->base + EMAC_MDIO_CTRL, val, - !(val & MDIO_BUSY), 100, MDIO_WAIT_TIMES * 100)) - return 0; - - /* failed to disable; ensure it is enabled before returning */ - emac_reg_update32(adpt->base + EMAC_MDIO_CTRL, 0, MDIO_AP_EN); - - return -EBUSY; -} - -/** - * emac_phy_mdio_autopoll_disable() - disable mdio autopoll - * @adpt: the emac adapter - * - * The EMAC has the ability to poll the external PHY on the MDIO - * bus for link state changes. This eliminates the need for the - * driver to poll the phy. If if the link state does change, - * the EMAC issues an interrupt on behalf of the PHY. - */ -static void emac_phy_mdio_autopoll_enable(struct emac_adapter *adpt) -{ - emac_reg_update32(adpt->base + EMAC_MDIO_CTRL, 0, MDIO_AP_EN); -} - static int emac_mdio_read(struct mii_bus *bus, int addr, int regnum) { struct emac_adapter *adpt = bus->priv; u32 reg; - int ret; - - ret = emac_phy_mdio_autopoll_disable(adpt); - if (ret) - return ret; emac_reg_update32(adpt->base + EMAC_PHY_STS, PHY_ADDR_BMSK, (addr << PHY_ADDR_SHFT)); @@ -122,24 +66,15 @@ static int emac_mdio_read(struct mii_bus *bus, int addr, int regnum) if (readl_poll_timeout(adpt->base + EMAC_MDIO_CTRL, reg, !(reg & (MDIO_START | MDIO_BUSY)), 100, MDIO_WAIT_TIMES * 100)) - ret = -EIO; - else - ret = (reg >> MDIO_DATA_SHFT) & MDIO_DATA_BMSK; + return -EIO; - emac_phy_mdio_autopoll_enable(adpt); - - return ret; + return (reg >> MDIO_DATA_SHFT) & MDIO_DATA_BMSK; } static int emac_mdio_write(struct mii_bus *bus, int addr, int regnum, u16 val) { struct emac_adapter *adpt = bus->priv; u32 reg; - int ret; - - ret = emac_phy_mdio_autopoll_disable(adpt); - if (ret) - return ret; emac_reg_update32(adpt->base + EMAC_PHY_STS, PHY_ADDR_BMSK, (addr << PHY_ADDR_SHFT)); @@ -155,11 +90,9 @@ static int emac_mdio_write(struct mii_bus *bus, int addr, int regnum, u16 val) if (readl_poll_timeout(adpt->base + EMAC_MDIO_CTRL, reg, !(reg & (MDIO_START | MDIO_BUSY)), 100, MDIO_WAIT_TIMES * 100)) - ret = -EIO; + return -EIO; - emac_phy_mdio_autopoll_enable(adpt); - - return ret; + return 0; } /* Configure the MDIO bus and connect the external PHY */ diff --git a/drivers/net/ethernet/qualcomm/emac/emac.c b/drivers/net/ethernet/qualcomm/emac/emac.c index 350d2362ddae..977aa31b6371 100644 --- a/drivers/net/ethernet/qualcomm/emac/emac.c +++ b/drivers/net/ethernet/qualcomm/emac/emac.c @@ -50,19 +50,7 @@ #define DMAR_DLY_CNT_DEF 15 #define DMAW_DLY_CNT_DEF 4 -#define IMR_NORMAL_MASK (\ - ISR_ERROR |\ - ISR_GPHY_LINK |\ - ISR_TX_PKT |\ - GPHY_WAKEUP_INT) - -#define IMR_EXTENDED_MASK (\ - SW_MAN_INT |\ - ISR_OVER |\ - ISR_ERROR |\ - ISR_GPHY_LINK |\ - ISR_TX_PKT |\ - GPHY_WAKEUP_INT) +#define IMR_NORMAL_MASK (ISR_ERROR | ISR_OVER | ISR_TX_PKT) #define ISR_TX_PKT (\ TX_PKT_INT |\ @@ -70,10 +58,6 @@ TX_PKT_INT2 |\ TX_PKT_INT3) -#define ISR_GPHY_LINK (\ - GPHY_LINK_UP_INT |\ - GPHY_LINK_DOWN_INT) - #define ISR_OVER (\ RFD0_UR_INT |\ RFD1_UR_INT |\ @@ -187,10 +171,6 @@ irqreturn_t emac_isr(int _irq, void *data) if (status & ISR_OVER) net_warn_ratelimited("warning: TX/RX overflow\n"); - /* link event */ - if (status & ISR_GPHY_LINK) - phy_mac_interrupt(adpt->phydev, !!(status & GPHY_LINK_UP_INT)); - exit: /* enable the interrupt */ writel(irq->mask, adpt->base + EMAC_INT_MASK);