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

[Christian Marangi <ansuelsmth@gmail.com>]

On Thu, Mar 27, 2025 at 11:24:26AM +0000, Russell King (Oracle) wrote:
> On Thu, Mar 27, 2025 at 12:35:03AM +0100, Christian Marangi wrote:
> > +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;
> 
> Isn't phydev->phy_id zero here for a clause 45 PHY? If the firmware
> has been loaded, I believ eyou said that PHY_ID_AS21XXX won't be
> used, so the if() will be true, and because we've read clause 45
> IDs, phydev->phy_id will be zero meaning this will never match. So
> a PHY with firmware loaded won't ever match any of these drivers.
> This is probably not what you want.

You are right. I will generalize the function to skip having to redo the
logic. With FW loaded either c45 and c22 ID are filled in.

> 
> I'd suggest converting the tail of phy_bus_match() so that you can
> call that to do the standard matching using either C22 or C45 IDs
> as appropriate without duplicating that code.
> 
> > +
> > +	/* 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;
> > +
> > +	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;
> > +
> > +	mutex_destroy(&priv->ipc_lock);
> > +	kfree(priv);
> > +
> > +	/* Return not maching anyway as PHY ID will change after
> > +	 * firmware is loaded.
> 
> Also "This relies on the driver probe order."
> 
> > +	 */
> > +	return 0;
> > +}
> > +
> > +static struct phy_driver as21xxx_drivers[] = {
> > +	{
> > +		/* PHY expose in C45 as 0x7500 0x9410
> > +		 * before firmware is loaded.
> 
> Also "This driver entry must be attempted first to load the firmware and
> thus update the ID registers."
> 
> > +		 */
> > +		PHY_ID_MATCH_EXACT(PHY_ID_AS21XXX),
> > +		.name		= "Aeonsemi AS21xxx",
> > +		.match_phy_device = as21xxx_match_phy_device,
> > +	},
> > +	{
> > +		PHY_ID_MATCH_EXACT(PHY_ID_AS21011JB1),
> > +		.name		= "Aeonsemi AS21011JB1",
> > +		.probe		= as21xxx_probe,
> > +		.match_phy_device = as21xxx_match_phy_device,
> > +		.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,
> 
> If I'm reading these driver entries correctly, the only reason for
> having separate entries is to be able to have a unique name printed
> for each - the methods themselves are all identical.
> 
> My feeling is that is not a sufficient reason to duplicate the driver
> entries, which adds bloat (not only in terms of static data, but also
> the data structures necessary to support each entry in sysfs.) However,
> lets see what Andrew says.
>

If you remember that was one of my crazy project in trying to reduce the
array but I remember it did end up bad or abbandoned with the problem of
having to reinvent each PHY. Probably my changes caused too much patch
delta.

The proposal was exactly to pack all the struct that have similar OPs
with introducing an array of PHY ID for each driver.

-- 
	Ansuel