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

[PATCH net-next v2 0/2] Add 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,
corresponding dt-bindings and dts. It's based from previous commit:
https://patchwork.kernel.org/project/linux-mediatek/patch/20250116012159.
3816135-4-SkyLake.Huang@mediatek.com/

Signed-off-by: Sky Huang <skylake.huang@mediatek.com>
---
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
---
Sky Huang (3):
  net: phy: mediatek: Add 2.5Gphy firmware dt-bindings and dts node
  dts: mt7988a: Add built-in ethernet phy firmware node
  net: phy: mediatek: add driver for built-in 2.5G ethernet PHY on
    MT7988

 .../bindings/net/mediatek,2p5gphy-fw.yaml     |  37 ++
 MAINTAINERS                                   |   4 +-
 arch/arm64/boot/dts/mediatek/mt7988a.dtsi     |   6 +
 drivers/net/phy/mediatek/Kconfig              |  11 +
 drivers/net/phy/mediatek/Makefile             |   1 +
 drivers/net/phy/mediatek/mtk-2p5ge.c          | 346 ++++++++++++++++++
 6 files changed, 404 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/net/mediatek,2p5gphy-fw.yaml
 create mode 100644 drivers/net/phy/mediatek/mtk-2p5ge.c

-- 
2.45.2

[PATCH net-next v2 1/3] net: phy: mediatek: Add 2.5Gphy firmware dt-bindings and dts node

[Sky Huang]

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

Add 2.5Gphy firmware dt-bindings and dts node since mtk-2p5ge
driver requires firmware to run. Also, update MAINTAINERS for
MediaTek's built-in 2.5Gphy dt-bindings and change MAINTAINER's name.

Signed-off-by: Sky Huang <skylake.huang@mediatek.com>
---
 .../bindings/net/mediatek,2p5gphy-fw.yaml     | 37 +++++++++++++++++++
 MAINTAINERS                                   |  3 +-
 2 files changed, 39 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/net/mediatek,2p5gphy-fw.yaml

