FAILED: patch "[PATCH] net: phy: marvell: fix detection of PHY on Topaz switches" failed to apply to 5.4-stable tree

FAILED: patch "[PATCH] net: phy: marvell: fix detection of PHY on Topaz switches" failed to apply to 5.4-stable tree

[gregkh]

The patch below does not apply to the 5.4-stable tree.
If someone wants it applied there, or to any other stable or longterm
tree, then please email the backport, including the original git commit
id to <stable@vger.kernel.org>.

thanks,

greg k-h

------------------ original commit in Linus's tree ------------------

From 1fe976d308acb6374c899a4ee8025a0a016e453e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Pali=20Roh=C3=A1r?= <pali@kernel.org>
Date: Mon, 12 Apr 2021 18:57:39 +0200
Subject: [PATCH] net: phy: marvell: fix detection of PHY on Topaz switches
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Since commit fee2d546414d ("net: phy: marvell: mv88e6390 temperature
sensor reading"), Linux reports the temperature of Topaz hwmon as
constant -75°C.

This is because switches from the Topaz family (88E6141 / 88E6341) have
the address of the temperature sensor register different from Peridot.

This address is instead compatible with 88E1510 PHYs, as was used for
Topaz before the above mentioned commit.

Create a new mapping table between switch family and PHY ID for families
which don't have a model number. And define PHY IDs for Topaz and Peridot
families.

Create a new PHY ID and a new PHY driver for Topaz's internal PHY.
The only difference from Peridot's PHY driver is the HWMON probing
method.

Prior this change Topaz's internal PHY is detected by kernel as:

  PHY [...] driver [Marvell 88E6390] (irq=63)

And afterwards as:

  PHY [...] driver [Marvell 88E6341 Family] (irq=63)

Signed-off-by: Pali Rohár <pali@kernel.org>
BugLink: https://github.com/globalscaletechnologies/linux/issues/1
Fixes: fee2d546414d ("net: phy: marvell: mv88e6390 temperature sensor reading")
Reviewed-by: Marek Behún <kabel@kernel.org>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: David S. Miller <davem@davemloft.net>

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 903d619e08ed..e08bf9377140 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -3026,10 +3026,17 @@ static int mv88e6xxx_setup(struct dsa_switch *ds)
 	return err;
 }
 
