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

Christian Marangi
2025-03-26T15:09:30Z

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