Message-ID: <20250702091708.7d459213@fedora.home>
Date: Wed, 2 Jul 2025 09:17:08 +0200
From: Maxime Chevallier <maxime.chevallier@...tlin.com>
To: Vivian Wang <wangruikang@...as.ac.cn>
Cc: Andrew Lunn <andrew+netdev@...n.ch>, "David S. Miller"
 <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski
 <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, Rob Herring
 <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Yixun Lan <dlan@...too.org>, Philipp Zabel
 <p.zabel@...gutronix.de>, Paul Walmsley <paul.walmsley@...ive.com>, Palmer
 Dabbelt <palmer@...belt.com>, Albert Ou <aou@...s.berkeley.edu>, Alexandre
 Ghiti <alex@...ti.fr>, Vivian Wang <uwu@...m.page>, Lukas Bulwahn
 <lukas.bulwahn@...hat.com>, Geert Uytterhoeven <geert+renesas@...der.be>,
 Parthiban Veerasooran <Parthiban.Veerasooran@...rochip.com>,
 netdev@...r.kernel.org, devicetree@...r.kernel.org,
 linux-riscv@...ts.infradead.org, spacemit@...ts.linux.dev,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next v3 2/5] net: spacemit: Add K1 Ethernet MAC

Hello Vivian,

On Wed, 02 Jul 2025 14:01:41 +0800
Vivian Wang <wangruikang@...as.ac.cn> wrote:

> The Ethernet MACs found on SpacemiT K1 appears to be a custom design
> that only superficially resembles some other embedded MACs. SpacemiT
> refers to them as "EMAC", so let's just call the driver "k1_emac".
> 
> This driver is based on "k1x-emac" in the same directory in the vendor's
> tree [1]. Some debugging tunables have been fixed to vendor-recommended
> defaults, and PTP support is not included yet.
> 
> [1]: https://github.com/spacemit-com/linux-k1x
> 
> Signed-off-by: Vivian Wang <wangruikang@...as.ac.cn>

I have a handful of tiny comments, the rest looks fine by me !

> +static int emac_phy_connect(struct net_device *ndev)
> +{
> +	struct emac_priv *priv = netdev_priv(ndev);
> +	struct device *dev = &priv->pdev->dev;
> +	struct phy_device *phydev;
> +	struct device_node *np;
> +	int ret;
> +
> +	ret = of_get_phy_mode(dev->of_node, &priv->phy_interface);
> +	if (ret) {
> +		dev_err(dev, "No phy-mode found");
> +		return ret;
> +	}
> +
> +	np = of_parse_phandle(dev->of_node, "phy-handle", 0);
> +	if (!np && of_phy_is_fixed_link(dev->of_node))
> +		np = of_node_get(dev->of_node);
> +
> +	if (!np) {
> +		dev_err(dev, "No PHY specified");
> +		return -ENODEV;
> +	}
> +
> +	ret = emac_phy_interface_config(priv);
> +	if (ret)
> +		goto err_node_put;
> +
> +	phydev = of_phy_connect(ndev, np, &emac_adjust_link, 0,
> +				priv->phy_interface);
> +	if (!phydev) {
> +		dev_err(dev, "Could not attach to PHY\n");
> +		ret = -ENODEV;
> +		goto err_node_put;
> +	}
> +
> +	phydev->mac_managed_pm = true;
> +
> +	ndev->phydev = phydev;

of_phy_connect() eventually calls phy_attach_direct(), which sets
ndev->phydev, so you don't need to do it here :)

> +
> +	emac_update_delay_line(priv);
> +
> +err_node_put:
> +	of_node_put(np);
> +	return ret;
> +}

[ ... ]

