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

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

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