[PATCH net-next 0/3] net: phy: mediatek: Add token-ring ops

[PATCH net-next 0/3] net: phy: mediatek: Add token-ring ops

[Sky Huang]

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

This patchset add token-ring ops and moves some macros from mtk-ge.c
into mtk-phy-lib.c. Also, MediaTek's built-in 2.5Gphy driver is added.

Signed-off-by: Sky Huang <skylake.huang@mediatek.com>
---
Sky Huang (2):
  net: phy: mediatek: Move some macros to phy-lib for later use
  net: phy: mediatek: add driver for built-in 2.5G ethernet PHY on
    MT7988

SkyLake.Huang (1):
  net: phy: mediatek: Add token ring access helper functions in
    mtk-phy-lib

 MAINTAINERS                            |   1 +
 drivers/net/phy/mediatek/Kconfig       |  11 +
 drivers/net/phy/mediatek/Makefile      |   1 +
 drivers/net/phy/mediatek/mtk-2p5ge.c   | 343 +++++++++++++++++++++++++
 drivers/net/phy/mediatek/mtk-ge-soc.c  | 297 +++++++++++++--------
 drivers/net/phy/mediatek/mtk-ge.c      |  78 ++++--
 drivers/net/phy/mediatek/mtk-phy-lib.c |  91 +++++++
 drivers/net/phy/mediatek/mtk.h         |  17 ++
 8 files changed, 714 insertions(+), 125 deletions(-)
 create mode 100644 drivers/net/phy/mediatek/mtk-2p5ge.c

-- 
2.45.2

[PATCH net-next 1/3] net: phy: mediatek: Add token ring access helper functions in mtk-phy-lib

[Sky Huang]

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

This patch adds TR(token ring) manipulations and adds correct
macro names for those magic numbers. TR is a way to access
proprietary registers on page 52b5. Use these helper functions
so we can see which fields we're going to modify/set/clear.

This patch doesn't really change registers' settings but just
enhances readability and maintainability.

Signed-off-by: SkyLake.Huang <skylake.huang@mediatek.com>
---
 drivers/net/phy/mediatek/mtk-ge-soc.c  | 297 ++++++++++++++++---------
 drivers/net/phy/mediatek/mtk-ge.c      |  82 +++++--
 drivers/net/phy/mediatek/mtk-phy-lib.c |  91 ++++++++
 drivers/net/phy/mediatek/mtk.h         |  13 ++
 4 files changed, 358 insertions(+), 125 deletions(-)

diff --git a/drivers/net/phy/mediatek/mtk-ge-soc.c b/drivers/net/phy/mediatek/mtk-ge-soc.c
index 38dc898..c1f1c79 100644
--- a/drivers/net/phy/mediatek/mtk-ge-soc.c
+++ b/drivers/net/phy/mediatek/mtk-ge-soc.c
@@ -24,7 +24,108 @@
 #define MTK_PHY_SMI_DET_ON_THRESH_MASK		GENMASK(13, 8)
 
 #define MTK_PHY_PAGE_EXTENDED_2A30		0x2a30
-#define MTK_PHY_PAGE_EXTENDED_52B5		0x52b5
+
+/* Registers on Token Ring debug nodes */
+/* ch_addr = 0x0, node_addr = 0x7, data_addr = 0x15 */
+/* NormMseLoThresh */
+#define NORMAL_MSE_LO_THRESH_MASK		GENMASK(15, 8)
+
+/* ch_addr = 0x0, node_addr = 0xf, data_addr = 0x3c */
+/* RemAckCntLimitCtrl */
+#define REMOTE_ACK_COUNT_LIMIT_CTRL_MASK	GENMASK(2, 1)
+
+/* ch_addr = 0x1, node_addr = 0xd, data_addr = 0x20 */
+/* VcoSlicerThreshBitsHigh */
+#define VCO_SLICER_THRESH_HIGH_MASK		GENMASK(23, 0)
+
+/* ch_addr = 0x1, node_addr = 0xf, data_addr = 0x0 */
+/* DfeTailEnableVgaThresh1000 */
+#define DFE_TAIL_EANBLE_VGA_TRHESH_1000		GENMASK(5, 1)
+
+/* ch_addr = 0x1, node_addr = 0xf, data_addr = 0x1 */
+/* MrvlTrFix100Kp */
+#define MRVL_TR_FIX_100KP_MASK			GENMASK(22, 20)
+/* MrvlTrFix100Kf */
+#define MRVL_TR_FIX_100KF_MASK			GENMASK(19, 17)
+/* MrvlTrFix1000Kp */
+#define MRVL_TR_FIX_1000KP_MASK			GENMASK(16, 14)
+/* MrvlTrFix1000Kf */
+#define MRVL_TR_FIX_1000KF_MASK			GENMASK(13, 11)
+
+/* ch_addr = 0x1, node_addr = 0xf, data_addr = 0x12 */
+/* VgaDecRate */
+#define VGA_DECIMATION_RATE_MASK		GENMASK(8, 5)
+
+/* ch_addr = 0x1, node_addr = 0xf, data_addr = 0x17 */
+/* SlvDSPreadyTime */
+#define SLAVE_DSP_READY_TIME_MASK		GENMASK(22, 15)
+/* MasDSPreadyTime */
+#define MASTER_DSP_READY_TIME_MASK		GENMASK(14, 7)
+
+/* ch_addr = 0x1, node_addr = 0xf, data_addr = 0x18 */
+/* EnabRandUpdTrig */
+#define ENABLE_RANDOM_UPDOWN_COUNTER_TRIGGER	BIT(8)
+
+/* ch_addr = 0x1, node_addr = 0xf, data_addr = 0x20 */
+/* ResetSyncOffset */
+#define RESET_SYNC_OFFSET_MASK			GENMASK(11, 8)
+
+/* ch_addr = 0x2, node_addr = 0xd, data_addr = 0x0 */
+/* FfeUpdGainForceVal */
+#define FFE_UPDATE_GAIN_FORCE_VAL_MASK		GENMASK(9, 7)
+/* FfeUpdGainForce */
+#define FFE_UPDATE_GAIN_FORCE			BIT(6)
+
+/* ch_addr = 0x2, node_addr = 0xd, data_addr = 0x3 */
+/* TrFreeze */
+#define TR_FREEZE_MASK				GENMASK(11, 0)
+
+/* ch_addr = 0x2, node_addr = 0xd, data_addr = 0x6 */
+/* SS: Steady-state, KP: Proportional Gain */
+/* SSTrKp100 */
+#define SS_TR_KP100_MASK			GENMASK(21, 19)
+/* SSTrKf100 */
+#define SS_TR_KF100_MASK			GENMASK(18, 16)
+/* SSTrKp1000Mas */
+#define SS_TR_KP1000_MASTER_MASK		GENMASK(15, 13)
+/* SSTrKf1000Mas */
+#define SS_TR_KF1000_MASTER_MASK		GENMASK(12, 10)
+/* SSTrKp1000Slv */
+#define SS_TR_KP1000_SLAVE_MASK			GENMASK(9, 7)
+/* SSTrKf1000Slv */
+#define SS_TR_KF1000_SLAVE_MASK			GENMASK(6, 4)
+
+/* ch_addr = 0x2, node_addr = 0xd, data_addr = 0x8 */
+/* clear this bit if wanna select from AFE */
+/* Regsigdet_sel_1000 */
+#define EEE1000_SELECT_SIGNAL_DETECTION_FROM_DFE	BIT(4)
+
+/* ch_addr = 0x2, node_addr = 0xd, data_addr = 0xd */
+/* RegEEE_st2TrKf1000 */
+#define EEE1000_STAGE2_TR_KF_MASK		GENMASK(13, 11)
+
+/* ch_addr = 0x2, node_addr = 0xd, data_addr = 0xf */
+/* RegEEE_slv_waketr_timer_tar */
+#define SLAVE_WAKETR_TIMER_MASK			GENMASK(20, 11)
+/* RegEEE_slv_remtx_timer_tar */
+#define SLAVE_REMTX_TIMER_MASK			GENMASK(10, 1)
+
+/* ch_addr = 0x2, node_addr = 0xd, data_addr = 0x10 */
+/* RegEEE_slv_wake_int_timer_tar */
+#define SLAVE_WAKEINT_TIMER_MASK		GENMASK(10, 1)
+
+/* ch_addr = 0x2, node_addr = 0xd, data_addr = 0x14 */
+/* RegEEE_trfreeze_timer2 */
+#define TR_FREEZE_TIMER2_MASK			GENMASK(9, 0)
+
+/* ch_addr = 0x2, node_addr = 0xd, data_addr = 0x1c */
+/* RegEEE100Stg1_tar */
+#define EEE100_LPSYNC_STAGE1_UPDATE_TIMER_MASK	GENMASK(8, 0)
+
+/* ch_addr = 0x2, node_addr = 0xd, data_addr = 0x25 */
+/* REGEEE_wake_slv_tr_wait_dfesigdet_en */
+#define WAKE_SLAVE_TR_WAIT_DFE_DETECTION_EN	BIT(11)
+
 
 #define ANALOG_INTERNAL_OPERATION_MAX_US	20
 #define TXRESERVE_MIN				0
