Message-ID: <64626e48052c4fba9057369060bfbc84@sphcmbx02.sunplus.com.tw>
Date:   Fri, 5 Nov 2021 11:25:53 +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>
Subject: RE: [PATCH 2/2] net: ethernet: Add driver for Sunplus SP7021

Hi,

Thanks a lot for your review.

> > +config NET_VENDOR_SUNPLUS
> > +	tristate "Sunplus Dual 10M/100M Ethernet (with L2 switch) devices"
> 
> The "with L2 Switch" is causing lots of warning bells to ring for me.
> 
> I don't see any references to switchdev or DSA in this driver. How is the
> switch managed? There have been a few examples in the past of similar two
> port switches being first supported in Dual MAC mode. Later trying to
> actually use the switch in the Linux was always ran into problems, and
> basically needed a new driver. So i want to make sure you don't have this
> problem.
> 
> In the Linux world, Ethernet switches default to having there
> ports/interfaces separated. This effectively gives you your dual MAC mode by
> default.  You then create a Linux bridge, and add the ports/interfaces to the
> bridge. switchdev is used to offload the bridge, telling the hardware to
> enable the L2 switch between the ports.
> 
> So you don't need the mode parameter in DT. switchdev tells you this.
> Switchdev gives user space access to the address table etc.

The L2 switch of Ethernet of SP7021 is not used to forward packets 
between two network interfaces.

Sunplus Dual Ethernet devices consists of one CPU port, two LAN 
ports, and a L2 switch. L2 switch is a circuitry which receives packets 
from CPU or LAN ports and then forwards them other ports. Rules of 
forwarding packets are set by driver.

Ethernet driver of SP7021 of Sunplus supports 3 operation modes:
  - Dual NIC mode
  - An NIC with two LAN ports mode (daisy-chain mode)
  - An NIC with two LAN ports mode 2

Dual NIC mode
Ethernet driver creates two net-device interfaces (eg: eth0 and eth1). 
Each has its dedicated LAN port. For example, LAN port 0 is for 
net-device interface eth0. LAN port 1 is for net-device interface 
eth1. Packets from LAN port 0 will be always forwarded to eth0 and 
vice versa by L2 switch. Similarly, packets from LAN port 1 will be 
always forwarded to eth1 and vice versa by L2 switch. Packets will 
never be forwarded between two LAN ports, or between eth0 and 
LAN port 1, or between eth1 and LAN port 0. The two network 
devices work independently.

An NIC with two LAN ports mode (daisy-chain mode)
Ethernet driver creates one net-device interface (eg: eth0), but the 
net-device interface has two LAN ports. In this mode, a packet from 
one LAN port will be either forwarded to net-device interface (eht0) 
if its destination address matches MAC address of net-device 
interface (eth0), or forwarded to other LAN port. A packet from 
net-device interface (eth0) will be forwarded to a LAN port if its 
destination address is learnt by L2 switch, or forwarded to both 
LAN ports if its destination has not been learnt yet.

An NIC with two LAN ports mode 2
This mode is similar to “An NIC with two LAN ports mode”. The 
difference is that a packet from net-device interface (eth0) will be 
always forwarded to both LAN ports. Learning function of L2 switch 
is turned off in this mode. This means L2 switch will never learn the 
source address of a packet. So, it always forward packets to both 
LAN ports. This mode works like you have 2-port Ethernet hub.

