[PATCH v3 0/5] Add Microchip KSZ 9897 Switch CPU PHY + Errata

[PATCH v3 0/5] Add Microchip KSZ 9897 Switch CPU PHY + Errata

[Enguerrand de Ribaucourt]

Back in 2022, I had posted a series of patches to support the KSZ9897
switch's CPU PHY ports but some discussions had not been concluded with
Microchip. I've been maintaining the patches since and I'm now
resubmitting them with some improvements to handle new KSZ9897 errata
sheets (also concerning the whole KSZ9477 family).

I'm very much listening for feedback on these patches. Please let me know if you
have any suggestions or concerns. Thank you.

[PATCH v3 1/5] net: phy: micrel: add Microchip KSZ 9897 Switch PHY support

[Enguerrand de Ribaucourt]

There is a DSA driver for microchip,ksz9897 which can be controlled
through SPI or I2C. This patch adds support for it's CPU ports PHYs to
also allow network access to the switch's CPU port.

The CPU ports PHYs of the KSZ9897 are not documented in the datasheet.
They weirdly use the same PHY ID as the KSZ8081, which is a different
PHY and that driver isn't compatible with KSZ9897. Before this patch,
the KSZ8081 driver was used for the CPU ports of the KSZ9897 but the
link would never come up.

A new driver for the KSZ9897 is added, based on the compatible KSZ87XX.
I could not test if Gigabit Ethernet works, but the link comes up and
can successfully allow packets to be sent and received with DSA tags.

To resolve the KSZ8081/KSZ9897 phy_id conflicts, I could not find any
stable register to distinguish them. The crude solution is to check if a
KSZ9897 DSA switch is present in the devicetree. A discussion to find
better alternatives had been opened with the Microchip team, with no
response yet.

See https://lore.kernel.org/all/20220207174532.362781-1-enguerrand.de-ribaucourt@savoirfairelinux.com/

Signed-off-by: Enguerrand de Ribaucourt <enguerrand.de-ribaucourt@savoirfairelinux.com>
---
 drivers/net/phy/micrel.c | 46 +++++++++++++++++++++++++++++++++++++---
 1 file changed, 43 insertions(+), 3 deletions(-)

diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
index 13e30ea7eec5..99322a3c3869 100644
--- a/drivers/net/phy/micrel.c
+++ b/drivers/net/phy/micrel.c
@@ -16,7 +16,7 @@
  *			   ksz8081, ksz8091,
  *			   ksz8061,
  *		Switch : ksz8873, ksz886x
- *			 ksz9477, lan8804
+ *			 ksz9477, ksz9897, lan8804
  */
 
 #include <linux/bitfield.h>
@@ -769,6 +769,38 @@ static int ksz8051_match_phy_device(struct phy_device *phydev)
 	return ksz8051_ksz8795_match_phy_device(phydev, true);
 }
 