@@ -701,40 +802,36 @@ static int tx_vcm_cal_sw(struct phy_device *phydev, u8 rg_txreserve_x)
 static void mt798x_phy_common_finetune(struct phy_device *phydev)
 {
 	phy_select_page(phydev, MTK_PHY_PAGE_EXTENDED_52B5);
-	/* SlvDSPreadyTime = 24, MasDSPreadyTime = 24 */
-	__phy_write(phydev, 0x11, 0xc71);
-	__phy_write(phydev, 0x12, 0xc);
-	__phy_write(phydev, 0x10, 0x8fae);
-
-	/* EnabRandUpdTrig = 1 */
-	__phy_write(phydev, 0x11, 0x2f00);
-	__phy_write(phydev, 0x12, 0xe);
-	__phy_write(phydev, 0x10, 0x8fb0);
-
-	/* NormMseLoThresh = 85 */
-	__phy_write(phydev, 0x11, 0x55a0);
-	__phy_write(phydev, 0x12, 0x0);
-	__phy_write(phydev, 0x10, 0x83aa);
-
-	/* FfeUpdGainForce = 1(Enable), FfeUpdGainForceVal = 4 */
-	__phy_write(phydev, 0x11, 0x240);
-	__phy_write(phydev, 0x12, 0x0);
-	__phy_write(phydev, 0x10, 0x9680);
-
-	/* TrFreeze = 0 (mt7988 default) */
-	__phy_write(phydev, 0x11, 0x0);
-	__phy_write(phydev, 0x12, 0x0);
-	__phy_write(phydev, 0x10, 0x9686);
-
-	/* SSTrKp100 = 5 */
-	/* SSTrKf100 = 6 */
-	/* SSTrKp1000Mas = 5 */
-	/* SSTrKf1000Mas = 6 */
-	/* SSTrKp1000Slv = 5 */
-	/* SSTrKf1000Slv = 6 */
-	__phy_write(phydev, 0x11, 0xbaef);
-	__phy_write(phydev, 0x12, 0x2e);
-	__phy_write(phydev, 0x10, 0x968c);
+	__mtk_tr_modify(phydev, 0x1, 0xf, 0x17,
+			SLAVE_DSP_READY_TIME_MASK | MASTER_DSP_READY_TIME_MASK,
+			FIELD_PREP(SLAVE_DSP_READY_TIME_MASK, 0x18) |
+			FIELD_PREP(MASTER_DSP_READY_TIME_MASK, 0x18));
+
+	__mtk_tr_set_bits(phydev, 0x1, 0xf, 0x18,
+			  ENABLE_RANDOM_UPDOWN_COUNTER_TRIGGER);
+
+	__mtk_tr_modify(phydev, 0x0, 0x7, 0x15,
+			NORMAL_MSE_LO_THRESH_MASK,
+			FIELD_PREP(NORMAL_MSE_LO_THRESH_MASK, 0x55));
+
+	__mtk_tr_modify(phydev, 0x2, 0xd, 0x0,
+			FFE_UPDATE_GAIN_FORCE_VAL_MASK,
+			FIELD_PREP(FFE_UPDATE_GAIN_FORCE_VAL_MASK, 0x4) |
+				   FFE_UPDATE_GAIN_FORCE);
+
+	__mtk_tr_clr_bits(phydev, 0x2, 0xd, 0x3, TR_FREEZE_MASK);
+
+	__mtk_tr_modify(phydev, 0x2, 0xd, 0x6,
+			SS_TR_KP100_MASK | SS_TR_KF100_MASK |
+			SS_TR_KP1000_MASTER_MASK | SS_TR_KF1000_MASTER_MASK |
+			SS_TR_KP1000_SLAVE_MASK | SS_TR_KF1000_SLAVE_MASK,
+			FIELD_PREP(SS_TR_KP100_MASK, 0x5) |
+			FIELD_PREP(SS_TR_KF100_MASK, 0x6) |
+			FIELD_PREP(SS_TR_KP1000_MASTER_MASK, 0x5) |
+			FIELD_PREP(SS_TR_KF1000_MASTER_MASK, 0x6) |
+			FIELD_PREP(SS_TR_KP1000_SLAVE_MASK, 0x5) |
+			FIELD_PREP(SS_TR_KF1000_SLAVE_MASK, 0x6));
+
 	phy_restore_page(phydev, MTK_PHY_PAGE_STANDARD, 0);
 }
 
@@ -757,27 +854,29 @@ static void mt7981_phy_finetune(struct phy_device *phydev)
 	}
 
 	phy_select_page(phydev, MTK_PHY_PAGE_EXTENDED_52B5);
-	/* ResetSyncOffset = 6 */
-	__phy_write(phydev, 0x11, 0x600);
-	__phy_write(phydev, 0x12, 0x0);
-	__phy_write(phydev, 0x10, 0x8fc0);
+	__mtk_tr_modify(phydev, 0x1, 0xf, 0x20,
+			RESET_SYNC_OFFSET_MASK,
+			FIELD_PREP(RESET_SYNC_OFFSET_MASK, 0x6));
 
-	/* VgaDecRate = 1 */
-	__phy_write(phydev, 0x11, 0x4c2a);
-	__phy_write(phydev, 0x12, 0x3e);
-	__phy_write(phydev, 0x10, 0x8fa4);
+	__mtk_tr_modify(phydev, 0x1, 0xf, 0x12,
+			VGA_DECIMATION_RATE_MASK,
+			FIELD_PREP(VGA_DECIMATION_RATE_MASK, 0x1));
 
 	/* MrvlTrFix100Kp = 3, MrvlTrFix100Kf = 2,
 	 * MrvlTrFix1000Kp = 3, MrvlTrFix1000Kf = 2
 	 */
-	__phy_write(phydev, 0x11, 0xd10a);
-	__phy_write(phydev, 0x12, 0x34);
-	__phy_write(phydev, 0x10, 0x8f82);
+	__mtk_tr_modify(phydev, 0x1, 0xf, 0x1,
+			MRVL_TR_FIX_100KP_MASK | MRVL_TR_FIX_100KF_MASK |
+			MRVL_TR_FIX_1000KP_MASK | MRVL_TR_FIX_1000KF_MASK,
+			FIELD_PREP(MRVL_TR_FIX_100KP_MASK, 0x3) |
+			FIELD_PREP(MRVL_TR_FIX_100KF_MASK, 0x2) |
+			FIELD_PREP(MRVL_TR_FIX_1000KP_MASK, 0x3) |
+			FIELD_PREP(MRVL_TR_FIX_1000KF_MASK, 0x2));
 
 	/* VcoSlicerThreshBitsHigh */
