On Thu, Jun 27, 2019 at 6:59 PM Rob Herring [off-list ref] wrote:

On Thu, Jun 27, 2019 at 1:40 AM Daniel Baluta [off-list ref] wrote:

quoted

<snip>

quoted

quoted

quoted

quoted

+  mboxes:
+    description:
+      List of phandle of 2 MU channels for TXDB, 2 MU channels for RXDB
+      (see mailbox/fsl,mu.txt)
+    maxItems: 1

Should be 4?

Actually is just a list with 1 item. I think is the terminology:

You can have an example here of the mboxes defined for SCU.
https://github.com/torvalds/linux/blob/master/arch/arm64/boot/dts/freescale/imx8qxp.dtsi#L123

mboxes = <&lsio_mu1 0 0
&lsio_mu1 0 1
&lsio_mu1 0 2
&lsio_mu1 0 3
&lsio_mu1 1 0
&lsio_mu1 1 1
&lsio_mu1 1 2
&lsio_mu1 1 3
&lsio_mu1 3 3>;

Logically, this is 9 entries and each entry is 3 cells ( or phandle
plus 2 cells). More below...

Ok..

quoted

quoted

quoted

quoted

+
+  mbox-names

Also, missing a ':' here. This won't build. Make sure you build this
(make dt_binding_check). See
Documentation/devicetree/writing-schemas.md.

Fixed in v2. Awesome!

I thought that Documentation/devicetree/bindings/dsp/fsl,dsp_ipc.yaml
is purely decorative and used as an example. But it's actually the schema for
the newly yaml dts, right?

Yes, that's the point. Enforcing that dts files contain what the
binding docs say.

quoted

Used make dt_binding_check everything looks OK now.

quoted

quoted

quoted

quoted

+    description:
+      Mailboxes names
+    allOf:
+      - $ref: "/schemas/types.yaml#/definitions/string"

No need for this, '*-names' already has a defined type.

So, should I remove the above two lines ?

Actually, all 4. There's no need to describe what 'mbox-names' is.

quoted

quoted

quoted

+      - enum: [ "txdb0", "txdb1", "rxdb0", "rxdb1" ]

Should be an 'items' list with 4 entries?

Let me better read the yaml spec. But "items" list indeed sounds better.

What you should end up with is:

items:
  - const: txdb0
  - const: txdb1
  - const: rxdb0
  - const: rxdb1

This is saying you have 4 strings in the listed order. The enum you
had would be a single string of one of the 4 values.

I see! Thanks.

quoted

quoted

quoted

quoted

+required:
+  - compatible
+  - mboxes
+  - mbox-names

This seems incomplete. How does one boot the DSP? Load firmware? No
resources that Linux has to manage. Shared memory?

This is only the IPC mailboxes used by DSP to communicate with Linux. The
loading of the firmware, the resources needed to be managed by Linux, etc
are part of the DSP node.

You should just add the mailboxes to the DSP node then. I suppose you
didn't because you want 2 drivers? If so, that's the OS's problem and
not part of DT. A Linux driver can instantiate devices for other
drivers.

Yes, I want the DSP IPC driver to be separated. And then the SOF Linux
driver that needs
to communicate with DSP just gets a handle to DSP IPC driver and does
the communication.

dts relevant nodes look like this now:

»       dsp_ipc: dsp_ipc {
»       »       compatible = "fsl,imx8qxp-dsp";
»       »       mbox-names = "txdb0", "txdb1",
»       »       »            "rxdb0", "rxdb1";
»       »       mboxes = <&lsio_mu13 2 0>,
»       »       »        <&lsio_mu13 2 1>,
»       »       »        <&lsio_mu13 3 0>,
»       »       »        <&lsio_mu13 3 1>;
»       };

»       adma_dsp: dsp@596e8000 {
»       »       compatible = "fsl,imx8qxp-sof-dsp";
»       »       reg = <0x596e8000 0x88000>;
»       »       reserved-region = <&dsp_reserved>;
»       »       ipc = <&dsp_ipc>;
»       };

Your suggeston would be to have something like this:

»       adma_dsp: dsp@596e8000 {
»       »       compatible = "fsl,imx8qxp-sof-dsp";
»       »       reg = <0x596e8000 0x88000>;
»       »       reserved-region = <&dsp_reserved>;
»                mbox-names = "txdb0", "txdb1",
»       »       »            "rxdb0", "rxdb1";
»       »       mboxes = <&lsio_mu13 2 0>,
»       »       »        <&lsio_mu13 2 1>,
»       »       »        <&lsio_mu13 3 0>,
»       »       »        <&lsio_mu13 3 1>;
»       };

Not sure exactly how to instantiate IPC DSP driver then.

DT is not the only way to instantiate drivers. A driver can create a
platform device itself which will then instantiate a 2nd driver.

Presumably the DSP needs to be booted, resources enabled, and firmware
loaded before IPC will work. The DSP driver controlling the lifetime
of the IPC driver is the right way to manage the dependencies.

I see your point. This way I will resolve the dependency problem. So far
SOF driver was probed before IPC driver and I needed to return -EPROBE_DEFFER.

The "sad" part is that SOF driver also needs in the same way the
System Controller
Firmware driver to be probed.

But the SC driver is already accepted with an interface that looks
like my old approach.

https://elixir.bootlin.com/linux/v5.2-rc6/source/drivers/firmware/imx/imx-scu.c#L93

Oh, well.

quoted

I already have prepared v2 with most of your feedback incorporated,
but not this latest
change with moving mboxes inside dsp driver.

More than that I have followed the model of SCFW IPC and having to
different approach
for similar IPC mechanism is a little bit confusing.

SC is system controller? Maybe I missed it, but I don't think system
controllers usually have 2 nodes. You only have the communications
interface exposed as the SC provides services to Linux and Linux
doesn't manage the SC resources.

Yes, SC is the system controller.

https://elixir.bootlin.com/linux/v5.2-rc6/source/drivers/firmware/imx/imx-scu.c

I see your point of only have 1 node and I will implement it like that.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel