Re: [PATCH 2/2] dt-bindings: arm: fsl: Add DSP IPC binding support

[Daniel Baluta]

<snip>

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

>
> > > > +
> > > > +  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?

Used make dt_binding_check everything looks OK now.

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

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

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.

Anyhow, will try to address your further feedback, will send v2 now to
have more feedback from
Oleksij.

thanks,
Daniel.