[PATCH 0/4] ksz9477 dsa switch driver improvements

[PATCH 0/4] ksz9477 dsa switch driver improvements

[Paul Barker]

These changes were made while debugging the ksz9477 driver for use on a
custom board which uses the ksz9893 switch supported by this driver. The
patches have been runtime tested on top of Linux 5.8.4, I couldn't
runtime test them on top of 5.9-rc3 due to unrelated issues. They have
been build tested on top of 5.9-rc3.

Paul Barker (4):
  net: dsa: microchip: Make switch detection more informative
  net: dsa: microchip: Add debug print for XMII port mode
  net: dsa: microchip: Disable RGMII in-band status on KSZ9893
  net: dsa: microchip: Implement recommended reset timing

 drivers/net/dsa/microchip/ksz9477.c    | 11 +++++++++++
 drivers/net/dsa/microchip/ksz_common.c |  3 ++-
 2 files changed, 13 insertions(+), 1 deletion(-)

-- 
2.28.0

[PATCH 1/4] net: dsa: microchip: Make switch detection more informative

[Paul Barker]

To make switch detection more informative print the result of the
ksz9477/ksz9893 compatibility check. With debug output enabled also
print the contents of the Chip ID registers as a 40-bit hex string.

As this detection is the first communication with the switch performed
by the driver, making it easy to see any errors here will help identify
issues with SPI data corruption or reset sequencing.

Signed-off-by: Paul Barker <pbarker@konsulko.com>
---
 drivers/net/dsa/microchip/ksz9477.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
index 3cb22d149813..a48f5edab561 100644
--- a/drivers/net/dsa/microchip/ksz9477.c
+++ b/drivers/net/dsa/microchip/ksz9477.c
@@ -13,6 +13,7 @@
 #include <linux/if_bridge.h>
 #include <net/dsa.h>
 #include <net/switchdev.h>
+#include <linux/printk.h>
 
 #include "ksz9477_reg.h"
 #include "ksz_common.h"
@@ -1426,10 +1427,12 @@ static int ksz9477_switch_detect(struct ksz_device *dev)
 	/* Default capability is gigabit capable. */
 	dev->features = GBIT_SUPPORT;
 
