[PATCH net-next v2 0/7] microchip_t1s: Update on Microchip 10BASE-T1S PHY driver

[PATCH net-next v2 0/7] microchip_t1s: Update on Microchip 10BASE-T1S PHY driver

[Parthiban Veerasooran]

This patch series contain the below updates,

v1:
- Restructured lan865x_write_cfg_params() and lan865x_read_cfg_params()
  functions arguments to more generic.
- Updated new/improved initial settings of LAN865X Rev.B0 from latest
  AN1760.
- Added support for LAN865X Rev.B1 from latest AN1760.
- Moved LAN867X reset handling to a new function for flexibility.
- Added support for LAN867X Rev.C1/C2 from latest AN1699.
- Disabled/enabled collision detection based on PLCA setting.

v2:
- Fixed indexing issue in the configuration parameter setup.

Parthiban Veerasooran (7):
  net: phy: microchip_t1s: restructure cfg read/write functions
    arguments
  net: phy: microchip_t1s: update new initial settings for LAN865X
    Rev.B0
  net: phy: microchip_t1s: add support for Microchip's LAN865X Rev.B1
  net: phy: microchip_t1s: move LAN867X reset handling to a new function
  net: phy: microchip_t1s: add support for Microchip's LAN867X Rev.C1
  net: phy: microchip_t1s: add support for Microchip's LAN867X Rev.C2
  net: phy: microchip_t1s: configure collision detection based on PLCA
    mode

 drivers/net/phy/Kconfig         |   4 +-
 drivers/net/phy/microchip_t1s.c | 299 +++++++++++++++++++++++++-------
 2 files changed, 239 insertions(+), 64 deletions(-)


base-commit: 221f9cce949ac8042f65b71ed1fde13b99073256
-- 
2.34.1

[PATCH net-next v2 1/7] net: phy: microchip_t1s: restructure cfg read/write functions arguments

[Parthiban Veerasooran]

This patch restructures lan865x_write_cfg_params() and
lan865x_read_cfg_params() functions arguments to more generic which will
be useful for the next patch which updates the improved initial
configuration for LAN8650/1 Rev.B0 published in the Configuration Note.

Signed-off-by: Parthiban Veerasooran <Parthiban.Veerasooran@microchip.com>
---
 drivers/net/phy/microchip_t1s.c | 27 ++++++++++++++++-----------
 1 file changed, 16 insertions(+), 11 deletions(-)

diff --git a/drivers/net/phy/microchip_t1s.c b/drivers/net/phy/microchip_t1s.c
index 534ca7d1b061..0110f3357489 100644
--- a/drivers/net/phy/microchip_t1s.c
+++ b/drivers/net/phy/microchip_t1s.c
@@ -112,7 +112,7 @@ static int lan865x_revb0_indirect_read(struct phy_device *phydev, u16 addr)
 /* This is pulled straight from AN1760 from 'calculation of offset 1' &
  * 'calculation of offset 2'
  */
