[PATCH] net: dsa: mv88e6xxx: don't force settings on CPU port

[PATCH] net: dsa: mv88e6xxx: don't force settings on CPU port

[Daniel Mack]

On hardware with a speed-reduced link to the CPU port, forcing the MAC
settings won't allow any packets to pass. The PHY will negotiate the
maximum possible speed, so let's allow the MAC to work with whatever
is available.

Signed-off-by: Daniel Mack <daniel@zonque.org>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 2f993e673ec7..48808c4add4f 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -2426,7 +2426,7 @@ static int mv88e6xxx_setup_port(struct mv88e6xxx_chip *chip, int port)
 	 * state to any particular values on physical ports, but force the CPU
 	 * port and all DSA ports to their maximum bandwidth and full duplex.
 	 */
-	if (dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, port))
+	if (dsa_is_dsa_port(ds, port))
 		err = mv88e6xxx_port_setup_mac(chip, port, LINK_FORCED_UP,
 					       SPEED_MAX, DUPLEX_FULL,
 					       PAUSE_OFF,
-- 
2.25.1

Re: [PATCH] net: dsa: mv88e6xxx: don't force settings on CPU port

[Andrew Lunn]

On Fri, Mar 27, 2020 at 08:51:56PM +0100, Daniel Mack wrote:
> On hardware with a speed-reduced link to the CPU port, forcing the MAC
> settings won't allow any packets to pass. The PHY will negotiate the
> maximum possible speed, so let's allow the MAC to work with whatever
> is available.

Hi Daniel

This will break board which rely on the CPU being forced to the
maximum speed, which has been the default since forever.

It sounds like you have the unusual situation of back to back PHYs?
And i assume the SoC PHY is limited to Fast Ethernet?

What i think you can do is have a phy-handle in the cpu node which
points to the PHY. That should then cause the PHY to be driven as a
normal PHY, and the result of auto neg passed to the MAC.

       Andrew

> 
> Signed-off-by: Daniel Mack <daniel@zonque.org>
> ---
>  drivers/net/dsa/mv88e6xxx/chip.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
> index 2f993e673ec7..48808c4add4f 100644
> --- a/drivers/net/dsa/mv88e6xxx/chip.c
> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> @@ -2426,7 +2426,7 @@ static int mv88e6xxx_setup_port(struct mv88e6xxx_chip *chip, int port)
>  	 * state to any particular values on physical ports, but force the CPU
>  	 * port and all DSA ports to their maximum bandwidth and full duplex.
>  	 */
> -	if (dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, port))
> +	if (dsa_is_dsa_port(ds, port))
>  		err = mv88e6xxx_port_setup_mac(chip, port, LINK_FORCED_UP,
>  					       SPEED_MAX, DUPLEX_FULL,
>  					       PAUSE_OFF,
> -- 
> 2.25.1
> 

Re: [PATCH] net: dsa: mv88e6xxx: don't force settings on CPU port

[Daniel Mack]

Hi Andrew,

On 27/3/2020 9:01 pm, Andrew Lunn wrote:
> On Fri, Mar 27, 2020 at 08:51:56PM +0100, Daniel Mack wrote:
>> On hardware with a speed-reduced link to the CPU port, forcing the MAC
>> settings won't allow any packets to pass. The PHY will negotiate the
>> maximum possible speed, so let's allow the MAC to work with whatever
>> is available.
> 
> This will break board which rely on the CPU being forced to the
> maximum speed, which has been the default since forever.

Will it? Wouldn't the PHY negotiate the maximum speed, and the MAC would
follow?

> It sounds like you have the unusual situation of back to back PHYs?
> And i assume the SoC PHY is limited to Fast Ethernet?

Yes, exactly.

> What i think you can do is have a phy-handle in the cpu node which
> points to the PHY. That should then cause the PHY to be driven as a
> normal PHY, and the result of auto neg passed to the MAC.

Yes, this is what I have. The maximum speed the is negotiable on that
link is 100M, and the PHYs see each other just fine (according to the
status registers of the external PHY). The problem is that the MAC
inside the switch is forced to 1G, which doesn't match what the PHY
negotiated.

Not sure what else can be done to make such setups work instead of
lifting that particular constraint on the MAC side. Could you explain
why the settings are forced at all?


Thanks,
Daniel

Re: [PATCH] net: dsa: mv88e6xxx: don't force settings on CPU port

[Andrew Lunn]

On Fri, Mar 27, 2020 at 09:48:56PM +0100, Daniel Mack wrote:
> Hi Andrew,
> 
> On 27/3/2020 9:01 pm, Andrew Lunn wrote:
> > On Fri, Mar 27, 2020 at 08:51:56PM +0100, Daniel Mack wrote:
> >> On hardware with a speed-reduced link to the CPU port, forcing the MAC
> >> settings won't allow any packets to pass. The PHY will negotiate the
> >> maximum possible speed, so let's allow the MAC to work with whatever
> >> is available.
> > 
> > This will break board which rely on the CPU being forced to the
> > maximum speed, which has been the default since forever.
> 
> Will it? Wouldn't the PHY negotiate the maximum speed, and the MAC would
> follow?

Most boards just connect the SoC MAC to the switch MAC. No PHY.

There is no need to have PHYs here, unless your switch is a long way
away from the SoC. It is just added extra expense for no reason.

> > It sounds like you have the unusual situation of back to back PHYs?
> > And i assume the SoC PHY is limited to Fast Ethernet?
> 
> Yes, exactly.

And i guess you are stuck with this design?

> > What i think you can do is have a phy-handle in the cpu node which
> > points to the PHY. That should then cause the PHY to be driven as a
> > normal PHY, and the result of auto neg passed to the MAC.
> 
> Yes, this is what I have. The maximum speed the is negotiable on that
> link is 100M, and the PHYs see each other just fine (according to the
> status registers of the external PHY). The problem is that the MAC
> inside the switch is forced to 1G, which doesn't match what the PHY
> negotiated.

So try a fixed link in the CPU node.

                                       port@6 {
                                                reg = <6>;
                                                label = "cpu";
                                                ethernet = <&fec1>;

                                                fixed-link {
                                                        speed = <100>;
                                                        full-duplex;
                                                };

This won't work with current net-next, which is broken at the
moment. But it might work with older kernels. I've not tried this when
there actually is a PHY. It is normally used when you need to slow the
port down from its default highest speed in the usual MAC-MAC
setting. In this case, the FEC is Fast Ethernet only, so we need the
Switch MAC to run at 100Mbps.

     Andrew

Re: [PATCH] net: dsa: mv88e6xxx: don't force settings on CPU port

[Daniel Mack]

On 27/3/2020 10:18 pm, Andrew Lunn wrote:
> On Fri, Mar 27, 2020 at 09:48:56PM +0100, Daniel Mack wrote:
>> On 27/3/2020 9:01 pm, Andrew Lunn wrote:
>>> On Fri, Mar 27, 2020 at 08:51:56PM +0100, Daniel Mack wrote:
>>>> On hardware with a speed-reduced link to the CPU port, forcing the MAC
>>>> settings won't allow any packets to pass. The PHY will negotiate the
>>>> maximum possible speed, so let's allow the MAC to work with whatever
>>>> is available.
>>>
>>> This will break board which rely on the CPU being forced to the
>>> maximum speed, which has been the default since forever.
>>
>> Will it? Wouldn't the PHY negotiate the maximum speed, and the MAC would
>> follow?
> 
> Most boards just connect the SoC MAC to the switch MAC. No PHY.

Yes, I know. In our design all RGMII ports are in use already, so we had
to go for a transformer-less link on a regular copper port, and connect
a phy on the other side.

> And i guess you are stuck with this design?

Yes.

>> Yes, this is what I have. The maximum speed the is negotiable on that
>> link is 100M, and the PHYs see each other just fine (according to the
>> status registers of the external PHY). The problem is that the MAC
>> inside the switch is forced to 1G, which doesn't match what the PHY
>> negotiated.
> 
> So try a fixed link in the CPU node.
> 
>                                        port@6 {
>                                                 reg = <6>;
>                                                 label = "cpu";
>                                                 ethernet = <&fec1>;
> 
>                                                 fixed-link {
>                                                         speed = <100>;
>                                                         full-duplex;
>                                                 };
>
> This won't work with current net-next, which is broken at the
> moment. But it might work with older kernels.

I tried this as well with v5.5, but that leads to the external phy not
seeing a link at all. Will check again though.

Could you think of a way to make this behavior conditional? Could we
introduce a DT property or something?

Worst case is to keep that one-liner as a downstream patch, but I could
well imagine other people facing the same issue.


Thanks,
Daniel

Re: [PATCH] net: dsa: mv88e6xxx: don't force settings on CPU port

[Andrew Lunn]

> I tried this as well with v5.5, but that leads to the external phy not
> seeing a link at all. Will check again though.

Did you turn off auto-neg on the external PHY and use fixed 100Full?
Ethtool on the SoC interface should show you if the switch PHY is
advertising anything. I'm guessing it is not, and hence you need to
turn off auto neg on the external PHY.

Another option would be something like

                                        port@6 {
                                                reg = <6>;
                                                label = "cpu";
                                                ethernet = <&fec1>;

                                                phy-handle = <phy6>;
                                        };
                                };

                                mdio {
                                        #address-cells = <1>;
                                        #size-cells = <0>;
                                        phy6: ethernet-phy@6 {
                                                reg = <6>;
                                                interrupt-parent = <&switch0>;
                                                interrupts = <0 IRQ_TYPE_LEVEL_HIGH>;
                                        };
                                };

By explicitly saying there is a PHY for the CPU node, phylink might
drive it.

       Andrew

Re: [PATCH] net: dsa: mv88e6xxx: don't force settings on CPU port

[Daniel Mack]

On 3/28/20 12:52 AM, Andrew Lunn wrote:
>> I tried this as well with v5.5, but that leads to the external phy not
>> seeing a link at all. Will check again though.
> 
> Did you turn off auto-neg on the external PHY and use fixed 100Full?
> Ethtool on the SoC interface should show you if the switch PHY is
> advertising anything. I'm guessing it is not, and hence you need to
> turn off auto neg on the external PHY.
> 
> Another option would be something like
> 
>                                         port@6 {
>                                                 reg = <6>;
>                                                 label = "cpu";
>                                                 ethernet = <&fec1>;
> 
>                                                 phy-handle = <phy6>;
>                                         };
>                                 };
> 
>                                 mdio {
>                                         #address-cells = <1>;
>                                         #size-cells = <0>;
>                                         phy6: ethernet-phy@6 {
>                                                 reg = <6>;
>                                                 interrupt-parent = <&switch0>;
>                                                 interrupts = <0 IRQ_TYPE_LEVEL_HIGH>;
>                                         };
>                                 };
> 
> By explicitly saying there is a PHY for the CPU node, phylink might
> drive it.

Hmm, no. No luck with this either.

Given that the code forces the MAC for cases in which there is no PHY,
could we maybe omit this if there _is_ a PHY? Or make it conditional via
a DT property?


Thanks,
Daniel

Re: [PATCH] net: dsa: mv88e6xxx: don't force settings on CPU port

[Andrew Lunn]

On Mon, Mar 30, 2020 at 11:29:27AM +0200, Daniel Mack wrote:
> On 3/28/20 12:52 AM, Andrew Lunn wrote:
> >> I tried this as well with v5.5, but that leads to the external phy not
> >> seeing a link at all. Will check again though.
> > 
> > Did you turn off auto-neg on the external PHY and use fixed 100Full?
> > Ethtool on the SoC interface should show you if the switch PHY is
> > advertising anything. I'm guessing it is not, and hence you need to
> > turn off auto neg on the external PHY.
> > 
> > Another option would be something like
> > 
> >                                         port@6 {
> >                                                 reg = <6>;
> >                                                 label = "cpu";
> >                                                 ethernet = <&fec1>;
> > 
> >                                                 phy-handle = <phy6>;
> >                                         };
> >                                 };
> > 
> >                                 mdio {
> >                                         #address-cells = <1>;
> >                                         #size-cells = <0>;
> >                                         phy6: ethernet-phy@6 {
> >                                                 reg = <6>;
> >                                                 interrupt-parent = <&switch0>;
> >                                                 interrupts = <0 IRQ_TYPE_LEVEL_HIGH>;
> >                                         };
> >                                 };
> > 
> > By explicitly saying there is a PHY for the CPU node, phylink might
> > drive it.

