(Sorry this is my second attempt, I fixed my email client)

I was trying to enable external SMI on a mv88e6176.
mv88e6390_g2_scratch_gpio_set_smi() would return -EBUSY when checking
the scratch register Config DATA2 p0 mode to ensure that the external
smi pins are free, but on my device, bit 4 of p0_mode was always 1,
even if the port was completely disabled.

Unless someone with the datasheet can prove me wrong, I believe that
at least for the 6176, the mode mask here is 3 bits wide, and the
fourth bit is irrelevant or reserved.

To fix this, I add a field in mv88e6xxx_info to denote a
p0_mode_mask_override to use. If this is set to the default
value of 0, then the definition of
MV88E6352_G2_SCRATCH_CONFIG_DATA2_P0_MODE_MASK will be used as normal.

I can confirm that this allows me to use external smi on my mv88e6176!

Fixes: 2510babcfaf0 ("net: dsa: mv88e6xxx: scratch registers and external MDIO pins")
Signed-off-by: Robert Cross <quantumcross@gmail.com>
---
 drivers/net/dsa/mv88e6xxx/chip.c            | 1 +
 drivers/net/dsa/mv88e6xxx/chip.h            | 7 +++++++
 drivers/net/dsa/mv88e6xxx/global2_scratch.c | 7 ++++++-
 3 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 2281d6ab8c9ab..0fe7b6fc7016c 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -6013,6 +6013,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.multi_chip = true,
 		.edsa_support = MV88E6XXX_EDSA_SUPPORTED,
 		.ops = &mv88e6176_ops,