-static int lan865x_generate_cfg_offsets(struct phy_device *phydev, s8 offsets[2])
+static int lan865x_generate_cfg_offsets(struct phy_device *phydev, s8 offsets[])
 {
 	const u16 fixup_regs[2] = {0x0004, 0x0008};
 	int ret;
@@ -130,13 +130,15 @@ static int lan865x_generate_cfg_offsets(struct phy_device *phydev, s8 offsets[2]
 	return 0;
 }
 
-static int lan865x_read_cfg_params(struct phy_device *phydev, u16 cfg_params[])
+static int lan865x_read_cfg_params(struct phy_device *phydev,
+				   const u16 cfg_regs[], u16 cfg_params[],
+				   u8 count)
 {
 	int ret;
 
-	for (int i = 0; i < ARRAY_SIZE(lan865x_revb0_fixup_cfg_regs); i++) {
+	for (int i = 0; i < count; i++) {
 		ret = phy_read_mmd(phydev, MDIO_MMD_VEND2,
-				   lan865x_revb0_fixup_cfg_regs[i]);
+				   cfg_regs[i]);
 		if (ret < 0)
 			return ret;
 		cfg_params[i] = (u16)ret;
@@ -145,13 +147,14 @@ static int lan865x_read_cfg_params(struct phy_device *phydev, u16 cfg_params[])
 	return 0;
 }
 
-static int lan865x_write_cfg_params(struct phy_device *phydev, u16 cfg_params[])
+static int lan865x_write_cfg_params(struct phy_device *phydev,
+				    const u16 cfg_regs[], u16 cfg_params[],
+				    u8 count)
 {
 	int ret;
 
-	for (int i = 0; i < ARRAY_SIZE(lan865x_revb0_fixup_cfg_regs); i++) {
-		ret = phy_write_mmd(phydev, MDIO_MMD_VEND2,
-				    lan865x_revb0_fixup_cfg_regs[i],
+	for (int i = 0; i < count; i++) {
+		ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, cfg_regs[i],
 				    cfg_params[i]);
 		if (ret)
 			return ret;
@@ -162,8 +165,8 @@ static int lan865x_write_cfg_params(struct phy_device *phydev, u16 cfg_params[])
 
 static int lan865x_setup_cfgparam(struct phy_device *phydev)
 {
+	u16 cfg_results[ARRAY_SIZE(lan865x_revb0_fixup_cfg_regs)];
 	u16 cfg_params[ARRAY_SIZE(lan865x_revb0_fixup_cfg_regs)];
-	u16 cfg_results[5];
 	s8 offsets[2];
 	int ret;
 
@@ -171,7 +174,8 @@ static int lan865x_setup_cfgparam(struct phy_device *phydev)
 	if (ret)
 		return ret;
 
-	ret = lan865x_read_cfg_params(phydev, cfg_params);
+	ret = lan865x_read_cfg_params(phydev, lan865x_revb0_fixup_cfg_regs,
+				      cfg_params, ARRAY_SIZE(cfg_params));
 	if (ret)
 		return ret;
 
@@ -190,7 +194,8 @@ static int lan865x_setup_cfgparam(struct phy_device *phydev)
 			  FIELD_PREP(GENMASK(15, 8), 17 + offsets[0]) |
 			  (22 + offsets[0]);
 
-	return lan865x_write_cfg_params(phydev, cfg_results);
+	return lan865x_write_cfg_params(phydev, lan865x_revb0_fixup_cfg_regs,
+					cfg_results, ARRAY_SIZE(cfg_results));
 }
 
 static int lan865x_revb0_config_init(struct phy_device *phydev)
-- 
2.34.1

[PATCH net-next v2 2/7] net: phy: microchip_t1s: update new initial settings for LAN865X Rev.B0

[Parthiban Veerasooran]

This patch configures the new/improved initial settings from the latest
configuration application note AN1760 released for LAN8650/1 Rev.B0
Revision F (DS60001760G - June 2024).
https://www.microchip.com/en-us/application-notes/an1760

Signed-off-by: Parthiban Veerasooran <Parthiban.Veerasooran@microchip.com>
---
 drivers/net/phy/microchip_t1s.c | 119 ++++++++++++++++++++++----------
 1 file changed, 83 insertions(+), 36 deletions(-)

diff --git a/drivers/net/phy/microchip_t1s.c b/drivers/net/phy/microchip_t1s.c
index 0110f3357489..fb651cfa3ee0 100644
--- a/drivers/net/phy/microchip_t1s.c
+++ b/drivers/net/phy/microchip_t1s.c
@@ -59,29 +59,45 @@ static const u16 lan867x_revb1_fixup_masks[12] = {
 	0x0600, 0x7F00, 0x2000, 0xFFFF,
 };
 
-/* LAN865x Rev.B0 configuration parameters from AN1760 */
-static const u32 lan865x_revb0_fixup_registers[28] = {
-	0x0091, 0x0081, 0x0043, 0x0044,
-	0x0045, 0x0053, 0x0054, 0x0055,
-	0x0040, 0x0050, 0x00D0, 0x00E9,
-	0x00F5, 0x00F4, 0x00F8, 0x00F9,
+/* LAN865x Rev.B0 configuration parameters from AN1760
+ * As per the Configuration Application Note AN1760 published in the below link,
+ * https://www.microchip.com/en-us/application-notes/an1760
+ * Revision F (DS60001760G - June 2024)
+ */
+static const u32 lan865x_revb0_fixup_registers[17] = {
+	0x00D0, 0x00E0, 0x00E9, 0x00F5,
+	0x00F4, 0x00F8, 0x00F9, 0x0081,
+	0x0091, 0x0043, 0x0044, 0x0045,
+	0x0053, 0x0054, 0x0055, 0x0040,
+	0x0050,
+};
+
+static const u16 lan865x_revb0_fixup_values[17] = {
+	0x3F31, 0xC000, 0x9E50, 0x1CF8,
+	0xC020, 0xB900, 0x4E53, 0x0080,
+	0x9660, 0x00FF, 0xFFFF, 0x0000,
+	0x00FF, 0xFFFF, 0x0000, 0x0002,
+	0x0002,
+};
+
+static const u16 lan865x_revb0_fixup_cfg_regs[2] = {
+	0x0084, 0x008A,
+};
+
+static const u32 lan865x_revb0_sqi_fixup_regs[12] = {
 	0x00B0, 0x00B1, 0x00B2, 0x00B3,
 	0x00B4, 0x00B5, 0x00B6, 0x00B7,
 	0x00B8, 0x00B9, 0x00BA, 0x00BB,
 };
 
-static const u16 lan865x_revb0_fixup_values[28] = {
-	0x9660, 0x00C0, 0x00FF, 0xFFFF,
-	0x0000, 0x00FF, 0xFFFF, 0x0000,
-	0x0002, 0x0002, 0x5F21, 0x9E50,
-	0x1CF8, 0xC020, 0x9B00, 0x4E53,
+static const u16 lan865x_revb0_sqi_fixup_values[12] = {
 	0x0103, 0x0910, 0x1D26, 0x002A,
 	0x0103, 0x070D, 0x1720, 0x0027,
 	0x0509, 0x0E13, 0x1C25, 0x002B,
 };
 
-static const u16 lan865x_revb0_fixup_cfg_regs[5] = {
-	0x0084, 0x008A, 0x00AD, 0x00AE, 0x00AF
+static const u16 lan865x_revb0_sqi_fixup_cfg_regs[3] = {
+	0x00AD, 0x00AE, 0x00AF,
 };
 
 /* Pulled from AN1760 describing 'indirect read'
@@ -121,6 +137,8 @@ static int lan865x_generate_cfg_offsets(struct phy_device *phydev, s8 offsets[])
 		ret = lan865x_revb0_indirect_read(phydev, fixup_regs[i]);
 		if (ret < 0)
 			return ret;
+
+		ret &= 0x1F;
 		if (ret & BIT(4))
 			offsets[i] = ret | 0xE0;
 		else
@@ -163,59 +181,88 @@ static int lan865x_write_cfg_params(struct phy_device *phydev,
 	return 0;
 }
 
-static int lan865x_setup_cfgparam(struct phy_device *phydev)
+static int lan865x_setup_cfgparam(struct phy_device *phydev, s8 offsets[])
 {
 	u16 cfg_results[ARRAY_SIZE(lan865x_revb0_fixup_cfg_regs)];
 	u16 cfg_params[ARRAY_SIZE(lan865x_revb0_fixup_cfg_regs)];
-	s8 offsets[2];
 	int ret;
 
-	ret = lan865x_generate_cfg_offsets(phydev, offsets);
+	ret = lan865x_read_cfg_params(phydev, lan865x_revb0_fixup_cfg_regs,
+				      cfg_params, ARRAY_SIZE(cfg_params));
 	if (ret)
 		return ret;
 
-	ret = lan865x_read_cfg_params(phydev, lan865x_revb0_fixup_cfg_regs,
+	cfg_results[0] = FIELD_PREP(GENMASK(15, 10), (9 + offsets[0]) & 0x3F) |
+			 FIELD_PREP(GENMASK(15, 4), (14 + offsets[0]) & 0x3F) |
+			 0x03;
+	cfg_results[1] = FIELD_PREP(GENMASK(15, 10), (40 + offsets[1]) & 0x3F);
+
+	return lan865x_write_cfg_params(phydev, lan865x_revb0_fixup_cfg_regs,
+					cfg_results, ARRAY_SIZE(cfg_results));
+}
+
+static int lan865x_setup_sqi_cfgparam(struct phy_device *phydev, s8 offsets[])
+{
+	u16 cfg_results[ARRAY_SIZE(lan865x_revb0_sqi_fixup_cfg_regs)];
+	u16 cfg_params[ARRAY_SIZE(lan865x_revb0_sqi_fixup_cfg_regs)];
+	int ret;
+
+	ret = lan865x_read_cfg_params(phydev, lan865x_revb0_sqi_fixup_cfg_regs,
 				      cfg_params, ARRAY_SIZE(cfg_params));
 	if (ret)
 		return ret;
 
-	cfg_results[0] = (cfg_params[0] & 0x000F) |
-			  FIELD_PREP(GENMASK(15, 10), 9 + offsets[0]) |
-			  FIELD_PREP(GENMASK(15, 4), 14 + offsets[0]);
-	cfg_results[1] = (cfg_params[1] & 0x03FF) |
-			  FIELD_PREP(GENMASK(15, 10), 40 + offsets[1]);
-	cfg_results[2] = (cfg_params[2] & 0xC0C0) |
-			  FIELD_PREP(GENMASK(15, 8), 5 + offsets[0]) |
-			  (9 + offsets[0]);
-	cfg_results[3] = (cfg_params[3] & 0xC0C0) |
-			  FIELD_PREP(GENMASK(15, 8), 9 + offsets[0]) |
-			  (14 + offsets[0]);
-	cfg_results[4] = (cfg_params[4] & 0xC0C0) |
-			  FIELD_PREP(GENMASK(15, 8), 17 + offsets[0]) |
-			  (22 + offsets[0]);
+	cfg_results[0] = FIELD_PREP(GENMASK(15, 8), (5 + offsets[0]) & 0x3F) |
+			 ((9 + offsets[0]) & 0x3F);
+	cfg_results[1] = FIELD_PREP(GENMASK(15, 8), (9 + offsets[0]) & 0x3F) |
+			 ((14 + offsets[0]) & 0x3F);
+	cfg_results[2] = FIELD_PREP(GENMASK(15, 8), (17 + offsets[0]) & 0x3F) |
+			 ((22 + offsets[0]) & 0x3F);
 
-	return lan865x_write_cfg_params(phydev, lan865x_revb0_fixup_cfg_regs,
+	return lan865x_write_cfg_params(phydev,
+					lan865x_revb0_sqi_fixup_cfg_regs,
 					cfg_results, ARRAY_SIZE(cfg_results));
 }
 
 static int lan865x_revb0_config_init(struct phy_device *phydev)
 {
+	s8 offsets[2];
 	int ret;
 
 	/* Reference to AN1760
 	 * https://ww1.microchip.com/downloads/aemDocuments/documents/AIS/ProductDocuments/SupportingCollateral/AN-LAN8650-1-Configuration-60001760.pdf
 	 */
+	ret = lan865x_generate_cfg_offsets(phydev, offsets);
+	if (ret)
+		return ret;
+
 	for (int i = 0; i < ARRAY_SIZE(lan865x_revb0_fixup_registers); i++) {
 		ret = phy_write_mmd(phydev, MDIO_MMD_VEND2,
 				    lan865x_revb0_fixup_registers[i],
 				    lan865x_revb0_fixup_values[i]);
 		if (ret)
 			return ret;
+
+		if (i == 1) {
+			ret = lan865x_setup_cfgparam(phydev, offsets);
+			if (ret)
+				return ret;
+		}
 	}
-	/* Function to calculate and write the configuration parameters in the
-	 * 0x0084, 0x008A, 0x00AD, 0x00AE and 0x00AF registers (from AN1760)
-	 */
-	return lan865x_setup_cfgparam(phydev);
+
+	ret = lan865x_setup_sqi_cfgparam(phydev, offsets);
+	if (ret)
+		return ret;
+
+	for (int i = 0; i < ARRAY_SIZE(lan865x_revb0_sqi_fixup_regs); i++) {
+		ret = phy_write_mmd(phydev, MDIO_MMD_VEND2,
+				    lan865x_revb0_sqi_fixup_regs[i],
+				    lan865x_revb0_sqi_fixup_values[i]);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
 }
 
 static int lan867x_revb1_config_init(struct phy_device *phydev)
-- 
2.34.1

[PATCH net-next v2 3/7] net: phy: microchip_t1s: add support for Microchip's LAN865X Rev.B1

[Parthiban Veerasooran]

This patch adds support for LAN8650/1 Rev.B1. As per the latest
configuration note AN1760 released (Revision F (DS60001760G - June 2024))
for Rev.B0 is also applicable for Rev.B1. Refer hardware revisions list
in the latest AN1760 Revision F (DS60001760G - June 2024).
https://www.microchip.com/en-us/application-notes/an1760

Signed-off-by: Parthiban Veerasooran <Parthiban.Veerasooran@microchip.com>
---
 drivers/net/phy/Kconfig         |  4 +--
 drivers/net/phy/microchip_t1s.c | 62 ++++++++++++++++-----------------
 2 files changed, 33 insertions(+), 33 deletions(-)

diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index 01b235b3bb7e..f18defab70cf 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -292,8 +292,8 @@ config MICREL_PHY
 config MICROCHIP_T1S_PHY
 	tristate "Microchip 10BASE-T1S Ethernet PHYs"
 	help
-	  Currently supports the LAN8670/1/2 Rev.B1 and LAN8650/1 Rev.B0 Internal
-	  PHYs.
+	  Currently supports the LAN8670/1/2 Rev.B1 and LAN8650/1 Rev.B0/B1
+	  Internal PHYs.
 
 config MICROCHIP_PHY
 	tristate "Microchip PHYs"
diff --git a/drivers/net/phy/microchip_t1s.c b/drivers/net/phy/microchip_t1s.c
index fb651cfa3ee0..58483a0fa324 100644
--- a/drivers/net/phy/microchip_t1s.c
+++ b/drivers/net/phy/microchip_t1s.c
@@ -4,7 +4,7 @@
  *
  * Support: Microchip Phys:
  *  lan8670/1/2 Rev.B1
- *  lan8650/1 Rev.B0 Internal PHYs
+ *  lan8650/1 Rev.B0/B1 Internal PHYs
  */
 
 #include <linux/kernel.h>
@@ -12,7 +12,8 @@
 #include <linux/phy.h>
 
 #define PHY_ID_LAN867X_REVB1 0x0007C162
-#define PHY_ID_LAN865X_REVB0 0x0007C1B3
+/* Both Rev.B0 and B1 clause 22 PHYID's are same due to B1 chip limitation */
+#define PHY_ID_LAN865X_REVB 0x0007C1B3
 
 #define LAN867X_REG_STS2 0x0019
 
@@ -59,12 +60,12 @@ static const u16 lan867x_revb1_fixup_masks[12] = {
 	0x0600, 0x7F00, 0x2000, 0xFFFF,
 };
 
-/* LAN865x Rev.B0 configuration parameters from AN1760
+/* LAN865x Rev.B0/B1 configuration parameters from AN1760
  * As per the Configuration Application Note AN1760 published in the below link,
  * https://www.microchip.com/en-us/application-notes/an1760
  * Revision F (DS60001760G - June 2024)
  */
-static const u32 lan865x_revb0_fixup_registers[17] = {
+static const u32 lan865x_revb_fixup_registers[17] = {
 	0x00D0, 0x00E0, 0x00E9, 0x00F5,
 	0x00F4, 0x00F8, 0x00F9, 0x0081,
 	0x0091, 0x0043, 0x0044, 0x0045,
@@ -72,7 +73,7 @@ static const u32 lan865x_revb0_fixup_registers[17] = {
 	0x0050,
 };
 
-static const u16 lan865x_revb0_fixup_values[17] = {
+static const u16 lan865x_revb_fixup_values[17] = {
 	0x3F31, 0xC000, 0x9E50, 0x1CF8,
 	0xC020, 0xB900, 0x4E53, 0x0080,
 	0x9660, 0x00FF, 0xFFFF, 0x0000,
@@ -80,23 +81,23 @@ static const u16 lan865x_revb0_fixup_values[17] = {
 	0x0002,
 };
 
-static const u16 lan865x_revb0_fixup_cfg_regs[2] = {
+static const u16 lan865x_revb_fixup_cfg_regs[2] = {
 	0x0084, 0x008A,
 };
 
-static const u32 lan865x_revb0_sqi_fixup_regs[12] = {
+static const u32 lan865x_revb_sqi_fixup_regs[12] = {
 	0x00B0, 0x00B1, 0x00B2, 0x00B3,
 	0x00B4, 0x00B5, 0x00B6, 0x00B7,
 	0x00B8, 0x00B9, 0x00BA, 0x00BB,
 };
 
-static const u16 lan865x_revb0_sqi_fixup_values[12] = {
+static const u16 lan865x_revb_sqi_fixup_values[12] = {
 	0x0103, 0x0910, 0x1D26, 0x002A,
 	0x0103, 0x070D, 0x1720, 0x0027,
 	0x0509, 0x0E13, 0x1C25, 0x002B,
 };
 
-static const u16 lan865x_revb0_sqi_fixup_cfg_regs[3] = {
+static const u16 lan865x_revb_sqi_fixup_cfg_regs[3] = {
 	0x00AD, 0x00AE, 0x00AF,
 };
 
@@ -108,7 +109,7 @@ static const u16 lan865x_revb0_sqi_fixup_cfg_regs[3] = {
  *
  * 0x4 refers to memory map selector 4, which maps to MDIO_MMD_VEND2
  */
-static int lan865x_revb0_indirect_read(struct phy_device *phydev, u16 addr)
+static int lan865x_revb_indirect_read(struct phy_device *phydev, u16 addr)
 {
 	int ret;
 
@@ -134,7 +135,7 @@ static int lan865x_generate_cfg_offsets(struct phy_device *phydev, s8 offsets[])
 	int ret;
 
 	for (int i = 0; i < ARRAY_SIZE(fixup_regs); i++) {
-		ret = lan865x_revb0_indirect_read(phydev, fixup_regs[i]);
+		ret = lan865x_revb_indirect_read(phydev, fixup_regs[i]);
 		if (ret < 0)
 			return ret;
 
@@ -183,11 +184,11 @@ static int lan865x_write_cfg_params(struct phy_device *phydev,
 
 static int lan865x_setup_cfgparam(struct phy_device *phydev, s8 offsets[])
 {
-	u16 cfg_results[ARRAY_SIZE(lan865x_revb0_fixup_cfg_regs)];
-	u16 cfg_params[ARRAY_SIZE(lan865x_revb0_fixup_cfg_regs)];
+	u16 cfg_results[ARRAY_SIZE(lan865x_revb_fixup_cfg_regs)];
+	u16 cfg_params[ARRAY_SIZE(lan865x_revb_fixup_cfg_regs)];
 	int ret;
 
-	ret = lan865x_read_cfg_params(phydev, lan865x_revb0_fixup_cfg_regs,
+	ret = lan865x_read_cfg_params(phydev, lan865x_revb_fixup_cfg_regs,
 				      cfg_params, ARRAY_SIZE(cfg_params));
 	if (ret)
 		return ret;
@@ -197,17 +198,17 @@ static int lan865x_setup_cfgparam(struct phy_device *phydev, s8 offsets[])
 			 0x03;
 	cfg_results[1] = FIELD_PREP(GENMASK(15, 10), (40 + offsets[1]) & 0x3F);
 
-	return lan865x_write_cfg_params(phydev, lan865x_revb0_fixup_cfg_regs,
+	return lan865x_write_cfg_params(phydev, lan865x_revb_fixup_cfg_regs,
 					cfg_results, ARRAY_SIZE(cfg_results));
 }
 
 static int lan865x_setup_sqi_cfgparam(struct phy_device *phydev, s8 offsets[])
 {
-	u16 cfg_results[ARRAY_SIZE(lan865x_revb0_sqi_fixup_cfg_regs)];
-	u16 cfg_params[ARRAY_SIZE(lan865x_revb0_sqi_fixup_cfg_regs)];
+	u16 cfg_results[ARRAY_SIZE(lan865x_revb_sqi_fixup_cfg_regs)];
+	u16 cfg_params[ARRAY_SIZE(lan865x_revb_sqi_fixup_cfg_regs)];
 	int ret;
 
-	ret = lan865x_read_cfg_params(phydev, lan865x_revb0_sqi_fixup_cfg_regs,
+	ret = lan865x_read_cfg_params(phydev, lan865x_revb_sqi_fixup_cfg_regs,
 				      cfg_params, ARRAY_SIZE(cfg_params));
 	if (ret)
 		return ret;
@@ -219,12 +220,11 @@ static int lan865x_setup_sqi_cfgparam(struct phy_device *phydev, s8 offsets[])
 	cfg_results[2] = FIELD_PREP(GENMASK(15, 8), (17 + offsets[0]) & 0x3F) |
 			 ((22 + offsets[0]) & 0x3F);
 
-	return lan865x_write_cfg_params(phydev,
-					lan865x_revb0_sqi_fixup_cfg_regs,
+	return lan865x_write_cfg_params(phydev, lan865x_revb_sqi_fixup_cfg_regs,
 					cfg_results, ARRAY_SIZE(cfg_results));
 }
 
-static int lan865x_revb0_config_init(struct phy_device *phydev)
+static int lan865x_revb_config_init(struct phy_device *phydev)
 {
 	s8 offsets[2];
 	int ret;
@@ -236,10 +236,10 @@ static int lan865x_revb0_config_init(struct phy_device *phydev)
 	if (ret)
 		return ret;
 
-	for (int i = 0; i < ARRAY_SIZE(lan865x_revb0_fixup_registers); i++) {
+	for (int i = 0; i < ARRAY_SIZE(lan865x_revb_fixup_registers); i++) {
 		ret = phy_write_mmd(phydev, MDIO_MMD_VEND2,
-				    lan865x_revb0_fixup_registers[i],
-				    lan865x_revb0_fixup_values[i]);
+				    lan865x_revb_fixup_registers[i],
+				    lan865x_revb_fixup_values[i]);
 		if (ret)
 			return ret;
 
@@ -254,10 +254,10 @@ static int lan865x_revb0_config_init(struct phy_device *phydev)
 	if (ret)
 		return ret;
 
-	for (int i = 0; i < ARRAY_SIZE(lan865x_revb0_sqi_fixup_regs); i++) {
+	for (int i = 0; i < ARRAY_SIZE(lan865x_revb_sqi_fixup_regs); i++) {
 		ret = phy_write_mmd(phydev, MDIO_MMD_VEND2,
-				    lan865x_revb0_sqi_fixup_regs[i],
-				    lan865x_revb0_sqi_fixup_values[i]);
+				    lan865x_revb_sqi_fixup_regs[i],
+				    lan865x_revb_sqi_fixup_values[i]);
 		if (ret)
 			return ret;
 	}
@@ -332,10 +332,10 @@ static struct phy_driver microchip_t1s_driver[] = {
 		.get_plca_status    = genphy_c45_plca_get_status,
 	},
 	{
-		PHY_ID_MATCH_EXACT(PHY_ID_LAN865X_REVB0),
-		.name               = "LAN865X Rev.B0 Internal Phy",
+		PHY_ID_MATCH_EXACT(PHY_ID_LAN865X_REVB),
+		.name               = "LAN865X Rev.B0/B1 Internal Phy",
 		.features           = PHY_BASIC_T1S_P2MP_FEATURES,
-		.config_init        = lan865x_revb0_config_init,
+		.config_init        = lan865x_revb_config_init,
 		.read_status        = lan86xx_read_status,
 		.get_plca_cfg	    = genphy_c45_plca_get_cfg,
 		.set_plca_cfg	    = genphy_c45_plca_set_cfg,
@@ -347,7 +347,7 @@ module_phy_driver(microchip_t1s_driver);
 
 static struct mdio_device_id __maybe_unused tbl[] = {
 	{ PHY_ID_MATCH_EXACT(PHY_ID_LAN867X_REVB1) },
-	{ PHY_ID_MATCH_EXACT(PHY_ID_LAN865X_REVB0) },
+	{ PHY_ID_MATCH_EXACT(PHY_ID_LAN865X_REVB) },
 	{ }
 };
 
-- 
2.34.1

[PATCH net-next v2 4/7] net: phy: microchip_t1s: move LAN867X reset handling to a new function

[Parthiban Veerasooran]

This patch moves LAN867X reset handling code to a new function called
lan867x_check_reset_complete() which will be useful for the next patch
which also uses the same code to handle the reset functionality.

Signed-off-by: Parthiban Veerasooran <Parthiban.Veerasooran@microchip.com>
---
 drivers/net/phy/microchip_t1s.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/microchip_t1s.c b/drivers/net/phy/microchip_t1s.c
index 58483a0fa324..5d133261dd69 100644
--- a/drivers/net/phy/microchip_t1s.c
+++ b/drivers/net/phy/microchip_t1s.c
@@ -265,7 +265,7 @@ static int lan865x_revb_config_init(struct phy_device *phydev)
 	return 0;
 }
 
-static int lan867x_revb1_config_init(struct phy_device *phydev)
+static int lan867x_check_reset_complete(struct phy_device *phydev)
 {
 	int err;
 
@@ -287,6 +287,17 @@ static int lan867x_revb1_config_init(struct phy_device *phydev)
 		}
 	}
 
+	return 0;
+}
+
+static int lan867x_revb1_config_init(struct phy_device *phydev)
+{
+	int err;
+
+	err = lan867x_check_reset_complete(phydev);
+	if (err)
+		return err;
+
 	/* Reference to AN1699
 	 * https://ww1.microchip.com/downloads/aemDocuments/documents/AIS/ProductDocuments/SupportingCollateral/AN-LAN8670-1-2-config-60001699.pdf
 	 * AN1699 says Read, Modify, Write, but the Write is not required if the
-- 
2.34.1

[PATCH net-next v2 5/7] net: phy: microchip_t1s: add support for Microchip's LAN867X Rev.C1

[Parthiban Veerasooran]

This patch adds support for LAN8670/1/2 Rev.C1 as per the latest
configuration note AN1699 released (Revision E (DS60001699F - June 2024))
https://www.microchip.com/en-us/application-notes/an1699

Signed-off-by: Parthiban Veerasooran <Parthiban.Veerasooran@microchip.com>
---
 drivers/net/phy/Kconfig         |  2 +-
 drivers/net/phy/microchip_t1s.c | 66 ++++++++++++++++++++++++++++++++-
 2 files changed, 66 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index f18defab70cf..04f605606a8a 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -292,7 +292,7 @@ config MICREL_PHY
 config MICROCHIP_T1S_PHY
 	tristate "Microchip 10BASE-T1S Ethernet PHYs"
 	help
-	  Currently supports the LAN8670/1/2 Rev.B1 and LAN8650/1 Rev.B0/B1
+	  Currently supports the LAN8670/1/2 Rev.B1/C1 and LAN8650/1 Rev.B0/B1
 	  Internal PHYs.
 
 config MICROCHIP_PHY
diff --git a/drivers/net/phy/microchip_t1s.c b/drivers/net/phy/microchip_t1s.c
index 5d133261dd69..62f5ce548c6a 100644
--- a/drivers/net/phy/microchip_t1s.c
+++ b/drivers/net/phy/microchip_t1s.c
@@ -3,7 +3,7 @@
  * Driver for Microchip 10BASE-T1S PHYs
  *
  * Support: Microchip Phys:
- *  lan8670/1/2 Rev.B1
+ *  lan8670/1/2 Rev.B1/C1
  *  lan8650/1 Rev.B0/B1 Internal PHYs
  */
 
@@ -12,6 +12,7 @@
 #include <linux/phy.h>
 
 #define PHY_ID_LAN867X_REVB1 0x0007C162
+#define PHY_ID_LAN867X_REVC1 0x0007C164
 /* Both Rev.B0 and B1 clause 22 PHYID's are same due to B1 chip limitation */
 #define PHY_ID_LAN865X_REVB 0x0007C1B3
 
@@ -290,6 +291,58 @@ static int lan867x_check_reset_complete(struct phy_device *phydev)
 	return 0;
 }
 
+static int lan867x_revc1_config_init(struct phy_device *phydev)
+{
+	s8 offsets[2];
+	int ret;
+
+	ret = lan867x_check_reset_complete(phydev);
+	if (ret)
+		return ret;
+
+	ret = lan865x_generate_cfg_offsets(phydev, offsets);
+	if (ret)
+		return ret;
+
+	/* LAN867x Rev.C1 configuration settings are equal to the first 9
+	 * configuration settings and all the sqi fixup settings from LAN865x
+	 * Rev.B0/B1. So the same fixup registers and values from LAN865x
+	 * Rev.B0/B1 are used for LAN867x Rev.C1 to avoid duplication.
+	 * Refer the below links for the comparision.
+	 * https://www.microchip.com/en-us/application-notes/an1760
+	 * Revision F (DS60001760G - June 2024)
+	 * https://www.microchip.com/en-us/application-notes/an1699
+	 * Revision E (DS60001699F - June 2024)
+	 */
+	for (int i = 0; i < 9; i++) {
+		ret = phy_write_mmd(phydev, MDIO_MMD_VEND2,
+				    lan865x_revb_fixup_registers[i],
+				    lan865x_revb_fixup_values[i]);
+		if (ret)
+			return ret;
+
+		if (i == 1) {
+			ret = lan865x_setup_cfgparam(phydev, offsets);
+			if (ret)
+				return ret;
+		}
+	}
+
+	ret = lan865x_setup_sqi_cfgparam(phydev, offsets);
+	if (ret)
+		return ret;
+
+	for (int i = 0; i < ARRAY_SIZE(lan865x_revb_sqi_fixup_regs); i++) {
+		ret = phy_write_mmd(phydev, MDIO_MMD_VEND2,
+				    lan865x_revb_sqi_fixup_regs[i],
+				    lan865x_revb_sqi_fixup_values[i]);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
 static int lan867x_revb1_config_init(struct phy_device *phydev)
 {
 	int err;
@@ -342,6 +395,16 @@ static struct phy_driver microchip_t1s_driver[] = {
 		.set_plca_cfg	    = genphy_c45_plca_set_cfg,
 		.get_plca_status    = genphy_c45_plca_get_status,
 	},
+	{
+		PHY_ID_MATCH_EXACT(PHY_ID_LAN867X_REVC1),
+		.name               = "LAN867X Rev.C1",
+		.features           = PHY_BASIC_T1S_P2MP_FEATURES,
+		.config_init        = lan867x_revc1_config_init,
+		.read_status        = lan86xx_read_status,
+		.get_plca_cfg	    = genphy_c45_plca_get_cfg,
+		.set_plca_cfg	    = genphy_c45_plca_set_cfg,
+		.get_plca_status    = genphy_c45_plca_get_status,
+	},
 	{
 		PHY_ID_MATCH_EXACT(PHY_ID_LAN865X_REVB),
 		.name               = "LAN865X Rev.B0/B1 Internal Phy",
@@ -358,6 +421,7 @@ module_phy_driver(microchip_t1s_driver);
 
 static struct mdio_device_id __maybe_unused tbl[] = {
 	{ PHY_ID_MATCH_EXACT(PHY_ID_LAN867X_REVB1) },
+	{ PHY_ID_MATCH_EXACT(PHY_ID_LAN867X_REVC1) },
 	{ PHY_ID_MATCH_EXACT(PHY_ID_LAN865X_REVB) },
 	{ }
 };
-- 
2.34.1

[PATCH net-next v2 6/7] net: phy: microchip_t1s: add support for Microchip's LAN867X Rev.C2

[Parthiban Veerasooran]

This patch adds support for LAN8670/1/2 Rev.C2 as per the latest
configuration note AN1699 released (Revision E (DS60001699F - June 2024))
for Rev.C1 is also applicable for Rev.C2. Refer hardware revisions list
in the latest AN1699 Revision E (DS60001699F - June 2024).
https://www.microchip.com/en-us/application-notes/an1699

Signed-off-by: Parthiban Veerasooran <Parthiban.Veerasooran@microchip.com>
---
 drivers/net/phy/Kconfig         |  4 ++--
 drivers/net/phy/microchip_t1s.c | 22 +++++++++++++++++-----
 2 files changed, 19 insertions(+), 7 deletions(-)

diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index 04f605606a8a..ee3ea0b56d48 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -292,8 +292,8 @@ config MICREL_PHY
 config MICROCHIP_T1S_PHY
 	tristate "Microchip 10BASE-T1S Ethernet PHYs"
 	help
-	  Currently supports the LAN8670/1/2 Rev.B1/C1 and LAN8650/1 Rev.B0/B1
-	  Internal PHYs.
+	  Currently supports the LAN8670/1/2 Rev.B1/C1/C2 and
+	  LAN8650/1 Rev.B0/B1 Internal PHYs.
 
 config MICROCHIP_PHY
 	tristate "Microchip PHYs"
diff --git a/drivers/net/phy/microchip_t1s.c b/drivers/net/phy/microchip_t1s.c
index 62f5ce548c6a..bd0c768df0af 100644
--- a/drivers/net/phy/microchip_t1s.c
+++ b/drivers/net/phy/microchip_t1s.c
@@ -3,7 +3,7 @@
  * Driver for Microchip 10BASE-T1S PHYs
  *
  * Support: Microchip Phys:
- *  lan8670/1/2 Rev.B1/C1
+ *  lan8670/1/2 Rev.B1/C1/C2
  *  lan8650/1 Rev.B0/B1 Internal PHYs
  */
 
@@ -13,6 +13,7 @@
 
 #define PHY_ID_LAN867X_REVB1 0x0007C162
 #define PHY_ID_LAN867X_REVC1 0x0007C164
+#define PHY_ID_LAN867X_REVC2 0x0007C165
 /* Both Rev.B0 and B1 clause 22 PHYID's are same due to B1 chip limitation */
 #define PHY_ID_LAN865X_REVB 0x0007C1B3
 
@@ -291,7 +292,7 @@ static int lan867x_check_reset_complete(struct phy_device *phydev)
 	return 0;
 }
 
-static int lan867x_revc1_config_init(struct phy_device *phydev)
+static int lan867x_revc_config_init(struct phy_device *phydev)
 {
 	s8 offsets[2];
 	int ret;
@@ -304,10 +305,10 @@ static int lan867x_revc1_config_init(struct phy_device *phydev)
 	if (ret)
 		return ret;
 
-	/* LAN867x Rev.C1 configuration settings are equal to the first 9
+	/* LAN867x Rev.C1/C2 configuration settings are equal to the first 9
 	 * configuration settings and all the sqi fixup settings from LAN865x
 	 * Rev.B0/B1. So the same fixup registers and values from LAN865x
-	 * Rev.B0/B1 are used for LAN867x Rev.C1 to avoid duplication.
+	 * Rev.B0/B1 are used for LAN867x Rev.C1/C2 to avoid duplication.
 	 * Refer the below links for the comparision.
 	 * https://www.microchip.com/en-us/application-notes/an1760
 	 * Revision F (DS60001760G - June 2024)
@@ -399,7 +400,17 @@ static struct phy_driver microchip_t1s_driver[] = {
 		PHY_ID_MATCH_EXACT(PHY_ID_LAN867X_REVC1),
 		.name               = "LAN867X Rev.C1",
 		.features           = PHY_BASIC_T1S_P2MP_FEATURES,
-		.config_init        = lan867x_revc1_config_init,
+		.config_init        = lan867x_revc_config_init,
+		.read_status        = lan86xx_read_status,
+		.get_plca_cfg	    = genphy_c45_plca_get_cfg,
+		.set_plca_cfg	    = genphy_c45_plca_set_cfg,
+		.get_plca_status    = genphy_c45_plca_get_status,
+	},
+	{
+		PHY_ID_MATCH_EXACT(PHY_ID_LAN867X_REVC2),
+		.name               = "LAN867X Rev.C2",
+		.features           = PHY_BASIC_T1S_P2MP_FEATURES,
+		.config_init        = lan867x_revc_config_init,
 		.read_status        = lan86xx_read_status,
 		.get_plca_cfg	    = genphy_c45_plca_get_cfg,
 		.set_plca_cfg	    = genphy_c45_plca_set_cfg,
@@ -422,6 +433,7 @@ module_phy_driver(microchip_t1s_driver);
 static struct mdio_device_id __maybe_unused tbl[] = {
 	{ PHY_ID_MATCH_EXACT(PHY_ID_LAN867X_REVB1) },
 	{ PHY_ID_MATCH_EXACT(PHY_ID_LAN867X_REVC1) },
+	{ PHY_ID_MATCH_EXACT(PHY_ID_LAN867X_REVC2) },
 	{ PHY_ID_MATCH_EXACT(PHY_ID_LAN865X_REVB) },
 	{ }
 };
-- 
2.34.1

[PATCH net-next v2 7/7] net: phy: microchip_t1s: configure collision detection based on PLCA mode

[Parthiban Veerasooran]

As per LAN8650/1 Rev.B0/B1 AN1760 (Revision F (DS60001760G - June 2024))
and LAN8670/1/2 Rev.C1/C2 AN1699 (Revision E (DS60001699F - June 2024)),
under normal operation, the device should be operated in PLCA mode.
Disabling collision detection is recommended to allow the device to
operate in noisy environments or when reflections and other inherent
transmission line distortion cause poor signal quality. Collision
detection must be re-enabled if the device is configured to operate in
CSMA/CD mode.

Signed-off-by: Parthiban Veerasooran <Parthiban.Veerasooran@microchip.com>
---
 drivers/net/phy/microchip_t1s.c | 42 ++++++++++++++++++++++++++++++---
 1 file changed, 39 insertions(+), 3 deletions(-)

diff --git a/drivers/net/phy/microchip_t1s.c b/drivers/net/phy/microchip_t1s.c
index bd0c768df0af..a0565508d7d2 100644
--- a/drivers/net/phy/microchip_t1s.c
+++ b/drivers/net/phy/microchip_t1s.c
@@ -26,6 +26,12 @@
 #define LAN865X_REG_CFGPARAM_CTRL 0x00DA
 #define LAN865X_REG_STS2 0x0019
 
+/* Collision Detector Control 0 Register */
+#define LAN86XX_REG_COL_DET_CTRL0	0x0087
+#define COL_DET_CTRL0_ENABLE_BIT_MASK	BIT(15)
+#define COL_DET_ENABLE			BIT(15)
+#define COL_DET_DISABLE			0x0000
+
 #define LAN865X_CFGPARAM_READ_ENABLE BIT(1)
 
 /* The arrays below are pulled from the following table from AN1699
@@ -370,6 +376,36 @@ static int lan867x_revb1_config_init(struct phy_device *phydev)
 	return 0;
 }
 
+/* As per LAN8650/1 Rev.B0/B1 AN1760 (Revision F (DS60001760G - June 2024)) and
+ * LAN8670/1/2 Rev.C1/C2 AN1699 (Revision E (DS60001699F - June 2024)), under
+ * normal operation, the device should be operated in PLCA mode. Disabling
+ * collision detection is recommended to allow the device to operate in noisy
+ * environments or when reflections and other inherent transmission line
+ * distortion cause poor signal quality. Collision detection must be re-enabled
+ * if the device is configured to operate in CSMA/CD mode.
+ *
+ * AN1760: https://www.microchip.com/en-us/application-notes/an1760
+ * AN1699: https://www.microchip.com/en-us/application-notes/an1699
+ */
+static int lan86xx_plca_set_cfg(struct phy_device *phydev,
+				const struct phy_plca_cfg *plca_cfg)
+{
+	int ret;
+
+	ret = genphy_c45_plca_set_cfg(phydev, plca_cfg);
+	if (ret)
+		return ret;
+
+	if (plca_cfg->enabled)
+		return phy_modify_mmd(phydev, MDIO_MMD_VEND2,
+				      LAN86XX_REG_COL_DET_CTRL0,
+				      COL_DET_CTRL0_ENABLE_BIT_MASK,
+				      COL_DET_DISABLE);
+
+	return phy_modify_mmd(phydev, MDIO_MMD_VEND2, LAN86XX_REG_COL_DET_CTRL0,
+			      COL_DET_CTRL0_ENABLE_BIT_MASK, COL_DET_ENABLE);
+}
+
 static int lan86xx_read_status(struct phy_device *phydev)
 {
 	/* The phy has some limitations, namely:
@@ -403,7 +439,7 @@ static struct phy_driver microchip_t1s_driver[] = {
 		.config_init        = lan867x_revc_config_init,
 		.read_status        = lan86xx_read_status,
 		.get_plca_cfg	    = genphy_c45_plca_get_cfg,
-		.set_plca_cfg	    = genphy_c45_plca_set_cfg,
+		.set_plca_cfg	    = lan86xx_plca_set_cfg,
 		.get_plca_status    = genphy_c45_plca_get_status,
 	},
 	{
@@ -413,7 +449,7 @@ static struct phy_driver microchip_t1s_driver[] = {
 		.config_init        = lan867x_revc_config_init,
 		.read_status        = lan86xx_read_status,
 		.get_plca_cfg	    = genphy_c45_plca_get_cfg,
-		.set_plca_cfg	    = genphy_c45_plca_set_cfg,
+		.set_plca_cfg	    = lan86xx_plca_set_cfg,
 		.get_plca_status    = genphy_c45_plca_get_status,
 	},
 	{
@@ -423,7 +459,7 @@ static struct phy_driver microchip_t1s_driver[] = {
 		.config_init        = lan865x_revb_config_init,
 		.read_status        = lan86xx_read_status,
 		.get_plca_cfg	    = genphy_c45_plca_get_cfg,
-		.set_plca_cfg	    = genphy_c45_plca_set_cfg,
+		.set_plca_cfg	    = lan86xx_plca_set_cfg,
 		.get_plca_status    = genphy_c45_plca_get_status,
 	},
 };
-- 
2.34.1

Re: [PATCH net-next v2 0/7] microchip_t1s: Update on Microchip 10BASE-T1S PHY driver

[Horatiu Vultur]

The 09/02/2024 20:04, Parthiban Veerasooran wrote:

Hi Parthiban,

> This patch series contain the below updates,
> 
> v1:

You usually use v1, v2, v3 to say what is the difference between series.
But in your case you already had v1 already in first patch series which
it is strange to be used. I think you wanted to just describe the
features of this patch series.
So, my suggestion is to drop v1 because those are the features not
changes between version, and put ':' instead of ',' after updates.

Also almost all your patches in this series start with 'This patch',
please change this to imperative mode:
https://www.kernel.org/doc/html/v4.10/process/submitting-patches.html

> - Restructured lan865x_write_cfg_params() and lan865x_read_cfg_params()
>   functions arguments to more generic.
> - Updated new/improved initial settings of LAN865X Rev.B0 from latest
>   AN1760.
> - Added support for LAN865X Rev.B1 from latest AN1760.
> - Moved LAN867X reset handling to a new function for flexibility.
> - Added support for LAN867X Rev.C1/C2 from latest AN1699.
> - Disabled/enabled collision detection based on PLCA setting.
> 
> v2:
> - Fixed indexing issue in the configuration parameter setup.
> 
> Parthiban Veerasooran (7):
>   net: phy: microchip_t1s: restructure cfg read/write functions
>     arguments
>   net: phy: microchip_t1s: update new initial settings for LAN865X
>     Rev.B0
>   net: phy: microchip_t1s: add support for Microchip's LAN865X Rev.B1
>   net: phy: microchip_t1s: move LAN867X reset handling to a new function
>   net: phy: microchip_t1s: add support for Microchip's LAN867X Rev.C1
>   net: phy: microchip_t1s: add support for Microchip's LAN867X Rev.C2
>   net: phy: microchip_t1s: configure collision detection based on PLCA
>     mode
> 
>  drivers/net/phy/Kconfig         |   4 +-
>  drivers/net/phy/microchip_t1s.c | 299 +++++++++++++++++++++++++-------
>  2 files changed, 239 insertions(+), 64 deletions(-)
> 
> 
> base-commit: 221f9cce949ac8042f65b71ed1fde13b99073256
> -- 
> 2.34.1
> 

-- 
/Horatiu

Re: [PATCH net-next v2 2/7] net: phy: microchip_t1s: update new initial settings for LAN865X Rev.B0

[Horatiu Vultur]

The 09/02/2024 20:04, Parthiban Veerasooran wrote:
> This patch configures the new/improved initial settings from the latest
> configuration application note AN1760 released for LAN8650/1 Rev.B0
> Revision F (DS60001760G - June 2024).
> https://www.microchip.com/en-us/application-notes/an1760
> 
> Signed-off-by: Parthiban Veerasooran <Parthiban.Veerasooran@microchip.com>
> ---
>  drivers/net/phy/microchip_t1s.c | 119 ++++++++++++++++++++++----------
>  1 file changed, 83 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/net/phy/microchip_t1s.c b/drivers/net/phy/microchip_t1s.c
> index 0110f3357489..fb651cfa3ee0 100644
> --- a/drivers/net/phy/microchip_t1s.c
> +++ b/drivers/net/phy/microchip_t1s.c
> @@ -59,29 +59,45 @@ static const u16 lan867x_revb1_fixup_masks[12] = {
>  	0x0600, 0x7F00, 0x2000, 0xFFFF,
>  };
>  
> -/* LAN865x Rev.B0 configuration parameters from AN1760 */
> -static const u32 lan865x_revb0_fixup_registers[28] = {
> -	0x0091, 0x0081, 0x0043, 0x0044,
> -	0x0045, 0x0053, 0x0054, 0x0055,
> -	0x0040, 0x0050, 0x00D0, 0x00E9,
> -	0x00F5, 0x00F4, 0x00F8, 0x00F9,
> +/* LAN865x Rev.B0 configuration parameters from AN1760
> + * As per the Configuration Application Note AN1760 published in the below link,
> + * https://www.microchip.com/en-us/application-notes/an1760
> + * Revision F (DS60001760G - June 2024)
> + */
> +static const u32 lan865x_revb0_fixup_registers[17] = {
> +	0x00D0, 0x00E0, 0x00E9, 0x00F5,
> +	0x00F4, 0x00F8, 0x00F9, 0x0081,
> +	0x0091, 0x0043, 0x0044, 0x0045,
> +	0x0053, 0x0054, 0x0055, 0x0040,
> +	0x0050,
> +};
> +
> +static const u16 lan865x_revb0_fixup_values[17] = {
> +	0x3F31, 0xC000, 0x9E50, 0x1CF8,
> +	0xC020, 0xB900, 0x4E53, 0x0080,
> +	0x9660, 0x00FF, 0xFFFF, 0x0000,
> +	0x00FF, 0xFFFF, 0x0000, 0x0002,
> +	0x0002,
> +};
> +
> +static const u16 lan865x_revb0_fixup_cfg_regs[2] = {
> +	0x0084, 0x008A,
> +};
> +
> +static const u32 lan865x_revb0_sqi_fixup_regs[12] = {
>  	0x00B0, 0x00B1, 0x00B2, 0x00B3,
>  	0x00B4, 0x00B5, 0x00B6, 0x00B7,
>  	0x00B8, 0x00B9, 0x00BA, 0x00BB,
>  };
>  
> -static const u16 lan865x_revb0_fixup_values[28] = {
> -	0x9660, 0x00C0, 0x00FF, 0xFFFF,
> -	0x0000, 0x00FF, 0xFFFF, 0x0000,
> -	0x0002, 0x0002, 0x5F21, 0x9E50,
> -	0x1CF8, 0xC020, 0x9B00, 0x4E53,
> +static const u16 lan865x_revb0_sqi_fixup_values[12] = {
>  	0x0103, 0x0910, 0x1D26, 0x002A,
>  	0x0103, 0x070D, 0x1720, 0x0027,
>  	0x0509, 0x0E13, 0x1C25, 0x002B,
>  };
>  
> -static const u16 lan865x_revb0_fixup_cfg_regs[5] = {
> -	0x0084, 0x008A, 0x00AD, 0x00AE, 0x00AF
> +static const u16 lan865x_revb0_sqi_fixup_cfg_regs[3] = {
> +	0x00AD, 0x00AE, 0x00AF,
>  };
>  
>  /* Pulled from AN1760 describing 'indirect read'
> @@ -121,6 +137,8 @@ static int lan865x_generate_cfg_offsets(struct phy_device *phydev, s8 offsets[])
>  		ret = lan865x_revb0_indirect_read(phydev, fixup_regs[i]);
>  		if (ret < 0)
>  			return ret;
> +
> +		ret &= 0x1F;

Is this diff supposed to be part of this patch?
Also you can use GENMASK here.

>  		if (ret & BIT(4))
>  			offsets[i] = ret | 0xE0;
>  		else
> @@ -163,59 +181,88 @@ static int lan865x_write_cfg_params(struct phy_device *phydev,
>  	return 0;
>  }
>  
> -static int lan865x_setup_cfgparam(struct phy_device *phydev)
> +static int lan865x_setup_cfgparam(struct phy_device *phydev, s8 offsets[])
>  {
>  	u16 cfg_results[ARRAY_SIZE(lan865x_revb0_fixup_cfg_regs)];
>  	u16 cfg_params[ARRAY_SIZE(lan865x_revb0_fixup_cfg_regs)];
> -	s8 offsets[2];
>  	int ret;
>  
> -	ret = lan865x_generate_cfg_offsets(phydev, offsets);
> +	ret = lan865x_read_cfg_params(phydev, lan865x_revb0_fixup_cfg_regs,
> +				      cfg_params, ARRAY_SIZE(cfg_params));
>  	if (ret)
>  		return ret;
>  
> -	ret = lan865x_read_cfg_params(phydev, lan865x_revb0_fixup_cfg_regs,
> +	cfg_results[0] = FIELD_PREP(GENMASK(15, 10), (9 + offsets[0]) & 0x3F) |
> +			 FIELD_PREP(GENMASK(15, 4), (14 + offsets[0]) & 0x3F) |
> +			 0x03;
> +	cfg_results[1] = FIELD_PREP(GENMASK(15, 10), (40 + offsets[1]) & 0x3F);
> +
> +	return lan865x_write_cfg_params(phydev, lan865x_revb0_fixup_cfg_regs,
> +					cfg_results, ARRAY_SIZE(cfg_results));
> +}
> +
> +static int lan865x_setup_sqi_cfgparam(struct phy_device *phydev, s8 offsets[])
> +{
> +	u16 cfg_results[ARRAY_SIZE(lan865x_revb0_sqi_fixup_cfg_regs)];
> +	u16 cfg_params[ARRAY_SIZE(lan865x_revb0_sqi_fixup_cfg_regs)];
> +	int ret;
> +
> +	ret = lan865x_read_cfg_params(phydev, lan865x_revb0_sqi_fixup_cfg_regs,
>  				      cfg_params, ARRAY_SIZE(cfg_params));
>  	if (ret)
>  		return ret;
>  
> -	cfg_results[0] = (cfg_params[0] & 0x000F) |
> -			  FIELD_PREP(GENMASK(15, 10), 9 + offsets[0]) |
> -			  FIELD_PREP(GENMASK(15, 4), 14 + offsets[0]);
> -	cfg_results[1] = (cfg_params[1] & 0x03FF) |
> -			  FIELD_PREP(GENMASK(15, 10), 40 + offsets[1]);
> -	cfg_results[2] = (cfg_params[2] & 0xC0C0) |
> -			  FIELD_PREP(GENMASK(15, 8), 5 + offsets[0]) |
> -			  (9 + offsets[0]);
> -	cfg_results[3] = (cfg_params[3] & 0xC0C0) |
> -			  FIELD_PREP(GENMASK(15, 8), 9 + offsets[0]) |
> -			  (14 + offsets[0]);
> -	cfg_results[4] = (cfg_params[4] & 0xC0C0) |
> -			  FIELD_PREP(GENMASK(15, 8), 17 + offsets[0]) |
> -			  (22 + offsets[0]);
> +	cfg_results[0] = FIELD_PREP(GENMASK(15, 8), (5 + offsets[0]) & 0x3F) |
> +			 ((9 + offsets[0]) & 0x3F);
> +	cfg_results[1] = FIELD_PREP(GENMASK(15, 8), (9 + offsets[0]) & 0x3F) |
> +			 ((14 + offsets[0]) & 0x3F);
> +	cfg_results[2] = FIELD_PREP(GENMASK(15, 8), (17 + offsets[0]) & 0x3F) |
> +			 ((22 + offsets[0]) & 0x3F);
>  
> -	return lan865x_write_cfg_params(phydev, lan865x_revb0_fixup_cfg_regs,
> +	return lan865x_write_cfg_params(phydev,
> +					lan865x_revb0_sqi_fixup_cfg_regs,
>  					cfg_results, ARRAY_SIZE(cfg_results));
>  }
>  
>  static int lan865x_revb0_config_init(struct phy_device *phydev)
>  {
> +	s8 offsets[2];
>  	int ret;
>  
>  	/* Reference to AN1760
>  	 * https://ww1.microchip.com/downloads/aemDocuments/documents/AIS/ProductDocuments/SupportingCollateral/AN-LAN8650-1-Configuration-60001760.pdf
>  	 */
> +	ret = lan865x_generate_cfg_offsets(phydev, offsets);
> +	if (ret)
> +		return ret;
> +
>  	for (int i = 0; i < ARRAY_SIZE(lan865x_revb0_fixup_registers); i++) {
>  		ret = phy_write_mmd(phydev, MDIO_MMD_VEND2,
>  				    lan865x_revb0_fixup_registers[i],
>  				    lan865x_revb0_fixup_values[i]);
>  		if (ret)
>  			return ret;
> +
> +		if (i == 1) {
> +			ret = lan865x_setup_cfgparam(phydev, offsets);
> +			if (ret)
> +				return ret;
> +		}
>  	}
> -	/* Function to calculate and write the configuration parameters in the
> -	 * 0x0084, 0x008A, 0x00AD, 0x00AE and 0x00AF registers (from AN1760)
> -	 */
> -	return lan865x_setup_cfgparam(phydev);
> +
> +	ret = lan865x_setup_sqi_cfgparam(phydev, offsets);
> +	if (ret)
> +		return ret;
> +
> +	for (int i = 0; i < ARRAY_SIZE(lan865x_revb0_sqi_fixup_regs); i++) {
> +		ret = phy_write_mmd(phydev, MDIO_MMD_VEND2,
> +				    lan865x_revb0_sqi_fixup_regs[i],
> +				    lan865x_revb0_sqi_fixup_values[i]);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
>  }
>  
>  static int lan867x_revb1_config_init(struct phy_device *phydev)
> -- 
> 2.34.1
> 

-- 
/Horatiu

Re: [PATCH net-next v2 3/7] net: phy: microchip_t1s: add support for Microchip's LAN865X Rev.B1

[Horatiu Vultur]

The 09/02/2024 20:04, Parthiban Veerasooran wrote:

Hi Parthiban,

> This patch adds support for LAN8650/1 Rev.B1. As per the latest
> configuration note AN1760 released (Revision F (DS60001760G - June 2024))
> for Rev.B0 is also applicable for Rev.B1. Refer hardware revisions list
> in the latest AN1760 Revision F (DS60001760G - June 2024).
> https://www.microchip.com/en-us/application-notes/an1760

Are all the changes here needed?
Is it not easier to just to update the comments that REV.B0 is
applicable also to REV.B1 and then you are done.

> 
> Signed-off-by: Parthiban Veerasooran <Parthiban.Veerasooran@microchip.com>
> ---
>  drivers/net/phy/Kconfig         |  4 +--
>  drivers/net/phy/microchip_t1s.c | 62 ++++++++++++++++-----------------
>  2 files changed, 33 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
> index 01b235b3bb7e..f18defab70cf 100644
> --- a/drivers/net/phy/Kconfig
> +++ b/drivers/net/phy/Kconfig
> @@ -292,8 +292,8 @@ config MICREL_PHY
>  config MICROCHIP_T1S_PHY
>  	tristate "Microchip 10BASE-T1S Ethernet PHYs"
>  	help
> -	  Currently supports the LAN8670/1/2 Rev.B1 and LAN8650/1 Rev.B0 Internal
> -	  PHYs.
> +	  Currently supports the LAN8670/1/2 Rev.B1 and LAN8650/1 Rev.B0/B1
> +	  Internal PHYs.
>  
>  config MICROCHIP_PHY
>  	tristate "Microchip PHYs"
> diff --git a/drivers/net/phy/microchip_t1s.c b/drivers/net/phy/microchip_t1s.c
> index fb651cfa3ee0..58483a0fa324 100644
> --- a/drivers/net/phy/microchip_t1s.c
> +++ b/drivers/net/phy/microchip_t1s.c
> @@ -4,7 +4,7 @@
>   *
>   * Support: Microchip Phys:
>   *  lan8670/1/2 Rev.B1
> - *  lan8650/1 Rev.B0 Internal PHYs
> + *  lan8650/1 Rev.B0/B1 Internal PHYs
>   */
>  
>  #include <linux/kernel.h>
> @@ -12,7 +12,8 @@
>  #include <linux/phy.h>
>  
>  #define PHY_ID_LAN867X_REVB1 0x0007C162
> -#define PHY_ID_LAN865X_REVB0 0x0007C1B3
> +/* Both Rev.B0 and B1 clause 22 PHYID's are same due to B1 chip limitation */
> +#define PHY_ID_LAN865X_REVB 0x0007C1B3
>  
>  #define LAN867X_REG_STS2 0x0019
>  
> @@ -59,12 +60,12 @@ static const u16 lan867x_revb1_fixup_masks[12] = {
>  	0x0600, 0x7F00, 0x2000, 0xFFFF,
>  };
>  
> -/* LAN865x Rev.B0 configuration parameters from AN1760
> +/* LAN865x Rev.B0/B1 configuration parameters from AN1760
>   * As per the Configuration Application Note AN1760 published in the below link,
>   * https://www.microchip.com/en-us/application-notes/an1760
>   * Revision F (DS60001760G - June 2024)
>   */
> -static const u32 lan865x_revb0_fixup_registers[17] = {
> +static const u32 lan865x_revb_fixup_registers[17] = {
>  	0x00D0, 0x00E0, 0x00E9, 0x00F5,
>  	0x00F4, 0x00F8, 0x00F9, 0x0081,
>  	0x0091, 0x0043, 0x0044, 0x0045,
> @@ -72,7 +73,7 @@ static const u32 lan865x_revb0_fixup_registers[17] = {
>  	0x0050,
>  };
>  
> -static const u16 lan865x_revb0_fixup_values[17] = {
> +static const u16 lan865x_revb_fixup_values[17] = {
>  	0x3F31, 0xC000, 0x9E50, 0x1CF8,
>  	0xC020, 0xB900, 0x4E53, 0x0080,
>  	0x9660, 0x00FF, 0xFFFF, 0x0000,
> @@ -80,23 +81,23 @@ static const u16 lan865x_revb0_fixup_values[17] = {
>  	0x0002,
>  };
>  
> -static const u16 lan865x_revb0_fixup_cfg_regs[2] = {
> +static const u16 lan865x_revb_fixup_cfg_regs[2] = {
>  	0x0084, 0x008A,
>  };
>  
> -static const u32 lan865x_revb0_sqi_fixup_regs[12] = {
> +static const u32 lan865x_revb_sqi_fixup_regs[12] = {
>  	0x00B0, 0x00B1, 0x00B2, 0x00B3,
>  	0x00B4, 0x00B5, 0x00B6, 0x00B7,
>  	0x00B8, 0x00B9, 0x00BA, 0x00BB,
>  };
>  
> -static const u16 lan865x_revb0_sqi_fixup_values[12] = {
> +static const u16 lan865x_revb_sqi_fixup_values[12] = {
>  	0x0103, 0x0910, 0x1D26, 0x002A,
>  	0x0103, 0x070D, 0x1720, 0x0027,
>  	0x0509, 0x0E13, 0x1C25, 0x002B,
>  };
>  
> -static const u16 lan865x_revb0_sqi_fixup_cfg_regs[3] = {
> +static const u16 lan865x_revb_sqi_fixup_cfg_regs[3] = {
>  	0x00AD, 0x00AE, 0x00AF,
>  };
>  
> @@ -108,7 +109,7 @@ static const u16 lan865x_revb0_sqi_fixup_cfg_regs[3] = {
>   *
>   * 0x4 refers to memory map selector 4, which maps to MDIO_MMD_VEND2
>   */
> -static int lan865x_revb0_indirect_read(struct phy_device *phydev, u16 addr)
> +static int lan865x_revb_indirect_read(struct phy_device *phydev, u16 addr)
>  {
>  	int ret;
>  
> @@ -134,7 +135,7 @@ static int lan865x_generate_cfg_offsets(struct phy_device *phydev, s8 offsets[])
>  	int ret;
>  
>  	for (int i = 0; i < ARRAY_SIZE(fixup_regs); i++) {
> -		ret = lan865x_revb0_indirect_read(phydev, fixup_regs[i]);
> +		ret = lan865x_revb_indirect_read(phydev, fixup_regs[i]);
>  		if (ret < 0)
>  			return ret;
>  
> @@ -183,11 +184,11 @@ static int lan865x_write_cfg_params(struct phy_device *phydev,
>  
>  static int lan865x_setup_cfgparam(struct phy_device *phydev, s8 offsets[])
>  {
> -	u16 cfg_results[ARRAY_SIZE(lan865x_revb0_fixup_cfg_regs)];
> -	u16 cfg_params[ARRAY_SIZE(lan865x_revb0_fixup_cfg_regs)];
> +	u16 cfg_results[ARRAY_SIZE(lan865x_revb_fixup_cfg_regs)];
> +	u16 cfg_params[ARRAY_SIZE(lan865x_revb_fixup_cfg_regs)];
>  	int ret;
>  
> -	ret = lan865x_read_cfg_params(phydev, lan865x_revb0_fixup_cfg_regs,
> +	ret = lan865x_read_cfg_params(phydev, lan865x_revb_fixup_cfg_regs,
>  				      cfg_params, ARRAY_SIZE(cfg_params));
>  	if (ret)
>  		return ret;
> @@ -197,17 +198,17 @@ static int lan865x_setup_cfgparam(struct phy_device *phydev, s8 offsets[])
>  			 0x03;
>  	cfg_results[1] = FIELD_PREP(GENMASK(15, 10), (40 + offsets[1]) & 0x3F);
>  
> -	return lan865x_write_cfg_params(phydev, lan865x_revb0_fixup_cfg_regs,
> +	return lan865x_write_cfg_params(phydev, lan865x_revb_fixup_cfg_regs,
>  					cfg_results, ARRAY_SIZE(cfg_results));
>  }
>  
>  static int lan865x_setup_sqi_cfgparam(struct phy_device *phydev, s8 offsets[])
>  {
> -	u16 cfg_results[ARRAY_SIZE(lan865x_revb0_sqi_fixup_cfg_regs)];
> -	u16 cfg_params[ARRAY_SIZE(lan865x_revb0_sqi_fixup_cfg_regs)];
> +	u16 cfg_results[ARRAY_SIZE(lan865x_revb_sqi_fixup_cfg_regs)];
> +	u16 cfg_params[ARRAY_SIZE(lan865x_revb_sqi_fixup_cfg_regs)];
>  	int ret;
>  
> -	ret = lan865x_read_cfg_params(phydev, lan865x_revb0_sqi_fixup_cfg_regs,
> +	ret = lan865x_read_cfg_params(phydev, lan865x_revb_sqi_fixup_cfg_regs,
>  				      cfg_params, ARRAY_SIZE(cfg_params));
>  	if (ret)
>  		return ret;
> @@ -219,12 +220,11 @@ static int lan865x_setup_sqi_cfgparam(struct phy_device *phydev, s8 offsets[])
>  	cfg_results[2] = FIELD_PREP(GENMASK(15, 8), (17 + offsets[0]) & 0x3F) |
>  			 ((22 + offsets[0]) & 0x3F);
>  
> -	return lan865x_write_cfg_params(phydev,
> -					lan865x_revb0_sqi_fixup_cfg_regs,
> +	return lan865x_write_cfg_params(phydev, lan865x_revb_sqi_fixup_cfg_regs,
>  					cfg_results, ARRAY_SIZE(cfg_results));
>  }
>  
> -static int lan865x_revb0_config_init(struct phy_device *phydev)
> +static int lan865x_revb_config_init(struct phy_device *phydev)
>  {
>  	s8 offsets[2];
>  	int ret;
> @@ -236,10 +236,10 @@ static int lan865x_revb0_config_init(struct phy_device *phydev)
>  	if (ret)
>  		return ret;
>  
> -	for (int i = 0; i < ARRAY_SIZE(lan865x_revb0_fixup_registers); i++) {
> +	for (int i = 0; i < ARRAY_SIZE(lan865x_revb_fixup_registers); i++) {
>  		ret = phy_write_mmd(phydev, MDIO_MMD_VEND2,
> -				    lan865x_revb0_fixup_registers[i],
> -				    lan865x_revb0_fixup_values[i]);
> +				    lan865x_revb_fixup_registers[i],
> +				    lan865x_revb_fixup_values[i]);
>  		if (ret)
>  			return ret;
>  
> @@ -254,10 +254,10 @@ static int lan865x_revb0_config_init(struct phy_device *phydev)
>  	if (ret)
>  		return ret;
>  
> -	for (int i = 0; i < ARRAY_SIZE(lan865x_revb0_sqi_fixup_regs); i++) {
> +	for (int i = 0; i < ARRAY_SIZE(lan865x_revb_sqi_fixup_regs); i++) {
>  		ret = phy_write_mmd(phydev, MDIO_MMD_VEND2,
> -				    lan865x_revb0_sqi_fixup_regs[i],
> -				    lan865x_revb0_sqi_fixup_values[i]);
> +				    lan865x_revb_sqi_fixup_regs[i],
> +				    lan865x_revb_sqi_fixup_values[i]);
>  		if (ret)
>  			return ret;
>  	}
> @@ -332,10 +332,10 @@ static struct phy_driver microchip_t1s_driver[] = {
>  		.get_plca_status    = genphy_c45_plca_get_status,
>  	},
>  	{
> -		PHY_ID_MATCH_EXACT(PHY_ID_LAN865X_REVB0),
> -		.name               = "LAN865X Rev.B0 Internal Phy",
> +		PHY_ID_MATCH_EXACT(PHY_ID_LAN865X_REVB),
> +		.name               = "LAN865X Rev.B0/B1 Internal Phy",
>  		.features           = PHY_BASIC_T1S_P2MP_FEATURES,
> -		.config_init        = lan865x_revb0_config_init,
> +		.config_init        = lan865x_revb_config_init,
>  		.read_status        = lan86xx_read_status,
>  		.get_plca_cfg	    = genphy_c45_plca_get_cfg,
>  		.set_plca_cfg	    = genphy_c45_plca_set_cfg,
> @@ -347,7 +347,7 @@ module_phy_driver(microchip_t1s_driver);
>  
>  static struct mdio_device_id __maybe_unused tbl[] = {
>  	{ PHY_ID_MATCH_EXACT(PHY_ID_LAN867X_REVB1) },
> -	{ PHY_ID_MATCH_EXACT(PHY_ID_LAN865X_REVB0) },
> +	{ PHY_ID_MATCH_EXACT(PHY_ID_LAN865X_REVB) },
>  	{ }
>  };
>  
> -- 
> 2.34.1
> 

-- 
/Horatiu

Re: [PATCH net-next v2 7/7] net: phy: microchip_t1s: configure collision detection based on PLCA mode

[Horatiu Vultur]

The 09/02/2024 20:04, Parthiban Veerasooran wrote:

Hi Parthiban,

> As per LAN8650/1 Rev.B0/B1 AN1760 (Revision F (DS60001760G - June 2024))
> and LAN8670/1/2 Rev.C1/C2 AN1699 (Revision E (DS60001699F - June 2024)),
> under normal operation, the device should be operated in PLCA mode.
> Disabling collision detection is recommended to allow the device to
> operate in noisy environments or when reflections and other inherent
> transmission line distortion cause poor signal quality. Collision
> detection must be re-enabled if the device is configured to operate in
> CSMA/CD mode.

Is this something that applies only to Microchip PHYs or is something
generic that applies to all the PHYs.

> 
> Signed-off-by: Parthiban Veerasooran <Parthiban.Veerasooran@microchip.com>
> ---
>  drivers/net/phy/microchip_t1s.c | 42 ++++++++++++++++++++++++++++++---
>  1 file changed, 39 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/phy/microchip_t1s.c b/drivers/net/phy/microchip_t1s.c
> index bd0c768df0af..a0565508d7d2 100644
> --- a/drivers/net/phy/microchip_t1s.c
> +++ b/drivers/net/phy/microchip_t1s.c
> @@ -26,6 +26,12 @@
>  #define LAN865X_REG_CFGPARAM_CTRL 0x00DA
>  #define LAN865X_REG_STS2 0x0019
>  
> +/* Collision Detector Control 0 Register */
> +#define LAN86XX_REG_COL_DET_CTRL0	0x0087
> +#define COL_DET_CTRL0_ENABLE_BIT_MASK	BIT(15)
> +#define COL_DET_ENABLE			BIT(15)
> +#define COL_DET_DISABLE			0x0000
> +
>  #define LAN865X_CFGPARAM_READ_ENABLE BIT(1)
>  
>  /* The arrays below are pulled from the following table from AN1699
> @@ -370,6 +376,36 @@ static int lan867x_revb1_config_init(struct phy_device *phydev)
>  	return 0;
>  }
>  
> +/* As per LAN8650/1 Rev.B0/B1 AN1760 (Revision F (DS60001760G - June 2024)) and
> + * LAN8670/1/2 Rev.C1/C2 AN1699 (Revision E (DS60001699F - June 2024)), under
> + * normal operation, the device should be operated in PLCA mode. Disabling
> + * collision detection is recommended to allow the device to operate in noisy
> + * environments or when reflections and other inherent transmission line
> + * distortion cause poor signal quality. Collision detection must be re-enabled
> + * if the device is configured to operate in CSMA/CD mode.
> + *
> + * AN1760: https://www.microchip.com/en-us/application-notes/an1760
> + * AN1699: https://www.microchip.com/en-us/application-notes/an1699
> + */
> +static int lan86xx_plca_set_cfg(struct phy_device *phydev,
> +				const struct phy_plca_cfg *plca_cfg)
> +{
> +	int ret;
> +
> +	ret = genphy_c45_plca_set_cfg(phydev, plca_cfg);
> +	if (ret)
> +		return ret;
> +
> +	if (plca_cfg->enabled)
> +		return phy_modify_mmd(phydev, MDIO_MMD_VEND2,
> +				      LAN86XX_REG_COL_DET_CTRL0,
> +				      COL_DET_CTRL0_ENABLE_BIT_MASK,
> +				      COL_DET_DISABLE);
> +
> +	return phy_modify_mmd(phydev, MDIO_MMD_VEND2, LAN86XX_REG_COL_DET_CTRL0,
> +			      COL_DET_CTRL0_ENABLE_BIT_MASK, COL_DET_ENABLE);
> +}
> +
>  static int lan86xx_read_status(struct phy_device *phydev)
>  {
>  	/* The phy has some limitations, namely:
> @@ -403,7 +439,7 @@ static struct phy_driver microchip_t1s_driver[] = {
>  		.config_init        = lan867x_revc_config_init,
>  		.read_status        = lan86xx_read_status,
>  		.get_plca_cfg	    = genphy_c45_plca_get_cfg,
> -		.set_plca_cfg	    = genphy_c45_plca_set_cfg,
> +		.set_plca_cfg	    = lan86xx_plca_set_cfg,
>  		.get_plca_status    = genphy_c45_plca_get_status,
>  	},
>  	{
> @@ -413,7 +449,7 @@ static struct phy_driver microchip_t1s_driver[] = {
>  		.config_init        = lan867x_revc_config_init,
>  		.read_status        = lan86xx_read_status,
>  		.get_plca_cfg	    = genphy_c45_plca_get_cfg,
> -		.set_plca_cfg	    = genphy_c45_plca_set_cfg,
> +		.set_plca_cfg	    = lan86xx_plca_set_cfg,
>  		.get_plca_status    = genphy_c45_plca_get_status,
>  	},
>  	{
> @@ -423,7 +459,7 @@ static struct phy_driver microchip_t1s_driver[] = {
>  		.config_init        = lan865x_revb_config_init,
>  		.read_status        = lan86xx_read_status,
>  		.get_plca_cfg	    = genphy_c45_plca_get_cfg,
> -		.set_plca_cfg	    = genphy_c45_plca_set_cfg,
> +		.set_plca_cfg	    = lan86xx_plca_set_cfg,
>  		.get_plca_status    = genphy_c45_plca_get_status,
>  	},
>  };
> -- 
> 2.34.1
> 

-- 
/Horatiu

Re: [PATCH net-next v2 5/7] net: phy: microchip_t1s: add support for Microchip's LAN867X Rev.C1

[Simon Horman]

On Mon, Sep 02, 2024 at 08:04:56PM +0530, Parthiban Veerasooran wrote:
> This patch adds support for LAN8670/1/2 Rev.C1 as per the latest
> configuration note AN1699 released (Revision E (DS60001699F - June 2024))
> https://www.microchip.com/en-us/application-notes/an1699
> 
> Signed-off-by: Parthiban Veerasooran <Parthiban.Veerasooran@microchip.com>

...

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

...

> @@ -290,6 +291,58 @@ static int lan867x_check_reset_complete(struct phy_device *phydev)
>  	return 0;
>  }
>  
> +static int lan867x_revc1_config_init(struct phy_device *phydev)
> +{
> +	s8 offsets[2];
> +	int ret;
> +
> +	ret = lan867x_check_reset_complete(phydev);
> +	if (ret)
> +		return ret;
> +
> +	ret = lan865x_generate_cfg_offsets(phydev, offsets);
> +	if (ret)
> +		return ret;
> +
> +	/* LAN867x Rev.C1 configuration settings are equal to the first 9
> +	 * configuration settings and all the sqi fixup settings from LAN865x
> +	 * Rev.B0/B1. So the same fixup registers and values from LAN865x
> +	 * Rev.B0/B1 are used for LAN867x Rev.C1 to avoid duplication.
> +	 * Refer the below links for the comparision.

nit: comparison

     Flagged by checkpatch.pl --codespell

> +	 * https://www.microchip.com/en-us/application-notes/an1760
> +	 * Revision F (DS60001760G - June 2024)
> +	 * https://www.microchip.com/en-us/application-notes/an1699
> +	 * Revision E (DS60001699F - June 2024)
> +	 */

Re: [PATCH net-next v2 2/7] net: phy: microchip_t1s: update new initial settings for LAN865X Rev.B0

[Parthiban.Veerasooran]

Hi Horatiu,

Thanks for reviewing the patches.

On 03/09/24 12:03 pm, Horatiu Vultur wrote:
> The 09/02/2024 20:04, Parthiban Veerasooran wrote:
>> This patch configures the new/improved initial settings from the latest
>> configuration application note AN1760 released for LAN8650/1 Rev.B0
>> Revision F (DS60001760G - June 2024).
>> https://www.microchip.com/en-us/application-notes/an1760
>>
>> Signed-off-by: Parthiban Veerasooran <Parthiban.Veerasooran@microchip.com>
>> ---
>>   drivers/net/phy/microchip_t1s.c | 119 ++++++++++++++++++++++----------
>>   1 file changed, 83 insertions(+), 36 deletions(-)
>>
>> diff --git a/drivers/net/phy/microchip_t1s.c b/drivers/net/phy/microchip_t1s.c
>> index 0110f3357489..fb651cfa3ee0 100644
>> --- a/drivers/net/phy/microchip_t1s.c
>> +++ b/drivers/net/phy/microchip_t1s.c
>> @@ -59,29 +59,45 @@ static const u16 lan867x_revb1_fixup_masks[12] = {
>>   	0x0600, 0x7F00, 0x2000, 0xFFFF,
>>   };
>>   
>> -/* LAN865x Rev.B0 configuration parameters from AN1760 */
>> -static const u32 lan865x_revb0_fixup_registers[28] = {
>> -	0x0091, 0x0081, 0x0043, 0x0044,
>> -	0x0045, 0x0053, 0x0054, 0x0055,
>> -	0x0040, 0x0050, 0x00D0, 0x00E9,
>> -	0x00F5, 0x00F4, 0x00F8, 0x00F9,
>> +/* LAN865x Rev.B0 configuration parameters from AN1760
>> + * As per the Configuration Application Note AN1760 published in the below link,
>> + * https://www.microchip.com/en-us/application-notes/an1760
>> + * Revision F (DS60001760G - June 2024)
>> + */
>> +static const u32 lan865x_revb0_fixup_registers[17] = {
>> +	0x00D0, 0x00E0, 0x00E9, 0x00F5,
>> +	0x00F4, 0x00F8, 0x00F9, 0x0081,
>> +	0x0091, 0x0043, 0x0044, 0x0045,
>> +	0x0053, 0x0054, 0x0055, 0x0040,
>> +	0x0050,
>> +};
>> +
>> +static const u16 lan865x_revb0_fixup_values[17] = {
>> +	0x3F31, 0xC000, 0x9E50, 0x1CF8,
>> +	0xC020, 0xB900, 0x4E53, 0x0080,
>> +	0x9660, 0x00FF, 0xFFFF, 0x0000,
>> +	0x00FF, 0xFFFF, 0x0000, 0x0002,
>> +	0x0002,
>> +};
>> +
>> +static const u16 lan865x_revb0_fixup_cfg_regs[2] = {
>> +	0x0084, 0x008A,
>> +};
>> +
>> +static const u32 lan865x_revb0_sqi_fixup_regs[12] = {
>>   	0x00B0, 0x00B1, 0x00B2, 0x00B3,
>>   	0x00B4, 0x00B5, 0x00B6, 0x00B7,
>>   	0x00B8, 0x00B9, 0x00BA, 0x00BB,
>>   };
>>   
>> -static const u16 lan865x_revb0_fixup_values[28] = {
>> -	0x9660, 0x00C0, 0x00FF, 0xFFFF,
>> -	0x0000, 0x00FF, 0xFFFF, 0x0000,
>> -	0x0002, 0x0002, 0x5F21, 0x9E50,
>> -	0x1CF8, 0xC020, 0x9B00, 0x4E53,
>> +static const u16 lan865x_revb0_sqi_fixup_values[12] = {
>>   	0x0103, 0x0910, 0x1D26, 0x002A,
>>   	0x0103, 0x070D, 0x1720, 0x0027,
>>   	0x0509, 0x0E13, 0x1C25, 0x002B,
>>   };
>>   
>> -static const u16 lan865x_revb0_fixup_cfg_regs[5] = {
>> -	0x0084, 0x008A, 0x00AD, 0x00AE, 0x00AF
>> +static const u16 lan865x_revb0_sqi_fixup_cfg_regs[3] = {
>> +	0x00AD, 0x00AE, 0x00AF,
>>   };
>>   
>>   /* Pulled from AN1760 describing 'indirect read'
>> @@ -121,6 +137,8 @@ static int lan865x_generate_cfg_offsets(struct phy_device *phydev, s8 offsets[])
>>   		ret = lan865x_revb0_indirect_read(phydev, fixup_regs[i]);
>>   		if (ret < 0)
>>   			return ret;
>> +
>> +		ret &= 0x1F;
> 
> Is this diff supposed to be part of this patch?
Yes.
> Also you can use GENMASK here.
Ah ok, it is GENMASK(4, 0) then.

Best regards,
Parthiban V
> 
>>   		if (ret & BIT(4))
>>   			offsets[i] = ret | 0xE0;
>>   		else
>> @@ -163,59 +181,88 @@ static int lan865x_write_cfg_params(struct phy_device *phydev,
>>   	return 0;
>>   }
>>   
>> -static int lan865x_setup_cfgparam(struct phy_device *phydev)
>> +static int lan865x_setup_cfgparam(struct phy_device *phydev, s8 offsets[])
>>   {
>>   	u16 cfg_results[ARRAY_SIZE(lan865x_revb0_fixup_cfg_regs)];
>>   	u16 cfg_params[ARRAY_SIZE(lan865x_revb0_fixup_cfg_regs)];
>> -	s8 offsets[2];
>>   	int ret;
>>   
>> -	ret = lan865x_generate_cfg_offsets(phydev, offsets);
>> +	ret = lan865x_read_cfg_params(phydev, lan865x_revb0_fixup_cfg_regs,
>> +				      cfg_params, ARRAY_SIZE(cfg_params));
>>   	if (ret)
>>   		return ret;
>>   
>> -	ret = lan865x_read_cfg_params(phydev, lan865x_revb0_fixup_cfg_regs,
>> +	cfg_results[0] = FIELD_PREP(GENMASK(15, 10), (9 + offsets[0]) & 0x3F) |
>> +			 FIELD_PREP(GENMASK(15, 4), (14 + offsets[0]) & 0x3F) |
>> +			 0x03;
>> +	cfg_results[1] = FIELD_PREP(GENMASK(15, 10), (40 + offsets[1]) & 0x3F);
>> +
>> +	return lan865x_write_cfg_params(phydev, lan865x_revb0_fixup_cfg_regs,
>> +					cfg_results, ARRAY_SIZE(cfg_results));
>> +}
>> +
>> +static int lan865x_setup_sqi_cfgparam(struct phy_device *phydev, s8 offsets[])
>> +{
>> +	u16 cfg_results[ARRAY_SIZE(lan865x_revb0_sqi_fixup_cfg_regs)];
>> +	u16 cfg_params[ARRAY_SIZE(lan865x_revb0_sqi_fixup_cfg_regs)];
>> +	int ret;
>> +
>> +	ret = lan865x_read_cfg_params(phydev, lan865x_revb0_sqi_fixup_cfg_regs,
>>   				      cfg_params, ARRAY_SIZE(cfg_params));
>>   	if (ret)
>>   		return ret;
>>   
>> -	cfg_results[0] = (cfg_params[0] & 0x000F) |
>> -			  FIELD_PREP(GENMASK(15, 10), 9 + offsets[0]) |
>> -			  FIELD_PREP(GENMASK(15, 4), 14 + offsets[0]);
>> -	cfg_results[1] = (cfg_params[1] & 0x03FF) |
>> -			  FIELD_PREP(GENMASK(15, 10), 40 + offsets[1]);
>> -	cfg_results[2] = (cfg_params[2] & 0xC0C0) |
>> -			  FIELD_PREP(GENMASK(15, 8), 5 + offsets[0]) |
>> -			  (9 + offsets[0]);
>> -	cfg_results[3] = (cfg_params[3] & 0xC0C0) |
>> -			  FIELD_PREP(GENMASK(15, 8), 9 + offsets[0]) |
>> -			  (14 + offsets[0]);
>> -	cfg_results[4] = (cfg_params[4] & 0xC0C0) |
>> -			  FIELD_PREP(GENMASK(15, 8), 17 + offsets[0]) |
>> -			  (22 + offsets[0]);
>> +	cfg_results[0] = FIELD_PREP(GENMASK(15, 8), (5 + offsets[0]) & 0x3F) |
>> +			 ((9 + offsets[0]) & 0x3F);
>> +	cfg_results[1] = FIELD_PREP(GENMASK(15, 8), (9 + offsets[0]) & 0x3F) |
>> +			 ((14 + offsets[0]) & 0x3F);
>> +	cfg_results[2] = FIELD_PREP(GENMASK(15, 8), (17 + offsets[0]) & 0x3F) |
>> +			 ((22 + offsets[0]) & 0x3F);
>>   
>> -	return lan865x_write_cfg_params(phydev, lan865x_revb0_fixup_cfg_regs,
>> +	return lan865x_write_cfg_params(phydev,
>> +					lan865x_revb0_sqi_fixup_cfg_regs,
>>   					cfg_results, ARRAY_SIZE(cfg_results));
>>   }
>>   
>>   static int lan865x_revb0_config_init(struct phy_device *phydev)
>>   {
>> +	s8 offsets[2];
>>   	int ret;
>>   
>>   	/* Reference to AN1760
>>   	 * https://ww1.microchip.com/downloads/aemDocuments/documents/AIS/ProductDocuments/SupportingCollateral/AN-LAN8650-1-Configuration-60001760.pdf
>>   	 */
>> +	ret = lan865x_generate_cfg_offsets(phydev, offsets);
>> +	if (ret)
>> +		return ret;
>> +
>>   	for (int i = 0; i < ARRAY_SIZE(lan865x_revb0_fixup_registers); i++) {
>>   		ret = phy_write_mmd(phydev, MDIO_MMD_VEND2,
>>   				    lan865x_revb0_fixup_registers[i],
>>   				    lan865x_revb0_fixup_values[i]);
>>   		if (ret)
>>   			return ret;
>> +
>> +		if (i == 1) {
>> +			ret = lan865x_setup_cfgparam(phydev, offsets);
>> +			if (ret)
>> +				return ret;
>> +		}
>>   	}
>> -	/* Function to calculate and write the configuration parameters in the
>> -	 * 0x0084, 0x008A, 0x00AD, 0x00AE and 0x00AF registers (from AN1760)
>> -	 */
>> -	return lan865x_setup_cfgparam(phydev);
>> +
>> +	ret = lan865x_setup_sqi_cfgparam(phydev, offsets);
>> +	if (ret)
>> +		return ret;
>> +
>> +	for (int i = 0; i < ARRAY_SIZE(lan865x_revb0_sqi_fixup_regs); i++) {
>> +		ret = phy_write_mmd(phydev, MDIO_MMD_VEND2,
>> +				    lan865x_revb0_sqi_fixup_regs[i],
>> +				    lan865x_revb0_sqi_fixup_values[i]);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>> +	return 0;
>>   }
>>   
>>   static int lan867x_revb1_config_init(struct phy_device *phydev)
>> -- 
>> 2.34.1
>>
> 

Re: [PATCH net-next v2 3/7] net: phy: microchip_t1s: add support for Microchip's LAN865X Rev.B1

[Parthiban.Veerasooran]

Hi Horatiu,

On 03/09/24 12:06 pm, Horatiu Vultur wrote:
> The 09/02/2024 20:04, Parthiban Veerasooran wrote:
> 
> Hi Parthiban,
> 
>> This patch adds support for LAN8650/1 Rev.B1. As per the latest
>> configuration note AN1760 released (Revision F (DS60001760G - June 2024))
>> for Rev.B0 is also applicable for Rev.B1. Refer hardware revisions list
>> in the latest AN1760 Revision F (DS60001760G - June 2024).
>> https://www.microchip.com/en-us/application-notes/an1760
> 
> Are all the changes here needed?
> Is it not easier to just to update the comments that REV.B0 is
> applicable also to REV.B1 and then you are done.
Ah yes, that's a good point. The reason why I renamed all the functions 
to be more generic for both the revisions.

Best regards,
Parthiban V
> 
>>
>> Signed-off-by: Parthiban Veerasooran <Parthiban.Veerasooran@microchip.com>
>> ---
>>   drivers/net/phy/Kconfig         |  4 +--
>>   drivers/net/phy/microchip_t1s.c | 62 ++++++++++++++++-----------------
>>   2 files changed, 33 insertions(+), 33 deletions(-)
>>
>> diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
>> index 01b235b3bb7e..f18defab70cf 100644
>> --- a/drivers/net/phy/Kconfig
>> +++ b/drivers/net/phy/Kconfig
>> @@ -292,8 +292,8 @@ config MICREL_PHY
>>   config MICROCHIP_T1S_PHY
>>   	tristate "Microchip 10BASE-T1S Ethernet PHYs"
>>   	help
>> -	  Currently supports the LAN8670/1/2 Rev.B1 and LAN8650/1 Rev.B0 Internal
>> -	  PHYs.
>> +	  Currently supports the LAN8670/1/2 Rev.B1 and LAN8650/1 Rev.B0/B1
>> +	  Internal PHYs.
>>   
>>   config MICROCHIP_PHY
>>   	tristate "Microchip PHYs"
>> diff --git a/drivers/net/phy/microchip_t1s.c b/drivers/net/phy/microchip_t1s.c
>> index fb651cfa3ee0..58483a0fa324 100644
>> --- a/drivers/net/phy/microchip_t1s.c
>> +++ b/drivers/net/phy/microchip_t1s.c
>> @@ -4,7 +4,7 @@
>>    *
>>    * Support: Microchip Phys:
>>    *  lan8670/1/2 Rev.B1
>> - *  lan8650/1 Rev.B0 Internal PHYs
>> + *  lan8650/1 Rev.B0/B1 Internal PHYs
>>    */
>>   
>>   #include <linux/kernel.h>
>> @@ -12,7 +12,8 @@
>>   #include <linux/phy.h>
>>   
>>   #define PHY_ID_LAN867X_REVB1 0x0007C162
>> -#define PHY_ID_LAN865X_REVB0 0x0007C1B3
>> +/* Both Rev.B0 and B1 clause 22 PHYID's are same due to B1 chip limitation */
>> +#define PHY_ID_LAN865X_REVB 0x0007C1B3
>>   
>>   #define LAN867X_REG_STS2 0x0019
>>   
>> @@ -59,12 +60,12 @@ static const u16 lan867x_revb1_fixup_masks[12] = {
>>   	0x0600, 0x7F00, 0x2000, 0xFFFF,
>>   };
>>   
>> -/* LAN865x Rev.B0 configuration parameters from AN1760
>> +/* LAN865x Rev.B0/B1 configuration parameters from AN1760
>>    * As per the Configuration Application Note AN1760 published in the below link,
>>    * https://www.microchip.com/en-us/application-notes/an1760
>>    * Revision F (DS60001760G - June 2024)
>>    */
>> -static const u32 lan865x_revb0_fixup_registers[17] = {
>> +static const u32 lan865x_revb_fixup_registers[17] = {
>>   	0x00D0, 0x00E0, 0x00E9, 0x00F5,
>>   	0x00F4, 0x00F8, 0x00F9, 0x0081,
>>   	0x0091, 0x0043, 0x0044, 0x0045,
>> @@ -72,7 +73,7 @@ static const u32 lan865x_revb0_fixup_registers[17] = {
>>   	0x0050,
>>   };
>>   
>> -static const u16 lan865x_revb0_fixup_values[17] = {
>> +static const u16 lan865x_revb_fixup_values[17] = {
>>   	0x3F31, 0xC000, 0x9E50, 0x1CF8,
>>   	0xC020, 0xB900, 0x4E53, 0x0080,
>>   	0x9660, 0x00FF, 0xFFFF, 0x0000,
>> @@ -80,23 +81,23 @@ static const u16 lan865x_revb0_fixup_values[17] = {
>>   	0x0002,
>>   };
>>   
>> -static const u16 lan865x_revb0_fixup_cfg_regs[2] = {
>> +static const u16 lan865x_revb_fixup_cfg_regs[2] = {
>>   	0x0084, 0x008A,
>>   };
>>   
>> -static const u32 lan865x_revb0_sqi_fixup_regs[12] = {
>> +static const u32 lan865x_revb_sqi_fixup_regs[12] = {
>>   	0x00B0, 0x00B1, 0x00B2, 0x00B3,
>>   	0x00B4, 0x00B5, 0x00B6, 0x00B7,
>>   	0x00B8, 0x00B9, 0x00BA, 0x00BB,
>>   };
>>   
>> -static const u16 lan865x_revb0_sqi_fixup_values[12] = {
>> +static const u16 lan865x_revb_sqi_fixup_values[12] = {
>>   	0x0103, 0x0910, 0x1D26, 0x002A,
>>   	0x0103, 0x070D, 0x1720, 0x0027,
>>   	0x0509, 0x0E13, 0x1C25, 0x002B,
>>   };
>>   
>> -static const u16 lan865x_revb0_sqi_fixup_cfg_regs[3] = {
>> +static const u16 lan865x_revb_sqi_fixup_cfg_regs[3] = {
>>   	0x00AD, 0x00AE, 0x00AF,
>>   };
>>   
>> @@ -108,7 +109,7 @@ static const u16 lan865x_revb0_sqi_fixup_cfg_regs[3] = {
>>    *
>>    * 0x4 refers to memory map selector 4, which maps to MDIO_MMD_VEND2
>>    */
>> -static int lan865x_revb0_indirect_read(struct phy_device *phydev, u16 addr)
>> +static int lan865x_revb_indirect_read(struct phy_device *phydev, u16 addr)
>>   {
>>   	int ret;
>>   
>> @@ -134,7 +135,7 @@ static int lan865x_generate_cfg_offsets(struct phy_device *phydev, s8 offsets[])
>>   	int ret;
>>   
>>   	for (int i = 0; i < ARRAY_SIZE(fixup_regs); i++) {
>> -		ret = lan865x_revb0_indirect_read(phydev, fixup_regs[i]);
>> +		ret = lan865x_revb_indirect_read(phydev, fixup_regs[i]);
>>   		if (ret < 0)
>>   			return ret;
>>   
>> @@ -183,11 +184,11 @@ static int lan865x_write_cfg_params(struct phy_device *phydev,
>>   
>>   static int lan865x_setup_cfgparam(struct phy_device *phydev, s8 offsets[])
>>   {
>> -	u16 cfg_results[ARRAY_SIZE(lan865x_revb0_fixup_cfg_regs)];
>> -	u16 cfg_params[ARRAY_SIZE(lan865x_revb0_fixup_cfg_regs)];
>> +	u16 cfg_results[ARRAY_SIZE(lan865x_revb_fixup_cfg_regs)];
>> +	u16 cfg_params[ARRAY_SIZE(lan865x_revb_fixup_cfg_regs)];
>>   	int ret;
>>   
>> -	ret = lan865x_read_cfg_params(phydev, lan865x_revb0_fixup_cfg_regs,
>> +	ret = lan865x_read_cfg_params(phydev, lan865x_revb_fixup_cfg_regs,
>>   				      cfg_params, ARRAY_SIZE(cfg_params));
>>   	if (ret)
>>   		return ret;
>> @@ -197,17 +198,17 @@ static int lan865x_setup_cfgparam(struct phy_device *phydev, s8 offsets[])
>>   			 0x03;
>>   	cfg_results[1] = FIELD_PREP(GENMASK(15, 10), (40 + offsets[1]) & 0x3F);
>>   
>> -	return lan865x_write_cfg_params(phydev, lan865x_revb0_fixup_cfg_regs,
>> +	return lan865x_write_cfg_params(phydev, lan865x_revb_fixup_cfg_regs,
>>   					cfg_results, ARRAY_SIZE(cfg_results));
>>   }
>>   
>>   static int lan865x_setup_sqi_cfgparam(struct phy_device *phydev, s8 offsets[])
>>   {
>> -	u16 cfg_results[ARRAY_SIZE(lan865x_revb0_sqi_fixup_cfg_regs)];
>> -	u16 cfg_params[ARRAY_SIZE(lan865x_revb0_sqi_fixup_cfg_regs)];
>> +	u16 cfg_results[ARRAY_SIZE(lan865x_revb_sqi_fixup_cfg_regs)];
>> +	u16 cfg_params[ARRAY_SIZE(lan865x_revb_sqi_fixup_cfg_regs)];
>>   	int ret;
>>   
>> -	ret = lan865x_read_cfg_params(phydev, lan865x_revb0_sqi_fixup_cfg_regs,
>> +	ret = lan865x_read_cfg_params(phydev, lan865x_revb_sqi_fixup_cfg_regs,
>>   				      cfg_params, ARRAY_SIZE(cfg_params));
>>   	if (ret)
>>   		return ret;
>> @@ -219,12 +220,11 @@ static int lan865x_setup_sqi_cfgparam(struct phy_device *phydev, s8 offsets[])
>>   	cfg_results[2] = FIELD_PREP(GENMASK(15, 8), (17 + offsets[0]) & 0x3F) |
>>   			 ((22 + offsets[0]) & 0x3F);
>>   
>> -	return lan865x_write_cfg_params(phydev,
>> -					lan865x_revb0_sqi_fixup_cfg_regs,
>> +	return lan865x_write_cfg_params(phydev, lan865x_revb_sqi_fixup_cfg_regs,
>>   					cfg_results, ARRAY_SIZE(cfg_results));
>>   }
>>   
>> -static int lan865x_revb0_config_init(struct phy_device *phydev)
>> +static int lan865x_revb_config_init(struct phy_device *phydev)
>>   {
>>   	s8 offsets[2];
>>   	int ret;
>> @@ -236,10 +236,10 @@ static int lan865x_revb0_config_init(struct phy_device *phydev)
>>   	if (ret)
>>   		return ret;
>>   
>> -	for (int i = 0; i < ARRAY_SIZE(lan865x_revb0_fixup_registers); i++) {
>> +	for (int i = 0; i < ARRAY_SIZE(lan865x_revb_fixup_registers); i++) {
>>   		ret = phy_write_mmd(phydev, MDIO_MMD_VEND2,
>> -				    lan865x_revb0_fixup_registers[i],
>> -				    lan865x_revb0_fixup_values[i]);
>> +				    lan865x_revb_fixup_registers[i],
>> +				    lan865x_revb_fixup_values[i]);
>>   		if (ret)
>>   			return ret;
>>   
>> @@ -254,10 +254,10 @@ static int lan865x_revb0_config_init(struct phy_device *phydev)
>>   	if (ret)
>>   		return ret;
>>   
>> -	for (int i = 0; i < ARRAY_SIZE(lan865x_revb0_sqi_fixup_regs); i++) {
>> +	for (int i = 0; i < ARRAY_SIZE(lan865x_revb_sqi_fixup_regs); i++) {
>>   		ret = phy_write_mmd(phydev, MDIO_MMD_VEND2,
>> -				    lan865x_revb0_sqi_fixup_regs[i],
>> -				    lan865x_revb0_sqi_fixup_values[i]);
>> +				    lan865x_revb_sqi_fixup_regs[i],
>> +				    lan865x_revb_sqi_fixup_values[i]);
>>   		if (ret)
>>   			return ret;
>>   	}
>> @@ -332,10 +332,10 @@ static struct phy_driver microchip_t1s_driver[] = {
>>   		.get_plca_status    = genphy_c45_plca_get_status,
>>   	},
>>   	{
>> -		PHY_ID_MATCH_EXACT(PHY_ID_LAN865X_REVB0),
>> -		.name               = "LAN865X Rev.B0 Internal Phy",
>> +		PHY_ID_MATCH_EXACT(PHY_ID_LAN865X_REVB),
>> +		.name               = "LAN865X Rev.B0/B1 Internal Phy",
>>   		.features           = PHY_BASIC_T1S_P2MP_FEATURES,
>> -		.config_init        = lan865x_revb0_config_init,
>> +		.config_init        = lan865x_revb_config_init,
>>   		.read_status        = lan86xx_read_status,
>>   		.get_plca_cfg	    = genphy_c45_plca_get_cfg,
>>   		.set_plca_cfg	    = genphy_c45_plca_set_cfg,
>> @@ -347,7 +347,7 @@ module_phy_driver(microchip_t1s_driver);
>>   
>>   static struct mdio_device_id __maybe_unused tbl[] = {
>>   	{ PHY_ID_MATCH_EXACT(PHY_ID_LAN867X_REVB1) },
>> -	{ PHY_ID_MATCH_EXACT(PHY_ID_LAN865X_REVB0) },
>> +	{ PHY_ID_MATCH_EXACT(PHY_ID_LAN865X_REVB) },
>>   	{ }
>>   };
>>   
>> -- 
>> 2.34.1
>>
> 

Re: [PATCH net-next v2 5/7] net: phy: microchip_t1s: add support for Microchip's LAN867X Rev.C1

[Parthiban.Veerasooran]

Hi Simon,

On 03/09/24 1:00 pm, Simon Horman wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Mon, Sep 02, 2024 at 08:04:56PM +0530, Parthiban Veerasooran wrote:
>> This patch adds support for LAN8670/1/2 Rev.C1 as per the latest
>> configuration note AN1699 released (Revision E (DS60001699F - June 2024))
>> https://www.microchip.com/en-us/application-notes/an1699
>>
>> Signed-off-by: Parthiban Veerasooran <Parthiban.Veerasooran@microchip.com>
> 
> ...
> 
>> diff --git a/drivers/net/phy/microchip_t1s.c b/drivers/net/phy/microchip_t1s.c
> 
> ...
> 
>> @@ -290,6 +291,58 @@ static int lan867x_check_reset_complete(struct phy_device *phydev)
>>        return 0;
>>   }
>>
>> +static int lan867x_revc1_config_init(struct phy_device *phydev)
>> +{
>> +     s8 offsets[2];
>> +     int ret;
>> +
>> +     ret = lan867x_check_reset_complete(phydev);
>> +     if (ret)
>> +             return ret;
>> +
>> +     ret = lan865x_generate_cfg_offsets(phydev, offsets);
>> +     if (ret)
>> +             return ret;
>> +
>> +     /* LAN867x Rev.C1 configuration settings are equal to the first 9
>> +      * configuration settings and all the sqi fixup settings from LAN865x
>> +      * Rev.B0/B1. So the same fixup registers and values from LAN865x
>> +      * Rev.B0/B1 are used for LAN867x Rev.C1 to avoid duplication.
>> +      * Refer the below links for the comparision.
> 
> nit: comparison
> 
>       Flagged by checkpatch.pl --codespell
Ah yes, will correct it in the next version.

Best regards,
Parthiban V
> 
>> +      * https://www.microchip.com/en-us/application-notes/an1760
>> +      * Revision F (DS60001760G - June 2024)
>> +      * https://www.microchip.com/en-us/application-notes/an1699
>> +      * Revision E (DS60001699F - June 2024)
>> +      */

Re: [PATCH net-next v2 0/7] microchip_t1s: Update on Microchip 10BASE-T1S PHY driver

[Parthiban.Veerasooran]

Hi Horatiu,

On 03/09/24 11:54 am, Horatiu Vultur wrote:
> The 09/02/2024 20:04, Parthiban Veerasooran wrote:
> 
> Hi Parthiban,
> 
>> This patch series contain the below updates,
>>
>> v1:
> 
> You usually use v1, v2, v3 to say what is the difference between series.
> But in your case you already had v1 already in first patch series which
> it is strange to be used. I think you wanted to just describe the
> features of this patch series.
> So, my suggestion is to drop v1 because those are the features not
> changes between version, and put ':' instead of ',' after updates.
Ah yes, will correct them in the next version.
> 
> Also almost all your patches in this series start with 'This patch',
> please change this to imperative mode:
> https://www.kernel.org/doc/html/v4.10/process/submitting-patches.html
OK. Will correct them in the next version.

Best regards,
Parthiban V
> 
>> - Restructured lan865x_write_cfg_params() and lan865x_read_cfg_params()
>>    functions arguments to more generic.
>> - Updated new/improved initial settings of LAN865X Rev.B0 from latest
>>    AN1760.
>> - Added support for LAN865X Rev.B1 from latest AN1760.
>> - Moved LAN867X reset handling to a new function for flexibility.
>> - Added support for LAN867X Rev.C1/C2 from latest AN1699.
>> - Disabled/enabled collision detection based on PLCA setting.
>>
>> v2:
>> - Fixed indexing issue in the configuration parameter setup.
>>
>> Parthiban Veerasooran (7):
>>    net: phy: microchip_t1s: restructure cfg read/write functions
>>      arguments
>>    net: phy: microchip_t1s: update new initial settings for LAN865X
>>      Rev.B0
>>    net: phy: microchip_t1s: add support for Microchip's LAN865X Rev.B1
>>    net: phy: microchip_t1s: move LAN867X reset handling to a new function
>>    net: phy: microchip_t1s: add support for Microchip's LAN867X Rev.C1
>>    net: phy: microchip_t1s: add support for Microchip's LAN867X Rev.C2
>>    net: phy: microchip_t1s: configure collision detection based on PLCA
>>      mode
>>
>>   drivers/net/phy/Kconfig         |   4 +-
>>   drivers/net/phy/microchip_t1s.c | 299 +++++++++++++++++++++++++-------
>>   2 files changed, 239 insertions(+), 64 deletions(-)
>>
>>
>> base-commit: 221f9cce949ac8042f65b71ed1fde13b99073256
>> -- 
>> 2.34.1
>>
> 

Re: [PATCH net-next v2 7/7] net: phy: microchip_t1s: configure collision detection based on PLCA mode

[Parthiban.Veerasooran]

Hi Horatiu,

On 03/09/24 12:13 pm, Horatiu Vultur wrote:
> The 09/02/2024 20:04, Parthiban Veerasooran wrote:
> 
> Hi Parthiban,
> 
>> As per LAN8650/1 Rev.B0/B1 AN1760 (Revision F (DS60001760G - June 2024))
>> and LAN8670/1/2 Rev.C1/C2 AN1699 (Revision E (DS60001699F - June 2024)),
>> under normal operation, the device should be operated in PLCA mode.
>> Disabling collision detection is recommended to allow the device to
>> operate in noisy environments or when reflections and other inherent
>> transmission line distortion cause poor signal quality. Collision
>> detection must be re-enabled if the device is configured to operate in
>> CSMA/CD mode.
> 
> Is this something that applies only to Microchip PHYs or is something
> generic that applies to all the PHYs.
Yes, the behavior is common for all the PHYs but it is purely up to the 
PHY chip design specific.

1. Some vendors will enable this feature in the chip level by latching 
the register bits. There we don't need software interface.
2. Some vendors need software interface to enable this feature like our 
Microchip PHY does.

Best regards,
Parthiban V
> 
>>
>> Signed-off-by: Parthiban Veerasooran <Parthiban.Veerasooran@microchip.com>
>> ---
>>   drivers/net/phy/microchip_t1s.c | 42 ++++++++++++++++++++++++++++++---
>>   1 file changed, 39 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/phy/microchip_t1s.c b/drivers/net/phy/microchip_t1s.c
>> index bd0c768df0af..a0565508d7d2 100644
>> --- a/drivers/net/phy/microchip_t1s.c
>> +++ b/drivers/net/phy/microchip_t1s.c
>> @@ -26,6 +26,12 @@
>>   #define LAN865X_REG_CFGPARAM_CTRL 0x00DA
>>   #define LAN865X_REG_STS2 0x0019
>>   
>> +/* Collision Detector Control 0 Register */
>> +#define LAN86XX_REG_COL_DET_CTRL0	0x0087
>> +#define COL_DET_CTRL0_ENABLE_BIT_MASK	BIT(15)
>> +#define COL_DET_ENABLE			BIT(15)
>> +#define COL_DET_DISABLE			0x0000
>> +
>>   #define LAN865X_CFGPARAM_READ_ENABLE BIT(1)
>>   
>>   /* The arrays below are pulled from the following table from AN1699
>> @@ -370,6 +376,36 @@ static int lan867x_revb1_config_init(struct phy_device *phydev)
>>   	return 0;
>>   }
>>   
>> +/* As per LAN8650/1 Rev.B0/B1 AN1760 (Revision F (DS60001760G - June 2024)) and
>> + * LAN8670/1/2 Rev.C1/C2 AN1699 (Revision E (DS60001699F - June 2024)), under
>> + * normal operation, the device should be operated in PLCA mode. Disabling
>> + * collision detection is recommended to allow the device to operate in noisy
>> + * environments or when reflections and other inherent transmission line
>> + * distortion cause poor signal quality. Collision detection must be re-enabled
>> + * if the device is configured to operate in CSMA/CD mode.
>> + *
>> + * AN1760: https://www.microchip.com/en-us/application-notes/an1760
>> + * AN1699: https://www.microchip.com/en-us/application-notes/an1699
>> + */
>> +static int lan86xx_plca_set_cfg(struct phy_device *phydev,
>> +				const struct phy_plca_cfg *plca_cfg)
>> +{
>> +	int ret;
>> +
>> +	ret = genphy_c45_plca_set_cfg(phydev, plca_cfg);
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (plca_cfg->enabled)
>> +		return phy_modify_mmd(phydev, MDIO_MMD_VEND2,
>> +				      LAN86XX_REG_COL_DET_CTRL0,
>> +				      COL_DET_CTRL0_ENABLE_BIT_MASK,
>> +				      COL_DET_DISABLE);
>> +
>> +	return phy_modify_mmd(phydev, MDIO_MMD_VEND2, LAN86XX_REG_COL_DET_CTRL0,
>> +			      COL_DET_CTRL0_ENABLE_BIT_MASK, COL_DET_ENABLE);
>> +}
>> +
>>   static int lan86xx_read_status(struct phy_device *phydev)
>>   {
>>   	/* The phy has some limitations, namely:
>> @@ -403,7 +439,7 @@ static struct phy_driver microchip_t1s_driver[] = {
>>   		.config_init        = lan867x_revc_config_init,
>>   		.read_status        = lan86xx_read_status,
>>   		.get_plca_cfg	    = genphy_c45_plca_get_cfg,
>> -		.set_plca_cfg	    = genphy_c45_plca_set_cfg,
>> +		.set_plca_cfg	    = lan86xx_plca_set_cfg,
>>   		.get_plca_status    = genphy_c45_plca_get_status,
>>   	},
>>   	{
>> @@ -413,7 +449,7 @@ static struct phy_driver microchip_t1s_driver[] = {
>>   		.config_init        = lan867x_revc_config_init,
>>   		.read_status        = lan86xx_read_status,
>>   		.get_plca_cfg	    = genphy_c45_plca_get_cfg,
>> -		.set_plca_cfg	    = genphy_c45_plca_set_cfg,
>> +		.set_plca_cfg	    = lan86xx_plca_set_cfg,
>>   		.get_plca_status    = genphy_c45_plca_get_status,
>>   	},
>>   	{
>> @@ -423,7 +459,7 @@ static struct phy_driver microchip_t1s_driver[] = {
>>   		.config_init        = lan865x_revb_config_init,
>>   		.read_status        = lan86xx_read_status,
>>   		.get_plca_cfg	    = genphy_c45_plca_get_cfg,
>> -		.set_plca_cfg	    = genphy_c45_plca_set_cfg,
>> +		.set_plca_cfg	    = lan86xx_plca_set_cfg,
>>   		.get_plca_status    = genphy_c45_plca_get_status,
>>   	},
>>   };
>> -- 
>> 2.34.1
>>
> 

Re: [PATCH net-next v2 2/7] net: phy: microchip_t1s: update new initial settings for LAN865X Rev.B0

[Horatiu Vultur - M31836]

The 09/04/2024 10:20, Parthiban Veerasooran - I17164 wrote:
> Hi Horatiu,
> 
> Thanks for reviewing the patches.
> 
> On 03/09/24 12:03 pm, Horatiu Vultur wrote:
> > The 09/02/2024 20:04, Parthiban Veerasooran wrote:
> >> This patch configures the new/improved initial settings from the latest
> >> configuration application note AN1760 released for LAN8650/1 Rev.B0
> >> Revision F (DS60001760G - June 2024).
> >> https://www.microchip.com/en-us/application-notes/an1760
> >>
> >> Signed-off-by: Parthiban Veerasooran <Parthiban.Veerasooran@microchip.com>
> >> ---
> >>   drivers/net/phy/microchip_t1s.c | 119 ++++++++++++++++++++++----------
> >>   1 file changed, 83 insertions(+), 36 deletions(-)
> >>
> >> diff --git a/drivers/net/phy/microchip_t1s.c b/drivers/net/phy/microchip_t1s.c
> >> index 0110f3357489..fb651cfa3ee0 100644
> >> --- a/drivers/net/phy/microchip_t1s.c
> >> +++ b/drivers/net/phy/microchip_t1s.c
> >> @@ -59,29 +59,45 @@ static const u16 lan867x_revb1_fixup_masks[12] = {
> >>   	0x0600, 0x7F00, 0x2000, 0xFFFF,
> >>   };
> >>   
> >> -/* LAN865x Rev.B0 configuration parameters from AN1760 */
> >> -static const u32 lan865x_revb0_fixup_registers[28] = {
> >> -	0x0091, 0x0081, 0x0043, 0x0044,
> >> -	0x0045, 0x0053, 0x0054, 0x0055,
> >> -	0x0040, 0x0050, 0x00D0, 0x00E9,
> >> -	0x00F5, 0x00F4, 0x00F8, 0x00F9,
> >> +/* LAN865x Rev.B0 configuration parameters from AN1760
> >> + * As per the Configuration Application Note AN1760 published in the below link,
> >> + * https://www.microchip.com/en-us/application-notes/an1760
> >> + * Revision F (DS60001760G - June 2024)
> >> + */
> >> +static const u32 lan865x_revb0_fixup_registers[17] = {
> >> +	0x00D0, 0x00E0, 0x00E9, 0x00F5,
> >> +	0x00F4, 0x00F8, 0x00F9, 0x0081,
> >> +	0x0091, 0x0043, 0x0044, 0x0045,
> >> +	0x0053, 0x0054, 0x0055, 0x0040,
> >> +	0x0050,
> >> +};
> >> +
> >> +static const u16 lan865x_revb0_fixup_values[17] = {
> >> +	0x3F31, 0xC000, 0x9E50, 0x1CF8,
> >> +	0xC020, 0xB900, 0x4E53, 0x0080,
> >> +	0x9660, 0x00FF, 0xFFFF, 0x0000,
> >> +	0x00FF, 0xFFFF, 0x0000, 0x0002,
> >> +	0x0002,
> >> +};
> >> +
> >> +static const u16 lan865x_revb0_fixup_cfg_regs[2] = {
> >> +	0x0084, 0x008A,
> >> +};
> >> +
> >> +static const u32 lan865x_revb0_sqi_fixup_regs[12] = {
> >>   	0x00B0, 0x00B1, 0x00B2, 0x00B3,
> >>   	0x00B4, 0x00B5, 0x00B6, 0x00B7,
> >>   	0x00B8, 0x00B9, 0x00BA, 0x00BB,
> >>   };
> >>   
> >> -static const u16 lan865x_revb0_fixup_values[28] = {
> >> -	0x9660, 0x00C0, 0x00FF, 0xFFFF,
> >> -	0x0000, 0x00FF, 0xFFFF, 0x0000,
> >> -	0x0002, 0x0002, 0x5F21, 0x9E50,
> >> -	0x1CF8, 0xC020, 0x9B00, 0x4E53,
> >> +static const u16 lan865x_revb0_sqi_fixup_values[12] = {
> >>   	0x0103, 0x0910, 0x1D26, 0x002A,
> >>   	0x0103, 0x070D, 0x1720, 0x0027,
> >>   	0x0509, 0x0E13, 0x1C25, 0x002B,
> >>   };
> >>   
> >> -static const u16 lan865x_revb0_fixup_cfg_regs[5] = {
> >> -	0x0084, 0x008A, 0x00AD, 0x00AE, 0x00AF
> >> +static const u16 lan865x_revb0_sqi_fixup_cfg_regs[3] = {
> >> +	0x00AD, 0x00AE, 0x00AF,
> >>   };
> >>   
> >>   /* Pulled from AN1760 describing 'indirect read'
> >> @@ -121,6 +137,8 @@ static int lan865x_generate_cfg_offsets(struct phy_device *phydev, s8 offsets[])
> >>   		ret = lan865x_revb0_indirect_read(phydev, fixup_regs[i]);
> >>   		if (ret < 0)
> >>   			return ret;
> >> +
> >> +		ret &= 0x1F;
> > 
> > Is this diff supposed to be part of this patch?
> Yes.

Just for my understanding, why this is needed now and not before?
Because I can see that now you always & the offset with 0x3f. Is it
because you might get overflow because the value is signed?

> > Also you can use GENMASK here.
> Ah ok, it is GENMASK(4, 0) then.
> 
> Best regards,
> Parthiban V
> > 
> >>   		if (ret & BIT(4))
> >>   			offsets[i] = ret | 0xE0;
> >>   		else
> >> @@ -163,59 +181,88 @@ static int lan865x_write_cfg_params(struct phy_device *phydev,
> >>   	return 0;
> >>   }
> >>   
> >> -static int lan865x_setup_cfgparam(struct phy_device *phydev)
> >> +static int lan865x_setup_cfgparam(struct phy_device *phydev, s8 offsets[])
> >>   {
> >>   	u16 cfg_results[ARRAY_SIZE(lan865x_revb0_fixup_cfg_regs)];
> >>   	u16 cfg_params[ARRAY_SIZE(lan865x_revb0_fixup_cfg_regs)];
> >> -	s8 offsets[2];
> >>   	int ret;
> >>   
> >> -	ret = lan865x_generate_cfg_offsets(phydev, offsets);
> >> +	ret = lan865x_read_cfg_params(phydev, lan865x_revb0_fixup_cfg_regs,
> >> +				      cfg_params, ARRAY_SIZE(cfg_params));
> >>   	if (ret)
> >>   		return ret;
> >>   
> >> -	ret = lan865x_read_cfg_params(phydev, lan865x_revb0_fixup_cfg_regs,
> >> +	cfg_results[0] = FIELD_PREP(GENMASK(15, 10), (9 + offsets[0]) & 0x3F) |
> >> +			 FIELD_PREP(GENMASK(15, 4), (14 + offsets[0]) & 0x3F) |
> >> +			 0x03;
> >> +	cfg_results[1] = FIELD_PREP(GENMASK(15, 10), (40 + offsets[1]) & 0x3F);
> >> +
> >> +	return lan865x_write_cfg_params(phydev, lan865x_revb0_fixup_cfg_regs,
> >> +					cfg_results, ARRAY_SIZE(cfg_results));
> >> +}
> >> +
> >> +static int lan865x_setup_sqi_cfgparam(struct phy_device *phydev, s8 offsets[])
> >> +{
> >> +	u16 cfg_results[ARRAY_SIZE(lan865x_revb0_sqi_fixup_cfg_regs)];
> >> +	u16 cfg_params[ARRAY_SIZE(lan865x_revb0_sqi_fixup_cfg_regs)];
> >> +	int ret;
> >> +
> >> +	ret = lan865x_read_cfg_params(phydev, lan865x_revb0_sqi_fixup_cfg_regs,
> >>   				      cfg_params, ARRAY_SIZE(cfg_params));
> >>   	if (ret)
> >>   		return ret;
> >>   
> >> -	cfg_results[0] = (cfg_params[0] & 0x000F) |
> >> -			  FIELD_PREP(GENMASK(15, 10), 9 + offsets[0]) |
> >> -			  FIELD_PREP(GENMASK(15, 4), 14 + offsets[0]);
> >> -	cfg_results[1] = (cfg_params[1] & 0x03FF) |
> >> -			  FIELD_PREP(GENMASK(15, 10), 40 + offsets[1]);
> >> -	cfg_results[2] = (cfg_params[2] & 0xC0C0) |
> >> -			  FIELD_PREP(GENMASK(15, 8), 5 + offsets[0]) |
> >> -			  (9 + offsets[0]);
> >> -	cfg_results[3] = (cfg_params[3] & 0xC0C0) |
> >> -			  FIELD_PREP(GENMASK(15, 8), 9 + offsets[0]) |
> >> -			  (14 + offsets[0]);
> >> -	cfg_results[4] = (cfg_params[4] & 0xC0C0) |
> >> -			  FIELD_PREP(GENMASK(15, 8), 17 + offsets[0]) |
> >> -			  (22 + offsets[0]);
> >> +	cfg_results[0] = FIELD_PREP(GENMASK(15, 8), (5 + offsets[0]) & 0x3F) |
> >> +			 ((9 + offsets[0]) & 0x3F);
> >> +	cfg_results[1] = FIELD_PREP(GENMASK(15, 8), (9 + offsets[0]) & 0x3F) |
> >> +			 ((14 + offsets[0]) & 0x3F);
> >> +	cfg_results[2] = FIELD_PREP(GENMASK(15, 8), (17 + offsets[0]) & 0x3F) |
> >> +			 ((22 + offsets[0]) & 0x3F);
> >>   
> >> -	return lan865x_write_cfg_params(phydev, lan865x_revb0_fixup_cfg_regs,
> >> +	return lan865x_write_cfg_params(phydev,
> >> +					lan865x_revb0_sqi_fixup_cfg_regs,
> >>   					cfg_results, ARRAY_SIZE(cfg_results));
> >>   }
> >>   
> >>   static int lan865x_revb0_config_init(struct phy_device *phydev)
> >>   {
> >> +	s8 offsets[2];
> >>   	int ret;
> >>   
> >>   	/* Reference to AN1760
> >>   	 * https://ww1.microchip.com/downloads/aemDocuments/documents/AIS/ProductDocuments/SupportingCollateral/AN-LAN8650-1-Configuration-60001760.pdf
> >>   	 */
> >> +	ret = lan865x_generate_cfg_offsets(phydev, offsets);
> >> +	if (ret)
> >> +		return ret;
> >> +
> >>   	for (int i = 0; i < ARRAY_SIZE(lan865x_revb0_fixup_registers); i++) {
> >>   		ret = phy_write_mmd(phydev, MDIO_MMD_VEND2,
> >>   				    lan865x_revb0_fixup_registers[i],
> >>   				    lan865x_revb0_fixup_values[i]);
> >>   		if (ret)
> >>   			return ret;
> >> +
> >> +		if (i == 1) {
> >> +			ret = lan865x_setup_cfgparam(phydev, offsets);
> >> +			if (ret)
> >> +				return ret;
> >> +		}
> >>   	}
> >> -	/* Function to calculate and write the configuration parameters in the
> >> -	 * 0x0084, 0x008A, 0x00AD, 0x00AE and 0x00AF registers (from AN1760)
> >> -	 */
> >> -	return lan865x_setup_cfgparam(phydev);
> >> +
> >> +	ret = lan865x_setup_sqi_cfgparam(phydev, offsets);
> >> +	if (ret)
> >> +		return ret;
> >> +
> >> +	for (int i = 0; i < ARRAY_SIZE(lan865x_revb0_sqi_fixup_regs); i++) {
> >> +		ret = phy_write_mmd(phydev, MDIO_MMD_VEND2,
> >> +				    lan865x_revb0_sqi_fixup_regs[i],
> >> +				    lan865x_revb0_sqi_fixup_values[i]);
> >> +		if (ret)
> >> +			return ret;
> >> +	}
> >> +
> >> +	return 0;
> >>   }
> >>   
> >>   static int lan867x_revb1_config_init(struct phy_device *phydev)
> >> -- 
> >> 2.34.1
> >>
> > 
> 

-- 
/Horatiu

Re: [PATCH net-next v2 7/7] net: phy: microchip_t1s: configure collision detection based on PLCA mode

[Horatiu Vultur - M31836]

The 09/04/2024 11:46, Parthiban Veerasooran - I17164 wrote:
> Hi Horatiu,
> 
> On 03/09/24 12:13 pm, Horatiu Vultur wrote:
> > The 09/02/2024 20:04, Parthiban Veerasooran wrote:
> > 
> > Hi Parthiban,
> > 
> >> As per LAN8650/1 Rev.B0/B1 AN1760 (Revision F (DS60001760G - June 2024))
> >> and LAN8670/1/2 Rev.C1/C2 AN1699 (Revision E (DS60001699F - June 2024)),
> >> under normal operation, the device should be operated in PLCA mode.
> >> Disabling collision detection is recommended to allow the device to
> >> operate in noisy environments or when reflections and other inherent
> >> transmission line distortion cause poor signal quality. Collision
> >> detection must be re-enabled if the device is configured to operate in
> >> CSMA/CD mode.
> > 
> > Is this something that applies only to Microchip PHYs or is something
> > generic that applies to all the PHYs.
> Yes, the behavior is common for all the PHYs but it is purely up to the 
> PHY chip design specific.
> 
> 1. Some vendors will enable this feature in the chip level by latching 
> the register bits. There we don't need software interface.
> 2. Some vendors need software interface to enable this feature like our 
> Microchip PHY does.

Don't you think then is better to create something more generic so other
PHYs to able to do this?

> 
> Best regards,
> Parthiban V
> > 
> >>
> >> Signed-off-by: Parthiban Veerasooran <Parthiban.Veerasooran@microchip.com>
> >> ---
> >>   drivers/net/phy/microchip_t1s.c | 42 ++++++++++++++++++++++++++++++---
> >>   1 file changed, 39 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/net/phy/microchip_t1s.c b/drivers/net/phy/microchip_t1s.c
> >> index bd0c768df0af..a0565508d7d2 100644
> >> --- a/drivers/net/phy/microchip_t1s.c
> >> +++ b/drivers/net/phy/microchip_t1s.c
> >> @@ -26,6 +26,12 @@
> >>   #define LAN865X_REG_CFGPARAM_CTRL 0x00DA
> >>   #define LAN865X_REG_STS2 0x0019
> >>   
> >> +/* Collision Detector Control 0 Register */
> >> +#define LAN86XX_REG_COL_DET_CTRL0	0x0087
> >> +#define COL_DET_CTRL0_ENABLE_BIT_MASK	BIT(15)
> >> +#define COL_DET_ENABLE			BIT(15)
> >> +#define COL_DET_DISABLE			0x0000
> >> +
> >>   #define LAN865X_CFGPARAM_READ_ENABLE BIT(1)
> >>   
> >>   /* The arrays below are pulled from the following table from AN1699
> >> @@ -370,6 +376,36 @@ static int lan867x_revb1_config_init(struct phy_device *phydev)
> >>   	return 0;
> >>   }
> >>   
> >> +/* As per LAN8650/1 Rev.B0/B1 AN1760 (Revision F (DS60001760G - June 2024)) and
> >> + * LAN8670/1/2 Rev.C1/C2 AN1699 (Revision E (DS60001699F - June 2024)), under
> >> + * normal operation, the device should be operated in PLCA mode. Disabling
> >> + * collision detection is recommended to allow the device to operate in noisy
> >> + * environments or when reflections and other inherent transmission line
> >> + * distortion cause poor signal quality. Collision detection must be re-enabled
> >> + * if the device is configured to operate in CSMA/CD mode.
> >> + *
> >> + * AN1760: https://www.microchip.com/en-us/application-notes/an1760
> >> + * AN1699: https://www.microchip.com/en-us/application-notes/an1699
> >> + */
> >> +static int lan86xx_plca_set_cfg(struct phy_device *phydev,
> >> +				const struct phy_plca_cfg *plca_cfg)
> >> +{
> >> +	int ret;
> >> +
> >> +	ret = genphy_c45_plca_set_cfg(phydev, plca_cfg);
> >> +	if (ret)
> >> +		return ret;
> >> +
> >> +	if (plca_cfg->enabled)
> >> +		return phy_modify_mmd(phydev, MDIO_MMD_VEND2,
> >> +				      LAN86XX_REG_COL_DET_CTRL0,
> >> +				      COL_DET_CTRL0_ENABLE_BIT_MASK,
> >> +				      COL_DET_DISABLE);
> >> +
> >> +	return phy_modify_mmd(phydev, MDIO_MMD_VEND2, LAN86XX_REG_COL_DET_CTRL0,
> >> +			      COL_DET_CTRL0_ENABLE_BIT_MASK, COL_DET_ENABLE);
> >> +}
> >> +
> >>   static int lan86xx_read_status(struct phy_device *phydev)
> >>   {
> >>   	/* The phy has some limitations, namely:
> >> @@ -403,7 +439,7 @@ static struct phy_driver microchip_t1s_driver[] = {
> >>   		.config_init        = lan867x_revc_config_init,
> >>   		.read_status        = lan86xx_read_status,
> >>   		.get_plca_cfg	    = genphy_c45_plca_get_cfg,
> >> -		.set_plca_cfg	    = genphy_c45_plca_set_cfg,
> >> +		.set_plca_cfg	    = lan86xx_plca_set_cfg,
> >>   		.get_plca_status    = genphy_c45_plca_get_status,
> >>   	},
> >>   	{
> >> @@ -413,7 +449,7 @@ static struct phy_driver microchip_t1s_driver[] = {
> >>   		.config_init        = lan867x_revc_config_init,
> >>   		.read_status        = lan86xx_read_status,
> >>   		.get_plca_cfg	    = genphy_c45_plca_get_cfg,
> >> -		.set_plca_cfg	    = genphy_c45_plca_set_cfg,
> >> +		.set_plca_cfg	    = lan86xx_plca_set_cfg,
> >>   		.get_plca_status    = genphy_c45_plca_get_status,
> >>   	},
> >>   	{
> >> @@ -423,7 +459,7 @@ static struct phy_driver microchip_t1s_driver[] = {
> >>   		.config_init        = lan865x_revb_config_init,
> >>   		.read_status        = lan86xx_read_status,
> >>   		.get_plca_cfg	    = genphy_c45_plca_get_cfg,
> >> -		.set_plca_cfg	    = genphy_c45_plca_set_cfg,
> >> +		.set_plca_cfg	    = lan86xx_plca_set_cfg,
> >>   		.get_plca_status    = genphy_c45_plca_get_status,
> >>   	},
> >>   };
> >> -- 
> >> 2.34.1
> >>
> > 
> 

-- 
/Horatiu

Re: [PATCH net-next v2 2/7] net: phy: microchip_t1s: update new initial settings for LAN865X Rev.B0

[Parthiban.Veerasooran]

Hi Horatiu,

On 06/09/24 12:07 pm, Horatiu Vultur - M31836 wrote:
> The 09/04/2024 10:20, Parthiban Veerasooran - I17164 wrote:
>> Hi Horatiu,
>>
>> Thanks for reviewing the patches.
>>
>> On 03/09/24 12:03 pm, Horatiu Vultur wrote:
>>> The 09/02/2024 20:04, Parthiban Veerasooran wrote:
>>>> This patch configures the new/improved initial settings from the latest
>>>> configuration application note AN1760 released for LAN8650/1 Rev.B0
>>>> Revision F (DS60001760G - June 2024).
>>>> https://www.microchip.com/en-us/application-notes/an1760
>>>>
>>>> Signed-off-by: Parthiban Veerasooran <Parthiban.Veerasooran@microchip.com>
>>>> ---
>>>>    drivers/net/phy/microchip_t1s.c | 119 ++++++++++++++++++++++----------
>>>>    1 file changed, 83 insertions(+), 36 deletions(-)
>>>>
>>>> diff --git a/drivers/net/phy/microchip_t1s.c b/drivers/net/phy/microchip_t1s.c
>>>> index 0110f3357489..fb651cfa3ee0 100644
>>>> --- a/drivers/net/phy/microchip_t1s.c
>>>> +++ b/drivers/net/phy/microchip_t1s.c
>>>> @@ -59,29 +59,45 @@ static const u16 lan867x_revb1_fixup_masks[12] = {
>>>>    	0x0600, 0x7F00, 0x2000, 0xFFFF,
>>>>    };
>>>>    
>>>> -/* LAN865x Rev.B0 configuration parameters from AN1760 */
>>>> -static const u32 lan865x_revb0_fixup_registers[28] = {
>>>> -	0x0091, 0x0081, 0x0043, 0x0044,
>>>> -	0x0045, 0x0053, 0x0054, 0x0055,
>>>> -	0x0040, 0x0050, 0x00D0, 0x00E9,
>>>> -	0x00F5, 0x00F4, 0x00F8, 0x00F9,
>>>> +/* LAN865x Rev.B0 configuration parameters from AN1760
>>>> + * As per the Configuration Application Note AN1760 published in the below link,
>>>> + * https://www.microchip.com/en-us/application-notes/an1760
>>>> + * Revision F (DS60001760G - June 2024)
>>>> + */
>>>> +static const u32 lan865x_revb0_fixup_registers[17] = {
>>>> +	0x00D0, 0x00E0, 0x00E9, 0x00F5,
>>>> +	0x00F4, 0x00F8, 0x00F9, 0x0081,
>>>> +	0x0091, 0x0043, 0x0044, 0x0045,
>>>> +	0x0053, 0x0054, 0x0055, 0x0040,
>>>> +	0x0050,
>>>> +};
>>>> +
>>>> +static const u16 lan865x_revb0_fixup_values[17] = {
>>>> +	0x3F31, 0xC000, 0x9E50, 0x1CF8,
>>>> +	0xC020, 0xB900, 0x4E53, 0x0080,
>>>> +	0x9660, 0x00FF, 0xFFFF, 0x0000,
>>>> +	0x00FF, 0xFFFF, 0x0000, 0x0002,
>>>> +	0x0002,
>>>> +};
>>>> +
>>>> +static const u16 lan865x_revb0_fixup_cfg_regs[2] = {
>>>> +	0x0084, 0x008A,
>>>> +};
>>>> +
>>>> +static const u32 lan865x_revb0_sqi_fixup_regs[12] = {
>>>>    	0x00B0, 0x00B1, 0x00B2, 0x00B3,
>>>>    	0x00B4, 0x00B5, 0x00B6, 0x00B7,
>>>>    	0x00B8, 0x00B9, 0x00BA, 0x00BB,
>>>>    };
>>>>    
>>>> -static const u16 lan865x_revb0_fixup_values[28] = {
>>>> -	0x9660, 0x00C0, 0x00FF, 0xFFFF,
>>>> -	0x0000, 0x00FF, 0xFFFF, 0x0000,
>>>> -	0x0002, 0x0002, 0x5F21, 0x9E50,
>>>> -	0x1CF8, 0xC020, 0x9B00, 0x4E53,
>>>> +static const u16 lan865x_revb0_sqi_fixup_values[12] = {
>>>>    	0x0103, 0x0910, 0x1D26, 0x002A,
>>>>    	0x0103, 0x070D, 0x1720, 0x0027,
>>>>    	0x0509, 0x0E13, 0x1C25, 0x002B,
>>>>    };
>>>>    
>>>> -static const u16 lan865x_revb0_fixup_cfg_regs[5] = {
>>>> -	0x0084, 0x008A, 0x00AD, 0x00AE, 0x00AF
>>>> +static const u16 lan865x_revb0_sqi_fixup_cfg_regs[3] = {
>>>> +	0x00AD, 0x00AE, 0x00AF,
>>>>    };
>>>>    
>>>>    /* Pulled from AN1760 describing 'indirect read'
>>>> @@ -121,6 +137,8 @@ static int lan865x_generate_cfg_offsets(struct phy_device *phydev, s8 offsets[])
>>>>    		ret = lan865x_revb0_indirect_read(phydev, fixup_regs[i]);
>>>>    		if (ret < 0)
>>>>    			return ret;
>>>> +
>>>> +		ret &= 0x1F;
>>>
>>> Is this diff supposed to be part of this patch?
>> Yes.
> 
> Just for my understanding, why this is needed now and not before?
> Because I can see that now you always & the offset with 0x3f. Is it
> because you might get overflow because the value is signed?
I don't know why it wasn't there in the old configuration note released. 
But in the latest improved configuration note in the below link says to 
mask the calculated offset with 0x1F which might be for signed value as 
you mentioned above. But there was no info in the configuration note for 
the reason.

https://www.microchip.com/en-us/application-notes/an1760

Best regards,
Parthiban V
> 
>>> Also you can use GENMASK here.
>> Ah ok, it is GENMASK(4, 0) then.
>>
>> Best regards,
>> Parthiban V
>>>
>>>>    		if (ret & BIT(4))
>>>>    			offsets[i] = ret | 0xE0;
>>>>    		else
>>>> @@ -163,59 +181,88 @@ static int lan865x_write_cfg_params(struct phy_device *phydev,
>>>>    	return 0;
>>>>    }
>>>>    
>>>> -static int lan865x_setup_cfgparam(struct phy_device *phydev)
>>>> +static int lan865x_setup_cfgparam(struct phy_device *phydev, s8 offsets[])
>>>>    {
>>>>    	u16 cfg_results[ARRAY_SIZE(lan865x_revb0_fixup_cfg_regs)];
>>>>    	u16 cfg_params[ARRAY_SIZE(lan865x_revb0_fixup_cfg_regs)];
>>>> -	s8 offsets[2];
>>>>    	int ret;
>>>>    
>>>> -	ret = lan865x_generate_cfg_offsets(phydev, offsets);
>>>> +	ret = lan865x_read_cfg_params(phydev, lan865x_revb0_fixup_cfg_regs,
>>>> +				      cfg_params, ARRAY_SIZE(cfg_params));
>>>>    	if (ret)
>>>>    		return ret;
>>>>    
>>>> -	ret = lan865x_read_cfg_params(phydev, lan865x_revb0_fixup_cfg_regs,
>>>> +	cfg_results[0] = FIELD_PREP(GENMASK(15, 10), (9 + offsets[0]) & 0x3F) |
>>>> +			 FIELD_PREP(GENMASK(15, 4), (14 + offsets[0]) & 0x3F) |
>>>> +			 0x03;
>>>> +	cfg_results[1] = FIELD_PREP(GENMASK(15, 10), (40 + offsets[1]) & 0x3F);
>>>> +
>>>> +	return lan865x_write_cfg_params(phydev, lan865x_revb0_fixup_cfg_regs,
>>>> +					cfg_results, ARRAY_SIZE(cfg_results));
>>>> +}
>>>> +
>>>> +static int lan865x_setup_sqi_cfgparam(struct phy_device *phydev, s8 offsets[])
>>>> +{
>>>> +	u16 cfg_results[ARRAY_SIZE(lan865x_revb0_sqi_fixup_cfg_regs)];
>>>> +	u16 cfg_params[ARRAY_SIZE(lan865x_revb0_sqi_fixup_cfg_regs)];
>>>> +	int ret;
>>>> +
>>>> +	ret = lan865x_read_cfg_params(phydev, lan865x_revb0_sqi_fixup_cfg_regs,
>>>>    				      cfg_params, ARRAY_SIZE(cfg_params));
>>>>    	if (ret)
>>>>    		return ret;
>>>>    
>>>> -	cfg_results[0] = (cfg_params[0] & 0x000F) |
>>>> -			  FIELD_PREP(GENMASK(15, 10), 9 + offsets[0]) |
>>>> -			  FIELD_PREP(GENMASK(15, 4), 14 + offsets[0]);
>>>> -	cfg_results[1] = (cfg_params[1] & 0x03FF) |
>>>> -			  FIELD_PREP(GENMASK(15, 10), 40 + offsets[1]);
>>>> -	cfg_results[2] = (cfg_params[2] & 0xC0C0) |
>>>> -			  FIELD_PREP(GENMASK(15, 8), 5 + offsets[0]) |
>>>> -			  (9 + offsets[0]);
>>>> -	cfg_results[3] = (cfg_params[3] & 0xC0C0) |
>>>> -			  FIELD_PREP(GENMASK(15, 8), 9 + offsets[0]) |
>>>> -			  (14 + offsets[0]);
>>>> -	cfg_results[4] = (cfg_params[4] & 0xC0C0) |
>>>> -			  FIELD_PREP(GENMASK(15, 8), 17 + offsets[0]) |
>>>> -			  (22 + offsets[0]);
>>>> +	cfg_results[0] = FIELD_PREP(GENMASK(15, 8), (5 + offsets[0]) & 0x3F) |
>>>> +			 ((9 + offsets[0]) & 0x3F);
>>>> +	cfg_results[1] = FIELD_PREP(GENMASK(15, 8), (9 + offsets[0]) & 0x3F) |
>>>> +			 ((14 + offsets[0]) & 0x3F);
>>>> +	cfg_results[2] = FIELD_PREP(GENMASK(15, 8), (17 + offsets[0]) & 0x3F) |
>>>> +			 ((22 + offsets[0]) & 0x3F);
>>>>    
>>>> -	return lan865x_write_cfg_params(phydev, lan865x_revb0_fixup_cfg_regs,
>>>> +	return lan865x_write_cfg_params(phydev,
>>>> +					lan865x_revb0_sqi_fixup_cfg_regs,
>>>>    					cfg_results, ARRAY_SIZE(cfg_results));
>>>>    }
>>>>    
>>>>    static int lan865x_revb0_config_init(struct phy_device *phydev)
>>>>    {
>>>> +	s8 offsets[2];
>>>>    	int ret;
>>>>    
>>>>    	/* Reference to AN1760
>>>>    	 * https://ww1.microchip.com/downloads/aemDocuments/documents/AIS/ProductDocuments/SupportingCollateral/AN-LAN8650-1-Configuration-60001760.pdf
>>>>    	 */
>>>> +	ret = lan865x_generate_cfg_offsets(phydev, offsets);
>>>> +	if (ret)
>>>> +		return ret;
>>>> +
>>>>    	for (int i = 0; i < ARRAY_SIZE(lan865x_revb0_fixup_registers); i++) {
>>>>    		ret = phy_write_mmd(phydev, MDIO_MMD_VEND2,
>>>>    				    lan865x_revb0_fixup_registers[i],
>>>>    				    lan865x_revb0_fixup_values[i]);
>>>>    		if (ret)
>>>>    			return ret;
>>>> +
>>>> +		if (i == 1) {
>>>> +			ret = lan865x_setup_cfgparam(phydev, offsets);
>>>> +			if (ret)
>>>> +				return ret;
>>>> +		}
>>>>    	}
>>>> -	/* Function to calculate and write the configuration parameters in the
>>>> -	 * 0x0084, 0x008A, 0x00AD, 0x00AE and 0x00AF registers (from AN1760)
>>>> -	 */
>>>> -	return lan865x_setup_cfgparam(phydev);
>>>> +
>>>> +	ret = lan865x_setup_sqi_cfgparam(phydev, offsets);
>>>> +	if (ret)
>>>> +		return ret;
>>>> +
>>>> +	for (int i = 0; i < ARRAY_SIZE(lan865x_revb0_sqi_fixup_regs); i++) {
>>>> +		ret = phy_write_mmd(phydev, MDIO_MMD_VEND2,
>>>> +				    lan865x_revb0_sqi_fixup_regs[i],
>>>> +				    lan865x_revb0_sqi_fixup_values[i]);
>>>> +		if (ret)
>>>> +			return ret;
>>>> +	}
>>>> +
>>>> +	return 0;
>>>>    }
>>>>    
>>>>    static int lan867x_revb1_config_init(struct phy_device *phydev)
>>>> -- 
>>>> 2.34.1
>>>>
>>>
>>
> 

Re: [PATCH net-next v2 7/7] net: phy: microchip_t1s: configure collision detection based on PLCA mode

[Parthiban.Veerasooran]

Hi Horatiu,

On 06/09/24 12:13 pm, Horatiu Vultur - M31836 wrote:
> The 09/04/2024 11:46, Parthiban Veerasooran - I17164 wrote:
>> Hi Horatiu,
>>
>> On 03/09/24 12:13 pm, Horatiu Vultur wrote:
>>> The 09/02/2024 20:04, Parthiban Veerasooran wrote:
>>>
>>> Hi Parthiban,
>>>
>>>> As per LAN8650/1 Rev.B0/B1 AN1760 (Revision F (DS60001760G - June 2024))
>>>> and LAN8670/1/2 Rev.C1/C2 AN1699 (Revision E (DS60001699F - June 2024)),
>>>> under normal operation, the device should be operated in PLCA mode.
>>>> Disabling collision detection is recommended to allow the device to
>>>> operate in noisy environments or when reflections and other inherent
>>>> transmission line distortion cause poor signal quality. Collision
>>>> detection must be re-enabled if the device is configured to operate in
>>>> CSMA/CD mode.
>>>
>>> Is this something that applies only to Microchip PHYs or is something
>>> generic that applies to all the PHYs.
>> Yes, the behavior is common for all the PHYs but it is purely up to the
>> PHY chip design specific.
>>
>> 1. Some vendors will enable this feature in the chip level by latching
>> the register bits. There we don't need software interface.
>> 2. Some vendors need software interface to enable this feature like our
>> Microchip PHY does.
> 
> Don't you think then is better to create something more generic so other
> PHYs to able to do this?
Ya, it is a good idea but the offset for the Collision Detector control 
register is vendor specific resulting each vendor will have different 
offsets. So we can't make it as a generic helper function.

Alternatively, we can provide a function pointer interface in the 
"struct phy_driver" which can implement the collision detection 
disable/enable functionality logic in the PHY driver. 
genphy_c45_plca_set_cfg() function in the phy-c45.c can call this 
function if it is implemented/valid in the PHY driver. Do you mean 
something like this?

In the above case also the PHY driver needs to implement the logic as 
the register offset will be different for different vendors.  Is this my 
understanding correct or do you have any other proposal?

Best regards,
Parthiban V
> 
>>
>> Best regards,
>> Parthiban V
>>>
>>>>
>>>> Signed-off-by: Parthiban Veerasooran <Parthiban.Veerasooran@microchip.com>
>>>> ---
>>>>    drivers/net/phy/microchip_t1s.c | 42 ++++++++++++++++++++++++++++++---
>>>>    1 file changed, 39 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/net/phy/microchip_t1s.c b/drivers/net/phy/microchip_t1s.c
>>>> index bd0c768df0af..a0565508d7d2 100644
>>>> --- a/drivers/net/phy/microchip_t1s.c
>>>> +++ b/drivers/net/phy/microchip_t1s.c
>>>> @@ -26,6 +26,12 @@
>>>>    #define LAN865X_REG_CFGPARAM_CTRL 0x00DA
>>>>    #define LAN865X_REG_STS2 0x0019
>>>>    
>>>> +/* Collision Detector Control 0 Register */
>>>> +#define LAN86XX_REG_COL_DET_CTRL0	0x0087
>>>> +#define COL_DET_CTRL0_ENABLE_BIT_MASK	BIT(15)
>>>> +#define COL_DET_ENABLE			BIT(15)
>>>> +#define COL_DET_DISABLE			0x0000
>>>> +
>>>>    #define LAN865X_CFGPARAM_READ_ENABLE BIT(1)
>>>>    
>>>>    /* The arrays below are pulled from the following table from AN1699
>>>> @@ -370,6 +376,36 @@ static int lan867x_revb1_config_init(struct phy_device *phydev)
>>>>    	return 0;
>>>>    }
>>>>    
>>>> +/* As per LAN8650/1 Rev.B0/B1 AN1760 (Revision F (DS60001760G - June 2024)) and
>>>> + * LAN8670/1/2 Rev.C1/C2 AN1699 (Revision E (DS60001699F - June 2024)), under
>>>> + * normal operation, the device should be operated in PLCA mode. Disabling
>>>> + * collision detection is recommended to allow the device to operate in noisy
>>>> + * environments or when reflections and other inherent transmission line
>>>> + * distortion cause poor signal quality. Collision detection must be re-enabled
>>>> + * if the device is configured to operate in CSMA/CD mode.
>>>> + *
>>>> + * AN1760: https://www.microchip.com/en-us/application-notes/an1760
>>>> + * AN1699: https://www.microchip.com/en-us/application-notes/an1699
>>>> + */
>>>> +static int lan86xx_plca_set_cfg(struct phy_device *phydev,
>>>> +				const struct phy_plca_cfg *plca_cfg)
>>>> +{
>>>> +	int ret;
>>>> +
>>>> +	ret = genphy_c45_plca_set_cfg(phydev, plca_cfg);
>>>> +	if (ret)
>>>> +		return ret;
>>>> +
>>>> +	if (plca_cfg->enabled)
>>>> +		return phy_modify_mmd(phydev, MDIO_MMD_VEND2,
>>>> +				      LAN86XX_REG_COL_DET_CTRL0,
>>>> +				      COL_DET_CTRL0_ENABLE_BIT_MASK,
>>>> +				      COL_DET_DISABLE);
>>>> +
>>>> +	return phy_modify_mmd(phydev, MDIO_MMD_VEND2, LAN86XX_REG_COL_DET_CTRL0,
>>>> +			      COL_DET_CTRL0_ENABLE_BIT_MASK, COL_DET_ENABLE);
>>>> +}
>>>> +
>>>>    static int lan86xx_read_status(struct phy_device *phydev)
>>>>    {
>>>>    	/* The phy has some limitations, namely:
>>>> @@ -403,7 +439,7 @@ static struct phy_driver microchip_t1s_driver[] = {
>>>>    		.config_init        = lan867x_revc_config_init,
>>>>    		.read_status        = lan86xx_read_status,
>>>>    		.get_plca_cfg	    = genphy_c45_plca_get_cfg,
>>>> -		.set_plca_cfg	    = genphy_c45_plca_set_cfg,
>>>> +		.set_plca_cfg	    = lan86xx_plca_set_cfg,
>>>>    		.get_plca_status    = genphy_c45_plca_get_status,
>>>>    	},
>>>>    	{
>>>> @@ -413,7 +449,7 @@ static struct phy_driver microchip_t1s_driver[] = {
>>>>    		.config_init        = lan867x_revc_config_init,
>>>>    		.read_status        = lan86xx_read_status,
>>>>    		.get_plca_cfg	    = genphy_c45_plca_get_cfg,
>>>> -		.set_plca_cfg	    = genphy_c45_plca_set_cfg,
>>>> +		.set_plca_cfg	    = lan86xx_plca_set_cfg,
>>>>    		.get_plca_status    = genphy_c45_plca_get_status,
>>>>    	},
>>>>    	{
>>>> @@ -423,7 +459,7 @@ static struct phy_driver microchip_t1s_driver[] = {
>>>>    		.config_init        = lan865x_revb_config_init,
>>>>    		.read_status        = lan86xx_read_status,
>>>>    		.get_plca_cfg	    = genphy_c45_plca_get_cfg,
>>>> -		.set_plca_cfg	    = genphy_c45_plca_set_cfg,
>>>> +		.set_plca_cfg	    = lan86xx_plca_set_cfg,
>>>>    		.get_plca_status    = genphy_c45_plca_get_status,
>>>>    	},
>>>>    };
>>>> -- 
>>>> 2.34.1
>>>>
>>>
>>
>