[RFC,01/10] net: phy: Add driver for Loongson PHY

Commit Message

Feiyang Chen July 13, 2023, 2:46 a.m. UTC

Add support for the Loongson PHY.

Signed-off-by: Feiyang Chen <[e-mail]>
---
 drivers/net/phy/Kconfig        |  5 +++
 drivers/net/phy/Makefile       |  1 +
 drivers/net/phy/loongson-phy.c | 69 ++++++++++++++++++++++++++++++++++
 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

Comments

Andrew Lunn July 13, 2023, 4 a.m. UTC | #1

On Thu, Jul 13, 2023 at 10:46:53AM +0800, Feiyang Chen wrote:
> Add support for the Loongson PHY.
> 
> Signed-off-by: Feiyang Chen <[e-mail]>
> ---
>  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.

>  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 <[e-mail]>
> + */
> +
> +#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?

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

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

> +	return -1;

Always use error codes. In this case EINVAL.

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

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

	Andrew

Russell King (Oracle) July 13, 2023, 7:53 a.m. UTC | #2

On Thu, Jul 13, 2023 at 10:46:53AM +0800, Feiyang Chen wrote:
> +#define PHY_ID_LS7A2000		0x00061ce0
> +#define GNET_REV_LS7A2000	0x00
> +
> +static int ls7a2000_config_aneg(struct phy_device *phydev)
> +{
> +	if (phydev->speed == SPEED_1000)
> +		phydev->autoneg = AUTONEG_ENABLE;
> +
> +	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);

While it's fine to use four spaces within the if () expression, this
"return" should be indented with a tab.

> +
> +	netdev_info(phydev->attached_dev, "Parameter Setting Error\n");

Does this give the opportunity for userspace to spam the kernel log?
E.g. by a daemon repeatedly trying to set link parameters? Should it
be rate limited?

> +	return -1;

Sigh, not this laziness disease yet again. -1 is -EPERM. Return a
real errno code.

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

	if (!phy_id_compare(phydev->phy_id, PHY_ID_LS7A2000, 0xfffffff0))
		return 0;

> +
> +	ndev = phydev->mdio.bus->priv;

This doesn't look safe to me - you're assuming that if the PHY ID
above matches, that the MDIO bus' private data is something you know
which is far from guaranteed.

The mdio bus has a parent device - that would be a safer route to
check what the parent device is, provided the mdio bus is created so
that it's a child of the parent PCI device.

Feiyang Chen July 14, 2023, 2:15 a.m. UTC | #3

On Thu, Jul 13, 2023 at 12:00 PM Andrew Lunn <[e-mail]> 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 <[e-mail]>
> > ---
> >  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 <[e-mail]>
> > + */
> > +
> > +#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

Feiyang Chen July 14, 2023, 2:41 a.m. UTC | #4

On Thu, Jul 13, 2023 at 3:53 PM Russell King (Oracle)
<[e-mail]> wrote:
>
> On Thu, Jul 13, 2023 at 10:46:53AM +0800, Feiyang Chen wrote:
> > +#define PHY_ID_LS7A2000              0x00061ce0
> > +#define GNET_REV_LS7A2000    0x00
> > +
> > +static int ls7a2000_config_aneg(struct phy_device *phydev)
> > +{
> > +     if (phydev->speed == SPEED_1000)
> > +             phydev->autoneg = AUTONEG_ENABLE;
> > +
> > +     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);
>
> While it's fine to use four spaces within the if () expression, this
> "return" should be indented with a tab.
>

Hi, Russell,

Sorry for the typo. I will correct it in the next version.

> > +
> > +     netdev_info(phydev->attached_dev, "Parameter Setting Error\n");
>
> Does this give the opportunity for userspace to spam the kernel log?
> E.g. by a daemon repeatedly trying to set link parameters? Should it
> be rate limited?
>
> > +     return -1;
>
> Sigh, not this laziness disease yet again. -1 is -EPERM. Return a
> real errno code.
>

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;
>
>         if (!phy_id_compare(phydev->phy_id, PHY_ID_LS7A2000, 0xfffffff0))
>                 return 0;
>

OK.

> > +
> > +     ndev = phydev->mdio.bus->priv;
>
> This doesn't look safe to me - you're assuming that if the PHY ID
> above matches, that the MDIO bus' private data is something you know
> which is far from guaranteed.
>
> The mdio bus has a parent device - that would be a safer route to
> check what the parent device is, provided the mdio bus is created so
> that it's a child of the parent PCI device.
>

