From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Fugang Duan <fugang.duan@nxp.com>
Cc: davem@davemloft.net, netdev@vger.kernel.org, andrew@lunn.ch,
cphealy@gmail.com, martin.fuzzey@flowbird.group
Subject: Re: [RESENT PATCH net--stat 1/1] net: ethernet: fec: Revert "net: ethernet: fec: Replace interrupt driven MDIO with polled IO"
Date: Mon, 27 Jul 2020 04:23:54 +0300 [thread overview]
Message-ID: <20200727012354.GT28704@pendragon.ideasonboard.com> (raw)
In-Reply-To: <1587996484-3504-1-git-send-email-fugang.duan@nxp.com>
Hi Fugang,
On Mon, Apr 27, 2020 at 10:08:04PM +0800, Fugang Duan wrote:
> This reverts commit 29ae6bd1b0d8a57d7c00ab12cbb949fc41986eef.
>
> The commit breaks ethernet function on i.MX6SX, i.MX7D, i.MX8MM,
> i.MX8MQ, and i.MX8QXP platforms. Boot yocto system by NFS mounting
> rootfs will be failed with the commit.
I'm afraid this commit breaks networking on i.MX7D for me :-( My board
is configured to boot over NFS root with IP autoconfiguration through
DHCP. The DHCP request goes out, the reply it sent back by the server,
but never noticed by the fec driver.
v5.7 works fine. As 29ae6bd1b0d8a57d7c00ab12cbb949fc41986eef was merged
during the v5.8 merge window, I suspect something else cropped in
between 29ae6bd1b0d8a57d7c00ab12cbb949fc41986eef and this patch that
needs to be reverted too. We're close to v5.8 and it would be annoying
to see this regression ending up in the released kernel. I can test
patches, but I'm not familiar enough with the driver (or the networking
subsystem) to fix the issue myself.
> Signed-off-by: Fugang Duan <fugang.duan@nxp.com>
>
> diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h
> index a6cdd5b..e74dd1f 100644
> --- a/drivers/net/ethernet/freescale/fec.h
> +++ b/drivers/net/ethernet/freescale/fec.h
> @@ -376,7 +376,8 @@ struct bufdesc_ex {
> #define FEC_ENET_TS_AVAIL ((uint)0x00010000)
> #define FEC_ENET_TS_TIMER ((uint)0x00008000)
>
> -#define FEC_DEFAULT_IMASK (FEC_ENET_TXF | FEC_ENET_RXF)
> +#define FEC_DEFAULT_IMASK (FEC_ENET_TXF | FEC_ENET_RXF | FEC_ENET_MII)
> +#define FEC_NAPI_IMASK FEC_ENET_MII
> #define FEC_RX_DISABLED_IMASK (FEC_DEFAULT_IMASK & (~FEC_ENET_RXF))
>
> /* ENET interrupt coalescing macro define */
> @@ -542,6 +543,7 @@ struct fec_enet_private {
> int link;
> int full_duplex;
> int speed;
> + struct completion mdio_done;
> int irq[FEC_IRQ_NUM];
> bool bufdesc_ex;
> int pause_flag;
> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
> index 1ae075a..c7b84bb 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -976,8 +976,8 @@ fec_restart(struct net_device *ndev)
> writel((__force u32)cpu_to_be32(temp_mac[1]),
> fep->hwp + FEC_ADDR_HIGH);
>
> - /* Clear any outstanding interrupt, except MDIO. */
> - writel((0xffffffff & ~FEC_ENET_MII), fep->hwp + FEC_IEVENT);
> + /* Clear any outstanding interrupt. */
> + writel(0xffffffff, fep->hwp + FEC_IEVENT);
>
> fec_enet_bd_init(ndev);
>
> @@ -1123,7 +1123,7 @@ fec_restart(struct net_device *ndev)
> if (fep->link)
> writel(FEC_DEFAULT_IMASK, fep->hwp + FEC_IMASK);
> else
> - writel(0, fep->hwp + FEC_IMASK);
> + writel(FEC_ENET_MII, fep->hwp + FEC_IMASK);
>
> /* Init the interrupt coalescing */
> fec_enet_itr_coal_init(ndev);
> @@ -1652,10 +1652,6 @@ fec_enet_interrupt(int irq, void *dev_id)
> irqreturn_t ret = IRQ_NONE;
>
> int_events = readl(fep->hwp + FEC_IEVENT);
> -
> - /* Don't clear MDIO events, we poll for those */
> - int_events &= ~FEC_ENET_MII;
> -
> writel(int_events, fep->hwp + FEC_IEVENT);
> fec_enet_collect_events(fep, int_events);
>
> @@ -1663,12 +1659,16 @@ fec_enet_interrupt(int irq, void *dev_id)
> ret = IRQ_HANDLED;
>
> if (napi_schedule_prep(&fep->napi)) {
> - /* Disable interrupts */
> - writel(0, fep->hwp + FEC_IMASK);
> + /* Disable the NAPI interrupts */
> + writel(FEC_NAPI_IMASK, fep->hwp + FEC_IMASK);
> __napi_schedule(&fep->napi);
> }
> }
>
> + if (int_events & FEC_ENET_MII) {
> + ret = IRQ_HANDLED;
> + complete(&fep->mdio_done);
> + }
> return ret;
> }
>
> @@ -1818,24 +1818,11 @@ static void fec_enet_adjust_link(struct net_device *ndev)
> phy_print_status(phy_dev);
> }
>
> -static int fec_enet_mdio_wait(struct fec_enet_private *fep)
> -{
> - uint ievent;
> - int ret;
> -
> - ret = readl_poll_timeout_atomic(fep->hwp + FEC_IEVENT, ievent,
> - ievent & FEC_ENET_MII, 2, 30000);
> -
> - if (!ret)
> - writel(FEC_ENET_MII, fep->hwp + FEC_IEVENT);
> -
> - return ret;
> -}
> -
> static int fec_enet_mdio_read(struct mii_bus *bus, int mii_id, int regnum)
> {
> struct fec_enet_private *fep = bus->priv;
> struct device *dev = &fep->pdev->dev;
> + unsigned long time_left;
> int ret = 0, frame_start, frame_addr, frame_op;
> bool is_c45 = !!(regnum & MII_ADDR_C45);
>
> @@ -1843,6 +1830,8 @@ static int fec_enet_mdio_read(struct mii_bus *bus, int mii_id, int regnum)
> if (ret < 0)
> return ret;
>
> + reinit_completion(&fep->mdio_done);
> +
> if (is_c45) {
> frame_start = FEC_MMFR_ST_C45;
>
> @@ -1854,9 +1843,11 @@ static int fec_enet_mdio_read(struct mii_bus *bus, int mii_id, int regnum)
> fep->hwp + FEC_MII_DATA);
>
> /* wait for end of transfer */
> - ret = fec_enet_mdio_wait(fep);
> - if (ret) {
> + time_left = wait_for_completion_timeout(&fep->mdio_done,
> + usecs_to_jiffies(FEC_MII_TIMEOUT));
> + if (time_left == 0) {
> netdev_err(fep->netdev, "MDIO address write timeout\n");
> + ret = -ETIMEDOUT;
> goto out;
> }
>
> @@ -1875,9 +1866,11 @@ static int fec_enet_mdio_read(struct mii_bus *bus, int mii_id, int regnum)
> FEC_MMFR_TA, fep->hwp + FEC_MII_DATA);
>
> /* wait for end of transfer */
> - ret = fec_enet_mdio_wait(fep);
> - if (ret) {
> + time_left = wait_for_completion_timeout(&fep->mdio_done,
> + usecs_to_jiffies(FEC_MII_TIMEOUT));
> + if (time_left == 0) {
> netdev_err(fep->netdev, "MDIO read timeout\n");
> + ret = -ETIMEDOUT;
> goto out;
> }
>
> @@ -1895,6 +1888,7 @@ static int fec_enet_mdio_write(struct mii_bus *bus, int mii_id, int regnum,
> {
> struct fec_enet_private *fep = bus->priv;
> struct device *dev = &fep->pdev->dev;
> + unsigned long time_left;
> int ret, frame_start, frame_addr;
> bool is_c45 = !!(regnum & MII_ADDR_C45);
>
> @@ -1904,6 +1898,8 @@ static int fec_enet_mdio_write(struct mii_bus *bus, int mii_id, int regnum,
> else
> ret = 0;
>
> + reinit_completion(&fep->mdio_done);
> +
> if (is_c45) {
> frame_start = FEC_MMFR_ST_C45;
>
> @@ -1915,9 +1911,11 @@ static int fec_enet_mdio_write(struct mii_bus *bus, int mii_id, int regnum,
> fep->hwp + FEC_MII_DATA);
>
> /* wait for end of transfer */
> - ret = fec_enet_mdio_wait(fep);
> - if (ret) {
> + time_left = wait_for_completion_timeout(&fep->mdio_done,
> + usecs_to_jiffies(FEC_MII_TIMEOUT));
> + if (time_left == 0) {
> netdev_err(fep->netdev, "MDIO address write timeout\n");
> + ret = -ETIMEDOUT;
> goto out;
> }
> } else {
> @@ -1933,9 +1931,12 @@ static int fec_enet_mdio_write(struct mii_bus *bus, int mii_id, int regnum,
> fep->hwp + FEC_MII_DATA);
>
> /* wait for end of transfer */
> - ret = fec_enet_mdio_wait(fep);
> - if (ret)
> + time_left = wait_for_completion_timeout(&fep->mdio_done,
> + usecs_to_jiffies(FEC_MII_TIMEOUT));
> + if (time_left == 0) {
> netdev_err(fep->netdev, "MDIO write timeout\n");
> + ret = -ETIMEDOUT;
> + }
>
> out:
> pm_runtime_mark_last_busy(dev);
> @@ -2144,9 +2145,6 @@ static int fec_enet_mii_init(struct platform_device *pdev)
>
> writel(fep->phy_speed, fep->hwp + FEC_MII_SPEED);
>
> - /* Clear any pending transaction complete indication */
> - writel(FEC_ENET_MII, fep->hwp + FEC_IEVENT);
> -
> fep->mii_bus = mdiobus_alloc();
> if (fep->mii_bus == NULL) {
> err = -ENOMEM;
> @@ -3688,6 +3686,7 @@ fec_probe(struct platform_device *pdev)
> fep->irq[i] = irq;
> }
>
> + init_completion(&fep->mdio_done);
> ret = fec_enet_mii_init(pdev);
> if (ret)
> goto failed_mii_init;
--
Regards,
Laurent Pinchart
next prev parent reply other threads:[~2020-07-27 1:24 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-04-27 14:08 [RESENT PATCH net--stat 1/1] net: ethernet: fec: Revert "net: ethernet: fec: Replace interrupt driven MDIO with polled IO" Fugang Duan
2020-05-01 3:31 ` David Miller
2020-07-27 1:23 ` Laurent Pinchart [this message]
2020-07-27 1:38 ` Andrew Lunn
2020-07-27 2:06 ` Laurent Pinchart
2020-07-27 2:13 ` Chris Healy
2020-07-27 2:21 ` [EXT] " Andy Duan
2020-07-27 2:36 ` Laurent Pinchart
2020-07-27 2:14 ` Andrew Lunn
2020-07-27 2:33 ` Laurent Pinchart
2020-07-27 2:35 ` Chris Healy
2020-07-27 2:39 ` Laurent Pinchart
2020-07-27 2:40 ` Chris Healy
2020-07-27 2:51 ` [EXT] " Andy Duan
2020-07-27 3:01 ` Chris Healy
2020-07-27 3:08 ` [EXT] " Andy Duan
2020-07-27 12:05 ` Andrew Lunn
2020-07-27 13:30 ` Chris Healy
2020-07-27 15:24 ` Laurent Pinchart
2020-07-27 15:41 ` Chris Healy
2020-07-27 17:37 ` Laurent Pinchart
2020-07-27 18:03 ` Laurent Pinchart
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200727012354.GT28704@pendragon.ideasonboard.com \
--to=laurent.pinchart@ideasonboard.com \
--cc=andrew@lunn.ch \
--cc=cphealy@gmail.com \
--cc=davem@davemloft.net \
--cc=fugang.duan@nxp.com \
--cc=martin.fuzzey@flowbird.group \
--cc=netdev@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).