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

[Shawn Guo <shawn.guo@linaro.org>]

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.

> 
> > +
> > +       if (!timeout)
> > +               return -ETIMEDOUT;
> > +
> > +       if (++bqr_pos >= RX_DESC_NUM)
> > +               bqr_pos = 0;
> > +
> > +       len = bqd->data_len;
> > +
> > +       /* CPU should not have touched this buffer since we added it to FQ */
> > +       invalidate_dcache_range(bqd->buf_addr, bqd->buf_addr + len);
> > +       *packetp = (void *)(unsigned long)bqd->buf_addr;
> > +
> > +       /* Record the RX_BQ descriptor that is holding RX data */
> > +       priv->rxdesc_in_use = bqr_pos;
> > +
> > +       return len;
> > +}

<snip>

> > +static int higmac_hw_init(struct higmac_priv *priv)
> > +{
> > +       int ret;
> > +
> > +       /* Initialize hardware queues */
> > +       ret = higmac_init_hw_queue(priv, RX_FQ);
> > +       if (ret)
> > +               return ret;
> > +
> > +       ret = higmac_init_hw_queue(priv, RX_BQ);
> > +       if (ret)
> > +               goto free_rx_fq;
> > +
> > +       ret = higmac_init_hw_queue(priv, TX_BQ);
> > +       if (ret)
> > +               goto free_rx_bq;
> > +
> > +       ret = higmac_init_hw_queue(priv, TX_RQ);
> > +       if (ret)
> > +               goto free_tx_bq;
> > +
> > +       /* Reset phy */
> > +       reset_assert(&priv->rst_phy);
> > +       mdelay(10);
> 
> I'm surprised the delay here is not a DT parameter.

We do not see the necessity for now.  We can make it a DT parameter when
we see the real need in the future.

> 
> > +       reset_deassert(&priv->rst_phy);
> > +       mdelay(30);
> 
> I'm surprised the delay here is not a DT parameter.
> 
> > +       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.

> 
> > +
> > +       return 0;
> > +
> > +free_tx_bq:
> > +       free(priv->txbq);
> > +free_rx_bq:
> > +       free(priv->rxbq);
> > +free_rx_fq:
> > +       free(priv->rxfq);
> > +       return ret;
> > +}
> > +
> > +static int higmac_probe(struct udevice *dev)
> > +{
> > +       struct higmac_priv *priv = dev_get_priv(dev);
> > +       struct phy_device *phydev;
> > +       struct mii_dev *bus;
> > +       int ret;
> > +
> > +       ret = higmac_hw_init(priv);
> > +       if (ret)
> > +               return ret;
> > +
> > +       bus = mdio_alloc();
> > +       if (!bus)
> > +               return -ENOMEM;
> > +
> > +       bus->read = higmac_mdio_read;
> > +       bus->write = higmac_mdio_write;
> > +       bus->priv = priv;
> > +       priv->bus = bus;
> > +
> > +       ret = mdio_register_seq(bus, dev->seq);
> > +       if (ret)
> > +               return ret;
> > +
> > +       phydev = phy_connect(bus, priv->phyaddr, dev, priv->phyintf);
> > +       if (!phydev)
> > +               return -ENODEV;
> > +
> > +       phydev->supported &= PHY_GBIT_FEATURES;
> > +       phydev->advertising = phydev->supported;
> > +       priv->phydev = phydev;
> > +
> > +       return phy_config(phydev);
> > +}
> > +
> > +static int higmac_remove(struct udevice *dev)
> > +{
> > +       struct higmac_priv *priv = dev_get_priv(dev);
> > +       int i;
> > +
> > +       mdio_unregister(priv->bus);
> > +       mdio_free(priv->bus);
> > +
> > +       /* Free RX packet buffers */
> > +       for (i = 0; i < RX_DESC_NUM; i++)
> > +               free((void *)(unsigned long)priv->rxfq[i].buf_addr);
> > +
> > +       return 0;
> > +}
> > +
> > +static int higmac_ofdata_to_platdata(struct udevice *dev)
> > +{
> > +       struct higmac_priv *priv = dev_get_priv(dev);
> > +       int phyintf = PHY_INTERFACE_MODE_NONE;
> > +       const char *phy_mode;
> > +       ofnode phy_node;
> > +
> > +       priv->base = dev_remap_addr_index(dev, 0);
> > +       priv->macif_ctrl = dev_remap_addr_index(dev, 1);
> > +
> > +       phy_mode = dev_read_string(dev, "phy-mode");
> 
> Should there not be a default? Is it supposed to be required to be
> specified in the DT?

Yes, we choose to implement it as a required property.  If the property
is missing, the device probe would fail.

Shawn

> 
> > +       if (phy_mode)
> > +               phyintf = phy_get_interface_by_name(phy_mode);
> > +       if (phyintf == PHY_INTERFACE_MODE_NONE)
> > +               return -ENODEV;
> > +       priv->phyintf = phyintf;
> > +
> > +       phy_node = dev_read_subnode(dev, "phy");
> > +       if (!ofnode_valid(phy_node)) {
> > +               debug("failed to find phy node\n");
> > +               return -ENODEV;
> > +       }
> > +       priv->phyaddr = ofnode_read_u32_default(phy_node, "reg", 0);
> > +
> > +       return reset_get_by_name(dev, "phy", &priv->rst_phy);
> > +}
> > +
> > +static const struct udevice_id higmac_ids[] = {
> > +       { .compatible = "hisilicon,hi3798cv200-gmac" },
> > +       { }
> > +};
> > +
> > +U_BOOT_DRIVER(eth_higmac) = {
> > +       .name   = "eth_higmac",
> > +       .id     = UCLASS_ETH,
> > +       .of_match = higmac_ids,
> > +       .ofdata_to_platdata = higmac_ofdata_to_platdata,
> > +       .probe  = higmac_probe,
> > +       .remove = higmac_remove,
> > +       .ops    = &higmac_ops,
> > +       .priv_auto_alloc_size = sizeof(struct higmac_priv),
> > +       .platdata_auto_alloc_size = sizeof(struct eth_pdata),
> > +};
> > --
> > 2.18.0
> >
> > _______________________________________________
> > U-Boot mailing list
> > U-Boot at lists.denx.de
> > https://lists.denx.de/listinfo/u-boot