Isolate the Basic Mode Control Register (BMCR) operations in the ksz8795
driver by moving the BMCR-related code segments from the ksz8_r_phy()
and ksz8_w_phy() functions to newly created ksz8_r_phy_bmcr() and
ksz8_w_phy_bmcr() functions.
Signed-off-by: Oleksij Rempel <[email protected]>
---
drivers/net/dsa/microchip/ksz8795.c | 351 ++++++++++++++++++----------
1 file changed, 227 insertions(+), 124 deletions(-)
diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c
index 4bf4d67557dc..835157815937 100644
--- a/drivers/net/dsa/microchip/ksz8795.c
+++ b/drivers/net/dsa/microchip/ksz8795.c
@@ -676,9 +676,98 @@ static int ksz8_r_phy_ctrl(struct ksz_device *dev, int port, u16 *val)
return 0;
}
+/**
+ * ksz8_r_phy_bmcr - Translates and reads from the SMI interface to a MIIM PHY
+ * Basic mode control register (Reg. 0).
+ * @dev: The KSZ device instance.
+ * @port: The port number to be read.
+ * @val: The value read from the SMI interface.
+ *
+ * This function reads the SMI interface and translates the hardware register
+ * bit values into their corresponding control settings for a MIIM PHY Basic
+ * mode control register.
+ *
+ * MIIM Bit Mapping Comparison between KSZ8794 and KSZ8873
+ * -------------------------------------------------------------------
+ * MIIM Bit | KSZ8794 Reg/Bit | KSZ8873 Reg/Bit
+ * ----------------------------+-----------------------------+----------------
+ * Bit 15 - Soft Reset | 0xF/4 | Not supported
+ * Bit 14 - Loopback | 0xD/0 (MAC), 0xF/7 (PHY) ~ 0xD/0 (PHY)
+ * Bit 13 - Force 100 | 0xC/6 = 0xC/6
+ * Bit 12 - AN Enable | 0xC/7 (reverse logic) ~ 0xC/7
+ * Bit 11 - Power Down | 0xD/3 = 0xD/3
+ * Bit 10 - PHY Isolate | 0xF/5 | Not supported
+ * Bit 9 - Restart AN | 0xD/5 = 0xD/5
+ * Bit 8 - Force Full-Duplex | 0xC/5 = 0xC/5
+ * Bit 7 - Collision Test/Res. | Not supported | Not supported
+ * Bit 6 - Reserved | Not supported | Not supported
+ * Bit 5 - Hp_mdix | 0x9/7 ~ 0xF/7
+ * Bit 4 - Force MDI | 0xD/1 = 0xD/1
+ * Bit 3 - Disable MDIX | 0xD/2 = 0xD/2
+ * Bit 2 - Disable Far-End F. | ???? | 0xD/4
+ * Bit 1 - Disable Transmit | 0xD/6 = 0xD/6
+ * Bit 0 - Disable LED | 0xD/7 = 0xD/7
+ * -------------------------------------------------------------------
+ *
+ * Return: 0 on success, error code on failure.
+ */
+static int ksz8_r_phy_bmcr(struct ksz_device *dev, int port, u16 *val)
+{
+ const u16 *regs = dev->info->regs;
+ u8 restart, speed, ctrl;
+ int ret;
+
+ *val = 0;
+
+ ret = ksz_pread8(dev, port, regs[P_NEG_RESTART_CTRL], &restart);
+ if (ret)
+ return ret;
+
+ ret = ksz_pread8(dev, port, regs[P_SPEED_STATUS], &speed);
+ if (ret)
+ return ret;
+
+ ret = ksz_pread8(dev, port, regs[P_FORCE_CTRL], &ctrl);
+ if (ret)
+ return ret;
+
+ if (restart & PORT_PHY_LOOPBACK)
+ *val |= BMCR_LOOPBACK;
+
+ if (ctrl & PORT_FORCE_100_MBIT)
+ *val |= BMCR_SPEED100;
+
+ if (ksz_is_ksz88x3(dev)) {
+ if ((ctrl & PORT_AUTO_NEG_ENABLE))
+ *val |= BMCR_ANENABLE;
+ } else {
+ if (!(ctrl & PORT_AUTO_NEG_DISABLE))
+ *val |= BMCR_ANENABLE;
+ }
+
+ if (restart & PORT_POWER_DOWN)
+ *val |= BMCR_PDOWN;
+ if (restart & PORT_AUTO_NEG_RESTART)
+ *val |= BMCR_ANRESTART;
+ if (ctrl & PORT_FORCE_FULL_DUPLEX)
+ *val |= BMCR_FULLDPLX;
+ if (speed & PORT_HP_MDIX)
+ *val |= KSZ886X_BMCR_HP_MDIX;
+ if (restart & PORT_FORCE_MDIX)
+ *val |= KSZ886X_BMCR_FORCE_MDI;
+ if (restart & PORT_AUTO_MDIX_DISABLE)
+ *val |= KSZ886X_BMCR_DISABLE_AUTO_MDIX;
+ if (restart & PORT_TX_DISABLE)
+ *val |= KSZ886X_BMCR_DISABLE_TRANSMIT;
+ if (restart & PORT_LED_OFF)
+ *val |= KSZ886X_BMCR_DISABLE_LED;
+
+ return 0;
+}
+
int ksz8_r_phy(struct ksz_device *dev, u16 phy, u16 reg, u16 *val)
{
- u8 restart, speed, ctrl, link;
+ u8 ctrl, link;
int processed = true;
const u16 *regs;
u8 val1, val2;
@@ -690,45 +779,9 @@ int ksz8_r_phy(struct ksz_device *dev, u16 phy, u16 reg, u16 *val)
switch (reg) {
case MII_BMCR:
- ret = ksz_pread8(dev, p, regs[P_NEG_RESTART_CTRL], &restart);
- if (ret)
- return ret;
-
- ret = ksz_pread8(dev, p, regs[P_SPEED_STATUS], &speed);
- if (ret)
- return ret;
-
- ret = ksz_pread8(dev, p, regs[P_FORCE_CTRL], &ctrl);
+ ret = ksz8_r_phy_bmcr(dev, p, &data);
if (ret)
return ret;
-
- if (restart & PORT_PHY_LOOPBACK)
- data |= BMCR_LOOPBACK;
- if (ctrl & PORT_FORCE_100_MBIT)
- data |= BMCR_SPEED100;
- if (ksz_is_ksz88x3(dev)) {
- if ((ctrl & PORT_AUTO_NEG_ENABLE))
- data |= BMCR_ANENABLE;
- } else {
- if (!(ctrl & PORT_AUTO_NEG_DISABLE))
- data |= BMCR_ANENABLE;
- }
- if (restart & PORT_POWER_DOWN)
- data |= BMCR_PDOWN;
- if (restart & PORT_AUTO_NEG_RESTART)
- data |= BMCR_ANRESTART;
- if (ctrl & PORT_FORCE_FULL_DUPLEX)
- data |= BMCR_FULLDPLX;
- if (speed & PORT_HP_MDIX)
- data |= KSZ886X_BMCR_HP_MDIX;
- if (restart & PORT_FORCE_MDIX)
- data |= KSZ886X_BMCR_FORCE_MDI;
- if (restart & PORT_AUTO_MDIX_DISABLE)
- data |= KSZ886X_BMCR_DISABLE_AUTO_MDIX;
- if (restart & PORT_TX_DISABLE)
- data |= KSZ886X_BMCR_DISABLE_TRANSMIT;
- if (restart & PORT_LED_OFF)
- data |= KSZ886X_BMCR_DISABLE_LED;
break;
case MII_BMSR:
ret = ksz_pread8(dev, p, regs[P_LINK_STATUS], &link);
@@ -860,113 +913,163 @@ static int ksz8_w_phy_ctrl(struct ksz_device *dev, int port, u16 val)
return ret;
}
-int ksz8_w_phy(struct ksz_device *dev, u16 phy, u16 reg, u16 val)
+/**
+ * ksz8_w_phy_bmcr - Translates and writes to the SMI interface from a MIIM PHY
+ * Basic mode control register (Reg. 0).
+ * @dev: The KSZ device instance.
+ * @port: The port number to be configured.
+ * @val: The register value to be written.
+ *
+ * This function translates control settings from a MIIM PHY Basic mode control
+ * register into their corresponding hardware register bit values for the SMI
+ * interface.
+ *
+ * MIIM Bit Mapping Comparison between KSZ8794 and KSZ8873
+ * -------------------------------------------------------------------
+ * MIIM Bit | KSZ8794 Reg/Bit | KSZ8873 Reg/Bit
+ * ----------------------------+-----------------------------+----------------
+ * Bit 15 - Soft Reset | 0xF/4 | Not supported
+ * Bit 14 - Loopback | 0xD/0 (MAC), 0xF/7 (PHY) ~ 0xD/0 (PHY)
+ * Bit 13 - Force 100 | 0xC/6 = 0xC/6
+ * Bit 12 - AN Enable | 0xC/7 (reverse logic) ~ 0xC/7
+ * Bit 11 - Power Down | 0xD/3 = 0xD/3
+ * Bit 10 - PHY Isolate | 0xF/5 | Not supported
+ * Bit 9 - Restart AN | 0xD/5 = 0xD/5
+ * Bit 8 - Force Full-Duplex | 0xC/5 = 0xC/5
+ * Bit 7 - Collision Test/Res. | Not supported | Not supported
+ * Bit 6 - Reserved | Not supported | Not supported
+ * Bit 5 - Hp_mdix | 0x9/7 ~ 0xF/7
+ * Bit 4 - Force MDI | 0xD/1 = 0xD/1
+ * Bit 3 - Disable MDIX | 0xD/2 = 0xD/2
+ * Bit 2 - Disable Far-End F. | ???? | 0xD/4
+ * Bit 1 - Disable Transmit | 0xD/6 = 0xD/6
+ * Bit 0 - Disable LED | 0xD/7 = 0xD/7
+ * -------------------------------------------------------------------
+ *
+ * Return: 0 on success, error code on failure.
+ */
+static int ksz8_w_phy_bmcr(struct ksz_device *dev, int port, u16 val)
{
- u8 restart, speed, ctrl, data;
- const u16 *regs;
- u8 p = phy;
+ const u16 *regs = dev->info->regs;
+ u8 restart, ctrl, speed, data;
int ret;
- regs = dev->info->regs;
+ /* Do not support PHY reset function. */
+ if (val & BMCR_RESET)
+ return 0;
- switch (reg) {
- case MII_BMCR:
+ ret = ksz_pread8(dev, port, regs[P_SPEED_STATUS], &speed);
+ if (ret)
+ return ret;
- /* Do not support PHY reset function. */
- if (val & BMCR_RESET)
- break;
- ret = ksz_pread8(dev, p, regs[P_SPEED_STATUS], &speed);
+ data = speed;
+ if (val & KSZ886X_BMCR_HP_MDIX)
+ data |= PORT_HP_MDIX;
+ else
+ data &= ~PORT_HP_MDIX;
+
+ if (data != speed) {
+ ret = ksz_pwrite8(dev, port, regs[P_SPEED_STATUS], data);
if (ret)
return ret;
+ }
+
+ ret = ksz_pread8(dev, port, regs[P_FORCE_CTRL], &ctrl);
+ if (ret)
+ return ret;
- data = speed;
- if (val & KSZ886X_BMCR_HP_MDIX)
- data |= PORT_HP_MDIX;
+ data = ctrl;
+ if (ksz_is_ksz88x3(dev)) {
+ if ((val & BMCR_ANENABLE))
+ data |= PORT_AUTO_NEG_ENABLE;
+ else
+ data &= ~PORT_AUTO_NEG_ENABLE;
+ } else {
+ if (!(val & BMCR_ANENABLE))
+ data |= PORT_AUTO_NEG_DISABLE;
else
- data &= ~PORT_HP_MDIX;
+ data &= ~PORT_AUTO_NEG_DISABLE;
- if (data != speed) {
- ret = ksz_pwrite8(dev, p, regs[P_SPEED_STATUS], data);
- if (ret)
- return ret;
- }
+ /* Fiber port does not support auto-negotiation. */
+ if (dev->ports[port].fiber)
+ data |= PORT_AUTO_NEG_DISABLE;
+ }
- ret = ksz_pread8(dev, p, regs[P_FORCE_CTRL], &ctrl);
+ if (val & BMCR_SPEED100)
+ data |= PORT_FORCE_100_MBIT;
+ else
+ data &= ~PORT_FORCE_100_MBIT;
+ if (val & BMCR_FULLDPLX)
+ data |= PORT_FORCE_FULL_DUPLEX;
+ else
+ data &= ~PORT_FORCE_FULL_DUPLEX;
+
+ if (data != ctrl) {
+ ret = ksz_pwrite8(dev, port, regs[P_FORCE_CTRL], data);
if (ret)
return ret;
+ }
- data = ctrl;
- if (ksz_is_ksz88x3(dev)) {
- if ((val & BMCR_ANENABLE))
- data |= PORT_AUTO_NEG_ENABLE;
- else
- data &= ~PORT_AUTO_NEG_ENABLE;
- } else {
- if (!(val & BMCR_ANENABLE))
- data |= PORT_AUTO_NEG_DISABLE;
- else
- data &= ~PORT_AUTO_NEG_DISABLE;
-
- /* Fiber port does not support auto-negotiation. */
- if (dev->ports[p].fiber)
- data |= PORT_AUTO_NEG_DISABLE;
- }
+ ret = ksz_pread8(dev, port, regs[P_NEG_RESTART_CTRL], &restart);
+ if (ret)
+ return ret;
- if (val & BMCR_SPEED100)
- data |= PORT_FORCE_100_MBIT;
- else
- data &= ~PORT_FORCE_100_MBIT;
- if (val & BMCR_FULLDPLX)
- data |= PORT_FORCE_FULL_DUPLEX;
- else
- data &= ~PORT_FORCE_FULL_DUPLEX;
+ data = restart;
+ if (val & KSZ886X_BMCR_DISABLE_LED)
+ data |= PORT_LED_OFF;
+ else
+ data &= ~PORT_LED_OFF;
+ if (val & KSZ886X_BMCR_DISABLE_TRANSMIT)
+ data |= PORT_TX_DISABLE;
+ else
+ data &= ~PORT_TX_DISABLE;
+ if (val & BMCR_ANRESTART)
+ data |= PORT_AUTO_NEG_RESTART;
+ else
+ data &= ~(PORT_AUTO_NEG_RESTART);
+ if (val & BMCR_PDOWN)
+ data |= PORT_POWER_DOWN;
+ else
+ data &= ~PORT_POWER_DOWN;
+ if (val & KSZ886X_BMCR_DISABLE_AUTO_MDIX)
+ data |= PORT_AUTO_MDIX_DISABLE;
+ else
+ data &= ~PORT_AUTO_MDIX_DISABLE;
+ if (val & KSZ886X_BMCR_FORCE_MDI)
+ data |= PORT_FORCE_MDIX;
+ else
+ data &= ~PORT_FORCE_MDIX;
+ if (val & BMCR_LOOPBACK)
+ data |= PORT_PHY_LOOPBACK;
+ else
+ data &= ~PORT_PHY_LOOPBACK;
- if (data != ctrl) {
- ret = ksz_pwrite8(dev, p, regs[P_FORCE_CTRL], data);
- if (ret)
- return ret;
- }
+ if (data != restart) {
+ ret = ksz_pwrite8(dev, port, regs[P_NEG_RESTART_CTRL],
+ data);
+ if (ret)
+ return ret;
+ }
- ret = ksz_pread8(dev, p, regs[P_NEG_RESTART_CTRL], &restart);
+ return 0;
+}
+
+int ksz8_w_phy(struct ksz_device *dev, u16 phy, u16 reg, u16 val)
+{
+ u8 ctrl, data;
+ const u16 *regs;
+ u8 p = phy;
+ int ret;
+
+ regs = dev->info->regs;
+
+ switch (reg) {
+ case MII_BMCR:
+ ret = ksz8_w_phy_bmcr(dev, p, val);
if (ret)
return ret;
- data = restart;
- if (val & KSZ886X_BMCR_DISABLE_LED)
- data |= PORT_LED_OFF;
- else
- data &= ~PORT_LED_OFF;
- if (val & KSZ886X_BMCR_DISABLE_TRANSMIT)
- data |= PORT_TX_DISABLE;
- else
- data &= ~PORT_TX_DISABLE;
- if (val & BMCR_ANRESTART)
- data |= PORT_AUTO_NEG_RESTART;
- else
- data &= ~(PORT_AUTO_NEG_RESTART);
- if (val & BMCR_PDOWN)
- data |= PORT_POWER_DOWN;
- else
- data &= ~PORT_POWER_DOWN;
- if (val & KSZ886X_BMCR_DISABLE_AUTO_MDIX)
- data |= PORT_AUTO_MDIX_DISABLE;
- else
- data &= ~PORT_AUTO_MDIX_DISABLE;
- if (val & KSZ886X_BMCR_FORCE_MDI)
- data |= PORT_FORCE_MDIX;
- else
- data &= ~PORT_FORCE_MDIX;
- if (val & BMCR_LOOPBACK)
- data |= PORT_PHY_LOOPBACK;
- else
- data &= ~PORT_PHY_LOOPBACK;
- if (data != restart) {
- ret = ksz_pwrite8(dev, p, regs[P_NEG_RESTART_CTRL],
- data);
- if (ret)
- return ret;
- }
break;
case MII_ADVERTISE:
ret = ksz_pread8(dev, p, regs[P_LOCAL_CTRL], &ctrl);
--
2.39.2
Correct the PHY loopback bit handling in the ksz8_w_phy_bmcr and
ksz8_r_phy_bmcr functions for KSZ8794 and KSZ8873 variants in the ksz8795
driver. Previously, the code erroneously used Bit 7 of port register 0xD
for both chip variants, which is actually for LED configuration. This
update ensures the correct registers and bits are used for the PHY
loopback feature:
- For KSZ8794: Use 0xF / Bit 7.
- For KSZ8873: Use 0xD / Bit 0.
Signed-off-by: Oleksij Rempel <[email protected]>
---
drivers/net/dsa/microchip/ksz8795.c | 39 +++++++++++++++++++------
drivers/net/dsa/microchip/ksz8795_reg.h | 1 +
2 files changed, 31 insertions(+), 9 deletions(-)
diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c
index 4c1e21fd87da..5b223d472548 100644
--- a/drivers/net/dsa/microchip/ksz8795.c
+++ b/drivers/net/dsa/microchip/ksz8795.c
@@ -731,16 +731,25 @@ static int ksz8_r_phy_bmcr(struct ksz_device *dev, int port, u16 *val)
if (ret)
return ret;
- if (restart & PORT_PHY_LOOPBACK)
- *val |= BMCR_LOOPBACK;
-
if (ctrl & PORT_FORCE_100_MBIT)
*val |= BMCR_SPEED100;
if (ksz_is_ksz88x3(dev)) {
+ if (restart & KSZ8873_PORT_PHY_LOOPBACK)
+ *val |= BMCR_LOOPBACK;
+
if ((ctrl & PORT_AUTO_NEG_ENABLE))
*val |= BMCR_ANENABLE;
} else {
+ u8 stat3;
+
+ ret = ksz_pread8(dev, port, REG_PORT_STATUS_3, &stat3);
+ if (ret)
+ return ret;
+
+ if (stat3 & PORT_PHY_LOOPBACK)
+ *val |= BMCR_LOOPBACK;
+
if (!(ctrl & PORT_AUTO_NEG_DISABLE))
*val |= BMCR_ANENABLE;
}
@@ -994,8 +1003,7 @@ static int ksz8_w_phy_bmcr(struct ksz_device *dev, int port, u16 val)
restart = 0;
restart_mask = PORT_LED_OFF | PORT_TX_DISABLE | PORT_AUTO_NEG_RESTART |
- PORT_POWER_DOWN | PORT_AUTO_MDIX_DISABLE | PORT_FORCE_MDIX |
- PORT_PHY_LOOPBACK;
+ PORT_POWER_DOWN | PORT_AUTO_MDIX_DISABLE | PORT_FORCE_MDIX;
if (val & KSZ886X_BMCR_DISABLE_LED)
restart |= PORT_LED_OFF;
if (val & KSZ886X_BMCR_DISABLE_TRANSMIT)
@@ -1008,8 +1016,23 @@ static int ksz8_w_phy_bmcr(struct ksz_device *dev, int port, u16 val)
restart |= PORT_AUTO_MDIX_DISABLE;
if (val & KSZ886X_BMCR_FORCE_MDI)
restart |= PORT_FORCE_MDIX;
- if (val & BMCR_LOOPBACK)
- restart |= PORT_PHY_LOOPBACK;
+
+ if (ksz_is_ksz88x3(dev)) {
+ restart_mask |= KSZ8873_PORT_PHY_LOOPBACK;
+
+ if (val & BMCR_LOOPBACK)
+ restart |= KSZ8873_PORT_PHY_LOOPBACK;
+ } else {
+ u8 stat3 = 0;
+
+ if (val & BMCR_LOOPBACK)
+ stat3 |= PORT_PHY_LOOPBACK;
+
+ ret = ksz_prmw8(dev, port, REG_PORT_STATUS_3, PORT_PHY_LOOPBACK,
+ stat3);
+ if (ret)
+ return ret;
+ }
return ksz_prmw8(dev, port, regs[P_NEG_RESTART_CTRL], restart_mask,
restart);
@@ -1029,8 +1052,6 @@ int ksz8_w_phy(struct ksz_device *dev, u16 phy, u16 reg, u16 val)
ret = ksz8_w_phy_bmcr(dev, p, val);
if (ret)
return ret;
-
-
break;
case MII_ADVERTISE:
ret = ksz_pread8(dev, p, regs[P_LOCAL_CTRL], &ctrl);
diff --git a/drivers/net/dsa/microchip/ksz8795_reg.h b/drivers/net/dsa/microchip/ksz8795_reg.h
index 3c9dae53e4d8..4c07e0e1fcca 100644
--- a/drivers/net/dsa/microchip/ksz8795_reg.h
+++ b/drivers/net/dsa/microchip/ksz8795_reg.h
@@ -262,6 +262,7 @@
#define PORT_AUTO_MDIX_DISABLE BIT(2)
#define PORT_FORCE_MDIX BIT(1)
#define PORT_MAC_LOOPBACK BIT(0)
+#define KSZ8873_PORT_PHY_LOOPBACK BIT(0)
#define REG_PORT_1_STATUS_2 0x1E
#define REG_PORT_2_STATUS_2 0x2E
--
2.39.2
Remove the manual checks for register value changes in the
ksz8_w_phy_bmcr function. Instead, rely on regmap_update_bits() for
optimizing register updates.
Signed-off-by: Oleksij Rempel <[email protected]>
---
drivers/net/dsa/microchip/ksz8795.c | 95 +++++++++--------------------
1 file changed, 28 insertions(+), 67 deletions(-)
diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c
index 835157815937..4c1e21fd87da 100644
--- a/drivers/net/dsa/microchip/ksz8795.c
+++ b/drivers/net/dsa/microchip/ksz8795.c
@@ -951,107 +951,68 @@ static int ksz8_w_phy_ctrl(struct ksz_device *dev, int port, u16 val)
static int ksz8_w_phy_bmcr(struct ksz_device *dev, int port, u16 val)
{
const u16 *regs = dev->info->regs;
- u8 restart, ctrl, speed, data;
+ u8 restart, speed, ctrl, restart_mask;
int ret;
/* Do not support PHY reset function. */
if (val & BMCR_RESET)
return 0;
- ret = ksz_pread8(dev, port, regs[P_SPEED_STATUS], &speed);
- if (ret)
- return ret;
-
- data = speed;
+ speed = 0;
if (val & KSZ886X_BMCR_HP_MDIX)
- data |= PORT_HP_MDIX;
- else
- data &= ~PORT_HP_MDIX;
-
- if (data != speed) {
- ret = ksz_pwrite8(dev, port, regs[P_SPEED_STATUS], data);
- if (ret)
- return ret;
- }
+ speed |= PORT_HP_MDIX;
- ret = ksz_pread8(dev, port, regs[P_FORCE_CTRL], &ctrl);
+ ret = ksz_prmw8(dev, port, regs[P_SPEED_STATUS], PORT_HP_MDIX, speed);
if (ret)
return ret;
- data = ctrl;
+ ctrl = 0;
if (ksz_is_ksz88x3(dev)) {
if ((val & BMCR_ANENABLE))
- data |= PORT_AUTO_NEG_ENABLE;
- else
- data &= ~PORT_AUTO_NEG_ENABLE;
+ ctrl |= PORT_AUTO_NEG_ENABLE;
} else {
if (!(val & BMCR_ANENABLE))
- data |= PORT_AUTO_NEG_DISABLE;
- else
- data &= ~PORT_AUTO_NEG_DISABLE;
+ ctrl |= PORT_AUTO_NEG_DISABLE;
/* Fiber port does not support auto-negotiation. */
if (dev->ports[port].fiber)
- data |= PORT_AUTO_NEG_DISABLE;
+ ctrl |= PORT_AUTO_NEG_DISABLE;
}
if (val & BMCR_SPEED100)
- data |= PORT_FORCE_100_MBIT;
- else
- data &= ~PORT_FORCE_100_MBIT;
+ ctrl |= PORT_FORCE_100_MBIT;
if (val & BMCR_FULLDPLX)
- data |= PORT_FORCE_FULL_DUPLEX;
- else
- data &= ~PORT_FORCE_FULL_DUPLEX;
+ ctrl |= PORT_FORCE_FULL_DUPLEX;
- if (data != ctrl) {
- ret = ksz_pwrite8(dev, port, regs[P_FORCE_CTRL], data);
- if (ret)
- return ret;
- }
-
- ret = ksz_pread8(dev, port, regs[P_NEG_RESTART_CTRL], &restart);
+ ret = ksz_prmw8(dev, port, regs[P_FORCE_CTRL], PORT_FORCE_100_MBIT |
+ /* PORT_AUTO_NEG_ENABLE and PORT_AUTO_NEG_DISABLE are the same
+ * bits
+ */
+ PORT_FORCE_FULL_DUPLEX | PORT_AUTO_NEG_ENABLE, ctrl);
if (ret)
return ret;
- data = restart;
+ restart = 0;
+ restart_mask = PORT_LED_OFF | PORT_TX_DISABLE | PORT_AUTO_NEG_RESTART |
+ PORT_POWER_DOWN | PORT_AUTO_MDIX_DISABLE | PORT_FORCE_MDIX |
+ PORT_PHY_LOOPBACK;
if (val & KSZ886X_BMCR_DISABLE_LED)
- data |= PORT_LED_OFF;
- else
- data &= ~PORT_LED_OFF;
+ restart |= PORT_LED_OFF;
if (val & KSZ886X_BMCR_DISABLE_TRANSMIT)
- data |= PORT_TX_DISABLE;
- else
- data &= ~PORT_TX_DISABLE;
+ restart |= PORT_TX_DISABLE;
if (val & BMCR_ANRESTART)
- data |= PORT_AUTO_NEG_RESTART;
- else
- data &= ~(PORT_AUTO_NEG_RESTART);
+ restart |= PORT_AUTO_NEG_RESTART;
if (val & BMCR_PDOWN)
- data |= PORT_POWER_DOWN;
- else
- data &= ~PORT_POWER_DOWN;
+ restart |= PORT_POWER_DOWN;
if (val & KSZ886X_BMCR_DISABLE_AUTO_MDIX)
- data |= PORT_AUTO_MDIX_DISABLE;
- else
- data &= ~PORT_AUTO_MDIX_DISABLE;
+ restart |= PORT_AUTO_MDIX_DISABLE;
if (val & KSZ886X_BMCR_FORCE_MDI)
- data |= PORT_FORCE_MDIX;
- else
- data &= ~PORT_FORCE_MDIX;
+ restart |= PORT_FORCE_MDIX;
if (val & BMCR_LOOPBACK)
- data |= PORT_PHY_LOOPBACK;
- else
- data &= ~PORT_PHY_LOOPBACK;
+ restart |= PORT_PHY_LOOPBACK;
- if (data != restart) {
- ret = ksz_pwrite8(dev, port, regs[P_NEG_RESTART_CTRL],
- data);
- if (ret)
- return ret;
- }
-
- return 0;
+ return ksz_prmw8(dev, port, regs[P_NEG_RESTART_CTRL], restart_mask,
+ restart);
}
int ksz8_w_phy(struct ksz_device *dev, u16 phy, u16 reg, u16 val)
--
2.39.2
Hi,
On Tue, 2023-11-21 at 16:24 +0100, Oleksij Rempel wrote:
> Correct the PHY loopback bit handling in the ksz8_w_phy_bmcr and
> ksz8_r_phy_bmcr functions for KSZ8794 and KSZ8873 variants in the ksz8795
> driver. Previously, the code erroneously used Bit 7 of port register 0xD
> for both chip variants, which is actually for LED configuration. This
> update ensures the correct registers and bits are used for the PHY
> loopback feature:
>
> - For KSZ8794: Use 0xF / Bit 7.
> - For KSZ8873: Use 0xD / Bit 0.
>
> Signed-off-by: Oleksij Rempel <[email protected]>
This looks like a bugfix, so possibly worth a Fixes tag? Given the
dependency on the previous refactor, I think we can take it via net-
next.
@Andrew, Florian, Vladimir: do you have any specific preference here?
Thanks!
Paolo
On Tue, 2023-11-21 at 16:24 +0100, Oleksij Rempel wrote:
> Remove the manual checks for register value changes in the
> ksz8_w_phy_bmcr function. Instead, rely on regmap_update_bits() for
> optimizing register updates.
>
> Signed-off-by: Oleksij Rempel <[email protected]>
> ---
> drivers/net/dsa/microchip/ksz8795.c | 95 +++++++++--------------------
> 1 file changed, 28 insertions(+), 67 deletions(-)
>
> diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c
> index 835157815937..4c1e21fd87da 100644
> --- a/drivers/net/dsa/microchip/ksz8795.c
> +++ b/drivers/net/dsa/microchip/ksz8795.c
> @@ -951,107 +951,68 @@ static int ksz8_w_phy_ctrl(struct ksz_device *dev, int port, u16 val)
> static int ksz8_w_phy_bmcr(struct ksz_device *dev, int port, u16 val)
> {
> const u16 *regs = dev->info->regs;
> - u8 restart, ctrl, speed, data;
> + u8 restart, speed, ctrl, restart_mask;
Very minor nit and only if you have to repost for some other reason,
please respect the reverse x-mas tree above.
Thanks,
Paolo
On Thu, Nov 23, 2023 at 11:52:57AM +0100, Paolo Abeni wrote:
> Hi,
>
> On Tue, 2023-11-21 at 16:24 +0100, Oleksij Rempel wrote:
> > Correct the PHY loopback bit handling in the ksz8_w_phy_bmcr and
> > ksz8_r_phy_bmcr functions for KSZ8794 and KSZ8873 variants in the ksz8795
> > driver. Previously, the code erroneously used Bit 7 of port register 0xD
> > for both chip variants, which is actually for LED configuration. This
> > update ensures the correct registers and bits are used for the PHY
> > loopback feature:
> >
> > - For KSZ8794: Use 0xF / Bit 7.
> > - For KSZ8873: Use 0xD / Bit 0.
> >
> > Signed-off-by: Oleksij Rempel <[email protected]>
>
> This looks like a bugfix, so possibly worth a Fixes tag? Given the
> dependency on the previous refactor, I think we can take it via net-
> next.
>
> @Andrew, Florian, Vladimir: do you have any specific preference here?
I do not think any one cares about supporting this switch variant in
stable :)
Regards,
Oleksij
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
On Wed, Dec 06, 2023 at 09:55:20AM +0100, Oleksij Rempel wrote:
> On Thu, Nov 23, 2023 at 11:52:57AM +0100, Paolo Abeni wrote:
> > Hi,
> >
> > On Tue, 2023-11-21 at 16:24 +0100, Oleksij Rempel wrote:
> > > Correct the PHY loopback bit handling in the ksz8_w_phy_bmcr and
> > > ksz8_r_phy_bmcr functions for KSZ8794 and KSZ8873 variants in the ksz8795
> > > driver. Previously, the code erroneously used Bit 7 of port register 0xD
> > > for both chip variants, which is actually for LED configuration. This
> > > update ensures the correct registers and bits are used for the PHY
> > > loopback feature:
> > >
> > > - For KSZ8794: Use 0xF / Bit 7.
> > > - For KSZ8873: Use 0xD / Bit 0.
> > >
> > > Signed-off-by: Oleksij Rempel <[email protected]>
> >
> > This looks like a bugfix, so possibly worth a Fixes tag? Given the
> > dependency on the previous refactor, I think we can take it via net-
> > next.
> >
> > @Andrew, Florian, Vladimir: do you have any specific preference here?
>
> I do not think any one cares about supporting this switch variant in
> stable :)
>
> Regards,
> Oleksij
Sorry, this simply fell through the cracks.
How is PHY loopback even supposed to be triggered? User space flips
NETIF_F_LOOPBACK on the netdev, driver ndo_set_features() catches it and
calls phy_loopback() and this calls into phylib's phydev->drv->set_loopback()
or the generic genphy_loopback()?
I don't see DSA implementing ndo_set_features(), nor offering NETIF_F_LOOPBACK.
The PHY is integrated, so DSA is the only relevant netdev driver. Is
there any other way to test this functionality?
If not, I think it's a case of "tree falling in the woods and nobody
hearing it". Not "stable" material. But it definitely has nothing to do
with not caring about the switch variant.
If my analysis is correct, then I actually have a suggestion for you,
Oleksij. Using the F word ("fix") can work against you, if you don't
have enough proof that you're really fixing something which has a user
visible impact. So either do a thorough analysis of the impact in the
commit message, or don't use the F word.
On Wed, Dec 06, 2023 at 05:14:06PM +0200, Vladimir Oltean wrote:
> On Wed, Dec 06, 2023 at 09:55:20AM +0100, Oleksij Rempel wrote:
> > On Thu, Nov 23, 2023 at 11:52:57AM +0100, Paolo Abeni wrote:
> > > Hi,
> > >
> > > On Tue, 2023-11-21 at 16:24 +0100, Oleksij Rempel wrote:
> > > > Correct the PHY loopback bit handling in the ksz8_w_phy_bmcr and
> > > > ksz8_r_phy_bmcr functions for KSZ8794 and KSZ8873 variants in the ksz8795
> > > > driver. Previously, the code erroneously used Bit 7 of port register 0xD
> > > > for both chip variants, which is actually for LED configuration. This
> > > > update ensures the correct registers and bits are used for the PHY
> > > > loopback feature:
> > > >
> > > > - For KSZ8794: Use 0xF / Bit 7.
> > > > - For KSZ8873: Use 0xD / Bit 0.
> > > >
> > > > Signed-off-by: Oleksij Rempel <[email protected]>
> > >
> > > This looks like a bugfix, so possibly worth a Fixes tag? Given the
> > > dependency on the previous refactor, I think we can take it via net-
> > > next.
> > >
> > > @Andrew, Florian, Vladimir: do you have any specific preference here?
> >
> > I do not think any one cares about supporting this switch variant in
> > stable :)
> >
> > Regards,
> > Oleksij
>
> Sorry, this simply fell through the cracks.
>
> How is PHY loopback even supposed to be triggered? User space flips
> NETIF_F_LOOPBACK on the netdev, driver ndo_set_features() catches it and
> calls phy_loopback() and this calls into phylib's phydev->drv->set_loopback()
> or the generic genphy_loopback()?
correct.
> I don't see DSA implementing ndo_set_features(), nor offering NETIF_F_LOOPBACK.
> The PHY is integrated, so DSA is the only relevant netdev driver. Is
> there any other way to test this functionality?
yes - net_selftest()
> If not, I think it's a case of "tree falling in the woods and nobody
> hearing it". Not "stable" material. But it definitely has nothing to do
> with not caring about the switch variant.
Sorry, my intention is not to criticize anyone. I am not getting
feedbacks or bug reports for ksz88xx variants, so it seems like not many
people use it in upstream.
When I have time slots to work on this driver, I try to use them to do
fixes and also clean up the code. Since there is some sort of fog of
uncertainty about when I get the next time slot, or even if I get it at
all, I am trying to push both fixes and cleanups together.
But, you are right, it is not a good reason for not caring about stable :)
What is the decision about this patch set?
Regards,
Oleksij
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
On Wed, Dec 06, 2023 at 04:54:40PM +0100, Oleksij Rempel wrote:
> > I don't see DSA implementing ndo_set_features(), nor offering NETIF_F_LOOPBACK.
> > The PHY is integrated, so DSA is the only relevant netdev driver. Is
> > there any other way to test this functionality?
>
> yes - net_selftest()
Ok, I didn't notice net_test_phy_loopback_enable(). So it can be
triggered after all, it seems.
But I mean, if it's exclusively a selftest that fails, and has always
failed since its introduction, I think it can be considered new
development work when it stops failing? I don't believe that the impact
of the bug is relevant for users. It's not a production functionality.
Documentation/process/stable-kernel-rules.rst doesn't specifically say
this, but it does imply that we should triage the "real bugs that bother
people" as much as possible.
> > If not, I think it's a case of "tree falling in the woods and nobody
> > hearing it". Not "stable" material. But it definitely has nothing to do
> > with not caring about the switch variant.
>
> Sorry, my intention is not to criticize anyone. I am not getting
> feedbacks or bug reports for ksz88xx variants, so it seems like not many
> people use it in upstream.
>
> When I have time slots to work on this driver, I try to use them to do
> fixes and also clean up the code. Since there is some sort of fog of
> uncertainty about when I get the next time slot, or even if I get it at
> all, I am trying to push both fixes and cleanups together.
>
> But, you are right, it is not a good reason for not caring about stable :)
>
> What is the decision about this patch set?
I wouldn't bend over backwards for this, and reorder the patches.
I would spend my time doing more meaningful things.
On Tue, Nov 21, 2023 at 04:24:24PM +0100, Oleksij Rempel wrote:
> Isolate the Basic Mode Control Register (BMCR) operations in the ksz8795
> driver by moving the BMCR-related code segments from the ksz8_r_phy()
> and ksz8_w_phy() functions to newly created ksz8_r_phy_bmcr() and
> ksz8_w_phy_bmcr() functions.
>
> Signed-off-by: Oleksij Rempel <[email protected]>
> ---
Reviewed-by: Vladimir Oltean <[email protected]>
Let's see if this does anything:
pw-bot: under-review
On Tue, Nov 21, 2023 at 04:24:25PM +0100, Oleksij Rempel wrote:
> Remove the manual checks for register value changes in the
> ksz8_w_phy_bmcr function. Instead, rely on regmap_update_bits() for
> optimizing register updates.
>
> Signed-off-by: Oleksij Rempel <[email protected]>
> ---
> drivers/net/dsa/microchip/ksz8795.c | 95 +++++++++--------------------
> 1 file changed, 28 insertions(+), 67 deletions(-)
The diffstat is nice. It's good that we can benefit from the "(tmp != orig)"
check from _regmap_update_bits().
Reviewed-by: Vladimir Oltean <[email protected]>
On Tue, Nov 21, 2023 at 04:24:26PM +0100, Oleksij Rempel wrote:
> Correct the PHY loopback bit handling in the ksz8_w_phy_bmcr and
> ksz8_r_phy_bmcr functions for KSZ8794 and KSZ8873 variants in the ksz8795
> driver. Previously, the code erroneously used Bit 7 of port register 0xD
> for both chip variants, which is actually for LED configuration. This
> update ensures the correct registers and bits are used for the PHY
> loopback feature:
>
> - For KSZ8794: Use 0xF / Bit 7.
> - For KSZ8873: Use 0xD / Bit 0.
>
> Signed-off-by: Oleksij Rempel <[email protected]>
> ---
How did you find, and how did you test this, and on which one of the switches?
Opening the KSZ8873 datasheet, I am confused about their description of
the "far-end loopback". They make it sound as if this loops the packets
_received_ from the media side of PHY port A back to the transmit side
of PHY port A. But the route that these packets take is through the MAC
of PHY port A, then the switching fabric, then PHY port B which reflects
them back to PHY port A, where they finally egress.
Actually, they even go as far as saying that if you set the loopback bit
of port 1, the packets that will be looped back will be from port 2's
RXP/RXM to TXP/TXM pins, and viceversa.
If true, I believe this isn't the behavior expected by phy_loopback(),
where the TX signals from the media side of the PHY are looped back into
the RX side.
Hi Oleksij,
On Tue, 2023-11-21 at 16:24 +0100, Oleksij Rempel wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
>
> Isolate the Basic Mode Control Register (BMCR) operations in the
> ksz8795
> driver by moving the BMCR-related code segments from the ksz8_r_phy()
> and ksz8_w_phy() functions to newly created ksz8_r_phy_bmcr() and
> ksz8_w_phy_bmcr() functions.
>
> Signed-off-by: Oleksij Rempel <[email protected]>
> ---
> drivers/net/dsa/microchip/ksz8795.c | 351 ++++++++++++++++++------
> ----
> 1 file changed, 227 insertions(+), 124 deletions(-)
>
> diff --git a/drivers/net/dsa/microchip/ksz8795.c
> b/drivers/net/dsa/microchip/ksz8795.c
> index 4bf4d67557dc..835157815937 100644
> --- a/drivers/net/dsa/microchip/ksz8795.c
> +++ b/drivers/net/dsa/microchip/ksz8795.c
> @@ -676,9 +676,98 @@ static int ksz8_r_phy_ctrl(struct ksz_device
> *dev, int port, u16 *val)
> return 0;
> }
>
>
> int ksz8_r_phy(struct ksz_device *dev, u16 phy, u16 reg, u16 *val)
> {
> - u8 restart, speed, ctrl, link;
> + u8 ctrl, link;
> int processed = true;
Reverse christmas tree
> const u16 *regs;
> u8 val1, val2;
> @@ -690,45 +779,9 @@ int ksz8_r_phy(struct ksz_device *dev, u16 phy,
> u16 reg, u16 *val)
>
> switch (reg) {
> case MII_BMCR:
> - ret = ksz_pread8(dev, p, regs[P_NEG_RESTART_CTRL],
> &restart);
> - if (ret)
> - return ret;
> -
> - ret = ksz_pread8(dev, p, regs[P_SPEED_STATUS],
> &speed);
> - if (ret)
> - return ret;
> -
> - ret = ksz_pread8(dev, p, regs[P_FORCE_CTRL], &ctrl);
> + ret = ksz8_r_phy_bmcr(dev, p, &data);
> if (ret)
> return ret;
>
> +static int ksz8_w_phy_bmcr(struct ksz_device *dev, int port, u16
> val)
> {
> - u8 restart, speed, ctrl, data;
> - const u16 *regs;
> - u8 p = phy;
> + const u16 *regs = dev->info->regs;
> + u8 restart, ctrl, speed, data;
> int ret;
>
> - regs = dev->info->regs;
> + /* Do not support PHY reset function. */
> + if (val & BMCR_RESET)
> + return 0;
>
> - switch (reg) {
> - case MII_BMCR:
> + ret = ksz_pread8(dev, port, regs[P_SPEED_STATUS], &speed);
> + if (ret)
> + return ret;
>
> - /* Do not support PHY reset function. */
> - if (val & BMCR_RESET)
> - break;
> - ret = ksz_pread8(dev, p, regs[P_SPEED_STATUS],
> &speed);
> + data = speed;
> + if (val & KSZ886X_BMCR_HP_MDIX)
> + data |= PORT_HP_MDIX;
> + else
> + data &= ~PORT_HP_MDIX;
> +
> + if (data != speed) {
> + ret = ksz_pwrite8(dev, port, regs[P_SPEED_STATUS],
> data);
> if (ret)
> return ret;
> + }
> +
> + ret = ksz_pread8(dev, port, regs[P_FORCE_CTRL], &ctrl);
> + if (ret)
> + return ret;
>
> - data = speed;
> - if (val & KSZ886X_BMCR_HP_MDIX)
> - data |= PORT_HP_MDIX;
> + data = ctrl;
> + if (ksz_is_ksz88x3(dev)) {
> + if ((val & BMCR_ANENABLE))
> + data |= PORT_AUTO_NEG_ENABLE;
> + else
> + data &= ~PORT_AUTO_NEG_ENABLE;
> + } else {
> + if (!(val & BMCR_ANENABLE))
> + data |= PORT_AUTO_NEG_DISABLE;
> else
> - data &= ~PORT_HP_MDIX;
> + data &= ~PORT_AUTO_NEG_DISABLE;
>
> - if (data != speed) {
> - ret = ksz_pwrite8(dev, p,
> regs[P_SPEED_STATUS], data);
> - if (ret)
> - return ret;
> - }
> + /* Fiber port does not support auto-negotiation. */
> + if (dev->ports[port].fiber)
> + data |= PORT_AUTO_NEG_DISABLE;
> + }
>
> - ret = ksz_pread8(dev, p, regs[P_FORCE_CTRL], &ctrl);
> + if (val & BMCR_SPEED100)
> + data |= PORT_FORCE_100_MBIT;
> + else
> + data &= ~PORT_FORCE_100_MBIT;
> + if (val & BMCR_FULLDPLX)
> + data |= PORT_FORCE_FULL_DUPLEX;
> + else
> + data &= ~PORT_FORCE_FULL_DUPLEX;
> +
> + if (data != ctrl) {
> + ret = ksz_pwrite8(dev, port, regs[P_FORCE_CTRL],
> data);
> if (ret)
> return ret;
> + }
>
> - data = ctrl;
> - if (ksz_is_ksz88x3(dev)) {
> - if ((val & BMCR_ANENABLE))
> - data |= PORT_AUTO_NEG_ENABLE;
> - else
> - data &= ~PORT_AUTO_NEG_ENABLE;
> - } else {
> - if (!(val & BMCR_ANENABLE))
> - data |= PORT_AUTO_NEG_DISABLE;
> - else
> - data &= ~PORT_AUTO_NEG_DISABLE;
> -
> - /* Fiber port does not support auto-
> negotiation. */
> - if (dev->ports[p].fiber)
> - data |= PORT_AUTO_NEG_DISABLE;
> - }
> + ret = ksz_pread8(dev, port, regs[P_NEG_RESTART_CTRL],
> &restart);
> + if (ret)
> + return ret;
>
> - if (val & BMCR_SPEED100)
> - data |= PORT_FORCE_100_MBIT;
> - else
> - data &= ~PORT_FORCE_100_MBIT;
suggestion: blank line between them increases readability.
> - if (val & BMCR_FULLDPLX)
> - data |= PORT_FORCE_FULL_DUPLEX;
> - else
> - data &= ~PORT_FORCE_FULL_DUPLEX;
> + data = restart;
> + if (val & KSZ886X_BMCR_DISABLE_LED)
> + data |= PORT_LED_OFF;
> + else
> + data &= ~PORT_LED_OFF;
> + if (val & KSZ886X_BMCR_DISABLE_TRANSMIT)
> + data |= PORT_TX_DISABLE;
> + else
> + data &= ~PORT_TX_DISABLE;
> + if (val & BMCR_ANRESTART)
> + data |= PORT_AUTO_NEG_RESTART;
> + else
> + data &= ~(PORT_AUTO_NEG_RESTART);
> + if (val & BMCR_PDOWN)
> + data |= PORT_POWER_DOWN;
> + else
> + data &= ~PORT_POWER_DOWN;
> + if (val & KSZ886X_BMCR_DISABLE_AUTO_MDIX)
> + data |= PORT_AUTO_MDIX_DISABLE;
> + else
> + data &= ~PORT_AUTO_MDIX_DISABLE;
> + if (val & KSZ886X_BMCR_FORCE_MDI)
> + data |= PORT_FORCE_MDIX;
> + else
> + data &= ~PORT_FORCE_MDIX;
> + if (val & BMCR_LOOPBACK)
> + data |= PORT_PHY_LOOPBACK;
> + else
> + data &= ~PORT_PHY_LOOPBACK;
>
> - if (data != ctrl) {
> - ret = ksz_pwrite8(dev, p, regs[P_FORCE_CTRL],
> data);
> - if (ret)
> - return ret;
> - }
> + if (data != restart) {
> + ret = ksz_pwrite8(dev, port,
> regs[P_NEG_RESTART_CTRL],
> + data);
> + if (ret)
> + return ret;
> + }
>
> - ret = ksz_pread8(dev, p, regs[P_NEG_RESTART_CTRL],
> &restart);
> + return 0;
> +}
> +
> +int ksz8_w_phy(struct ksz_device *dev, u16 phy, u16 reg, u16 val)
> +{
> + u8 ctrl, data;
> + const u16 *regs;
reverse christmas tree
> + u8 p = phy;
> + int ret;
> +
> + regs = dev->info->regs;
> +
> + switch (reg) {
> + case MII_BMCR:
> + ret = ksz8_w_phy_bmcr(dev, p, val);
In ksz8_w_phy_bmcr, p is of int, here you are passing p which is u8.
also it is assigned using u16. Any reason behind it. if not, have
consistency.
> if (ret)
> return ret;
>
> - data = restart;
> - if (val & KSZ886X_BMCR_DISABLE_LED)
> - data |= PORT_LED_OFF;
> - else
> - data &= ~PORT_LED_OFF;
> - if (val & KSZ886X_BMCR_DISABLE_TRANSMIT)
> - data |= PORT_TX_DISABLE;
> - else
> - data &= ~PORT_TX_DISABLE;
> - if (val & BMCR_ANRESTART)
> - data |= PORT_AUTO_NEG_RESTART;
> - else
> - data &= ~(PORT_AUTO_NEG_RESTART);
> - if (val & BMCR_PDOWN)
> - data |= PORT_POWER_DOWN;
> - else
> - data &= ~PORT_POWER_DOWN;
> - if (val & KSZ886X_BMCR_DISABLE_AUTO_MDIX)
> - data |= PORT_AUTO_MDIX_DISABLE;
> - else
> - data &= ~PORT_AUTO_MDIX_DISABLE;
> - if (val & KSZ886X_BMCR_FORCE_MDI)
> - data |= PORT_FORCE_MDIX;
> - else
> - data &= ~PORT_FORCE_MDIX;
> - if (val & BMCR_LOOPBACK)
> - data |= PORT_PHY_LOOPBACK;
> - else
> - data &= ~PORT_PHY_LOOPBACK;
>
> - if (data != restart) {
> - ret = ksz_pwrite8(dev, p,
> regs[P_NEG_RESTART_CTRL],
> - data);
> - if (ret)
> - return ret;
> - }
> break;
> case MII_ADVERTISE:
> ret = ksz_pread8(dev, p, regs[P_LOCAL_CTRL], &ctrl);
> --
> 2.39.2
>
On Thu, Dec 07, 2023 at 02:28:23AM +0200, Vladimir Oltean wrote:
> On Tue, Nov 21, 2023 at 04:24:26PM +0100, Oleksij Rempel wrote:
> > Correct the PHY loopback bit handling in the ksz8_w_phy_bmcr and
> > ksz8_r_phy_bmcr functions for KSZ8794 and KSZ8873 variants in the ksz8795
> > driver. Previously, the code erroneously used Bit 7 of port register 0xD
> > for both chip variants, which is actually for LED configuration. This
> > update ensures the correct registers and bits are used for the PHY
> > loopback feature:
> >
> > - For KSZ8794: Use 0xF / Bit 7.
> > - For KSZ8873: Use 0xD / Bit 0.
> >
> > Signed-off-by: Oleksij Rempel <[email protected]>
> > ---
>
> How did you find, and how did you test this, and on which one of the switches?
I tested it by using "ethtool -t lanX" command on KSZ8873. Before this
patch the link will stop to work _after_ end of the selftest. The
selftest will fail too.
After this patch, the selftest is passed, except of the TCP test. And
link is working _after_ the selftest,
> Opening the KSZ8873 datasheet, I am confused about their description of
> the "far-end loopback". They make it sound as if this loops the packets
> _received_ from the media side of PHY port A back to the transmit side
> of PHY port A. But the route that these packets take is through the MAC
> of PHY port A, then the switching fabric, then PHY port B which reflects
> them back to PHY port A, where they finally egress.
>
> Actually, they even go as far as saying that if you set the loopback bit
> of port 1, the packets that will be looped back will be from port 2's
> RXP/RXM to TXP/TXM pins, and viceversa.
>
> If true, I believe this isn't the behavior expected by phy_loopback(),
> where the TX signals from the media side of the PHY are looped back into
> the RX side.
>
>
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
On Thu, Dec 07, 2023 at 06:15:02AM +0100, Oleksij Rempel wrote:
> On Thu, Dec 07, 2023 at 02:28:23AM +0200, Vladimir Oltean wrote:
> > On Tue, Nov 21, 2023 at 04:24:26PM +0100, Oleksij Rempel wrote:
> > > Correct the PHY loopback bit handling in the ksz8_w_phy_bmcr and
> > > ksz8_r_phy_bmcr functions for KSZ8794 and KSZ8873 variants in the ksz8795
> > > driver. Previously, the code erroneously used Bit 7 of port register 0xD
> > > for both chip variants, which is actually for LED configuration. This
> > > update ensures the correct registers and bits are used for the PHY
> > > loopback feature:
> > >
> > > - For KSZ8794: Use 0xF / Bit 7.
> > > - For KSZ8873: Use 0xD / Bit 0.
> > >
> > > Signed-off-by: Oleksij Rempel <[email protected]>
> > > ---
> >
> > How did you find, and how did you test this, and on which one of the switches?
>
> I tested it by using "ethtool -t lanX" command on KSZ8873. Before this
> patch the link will stop to work _after_ end of the selftest. The
> selftest will fail too.
>
> After this patch, the selftest is passed, except of the TCP test. And
> link is working _after_ the selftest,
So you are suggesting that this far-end loopback mode does work as
expected by the kernel.
But is that consistent with the description from the datasheet? It speaks
about an "originating PHY port", but maybe this is confusing, because
based on your test, even the CPU port could be originating the traffic
that gets looped back?
I see it says that far-end loopback goes through the switching fabric.
So the packet, on its return path from the loopback port, gets forwarded
by its MAC DA? That can't be, because the MAC DA lookup has already
determined the destination to be the loopback port (and no MAC SA<->DA
swapping should take place). Or it is forced by the switch to return
specifically to the originating port?
With a bridge between the 2 LAN ports, and lan1 put in loopback, what
happens if you send a broadcast packet towards lan1? Will you also see
it on lan2's link partner, or only on the CPU port?
It's not your fault, but this is all a bit confusing, and I'm not quite
able to match up the documentation with your results. I will trust the
experimental results, however.
On Thu, Dec 07, 2023 at 04:00:30PM +0200, Vladimir Oltean wrote:
> On Thu, Dec 07, 2023 at 06:15:02AM +0100, Oleksij Rempel wrote:
> > On Thu, Dec 07, 2023 at 02:28:23AM +0200, Vladimir Oltean wrote:
> > > On Tue, Nov 21, 2023 at 04:24:26PM +0100, Oleksij Rempel wrote:
> > > > Correct the PHY loopback bit handling in the ksz8_w_phy_bmcr and
> > > > ksz8_r_phy_bmcr functions for KSZ8794 and KSZ8873 variants in the ksz8795
> > > > driver. Previously, the code erroneously used Bit 7 of port register 0xD
> > > > for both chip variants, which is actually for LED configuration. This
> > > > update ensures the correct registers and bits are used for the PHY
> > > > loopback feature:
> > > >
> > > > - For KSZ8794: Use 0xF / Bit 7.
> > > > - For KSZ8873: Use 0xD / Bit 0.
> > > >
> > > > Signed-off-by: Oleksij Rempel <[email protected]>
> > > > ---
> > >
> > > How did you find, and how did you test this, and on which one of the switches?
> >
> > I tested it by using "ethtool -t lanX" command on KSZ8873. Before this
> > patch the link will stop to work _after_ end of the selftest. The
> > selftest will fail too.
> >
> > After this patch, the selftest is passed, except of the TCP test. And
> > link is working _after_ the selftest,
>
> So you are suggesting that this far-end loopback mode does work as
> expected by the kernel.
>
> But is that consistent with the description from the datasheet? It speaks
> about an "originating PHY port", but maybe this is confusing, because
> based on your test, even the CPU port could be originating the traffic
> that gets looped back?
>
> I see it says that far-end loopback goes through the switching fabric.
> So the packet, on its return path from the loopback port, gets forwarded
> by its MAC DA? That can't be, because the MAC DA lookup has already
> determined the destination to be the loopback port (and no MAC SA<->DA
> swapping should take place). Or it is forced by the switch to return
> specifically to the originating port?
>
> With a bridge between the 2 LAN ports, and lan1 put in loopback, what
> happens if you send a broadcast packet towards lan1? Will you also see
> it on lan2's link partner, or only on the CPU port?
>
> It's not your fault, but this is all a bit confusing, and I'm not quite
> able to match up the documentation with your results. I will trust the
> experimental results, however.
I did following tests:
ip l a name br0 type bridge
ip l s dev br0 up
ip l s lan1 master br0
ip l s dev lan1 up
ip l s lan2 master br0
ip l s dev lan2 up
# to avoid link drop with the loopback mode
ethtool -s lan1 speed 100 duplex full autoneg off
# currently no tool support loopback configuration, so set it directly
# to the register
mii -i lan1 -p 0 -r 0 -v 6120
################# Test 1 ###########################
# on DUT
ping 192.168.2.200 -I lan1
on eth0/cpu interface:
00:03:20.656445 AF Unknown (4294967295), length 61:
0x0000: ffff 000e cd00 cdbe 0806 0001 0800 0604 ................
0x0010: 0001 000e cd00 cdbe c0a8 010e 0000 0000 ................
0x0020: 0000 c0a8 02c8 0000 0000 0000 0000 0000 ................
0x0030: 0000 0000 0000 0000 01 .........
00:03:20.656548 AF Unknown (4294967295), length 61:
0x0000: ffff 000e cd00 cdbe 0806 0001 0800 0604 ................
0x0010: 0001 000e cd00 cdbe c0a8 010e 0000 0000 ................
0x0020: 0000 c0a8 02c8 0000 0000 0000 0000 0000 ................
0x0030: 0000 0000 0000 0000 00 .........
on lan1 remote system:
100 701.752654927 00:0e:cd:00:cd:be → ff:ff:ff:ff:ff:ff ARP 60 Who has 192.168.2.200? Tell 192.168.1.14
on lan2 remote system:
347 1071.154437549 00:0e:cd:00:cd:be → ff:ff:ff:ff:ff:ff ARP 60 Who has 192.168.2.200? Tell 192.168.1.14
################# Test 2 ###########################
# send ping on on lan1 remote system:
ping 172.17.0.1
on eth0/cpu interface DUT:
-- nothing --
on lan1 remote system:
109 946.617862621 80:61:5f:0c:29:54 → ff:ff:ff:ff:ff:ff ARP 42 Who has 172.17.0.1? Tell 172.17.0.12
on lan2 remote system:
-- nothing --
################# Test 3 ###########################
# send ping on on lan2 remote system:
ping 172.17.0.122
on eth0/cpu interface DUT:
00:12:18.034220 AF Unknown (4294967295), length 61:
0x0000: ffff 8061 5f0c 2955 0806 0001 0800 0604 ...a_.)U........
0x0010: 0001 8061 5f0c 2955 ac11 000d 0000 0000 ...a_.)U........
0x0020: 0000 ac11 007a 0000 0000 0000 0000 0000 .....z..........
0x0030: 0000 0000 0000 0000 01 .........
00:12:18.034228 AF Unknown (4294967295), length 61:
0x0000: ffff 8061 5f0c 2955 0806 0001 0800 0604 ...a_.)U........
0x0010: 0001 8061 5f0c 2955 ac11 000d 0000 0000 ...a_.)U........
0x0020: 0000 ac11 007a 0000 0000 0000 0000 0000 .....z..........
0x0030: 0000 0000 0000 0000 00
on lan1 remote system:
-- nothing --
on lan2 remote system:
476 1608.560311470 80:61:5f:0c:29:55 → ff:ff:ff:ff:ff:ff ARP 42 Who has 172.17.0.122? Tell 172.17.0.13
477 1608.560361808 80:61:5f:0c:29:55 → ff:ff:ff:ff:ff:ff ARP 60 Who has 172.17.0.122? Tell 172.17.0.13
I also retest selftest results and noted that it is not working if
port is part of bridge.
Regards,
Oleksij
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |