Message-ID: <36a43bba.a05.198f023b6a6.Coremail.lizhi2@eswincomputing.com>
Date: Thu, 28 Aug 2025 18:05:29 +0800 (GMT+08:00)
From: 李志 <lizhi2@...incomputing.com>
To: "Russell King (Oracle)" <linux@...linux.org.uk>
Cc: weishangjuan@...incomputing.com, devicetree@...r.kernel.org,
	andrew+netdev@...n.ch, davem@...emloft.net, edumazet@...gle.com,
	kuba@...nel.org, pabeni@...hat.com, robh@...nel.org,
	krzk+dt@...nel.org, conor+dt@...nel.org,
	linux-arm-kernel@...ts.infradead.org, mcoquelin.stm32@...il.com,
	alexandre.torgue@...s.st.com, yong.liang.choong@...ux.intel.com,
	vladimir.oltean@....com, faizal.abdul.rahim@...ux.intel.com,
	prabhakar.mahadev-lad.rj@...renesas.com, inochiama@...il.com,
	jan.petrous@....nxp.com, jszhang@...nel.org, p.zabel@...gutronix.de,
	boon.khai.ng@...era.com, 0x1207@...il.com, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org,
	linux-stm32@...md-mailman.stormreply.com, ningyu@...incomputing.com,
	linmin@...incomputing.com, pinkesh.vaghela@...fochips.com
Subject: Re: Re: [PATCH v4 2/2] ethernet: eswin: Add eic7700 ethernet driver

Dear Russell King,
Thank you for your valuable and professional suggestions.
Please find our questions and explanations embedded below your comments
in the original email.

Best regards,

Li Zhi
Eswin Computing


