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

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

To whom it may concern:

I’ve changed the original version of the driver for Airoha EN8811H 2.5G PHY a bit. It is still ‘work in progress’. The EN8811H is used on the BPI-R3mini.

Main goal is to have it functional on kernel v6+ and have it using phylink.

Using phylink, you can see/change all the usual settings of the PHY using ethtool.

Use as much as possible generic Clause 22 code.

I’ve noted all major changes and found hardware bugs at the top of the source.

Known issues:

  • MDIO_AN_10GBT_STAT_LP2_5G does not get set by phy when it should be (some other bits in this register do change).
  • When link partner connects at 100M with autoneg off, link does not go up (also using original driver).

TODO:

  • Get datasheet
  • More testing
  • Switch to sgmii mode when link-partner is at 1000M/100M (needs datasheet)
  • Interrupt handling (If not configured at firmware load, also needs datasheet)
  • Fix known bugs (fix in firmware?) or find workaround

Edit:

Most recent version here: wip branch

2 Likes
2

Thanks for working on it

Why this change?

Load firmware from /lib/firmware/airoha/ instead of /lib/firmware/

Firmware blobs are not yet in linux firmware and this will break fw load after upgrade kernel

3

I changed it, because most files in linux-firmware follow this convention. I would assume it helps keeping the folder-tree clean. I could add the possibility for both paths. request_firmware_direct() will always look in /lib/firmware, so I’ve only added the subfolder airoha,

Anyway, I would send a RFC patch first and would see which comments it would get…

Also, without the datasheet, this is as far as I can go with the code. So I will clean the code and that is it for now…

I’ve tried contact airoha about datasheet, but there is no reply.

About firmware I read:

Your commit **must** contain a `Signed-Off-By:` from someone authoritative on
the licensing of the firmware in question (i.e. from within the company
that owns the code)

So we also need contact with Airoha…

Edit:

I’ve read that airoha somehow is connected with mediatek, so maybe someone at mediatek can help?

4

While it’s true that this is needed on the R3mini, be aware that on the PoE-variant of the R4 SinoVoip is intending to use the internal 2.5G PHY of MT7988 which requires yet another driver as well as firmware binaries.

5

Thanks for the info. I’ve removed the R4 part in the first post.

6

This guess was from me (in pm with eric) as i thought r4 use this phy too for the poe variant because you have this driver in your wip tree too. Thanks for clarification,sorry for confusion

7

Making the code pass checkpatch.pl, it needed a lot of changes.

So it was time to clean the code up, et voila!

Pretty much ready.

However, one warning I do not understand:

scripts/checkpatch.pl --strict 0001-Add-the-Airoha-EN8811H-PHY-driver.patch 
WARNING: please write a help paragraph that fully describes the config symbol
#25: FILE: drivers/net/phy/Kconfig:405:
+config AIR_EN8811H_PHY
+	tristate "Drivers for Airoha EN8811H 2.5 Gigabit PHY"
+	help
+	  Currently supports the Airoha EN8811H PHY.
+

It has a help paragraph right?

8

Try writing more than one line in the help :slight_smile:

9 
scripts/checkpatch.pl --strict 0001-Add-the-Airoha-EN8811H-PHY-driver.patch 
WARNING: please write a help paragraph that fully describes the config symbol
#25: FILE: drivers/net/phy/Kconfig:405:
+config AIR_EN8811H_PHY
+	tristate "Drivers for Airoha EN8811H 2.5 Gigabit PHY"
+	help
+	  Currently supports the Airoha EN8811H PHY. Now I make the paragraph
+	  longer so it will not give the warning...
+

Perhaps it just always give this warning…

But anyway, what about the code?

And what if I get no reply from airoha/mediatek? Just send the code mainline? Still need someone to sign off on the firmware though.

10

It can actually use Clause 45 if you have access to the datasheet. :wink:

This is not supported at the moment.

Also I would suggest you to drop all the Airoha LED functions since now there’s a generic way to handle PHY LED in upstream.

11

Do you know who I can contact for this?

Thanks for the tip and looking into my efforts. Do you know which mainline driver would be the best example on how to implement this?

12

Sorry I can’t help with that.

mediatek-ge-soc

13

I’m really looking for it, but all led functions use MDIO_MMD_VEND2 ( or MDIO_MMD_VEND1) and thus these are vendor specific registers. Also in drivers/net/phy/mediatek-ge-soc.c. I haven’t found any generic led functions, but I’ll be happy if you can correct me and point me to this.

Edit:

There are these:

		.led_blink_set	= mt798x_phy_led_blink_set,
		.led_brightness_set = mt798x_phy_led_brightness_set,
		.led_hw_is_supported = mt798x_phy_led_hw_is_supported,
		.led_hw_control_set = mt798x_phy_led_hw_control_set,
		.led_hw_control_get = mt798x_phy_led_hw_control_get,

So are you suggesting I move the call to the vendor specific led functions from config_init() to there?

14

PHY LED control registers are always vendor-specific registers. There aren’t any generic ones.

May I ask why you prefer C22 over C45? With more complex PHYs, using C22 usually means having to take care of page selection (ie. set page, access register, restore page; all without being interrupted by e.g. PHY polling, so needs locking, …), which ends up to be quite ugly instead of having atomic C45 operations.

15

Actually I do not. The original code uses C45 over C22, so until now I haven’t changed this. A datasheet would be helpful to make this transition.

Edit:

Actually it also uses air_buckpbus_reg_write() and air_buckpbus_reg_read() which is even more ugly then C45 over C22.

16

Oh, I missed that part:

    .read_mmd       = air_mii_cl45_read,
    .write_mmd      = air_mii_cl45_write,

Looking at the actual implementation this looks like standard C45-over-C22, so I guess you can drop all that entirely and Linux/phylib should automatically do the exact same thing should you define the PHY to be accessible only via C22.

Edit: Ironically they are even using the macros (MII_MMD_ACC_CTL_REG, …) used for the identical generic implementation.

17

You will need to implement those for EN8811H.

Reason I suggest mediatek-ge-soc is that the LED register address are the same.

18

I’ve had problems with the driver hanging (I think accessing the eee registers), so I have done this:

	.read_mmd		= genphy_read_mmd_unsupported,
	.write_mmd		= genphy_write_mmd_unsupported,

To prevent the genphy functions from calling this on some registers where it seems to hang…

I might install a filter, where it does get supported, only on certain registers.

19

Thanks, do you think the contents of the registers are also the same?

20

Mostly. EN8811H has some extra LED bits for 2500M