You want to debug this. Although what you have is unusual, yours is
not the only board. It is something we want to work. And ideally,
there should be something controlling the PHY.

      Andrew

Re: [PATCH] net: dsa: mv88e6xxx: don't force settings on CPU port

[Daniel Mack]

Hi Andrew,

Thanks for all your input.

On 3/30/20 3:40 PM, Andrew Lunn wrote:
> On Mon, Mar 30, 2020 at 11:29:27AM +0200, Daniel Mack wrote:
>> On 3/28/20 12:52 AM, Andrew Lunn wrote:

>>> By explicitly saying there is a PHY for the CPU node, phylink might
>>> drive it.
> 
> You want to debug this. Although what you have is unusual, yours is
> not the only board. It is something we want to work. And ideally,
> there should be something controlling the PHY.

I agree, but what I believe is happening here is this. The PHY inside
the switch negotiates a link to the 'external' PHY which is forced to
100M maximum speed. That link seems to work fine; the LEDs connected to
that external PHY indicate that there is link. However, the internal PHY
in the switch does not receive any packets as the MAC connected to it
only wants to communicate with 1G.

Not sure what else could be done other than allowing for reduced speed
on the MAC as well, which is what my patch is doing. I agree that my
approach falls short for boards where there is no PHY on the port
connected to the CPU, but maybe there is some common ground here, and a
rule can be defined under which circumstances the MAC speed should be
forced?


