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

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

[Parthiban Veerasooran]

This patch series contain the below updates,
- Fixes on the Microchip LAN8670/1/2 10BASE-T1S PHYs support in the
  net/phy/microchip_t1s.c driver.
- Adds support for the Microchip LAN8650/1 Rev.B0 10BASE-T1S Internal
  PHYs in the net/phy/microchip_t1s.c driver.

Changes:
v2:
- Updated cover letter contents.
- Modified driver description is more generic as it is common for all the
  Microchip 10BASE-T1S PHYs.
- Replaced read-modify-write code with phy_modify_mmd function.
- Moved */ to the same line for the single line comments.
- Changed the type int to u16 for LAN865X Rev.B0 fixup registers
  declaration.
- Changed all the comments starting letter to upper case for the
  consistency.
- Removed return value check of phy_read_mmd and returned directly in the
  last line of the function lan865x_revb0_indirect_read.
- Used reverse christmas notation wherever is possible.
- Used FIELD_PREP instead of << in all the places.
- Used 4 byte representation for all the register addresses and values
  for consistency.
- Comment for indirect read is modified.
- Implemented "Reset Complete" status polling in config_init.
- Function lan865x_setup_cfgparam is split into multiple functions for
  readability.
- Reference to AN1760 document is added in the comment.
- Removed interrupt disabling code as it is not needed.
- Provided meaningful macros for the LAN865X Rev.B0 indirect read
  registers and control.
- Replaced 0x10 with BIT(4).
- Removed collision detection disable/enable code as it can be done with
  a separate patch later.

v3:
- Comment for phy_modify_mmd() is extended to indicate that the write is
  not required if the register already has the required value.
- Commit message is updated for the not supported hardware revisions
  0x0007C160 (Rev.A0) and 0x0007C161 (Rev.B0) since they are never
  released to production.
- Commit message is updated to indicate that the Reset Complete interrupt
  will be cleared when the STS2 register read is done.
- Corrected the typo in the offset calculation comment.
- Used reverse christmas notation for the local variable declarations.

Parthiban Veerasooran (6):
  net: phy: microchip_t1s: modify driver description to be more generic
  net: phy: microchip_t1s: replace read-modify-write code with
    phy_modify_mmd
  net: phy: microchip_t1s: update LAN867x PHY supported revision number
  net: phy: microchip_t1s: fix reset complete status handling
  net: phy: microchip_t1s: remove unnecessary interrupts disabling code
  net: phy: microchip_t1s: add support for Microchip LAN865x Rev.B0 PHYs

 drivers/net/phy/microchip_t1s.c | 269 +++++++++++++++++++++++++++-----
 1 file changed, 229 insertions(+), 40 deletions(-)

-- 
2.34.1

[PATCH net-next v3 1/6] net: phy: microchip_t1s: modify driver description to be more generic

[Parthiban Veerasooran]

Remove LAN867X from the driver description as this driver is common for
all the Microchip 10BASE-T1S PHYs.

Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Parthiban Veerasooran <Parthiban.Veerasooran@microchip.com>
---
 drivers/net/phy/microchip_t1s.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/phy/microchip_t1s.c b/drivers/net/phy/microchip_t1s.c
index 094967b3c111..a42a6bb6e3bd 100644
--- a/drivers/net/phy/microchip_t1s.c
+++ b/drivers/net/phy/microchip_t1s.c
@@ -1,6 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0+
 /*
- * Driver for Microchip 10BASE-T1S LAN867X PHY
+ * Driver for Microchip 10BASE-T1S PHYs
  *
  * Support: Microchip Phys:
  *  lan8670, lan8671, lan8672
@@ -111,7 +111,7 @@ static int lan867x_read_status(struct phy_device *phydev)
 	return 0;
 }
 
-static struct phy_driver lan867x_driver[] = {
+static struct phy_driver microchip_t1s_driver[] = {
 	{
 		PHY_ID_MATCH_MODEL(PHY_ID_LAN867X),
 		.name               = "LAN867X",
@@ -124,7 +124,7 @@ static struct phy_driver lan867x_driver[] = {
 	}
 };
 
-module_phy_driver(lan867x_driver);
+module_phy_driver(microchip_t1s_driver);
 
 static struct mdio_device_id __maybe_unused tbl[] = {
 	{ PHY_ID_MATCH_MODEL(PHY_ID_LAN867X) },
@@ -133,6 +133,6 @@ static struct mdio_device_id __maybe_unused tbl[] = {
 
 MODULE_DEVICE_TABLE(mdio, tbl);
 
-MODULE_DESCRIPTION("Microchip 10BASE-T1S lan867x Phy driver");
+MODULE_DESCRIPTION("Microchip 10BASE-T1S PHYs driver");
 MODULE_AUTHOR("Ramón Nordin Rodriguez");
 MODULE_LICENSE("GPL");
-- 
2.34.1

[PATCH net-next v3 2/6] net: phy: microchip_t1s: replace read-modify-write code with phy_modify_mmd

[Parthiban Veerasooran]

Replace read-modify-write code in the lan867x_config_init function to
avoid handling data type mismatch and to simplify the code.

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

diff --git a/drivers/net/phy/microchip_t1s.c b/drivers/net/phy/microchip_t1s.c
index a42a6bb6e3bd..b5b5a95fa6e7 100644
--- a/drivers/net/phy/microchip_t1s.c
+++ b/drivers/net/phy/microchip_t1s.c
@@ -31,19 +31,19 @@
  * W   0x1F 0x0099 0x7F80 ------
  */
 
