Re: [net-next RFC PATCH v2 2/3] net: phy: Add support for Aeonsemi AS21xxx PHYs

[Christian Marangi <ansuelsmth@gmail.com>]

On Wed, Mar 26, 2025 at 03:56:15PM +0100, Andrew Lunn wrote:
> > +#define PHY_ID_AS21XXX			0x75009410
> > +/* AS21xxx ID Legend
> > + * AS21x1xxB1
> > + *     ^ ^^
> > + *     | |J: Supports SyncE/PTP
> > + *     | |P: No SyncE/PTP support
> > + *     | 1: Supports 2nd Serdes
> > + *     | 2: Not 2nd Serdes support
> > + *     0: 10G, 5G, 2.5G
> > + *     5: 5G, 2.5G
> > + *     2: 2.5G
> > + */
> > +#define PHY_ID_AS21011JB1		0x75009402
> > +#define PHY_ID_AS21011PB1		0x75009412
> > +#define PHY_ID_AS21010JB1		0x75009422
> > +#define PHY_ID_AS21010PB1		0x75009432
> > +#define PHY_ID_AS21511JB1		0x75009442
> > +#define PHY_ID_AS21511PB1		0x75009452
> > +#define PHY_ID_AS21510JB1		0x75009462
> > +#define PHY_ID_AS21510PB1		0x75009472
> > +#define PHY_ID_AS21210JB1		0x75009482
> > +#define PHY_ID_AS21210PB1		0x75009492
> > +#define PHY_VENDOR_AEONSEMI		0x75009400
> 
> O.K. This helps.
> 
> > +static struct phy_driver as21xxx_drivers[] = {
> > +	{
> > +		/* PHY expose in C45 as 0x7500 0x9410
> > +		 * before firmware is loaded.
> > +		 */
> > +		PHY_ID_MATCH_EXACT(PHY_ID_AS21XXX),
> > +		.name		= "Aeonsemi AS21xxx",
> > +		.probe		= as21xxx_probe,
> > +	},
> > +	{
> > +		PHY_ID_MATCH_EXACT(PHY_ID_AS21011JB1),
> > +		.name		= "Aeonsemi AS21011JB1",
> > +		.read_status	= as21xxx_read_status,
> > +		.led_brightness_set = as21xxx_led_brightness_set,
> > +		.led_hw_is_supported = as21xxx_led_hw_is_supported,
> > +		.led_hw_control_set = as21xxx_led_hw_control_set,
> > +		.led_hw_control_get = as21xxx_led_hw_control_get,
> > +		.led_polarity_set = as21xxx_led_polarity_set,
> > +	},
> 
> It is guaranteed by the current code that these entries are tried in
> the order listed here. If that was to change, other drivers would
> break.
> 
> So what you can do is have the first entry for PHY_ID_AS21XXX with
> as21xxx_probe, have the probe download the firmware and then return
> -ENODEV. PHY_ID_AS21XXX tells us there is no firmware, so this is what
> we need to do. The -ENODEV then tells the core that this driver entry
> does not match the hardware, try the next.
> 
> After the firmware download, the phylib core will still have the wrong
> ID values. So you cannot use PHY_ID_MATCH_EXACT(PHY_ID_AS21011JB1).
> But what you can do is have a .match_phy_device function. It will get
> called, and it can read the real ID from the device, and perform a
> match. If it does not match return -ENODEV, and the core will try the
> next entry.
> 
> You either need N match_phy_device functions, one per ID value, or you
> can make use of the .driver_data in phy_driver, and place the matching
> data there.
> 
> In the end you should have the correct phy_driver structure for the
> device. The core will still have the wrong ID values, which you should
> document with a comment. But that mostly only effects
> /sys/class/bus/mdio-bus/...
>

Thanks a lot for the hint on how to use .match_phy_device for this,
wasn't aware of the enforced PHY order.

I will investigate this but may I ask who creates the sysfs entry and
at what stage? After phy_register?

Cause if that is the case can't this be solved by making the PHY
function rescan the ID? Checking patch 1 of this series, it won't be
driver_release/attach but just a flag to read the PHY again and update
phy_device.

From what I observed, updating the phy_id entry in phy_device (or
c45_ids) is harmless. A rescan totally deletes the problem of having to
use the match functions.

-- 
	Ansuel