> -----原始邮件-----
> 发件人: "Russell King (Oracle)" <linux@...linux.org.uk>
> 发送时间:2025-08-27 16:24:51 (星期三)
> 收件人: weishangjuan@...incomputing.com
> 抄送: devicetree@...r.kernel.org, andrew+netdev@...n.ch, davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com, robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org, linux-arm-kernel@...ts.infradead.org, mcoquelin.stm32@...il.com, alexandre.torgue@...s.st.com, yong.liang.choong@...ux.intel.com, vladimir.oltean@....com, faizal.abdul.rahim@...ux.intel.com, prabhakar.mahadev-lad.rj@...renesas.com, inochiama@...il.com, jan.petrous@....nxp.com, jszhang@...nel.org, p.zabel@...gutronix.de, boon.khai.ng@...era.com, 0x1207@...il.com, netdev@...r.kernel.org, linux-kernel@...r.kernel.org, linux-stm32@...md-mailman.stormreply.com, ningyu@...incomputing.com, linmin@...incomputing.com, lizhi2@...incomputing.com
> 主题: Re: [PATCH v4 2/2] ethernet: eswin: Add eic7700 ethernet driver
> 
> On Wed, Aug 27, 2025 at 04:14:17PM +0800, weishangjuan@...incomputing.com wrote:
> > +struct eic7700_qos_priv {
> > +	struct device *dev;
> > +	struct regmap *hsp_regmap;
> > +	struct clk *clk_tx;
> 
> Consider putting a pointer to the plat_dat here instead of clk_tx.
> 
> > +	struct clk *clk_axi;
> > +	struct clk *clk_cfg;
> 
> Consider moving these into plat_dat->clks.
> 
> > +	u32 tx_delay_ps;
> > +	u32 rx_delay_ps;
> > +};
> > +
> > +/**
> > + * eic7700_apply_delay - Update TX or RX delay bits in delay parameter value.
> > + * @delay_ps: Delay in picoseconds (capped at 12.7ns).
> > + * @reg:      Pointer to register value to modify.
> > + * @is_rx:    True for RX delay (bits 30:24), false for TX delay (bits 14:8).
> > + *
> > + * Converts delay to 0.1ns units, caps at 0x7F, and sets appropriate bits.
> > + * Only RX or TX bits are updated; other bits remain unchanged.
> > + */
> > +static inline void eic7700_apply_delay(u32 delay_ps, u32 *reg, bool is_rx)
> > +{
> > +	if (!reg)
> > +		return;
> > +
> > +	u32 val = min(delay_ps / 100, EIC7700_MAX_DELAY_UNIT);
> > +
> > +	if (is_rx) {
> > +		*reg &= ~EIC7700_ETH_RX_ADJ_DELAY;
> > +		*reg |= (val << 24) & EIC7700_ETH_RX_ADJ_DELAY;
> > +	} else {
> > +		*reg &= ~EIC7700_ETH_TX_ADJ_DELAY;
> > +		*reg |= (val << 8) & EIC7700_ETH_TX_ADJ_DELAY;
> > +	}
> > +}
> > +
> > +static int eic7700_clks_config(void *priv, bool enabled)
> > +{
> > +	struct eic7700_qos_priv *dwc = (struct eic7700_qos_priv *)priv;
> > +	int ret = 0;
> > +
> > +	if (enabled) {
> > +		ret = clk_prepare_enable(dwc->clk_tx);
> > +		if (ret < 0) {
> > +			dev_err(dwc->dev, "Failed to enable tx clock: %d\n",
> > +				ret);
> > +			goto err;
> > +		}
> > +
> > +		ret = clk_prepare_enable(dwc->clk_axi);
> > +		if (ret < 0) {
> > +			dev_err(dwc->dev, "Failed to enable axi clock: %d\n",
> > +				ret);
> > +			goto err_tx;
> > +		}
> > +
> > +		ret = clk_prepare_enable(dwc->clk_cfg);
> > +		if (ret < 0) {
> > +			dev_err(dwc->dev, "Failed to enable cfg clock: %d\n",
> > +				ret);
> > +			goto err_axi;
> > +		}
> 
> You can then use clk_bulk_prepare_enable() here without the complex
> unwinding if one enable fails.
> 
> > +	} else {
> > +		clk_disable_unprepare(dwc->clk_tx);
> > +		clk_disable_unprepare(dwc->clk_axi);
> > +		clk_disable_unprepare(dwc->clk_cfg);
> 
> and clk_bulk_disable_unprepare() here.
> 
> > +	}
> > +	return ret;
> > +
> > +err_axi:
> > +	clk_disable_unprepare(dwc->clk_axi);
> > +err_tx:
> > +	clk_disable_unprepare(dwc->clk_tx);
> > +err:
> > +	return ret;
> > +}
> > +
> > +static int eic7700_dwmac_probe(struct platform_device *pdev)
> > +{
> > +	struct plat_stmmacenet_data *plat_dat;
> > +	struct stmmac_resources stmmac_res;
> > +	struct eic7700_qos_priv *dwc_priv;
> > +	u32 eth_axi_lp_ctrl_offset;
> > +	u32 eth_phy_ctrl_offset;
> > +	u32 eth_phy_ctrl_regset;
> > +	u32 eth_rxd_dly_offset;
> > +	u32 eth_dly_param = 0;
> > +	int ret;
> > +
> > +	ret = stmmac_get_platform_resources(pdev, &stmmac_res);
> > +	if (ret)
> > +		return dev_err_probe(&pdev->dev, ret,
> > +				"failed to get resources\n");
> > +
> > +	plat_dat = devm_stmmac_probe_config_dt(pdev, stmmac_res.mac);
> > +	if (IS_ERR(plat_dat))
> > +		return dev_err_probe(&pdev->dev, PTR_ERR(plat_dat),
> > +				"dt configuration failed\n");
> > +
> > +	dwc_priv = devm_kzalloc(&pdev->dev, sizeof(*dwc_priv), GFP_KERNEL);
> > +	if (!dwc_priv)
> > +		return -ENOMEM;
> > +
> > +	dwc_priv->dev = &pdev->dev;
> > +
> > +	/* Read rx-internal-delay-ps and update rx_clk delay */
> > +	if (!of_property_read_u32(pdev->dev.of_node,
> > +				  "rx-internal-delay-ps",
> > +				  &dwc_priv->rx_delay_ps)) {
> > +		eic7700_apply_delay(dwc_priv->rx_delay_ps,
> > +				    &eth_dly_param, true);
> > +	} else {
> > +		dev_warn(&pdev->dev, "can't get rx-internal-delay-ps\n");
> > +	}
> > +
> > +	/* Read tx-internal-delay-ps and update tx_clk delay */
> > +	if (!of_property_read_u32(pdev->dev.of_node,
> > +				  "tx-internal-delay-ps",
> > +				  &dwc_priv->tx_delay_ps)) {
> > +		eic7700_apply_delay(dwc_priv->tx_delay_ps,
> > +				    &eth_dly_param, false);
> > +	} else {
> > +		dev_warn(&pdev->dev, "can't get tx-internal-delay-ps\n");
> > +	}
> > +
> > +	dwc_priv->hsp_regmap =
> > +		syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
> > +						"eswin,hsp-sp-csr");
> > +	if (IS_ERR(dwc_priv->hsp_regmap))
> > +		return dev_err_probe(&pdev->dev,
> > +				PTR_ERR(dwc_priv->hsp_regmap),
> > +				"Failed to get hsp-sp-csr regmap\n");
> > +
> > +	ret = of_property_read_u32_index(pdev->dev.of_node,
> > +					 "eswin,hsp-sp-csr",
> > +					 1, &eth_phy_ctrl_offset);
> > +	if (ret)
> > +		return dev_err_probe(&pdev->dev,
> > +				ret,
> > +				"can't get eth_phy_ctrl_offset\n");
> > +
> > +	regmap_read(dwc_priv->hsp_regmap, eth_phy_ctrl_offset,
> > +		    &eth_phy_ctrl_regset);
> > +	eth_phy_ctrl_regset |=
> > +		(EIC7700_ETH_TX_CLK_SEL | EIC7700_ETH_PHY_INTF_SELI);
> > +	regmap_write(dwc_priv->hsp_regmap, eth_phy_ctrl_offset,
> > +		     eth_phy_ctrl_regset);
> > +
> > +	ret = of_property_read_u32_index(pdev->dev.of_node,
> > +					 "eswin,hsp-sp-csr",
> > +					 2, &eth_axi_lp_ctrl_offset);
> > +	if (ret)
> > +		return dev_err_probe(&pdev->dev,
> > +				ret,
> > +				"can't get eth_axi_lp_ctrl_offset\n");
> > +
> > +	regmap_write(dwc_priv->hsp_regmap, eth_axi_lp_ctrl_offset,
> > +		     EIC7700_ETH_CSYSREQ_VAL);
> > +
> > +	ret = of_property_read_u32_index(pdev->dev.of_node,
> > +					 "eswin,hsp-sp-csr",
> > +					 3, &eth_rxd_dly_offset);
> > +	if (ret)
> > +		return dev_err_probe(&pdev->dev,
> > +				ret,
> > +				"can't get eth_rxd_dly_offset\n");
> > +
> > +	regmap_write(dwc_priv->hsp_regmap, eth_rxd_dly_offset,
> > +		     eth_dly_param);
> > +
> > +	dwc_priv->clk_tx = devm_clk_get(&pdev->dev, "tx");
> > +	if (IS_ERR(dwc_priv->clk_tx))
> > +		return dev_err_probe(&pdev->dev,
> > +				PTR_ERR(dwc_priv->clk_tx),
> > +				"error getting tx clock\n");
> > +
> > +	dwc_priv->clk_axi = devm_clk_get(&pdev->dev, "axi");
> > +	if (IS_ERR(dwc_priv->clk_axi))
> > +		return dev_err_probe(&pdev->dev,
> > +				PTR_ERR(dwc_priv->clk_axi),
> > +				"error getting axi clock\n");
> > +
> > +	dwc_priv->clk_cfg = devm_clk_get(&pdev->dev, "cfg");
> > +	if (IS_ERR(dwc_priv->clk_cfg))
> > +		return dev_err_probe(&pdev->dev,
> > +				PTR_ERR(dwc_priv->clk_cfg),
> > +				"error getting cfg clock\n");
> 
> These then become devm_clk_bulk_get_all().
> 

