Hi Andrew,

On Wed, 2026-03-04 at 15:59 +0100, Andrew Lunn wrote:

quoted

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

quoted

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

quoted

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

quoted

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

quoted

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

quoted

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

quoted

+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