Message-ID: <7dfcfb04-8a7f-4884-9c91-413a6fb2a56b@lunn.ch>
Date: Fri, 13 Jun 2025 16:32:40 +0200
From: Andrew Lunn <andrew@...n.ch>
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>,
	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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

> +static void emac_shutdown(struct platform_device *pdev)
> +{
> +}

Since it is empty, is it needed?

	Andrew

Powered by blists - more mailing lists