Message-ID: <CACWXhKkX-syR01opOky=t-b8C3nhV5f_WNfCQ-kOE+4o0xh4tA@mail.gmail.com>
Date: Fri, 14 Jul 2023 10:15:27 +0800
From: Feiyang Chen <chris.chenfeiyang@...il.com>
To: Andrew Lunn <andrew@...n.ch>
Cc: Feiyang Chen <chenfeiyang@...ngson.cn>, hkallweit1@...il.com, peppe.cavallaro@...com, 
	alexandre.torgue@...s.st.com, joabreu@...opsys.com, chenhuacai@...ngson.cn, 
	linux@...linux.org.uk, dongbiao@...ngson.cn, 
	loongson-kernel@...ts.loongnix.cn, netdev@...r.kernel.org, 
	loongarch@...ts.linux.dev
Subject: Re: [RFC PATCH 01/10] net: phy: Add driver for Loongson PHY

On Thu, Jul 13, 2023 at 12:00 PM Andrew Lunn <andrew@...n.ch> wrote:
>
> On Thu, Jul 13, 2023 at 10:46:53AM +0800, Feiyang Chen wrote:
> > Add support for the Loongson PHY.
> >
> > Signed-off-by: Feiyang Chen <chenfeiyang@...ngson.cn>
> > ---
> >  drivers/net/phy/Kconfig        |  5 +++
> >  drivers/net/phy/Makefile       |  1 +
> >  drivers/net/phy/loongson-phy.c | 69 ++++++++++++++++++++++++++++++++++
>
> Please drop -phy from the filename. No other phy driver does this.
>

Hi, Andrew,

OK.

> >  drivers/net/phy/phy_device.c   | 16 ++++++++
> >  include/linux/phy.h            |  2 +
> >  5 files changed, 93 insertions(+)
> >  create mode 100644 drivers/net/phy/loongson-phy.c
> >
> > diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
> > index 93b8efc79227..4f8ea32cbc68 100644
> > --- a/drivers/net/phy/Kconfig
> > +++ b/drivers/net/phy/Kconfig
> > @@ -202,6 +202,11 @@ config INTEL_XWAY_PHY
> >         PEF 7061, PEF 7071 and PEF 7072 or integrated into the Intel
> >         SoCs xRX200, xRX300, xRX330, xRX350 and xRX550.
> >
> > +config LOONGSON_PHY
> > +     tristate "Loongson PHY driver"
> > +     help
> > +       Supports the Loongson PHY.
> > +
> >  config LSI_ET1011C_PHY
> >       tristate "LSI ET1011C PHY"
> >       help
> > diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
> > index f289ab16a1da..f775373e12b7 100644
> > --- a/drivers/net/phy/Makefile
> > +++ b/drivers/net/phy/Makefile
> > @@ -62,6 +62,7 @@ obj-$(CONFIG_DP83TD510_PHY) += dp83td510.o
> >  obj-$(CONFIG_FIXED_PHY)              += fixed_phy.o
> >  obj-$(CONFIG_ICPLUS_PHY)     += icplus.o
> >  obj-$(CONFIG_INTEL_XWAY_PHY) += intel-xway.o
> > +obj-$(CONFIG_LOONGSON_PHY)   += loongson-phy.o
> >  obj-$(CONFIG_LSI_ET1011C_PHY)        += et1011c.o
> >  obj-$(CONFIG_LXT_PHY)                += lxt.o
> >  obj-$(CONFIG_MARVELL_10G_PHY)        += marvell10g.o
> > diff --git a/drivers/net/phy/loongson-phy.c b/drivers/net/phy/loongson-phy.c
> > new file mode 100644
> > index 000000000000..d4aefa2110f8
> > --- /dev/null
> > +++ b/drivers/net/phy/loongson-phy.c
> > @@ -0,0 +1,69 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * LS7A PHY driver
> > + *
> > + * Copyright (C) 2020-2023 Loongson Technology Corporation Limited
> > + *
> > + * Author: Zhang Baoqi <zhangbaoqi@...ngson.cn>
> > + */
> > +
> > +#include <linux/mii.h>
> > +#include <linux/module.h>
> > +#include <linux/netdevice.h>
> > +#include <linux/pci.h>
> > +#include <linux/phy.h>
> > +
> > +#define PHY_ID_LS7A2000              0x00061ce0
>
> What is Loongson OUI?
>

Currently, we do not have an OUI for Loongson, but we are in the
process of applying for one.

> > +#define GNET_REV_LS7A2000    0x00
> > +
> > +static int ls7a2000_config_aneg(struct phy_device *phydev)
> > +{
> > +     if (phydev->speed == SPEED_1000)
> > +             phydev->autoneg = AUTONEG_ENABLE;
>
> Please explain.
>

The PHY itself supports half-duplex, but there are issues with the
controller used in the 7A2000 chip. Moreover, the controller only
supports auto-negotiation for gigabit speeds.

> > +
> > +     if (linkmode_test_bit(ETHTOOL_LINK_MODE_10baseT_Full_BIT,
> > +         phydev->advertising) ||
> > +         linkmode_test_bit(ETHTOOL_LINK_MODE_100baseT_Full_BIT,
> > +         phydev->advertising) ||
> > +         linkmode_test_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT,
> > +         phydev->advertising))
> > +         return genphy_config_aneg(phydev);
> > +
> > +     netdev_info(phydev->attached_dev, "Parameter Setting Error\n");
>
> This also needs explaining. How can it be asked to do something it
> does not support?
>

Perhaps I missed something, but I think that if someone uses ethtool,
they can request it to perform actions or configurations that the
tool does not support.

> > +     return -1;
>
> Always use error codes. In this case EINVAL.
>

OK.

> > +}
> > +
> > +int ls7a2000_match_phy_device(struct phy_device *phydev)
> > +{
> > +     struct net_device *ndev;
> > +     struct pci_dev *pdev;
> > +
> > +     if ((phydev->phy_id & 0xfffffff0) != PHY_ID_LS7A2000)
> > +             return 0;
> > +
> > +     ndev = phydev->mdio.bus->priv;
> > +     pdev = to_pci_dev(ndev->dev.parent);
> > +
> > +     return pdev->revision == GNET_REV_LS7A2000;
>
> That is very unusual. Why is the PHY ID not sufficient?
>

To work around the controller's issues, we enable the usage of this
driver specifically for a certain version of the 7A2000 chip.

> > +}
> > +
> > +static struct phy_driver loongson_phy_driver[] = {
> > +     {
> > +             PHY_ID_MATCH_MODEL(PHY_ID_LS7A2000),
> > +             .name                   = "LS7A2000 PHY",
> > +             .features               = PHY_LOONGSON_FEATURES,
>
> So what are the capabilities of this PHY? You seem to have some very
> odd hacks here, and no explanation of why they are needed. If you do
> something which no other device does, you need to explain it.
>
> Does the PHY itself only support full duplex? No half duplex? Does the
> PHY support autoneg? Does it support fixed settings? What does
> genphy_read_abilities() return for this PHY?
>

As mentioned earlier, this driver is specifically designed for the PHY
on the problematic 7A2000 chip. Therefore, we assume that this PHY only
supports full- duplex mode and performs auto-negotiation exclusively for
gigabit speeds.

Thanks,
Feiyang

>         Andrew

Powered by blists - more mailing lists