From: "SkyLake.Huang" <skylake.huang@mediatek.com>

Add support for internal 2.5Gphy on MT7988. This driver will load
necessary firmware, add appropriate time delay and figure out LED.
Also, certain control registers will be set to fix link-up issues.

Signed-off-by: SkyLake.Huang <skylake.huang@mediatek.com>
---
Changes in v2:
1. Move md32_en_cfg_base & pmb_addr detection in probe function.
2. Do not read PMB & MD32_EN_CFG base addresses from dts. We won't
change that from board to board. Leave them in driver code. Also,
release those addresses after firmware is triggered.
3. Remove half duplex code which leads to ambiguity. Those are for
testing & developing previously.
4. Use correct BMCR definitions.
5. Correct config_aneg / get_features / read_status functions.
6. Change mt7988_2p5ge prefix to mt798x_2p5ge in case that our next
platform uses this 2.5Gphy driver.

Changes in v3:
1. Add range check for firmware.
2. Fix c45_ids.mmds_present in probe function.
3. Still use genphy_update_link() in read_status because
genphy_c45_read_link() can't correct detect link on this phy.

Changes in v4:
1. Move firmware loading function to mt798x_2p5ge_phy_load_fw()
2. Add AN disable warning in mt798x_2p5ge_phy_config_aneg()
3. Clarify the HDX comments in mt798x_2p5ge_phy_get_features()

Changes in v5:
1. Move md32_en_cfg_base & pmb_addr to local variables to achieve
symmetric code.
2. Print out firmware date code & version.
3. Don't return error if LED pinctrl switching fails. Also, add
comments to this unusual operations.
4. Return -EOPNOTSUPP for AN off case in config_aneg().

Changes in v6:
1. Force casting (fw->data + MT7988_2P5GE_PMB_SIZE - 8) with __be16.
2. Remove parens on RHS of "phydev->c45_ids.mmds_present |=".
3. Add PHY_INTERFACE_MODE_INTERNAL check in
mt798x_2p5ge_phy_get_rate_matching()
4. Arrange local variables in reverse Xmas tree order.
---
 MAINTAINERS                          |   1 +
 drivers/net/phy/mediatek/Kconfig     |  11 +
 drivers/net/phy/mediatek/Makefile    |   1 +
 drivers/net/phy/mediatek/mtk-2p5ge.c | 435 +++++++++++++++++++++++++++
 4 files changed, 448 insertions(+)
 create mode 100644 drivers/net/phy/mediatek/mtk-2p5ge.c

diff --git a/MAINTAINERS b/MAINTAINERS
index e58e05c..fe380f2 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -13793,6 +13793,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 448bc20..1490352 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 814879d..c6db8ab 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 0000000..0c92608
--- /dev/null
+++ b/drivers/net/phy/mediatek/mtk-2p5ge.c
@@ -0,0 +1,435 @@
+// 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 "mediatek/mt7988/i2p5ge-phy-pmb.bin"
+#define MT7988_2P5GE_PMB_SIZE	(0x20000)
+#define MT7988_2P5GE_PMB_BASE	(0x0f100000)
+#define MT7988_2P5GE_PMB_LEN	(0x20000)
+#define MT7988_2P5GE_MD32_EN_CFG_BASE	(0x0f0f0018)
+#define MT7988_2P5GE_MD32_EN_CFG_LEN	(0x20)
+#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)
+
+#define MTK_PHY_LPI_PCS_DSP_CTRL		(0x121)
+#define   MTK_PHY_LPI_SIG_EN_LO_THRESH100_MASK	GENMASK(12, 8)
+
+/* 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;
+	unsigned long led_state;
+};
+
+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 *md32_en_cfg_base, *pmb_addr;
+	struct device *dev = &phydev->mdio.dev;
+	const struct firmware *fw;
+	int ret, i;
+	u16 reg;
+
+	if (priv->fw_loaded)
+		return 0;
+
+	pmb_addr = ioremap(MT7988_2P5GE_PMB_BASE, MT7988_2P5GE_PMB_LEN);
+	if (!pmb_addr)
+		return -ENOMEM;
+	md32_en_cfg_base = ioremap(MT7988_2P5GE_MD32_EN_CFG_BASE,
+				   MT7988_2P5GE_MD32_EN_CFG_LEN);
+	if (!md32_en_cfg_base) {
+		ret = -ENOMEM;
+		goto free_pmb;
+	}
+
+	ret = request_firmware(&fw, MT7988_2P5GE_PMB, dev);
+	if (ret) {
+		dev_err(dev, "failed to load firmware: %s, ret: %d\n",
+			MT7988_2P5GE_PMB, ret);
+		goto free;
+	}
+
+	if (fw->size != MT7988_2P5GE_PMB_SIZE) {
+		dev_err(dev, "Firmware size 0x%zx != 0x%x\n",
+			fw->size, MT7988_2P5GE_PMB_SIZE);
+		ret = -EINVAL;
+		goto free;
+	}
+
+	reg = readw(md32_en_cfg_base);
+	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, 0x800e, 0x1100);
+	phy_write_mmd(phydev, MDIO_MMD_VEND1, 0x800f, 0x00df);
+
+	for (i = 0; i < MT7988_2P5GE_PMB_SIZE - 1; i += 4)
+		writel(*((uint32_t *)(fw->data + i)), pmb_addr + i);
+	release_firmware(fw);
+	dev_info(dev, "Firmware date code: %x/%x/%x, version: %x.%x\n",
+		 be16_to_cpu(*((__be16 *)(fw->data +
+					  MT7988_2P5GE_PMB_SIZE - 8))),
+		 *(fw->data + MT7988_2P5GE_PMB_SIZE - 6),
+		 *(fw->data + MT7988_2P5GE_PMB_SIZE - 5),
+		 *(fw->data + MT7988_2P5GE_PMB_SIZE - 2),
+		 *(fw->data + MT7988_2P5GE_PMB_SIZE - 1));
+
+	writew(reg & ~MD32_EN, md32_en_cfg_base);
+	writew(reg | MD32_EN, md32_en_cfg_base);
+	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;
+
+free:
+	iounmap(md32_en_cfg_base);
+free_pmb:
+	iounmap(pmb_addr);
+
+	return ret ? ret : 0;
+}
+
+static int mt798x_2p5ge_phy_config_init(struct phy_device *phydev)
+{
+	struct pinctrl *pinctrl;
+	int ret;
+
+	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 */
+	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;
+
+	/* In fact, if we disable autoneg, we can't link up correctly:
+	 *  2.5G/1G: Need AN to exchange master/slave information.
+	 *  100M: Without AN, link starts at half duplex (According to
+	 *        IEEE 802.3-2018), which this phy doesn't support.
+	 *   10M: Deprecated in this ethernet phy.
+	 */
+	if (phydev->autoneg == AUTONEG_DISABLE)
+		return -EOPNOTSUPP;
+
+	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_c45_check_and_restart_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)
+{
+	if (iface == PHY_INTERFACE_MODE_XGMII ||
+	    iface == PHY_INTERFACE_MODE_INTERNAL)
+		return RATE_MATCH_PAUSE;
+	return RATE_MATCH_NONE;
+}
+
+static const unsigned long supported_triggers =
+	(BIT(TRIGGER_NETDEV_FULL_DUPLEX) |
+	 BIT(TRIGGER_NETDEV_LINK)        |
+	 BIT(TRIGGER_NETDEV_LINK_10)     |
+	 BIT(TRIGGER_NETDEV_LINK_100)    |
+	 BIT(TRIGGER_NETDEV_LINK_1000)   |
+	 BIT(TRIGGER_NETDEV_LINK_2500)   |
+	 BIT(TRIGGER_NETDEV_RX)          |
+	 BIT(TRIGGER_NETDEV_TX));
+
+static int mt798x_2p5ge_phy_led_blink_set(struct phy_device *phydev, u8 index,
+					  unsigned long *delay_on,
+					  unsigned long *delay_off)
+{
+	struct mtk_i2p5ge_phy_priv *priv = phydev->priv;
+	bool blinking = false;
+	int err = 0;
+
+	if (index > 1)
+		return -EINVAL;
+
+	if (delay_on && delay_off && (*delay_on > 0) && (*delay_off > 0)) {
+		blinking = true;
+		*delay_on = 50;
+		*delay_off = 50;
+	}
+
+	err = mtk_phy_hw_led_blink_set(phydev, index, &priv->led_state,
+				       blinking);
+	if (err)
+		return err;
+
+	return mtk_phy_hw_led_on_set(phydev, index, &priv->led_state,
+				     MTK_2P5GPHY_LED_ON_MASK, false);
+}
+
+static int mt798x_2p5ge_phy_led_brightness_set(struct phy_device *phydev,
+					       u8 index,
+					       enum led_brightness value)
+{
+	struct mtk_i2p5ge_phy_priv *priv = phydev->priv;
+	int err;
+
+	err = mtk_phy_hw_led_blink_set(phydev, index, &priv->led_state, false);
+	if (err)
+		return err;
+
+	return mtk_phy_hw_led_on_set(phydev, index, &priv->led_state,
+				     MTK_2P5GPHY_LED_ON_MASK,
+				     (value != LED_OFF));
+}
+
+static int mt798x_2p5ge_phy_led_hw_is_supported(struct phy_device *phydev,
+						u8 index, unsigned long rules)
+{
+	return mtk_phy_led_hw_is_supported(phydev, index, rules,
+					   supported_triggers);
+}
+
+static int mt798x_2p5ge_phy_led_hw_control_get(struct phy_device *phydev,
+					       u8 index, unsigned long *rules)
+{
+	struct mtk_i2p5ge_phy_priv *priv = phydev->priv;
+
+	return mtk_phy_led_hw_ctrl_get(phydev, index, rules, &priv->led_state,
+				       MTK_2P5GPHY_LED_ON_SET,
+				       MTK_2P5GPHY_LED_RX_BLINK_SET,
+				       MTK_2P5GPHY_LED_TX_BLINK_SET);
+};
+
+static int mt798x_2p5ge_phy_led_hw_control_set(struct phy_device *phydev,
+					       u8 index, unsigned long rules)
+{
+	struct mtk_i2p5ge_phy_priv *priv = phydev->priv;
+
+	return mtk_phy_led_hw_ctrl_set(phydev, index, rules, &priv->led_state,
+				       MTK_2P5GPHY_LED_ON_SET,
+				       MTK_2P5GPHY_LED_RX_BLINK_SET,
+				       MTK_2P5GPHY_LED_TX_BLINK_SET);
+};
+
+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_gephy_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,
+		.led_blink_set	= mt798x_2p5ge_phy_led_blink_set,
+		.led_brightness_set = mt798x_2p5ge_phy_led_brightness_set,
+		.led_hw_is_supported = mt798x_2p5ge_phy_led_hw_is_supported,
+		.led_hw_control_get = mt798x_2p5ge_phy_led_hw_control_get,
+		.led_hw_control_set = mt798x_2p5ge_phy_led_hw_control_set,
+	},
+};
+
+module_phy_driver(mtk_gephy_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);
-- 
2.34.1

