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

[Andrew Lunn]

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

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

> +/* 5 LED at step of 0x20
> + * FE: Fast-Ethernet (100)
> + * GE: Gigabit-Ethernet (1000)
> + * NG: New-Generation (2500/5000/10000)
> + * (Lovely ChatGPT managed to translate meaning of NG)

It might be a reference to NBase-T Gigabit.

Please add a comment somewhere about how locking works for IPCs. As
far as i see, the current locking scheme is that IPCs are only called
from probe, so no locking is actually required. But:

> +#define IPC_CMD_NG_TESTMODE		0x1b /* Set NG test mode and tone */
> +#define IPC_CMD_TEMP_MON		0x15 /* Temperature monitoring function */
> +#define IPC_CMD_SET_LED			0x23 /* Set led */

suggests IPCs might in the future be needed outside of probe, and then
a different locking scheme might be needed, particularly for
temperature monitoring.

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

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

	Andrew