Re: [PATCH net-next 3/4] net: stmmac: Add glue driver for Motorcomm YT6801 ethernet controller

Andrew Lunn
2025-10-16T20:11:27Z

> +static int motorcomm_efuse_read_byte(struct dwmac_motorcomm_priv *priv,
> + u8 offset, u8 *byte)
> +{
> + u32 reg;
> + int ret;
> +
> + writel(FIELD_PREP(EFUSE_OP_MODE, EFUSE_OP_ROW_READ) |
> + FIELD_PREP(EFUSE_OP_ADDR, offset) |
> + EFUSE_OP_START, priv->base + EFUSE_OP_CTRL_0);
> +
> + ret = readl_poll_timeout(priv->base + EFUSE_OP_CTRL_1,
> + reg, reg & EFUSE_OP_DONE, 2000,
> + EFUSE_READ_TIMEOUT_US);
> +
> + reg = readl(priv->base + EFUSE_OP_CTRL_1);

Do you actually need this read? The documentation says:

* Returns: 0 on success and -ETIMEDOUT upon a timeout. In either
* case, the last read value at @addr is stored in @val.

> + *byte = FIELD_GET(EFUSE_OP_RD_DATA, reg);
> +
> + return ret;
> +}

> +static void motorcomm_reset_phy(struct dwmac_motorcomm_priv *priv)
> +{
> + u32 reg = readl(priv->base + EPHY_CTRL);
> +
> + reg &= ~EPHY_RESET;
> + writel(reg, priv->base + EPHY_CTRL);
> +
> + reg |= EPHY_RESET;
> + writel(reg, priv->base + EPHY_CTRL);
> +}

How does this differ to the PHY doing its own reset via BMCR?

We need to be careful of lifetimes here. It would be better if the PHY
controlled its own reset. We don't want phylib to configure the PHY
and then the MAC driver reset it etc.

> +static int motorcomm_resume(struct device *dev, void *bsp_priv)
> +{
> + struct dwmac_motorcomm_priv *priv = bsp_priv;
> + struct pci_dev *pdev = to_pci_dev(dev);
> + int ret;
> +
> + pci_restore_state(pdev);
> + pci_set_power_state(pdev, PCI_D0);
> +
> + ret = pcim_enable_device(pdev);
> + if (ret)
> + return ret;
> +
> + pci_set_master(pdev);
> +
> + motorcomm_reset_phy(priv);

Does the PHY support WoL? You probably should not be touching it if it
can wake the system.

> + return NULL;
> +
> + plat->mdio_bus_data = devm_kzalloc(dev, sizeof(*plat->mdio_bus_data),
> + GFP_KERNEL);
> + if (!plat->mdio_bus_data)
> + return NULL;

Is this required? If you look at other glue drivers which allocate
such a structure, they set members in it:

dwmac-intel.c: plat->mdio_bus_data->needs_reset = true;
dwmac-loongson.c: plat->mdio_bus_data->needs_reset = true;
dwmac-tegra.c: plat->mdio_bus_data->needs_reset = true;
stmmac_pci.c: plat->mdio_bus_data->needs_reset = true;
stmmac_platform.c: plat->mdio_bus_data->needs_reset = true;

You don't set anything.

Andrew