Hi,
> 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.
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.