OK. I’ll get right on that.

Thanks,
Feiyang



> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Andrew Lunn July 14, 2023, 4:19 a.m. UTC | #5

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

Is the value 0x00061ce0 baked into the silicon? Or can it be changed
once you have an OUI?

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

So you can force 10/100/1000, but auto neg only succeeds for 1G?

Are the LP autoneg values available for genphy_read_lpa() to read? If
the LP values are available, maybe the PHY driver can resolve the
autoneg for 10 an 100.

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

No. The PHY should indicate what it can do, via the .get_abilities()
etc. The MAC can also remove some of those link modes if it is more
limited than the PHY. phylib will then not allow ksetting_set() to
enable things which are not supported. So this should not happen.  It
would actually be a bad design, since it would force every driver to
do such checks, rather than implement it once in the core.

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

So what does genphy_read_abilities() return?

Nobody else going to use PHY_LOONGSON_FEATURES, so i would prefer not
to add it to the core. If your PHY is designed correctly,
genphy_read_abilities() should determine what the PHY can do from its
registers. If the registers are wrong, it is better to add a
.get_features function.

	Andrew

Feiyang Chen July 17, 2023, 2:43 a.m. UTC | #6

On Fri, Jul 14, 2023 at 12:20 PM Andrew Lunn <[e-mail]> wrote:
>
> > > > +#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.
>
> Is the value 0x00061ce0 baked into the silicon? Or can it be changed
> once you have an OUI?
>

Hi, Andrew,

The value is baked into the silicon.

> > > > +#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.
>
> So you can force 10/100/1000, but auto neg only succeeds for 1G?
>
> Are the LP autoneg values available for genphy_read_lpa() to read? If
> the LP values are available, maybe the PHY driver can resolve the
> autoneg for 10 an 100.
>

I apologize for the confusion caused by my previous description. To
clarify, the PHY supports auto-negotiation and non-auto-negotiation
for 10/100, but non-auto-negotiation for 1000 does not work correctly.

> > > > +     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.
>
> No. The PHY should indicate what it can do, via the .get_abilities()
> etc. The MAC can also remove some of those link modes if it is more
> limited than the PHY. phylib will then not allow ksetting_set() to
> enable things which are not supported. So this should not happen.  It
> would actually be a bad design, since it would force every driver to
> do such checks, rather than implement it once in the core.
>

I see.

> > > > +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.
>
> So what does genphy_read_abilities() return?
>
> Nobody else going to use PHY_LOONGSON_FEATURES, so i would prefer not
> to add it to the core. If your PHY is designed correctly,
> genphy_read_abilities() should determine what the PHY can do from its
> registers. If the registers are wrong, it is better to add a
> .get_features function.
>

genphy_read_abilities() returns 0, and it seems to be working fine.
The registers are incorrect, so I will use the .get_features function
to clear some of the half-duplex bits.

Thanks,
Feiyang

>         Andrew

Andrew Lunn July 17, 2023, 12:22 p.m. UTC | #7

> > > > > +#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.
> >
> > Is the value 0x00061ce0 baked into the silicon? Or can it be changed
> > once you have an OUI?
> >
> 
> Hi, Andrew,
> 
> The value is baked into the silicon.

O.K. Thanks. Do you have an idea how long it will take to get an OUI?

Does the PCI ID uniquely identify the MAC+PHY combination? 

> > > 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.
> >
> > So you can force 10/100/1000, but auto neg only succeeds for 1G?
> >
> > Are the LP autoneg values available for genphy_read_lpa() to read? If
> > the LP values are available, maybe the PHY driver can resolve the
> > autoneg for 10 an 100.
> >
> 
> I apologize for the confusion caused by my previous description. To
> clarify, the PHY supports auto-negotiation and non-auto-negotiation
> for 10/100, but non-auto-negotiation for 1000 does not work correctly.

So you can force 10/100, but auto-neg 10/100/1000.

So i would suggest .get_features() indicates normal 10/100/1000
operation. Have your .config_aneg function which is used for both
auto-neg and forced configuration check for phydev->autoneg ==
AUTONEG_DISABLE and phydev->speed == SPEED_1000 and return
-EOPNOTSUPP. Otherwise call genphy_config_aneg().

