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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAEnQRZCjp9dUt0JTjhN0CnV0+Xzc+q1EHCnJn_TNOQoUWZBTsg@mail.gmail.com>
Date:   Thu, 27 Jun 2019 10:40:19 +0300
From:   Daniel Baluta <daniel.baluta@...il.com>
To:     Rob Herring <robh+dt@...nel.org>
Cc:     Daniel Baluta <daniel.baluta@....com>,
        Shawn Guo <shawnguo@...nel.org>,
        Sascha Hauer <s.hauer@...gutronix.de>,
        "S.j. Wang" <shengjiu.wang@....com>,
        Fabio Estevam <festevam@...il.com>,
        NXP Linux Team <linux-imx@....com>,
        Dong Aisheng <aisheng.dong@....com>,
        Anson Huang <anson.huang@....com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE" 
        <linux-arm-kernel@...ts.infradead.org>,
        Mark Rutland <mark.rutland@....com>,
        Devicetree List <devicetree@...r.kernel.org>,
        Oleksij Rempel <o.rempel@...gutronix.de>
Subject: Re: [PATCH 2/2] dt-bindings: arm: fsl: Add DSP IPC binding support

<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@...e8000 {
»       »       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@...e8000 {
»       »       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.