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

From: Maxime Chevallier
Date: Thu Mar 27 2025 - 04:24:36 EST


Hi Christian,

On Thu, 27 Mar 2025 00:35:03 +0100
Christian Marangi <ansuelsmth@xxxxxxxxx> wrote:

> Add support for Aeonsemi AS21xxx 10G C45 PHYs. These PHYs integrate
> an IPC to setup some configuration and require special handling to
> sync with the parity bit. The parity bit is a way the IPC use to
> follow correct order of command sent.
>
> Supported PHYs AS21011JB1, AS21011PB1, AS21010JB1, AS21010PB1,
> AS21511JB1, AS21511PB1, AS21510JB1, AS21510PB1, AS21210JB1,
> AS21210PB1 that all register with the PHY ID 0x7500 0x7510
> before the firmware is loaded.
>
> They all support up to 5 LEDs with various HW mode supported.
>
> While implementing it was found some strange coincidence with using the
> same logic for implementing C22 in MMD regs in Broadcom PHYs.
>
> For reference here the AS21xxx PHY name logic:
>
> 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
>
> Signed-off-by: Christian Marangi <ansuelsmth@xxxxxxxxx>

[...]

I know this is only RFC, but I have some questions

> +static int as21xxx_match_phy_device(struct phy_device *phydev,
> + const struct phy_driver *phydrv)
> +{
> + struct as21xxx_priv *priv;
> + u32 phy_id;
> + int ret;
> +
> + /* Skip PHY that are not AS21xxx or already have firmware loaded */
> + if (phydev->c45_ids.device_ids[MDIO_MMD_PCS] != PHY_ID_AS21XXX)
> + return phydev->phy_id == phydrv->phy_id;
> +
> + /* Read PHY ID to handle firmware just loaded */
> + ret = phy_read_mmd(phydev, MDIO_MMD_PCS, MII_PHYSID1);
> + if (ret < 0)
> + return ret;
> + phy_id = ret << 16;
> +
> + ret = phy_read_mmd(phydev, MDIO_MMD_PCS, MII_PHYSID2);
> + if (ret < 0)
> + return ret;
> + phy_id |= ret;
> +
> + /* With PHY ID not the generic AS21xxx one assume
> + * the firmware just loaded
> + */
> + if (phy_id != PHY_ID_AS21XXX)
> + return phy_id == phydrv->phy_id;
> +
> + /* Allocate temp priv and load the firmware */
> + priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + mutex_init(&priv->ipc_lock);
> +
> + ret = aeon_firmware_load(phydev);
> + if (ret)
> + return ret;

Here, and below, you leak priv by returning early.

> +
> + ret = aeon_ipc_sync_parity(phydev, priv);
> + if (ret)
> + return ret;
> +
> + /* Enable PTP clk if not already Enabled */
> + ret = phy_set_bits_mmd(phydev, MDIO_MMD_VEND1, VEND1_PTP_CLK,
> + VEND1_PTP_CLK_EN);
> + if (ret)
> + return ret;
> +
> + ret = aeon_dpc_ra_enable(phydev, priv);
> + if (ret)
> + return ret;

Does all of the above with sync_parity, PTP clock cfg and so on needs
to be done in this first pass of the matching process for this PHY ?

From what I got from the discussions, the only important bit is to load
the FW to get the correct PHY id ?

> + mutex_destroy(&priv->ipc_lock);
> + kfree(priv);
> +
> + /* Return not maching anyway as PHY ID will change after
> + * firmware is loaded.
> + */
> + return 0;
> +}

Maxime