> +static int emac_down(struct emac_priv *priv)
> +{
> +	struct platform_device *pdev = priv->pdev;
> +	struct net_device *ndev = priv->ndev;
> +
> +	netif_stop_queue(ndev);
> +
> +	phy_stop(ndev->phydev);

phy_disconnect() will call phy_stop() for you, you can remove it.

> +	phy_disconnect(ndev->phydev);
> +
> +	emac_wr(priv, MAC_INTERRUPT_ENABLE, 0x0);
> +	emac_wr(priv, DMA_INTERRUPT_ENABLE, 0x0);
> +
> +	free_irq(priv->irq, ndev);
> +
> +	napi_disable(&priv->napi);
> +
> +	emac_reset_hw(priv);
> +
> +	pm_runtime_put_sync(&pdev->dev);
> +	return 0;
> +}
> +

[ ... ]

> +static int emac_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct reset_control *reset;
> +	struct net_device *ndev;
> +	struct emac_priv *priv;
> +	int ret;
> +
> +	ndev = devm_alloc_etherdev(dev, sizeof(struct emac_priv));
> +	if (!ndev)
> +		return -ENOMEM;
> +
> +	ndev->hw_features = NETIF_F_SG;
> +	ndev->features |= ndev->hw_features;
> +
> +	ndev->min_mtu = ETH_MIN_MTU;

This should already be the default value when using
devm_alloc_etherdev()

> +	ndev->max_mtu = EMAC_RX_BUF_4K - (ETH_HLEN + ETH_FCS_LEN);
> +
> +	priv = netdev_priv(ndev);
> +	priv->ndev = ndev;
> +	priv->pdev = pdev;
> +	platform_set_drvdata(pdev, priv);
> +	priv->hw_stats = devm_kzalloc(dev, sizeof(*priv->hw_stats), GFP_KERNEL);
> +	if (!priv->hw_stats) {
> +		dev_err(dev, "Failed to allocate memory for stats\n");
> +		ret = -ENOMEM;
> +		goto err;
> +	}
> +
> +	ret = emac_config_dt(pdev, priv);
> +	if (ret < 0) {
> +		dev_err(dev, "Configuration failed\n");
> +		goto err;
> +	}
> +
> +	ndev->watchdog_timeo = 5 * HZ;
> +	ndev->base_addr = (unsigned long)priv->iobase;
> +	ndev->irq = priv->irq;
> +
> +	ndev->ethtool_ops = &emac_ethtool_ops;
> +	ndev->netdev_ops = &emac_netdev_ops;
> +
> +	devm_pm_runtime_enable(&pdev->dev);
> +
> +	priv->bus_clk = devm_clk_get_enabled(&pdev->dev, NULL);
> +	if (IS_ERR(priv->bus_clk)) {
> +		ret = dev_err_probe(dev, PTR_ERR(priv->bus_clk),
> +				    "Failed to get clock\n");
> +		goto err;
> +	}
> +
> +	reset = devm_reset_control_get_optional_exclusive_deasserted(&pdev->dev,
> +								     NULL);
> +	if (IS_ERR(reset)) {
> +		ret = dev_err_probe(dev, PTR_ERR(reset),
> +				    "Failed to get reset\n");
> +		goto err;
> +	}
> +
> +	emac_sw_init(priv);
> +
> +	if (of_phy_is_fixed_link(dev->of_node)) {
> +		ret = of_phy_register_fixed_link(dev->of_node);
> +		if (ret) {
> +			dev_err_probe(dev, ret,
> +				      "Failed to register fixed-link");
> +			goto err_timer_delete;
> +		}

It looks like you're missing the calls to:

  of_phy_deregister_fixed_link()

in the error path here as well as in the .remove() function.

> +	}
> +
> +	ret = emac_mdio_init(priv);
> +	if (ret)
> +		goto err_timer_delete;
> +
> +	SET_NETDEV_DEV(ndev, &pdev->dev);
> +
> +	ret = devm_register_netdev(dev, ndev);
> +	if (ret) {
> +		dev_err(dev, "devm_register_netdev failed\n");
> +		goto err_timer_delete;
> +	}
> +
> +	netif_napi_add(ndev, &priv->napi, emac_rx_poll);
> +	netif_carrier_off(ndev);
> +
> +	return 0;
> +
> +err_timer_delete:
> +	timer_delete_sync(&priv->txtimer);
> +err:
> +	return ret;
> +}

Maxime

Powered by blists - more mailing lists