On Tue, Mar 5, 2019 at 7:42 PM Shawn Guo <shawn....@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@lists.denx.de > > > https://lists.denx.de/listinfo/u-boot > _______________________________________________ > U-Boot mailing list > U-Boot@lists.denx.de > https://lists.denx.de/listinfo/u-boot _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot