Re: [PATCH net-next v11 3/3] net: stmmac: dwmac-nuvoton: Add dwmac glue for Nuvoton MA35 family

[Russell King (Oracle)]

Hi,

On Thu, Feb 05, 2026 at 09:40:05AM +0800, Joey Lu wrote:
> +
> +struct nvt_priv_data {
> +	struct platform_device *pdev;

This looks to me like it's write-only, does it serve a useful purpose?

> +	struct regmap *regmap;

This doesn't seem to be used outside of nvt_gmac_setup().

> +};

Given the above two comments, do you actually need struct nvt_priv_data ?

> +
> +static struct nvt_priv_data *
> +nvt_gmac_setup(struct platform_device *pdev, struct plat_stmmacenet_data *plat)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct nvt_priv_data *bsp_priv;
> +	phy_interface_t phy_mode;
> +	u32 macid, arg, reg;
> +	u32 tx_delay_step;
> +	u32 rx_delay_step;
> +	u32 miscr;
> +
> +	bsp_priv = devm_kzalloc(dev, sizeof(*bsp_priv), GFP_KERNEL);
> +	if (!bsp_priv)
> +		return ERR_PTR(-ENOMEM);
> +
> +	bsp_priv->regmap =
> +		syscon_regmap_lookup_by_phandle_args(dev->of_node, "nuvoton,sys", 1, &macid);
> +	if (IS_ERR(bsp_priv->regmap))
> +		return ERR_PTR(dev_err_probe(dev, PTR_ERR(bsp_priv->regmap),
> +				     "Failed to get sys register\n"));
> +	if (macid > 1) {
> +		dev_err(dev, "Invalid sys arguments\n");
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	if (of_property_read_u32(dev->of_node, "tx-internal-delay-ps", &arg)) {
> +		tx_delay_step = 0;
> +	} else {
> +		if (arg <= 2000) {
> +			tx_delay_step = (arg == 2000) ? 0xf : (arg / NVT_PATH_DELAY_STEP);
> +			dev_dbg(dev, "Set Tx path delay to 0x%x\n", tx_delay_step);
> +		} else {
> +			dev_err(dev, "Invalid Tx path delay argument.\n");
> +			return ERR_PTR(-EINVAL);
> +		}
> +	}
> +	if (of_property_read_u32(dev->of_node, "rx-internal-delay-ps", &arg)) {
> +		rx_delay_step = 0;
> +	} else {
> +		if (arg <= 2000) {
> +			rx_delay_step = (arg == 2000) ? 0xf : (arg / NVT_PATH_DELAY_STEP);
> +			dev_dbg(dev, "Set Rx path delay to 0x%x\n", rx_delay_step);
> +		} else {
> +			dev_err(dev, "Invalid Rx path delay argument.\n");
> +			return ERR_PTR(-EINVAL);
> +		}
> +	}

Each of these could be moved into a separate function:

static int nvt_gmac_get_delay(struct device *dev, const char *property)
{
	u32 arg;

	if (of_property_read_u32(dev->of_node, property, &arg))
		return 0;

	if (arg > 2000) {
		dev_err(dev, "Invalid %s argument.\n", property);
		return -EINVAL;
	}

	if (arg == 2000)
		return 15;

	return arg / NVT_PATH_DELAY_STEP;
}

then:
	int ret;

	ret = nvt_gmac_get_delay(dev, "tx-internal-delay-ps");
	if (ret < 0)
		return ERR_PTR(ret);

	tx_delay = ret;

	ret = nvt_gmac_get_delay(dev, "rx-internal-delay-ps");
	if (ret < 0)
		return ERR_PTR(ret);

	rx_delay = ret;

> +
> +	miscr = (macid == 0) ? NVT_REG_SYS_GMAC0MISCR : NVT_REG_SYS_GMAC1MISCR;
> +	regmap_read(bsp_priv->regmap, miscr, &reg);
> +	reg &= ~(NVT_TX_DELAY_MASK | NVT_RX_DELAY_MASK);
> +
> +	if (of_get_phy_mode(pdev->dev.of_node, &phy_mode)) {
> +		dev_err(dev, "missing phy mode property\n");
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	switch (phy_mode) {
> +	case PHY_INTERFACE_MODE_RGMII:
> +	case PHY_INTERFACE_MODE_RGMII_ID:
> +	case PHY_INTERFACE_MODE_RGMII_RXID:
> +	case PHY_INTERFACE_MODE_RGMII_TXID:
> +		reg &= ~NVT_MISCR_RMII;
> +		break;
> +	case PHY_INTERFACE_MODE_RMII:
> +		reg |= NVT_MISCR_RMII;
> +		break;
> +	default:
> +		dev_err(dev, "Unsupported phy-mode (%d)\n", phy_mode);
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	if (!(reg & NVT_MISCR_RMII)) {
> +		reg |= FIELD_PREP(NVT_TX_DELAY_MASK, tx_delay_step);
> +		reg |= FIELD_PREP(NVT_RX_DELAY_MASK, rx_delay_step);

You can move this inside the switch above under the RGMII case. Theses
delays are, after all, only for RGMII.

> +	}
> +
> +	regmap_write(bsp_priv->regmap, miscr, reg);

Consider:

	regmap_update_bits(bsp_priv->regmap, miscr,
			   NVT_TX_DELAY_MASK | NVT_RX_DELAY_MASK |
			   NVT_MISCR_RMII, reg);

> +	plat_dat = devm_stmmac_probe_config_dt(pdev, stmmac_res.mac);
> +	if (IS_ERR(plat_dat))
> +		return PTR_ERR(plat_dat);
> +
> +	/* Nuvoton DWMAC configs */
> +	plat_dat->core_type = DWMAC_CORE_GMAC;

Is the hardware not compatible with any of the compatible types that
devm_stmmac_probe_config_dt() will automatically set this for you?
Which version of the core do you have?

> +	plat_dat->tx_fifo_size = 2048;
> +	plat_dat->rx_fifo_size = 4096;

There are tx-fifo-depth / rx-fifo-depth properties that can be used to
describe these in DT.

> +	plat_dat->multicast_filter_bins = 0;
> +	plat_dat->unicast_filter_entries = 8;

If this core is v3.50, v3.70 or v3.72, then there are
snps,multicast-filter-bins and snps,perfect-filter-entries which
can be used to describe both of these.

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!