Thanks,
Daniel

Re: [PATCH] net: dsa: mv88e6xxx: don't force settings on CPU port

[Andrew Lunn]

On Mon, Mar 30, 2020 at 08:04:08PM +0200, Daniel Mack wrote:
> Hi Andrew,
> 
> Thanks for all your input.
> 
> On 3/30/20 3:40 PM, Andrew Lunn wrote:
> > On Mon, Mar 30, 2020 at 11:29:27AM +0200, Daniel Mack wrote:
> >> On 3/28/20 12:52 AM, Andrew Lunn wrote:
> 
> >>> By explicitly saying there is a PHY for the CPU node, phylink might
> >>> drive it.
> > 
> > You want to debug this. Although what you have is unusual, yours is
> > not the only board. It is something we want to work. And ideally,
> > there should be something controlling the PHY.
> 
> I agree, but what I believe is happening here is this. The PHY inside
> the switch negotiates a link to the 'external' PHY which is forced to
> 100M maximum speed. That link seems to work fine; the LEDs connected to
> that external PHY indicate that there is link. However, the internal PHY
> in the switch does not receive any packets as the MAC connected to it
> only wants to communicate with 1G.

Which is what phylink is all about. phylink will talk to the PHY,
figure out what it has negotiated, and then configure the MAC to
fit. So you need to debug why this is not happening.

       Andrew