-	__phy_write(phydev, 0x11, 0x5555);
-	__phy_write(phydev, 0x12, 0x55);
-	__phy_write(phydev, 0x10, 0x8ec0);
+	__mtk_tr_modify(phydev, 0x1, 0xd, 0x20,
+			VCO_SLICER_THRESH_HIGH_MASK,
+			FIELD_PREP(VCO_SLICER_THRESH_HIGH_MASK, 0x555555));
 	phy_restore_page(phydev, MTK_PHY_PAGE_STANDARD, 0);
 
 	/* TR_OPEN_LOOP_EN = 1, lpf_x_average = 9 */
@@ -829,25 +928,23 @@ static void mt7988_phy_finetune(struct phy_device *phydev)
 	phy_write_mmd(phydev, MDIO_MMD_VEND1, MTK_PHY_RG_TX_FILTER, 0x5);
 
 	phy_select_page(phydev, MTK_PHY_PAGE_EXTENDED_52B5);
-	/* ResetSyncOffset = 5 */
-	__phy_write(phydev, 0x11, 0x500);
-	__phy_write(phydev, 0x12, 0x0);
-	__phy_write(phydev, 0x10, 0x8fc0);
+	__mtk_tr_modify(phydev, 0x1, 0xf, 0x20,
+			RESET_SYNC_OFFSET_MASK,
+			FIELD_PREP(RESET_SYNC_OFFSET_MASK, 0x5));
 
 	/* VgaDecRate is 1 at default on mt7988 */
 
-	/* MrvlTrFix100Kp = 6, MrvlTrFix100Kf = 7,
-	 * MrvlTrFix1000Kp = 6, MrvlTrFix1000Kf = 7
-	 */
-	__phy_write(phydev, 0x11, 0xb90a);
-	__phy_write(phydev, 0x12, 0x6f);
-	__phy_write(phydev, 0x10, 0x8f82);
-
-	/* RemAckCntLimitCtrl = 1 */
-	__phy_write(phydev, 0x11, 0xfbba);
-	__phy_write(phydev, 0x12, 0xc3);
-	__phy_write(phydev, 0x10, 0x87f8);
-
+	__mtk_tr_modify(phydev, 0x1, 0xf, 0x1,
+			MRVL_TR_FIX_100KP_MASK | MRVL_TR_FIX_100KF_MASK |
+			MRVL_TR_FIX_1000KP_MASK | MRVL_TR_FIX_1000KF_MASK,
+			FIELD_PREP(MRVL_TR_FIX_100KP_MASK, 0x6) |
+			FIELD_PREP(MRVL_TR_FIX_100KF_MASK, 0x7) |
+			FIELD_PREP(MRVL_TR_FIX_1000KP_MASK, 0x6) |
+			FIELD_PREP(MRVL_TR_FIX_1000KF_MASK, 0x7));
+
+	__mtk_tr_modify(phydev, 0x0, 0xf, 0x3c,
+			REMOTE_ACK_COUNT_LIMIT_CTRL_MASK,
+			FIELD_PREP(REMOTE_ACK_COUNT_LIMIT_CTRL_MASK, 0x1));
 	phy_restore_page(phydev, MTK_PHY_PAGE_STANDARD, 0);
 
 	/* TR_OPEN_LOOP_EN = 1, lpf_x_average = 10 */
@@ -923,45 +1020,37 @@ static void mt798x_phy_eee(struct phy_device *phydev)
 			 MTK_PHY_TR_READY_SKIP_AFE_WAKEUP);
 
 	phy_select_page(phydev, MTK_PHY_PAGE_EXTENDED_52B5);
-	/* Regsigdet_sel_1000 = 0 */
-	__phy_write(phydev, 0x11, 0xb);
-	__phy_write(phydev, 0x12, 0x0);
-	__phy_write(phydev, 0x10, 0x9690);
-
-	/* REG_EEE_st2TrKf1000 = 2 */
-	__phy_write(phydev, 0x11, 0x114f);
-	__phy_write(phydev, 0x12, 0x2);
-	__phy_write(phydev, 0x10, 0x969a);
-
-	/* RegEEE_slv_wake_tr_timer_tar = 6, RegEEE_slv_remtx_timer_tar = 20 */
-	__phy_write(phydev, 0x11, 0x3028);
-	__phy_write(phydev, 0x12, 0x0);
-	__phy_write(phydev, 0x10, 0x969e);
-
-	/* RegEEE_slv_wake_int_timer_tar = 8 */
-	__phy_write(phydev, 0x11, 0x5010);
-	__phy_write(phydev, 0x12, 0x0);
-	__phy_write(phydev, 0x10, 0x96a0);
-
-	/* RegEEE_trfreeze_timer2 = 586 */
-	__phy_write(phydev, 0x11, 0x24a);
-	__phy_write(phydev, 0x12, 0x0);
-	__phy_write(phydev, 0x10, 0x96a8);
-
-	/* RegEEE100Stg1_tar = 16 */
-	__phy_write(phydev, 0x11, 0x3210);
-	__phy_write(phydev, 0x12, 0x0);
-	__phy_write(phydev, 0x10, 0x96b8);
-
-	/* REGEEE_wake_slv_tr_wait_dfesigdet_en = 0 */
-	__phy_write(phydev, 0x11, 0x1463);
-	__phy_write(phydev, 0x12, 0x0);
-	__phy_write(phydev, 0x10, 0x96ca);
-
-	/* DfeTailEnableVgaThresh1000 = 27 */
-	__phy_write(phydev, 0x11, 0x36);
-	__phy_write(phydev, 0x12, 0x0);
-	__phy_write(phydev, 0x10, 0x8f80);
+	__mtk_tr_clr_bits(phydev, 0x2, 0xd, 0x8,
+			  EEE1000_SELECT_SIGNAL_DETECTION_FROM_DFE);
+
+	__mtk_tr_modify(phydev, 0x2, 0xd, 0xd,
+			EEE1000_STAGE2_TR_KF_MASK,
+			FIELD_PREP(EEE1000_STAGE2_TR_KF_MASK, 0x2));
+
+	__mtk_tr_modify(phydev, 0x2, 0xd, 0xf,
+			SLAVE_WAKETR_TIMER_MASK | SLAVE_REMTX_TIMER_MASK,
+			FIELD_PREP(SLAVE_WAKETR_TIMER_MASK, 0x6) |
+			FIELD_PREP(SLAVE_REMTX_TIMER_MASK, 0x14));
+
+	__mtk_tr_modify(phydev, 0x2, 0xd, 0x10,
+			SLAVE_WAKEINT_TIMER_MASK,
+			FIELD_PREP(SLAVE_WAKEINT_TIMER_MASK, 0x8));
+
+	__mtk_tr_modify(phydev, 0x2, 0xd, 0x14,
+			TR_FREEZE_TIMER2_MASK,
+			FIELD_PREP(TR_FREEZE_TIMER2_MASK, 0x24a));
+
+	__mtk_tr_modify(phydev, 0x2, 0xd, 0x1c,
+			EEE100_LPSYNC_STAGE1_UPDATE_TIMER_MASK,
+			FIELD_PREP(EEE100_LPSYNC_STAGE1_UPDATE_TIMER_MASK,
+				   0x10));
+
+	__mtk_tr_clr_bits(phydev, 0x2, 0xd, 0x25,
+			  WAKE_SLAVE_TR_WAIT_DFE_DETECTION_EN);
+
+	__mtk_tr_modify(phydev, 0x1, 0xf, 0x0,
+			DFE_TAIL_EANBLE_VGA_TRHESH_1000,
+			FIELD_PREP(DFE_TAIL_EANBLE_VGA_TRHESH_1000, 0x1b));
 	phy_restore_page(phydev, MTK_PHY_PAGE_STANDARD, 0);
 
 	phy_select_page(phydev, MTK_PHY_PAGE_EXTENDED_3);
