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

[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

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

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]

> > > +/* 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 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]

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: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]

> 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 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]

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 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

[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

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

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>
---
Changes since v1:
* create direct mapping table between family and prod_id which avoid need
  to use of forward declaration and simplify implementation
* fill mapping table only for Topaz and Peridot families
---
 drivers/net/dsa/mv88e6xxx/chip.c | 30 +++++++++++++-----------------
 drivers/net/phy/marvell.c        | 32 +++++++++++++++++++++++++++++---
 include/linux/marvell_phy.h      |  5 +++--
 3 files changed, 45 insertions(+), 22 deletions(-)

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)
 
-- 
2.20.1

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

[Andrew Lunn]

On Mon, Apr 12, 2021 at 06:57:39PM +0200, Pali Rohár wrote:
> 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>

    Andrew

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

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (refs/heads/master):

On Mon, 12 Apr 2021 18:57:39 +0200 you wrote:
> 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.
> 
> [...]

Here is the summary with links:
  - [v2] net: phy: marvell: fix detection of PHY on Topaz switches
    https://git.kernel.org/netdev/net/c/1fe976d308ac

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

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

[Marek Behún]

> +	/* 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;

Isn't if(prod_id) check redundant here? If prod_id is 0, the |=
statement won't do anything.

Marek