[PATCH net-next v3 0/2] Add built-in 2.5G ethernet phy support on MT7988

[PATCH net-next v3 0/2] Add built-in 2.5G ethernet phy support on MT7988

[Sky Huang]

From: Sky Huang <skylake.huang@mediatek.com>

This patchset adds support for built-in 2.5Gphy on MT7988, change file
and config sequence in related Kconfig and Makefile.

---
Change in v2:
- Add missing dt-bindings and dts node.
- Remove mtk_phy_leds_state_init() temporarily. I'm going to add LED support
later.
- Remove "Firmware loading/trigger ok" log.
- Add macro define for 0x800e & 0x800f

Change in v3:
1. Remove unnecessary headers and unnecessary print log.
2. ioremap IO space (MT7988_2P5GE_PMB_FW_BASE/MTK_2P5GPHY_MCU_CSR_BASE)
directly instead of of_iomap() compatible node (mediatek,2p5gphy-fw) from dtsi
3. Call mt798x_2p5ge_phy_load_fw() from .probe instead of .config_init. This is
tested ok with openWRT-24.10 and "mediatek/mt7988/i2p5ge-phy-pmb.bin" firmware
is embedded into kernel image instead of rootfs image.
4. Use request_firmware_direct instead of request_firmware. We don't want sysfs
fallback in this driver to avoid blocking.
5. Remove mtk_i2p5ge_phy_priv sturct since it's not used anymore.
---
Sky Huang (2):
  net: phy: mediatek: Sort config and file names in Kconfig and Makefile
  net: phy: mediatek: add driver for built-in 2.5G ethernet PHY on
    MT7988

 MAINTAINERS                          |   1 +
 drivers/net/phy/mediatek/Kconfig     |  31 ++-
 drivers/net/phy/mediatek/Makefile    |   3 +-
 drivers/net/phy/mediatek/mtk-2p5ge.c | 322 +++++++++++++++++++++++++++
 4 files changed, 346 insertions(+), 11 deletions(-)
 create mode 100644 drivers/net/phy/mediatek/mtk-2p5ge.c

-- 
2.45.2

[PATCH net-next v3 1/2] net: phy: mediatek: Sort config and file names in Kconfig and Makefile

[Sky Huang]

From: Sky Huang <skylake.huang@mediatek.com>

Sort config and file names in Kconfig and Makefile in
drivers/net/phy/mediatek/ according to sequence in MAINTAINERS.

Signed-off-by: Sky Huang <skylake.huang@mediatek.com>
---
 drivers/net/phy/mediatek/Kconfig  | 28 ++++++++++++++--------------
 drivers/net/phy/mediatek/Makefile |  2 +-
 2 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/drivers/net/phy/mediatek/Kconfig b/drivers/net/phy/mediatek/Kconfig
index 4308002bb82c..3abf23e37b4b 100644
--- a/drivers/net/phy/mediatek/Kconfig
+++ b/drivers/net/phy/mediatek/Kconfig
@@ -1,18 +1,4 @@
 # SPDX-License-Identifier: GPL-2.0-only
-config MTK_NET_PHYLIB
-	tristate
-
-config MEDIATEK_GE_PHY
-	tristate "MediaTek Gigabit Ethernet PHYs"
-	select MTK_NET_PHYLIB
-	help
-	  Supports the MediaTek non-built-in Gigabit Ethernet PHYs.
-
-	  Non-built-in Gigabit Ethernet PHYs include mt7530/mt7531.
-	  You may find mt7530 inside mt7621. This driver shares some
-	  common operations with MediaTek SoC built-in Gigabit
-	  Ethernet PHYs.
-
 config MEDIATEK_GE_SOC_PHY
 	tristate "MediaTek SoC Ethernet PHYs"
 	depends on ARM64 || COMPILE_TEST
@@ -26,3 +12,17 @@ config MEDIATEK_GE_SOC_PHY
 	  the MT7981 and MT7988 SoCs. These PHYs need calibration data
 	  present in the SoCs efuse and will dynamically calibrate VCM
 	  (common-mode voltage) during startup.
+
+config MTK_NET_PHYLIB
+	tristate
+
+config MEDIATEK_GE_PHY
+	tristate "MediaTek Gigabit Ethernet PHYs"
+	select MTK_NET_PHYLIB
+	help
+	  Supports the MediaTek non-built-in Gigabit Ethernet PHYs.
+
+	  Non-built-in Gigabit Ethernet PHYs include mt7530/mt7531.
+	  You may find mt7530 inside mt7621. This driver shares some
+	  common operations with MediaTek SoC built-in Gigabit
+	  Ethernet PHYs.
diff --git a/drivers/net/phy/mediatek/Makefile b/drivers/net/phy/mediatek/Makefile
index 814879d0abe5..ff13205d614f 100644
--- a/drivers/net/phy/mediatek/Makefile
+++ b/drivers/net/phy/mediatek/Makefile
@@ -1,4 +1,4 @@
 # SPDX-License-Identifier: GPL-2.0
