RE: [PATCH v2 2/2] net: ethernet: Add driver for Sunplus SP7021

["Wells Lu 呂芳騰" <wells.lu@sunplus.com>]

Hi,

> On 11/11/21 1:04 AM, Wells Lu wrote:
> > Add driver for Sunplus SP7021.
> >
> > Signed-off-by: Wells Lu <wells.lu@sunplus.com>
> > ---
> 
> [snip]
> 
> > +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.


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


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


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


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