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
