Date:   Sat, 8 Oct 2022 10:56:37 -0700
From:   Colin Foster <colin.foster@...advantage.com>
To:     Vladimir Oltean <olteanv@...il.com>
Cc:     linux-gpio@...r.kernel.org, linux-kernel@...r.kernel.org,
        devicetree@...r.kernel.org, netdev@...r.kernel.org,
        Russell King <linux@...linux.org.uk>,
        Linus Walleij <linus.walleij@...aro.org>,
        UNGLinuxDriver@...rochip.com,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        Claudiu Manoil <claudiu.manoil@....com>,
        Lee Jones <lee@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Rob Herring <robh+dt@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Eric Dumazet <edumazet@...gle.com>,
        "David S. Miller" <davem@...emloft.net>,
        Florian Fainelli <f.fainelli@...il.com>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Andrew Lunn <andrew@...n.ch>
Subject: Re: [PATCH v3 net-next 12/14] dt-bindings: net: dsa: ocelot: add
 ocelot-ext documentation

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.

Powered by blists - more mailing lists