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

[PATCH net-next v2 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.

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 | 265 +++++++++++++++++++++++++++-----
 1 file changed, 226 insertions(+), 39 deletions(-)

-- 
2.34.1

[PATCH net-next v2 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.

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

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

[Andrew Lunn]

On Mon, May 22, 2023 at 05:03:26PM +0530, Parthiban Veerasooran wrote:
> Remove LAN867X from the driver description as this driver is common for
> all the Microchip 10BASE-T1S PHYs.
> 
> Signed-off-by: Parthiban Veerasooran <Parthiban.Veerasooran@microchip.com>

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

    Andrew

[PATCH net-next v2 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 | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

diff --git a/drivers/net/phy/microchip_t1s.c b/drivers/net/phy/microchip_t1s.c
index a42a6bb6e3bd..534d9faf8475 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,9 +63,7 @@ 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
@@ -74,12 +72,11 @@ static int lan867x_config_init(struct phy_device *phydev)
 	 * write_register(mmd, addr, new_val) // Write back updated register 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

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

[Andrew Lunn]

>  
>  	/* Read-Modified Write Pseudocode (from AN1699)
>  	 * current_val = read_register(mmd, addr) // Read current register value

Hi Parthiban

Maybe extend the comment to indicate that although AN1699 says Read,
Modify, Write, the write is not required if the register already has
the required value. That is what phy_modify_mmd() actually does.

> @@ -74,12 +72,11 @@ static int lan867x_config_init(struct phy_device *phydev)
>  	 * write_register(mmd, addr, new_val) // Write back updated register 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;
>  	}


    Andrew

---
pw-bot: cr

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

[Parthiban.Veerasooran]

Hi Andrew,

On 22/05/23 6:01 pm, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
>>
>>        /* Read-Modified Write Pseudocode (from AN1699)
>>         * current_val = read_register(mmd, addr) // Read current register value
> 
> Hi Parthiban
> 
> Maybe extend the comment to indicate that although AN1699 says Read,
> Modify, Write, the write is not required if the register already has
> the required value. That is what phy_modify_mmd() actually does.
Sure, I will add it in the next version.

Best Regards,
Parthiban V
> 
>> @@ -74,12 +72,11 @@ static int lan867x_config_init(struct phy_device *phydev)
>>         * write_register(mmd, addr, new_val) // Write back updated register 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;
>>        }
> 
> 
>      Andrew
> 
> ---
> pw-bot: cr

[PATCH net-next v2 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.

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 534d9faf8475..869c7f403ea1 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
@@ -71,11 +71,11 @@ static int lan867x_config_init(struct phy_device *phydev)
 	 * new_val = new_val OR value // Set bits
 	 * write_register(mmd, addr, new_val) // Write back updated register 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;
 	}
@@ -110,10 +110,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,
@@ -124,7 +124,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

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

[Andrew Lunn]

> -#define PHY_ID_LAN867X 0x0007C160
> +#define PHY_ID_LAN867X_REVB1 0x0007C162

>  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,
> @@ -124,7 +124,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) },
>  	{ }
>  };

Maybe i asked this last time...

What versions actually exist? The old entry would match 0x0007C16X, so
0x0007C160 and 0x0007C161, 0x0007C162, if those actually exist. Now
you are narrowing it down to just 0x0007C162.

It would be good to comment on this in the commit message, that
0x0007C160 and 0x0007C161 never escaped the testing facility and hence
don't need to be supported.

	Andrew

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

[Parthiban.Veerasooran]

Hi Andrew,

On 22/05/23 6:08 pm, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
>> -#define PHY_ID_LAN867X 0x0007C160
>> +#define PHY_ID_LAN867X_REVB1 0x0007C162
> 
>>   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,
>> @@ -124,7 +124,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) },
>>        { }
>>   };
> 
> Maybe i asked this last time...
> 
> What versions actually exist? The old entry would match 0x0007C16X, so
> 0x0007C160 and 0x0007C161, 0x0007C162, if those actually exist. Now
> you are narrowing it down to just 0x0007C162.
> 
> It would be good to comment on this in the commit message, that
> 0x0007C160 and 0x0007C161 never escaped the testing facility and hence
> don't need to be supported.
Ah ok, I will do it in the next version.

Best Regards,
Parthiban V
> 
>          Andrew

[PATCH net-next v2 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 to be checked before proceeding for the
initial configuration.

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 869c7f403ea1..2f22a1954c09 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

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

[Andrew Lunn]

On Mon, May 22, 2023 at 05:03:29PM +0530, Parthiban Veerasooran wrote:
> As per the datasheet DS-LAN8670-1-2-60001573C.pdf, the Reset Complete
> status bit in the STS2 register to be checked before proceeding for the
> initial configuration.

Is this the unmaskable interrupt status bit which needs clearing?
There is no mention of interrupts here.

	Andrew

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

[Parthiban.Veerasooran]

Hi Andrew,

On 22/05/23 6:13 pm, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Mon, May 22, 2023 at 05:03:29PM +0530, Parthiban Veerasooran wrote:
>> As per the datasheet DS-LAN8670-1-2-60001573C.pdf, the Reset Complete
>> status bit in the STS2 register to be checked before proceeding for the
>> initial configuration.
> 
> Is this the unmaskable interrupt status bit which needs clearing?
Yes, it is non-maskable interrupt.
> There is no mention of interrupts here.
The device will assert the Reset Complete (RESETC) bit in the Status 2 
(STS2) register to indicate that it has completed its internal 
initialization and is ready for configuration. As the Reset Complete 
status is non-maskable, the IRQ_N pin will always be asserted and driven 
low following a device reset. Upon reading of the Status 2 register, the 
pending Reset Complete status bit will be automatically cleared causing 
the IRQ_N pin to be released and pulled high again.

Do you think it makes sense to add these explanation regarding the reset 
and interrupt behavior with the above comment for a better understanding?

Note: This explanation is pulled from DS-LAN8670-1-2-60001573C.pdf

Best Regards,
Parthiban V
> 
>          Andrew

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

[Andrew Lunn]

On Tue, May 23, 2023 at 05:30:06AM +0000, Parthiban.Veerasooran@microchip.com wrote:
> Hi Andrew,
> 
> On 22/05/23 6:13 pm, Andrew Lunn wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > 
> > On Mon, May 22, 2023 at 05:03:29PM +0530, Parthiban Veerasooran wrote:
> >> As per the datasheet DS-LAN8670-1-2-60001573C.pdf, the Reset Complete
> >> status bit in the STS2 register to be checked before proceeding for the
> >> initial configuration.
> > 
> > Is this the unmaskable interrupt status bit which needs clearing?
> Yes, it is non-maskable interrupt.
> > There is no mention of interrupts here.
> The device will assert the Reset Complete (RESETC) bit in the Status 2 
> (STS2) register to indicate that it has completed its internal 
> initialization and is ready for configuration. As the Reset Complete 
> status is non-maskable, the IRQ_N pin will always be asserted and driven 
> low following a device reset. Upon reading of the Status 2 register, the 
> pending Reset Complete status bit will be automatically cleared causing 
> the IRQ_N pin to be released and pulled high again.
> 
> Do you think it makes sense to add these explanation regarding the reset 
> and interrupt behavior with the above comment for a better understanding?

Comments should explain 'Why?'. At the moment, it is not clear why you
are reading the status. The discussion so far has been about clearing
the interrupt, not about checking it has actually finished its
internal reset. So i think you should be mentioning interrupts
somewhere. Especially since this is a rather odd behaviour.

	   Andrew

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

[Parthiban.Veerasooran]

Hi Andrew,

On 23/05/23 5:53 pm, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Tue, May 23, 2023 at 05:30:06AM +0000, Parthiban.Veerasooran@microchip.com wrote:
>> Hi Andrew,
>>
>> On 22/05/23 6:13 pm, Andrew Lunn wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> On Mon, May 22, 2023 at 05:03:29PM +0530, Parthiban Veerasooran wrote:
>>>> As per the datasheet DS-LAN8670-1-2-60001573C.pdf, the Reset Complete
>>>> status bit in the STS2 register to be checked before proceeding for the
>>>> initial configuration.
>>>
>>> Is this the unmaskable interrupt status bit which needs clearing?
>> Yes, it is non-maskable interrupt.
>>> There is no mention of interrupts here.
>> The device will assert the Reset Complete (RESETC) bit in the Status 2
>> (STS2) register to indicate that it has completed its internal
>> initialization and is ready for configuration. As the Reset Complete
>> status is non-maskable, the IRQ_N pin will always be asserted and driven
>> low following a device reset. Upon reading of the Status 2 register, the
>> pending Reset Complete status bit will be automatically cleared causing
>> the IRQ_N pin to be released and pulled high again.
>>
>> Do you think it makes sense to add these explanation regarding the reset
>> and interrupt behavior with the above comment for a better understanding?
> 
> Comments should explain 'Why?'. At the moment, it is not clear why you
> are reading the status. The discussion so far has been about clearing
> the interrupt, not about checking it has actually finished its
> internal reset. So i think you should be mentioning interrupts
> somewhere. Especially since this is a rather odd behaviour.
Shall I update the above commit message like below,

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

Or still I have a misunderstanding here?

Best Regards,
Parthiban V
> 
>             Andrew

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

[Parthiban.Veerasooran]

Hi Andrew,

Please ignore the previous reply for this comment and consider this one.

On 23/05/23 5:53 pm, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Tue, May 23, 2023 at 05:30:06AM +0000, Parthiban.Veerasooran@microchip.com wrote:
>> Hi Andrew,
>>
>> On 22/05/23 6:13 pm, Andrew Lunn wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> On Mon, May 22, 2023 at 05:03:29PM +0530, Parthiban Veerasooran wrote:
>>>> As per the datasheet DS-LAN8670-1-2-60001573C.pdf, the Reset Complete
>>>> status bit in the STS2 register to be checked before proceeding for the
>>>> initial configuration.
>>>
>>> Is this the unmaskable interrupt status bit which needs clearing?
>> Yes, it is non-maskable interrupt.
>>> There is no mention of interrupts here.
>> The device will assert the Reset Complete (RESETC) bit in the Status 2
>> (STS2) register to indicate that it has completed its internal
>> initialization and is ready for configuration. As the Reset Complete
>> status is non-maskable, the IRQ_N pin will always be asserted and driven
>> low following a device reset. Upon reading of the Status 2 register, the
>> pending Reset Complete status bit will be automatically cleared causing
>> the IRQ_N pin to be released and pulled high again.
>>
>> Do you think it makes sense to add these explanation regarding the reset
>> and interrupt behavior with the above comment for a better understanding?
> 
> Comments should explain 'Why?'. At the moment, it is not clear why you
> are reading the status. The discussion so far has been about clearing
> the interrupt, not about checking it has actually finished its
> internal reset. So i think you should be mentioning interrupts
> somewhere. Especially since this is a rather odd behaviour.
Shall I update the above commit message like below?

As per the datasheet DS-LAN8670-1-2-60001573C.pdf, during the Power ON 
Reset(POR)/Hard Reset/Soft Reset, the Reset Complete status bit in the 
STS2 register to be checked before proceeding for the initial 
configuration. Reading STS2 register will also clear the Reset Complete 
interrupt which is non-maskable.

Or still I have a misunderstanding here?

Best Regards,
Parthiban V
> 
>             Andrew

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

[Andrew Lunn]

> As per the datasheet DS-LAN8670-1-2-60001573C.pdf, during the Power ON 
> Reset(POR)/Hard Reset/Soft Reset, the Reset Complete status bit in the 
> STS2 register to be checked before proceeding for the initial 

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.

Otherwise, this is O.K.

	Andrew

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

[Parthiban.Veerasooran]

Hi Andrew,

On 24/05/23 5:33 pm, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
>> As per the datasheet DS-LAN8670-1-2-60001573C.pdf, during the Power ON
>> Reset(POR)/Hard Reset/Soft Reset, the Reset Complete status bit in the
>> STS2 register to be checked before proceeding for the initial
> 
> 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.
> 
> Otherwise, this is O.K.
Thanks for your feedback. I will prepare the next version and send for 
the review.

Best Regards,
Parthiban V
> 
>          Andrew
> 

[PATCH net-next v2 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.

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 2f22a1954c09..a612f7867df8 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)
@@ -104,17 +102,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

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

[Andrew Lunn]

On Mon, May 22, 2023 at 05:03:30PM +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.
> 
> Signed-off-by: Parthiban Veerasooran <Parthiban.Veerasooran@microchip.com>

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

    Andrew

[PATCH net-next v2 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 a612f7867df8..385d97d47cb4 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 straigt from AN1760 from 'calulation 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_results[5];
+	u16 cfg_params[ARRAY_SIZE(lan865x_revb0_fixup_cfg_regs)];
+	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
@@ -105,7 +272,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
@@ -126,17 +293,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 v2 6/6] net: phy: microchip_t1s: add support for Microchip LAN865x Rev.B0 PHYs

[Andrew Lunn]

> +static int lan865x_setup_cfgparam(struct phy_device *phydev)
> +{
> +	u16 cfg_results[5];
> +	u16 cfg_params[ARRAY_SIZE(lan865x_revb0_fixup_cfg_regs)];
> +	s8 offsets[2];
> +	int ret;

Reverse Christmas tree please.

> +
> +	ret = lan865x_generate_cfg_offsets(phydev, offsets);
> +	if (ret)
> +		return ret;
> +
> +	ret = lan865x_read_cfg_params(phydev, cfg_params);

Is this doing a read from fuses? Is anything documented about this?
What the values mean? Would a board designer ever need to use
different values? Or is this just a case of 'trust us', you don't need
to understand this magic.

> +	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);
> +}


	Andrew

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

[Parthiban.Veerasooran]

Hi Andrew,

On 22/05/23 6:26 pm, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
>> +static int lan865x_setup_cfgparam(struct phy_device *phydev)
>> +{
>> +     u16 cfg_results[5];
>> +     u16 cfg_params[ARRAY_SIZE(lan865x_revb0_fixup_cfg_regs)];
>> +     s8 offsets[2];
>> +     int ret;
> 
> Reverse Christmas tree please.
Ah yes, surely will correct it in the next version.
> 
>> +
>> +     ret = lan865x_generate_cfg_offsets(phydev, offsets);
>> +     if (ret)
>> +             return ret;
>> +
>> +     ret = lan865x_read_cfg_params(phydev, cfg_params);
> 
> Is this doing a read from fuses? Is anything documented about this?
> What the values mean? Would a board designer ever need to use
> different values? Or is this just a case of 'trust us', you don't need
> to understand this magic.
Yes, it is a read from fuses and those values are specific/unique for 
each PHY chip. Those values are calculated based on some characteristics 
of the PHY chip behavior for optimal performance and they are fused in 
the PHY chip for the driver to configure it during the initialization. 
This is done in the production/testing stage of the PHY chip. As it is 
specific to PHY chip, a board designer doesn't have any influence on 
this and need not to worry about it. Unfortunately they can't be 
documented anywhere as they are design specific. So simply 'trust us'.

Best Regards,
Parthiban V
> 
>> +     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);
>> +}
> 
> 
>          Andrew
> 

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

[Andrew Lunn]

> > Is this doing a read from fuses? Is anything documented about this?
> > What the values mean? Would a board designer ever need to use
> > different values? Or is this just a case of 'trust us', you don't need
> > to understand this magic.
> Yes, it is a read from fuses and those values are specific/unique for 
> each PHY chip. Those values are calculated based on some characteristics 
> of the PHY chip behavior for optimal performance and they are fused in 
> the PHY chip for the driver to configure it during the initialization. 
> This is done in the production/testing stage of the PHY chip. As it is 
> specific to PHY chip, a board designer doesn't have any influence on 
> this and need not to worry about it. Unfortunately they can't be 
> documented anywhere as they are design specific. So simply 'trust us'.

O.K. Please consider for future generations that you move all this
magic into the PHY firmware. There does not seem to be any reason the
OS needs to know about this.

     Andrew

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

[Parthiban.Veerasooran]

Hi Andrew,

On 23/05/23 5:59 pm, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
>>> Is this doing a read from fuses? Is anything documented about this?
>>> What the values mean? Would a board designer ever need to use
>>> different values? Or is this just a case of 'trust us', you don't need
>>> to understand this magic.
>> Yes, it is a read from fuses and those values are specific/unique for
>> each PHY chip. Those values are calculated based on some characteristics
>> of the PHY chip behavior for optimal performance and they are fused in
>> the PHY chip for the driver to configure it during the initialization.
>> This is done in the production/testing stage of the PHY chip. As it is
>> specific to PHY chip, a board designer doesn't have any influence on
>> this and need not to worry about it. Unfortunately they can't be
>> documented anywhere as they are design specific. So simply 'trust us'.
> 
> O.K. Please consider for future generations that you move all this
> magic into the PHY firmware. There does not seem to be any reason the
> OS needs to know about this.
Thanks for the feedback.

Best Regards,
Parthiban V
> 
>       Andrew

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

[Simon Horman]

On Mon, May 22, 2023 at 05:03:31PM +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>

Hi Parthiban,

thanks for your patch.
Some minor nits from my side.

...

> +/* This is pulled straigt from AN1760 from 'calulation of offset 1' &
> + * 'calculation of offset 2'
> + */

nit: s/straigt/straight/

> +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_setup_cfgparam(struct phy_device *phydev)
> +{
> +	u16 cfg_results[5];
> +	u16 cfg_params[ARRAY_SIZE(lan865x_revb0_fixup_cfg_regs)];
> +	s8 offsets[2];
> +	int ret;

nit: Please use reverse xmas tree order - longest line to shortest -
     for local variable declarations in networking code.

...

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

[Parthiban.Veerasooran]

Hi Simon,

Thanks for your interest in reviewing my patches.

On 22/05/23 8:32 pm, Simon Horman wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Mon, May 22, 2023 at 05:03:31PM +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>
> 
> Hi Parthiban,
> 
> thanks for your patch.
> Some minor nits from my side.
> 
> ...
> 
>> +/* This is pulled straigt from AN1760 from 'calulation of offset 1' &
>> + * 'calculation of offset 2'
>> + */
> 
> nit: s/straigt/straight/
Ah yes, surely will correct it in the next version.
> 
>> +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_setup_cfgparam(struct phy_device *phydev)
>> +{
>> +     u16 cfg_results[5];
>> +     u16 cfg_params[ARRAY_SIZE(lan865x_revb0_fixup_cfg_regs)];
>> +     s8 offsets[2];
>> +     int ret;
> 
> nit: Please use reverse xmas tree order - longest line to shortest -
>       for local variable declarations in networking code.
Yes understood, surely will correct it in the next version.

Best Regards,
Parthiban V
> 
> ...