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

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

Hi Russell,

On Wed, 2026-03-04 at 16:32 +0000, Russell King (Oracle) wrote:
> On Wed, Mar 04, 2026 at 10:35:29AM +0100, Louis-Alexis Eyraud wrote:
> > +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;
> > +
> > +	if (reg_val & AN8801_WOL_WAKE_MAGIC_EN)
> > +		wol->wolopts |= WAKE_MAGIC;
> > +	else
> > +		wol->wolopts &= ~WAKE_MAGIC;
> 
> Please only support WoL if you know that the PHY has been wired up in
> such a way to allow it to actually wake the system. The PHY itself
> merely supporting WoL is insufficient.
> 
> Please look at my recent change to realtek_main.c in commit
> b826bf795564 ("net: phy: realtek: fix RTL8211F wake-on-lan support")
> to see a possible way to achieve this.
> 
First, sorry for the delay, and thank you for pointing out this commit.

It indeed showed me what the WoL implementation for this PHY driver was
missing, not only for the get_wol/set_wol but also elsewhere in the
driver.

So for v2, I've reworked in a similar way the get_wol/set_wol, the
interrupt handling (to process differently the magic packet and the
link change interrupts) and also added custom probe, suspend and resume
callbacks (to be able to disable link change interrupt during suspend
time and enable it again after resume if the user has enabled the WoL
setting, like you did for RTL8211F).

I had a bit of trouble make it work right. At first I could not read
properly the PHY buckpbus registers in the suspend callback, and adding
a delay at resume time was needed as a workaround to make the WoL
behaviour work consistently. But in the end I found out it was the
Ethernet interface pinctrl config for sleep state in my board
devicetree that caused me those issues.
It works fine now without any workaround.

> > +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;
> 
> Are you sure this is safe, e.g. over a suspend/resume, and doesn't
> cause the hardware vs software state to desync? 
While reworking PHY WoL support, I've tested removing this EEE
disabling done during driver initial config and I did not notice any
particular issue, especially during suspend/resume sequences. Still
unsure why the downstream driver disabled it in first place.

The EEE support seems working fine too from what ethtool reports on my
board, so I'll remove the lines from v2.
> 
> > +
> > +	prev_page = phy_select_page(phydev,
> > AIR_PHY_PAGE_EXTENDED_1);
> > +	if (prev_page < 0)
> > +		return prev_page;
> 
> No, this is buggy. Please read the phy_select_page() documentation to
> find out why.
> 
> > +
> > +	/* Set the PHY to perform auto-downshift after 3 auto-
> > negotiation
> > +	 * attempts
> > +	 */
> > +	__phy_write(phydev, AN8801_EXT_REG_PHY,
> > +		    FIELD_PREP(AN8801_EXT_PHY_CTRL1, 0x1d) |
> > +		    FIELD_PREP(AN8801_EXT_PHY_DOWNSHIFT_CTL, 1) |
> > +		    AN8801_EXT_PHY_DOWNSHIFT_EN);
> > +
> > +	ret = phy_restore_page(phydev, prev_page, ret);
> > +	if (ret)
> > +		return ret;
> 
> However, the bug could've been avoided by using the appropriate
> accessor:
> 
> 	ret = phy_write_paged(phydev, AIR_PHY_PAGE_EXTENDED_1,
> 			      AN8801_EXT_REG_PHY,
> 			      FIELD_PREP(AN8801_EXT_PHY_CTRL1, 0x1d)
> |
> 			     
> FIELD_PREP(AN8801_EXT_PHY_DOWNSHIFT_CTL, 1) |
> 			      AN8801_EXT_PHY_DOWNSHIFT_EN);
> 	if (ret < 0)
> 		return ret;
thanks for catching this bug.
I've replaced for v2 the __phy_write call by phy_write_paged cas you
suggested.
> > +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;
> > +
> > +	if (!phydev->link)
> > +		return 0;
> > +
> > +	if (prev_speed != phydev->speed) {
> 
> Maybe:
> 
> 	if (phydev->link && prev_speed != phydev->speed) {
> 
> ?
Ack.

Thanks again for the review.

Regards,
Louis-Alexis
> 
> Thanks.