Re: [net-next PATCH 1/2] net: phy: Add support for new Aeonsemi PHYs

From: Andrew Lunn
Date: Mon Mar 24 2025 - 11:18:24 EST


On Mon, Mar 24, 2025 at 03:16:08PM +0100, Christian Marangi wrote:
> On Mon, Mar 24, 2025 at 03:03:51PM +0100, Andrew Lunn wrote:
> > > Supported PHYs AS21011JB1, AS21011PB1, AS21010JB1, AS21010PB1,
> > > AS21511JB1, AS21511PB1, AS21510JB1, AS21510PB1, AS21210JB1,
> > > AS21210PB1 that all register with the PHY ID 0x7500 0x7500
> > > before the firmware is loaded.
> >
> > Does the value change after the firmware is loaded? Is the same
> > firmware used for all variants?
> >
>
> Yes It does... Can PHY subsystem react on this? Like probe for the
> generic one and then use the OPs for the real PHY ID?

Multiple thoughts here....

If the firmware is in SPI, i assume by the time the MDIO bus is
probed, the PHY has its 'real' IDs. So you need entries for those as
well as the dummy ID.

I think this is the first PHY which changes its IDs at runtime. So we
don't have a generic solution to this. USB and PCI probably have
support for this, so maybe there is something we can copy. It could be
they support hotplug, so the device disappears and returns. That is
not really something we have in MDIO. So i don't know if we can reuse
ideas from there.

When the firmware is running, do the different variants all share the
same ID? Is there a way to tell them apart. We always have
phy_driver->match_phy_device(). It could look for 0x75007500, download
the firmware, and then match on the real IDs.

> > > +++ b/drivers/net/phy/Kconfig
> > > @@ -121,6 +121,18 @@ config AMCC_QT2025_PHY
> > >
> > > source "drivers/net/phy/aquantia/Kconfig"
> > >
> > > +config AS21XXX_PHY
> > > + tristate "Aeonsemi AS21xxx PHYs"
> >
> > The sorting is based on the tristate value, so that when you look at
> > 'make menuconfig' the menu is in alphabetical order. So this goes
> > before aquantia.
> >
>
> Tought it was only alphabetical, will move.

Yes, it is not obvious, it should have a comment added at the top.
But the top is so far away, i guess many developers would miss it
anyway.

> > > +static int as21xxx_get_features(struct phy_device *phydev)
> > > +{
> > > + int ret;
> > > +
> > > + ret = genphy_read_abilities(phydev);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + /* AS21xxx supports 100M/1G/2.5G/5G/10G speed. */
> > > + linkmode_clear_bit(ETHTOOL_LINK_MODE_10baseT_Half_BIT,
> > > + phydev->supported);
> > > + linkmode_clear_bit(ETHTOOL_LINK_MODE_10baseT_Full_BIT,
> > > + phydev->supported);
> > > + linkmode_clear_bit(ETHTOOL_LINK_MODE_100baseT_Half_BIT,
> > > + phydev->supported);
> >
> > Does this mean the registers genphy_read_abilities() reads are broken
> > and report link modes it does not actually support?
> >
> > > + linkmode_set_bit(ETHTOOL_LINK_MODE_100baseT_Full_BIT,
> > > + phydev->supported);
> > > + linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT,
> > > + phydev->supported);
> >
> > and it is also not reporting modes it does actually support? Is
> > genphy_read_abilities() actually doing anything useful? Some more
> > comments would be good here.
> >
> > > + linkmode_set_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT,
> > > + phydev->supported);
> > > + linkmode_set_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT,
> > > + phydev->supported);
> > > + linkmode_set_bit(ETHTOOL_LINK_MODE_10000baseT_Full_BIT,
> > > + phydev->supported);
> >
> > Does this mean genphy_c45_pma_read_abilities() also returns the wrong
> > values?
> >
>
> Honestly I followed what the SDK driver did for the get_feature. I will
> check the register making sure correct bits are reported.
>
> Looking at the get_status It might be conflicting as they map C22 in C45
> vendor regs.

If all the registers used for automatic feature detection are broken,
just comment on it and don't use genphy_read_abilities() etc.

> > > +static int as21xxx_read_link(struct phy_device *phydev, int *bmcr)
> > > +{
> > > + int status;
> > > +
> > > + *bmcr = phy_read_mmd(phydev, MDIO_MMD_AN,
> > > + MDIO_AN_C22 + MII_BMCR);
> > > + if (*bmcr < 0)
> > > + return *bmcr;
> > > +
> > > + /* Autoneg is being started, therefore disregard current
> > > + * link status and report link as down.
> > > + */
> > > + if (*bmcr & BMCR_ANRESTART) {
> > > + phydev->link = 0;
> > > + return 0;
> > > + }
> > > +
> > > + status = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_STAT1);
> >
> > No MDIO_AN_C22 + here? Maybe add a comment about which C22 registers
> > are mapped into C45 space.
> >
>
> Nope, not a typo... They took the vendor route for some register but for
> BMSR they used the expected one.
>
> Just to make it clear, using the AN_C22 wasn't a choice... the C45 BMCR
> reports inconsistent data compared to the vendor C22 one.

Comments would be good.

Is there an open datasheet for this device?

Andrew