[PATCH] net: dsa: mv88e6xxx: Single chip mode detection for MV88E6*41

[PATCH] net: dsa: mv88e6xxx: Single chip mode detection for MV88E6*41

[Nathan Rossi]

The mv88e6xxx driver expects switches that are configured in single chip
addressing mode to have the MDIO address configured as 0. This is due to
the switch ADDR pins representing the single chip addressing mode as 0.
However depending on the device (e.g. MV88E6*41) the switch does not
respond on address 0 or any other address below 16 (the first port
address) in single chip addressing mode. This allows for other devices
to be on the same shared MDIO bus despite the switch being in single
chip addressing mode.

When using a switch that works this way it is not possible to configure
switch driver as single chip addressing via device tree, along with
another MDIO device on the same bus with address 0, as both devices
would have the same address of 0 resulting in mdiobus_register_device
-EBUSY errors for one of the devices with address 0.

In order to support this configuration the switch node can have its MDIO
address configured as 16 (the first address that the device responds
to). During initialization the driver will treat this address similar to
how address 0 is, however because this address is also a valid
multi-chip address (in certain switch models, but not all) the driver
will configure the SMI in single chip addressing mode and attempt to
detect the switch model. If the device is configured in single chip
addressing mode this will succeed and the initialization process can
continue. If it fails to detect a valid model this is because the switch
model register is not a valid register when in multi-chip mode, it will
then fall back to the existing SMI initialization process using the MDIO
address as the multi-chip mode address.

This detection method is safe if the device is in either mode because
the single chip addressing mode read is a direct SMI/MDIO read operation
and has no side effects compared to the SMI writes required for the
multi-chip addressing mode.

In order to implement this change, the reset gpio configuration is moved
to occur before any SMI initialization. This ensures that the device has
the same/correct reset gpio state for both mv88e6xxx_smi_init calls.

Signed-off-by: Nathan Rossi <nathan@nathanrossi.com>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 45 +++++++++++++++++++++++++++++++++-------
 1 file changed, 38 insertions(+), 7 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 64f4fdd029..8cdfafb5d2 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -6276,6 +6276,32 @@ static int mv88e6xxx_detect(struct mv88e6xxx_chip *chip)
 	return 0;
 }
 
+static int mv88e6xxx_single_chip_detect(struct mv88e6xxx_chip *chip,
+					struct mdio_device *mdiodev)
+{
+	int err;
+
+	/* dual_chip takes precedence over single/multi-chip modes */
+	if (chip->info->dual_chip)
+		return -EINVAL;
+
+	/* If the mdio addr is 16 indicating the first port address of a switch
+	 * (e.g. mv88e6*41) in single chip addressing mode the device may be
+	 * configured in single chip addressing mode. Setup the smi access as
+	 * single chip addressing mode and attempt to detect the model of the
+	 * switch, if this fails the device is not configured in single chip
+	 * addressing mode.
+	 */
+	if (mdiodev->addr != 16)
+		return -EINVAL;
+
+	err = mv88e6xxx_smi_init(chip, mdiodev->bus, 0);
+	if (err)
+		return err;
+
+	return mv88e6xxx_detect(chip);
+}
+
 static struct mv88e6xxx_chip *mv88e6xxx_alloc_chip(struct device *dev)
 {
 	struct mv88e6xxx_chip *chip;
@@ -6959,10 +6985,6 @@ static int mv88e6xxx_probe(struct mdio_device *mdiodev)
 
 	chip->info = compat_info;
 
-	err = mv88e6xxx_smi_init(chip, mdiodev->bus, mdiodev->addr);
-	if (err)
-		goto out;
-
 	chip->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
 	if (IS_ERR(chip->reset)) {
 		err = PTR_ERR(chip->reset);
@@ -6971,9 +6993,18 @@ static int mv88e6xxx_probe(struct mdio_device *mdiodev)
 	if (chip->reset)
 		usleep_range(1000, 2000);
 
-	err = mv88e6xxx_detect(chip);
-	if (err)
-		goto out;
+	/* Detect if the device is configured in single chip addressing mode,
+	 * otherwise continue with address specific smi init/detection.
+	 */
+	if (mv88e6xxx_single_chip_detect(chip, mdiodev)) {
+		err = mv88e6xxx_smi_init(chip, mdiodev->bus, mdiodev->addr);
+		if (err)
+			goto out;
+
+		err = mv88e6xxx_detect(chip);
+		if (err)
+			goto out;
+	}
 
 	if (chip->info->edsa_support == MV88E6XXX_EDSA_SUPPORTED)
 		chip->tag_protocol = DSA_TAG_PROTO_EDSA;
---
2.35.2

Re: [PATCH] net: dsa: mv88e6xxx: Single chip mode detection for MV88E6*41

[Andrew Lunn]

On Sun, Apr 24, 2022 at 12:54:51PM +0000, Nathan Rossi wrote:
> The mv88e6xxx driver expects switches that are configured in single chip
> addressing mode to have the MDIO address configured as 0. This is due to
> the switch ADDR pins representing the single chip addressing mode as 0.
> However depending on the device (e.g. MV88E6*41) the switch does not
> respond on address 0 or any other address below 16 (the first port
> address) in single chip addressing mode. This allows for other devices
> to be on the same shared MDIO bus despite the switch being in single
> chip addressing mode.
> 
> When using a switch that works this way it is not possible to configure
> switch driver as single chip addressing via device tree, along with
> another MDIO device on the same bus with address 0, as both devices
> would have the same address of 0 resulting in mdiobus_register_device
> -EBUSY errors for one of the devices with address 0.
> 
> In order to support this configuration the switch node can have its MDIO
> address configured as 16 (the first address that the device responds
> to). During initialization the driver will treat this address similar to
> how address 0 is, however because this address is also a valid
> multi-chip address (in certain switch models, but not all) the driver
> will configure the SMI in single chip addressing mode and attempt to
> detect the switch model. If the device is configured in single chip
> addressing mode this will succeed and the initialization process can
> continue. If it fails to detect a valid model this is because the switch
> model register is not a valid register when in multi-chip mode, it will
> then fall back to the existing SMI initialization process using the MDIO
> address as the multi-chip mode address.
> 
> This detection method is safe if the device is in either mode because
> the single chip addressing mode read is a direct SMI/MDIO read operation
> and has no side effects compared to the SMI writes required for the
> multi-chip addressing mode.

Thanks for rewording the commit message. This makes it a lot clearer
what is going on and how it is fixed.

> @@ -6971,9 +6993,18 @@ static int mv88e6xxx_probe(struct mdio_device *mdiodev)
>  	if (chip->reset)
>  		usleep_range(1000, 2000);
>  
> -	err = mv88e6xxx_detect(chip);
> -	if (err)
> -		goto out;
> +	/* Detect if the device is configured in single chip addressing mode,
> +	 * otherwise continue with address specific smi init/detection.
> +	 */
> +	if (mv88e6xxx_single_chip_detect(chip, mdiodev)) {
> +		err = mv88e6xxx_smi_init(chip, mdiodev->bus, mdiodev->addr);
> +		if (err)
> +			goto out;
> +

This is confusing. Then name mv88e6xxx_single_chip_detect() suggests
it will return true if it detects a single chip device. When it fact
is return 0 == False if it does find such a device.

So i think this would be better coded as

	err = mv88e6xxx_single_chip_detect(chip, mdiodev);
	if (err) {
		err = mv88e6xxx_smi_init(chip, mdiodev->bus, mdiodev->addr);
		if (err)
			goto out;

I did however test this code on my 370rd, and it does work. So once we
get this sorted out, it is good to go.

	Andrew