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

[Joey Lu <a0987203069@gmail.com>]

On 2/5/2026 5:38 PM, Russell King (Oracle) wrote:
> 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 ?
You are right. I'll drop it in the next revision.
>
>> +
>> +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;
I'll update the code according to your suggestions.
>> +
>> +	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.
Got it. I'll move them into the RGMII case.
>> +	}
>> +
>> +	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.

Thanks for the feedback.

This GMAC is based on v3.73a. While this specific revision isn’t 
explicitly documented in the current DT binding YAML, the relevant FIFO 
sizing and filter capabilities match the behavior introduced in earlier 
v3.70+ cores.

Given that, I agree it makes sense to describe these parameters using 
the existing DT properties.

I will update the DT and driver accordingly in the next revision.

Joey

>