[PATCH] net: dsa: mv88e6xxx: Allow MAC configuration for ports with internal PHY

[PATCH] net: dsa: mv88e6xxx: Allow MAC configuration for ports with internal PHY

[Daniel Mack]

Ports with internal PHYs that are not in 'fixed-link' mode are currently
only set up once at startup with a static config. Attempts to change the
link speed or duplex settings are currently prevented by an early bail
in mv88e6xxx_mac_config(). As the default config forces the speed to
1000M, setups with reduced link speed on such ports are unsupported.

Change that, and allow the configuration of all ports with the passed
settings.

Signed-off-by: Daniel Mack <daniel@zonque.org>
---
Russell,

This changes the behaviour implemented in c9a2356f35409a ("net:
dsa: mv88e6xxx: add PHYLINK support"). Do you recall why your code
didn't touch the MLO_AN_PHY mode links in the first place?

 drivers/net/dsa/mv88e6xxx/chip.c | 30 ++++++++++++------------------
 1 file changed, 12 insertions(+), 18 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 2f993e673ec74..5452490dbe9d5 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -471,13 +471,6 @@ int mv88e6xxx_port_setup_mac(struct mv88e6xxx_chip *chip, int port, int link,
 	return err;
 }
 
-static int mv88e6xxx_phy_is_internal(struct dsa_switch *ds, int port)
-{
-	struct mv88e6xxx_chip *chip = ds->priv;
-
-	return port < chip->info->num_internal_phys;
-}
-
 static void mv88e6065_phylink_validate(struct mv88e6xxx_chip *chip, int port,
 				       unsigned long *mask,
 				       struct phylink_link_state *state)
@@ -605,23 +598,24 @@ static void mv88e6xxx_mac_config(struct dsa_switch *ds, int port,
 	struct mv88e6xxx_chip *chip = ds->priv;
 	int speed, duplex, link, pause, err;
 
-	if ((mode == MLO_AN_PHY) && mv88e6xxx_phy_is_internal(ds, port))
-		return;
+	speed = state->speed;
+	duplex = state->duplex;
+	pause = !!phylink_test(state->advertising, Pause);
 
-	if (mode == MLO_AN_FIXED) {
-		link = LINK_FORCED_UP;
-		speed = state->speed;
-		duplex = state->duplex;
-	} else if (!mv88e6xxx_phy_is_internal(ds, port)) {
+	switch (mode) {
+	case MLO_AN_PHY:
 		link = state->link;
-		speed = state->speed;
-		duplex = state->duplex;
-	} else {
+		break;
+
+	case MLO_AN_FIXED:
+		link = LINK_FORCED_UP;
+		break;
+
+	default:
 		speed = SPEED_UNFORCED;
 		duplex = DUPLEX_UNFORCED;
 		link = LINK_UNFORCED;
 	}
-	pause = !!phylink_test(state->advertising, Pause);
 
 	mv88e6xxx_reg_lock(chip);
 	err = mv88e6xxx_port_setup_mac(chip, port, link, speed, duplex, pause,
-- 
2.26.2

Re: [PATCH] net: dsa: mv88e6xxx: Allow MAC configuration for ports with internal PHY

[Andrew Lunn]

On Mon, Jun 22, 2020 at 08:34:43PM +0200, Daniel Mack wrote:
> Ports with internal PHYs that are not in 'fixed-link' mode are currently
> only set up once at startup with a static config. Attempts to change the
> link speed or duplex settings are currently prevented by an early bail
> in mv88e6xxx_mac_config(). As the default config forces the speed to
> 1000M, setups with reduced link speed on such ports are unsupported.

Hi Daniel

How are you trying to change the speed?

Thanks
	Andrew

Re: [PATCH] net: dsa: mv88e6xxx: Allow MAC configuration for ports with internal PHY

[Daniel Mack]

On 6/22/20 8:41 PM, Andrew Lunn wrote:
> On Mon, Jun 22, 2020 at 08:34:43PM +0200, Daniel Mack wrote:
>> Ports with internal PHYs that are not in 'fixed-link' mode are currently
>> only set up once at startup with a static config. Attempts to change the
>> link speed or duplex settings are currently prevented by an early bail
>> in mv88e6xxx_mac_config(). As the default config forces the speed to
>> 1000M, setups with reduced link speed on such ports are unsupported.
> 
> Hi Daniel
> 
> How are you trying to change the speed?

With ethtool for instance. But all userspace tools are bailing out early
on this port for the reason I described.


Thanks,
Daniel

Re: [PATCH] net: dsa: mv88e6xxx: Allow MAC configuration for ports with internal PHY

[Andrew Lunn]

On Mon, Jun 22, 2020 at 08:44:51PM +0200, Daniel Mack wrote:
> On 6/22/20 8:41 PM, Andrew Lunn wrote:
> > On Mon, Jun 22, 2020 at 08:34:43PM +0200, Daniel Mack wrote:
> >> Ports with internal PHYs that are not in 'fixed-link' mode are currently
> >> only set up once at startup with a static config. Attempts to change the
> >> link speed or duplex settings are currently prevented by an early bail
> >> in mv88e6xxx_mac_config(). As the default config forces the speed to
> >> 1000M, setups with reduced link speed on such ports are unsupported.
> > 
> > Hi Daniel
> > 
> > How are you trying to change the speed?
> 
> With ethtool for instance. But all userspace tools are bailing out early
> on this port for the reason I described.

So you mean ethtool -s devname advertise 0x001

set make it advertise 10BaseT Half?

    Andrew

Re: [PATCH] net: dsa: mv88e6xxx: Allow MAC configuration for ports with internal PHY

[Russell King - ARM Linux admin]

On Mon, Jun 22, 2020 at 08:34:43PM +0200, Daniel Mack wrote:
> Ports with internal PHYs that are not in 'fixed-link' mode are currently
> only set up once at startup with a static config. Attempts to change the
> link speed or duplex settings are currently prevented by an early bail
> in mv88e6xxx_mac_config(). As the default config forces the speed to
> 1000M, setups with reduced link speed on such ports are unsupported.
> 
> Change that, and allow the configuration of all ports with the passed
> settings.
> 
> Signed-off-by: Daniel Mack <daniel@zonque.org>
> ---
> Russell,
> 
> This changes the behaviour implemented in c9a2356f35409a ("net:
> dsa: mv88e6xxx: add PHYLINK support"). Do you recall why your code
> didn't touch the MLO_AN_PHY mode links in the first place?

The reason is that's how it was before - each port with a PHY has
PHY polling enabled, which means that the PPU (phy polling unit)
reads the PHY state and updates the port with that.  The port
follows whatever the state of the PHY, and that does not require
phylink to program the port.

I believe it to be incorrect to force ports where the PPU is
enabled, and since Marvell DSA switches have always had the PPU
enabled, it is incorrect to force these ports.

Note that there was no change in behaviour - the code was originally
doing this prior to phylink:

static void mv88e6xxx_adjust_link(struct dsa_switch *ds, int port,
                                  struct phy_device *phydev)
{
        struct mv88e6xxx_chip *chip = ds->priv;
        int err;

        if (!phy_is_pseudo_fixed_link(phydev))
                return;

        mutex_lock(&chip->reg_lock);
        err = mv88e6xxx_port_setup_mac(chip, port, phydev->link, phydev->speed,
                                       phydev->duplex, phydev->interface);
        mutex_unlock(&chip->reg_lock);

        if (err && err != -EOPNOTSUPP)
                dev_err(ds->dev, "p%d: failed to configure MAC\n", port);
}

So the internal PHYs (phys which are not a pseudo fixed-link) were
being ignored before my code.

As is normal for me, when ever I convert something from one thing to
another, I try to preserve as much of the original behaviour as
possible, and with regard to the issue that you raise, I preserved
the behaviour for the internal PHYs.

Hence, I believe your patch to be incorrect.

What problem are you seeing, and with which switch?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH] net: dsa: mv88e6xxx: Allow MAC configuration for ports with internal PHY

[Russell King - ARM Linux admin]

On Mon, Jun 22, 2020 at 08:44:51PM +0200, Daniel Mack wrote:
> On 6/22/20 8:41 PM, Andrew Lunn wrote:
> > On Mon, Jun 22, 2020 at 08:34:43PM +0200, Daniel Mack wrote:
> >> Ports with internal PHYs that are not in 'fixed-link' mode are currently
> >> only set up once at startup with a static config. Attempts to change the
> >> link speed or duplex settings are currently prevented by an early bail
> >> in mv88e6xxx_mac_config(). As the default config forces the speed to
> >> 1000M, setups with reduced link speed on such ports are unsupported.
> > 
> > Hi Daniel
> > 
> > How are you trying to change the speed?
> 
> With ethtool for instance. But all userspace tools are bailing out early
> on this port for the reason I described.

A simple "return" to ignore a call in a void function won't have that
effect.

I don't see an issue here:

# ethtool -s lan1 autoneg off speed 10 duplex half
# ethtool lan1
Settings for lan1:
        Supported ports: [ TP MII ]
        Supported link modes:   10baseT/Half 10baseT/Full
                                100baseT/Half 100baseT/Full
                                1000baseT/Full
        Supported pause frame use: Symmetric
        Supports auto-negotiation: Yes
        Supported FEC modes: Not reported
        Advertised link modes:  10baseT/Half
        Advertised pause frame use: Symmetric
        Advertised auto-negotiation: No
        Advertised FEC modes: Not reported
        Speed: 10Mb/s
        Duplex: Half
        Port: MII
        PHYAD: 4
        Transceiver: internal
        Auto-negotiation: off
        Supports Wake-on: d
        Wake-on: d
        Link detected: yes

libmii.c:v2.11 2/28/2005  Donald Becker (becker@scyld.com)
 http://www.scyld.com/diag/index.html
 MII PHY #4 transceiver registers:
   0000 794d 0141 0eb1 05e1 0000 0004 2001
   0000 0e00 0000 0000 0000 0003 0000 3000
   3360 0c48 6400 0000 0020 0000 0000 0000
   0000 0000 8040 0000 0000 0000 0000 0000.
 Basic mode control register 0x0000: Auto-negotiation disabled!
   Speed fixed at 10 mbps, half-duplex.
 Basic mode status register 0x794d ... 794d.
   Link status: established.
   Capable of  100baseTx-FD 100baseTx 10baseT-FD 10baseT.
   Able to perform Auto-negotiation, negotiation not complete.
 Vendor ID is 00:50:43:--:--:--, model 43 rev. 1.
   No specific information is known about this transceiver type.
 I'm advertising 05e1: Flow-control 100baseTx-FD 100baseTx 10baseT-FD 10baseT
   Advertising no additional info pages.
   IEEE 802.3 CSMA/CD protocol.
 Link partner capability is 0000:.
   Negotiation did not complete.

and traffic passes.

I've also been able to change what is advertised just fine, and the
link comes up as expected - in fact, I was running one of the switch
ports at 10Mbps to one of my machines and using the 'scope on the
ethernet signals over the weekend to debug a problem, which turned
out to be broken RGMII clock delay timings.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH] net: dsa: mv88e6xxx: Allow MAC configuration for ports with internal PHY

[Daniel Mack]

Hi Russell,

On 6/22/20 8:58 PM, Russell King - ARM Linux admin wrote:
> On Mon, Jun 22, 2020 at 08:44:51PM +0200, Daniel Mack wrote:
>> On 6/22/20 8:41 PM, Andrew Lunn wrote:

>>> How are you trying to change the speed?
>>
>> With ethtool for instance. But all userspace tools are bailing out early
>> on this port for the reason I described.
> 
> A simple "return" to ignore a call in a void function won't have that
> effect.

It has the effect that mv88e6xxx_port_setup_mac() is currently not being
called from mv88e6xxx_mac_config().

> I don't see an issue here:
> 
> # ethtool -s lan1 autoneg off speed 10 duplex half

I've tried that of course, but that doesn't fix the problem here. Which
switch port does 'lan1' map to in your setup? My CPU port maps to port 4.

Correct me if I'm mistaken, but speed and duplex settings are only being
communicated to the MAC driver through the aforementioned chain of
calls, right?

> I've also been able to change what is advertised just fine, and the
> link comes up as expected - in fact, I was running one of the switch
> ports at 10Mbps to one of my machines and using the 'scope on the
> ethernet signals over the weekend to debug a problem, which turned
> out to be broken RGMII clock delay timings.

To recap, my setup features a Cadence GEM that is connected to a 88E1510
PHY which is then connected to port 4 of the switch (which has an
internal PHY) through a transformer-less link. I know this is not
optimal as the speed is limited to 100M by that, but that was the only
way as all other ports where used up.

The setup works just fine in principle, I'm just struggling with a
correct way of configuring the drivers to allow that setting.

I can control what is advertised on eth0, which is the GEM, and the PHY
there reports the correct link speed:


# ethtool -s eth0 advertise 0x008
[   79.573992] macb e000b000.ethernet eth0: Link is Down
[   79.637048] mv88e6085 e000b000.ethernet-ffffffff:02: Link is Down
[   81.221974] macb e000b000.ethernet eth0: Link is Up - 100Mbps/Full -
flow control off
[   81.285639] mv88e6085 e000b000.ethernet-ffffffff:02: Link is Up -
100Mbps/Full - flow control off

However, the MAC in the switch is not changed by that, and it was forced
to 1 Gbit at probe time of the driver. Hence no packets are being seen
by the GEM, even though the PHYs seem to see each other just fine
(traffic is also signaled by an LED on the 88E1510).

I'm happy to try other solutions of course.


Thanks,
Daniel

Re: [PATCH] net: dsa: mv88e6xxx: Allow MAC configuration for ports with internal PHY

[Andrew Lunn]

> To recap, my setup features a Cadence GEM that is connected to a 88E1510
> PHY which is then connected to port 4 of the switch (which has an
> internal PHY) through a transformer-less link. I know this is not
> optimal as the speed is limited to 100M by that, but that was the only
> way as all other ports where used up.

This is the important bit you failed to mention. Given the number of
patches on netdev, you should assume anything older than three days
has been forgotten.

Back to Back PHYs for the CPU port has never really been supported.
It does however work if the PHYs are 1G and there are a few boards out
there like this, with their owns having crossed fingers this never
breaks. Because it is not really supported.

I guess you need to work out why PPU is not working for you. I would
not be too surprised if it is because it is the CPU port, it is not
supposed to have a PHY, so it is not enabled.

	 Andrew

Re: [PATCH] net: dsa: mv88e6xxx: Allow MAC configuration for ports with internal PHY

[Russell King - ARM Linux admin]

On Mon, Jun 22, 2020 at 09:16:59PM +0200, Daniel Mack wrote:
> Hi Russell,
> 
> On 6/22/20 8:58 PM, Russell King - ARM Linux admin wrote:
> > On Mon, Jun 22, 2020 at 08:44:51PM +0200, Daniel Mack wrote:
> >> On 6/22/20 8:41 PM, Andrew Lunn wrote:
> 
> >>> How are you trying to change the speed?
> >>
> >> With ethtool for instance. But all userspace tools are bailing out early
> >> on this port for the reason I described.
> > 
> > A simple "return" to ignore a call in a void function won't have that
> > effect.
> 
> It has the effect that mv88e6xxx_port_setup_mac() is currently not being
> called from mv88e6xxx_mac_config().

Which is correct.

> > I don't see an issue here:
> > 
> > # ethtool -s lan1 autoneg off speed 10 duplex half
> 
> I've tried that of course, but that doesn't fix the problem here. Which
> switch port does 'lan1' map to in your setup? My CPU port maps to port 4.

This is a clearfog, it maps to the port closest to the SFP port.

> Correct me if I'm mistaken, but speed and duplex settings are only being
> communicated to the MAC driver through the aforementioned chain of
> calls, right?

No, as I explained, the PPU (which is hardware inside the switch)
takes care of keeping the switch port in sync with the internal
PHY.

> > I've also been able to change what is advertised just fine, and the
> > link comes up as expected - in fact, I was running one of the switch
> > ports at 10Mbps to one of my machines and using the 'scope on the
> > ethernet signals over the weekend to debug a problem, which turned
> > out to be broken RGMII clock delay timings.
> 
> To recap, my setup features a Cadence GEM that is connected to a 88E1510
> PHY which is then connected to port 4 of the switch (which has an
> internal PHY) through a transformer-less link. I know this is not
> optimal as the speed is limited to 100M by that, but that was the only
> way as all other ports where used up.

So you have:

switch port <--> internal switch PHY <--> 88E1510 <--> Cadence GEM

and the switch will poll its internal PHY for the status of the link
between the two PHYs.  You should _not_ attach the 88E1510 to the
switch port - it is on the Cadence side of the link, and it should
be up to the Cadence end to manage that PHY, and not the switch side.

This is not much different than the inter-PHY transformer-less link
being a conventional RJ45 cable, except you want to operate the
switch port in "CPU" mode so that management frames are forwarded
out that link?

> The setup works just fine in principle, I'm just struggling with a
> correct way of configuring the drivers to allow that setting.

Right, so this is something new, and a setup we haven't seen before,
so likely it isn't supported.

I wonder if the PPU is being turned off by the fact that the port is
placed into "CPU" mode - I don't know off hand, I don't know the
Marvell DSA code all that well to be able to pull that knowledge out
of the top of my head, and I don't have time right now to read the
code to find out - I'm supposed to be on a Zoom meeting as of 15
minutes ago.

> I can control what is advertised on eth0, which is the GEM, and the PHY
> there reports the correct link speed:
> 
> 
> # ethtool -s eth0 advertise 0x008
> [   79.573992] macb e000b000.ethernet eth0: Link is Down
> [   79.637048] mv88e6085 e000b000.ethernet-ffffffff:02: Link is Down
> [   81.221974] macb e000b000.ethernet eth0: Link is Up - 100Mbps/Full -
> flow control off
> [   81.285639] mv88e6085 e000b000.ethernet-ffffffff:02: Link is Up -
> 100Mbps/Full - flow control off
> 
> However, the MAC in the switch is not changed by that, and it was forced
> to 1 Gbit at probe time of the driver. Hence no packets are being seen
> by the GEM, even though the PHYs seem to see each other just fine
> (traffic is also signaled by an LED on the 88E1510).
> 
> I'm happy to try other solutions of course.

I think this is going to be something that will need discussion with
the DSA folk, given that it's an entirely new setup that hasn't been
catered for.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH] net: dsa: mv88e6xxx: Allow MAC configuration for ports with internal PHY

[Daniel Mack]

On 6/22/20 9:43 PM, Russell King - ARM Linux admin wrote:
> On Mon, Jun 22, 2020 at 09:16:59PM +0200, Daniel Mack wrote:
>> On 6/22/20 8:58 PM, Russell King - ARM Linux admin wrote:

>>> I don't see an issue here:
>>>
>>> # ethtool -s lan1 autoneg off speed 10 duplex half
>>
>> I've tried that of course, but that doesn't fix the problem here. Which
>> switch port does 'lan1' map to in your setup? My CPU port maps to port 4.
> 
> This is a clearfog, it maps to the port closest to the SFP port.
> 
>> Correct me if I'm mistaken, but speed and duplex settings are only being
>> communicated to the MAC driver through the aforementioned chain of
>> calls, right?
> 
> No, as I explained, the PPU (which is hardware inside the switch)
> takes care of keeping the switch port in sync with the internal
> PHY.

Right. Got it now. As Andrew also explained, the most plausible
explanation is that the PPU is off for that port for some reason. I'll
go digging in that direction.

Consider that patch dropped hence.


Thanks a lot for your time and help!
Daniel