Re: [PATCH v2 01/14] dt-bindings: remoteproc: Add TI PRUSS bindings

Murali Karicheri <m-karicheri2@xxxxxx> · Tue, 5 Feb 2019 11:15:13 -0500

Roger,

On 02/05/2019 10:41 AM, Roger Quadros wrote:
Murali,

On 05/02/19 17:08, Murali Karicheri wrote:
Hi Roger,

On 02/05/2019 04:39 AM, Roger Quadros wrote:
Hi Tony & Suman,

On 04/02/19 18:33, Tony Lindgren wrote:
Hi,

* Roger Quadros <rogerq@xxxxxx> [190204 14:23]:
From: Suman Anna <s-anna@xxxxxx>
...
+Example:
+========
+1.    /* AM33xx PRU-ICSS */
+
+    pruss: pruss@0 {
+        compatible = "ti,am3356-pruss";
+        reg = <0x0 0x2000>,
+              <0x2000 0x2000>,
+              <0x10000 0x3000>;
+        reg-names = "dram0", "dram1",
+                "shrdram2";
+        #address-cells = <1>;
+        #size-cells = <1>;
+        ranges;

Thanks for fixing up the reg ranges for the top level node.

Ideally there would not even be a top level node here as
AFAIK the whole PRUSS is a collection of devices on a PRU
internal interconnect. So following that path a bit further..
How about just get rid of the top level node and just do:

pruss: pruss@0 {
     dram0: memory@0 {
            device_type = "memory";
            reg = <0x0 0x2000>;
     };

     dram1: memory@2000 {
            device_type = "memory";
            reg = <0x2000 0x2000>;
     };

Actually dram0 and dram1 are data memories for PRU0 and PRU1 respectively.
Isn't it better if they are moved to the pru node?
e.g.

     pru0: pru@34000 {
         compatible = "ti,am3356-pru";
         reg = <0x34000 0x2000>,
               <0x22000 0x400>,
               <0x22400 0x100>,
               <0x0     0x2000>;
         reg-names = "iram", "control", "debug", "dram";
         ...
     };

     pru1: pru@38000 {
         compatible = "ti,am3356-pru";
         reg = <0x38000 0x2000>,
               <0x24000 0x400>,
               <0x24400 0x100>,
               <0x2000  0x2000>;
         reg-names = "iram", "control", "debug", "dram";
         ...
     };

I think it is better to place a restriction that firmware on PRU0 cannot use data
memory of PRU1 and vice versa.

That will not work as there are switch firmware cases where PRU access
DRAM of other PRU and is a valid case to support in the future. So let
us not do that.

PRU firmware accessing DRAM of other PRU is a design contract and that use case
requires both PRUs to be loaded with matching firmware. That should continue to work.

What I'm suggesting here is that kernel remoteproc driver should have nothing to do
with the other PRU's data RAM.

The application driver if needs both PRUs then it can obviously access both DRAMs
as it has a phandle to both PRUs.

That should be fine.

Regards,
Murali

cheers,
-roger

Murali
Application drivers do sometimes need to read/write to data memory. The pru_rproc
driver could provide a API for the application drivers to get virtual address of
the respective PRU's data memory.

     shrdram2: memory@10000 {
         device_type = "memory";
         reg = <0x10000 0x3000>;
     };

Shared RAM is not so straight forward. Both PRU firmwares and both application drivers
might need to read/write here. The area split is decided by firmware design and there
is no hardware protection to prevent from stomping on each others toes.

We need a carveout based memory allocator at least I think that can do a
allocate(base_offset, size); into shared RAM.

This could be used by pru_rproc driver at firmware load time and by application drivers
at initialization time.

Thoughts?

     pruss_cfg: cfg@26000 {
         ...
     };
     ...
};

If the device_type = "memory" cannot be used here for
being specific to the top level properties, then
there's probably some other generic property usable
here :)

+        pruss_mii_rt: mii_rt@32000 {
+            reg = <0x32000 0x58>;
+        };

The node name should not have underscores so
pruss_mii_rt: mii-rt@32000. Please check the others
too, like app_node.

OK.

+    app_node: app_node {
+        prus = <&pru0>, <&pru1>;
+        firmware-name = "pruss-app-fw", "pruss-app-fw-2";
+        ti,pruss-gp-mux-sel = <2>, <1>;
+        /* setup interrupts for prus:
+           prus[0] => pru1_0: ev=16, chnl=2, host-irq=7,
+           prus[1] => pru1_1: ev=19, chnl=1, host-irq=3 */
+        ti,pru-interrupt-map = <0 16 2 7 >, <1 19 1 3>;
+    }

If the ti,pruss-gp-mux-sel and ti,pru-interrupt-map are
firmware configuration options, maybe leave them out of
the dts completely and make the app-node optional.

Yes the app-node is optional. I will mention it.

No, ti,pruss-gp-mux-sel and ti,pru-interrupt-map are not firmware options.
But these settings are application/firmware specific.

ti,pru-interrupt-map specifies the configuration to be used for the INTC interrupt
controller.

ti,pruss-gp-mux-sel is used to configure this register.
"Table 30-20. PRUSS_GPCFG0" in http://www.tij.co.jp/jp/lit/ug/spruhz7h/spruhz7h.pdf
"29:26 PR1_PRU0_GP_MUX_SEL"

It configures how the pins from the PRUSS module are routed internally
to the various modules.

see "30.2.1 PRU-ICSS I/O Interface"
and "Table 30-1. PRU-ICSS1 I/O Signals"

And have a proper compatible for this node such as
"ti,pruss-app-xyz". And this should be only set if the the
hardware is wired up in such way that things need to be
configured in the dts rather than by the firmware.

Yes, compatible is a required property as we need to load
the appropriate application (kernel space) driver for it.
I will fix the example.

And then you can just hide mux-sel and interrupt-map
behind the compatible property for that hardware. And
leave them out from the dts and have the handling driver
would set mux-sel and interrupt-map based on the
match->data during probe.

To summarize:

I'll mark the app node as optional. Only required if a kernel
driver is required for the application.
Compatible is mandatory for app node.
ti,pruss-gp-mux-sel and ti,pru-interrupt-map are optional
for app node.

cheers,
-roger