diff --git a/drivers/net/phy/mediatek/mtk-ge.c b/drivers/net/phy/mediatek/mtk-ge.c
index ed2617b..79663da 100644
--- a/drivers/net/phy/mediatek/mtk-ge.c
+++ b/drivers/net/phy/mediatek/mtk-ge.c
@@ -8,31 +8,62 @@
 #define MTK_GPHY_ID_MT7530		0x03a29412
 #define MTK_GPHY_ID_MT7531		0x03a29441
 
-#define MTK_EXT_PAGE_ACCESS		0x1f
-#define MTK_PHY_PAGE_STANDARD		0x0000
-#define MTK_PHY_PAGE_EXTENDED		0x0001
-#define MTK_PHY_PAGE_EXTENDED_2		0x0002
-#define MTK_PHY_PAGE_EXTENDED_3		0x0003
-#define MTK_PHY_PAGE_EXTENDED_2A30	0x2a30
-#define MTK_PHY_PAGE_EXTENDED_52B5	0x52b5
+#define MTK_PHY_PAGE_EXTENDED_1			0x0001
+#define MTK_PHY_AUX_CTRL_AND_STATUS		0x14
+#define   MTK_PHY_ENABLE_DOWNSHIFT		BIT(4)
+
+#define MTK_PHY_PAGE_EXTENDED_2			0x0002
+#define MTK_PHY_PAGE_EXTENDED_3			0x0003
+#define MTK_PHY_RG_LPI_PCS_DSP_CTRL_REG11	0x11
+
+#define MTK_PHY_PAGE_EXTENDED_2A30		0x2a30
+
+/* Registers on Token Ring debug nodes */
+/* ch_addr = 0x1, node_addr = 0xf, data_addr = 0x17 */
+#define SLAVE_DSP_READY_TIME_MASK		GENMASK(22, 15)
+
+/* Registers on MDIO_MMD_VEND1 */
+#define MTK_PHY_GBE_MODE_TX_DELAY_SEL		0x13
+#define MTK_PHY_TEST_MODE_TX_DELAY_SEL		0x14
+#define   MTK_TX_DELAY_PAIR_B_MASK		GENMASK(10, 8)
+#define   MTK_TX_DELAY_PAIR_D_MASK		GENMASK(2, 0)
+
+#define MTK_PHY_MCC_CTRL_AND_TX_POWER_CTRL	0xa6
+#define   MTK_MCC_NEARECHO_OFFSET_MASK		GENMASK(15, 8)
+
+#define MTK_PHY_RXADC_CTRL_RG7			0xc6
+#define   MTK_PHY_DA_AD_BUF_BIAS_LP_MASK	GENMASK(9, 8)
+
+#define MTK_PHY_RG_LPI_PCS_DSP_CTRL_REG123	0x123
+#define   MTK_PHY_LPI_NORM_MSE_LO_THRESH100_MASK	GENMASK(15, 8)
+#define   MTK_PHY_LPI_NORM_MSE_HI_THRESH100_MASK	GENMASK(7, 0)
 
 static void mtk_gephy_config_init(struct phy_device *phydev)
 {
 	/* Enable HW auto downshift */
-	phy_modify_paged(phydev, MTK_PHY_PAGE_EXTENDED, 0x14, 0, BIT(4));
+	phy_modify_paged(phydev, MTK_PHY_PAGE_EXTENDED_1,
+			 MTK_PHY_AUX_CTRL_AND_STATUS,
+			 0, MTK_PHY_ENABLE_DOWNSHIFT);
 
 	/* Increase SlvDPSready time */
-	phy_select_page(phydev, MTK_PHY_PAGE_EXTENDED_52B5);
-	__phy_write(phydev, 0x10, 0xafae);
-	__phy_write(phydev, 0x12, 0x2f);
-	__phy_write(phydev, 0x10, 0x8fae);
-	phy_restore_page(phydev, MTK_PHY_PAGE_STANDARD, 0);
+	mtk_tr_modify(phydev, 0x1, 0xf, 0x17, SLAVE_DSP_READY_TIME_MASK,
+		      FIELD_PREP(SLAVE_DSP_READY_TIME_MASK, 0x5e));
 
 	/* Adjust 100_mse_threshold */
-	phy_write_mmd(phydev, MDIO_MMD_VEND1, 0x123, 0xffff);
-
-	/* Disable mcc */
-	phy_write_mmd(phydev, MDIO_MMD_VEND1, 0xa6, 0x300);
+	phy_modify_mmd(phydev, MDIO_MMD_VEND1,
+		       MTK_PHY_RG_LPI_PCS_DSP_CTRL_REG123,
+		       MTK_PHY_LPI_NORM_MSE_LO_THRESH100_MASK |
+		       MTK_PHY_LPI_NORM_MSE_HI_THRESH100_MASK,
+		       FIELD_PREP(MTK_PHY_LPI_NORM_MSE_LO_THRESH100_MASK,
+				  0xff) |
+		       FIELD_PREP(MTK_PHY_LPI_NORM_MSE_HI_THRESH100_MASK,
+				  0xff));
+
+	/* If echo time is narrower than 0x3, it will be regarded as noise */
+	phy_modify_mmd(phydev, MDIO_MMD_VEND1,
+		       MTK_PHY_MCC_CTRL_AND_TX_POWER_CTRL,
+		       MTK_MCC_NEARECHO_OFFSET_MASK,
+		       FIELD_PREP(MTK_MCC_NEARECHO_OFFSET_MASK, 0x3));
 }
 
 static int mt7530_phy_config_init(struct phy_device *phydev)
@@ -40,7 +71,8 @@ static int mt7530_phy_config_init(struct phy_device *phydev)
 	mtk_gephy_config_init(phydev);
 
 	/* Increase post_update_timer */
-	phy_write_paged(phydev, MTK_PHY_PAGE_EXTENDED_3, 0x11, 0x4b);
+	phy_write_paged(phydev, MTK_PHY_PAGE_EXTENDED_3,
+			MTK_PHY_RG_LPI_PCS_DSP_CTRL_REG11, 0x4b);
 
 	return 0;
 }
@@ -51,11 +83,19 @@ static int mt7531_phy_config_init(struct phy_device *phydev)
 
 	/* PHY link down power saving enable */
 	phy_set_bits(phydev, 0x17, BIT(4));
-	phy_clear_bits_mmd(phydev, MDIO_MMD_VEND1, 0xc6, 0x300);
+	phy_modify_mmd(phydev, MDIO_MMD_VEND1, MTK_PHY_RXADC_CTRL_RG7,
+		       MTK_PHY_DA_AD_BUF_BIAS_LP_MASK,
+		       FIELD_PREP(MTK_PHY_DA_AD_BUF_BIAS_LP_MASK, 0x3));
 
 	/* Set TX Pair delay selection */
