[PATCH net-next v3 2/5] net: spacemit: Add K1 Ethernet MAC

[Vivian Wang]

Hi Maxime,

Thanks for your suggestions.

On 7/2/25 15:17, Maxime Chevallier wrote:
> Hello Vivian,
>
> On Wed, 02 Jul 2025 14:01:41 +0800
> Vivian Wang <wangruikang at iscas.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 at iscas.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 :)

I will remove it next version.

>> +
>> +	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.

Thanks, I will simplify handling of this.

>> +	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()

I will remove next version.

>> +	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.

It seems I had misunderstood the use of of_phy_register_fixed_link, I
will fix this next version.

Thanks,
Vivian "dramforever" Wang

>> +	}
>> +
>> +	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