| Message ID | 20190614081650.11880-3-daniel.baluta@nxp.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Add support for DSP IPC protocol driver | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/dt-meta-schema | fail | build log |
On Fri, Jun 14, 2019 at 2:15 AM <daniel.baluta@nxp.com> wrote: > > From: Daniel Baluta <daniel.baluta@nxp.com> > > DSP IPC is the layer that allows the Host CPU to communicate > with DSP firmware. > DSP is part of some i.MX8 boards (e.g i.MX8QM, i.MX8QXP) > > Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com> > --- > .../bindings/arm/freescale/fsl,dsp.yaml | 43 +++++++++++++++++++ bindings/dsp/... > 1 file changed, 43 insertions(+) > create mode 100644 Documentation/devicetree/bindings/arm/freescale/fsl,dsp.yaml > > diff --git a/Documentation/devicetree/bindings/arm/freescale/fsl,dsp.yaml b/Documentation/devicetree/bindings/arm/freescale/fsl,dsp.yaml > new file mode 100644 > index 000000000000..16d9df1d397b > --- /dev/null > +++ b/Documentation/devicetree/bindings/arm/freescale/fsl,dsp.yaml > @@ -0,0 +1,43 @@ > +# SPDX-License-Identifier: GPL-2.0 The preference is to dual license new bindings: GPL-2.0 OR BSD-2-Clause > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/arm/freescale/fsl,dsp.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: NXP i.MX IPC DSP driver This isn't a driver. > + > +maintainers: > + - Daniel Baluta <daniel.baluta@nxp.com> > + > +description: | > + IPC communication layer between Host CPU and DSP on NXP i.MX8 platforms > + > +properties: > + compatible: > + enum: > + - fsl,imx-dsp You can have a fallback, but it needs SoC specific compatible(s). > + > + 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? > + > + mbox-names > + description: > + Mailboxes names > + allOf: > + - $ref: "/schemas/types.yaml#/definitions/string" No need for this, '*-names' already has a defined type. > + - enum: [ "txdb0", "txdb1", "rxdb0", "rxdb1" ] Should be an 'items' list with 4 entries? > +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? > + > +examples: > + - | > + dsp { > + compatbile = "fsl,imx-dsp"; > + mbox-names = "txdb0", "txdb1", "rxdb0", "rxdb1"; > + mboxes = <&lsio_mu13 2 0 &lsio_mu13 2 1 &lsio_mu13 3 0 &lsio_mu13 3 1>; mboxes = <&lsio_mu13 2 0>, <&lsio_mu13 2 1>, <&lsio_mu13 3 0>, <&lsio_mu13 3 1>; > + }; > -- > 2.17.1 >
Hi Rob, This is my first time documenting the bindings using the new yaml format so thanks for your patience and explanations! On Fri, Jun 14, 2019 at 5:53 PM Rob Herring <robh+dt@kernel.org> wrote: > > On Fri, Jun 14, 2019 at 2:15 AM <daniel.baluta@nxp.com> wrote: > > > > From: Daniel Baluta <daniel.baluta@nxp.com> > > > > DSP IPC is the layer that allows the Host CPU to communicate > > with DSP firmware. > > DSP is part of some i.MX8 boards (e.g i.MX8QM, i.MX8QXP) > > > > Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com> > > --- > > .../bindings/arm/freescale/fsl,dsp.yaml | 43 +++++++++++++++++++ > > bindings/dsp/... Fair enough. Will fix in v2. > > > 1 file changed, 43 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/arm/freescale/fsl,dsp.yaml > > > > diff --git a/Documentation/devicetree/bindings/arm/freescale/fsl,dsp.yaml b/Documentation/devicetree/bindings/arm/freescale/fsl,dsp.yaml > > new file mode 100644 > > index 000000000000..16d9df1d397b > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/arm/freescale/fsl,dsp.yaml > > @@ -0,0 +1,43 @@ > > +# SPDX-License-Identifier: GPL-2.0 > > The preference is to dual license new bindings: GPL-2.0 OR BSD-2-Clause > > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/arm/freescale/fsl,dsp.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: NXP i.MX IPC DSP driver > > This isn't a driver. I see. This node is actually the representation of DSP IPC so not a driver. > > > + > > +maintainers: > > + - Daniel Baluta <daniel.baluta@nxp.com> > > + > > +description: | > > + IPC communication layer between Host CPU and DSP on NXP i.MX8 platforms > > + > > +properties: > > + compatible: > > + enum: > > + - fsl,imx-dsp > > You can have a fallback, but it needs SoC specific compatible(s). Agree. Will fix in v2. > > > + > > + 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 > > > + > > + mbox-names > > + 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 ? > > > + - 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. > > > +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. To avoid confusion I have renamed this node from dsp to dsp_ipc. > > > + > > +examples: > > + - | > > + dsp { > > + compatbile = "fsl,imx-dsp"; > > + mbox-names = "txdb0", "txdb1", "rxdb0", "rxdb1"; > > + mboxes = <&lsio_mu13 2 0 &lsio_mu13 2 1 &lsio_mu13 3 0 &lsio_mu13 3 1>; > > mboxes = <&lsio_mu13 2 0>, <&lsio_mu13 2 1>, <&lsio_mu13 3 0>, <&lsio_mu13 3 1>; Actually no! It looks like the imx mailbox expects one element with a list of phandles directions and index. See again, how SCU uses the mailbox node. https://github.com/torvalds/linux/blob/master/arch/arm64/boot/dts/freescale/imx8qxp.dtsi#L123 > > > + }; > > -- > > 2.17.1 > >
On Wed, Jun 26, 2019 at 8:49 AM Daniel Baluta <daniel.baluta@gmail.com> wrote: > > Hi Rob, > > This is my first time documenting the bindings using the > new yaml format so thanks for your patience and explanations! > > On Fri, Jun 14, 2019 at 5:53 PM Rob Herring <robh+dt@kernel.org> wrote: > > > > On Fri, Jun 14, 2019 at 2:15 AM <daniel.baluta@nxp.com> wrote: > > > > > > From: Daniel Baluta <daniel.baluta@nxp.com> > > > > > > DSP IPC is the layer that allows the Host CPU to communicate > > > with DSP firmware. > > > DSP is part of some i.MX8 boards (e.g i.MX8QM, i.MX8QXP) > > > > > > Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com> > > > --- > > > .../bindings/arm/freescale/fsl,dsp.yaml | 43 +++++++++++++++++++ > > > > bindings/dsp/... > > Fair enough. Will fix in v2. > > > > > > 1 file changed, 43 insertions(+) > > > create mode 100644 Documentation/devicetree/bindings/arm/freescale/fsl,dsp.yaml > > > > > > diff --git a/Documentation/devicetree/bindings/arm/freescale/fsl,dsp.yaml b/Documentation/devicetree/bindings/arm/freescale/fsl,dsp.yaml > > > new file mode 100644 > > > index 000000000000..16d9df1d397b > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/arm/freescale/fsl,dsp.yaml > > > @@ -0,0 +1,43 @@ > > > +# SPDX-License-Identifier: GPL-2.0 > > > > The preference is to dual license new bindings: GPL-2.0 OR BSD-2-Clause > > > > > +%YAML 1.2 > > > +--- > > > +$id: http://devicetree.org/schemas/arm/freescale/fsl,dsp.yaml# > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > + > > > +title: NXP i.MX IPC DSP driver > > > > This isn't a driver. > > I see. This node is actually the representation of DSP IPC so not a driver. > > > > > + > > > +maintainers: > > > + - Daniel Baluta <daniel.baluta@nxp.com> > > > + > > > +description: | > > > + IPC communication layer between Host CPU and DSP on NXP i.MX8 platforms > > > + > > > +properties: > > > + compatible: > > > + enum: > > > + - fsl,imx-dsp > > > > You can have a fallback, but it needs SoC specific compatible(s). > Agree. Will fix in v2. > > > > > > + > > > + 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... > > > + > > > + 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. > > > + 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. > > > +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. > To avoid confusion I have renamed this node from dsp to dsp_ipc. > > > > > > + > > > +examples: > > > + - | > > > + dsp { > > > + compatbile = "fsl,imx-dsp"; > > > + mbox-names = "txdb0", "txdb1", "rxdb0", "rxdb1"; > > > + mboxes = <&lsio_mu13 2 0 &lsio_mu13 2 1 &lsio_mu13 3 0 &lsio_mu13 3 1>; > > > > mboxes = <&lsio_mu13 2 0>, <&lsio_mu13 2 1>, <&lsio_mu13 3 0>, <&lsio_mu13 3 1>; > > Actually no! It looks like the imx mailbox expects one element with a > list of phandles directions and index. There's not actually any difference in what the OS sees. Both source syntaxes result in the same data encoding in the dtb. It's simply 12 words of data. What's a phandle is only known because the OS knows what 'mboxes' contains and by reading #mbox-cells in lsio_mu13. However, we are using this source grouping to maintain type information to do schema validation. The grouping is kept thru to the yaml encoding (of the DT, not to be confused with the schemas). So we're going to have to start being stricter in dts files. Rob
<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.
On Thu, Jun 27, 2019 at 1:40 AM Daniel Baluta <daniel.baluta@gmail.com> wrote: > > <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? Yes, that's the point. Enforcing that dts files contain what the binding docs say. > > 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. 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 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. Rob
On Thu, Jun 27, 2019 at 6:59 PM Rob Herring <robh+dt@kernel.org> wrote: > > On Thu, Jun 27, 2019 at 1:40 AM Daniel Baluta <daniel.baluta@gmail.com> wrote: > > > > <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? > > Yes, that's the point. Enforcing that dts files contain what the > binding docs say. > > > > > 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. > > 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. > > > > > 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.
diff --git a/Documentation/devicetree/bindings/arm/freescale/fsl,dsp.yaml b/Documentation/devicetree/bindings/arm/freescale/fsl,dsp.yaml new file mode 100644 index 000000000000..16d9df1d397b --- /dev/null +++ b/Documentation/devicetree/bindings/arm/freescale/fsl,dsp.yaml @@ -0,0 +1,43 @@ +# SPDX-License-Identifier: GPL-2.0 +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/arm/freescale/fsl,dsp.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: NXP i.MX IPC DSP driver + +maintainers: + - Daniel Baluta <daniel.baluta@nxp.com> + +description: | + IPC communication layer between Host CPU and DSP on NXP i.MX8 platforms + +properties: + compatible: + enum: + - fsl,imx-dsp + + mboxes: + description: + List of phandle of 2 MU channels for TXDB, 2 MU channels for RXDB + (see mailbox/fsl,mu.txt) + maxItems: 1 + + mbox-names + description: + Mailboxes names + allOf: + - $ref: "/schemas/types.yaml#/definitions/string" + - enum: [ "txdb0", "txdb1", "rxdb0", "rxdb1" ] +required: + - compatible + - mboxes + - mbox-names + +examples: + - | + dsp { + compatbile = "fsl,imx-dsp"; + mbox-names = "txdb0", "txdb1", "rxdb0", "rxdb1"; + mboxes = <&lsio_mu13 2 0 &lsio_mu13 2 1 &lsio_mu13 3 0 &lsio_mu13 3 1>; + };