+		.p0_mode_mask_override = 0x7,
 	},
 
 	[MV88E6185] = {
diff --git a/drivers/net/dsa/mv88e6xxx/chip.h b/drivers/net/dsa/mv88e6xxx/chip.h
index 7d00482f53a3b..6307c225ce94c 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.h
+++ b/drivers/net/dsa/mv88e6xxx/chip.h
@@ -178,6 +178,13 @@ struct mv88e6xxx_info {
 	 * port 0, 1 means internal PHYs range starts at port 1, etc
 	 */
 	unsigned int internal_phys_offset;
+
+	/* Some chips use 3 bits for the port mode in scratch
+         * register CONFIG Data2.
+         * If this is set to 0x0, it will use the default mask of
+         * MV88E6352_G2_SCRATCH_CONFIG_DATA2_P0_MODE_MASK
+         */
+	u8 p0_mode_mask_override;
 };
 
 struct mv88e6xxx_atu_entry {
diff --git a/drivers/net/dsa/mv88e6xxx/global2_scratch.c b/drivers/net/dsa/mv88e6xxx/global2_scratch.c
index 53a6d3ed63b32..7f1657eba7a4e 100644
--- a/drivers/net/dsa/mv88e6xxx/global2_scratch.c
+++ b/drivers/net/dsa/mv88e6xxx/global2_scratch.c
@@ -255,6 +255,7 @@ int mv88e6390_g2_scratch_gpio_set_smi(struct mv88e6xxx_chip *chip,
 	int config_data1 = MV88E6352_G2_SCRATCH_CONFIG_DATA1;
 	int config_data2 = MV88E6352_G2_SCRATCH_CONFIG_DATA2;
 	bool no_cpu;
+	u8 p0_mode_mask;
 	u8 p0_mode;
 	int err;
 	u8 val;
@@ -263,7 +264,11 @@ int mv88e6390_g2_scratch_gpio_set_smi(struct mv88e6xxx_chip *chip,
 	if (err)
 		return err;
 
-	p0_mode = val & MV88E6352_G2_SCRATCH_CONFIG_DATA2_P0_MODE_MASK;
+	p0_mode_mask = chip->info->p0_mode_mask_override;
+	if( p0_mode_mask == 0x0) {
+		p0_mode_mask = MV88E6352_G2_SCRATCH_CONFIG_DATA2_P0_MODE_MASK;
+	}
+	p0_mode = val & p0_mode_mask;
 
 	if (p0_mode == 0x01 || p0_mode == 0x02)
 		return -EBUSY;
-- 
2.39.5

On Mon, Jun 16, 2025 at 12:20:25PM -0400, Robert Cross wrote:
> (Sorry this is my second attempt, I fixed my email client)
> 
> I was trying to enable external SMI on a mv88e6176.

        MV88E6XXX_FAMILY_6352,  /* 6172 6176 6240 6352 */

So it is part of the 6352 family. That family does not have an
internal and external MDIO bus. It has a single bus which is both
internal and external.

It is only the 6390 family and above which has two MDIO busses.

    Andrew

---
pw-bot: cr

According to the documents I'm looking at, the 88E6172 and
88E6176 both have external MDIO buses. I have brought up
a board with two connected 88E6176 chips, each with a PHY
that can only be managed with the MDC/MDIO_PHY pins of
the 88E6176s.

After applying this patch I was able to successfully manage
and control these external PHYs without issue. I'm not sure
if you have access to the 88E6176 datasheet specifically,
but this chip absolutely does have an external MDIO.

(also, I fixed the problems with the checkpatch.pl, I will submit
the fixed version again tomorrow)

- Robert Cross

On Mon, Jun 16, 2025 at 1:55 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Mon, Jun 16, 2025 at 12:20:25PM -0400, Robert Cross wrote:
> > (Sorry this is my second attempt, I fixed my email client)
> >
> > I was trying to enable external SMI on a mv88e6176.
>
>         MV88E6XXX_FAMILY_6352,  /* 6172 6176 6240 6352 */
>
> So it is part of the 6352 family. That family does not have an
> internal and external MDIO bus. It has a single bus which is both
> internal and external.
>
> It is only the 6390 family and above which has two MDIO busses.
>
>     Andrew
>
> ---
> pw-bot: cr

On Mon, Jun 16, 2025 at 02:22:43PM -0400, Robert Cross wrote:
> According to the documents I'm looking at, the 88E6172 and
> 88E6176 both have external MDIO buses. I have brought up
> a board with two connected 88E6176 chips, each with a PHY
> that can only be managed with the MDC/MDIO_PHY pins of
> the 88E6176s.
> 
> After applying this patch I was able to successfully manage
> and control these external PHYs without issue. I'm not sure
> if you have access to the 88E6176 datasheet specifically,
> but this chip absolutely does have an external MDIO.

You are not understanding what i'm saying. This family has a single
MDIO bus controller. That controller is used by both the internal PHY
devices, plus there are two pins on the chip for external PHYs.

All the PHYs will appear on that one MDIO bus controller.

The MV88E6390_G2_SMI_PHY_CMD_FUNC_EXTERNAL bit is reserved on the 6352
family.

	Andrew

Hi Andrew,

On Mon, Jun 16, 2025 at 08:43:14PM +0200, Andrew Lunn wrote:
> On Mon, Jun 16, 2025 at 02:22:43PM -0400, Robert Cross wrote:
> > According to the documents I'm looking at, the 88E6172 and
> > 88E6176 both have external MDIO buses. I have brought up
> > a board with two connected 88E6176 chips, each with a PHY
> > that can only be managed with the MDC/MDIO_PHY pins of
> > the 88E6176s.
> > 
> > After applying this patch I was able to successfully manage
> > and control these external PHYs without issue. I'm not sure
> > if you have access to the 88E6176 datasheet specifically,
> > but this chip absolutely does have an external MDIO.
> 
> You are not understanding what i'm saying. This family has a single
> MDIO bus controller. That controller is used by both the internal PHY
> devices, plus there are two pins on the chip for external PHYs.
> 
> All the PHYs will appear on that one MDIO bus controller.
> 
> The MV88E6390_G2_SMI_PHY_CMD_FUNC_EXTERNAL bit is reserved on the 6352
> family.
> 
> 	Andrew

Is there any addition to Documentation/devicetree/bindings/net/dsa/marvell,mv88e6xxx.yaml
that we could make in order to clarify which switch families have a
combined internal+external MDIO bus and which ones have them separate?
As you're saying, this is an area where mistakes happen relatively
frequently.

> Is there any addition to Documentation/devicetree/bindings/net/dsa/marvell,mv88e6xxx.yaml
> that we could make in order to clarify which switch families have a
> combined internal+external MDIO bus and which ones have them separate?
> As you're saying, this is an area where mistakes happen relatively
> frequently.

It should be possible to add a constraint. Only compatible
"marvell,mv88e6190" can use "mdio-external"

	Andrew

Imagine that, it just works when I set it up like that...


And now I don't get concerning watchdog event messages probably from
putting the chip and driver in an undefined state!

[  145.839142] mv88e6085 gpio-0:04: Watchdog event: 0x003b

Thank you again!

> The MV88E6390_G2_SMI_PHY_CMD_FUNC_EXTERNAL bit is reserved on the 6352
> family.

Indeed it is...

> You are not understanding what i'm saying. This family has a single
> MDIO bus controller. That controller is used by both the internal PHY
> devices, plus there are two pins on the chip for external PHYs.
>
> All the PHYs will appear on that one MDIO bus controller.

So you're saying that if I removed my hack that apparently just sets
this reserved bit, and I take my PHY on port 6 and remove it from
the mdio_ext { compatible = "marvell,mv88e6xxx-mdio-external"; } entry
and put it in my mdio { } node it will direct requests to address 6 to
the external phy via the MDC/MDIO_PHY pins just fine?

I'm guessing it will just automatically enable or disable the external
SMI pins depending on the state of port 5 which shares pins?

I'm also guessing that ports 0, 1, 2, 3, and 4 will map to the
internal PHYs (because there are 5) and then ports 5 and 6
automagically externally...

I shudder to think how by what forbidden voodoo my current device
tree actually works with this hack...

Thank you so much for your explanation. Hopefully I'll have a
real substantive patch in the future :)

On Mon, Jun 16, 2025 at 03:12:14PM -0400, Robert Cross wrote:
> > The MV88E6390_G2_SMI_PHY_CMD_FUNC_EXTERNAL bit is reserved on the 6352
> > family.
> 
> Indeed it is...
> 
> > You are not understanding what i'm saying. This family has a single
> > MDIO bus controller. That controller is used by both the internal PHY
> > devices, plus there are two pins on the chip for external PHYs.
> >
> > All the PHYs will appear on that one MDIO bus controller.
> 
> So you're saying that if I removed my hack that apparently just sets
> this reserved bit, and I take my PHY on port 6 and remove it from
> the mdio_ext { compatible = "marvell,mv88e6xxx-mdio-external"; } entry
> and put it in my mdio { } node it will direct requests to address 6 to
> the external phy via the MDC/MDIO_PHY pins just fine?

Yep.

> I'm guessing it will just automatically enable or disable the external
> SMI pins depending on the state of port 5 which shares pins?

I _think_ you will actually see all MDIO transactions on the external
pins. SolidRun got one of their board designs wrong, and put an
external PHY on the same address as an internal PHY. That results in
both not working because they stomp over each other on the bus.

	Andrew