+obj-$(CONFIG_MEDIATEK_GE_SOC_PHY)	+= mtk-ge-soc.o
 obj-$(CONFIG_MTK_NET_PHYLIB)		+= mtk-phy-lib.o
 obj-$(CONFIG_MEDIATEK_GE_PHY)		+= mtk-ge.o
-obj-$(CONFIG_MEDIATEK_GE_SOC_PHY)	+= mtk-ge-soc.o
-- 
2.45.2

Re: [PATCH net-next v3 1/2] net: phy: mediatek: Sort config and file names in Kconfig and Makefile

[Andrew Lunn]

On Wed, May 14, 2025 at 06:57:37PM +0800, Sky Huang wrote:
> From: Sky Huang <skylake.huang@mediatek.com>
> 
> Sort config and file names in Kconfig and Makefile in
> drivers/net/phy/mediatek/ according to sequence in MAINTAINERS.

If you use "make menuconfig" you will notice PHYs are sorted by
tristate string. So having Gigabit before Soc is correct.

> --- a/drivers/net/phy/mediatek/Makefile
> +++ b/drivers/net/phy/mediatek/Makefile
> @@ -1,4 +1,4 @@
>  # SPDX-License-Identifier: GPL-2.0
> +obj-$(CONFIG_MEDIATEK_GE_SOC_PHY)	+= mtk-ge-soc.o
>  obj-$(CONFIG_MTK_NET_PHYLIB)		+= mtk-phy-lib.o
>  obj-$(CONFIG_MEDIATEK_GE_PHY)		+= mtk-ge.o
> -obj-$(CONFIG_MEDIATEK_GE_SOC_PHY)	+= mtk-ge-soc.o

These should be in alphabetic order based on CONFIG_. So
CONFIG_MTK_NET_PHYLIB is what should move.

    Andrew

---
pw-bot: cr

Re: [PATCH net-next v3 1/2] net: phy: mediatek: Sort config and file names in Kconfig and Makefile

[SkyLake Huang (黃啟澤)]

On Wed, 2025-05-14 at 14:09 +0200, Andrew Lunn wrote:
> 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> On Wed, May 14, 2025 at 06:57:37PM +0800, Sky Huang wrote:
> > From: Sky Huang <skylake.huang@mediatek.com>
> > 
> > Sort config and file names in Kconfig and Makefile in
> > drivers/net/phy/mediatek/ according to sequence in MAINTAINERS.
> 
> If you use "make menuconfig" you will notice PHYs are sorted by
> tristate string. So having Gigabit before Soc is correct.
> 
> > --- a/drivers/net/phy/mediatek/Makefile
> > +++ b/drivers/net/phy/mediatek/Makefile
> > @@ -1,4 +1,4 @@
> >  # SPDX-License-Identifier: GPL-2.0
> > +obj-$(CONFIG_MEDIATEK_GE_SOC_PHY)    += mtk-ge-soc.o
> >  obj-$(CONFIG_MTK_NET_PHYLIB)         += mtk-phy-lib.o
> >  obj-$(CONFIG_MEDIATEK_GE_PHY)                += mtk-ge.o
> > -obj-$(CONFIG_MEDIATEK_GE_SOC_PHY)    += mtk-ge-soc.o
> 
> These should be in alphabetic order based on CONFIG_. So
> CONFIG_MTK_NET_PHYLIB is what should move.
> 
>     Andrew
> 
> ---
> pw-bot: crq:

Oops. I misunderstood what you said in previous patchset.
I'll rearrange this to:
obj-$(CONFIG_MEDIATEK_GE_PHY)        += mtk-ge.o
obj-$(CONFIG_MEDIATEK_GE_SOC_PHY)    += mtk-ge-soc.o
obj-$(CONFIG_MTK_NET_PHYLIB)         += mtk-phy-lib.o

BRs,
Sky

[PATCH net-next v3 2/2] net: phy: mediatek: add driver for built-in 2.5G ethernet PHY on MT7988

[Sky Huang]

From: Sky Huang <skylake.huang@mediatek.com>

Add support for internal 2.5Gphy on MT7988. This driver will load
necessary firmware and add appropriate time delay to make sure
that firmware works stably. The firmware loading procedure takes
about 11ms in this driver.

