| Message ID | YsQIjC7UpcGWJovx@shell.armlinux.org.uk (mailing list archive) |
|---|---|
| Headers | show |
| Series | net: dsa: always use phylink | expand |
On 7/5/22 02:46, Russell King (Oracle) wrote: > A new revision of the series which incorporates changes that Marek > suggested. Specifically, the changes are: > > 1. Patch 2 - use the phylink_get_caps method in mv88e6xxx to get the > default interface rather than re-using port_max_speed_mode() > > 2. Patch 4 - if no default interface is provided, use the supported > interface mask to search for the first interface that gives the > fastest speed. > > 3. Patch 5 - now also removes the port_max_speed_mode() method This was tested with bcm_sf2.c and b53_srab.b and did not cause regressions, however we do have a 'fixed-link' property for the CPU port (always have had one), so there was no regression expected. See answers to your RFC v1 below. > > drivers/net/dsa/b53/b53_common.c | 3 +- > drivers/net/dsa/bcm_sf2.c | 3 +- > drivers/net/dsa/hirschmann/hellcreek.c | 3 +- > drivers/net/dsa/lantiq_gswip.c | 6 +- > drivers/net/dsa/microchip/ksz_common.c | 3 +- > drivers/net/dsa/mt7530.c | 3 +- > drivers/net/dsa/mv88e6xxx/chip.c | 136 +++++++++++++++--------------- > drivers/net/dsa/mv88e6xxx/chip.h | 6 +- > drivers/net/dsa/mv88e6xxx/port.c | 32 ------- > drivers/net/dsa/mv88e6xxx/port.h | 5 -- > drivers/net/dsa/ocelot/felix.c | 3 +- > drivers/net/dsa/qca/ar9331.c | 3 +- > drivers/net/dsa/qca8k.c | 3 +- > drivers/net/dsa/realtek/rtl8365mb.c | 3 +- > drivers/net/dsa/sja1105/sja1105_main.c | 3 +- > drivers/net/dsa/xrs700x/xrs700x.c | 3 +- > drivers/net/phy/phylink.c | 150 ++++++++++++++++++++++++++++++--- > include/linux/phylink.h | 5 ++ > include/net/dsa.h | 3 +- > net/dsa/port.c | 47 +++++++---- > 20 files changed, 270 insertions(+), 153 deletions(-) > > On Wed, Jun 29, 2022 at 01:49:57PM +0100, Russell King (Oracle) wrote: >> Mostly the same as the previous RFC, except: >> >> 1) incldues the phylink_validate_mask_caps() function >> 2) has Marek's idea of searching the supported_interfaces bitmap for the >> fastest interface we can use >> 3) includes a final patch to add a print which will be useful to hear >> from people testing it. >> >> Some of the questions from the original RFC remain though, so I've >> included that text below. I'm guessing as they remain unanswered that >> no one has any opinions on them? >> >> drivers/net/dsa/b53/b53_common.c | 3 +- >> drivers/net/dsa/bcm_sf2.c | 3 +- >> drivers/net/dsa/hirschmann/hellcreek.c | 3 +- >> drivers/net/dsa/lantiq_gswip.c | 6 +- >> drivers/net/dsa/microchip/ksz_common.c | 3 +- >> drivers/net/dsa/mt7530.c | 3 +- >> drivers/net/dsa/mv88e6xxx/chip.c | 53 ++++-------- >> drivers/net/dsa/ocelot/felix.c | 3 +- >> drivers/net/dsa/qca/ar9331.c | 3 +- >> drivers/net/dsa/qca8k.c | 3 +- >> drivers/net/dsa/realtek/rtl8365mb.c | 3 +- >> drivers/net/dsa/sja1105/sja1105_main.c | 3 +- >> drivers/net/dsa/xrs700x/xrs700x.c | 3 +- >> drivers/net/phy/phylink.c | 148 ++++++++++++++++++++++++++++++--- >> include/linux/phylink.h | 5 ++ >> include/net/dsa.h | 3 +- >> net/dsa/port.c | 47 +++++++---- >> 17 files changed, 215 insertions(+), 80 deletions(-) >> >> On Fri, Jun 24, 2022 at 12:41:26PM +0100, Russell King (Oracle) wrote: >>> Hi, >>> >>> Currently, the core DSA code conditionally uses phylink for CPU and DSA >>> ports depending on whether the firmware specifies a fixed-link or a PHY. >>> If either of these are specified, then phylink is used for these ports, >>> otherwise phylink is not, and we rely on the DSA drivers to "do the >>> right thing". However, this detail is not mentioned in the DT binding, >>> but Andrew has said that this behaviour has always something that DSA >>> wants. >>> >>> mv88e6xxx has had support for this for a long time with its "SPEED_MAX" >>> thing, which I recently reworked to make use of the mac_capabilities in >>> preparation to solving this more fully. >>> >>> This series is an experiment to solve this properly, and it does this >>> in two steps. >>> >>> The first step consists of the first two patches. Phylink needs to >>> know the PHY interface mode that is being used so it can (a) pass the >>> right mode into the MAC/PCS etc and (b) know the properties of the >>> link and therefore which speeds can be supported across it. >>> >>> In order to achieve this, the DSA phylink_get_caps() method has an >>> extra argument added to it so that DSA drivers can report the >>> interface mode that they will be using for this port back to the core >>> DSA code, thereby allowing phylink to be initialised with the correct >>> interface mode. >>> >>> Note that this can only be used for CPU and DSA ports as "user" ports >>> need a different behaviour - they rely on getting the interface mode >>> from phylib, which will only happen if phylink is initialised with >>> PHY_INTERFACE_MODE_NA. Unfortunately, changing this behaviour is likely >>> to cause widespread regressions. >>> >>> Obvious questions: >>> 1. Should phylink_get_caps() be augmented in this way, or should it be >>> a separate method? >>> >>> 2. DSA has traditionally used "interface mode for the maximum supported >>> speed on this port" where the interface mode is programmable (via >>> its internal port_max_speed_mode() method) but this is only present >>> for a few of the sub-drivers. Is reporting the current interface >>> mode correct where this method is not implemented? >>> >>> The second step is to introduce a function that allows phylink to be >>> reconfigured after creation time to operate at max-speed fixed-link >>> mode for the PHY interface mode, also using the MAC capabilities to >>> determine the speed and duplex mode we should be using. >>> >>> Obvious questions: >>> 1. Should we be allowing half-duplex for this? Except for testing, not sure I do see a point as it should not be a configuration being used at all? >>> 2. If we do allow half-duplex, should we prefer fastest speed over >>> duplex setting, or should we prefer fastest full-duplex speed >>> over any half-duplex? I would opt for fastest speed over duplex setting. >>> 3. How do we sanely switch DSA from its current behaviour to always >>> using phylink for these ports without breakage - this is the >>> difficult one, because it's not obvious which drivers have been >>> coded to either work around this quirk of the DSA implementation. >>> For example, if we start forcing the link down before calling >>> dsa_port_phylink_create(), and we then fail to set max-fixed-link, >>> then the CPU/DSA port is going to fail, and we're going to have >>> lots of regressions. Good question, we already have a legacy_pre_march2020 behavior for a piece of infrastructure code that is not so old, I doubt that we would want to add more of that type of quirk.
Hi Florian, On Tue, Jul 05, 2022 at 09:42:33AM -0700, Florian Fainelli wrote: > On 7/5/22 02:46, Russell King (Oracle) wrote: > > A new revision of the series which incorporates changes that Marek > > suggested. Specifically, the changes are: > > > > 1. Patch 2 - use the phylink_get_caps method in mv88e6xxx to get the > > default interface rather than re-using port_max_speed_mode() > > > > 2. Patch 4 - if no default interface is provided, use the supported > > interface mask to search for the first interface that gives the > > fastest speed. > > > > 3. Patch 5 - now also removes the port_max_speed_mode() method > > This was tested with bcm_sf2.c and b53_srab.b and did not cause regressions, > however we do have a 'fixed-link' property for the CPU port (always have had > one), so there was no regression expected. What about arch/arm/boot/dts/bcm47189-tenda-ac9.dts? Just to make sure I'm simply not seeing something, I compiled and then decompiled the dtb. We have: ethernet@5000 { reg = <0x5000 0x1000>; phandle = <0x03>; mdio { #address-cells = <0x01>; #size-cells = <0x00>; switch@1e { compatible = "brcm,bcm53125"; reg = <0x1e>; status = "okay"; ports { #address-cells = <0x01>; #size-cells = <0x00>; port@0 { reg = <0x00>; label = "wan"; }; port@1 { reg = <0x01>; label = "lan1"; }; port@2 { reg = <0x02>; label = "lan2"; }; port@3 { reg = <0x03>; label = "lan3"; }; port@4 { reg = <0x04>; label = "lan4"; }; port@5 { reg = <0x05>; label = "cpu"; ethernet = <0x03>; <- CPU port with no phy-handle, no fixed-link }; }; }; }; };
On 7/6/22 03:14, Vladimir Oltean wrote: > Hi Florian, > > On Tue, Jul 05, 2022 at 09:42:33AM -0700, Florian Fainelli wrote: >> On 7/5/22 02:46, Russell King (Oracle) wrote: >>> A new revision of the series which incorporates changes that Marek >>> suggested. Specifically, the changes are: >>> >>> 1. Patch 2 - use the phylink_get_caps method in mv88e6xxx to get the >>> default interface rather than re-using port_max_speed_mode() >>> >>> 2. Patch 4 - if no default interface is provided, use the supported >>> interface mask to search for the first interface that gives the >>> fastest speed. >>> >>> 3. Patch 5 - now also removes the port_max_speed_mode() method >> >> This was tested with bcm_sf2.c and b53_srab.b and did not cause regressions, >> however we do have a 'fixed-link' property for the CPU port (always have had >> one), so there was no regression expected. > > What about arch/arm/boot/dts/bcm47189-tenda-ac9.dts? You found one of the devices that I do not have access to and did not test, thanks. We do expect to run the port at 2GBits/sec on these devices however there is no "official" way to advertise that a port can run at 2Gbits/sec, as this is not even a "sanctioned" speed. I do have a similar device however, so let me run some more tests, we won't see a regression however since we do not use the NATP accelerator which would be the reason to run the port at 2Gbits/sec.
Hi Russell, On Tue Jul 05 2022, Russell King wrote: > A new revision of the series which incorporates changes that Marek > suggested. Specifically, the changes are: > > 1. Patch 2 - use the phylink_get_caps method in mv88e6xxx to get the > default interface rather than re-using port_max_speed_mode() > > 2. Patch 4 - if no default interface is provided, use the supported > interface mask to search for the first interface that gives the > fastest speed. > > 3. Patch 5 - now also removes the port_max_speed_mode() method Tested with hellcreek driver and no problems observed. Thanks, Kurt
On 7/6/22 18:27, Florian Fainelli wrote: > On 7/6/22 03:14, Vladimir Oltean wrote: >> Hi Florian, >> >> On Tue, Jul 05, 2022 at 09:42:33AM -0700, Florian Fainelli wrote: >>> On 7/5/22 02:46, Russell King (Oracle) wrote: >>>> A new revision of the series which incorporates changes that Marek >>>> suggested. Specifically, the changes are: >>>> >>>> 1. Patch 2 - use the phylink_get_caps method in mv88e6xxx to get the >>>> default interface rather than re-using port_max_speed_mode() >>>> >>>> 2. Patch 4 - if no default interface is provided, use the supported >>>> interface mask to search for the first interface that gives the >>>> fastest speed. >>>> >>>> 3. Patch 5 - now also removes the port_max_speed_mode() method >>> >>> This was tested with bcm_sf2.c and b53_srab.b and did not cause >>> regressions, >>> however we do have a 'fixed-link' property for the CPU port (always >>> have had >>> one), so there was no regression expected. >> >> What about arch/arm/boot/dts/bcm47189-tenda-ac9.dts? > > You found one of the devices that I do not have access to and did not > test, thanks. We do expect to run the port at 2GBits/sec on these > devices however there is no "official" way to advertise that a port can > run at 2Gbits/sec, as this is not even a "sanctioned" speed. I do have a > similar device however, so let me run some more tests, we won't see a > regression however since we do not use the NATP accelerator which would > be the reason to run the port at 2Gbits/sec. I will try this change on some devices with the lantiq gswip driver at the weekend. On the SoC supported by the lantiq gswip driver the switch is integrated in the SoC and there is a internal link with more than 1GBit/s connecting the switch to the rest of the system. I think it is also around 2GBit/s. We can not configure the interface speed or many other interface settings for the link between the switch and the CPU. How should the device tree ideally look for this setup? Hauke
On Wed, Jul 06, 2022 at 09:05:22PM +0200, Hauke Mehrtens wrote: > On 7/6/22 18:27, Florian Fainelli wrote: > > On 7/6/22 03:14, Vladimir Oltean wrote: > > > Hi Florian, > > > > > > On Tue, Jul 05, 2022 at 09:42:33AM -0700, Florian Fainelli wrote: > > > > On 7/5/22 02:46, Russell King (Oracle) wrote: > > > > > A new revision of the series which incorporates changes that Marek > > > > > suggested. Specifically, the changes are: > > > > > > > > > > 1. Patch 2 - use the phylink_get_caps method in mv88e6xxx to get the > > > > > default interface rather than re-using port_max_speed_mode() > > > > > > > > > > 2. Patch 4 - if no default interface is provided, use the supported > > > > > interface mask to search for the first interface that gives the > > > > > fastest speed. > > > > > > > > > > 3. Patch 5 - now also removes the port_max_speed_mode() method > > > > > > > > This was tested with bcm_sf2.c and b53_srab.b and did not cause > > > > regressions, > > > > however we do have a 'fixed-link' property for the CPU port > > > > (always have had > > > > one), so there was no regression expected. > > > > > > What about arch/arm/boot/dts/bcm47189-tenda-ac9.dts? > > > > You found one of the devices that I do not have access to and did not > > test, thanks. We do expect to run the port at 2GBits/sec on these > > devices however there is no "official" way to advertise that a port can > > run at 2Gbits/sec, as this is not even a "sanctioned" speed. I do have a > > similar device however, so let me run some more tests, we won't see a > > regression however since we do not use the NATP accelerator which would > > be the reason to run the port at 2Gbits/sec. > > I will try this change on some devices with the lantiq gswip driver at the > weekend. > > On the SoC supported by the lantiq gswip driver the switch is integrated in > the SoC and there is a internal link with more than 1GBit/s connecting the > switch to the rest of the system. I think it is also around 2GBit/s. We can > not configure the interface speed or many other interface settings for the > link between the switch and the CPU. How should the device tree ideally look > for this setup? I think this falls into Andrew's "don't specify anything" category, which means that we should default to the maximum speed for the port - which is in this case quite clearly fixed at whatever the internal link actually is. From the get_caps() function, the fastest speed apparently supported is 1Gbps. I'm guessing the CPU port is one of ports 2..5 on xrx200 and 1..5 on xrx300 - in which case, PHY_INTERFACE_MODE_INTERNAL is likely to be selected, which seems appropriate given what you've said above. (PHY_INTERFACE_MODE_INTERNAL will be the first interface type found that will give the highest speed as it permits any speed given in the mac_capabilities.)
On Tue, Jul 5, 2022 at 11:47 AM Russell King (Oracle) <linux@armlinux.org.uk> wrote: > A new revision of the series which incorporates changes that Marek > suggested. Specifically, the changes are: > > 1. Patch 2 - use the phylink_get_caps method in mv88e6xxx to get the > default interface rather than re-using port_max_speed_mode() > > 2. Patch 4 - if no default interface is provided, use the supported > interface mask to search for the first interface that gives the > fastest speed. > > 3. Patch 5 - now also removes the port_max_speed_mode() method Pulled in the patch set on top of net-next and tested on the D-Link DIR-685 with the RTL8366RB switch with no regressions, so: Tested-by: Linus Walleij <linus.walleij@linaro.org> The plan is to enhance the phylink handling in the RTL8366RB on top of Russell's patch in some time. Yours, Linus Walleij
On Thu, Jul 7, 2022 at 12:46 AM Linus Walleij <linus.walleij@linaro.org> wrote: > Pulled in the patch set on top of net-next and tested on the > D-Link DIR-685 with the RTL8366RB switch with no > regressions, so: > Tested-by: Linus Walleij <linus.walleij@linaro.org> > > The plan is to enhance the phylink handling in the RTL8366RB > on top of Russell's patch in some time. I developed a patch like this and tested it, it works great and the DSA core now properly configures all the links (as expected). I will send this out once Russell's patches are finalized and merged. Yours, Linus Walleij