Re: [U-Boot] [PATCH v2 2/2] net: mvgbe: convert to DM

On Mon, Jul 9, 2018 at 9:05 AM Michael Walle michael@walle.cc wrote:

Hi Chris,

thanks for your efforts. This basically works for me on my Kirkwood LSCHLv2 board. So you may add

Tested-by: Michael Walle michael@walle.cc

But there are some issues. See below.

[snip]

...

-#if defined(CONFIG_PHYLIB) +#if defined(CONFIG_PHYLIB) || defined(CONFIG_DM_ETH) +#if defined(CONFIG_DM_ETH) +static struct phy_device *__mvgbe_phy_init(struct udevice *dev,

•                                    struct mii_dev *bus,
  

•                                    phy_interface_t phy_interface,
  

•                                    int phyid)
  

+#else +static struct phy_device *__mvgbe_phy_init(struct eth_device *dev,

•                                    struct mii_dev *bus,
  

•                                    phy_interface_t phy_interface,
  

•                                    int phyid)
  

+#endif +{

• struct phy_device *phydev;
  

• /* Set phy address of the port */
  

• miiphy_write(dev->name, MV_PHY_ADR_REQUEST, MV_PHY_ADR_REQUEST,
  

•              phyid);
  

• phydev = phy_connect(bus, phyid, dev, phy_interface);
  

So CONFIG_PHYLIB is required if CONFIG_DM_ETH is enabled? If so, then can we add

selects PHYLIB if DM_ETH

to the CONFIG_MVGBE Kconfig part?

Yes will do. I even erroneously added it at one point (without the if DM_ETH).

...

• if (!phydev) {
  

•         printf("phy_connect failed\n");
  

•         return NULL;
  

• }
  

• phy_config(phydev);
  

• phy_startup(phydev);
  

• return phydev;
  

+} +#endif /* CONFIG_PHYLIB || CONFIG_DM_ETH */

[snip]

...

+static int mvgbe_ofdata_to_platdata(struct udevice *dev) +{

• struct eth_pdata *pdata = dev_get_platdata(dev);
  

• struct mvgbe_device *dmvgbe = dev_get_priv(dev);
  

• void *blob = (void *)gd->fdt_blob;
  

• int node = dev_of_offset(dev);
  

• const char *phy_mode;
  

• int fl_node;
  

• unsigned long addr;
  

• pdata->iobase = devfdt_get_addr(dev);
  

• /* Get phy-mode / phy_interface from DT */
  

• pdata->phy_interface = -1;
  

• phy_mode = fdt_getprop(gd->fdt_blob, dev_of_offset(dev), "phy-mode",
  

•                        NULL);
  

• if (phy_mode)
  

•         pdata->phy_interface = phy_get_interface_by_name(phy_mode);
  

• if (pdata->phy_interface == -1) {
  

•         debug("%s: Invalid PHY interface '%s'\n", __func__, phy_mode);
  

•         return -EINVAL;
  

• }
  

• dmvgbe->phy_interface = pdata->phy_interface;
  

• /* fetch 'fixed-link' property */
  

• fl_node = fdt_subnode_offset(blob, node, "fixed-link");
  

• if (fl_node != -FDT_ERR_NOTFOUND) {
  

•         /* set phy_addr to invalid value for fixed link */
  

•         dmvgbe->phyaddr = PHY_MAX_ADDR + 1;
  

•         dmvgbe->duplex = fdtdec_get_bool(blob, fl_node, "full-duplex");
  

•         dmvgbe->speed = fdtdec_get_int(blob, fl_node, "speed", 0);
  

• } else {
  

•         /* Now read phyaddr from DT */
  

•         addr = fdtdec_get_int(blob, node, "phy", 0);
  

Should be "phy-handle", shouldn't it? And isn't there already function for this which gets the phy address? Also, the phy-handle is on the port subnode in the device tree. I guess, this applies to phy-mode, too.

Hmm my hacky dts for testing this called it "phy" and attached it to the eth node. I'll look at a couple of proper boards and code to them.

...

•         addr = fdt_node_offset_by_phandle(blob, addr);
  

•         dmvgbe->phyaddr = fdtdec_get_int(blob, addr, "reg", 0);
  

• }
  

• return 0;
  

+}

-michael