| Message ID | 20230216075321.2898003-1-colin.foster@in-advantage.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | add support for ocelot external ports | expand |
On Wed, Feb 15, 2023 at 11:53:14PM -0800, Colin Foster wrote: > Part 3 will, at a minimum, add support for ports 4-7, which are > configured to use QSGMII to an external phy (Return Of The QSGMII). With > any luck, and some guidance, support for SGMII, SFPs, etc. will also be > part of this series. > > > This patch series is absolutely an RFC at this point. While all 8 copper > ports on the VSC7512 are currently functional, I recognize there are a > couple empty function callbacks in the last patch that likely need to be > implemented. > ... > > Also, with patch 7 ("net: dsa: ocelot_ext: add support for external phys") > my basis was the function mscc_ocelot_init_ports(), but there were several > changes I had to make for DSA / Phylink. Are my implementations of > ocelot_ext_parse_port_node() and ocelot_ext_phylink_create() barking up > the right tree? DSA already creates phylink instances per DSA port, and provides many of the phylink MAC operations to the DSA driver via the .phylink_* operations in the dsa_switch_ops structure, and this phylink instance should be used for managing the status and configuring the port according to phylink's callbacks. The core felix code already makes use of this, implementing the mac_link_down() and mac_link_up() operations to handle when the link comes up or goes down. I don't see why one would need to create a separate phylink instance to support external PHYs, SFPs, etc on a DSA switch. The phylink instance created by DSA is there for the DSA driver to make use of for the port, and should be sufficient for this. I think if you use the DSA-created phylink instance, then you don't need any of patch 6. I'm not yet convinced that you need anything from patch 7, but maybe you could explain what patch 7 provides that the existing felix phylink implementation doesn't already provide. I do get the impression that the use of the PCS instance in patch 7 is an attempt to work around the use of a private instance, redirecting the pcs_config and pcs_link_up methods to the corresponding MAC operations as a workaround for having the private instance. It looks like you need to hook into the mac_config(), mac_link_up() and mac_link_down() methods at the core felix layer, so I would suggest looking at the felix_info structure, adding methods there for each of these, and arranging for the core felix code to forward these calls down to the implementation as required.
Hi Russell, On Thu, Feb 16, 2023 at 12:14:38PM +0000, Russell King (Oracle) wrote: > On Wed, Feb 15, 2023 at 11:53:14PM -0800, Colin Foster wrote: > > Part 3 will, at a minimum, add support for ports 4-7, which are > > configured to use QSGMII to an external phy (Return Of The QSGMII). With > > any luck, and some guidance, support for SGMII, SFPs, etc. will also be > > part of this series. > > > > > > This patch series is absolutely an RFC at this point. While all 8 copper > > ports on the VSC7512 are currently functional, I recognize there are a > > couple empty function callbacks in the last patch that likely need to be > > implemented. > > > ... > > > > Also, with patch 7 ("net: dsa: ocelot_ext: add support for external phys") > > my basis was the function mscc_ocelot_init_ports(), but there were several > > changes I had to make for DSA / Phylink. Are my implementations of > > ocelot_ext_parse_port_node() and ocelot_ext_phylink_create() barking up > > the right tree? > > DSA already creates phylink instances per DSA port, and provides many > of the phylink MAC operations to the DSA driver via the .phylink_* > operations in the dsa_switch_ops structure, and this phylink instance > should be used for managing the status and configuring the port > according to phylink's callbacks. The core felix code already makes > use of this, implementing the mac_link_down() and mac_link_up() > operations to handle when the link comes up or goes down. > > I don't see why one would need to create a separate phylink instance > to support external PHYs, SFPs, etc on a DSA switch. The phylink > instance created by DSA is there for the DSA driver to make use of > for the port, and should be sufficient for this. This is essentially the feedback I was looking for. "This looks wrong" which means I'll take a step back. > > I think if you use the DSA-created phylink instance, then you don't > need any of patch 6. I'm not yet convinced that you need anything > from patch 7, but maybe you could explain what patch 7 provides that > the existing felix phylink implementation doesn't already provide. I'll have to go through it again to remember exactly what I was up against - it was a while ago now. All of the logic was based on the logic in ocelot_port_phylink_create() - which is part of the vsc7514 switchdev implementation (a chip that is essentially identical, except for an internal MIPS instead of external SPI control) I believe the main gotcha was that control over the phy itself, by way of phy_set_mode_ext(). That needed the 'struct device_node *portnp' .... Keeps looking .... Ahh, yes. Regmaps and regfields aren't initialized at the time of dt parsing in felix. And the MDIO bus isn't allocated until after that. That's the reason for patch 6 parse_port_node() - I need the tree node to get MDIO access to the phy, which I don't have until I'm done parsing the tree... There might be a cleaner way for me to do that. I'm tiptoeing a little bit to avoid any regressions with the felix_vsc9959 or seville_vsc9953. > I do get the impression that the use of the PCS instance in patch 7 > is an attempt to work around the use of a private instance, > redirecting the pcs_config and pcs_link_up methods to the > corresponding MAC operations as a workaround for having the private > instance. I'm not convinced I don't need PCS here, and just have things working wrong. The configuration looks like this: |------------------------------------------------| | CPU | |------------------------------------------------| | SPI | |------------------------------------------------| | VSC7512 | |------------------------------------------------| || || || || | |-------| |-------| |-------| |-------| | | port4 | | port5 | | port6 | | port7 | | |-------| |-------| |-------| |-------| | || | QSGMII MDIO || | |------------------------------------------------| | VSC8512 | |------------------------------------------------| || || || || |-------| |-------| |-------| |-------| | sw0p4 | | sw0p5 | | sw0p6 | | sw0p7 | |-------| |-------| |-------| |-------| Would phylink_pcs need to get involved in the QSGMII link at all, or should the phylnk from dsa_port_phylink_create() be all that's needed? > > It looks like you need to hook into the mac_config(), mac_link_up() > and mac_link_down() methods at the core felix layer, so I would > suggest looking at the felix_info structure, adding methods there for > each of these, and arranging for the core felix code to forward these > calls down to the implementation as required. Yes, I'll look at how I can clean things up. I greatly appreciate your feedback! > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
On Thu, Feb 16, 2023 at 04:42:06PM -0800, Colin Foster wrote: > I believe the main gotcha was that control over the phy itself, by way > of phy_set_mode_ext(). That needed the 'struct device_node *portnp' DT parsing in felix_parse_dt() is not the only DT parsing that is done, and certainly nothing depends on it in the way you describe. dsa_switch_parse_of() also parses the device tree. felix_parse_dt() only exists because the SERDES/PCS drivers from NXP LS1028A do not support dynamic reconfiguration of the SERDES protocol. So we parse the device tree to set the initial ocelot_port->phy_mode, and then (with the current phylink API) we populate phylink's config->supported_interfaces with just that one bit set, to prevent SERDES protocol changes. Do not get too hung up on this parsing (unless you believe you could simplify the code by removing it; case in which I'd be interested if you had patches in this area). Each port's device_node is also available in struct dsa_port :: dn. > > .... Keeps looking .... > > Ahh, yes. Regmaps and regfields aren't initialized at the time of > dt parsing in felix. And the MDIO bus isn't allocated until after that. > That's the reason for patch 6 parse_port_node() - I need the tree node > to get MDIO access to the phy, which I don't have until I'm done parsing > the tree... Nope. Device tree parsing in DSA is done from dsa_register_switch(), and dsa_switch_ops :: setup() (aka felix_setup()) is the first callback in which the information is reliably available. You can *easily* call phy_set_mode_ext() from the "setup()" callback. In fact, you're already doing that. Not sure what the problem seems to be. It doesn't seem to be an ordering problem between phy_set_mode_ext() and phylink_create() either, because DSA calls phylink_create() after both the dsa_switch_ops :: setup() as well as port_setup() callbacks. So there should be plenty of opportunity for you to prepare. > There might be a cleaner way for me to do that. I'm tiptoeing a little > bit to avoid any regressions with the felix_vsc9959 or seville_vsc9953. I can test those. I'd much prefer if you made an initial effort to keep a relatively consistent code structure.
On Fri, Feb 17, 2023 at 03:11:55AM +0200, Vladimir Oltean wrote: > On Thu, Feb 16, 2023 at 04:42:06PM -0800, Colin Foster wrote: > > I believe the main gotcha was that control over the phy itself, by way > > of phy_set_mode_ext(). That needed the 'struct device_node *portnp' > > DT parsing in felix_parse_dt() is not the only DT parsing that is done, > and certainly nothing depends on it in the way you describe. > > dsa_switch_parse_of() also parses the device tree. felix_parse_dt() only > exists because the SERDES/PCS drivers from NXP LS1028A do not support > dynamic reconfiguration of the SERDES protocol. So we parse the device > tree to set the initial ocelot_port->phy_mode, and then (with the > current phylink API) we populate phylink's config->supported_interfaces > with just that one bit set, to prevent SERDES protocol changes. > > Do not get too hung up on this parsing (unless you believe you could > simplify the code by removing it; case in which I'd be interested if you > had patches in this area). Each port's device_node is also available in > struct dsa_port :: dn. > > > > > .... Keeps looking .... > > > > Ahh, yes. Regmaps and regfields aren't initialized at the time of > > dt parsing in felix. And the MDIO bus isn't allocated until after that. > > That's the reason for patch 6 parse_port_node() - I need the tree node > > to get MDIO access to the phy, which I don't have until I'm done parsing > > the tree... > > Nope. Device tree parsing in DSA is done from dsa_register_switch(), and > dsa_switch_ops :: setup() (aka felix_setup()) is the first callback in > which the information is reliably available. > > You can *easily* call phy_set_mode_ext() from the "setup()" callback. > In fact, you're already doing that. Not sure what the problem seems to be. > It doesn't seem to be an ordering problem between phy_set_mode_ext() and > phylink_create() either, because DSA calls phylink_create() after both > the dsa_switch_ops :: setup() as well as port_setup() callbacks. > So there should be plenty of opportunity for you to prepare. Thank you Vladimir and Russel for the nudge in the right direction! I made this a lot harder on myself than it needed to be. Yes, I can drop patches 6 and 7 (they felt pretty wrong as I was writing them), implement phylink_mac_config() in felix_switch_ops, and do the phy configuration using the dsa port's dp->dn node. I'll probably send out another RFC in the next couple weeks with the much better implementation. Then hopefully it'll be ready to be upgraded to patch status after the merge window. (I hope I don't regret saying that)
This is the start of part 3 of what is hopefully a 3-part series to add Ethernet switching support to Ocelot chips. Part 1 of the series (A New Chip) added general support for Ocelot chips that were controlled externally via SPI. https://lore.kernel.org/all/20220815005553.1450359-1-colin.foster@in-advantage.com/ Part 2 of the series (The Ethernet Strikes Back) added DSA Ethernet support for ports 0-3, which are the four copper ports that are internal to the chip. https://lore.kernel.org/all/20230127193559.1001051-1-colin.foster@in-advantage.com/ Part 3 will, at a minimum, add support for ports 4-7, which are configured to use QSGMII to an external phy (Return Of The QSGMII). With any luck, and some guidance, support for SGMII, SFPs, etc. will also be part of this series. This patch series is absolutely an RFC at this point. While all 8 copper ports on the VSC7512 are currently functional, I recognize there are a couple empty function callbacks in the last patch that likely need to be implemented. Aside from that, there is feedback I'd greatly appreciate. Specifically patch 6 ("net: dsa: felix: allow external parsing of port nodes") and whether that is an acceptable way to solve the problem at hand. Also, with patch 7 ("net: dsa: ocelot_ext: add support for external phys") my basis was the function mscc_ocelot_init_ports(), but there were several changes I had to make for DSA / Phylink. Are my implementations of ocelot_ext_parse_port_node() and ocelot_ext_phylink_create() barking up the right tree? For reference, a boot log in case it is useful: [ 3.222208] pinctrl-ocelot ocelot-pinctrl.0.auto: DMA mask not set [ 3.231781] pinctrl-ocelot ocelot-pinctrl.0.auto: driver registered [ 3.241747] pinctrl-microchip-sgpio ocelot-sgpio.1.auto: DMA mask not set [ 3.260366] mscc-miim ocelot-miim0.2.auto: DMA mask not set [ 3.787309] mscc-miim ocelot-miim1.3.auto: DMA mask not set [ 3.822367] mscc,ocelot-serdes ocelot-serdes.4.auto: DMA mask not set [ 3.837637] ocelot-switch ocelot-switch.5.auto: DMA mask not set [ 5.368119] ocelot-switch ocelot-switch.5.auto: PHY [ocelot-miim1.3.auto-mii:04] driver [Microsemi GE VSC8514 SyncE] (irq=POLL) [ 5.668093] ocelot-switch ocelot-switch.5.auto: PHY [ocelot-miim1.3.auto-mii:05] driver [Microsemi GE VSC8514 SyncE] (irq=POLL) [ 5.968342] ocelot-switch ocelot-switch.5.auto: PHY [ocelot-miim1.3.auto-mii:06] driver [Microsemi GE VSC8514 SyncE] (irq=POLL) [ 6.288098] ocelot-switch ocelot-switch.5.auto: PHY [ocelot-miim1.3.auto-mii:07] driver [Microsemi GE VSC8514 SyncE] (irq=POLL) [ 6.364114] ocelot-switch ocelot-switch.5.auto: PHY [ocelot-miim0.2.auto-mii:00] driver [Generic PHY] (irq=POLL) [ 6.375061] ocelot-switch ocelot-switch.5.auto: configuring for phy/internal link mode [ 6.391843] ocelot-switch ocelot-switch.5.auto swp1 (uninitialized): PHY [ocelot-miim0.2.auto-mii:01] driver [Generic PHY] (irq=POLL) [ 6.410033] ocelot-switch ocelot-switch.5.auto swp2 (uninitialized): PHY [ocelot-miim0.2.auto-mii:02] driver [Generic PHY] (irq=POLL) [ 6.427761] ocelot-switch ocelot-switch.5.auto swp3 (uninitialized): PHY [ocelot-miim0.2.auto-mii:03] driver [Generic PHY] (irq=POLL) [ 6.598305] ocelot-switch ocelot-switch.5.auto swp4 (uninitialized): PHY [ocelot-miim1.3.auto-mii:04] driver [Microsemi GE VSC8514 SyncE] (irq=POLL) [ 6.778331] ocelot-switch ocelot-switch.5.auto swp5 (uninitialized): PHY [ocelot-miim1.3.auto-mii:05] driver [Microsemi GE VSC8514 SyncE] (irq=POLL) [ 6.958337] ocelot-switch ocelot-switch.5.auto swp6 (uninitialized): PHY [ocelot-miim1.3.auto-mii:06] driver [Microsemi GE VSC8514 SyncE] (irq=POLL) [ 7.138379] ocelot-switch ocelot-switch.5.auto swp7 (uninitialized): PHY [ocelot-miim1.3.auto-mii:07] driver [Microsemi GE VSC8514 SyncE] (irq=POLL) [ 7.156375] device eth0 entered promiscuous mode [ 7.161140] DSA: tree 0 setup And a couple device tree snippets of the ports: &spi0 { #address-cells = <1>; #size-cells = <0>; status = "okay"; soc@0 { compatible = "mscc,vsc7512"; spi-max-frequency = <2500000>; reg = <0 0>; #address-cells = <1>; #size-cells = <1>; ethernet-switch@71010000 { compatible = "mscc,vsc7512-switch"; ... ports { ... port@4 { reg = <4>; label = "swp4"; status = "okay"; phy-handle = <&sw_phy4>; phy-mode = "qsgmii"; phys = <&serdes 4 SERDES6G(0)>; }; ... }; ... }; mdio@710700c0 { compatible = "mscc,ocelot-miim"; pinctrl-names = "default"; pinctrl-0 = <&miim1>; #address-cells = <1>; #size-cells = <0>; reg = <0x710700c0 0x24>; sw_phy4: ethernet-phy@4 { reg = <0x4>; }; ... }; ... serdes: serdes { compatible = "mscc,vsc7514-serdes"; #phy-cells = <2>; }; }; }; Colin Foster (7): phy: phy-ocelot-serdes: add ability to be used in a non-syscon configuration mfd: ocelot: add ocelot-serdes capability net: mscc: ocelot: expose ocelot_pll5_init routine net: mscc: ocelot: expose generic phylink_mac_config routine net: dsa: felix: attempt to initialize internal hsio plls net: dsa: felix: allow external parsing of port nodes net: dsa: ocelot_ext: add support for external phys drivers/mfd/ocelot-core.c | 13 + drivers/net/dsa/ocelot/felix.c | 57 +++- drivers/net/dsa/ocelot/felix.h | 6 + drivers/net/dsa/ocelot/ocelot_ext.c | 319 ++++++++++++++++++++- drivers/net/ethernet/mscc/ocelot.c | 57 ++++ drivers/net/ethernet/mscc/ocelot_net.c | 21 +- drivers/net/ethernet/mscc/ocelot_vsc7514.c | 30 -- drivers/phy/mscc/phy-ocelot-serdes.c | 9 + include/soc/mscc/ocelot.h | 5 + 9 files changed, 440 insertions(+), 77 deletions(-)