> 
> > +obj-$(CONFIG_NET_VENDOR_SUNPLUS) += sp_l2sw.o
> ...
> > +struct l2sw_common {
> 
> Please change your prefix. l2sw is a common prefix, there are other silicon
> vendors using l2sw. I would suggest sp_l2sw or spl2sw.

Ok, I'll modify two struct names in next patch as shown below:
l2sw_common --> sp_common
l2sw_mac --> sp_mac

Should I also modify prefix of file name?

> > +static int ethernet_do_ioctl(struct net_device *net_dev, struct ifreq
> > +*ifr, int cmd) {
> > +	struct l2sw_mac *mac = netdev_priv(net_dev);
> > +	struct l2sw_common *comm = mac->comm;
> > +	struct mii_ioctl_data *data = if_mii(ifr);
> > +	unsigned long flags;
> > +
> > +	pr_debug(" if = %s, cmd = %04x\n", ifr->ifr_ifrn.ifrn_name, cmd);
> > +	pr_debug(" phy_id = %d, reg_num = %d, val_in = %04x\n",
> data->phy_id,
> > +		 data->reg_num, data->val_in);
> 
> You should not be using any of the pr_ functions. You have a net_dev, so
> netdev_dbg().

Ok, I will replace pr_xxx() functions with netdev_xxx() functions 
where 'net_dev' is available in next patch.


> > +
> > +	// Check parameters' range.
> > +	if ((cmd == SIOCGMIIREG) || (cmd == SIOCSMIIREG)) {
> > +		if (data->reg_num > 31) {
> > +			pr_err(" reg_num (= %d) excesses range!\n",
> (int)data->reg_num);
> 
> Don't spam the kernel log for things like this.

Ok, I'll remove the pr_err() line in next patch.


> > +			return -EINVAL;
> > +		}
> > +	}
> > +
> > +	switch (cmd) {
> > +	case SIOCGMIIPHY:
> > +		if (comm->dual_nic && (strcmp(ifr->ifr_ifrn.ifrn_name, "eth1") ==
> > +0))
> 
> You cannot rely on the name, systemd has probably renamed it. If you have
> using phylib correctly, net_dev->phydev is what you want.

Ok, I'll use name of the second net device to do compare, 
instead of using fixed string "eth1", in next patch.


> 
> 
> > +			return comm->phy2_addr;
> > +		else
> > +			return comm->phy1_addr;
> > +
> > +	case SIOCGMIIREG:
> > +		spin_lock_irqsave(&comm->ioctl_lock, flags);
> > +		data->val_out = mdio_read(data->phy_id, data->reg_num);
> > +		spin_unlock_irqrestore(&comm->ioctl_lock, flags);
> > +		pr_debug(" val_out = %04x\n", data->val_out);
> > +		break;
> > +
> > +	case SIOCSMIIREG:
> > +		spin_lock_irqsave(&comm->ioctl_lock, flags);
> > +		mdio_write(data->phy_id, data->reg_num, data->val_in);
> > +		spin_unlock_irqrestore(&comm->ioctl_lock, flags);
> > +		break;
> > +
> 
> You should be using phylink_mii_ioctl() or phy_mii_ioctl().
> 
> You locking is also suspect.

Ok, I'll use phy_mii_ioctl() for SIOCSMIIREG and SIOCGMIIREG commands 
and remove spin_lock in next patch.


> > +static int ethernet_change_mtu(struct net_device *ndev, int new_mtu)
> > +{
> > +	if (netif_running(ndev))
> > +		return -EBUSY;
> > +
> > +	if (new_mtu < 68 || new_mtu > ETH_DATA_LEN)
> > +		return -EINVAL;
> 
> The core will do this for you, if you set the values in the ndev correct at
> probe time.

Ok, I'll remove the whole function ethernet_change_mtu () in next patch 
as the core will do everything for changing mtu.


> > +
> > +	ndev->mtu = new_mtu;
> > +
> > +	return 0;
> > +}
> > +
> 
> 
> > +static int mdio_access(u8 op_cd, u8 dev_reg_addr, u8 phy_addr, u32
> > +wdata) {
> > +	u32 value, time = 0;
> > +
> > +	HWREG_W(phy_cntl_reg0, (wdata << 16) | (op_cd << 13) |
> (dev_reg_addr << 8) | phy_addr);
> > +	wmb();			// make sure settings are effective.
> 
> That suggests you are using the wrong macros to access the registers.

Ok, I'll modify HWREG_W() and HWREG_R() and remove wmb().


> > +	do {
> > +		if (++time > MDIO_RW_TIMEOUT_RETRY_NUMBERS) {
> > +			pr_err(" mdio failed to operate!\n");
> > +			time = 0;
> > +		}
> > +
> > +		value = HWREG_R(phy_cntl_reg1);
> > +	} while ((value & 0x3) == 0);
> 
> include/linux/iopoll.h.
> 
> 
> > +	if (time == 0)
> > +		return -1;
> 
> -ETIMDEOUT. One of the advantages of iopoll.h is that reusing code  avoids
> issues like this.

Ok, I'll replace the do-while loop with read_poll_timeout().


> > +u32 mdio_read(u32 phy_id, u16 regnum) {
> > +	int ret;
> 
> Please check for C45 and return -EOPNOTSUPP.

Ok, I'll modify mdio_read() to return -EOPNOTSUPP 
when error (time-out) occurs.

> > +
> > +	ret = mdio_access(MDIO_READ_CMD, regnum, phy_id, 0);
> > +	if (ret < 0)
> > +		return -EIO;
> > +
> > +	return ret;
> > +}
> > +
> > +u32 mdio_write(u32 phy_id, u32 regnum, u16 val) {
> > +	int ret;
> > +
> 
> Please check for C45 and return -EOPNOTSUPP.

Ok, I'll modify mdio_read() to return -EOPNOTSUPP 
when error (time-out) occurs.


> > +
> > +inline void tx_trigger(void)
> 
> No inline functions in C code. Let the compiler decide.

Ok, I'll remove 'inline' for all functions in next patch.


> > +int phy_cfg(struct l2sw_mac *mac)
> > +{
> > +	// Bug workaround:
> > +	// Flow-control of phy should be enabled. L2SW IP flow-control will refer
> > +	// to the bit to decide to enable or disable flow-control.
> > +	mdio_write(mac->comm->phy1_addr, 4,
> mdio_read(mac->comm->phy1_addr, 4) | (1 << 10));
> > +	mdio_write(mac->comm->phy2_addr, 4,
> mdio_read(mac->comm->phy2_addr,
> > +4) | (1 << 10));
> 
> This should be in the PHY driver. The MAC driver should never need to touch
> PHY registers.

Sunplus Ethernet MAC integrates MDIO controller. 
So Ethernet driver has MDIO- and PHY-related code. 
To work-around a circuitry bug, we need to enable 
bit 10 of register 4 of PHY.
Where should we place the code?


> > +++ b/drivers/net/ethernet/sunplus/l2sw_mdio.c
> > @@ -0,0 +1,118 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/* Copyright Sunplus Technology Co., Ltd.
> > + *       All rights reserved.
> > + */
> > +
> > +#include "l2sw_mdio.h"
> > +
> > +static int mii_read(struct mii_bus *bus, int phy_id, int regnum) {
> > +	return mdio_read(phy_id, regnum);
> > +}
> > +
> > +static int mii_write(struct mii_bus *bus, int phy_id, int regnum, u16
> > +val) {
> > +	return mdio_write(phy_id, regnum, val); }
> > +
> > +u32 mdio_init(struct platform_device *pdev, struct net_device
> > +*net_dev) {
> > +	struct l2sw_mac *mac = netdev_priv(net_dev);
> > +	struct mii_bus *mii_bus;
> > +	struct device_node *mdio_node;
> > +	u32 ret;
> > +
> > +	mii_bus = mdiobus_alloc();
> > +	if (!mii_bus) {
> > +		pr_err(" Failed to allocate mdio_bus memory!\n");
> > +		return -ENOMEM;
> > +	}
> > +
> > +	mii_bus->name = "sunplus_mii_bus";
> > +	mii_bus->parent = &pdev->dev;
> > +	mii_bus->priv = mac;
> > +	mii_bus->read = mii_read;
> > +	mii_bus->write = mii_write;
> > +	snprintf(mii_bus->id, MII_BUS_ID_SIZE, "%s-mii",
> > +dev_name(&pdev->dev));
> > +
> > +	mdio_node = of_get_parent(mac->comm->phy1_node);
> > +	ret = of_mdiobus_register(mii_bus, mdio_node);
> > +	if (ret) {
> > +		pr_err(" Failed to register mii bus (ret = %d)!\n", ret);
> > +		mdiobus_free(mii_bus);
> > +		return ret;
> > +	}
> > +
> > +	mac->comm->mii_bus = mii_bus;
> > +	return ret;
> > +}
> > +
> > +void mdio_remove(struct net_device *net_dev) {
> > +	struct l2sw_mac *mac = netdev_priv(net_dev);
> > +
> > +	if (mac->comm->mii_bus) {
> > +		mdiobus_unregister(mac->comm->mii_bus);
> > +		mdiobus_free(mac->comm->mii_bus);
> > +		mac->comm->mii_bus = NULL;
> > +	}
> > +}
> 
> You MDIO code is pretty scattered around. Please bring it all together in one
> file.

Ok, I'll move mdio_read() and mdio_write() to 'l2sw_mdio.c',
but keep mdio_access() in 'l2sw_hal.c' because it is a function 
which accesses hardware registers actually.


> > +static void mii_linkchange(struct net_device *netdev) { }
> 
> Nothing to do? Seems very odd. Don't you need to tell the MAC it should do
> 10Mbps or 100Mbps? What about pause?

No, hardware does it automatically.
Sunplus MAC integrates MDIO controller.
It reads PHY status and set MAC automatically.


> > +
> > +int mac_phy_probe(struct net_device *netdev) {
> > +	struct l2sw_mac *mac = netdev_priv(netdev);
> > +	struct phy_device *phydev;
> > +	int i;
> > +
> > +	phydev = of_phy_connect(mac->net_dev, mac->comm->phy1_node,
> mii_linkchange,
> > +				0, PHY_INTERFACE_MODE_RGMII_ID);
> 
> You should not hard code PHY_INTERFACE_MODE_RGMII_ID. Use the DT
> property "phy-mode"

Ok, I'll do it in next patch.
I create a new file 'l2sw_phy.c' and move phy-related functions
from 'l2sw_hal.c' to it.


> > +	if (!phydev) {
> > +		pr_err(" \"%s\" has no phy found\n", netdev->name);
> > +		return -1;
> 
> -ENODEV;
> 
> Never use -1, pick an error code.

Ok, I'll modify it in next patch.

> 
> > +	}
> > +
> > +	if (mac->comm->phy2_node) {
> > +		of_phy_connect(mac->net_dev, mac->comm->phy2_node,
> mii_linkchange,
> > +			       0, PHY_INTERFACE_MODE_RGMII_ID);
> > +	}
> > +
> > +	linkmode_clear_bit(ETHTOOL_LINK_MODE_Pause_BIT,
> phydev->supported);
> > +	linkmode_clear_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT,
> > +phydev->supported);
> 
> So the MAC does not support pause? I'm then confused about phy_cfg().

Yes, MAC supports pause. MAC (hardware) takes care of pause 
automatically.

Should I remove the two lines?


> 
>    Andrew

Powered by blists - more mailing lists