[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