On Mon, Jun 03, 2024 at 08:18:34PM +0800, Sky Huang wrote:
> From: "SkyLake.Huang" <skylake.huang@mediatek.com>
> 
> Add support for internal 2.5Gphy on MT7988. This driver will load
> necessary firmware, add appropriate time delay and figure out LED.
> Also, certain control registers will be set to fix link-up issues.
> 
> Signed-off-by: SkyLake.Huang <skylake.huang@mediatek.com>
> ---
> Changes in v2:
> 1. Move md32_en_cfg_base & pmb_addr detection in probe function.
> 2. Do not read PMB & MD32_EN_CFG base addresses from dts. We won't
> change that from board to board. Leave them in driver code. Also,
> release those addresses after firmware is triggered.
> 3. Remove half duplex code which leads to ambiguity. Those are for
> testing & developing previously.
> 4. Use correct BMCR definitions.
> 5. Correct config_aneg / get_features / read_status functions.
> 6. Change mt7988_2p5ge prefix to mt798x_2p5ge in case that our next
> platform uses this 2.5Gphy driver.
> 
> Changes in v3:
> 1. Add range check for firmware.
> 2. Fix c45_ids.mmds_present in probe function.
> 3. Still use genphy_update_link() in read_status because
> genphy_c45_read_link() can't correct detect link on this phy.
> 
> Changes in v4:
> 1. Move firmware loading function to mt798x_2p5ge_phy_load_fw()
> 2. Add AN disable warning in mt798x_2p5ge_phy_config_aneg()
> 3. Clarify the HDX comments in mt798x_2p5ge_phy_get_features()
> 
> Changes in v5:
> 1. Move md32_en_cfg_base & pmb_addr to local variables to achieve
> symmetric code.
> 2. Print out firmware date code & version.
> 3. Don't return error if LED pinctrl switching fails. Also, add
> comments to this unusual operations.
> 4. Return -EOPNOTSUPP for AN off case in config_aneg().
> 
> Changes in v6:
> 1. Force casting (fw->data + MT7988_2P5GE_PMB_SIZE - 8) with __be16.
> 2. Remove parens on RHS of "phydev->c45_ids.mmds_present |=".
> 3. Add PHY_INTERFACE_MODE_INTERNAL check in
> mt798x_2p5ge_phy_get_rate_matching()
> 4. Arrange local variables in reverse Xmas tree order.
> ---
>  MAINTAINERS                          |   1 +
>  drivers/net/phy/mediatek/Kconfig     |  11 +
>  drivers/net/phy/mediatek/Makefile    |   1 +
>  drivers/net/phy/mediatek/mtk-2p5ge.c | 435 +++++++++++++++++++++++++++
>  4 files changed, 448 insertions(+)
>  create mode 100644 drivers/net/phy/mediatek/mtk-2p5ge.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index e58e05c..fe380f2 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -13793,6 +13793,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 448bc20..1490352 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 814879d..c6db8ab 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 0000000..0c92608
> --- /dev/null
> +++ b/drivers/net/phy/mediatek/mtk-2p5ge.c
> @@ -0,0 +1,435 @@
> +// 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 "mediatek/mt7988/i2p5ge-phy-pmb.bin"
> +#define MT7988_2P5GE_PMB_SIZE	(0x20000)
> +#define MT7988_2P5GE_PMB_BASE	(0x0f100000)
> +#define MT7988_2P5GE_PMB_LEN	(0x20000)
> +#define MT7988_2P5GE_MD32_EN_CFG_BASE	(0x0f0f0018)
> +#define MT7988_2P5GE_MD32_EN_CFG_LEN	(0x20)
> +#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)
> +
> +#define MTK_PHY_LPI_PCS_DSP_CTRL		(0x121)
> +#define   MTK_PHY_LPI_SIG_EN_LO_THRESH100_MASK	GENMASK(12, 8)
> +
> +/* 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;
> +	unsigned long led_state;
> +};
> +
> +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 *md32_en_cfg_base, *pmb_addr;
> +	struct device *dev = &phydev->mdio.dev;
> +	const struct firmware *fw;
> +	int ret, i;
> +	u16 reg;
> +
> +	if (priv->fw_loaded)
> +		return 0;
> +
> +	pmb_addr = ioremap(MT7988_2P5GE_PMB_BASE, MT7988_2P5GE_PMB_LEN);
> +	if (!pmb_addr)
> +		return -ENOMEM;
> +	md32_en_cfg_base = ioremap(MT7988_2P5GE_MD32_EN_CFG_BASE,
> +				   MT7988_2P5GE_MD32_EN_CFG_LEN);
> +	if (!md32_en_cfg_base) {
> +		ret = -ENOMEM;
> +		goto free_pmb;
> +	}
> +
> +	ret = request_firmware(&fw, MT7988_2P5GE_PMB, dev);
> +	if (ret) {
> +		dev_err(dev, "failed to load firmware: %s, ret: %d\n",
> +			MT7988_2P5GE_PMB, ret);
> +		goto free;
> +	}
> +
> +	if (fw->size != MT7988_2P5GE_PMB_SIZE) {
> +		dev_err(dev, "Firmware size 0x%zx != 0x%x\n",
> +			fw->size, MT7988_2P5GE_PMB_SIZE);
> +		ret = -EINVAL;
> +		goto free;
> +	}
> +
> +	reg = readw(md32_en_cfg_base);
> +	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, 0x800e, 0x1100);
> +	phy_write_mmd(phydev, MDIO_MMD_VEND1, 0x800f, 0x00df);
> +
> +	for (i = 0; i < MT7988_2P5GE_PMB_SIZE - 1; i += 4)
> +		writel(*((uint32_t *)(fw->data + i)), pmb_addr + i);
> +	release_firmware(fw);
> +	dev_info(dev, "Firmware date code: %x/%x/%x, version: %x.%x\n",
> +		 be16_to_cpu(*((__be16 *)(fw->data +
> +					  MT7988_2P5GE_PMB_SIZE - 8))),
> +		 *(fw->data + MT7988_2P5GE_PMB_SIZE - 6),
> +		 *(fw->data + MT7988_2P5GE_PMB_SIZE - 5),
> +		 *(fw->data + MT7988_2P5GE_PMB_SIZE - 2),
> +		 *(fw->data + MT7988_2P5GE_PMB_SIZE - 1));
> +
> +	writew(reg & ~MD32_EN, md32_en_cfg_base);
> +	writew(reg | MD32_EN, md32_en_cfg_base);
> +	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;
> +
> +free:
> +	iounmap(md32_en_cfg_base);
> +free_pmb:
> +	iounmap(pmb_addr);
> +
> +	return ret ? ret : 0;
> +}
> +
> +static int mt798x_2p5ge_phy_config_init(struct phy_device *phydev)
> +{
> +	struct pinctrl *pinctrl;
> +	int ret;
> +
> +	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 */
> +	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;
> +
> +	/* In fact, if we disable autoneg, we can't link up correctly:
> +	 *  2.5G/1G: Need AN to exchange master/slave information.
> +	 *  100M: Without AN, link starts at half duplex (According to
> +	 *        IEEE 802.3-2018), which this phy doesn't support.
> +	 *   10M: Deprecated in this ethernet phy.
> +	 */
> +	if (phydev->autoneg == AUTONEG_DISABLE)
> +		return -EOPNOTSUPP;
> +
> +	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_c45_check_and_restart_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)
> +{
> +	if (iface == PHY_INTERFACE_MODE_XGMII ||
> +	    iface == PHY_INTERFACE_MODE_INTERNAL)
> +		return RATE_MATCH_PAUSE;
> +	return RATE_MATCH_NONE;

As the phy is always connected in the same way internally inside the MT7988
this check and destinction doesn't make sense to me.

Imho you should always return RATE_MATCH_PAUSE, unless the same PHY also
exists in other SoCs and/or is connected in different interface modes.

In any way, please explain this part to us, especially in which situation
exactly you want to return RATE_MATCH_NONE and for which reason.



> +}
> +
> +static const unsigned long supported_triggers =
> +	(BIT(TRIGGER_NETDEV_FULL_DUPLEX) |
> +	 BIT(TRIGGER_NETDEV_LINK)        |
> +	 BIT(TRIGGER_NETDEV_LINK_10)     |
> +	 BIT(TRIGGER_NETDEV_LINK_100)    |
> +	 BIT(TRIGGER_NETDEV_LINK_1000)   |
> +	 BIT(TRIGGER_NETDEV_LINK_2500)   |
> +	 BIT(TRIGGER_NETDEV_RX)          |
> +	 BIT(TRIGGER_NETDEV_TX));
> +
> +static int mt798x_2p5ge_phy_led_blink_set(struct phy_device *phydev, u8 index,
> +					  unsigned long *delay_on,
> +					  unsigned long *delay_off)
> +{
> +	struct mtk_i2p5ge_phy_priv *priv = phydev->priv;
> +	bool blinking = false;
> +	int err = 0;
> +
> +	if (index > 1)
> +		return -EINVAL;
> +
> +	if (delay_on && delay_off && (*delay_on > 0) && (*delay_off > 0)) {
> +		blinking = true;
> +		*delay_on = 50;
> +		*delay_off = 50;
> +	}
> +
> +	err = mtk_phy_hw_led_blink_set(phydev, index, &priv->led_state,
> +				       blinking);
> +	if (err)
> +		return err;
> +
> +	return mtk_phy_hw_led_on_set(phydev, index, &priv->led_state,
> +				     MTK_2P5GPHY_LED_ON_MASK, false);
> +}
> +
> +static int mt798x_2p5ge_phy_led_brightness_set(struct phy_device *phydev,
> +					       u8 index,
> +					       enum led_brightness value)
> +{
> +	struct mtk_i2p5ge_phy_priv *priv = phydev->priv;
> +	int err;
> +
> +	err = mtk_phy_hw_led_blink_set(phydev, index, &priv->led_state, false);
> +	if (err)
> +		return err;
> +
> +	return mtk_phy_hw_led_on_set(phydev, index, &priv->led_state,
> +				     MTK_2P5GPHY_LED_ON_MASK,
> +				     (value != LED_OFF));
> +}
> +
> +static int mt798x_2p5ge_phy_led_hw_is_supported(struct phy_device *phydev,
> +						u8 index, unsigned long rules)
> +{
> +	return mtk_phy_led_hw_is_supported(phydev, index, rules,
> +					   supported_triggers);
> +}
> +
> +static int mt798x_2p5ge_phy_led_hw_control_get(struct phy_device *phydev,
> +					       u8 index, unsigned long *rules)
> +{
> +	struct mtk_i2p5ge_phy_priv *priv = phydev->priv;
> +
> +	return mtk_phy_led_hw_ctrl_get(phydev, index, rules, &priv->led_state,
> +				       MTK_2P5GPHY_LED_ON_SET,
> +				       MTK_2P5GPHY_LED_RX_BLINK_SET,
> +				       MTK_2P5GPHY_LED_TX_BLINK_SET);
> +};
> +
> +static int mt798x_2p5ge_phy_led_hw_control_set(struct phy_device *phydev,
> +					       u8 index, unsigned long rules)
> +{
> +	struct mtk_i2p5ge_phy_priv *priv = phydev->priv;
> +
> +	return mtk_phy_led_hw_ctrl_set(phydev, index, rules, &priv->led_state,
> +				       MTK_2P5GPHY_LED_ON_SET,
> +				       MTK_2P5GPHY_LED_RX_BLINK_SET,
> +				       MTK_2P5GPHY_LED_TX_BLINK_SET);
> +};
> +
> +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_gephy_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,
> +		.led_blink_set	= mt798x_2p5ge_phy_led_blink_set,
> +		.led_brightness_set = mt798x_2p5ge_phy_led_brightness_set,
> +		.led_hw_is_supported = mt798x_2p5ge_phy_led_hw_is_supported,
> +		.led_hw_control_get = mt798x_2p5ge_phy_led_hw_control_get,
> +		.led_hw_control_set = mt798x_2p5ge_phy_led_hw_control_set,
> +	},
> +};
> +
> +module_phy_driver(mtk_gephy_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);
> -- 
> 2.34.1
> 
>

