[BPi-R3-Mini] Airoha EN8811H 2.5G PHY driver

Banana Pi Router design BPI-R3/R3 Mini(MT7986)
81

Some bug still needs to be removed from the latest changes…

Edit:

Turns out it was a typo :wink:

82

Next up: interrupt support :slight_smile: Coming soon…

1 Like
83

How do you add support for that?

84

Actually, it already is… Only two function pointers commented out. But now I ran in to another bug (without the interrupt).

If link is up, and then link partner sets link down,via:

ip link set enxxxxx down

First, I see also on en8811h the link going down, but after a second, it is falsely reporting up again. I see this on both versions of firmware, and on multiple versions of my upstream driver. I’ll see if I can get more debugging info. I guess I could try on the original bpi openwrt version, but that will be somewhere next week.

I think I only tested link status, by actually disconnecting the wire, not so much with the command, so I did not notice it before.

This makes testing the interrupt a bit difficult, as this is the interrupt for link up/down change. So I have disabled the interrupt handling temporarily.

85

That version is not very usefull, as it does not use phylink and I do not see any kernel messages with link up/down on the other side…

86

Update:

Link status in the C45 register seems reliable. I still do not understand why the link status from the C22 register is not reliable.

So then I fixed the issue with genphy_c45_read_link(). This function uses phydev → c45_ids.mmds_present, but it is empty. So there now is a fix in probe() to set:

phydev->c45_ids.mmds_present |= MDIO_DEVS_PMAPMD | MDIO_DEVS_AN;

Now genphy_c45_read_link() functions correctly and also the interrupt support seems to work ok :slight_smile:

1 Like
87

I mean more like how did you know which register is for interrupt and how does the interrupt work.

88

Setup the correct pin in the DTS as interrupt edge falling… Then set the 2 functions pointers. Now interrupt is enabled. Use the magic number sequence to reset the interrupt pin. See the code. The value of the numbers is from Airoha.

89

Yes I know they are magic number but how did you know which ones are for interrupt.

they aren’t in the document I have

90

The name of the pin in the R3-mini schematics gave away the purpose of the pin :wink:

91

why have you removed the duplex-setting in last change?

@@ -994,7 +920,6 @@ static int en8811h_read_status(struct phy_device *phydev)
        switch (pbus_value & EN8811H_SPEED_MASK) {
        case EN8811H_SPEED_2500:
                phydev->speed = SPEED_2500;
-               phydev->duplex = DUPLEX_FULL;
                /* BUG in PHY: MDIO_AN_10GBT_STAT_LP2_5G does not get set
                 * Assume link partner advertised it, dirty patch for bug
                 */
@@ -1003,11 +928,9 @@ static int en8811h_read_status(struct phy_device *phydev)
                break;
        case EN8811H_SPEED_1000:
                phydev->speed = SPEED_1000;
-               phydev->duplex = DUPLEX_FULL;
                break;
        case EN8811H_SPEED_100:
                phydev->speed = SPEED_100;
-               phydev->duplex = DUPLEX_FULL;
                break;
        }
92

Because it was set twice to full duplex once should be enough and that is here:

It only supports full-duplex.

Edit:

So it has become: 

Bla bla bla

Bla bla bla
1 Like
94

So the last known issue, is not going to be resolved. It cannot be fixed with a firmware update, so we’ll have to live with it. This is the issue of not being reported about 2500base-t link partner advertisement. I will leave the ‘fix’ in place.

@dangowrt @frank-w @castiel652 :

Further, I was thinking, looking at the drivers/net/phy folder, what would be the suitable filename. I guess it should be:

drivers/net/phy/airoha.c

And the config name:

CONFIG_AIROHA_PHY

It would be easier to rename it now, then when it is already upstream… If Airoha comes with the next generation of phy…

95

You can send the driver as RFC patch and ask about the name.

Driver looks clean now.

This doesn’t seem to work on my end. phydev->link is always 0

I ended up just read the AN link status directly in read_status.

This should be fine. Some other drivers do this too.

96

Was this caused by the serdes polarity? Or was it casued by:

		/* Autoneg is being started, therefore disregard current
		 * link status and report link as down.
		 */
		if (val & MDIO_AN_CTRL1_RESTART) {
			phydev->link = 0;
			return 0;
		}

Or is it realy the MDIO_STAT1_LSTATUS

phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_STAT1);

Do you mean

phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_STAT1);

Instead of

phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_STAT1);
97

Not caused by serdes polarity. I’ve changed it to the value that works on my board.

yes this and check if MDIO_STAT1_LSTATUS is set.

98

What is the value that works on your board? In the code I have, both bits are set.

Of course, that part is the same, so I left it out for the question of the difference.

99

on mine only TX is set

100

Do you have the latest firmware binairies from linux-firmware?

Edit:

I see something changed over the evolution of the original driver:

/* v1.1.3 */
pbus_value = (pbus_value & 0xfffffffc) | EN8811H_RX_POLARITY_NORMAL | EN8811H_TX_POLARITY_NORMAL;

/* v1.2.1 They changed this for R3mini */ 
pbus_value = (pbus_value & 0xfffffffc) | EN8811H_RX_POLARITY_REVERSE | EN8811H_TX_POLARITY_NORMAL;
101

It depends on the board design. Mine is not BPI R3 Mini.