Re: [PATCH v4 08/15] dt-bindings: dma: ti: Add document for K3 UDMA

[Peter Ujfalusi]

Rob,

On 14/11/2019 19.53, Rob Herring wrote:
> On Tue, Nov 5, 2019 at 4:07 AM Peter Ujfalusi <peter.ujfalusi@xxxxxx> wrote:
>>
>>
>>
>> On 05/11/2019 4.19, Rob Herring wrote:
>>> On Fri, Nov 01, 2019 at 10:41:28AM +0200, Peter Ujfalusi wrote:
>>>> New binding document for
>>>> Texas Instruments K3 NAVSS Unified DMA â Peripheral Root Complex (UDMA-P).
>>>>
>>>> UDMA-P is introduced as part of the K3 architecture and can be found in
>>>> AM654 and j721e.
>>>>
>>>> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@xxxxxx>
>>>> ---
>>>> Rob,
>>>>
>>>> can you give me some hint on how to fix these two warnings from dt_binding_check:
>>>>
>>>>   DTC     Documentation/devicetree/bindings/dma/ti/k3-udma.example.dt.yaml
>>>> Documentation/devicetree/bindings/dma/ti/k3-udma.example.dts:23.13-72: Warning (ranges_format): /example-0/interconnect@30800000:ranges: "ranges" property has invalid length (24 bytes) (parent #address-cells == 1, child #address-cells == 2, #size-cells == 2)
>>>>   CHECK   Documentation/devicetree/bindings/dma/ti/k3-udma.example.dt.yaml
>>>
>>> The default #address-cells is 1 for examples. So you need to
>>> either override it or change ranges parent address size.
>>
>> wrapping the cbass_main_navss inside:
>> cbass_main {
>>     #address-cells = <2>;
>>     #size-cells = <2>;
>>     ...
>> };
>>
>> fixes it.
>>
>>>>
>>>> Documentation/devicetree/bindings/dma/ti/k3-udma.example.dt.yaml: interconnect@30800000: $nodename:0: 'interconnect@30800000' does not match '^(bus|soc|axi|ahb|apb)(@[0-9a-f]+)?$'
>>>
>>> Use 'bus' for the node name of 'simple-bus'.
>>
>> I took the navss node from the upstream dts (I'm going to fix it there
>> as well).
>> It has simple-bus for the navss, which is not quite right as NAVSS is
>> not a bus, but a big subsystem with multiple components (UDMAP, ringacc,
>> INTA, INTR, timers, etc).
>>
>> What about to change the binding doc to simple-mfd like this
> 
> That's really for things not memory-mapped (I'm sure you can probably
> find an example to contradict me), so better to keep simple-bus if all
> the child nodes have addresses.

According to Documentation/devicetree/bindings/mfd/mfd.txt:
- A range of memory registers containing "miscellaneous system
  registers" also known as a system controller "syscon" or any other
  memory range containing a mix of unrelated hardware devices.

NAVSS (NAVigator SubSystem) falls in the later case, it contains
unrelated blocks, like the UDMAP, ringacc, mailboxes, spinlocks,
interrupt aggregator, interrupt router, etc.

- compatible : "simple-mfd" - this signifies that the operating system
  should consider all subnodes of the MFD device as separate devices
  akin to how "simple-bus" indicates when to see subnodes as children
  for a simple memory-mapped bus.

This is a bit confusing, but NAVSS is not really a bus, everything in it
can be accessed by the CPU via memory mapped registers (some sub devices
does not have registers defined, they are controlled via system firmware).

> Do you need the node name to be 'navss' for some reason? If so, then
> better have a compatible string in there to identify it. If not, just
> use 'bus' and be done with it.

We don't need unique compatible for the NAVSS itself as there is not
much we can configure on the top level, it is 'just' a big subsystem
with all sorts of things.

I like to keep the 'navss' as node name as it gives human understandable
representation of it in /sys for example, easier to see the topology.

I just feel that the 'bus' does not really apply to what NAVSS is.
Probably my view of simple-bus is not correct.

>> cbass_main_navss: navss@30800000 {
>>     compatible = "simple-mfd";
>>     #address-cells = <2>;
>>     #size-cells = <2>;
>>     ...
>> };
>>
>> and fix up the DT when I got to the point when I can send the patches to
>> enable DMA for am654 and j721e?
> 
> There's no requirement yet for DTS files to not have warnings.

Sure, but it does not hurt if they are clean ;)

>>>> +  compatible:
>>>> +    oneOf:
>>>> +      - const: ti,am654-navss-main-udmap
>>>> +      - const: ti,am654-navss-mcu-udmap
>>>> +      - const: ti,j721e-navss-main-udmap
>>>> +      - const: ti,j721e-navss-mcu-udmap
>>>
>>> enum works better than oneOf+const. Better error messages.
>>
>> Like this:
>>   compatible:
>>     oneOf:
>>       - description: for AM654
>>         items:
>>           - enum:
>>               - ti,am654-navss-main-udmap
>>               - ti,am654-navss-mcu-udmap
>>
>>       - description: for J721E
>>         items:
>>           - enum:
>>               - ti,j721e-navss-main-udmap
>>               - ti,j721e-navss-mcu-udmap
> 
> If the 'description' was useful, but it's not. Just:
> 
> compatible:
>   enum:
>     - ti,am654-navss-main-udmap
>     - ti,am654-navss-mcu-udmap
>     - ti,j721e-navss-main-udmap
>     - ti,j721e-navss-mcu-udmap

OK, can I keep your Reviewed-by you have given to v5 if I do this change
for v6?

> 
> 
> Rob
> 

- PÃter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki