[PATCH 0/3] net: microchip_t1s: additional phy support and collision detect handling

[PATCH 0/3] net: microchip_t1s: additional phy support and collision detect handling

[Ramón N.Rodriguez]

Hi,

This patch series adds support for LAN867x Rev.C and sets the collision
enable bit conditionally on plca enable for LAN865X and LAN867X phys.

The addition of Rev.C support is pretty straight forward (but weird), it 
follows the recommended magic and unexplained steps in Microchip
Application Note 1699.

The second change, conditionally setting collision detection has a
significant impact on link performance. Having both PLCA and collision
detection enabled led to a lot of dropped packets in the setup I've been
developing on.
The datasheets recommends disabling collision detect when PLCA is
enabled.
Took me a couple of weeks to find said footnote, hoping this series can
save the next dev some headache.

Following is a brief description of my test/eval setup:

LAN867x Rev.C
This was tested with Microchips usb eval board, the only testing I did
was that I got the driver attached and got a link.
No performance/stress testing has been performed.

Collision detection
This has been tested on a setup where one ARM system with a LAN8650
mac-phy is daisy-chained to 8 mcus using lan8670 phys. Without the patch we
were limited to really short cables, about 1m per node, but we were
still getting a lot of connection drops.
With the patch we could increase the total cable length to at least 40M.
Electrical properties play in here, both cable types and how the
termination is designed, so these results might not be reproducible in
another setup.
The only testing that has been performed has been a best effort of
estimating dropped packets on the linux node, with low frequency traffic.

Ramón Nordin Rodriguez (3):
  net: microchip_t1s: refactor reset functionality
  net: microchip_t1s: add support for LAN867x Rev.C1
  net: microchip_t1s: conditional collision detection

 drivers/net/phy/microchip_t1s.c | 135 +++++++++++++++++++++++++++++++-
 1 file changed, 132 insertions(+), 3 deletions(-)

-- 
2.40.1

[PATCH 1/3] net: microchip_t1s: refactor reset functionality

[Ramón N.Rodriguez]

From: Ramón Nordin Rodriguez <ramon.nordin.rodriguez@ferroamp.se>

This commit moves the reset functionality for lan867x from the revb1
init function to a separate function. The intention with this minor
refactor is to prepare for adding support for lan867x rev C1.

Signed-off-by: Ramón Nordin Rodriguez <ramon.nordin.rodriguez@ferroamp.se>
---
 drivers/net/phy/microchip_t1s.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/microchip_t1s.c b/drivers/net/phy/microchip_t1s.c
index 534ca7d1b061..ace2bf35a18a 100644
--- a/drivers/net/phy/microchip_t1s.c
+++ b/drivers/net/phy/microchip_t1s.c
@@ -213,7 +213,7 @@ static int lan865x_revb0_config_init(struct phy_device *phydev)
 	return lan865x_setup_cfgparam(phydev);
 }
 
-static int lan867x_revb1_config_init(struct phy_device *phydev)
+static int lan867x_wait_for_reset_complete(struct phy_device *phydev)
 {
 	int err;
 
@@ -234,6 +234,16 @@ static int lan867x_revb1_config_init(struct phy_device *phydev)
 			return -ENODEV;
 		}
 	}
