phy: add support for airoha en8811h 2.5G PHY#422
phy: add support for airoha en8811h 2.5G PHY#422Yuzhii0718 wants to merge 1 commit intohanwckf:openwrt-21.02from
Conversation
There was a problem hiding this comment.
Pull request overview
This PR replaces the previous EN8811H PHY kernel-tree patch approach with an updated Airoha EN8811H (v1.2.5) out-of-tree driver package, and updates MT7986 device images to depend on the renamed kernel module package.
Changes:
- Remove the old mediatek kernel patch that injected
air_en8811hinto the 5.4 kernel tree and drop the oldkmod-phy-air-en8811hkernel module definition. - Add a new
package/airoha/drivers/an8811kernel module package containing the EN8811H driver sources and firmware assets. - Update MT7986 (BPI-R3MINI*) images to use
kmod-phy-airoha-en8811h.
Reviewed changes
Copilot reviewed 13 out of 16 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| target/linux/mediatek/patches-5.4/9999-mt7986a-bananapi-bpi-r3-air_en8811-2.5G-phy.patch | Removes the old in-tree kernel patch for EN8811H. |
| target/linux/mediatek/image/mt7986.mk | Switches device package dependency to kmod-phy-airoha-en8811h. |
| package/kernel/linux/modules/netdevices.mk | Removes the old KernelPackage/phy-air-en8811h definition. |
| package/airoha/drivers/an8811/Makefile | Introduces a new OpenWrt kernel module package and installs firmware files. |
| package/airoha/drivers/an8811/README.md | Adds English documentation for the new package. |
| package/airoha/drivers/an8811/document/README.sc.md | Adds Simplified Chinese documentation for the new package. |
| package/airoha/drivers/an8811/src/mtk/Makefile | Adds out-of-tree module build rules for air_en8811h. |
| package/airoha/drivers/an8811/src/mtk/Kconfig | Adds module-local Kconfig options (currently not integrated into the kernel Kconfig tree). |
| package/airoha/drivers/an8811/src/mtk/air_en8811h_main.c | Adds the v1.2.5 EN8811H PHY driver main implementation. |
| package/airoha/drivers/an8811/src/mtk/air_en8811h_api.c | Adds EN8811H MDIO/PBUS access helpers and optional debugfs tooling (contains several correctness issues). |
| package/airoha/drivers/an8811/src/mtk/air_en8811h_api.h | Adds API declarations and debugfs constants. |
| package/airoha/drivers/an8811/src/mtk/air_en8811h.h | Adds shared defines/structs and driver version update to v1.2.5. |
| package/airoha/drivers/an8811/src/mtk/air_en8811h_fw.h | Adds embedded firmware arrays used by the driver. |
| package/airoha/drivers/an8811/src/mtk/EthMD32.dm.bin | Adds DM firmware binary asset. |
| package/airoha/drivers/an8811/src/mtk/EthMD32.DSP.bin | Adds DSP firmware binary asset. |
| package/airoha/drivers/an8811/src/mtk/EthMD32_PM_CheckSum.txt | Adds a firmware checksum reference file. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (val < 0 || val > 1) { | ||
| dev_err(dev, | ||
| "airoha,surge value %u out of range.", | ||
| val); | ||
| return -EINVAL; |
There was a problem hiding this comment.
airoha,surge is read into a u32 val, so the val < 0 part of the range check is ineffective. The check should only validate the upper bound (e.g., val > 1) and/or use a signed type if negatives are meaningful.
| "[debugfs] = /sys/kernel/debug/mdio-bus\':[phy_addr]\n" | ||
| "echo [tx polarity] [rx polarity] > /[debugfs]/polarity\n" | ||
| "option: tx_normal, tx_reverse, rx_normal, rx_revers\n"); | ||
| } | ||
|
|
||
| static int air_set_polarity(struct phy_device *phydev, int tx_rx) | ||
| { | ||
| int ret = 0; | ||
| unsigned int pbus_data = 0; | ||
|
|
||
| pr_debug("\nPolarit %s\n", tx_rx_string[tx_rx]); |
There was a problem hiding this comment.
The debugfs help text has typos that could confuse users (rx_revers, Polarit). Please correct these strings (e.g., rx_reverse, Polarity).
| if (!dir) { | ||
| dev_err(phydev_dev(phydev), "%s:err at %d\n", | ||
| __func__, __LINE__); | ||
| ret = -ENOMEM; |
There was a problem hiding this comment.
If debugfs_create_dir() fails, the function sets ret = -ENOMEM but still continues creating files with dir == NULL and stores priv->debugfs_root = dir. This can lead to unexpected placement under the debugfs root and/or NULL derefs later. Return immediately on failure (or gate file creation on dir).
| ret = -ENOMEM; | |
| ret = -ENOMEM; | |
| return ret; |
| ### 1) Firmware | ||
|
|
||
| The driver loads firmware using `request_firmware_direct()`: | ||
|
|
||
| - `EthMD32.dm.bin` | ||
| - `EthMD32.DSP.bin` | ||
|
|
||
| Ensure these files are present in the target system’s `/lib/firmware/`. |
There was a problem hiding this comment.
The README states the driver loads firmware via request_firmware_direct() from /lib/firmware/, but the current driver code embeds firmware arrays via air_en8811h_fw.h and calls air_mdio_write_buf(...) (no request_firmware* usage). Please update the documentation to match the implementation (or switch the driver back to runtime firmware loading if that’s the goal).
| KCONFIG:= \ | ||
| CONFIG_AIROHA_EN8811H_PHY=m \ | ||
| CONFIG_AIROHA_EN8811H_PHY_DEBUGFS=n |
There was a problem hiding this comment.
The KCONFIG entries set CONFIG_AIROHA_EN8811H_PHY*, but these symbols don’t exist in the kernel Kconfig tree (only in the external module’s unused src/mtk/Kconfig). As a result, these settings won’t reliably affect compilation (e.g., enabling debugfs). Consider controlling module features via ccflags-y/EXTRA_CFLAGS or keying off existing kernel symbols like CONFIG_DEBUG_FS instead of non-existent ones.
| KCONFIG:= \ | |
| CONFIG_AIROHA_EN8811H_PHY=m \ | |
| CONFIG_AIROHA_EN8811H_PHY_DEBUGFS=n |
| memset(buf, 0, 64); | ||
| if (copy_from_user(buf, buffer, count)) | ||
| return -EFAULT; |
There was a problem hiding this comment.
airphy_debugfs_pbus() copies count bytes from userspace into a fixed 64-byte stack buffer without validating count first. This is a stack overflow risk. Add the same if (count > sizeof(buf) - 1) return -EINVAL; guard used in airphy_debugfs_buckpbus() (and consider NUL-terminating before parsing).
| memset(buf, 0, 64); | |
| if (copy_from_user(buf, buffer, count)) | |
| return -EFAULT; | |
| memset(buf, 0, 64); | |
| if (count > sizeof(buf) - 1) | |
| return -EINVAL; | |
| if (copy_from_user(buf, buffer, count)) | |
| return -EFAULT; | |
| buf[count] = '\0'; |
| if (buf[0] == 'w') { | ||
| if (sscanf(buf, "w %x %x", ®, &val) == -1) | ||
| return -EFAULT; | ||
|
|
There was a problem hiding this comment.
Several debugfs handlers use sscanf(...) == -1 to validate input (e.g., here for the write path). sscanf returns the number of successfully parsed items, so malformed input can return 0/1/2 (not -1) and leave reg/val uninitialized, leading to unintended register reads/writes. Validate the exact expected count (e.g., != 2).
| seq_printf(seq, "| Boot mode : %s\n", | ||
| ((val & BIT(24)) >> 24) ? "Flash" : "Download Code"); | ||
| seq_printf(seq, "| EthMD32.dm.bin CRC32: %08x\n", | ||
| priv->dm_crc32); | ||
| seq_printf(seq, "| EthMD32.DSP.bin CRC32: %08x\n", | ||
| priv->dsp_crc32); | ||
| val = air_buckpbus_reg_read(phydev, 0x3b3c); |
There was a problem hiding this comment.
airphy_info_show() prints priv->dm_crc32 and priv->dsp_crc32, but these fields are never assigned anywhere in the driver (so they’ll always read as 0/garbage). Either compute/store the CRC32 values during firmware load/init or remove these fields from the debugfs output.
| struct device *dev = phydev_dev(phydev); | ||
| int ret = 0; | ||
| u32 pbus_value = 0; | ||
| struct en8811h_priv *priv = phydev->priv; |
There was a problem hiding this comment.
en8811h_load_firmware() declares struct en8811h_priv *priv = phydev->priv; but never uses it. Please remove the unused variable (or use it to populate fields like firmware CRCs if that was the intent) to avoid warnings and keep the function clear.
| struct en8811h_priv *priv = phydev->priv; |
| ### 1) 固件 | ||
|
|
||
| 驱动通过 `request_firmware_direct()` 加载: | ||
|
|
||
| - `EthMD32.dm.bin` | ||
| - `EthMD32.DSP.bin` | ||
|
|
||
| 确保它们在目标系统 `/lib/firmware/` 中可见。 | ||
|
|
There was a problem hiding this comment.
文档写明驱动通过 request_firmware_direct() 从 /lib/firmware/ 加载固件,但当前实现是通过 air_en8811h_fw.h 内嵌固件数据并直接写入(代码中无 request_firmware* 调用)。请同步更新说明(或改回运行时加载固件)。
59aafcd to
b64b93c
Compare
b64b93c to
6b03a81
Compare
In the current repository, the driver for en8811h depends on
target/linux/mediatek/patches-5.4/9999-mt7986a-bananapi-bpi-r3-air_en8811-2.5G-phy.patch, which doesn't work well. I updated to version v1.2.5 of airoha, and it works correctly.change
kmod-phy-air-en8811htokmod-phy-airoha-en8811h