Message ID | 20200327195156.1728163-1-daniel@zonque.org |
---|---|
State | Changes Requested |
Delegated to: | David Miller |
Headers | show |
Series | net: dsa: mv88e6xxx: don't force settings on CPU port | expand |
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 >
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
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
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
> 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
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
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
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
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
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
> 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
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
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,
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(-)