-	phy_write_mmd(phydev, MDIO_MMD_VEND1, 0x13, 0x404);
-	phy_write_mmd(phydev, MDIO_MMD_VEND1, 0x14, 0x404);
+	phy_modify_mmd(phydev, MDIO_MMD_VEND1, MTK_PHY_GBE_MODE_TX_DELAY_SEL,
+		       MTK_TX_DELAY_PAIR_B_MASK | MTK_TX_DELAY_PAIR_D_MASK,
+		       FIELD_PREP(MTK_TX_DELAY_PAIR_B_MASK, 0x4) |
+		       FIELD_PREP(MTK_TX_DELAY_PAIR_D_MASK, 0x4));
+	phy_modify_mmd(phydev, MDIO_MMD_VEND1, MTK_PHY_TEST_MODE_TX_DELAY_SEL,
+		       MTK_TX_DELAY_PAIR_B_MASK | MTK_TX_DELAY_PAIR_D_MASK,
+		       FIELD_PREP(MTK_TX_DELAY_PAIR_B_MASK, 0x4) |
+		       FIELD_PREP(MTK_TX_DELAY_PAIR_D_MASK, 0x4));
 
 	return 0;
 }
diff --git a/drivers/net/phy/mediatek/mtk-phy-lib.c b/drivers/net/phy/mediatek/mtk-phy-lib.c
index 98a09d6..b562bf0 100644
--- a/drivers/net/phy/mediatek/mtk-phy-lib.c
+++ b/drivers/net/phy/mediatek/mtk-phy-lib.c
@@ -6,6 +6,97 @@
 
 #include "mtk.h"
 
+/* Difference between functions with mtk_tr* and __mtk_tr* prefixes is
+ * mtk_tr* functions: wrapped by page switching operations
+ * __mtk_tr* functions: no page switching operations
+ */
+
+static void __mtk_tr_access(struct phy_device *phydev, bool read, u8 ch_addr,
+			    u8 node_addr, u8 data_addr)
+{
+	u16 tr_cmd = BIT(15); /* bit 14 & 0 are reserved */
+
+	if (read)
+		tr_cmd |= BIT(13);
+
+	tr_cmd |= (((ch_addr & 0x3) << 11) |
+		   ((node_addr & 0xf) << 7) |
+		   ((data_addr & 0x3f) << 1));
+	dev_dbg(&phydev->mdio.dev, "tr_cmd: 0x%x\n", tr_cmd);
+	__phy_write(phydev, 0x10, tr_cmd);
+}
+
+static void __mtk_tr_read(struct phy_device *phydev, u8 ch_addr, u8 node_addr,
+			  u8 data_addr, u16 *tr_high, u16 *tr_low)
+{
+	__mtk_tr_access(phydev, true, ch_addr, node_addr, data_addr);
+	*tr_low = __phy_read(phydev, 0x11);
+	*tr_high = __phy_read(phydev, 0x12);
+	dev_dbg(&phydev->mdio.dev, "tr_high read: 0x%x, tr_low read: 0x%x\n",
+		*tr_high, *tr_low);
+}
+
+u32 mtk_tr_read(struct phy_device *phydev, u8 ch_addr, u8 node_addr,
+		u8 data_addr)
+{
+	u16 tr_high;
+	u16 tr_low;
+
+	phy_select_page(phydev, MTK_PHY_PAGE_EXTENDED_52B5);
+	__mtk_tr_read(phydev, ch_addr, node_addr, data_addr, &tr_high, &tr_low);
+	phy_restore_page(phydev, MTK_PHY_PAGE_STANDARD, 0);
+
+	return (tr_high << 16) | tr_low;
+}
+EXPORT_SYMBOL_GPL(mtk_tr_read);
+
+static void __mtk_tr_write(struct phy_device *phydev, u8 ch_addr, u8 node_addr,
+			   u8 data_addr, u32 tr_data)
+{
+	__phy_write(phydev, 0x11, tr_data & 0xffff);
+	__phy_write(phydev, 0x12, tr_data >> 16);
+	dev_dbg(&phydev->mdio.dev, "tr_high write: 0x%x, tr_low write: 0x%x\n",
+		tr_data >> 16, tr_data & 0xffff);
+	__mtk_tr_access(phydev, false, ch_addr, node_addr, data_addr);
+}
+
+void __mtk_tr_modify(struct phy_device *phydev, u8 ch_addr, u8 node_addr,
+		     u8 data_addr, u32 mask, u32 set)
+{
+	u32 tr_data;
+	u16 tr_high;
+	u16 tr_low;
+
+	__mtk_tr_read(phydev, ch_addr, node_addr, data_addr, &tr_high, &tr_low);
+	tr_data = (tr_high << 16) | tr_low;
+	tr_data = (tr_data & ~mask) | set;
+	__mtk_tr_write(phydev, ch_addr, node_addr, data_addr, tr_data);
+}
+EXPORT_SYMBOL_GPL(__mtk_tr_modify);
+
+void mtk_tr_modify(struct phy_device *phydev, u8 ch_addr, u8 node_addr,
+		   u8 data_addr, u32 mask, u32 set)
+{
+	phy_select_page(phydev, MTK_PHY_PAGE_EXTENDED_52B5);
+	__mtk_tr_modify(phydev, ch_addr, node_addr, data_addr, mask, set);
+	phy_restore_page(phydev, MTK_PHY_PAGE_STANDARD, 0);
+}
+EXPORT_SYMBOL_GPL(mtk_tr_modify);
+
+void __mtk_tr_set_bits(struct phy_device *phydev, u8 ch_addr, u8 node_addr,
+		       u8 data_addr, u32 set)
+{
+	__mtk_tr_modify(phydev, ch_addr, node_addr, data_addr, 0, set);
+}
+EXPORT_SYMBOL_GPL(__mtk_tr_set_bits);
+
+void __mtk_tr_clr_bits(struct phy_device *phydev, u8 ch_addr, u8 node_addr,
+		       u8 data_addr, u32 clr)
+{
+	__mtk_tr_modify(phydev, ch_addr, node_addr, data_addr, clr, 0);
+}
+EXPORT_SYMBOL_GPL(__mtk_tr_clr_bits);
+
 int mtk_phy_read_page(struct phy_device *phydev)
 {
 	return __phy_read(phydev, MTK_EXT_PAGE_ACCESS);
diff --git a/drivers/net/phy/mediatek/mtk.h b/drivers/net/phy/mediatek/mtk.h
index 63d9fe1..712a9f0 100644
--- a/drivers/net/phy/mediatek/mtk.h
+++ b/drivers/net/phy/mediatek/mtk.h
@@ -9,6 +9,8 @@
 #define _MTK_EPHY_H_
 
 #define MTK_EXT_PAGE_ACCESS			0x1f
+#define MTK_PHY_PAGE_STANDARD			0x0000
+#define MTK_PHY_PAGE_EXTENDED_52B5		0x52b5
 
 /* Registers on MDIO_MMD_VEND2 */
 #define MTK_PHY_LED0_ON_CTRL			0x24
@@ -66,6 +68,17 @@ struct mtk_socphy_priv {
 	unsigned long		led_state;
 };
 
+u32 mtk_tr_read(struct phy_device *phydev, u8 ch_addr, u8 node_addr,
+		u8 data_addr);
+void __mtk_tr_modify(struct phy_device *phydev, u8 ch_addr, u8 node_addr,
+		     u8 data_addr, u32 mask, u32 set);
+void mtk_tr_modify(struct phy_device *phydev, u8 ch_addr, u8 node_addr,
+		   u8 data_addr, u32 mask, u32 set);
+void __mtk_tr_set_bits(struct phy_device *phydev, u8 ch_addr, u8 node_addr,
+		       u8 data_addr, u32 set);
+void __mtk_tr_clr_bits(struct phy_device *phydev, u8 ch_addr, u8 node_addr,
+		       u8 data_addr, u32 clr);
+
 int mtk_phy_read_page(struct phy_device *phydev);
 int mtk_phy_write_page(struct phy_device *phydev, int page);
 
-- 
2.45.2

Re: [PATCH net-next 1/3] net: phy: mediatek: Add token ring access helper functions in mtk-phy-lib

[Krzysztof Kozlowski]

On 16/01/2025 02:21, Sky Huang wrote:
> From: "SkyLake.Huang" <skylake.huang@mediatek.com>
> 
> This patch adds TR(token ring) manipulations and adds correct
> macro names for those magic numbers. TR is a way to access
> proprietary registers on page 52b5. Use these helper functions
> so we can see which fields we're going to modify/set/clear.
> 
> This patch doesn't really change registers' settings but just
> enhances readability and maintainability.
> 
> Signed-off-by: SkyLake.Huang <skylake.huang@mediatek.com>
Few days ago I complained the Mediatek too frequent uses login as full
name. Several different Mediatek contributors repeat the same mistake,
so I asked to fix this internally with some sort of
guideline/checklist/internal reviews.

Other patches here look OK, so there is some progress, but not complete
- please fix here as well.

Best regards,
Krzysztof

Re: [PATCH net-next 1/3] net: phy: mediatek: Add token ring access helper functions in mtk-phy-lib

[Andrew Lunn]

> +/* ch_addr = 0x1, node_addr = 0xf, data_addr = 0x1 */
> +/* MrvlTrFix100Kp */
> +#define MRVL_TR_FIX_100KP_MASK			GENMASK(22, 20)
> +/* MrvlTrFix100Kf */
> +#define MRVL_TR_FIX_100KF_MASK			GENMASK(19, 17)
> +/* MrvlTrFix1000Kp */
> +#define MRVL_TR_FIX_1000KP_MASK			GENMASK(16, 14)
> +/* MrvlTrFix1000Kf */
> +#define MRVL_TR_FIX_1000KF_MASK			GENMASK(13, 11)

What does the Mrvl prefix stand for?

This patch is pretty much impossible to review because it makes so
many changes. Please split it up into lots of small simple changes.

    Andrew

---
pw-bot: cr

Re: [PATCH net-next 1/3] net: phy: mediatek: Add token ring access helper functions in mtk-phy-lib

[SkyLake Huang (黃啟澤)]

On Sun, 2025-01-19 at 18:12 +0100, Andrew Lunn wrote:
> 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> > +/* ch_addr = 0x1, node_addr = 0xf, data_addr = 0x1 */
> > +/* MrvlTrFix100Kp */
> > +#define MRVL_TR_FIX_100KP_MASK                       GENMASK(22,
> > 20)
> > +/* MrvlTrFix100Kf */
> > +#define MRVL_TR_FIX_100KF_MASK                       GENMASK(19,
> > 17)
> > +/* MrvlTrFix1000Kp */
> > +#define MRVL_TR_FIX_1000KP_MASK                      GENMASK(16,
> > 14)
> > +/* MrvlTrFix1000Kf */
> > +#define MRVL_TR_FIX_1000KF_MASK                      GENMASK(13,
> > 11)
> 
> What does the Mrvl prefix stand for?
> 
> This patch is pretty much impossible to review because it makes so
> many changes. Please split it up into lots of small simple changes.
> 
>     Andrew
> 
> ---
> pw-bot: cr
Those registers with Mrvl* prefix were originally designed for
connection with certain Marvell devices. It's our DSP parameters.