+	return 0;
+}
+
+static int lan867x_revb1_config_init(struct phy_device *phydev)
+{
+	int err;
+
+	err = lan867x_wait_for_reset_complete(phydev);
+	if (err)
+		return err;
 
 	/* Reference to AN1699
 	 * https://ww1.microchip.com/downloads/aemDocuments/documents/AIS/ProductDocuments/SupportingCollateral/AN-LAN8670-1-2-config-60001699.pdf
-- 
2.40.1

[PATCH 2/3] net: microchip_t1s: add support for LAN867x Rev.C1

[Ramón N.Rodriguez]

From: Ramón Nordin Rodriguez <ramon.nordin.rodriguez@ferroamp.se>

This commit adds support for yet another Microchip T1S lan867x phy
revision. The only meaningful difference between Rev.B that already is
supported and Rev.C is the init change where other undocumented regs
needs writes with unexplained values.
The changes introduced here attempts to follow the manufacturer
recommendations in AN1699.

Signed-off-by: Ramón Nordin Rodriguez <ramon.nordin.rodriguez@ferroamp.se>
---
 drivers/net/phy/microchip_t1s.c | 96 +++++++++++++++++++++++++++++++++
 1 file changed, 96 insertions(+)

diff --git a/drivers/net/phy/microchip_t1s.c b/drivers/net/phy/microchip_t1s.c
index ace2bf35a18a..db84d850b165 100644
--- a/drivers/net/phy/microchip_t1s.c
+++ b/drivers/net/phy/microchip_t1s.c
@@ -12,6 +12,7 @@
 #include <linux/phy.h>
 
 #define PHY_ID_LAN867X_REVB1 0x0007C162
+#define PHY_ID_LAN867X_REVC1 0x0007C164
 #define PHY_ID_LAN865X_REVB0 0x0007C1B3
 
 #define LAN867X_REG_STS2 0x0019
@@ -59,6 +60,22 @@ static const u16 lan867x_revb1_fixup_masks[12] = {
 	0x0600, 0x7F00, 0x2000, 0xFFFF,
 };
 
+static const u16 lan867x_revc1_fixup_registers[12] = {
+	0x00D0, 0x00E0, 0x0084, 0x008A,
+	0x00E9, 0x00F5, 0x00F4, 0x00F8,
+	0x00F9, 0x0081, 0x0091, 0x0093,
+};
+
+/* Index 2 & 3 will not be used, these are runtime populated/calculated.
+ * It makes the code a lot easier to read having this arr the same len
+ * as the _fixup_registers arr though
+ */
+static const u16 lan867x_revc1_fixup_values[12] = {
+	0x3F31, 0xC000, 0xFFFF, 0xFFFF,
+	0x9E50, 0x1CF8, 0xC020, 0x9B00,
+	0x4E53, 0x0080, 0x9660, 0x06E9,
+};
+
 /* LAN865x Rev.B0 configuration parameters from AN1760 */
 static const u32 lan865x_revb0_fixup_registers[28] = {
 	0x0091, 0x0081, 0x0043, 0x0044,
@@ -263,6 +280,74 @@ static int lan867x_revb1_config_init(struct phy_device *phydev)
 	return 0;
 }
 
+static int lan867x_revc1_read_fixup_value(struct phy_device *phydev, u16 addr)
+{
+	int regval;
+	/* The AN pretty much just states 'trust us' regarding these magic vals */
+	const u16 magic_or = 0xE0;
+	const u16 magic_reg_mask = 0x1F;
+	const u16 magic_check_mask = 0x10;
+
+	regval = lan865x_revb0_indirect_read(phydev, addr);
+	if (regval < 0)
+		return regval;
+
+	regval &= magic_reg_mask;
+
+	return (regval & magic_check_mask) ? regval | magic_or : regval;
+}
+
+static int lan867x_revc1_config_init(struct phy_device *phydev)
+{
+	int err;
+	int regval;
+	u16 override0;
+	u16 override1;
+	const u16 override_addr0 = 0x4;
+	const u16 override_addr1 = 0x8;
+	const u8 index_to_override0 = 2;
+	const u8 index_to_override1 = 3;
+
+	err = lan867x_wait_for_reset_complete(phydev);
+	if (err)
+		return err;
+
+	/* The application note specifies a super convenient process
+	 * where 2 of the fixup regs needs a write with a value that is
+	 * a modified result of another reg read.
+	 * Enjoy the magic show.
+	 */
+	regval = lan867x_revc1_read_fixup_value(phydev, override_addr0);
+	if (regval < 0)
+		return regval;
+	override0 = ((regval + 9) << 10) | ((regval + 14) << 4) | 0x3;
+
+	regval = lan867x_revc1_read_fixup_value(phydev, override_addr1);
+	if (regval < 0)
+		return regval;
+	override1 = (regval + 40) << 10;
+
+	for (int i = 0; i < ARRAY_SIZE(lan867x_revc1_fixup_registers); i++) {
+		/* The hardcoded  */
+		if (i == index_to_override0)
+			err = phy_write_mmd(phydev, MDIO_MMD_VEND2,
+					    lan867x_revc1_fixup_registers[i],
+					    override0);
+		else if (i == index_to_override1)
+			err = phy_write_mmd(phydev, MDIO_MMD_VEND2,
+					    lan867x_revc1_fixup_registers[i],
+					    override1);
+		else
+			err = phy_write_mmd(phydev, MDIO_MMD_VEND2,
+					    lan867x_revc1_fixup_registers[i],
+					    lan867x_revc1_fixup_values[i]);
+		if (err)
+			return err;
+	}
+
+	return 0;
+}
+
 static int lan86xx_read_status(struct phy_device *phydev)
 {
 	/* The phy has some limitations, namely:
@@ -289,6 +374,16 @@ static struct phy_driver microchip_t1s_driver[] = {
 		.set_plca_cfg	    = genphy_c45_plca_set_cfg,
 		.get_plca_status    = genphy_c45_plca_get_status,
 	},
+	{
+		PHY_ID_MATCH_EXACT(PHY_ID_LAN867X_REVC1),
+		.name               = "LAN867X Rev.C1",
+		.features           = PHY_BASIC_T1S_P2MP_FEATURES,
+		.config_init        = lan867x_revc1_config_init,
+		.read_status        = lan86xx_read_status,
+		.get_plca_cfg	    = genphy_c45_plca_get_cfg,
+		.set_plca_cfg	    = genphy_c45_plca_set_cfg,
+		.get_plca_status    = genphy_c45_plca_get_status,
+	},
 	{
 		PHY_ID_MATCH_EXACT(PHY_ID_LAN865X_REVB0),
 		.name               = "LAN865X Rev.B0 Internal Phy",
@@ -305,6 +400,7 @@ module_phy_driver(microchip_t1s_driver);
 
 static struct mdio_device_id __maybe_unused tbl[] = {
 	{ PHY_ID_MATCH_EXACT(PHY_ID_LAN867X_REVB1) },
+	{ PHY_ID_MATCH_EXACT(PHY_ID_LAN867X_REVC1) },
 	{ PHY_ID_MATCH_EXACT(PHY_ID_LAN865X_REVB0) },
 	{ }
 };
-- 
2.40.1

[PATCH 3/3] net: microchip_t1s: conditional collision detection

[Ramón N.Rodriguez]

From: Ramón Nordin Rodriguez <ramon.nordin.rodriguez@ferroamp.se>

This commit conditionally sets the collision detection bit on lan867x
and lan865x phys on changing PLCA enabled on/off. The intended realworld
scenario is that all nodes on the network run the same settings with
regards to plca, and when plca is enabled the physical layer guarantees
that no collisions should occur.
In a practical setting where it was tested running one node with
collision detection on and other off, the node with collision detection
on dropped a lot of packets, leading to a poor performing link.
Worth noting here is that the phys default/reset to plca disabled and
collision detection enabled. Thus this change would only have an effect
when changing settings via ethtool.

Signed-off-by: Ramón Nordin Rodriguez <ramon.nordin.rodriguez@ferroamp.se>
---
 drivers/net/phy/microchip_t1s.c | 29 ++++++++++++++++++++++++++---
 1 file changed, 26 insertions(+), 3 deletions(-)

diff --git a/drivers/net/phy/microchip_t1s.c b/drivers/net/phy/microchip_t1s.c
index db84d850b165..3b1e82ecdf69 100644
--- a/drivers/net/phy/microchip_t1s.c
+++ b/drivers/net/phy/microchip_t1s.c
@@ -23,8 +23,10 @@
 #define LAN865X_REG_CFGPARAM_DATA 0x00D9
 #define LAN865X_REG_CFGPARAM_CTRL 0x00DA
 #define LAN865X_REG_STS2 0x0019
+#define LAN86XX_REG_COLLISION_DETECT 0x0087
 
 #define LAN865X_CFGPARAM_READ_ENABLE BIT(1)
+#define LAN86XX_COLLISION_DETECT_ENABLE BIT(15)
 
 /* The arrays below are pulled from the following table from AN1699
  * Access MMD Address Value Mask
@@ -363,6 +365,27 @@ static int lan86xx_read_status(struct phy_device *phydev)
 	return 0;
 }
 
+static int lan86xx_plca_set_cfg(struct phy_device *phydev,
+				const struct phy_plca_cfg *plca_cfg)
+{
+	int err;
+
+	err = genphy_c45_plca_set_cfg(phydev, plca_cfg);
+	if (err)
+		return err;
+
+	/* Disable collision detect on the phy when PLCA is enabled.
+	 * Noise can be picked up as a false positive for collisions
+	 * leading to the phy dropping legitimate packets.
+	 * No collisions should be possible when all nodes are setup
+	 * for running PLCA.
+	 */
+	return phy_modify_mmd(phydev, MDIO_MMD_VEND2,
+			LAN86XX_REG_COLLISION_DETECT,
+			LAN86XX_COLLISION_DETECT_ENABLE,
+			plca_cfg->enabled ? 0 : LAN86XX_COLLISION_DETECT_ENABLE);
+}
+
 static struct phy_driver microchip_t1s_driver[] = {
 	{
 		PHY_ID_MATCH_EXACT(PHY_ID_LAN867X_REVB1),
@@ -371,7 +394,7 @@ static struct phy_driver microchip_t1s_driver[] = {
 		.config_init        = lan867x_revb1_config_init,
 		.read_status        = lan86xx_read_status,
 		.get_plca_cfg	    = genphy_c45_plca_get_cfg,
-		.set_plca_cfg	    = genphy_c45_plca_set_cfg,
+		.set_plca_cfg	    = lan86xx_plca_set_cfg,
 		.get_plca_status    = genphy_c45_plca_get_status,
 	},
 	{
@@ -381,7 +404,7 @@ static struct phy_driver microchip_t1s_driver[] = {
 		.config_init        = lan867x_revc1_config_init,
 		.read_status        = lan86xx_read_status,
 		.get_plca_cfg	    = genphy_c45_plca_get_cfg,
-		.set_plca_cfg	    = genphy_c45_plca_set_cfg,
+		.set_plca_cfg	    = lan86xx_plca_set_cfg,
 		.get_plca_status    = genphy_c45_plca_get_status,
 	},
 	{
@@ -391,7 +414,7 @@ static struct phy_driver microchip_t1s_driver[] = {
 		.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,
+		.set_plca_cfg	    = lan86xx_plca_set_cfg,
 		.get_plca_status    = genphy_c45_plca_get_status,
 	},
 };
-- 
2.40.1

Re: [PATCH 1/3] net: microchip_t1s: refactor reset functionality

[Andrew Lunn]

On Mon, Nov 27, 2023 at 11:40:43AM +0100, Ramón N.Rodriguez wrote:
> From: Ramón Nordin Rodriguez <ramon.nordin.rodriguez@ferroamp.se>
> 
> This commit moves the reset functionality for lan867x from the revb1
> init function to a separate function. The intention with this minor
> refactor is to prepare for adding support for lan867x rev C1.
> 
> Signed-off-by: Ramón Nordin Rodriguez <ramon.nordin.rodriguez@ferroamp.se>

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

    Andrew

Re: [PATCH 2/3] net: microchip_t1s: add support for LAN867x Rev.C1

[Andrew Lunn]

>  #define PHY_ID_LAN867X_REVB1 0x0007C162
> +#define PHY_ID_LAN867X_REVC1 0x0007C164

So there is a gap in the revisions. Maybe a B2 exists?

> +static int lan867x_revc1_read_fixup_value(struct phy_device *phydev, u16 addr)
> +{
> +	int regval;
> +	/* The AN pretty much just states 'trust us' regarding these magic vals */
> +	const u16 magic_or = 0xE0;
> +	const u16 magic_reg_mask = 0x1F;
> +	const u16 magic_check_mask = 0x10;

Reverse christmass tree please. Longest first, shorted last.

> +	regval = lan865x_revb0_indirect_read(phydev, addr);
> +	if (regval < 0)
> +		return regval;
> +
> +	regval &= magic_reg_mask;
> +
> +	return (regval & magic_check_mask) ? regval | magic_or : regval;
> +}
> +
> +static int lan867x_revc1_config_init(struct phy_device *phydev)
> +{
> +	int err;
> +	int regval;
> +	u16 override0;
> +	u16 override1;
> +	const u16 override_addr0 = 0x4;
> +	const u16 override_addr1 = 0x8;
> +	const u8 index_to_override0 = 2;
> +	const u8 index_to_override1 = 3;

Same here.

> +
> +	err = lan867x_wait_for_reset_complete(phydev);
> +	if (err)
> +		return err;
> +
> +	/* The application note specifies a super convenient process
> +	 * where 2 of the fixup regs needs a write with a value that is
> +	 * a modified result of another reg read.
> +	 * Enjoy the magic show.
> +	 */

I really do hope that by revision D1 they get the firmware sorted out
so none of this undocumented magic is needed.

	Andrew

Re: [PATCH 0/3] net: microchip_t1s: additional phy support and collision detect handling

[Andrew Lunn]

> Collision detection
> This has been tested on a setup where one ARM system with a LAN8650
> mac-phy is daisy-chained to 8 mcus using lan8670 phys. Without the patch we
> were limited to really short cables, about 1m per node, but we were
> still getting a lot of connection drops.
> With the patch we could increase the total cable length to at least 40M.

Did you do any testing of collision detection enabled, PLCA disabled?

You say you think this is noise related. But the noise should be the
same with or without PLCA. I'm just thinking maybe collision detection
is just plain broken and should always be disabled?

I've not read much about T1S, but if we assume it is doing good old
fashioned CSMA/CD, with short cables the CS bit works well and the CD
is less important. CD was needed when you have 1000m cable, and you
can fit 64 bytes on the 1000m cable. So always turning of CD might be
appropriate.

	Andrew

Re: [PATCH 2/3] net: microchip_t1s: add support for LAN867x Rev.C1

[Ramón Nordin Rodriguez]

On Mon, Nov 27, 2023 at 02:37:41PM +0100, Andrew Lunn wrote:
> >  #define PHY_ID_LAN867X_REVB1 0x0007C162
> > +#define PHY_ID_LAN867X_REVC1 0x0007C164
> 
> So there is a gap in the revisions. Maybe a B2 exists?

The datasheet lists A0, B1 and C1, seems like Microchip removes the
application notes for old revisions, so no way that I can see to add the
init-fixup for A0.

I'm guessing there is a rev.c0 that was never released to the public.

> > +	const u16 magic_or = 0xE0;
> > +	const u16 magic_reg_mask = 0x1F;
> > +	const u16 magic_check_mask = 0x10;
> 
> Reverse christmass tree please. Longest first, shorted last.

My bad, I was just thinking 'christmas tree' forgot about the reverse.
I'll fix that.

> > +	int err;
> > +	int regval;
> > +	u16 override0;
> > +	u16 override1;
> > +	const u16 override_addr0 = 0x4;
> > +	const u16 override_addr1 = 0x8;
> > +	const u8 index_to_override0 = 2;
> > +	const u8 index_to_override1 = 3;
> 
> Same here.
I'll fix this.

> 
> > +
> > +	err = lan867x_wait_for_reset_complete(phydev);
> > +	if (err)
> > +		return err;
> > +
> > +	/* The application note specifies a super convenient process
> > +	 * where 2 of the fixup regs needs a write with a value that is
> > +	 * a modified result of another reg read.
> > +	 * Enjoy the magic show.
> > +	 */
> 
> I really do hope that by revision D1 they get the firmware sorted out
> so none of this undocumented magic is needed.
> 
> 	Andrew

Really do hope so.. 

Re: [PATCH 0/3] net: microchip_t1s: additional phy support and collision detect handling

[Ramón Nordin Rodriguez]

On Mon, Nov 27, 2023 at 02:58:32PM +0100, Andrew Lunn wrote:
> > Collision detection
> > This has been tested on a setup where one ARM system with a LAN8650
> > mac-phy is daisy-chained to 8 mcus using lan8670 phys. Without the patch we
> > were limited to really short cables, about 1m per node, but we were
> > still getting a lot of connection drops.
> > With the patch we could increase the total cable length to at least 40M.
> 
> Did you do any testing of collision detection enabled, PLCA disabled?
> 

In our dev system we've only tested with PLCA enabled, bit too tricky
changing internals on the microcontrollers.
But I have a lot of usb eval dongles that I can test with.

> You say you think this is noise related. But the noise should be the
> same with or without PLCA. I'm just thinking maybe collision detection
> is just plain broken and should always be disabled?
> 

I don't have access to the equipment to measure noise or reflections,
I've looked at the link with an oscilloscope and it looked fine to me.
The reason I'm mentioning noise is just me parroting the datasheet, for
context I'll quote the footnote here

"No physical collisions will occur when all nodes in a mixing segment are properly
configured for PLCA operation. As a result, for improved performance in high noise
environments where false collisions may be detected leading to dropped packets, it is
recommended that the user write this bit to a ‘0’ to disable collision detection when PLCA
is enabled. When collision detection is disabled, the PLCA reconciliation sublayer will still
assert logical collisions to the MAC as part of normal operation."
LAN8650 datasheet 11.5.51

> I've not read much about T1S, but if we assume it is doing good old
> fashioned CSMA/CD, with short cables the CS bit works well and the CD
> is less important. CD was needed when you have 1000m cable, and you
> can fit 64 bytes on the 1000m cable. So always turning of CD might be
> appropriate.
> 
> 	Andrew

As you assume when PLCA is disabled the phy runs in CSMA/CD mode.

I'll do some tests with both PLCA and CD off/disabled. My thinking is that a
adequate test bench would look like

* 3-4 nodes (depending on how many usb ports and dongles I have)
* run iperf with long cables and CSMA/CD
* run iperf with long cables and CMSA/No CD

I'll report back the results. Anything you'd like to add/focus on with
evaluation?

Ramón

Re: [PATCH 3/3] net: microchip_t1s: conditional collision detection

[Parthiban.Veerasooran]

Hi,

This implementation was introduced in the below patch itself.

https://lore.kernel.org/netdev/20230426205049.xlfqluzwcvlm6ihh@soft-dev3-1/T/#m9a52b6c03b7fa637f70aed306b50b442590e24a3

As it is recommended to do it in a separate patch and also the 
datasheets of LAN867X Rev.B1 and LAN865X Rev.B0 internal PHY have these 
register is reserved, we were working for a feasible solution to 
describe this for customer and mainline. By the time many other things 
messed up and couldn't reach the mainline on time.

We also implemented LAN867X Rev.C1 support already in the driver and 
published in our product site and in the process of preparing mainline 
patches. But unfortunately it took little more time to make it.

https://ww1.microchip.com/downloads/aemDocuments/documents/AIS/ProductDocuments/CodeExamples/EVB-LAN8670-USB_Linux_Driver_1v0.zip

Anyway, thank you for the support. Good luck!

Best regards,
Parthiban V

On 27/11/23 4:10 pm, Ramón N.Rodriguez wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> From: Ramón Nordin Rodriguez <ramon.nordin.rodriguez@ferroamp.se>
> 
> This commit conditionally sets the collision detection bit on lan867x
> and lan865x phys on changing PLCA enabled on/off. The intended realworld
> scenario is that all nodes on the network run the same settings with
> regards to plca, and when plca is enabled the physical layer guarantees
> that no collisions should occur.
> In a practical setting where it was tested running one node with
> collision detection on and other off, the node with collision detection
> on dropped a lot of packets, leading to a poor performing link.
> Worth noting here is that the phys default/reset to plca disabled and
> collision detection enabled. Thus this change would only have an effect
> when changing settings via ethtool.
> 
> Signed-off-by: Ramón Nordin Rodriguez <ramon.nordin.rodriguez@ferroamp.se>
> ---
>   drivers/net/phy/microchip_t1s.c | 29 ++++++++++++++++++++++++++---
>   1 file changed, 26 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/phy/microchip_t1s.c b/drivers/net/phy/microchip_t1s.c
> index db84d850b165..3b1e82ecdf69 100644
> --- a/drivers/net/phy/microchip_t1s.c
> +++ b/drivers/net/phy/microchip_t1s.c
> @@ -23,8 +23,10 @@
>   #define LAN865X_REG_CFGPARAM_DATA 0x00D9
>   #define LAN865X_REG_CFGPARAM_CTRL 0x00DA
>   #define LAN865X_REG_STS2 0x0019
> +#define LAN86XX_REG_COLLISION_DETECT 0x0087
> 
>   #define LAN865X_CFGPARAM_READ_ENABLE BIT(1)
> +#define LAN86XX_COLLISION_DETECT_ENABLE BIT(15)
> 
>   /* The arrays below are pulled from the following table from AN1699
>    * Access MMD Address Value Mask
> @@ -363,6 +365,27 @@ static int lan86xx_read_status(struct phy_device *phydev)
>          return 0;
>   }
> 
> +static int lan86xx_plca_set_cfg(struct phy_device *phydev,
> +                               const struct phy_plca_cfg *plca_cfg)
> +{
> +       int err;
> +
> +       err = genphy_c45_plca_set_cfg(phydev, plca_cfg);
> +       if (err)
> +               return err;
> +
> +       /* Disable collision detect on the phy when PLCA is enabled.
> +        * Noise can be picked up as a false positive for collisions
> +        * leading to the phy dropping legitimate packets.
> +        * No collisions should be possible when all nodes are setup
> +        * for running PLCA.
> +        */
> +       return phy_modify_mmd(phydev, MDIO_MMD_VEND2,
> +                       LAN86XX_REG_COLLISION_DETECT,
> +                       LAN86XX_COLLISION_DETECT_ENABLE,
> +                       plca_cfg->enabled ? 0 : LAN86XX_COLLISION_DETECT_ENABLE);
> +}
> +
>   static struct phy_driver microchip_t1s_driver[] = {
>          {
>                  PHY_ID_MATCH_EXACT(PHY_ID_LAN867X_REVB1),
> @@ -371,7 +394,7 @@ static struct phy_driver microchip_t1s_driver[] = {
>                  .config_init        = lan867x_revb1_config_init,
>                  .read_status        = lan86xx_read_status,
>                  .get_plca_cfg       = genphy_c45_plca_get_cfg,
> -               .set_plca_cfg       = genphy_c45_plca_set_cfg,
> +               .set_plca_cfg       = lan86xx_plca_set_cfg,
>                  .get_plca_status    = genphy_c45_plca_get_status,
>          },
>          {
> @@ -381,7 +404,7 @@ static struct phy_driver microchip_t1s_driver[] = {
>                  .config_init        = lan867x_revc1_config_init,
>                  .read_status        = lan86xx_read_status,
>                  .get_plca_cfg       = genphy_c45_plca_get_cfg,
> -               .set_plca_cfg       = genphy_c45_plca_set_cfg,
> +               .set_plca_cfg       = lan86xx_plca_set_cfg,
>                  .get_plca_status    = genphy_c45_plca_get_status,
>          },
>          {
> @@ -391,7 +414,7 @@ static struct phy_driver microchip_t1s_driver[] = {
>                  .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,
> +               .set_plca_cfg       = lan86xx_plca_set_cfg,
>                  .get_plca_status    = genphy_c45_plca_get_status,
>          },
>   };
> --
> 2.40.1
> 
> 

Re: [PATCH 0/3] net: microchip_t1s: additional phy support and collision detect handling

[Andrew Lunn]

> * 3-4 nodes (depending on how many usb ports and dongles I have)
> * run iperf with long cables and CSMA/CD
> * run iperf with long cables and CMSA/No CD
> 
> I'll report back the results. Anything you'd like to add/focus on with
> evaluation?

Humm, thinking about how CSMA/CD works...

Maybe look at what counters the MAC provides. Does it have collisions
and bad FCS? A collision should result in a bad FCS, if you are not
using CD. So if things are working correctly, the count for CD should
move to FCS if you turn CD off. If CD is falsely triggering, FCS as a
% should not really change, but you probably get more frames over the
link?

	Andrew

Re: [PATCH 3/3] net: microchip_t1s: conditional collision detection

[Ramón Nordin Rodriguez]

On Mon, Nov 27, 2023 at 04:00:18PM +0000, Parthiban.Veerasooran@microchip.com wrote:
> Hi,
> 
> This implementation was introduced in the below patch itself.
> 
> https://lore.kernel.org/netdev/20230426205049.xlfqluzwcvlm6ihh@soft-dev3-1/T/#m9a52b6c03b7fa637f70aed306b50b442590e24a3
> 

But the change was dropped in that patchset right? It's not present in
netdev-next.

> As it is recommended to do it in a separate patch and also the 
> datasheets of LAN867X Rev.B1 and LAN865X Rev.B0 internal PHY have these 
> register is reserved, we were working for a feasible solution to 
> describe this for customer and mainline. By the time many other things 
> messed up and couldn't reach the mainline on time.
> 

Far as I can tell 'collision detect' is described in the following
sections of respective datasheet:

* 11.5.51 - LAN8650
* 5.4.48  - LAN8670

The rest of the bits are reserved though. The change I propose only
manipulate the documented (bit 15) collision bit.

Is your point that the lan8670 datasheet is only valid for rev.c and not
rev.b?

Andrew suggested on the cover letter that it be interesting to look at
completly disabling collision detect, any strings you can pull at
Microchip to investigate that?
Also any input on my suggested testing methodology is more than welcome.

> We also implemented LAN867X Rev.C1 support already in the driver and 
> published in our product site and in the process of preparing mainline 
> patches. But unfortunately it took little more time to make it.
> 
> https://ww1.microchip.com/downloads/aemDocuments/documents/AIS/ProductDocuments/CodeExamples/EVB-LAN8670-USB_Linux_Driver_1v0.zip

I'm aware, we've been using a derivative of that work at ferroamp for
development. But it's been driving me nuts, being the 't1s guy' at work,
and maintaining out of tree drivers for weird dev boxes.

It's not my intention to beat you to the punch, I just want a mainlined
driver so that I can spend less of my time on plumbing.

> 
> Anyway, thank you for the support. Good luck!

Likewise!
R

Re: [PATCH 3/3] net: microchip_t1s: conditional collision detection

[Parthiban.Veerasooran]

Hi,

On 27/11/23 10:02 pm, Ramón Nordin Rodriguez wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Mon, Nov 27, 2023 at 04:00:18PM +0000, Parthiban.Veerasooran@microchip.com wrote:
>> Hi,
>>
>> This implementation was introduced in the below patch itself.
>>
>> https://lore.kernel.org/netdev/20230426205049.xlfqluzwcvlm6ihh@soft-dev3-1/T/#m9a52b6c03b7fa637f70aed306b50b442590e24a3
>>
> 
> But the change was dropped in that patchset right? It's not present in
> netdev-next.
Yes, it was dropped. The reason why I gave this info is, you mentioned 
in the cover letter that it took some time for you to find this in the 
datasheet.
> 
>> As it is recommended to do it in a separate patch and also the
>> datasheets of LAN867X Rev.B1 and LAN865X Rev.B0 internal PHY have these
>> register is reserved, we were working for a feasible solution to
>> describe this for customer and mainline. By the time many other things
>> messed up and couldn't reach the mainline on time.
>>
> 
> Far as I can tell 'collision detect' is described in the following
> sections of respective datasheet:
> 
> * 11.5.51 - LAN8650
> * 5.4.48  - LAN8670
> 
> The rest of the bits are reserved though. The change I propose only
> manipulate the documented (bit 15) collision bit.
> 
> Is your point that the lan8670 datasheet is only valid for rev.c and not
> rev.b?
It is valid for rev.b1 as well but the current datasheet for rev.c1 
doesn't show that info.
> 
> Andrew suggested on the cover letter that it be interesting to look at
> completly disabling collision detect, any strings you can pull at
> Microchip to investigate that?
Unfortunately I can't commit anything from my side as we are occupied 
with other activities. But definitely I will try my level best if time 
permits. Alternatively you can contact our Microchip customer support 
team if you are interested to do this testing at Microchip.
> Also any input on my suggested testing methodology is more than welcome.
> 
>> We also implemented LAN867X Rev.C1 support already in the driver and
>> published in our product site and in the process of preparing mainline
>> patches. But unfortunately it took little more time to make it.
>>
>> https://ww1.microchip.com/downloads/aemDocuments/documents/AIS/ProductDocuments/CodeExamples/EVB-LAN8670-USB_Linux_Driver_1v0.zip
> 
> I'm aware, we've been using a derivative of that work at ferroamp for
> development. But it's been driving me nuts, being the 't1s guy' at work,
> and maintaining out of tree drivers for weird dev boxes.
> 
> It's not my intention to beat you to the punch, I just want a mainlined
> driver so that I can spend less of my time on plumbing.
I completely understand. Also it was not my intention too. Just to let 
you know why it is delayed in reaching mainline and a quick reference 
for the existing implementation. Enjoy!

Best regards,
Parthiban V
> 
>>
>> Anyway, thank you for the support. Good luck!
> 
> Likewise!
> R

Re: [PATCH 0/3] net: microchip_t1s: additional phy support and collision detect handling

[Ramón Nordin Rodriguez]

On Mon, Nov 27, 2023 at 05:03:54PM +0100, Andrew Lunn wrote:
> > * 3-4 nodes (depending on how many usb ports and dongles I have)
> > * run iperf with long cables and CSMA/CD
> > * run iperf with long cables and CMSA/No CD
> > 
> > I'll report back the results. Anything you'd like to add/focus on with
> > evaluation?
> 
> Humm, thinking about how CSMA/CD works...
> 
> Maybe look at what counters the MAC provides. Does it have collisions
> and bad FCS? A collision should result in a bad FCS, if you are not
> using CD. So if things are working correctly, the count for CD should
> move to FCS if you turn CD off. If CD is falsely triggering, FCS as a
> % should not really change, but you probably get more frames over the
> link?
> 

That is some really cool input, I have to do some datasheet digging and
hacking. I'll try to set everything up today, tomorrow I can hang in
the lab after hours and test things out!

Partihban suggested that Microchips support might be able to help with
testing, might give them a ping soon as I a solid plan.

Really appreciate the insight! 
R

Re: [PATCH 3/3] net: microchip_t1s: conditional collision detection

[Ramón Nordin Rodriguez]

> > 
> > But the change was dropped in that patchset right? It's not present in
> > netdev-next.
> Yes, it was dropped. The reason why I gave this info is, you mentioned 
> in the cover letter that it took some time for you to find this in the 
> datasheet.

Ha, sometimes I have bad luck while thinking. I guess I never understood
that change and subsequently forgot about it.

> > 
> >> As it is recommended to do it in a separate patch and also the
> >> datasheets of LAN867X Rev.B1 and LAN865X Rev.B0 internal PHY have these
> >> register is reserved, we were working for a feasible solution to
> >> describe this for customer and mainline. By the time many other things
> >> messed up and couldn't reach the mainline on time.
> >>
> > 
> > Far as I can tell 'collision detect' is described in the following
> > sections of respective datasheet:
> > 
> > * 11.5.51 - LAN8650
> > * 5.4.48  - LAN8670
> > 
> > The rest of the bits are reserved though. The change I propose only
> > manipulate the documented (bit 15) collision bit.
> > 
> > Is your point that the lan8670 datasheet is only valid for rev.c and not
> > rev.b?
> It is valid for rev.b1 as well but the current datasheet for rev.c1 
> doesn't show that info.

Thank you for clearing that up! So if I get you correctly this change
would in fact be correct for both lan867x rev.b and rev.c.

> > 
> > Andrew suggested on the cover letter that it be interesting to look at
> > completly disabling collision detect, any strings you can pull at
> > Microchip to investigate that?
> Unfortunately I can't commit anything from my side as we are occupied 
> with other activities. But definitely I will try my level best if time 
> permits. Alternatively you can contact our Microchip customer support 
> team if you are interested to do this testing at Microchip.

I get that, might do as you suggest.

> > Also any input on my suggested testing methodology is more than welcome.
> > 
> >> We also implemented LAN867X Rev.C1 support already in the driver and
> >> published in our product site and in the process of preparing mainline
> >> patches. But unfortunately it took little more time to make it.
> >>
> >> https://ww1.microchip.com/downloads/aemDocuments/documents/AIS/ProductDocuments/CodeExamples/EVB-LAN8670-USB_Linux_Driver_1v0.zip
> > 
> > I'm aware, we've been using a derivative of that work at ferroamp for
> > development. But it's been driving me nuts, being the 't1s guy' at work,
> > and maintaining out of tree drivers for weird dev boxes.
> > 
> > It's not my intention to beat you to the punch, I just want a mainlined
> > driver so that I can spend less of my time on plumbing.
> I completely understand. Also it was not my intention too. Just to let 
> you know why it is delayed in reaching mainline and a quick reference 
> for the existing implementation. Enjoy!

I get that, in an ideal world FOSS would be the top prio for all
industry actors. I'm lucky enough to get some time reserved for it.

R

(no subject)

[Félix Piédallu]

Subject: Re: [PATCH 2/3] net: microchip_t1s: add support for LAN867x Rev.C1

Hi, 

> So there is a gap in the revisions. Maybe a B2 exists?

Actually, probably not. Some search gives this datasheet:

https://ww1.microchip.com/downloads/aemDocuments/documents/AIS/ProductDocuments/DataSheets/LAN8670-1-2-Data-Sheet-60001573.pdf

And page 2 (table 1) shows only revisions A0 (rev0), B1, (rev2), C1 (rev4).
Not sure about why only even revision numbers are released ?

Page 193 (table 10-1) also shows only B1 and C1. So you can be confident that only those exist.

@Ramón, thank you for your work on this driver!

Félix Piédallu

Re:

[Ramón Nordin Rodriguez]

> > So there is a gap in the revisions. Maybe a B2 exists?
> 
> Actually, probably not. Some search gives this datasheet:
> 
> https://ww1.microchip.com/downloads/aemDocuments/documents/AIS/ProductDocuments/DataSheets/LAN8670-1-2-Data-Sheet-60001573.pdf
> 
> And page 2 (table 1) shows only revisions A0 (rev0), B1, (rev2), C1 (rev4).
> Not sure about why only even revision numbers are released ?
> 
> Page 193 (table 10-1) also shows only B1 and C1. So you can be confident that only those exist.
> 

Thanks for clearing that up!

> @Ramón, thank you for your work on this driver!

Much appreciated
R

Re: [PATCH 0/3] net: microchip_t1s: additional phy support and collision detect handling

[Ramón Nordin Rodriguez]

On Mon, Nov 27, 2023 at 05:03:54PM +0100, Andrew Lunn wrote:
> > * 3-4 nodes (depending on how many usb ports and dongles I have)
> > * run iperf with long cables and CSMA/CD
> > * run iperf with long cables and CMSA/No CD
> > 
> > I'll report back the results. Anything you'd like to add/focus on with
> > evaluation?
> 
> Humm, thinking about how CSMA/CD works...
> 
> Maybe look at what counters the MAC provides. Does it have collisions
> and bad FCS? A collision should result in a bad FCS, if you are not
> using CD. So if things are working correctly, the count for CD should
> move to FCS if you turn CD off. If CD is falsely triggering, FCS as a
> % should not really change, but you probably get more frames over the
> link?
> 

# setup

Andrew suggested that I try to get statistics from the MAC, I did some
investigation here but could not figure it out.

Using iperf3
Client: Arm based system running lan865x macphy 
Server: PC running lan867x revB usb dongle


# test results

The results below should be considered fairly represenative but far from
perfect. There was some bounce up and down when rerunning, but these resutls
are an eye-ball average.

No meaningful difference was seen with short (2m) cables or long (12m).

## with collision detection enabled

iperf3 normal
- - - - - - - - - - - - - - - - - - - - - - - - -
[ ID] Interval           Transfer     Bitrate         Retr
[  5]   0.00-10.00  sec  5.54 MBytes  4.65 Mbits/sec    0             sender
[  5]   0.00-10.01  sec  5.40 MBytes  4.53 Mbits/sec                  receiver

iperf3 reverse
- - - - - - - - - - - - - - - - - - - - - - - - -
[ ID] Interval           Transfer     Bitrate         Retr
[  5]   0.00-10.00  sec   929 KBytes   761 Kbits/sec  293             sender
[  5]   0.00-10.00  sec   830 KBytes   680 Kbits/sec                  receiver


## with collision detection disabled

iperf3 normal
- - - - - - - - - - - - - - - - - - - - - - - - -
[ ID] Interval           Transfer     Bitrate         Retr
[  5]   0.00-10.00  sec  6.39 MBytes  5.36 Mbits/sec    0             sender
[  5]   0.00-10.04  sec  6.19 MBytes  5.17 Mbits/sec                  receiver

iperf3 reverse
- - - - - - - - - - - - - - - - - - - - - - - - -
[ ID] Interval           Transfer     Bitrate         Retr
[  5]   0.00-10.27  sec  1.10 MBytes   897 Kbits/sec  268             sender
[  5]   0.00-10.00  sec  1.01 MBytes   843 Kbits/sec                  receiver

# Conclusions

The arm system running the lan865x macphy uses a not yet mainlined driver, see
https://lore.kernel.org/all/20231023154649.45931-1-Parthiban.Veerasooran@microchip.com/

The lan865x driver crashed out every once in a while on reverse mode, there
is definetly something biased in the driver for tx over rx.
Then again it's not accepted yet.

Disabling collision detection seemes to have an positive effect.
Slightly higher speeds and slightly fewer retransmissions.

I don't have a black and white result to present, but things seems to work
slightly better with CD disabled, so I'm leaning towards just unconditionally 
disabling it for the lan865x and lan867x phys for the v2 patch.

I'll wait with submitting v2 for a day so anyone interested gets a
chance to weigh in on this.

R

Re: [PATCH 0/3] net: microchip_t1s: additional phy support and collision detect handling

[Andrew Lunn]

> ## with collision detection enabled
> 
> iperf3 normal
> - - - - - - - - - - - - - - - - - - - - - - - - -
> [ ID] Interval           Transfer     Bitrate         Retr
> [  5]   0.00-10.00  sec  5.54 MBytes  4.65 Mbits/sec    0             sender
> [  5]   0.00-10.01  sec  5.40 MBytes  4.53 Mbits/sec                  receiver
> 
> iperf3 reverse
> - - - - - - - - - - - - - - - - - - - - - - - - -
> [ ID] Interval           Transfer     Bitrate         Retr
> [  5]   0.00-10.00  sec   929 KBytes   761 Kbits/sec  293             sender
> [  5]   0.00-10.00  sec   830 KBytes   680 Kbits/sec                  receiver
> 
> 
> ## with collision detection disabled
> 
> iperf3 normal
> - - - - - - - - - - - - - - - - - - - - - - - - -
> [ ID] Interval           Transfer     Bitrate         Retr
> [  5]   0.00-10.00  sec  6.39 MBytes  5.36 Mbits/sec    0             sender
> [  5]   0.00-10.04  sec  6.19 MBytes  5.17 Mbits/sec                  receiver
> 
> iperf3 reverse
> - - - - - - - - - - - - - - - - - - - - - - - - -
> [ ID] Interval           Transfer     Bitrate         Retr
> [  5]   0.00-10.27  sec  1.10 MBytes   897 Kbits/sec  268             sender
> [  5]   0.00-10.00  sec  1.01 MBytes   843 Kbits/sec                  receiver
> 
> # Conclusions
> 
> The arm system running the lan865x macphy uses a not yet mainlined driver, see
> https://lore.kernel.org/all/20231023154649.45931-1-Parthiban.Veerasooran@microchip.com/
> 
> The lan865x driver crashed out every once in a while on reverse mode, there
> is definetly something biased in the driver for tx over rx.
>
> Then again it's not accepted yet.
> 
> Disabling collision detection seemes to have an positive effect.
> Slightly higher speeds and slightly fewer retransmissions.

I would want to first understand why there is such a big difference
with the direction. Is it TCP backing off because of the packet loss?
Or is there some other problem.

Maybe try with UDP streams, say with a bandwidth of 5Mbps. Do you
loose 4Mbps in one direction? Or a much smaller number of packets.

Are there any usable statistics? FCS errors?

    Andrew