-static const int lan867x_fixup_registers[12] = {
+static const u32 lan867x_fixup_registers[12] = {
 	0x00D0, 0x00D1, 0x0084, 0x0085,
 	0x008A, 0x0087, 0x0088, 0x008B,
 	0x0080, 0x00F1, 0x0096, 0x0099,
 };
 
-static const int lan867x_fixup_values[12] = {
+static const u16 lan867x_fixup_values[12] = {
 	0x0002, 0x0000, 0x3380, 0x0006,
 	0xC000, 0x801C, 0x033F, 0x0404,
 	0x0600, 0x2400, 0x2000, 0x7F80,
 };
 
-static const int lan867x_fixup_masks[12] = {
+static const u16 lan867x_fixup_masks[12] = {
 	0x0E03, 0x0300, 0xFFC0, 0x000F,
 	0xF800, 0x801C, 0x1FFF, 0xFFFF,
 	0x0600, 0x7F00, 0x2000, 0xFFFF,
@@ -63,23 +63,22 @@ static int lan867x_config_init(struct phy_device *phydev)
 	 * used, which might then write the same value back as read + modified.
 	 */
 
-	int reg_value;
 	int err;
-	int reg;
 
 	/* Read-Modified Write Pseudocode (from AN1699)
 	 * current_val = read_register(mmd, addr) // Read current register value
 	 * new_val = current_val AND (NOT mask) // Clear bit fields to be written
 	 * new_val = new_val OR value // Set bits
-	 * write_register(mmd, addr, new_val) // Write back updated register value
+	 * write_register(mmd, addr, new_val) // Write back updated register value.
+	 * Although AN1699 says Read, Modify, Write, the write is not required if
+	 * the register already has the required value.
 	 */
 	for (int i = 0; i < ARRAY_SIZE(lan867x_fixup_registers); i++) {
-		reg = lan867x_fixup_registers[i];
-		reg_value = phy_read_mmd(phydev, MDIO_MMD_VEND2, reg);
-		reg_value &= ~lan867x_fixup_masks[i];
-		reg_value |= lan867x_fixup_values[i];
-		err = phy_write_mmd(phydev, MDIO_MMD_VEND2, reg, reg_value);
-		if (err != 0)
+		err = phy_modify_mmd(phydev, MDIO_MMD_VEND2,
+				     lan867x_fixup_registers[i],
+				     lan867x_fixup_masks[i],
+				     lan867x_fixup_values[i]);
+		if (err)
 			return err;
 	}
 
-- 
2.34.1

[PATCH net-next v3 3/6] net: phy: microchip_t1s: update LAN867x PHY supported revision number

[Parthiban Veerasooran]

As per AN1699, the initial configuration in the driver applies to LAN867x
Rev.B1 hardware revision. 0x0007C160 (Rev.A0) and 0x0007C161 (Rev.B0)
never released to production and hence they don't need to be supported.

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

diff --git a/drivers/net/phy/microchip_t1s.c b/drivers/net/phy/microchip_t1s.c
index b5b5a95fa6e7..8f29d9802131 100644
--- a/drivers/net/phy/microchip_t1s.c
+++ b/drivers/net/phy/microchip_t1s.c
@@ -3,14 +3,14 @@
  * Driver for Microchip 10BASE-T1S PHYs
  *
  * Support: Microchip Phys:
- *  lan8670, lan8671, lan8672
+ *  lan8670/1/2 Rev.B1
  */
 
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/phy.h>
 
-#define PHY_ID_LAN867X 0x0007C160
+#define PHY_ID_LAN867X_REVB1 0x0007C162
 
 #define LAN867X_REG_IRQ_1_CTL 0x001C
 #define LAN867X_REG_IRQ_2_CTL 0x001D
@@ -31,25 +31,25 @@
  * W   0x1F 0x0099 0x7F80 ------
  */
 
-static const u32 lan867x_fixup_registers[12] = {
+static const u32 lan867x_revb1_fixup_registers[12] = {
 	0x00D0, 0x00D1, 0x0084, 0x0085,
 	0x008A, 0x0087, 0x0088, 0x008B,
 	0x0080, 0x00F1, 0x0096, 0x0099,
 };
 
-static const u16 lan867x_fixup_values[12] = {
+static const u16 lan867x_revb1_fixup_values[12] = {
 	0x0002, 0x0000, 0x3380, 0x0006,
 	0xC000, 0x801C, 0x033F, 0x0404,
 	0x0600, 0x2400, 0x2000, 0x7F80,
 };
 
-static const u16 lan867x_fixup_masks[12] = {
+static const u16 lan867x_revb1_fixup_masks[12] = {
 	0x0E03, 0x0300, 0xFFC0, 0x000F,
 	0xF800, 0x801C, 0x1FFF, 0xFFFF,
 	0x0600, 0x7F00, 0x2000, 0xFFFF,
 };
 
-static int lan867x_config_init(struct phy_device *phydev)
+static int lan867x_revb1_config_init(struct phy_device *phydev)
 {
 	/* HW quirk: Microchip states in the application note (AN1699) for the phy
 	 * that a set of read-modify-write (rmw) operations has to be performed
@@ -73,11 +73,11 @@ static int lan867x_config_init(struct phy_device *phydev)
 	 * Although AN1699 says Read, Modify, Write, the write is not required if
 	 * the register already has the required value.
 	 */
-	for (int i = 0; i < ARRAY_SIZE(lan867x_fixup_registers); i++) {
+	for (int i = 0; i < ARRAY_SIZE(lan867x_revb1_fixup_registers); i++) {
 		err = phy_modify_mmd(phydev, MDIO_MMD_VEND2,
-				     lan867x_fixup_registers[i],
-				     lan867x_fixup_masks[i],
-				     lan867x_fixup_values[i]);
+				     lan867x_revb1_fixup_registers[i],
+				     lan867x_revb1_fixup_masks[i],
+				     lan867x_revb1_fixup_values[i]);
 		if (err)
 			return err;
 	}
@@ -112,10 +112,10 @@ static int lan867x_read_status(struct phy_device *phydev)
 
 static struct phy_driver microchip_t1s_driver[] = {
 	{
-		PHY_ID_MATCH_MODEL(PHY_ID_LAN867X),
-		.name               = "LAN867X",
+		PHY_ID_MATCH_EXACT(PHY_ID_LAN867X_REVB1),
+		.name               = "LAN867X Rev.B1",
 		.features           = PHY_BASIC_T1S_P2MP_FEATURES,
-		.config_init        = lan867x_config_init,
+		.config_init        = lan867x_revb1_config_init,
 		.read_status        = lan867x_read_status,
 		.get_plca_cfg	    = genphy_c45_plca_get_cfg,
 		.set_plca_cfg	    = genphy_c45_plca_set_cfg,
@@ -126,7 +126,7 @@ static struct phy_driver microchip_t1s_driver[] = {
 module_phy_driver(microchip_t1s_driver);
 
 static struct mdio_device_id __maybe_unused tbl[] = {
-	{ PHY_ID_MATCH_MODEL(PHY_ID_LAN867X) },
+	{ PHY_ID_MATCH_EXACT(PHY_ID_LAN867X_REVB1) },
 	{ }
 };
 
-- 
2.34.1

[PATCH net-next v3 4/6] net: phy: microchip_t1s: fix reset complete status handling

[Parthiban Veerasooran]

As per the datasheet DS-LAN8670-1-2-60001573C.pdf, the Reset Complete
status bit in the STS2 register has to be checked before proceeding to
the initial configuration. Reading STS2 register will also clear the
Reset Complete interrupt which is non-maskable.

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

diff --git a/drivers/net/phy/microchip_t1s.c b/drivers/net/phy/microchip_t1s.c
index 8f29d9802131..05a88b561993 100644
--- a/drivers/net/phy/microchip_t1s.c
+++ b/drivers/net/phy/microchip_t1s.c
@@ -14,6 +14,9 @@
 
 #define LAN867X_REG_IRQ_1_CTL 0x001C
 #define LAN867X_REG_IRQ_2_CTL 0x001D
+#define LAN867X_REG_STS2 0x0019
+
+#define LAN867x_RESET_COMPLETE_STS BIT(11)
 
 /* The arrays below are pulled from the following table from AN1699
  * Access MMD Address Value Mask
@@ -65,6 +68,27 @@ static int lan867x_revb1_config_init(struct phy_device *phydev)
 
 	int err;
 
+	/* Read STS2 register and check for the Reset Complete status to do the
+	 * init configuration. If the Reset Complete is not set, wait for 5us
+	 * and then read STS2 register again and check for Reset Complete status.
+	 * Still if it is failed then declare PHY reset error or else proceed
+	 * for the PHY initial register configuration.
+	 */
+	err = phy_read_mmd(phydev, MDIO_MMD_VEND2, LAN867X_REG_STS2);
+	if (err < 0)
+		return err;
+
+	if (!(err & LAN867x_RESET_COMPLETE_STS)) {
+		udelay(5);
+		err = phy_read_mmd(phydev, MDIO_MMD_VEND2, LAN867X_REG_STS2);
+		if (err < 0)
+			return err;
+		if (!(err & LAN867x_RESET_COMPLETE_STS)) {
+			phydev_err(phydev, "PHY reset failed\n");
+			return -ENODEV;
+		}
+	}
+
 	/* Read-Modified Write Pseudocode (from AN1699)
 	 * current_val = read_register(mmd, addr) // Read current register value
 	 * new_val = current_val AND (NOT mask) // Clear bit fields to be written
-- 
2.34.1

[PATCH net-next v3 5/6] net: phy: microchip_t1s: remove unnecessary interrupts disabling code

[Parthiban Veerasooran]

By default, except Reset Complete interrupt in the Interrupt Mask 2
Register all other interrupts are disabled/masked. As Reset Complete
status is already handled, it doesn't make sense to disable it.

Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Parthiban Veerasooran <Parthiban.Veerasooran@microchip.com>
---
 drivers/net/phy/microchip_t1s.c | 14 +-------------
 1 file changed, 1 insertion(+), 13 deletions(-)

diff --git a/drivers/net/phy/microchip_t1s.c b/drivers/net/phy/microchip_t1s.c
index 05a88b561993..6f9e197d8623 100644
--- a/drivers/net/phy/microchip_t1s.c
+++ b/drivers/net/phy/microchip_t1s.c
@@ -12,8 +12,6 @@
 
 #define PHY_ID_LAN867X_REVB1 0x0007C162
 
-#define LAN867X_REG_IRQ_1_CTL 0x001C
-#define LAN867X_REG_IRQ_2_CTL 0x001D
 #define LAN867X_REG_STS2 0x0019
 
 #define LAN867x_RESET_COMPLETE_STS BIT(11)
@@ -106,17 +104,7 @@ static int lan867x_revb1_config_init(struct phy_device *phydev)
 			return err;
 	}
 
-	/* None of the interrupts in the lan867x phy seem relevant.
-	 * Other phys inspect the link status and call phy_trigger_machine
-	 * in the interrupt handler.
-	 * This phy does not support link status, and thus has no interrupt
-	 * for it either.
-	 * So we'll just disable all interrupts on the chip.
-	 */
-	err = phy_write_mmd(phydev, MDIO_MMD_VEND2, LAN867X_REG_IRQ_1_CTL, 0xFFFF);
-	if (err != 0)
-		return err;
-	return phy_write_mmd(phydev, MDIO_MMD_VEND2, LAN867X_REG_IRQ_2_CTL, 0xFFFF);
+	return 0;
 }
 
 static int lan867x_read_status(struct phy_device *phydev)
-- 
2.34.1

[PATCH net-next v3 6/6] net: phy: microchip_t1s: add support for Microchip LAN865x Rev.B0 PHYs

[Parthiban Veerasooran]

Add support for the Microchip LAN865x Rev.B0 10BASE-T1S Internal PHYs
(LAN8650/1). The LAN865x combines a Media Access Controller (MAC) and an
internal 10BASE-T1S Ethernet PHY to access 10BASE‑T1S networks. As
LAN867X and LAN865X are using the same function for the read_status,
rename the function as lan86xx_read_status.

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

diff --git a/drivers/net/phy/microchip_t1s.c b/drivers/net/phy/microchip_t1s.c
index 6f9e197d8623..00c4c23906ce 100644
--- a/drivers/net/phy/microchip_t1s.c
+++ b/drivers/net/phy/microchip_t1s.c
@@ -4,6 +4,7 @@
  *
  * Support: Microchip Phys:
  *  lan8670/1/2 Rev.B1
+ *  lan8650/1 Rev.B0 Internal PHYs
  */
 
 #include <linux/kernel.h>
@@ -11,11 +12,19 @@
 #include <linux/phy.h>
 
 #define PHY_ID_LAN867X_REVB1 0x0007C162
+#define PHY_ID_LAN865X_REVB0 0x0007C1B3
 
 #define LAN867X_REG_STS2 0x0019
 
 #define LAN867x_RESET_COMPLETE_STS BIT(11)
 
+#define LAN865X_REG_CFGPARAM_ADDR 0x00D8
+#define LAN865X_REG_CFGPARAM_DATA 0x00D9
+#define LAN865X_REG_CFGPARAM_CTRL 0x00DA
+#define LAN865X_REG_STS2 0x0019
+
+#define LAN865X_CFGPARAM_READ_ENABLE BIT(1)
+
 /* The arrays below are pulled from the following table from AN1699
  * Access MMD Address Value Mask
  * RMW 0x1F 0x00D0 0x0002 0x0E03
@@ -50,6 +59,164 @@ 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,
+	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,
+	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
+};
+
+/* Pulled from AN1760 describing 'indirect read'
+ *
+ * write_register(0x4, 0x00D8, addr)
+ * write_register(0x4, 0x00DA, 0x2)
+ * return (int8)(read_register(0x4, 0x00D9))
+ *
+ * 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)
+{
+	int ret;
+
+	ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, LAN865X_REG_CFGPARAM_ADDR,
+			    addr);
+	if (ret)
+		return ret;
+
+	ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, LAN865X_REG_CFGPARAM_CTRL,
+			    LAN865X_CFGPARAM_READ_ENABLE);
+	if (ret)
+		return ret;
+
+	return phy_read_mmd(phydev, MDIO_MMD_VEND2, LAN865X_REG_CFGPARAM_DATA);
+}
+
+/* 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])
+{
+	const u16 fixup_regs[2] = {0x0004, 0x0008};
+	int ret;
+
+	for (int i = 0; i < ARRAY_SIZE(fixup_regs); i++) {
+		ret = lan865x_revb0_indirect_read(phydev, fixup_regs[i]);
+		if (ret < 0)
+			return ret;
+		if (ret & BIT(4))
+			offsets[i] = ret | 0xE0;
+		else
+			offsets[i] = ret;
+	}
+
+	return 0;
+}
+
+static int lan865x_read_cfg_params(struct phy_device *phydev, u16 cfg_params[])
+{
+	int ret;
+
+	for (int i = 0; i < ARRAY_SIZE(lan865x_revb0_fixup_cfg_regs); i++) {
+		ret = phy_read_mmd(phydev, MDIO_MMD_VEND2,
+				   lan865x_revb0_fixup_cfg_regs[i]);
+		if (ret < 0)
+			return ret;
+		cfg_params[i] = (u16)ret;
+	}
+
+	return 0;
+}
+
+static int lan865x_write_cfg_params(struct phy_device *phydev, u16 cfg_params[])
+{
+	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],
+				    cfg_params[i]);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static int lan865x_setup_cfgparam(struct phy_device *phydev)
+{
+	u16 cfg_params[ARRAY_SIZE(lan865x_revb0_fixup_cfg_regs)];
+	u16 cfg_results[5];
+	s8 offsets[2];
+	int ret;
+
+	ret = lan865x_generate_cfg_offsets(phydev, offsets);
+	if (ret)
+		return ret;
+
+	ret = lan865x_read_cfg_params(phydev, 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]);
+
+	return lan865x_write_cfg_params(phydev, cfg_results);
+}
+
+static int lan865x_revb0_config_init(struct phy_device *phydev)
+{
+	int ret;
+
+	/* Reference to AN1760
+	 * https://ww1.microchip.com/downloads/aemDocuments/documents/AIS/ProductDocuments/SupportingCollateral/AN-LAN8650-1-Configuration-60001760.pdf
+	 */
+	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;
+	}
+	/* Function to calculate and write the configuration parameters in the
+	 * 0x0084, 0x008A, 0x00AD, 0x00AE and 0x00AF registers (from AN1760)
+	 */
+	ret = lan865x_setup_cfgparam(phydev);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
 static int lan867x_revb1_config_init(struct phy_device *phydev)
 {
 	/* HW quirk: Microchip states in the application note (AN1699) for the phy
@@ -107,7 +274,7 @@ static int lan867x_revb1_config_init(struct phy_device *phydev)
 	return 0;
 }
 
-static int lan867x_read_status(struct phy_device *phydev)
+static int lan86xx_read_status(struct phy_device *phydev)
 {
 	/* The phy has some limitations, namely:
 	 *  - always reports link up
@@ -128,17 +295,28 @@ static struct phy_driver microchip_t1s_driver[] = {
 		.name               = "LAN867X Rev.B1",
 		.features           = PHY_BASIC_T1S_P2MP_FEATURES,
 		.config_init        = lan867x_revb1_config_init,
-		.read_status        = lan867x_read_status,
+		.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_REVB0),
+		.name               = "LAN865X Rev.B0 Internal Phy",
+		.features           = PHY_BASIC_T1S_P2MP_FEATURES,
+		.config_init        = lan865x_revb0_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,
+	},
 };
 
 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) },
 	{ }
 };
 
-- 
2.34.1

Re: [PATCH net-next v3 2/6] net: phy: microchip_t1s: replace read-modify-write code with phy_modify_mmd

[Andrew Lunn]

On Wed, May 24, 2023 at 08:15:35PM +0530, Parthiban Veerasooran wrote:
> Replace read-modify-write code in the lan867x_config_init function to
> avoid handling data type mismatch and to simplify the code.
> 
> Signed-off-by: Parthiban Veerasooran <Parthiban.Veerasooran@microchip.com>

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

    Andrew

Re: [PATCH net-next v3 3/6] net: phy: microchip_t1s: update LAN867x PHY supported revision number

[Andrew Lunn]

On Wed, May 24, 2023 at 08:15:36PM +0530, Parthiban Veerasooran wrote:
> As per AN1699, the initial configuration in the driver applies to LAN867x
> Rev.B1 hardware revision. 0x0007C160 (Rev.A0) and 0x0007C161 (Rev.B0)
> never released to production and hence they don't need to be supported.
> 
> Signed-off-by: Parthiban Veerasooran <Parthiban.Veerasooran@microchip.com>

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

    Andrew

Re: [PATCH net-next v3 4/6] net: phy: microchip_t1s: fix reset complete status handling

[Andrew Lunn]

On Wed, May 24, 2023 at 08:15:37PM +0530, Parthiban Veerasooran wrote:
> As per the datasheet DS-LAN8670-1-2-60001573C.pdf, the Reset Complete
> status bit in the STS2 register has to be checked before proceeding to
> the initial configuration. Reading STS2 register will also clear the
> Reset Complete interrupt which is non-maskable.
> 
> Signed-off-by: Parthiban Veerasooran <Parthiban.Veerasooran@microchip.com>

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

    Andrew

Re: [PATCH net-next v3 6/6] net: phy: microchip_t1s: add support for Microchip LAN865x Rev.B0 PHYs

[Andrew Lunn]

On Wed, May 24, 2023 at 08:15:39PM +0530, Parthiban Veerasooran wrote:
> Add support for the Microchip LAN865x Rev.B0 10BASE-T1S Internal PHYs
> (LAN8650/1). The LAN865x combines a Media Access Controller (MAC) and an
> internal 10BASE-T1S Ethernet PHY to access 10BASE‑T1S networks. As
> LAN867X and LAN865X are using the same function for the read_status,
> rename the function as lan86xx_read_status.
> 
> Signed-off-by: Parthiban Veerasooran <Parthiban.Veerasooran@microchip.com>

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

    Andrew

Re: [PATCH net-next v3 2/6] net: phy: microchip_t1s: replace read-modify-write code with phy_modify_mmd

[Ramón Nordin Rodriguez]

On Wed, May 24, 2023 at 08:15:35PM +0530, Parthiban Veerasooran wrote:
> Replace read-modify-write code in the lan867x_config_init function to
> avoid handling data type mismatch and to simplify the code.
> 
> Signed-off-by: Parthiban Veerasooran <Parthiban.Veerasooran@microchip.com>
> ---
>  drivers/net/phy/microchip_t1s.c | 23 +++++++++++------------
>  1 file changed, 11 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/phy/microchip_t1s.c b/drivers/net/phy/microchip_t1s.c
> index a42a6bb6e3bd..b5b5a95fa6e7 100644
> --- a/drivers/net/phy/microchip_t1s.c
> +++ b/drivers/net/phy/microchip_t1s.c
> @@ -31,19 +31,19 @@
>   * W   0x1F 0x0099 0x7F80 ------
>   */
>  
> -static const int lan867x_fixup_registers[12] = {
> +static const u32 lan867x_fixup_registers[12] = {
>  	0x00D0, 0x00D1, 0x0084, 0x0085,
>  	0x008A, 0x0087, 0x0088, 0x008B,
>  	0x0080, 0x00F1, 0x0096, 0x0099,
>  };
>  
> -static const int lan867x_fixup_values[12] = {
> +static const u16 lan867x_fixup_values[12] = {
>  	0x0002, 0x0000, 0x3380, 0x0006,
>  	0xC000, 0x801C, 0x033F, 0x0404,
>  	0x0600, 0x2400, 0x2000, 0x7F80,
>  };
>  
> -static const int lan867x_fixup_masks[12] = {
> +static const u16 lan867x_fixup_masks[12] = {
>  	0x0E03, 0x0300, 0xFFC0, 0x000F,
>  	0xF800, 0x801C, 0x1FFF, 0xFFFF,
>  	0x0600, 0x7F00, 0x2000, 0xFFFF,
> @@ -63,23 +63,22 @@ static int lan867x_config_init(struct phy_device *phydev)
>  	 * used, which might then write the same value back as read + modified.
>  	 */
>  
> -	int reg_value;
>  	int err;
> -	int reg;
>  
>  	/* Read-Modified Write Pseudocode (from AN1699)
>  	 * current_val = read_register(mmd, addr) // Read current register value
>  	 * new_val = current_val AND (NOT mask) // Clear bit fields to be written
>  	 * new_val = new_val OR value // Set bits
> -	 * write_register(mmd, addr, new_val) // Write back updated register value
> +	 * write_register(mmd, addr, new_val) // Write back updated register value.
> +	 * Although AN1699 says Read, Modify, Write, the write is not required if
> +	 * the register already has the required value.
>  	 */

Nitpick, I think this block comment can be reduced to:
/* The following block deviates from AN1699 which states that a values
 * should be written back, even if unmodified.
 * Which is not necessary, so it's safe to use phy_modify_mmd here.*/

 The comment I added was intended to describe why I was doing weird
 things, but now I think it's more interesting to describe why we're
 deviating from the AN.

 Or the block comment could be dropped all togheter, I'm guessing no one
 is going to consult the AN if things 'just work'

>  	for (int i = 0; i < ARRAY_SIZE(lan867x_fixup_registers); i++) {
> -		reg = lan867x_fixup_registers[i];
> -		reg_value = phy_read_mmd(phydev, MDIO_MMD_VEND2, reg);
> -		reg_value &= ~lan867x_fixup_masks[i];
> -		reg_value |= lan867x_fixup_values[i];
> -		err = phy_write_mmd(phydev, MDIO_MMD_VEND2, reg, reg_value);
> -		if (err != 0)
> +		err = phy_modify_mmd(phydev, MDIO_MMD_VEND2,
> +				     lan867x_fixup_registers[i],
> +				     lan867x_fixup_masks[i],
> +				     lan867x_fixup_values[i]);
> +		if (err)
>  			return err;
>  	}
>  
> -- 
> 2.34.1
> 

Re: [PATCH net-next v3 6/6] net: phy: microchip_t1s: add support for Microchip LAN865x Rev.B0 PHYs

[Ramón Nordin Rodriguez]

On Wed, May 24, 2023 at 08:15:39PM +0530, Parthiban Veerasooran wrote:
> Add support for the Microchip LAN865x Rev.B0 10BASE-T1S Internal PHYs
> (LAN8650/1). The LAN865x combines a Media Access Controller (MAC) and an
> internal 10BASE-T1S Ethernet PHY to access 10BASE‑T1S networks. As
> LAN867X and LAN865X are using the same function for the read_status,
> rename the function as lan86xx_read_status.
> 
> Signed-off-by: Parthiban Veerasooran <Parthiban.Veerasooran@microchip.com>
> ---
>  drivers/net/phy/microchip_t1s.c | 184 +++++++++++++++++++++++++++++++-
>  1 file changed, 181 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/phy/microchip_t1s.c b/drivers/net/phy/microchip_t1s.c
> index 6f9e197d8623..00c4c23906ce 100644
> --- a/drivers/net/phy/microchip_t1s.c
> +++ b/drivers/net/phy/microchip_t1s.c
> @@ -4,6 +4,7 @@
>   *
>   * Support: Microchip Phys:
>   *  lan8670/1/2 Rev.B1
> + *  lan8650/1 Rev.B0 Internal PHYs
>   */
>  
>  #include <linux/kernel.h>
> @@ -11,11 +12,19 @@
>  #include <linux/phy.h>
>  
>  #define PHY_ID_LAN867X_REVB1 0x0007C162
> +#define PHY_ID_LAN865X_REVB0 0x0007C1B3
>  
>  #define LAN867X_REG_STS2 0x0019
>  
>  #define LAN867x_RESET_COMPLETE_STS BIT(11)
>  
> +#define LAN865X_REG_CFGPARAM_ADDR 0x00D8
> +#define LAN865X_REG_CFGPARAM_DATA 0x00D9
> +#define LAN865X_REG_CFGPARAM_CTRL 0x00DA
> +#define LAN865X_REG_STS2 0x0019
> +
> +#define LAN865X_CFGPARAM_READ_ENABLE BIT(1)
> +
>  /* The arrays below are pulled from the following table from AN1699
>   * Access MMD Address Value Mask
>   * RMW 0x1F 0x00D0 0x0002 0x0E03
> @@ -50,6 +59,164 @@ 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,
> +	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,
> +	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
> +};
> +
> +/* Pulled from AN1760 describing 'indirect read'
> + *
> + * write_register(0x4, 0x00D8, addr)
> + * write_register(0x4, 0x00DA, 0x2)
> + * return (int8)(read_register(0x4, 0x00D9))
> + *
> + * 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)
> +{
> +	int ret;
> +
> +	ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, LAN865X_REG_CFGPARAM_ADDR,
> +			    addr);
> +	if (ret)
> +		return ret;
> +
> +	ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, LAN865X_REG_CFGPARAM_CTRL,
> +			    LAN865X_CFGPARAM_READ_ENABLE);
> +	if (ret)
> +		return ret;
> +
> +	return phy_read_mmd(phydev, MDIO_MMD_VEND2, LAN865X_REG_CFGPARAM_DATA);
> +}
> +
> +/* 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])
> +{
> +	const u16 fixup_regs[2] = {0x0004, 0x0008};
> +	int ret;
> +
> +	for (int i = 0; i < ARRAY_SIZE(fixup_regs); i++) {
> +		ret = lan865x_revb0_indirect_read(phydev, fixup_regs[i]);
> +		if (ret < 0)
> +			return ret;
> +		if (ret & BIT(4))
> +			offsets[i] = ret | 0xE0;
> +		else
> +			offsets[i] = ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int lan865x_read_cfg_params(struct phy_device *phydev, u16 cfg_params[])
> +{
> +	int ret;
> +
> +	for (int i = 0; i < ARRAY_SIZE(lan865x_revb0_fixup_cfg_regs); i++) {
> +		ret = phy_read_mmd(phydev, MDIO_MMD_VEND2,
> +				   lan865x_revb0_fixup_cfg_regs[i]);
> +		if (ret < 0)
> +			return ret;
> +		cfg_params[i] = (u16)ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int lan865x_write_cfg_params(struct phy_device *phydev, u16 cfg_params[])
> +{
> +	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],
> +				    cfg_params[i]);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int lan865x_setup_cfgparam(struct phy_device *phydev)
> +{
> +	u16 cfg_params[ARRAY_SIZE(lan865x_revb0_fixup_cfg_regs)];
> +	u16 cfg_results[5];
> +	s8 offsets[2];
> +	int ret;
> +
> +	ret = lan865x_generate_cfg_offsets(phydev, offsets);
> +	if (ret)
> +		return ret;
> +
> +	ret = lan865x_read_cfg_params(phydev, 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]);
> +
> +	return lan865x_write_cfg_params(phydev, cfg_results);
> +}
> +
> +static int lan865x_revb0_config_init(struct phy_device *phydev)
> +{
> +	int ret;
> +
> +	/* Reference to AN1760
> +	 * https://ww1.microchip.com/downloads/aemDocuments/documents/AIS/ProductDocuments/SupportingCollateral/AN-LAN8650-1-Configuration-60001760.pdf
> +	 */
> +	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;
> +	}
> +	/* Function to calculate and write the configuration parameters in the
> +	 * 0x0084, 0x008A, 0x00AD, 0x00AE and 0x00AF registers (from AN1760)
> +	 */
> +	ret = lan865x_setup_cfgparam(phydev);

Nit, you could return the result of lan865x_setup_cfgparam directly

> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +}
> +
>  static int lan867x_revb1_config_init(struct phy_device *phydev)
>  {
>  	/* HW quirk: Microchip states in the application note (AN1699) for the phy
> @@ -107,7 +274,7 @@ static int lan867x_revb1_config_init(struct phy_device *phydev)
>  	return 0;
>  }
>  
> -static int lan867x_read_status(struct phy_device *phydev)
> +static int lan86xx_read_status(struct phy_device *phydev)
>  {
>  	/* The phy has some limitations, namely:
>  	 *  - always reports link up
> @@ -128,17 +295,28 @@ static struct phy_driver microchip_t1s_driver[] = {
>  		.name               = "LAN867X Rev.B1",
>  		.features           = PHY_BASIC_T1S_P2MP_FEATURES,
>  		.config_init        = lan867x_revb1_config_init,
> -		.read_status        = lan867x_read_status,
> +		.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_REVB0),
> +		.name               = "LAN865X Rev.B0 Internal Phy",
> +		.features           = PHY_BASIC_T1S_P2MP_FEATURES,
> +		.config_init        = lan865x_revb0_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,
> +	},
>  };
>  
>  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) },
>  	{ }
>  };
>  
> -- 
> 2.34.1
> 

Re: [PATCH net-next v3 2/6] net: phy: microchip_t1s: replace read-modify-write code with phy_modify_mmd

[Ramón Nordin Rodriguez]

On Wed, May 24, 2023 at 08:15:35PM +0530, Parthiban Veerasooran wrote:
> Replace read-modify-write code in the lan867x_config_init function to
> avoid handling data type mismatch and to simplify the code.
> 
> Signed-off-by: Parthiban Veerasooran <Parthiban.Veerasooran@microchip.com>
> ---
>  drivers/net/phy/microchip_t1s.c | 23 +++++++++++------------
>  1 file changed, 11 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/phy/microchip_t1s.c b/drivers/net/phy/microchip_t1s.c
> index a42a6bb6e3bd..b5b5a95fa6e7 100644
> --- a/drivers/net/phy/microchip_t1s.c
> +++ b/drivers/net/phy/microchip_t1s.c
> @@ -31,19 +31,19 @@
>   * W   0x1F 0x0099 0x7F80 ------
>   */
>  
> -static const int lan867x_fixup_registers[12] = {
> +static const u32 lan867x_fixup_registers[12] = {
>  	0x00D0, 0x00D1, 0x0084, 0x0085,
>  	0x008A, 0x0087, 0x0088, 0x008B,
>  	0x0080, 0x00F1, 0x0096, 0x0099,
>  };
>  
> -static const int lan867x_fixup_values[12] = {
> +static const u16 lan867x_fixup_values[12] = {
>  	0x0002, 0x0000, 0x3380, 0x0006,
>  	0xC000, 0x801C, 0x033F, 0x0404,
>  	0x0600, 0x2400, 0x2000, 0x7F80,
>  };
>  
> -static const int lan867x_fixup_masks[12] = {
> +static const u16 lan867x_fixup_masks[12] = {
>  	0x0E03, 0x0300, 0xFFC0, 0x000F,
>  	0xF800, 0x801C, 0x1FFF, 0xFFFF,
>  	0x0600, 0x7F00, 0x2000, 0xFFFF,
> @@ -63,23 +63,22 @@ static int lan867x_config_init(struct phy_device *phydev)
>  	 * used, which might then write the same value back as read + modified.
>  	 */
>  
> -	int reg_value;
>  	int err;
> -	int reg;
>  
>  	/* Read-Modified Write Pseudocode (from AN1699)
>  	 * current_val = read_register(mmd, addr) // Read current register value
>  	 * new_val = current_val AND (NOT mask) // Clear bit fields to be written
>  	 * new_val = new_val OR value // Set bits
> -	 * write_register(mmd, addr, new_val) // Write back updated register value
> +	 * write_register(mmd, addr, new_val) // Write back updated register value.
> +	 * Although AN1699 says Read, Modify, Write, the write is not required if
> +	 * the register already has the required value.
>  	 */
>  	for (int i = 0; i < ARRAY_SIZE(lan867x_fixup_registers); i++) {
> -		reg = lan867x_fixup_registers[i];
> -		reg_value = phy_read_mmd(phydev, MDIO_MMD_VEND2, reg);
> -		reg_value &= ~lan867x_fixup_masks[i];
> -		reg_value |= lan867x_fixup_values[i];
> -		err = phy_write_mmd(phydev, MDIO_MMD_VEND2, reg, reg_value);
> -		if (err != 0)
> +		err = phy_modify_mmd(phydev, MDIO_MMD_VEND2,
> +				     lan867x_fixup_registers[i],
> +				     lan867x_fixup_masks[i],
> +				     lan867x_fixup_values[i]);

This change answers an uncertainty in the block comment in the top of
this func, pasted here for your convenience

	/* HW quirk: Microchip states in the application note (AN1699) for the phy
	 * that a set of read-modify-write (rmw) operations has to be performed
	 * on a set of seemingly magic registers.
	 * The result of these operations is just described as 'optimal performance'
	 * Microchip gives no explanation as to what these mmd regs do,
	 * in fact they are marked as reserved in the datasheet.
	 * It is unclear if phy_modify_mmd would be safe to use or if a write
	 * really has to happen to each register.
	 * In order to exactly conform to what is stated in the AN phy_write_mmd is
	 * used, which might then write the same value back as read + modified.
	 */

This change also invalidates most of the comment. I think this should be
reduced to something along the lines of:
	/* HW quirk: Microchip states in the application note (AN1699) for the phy
	 * that a set of read-modify-write (rmw) operations has to be performed
	 * on a set of seemingly magic registers.
	 * The result of these operations is just described as 'optimal performance'
	 * Microchip gives no explanation as to what these mmd regs do,
	 * in fact they are marked as reserved in the datasheet.*/

Additionally I don't mind it if you change the tone of the comment. This was brought
up in the sitdown we had, where it was explained from Microchip that
documenting what the reg operations actually does would expose to much
of the internal workings of the chip.

Possibly this comment should move down to where the fixup reg operations
are performed, and replace the comment about the 'read write modify'
stuff all togheter.
In my opinion I kind of loose context about what the func does when it
first explains the fixup stuff, then actually does something with the
STS2 regs, and finally actually does the fixup.
> +		if (err)
>  			return err;
>  	}
>  
> -- 
> 2.34.1
> 

Re: [PATCH net-next v3 2/6] net: phy: microchip_t1s: replace read-modify-write code with phy_modify_mmd

[Andrew Lunn]

> This change also invalidates most of the comment. I think this should be
> reduced to something along the lines of:
> 	/* HW quirk: Microchip states in the application note (AN1699) for the phy
> 	 * that a set of read-modify-write (rmw) operations has to be performed
> 	 * on a set of seemingly magic registers.
> 	 * The result of these operations is just described as 'optimal performance'
> 	 * Microchip gives no explanation as to what these mmd regs do,
> 	 * in fact they are marked as reserved in the datasheet.*/

I agree the comments should be reviewed in light of these changes.

> 
> Additionally I don't mind it if you change the tone of the comment. This was brought
> up in the sitdown we had, where it was explained from Microchip that
> documenting what the reg operations actually does would expose to much
> of the internal workings of the chip.

They cannot care too much, or the firmware in the PHY would do this
where it is all hidden away.

      Andrew

Re: [PATCH net-next v3 1/6] net: phy: microchip_t1s: modify driver description to be more generic

[Ramón Nordin Rodriguez]

On Wed, May 24, 2023 at 08:15:34PM +0530, Parthiban Veerasooran wrote:
> Remove LAN867X from the driver description as this driver is common for
> all the Microchip 10BASE-T1S PHYs.
> 
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> Signed-off-by: Parthiban Veerasooran <Parthiban.Veerasooran@microchip.com>
> ---

Reviewed-by: Ramón Nordin Rodriguez <ramon.nordin.rodriguez@ferroamp.se>

Re: [PATCH net-next v3 3/6] net: phy: microchip_t1s: update LAN867x PHY supported revision number

[Ramón Nordin Rodriguez]

On Wed, May 24, 2023 at 08:15:36PM +0530, Parthiban Veerasooran wrote:
> As per AN1699, the initial configuration in the driver applies to LAN867x
> Rev.B1 hardware revision. 0x0007C160 (Rev.A0) and 0x0007C161 (Rev.B0)
> never released to production and hence they don't need to be supported.
> 
> Signed-off-by: Parthiban Veerasooran <Parthiban.Veerasooran@microchip.com>
> ---

Reviewed-by: Ramón Nordin Rodriguez <ramon.nordin.rodriguez@ferroamp.se>

Re: [PATCH net-next v3 4/6] net: phy: microchip_t1s: fix reset complete status handling

[Ramón Nordin Rodriguez]

> +	/* Read STS2 register and check for the Reset Complete status to do the
> +	 * init configuration. If the Reset Complete is not set, wait for 5us
> +	 * and then read STS2 register again and check for Reset Complete status.
> +	 * Still if it is failed then declare PHY reset error or else proceed
> +	 * for the PHY initial register configuration.
> +	 */
> +	err = phy_read_mmd(phydev, MDIO_MMD_VEND2, LAN867X_REG_STS2);
> +	if (err < 0)
> +		return err;
> +
> +	if (!(err & LAN867x_RESET_COMPLETE_STS)) {
> +		udelay(5);
> +		err = phy_read_mmd(phydev, MDIO_MMD_VEND2, LAN867X_REG_STS2);
> +		if (err < 0)
> +			return err;
> +		if (!(err & LAN867x_RESET_COMPLETE_STS)) {
> +			phydev_err(phydev, "PHY reset failed\n");
> +			return -ENODEV;
> +		}
> +	}

This comment explains exactly what the code does, which is also obvious
from reading the code. A meaningful comment would be explaining why the
state can change 5us later.

Re: [PATCH net-next v3 5/6] net: phy: microchip_t1s: remove unnecessary interrupts disabling code

[Ramón Nordin Rodriguez]

On Wed, May 24, 2023 at 08:15:38PM +0530, Parthiban Veerasooran wrote:
> By default, except Reset Complete interrupt in the Interrupt Mask 2
> Register all other interrupts are disabled/masked. As Reset Complete
> status is already handled, it doesn't make sense to disable it.
> 
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> Signed-off-by: Parthiban Veerasooran <Parthiban.Veerasooran@microchip.com>
> ---

Reviewed-by: Ramón Nordin Rodriguez <ramon.nordin.rodriguez@ferroamp.se>
Tested-by: Ramón Nordin Rodriguez <ramon.nordin.rodriguez@ferroamp.se>

Testing has been pretty rudamentary, but the procedure has been as
follows:
    * Hotplug 2 devices
    * Unload and reload the lkm
    * Ping 2 devices over ipv6 link local addresses
All testing has been performed with the EVB-LAN8670-USB

Re: [PATCH net-next v3 6/6] net: phy: microchip_t1s: add support for Microchip LAN865x Rev.B0 PHYs

[Ramón Nordin Rodriguez]

On Wed, May 24, 2023 at 08:15:39PM +0530, Parthiban Veerasooran wrote:
> Add support for the Microchip LAN865x Rev.B0 10BASE-T1S Internal PHYs
> (LAN8650/1). The LAN865x combines a Media Access Controller (MAC) and an
> internal 10BASE-T1S Ethernet PHY to access 10BASE‑T1S networks. As
> LAN867X and LAN865X are using the same function for the read_status,
> rename the function as lan86xx_read_status.
> 
> Signed-off-by: Parthiban Veerasooran <Parthiban.Veerasooran@microchip.com>
> ---

Reviewed-by: Ramón Nordin Rodriguez <ramon.nordin.rodriguez@ferroamp.se>

Re: [PATCH net-next v3 6/6] net: phy: microchip_t1s: add support for Microchip LAN865x Rev.B0 PHYs

[Ramón Nordin Rodriguez]

On Wed, May 24, 2023 at 08:15:39PM +0530, Parthiban Veerasooran wrote:
> Add support for the Microchip LAN865x Rev.B0 10BASE-T1S Internal PHYs
> (LAN8650/1). The LAN865x combines a Media Access Controller (MAC) and an
> internal 10BASE-T1S Ethernet PHY to access 10BASE‑T1S networks. As
> LAN867X and LAN865X are using the same function for the read_status,
> rename the function as lan86xx_read_status.
> 
> Signed-off-by: Parthiban Veerasooran <Parthiban.Veerasooran@microchip.com>
> ---

I spotted something that's missing, the help text for the
MICROCHIP_T1S_PHY config option in driver/net/phy/Kconfig
should be updated. Currently it says:
	  Currently supports the LAN8670, LAN8671, LAN8672

Which should be extended with the 865x phys

Re: [PATCH net-next v3 2/6] net: phy: microchip_t1s: replace read-modify-write code with phy_modify_mmd

[Parthiban.Veerasooran]

Hi Ramon,

On 25/05/23 8:38 pm, Ramón Nordin Rodriguez wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Wed, May 24, 2023 at 08:15:35PM +0530, Parthiban Veerasooran wrote:
>> Replace read-modify-write code in the lan867x_config_init function to
>> avoid handling data type mismatch and to simplify the code.
>>
>> Signed-off-by: Parthiban Veerasooran <Parthiban.Veerasooran@microchip.com>
>> ---
>>   drivers/net/phy/microchip_t1s.c | 23 +++++++++++------------
>>   1 file changed, 11 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/net/phy/microchip_t1s.c b/drivers/net/phy/microchip_t1s.c
>> index a42a6bb6e3bd..b5b5a95fa6e7 100644
>> --- a/drivers/net/phy/microchip_t1s.c
>> +++ b/drivers/net/phy/microchip_t1s.c
>> @@ -31,19 +31,19 @@
>>    * W   0x1F 0x0099 0x7F80 ------
>>    */
>>
>> -static const int lan867x_fixup_registers[12] = {
>> +static const u32 lan867x_fixup_registers[12] = {
>>        0x00D0, 0x00D1, 0x0084, 0x0085,
>>        0x008A, 0x0087, 0x0088, 0x008B,
>>        0x0080, 0x00F1, 0x0096, 0x0099,
>>   };
>>
>> -static const int lan867x_fixup_values[12] = {
>> +static const u16 lan867x_fixup_values[12] = {
>>        0x0002, 0x0000, 0x3380, 0x0006,
>>        0xC000, 0x801C, 0x033F, 0x0404,
>>        0x0600, 0x2400, 0x2000, 0x7F80,
>>   };
>>
>> -static const int lan867x_fixup_masks[12] = {
>> +static const u16 lan867x_fixup_masks[12] = {
>>        0x0E03, 0x0300, 0xFFC0, 0x000F,
>>        0xF800, 0x801C, 0x1FFF, 0xFFFF,
>>        0x0600, 0x7F00, 0x2000, 0xFFFF,
>> @@ -63,23 +63,22 @@ static int lan867x_config_init(struct phy_device *phydev)
>>         * used, which might then write the same value back as read + modified.
>>         */
>>
>> -     int reg_value;
>>        int err;
>> -     int reg;
>>
>>        /* Read-Modified Write Pseudocode (from AN1699)
>>         * current_val = read_register(mmd, addr) // Read current register value
>>         * new_val = current_val AND (NOT mask) // Clear bit fields to be written
>>         * new_val = new_val OR value // Set bits
>> -      * write_register(mmd, addr, new_val) // Write back updated register value
>> +      * write_register(mmd, addr, new_val) // Write back updated register value.
>> +      * Although AN1699 says Read, Modify, Write, the write is not required if
>> +      * the register already has the required value.
>>         */
> 
> Nitpick, I think this block comment can be reduced to:
> /* The following block deviates from AN1699 which states that a values
>   * should be written back, even if unmodified.
>   * Which is not necessary, so it's safe to use phy_modify_mmd here.*/
> 
>   The comment I added was intended to describe why I was doing weird
>   things, but now I think it's more interesting to describe why we're
>   deviating from the AN.
> 
>   Or the block comment could be dropped all togheter, I'm guessing no one
>   is going to consult the AN if things 'just work'
> 
By consolidating all your comments in the other emails as well on this 
2nd patch, do you agree for my below proposal?

We will remove all block comments and simply put AN1699 reference as we 
did for lan865x_revb0_config_init with a small addition on top of 
phy_modify_mmd for loop? so the comment will look like below,

/* 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  register already has the required value. So it is safe to use 
phy_modify_mmd here.
  */

So in future, if someone wants to know about this configuration they can 
simply refer the AN1699.

What do you think?

Best Regards,
Parthiban V
>>        for (int i = 0; i < ARRAY_SIZE(lan867x_fixup_registers); i++) {
>> -             reg = lan867x_fixup_registers[i];
>> -             reg_value = phy_read_mmd(phydev, MDIO_MMD_VEND2, reg);
>> -             reg_value &= ~lan867x_fixup_masks[i];
>> -             reg_value |= lan867x_fixup_values[i];
>> -             err = phy_write_mmd(phydev, MDIO_MMD_VEND2, reg, reg_value);
>> -             if (err != 0)
>> +             err = phy_modify_mmd(phydev, MDIO_MMD_VEND2,
>> +                                  lan867x_fixup_registers[i],
>> +                                  lan867x_fixup_masks[i],
>> +                                  lan867x_fixup_values[i]);
>> +             if (err)
>>                        return err;
>>        }
>>
>> --
>> 2.34.1
>>

Re: [PATCH net-next v3 4/6] net: phy: microchip_t1s: fix reset complete status handling

[Parthiban.Veerasooran]

Hi Ramon,

On 25/05/23 11:56 pm, Ramón Nordin Rodriguez wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
>> +     /* Read STS2 register and check for the Reset Complete status to do the
>> +      * init configuration. If the Reset Complete is not set, wait for 5us
>> +      * and then read STS2 register again and check for Reset Complete status.
>> +      * Still if it is failed then declare PHY reset error or else proceed
>> +      * for the PHY initial register configuration.
>> +      */
>> +     err = phy_read_mmd(phydev, MDIO_MMD_VEND2, LAN867X_REG_STS2);
>> +     if (err < 0)
>> +             return err;
>> +
>> +     if (!(err & LAN867x_RESET_COMPLETE_STS)) {
>> +             udelay(5);
>> +             err = phy_read_mmd(phydev, MDIO_MMD_VEND2, LAN867X_REG_STS2);
>> +             if (err < 0)
>> +                     return err;
>> +             if (!(err & LAN867x_RESET_COMPLETE_STS)) {
>> +                     phydev_err(phydev, "PHY reset failed\n");
>> +                     return -ENODEV;
>> +             }
>> +     }
> 
> This comment explains exactly what the code does, which is also obvious
> from reading the code. A meaningful comment would be explaining why the
> state can change 5us later.
> 
As per design, LAN867x reset to be completed by 3us. Just for a safer 
side it is recommended to use 5us. With the assumption of more than 3us 
completion, the first read checks for the Reset Complete. If the 
config_init is more faster, then once again checks for it after 5us.

As you mentioned, can we remove the existing block comment as it 
explains the code and add the above comment to explain 5us delay.
What is your opinion on this proposal?

Best Regards,
Parthiban V

Re: [PATCH net-next v3 6/6] net: phy: microchip_t1s: add support for Microchip LAN865x Rev.B0 PHYs

[Parthiban.Veerasooran]

Hi Ramon,

On 26/05/23 12:52 am, Ramón Nordin Rodriguez wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Wed, May 24, 2023 at 08:15:39PM +0530, Parthiban Veerasooran wrote:
>> Add support for the Microchip LAN865x Rev.B0 10BASE-T1S Internal PHYs
>> (LAN8650/1). The LAN865x combines a Media Access Controller (MAC) and an
>> internal 10BASE-T1S Ethernet PHY to access 10BASE‑T1S networks. As
>> LAN867X and LAN865X are using the same function for the read_status,
>> rename the function as lan86xx_read_status.
>>
>> Signed-off-by: Parthiban Veerasooran <Parthiban.Veerasooran@microchip.com>
>> ---
> 
> I spotted something that's missing, the help text for the
> MICROCHIP_T1S_PHY config option in driver/net/phy/Kconfig
> should be updated. Currently it says:
>            Currently supports the LAN8670, LAN8671, LAN8672
> 
> Which should be extended with the 865x phys
Thanks for pointing out this. Sure will update in the next version.

Best Regards,
Parthiban V

Re: [PATCH net-next v3 6/6] net: phy: microchip_t1s: add support for Microchip LAN865x Rev.B0 PHYs

[Parthiban.Veerasooran]

Hi Ramon,

On 25/05/23 8:46 pm, Ramón Nordin Rodriguez wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Wed, May 24, 2023 at 08:15:39PM +0530, Parthiban Veerasooran wrote:
>> Add support for the Microchip LAN865x Rev.B0 10BASE-T1S Internal PHYs
>> (LAN8650/1). The LAN865x combines a Media Access Controller (MAC) and an
>> internal 10BASE-T1S Ethernet PHY to access 10BASE‑T1S networks. As
>> LAN867X and LAN865X are using the same function for the read_status,
>> rename the function as lan86xx_read_status.
>>
>> Signed-off-by: Parthiban Veerasooran <Parthiban.Veerasooran@microchip.com>
>> ---
>>   drivers/net/phy/microchip_t1s.c | 184 +++++++++++++++++++++++++++++++-
>>   1 file changed, 181 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/phy/microchip_t1s.c b/drivers/net/phy/microchip_t1s.c
>> index 6f9e197d8623..00c4c23906ce 100644
>> --- a/drivers/net/phy/microchip_t1s.c
>> +++ b/drivers/net/phy/microchip_t1s.c
>> @@ -4,6 +4,7 @@
>>    *
>>    * Support: Microchip Phys:
>>    *  lan8670/1/2 Rev.B1
>> + *  lan8650/1 Rev.B0 Internal PHYs
>>    */
>>
>>   #include <linux/kernel.h>
>> @@ -11,11 +12,19 @@
>>   #include <linux/phy.h>
>>
>>   #define PHY_ID_LAN867X_REVB1 0x0007C162
>> +#define PHY_ID_LAN865X_REVB0 0x0007C1B3
>>
>>   #define LAN867X_REG_STS2 0x0019
>>
>>   #define LAN867x_RESET_COMPLETE_STS BIT(11)
>>
>> +#define LAN865X_REG_CFGPARAM_ADDR 0x00D8
>> +#define LAN865X_REG_CFGPARAM_DATA 0x00D9
>> +#define LAN865X_REG_CFGPARAM_CTRL 0x00DA
>> +#define LAN865X_REG_STS2 0x0019
>> +
>> +#define LAN865X_CFGPARAM_READ_ENABLE BIT(1)
>> +
>>   /* The arrays below are pulled from the following table from AN1699
>>    * Access MMD Address Value Mask
>>    * RMW 0x1F 0x00D0 0x0002 0x0E03
>> @@ -50,6 +59,164 @@ 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,
>> +     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,
>> +     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
>> +};
>> +
>> +/* Pulled from AN1760 describing 'indirect read'
>> + *
>> + * write_register(0x4, 0x00D8, addr)
>> + * write_register(0x4, 0x00DA, 0x2)
>> + * return (int8)(read_register(0x4, 0x00D9))
>> + *
>> + * 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)
>> +{
>> +     int ret;
>> +
>> +     ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, LAN865X_REG_CFGPARAM_ADDR,
>> +                         addr);
>> +     if (ret)
>> +             return ret;
>> +
>> +     ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, LAN865X_REG_CFGPARAM_CTRL,
>> +                         LAN865X_CFGPARAM_READ_ENABLE);
>> +     if (ret)
>> +             return ret;
>> +
>> +     return phy_read_mmd(phydev, MDIO_MMD_VEND2, LAN865X_REG_CFGPARAM_DATA);
>> +}
>> +
>> +/* 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])
>> +{
>> +     const u16 fixup_regs[2] = {0x0004, 0x0008};
>> +     int ret;
>> +
>> +     for (int i = 0; i < ARRAY_SIZE(fixup_regs); i++) {
>> +             ret = lan865x_revb0_indirect_read(phydev, fixup_regs[i]);
>> +             if (ret < 0)
>> +                     return ret;
>> +             if (ret & BIT(4))
>> +                     offsets[i] = ret | 0xE0;
>> +             else
>> +                     offsets[i] = ret;
>> +     }
>> +
>> +     return 0;
>> +}
>> +
>> +static int lan865x_read_cfg_params(struct phy_device *phydev, u16 cfg_params[])
>> +{
>> +     int ret;
>> +
>> +     for (int i = 0; i < ARRAY_SIZE(lan865x_revb0_fixup_cfg_regs); i++) {
>> +             ret = phy_read_mmd(phydev, MDIO_MMD_VEND2,
>> +                                lan865x_revb0_fixup_cfg_regs[i]);
>> +             if (ret < 0)
>> +                     return ret;
>> +             cfg_params[i] = (u16)ret;
>> +     }
>> +
>> +     return 0;
>> +}
>> +
>> +static int lan865x_write_cfg_params(struct phy_device *phydev, u16 cfg_params[])
>> +{
>> +     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],
>> +                                 cfg_params[i]);
>> +             if (ret)
>> +                     return ret;
>> +     }
>> +
>> +     return 0;
>> +}
>> +
>> +static int lan865x_setup_cfgparam(struct phy_device *phydev)
>> +{
>> +     u16 cfg_params[ARRAY_SIZE(lan865x_revb0_fixup_cfg_regs)];
>> +     u16 cfg_results[5];
>> +     s8 offsets[2];
>> +     int ret;
>> +
>> +     ret = lan865x_generate_cfg_offsets(phydev, offsets);
>> +     if (ret)
>> +             return ret;
>> +
>> +     ret = lan865x_read_cfg_params(phydev, 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]);
>> +
>> +     return lan865x_write_cfg_params(phydev, cfg_results);
>> +}
>> +
>> +static int lan865x_revb0_config_init(struct phy_device *phydev)
>> +{
>> +     int ret;
>> +
>> +     /* Reference to AN1760
>> +      * https://ww1.microchip.com/downloads/aemDocuments/documents/AIS/ProductDocuments/SupportingCollateral/AN-LAN8650-1-Configuration-60001760.pdf
>> +      */
>> +     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;
>> +     }
>> +     /* Function to calculate and write the configuration parameters in the
>> +      * 0x0084, 0x008A, 0x00AD, 0x00AE and 0x00AF registers (from AN1760)
>> +      */
>> +     ret = lan865x_setup_cfgparam(phydev);
> 
> Nit, you could return the result of lan865x_setup_cfgparam directly
Ah ok, will do it in the next version.

Best Regards,
Parthiban V
> 
>> +     if (ret < 0)
>> +             return ret;
>> +
>> +     return 0;
>> +}
>> +
>>   static int lan867x_revb1_config_init(struct phy_device *phydev)
>>   {
>>        /* HW quirk: Microchip states in the application note (AN1699) for the phy
>> @@ -107,7 +274,7 @@ static int lan867x_revb1_config_init(struct phy_device *phydev)
>>        return 0;
>>   }
>>
>> -static int lan867x_read_status(struct phy_device *phydev)
>> +static int lan86xx_read_status(struct phy_device *phydev)
>>   {
>>        /* The phy has some limitations, namely:
>>         *  - always reports link up
>> @@ -128,17 +295,28 @@ static struct phy_driver microchip_t1s_driver[] = {
>>                .name               = "LAN867X Rev.B1",
>>                .features           = PHY_BASIC_T1S_P2MP_FEATURES,
>>                .config_init        = lan867x_revb1_config_init,
>> -             .read_status        = lan867x_read_status,
>> +             .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_REVB0),
>> +             .name               = "LAN865X Rev.B0 Internal Phy",
>> +             .features           = PHY_BASIC_T1S_P2MP_FEATURES,
>> +             .config_init        = lan865x_revb0_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,
>> +     },
>>   };
>>
>>   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) },
>>        { }
>>   };
>>
>> --
>> 2.34.1
>>
> 

Re: [PATCH net-next v3 2/6] net: phy: microchip_t1s: replace read-modify-write code with phy_modify_mmd

[Ramón Nordin Rodriguez]

On Fri, May 26, 2023 at 05:48:25AM +0000, Parthiban.Veerasooran@microchip.com wrote:
> Hi Ramon,
> > Nitpick, I think this block comment can be reduced to:
> > /* The following block deviates from AN1699 which states that a values
> >   * should be written back, even if unmodified.
> >   * Which is not necessary, so it's safe to use phy_modify_mmd here.*/
> > 
> >   The comment I added was intended to describe why I was doing weird
> >   things, but now I think it's more interesting to describe why we're
> >   deviating from the AN.
> > 
> >   Or the block comment could be dropped all togheter, I'm guessing no one
> >   is going to consult the AN if things 'just work'
> > 
> By consolidating all your comments in the other emails as well on this 
> 2nd patch, do you agree for my below proposal?
> 
> We will remove all block comments and simply put AN1699 reference as we 
> did for lan865x_revb0_config_init with a small addition on top of 
> phy_modify_mmd for loop? so the comment will look like below,
> 
> /* 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  register already has the required value. So it is safe to use 
> phy_modify_mmd here.
>   */
> 
> So in future, if someone wants to know about this configuration they can 
> simply refer the AN1699.
> 
> What do you think?
> 

I'm not sure about the link, resources have a tendency to move.
Otherwise LGTM

> Best Regards,
> Parthiban V

Re: [PATCH net-next v3 4/6] net: phy: microchip_t1s: fix reset complete status handling

[Ramón Nordin Rodriguez]

On Fri, May 26, 2023 at 06:00:08AM +0000, Parthiban.Veerasooran@microchip.com wrote:
> Hi Ramon,
> >> +     /* Read STS2 register and check for the Reset Complete status to do the
> >> +      * init configuration. If the Reset Complete is not set, wait for 5us
> >> +      * and then read STS2 register again and check for Reset Complete status.
> >> +      * Still if it is failed then declare PHY reset error or else proceed
> >> +      * for the PHY initial register configuration.
> >> +      */
> > 
> > This comment explains exactly what the code does, which is also obvious
> > from reading the code. A meaningful comment would be explaining why the
> > state can change 5us later.
> > 
> As per design, LAN867x reset to be completed by 3us. Just for a safer 
> side it is recommended to use 5us. With the assumption of more than 3us 
> completion, the first read checks for the Reset Complete. If the 
> config_init is more faster, then once again checks for it after 5us.
> 
> As you mentioned, can we remove the existing block comment as it 
> explains the code and add the above comment to explain 5us delay.
> What is your opinion on this proposal?
> 
> Best Regards,
> Parthiban V
> 

I'd suggest the following
/*The chip completes a reset in 3us, we might get here earlier than that,
as an added margin we'll conditionally sleep 5us*/

Re: [PATCH net-next v3 4/6] net: phy: microchip_t1s: fix reset complete status handling

[Parthiban.Veerasooran]

Hi Ramon,

On 26/05/23 12:44 pm, Ramón Nordin Rodriguez wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Fri, May 26, 2023 at 06:00:08AM +0000, Parthiban.Veerasooran@microchip.com wrote:
>> Hi Ramon,
>>>> +     /* Read STS2 register and check for the Reset Complete status to do the
>>>> +      * init configuration. If the Reset Complete is not set, wait for 5us
>>>> +      * and then read STS2 register again and check for Reset Complete status.
>>>> +      * Still if it is failed then declare PHY reset error or else proceed
>>>> +      * for the PHY initial register configuration.
>>>> +      */
>>>
>>> This comment explains exactly what the code does, which is also obvious
>>> from reading the code. A meaningful comment would be explaining why the
>>> state can change 5us later.
>>>
>> As per design, LAN867x reset to be completed by 3us. Just for a safer
>> side it is recommended to use 5us. With the assumption of more than 3us
>> completion, the first read checks for the Reset Complete. If the
>> config_init is more faster, then once again checks for it after 5us.
>>
>> As you mentioned, can we remove the existing block comment as it
>> explains the code and add the above comment to explain 5us delay.
>> What is your opinion on this proposal?
>>
>> Best Regards,
>> Parthiban V
>>
> 
> I'd suggest the following
> /*The chip completes a reset in 3us, we might get here earlier than that,
> as an added margin we'll conditionally sleep 5us*/
Ok I will use this proposal in the next version.

Best Regards,
Parthiban V

Re: [PATCH net-next v3 2/6] net: phy: microchip_t1s: replace read-modify-write code with phy_modify_mmd

[Parthiban.Veerasooran]

On 26/05/23 12:40 pm, Ramón Nordin Rodriguez wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Fri, May 26, 2023 at 05:48:25AM +0000, Parthiban.Veerasooran@microchip.com wrote:
>> Hi Ramon,
>>> Nitpick, I think this block comment can be reduced to:
>>> /* The following block deviates from AN1699 which states that a values
>>>    * should be written back, even if unmodified.
>>>    * Which is not necessary, so it's safe to use phy_modify_mmd here.*/
>>>
>>>    The comment I added was intended to describe why I was doing weird
>>>    things, but now I think it's more interesting to describe why we're
>>>    deviating from the AN.
>>>
>>>    Or the block comment could be dropped all togheter, I'm guessing no one
>>>    is going to consult the AN if things 'just work'
>>>
>> By consolidating all your comments in the other emails as well on this
>> 2nd patch, do you agree for my below proposal?
>>
>> We will remove all block comments and simply put AN1699 reference as we
>> did for lan865x_revb0_config_init with a small addition on top of
>> phy_modify_mmd for loop? so the comment will look like below,
>>
>> /* 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  register already has the required value. So it is safe to use
>> phy_modify_mmd here.
>>    */
>>
>> So in future, if someone wants to know about this configuration they can
>> simply refer the AN1699.
>>
>> What do you think?
>>
> 
> I'm not sure about the link, resources have a tendency to move.
Yes, I agree with you but somehow there is no way for giving the 
reference to this document. May be we will keep this link for the 
reference, later if someone is not able to access the link then they can 
request Microchip to get the document.

What do you think about this proposal? If you agree then I will proceed 
for preparing the next version with your comments.
> Otherwise LGTM
> 
>> Best Regards,
>> Parthiban V

Re: [PATCH net-next v3 2/6] net: phy: microchip_t1s: replace read-modify-write code with phy_modify_mmd

[Ramón Nordin Rodriguez]

> >> /* 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  register already has the required value. So it is safe to use
> >> phy_modify_mmd here.
> >>    */
> >>
> >> So in future, if someone wants to know about this configuration they can
> >> simply refer the AN1699.
> >>
> >> What do you think?
> >>
> > 
> > I'm not sure about the link, resources have a tendency to move.
> Yes, I agree with you but somehow there is no way for giving the 
> reference to this document. May be we will keep this link for the 
> reference, later if someone is not able to access the link then they can 
> request Microchip to get the document.
> 
> What do you think about this proposal? If you agree then I will proceed 
> for preparing the next version with your comments.

Thumbs up from me
R

> > Otherwise LGTM
> > 
> >> Best Regards,
> >> Parthiban V
> 

Re: [PATCH net-next v3 2/6] net: phy: microchip_t1s: replace read-modify-write code with phy_modify_mmd

[Parthiban.Veerasooran]

Hi Ramon,

On 26/05/23 7:32 pm, Ramón Nordin Rodriguez wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
>>>> /* 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  register already has the required value. So it is safe to use
>>>> phy_modify_mmd here.
>>>>     */
>>>>
>>>> So in future, if someone wants to know about this configuration they can
>>>> simply refer the AN1699.
>>>>
>>>> What do you think?
>>>>
>>>
>>> I'm not sure about the link, resources have a tendency to move.
>> Yes, I agree with you but somehow there is no way for giving the
>> reference to this document. May be we will keep this link for the
>> reference, later if someone is not able to access the link then they can
>> request Microchip to get the document.
>>
>> What do you think about this proposal? If you agree then I will proceed
>> for preparing the next version with your comments.
> 
> Thumbs up from me
Thanks.

Best Regards,
Parthiban V
> R
> 
>>> Otherwise LGTM
>>>
>>>> Best Regards,
>>>> Parthiban V
>>