| Message ID | 20220926002928.2744638-13-colin.foster@in-advantage.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | add support for the the vsc7512 internal copper phys | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | fail | build log |
On Sun, Sep 25, 2022 at 05:29:26PM -0700, Colin Foster wrote: > --- > .../bindings/net/dsa/mscc,ocelot.yaml | 59 +++++++++++++++++++ > 1 file changed, 59 insertions(+) > > diff --git a/Documentation/devicetree/bindings/net/dsa/mscc,ocelot.yaml b/Documentation/devicetree/bindings/net/dsa/mscc,ocelot.yaml > index 8d93ed9c172c..49450a04e589 100644 > --- a/Documentation/devicetree/bindings/net/dsa/mscc,ocelot.yaml > +++ b/Documentation/devicetree/bindings/net/dsa/mscc,ocelot.yaml > @@ -54,9 +54,22 @@ description: | > - phy-mode = "1000base-x": on ports 0, 1, 2, 3 > - phy-mode = "2500base-x": on ports 0, 1, 2, 3 > > + VSC7412 (Ocelot-Ext): VSC7512 > + > + The Ocelot family consists of four devices, the VSC7511, VSC7512, VSC7513, > + and the VSC7514. The VSC7513 and VSC7514 both have an internal MIPS > + processor that natively support Linux. Additionally, all four devices > + support control over external interfaces, SPI and PCIe. The Ocelot-Ext > + driver is for the external control portion. > + > + The following PHY interface types are supported: > + > + - phy-mode = "internal": on ports 0, 1, 2, 3 More PHY interface types are supported. Please document them all. It doesn't matter what the driver supports. Drivers and device tree blobs should be able to have different lifetimes. A driver which doesn't support the SERDES ports should work with a device tree that defines them, and a driver that supports the SERDES ports should work with a device tree that doesn't. Similar for the other stuff which isn't documented (interrupts, SERDES PHY handles etc). Since there is already an example with vsc7514, you know how they need to look, even if they don't work yet on your hardware, no? > + > properties: > compatible: > enum: > + - mscc,vsc7512-switch > - mscc,vsc9953-switch > - pci1957,eef0 > > @@ -258,3 +271,49 @@ examples: > }; > }; > }; > + # Ocelot-ext VSC7512 > + - | > + spi { > + soc@0 { > + compatible = "mscc,vsc7512"; > + #address-cells = <1>; > + #size-cells = <1>; > + > + ethernet-switch@0 { > + compatible = "mscc,vsc7512-switch"; > + reg = <0 0>; What is the idea behind reg = <0 0> here? I would expect this driver to follow the same conventions as Documentation/devicetree/bindings/net/mscc,vsc7514-switch.yaml. The hardware is mostly the same, so the switch portion of the DT bindings should be mostly plug and play between the switchdev and the DSA variant. So you can pick the "sys" target as the one giving the address of the node, and define all targets via "reg" and "reg-names" here. Like so: reg = <0x71010000 0x00010000>, <0x71030000 0x00010000>, <0x71080000 0x00000100>, <0x710e0000 0x00010000>, <0x711e0000 0x00000100>, <0x711f0000 0x00000100>, <0x71200000 0x00000100>, <0x71210000 0x00000100>, <0x71220000 0x00000100>, <0x71230000 0x00000100>, <0x71240000 0x00000100>, <0x71250000 0x00000100>, <0x71260000 0x00000100>, <0x71270000 0x00000100>, <0x71280000 0x00000100>, <0x71800000 0x00080000>, <0x71880000 0x00010000>, <0x71040000 0x00010000>, <0x71050000 0x00010000>, <0x71060000 0x00010000>; reg-names = "sys", "rew", "qs", "ptp", "port0", "port1", "port2", "port3", "port4", "port5", "port6", "port7", "port8", "port9", "port10", "qsys", "ana", "s0", "s1", "s2"; The mfd driver can use these resources or can choose to ignore them, but I don't see a reason why the dt-bindings should diverge from vsc7514, its closest cousin. > + > + ethernet-ports { > + #address-cells = <1>; > + #size-cells = <0>; > + > + port@0 { > + reg = <0>; > + label = "cpu"; label = "cpu" is not used, please remove. > + ethernet = <&mac_sw>; > + phy-handle = <&phy0>; > + phy-mode = "internal"; > + }; > + > + port@1 { > + reg = <1>; > + label = "swp1"; > + phy-mode = "internal"; > + phy-handle = <&phy1>; > + }; > + > + port@2 { > + reg = <2>; > + phy-mode = "internal"; > + phy-handle = <&phy2>; > + }; > + > + port@3 { > + reg = <3>; > + phy-mode = "internal"; > + phy-handle = <&phy3>; > + }; > + }; > + }; > + }; > + }; > -- > 2.25.1 >
On Tue, Sep 27, 2022 at 11:26:00PM +0300, Vladimir Oltean wrote: > On Sun, Sep 25, 2022 at 05:29:26PM -0700, Colin Foster wrote: > > --- > > .../bindings/net/dsa/mscc,ocelot.yaml | 59 +++++++++++++++++++ > > 1 file changed, 59 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/net/dsa/mscc,ocelot.yaml b/Documentation/devicetree/bindings/net/dsa/mscc,ocelot.yaml > > index 8d93ed9c172c..49450a04e589 100644 > > --- a/Documentation/devicetree/bindings/net/dsa/mscc,ocelot.yaml > > +++ b/Documentation/devicetree/bindings/net/dsa/mscc,ocelot.yaml > > @@ -54,9 +54,22 @@ description: | > > - phy-mode = "1000base-x": on ports 0, 1, 2, 3 > > - phy-mode = "2500base-x": on ports 0, 1, 2, 3 > > > > + VSC7412 (Ocelot-Ext): > > VSC7512 Oops. Thanks. > > > + > > + The Ocelot family consists of four devices, the VSC7511, VSC7512, VSC7513, > > + and the VSC7514. The VSC7513 and VSC7514 both have an internal MIPS > > + processor that natively support Linux. Additionally, all four devices > > + support control over external interfaces, SPI and PCIe. The Ocelot-Ext > > + driver is for the external control portion. > > + > > + The following PHY interface types are supported: > > + > > + - phy-mode = "internal": on ports 0, 1, 2, 3 > > More PHY interface types are supported. Please document them all. > It doesn't matter what the driver supports. Drivers and device tree > blobs should be able to have different lifetimes. A driver which doesn't > support the SERDES ports should work with a device tree that defines > them, and a driver that supports the SERDES ports should work with a > device tree that doesn't. > > Similar for the other stuff which isn't documented (interrupts, SERDES > PHY handles etc). Since there is already an example with vsc7514, you > know how they need to look, even if they don't work yet on your > hardware, no? Understood. My concern was "oh, all these ports are supported in the documentation, so they must work" when in actuality they won't. But I understand DTB compatibility. This is the same thing Krzysztof was saying as well I belive. I'll update for v4, with apologies. > > > + > > properties: > > compatible: > > enum: > > + - mscc,vsc7512-switch > > - mscc,vsc9953-switch > > - pci1957,eef0 > > > > @@ -258,3 +271,49 @@ examples: > > }; > > }; > > }; > > + # Ocelot-ext VSC7512 > > + - | > > + spi { > > + soc@0 { > > + compatible = "mscc,vsc7512"; > > + #address-cells = <1>; > > + #size-cells = <1>; > > + > > + ethernet-switch@0 { > > + compatible = "mscc,vsc7512-switch"; > > + reg = <0 0>; > > What is the idea behind reg = <0 0> here? I would expect this driver to > follow the same conventions as Documentation/devicetree/bindings/net/mscc,vsc7514-switch.yaml. > The hardware is mostly the same, so the switch portion of the DT bindings > should be mostly plug and play between the switchdev and the DSA variant. > So you can pick the "sys" target as the one giving the address of the > node, and define all targets via "reg" and "reg-names" here. > > Like so: > > reg = <0x71010000 0x00010000>, > <0x71030000 0x00010000>, > <0x71080000 0x00000100>, > <0x710e0000 0x00010000>, > <0x711e0000 0x00000100>, > <0x711f0000 0x00000100>, > <0x71200000 0x00000100>, > <0x71210000 0x00000100>, > <0x71220000 0x00000100>, > <0x71230000 0x00000100>, > <0x71240000 0x00000100>, > <0x71250000 0x00000100>, > <0x71260000 0x00000100>, > <0x71270000 0x00000100>, > <0x71280000 0x00000100>, > <0x71800000 0x00080000>, > <0x71880000 0x00010000>, > <0x71040000 0x00010000>, > <0x71050000 0x00010000>, > <0x71060000 0x00010000>; > reg-names = "sys", "rew", "qs", "ptp", "port0", "port1", > "port2", "port3", "port4", "port5", "port6", > "port7", "port8", "port9", "port10", "qsys", > "ana", "s0", "s1", "s2"; > > The mfd driver can use these resources or can choose to ignore them, but > I don't see a reason why the dt-bindings should diverge from vsc7514, > its closest cousin. This one I can answer. (from November 2021). Also I'm not saying that my interpretation is correct. Historically when there are things up for interpretation, I choose the incorrect path. (case in point... the other part of this email) https://patchwork.kernel.org/project/netdevbpf/patch/20211125201301.3748513-4-colin.foster@in-advantage.com/#24620755 ''' The thing with putting the targets in the device tree is that you're inflicting yourself unnecessary pain. Take a look at Documentation/devicetree/bindings/net/mscc-ocelot.txt, and notice that they mark the "ptp" target as optional because it wasn't needed when they first published the device tree, and now they need to maintain compatibility with those old blobs. To me that is one of the sillier reasons why you would not support PTP, because you don't know where your registers are. And that document is not even up to date, it hasn't been updated when VCAP ES0, IS1, IS2 were added. I don't think that Horatiu even bothered to maintain backwards compatibility when he initially added tc-flower offload for VCAP IS2, and as a result, I did not bother either when extending it for the S0 and S1 targets. At some point afterwards, the Microchip people even stopped complaining and just went along with it. (the story is pretty much told from memory, I'm sorry if I mixed up some facts). It's pretty messy, and that's what you get for creating these micro-maps of registers spread through the guts of the SoC and then a separate reg-name for each. When we worked on the device tree for LS1028A and then T1040, it was very much a conscious decision for the driver to have a single, big register map and split it up pretty much in whichever way it wants to. In fact I think we wouldn't be having the discussion about how to split things right now if we didn't have that flexibility. ''' I'm happy to go any way. The two that make the most sense might be: micro-maps to make the VSC7512 "switch" portion match the VSC7514. The ethernet switch portion might still have to ignore these... A 'mega-map' that would also be ignored by the switch. It would be less arbitrary than the <0 0> that I went with. Maybe something like <0x70000000 0x02000000> to at least point to some valid region. > > > + > > + ethernet-ports { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + port@0 { > > + reg = <0>; > > + label = "cpu"; > > label = "cpu" is not used, please remove. Will do. This sounds familiar, so I'm sorry if it fell through the cracks.
On Tue, Sep 27, 2022 at 11:26:00PM +0300, Vladimir Oltean wrote: > On Sun, Sep 25, 2022 at 05:29:26PM -0700, Colin Foster wrote: > > --- > > + - phy-mode = "internal": on ports 0, 1, 2, 3 > > More PHY interface types are supported. Please document them all. > It doesn't matter what the driver supports. Drivers and device tree > blobs should be able to have different lifetimes. A driver which doesn't > support the SERDES ports should work with a device tree that defines > them, and a driver that supports the SERDES ports should work with a > device tree that doesn't. This will change my patch a little bit then. I didn't undersand this requirement. My current device tree has all 8 ethernet ports populated. ocelot_ext believes "all these port modes are accepted" by way of a fully-populated vsc7512_port_modes[] array. As a result, when I'm testing, swp4 through swp7 all enumerate as devices, though they don't actually function. It isn't until serdes / phylink / pcs / pll5 come along that they become functional ports. I doubt this is desired. Though if I'm using the a new macro OCELOT_PORT_MODE_NONE, felix.c stops after felix_validate_phy_mode. I think the only thing I can do is to allow felix to ignore invalid phy modes on some ports (which might be desired) and continue on with the most it can do. That seems like a potential improvement to the felix driver... The other option is to allow the ports to enumerate, but leave them non-functional. This is how my system currently acts, but as I said, I bet it would be confusing to any user. Thoughts?
On Fri, Sep 30, 2022 at 02:15:58PM -0700, Colin Foster wrote: > On Tue, Sep 27, 2022 at 11:26:00PM +0300, Vladimir Oltean wrote: > > On Sun, Sep 25, 2022 at 05:29:26PM -0700, Colin Foster wrote: > > > --- > > > + - phy-mode = "internal": on ports 0, 1, 2, 3 > > > > More PHY interface types are supported. Please document them all. > > It doesn't matter what the driver supports. Drivers and device tree > > blobs should be able to have different lifetimes. A driver which doesn't > > support the SERDES ports should work with a device tree that defines > > them, and a driver that supports the SERDES ports should work with a > > device tree that doesn't. > > This will change my patch a little bit then. I didn't undersand this > requirement. > > My current device tree has all 8 ethernet ports populated. ocelot_ext > believes "all these port modes are accepted" by way of a fully-populated > vsc7512_port_modes[] array. > > As a result, when I'm testing, swp4 through swp7 all enumerate as > devices, though they don't actually function. It isn't until serdes / > phylink / pcs / pll5 come along that they become functional ports. > > I doubt this is desired. Though if I'm using the a new macro > OCELOT_PORT_MODE_NONE, felix.c stops after felix_validate_phy_mode. > > I think the only thing I can do is to allow felix to ignore invalid phy > modes on some ports (which might be desired) and continue on with the > most it can do. That seems like a potential improvement to the felix > driver... > > The other option is to allow the ports to enumerate, but leave them > non-functional. This is how my system currently acts, but as I said, I > bet it would be confusing to any user. > > Thoughts? > Also, for what its worth, I tried this just now by making this change: err = felix_validate_phy_mode(felix, port, phy_mode); if (err < 0) { dev_err(dev, "Unsupported PHY mode %s on port %d\n", phy_modes(phy_mode), port); of_node_put(child); - return err; + continue; } This functions in that I only see swp1-swp3, but I don't think it should - it is just leaving phy_mode set to 0 (PHY_INTERFACE_MODE_NA). My guess is it'll need more logic to say "don't add these DSA ports because the driver doesn't support those PHY interfaces" [ 3.555367] ocelot-switch ocelot-switch.4.auto: Unsupported PHY mode qsgmii on port 4 [ 3.563551] ocelot-switch ocelot-switch.4.auto: Unsupported PHY mode qsgmii on port 5 [ 3.571570] ocelot-switch ocelot-switch.4.auto: Unsupported PHY mode qsgmii on port 6 [ 3.579459] ocelot-switch ocelot-switch.4.auto: Unsupported PHY mode qsgmii on port 7 [ 4.271832] ocelot-switch ocelot-switch.4.auto: PHY [ocelot-miim0.2.auto-mii:00] driver [Generic PHY] (irq=POLL) [ 4.282715] ocelot-switch ocelot-switch.4.auto: configuring for phy/internal link mode [ 4.296478] ocelot-switch ocelot-switch.4.auto swp1 (uninitialized): PHY [ocelot-miim0.2.auto-mii:01] driver [Generic PHY] (irq=POLL) [ 4.312876] ocelot-switch ocelot-switch.4.auto swp2 (uninitialized): PHY [ocelot-miim0.2.auto-mii:02] driver [Generic PHY] (irq=POLL) [ 4.328897] ocelot-switch ocelot-switch.4.auto swp3 (uninitialized): PHY [ocelot-miim0.2.auto-mii:03] driver [Generic PHY] (irq=POLL) [ 5.032849] ocelot-switch ocelot-switch.4.auto swp4 (uninitiailized): validation of qsgmii with support 00000000,00000000,000062ff and advertisement 00000000,00000000,000062ff failed: -EINVAL [ 5.051265] ocelot-switch ocelot-switch.4.auto swp4 (uninitialized): failed to connect to PHY: -EINVAL [ 5.060670] ocelot-switch ocelot-switch.4.auto swp4 (uninitialized): error -22 setting up PHY for tree 0, switch 0, port 4 (repeated for swp5-7)
On Fri, Sep 30, 2022 at 05:20:22PM -0700, Colin Foster wrote: > On Fri, Sep 30, 2022 at 02:15:58PM -0700, Colin Foster wrote: > > On Tue, Sep 27, 2022 at 11:26:00PM +0300, Vladimir Oltean wrote: > > > On Sun, Sep 25, 2022 at 05:29:26PM -0700, Colin Foster wrote: > > > > --- > > > > + - phy-mode = "internal": on ports 0, 1, 2, 3 > > > > > > More PHY interface types are supported. Please document them all. > > > It doesn't matter what the driver supports. Drivers and device tree > > > blobs should be able to have different lifetimes. A driver which doesn't > > > support the SERDES ports should work with a device tree that defines > > > them, and a driver that supports the SERDES ports should work with a > > > device tree that doesn't. > > > > This will change my patch a little bit then. I didn't undersand this > > requirement. > > > > My current device tree has all 8 ethernet ports populated. ocelot_ext > > believes "all these port modes are accepted" by way of a fully-populated > > vsc7512_port_modes[] array. > > > > As a result, when I'm testing, swp4 through swp7 all enumerate as > > devices, though they don't actually function. It isn't until serdes / > > phylink / pcs / pll5 come along that they become functional ports. > > > > I doubt this is desired. Though if I'm using the a new macro > > OCELOT_PORT_MODE_NONE, felix.c stops after felix_validate_phy_mode. > > > > I think the only thing I can do is to allow felix to ignore invalid phy > > modes on some ports (which might be desired) and continue on with the > > most it can do. That seems like a potential improvement to the felix > > driver... > > > > The other option is to allow the ports to enumerate, but leave them > > non-functional. This is how my system currently acts, but as I said, I > > bet it would be confusing to any user. > > > > Thoughts? Having the interfaces probe but not work isn't the worst, but if we could make just the SERDES ports fail to probe, it would be better. > Also, for what its worth, I tried this just now by making this change: > > err = felix_validate_phy_mode(felix, port, phy_mode); > if (err < 0) { > dev_err(dev, "Unsupported PHY mode %s on port %d\n", > phy_modes(phy_mode), port); > of_node_put(child); > - return err; > + continue; > } > > This functions in that I only see swp1-swp3, but I don't think it > should - it is just leaving phy_mode set to 0 (PHY_INTERFACE_MODE_NA). You could add a comment above the "continue" statement explaining this. > My guess is it'll need more logic to say "don't add these DSA ports because > the driver doesn't support those PHY interfaces" > > [ 3.555367] ocelot-switch ocelot-switch.4.auto: Unsupported PHY mode qsgmii on port 4 > [ 3.563551] ocelot-switch ocelot-switch.4.auto: Unsupported PHY mode qsgmii on port 5 > [ 3.571570] ocelot-switch ocelot-switch.4.auto: Unsupported PHY mode qsgmii on port 6 > [ 3.579459] ocelot-switch ocelot-switch.4.auto: Unsupported PHY mode qsgmii on port 7 > [ 4.271832] ocelot-switch ocelot-switch.4.auto: PHY [ocelot-miim0.2.auto-mii:00] driver [Generic PHY] (irq=POLL) > [ 4.282715] ocelot-switch ocelot-switch.4.auto: configuring for phy/internal link mode > [ 4.296478] ocelot-switch ocelot-switch.4.auto swp1 (uninitialized): PHY [ocelot-miim0.2.auto-mii:01] driver [Generic PHY] (irq=POLL) > [ 4.312876] ocelot-switch ocelot-switch.4.auto swp2 (uninitialized): PHY [ocelot-miim0.2.auto-mii:02] driver [Generic PHY] (irq=POLL) > [ 4.328897] ocelot-switch ocelot-switch.4.auto swp3 (uninitialized): PHY [ocelot-miim0.2.auto-mii:03] driver [Generic PHY] (irq=POLL) > [ 5.032849] ocelot-switch ocelot-switch.4.auto swp4 (uninitiailized): validation of qsgmii with support 00000000,00000000,000062ff and advertisement 00000000,00000000,000062ff failed: -EINVAL > [ 5.051265] ocelot-switch ocelot-switch.4.auto swp4 (uninitialized): failed to connect to PHY: -EINVAL > [ 5.060670] ocelot-switch ocelot-switch.4.auto swp4 (uninitialized): error -22 setting up PHY for tree 0, switch 0, port 4 > (repeated for swp5-7) I think the behavior is correct and sufficient. The ocelot driver always requires a valid phy-mode in the device tree for all ports, and PHY_INTERFACE_MODE_NA means the lack of one. In turn, this is enough to make phylink_validate() fail with any valid device tree. And DSA is smart enough to limp on with the rest of its ports if phylink setup failed for some of them - see dsa_port_setup_as_unused() in the current net-next git tree. If you don't think this is enough, you could also patch felix_phylink_get_caps() to exclude ocelot->ports[port]->phy_mode == PHY_INTERFACE_MODE_NA from applying this assignment (which would make config->supported_interfaces remain empty): __set_bit(ocelot->ports[port]->phy_mode, config->supported_interfaces);
On 26/09/2022 02:29, Colin Foster wrote: > The ocelot-ext driver is another sub-device of the Ocelot / Felix driver > system, which currently supports the four internal copper phys. > > Signed-off-by: Colin Foster <colin.foster@in-advantage.com> > --- > > v3 > * Remove "currently supported" verbage > The Seville and Felix 9959 all list their supported modes following > the sentence "The following PHY interface types are supported". > During V2, I had used "currently supported" to suggest more interface > modes are around the corner, though this had raised questions. > > The suggestion was to drop the entire sentence. I did leave the > modified sentence there because it exactly matches the other two > supported products. > > v2 > * New patch > > --- > .../bindings/net/dsa/mscc,ocelot.yaml | 59 +++++++++++++++++++ > 1 file changed, 59 insertions(+) > > diff --git a/Documentation/devicetree/bindings/net/dsa/mscc,ocelot.yaml b/Documentation/devicetree/bindings/net/dsa/mscc,ocelot.yaml > index 8d93ed9c172c..49450a04e589 100644 > --- a/Documentation/devicetree/bindings/net/dsa/mscc,ocelot.yaml > +++ b/Documentation/devicetree/bindings/net/dsa/mscc,ocelot.yaml > @@ -54,9 +54,22 @@ description: | > - phy-mode = "1000base-x": on ports 0, 1, 2, 3 > - phy-mode = "2500base-x": on ports 0, 1, 2, 3 > > + VSC7412 (Ocelot-Ext): > + > + The Ocelot family consists of four devices, the VSC7511, VSC7512, VSC7513, > + and the VSC7514. The VSC7513 and VSC7514 both have an internal MIPS > + processor that natively support Linux. Additionally, all four devices > + support control over external interfaces, SPI and PCIe. The Ocelot-Ext > + driver is for the external control portion. > + > + The following PHY interface types are supported: > + > + - phy-mode = "internal": on ports 0, 1, 2, 3 > + > properties: > compatible: > enum: > + - mscc,vsc7512-switch > - mscc,vsc9953-switch > - pci1957,eef0 > > @@ -258,3 +271,49 @@ examples: > }; > }; > }; > + # Ocelot-ext VSC7512 > + - | > + spi { > + soc@0 { soc in spi is a bit confusing. Does it even pass the tests? You have unit address but no reg. > + compatible = "mscc,vsc7512"; > + #address-cells = <1>; > + #size-cells = <1>; > + > + ethernet-switch@0 { > + compatible = "mscc,vsc7512-switch"; > + reg = <0 0>; 0 is the address on which soc bus? > + > + ethernet-ports { > + #address-cells = <1>; > + #size-cells = <0>; > + > + port@0 { > + reg = <0>; > + label = "cpu"; > + ethernet = <&mac_sw>; > + phy-handle = <&phy0>; > + phy-mode = "internal"; > + }; > + > + port@1 { > + reg = <1>; > + label = "swp1"; > + phy-mode = "internal"; > + phy-handle = <&phy1>; > + }; > + > + port@2 { > + reg = <2>; > + phy-mode = "internal"; > + phy-handle = <&phy2>; > + }; > + > + port@3 { > + reg = <3>; > + phy-mode = "internal"; > + phy-handle = <&phy3>; > + }; How is this example different than previous one (existing soc example)? If by compatible and number of ports, then there is no much value here. > + }; > + }; > + }; > + }; Best regards, Krzysztof
On Tue, Oct 04, 2022 at 01:19:33PM +0200, Krzysztof Kozlowski wrote: > > + # Ocelot-ext VSC7512 > > + - | > > + spi { > > + soc@0 { > > soc in spi is a bit confusing. Do you have a better suggestion for a node name? This is effectively a container for peripherals which would otherwise live under a /soc node, if they were accessed over MMIO by the internal microprocessor of the SoC, rather than by an external processor over SPI. > How is this example different than previous one (existing soc example)? > If by compatible and number of ports, then there is no much value here. The positioning relative to the other nodes is what's different.
On 04/10/2022 14:15, Vladimir Oltean wrote: > On Tue, Oct 04, 2022 at 01:19:33PM +0200, Krzysztof Kozlowski wrote: >>> + # Ocelot-ext VSC7512 >>> + - | >>> + spi { >>> + soc@0 { >> >> soc in spi is a bit confusing. > > Do you have a better suggestion for a node name? This is effectively a > container for peripherals which would otherwise live under a /soc node, /soc node implies it does not live under /spi node. Otherwise it would be /spi/soc, right? > if they were accessed over MMIO by the internal microprocessor of the > SoC, rather than by an external processor over SPI. > >> How is this example different than previous one (existing soc example)? >> If by compatible and number of ports, then there is no much value here. > > The positioning relative to the other nodes is what's different. Positioning of nodes is not worth another example, if everything else is the same. What is here exactly tested or shown by example? Using a device in SPI controller? Best regards, Krzysztof
On Tue, Oct 04, 2022 at 04:59:02PM +0200, Krzysztof Kozlowski wrote: > On 04/10/2022 14:15, Vladimir Oltean wrote: > > On Tue, Oct 04, 2022 at 01:19:33PM +0200, Krzysztof Kozlowski wrote: > >>> + # Ocelot-ext VSC7512 > >>> + - | > >>> + spi { > >>> + soc@0 { > >> > >> soc in spi is a bit confusing. > > > > Do you have a better suggestion for a node name? This is effectively a > > container for peripherals which would otherwise live under a /soc node, > > /soc node implies it does not live under /spi node. Otherwise it would > be /spi/soc, right? Did you read what's written right below? I can explain if you want, but there's no point if you're not going to read or ask other clarification questions. > > if they were accessed over MMIO by the internal microprocessor of the > > SoC, rather than by an external processor over SPI. ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ The /spi/soc@0 node actually has a compatible of "mscc,vsc7512" which Colin did not show in the example (it is not "simple-bus"). It is covered by Documentation/devicetree/bindings/mfd/mscc,ocelot.yaml. Still waiting for a better suggestion for how to name the mfd container node. > >> How is this example different than previous one (existing soc example)? > >> If by compatible and number of ports, then there is no much value here. > > > > The positioning relative to the other nodes is what's different. > > Positioning of nodes is not worth another example, if everything else is > the same. What is here exactly tested or shown by example? Using a > device in SPI controller? Everything is not the same, it is not the same hardware as what is currenly covered by Documentation/devicetree/bindings/net/dsa/mscc,ocelot.yaml. The "existing soc example" (mscc,vsc9953-switch) has a different port count, integration with a different SERDES, interrupt controller, pin controller, things like that. The examples already differ in port count and phy-mode values, I expect they will start diverging more in the future. If you still believe it's not worth having an example of how to instantiate a SPI-controlled VSC7512 because there also exists an example of an MMIO-controlled VSC9953, then what can I say. ------ cut here ------ Unrelated to your "existing soc example" (the VSC9953), but relevant and you may want to share your opinion on this: The same hardware present in the VSC7514 SoC can also be driven by an integrated MIPS processor, and in that case, it is indeed expected that the same dt-bindings cover both the /soc and the /spi/soc@0/ relative positioning of their OF node. This is true for simpler peripherals like "mscc,ocelot-miim", "mscc,ocelot-pinctrl", "mscc,ocelot-sgpio". However it is not true for the main switching IP of the SoC itself. When driven by a switchdev driver, by the internal MIPS processor (the DMA engine is what is used for packet I/O), the switching IP follows the Documentation/devicetree/bindings/net/mscc,vsc7514-switch.yaml binding document. When driven by a DSA driver (external processor, host frames are redirected through an Ethernet port instead of DMA controller), the switching IP follows the Documentation/devicetree/bindings/net/dsa/mscc,ocelot.yaml document. The switching IP is special in this regard because the hardware is not used in the same way. The DSA dt-binding also needs the 'ethernet' phandle to be present in a port node. The different placement of the bindings according to the use case of the hardware is a bit awkward, but is a direct consequence of the separation between DSA and pure switchdev drivers that has existed thus far (and the fact that DSA has its own folder in the dt-bindings, with common properties in dsa.yaml and dsa-port.yaml etc). It is relatively uncommon for a switching IP to have provisioning to be used in both modes, and for Linux to support both modes (using different drivers), yet this is what we have here.
Hi Krzysztof, On Tue, Oct 04, 2022 at 01:19:33PM +0200, Krzysztof Kozlowski wrote: > On 26/09/2022 02:29, Colin Foster wrote: > > The ocelot-ext driver is another sub-device of the Ocelot / Felix driver > > system, which currently supports the four internal copper phys. > > > > Signed-off-by: Colin Foster <colin.foster@in-advantage.com> ... > > + # Ocelot-ext VSC7512 > > + - | > > + spi { > > + soc@0 { > > soc in spi is a bit confusing. > > Does it even pass the tests? You have unit address but no reg. I omitted those from the documentation. Rob's bot is usually quick to alert me when I forgot to run dt_binding_check and something fails though. I'll double check, but I thought everything passed. > > > + compatible = "mscc,vsc7512"; > > > > + #address-cells = <1>; > > + #size-cells = <1>; > > + > > + ethernet-switch@0 { > > + compatible = "mscc,vsc7512-switch"; > > + reg = <0 0>; > > 0 is the address on which soc bus? This one Vladimir brought up as well. The MIPS cousin of this chip is the VSC7514. They have exactly (or almost exactly) the same hardware, except the 7514 has an internal MIPS while the 7512 has an 8051. Both chips can be controlled externally via SPI or PCIe. This is adding control for the chip via SPI. For the 7514, you can see there's an array of 20 register ranges that all get mmap'd to 20 different regmaps. (Documentation/devicetree/bindings/net/mscc,vsc7514-switch.yaml) switch@1010000 { compatible = "mscc,vsc7514-switch"; reg = <0x1010000 0x10000>, <0x1030000 0x10000>, <0x1080000 0x100>, <0x10e0000 0x10000>, <0x11e0000 0x100>, <0x11f0000 0x100>, <0x1200000 0x100>, <0x1210000 0x100>, <0x1220000 0x100>, <0x1230000 0x100>, <0x1240000 0x100>, <0x1250000 0x100>, <0x1260000 0x100>, <0x1270000 0x100>, <0x1280000 0x100>, <0x1800000 0x80000>, <0x1880000 0x10000>, <0x1040000 0x10000>, <0x1050000 0x10000>, <0x1060000 0x10000>, <0x1a0 0x1c4>; reg-names = "sys", "rew", "qs", "ptp", "port0", "port1", "port2", "port3", "port4", "port5", "port6", "port7", "port8", "port9", "port10", "qsys", "ana", "s0", "s1", "s2", "fdma"; The suggestion was to keep the device trees of the 7512 and 7514 as similar as possible, so this will essentially become: switch@71010000 { compatible = "mscc,vsc7512-switch"; reg = <0x71010000 0x10000>, <0x71030000 0x10000>, ...
On 05/10/2022 02:08, Colin Foster wrote: > Hi Krzysztof, > > On Tue, Oct 04, 2022 at 01:19:33PM +0200, Krzysztof Kozlowski wrote: >> On 26/09/2022 02:29, Colin Foster wrote: >>> The ocelot-ext driver is another sub-device of the Ocelot / Felix driver >>> system, which currently supports the four internal copper phys. >>> >>> Signed-off-by: Colin Foster <colin.foster@in-advantage.com> > ... >>> + # Ocelot-ext VSC7512 >>> + - | >>> + spi { >>> + soc@0 { >> >> soc in spi is a bit confusing. >> >> Does it even pass the tests? You have unit address but no reg. > > I omitted those from the documentation. Rob's bot is usually quick to > alert me when I forgot to run dt_binding_check and something fails > though. I'll double check, but I thought everything passed. > >> >>> + compatible = "mscc,vsc7512"; >> >> >>> + #address-cells = <1>; >>> + #size-cells = <1>; >>> + >>> + ethernet-switch@0 { >>> + compatible = "mscc,vsc7512-switch"; >>> + reg = <0 0>; >> >> 0 is the address on which soc bus? > > This one Vladimir brought up as well. The MIPS cousin of this chip > is the VSC7514. They have exactly (or almost exactly) the same hardware, > except the 7514 has an internal MIPS while the 7512 has an 8051. > > Both chips can be controlled externally via SPI or PCIe. This is adding > control for the chip via SPI. > > For the 7514, you can see there's an array of 20 register ranges that > all get mmap'd to 20 different regmaps. > > (Documentation/devicetree/bindings/net/mscc,vsc7514-switch.yaml) > > switch@1010000 { > compatible = "mscc,vsc7514-switch"; > reg = <0x1010000 0x10000>, > <0x1030000 0x10000>, > <0x1080000 0x100>, > <0x10e0000 0x10000>, > <0x11e0000 0x100>, > <0x11f0000 0x100>, > <0x1200000 0x100>, > <0x1210000 0x100>, > <0x1220000 0x100>, > <0x1230000 0x100>, > <0x1240000 0x100>, > <0x1250000 0x100>, > <0x1260000 0x100>, > <0x1270000 0x100>, > <0x1280000 0x100>, > <0x1800000 0x80000>, > <0x1880000 0x10000>, > <0x1040000 0x10000>, > <0x1050000 0x10000>, > <0x1060000 0x10000>, > <0x1a0 0x1c4>; > reg-names = "sys", "rew", "qs", "ptp", "port0", "port1", > "port2", "port3", "port4", "port5", "port6", > "port7", "port8", "port9", "port10", "qsys", > "ana", "s0", "s1", "s2", "fdma"; > > > The suggestion was to keep the device trees of the 7512 and 7514 as > similar as possible, so this will essentially become: > switch@71010000 { > compatible = "mscc,vsc7512-switch"; > reg = <0x71010000 0x10000>, > <0x71030000 0x10000>, > ... I don't understand how your answer relates to "reg=<0 0>;". How is it going to become 0x71010000 if there is no other reg/ranges set in parent nodes. The node has only one IO address, but you say the switch has 20 addresses... Are we talking about same hardware? Best regards, Krzysztof
On 04/10/2022 18:01, Vladimir Oltean wrote: > On Tue, Oct 04, 2022 at 04:59:02PM +0200, Krzysztof Kozlowski wrote: >> On 04/10/2022 14:15, Vladimir Oltean wrote: >>> On Tue, Oct 04, 2022 at 01:19:33PM +0200, Krzysztof Kozlowski wrote: >>>>> + # Ocelot-ext VSC7512 >>>>> + - | >>>>> + spi { >>>>> + soc@0 { >>>> >>>> soc in spi is a bit confusing. >>> >>> Do you have a better suggestion for a node name? This is effectively a >>> container for peripherals which would otherwise live under a /soc node, >> >> /soc node implies it does not live under /spi node. Otherwise it would >> be /spi/soc, right? > > Did you read what's written right below? I can explain if you want, but > there's no point if you're not going to read or ask other clarification > questions. > >>> if they were accessed over MMIO by the internal microprocessor of the >>> SoC, rather than by an external processor over SPI. > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > The /spi/soc@0 node actually has a compatible of "mscc,vsc7512" which > Colin did not show in the example (it is not "simple-bus"). It is covered > by Documentation/devicetree/bindings/mfd/mscc,ocelot.yaml. Still waiting > for a better suggestion for how to name the mfd container node. Then still the /spi node does not seem related. If I understand correctly, your device described in this bindings is a child of soc@0. Sounds fine. How that soc@0 is connected to the parent - via SPI or whatever - is not related to this binding, is it? It is related to the soc binding, but not here. > >>>> How is this example different than previous one (existing soc example)? >>>> If by compatible and number of ports, then there is no much value here. >>> >>> The positioning relative to the other nodes is what's different. >> >> Positioning of nodes is not worth another example, if everything else is >> the same. What is here exactly tested or shown by example? Using a >> device in SPI controller? > > Everything is not the same, it is not the same hardware as what is currenly > covered by Documentation/devicetree/bindings/net/dsa/mscc,ocelot.yaml. > The "existing soc example" (mscc,vsc9953-switch) has a different port > count, integration with a different SERDES, interrupt controller, pin > controller, things like that. The examples already differ in port count > and phy-mode values, I expect they will start diverging more in the > future. If you still believe it's not worth having an example of how to > instantiate a SPI-controlled VSC7512 because there also exists an > example of an MMIO-controlled VSC9953, then what can I say. > > ------ cut here ------ > > Unrelated to your "existing soc example" (the VSC9953), but relevant and > you may want to share your opinion on this: > > The same hardware present in the VSC7514 SoC can also be driven by an > integrated MIPS processor, and in that case, it is indeed expected that > the same dt-bindings cover both the /soc and the /spi/soc@0/ relative > positioning of their OF node. This is true for simpler peripherals like > "mscc,ocelot-miim", "mscc,ocelot-pinctrl", "mscc,ocelot-sgpio". However > it is not true for the main switching IP of the SoC itself. > > When driven by a switchdev driver, by the internal MIPS processor (the > DMA engine is what is used for packet I/O), the switching IP follows the > Documentation/devicetree/bindings/net/mscc,vsc7514-switch.yaml binding > document. > > When driven by a DSA driver (external processor, host frames are > redirected through an Ethernet port instead of DMA controller), > the switching IP follows the Documentation/devicetree/bindings/net/dsa/mscc,ocelot.yaml > document. > > The switching IP is special in this regard because the hardware is not > used in the same way. The DSA dt-binding also needs the 'ethernet' > phandle to be present in a port node. The different placement of the > bindings according to the use case of the hardware is a bit awkward, but > is a direct consequence of the separation between DSA and pure switchdev > drivers that has existed thus far (and the fact that DSA has its own > folder in the dt-bindings, with common properties in dsa.yaml and > dsa-port.yaml etc). It is relatively uncommon for a switching IP to have > provisioning to be used in both modes, and for Linux to support both > modes (using different drivers), yet this is what we have here. Is there a question here to me? What shall I do with this paragraph? You know, I do not have a problem of lack of material to read... Best regards, Krzysztof
On Wed, Oct 05, 2022 at 10:03:04AM +0200, Krzysztof Kozlowski wrote: > On 05/10/2022 02:08, Colin Foster wrote: > > Hi Krzysztof, > > > > On Tue, Oct 04, 2022 at 01:19:33PM +0200, Krzysztof Kozlowski wrote: > >> On 26/09/2022 02:29, Colin Foster wrote: > >>> The ocelot-ext driver is another sub-device of the Ocelot / Felix driver > >>> system, which currently supports the four internal copper phys. > >>> > >>> Signed-off-by: Colin Foster <colin.foster@in-advantage.com> > > ... > >>> + # Ocelot-ext VSC7512 > >>> + - | > >>> + spi { > >>> + soc@0 { > >> > >> soc in spi is a bit confusing. > >> > >> Does it even pass the tests? You have unit address but no reg. > > > > I omitted those from the documentation. Rob's bot is usually quick to > > alert me when I forgot to run dt_binding_check and something fails > > though. I'll double check, but I thought everything passed. > > > >> > >>> + compatible = "mscc,vsc7512"; > >> > >> > >>> + #address-cells = <1>; > >>> + #size-cells = <1>; > >>> + > >>> + ethernet-switch@0 { > >>> + compatible = "mscc,vsc7512-switch"; > >>> + reg = <0 0>; > >> > >> 0 is the address on which soc bus? > > > > This one Vladimir brought up as well. The MIPS cousin of this chip > > is the VSC7514. They have exactly (or almost exactly) the same hardware, > > except the 7514 has an internal MIPS while the 7512 has an 8051. > > > > Both chips can be controlled externally via SPI or PCIe. This is adding > > control for the chip via SPI. > > > > For the 7514, you can see there's an array of 20 register ranges that > > all get mmap'd to 20 different regmaps. > > > > (Documentation/devicetree/bindings/net/mscc,vsc7514-switch.yaml) > > > > switch@1010000 { > > compatible = "mscc,vsc7514-switch"; > > reg = <0x1010000 0x10000>, > > <0x1030000 0x10000>, > > <0x1080000 0x100>, > > <0x10e0000 0x10000>, > > <0x11e0000 0x100>, > > <0x11f0000 0x100>, > > <0x1200000 0x100>, > > <0x1210000 0x100>, > > <0x1220000 0x100>, > > <0x1230000 0x100>, > > <0x1240000 0x100>, > > <0x1250000 0x100>, > > <0x1260000 0x100>, > > <0x1270000 0x100>, > > <0x1280000 0x100>, > > <0x1800000 0x80000>, > > <0x1880000 0x10000>, > > <0x1040000 0x10000>, > > <0x1050000 0x10000>, > > <0x1060000 0x10000>, > > <0x1a0 0x1c4>; > > reg-names = "sys", "rew", "qs", "ptp", "port0", "port1", > > "port2", "port3", "port4", "port5", "port6", > > "port7", "port8", "port9", "port10", "qsys", > > "ana", "s0", "s1", "s2", "fdma"; > > > > > > The suggestion was to keep the device trees of the 7512 and 7514 as > > similar as possible, so this will essentially become: > > switch@71010000 { > > compatible = "mscc,vsc7512-switch"; > > reg = <0x71010000 0x10000>, > > <0x71030000 0x10000>, > > ... > > I don't understand how your answer relates to "reg=<0 0>;". How is it > going to become 0x71010000 if there is no other reg/ranges set in parent > nodes. The node has only one IO address, but you say the switch has 20 > addresses... > > Are we talking about same hardware? Yes. The switch driver for both the VSC7512 and VSC7514 use up to ~20 regmaps depending on what capabilities it is to have. In the 7514 they are all memory-mapped from the device tree. While the 7512 does need these regmaps, they are managed by the MFD, not the device tree. So there isn't a _need_ for them to be here, since at the end of the day they're ignored. The "reg=<0 0>;" was my attempt to indicate that they are ignored, but I understand that isn't desired. So moving forward I'll add all the regmaps back into the device tree. > > Best regards, > Krzysztof >
On 05/10/2022 17:44, Colin Foster wrote: > On Wed, Oct 05, 2022 at 10:03:04AM +0200, Krzysztof Kozlowski wrote: >> On 05/10/2022 02:08, Colin Foster wrote: >>> Hi Krzysztof, >>> >>> On Tue, Oct 04, 2022 at 01:19:33PM +0200, Krzysztof Kozlowski wrote: >>>> On 26/09/2022 02:29, Colin Foster wrote: >>>>> The ocelot-ext driver is another sub-device of the Ocelot / Felix driver >>>>> system, which currently supports the four internal copper phys. >>>>> >>>>> Signed-off-by: Colin Foster <colin.foster@in-advantage.com> >>> ... >>>>> + # Ocelot-ext VSC7512 >>>>> + - | >>>>> + spi { >>>>> + soc@0 { >>>> >>>> soc in spi is a bit confusing. >>>> >>>> Does it even pass the tests? You have unit address but no reg. >>> >>> I omitted those from the documentation. Rob's bot is usually quick to >>> alert me when I forgot to run dt_binding_check and something fails >>> though. I'll double check, but I thought everything passed. >>> >>>> >>>>> + compatible = "mscc,vsc7512"; >>>> >>>> >>>>> + #address-cells = <1>; >>>>> + #size-cells = <1>; >>>>> + >>>>> + ethernet-switch@0 { >>>>> + compatible = "mscc,vsc7512-switch"; >>>>> + reg = <0 0>; >>>> >>>> 0 is the address on which soc bus? >>> >>> This one Vladimir brought up as well. The MIPS cousin of this chip >>> is the VSC7514. They have exactly (or almost exactly) the same hardware, >>> except the 7514 has an internal MIPS while the 7512 has an 8051. >>> >>> Both chips can be controlled externally via SPI or PCIe. This is adding >>> control for the chip via SPI. >>> >>> For the 7514, you can see there's an array of 20 register ranges that >>> all get mmap'd to 20 different regmaps. >>> >>> (Documentation/devicetree/bindings/net/mscc,vsc7514-switch.yaml) >>> >>> switch@1010000 { >>> compatible = "mscc,vsc7514-switch"; >>> reg = <0x1010000 0x10000>, >>> <0x1030000 0x10000>, >>> <0x1080000 0x100>, >>> <0x10e0000 0x10000>, >>> <0x11e0000 0x100>, >>> <0x11f0000 0x100>, >>> <0x1200000 0x100>, >>> <0x1210000 0x100>, >>> <0x1220000 0x100>, >>> <0x1230000 0x100>, >>> <0x1240000 0x100>, >>> <0x1250000 0x100>, >>> <0x1260000 0x100>, >>> <0x1270000 0x100>, >>> <0x1280000 0x100>, >>> <0x1800000 0x80000>, >>> <0x1880000 0x10000>, >>> <0x1040000 0x10000>, >>> <0x1050000 0x10000>, >>> <0x1060000 0x10000>, >>> <0x1a0 0x1c4>; >>> reg-names = "sys", "rew", "qs", "ptp", "port0", "port1", >>> "port2", "port3", "port4", "port5", "port6", >>> "port7", "port8", "port9", "port10", "qsys", >>> "ana", "s0", "s1", "s2", "fdma"; >>> >>> >>> The suggestion was to keep the device trees of the 7512 and 7514 as >>> similar as possible, so this will essentially become: >>> switch@71010000 { >>> compatible = "mscc,vsc7512-switch"; >>> reg = <0x71010000 0x10000>, >>> <0x71030000 0x10000>, >>> ... >> >> I don't understand how your answer relates to "reg=<0 0>;". How is it >> going to become 0x71010000 if there is no other reg/ranges set in parent >> nodes. The node has only one IO address, but you say the switch has 20 >> addresses... >> >> Are we talking about same hardware? > > Yes. The switch driver for both the VSC7512 and VSC7514 use up to ~20 regmaps > depending on what capabilities it is to have. In the 7514 they are all > memory-mapped from the device tree. While the 7512 does need these > regmaps, they are managed by the MFD, not the device tree. So there > isn't a _need_ for them to be here, since at the end of the day they're > ignored. > > The "reg=<0 0>;" was my attempt to indicate that they are ignored, but I > understand that isn't desired. So moving forward I'll add all the > regmaps back into the device tree. You need to describe the hardware. If hardware has IO address space, how does it matter that some driver needs or needs not something? You mentioned that address space is mapped to regmaps. Regmap is Linux specific implementation detail, so this does not answer at all about hardware. On the other hand, if your DTS design requires this is a child of something else and by itself it does not have address space, it would be understandable to skip unit address entirely... but so far it is still confusing, especially that you use arguments related to implementation to justify the DTS. Best regards, Krzysztof
On Tue, Sep 27, 2022 at 11:26:00PM +0300, Vladimir Oltean wrote: > On Sun, Sep 25, 2022 at 05:29:26PM -0700, Colin Foster wrote: > > + > > + The Ocelot family consists of four devices, the VSC7511, VSC7512, VSC7513, > > + and the VSC7514. The VSC7513 and VSC7514 both have an internal MIPS > > + processor that natively support Linux. Additionally, all four devices > > + support control over external interfaces, SPI and PCIe. The Ocelot-Ext > > + driver is for the external control portion. > > + > > + The following PHY interface types are supported: > > + > > + - phy-mode = "internal": on ports 0, 1, 2, 3 > > More PHY interface types are supported. Please document them all. > It doesn't matter what the driver supports. Drivers and device tree > blobs should be able to have different lifetimes. A driver which doesn't > support the SERDES ports should work with a device tree that defines > them, and a driver that supports the SERDES ports should work with a > device tree that doesn't. > > Similar for the other stuff which isn't documented (interrupts, SERDES > PHY handles etc). Since there is already an example with vsc7514, you > know how they need to look, even if they don't work yet on your > hardware, no? > With regards to the interrupts - I don't really have a concept of how those will work, since there isn't a processor for those lines to interrupt. So while there is this for the 7514: interrupts = <18 21 16>; interrupt-names = "ptp_rdy", "xtr", "fdma"; it seems like there isn't anything to add there. That is, unless there's something deeper that is going on that I don't fully understand yet. It wouldn't be the first time and, realistically, won't be the last. I'll copy the 7514 for now, as I plan to send out an RFC shortly with all these updates.
On Fri, Oct 07, 2022 at 01:44:10PM -0700, Colin Foster wrote: > With regards to the interrupts - I don't really have a concept of how > those will work, since there isn't a processor for those lines to > interrupt. So while there is this for the 7514: > > interrupts = <18 21 16>; > interrupt-names = "ptp_rdy", "xtr", "fdma"; > > it seems like there isn't anything to add there. > > That is, unless there's something deeper that is going on that I don't > fully understand yet. It wouldn't be the first time and, realistically, > won't be the last. I'll copy the 7514 for now, as I plan to send out an > RFC shortly with all these updates. I was under the impression that the interrupt controller could be configured to route the interrupts to external destinations EXT_DST0 or EXT_DST1, which have the indices 2 and 3, respectively, in the DST_INTR_* set of registers of the ICPU_CFG:INTR block. I could be wrong, though, maybe this is just for PCIe, I never looked at the pinout of this chip to study whether it's possible to use these as I expect, but normally for things like PTP TX timestamping, you'd expect that the switch notifies the external host when a packet has been timestamped and that timestamp is available in the FIFO. The interrupts out of this switch could also be useful for the PHY state machine, to disable polling. Although in the general sense I agree with you, it's better not to add anything than to add something and be wrong about it. This is where the limitations start showing for the idea that "device tree describes hardware, which is independent of software implementation". It's all too easy to say this when you have an implementation already written. Anyway. DT doesn't describe hardware, but what software wants to understand of it, and that makes it inseparable to some degree from software implementation.
On Tue, Sep 27, 2022 at 03:20:47PM -0700, Colin Foster wrote: > > The mfd driver can use these resources or can choose to ignore them, but > > I don't see a reason why the dt-bindings should diverge from vsc7514, > > its closest cousin. > > This one I can answer. (from November 2021). Also I'm not saying that my > interpretation is correct. Historically when there are things up for > interpretation, I choose the incorrect path. (case in point... the other > part of this email) > > https://patchwork.kernel.org/project/netdevbpf/patch/20211125201301.3748513-4-colin.foster@in-advantage.com/#24620755 > > ''' > The thing with putting the targets in the device tree is that you're > inflicting yourself unnecessary pain. Take a look at > Documentation/devicetree/bindings/net/mscc-ocelot.txt, and notice that > they mark the "ptp" target as optional because it wasn't needed when > they first published the device tree, and now they need to maintain > compatibility with those old blobs. To me that is one of the sillier > reasons why you would not support PTP, because you don't know where your > registers are. And that document is not even up to date, it hasn't been > updated when VCAP ES0, IS1, IS2 were added. I don't think that Horatiu > even bothered to maintain backwards compatibility when he initially > added tc-flower offload for VCAP IS2, and as a result, I did not bother > either when extending it for the S0 and S1 targets. At some point > afterwards, the Microchip people even stopped complaining and just went > along with it. (the story is pretty much told from memory, I'm sorry if > I mixed up some facts). It's pretty messy, and that's what you get for > creating these micro-maps of registers spread through the guts of the > SoC and then a separate reg-name for each. When we worked on the device > tree for LS1028A and then T1040, it was very much a conscious decision > for the driver to have a single, big register map and split it up pretty > much in whichever way it wants to. In fact I think we wouldn't be > having the discussion about how to split things right now if we didn't > have that flexibility. > ''' > > I'm happy to go any way. The two that make the most sense might be: > > micro-maps to make the VSC7512 "switch" portion match the VSC7514. The > ethernet switch portion might still have to ignore these... > > A 'mega-map' that would also be ignored by the switch. It would be less > arbitrary than the <0 0> that I went with. Maybe something like > <0x70000000 0x02000000> to at least point to some valid region. A mega-map for the switch makes a lot more sense to me, if feasible (it should not overlap with the regions of any other peripherals). Something isn't quite right to me in having 20 reg-names for a single device tree node, and I still stand for just describing the whole range and letting the driver split it up according to its needs. I don't know why this approach wasn't chosen for the ocelot switch and I did not have the patience to map out the addresses that the peripherals use in the Microchip SoCs relative to each other, so see if what I'm proposing is possible. But on the other hand this also needs to be balanced with the fact that one day, someone might come along with a mscc,vsc7514-switch that's SPI controlled, and expect that the dt-bindings for it in DSA mode expect the same reg-names that they do in switchdev mode. Or maybe they wouldn't expect that, I don't know. In any case, for NXP switches I didn't have a compatibility issue with switchdev-mode Ocelot to concern myself with, so I went with what made the most sense.
On Wed, Oct 05, 2022 at 10:09:06AM +0200, Krzysztof Kozlowski wrote: > > The /spi/soc@0 node actually has a compatible of "mscc,vsc7512" which > > Colin did not show in the example (it is not "simple-bus"). It is covered > > by Documentation/devicetree/bindings/mfd/mscc,ocelot.yaml. Still waiting > > for a better suggestion for how to name the mfd container node. > > Then still the /spi node does not seem related. If I understand > correctly, your device described in this bindings is a child of soc@0. > Sounds fine. How that soc@0 is connected to the parent - via SPI or > whatever - is not related to this binding, is it? It is related to the > soc binding, but not here. It's an example, it's meant to be informative. It is the first DSA driver of its kind. When everybody else ATM puts the ethernet-switch node under the &spi controller node, this puts it under &spi/soc@<chip-select>/, for reasons that have to do with scalability. If the examples aren't a good place to make this more obvious, I don't know why we don't just tell people to RTFD. > > Unrelated to your "existing soc example" (the VSC9953), but relevant and > > you may want to share your opinion on this: > > > > The same hardware present in the VSC7514 SoC can also be driven by an > > integrated MIPS processor, and in that case, it is indeed expected that > > the same dt-bindings cover both the /soc and the /spi/soc@0/ relative > > positioning of their OF node. This is true for simpler peripherals like > > "mscc,ocelot-miim", "mscc,ocelot-pinctrl", "mscc,ocelot-sgpio". However > > it is not true for the main switching IP of the SoC itself. > > > > When driven by a switchdev driver, by the internal MIPS processor (the > > DMA engine is what is used for packet I/O), the switching IP follows the > > Documentation/devicetree/bindings/net/mscc,vsc7514-switch.yaml binding > > document. > > > > When driven by a DSA driver (external processor, host frames are > > redirected through an Ethernet port instead of DMA controller), > > the switching IP follows the Documentation/devicetree/bindings/net/dsa/mscc,ocelot.yaml > > document. > > > > The switching IP is special in this regard because the hardware is not > > used in the same way. The DSA dt-binding also needs the 'ethernet' > > phandle to be present in a port node. The different placement of the > > bindings according to the use case of the hardware is a bit awkward, but > > is a direct consequence of the separation between DSA and pure switchdev > > drivers that has existed thus far (and the fact that DSA has its own > > folder in the dt-bindings, with common properties in dsa.yaml and > > dsa-port.yaml etc). It is relatively uncommon for a switching IP to have > > provisioning to be used in both modes, and for Linux to support both > > modes (using different drivers), yet this is what we have here. > > Is there a question here to me? What shall I do with this paragraph? You > know, I do not have a problem of lack of material to read... For mscc,vsc7514-switch we have a switchdev driver. For mscc,vsc7512-switch, Colin is working on a DSA driver. Their dt-bindings currently live in different folders. The mscc,vsc7514-switch can also be used together with a DSA driver, and support for that will inevitably be added. When it will, how and where do you recommend the dt-bindings should be added? In net/dsa/mscc,ocelot.yaml, together with the other switches used in DSA mode, or in net/mscc,vsc7514-switch.yaml, because its compatible string already exists there? We can't have a compatible string present in multiple schemas, right? This matters because it has implications upon what Colin should do with the mscc,vsc7512-switch. If your answer to my question is "add $ref: dsa.yaml# to net/mscc,vsc7514-switch.yaml", then I don't see why we wouldn't do that now, and wait until the vsc7514 to make that move anyway.
On Wed, Oct 05, 2022 at 06:09:59PM +0200, Krzysztof Kozlowski wrote: > >> I don't understand how your answer relates to "reg=<0 0>;". How is it > >> going to become 0x71010000 if there is no other reg/ranges set in parent > >> nodes. The node has only one IO address, but you say the switch has 20 > >> addresses... > >> > >> Are we talking about same hardware? > > > > Yes. The switch driver for both the VSC7512 and VSC7514 use up to ~20 regmaps > > depending on what capabilities it is to have. In the 7514 they are all > > memory-mapped from the device tree. While the 7512 does need these > > regmaps, they are managed by the MFD, not the device tree. So there > > isn't a _need_ for them to be here, since at the end of the day they're > > ignored. > > > > The "reg=<0 0>;" was my attempt to indicate that they are ignored, but I > > understand that isn't desired. So moving forward I'll add all the > > regmaps back into the device tree. > > You need to describe the hardware. If hardware has IO address space, how > does it matter that some driver needs or needs not something? What do you mean by IO address space exactly? It is a SPI device with registers. Does that constitute an IO address space to you? The driver need matters because you don't usually see DT nodes of SPI, I2C, MDIO devices describing the address space of their registers, and having child nodes with unit addresses in that address space. Only when those devices are so complex that the need arises to identify smaller building blocks is when you will end up needing that. And this is an implementation detail which shapes how the dt-bindings will look like. > You mentioned that address space is mapped to regmaps. Regmap is Linux > specific implementation detail, so this does not answer at all about > hardware. > > On the other hand, if your DTS design requires this is a child of > something else and by itself it does not have address space, it would be > understandable to skip unit address entirely... but so far it is still > confusing, especially that you use arguments related to implementation > to justify the DTS. If Colin skips the unit address entirely, then how could he distinguish between the otherwise identical MDIO controllers mdio@7107009c and mdio@710700c0 from Documentation/devicetree/bindings/mfd/mscc,ocelot.yaml? The ethernet-switch node added here is on the same hierarchical level with the MDIO controller nodes, so it must have a unit address just like them. But I don't support Colin's choice of "reg=<0 0>;" either. A choice must be made between 2 options: - mapping all 20 regions of the SPI address space into "reg" values - mapping a single region from the smallest until the largest address of those 20, and hope nothing overlaps with some other peripheral, or worse, that this region will never need to be expanded to the left. What information do you need to provide some best practices that can be applied here and are more useful than "you need to describe the hardware"? Verilog/VHDL is what the hardware description that's independent of software implementation is, good luck parsing that.
On Sat, Oct 08, 2022 at 01:48:08AM +0300, Vladimir Oltean wrote: > On Tue, Sep 27, 2022 at 03:20:47PM -0700, Colin Foster wrote: > > > The mfd driver can use these resources or can choose to ignore them, but > > > I don't see a reason why the dt-bindings should diverge from vsc7514, > > > its closest cousin. > > > > This one I can answer. (from November 2021). Also I'm not saying that my > > interpretation is correct. Historically when there are things up for > > interpretation, I choose the incorrect path. (case in point... the other > > part of this email) > > > > https://patchwork.kernel.org/project/netdevbpf/patch/20211125201301.3748513-4-colin.foster@in-advantage.com/#24620755 > > > > ''' > > The thing with putting the targets in the device tree is that you're > > inflicting yourself unnecessary pain. Take a look at > > Documentation/devicetree/bindings/net/mscc-ocelot.txt, and notice that > > they mark the "ptp" target as optional because it wasn't needed when > > they first published the device tree, and now they need to maintain > > compatibility with those old blobs. To me that is one of the sillier > > reasons why you would not support PTP, because you don't know where your > > registers are. And that document is not even up to date, it hasn't been > > updated when VCAP ES0, IS1, IS2 were added. I don't think that Horatiu > > even bothered to maintain backwards compatibility when he initially > > added tc-flower offload for VCAP IS2, and as a result, I did not bother > > either when extending it for the S0 and S1 targets. At some point > > afterwards, the Microchip people even stopped complaining and just went > > along with it. (the story is pretty much told from memory, I'm sorry if > > I mixed up some facts). It's pretty messy, and that's what you get for > > creating these micro-maps of registers spread through the guts of the > > SoC and then a separate reg-name for each. When we worked on the device > > tree for LS1028A and then T1040, it was very much a conscious decision > > for the driver to have a single, big register map and split it up pretty > > much in whichever way it wants to. In fact I think we wouldn't be > > having the discussion about how to split things right now if we didn't > > have that flexibility. > > ''' > > > > I'm happy to go any way. The two that make the most sense might be: > > > > micro-maps to make the VSC7512 "switch" portion match the VSC7514. The > > ethernet switch portion might still have to ignore these... > > > > A 'mega-map' that would also be ignored by the switch. It would be less > > arbitrary than the <0 0> that I went with. Maybe something like > > <0x70000000 0x02000000> to at least point to some valid region. > > A mega-map for the switch makes a lot more sense to me, if feasible > (it should not overlap with the regions of any other peripherals). It does overlap. At least DEVCPU_GCB and HSIO are in the middle of the mega-map. I'll stick with the 20-map solution for now. It does invoke this warning from dt_bindings_check though: /Documentation/devicetree/bindings/net/dsa/mscc,ocelot.example.dtb: soc@0: ethernet-switch@0:reg: [[1895890944, 65536], [1896022016, 65536], [1896349696, 256], [1896742912, 65536], [1897791488, 256], [1897857024, 256], [1897922560, 256], [1897988096, 256], [1898053632, 256], [1898119168, 256], [1898184704, 256], [1898250240, 256], [1898315776, 256], [1898381312, 256], [1898446848, 256], [1904214016, 524288], [1904738304, 65536], [189087552, 65536], [1896153088, 65536], [1896218624, 65536]] is too long > Something isn't quite right to me in having 20 reg-names for a single > device tree node, and I still stand for just describing the whole range > and letting the driver split it up according to its needs. I don't know > why this approach wasn't chosen for the ocelot switch and I did not have > the patience to map out the addresses that the peripherals use in the > Microchip SoCs relative to each other, so see if what I'm proposing is > possible. > > But on the other hand this also needs to be balanced with the fact that > one day, someone might come along with a mscc,vsc7514-switch that's SPI > controlled, and expect that the dt-bindings for it in DSA mode expect > the same reg-names that they do in switchdev mode. Or maybe they > wouldn't expect that, I don't know. In any case, for NXP switches I > didn't have a compatibility issue with switchdev-mode Ocelot to concern > myself with, so I went with what made the most sense.
On 08/10/2022 01:10, Vladimir Oltean wrote: > On Wed, Oct 05, 2022 at 10:09:06AM +0200, Krzysztof Kozlowski wrote: >>> The /spi/soc@0 node actually has a compatible of "mscc,vsc7512" which >>> Colin did not show in the example (it is not "simple-bus"). It is covered >>> by Documentation/devicetree/bindings/mfd/mscc,ocelot.yaml. Still waiting >>> for a better suggestion for how to name the mfd container node. >> >> Then still the /spi node does not seem related. If I understand >> correctly, your device described in this bindings is a child of soc@0. >> Sounds fine. How that soc@0 is connected to the parent - via SPI or >> whatever - is not related to this binding, is it? It is related to the >> soc binding, but not here. > > It's an example, it's meant to be informative. It is the first DSA > driver of its kind. When everybody else ATM puts the ethernet-switch node > under the &spi controller node, this puts it under &spi/soc@<chip-select>/, > for reasons that have to do with scalability. If the examples aren't a > good place to make this more obvious, I don't know why we don't just > tell people to RTFD. It still does not help me to understand why do you need that &spi. The device is part of the soc@CS and that's it. Where the soc@ is located, does not matter for this device, right? The example shows usage of this device, not of the soc@CS. Bindings for soc@CS should show how to use it inside spi etc. > >>> Unrelated to your "existing soc example" (the VSC9953), but relevant and >>> you may want to share your opinion on this: >>> >>> The same hardware present in the VSC7514 SoC can also be driven by an >>> integrated MIPS processor, and in that case, it is indeed expected that >>> the same dt-bindings cover both the /soc and the /spi/soc@0/ relative >>> positioning of their OF node. This is true for simpler peripherals like >>> "mscc,ocelot-miim", "mscc,ocelot-pinctrl", "mscc,ocelot-sgpio". However >>> it is not true for the main switching IP of the SoC itself. >>> >>> When driven by a switchdev driver, by the internal MIPS processor (the >>> DMA engine is what is used for packet I/O), the switching IP follows the >>> Documentation/devicetree/bindings/net/mscc,vsc7514-switch.yaml binding >>> document. >>> >>> When driven by a DSA driver (external processor, host frames are >>> redirected through an Ethernet port instead of DMA controller), >>> the switching IP follows the Documentation/devicetree/bindings/net/dsa/mscc,ocelot.yaml >>> document. >>> >>> The switching IP is special in this regard because the hardware is not >>> used in the same way. The DSA dt-binding also needs the 'ethernet' >>> phandle to be present in a port node. The different placement of the >>> bindings according to the use case of the hardware is a bit awkward, but >>> is a direct consequence of the separation between DSA and pure switchdev >>> drivers that has existed thus far (and the fact that DSA has its own >>> folder in the dt-bindings, with common properties in dsa.yaml and >>> dsa-port.yaml etc). It is relatively uncommon for a switching IP to have >>> provisioning to be used in both modes, and for Linux to support both >>> modes (using different drivers), yet this is what we have here. >> >> Is there a question here to me? What shall I do with this paragraph? You >> know, I do not have a problem of lack of material to read... > > For mscc,vsc7514-switch we have a switchdev driver. For mscc,vsc7512-switch, > Colin is working on a DSA driver. Their dt-bindings currently live in > different folders. The mscc,vsc7514-switch can also be used together > with a DSA driver, and support for that will inevitably be added. When > it will, how and where do you recommend the dt-bindings should be added? The bindings should in general describe the hardware, not the Linux drivers. I assume there is only one VSC7514 device, so there should be only one binding file. If bindings are correct, then this one hardware description can be used by two different driver implementations. That's said, for practical reasons entirely different implementations might require different bindings, but this should be rather exception requiring serious reasons. > In net/dsa/mscc,ocelot.yaml, together with the other switches used in > DSA mode, or in net/mscc,vsc7514-switch.yaml, because its compatible > string already exists there? We can't have a compatible string present > in multiple schemas, right? You can, if bindings are the same... but then why would you have the same bindings in two files? Which leads to solution: have only one binding file. If bindings are entirely different (and not compatible to each other), you cannot have same compatible in two different places... and this leads to paragraph before - there should be only one binding, thus only one place to document the compatible. > > This matters because it has implications upon what Colin should do with > the mscc,vsc7512-switch. If your answer to my question is "add $ref: dsa.yaml# > to net/mscc,vsc7514-switch.yaml", then I don't see why we wouldn't do > that now, and wait until the vsc7514 to make that move anyway. Best regards, Krzysztof
On 08/10/2022 02:00, Vladimir Oltean wrote: > On Wed, Oct 05, 2022 at 06:09:59PM +0200, Krzysztof Kozlowski wrote: >>>> I don't understand how your answer relates to "reg=<0 0>;". How is it >>>> going to become 0x71010000 if there is no other reg/ranges set in parent >>>> nodes. The node has only one IO address, but you say the switch has 20 >>>> addresses... >>>> >>>> Are we talking about same hardware? >>> >>> Yes. The switch driver for both the VSC7512 and VSC7514 use up to ~20 regmaps >>> depending on what capabilities it is to have. In the 7514 they are all >>> memory-mapped from the device tree. While the 7512 does need these >>> regmaps, they are managed by the MFD, not the device tree. So there >>> isn't a _need_ for them to be here, since at the end of the day they're >>> ignored. >>> >>> The "reg=<0 0>;" was my attempt to indicate that they are ignored, but I >>> understand that isn't desired. So moving forward I'll add all the >>> regmaps back into the device tree. >> >> You need to describe the hardware. If hardware has IO address space, how >> does it matter that some driver needs or needs not something? > > What do you mean by IO address space exactly? It is a SPI device with registers. > Does that constitute an IO address space to you? By IO I meant MMIO (or similar) which resides in reg (thus in unit address). The SPI devices have only chip-select as reg, AFAIR. > > The driver need matters because you don't usually see DT nodes of SPI, > I2C, MDIO devices describing the address space of their registers, and > having child nodes with unit addresses in that address space. Only when > those devices are so complex that the need arises to identify smaller > building blocks is when you will end up needing that. And this is an > implementation detail which shapes how the dt-bindings will look like. So probably I misunderstood here. If this is I2C or SPI device, then of course reg and unit address do not represent registers. > >> You mentioned that address space is mapped to regmaps. Regmap is Linux >> specific implementation detail, so this does not answer at all about >> hardware. >> >> On the other hand, if your DTS design requires this is a child of >> something else and by itself it does not have address space, it would be >> understandable to skip unit address entirely... but so far it is still >> confusing, especially that you use arguments related to implementation >> to justify the DTS. > > If Colin skips the unit address entirely, then how could he distinguish > between the otherwise identical MDIO controllers mdio@7107009c and > mdio@710700c0 from Documentation/devicetree/bindings/mfd/mscc,ocelot.yaml? > The ethernet-switch node added here is on the same hierarchical level > with the MDIO controller nodes, so it must have a unit address just like > them. So what is @710700c0? It's not chip-select, but MMIO or some other bus (specific to the device), right? The mscc,ocelot.yaml has a soc@0 SPI device. Children of soc@0 use unit addresses/reg meaningful for that soc@0. > > But I don't support Colin's choice of "reg=<0 0>;" either. A choice must > be made between 2 options: > - mapping all 20 regions of the SPI address space into "reg" values > - mapping a single region from the smallest until the largest address of > those 20, and hope nothing overlaps with some other peripheral, or > worse, that this region will never need to be expanded to the left. Yeah, at least to my limited knowledge of this hardware. > What information do you need to provide some best practices that can be > applied here and are more useful than "you need to describe the > hardware"? Describe the hardware properties in terms of it fit in to the whole system - so some inputs (clocks, GPIOs), outputs (interrupts), characteristics of a device (e.g. clock provider -> clock cells) and properties configuring hardware per specific implementation. But mostly this argument "describe hardware" should be understood like: do not describe software (Linux drivers) and software policies (driver choices)... > Verilog/VHDL is what the hardware description that's > independent of software implementation is, good luck parsing that. Best regards, Krzysztof
On Sun, Oct 09, 2022 at 12:14:22PM -0400, Krzysztof Kozlowski wrote: > On 08/10/2022 02:00, Vladimir Oltean wrote: > > On Wed, Oct 05, 2022 at 06:09:59PM +0200, Krzysztof Kozlowski wrote: > >>>> I don't understand how your answer relates to "reg=<0 0>;". How is it > >>>> going to become 0x71010000 if there is no other reg/ranges set in parent > >>>> nodes. The node has only one IO address, but you say the switch has 20 > >>>> addresses... > >>>> > >>>> Are we talking about same hardware? > >>> > >>> Yes. The switch driver for both the VSC7512 and VSC7514 use up to ~20 regmaps > >>> depending on what capabilities it is to have. In the 7514 they are all > >>> memory-mapped from the device tree. While the 7512 does need these > >>> regmaps, they are managed by the MFD, not the device tree. So there > >>> isn't a _need_ for them to be here, since at the end of the day they're > >>> ignored. > >>> > >>> The "reg=<0 0>;" was my attempt to indicate that they are ignored, but I > >>> understand that isn't desired. So moving forward I'll add all the > >>> regmaps back into the device tree. > >> > >> You need to describe the hardware. If hardware has IO address space, how > >> does it matter that some driver needs or needs not something? > > > > What do you mean by IO address space exactly? It is a SPI device with registers. > > Does that constitute an IO address space to you? > > By IO I meant MMIO (or similar) which resides in reg (thus in unit > address). The SPI devices have only chip-select as reg, AFAIR. Again, the SPI device (soc@0) has a unit address that describes the chip select, yes. The children of the SPI device have a unit address that describes the address space of the SPI registers of the mini-peripherals within that SPI device. > > The driver need matters because you don't usually see DT nodes of SPI, > > I2C, MDIO devices describing the address space of their registers, and > > having child nodes with unit addresses in that address space. Only when > > those devices are so complex that the need arises to identify smaller > > building blocks is when you will end up needing that. And this is an > > implementation detail which shapes how the dt-bindings will look like. > > So probably I misunderstood here. If this is I2C or SPI device, then of > course reg and unit address do not represent registers. Except we're not talking about the SPI device, I'm reminding you that we are talking about "reg = <0 0>" which Colin used to describe the /spi/soc@0/ethernet-switch node. Colin made the incorrect decision to describe "reg = <0 0>" for the switch OF node in an attempt to point out that "reg" will *not* be used by his implementation, whatever value it has. You may want to revisit some of the things that were said. What *is* used in the implementation is the array of resources from struct mfd_cell vsc7512_devs[] in drivers/mfd/ocelot-core.c, because MFD allows you to do this (and I suppose because it is more convenient than to rely on the DT). Colin's entire confusion comes from the fact that he thought it wouldn't be necessary to describe the unit addresses of MFD children if those addresses won't be retrieved from DT. > >> You mentioned that address space is mapped to regmaps. Regmap is Linux > >> specific implementation detail, so this does not answer at all about > >> hardware. > >> > >> On the other hand, if your DTS design requires this is a child of > >> something else and by itself it does not have address space, it would be > >> understandable to skip unit address entirely... but so far it is still > >> confusing, especially that you use arguments related to implementation > >> to justify the DTS. > > > > If Colin skips the unit address entirely, then how could he distinguish > > between the otherwise identical MDIO controllers mdio@7107009c and > > mdio@710700c0 from Documentation/devicetree/bindings/mfd/mscc,ocelot.yaml? > > The ethernet-switch node added here is on the same hierarchical level > > with the MDIO controller nodes, so it must have a unit address just like > > them. > > So what is @710700c0? @710700c0 is VSC7512_MIIM1_RES_START, i.e. the base address of the second MDIO controller embedded within the SoC (accessed over whatever; SPI or MMIO). > It's not chip-select, but MMIO or some other bus (specific to the > device), right? Yes, it is not chip select. Think of the /spi/soc@0 node as an AHB to SPI bridge (it is possibly not quite that, but for the sake of imagination it's a good enough description), and the children of /spi/soc@0 are nodes whose registers are accessed through that AHB to SPI bridge. The same addresses can also be accessed via direct MMIO by the processor *inside* the switch SoC, which in some cases can also run Linux (although not here in VSC7512, but in VSC7514). > The mscc,ocelot.yaml has a soc@0 SPI device. Children of soc@0 use unit > addresses/reg meaningful for that soc@0. Which they do. > > But I don't support Colin's choice of "reg=<0 0>;" either. A choice must > > be made between 2 options: > > - mapping all 20 regions of the SPI address space into "reg" values > > - mapping a single region from the smallest until the largest address of > > those 20, and hope nothing overlaps with some other peripheral, or > > worse, that this region will never need to be expanded to the left. > > Yeah, at least to my limited knowledge of this hardware. Yeah what? That a decision must be made? > > What information do you need to provide some best practices that can be > > applied here and are more useful than "you need to describe the > > hardware"? > > Describe the hardware properties in terms of it fit in to the whole > system - so some inputs (clocks, GPIOs), outputs (interrupts), > characteristics of a device (e.g. clock provider -> clock cells) and > properties configuring hardware per specific implementation. > > But mostly this argument "describe hardware" should be understood like: > do not describe software (Linux drivers) and software policies (driver > choices)... Let's bring this back on track. The discussion started with you saying: | soc in spi is a bit confusing. which I completely agree with, it really is. But it's also not wrong (or at least you didn't point out reasons why it would be, despite being asked to), and very descriptive of what actually takes place here: SoC registers are being accessed over SPI by an external host. If you're going to keep giving mechanical review to this, my fear is that a very complex set of schemas is going to fall through the cracks of bureaucracy, and while it will end up being formally correct, absolutely no one will understand what is actually required when piecing everything together. In your review of the example provided by Colin here, you first have this comment about "soc in spi" being confusing, then you seem to forget everything about that, and ask "How is this example different than previous one (existing soc example)?" There are more things to unpack in order to answer that. The main point is that we wanted to reuse the existing MMIO-based drivers when accessing the devices over SPI. So the majority of peripherals have the same dt-bindings whether they are on /soc or on /spi/soc. Linux also provides us with the mfd and regmap abstractions, so all is good there. So you are not completely wrong to expect that an ethernet-switch with the "mscc,vsc7512-switch" compatible string should have the same bindings regardless of whatever its parent is. Except this is not actually true, and the risk is that this will appear as seamless as just that when it actually isn't. First (and here Colin is also wrong), the switch Colin adds support for is not directly comparable with "the existing soc example" (vsc9953). That is different (NXP) hardware which just happens to be supported by the same driver (drivers/net/dsa/ocelot). It's worth reiterating that dissimilar hardware driven by a common driver should not necessarily have the same dt-bindings. Case in point, the NXP switches have a single (larger) "reg", because the SoC integration was tidier and the switch doesn't have 20 regions spread out through the SoC's guts, which overlap with other peripherals as in the case of VSC7512/VSC7514. Anyway, Colin's SPI-controlled VSC7512 switch is most similar to mscc,vsc7514-switch.yaml (to the point of the hardware being identical), and I believe that this is the schema he should append his information to, rather than what he's currently proposing in his patches. *But* accessing an Ethernet switch over SPI is not functionally identical to accessing it over MMIO, unless you want to have an Ethernet throughput in the order of tens of bits per second. This is where implementation starts to matter, and while mscc,vsc7514-switch.yaml describes a switch where packets are sent and received over MMIO (which wouldn't be feasible over SPI), Colin's VSC7512 schema describes a switch used in DSA mode (packets are sent/received over a host Ethernet port, fact which helps overcome the bandwidth limitations of SPI). To make matters worse, even VSC7514 can be used in DSA mode. When used in DSA mode, a *different* driver, with *different* dt-bindings (because of different histories) controls it. So what must be done is that mscc,vsc7514-switch.yaml must incorporate *elements* of dsa.yaml, but *only* when it is not accessed using MMIO (i.e. the Linux on the MIPS VSC7514 doesn't support an external host driving the switch in DSA mode). I was kind of expecting this discussion to converge towards ways in which we can modify mscc,vsc7514-switch.yaml to support a switch used in DSA mode or in switchdev mode. Most notable in dsa-port.yaml is the presence of an 'ethernet' phandle, but there are also other subtle differences, like the 'label' property which mscc,vsc7514-switch.yaml does not have (and which in the switchdev implementation at drivers/net/ethernet/mscc/ does not support, either).
On 10/10/2022 09:07, Vladimir Oltean wrote: > On Sun, Oct 09, 2022 at 12:14:22PM -0400, Krzysztof Kozlowski wrote: >> On 08/10/2022 02:00, Vladimir Oltean wrote: >>> On Wed, Oct 05, 2022 at 06:09:59PM +0200, Krzysztof Kozlowski wrote: >>>>>> I don't understand how your answer relates to "reg=<0 0>;". How is it >>>>>> going to become 0x71010000 if there is no other reg/ranges set in parent >>>>>> nodes. The node has only one IO address, but you say the switch has 20 >>>>>> addresses... >>>>>> >>>>>> Are we talking about same hardware? >>>>> >>>>> Yes. The switch driver for both the VSC7512 and VSC7514 use up to ~20 regmaps >>>>> depending on what capabilities it is to have. In the 7514 they are all >>>>> memory-mapped from the device tree. While the 7512 does need these >>>>> regmaps, they are managed by the MFD, not the device tree. So there >>>>> isn't a _need_ for them to be here, since at the end of the day they're >>>>> ignored. >>>>> >>>>> The "reg=<0 0>;" was my attempt to indicate that they are ignored, but I >>>>> understand that isn't desired. So moving forward I'll add all the >>>>> regmaps back into the device tree. >>>> >>>> You need to describe the hardware. If hardware has IO address space, how >>>> does it matter that some driver needs or needs not something? >>> >>> What do you mean by IO address space exactly? It is a SPI device with registers. >>> Does that constitute an IO address space to you? >> >> By IO I meant MMIO (or similar) which resides in reg (thus in unit >> address). The SPI devices have only chip-select as reg, AFAIR. > > Again, the SPI device (soc@0) has a unit address that describes the chip > select, yes. The children of the SPI device have a unit address that > describes the address space of the SPI registers of the mini-peripherals > within that SPI device. > >>> The driver need matters because you don't usually see DT nodes of SPI, >>> I2C, MDIO devices describing the address space of their registers, and >>> having child nodes with unit addresses in that address space. Only when >>> those devices are so complex that the need arises to identify smaller >>> building blocks is when you will end up needing that. And this is an >>> implementation detail which shapes how the dt-bindings will look like. >> >> So probably I misunderstood here. If this is I2C or SPI device, then of >> course reg and unit address do not represent registers. > > Except we're not talking about the SPI device, I'm reminding you that we > are talking about "reg = <0 0>" which Colin used to describe the > /spi/soc@0/ethernet-switch node. > > Colin made the incorrect decision to describe "reg = <0 0>" for the > switch OF node in an attempt to point out that "reg" will *not* be used > by his implementation, whatever value it has. You may want to revisit > some of the things that were said. > > What *is* used in the implementation is the array of resources from > struct mfd_cell vsc7512_devs[] in drivers/mfd/ocelot-core.c, because MFD > allows you to do this (and I suppose because it is more convenient than > to rely on the DT). Colin's entire confusion comes from the fact that he > thought it wouldn't be necessary to describe the unit addresses of MFD > children if those addresses won't be retrieved from DT. > >>>> You mentioned that address space is mapped to regmaps. Regmap is Linux >>>> specific implementation detail, so this does not answer at all about >>>> hardware. >>>> >>>> On the other hand, if your DTS design requires this is a child of >>>> something else and by itself it does not have address space, it would be >>>> understandable to skip unit address entirely... but so far it is still >>>> confusing, especially that you use arguments related to implementation >>>> to justify the DTS. >>> >>> If Colin skips the unit address entirely, then how could he distinguish >>> between the otherwise identical MDIO controllers mdio@7107009c and >>> mdio@710700c0 from Documentation/devicetree/bindings/mfd/mscc,ocelot.yaml? >>> The ethernet-switch node added here is on the same hierarchical level >>> with the MDIO controller nodes, so it must have a unit address just like >>> them. >> >> So what is @710700c0? > > @710700c0 is VSC7512_MIIM1_RES_START, i.e. the base address of the > second MDIO controller embedded within the SoC (accessed over whatever; > SPI or MMIO). > >> It's not chip-select, but MMIO or some other bus (specific to the >> device), right? > > Yes, it is not chip select. Think of the /spi/soc@0 node as an AHB to > SPI bridge (it is possibly not quite that, but for the sake of imagination > it's a good enough description), and the children of /spi/soc@0 are > nodes whose registers are accessed through that AHB to SPI bridge. > The same addresses can also be accessed via direct MMIO by the processor > *inside* the switch SoC, which in some cases can also run Linux > (although not here in VSC7512, but in VSC7514). > >> The mscc,ocelot.yaml has a soc@0 SPI device. Children of soc@0 use unit >> addresses/reg meaningful for that soc@0. > > Which they do. > >>> But I don't support Colin's choice of "reg=<0 0>;" either. A choice must >>> be made between 2 options: >>> - mapping all 20 regions of the SPI address space into "reg" values >>> - mapping a single region from the smallest until the largest address of >>> those 20, and hope nothing overlaps with some other peripheral, or >>> worse, that this region will never need to be expanded to the left. >> >> Yeah, at least to my limited knowledge of this hardware. > > Yeah what? That a decision must be made? Yep. That's it. You ask me to learn this hardware, read datasheet and design it instead of Colin or other people working on it. I can give you generic guidelines how it should look, but that's it. > >>> What information do you need to provide some best practices that can be >>> applied here and are more useful than "you need to describe the >>> hardware"? >> >> Describe the hardware properties in terms of it fit in to the whole >> system - so some inputs (clocks, GPIOs), outputs (interrupts), >> characteristics of a device (e.g. clock provider -> clock cells) and >> properties configuring hardware per specific implementation. >> >> But mostly this argument "describe hardware" should be understood like: >> do not describe software (Linux drivers) and software policies (driver >> choices)... > > Let's bring this back on track. The discussion started with you saying: > > | soc in spi is a bit confusing. > > which I completely agree with, it really is. But it's also not wrong > (or at least you didn't point out reasons why it would be, despite being > asked to), and very descriptive of what actually takes place here: > SoC registers are being accessed over SPI by an external host. My comment was not only about this. My comment was about soc@0 not having reg. And then having ethernet@0 with reg=<0,0> which is unusual, because - as you explained - it is not a SPI device. > > If you're going to keep giving mechanical review to this, my fear is > that a very complex set of schemas is going to fall through the cracks > of bureaucracy, and while it will end up being formally correct, > absolutely no one will understand what is actually required when > piecing everything together. > > In your review of the example provided by Colin here, you first have > this comment about "soc in spi" being confusing, then you seem to forget > everything about that, and ask "How is this example different than > previous one (existing soc example)?" That one was a different topic, but we stopped discussing it. You explained the differences and its fine. > > There are more things to unpack in order to answer that. > > The main point is that we wanted to reuse the existing MMIO-based > drivers when accessing the devices over SPI. So the majority of > peripherals have the same dt-bindings whether they are on /soc or on > /spi/soc. Linux also provides us with the mfd and regmap abstractions, > so all is good there. So you are not completely wrong to expect that an > ethernet-switch with the "mscc,vsc7512-switch" compatible string should > have the same bindings regardless of whatever its parent is. > > Except this is not actually true, and the risk is that this will appear > as seamless as just that when it actually isn't. > > First (and here Colin is also wrong), the switch Colin adds support for > is not directly comparable with "the existing soc example" (vsc9953). > That is different (NXP) hardware which just happens to be supported by > the same driver (drivers/net/dsa/ocelot). If it is different hardware, then you have different compatible, so why this is a problem? > It's worth reiterating that > dissimilar hardware driven by a common driver should not necessarily > have the same dt-bindings. Which is obvious... > Case in point, the NXP switches have a single > (larger) "reg", because the SoC integration was tidier and the switch > doesn't have 20 regions spread out through the SoC's guts, which overlap > with other peripherals as in the case of VSC7512/VSC7514. > > Anyway, Colin's SPI-controlled VSC7512 switch is most similar to > mscc,vsc7514-switch.yaml (to the point of the hardware being identical), > and I believe that this is the schema he should append his information to, > rather than what he's currently proposing in his patches. > > *But* accessing an Ethernet switch over SPI is not functionally > identical to accessing it over MMIO, unless you want to have an Ethernet > throughput in the order of tens of bits per second. > > This is where implementation starts to matter, and while mscc,vsc7514-switch.yaml Not really, implementation still does not matter to the bindings and that argument proves nothing. No one forces you to model it as SPI in bindings... > describes a switch where packets are sent and received over MMIO (which > wouldn't be feasible over SPI), Colin's VSC7512 schema describes a > switch used in DSA mode (packets are sent/received over a host Ethernet > port, fact which helps overcome the bandwidth limitations of SPI). > To make matters worse, even VSC7514 can be used in DSA mode. When used > in DSA mode, a *different* driver, with *different* dt-bindings (because > of different histories) controls it. The histories also do not matter here - you can deprecate bindings, e.g. with a new compatible, and write everything a bit more generic (to cover different setups). > > So what must be done is that mscc,vsc7514-switch.yaml must incorporate > *elements* of dsa.yaml, but *only* when it is not accessed using MMIO > (i.e. the Linux on the MIPS VSC7514 doesn't support an external host > driving the switch in DSA mode). Yes and? You write such stuff like there was any objection from my side... > > I was kind of expecting this discussion to converge towards ways in > which we can modify mscc,vsc7514-switch.yaml to support a switch used > in DSA mode or in switchdev mode. Most notable in dsa-port.yaml is the > presence of an 'ethernet' phandle, but there are also other subtle > differences, like the 'label' property which mscc,vsc7514-switch.yaml > does not have (and which in the switchdev implementation at > drivers/net/ethernet/mscc/ does not support, either). What stops you from doing that? What do you need from me? Best regards, Krzysztof
On Mon, Oct 10, 2022 at 09:37:23AM -0400, Krzysztof Kozlowski wrote:
> What stops you from doing that? What do you need from me?
To end the discussion on a constructive note, I think if I were Colin,
I would do the following, in the following order, according to what was
expressed as a constraint:
1. Reword the "driver" word out of mscc,vsc7514-switch.yaml and express
the description in terms of what the switch can do, not what the
driver can do.
2. Make qca8k.yaml have "$ref: dsa.yaml#". Remove "$ref: dsa-port.yaml#"
from the same schema.
3. Remove "- $ref: dsa-port.yaml#" from mediatek,mt7530.yaml. It doesn't
seem to be needed, since dsa.yaml also has this. We need this because
we want to make sure no one except dsa.yaml references dsa-port.yaml.
4. Move the DSA-unspecific portion from dsa.yaml into a new
ethernet-switch.yaml. What remains in dsa.yaml is "dsa,member".
The dsa.yaml schema will have "$ref: ethernet-switch.yaml#" for the
"(ethernet-)switch" node, plus its custom additions.
5. Move the DSA-unspecific portion from dsa-port.yaml into a new
ethernet-switch-port.yaml. What remains in dsa-port.yaml is:
* ethernet phandle
* link phandle
* label property
* dsa-tag-protocol property
* the constraint that CPU and DSA ports must have phylink bindings
6. The ethernet-switch.yaml will have "$ref: ethernet-switch-port.yaml#"
and "$ref: dsa-port.yaml". The dsa-port.yaml schema will *not* have
"$ref: ethernet-switch-port.yaml#", just its custom additions.
I'm not 100% on this, but I think there will be a problem if:
- dsa.yaml references ethernet-switch.yaml
- ethernet-switch.yaml references ethernet-switch-port.yaml
- dsa.yaml also references dsa-port.yaml
- dsa-port.yaml references ethernet-switch-port.yaml
because ethernet-switch-port.yaml will be referenced twice. Again,
not sure if this is a problem. If it isn't, things can be simpler,
just make dsa-port.yaml reference ethernet-switch-port.yaml, and skip
steps 2 and 3 since dsa-port.yaml containing just the DSA specifics
is no longer problematic.
7. Make mscc,vsc7514-switch.yaml have "$ref: ethernet-switch.yaml#" for
the "mscc,vsc7514-switch.yaml" compatible string. This will eliminate
its own definitions for the generic properties: $nodename and
ethernet-ports (~45 lines of code if I'm not mistaken).
8. Introduce the "mscc,vsc7512-switch" compatible string as part of
mscc,vsc7514-switch.yaml, but this will have "$ref: dsa.yaml#" (this
will have to be referenced by full path because they are in different
folders) instead of "ethernet-switch.yaml". Doing this will include
the common bindings for a switch, plus the DSA specifics.
9. Optional: rework ti,cpsw-switch.yaml, microchip,lan966x-switch.yaml,
microchip,sparx5-switch.yaml to have "$ref: ethernet-switch.yaml#"
which should reduce some duplication in existing schemas.
10. Question for future support of VSC7514 in DSA mode: how do we decide
whether to $ref: ethernet-switch.yaml or dsa.yaml? If the parent MFD
node has a compatible string similar to "mscc,vsc7512", then use DSA,
otherwise use generic ethernet-switch?
Colin, how does this sound?
On Mon, Oct 10, 2022 at 08:48:56PM +0300, Vladimir Oltean wrote: > On Mon, Oct 10, 2022 at 09:37:23AM -0400, Krzysztof Kozlowski wrote: > > What stops you from doing that? What do you need from me? > > To end the discussion on a constructive note, I think if I were Colin, > I would do the following, in the following order, according to what was > expressed as a constraint: > > 1. Reword the "driver" word out of mscc,vsc7514-switch.yaml and express > the description in terms of what the switch can do, not what the > driver can do. > > 2. Make qca8k.yaml have "$ref: dsa.yaml#". Remove "$ref: dsa-port.yaml#" > from the same schema. > > 3. Remove "- $ref: dsa-port.yaml#" from mediatek,mt7530.yaml. It doesn't > seem to be needed, since dsa.yaml also has this. We need this because > we want to make sure no one except dsa.yaml references dsa-port.yaml. > > 4. Move the DSA-unspecific portion from dsa.yaml into a new > ethernet-switch.yaml. What remains in dsa.yaml is "dsa,member". > The dsa.yaml schema will have "$ref: ethernet-switch.yaml#" for the > "(ethernet-)switch" node, plus its custom additions. > > 5. Move the DSA-unspecific portion from dsa-port.yaml into a new > ethernet-switch-port.yaml. What remains in dsa-port.yaml is: > * ethernet phandle > * link phandle > * label property > * dsa-tag-protocol property > * the constraint that CPU and DSA ports must have phylink bindings > > 6. The ethernet-switch.yaml will have "$ref: ethernet-switch-port.yaml#" > and "$ref: dsa-port.yaml". The dsa-port.yaml schema will *not* have > "$ref: ethernet-switch-port.yaml#", just its custom additions. > I'm not 100% on this, but I think there will be a problem if: > - dsa.yaml references ethernet-switch.yaml > - ethernet-switch.yaml references ethernet-switch-port.yaml > - dsa.yaml also references dsa-port.yaml > - dsa-port.yaml references ethernet-switch-port.yaml > because ethernet-switch-port.yaml will be referenced twice. Again, > not sure if this is a problem. If it isn't, things can be simpler, > just make dsa-port.yaml reference ethernet-switch-port.yaml, and skip > steps 2 and 3 since dsa-port.yaml containing just the DSA specifics > is no longer problematic. > > 7. Make mscc,vsc7514-switch.yaml have "$ref: ethernet-switch.yaml#" for > the "mscc,vsc7514-switch.yaml" compatible string. This will eliminate > its own definitions for the generic properties: $nodename and > ethernet-ports (~45 lines of code if I'm not mistaken). > > 8. Introduce the "mscc,vsc7512-switch" compatible string as part of > mscc,vsc7514-switch.yaml, but this will have "$ref: dsa.yaml#" (this > will have to be referenced by full path because they are in different > folders) instead of "ethernet-switch.yaml". Doing this will include > the common bindings for a switch, plus the DSA specifics. > > 9. Optional: rework ti,cpsw-switch.yaml, microchip,lan966x-switch.yaml, > microchip,sparx5-switch.yaml to have "$ref: ethernet-switch.yaml#" > which should reduce some duplication in existing schemas. > > 10. Question for future support of VSC7514 in DSA mode: how do we decide > whether to $ref: ethernet-switch.yaml or dsa.yaml? If the parent MFD > node has a compatible string similar to "mscc,vsc7512", then use DSA, > otherwise use generic ethernet-switch? > > Colin, how does this sound? Thank you for laying this path out for me. Hopefully when I go heads-down to implement this there won't be any gotchas. It seems pretty straightforward. Maybe my only question would be where to send these patches. If these can all go through net-next it seems like there'd be no issue when step 8 (add 7512 documentation) comes along with this current patch set. Otherwise this sounds good. I'll switch to getting a patch set of steps 1-7 as you suggest.
On Mon, Oct 10, 2022 at 11:47:09AM -0700, Colin Foster wrote: > Thank you for laying this path out for me. Hopefully when I go > heads-down to implement this there won't be any gotchas. It seems pretty > straightforward. > > Maybe my only question would be where to send these patches. If these > can all go through net-next it seems like there'd be no issue when step > 8 (add 7512 documentation) comes along with this current patch set. > > Otherwise this sounds good. I'll switch to getting a patch set of steps > 1-7 as you suggest. Generally patches on dt-bindings go through the subsystem to which they belong, for example net-next etc. I don't think there are other dependencies?
On Mon, Oct 10, 2022 at 11:47:09AM -0700, Colin Foster wrote: > Thank you for laying this path out for me. Hopefully when I go > heads-down to implement this there won't be any gotchas. It seems pretty > straightforward. > > Maybe my only question would be where to send these patches. If these > can all go through net-next it seems like there'd be no issue when step > 8 (add 7512 documentation) comes along with this current patch set. > > Otherwise this sounds good. I'll switch to getting a patch set of steps > 1-7 as you suggest. I have some doubts whether it would be good to also merge net/dsa/mscc,ocelot.yaml (i.e. the NXP variants) into net/mscc,vsc7514-switch.yaml. A few "if" statements on compatible string which decide the format of "reg" and of "reg-names" should cover the differences. Not sure how the end result will look like.
On Mon, Oct 10, 2022 at 08:48:56PM +0300, Vladimir Oltean wrote: > On Mon, Oct 10, 2022 at 09:37:23AM -0400, Krzysztof Kozlowski wrote: > > What stops you from doing that? What do you need from me? > > To end the discussion on a constructive note, I think if I were Colin, > I would do the following, in the following order, according to what was > expressed as a constraint: > ... > 8. Introduce the "mscc,vsc7512-switch" compatible string as part of > mscc,vsc7514-switch.yaml, but this will have "$ref: dsa.yaml#" (this > will have to be referenced by full path because they are in different > folders) instead of "ethernet-switch.yaml". Doing this will include > the common bindings for a switch, plus the DSA specifics. Resurrecting this conversation for a quick question / feedback, now that steps 1-7 are essentially done with everyone's help. I don't want to send out a full RFC / Patch, since I can't currently test on hardware this week. But I'd really like feedback on the documentation change that is coming up. And I also don't want to necessarily do a separate RFC for just this patch. What happens here is interrupts and interrupt-names work as expected. They're required for the 7514, and optional for the 7512. Fantastic. I'm not sure if the "$ref: ethernet-switch.yaml" and "$ref: /schemas/net/dsa/dsa.yaml#" have an effect, since removing that line outright doesn't seem to have an effect on dt_bindings_check. The "fdma" doesn't make sense for the 7512, and seems to be handled correctly by way of maxItems for the two scenarios. The big miss in this patch is ethernet-switch-port vs dsa-port in the two scenarios. It isn't working as I'd hoped, where the 7514 pulls in ethernet-switch-port.yaml and the 7512 pulls in dsa-port.yaml. To squash errors about the incorrect "ethernet" property I switched this line: - $ref: ethernet-switch-port.yaml# + $ref: /schemas/net/dsa/dsa-port.yaml# ... knowing full well that the correct solution should be along the lines of "remove this, and only reference them in the conditional". That doesn't seem to work though... Is what I'm trying to do possible? I utilized Documentation/devicetree/bindings/net/dsa/*.yaml and Documentation/devicetree/bindings/net/*.yaml and found examples to get to my current state. diff --git a/Documentation/devicetree/bindings/net/mscc,vsc7514-switch.yaml b/Documentation/devicetree/bindings/net/mscc,vsc7514-switch.yaml index 5ffe831e59e4..f012c64a0da3 100644 --- a/Documentation/devicetree/bindings/net/mscc,vsc7514-switch.yaml +++ b/Documentation/devicetree/bindings/net/mscc,vsc7514-switch.yaml @@ -18,13 +18,50 @@ description: | packets using CPU. Additionally, PTP is supported as well as FDMA for faster packet extraction/injection. -$ref: ethernet-switch.yaml# +allOf: + - if: + properties: + compatible: + const: mscc,vsc7514-switch + then: + $ref: ethernet-switch.yaml# + required: + - interrupts + - interrupt-names + properties: + reg: + minItems: 21 + reg-names: + minItems: 21 + ethernet-ports: + patternProperties: + "^port@[0-9a-f]+$": + $ref: ethernet-switch-port.yaml# + + - if: + properties: + compatible: + const: mscc,vsc7512-switch + then: + $ref: /schemas/net/dsa/dsa.yaml# + properties: + reg: + maxItems: 20 + reg-names: + maxItems: 20 + ethernet-ports: + patternProperties: + "^port@[0-9a-f]+$": + $ref: /schemas/net/dsa/dsa-port.yaml# properties: compatible: - const: mscc,vsc7514-switch + enum: + - mscc,vsc7512-switch + - mscc,vsc7514-switch reg: + minItems: 20 items: - description: system target - description: rewriter target @@ -49,6 +86,7 @@ properties: - description: fdma target reg-names: + minItems: 20 items: - const: sys - const: rew @@ -100,7 +138,7 @@ properties: patternProperties: "^port@[0-9a-f]+$": - $ref: ethernet-switch-port.yaml# + $ref: /schemas/net/dsa/dsa-port.yaml# unevaluatedProperties: false @@ -108,13 +146,12 @@ required: - compatible - reg - reg-names - - interrupts - - interrupt-names - ethernet-ports additionalProperties: false examples: + # VSC7514 (Switchdev) - | switch@1010000 { compatible = "mscc,vsc7514-switch"; @@ -162,5 +199,51 @@ examples: }; }; }; + # VSC7512 (DSA) + - | + ethernet-switch@1{ + compatible = "mscc,vsc7512-switch"; + reg = <0x71010000 0x10000>, + <0x71030000 0x10000>, + <0x71080000 0x100>, + <0x710e0000 0x10000>, + <0x711e0000 0x100>, + <0x711f0000 0x100>, + <0x71200000 0x100>, + <0x71210000 0x100>, + <0x71220000 0x100>, + <0x71230000 0x100>, + <0x71240000 0x100>, + <0x71250000 0x100>, + <0x71260000 0x100>, + <0x71270000 0x100>, + <0x71280000 0x100>, + <0x71800000 0x80000>, + <0x71880000 0x10000>, + <0x71040000 0x10000>, + <0x71050000 0x10000>, + <0x71060000 0x10000>; + reg-names = "sys", "rew", "qs", "ptp", "port0", "port1", + "port2", "port3", "port4", "port5", "port6", + "port7", "port8", "port9", "port10", "qsys", + "ana", "s0", "s1", "s2"; + + ethernet-ports { + #address-cells = <1>; + #size-cells = <0>; + + port@0 { + reg = <0>; + ethernet = <&mac_sw>; + phy-handle = <&phy0>; + phy-mode = "internal"; + }; + port@1 { + reg = <1>; + phy-handle = <&phy1>; + phy-mode = "internal"; + }; + }; + };
Hi Colin, On Wed, Jan 18, 2023 at 12:28:36PM -1000, Colin Foster wrote: > Resurrecting this conversation for a quick question / feedback, now that > steps 1-7 are essentially done with everyone's help. > > I don't want to send out a full RFC / Patch, since I can't currently > test on hardware this week. But I'd really like feedback on the > documentation change that is coming up. And I also don't want to > necessarily do a separate RFC for just this patch. > > What happens here is interrupts and interrupt-names work as expected. > They're required for the 7514, and optional for the 7512. Fantastic. > > I'm not sure if the "$ref: ethernet-switch.yaml" and > "$ref: /schemas/net/dsa/dsa.yaml#" have an effect, since removing that > line outright doesn't seem to have an effect on dt_bindings_check. > > The "fdma" doesn't make sense for the 7512, and seems to be handled > correctly by way of maxItems for the two scenarios. > > > The big miss in this patch is ethernet-switch-port vs dsa-port in the > two scenarios. It isn't working as I'd hoped, where the 7514 pulls in > ethernet-switch-port.yaml and the 7512 pulls in dsa-port.yaml. To squash > errors about the incorrect "ethernet" property I switched this line: > > - $ref: ethernet-switch-port.yaml# > + $ref: /schemas/net/dsa/dsa-port.yaml# > > ... knowing full well that the correct solution should be along the > lines of "remove this, and only reference them in the conditional". That > doesn't seem to work though... > > Is what I'm trying to do possible? I utilized > Documentation/devicetree/bindings/net/dsa/*.yaml and > Documentation/devicetree/bindings/net/*.yaml and found examples to get > to my current state. > > > diff --git a/Documentation/devicetree/bindings/net/mscc,vsc7514-switch.yaml b/Documentation/devicetree/bindings/net/mscc,vsc7514-switch.yaml > index 5ffe831e59e4..f012c64a0da3 100644 > --- a/Documentation/devicetree/bindings/net/mscc,vsc7514-switch.yaml > +++ b/Documentation/devicetree/bindings/net/mscc,vsc7514-switch.yaml > @@ -18,13 +18,50 @@ description: | > packets using CPU. Additionally, PTP is supported as well as FDMA for faster > packet extraction/injection. > > -$ref: ethernet-switch.yaml# > +allOf: > + - if: > + properties: > + compatible: > + const: mscc,vsc7514-switch > + then: > + $ref: ethernet-switch.yaml# > + required: > + - interrupts > + - interrupt-names > + properties: > + reg: > + minItems: 21 > + reg-names: > + minItems: 21 > + ethernet-ports: > + patternProperties: > + "^port@[0-9a-f]+$": > + $ref: ethernet-switch-port.yaml# > + > + - if: > + properties: > + compatible: > + const: mscc,vsc7512-switch > + then: > + $ref: /schemas/net/dsa/dsa.yaml# > + properties: > + reg: > + maxItems: 20 > + reg-names: > + maxItems: 20 > + ethernet-ports: > + patternProperties: > + "^port@[0-9a-f]+$": > + $ref: /schemas/net/dsa/dsa-port.yaml# > > properties: > compatible: > - const: mscc,vsc7514-switch > + enum: > + - mscc,vsc7512-switch > + - mscc,vsc7514-switch > > reg: > + minItems: 20 > items: > - description: system target > - description: rewriter target > @@ -49,6 +86,7 @@ properties: > - description: fdma target > > reg-names: > + minItems: 20 > items: > - const: sys > - const: rew > @@ -100,7 +138,7 @@ properties: > patternProperties: > "^port@[0-9a-f]+$": > > - $ref: ethernet-switch-port.yaml# > + $ref: /schemas/net/dsa/dsa-port.yaml# > > unevaluatedProperties: false I'm not sure at all why this chunk (is sub-schema the right word) even exists, considering you have the other one?! > > @@ -108,13 +146,12 @@ required: > - compatible > - reg > - reg-names > - - interrupts > - - interrupt-names > - ethernet-ports > > additionalProperties: false This should be "unevaluatedProperties: false" I guess? Maybe this is why deleting the ethernet-switch.yaml or dsa.yaml schema appears to do nothing? The following delta compared to net-next works for me, I think: diff --git a/Documentation/devicetree/bindings/net/mscc,vsc7514-switch.yaml b/Documentation/devicetree/bindings/net/mscc,vsc7514-switch.yaml index 5ffe831e59e4..dc3319ea40b9 100644 --- a/Documentation/devicetree/bindings/net/mscc,vsc7514-switch.yaml +++ b/Documentation/devicetree/bindings/net/mscc,vsc7514-switch.yaml @@ -18,13 +18,52 @@ description: | packets using CPU. Additionally, PTP is supported as well as FDMA for faster packet extraction/injection. -$ref: ethernet-switch.yaml# +allOf: + - if: + properties: + compatible: + const: mscc,vsc7514-switch + then: + $ref: ethernet-switch.yaml# + required: + - interrupts + - interrupt-names + properties: + reg: + minItems: 21 + reg-names: + minItems: 21 + ethernet-ports: + patternProperties: + "^port@[0-9a-f]+$": + $ref: ethernet-switch-port.yaml# + unevaluatedProperties: false + + - if: + properties: + compatible: + const: mscc,vsc7512-switch + then: + $ref: /schemas/net/dsa/dsa.yaml# + properties: + reg: + maxItems: 20 + reg-names: + maxItems: 20 + ethernet-ports: + patternProperties: + "^port@[0-9a-f]+$": + $ref: /schemas/net/dsa/dsa-port.yaml# + unevaluatedProperties: false properties: compatible: - const: mscc,vsc7514-switch + enum: + - mscc,vsc7512-switch + - mscc,vsc7514-switch reg: + minItems: 20 items: - description: system target - description: rewriter target @@ -49,6 +88,7 @@ properties: - description: fdma target reg-names: + minItems: 20 items: - const: sys - const: rew @@ -86,35 +126,16 @@ properties: - const: xtr - const: fdma - ethernet-ports: - type: object - - properties: - '#address-cells': - const: 1 - '#size-cells': - const: 0 - - additionalProperties: false - - patternProperties: - "^port@[0-9a-f]+$": - - $ref: ethernet-switch-port.yaml# - - unevaluatedProperties: false - required: - compatible - reg - reg-names - - interrupts - - interrupt-names - ethernet-ports -additionalProperties: false +unevaluatedProperties: false examples: + # VSC7514 (Switchdev) - | switch@1010000 { compatible = "mscc,vsc7514-switch"; @@ -154,6 +175,7 @@ examples: reg = <0>; phy-handle = <&phy0>; phy-mode = "internal"; + ethernet = <&mac_sw>; # fails validation as expected }; port1: port@1 { reg = <1>; @@ -162,5 +184,51 @@ examples: }; }; }; + # VSC7512 (DSA) + - | + ethernet-switch@1{ + compatible = "mscc,vsc7512-switch"; + reg = <0x71010000 0x10000>, + <0x71030000 0x10000>, + <0x71080000 0x100>, + <0x710e0000 0x10000>, + <0x711e0000 0x100>, + <0x711f0000 0x100>, + <0x71200000 0x100>, + <0x71210000 0x100>, + <0x71220000 0x100>, + <0x71230000 0x100>, + <0x71240000 0x100>, + <0x71250000 0x100>, + <0x71260000 0x100>, + <0x71270000 0x100>, + <0x71280000 0x100>, + <0x71800000 0x80000>, + <0x71880000 0x10000>, + <0x71040000 0x10000>, + <0x71050000 0x10000>, + <0x71060000 0x10000>; + reg-names = "sys", "rew", "qs", "ptp", "port0", "port1", + "port2", "port3", "port4", "port5", "port6", + "port7", "port8", "port9", "port10", "qsys", + "ana", "s0", "s1", "s2"; + + ethernet-ports { + #address-cells = <1>; + #size-cells = <0>; + + port@0 { + reg = <0>; + ethernet = <&mac_sw>; + phy-handle = <&phy0>; + phy-mode = "internal"; + }; + port@1 { + reg = <1>; + phy-handle = <&phy1>; + phy-mode = "internal"; + }; + }; + }; ... Of course this is a completely uneducated attempt on my part.
On Thu, Jan 19, 2023 at 10:21:13PM +0200, Vladimir Oltean wrote: > Hi Colin, > > On Wed, Jan 18, 2023 at 12:28:36PM -1000, Colin Foster wrote: > > - ethernet-ports: > - type: object > - > - properties: > - '#address-cells': > - const: 1 > - '#size-cells': > - const: 0 > - > - additionalProperties: false > - > - patternProperties: > - "^port@[0-9a-f]+$": > - > - $ref: ethernet-switch-port.yaml# > - > - unevaluatedProperties: false > - I think removing this entire section was the one thing I didn't try. And sure enough - it seems to work exactly as I'd hope! Thanks! Next week I'll do some verification and will hopefully get the next patch set sent out. > required: > - compatible > - reg > - reg-names > - - interrupts > - - interrupt-names > - ethernet-ports > > -additionalProperties: false > +unevaluatedProperties: false > > examples: > + # VSC7514 (Switchdev) > - | > switch@1010000 { > compatible = "mscc,vsc7514-switch"; > @@ -154,6 +175,7 @@ examples: > reg = <0>; > phy-handle = <&phy0>; > phy-mode = "internal"; > + ethernet = <&mac_sw>; # fails validation as expected > }; > port1: port@1 { > reg = <1>; > @@ -162,5 +184,51 @@ examples: > }; > }; > }; > + # VSC7512 (DSA) > + - | > + ethernet-switch@1{ > + compatible = "mscc,vsc7512-switch"; > + reg = <0x71010000 0x10000>, > + <0x71030000 0x10000>, > + <0x71080000 0x100>, > + <0x710e0000 0x10000>, > + <0x711e0000 0x100>, > + <0x711f0000 0x100>, > + <0x71200000 0x100>, > + <0x71210000 0x100>, > + <0x71220000 0x100>, > + <0x71230000 0x100>, > + <0x71240000 0x100>, > + <0x71250000 0x100>, > + <0x71260000 0x100>, > + <0x71270000 0x100>, > + <0x71280000 0x100>, > + <0x71800000 0x80000>, > + <0x71880000 0x10000>, > + <0x71040000 0x10000>, > + <0x71050000 0x10000>, > + <0x71060000 0x10000>; > + reg-names = "sys", "rew", "qs", "ptp", "port0", "port1", > + "port2", "port3", "port4", "port5", "port6", > + "port7", "port8", "port9", "port10", "qsys", > + "ana", "s0", "s1", "s2"; > + > + ethernet-ports { > + #address-cells = <1>; > + #size-cells = <0>; > + > + port@0 { > + reg = <0>; > + ethernet = <&mac_sw>; > + phy-handle = <&phy0>; > + phy-mode = "internal"; > + }; > + port@1 { > + reg = <1>; > + phy-handle = <&phy1>; > + phy-mode = "internal"; > + }; > + }; > + }; > > ... > > Of course this is a completely uneducated attempt on my part.
diff --git a/Documentation/devicetree/bindings/net/dsa/mscc,ocelot.yaml b/Documentation/devicetree/bindings/net/dsa/mscc,ocelot.yaml index 8d93ed9c172c..49450a04e589 100644 --- a/Documentation/devicetree/bindings/net/dsa/mscc,ocelot.yaml +++ b/Documentation/devicetree/bindings/net/dsa/mscc,ocelot.yaml @@ -54,9 +54,22 @@ description: | - phy-mode = "1000base-x": on ports 0, 1, 2, 3 - phy-mode = "2500base-x": on ports 0, 1, 2, 3 + VSC7412 (Ocelot-Ext): + + The Ocelot family consists of four devices, the VSC7511, VSC7512, VSC7513, + and the VSC7514. The VSC7513 and VSC7514 both have an internal MIPS + processor that natively support Linux. Additionally, all four devices + support control over external interfaces, SPI and PCIe. The Ocelot-Ext + driver is for the external control portion. + + The following PHY interface types are supported: + + - phy-mode = "internal": on ports 0, 1, 2, 3 + properties: compatible: enum: + - mscc,vsc7512-switch - mscc,vsc9953-switch - pci1957,eef0 @@ -258,3 +271,49 @@ examples: }; }; }; + # Ocelot-ext VSC7512 + - | + spi { + soc@0 { + compatible = "mscc,vsc7512"; + #address-cells = <1>; + #size-cells = <1>; + + ethernet-switch@0 { + compatible = "mscc,vsc7512-switch"; + reg = <0 0>; + + ethernet-ports { + #address-cells = <1>; + #size-cells = <0>; + + port@0 { + reg = <0>; + label = "cpu"; + ethernet = <&mac_sw>; + phy-handle = <&phy0>; + phy-mode = "internal"; + }; + + port@1 { + reg = <1>; + label = "swp1"; + phy-mode = "internal"; + phy-handle = <&phy1>; + }; + + port@2 { + reg = <2>; + phy-mode = "internal"; + phy-handle = <&phy2>; + }; + + port@3 { + reg = <3>; + phy-mode = "internal"; + phy-handle = <&phy3>; + }; + }; + }; + }; + };
The ocelot-ext driver is another sub-device of the Ocelot / Felix driver system, which currently supports the four internal copper phys. Signed-off-by: Colin Foster <colin.foster@in-advantage.com> --- v3 * Remove "currently supported" verbage The Seville and Felix 9959 all list their supported modes following the sentence "The following PHY interface types are supported". During V2, I had used "currently supported" to suggest more interface modes are around the corner, though this had raised questions. The suggestion was to drop the entire sentence. I did leave the modified sentence there because it exactly matches the other two supported products. v2 * New patch --- .../bindings/net/dsa/mscc,ocelot.yaml | 59 +++++++++++++++++++ 1 file changed, 59 insertions(+)