lkml.org 
[lkml]   [2021]   [Nov]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH v2 2/2] net: ethernet: Add driver for Sunplus SP7021
    From
    Date
    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.

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

    \
     
     \ /
      Last update: 2021-11-13 00:16    [W:2.418 / U:0.020 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site