ACK/cmnt: [PATCH] net: qcom/emac: do not use hardware mdio automatic polling
Stefan Bader
stefan.bader at canonical.com
Wed Jun 21 07:50:13 UTC 2017
On 07.06.2017 00:00, Manoj Iyer wrote:
> From: Timur Tabi <timur at 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 at vger.kernel.org # 4.11.x
> Tested-by: Manoj Iyer <manoj.iyer at canonical.com>
> Signed-off-by: Timur Tabi <timur at codeaurora.org>
> Signed-off-by: David S. Miller <davem at davemloft.net>
> (cherry picked from commit 246096690be0742d9bb5f3456d2cb95b68f7b46d)
> Signed-off-by: Manoj Iyer <manoj.iyer at canonical.com>
Acked-by: Stefan Bader <stefan.bader at 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);
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <https://lists.ubuntu.com/archives/kernel-team/attachments/20170621/b7e27f32/attachment.sig>
More information about the kernel-team
mailing list