I'll split this change up in v2.

BRs,
Sky

Re: [PATCH net-next 1/3] net: phy: mediatek: Add token ring access helper functions in mtk-phy-lib

[Andrew Lunn]

On Thu, Feb 13, 2025 at 07:39:39AM +0000, SkyLake Huang (黃啟澤) wrote:
> On Sun, 2025-01-19 at 18:12 +0100, Andrew Lunn wrote:
> > 
> > External email : Please do not click links or open attachments until
> > you have verified the sender or the content.
> > 
> > 
> > > +/* ch_addr = 0x1, node_addr = 0xf, data_addr = 0x1 */
> > > +/* MrvlTrFix100Kp */
> > > +#define MRVL_TR_FIX_100KP_MASK                       GENMASK(22,
> > > 20)
> > > +/* MrvlTrFix100Kf */
> > > +#define MRVL_TR_FIX_100KF_MASK                       GENMASK(19,
> > > 17)
> > > +/* MrvlTrFix1000Kp */
> > > +#define MRVL_TR_FIX_1000KP_MASK                      GENMASK(16,
> > > 14)
> > > +/* MrvlTrFix1000Kf */
> > > +#define MRVL_TR_FIX_1000KF_MASK                      GENMASK(13,
> > > 11)
> > 
> > What does the Mrvl prefix stand for?
> > 
> > This patch is pretty much impossible to review because it makes so
> > many changes. Please split it up into lots of small simple changes.
> > 
> >     Andrew
> > 
> > ---
> > pw-bot: cr
> Those registers with Mrvl* prefix were originally designed for
> connection with certain Marvell devices. It's our DSP parameters.

Will this code work with real Marvell devices? Is this PHY actually
licensed from Marvell?

	Andrew

Re: [PATCH net-next 1/3] net: phy: mediatek: Add token ring access helper functions in mtk-phy-lib

[Daniel Golle]

On Thu, Feb 13, 2025 at 02:29:35PM +0100, Andrew Lunn wrote:
> On Thu, Feb 13, 2025 at 07:39:39AM +0000, SkyLake Huang (黃啟澤) wrote:
> > On Sun, 2025-01-19 at 18:12 +0100, Andrew Lunn wrote:
> > > 
> > > External email : Please do not click links or open attachments until
> > > you have verified the sender or the content.
> > > 
> > > 
> > > > +/* ch_addr = 0x1, node_addr = 0xf, data_addr = 0x1 */
> > > > +/* MrvlTrFix100Kp */
> > > > +#define MRVL_TR_FIX_100KP_MASK                       GENMASK(22,
> > > > 20)
> > > > +/* MrvlTrFix100Kf */
> > > > +#define MRVL_TR_FIX_100KF_MASK                       GENMASK(19,
> > > > 17)
> > > > +/* MrvlTrFix1000Kp */
> > > > +#define MRVL_TR_FIX_1000KP_MASK                      GENMASK(16,
> > > > 14)
> > > > +/* MrvlTrFix1000Kf */
> > > > +#define MRVL_TR_FIX_1000KF_MASK                      GENMASK(13,
> > > > 11)
> > > 
> > > What does the Mrvl prefix stand for?
> > > 
> > > This patch is pretty much impossible to review because it makes so
> > > many changes. Please split it up into lots of small simple changes.
> > > 
> > >     Andrew
> > > 
> > > ---
> > > pw-bot: cr
> > Those registers with Mrvl* prefix were originally designed for
> > connection with certain Marvell devices. It's our DSP parameters.
> 
> Will this code work with real Marvell devices? Is this PHY actually
> licensed from Marvell?

From what I understood the tuning of those parameters is required
to connect to a Marvell PHY link partner on the other end.

Re: [PATCH net-next 1/3] net: phy: mediatek: Add token ring access helper functions in mtk-phy-lib

[Andrew Lunn]

> > > Those registers with Mrvl* prefix were originally designed for
> > > connection with certain Marvell devices. It's our DSP parameters.
> > 
> > Will this code work with real Marvell devices? Is this PHY actually
> > licensed from Marvell?
> 
> >From what I understood the tuning of those parameters is required
> to connect to a Marvell PHY link partner on the other end.

If so, the naming is bad. I assume you need the same settings for
Microchip, Atheros, Broadcom, etc. These settings just tune the
hardware to be standards conforming?

	Andrew

Re: [PATCH net-next 1/3] net: phy: mediatek: Add token ring access helper functions in mtk-phy-lib

