From mboxrd@z Thu Jan 1 00:00:00 1970 From: Shawn Guo Date: Wed, 6 Mar 2019 09:41:29 +0800 Subject: [U-Boot] [PATCH v2 2/3] net: add higmacv300 Ethernet driver for HiSilicon platform In-Reply-To: References: <20190218033742.21342-1-shawn.guo@linaro.org> <20190218033742.21342-3-shawn.guo@linaro.org> Message-ID: <20190306014126.GA11908@dragon> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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; > > +} > > +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