Signed-off-by: Sky Huang <skylake.huang@mediatek.com>
---
 MAINTAINERS                          |   1 +
 drivers/net/phy/mediatek/Kconfig     |  11 +
 drivers/net/phy/mediatek/Makefile    |   1 +
 drivers/net/phy/mediatek/mtk-2p5ge.c | 322 +++++++++++++++++++++++++++
 4 files changed, 335 insertions(+)
 create mode 100644 drivers/net/phy/mediatek/mtk-2p5ge.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 800d23264c94..5541126b4a4c 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -15035,6 +15035,7 @@ M:	Qingfang Deng <dqfext@gmail.com>
 M:	SkyLake Huang <SkyLake.Huang@mediatek.com>
 L:	netdev@vger.kernel.org
 S:	Maintained
+F:	drivers/net/phy/mediatek/mtk-2p5ge.c
 F:	drivers/net/phy/mediatek/mtk-ge-soc.c
 F:	drivers/net/phy/mediatek/mtk-phy-lib.c
 F:	drivers/net/phy/mediatek/mtk-ge.c
diff --git a/drivers/net/phy/mediatek/Kconfig b/drivers/net/phy/mediatek/Kconfig
index 3abf23e37b4b..e440caf3bd04 100644
--- a/drivers/net/phy/mediatek/Kconfig
+++ b/drivers/net/phy/mediatek/Kconfig
@@ -1,4 +1,15 @@
 # SPDX-License-Identifier: GPL-2.0-only
+config MEDIATEK_2P5GE_PHY
+	tristate "MediaTek 2.5Gb Ethernet PHYs"
+	depends on (ARM64 && ARCH_MEDIATEK) || COMPILE_TEST
+	select MTK_NET_PHYLIB
+	help
+	  Supports MediaTek SoC built-in 2.5Gb Ethernet PHYs.
+
+	  This will load necessary firmware and add appropriate time delay.
+	  Accelerate this procedure through internal pbus instead of MDIO
+	  bus. Certain link-up issues will also be fixed here.
+
 config MEDIATEK_GE_SOC_PHY
 	tristate "MediaTek SoC Ethernet PHYs"
 	depends on ARM64 || COMPILE_TEST
diff --git a/drivers/net/phy/mediatek/Makefile b/drivers/net/phy/mediatek/Makefile
index ff13205d614f..f3c534889b30 100644
--- a/drivers/net/phy/mediatek/Makefile
+++ b/drivers/net/phy/mediatek/Makefile
@@ -1,4 +1,5 @@
 # SPDX-License-Identifier: GPL-2.0
+obj-$(CONFIG_MEDIATEK_2P5GE_PHY)	+= mtk-2p5ge.o
 obj-$(CONFIG_MEDIATEK_GE_SOC_PHY)	+= mtk-ge-soc.o
 obj-$(CONFIG_MTK_NET_PHYLIB)		+= mtk-phy-lib.o
 obj-$(CONFIG_MEDIATEK_GE_PHY)		+= mtk-ge.o
