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

[Florian Fainelli]

On 11/11/21 1:04 AM, Wells Lu wrote:
> Add driver for Sunplus SP7021.
> 
> Signed-off-by: Wells Lu <wells.lu@xxxxxxxxxxx>
> ---

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

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

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

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

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

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