Rob,

On 14/11/2019 19.53, Rob Herring wrote:

On Tue, Nov 5, 2019 at 4:07 AM Peter Ujfalusi [off-list ref] wrote:

quoted

On 05/11/2019 4.19, Rob Herring wrote:

quoted

On Fri, Nov 01, 2019 at 10:41:28AM +0200, Peter Ujfalusi wrote:

quoted

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 <redacted>
---
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.

quoted

quoted

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.

quoted

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 ;)

quoted

quoted

quoted

+  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