Date:   Thu, 2 Dec 2021 11:20:47 +0000
From:   Wells Lu 呂芳騰 <wells.lu@...plus.com>
To:     Andrew Lunn <andrew@...n.ch>, Wells Lu <wellslutw@...il.com>
CC:     "davem@...emloft.net" <davem@...emloft.net>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "p.zabel@...gutronix.de" <p.zabel@...gutronix.de>,
        Vincent Shih 施錕鴻 <vincent.shih@...plus.com>
Subject: RE: [PATCH net-next v3 2/2] net: ethernet: Add driver for Sunplus
 SP7021

Hi Andrew,


Thank you very much for your review.


> > +++ b/drivers/net/ethernet/sunplus/spl2sw_define.h
> > @@ -0,0 +1,301 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/* Copyright Sunplus Technology Co., Ltd.
> > + *       All rights reserved.
> > + */
> > +
> > +#ifndef __SPL2SW_DEFINE_H__
> > +#define __SPL2SW_DEFINE_H__
> > +
> > +#include <linux/module.h>
> > +#include <linux/errno.h>
> > +#include <linux/types.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/netdevice.h>
> > +#include <linux/etherdevice.h>
> > +#include <linux/skbuff.h>
> > +#include <linux/ethtool.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/phy.h>
> > +#include <linux/mii.h>
> > +#include <linux/io.h>
> > +#include <linux/dma-mapping.h>
> > +#include <linux/of_address.h>
> > +#include <linux/of_mdio.h>
> > +#include <linux/bitfield.h>
> 
> Please put these in the .c file, and only include those that are needed in each .c file.

Yes, I'll move all "#include ..." to .c files and 
remove unnecessary inclusions next patch.