[SkyLake Huang (黃啟澤)]

On Thu, 2025-02-13 at 16:34 +0100, Andrew Lunn wrote:
> 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> > > > Those registers with Mrvl* prefix were originally designed for
> > > > connection with certain Marvell devices. It's our DSP
> > > > parameters.
> > > 
> > > Will this code work with real Marvell devices? Is this PHY
> > > actually
> > > licensed from Marvell?
> > 
> > > From what I understood the tuning of those parameters is required
> > to connect to a Marvell PHY link partner on the other end.
> 
> If so, the naming is bad. I assume you need the same settings for
> Microchip, Atheros, Broadcom, etc. These settings just tune the
> hardware to be standards conforming?
> 
>         Andrew
> 
This part is pretty old old old design. Some compatibility issues were
firstly found on Marvell link partners, so the registers are named
accordingly. And yes, now, those Kf/Kp settings will be used for
connection with other link partners. However, if I change the register
names, it violates our hardware register map application note. If this
does bother, I can add some comments before these macros like:

/* Mrvl* prefix only means that in the very beginning we tune the
 * parameters with Marvell link partners. These settings will be used
 * for all link partners now.
 */

BRs,
Sky

Re: [PATCH net-next 1/3] net: phy: mediatek: Add token ring access helper functions in mtk-phy-lib

[Andrew Lunn]

On Fri, Feb 14, 2025 at 03:35:10AM +0000, SkyLake Huang (黃啟澤) wrote:
> On Thu, 2025-02-13 at 16:34 +0100, Andrew Lunn wrote:
> > 
> > External email : Please do not click links or open attachments until
> > you have verified the sender or the content.
> > 
> > 
> > > > > Those registers with Mrvl* prefix were originally designed for
> > > > > connection with certain Marvell devices. It's our DSP
> > > > > parameters.
> > > > 
> > > > Will this code work with real Marvell devices? Is this PHY
> > > > actually
> > > > licensed from Marvell?
> > > 
> > > > From what I understood the tuning of those parameters is required
> > > to connect to a Marvell PHY link partner on the other end.
> > 
> > If so, the naming is bad. I assume you need the same settings for
> > Microchip, Atheros, Broadcom, etc. These settings just tune the
> > hardware to be standards conforming?
> > 
> >         Andrew
> > 
> This part is pretty old old old design. Some compatibility issues were
> firstly found on Marvell link partners, so the registers are named
> accordingly. And yes, now, those Kf/Kp settings will be used for
> connection with other link partners. However, if I change the register
> names, it violates our hardware register map application note. If this
> does bother, I can add some comments before these macros like:
> 
> /* Mrvl* prefix only means that in the very beginning we tune the
>  * parameters with Marvell link partners. These settings will be used
>  * for all link partners now.
>  */

A comment would be good. The current names lead the questions....

	Andrew

[PATCH net-next 2/3] net: phy: mediatek: Move some macros to phy-lib for later use

[Sky Huang]

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

Move some macros to phy-lib because MediaTek's 2.5G built-in
ethernet PHY will also use them.

Signed-off-by: Sky Huang <skylake.huang@mediatek.com>
---
 drivers/net/phy/mediatek/mtk-ge.c | 4 ----
 drivers/net/phy/mediatek/mtk.h    | 4 ++++
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/phy/mediatek/mtk-ge.c b/drivers/net/phy/mediatek/mtk-ge.c
index 79663da..937254a 100644
--- a/drivers/net/phy/mediatek/mtk-ge.c
+++ b/drivers/net/phy/mediatek/mtk-ge.c
@@ -8,10 +8,6 @@
 #define MTK_GPHY_ID_MT7530		0x03a29412
 #define MTK_GPHY_ID_MT7531		0x03a29441
 
-#define MTK_PHY_PAGE_EXTENDED_1			0x0001
-#define MTK_PHY_AUX_CTRL_AND_STATUS		0x14
-#define   MTK_PHY_ENABLE_DOWNSHIFT		BIT(4)
-
 #define MTK_PHY_PAGE_EXTENDED_2			0x0002
 #define MTK_PHY_PAGE_EXTENDED_3			0x0003
 #define MTK_PHY_RG_LPI_PCS_DSP_CTRL_REG11	0x11
diff --git a/drivers/net/phy/mediatek/mtk.h b/drivers/net/phy/mediatek/mtk.h
index 712a9f0..cda1dc8 100644
--- a/drivers/net/phy/mediatek/mtk.h
+++ b/drivers/net/phy/mediatek/mtk.h
@@ -8,7 +8,11 @@
 #ifndef _MTK_EPHY_H_
 #define _MTK_EPHY_H_
 
+#define MTK_PHY_AUX_CTRL_AND_STATUS		0x14
+#define   MTK_PHY_ENABLE_DOWNSHIFT		BIT(4)
+
 #define MTK_EXT_PAGE_ACCESS			0x1f
+#define MTK_PHY_PAGE_EXTENDED_1			0x0001
 #define MTK_PHY_PAGE_STANDARD			0x0000
 #define MTK_PHY_PAGE_EXTENDED_52B5		0x52b5
 
-- 
2.45.2

Re: [PATCH net-next 2/3] net: phy: mediatek: Move some macros to phy-lib for later use

[Andrew Lunn]

On Thu, Jan 16, 2025 at 09:21:57AM +0800, Sky Huang wrote:
> From: Sky Huang <skylake.huang@mediatek.com>
> 
> Move some macros to phy-lib because MediaTek's 2.5G built-in
> ethernet PHY will also use them.
> 
> Signed-off-by: Sky Huang <skylake.huang@mediatek.com>
> ---
>  drivers/net/phy/mediatek/mtk-ge.c | 4 ----
>  drivers/net/phy/mediatek/mtk.h    | 4 ++++
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/phy/mediatek/mtk-ge.c b/drivers/net/phy/mediatek/mtk-ge.c
> index 79663da..937254a 100644
> --- a/drivers/net/phy/mediatek/mtk-ge.c
> +++ b/drivers/net/phy/mediatek/mtk-ge.c
> @@ -8,10 +8,6 @@
>  #define MTK_GPHY_ID_MT7530		0x03a29412
>  #define MTK_GPHY_ID_MT7531		0x03a29441
>  
> -#define MTK_PHY_PAGE_EXTENDED_1			0x0001
> -#define MTK_PHY_AUX_CTRL_AND_STATUS		0x14
> -#define   MTK_PHY_ENABLE_DOWNSHIFT		BIT(4)
> -
>  #define MTK_PHY_PAGE_EXTENDED_2			0x0002
>  #define MTK_PHY_PAGE_EXTENDED_3			0x0003
>  #define MTK_PHY_RG_LPI_PCS_DSP_CTRL_REG11	0x11
> diff --git a/drivers/net/phy/mediatek/mtk.h b/drivers/net/phy/mediatek/mtk.h
> index 712a9f0..cda1dc8 100644
> --- a/drivers/net/phy/mediatek/mtk.h
> +++ b/drivers/net/phy/mediatek/mtk.h
> @@ -8,7 +8,11 @@
>  #ifndef _MTK_EPHY_H_
>  #define _MTK_EPHY_H_
>  
> +#define MTK_PHY_AUX_CTRL_AND_STATUS		0x14
> +#define   MTK_PHY_ENABLE_DOWNSHIFT		BIT(4)
> +
>  #define MTK_EXT_PAGE_ACCESS			0x1f
> +#define MTK_PHY_PAGE_EXTENDED_1			0x0001
>  #define MTK_PHY_PAGE_STANDARD			0x0000
>  #define MTK_PHY_PAGE_EXTENDED_52B5		0x52b5