+	dev_dbg(dev->dev, "Switch detect: ID=%08x%02x\n", id32, data8);
 	id_hi = (u8)(id32 >> 16);
 	id_lo = (u8)(id32 >> 8);
 	if ((id_lo & 0xf) == 3) {
 		/* Chip is from KSZ9893 design. */
+		dev_info(dev->dev, "Found KSZ9893\n");
 		dev->features |= IS_9893;
 
 		/* Chip does not support gigabit. */
@@ -1438,6 +1441,7 @@ static int ksz9477_switch_detect(struct ksz_device *dev)
 		dev->mib_port_cnt = 3;
 		dev->phy_port_cnt = 2;
 	} else {
+		dev_info(dev->dev, "Found KSZ9477 or compatible\n");
 		/* Chip uses new XMII register definitions. */
 		dev->features |= NEW_XMII;
 
-- 
2.28.0

[PATCH 2/4] net: dsa: microchip: Add debug print for XMII port mode

[Paul Barker]

When debug is enabled for this driver, this allows users to confirm that
the correct port mode is in use.

Signed-off-by: Paul Barker <pbarker@konsulko.com>
---
 drivers/net/dsa/microchip/ksz9477.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
index a48f5edab561..3e36aa628c9f 100644
--- a/drivers/net/dsa/microchip/ksz9477.c
+++ b/drivers/net/dsa/microchip/ksz9477.c
@@ -1211,21 +1211,25 @@ static void ksz9477_port_setup(struct ksz_device *dev, int port, bool cpu_port)
 		ksz_pread8(dev, port, REG_PORT_XMII_CTRL_1, &data8);
 		switch (dev->interface) {
 		case PHY_INTERFACE_MODE_MII:
+			dev_dbg(dev->dev, "Port%d: MII mode\n", port);
 			ksz9477_set_xmii(dev, 0, &data8);
 			ksz9477_set_gbit(dev, false, &data8);
 			p->phydev.speed = SPEED_100;
 			break;
 		case PHY_INTERFACE_MODE_RMII:
+			dev_dbg(dev->dev, "Port%d: RMII mode\n", port);
 			ksz9477_set_xmii(dev, 1, &data8);
 			ksz9477_set_gbit(dev, false, &data8);
 			p->phydev.speed = SPEED_100;
 			break;
 		case PHY_INTERFACE_MODE_GMII:
+			dev_dbg(dev->dev, "Port%d: GMII mode\n", port);
 			ksz9477_set_xmii(dev, 2, &data8);
 			ksz9477_set_gbit(dev, true, &data8);
 			p->phydev.speed = SPEED_1000;
 			break;
 		default:
+			dev_dbg(dev->dev, "Port%d: RGMII mode\n", port);
 			ksz9477_set_xmii(dev, 3, &data8);
 			ksz9477_set_gbit(dev, true, &data8);
 			data8 &= ~PORT_RGMII_ID_IG_ENABLE;
-- 
2.28.0

[PATCH 3/4] net: dsa: microchip: Disable RGMII in-band status on KSZ9893

[Paul Barker]

We can't assume that the link partner supports the in-band status
reporting which is enabled by default on the KSZ9893 when using RGMII
for the upstream port.

Signed-off-by: Paul Barker <pbarker@konsulko.com>
---
 drivers/net/dsa/microchip/ksz9477.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
index 3e36aa628c9f..2c953ab6ce16 100644
--- a/drivers/net/dsa/microchip/ksz9477.c
+++ b/drivers/net/dsa/microchip/ksz9477.c
@@ -1240,6 +1240,9 @@ static void ksz9477_port_setup(struct ksz_device *dev, int port, bool cpu_port)
 			if (dev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
 			    dev->interface == PHY_INTERFACE_MODE_RGMII_TXID)
 				data8 |= PORT_RGMII_ID_EG_ENABLE;
+			/* On KSZ9893, disable RGMII in-band status support */
+			if (dev->features & IS_9893)
+				data8 &= ~PORT_MII_MAC_MODE;
 			p->phydev.speed = SPEED_1000;
 			break;
 		}
-- 
2.28.0

[PATCH 4/4] net: dsa: microchip: Implement recommended reset timing

[Paul Barker]

The datasheet for the ksz9893 and ksz9477 switches recommend waiting at
least 100us after the de-assertion of reset before trying to program the
device through any interface.

Also switch the existing mdelay() call to usleep_range() as recommended
in Documentation/timers/timers-howto.rst. The 2ms range used here is
somewhat arbitrary, as long as the reset is asserted for at least 10ms
we should be ok.

Signed-off-by: Paul Barker <pbarker@konsulko.com>
---
 drivers/net/dsa/microchip/ksz_common.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index 8d53b12d40a8..a31738662d95 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -400,8 +400,9 @@ int ksz_switch_register(struct ksz_device *dev,
 
 	if (dev->reset_gpio) {
 		gpiod_set_value_cansleep(dev->reset_gpio, 1);
-		mdelay(10);
+		usleep_range(10000, 12000);
 		gpiod_set_value_cansleep(dev->reset_gpio, 0);
+		usleep_range(100, 1000);
 	}
 
 	mutex_init(&dev->dev_mutex);
-- 
2.28.0

Re: [PATCH 1/4] net: dsa: microchip: Make switch detection more informative

[Andrew Lunn]

On Sat, Sep 05, 2020 at 03:03:22PM +0100, Paul Barker wrote:
> To make switch detection more informative print the result of the
> ksz9477/ksz9893 compatibility check. With debug output enabled also
> print the contents of the Chip ID registers as a 40-bit hex string.
> 
> As this detection is the first communication with the switch performed
> by the driver, making it easy to see any errors here will help identify
> issues with SPI data corruption or reset sequencing.
> 
> Signed-off-by: Paul Barker <pbarker@konsulko.com>
> ---
>  drivers/net/dsa/microchip/ksz9477.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
> index 3cb22d149813..a48f5edab561 100644
> --- a/drivers/net/dsa/microchip/ksz9477.c
> +++ b/drivers/net/dsa/microchip/ksz9477.c
> @@ -13,6 +13,7 @@
>  #include <linux/if_bridge.h>
>  #include <net/dsa.h>
>  #include <net/switchdev.h>
> +#include <linux/printk.h>

It is not often you see that include. linux/kernel.h includes it
anyway, and given how few files include printk.h, i doubt it will ever
be removed from kernel.h

   Andrew

Re: [PATCH 2/4] net: dsa: microchip: Add debug print for XMII port mode

[Andrew Lunn]

On Sat, Sep 05, 2020 at 03:03:23PM +0100, Paul Barker wrote:
> When debug is enabled for this driver, this allows users to confirm that
> the correct port mode is in use.
> 
> Signed-off-by: Paul Barker <pbarker@konsulko.com>
> ---
>  drivers/net/dsa/microchip/ksz9477.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
> index a48f5edab561..3e36aa628c9f 100644
> --- a/drivers/net/dsa/microchip/ksz9477.c
> +++ b/drivers/net/dsa/microchip/ksz9477.c
> @@ -1211,21 +1211,25 @@ static void ksz9477_port_setup(struct ksz_device *dev, int port, bool cpu_port)
>  		ksz_pread8(dev, port, REG_PORT_XMII_CTRL_1, &data8);
>  		switch (dev->interface) {
>  		case PHY_INTERFACE_MODE_MII:
> +			dev_dbg(dev->dev, "Port%d: MII mode\n", port);
>  			ksz9477_set_xmii(dev, 0, &data8);
>  			ksz9477_set_gbit(dev, false, &data8);
>  			p->phydev.speed = SPEED_100;
>  			break;
>  		case PHY_INTERFACE_MODE_RMII:
> +			dev_dbg(dev->dev, "Port%d: RMII mode\n", port);
>  			ksz9477_set_xmii(dev, 1, &data8);
>  			ksz9477_set_gbit(dev, false, &data8);
>  			p->phydev.speed = SPEED_100;
>  			break;
>  		case PHY_INTERFACE_MODE_GMII:
> +			dev_dbg(dev->dev, "Port%d: GMII mode\n", port);
>  			ksz9477_set_xmii(dev, 2, &data8);
>  			ksz9477_set_gbit(dev, true, &data8);
>  			p->phydev.speed = SPEED_1000;
>  			break;
>  		default:
> +			dev_dbg(dev->dev, "Port%d: RGMII mode\n", port);
>  			ksz9477_set_xmii(dev, 3, &data8);
>  			ksz9477_set_gbit(dev, true, &data8);
>  			data8 &= ~PORT_RGMII_ID_IG_ENABLE;

You could do

dev_dbg(dev->dev, "Port%d: %s\n", port, phy_mode(dev->interface));

just before the switch statement.    

     Andrew

Re: [PATCH 3/4] net: dsa: microchip: Disable RGMII in-band status on KSZ9893

[Andrew Lunn]

On Sat, Sep 05, 2020 at 03:03:24PM +0100, Paul Barker wrote:
> We can't assume that the link partner supports the in-band status
> reporting which is enabled by default on the KSZ9893 when using RGMII
> for the upstream port.

What do you mean by RGMII inband status reporting? SGMII/1000BaseX has
in band signalling, but RGMII?

   Andrew

Re: [PATCH 0/4] ksz9477 dsa switch driver improvements

[Andrew Lunn]

On Sat, Sep 05, 2020 at 03:03:21PM +0100, Paul Barker wrote:
> These changes were made while debugging the ksz9477 driver for use on a
> custom board which uses the ksz9893 switch supported by this driver. The
> patches have been runtime tested on top of Linux 5.8.4, I couldn't
> runtime test them on top of 5.9-rc3 due to unrelated issues. They have
> been build tested on top of 5.9-rc3.

Hi Paul

Please rebase onto net-next. Take a look at:

https://www.kernel.org/doc/html/latest/networking/netdev-FAQ.html

	Andrew

Re: [PATCH 3/4] net: dsa: microchip: Disable RGMII in-band status on KSZ9893

[Paul Barker]

On Sat, 5 Sep 2020 at 16:32, Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Sat, Sep 05, 2020 at 03:03:24PM +0100, Paul Barker wrote:
> > We can't assume that the link partner supports the in-band status
> > reporting which is enabled by default on the KSZ9893 when using RGMII
> > for the upstream port.
>
> What do you mean by RGMII inband status reporting? SGMII/1000BaseX has
> in band signalling, but RGMII?
>
>    Andrew

I'm referencing page 56 of the KSZ9893 datasheet
(http://ww1.microchip.com/downloads/en/DeviceDoc/KSZ9893R-Data-Sheet-DS00002420D.pdf).
The datasheet says "The RGMII port will not function properly if IBS
is enabled in the switch, but it is not receiving in-band status from
a connected PHY." Since we can't guarantee all possible link partners
will support this it should be disabled. In particular, the IMX6 SoC
we're using with this switch doesn't support this on its Ethernet
port.

I don't really know much about how this is implemented or how widely
it's supported.

Thanks,

-- 
Paul Barker
Konsulko Group

Re: [PATCH 0/4] ksz9477 dsa switch driver improvements

[Paul Barker]

On Sat, 5 Sep 2020 at 16:34, Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Sat, Sep 05, 2020 at 03:03:21PM +0100, Paul Barker wrote:
> > These changes were made while debugging the ksz9477 driver for use on a
> > custom board which uses the ksz9893 switch supported by this driver. The
> > patches have been runtime tested on top of Linux 5.8.4, I couldn't
> > runtime test them on top of 5.9-rc3 due to unrelated issues. They have
> > been build tested on top of 5.9-rc3.
>
> Hi Paul
>
> Please rebase onto net-next. Take a look at:
>
> https://www.kernel.org/doc/html/latest/networking/netdev-FAQ.html

Will do. I'll send a rebased v2 which also addresses your other comments.

Thanks,

-- 
Paul Barker
Konsulko Group

Re: [PATCH 3/4] net: dsa: microchip: Disable RGMII in-band status on KSZ9893

[Florian Fainelli]

On 9/5/2020 8:53 AM, Paul Barker wrote:
> On Sat, 5 Sep 2020 at 16:32, Andrew Lunn <andrew@lunn.ch> wrote:
>>
>> On Sat, Sep 05, 2020 at 03:03:24PM +0100, Paul Barker wrote:
>>> We can't assume that the link partner supports the in-band status
>>> reporting which is enabled by default on the KSZ9893 when using RGMII
>>> for the upstream port.
>>
>> What do you mean by RGMII inband status reporting? SGMII/1000BaseX has
>> in band signalling, but RGMII?
>>
>>     Andrew
> 
> I'm referencing page 56 of the KSZ9893 datasheet
> (http://ww1.microchip.com/downloads/en/DeviceDoc/KSZ9893R-Data-Sheet-DS00002420D.pdf).
> The datasheet says "The RGMII port will not function properly if IBS
> is enabled in the switch, but it is not receiving in-band status from
> a connected PHY." Since we can't guarantee all possible link partners
> will support this it should be disabled. In particular, the IMX6 SoC
> we're using with this switch doesn't support this on its Ethernet
> port.

The RGMII 2.0 specification, pages 7 and 8 has more details:

http://web.archive.org/web/20160303171328/http://www.hp.com/rnd/pdfs/RGMIIv2_0_final_hp.pdf

and section 3.4.1 indicates that this is optional anyway for link 
status/speed/duplex.

It comes down to putting a appropriate data word on RXD[7:0] to signal 
link status, speed and speed, if the link partner does not provide the 
inter-frame word, then the receiver cannot reconstruct that information, 
or it will incorrectly decode it.

> 
> I don't really know much about how this is implemented or how widely
> it's supported.

It is supported by the Broadcom GENET adapter and probably a few others, 
however for the same reasons, I have not seen it being widely used. You 
would save two reads of BMSR to determine the link status which is nice, 
but link parameter changes are disruptive anyway.
-- 
Florian

Re: [PATCH 3/4] net: dsa: microchip: Disable RGMII in-band status on KSZ9893

[Andrew Lunn]

On Sat, Sep 05, 2020 at 04:53:20PM +0100, Paul Barker wrote:
> On Sat, 5 Sep 2020 at 16:32, Andrew Lunn <andrew@lunn.ch> wrote:
> >
> > On Sat, Sep 05, 2020 at 03:03:24PM +0100, Paul Barker wrote:
> > > We can't assume that the link partner supports the in-band status
> > > reporting which is enabled by default on the KSZ9893 when using RGMII
> > > for the upstream port.
> >
> > What do you mean by RGMII inband status reporting? SGMII/1000BaseX has
> > in band signalling, but RGMII?
> >
> >    Andrew
> 
> I'm referencing page 56 of the KSZ9893 datasheet
> (http://ww1.microchip.com/downloads/en/DeviceDoc/KSZ9893R-Data-Sheet-DS00002420D.pdf).
> The datasheet says "The RGMII port will not function properly if IBS
> is enabled in the switch, but it is not receiving in-band status from
> a connected PHY." Since we can't guarantee all possible link partners
> will support this it should be disabled. In particular, the IMX6 SoC
> we're using with this switch doesn't support this on its Ethernet
> port.
> 
> I don't really know much about how this is implemented or how widely
> it's supported.

I never knew RGMII had this. What i did find was:

http://web.archive.org/web/20160303171328/http://www.hp.com/rnd/pdfs/RGMIIv2_0_final_hp.pdf

which does talk about it, section 3.4.1. It is clearly optional, and
since this is the first time i've heard of it, i suspect not many
systems actually implement it. So turning it off seems wise.

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

    Andrew