diff --git a/Documentation/devicetree/bindings/net/mediatek,2p5gphy-fw.yaml b/Documentation/devicetree/bindings/net/mediatek,2p5gphy-fw.yaml
new file mode 100644
index 000000000000..56ebe88b8921
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/mediatek,2p5gphy-fw.yaml
@@ -0,0 +1,37 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/mediatek,2p5gphy-fw.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: MediaTek Built-in 2.5G Ethernet PHY
+
+maintainers:
+  - Sky Huang <SkyLake.Huang@mediatek.com>
+
+description: |
+  MediaTek Built-in 2.5G Ethernet PHY needs to load firmware so it can
+  run correctly.
+
+properties:
+  compatible:
+    const: "mediatek,2p5gphy-fw"
+
+  reg:
+    items:
+      - description: pmb firmware load address
+      - description: firmware trigger register
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    phyfw: phy-firmware@f000000 {
+      compatible = "mediatek,2p5gphy-fw";
+      reg = <0 0x0f100000 0 0x20000>,
+            <0 0x0f0f0018 0 0x20>;
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index de81a3d68396..42ec8b8d03bf 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14716,9 +14716,10 @@ F:	include/linux/pcs/pcs-mtk-lynxi.h
 MEDIATEK ETHERNET PHY DRIVERS
 M:	Daniel Golle <daniel@makrotopia.org>
 M:	Qingfang Deng <dqfext@gmail.com>
-M:	SkyLake Huang <SkyLake.Huang@mediatek.com>
+M:	Sky Huang <SkyLake.Huang@mediatek.com>
 L:	netdev@vger.kernel.org
 S:	Maintained
+F:	Documentation/devicetree/bindings/net/mediatek,2p5gphy-fw.yaml
 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
-- 
2.45.2

[PATCH net-next v2 2/3] dts: mt7988a: Add built-in ethernet phy firmware node

[Sky Huang]

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

Add built-in ethernet phy firmware node in mt7988a.dtsi.

Signed-off-by: Sky Huang <skylake.huang@mediatek.com>
---
 arch/arm64/boot/dts/mediatek/mt7988a.dtsi | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm64/boot/dts/mediatek/mt7988a.dtsi b/arch/arm64/boot/dts/mediatek/mt7988a.dtsi
index 88b56a24efca..f2679702c328 100644
--- a/arch/arm64/boot/dts/mediatek/mt7988a.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt7988a.dtsi
@@ -322,6 +322,12 @@ lvts: lvts@1100a000 {
 			nvmem-cell-names = "lvts-calib-data-1";
 		};
 
+		phyfw: phy-firmware@f000000 {
+			compatible = "mediatek,2p5gphy-fw";
+			reg = <0 0x0f100000 0 0x20000>,
+			      <0 0x0f0f0018 0 0x20>;
+		};
+
 		usb@11190000 {
 			compatible = "mediatek,mt7988-xhci", "mediatek,mtk-xhci";
 			reg = <0 0x11190000 0 0x2e00>,
-- 
2.45.2

[PATCH net-next v2 3/3] 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. Also, certain control registers will
be set to fix link-up issues.

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 | 346 +++++++++++++++++++++++++++
 4 files changed, 359 insertions(+)
 create mode 100644 drivers/net/phy/mediatek/mtk-2p5ge.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 42ec8b8d03bf..250ffe90b056 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14720,6 +14720,7 @@ M:	Sky Huang <SkyLake.Huang@mediatek.com>
 L:	netdev@vger.kernel.org
 S:	Maintained
 F:	Documentation/devicetree/bindings/net/mediatek,2p5gphy-fw.yaml
+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 2a8ac5aed0f8..02d0c215cfe0 100644
--- a/drivers/net/phy/mediatek/Kconfig
+++ b/drivers/net/phy/mediatek/Kconfig
@@ -25,3 +25,14 @@ 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 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.
diff --git a/drivers/net/phy/mediatek/Makefile b/drivers/net/phy/mediatek/Makefile
index 814879d0abe5..c6db8abd2c9c 100644
--- a/drivers/net/phy/mediatek/Makefile
+++ b/drivers/net/phy/mediatek/Makefile
@@ -2,3 +2,4 @@
 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
+obj-$(CONFIG_MEDIATEK_2P5GE_PHY)	+= mtk-2p5ge.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..adb03df331ab
--- /dev/null
+++ b/drivers/net/phy/mediatek/mtk-2p5ge.c
@@ -0,0 +1,346 @@
+// SPDX-License-Identifier: GPL-2.0+
+#include <linux/bitfield.h>
+#include <linux/firmware.h>
+#include <linux/module.h>
+#include <linux/nvmem-consumer.h>
+#include <linux/of_address.h>
+#include <linux/of_platform.h>
+#include <linux/pinctrl/consumer.h>
+#include <linux/phy.h>
+#include <linux/pm_domain.h>
+#include <linux/pm_runtime.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 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)
+
+struct mtk_i2p5ge_phy_priv {
+	bool fw_loaded;
+};
+
+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)
+{
+	struct mtk_i2p5ge_phy_priv *priv = phydev->priv;
+	void __iomem *mcu_csr_base, *pmb_addr;
+	struct device *dev = &phydev->mdio.dev;
+	const struct firmware *fw;
+	struct device_node *np;
+	int ret, i;
+	u32 reg;
+
+	if (priv->fw_loaded)
+		return 0;
+
+	np = of_find_compatible_node(NULL, NULL, "mediatek,2p5gphy-fw");
+	if (!np)
+		return -ENOENT;
+
+	pmb_addr = of_iomap(np, 1);
+	if (!pmb_addr)
+		return -ENOMEM;
+	mcu_csr_base = of_iomap(np, 2);
+	if (!mcu_csr_base) {
+		ret = -ENOMEM;
+		goto free_pmb;
+	}
+
+	ret = request_firmware(&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);
+	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));
+
+	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 loading/trigger ok.\n");
+
+	priv->fw_loaded = true;
+
+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;
+	int ret;
+
+	/* Check if PHY interface type is compatible */
+	if (phydev->interface != PHY_INTERFACE_MODE_INTERNAL)
+		return -ENODEV;
+
+	ret = mt798x_2p5ge_phy_load_fw(phydev);
+	if (ret < 0)
+		return ret;
+
+	/* 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;
+		/* FIXME:
+		 * The current firmware always enables rate adaptation mode.
+		 */
+		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)
+{
+	struct mtk_i2p5ge_phy_priv *priv;
+
+	priv = devm_kzalloc(&phydev->mdio.dev,
+			    sizeof(struct mtk_i2p5ge_phy_priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	switch (phydev->drv->phy_id) {
+	case MTK_2P5GPHY_ID_MT7988:
+		/* The original hardware only sets MDIO_DEVS_PMAPMD */
+		phydev->c45_ids.mmds_present |= MDIO_DEVS_PCS |
+						MDIO_DEVS_AN |
+						MDIO_DEVS_VEND1 |
+						MDIO_DEVS_VEND2;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	priv->fw_loaded = false;
+	phydev->priv = priv;
+
+	mtk_phy_leds_state_init(phydev);
+
+	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 v2 3/3] net: phy: mediatek: add driver for built-in 2.5G ethernet PHY on MT7988

[Russell King (Oracle)]

On Wed, Feb 19, 2025 at 04:39:10PM +0800, Sky Huang wrote:
> +static int mt798x_2p5ge_phy_config_init(struct phy_device *phydev)
> +{
> +	struct pinctrl *pinctrl;
> +	int ret;
> +
> +	/* Check if PHY interface type is compatible */
> +	if (phydev->interface != PHY_INTERFACE_MODE_INTERNAL)
> +		return -ENODEV;
> +
> +	ret = mt798x_2p5ge_phy_load_fw(phydev);
> +	if (ret < 0)
> +		return ret;

Firmware should not be loaded in the .config_init method. The above
call will block while holding the RTNL which will prevent all other
network configuration until the firmware has been loaded or the load
fails.

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 v2 1/3] net: phy: mediatek: Add 2.5Gphy firmware dt-bindings and dts node

[Krzysztof Kozlowski]

On 19/02/2025 09:39, Sky Huang wrote:
> From: Sky Huang <skylake.huang@mediatek.com>
> 
> Add 2.5Gphy firmware dt-bindings and dts node since mtk-2p5ge
> driver requires firmware to run. Also, update MAINTAINERS for
> MediaTek's built-in 2.5Gphy dt-bindings and change MAINTAINER's name.
> 
> Signed-off-by: Sky Huang <skylake.huang@mediatek.com>

Please use subject prefixes matching the subsystem. You can get them for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching. For bindings, the preferred subjects are
explained here:
https://www.kernel.org/doc/html/latest/devicetree/bindings/submitting-patches.html#i-for-patch-submitters

<form letter>
Please use scripts/get_maintainers.pl to get a list of necessary people
and lists to CC. It might happen, that command when run on an older
kernel, gives you outdated entries. Therefore please be sure you base
your patches on recent Linux kernel.

Tools like b4 or scripts/get_maintainer.pl provide you proper list of
people, so fix your workflow. Tools might also fail if you work on some
ancient tree (don't, instead use mainline) or work on fork of kernel
(don't, instead use mainline). Just use b4 and everything should be
fine, although remember about `b4 prep --auto-to-cc` if you added new
patches to the patchset.

You missed at least devicetree list (maybe more), so this won't be
tested by automated tooling. Performing review on untested code might be
a waste of time.

Please kindly resend and include all necessary To/Cc entries.
</form letter>


> ---
>  .../bindings/net/mediatek,2p5gphy-fw.yaml     | 37 +++++++++++++++++++
>  MAINTAINERS                                   |  3 +-
>  2 files changed, 39 insertions(+), 1 deletion(-)
>  create mode 100644 Documentation/devicetree/bindings/net/mediatek,2p5gphy-fw.yaml
> 
> diff --git a/Documentation/devicetree/bindings/net/mediatek,2p5gphy-fw.yaml b/Documentation/devicetree/bindings/net/mediatek,2p5gphy-fw.yaml
> new file mode 100644
> index 000000000000..56ebe88b8921
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/mediatek,2p5gphy-fw.yaml
> @@ -0,0 +1,37 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/net/mediatek,2p5gphy-fw.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: MediaTek Built-in 2.5G Ethernet PHY
> +
> +maintainers:
> +  - Sky Huang <SkyLake.Huang@mediatek.com>
> +
> +description: |
> +  MediaTek Built-in 2.5G Ethernet PHY needs to load firmware so it can
> +  run correctly.
> +
> +properties:
> +  compatible:
> +    const: "mediatek,2p5gphy-fw"


Not tested.

I have doubts that's a real device... Model name looks exactly like 2.5G
phy. "FW" suggests you do it for driver.

Read writing and submitting bindings documents.

Best regards,
Krzysztof

Re: [PATCH net-next v2 2/3] dts: mt7988a: Add built-in ethernet phy firmware node

[Krzysztof Kozlowski]

On 19/02/2025 09:39, Sky Huang wrote:
> From: Sky Huang <skylake.huang@mediatek.com>
> 
> Add built-in ethernet phy firmware node in mt7988a.dtsi.
> 
> Signed-off-by: Sky Huang <skylake.huang@mediatek.com>
> ---
>  arch/arm64/boot/dts/mediatek/mt7988a.dtsi | 6 ++++++

Please use subject prefixes matching the subsystem. You can get them for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching. For bindings, the preferred subjects are
explained here:
https://www.kernel.org/doc/html/latest/devicetree/bindings/submitting-patches.html#i-for-patch-submitters


>  1 file changed, 6 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/mediatek/mt7988a.dtsi b/arch/arm64/boot/dts/mediatek/mt7988a.dtsi
> index 88b56a24efca..f2679702c328 100644
> --- a/arch/arm64/boot/dts/mediatek/mt7988a.dtsi
> +++ b/arch/arm64/boot/dts/mediatek/mt7988a.dtsi
> @@ -322,6 +322,12 @@ lvts: lvts@1100a000 {
>  			nvmem-cell-names = "lvts-calib-data-1";
>  		};
>  
> +		phyfw: phy-firmware@f000000 {

Not a real device.

> +			compatible = "mediatek,2p5gphy-fw";
> +			reg = <0 0x0f100000 0 0x20000>,

Not tested enough. See SoC maintainer profile for clean DTS.

> +			      <0 0x0f0f0018 0 0x20>;
> +		};

Best regards,
Krzysztof

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

[Andrew Lunn]

On Wed, Feb 19, 2025 at 09:33:44AM +0000, Russell King (Oracle) wrote:
> On Wed, Feb 19, 2025 at 04:39:10PM +0800, Sky Huang wrote:
> > +static int mt798x_2p5ge_phy_config_init(struct phy_device *phydev)
> > +{
> > +	struct pinctrl *pinctrl;
> > +	int ret;
> > +
> > +	/* Check if PHY interface type is compatible */
> > +	if (phydev->interface != PHY_INTERFACE_MODE_INTERNAL)
> > +		return -ENODEV;
> > +
> > +	ret = mt798x_2p5ge_phy_load_fw(phydev);
> > +	if (ret < 0)
> > +		return ret;
> 
> Firmware should not be loaded in the .config_init method. The above
> call will block while holding the RTNL which will prevent all other
> network configuration until the firmware has been loaded or the load
> fails.

The aquantia and qt2025 drivers are good examples to copy.

	Andrew

Re: [PATCH net-next v2 1/3] net: phy: mediatek: Add 2.5Gphy firmware dt-bindings and dts node

[Andrew Lunn]

> +description: |
> +  MediaTek Built-in 2.5G Ethernet PHY needs to load firmware so it can
> +  run correctly.
> +
> +properties:
> +  compatible:
> +    const: "mediatek,2p5gphy-fw"
> +
> +  reg:
> +    items:
> +      - description: pmb firmware load address
> +      - description: firmware trigger register
> +
> +required:
> +  - compatible
> +  - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    phyfw: phy-firmware@f000000 {
> +      compatible = "mediatek,2p5gphy-fw";
> +      reg = <0 0x0f100000 0 0x20000>,
> +            <0 0x0f0f0018 0 0x20>;
> +    };

This is not a device in itself is it? There is no driver for this.

It seems like these should be properties in the PHY node, since it is
the PHY driver which make use of them. This cannot be the first SoC
device which is both on some sort of serial bus, but also has memory
mapped registers. Please look around and find the correct way to do
this.

	Andrew

Re: [PATCH net-next v2 1/3] net: phy: mediatek: Add 2.5Gphy firmware dt-bindings and dts node

[Daniel Golle]

On Wed, Feb 19, 2025 at 04:26:21PM +0100, Andrew Lunn wrote:
> > +description: |
> > +  MediaTek Built-in 2.5G Ethernet PHY needs to load firmware so it can
> > +  run correctly.
> > +
> > +properties:
> > +  compatible:
> > +    const: "mediatek,2p5gphy-fw"
> > +
> > +  reg:
> > +    items:
> > +      - description: pmb firmware load address
> > +      - description: firmware trigger register
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    phyfw: phy-firmware@f000000 {
> > +      compatible = "mediatek,2p5gphy-fw";
> > +      reg = <0 0x0f100000 0 0x20000>,
> > +            <0 0x0f0f0018 0 0x20>;
> > +    };
> 
> This is not a device in itself is it? There is no driver for this.
> 
> It seems like these should be properties in the PHY node, since it is
> the PHY driver which make use of them. This cannot be the first SoC
> device which is both on some sort of serial bus, but also has memory
> mapped registers.

I'm afraid it is...

> Please look around and find the correct way to do this.

Would using a 'reserved-memory' region be an option maybe?

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

[Andrew Lunn]

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

Please keep the file sorted, this should be the first entry.

> diff --git a/drivers/net/phy/mediatek/Makefile b/drivers/net/phy/mediatek/Makefile
> index 814879d0abe5..c6db8abd2c9c 100644
> --- a/drivers/net/phy/mediatek/Makefile
> +++ b/drivers/net/phy/mediatek/Makefile
> @@ -2,3 +2,4 @@
>  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
> +obj-$(CONFIG_MEDIATEK_2P5GE_PHY)	+= mtk-2p5ge.o

I suppose you could say this file is sorted in reverse order so is
correct?

> diff --git a/drivers/net/phy/mediatek/mtk-2p5ge.c b/drivers/net/phy/mediatek/mtk-2p5ge.c
> new file mode 100644
> index 000000000000..adb03df331ab
> --- /dev/null
> +++ b/drivers/net/phy/mediatek/mtk-2p5ge.c
> @@ -0,0 +1,346 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +#include <linux/bitfield.h>
> +#include <linux/firmware.h>
> +#include <linux/module.h>
> +#include <linux/nvmem-consumer.h>

Is this header needed?

> +#include <linux/of_address.h>
> +#include <linux/of_platform.h>
> +#include <linux/pinctrl/consumer.h>
> +#include <linux/phy.h>
> +#include <linux/pm_domain.h>
> +#include <linux/pm_runtime.h>

And these two? Please only use those that are needed.

> +static int mt798x_2p5ge_phy_load_fw(struct phy_device *phydev)
> +{
> +
> +	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);

Does the reset bit clear when the MCU is ready? That is what 802.3
defines. phy_poll_reset() might do what you need.

> +	dev_info(dev, "Firmware loading/trigger ok.\n");

dev_dbg(), if at all. You have already spammed the log with the
firmware version, so this adds nothing useful.

> +		phydev->duplex = DUPLEX_FULL;
> +		/* FIXME:
> +		 * The current firmware always enables rate adaptation mode.
> +		 */
> +		phydev->rate_matching = RATE_MATCH_PAUSE;

Can we tell current firmware for future firmware? Is this actually
fixable?

> +	}
> +
> +	return 0;
> +}
> +



> +static int mt798x_2p5ge_phy_probe(struct phy_device *phydev)
> +{
> +	struct mtk_i2p5ge_phy_priv *priv;
> +
> +	priv = devm_kzalloc(&phydev->mdio.dev,
> +			    sizeof(struct mtk_i2p5ge_phy_priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	switch (phydev->drv->phy_id) {
> +	case MTK_2P5GPHY_ID_MT7988:
> +		/* The original hardware only sets MDIO_DEVS_PMAPMD */

What do you mean by "original hardware"? 

You use PHY_ID_MATCH_MODEL(MTK_2P5GPHY_ID_MT7988), so do you mean
revision 0 is broken, but revision 1 fixed it?


> +		phydev->c45_ids.mmds_present |= MDIO_DEVS_PCS |
> +						MDIO_DEVS_AN |
> +						MDIO_DEVS_VEND1 |
> +						MDIO_DEVS_VEND2;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	priv->fw_loaded = false;
> +	phydev->priv = priv;
> +
> +	mtk_phy_leds_state_init(phydev);

The LEDs work without firmware?

    Andrew

---
pw-bot: cr

Re: [PATCH net-next v2 1/3] net: phy: mediatek: Add 2.5Gphy firmware dt-bindings and dts node

[SkyLake Huang (黃啟澤)]

On Wed, 2025-02-19 at 15:30 +0000, Daniel Golle wrote:
> 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> On Wed, Feb 19, 2025 at 04:26:21PM +0100, Andrew Lunn wrote:
> > > +description: |
> > > +  MediaTek Built-in 2.5G Ethernet PHY needs to load firmware so
> > > it can
> > > +  run correctly.
> > > +
> > > +properties:
> > > +  compatible:
> > > +    const: "mediatek,2p5gphy-fw"
> > > +
> > > +  reg:
> > > +    items:
> > > +      - description: pmb firmware load address
> > > +      - description: firmware trigger register
> > > +
> > > +required:
> > > +  - compatible
> > > +  - reg
> > > +
> > > +additionalProperties: false
> > > +
> > > +examples:
> > > +  - |
> > > +    phyfw: phy-firmware@f000000 {
> > > +      compatible = "mediatek,2p5gphy-fw";
> > > +      reg = <0 0x0f100000 0 0x20000>,
> > > +            <0 0x0f0f0018 0 0x20>;
> > > +    };
> > 
> > This is not a device in itself is it? There is no driver for this.
> > 
> > It seems like these should be properties in the PHY node, since it
> > is
> > the PHY driver which make use of them. This cannot be the first SoC
> > device which is both on some sort of serial bus, but also has
> > memory
> > mapped registers.
> 
> I'm afraid it is...
> 
It's actually MCU's memory mapped registers. This MCU will control all
of this built-in the 2.5Gphy's behaviors and it provides faster bus
access speed than MDC/MDIO.

> > Please look around and find the correct way to do this.
> 
> Would using a 'reserved-memory' region be an option maybe?
Or maybe just leave those mapped registers' addresses in driver code
(mtk-2p5ge.c)? Like:
#define MT7988_2P5GE_PMB_BASE (0x0f100000)
#define MT7988_2P5GE_PMB_LEN  (0x20000)

I'm not sure which is more Linux upstream style.

BRs,
Sky

Re: [PATCH net-next v2 1/3] net: phy: mediatek: Add 2.5Gphy firmware dt-bindings and dts node

[Andrew Lunn]

> > Would using a 'reserved-memory' region be an option maybe?
> Or maybe just leave those mapped registers' addresses in driver code
> (mtk-2p5ge.c)? Like:
> #define MT7988_2P5GE_PMB_BASE (0x0f100000)
> #define MT7988_2P5GE_PMB_LEN  (0x20000)

The problem with hard coding them is you need some way to know which
set of hard coded values to use, because the hardware engineers will
not guarantee to never move them, or change the bit layout for the
next generation of devices.

PHYs don't use compatibles because they have an ID in register 2 and
3. Is this ID specific to the MT7988?

	Andrew

Re: [PATCH net-next v2 1/3] net: phy: mediatek: Add 2.5Gphy firmware dt-bindings and dts node

[SkyLake Huang (黃啟澤)]

On Tue, 2025-02-25 at 14:51 +0100, Andrew Lunn wrote:
> 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> > > Would using a 'reserved-memory' region be an option maybe?
> > Or maybe just leave those mapped registers' addresses in driver
> > code
> > (mtk-2p5ge.c)? Like:
> > #define MT7988_2P5GE_PMB_BASE (0x0f100000)
> > #define MT7988_2P5GE_PMB_LEN  (0x20000)
> 
> The problem with hard coding them is you need some way to know which
> set of hard coded values to use, because the hardware engineers will
> not guarantee to never move them, or change the bit layout for the
> next generation of devices.
> 
> PHYs don't use compatibles because they have an ID in register 2 and
> 3. Is this ID specific to the MT7988?
> 
>         Andrew
Do you mean that "are MT7988_2P5GE_PMB_BASE & MT7988_2P5GE_PMB_LEN
specific to MT7988"? There'a another our new platform, MT7987, has
almost the same built-in 2.5Gphy. It will use the same "PMB" base
address to load firmware.

So I guess I can do the following according to the previous discussion:
1) Reserve a memory region in mt7988.dtsi
reserved-memory {
	#address-cells = <2>;
	#size-celss = <2>;
	ranges;

	/* 0x0f0100000~0x0f1f0024 are specific for built-in 2.5Gphy.
	 * In this range, it includes "PMB_FW_BASE"(0x0f100000)
	 * and "MCU_CSR_BASE"(0x0f0f0000)
	 */
	i2p5g: i2p5g@0f100000 {
		reg = <0 0x0f010000 0 0x1e0024>;
		no-map;
	};
};

Reserve a memory region in mt7987.dtsi
reserved-memory {
	#address-cells = <2>;
	#size-celss = <2>;
	ranges;

	i2p5g: i2p5g@0f100000 {
		reg = <0 0x0f010000 0 0x1e0024>;
		no-map;
	};

	/* For built-in 2.5Gphy's top reset */
	i2p5g_apb: i2p5g_apb@11c30000 {
		reg = <0 0x11c30000 0 0x10c>;
		no-map;
	};
};

2) Since PHYs don't use compatibles, hardcode address in mtk-2p5ge.c:
/* MTK_ prefix means that the macro is used for both MT7988 & MT7987*/
#define MTK_2P5GPHY_PMB_FW_BASE		(0x0f100000)
#define MT7988_2P5GE_PMB_FW_LEN		(0x20000)
#define MT7987_2P5GE_PMB_FW_LEN		(0x18000)
#define MTK_2P5GPHY_MCU_CSR_BASE	(0x0f0f0000)
#define MTK_2P5GPHY_MCU_CSR_LEN		(0x20)

/* On MT7987, we separate firmware bin to 2 files and total size
 * is decreased from 128KB(mediatek/mt7988/i2p5ge-phy-pmb.bin) to
 * 96KB(mediatek/mt7987/i2p5ge-phy-pmb.bin)+
 * 28KB(mediatek/mt7987/i2p5ge-phy-DSPBitTb.bin)
 * And i2p5ge-phy-DSPBitTb.bin will be loaded to
 * MT7987_2P5GE_XBZ_PMA_RX_BASE
 */
#define MT7987_2P5GE_XBZ_PMA_RX_BASE	(0x0f080000)
#define MT7987_2P5GE_XBZ_PMA_RX_LEN	(0x5228)
#define MT7987_2P5GE_DSPBITTB_SIZE	(0x7000)

/* MT7987 requires these base addresses to manipulate some
 * registers before loading firmware.
 */
#define MT7987_2P5GE_APB_BASE		(0x11c30000)
#define MT7987_2P5GE_APB_LEN		(0x9c)
#define MT7987_2P5GE_PMD_REG_BASE	(0x0f010000)
#define MT7987_2P5GE_PMD_REG_LEN	(0x210)
#define MT7987_2P5GE_XBZ_PCS_REG_BASE	(0x0f030000)
#define MT7987_2P5GE_XBZ_PCS_REG_LEN	(0x844)
#define MT7987_2P5GE_CHIP_SCU_BASE	(0x0f0cf800)
#define MT7987_2P5GE_CHIP_SCU_LEN	(0x12c)

static int mt7988_2p5ge_phy_load_fw(struct phy_device *phydev)
{
	struct mtk_i2p5ge_phy_priv *priv = phydev->priv;
	void __iomem *mcu_csr_base, *pmb_addr;
	struct device *dev = &phydev->mdio.dev;
	const struct firmware *fw;
	int ret, i;
	u32 reg;

	if (priv->fw_loaded)
		return 0;

	pmb_addr = ioremap(MTK_2P5GPHY_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;
	}
...
free:
	iounmap(mcu_csr_base);
free_pmb:
	iounmap(pmb_addr);
...
}

BRs,
Sky

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

[SkyLake Huang (黃啟澤)]

On Wed, 2025-02-19 at 09:33 +0000, Russell King (Oracle) wrote:
> 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> On Wed, Feb 19, 2025 at 04:39:10PM +0800, Sky Huang wrote:
> > +static int mt798x_2p5ge_phy_config_init(struct phy_device *phydev)
> > +{
> > +     struct pinctrl *pinctrl;
> > +     int ret;
> > +
> > +     /* Check if PHY interface type is compatible */
> > +     if (phydev->interface != PHY_INTERFACE_MODE_INTERNAL)
> > +             return -ENODEV;
> > +
> > +     ret = mt798x_2p5ge_phy_load_fw(phydev);
> > +     if (ret < 0)
> > +             return ret;
> 
> Firmware should not be loaded in the .config_init method. The above
> call will block while holding the RTNL which will prevent all other
> network configuration until the firmware has been loaded or the load
> fails.
> 
> Thanks.
> 
> --
> RMK's Patch system:
> https://urldefense.com/v3/__https://www.armlinux.org.uk/developer/patches/__;!!CTRNKA9wMg0ARbw!iV-1ViPFsUV-lLj7aIycan8nery6sQO3t6mkpdlb_GW8hswhxc4ejJozxqkU3s2WzxSizs4kfdC77yr7HGGRIuU$
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
Hi Russell,
mt798x_p5ge_phy_load_fw() will only load firmware once after driver is
probed through priv->fw_loaded. And actually, firmware loading
procedure only takes about 11ms. This was discussed earlier in:
https://patchwork.kernel.org/project/linux-mediatek/patch/20240520113456.21675-6-SkyLake.Huang@mediatek.com/#25856462
https://patchwork.kernel.org/project/linux-mediatek/patch/20240520113456.21675-6-SkyLake.Huang@mediatek.com/#25857174

BRs,
Sky

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

[Russell King (Oracle)]

On Wed, Feb 26, 2025 at 06:48:34AM +0000, SkyLake Huang (黃啟澤) wrote:
> On Wed, 2025-02-19 at 09:33 +0000, Russell King (Oracle) wrote:
> > 
> > External email : Please do not click links or open attachments until
> > you have verified the sender or the content.
> > 
> > 
> > On Wed, Feb 19, 2025 at 04:39:10PM +0800, Sky Huang wrote:
> > > +static int mt798x_2p5ge_phy_config_init(struct phy_device *phydev)
> > > +{
> > > +     struct pinctrl *pinctrl;
> > > +     int ret;
> > > +
> > > +     /* Check if PHY interface type is compatible */
> > > +     if (phydev->interface != PHY_INTERFACE_MODE_INTERNAL)
> > > +             return -ENODEV;
> > > +
> > > +     ret = mt798x_2p5ge_phy_load_fw(phydev);
> > > +     if (ret < 0)
> > > +             return ret;
> > 
> > Firmware should not be loaded in the .config_init method. The above
> > call will block while holding the RTNL which will prevent all other
> > network configuration until the firmware has been loaded or the load
> > fails.
> > 
> > Thanks.
> > 
> > --
> > RMK's Patch system:
> > https://urldefense.com/v3/__https://www.armlinux.org.uk/developer/patches/__;!!CTRNKA9wMg0ARbw!iV-1ViPFsUV-lLj7aIycan8nery6sQO3t6mkpdlb_GW8hswhxc4ejJozxqkU3s2WzxSizs4kfdC77yr7HGGRIuU$
> > FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
> Hi Russell,
> mt798x_p5ge_phy_load_fw() will only load firmware once after driver is
> probed through priv->fw_loaded. And actually, firmware loading
> procedure only takes about 11ms. This was discussed earlier in:
> https://patchwork.kernel.org/project/linux-mediatek/patch/20240520113456.21675-6-SkyLake.Huang@mediatek.com/#25856462
> https://patchwork.kernel.org/project/linux-mediatek/patch/20240520113456.21675-6-SkyLake.Huang@mediatek.com/#25857174

1. Wouldn't it be a good idea to include the loading time in the patch
   description or a comment in the patch?

2. What about the time it takes for request_firmware() uses the sysfs
   fallback, which essentially passes the firmware request to userspace
   to deal with? That can block for an indeterminate amount of time.

-- 
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 v2 1/3] net: phy: mediatek: Add 2.5Gphy firmware dt-bindings and dts node

[Andrew Lunn]

> So I guess I can do the following according to the previous discussion:
> 1) Reserve a memory region in mt7988.dtsi
> reserved-memory {
> 	#address-cells = <2>;
> 	#size-celss = <2>;
> 	ranges;
> 
> 	/* 0x0f0100000~0x0f1f0024 are specific for built-in 2.5Gphy.
> 	 * In this range, it includes "PMB_FW_BASE"(0x0f100000)
> 	 * and "MCU_CSR_BASE"(0x0f0f0000)
> 	 */
> 	i2p5g: i2p5g@0f100000 {
> 		reg = <0 0x0f010000 0 0x1e0024>;
> 		no-map;
> 	};
> };

Do you even need these? I assume this is in the IO space, not DRAM. So
the kernel is not going to use it by default. That is why you need to
specifically ioremap() it.

> 2) Since PHYs don't use compatibles, hardcode address in mtk-2p5ge.c:
> /* MTK_ prefix means that the macro is used for both MT7988 & MT7987*/
> #define MTK_2P5GPHY_PMB_FW_BASE		(0x0f100000)
> #define MT7988_2P5GE_PMB_FW_LEN		(0x20000)
> #define MT7987_2P5GE_PMB_FW_LEN		(0x18000)
> #define MTK_2P5GPHY_MCU_CSR_BASE	(0x0f0f0000)
> #define MTK_2P5GPHY_MCU_CSR_LEN		(0x20)
> 
> /* On MT7987, we separate firmware bin to 2 files and total size
>  * is decreased from 128KB(mediatek/mt7988/i2p5ge-phy-pmb.bin) to
>  * 96KB(mediatek/mt7987/i2p5ge-phy-pmb.bin)+
>  * 28KB(mediatek/mt7987/i2p5ge-phy-DSPBitTb.bin)
>  * And i2p5ge-phy-DSPBitTb.bin will be loaded to
>  * MT7987_2P5GE_XBZ_PMA_RX_BASE
>  */
> #define MT7987_2P5GE_XBZ_PMA_RX_BASE	(0x0f080000)
> #define MT7987_2P5GE_XBZ_PMA_RX_LEN	(0x5228)
> #define MT7987_2P5GE_DSPBITTB_SIZE	(0x7000)
> 
> /* MT7987 requires these base addresses to manipulate some
>  * registers before loading firmware.
>  */
> #define MT7987_2P5GE_APB_BASE		(0x11c30000)
> #define MT7987_2P5GE_APB_LEN		(0x9c)
> #define MT7987_2P5GE_PMD_REG_BASE	(0x0f010000)
> #define MT7987_2P5GE_PMD_REG_LEN	(0x210)
> #define MT7987_2P5GE_XBZ_PCS_REG_BASE	(0x0f030000)
> #define MT7987_2P5GE_XBZ_PCS_REG_LEN	(0x844)

Should the PCS registers actually be in the PCS driver, not the PHY
driver? Hard to say until we know what these registers actually are.

> #define MT7987_2P5GE_CHIP_SCU_BASE	(0x0f0cf800)
> #define MT7987_2P5GE_CHIP_SCU_LEN	(0x12c)
> 
> static int mt7988_2p5ge_phy_load_fw(struct phy_device *phydev)
> {
> 	struct mtk_i2p5ge_phy_priv *priv = phydev->priv;
> 	void __iomem *mcu_csr_base, *pmb_addr;
> 	struct device *dev = &phydev->mdio.dev;
> 	const struct firmware *fw;
> 	int ret, i;
> 	u32 reg;
> 
> 	if (priv->fw_loaded)
> 		return 0;
> 
> 	pmb_addr = ioremap(MTK_2P5GPHY_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;
> 	}
> ...
> free:
> 	iounmap(mcu_csr_base);
> free_pmb:
> 	iounmap(pmb_addr);
> ...
> }

This looks O.K. It is basically what we did before device tree was
used.

	Andrew

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

[Andrew Lunn]

On Wed, Feb 26, 2025 at 06:48:34AM +0000, SkyLake Huang (黃啟澤) wrote:
> On Wed, 2025-02-19 at 09:33 +0000, Russell King (Oracle) wrote:
> > 
> > External email : Please do not click links or open attachments until
> > you have verified the sender or the content.
> > 
> > 
> > On Wed, Feb 19, 2025 at 04:39:10PM +0800, Sky Huang wrote:
> > > +static int mt798x_2p5ge_phy_config_init(struct phy_device *phydev)
> > > +{
> > > +     struct pinctrl *pinctrl;
> > > +     int ret;
> > > +
> > > +     /* Check if PHY interface type is compatible */
> > > +     if (phydev->interface != PHY_INTERFACE_MODE_INTERNAL)
> > > +             return -ENODEV;
> > > +
> > > +     ret = mt798x_2p5ge_phy_load_fw(phydev);
> > > +     if (ret < 0)
> > > +             return ret;
> > 
> > Firmware should not be loaded in the .config_init method. The above
> > call will block while holding the RTNL which will prevent all other
> > network configuration until the firmware has been loaded or the load
> > fails.
> > 
> > Thanks.
> > 
> > --
> > RMK's Patch system:
> > https://urldefense.com/v3/__https://www.armlinux.org.uk/developer/patches/__;!!CTRNKA9wMg0ARbw!iV-1ViPFsUV-lLj7aIycan8nery6sQO3t6mkpdlb_GW8hswhxc4ejJozxqkU3s2WzxSizs4kfdC77yr7HGGRIuU$
> > FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
> Hi Russell,
> mt798x_p5ge_phy_load_fw() will only load firmware once after driver is
> probed through priv->fw_loaded. And actually, firmware loading
> procedure only takes about 11ms. This was discussed earlier in:
> https://patchwork.kernel.org/project/linux-mediatek/patch/20240520113456.21675-6-SkyLake.Huang@mediatek.com/#25856462
> https://patchwork.kernel.org/project/linux-mediatek/patch/20240520113456.21675-6-SkyLake.Huang@mediatek.com/#25857174

Ideally, all PHY drivers should look like each other. That makes
maintenance simpler. Currently, air_en8811h.c, aquantia_main.c, and
qt2025.rs load firmware in there probe function. Is there a good
reason this driver needs to be different?

       Andrew

Re: [PATCH net-next v2 1/3] net: phy: mediatek: Add 2.5Gphy firmware dt-bindings and dts node

[SkyLake Huang (黃啟澤)]

On Wed, 2025-02-26 at 14:26 +0100, Andrew Lunn wrote:
> 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> > So I guess I can do the following according to the previous
> > discussion:
> > 1) Reserve a memory region in mt7988.dtsi
> > reserved-memory {
> >       #address-cells = <2>;
> >       #size-celss = <2>;
> >       ranges;
> > 
> >       /* 0x0f0100000~0x0f1f0024 are specific for built-in 2.5Gphy.
> >        * In this range, it includes "PMB_FW_BASE"(0x0f100000)
> >        * and "MCU_CSR_BASE"(0x0f0f0000)
> >        */
> >       i2p5g: i2p5g@0f100000 {
> >               reg = <0 0x0f010000 0 0x1e0024>;
> >               no-map;
> >       };
> > };
> 
> Do you even need these? I assume this is in the IO space, not DRAM.
> So
> the kernel is not going to use it by default. That is why you need to
> specifically ioremap() it.

Agree. I'll remove this.

> 
> > 2) Since PHYs don't use compatibles, hardcode address in mtk-
> > 2p5ge.c:
> > /* MTK_ prefix means that the macro is used for both MT7988 &
> > MT7987*/
> > #define MTK_2P5GPHY_PMB_FW_BASE               (0x0f100000)
> > #define MT7988_2P5GE_PMB_FW_LEN               (0x20000)
> > #define MT7987_2P5GE_PMB_FW_LEN               (0x18000)
> > #define MTK_2P5GPHY_MCU_CSR_BASE      (0x0f0f0000)
> > #define MTK_2P5GPHY_MCU_CSR_LEN               (0x20)
> > 
> > /* On MT7987, we separate firmware bin to 2 files and total size
> >  * is decreased from 128KB(mediatek/mt7988/i2p5ge-phy-pmb.bin) to
> >  * 96KB(mediatek/mt7987/i2p5ge-phy-pmb.bin)+
> >  * 28KB(mediatek/mt7987/i2p5ge-phy-DSPBitTb.bin)
> >  * And i2p5ge-phy-DSPBitTb.bin will be loaded to
> >  * MT7987_2P5GE_XBZ_PMA_RX_BASE
> >  */
> > #define MT7987_2P5GE_XBZ_PMA_RX_BASE  (0x0f080000)
> > #define MT7987_2P5GE_XBZ_PMA_RX_LEN   (0x5228)
> > #define MT7987_2P5GE_DSPBITTB_SIZE    (0x7000)
> > 
> > /* MT7987 requires these base addresses to manipulate some
> >  * registers before loading firmware.
> >  */
> > #define MT7987_2P5GE_APB_BASE         (0x11c30000)
> > #define MT7987_2P5GE_APB_LEN          (0x9c)
> > #define MT7987_2P5GE_PMD_REG_BASE     (0x0f010000)
> > #define MT7987_2P5GE_PMD_REG_LEN      (0x210)
> > #define MT7987_2P5GE_XBZ_PCS_REG_BASE (0x0f030000)
> > #define MT7987_2P5GE_XBZ_PCS_REG_LEN  (0x844)
> 
> Should the PCS registers actually be in the PCS driver, not the PHY
> driver? Hard to say until we know what these registers actually are.
> 
These PCS registers are in different domain with USXGMII's PCS on
MT7988. These PCS registers are only used by built-in 2.5Gphy when
loading firmware. I'll submit MT7987's built-in 2.5Gphy driver later
and we can check if another PCS driver is needed or not.

> > #define MT7987_2P5GE_CHIP_SCU_BASE    (0x0f0cf800)
> > #define MT7987_2P5GE_CHIP_SCU_LEN     (0x12c)
> > 
> > static int mt7988_2p5ge_phy_load_fw(struct phy_device *phydev)
> > {
> >       struct mtk_i2p5ge_phy_priv *priv = phydev->priv;
> >       void __iomem *mcu_csr_base, *pmb_addr;
> >       struct device *dev = &phydev->mdio.dev;
> >       const struct firmware *fw;
> >       int ret, i;
> >       u32 reg;
> > 
> >       if (priv->fw_loaded)
> >               return 0;
> > 
> >       pmb_addr = ioremap(MTK_2P5GPHY_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;
> >       }
> > ...
> > free:
> >       iounmap(mcu_csr_base);
> > free_pmb:
> >       iounmap(pmb_addr);
> > ...
> > }
> 
> This looks O.K. It is basically what we did before device tree was
> used.
> 
>         Andrew

OK. I'll submit v3 in this way.

BRs,
Sky

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

[SkyLake Huang (黃啟澤)]

On Wed, 2025-02-19 at 09:33 +0000, Russell King (Oracle) wrote:
> 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> On Wed, Feb 19, 2025 at 04:39:10PM +0800, Sky Huang wrote:
> > +static int mt798x_2p5ge_phy_config_init(struct phy_device *phydev)
> > +{
> > +     struct pinctrl *pinctrl;
> > +     int ret;
> > +
> > +     /* Check if PHY interface type is compatible */
> > +     if (phydev->interface != PHY_INTERFACE_MODE_INTERNAL)
> > +             return -ENODEV;
> > +
> > +     ret = mt798x_2p5ge_phy_load_fw(phydev);
> > +     if (ret < 0)
> > +             return ret;
> 
> Firmware should not be loaded in the .config_init method. The above
> call will block while holding the RTNL which will prevent all other
> network configuration until the firmware has been loaded or the load
> fails.
> 
> Thanks.
> 
> --
> RMK's Patch system:
> https://urldefense.com/v3/__https://www.armlinux.org.uk/developer/patches/__;!!CTRNKA9wMg0ARbw!iV-1ViPFsUV-lLj7aIycan8nery6sQO3t6mkpdlb_GW8hswhxc4ejJozxqkU3s2WzxSizs4kfdC77yr7HGGRIuU$
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Actually, I wrote fw loading flow in .probe. However, during boot time,
.probe is called at very early stage (about the first 2s after booting
into Linux Kernel). At that time, filesystem isn't ready yet and phy
driver can't locate /lib/firmware/mediatek/mt7988/i2p5ge-phy-pmb.bin.
Adding "-EPROBE_DEFER" doesn't help at this moment. I'm not sure how
aquantia and qt2025 drivers handle this.

But anyway, mt7988's built-in phy driver loads firmware in only a few
ms. This is via internal bus instead of MDIO bus, so loading speed is
much faster. Will this have any impact, if that's the case?

BRs,
Sky

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

[SkyLake Huang (黃啟澤)]

On Wed, 2025-02-19 at 16:47 +0100, Andrew Lunn wrote:
> 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> > +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.
> 
> Please keep the file sorted, this should be the first entry.
> 
I'll sort in this way in v3:
config MEDIATEK_2P5GE_PHY
...
config MEDIATEK_GE_SOC_PHY
...
config MTK_NET_PHYLIB
...
config MEDIATEK_GE_PHY
...

> > diff --git a/drivers/net/phy/mediatek/Makefile
> > b/drivers/net/phy/mediatek/Makefile
> > index 814879d0abe5..c6db8abd2c9c 100644
> > --- a/drivers/net/phy/mediatek/Makefile
> > +++ b/drivers/net/phy/mediatek/Makefile
> > @@ -2,3 +2,4 @@
> >  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
> > +obj-$(CONFIG_MEDIATEK_2P5GE_PHY)     += mtk-2p5ge.o
> 
> I suppose you could say this file is sorted in reverse order so is
> correct?
> 
I'll change this in v3 in this way:
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..adb03df331ab
> > --- /dev/null
> > +++ b/drivers/net/phy/mediatek/mtk-2p5ge.c
> > @@ -0,0 +1,346 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +#include <linux/bitfield.h>
> > +#include <linux/firmware.h>
> > +#include <linux/module.h>
> > +#include <linux/nvmem-consumer.h>
> 
> Is this header needed?
> 
> > +#include <linux/of_address.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/pinctrl/consumer.h>
> > +#include <linux/phy.h>
> > +#include <linux/pm_domain.h>
> > +#include <linux/pm_runtime.h>
> 
> And these two? Please only use those that are needed.
> 
I'll remove linux/nvmem-
consumer.h>/<linux/pm_domain.h>/<linux/pm_runtime.h> in v3. I forgot
removing them after developing this driver.

> > +static int mt798x_2p5ge_phy_load_fw(struct phy_device *phydev)
> > +{
> > +
> > +     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);
> 
> Does the reset bit clear when the MCU is ready? That is what 802.3
> defines. phy_poll_reset() might do what you need.

CL22 reset bit will be cleared right after I set it on MT7988.
usleep_range() here is used to allow the MCU to stabilize, rather than
to wait for the reset to complete.
> 
> > +     dev_info(dev, "Firmware loading/trigger ok.\n");
> 
> dev_dbg(), if at all. You have already spammed the log with the
> firmware version, so this adds nothing useful.

Well then... In v3, I'll remove this line and print firmware version at
last like this:

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));

priv->fw_loaded = true;

> > +             phydev->duplex = DUPLEX_FULL;
> > +             /* FIXME:
> > +              * The current firmware always enables rate
> > adaptation mode.
> > +              */
> > +             phydev->rate_matching = RATE_MATCH_PAUSE;
> 
> Can we tell current firmware for future firmware? Is this actually
> fixable?
> 
We decided to always enable rate adaptation mode for MT7988's current
and future built-in 2.5GbE firmware. I'll remove FIXME comment here in
v3.

> > +     }
> > +
> > +     return 0;
> > +}
> > +
> 
> 
> 
> > +static int mt798x_2p5ge_phy_probe(struct phy_device *phydev)
> > +{
> > +     struct mtk_i2p5ge_phy_priv *priv;
> > +
> > +     priv = devm_kzalloc(&phydev->mdio.dev,
> > +                         sizeof(struct mtk_i2p5ge_phy_priv),
> > GFP_KERNEL);
> > +     if (!priv)
> > +             return -ENOMEM;
> > +
> > +     switch (phydev->drv->phy_id) {
> > +     case MTK_2P5GPHY_ID_MT7988:
> > +             /* The original hardware only sets MDIO_DEVS_PMAPMD
> > */
> 
> What do you mean by "original hardware"?
> 
> You use PHY_ID_MATCH_MODEL(MTK_2P5GPHY_ID_MT7988), so do you mean
> revision 0 is broken, but revision 1 fixed it?
> 
I'll fix this ambiguous comment to:
/* This built-in 2.5GbE hardware only sets MDIO_DEVS_PMAPMD. Set the  *
rest by this driver since PCS/AN/VEND1/VEND2 mmds exist.
 */

> 
> > +             phydev->c45_ids.mmds_present |= MDIO_DEVS_PCS |
> > +                                             MDIO_DEVS_AN |
> > +                                             MDIO_DEVS_VEND1 |
> > +                                             MDIO_DEVS_VEND2;
> > +             break;
> > +     default:
> > +             return -EINVAL;
> > +     }
> > +
> > +     priv->fw_loaded = false;
> > +     phydev->priv = priv;
> > +
> > +     mtk_phy_leds_state_init(phydev);
> 
> The LEDs work without firmware?
> 
>     Andrew
> 
> ---
> pw-bot: cr
I'll remove this line v3 and propose LED part in later patchset.

BRs,
Sky

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

[SkyLake Huang (黃啟澤)]

On Tue, 2025-05-13 at 18:12 +0800, SkyLake.Huang wrote:
> On Wed, 2025-02-19 at 09:33 +0000, Russell King (Oracle) wrote:
> > 
> > External email : Please do not click links or open attachments
> > until
> > you have verified the sender or the content.
> > 
> > 
> > On Wed, Feb 19, 2025 at 04:39:10PM +0800, Sky Huang wrote:
> > > +static int mt798x_2p5ge_phy_config_init(struct phy_device
> > > *phydev)
> > > +{
> > > +     struct pinctrl *pinctrl;
> > > +     int ret;
> > > +
> > > +     /* Check if PHY interface type is compatible */
> > > +     if (phydev->interface != PHY_INTERFACE_MODE_INTERNAL)
> > > +             return -ENODEV;
> > > +
> > > +     ret = mt798x_2p5ge_phy_load_fw(phydev);
> > > +     if (ret < 0)
> > > +             return ret;
> > 
> > Firmware should not be loaded in the .config_init method. The above
> > call will block while holding the RTNL which will prevent all other
> > network configuration until the firmware has been loaded or the
> > load
> > fails.
> > 
> > Thanks.
> > 
> > --
> > RMK's Patch system:
> > https://urldefense.com/v3/__https://www.armlinux.org.uk/developer/patches/__;!!CTRNKA9wMg0ARbw!iV-1ViPFsUV-lLj7aIycan8nery6sQO3t6mkpdlb_GW8hswhxc4ejJozxqkU3s2WzxSizs4kfdC77yr7HGGRIuU$
> > FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
> 
> Actually, I wrote fw loading flow in .probe. However, during boot
> time,
> .probe is called at very early stage (about the first 2s after
> booting
> into Linux Kernel). At that time, filesystem isn't ready yet and phy
> driver can't locate /lib/firmware/mediatek/mt7988/i2p5ge-phy-pmb.bin.
> Adding "-EPROBE_DEFER" doesn't help at this moment. I'm not sure how
> aquantia and qt2025 drivers handle this.
> 
> But anyway, mt7988's built-in phy driver loads firmware in only a few
> ms. This is via internal bus instead of MDIO bus, so loading speed is
> much faster. Will this have any impact, if that's the case?
> 
> BRs,
> Sky

This message is replied in wrong place. Please ignore, thanks.

BRs,
Sky

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

[SkyLake Huang (黃啟澤)]

On Wed, 2025-02-26 at 09:52 +0000, Russell King (Oracle) wrote:
> 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> On Wed, Feb 26, 2025 at 06:48:34AM +0000, SkyLake Huang (黃啟澤) wrote:
> > On Wed, 2025-02-19 at 09:33 +0000, Russell King (Oracle) wrote:
> > > 
> > > External email : Please do not click links or open attachments
> > > until
> > > you have verified the sender or the content.
> > > 
> > > 
> > > On Wed, Feb 19, 2025 at 04:39:10PM +0800, Sky Huang wrote:
> > > > +static int mt798x_2p5ge_phy_config_init(struct phy_device
> > > > *phydev)
> > > > +{
> > > > +     struct pinctrl *pinctrl;
> > > > +     int ret;
> > > > +
> > > > +     /* Check if PHY interface type is compatible */
> > > > +     if (phydev->interface != PHY_INTERFACE_MODE_INTERNAL)
> > > > +             return -ENODEV;
> > > > +
> > > > +     ret = mt798x_2p5ge_phy_load_fw(phydev);
> > > > +     if (ret < 0)
> > > > +             return ret;
> > > 
> > > Firmware should not be loaded in the .config_init method. The
> > > above
> > > call will block while holding the RTNL which will prevent all
> > > other
> > > network configuration until the firmware has been loaded or the
> > > load
> > > fails.
> > > 
> > > Thanks.
> > > 
> > > --
> > > RMK's Patch system:
> > > https://urldefense.com/v3/__https://www.armlinux.org.uk/developer/patches/__;!!CTRNKA9wMg0ARbw!iV-1ViPFsUV-lLj7aIycan8nery6sQO3t6mkpdlb_GW8hswhxc4ejJozxqkU3s2WzxSizs4kfdC77yr7HGGRIuU$
> > > FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
> > Hi Russell,
> > mt798x_p5ge_phy_load_fw() will only load firmware once after driver
> > is
> > probed through priv->fw_loaded. And actually, firmware loading
> > procedure only takes about 11ms. This was discussed earlier in:
> > https://urldefense.com/v3/__https://patchwork.kernel.org/project/linux-mediatek/patch/20240520113456.21675-6-SkyLake.Huang@mediatek.com/*25856462__;Iw!!CTRNKA9wMg0ARbw!lQ7b7kgD0JGOHR_CLeHtF5SF-knx3hZcm2u_tYbzgBnX91a7muRwqGhDUaP3XorHlu9qQozNX_pXDMIxO1DyVIY$
> > https://urldefense.com/v3/__https://patchwork.kernel.org/project/linux-mediatek/patch/20240520113456.21675-6-SkyLake.Huang@mediatek.com/*25857174__;Iw!!CTRNKA9wMg0ARbw!lQ7b7kgD0JGOHR_CLeHtF5SF-knx3hZcm2u_tYbzgBnX91a7muRwqGhDUaP3XorHlu9qQozNX_pXDMIxHDm1IuQ$
> 
> 1. Wouldn't it be a good idea to include the loading time in the
> patch
>    description or a comment in the patch?
> 
Sure. I'll include this in v3's commit message.

> 2. What about the time it takes for request_firmware() uses the sysfs
>    fallback, which essentially passes the firmware request to
> userspace
>    to deal with? That can block for an indeterminate amount of time.
> 
I'm not really sure how to achieve this. As far as I know, I can
provide fw via /sys/class/firmware/<device-name>/ after I boot into
Linux shell. But at that moment, we need to insmod phy driver again.
Users will also need to load fw file manually. This will add a lot of
complexity. Is this what upstream style expects?

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

BRs,
Sky

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

[SkyLake Huang (黃啟澤)]

On Wed, 2025-02-26 at 14:32 +0100, Andrew Lunn wrote:
> 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> On Wed, Feb 26, 2025 at 06:48:34AM +0000, SkyLake Huang (黃啟澤) wrote:
> > On Wed, 2025-02-19 at 09:33 +0000, Russell King (Oracle) wrote:
> > > 
> > > External email : Please do not click links or open attachments
> > > until
> > > you have verified the sender or the content.
> > > 
> > > 
> > > On Wed, Feb 19, 2025 at 04:39:10PM +0800, Sky Huang wrote:
> > > > +static int mt798x_2p5ge_phy_config_init(struct phy_device
> > > > *phydev)
> > > > +{
> > > > +     struct pinctrl *pinctrl;
> > > > +     int ret;
> > > > +
> > > > +     /* Check if PHY interface type is compatible */
> > > > +     if (phydev->interface != PHY_INTERFACE_MODE_INTERNAL)
> > > > +             return -ENODEV;
> > > > +
> > > > +     ret = mt798x_2p5ge_phy_load_fw(phydev);
> > > > +     if (ret < 0)
> > > > +             return ret;
> > > 
> > > Firmware should not be loaded in the .config_init method. The
> > > above
> > > call will block while holding the RTNL which will prevent all
> > > other
> > > network configuration until the firmware has been loaded or the
> > > load
> > > fails.
> > > 
> > > Thanks.
> > > 
> > > --
> > > RMK's Patch system:
> > > https://urldefense.com/v3/__https://www.armlinux.org.uk/developer/patches/__;!!CTRNKA9wMg0ARbw!iV-1ViPFsUV-lLj7aIycan8nery6sQO3t6mkpdlb_GW8hswhxc4ejJozxqkU3s2WzxSizs4kfdC77yr7HGGRIuU$
> > > FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
> > Hi Russell,
> > mt798x_p5ge_phy_load_fw() will only load firmware once after driver
> > is
> > probed through priv->fw_loaded. And actually, firmware loading
> > procedure only takes about 11ms. This was discussed earlier in:
> > https://urldefense.com/v3/__https://patchwork.kernel.org/project/linux-mediatek/patch/20240520113456.21675-6-SkyLake.Huang@mediatek.com/*25856462__;Iw!!CTRNKA9wMg0ARbw!nEJAqWq9eSPyvD4sikg0DKqNCU5OPGCJx42J6muU1dWrHsNliA4BR1OU7r_XBf53OC02GphE1odIIYTT6687$
> > https://urldefense.com/v3/__https://patchwork.kernel.org/project/linux-mediatek/patch/20240520113456.21675-6-SkyLake.Huang@mediatek.com/*25857174__;Iw!!CTRNKA9wMg0ARbw!nEJAqWq9eSPyvD4sikg0DKqNCU5OPGCJx42J6muU1dWrHsNliA4BR1OU7r_XBf53OC02GphE1odIIc2tO_Yi$
> 
> Ideally, all PHY drivers should look like each other. That makes
> maintenance simpler. Currently, air_en8811h.c, aquantia_main.c, and
> qt2025.rs load firmware in there probe function. Is there a good
> reason this driver needs to be different?
> 
>        Andrew
  Actually, I wrote fw loading flow in .probe at first. However, during
boot time, .probe is called at very early stage (about the first 2s
after booting into Linux Kernel). At that time, filesystem isn't ready
yet and phy driver can't locate /lib/firmware/mediatek/mt7988/i2p5ge-
phy-pmb.bin. Also, adding "-EPROBE_DEFER" doesn't help.
  I'm not sure how aquantia and qt2025 drivers handle this.

BRs,
Sky

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

[Andrew Lunn]

On Tue, May 13, 2025 at 10:12:04AM +0000, SkyLake Huang (黃啟澤) wrote:
> On Wed, 2025-02-19 at 09:33 +0000, Russell King (Oracle) wrote:
> > 
> > External email : Please do not click links or open attachments until
> > you have verified the sender or the content.
> > 
> > 
> > On Wed, Feb 19, 2025 at 04:39:10PM +0800, Sky Huang wrote:
> > > +static int mt798x_2p5ge_phy_config_init(struct phy_device *phydev)
> > > +{
> > > +     struct pinctrl *pinctrl;
> > > +     int ret;
> > > +
> > > +     /* Check if PHY interface type is compatible */
> > > +     if (phydev->interface != PHY_INTERFACE_MODE_INTERNAL)
> > > +             return -ENODEV;
> > > +
> > > +     ret = mt798x_2p5ge_phy_load_fw(phydev);
> > > +     if (ret < 0)
> > > +             return ret;
> > 
> > Firmware should not be loaded in the .config_init method. The above
> > call will block while holding the RTNL which will prevent all other
> > network configuration until the firmware has been loaded or the load
> > fails.
> > 
> > Thanks.
> > 
> > --
> > RMK's Patch system:
> > https://urldefense.com/v3/__https://www.armlinux.org.uk/developer/patches/__;!!CTRNKA9wMg0ARbw!iV-1ViPFsUV-lLj7aIycan8nery6sQO3t6mkpdlb_GW8hswhxc4ejJozxqkU3s2WzxSizs4kfdC77yr7HGGRIuU$
> > FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
> 
> Actually, I wrote fw loading flow in .probe. However, during boot time,
> .probe is called at very early stage (about the first 2s after booting
> into Linux Kernel). At that time, filesystem isn't ready yet and phy
> driver can't locate /lib/firmware/mediatek/mt7988/i2p5ge-phy-pmb.bin.

Tell Dracut or whatever you are using to build the initramfs to
include the firmware. That is what MODULE_FIRMWARE() is for.

	Andrew