Re: [PATCH] net: dsa: mv88e6xxx: don't force settings on CPU port

[Daniel Mack]

On 3/30/20 8:23 PM, Andrew Lunn wrote:
> On Mon, Mar 30, 2020 at 08:04:08PM +0200, Daniel Mack wrote:
>> Hi Andrew,
>>
>> Thanks for all your input.
>>
>> On 3/30/20 3:40 PM, Andrew Lunn wrote:
>>> On Mon, Mar 30, 2020 at 11:29:27AM +0200, Daniel Mack wrote:
>>>> On 3/28/20 12:52 AM, Andrew Lunn wrote:
>>
>>>>> By explicitly saying there is a PHY for the CPU node, phylink might
>>>>> drive it.
>>>
>>> You want to debug this. Although what you have is unusual, yours is
>>> not the only board. It is something we want to work. And ideally,
>>> there should be something controlling the PHY.
>>
>> I agree, but what I believe is happening here is this. The PHY inside
>> the switch negotiates a link to the 'external' PHY which is forced to
>> 100M maximum speed. That link seems to work fine; the LEDs connected to
>> that external PHY indicate that there is link. However, the internal PHY
>> in the switch does not receive any packets as the MAC connected to it
>> only wants to communicate with 1G.
> 
> Which is what phylink is all about. phylink will talk to the PHY,
> figure out what it has negotiated, and then configure the MAC to
> fit. So you need to debug why this is not happening.

