[U-Boot] [PATCH v2 2/3] net: add higmacv300 Ethernet driver for HiSilicon platform
Shawn Guo
shawn.guo at linaro.org
Wed Mar 6 01:41:29 UTC 2019
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
More information about the U-Boot
mailing list