> > So what does genphy_read_abilities() return?
> >
> > Nobody else going to use PHY_LOONGSON_FEATURES, so i would prefer not
> > to add it to the core. If your PHY is designed correctly,
> > genphy_read_abilities() should determine what the PHY can do from its
> > registers. If the registers are wrong, it is better to add a
> > .get_features function.
> >
> 
> genphy_read_abilities() returns 0, and it seems to be working fine.
> The registers are incorrect, so I will use the .get_features function
> to clear some of the half-duplex bits.

You said above that the PHY supports half duplex, the MAC has problems
with half duplex. So it should be the MAC which indicates it does not
unsupported half duplex link modes.

So you need to modify phylink_config.mac_capabilities before
phylink_create() is called. There is already

        /* Half-Duplex can only work with single queue */
	if (priv->plat->tx_queues_to_use > 1)
                priv->phylink_config.mac_capabilities &=
                        ~(MAC_10HD | MAC_100HD | MAC_1000HD);

so you just need to add a quirk for your broken hardware.

	Andrew

Feiyang Chen July 21, 2023, 3:31 a.m. UTC | #8

On Mon, Jul 17, 2023 at 8:22 PM Andrew Lunn <[e-mail]> wrote:
>
> > > > > > +#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.
> > >
> > > Is the value 0x00061ce0 baked into the silicon? Or can it be changed
> > > once you have an OUI?
> > >
> >
> > Hi, Andrew,
> >
> > The value is baked into the silicon.
>
> O.K. Thanks. Do you have an idea how long it will take to get an OUI?
>
> Does the PCI ID uniquely identify the MAC+PHY combination?
>

Hi, Andrew,

Sorry, I currently don't have an exact timeline for when the OUI will
be available. The next hardware version will address these bugs, so
we won't be going with this driver.

> > > > 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.
> > >
> > > So you can force 10/100/1000, but auto neg only succeeds for 1G?
> > >
> > > Are the LP autoneg values available for genphy_read_lpa() to read? If
> > > the LP values are available, maybe the PHY driver can resolve the
> > > autoneg for 10 an 100.
> > >
> >
> > I apologize for the confusion caused by my previous description. To
> > clarify, the PHY supports auto-negotiation and non-auto-negotiation
> > for 10/100, but non-auto-negotiation for 1000 does not work correctly.
>
> So you can force 10/100, but auto-neg 10/100/1000.
>
> So i would suggest .get_features() indicates normal 10/100/1000
> operation. Have your .config_aneg function which is used for both
> auto-neg and forced configuration check for phydev->autoneg ==
> AUTONEG_DISABLE and phydev->speed == SPEED_1000 and return
> -EOPNOTSUPP. Otherwise call genphy_config_aneg().
>

Well, can I return -EINVAL in the .set_link_ksettings callback?
Considering that our next hardware version will have the OUI
allocated and these bugs fixed, I won't submit this driver in
the next patch version. I believe we can just use the generic
PHY for now.

> > > So what does genphy_read_abilities() return?
> > >
> > > Nobody else going to use PHY_LOONGSON_FEATURES, so i would prefer not
> > > to add it to the core. If your PHY is designed correctly,
> > > genphy_read_abilities() should determine what the PHY can do from its
> > > registers. If the registers are wrong, it is better to add a
> > > .get_features function.
> > >
> >
> > genphy_read_abilities() returns 0, and it seems to be working fine.
> > The registers are incorrect, so I will use the .get_features function
> > to clear some of the half-duplex bits.
>
> You said above that the PHY supports half duplex, the MAC has problems
> with half duplex. So it should be the MAC which indicates it does not
> unsupported half duplex link modes.
>
> So you need to modify phylink_config.mac_capabilities before
> phylink_create() is called. There is already
>
>         /* Half-Duplex can only work with single queue */
>         if (priv->plat->tx_queues_to_use > 1)
>                 priv->phylink_config.mac_capabilities &=
>                         ~(MAC_10HD | MAC_100HD | MAC_1000HD);
>
> so you just need to add a quirk for your broken hardware.
>

OK.

Thanks,
Feiyang

>         Andrew
>

Andrew Lunn July 21, 2023, 9:04 a.m. UTC | #9

> Hi, Andrew,
> 
> Sorry, I currently don't have an exact timeline for when the OUI will
> be available. The next hardware version will address these bugs, so
> we won't be going with this driver.

