Message-ID: <CACWXhKk23muXROj6OrmeFna88ViJHA_7QpQZoWiFgzEPb4pLWQ@mail.gmail.com>
Date: Mon, 17 Jul 2023 10:43:14 +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 Fri, Jul 14, 2023 at 12:20 PM Andrew Lunn <andrew@...n.ch> 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

Powered by blists - more mailing lists