* [PATCH net-next v3 1/5] net: phy: mediatek: Re-organize MediaTek ethernet phy drivers
2024-05-20 11:34 [PATCH net-next v3 0/5] net: phy: mediatek: Introduce mtk-phy-lib and add 2.5Gphy support Sky Huang
@ 2024-05-20 11:34 ` Sky Huang
2024-05-20 13:08 ` Andrew Lunn
2024-05-20 11:34 ` [PATCH net-next v3 2/5] net: phy: mediatek: Move LED and read/write page helper functions into mtk phy lib Sky Huang
` (5 subsequent siblings)
6 siblings, 1 reply; 19+ messages in thread
From: Sky Huang @ 2024-05-20 11:34 UTC (permalink / raw)
To: Andrew Lunn, Heiner Kallweit, Russell King, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Daniel Golle,
Qingfang Deng, SkyLake Huang, Matthias Brugger,
AngeloGioacchino Del Regno, linux-kernel, netdev,
linux-arm-kernel, linux-mediatek
Cc: Steven Liu, SkyLake.Huang
From: "SkyLake.Huang" <skylake.huang@mediatek.com>
Re-organize MediaTek ethernet phy driver files and get ready to integrate
some common functions and add new 2.5G phy driver.
mtk-ge.c: MT7530 Gphy on MT7621 & MT7531 Gphy
mtk-ge-soc.c: Built-in Gphy on MT7981 & Built-in switch Gphy on MT7988
mtk-2p5ge.c: Planned for built-in 2.5G phy on MT7988
Signed-off-by: SkyLake.Huang <skylake.huang@mediatek.com>
---
MAINTAINERS | 4 ++--
drivers/net/phy/Kconfig | 17 +-------------
drivers/net/phy/Makefile | 3 +--
drivers/net/phy/mediatek/Kconfig | 22 +++++++++++++++++++
drivers/net/phy/mediatek/Makefile | 3 +++
.../mtk-ge-soc.c} | 0
.../phy/{mediatek-ge.c => mediatek/mtk-ge.c} | 0
7 files changed, 29 insertions(+), 20 deletions(-)
create mode 100644 drivers/net/phy/mediatek/Kconfig
create mode 100644 drivers/net/phy/mediatek/Makefile
rename drivers/net/phy/{mediatek-ge-soc.c => mediatek/mtk-ge-soc.c} (100%)
rename drivers/net/phy/{mediatek-ge.c => mediatek/mtk-ge.c} (100%)
diff --git a/MAINTAINERS b/MAINTAINERS
index 3fdc3b09c171..b5313673520e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -13959,8 +13959,8 @@ 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-ge-soc.c
-F: drivers/net/phy/mediatek-ge.c
+F: drivers/net/phy/mediatek/mtk-ge-soc.c
+F: drivers/net/phy/mediatek/mtk-ge.c
MEDIATEK I2C CONTROLLER DRIVER
M: Qii Wang <qii.wang@mediatek.com>
diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index 7fddc8306d82..c5ff747bb277 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -256,22 +256,7 @@ config MAXLINEAR_GPHY
Support for the Maxlinear GPY115, GPY211, GPY212, GPY215,
GPY241, GPY245 PHYs.
-config MEDIATEK_GE_PHY
- tristate "MediaTek Gigabit Ethernet PHYs"
- help
- Supports the MediaTek Gigabit Ethernet PHYs.
-
-config MEDIATEK_GE_SOC_PHY
- tristate "MediaTek SoC Ethernet PHYs"
- depends on (ARM64 && ARCH_MEDIATEK) || COMPILE_TEST
- depends on NVMEM_MTK_EFUSE
- help
- Supports MediaTek SoC built-in Gigabit Ethernet PHYs.
-
- Include support for built-in Ethernet PHYs which are present in
- 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.
+source "drivers/net/phy/mediatek/Kconfig"
config MICREL_PHY
tristate "Micrel PHYs"
diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
index 202ed7f450da..a210cf6518d2 100644
--- a/drivers/net/phy/Makefile
+++ b/drivers/net/phy/Makefile
@@ -72,8 +72,7 @@ obj-$(CONFIG_MARVELL_PHY) += marvell.o
obj-$(CONFIG_MARVELL_88Q2XXX_PHY) += marvell-88q2xxx.o
obj-$(CONFIG_MARVELL_88X2222_PHY) += marvell-88x2222.o
obj-$(CONFIG_MAXLINEAR_GPHY) += mxl-gpy.o
-obj-$(CONFIG_MEDIATEK_GE_PHY) += mediatek-ge.o
-obj-$(CONFIG_MEDIATEK_GE_SOC_PHY) += mediatek-ge-soc.o
+obj-y += mediatek/
obj-$(CONFIG_MESON_GXL_PHY) += meson-gxl.o
obj-$(CONFIG_MICREL_KS8995MA) += spi_ks8995.o
obj-$(CONFIG_MICREL_PHY) += micrel.o
diff --git a/drivers/net/phy/mediatek/Kconfig b/drivers/net/phy/mediatek/Kconfig
new file mode 100644
index 000000000000..2fa3a786720d
--- /dev/null
+++ b/drivers/net/phy/mediatek/Kconfig
@@ -0,0 +1,22 @@
+# SPDX-License-Identifier: GPL-2.0-only
+config MEDIATEK_GE_PHY
+ tristate "MediaTek Gigabit Ethernet PHYs"
+ help
+ Supports the MediaTek non-built-in Gigabit Ethernet PHYs.
+
+ Non-built-in Gigabit Ethernet PHYs include mt7530/mt7531.
+ You may find mt7530 inside mt7621. This driver shares some
+ common operations with MediaTek SoC built-in Gigabit
+ Ethernet PHYs.
+
+config MEDIATEK_GE_SOC_PHY
+ bool "MediaTek SoC Ethernet PHYs"
+ depends on (ARM64 && ARCH_MEDIATEK) || COMPILE_TEST
+ select NVMEM_MTK_EFUSE
+ help
+ Supports MediaTek SoC built-in Gigabit Ethernet PHYs.
+
+ Include support for built-in Ethernet PHYs which are present in
+ 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.
diff --git a/drivers/net/phy/mediatek/Makefile b/drivers/net/phy/mediatek/Makefile
new file mode 100644
index 000000000000..005bde26c1d7
--- /dev/null
+++ b/drivers/net/phy/mediatek/Makefile
@@ -0,0 +1,3 @@
+# SPDX-License-Identifier: GPL-2.0
+obj-$(CONFIG_MEDIATEK_GE_PHY) += mtk-ge.o
+obj-$(CONFIG_MEDIATEK_GE_SOC_PHY) += mtk-ge-soc.o
diff --git a/drivers/net/phy/mediatek-ge-soc.c b/drivers/net/phy/mediatek/mtk-ge-soc.c
similarity index 100%
rename from drivers/net/phy/mediatek-ge-soc.c
rename to drivers/net/phy/mediatek/mtk-ge-soc.c
diff --git a/drivers/net/phy/mediatek-ge.c b/drivers/net/phy/mediatek/mtk-ge.c
similarity index 100%
rename from drivers/net/phy/mediatek-ge.c
rename to drivers/net/phy/mediatek/mtk-ge.c
--
2.18.0
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH net-next v3 1/5] net: phy: mediatek: Re-organize MediaTek ethernet phy drivers
2024-05-20 11:34 ` [PATCH net-next v3 1/5] net: phy: mediatek: Re-organize MediaTek ethernet phy drivers Sky Huang
@ 2024-05-20 13:08 ` Andrew Lunn
0 siblings, 0 replies; 19+ messages in thread
From: Andrew Lunn @ 2024-05-20 13:08 UTC (permalink / raw)
To: Sky Huang
Cc: Heiner Kallweit, Russell King, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Daniel Golle, Qingfang Deng,
Matthias Brugger, AngeloGioacchino Del Regno, linux-kernel,
netdev, linux-arm-kernel, linux-mediatek, Steven Liu
On Mon, May 20, 2024 at 07:34:52PM +0800, Sky Huang wrote:
> From: "SkyLake.Huang" <skylake.huang@mediatek.com>
>
> Re-organize MediaTek ethernet phy driver files and get ready to integrate
> some common functions and add new 2.5G phy driver.
> mtk-ge.c: MT7530 Gphy on MT7621 & MT7531 Gphy
> mtk-ge-soc.c: Built-in Gphy on MT7981 & Built-in switch Gphy on MT7988
> mtk-2p5ge.c: Planned for built-in 2.5G phy on MT7988
>
> Signed-off-by: SkyLake.Huang <skylake.huang@mediatek.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH net-next v3 2/5] net: phy: mediatek: Move LED and read/write page helper functions into mtk phy lib
2024-05-20 11:34 [PATCH net-next v3 0/5] net: phy: mediatek: Introduce mtk-phy-lib and add 2.5Gphy support Sky Huang
2024-05-20 11:34 ` [PATCH net-next v3 1/5] net: phy: mediatek: Re-organize MediaTek ethernet phy drivers Sky Huang
@ 2024-05-20 11:34 ` Sky Huang
2024-05-20 11:34 ` [PATCH net-next v3 3/5] net: phy: mediatek: Add token ring access helper functions in mtk-phy-lib Sky Huang
` (4 subsequent siblings)
6 siblings, 0 replies; 19+ messages in thread
From: Sky Huang @ 2024-05-20 11:34 UTC (permalink / raw)
To: Andrew Lunn, Heiner Kallweit, Russell King, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Daniel Golle,
Qingfang Deng, SkyLake Huang, Matthias Brugger,
AngeloGioacchino Del Regno, linux-kernel, netdev,
linux-arm-kernel, linux-mediatek
Cc: Steven Liu, SkyLake.Huang
From: "SkyLake.Huang" <skylake.huang@mediatek.com>
This patch moves mtk-ge-soc.c's LED code into mtk phy lib. We
can use those helper functions in mtk-ge.c as well. That is to
say, we have almost the same HW LED controller design in
mt7530/mt7531/mt7981/mt7988's Giga ethernet phy.
Also integrate read/write pages into one helper function. They
are basically the same.
Signed-off-by: SkyLake.Huang <skylake.huang@mediatek.com>
---
MAINTAINERS | 2 +
drivers/net/phy/mediatek/Kconfig | 5 +
drivers/net/phy/mediatek/Makefile | 1 +
drivers/net/phy/mediatek/mtk-ge-soc.c | 267 +++----------------------
drivers/net/phy/mediatek/mtk-ge.c | 128 ++++++++++--
drivers/net/phy/mediatek/mtk-phy-lib.c | 230 +++++++++++++++++++++
drivers/net/phy/mediatek/mtk.h | 82 ++++++++
7 files changed, 455 insertions(+), 260 deletions(-)
create mode 100644 drivers/net/phy/mediatek/mtk-phy-lib.c
create mode 100644 drivers/net/phy/mediatek/mtk.h
diff --git a/MAINTAINERS b/MAINTAINERS
index b5313673520e..37e36b2f246e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -13960,7 +13960,9 @@ M: SkyLake Huang <SkyLake.Huang@mediatek.com>
L: netdev@vger.kernel.org
S: Maintained
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
+F: drivers/net/phy/mediatek/mtk.h
MEDIATEK I2C CONTROLLER DRIVER
M: Qii Wang <qii.wang@mediatek.com>
diff --git a/drivers/net/phy/mediatek/Kconfig b/drivers/net/phy/mediatek/Kconfig
index 2fa3a786720d..b11b19523ff1 100644
--- a/drivers/net/phy/mediatek/Kconfig
+++ b/drivers/net/phy/mediatek/Kconfig
@@ -1,6 +1,10 @@
# SPDX-License-Identifier: GPL-2.0-only
+config MTK_NET_PHYLIB
+ tristate
+
config MEDIATEK_GE_PHY
tristate "MediaTek Gigabit Ethernet PHYs"
+ select MTK_NET_PHYLIB
help
Supports the MediaTek non-built-in Gigabit Ethernet PHYs.
@@ -13,6 +17,7 @@ config MEDIATEK_GE_SOC_PHY
bool "MediaTek SoC Ethernet PHYs"
depends on (ARM64 && ARCH_MEDIATEK) || COMPILE_TEST
select NVMEM_MTK_EFUSE
+ select MTK_NET_PHYLIB
help
Supports MediaTek SoC built-in Gigabit Ethernet PHYs.
diff --git a/drivers/net/phy/mediatek/Makefile b/drivers/net/phy/mediatek/Makefile
index 005bde26c1d7..814879d0abe5 100644
--- a/drivers/net/phy/mediatek/Makefile
+++ b/drivers/net/phy/mediatek/Makefile
@@ -1,3 +1,4 @@
# SPDX-License-Identifier: GPL-2.0
+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
diff --git a/drivers/net/phy/mediatek/mtk-ge-soc.c b/drivers/net/phy/mediatek/mtk-ge-soc.c
index f4f9412d0cd7..534d4e4f5e1f 100644
--- a/drivers/net/phy/mediatek/mtk-ge-soc.c
+++ b/drivers/net/phy/mediatek/mtk-ge-soc.c
@@ -8,6 +8,8 @@
#include <linux/phy.h>
#include <linux/regmap.h>
+#include "mtk.h"
+
#define MTK_GPHY_ID_MT7981 0x03a29461
#define MTK_GPHY_ID_MT7988 0x03a29481
@@ -210,41 +212,6 @@
#define MTK_PHY_DA_TX_R50_PAIR_D 0x540
/* Registers on MDIO_MMD_VEND2 */
-#define MTK_PHY_LED0_ON_CTRL 0x24
-#define MTK_PHY_LED1_ON_CTRL 0x26
-#define MTK_PHY_LED_ON_MASK GENMASK(6, 0)
-#define MTK_PHY_LED_ON_LINK1000 BIT(0)
-#define MTK_PHY_LED_ON_LINK100 BIT(1)
-#define MTK_PHY_LED_ON_LINK10 BIT(2)
-#define MTK_PHY_LED_ON_LINK (MTK_PHY_LED_ON_LINK10 |\
- MTK_PHY_LED_ON_LINK100 |\
- MTK_PHY_LED_ON_LINK1000)
-#define MTK_PHY_LED_ON_LINKDOWN BIT(3)
-#define MTK_PHY_LED_ON_FDX BIT(4) /* Full duplex */
-#define MTK_PHY_LED_ON_HDX BIT(5) /* Half duplex */
-#define MTK_PHY_LED_ON_FORCE_ON BIT(6)
-#define MTK_PHY_LED_ON_POLARITY BIT(14)
-#define MTK_PHY_LED_ON_ENABLE BIT(15)
-
-#define MTK_PHY_LED0_BLINK_CTRL 0x25
-#define MTK_PHY_LED1_BLINK_CTRL 0x27
-#define MTK_PHY_LED_BLINK_1000TX BIT(0)
-#define MTK_PHY_LED_BLINK_1000RX BIT(1)
-#define MTK_PHY_LED_BLINK_100TX BIT(2)
-#define MTK_PHY_LED_BLINK_100RX BIT(3)
-#define MTK_PHY_LED_BLINK_10TX BIT(4)
-#define MTK_PHY_LED_BLINK_10RX BIT(5)
-#define MTK_PHY_LED_BLINK_RX (MTK_PHY_LED_BLINK_10RX |\
- MTK_PHY_LED_BLINK_100RX |\
- MTK_PHY_LED_BLINK_1000RX)
-#define MTK_PHY_LED_BLINK_TX (MTK_PHY_LED_BLINK_10TX |\
- MTK_PHY_LED_BLINK_100TX |\
- MTK_PHY_LED_BLINK_1000TX)
-#define MTK_PHY_LED_BLINK_COLLISION BIT(6)
-#define MTK_PHY_LED_BLINK_RX_CRC_ERR BIT(7)
-#define MTK_PHY_LED_BLINK_RX_IDLE_ERR BIT(8)
-#define MTK_PHY_LED_BLINK_FORCE_BLINK BIT(9)
-
#define MTK_PHY_LED1_DEFAULT_POLARITIES BIT(1)
#define MTK_PHY_RG_BG_RASEL 0x115
@@ -299,10 +266,6 @@ enum CAL_MODE {
SW_M
};
-#define MTK_PHY_LED_STATE_FORCE_ON 0
-#define MTK_PHY_LED_STATE_FORCE_BLINK 1
-#define MTK_PHY_LED_STATE_NETDEV 2
-
struct mtk_socphy_priv {
unsigned long led_state;
};
@@ -312,16 +275,6 @@ struct mtk_socphy_shared {
struct mtk_socphy_priv priv[4];
};
-static int mtk_socphy_read_page(struct phy_device *phydev)
-{
- return __phy_read(phydev, MTK_EXT_PAGE_ACCESS);
-}
-
-static int mtk_socphy_write_page(struct phy_device *phydev, int page)
-{
- return __phy_write(phydev, MTK_EXT_PAGE_ACCESS, page);
-}
-
/* One calibration cycle consists of:
* 1.Set DA_CALIN_FLAG high to start calibration. Keep it high
* until AD_CAL_COMP is ready to output calibration result.
@@ -1130,57 +1083,13 @@ static int mt798x_phy_config_init(struct phy_device *phydev)
return mt798x_phy_calibration(phydev);
}
-static int mt798x_phy_hw_led_on_set(struct phy_device *phydev, u8 index,
- bool on)
-{
- unsigned int bit_on = MTK_PHY_LED_STATE_FORCE_ON + (index ? 16 : 0);
- struct mtk_socphy_priv *priv = phydev->priv;
- bool changed;
-
- if (on)
- changed = !test_and_set_bit(bit_on, &priv->led_state);
- else
- changed = !!test_and_clear_bit(bit_on, &priv->led_state);
-
- changed |= !!test_and_clear_bit(MTK_PHY_LED_STATE_NETDEV +
- (index ? 16 : 0), &priv->led_state);
- if (changed)
- return phy_modify_mmd(phydev, MDIO_MMD_VEND2, index ?
- MTK_PHY_LED1_ON_CTRL : MTK_PHY_LED0_ON_CTRL,
- MTK_PHY_LED_ON_MASK,
- on ? MTK_PHY_LED_ON_FORCE_ON : 0);
- else
- return 0;
-}
-
-static int mt798x_phy_hw_led_blink_set(struct phy_device *phydev, u8 index,
- bool blinking)
-{
- unsigned int bit_blink = MTK_PHY_LED_STATE_FORCE_BLINK + (index ? 16 : 0);
- struct mtk_socphy_priv *priv = phydev->priv;
- bool changed;
-
- if (blinking)
- changed = !test_and_set_bit(bit_blink, &priv->led_state);
- else
- changed = !!test_and_clear_bit(bit_blink, &priv->led_state);
-
- changed |= !!test_bit(MTK_PHY_LED_STATE_NETDEV +
- (index ? 16 : 0), &priv->led_state);
- if (changed)
- return phy_write_mmd(phydev, MDIO_MMD_VEND2, index ?
- MTK_PHY_LED1_BLINK_CTRL : MTK_PHY_LED0_BLINK_CTRL,
- blinking ? MTK_PHY_LED_BLINK_FORCE_BLINK : 0);
- else
- return 0;
-}
-
static int mt798x_phy_led_blink_set(struct phy_device *phydev, u8 index,
unsigned long *delay_on,
unsigned long *delay_off)
{
bool blinking = false;
int err = 0;
+ struct mtk_socphy_priv *priv = phydev->priv;
if (index > 1)
return -EINVAL;
@@ -1191,23 +1100,25 @@ static int mt798x_phy_led_blink_set(struct phy_device *phydev, u8 index,
*delay_off = 50;
}
- err = mt798x_phy_hw_led_blink_set(phydev, index, blinking);
+ err = mtk_phy_hw_led_blink_set(phydev, index, &priv->led_state, blinking);
if (err)
return err;
- return mt798x_phy_hw_led_on_set(phydev, index, false);
+ return mtk_phy_hw_led_on_set(phydev, index, &priv->led_state, MTK_GPHY_LED_ON_MASK, false);
}
static int mt798x_phy_led_brightness_set(struct phy_device *phydev,
u8 index, enum led_brightness value)
{
int err;
+ struct mtk_socphy_priv *priv = phydev->priv;
- err = mt798x_phy_hw_led_blink_set(phydev, index, false);
+ err = mtk_phy_hw_led_blink_set(phydev, index, &priv->led_state, false);
if (err)
return err;
- return mt798x_phy_hw_led_on_set(phydev, index, (value != LED_OFF));
+ return mtk_phy_hw_led_on_set(phydev, index, &priv->led_state,
+ MTK_GPHY_LED_ON_MASK, (value != LED_OFF));
}
static const unsigned long supported_triggers = (BIT(TRIGGER_NETDEV_FULL_DUPLEX) |
@@ -1219,151 +1130,29 @@ static const unsigned long supported_triggers = (BIT(TRIGGER_NETDEV_FULL_DUPLEX)
BIT(TRIGGER_NETDEV_RX) |
BIT(TRIGGER_NETDEV_TX));
-static int mt798x_phy_led_hw_is_supported(struct phy_device *phydev, u8 index,
- unsigned long rules)
+static int mt798x_phy_led_hw_is_supported(struct phy_device *phydev, u8 index, unsigned long rules)
{
- if (index > 1)
- return -EINVAL;
-
- /* All combinations of the supported triggers are allowed */
- if (rules & ~supported_triggers)
- return -EOPNOTSUPP;
-
- return 0;
-};
+ return mtk_phy_led_hw_is_supported(phydev, index, rules, supported_triggers);
+}
static int mt798x_phy_led_hw_control_get(struct phy_device *phydev, u8 index,
unsigned long *rules)
{
- unsigned int bit_blink = MTK_PHY_LED_STATE_FORCE_BLINK + (index ? 16 : 0);
- unsigned int bit_netdev = MTK_PHY_LED_STATE_NETDEV + (index ? 16 : 0);
- unsigned int bit_on = MTK_PHY_LED_STATE_FORCE_ON + (index ? 16 : 0);
struct mtk_socphy_priv *priv = phydev->priv;
- int on, blink;
-
- if (index > 1)
- return -EINVAL;
-
- on = phy_read_mmd(phydev, MDIO_MMD_VEND2,
- index ? MTK_PHY_LED1_ON_CTRL : MTK_PHY_LED0_ON_CTRL);
-
- if (on < 0)
- return -EIO;
-
- blink = phy_read_mmd(phydev, MDIO_MMD_VEND2,
- index ? MTK_PHY_LED1_BLINK_CTRL :
- MTK_PHY_LED0_BLINK_CTRL);
- if (blink < 0)
- return -EIO;
- if ((on & (MTK_PHY_LED_ON_LINK | MTK_PHY_LED_ON_FDX | MTK_PHY_LED_ON_HDX |
- MTK_PHY_LED_ON_LINKDOWN)) ||
- (blink & (MTK_PHY_LED_BLINK_RX | MTK_PHY_LED_BLINK_TX)))
- set_bit(bit_netdev, &priv->led_state);
- else
- clear_bit(bit_netdev, &priv->led_state);
-
- if (on & MTK_PHY_LED_ON_FORCE_ON)
- set_bit(bit_on, &priv->led_state);
- else
- clear_bit(bit_on, &priv->led_state);
-
- if (blink & MTK_PHY_LED_BLINK_FORCE_BLINK)
- set_bit(bit_blink, &priv->led_state);
- else
- clear_bit(bit_blink, &priv->led_state);
-
- if (!rules)
- return 0;
-
- if (on & MTK_PHY_LED_ON_LINK)
- *rules |= BIT(TRIGGER_NETDEV_LINK);
-
- if (on & MTK_PHY_LED_ON_LINK10)
- *rules |= BIT(TRIGGER_NETDEV_LINK_10);
-
- if (on & MTK_PHY_LED_ON_LINK100)
- *rules |= BIT(TRIGGER_NETDEV_LINK_100);
-
- if (on & MTK_PHY_LED_ON_LINK1000)
- *rules |= BIT(TRIGGER_NETDEV_LINK_1000);
-
- if (on & MTK_PHY_LED_ON_FDX)
- *rules |= BIT(TRIGGER_NETDEV_FULL_DUPLEX);
-
- if (on & MTK_PHY_LED_ON_HDX)
- *rules |= BIT(TRIGGER_NETDEV_HALF_DUPLEX);
-
- if (blink & MTK_PHY_LED_BLINK_RX)
- *rules |= BIT(TRIGGER_NETDEV_RX);
-
- if (blink & MTK_PHY_LED_BLINK_TX)
- *rules |= BIT(TRIGGER_NETDEV_TX);
-
- return 0;
+ return mtk_phy_led_hw_ctrl_get(phydev, index, rules, &priv->led_state,
+ MTK_GPHY_LED_ON_SET,
+ MTK_GPHY_LED_RX_BLINK_SET, MTK_GPHY_LED_TX_BLINK_SET);
};
static int mt798x_phy_led_hw_control_set(struct phy_device *phydev, u8 index,
unsigned long rules)
{
- unsigned int bit_netdev = MTK_PHY_LED_STATE_NETDEV + (index ? 16 : 0);
struct mtk_socphy_priv *priv = phydev->priv;
- u16 on = 0, blink = 0;
- int ret;
-
- if (index > 1)
- return -EINVAL;
- if (rules & BIT(TRIGGER_NETDEV_FULL_DUPLEX))
- on |= MTK_PHY_LED_ON_FDX;
-
- if (rules & BIT(TRIGGER_NETDEV_HALF_DUPLEX))
- on |= MTK_PHY_LED_ON_HDX;
-
- if (rules & (BIT(TRIGGER_NETDEV_LINK_10) | BIT(TRIGGER_NETDEV_LINK)))
- on |= MTK_PHY_LED_ON_LINK10;
-
- if (rules & (BIT(TRIGGER_NETDEV_LINK_100) | BIT(TRIGGER_NETDEV_LINK)))
- on |= MTK_PHY_LED_ON_LINK100;
-
- if (rules & (BIT(TRIGGER_NETDEV_LINK_1000) | BIT(TRIGGER_NETDEV_LINK)))
- on |= MTK_PHY_LED_ON_LINK1000;
-
- if (rules & BIT(TRIGGER_NETDEV_RX)) {
- blink |= (on & MTK_PHY_LED_ON_LINK) ?
- (((on & MTK_PHY_LED_ON_LINK10) ? MTK_PHY_LED_BLINK_10RX : 0) |
- ((on & MTK_PHY_LED_ON_LINK100) ? MTK_PHY_LED_BLINK_100RX : 0) |
- ((on & MTK_PHY_LED_ON_LINK1000) ? MTK_PHY_LED_BLINK_1000RX : 0)) :
- MTK_PHY_LED_BLINK_RX;
- }
-
- if (rules & BIT(TRIGGER_NETDEV_TX)) {
- blink |= (on & MTK_PHY_LED_ON_LINK) ?
- (((on & MTK_PHY_LED_ON_LINK10) ? MTK_PHY_LED_BLINK_10TX : 0) |
- ((on & MTK_PHY_LED_ON_LINK100) ? MTK_PHY_LED_BLINK_100TX : 0) |
- ((on & MTK_PHY_LED_ON_LINK1000) ? MTK_PHY_LED_BLINK_1000TX : 0)) :
- MTK_PHY_LED_BLINK_TX;
- }
-
- if (blink || on)
- set_bit(bit_netdev, &priv->led_state);
- else
- clear_bit(bit_netdev, &priv->led_state);
-
- ret = phy_modify_mmd(phydev, MDIO_MMD_VEND2, index ?
- MTK_PHY_LED1_ON_CTRL :
- MTK_PHY_LED0_ON_CTRL,
- MTK_PHY_LED_ON_FDX |
- MTK_PHY_LED_ON_HDX |
- MTK_PHY_LED_ON_LINK,
- on);
-
- if (ret)
- return ret;
-
- return phy_write_mmd(phydev, MDIO_MMD_VEND2, index ?
- MTK_PHY_LED1_BLINK_CTRL :
- MTK_PHY_LED0_BLINK_CTRL, blink);
+ return mtk_phy_led_hw_ctrl_set(phydev, index, rules, &priv->led_state,
+ MTK_GPHY_LED_ON_SET,
+ MTK_GPHY_LED_RX_BLINK_SET, MTK_GPHY_LED_TX_BLINK_SET);
};
static bool mt7988_phy_led_get_polarity(struct phy_device *phydev, int led_num)
@@ -1437,14 +1226,6 @@ static int mt7988_phy_probe_shared(struct phy_device *phydev)
return 0;
}
-static void mt798x_phy_leds_state_init(struct phy_device *phydev)
-{
- int i;
-
- for (i = 0; i < 2; ++i)
- mt798x_phy_led_hw_control_get(phydev, i, NULL);
-}
-
static int mt7988_phy_probe(struct phy_device *phydev)
{
struct mtk_socphy_shared *shared;
@@ -1470,7 +1251,7 @@ static int mt7988_phy_probe(struct phy_device *phydev)
phydev->priv = priv;
- mt798x_phy_leds_state_init(phydev);
+ mtk_phy_leds_state_init(phydev);
err = mt7988_phy_fix_leds_polarities(phydev);
if (err)
@@ -1497,7 +1278,7 @@ static int mt7981_phy_probe(struct phy_device *phydev)
phydev->priv = priv;
- mt798x_phy_leds_state_init(phydev);
+ mtk_phy_leds_state_init(phydev);
return mt798x_phy_calibration(phydev);
}
@@ -1512,8 +1293,8 @@ static struct phy_driver mtk_socphy_driver[] = {
.probe = mt7981_phy_probe,
.suspend = genphy_suspend,
.resume = genphy_resume,
- .read_page = mtk_socphy_read_page,
- .write_page = mtk_socphy_write_page,
+ .read_page = mtk_phy_read_page,
+ .write_page = mtk_phy_write_page,
.led_blink_set = mt798x_phy_led_blink_set,
.led_brightness_set = mt798x_phy_led_brightness_set,
.led_hw_is_supported = mt798x_phy_led_hw_is_supported,
@@ -1529,8 +1310,8 @@ static struct phy_driver mtk_socphy_driver[] = {
.probe = mt7988_phy_probe,
.suspend = genphy_suspend,
.resume = genphy_resume,
- .read_page = mtk_socphy_read_page,
- .write_page = mtk_socphy_write_page,
+ .read_page = mtk_phy_read_page,
+ .write_page = mtk_phy_write_page,
.led_blink_set = mt798x_phy_led_blink_set,
.led_brightness_set = mt798x_phy_led_brightness_set,
.led_hw_is_supported = mt798x_phy_led_hw_is_supported,
diff --git a/drivers/net/phy/mediatek/mtk-ge.c b/drivers/net/phy/mediatek/mtk-ge.c
index 54ea64a37ab3..80425d65556d 100644
--- a/drivers/net/phy/mediatek/mtk-ge.c
+++ b/drivers/net/phy/mediatek/mtk-ge.c
@@ -3,6 +3,11 @@
#include <linux/module.h>
#include <linux/phy.h>
+#include "mtk.h"
+
+#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
@@ -11,15 +16,9 @@
#define MTK_PHY_PAGE_EXTENDED_2A30 0x2a30
#define MTK_PHY_PAGE_EXTENDED_52B5 0x52b5
-static int mtk_gephy_read_page(struct phy_device *phydev)
-{
- return __phy_read(phydev, MTK_EXT_PAGE_ACCESS);
-}
-
-static int mtk_gephy_write_page(struct phy_device *phydev, int page)
-{
- return __phy_write(phydev, MTK_EXT_PAGE_ACCESS, page);
-}
+struct mtk_gephy_priv {
+ unsigned long led_state;
+};
static void mtk_gephy_config_init(struct phy_device *phydev)
{
@@ -65,9 +64,97 @@ static int mt7531_phy_config_init(struct phy_device *phydev)
return 0;
}
+static int mt7531_phy_probe(struct phy_device *phydev)
+{
+ struct mtk_gephy_priv *priv;
+
+ priv = devm_kzalloc(&phydev->mdio.dev, sizeof(struct mtk_gephy_priv),
+ GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ phydev->priv = priv;
+
+ mtk_phy_leds_state_init(phydev);
+
+ return 0;
+}
+
+static int mt753x_phy_led_blink_set(struct phy_device *phydev, u8 index,
+ unsigned long *delay_on,
+ unsigned long *delay_off)
+{
+ bool blinking = false;
+ int err = 0;
+ struct mtk_gephy_priv *priv = phydev->priv;
+
+ 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_GPHY_LED_ON_MASK, false);
+}
+
+static int mt753x_phy_led_brightness_set(struct phy_device *phydev,
+ u8 index, enum led_brightness value)
+{
+ int err;
+ struct mtk_gephy_priv *priv = phydev->priv;
+
+ 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_GPHY_LED_ON_MASK, (value != LED_OFF));
+}
+
+static const unsigned long supported_triggers = (BIT(TRIGGER_NETDEV_FULL_DUPLEX) |
+ BIT(TRIGGER_NETDEV_HALF_DUPLEX) |
+ BIT(TRIGGER_NETDEV_LINK) |
+ BIT(TRIGGER_NETDEV_LINK_10) |
+ BIT(TRIGGER_NETDEV_LINK_100) |
+ BIT(TRIGGER_NETDEV_LINK_1000) |
+ BIT(TRIGGER_NETDEV_RX) |
+ BIT(TRIGGER_NETDEV_TX));
+
+static int mt753x_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 mt753x_phy_led_hw_control_get(struct phy_device *phydev, u8 index,
+ unsigned long *rules)
+{
+ struct mtk_gephy_priv *priv = phydev->priv;
+
+ return mtk_phy_led_hw_ctrl_get(phydev, index, rules, &priv->led_state,
+ MTK_GPHY_LED_ON_SET,
+ MTK_GPHY_LED_RX_BLINK_SET, MTK_GPHY_LED_TX_BLINK_SET);
+};
+
+static int mt753x_phy_led_hw_control_set(struct phy_device *phydev, u8 index,
+ unsigned long rules)
+{
+ struct mtk_gephy_priv *priv = phydev->priv;
+
+ return mtk_phy_led_hw_ctrl_set(phydev, index, rules, &priv->led_state,
+ MTK_GPHY_LED_ON_SET,
+ MTK_GPHY_LED_RX_BLINK_SET, MTK_GPHY_LED_TX_BLINK_SET);
+};
+
static struct phy_driver mtk_gephy_driver[] = {
{
- PHY_ID_MATCH_EXACT(0x03a29412),
+ PHY_ID_MATCH_EXACT(MTK_GPHY_ID_MT7530),
.name = "MediaTek MT7530 PHY",
.config_init = mt7530_phy_config_init,
/* Interrupts are handled by the switch, not the PHY
@@ -77,12 +164,14 @@ static struct phy_driver mtk_gephy_driver[] = {
.handle_interrupt = genphy_handle_interrupt_no_ack,
.suspend = genphy_suspend,
.resume = genphy_resume,
- .read_page = mtk_gephy_read_page,
- .write_page = mtk_gephy_write_page,
+ .read_page = mtk_phy_read_page,
+ .write_page = mtk_phy_write_page,
+ .led_hw_is_supported = mt753x_phy_led_hw_is_supported,
},
{
- PHY_ID_MATCH_EXACT(0x03a29441),
+ PHY_ID_MATCH_EXACT(MTK_GPHY_ID_MT7531),
.name = "MediaTek MT7531 PHY",
+ .probe = mt7531_phy_probe,
.config_init = mt7531_phy_config_init,
/* Interrupts are handled by the switch, not the PHY
* itself.
@@ -91,16 +180,21 @@ static struct phy_driver mtk_gephy_driver[] = {
.handle_interrupt = genphy_handle_interrupt_no_ack,
.suspend = genphy_suspend,
.resume = genphy_resume,
- .read_page = mtk_gephy_read_page,
- .write_page = mtk_gephy_write_page,
+ .read_page = mtk_phy_read_page,
+ .write_page = mtk_phy_write_page,
+ .led_blink_set = mt753x_phy_led_blink_set,
+ .led_brightness_set = mt753x_phy_led_brightness_set,
+ .led_hw_is_supported = mt753x_phy_led_hw_is_supported,
+ .led_hw_control_set = mt753x_phy_led_hw_control_set,
+ .led_hw_control_get = mt753x_phy_led_hw_control_get,
},
};
module_phy_driver(mtk_gephy_driver);
static struct mdio_device_id __maybe_unused mtk_gephy_tbl[] = {
- { PHY_ID_MATCH_EXACT(0x03a29441) },
- { PHY_ID_MATCH_EXACT(0x03a29412) },
+ { PHY_ID_MATCH_EXACT(MTK_GPHY_ID_MT7530) },
+ { PHY_ID_MATCH_EXACT(MTK_GPHY_ID_MT7531) },
{ }
};
diff --git a/drivers/net/phy/mediatek/mtk-phy-lib.c b/drivers/net/phy/mediatek/mtk-phy-lib.c
new file mode 100644
index 000000000000..8a287877cf81
--- /dev/null
+++ b/drivers/net/phy/mediatek/mtk-phy-lib.c
@@ -0,0 +1,230 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <linux/phy.h>
+#include <linux/module.h>
+
+#include <linux/netdevice.h>
+
+#include "mtk.h"
+
+int mtk_phy_read_page(struct phy_device *phydev)
+{
+ return __phy_read(phydev, MTK_EXT_PAGE_ACCESS);
+}
+EXPORT_SYMBOL_GPL(mtk_phy_read_page);
+
+int mtk_phy_write_page(struct phy_device *phydev, int page)
+{
+ return __phy_write(phydev, MTK_EXT_PAGE_ACCESS, page);
+}
+EXPORT_SYMBOL_GPL(mtk_phy_write_page);
+
+int mtk_phy_led_hw_is_supported(struct phy_device *phydev, u8 index, unsigned long rules,
+ unsigned long supported_triggers)
+{
+ if (index > 1)
+ return -EINVAL;
+
+ /* All combinations of the supported triggers are allowed */
+ if (rules & ~supported_triggers)
+ return -EOPNOTSUPP;
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(mtk_phy_led_hw_is_supported);
+
+int mtk_phy_led_hw_ctrl_get(struct phy_device *phydev, u8 index, unsigned long *rules,
+ unsigned long *led_state, u16 on_set,
+ u16 rx_blink_set, u16 tx_blink_set)
+{
+ unsigned int bit_blink = MTK_PHY_LED_STATE_FORCE_BLINK + (index ? 16 : 0);
+ unsigned int bit_netdev = MTK_PHY_LED_STATE_NETDEV + (index ? 16 : 0);
+ unsigned int bit_on = MTK_PHY_LED_STATE_FORCE_ON + (index ? 16 : 0);
+ int on, blink;
+
+ if (index > 1)
+ return -EINVAL;
+
+ on = phy_read_mmd(phydev, MDIO_MMD_VEND2,
+ index ? MTK_PHY_LED1_ON_CTRL : MTK_PHY_LED0_ON_CTRL);
+
+ if (on < 0)
+ return -EIO;
+
+ blink = phy_read_mmd(phydev, MDIO_MMD_VEND2,
+ index ? MTK_PHY_LED1_BLINK_CTRL :
+ MTK_PHY_LED0_BLINK_CTRL);
+ if (blink < 0)
+ return -EIO;
+
+ if ((on & (on_set | MTK_PHY_LED_ON_FDX | MTK_PHY_LED_ON_HDX | MTK_PHY_LED_ON_LINKDOWN)) ||
+ (blink & (rx_blink_set | tx_blink_set)))
+ set_bit(bit_netdev, led_state);
+ else
+ clear_bit(bit_netdev, led_state);
+
+ if (on & MTK_PHY_LED_ON_FORCE_ON)
+ set_bit(bit_on, led_state);
+ else
+ clear_bit(bit_on, led_state);
+
+ if (blink & MTK_PHY_LED_BLINK_FORCE_BLINK)
+ set_bit(bit_blink, led_state);
+ else
+ clear_bit(bit_blink, led_state);
+
+ if (!rules)
+ return 0;
+
+ if (on & on_set)
+ *rules |= BIT(TRIGGER_NETDEV_LINK);
+
+ if (on & MTK_PHY_LED_ON_LINK10)
+ *rules |= BIT(TRIGGER_NETDEV_LINK_10);
+
+ if (on & MTK_PHY_LED_ON_LINK100)
+ *rules |= BIT(TRIGGER_NETDEV_LINK_100);
+
+ if (on & MTK_PHY_LED_ON_LINK1000)
+ *rules |= BIT(TRIGGER_NETDEV_LINK_1000);
+
+ if (on & MTK_PHY_LED_ON_LINK2500)
+ *rules |= BIT(TRIGGER_NETDEV_LINK_2500);
+
+ if (on & MTK_PHY_LED_ON_FDX)
+ *rules |= BIT(TRIGGER_NETDEV_FULL_DUPLEX);
+
+ if (on & MTK_PHY_LED_ON_HDX)
+ *rules |= BIT(TRIGGER_NETDEV_HALF_DUPLEX);
+
+ if (blink & rx_blink_set)
+ *rules |= BIT(TRIGGER_NETDEV_RX);
+
+ if (blink & tx_blink_set)
+ *rules |= BIT(TRIGGER_NETDEV_TX);
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(mtk_phy_led_hw_ctrl_get);
+
+int mtk_phy_led_hw_ctrl_set(struct phy_device *phydev, u8 index, unsigned long rules,
+ unsigned long *led_state, u16 on_set,
+ u16 rx_blink_set, u16 tx_blink_set)
+{
+ unsigned int bit_netdev = MTK_PHY_LED_STATE_NETDEV + (index ? 16 : 0);
+ u16 on = 0, blink = 0;
+ int ret;
+
+ if (index > 1)
+ return -EINVAL;
+
+ if (rules & BIT(TRIGGER_NETDEV_FULL_DUPLEX))
+ on |= MTK_PHY_LED_ON_FDX;
+
+ if (rules & BIT(TRIGGER_NETDEV_HALF_DUPLEX))
+ on |= MTK_PHY_LED_ON_HDX;
+
+ if (rules & (BIT(TRIGGER_NETDEV_LINK_10) | BIT(TRIGGER_NETDEV_LINK)))
+ on |= MTK_PHY_LED_ON_LINK10;
+
+ if (rules & (BIT(TRIGGER_NETDEV_LINK_100) | BIT(TRIGGER_NETDEV_LINK)))
+ on |= MTK_PHY_LED_ON_LINK100;
+
+ if (rules & (BIT(TRIGGER_NETDEV_LINK_1000) | BIT(TRIGGER_NETDEV_LINK)))
+ on |= MTK_PHY_LED_ON_LINK1000;
+
+ if (rules & (BIT(TRIGGER_NETDEV_LINK_2500) | BIT(TRIGGER_NETDEV_LINK)))
+ on |= MTK_PHY_LED_ON_LINK2500;
+
+ if (rules & BIT(TRIGGER_NETDEV_RX)) {
+ blink |= (on & on_set) ?
+ (((on & MTK_PHY_LED_ON_LINK10) ? MTK_PHY_LED_BLINK_10RX : 0) |
+ ((on & MTK_PHY_LED_ON_LINK100) ? MTK_PHY_LED_BLINK_100RX : 0) |
+ ((on & MTK_PHY_LED_ON_LINK1000) ? MTK_PHY_LED_BLINK_1000RX : 0) |
+ ((on & MTK_PHY_LED_ON_LINK2500) ? MTK_PHY_LED_BLINK_2500RX : 0)) :
+ rx_blink_set;
+ }
+
+ if (rules & BIT(TRIGGER_NETDEV_TX)) {
+ blink |= (on & on_set) ?
+ (((on & MTK_PHY_LED_ON_LINK10) ? MTK_PHY_LED_BLINK_10TX : 0) |
+ ((on & MTK_PHY_LED_ON_LINK100) ? MTK_PHY_LED_BLINK_100TX : 0) |
+ ((on & MTK_PHY_LED_ON_LINK1000) ? MTK_PHY_LED_BLINK_1000TX : 0) |
+ ((on & MTK_PHY_LED_ON_LINK2500) ? MTK_PHY_LED_BLINK_2500TX : 0)) :
+ tx_blink_set;
+ }
+
+ if (blink || on)
+ set_bit(bit_netdev, led_state);
+ else
+ clear_bit(bit_netdev, led_state);
+
+ ret = phy_modify_mmd(phydev, MDIO_MMD_VEND2, index ?
+ MTK_PHY_LED1_ON_CTRL : MTK_PHY_LED0_ON_CTRL,
+ MTK_PHY_LED_ON_FDX | MTK_PHY_LED_ON_HDX | on_set, on);
+
+ if (ret)
+ return ret;
+
+ return phy_write_mmd(phydev, MDIO_MMD_VEND2, index ?
+ MTK_PHY_LED1_BLINK_CTRL :
+ MTK_PHY_LED0_BLINK_CTRL, blink);
+}
+EXPORT_SYMBOL_GPL(mtk_phy_led_hw_ctrl_set);
+
+int mtk_phy_hw_led_on_set(struct phy_device *phydev, u8 index, unsigned long *led_state,
+ u16 led_on_mask, bool on)
+{
+ unsigned int bit_on = MTK_PHY_LED_STATE_FORCE_ON + (index ? 16 : 0);
+ bool changed;
+
+ if (on)
+ changed = !test_and_set_bit(bit_on, led_state);
+ else
+ changed = !!test_and_clear_bit(bit_on, led_state);
+
+ changed |= !!test_and_clear_bit(MTK_PHY_LED_STATE_NETDEV +
+ (index ? 16 : 0), led_state);
+ if (changed)
+ return phy_modify_mmd(phydev, MDIO_MMD_VEND2, index ?
+ MTK_PHY_LED1_ON_CTRL : MTK_PHY_LED0_ON_CTRL,
+ led_on_mask,
+ on ? MTK_PHY_LED_ON_FORCE_ON : 0);
+ else
+ return 0;
+}
+EXPORT_SYMBOL_GPL(mtk_phy_hw_led_on_set);
+
+int mtk_phy_hw_led_blink_set(struct phy_device *phydev, u8 index, unsigned long *led_state,
+ bool blinking)
+{
+ unsigned int bit_blink = MTK_PHY_LED_STATE_FORCE_BLINK + (index ? 16 : 0);
+ bool changed;
+
+ if (blinking)
+ changed = !test_and_set_bit(bit_blink, led_state);
+ else
+ changed = !!test_and_clear_bit(bit_blink, led_state);
+
+ changed |= !!test_bit(MTK_PHY_LED_STATE_NETDEV +
+ (index ? 16 : 0), led_state);
+ if (changed)
+ return phy_write_mmd(phydev, MDIO_MMD_VEND2, index ?
+ MTK_PHY_LED1_BLINK_CTRL : MTK_PHY_LED0_BLINK_CTRL,
+ blinking ? MTK_PHY_LED_BLINK_FORCE_BLINK : 0);
+ else
+ return 0;
+}
+EXPORT_SYMBOL_GPL(mtk_phy_hw_led_blink_set);
+
+void mtk_phy_leds_state_init(struct phy_device *phydev)
+{
+ int i;
+
+ for (i = 0; i < 2; ++i)
+ phydev->drv->led_hw_control_get(phydev, i, NULL);
+}
+EXPORT_SYMBOL_GPL(mtk_phy_leds_state_init);
+
+MODULE_DESCRIPTION("MediaTek Ethernet PHY driver common");
+MODULE_AUTHOR("Sky Huang <SkyLake.Huang@mediatek.com>");
+MODULE_LICENSE("GPL");
diff --git a/drivers/net/phy/mediatek/mtk.h b/drivers/net/phy/mediatek/mtk.h
new file mode 100644
index 000000000000..c392c38be1ab
--- /dev/null
+++ b/drivers/net/phy/mediatek/mtk.h
@@ -0,0 +1,82 @@
+/* SPDX-License-Identifier: GPL-2.0
+ *
+ * Common definition for Mediatek Ethernet PHYs
+ * Author: SkyLake Huang <SkyLake.Huang@mediatek.com>
+ * Copyright (c) 2024 MediaTek Inc.
+ */
+
+#ifndef _MTK_EPHY_H_
+#define _MTK_EPHY_H_
+
+#define MTK_EXT_PAGE_ACCESS 0x1f
+
+/* Registers on MDIO_MMD_VEND2 */
+#define MTK_PHY_LED0_ON_CTRL 0x24
+#define MTK_PHY_LED1_ON_CTRL 0x26
+#define MTK_GPHY_LED_ON_MASK GENMASK(6, 0)
+#define MTK_2P5GPHY_LED_ON_MASK GENMASK(7, 0)
+#define MTK_PHY_LED_ON_LINK1000 BIT(0)
+#define MTK_PHY_LED_ON_LINK100 BIT(1)
+#define MTK_PHY_LED_ON_LINK10 BIT(2)
+#define MTK_PHY_LED_ON_LINKDOWN BIT(3)
+#define MTK_PHY_LED_ON_FDX BIT(4) /* Full duplex */
+#define MTK_PHY_LED_ON_HDX BIT(5) /* Half duplex */
+#define MTK_PHY_LED_ON_FORCE_ON BIT(6)
+#define MTK_PHY_LED_ON_LINK2500 BIT(7)
+#define MTK_PHY_LED_ON_POLARITY BIT(14)
+#define MTK_PHY_LED_ON_ENABLE BIT(15)
+
+#define MTK_PHY_LED0_BLINK_CTRL 0x25
+#define MTK_PHY_LED1_BLINK_CTRL 0x27
+#define MTK_PHY_LED_BLINK_1000TX BIT(0)
+#define MTK_PHY_LED_BLINK_1000RX BIT(1)
+#define MTK_PHY_LED_BLINK_100TX BIT(2)
+#define MTK_PHY_LED_BLINK_100RX BIT(3)
+#define MTK_PHY_LED_BLINK_10TX BIT(4)
+#define MTK_PHY_LED_BLINK_10RX BIT(5)
+#define MTK_PHY_LED_BLINK_COLLISION BIT(6)
+#define MTK_PHY_LED_BLINK_RX_CRC_ERR BIT(7)
+#define MTK_PHY_LED_BLINK_RX_IDLE_ERR BIT(8)
+#define MTK_PHY_LED_BLINK_FORCE_BLINK BIT(9)
+#define MTK_PHY_LED_BLINK_2500TX BIT(10)
+#define MTK_PHY_LED_BLINK_2500RX BIT(11)
+
+#define MTK_GPHY_LED_ON_SET (MTK_PHY_LED_ON_LINK1000 | \
+ MTK_PHY_LED_ON_LINK100 | \
+ MTK_PHY_LED_ON_LINK10)
+#define MTK_GPHY_LED_RX_BLINK_SET (MTK_PHY_LED_BLINK_1000RX | \
+ MTK_PHY_LED_BLINK_100RX | \
+ MTK_PHY_LED_BLINK_10RX)
+#define MTK_GPHY_LED_TX_BLINK_SET (MTK_PHY_LED_BLINK_1000RX | \
+ MTK_PHY_LED_BLINK_100RX | \
+ MTK_PHY_LED_BLINK_10RX)
+
+#define MTK_2P5GPHY_LED_ON_SET (MTK_PHY_LED_ON_LINK2500 | \
+ MTK_GPHY_LED_ON_SET)
+#define MTK_2P5GPHY_LED_RX_BLINK_SET (MTK_PHY_LED_BLINK_2500RX | \
+ MTK_GPHY_LED_RX_BLINK_SET)
+#define MTK_2P5GPHY_LED_TX_BLINK_SET (MTK_PHY_LED_BLINK_2500RX | \
+ MTK_GPHY_LED_TX_BLINK_SET)
+
+#define MTK_PHY_LED_STATE_FORCE_ON 0
+#define MTK_PHY_LED_STATE_FORCE_BLINK 1
+#define MTK_PHY_LED_STATE_NETDEV 2
+
+int mtk_phy_read_page(struct phy_device *phydev);
+int mtk_phy_write_page(struct phy_device *phydev, int page);
+
+int mtk_phy_led_hw_is_supported(struct phy_device *phydev, u8 index, unsigned long rules,
+ unsigned long supported_triggers);
+int mtk_phy_led_hw_ctrl_set(struct phy_device *phydev, u8 index, unsigned long rules,
+ unsigned long *led_state, u16 on_set,
+ u16 rx_blink_set, u16 tx_blink_set);
+int mtk_phy_led_hw_ctrl_get(struct phy_device *phydev, u8 index, unsigned long *rules,
+ unsigned long *led_state, u16 on_set,
+ u16 rx_blink_set, u16 tx_blink_set);
+int mtk_phy_hw_led_on_set(struct phy_device *phydev, u8 index, unsigned long *led_state,
+ u16 led_on_mask, bool on);
+int mtk_phy_hw_led_blink_set(struct phy_device *phydev, u8 index, unsigned long *led_state,
+ bool blinking);
+void mtk_phy_leds_state_init(struct phy_device *phydev);
+
+#endif /* _MTK_EPHY_H_ */
--
2.18.0
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH net-next v3 3/5] net: phy: mediatek: Add token ring access helper functions in mtk-phy-lib
2024-05-20 11:34 [PATCH net-next v3 0/5] net: phy: mediatek: Introduce mtk-phy-lib and add 2.5Gphy support Sky Huang
2024-05-20 11:34 ` [PATCH net-next v3 1/5] net: phy: mediatek: Re-organize MediaTek ethernet phy drivers Sky Huang
2024-05-20 11:34 ` [PATCH net-next v3 2/5] net: phy: mediatek: Move LED and read/write page helper functions into mtk phy lib Sky Huang
@ 2024-05-20 11:34 ` Sky Huang
2024-05-20 11:34 ` [PATCH net-next v3 4/5] net: phy: mediatek: Extend 1G TX/RX link pulse time Sky Huang
` (3 subsequent siblings)
6 siblings, 0 replies; 19+ messages in thread
From: Sky Huang @ 2024-05-20 11:34 UTC (permalink / raw)
To: Andrew Lunn, Heiner Kallweit, Russell King, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Daniel Golle,
Qingfang Deng, SkyLake Huang, Matthias Brugger,
AngeloGioacchino Del Regno, linux-kernel, netdev,
linux-arm-kernel, linux-mediatek
Cc: Steven Liu, SkyLake.Huang
From: "SkyLake.Huang" <skylake.huang@mediatek.com>
This patch adda TR(token ring) manipulations and add correct
macro name for those magic numbers. TR is a way to access
proprietary register 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 | 259 +++++++++++++++----------
drivers/net/phy/mediatek/mtk-ge.c | 80 ++++++--
drivers/net/phy/mediatek/mtk-phy-lib.c | 88 +++++++++
drivers/net/phy/mediatek/mtk.h | 10 +
4 files changed, 312 insertions(+), 125 deletions(-)
diff --git a/drivers/net/phy/mediatek/mtk-ge-soc.c b/drivers/net/phy/mediatek/mtk-ge-soc.c
index 534d4e4f5e1f..8f23137c960f 100644
--- a/drivers/net/phy/mediatek/mtk-ge-soc.c
+++ b/drivers/net/phy/mediatek/mtk-ge-soc.c
@@ -24,7 +24,78 @@
#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 */
+#define NORMAL_MSE_LO_THRESH_MASK GENMASK(15, 8) /* NormMseLoThresh */
+
+/* ch_addr = 0x0, node_addr = 0xf, data_addr = 0x3c */
+#define REMOTE_ACK_COUNT_LIMIT_CTRL_MASK GENMASK(2, 1) /* RemAckCntLimitCtrl */
+
+/* ch_addr = 0x1, node_addr = 0xd, data_addr = 0x20 */
+#define VCO_SLICER_THRESH_HIGH_MASK GENMASK(23, 0) /* VcoSlicerThreshBitsHigh */
+
+/* ch_addr = 0x1, node_addr = 0xf, data_addr = 0x0 */
+#define DFE_TAIL_EANBLE_VGA_TRHESH_1000 GENMASK(5, 1) /* DfeTailEnableVgaThresh1000 */
+
+/* ch_addr = 0x1, node_addr = 0xf, data_addr = 0x1 */
+#define MRVL_TR_FIX_100KP_MASK GENMASK(22, 20) /* MrvlTrFix100Kp */
+#define MRVL_TR_FIX_100KF_MASK GENMASK(19, 17) /* MrvlTrFix100Kf */
+#define MRVL_TR_FIX_1000KP_MASK GENMASK(16, 14) /* MrvlTrFix1000Kp */
+#define MRVL_TR_FIX_1000KF_MASK GENMASK(13, 11) /* MrvlTrFix1000Kf */
+
+/* ch_addr = 0x1, node_addr = 0xf, data_addr = 0x12 */
+#define VGA_DECIMATION_RATE_MASK GENMASK(8, 5) /* VgaDecRate */
+
+/* ch_addr = 0x1, node_addr = 0xf, data_addr = 0x17 */
+#define SLAVE_DSP_READY_TIME_MASK GENMASK(22, 15) /* SlvDSPreadyTime */
+#define MASTER_DSP_READY_TIME_MASK GENMASK(14, 7) /* MasDSPreadyTime */
+
+/* ch_addr = 0x1, node_addr = 0xf, data_addr = 0x18 */
+#define ENABLE_RANDOM_UPDOWN_COUNTER_TRIGGER BIT(8) /* EnabRandUpdTrig */
+
+/* ch_addr = 0x1, node_addr = 0xf, data_addr = 0x20 */
+#define RESET_SYNC_OFFSET_MASK GENMASK(11, 8) /* ResetSyncOffset */
+
+/* ch_addr = 0x2, node_addr = 0xd, data_addr = 0x0 */
+#define FFE_UPDATE_GAIN_FORCE_VAL_MASK GENMASK(9, 7) /* FfeUpdGainForceVal */
+#define FFE_UPDATE_GAIN_FORCE BIT(6) /* FfeUpdGainForce */
+
+/* ch_addr = 0x2, node_addr = 0xd, data_addr = 0x3 */
+#define TR_FREEZE_MASK GENMASK(11, 0) /* TrFreeze */
+
+/* ch_addr = 0x2, node_addr = 0xd, data_addr = 0x6 */
+/* SS: Steady-state, KP: Proportional Gain */
+#define SS_TR_KP100_MASK GENMASK(21, 19) /* SSTrKp100 */
+#define SS_TR_KF100_MASK GENMASK(18, 16) /* SSTrKf100 */
+#define SS_TR_KP1000_MASTER_MASK GENMASK(15, 13) /* SSTrKp1000Mas */
+#define SS_TR_KF1000_MASTER_MASK GENMASK(12, 10) /* SSTrKf1000Mas */
+#define SS_TR_KP1000_SLAVE_MASK GENMASK(9, 7) /* SSTrKp1000Slv */
+#define SS_TR_KF1000_SLAVE_MASK GENMASK(6, 4) /* SSTrKf1000Slv */
+
+/* ch_addr = 0x2, node_addr = 0xd, data_addr = 0x8 */
+/* clear this bit if wanna select from AFE */
+#define EEE1000_SELECT_SIGNEL_DETECTION_FROM_DFE BIT(4) /* Regsigdet_sel_1000 */
+
+/* ch_addr = 0x2, node_addr = 0xd, data_addr = 0xd */
+#define EEE1000_STAGE2_TR_KF_MASK GENMASK(13, 11) /* RegEEE_st2TrKf1000 */
+
+/* ch_addr = 0x2, node_addr = 0xd, data_addr = 0xf */
+#define SLAVE_WAKETR_TIMER_MASK GENMASK(20, 11) /* RegEEE_slv_waketr_timer_tar */
+#define SLAVE_REMTX_TIMER_MASK GENMASK(10, 1) /* RegEEE_slv_remtx_timer_tar */
+
+/* ch_addr = 0x2, node_addr = 0xd, data_addr = 0x10 */
+#define SLAVE_WAKEINT_TIMER_MASK GENMASK(10, 1) /* RegEEE_slv_wake_int_timer_tar */
+
+/* ch_addr = 0x2, node_addr = 0xd, data_addr = 0x14 */
+#define TR_FREEZE_TIMER2_MASK GENMASK(9, 0) /* RegEEE_trfreeze_timer2 */
+
+/* ch_addr = 0x2, node_addr = 0xd, data_addr = 0x1c */
+#define EEE100_LPSYNC_STAGE1_UPDATE_TIMER_MASK GENMASK(8, 0) /* RegEEE100Stg1_tar */
+
+/* ch_addr = 0x2, node_addr = 0xd, data_addr = 0x25 */
+#define WAKE_SLAVE_TR_WAIT_DFE_DETECTION_EN BIT(11) /* REGEEE_wake_slv_tr_wait_dfesigdet_en */
+
#define ANALOG_INTERNAL_OPERATION_MAX_US 20
#define TXRESERVE_MIN 0
@@ -667,40 +738,34 @@ 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);
+ __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));
+
+ __tr_set_bits(phydev, 0x1, 0xf, 0x18, ENABLE_RANDOM_UPDOWN_COUNTER_TRIGGER);
+
+ __tr_modify(phydev, 0x0, 0x7, 0x15,
+ NORMAL_MSE_LO_THRESH_MASK,
+ FIELD_PREP(NORMAL_MSE_LO_THRESH_MASK, 0x55));
+
+ __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);
+
+ __tr_clr_bits(phydev, 0x2, 0xd, 0x3, TR_FREEZE_MASK);
+
+ __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);
}
@@ -723,27 +788,27 @@ 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);
+ __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);
+ __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);
+ __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);
+ __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 */
@@ -794,25 +859,22 @@ 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);
+ __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);
-
+ __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));
+
+ __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 */
@@ -887,45 +949,34 @@ 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);
+ __tr_clr_bits(phydev, 0x2, 0xd, 0x8, EEE1000_SELECT_SIGNEL_DETECTION_FROM_DFE);
+
+ __tr_modify(phydev, 0x2, 0xd, 0xd,
+ EEE1000_STAGE2_TR_KF_MASK,
+ FIELD_PREP(EEE1000_STAGE2_TR_KF_MASK, 0x2));
+
+ __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));
+
+ __tr_modify(phydev, 0x2, 0xd, 0x10,
+ SLAVE_WAKEINT_TIMER_MASK,
+ FIELD_PREP(SLAVE_WAKEINT_TIMER_MASK, 0x8));
+
+ __tr_modify(phydev, 0x2, 0xd, 0x14,
+ TR_FREEZE_TIMER2_MASK,
+ FIELD_PREP(TR_FREEZE_TIMER2_MASK, 0x24a));
+
+ __tr_modify(phydev, 0x2, 0xd, 0x1c,
+ EEE100_LPSYNC_STAGE1_UPDATE_TIMER_MASK,
+ FIELD_PREP(EEE100_LPSYNC_STAGE1_UPDATE_TIMER_MASK, 0x10));
+
+ __tr_clr_bits(phydev, 0x2, 0xd, 0x25, WAKE_SLAVE_TR_WAIT_DFE_DETECTION_EN);
+
+ __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 80425d65556d..5c0226d9f2ee 100644
--- a/drivers/net/phy/mediatek/mtk-ge.c
+++ b/drivers/net/phy/mediatek/mtk-ge.c
@@ -8,13 +8,35 @@
#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)
struct mtk_gephy_priv {
unsigned long led_state;
@@ -23,20 +45,27 @@ struct mtk_gephy_priv {
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);
+ 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)
@@ -44,7 +73,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;
}
@@ -55,11 +85,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 8a287877cf81..39bfefedcfde 100644
--- a/drivers/net/phy/mediatek/mtk-phy-lib.c
+++ b/drivers/net/phy/mediatek/mtk-phy-lib.c
@@ -6,6 +6,94 @@
#include "mtk.h"
+/* Difference between functions with tr* and __tr* prefixes is
+ * tr* functions: wrapped by page switching operations
+ * __tr* fucntions: no page switching operations
+ */
+
+static void __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 __tr_read(struct phy_device *phydev, u8 ch_addr, u8 node_addr, u8 data_addr,
+ u16 *tr_high, u16 *tr_low)
+{
+ __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 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);
+ __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(tr_read);
+
+static void __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);
+ __tr_access(phydev, false, ch_addr, node_addr, data_addr);
+}
+
+void __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;
+
+ __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;
+ __tr_write(phydev, ch_addr, node_addr, data_addr, tr_data);
+}
+EXPORT_SYMBOL_GPL(__tr_modify);
+
+void 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);
+ __tr_modify(phydev, ch_addr, node_addr, data_addr, mask, set);
+ phy_restore_page(phydev, MTK_PHY_PAGE_STANDARD, 0);
+}
+EXPORT_SYMBOL_GPL(tr_modify);
+
+void __tr_set_bits(struct phy_device *phydev, u8 ch_addr, u8 node_addr, u8 data_addr, u32 set)
+{
+ __tr_modify(phydev, ch_addr, node_addr, data_addr, 0, set);
+}
+EXPORT_SYMBOL_GPL(__tr_set_bits);
+
+void __tr_clr_bits(struct phy_device *phydev, u8 ch_addr, u8 node_addr, u8 data_addr, u32 clr)
+{
+ __tr_modify(phydev, ch_addr, node_addr, data_addr, clr, 0);
+}
+EXPORT_SYMBOL_GPL(__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 c392c38be1ab..10ee9bee2304 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
@@ -62,6 +64,14 @@
#define MTK_PHY_LED_STATE_FORCE_BLINK 1
#define MTK_PHY_LED_STATE_NETDEV 2
+u32 tr_read(struct phy_device *phydev, u8 ch_addr, u8 node_addr, u8 data_addr);
+void __tr_modify(struct phy_device *phydev, u8 ch_addr, u8 node_addr, u8 data_addr,
+ u32 mask, u32 set);
+void tr_modify(struct phy_device *phydev, u8 ch_addr, u8 node_addr, u8 data_addr,
+ u32 mask, u32 set);
+void __tr_set_bits(struct phy_device *phydev, u8 ch_addr, u8 node_addr, u8 data_addr, u32 set);
+void __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.18.0
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH net-next v3 4/5] net: phy: mediatek: Extend 1G TX/RX link pulse time
2024-05-20 11:34 [PATCH net-next v3 0/5] net: phy: mediatek: Introduce mtk-phy-lib and add 2.5Gphy support Sky Huang
` (2 preceding siblings ...)
2024-05-20 11:34 ` [PATCH net-next v3 3/5] net: phy: mediatek: Add token ring access helper functions in mtk-phy-lib Sky Huang
@ 2024-05-20 11:34 ` Sky Huang
2024-05-20 13:17 ` Andrew Lunn
2024-05-20 11:34 ` [PATCH net-next v3 5/5] net: phy: add driver for built-in 2.5G ethernet PHY on MT7988 Sky Huang
` (2 subsequent siblings)
6 siblings, 1 reply; 19+ messages in thread
From: Sky Huang @ 2024-05-20 11:34 UTC (permalink / raw)
To: Andrew Lunn, Heiner Kallweit, Russell King, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Daniel Golle,
Qingfang Deng, SkyLake Huang, Matthias Brugger,
AngeloGioacchino Del Regno, linux-kernel, netdev,
linux-arm-kernel, linux-mediatek
Cc: Steven Liu, SkyLake.Huang
From: "SkyLake.Huang" <skylake.huang@mediatek.com>
We observe that some 10G devices' (mostly Marvell's chips inside) 1G
training time violates specification, which may last 2230ms and affect
later TX/RX link pulse time. This will invalidate MediaTek series
gigabit Ethernet PHYs' hardware auto downshift mechanism.
Without this patch, if someone is trying to use "4-wire" cable to
connect above devices, MediaTek' gigabit Ethernet PHYs may fail
to downshift to 100Mbps. (If partner 10G devices' downshift mechanism
stops at 1G)
This patch extends our 1G TX/RX link pulse time so that we can still
link up with those 10G devices.
Tested device:
- Netgear GS110EMX's 10G port (Marvell 88X3340P)
- QNAP QSW-M408-4C
v3:
Refactor mtk_gphy_cl22_read_status() with genphy_read_status().
Signed-off-by: SkyLake.Huang <skylake.huang@mediatek.com>
---
drivers/net/phy/mediatek/mtk-ge-soc.c | 2 +
drivers/net/phy/mediatek/mtk-ge.c | 1 +
drivers/net/phy/mediatek/mtk-phy-lib.c | 59 ++++++++++++++++++++++++++
drivers/net/phy/mediatek/mtk.h | 16 +++++++
4 files changed, 78 insertions(+)
diff --git a/drivers/net/phy/mediatek/mtk-ge-soc.c b/drivers/net/phy/mediatek/mtk-ge-soc.c
index 8f23137c960f..62424d4f3b7a 100644
--- a/drivers/net/phy/mediatek/mtk-ge-soc.c
+++ b/drivers/net/phy/mediatek/mtk-ge-soc.c
@@ -1339,6 +1339,7 @@ static struct phy_driver mtk_socphy_driver[] = {
PHY_ID_MATCH_EXACT(MTK_GPHY_ID_MT7981),
.name = "MediaTek MT7981 PHY",
.config_init = mt798x_phy_config_init,
+ .read_status = mtk_gphy_cl22_read_status,
.config_intr = genphy_no_config_intr,
.handle_interrupt = genphy_handle_interrupt_no_ack,
.probe = mt7981_phy_probe,
@@ -1356,6 +1357,7 @@ static struct phy_driver mtk_socphy_driver[] = {
PHY_ID_MATCH_EXACT(MTK_GPHY_ID_MT7988),
.name = "MediaTek MT7988 PHY",
.config_init = mt798x_phy_config_init,
+ .read_status = mtk_gphy_cl22_read_status,
.config_intr = genphy_no_config_intr,
.handle_interrupt = genphy_handle_interrupt_no_ack,
.probe = mt7988_phy_probe,
diff --git a/drivers/net/phy/mediatek/mtk-ge.c b/drivers/net/phy/mediatek/mtk-ge.c
index 5c0226d9f2ee..c832c90a85f6 100644
--- a/drivers/net/phy/mediatek/mtk-ge.c
+++ b/drivers/net/phy/mediatek/mtk-ge.c
@@ -211,6 +211,7 @@ static struct phy_driver mtk_gephy_driver[] = {
.name = "MediaTek MT7531 PHY",
.probe = mt7531_phy_probe,
.config_init = mt7531_phy_config_init,
+ .read_status = mtk_gphy_cl22_read_status,
/* Interrupts are handled by the switch, not the PHY
* itself.
*/
diff --git a/drivers/net/phy/mediatek/mtk-phy-lib.c b/drivers/net/phy/mediatek/mtk-phy-lib.c
index 39bfefedcfde..f9fd2b3d8437 100644
--- a/drivers/net/phy/mediatek/mtk-phy-lib.c
+++ b/drivers/net/phy/mediatek/mtk-phy-lib.c
@@ -106,6 +106,65 @@ int mtk_phy_write_page(struct phy_device *phydev, int page)
}
EXPORT_SYMBOL_GPL(mtk_phy_write_page);
+static void extend_an_new_lp_cnt_limit(struct phy_device *phydev)
+{
+ int mmd_read_ret;
+ int ret;
+ u32 reg_val;
+
+ ret = read_poll_timeout(mmd_read_ret = phy_read_mmd, reg_val,
+ (mmd_read_ret < 0) || reg_val & MTK_PHY_FINAL_SPEED_1000,
+ 10000, 1000000, false, phydev,
+ MDIO_MMD_VEND1, MTK_PHY_LINK_STATUS_MISC);
+ if (mmd_read_ret < 0)
+ ret = mmd_read_ret;
+ /* If final_speed_1000 is raised, try to extend timeout period
+ * of auto downshift.
+ */
+ if (!ret) {
+ tr_modify(phydev, 0x0, 0xf, 0x3c, AN_NEW_LP_CNT_LIMIT_MASK,
+ FIELD_PREP(AN_NEW_LP_CNT_LIMIT_MASK, 0xf));
+ mdelay(1500);
+
+ ret = read_poll_timeout(mmd_read_ret = tr_read, reg_val,
+ (mmd_read_ret < 0) ||
+ (reg_val & AN_STATE_MASK) !=
+ (AN_STATE_TX_DISABLE << AN_STATE_SHIFT),
+ 10000, 1000000, false, phydev,
+ 0x0, 0xf, 0x2);
+
+ if (mmd_read_ret < 0)
+ ret = mmd_read_ret;
+
+ if (!ret) {
+ mdelay(625);
+ tr_modify(phydev, 0x0, 0xf, 0x3c, AN_NEW_LP_CNT_LIMIT_MASK,
+ FIELD_PREP(AN_NEW_LP_CNT_LIMIT_MASK, 0x8));
+ mdelay(500);
+ tr_modify(phydev, 0x0, 0xf, 0x3c, AN_NEW_LP_CNT_LIMIT_MASK,
+ FIELD_PREP(AN_NEW_LP_CNT_LIMIT_MASK, 0xf));
+ }
+ }
+}
+
+int mtk_gphy_cl22_read_status(struct phy_device *phydev)
+{
+ int ret;
+
+ ret = genphy_read_status(phydev);
+ if (ret)
+ return ret;
+
+ if (phydev->autoneg == AUTONEG_ENABLE && !phydev->autoneg_complete) {
+ ret = phy_read(phydev, MII_CTRL1000);
+ if ((ret & ADVERTISE_1000FULL) || (ret & ADVERTISE_1000HALF))
+ extend_an_new_lp_cnt_limit(phydev);
+ }
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(mtk_gphy_cl22_read_status);
+
int mtk_phy_led_hw_is_supported(struct phy_device *phydev, u8 index, unsigned long rules,
unsigned long supported_triggers)
{
diff --git a/drivers/net/phy/mediatek/mtk.h b/drivers/net/phy/mediatek/mtk.h
index 10ee9bee2304..32fa3f183247 100644
--- a/drivers/net/phy/mediatek/mtk.h
+++ b/drivers/net/phy/mediatek/mtk.h
@@ -12,6 +12,20 @@
#define MTK_PHY_PAGE_STANDARD 0x0000
#define MTK_PHY_PAGE_EXTENDED_52B5 0x52b5
+/* Registers on Token Ring debug nodes */
+/* ch_addr = 0x0, node_addr = 0xf, data_addr = 0x2 */
+#define AN_STATE_MASK GENMASK(22, 19)
+#define AN_STATE_SHIFT 19
+#define AN_STATE_TX_DISABLE 1
+
+/* ch_addr = 0x0, node_addr = 0xf, data_addr = 0x3c */
+#define AN_NEW_LP_CNT_LIMIT_MASK GENMASK(23, 20)
+#define AUTO_NP_10XEN BIT(6)
+
+/* Registers on MDIO_MMD_VEND1 */
+#define MTK_PHY_LINK_STATUS_MISC (0xa2)
+#define MTK_PHY_FINAL_SPEED_1000 BIT(3)
+
/* Registers on MDIO_MMD_VEND2 */
#define MTK_PHY_LED0_ON_CTRL 0x24
#define MTK_PHY_LED1_ON_CTRL 0x26
@@ -75,6 +89,8 @@ void __tr_clr_bits(struct phy_device *phydev, u8 ch_addr, u8 node_addr, u8 data_
int mtk_phy_read_page(struct phy_device *phydev);
int mtk_phy_write_page(struct phy_device *phydev, int page);
+int mtk_gphy_cl22_read_status(struct phy_device *phydev);
+
int mtk_phy_led_hw_is_supported(struct phy_device *phydev, u8 index, unsigned long rules,
unsigned long supported_triggers);
int mtk_phy_led_hw_ctrl_set(struct phy_device *phydev, u8 index, unsigned long rules,
--
2.18.0
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH net-next v3 4/5] net: phy: mediatek: Extend 1G TX/RX link pulse time
2024-05-20 11:34 ` [PATCH net-next v3 4/5] net: phy: mediatek: Extend 1G TX/RX link pulse time Sky Huang
@ 2024-05-20 13:17 ` Andrew Lunn
0 siblings, 0 replies; 19+ messages in thread
From: Andrew Lunn @ 2024-05-20 13:17 UTC (permalink / raw)
To: Sky Huang
Cc: Heiner Kallweit, Russell King, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Daniel Golle, Qingfang Deng,
Matthias Brugger, AngeloGioacchino Del Regno, linux-kernel,
netdev, linux-arm-kernel, linux-mediatek, Steven Liu
> +static void extend_an_new_lp_cnt_limit(struct phy_device *phydev)
> +{
> + int mmd_read_ret;
> + int ret;
> + u32 reg_val;
> +
> + ret = read_poll_timeout(mmd_read_ret = phy_read_mmd, reg_val,
> + (mmd_read_ret < 0) || reg_val & MTK_PHY_FINAL_SPEED_1000,
> + 10000, 1000000, false, phydev,
> + MDIO_MMD_VEND1, MTK_PHY_LINK_STATUS_MISC);
> + if (mmd_read_ret < 0)
> + ret = mmd_read_ret;
> + /* If final_speed_1000 is raised, try to extend timeout period
> + * of auto downshift.
> + */
> + if (!ret) {
If you look at other Linux code, the general pattern is to look if a
function returned an error. If it does, either return immediately, or
jump to the end of the function where the cleanup is.
Since this is a void function:
> + if (mmd_read_ret < 0)
> + return;
And then you don't need the
> + if (!ret) {
> + tr_modify(phydev, 0x0, 0xf, 0x3c, AN_NEW_LP_CNT_LIMIT_MASK,
> + FIELD_PREP(AN_NEW_LP_CNT_LIMIT_MASK, 0xf));
> + mdelay(1500);
> +
> + ret = read_poll_timeout(mmd_read_ret = tr_read, reg_val,
> + (mmd_read_ret < 0) ||
> + (reg_val & AN_STATE_MASK) !=
> + (AN_STATE_TX_DISABLE << AN_STATE_SHIFT),
> + 10000, 1000000, false, phydev,
> + 0x0, 0xf, 0x2);
> +
> + if (mmd_read_ret < 0)
> + ret = mmd_read_ret;
> +
> + if (!ret) {
This if can also be removed.
> + mdelay(625);
> + tr_modify(phydev, 0x0, 0xf, 0x3c, AN_NEW_LP_CNT_LIMIT_MASK,
> + FIELD_PREP(AN_NEW_LP_CNT_LIMIT_MASK, 0x8));
> + mdelay(500);
> + tr_modify(phydev, 0x0, 0xf, 0x3c, AN_NEW_LP_CNT_LIMIT_MASK,
> + FIELD_PREP(AN_NEW_LP_CNT_LIMIT_MASK, 0xf));
> + }
> + }
One question i have is, should this really be a void function? What
does it mean if read_poll_timeout() returns an error? Why is it safe
to ignore it? Why not return the error?
> +}
> +
> +int mtk_gphy_cl22_read_status(struct phy_device *phydev)
> +{
> + int ret;
> +
> + ret = genphy_read_status(phydev);
> + if (ret)
> + return ret;
> +
> + if (phydev->autoneg == AUTONEG_ENABLE && !phydev->autoneg_complete) {
> + ret = phy_read(phydev, MII_CTRL1000);
> + if ((ret & ADVERTISE_1000FULL) || (ret & ADVERTISE_1000HALF))
> + extend_an_new_lp_cnt_limit(phydev);
> + }
> +
> + return 0;
If extend_an_new_lp_cnt_limit() fails, what does it mean? Do we
actually want mtk_gphy_cl22_read_status() to indicate something has
gone wrong? Or does extend_an_new_lp_cnt_limit() failing not matter?
Andrew
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH net-next v3 5/5] net: phy: add driver for built-in 2.5G ethernet PHY on MT7988
2024-05-20 11:34 [PATCH net-next v3 0/5] net: phy: mediatek: Introduce mtk-phy-lib and add 2.5Gphy support Sky Huang
` (3 preceding siblings ...)
2024-05-20 11:34 ` [PATCH net-next v3 4/5] net: phy: mediatek: Extend 1G TX/RX link pulse time Sky Huang
@ 2024-05-20 11:34 ` Sky Huang
2024-05-20 13:33 ` Andrew Lunn
2024-05-20 13:35 ` Andrew Lunn
2024-05-20 13:06 ` [PATCH net-next v3 0/5] net: phy: mediatek: Introduce mtk-phy-lib and add 2.5Gphy support Andrew Lunn
2024-05-21 9:02 ` Paolo Abeni
6 siblings, 2 replies; 19+ messages in thread
From: Sky Huang @ 2024-05-20 11:34 UTC (permalink / raw)
To: Andrew Lunn, Heiner Kallweit, Russell King, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Daniel Golle,
Qingfang Deng, SkyLake Huang, Matthias Brugger,
AngeloGioacchino Del Regno, linux-kernel, netdev,
linux-arm-kernel, linux-mediatek
Cc: Steven Liu, SkyLake.Huang
From: "SkyLake.Huang" <skylake.huang@mediatek.com>
v1:
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.
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.
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.
Signed-off-by: SkyLake.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 | 398 +++++++++++++++++++++++++++
4 files changed, 411 insertions(+)
create mode 100644 drivers/net/phy/mediatek/mtk-2p5ge.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 37e36b2f246e..27057d16cc29 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -13959,6 +13959,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 b11b19523ff1..88ca8819a8ff 100644
--- a/drivers/net/phy/mediatek/Kconfig
+++ b/drivers/net/phy/mediatek/Kconfig
@@ -25,3 +25,14 @@ config MEDIATEK_GE_SOC_PHY
the MT7981 and MT7988 SoCs. These PHYs need calibration data
present in the SoCs efuse and will dynamically calibrate VCM
(common-mode voltage) during startup.
+
+config MEDIATEK_2P5GE_PHY
+ tristate "MediaTek 2.5Gb Ethernet PHYs"
+ depends on (ARM64 && ARCH_MEDIATEK) || COMPILE_TEST
+ select MTK_NET_PHYLIB
+ help
+ Supports MediaTek SoC built-in 2.5Gb Ethernet PHYs.
+
+ This will load necessary firmware and add appropriate time delay.
+ Accelerate this procedure through internal pbus instead of MDIO
+ bus. Certain link-up issues will also be fixed here.
diff --git a/drivers/net/phy/mediatek/Makefile b/drivers/net/phy/mediatek/Makefile
index 814879d0abe5..c6db8abd2c9c 100644
--- a/drivers/net/phy/mediatek/Makefile
+++ b/drivers/net/phy/mediatek/Makefile
@@ -2,3 +2,4 @@
obj-$(CONFIG_MTK_NET_PHYLIB) += mtk-phy-lib.o
obj-$(CONFIG_MEDIATEK_GE_PHY) += mtk-ge.o
obj-$(CONFIG_MEDIATEK_GE_SOC_PHY) += mtk-ge-soc.o
+obj-$(CONFIG_MEDIATEK_2P5GE_PHY) += mtk-2p5ge.o
diff --git a/drivers/net/phy/mediatek/mtk-2p5ge.c b/drivers/net/phy/mediatek/mtk-2p5ge.c
new file mode 100644
index 000000000000..772adc0ca0dd
--- /dev/null
+++ b/drivers/net/phy/mediatek/mtk-2p5ge.c
@@ -0,0 +1,398 @@
+// 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 MTK phy page 1*/
+#define MTK_PHY_PAGE_EXTENDED_1 0x0001
+#define MTK_PHY_AUX_CTRL_AND_STATUS (0x14)
+#define MTK_PHY_ENABLE_DOWNSHIFT BIT(4)
+
+/* 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;
+ void __iomem *pmb_addr;
+ void __iomem *md32_en_cfg_base;
+ 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_config_init(struct phy_device *phydev)
+{
+ struct mtk_i2p5ge_phy_priv *priv = phydev->priv;
+ struct device *dev = &phydev->mdio.dev;
+ const struct firmware *fw;
+ struct pinctrl *pinctrl;
+ int ret, i;
+ u16 reg;
+
+ if (!priv->fw_loaded) {
+ if (!priv->md32_en_cfg_base || !priv->pmb_addr) {
+ dev_err(dev, "MD32_EN_CFG base & PMB addresses aren't valid\n");
+ return -EINVAL;
+ }
+
+ 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);
+ return ret;
+ }
+
+ if (fw->size != MT7988_2P5GE_PMB_SIZE) {
+ dev_err(dev, "Firmware size 0x%zx != 0x%x\n",
+ fw->size, MT7988_2P5GE_PMB_SIZE);
+ return -EINVAL;
+ }
+
+ reg = readw(priv->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)), priv->pmb_addr + i);
+ release_firmware(fw);
+
+ writew(reg & ~MD32_EN, priv->md32_en_cfg_base);
+ writew(reg | MD32_EN, priv->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;
+ iounmap(priv->md32_en_cfg_base);
+ iounmap(priv->pmb_addr);
+ }
+
+ /* 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);
+
+ 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");
+ return PTR_ERR(pinctrl);
+ }
+
+ 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 advertizing */
+ 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;
+
+ if (phydev->autoneg == AUTONEG_DISABLE)
+ return genphy_c45_pma_setup_forced(phydev);
+
+ 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;
+
+ /* We don't support HDX at MAC layer on mt7988. So mask phy's HDX capabilities here. */
+ 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) {
+ return -EOPNOTSUPP;
+ }
+
+ 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)
+ 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)
+{
+ bool blinking = false;
+ int err = 0;
+ struct mtk_i2p5ge_phy_priv *priv = phydev->priv;
+
+ 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)
+{
+ int err;
+ struct mtk_i2p5ge_phy_priv *priv = phydev->priv;
+
+ 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:
+ priv->pmb_addr = ioremap(MT7988_2P5GE_PMB_BASE, MT7988_2P5GE_PMB_LEN);
+ if (!priv->pmb_addr)
+ return -ENOMEM;
+ priv->md32_en_cfg_base = ioremap(MT7988_2P5GE_MD32_EN_CFG_BASE,
+ MT7988_2P5GE_MD32_EN_CFG_LEN);
+ if (!priv->md32_en_cfg_base)
+ return -ENOMEM;
+
+ /* 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 MT798x 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.18.0
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH net-next v3 5/5] net: phy: add driver for built-in 2.5G ethernet PHY on MT7988
2024-05-20 11:34 ` [PATCH net-next v3 5/5] net: phy: add driver for built-in 2.5G ethernet PHY on MT7988 Sky Huang
@ 2024-05-20 13:33 ` Andrew Lunn
2024-05-21 9:07 ` SkyLake Huang (黃啟澤)
2024-05-20 13:35 ` Andrew Lunn
1 sibling, 1 reply; 19+ messages in thread
From: Andrew Lunn @ 2024-05-20 13:33 UTC (permalink / raw)
To: Sky Huang
Cc: Heiner Kallweit, Russell King, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Daniel Golle, Qingfang Deng,
Matthias Brugger, AngeloGioacchino Del Regno, linux-kernel,
netdev, linux-arm-kernel, linux-mediatek, Steven Liu
> +static int mt798x_2p5ge_phy_config_init(struct phy_device *phydev)
> +{
> + struct mtk_i2p5ge_phy_priv *priv = phydev->priv;
> + struct device *dev = &phydev->mdio.dev;
> + const struct firmware *fw;
> + struct pinctrl *pinctrl;
> + int ret, i;
> + u16 reg;
> +
> + if (!priv->fw_loaded) {
> + if (!priv->md32_en_cfg_base || !priv->pmb_addr) {
> + dev_err(dev, "MD32_EN_CFG base & PMB addresses aren't valid\n");
> + return -EINVAL;
> + }
> +
https://www.kernel.org/doc/html/latest/process/coding-style.html
6) Functions
Functions should be short and sweet, and do just one thing. They
should fit on one or two screenfuls of text (the ISO/ANSI screen
size is 80x24, as we all know), and do one thing and do that well.
This is a big function, which does multiple things. Maybe pull the
downloading of firmware into a helper.
> + 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);
> + return ret;
> + }
> +
> + if (fw->size != MT7988_2P5GE_PMB_SIZE) {
> + dev_err(dev, "Firmware size 0x%zx != 0x%x\n",
> + fw->size, MT7988_2P5GE_PMB_SIZE);
> + return -EINVAL;
> + }
> +
> + reg = readw(priv->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)), priv->pmb_addr + i);
> + release_firmware(fw);
> +
> + writew(reg & ~MD32_EN, priv->md32_en_cfg_base);
> + writew(reg | MD32_EN, priv->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;
So there is no way to know if this has already happened? Maybe the
bootloader downloaded the firmware so it could TFTP boot? Linux will
download the firmware again, which is a waste of time.
> + iounmap(priv->md32_en_cfg_base);
> + iounmap(priv->pmb_addr);
> + }
> +
> + /* 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);
> +
> + pinctrl = devm_pinctrl_get_select(&phydev->mdio.dev, "i2p5gbe-led");
Calls to devm_pinctrl_get_select() is pretty unusual in drivers:
https://elixir.bootlin.com/linux/latest/C/ident/devm_pinctrl_get_select
Why is this needed? Generally, the DT file should describe the needed
pinmux setting, without needed anything additionally.
> +static int mt798x_2p5ge_phy_get_features(struct phy_device *phydev)
> +{
> + int ret;
> +
> + ret = genphy_c45_pma_read_abilities(phydev);
> + if (ret)
> + return ret;
> +
> + /* We don't support HDX at MAC layer on mt7988. So mask phy's HDX capabilities here. */
So you make it clear, the MAC does not support half duplex. The MAC
should then remove it, not the PHY.
> + 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) {
> + return -EOPNOTSUPP;
> + }
It is a bit late doing this now. The user requested this a long time
ago, and it will be hard to understand why it now returns EOPNOTSUPP.
You should check for AUTONEG_DISABLE in config_aneg() and return the
error there.
Andrew
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH net-next v3 5/5] net: phy: add driver for built-in 2.5G ethernet PHY on MT7988
2024-05-20 13:33 ` Andrew Lunn
@ 2024-05-21 9:07 ` SkyLake Huang (黃啟澤)
2024-05-21 17:20 ` Andrew Lunn
0 siblings, 1 reply; 19+ messages in thread
From: SkyLake Huang (黃啟澤) @ 2024-05-21 9:07 UTC (permalink / raw)
To: andrew@lunn.ch
Cc: linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, linux-mediatek@lists.infradead.org,
linux@armlinux.org.uk, kuba@kernel.org, pabeni@redhat.com,
edumazet@google.com, netdev@vger.kernel.org, dqfext@gmail.com,
Steven Liu (劉人豪), matthias.bgg@gmail.com,
davem@davemloft.net, hkallweit1@gmail.com, daniel@makrotopia.org,
angelogioacchino.delregno@collabora.com
On Mon, 2024-05-20 at 15:33 +0200, Andrew Lunn wrote:
>
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> > +static int mt798x_2p5ge_phy_config_init(struct phy_device
> *phydev)
> > +{
> > +struct mtk_i2p5ge_phy_priv *priv = phydev->priv;
> > +struct device *dev = &phydev->mdio.dev;
> > +const struct firmware *fw;
> > +struct pinctrl *pinctrl;
> > +int ret, i;
> > +u16 reg;
> > +
> > +if (!priv->fw_loaded) {
> > +if (!priv->md32_en_cfg_base || !priv->pmb_addr) {
> > +dev_err(dev, "MD32_EN_CFG base & PMB addresses aren't valid\n");
> > +return -EINVAL;
> > +}
> > +
>
> https://www.kernel.org/doc/html/latest/process/coding-style.html
>
> 6) Functions
>
> Functions should be short and sweet, and do just one thing. They
> should fit on one or two screenfuls of text (the ISO/ANSI screen
> size is 80x24, as we all know), and do one thing and do that well.
>
> This is a big function, which does multiple things. Maybe pull the
> downloading of firmware into a helper.
>
Agree. I'll move this part to another function.
> > +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);
> > +return ret;
> > +}
> > +
> > +if (fw->size != MT7988_2P5GE_PMB_SIZE) {
> > +dev_err(dev, "Firmware size 0x%zx != 0x%x\n",
> > +fw->size, MT7988_2P5GE_PMB_SIZE);
> > +return -EINVAL;
> > +}
> > +
> > +reg = readw(priv->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)), priv->pmb_addr + i);
> > +release_firmware(fw);
> > +
> > +writew(reg & ~MD32_EN, priv->md32_en_cfg_base);
> > +writew(reg | MD32_EN, priv->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;
>
> So there is no way to know if this has already happened? Maybe the
> bootloader downloaded the firmware so it could TFTP boot? Linux will
> download the firmware again, which is a waste of time.
>
Normal MTK's internal 2.5Gphy firmware loading procedure will be like:
1. request firmware
2. write firmware to correspoding memory address via sbus2pbus(faster
than MDIO)
3. Trigger MCU to run firmware by setting MD32_EN bit.
So logically, we can determine if firmware is loaded by
bootloader(Uboot) or not by reading MD32_EN bit (step 3 above).
However, if step2 is bypassed, MD32_EN bit can still be set if CL22
reset (BMCR_RESET) is triggered by user. In this case, internal
2.5Gphy's MCU is running nothing.
That is to say, for safety, we need to load firmware again right
atfer booting into Linux kernel. Actually, this takes just about 11ms.
> > +iounmap(priv->md32_en_cfg_base);
> > +iounmap(priv->pmb_addr);
> > +}
> > +
> > +/* 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);
> > +
> > +pinctrl = devm_pinctrl_get_select(&phydev->mdio.dev, "i2p5gbe-
> led");
>
> Calls to devm_pinctrl_get_select() is pretty unusual in drivers:
>
>
https://elixir.bootlin.com/linux/latest/C/ident/devm_pinctrl_get_select
>
> Why is this needed? Generally, the DT file should describe the needed
> pinmux setting, without needed anything additionally.
>
This is needed because we need to switch to i2p5gbe-led pinmux group
after we set correct polarity. Or LED may blink unexpectedly.
> > +static int mt798x_2p5ge_phy_get_features(struct phy_device
> *phydev)
> > +{
> > +int ret;
> > +
> > +ret = genphy_c45_pma_read_abilities(phydev);
> > +if (ret)
> > +return ret;
> > +
> > +/* We don't support HDX at MAC layer on mt7988. So mask phy's HDX
> capabilities here. */
>
> So you make it clear, the MAC does not support half duplex. The MAC
> should then remove it, not the PHY.
>
> > +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) {
> > +return -EOPNOTSUPP;
> > +}
>
> It is a bit late doing this now. The user requested this a long time
> ago, and it will be hard to understand why it now returns EOPNOTSUPP.
> You should check for AUTONEG_DISABLE in config_aneg() and return the
> error there.
>
> Andrew
Maybe I shouldn't return EOPNOTSUPP in config_aneg directly?
In this way, _phy_state_machine will be broken if I trigger "$ ethtool
-s ethtool eth1 autoneg off"
[ 520.781368] ------------[ cut here ]------------
root@OpenWrt:/# [ 520.785978] _phy_start_aneg+0x0/0xa4: returned: -95
[ 520.792263] WARNING: CPU: 3 PID: 423 at drivers/net/phy/phy.c:1254 _phy_state_machine+0x78/0x258
[ 520.801039] Modules linked in:
[ 520.804087] CPU: 3 PID: 423 Comm: kworker/u16:4 Tainted: G W 6.8.0-rc7-next-20240306-bpi-r3+ #102
[ 520.814335] Hardware name: MediaTek MT7988A Reference Board (DT)
[ 520.820330] Workqueue: events_power_efficient phy_state_machine
[ 520.826240] pstate: 60400005 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[ 520.833190] pc : _phy_state_machine+0x78/0x258
[ 520.837623] lr : _phy_state_machine+0x78/0x258
[ 520.842056] sp : ffff800084b7bd30
[ 520.845360] x29: ffff800084b7bd30 x28: 0000000000000000 x27: 0000000000000000
[ 520.852487] x26: ffff000000c56900 x25: ffff000000c56980 x24: ffff000000012ac0
[ 520.859613] x23: ffff00000001d005 x22: ffff000000fdf000 x21: 0000000000000001
[ 520.866738] x20: 0000000000000004 x19: ffff000003a90000 x18: ffffffffffffffff
[ 520.873864] x17: 0000000000000000 x16: 0000000000000000 x15: ffff800104b7b977
[ 520.880988] x14: 0000000000000000 x13: 0000000000000183 x12: 00000000ffffffea
[ 520.888114] x11: 0000000000000001 x10: 0000000000000001 x9 : ffff8000837222f0
[ 520.895239] x8 : 0000000000017fe8 x7 : c0000000ffffefff x6 : 0000000000000001
[ 520.902365] x5 : 0000000000000000 x4 : 0000000000000000 x3 : 0000000000000000
[ 520.909490] x2 : 0000000000000000 x1 : 0000000000000000 x0 : ffff000004120000
[ 520.916615] Call trace:
[ 520.919052] _phy_state_machine+0x78/0x258
[ 520.923139] phy_state_machine+0x2c/0x80
[ 520.927051] process_one_work+0x178/0x31c
[ 520.931054] worker_thread+0x280/0x494
[ 520.934795] kthread+0xe4/0xe8
[ 520.937841] ret_from_fork+0x10/0x20
[ 520.941408] ---[ end trace 0000000000000000 ]---
Now I prefer to give a warning like this, in
mt798x_2p5ge_phy_config_aneg():
if (phydev->autoneg == AUTONEG_DISABLE) {
dev_warn(&phydev->mdio.dev, "Once AN off is set, this phy can't
link.\n");
return genphy_c45_pma_setup_forced(phydev);
}
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH net-next v3 5/5] net: phy: add driver for built-in 2.5G ethernet PHY on MT7988
2024-05-21 9:07 ` SkyLake Huang (黃啟澤)
@ 2024-05-21 17:20 ` Andrew Lunn
2024-05-24 4:38 ` SkyLake Huang (黃啟澤)
0 siblings, 1 reply; 19+ messages in thread
From: Andrew Lunn @ 2024-05-21 17:20 UTC (permalink / raw)
To: SkyLake Huang (黃啟澤)
Cc: linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, linux-mediatek@lists.infradead.org,
linux@armlinux.org.uk, kuba@kernel.org, pabeni@redhat.com,
edumazet@google.com, netdev@vger.kernel.org, dqfext@gmail.com,
Steven Liu (劉人豪), matthias.bgg@gmail.com,
davem@davemloft.net, hkallweit1@gmail.com, daniel@makrotopia.org,
angelogioacchino.delregno@collabora.com
> That is to say, for safety, we need to load firmware again right
> atfer booting into Linux kernel. Actually, this takes just about 11ms.
Since this is only 11ms, its not a big deal. If this was going over
MDIO it would be much slower and then it does become significant.
> > > +/* 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);
> > > +
> > > +pinctrl = devm_pinctrl_get_select(&phydev->mdio.dev, "i2p5gbe-
> > led");
> >
> > Calls to devm_pinctrl_get_select() is pretty unusual in drivers:
> >
> >
> https://elixir.bootlin.com/linux/latest/C/ident/devm_pinctrl_get_select
> >
> > Why is this needed? Generally, the DT file should describe the needed
> > pinmux setting, without needed anything additionally.
> >
> This is needed because we need to switch to i2p5gbe-led pinmux group
> after we set correct polarity. Or LED may blink unexpectedly.
Since this is unusual, you should add a comment. Also, does the device
tree binding explain this? I expect most DT authors are used to
listing all the needed pins in the default pinmux node, and so will do
that, unless there is a comment in the binding advising against it.
> > It is a bit late doing this now. The user requested this a long time
> > ago, and it will be hard to understand why it now returns EOPNOTSUPP.
> > You should check for AUTONEG_DISABLE in config_aneg() and return the
> > error there.
> >
> > Andrew
> Maybe I shouldn't return EOPNOTSUPP in config_aneg directly?
> In this way, _phy_state_machine will be broken if I trigger "$ ethtool
> -s ethtool eth1 autoneg off"
>
> [ 520.781368] ------------[ cut here ]------------
> root@OpenWrt:/# [ 520.785978] _phy_start_aneg+0x0/0xa4: returned: -95
> [ 520.792263] WARNING: CPU: 3 PID: 423 at drivers/net/phy/phy.c:1254 _phy_state_machine+0x78/0x258
> [ 520.801039] Modules linked in:
> [ 520.804087] CPU: 3 PID: 423 Comm: kworker/u16:4 Tainted: G W 6.8.0-rc7-next-20240306-bpi-r3+ #102
> [ 520.814335] Hardware name: MediaTek MT7988A Reference Board (DT)
> [ 520.820330] Workqueue: events_power_efficient phy_state_machine
> [ 520.826240] pstate: 60400005 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> [ 520.833190] pc : _phy_state_machine+0x78/0x258
> [ 520.837623] lr : _phy_state_machine+0x78/0x258
> [ 520.842056] sp : ffff800084b7bd30
> [ 520.845360] x29: ffff800084b7bd30 x28: 0000000000000000 x27: 0000000000000000
> [ 520.852487] x26: ffff000000c56900 x25: ffff000000c56980 x24: ffff000000012ac0
> [ 520.859613] x23: ffff00000001d005 x22: ffff000000fdf000 x21: 0000000000000001
> [ 520.866738] x20: 0000000000000004 x19: ffff000003a90000 x18: ffffffffffffffff
> [ 520.873864] x17: 0000000000000000 x16: 0000000000000000 x15: ffff800104b7b977
> [ 520.880988] x14: 0000000000000000 x13: 0000000000000183 x12: 00000000ffffffea
> [ 520.888114] x11: 0000000000000001 x10: 0000000000000001 x9 : ffff8000837222f0
> [ 520.895239] x8 : 0000000000017fe8 x7 : c0000000ffffefff x6 : 0000000000000001
> [ 520.902365] x5 : 0000000000000000 x4 : 0000000000000000 x3 : 0000000000000000
> [ 520.909490] x2 : 0000000000000000 x1 : 0000000000000000 x0 : ffff000004120000
> [ 520.916615] Call trace:
> [ 520.919052] _phy_state_machine+0x78/0x258
> [ 520.923139] phy_state_machine+0x2c/0x80
> [ 520.927051] process_one_work+0x178/0x31c
> [ 520.931054] worker_thread+0x280/0x494
> [ 520.934795] kthread+0xe4/0xe8
> [ 520.937841] ret_from_fork+0x10/0x20
> [ 520.941408] ---[ end trace 0000000000000000 ]---
>
> Now I prefer to give a warning like this, in
> mt798x_2p5ge_phy_config_aneg():
> if (phydev->autoneg == AUTONEG_DISABLE) {
> dev_warn(&phydev->mdio.dev, "Once AN off is set, this phy can't
> link.\n");
> return genphy_c45_pma_setup_forced(phydev);
> }
That is ugly.
Ideally we should fix phylib to support a PHY which cannot do fixed
link. I suggest you first look at phy_ethtool_ksettings_set() and see
what it does if passed cmd->base.autoneg == True, but
ETHTOOL_LINK_MODE_Autoneg_BIT is not set in supported, because the PHY
does not support autoneg. Is that handled? Does it return EOPNOTSUPP?
Understanding this might help you implement the opposite.
The opposite is however not easy. There is no linkmode bit indicating
a PHY can do forced settings. The BMSR has a bit indicating the PHY is
capable of auto-neg, which is used to set
ETHTOOL_LINK_MODE_Autoneg_BIT. However there is no bit to indicate the
PHY supports forced configuration. The standard just assumes all PHYs
which are standard conforming can do forced settings. And i think this
is the first PHY we have come across which is broken like this.
So maybe we cannot fix this in phylib. In that case, the MAC drivers
ksetting_set() should check if the user is attempting to disable
autoneg, and return -EOPNOTSUPP. And i would keep the stack trace
above, which will help developers understand they need the MAC to help
out work around the broken PHY. You can probably also simplify the PHY
driver, take out any code which tries to handle forced settings.
Andrew
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH net-next v3 5/5] net: phy: add driver for built-in 2.5G ethernet PHY on MT7988
2024-05-21 17:20 ` Andrew Lunn
@ 2024-05-24 4:38 ` SkyLake Huang (黃啟澤)
0 siblings, 0 replies; 19+ messages in thread
From: SkyLake Huang (黃啟澤) @ 2024-05-24 4:38 UTC (permalink / raw)
To: andrew@lunn.ch
Cc: linux-kernel@vger.kernel.org, linux-mediatek@lists.infradead.org,
linux@armlinux.org.uk, kuba@kernel.org, pabeni@redhat.com,
edumazet@google.com, linux-arm-kernel@lists.infradead.org,
netdev@vger.kernel.org, dqfext@gmail.com, matthias.bgg@gmail.com,
Steven Liu (劉人豪), davem@davemloft.net,
hkallweit1@gmail.com, angelogioacchino.delregno@collabora.com,
daniel@makrotopia.org
On Tue, 2024-05-21 at 19:20 +0200, Andrew Lunn wrote:
> > > > +/* 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);
> > > > +
> > > > +pinctrl = devm_pinctrl_get_select(&phydev->mdio.dev, "i2p5gbe-
> > > led");
> > >
> > > Calls to devm_pinctrl_get_select() is pretty unusual in drivers:
> > >
> > >
> >
> https://elixir.bootlin.com/linux/latest/C/ident/devm_pinctrl_get_select
> > >
> > > Why is this needed? Generally, the DT file should describe the
> needed
> > > pinmux setting, without needed anything additionally.
> > >
> > This is needed because we need to switch to i2p5gbe-led pinmux
> group
> > after we set correct polarity. Or LED may blink unexpectedly.
>
> Since this is unusual, you should add a comment. Also, does the
> device
> tree binding explain this? I expect most DT authors are used to
> listing all the needed pins in the default pinmux node, and so will
> do
> that, unless there is a comment in the binding advising against it.
>
Then I'll add comments and remove "returning error if picntrl switching
fails" like this:
/* 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");
}
Actually current drivers/net/phy/mediatek-ge-soc.c has the same
mechanism, which utilizing gbe-led pinmux group in
mt7988_phy_fix_leds_polarities():
/* Only now setup pinctrl to avoid bogus blinking */
pinctrl = devm_pinctrl_get_select(&phydev->mdio.dev, "gbe-led");
if (IS_ERR(pinctrl))
dev_err(&phydev->mdio.bus->dev, "Failed to setup PHY LED
pinctrl\n");
Actually those pinmux groups exist in drivers/pinctrl/mediatek/pinctrl-
mt7988 of current openWRT tree:
https://git.openwrt.org/?p=openwrt/openwrt.git;a=blob;f=target/linux/mediatek/files-6.1/drivers/pinctrl/mediatek/pinctrl-mt7988.c;h=9f9291124522c916c4e92f1598e0be44c3badad5;hb=f16dc4b42fb265affb2298e815a7ce0a13d60da6
I believe this will be upstreamed later.
> > > It is a bit late doing this now. The user requested this a long
> time
> > > ago, and it will be hard to understand why it now returns
> EOPNOTSUPP.
> > > You should check for AUTONEG_DISABLE in config_aneg() and return
> the
> > > error there.
> > >
> > > Andrew
> > Maybe I shouldn't return EOPNOTSUPP in config_aneg directly?
> > In this way, _phy_state_machine will be broken if I trigger "$
> ethtool
> > -s ethtool eth1 autoneg off"
> >
> > [ 520.781368] ------------[ cut here ]------------
> > root@OpenWrt:/# [ 520.785978] _phy_start_aneg+0x0/0xa4: returned:
> -95
> > [ 520.792263] WARNING: CPU: 3 PID: 423 at
> drivers/net/phy/phy.c:1254 _phy_state_machine+0x78/0x258
> > [ 520.801039] Modules linked in:
> > [ 520.804087] CPU: 3 PID: 423 Comm: kworker/u16:4 Tainted:
> G W 6.8.0-rc7-next-20240306-bpi-r3+ #102
> > [ 520.814335] Hardware name: MediaTek MT7988A Reference Board (DT)
> > [ 520.820330] Workqueue: events_power_efficient phy_state_machine
> > [ 520.826240] pstate: 60400005 (nZCv daif +PAN -UAO -TCO -DIT
> -SSBS BTYPE=--)
> > [ 520.833190] pc : _phy_state_machine+0x78/0x258
> > [ 520.837623] lr : _phy_state_machine+0x78/0x258
> > [ 520.842056] sp : ffff800084b7bd30
> > [ 520.845360] x29: ffff800084b7bd30 x28: 0000000000000000 x27:
> 0000000000000000
> > [ 520.852487] x26: ffff000000c56900 x25: ffff000000c56980 x24:
> ffff000000012ac0
> > [ 520.859613] x23: ffff00000001d005 x22: ffff000000fdf000 x21:
> 0000000000000001
> > [ 520.866738] x20: 0000000000000004 x19: ffff000003a90000 x18:
> ffffffffffffffff
> > [ 520.873864] x17: 0000000000000000 x16: 0000000000000000 x15:
> ffff800104b7b977
> > [ 520.880988] x14: 0000000000000000 x13: 0000000000000183 x12:
> 00000000ffffffea
> > [ 520.888114] x11: 0000000000000001 x10: 0000000000000001 x9 :
> ffff8000837222f0
> > [ 520.895239] x8 : 0000000000017fe8 x7 : c0000000ffffefff x6 :
> 0000000000000001
> > [ 520.902365] x5 : 0000000000000000 x4 : 0000000000000000 x3 :
> 0000000000000000
> > [ 520.909490] x2 : 0000000000000000 x1 : 0000000000000000 x0 :
> ffff000004120000
> > [ 520.916615] Call trace:
> > [ 520.919052] _phy_state_machine+0x78/0x258
> > [ 520.923139] phy_state_machine+0x2c/0x80
> > [ 520.927051] process_one_work+0x178/0x31c
> > [ 520.931054] worker_thread+0x280/0x494
> > [ 520.934795] kthread+0xe4/0xe8
> > [ 520.937841] ret_from_fork+0x10/0x20
> > [ 520.941408] ---[ end trace 0000000000000000 ]---
> >
> > Now I prefer to give a warning like this, in
> > mt798x_2p5ge_phy_config_aneg():
> > if (phydev->autoneg == AUTONEG_DISABLE) {
> > dev_warn(&phydev->mdio.dev, "Once AN off is set, this phy can't
> > link.\n");
> > return genphy_c45_pma_setup_forced(phydev);
> > }
>
> That is ugly.
>
> Ideally we should fix phylib to support a PHY which cannot do fixed
> link. I suggest you first look at phy_ethtool_ksettings_set() and see
> what it does if passed cmd->base.autoneg == True, but
> ETHTOOL_LINK_MODE_Autoneg_BIT is not set in supported, because the
> PHY
> does not support autoneg. Is that handled? Does it return EOPNOTSUPP?
> Understanding this might help you implement the opposite.
>
> The opposite is however not easy. There is no linkmode bit indicating
> a PHY can do forced settings. The BMSR has a bit indicating the PHY
> is
> capable of auto-neg, which is used to set
> ETHTOOL_LINK_MODE_Autoneg_BIT. However there is no bit to indicate
> the
> PHY supports forced configuration. The standard just assumes all PHYs
> which are standard conforming can do forced settings. And i think
> this
> is the first PHY we have come across which is broken like this.
>
Thanks for your clear explanation.
> So maybe we cannot fix this in phylib. In that case, the MAC drivers
> ksetting_set() should check if the user is attempting to disable
> autoneg, and return -EOPNOTSUPP. And i would keep the stack trace
> above, which will help developers understand they need the MAC to
> help
> out work around the broken PHY. You can probably also simplify the
> PHY
> driver, take out any code which tries to handle forced settings.
>
> Andrew
>
According to this, I think maybe we may need to patch
drivers/net/ethernet/mediatek/mtk_eth_soc.c later. I'll fix
mt798x_2p5ge_phy_config_aneg() like this first:
if (phydev->autoneg == AUTONEG_DISABLE)
return -EOPNOTSUPP;
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH net-next v3 5/5] net: phy: add driver for built-in 2.5G ethernet PHY on MT7988
2024-05-20 11:34 ` [PATCH net-next v3 5/5] net: phy: add driver for built-in 2.5G ethernet PHY on MT7988 Sky Huang
2024-05-20 13:33 ` Andrew Lunn
@ 2024-05-20 13:35 ` Andrew Lunn
2024-05-21 8:17 ` SkyLake Huang (黃啟澤)
1 sibling, 1 reply; 19+ messages in thread
From: Andrew Lunn @ 2024-05-20 13:35 UTC (permalink / raw)
To: Sky Huang
Cc: Heiner Kallweit, Russell King, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Daniel Golle, Qingfang Deng,
Matthias Brugger, AngeloGioacchino Del Regno, linux-kernel,
netdev, linux-arm-kernel, linux-mediatek, Steven Liu
> +static int mt798x_2p5ge_phy_config_init(struct phy_device *phydev)
> +{
> + struct mtk_i2p5ge_phy_priv *priv = phydev->priv;
> + struct device *dev = &phydev->mdio.dev;
> + const struct firmware *fw;
> + struct pinctrl *pinctrl;
> + int ret, i;
> + u16 reg;
> +
> + if (!priv->fw_loaded) {
> + if (!priv->md32_en_cfg_base || !priv->pmb_addr) {
> + dev_err(dev, "MD32_EN_CFG base & PMB addresses aren't valid\n");
> + return -EINVAL;
> + }
...
> +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:
> + priv->pmb_addr = ioremap(MT7988_2P5GE_PMB_BASE, MT7988_2P5GE_PMB_LEN);
> + if (!priv->pmb_addr)
> + return -ENOMEM;
> + priv->md32_en_cfg_base = ioremap(MT7988_2P5GE_MD32_EN_CFG_BASE,
> + MT7988_2P5GE_MD32_EN_CFG_LEN);
> + if (!priv->md32_en_cfg_base)
> + return -ENOMEM;
> +
> + /* 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;
> + }
How can priv->md32_en_cfg_base or priv->pmb_addr not be set in
mt798x_2p5ge_phy_config_init()
Andrew
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH net-next v3 5/5] net: phy: add driver for built-in 2.5G ethernet PHY on MT7988
2024-05-20 13:35 ` Andrew Lunn
@ 2024-05-21 8:17 ` SkyLake Huang (黃啟澤)
2024-05-21 17:28 ` Andrew Lunn
0 siblings, 1 reply; 19+ messages in thread
From: SkyLake Huang (黃啟澤) @ 2024-05-21 8:17 UTC (permalink / raw)
To: andrew@lunn.ch
Cc: linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, linux-mediatek@lists.infradead.org,
linux@armlinux.org.uk, kuba@kernel.org, pabeni@redhat.com,
edumazet@google.com, netdev@vger.kernel.org, dqfext@gmail.com,
Steven Liu (劉人豪), matthias.bgg@gmail.com,
davem@davemloft.net, hkallweit1@gmail.com, daniel@makrotopia.org,
angelogioacchino.delregno@collabora.com
On Mon, 2024-05-20 at 15:35 +0200, Andrew Lunn wrote:
>
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> > +static int mt798x_2p5ge_phy_config_init(struct phy_device
> *phydev)
> > +{
> > +struct mtk_i2p5ge_phy_priv *priv = phydev->priv;
> > +struct device *dev = &phydev->mdio.dev;
> > +const struct firmware *fw;
> > +struct pinctrl *pinctrl;
> > +int ret, i;
> > +u16 reg;
> > +
> > +if (!priv->fw_loaded) {
> > +if (!priv->md32_en_cfg_base || !priv->pmb_addr) {
> > +dev_err(dev, "MD32_EN_CFG base & PMB addresses aren't valid\n");
> > +return -EINVAL;
> > +}
>
> ...
>
> > +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:
> > +priv->pmb_addr = ioremap(MT7988_2P5GE_PMB_BASE,
> MT7988_2P5GE_PMB_LEN);
> > +if (!priv->pmb_addr)
> > +return -ENOMEM;
> > +priv->md32_en_cfg_base = ioremap(MT7988_2P5GE_MD32_EN_CFG_BASE,
> > + MT7988_2P5GE_MD32_EN_CFG_LEN);
> > +if (!priv->md32_en_cfg_base)
> > +return -ENOMEM;
> > +
> > +/* 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;
> > +}
>
> How can priv->md32_en_cfg_base or priv->pmb_addr not be set in
> mt798x_2p5ge_phy_config_init()
>
> Andrew
Use command "$ifconfig eth1 down" and then "$ifconfig eth1 up",
mt798x_2p5ge_phy_config_init() will be called again and again. priv-
>md32_en_cfg_base & priv->pmb_addr are released after first firmware
loading. So just check these two values again for safety once priv-
>fw_loaded is overrided unexpectedly.
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH net-next v3 5/5] net: phy: add driver for built-in 2.5G ethernet PHY on MT7988
2024-05-21 8:17 ` SkyLake Huang (黃啟澤)
@ 2024-05-21 17:28 ` Andrew Lunn
2024-05-24 4:44 ` SkyLake Huang (黃啟澤)
0 siblings, 1 reply; 19+ messages in thread
From: Andrew Lunn @ 2024-05-21 17:28 UTC (permalink / raw)
To: SkyLake Huang (黃啟澤)
Cc: linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, linux-mediatek@lists.infradead.org,
linux@armlinux.org.uk, kuba@kernel.org, pabeni@redhat.com,
edumazet@google.com, netdev@vger.kernel.org, dqfext@gmail.com,
Steven Liu (劉人豪), matthias.bgg@gmail.com,
davem@davemloft.net, hkallweit1@gmail.com, daniel@makrotopia.org,
angelogioacchino.delregno@collabora.com
On Tue, May 21, 2024 at 08:17:32AM +0000, SkyLake Huang (黃啟澤) wrote:
> On Mon, 2024-05-20 at 15:35 +0200, Andrew Lunn wrote:
> >
> > External email : Please do not click links or open attachments until
> > you have verified the sender or the content.
> > > +static int mt798x_2p5ge_phy_config_init(struct phy_device
> > *phydev)
> > > +{
> > > +struct mtk_i2p5ge_phy_priv *priv = phydev->priv;
> > > +struct device *dev = &phydev->mdio.dev;
> > > +const struct firmware *fw;
> > > +struct pinctrl *pinctrl;
> > > +int ret, i;
> > > +u16 reg;
> > > +
> > > +if (!priv->fw_loaded) {
> > > +if (!priv->md32_en_cfg_base || !priv->pmb_addr) {
> > > +dev_err(dev, "MD32_EN_CFG base & PMB addresses aren't valid\n");
> > > +return -EINVAL;
> > > +}
> >
> > ...
> >
> > > +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:
> > > +priv->pmb_addr = ioremap(MT7988_2P5GE_PMB_BASE,
> > MT7988_2P5GE_PMB_LEN);
> > > +if (!priv->pmb_addr)
> > > +return -ENOMEM;
> > > +priv->md32_en_cfg_base = ioremap(MT7988_2P5GE_MD32_EN_CFG_BASE,
> > > + MT7988_2P5GE_MD32_EN_CFG_LEN);
> > > +if (!priv->md32_en_cfg_base)
> > > +return -ENOMEM;
> > > +
> > > +/* 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;
> > > +}
> >
> > How can priv->md32_en_cfg_base or priv->pmb_addr not be set in
> > mt798x_2p5ge_phy_config_init()
> >
> > Andrew
> Use command "$ifconfig eth1 down" and then "$ifconfig eth1 up",
> mt798x_2p5ge_phy_config_init() will be called again and again. priv-
> >md32_en_cfg_base & priv->pmb_addr are released after first firmware
> loading. So just check these two values again for safety once priv-
> >fw_loaded is overrided unexpectedly.
So the code is unsymmetrical. The memory is mapped in
mt798x_2p5ge_phy_probe() but unmapped in
mt798x_2p5ge_phy_config_init(). It would be better style to unmap it
in mt798x_2p5ge_phy_remove(). Alternatively, just map it when
downloading firmware, and unmap it straight afterwards.
Also, we generally discourage defensive programming. It is much better
to actually understand the code and know something is not possible.
Andrew
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH net-next v3 5/5] net: phy: add driver for built-in 2.5G ethernet PHY on MT7988
2024-05-21 17:28 ` Andrew Lunn
@ 2024-05-24 4:44 ` SkyLake Huang (黃啟澤)
0 siblings, 0 replies; 19+ messages in thread
From: SkyLake Huang (黃啟澤) @ 2024-05-24 4:44 UTC (permalink / raw)
To: andrew@lunn.ch
Cc: linux-kernel@vger.kernel.org, linux-mediatek@lists.infradead.org,
linux@armlinux.org.uk, kuba@kernel.org, pabeni@redhat.com,
edumazet@google.com, linux-arm-kernel@lists.infradead.org,
netdev@vger.kernel.org, dqfext@gmail.com, matthias.bgg@gmail.com,
Steven Liu (劉人豪), davem@davemloft.net,
hkallweit1@gmail.com, angelogioacchino.delregno@collabora.com,
daniel@makrotopia.org
On Tue, 2024-05-21 at 19:28 +0200, Andrew Lunn wrote:
>
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> On Tue, May 21, 2024 at 08:17:32AM +0000, SkyLake Huang (黃啟澤) wrote:
> > On Mon, 2024-05-20 at 15:35 +0200, Andrew Lunn wrote:
> > >
> > > External email : Please do not click links or open attachments
> until
> > > you have verified the sender or the content.
> > > > +static int mt798x_2p5ge_phy_config_init(struct phy_device
> > > *phydev)
> > > > +{
> > > > +struct mtk_i2p5ge_phy_priv *priv = phydev->priv;
> > > > +struct device *dev = &phydev->mdio.dev;
> > > > +const struct firmware *fw;
> > > > +struct pinctrl *pinctrl;
> > > > +int ret, i;
> > > > +u16 reg;
> > > > +
> > > > +if (!priv->fw_loaded) {
> > > > +if (!priv->md32_en_cfg_base || !priv->pmb_addr) {
> > > > +dev_err(dev, "MD32_EN_CFG base & PMB addresses aren't
> valid\n");
> > > > +return -EINVAL;
> > > > +}
> > >
> > > ...
> > >
> > > > +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:
> > > > +priv->pmb_addr = ioremap(MT7988_2P5GE_PMB_BASE,
> > > MT7988_2P5GE_PMB_LEN);
> > > > +if (!priv->pmb_addr)
> > > > +return -ENOMEM;
> > > > +priv->md32_en_cfg_base =
> ioremap(MT7988_2P5GE_MD32_EN_CFG_BASE,
> > > > + MT7988_2P5GE_MD32_EN_CFG_LEN);
> > > > +if (!priv->md32_en_cfg_base)
> > > > +return -ENOMEM;
> > > > +
> > > > +/* 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;
> > > > +}
> > >
> > > How can priv->md32_en_cfg_base or priv->pmb_addr not be set in
> > > mt798x_2p5ge_phy_config_init()
> > >
> > > Andrew
> > Use command "$ifconfig eth1 down" and then "$ifconfig eth1 up",
> > mt798x_2p5ge_phy_config_init() will be called again and again.
> priv-
> > >md32_en_cfg_base & priv->pmb_addr are released after first
> firmware
> > loading. So just check these two values again for safety once priv-
> > >fw_loaded is overrided unexpectedly.
>
> So the code is unsymmetrical. The memory is mapped in
> mt798x_2p5ge_phy_probe() but unmapped in
> mt798x_2p5ge_phy_config_init(). It would be better style to unmap it
> in mt798x_2p5ge_phy_remove(). Alternatively, just map it when
> downloading firmware, and unmap it straight afterwards.
>
> Also, we generally discourage defensive programming. It is much
> better
> to actually understand the code and know something is not possible.
>
> Andrew
OK. I'll fix this in v5. Map & unmap it locally in
mt798x_2p5ge_phy_config_aneg()
-> mt798x_2p5ge_hy_load_fw()
Sky
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH net-next v3 0/5] net: phy: mediatek: Introduce mtk-phy-lib and add 2.5Gphy support
2024-05-20 11:34 [PATCH net-next v3 0/5] net: phy: mediatek: Introduce mtk-phy-lib and add 2.5Gphy support Sky Huang
` (4 preceding siblings ...)
2024-05-20 11:34 ` [PATCH net-next v3 5/5] net: phy: add driver for built-in 2.5G ethernet PHY on MT7988 Sky Huang
@ 2024-05-20 13:06 ` Andrew Lunn
2024-05-21 9:02 ` Paolo Abeni
6 siblings, 0 replies; 19+ messages in thread
From: Andrew Lunn @ 2024-05-20 13:06 UTC (permalink / raw)
To: Sky Huang
Cc: Heiner Kallweit, Russell King, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Daniel Golle, Qingfang Deng,
Matthias Brugger, AngeloGioacchino Del Regno, linux-kernel,
netdev, linux-arm-kernel, linux-mediatek, Steven Liu
On Mon, May 20, 2024 at 07:34:51PM +0800, Sky Huang wrote:
> From: "SkyLake.Huang" <skylake.huang@mediatek.com>
>
> Re-organize MTK ethernet phy drivers and integrate common manipulations
> into mtk-phy-lib. Also, add support for build-in 2.5Gphy on MT7988.
>
> v2:
> - Apply correct PATCH tag.
> - Break LED/Token ring/Extend-link-pulse-time features into 3 patches.
> - Fix contents according to v1 comments.
>
> v3:
> - Fix patch 4/5 & 5/5 according to v2 comments.
Ideally state what you actually changed. The purpose of this history
is to remind reviewers what they said, so they can
1) Check if anything has been missed
2) Check if the changes made fit to the review comments.
So please include a one line summary per change made.
> - Rebase code and now this patch series can apply to net-next tree.
https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html#netdev-faq
Andrew
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH net-next v3 0/5] net: phy: mediatek: Introduce mtk-phy-lib and add 2.5Gphy support
2024-05-20 11:34 [PATCH net-next v3 0/5] net: phy: mediatek: Introduce mtk-phy-lib and add 2.5Gphy support Sky Huang
` (5 preceding siblings ...)
2024-05-20 13:06 ` [PATCH net-next v3 0/5] net: phy: mediatek: Introduce mtk-phy-lib and add 2.5Gphy support Andrew Lunn
@ 2024-05-21 9:02 ` Paolo Abeni
6 siblings, 0 replies; 19+ messages in thread
From: Paolo Abeni @ 2024-05-21 9:02 UTC (permalink / raw)
To: Sky Huang, Andrew Lunn, Heiner Kallweit, Russell King,
David S. Miller, Eric Dumazet, Jakub Kicinski, Daniel Golle,
Qingfang Deng, Matthias Brugger, AngeloGioacchino Del Regno,
linux-kernel, netdev, linux-arm-kernel, linux-mediatek
Cc: Steven Liu
On Mon, 2024-05-20 at 19:34 +0800, Sky Huang wrote:
> From: "SkyLake.Huang" <skylake.huang@mediatek.com>
>
> Re-organize MTK ethernet phy drivers and integrate common manipulations
> into mtk-phy-lib. Also, add support for build-in 2.5Gphy on MT7988.
>
> v2:
> - Apply correct PATCH tag.
> - Break LED/Token ring/Extend-link-pulse-time features into 3 patches.
> - Fix contents according to v1 comments.
>
> v3:
> - Fix patch 4/5 & 5/5 according to v2 comments.
> - Rebase code and now this patch series can apply to net-next tree.
## Form letter - net-next-closed
The merge window for v6.10 has begun and we have already posted our
pull
request. Therefore net-next is closed for new drivers, features, code
refactoring and optimizations. We are currently accepting bug fixes
only.
Please repost when net-next reopens after May 26th.
RFC patches sent for review only are obviously welcome at any time.
See:
https://www.kernel.org/doc/html/next/process/maintainer-netdev.html#development-cycle
^ permalink raw reply [flat|nested] 19+ messages in thread