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

From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
To: Vladimir Oltean <olteanv@gmail.com>
Cc: Colin Foster <colin.foster@in-advantage.com>,
	linux-gpio@vger.kernel.org, linux-kernel@vger.kernel.org,
	devicetree@vger.kernel.org, netdev@vger.kernel.org,
	Russell King <linux@armlinux.org.uk>,
	Linus Walleij <linus.walleij@linaro.org>,
	UNGLinuxDriver@microchip.com,
	Alexandre Belloni <alexandre.belloni@bootlin.com>,
	Claudiu Manoil <claudiu.manoil@nxp.com>,
	Lee Jones <lee@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Rob Herring <robh+dt@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
	Jakub Kicinski <kuba@kernel.org>,
	Eric Dumazet <edumazet@google.com>,
	"David S. Miller" <davem@davemloft.net>,
	Florian Fainelli <f.fainelli@gmail.com>,
	Vivien Didelot <vivien.didelot@gmail.com>,
	Andrew Lunn <andrew@lunn.ch>
Subject: Re: [PATCH v3 net-next 12/14] dt-bindings: net: dsa: ocelot: add ocelot-ext documentation
Date: Sun, 9 Oct 2022 12:14:22 -0400	[thread overview]
Message-ID: <c9ce1d83-d1ca-4640-bba2-724e18e6e56b@linaro.org> (raw)
In-Reply-To: <20221008000014.vs2m3vei5la2r2nd@skbuf>

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