Even when the MAC is *forced* to 1G, which is what the code currently
does? Sorry for the dumb question, but wich code path would undo these
settings? Where would you start debugging this?


Thanks,
Daniel

Re: [PATCH] net: dsa: mv88e6xxx: don't force settings on CPU port

[Andrew Lunn]

> Even when the MAC is *forced* to 1G, which is what the code currently
> does? Sorry for the dumb question, but wich code path would undo these
> settings? Where would you start debugging this?

You could add #define DEBUG to the top of driver/net/phy/phylink.c.
You should then see what phylink is up to, including it trying to
configure the MAC. You can add additional printk to
mv88e6xxx_mac_link_up(), mv88e6xxx_mac_link_down(),
mv88e6xxx_mac_config(), etc.

	  Andrew

Re: [PATCH] net: dsa: mv88e6xxx: don't force settings on CPU port

[Daniel Mack]

Hi Andrew,

Picking up this ancient thread, sorry for the delay.

On 3/30/20 3:40 PM, Andrew Lunn wrote:
> On Mon, Mar 30, 2020 at 11:29:27AM +0200, Daniel Mack wrote:
>> On 3/28/20 12:52 AM, Andrew Lunn wrote:
>>> Did you turn off auto-neg on the external PHY and use fixed 100Full?
>>> Ethtool on the SoC interface should show you if the switch PHY is
>>> advertising anything. I'm guessing it is not, and hence you need to
>>> turn off auto neg on the external PHY.
>>>
>>> Another option would be something like
>>>
>>>                                         port@6 {
>>>                                                 reg = <6>;
>>>                                                 label = "cpu";
>>>                                                 ethernet = <&fec1>;
>>>
>>>                                                 phy-handle = <phy6>;
>>>                                         };
>>>                                 };
>>>
>>>                                 mdio {
>>>                                         #address-cells = <1>;
>>>                                         #size-cells = <0>;
>>>                                         phy6: ethernet-phy@6 {
>>>                                                 reg = <6>;
>>>                                                 interrupt-parent = <&switch0>;
>>>                                                 interrupts = <0 IRQ_TYPE_LEVEL_HIGH>;
>>>                                         };
>>>                                 };
>>>
>>> By explicitly saying there is a PHY for the CPU node, phylink might
>>> drive it.
> 
> You want to debug this. Although what you have is unusual, yours is
> not the only board. It is something we want to work. And ideally,
> there should be something controlling the PHY.

I spent some more time on this today, and the reason for why this fails
is simple. The PHY on port 4 is internal, and mv88e6xxx_mac_config()
hence decides to not touch the config of this port, unless it's a
fixed-linked config. And the latter is not an option the port has a
phy-handle.

This means that non-fixed ports with internal PHYs are only programmed
once at probe time, and userspace can't modify the settings later on.

I've sent a patch to relax that check, but tbh I'm not sure whether I
miss a relevant piece of detail about the current code.


Thanks,
Daniel