| Message ID | 20210710192602.2186370-1-colin.foster@in-advantage.com |
|---|---|
| Headers | show |
| Series | Add support for VSC7511-7514 chips over SPI | expand |
On 10/07/2021 12:25:54-0700, Colin Foster wrote: > Add support for configuration and control of the VSC7511, VSC7512, VSC7513, and > VSC7514 chips over a SPI interface. The intent is to control these chips from an > external CPU. The expectation is to have most of the features of the > net/ethernet/mscc/ocelot_vsc7514 driver. > > I have tried to heed all the advice from my first patch RFC. Thanks to everyone > for all the feedback. > > The current status is that there are two functional "bugs" that need > investigation: > 1. The first probe of the internal MDIO bus fails. I suspect this is related to > power supplies / grounding issues that would not appear on standard hardware. Did you properly reset the internal phys? mdio-mscc-miim.c does that.
On Sat, Jul 10, 2021 at 12:26:02PM -0700, Colin Foster wrote: > Signed-off-by: Colin Foster <colin.foster@in-advantage.com> > --- > .../devicetree/bindings/net/dsa/ocelot.txt | 68 +++++++++++++++++++ > 1 file changed, 68 insertions(+) > > diff --git a/Documentation/devicetree/bindings/net/dsa/ocelot.txt b/Documentation/devicetree/bindings/net/dsa/ocelot.txt > index 7a271d070b72..f5d05bf8b093 100644 > --- a/Documentation/devicetree/bindings/net/dsa/ocelot.txt > +++ b/Documentation/devicetree/bindings/net/dsa/ocelot.txt > @@ -8,6 +8,7 @@ Currently the switches supported by the felix driver are: > > - VSC9959 (Felix) > - VSC9953 (Seville) > +- VSC7511, VSC7512, VSC7513, VSC7514 via SPI > > The VSC9959 switch is found in the NXP LS1028A. It is a PCI device, part of the > larger ENETC root complex. As a result, the ethernet-switch node is a sub-node > @@ -211,3 +212,70 @@ Example: > }; > }; > }; > + > +The VSC7513 and VSC7514 switches can be controlled internally via the MIPS > +processor. The VSC7511 and VSC7512 don't have this internal processor, but all > +four chips can be controlled externally through SPI with the following required > +properties: > + > +- compatible: > + Can be "mscc,vsc7511", "mscc,vsc7512", "mscc,vsc7513", or > + "mscc,vsc7514". > + > +Supported phy modes for all chips are: > + > +* phy_mode = "internal": on ports 0, 1, 2, 3 > + > +Additionally, the VSC7512 and VSC7514 support SGMII and QSGMII on various ports, > +though that is currently untested. > + > +Example for control from a BeagleBone Black > + > +&spi0 { > + #address-cells = <1>; > + #size-cells = <0>; > + status = "okay"; > + > + vsc7512: vsc7512@0 { ethernet-switch@0 > + compatible = "mscc,vsc7512"; > + spi-max-frequency = <250000>; > + reg = <0>; > + > + ports { > + #address-cells = <1>; > + #size-cells = <0>; > + > + port@0 { > + reg = <0>; > + ethernet = <&mac>; > + phy-mode = "internal"; > + > + fixed-link { > + speed = <100>; > + full-duplex; > + }; > + }; > + > + port@1 { > + reg = <1>; > + label = "swp1"; > + status = "okay"; I am not convinced that the status = "okay" lines are useful in the example. > + phy-mode = "internal"; This syntax is ambiguous and does not obviously mean that the port has an internal copper PHY. Please see this discussion for other meanings of no 'phy-handle' and no 'fixed-link'. https://www.mail-archive.com/u-boot@lists.denx.de/msg409571.html I think it would be in the best interest of everyone to go through phylink_of_phy_connect() instead of phylink_connect_phy(), aka use the standard phy-handle property and create an mdio node under ethernet-switch@0 where the internal PHY OF nodes are defined. I don't know if this is true for VSC7512 or not, but for example on NXP SJA1110, the internal PHYs can be accessed in 2 modes: (a) through SPI transfers (b) through an MDIO slave access point exposed by the switch chip, which can be connected to an external MDIO controller Some boards will use method (a), and others will use method (b). Requiring a phy-handle under the port property is an absolutely generic way to seamlessly deal with both cases. In case (a), the phy-handle points to a child of an MDIO bus provided by the ocelot driver, in case (b) the phy-handle points to a child provided by some other MDIO controller driver. > + }; > + > + port@2 { > + reg = <2>; > + label = "swp2"; > + status = "okay"; > + phy-mode = "internal"; > + }; > + > + port@3 { > + reg = <3>; > + label = "swp3"; > + status = "okay"; > + phy-mode = "internal"; > + }; > + }; > + }; > +}; > -- > 2.25.1 >
On Sat, Jul 10, 2021 at 12:26:01PM -0700, Colin Foster wrote: > +static const struct felix_info ocelot_spi_info = { > + .target_io_res = vsc7512_target_io_res, > + .port_io_res = vsc7512_port_io_res, > + .regfields = vsc7512_regfields, > + .map = vsc7512_regmap, > + .ops = &vsc7512_ops, > + .stats_layout = vsc7512_stats_layout, > + .num_stats = ARRAY_SIZE(vsc7512_stats_layout), > + .vcap = vsc7512_vcap_props, > + .num_mact_rows = 1024, > + > + /* The 7512 and 7514 both have support for up to 10 ports. The 7511 and > + * 7513 have support for 4. Due to lack of hardware to test and > + * validate external phys, this is currently limited to 4 ports. > + * Expanding this to 10 for the 7512 and 7514 and defining the > + * appropriate phy-handle values in the device tree should be possible. > + */ > + .num_ports = 4, Ouch, this was probably not a good move. felix_setup() -> felix_init_structs sets ocelot->num_phys_ports based on this value. If you search for ocelot->num_phys_ports in ocelot and in felix, it is widely used to denote "the index of the CPU port module within the analyzer block", since the CPU port module's number is equal to the number of the last physical port + 1. If VSC7512 has 10 ports, then the CPU port module is port 10, and if you set num_ports to 4 you will cause the driver to misbehave. > + .num_tx_queues = OCELOT_NUM_TC, > + .mdio_bus_alloc = felix_mdio_bus_alloc, > + .mdio_bus_free = felix_mdio_bus_free, > + .phylink_validate = vsc7512_phylink_validate, > + .prevalidate_phy_mode = vsc7512_prevalidate_phy_mode, > + .port_setup_tc = vsc7512_port_setup_tc, > + .init_regmap = vsc7512_regmap_init, > +}; > + /* Not sure about this */ > + ocelot->num_flooding_pgids = 1; Why are you not sure? It's the same as ocelot.
On Sat, Jul 10, 2021 at 10:47:55PM +0300, Vladimir Oltean wrote: > On Sat, Jul 10, 2021 at 12:25:54PM -0700, Colin Foster wrote: > > 1. The first probe of the internal MDIO bus fails. I suspect this is related to > > power supplies / grounding issues that would not appear on standard hardware. > > Where and with what error code does it fail exactly? I don't understand > the concept of "the first probe"? You mean that there is an > -EPROBE_DEFER and it tries again immediately afterwards? Or you need to > unbind and rebind the driver to the device, or what? My hardware I'm using for test / dev is a bit of a hack. Beaglebone jumped to a VSC7512 dev board SPI lines after some modifications. I believe I'm seeing grounding-related issues as a result of this. For instance, if I power on the dev board before powering on the Beaglebone, the BB will be held in reset. The same thing will happen if I reset the BB when the Microsemi dev board has been powered on, but left unconfigured. When I run "modprobe mscc_ocelot_spi" the first time the driver fails to enumerate swp1-3 with "failed to connect to port X: -19" message. But after doing that, I can reset the BB to my heart's content. Future boots will be able to successfully find those at initial mod insertion. And reloading / rebinding the driver successfully registers the ports at spi0.0-imdio:0X > > > 2. Communication to the CPU bus doesn't seem to function properly. I suspect > > this is due to the fact that ocelot / felix assumes it is using the built-in CPU > > / NPI port for forwarding, though I am not positive. > > What is the CPU bus and what doesn't function properly about it? I'm still learning a lot about how the chip functions, especially now that I have attained some functionality from the driver. There's a CPU bus that can be utilized when you're running internally, and I believe that can be configured as the NPI bus if you're running through PCIe. When controlling through SPI I'm not convinced we'll have access to that bus, and if the Ocelot driver relies on that functionality I've got some more work in front of me. > > > Nonetheless, these two issues likely won't require a large architecture change, > > and perhaps those who know much more about the ocelot chips than I could chime > > in. > > I guess you don't know if that's the case or not until you know what the > problem is... True
On Sat, Jul 10, 2021 at 10:01:45PM +0200, Alexandre Belloni wrote: > On 10/07/2021 12:25:54-0700, Colin Foster wrote: > > Add support for configuration and control of the VSC7511, VSC7512, VSC7513, and > > VSC7514 chips over a SPI interface. The intent is to control these chips from an > > external CPU. The expectation is to have most of the features of the > > net/ethernet/mscc/ocelot_vsc7514 driver. > > > > I have tried to heed all the advice from my first patch RFC. Thanks to everyone > > for all the feedback. > > > > The current status is that there are two functional "bugs" that need > > investigation: > > 1. The first probe of the internal MDIO bus fails. I suspect this is related to > > power supplies / grounding issues that would not appear on standard hardware. > > Did you properly reset the internal phys? mdio-mscc-miim.c does that. That code looks familiar to what I wrote, but the mdio-mscc-miim.c implementation has the 500ms delay. That might be all I need...
On Sat, Jul 10, 2021 at 11:34:11PM +0300, Vladimir Oltean wrote: > On Sat, Jul 10, 2021 at 12:26:02PM -0700, Colin Foster wrote: > > Signed-off-by: Colin Foster <colin.foster@in-advantage.com> > > --- > > .../devicetree/bindings/net/dsa/ocelot.txt | 68 +++++++++++++++++++ > > 1 file changed, 68 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/net/dsa/ocelot.txt b/Documentation/devicetree/bindings/net/dsa/ocelot.txt > > index 7a271d070b72..f5d05bf8b093 100644 > > --- a/Documentation/devicetree/bindings/net/dsa/ocelot.txt > > +++ b/Documentation/devicetree/bindings/net/dsa/ocelot.txt > > @@ -8,6 +8,7 @@ Currently the switches supported by the felix driver are: > > > > - VSC9959 (Felix) > > - VSC9953 (Seville) > > +- VSC7511, VSC7512, VSC7513, VSC7514 via SPI > > > > The VSC9959 switch is found in the NXP LS1028A. It is a PCI device, part of the > > larger ENETC root complex. As a result, the ethernet-switch node is a sub-node > > @@ -211,3 +212,70 @@ Example: > > }; > > }; > > }; > > + > > +The VSC7513 and VSC7514 switches can be controlled internally via the MIPS > > +processor. The VSC7511 and VSC7512 don't have this internal processor, but all > > +four chips can be controlled externally through SPI with the following required > > +properties: > > + > > +- compatible: > > + Can be "mscc,vsc7511", "mscc,vsc7512", "mscc,vsc7513", or > > + "mscc,vsc7514". > > + > > +Supported phy modes for all chips are: > > + > > +* phy_mode = "internal": on ports 0, 1, 2, 3 > > + > > +Additionally, the VSC7512 and VSC7514 support SGMII and QSGMII on various ports, > > +though that is currently untested. > > + > > +Example for control from a BeagleBone Black > > + > > +&spi0 { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + status = "okay"; > > + > > + vsc7512: vsc7512@0 { > > ethernet-switch@0 > > > + compatible = "mscc,vsc7512"; > > + spi-max-frequency = <250000>; > > + reg = <0>; > > + > > + ports { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + port@0 { > > + reg = <0>; > > + ethernet = <&mac>; > > + phy-mode = "internal"; > > + > > + fixed-link { > > + speed = <100>; > > + full-duplex; > > + }; > > + }; > > + > > + port@1 { > > + reg = <1>; > > + label = "swp1"; > > + status = "okay"; > > I am not convinced that the status = "okay" lines are useful in the > example. Fair enough > > > + phy-mode = "internal"; > > This syntax is ambiguous and does not obviously mean that the port has > an internal copper PHY. Please see this discussion for other meanings of > no 'phy-handle' and no 'fixed-link'. > > https://www.mail-archive.com/u-boot@lists.denx.de/msg409571.html > > I think it would be in the best interest of everyone to go through > phylink_of_phy_connect() instead of phylink_connect_phy(), aka use the > standard phy-handle property and create an mdio node under > ethernet-switch@0 where the internal PHY OF nodes are defined. > > I don't know if this is true for VSC7512 or not, but for example on > NXP SJA1110, the internal PHYs can be accessed in 2 modes: > (a) through SPI transfers > (b) through an MDIO slave access point exposed by the switch chip, which > can be connected to an external MDIO controller > > Some boards will use method (a), and others will use method (b). > > Requiring a phy-handle under the port property is an absolutely generic > way to seamlessly deal with both cases. In case (a), the phy-handle > points to a child of an MDIO bus provided by the ocelot driver, in case > (b) the phy-handle points to a child provided by some other MDIO > controller driver. > Yes, the Ocelot chips have the same functionality with the indirect / direct access. It seems like this would be coupled with the other discussion of updating mdio-mscc-miim.c so that the MDIO bus can be defined. And thank you for pointing out some examples. Having some starting points really helps! > > + }; > > + > > + port@2 { > > + reg = <2>; > > + label = "swp2"; > > + status = "okay"; > > + phy-mode = "internal"; > > + }; > > + > > + port@3 { > > + reg = <3>; > > + label = "swp3"; > > + status = "okay"; > > + phy-mode = "internal"; > > + }; > > + }; > > + }; > > +}; > > -- > > 2.25.1 > >
On Sat, Jul 10, 2021 at 11:52:05PM +0300, Vladimir Oltean wrote: > On Sat, Jul 10, 2021 at 12:26:01PM -0700, Colin Foster wrote: > > +static const struct felix_info ocelot_spi_info = { > > + .target_io_res = vsc7512_target_io_res, > > + .port_io_res = vsc7512_port_io_res, > > + .regfields = vsc7512_regfields, > > + .map = vsc7512_regmap, > > + .ops = &vsc7512_ops, > > + .stats_layout = vsc7512_stats_layout, > > + .num_stats = ARRAY_SIZE(vsc7512_stats_layout), > > + .vcap = vsc7512_vcap_props, > > + .num_mact_rows = 1024, > > + > > + /* The 7512 and 7514 both have support for up to 10 ports. The 7511 and > > + * 7513 have support for 4. Due to lack of hardware to test and > > + * validate external phys, this is currently limited to 4 ports. > > + * Expanding this to 10 for the 7512 and 7514 and defining the > > + * appropriate phy-handle values in the device tree should be possible. > > + */ > > + .num_ports = 4, > > Ouch, this was probably not a good move. > felix_setup() -> felix_init_structs sets ocelot->num_phys_ports based on > this value. > If you search for ocelot->num_phys_ports in ocelot and in felix, it is > widely used to denote "the index of the CPU port module within the > analyzer block", since the CPU port module's number is equal to the > number of the last physical port + 1. If VSC7512 has 10 ports, then the > CPU port module is port 10, and if you set num_ports to 4 you will cause > the driver to misbehave. Yes, this is part of my concern with the CPU / NPI module mentioned before. In my hardware, I'd have port 0 plugged to the external CPU. In Ocelot it is the internal bus, and in Felix it is the NPI. In this SPI design, does the driver lose significant functionality by not having access to those ports? In my test setup (and our expected production) we'd have port 0 connected to the external chip, and ports 1-3 exposed. Does Ocelot need to be modified to allow a parameter for the CPU port? And obviously I'd imagine this would want to be done in such a way that it doesn't break existing device trees... > > > + .num_tx_queues = OCELOT_NUM_TC, > > + .mdio_bus_alloc = felix_mdio_bus_alloc, > > + .mdio_bus_free = felix_mdio_bus_free, > > + .phylink_validate = vsc7512_phylink_validate, > > + .prevalidate_phy_mode = vsc7512_prevalidate_phy_mode, > > + .port_setup_tc = vsc7512_port_setup_tc, > > + .init_regmap = vsc7512_regmap_init, > > +}; > > > + /* Not sure about this */ > > + ocelot->num_flooding_pgids = 1; > > Why are you not sure? It's the same as ocelot. Sorry - missed removing that comment... Removed.
On 11/07/2021 10:09:27-0700, Colin Foster wrote: > On Sat, Jul 10, 2021 at 11:52:05PM +0300, Vladimir Oltean wrote: > > On Sat, Jul 10, 2021 at 12:26:01PM -0700, Colin Foster wrote: > > > +static const struct felix_info ocelot_spi_info = { > > > + .target_io_res = vsc7512_target_io_res, > > > + .port_io_res = vsc7512_port_io_res, > > > + .regfields = vsc7512_regfields, > > > + .map = vsc7512_regmap, > > > + .ops = &vsc7512_ops, > > > + .stats_layout = vsc7512_stats_layout, > > > + .num_stats = ARRAY_SIZE(vsc7512_stats_layout), > > > + .vcap = vsc7512_vcap_props, > > > + .num_mact_rows = 1024, > > > + > > > + /* The 7512 and 7514 both have support for up to 10 ports. The 7511 and > > > + * 7513 have support for 4. Due to lack of hardware to test and > > > + * validate external phys, this is currently limited to 4 ports. > > > + * Expanding this to 10 for the 7512 and 7514 and defining the > > > + * appropriate phy-handle values in the device tree should be possible. > > > + */ > > > + .num_ports = 4, > > > > Ouch, this was probably not a good move. > > felix_setup() -> felix_init_structs sets ocelot->num_phys_ports based on > > this value. > > If you search for ocelot->num_phys_ports in ocelot and in felix, it is > > widely used to denote "the index of the CPU port module within the > > analyzer block", since the CPU port module's number is equal to the > > number of the last physical port + 1. If VSC7512 has 10 ports, then the > > CPU port module is port 10, and if you set num_ports to 4 you will cause > > the driver to misbehave. > > Yes, this is part of my concern with the CPU / NPI module mentioned > before. In my hardware, I'd have port 0 plugged to the external CPU. In > Ocelot it is the internal bus, and in Felix it is the NPI. In this SPI > design, does the driver lose significant functionality by not having > access to those ports? > From the switchdev driver perspective, the CPU port is special because it is the one allowing to send and receive frames to/from the exposed ethernet interfaces. However, the goal is definitively to use that as little as possible (especially since as implemented right now, throughput is about 20Mbps). I didn't have a look at the DSA implementation but I wouldn't expect the NPI port to be that special. > In my test setup (and our expected production) we'd have port 0 > connected to the external chip, and ports 1-3 exposed. Does Ocelot need > to be modified to allow a parameter for the CPU port? > DSA is what allows you to select which of the port is the port connected to the BBB (this is the CPU port in DSA parlance). This is what you see in the example in Documentation/devicetree/bindings/net/dsa/ocelot.txt -- Alexandre Belloni, co-owner and COO, Bootlin Embedded Linux and Kernel engineering https://bootlin.com