On Mon, 2024-06-03 at 20:40 +0100, Daniel Golle wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  On Mon, Jun 03, 2024 at 08:18:34PM +0800, Sky Huang wrote:
> > From: "SkyLake.Huang" <skylake.huang@mediatek.com>
> > +
> > +static int mt798x_2p5ge_phy_get_rate_matching(struct phy_device
> *phydev,
> > +      phy_interface_t iface)
> > +{
> > +if (iface == PHY_INTERFACE_MODE_XGMII ||
> > +    iface == PHY_INTERFACE_MODE_INTERNAL)
> > +return RATE_MATCH_PAUSE;
> > +return RATE_MATCH_NONE;
> 
> As the phy is always connected in the same way internally inside the
> MT7988
> this check and destinction doesn't make sense to me.
> 
> Imho you should always return RATE_MATCH_PAUSE, unless the same PHY
> also
> exists in other SoCs and/or is connected in different interface
> modes.
> 
> In any way, please explain this part to us, especially in which
> situation
> exactly you want to return RATE_MATCH_NONE and for which reason.
> 
  Actually internal 2.5Gphy is planned to be conntected to both XFI-MAC 
and GMAC at very first: (2.5G speed relies on XGMII path and
1G/100M/10M rely on GMII/MII path)
+---------+       +--------+
|         |       |        |
| XFI-MAC |       |  GMAC  |
|         |       |        |
+---------+       +--------+
        |           |
        |           |
      (FCM)         |
    +-------+----------+
    | XGMII | GMII/MII |
    +------------------+
    |                  |
    |     built-in     |
    |      2.5Gphy     |
    |                  |
    +------------------+
  This phy's rate adaptation is implemented in FCM (flow control
module) on the XGMII path. So mt798x_2p5ge_phy_get_rate_matching() will
only return RATE_MATCH_PAUSE for this path.(for developing purpose)
  However, GMII/MII is deprecated later in our built-in 2.5Gphy