Not having an OUI breaks the standard. So i was actually thinking you
should trap the reads to the ID registers in the MDIO bus driver and
return valid values. Some Marvell Ethernet switch integrated PHYs have
a valid OUI, and no device part. We trap those and insert valid
values. So there is some president for doing this. Doing this would
also allow you to avoid the PHY driver poking around the MAC drivers
PCI bus.

> > So i would suggest .get_features() indicates normal 10/100/1000
> > operation. Have your .config_aneg function which is used for both
> > auto-neg and forced configuration check for phydev->autoneg ==
> > AUTONEG_DISABLE and phydev->speed == SPEED_1000 and return
> > -EOPNOTSUPP. Otherwise call genphy_config_aneg().
> >
> 
> Well, can I return -EINVAL in the .set_link_ksettings callback?

If the PHY is broken, the PHY should do it. If the MAC is broken, the
MAC should do it. We have clean separation here, even when the
hardware is integrated.

> Considering that our next hardware version will have the OUI
> allocated and these bugs fixed, I won't submit this driver in
> the next patch version. I believe we can just use the generic
> PHY for now.

The problem with the generic driver is you cannot have workarounds in
it. You don't want to put PHY workarounds in the MAC driver.

    Andrew

Feiyang Chen July 25, 2023, 2:02 a.m. UTC | #10

On Fri, Jul 21, 2023 at 5:04 PM Andrew Lunn <[e-mail]> wrote:
>
> > Hi, Andrew,
> >
> > Sorry, I currently don't have an exact timeline for when the OUI will
> > be available. The next hardware version will address these bugs, so
> > we won't be going with this driver.
>
> Not having an OUI breaks the standard. So i was actually thinking you
> should trap the reads to the ID registers in the MDIO bus driver and
> return valid values. Some Marvell Ethernet switch integrated PHYs have
> a valid OUI, and no device part. We trap those and insert valid
> values. So there is some president for doing this. Doing this would
> also allow you to avoid the PHY driver poking around the MAC drivers
> PCI bus.
>
> > > So i would suggest .get_features() indicates normal 10/100/1000
> > > operation. Have your .config_aneg function which is used for both
> > > auto-neg and forced configuration check for phydev->autoneg ==
> > > AUTONEG_DISABLE and phydev->speed == SPEED_1000 and return
> > > -EOPNOTSUPP. Otherwise call genphy_config_aneg().
> > >
> >
> > Well, can I return -EINVAL in the .set_link_ksettings callback?
>
> If the PHY is broken, the PHY should do it. If the MAC is broken, the
> MAC should do it. We have clean separation here, even when the
> hardware is integrated.
>

Hi, Andrew,

I get it. To be honest, I'm not familiar with the hardware, so I asked
the colleague who designed this chip. The real problem is the same as I
described in Patch 10 - the Ethernet controller doesn't work well with
the PHY. If the controller works properly, we can force 1000. Are there
any methods to work around this problem in the MAC driver?

> > Considering that our next hardware version will have the OUI
> > allocated and these bugs fixed, I won't submit this driver in
> > the next patch version. I believe we can just use the generic
> > PHY for now.
>
> The problem with the generic driver is you cannot have workarounds in
> it. You don't want to put PHY workarounds in the MAC driver.
>

Sure, we should not put PHY workarounds in the MAC driver and vice
versa.

This PHY driver was designed to solve two problems at first:
1. We cannot force 1000.
2. We cannot use half duplex.

Now, we are sure that the MAC is broken, not the PHY, so I think we
can solve these problems in the MAC driver.

Thanks,
Feiyang

>     Andrew
>

Andrew Lunn July 25, 2023, 5:20 p.m. UTC | #11

> This PHY driver was designed to solve two problems at first:
> 1. We cannot force 1000.
> 2. We cannot use half duplex.
> 
> Now, we are sure that the MAC is broken, not the PHY, so I think we
> can solve these problems in the MAC driver.

You can solve both of these in the MAC. You can remove the half duplex
link modes after the PHY is connected to the MAC. And you can add a
test in ksetting_set to check for 1000 Force and return EOPNOTSUPP.

     Andrew

