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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <67e29bce.050a0220.15db86.84a4@mx.google.com>
Date: Tue, 25 Mar 2025 13:04:30 +0100
From: Christian Marangi <ansuelsmth@...il.com>
To: Andrew Lunn <andrew@...n.ch>
Cc: Andrew Lunn <andrew+netdev@...n.ch>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Heiner Kallweit <hkallweit1@...il.com>,
	Russell King <linux@...linux.org.uk>, netdev@...r.kernel.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [net-next PATCH 1/2] net: phy: Add support for new Aeonsemi PHYs

On Mon, Mar 24, 2025 at 04:16:09PM +0100, Andrew Lunn wrote:
> 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.
>

Ok update on this... The PHY report 7500 7500 but on enabling PTP clock,
a more specific ""family"" ID is filled in MMD that is 0x7500 0x9410.

They all use the same firmware so matching for the family ID might not
be a bad idea... The alternative is either load the firmware in
match_phy_device or introduce some additional OPs to handle this
correctly...

Considering how the thing are evolving with PHY I really feel it's time
we start introducing specific OP for firmware loading and we might call
this OP before PHY ID matching is done (or maybe do it again).

Lets see how bad the thing goes API wise tho.

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

Sadly no and I don't even have datasheet, just some contacts to confirm
1-2 thing for the PHY.

-- 
	Ansuel