+/* prod_id for switch families which do not have a PHY model number */
+static const u16 family_prod_id_table[] = {
+	[MV88E6XXX_FAMILY_6341] = MV88E6XXX_PORT_SWITCH_ID_PROD_6341,
+	[MV88E6XXX_FAMILY_6390] = MV88E6XXX_PORT_SWITCH_ID_PROD_6390,
+};
+
 static int mv88e6xxx_mdio_read(struct mii_bus *bus, int phy, int reg)
 {
 	struct mv88e6xxx_mdio_bus *mdio_bus = bus->priv;
 	struct mv88e6xxx_chip *chip = mdio_bus->chip;
+	u16 prod_id;
 	u16 val;
 	int err;
 
@@ -3040,23 +3047,12 @@ static int mv88e6xxx_mdio_read(struct mii_bus *bus, int phy, int reg)
 	err = chip->info->ops->phy_read(chip, bus, phy, reg, &val);
 	mv88e6xxx_reg_unlock(chip);
 
-	if (reg == MII_PHYSID2) {
-		/* Some internal PHYs don't have a model number. */
-		if (chip->info->family != MV88E6XXX_FAMILY_6165)
-			/* Then there is the 6165 family. It gets is
-			 * PHYs correct. But it can also have two
-			 * SERDES interfaces in the PHY address
-			 * space. And these don't have a model
-			 * number. But they are not PHYs, so we don't
-			 * want to give them something a PHY driver
-			 * will recognise.
-			 *
-			 * Use the mv88e6390 family model number
-			 * instead, for anything which really could be
-			 * a PHY,
-			 */
-			if (!(val & 0x3f0))
-				val |= MV88E6XXX_PORT_SWITCH_ID_PROD_6390 >> 4;
+	/* Some internal PHYs don't have a model number. */
+	if (reg == MII_PHYSID2 && !(val & 0x3f0) &&
+	    chip->info->family < ARRAY_SIZE(family_prod_id_table)) {
+		prod_id = family_prod_id_table[chip->info->family];
+		if (prod_id)
+			val |= prod_id >> 4;
 	}
 
 	return err ? err : val;
diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index e26a5d663f8a..8018ddf7f316 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -3021,9 +3021,34 @@ static struct phy_driver marvell_drivers[] = {
 		.get_stats = marvell_get_stats,
 	},
 	{
-		.phy_id = MARVELL_PHY_ID_88E6390,
+		.phy_id = MARVELL_PHY_ID_88E6341_FAMILY,
 		.phy_id_mask = MARVELL_PHY_ID_MASK,
-		.name = "Marvell 88E6390",
+		.name = "Marvell 88E6341 Family",
+		/* PHY_GBIT_FEATURES */
+		.flags = PHY_POLL_CABLE_TEST,
+		.probe = m88e1510_probe,
+		.config_init = marvell_config_init,
+		.config_aneg = m88e6390_config_aneg,
+		.read_status = marvell_read_status,
+		.config_intr = marvell_config_intr,
+		.handle_interrupt = marvell_handle_interrupt,
+		.resume = genphy_resume,
+		.suspend = genphy_suspend,
+		.read_page = marvell_read_page,
+		.write_page = marvell_write_page,
+		.get_sset_count = marvell_get_sset_count,
+		.get_strings = marvell_get_strings,
+		.get_stats = marvell_get_stats,
+		.get_tunable = m88e1540_get_tunable,
+		.set_tunable = m88e1540_set_tunable,
+		.cable_test_start = marvell_vct7_cable_test_start,
+		.cable_test_tdr_start = marvell_vct5_cable_test_tdr_start,
+		.cable_test_get_status = marvell_vct7_cable_test_get_status,
+	},
+	{
+		.phy_id = MARVELL_PHY_ID_88E6390_FAMILY,
+		.phy_id_mask = MARVELL_PHY_ID_MASK,
+		.name = "Marvell 88E6390 Family",
 		/* PHY_GBIT_FEATURES */
 		.flags = PHY_POLL_CABLE_TEST,
 		.probe = m88e6390_probe,
@@ -3107,7 +3132,8 @@ static struct mdio_device_id __maybe_unused marvell_tbl[] = {
 	{ MARVELL_PHY_ID_88E1540, MARVELL_PHY_ID_MASK },
 	{ MARVELL_PHY_ID_88E1545, MARVELL_PHY_ID_MASK },
 	{ MARVELL_PHY_ID_88E3016, MARVELL_PHY_ID_MASK },
-	{ MARVELL_PHY_ID_88E6390, MARVELL_PHY_ID_MASK },
+	{ MARVELL_PHY_ID_88E6341_FAMILY, MARVELL_PHY_ID_MASK },
+	{ MARVELL_PHY_ID_88E6390_FAMILY, MARVELL_PHY_ID_MASK },
 	{ MARVELL_PHY_ID_88E1340S, MARVELL_PHY_ID_MASK },
 	{ MARVELL_PHY_ID_88E1548P, MARVELL_PHY_ID_MASK },
 	{ }
diff --git a/include/linux/marvell_phy.h b/include/linux/marvell_phy.h
index 52b1610eae68..c544b70dfbd2 100644
--- a/include/linux/marvell_phy.h
+++ b/include/linux/marvell_phy.h
@@ -28,11 +28,12 @@
 /* Marvel 88E1111 in Finisar SFP module with modified PHY ID */
 #define MARVELL_PHY_ID_88E1111_FINISAR	0x01ff0cc0
 
-/* The MV88e6390 Ethernet switch contains embedded PHYs. These PHYs do
+/* These Ethernet switch families contain embedded PHYs, but they do
  * not have a model ID. So the switch driver traps reads to the ID2
  * register and returns the switch family ID
  */
-#define MARVELL_PHY_ID_88E6390		0x01410f90
+#define MARVELL_PHY_ID_88E6341_FAMILY	0x01410f41
+#define MARVELL_PHY_ID_88E6390_FAMILY	0x01410f90
 
 #define MARVELL_PHY_FAMILY_ID(id)	((id) >> 4)
 

FAILED: patch "[PATCH] net: phy: marvell: fix detection of PHY on Topaz switches" failed to apply to 5.4-stable tree

[gregkh]

The patch below does not apply to the 5.4-stable tree.
If someone wants it applied there, or to any other stable or longterm
tree, then please email the backport, including the original git commit
id to <stable@vger.kernel.org>.

thanks,

greg k-h

------------------ original commit in Linus's tree ------------------

From 1fe976d308acb6374c899a4ee8025a0a016e453e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Pali=20Roh=C3=A1r?= <pali@kernel.org>
Date: Mon, 12 Apr 2021 18:57:39 +0200
Subject: [PATCH] net: phy: marvell: fix detection of PHY on Topaz switches
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Since commit fee2d546414d ("net: phy: marvell: mv88e6390 temperature
sensor reading"), Linux reports the temperature of Topaz hwmon as
constant -75°C.

This is because switches from the Topaz family (88E6141 / 88E6341) have
the address of the temperature sensor register different from Peridot.

This address is instead compatible with 88E1510 PHYs, as was used for
Topaz before the above mentioned commit.

Create a new mapping table between switch family and PHY ID for families
which don't have a model number. And define PHY IDs for Topaz and Peridot
families.

Create a new PHY ID and a new PHY driver for Topaz's internal PHY.
The only difference from Peridot's PHY driver is the HWMON probing
method.

Prior this change Topaz's internal PHY is detected by kernel as:

  PHY [...] driver [Marvell 88E6390] (irq=63)

And afterwards as:

  PHY [...] driver [Marvell 88E6341 Family] (irq=63)

Signed-off-by: Pali Rohár <pali@kernel.org>
BugLink: https://github.com/globalscaletechnologies/linux/issues/1
Fixes: fee2d546414d ("net: phy: marvell: mv88e6390 temperature sensor reading")
Reviewed-by: Marek Behún <kabel@kernel.org>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: David S. Miller <davem@davemloft.net>

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 903d619e08ed..e08bf9377140 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -3026,10 +3026,17 @@ static int mv88e6xxx_setup(struct dsa_switch *ds)
 	return err;
 }
 
+/* prod_id for switch families which do not have a PHY model number */
+static const u16 family_prod_id_table[] = {
+	[MV88E6XXX_FAMILY_6341] = MV88E6XXX_PORT_SWITCH_ID_PROD_6341,
+	[MV88E6XXX_FAMILY_6390] = MV88E6XXX_PORT_SWITCH_ID_PROD_6390,
+};
+
 static int mv88e6xxx_mdio_read(struct mii_bus *bus, int phy, int reg)
 {
 	struct mv88e6xxx_mdio_bus *mdio_bus = bus->priv;
 	struct mv88e6xxx_chip *chip = mdio_bus->chip;
+	u16 prod_id;
 	u16 val;
 	int err;
 
@@ -3040,23 +3047,12 @@ static int mv88e6xxx_mdio_read(struct mii_bus *bus, int phy, int reg)
 	err = chip->info->ops->phy_read(chip, bus, phy, reg, &val);
 	mv88e6xxx_reg_unlock(chip);
 
-	if (reg == MII_PHYSID2) {
-		/* Some internal PHYs don't have a model number. */
-		if (chip->info->family != MV88E6XXX_FAMILY_6165)
-			/* Then there is the 6165 family. It gets is
-			 * PHYs correct. But it can also have two
-			 * SERDES interfaces in the PHY address
-			 * space. And these don't have a model
-			 * number. But they are not PHYs, so we don't
-			 * want to give them something a PHY driver
-			 * will recognise.
-			 *
-			 * Use the mv88e6390 family model number
-			 * instead, for anything which really could be
-			 * a PHY,
-			 */
-			if (!(val & 0x3f0))
-				val |= MV88E6XXX_PORT_SWITCH_ID_PROD_6390 >> 4;
+	/* Some internal PHYs don't have a model number. */
+	if (reg == MII_PHYSID2 && !(val & 0x3f0) &&
+	    chip->info->family < ARRAY_SIZE(family_prod_id_table)) {
+		prod_id = family_prod_id_table[chip->info->family];
+		if (prod_id)
+			val |= prod_id >> 4;
 	}
 
 	return err ? err : val;
diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index e26a5d663f8a..8018ddf7f316 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -3021,9 +3021,34 @@ static struct phy_driver marvell_drivers[] = {
 		.get_stats = marvell_get_stats,
 	},
 	{
-		.phy_id = MARVELL_PHY_ID_88E6390,
+		.phy_id = MARVELL_PHY_ID_88E6341_FAMILY,
 		.phy_id_mask = MARVELL_PHY_ID_MASK,
-		.name = "Marvell 88E6390",
+		.name = "Marvell 88E6341 Family",
+		/* PHY_GBIT_FEATURES */
+		.flags = PHY_POLL_CABLE_TEST,
+		.probe = m88e1510_probe,
+		.config_init = marvell_config_init,
+		.config_aneg = m88e6390_config_aneg,
+		.read_status = marvell_read_status,
+		.config_intr = marvell_config_intr,
+		.handle_interrupt = marvell_handle_interrupt,
+		.resume = genphy_resume,
+		.suspend = genphy_suspend,
+		.read_page = marvell_read_page,
+		.write_page = marvell_write_page,
+		.get_sset_count = marvell_get_sset_count,
+		.get_strings = marvell_get_strings,
+		.get_stats = marvell_get_stats,
+		.get_tunable = m88e1540_get_tunable,
+		.set_tunable = m88e1540_set_tunable,
+		.cable_test_start = marvell_vct7_cable_test_start,
+		.cable_test_tdr_start = marvell_vct5_cable_test_tdr_start,
+		.cable_test_get_status = marvell_vct7_cable_test_get_status,
+	},
+	{
+		.phy_id = MARVELL_PHY_ID_88E6390_FAMILY,
+		.phy_id_mask = MARVELL_PHY_ID_MASK,
+		.name = "Marvell 88E6390 Family",
 		/* PHY_GBIT_FEATURES */
 		.flags = PHY_POLL_CABLE_TEST,
 		.probe = m88e6390_probe,
@@ -3107,7 +3132,8 @@ static struct mdio_device_id __maybe_unused marvell_tbl[] = {
 	{ MARVELL_PHY_ID_88E1540, MARVELL_PHY_ID_MASK },
 	{ MARVELL_PHY_ID_88E1545, MARVELL_PHY_ID_MASK },
 	{ MARVELL_PHY_ID_88E3016, MARVELL_PHY_ID_MASK },
-	{ MARVELL_PHY_ID_88E6390, MARVELL_PHY_ID_MASK },
+	{ MARVELL_PHY_ID_88E6341_FAMILY, MARVELL_PHY_ID_MASK },
+	{ MARVELL_PHY_ID_88E6390_FAMILY, MARVELL_PHY_ID_MASK },
 	{ MARVELL_PHY_ID_88E1340S, MARVELL_PHY_ID_MASK },
 	{ MARVELL_PHY_ID_88E1548P, MARVELL_PHY_ID_MASK },
 	{ }
diff --git a/include/linux/marvell_phy.h b/include/linux/marvell_phy.h
index 52b1610eae68..c544b70dfbd2 100644
--- a/include/linux/marvell_phy.h
+++ b/include/linux/marvell_phy.h
@@ -28,11 +28,12 @@
 /* Marvel 88E1111 in Finisar SFP module with modified PHY ID */
 #define MARVELL_PHY_ID_88E1111_FINISAR	0x01ff0cc0
 
-/* The MV88e6390 Ethernet switch contains embedded PHYs. These PHYs do
+/* These Ethernet switch families contain embedded PHYs, but they do
  * not have a model ID. So the switch driver traps reads to the ID2
  * register and returns the switch family ID
  */
-#define MARVELL_PHY_ID_88E6390		0x01410f90
+#define MARVELL_PHY_ID_88E6341_FAMILY	0x01410f41
+#define MARVELL_PHY_ID_88E6390_FAMILY	0x01410f90
 
 #define MARVELL_PHY_FAMILY_ID(id)	((id) >> 4)
 

[PATCH] net: phy: marvell: fix detection of PHY on Topaz switches

[Pali Rohár]

commit 1fe976d308acb6374c899a4ee8025a0a016e453e upstream.

Since commit fee2d546414d ("net: phy: marvell: mv88e6390 temperature
sensor reading"), Linux reports the temperature of Topaz hwmon as
constant -75°C.

This is because switches from the Topaz family (88E6141 / 88E6341) have
the address of the temperature sensor register different from Peridot.

This address is instead compatible with 88E1510 PHYs, as was used for
Topaz before the above mentioned commit.

Create a new mapping table between switch family and PHY ID for families
which don't have a model number. And define PHY IDs for Topaz and Peridot
families.

Create a new PHY ID and a new PHY driver for Topaz's internal PHY.
The only difference from Peridot's PHY driver is the HWMON probing
method.

Prior this change Topaz's internal PHY is detected by kernel as:

  PHY [...] driver [Marvell 88E6390] (irq=63)

And afterwards as:

  PHY [...] driver [Marvell 88E6341 Family] (irq=63)

Signed-off-by: Pali Rohár <pali@kernel.org>
BugLink: https://github.com/globalscaletechnologies/linux/issues/1
Fixes: fee2d546414d ("net: phy: marvell: mv88e6390 temperature sensor reading")
Reviewed-by: Marek Behún <kabel@kernel.org>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: David S. Miller <davem@davemloft.net>
[pali: Backported to 5.4 version]
---
This patch is backported to 5.4 version. Tested on Turris Mox with Topaz switch.
---
 drivers/net/dsa/mv88e6xxx/chip.c | 30 +++++++++++++-----------------
 drivers/net/phy/marvell.c        | 29 ++++++++++++++++++++++++++---
 include/linux/marvell_phy.h      |  5 +++--
 3 files changed, 42 insertions(+), 22 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 1af09fd3fed1..446eb06e50b4 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -2766,10 +2766,17 @@ static int mv88e6xxx_setup(struct dsa_switch *ds)
 	return err;
 }
 
+/* prod_id for switch families which do not have a PHY model number */
+static const u16 family_prod_id_table[] = {
+	[MV88E6XXX_FAMILY_6341] = MV88E6XXX_PORT_SWITCH_ID_PROD_6341,
+	[MV88E6XXX_FAMILY_6390] = MV88E6XXX_PORT_SWITCH_ID_PROD_6390,
+};
+
 static int mv88e6xxx_mdio_read(struct mii_bus *bus, int phy, int reg)
 {
 	struct mv88e6xxx_mdio_bus *mdio_bus = bus->priv;
 	struct mv88e6xxx_chip *chip = mdio_bus->chip;
+	u16 prod_id;
 	u16 val;
 	int err;
 
@@ -2780,23 +2787,12 @@ static int mv88e6xxx_mdio_read(struct mii_bus *bus, int phy, int reg)
 	err = chip->info->ops->phy_read(chip, bus, phy, reg, &val);
 	mv88e6xxx_reg_unlock(chip);
 
-	if (reg == MII_PHYSID2) {
-		/* Some internal PHYs don't have a model number. */
-		if (chip->info->family != MV88E6XXX_FAMILY_6165)
-			/* Then there is the 6165 family. It gets is
-			 * PHYs correct. But it can also have two
-			 * SERDES interfaces in the PHY address
-			 * space. And these don't have a model
-			 * number. But they are not PHYs, so we don't
-			 * want to give them something a PHY driver
-			 * will recognise.
-			 *
-			 * Use the mv88e6390 family model number
-			 * instead, for anything which really could be
-			 * a PHY,
-			 */
-			if (!(val & 0x3f0))
-				val |= MV88E6XXX_PORT_SWITCH_ID_PROD_6390 >> 4;
+	/* Some internal PHYs don't have a model number. */
+	if (reg == MII_PHYSID2 && !(val & 0x3f0) &&
+	    chip->info->family < ARRAY_SIZE(family_prod_id_table)) {
+		prod_id = family_prod_id_table[chip->info->family];
+		if (prod_id)
+			val |= prod_id >> 4;
 	}
 
 	return err ? err : val;
diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index 91cf1d167263..9dbe625ad447 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -2401,9 +2401,31 @@ static struct phy_driver marvell_drivers[] = {
 		.get_stats = marvell_get_stats,
 	},
 	{
-		.phy_id = MARVELL_PHY_ID_88E6390,
+		.phy_id = MARVELL_PHY_ID_88E6341_FAMILY,
 		.phy_id_mask = MARVELL_PHY_ID_MASK,
-		.name = "Marvell 88E6390",
+		.name = "Marvell 88E6341 Family",
+		/* PHY_GBIT_FEATURES */
+		.probe = m88e1510_probe,
+		.config_init = &marvell_config_init,
+		.config_aneg = &m88e6390_config_aneg,
+		.read_status = &marvell_read_status,
+		.ack_interrupt = &marvell_ack_interrupt,
+		.config_intr = &marvell_config_intr,
+		.did_interrupt = &m88e1121_did_interrupt,
+		.resume = &genphy_resume,
+		.suspend = &genphy_suspend,
+		.read_page = marvell_read_page,
+		.write_page = marvell_write_page,
+		.get_sset_count = marvell_get_sset_count,
+		.get_strings = marvell_get_strings,
+		.get_stats = marvell_get_stats,
+		.get_tunable = m88e1540_get_tunable,
+		.set_tunable = m88e1540_set_tunable,
+	},
+	{
+		.phy_id = MARVELL_PHY_ID_88E6390_FAMILY,
+		.phy_id_mask = MARVELL_PHY_ID_MASK,
+		.name = "Marvell 88E6390 Family",
 		/* PHY_GBIT_FEATURES */
 		.probe = m88e6390_probe,
 		.config_init = &marvell_config_init,
@@ -2441,7 +2463,8 @@ static struct mdio_device_id __maybe_unused marvell_tbl[] = {
 	{ MARVELL_PHY_ID_88E1540, MARVELL_PHY_ID_MASK },
 	{ MARVELL_PHY_ID_88E1545, MARVELL_PHY_ID_MASK },
 	{ MARVELL_PHY_ID_88E3016, MARVELL_PHY_ID_MASK },
-	{ MARVELL_PHY_ID_88E6390, MARVELL_PHY_ID_MASK },
+	{ MARVELL_PHY_ID_88E6341_FAMILY, MARVELL_PHY_ID_MASK },
+	{ MARVELL_PHY_ID_88E6390_FAMILY, MARVELL_PHY_ID_MASK },
 	{ }
 };
 
diff --git a/include/linux/marvell_phy.h b/include/linux/marvell_phy.h
index af6b11d4d673..1847a0784243 100644
--- a/include/linux/marvell_phy.h
+++ b/include/linux/marvell_phy.h
@@ -23,11 +23,12 @@
 #define MARVELL_PHY_ID_88X3310		0x002b09a0
 #define MARVELL_PHY_ID_88E2110		0x002b09b0
 
-/* The MV88e6390 Ethernet switch contains embedded PHYs. These PHYs do
+/* These Ethernet switch families contain embedded PHYs, but they do
  * not have a model ID. So the switch driver traps reads to the ID2
  * register and returns the switch family ID
  */
-#define MARVELL_PHY_ID_88E6390		0x01410f90
+#define MARVELL_PHY_ID_88E6341_FAMILY	0x01410f41
+#define MARVELL_PHY_ID_88E6390_FAMILY	0x01410f90
 
 #define MARVELL_PHY_FAMILY_ID(id)	((id) >> 4)
 
-- 
2.20.1

Re: [PATCH] net: phy: marvell: fix detection of PHY on Topaz switches

[Greg KH]

On Sun, Apr 18, 2021 at 03:13:44PM +0200, Pali Rohár wrote:
> commit 1fe976d308acb6374c899a4ee8025a0a016e453e upstream.
> 
> Since commit fee2d546414d ("net: phy: marvell: mv88e6390 temperature
> sensor reading"), Linux reports the temperature of Topaz hwmon as
> constant -75°C.
> 
> This is because switches from the Topaz family (88E6141 / 88E6341) have
> the address of the temperature sensor register different from Peridot.
> 
> This address is instead compatible with 88E1510 PHYs, as was used for
> Topaz before the above mentioned commit.
> 
> Create a new mapping table between switch family and PHY ID for families
> which don't have a model number. And define PHY IDs for Topaz and Peridot
> families.
> 
> Create a new PHY ID and a new PHY driver for Topaz's internal PHY.
> The only difference from Peridot's PHY driver is the HWMON probing
> method.
> 
> Prior this change Topaz's internal PHY is detected by kernel as:
> 
>   PHY [...] driver [Marvell 88E6390] (irq=63)
> 
> And afterwards as:
> 
>   PHY [...] driver [Marvell 88E6341 Family] (irq=63)
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> BugLink: https://github.com/globalscaletechnologies/linux/issues/1
> Fixes: fee2d546414d ("net: phy: marvell: mv88e6390 temperature sensor reading")
> Reviewed-by: Marek Behún <kabel@kernel.org>
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> Signed-off-by: David S. Miller <davem@davemloft.net>
> [pali: Backported to 5.4 version]
> ---
> This patch is backported to 5.4 version. Tested on Turris Mox with Topaz switch.

What about a 5.10 version?

thanks,

greg k-h

Re: [PATCH] net: phy: marvell: fix detection of PHY on Topaz switches

[Pali Rohár]

On Monday 19 April 2021 14:05:18 Greg KH wrote:
> On Sun, Apr 18, 2021 at 03:13:44PM +0200, Pali Rohár wrote:
> > commit 1fe976d308acb6374c899a4ee8025a0a016e453e upstream.
> > 
> > Since commit fee2d546414d ("net: phy: marvell: mv88e6390 temperature
> > sensor reading"), Linux reports the temperature of Topaz hwmon as
> > constant -75°C.
> > 
> > This is because switches from the Topaz family (88E6141 / 88E6341) have
> > the address of the temperature sensor register different from Peridot.
> > 
> > This address is instead compatible with 88E1510 PHYs, as was used for
> > Topaz before the above mentioned commit.
> > 
> > Create a new mapping table between switch family and PHY ID for families
> > which don't have a model number. And define PHY IDs for Topaz and Peridot
> > families.
> > 
> > Create a new PHY ID and a new PHY driver for Topaz's internal PHY.
> > The only difference from Peridot's PHY driver is the HWMON probing
> > method.
> > 
> > Prior this change Topaz's internal PHY is detected by kernel as:
> > 
> >   PHY [...] driver [Marvell 88E6390] (irq=63)
> > 
> > And afterwards as:
> > 
> >   PHY [...] driver [Marvell 88E6341 Family] (irq=63)
> > 
> > Signed-off-by: Pali Rohár <pali@kernel.org>
> > BugLink: https://github.com/globalscaletechnologies/linux/issues/1
> > Fixes: fee2d546414d ("net: phy: marvell: mv88e6390 temperature sensor reading")
> > Reviewed-by: Marek Behún <kabel@kernel.org>
> > Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> > Signed-off-by: David S. Miller <davem@davemloft.net>
> > [pali: Backported to 5.4 version]
> > ---
> > This patch is backported to 5.4 version. Tested on Turris Mox with Topaz switch.
> 
> What about a 5.10 version?

Is manual backport required also for 5.10? I got email that automatic
backporting failed only for 4.19 and 5.4 versions.

> thanks,
> 
> greg k-h

Re: [PATCH] net: phy: marvell: fix detection of PHY on Topaz switches

[Greg KH]

On Mon, Apr 19, 2021 at 02:08:56PM +0200, Pali Rohár wrote:
> On Monday 19 April 2021 14:05:18 Greg KH wrote:
> > On Sun, Apr 18, 2021 at 03:13:44PM +0200, Pali Rohár wrote:
> > > commit 1fe976d308acb6374c899a4ee8025a0a016e453e upstream.
> > > 
> > > Since commit fee2d546414d ("net: phy: marvell: mv88e6390 temperature
> > > sensor reading"), Linux reports the temperature of Topaz hwmon as
> > > constant -75°C.
> > > 
> > > This is because switches from the Topaz family (88E6141 / 88E6341) have
> > > the address of the temperature sensor register different from Peridot.
> > > 
> > > This address is instead compatible with 88E1510 PHYs, as was used for
> > > Topaz before the above mentioned commit.
> > > 
> > > Create a new mapping table between switch family and PHY ID for families
> > > which don't have a model number. And define PHY IDs for Topaz and Peridot
> > > families.
> > > 
> > > Create a new PHY ID and a new PHY driver for Topaz's internal PHY.
> > > The only difference from Peridot's PHY driver is the HWMON probing
> > > method.
> > > 
> > > Prior this change Topaz's internal PHY is detected by kernel as:
> > > 
> > >   PHY [...] driver [Marvell 88E6390] (irq=63)
> > > 
> > > And afterwards as:
> > > 
> > >   PHY [...] driver [Marvell 88E6341 Family] (irq=63)
> > > 
> > > Signed-off-by: Pali Rohár <pali@kernel.org>
> > > BugLink: https://github.com/globalscaletechnologies/linux/issues/1
> > > Fixes: fee2d546414d ("net: phy: marvell: mv88e6390 temperature sensor reading")
> > > Reviewed-by: Marek Behún <kabel@kernel.org>
> > > Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> > > Signed-off-by: David S. Miller <davem@davemloft.net>
> > > [pali: Backported to 5.4 version]
> > > ---
> > > This patch is backported to 5.4 version. Tested on Turris Mox with Topaz switch.
> > 
> > What about a 5.10 version?
> 
> Is manual backport required also for 5.10? I got email that automatic
> backporting failed only for 4.19 and 5.4 versions.

It also failed for 5.10.y, so yes, if you could provide a version for
that tree it would be most appreciated.

thanks,

greg k-h

Re: [PATCH] net: phy: marvell: fix detection of PHY on Topaz switches

[Pali Rohár]

On Monday 19 April 2021 14:35:19 Greg KH wrote:
> On Mon, Apr 19, 2021 at 02:08:56PM +0200, Pali Rohár wrote:
> > On Monday 19 April 2021 14:05:18 Greg KH wrote:
> > > On Sun, Apr 18, 2021 at 03:13:44PM +0200, Pali Rohár wrote:
> > > > commit 1fe976d308acb6374c899a4ee8025a0a016e453e upstream.
> > > > 
> > > > Since commit fee2d546414d ("net: phy: marvell: mv88e6390 temperature
> > > > sensor reading"), Linux reports the temperature of Topaz hwmon as
> > > > constant -75°C.
> > > > 
> > > > This is because switches from the Topaz family (88E6141 / 88E6341) have
> > > > the address of the temperature sensor register different from Peridot.
> > > > 
> > > > This address is instead compatible with 88E1510 PHYs, as was used for
> > > > Topaz before the above mentioned commit.
> > > > 
> > > > Create a new mapping table between switch family and PHY ID for families
> > > > which don't have a model number. And define PHY IDs for Topaz and Peridot
> > > > families.
> > > > 
> > > > Create a new PHY ID and a new PHY driver for Topaz's internal PHY.
> > > > The only difference from Peridot's PHY driver is the HWMON probing
> > > > method.
> > > > 
> > > > Prior this change Topaz's internal PHY is detected by kernel as:
> > > > 
> > > >   PHY [...] driver [Marvell 88E6390] (irq=63)
> > > > 
> > > > And afterwards as:
> > > > 
> > > >   PHY [...] driver [Marvell 88E6341 Family] (irq=63)
> > > > 
> > > > Signed-off-by: Pali Rohár <pali@kernel.org>
> > > > BugLink: https://github.com/globalscaletechnologies/linux/issues/1
> > > > Fixes: fee2d546414d ("net: phy: marvell: mv88e6390 temperature sensor reading")
> > > > Reviewed-by: Marek Behún <kabel@kernel.org>
> > > > Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> > > > Signed-off-by: David S. Miller <davem@davemloft.net>
> > > > [pali: Backported to 5.4 version]
> > > > ---
> > > > This patch is backported to 5.4 version. Tested on Turris Mox with Topaz switch.
> > > 
> > > What about a 5.10 version?
> > 
> > Is manual backport required also for 5.10? I got email that automatic
> > backporting failed only for 4.19 and 5.4 versions.
> 
> It also failed for 5.10.y, so yes, if you could provide a version for
> that tree it would be most appreciated.

Ok! I will prepare it, no problem. I just did not know that it failed
also for 5.10 as I have not received any email about it.

> thanks,
> 
> greg k-h

Re: [PATCH] net: phy: marvell: fix detection of PHY on Topaz switches

[Greg KH]

On Mon, Apr 19, 2021 at 02:47:11PM +0200, Pali Rohár wrote:
> On Monday 19 April 2021 14:35:19 Greg KH wrote:
> > On Mon, Apr 19, 2021 at 02:08:56PM +0200, Pali Rohár wrote:
> > > On Monday 19 April 2021 14:05:18 Greg KH wrote:
> > > > On Sun, Apr 18, 2021 at 03:13:44PM +0200, Pali Rohár wrote:
> > > > > commit 1fe976d308acb6374c899a4ee8025a0a016e453e upstream.
> > > > > 
> > > > > Since commit fee2d546414d ("net: phy: marvell: mv88e6390 temperature
> > > > > sensor reading"), Linux reports the temperature of Topaz hwmon as
> > > > > constant -75°C.
> > > > > 
> > > > > This is because switches from the Topaz family (88E6141 / 88E6341) have
> > > > > the address of the temperature sensor register different from Peridot.
> > > > > 
> > > > > This address is instead compatible with 88E1510 PHYs, as was used for
> > > > > Topaz before the above mentioned commit.
> > > > > 
> > > > > Create a new mapping table between switch family and PHY ID for families
> > > > > which don't have a model number. And define PHY IDs for Topaz and Peridot
> > > > > families.
> > > > > 
> > > > > Create a new PHY ID and a new PHY driver for Topaz's internal PHY.
> > > > > The only difference from Peridot's PHY driver is the HWMON probing
> > > > > method.
> > > > > 
> > > > > Prior this change Topaz's internal PHY is detected by kernel as:
> > > > > 
> > > > >   PHY [...] driver [Marvell 88E6390] (irq=63)
> > > > > 
> > > > > And afterwards as:
> > > > > 
> > > > >   PHY [...] driver [Marvell 88E6341 Family] (irq=63)
> > > > > 
> > > > > Signed-off-by: Pali Rohár <pali@kernel.org>
> > > > > BugLink: https://github.com/globalscaletechnologies/linux/issues/1
> > > > > Fixes: fee2d546414d ("net: phy: marvell: mv88e6390 temperature sensor reading")
> > > > > Reviewed-by: Marek Behún <kabel@kernel.org>
> > > > > Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> > > > > Signed-off-by: David S. Miller <davem@davemloft.net>
> > > > > [pali: Backported to 5.4 version]
> > > > > ---
> > > > > This patch is backported to 5.4 version. Tested on Turris Mox with Topaz switch.
> > > > 
> > > > What about a 5.10 version?
> > > 
> > > Is manual backport required also for 5.10? I got email that automatic
> > > backporting failed only for 4.19 and 5.4 versions.
> > 
> > It also failed for 5.10.y, so yes, if you could provide a version for
> > that tree it would be most appreciated.
> 
> Ok! I will prepare it, no problem. I just did not know that it failed
> also for 5.10 as I have not received any email about it.

Odd, I must have forgotten to add that version to the command line
script I run to tell people that a patch failed, sorry about that.

greg k-h

Re: [PATCH] net: phy: marvell: fix detection of PHY on Topaz switches

[Pali Rohár]

On Monday 19 April 2021 14:56:54 Greg KH wrote:
> On Mon, Apr 19, 2021 at 02:47:11PM +0200, Pali Rohár wrote:
> > On Monday 19 April 2021 14:35:19 Greg KH wrote:
> > > On Mon, Apr 19, 2021 at 02:08:56PM +0200, Pali Rohár wrote:
> > > > On Monday 19 April 2021 14:05:18 Greg KH wrote:
> > > > > On Sun, Apr 18, 2021 at 03:13:44PM +0200, Pali Rohár wrote:
> > > > > > commit 1fe976d308acb6374c899a4ee8025a0a016e453e upstream.
> > > > > > 
> > > > > > Since commit fee2d546414d ("net: phy: marvell: mv88e6390 temperature
> > > > > > sensor reading"), Linux reports the temperature of Topaz hwmon as
> > > > > > constant -75°C.
> > > > > > 
> > > > > > This is because switches from the Topaz family (88E6141 / 88E6341) have
> > > > > > the address of the temperature sensor register different from Peridot.
> > > > > > 
> > > > > > This address is instead compatible with 88E1510 PHYs, as was used for
> > > > > > Topaz before the above mentioned commit.
> > > > > > 
> > > > > > Create a new mapping table between switch family and PHY ID for families
> > > > > > which don't have a model number. And define PHY IDs for Topaz and Peridot
> > > > > > families.
> > > > > > 
> > > > > > Create a new PHY ID and a new PHY driver for Topaz's internal PHY.
> > > > > > The only difference from Peridot's PHY driver is the HWMON probing
> > > > > > method.
> > > > > > 
> > > > > > Prior this change Topaz's internal PHY is detected by kernel as:
> > > > > > 
> > > > > >   PHY [...] driver [Marvell 88E6390] (irq=63)
> > > > > > 
> > > > > > And afterwards as:
> > > > > > 
> > > > > >   PHY [...] driver [Marvell 88E6341 Family] (irq=63)
> > > > > > 
> > > > > > Signed-off-by: Pali Rohár <pali@kernel.org>
> > > > > > BugLink: https://github.com/globalscaletechnologies/linux/issues/1
> > > > > > Fixes: fee2d546414d ("net: phy: marvell: mv88e6390 temperature sensor reading")
> > > > > > Reviewed-by: Marek Behún <kabel@kernel.org>
> > > > > > Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> > > > > > Signed-off-by: David S. Miller <davem@davemloft.net>
> > > > > > [pali: Backported to 5.4 version]
> > > > > > ---
> > > > > > This patch is backported to 5.4 version. Tested on Turris Mox with Topaz switch.
> > > > > 
> > > > > What about a 5.10 version?
> > > > 
> > > > Is manual backport required also for 5.10? I got email that automatic
> > > > backporting failed only for 4.19 and 5.4 versions.
> > > 
> > > It also failed for 5.10.y, so yes, if you could provide a version for
> > > that tree it would be most appreciated.
> > 
> > Ok! I will prepare it, no problem. I just did not know that it failed
> > also for 5.10 as I have not received any email about it.
> 
> Odd, I must have forgotten to add that version to the command line
> script I run to tell people that a patch failed, sorry about that.
> 
> greg k-h

Now I have sent version also for 5.10. Beware that patch for every
kernel version is slightly different.

Re: [PATCH] net: phy: marvell: fix detection of PHY on Topaz switches

[Sasha Levin]

On Mon, Apr 19, 2021 at 02:56:54PM +0200, Greg KH wrote:
>On Mon, Apr 19, 2021 at 02:47:11PM +0200, Pali Rohár wrote:
>> Ok! I will prepare it, no problem. I just did not know that it failed
>> also for 5.10 as I have not received any email about it.
>
>Odd, I must have forgotten to add that version to the command line
>script I run to tell people that a patch failed, sorry about that.

Not sure what happened, but there are 2 "FAILED" mails for 5.4, one of
them must have been for 5.10 :)

-- 
Thanks,
Sasha

Re: [PATCH] net: phy: marvell: fix detection of PHY on Topaz switches

[Greg KH]

On Mon, Apr 19, 2021 at 03:47:09PM +0200, Pali Rohár wrote:
> commit 1fe976d308acb6374c899a4ee8025a0a016e453e upstream.
> 
> Since commit fee2d546414d ("net: phy: marvell: mv88e6390 temperature
> sensor reading"), Linux reports the temperature of Topaz hwmon as
> constant -75°C.
> 
> This is because switches from the Topaz family (88E6141 / 88E6341) have
> the address of the temperature sensor register different from Peridot.
> 
> This address is instead compatible with 88E1510 PHYs, as was used for
> Topaz before the above mentioned commit.
> 
> Create a new mapping table between switch family and PHY ID for families
> which don't have a model number. And define PHY IDs for Topaz and Peridot
> families.
> 
> Create a new PHY ID and a new PHY driver for Topaz's internal PHY.
> The only difference from Peridot's PHY driver is the HWMON probing
> method.
> 
> Prior this change Topaz's internal PHY is detected by kernel as:
> 
>   PHY [...] driver [Marvell 88E6390] (irq=63)
> 
> And afterwards as:
> 
>   PHY [...] driver [Marvell 88E6341 Family] (irq=63)
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> BugLink: https://github.com/globalscaletechnologies/linux/issues/1
> Fixes: fee2d546414d ("net: phy: marvell: mv88e6390 temperature sensor reading")
> Reviewed-by: Marek Behún <kabel@kernel.org>
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> Signed-off-by: David S. Miller <davem@davemloft.net>
> [pali: Backported to 5.10 version]
> ---
> This patch is backported to 5.10 version. Tested on Turris Mox with Topaz switch.

Now queued up,t hanks.

greg k-h

[PATCH] net: phy: marvell: fix detection of PHY on Topaz switches

[Pali Rohár]

commit 1fe976d308acb6374c899a4ee8025a0a016e453e upstream.

Since commit fee2d546414d ("net: phy: marvell: mv88e6390 temperature
sensor reading"), Linux reports the temperature of Topaz hwmon as
constant -75°C.

This is because switches from the Topaz family (88E6141 / 88E6341) have
the address of the temperature sensor register different from Peridot.

This address is instead compatible with 88E1510 PHYs, as was used for
Topaz before the above mentioned commit.

Create a new mapping table between switch family and PHY ID for families
which don't have a model number. And define PHY IDs for Topaz and Peridot
families.

Create a new PHY ID and a new PHY driver for Topaz's internal PHY.
The only difference from Peridot's PHY driver is the HWMON probing
method.

Prior this change Topaz's internal PHY is detected by kernel as:

  PHY [...] driver [Marvell 88E6390] (irq=63)

And afterwards as:

  PHY [...] driver [Marvell 88E6341 Family] (irq=63)

Signed-off-by: Pali Rohár <pali@kernel.org>
BugLink: https://github.com/globalscaletechnologies/linux/issues/1
Fixes: fee2d546414d ("net: phy: marvell: mv88e6390 temperature sensor reading")
Reviewed-by: Marek Behún <kabel@kernel.org>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: David S. Miller <davem@davemloft.net>
[pali: Backported to 5.10 version]
---
This patch is backported to 5.10 version. Tested on Turris Mox with Topaz switch.
---
 drivers/net/dsa/mv88e6xxx/chip.c | 30 +++++++++++++----------------
 drivers/net/phy/marvell.c        | 33 +++++++++++++++++++++++++++++---
 include/linux/marvell_phy.h      |  5 +++--
 3 files changed, 46 insertions(+), 22 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 87160e723dfc..70ec17f3c300 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -2994,10 +2994,17 @@ static int mv88e6xxx_setup(struct dsa_switch *ds)
 	return err;
 }
 
+/* prod_id for switch families which do not have a PHY model number */
+static const u16 family_prod_id_table[] = {
+	[MV88E6XXX_FAMILY_6341] = MV88E6XXX_PORT_SWITCH_ID_PROD_6341,
+	[MV88E6XXX_FAMILY_6390] = MV88E6XXX_PORT_SWITCH_ID_PROD_6390,
+};
+
 static int mv88e6xxx_mdio_read(struct mii_bus *bus, int phy, int reg)
 {
 	struct mv88e6xxx_mdio_bus *mdio_bus = bus->priv;
 	struct mv88e6xxx_chip *chip = mdio_bus->chip;
+	u16 prod_id;
 	u16 val;
 	int err;
 
@@ -3008,23 +3015,12 @@ static int mv88e6xxx_mdio_read(struct mii_bus *bus, int phy, int reg)
 	err = chip->info->ops->phy_read(chip, bus, phy, reg, &val);
 	mv88e6xxx_reg_unlock(chip);
 
-	if (reg == MII_PHYSID2) {
-		/* Some internal PHYs don't have a model number. */
-		if (chip->info->family != MV88E6XXX_FAMILY_6165)
-			/* Then there is the 6165 family. It gets is
-			 * PHYs correct. But it can also have two
-			 * SERDES interfaces in the PHY address
-			 * space. And these don't have a model
-			 * number. But they are not PHYs, so we don't
-			 * want to give them something a PHY driver
-			 * will recognise.
-			 *
-			 * Use the mv88e6390 family model number
-			 * instead, for anything which really could be
-			 * a PHY,
-			 */
-			if (!(val & 0x3f0))
-				val |= MV88E6XXX_PORT_SWITCH_ID_PROD_6390 >> 4;
+	/* Some internal PHYs don't have a model number. */
+	if (reg == MII_PHYSID2 && !(val & 0x3f0) &&
+	    chip->info->family < ARRAY_SIZE(family_prod_id_table)) {
+		prod_id = family_prod_id_table[chip->info->family];
+		if (prod_id)
+			val |= prod_id >> 4;
 	}
 
 	return err ? err : val;
diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index 5dbdaf0f5f09..823a89354466 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -2913,9 +2913,35 @@ static struct phy_driver marvell_drivers[] = {
 		.get_stats = marvell_get_stats,
 	},
 	{
-		.phy_id = MARVELL_PHY_ID_88E6390,
+		.phy_id = MARVELL_PHY_ID_88E6341_FAMILY,
 		.phy_id_mask = MARVELL_PHY_ID_MASK,
-		.name = "Marvell 88E6390",
+		.name = "Marvell 88E6341 Family",
+		/* PHY_GBIT_FEATURES */
+		.flags = PHY_POLL_CABLE_TEST,
+		.probe = m88e1510_probe,
+		.config_init = marvell_config_init,
+		.config_aneg = m88e6390_config_aneg,
+		.read_status = marvell_read_status,
+		.ack_interrupt = marvell_ack_interrupt,
+		.config_intr = marvell_config_intr,
+		.did_interrupt = m88e1121_did_interrupt,
+		.resume = genphy_resume,
+		.suspend = genphy_suspend,
+		.read_page = marvell_read_page,
+		.write_page = marvell_write_page,
+		.get_sset_count = marvell_get_sset_count,
+		.get_strings = marvell_get_strings,
+		.get_stats = marvell_get_stats,
+		.get_tunable = m88e1540_get_tunable,
+		.set_tunable = m88e1540_set_tunable,
+		.cable_test_start = marvell_vct7_cable_test_start,
+		.cable_test_tdr_start = marvell_vct5_cable_test_tdr_start,
+		.cable_test_get_status = marvell_vct7_cable_test_get_status,
+	},
+	{
+		.phy_id = MARVELL_PHY_ID_88E6390_FAMILY,
+		.phy_id_mask = MARVELL_PHY_ID_MASK,
+		.name = "Marvell 88E6390 Family",
 		/* PHY_GBIT_FEATURES */
 		.flags = PHY_POLL_CABLE_TEST,
 		.probe = m88e6390_probe,
@@ -3001,7 +3027,8 @@ static struct mdio_device_id __maybe_unused marvell_tbl[] = {
 	{ MARVELL_PHY_ID_88E1540, MARVELL_PHY_ID_MASK },
 	{ MARVELL_PHY_ID_88E1545, MARVELL_PHY_ID_MASK },
 	{ MARVELL_PHY_ID_88E3016, MARVELL_PHY_ID_MASK },
-	{ MARVELL_PHY_ID_88E6390, MARVELL_PHY_ID_MASK },
+	{ MARVELL_PHY_ID_88E6341_FAMILY, MARVELL_PHY_ID_MASK },
+	{ MARVELL_PHY_ID_88E6390_FAMILY, MARVELL_PHY_ID_MASK },
 	{ MARVELL_PHY_ID_88E1340S, MARVELL_PHY_ID_MASK },
 	{ MARVELL_PHY_ID_88E1548P, MARVELL_PHY_ID_MASK },
 	{ }
diff --git a/include/linux/marvell_phy.h b/include/linux/marvell_phy.h
index ff7b7607c8cf..f5cf19d19776 100644
--- a/include/linux/marvell_phy.h
+++ b/include/linux/marvell_phy.h
@@ -25,11 +25,12 @@
 #define MARVELL_PHY_ID_88X3310		0x002b09a0
 #define MARVELL_PHY_ID_88E2110		0x002b09b0
 
-/* The MV88e6390 Ethernet switch contains embedded PHYs. These PHYs do
+/* These Ethernet switch families contain embedded PHYs, but they do
  * not have a model ID. So the switch driver traps reads to the ID2
  * register and returns the switch family ID
  */
-#define MARVELL_PHY_ID_88E6390		0x01410f90
+#define MARVELL_PHY_ID_88E6341_FAMILY	0x01410f41
+#define MARVELL_PHY_ID_88E6390_FAMILY	0x01410f90
 
 #define MARVELL_PHY_FAMILY_ID(id)	((id) >> 4)
 
-- 
2.20.1

[PATCH] net: phy: marvell: fix detection of PHY on Topaz switches

[Pali Rohár]

commit 1fe976d308acb6374c899a4ee8025a0a016e453e upstream.

Since commit fee2d546414d ("net: phy: marvell: mv88e6390 temperature
sensor reading"), Linux reports the temperature of Topaz hwmon as
constant -75°C.

This is because switches from the Topaz family (88E6141 / 88E6341) have
the address of the temperature sensor register different from Peridot.

This address is instead compatible with 88E1510 PHYs, as was used for
Topaz before the above mentioned commit.

Create a new mapping table between switch family and PHY ID for families
which don't have a model number. And define PHY IDs for Topaz and Peridot
families.

Create a new PHY ID and a new PHY driver for Topaz's internal PHY.
The only difference from Peridot's PHY driver is the HWMON probing
method.

Prior this change Topaz's internal PHY is detected by kernel as:

  PHY [...] driver [Marvell 88E6390] (irq=63)

And afterwards as:

  PHY [...] driver [Marvell 88E6341 Family] (irq=63)

Signed-off-by: Pali Rohár <pali@kernel.org>
BugLink: https://github.com/globalscaletechnologies/linux/issues/1
Fixes: fee2d546414d ("net: phy: marvell: mv88e6390 temperature sensor reading")
Reviewed-by: Marek Behún <kabel@kernel.org>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: David S. Miller <davem@davemloft.net>
[pali: Backported to 4.19 version]
---
This patch is backported to 4.19 version. Compile-tested only.
---
 drivers/net/dsa/mv88e6xxx/chip.c | 30 +++++++++++++-----------------
 drivers/net/phy/marvell.c        | 28 +++++++++++++++++++++++++---
 include/linux/marvell_phy.h      |  5 +++--
 3 files changed, 41 insertions(+), 22 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index e04b7fa068af..67c0ad3b8079 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -2634,10 +2634,17 @@ static int mv88e6xxx_setup(struct dsa_switch *ds)
 	return err;
 }
 
+/* prod_id for switch families which do not have a PHY model number */
+static const u16 family_prod_id_table[] = {
+	[MV88E6XXX_FAMILY_6341] = MV88E6XXX_PORT_SWITCH_ID_PROD_6341,
+	[MV88E6XXX_FAMILY_6390] = MV88E6XXX_PORT_SWITCH_ID_PROD_6390,
+};
+
 static int mv88e6xxx_mdio_read(struct mii_bus *bus, int phy, int reg)
 {
 	struct mv88e6xxx_mdio_bus *mdio_bus = bus->priv;
 	struct mv88e6xxx_chip *chip = mdio_bus->chip;
+	u16 prod_id;
 	u16 val;
 	int err;
 
@@ -2648,23 +2655,12 @@ static int mv88e6xxx_mdio_read(struct mii_bus *bus, int phy, int reg)
 	err = chip->info->ops->phy_read(chip, bus, phy, reg, &val);
 	mutex_unlock(&chip->reg_lock);
 
-	if (reg == MII_PHYSID2) {
-		/* Some internal PHYs don't have a model number. */
-		if (chip->info->family != MV88E6XXX_FAMILY_6165)
-			/* Then there is the 6165 family. It gets is
-			 * PHYs correct. But it can also have two
-			 * SERDES interfaces in the PHY address
-			 * space. And these don't have a model
-			 * number. But they are not PHYs, so we don't
-			 * want to give them something a PHY driver
-			 * will recognise.
-			 *
-			 * Use the mv88e6390 family model number
-			 * instead, for anything which really could be
-			 * a PHY,
-			 */
-			if (!(val & 0x3f0))
-				val |= MV88E6XXX_PORT_SWITCH_ID_PROD_6390 >> 4;
+	/* Some internal PHYs don't have a model number. */
+	if (reg == MII_PHYSID2 && !(val & 0x3f0) &&
+	    chip->info->family < ARRAY_SIZE(family_prod_id_table)) {
+		prod_id = family_prod_id_table[chip->info->family];
+		if (prod_id)
+			val |= prod_id >> 4;
 	}
 
 	return err ? err : val;
diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index bb6107f3b947..832a401c5fa5 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -2329,9 +2329,30 @@ static struct phy_driver marvell_drivers[] = {
 		.get_stats = marvell_get_stats,
 	},
 	{
-		.phy_id = MARVELL_PHY_ID_88E6390,
+		.phy_id = MARVELL_PHY_ID_88E6341_FAMILY,
 		.phy_id_mask = MARVELL_PHY_ID_MASK,
-		.name = "Marvell 88E6390",
+		.name = "Marvell 88E6341 Family",
+		.features = PHY_GBIT_FEATURES,
+		.flags = PHY_HAS_INTERRUPT,
+		.probe = m88e1510_probe,
+		.config_init = &marvell_config_init,
+		.config_aneg = &m88e6390_config_aneg,
+		.read_status = &marvell_read_status,
+		.ack_interrupt = &marvell_ack_interrupt,
+		.config_intr = &marvell_config_intr,
+		.did_interrupt = &m88e1121_did_interrupt,
+		.resume = &genphy_resume,
+		.suspend = &genphy_suspend,
+		.read_page = marvell_read_page,
+		.write_page = marvell_write_page,
+		.get_sset_count = marvell_get_sset_count,
+		.get_strings = marvell_get_strings,
+		.get_stats = marvell_get_stats,
+	},
+	{
+		.phy_id = MARVELL_PHY_ID_88E6390_FAMILY,
+		.phy_id_mask = MARVELL_PHY_ID_MASK,
+		.name = "Marvell 88E6390 Family",
 		.features = PHY_GBIT_FEATURES,
 		.flags = PHY_HAS_INTERRUPT,
 		.probe = m88e6390_probe,
@@ -2368,7 +2389,8 @@ static struct mdio_device_id __maybe_unused marvell_tbl[] = {
 	{ MARVELL_PHY_ID_88E1540, MARVELL_PHY_ID_MASK },
 	{ MARVELL_PHY_ID_88E1545, MARVELL_PHY_ID_MASK },
 	{ MARVELL_PHY_ID_88E3016, MARVELL_PHY_ID_MASK },
-	{ MARVELL_PHY_ID_88E6390, MARVELL_PHY_ID_MASK },
+	{ MARVELL_PHY_ID_88E6341_FAMILY, MARVELL_PHY_ID_MASK },
+	{ MARVELL_PHY_ID_88E6390_FAMILY, MARVELL_PHY_ID_MASK },
 	{ }
 };
 
diff --git a/include/linux/marvell_phy.h b/include/linux/marvell_phy.h
index 1eb6f244588d..9a488497ebc2 100644
--- a/include/linux/marvell_phy.h
+++ b/include/linux/marvell_phy.h
@@ -21,11 +21,12 @@
 #define MARVELL_PHY_ID_88E1545		0x01410ea0
 #define MARVELL_PHY_ID_88E3016		0x01410e60
 
-/* The MV88e6390 Ethernet switch contains embedded PHYs. These PHYs do
+/* These Ethernet switch families contain embedded PHYs, but they do
  * not have a model ID. So the switch driver traps reads to the ID2
  * register and returns the switch family ID
  */
-#define MARVELL_PHY_ID_88E6390		0x01410f90
+#define MARVELL_PHY_ID_88E6341_FAMILY	0x01410f41
+#define MARVELL_PHY_ID_88E6390_FAMILY	0x01410f90
 
 #define MARVELL_PHY_FAMILY_ID(id)	((id) >> 4)
 
-- 
2.20.1

Re: [PATCH] net: phy: marvell: fix detection of PHY on Topaz switches

[Marek Behún]

On Mon, 12 Apr 2021 18:38:29 +0200
Pali Rohár <pali@kernel.org> wrote:

> On Monday 12 April 2021 18:12:35 Andrew Lunn wrote:
> > On Mon, Apr 12, 2021 at 05:52:39PM +0200, Pali Rohár wrote:  
> > > On Monday 12 April 2021 17:32:33 Andrew Lunn wrote:  
> > > > > Anyway, now I'm looking at phy/marvell.c driver again and it supports
> > > > > only 88E6341 and 88E6390 families from whole 88E63xxx range.
> > > > > 
> > > > > So do we need to define for now table for more than
> > > > > MV88E6XXX_FAMILY_6341 and MV88E6XXX_FAMILY_6390 entries?  
> > > > 
> > > > Probably not. I've no idea if the 6393 has an ID, so to be safe you
> > > > should add that. Assuming it has a family of its own.  
> > > 
> > > So what about just?
> > > 
> > > 	if (reg == MII_PHYSID2 && !(val & 0x3f0)) {
> > > 		if (chip->info->family == MV88E6XXX_FAMILY_6341)
> > > 			val |= MV88E6XXX_PORT_SWITCH_ID_PROD_6341 >> 4;
> > > 		else if (chip->info->family == MV88E6XXX_FAMILY_6390)
> > > 			val |= MV88E6XXX_PORT_SWITCH_ID_PROD_6390 >> 4;
> > > 	}  
> > 
> > As i said, i expect the 6393 also has no ID. And i recently found out
> > Marvell have some automotive switches, 88Q5xxx which are actually
> > based around the same IP and could be added to this driver. They also
> > might not have an ID. I suspect this list is going to get longer, so
> > having it table driven will make that simpler, less error prone.
> > 
> >      Andrew  
> 
> Ok, I will use table but I fill it only with Topaz (6341) and Peridot
> (6390) which was there before as I do not have 6393 switch for testing.
> 
> If you or anybody else has 6393 unit for testing, please extend then
> table.

6393 PHYs report PHY ID 0x002b0808, I.e. no model number.

I now realize that I did not implement this for 6393, these PHYs are
detected as
  mv88e6085 ... PHY [...] driver [Generic PHY] (irq=POLL)

And it seems that this temperature sensor is different from 1510, 6341
and 6390 :) I will look into this and send a patch.

Marek

Re: [PATCH] net: phy: marvell: fix detection of PHY on Topaz switches

[Pali Rohár]

On Monday 12 April 2021 18:12:35 Andrew Lunn wrote:
> On Mon, Apr 12, 2021 at 05:52:39PM +0200, Pali Rohár wrote:
> > On Monday 12 April 2021 17:32:33 Andrew Lunn wrote:
> > > > Anyway, now I'm looking at phy/marvell.c driver again and it supports
> > > > only 88E6341 and 88E6390 families from whole 88E63xxx range.
> > > > 
> > > > So do we need to define for now table for more than
> > > > MV88E6XXX_FAMILY_6341 and MV88E6XXX_FAMILY_6390 entries?
> > > 
> > > Probably not. I've no idea if the 6393 has an ID, so to be safe you
> > > should add that. Assuming it has a family of its own.
> > 
> > So what about just?
> > 
> > 	if (reg == MII_PHYSID2 && !(val & 0x3f0)) {
> > 		if (chip->info->family == MV88E6XXX_FAMILY_6341)
> > 			val |= MV88E6XXX_PORT_SWITCH_ID_PROD_6341 >> 4;
> > 		else if (chip->info->family == MV88E6XXX_FAMILY_6390)
> > 			val |= MV88E6XXX_PORT_SWITCH_ID_PROD_6390 >> 4;
> > 	}
> 
> As i said, i expect the 6393 also has no ID. And i recently found out
> Marvell have some automotive switches, 88Q5xxx which are actually
> based around the same IP and could be added to this driver. They also
> might not have an ID. I suspect this list is going to get longer, so
> having it table driven will make that simpler, less error prone.
> 
>      Andrew

Ok, I will use table but I fill it only with Topaz (6341) and Peridot
(6390) which was there before as I do not have 6393 switch for testing.

If you or anybody else has 6393 unit for testing, please extend then
table.

Re: [PATCH] net: phy: marvell: fix detection of PHY on Topaz switches

[Andrew Lunn]

On Mon, Apr 12, 2021 at 05:52:39PM +0200, Pali Rohár wrote:
> On Monday 12 April 2021 17:32:33 Andrew Lunn wrote:
> > > Anyway, now I'm looking at phy/marvell.c driver again and it supports
> > > only 88E6341 and 88E6390 families from whole 88E63xxx range.
> > > 
> > > So do we need to define for now table for more than
> > > MV88E6XXX_FAMILY_6341 and MV88E6XXX_FAMILY_6390 entries?
> > 
> > Probably not. I've no idea if the 6393 has an ID, so to be safe you
> > should add that. Assuming it has a family of its own.
> 
> So what about just?
> 
> 	if (reg == MII_PHYSID2 && !(val & 0x3f0)) {
> 		if (chip->info->family == MV88E6XXX_FAMILY_6341)
> 			val |= MV88E6XXX_PORT_SWITCH_ID_PROD_6341 >> 4;
> 		else if (chip->info->family == MV88E6XXX_FAMILY_6390)
> 			val |= MV88E6XXX_PORT_SWITCH_ID_PROD_6390 >> 4;
> 	}

As i said, i expect the 6393 also has no ID. And i recently found out
Marvell have some automotive switches, 88Q5xxx which are actually
based around the same IP and could be added to this driver. They also
might not have an ID. I suspect this list is going to get longer, so
having it table driven will make that simpler, less error prone.

     Andrew

Re: [PATCH] net: phy: marvell: fix detection of PHY on Topaz switches

[Pali Rohár]

On Monday 12 April 2021 17:32:33 Andrew Lunn wrote:
> > Anyway, now I'm looking at phy/marvell.c driver again and it supports
> > only 88E6341 and 88E6390 families from whole 88E63xxx range.
> > 
> > So do we need to define for now table for more than
> > MV88E6XXX_FAMILY_6341 and MV88E6XXX_FAMILY_6390 entries?
> 
> Probably not. I've no idea if the 6393 has an ID, so to be safe you
> should add that. Assuming it has a family of its own.

So what about just?

	if (reg == MII_PHYSID2 && !(val & 0x3f0)) {
		if (chip->info->family == MV88E6XXX_FAMILY_6341)
			val |= MV88E6XXX_PORT_SWITCH_ID_PROD_6341 >> 4;
		else if (chip->info->family == MV88E6XXX_FAMILY_6390)
			val |= MV88E6XXX_PORT_SWITCH_ID_PROD_6390 >> 4;
	}

Re: [PATCH] net: phy: marvell: fix detection of PHY on Topaz switches

[Andrew Lunn]

> Anyway, now I'm looking at phy/marvell.c driver again and it supports
> only 88E6341 and 88E6390 families from whole 88E63xxx range.
> 
> So do we need to define for now table for more than
> MV88E6XXX_FAMILY_6341 and MV88E6XXX_FAMILY_6390 entries?

Probably not. I've no idea if the 6393 has an ID, so to be safe you
should add that. Assuming it has a family of its own.

       Andrew

Re: [PATCH] net: phy: marvell: fix detection of PHY on Topaz switches

[Pali Rohár]

On Monday 12 April 2021 16:44:11 Andrew Lunn wrote:
> On Mon, Apr 12, 2021 at 03:34:47PM +0200, Pali Rohár wrote:
> > On Monday 12 April 2021 15:15:07 Andrew Lunn wrote:
> > > > +static u16 mv88e6xxx_physid_for_family(enum mv88e6xxx_family family);
> > > > +
> > > 
> > > No forward declaration please. Move the code around. It is often best
> > > to do that in a patch which just moves code, no other changes. It
> > > makes it easier to review.
> > 
> > Avoiding forward declaration would mean to move about half of source
> > code. mv88e6xxx_physid_for_family depends on mv88e6xxx_table which
> > depends on all _ops structures which depends on all lot of other
> > functions.
> 
> So this is basically what you are trying to do:
> 
> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
> index 903d619e08ed..ef4dbcb052b7 100644
> --- a/drivers/net/dsa/mv88e6xxx/chip.c
> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> @@ -3026,6 +3026,18 @@ static int mv88e6xxx_setup(struct dsa_switch *ds)
>         return err;
>  }
>  
> +static const enum mv88e6xxx_model family_model_table[] = {
> +       [MV88E6XXX_FAMILY_6095] = MV88E6XXX_PORT_SWITCH_ID_PROD_6095,
> +       [MV88E6XXX_FAMILY_6097] = MV88E6XXX_PORT_SWITCH_ID_PROD_6097,
> +       [MV88E6XXX_FAMILY_6185] = MV88E6XXX_PORT_SWITCH_ID_PROD_6185,
> +       [MV88E6XXX_FAMILY_6250] = MV88E6XXX_PORT_SWITCH_ID_PROD_6250,
> +       [MV88E6XXX_FAMILY_6320] = MV88E6XXX_PORT_SWITCH_ID_PROD_6320,
> +       [MV88E6XXX_FAMILY_6341] = MV88E6XXX_PORT_SWITCH_ID_PROD_6341,
> +       [MV88E6XXX_FAMILY_6351] = MV88E6XXX_PORT_SWITCH_ID_PROD_6351,
> +       [MV88E6XXX_FAMILY_6352] = MV88E6XXX_PORT_SWITCH_ID_PROD_6352,
> +       [MV88E6XXX_FAMILY_6390] = MV88E6XXX_PORT_SWITCH_ID_PROD_6390,
> +};

Ok, no problem. I can change it in this way. I just thought that if
prod_id is already defined for every model in mv88e6xxx_table[] table I
could reuse it, instead of duplicating it...

Anyway, now I'm looking at phy/marvell.c driver again and it supports
only 88E6341 and 88E6390 families from whole 88E63xxx range.

So do we need to define for now table for more than
MV88E6XXX_FAMILY_6341 and MV88E6XXX_FAMILY_6390 entries?

> +
>  static int mv88e6xxx_mdio_read(struct mii_bus *bus, int phy, int reg)
>  {
>         struct mv88e6xxx_mdio_bus *mdio_bus = bus->priv;
> @@ -3056,7 +3068,7 @@ static int mv88e6xxx_mdio_read(struct mii_bus *bus, int phy, int reg)
>                          * a PHY,
>                          */
>                         if (!(val & 0x3f0))
> -                               val |= MV88E6XXX_PORT_SWITCH_ID_PROD_6390 >> 4;
> +                               val |= family_model_table[chip->info->family] >> 4;
>         }
> 
> and it compiles. No forward declarations needed. It is missing all the
> error checking etc, but i don't see why that should change the
> dependencies.
> 
> 	Andrew

Re: [PATCH] net: phy: marvell: fix detection of PHY on Topaz switches

[Andrew Lunn]

On Mon, Apr 12, 2021 at 03:34:47PM +0200, Pali Rohár wrote:
> On Monday 12 April 2021 15:15:07 Andrew Lunn wrote:
> > > +static u16 mv88e6xxx_physid_for_family(enum mv88e6xxx_family family);
> > > +
> > 
> > No forward declaration please. Move the code around. It is often best
> > to do that in a patch which just moves code, no other changes. It
> > makes it easier to review.
> 
> Avoiding forward declaration would mean to move about half of source
> code. mv88e6xxx_physid_for_family depends on mv88e6xxx_table which
> depends on all _ops structures which depends on all lot of other
> functions.

So this is basically what you are trying to do:

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 903d619e08ed..ef4dbcb052b7 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -3026,6 +3026,18 @@ static int mv88e6xxx_setup(struct dsa_switch *ds)
        return err;
 }
 
+static const enum mv88e6xxx_model family_model_table[] = {
+       [MV88E6XXX_FAMILY_6095] = MV88E6XXX_PORT_SWITCH_ID_PROD_6095,
+       [MV88E6XXX_FAMILY_6097] = MV88E6XXX_PORT_SWITCH_ID_PROD_6097,
+       [MV88E6XXX_FAMILY_6185] = MV88E6XXX_PORT_SWITCH_ID_PROD_6185,
+       [MV88E6XXX_FAMILY_6250] = MV88E6XXX_PORT_SWITCH_ID_PROD_6250,
+       [MV88E6XXX_FAMILY_6320] = MV88E6XXX_PORT_SWITCH_ID_PROD_6320,
+       [MV88E6XXX_FAMILY_6341] = MV88E6XXX_PORT_SWITCH_ID_PROD_6341,
+       [MV88E6XXX_FAMILY_6351] = MV88E6XXX_PORT_SWITCH_ID_PROD_6351,
+       [MV88E6XXX_FAMILY_6352] = MV88E6XXX_PORT_SWITCH_ID_PROD_6352,
+       [MV88E6XXX_FAMILY_6390] = MV88E6XXX_PORT_SWITCH_ID_PROD_6390,
+};
+
 static int mv88e6xxx_mdio_read(struct mii_bus *bus, int phy, int reg)
 {
        struct mv88e6xxx_mdio_bus *mdio_bus = bus->priv;
@@ -3056,7 +3068,7 @@ static int mv88e6xxx_mdio_read(struct mii_bus *bus, int phy, int reg)
                         * a PHY,
                         */
                        if (!(val & 0x3f0))
-                               val |= MV88E6XXX_PORT_SWITCH_ID_PROD_6390 >> 4;
+                               val |= family_model_table[chip->info->family] >> 4;
        }

and it compiles. No forward declarations needed. It is missing all the
error checking etc, but i don't see why that should change the
dependencies.

	Andrew

Re: [PATCH] net: phy: marvell: fix detection of PHY on Topaz switches

[Pali Rohár]

On Monday 12 April 2021 16:30:03 Andrew Lunn wrote:
> > > > +/* This table contains representative model for every family */
> > > > +static const enum mv88e6xxx_model family_model_table[] = {
> > > > +	[MV88E6XXX_FAMILY_6095] = MV88E6095,
> > > > +	[MV88E6XXX_FAMILY_6097] = MV88E6097,
> > > > +	[MV88E6XXX_FAMILY_6185] = MV88E6185,
> > > > +	[MV88E6XXX_FAMILY_6250] = MV88E6250,
> > > > +	[MV88E6XXX_FAMILY_6320] = MV88E6320,
> > > > +	[MV88E6XXX_FAMILY_6341] = MV88E6341,
> > > > +	[MV88E6XXX_FAMILY_6351] = MV88E6351,
> > > > +	[MV88E6XXX_FAMILY_6352] = MV88E6352,
> > > > +	[MV88E6XXX_FAMILY_6390] = MV88E6390,
> > > > +};
> > > 
> > > This table is wrong. MV88E6390 does not equal
> > > MV88E6XXX_PORT_SWITCH_ID_PROD_6390. MV88E6XXX_PORT_SWITCH_ID_PROD_6390
> > > was chosen because it is already an MDIO device ID, in register 2 and
> > > 3. It probably will never clash with a real Marvell PHY ID. MV88E6390
> > > is just a small integer, and there is a danger it will clash with a
> > > real PHY.
> > 
> > So... how to solve this issue? What should be in the mapping table?
> 
> You need to use MV88E6XXX_PORT_SWITCH_ID_PROD_6095,
> MV88E6XXX_PORT_SWITCH_ID_PROD_6097,
> ...
> MV88E6XXX_PORT_SWITCH_ID_PROD_6390,

But I'm using it.

First chip->info->family (enum mv88e6xxx_family; MV88E6XXX_FAMILY_6341)
is mapped to enum mv88e6xxx_model (MV88E6341) via family_model_table[]
and then enum mv88e6xxx_model (MV88E6341) is mapped to prod_num
(MV88E6XXX_PORT_SWITCH_ID_PROD_6341) via mv88e6xxx_table[].

All this is done in mv88e6xxx_physid_for_family() function.

So at the end, this function converts MV88E6XXX_FAMILY_6341 to
MV88E6XXX_PORT_SWITCH_ID_PROD_6341.

And therefore I do not see anything wrong in family_model_table[] table.

I defined family_model_table[] table to just maps enum mv88e6xxx_family
to enum mv88e6xxx_model as mv88e6xxx_table[] table already contains
mapping from enum mv88e6xxx_model to phys_id, to simplify
implementation.

Re: [PATCH] net: phy: marvell: fix detection of PHY on Topaz switches

[Andrew Lunn]

> > > +/* This table contains representative model for every family */
> > > +static const enum mv88e6xxx_model family_model_table[] = {
> > > +	[MV88E6XXX_FAMILY_6095] = MV88E6095,
> > > +	[MV88E6XXX_FAMILY_6097] = MV88E6097,
> > > +	[MV88E6XXX_FAMILY_6185] = MV88E6185,
> > > +	[MV88E6XXX_FAMILY_6250] = MV88E6250,
> > > +	[MV88E6XXX_FAMILY_6320] = MV88E6320,
> > > +	[MV88E6XXX_FAMILY_6341] = MV88E6341,
> > > +	[MV88E6XXX_FAMILY_6351] = MV88E6351,
> > > +	[MV88E6XXX_FAMILY_6352] = MV88E6352,
> > > +	[MV88E6XXX_FAMILY_6390] = MV88E6390,
> > > +};
> > 
> > This table is wrong. MV88E6390 does not equal
> > MV88E6XXX_PORT_SWITCH_ID_PROD_6390. MV88E6XXX_PORT_SWITCH_ID_PROD_6390
> > was chosen because it is already an MDIO device ID, in register 2 and
> > 3. It probably will never clash with a real Marvell PHY ID. MV88E6390
> > is just a small integer, and there is a danger it will clash with a
> > real PHY.
> 
> So... how to solve this issue? What should be in the mapping table?

You need to use MV88E6XXX_PORT_SWITCH_ID_PROD_6095,
MV88E6XXX_PORT_SWITCH_ID_PROD_6097,
...
MV88E6XXX_PORT_SWITCH_ID_PROD_6390,

> > You cannot just replace the MARVELL_PHY_ID_88E6390. That will break
> > the 6390! You need to add the new PHY for the 88E6341.
> 
> I have not replaced anything.

Yes, sorry. I read the diff wrong.

     Andrew

Re: [PATCH] net: phy: marvell: fix detection of PHY on Topaz switches

[Pali Rohár]

On Monday 12 April 2021 15:15:07 Andrew Lunn wrote:
> > +static u16 mv88e6xxx_physid_for_family(enum mv88e6xxx_family family);
> > +
> 
> No forward declaration please. Move the code around. It is often best
> to do that in a patch which just moves code, no other changes. It
> makes it easier to review.

Avoiding forward declaration would mean to move about half of source
code. mv88e6xxx_physid_for_family depends on mv88e6xxx_table which
depends on all _ops structures which depends on all lot of other
functions.

I wanted to create a small fixup patch which can be easily backported to
stable releases which are affected by this issue.

If you do not like forward declarations, I can create a followup patch
which moves this half of code in this file to avoid forward declaration.
But having it in one patch would mean to complicate reviewing code...

> >  static int mv88e6xxx_mdio_read(struct mii_bus *bus, int phy, int reg)
> >  {
> >  	struct mv88e6xxx_mdio_bus *mdio_bus = bus->priv;
> > @@ -3040,24 +3042,9 @@ static int mv88e6xxx_mdio_read(struct mii_bus *bus, int phy, int reg)
> >  	err = chip->info->ops->phy_read(chip, bus, phy, reg, &val);
> >  	mv88e6xxx_reg_unlock(chip);
> >  
> > -	if (reg == MII_PHYSID2) {
> > -		/* Some internal PHYs don't have a model number. */
> > -		if (chip->info->family != MV88E6XXX_FAMILY_6165)
> > -			/* Then there is the 6165 family. It gets is
> > -			 * PHYs correct. But it can also have two
> > -			 * SERDES interfaces in the PHY address
> > -			 * space. And these don't have a model
> > -			 * number. But they are not PHYs, so we don't
> > -			 * want to give them something a PHY driver
> > -			 * will recognise.
> > -			 *
> > -			 * Use the mv88e6390 family model number
> > -			 * instead, for anything which really could be
> > -			 * a PHY,
> > -			 */
> > -			if (!(val & 0x3f0))
> > -				val |= MV88E6XXX_PORT_SWITCH_ID_PROD_6390 >> 4;
> > -	}
> > +	/* Some internal PHYs don't have a model number. */
> > +	if (reg == MII_PHYSID2 && !(val & 0x3f0))
> > +		val |= mv88e6xxx_physid_for_family(chip->info->family);
> >  
> >  	return err ? err : val;
> >  }
> > @@ -5244,6 +5231,39 @@ static const struct mv88e6xxx_info *mv88e6xxx_lookup_info(unsigned int prod_num)
> >  	return NULL;
> >  }
> >  
> > +/* This table contains representative model for every family */
> > +static const enum mv88e6xxx_model family_model_table[] = {
> > +	[MV88E6XXX_FAMILY_6095] = MV88E6095,
> > +	[MV88E6XXX_FAMILY_6097] = MV88E6097,
> > +	[MV88E6XXX_FAMILY_6185] = MV88E6185,
> > +	[MV88E6XXX_FAMILY_6250] = MV88E6250,
> > +	[MV88E6XXX_FAMILY_6320] = MV88E6320,
> > +	[MV88E6XXX_FAMILY_6341] = MV88E6341,
> > +	[MV88E6XXX_FAMILY_6351] = MV88E6351,
> > +	[MV88E6XXX_FAMILY_6352] = MV88E6352,
> > +	[MV88E6XXX_FAMILY_6390] = MV88E6390,
> > +};
> 
> This table is wrong. MV88E6390 does not equal
> MV88E6XXX_PORT_SWITCH_ID_PROD_6390. MV88E6XXX_PORT_SWITCH_ID_PROD_6390
> was chosen because it is already an MDIO device ID, in register 2 and
> 3. It probably will never clash with a real Marvell PHY ID. MV88E6390
> is just a small integer, and there is a danger it will clash with a
> real PHY.

So... how to solve this issue? What should be in the mapping table?

> > --- a/drivers/net/phy/marvell.c
> > +++ b/drivers/net/phy/marvell.c
> > @@ -3021,9 +3021,34 @@ static struct phy_driver marvell_drivers[] = {
> >  		.get_stats = marvell_get_stats,
> >  	},
> >  	{
> > -		.phy_id = MARVELL_PHY_ID_88E6390,
> > +		.phy_id = MARVELL_PHY_ID_88E6341_FAMILY,
> >  		.phy_id_mask = MARVELL_PHY_ID_MASK,
> > -		.name = "Marvell 88E6390",
> > +		.name = "Marvell 88E6341 Family",
> 
> You cannot just replace the MARVELL_PHY_ID_88E6390. That will break
> the 6390! You need to add the new PHY for the 88E6341.

I have not replaced anything. I just put there a new phy_id section for
88E6341. You are probably confused by 'git diff' output as quickly
looking at it, somebody can think that there is phy replacement. But
there is no replacement, I only added a new PHY. Entry for 88E6390 is
still there!

Also this is reason why I wanted to avoid big code movement. It will be
harder to read the 'git diff' output in this patch.

Re: [PATCH] net: phy: marvell: fix detection of PHY on Topaz switches

[Andrew Lunn]

> +static u16 mv88e6xxx_physid_for_family(enum mv88e6xxx_family family);
> +

No forward declaration please. Move the code around. It is often best
to do that in a patch which just moves code, no other changes. It
makes it easier to review.

>  static int mv88e6xxx_mdio_read(struct mii_bus *bus, int phy, int reg)
>  {
>  	struct mv88e6xxx_mdio_bus *mdio_bus = bus->priv;
> @@ -3040,24 +3042,9 @@ static int mv88e6xxx_mdio_read(struct mii_bus *bus, int phy, int reg)
>  	err = chip->info->ops->phy_read(chip, bus, phy, reg, &val);
>  	mv88e6xxx_reg_unlock(chip);
>  
> -	if (reg == MII_PHYSID2) {
> -		/* Some internal PHYs don't have a model number. */
> -		if (chip->info->family != MV88E6XXX_FAMILY_6165)
> -			/* Then there is the 6165 family. It gets is
> -			 * PHYs correct. But it can also have two
> -			 * SERDES interfaces in the PHY address
> -			 * space. And these don't have a model
> -			 * number. But they are not PHYs, so we don't
> -			 * want to give them something a PHY driver
> -			 * will recognise.
> -			 *
> -			 * Use the mv88e6390 family model number
> -			 * instead, for anything which really could be
> -			 * a PHY,
> -			 */
> -			if (!(val & 0x3f0))
> -				val |= MV88E6XXX_PORT_SWITCH_ID_PROD_6390 >> 4;
> -	}
> +	/* Some internal PHYs don't have a model number. */
> +	if (reg == MII_PHYSID2 && !(val & 0x3f0))
> +		val |= mv88e6xxx_physid_for_family(chip->info->family);
>  
>  	return err ? err : val;
>  }
> @@ -5244,6 +5231,39 @@ static const struct mv88e6xxx_info *mv88e6xxx_lookup_info(unsigned int prod_num)
>  	return NULL;
>  }
>  
> +/* This table contains representative model for every family */
> +static const enum mv88e6xxx_model family_model_table[] = {
> +	[MV88E6XXX_FAMILY_6095] = MV88E6095,
> +	[MV88E6XXX_FAMILY_6097] = MV88E6097,
> +	[MV88E6XXX_FAMILY_6185] = MV88E6185,
> +	[MV88E6XXX_FAMILY_6250] = MV88E6250,
> +	[MV88E6XXX_FAMILY_6320] = MV88E6320,
> +	[MV88E6XXX_FAMILY_6341] = MV88E6341,
> +	[MV88E6XXX_FAMILY_6351] = MV88E6351,
> +	[MV88E6XXX_FAMILY_6352] = MV88E6352,
> +	[MV88E6XXX_FAMILY_6390] = MV88E6390,
> +};

This table is wrong. MV88E6390 does not equal
MV88E6XXX_PORT_SWITCH_ID_PROD_6390. MV88E6XXX_PORT_SWITCH_ID_PROD_6390
was chosen because it is already an MDIO device ID, in register 2 and
3. It probably will never clash with a real Marvell PHY ID. MV88E6390
is just a small integer, and there is a danger it will clash with a
real PHY.

> --- a/drivers/net/phy/marvell.c
> +++ b/drivers/net/phy/marvell.c
> @@ -3021,9 +3021,34 @@ static struct phy_driver marvell_drivers[] = {
>  		.get_stats = marvell_get_stats,
>  	},
>  	{
> -		.phy_id = MARVELL_PHY_ID_88E6390,
> +		.phy_id = MARVELL_PHY_ID_88E6341_FAMILY,
>  		.phy_id_mask = MARVELL_PHY_ID_MASK,
> -		.name = "Marvell 88E6390",
> +		.name = "Marvell 88E6341 Family",

You cannot just replace the MARVELL_PHY_ID_88E6390. That will break
the 6390! You need to add the new PHY for the 88E6341.

    Andrew

[PATCH] net: phy: marvell: fix detection of PHY on Topaz switches

[Pali Rohár]

Since commit fee2d546414d ("net: phy: marvell: mv88e6390 temperature
sensor reading"), Linux reports the temperature of Topaz hwmon as
constant -75°C.

This is because switches from the Topaz family (88E6141 / 88E6341) have
the address of the temperature sensor register different from Peridot.

This address is instead compatible with 88E1510 PHYs, as was used for
Topaz before the above mentioned commit.

Define a representative model for each switch family and add a mapping
table for constructing PHY IDs for the internal PHYs (since they don't
have a model number).

Create a new PHY ID and a new PHY driver for Topaz' internal PHY.
The only difference from Peridot's PHY driver is the HWMON probing
method.

Prior this change Topaz's internal PHY is detected by kernel as:

  PHY [...] driver [Marvell 88E6390] (irq=63)

And afterwards as:

  PHY [...] driver [Marvell 88E6341 Family] (irq=63)

Signed-off-by: Pali Rohár <pali@kernel.org>
BugLink: https://github.com/globalscaletechnologies/linux/issues/1
Fixes: fee2d546414d ("net: phy: marvell: mv88e6390 temperature sensor reading")
Reviewed-by: Marek Behún <kabel@kernel.org>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 56 ++++++++++++++++++++++----------
 drivers/net/dsa/mv88e6xxx/chip.h |  2 ++
 drivers/net/phy/marvell.c        | 32 ++++++++++++++++--
 include/linux/marvell_phy.h      |  5 +--
 4 files changed, 72 insertions(+), 23 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 903d619e08ed..e602c9816aee 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -3026,6 +3026,8 @@ static int mv88e6xxx_setup(struct dsa_switch *ds)
 	return err;
 }
 
+static u16 mv88e6xxx_physid_for_family(enum mv88e6xxx_family family);
+
 static int mv88e6xxx_mdio_read(struct mii_bus *bus, int phy, int reg)
 {
 	struct mv88e6xxx_mdio_bus *mdio_bus = bus->priv;
@@ -3040,24 +3042,9 @@ static int mv88e6xxx_mdio_read(struct mii_bus *bus, int phy, int reg)
 	err = chip->info->ops->phy_read(chip, bus, phy, reg, &val);
 	mv88e6xxx_reg_unlock(chip);
 
-	if (reg == MII_PHYSID2) {
-		/* Some internal PHYs don't have a model number. */
-		if (chip->info->family != MV88E6XXX_FAMILY_6165)
-			/* Then there is the 6165 family. It gets is
-			 * PHYs correct. But it can also have two
-			 * SERDES interfaces in the PHY address
-			 * space. And these don't have a model
-			 * number. But they are not PHYs, so we don't
-			 * want to give them something a PHY driver
-			 * will recognise.
-			 *
-			 * Use the mv88e6390 family model number
-			 * instead, for anything which really could be
-			 * a PHY,
-			 */
-			if (!(val & 0x3f0))
-				val |= MV88E6XXX_PORT_SWITCH_ID_PROD_6390 >> 4;
-	}
+	/* Some internal PHYs don't have a model number. */
+	if (reg == MII_PHYSID2 && !(val & 0x3f0))
+		val |= mv88e6xxx_physid_for_family(chip->info->family);
 
 	return err ? err : val;
 }
@@ -5244,6 +5231,39 @@ static const struct mv88e6xxx_info *mv88e6xxx_lookup_info(unsigned int prod_num)
 	return NULL;
 }
 
+/* This table contains representative model for every family */
+static const enum mv88e6xxx_model family_model_table[] = {
+	[MV88E6XXX_FAMILY_6095] = MV88E6095,
+	[MV88E6XXX_FAMILY_6097] = MV88E6097,
+	[MV88E6XXX_FAMILY_6185] = MV88E6185,
+	[MV88E6XXX_FAMILY_6250] = MV88E6250,
+	[MV88E6XXX_FAMILY_6320] = MV88E6320,
+	[MV88E6XXX_FAMILY_6341] = MV88E6341,
+	[MV88E6XXX_FAMILY_6351] = MV88E6351,
+	[MV88E6XXX_FAMILY_6352] = MV88E6352,
+	[MV88E6XXX_FAMILY_6390] = MV88E6390,
+};
+static_assert(ARRAY_SIZE(family_model_table) == MV88E6XXX_FAMILY_LAST);
+
+static u16 mv88e6xxx_physid_for_family(enum mv88e6xxx_family family)
+{
+	enum mv88e6xxx_model model;
+
+	/* 6165 family gets it's PHYs correct. But it can also have two SERDES
+	 * interfaces in the PHY address space. And these don't have a model
+	 * number. But they are not PHYs, so we don't want to give them
+	 * something a PHY driver will recognise.
+	 */
+	if (family == MV88E6XXX_FAMILY_6165)
+		return 0;
+
+	model = family_model_table[family];
+	if (model == MV88E_NA)
+		return 0;
+
+	return mv88e6xxx_table[model].prod_num >> 4;
+}
+
 static int mv88e6xxx_detect(struct mv88e6xxx_chip *chip)
 {
 	const struct mv88e6xxx_info *info;
diff --git a/drivers/net/dsa/mv88e6xxx/chip.h b/drivers/net/dsa/mv88e6xxx/chip.h
index a57c8886f3ac..70c736788a68 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.h
+++ b/drivers/net/dsa/mv88e6xxx/chip.h
@@ -47,6 +47,7 @@ enum mv88e6xxx_frame_mode {
 
 /* List of supported models */
 enum mv88e6xxx_model {
+	MV88E_NA = 0, /* must be zero */
 	MV88E6085,
 	MV88E6095,
 	MV88E6097,
@@ -90,6 +91,7 @@ enum mv88e6xxx_family {
 	MV88E6XXX_FAMILY_6351,	/* 6171 6175 6350 6351 */
 	MV88E6XXX_FAMILY_6352,	/* 6172 6176 6240 6352 */
 	MV88E6XXX_FAMILY_6390,  /* 6190 6190X 6191 6290 6390 6390X */
+	MV88E6XXX_FAMILY_LAST
 };
 
 struct mv88e6xxx_ops;
diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index e26a5d663f8a..8018ddf7f316 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -3021,9 +3021,34 @@ static struct phy_driver marvell_drivers[] = {
 		.get_stats = marvell_get_stats,
 	},
 	{
-		.phy_id = MARVELL_PHY_ID_88E6390,
+		.phy_id = MARVELL_PHY_ID_88E6341_FAMILY,
 		.phy_id_mask = MARVELL_PHY_ID_MASK,
-		.name = "Marvell 88E6390",
+		.name = "Marvell 88E6341 Family",
+		/* PHY_GBIT_FEATURES */
+		.flags = PHY_POLL_CABLE_TEST,
+		.probe = m88e1510_probe,
+		.config_init = marvell_config_init,
+		.config_aneg = m88e6390_config_aneg,
+		.read_status = marvell_read_status,
+		.config_intr = marvell_config_intr,
+		.handle_interrupt = marvell_handle_interrupt,
+		.resume = genphy_resume,
+		.suspend = genphy_suspend,
+		.read_page = marvell_read_page,
+		.write_page = marvell_write_page,
+		.get_sset_count = marvell_get_sset_count,
+		.get_strings = marvell_get_strings,
+		.get_stats = marvell_get_stats,
+		.get_tunable = m88e1540_get_tunable,
+		.set_tunable = m88e1540_set_tunable,
+		.cable_test_start = marvell_vct7_cable_test_start,
+		.cable_test_tdr_start = marvell_vct5_cable_test_tdr_start,
+		.cable_test_get_status = marvell_vct7_cable_test_get_status,
+	},
+	{
+		.phy_id = MARVELL_PHY_ID_88E6390_FAMILY,
+		.phy_id_mask = MARVELL_PHY_ID_MASK,
+		.name = "Marvell 88E6390 Family",
 		/* PHY_GBIT_FEATURES */
 		.flags = PHY_POLL_CABLE_TEST,
 		.probe = m88e6390_probe,
@@ -3107,7 +3132,8 @@ static struct mdio_device_id __maybe_unused marvell_tbl[] = {
 	{ MARVELL_PHY_ID_88E1540, MARVELL_PHY_ID_MASK },
 	{ MARVELL_PHY_ID_88E1545, MARVELL_PHY_ID_MASK },
 	{ MARVELL_PHY_ID_88E3016, MARVELL_PHY_ID_MASK },
-	{ MARVELL_PHY_ID_88E6390, MARVELL_PHY_ID_MASK },
+	{ MARVELL_PHY_ID_88E6341_FAMILY, MARVELL_PHY_ID_MASK },
+	{ MARVELL_PHY_ID_88E6390_FAMILY, MARVELL_PHY_ID_MASK },
 	{ MARVELL_PHY_ID_88E1340S, MARVELL_PHY_ID_MASK },
 	{ MARVELL_PHY_ID_88E1548P, MARVELL_PHY_ID_MASK },
 	{ }
diff --git a/include/linux/marvell_phy.h b/include/linux/marvell_phy.h
index 52b1610eae68..c544b70dfbd2 100644
--- a/include/linux/marvell_phy.h
+++ b/include/linux/marvell_phy.h
@@ -28,11 +28,12 @@
 /* Marvel 88E1111 in Finisar SFP module with modified PHY ID */
 #define MARVELL_PHY_ID_88E1111_FINISAR	0x01ff0cc0
 
-/* The MV88e6390 Ethernet switch contains embedded PHYs. These PHYs do
+/* These Ethernet switch families contain embedded PHYs, but they do
  * not have a model ID. So the switch driver traps reads to the ID2
  * register and returns the switch family ID
  */
-#define MARVELL_PHY_ID_88E6390		0x01410f90
+#define MARVELL_PHY_ID_88E6341_FAMILY	0x01410f41
+#define MARVELL_PHY_ID_88E6390_FAMILY	0x01410f90
 
 #define MARVELL_PHY_FAMILY_ID(id)	((id) >> 4)
 
-- 
2.20.1