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
