RE: [EXT] Re: [RESENT PATCH net--stat 1/1] net: ethernet: fec: Revert "net: ethernet: fec: Replace interrupt driven MDIO with polled IO"

[Andy Duan <fugang.duan@nxp.com>]

From: Chris Healy <cphealy@gmail.com> Sent: Monday, July 27, 2020 10:13 AM
> On Sun, Jul 26, 2020 at 7:06 PM Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
> >
> > On Mon, Jul 27, 2020 at 04:24:02AM +0300, Laurent Pinchart wrote:
> > > 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.
> >
> > If it can be of any help, I've confirmed that, to get the network back
> > to usable state from v5.8-rc6, I have to revert all patches up to this
> > one. This is the top of my branch, on top of v5.8-rc6:
> >
> > 5bbe80c9efea Revert "net: ethernet: fec: Revert "net: ethernet: fec: Replace
> interrupt driven MDIO with polled IO""
> > 5462896a08c1 Revert "net: ethernet: fec: Replace interrupt driven MDIO
> with polled IO"
> > 824a82e2bdfa Revert "net: ethernet: fec: move GPR register offset and bit
> into DT"
> > bfe330591cab Revert "net: fec: disable correct clk in the err path of
> fec_enet_clk_enable"
> > 109958cad578 Revert "net: ethernet: fec: prevent tx starvation under high rx
> load"
> 
> I just fired up net-next on my i.MX7d based design (not NFS root though).  I can
> bring up the network interface with a gigabit connection but ALL RX traffic is
> failing with CRC errors.  Now, my design is using a Micrel KSZ9031 which might
> be part of the problem for me as there were some recent KSZ9031 changes
> made so take what I'm seeing with a grain of salt.
> 
> Laurent, couple questions:
> 
> 1) Are you able to boot without NFS root and communicate correctly or is this
> issue just when doing an NFS root?
> 2) If you are able to boot up without NFS root, can you check the ethtool
> statistics and see the same RX CRC errors I'm seeing?
> 
> 
Laurent, Chris, 

I am trying imx7d sdb board with BCM54220 siwtch PHY on net-next now,
will update my test result ASAP.

Regards,
Fugang
> >
> > > > 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