+static int ksz8081_ksz9897_match_phy_device(struct phy_device *phydev,
+					    const bool ksz_8081)
+{
+	/* KSZ8081 and KSZ9897 CPU port share the same exact PHY ID. Despite driver
+	 * differences. None of the PHY registers allow to distinguish them
+	 * accurately. The driver initialization order may also start with the PHY
+	 * matching before the DSA switch node is attached to the CPU port. We fall
+	 * back to looking up if any KSZ9897 is present in the devicetree.
+	 */
+	struct device_node *node;
+
+	if (!phy_id_compare(phydev->phy_id, PHY_ID_KSZ8081, MICREL_PHY_ID_MASK))
+		return 0;
+
+	node = of_find_compatible_node(NULL, NULL, "microchip,ksz9897");
+
+	if (ksz_8081)
+		return node == NULL;
+	else
+		return node != NULL;
+}
+
+static int ksz8081_match_phy_device(struct phy_device *phydev)
+{
+	return ksz8081_ksz9897_match_phy_device(phydev, true);
+}
+
+static int ksz9897_match_phy_device(struct phy_device *phydev)
+{
+	return ksz8081_ksz9897_match_phy_device(phydev, false);
+}
+
 static int ksz8081_config_init(struct phy_device *phydev)
 {
 	/* KSZPHY_OMSO_FACTORY_TEST is set at de-assertion of the reset line
@@ -5300,9 +5332,7 @@ static struct phy_driver ksphy_driver[] = {
 	.suspend	= kszphy_suspend,
 	.resume		= kszphy_resume,
 }, {
-	.phy_id		= PHY_ID_KSZ8081,
 	.name		= "Micrel KSZ8081 or KSZ8091",
-	.phy_id_mask	= MICREL_PHY_ID_MASK,
 	.flags		= PHY_POLL_CABLE_TEST,
 	/* PHY_BASIC_FEATURES */
 	.driver_data	= &ksz8081_type,
@@ -5316,6 +5346,7 @@ static struct phy_driver ksphy_driver[] = {
 	.get_sset_count = kszphy_get_sset_count,
 	.get_strings	= kszphy_get_strings,
 	.get_stats	= kszphy_get_stats,
+	.match_phy_device = ksz8081_match_phy_device,
 	.suspend	= kszphy_suspend,
 	.resume		= kszphy_resume,
 	.cable_test_start	= ksz886x_cable_test_start,
@@ -5486,6 +5517,15 @@ static struct phy_driver ksphy_driver[] = {
 	.suspend	= genphy_suspend,
 	.resume		= genphy_resume,
 	.get_features	= ksz9477_get_features,
+}, {
+	.name		= "Microchip KSZ9897 Switch",
+	/* PHY_BASIC_FEATURES */
+	.config_init	= kszphy_config_init,
+	.config_aneg	= ksz8873mll_config_aneg,
+	.read_status	= ksz8873mll_read_status,
+	.match_phy_device = ksz9897_match_phy_device,
+	.suspend	= genphy_suspend,
+	.resume		= genphy_resume,
 } };
 
 module_phy_driver(ksphy_driver);
-- 
2.34.1

Re: [PATCH v3 1/5] net: phy: micrel: add Microchip KSZ 9897 Switch PHY support

[Andrew Lunn]

On Thu, May 30, 2024 at 10:24:32AM +0000, Enguerrand de Ribaucourt wrote:
> There is a DSA driver for microchip,ksz9897 which can be controlled
> through SPI or I2C. This patch adds support for it's CPU ports PHYs to
> also allow network access to the switch's CPU port.
> 
> The CPU ports PHYs of the KSZ9897 are not documented in the datasheet.
> They weirdly use the same PHY ID as the KSZ8081, which is a different
> PHY and that driver isn't compatible with KSZ9897. Before this patch,
> the KSZ8081 driver was used for the CPU ports of the KSZ9897 but the
> link would never come up.
> 
> A new driver for the KSZ9897 is added, based on the compatible KSZ87XX.
> I could not test if Gigabit Ethernet works, but the link comes up and
> can successfully allow packets to be sent and received with DSA tags.
> 
> To resolve the KSZ8081/KSZ9897 phy_id conflicts, I could not find any
> stable register to distinguish them. The crude solution is to check if a
> KSZ9897 DSA switch is present in the devicetree. A discussion to find
> better alternatives had been opened with the Microchip team, with no
> response yet.

A better solution it to use:

      - pattern: "^ethernet-phy-id[a-f0-9]{4}\\.[a-f0-9]{4}$"
        description:
          If the PHY reports an incorrect ID (or none at all) then the
          compatible list may contain an entry with the correct PHY ID
          in the above form.

If there is no official ID for this PHY, you can ask Microchip to
allocate one. Or just pick the highest free ID in Microchips range.

	Andrew

[PATCH v3 2/5] net: phy: micrel: disable suspend/resume callbacks following errata

[Enguerrand de Ribaucourt]

Microchip's erratas state that powering down a PHY may cause errors
on the adjacent PHYs due to the power supply noise. The suggested
workaround is to avoid toggling the powerdown bit dynamically while
traffic may be present.

Signed-off-by: Enguerrand de Ribaucourt <enguerrand.de-ribaucourt@savoirfairelinux.com>
---
 drivers/net/phy/micrel.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
index 99322a3c3869..78f2040c3cd1 100644
--- a/drivers/net/phy/micrel.c
+++ b/drivers/net/phy/micrel.c
@@ -5514,8 +5514,9 @@ static struct phy_driver ksphy_driver[] = {
 	.config_init	= ksz9477_config_init,
 	.config_intr	= kszphy_config_intr,
 	.handle_interrupt = kszphy_handle_interrupt,
-	.suspend	= genphy_suspend,
-	.resume		= genphy_resume,
+	/* No suspend/resume callbacks because of errata DS80000758:
+	 * Toggling PHY Powerdown can cause errors or link failures in adjacent PHYs
+	 */
 	.get_features	= ksz9477_get_features,
 }, {
 	.name		= "Microchip KSZ9897 Switch",
@@ -5524,8 +5525,9 @@ static struct phy_driver ksphy_driver[] = {
 	.config_aneg	= ksz8873mll_config_aneg,
 	.read_status	= ksz8873mll_read_status,
 	.match_phy_device = ksz9897_match_phy_device,
-	.suspend	= genphy_suspend,
-	.resume		= genphy_resume,
+	/* No suspend/resume callbacks because of errata DS00002330D:
+	 * Toggling PHY Powerdown can cause errors or link failures in adjacent PHYs
+	 */
 } };
 
 module_phy_driver(ksphy_driver);
-- 
2.34.1

[PATCH v3 3/5] net: phy: micrel: add Microchip KSZ 9477 to the device table

[Enguerrand de Ribaucourt]

PHY_ID_KSZ9477 was supported but not added to the device table passed to
MODULE_DEVICE_TABLE.

Signed-off-by: Enguerrand de Ribaucourt <enguerrand.de-ribaucourt@savoirfairelinux.com>
---
 drivers/net/phy/micrel.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
index 78f2040c3cd1..5930ac9fa661 100644
--- a/drivers/net/phy/micrel.c
+++ b/drivers/net/phy/micrel.c
@@ -5550,6 +5550,7 @@ static struct mdio_device_id __maybe_unused micrel_tbl[] = {
 	{ PHY_ID_KSZ8081, MICREL_PHY_ID_MASK },
 	{ PHY_ID_KSZ8873MLL, MICREL_PHY_ID_MASK },
 	{ PHY_ID_KSZ886X, MICREL_PHY_ID_MASK },
+	{ PHY_ID_KSZ9477, MICREL_PHY_ID_MASK },
 	{ PHY_ID_LAN8814, MICREL_PHY_ID_MASK },
 	{ PHY_ID_LAN8804, MICREL_PHY_ID_MASK },
 	{ PHY_ID_LAN8841, MICREL_PHY_ID_MASK },
-- 
2.34.1

[PATCH v3 4/5] net: dsa: microchip: use collision based back pressure mode

[Enguerrand de Ribaucourt]

Errata DS80000758 states that carrier sense back pressure mode can cause
link down issues in 100BASE-TX half duplex mode. The datasheet also
recommends to always use the collision based back pressure mode.

Signed-off-by: Enguerrand de Ribaucourt <enguerrand.de-ribaucourt@savoirfairelinux.com>
---
 drivers/net/dsa/microchip/ksz9477.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
index f8ad7833f5d9..343b9d7538e9 100644
--- a/drivers/net/dsa/microchip/ksz9477.c
+++ b/drivers/net/dsa/microchip/ksz9477.c
@@ -1299,6 +1299,9 @@ int ksz9477_setup(struct dsa_switch *ds)
 	/* Enable REG_SW_MTU__2 reg by setting SW_JUMBO_PACKET */
 	ksz_cfg(dev, REG_SW_MAC_CTRL_1, SW_JUMBO_PACKET, true);
 
+	/* Use collision based back pressure mode. */
+	ksz_cfg(dev, REG_SW_MAC_CTRL_1, SW_BACK_PRESSURE, false);
+
 	/* Now we can configure default MTU value */
 	ret = regmap_update_bits(ksz_regmap_16(dev), REG_SW_MTU__2, REG_SW_MTU_MASK,
 				 VLAN_ETH_FRAME_LEN + ETH_FCS_LEN);
-- 
2.34.1

[PATCH v3 5/5] net: dsa: microchip: monitor potential faults in half-duplex mode

[Enguerrand de Ribaucourt]

The errata DS80000754 recommends monitoring potential faults in
half-duplex mode for the KSZ9477 familly.

half-duplex is not very common so I just added a critical message
when the fault conditions are detected. The switch can be expected
to be unable to communicate anymore in these states and a software
reset of the switch would be required which I did not implement.

Signed-off-by: Enguerrand de Ribaucourt <enguerrand.de-ribaucourt@savoirfairelinux.com>
---
 drivers/net/dsa/microchip/ksz9477.c     | 34 +++++++++++++++++++++++++
 drivers/net/dsa/microchip/ksz9477.h     |  2 ++
 drivers/net/dsa/microchip/ksz9477_reg.h |  8 ++++--
 drivers/net/dsa/microchip/ksz_common.c  |  7 +++++
 drivers/net/dsa/microchip/ksz_common.h  |  1 +
 5 files changed, 50 insertions(+), 2 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
index 343b9d7538e9..ea1c12304f7f 100644
--- a/drivers/net/dsa/microchip/ksz9477.c
+++ b/drivers/net/dsa/microchip/ksz9477.c
@@ -429,6 +429,40 @@ void ksz9477_freeze_mib(struct ksz_device *dev, int port, bool freeze)
 	mutex_unlock(&p->mib.cnt_mutex);
 }
 
+void ksz9477_errata_monitor(struct ksz_device *dev, int port,
+			    u64 tx_late_col)
+{
+	u8 status;
+	u16 pqm;
+	u32 pmavbc;
+
+	ksz_pread8(dev, port, REG_PORT_STATUS_0, &status);
+	if (!((status & PORT_INTF_SPEED_MASK) == PORT_INTF_SPEED_MASK) &&
+	    !(status & PORT_INTF_FULL_DUPLEX)) {
+		dev_warn_once(dev->dev,
+			      "Half-duplex detected on port %d, transmission halt may occur\n",
+			      port);
+		/* Errata DS80000754 recommends monitoring potential faults in
+		 * half-duplex mode. The switch might not be able to communicate anymore
+		 * in these states.
+		 */
+		if (tx_late_col != 0) {
+			/* Transmission halt with late collisions */
+			dev_crit_ratelimited(dev->dev,
+					     "TX late collisions detected, transmission may be halted on port %d\n",
+					     port);
+		}
+		ksz_pread16(dev, port, REG_PORT_QM_TX_CNT_0__4, &pqm);
+		ksz_read32(dev, REG_PMAVBC, &pmavbc);
+		if (((pmavbc & PMAVBC_MASK) >> PMAVBC_SHIFT <= 0x580) ||
+		    ((pqm & PORT_QM_TX_CNT_M) >= 0x200)) {
+			/* Transmission halt with Half-Duplex and VLAN */
+			dev_crit_ratelimited(dev->dev,
+					     "resources out of limits, transmission may be halted\n");
+		}
+	}
+}
+
 void ksz9477_port_init_cnt(struct ksz_device *dev, int port)
 {
 	struct ksz_port_mib *mib = &dev->ports[port].mib;
diff --git a/drivers/net/dsa/microchip/ksz9477.h b/drivers/net/dsa/microchip/ksz9477.h
index ce1e656b800b..3312ef28e99c 100644
--- a/drivers/net/dsa/microchip/ksz9477.h
+++ b/drivers/net/dsa/microchip/ksz9477.h
@@ -36,6 +36,8 @@ int ksz9477_port_mirror_add(struct ksz_device *dev, int port,
 			    bool ingress, struct netlink_ext_ack *extack);
 void ksz9477_port_mirror_del(struct ksz_device *dev, int port,
 			     struct dsa_mall_mirror_tc_entry *mirror);
+void ksz9477_errata_monitor(struct ksz_device *dev, int port,
+			    u64 tx_late_col);
 void ksz9477_get_caps(struct ksz_device *dev, int port,
 		      struct phylink_config *config);
 int ksz9477_fdb_dump(struct ksz_device *dev, int port,
diff --git a/drivers/net/dsa/microchip/ksz9477_reg.h b/drivers/net/dsa/microchip/ksz9477_reg.h
index f3a205ee483f..3238b9748d0f 100644
--- a/drivers/net/dsa/microchip/ksz9477_reg.h
+++ b/drivers/net/dsa/microchip/ksz9477_reg.h
@@ -842,8 +842,7 @@
 
 #define REG_PORT_STATUS_0		0x0030
 
-#define PORT_INTF_SPEED_M		0x3
-#define PORT_INTF_SPEED_S		3
+#define PORT_INTF_SPEED_MASK		0x0018
 #define PORT_INTF_FULL_DUPLEX		BIT(2)
 #define PORT_TX_FLOW_CTRL		BIT(1)
 #define PORT_RX_FLOW_CTRL		BIT(0)
@@ -1167,6 +1166,11 @@
 #define PORT_RMII_CLK_SEL		BIT(7)
 #define PORT_MII_SEL_EDGE		BIT(5)
 
+#define REG_PMAVBC				0x03AC
+
+#define PMAVBC_MASK				0x7ff0000
+#define PMAVBC_SHIFT			16
+
 /* 4 - MAC */
 #define REG_PORT_MAC_CTRL_0		0x0400
 
diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index 1e0085cd9a9a..26e2fcd74ba8 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -1382,6 +1382,7 @@ const struct ksz_chip_data ksz_switch_chips[] = {
 		.tc_cbs_supported = true,
 		.ops = &ksz9477_dev_ops,
 		.phylink_mac_ops = &ksz9477_phylink_mac_ops,
+		.phy_errata_9477 = true,
 		.mib_names = ksz9477_mib_names,
 		.mib_cnt = ARRAY_SIZE(ksz9477_mib_names),
 		.reg_mib_cnt = MIB_COUNTER_NUM,
@@ -1416,6 +1417,7 @@ const struct ksz_chip_data ksz_switch_chips[] = {
 		.num_ipms = 8,
 		.ops = &ksz9477_dev_ops,
 		.phylink_mac_ops = &ksz9477_phylink_mac_ops,
+		.phy_errata_9477 = true,
 		.mib_names = ksz9477_mib_names,
 		.mib_cnt = ARRAY_SIZE(ksz9477_mib_names),
 		.reg_mib_cnt = MIB_COUNTER_NUM,
@@ -1450,6 +1452,7 @@ const struct ksz_chip_data ksz_switch_chips[] = {
 		.num_ipms = 8,
 		.ops = &ksz9477_dev_ops,
 		.phylink_mac_ops = &ksz9477_phylink_mac_ops,
+		.phy_errata_9477 = true,
 		.mib_names = ksz9477_mib_names,
 		.mib_cnt = ARRAY_SIZE(ksz9477_mib_names),
 		.reg_mib_cnt = MIB_COUNTER_NUM,
@@ -1540,6 +1543,7 @@ const struct ksz_chip_data ksz_switch_chips[] = {
 		.tc_cbs_supported = true,
 		.ops = &ksz9477_dev_ops,
 		.phylink_mac_ops = &ksz9477_phylink_mac_ops,
+		.phy_errata_9477 = true,
 		.mib_names = ksz9477_mib_names,
 		.mib_cnt = ARRAY_SIZE(ksz9477_mib_names),
 		.reg_mib_cnt = MIB_COUNTER_NUM,
@@ -1861,6 +1865,9 @@ void ksz_r_mib_stats64(struct ksz_device *dev, int port)
 	pstats->rx_pause_frames = raw->rx_pause;
 
 	spin_unlock(&mib->stats64_lock);
+
+	if (dev->info->phy_errata_9477)
+		ksz9477_errata_monitor(dev, port, raw->tx_late_col);
 }
 
 void ksz88xx_r_mib_stats64(struct ksz_device *dev, int port)
diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
index c784fd23a993..ee7db46e469d 100644
--- a/drivers/net/dsa/microchip/ksz_common.h
+++ b/drivers/net/dsa/microchip/ksz_common.h
@@ -66,6 +66,7 @@ struct ksz_chip_data {
 	bool tc_cbs_supported;
 	const struct ksz_dev_ops *ops;
 	const struct phylink_mac_ops *phylink_mac_ops;
+	bool phy_errata_9477;
 	bool ksz87xx_eee_link_erratum;
 	const struct ksz_mib_names *mib_names;
 	int mib_cnt;
-- 
2.34.1

[PATCH v3 5/5] net: dsa: microchip: monitor potential faults in half-duplex mode

[Hariprasad Kelam]

> ----------------------------------------------------------------------
> The errata DS80000754 recommends monitoring potential faults in half-
> duplex mode for the KSZ9477 familly.
> 
> half-duplex is not very common so I just added a critical message when the
> fault conditions are detected. The switch can be expected to be unable to
> communicate anymore in these states and a software reset of the switch
> would be required which I did not implement.
> 
> Signed-off-by: Enguerrand de Ribaucourt <enguerrand.de-
> ribaucourt@savoirfairelinux.com>
> ---
>  drivers/net/dsa/microchip/ksz9477.c     | 34 +++++++++++++++++++++++++
>  drivers/net/dsa/microchip/ksz9477.h     |  2 ++
>  drivers/net/dsa/microchip/ksz9477_reg.h |  8 ++++--
> drivers/net/dsa/microchip/ksz_common.c  |  7 +++++
> drivers/net/dsa/microchip/ksz_common.h  |  1 +
>  5 files changed, 50 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/dsa/microchip/ksz9477.c
> b/drivers/net/dsa/microchip/ksz9477.c
> index 343b9d7538e9..ea1c12304f7f 100644
> --- a/drivers/net/dsa/microchip/ksz9477.c
> +++ b/drivers/net/dsa/microchip/ksz9477.c
> @@ -429,6 +429,40 @@ void ksz9477_freeze_mib(struct ksz_device *dev, int
> port, bool freeze)
>  	mutex_unlock(&p->mib.cnt_mutex);
>  }
> 
> +void ksz9477_errata_monitor(struct ksz_device *dev, int port,
> +			    u64 tx_late_col)
> +{
> +	u8 status;
> +	u16 pqm;
> +	u32 pmavbc;
> +
          Follow reverse x-mas tree notation.

> +	ksz_pread8(dev, port, REG_PORT_STATUS_0, &status);
> +	if (!((status & PORT_INTF_SPEED_MASK) ==
> PORT_INTF_SPEED_MASK) &&
> +	    !(status & PORT_INTF_FULL_DUPLEX)) {
> +		dev_warn_once(dev->dev,
> +			      "Half-duplex detected on port %d, transmission
> halt may occur\n",
> +			      port);
> +		/* Errata DS80000754 recommends monitoring potential
> faults in
> +		 * half-duplex mode. The switch might not be able to
> communicate anymore
> +		 * in these states.
> +		 */
> +		if (tx_late_col != 0) {
> +			/* Transmission halt with late collisions */
> +			dev_crit_ratelimited(dev->dev,
> +					     "TX late collisions detected,
> transmission may be halted on port %d\n",
> +					     port);
> +		}
> +		ksz_pread16(dev, port, REG_PORT_QM_TX_CNT_0__4,
> &pqm);
> +		ksz_read32(dev, REG_PMAVBC, &pmavbc);
> +		if (((pmavbc & PMAVBC_MASK) >> PMAVBC_SHIFT <= 0x580)
> ||
> +		    ((pqm & PORT_QM_TX_CNT_M) >= 0x200)) {
> +			/* Transmission halt with Half-Duplex and VLAN */
> +			dev_crit_ratelimited(dev->dev,
> +					     "resources out of limits,
> transmission may be halted\n");
> +		}
> +	}
> +}
> +
>  void ksz9477_port_init_cnt(struct ksz_device *dev, int port)  {
>  	struct ksz_port_mib *mib = &dev->ports[port].mib; diff --git
> a/drivers/net/dsa/microchip/ksz9477.h
> b/drivers/net/dsa/microchip/ksz9477.h
> index ce1e656b800b..3312ef28e99c 100644
> --- a/drivers/net/dsa/microchip/ksz9477.h
> +++ b/drivers/net/dsa/microchip/ksz9477.h
> @@ -36,6 +36,8 @@ int ksz9477_port_mirror_add(struct ksz_device *dev, int
> port,
>  			    bool ingress, struct netlink_ext_ack *extack);  void
> ksz9477_port_mirror_del(struct ksz_device *dev, int port,
>  			     struct dsa_mall_mirror_tc_entry *mirror);
> +void ksz9477_errata_monitor(struct ksz_device *dev, int port,
> +			    u64 tx_late_col);
>  void ksz9477_get_caps(struct ksz_device *dev, int port,
>  		      struct phylink_config *config);  int
> ksz9477_fdb_dump(struct ksz_device *dev, int port, diff --git
> a/drivers/net/dsa/microchip/ksz9477_reg.h
> b/drivers/net/dsa/microchip/ksz9477_reg.h
> index f3a205ee483f..3238b9748d0f 100644
> --- a/drivers/net/dsa/microchip/ksz9477_reg.h
> +++ b/drivers/net/dsa/microchip/ksz9477_reg.h
> @@ -842,8 +842,7 @@
> 
>  #define REG_PORT_STATUS_0		0x0030
> 
> -#define PORT_INTF_SPEED_M		0x3
> -#define PORT_INTF_SPEED_S		3
> +#define PORT_INTF_SPEED_MASK		0x0018
>  #define PORT_INTF_FULL_DUPLEX		BIT(2)
>  #define PORT_TX_FLOW_CTRL		BIT(1)
>  #define PORT_RX_FLOW_CTRL		BIT(0)
> @@ -1167,6 +1166,11 @@
>  #define PORT_RMII_CLK_SEL		BIT(7)
>  #define PORT_MII_SEL_EDGE		BIT(5)
> 
> +#define REG_PMAVBC				0x03AC
> +
> +#define PMAVBC_MASK				0x7ff0000
> +#define PMAVBC_SHIFT			16
> +
>  /* 4 - MAC */
>  #define REG_PORT_MAC_CTRL_0		0x0400
> 
> diff --git a/drivers/net/dsa/microchip/ksz_common.c
> b/drivers/net/dsa/microchip/ksz_common.c
> index 1e0085cd9a9a..26e2fcd74ba8 100644
> --- a/drivers/net/dsa/microchip/ksz_common.c
> +++ b/drivers/net/dsa/microchip/ksz_common.c
> @@ -1382,6 +1382,7 @@ const struct ksz_chip_data ksz_switch_chips[] = {
>  		.tc_cbs_supported = true,
>  		.ops = &ksz9477_dev_ops,
>  		.phylink_mac_ops = &ksz9477_phylink_mac_ops,
> +		.phy_errata_9477 = true,
>  		.mib_names = ksz9477_mib_names,
>  		.mib_cnt = ARRAY_SIZE(ksz9477_mib_names),
>  		.reg_mib_cnt = MIB_COUNTER_NUM,
> @@ -1416,6 +1417,7 @@ const struct ksz_chip_data ksz_switch_chips[] = {
>  		.num_ipms = 8,
>  		.ops = &ksz9477_dev_ops,
>  		.phylink_mac_ops = &ksz9477_phylink_mac_ops,
> +		.phy_errata_9477 = true,
>  		.mib_names = ksz9477_mib_names,
>  		.mib_cnt = ARRAY_SIZE(ksz9477_mib_names),
>  		.reg_mib_cnt = MIB_COUNTER_NUM,
> @@ -1450,6 +1452,7 @@ const struct ksz_chip_data ksz_switch_chips[] = {
>  		.num_ipms = 8,
>  		.ops = &ksz9477_dev_ops,
>  		.phylink_mac_ops = &ksz9477_phylink_mac_ops,
> +		.phy_errata_9477 = true,
>  		.mib_names = ksz9477_mib_names,
>  		.mib_cnt = ARRAY_SIZE(ksz9477_mib_names),
>  		.reg_mib_cnt = MIB_COUNTER_NUM,
> @@ -1540,6 +1543,7 @@ const struct ksz_chip_data ksz_switch_chips[] = {
>  		.tc_cbs_supported = true,
>  		.ops = &ksz9477_dev_ops,
>  		.phylink_mac_ops = &ksz9477_phylink_mac_ops,
> +		.phy_errata_9477 = true,
>  		.mib_names = ksz9477_mib_names,
>  		.mib_cnt = ARRAY_SIZE(ksz9477_mib_names),
>  		.reg_mib_cnt = MIB_COUNTER_NUM,
> @@ -1861,6 +1865,9 @@ void ksz_r_mib_stats64(struct ksz_device *dev, int
> port)
>  	pstats->rx_pause_frames = raw->rx_pause;
> 
>  	spin_unlock(&mib->stats64_lock);
> +
> +	if (dev->info->phy_errata_9477)
> +		ksz9477_errata_monitor(dev, port, raw->tx_late_col);
>  }
> 
>  void ksz88xx_r_mib_stats64(struct ksz_device *dev, int port) diff --git
> a/drivers/net/dsa/microchip/ksz_common.h
> b/drivers/net/dsa/microchip/ksz_common.h
> index c784fd23a993..ee7db46e469d 100644
> --- a/drivers/net/dsa/microchip/ksz_common.h
> +++ b/drivers/net/dsa/microchip/ksz_common.h
> @@ -66,6 +66,7 @@ struct ksz_chip_data {
>  	bool tc_cbs_supported;
>  	const struct ksz_dev_ops *ops;
>  	const struct phylink_mac_ops *phylink_mac_ops;
> +	bool phy_errata_9477;
>  	bool ksz87xx_eee_link_erratum;
>  	const struct ksz_mib_names *mib_names;
>  	int mib_cnt;
> --
> 2.34.1
> 

Re: [PATCH v3 5/5] net: dsa: microchip: monitor potential faults in half-duplex mode

[Andrew Lunn]

> > +void ksz9477_errata_monitor(struct ksz_device *dev, int port,
> > +			    u64 tx_late_col)
> > +{
> > +	u8 status;
> > +	u16 pqm;
> > +	u32 pmavbc;
> > +
>           Follow reverse x-mas tree notation.


Hi Hariprasad

Please trim messages when replying. Just quote what is needed.

       Andrew

Re: [PATCH v3 0/5] Add Microchip KSZ 9897 Switch CPU PHY + Errata

[Andrew Lunn]

On Thu, May 30, 2024 at 10:24:31AM +0000, Enguerrand de Ribaucourt wrote:
> Back in 2022, I had posted a series of patches to support the KSZ9897
> switch's CPU PHY ports but some discussions had not been concluded with
> Microchip. I've been maintaining the patches since and I'm now
> resubmitting them with some improvements to handle new KSZ9897 errata
> sheets (also concerning the whole KSZ9477 family).
> 
> I'm very much listening for feedback on these patches. Please let me know if you
> have any suggestions or concerns. Thank you.

Since these are Fixes, please could you base these on net. Also
include a Fixes: tag.

https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html#netdev-faq

    Andrew

---
pw-bot: cr

RE: [PATCH v3 0/5] Add Microchip KSZ 9897 Switch CPU PHY + Errata

[Woojung.Huh]

Hi Enguerrand,

Thanks for the patch.
Could you please add UNGLinuxDriver@microchip.com in next revision
to be monitored by proper reviewers.
It is in MAINTAINERS list of Microchip KSZ series.

Best regards,
Woojung

> -----Original Message-----
> From: Enguerrand de Ribaucourt <enguerrand.de-
> ribaucourt@savoirfairelinux.com>
> Sent: Thursday, May 30, 2024 6:25 AM
> To: netdev@vger.kernel.org
> Cc: andrew@lunn.ch; hkallweit1@gmail.com; linux@armlinux.org.uk; Woojung Huh
> - C21699 <Woojung.Huh@microchip.com>; embedded-
> discuss@lists.savoirfairelinux.net
> Subject: [PATCH v3 0/5] Add Microchip KSZ 9897 Switch CPU PHY + Errata
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> Back in 2022, I had posted a series of patches to support the KSZ9897
> switch's CPU PHY ports but some discussions had not been concluded with
> Microchip. I've been maintaining the patches since and I'm now
> resubmitting them with some improvements to handle new KSZ9897 errata
> sheets (also concerning the whole KSZ9477 family).
> 
> I'm very much listening for feedback on these patches. Please let me know if
> you
> have any suggestions or concerns. Thank you.
> 
> 

[PATCH v4 0/5] Add Microchip KSZ 9897 Switch CPU PHY + Errata

[Enguerrand de Ribaucourt]

Back in 2022, I had posted a series of patches to support the KSZ9897
switch's CPU PHY ports but some discussions had not been concluded with
Microchip. I've been maintaining the patches since and I'm now
resubmitting them with some improvements to handle new KSZ9897 errata
sheets (also concerning the whole KSZ9477 family).

I'm very much listening for feedback on these patches. Please let me know if you
have any suggestions or concerns. Thank you.

---
v4:
 - Rebase on net/main
 - Add Fixes: tags to the patches
 - reverse x-mas tree order
 - use pseudo phy_id instead of match_phy_device
v3: https://lore.kernel.org/all/20240530102436.226189-1-enguerrand.de-ribaucourt@savoirfairelinux.com/

[PATCH net v4 1/5] net: phy: micrel: add Microchip KSZ 9897 Switch PHY support

[Enguerrand de Ribaucourt]

There is a DSA driver for microchip,ksz9897 which can be controlled
through SPI or I2C. This patch adds support for it's CPU ports PHYs to
also allow network access to the switch's CPU port.

The CPU ports PHYs of the KSZ9897 are not documented in the datasheet.
They weirdly use the same PHY ID as the KSZ8081, which is a different
PHY and that driver isn't compatible with KSZ9897. Before this patch,
the KSZ8081 driver was used for the CPU ports of the KSZ9897 but the
link would never come up.

A new driver for the KSZ9897 is added, based on the compatible KSZ87XX.
I could not test if Gigabit Ethernet works, but the link comes up and
can successfully allow packets to be sent and received with DSA tags.

To resolve the KSZ8081/KSZ9897 phy_id conflicts, I could not find any
stable register to distinguish them. Instead of a match_phy_device() ,
I've declared a virtual phy_id with the highest value in Microchip's OUI
range.

Example usage in the device tree:
	compatible = "ethernet-phy-id0022.17ff";

A discussion to find better alternatives had been opened with the
Microchip team, with no response yet.

See https://lore.kernel.org/all/20220207174532.362781-1-enguerrand.de-ribaucourt@savoirfairelinux.com/

Fixes: b987e98e50ab ("dsa: add DSA switch driver for Microchip KSZ9477")
Signed-off-by: Enguerrand de Ribaucourt <enguerrand.de-ribaucourt@savoirfairelinux.com>
---
v4:
 - rebase on net/main
 - add Fixes tag
 - use pseudo phy_id instead of of_tree search
v3: https://lore.kernel.org/all/20240530102436.226189-2-enguerrand.de-ribaucourt@savoirfairelinux.com/
---
 drivers/net/phy/micrel.c   | 14 +++++++++++++-
 include/linux/micrel_phy.h |  4 ++++
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
index 2b8f8b7f1517..8a6dfaceeab3 100644
--- a/drivers/net/phy/micrel.c
+++ b/drivers/net/phy/micrel.c
@@ -16,7 +16,7 @@
  *			   ksz8081, ksz8091,
  *			   ksz8061,
  *		Switch : ksz8873, ksz886x
- *			 ksz9477, lan8804
+ *			 ksz9477, ksz9897, lan8804
  */
 
 #include <linux/bitfield.h>
@@ -5495,6 +5495,17 @@ static struct phy_driver ksphy_driver[] = {
 	.suspend	= genphy_suspend,
 	.resume		= genphy_resume,
 	.get_features	= ksz9477_get_features,
+}, {
+	.phy_id		= PHY_ID_KSZ9897,
+	.phy_id_mask	= MICREL_PHY_ID_MASK,
+	.name		= "Microchip KSZ9897 Switch",
+	/* PHY_BASIC_FEATURES */
+	.config_init	= kszphy_config_init,
+	.config_aneg	= ksz8873mll_config_aneg,
+	.read_status	= ksz8873mll_read_status,
+	/* No suspend/resume callbacks because of errata DS00002330D:
+	 * Toggling PHY Powerdown can cause errors or link failures in adjacent PHYs
+	 */
 } };
 
 module_phy_driver(ksphy_driver);
@@ -5520,6 +5531,7 @@ static struct mdio_device_id __maybe_unused micrel_tbl[] = {
 	{ PHY_ID_LAN8814, MICREL_PHY_ID_MASK },
 	{ PHY_ID_LAN8804, MICREL_PHY_ID_MASK },
 	{ PHY_ID_LAN8841, MICREL_PHY_ID_MASK },
+	{ PHY_ID_KSZ9897, MICREL_PHY_ID_MASK },
 	{ }
 };
 
diff --git a/include/linux/micrel_phy.h b/include/linux/micrel_phy.h
index 591bf5b5e8dc..81cc16dc2ddf 100644
--- a/include/linux/micrel_phy.h
+++ b/include/linux/micrel_phy.h
@@ -39,6 +39,10 @@
 #define PHY_ID_KSZ87XX		0x00221550
 
 #define	PHY_ID_KSZ9477		0x00221631
+/* Pseudo ID to specify in compatible field of device tree.
+ * Otherwise the device reports the same ID as KSZ8081 on CPU ports.
+ */
+#define	PHY_ID_KSZ9897		0x002217ff
 
 /* struct phy_device dev_flags definitions */
 #define MICREL_PHY_50MHZ_CLK	BIT(0)
-- 
2.34.1

RE: [PATCH net v4 1/5] net: phy: micrel: add Microchip KSZ 9897 Switch PHY support

[Tristram.Ha]

> -----Original Message-----
> From: Enguerrand de Ribaucourt <enguerrand.de-ribaucourt@savoirfairelinux.com>
> Sent: Friday, May 31, 2024 7:24 AM
> To: netdev@vger.kernel.org
> Cc: andrew@lunn.ch; hkallweit1@gmail.com; linux@armlinux.org.uk; Woojung Huh -
> C21699 <Woojung.Huh@microchip.com>; UNGLinuxDriver
> <UNGLinuxDriver@microchip.com>; Enguerrand de Ribaucourt <enguerrand.de-
> ribaucourt@savoirfairelinux.com>
> Subject: [PATCH net v4 1/5] net: phy: micrel: add Microchip KSZ 9897 Switch PHY
> support
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content
> is safe
> 
> There is a DSA driver for microchip,ksz9897 which can be controlled
> through SPI or I2C. This patch adds support for it's CPU ports PHYs to
> also allow network access to the switch's CPU port.
> 
> The CPU ports PHYs of the KSZ9897 are not documented in the datasheet.
> They weirdly use the same PHY ID as the KSZ8081, which is a different
> PHY and that driver isn't compatible with KSZ9897. Before this patch,
> the KSZ8081 driver was used for the CPU ports of the KSZ9897 but the
> link would never come up.
> 
> A new driver for the KSZ9897 is added, based on the compatible KSZ87XX.
> I could not test if Gigabit Ethernet works, but the link comes up and
> can successfully allow packets to be sent and received with DSA tags.
> 
> To resolve the KSZ8081/KSZ9897 phy_id conflicts, I could not find any
> stable register to distinguish them. Instead of a match_phy_device() ,
> I've declared a virtual phy_id with the highest value in Microchip's OUI
> range.
> 
> Example usage in the device tree:
>         compatible = "ethernet-phy-id0022.17ff";
> 
> A discussion to find better alternatives had been opened with the
> Microchip team, with no response yet.
> 
> See https://lore.kernel.org/all/20220207174532.362781-1-enguerrand.de-
> ribaucourt@savoirfairelinux.com/
> 
> Fixes: b987e98e50ab ("dsa: add DSA switch driver for Microchip KSZ9477")
> Signed-off-by: Enguerrand de Ribaucourt <enguerrand.de-
> ribaucourt@savoirfairelinux.com>
> ---
> v4:
>  - rebase on net/main
>  - add Fixes tag
>  - use pseudo phy_id instead of of_tree search
> v3: https://lore.kernel.org/all/20240530102436.226189-2-enguerrand.de-
> ribaucourt@savoirfairelinux.com/
> ---
>  drivers/net/phy/micrel.c   | 14 +++++++++++++-
>  include/linux/micrel_phy.h |  4 ++++
>  2 files changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
> index 2b8f8b7f1517..8a6dfaceeab3 100644
> --- a/drivers/net/phy/micrel.c
> +++ b/drivers/net/phy/micrel.c
> @@ -16,7 +16,7 @@
>   *                        ksz8081, ksz8091,
>   *                        ksz8061,
>   *             Switch : ksz8873, ksz886x
> - *                      ksz9477, lan8804
> + *                      ksz9477, ksz9897, lan8804
>   */
> 
>  #include <linux/bitfield.h>
> @@ -5495,6 +5495,17 @@ static struct phy_driver ksphy_driver[] = {
>         .suspend        = genphy_suspend,
>         .resume         = genphy_resume,
>         .get_features   = ksz9477_get_features,
> +}, {
> +       .phy_id         = PHY_ID_KSZ9897,
> +       .phy_id_mask    = MICREL_PHY_ID_MASK,
> +       .name           = "Microchip KSZ9897 Switch",
> +       /* PHY_BASIC_FEATURES */
> +       .config_init    = kszphy_config_init,
> +       .config_aneg    = ksz8873mll_config_aneg,
> +       .read_status    = ksz8873mll_read_status,
> +       /* No suspend/resume callbacks because of errata DS00002330D:
> +        * Toggling PHY Powerdown can cause errors or link failures in adjacent PHYs
> +        */
>  } };
> 
>  module_phy_driver(ksphy_driver);
> @@ -5520,6 +5531,7 @@ static struct mdio_device_id __maybe_unused micrel_tbl[]
> = {
>         { PHY_ID_LAN8814, MICREL_PHY_ID_MASK },
>         { PHY_ID_LAN8804, MICREL_PHY_ID_MASK },
>         { PHY_ID_LAN8841, MICREL_PHY_ID_MASK },
> +       { PHY_ID_KSZ9897, MICREL_PHY_ID_MASK },
>         { }
>  };
> 
> diff --git a/include/linux/micrel_phy.h b/include/linux/micrel_phy.h
> index 591bf5b5e8dc..81cc16dc2ddf 100644
> --- a/include/linux/micrel_phy.h
> +++ b/include/linux/micrel_phy.h
> @@ -39,6 +39,10 @@
>  #define PHY_ID_KSZ87XX         0x00221550
> 
>  #define        PHY_ID_KSZ9477          0x00221631
> +/* Pseudo ID to specify in compatible field of device tree.
> + * Otherwise the device reports the same ID as KSZ8081 on CPU ports.
> + */
> +#define        PHY_ID_KSZ9897          0x002217ff
>

I am curious about this KSZ9897 device.  Can you point out its product
page on Microchip website?

KSZ9897 is typically referred to the KSZ9897 switch family, which
contains KSZ9897, KSZ9896, KSZ9567, KSZ8567, KSZ9477 and some others.

I am not aware that KSZ9897 has MDIO access.  The switch is only accessed
through I2C and SPI and proprietary IBA.

It seems the only function is just to report link so a fixed PHY should
be adequate in this situation.

MDIO only mode is present in KSZ8863/KSZ8873 switches.  I do not know
useful to use such mode in KSZ9897.

Re: [PATCH net v4 1/5] net: phy: micrel: add Microchip KSZ 9897 Switch PHY support

[Enguerrand de Ribaucourt]

On 31/05/2024 21:39, Tristram.Ha@microchip.com wrote:
> 
> 
>> -----Original Message-----
>> From: Enguerrand de Ribaucourt <enguerrand.de-ribaucourt@savoirfairelinux.com>
>> Sent: Friday, May 31, 2024 7:24 AM
>> diff --git a/include/linux/micrel_phy.h b/include/linux/micrel_phy.h
>> index 591bf5b5e8dc..81cc16dc2ddf 100644
>> --- a/include/linux/micrel_phy.h
>> +++ b/include/linux/micrel_phy.h
>> @@ -39,6 +39,10 @@
>>   #define PHY_ID_KSZ87XX         0x00221550
>>
>>   #define        PHY_ID_KSZ9477          0x00221631
>> +/* Pseudo ID to specify in compatible field of device tree.
>> + * Otherwise the device reports the same ID as KSZ8081 on CPU ports.
>> + */
>> +#define        PHY_ID_KSZ9897          0x002217ff
>>
> 
> I am curious about this KSZ9897 device.  Can you point out its product
> page on Microchip website?
> 
> KSZ9897 is typically referred to the KSZ9897 switch family, which
> contains KSZ9897, KSZ9896, KSZ9567, KSZ8567, KSZ9477 and some others.
> 
> I am not aware that KSZ9897 has MDIO access.  The switch is only accessed
> through I2C and SPI and proprietary IBA.
> 
> It seems the only function is just to report link so a fixed PHY should
> be adequate in this situation.
> 
> MDIO only mode is present in KSZ8863/KSZ8873 switches.  I do not know
> useful to use such mode in KSZ9897.
> 

I'm using the KSZ9897R from this page:
  - https://www.microchip.com/en-us/product/ksz9897

My CPU (i.MX6ULL) is connected to the CPU port 6 in RMII, listed in "Two 
Configurable External MAC Ports" with RGMII. This is for network 
connectivity with the switch, while I'm using SPI for DSA control. 
FIGURE 2-1 illustrates that architecture. However, this MDIO interface 
is indeed missing some documentation. For instance, it's phy_id is never 
listed (Section 5.2.2.3 only for ports 1-5).

I use a fixed-link property in the device tree, but the link would never 
come up if I use the KSZ8081 PHY driver which was selected without these 
patches because it emits the same phy_id as port 6. The genphy and 
KSZ9477 PHY dirvers were no better. I found out that the KSZ8873MLL 
driver was compatible though, which seems to make sense given your 
explanation.

-- 
Savoir-faire Linux
Enguerrand de Ribaucourt

Re: [PATCH net v4 1/5] net: phy: micrel: add Microchip KSZ 9897 Switch PHY support

[Simon Horman]

On Fri, May 31, 2024 at 02:24:26PM +0000, Enguerrand de Ribaucourt wrote:
> There is a DSA driver for microchip,ksz9897 which can be controlled
> through SPI or I2C. This patch adds support for it's CPU ports PHYs to
> also allow network access to the switch's CPU port.
> 
> The CPU ports PHYs of the KSZ9897 are not documented in the datasheet.
> They weirdly use the same PHY ID as the KSZ8081, which is a different
> PHY and that driver isn't compatible with KSZ9897. Before this patch,
> the KSZ8081 driver was used for the CPU ports of the KSZ9897 but the
> link would never come up.
> 
> A new driver for the KSZ9897 is added, based on the compatible KSZ87XX.
> I could not test if Gigabit Ethernet works, but the link comes up and
> can successfully allow packets to be sent and received with DSA tags.
> 
> To resolve the KSZ8081/KSZ9897 phy_id conflicts, I could not find any
> stable register to distinguish them. Instead of a match_phy_device() ,
> I've declared a virtual phy_id with the highest value in Microchip's OUI
> range.
> 
> Example usage in the device tree:
> 	compatible = "ethernet-phy-id0022.17ff";
> 
> A discussion to find better alternatives had been opened with the
> Microchip team, with no response yet.
> 
> See https://lore.kernel.org/all/20220207174532.362781-1-enguerrand.de-ribaucourt@savoirfairelinux.com/
> 
> Fixes: b987e98e50ab ("dsa: add DSA switch driver for Microchip KSZ9477")
> Signed-off-by: Enguerrand de Ribaucourt <enguerrand.de-ribaucourt@savoirfairelinux.com>

...

> diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c

...

> @@ -5495,6 +5495,17 @@ static struct phy_driver ksphy_driver[] = {
>  	.suspend	= genphy_suspend,
>  	.resume		= genphy_resume,
>  	.get_features	= ksz9477_get_features,
> +}, {
> +	.phy_id		= PHY_ID_KSZ9897,
> +	.phy_id_mask	= MICREL_PHY_ID_MASK,
> +	.name		= "Microchip KSZ9897 Switch",
> +	/* PHY_BASIC_FEATURES */
> +	.config_init	= kszphy_config_init,
> +	.config_aneg	= ksz8873mll_config_aneg,
> +	.read_status	= ksz8873mll_read_status,
> +	/* No suspend/resume callbacks because of errata DS00002330D:
> +	 * Toggling PHY Powerdown can cause errors or link failures in adjacent PHYs
> +	 */

It looks like there will be another version of this patchset.
If so, please line-wrap the comment above so it is 80 columns wide or less,
as is preferred for Networking code.

Likewise in the following patch.

Flagged by checkpatch.pl --max-line-length=80

>  } };
>  
>  module_phy_driver(ksphy_driver);

...

[PATCH net v4 2/5] net: phy: micrel: disable suspend/resume callbacks following errata

[Enguerrand de Ribaucourt]

Microchip's erratas state that powering down a PHY may cause errors
on the adjacent PHYs due to the power supply noise. The suggested
workaround is to avoid toggling the powerdown bit dynamically while
traffic may be present.

Fixes: fc3973a1fa09 ("phy: micrel: add Microchip KSZ 9477 Switch PHY support")
Signed-off-by: Enguerrand de Ribaucourt <enguerrand.de-ribaucourt@savoirfairelinux.com>
---
v4:
 - rebase on net/main
 - add Fixes tag
v3: https://lore.kernel.org/all/20240530102436.226189-3-enguerrand.de-ribaucourt@savoirfairelinux.com/
---
 drivers/net/phy/micrel.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
index 8a6dfaceeab3..2608d6cc7257 100644
--- a/drivers/net/phy/micrel.c
+++ b/drivers/net/phy/micrel.c
@@ -5492,8 +5492,9 @@ static struct phy_driver ksphy_driver[] = {
 	.config_init	= ksz9477_config_init,
 	.config_intr	= kszphy_config_intr,
 	.handle_interrupt = kszphy_handle_interrupt,
-	.suspend	= genphy_suspend,
-	.resume		= genphy_resume,
+	/* No suspend/resume callbacks because of errata DS80000758:
+	 * Toggling PHY Powerdown can cause errors or link failures in adjacent PHYs
+	 */
 	.get_features	= ksz9477_get_features,
 }, {
 	.phy_id		= PHY_ID_KSZ9897,
-- 
2.34.1

RE: [PATCH net v4 2/5] net: phy: micrel: disable suspend/resume callbacks following errata

[Tristram.Ha]

> -----Original Message-----
> From: Enguerrand de Ribaucourt <enguerrand.de-ribaucourt@savoirfairelinux.com>
> Sent: Friday, May 31, 2024 7:24 AM
> To: netdev@vger.kernel.org
> Cc: andrew@lunn.ch; hkallweit1@gmail.com; linux@armlinux.org.uk; Woojung Huh -
> C21699 <Woojung.Huh@microchip.com>; UNGLinuxDriver
> <UNGLinuxDriver@microchip.com>; Enguerrand de Ribaucourt <enguerrand.de-
> ribaucourt@savoirfairelinux.com>
> Subject: [PATCH net v4 2/5] net: phy: micrel: disable suspend/resume callbacks
> following errata
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content
> is safe
> 
> Microchip's erratas state that powering down a PHY may cause errors
> on the adjacent PHYs due to the power supply noise. The suggested
> workaround is to avoid toggling the powerdown bit dynamically while
> traffic may be present.
> 
> Fixes: fc3973a1fa09 ("phy: micrel: add Microchip KSZ 9477 Switch PHY support")
> Signed-off-by: Enguerrand de Ribaucourt <enguerrand.de-
> ribaucourt@savoirfairelinux.com>
> ---
> v4:
>  - rebase on net/main
>  - add Fixes tag
> v3: https://lore.kernel.org/all/20240530102436.226189-3-enguerrand.de-
> ribaucourt@savoirfairelinux.com/
> ---
>  drivers/net/phy/micrel.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
> index 8a6dfaceeab3..2608d6cc7257 100644
> --- a/drivers/net/phy/micrel.c
> +++ b/drivers/net/phy/micrel.c
> @@ -5492,8 +5492,9 @@ static struct phy_driver ksphy_driver[] = {
>         .config_init    = ksz9477_config_init,
>         .config_intr    = kszphy_config_intr,
>         .handle_interrupt = kszphy_handle_interrupt,
> -       .suspend        = genphy_suspend,
> -       .resume         = genphy_resume,
> +       /* No suspend/resume callbacks because of errata DS80000758:
> +        * Toggling PHY Powerdown can cause errors or link failures in adjacent PHYs
> +        */
>         .get_features   = ksz9477_get_features,
>  }, {
>         .phy_id         = PHY_ID_KSZ9897,

KSZ9893 uses the same PHY driver as KSZ9477.  KSZ9893 belongs to KSZ9893
family while KSZ9477 belongs to KSZ9897 family.  They share most
registers but KSZ9893 does not require PHY setup for link compatibility
and does not have this PHY power up link lost issue, so it is not
appropriate to completely disable PHY power down.

PHY power down is executed when the network device is turned off.  The
PHY is powered up when the network device is turned on.  This sometimes
can cause other ports in KSZ9897 switch to lose link temporarily.  The
link will come back.

In my opinion this problem does not impact much as the network devices
are not likely to be turned off/on many times and likely to be turned
off once during system initialization.

[PATCH net v4 3/5] net: phy: micrel: add Microchip KSZ 9477 to the device table

[Enguerrand de Ribaucourt]

PHY_ID_KSZ9477 was supported but not added to the device table passed to
MODULE_DEVICE_TABLE.

Fixes: fc3973a1fa09 ("phy: micrel: add Microchip KSZ 9477 Switch PHY support")
Signed-off-by: Enguerrand de Ribaucourt <enguerrand.de-ribaucourt@savoirfairelinux.com>
---
v4:
 - rebase on net/main
 - add Fixes tag
v3: https://lore.kernel.org/all/20240530102436.226189-4-enguerrand.de-ribaucourt@savoirfairelinux.com/
---
 drivers/net/phy/micrel.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
index 2608d6cc7257..f700485793bd 100644
--- a/drivers/net/phy/micrel.c
+++ b/drivers/net/phy/micrel.c
@@ -5529,6 +5529,7 @@ static struct mdio_device_id __maybe_unused micrel_tbl[] = {
 	{ PHY_ID_KSZ8081, MICREL_PHY_ID_MASK },
 	{ PHY_ID_KSZ8873MLL, MICREL_PHY_ID_MASK },
 	{ PHY_ID_KSZ886X, MICREL_PHY_ID_MASK },
+	{ PHY_ID_KSZ9477, MICREL_PHY_ID_MASK },
 	{ PHY_ID_LAN8814, MICREL_PHY_ID_MASK },
 	{ PHY_ID_LAN8804, MICREL_PHY_ID_MASK },
 	{ PHY_ID_LAN8841, MICREL_PHY_ID_MASK },
-- 
2.34.1

[PATCH net v4 4/5] net: dsa: microchip: use collision based back pressure mode

[Enguerrand de Ribaucourt]

Errata DS80000758 states that carrier sense back pressure mode can cause
link down issues in 100BASE-TX half duplex mode. The datasheet also
recommends to always use the collision based back pressure mode.

Fixes: b987e98e50ab ("dsa: add DSA switch driver for Microchip KSZ9477")
Signed-off-by: Enguerrand de Ribaucourt <enguerrand.de-ribaucourt@savoirfairelinux.com>
---
v4:
 - rebase on net/main
 - add Fixes tag
v3: https://lore.kernel.org/all/20240530102436.226189-5-enguerrand.de-ribaucourt@savoirfairelinux.com/
---
 drivers/net/dsa/microchip/ksz9477.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
index f8ad7833f5d9..343b9d7538e9 100644
--- a/drivers/net/dsa/microchip/ksz9477.c
+++ b/drivers/net/dsa/microchip/ksz9477.c
@@ -1299,6 +1299,9 @@ int ksz9477_setup(struct dsa_switch *ds)
 	/* Enable REG_SW_MTU__2 reg by setting SW_JUMBO_PACKET */
 	ksz_cfg(dev, REG_SW_MAC_CTRL_1, SW_JUMBO_PACKET, true);
 
+	/* Use collision based back pressure mode. */
+	ksz_cfg(dev, REG_SW_MAC_CTRL_1, SW_BACK_PRESSURE, false);
+
 	/* Now we can configure default MTU value */
 	ret = regmap_update_bits(ksz_regmap_16(dev), REG_SW_MTU__2, REG_SW_MTU_MASK,
 				 VLAN_ETH_FRAME_LEN + ETH_FCS_LEN);
-- 
2.34.1

Re: [PATCH net v4 4/5] net: dsa: microchip: use collision based back pressure mode

[Arun.Ramadoss]

Hi Enguerrand,

> 
> diff --git a/drivers/net/dsa/microchip/ksz9477.c
> b/drivers/net/dsa/microchip/ksz9477.c
> index f8ad7833f5d9..343b9d7538e9 100644
> --- a/drivers/net/dsa/microchip/ksz9477.c
> +++ b/drivers/net/dsa/microchip/ksz9477.c
> @@ -1299,6 +1299,9 @@ int ksz9477_setup(struct dsa_switch *ds)
>         /* Enable REG_SW_MTU__2 reg by setting SW_JUMBO_PACKET */
>         ksz_cfg(dev, REG_SW_MAC_CTRL_1, SW_JUMBO_PACKET, true);
> 
> +       /* Use collision based back pressure mode. */
> +       ksz_cfg(dev, REG_SW_MAC_CTRL_1, SW_BACK_PRESSURE, false);

nitpick: Comment says use collision based back pressure mode, but in
the register write we are clearing it (false). From the datasheet, it
mentions 1 - Carrier based back pressure 0 - Collision based back
pressure. Instead of false, we can have macros like below or something
similar

#define SW_BACK_PRESSURE_COLLISION 0

> +
>         /* Now we can configure default MTU value */
>         ret = regmap_update_bits(ksz_regmap_16(dev), REG_SW_MTU__2,
> REG_SW_MTU_MASK,
>                                  VLAN_ETH_FRAME_LEN + ETH_FCS_LEN);
> --
> 2.34.1
> 

[PATCH net v4 5/5] net: dsa: microchip: monitor potential faults in half-duplex mode

[Enguerrand de Ribaucourt]

The errata DS80000754 recommends monitoring potential faults in
half-duplex mode for the KSZ9477 familly.

half-duplex is not very common so I just added a critical message
when the fault conditions are detected. The switch can be expected
to be unable to communicate anymore in these states and a software
reset of the switch would be required which I did not implement.

Fixes: b987e98e50ab ("dsa: add DSA switch driver for Microchip KSZ9477")
Signed-off-by: Enguerrand de Ribaucourt <enguerrand.de-ribaucourt@savoirfairelinux.com>
---
v4:
 - rebase on net/main
 - add Fixes tag
 - reverse x-mas tree
v3: https://lore.kernel.org/all/20240530102436.226189-6-enguerrand.de-ribaucourt@savoirfairelinux.com/
---
 drivers/net/dsa/microchip/ksz9477.c     | 34 +++++++++++++++++++++++++
 drivers/net/dsa/microchip/ksz9477.h     |  2 ++
 drivers/net/dsa/microchip/ksz9477_reg.h |  8 ++++--
 drivers/net/dsa/microchip/ksz_common.c  |  7 +++++
 drivers/net/dsa/microchip/ksz_common.h  |  1 +
 5 files changed, 50 insertions(+), 2 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
index 343b9d7538e9..9c69c78c0b92 100644
--- a/drivers/net/dsa/microchip/ksz9477.c
+++ b/drivers/net/dsa/microchip/ksz9477.c
@@ -429,6 +429,40 @@ void ksz9477_freeze_mib(struct ksz_device *dev, int port, bool freeze)
 	mutex_unlock(&p->mib.cnt_mutex);
 }
 
+void ksz9477_errata_monitor(struct ksz_device *dev, int port,
+			    u64 tx_late_col)
+{
+	u32 pmavbc;
+	u8 status;
+	u16 pqm;
+
+	ksz_pread8(dev, port, REG_PORT_STATUS_0, &status);
+	if (!((status & PORT_INTF_SPEED_MASK) == PORT_INTF_SPEED_MASK) &&
+	    !(status & PORT_INTF_FULL_DUPLEX)) {
+		dev_warn_once(dev->dev,
+			      "Half-duplex detected on port %d, transmission halt may occur\n",
+			      port);
+		/* Errata DS80000754 recommends monitoring potential faults in
+		 * half-duplex mode. The switch might not be able to communicate anymore
+		 * in these states.
+		 */
+		if (tx_late_col != 0) {
+			/* Transmission halt with late collisions */
+			dev_crit_ratelimited(dev->dev,
+					     "TX late collisions detected, transmission may be halted on port %d\n",
+					     port);
+		}
+		ksz_pread16(dev, port, REG_PORT_QM_TX_CNT_0__4, &pqm);
+		ksz_read32(dev, REG_PMAVBC, &pmavbc);
+		if (((pmavbc & PMAVBC_MASK) >> PMAVBC_SHIFT <= 0x580) ||
+		    ((pqm & PORT_QM_TX_CNT_M) >= 0x200)) {
+			/* Transmission halt with Half-Duplex and VLAN */
+			dev_crit_ratelimited(dev->dev,
+					     "resources out of limits, transmission may be halted\n");
+		}
+	}
+}
+
 void ksz9477_port_init_cnt(struct ksz_device *dev, int port)
 {
 	struct ksz_port_mib *mib = &dev->ports[port].mib;
diff --git a/drivers/net/dsa/microchip/ksz9477.h b/drivers/net/dsa/microchip/ksz9477.h
index ce1e656b800b..3312ef28e99c 100644
--- a/drivers/net/dsa/microchip/ksz9477.h
+++ b/drivers/net/dsa/microchip/ksz9477.h
@@ -36,6 +36,8 @@ int ksz9477_port_mirror_add(struct ksz_device *dev, int port,
 			    bool ingress, struct netlink_ext_ack *extack);
 void ksz9477_port_mirror_del(struct ksz_device *dev, int port,
 			     struct dsa_mall_mirror_tc_entry *mirror);
+void ksz9477_errata_monitor(struct ksz_device *dev, int port,
+			    u64 tx_late_col);
 void ksz9477_get_caps(struct ksz_device *dev, int port,
 		      struct phylink_config *config);
 int ksz9477_fdb_dump(struct ksz_device *dev, int port,
diff --git a/drivers/net/dsa/microchip/ksz9477_reg.h b/drivers/net/dsa/microchip/ksz9477_reg.h
index f3a205ee483f..3238b9748d0f 100644
--- a/drivers/net/dsa/microchip/ksz9477_reg.h
+++ b/drivers/net/dsa/microchip/ksz9477_reg.h
@@ -842,8 +842,7 @@
 
 #define REG_PORT_STATUS_0		0x0030
 
-#define PORT_INTF_SPEED_M		0x3
-#define PORT_INTF_SPEED_S		3
+#define PORT_INTF_SPEED_MASK		0x0018
 #define PORT_INTF_FULL_DUPLEX		BIT(2)
 #define PORT_TX_FLOW_CTRL		BIT(1)
 #define PORT_RX_FLOW_CTRL		BIT(0)
@@ -1167,6 +1166,11 @@
 #define PORT_RMII_CLK_SEL		BIT(7)
 #define PORT_MII_SEL_EDGE		BIT(5)
 
+#define REG_PMAVBC				0x03AC
+
+#define PMAVBC_MASK				0x7ff0000
+#define PMAVBC_SHIFT			16
+
 /* 4 - MAC */
 #define REG_PORT_MAC_CTRL_0		0x0400
 
diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index 2818e24e2a51..edc7467f101d 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -1382,6 +1382,7 @@ const struct ksz_chip_data ksz_switch_chips[] = {
 		.tc_cbs_supported = true,
 		.ops = &ksz9477_dev_ops,
 		.phylink_mac_ops = &ksz9477_phylink_mac_ops,
+		.phy_errata_9477 = true,
 		.mib_names = ksz9477_mib_names,
 		.mib_cnt = ARRAY_SIZE(ksz9477_mib_names),
 		.reg_mib_cnt = MIB_COUNTER_NUM,
@@ -1416,6 +1417,7 @@ const struct ksz_chip_data ksz_switch_chips[] = {
 		.num_ipms = 8,
 		.ops = &ksz9477_dev_ops,
 		.phylink_mac_ops = &ksz9477_phylink_mac_ops,
+		.phy_errata_9477 = true,
 		.mib_names = ksz9477_mib_names,
 		.mib_cnt = ARRAY_SIZE(ksz9477_mib_names),
 		.reg_mib_cnt = MIB_COUNTER_NUM,
@@ -1450,6 +1452,7 @@ const struct ksz_chip_data ksz_switch_chips[] = {
 		.num_ipms = 8,
 		.ops = &ksz9477_dev_ops,
 		.phylink_mac_ops = &ksz9477_phylink_mac_ops,
+		.phy_errata_9477 = true,
 		.mib_names = ksz9477_mib_names,
 		.mib_cnt = ARRAY_SIZE(ksz9477_mib_names),
 		.reg_mib_cnt = MIB_COUNTER_NUM,
@@ -1540,6 +1543,7 @@ const struct ksz_chip_data ksz_switch_chips[] = {
 		.tc_cbs_supported = true,
 		.ops = &ksz9477_dev_ops,
 		.phylink_mac_ops = &ksz9477_phylink_mac_ops,
+		.phy_errata_9477 = true,
 		.mib_names = ksz9477_mib_names,
 		.mib_cnt = ARRAY_SIZE(ksz9477_mib_names),
 		.reg_mib_cnt = MIB_COUNTER_NUM,
@@ -1861,6 +1865,9 @@ void ksz_r_mib_stats64(struct ksz_device *dev, int port)
 	pstats->rx_pause_frames = raw->rx_pause;
 
 	spin_unlock(&mib->stats64_lock);
+
+	if (dev->info->phy_errata_9477)
+		ksz9477_errata_monitor(dev, port, raw->tx_late_col);
 }
 
 void ksz88xx_r_mib_stats64(struct ksz_device *dev, int port)
diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
index c784fd23a993..ee7db46e469d 100644
--- a/drivers/net/dsa/microchip/ksz_common.h
+++ b/drivers/net/dsa/microchip/ksz_common.h
@@ -66,6 +66,7 @@ struct ksz_chip_data {
 	bool tc_cbs_supported;
 	const struct ksz_dev_ops *ops;
 	const struct phylink_mac_ops *phylink_mac_ops;
+	bool phy_errata_9477;
 	bool ksz87xx_eee_link_erratum;
 	const struct ksz_mib_names *mib_names;
 	int mib_cnt;
-- 
2.34.1

Re: [PATCH net v4 5/5] net: dsa: microchip: monitor potential faults in half-duplex mode

[Arun.Ramadoss]

Hi Enguerrand,

On Fri, 2024-05-31 at 14:24 +0000, Enguerrand de Ribaucourt wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> The errata DS80000754 recommends monitoring potential faults in
> half-duplex mode for the KSZ9477 familly.
> 
> half-duplex is not very common so I just added a critical message
> when the fault conditions are detected. The switch can be expected
> to be unable to communicate anymore in these states and a software
> reset of the switch would be required which I did not implement.
> 
> 
>  
> 
> +void ksz9477_errata_monitor(struct ksz_device *dev, int port,
> +                           u64 tx_late_col)
> +{
> +       u32 pmavbc;
> +       u8 status;
> +       u16 pqm;
> +
> +       ksz_pread8(dev, port, REG_PORT_STATUS_0, &status);

It good to check return value for ksz_pread8/16/32.

> +       if (!((status & PORT_INTF_SPEED_MASK) ==
> PORT_INTF_SPEED_MASK) &&
> +           !(status & PORT_INTF_FULL_DUPLEX)) {
> +               dev_warn_once(dev->dev,
> +                             "Half-duplex detected on port %d,
> transmission halt may occur\n",
> +                             port);
> +               /* Errata DS80000754 recommends monitoring potential
> faults in
> +                * half-duplex mode. The switch might not be able to
> communicate anymore
> +                * in these states.
> +                */
> +               if (tx_late_col != 0) {
> +                       /* Transmission halt with late collisions */
> +                       dev_crit_ratelimited(dev->dev,
> +                                            "TX late collisions
> detected, transmission may be halted on port %d\n",
> +                                            port);
> +               }
> +               ksz_pread16(dev, port, REG_PORT_QM_TX_CNT_0__4,
> &pqm);
> +               ksz_read32(dev, REG_PMAVBC, &pmavbc);
> +               if (((pmavbc & PMAVBC_MASK) >> PMAVBC_SHIFT <= 0x580)

magic numbers can be replaced by macros.
Also it can be replaced by FIELD_GET(PMAVBC_MASK, pmavbc)
> ||
> +                   ((pqm & PORT_QM_TX_CNT_M) >= 0x200)) {
> +                       /* Transmission halt with Half-Duplex and
> VLAN */
> +                       dev_crit_ratelimited(dev->dev,
> +                                            "resources out of
> limits, transmission may be halted\n");
> +               }
> +       }
> +}
> +
>  void ksz9477_port_init_cnt(struct ksz_device *dev, int port)
>  {
>         struct ksz_port_mib *mib = &dev->ports[port].mib;
> diff --git a/drivers/net/dsa/microchip/ksz9477.h
> b/drivers/net/dsa/microchip/ksz9477.h
> index ce1e656b800b..3312ef28e99c 100644
> --- a/drivers/net/dsa/microchip/ksz9477.h
> +++ b/drivers/net/dsa/microchip/ksz9477.h
> @@ -36,6 +36,8 @@ int ksz9477_port_mirror_add(struct ksz_device *dev,
> int port,
>                             bool ingress, struct netlink_ext_ack
> *extack);
>  void ksz9477_port_mirror_del(struct ksz_device *dev, int port,
>                              struct dsa_mall_mirror_tc_entry
> *mirror);
> +void ksz9477_errata_monitor(struct ksz_device *dev, int port,
> +                           u64 tx_late_col);
>  void ksz9477_get_caps(struct ksz_device *dev, int port,
>                       struct phylink_config *config);
>  int ksz9477_fdb_dump(struct ksz_device *dev, int port,
> diff --git a/drivers/net/dsa/microchip/ksz9477_reg.h
> b/drivers/net/dsa/microchip/ksz9477_reg.h
> index f3a205ee483f..3238b9748d0f 100644
> --- a/drivers/net/dsa/microchip/ksz9477_reg.h
> +++ b/drivers/net/dsa/microchip/ksz9477_reg.h
> @@ -842,8 +842,7 @@
> 
>  #define REG_PORT_STATUS_0              0x0030
> 
> -#define PORT_INTF_SPEED_M              0x3
> -#define PORT_INTF_SPEED_S              3
> +#define PORT_INTF_SPEED_MASK           0x0018

nitpick: it can be replaced by GENMASK(4,3)

>  #define PORT_INTF_FULL_DUPLEX          BIT(2)
>  #define PORT_TX_FLOW_CTRL              BIT(1)
>  #define PORT_RX_FLOW_CTRL              BIT(0)
> @@ -1167,6 +1166,11 @@
>  #define PORT_RMII_CLK_SEL              BIT(7)
>  #define PORT_MII_SEL_EDGE              BIT(5)
> 
> +#define REG_PMAVBC                             0x03AC
> +
> +#define PMAVBC_MASK                            0x7ff0000

nitpick: it can be replaced by GENMASK(26, 16)

> +#define PMAVBC_SHIFT                   16
> +
>  /* 4 - MAC */
>  #define REG_PORT_MAC_CTRL_0            0x0400
> 
> 
> 

Re: [PATCH net v4 5/5] net: dsa: microchip: monitor potential faults in half-duplex mode

[Simon Horman]

On Fri, May 31, 2024 at 02:24:30PM +0000, Enguerrand de Ribaucourt wrote:
> The errata DS80000754 recommends monitoring potential faults in
> half-duplex mode for the KSZ9477 familly.

nit: family

     checkpatch.py --codespell is your friend.

> 
> half-duplex is not very common so I just added a critical message
> when the fault conditions are detected. The switch can be expected
> to be unable to communicate anymore in these states and a software
> reset of the switch would be required which I did not implement.
> 
> Fixes: b987e98e50ab ("dsa: add DSA switch driver for Microchip KSZ9477")
> Signed-off-by: Enguerrand de Ribaucourt <enguerrand.de-ribaucourt@savoirfairelinux.com>

...

[PATCH net v5 0/5] Add Microchip KSZ 9897 Switch CPU PHY + Errata

[Enguerrand de Ribaucourt]

Back in 2022, I had posted a series of patches to support the KSZ9897
switch's CPU PHY ports but some discussions had not been concluded with
Microchip. I've been maintaining the patches since and I'm now
resubmitting them with some improvements to handle new KSZ9897 errata
sheets (also concerning the whole KSZ9477 family).

I'm very much listening for feedback on these patches. Please let me know if you
have any suggestions or concerns. Thank you.

---
v5:
 - use macros for bitfields
 - rewrap comments
 - check ksz_pread* return values
 - fix spelling mistakes
 - remove KSZ9477 suspend/resume deletion patch
v4: https://lore.kernel.org/all/20240531142430.678198-1-enguerrand.de-ribaucourt@savoirfairelinux.com/
 - Rebase on net/main
 - Add Fixes: tags to the patches
 - reverse x-mas tree order
 - use pseudo phy_id instead of match_phy_device
v3: https://lore.kernel.org/all/20240530102436.226189-1-enguerrand.de-ribaucourt@savoirfairelinux.com/

Re: [PATCH net v5 0/5] Add Microchip KSZ 9897 Switch CPU PHY + Errata

[Jakub Kicinski]

On Tue,  4 Jun 2024 09:23:01 +0000 Enguerrand de Ribaucourt wrote:
> Back in 2022, I had posted a series of patches to support the KSZ9897
> switch's CPU PHY ports but some discussions had not been concluded with
> Microchip. I've been maintaining the patches since and I'm now
> resubmitting them with some improvements to handle new KSZ9897 errata
> sheets (also concerning the whole KSZ9477 family).
> 
> I'm very much listening for feedback on these patches. Please let me know if you
> have any suggestions or concerns. Thank you.

The v5 patches did not get grouped correctly by patchwork:
https://patchwork.kernel.org/project/netdevbpf/list/?series=858591&state=*

Don't try to do fancy --in-reply-to, please, just resend as a new
thread. You're already adding lore links for folks to find previous
versions (which is great!)

RE: [PATCH net v5 0/5] Add Microchip KSZ 9897 Switch CPU PHY + Errata

[Woojung.Huh]

Hi Enguerrand,

Thanks for the patch.

Because some reviewers are out for vacation, I'll try my best to review
even though I'm not directly involved in this system.
Please pardon me if asking same question again.

Thanks.
Woojung

> -----Original Message-----
> From: Enguerrand de Ribaucourt <enguerrand.de-
> ribaucourt@savoirfairelinux.com>
> Sent: Tuesday, June 4, 2024 5:23 AM
> To: netdev@vger.kernel.org
> Cc: andrew@lunn.ch; hkallweit1@gmail.com; linux@armlinux.org.uk; Woojung Huh
> - C21699 <Woojung.Huh@microchip.com>; UNGLinuxDriver
> <UNGLinuxDriver@microchip.com>; horms@kernel.org; Tristram Ha - C24268
> <Tristram.Ha@microchip.com>; Arun Ramadoss - I17769
> <Arun.Ramadoss@microchip.com>
> Subject: [PATCH net v5 0/5] Add Microchip KSZ 9897 Switch CPU PHY + Errata
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> Back in 2022, I had posted a series of patches to support the KSZ9897
> switch's CPU PHY ports but some discussions had not been concluded with
> Microchip. I've been maintaining the patches since and I'm now
> resubmitting them with some improvements to handle new KSZ9897 errata
> sheets (also concerning the whole KSZ9477 family).
> 
> I'm very much listening for feedback on these patches. Please let me know if
> you
> have any suggestions or concerns. Thank you.
> 
> ---
> v5:
>  - use macros for bitfields
>  - rewrap comments
>  - check ksz_pread* return values
>  - fix spelling mistakes
>  - remove KSZ9477 suspend/resume deletion patch
> v4: https://lore.kernel.org/all/20240531142430.678198-1-enguerrand.de-
> ribaucourt@savoirfairelinux.com/
>  - Rebase on net/main
>  - Add Fixes: tags to the patches
>  - reverse x-mas tree order
>  - use pseudo phy_id instead of match_phy_device
> v3: https://lore.kernel.org/all/20240530102436.226189-1-enguerrand.de-
> ribaucourt@savoirfairelinux.com/
> 

[PATCH net v5 1/4] net: phy: micrel: add Microchip KSZ 9897 Switch PHY support

[Enguerrand de Ribaucourt]

There is a DSA driver for microchip,ksz9897 which can be controlled
through SPI or I2C. This patch adds support for it's CPU ports PHYs to
also allow network access to the switch's CPU port.

The CPU ports PHYs of the KSZ9897 are not documented in the datasheet.
They weirdly use the same PHY ID as the KSZ8081, which is a different
PHY and that driver isn't compatible with KSZ9897. Before this patch,
the KSZ8081 driver was used for the CPU ports of the KSZ9897 but the
link would never come up.

A new driver for the KSZ9897 is added, based on the compatible KSZ87XX.
I could not test if Gigabit Ethernet works, but the link comes up and
can successfully allow packets to be sent and received with DSA tags.

To resolve the KSZ8081/KSZ9897 phy_id conflicts, I could not find any
stable register to distinguish them. Instead of a match_phy_device() ,
I've declared a virtual phy_id with the highest value in Microchip's OUI
range.

Example usage in the device tree:
	compatible = "ethernet-phy-id0022.17ff";

A discussion to find better alternatives had been opened with the
Microchip team, with no response yet.

See https://lore.kernel.org/all/20220207174532.362781-1-enguerrand.de-ribaucourt@savoirfairelinux.com/

Fixes: b987e98e50ab ("dsa: add DSA switch driver for Microchip KSZ9477")
Signed-off-by: Enguerrand de Ribaucourt <enguerrand.de-ribaucourt@savoirfairelinux.com>
---
v5:
 - rewrap comments
 - restore suspend/resume for KSZ9897
v4: https://lore.kernel.org/all/20240531142430.678198-2-enguerrand.de-ribaucourt@savoirfairelinux.com/
 - rebase on net/main
 - add Fixes tag
 - use pseudo phy_id instead of of_tree search
v3: https://lore.kernel.org/all/20240530102436.226189-2-enguerrand.de-ribaucourt@savoirfairelinux.com/
---
 drivers/net/phy/micrel.c   | 13 ++++++++++++-
 include/linux/micrel_phy.h |  4 ++++
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
index 8c20cf937530..11e58fc628df 100644
--- a/drivers/net/phy/micrel.c
+++ b/drivers/net/phy/micrel.c
@@ -16,7 +16,7 @@
  *			   ksz8081, ksz8091,
  *			   ksz8061,
  *		Switch : ksz8873, ksz886x
- *			 ksz9477, lan8804
+ *			 ksz9477, ksz9897, lan8804
  */
 
 #include <linux/bitfield.h>
@@ -5545,6 +5545,16 @@ static struct phy_driver ksphy_driver[] = {
 	.suspend	= genphy_suspend,
 	.resume		= ksz9477_resume,
 	.get_features	= ksz9477_get_features,
+}, {
+	.phy_id		= PHY_ID_KSZ9897,
+	.phy_id_mask	= MICREL_PHY_ID_MASK,
+	.name		= "Microchip KSZ9897 Switch",
+	/* PHY_BASIC_FEATURES */
+	.config_init	= kszphy_config_init,
+	.config_aneg	= ksz8873mll_config_aneg,
+	.read_status	= ksz8873mll_read_status,
+	.suspend	= genphy_suspend,
+	.resume		= genphy_resume,
 } };
 
 module_phy_driver(ksphy_driver);
@@ -5570,6 +5580,7 @@ static struct mdio_device_id __maybe_unused micrel_tbl[] = {
 	{ PHY_ID_LAN8814, MICREL_PHY_ID_MASK },
 	{ PHY_ID_LAN8804, MICREL_PHY_ID_MASK },
 	{ PHY_ID_LAN8841, MICREL_PHY_ID_MASK },
+	{ PHY_ID_KSZ9897, MICREL_PHY_ID_MASK },
 	{ }
 };
 
diff --git a/include/linux/micrel_phy.h b/include/linux/micrel_phy.h
index 591bf5b5e8dc..81cc16dc2ddf 100644
--- a/include/linux/micrel_phy.h
+++ b/include/linux/micrel_phy.h
@@ -39,6 +39,10 @@
 #define PHY_ID_KSZ87XX		0x00221550
 
 #define	PHY_ID_KSZ9477		0x00221631
+/* Pseudo ID to specify in compatible field of device tree.
+ * Otherwise the device reports the same ID as KSZ8081 on CPU ports.
+ */
+#define	PHY_ID_KSZ9897		0x002217ff
 
 /* struct phy_device dev_flags definitions */
 #define MICREL_PHY_50MHZ_CLK	BIT(0)
-- 
2.34.1

RE: [PATCH net v5 1/4] net: phy: micrel: add Microchip KSZ 9897 Switch PHY support

[Woojung.Huh]

Hi Enguerrand, 

Can you help me to understand your setup? I could see you are using
 - Host CPU : i.MX6ULL
 - DSA Switch : KSZ9897R (https://www.microchip.com/en-us/product/ksz9897)
 - Host-to-KSZ interface : RGMII for data path & SPI for control
Based on this, CPU port is either GMAC6 or GMAC7 (Figure 2-1 of [1])

I have two questions for you.
1. PHY on CPU port
   Which GMAC (or port number) is connected between Host CPU and KSZ9897R?
   If CPU port is either GMAC6 or GMAC7, it is just a MAC-to-MAC connection over RGMII.
2. PHY ID
   Its PHY ID is different when checking datasheet of KSZ9897 and KSZ8081.
   PHY ID of Port 1-5 of KSZ9897 is 0x0022-0x1631 per [1]
   PHY ID of KSZ8081 is 0x0022-0x0156x per [2]

Beside patch, you can create a ticket to Microchip site (https://microchipsupport.force.com/s/supportservice)
if you think it is easier to solve your problem.

Best regards,
Woojung

[1] https://ww1.microchip.com/downloads/aemDocuments/documents/OTH/ProductDocuments/DataSheets/KSZ9897R-Data-Sheet-DS00002330D.pdf
[2] https://www.microchip.com/en-us/product/ksz8081#document-table

> -----Original Message-----
> From: Enguerrand de Ribaucourt <enguerrand.de-
> ribaucourt@savoirfairelinux.com>
> Sent: Tuesday, June 4, 2024 5:23 AM
> To: netdev@vger.kernel.org
> Cc: andrew@lunn.ch; hkallweit1@gmail.com; linux@armlinux.org.uk; Woojung Huh
> - C21699 <Woojung.Huh@microchip.com>; UNGLinuxDriver
> <UNGLinuxDriver@microchip.com>; horms@kernel.org; Tristram Ha - C24268
> <Tristram.Ha@microchip.com>; Arun Ramadoss - I17769
> <Arun.Ramadoss@microchip.com>; Enguerrand de Ribaucourt <enguerrand.de-
> ribaucourt@savoirfairelinux.com>
> Subject: [PATCH net v5 1/4] net: phy: micrel: add Microchip KSZ 9897 Switch
> PHY support
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> There is a DSA driver for microchip,ksz9897 which can be controlled
> through SPI or I2C. This patch adds support for it's CPU ports PHYs to
> also allow network access to the switch's CPU port.
> 
> The CPU ports PHYs of the KSZ9897 are not documented in the datasheet.
> They weirdly use the same PHY ID as the KSZ8081, which is a different
> PHY and that driver isn't compatible with KSZ9897. Before this patch,
> the KSZ8081 driver was used for the CPU ports of the KSZ9897 but the
> link would never come up.
> 
> A new driver for the KSZ9897 is added, based on the compatible KSZ87XX.
> I could not test if Gigabit Ethernet works, but the link comes up and
> can successfully allow packets to be sent and received with DSA tags.
> 
> To resolve the KSZ8081/KSZ9897 phy_id conflicts, I could not find any
> stable register to distinguish them. Instead of a match_phy_device() ,
> I've declared a virtual phy_id with the highest value in Microchip's OUI
> range.
> 
> Example usage in the device tree:
>         compatible = "ethernet-phy-id0022.17ff";
> 
> A discussion to find better alternatives had been opened with the
> Microchip team, with no response yet.
> 
> See https://lore.kernel.org/all/20220207174532.362781-1-enguerrand.de-
> ribaucourt@savoirfairelinux.com/
> 
> Fixes: b987e98e50ab ("dsa: add DSA switch driver for Microchip KSZ9477")
> Signed-off-by: Enguerrand de Ribaucourt <enguerrand.de-
> ribaucourt@savoirfairelinux.com>
> ---
> v5:
>  - rewrap comments
>  - restore suspend/resume for KSZ9897
> v4: https://lore.kernel.org/all/20240531142430.678198-2-enguerrand.de-
> ribaucourt@savoirfairelinux.com/
>  - rebase on net/main
>  - add Fixes tag
>  - use pseudo phy_id instead of of_tree search
> v3: https://lore.kernel.org/all/20240530102436.226189-2-enguerrand.de-
> ribaucourt@savoirfairelinux.com/
> ---
>  drivers/net/phy/micrel.c   | 13 ++++++++++++-
>  include/linux/micrel_phy.h |  4 ++++
>  2 files changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
> index 8c20cf937530..11e58fc628df 100644
> --- a/drivers/net/phy/micrel.c
> +++ b/drivers/net/phy/micrel.c
> @@ -16,7 +16,7 @@
>   *                        ksz8081, ksz8091,
>   *                        ksz8061,
>   *             Switch : ksz8873, ksz886x
> - *                      ksz9477, lan8804
> + *                      ksz9477, ksz9897, lan8804
>   */
> 
>  #include <linux/bitfield.h>
> @@ -5545,6 +5545,16 @@ static struct phy_driver ksphy_driver[] = {
>         .suspend        = genphy_suspend,
>         .resume         = ksz9477_resume,
>         .get_features   = ksz9477_get_features,
> +}, {
> +       .phy_id         = PHY_ID_KSZ9897,
> +       .phy_id_mask    = MICREL_PHY_ID_MASK,
> +       .name           = "Microchip KSZ9897 Switch",
> +       /* PHY_BASIC_FEATURES */
> +       .config_init    = kszphy_config_init,
> +       .config_aneg    = ksz8873mll_config_aneg,
> +       .read_status    = ksz8873mll_read_status,
> +       .suspend        = genphy_suspend,
> +       .resume         = genphy_resume,
>  } };
> 
>  module_phy_driver(ksphy_driver);
> @@ -5570,6 +5580,7 @@ static struct mdio_device_id __maybe_unused
> micrel_tbl[] = {
>         { PHY_ID_LAN8814, MICREL_PHY_ID_MASK },
>         { PHY_ID_LAN8804, MICREL_PHY_ID_MASK },
>         { PHY_ID_LAN8841, MICREL_PHY_ID_MASK },
> +       { PHY_ID_KSZ9897, MICREL_PHY_ID_MASK },
>         { }
>  };
> 
> diff --git a/include/linux/micrel_phy.h b/include/linux/micrel_phy.h
> index 591bf5b5e8dc..81cc16dc2ddf 100644
> --- a/include/linux/micrel_phy.h
> +++ b/include/linux/micrel_phy.h
> @@ -39,6 +39,10 @@
>  #define PHY_ID_KSZ87XX         0x00221550
> 
>  #define        PHY_ID_KSZ9477          0x00221631
> +/* Pseudo ID to specify in compatible field of device tree.
> + * Otherwise the device reports the same ID as KSZ8081 on CPU ports.
> + */
> +#define        PHY_ID_KSZ9897          0x002217ff
> 
>  /* struct phy_device dev_flags definitions */
>  #define MICREL_PHY_50MHZ_CLK   BIT(0)
> --
> 2.34.1

Re: [PATCH net v5 1/4] net: phy: micrel: add Microchip KSZ 9897 Switch PHY support

[Enguerrand de Ribaucourt]

Hello,

On 04/06/2024 22:49, Woojung.Huh@microchip.com wrote:
> Hi Enguerrand,
> 
> Can you help me to understand your setup? I could see you are using
>   - Host CPU : i.MX6ULL
>   - DSA Switch : KSZ9897R (https://www.microchip.com/en-us/product/ksz9897)
>   - Host-to-KSZ interface : RGMII for data path & SPI for control
> Based on this, CPU port is either GMAC6 or GMAC7 (Figure 2-1 of [1])
> 
> I have two questions for you.
> 1. PHY on CPU port
>     Which GMAC (or port number) is connected between Host CPU and KSZ9897R?
>     If CPU port is either GMAC6 or GMAC7, it is just a MAC-to-MAC connection over RGMII.

I'm using port number 6 as the CPU port for KSZ9897R. GMAC6 is directly 
connected to the MAC of i.MX6ULL (driver is i.MX fec). I'm using RMII 
since gigabit is not supported by the i.MX6ULL.

> 2. PHY ID
>     Its PHY ID is different when checking datasheet of KSZ9897 and KSZ8081.
>     PHY ID of Port 1-5 of KSZ9897 is 0x0022-0x1631 per [1]
>     PHY ID of KSZ8081 is 0x0022-0x0156x per [2]
That's true for port 1-5, however, I found out that the phy_id emitted 
by GMAC6 is 0x00221561. It is the same as KSZ8081-revA3 according to the 
datasheet. I also studied all registers at runtime for a reliable 
difference to implement something like ksz8051_ksz8795_match_phy_device 
between GMAC6 and KSZ8081, but none appeared to me. Following 
suggestions by Andrew Lunn, I added this virtual phy_id (0x002217ff) to 
hardcode in the devicetree. I'm happy with this solution.
> 
> Beside patch, you can create a ticket to Microchip site (https://microchipsupport.force.com/s/supportservice)
> if you think it is easier to solve your problem.
I created a joined ticket for tracking (Case number 01457279).
> 

Thank you very much for your time,

Enguerrand de Ribaucourt

> Best regards,
> Woojung
> 
> [1] https://ww1.microchip.com/downloads/aemDocuments/documents/OTH/ProductDocuments/DataSheets/KSZ9897R-Data-Sheet-DS00002330D.pdf
> [2] https://www.microchip.com/en-us/product/ksz8081#document-table
> 
>> -----Original Message-----
>> From: Enguerrand de Ribaucourt <enguerrand.de-
>> ribaucourt@savoirfairelinux.com>
>> Sent: Tuesday, June 4, 2024 5:23 AM
>> To: netdev@vger.kernel.org
>> Cc: andrew@lunn.ch; hkallweit1@gmail.com; linux@armlinux.org.uk; Woojung Huh
>> - C21699 <Woojung.Huh@microchip.com>; UNGLinuxDriver
>> <UNGLinuxDriver@microchip.com>; horms@kernel.org; Tristram Ha - C24268
>> <Tristram.Ha@microchip.com>; Arun Ramadoss - I17769
>> <Arun.Ramadoss@microchip.com>; Enguerrand de Ribaucourt <enguerrand.de-
>> ribaucourt@savoirfairelinux.com>
>> Subject: [PATCH net v5 1/4] net: phy: micrel: add Microchip KSZ 9897 Switch
>> PHY support
>>
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
>> content is safe
>>
>> There is a DSA driver for microchip,ksz9897 which can be controlled
>> through SPI or I2C. This patch adds support for it's CPU ports PHYs to
>> also allow network access to the switch's CPU port.
>>
>> The CPU ports PHYs of the KSZ9897 are not documented in the datasheet.
>> They weirdly use the same PHY ID as the KSZ8081, which is a different
>> PHY and that driver isn't compatible with KSZ9897. Before this patch,
>> the KSZ8081 driver was used for the CPU ports of the KSZ9897 but the
>> link would never come up.
>>
>> A new driver for the KSZ9897 is added, based on the compatible KSZ87XX.
>> I could not test if Gigabit Ethernet works, but the link comes up and
>> can successfully allow packets to be sent and received with DSA tags.
>>
>> To resolve the KSZ8081/KSZ9897 phy_id conflicts, I could not find any
>> stable register to distinguish them. Instead of a match_phy_device() ,
>> I've declared a virtual phy_id with the highest value in Microchip's OUI
>> range.
>>
>> Example usage in the device tree:
>>          compatible = "ethernet-phy-id0022.17ff";
>>
>> A discussion to find better alternatives had been opened with the
>> Microchip team, with no response yet.
>>
>> See https://lore.kernel.org/all/20220207174532.362781-1-enguerrand.de-
>> ribaucourt@savoirfairelinux.com/
>>
>> Fixes: b987e98e50ab ("dsa: add DSA switch driver for Microchip KSZ9477")
>> Signed-off-by: Enguerrand de Ribaucourt <enguerrand.de-
>> ribaucourt@savoirfairelinux.com>
>> ---
>> v5:
>>   - rewrap comments
>>   - restore suspend/resume for KSZ9897
>> v4: https://lore.kernel.org/all/20240531142430.678198-2-enguerrand.de-
>> ribaucourt@savoirfairelinux.com/
>>   - rebase on net/main
>>   - add Fixes tag
>>   - use pseudo phy_id instead of of_tree search
>> v3: https://lore.kernel.org/all/20240530102436.226189-2-enguerrand.de-
>> ribaucourt@savoirfairelinux.com/
>> ---
>>   drivers/net/phy/micrel.c   | 13 ++++++++++++-
>>   include/linux/micrel_phy.h |  4 ++++
>>   2 files changed, 16 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
>> index 8c20cf937530..11e58fc628df 100644
>> --- a/drivers/net/phy/micrel.c
>> +++ b/drivers/net/phy/micrel.c
>> @@ -16,7 +16,7 @@
>>    *                        ksz8081, ksz8091,
>>    *                        ksz8061,
>>    *             Switch : ksz8873, ksz886x
>> - *                      ksz9477, lan8804
>> + *                      ksz9477, ksz9897, lan8804
>>    */
>>
>>   #include <linux/bitfield.h>
>> @@ -5545,6 +5545,16 @@ static struct phy_driver ksphy_driver[] = {
>>          .suspend        = genphy_suspend,
>>          .resume         = ksz9477_resume,
>>          .get_features   = ksz9477_get_features,
>> +}, {
>> +       .phy_id         = PHY_ID_KSZ9897,
>> +       .phy_id_mask    = MICREL_PHY_ID_MASK,
>> +       .name           = "Microchip KSZ9897 Switch",
>> +       /* PHY_BASIC_FEATURES */
>> +       .config_init    = kszphy_config_init,
>> +       .config_aneg    = ksz8873mll_config_aneg,
>> +       .read_status    = ksz8873mll_read_status,
>> +       .suspend        = genphy_suspend,
>> +       .resume         = genphy_resume,
>>   } };
>>
>>   module_phy_driver(ksphy_driver);
>> @@ -5570,6 +5580,7 @@ static struct mdio_device_id __maybe_unused
>> micrel_tbl[] = {
>>          { PHY_ID_LAN8814, MICREL_PHY_ID_MASK },
>>          { PHY_ID_LAN8804, MICREL_PHY_ID_MASK },
>>          { PHY_ID_LAN8841, MICREL_PHY_ID_MASK },
>> +       { PHY_ID_KSZ9897, MICREL_PHY_ID_MASK },
>>          { }
>>   };
>>
>> diff --git a/include/linux/micrel_phy.h b/include/linux/micrel_phy.h
>> index 591bf5b5e8dc..81cc16dc2ddf 100644
>> --- a/include/linux/micrel_phy.h
>> +++ b/include/linux/micrel_phy.h
>> @@ -39,6 +39,10 @@
>>   #define PHY_ID_KSZ87XX         0x00221550
>>
>>   #define        PHY_ID_KSZ9477          0x00221631
>> +/* Pseudo ID to specify in compatible field of device tree.
>> + * Otherwise the device reports the same ID as KSZ8081 on CPU ports.
>> + */
>> +#define        PHY_ID_KSZ9897          0x002217ff
>>
>>   /* struct phy_device dev_flags definitions */
>>   #define MICREL_PHY_50MHZ_CLK   BIT(0)
>> --
>> 2.34.1
> 

RE: [PATCH net v5 1/4] net: phy: micrel: add Microchip KSZ 9897 Switch PHY support

[Woojung.Huh]

Hi Enguerrand,

Thanks for detail and ticket number.
Let me ask our team about your observations and get back to you.

Thanks
Woojung

> -----Original Message-----
> From: Enguerrand de Ribaucourt <enguerrand.de-
> ribaucourt@savoirfairelinux.com>
> Sent: Wednesday, June 5, 2024 4:34 AM
> To: Woojung Huh - C21699 <Woojung.Huh@microchip.com>
> Cc: andrew@lunn.ch; hkallweit1@gmail.com; linux@armlinux.org.uk;
> UNGLinuxDriver <UNGLinuxDriver@microchip.com>; horms@kernel.org; Tristram Ha
> - C24268 <Tristram.Ha@microchip.com>; Arun Ramadoss - I17769
> <Arun.Ramadoss@microchip.com>; netdev@vger.kernel.org
> Subject: Re: [PATCH net v5 1/4] net: phy: micrel: add Microchip KSZ 9897
> Switch PHY support
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> Hello,
> 
> On 04/06/2024 22:49, Woojung.Huh@microchip.com wrote:
> > Hi Enguerrand,
> >
> > Can you help me to understand your setup? I could see you are using
> >   - Host CPU : i.MX6ULL
> >   - DSA Switch : KSZ9897R (https://www.microchip.com/en-us/product/ksz9897)
> >   - Host-to-KSZ interface : RGMII for data path & SPI for control
> > Based on this, CPU port is either GMAC6 or GMAC7 (Figure 2-1 of [1])
> >
> > I have two questions for you.
> > 1. PHY on CPU port
> >     Which GMAC (or port number) is connected between Host CPU and KSZ9897R?
> >     If CPU port is either GMAC6 or GMAC7, it is just a MAC-to-MAC
> connection over RGMII.
> 
> I'm using port number 6 as the CPU port for KSZ9897R. GMAC6 is directly
> connected to the MAC of i.MX6ULL (driver is i.MX fec). I'm using RMII
> since gigabit is not supported by the i.MX6ULL.
> 
> > 2. PHY ID
> >     Its PHY ID is different when checking datasheet of KSZ9897 and KSZ8081.
> >     PHY ID of Port 1-5 of KSZ9897 is 0x0022-0x1631 per [1]
> >     PHY ID of KSZ8081 is 0x0022-0x0156x per [2]
> That's true for port 1-5, however, I found out that the phy_id emitted
> by GMAC6 is 0x00221561. It is the same as KSZ8081-revA3 according to the
> datasheet. I also studied all registers at runtime for a reliable
> difference to implement something like ksz8051_ksz8795_match_phy_device
> between GMAC6 and KSZ8081, but none appeared to me. Following
> suggestions by Andrew Lunn, I added this virtual phy_id (0x002217ff) to
> hardcode in the devicetree. I'm happy with this solution.
> >
> > Beside patch, you can create a ticket to Microchip site
> (https://microchipsupport.force.com/s/supportservice)
> > if you think it is easier to solve your problem.
> I created a joined ticket for tracking (Case number 01457279).
> >
> 
> Thank you very much for your time,
> 
> Enguerrand de Ribaucourt
> 
> > Best regards,
> > Woojung
> >
> > [1]
> https://ww1.microchip.com/downloads/aemDocuments/documents/OTH/ProductDocume
> nts/DataSheets/KSZ9897R-Data-Sheet-DS00002330D.pdf
> > [2] https://www.microchip.com/en-us/product/ksz8081#document-table
> >
> >> -----Original Message-----
> >> From: Enguerrand de Ribaucourt <enguerrand.de-
> >> ribaucourt@savoirfairelinux.com>
> >> Sent: Tuesday, June 4, 2024 5:23 AM
> >> To: netdev@vger.kernel.org
> >> Cc: andrew@lunn.ch; hkallweit1@gmail.com; linux@armlinux.org.uk; Woojung
> Huh
> >> - C21699 <Woojung.Huh@microchip.com>; UNGLinuxDriver
> >> <UNGLinuxDriver@microchip.com>; horms@kernel.org; Tristram Ha - C24268
> >> <Tristram.Ha@microchip.com>; Arun Ramadoss - I17769
> >> <Arun.Ramadoss@microchip.com>; Enguerrand de Ribaucourt <enguerrand.de-
> >> ribaucourt@savoirfairelinux.com>
> >> Subject: [PATCH net v5 1/4] net: phy: micrel: add Microchip KSZ 9897
> Switch
> >> PHY support
> >>
> >> EXTERNAL EMAIL: Do not click links or open attachments unless you know
> the
> >> content is safe
> >>
> >> There is a DSA driver for microchip,ksz9897 which can be controlled
> >> through SPI or I2C. This patch adds support for it's CPU ports PHYs to
> >> also allow network access to the switch's CPU port.
> >>
> >> The CPU ports PHYs of the KSZ9897 are not documented in the datasheet.
> >> They weirdly use the same PHY ID as the KSZ8081, which is a different
> >> PHY and that driver isn't compatible with KSZ9897. Before this patch,
> >> the KSZ8081 driver was used for the CPU ports of the KSZ9897 but the
> >> link would never come up.
> >>
> >> A new driver for the KSZ9897 is added, based on the compatible KSZ87XX.
> >> I could not test if Gigabit Ethernet works, but the link comes up and
> >> can successfully allow packets to be sent and received with DSA tags.
> >>
> >> To resolve the KSZ8081/KSZ9897 phy_id conflicts, I could not find any
> >> stable register to distinguish them. Instead of a match_phy_device() ,
> >> I've declared a virtual phy_id with the highest value in Microchip's OUI
> >> range.
> >>
> >> Example usage in the device tree:
> >>          compatible = "ethernet-phy-id0022.17ff";
> >>
> >> A discussion to find better alternatives had been opened with the
> >> Microchip team, with no response yet.
> >>
> >> See https://lore.kernel.org/all/20220207174532.362781-1-enguerrand.de-
> >> ribaucourt@savoirfairelinux.com/
> >>
> >> Fixes: b987e98e50ab ("dsa: add DSA switch driver for Microchip KSZ9477")
> >> Signed-off-by: Enguerrand de Ribaucourt <enguerrand.de-
> >> ribaucourt@savoirfairelinux.com>
> >> ---
> >> v5:
> >>   - rewrap comments
> >>   - restore suspend/resume for KSZ9897
> >> v4: https://lore.kernel.org/all/20240531142430.678198-2-enguerrand.de-
> >> ribaucourt@savoirfairelinux.com/
> >>   - rebase on net/main
> >>   - add Fixes tag
> >>   - use pseudo phy_id instead of of_tree search
> >> v3: https://lore.kernel.org/all/20240530102436.226189-2-enguerrand.de-
> >> ribaucourt@savoirfairelinux.com/
> >> ---
> >>   drivers/net/phy/micrel.c   | 13 ++++++++++++-
> >>   include/linux/micrel_phy.h |  4 ++++
> >>   2 files changed, 16 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
> >> index 8c20cf937530..11e58fc628df 100644
> >> --- a/drivers/net/phy/micrel.c
> >> +++ b/drivers/net/phy/micrel.c
> >> @@ -16,7 +16,7 @@
> >>    *                        ksz8081, ksz8091,
> >>    *                        ksz8061,
> >>    *             Switch : ksz8873, ksz886x
> >> - *                      ksz9477, lan8804
> >> + *                      ksz9477, ksz9897, lan8804
> >>    */
> >>
> >>   #include <linux/bitfield.h>
> >> @@ -5545,6 +5545,16 @@ static struct phy_driver ksphy_driver[] = {
> >>          .suspend        = genphy_suspend,
> >>          .resume         = ksz9477_resume,
> >>          .get_features   = ksz9477_get_features,
> >> +}, {
> >> +       .phy_id         = PHY_ID_KSZ9897,
> >> +       .phy_id_mask    = MICREL_PHY_ID_MASK,
> >> +       .name           = "Microchip KSZ9897 Switch",
> >> +       /* PHY_BASIC_FEATURES */
> >> +       .config_init    = kszphy_config_init,
> >> +       .config_aneg    = ksz8873mll_config_aneg,
> >> +       .read_status    = ksz8873mll_read_status,
> >> +       .suspend        = genphy_suspend,
> >> +       .resume         = genphy_resume,
> >>   } };
> >>
> >>   module_phy_driver(ksphy_driver);
> >> @@ -5570,6 +5580,7 @@ static struct mdio_device_id __maybe_unused
> >> micrel_tbl[] = {
> >>          { PHY_ID_LAN8814, MICREL_PHY_ID_MASK },
> >>          { PHY_ID_LAN8804, MICREL_PHY_ID_MASK },
> >>          { PHY_ID_LAN8841, MICREL_PHY_ID_MASK },
> >> +       { PHY_ID_KSZ9897, MICREL_PHY_ID_MASK },
> >>          { }
> >>   };
> >>
> >> diff --git a/include/linux/micrel_phy.h b/include/linux/micrel_phy.h
> >> index 591bf5b5e8dc..81cc16dc2ddf 100644
> >> --- a/include/linux/micrel_phy.h
> >> +++ b/include/linux/micrel_phy.h
> >> @@ -39,6 +39,10 @@
> >>   #define PHY_ID_KSZ87XX         0x00221550
> >>
> >>   #define        PHY_ID_KSZ9477          0x00221631
> >> +/* Pseudo ID to specify in compatible field of device tree.
> >> + * Otherwise the device reports the same ID as KSZ8081 on CPU ports.
> >> + */
> >> +#define        PHY_ID_KSZ9897          0x002217ff
> >>
> >>   /* struct phy_device dev_flags definitions */
> >>   #define MICREL_PHY_50MHZ_CLK   BIT(0)
> >> --
> >> 2.34.1
> >

RE: [PATCH net v5 1/4] net: phy: micrel: add Microchip KSZ 9897 Switch PHY support

[Woojung.Huh]

Hi Enguerrand,

We still can't reproduce what you observed with KSZ9897.

Just to be sure, you accessed PHY register of Port 6 which is GMAC6.
It is directly connected to MAC of i.MX6ULL over RMII.
I guess the PHY ID access is register 0x6104-0x6105 of KSZ9897.
And, return value of PHY ID is 0x0022-0x1561.

Correct understanding?

Thanks.
Woojung

> -----Original Message-----
> From: Enguerrand de Ribaucourt <enguerrand.de-
> ribaucourt@savoirfairelinux.com>
> Sent: Wednesday, June 5, 2024 4:34 AM
> To: Woojung Huh - C21699 <Woojung.Huh@microchip.com>
> Cc: andrew@lunn.ch; hkallweit1@gmail.com; linux@armlinux.org.uk;
> UNGLinuxDriver <UNGLinuxDriver@microchip.com>; horms@kernel.org; Tristram Ha
> - C24268 <Tristram.Ha@microchip.com>; Arun Ramadoss - I17769
> <Arun.Ramadoss@microchip.com>; netdev@vger.kernel.org
> Subject: Re: [PATCH net v5 1/4] net: phy: micrel: add Microchip KSZ 9897
> Switch PHY support
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> Hello,
> 
> On 04/06/2024 22:49, Woojung.Huh@microchip.com wrote:
> > Hi Enguerrand,
> >
> > Can you help me to understand your setup? I could see you are using
> >   - Host CPU : i.MX6ULL
> >   - DSA Switch : KSZ9897R (https://www.microchip.com/en-us/product/ksz9897)
> >   - Host-to-KSZ interface : RGMII for data path & SPI for control
> > Based on this, CPU port is either GMAC6 or GMAC7 (Figure 2-1 of [1])
> >
> > I have two questions for you.
> > 1. PHY on CPU port
> >     Which GMAC (or port number) is connected between Host CPU and KSZ9897R?
> >     If CPU port is either GMAC6 or GMAC7, it is just a MAC-to-MAC
> connection over RGMII.
> 
> I'm using port number 6 as the CPU port for KSZ9897R. GMAC6 is directly
> connected to the MAC of i.MX6ULL (driver is i.MX fec). I'm using RMII
> since gigabit is not supported by the i.MX6ULL.
> 
> > 2. PHY ID
> >     Its PHY ID is different when checking datasheet of KSZ9897 and KSZ8081.
> >     PHY ID of Port 1-5 of KSZ9897 is 0x0022-0x1631 per [1]
> >     PHY ID of KSZ8081 is 0x0022-0x0156x per [2]
> That's true for port 1-5, however, I found out that the phy_id emitted
> by GMAC6 is 0x00221561. It is the same as KSZ8081-revA3 according to the
> datasheet. I also studied all registers at runtime for a reliable
> difference to implement something like ksz8051_ksz8795_match_phy_device
> between GMAC6 and KSZ8081, but none appeared to me. Following
> suggestions by Andrew Lunn, I added this virtual phy_id (0x002217ff) to
> hardcode in the devicetree. I'm happy with this solution.
> >
> > Beside patch, you can create a ticket to Microchip site
> (https://microchipsupport.force.com/s/supportservice)
> > if you think it is easier to solve your problem.
> I created a joined ticket for tracking (Case number 01457279).
> >
> 
> Thank you very much for your time,
> 
> Enguerrand de Ribaucourt
> 
> > Best regards,
> > Woojung
> >
> > [1]
> https://ww1.microchip.com/downloads/aemDocuments/documents/OTH/ProductDocume
> nts/DataSheets/KSZ9897R-Data-Sheet-DS00002330D.pdf
> > [2] https://www.microchip.com/en-us/product/ksz8081#document-table
> >
> >> -----Original Message-----
> >> From: Enguerrand de Ribaucourt <enguerrand.de-
> >> ribaucourt@savoirfairelinux.com>
> >> Sent: Tuesday, June 4, 2024 5:23 AM
> >> To: netdev@vger.kernel.org
> >> Cc: andrew@lunn.ch; hkallweit1@gmail.com; linux@armlinux.org.uk; Woojung
> Huh
> >> - C21699 <Woojung.Huh@microchip.com>; UNGLinuxDriver
> >> <UNGLinuxDriver@microchip.com>; horms@kernel.org; Tristram Ha - C24268
> >> <Tristram.Ha@microchip.com>; Arun Ramadoss - I17769
> >> <Arun.Ramadoss@microchip.com>; Enguerrand de Ribaucourt <enguerrand.de-
> >> ribaucourt@savoirfairelinux.com>
> >> Subject: [PATCH net v5 1/4] net: phy: micrel: add Microchip KSZ 9897
> Switch
> >> PHY support
> >>
> >> EXTERNAL EMAIL: Do not click links or open attachments unless you know
> the
> >> content is safe
> >>
> >> There is a DSA driver for microchip,ksz9897 which can be controlled
> >> through SPI or I2C. This patch adds support for it's CPU ports PHYs to
> >> also allow network access to the switch's CPU port.
> >>
> >> The CPU ports PHYs of the KSZ9897 are not documented in the datasheet.
> >> They weirdly use the same PHY ID as the KSZ8081, which is a different
> >> PHY and that driver isn't compatible with KSZ9897. Before this patch,
> >> the KSZ8081 driver was used for the CPU ports of the KSZ9897 but the
> >> link would never come up.
> >>
> >> A new driver for the KSZ9897 is added, based on the compatible KSZ87XX.
> >> I could not test if Gigabit Ethernet works, but the link comes up and
> >> can successfully allow packets to be sent and received with DSA tags.
> >>
> >> To resolve the KSZ8081/KSZ9897 phy_id conflicts, I could not find any
> >> stable register to distinguish them. Instead of a match_phy_device() ,
> >> I've declared a virtual phy_id with the highest value in Microchip's OUI
> >> range.
> >>
> >> Example usage in the device tree:
> >>          compatible = "ethernet-phy-id0022.17ff";
> >>
> >> A discussion to find better alternatives had been opened with the
> >> Microchip team, with no response yet.
> >>
> >> See https://lore.kernel.org/all/20220207174532.362781-1-enguerrand.de-
> >> ribaucourt@savoirfairelinux.com/
> >>
> >> Fixes: b987e98e50ab ("dsa: add DSA switch driver for Microchip KSZ9477")
> >> Signed-off-by: Enguerrand de Ribaucourt <enguerrand.de-
> >> ribaucourt@savoirfairelinux.com>
> >> ---
> >> v5:
> >>   - rewrap comments
> >>   - restore suspend/resume for KSZ9897
> >> v4: https://lore.kernel.org/all/20240531142430.678198-2-enguerrand.de-
> >> ribaucourt@savoirfairelinux.com/
> >>   - rebase on net/main
> >>   - add Fixes tag
> >>   - use pseudo phy_id instead of of_tree search
> >> v3: https://lore.kernel.org/all/20240530102436.226189-2-enguerrand.de-
> >> ribaucourt@savoirfairelinux.com/
> >> ---
> >>   drivers/net/phy/micrel.c   | 13 ++++++++++++-
> >>   include/linux/micrel_phy.h |  4 ++++
> >>   2 files changed, 16 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
> >> index 8c20cf937530..11e58fc628df 100644
> >> --- a/drivers/net/phy/micrel.c
> >> +++ b/drivers/net/phy/micrel.c
> >> @@ -16,7 +16,7 @@
> >>    *                        ksz8081, ksz8091,
> >>    *                        ksz8061,
> >>    *             Switch : ksz8873, ksz886x
> >> - *                      ksz9477, lan8804
> >> + *                      ksz9477, ksz9897, lan8804
> >>    */
> >>
> >>   #include <linux/bitfield.h>
> >> @@ -5545,6 +5545,16 @@ static struct phy_driver ksphy_driver[] = {
> >>          .suspend        = genphy_suspend,
> >>          .resume         = ksz9477_resume,
> >>          .get_features   = ksz9477_get_features,
> >> +}, {
> >> +       .phy_id         = PHY_ID_KSZ9897,
> >> +       .phy_id_mask    = MICREL_PHY_ID_MASK,
> >> +       .name           = "Microchip KSZ9897 Switch",
> >> +       /* PHY_BASIC_FEATURES */
> >> +       .config_init    = kszphy_config_init,
> >> +       .config_aneg    = ksz8873mll_config_aneg,
> >> +       .read_status    = ksz8873mll_read_status,
> >> +       .suspend        = genphy_suspend,
> >> +       .resume         = genphy_resume,
> >>   } };
> >>
> >>   module_phy_driver(ksphy_driver);
> >> @@ -5570,6 +5580,7 @@ static struct mdio_device_id __maybe_unused
> >> micrel_tbl[] = {
> >>          { PHY_ID_LAN8814, MICREL_PHY_ID_MASK },
> >>          { PHY_ID_LAN8804, MICREL_PHY_ID_MASK },
> >>          { PHY_ID_LAN8841, MICREL_PHY_ID_MASK },
> >> +       { PHY_ID_KSZ9897, MICREL_PHY_ID_MASK },
> >>          { }
> >>   };
> >>
> >> diff --git a/include/linux/micrel_phy.h b/include/linux/micrel_phy.h
> >> index 591bf5b5e8dc..81cc16dc2ddf 100644
> >> --- a/include/linux/micrel_phy.h
> >> +++ b/include/linux/micrel_phy.h
> >> @@ -39,6 +39,10 @@
> >>   #define PHY_ID_KSZ87XX         0x00221550
> >>
> >>   #define        PHY_ID_KSZ9477          0x00221631
> >> +/* Pseudo ID to specify in compatible field of device tree.
> >> + * Otherwise the device reports the same ID as KSZ8081 on CPU ports.
> >> + */
> >> +#define        PHY_ID_KSZ9897          0x002217ff
> >>
> >>   /* struct phy_device dev_flags definitions */
> >>   #define MICREL_PHY_50MHZ_CLK   BIT(0)
> >> --
> >> 2.34.1
> >

Re: [PATCH net v5 1/4] net: phy: micrel: add Microchip KSZ 9897 Switch PHY support

[Enguerrand de Ribaucourt]

Hello,

The exact hardware is a Phycore-i.MX6ULL. ENET2 is directly the 
i.MX6ULL's FEC that connects to port 6 of the KSZ9897R (GMAC6) in RMII:

  - X_ENET2_TX_CLK -- TX_CLK6
  - X_ENET2_TX_EN  -- TX_CTL6
  - X_ENET2_TX_D1  -- TXD6_1
  - X_ENET2_TX_D0  -- TXD6_0
  - X_ENET2_RX_EN  -- RX_CTL6
  - X_ENET2_RX_ER  -- RX_ER6
  - X_ENET2_RX_D1  -- RXD6_1
  - X_ENET2_RX_D0  -- RXD6_0

The DSA control is using SPI, but not involved in reading the phy_id in 
my case.

This is materialized in my device tree:

```c
ethernet@20b4000 {
	compatible = "fsl,imx6ul-fec\0fsl,imx6q-fec";
	...
	phy-mode = "rmii";
	phy-handle = <0x15>;
	fixed-link {
		speed = <0x64>;
		full-duplex;
	};
};

// MDIO bus is only defined on eth1 but shared with eth2
ethernet@2188000 {
         ...
	mdio {
                 ...
		ksz9897port5@1 {
			compatible = "ethernet-phy-ieee802.3-c22";
			...
			clock-names = "rmii-ref";
			phandle = <0x15>;
		};
};

spi@2010000 {
         ...
	ksz9897@0 {
		compatible = "microchip,ksz9897";
		...
		ports {
			...
			// GMAC6
			port@5 {
				reg = <0x05>;
				label = "cpu";
				ethernet = <0x0c>;
				phy-mode = "rmii";
				rx-internal-delay-ps = <0x5dc>;
				fixed-link {
					speed = <0x64>;
					full-duplex;
				};
			};
		};
	};
};
```

Before I implemented the pseudo phy_id, it was read in the generic IEEE 
clause 22 PHY registers, through the compatible 
"ethernet-phy-ieee802.3-c22". That would be implemented in 
get_phy_c22_id() in MII_PHYSID1/2 registers at 0x2 of the MDIO device.

It is not read through SPI registers 0x6104-0x6105 which are not defined 
in the datasheet for port 6/7 (section 5.2.2.3):
	Address: 0xN104, Size: 16 bits, Port N: 1-5

Do you have other suggestions to read the phy_id?

Thanks for your support,
Enguerrand de Ribaucourt


On 07/06/2024 00:57, Woojung.Huh@microchip.com wrote:
> Hi Enguerrand,
> 
> We still can't reproduce what you observed with KSZ9897.
> 
> Just to be sure, you accessed PHY register of Port 6 which is GMAC6.
> It is directly connected to MAC of i.MX6ULL over RMII.
> I guess the PHY ID access is register 0x6104-0x6105 of KSZ9897.
> And, return value of PHY ID is 0x0022-0x1561.
> 
> Correct understanding?
> > Thanks.
> Woojung
> 
>> -----Original Message-----
>> From: Enguerrand de Ribaucourt <enguerrand.de-
>> ribaucourt@savoirfairelinux.com>
>> Sent: Wednesday, June 5, 2024 4:34 AM
>> To: Woojung Huh - C21699 <Woojung.Huh@microchip.com>
>> Cc: andrew@lunn.ch; hkallweit1@gmail.com; linux@armlinux.org.uk;
>> UNGLinuxDriver <UNGLinuxDriver@microchip.com>; horms@kernel.org; Tristram Ha
>> - C24268 <Tristram.Ha@microchip.com>; Arun Ramadoss - I17769
>> <Arun.Ramadoss@microchip.com>; netdev@vger.kernel.org
>> Subject: Re: [PATCH net v5 1/4] net: phy: micrel: add Microchip KSZ 9897
>> Switch PHY support
>>
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
>> content is safe
>>
>> Hello,
>>
>> On 04/06/2024 22:49, Woojung.Huh@microchip.com wrote:
>>> Hi Enguerrand,
>>>
>>> Can you help me to understand your setup? I could see you are using
>>>    - Host CPU : i.MX6ULL
>>>    - DSA Switch : KSZ9897R (https://www.microchip.com/en-us/product/ksz9897)
>>>    - Host-to-KSZ interface : RGMII for data path & SPI for control
>>> Based on this, CPU port is either GMAC6 or GMAC7 (Figure 2-1 of [1])
>>>
>>> I have two questions for you.
>>> 1. PHY on CPU port
>>>      Which GMAC (or port number) is connected between Host CPU and KSZ9897R?
>>>      If CPU port is either GMAC6 or GMAC7, it is just a MAC-to-MAC
>> connection over RGMII.
>>
>> I'm using port number 6 as the CPU port for KSZ9897R. GMAC6 is directly
>> connected to the MAC of i.MX6ULL (driver is i.MX fec). I'm using RMII
>> since gigabit is not supported by the i.MX6ULL.
>>
>>> 2. PHY ID
>>>      Its PHY ID is different when checking datasheet of KSZ9897 and KSZ8081.
>>>      PHY ID of Port 1-5 of KSZ9897 is 0x0022-0x1631 per [1]
>>>      PHY ID of KSZ8081 is 0x0022-0x0156x per [2]
>> That's true for port 1-5, however, I found out that the phy_id emitted
>> by GMAC6 is 0x00221561. It is the same as KSZ8081-revA3 according to the
>> datasheet. I also studied all registers at runtime for a reliable
>> difference to implement something like ksz8051_ksz8795_match_phy_device
>> between GMAC6 and KSZ8081, but none appeared to me. Following
>> suggestions by Andrew Lunn, I added this virtual phy_id (0x002217ff) to
>> hardcode in the devicetree. I'm happy with this solution.
>>>
>>> Beside patch, you can create a ticket to Microchip site
>> (https://microchipsupport.force.com/s/supportservice)
>>> if you think it is easier to solve your problem.
>> I created a joined ticket for tracking (Case number 01457279).
>>>
>>
>> Thank you very much for your time,
>>
>> Enguerrand de Ribaucourt
>>
>>> Best regards,
>>> Woojung
>>>
>>> [1]
>> https://ww1.microchip.com/downloads/aemDocuments/documents/OTH/ProductDocume
>> nts/DataSheets/KSZ9897R-Data-Sheet-DS00002330D.pdf
>>> [2] https://www.microchip.com/en-us/product/ksz8081#document-table
>>>
>>>> -----Original Message-----
>>>> From: Enguerrand de Ribaucourt <enguerrand.de-
>>>> ribaucourt@savoirfairelinux.com>
>>>> Sent: Tuesday, June 4, 2024 5:23 AM
>>>> To: netdev@vger.kernel.org
>>>> Cc: andrew@lunn.ch; hkallweit1@gmail.com; linux@armlinux.org.uk; Woojung
>> Huh
>>>> - C21699 <Woojung.Huh@microchip.com>; UNGLinuxDriver
>>>> <UNGLinuxDriver@microchip.com>; horms@kernel.org; Tristram Ha - C24268
>>>> <Tristram.Ha@microchip.com>; Arun Ramadoss - I17769
>>>> <Arun.Ramadoss@microchip.com>; Enguerrand de Ribaucourt <enguerrand.de-
>>>> ribaucourt@savoirfairelinux.com>
>>>> Subject: [PATCH net v5 1/4] net: phy: micrel: add Microchip KSZ 9897
>> Switch
>>>> PHY support
>>>>
>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know
>> the
>>>> content is safe
>>>>
>>>> There is a DSA driver for microchip,ksz9897 which can be controlled
>>>> through SPI or I2C. This patch adds support for it's CPU ports PHYs to
>>>> also allow network access to the switch's CPU port.
>>>>
>>>> The CPU ports PHYs of the KSZ9897 are not documented in the datasheet.
>>>> They weirdly use the same PHY ID as the KSZ8081, which is a different
>>>> PHY and that driver isn't compatible with KSZ9897. Before this patch,
>>>> the KSZ8081 driver was used for the CPU ports of the KSZ9897 but the
>>>> link would never come up.
>>>>
>>>> A new driver for the KSZ9897 is added, based on the compatible KSZ87XX.
>>>> I could not test if Gigabit Ethernet works, but the link comes up and
>>>> can successfully allow packets to be sent and received with DSA tags.
>>>>
>>>> To resolve the KSZ8081/KSZ9897 phy_id conflicts, I could not find any
>>>> stable register to distinguish them. Instead of a match_phy_device() ,
>>>> I've declared a virtual phy_id with the highest value in Microchip's OUI
>>>> range.
>>>>
>>>> Example usage in the device tree:
>>>>           compatible = "ethernet-phy-id0022.17ff";
>>>>
>>>> A discussion to find better alternatives had been opened with the
>>>> Microchip team, with no response yet.
>>>>
>>>> See https://lore.kernel.org/all/20220207174532.362781-1-enguerrand.de-
>>>> ribaucourt@savoirfairelinux.com/
>>>>
>>>> Fixes: b987e98e50ab ("dsa: add DSA switch driver for Microchip KSZ9477")
>>>> Signed-off-by: Enguerrand de Ribaucourt <enguerrand.de-
>>>> ribaucourt@savoirfairelinux.com>
>>>> ---
>>>> v5:
>>>>    - rewrap comments
>>>>    - restore suspend/resume for KSZ9897
>>>> v4: https://lore.kernel.org/all/20240531142430.678198-2-enguerrand.de-
>>>> ribaucourt@savoirfairelinux.com/
>>>>    - rebase on net/main
>>>>    - add Fixes tag
>>>>    - use pseudo phy_id instead of of_tree search
>>>> v3: https://lore.kernel.org/all/20240530102436.226189-2-enguerrand.de-
>>>> ribaucourt@savoirfairelinux.com/
>>>> ---
>>>>    drivers/net/phy/micrel.c   | 13 ++++++++++++-
>>>>    include/linux/micrel_phy.h |  4 ++++
>>>>    2 files changed, 16 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
>>>> index 8c20cf937530..11e58fc628df 100644
>>>> --- a/drivers/net/phy/micrel.c
>>>> +++ b/drivers/net/phy/micrel.c
>>>> @@ -16,7 +16,7 @@
>>>>     *                        ksz8081, ksz8091,
>>>>     *                        ksz8061,
>>>>     *             Switch : ksz8873, ksz886x
>>>> - *                      ksz9477, lan8804
>>>> + *                      ksz9477, ksz9897, lan8804
>>>>     */
>>>>
>>>>    #include <linux/bitfield.h>
>>>> @@ -5545,6 +5545,16 @@ static struct phy_driver ksphy_driver[] = {
>>>>           .suspend        = genphy_suspend,
>>>>           .resume         = ksz9477_resume,
>>>>           .get_features   = ksz9477_get_features,
>>>> +}, {
>>>> +       .phy_id         = PHY_ID_KSZ9897,
>>>> +       .phy_id_mask    = MICREL_PHY_ID_MASK,
>>>> +       .name           = "Microchip KSZ9897 Switch",
>>>> +       /* PHY_BASIC_FEATURES */
>>>> +       .config_init    = kszphy_config_init,
>>>> +       .config_aneg    = ksz8873mll_config_aneg,
>>>> +       .read_status    = ksz8873mll_read_status,
>>>> +       .suspend        = genphy_suspend,
>>>> +       .resume         = genphy_resume,
>>>>    } };
>>>>
>>>>    module_phy_driver(ksphy_driver);
>>>> @@ -5570,6 +5580,7 @@ static struct mdio_device_id __maybe_unused
>>>> micrel_tbl[] = {
>>>>           { PHY_ID_LAN8814, MICREL_PHY_ID_MASK },
>>>>           { PHY_ID_LAN8804, MICREL_PHY_ID_MASK },
>>>>           { PHY_ID_LAN8841, MICREL_PHY_ID_MASK },
>>>> +       { PHY_ID_KSZ9897, MICREL_PHY_ID_MASK },
>>>>           { }
>>>>    };
>>>>
>>>> diff --git a/include/linux/micrel_phy.h b/include/linux/micrel_phy.h
>>>> index 591bf5b5e8dc..81cc16dc2ddf 100644
>>>> --- a/include/linux/micrel_phy.h
>>>> +++ b/include/linux/micrel_phy.h
>>>> @@ -39,6 +39,10 @@
>>>>    #define PHY_ID_KSZ87XX         0x00221550
>>>>
>>>>    #define        PHY_ID_KSZ9477          0x00221631
>>>> +/* Pseudo ID to specify in compatible field of device tree.
>>>> + * Otherwise the device reports the same ID as KSZ8081 on CPU ports.
>>>> + */
>>>> +#define        PHY_ID_KSZ9897          0x002217ff
>>>>
>>>>    /* struct phy_device dev_flags definitions */
>>>>    #define MICREL_PHY_50MHZ_CLK   BIT(0)
>>>> --
>>>> 2.34.1
>>>

Re: [PATCH net v5 1/4] net: phy: micrel: add Microchip KSZ 9897 Switch PHY support

[Enguerrand de Ribaucourt]

On 07/06/2024 10:11, Enguerrand de Ribaucourt wrote:
> 
> Hello,
> 
> The exact hardware is a Phycore-i.MX6ULL. ENET2 is directly the 
> i.MX6ULL's FEC that connects to port 6 of the KSZ9897R (GMAC6) in RMII:
> 
>   - X_ENET2_TX_CLK -- TX_CLK6
>   - X_ENET2_TX_EN  -- TX_CTL6
>   - X_ENET2_TX_D1  -- TXD6_1
>   - X_ENET2_TX_D0  -- TXD6_0
>   - X_ENET2_RX_EN  -- RX_CTL6
>   - X_ENET2_RX_ER  -- RX_ER6
>   - X_ENET2_RX_D1  -- RXD6_1
>   - X_ENET2_RX_D0  -- RXD6_0
> 
> The DSA control is using SPI, but not involved in reading the phy_id in 
> my case.
> 
> This is materialized in my device tree:
> 
> ```c
> ethernet@20b4000 {
>      compatible = "fsl,imx6ul-fec\0fsl,imx6q-fec";
>      ...
>      phy-mode = "rmii";
>      phy-handle = <0x15>;
>      fixed-link {
>          speed = <0x64>;
>          full-duplex;
>      };
> };
> 
> // MDIO bus is only defined on eth1 but shared with eth2
> ethernet@2188000 {
>          ...
>      mdio {
>                  ...
>          ksz9897port5@1 {
>              compatible = "ethernet-phy-ieee802.3-c22";
>              ...
>              clock-names = "rmii-ref";
>              phandle = <0x15>;
>          };
> };
> 
> spi@2010000 {
>          ...
>      ksz9897@0 {
>          compatible = "microchip,ksz9897";
>          ...
>          ports {
>              ...
>              // GMAC6
>              port@5 {
>                  reg = <0x05>;
>                  label = "cpu";
>                  ethernet = <0x0c>;
>                  phy-mode = "rmii";
>                  rx-internal-delay-ps = <0x5dc>;
>                  fixed-link {
>                      speed = <0x64>;
>                      full-duplex;
>                  };
>              };
>          };
>      };
> };
> ```
> 

I also checked using `phy-mode = "internal";` on both ends, but ended up 
with error "Unable to connect to phy".

> Before I implemented the pseudo phy_id, it was read in the generic IEEE 
> clause 22 PHY registers, through the compatible 
> "ethernet-phy-ieee802.3-c22". That would be implemented in 
> get_phy_c22_id() in MII_PHYSID1/2 registers at 0x2 of the MDIO device.
> 
> It is not read through SPI registers 0x6104-0x6105 which are not defined 
> in the datasheet for port 6/7 (section 5.2.2.3):
>      Address: 0xN104, Size: 16 bits, Port N: 1-5
> 
> Do you have other suggestions to read the phy_id?
> 
> Thanks for your support,
> Enguerrand de Ribaucourt
> 
> 
> On 07/06/2024 00:57, Woojung.Huh@microchip.com wrote:
>> Hi Enguerrand,
>>
>> We still can't reproduce what you observed with KSZ9897.
>>
>> Just to be sure, you accessed PHY register of Port 6 which is GMAC6.
>> It is directly connected to MAC of i.MX6ULL over RMII.
>> I guess the PHY ID access is register 0x6104-0x6105 of KSZ9897.
>> And, return value of PHY ID is 0x0022-0x1561.
>>
>> Correct understanding?
>> > Thanks.
>> Woojung
>>
>>> -----Original Message-----
>>> From: Enguerrand de Ribaucourt <enguerrand.de-
>>> ribaucourt@savoirfairelinux.com>
>>> Sent: Wednesday, June 5, 2024 4:34 AM
>>> To: Woojung Huh - C21699 <Woojung.Huh@microchip.com>
>>> Cc: andrew@lunn.ch; hkallweit1@gmail.com; linux@armlinux.org.uk;
>>> UNGLinuxDriver <UNGLinuxDriver@microchip.com>; horms@kernel.org; 
>>> Tristram Ha
>>> - C24268 <Tristram.Ha@microchip.com>; Arun Ramadoss - I17769
>>> <Arun.Ramadoss@microchip.com>; netdev@vger.kernel.org
>>> Subject: Re: [PATCH net v5 1/4] net: phy: micrel: add Microchip KSZ 9897
>>> Switch PHY support
>>>
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you 
>>> know the
>>> content is safe
>>>
>>> Hello,
>>>
>>> On 04/06/2024 22:49, Woojung.Huh@microchip.com wrote:
>>>> Hi Enguerrand,
>>>>
>>>> Can you help me to understand your setup? I could see you are using
>>>>    - Host CPU : i.MX6ULL
>>>>    - DSA Switch : KSZ9897R 
>>>> (https://www.microchip.com/en-us/product/ksz9897)
>>>>    - Host-to-KSZ interface : RGMII for data path & SPI for control
>>>> Based on this, CPU port is either GMAC6 or GMAC7 (Figure 2-1 of [1])
>>>>
>>>> I have two questions for you.
>>>> 1. PHY on CPU port
>>>>      Which GMAC (or port number) is connected between Host CPU and 
>>>> KSZ9897R?
>>>>      If CPU port is either GMAC6 or GMAC7, it is just a MAC-to-MAC
>>> connection over RGMII.
>>>
>>> I'm using port number 6 as the CPU port for KSZ9897R. GMAC6 is directly
>>> connected to the MAC of i.MX6ULL (driver is i.MX fec). I'm using RMII
>>> since gigabit is not supported by the i.MX6ULL.
>>>
>>>> 2. PHY ID
>>>>      Its PHY ID is different when checking datasheet of KSZ9897 and 
>>>> KSZ8081.
>>>>      PHY ID of Port 1-5 of KSZ9897 is 0x0022-0x1631 per [1]
>>>>      PHY ID of KSZ8081 is 0x0022-0x0156x per [2]
>>> That's true for port 1-5, however, I found out that the phy_id emitted
>>> by GMAC6 is 0x00221561. It is the same as KSZ8081-revA3 according to the
>>> datasheet. I also studied all registers at runtime for a reliable
>>> difference to implement something like ksz8051_ksz8795_match_phy_device
>>> between GMAC6 and KSZ8081, but none appeared to me. Following
>>> suggestions by Andrew Lunn, I added this virtual phy_id (0x002217ff) to
>>> hardcode in the devicetree. I'm happy with this solution.
>>>>
>>>> Beside patch, you can create a ticket to Microchip site
>>> (https://microchipsupport.force.com/s/supportservice)
>>>> if you think it is easier to solve your problem.
>>> I created a joined ticket for tracking (Case number 01457279).
>>>>
>>>
>>> Thank you very much for your time,
>>>
>>> Enguerrand de Ribaucourt
>>>
>>>> Best regards,
>>>> Woojung
>>>>
>>>> [1]
>>> https://ww1.microchip.com/downloads/aemDocuments/documents/OTH/ProductDocume
>>> nts/DataSheets/KSZ9897R-Data-Sheet-DS00002330D.pdf
>>>> [2] https://www.microchip.com/en-us/product/ksz8081#document-table
>>>>
>>>>> -----Original Message-----
>>>>> From: Enguerrand de Ribaucourt <enguerrand.de-
>>>>> ribaucourt@savoirfairelinux.com>
>>>>> Sent: Tuesday, June 4, 2024 5:23 AM
>>>>> To: netdev@vger.kernel.org
>>>>> Cc: andrew@lunn.ch; hkallweit1@gmail.com; linux@armlinux.org.uk; 
>>>>> Woojung
>>> Huh
>>>>> - C21699 <Woojung.Huh@microchip.com>; UNGLinuxDriver
>>>>> <UNGLinuxDriver@microchip.com>; horms@kernel.org; Tristram Ha - C24268
>>>>> <Tristram.Ha@microchip.com>; Arun Ramadoss - I17769
>>>>> <Arun.Ramadoss@microchip.com>; Enguerrand de Ribaucourt 
>>>>> <enguerrand.de-
>>>>> ribaucourt@savoirfairelinux.com>
>>>>> Subject: [PATCH net v5 1/4] net: phy: micrel: add Microchip KSZ 9897
>>> Switch
>>>>> PHY support
>>>>>
>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know
>>> the
>>>>> content is safe
>>>>>
>>>>> There is a DSA driver for microchip,ksz9897 which can be controlled
>>>>> through SPI or I2C. This patch adds support for it's CPU ports PHYs to
>>>>> also allow network access to the switch's CPU port.
>>>>>
>>>>> The CPU ports PHYs of the KSZ9897 are not documented in the datasheet.
>>>>> They weirdly use the same PHY ID as the KSZ8081, which is a different
>>>>> PHY and that driver isn't compatible with KSZ9897. Before this patch,
>>>>> the KSZ8081 driver was used for the CPU ports of the KSZ9897 but the
>>>>> link would never come up.
>>>>>
>>>>> A new driver for the KSZ9897 is added, based on the compatible 
>>>>> KSZ87XX.
>>>>> I could not test if Gigabit Ethernet works, but the link comes up and
>>>>> can successfully allow packets to be sent and received with DSA tags.
>>>>>
>>>>> To resolve the KSZ8081/KSZ9897 phy_id conflicts, I could not find any
>>>>> stable register to distinguish them. Instead of a match_phy_device() ,
>>>>> I've declared a virtual phy_id with the highest value in 
>>>>> Microchip's OUI
>>>>> range.
>>>>>
>>>>> Example usage in the device tree:
>>>>>           compatible = "ethernet-phy-id0022.17ff";
>>>>>
>>>>> A discussion to find better alternatives had been opened with the
>>>>> Microchip team, with no response yet.
>>>>>
>>>>> See https://lore.kernel.org/all/20220207174532.362781-1-enguerrand.de-
>>>>> ribaucourt@savoirfairelinux.com/
>>>>>
>>>>> Fixes: b987e98e50ab ("dsa: add DSA switch driver for Microchip 
>>>>> KSZ9477")
>>>>> Signed-off-by: Enguerrand de Ribaucourt <enguerrand.de-
>>>>> ribaucourt@savoirfairelinux.com>
>>>>> ---
>>>>> v5:
>>>>>    - rewrap comments
>>>>>    - restore suspend/resume for KSZ9897
>>>>> v4: https://lore.kernel.org/all/20240531142430.678198-2-enguerrand.de-
>>>>> ribaucourt@savoirfairelinux.com/
>>>>>    - rebase on net/main
>>>>>    - add Fixes tag
>>>>>    - use pseudo phy_id instead of of_tree search
>>>>> v3: https://lore.kernel.org/all/20240530102436.226189-2-enguerrand.de-
>>>>> ribaucourt@savoirfairelinux.com/
>>>>> ---
>>>>>    drivers/net/phy/micrel.c   | 13 ++++++++++++-
>>>>>    include/linux/micrel_phy.h |  4 ++++
>>>>>    2 files changed, 16 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
>>>>> index 8c20cf937530..11e58fc628df 100644
>>>>> --- a/drivers/net/phy/micrel.c
>>>>> +++ b/drivers/net/phy/micrel.c
>>>>> @@ -16,7 +16,7 @@
>>>>>     *                        ksz8081, ksz8091,
>>>>>     *                        ksz8061,
>>>>>     *             Switch : ksz8873, ksz886x
>>>>> - *                      ksz9477, lan8804
>>>>> + *                      ksz9477, ksz9897, lan8804
>>>>>     */
>>>>>
>>>>>    #include <linux/bitfield.h>
>>>>> @@ -5545,6 +5545,16 @@ static struct phy_driver ksphy_driver[] = {
>>>>>           .suspend        = genphy_suspend,
>>>>>           .resume         = ksz9477_resume,
>>>>>           .get_features   = ksz9477_get_features,
>>>>> +}, {
>>>>> +       .phy_id         = PHY_ID_KSZ9897,
>>>>> +       .phy_id_mask    = MICREL_PHY_ID_MASK,
>>>>> +       .name           = "Microchip KSZ9897 Switch",
>>>>> +       /* PHY_BASIC_FEATURES */
>>>>> +       .config_init    = kszphy_config_init,
>>>>> +       .config_aneg    = ksz8873mll_config_aneg,
>>>>> +       .read_status    = ksz8873mll_read_status,
>>>>> +       .suspend        = genphy_suspend,
>>>>> +       .resume         = genphy_resume,
>>>>>    } };
>>>>>
>>>>>    module_phy_driver(ksphy_driver);
>>>>> @@ -5570,6 +5580,7 @@ static struct mdio_device_id __maybe_unused
>>>>> micrel_tbl[] = {
>>>>>           { PHY_ID_LAN8814, MICREL_PHY_ID_MASK },
>>>>>           { PHY_ID_LAN8804, MICREL_PHY_ID_MASK },
>>>>>           { PHY_ID_LAN8841, MICREL_PHY_ID_MASK },
>>>>> +       { PHY_ID_KSZ9897, MICREL_PHY_ID_MASK },
>>>>>           { }
>>>>>    };
>>>>>
>>>>> diff --git a/include/linux/micrel_phy.h b/include/linux/micrel_phy.h
>>>>> index 591bf5b5e8dc..81cc16dc2ddf 100644
>>>>> --- a/include/linux/micrel_phy.h
>>>>> +++ b/include/linux/micrel_phy.h
>>>>> @@ -39,6 +39,10 @@
>>>>>    #define PHY_ID_KSZ87XX         0x00221550
>>>>>
>>>>>    #define        PHY_ID_KSZ9477          0x00221631
>>>>> +/* Pseudo ID to specify in compatible field of device tree.
>>>>> + * Otherwise the device reports the same ID as KSZ8081 on CPU ports.
>>>>> + */
>>>>> +#define        PHY_ID_KSZ9897          0x002217ff
>>>>>
>>>>>    /* struct phy_device dev_flags definitions */
>>>>>    #define MICREL_PHY_50MHZ_CLK   BIT(0)
>>>>> -- 
>>>>> 2.34.1
>>>>
> 

-- 
Savoir-faire Linux
Enguerrand de Ribaucourt
Consultant en logiciel libre / Ingénieur systèmes embarqués | Rennes, Fr
Site web <https://www.savoirfairelinux.com/> | Blog
<https://blog.savoirfairelinux.com/fr-ca/> | Jami <https://jami.net/>

Messages de confidentialité : Ce courriel (de même que les fichiers
joints) est strictement réservé à l'usage de la personne ou de l'entité
à qui il est adressé et peut contenir de l'information privilégiée et
confidentielle. Toute divulgation, distribution ou copie de ce courriel
est strictement prohibée. Si vous avez reçu ce courriel par erreur,
veuillez nous en aviser sur-le-champ, détruire toutes les copies et le
supprimer de votre système informatique.

RE: [PATCH net v5 1/4] net: phy: micrel: add Microchip KSZ 9897 Switch PHY support

[Woojung.Huh]

Hi Enguerrand,

I think this thread is becoming hardware related debugging than patch work.
Would you mind spinning off new thread with fewer recipients for further discussion?

Thanks.
Woojung

> -----Original Message-----
> From: Enguerrand de Ribaucourt <enguerrand.de-
> ribaucourt@savoirfairelinux.com>
> Sent: Friday, June 7, 2024 4:33 AM
> To: Woojung Huh - C21699 <Woojung.Huh@microchip.com>
> Cc: andrew@lunn.ch; hkallweit1@gmail.com; linux@armlinux.org.uk;
> UNGLinuxDriver <UNGLinuxDriver@microchip.com>; horms@kernel.org; Tristram Ha
> - C24268 <Tristram.Ha@microchip.com>; Arun Ramadoss - I17769
> <Arun.Ramadoss@microchip.com>; netdev@vger.kernel.org
> Subject: Re: [PATCH net v5 1/4] net: phy: micrel: add Microchip KSZ 9897
> Switch PHY support
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> On 07/06/2024 10:11, Enguerrand de Ribaucourt wrote:
> >
> > Hello,
> >
> > The exact hardware is a Phycore-i.MX6ULL. ENET2 is directly the
> > i.MX6ULL's FEC that connects to port 6 of the KSZ9897R (GMAC6) in RMII:
> >
> >   - X_ENET2_TX_CLK -- TX_CLK6
> >   - X_ENET2_TX_EN  -- TX_CTL6
> >   - X_ENET2_TX_D1  -- TXD6_1
> >   - X_ENET2_TX_D0  -- TXD6_0
> >   - X_ENET2_RX_EN  -- RX_CTL6
> >   - X_ENET2_RX_ER  -- RX_ER6
> >   - X_ENET2_RX_D1  -- RXD6_1
> >   - X_ENET2_RX_D0  -- RXD6_0
> >
> > The DSA control is using SPI, but not involved in reading the phy_id in
> > my case.
> >
> > This is materialized in my device tree:
> >
> > ```c
> > ethernet@20b4000 {
> >      compatible = "fsl,imx6ul-fec\0fsl,imx6q-fec";
> >      ...
> >      phy-mode = "rmii";
> >      phy-handle = <0x15>;
> >      fixed-link {
> >          speed = <0x64>;
> >          full-duplex;
> >      };
> > };
> >
> > // MDIO bus is only defined on eth1 but shared with eth2
> > ethernet@2188000 {
> >          ...
> >      mdio {
> >                  ...
> >          ksz9897port5@1 {
> >              compatible = "ethernet-phy-ieee802.3-c22";
> >              ...
> >              clock-names = "rmii-ref";
> >              phandle = <0x15>;
> >          };
> > };
> >
> > spi@2010000 {
> >          ...
> >      ksz9897@0 {
> >          compatible = "microchip,ksz9897";
> >          ...
> >          ports {
> >              ...
> >              // GMAC6
> >              port@5 {
> >                  reg = <0x05>;
> >                  label = "cpu";
> >                  ethernet = <0x0c>;
> >                  phy-mode = "rmii";
> >                  rx-internal-delay-ps = <0x5dc>;
> >                  fixed-link {
> >                      speed = <0x64>;
> >                      full-duplex;
> >                  };
> >              };
> >          };
> >      };
> > };
> > ```
> >
> 
> I also checked using `phy-mode = "internal";` on both ends, but ended up
> with error "Unable to connect to phy".
> 
> > Before I implemented the pseudo phy_id, it was read in the generic IEEE
> > clause 22 PHY registers, through the compatible
> > "ethernet-phy-ieee802.3-c22". That would be implemented in
> > get_phy_c22_id() in MII_PHYSID1/2 registers at 0x2 of the MDIO device.
> >
> > It is not read through SPI registers 0x6104-0x6105 which are not defined
> > in the datasheet for port 6/7 (section 5.2.2.3):
> >      Address: 0xN104, Size: 16 bits, Port N: 1-5
> >
> > Do you have other suggestions to read the phy_id?
> >
> > Thanks for your support,
> > Enguerrand de Ribaucourt
> >
> >
> > On 07/06/2024 00:57, Woojung.Huh@microchip.com wrote:
> >> Hi Enguerrand,
> >>
> >> We still can't reproduce what you observed with KSZ9897.
> >>
> >> Just to be sure, you accessed PHY register of Port 6 which is GMAC6.
> >> It is directly connected to MAC of i.MX6ULL over RMII.
> >> I guess the PHY ID access is register 0x6104-0x6105 of KSZ9897.
> >> And, return value of PHY ID is 0x0022-0x1561.
> >>
> >> Correct understanding?
> >> > Thanks.
> >> Woojung
> >>
> >>> -----Original Message-----
> >>> From: Enguerrand de Ribaucourt <enguerrand.de-
> >>> ribaucourt@savoirfairelinux.com>
> >>> Sent: Wednesday, June 5, 2024 4:34 AM
> >>> To: Woojung Huh - C21699 <Woojung.Huh@microchip.com>
> >>> Cc: andrew@lunn.ch; hkallweit1@gmail.com; linux@armlinux.org.uk;
> >>> UNGLinuxDriver <UNGLinuxDriver@microchip.com>; horms@kernel.org;
> >>> Tristram Ha
> >>> - C24268 <Tristram.Ha@microchip.com>; Arun Ramadoss - I17769
> >>> <Arun.Ramadoss@microchip.com>; netdev@vger.kernel.org
> >>> Subject: Re: [PATCH net v5 1/4] net: phy: micrel: add Microchip KSZ 9897
> >>> Switch PHY support
> >>>
> >>> EXTERNAL EMAIL: Do not click links or open attachments unless you
> >>> know the
> >>> content is safe
> >>>
> >>> Hello,
> >>>
> >>> On 04/06/2024 22:49, Woojung.Huh@microchip.com wrote:
> >>>> Hi Enguerrand,
> >>>>
> >>>> Can you help me to understand your setup? I could see you are using
> >>>>    - Host CPU : i.MX6ULL
> >>>>    - DSA Switch : KSZ9897R
> >>>> (https://www.microchip.com/en-us/product/ksz9897)
> >>>>    - Host-to-KSZ interface : RGMII for data path & SPI for control
> >>>> Based on this, CPU port is either GMAC6 or GMAC7 (Figure 2-1 of [1])
> >>>>
> >>>> I have two questions for you.
> >>>> 1. PHY on CPU port
> >>>>      Which GMAC (or port number) is connected between Host CPU and
> >>>> KSZ9897R?
> >>>>      If CPU port is either GMAC6 or GMAC7, it is just a MAC-to-MAC
> >>> connection over RGMII.
> >>>
> >>> I'm using port number 6 as the CPU port for KSZ9897R. GMAC6 is directly
> >>> connected to the MAC of i.MX6ULL (driver is i.MX fec). I'm using RMII
> >>> since gigabit is not supported by the i.MX6ULL.
> >>>
> >>>> 2. PHY ID
> >>>>      Its PHY ID is different when checking datasheet of KSZ9897 and
> >>>> KSZ8081.
> >>>>      PHY ID of Port 1-5 of KSZ9897 is 0x0022-0x1631 per [1]
> >>>>      PHY ID of KSZ8081 is 0x0022-0x0156x per [2]
> >>> That's true for port 1-5, however, I found out that the phy_id emitted
> >>> by GMAC6 is 0x00221561. It is the same as KSZ8081-revA3 according to the
> >>> datasheet. I also studied all registers at runtime for a reliable
> >>> difference to implement something like ksz8051_ksz8795_match_phy_device
> >>> between GMAC6 and KSZ8081, but none appeared to me. Following
> >>> suggestions by Andrew Lunn, I added this virtual phy_id (0x002217ff) to
> >>> hardcode in the devicetree. I'm happy with this solution.
> >>>>
> >>>> Beside patch, you can create a ticket to Microchip site
> >>> (https://microchipsupport.force.com/s/supportservice)
> >>>> if you think it is easier to solve your problem.
> >>> I created a joined ticket for tracking (Case number 01457279).
> >>>>
> >>>
> >>> Thank you very much for your time,
> >>>
> >>> Enguerrand de Ribaucourt
> >>>
> >>>> Best regards,
> >>>> Woojung
> >>>>
> >>>> [1]
> >>>
> https://ww1.microchip.com/downloads/aemDocuments/documents/OTH/ProductDocume
> >>> nts/DataSheets/KSZ9897R-Data-Sheet-DS00002330D.pdf
> >>>> [2] https://www.microchip.com/en-us/product/ksz8081#document-table
> >>>>
> >>>>> -----Original Message-----
> >>>>> From: Enguerrand de Ribaucourt <enguerrand.de-
> >>>>> ribaucourt@savoirfairelinux.com>
> >>>>> Sent: Tuesday, June 4, 2024 5:23 AM
> >>>>> To: netdev@vger.kernel.org
> >>>>> Cc: andrew@lunn.ch; hkallweit1@gmail.com; linux@armlinux.org.uk;
> >>>>> Woojung
> >>> Huh
> >>>>> - C21699 <Woojung.Huh@microchip.com>; UNGLinuxDriver
> >>>>> <UNGLinuxDriver@microchip.com>; horms@kernel.org; Tristram Ha - C24268
> >>>>> <Tristram.Ha@microchip.com>; Arun Ramadoss - I17769
> >>>>> <Arun.Ramadoss@microchip.com>; Enguerrand de Ribaucourt
> >>>>> <enguerrand.de-
> >>>>> ribaucourt@savoirfairelinux.com>
> >>>>> Subject: [PATCH net v5 1/4] net: phy: micrel: add Microchip KSZ 9897
> >>> Switch
> >>>>> PHY support
> >>>>>
> >>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know
> >>> the
> >>>>> content is safe
> >>>>>
> >>>>> There is a DSA driver for microchip,ksz9897 which can be controlled
> >>>>> through SPI or I2C. This patch adds support for it's CPU ports PHYs to
> >>>>> also allow network access to the switch's CPU port.
> >>>>>
> >>>>> The CPU ports PHYs of the KSZ9897 are not documented in the datasheet.
> >>>>> They weirdly use the same PHY ID as the KSZ8081, which is a different
> >>>>> PHY and that driver isn't compatible with KSZ9897. Before this patch,
> >>>>> the KSZ8081 driver was used for the CPU ports of the KSZ9897 but the
> >>>>> link would never come up.
> >>>>>
> >>>>> A new driver for the KSZ9897 is added, based on the compatible
> >>>>> KSZ87XX.
> >>>>> I could not test if Gigabit Ethernet works, but the link comes up and
> >>>>> can successfully allow packets to be sent and received with DSA tags.
> >>>>>
> >>>>> To resolve the KSZ8081/KSZ9897 phy_id conflicts, I could not find any
> >>>>> stable register to distinguish them. Instead of a match_phy_device() ,
> >>>>> I've declared a virtual phy_id with the highest value in
> >>>>> Microchip's OUI
> >>>>> range.
> >>>>>
> >>>>> Example usage in the device tree:
> >>>>>           compatible = "ethernet-phy-id0022.17ff";
> >>>>>
> >>>>> A discussion to find better alternatives had been opened with the
> >>>>> Microchip team, with no response yet.
> >>>>>
> >>>>> See https://lore.kernel.org/all/20220207174532.362781-1-enguerrand.de-
> >>>>> ribaucourt@savoirfairelinux.com/
> >>>>>
> >>>>> Fixes: b987e98e50ab ("dsa: add DSA switch driver for Microchip
> >>>>> KSZ9477")
> >>>>> Signed-off-by: Enguerrand de Ribaucourt <enguerrand.de-
> >>>>> ribaucourt@savoirfairelinux.com>
> >>>>> ---
> >>>>> v5:
> >>>>>    - rewrap comments
> >>>>>    - restore suspend/resume for KSZ9897
> >>>>> v4: https://lore.kernel.org/all/20240531142430.678198-2-enguerrand.de-
> >>>>> ribaucourt@savoirfairelinux.com/
> >>>>>    - rebase on net/main
> >>>>>    - add Fixes tag
> >>>>>    - use pseudo phy_id instead of of_tree search
> >>>>> v3: https://lore.kernel.org/all/20240530102436.226189-2-enguerrand.de-
> >>>>> ribaucourt@savoirfairelinux.com/
> >>>>> ---
> >>>>>    drivers/net/phy/micrel.c   | 13 ++++++++++++-
> >>>>>    include/linux/micrel_phy.h |  4 ++++
> >>>>>    2 files changed, 16 insertions(+), 1 deletion(-)
> >>>>>
> >>>>> diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
> >>>>> index 8c20cf937530..11e58fc628df 100644
> >>>>> --- a/drivers/net/phy/micrel.c
> >>>>> +++ b/drivers/net/phy/micrel.c
> >>>>> @@ -16,7 +16,7 @@
> >>>>>     *                        ksz8081, ksz8091,
> >>>>>     *                        ksz8061,
> >>>>>     *             Switch : ksz8873, ksz886x
> >>>>> - *                      ksz9477, lan8804
> >>>>> + *                      ksz9477, ksz9897, lan8804
> >>>>>     */
> >>>>>
> >>>>>    #include <linux/bitfield.h>
> >>>>> @@ -5545,6 +5545,16 @@ static struct phy_driver ksphy_driver[] = {
> >>>>>           .suspend        = genphy_suspend,
> >>>>>           .resume         = ksz9477_resume,
> >>>>>           .get_features   = ksz9477_get_features,
> >>>>> +}, {
> >>>>> +       .phy_id         = PHY_ID_KSZ9897,
> >>>>> +       .phy_id_mask    = MICREL_PHY_ID_MASK,
> >>>>> +       .name           = "Microchip KSZ9897 Switch",
> >>>>> +       /* PHY_BASIC_FEATURES */
> >>>>> +       .config_init    = kszphy_config_init,
> >>>>> +       .config_aneg    = ksz8873mll_config_aneg,
> >>>>> +       .read_status    = ksz8873mll_read_status,
> >>>>> +       .suspend        = genphy_suspend,
> >>>>> +       .resume         = genphy_resume,
> >>>>>    } };
> >>>>>
> >>>>>    module_phy_driver(ksphy_driver);
> >>>>> @@ -5570,6 +5580,7 @@ static struct mdio_device_id __maybe_unused
> >>>>> micrel_tbl[] = {
> >>>>>           { PHY_ID_LAN8814, MICREL_PHY_ID_MASK },
> >>>>>           { PHY_ID_LAN8804, MICREL_PHY_ID_MASK },
> >>>>>           { PHY_ID_LAN8841, MICREL_PHY_ID_MASK },
> >>>>> +       { PHY_ID_KSZ9897, MICREL_PHY_ID_MASK },
> >>>>>           { }
> >>>>>    };
> >>>>>
> >>>>> diff --git a/include/linux/micrel_phy.h b/include/linux/micrel_phy.h
> >>>>> index 591bf5b5e8dc..81cc16dc2ddf 100644
> >>>>> --- a/include/linux/micrel_phy.h
> >>>>> +++ b/include/linux/micrel_phy.h
> >>>>> @@ -39,6 +39,10 @@
> >>>>>    #define PHY_ID_KSZ87XX         0x00221550
> >>>>>
> >>>>>    #define        PHY_ID_KSZ9477          0x00221631
> >>>>> +/* Pseudo ID to specify in compatible field of device tree.
> >>>>> + * Otherwise the device reports the same ID as KSZ8081 on CPU ports.
> >>>>> + */
> >>>>> +#define        PHY_ID_KSZ9897          0x002217ff
> >>>>>
> >>>>>    /* struct phy_device dev_flags definitions */
> >>>>>    #define MICREL_PHY_50MHZ_CLK   BIT(0)
> >>>>> --
> >>>>> 2.34.1
> >>>>
> >
> 
> --
> Savoir-faire Linux
> Enguerrand de Ribaucourt
> Consultant en logiciel libre / Ingénieur systèmes embarqués | Rennes, Fr
> Site web <https://www.savoirfairelinux.com/> | Blog
> <https://blog.savoirfairelinux.com/fr-ca/> | Jami <https://jami.net/>
> 
> Messages de confidentialité : Ce courriel (de même que les fichiers
> joints) est strictement réservé à l'usage de la personne ou de l'entité
> à qui il est adressé et peut contenir de l'information privilégiée et
> confidentielle. Toute divulgation, distribution ou copie de ce courriel
> est strictement prohibée. Si vous avez reçu ce courriel par erreur,
> veuillez nous en aviser sur-le-champ, détruire toutes les copies et le
> supprimer de votre système informatique.

[PATCH net v5 2/4] net: phy: micrel: add Microchip KSZ 9477 to the device table

[Enguerrand de Ribaucourt]

PHY_ID_KSZ9477 was supported but not added to the device table passed to
MODULE_DEVICE_TABLE.

Fixes: fc3973a1fa09 ("phy: micrel: add Microchip KSZ 9477 Switch PHY support")
Signed-off-by: Enguerrand de Ribaucourt <enguerrand.de-ribaucourt@savoirfairelinux.com>
---
v4:
 - rebase on net/main
 - add Fixes tag
v3: https://lore.kernel.org/all/20240530102436.226189-4-enguerrand.de-ribaucourt@savoirfairelinux.com/
---
 drivers/net/phy/micrel.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
index 11e58fc628df..d0ecbb3f7429 100644
--- a/drivers/net/phy/micrel.c
+++ b/drivers/net/phy/micrel.c
@@ -5577,6 +5577,7 @@ static struct mdio_device_id __maybe_unused micrel_tbl[] = {
 	{ PHY_ID_KSZ8081, MICREL_PHY_ID_MASK },
 	{ PHY_ID_KSZ8873MLL, MICREL_PHY_ID_MASK },
 	{ PHY_ID_KSZ886X, MICREL_PHY_ID_MASK },
+	{ PHY_ID_KSZ9477, MICREL_PHY_ID_MASK },
 	{ PHY_ID_LAN8814, MICREL_PHY_ID_MASK },
 	{ PHY_ID_LAN8804, MICREL_PHY_ID_MASK },
 	{ PHY_ID_LAN8841, MICREL_PHY_ID_MASK },
-- 
2.34.1

[PATCH net v5 3/4] net: dsa: microchip: use collision based back pressure mode

[Enguerrand de Ribaucourt]

Errata DS80000758 states that carrier sense back pressure mode can cause
link down issues in 100BASE-TX half duplex mode. The datasheet also
recommends to always use the collision based back pressure mode.

Fixes: b987e98e50ab ("dsa: add DSA switch driver for Microchip KSZ9477")
Signed-off-by: Enguerrand de Ribaucourt <enguerrand.de-ribaucourt@savoirfairelinux.com>
---
v5:
 - define SW_BACK_PRESSURE_COLLISION
v4: https://lore.kernel.org/all/20240531142430.678198-5-enguerrand.de-ribaucourt@savoirfairelinux.com/
 - rebase on net/main
 - add Fixes tag
v3: https://lore.kernel.org/all/20240530102436.226189-5-enguerrand.de-ribaucourt@savoirfairelinux.com/
---
 drivers/net/dsa/microchip/ksz9477.c     | 4 ++++
 drivers/net/dsa/microchip/ksz9477_reg.h | 1 +
 2 files changed, 5 insertions(+)

diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
index f8ad7833f5d9..c2878dd0ad7e 100644
--- a/drivers/net/dsa/microchip/ksz9477.c
+++ b/drivers/net/dsa/microchip/ksz9477.c
@@ -1299,6 +1299,10 @@ int ksz9477_setup(struct dsa_switch *ds)
 	/* Enable REG_SW_MTU__2 reg by setting SW_JUMBO_PACKET */
 	ksz_cfg(dev, REG_SW_MAC_CTRL_1, SW_JUMBO_PACKET, true);
 
+	/* Use collision based back pressure mode. */
+	ksz_cfg(dev, REG_SW_MAC_CTRL_1, SW_BACK_PRESSURE,
+		SW_BACK_PRESSURE_COLLISION);
+
 	/* Now we can configure default MTU value */
 	ret = regmap_update_bits(ksz_regmap_16(dev), REG_SW_MTU__2, REG_SW_MTU_MASK,
 				 VLAN_ETH_FRAME_LEN + ETH_FCS_LEN);
diff --git a/drivers/net/dsa/microchip/ksz9477_reg.h b/drivers/net/dsa/microchip/ksz9477_reg.h
index f3a205ee483f..fb124be8edd3 100644
--- a/drivers/net/dsa/microchip/ksz9477_reg.h
+++ b/drivers/net/dsa/microchip/ksz9477_reg.h
@@ -247,6 +247,7 @@
 #define REG_SW_MAC_CTRL_1		0x0331
 
 #define SW_BACK_PRESSURE		BIT(5)
+#define SW_BACK_PRESSURE_COLLISION	0
 #define FAIR_FLOW_CTRL			BIT(4)
 #define NO_EXC_COLLISION_DROP		BIT(3)
 #define SW_JUMBO_PACKET			BIT(2)
-- 
2.34.1

RE: [PATCH net v5 3/4] net: dsa: microchip: use collision based back pressure mode

[Woojung.Huh]

> -----Original Message-----
> From: Enguerrand de Ribaucourt <enguerrand.de-
> ribaucourt@savoirfairelinux.com>
> Sent: Tuesday, June 4, 2024 5:23 AM
> To: netdev@vger.kernel.org
> Cc: andrew@lunn.ch; hkallweit1@gmail.com; linux@armlinux.org.uk; Woojung Huh
> - C21699 <Woojung.Huh@microchip.com>; UNGLinuxDriver
> <UNGLinuxDriver@microchip.com>; horms@kernel.org; Tristram Ha - C24268
> <Tristram.Ha@microchip.com>; Arun Ramadoss - I17769
> <Arun.Ramadoss@microchip.com>; Enguerrand de Ribaucourt <enguerrand.de-
> ribaucourt@savoirfairelinux.com>
> Subject: [PATCH net v5 3/4] net: dsa: microchip: use collision based back
> pressure mode
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> Errata DS80000758 states that carrier sense back pressure mode can cause
> link down issues in 100BASE-TX half duplex mode. The datasheet also
> recommends to always use the collision based back pressure mode.
> 
> Fixes: b987e98e50ab ("dsa: add DSA switch driver for Microchip KSZ9477")
> Signed-off-by: Enguerrand de Ribaucourt <enguerrand.de-
> ribaucourt@savoirfairelinux.com>
> ---
> v5:
>  - define SW_BACK_PRESSURE_COLLISION
> v4: https://lore.kernel.org/all/20240531142430.678198-5-enguerrand.de-
> ribaucourt@savoirfairelinux.com/
>  - rebase on net/main
>  - add Fixes tag
> v3: https://lore.kernel.org/all/20240530102436.226189-5-enguerrand.de-
> ribaucourt@savoirfairelinux.com/

Reviewed-by: Woojung Huh <Woojung.huh@microchip.com>

Re: [PATCH net v5 3/4] net: dsa: microchip: use collision based back pressure mode

[Arun.Ramadoss]

On Tue, 2024-06-04 at 09:23 +0000, Enguerrand de Ribaucourt wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> Errata DS80000758 states that carrier sense back pressure mode can
> cause
> link down issues in 100BASE-TX half duplex mode. The datasheet also
> recommends to always use the collision based back pressure mode.
> 
> Fixes: b987e98e50ab ("dsa: add DSA switch driver for Microchip
> KSZ9477")
> Signed-off-by: Enguerrand de Ribaucourt <
> enguerrand.de-ribaucourt@savoirfairelinux.com>

Acked-by: Arun Ramadoss <arun.ramadoss@microchip.com>

[PATCH net v5 4/4] net: dsa: microchip: monitor potential faults in half-duplex mode

[Enguerrand de Ribaucourt]

The errata DS80000754 recommends monitoring potential faults in
half-duplex mode for the KSZ9477 family.

half-duplex is not very common so I just added a critical message
when the fault conditions are detected. The switch can be expected
to be unable to communicate anymore in these states and a software
reset of the switch would be required which I did not implement.

Fixes: b987e98e50ab ("dsa: add DSA switch driver for Microchip KSZ9477")
Signed-off-by: Enguerrand de Ribaucourt <enguerrand.de-ribaucourt@savoirfairelinux.com>
---
v5:
 - use macros for bitmasks
 - check for return values on ksz_pread*
v4: https://lore.kernel.org/all/20240531142430.678198-6-enguerrand.de-ribaucourt@savoirfairelinux.com/
 - rebase on net/main
 - add Fixes tag
 - reverse x-mas tree
v3: https://lore.kernel.org/all/20240530102436.226189-6-enguerrand.de-ribaucourt@savoirfairelinux.com/
---
 drivers/net/dsa/microchip/ksz9477.c     | 42 +++++++++++++++++++++++++
 drivers/net/dsa/microchip/ksz9477.h     |  2 ++
 drivers/net/dsa/microchip/ksz9477_reg.h |  9 ++++--
 drivers/net/dsa/microchip/ksz_common.c  | 11 +++++++
 drivers/net/dsa/microchip/ksz_common.h  |  1 +
 5 files changed, 63 insertions(+), 2 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
index c2878dd0ad7e..30c1d8a748d4 100644
--- a/drivers/net/dsa/microchip/ksz9477.c
+++ b/drivers/net/dsa/microchip/ksz9477.c
@@ -429,6 +429,48 @@ void ksz9477_freeze_mib(struct ksz_device *dev, int port, bool freeze)
 	mutex_unlock(&p->mib.cnt_mutex);
 }
 
+int ksz9477_errata_monitor(struct ksz_device *dev, int port,
+			   u64 tx_late_col)
+{
+	u32 pmavbc;
+	u8 status;
+	u16 pqm;
+	int ret;
+
+	ret = ksz_pread8(dev, port, REG_PORT_STATUS_0, &status);
+	if (ret)
+		return ret;
+	if (!((status & PORT_INTF_SPEED_MASK) == PORT_INTF_SPEED_MASK) &&
+	    !(status & PORT_INTF_FULL_DUPLEX)) {
+		dev_warn_once(dev->dev,
+			      "Half-duplex detected on port %d, transmission halt may occur\n",
+			      port);
+		/* Errata DS80000754 recommends monitoring potential faults in
+		 * half-duplex mode. The switch might not be able to communicate anymore
+		 * in these states.
+		 */
+		if (tx_late_col != 0) {
+			/* Transmission halt with late collisions */
+			dev_crit_ratelimited(dev->dev,
+					     "TX late collisions detected, transmission may be halted on port %d\n",
+					     port);
+		}
+		ret = ksz_pread16(dev, port, REG_PORT_QM_TX_CNT_0__4, &pqm);
+		if (ret)
+			return ret;
+		ret = ksz_read32(dev, REG_PMAVBC, &pmavbc);
+		if (ret)
+			return ret;
+		if ((FIELD_GET(PMAVBC_MASK, pmavbc) <= PMAVBC_MIN) ||
+		    (FIELD_GET(PORT_QM_TX_CNT_M, pqm) >= PORT_QM_TX_CNT_MAX)) {
+			/* Transmission halt with Half-Duplex and VLAN */
+			dev_crit_ratelimited(dev->dev,
+					     "resources out of limits, transmission may be halted\n");
+		}
+	}
+	return ret;
+}
+
 void ksz9477_port_init_cnt(struct ksz_device *dev, int port)
 {
 	struct ksz_port_mib *mib = &dev->ports[port].mib;
diff --git a/drivers/net/dsa/microchip/ksz9477.h b/drivers/net/dsa/microchip/ksz9477.h
index ce1e656b800b..239a281da10b 100644
--- a/drivers/net/dsa/microchip/ksz9477.h
+++ b/drivers/net/dsa/microchip/ksz9477.h
@@ -36,6 +36,8 @@ int ksz9477_port_mirror_add(struct ksz_device *dev, int port,
 			    bool ingress, struct netlink_ext_ack *extack);
 void ksz9477_port_mirror_del(struct ksz_device *dev, int port,
 			     struct dsa_mall_mirror_tc_entry *mirror);
+int ksz9477_errata_monitor(struct ksz_device *dev, int port,
+			   u64 tx_late_col);
 void ksz9477_get_caps(struct ksz_device *dev, int port,
 		      struct phylink_config *config);
 int ksz9477_fdb_dump(struct ksz_device *dev, int port,
diff --git a/drivers/net/dsa/microchip/ksz9477_reg.h b/drivers/net/dsa/microchip/ksz9477_reg.h
index fb124be8edd3..21fd9cbc3cc1 100644
--- a/drivers/net/dsa/microchip/ksz9477_reg.h
+++ b/drivers/net/dsa/microchip/ksz9477_reg.h
@@ -843,8 +843,7 @@
 
 #define REG_PORT_STATUS_0		0x0030
 
-#define PORT_INTF_SPEED_M		0x3
-#define PORT_INTF_SPEED_S		3
+#define PORT_INTF_SPEED_MASK		GENMASK(4, 3)
 #define PORT_INTF_FULL_DUPLEX		BIT(2)
 #define PORT_TX_FLOW_CTRL		BIT(1)
 #define PORT_RX_FLOW_CTRL		BIT(0)
@@ -1168,6 +1167,11 @@
 #define PORT_RMII_CLK_SEL		BIT(7)
 #define PORT_MII_SEL_EDGE		BIT(5)
 
+#define REG_PMAVBC			0x03AC
+
+#define PMAVBC_MASK			GENMASK(26, 16)
+#define PMAVBC_MIN			0x580
+
 /* 4 - MAC */
 #define REG_PORT_MAC_CTRL_0		0x0400
 
@@ -1495,6 +1499,7 @@
 
 #define PORT_QM_TX_CNT_USED_S		0
 #define PORT_QM_TX_CNT_M		(BIT(11) - 1)
+#define PORT_QM_TX_CNT_MAX		0x200
 
 #define REG_PORT_QM_TX_CNT_1__4		0x0A14
 
diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index 2818e24e2a51..0433109b42e5 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -1382,6 +1382,7 @@ const struct ksz_chip_data ksz_switch_chips[] = {
 		.tc_cbs_supported = true,
 		.ops = &ksz9477_dev_ops,
 		.phylink_mac_ops = &ksz9477_phylink_mac_ops,
+		.phy_errata_9477 = true,
 		.mib_names = ksz9477_mib_names,
 		.mib_cnt = ARRAY_SIZE(ksz9477_mib_names),
 		.reg_mib_cnt = MIB_COUNTER_NUM,
@@ -1416,6 +1417,7 @@ const struct ksz_chip_data ksz_switch_chips[] = {
 		.num_ipms = 8,
 		.ops = &ksz9477_dev_ops,
 		.phylink_mac_ops = &ksz9477_phylink_mac_ops,
+		.phy_errata_9477 = true,
 		.mib_names = ksz9477_mib_names,
 		.mib_cnt = ARRAY_SIZE(ksz9477_mib_names),
 		.reg_mib_cnt = MIB_COUNTER_NUM,
@@ -1450,6 +1452,7 @@ const struct ksz_chip_data ksz_switch_chips[] = {
 		.num_ipms = 8,
 		.ops = &ksz9477_dev_ops,
 		.phylink_mac_ops = &ksz9477_phylink_mac_ops,
+		.phy_errata_9477 = true,
 		.mib_names = ksz9477_mib_names,
 		.mib_cnt = ARRAY_SIZE(ksz9477_mib_names),
 		.reg_mib_cnt = MIB_COUNTER_NUM,
@@ -1540,6 +1543,7 @@ const struct ksz_chip_data ksz_switch_chips[] = {
 		.tc_cbs_supported = true,
 		.ops = &ksz9477_dev_ops,
 		.phylink_mac_ops = &ksz9477_phylink_mac_ops,
+		.phy_errata_9477 = true,
 		.mib_names = ksz9477_mib_names,
 		.mib_cnt = ARRAY_SIZE(ksz9477_mib_names),
 		.reg_mib_cnt = MIB_COUNTER_NUM,
@@ -1820,6 +1824,7 @@ void ksz_r_mib_stats64(struct ksz_device *dev, int port)
 	struct rtnl_link_stats64 *stats;
 	struct ksz_stats_raw *raw;
 	struct ksz_port_mib *mib;
+	int ret;
 
 	mib = &dev->ports[port].mib;
 	stats = &mib->stats64;
@@ -1861,6 +1866,12 @@ void ksz_r_mib_stats64(struct ksz_device *dev, int port)
 	pstats->rx_pause_frames = raw->rx_pause;
 
 	spin_unlock(&mib->stats64_lock);
+
+	if (dev->info->phy_errata_9477) {
+		ret = ksz9477_errata_monitor(dev, port, raw->tx_late_col);
+		if (ret)
+			dev_err(dev->dev, "Failed to monitor transmission halt\n");
+	}
 }
 
 void ksz88xx_r_mib_stats64(struct ksz_device *dev, int port)
diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
index c784fd23a993..ee7db46e469d 100644
--- a/drivers/net/dsa/microchip/ksz_common.h
+++ b/drivers/net/dsa/microchip/ksz_common.h
@@ -66,6 +66,7 @@ struct ksz_chip_data {
 	bool tc_cbs_supported;
 	const struct ksz_dev_ops *ops;
 	const struct phylink_mac_ops *phylink_mac_ops;
+	bool phy_errata_9477;
 	bool ksz87xx_eee_link_erratum;
 	const struct ksz_mib_names *mib_names;
 	int mib_cnt;
-- 
2.34.1

RE: [PATCH net v5 4/4] net: dsa: microchip: monitor potential faults in half-duplex mode

[Woojung.Huh]

Hi Enguerrand,

Looks you are covering Module 17 & 23 in Errata[1].

[1] https://ww1.microchip.com/downloads/aemDocuments/documents/UNG/ProductDocuments/Errata/KSZ9477S-Errata-DS80000754.pdf

> The errata DS80000754 recommends monitoring potential faults in
> half-duplex mode for the KSZ9477 family.
> 
> half-duplex is not very common so I just added a critical message
> when the fault conditions are detected. The switch can be expected
> to be unable to communicate anymore in these states and a software
> reset of the switch would be required which I did not implement.
> 
> Fixes: b987e98e50ab ("dsa: add DSA switch driver for Microchip KSZ9477")
> Signed-off-by: Enguerrand de Ribaucourt <enguerrand.de-
> ribaucourt@savoirfairelinux.com>
...
> +int ksz9477_errata_monitor(struct ksz_device *dev, int port,
> +                          u64 tx_late_col)
> +{
> +       u32 pmavbc;
> +       u8 status;
> +       u16 pqm;
> +       int ret;
> +
> +       ret = ksz_pread8(dev, port, REG_PORT_STATUS_0, &status);
> +       if (ret)
> +               return ret;
> +       if (!((status & PORT_INTF_SPEED_MASK) == PORT_INTF_SPEED_MASK) &&
> +           !(status & PORT_INTF_FULL_DUPLEX)) {
> +               dev_warn_once(dev->dev,
> +                             "Half-duplex detected on port %d, transmission
> halt may occur\n",
> +                             port);
> +               /* Errata DS80000754 recommends monitoring potential faults
> in
> +                * half-duplex mode. The switch might not be able to
> communicate anymore
> +                * in these states.
> +                */
> +               if (tx_late_col != 0) {
> +                       /* Transmission halt with late collisions */
> +                       dev_crit_ratelimited(dev->dev,
> +                                            "TX late collisions detected,
> transmission may be halted on port %d\n",
> +                                            port);
> +               }

This covers method 1 of Module 18.

Even though MIB read is not high bandwidth (every 30sec) work,
however, still looping over all ports.
Can you tighten condition for Method 2 because it happens
when both half-duplex and VLAN are enabled?

Adding condition such as 
	ret = ksz_read8(dev, REG_SW_LUE_CTRL_0, &status)
	if (status & SW_VLAN_ENABLE) {
	....
	}

> +               ret = ksz_pread16(dev, port, REG_PORT_QM_TX_CNT_0__4, &pqm);
> +               if (ret)
> +                       return ret;
> +               ret = ksz_read32(dev, REG_PMAVBC, &pmavbc);
> +               if (ret)
> +                       return ret;
> +               if ((FIELD_GET(PMAVBC_MASK, pmavbc) <= PMAVBC_MIN) ||
> +                   (FIELD_GET(PORT_QM_TX_CNT_M, pqm) >= PORT_QM_TX_CNT_MAX))
> {
> +                       /* Transmission halt with Half-Duplex and VLAN */
> +                       dev_crit_ratelimited(dev->dev,
> +                                            "resources out of limits,
> transmission may be halted\n");
> +               }
> +       }
> +       return ret;
> +}
> +

Thanks
Woojung

Re: [PATCH net v5 4/4] net: dsa: microchip: monitor potential faults in half-duplex mode

[Arun.Ramadoss]

Hi Enguerrand,


> 
> +int ksz9477_errata_monitor(struct ksz_device *dev, int port,
> +                          u64 tx_late_col)
> +{
> +       u32 pmavbc;
> +       u8 status;
> +       u16 pqm;
> +       int ret;
> +
> +       ret = ksz_pread8(dev, port, REG_PORT_STATUS_0, &status);
> +       if (ret)
> +               return ret;

Blank line after return ret will increase readability.

> +       if (!((status & PORT_INTF_SPEED_MASK) ==
> PORT_INTF_SPEED_MASK) &&

Why this check is needed. Is it to check reserved value 11b.


> +           !(status & PORT_INTF_FULL_DUPLEX)) {
> +               dev_warn_once(dev->dev,
> +                             "Half-duplex detected on port %d,
> transmission halt may occur\n",
> +                             port);
> +               /* Errata DS80000754 recommends monitoring potential
> faults in
> +                * half-duplex mode. The switch might not be able to
> communicate anymore
> +                * in these states.
> +                */
> +               if (tx_late_col != 0) {
> +                       /* Transmission halt with late collisions */
> +                       dev_crit_ratelimited(dev->dev,
> +                                            "TX late collisions
> detected, transmission may be halted on port %d\n",
> +                                            port);
> +               }
> +               ret = ksz_pread16(dev, port, REG_PORT_QM_TX_CNT_0__4,
> &pqm);
> +               if (ret)
> +                       return ret;
> +               ret = ksz_read32(dev, REG_PMAVBC, &pmavbc);
> +               if (ret)
> +                       return ret;
> +               if ((FIELD_GET(PMAVBC_MASK, pmavbc) <= PMAVBC_MIN) ||
> +                   (FIELD_GET(PORT_QM_TX_CNT_M, pqm) >=
> PORT_QM_TX_CNT_MAX)) {
> +                       /* Transmission halt with Half-Duplex and
> VLAN */
> +                       dev_crit_ratelimited(dev->dev,
> +                                            "resources out of
> limits, transmission may be halted\n");
> +               }
> +       }
> +       return ret;
> +}
> +
>  
> 

Re: [PATCH net v5 4/4] net: dsa: microchip: monitor potential faults in half-duplex mode

[Enguerrand de Ribaucourt]

Hello,

On 05/06/2024 05:31, Arun.Ramadoss@microchip.com wrote:
> Hi Enguerrand,
> 
> 
>>
>> +int ksz9477_errata_monitor(struct ksz_device *dev, int port,
>> +                          u64 tx_late_col)
>> +{
>> +       u32 pmavbc;
>> +       u8 status;
>> +       u16 pqm;
>> +       int ret;
>> +
>> +       ret = ksz_pread8(dev, port, REG_PORT_STATUS_0, &status);
>> +       if (ret)
>> +               return ret;
> 
> Blank line after return ret will increase readability.
> 
>> +       if (!((status & PORT_INTF_SPEED_MASK) ==
>> PORT_INTF_SPEED_MASK) &&
> 
> Why this check is needed. Is it to check reserved value 11b.
> 
Yes indeed, 11b would means that the port is not connected. So here I'm 
isolating ports in half duplex which are properly up.
> 
>> +           !(status & PORT_INTF_FULL_DUPLEX)) {
>> +               dev_warn_once(dev->dev,
>> +                             "Half-duplex detected on port %d,
>> transmission halt may occur\n",
>> +                             port);
>>   
>>

-- 
Savoir-faire Linux
Enguerrand de Ribaucourt

Re: [PATCH net v5 4/4] net: dsa: microchip: monitor potential faults in half-duplex mode

[Arun.Ramadoss]

Hi, 
On Wed, 2024-06-05 at 09:53 +0200, Enguerrand de Ribaucourt wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> Hello,
> 
> On 05/06/2024 05:31, Arun.Ramadoss@microchip.com wrote:
> > Hi Enguerrand,
> > 
> > 
> > > +int ksz9477_errata_monitor(struct ksz_device *dev, int port,
> > > +                          u64 tx_late_col)
> > > +{
> > > +       u32 pmavbc;
> > > +       u8 status;
> > > +       u16 pqm;
> > > +       int ret;
> > > +
> > > +       ret = ksz_pread8(dev, port, REG_PORT_STATUS_0, &status);
> > > +       if (ret)
> > > +               return ret;
> > 
> > Blank line after return ret will increase readability.
> > 
> > > +       if (!((status & PORT_INTF_SPEED_MASK) ==
> > > PORT_INTF_SPEED_MASK) &&
> > 
> > Why this check is needed. Is it to check reserved value 11b.
> > 
> Yes indeed, 11b would means that the port is not connected. So here
> I'm
> isolating ports in half duplex which are properly up.

Then can you add this either comment in code or commit description, so
that we can understand. Because macros specifies like we are checking
speed of interface but actual check is whether port is connected or
not.