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);elsespace = 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@lists.denx.de https://lists.denx.de/listinfo/u-boot