A patch like this i can easily review. I can see somethings removed
from one file and added to another, without any changes. It is
obviously correct, and toke about 10 seconds to review.

Another way to look at this. Say somebody reports your previous patch
breaks their system. How are you going to debug it? If rather than
being one huge patch, it was 10 simpler patches, you could ask them to
run a git bisect. That will narrow down the code where you need to
look for the bug to 1/10 of the code. And that one patch will
hopefully be a lot simpler, making the code inspection to find the
problem a lot simpler....

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

[PATCH net-next 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 | 343 +++++++++++++++++++++++++++
 4 files changed, 356 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 2a8ac5a..02d0c21 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..d061dc9
--- /dev/null
+++ b/drivers/net/phy/mediatek/mtk-2p5ge.c
@@ -0,0 +1,343 @@
+// 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)
+
+#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;
+};
+
+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, 0x800e, 0x1100);
+	phy_write_mmd(phydev, MDIO_MMD_VEND1, 0x800f, 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 3/3] net: phy: mediatek: add driver for built-in 2.5G ethernet PHY on MT7988

[Daniel Golle]

Hi Sky,

On Thu, Jan 16, 2025 at 09:21:58AM +0800, Sky Huang wrote:
> 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 | 343 +++++++++++++++++++++++++++
>  4 files changed, 356 insertions(+)
>  create mode 100644 drivers/net/phy/mediatek/mtk-2p5ge.c
> 
> [...]
> +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);

Calling mtk_phy_leds_state_init() can't work without also defining
led_hw_control_get() for that driver.

This is what mtk_phy_leds_state_init() does:
        for (i = 0; i < 2; ++i)
                phydev->drv->led_hw_control_get(phydev, i, NULL);

The driver lacking led_hw_control_get() method (see below) will make
this a call to a NULL function pointer.

Imho it's fine to add the driver without support for the LEDs for now
and add LED support later on. But in that case you also shouldn't call
mtk_phy_leds_state_init().

> +
> +	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 3/3] net: phy: mediatek: add driver for built-in 2.5G ethernet PHY on MT7988

[Andrew Lunn]

> Imho it's fine to add the driver without support for the LEDs for now
> and add LED support later on. But in that case you also shouldn't call
> mtk_phy_leds_state_init().

It is also O.K. to hard code the default LED meanings, different to
the reset defaults. But you need to think ahead to when you do add
full LED support, you want the defaults to be something which the LED
subsystem can represent and take over when it controls the LEDs.

	Andrew

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

[SkyLake Huang (黃啟澤)]

On Thu, 2025-01-16 at 01:58 +0000, Daniel Golle wrote:
> 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> Hi Sky,
> 
> On Thu, Jan 16, 2025 at 09:21:58AM +0800, Sky Huang wrote:
> > 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 | 343
> > +++++++++++++++++++++++++++
> >  4 files changed, 356 insertions(+)
> >  create mode 100644 drivers/net/phy/mediatek/mtk-2p5ge.c
> > 
> > [...]
> > +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);
> 
> Calling mtk_phy_leds_state_init() can't work without also defining
> led_hw_control_get() for that driver.
> 
> This is what mtk_phy_leds_state_init() does:
>         for (i = 0; i < 2; ++i)
>                 phydev->drv->led_hw_control_get(phydev, i, NULL);
> 
> The driver lacking led_hw_control_get() method (see below) will make
> this a call to a NULL function pointer.
> 
> Imho it's fine to add the driver without support for the LEDs for now
> and add LED support later on. But in that case you also shouldn't
> call
> mtk_phy_leds_state_init().

Hi Danial,
  Thanks. I missed this. I'll remove mtk_phy_leds_state_init()
temporarily and submit it with LED support later.

BRs,
Sky

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

[Krzysztof Kozlowski]

On 16/01/2025 02:21, Sky Huang wrote:
> +
> +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");

There is no such compatible. You cannot just add undocumented bindings.

Also, devices should not just look for some random compatibles. Express
proper relationships with phandles or node hierarchy.

> +	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;
> +	}
> +
Best regards,
Krzysztof

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

[SkyLake Huang (黃啟澤)]

On Thu, 2025-01-16 at 13:45 +0100, Krzysztof Kozlowski wrote:
> 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> On 16/01/2025 02:21, Sky Huang wrote:
> > +
> > +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");
> 
> There is no such compatible. You cannot just add undocumented
> bindings.
> 
> Also, devices should not just look for some random compatibles.
> Express
> proper relationships with phandles or node hierarchy.
> 
Hi Krzysztof,
  OK. I'll add dt-bindings' document in next version.

BR,
Sky
> 

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

[Krzysztof Kozlowski]

On 14/02/2025 14:23, SkyLake Huang (黃啟澤) wrote:
> On Thu, 2025-01-16 at 13:45 +0100, Krzysztof Kozlowski wrote:
>>
>> External email : Please do not click links or open attachments until
>> you have verified the sender or the content.
>>
>>
>> On 16/01/2025 02:21, Sky Huang wrote:
>>> +
>>> +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");
>>
>> There is no such compatible. You cannot just add undocumented
>> bindings.
>>
>> Also, devices should not just look for some random compatibles.
>> Express
>> proper relationships with phandles or node hierarchy.
>>
> Hi Krzysztof,
>   OK. I'll add dt-bindings' document in next version.

Carefully read the comment instead. Anyway, if you come with response to
the comment after one month (nice disproportion between received review
and responding to feedback), my entire context is gone.

Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

On 16/01/2025 02:21, Sky Huang wrote:
> 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 | 343 +++++++++++++++++++++++++++
>  4 files changed, 356 insertions(+)
>  create mode 100644 drivers/net/phy/mediatek/mtk-2p5ge.c

I don't understand this. You already sent it half a year ago. You
received detailed review, but you now send again the same? Till we accept?

Doing same review twice is not acceptable. Ignoring the review either.

NAK.

Implement all the changes you were asked for, then send a next version
(v2) with proper, detailed, exhaustive changelog.

Best regards,
Krzysztof

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

[Andrew Lunn]

> +	np = of_find_compatible_node(NULL, NULL, "mediatek,2p5gphy-fw");
> +	if (!np)
> +		return -ENOENT;

The device tree binding need documenting.

> +	/* 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);

0x1100 and 0x00df are magic numbers, bit 0x800e and 0x800f are
not. Please add #defines.


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

We generally don't spam the log for "Happy Days" conditions. Please
only log if firmware download fails.

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

So it can do 10BaseT_Half? What about 1000BaseT_Half?

As you said somewhere, 10/100/1G are not in the C45 space. So does
genphy_c45_pma_read_abilities() report these features?

	Andrew

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

[SkyLake Huang (黃啟澤)]

On Sun, 2025-01-19 at 18:31 +0100, Andrew Lunn wrote:
> 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> > +     np = of_find_compatible_node(NULL, NULL, "mediatek,2p5gphy-
> > fw");
> > +     if (!np)
> > +             return -ENOENT;
> 
> The device tree binding need documenting.
> 
> > +     /* 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);
> 
> 0x1100 and 0x00df are magic numbers, bit 0x800e and 0x800f are
> not. Please add #defines.
> 
> 
> > +
> > +     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");
> 
> We generally don't spam the log for "Happy Days" conditions. Please
> only log if firmware download fails.
> 
Thanks. I'll fix all the above.

> > +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);
> 
> So it can do 10BaseT_Half? What about 1000BaseT_Half?
> 
> As you said somewhere, 10/100/1G are not in the C45 space. So does
> genphy_c45_pma_read_abilities() report these features?
> 
>         Andrew
Nope. It can neither do 10BaseT_Half nor 1000BaseT_Half. 10 & 1000
Base_Half bits are already cleared in firmware.

BRs,
Sky