Several technical points in the driver patches need clarification:
1. Our platform uses four clocks: stmmaceth, tx, axi, and cfg. The stmmaceth
   clock is automatically managed by stmmac_platform.c when
   "snps,dwc-qos-ethernet-4.10" is not configured in the DTS. Therefore,
   devm_clk_bulk_get_all() / devm_clk_bulk_get_all_enabled() are not applicable
   in our driver. For the "tx", "axi", and "cfg" clocks, we plan to refer to
   dwmac-rk.c in the next patch and use devm_clk_bulk_get_optional() to manage
   them in bulk. This approach allows automatic handling of probe and remove,
   simplifying the current enable/unwind logic. Is this correct?
2. With devm_clk_bulk_get_optional() managing the clocks in
   plat_stmmacenet_data, the eic7700_clks_config() API will access
   plat_stmmacenet_data via eic7700_qos_priv in the next patch. This
   design approach is appropriate, correct?

> > +
> > +	ret = eic7700_clks_config(dwc_priv, true);
> > +	if (ret)
> > +		return dev_err_probe(&pdev->dev,
> > +				ret,
> > +				"error enable clock\n");
> 
> Maybe even devm_clk_bulk_get_all_enabled() which will omit this
> step...
> 
> > +
> > +	plat_dat->clk_tx_i = dwc_priv->clk_tx;
> > +	plat_dat->set_clk_tx_rate = stmmac_set_clk_tx_rate;
> > +	plat_dat->bsp_priv = dwc_priv;
> > +	plat_dat->clks_config = eic7700_clks_config;
> > +
> > +	ret = stmmac_dvr_probe(&pdev->dev, plat_dat, &stmmac_res);
> > +	if (ret) {
> > +		eic7700_clks_config(dwc_priv, false);
> 
> ... and means you don't need this call...
> 
> > +		return dev_err_probe(&pdev->dev,
> > +				ret,
> > +				"Failed to driver probe\n");
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> > +static void eic7700_dwmac_remove(struct platform_device *pdev)
> > +{
> > +	struct eic7700_qos_priv *dwc_priv = get_stmmac_bsp_priv(&pdev->dev);
> > +
> > +	stmmac_pltfr_remove(pdev);
> > +	eic7700_clks_config(dwc_priv, false);
> > +}
> 
> ... and you can remove this function entirely ...
> 
> > +
> > +static const struct of_device_id eic7700_dwmac_match[] = {
> > +	{ .compatible = "eswin,eic7700-qos-eth" },
> > +	{ }
> > +};
> > +MODULE_DEVICE_TABLE(of, eic7700_dwmac_match);
> > +
> > +static struct platform_driver eic7700_dwmac_driver = {
> > +	.probe  = eic7700_dwmac_probe,
> > +	.remove = eic7700_dwmac_remove,
> 
> ... replacing this with stmmac_pltfr_remove().
> 
> Thanks.
> 
> -- 
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Powered by blists - more mailing lists