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

Joe Hershberger joe.hershberger at ni.com
Wed Mar 6 20:48:39 UTC 2019


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.

>
> >
> > > +
> > > +       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.

OK

>
> >
> > > +       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.

I would prefer a polarity to be defined in the DT for this reset.

>
> >
> > > +
> > > +       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.

OK.

Thanks,
-Joe

>
> 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
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot


More information about the U-Boot mailing list