hardware design. So yes, you're right.
mt798x_2p5ge_phy_get_rate_matching() should always return
RATE_MATCH_PAUSE now. I'll change this in next version.

Sky

On Mon, Jun 03, 2024 at 08:18:34PM +0800, Sky Huang wrote:
> Add support for internal 2.5Gphy on MT7988. This driver will load
> necessary firmware, add appropriate time delay and figure out LED.
> Also, certain control registers will be set to fix link-up issues.

Based on our previous discussion, it may be worth checking in the
.config_init() method whether phydev->interface is one of the
PHY interface modes that this PHY supports. As I understand from one
of your previous emails, the possibilities are XGMII, USXGMII or
INTERNAL. Thus:

> +static int mt798x_2p5ge_phy_config_init(struct phy_device *phydev)
> +{
> +	struct pinctrl *pinctrl;
> +	int ret;

	/* Check that the PHY interface type is compatible */
	if (phydev->interface != PHY_INTERFACE_MODE_INTERNAL &&
	    phydev->interface != PHY_INTERFACE_MODE_XGMII &&
	    phydev->interface != PHY_INTERFACE_MODE_USXGMII)
		return -ENODEV;

Thanks.

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

On Mon, Jun 03, 2024 at 02:25:01PM +0100, Russell King (Oracle) wrote:
> On Mon, Jun 03, 2024 at 08:18:34PM +0800, Sky Huang wrote:
> > Add support for internal 2.5Gphy on MT7988. This driver will load
> > necessary firmware, add appropriate time delay and figure out LED.
> > Also, certain control registers will be set to fix link-up issues.
> 
> Based on our previous discussion, it may be worth checking in the
> .config_init() method whether phydev->interface is one of the
> PHY interface modes that this PHY supports. As I understand from one
> of your previous emails, the possibilities are XGMII, USXGMII or
> INTERNAL. Thus:
> 
> > +static int mt798x_2p5ge_phy_config_init(struct phy_device *phydev)
> > +{
> > +	struct pinctrl *pinctrl;
> > +	int ret;
> 
> 	/* Check that the PHY interface type is compatible */
> 	if (phydev->interface != PHY_INTERFACE_MODE_INTERNAL &&
> 	    phydev->interface != PHY_INTERFACE_MODE_XGMII &&
> 	    phydev->interface != PHY_INTERFACE_MODE_USXGMII)
> 		return -ENODEV;

The PHY is built-into the SoC, and as such the connection type should
always be "internal". The PHY does not exist as dedicated IC, only
as built-in part of the MT7988 SoC.

On Mon, Jun 03, 2024 at 02:31:46PM +0100, Daniel Golle wrote:
> On Mon, Jun 03, 2024 at 02:25:01PM +0100, Russell King (Oracle) wrote:
> > On Mon, Jun 03, 2024 at 08:18:34PM +0800, Sky Huang wrote:
> > > Add support for internal 2.5Gphy on MT7988. This driver will load
> > > necessary firmware, add appropriate time delay and figure out LED.
> > > Also, certain control registers will be set to fix link-up issues.
> > 
> > Based on our previous discussion, it may be worth checking in the
> > .config_init() method whether phydev->interface is one of the
> > PHY interface modes that this PHY supports. As I understand from one
> > of your previous emails, the possibilities are XGMII, USXGMII or
> > INTERNAL. Thus:
> > 
> > > +static int mt798x_2p5ge_phy_config_init(struct phy_device *phydev)
> > > +{
> > > +	struct pinctrl *pinctrl;
> > > +	int ret;
> > 
> > 	/* Check that the PHY interface type is compatible */
> > 	if (phydev->interface != PHY_INTERFACE_MODE_INTERNAL &&
> > 	    phydev->interface != PHY_INTERFACE_MODE_XGMII &&
> > 	    phydev->interface != PHY_INTERFACE_MODE_USXGMII)
> > 		return -ENODEV;
> 
> The PHY is built-into the SoC, and as such the connection type should
> always be "internal". The PHY does not exist as dedicated IC, only
> as built-in part of the MT7988 SoC.

That's not how it was described to me by Sky.

If what you say is correct, then the implementation of
mt798x_2p5ge_phy_get_rate_matching() which checks for interface modes
other than INTERNAL is not correct. Also it means that config_init()
should not permit anything but INTERNAL.

Thanks.

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

On Mon, Jun 03, 2024 at 02:41:44PM +0100, Russell King (Oracle) wrote:
> On Mon, Jun 03, 2024 at 02:31:46PM +0100, Daniel Golle wrote:
> > On Mon, Jun 03, 2024 at 02:25:01PM +0100, Russell King (Oracle) wrote:
> > > On Mon, Jun 03, 2024 at 08:18:34PM +0800, Sky Huang wrote:
> > > > Add support for internal 2.5Gphy on MT7988. This driver will load
> > > > necessary firmware, add appropriate time delay and figure out LED.
> > > > Also, certain control registers will be set to fix link-up issues.
> > > 
> > > Based on our previous discussion, it may be worth checking in the
> > > .config_init() method whether phydev->interface is one of the
> > > PHY interface modes that this PHY supports. As I understand from one
> > > of your previous emails, the possibilities are XGMII, USXGMII or
> > > INTERNAL. Thus:
> > > 
> > > > +static int mt798x_2p5ge_phy_config_init(struct phy_device *phydev)
> > > > +{
> > > > +	struct pinctrl *pinctrl;
> > > > +	int ret;
> > > 
> > > 	/* Check that the PHY interface type is compatible */
> > > 	if (phydev->interface != PHY_INTERFACE_MODE_INTERNAL &&
> > > 	    phydev->interface != PHY_INTERFACE_MODE_XGMII &&
> > > 	    phydev->interface != PHY_INTERFACE_MODE_USXGMII)
> > > 		return -ENODEV;
> > 
> > The PHY is built-into the SoC, and as such the connection type should
> > always be "internal". The PHY does not exist as dedicated IC, only
> > as built-in part of the MT7988 SoC.
> 
> That's not how it was described to me by Sky.
> 
> If what you say is correct, then the implementation of
> mt798x_2p5ge_phy_get_rate_matching() which checks for interface modes
> other than INTERNAL is not correct. Also it means that config_init()
> should not permit anything but INTERNAL.

The way the PHY is connected to the MAC *inside the chip* is XGMII
according the MediaTek. So call it "internal" or "xgmii", however, up to
my knowledge it's a fact that there is **only one way** this PHY is
connected and used, and that is being an internal part of the MT7988 SoC.

Imho, as there are no actual XGMII signals exposed anywhere I'd use
"internal" to describe the link between MAC and PHY (which are both
inside the same chip package).

On Mon, Jun 03, 2024 at 03:52:19PM +0100, Daniel Golle wrote:
> On Mon, Jun 03, 2024 at 02:41:44PM +0100, Russell King (Oracle) wrote:
> > On Mon, Jun 03, 2024 at 02:31:46PM +0100, Daniel Golle wrote:
> > > On Mon, Jun 03, 2024 at 02:25:01PM +0100, Russell King (Oracle) wrote:
> > > > On Mon, Jun 03, 2024 at 08:18:34PM +0800, Sky Huang wrote:
> > > > > Add support for internal 2.5Gphy on MT7988. This driver will load
> > > > > necessary firmware, add appropriate time delay and figure out LED.
> > > > > Also, certain control registers will be set to fix link-up issues.
> > > > 
> > > > Based on our previous discussion, it may be worth checking in the
> > > > .config_init() method whether phydev->interface is one of the
> > > > PHY interface modes that this PHY supports. As I understand from one
> > > > of your previous emails, the possibilities are XGMII, USXGMII or
> > > > INTERNAL. Thus:
> > > > 
> > > > > +static int mt798x_2p5ge_phy_config_init(struct phy_device *phydev)
> > > > > +{
> > > > > +	struct pinctrl *pinctrl;
> > > > > +	int ret;
> > > > 
> > > > 	/* Check that the PHY interface type is compatible */
> > > > 	if (phydev->interface != PHY_INTERFACE_MODE_INTERNAL &&
> > > > 	    phydev->interface != PHY_INTERFACE_MODE_XGMII &&
> > > > 	    phydev->interface != PHY_INTERFACE_MODE_USXGMII)
> > > > 		return -ENODEV;
> > > 
> > > The PHY is built-into the SoC, and as such the connection type should
> > > always be "internal". The PHY does not exist as dedicated IC, only
> > > as built-in part of the MT7988 SoC.
> > 
> > That's not how it was described to me by Sky.
> > 
> > If what you say is correct, then the implementation of
> > mt798x_2p5ge_phy_get_rate_matching() which checks for interface modes
> > other than INTERNAL is not correct. Also it means that config_init()
> > should not permit anything but INTERNAL.
> 
> The way the PHY is connected to the MAC *inside the chip* is XGMII
> according the MediaTek. So call it "internal" or "xgmii", however, up to
> my knowledge it's a fact that there is **only one way** this PHY is
> connected and used, and that is being an internal part of the MT7988 SoC.
> 
> Imho, as there are no actual XGMII signals exposed anywhere I'd use
> "internal" to describe the link between MAC and PHY (which are both
> inside the same chip package).

I don't care what gets decided about what's acceptable for the PHY to
accept, just that it checks for the acceptable modes in .config_init()
and the .get_rate_matching() method is not checking for interface
modes that are not permitted.

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

On Mon, Jun 03, 2024 at 04:47:28PM +0100, Russell King (Oracle) wrote:
> On Mon, Jun 03, 2024 at 03:52:19PM +0100, Daniel Golle wrote:
> > On Mon, Jun 03, 2024 at 02:41:44PM +0100, Russell King (Oracle) wrote:
> > > On Mon, Jun 03, 2024 at 02:31:46PM +0100, Daniel Golle wrote:
> > > > On Mon, Jun 03, 2024 at 02:25:01PM +0100, Russell King (Oracle) wrote:
> > > > > On Mon, Jun 03, 2024 at 08:18:34PM +0800, Sky Huang wrote:
> > > > > > Add support for internal 2.5Gphy on MT7988. This driver will load
> > > > > > necessary firmware, add appropriate time delay and figure out LED.
> > > > > > Also, certain control registers will be set to fix link-up issues.
> > > > > 
> > > > > Based on our previous discussion, it may be worth checking in the
> > > > > .config_init() method whether phydev->interface is one of the
> > > > > PHY interface modes that this PHY supports. As I understand from one
> > > > > of your previous emails, the possibilities are XGMII, USXGMII or
> > > > > INTERNAL. Thus:
> > > > > 
> > > > > > +static int mt798x_2p5ge_phy_config_init(struct phy_device *phydev)
> > > > > > +{
> > > > > > +	struct pinctrl *pinctrl;
> > > > > > +	int ret;
> > > > > 
> > > > > 	/* Check that the PHY interface type is compatible */
> > > > > 	if (phydev->interface != PHY_INTERFACE_MODE_INTERNAL &&
> > > > > 	    phydev->interface != PHY_INTERFACE_MODE_XGMII &&
> > > > > 	    phydev->interface != PHY_INTERFACE_MODE_USXGMII)
> > > > > 		return -ENODEV;
> > > > 
> > > > The PHY is built-into the SoC, and as such the connection type should
> > > > always be "internal". The PHY does not exist as dedicated IC, only
> > > > as built-in part of the MT7988 SoC.
> > > 
> > > That's not how it was described to me by Sky.
> > > 
> > > If what you say is correct, then the implementation of
> > > mt798x_2p5ge_phy_get_rate_matching() which checks for interface modes
> > > other than INTERNAL is not correct. Also it means that config_init()
> > > should not permit anything but INTERNAL.
> > 
> > The way the PHY is connected to the MAC *inside the chip* is XGMII
> > according the MediaTek. So call it "internal" or "xgmii", however, up to
> > my knowledge it's a fact that there is **only one way** this PHY is
> > connected and used, and that is being an internal part of the MT7988 SoC.
> > 
> > Imho, as there are no actual XGMII signals exposed anywhere I'd use
> > "internal" to describe the link between MAC and PHY (which are both
> > inside the same chip package).
> 
> I don't care what gets decided about what's acceptable for the PHY to
> accept, just that it checks for the acceptable modes in .config_init()
> and the .get_rate_matching() method is not checking for interface
> modes that are not permitted.

What I meant to express is that there is no need for such a check, also
not in config_init. There is only one way and one MAC-side interface mode
to operate that PHY, so the value will anyway not be considered anywhere
in the driver.

On Mon, Jun 03, 2024 at 06:00:49PM +0100, Daniel Golle wrote:
> On Mon, Jun 03, 2024 at 04:47:28PM +0100, Russell King (Oracle) wrote:
> > On Mon, Jun 03, 2024 at 03:52:19PM +0100, Daniel Golle wrote:
> > > On Mon, Jun 03, 2024 at 02:41:44PM +0100, Russell King (Oracle) wrote:
> > > > On Mon, Jun 03, 2024 at 02:31:46PM +0100, Daniel Golle wrote:
> > > > > On Mon, Jun 03, 2024 at 02:25:01PM +0100, Russell King (Oracle) wrote:
> > > > > > On Mon, Jun 03, 2024 at 08:18:34PM +0800, Sky Huang wrote:
> > > > > > > Add support for internal 2.5Gphy on MT7988. This driver will load
> > > > > > > necessary firmware, add appropriate time delay and figure out LED.
> > > > > > > Also, certain control registers will be set to fix link-up issues.
> > > > > > 
> > > > > > Based on our previous discussion, it may be worth checking in the
> > > > > > .config_init() method whether phydev->interface is one of the
> > > > > > PHY interface modes that this PHY supports. As I understand from one
> > > > > > of your previous emails, the possibilities are XGMII, USXGMII or
> > > > > > INTERNAL. Thus:
> > > > > > 
> > > > > > > +static int mt798x_2p5ge_phy_config_init(struct phy_device *phydev)
> > > > > > > +{
> > > > > > > +	struct pinctrl *pinctrl;
> > > > > > > +	int ret;
> > > > > > 
> > > > > > 	/* Check that the PHY interface type is compatible */
> > > > > > 	if (phydev->interface != PHY_INTERFACE_MODE_INTERNAL &&
> > > > > > 	    phydev->interface != PHY_INTERFACE_MODE_XGMII &&
> > > > > > 	    phydev->interface != PHY_INTERFACE_MODE_USXGMII)
> > > > > > 		return -ENODEV;
> > > > > 
> > > > > The PHY is built-into the SoC, and as such the connection type should
> > > > > always be "internal". The PHY does not exist as dedicated IC, only
> > > > > as built-in part of the MT7988 SoC.
> > > > 
> > > > That's not how it was described to me by Sky.
> > > > 
> > > > If what you say is correct, then the implementation of
> > > > mt798x_2p5ge_phy_get_rate_matching() which checks for interface modes
> > > > other than INTERNAL is not correct. Also it means that config_init()
> > > > should not permit anything but INTERNAL.
> > > 
> > > The way the PHY is connected to the MAC *inside the chip* is XGMII
> > > according the MediaTek. So call it "internal" or "xgmii", however, up to
> > > my knowledge it's a fact that there is **only one way** this PHY is
> > > connected and used, and that is being an internal part of the MT7988 SoC.
> > > 
> > > Imho, as there are no actual XGMII signals exposed anywhere I'd use
> > > "internal" to describe the link between MAC and PHY (which are both
> > > inside the same chip package).
> > 
> > I don't care what gets decided about what's acceptable for the PHY to
> > accept, just that it checks for the acceptable modes in .config_init()
> > and the .get_rate_matching() method is not checking for interface
> > modes that are not permitted.
> 
> What I meant to express is that there is no need for such a check, also
> not in config_init. There is only one way and one MAC-side interface mode
> to operate that PHY, so the value will anyway not be considered anywhere
> in the driver.

No, it matters. With drivers using phylink, the PHY interface mode is
used in certain circumstances to constrain what the net device can do.
So, it makes sense for new PHY drivers to ensure that the PHY interface
mode is one that they can support, rather than just accepting whatever
is passed to them (which then can lead to maintainability issues for
subsystems.)

So, excuse me for disagreeing with you, but I do want to see such a
check in new PHY drivers.

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

On Mon, 2024-06-03 at 19:30 +0100, Russell King (Oracle) wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  On Mon, Jun 03, 2024 at 06:00:49PM +0100, Daniel Golle wrote:
> > On Mon, Jun 03, 2024 at 04:47:28PM +0100, Russell King (Oracle)
> wrote:
> > > On Mon, Jun 03, 2024 at 03:52:19PM +0100, Daniel Golle wrote:
> > > > On Mon, Jun 03, 2024 at 02:41:44PM +0100, Russell King (Oracle)
> wrote:
> > > > > On Mon, Jun 03, 2024 at 02:31:46PM +0100, Daniel Golle wrote:
> > > > > > On Mon, Jun 03, 2024 at 02:25:01PM +0100, Russell King
> (Oracle) wrote:
> > > > > > > On Mon, Jun 03, 2024 at 08:18:34PM +0800, Sky Huang
> wrote:
> > > > > > > > Add support for internal 2.5Gphy on MT7988. This driver
> will load
> > > > > > > > necessary firmware, add appropriate time delay and
> figure out LED.
> > > > > > > > Also, certain control registers will be set to fix
> link-up issues.
> > > > > > > 
> > > > > > > Based on our previous discussion, it may be worth
> checking in the
> > > > > > > .config_init() method whether phydev->interface is one of
> the
> > > > > > > PHY interface modes that this PHY supports. As I
> understand from one
> > > > > > > of your previous emails, the possibilities are XGMII,
> USXGMII or
> > > > > > > INTERNAL. Thus:
> > > > > > > 
> > > > > > > > +static int mt798x_2p5ge_phy_config_init(struct
> phy_device *phydev)
> > > > > > > > +{
> > > > > > > > +struct pinctrl *pinctrl;
> > > > > > > > +int ret;
> > > > > > > 
> > > > > > > /* Check that the PHY interface type is compatible */
> > > > > > > if (phydev->interface != PHY_INTERFACE_MODE_INTERNAL &&
> > > > > > >     phydev->interface != PHY_INTERFACE_MODE_XGMII &&
> > > > > > >     phydev->interface != PHY_INTERFACE_MODE_USXGMII)
> > > > > > > return -ENODEV;
> > > > > > 
> > > > > > The PHY is built-into the SoC, and as such the connection
> type should
> > > > > > always be "internal". The PHY does not exist as dedicated
> IC, only
> > > > > > as built-in part of the MT7988 SoC.
> > > > > 
> > > > > That's not how it was described to me by Sky.
> > > > > 
> > > > > If what you say is correct, then the implementation of
> > > > > mt798x_2p5ge_phy_get_rate_matching() which checks for
> interface modes
> > > > > other than INTERNAL is not correct. Also it means that
> config_init()
> > > > > should not permit anything but INTERNAL.
> > > > 
> > > > The way the PHY is connected to the MAC *inside the chip* is
> XGMII
> > > > according the MediaTek. So call it "internal" or "xgmii",
> however, up to
> > > > my knowledge it's a fact that there is **only one way** this
> PHY is
> > > > connected and used, and that is being an internal part of the
> MT7988 SoC.
> > > > 
> > > > Imho, as there are no actual XGMII signals exposed anywhere I'd
> use
> > > > "internal" to describe the link between MAC and PHY (which are
> both
> > > > inside the same chip package).
> > > 
> > > I don't care what gets decided about what's acceptable for the
> PHY to
> > > accept, just that it checks for the acceptable modes in
> .config_init()
> > > and the .get_rate_matching() method is not checking for interface
> > > modes that are not permitted.
> > 
> > What I meant to express is that there is no need for such a check,
> also
> > not in config_init. There is only one way and one MAC-side
> interface mode
> > to operate that PHY, so the value will anyway not be considered
> anywhere
> > in the driver.
> 
> No, it matters. With drivers using phylink, the PHY interface mode is
> used in certain circumstances to constrain what the net device can
> do.
> So, it makes sense for new PHY drivers to ensure that the PHY
> interface
> mode is one that they can support, rather than just accepting
> whatever
> is passed to them (which then can lead to maintainability issues for
> subsystems.)
> 
> So, excuse me for disagreeing with you, but I do want to see such a
> check in new PHY drivers.
> 
> -- 
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Hi Russell/Daniel,
  IMO, we can check PHY_INTERFACE_MODE_INTERNAL &
PHY_INTERFACE_MODE_XGMII in config_init() or probe(). However,
PHY_INTERFACE_MODE_USXGMII isn't supported by this phy, and
drivers/net/ethernet/mediatek/mtk_eth_path.c uses
PHY_INTERFACE_MODE_USXGMII to switch netsys pcs mux (set
MUX_G2_USXGMII_SEL bit in TOP_MISC_NETSYS_PCS_MUX) so that XFI-MAC can
be connected to external 10Gphy.
  So, basically, for 1st XFI-MAC on mt7988:
- PHY_INTERFACE_MODE_XGMII/PHY_INTERFACE_MODE_INTERNAL: built-in
2.5Gphy
- PHY_INTERFACE_MODE_USXGMII: external 10Gphy

  I add check in config_init():
/* Check if PHY interface type is compatible */
if (phydev->interface != PHY_INTERFACE_MODE_XGMII &&
    phydev->interface != PHY_INTERFACE_MODE_INTERNAL)
	return -ENODEV;

  Also, test with different phy mode in dts:
[PHY_INTERFACE_MODE_USXGMII]
[   18.702102] mtk_soc_eth 15100000.ethernet eth1: mtk_open: could not
attach PHY: -19
root@OpenWrt:/# cat /proc/device-tree/soc/ethernet@15100000/mac@1/phy-c
onnection-type
usxgmii

[PHY_INTERFACE_MODE_INTERNAL]
[   18.329513] mtk_soc_eth 15100000.ethernet eth1: PHY [mdio-bus:0f]
driver [MediaTek MT7988 2.5GbE PHY] (irq=POLL)
[   18.339708] mtk_soc_eth 15100000.ethernet eth1: configuring for
phy/internal link mode
root@OpenWrt:/# cat /proc/device-tree/soc/ethernet@15100000
/mac@1/phy-connection-type
internal

Sky

On Tue, Jun 04, 2024 at 08:42:57AM +0000, SkyLake Huang (黃啟澤) wrote:
> On Mon, 2024-06-03 at 19:30 +0100, Russell King (Oracle) wrote:
> >  	 
> > External email : Please do not click links or open attachments until
> > you have verified the sender or the content.
> >  On Mon, Jun 03, 2024 at 06:00:49PM +0100, Daniel Golle wrote:
> > > On Mon, Jun 03, 2024 at 04:47:28PM +0100, Russell King (Oracle)
> > wrote:
> > > > On Mon, Jun 03, 2024 at 03:52:19PM +0100, Daniel Golle wrote:
> > > > > On Mon, Jun 03, 2024 at 02:41:44PM +0100, Russell King (Oracle)
> > wrote:
> > > > > > On Mon, Jun 03, 2024 at 02:31:46PM +0100, Daniel Golle wrote:
> > > > > > > On Mon, Jun 03, 2024 at 02:25:01PM +0100, Russell King
> > (Oracle) wrote:
> > > > > > > > On Mon, Jun 03, 2024 at 08:18:34PM +0800, Sky Huang
> > wrote:
> > > > > > > > > Add support for internal 2.5Gphy on MT7988. This driver
> > will load
> > > > > > > > > necessary firmware, add appropriate time delay and
> > figure out LED.
> > > > > > > > > Also, certain control registers will be set to fix
> > link-up issues.
> > > > > > > > 
> > > > > > > > Based on our previous discussion, it may be worth
> > checking in the
> > > > > > > > .config_init() method whether phydev->interface is one of
> > the
> > > > > > > > PHY interface modes that this PHY supports. As I
> > understand from one
> > > > > > > > of your previous emails, the possibilities are XGMII,
> > USXGMII or
> > > > > > > > INTERNAL. Thus:
> > > > > > > > 
> > > > > > > > > +static int mt798x_2p5ge_phy_config_init(struct
> > phy_device *phydev)
> > > > > > > > > +{
> > > > > > > > > +struct pinctrl *pinctrl;
> > > > > > > > > +int ret;
> > > > > > > > 
> > > > > > > > /* Check that the PHY interface type is compatible */
> > > > > > > > if (phydev->interface != PHY_INTERFACE_MODE_INTERNAL &&
> > > > > > > >     phydev->interface != PHY_INTERFACE_MODE_XGMII &&
> > > > > > > >     phydev->interface != PHY_INTERFACE_MODE_USXGMII)
> > > > > > > > return -ENODEV;
> > > > > > > 
> > > > > > > The PHY is built-into the SoC, and as such the connection
> > type should
> > > > > > > always be "internal". The PHY does not exist as dedicated
> > IC, only
> > > > > > > as built-in part of the MT7988 SoC.
> > > > > > 
> > > > > > That's not how it was described to me by Sky.
> > > > > > 
> > > > > > If what you say is correct, then the implementation of
> > > > > > mt798x_2p5ge_phy_get_rate_matching() which checks for
> > interface modes
> > > > > > other than INTERNAL is not correct. Also it means that
> > config_init()
> > > > > > should not permit anything but INTERNAL.
> > > > > 
> > > > > The way the PHY is connected to the MAC *inside the chip* is
> > XGMII
> > > > > according the MediaTek. So call it "internal" or "xgmii",
> > however, up to
> > > > > my knowledge it's a fact that there is **only one way** this
> > PHY is
> > > > > connected and used, and that is being an internal part of the
> > MT7988 SoC.
> > > > > 
> > > > > Imho, as there are no actual XGMII signals exposed anywhere I'd
> > use
> > > > > "internal" to describe the link between MAC and PHY (which are
> > both
> > > > > inside the same chip package).
> > > > 
> > > > I don't care what gets decided about what's acceptable for the
> > PHY to
> > > > accept, just that it checks for the acceptable modes in
> > .config_init()
> > > > and the .get_rate_matching() method is not checking for interface
> > > > modes that are not permitted.
> > > 
> > > What I meant to express is that there is no need for such a check,
> > also
> > > not in config_init. There is only one way and one MAC-side
> > interface mode
> > > to operate that PHY, so the value will anyway not be considered
> > anywhere
> > > in the driver.
> > 
> > No, it matters. With drivers using phylink, the PHY interface mode is
> > used in certain circumstances to constrain what the net device can
> > do.
> > So, it makes sense for new PHY drivers to ensure that the PHY
> > interface
> > mode is one that they can support, rather than just accepting
> > whatever
> > is passed to them (which then can lead to maintainability issues for
> > subsystems.)
> > 
> > So, excuse me for disagreeing with you, but I do want to see such a
> > check in new PHY drivers.
> > 
> > -- 
> > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> > FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
> 
> Hi Russell/Daniel,
>   IMO, we can check PHY_INTERFACE_MODE_INTERNAL &
> PHY_INTERFACE_MODE_XGMII in config_init() or probe(). However,
> PHY_INTERFACE_MODE_USXGMII isn't supported by this phy, and
> drivers/net/ethernet/mediatek/mtk_eth_path.c uses
> PHY_INTERFACE_MODE_USXGMII to switch netsys pcs mux (set
> MUX_G2_USXGMII_SEL bit in TOP_MISC_NETSYS_PCS_MUX) so that XFI-MAC can
> be connected to external 10Gphy.
>   So, basically, for 1st XFI-MAC on mt7988:
> - PHY_INTERFACE_MODE_XGMII/PHY_INTERFACE_MODE_INTERNAL: built-in
> 2.5Gphy

Why both? Wouldn't just PHY_INTERFACE_MODE_INTERNAL be more clear?
There is no XGMII interface exposed anywhere and both "internal" and
"xgmii" would be used to express the exact same thing.

> - PHY_INTERFACE_MODE_USXGMII: external 10Gphy
> 
>   I add check in config_init():
> /* Check if PHY interface type is compatible */
> if (phydev->interface != PHY_INTERFACE_MODE_XGMII &&
>     phydev->interface != PHY_INTERFACE_MODE_INTERNAL)
> 	return -ENODEV;
> 
>   Also, test with different phy mode in dts:
> [PHY_INTERFACE_MODE_USXGMII]
> [   18.702102] mtk_soc_eth 15100000.ethernet eth1: mtk_open: could not
> attach PHY: -19
> root@OpenWrt:/# cat /proc/device-tree/soc/ethernet@15100000/mac@1/phy-c
> onnection-type
> usxgmii
> 
> [PHY_INTERFACE_MODE_INTERNAL]
> [   18.329513] mtk_soc_eth 15100000.ethernet eth1: PHY [mdio-bus:0f]
> driver [MediaTek MT7988 2.5GbE PHY] (irq=POLL)
> [   18.339708] mtk_soc_eth 15100000.ethernet eth1: configuring for
> phy/internal link mode
> root@OpenWrt:/# cat /proc/device-tree/soc/ethernet@15100000
> /mac@1/phy-connection-type
> internal
> 
> Sky

On Tue, 2024-06-04 at 14:50 +0100, Daniel Golle wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  On Tue, Jun 04, 2024 at 08:42:57AM +0000, SkyLake Huang (黃啟澤) wrote:
> > On Mon, 2024-06-03 at 19:30 +0100, Russell King (Oracle) wrote:
> > >   
> > > External email : Please do not click links or open attachments
> until
> > > you have verified the sender or the content.
> > >  On Mon, Jun 03, 2024 at 06:00:49PM +0100, Daniel Golle wrote:
> > > > On Mon, Jun 03, 2024 at 04:47:28PM +0100, Russell King (Oracle)
> > > wrote:
> > > > > On Mon, Jun 03, 2024 at 03:52:19PM +0100, Daniel Golle wrote:
> > > > > > On Mon, Jun 03, 2024 at 02:41:44PM +0100, Russell King
> (Oracle)
> > > wrote:
> > > > > > > On Mon, Jun 03, 2024 at 02:31:46PM +0100, Daniel Golle
> wrote:
> > > > > > > > On Mon, Jun 03, 2024 at 02:25:01PM +0100, Russell King
> > > (Oracle) wrote:
> > > > > > > > > On Mon, Jun 03, 2024 at 08:18:34PM +0800, Sky Huang
> > > wrote:
> > > > > > > > > > Add support for internal 2.5Gphy on MT7988. This
> driver
> > > will load
> > > > > > > > > > necessary firmware, add appropriate time delay and
> > > figure out LED.
> > > > > > > > > > Also, certain control registers will be set to fix
> > > link-up issues.
> > > > > > > > > 
> > > > > > > > > Based on our previous discussion, it may be worth
> > > checking in the
> > > > > > > > > .config_init() method whether phydev->interface is
> one of
> > > the
> > > > > > > > > PHY interface modes that this PHY supports. As I
> > > understand from one
> > > > > > > > > of your previous emails, the possibilities are XGMII,
> > > USXGMII or
> > > > > > > > > INTERNAL. Thus:
> > > > > > > > > 
> > > > > > > > > > +static int mt798x_2p5ge_phy_config_init(struct
> > > phy_device *phydev)
> > > > > > > > > > +{
> > > > > > > > > > +struct pinctrl *pinctrl;
> > > > > > > > > > +int ret;
> > > > > > > > > 
> > > > > > > > > /* Check that the PHY interface type is compatible */
> > > > > > > > > if (phydev->interface != PHY_INTERFACE_MODE_INTERNAL
> &&
> > > > > > > > >     phydev->interface != PHY_INTERFACE_MODE_XGMII &&
> > > > > > > > >     phydev->interface != PHY_INTERFACE_MODE_USXGMII)
> > > > > > > > > return -ENODEV;
> > > > > > > > 
> > > > > > > > The PHY is built-into the SoC, and as such the
> connection
> > > type should
> > > > > > > > always be "internal". The PHY does not exist as
> dedicated
> > > IC, only
> > > > > > > > as built-in part of the MT7988 SoC.
> > > > > > > 
> > > > > > > That's not how it was described to me by Sky.
> > > > > > > 
> > > > > > > If what you say is correct, then the implementation of
> > > > > > > mt798x_2p5ge_phy_get_rate_matching() which checks for
> > > interface modes
> > > > > > > other than INTERNAL is not correct. Also it means that
> > > config_init()
> > > > > > > should not permit anything but INTERNAL.
> > > > > > 
> > > > > > The way the PHY is connected to the MAC *inside the chip*
> is
> > > XGMII
> > > > > > according the MediaTek. So call it "internal" or "xgmii",
> > > however, up to
> > > > > > my knowledge it's a fact that there is **only one way**
> this
> > > PHY is
> > > > > > connected and used, and that is being an internal part of
> the
> > > MT7988 SoC.
> > > > > > 
> > > > > > Imho, as there are no actual XGMII signals exposed anywhere
> I'd
> > > use
> > > > > > "internal" to describe the link between MAC and PHY (which
> are
> > > both
> > > > > > inside the same chip package).
> > > > > 
> > > > > I don't care what gets decided about what's acceptable for
> the
> > > PHY to
> > > > > accept, just that it checks for the acceptable modes in
> > > .config_init()
> > > > > and the .get_rate_matching() method is not checking for
> interface
> > > > > modes that are not permitted.
> > > > 
> > > > What I meant to express is that there is no need for such a
> check,
> > > also
> > > > not in config_init. There is only one way and one MAC-side
> > > interface mode
> > > > to operate that PHY, so the value will anyway not be considered
> > > anywhere
> > > > in the driver.
> > > 
> > > No, it matters. With drivers using phylink, the PHY interface
> mode is
> > > used in certain circumstances to constrain what the net device
> can
> > > do.
> > > So, it makes sense for new PHY drivers to ensure that the PHY
> > > interface
> > > mode is one that they can support, rather than just accepting
> > > whatever
> > > is passed to them (which then can lead to maintainability issues
> for
> > > subsystems.)
> > > 
> > > So, excuse me for disagreeing with you, but I do want to see such
> a
> > > check in new PHY drivers.
> > > 
> > > -- 
> > > RMK's Patch system: 
> https://www.armlinux.org.uk/developer/patches/
> > > FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
> > 
> > Hi Russell/Daniel,
> >   IMO, we can check PHY_INTERFACE_MODE_INTERNAL &
> > PHY_INTERFACE_MODE_XGMII in config_init() or probe(). However,
> > PHY_INTERFACE_MODE_USXGMII isn't supported by this phy, and
> > drivers/net/ethernet/mediatek/mtk_eth_path.c uses
> > PHY_INTERFACE_MODE_USXGMII to switch netsys pcs mux (set
> > MUX_G2_USXGMII_SEL bit in TOP_MISC_NETSYS_PCS_MUX) so that XFI-MAC
> can
> > be connected to external 10Gphy.
> >   So, basically, for 1st XFI-MAC on mt7988:
> > - PHY_INTERFACE_MODE_XGMII/PHY_INTERFACE_MODE_INTERNAL: built-in
> > 2.5Gphy
> 
> Why both? Wouldn't just PHY_INTERFACE_MODE_INTERNAL be more clear?
> There is no XGMII interface exposed anywhere and both "internal" and
> "xgmii" would be used to express the exact same thing.
> 
Since I found out that there's phy-mode="gmii" in mt7981-rfb.dts on
openWRT, I thought maybe we need gmii&internal for mt7981's built-in
GbE and xgmii&internal for mt7988's built-in 2.5GbE.

https://git.openwrt.org/?p=openwrt/openwrt.git;a=blob;f=target/linux/mediatek/files-6.6/arch/arm64/boot/dts/mediatek/mt7981-rfb.dts;h=b2bb6929562ebd58550477cdbf8838d8bb38d489;hb=refs/heads/main

However, that seems like a forgotten left-over as you mentioned.
So I'll follow your recommendation and remove XGMII check in v7 so that
mtk-ge-soc.c & mtk-2p5ge.c follow the same logic since these two
drivers are both for built-in ethernet phy.

Sky
> > - PHY_INTERFACE_MODE_USXGMII: external 10Gphy
> > 
> >   I add check in config_init():
> > /* Check if PHY interface type is compatible */
> > if (phydev->interface != PHY_INTERFACE_MODE_XGMII &&
> >     phydev->interface != PHY_INTERFACE_MODE_INTERNAL)
> > return -ENODEV;
> > 
> >   Also, test with different phy mode in dts:
> > [PHY_INTERFACE_MODE_USXGMII]
> > [   18.702102] mtk_soc_eth 15100000.ethernet eth1: mtk_open: could
> not
> > attach PHY: -19
> > root@OpenWrt:/# cat /proc/device-tree/soc/ethernet@15100000/mac@1/p
> hy-c
> > onnection-type
> > usxgmii
> > 
> > [PHY_INTERFACE_MODE_INTERNAL]
> > [   18.329513] mtk_soc_eth 15100000.ethernet eth1: PHY [mdio-
> bus:0f]
> > driver [MediaTek MT7988 2.5GbE PHY] (irq=POLL)
> > [   18.339708] mtk_soc_eth 15100000.ethernet eth1: configuring for
> > phy/internal link mode
> > root@OpenWrt:/# cat /proc/device-tree/soc/ethernet@15100000
> > /mac@1/phy-connection-type
> > internal
> > 
> > Sky

> Since I found out that there's phy-mode="gmii" in mt7981-rfb.dts on
> openWRT, I thought maybe we need gmii&internal for mt7981's built-in
> GbE and xgmii&internal for mt7988's built-in 2.5GbE.

Just because openwrt is using it, does not mean it is correct. We do
not break in tree DT files, but out of tree DT can be ignored.
Hopefully if mt7981-rfb.dts is ever submitted for mainline, the
problem will be fixed. And when OpenWRT updates to a newer kernel they
are likely to fix it.

    Andrew