Message-ID: <c60e9579-c261-41e3-ad5a-f96ce6d76820@iscas.ac.cn>
Date: Mon, 16 Jun 2025 11:04:36 +0800
From: Vivian Wang <wangruikang@...as.ac.cn>
To: Andrew Lunn <andrew@...n.ch>
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>,
 Paul Walmsley <paul.walmsley@...ive.com>, Palmer Dabbelt
 <palmer@...belt.com>, Albert Ou <aou@...s.berkeley.edu>,
 Alexandre Ghiti <alex@...ti.fr>, Richard Cochran <richardcochran@...il.com>,
 Philipp Zabel <p.zabel@...gutronix.de>, Russell King
 <linux@...linux.org.uk>, Vivian Wang <uwu@...m.page>,
 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 2/4] net: spacemit: Add K1 Ethernet MAC

Hi Andrew,

Thanks for your review and suggestions.

On 6/13/25 22:32, Andrew Lunn wrote:
>> +static inline void emac_wr(struct emac_priv *priv, u32 reg, u32 val)
>> +{
>> +	writel(val, priv->iobase + reg);
>> +}
>> +
>> +static inline int emac_rd(struct emac_priv *priv, u32 reg)
>> +{
>> +	return readl(priv->iobase + reg);
>> +}
> I only took a very quick look at the code. I'm sure there are more
> issues....
>
> Please do not user inline functions in a .c file. Let the compiler
> decide.
I will remove "inline" in next version.
>> +static void emac_alloc_rx_desc_buffers(struct emac_priv *priv);
>> +static int emac_phy_connect(struct net_device *dev);
>> +static void emac_tx_timeout_task(struct work_struct *work);
> No forward declarations. Move the code around so they are not needed.
I will reorganize in next version.
>> +static int emac_ioctl(struct net_device *ndev, struct ifreq *rq, int cmd)
>> +{
>> +	int ret = -EOPNOTSUPP;
>> +
>> +	if (!netif_running(ndev))
>> +		return -EINVAL;
>> +
>> +	switch (cmd) {
>> +	case SIOCGMIIPHY:
>> +	case SIOCGMIIREG:
>> +	case SIOCSMIIREG:
> There is no need to test for these values. Just call phy_mii_ioctl()
> and it will only act on IOCTLs it knows.
I will simplify in next version.
>> +		if (!ndev->phydev)
>> +			return -EINVAL;
>> +		ret = phy_mii_ioctl(ndev->phydev, rq, cmd);
>> +		break;
>> +	default:
>> +		break;
>> +	}
>> +
>> +	return ret;
>> +}
>> +static int emac_up(struct emac_priv *priv)
>> +{
>> +	struct platform_device *pdev = priv->pdev;
>> +	struct net_device *ndev = priv->ndev;
>> +	int ret;
>> +
>> +#ifdef CONFIG_PM_SLEEP
>> +	pm_runtime_get_sync(&pdev->dev);
>> +#endif
> You don't need this #ifdef, there is a stub function is PM_SLEEP is
> not enabled.
I will remove "#ifdef" in next version.
>> +
>> +	ret = emac_phy_connect(ndev);
>> +	if (ret) {
>> +		dev_err(&pdev->dev, "emac_phy_connect failed\n");
>> +		goto err;
>> +	}
>> +
>> +	emac_init_hw(priv);
>> +
>> +	emac_set_mac_addr(priv, ndev->dev_addr);
>> +	emac_configure_tx(priv);
>> +	emac_configure_rx(priv);
>> +
>> +	emac_alloc_rx_desc_buffers(priv);
>> +
>> +	if (ndev->phydev)
>> +		phy_start(ndev->phydev);
> Is it possible to not have a PHY? emac_phy_connect() seems to return
> an error if it cannot find one.
>
I will remove unnecessary if (ndev->phydev) checks in next version.
>> +static int emac_down(struct emac_priv *priv)
>> +{
>> +	struct platform_device *pdev = priv->pdev;
>> +	struct net_device *ndev = priv->ndev;
>> +
>> +	netif_stop_queue(ndev);
>> +
>> +	if (ndev->phydev) {
>> +		phy_stop(ndev->phydev);
>> +		phy_disconnect(ndev->phydev);
>> +	}
>> +
>> +	priv->link = false;
>> +	priv->duplex = DUPLEX_UNKNOWN;
>> +	priv->speed = SPEED_UNKNOWN;
>> +
>> +	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);
>> +	netif_carrier_off(ndev);
> phylib will of done this when phy_stop() is called. Let phylib manage
> the carrier. The only thing you probably need is netif_carrier_off()
> in probe().
I will remove netif_carrier_off here and add to probe.
>> +static int emac_change_mtu(struct net_device *ndev, int mtu)
>> +{
>> +	struct emac_priv *priv = netdev_priv(ndev);
>> +	u32 frame_len;
>> +
>> +	if (netif_running(ndev)) {
>> +		netdev_err(ndev, "must be stopped to change MTU\n");
>> +		return -EBUSY;
>> +	}
>> +
>> +	frame_len = mtu + ETHERNET_HEADER_SIZE + ETHERNET_FCS_SIZE;
>> +
>> +	if (frame_len < MINIMUM_ETHERNET_FRAME_SIZE ||
>> +	    frame_len > EMAC_RX_BUF_4K) {
>> +		netdev_err(ndev, "Invalid MTU setting\n");
>> +		return -EINVAL;
>> +	}
> If you set ndev->mtu_max and ndev->mtu_min, the core will check this
> for you.
>
I will remove this check and use mtu_{min,max} instead.
>> +static void emac_reset(struct emac_priv *priv)
>> +{
>> +	if (!test_and_clear_bit(EMAC_RESET_REQUESTED, &priv->state))
>> +		return;
>> +	if (test_bit(EMAC_DOWN, &priv->state))
>> +		return;
>> +
>> +	netdev_err(priv->ndev, "Reset controller\n");
>> +
>> +	rtnl_lock();
>> +	netif_trans_update(priv->ndev);
>> +	while (test_and_set_bit(EMAC_RESETING, &priv->state))
>> +		usleep_range(1000, 2000);
> Don't do endless loops waiting for the hardware. It may never
> happen. Please use something from iopoll.h
It seems that I had misunderstood the original code here. I will
simplify the logic here in next version.
>> +static int emac_mii_read(struct mii_bus *bus, int phy_addr, int regnum)
>> +{
>> +	struct emac_priv *priv = bus->priv;
>> +	u32 cmd = 0;
>> +	u32 val;
>> +
>> +	cmd |= phy_addr & 0x1F;
>> +	cmd |= (regnum & 0x1F) << 5;
>> +	cmd |= MREGBIT_START_MDIO_TRANS | MREGBIT_MDIO_READ_WRITE;
>> +
>> +	emac_wr(priv, MAC_MDIO_DATA, 0x0);
>> +	emac_wr(priv, MAC_MDIO_CONTROL, cmd);
>> +
>> +	if (readl_poll_timeout(priv->iobase + MAC_MDIO_CONTROL, val,
>> +			       !((val >> 15) & 0x1), 100, 10000))
>> +		return -EBUSY;
> readl_poll_timeout() returns an error code. Don't replace it.
I will fix it next version.
>> +static void emac_adjust_link(struct net_device *dev)
>> +{
>> +	struct emac_priv *priv = netdev_priv(dev);
>> +	struct phy_device *phydev = dev->phydev;
>> +	bool link_changed = false;
>> +	u32 ctrl;
>> +
>> +	if (!phydev)
>> +		return;
> How does that happen?
>
>> +	if (phydev->link) {
>> +		ctrl = emac_rd(priv, MAC_GLOBAL_CONTROL);
>> +
>> +		/* Update duplex and speed from PHY */
>> +
>> +		if (phydev->duplex != priv->duplex) {
>> +			link_changed = true;
>> +
>> +			if (!phydev->duplex)
>> +				ctrl &= ~MREGBIT_FULL_DUPLEX_MODE;
>> +			else
>> +				ctrl |= MREGBIT_FULL_DUPLEX_MODE;
>> +			priv->duplex = phydev->duplex;
>> +		}
>> +
>> +		if (phydev->speed != priv->speed) {
>> +			link_changed = true;
>> +
>> +			ctrl &= ~MREGBIT_SPEED;
>> +
>> +			switch (phydev->speed) {
>> +			case SPEED_1000:
>> +				ctrl |= MREGBIT_SPEED_1000M;
>> +				break;
>> +			case SPEED_100:
>> +				ctrl |= MREGBIT_SPEED_100M;
>> +				break;
>> +			case SPEED_10:
>> +				ctrl |= MREGBIT_SPEED_10M;
>> +				break;
>> +			default:
>> +				netdev_err(dev, "Unknown speed: %d\n",
>> +					   phydev->speed);
>> +				phydev->speed = SPEED_UNKNOWN;
>> +				break;
>> +			}
>> +
>> +			if (phydev->speed != SPEED_UNKNOWN)
>> +				priv->speed = phydev->speed;
>> +		}
>> +
>> +		emac_wr(priv, MAC_GLOBAL_CONTROL, ctrl);
>> +
>> +		if (!priv->link) {
>> +			priv->link = true;
>> +			link_changed = true;
>> +		}
>> +	} else if (priv->link) {
>> +		priv->link = false;
>> +		link_changed = true;
>> +		priv->duplex = DUPLEX_UNKNOWN;
>> +		priv->speed = SPEED_UNKNOWN;
>> +	}
>> +
>> +	if (link_changed)
>> +		phy_print_status(phydev);
> Can this ever be false?
>
I will remove these checks in next version.
>> +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 (IS_ERR_OR_NULL(phydev)) {
>> +		dev_err(dev, "Could not attach to PHY\n");
>> +		ret = phydev ? PTR_ERR(phydev) : -ENODEV;
>> +		goto err_node_put;
>> +	}
>> +
>> +	dev_info(dev, "%s: attached to PHY (UID 0x%x) Link = %d\n", ndev->name,
>> +		 phydev->phy_id, phydev->link);
> Don't spam the log. Only output something if something unexpected
> happens, an error etc.
I will remove this in next version.
>> +static int emac_mdio_init(struct emac_priv *priv)
>> +{
>> +	struct device *dev = &priv->pdev->dev;
>> +	struct device_node *mii_np;
>> +	struct mii_bus *mii;
>> +	int ret;
>> +
>> +	mii_np = of_get_child_by_name(dev->of_node, "mdio-bus");
>> +	if (!mii_np) {
>> +		if (of_phy_is_fixed_link(dev->of_node)) {
>> +			if ((of_phy_register_fixed_link(dev->of_node) < 0))
>> +				return -ENODEV;
>> +
>> +			return 0;
>> +		}
>> +
>> +		dev_err(dev, "no %s child node found", "mdio-bus");
> Why is that an error?
I will remove this in the next version.
>> +		return -ENODEV;
>> +	}
>> +
>> +	if (!of_device_is_available(mii_np)) {
>> +		ret = -ENODEV;
>> +		goto err_put_node;
>> +	}
>> +
>> +	mii = devm_mdiobus_alloc(dev);
>> +	priv->mii = mii;
>> +
>> +	if (!mii) {
>> +		ret = -ENOMEM;
>> +		goto err_put_node;
>> +	}
>> +	mii->priv = priv;
>> +	mii->name = "emac mii";
>> +	mii->read = emac_mii_read;
>> +	mii->write = emac_mii_write;
>> +	mii->parent = dev;
>> +	mii->phy_mask = 0xffffffff;
>> +	snprintf(mii->id, MII_BUS_ID_SIZE, "%s", priv->pdev->name);
>> +
>> +	ret = devm_of_mdiobus_register(dev, mii, mii_np);
>> +	if (ret) {
>> +		dev_err_probe(dev, ret, "Failed to register mdio bus.\n");
>> +		goto err_put_node;
>> +	}
>> +
>> +	priv->phy = phy_find_first(mii);
>> +	if (!priv->phy) {
>> +		dev_err(dev, "no PHY found\n");
>> +		ret = -ENODEV;
> Please don't use phy_find_first(). Use phy-handle to point to the phy.
>
I will remove this next version.
>> +static void emac_ethtool_get_regs(struct net_device *dev,
>> +				  struct ethtool_regs *regs, void *space)
>> +{
>> +	struct emac_priv *priv = netdev_priv(dev);
>> +	u32 *reg_space = space;
>> +	int i;
>> +
>> +	regs->version = 1;
>> +
>> +	memset(reg_space, 0x0, EMAC_REG_SPACE_SIZE);
> Is that needed?
I will remove in next version.
>> +static int emac_get_link_ksettings(struct net_device *ndev,
>> +				   struct ethtool_link_ksettings *cmd)
>> +{
>> +	if (!ndev->phydev)
>> +		return -ENODEV;
>> +
>> +	phy_ethtool_ksettings_get(ndev->phydev, cmd);
> phy_ethtool_get_link_ksettings().
I will use phy_ethtool_{get,set}_link_ksettings in next version.
>> +	if (priv->tx_delay > EMAC_MAX_DELAY_PS) {
>> +		dev_err(&pdev->dev, "tx-internal-delay-ps delay too large, clamped");
> Please return -EINVAL;
>
>> +		priv->tx_delay = EMAC_MAX_DELAY_PS;
>> +	}
>> +
>> +	if (priv->rx_delay > EMAC_MAX_DELAY_PS) {
>> +		dev_err(&pdev->dev, "rx-internal-delay-ps delay too large, clamped");
> and here. The device tree is broken, and we want the developer to
> notice and fix it. The easiest way to do that is to refuse to load the
> driver.
>
>> +		priv->rx_delay = EMAC_MAX_DELAY_PS;
>> +	}
>> +
>> +	if (priv->tx_delay || priv->rx_delay) {
> Why the if () ?
>
>> +		priv->tx_delay = delay_ps_to_unit(priv->tx_delay);
>> +		priv->rx_delay = delay_ps_to_unit(priv->rx_delay);
>> +
>> +		/* Show rounded result here for convenience */
>> +		dev_info(&pdev->dev,
>> +			 "MAC internal delay: TX: %u ps, RX: %u ps",
>> +			 delay_unit_to_ps(priv->tx_delay),
>> +			 delay_unit_to_ps(priv->rx_delay));
> Please don't. 
I will simplify the checking logic and remove the print in next version.
>> +static void emac_shutdown(struct platform_device *pdev)
>> +{
>> +}

I will get rid of it in next version.

Thanks again for the review.

Regards,
Vivian "dramforever" Wang

> Since it is empty, is it needed?
>
> 	Andrew

Powered by blists - more mailing lists