diff --git a/drivers/net/phy/mediatek/mtk-2p5ge.c b/drivers/net/phy/mediatek/mtk-2p5ge.c
new file mode 100644
index 000000000000..2386360dd77e
--- /dev/null
+++ b/drivers/net/phy/mediatek/mtk-2p5ge.c
@@ -0,0 +1,322 @@
+// SPDX-License-Identifier: GPL-2.0+
+#include <linux/bitfield.h>
+#include <linux/firmware.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/of_platform.h>
+#include <linux/pinctrl/consumer.h>
+#include <linux/phy.h>
+
+#include "mtk.h"
+
+#define MTK_2P5GPHY_ID_MT7988		(0x00339c11)
+
+#define MT7988_2P5GE_PMB_FW		"mediatek/mt7988/i2p5ge-phy-pmb.bin"
+#define MT7988_2P5GE_PMB_FW_SIZE	(0x20000)
+#define MT7988_2P5GE_PMB_FW_BASE	(0x0f100000)
+#define MT7988_2P5GE_PMB_FW_LEN		(0x20000)
+#define MTK_2P5GPHY_MCU_CSR_BASE	(0x0f0f0000)
+#define MTK_2P5GPHY_MCU_CSR_LEN		(0x20)
+#define MD32_EN_CFG			(0x18)
+#define   MD32_EN			BIT(0)
+
+#define BASE100T_STATUS_EXTEND		(0x10)
+#define BASE1000T_STATUS_EXTEND		(0x11)
+#define EXTEND_CTRL_AND_STATUS		(0x16)
+
+#define PHY_AUX_CTRL_STATUS		(0x1d)
+#define   PHY_AUX_DPX_MASK		GENMASK(5, 5)
+#define   PHY_AUX_SPEED_MASK		GENMASK(4, 2)
+
+/* Registers on MDIO_MMD_VEND1 */
+#define MTK_PHY_LPI_PCS_DSP_CTRL		(0x121)
+#define   MTK_PHY_LPI_SIG_EN_LO_THRESH100_MASK	GENMASK(12, 8)
+
+#define MTK_PHY_HOST_CMD1		0x800e
+#define MTK_PHY_HOST_CMD2		0x800f
+/* Registers on Token Ring debug nodes */
+/* ch_addr = 0x0, node_addr = 0xf, data_addr = 0x3c */
+#define AUTO_NP_10XEN				BIT(6)
+
+enum {
+	PHY_AUX_SPD_10 = 0,
+	PHY_AUX_SPD_100,
+	PHY_AUX_SPD_1000,
+	PHY_AUX_SPD_2500,
+};
+
+static int mt798x_2p5ge_phy_load_fw(struct phy_device *phydev)
+{
+	void __iomem *mcu_csr_base, *pmb_addr;
+	struct device *dev = &phydev->mdio.dev;
+	const struct firmware *fw;
+	int ret, i;
+	u32 reg;
+
+	pmb_addr = ioremap(MT7988_2P5GE_PMB_FW_BASE, MT7988_2P5GE_PMB_FW_LEN);
+	if (!pmb_addr)
+		return -ENOMEM;
+	mcu_csr_base = ioremap(MTK_2P5GPHY_MCU_CSR_BASE,
+			       MTK_2P5GPHY_MCU_CSR_LEN);
+	if (!mcu_csr_base) {
+		ret = -ENOMEM;
+		goto free_pmb;
+	}
+
+	ret = request_firmware_direct(&fw, MT7988_2P5GE_PMB_FW, dev);
+	if (ret) {
+		dev_err(dev, "failed to load firmware: %s, ret: %d\n",
+			MT7988_2P5GE_PMB_FW, ret);
+		goto free;
+	}
+
+	if (fw->size != MT7988_2P5GE_PMB_FW_SIZE) {
+		dev_err(dev, "Firmware size 0x%zx != 0x%x\n",
+			fw->size, MT7988_2P5GE_PMB_FW_SIZE);
+		ret = -EINVAL;
+		goto release_fw;
+	}
+
+	reg = readw(mcu_csr_base + MD32_EN_CFG);
+	if (reg & MD32_EN) {
+		phy_set_bits(phydev, MII_BMCR, BMCR_RESET);
+		usleep_range(10000, 11000);
+	}
+	phy_set_bits(phydev, MII_BMCR, BMCR_PDOWN);
+
+	/* Write magic number to safely stall MCU */
+	phy_write_mmd(phydev, MDIO_MMD_VEND1, MTK_PHY_HOST_CMD1, 0x1100);
+	phy_write_mmd(phydev, MDIO_MMD_VEND1, MTK_PHY_HOST_CMD2, 0x00df);
+
+	for (i = 0; i < MT7988_2P5GE_PMB_FW_SIZE - 1; i += 4)
+		writel(*((uint32_t *)(fw->data + i)), pmb_addr + i);
+
+	writew(reg & ~MD32_EN, mcu_csr_base + MD32_EN_CFG);
+	writew(reg | MD32_EN, mcu_csr_base + MD32_EN_CFG);
+	phy_set_bits(phydev, MII_BMCR, BMCR_RESET);
+	/* We need a delay here to stabilize initialization of MCU */
+	usleep_range(7000, 8000);
+
+	dev_info(dev, "Firmware date code: %x/%x/%x, version: %x.%x\n",
+		 be16_to_cpu(*((__be16 *)(fw->data +
+					  MT7988_2P5GE_PMB_FW_SIZE - 8))),
+		 *(fw->data + MT7988_2P5GE_PMB_FW_SIZE - 6),
+		 *(fw->data + MT7988_2P5GE_PMB_FW_SIZE - 5),
+		 *(fw->data + MT7988_2P5GE_PMB_FW_SIZE - 2),
+		 *(fw->data + MT7988_2P5GE_PMB_FW_SIZE - 1));
+
+release_fw:
+	release_firmware(fw);
+free:
+	iounmap(mcu_csr_base);
+free_pmb:
+	iounmap(pmb_addr);
+
+	return ret;
+}
+
+static int mt798x_2p5ge_phy_config_init(struct phy_device *phydev)
+{
+	struct pinctrl *pinctrl;
+
+	/* Check if PHY interface type is compatible */
+	if (phydev->interface != PHY_INTERFACE_MODE_INTERNAL)
+		return -ENODEV;
+
+	/* Setup LED */
+	phy_set_bits_mmd(phydev, MDIO_MMD_VEND2, MTK_PHY_LED0_ON_CTRL,
+			 MTK_PHY_LED_ON_POLARITY | MTK_PHY_LED_ON_LINK10 |
+			 MTK_PHY_LED_ON_LINK100 | MTK_PHY_LED_ON_LINK1000 |
+			 MTK_PHY_LED_ON_LINK2500);
+	phy_set_bits_mmd(phydev, MDIO_MMD_VEND2, MTK_PHY_LED1_ON_CTRL,
+			 MTK_PHY_LED_ON_FDX | MTK_PHY_LED_ON_HDX);
+
+	/* Switch pinctrl after setting polarity to avoid bogus blinking */
+	pinctrl = devm_pinctrl_get_select(&phydev->mdio.dev, "i2p5gbe-led");
+	if (IS_ERR(pinctrl))
+		dev_err(&phydev->mdio.dev, "Fail to set LED pins!\n");
+
+	phy_modify_mmd(phydev, MDIO_MMD_VEND1, MTK_PHY_LPI_PCS_DSP_CTRL,
+		       MTK_PHY_LPI_SIG_EN_LO_THRESH100_MASK, 0);
+
+	/* Enable 16-bit next page exchange bit if 1000-BT isn't advertising */
+	mtk_tr_modify(phydev, 0x0, 0xf, 0x3c, AUTO_NP_10XEN,
+		      FIELD_PREP(AUTO_NP_10XEN, 0x1));
+
+	/* Enable HW auto downshift */
+	phy_modify_paged(phydev, MTK_PHY_PAGE_EXTENDED_1,
+			 MTK_PHY_AUX_CTRL_AND_STATUS,
+			 0, MTK_PHY_ENABLE_DOWNSHIFT);
+
+	return 0;
+}
+
+static int mt798x_2p5ge_phy_config_aneg(struct phy_device *phydev)
+{
+	bool changed = false;
+	u32 adv;
+	int ret;
+
+	ret = genphy_c45_an_config_aneg(phydev);
+	if (ret < 0)
+		return ret;
+	if (ret > 0)
+		changed = true;
+
+	/* Clause 45 doesn't define 1000BaseT support. Use Clause 22 instead in
+	 * our design.
+	 */
+	adv = linkmode_adv_to_mii_ctrl1000_t(phydev->advertising);
+	ret = phy_modify_changed(phydev, MII_CTRL1000, ADVERTISE_1000FULL, adv);
+	if (ret < 0)
+		return ret;
+	if (ret > 0)
+		changed = true;
+
+	return __genphy_config_aneg(phydev, changed);
+}
+
+static int mt798x_2p5ge_phy_get_features(struct phy_device *phydev)
+{
+	int ret;
+
+	ret = genphy_c45_pma_read_abilities(phydev);
+	if (ret)
+		return ret;
+
+	/* This phy can't handle collision, and neither can (XFI)MAC it's
+	 * connected to. Although it can do HDX handshake, it doesn't support
+	 * CSMA/CD that HDX requires.
+	 */
+	linkmode_clear_bit(ETHTOOL_LINK_MODE_100baseT_Half_BIT,
+			   phydev->supported);
+
+	return 0;
+}
+
+static int mt798x_2p5ge_phy_read_status(struct phy_device *phydev)
+{
+	int ret;
+
+	/* When MDIO_STAT1_LSTATUS is raised genphy_c45_read_link(), this phy
+	 * actually hasn't finished AN. So use CL22's link update function
+	 * instead.
+	 */
+	ret = genphy_update_link(phydev);
+	if (ret)
+		return ret;
+
+	phydev->speed = SPEED_UNKNOWN;
+	phydev->duplex = DUPLEX_UNKNOWN;
+	phydev->pause = 0;
+	phydev->asym_pause = 0;
+
+	/* We'll read link speed through vendor specific registers down below.
+	 * So remove phy_resolve_aneg_linkmode (AN on) & genphy_c45_read_pma
+	 * (AN off).
+	 */
+	if (phydev->autoneg == AUTONEG_ENABLE && phydev->autoneg_complete) {
+		ret = genphy_c45_read_lpa(phydev);
+		if (ret < 0)
+			return ret;
+
+		/* Clause 45 doesn't define 1000BaseT support. Read the link
+		 * partner's 1G advertisement via Clause 22.
+		 */
+		ret = phy_read(phydev, MII_STAT1000);
+		if (ret < 0)
+			return ret;
+		mii_stat1000_mod_linkmode_lpa_t(phydev->lp_advertising, ret);
+	} else if (phydev->autoneg == AUTONEG_DISABLE) {
+		linkmode_zero(phydev->lp_advertising);
+	}
+
+	if (phydev->link) {
+		ret = phy_read(phydev, PHY_AUX_CTRL_STATUS);
+		if (ret < 0)
+			return ret;
+
+		switch (FIELD_GET(PHY_AUX_SPEED_MASK, ret)) {
+		case PHY_AUX_SPD_10:
+			phydev->speed = SPEED_10;
+			break;
+		case PHY_AUX_SPD_100:
+			phydev->speed = SPEED_100;
+			break;
+		case PHY_AUX_SPD_1000:
+			phydev->speed = SPEED_1000;
+			break;
+		case PHY_AUX_SPD_2500:
+			phydev->speed = SPEED_2500;
+			break;
+		}
+
+		phydev->duplex = DUPLEX_FULL;
+		phydev->rate_matching = RATE_MATCH_PAUSE;
+	}
+
+	return 0;
+}
+
+static int mt798x_2p5ge_phy_get_rate_matching(struct phy_device *phydev,
+					      phy_interface_t iface)
+{
+	return RATE_MATCH_PAUSE;
+}
+
+static int mt798x_2p5ge_phy_probe(struct phy_device *phydev)
+{
+	int ret;
+
+	switch (phydev->drv->phy_id) {
+	case MTK_2P5GPHY_ID_MT7988:
+		/* This built-in 2.5GbE hardware only sets MDIO_DEVS_PMAPMD.
+		 * Set the rest by this driver since PCS/AN/VEND1/VEND2 MDIO
+		 * manageable devices actually exist.
+		 */
+		phydev->c45_ids.mmds_present |= MDIO_DEVS_PCS |
+						MDIO_DEVS_AN |
+						MDIO_DEVS_VEND1 |
+						MDIO_DEVS_VEND2;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	ret = mt798x_2p5ge_phy_load_fw(phydev);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
+static struct phy_driver mtk_2p5gephy_driver[] = {
+	{
+		PHY_ID_MATCH_MODEL(MTK_2P5GPHY_ID_MT7988),
+		.name = "MediaTek MT7988 2.5GbE PHY",
+		.probe = mt798x_2p5ge_phy_probe,
+		.config_init = mt798x_2p5ge_phy_config_init,
+		.config_aneg = mt798x_2p5ge_phy_config_aneg,
+		.get_features = mt798x_2p5ge_phy_get_features,
+		.read_status = mt798x_2p5ge_phy_read_status,
+		.get_rate_matching = mt798x_2p5ge_phy_get_rate_matching,
+		.suspend = genphy_suspend,
+		.resume = genphy_resume,
+		.read_page = mtk_phy_read_page,
+		.write_page = mtk_phy_write_page,
+	},
+};
+
+module_phy_driver(mtk_2p5gephy_driver);
+
+static struct mdio_device_id __maybe_unused mtk_2p5ge_phy_tbl[] = {
+	{ PHY_ID_MATCH_VENDOR(0x00339c00) },
+	{ }
+};
+
+MODULE_DESCRIPTION("MediaTek 2.5Gb Ethernet PHY driver");
+MODULE_AUTHOR("SkyLake Huang <SkyLake.Huang@mediatek.com>");
+MODULE_LICENSE("GPL");
+
+MODULE_DEVICE_TABLE(mdio, mtk_2p5ge_phy_tbl);
+MODULE_FIRMWARE(MT7988_2P5GE_PMB_FW);
-- 
2.45.2

Re: [PATCH net-next v3 2/2] net: phy: mediatek: add driver for built-in 2.5G ethernet PHY on MT7988

[Russell King (Oracle)]

Hi,

On Wed, May 14, 2025 at 06:57:38PM +0800, Sky Huang wrote:
> +#define MTK_2P5GPHY_ID_MT7988		(0x00339c11)
> +
> +#define MT7988_2P5GE_PMB_FW		"mediatek/mt7988/i2p5ge-phy-pmb.bin"
> +#define MT7988_2P5GE_PMB_FW_SIZE	(0x20000)
> +#define MT7988_2P5GE_PMB_FW_BASE	(0x0f100000)
> +#define MT7988_2P5GE_PMB_FW_LEN		(0x20000)
> +#define MTK_2P5GPHY_MCU_CSR_BASE	(0x0f0f0000)
> +#define MTK_2P5GPHY_MCU_CSR_LEN		(0x20)
> +#define MD32_EN_CFG			(0x18)

These parens are all unnecessary, as are ones below around a simple
number.

> +#define   MD32_EN			BIT(0)
> +
> +#define BASE100T_STATUS_EXTEND		(0x10)
> +#define BASE1000T_STATUS_EXTEND		(0x11)
> +#define EXTEND_CTRL_AND_STATUS		(0x16)
> +
> +#define PHY_AUX_CTRL_STATUS		(0x1d)
> +#define   PHY_AUX_DPX_MASK		GENMASK(5, 5)
> +#define   PHY_AUX_SPEED_MASK		GENMASK(4, 2)
> +
> +/* Registers on MDIO_MMD_VEND1 */
> +#define MTK_PHY_LPI_PCS_DSP_CTRL		(0x121)

...

> +static int mt798x_2p5ge_phy_load_fw(struct phy_device *phydev)
> +{
> +	void __iomem *mcu_csr_base, *pmb_addr;
> +	struct device *dev = &phydev->mdio.dev;

This will attract a comment about reverse christmas tree.

> +	const struct firmware *fw;
> +	int ret, i;
> +	u32 reg;

...

> +static int mt798x_2p5ge_phy_config_init(struct phy_device *phydev)
> +{
> +	struct pinctrl *pinctrl;
> +
> +	/* Check if PHY interface type is compatible */
> +	if (phydev->interface != PHY_INTERFACE_MODE_INTERNAL)
> +		return -ENODEV;
> +
> +	/* Setup LED */
> +	phy_set_bits_mmd(phydev, MDIO_MMD_VEND2, MTK_PHY_LED0_ON_CTRL,
> +			 MTK_PHY_LED_ON_POLARITY | MTK_PHY_LED_ON_LINK10 |
> +			 MTK_PHY_LED_ON_LINK100 | MTK_PHY_LED_ON_LINK1000 |
> +			 MTK_PHY_LED_ON_LINK2500);
> +	phy_set_bits_mmd(phydev, MDIO_MMD_VEND2, MTK_PHY_LED1_ON_CTRL,
> +			 MTK_PHY_LED_ON_FDX | MTK_PHY_LED_ON_HDX);
> +
> +	/* Switch pinctrl after setting polarity to avoid bogus blinking */
> +	pinctrl = devm_pinctrl_get_select(&phydev->mdio.dev, "i2p5gbe-led");
> +	if (IS_ERR(pinctrl))
> +		dev_err(&phydev->mdio.dev, "Fail to set LED pins!\n");

No, don't do this. config_init() can be called multiple times during
the lifetime of the driver bound to the device, and each time it is,
a new managed-dev structure will be allocated to release this action
each time, thus consuming more and more memory, or possibly failing
after the first depending on the pinctrl_get_select() behaviour.
Please find a different way.

...

> +static int mt798x_2p5ge_phy_config_aneg(struct phy_device *phydev)
> +{
> +	bool changed = false;
> +	u32 adv;
> +	int ret;
> +
> +	ret = genphy_c45_an_config_aneg(phydev);
> +	if (ret < 0)
> +		return ret;
> +	if (ret > 0)
> +		changed = true;
> +
> +	/* Clause 45 doesn't define 1000BaseT support. Use Clause 22 instead in
> +	 * our design.
> +	 */
> +	adv = linkmode_adv_to_mii_ctrl1000_t(phydev->advertising);
> +	ret = phy_modify_changed(phydev, MII_CTRL1000, ADVERTISE_1000FULL, adv);
> +	if (ret < 0)
> +		return ret;
> +	if (ret > 0)
> +		changed = true;
> +
> +	return __genphy_config_aneg(phydev, changed);

Do you want this (which will program EEE and the 10/100 advert) or do
you want genphy_check_and_restart_aneg() here? Note that
genphy_c45_an_config_aneg() will already have programmed both the EEE
and 10/100 adverts via the C45 registers.

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next v3 2/2] net: phy: mediatek: add driver for built-in 2.5G ethernet PHY on MT7988

[SkyLake Huang (黃啟澤)]

On Wed, 2025-05-14 at 13:13 +0100, Russell King (Oracle) wrote:
> 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> Hi,
> 
> On Wed, May 14, 2025 at 06:57:38PM +0800, Sky Huang wrote:
> > +#define MTK_2P5GPHY_ID_MT7988                (0x00339c11)
> > +
> > +#define MT7988_2P5GE_PMB_FW          "mediatek/mt7988/i2p5ge-phy-
> > pmb.bin"
> > +#define MT7988_2P5GE_PMB_FW_SIZE     (0x20000)
> > +#define MT7988_2P5GE_PMB_FW_BASE     (0x0f100000)
> > +#define MT7988_2P5GE_PMB_FW_LEN              (0x20000)
> > +#define MTK_2P5GPHY_MCU_CSR_BASE     (0x0f0f0000)
> > +#define MTK_2P5GPHY_MCU_CSR_LEN              (0x20)
> > +#define MD32_EN_CFG                  (0x18)
> 
> These parens are all unnecessary, as are ones below around a simple
> number.
> 
I'll clean this up in v4.

> > +#define   MD32_EN                    BIT(0)
> > +
> > +#define BASE100T_STATUS_EXTEND               (0x10)
> > +#define BASE1000T_STATUS_EXTEND              (0x11)
> > +#define EXTEND_CTRL_AND_STATUS               (0x16)
> > +
> > +#define PHY_AUX_CTRL_STATUS          (0x1d)
> > +#define   PHY_AUX_DPX_MASK           GENMASK(5, 5)
> > +#define   PHY_AUX_SPEED_MASK         GENMASK(4, 2)
> > +
> > +/* Registers on MDIO_MMD_VEND1 */
> > +#define MTK_PHY_LPI_PCS_DSP_CTRL             (0x121)
> 
> ...
> 
> > +static int mt798x_2p5ge_phy_load_fw(struct phy_device *phydev)
> > +{
> > +     void __iomem *mcu_csr_base, *pmb_addr;
> > +     struct device *dev = &phydev->mdio.dev;
> 
> This will attract a comment about reverse christmas tree.
> 
Thanks. I missed this part. I'll fix it in v4.

> > +     const struct firmware *fw;
> > +     int ret, i;
> > +     u32 reg;
> 
> ...
> 
> > +static int mt798x_2p5ge_phy_config_init(struct phy_device *phydev)
> > +{
> > +     struct pinctrl *pinctrl;
> > +
> > +     /* Check if PHY interface type is compatible */
> > +     if (phydev->interface != PHY_INTERFACE_MODE_INTERNAL)
> > +             return -ENODEV;
> > +
> > +     /* Setup LED */
> > +     phy_set_bits_mmd(phydev, MDIO_MMD_VEND2,
> > MTK_PHY_LED0_ON_CTRL,
> > +                      MTK_PHY_LED_ON_POLARITY |
> > MTK_PHY_LED_ON_LINK10 |
> > +                      MTK_PHY_LED_ON_LINK100 |
> > MTK_PHY_LED_ON_LINK1000 |
> > +                      MTK_PHY_LED_ON_LINK2500);
> > +     phy_set_bits_mmd(phydev, MDIO_MMD_VEND2,
> > MTK_PHY_LED1_ON_CTRL,
> > +                      MTK_PHY_LED_ON_FDX | MTK_PHY_LED_ON_HDX);
> > +
> > +     /* Switch pinctrl after setting polarity to avoid bogus
> > blinking */
> > +     pinctrl = devm_pinctrl_get_select(&phydev->mdio.dev,
> > "i2p5gbe-led");
> > +     if (IS_ERR(pinctrl))
> > +             dev_err(&phydev->mdio.dev, "Fail to set LED
> > pins!\n");
> 
> No, don't do this. config_init() can be called multiple times during
> the lifetime of the driver bound to the device, and each time it is,
> a new managed-dev structure will be allocated to release this action
> each time, thus consuming more and more memory, or possibly failing
> after the first depending on the pinctrl_get_select() behaviour.
> Please find a different way.
> 
I'll move this part (LED initialization) to .probe then.

> ...
> 
> > +static int mt798x_2p5ge_phy_config_aneg(struct phy_device *phydev)
> > +{
> > +     bool changed = false;
> > +     u32 adv;
> > +     int ret;
> > +
> > +     ret = genphy_c45_an_config_aneg(phydev);
> > +     if (ret < 0)
> > +             return ret;
> > +     if (ret > 0)
> > +             changed = true;
> > +
> > +     /* Clause 45 doesn't define 1000BaseT support. Use Clause 22
> > instead in
> > +      * our design.
> > +      */
> > +     adv = linkmode_adv_to_mii_ctrl1000_t(phydev->advertising);
> > +     ret = phy_modify_changed(phydev, MII_CTRL1000,
> > ADVERTISE_1000FULL, adv);
> > +     if (ret < 0)
> > +             return ret;
> > +     if (ret > 0)
> > +             changed = true;
> > +
> > +     return __genphy_config_aneg(phydev, changed);
> 
> Do you want this (which will program EEE and the 10/100 advert) or do
> you want genphy_check_and_restart_aneg() here? Note that
> genphy_c45_an_config_aneg() will already have programmed both the EEE
> and 10/100 adverts via the C45 registers.
> 
> Thanks.
> 
I'll replace "return __genphy_config_aneg(phydev, changed);" with
"return genphy_c45_check_and_restart_aneg(phydev, changed);".

BRs,
Sky

> --
> RMK's Patch system:
> https://urldefense.com/v3/__https://www.armlinux.org.uk/developer/patches/__;!!CTRNKA9wMg0ARbw!iEpNINN4map9ydq6phOk034MHxZo6vhBJJn7U5XFcEMHthdxJrjhgfHUdsH5R3wJAUyyPe_DlFIiuRWE0GE3Uz0$
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!