> > +int spl2sw_rx_descs_init(struct spl2sw_common *comm) {
> > +	struct spl2sw_skb_info *rx_skbinfo;
> > +	struct spl2sw_mac_desc *rx_desc;
> > +	struct sk_buff *skb;
> > +	u32 i, j;
> > +
> > +	for (i = 0; i < RX_DESC_QUEUE_NUM; i++) {
> > +		comm->rx_skb_info[i] = kcalloc(comm->rx_desc_num[i], sizeof(*rx_skbinfo),
> > +					       GFP_KERNEL | GFP_DMA);
> > +		if (!comm->rx_skb_info[i])
> > +			goto mem_alloc_fail;
> > +
> > +		rx_skbinfo = comm->rx_skb_info[i];
> > +		rx_desc = comm->rx_desc[i];
> > +		for (j = 0; j < comm->rx_desc_num[i]; j++) {
> > +			skb = __dev_alloc_skb(comm->rx_desc_buff_size, GFP_KERNEL |
> > +GFP_DMA);
> 
> I generally don't look to closely at buffer handling in drivers. But the __ caught my eye.
> There is a comment:
> 
> /* legacy helper around __netdev_alloc_skb() */
> 
> Since this is legacy, you probably should not be using it. I don't know what you should
> be using though.

__netdev_alloc_skb() is used to set gfp_flags to GFP_DMA or GFP_ATOMIC.

I'll replace __dev_alloc_skb() with dev_alloc_skb().
I'll replace __napi_schedule() with napi_schedule().
I'll review dev_kfree_skb() and dev_kfree_skb_irq().

I found many drivers still use __skb_put().
Should I replace it with skb_put()?


> > +static u32 spl2sw_init_netdev(struct platform_device *pdev, int eth_no,
> > +			      struct net_device **r_ndev)
> > +{
> > +	struct net_device *ndev;
> > +	struct spl2sw_mac *mac;
> > +	char *m_addr_name;
> > +	ssize_t otp_l = 0;
> > +	char *otp_v;
> > +	int ret;
> > +
> > +	m_addr_name = (eth_no == 0) ? "mac_addr0" : "mac_addr1";
> > +
> > +	/* Allocate the devices, and also allocate spl2sw_mac,
> > +	 * we can get it by netdev_priv().
> > +	 */
> > +	ndev = devm_alloc_etherdev(&pdev->dev, sizeof(*mac));
> > +	if (!ndev) {
> > +		*r_ndev = NULL;
> > +		return -ENOMEM;
> > +	}
> > +	SET_NETDEV_DEV(ndev, &pdev->dev);
> > +	ndev->netdev_ops = &netdev_ops;
> > +
> > +	mac = netdev_priv(ndev);
> > +	mac->ndev = ndev;
> > +
> > +	/* Get property 'mac-addr0' or 'mac-addr1' from dts. */
> > +	otp_v = spl2sw_otp_read_mac(&pdev->dev, &otp_l, m_addr_name);
> > +	if (otp_l < ETH_ALEN || IS_ERR_OR_NULL(otp_v)) {
> > +		dev_err(&pdev->dev, "OTP mac %s (len = %zd) is invalid, using default!\n",
> > +			m_addr_name, otp_l);
> > +		otp_l = 0;
> 
> This is not actually an error, in that you keep going and use the default. So dev_info()
> would be better, here and the other calls in this function.

I'll replace the dev_err() with dev_info() in this function next patch.


> > +	} else {
> > +		/* Check if MAC address is valid or not. If not, copy from default. */
> > +		ether_addr_copy(mac->mac_addr, otp_v);
> > +
> > +		/* Byte order of Some samples are reversed. Convert byte order here. */
> > +		spl2sw_check_mac_vendor_id_and_convert(mac->mac_addr);
> > +
> > +		if (!is_valid_ether_addr(mac->mac_addr)) {
> > +			dev_err(&pdev->dev, "Invalid mac in OTP[%s] = %pM, use default!\n",
> > +				m_addr_name, mac->mac_addr);
> > +			otp_l = 0;
> > +		}
> > +	}
> > +	if (otp_l != 6) {
> > +		/* MAC address is invalid. Generate one using random number. */
> > +		ether_addr_copy(mac->mac_addr, spl2sw_def_mac_addr);
> > +		mac->mac_addr[3] = get_random_int() % 256;
> > +		mac->mac_addr[4] = get_random_int() % 256;
> > +		mac->mac_addr[5] = get_random_int() % 256;
> > +	}
> > +
> > +	eth_hw_addr_set(ndev, mac->mac_addr);
> > +	dev_info(&pdev->dev, "HW Addr = %pM\n", mac->mac_addr);
> > +
> > +	ret = register_netdev(ndev);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "Failed to register net device \"%s\"!\n",
> > +			ndev->name);
> > +		free_netdev(ndev);
> > +		*r_ndev = NULL;
> > +		return ret;
> > +	}
> > +	netdev_info(ndev, "Registered net device \"%s\" successfully.\n",
> > +ndev->name);
> 
> netdev_dbg().

Yes, I'll replace the netdev_info() with netdev_dbg() next patch.


> > +	*r_ndev = ndev;
> > +	return 0;
> > +}
> 
> > +
> > +static int spl2sw_probe(struct platform_device *pdev) {
> > +	struct device_node *eth_ports_np;
> > +	struct device_node *port_np;
> > +	struct spl2sw_common *comm;
> > +	struct device_node *phy_np;
> > +	phy_interface_t phy_mode;
> > +	struct net_device *ndev;
> > +	struct spl2sw_mac *mac;
> > +	struct resource *rc;
> > +	int irq, i;
> > +	int ret;
> > +
> > +	if (platform_get_drvdata(pdev))
> > +		return -ENODEV;
> > +
> > +	/* Allocate memory for 'spl2sw_common' area. */
> > +	comm = devm_kzalloc(&pdev->dev, sizeof(*comm), GFP_KERNEL);
> > +	if (!comm)
> > +		return -ENOMEM;
> > +	comm->pdev = pdev;
> > +
> > +	spin_lock_init(&comm->rx_lock);
> > +	spin_lock_init(&comm->tx_lock);
> > +	spin_lock_init(&comm->mdio_lock);
> > +
> > +	/* Get memory resoruce "emac" from dts. */
> 
> resource

I'll fix the two typo 'resoruce' next patch.


> > +	/* Enable clock. */
> > +	clk_prepare_enable(comm->clk);
> > +	udelay(1);
> > +
> > +	reset_control_assert(comm->rstc);
> > +	udelay(1);
> > +	reset_control_deassert(comm->rstc);
> > +	udelay(1);
> > +
> > +	/* Get child node ethernet-ports. */
> > +	eth_ports_np = of_get_child_by_name(pdev->dev.of_node, "ethernet-ports");
> > +	if (!eth_ports_np) {
> > +		dev_err(&pdev->dev, "No ethernet-ports child node found!\n");
> 
> You should disable the clock before returning.

I'll add 'disable the clock' at exit of the function next patch.


> > +		return -ENODEV;
> > +	}
> > +
> > +	for (i = 0; i < MAX_NETDEV_NUM; i++) {
> > +		/* Get port@i of node ethernet-ports. */
> > +		port_np = spl2sw_get_eth_child_node(eth_ports_np, i);
> > +		if (!port_np)
> > +			continue;
> > +
> > +		/* Get phy-mode. */
> > +		if (of_get_phy_mode(port_np, &phy_mode)) {
> > +			dev_err(&pdev->dev, "Failed to get phy-mode property of port@%d!\n",
> > +				i);
> > +			continue;
> > +		}
> > +
> > +		/* Get phy-handle. */
> > +		phy_np = of_parse_phandle(port_np, "phy-handle", 0);
> > +		if (!phy_np) {
> > +			dev_err(&pdev->dev, "Failed to get phy-handle property of port@%d!\n",
> > +				i);
> > +			continue;
> > +		}
> > +
> > +		/* Get address of phy. */
> > +		if (of_property_read_u32(phy_np, "reg", &comm->phy_addr[i])) {
> 
> This does not appear to be used.

I'll remove "get address of phy" statements and also remove
member phy_addr from 'struct spl2sw_commmon' next patch.


> > +			dev_err(&pdev->dev, "Failed to get reg property of phy node!\n");
> > +			continue;
> > +		}
> > +
> > +		if (comm->phy_addr[i] >= PHY_MAX_ADDR - 1) {
> > +			dev_err(&pdev->dev, "Invalid phy address (reg = <%d>)!\n",
> > +				comm->phy_addr[i]);
> > +			continue;
> > +		}
> 
> phylib should validate this.

I'll remove the whole if-statement next patch.


> > +
> > +		if (!comm->mdio_node) {
> > +			comm->mdio_node = of_get_parent(phy_np);
> > +			if (!comm->mdio_node) {
> > +				dev_err(&pdev->dev, "Failed to get mdio_node!\n");
> > +				return -ENODATA;
> > +			}
> > +		}
> 
> This does not look correct. The PHY could be on any MDIO bus. It does not have to be the
> bus of this device. There should not be any need to follow the pointer.

I'll remove the two 'if (!comm->mdio_node)' statements and also remove
member mdio_node from 'struct spl2sw_common' next patch.



> > +static int spl2sw_bit_pos_to_port_num(int n) {
> > +	int i;
> > +
> > +	for (i = 0; i < MAX_NETDEV_NUM; i++) {
> > +		if (n & 1)
> > +			break;
> > +		n >>= 1;
> > +	}
> > +	return i;
> 
> Look at the ffs() helper. But since MAX_NETDEV_NUM is two, the compiler might be smart
> enough to unroll the loop and just use simple logic operations which could be faster.

I'll replace spl2sw_bit_pos_to_num(x) with (ffs(x) - 1) next patch.


> > +void spl2sw_mac_addr_add(struct spl2sw_mac *mac) {
> > +	struct spl2sw_common *comm = mac->comm;
> > +	u32 reg;
> > +
> > +	/* Write 6-octet MAC address. */
> > +	writel((mac->mac_addr[0] << 0) + (mac->mac_addr[1] << 8),
> > +	       comm->l2sw_reg_base + L2SW_W_MAC_15_0);
> > +	writel((mac->mac_addr[2] << 0) + (mac->mac_addr[3] << 8) +
> > +	       (mac->mac_addr[4] << 16) + (mac->mac_addr[5] << 24),
> > +	       comm->l2sw_reg_base + L2SW_W_MAC_47_16);
> > +
> > +	/* Set learn port = cpu_port, aging = 1 */
> > +	reg = MAC_W_CPU_PORT_0 | FIELD_PREP(MAC_W_VID, mac->vlan_id) |
> > +	      FIELD_PREP(MAC_W_AGE, 1) | MAC_W_MAC_CMD;
> > +	writel(reg, comm->l2sw_reg_base + L2SW_WT_MAC_AD0);
> > +
> > +	/* Wait for completing. */
> > +	do {
> > +		reg = readl(comm->l2sw_reg_base + L2SW_WT_MAC_AD0);
> > +		ndelay(10);
> > +		netdev_dbg(mac->ndev, "wt_mac_ad0 = %08x\n", reg);
> > +	} while (!(reg & MAC_W_MAC_DONE));
> 
> linux/iopoll.h

I'll use read_poll_timeout() to replace the while-loops next patch


> > +
> > +	netdev_dbg(mac->ndev, "mac_ad0 = %08x, mac_ad = %08x%04x\n",
> > +		   readl(comm->l2sw_reg_base + L2SW_WT_MAC_AD0),
> > +		   (u32)FIELD_GET(MAC_W_MAC_47_16,
> > +		   readl(comm->l2sw_reg_base + L2SW_W_MAC_47_16)),
> > +		   (u32)FIELD_GET(MAC_W_MAC_15_0,
> > +		   readl(comm->l2sw_reg_base + L2SW_W_MAC_15_0))); }
> > +
> > +void spl2sw_mac_addr_del(struct spl2sw_mac *mac) {
> > +	struct spl2sw_common *comm = mac->comm;
> > +	u32 reg;
> > +
> > +	/* Write 6-octet MAC address. */
> > +	writel((mac->mac_addr[0] << 0) + (mac->mac_addr[1] << 8),
> > +	       comm->l2sw_reg_base + L2SW_W_MAC_15_0);
> > +	writel((mac->mac_addr[2] << 0) + (mac->mac_addr[3] << 8) +
> > +	       (mac->mac_addr[4] << 16) + (mac->mac_addr[5] << 24),
> > +	       comm->l2sw_reg_base + L2SW_W_MAC_47_16);
> > +
> > +	/* Set learn port = lan_port0 and aging = 0
> > +	 * to wipe (age) out the entry.
> > +	 */
> > +	reg = MAC_W_LAN_PORT_0 | FIELD_PREP(MAC_W_VID, mac->vlan_id) | MAC_W_MAC_CMD;
> > +	writel(reg, comm->l2sw_reg_base + L2SW_WT_MAC_AD0);
> > +
> > +	/* Wait for completing. */
> > +	do {
> > +		reg = readl(comm->l2sw_reg_base + L2SW_WT_MAC_AD0);
> > +		ndelay(10);
> > +		netdev_dbg(mac->ndev, "wt_mac_ad0 = %08x\n", reg);
> > +	} while (!(reg & MAC_W_MAC_DONE));
> 
> Here as well. Anywhere you need to wait for some sort of completion, it is best you use
> these helpers. They will also do a timeout, just in case the hardware dies.

I'll use read_poll_timeout() to replace the while-loops next patch.
Totally, 5 while-loops will be modified in 'spl2sw_mac.c' next patch.


> > +static int spl2sw_mii_read(struct mii_bus *bus, int addr, int regnum)
> > +{
> > +	struct spl2sw_common *comm = bus->priv;
> > +	int ret;
> > +
> > +	if (regnum & MII_ADDR_C45)
> > +		return -EOPNOTSUPP;
> > +
> > +	ret = spl2sw_mdio_access(comm, SPL2SW_MDIO_READ_CMD, addr, regnum, 0);
> > +	if (ret < 0)
> > +		return -EOPNOTSUPP;
> 
> spl2sw_mdio_access() returns an error code, -ETIMEDOUT. So us it.

Yes, I'll use the 'return value' of spl2sw_mdio_access() next patch.


> > +u32 spl2sw_mdio_init(struct spl2sw_common *comm) {
> > +	struct mii_bus *mii_bus;
> > +	int ret;
> > +
> > +	mii_bus = devm_mdiobus_alloc(&comm->pdev->dev);
> > +	if (!mii_bus)
> > +		return -ENOMEM;
> > +
> > +	mii_bus->name = "sunplus_mii_bus";
> > +	mii_bus->parent = &comm->pdev->dev;
> > +	mii_bus->priv = comm;
> > +	mii_bus->read = spl2sw_mii_read;
> > +	mii_bus->write = spl2sw_mii_write;
> > +	snprintf(mii_bus->id, MII_BUS_ID_SIZE, "%s-mii",
> > +dev_name(&comm->pdev->dev));
> > +
> > +	ret = of_mdiobus_register(mii_bus, comm->mdio_node);
> 
> Here you should be looking into the device tree to find the mdio node and passing it.

Yes, I'll use of_get_child_by_name() to get mdio child node next patch.


> > +	if (ret) {
> > +		dev_err(&comm->pdev->dev, "Failed to register mdiobus!\n");
> > +		return ret;
> > +	}
> > +
> > +	comm->mii_bus = mii_bus;
> > +	return ret;
> > +}
> 
> > +int spl2sw_phy_connect(struct spl2sw_common *comm) {
> > +	struct phy_device *phydev;
> > +	struct net_device *ndev;
> > +	struct spl2sw_mac *mac;
> > +	int i;
> > +
> > +	for (i = 0; i < MAX_NETDEV_NUM; i++)
> > +		if (comm->ndev[i]) {
> > +			ndev = comm->ndev[i];
> > +			mac = netdev_priv(ndev);
> > +			phydev = of_phy_connect(ndev, mac->phy_node, spl2sw_mii_link_change,
> > +						0, mac->phy_mode);
> > +			if (!phydev)
> > +				return -ENODEV;
> > +
> > +			linkmode_copy(phydev->advertising, phydev->supported);
> 
> There should not be any need to do this.

I'll remove the linkmode_copy(...) statement next patch.

I printed out the value of 'supported' and 'advertising'.
'supported' shows PHY device supports Pause and AsymPause (0x62cf).
But 'advertising' shows PHY device does not support Pause or AsymPause (0x02cf).
Is this correct?

How to let link partner know local node supports 
Pause & AsymPause (flow control)?


> > +
> > +			/* Enable polling mode */
> > +			phydev->irq = PHY_POLL;
> 
> And that should be the default, so no need to set it.

Yes, I'll remove the statements next patch.


> > +		}
> > +
> > +	return 0;
> > +}
> 
> This is looking a lot better.
> 
>      Andrew

Can I ask a question?
Will mii_read() and mii_write() be called in interrupt context?
Should I replace read_poll_timeout() with read_poll_timeout_atomic()
in spl2sw_mdio_access()?


Best regards,
Wells

Powered by blists - more mailing lists