Re: [PATCH net-next 2/2] net: phy: Introduce Airoha AN8801/R Gigabit Ethernet PHY driver

[Louis-Alexis Eyraud <louisalexis.eyraud@collabora.com>]

Hi Andrew,

On Wed, 2026-03-04 at 15:59 +0100, Andrew Lunn wrote:
> > +static int an8801r_did_interrupt(struct phy_device *phydev)
> > +{
> > +	u32 irq_en, irq_status;
> > +	int ret;
> > +
> > +	ret = air_buckpbus_reg_read(phydev,
> > AN8801_BPBUS_REG_WAKE_IRQ_EN,
> > +				    &irq_en);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = air_buckpbus_reg_read(phydev,
> > AN8801_BPBUS_REG_WAKE_IRQ_STS,
> > +				    &irq_status);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (irq_status & AN8801_IRQ_WAKE_MAGICPKT)
> > +		return 0;
> 
> With a name like an8801r_did_interrupt() you would expect the return
> value to be some value of True, if there was an interrupt. I would
> suggest either a different name, or return 1. Maybe also add a
> kerneldoc header indicating the return values, since it is probably
> not going to be standard.
> 
The function name was not great and confusing.

While reworking the interrupt handling for WoL support for v2, I merged
the function with the an8801r_handle_interrupt function (to process
differently the Magic Packet and Link Change interrupts) so this
function won't be in v2.
 
> > +static void an8801r_get_wol(struct phy_device *phydev,
> > +			    struct ethtool_wolinfo *wol)
> > +{
> > +	u32 reg_val;
> > +
> > +	air_buckpbus_reg_read(phydev,
> > AN8801_BPBUS_REG_WAKEUP_CTL1, &reg_val);
> > +
> > +	wol->supported = WAKE_MAGIC;
> 
> How does WoL work on this device. Only via interrupts? If so, maybe
> you should only return WAKE_MAGIC as supported if there is a valid
> interrupt?
> 
Yes, the WoL works via interrupts and indeed it lacks the interrupt
validity check.

In v2, following the RTL8211F wake-on-lan support example given to me
by Russell in his review, I'll fix this by adding a check using
device_can_wakeup in an8801r_get_wol and an8801r_set_wol functions and
adding a probe function (there was none in v1) to mark the PHY device
as wakeup capable if it has a valid interrupt and if the wakeup-source
property is present in the devicetree for the device node.

> > +static int an8801r_rgmii_delay_config(struct phy_device *phydev)
> > +{
> > +	switch (phydev->interface) {
> > +	case PHY_INTERFACE_MODE_RGMII_TXID:
> > +		return an8801r_rgmii_txdelay(phydev, 4);
> > +	case PHY_INTERFACE_MODE_RGMII_RXID:
> > +		return an8801r_rgmii_rxdelay(phydev, 0);
> > +	case PHY_INTERFACE_MODE_RGMII_ID:
> > +		return an8801r_rgmii_txdelay(phydev, 4);
> > +		return an8801r_rgmii_rxdelay(phydev, 0);
> 
> The parameters look very odd here. 4 means 2ns, but 0 also means 0ns?
> Can this API be improved?
From the info I finally got about these magic values, the differences
between the RX and TX values for the default insert delays can be
explained by the additional RGMII_RXDELAY_ALIGN bit setting when
writing the RX delay register (AN8801_BPBUS_REG_RXDLY_STEP), done by
an8801r_rgmii_rxdelay function because it adds an extra offset.

For TX, the 4 value is the delay step value that is the closest to 2ns
(1.883ns). But For RX, setting the step value to 0 and setting the
RGMII_RXDELAY_ALIGN bit too, inserts a 1.992ns delay. Without align
bit, it would indeed be -0.008ns.

Those delays are also inserted because the an8801r_rgmii_rxdelay and
an8801r_rgmii_txdelay function set the force mode bit
(RGMII_RXDELAY_FORCE_MODE / RGMII_TXDELAY_FORCE_MODE). 
If this bit is unset, it prevents inserting a delay.

> Also, PHY_INTERFACE_MODE_RGMII_TXID means 2ns delay for TX, but it
> also means 0ns delay for RX. The code appears to be missing this
> second part.
> 
There is indeed a bug with this double return in
PHY_INTERFACE_MODE_RGMII_ID case so the RX delay is not inserted.

> > +	case PHY_INTERFACE_MODE_RGMII:
> 
> And here you should be disabling all delays. We have seen boards
> where
> the strapping is wrong, the PHY boots in RGMII_ID, but RGMII is
> required, and so the driver must fully implement
> PHY_INTERFACE_MODE_RGMII disabling the delays.
> 
You're right it is also missing the delay disabling part.

The an8801r_rgmii_txdelay and an8801r_rgmii_rxdelay function don't
allow to disable them since they always set force mode bit and because
the 0 values does not completely mean no inserted delay.
Their implementations should be modify to be able to do that.

For v2, I've reworked the an8801r_rgmii_delay_config and
an8801r_rgmii_rx/txdelay to handle properly all RGMII configuration
cases and I hope in a simpler manner.

> > +static int an8801r_config_init(struct phy_device *phydev)
> > +{
> > +	u8 led_default_function[AN8801R_NUM_LEDS] = { 0 };
> > +	int prev_page, ret;
> > +
> > +	ret = an8801r_of_init_leds(phydev, led_default_function);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* Disable Low Power Mode (LPM) */
> > +	ret = phy_write_mmd(phydev, MDIO_MMD_VEND2,
> > AN8801_REG_PHY_INTERNAL0,
> > +			    FIELD_PREP(AN8801_PHY_INTFUNC_MASK,
> > 0x1e));
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = phy_write_mmd(phydev, MDIO_MMD_VEND2,
> > AN8801_REG_PHY_INTERNAL1,
> > +			    FIELD_PREP(AN8801_PHY_INTFUNC_MASK,
> > 0x2));
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* Disable EEE by default */
> > +	ret = phy_write_mmd(phydev, MDIO_MMD_AN, MDIO_AN_EEE_ADV,
> > 0);
> > +	if (ret)
> > +		return ret;
> 
> Why? If EEE is broken, this is not sufficient to stop a user
> re-enabling it.
> 
I've tested by removing these lines and EEE seems working fine.
I'll remove them in v2.

> > +static int an8801r_read_status(struct phy_device *phydev)
> > +{
> > +	int prev_speed, ret;
> > +	u32 val;
> > +
> > +	prev_speed = phydev->speed;
> > +
> > +	ret = genphy_read_status(phydev);
> > +	if (ret)
> > +		return ret;
> 
> You configure the PHY to support downshift. If it has performed a
> downshift, does it report the actual speed in the usual registers
> read
> by genphy_read_status(), or is it necessary to read a vendor
> register?
From the tests I've done, I got the actual speed read correctly by
genphy_read_status function.
I did a test by adding in this function the vendor register reading at
the same time and comparing it and did not get a discrepancy too when
switching with ethtool between different speed configurations.

Would it be more reliable to use the vendor register instead?

> 
> > +static struct phy_driver airoha_driver[] = {
> > +{
> > +	PHY_ID_MATCH_MODEL(AN8801R_PHY_ID),
> > +	.name			= "Airoha AN8801R",
> > +	.features		= PHY_GBIT_FEATURES,
> 
> Should not be needed, if the PHY enumerates its capabilities
> correctly.
I confirm it is not needed, I'll remove it in v2.

Thanks for the review.

Regards,
Louis-Alexis

> 
>     Andrew
> 
> ---
> pw-bot: cr