[U-Boot] [PATCH v2 2/3] net: add higmacv300 Ethernet driver for HiSilicon platform

[Shawn Guo]

On Wed, Mar 06, 2019 at 08:48:40PM +0000, Joe Hershberger wrote:
> On Tue, Mar 5, 2019 at 7:42 PM Shawn Guo <shawn.guo at linaro.org> wrote:
> >
> > On Tue, Mar 05, 2019 at 11:58:26PM +0000, Joe Hershberger wrote:
> > > > +static int higmac_recv(struct udevice *dev, int flags, uchar **packetp)
> > > > +{
> > > > +       struct higmac_priv *priv = dev_get_priv(dev);
> > > > +       struct higmac_desc *fqd = priv->rxfq;
> > > > +       struct higmac_desc *bqd = priv->rxbq;
> > > > +       int fqw_pos, fqr_pos, bqw_pos, bqr_pos;
> > > > +       int timeout = 100000;
> > > > +       int len = 0;
> > > > +       int space;
> > > > +       int i;
> > > > +
> > > > +       fqw_pos = DESC_CNT(readl(priv->base + RX_FQ_WR_ADDR));
> > > > +       fqr_pos = DESC_CNT(readl(priv->base + RX_FQ_RD_ADDR));
> > > > +
> > > > +       if (fqw_pos >= fqr_pos)
> > > > +               space = RX_DESC_NUM - (fqw_pos - fqr_pos);
> > > > +       else
> > > > +               space = fqr_pos - fqw_pos;
> > > > +
> > > > +       /* Leave one free to distinguish full filled from empty buffer */
> > > > +       for (i = 0; i < space - 1; i++) {
> > > > +               fqd = priv->rxfq + fqw_pos;
> > > > +               invalidate_dcache_range(fqd->buf_addr,
> > > > +                                       fqd->buf_addr + MAC_MAX_FRAME_SIZE);
> > > > +
> > > > +               if (++fqw_pos >= RX_DESC_NUM)
> > > > +                       fqw_pos = 0;
> > > > +
> > > > +               writel(DESC_BYTE(fqw_pos), priv->base + RX_FQ_WR_ADDR);
> > > > +       }
> > > > +
> > > > +       bqr_pos = DESC_CNT(readl(priv->base + RX_BQ_RD_ADDR));
> > > > +       bqd += bqr_pos;
> > > > +       /* BQ is only ever written by GMAC */
> > > > +       invalidate_desc(bqd);
> > > > +
> > > > +       do {
> > > > +               bqw_pos = DESC_CNT(readl(priv->base + RX_BQ_WR_ADDR));
> > > > +               udelay(1);
> > > > +       } while (--timeout && bqw_pos == bqr_pos);
> > >
> > > Did you look into using wait bit macros?
> >
> > I may miss your point, but this is not a loop waiting for some bits set
> > or clear.  It's waiting for a given number.
> 
> OK, I see that, thanks. Should you make these "breakable" in the same
> way that wait_for_bit_* does? The timeout seems quite long.

I assume that you mean the "breakable" as user interaction (CTRL-C)?
I'm not sure 100000 us (0.1 s) is so long for user to break.

<snip>

> > > > +       reset_assert(&priv->rst_phy);
> > > > +       mdelay(30);
> > >
> > > Why is this reasserted?
> >
> > I have to admit this is a bit hackish.  Ideally, the reset sequence
> > should be: deassert -> assert -> deassert.  But this reset signal gets
> > an opposite polarity than others that reset driver handles.  I can add a
> > comment to explain it if you can tolerate this little hack, or I will
> > add polarity support to reset driver to handle this phy reset quirk.
> > Please let me know your preference.
> 
> I would prefer a polarity to be defined in the DT for this reset.

OK, I will implement it in v3.  Thanks for the comment.

Shawn