• Vladimir Oltean's avatar
    net: dsa: make phylink-related OF properties mandatory on DSA and CPU ports · e09e9873
    Vladimir Oltean authored
    Early DSA drivers were kind of simplistic in that they assumed a fairly
    narrow hardware layout. User ports would have integrated PHYs at an
    internal MDIO address that is derivable from the port number, and shared
    (DSA and CPU) ports would have an MII-style (serial or parallel)
    connection to another MAC. Phylib and then phylink were used to drive
    the internal PHYs, and this needed little to no description through the
    platform data structures. Bringing up the shared ports at the maximum
    supported link speed was the responsibility of the drivers.
    
    As a result of this, when these early drivers were converted from
    platform data to the new DSA OF bindings, there was no link information
    translated into the first DT bindings.
    
    https://lore.kernel.org/all/YtXFtTsf++AeDm1l@lunn.ch/
    
    Later, phylink was adopted for shared ports as well, and today we have a
    workaround in place, introduced by commit a20f9970 ("net: dsa: Don't
    instantiate phylink for CPU/DSA ports unless needed"). There, DSA checks
    for the presence of phy-handle/fixed-link/managed OF properties, and if
    missing, phylink registration would be skipped. This is because phylink
    is optional for some drivers (the shared ports already work without it),
    but the process of starting to register a port with phylink is
    irreversible: if phylink_create() fails to find the fwnode properties it
    needs, it bails out and it leaves the ports inoperational (because
    phylink expects ports to be initially down, so DSA necessarily takes
    them down, and doesn't know how to put them back up again).
    
    DSA being a common framework, new drivers opt into this workaround
    willy-nilly, but the ideal behavior from the DSA core's side would have
    been to not interfere with phylink's process of failing at all. This
    isn't possible because of regression concerns with pre-phylink DT blobs,
    but at least DSA should put a stop to the proliferation of more of such
    cases that rely on the workaround to skip phylink registration, and
    sanitize the environment that new drivers work in.
    
    To that end, create a list of compatible strings for which the
    workaround is preserved, and don't apply the workaround for any drivers
    outside that list (this includes new drivers).
    
    In some cases, we make the assumption that even existing drivers don't
    rely on DSA's workaround, and we do this by looking at the device trees
    in which they appear. We can't fully know what is the situation with
    downstream DT blobs, but we can guess the overall trend by studying the
    DT blobs that were submitted upstream. If there are upstream blobs that
    have lacking descriptions, we take it as very likely that there are many
    more downstream blobs that do so too. If all upstream blobs have
    complete descriptions, we take that as a hint that the driver is a
    candidate for enforcing strict DT bindings (considering that most
    bindings are copy-pasted). If there are no upstream DT blobs, we take
    the conservative route of allowing the workaround, unless the driver
    maintainer instructs us otherwise.
    
    The driver situation is as follows:
    
    ar9331
    ~~~~~~
    
        compatible strings:
        - qca,ar9331-switch
    
        1 occurrence in mainline device trees, part of SoC dtsi
        (arch/mips/boot/dts/qca/ar9331.dtsi), description is not problematic.
    
        Verdict: opt into strict DT bindings and out of workarounds.
    
    b53
    ~~~
    
        compatible strings:
        - brcm,bcm5325
        - brcm,bcm53115
        - brcm,bcm53125
        - brcm,bcm53128
        - brcm,bcm5365
        - brcm,bcm5389
        - brcm,bcm5395
        - brcm,bcm5397
        - brcm,bcm5398
    
        - brcm,bcm53010-srab
        - brcm,bcm53011-srab
        - brcm,bcm53012-srab
        - brcm,bcm53018-srab
        - brcm,bcm53019-srab
        - brcm,bcm5301x-srab
        - brcm,bcm11360-srab
        - brcm,bcm58522-srab
        - brcm,bcm58525-srab
        - brcm,bcm58535-srab
        - brcm,bcm58622-srab
        - brcm,bcm58623-srab
        - brcm,bcm58625-srab
        - brcm,bcm88312-srab
        - brcm,cygnus-srab
        - brcm,nsp-srab
        - brcm,omega-srab
    
        - brcm,bcm3384-switch
        - brcm,bcm6328-switch
        - brcm,bcm6368-switch
        - brcm,bcm63xx-switch
    
        I've found at least these mainline DT blobs with problems:
    
        arch/arm/boot/dts/bcm47094-linksys-panamera.dts
        - lacks phy-mode
        arch/arm/boot/dts/bcm47189-tenda-ac9.dts
        - lacks phy-mode and fixed-link
        arch/arm/boot/dts/bcm47081-luxul-xap-1410.dts
        arch/arm/boot/dts/bcm47081-luxul-xwr-1200.dts
        arch/arm/boot/dts/bcm47081-buffalo-wzr-600dhp2.dts
        - lacks phy-mode and fixed-link
        arch/arm/boot/dts/bcm47094-luxul-xbr-4500.dts
        arch/arm/boot/dts/bcm4708-smartrg-sr400ac.dts
        arch/arm/boot/dts/bcm4708-luxul-xap-1510.dts
        arch/arm/boot/dts/bcm953012er.dts
        arch/arm/boot/dts/bcm4708-netgear-r6250.dts
        arch/arm/boot/dts/bcm4708-buffalo-wzr-1166dhp-common.dtsi
        arch/arm/boot/dts/bcm4708-luxul-xwc-1000.dts
        arch/arm/boot/dts/bcm47094-luxul-abr-4500.dts
        - lacks phy-mode and fixed-link
        arch/arm/boot/dts/bcm53016-meraki-mr32.dts
        - lacks phy-mode
    
        Verdict: opt into DSA workarounds.
    
    bcm_sf2
    ~~~~~~~
    
        compatible strings:
        - brcm,bcm4908-switch
        - brcm,bcm7445-switch-v4.0
        - brcm,bcm7278-switch-v4.0
        - brcm,bcm7278-switch-v4.8
    
        A single occurrence in mainline
        (arch/arm64/boot/dts/broadcom/bcm4908/bcm4908.dtsi), part of a SoC
        dtsi, valid description. Florian Fainelli explains that most of the
        bcm_sf2 device trees lack a full description for the internal IMP
        ports.
    
        Verdict: opt the BCM4908 into strict DT bindings, and opt the rest
        into the workarounds. Note that even though BCM4908 has strict DT
        bindings, it still does not register with phylink on the IMP port
        due to it implementing ->adjust_link().
    
    hellcreek
    ~~~~~~~~~
    
        compatible strings:
        - hirschmann,hellcreek-de1soc-r1
    
        No occurrence in mainline device trees. Kurt Kanzenbach explains
        that the downstream device trees lacked phy-mode and fixed link, and
        needed work, but were fixed in the meantime.
    
        Verdict: opt into strict DT bindings and out of workarounds.
    
    lan9303
    ~~~~~~~
    
        compatible strings:
        - smsc,lan9303-mdio
        - smsc,lan9303-i2c
    
        1 occurrence in mainline device trees:
        arch/arm/boot/dts/imx53-kp-hsc.dts
        - no phy-mode, no fixed-link
    
        Verdict: opt out of strict DT bindings and into workarounds.
    
    lantiq_gswip
    ~~~~~~~~~~~~
    
        compatible strings:
        - lantiq,xrx200-gswip
        - lantiq,xrx300-gswip
        - lantiq,xrx330-gswip
    
        No occurrences in mainline device trees. Martin Blumenstingl
        confirms that the downstream OpenWrt device trees lack a proper
        fixed-link and need work, and that the incomplete description can
        even be seen in the example from
        Documentation/devicetree/bindings/net/dsa/lantiq-gswip.txt.
    
        Verdict: opt out of strict DT bindings and into workarounds.
    
    microchip ksz
    ~~~~~~~~~~~~~
    
        compatible strings:
        - microchip,ksz8765
        - microchip,ksz8794
        - microchip,ksz8795
        - microchip,ksz8863
        - microchip,ksz8873
        - microchip,ksz9477
        - microchip,ksz9897
        - microchip,ksz9893
        - microchip,ksz9563
        - microchip,ksz8563
        - microchip,ksz9567
        - microchip,lan9370
        - microchip,lan9371
        - microchip,lan9372
        - microchip,lan9373
        - microchip,lan9374
    
        5 occurrences in mainline device trees, all descriptions are valid.
        But we had a snafu for the ksz8795 and ksz9477 drivers where the
        phy-mode property would be expected to be located directly under the
        'switch' node rather than under a port OF node. It was fixed by
        commit edecfa98 ("net: dsa: microchip: look for phy-mode in port
        nodes"). The driver still has compatibility with the old DT blobs.
        The lan937x support was added later than the above snafu was fixed,
        and even though it has support for the broken DT blobs by virtue of
        sharing a common probing function, I'll take it that its DT blobs
        are correct.
    
        Verdict: opt lan937x into strict DT bindings, and the others out.
    
    mt7530
    ~~~~~~
    
        compatible strings
        - mediatek,mt7621
        - mediatek,mt7530
        - mediatek,mt7531
    
        Multiple occurrences in mainline device trees, one is part of an SoC
        dtsi (arch/mips/boot/dts/ralink/mt7621.dtsi), all descriptions are fine.
    
        Verdict: opt into strict DT bindings and out of workarounds.
    
    mv88e6060
    ~~~~~~~~~
    
        compatible string:
        - marvell,mv88e6060
    
        no occurrences in mainline, nobody knows anybody who uses it.
    
        Verdict: opt out of strict DT bindings and into workarounds.
    
    mv88e6xxx
    ~~~~~~~~~
    
        compatible strings:
        - marvell,mv88e6085
        - marvell,mv88e6190
        - marvell,mv88e6250
    
        Device trees that have incomplete descriptions of CPU or DSA ports:
        arch/arm64/boot/dts/freescale/imx8mq-zii-ultra.dtsi
        - lacks phy-mode
        arch/arm64/boot/dts/marvell/cn9130-crb.dtsi
        - lacks phy-mode and fixed-link
        arch/arm/boot/dts/vf610-zii-ssmb-spu3.dts
        - lacks phy-mode
        arch/arm/boot/dts/kirkwood-mv88f6281gtw-ge.dts
        - lacks phy-mode
        arch/arm/boot/dts/vf610-zii-spb4.dts
        - lacks phy-mode
        arch/arm/boot/dts/vf610-zii-cfu1.dts
        - lacks phy-mode
        arch/arm/boot/dts/vf610-zii-dev-rev-c.dts
        - lacks phy-mode on CPU port, fixed-link on DSA ports
        arch/arm/boot/dts/vf610-zii-dev-rev-b.dts
        - lacks phy-mode on CPU port
        arch/arm/boot/dts/armada-381-netgear-gs110emx.dts
        - lacks phy-mode
        arch/arm/boot/dts/vf610-zii-scu4-aib.dts
        - lacks fixed-link on xgmii DSA ports and/or in-band-status on
          2500base-x DSA ports, and phy-mode on CPU port
        arch/arm/boot/dts/imx6qdl-gw5904.dtsi
        - lacks phy-mode and fixed-link
        arch/arm/boot/dts/armada-385-clearfog-gtr-l8.dts
        - lacks phy-mode and fixed-link
        arch/arm/boot/dts/vf610-zii-ssmb-dtu.dts
        - lacks phy-mode
        arch/arm/boot/dts/kirkwood-dir665.dts
        - lacks phy-mode
        arch/arm/boot/dts/kirkwood-rd88f6281.dtsi
        - lacks phy-mode
        arch/arm/boot/dts/orion5x-netgear-wnr854t.dts
        - lacks phy-mode and fixed-link
        arch/arm/boot/dts/armada-388-clearfog.dts
        - lacks phy-mode
        arch/arm/boot/dts/armada-xp-linksys-mamba.dts
        - lacks phy-mode
        arch/arm/boot/dts/armada-385-linksys.dtsi
        - lacks phy-mode
        arch/arm/boot/dts/imx6q-b450v3.dts
        arch/arm/boot/dts/imx6q-b850v3.dts
        - has a phy-handle but not a phy-mode?
        arch/arm/boot/dts/armada-370-rd.dts
        - lacks phy-mode
        arch/arm/boot/dts/kirkwood-linksys-viper.dts
        - lacks phy-mode
        arch/arm/boot/dts/imx51-zii-rdu1.dts
        - lacks phy-mode
        arch/arm/boot/dts/imx51-zii-scu2-mezz.dts
        - lacks phy-mode
        arch/arm/boot/dts/imx6qdl-zii-rdu2.dtsi
        - lacks phy-mode
        arch/arm/boot/dts/armada-385-clearfog-gtr-s4.dts
        - lacks phy-mode and fixed-link
    
        Verdict: opt out of strict DT bindings and into workarounds.
    
    ocelot
    ~~~~~~
    
        compatible strings:
        - mscc,vsc9953-switch
        - felix (arch/arm64/boot/dts/freescale/fsl-ls1028a.dtsi) is a PCI
          device, has no compatible string
    
        2 occurrences in mainline, both are part of SoC dtsi and complete.
    
        Verdict: opt into strict DT bindings and out of workarounds.
    
    qca8k
    ~~~~~
    
        compatible strings:
        - qca,qca8327
        - qca,qca8328
        - qca,qca8334
        - qca,qca8337
    
        5 occurrences in mainline device trees, none of the descriptions are
        problematic.
    
        Verdict: opt into strict DT bindings and out of workarounds.
    
    realtek
    ~~~~~~~
    
        compatible strings:
        - realtek,rtl8366rb
        - realtek,rtl8365mb
    
        2 occurrences in mainline, both descriptions are fine, additionally
        rtl8365mb.c has a comment "The device tree firmware should also
        specify the link partner of the extension port - either via a
        fixed-link or other phy-handle."
    
        Verdict: opt into strict DT bindings and out of workarounds.
    
    rzn1_a5psw
    ~~~~~~~~~~
    
        compatible strings:
        - renesas,rzn1-a5psw
    
        One single occurrence, part of SoC dtsi
        (arch/arm/boot/dts/r9a06g032.dtsi), description is fine.
    
        Verdict: opt into strict DT bindings and out of workarounds.
    
    sja1105
    ~~~~~~~
    
        Driver already validates its port OF nodes in
        sja1105_parse_ports_node().
    
        Verdict: opt into strict DT bindings and out of workarounds.
    
    vsc73xx
    ~~~~~~~
    
        compatible strings:
        - vitesse,vsc7385
        - vitesse,vsc7388
        - vitesse,vsc7395
        - vitesse,vsc7398
    
        2 occurrences in mainline device trees, both descriptions are fine.
    
        Verdict: opt into strict DT bindings and out of workarounds.
    
    xrs700x
    ~~~~~~~
    
        compatible strings:
        - arrow,xrs7003e
        - arrow,xrs7003f
        - arrow,xrs7004e
        - arrow,xrs7004f
    
        no occurrences in mainline, we don't know.
    
        Verdict: opt out of strict DT bindings and into workarounds.
    
    Because there is a pattern where newly added switches reuse existing
    drivers more often than introducing new ones, I've opted for deciding
    who gets to opt into the workaround based on an OF compatible match
    table in the DSA core. The alternative would have been to add another
    boolean property to struct dsa_switch, like configure_vlan_while_not_filtering.
    But this avoids situations where sometimes driver maintainers obfuscate
    what goes on by sharing a common probing function, and therefore making
    new switches inherit old quirks.
    
    Side note, we also warn about missing properties for drivers that rely
    on the workaround. This isn't an indication that we'll break
    compatibility with those DT blobs any time soon, but is rather done to
    raise awareness about the change, for future DT blob authors.
    
    Cc: Rob Herring <robh+dt@kernel.org>
    Cc: Frank Rowand <frowand.list@gmail.com>
    Acked-by: Alvin Šipraga <alsi@bang-olufsen.dk> # realtek
    Signed-off-by: default avatarVladimir Oltean <vladimir.oltean@nxp.com>
    Signed-off-by: default avatarJakub Kicinski <kuba@kernel.org>
    e09e9873
port.c 45.7 KB