Hi,

On 11/11/21 1:04 AM, Wells Lu wrote:

quoted

Add driver for Sunplus SP7021.

Signed-off-by: Wells Lu <redacted>
---

[snip]

quoted

+u32 mdio_read(struct sp_mac *mac, u32 phy_id, u16 regnum) {
+	int ret;
+
+	ret = hal_mdio_access(mac, MDIO_READ_CMD, phy_id, regnum, 0);
+	if (ret < 0)
+		return -EOPNOTSUPP;
+
+	return ret;
+}
+
+u32 mdio_write(struct sp_mac *mac, u32 phy_id, u32 regnum, u16 val) {
+	int ret;
+
+	ret = hal_mdio_access(mac, MDIO_WRITE_CMD, phy_id, regnum, val);
+	if (ret < 0)
+		return -EOPNOTSUPP;
+
+	return 0;
+}

You should not be exposing these functions, if you do, that means another part of your
code performs MDIO bus read/write operations without using the appropriate layer, so no.

Yes, I'll re-declare the two functions as static functions.

quoted

+
+static int mii_read(struct mii_bus *bus, int phy_id, int regnum) {
+	struct sp_mac *mac = bus->priv;
+
+	return mdio_read(mac, phy_id, regnum); }
+
+static int mii_write(struct mii_bus *bus, int phy_id, int regnum, u16
+val) {
+	struct sp_mac *mac = bus->priv;
+
+	return mdio_write(mac, phy_id, regnum, val); }
+
+u32 mdio_init(struct platform_device *pdev, struct net_device *ndev)

Those function names need to be prefixed with sp_ to denote the driver local scope, this
applies for your entire patch set.

Yes, I'll add vendor-specified prefix to the two functions and all the other functions in
the drivers.

[snip]

quoted

diff --git a/drivers/net/ethernet/sunplus/sp_mdio.h

b/drivers/net/ethernet/sunplus/sp_mdio.h
new file mode 100644
index 0000000..d708624

--- /dev/null
+++ b/drivers/net/ethernet/sunplus/sp_mdio.h

@@ -0,0 +1,20 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Copyright Sunplus Technology Co., Ltd.
+ *       All rights reserved.
+ */
+
+#ifndef __SP_MDIO_H__
+#define __SP_MDIO_H__
+
+#include "sp_define.h"
+#include "sp_hal.h"
+
+#define MDIO_READ_CMD           0x02
+#define MDIO_WRITE_CMD          0x01
+
+u32  mdio_read(struct sp_mac *mac, u32 phy_id, u16 regnum);
+u32  mdio_write(struct sp_mac *mac, u32 phy_id, u32 regnum, u16 val);

Please scope your functions better, and name them sp_mdio_read, etc.
because mdio_read() is way too generic. Also, can you please follow the same prototype
as what include/linux/mdio.h has for the mdiobus->read and ->write calls, that is phy_id
is int, regnum is u32, etc.

Yes, I'll re-declare the two functions, mdio_read() and mdio_write(), as static 
functions and add vendor-specified prefix to them.

The wrong declaration of the two functions will be removed from the header file.

quoted

+u32  mdio_init(struct platform_device *pdev, struct net_device
+*ndev); void mdio_remove(struct net_device *ndev);
+
+#endif

diff --git a/drivers/net/ethernet/sunplus/sp_phy.c

b/drivers/net/ethernet/sunplus/sp_phy.c
new file mode 100644
index 0000000..df6df3a

--- /dev/null
+++ b/drivers/net/ethernet/sunplus/sp_phy.c

@@ -0,0 +1,64 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright Sunplus Technology Co., Ltd.
+ *       All rights reserved.
+ */
+
+#include "sp_phy.h"
+#include "sp_mdio.h"
+
+static void mii_linkchange(struct net_device *netdev) { }

Does your MAC fully auto-configure based on the PHY's link parameters, if so, how does
it do it? You most certainly need to act on duplex changes, or speed changes no?

Yes, it does. SP7021 MAC communicates with PHY automatically.
It reads link status (half- or full-duplex, 10M or 100M) from PHY 
and sets itself automatically.
It also reads port status (link up or down) and generates 
interrupt to driver.

quoted

+
+int sp_phy_probe(struct net_device *ndev) {
+	struct sp_mac *mac = netdev_priv(ndev);
+	struct phy_device *phydev;
+	int i;
+
+	phydev = of_phy_connect(ndev, mac->phy_node, mii_linkchange,
+				0, mac->phy_mode);
+	if (!phydev) {
+		netdev_err(ndev, "\"%s\" failed to connect to phy!\n", ndev->name);
+		return -ENODEV;
+	}
+
+	for (i = 0; i < sizeof(phydev->supported) / sizeof(long); i++)
+		phydev->advertising[i] = phydev->supported[i];
+
+	phydev->irq = PHY_MAC_INTERRUPT;
+	mac->phy_dev = phydev;
+
+	// Bug workaround:
+	// Flow-control of phy should be enabled. MAC flow-control will refer
+	// to the bit to decide to enable or disable flow-control.
+	mdio_write(mac, mac->phy_addr, 4, mdio_read(mac, mac->phy_addr, 4) |
+(1 << 10));

This is a layering violation, and you should not be doing those things here, if you need
to advertise flow control, then please set ADVERTISE_PAUSE_CAP and/or ADVERTISE_PAUSE_ASYM
accordingly, see whether
phy_set_asym_pause() can do what you need it to.

Yes, I'll remove the statement, instead, use phy_set_asym_pause().

quoted

+
+	return 0;
+}
+
+void sp_phy_start(struct net_device *ndev) {
+	struct sp_mac *mac = netdev_priv(ndev);
+
+	if (mac->phy_dev)
+		phy_start(mac->phy_dev);
+}
+
+void sp_phy_stop(struct net_device *ndev) {
+	struct sp_mac *mac = netdev_priv(ndev);
+
+	if (mac->phy_dev)
+		phy_stop(mac->phy_dev);
+}
+
+void sp_phy_remove(struct net_device *ndev) {
+	struct sp_mac *mac = netdev_priv(ndev);
+
+	if (mac->phy_dev) {
+		phy_disconnect(mac->phy_dev);
+		mac->phy_dev = NULL;
+	}

The net_device structure already contains a phy_device pointer, you don't need to have
one in your sp_mac structure, too.

Yes, I'll remove phy_device from struct sp_mac.

--
Florian

Thank you very much for your review.