Patch

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 <[e-mail]>
+ */
+
+#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
+#define GNET_REV_LS7A2000	0x00
+
+static int ls7a2000_config_aneg(struct phy_device *phydev)
+{
+	if (phydev->speed == SPEED_1000)
+		phydev->autoneg = AUTONEG_ENABLE;
+
+	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");
+	return -1;
+}
+
+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;
+}
+
+static struct phy_driver loongson_phy_driver[] = {
+	{
+		PHY_ID_MATCH_MODEL(PHY_ID_LS7A2000),
+		.name			= "LS7A2000 PHY",
+		.features		= PHY_LOONGSON_FEATURES,
+		.config_aneg		= ls7a2000_config_aneg,
+		.match_phy_device	= ls7a2000_match_phy_device,
+	},
+};
+module_phy_driver(loongson_phy_driver);
+
+static struct mdio_device_id __maybe_unused loongson_tbl[] = {
+	{ PHY_ID_MATCH_MODEL(PHY_ID_LS7A2000) },
+	{ },
+};
+MODULE_DEVICE_TABLE(mdio, loongson_tbl);
+
+MODULE_DESCRIPTION("Loongson PHY driver");
+MODULE_AUTHOR("Zhang Baoqi <[e-mail]>");
+MODULE_LICENSE("GPL");
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 53598210be6c..00568608118a 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -146,6 +146,15 @@  static const int phy_eee_cap1_features_array[] = {
 __ETHTOOL_DECLARE_LINK_MODE_MASK(phy_eee_cap1_features) __ro_after_init;
 EXPORT_SYMBOL_GPL(phy_eee_cap1_features);
 
+static const int phy_10_100_1000_full_features_array[] = {
+	ETHTOOL_LINK_MODE_10baseT_Full_BIT,
+	ETHTOOL_LINK_MODE_100baseT_Full_BIT,
+	ETHTOOL_LINK_MODE_1000baseT_Full_BIT,
+};
+
+__ETHTOOL_DECLARE_LINK_MODE_MASK(phy_loongson_features) __ro_after_init;
+EXPORT_SYMBOL_GPL(phy_loongson_features);
+
 static void features_init(void)
 {
 	/* 10/100 half/full*/
@@ -230,6 +239,13 @@  static void features_init(void)
 	linkmode_set_bit_array(phy_eee_cap1_features_array,
 			       ARRAY_SIZE(phy_eee_cap1_features_array),
 			       phy_eee_cap1_features);
+	/* 10/100/1000 full */
+	linkmode_set_bit_array(phy_basic_ports_array,
+			       ARRAY_SIZE(phy_basic_ports_array),
+			       phy_loongson_features);
+	linkmode_set_bit_array(phy_10_100_1000_full_features_array,
+			       ARRAY_SIZE(phy_10_100_1000_full_features_array),
+			       phy_loongson_features);
 
 }
 
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 6478838405a0..6bdb2a479755 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -54,6 +54,7 @@  extern __ETHTOOL_DECLARE_LINK_MODE_MASK(phy_10gbit_features) __ro_after_init;
 extern __ETHTOOL_DECLARE_LINK_MODE_MASK(phy_10gbit_fec_features) __ro_after_init;
 extern __ETHTOOL_DECLARE_LINK_MODE_MASK(phy_10gbit_full_features) __ro_after_init;
 extern __ETHTOOL_DECLARE_LINK_MODE_MASK(phy_eee_cap1_features) __ro_after_init;
+extern __ETHTOOL_DECLARE_LINK_MODE_MASK(phy_loongson_features) __ro_after_init;
 
 #define PHY_BASIC_FEATURES ((unsigned long *)&phy_basic_features)
 #define PHY_BASIC_T1_FEATURES ((unsigned long *)&phy_basic_t1_features)
@@ -65,6 +66,7 @@  extern __ETHTOOL_DECLARE_LINK_MODE_MASK(phy_eee_cap1_features) __ro_after_init;
 #define PHY_10GBIT_FEC_FEATURES ((unsigned long *)&phy_10gbit_fec_features)
 #define PHY_10GBIT_FULL_FEATURES ((unsigned long *)&phy_10gbit_full_features)
 #define PHY_EEE_CAP1_FEATURES ((unsigned long *)&phy_eee_cap1_features)
+#define PHY_LOONGSON_FEATURES ((unsigned long *)&phy_loongson_features)
 
 extern const int phy_basic_ports_array[3];
 extern const int phy_fibre_port_array[1];