diff mbox series

[v3,net-next,12/14] dt-bindings: net: dsa: ocelot: add ocelot-ext documentation

Message ID 20220926002928.2744638-13-colin.foster@in-advantage.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series add support for the the vsc7512 internal copper phys | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for net-next, async
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix success Link
netdev/cover_letter success Series has a cover letter
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers success CCed 15 of 15 maintainers
netdev/build_clang success Errors and warnings before: 0 this patch: 0
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 71 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Colin Foster Sept. 26, 2022, 12:29 a.m. UTC
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(+)

Comments

Vladimir Oltean Sept. 27, 2022, 8:26 p.m. UTC | #1
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
>
Colin Foster Sept. 27, 2022, 10:20 p.m. UTC | #2
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.
Colin Foster Sept. 30, 2022, 9:15 p.m. UTC | #3
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?
Colin Foster Oct. 1, 2022, 12:20 a.m. UTC | #4
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)
Vladimir Oltean Oct. 3, 2022, 3:28 p.m. UTC | #5
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);
Krzysztof Kozlowski Oct. 4, 2022, 11:19 a.m. UTC | #6
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
Vladimir Oltean Oct. 4, 2022, 12:15 p.m. UTC | #7
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.
Krzysztof Kozlowski Oct. 4, 2022, 2:59 p.m. UTC | #8
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
Vladimir Oltean Oct. 4, 2022, 4:01 p.m. UTC | #9
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.
Colin Foster Oct. 5, 2022, 12:08 a.m. UTC | #10
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>,
      ...
Krzysztof Kozlowski Oct. 5, 2022, 8:03 a.m. UTC | #11
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
Krzysztof Kozlowski Oct. 5, 2022, 8:09 a.m. UTC | #12
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
Colin Foster Oct. 5, 2022, 3:44 p.m. UTC | #13
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
>
Krzysztof Kozlowski Oct. 5, 2022, 4:09 p.m. UTC | #14
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
Colin Foster Oct. 7, 2022, 8:44 p.m. UTC | #15
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.
Vladimir Oltean Oct. 7, 2022, 10:38 p.m. UTC | #16
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.
Vladimir Oltean Oct. 7, 2022, 10:48 p.m. UTC | #17
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.
Vladimir Oltean Oct. 7, 2022, 11:10 p.m. UTC | #18
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.
Vladimir Oltean Oct. 8, 2022, midnight UTC | #19
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.
Colin Foster Oct. 8, 2022, 5:56 p.m. UTC | #20
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.
Krzysztof Kozlowski Oct. 9, 2022, 3:49 p.m. UTC | #21
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
Krzysztof Kozlowski Oct. 9, 2022, 4:14 p.m. UTC | #22
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
Vladimir Oltean Oct. 10, 2022, 1:07 p.m. UTC | #23
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).
Krzysztof Kozlowski Oct. 10, 2022, 1:37 p.m. UTC | #24
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
Vladimir Oltean Oct. 10, 2022, 5:48 p.m. UTC | #25
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?
Colin Foster Oct. 10, 2022, 6:47 p.m. UTC | #26
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.
Vladimir Oltean Oct. 10, 2022, 7:11 p.m. UTC | #27
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?
Vladimir Oltean Oct. 11, 2022, 9:53 a.m. UTC | #28
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.
Colin Foster Jan. 18, 2023, 10:28 p.m. UTC | #29
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";
+          };
+        };
+      };
Vladimir Oltean Jan. 19, 2023, 8:21 p.m. UTC | #30
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.
Colin Foster Jan. 20, 2023, 6:16 p.m. UTC | #31
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 mbox series

Patch

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>;
+                    };
+                };
+            };
+        };
+    };