[PATCH 0/2] Add support for DSP IPC protocol driver

[PATCH 0/2] Add support for DSP IPC protocol driver

[daniel.baluta]

From: Daniel Baluta <daniel.baluta@nxp.com>

Hifi4 DSP can be found on some i.MX8 platforms (e.g i.MX8QXP, i.MX8QM).
This patch series introduces the layer that allows Host CPU to
communicate with DSP.

This layer provides a doorbell and clients can used that to notify DSP
that a message is placed somewhere in the shared memory.

The protocol used is request - reply. Usually, Host/DSP write a message
in a shared memory area and notify the other side. The other side will
also write a reply in a designated shared memory area and then ring
the doorbell to let the counterpart that a message is ready.

Daniel Baluta (2):
  firmware: imx: Add DSP IPC protocol driver
  dt-bindings: arm: fsl: Add DSP IPC binding support

 .../bindings/arm/freescale/fsl,dsp.yaml       |  43 +++++
 drivers/firmware/imx/Kconfig                  |  11 ++
 drivers/firmware/imx/Makefile                 |   1 +
 drivers/firmware/imx/imx-dsp.c                | 167 ++++++++++++++++++
 include/linux/firmware/imx/dsp.h              |  61 +++++++
 5 files changed, 283 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/arm/freescale/fsl,dsp.yaml
 create mode 100644 drivers/firmware/imx/imx-dsp.c
 create mode 100644 include/linux/firmware/imx/dsp.h

-- 
2.17.1

[PATCH 1/2] firmware: imx: Add DSP IPC protocol driver

[daniel.baluta]

From: Daniel Baluta <daniel.baluta@nxp.com>

Some of i.MX8 processors (e.g i.MX8QM, i.MX8QXP) contain
the Tensilica HiFi4 DSP for advanced pre- and post-audio
processing.

The communication between Host CPU and DSP firmware is
taking place using a shared memory area for message passing
and a dedicated Messaging Unit for notifications.

DSP IPC protocol driver offers a doorbell interface using
imx-mailbox API.

We use 4 MU channels (2 x TXDB, 2 x RXDB) to implement a
request-reply protocol.

Connection 0 (txdb0, rxdb0):
        - Host writes messasge to shared memory [SHMEM]
	- Host sends a request [MU]
	- DSP handles request [SHMEM]
	- DSP sends reply [MU]

Connection 1 (txdb1, rxdb1):
	- DSP writes a message to shared memory [SHMEM]
	- DSP sends a request [MU]
	- Host handles request [SHMEM]
	- Host sends reply [MU]

The protocol driver will be used by a Host client to
communicate with the DSP. First client will be the i.MX8
part from Sound Open Firmware infrastructure.

The protocol drivers offers the following interface:

On Tx:
   - imx_dsp_ring_doorbell, will be called to notify the DSP
   that it needs to handle a request.

On Rx:
   - clients need to provide two callbacks:
	.handle_reply
	.handle_request
  - the callbacks will be used by the protocol driver on
    notification arrival from DSP.

Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
---
 drivers/firmware/imx/Kconfig     |  11 ++
 drivers/firmware/imx/Makefile    |   1 +
 drivers/firmware/imx/imx-dsp.c   | 167 +++++++++++++++++++++++++++++++
 include/linux/firmware/imx/dsp.h |  61 +++++++++++
 4 files changed, 240 insertions(+)
 create mode 100644 drivers/firmware/imx/imx-dsp.c
 create mode 100644 include/linux/firmware/imx/dsp.h

diff --git a/drivers/firmware/imx/Kconfig b/drivers/firmware/imx/Kconfig
index 42b566f8903f..383996b679a8 100644
--- a/drivers/firmware/imx/Kconfig
+++ b/drivers/firmware/imx/Kconfig
@@ -1,4 +1,15 @@
 # SPDX-License-Identifier: GPL-2.0-only
+config IMX_DSP
+	bool "IMX DSP Protocol driver"
+	depends on IMX_MBOX
+	help
+	  This enables DSP IPC protocol between host CPU (Linux)
+	  and the firmware running on DSP.
+	  DSP exists on some i.MX8 processors (e.g i.MX8QM, i.MX8QXP).
+
+          It acts like a doorbell. Client might use shared memory to
+	  exchange information with DSP side.
+
 config IMX_SCU
 	bool "IMX SCU Protocol driver"
 	depends on IMX_MBOX
diff --git a/drivers/firmware/imx/Makefile b/drivers/firmware/imx/Makefile
index 802c4ad8e8f9..08bc9ddfbdfb 100644
--- a/drivers/firmware/imx/Makefile
+++ b/drivers/firmware/imx/Makefile
@@ -1,3 +1,4 @@
 # SPDX-License-Identifier: GPL-2.0
+obj-$(CONFIG_IMX_DSP)		+= imx-dsp.o
 obj-$(CONFIG_IMX_SCU)		+= imx-scu.o misc.o imx-scu-irq.o
 obj-$(CONFIG_IMX_SCU_PD)	+= scu-pd.o
diff --git a/drivers/firmware/imx/imx-dsp.c b/drivers/firmware/imx/imx-dsp.c
new file mode 100644
index 000000000000..953fd364ad76
--- /dev/null
+++ b/drivers/firmware/imx/imx-dsp.c
@@ -0,0 +1,167 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright 2018 NXP
+ *  Author: Daniel Baluta <daniel.baluta@nxp.com>
+ *
+ * Implementation of the DSP IPC interface (host side)
+ */
+
+#include <linux/firmware/imx/dsp.h>
+#include <linux/kernel.h>
+#include <linux/mailbox_client.h>
+#include <linux/module.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+static struct imx_dsp_ipc *imx_dsp_handle;
+
+/*
+ * Get the default handle used by DSP
+ */
+int imx_dsp_get_handle(struct imx_dsp_ipc **ipc)
+{
+	if (!imx_dsp_handle)
+		return -EPROBE_DEFER;
+
+	*ipc = imx_dsp_handle;
+	return 0;
+}
+EXPORT_SYMBOL(imx_dsp_get_handle);
+
+void imx_dsp_set_data(struct imx_dsp_ipc *ipc, void *data)
+{
+	if (!ipc)
+		return;
+
+	ipc->private_data = data;
+}
+EXPORT_SYMBOL(imx_dsp_set_data);
+
+void *imx_dsp_get_data(struct imx_dsp_ipc *ipc)
+{
+	if (!ipc)
+		return NULL;
+
+	return ipc->private_data;
+}
+EXPORT_SYMBOL(imx_dsp_get_data);
+
+/*
+ * imx_dsp_ring_doorbell - triggers an interrupt on the other side (DSP)
+ *
+ * @dsp: DSP IPC handle
+ * @chan_idx: index of the channel where to trigger the interrupt
+ *
+ * Returns non-negative value for success, negative value for error
+ */
+int imx_dsp_ring_doorbell(struct imx_dsp_ipc *ipc, unsigned int idx)
+{
+	int ret;
+	struct imx_dsp_chan *dsp_chan;
+
+	if (idx > DSP_MU_CHAN_NUM)
+		return -EINVAL;
+
+	dsp_chan = &ipc->chans[idx];
+	ret = mbox_send_message(dsp_chan->ch, NULL);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+EXPORT_SYMBOL(imx_dsp_ring_doorbell);
+
+/*
+ * imx_dsp_handle_rx - rx callback used by imx mailbox
+ *
+ * @c: mbox client
+ * @msg: message received
+ *
+ * Users of DSP IPC will need to privde handle_reply and handle_request
+ * callbacks.
+ */
+static void imx_dsp_handle_rx(struct mbox_client *c, void *msg)
+{
+	struct imx_dsp_chan *chan = container_of(c, struct imx_dsp_chan, cl);
+
+	if (chan->idx == 0) {
+		chan->ipc->ops->handle_reply(chan->ipc);
+	} else {
+		chan->ipc->ops->handle_request(chan->ipc);
+		imx_dsp_ring_doorbell(chan->ipc, 1);
+	}
+}
+
+static int imx_dsp_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct imx_dsp_ipc *dsp_ipc;
+	struct imx_dsp_chan *dsp_chan;
+	struct mbox_client *cl;
+	char *chan_name;
+	int ret;
+	int i;
+
+	dsp_ipc = devm_kzalloc(dev, sizeof(*dsp_ipc), GFP_KERNEL);
+	if (!dsp_ipc)
+		return -ENOMEM;
+
+	for (i = 0; i < DSP_MU_CHAN_NUM; i++) {
+		if (i < 2)
+			chan_name = kasprintf(GFP_KERNEL, "txdb%d", i);
+		else
+			chan_name = kasprintf(GFP_KERNEL, "rxdb%d", i - 2);
+
+		if (!chan_name)
+			return -ENOMEM;
+
+		dsp_chan = &dsp_ipc->chans[i];
+		cl = &dsp_chan->cl;
+		cl->dev = dev;
+		cl->tx_block = false;
+		cl->knows_txdone = true;
+		cl->rx_callback = imx_dsp_handle_rx;
+
+		dsp_chan->ipc = dsp_ipc;
+		dsp_chan->idx = i % 2;
+		dsp_chan->ch = mbox_request_channel_byname(cl, chan_name);
+		if (IS_ERR(dsp_chan->ch)) {
+			ret = PTR_ERR(dsp_chan->ch);
+			if (ret != -EPROBE_DEFER)
+				dev_err(dev, "Failed to request mbox chan %s ret %d\n",
+					chan_name, ret);
+			return ret;
+		}
+
+		dev_dbg(dev, "request mbox chan %s\n", chan_name);
+		/* chan_name is not used anymore by framework */
+		kfree(chan_name);
+	}
+
+	dsp_ipc->dev = dev;
+
+	imx_dsp_handle = dsp_ipc;
+
+	dev_info(dev, "NXP i.MX DSP IPC initialized\n");
+
+	return devm_of_platform_populate(dev);
+}
+
+static const struct of_device_id imx_dsp_match[] = {
+	{ .compatible = "fsl,imx-dsp", },
+	{ /* Sentinel */ }
+};
+
+static struct platform_driver imx_dsp_driver = {
+	.driver = {
+		.name = "imx-dsp",
+		.of_match_table = imx_dsp_match,
+	},
+	.probe = imx_dsp_probe,
+};
+builtin_platform_driver(imx_dsp_driver);
+
+MODULE_AUTHOR("Daniel Baluta <daniel.baluta@nxp.com>");
+MODULE_DESCRIPTION("IMX DSP IPC protocol driver");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/firmware/imx/dsp.h b/include/linux/firmware/imx/dsp.h
new file mode 100644
index 000000000000..75637d8fab34
--- /dev/null
+++ b/include/linux/firmware/imx/dsp.h
@@ -0,0 +1,61 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * Copyright 2018 NXP
+ *
+ * Header file for the DSP IPC implementation
+ */
+
+#ifndef _IMX_DSP_IPC_H
+#define _IMX_DSP_IPC_H
+
+#include <linux/device.h>
+#include <linux/types.h>
+#include <linux/mailbox_client.h>
+
+#define DSP_MU_CHAN_NUM		4
+
+struct imx_dsp_chan {
+	struct imx_dsp_ipc *ipc;
+	struct mbox_client cl;
+	struct mbox_chan *ch;
+	int idx;
+};
+
+struct imx_dsp_ops {
+	void (*handle_reply)(struct imx_dsp_ipc *ipc);
+	void (*handle_request)(struct imx_dsp_ipc *ipc);
+};
+
+struct imx_dsp_ipc {
+	/* Host <-> DSP communication uses 2 txdb and 2 rxdb channels */
+	struct imx_dsp_chan chans[DSP_MU_CHAN_NUM];
+	struct device *dev;
+	struct imx_dsp_ops *ops;
+	void *private_data;
+};
+
+#if IS_ENABLED(CONFIG_IMX_DSP)
+
+int imx_dsp_ring_doorbell(struct imx_dsp_ipc *dsp, unsigned int chan_idx);
+int imx_dsp_get_handle(struct imx_dsp_ipc **ipc);
+void imx_dsp_set_data(struct imx_dsp_ipc *ipc, void *data);
+void *imx_dsp_get_data(struct imx_dsp_ipc *ipc);
+
+#else
+
+static inline int imx_dsp_get_handle(struct imx_dsp_ipc **ipc)
+{
+	return -EIO;
+}
+
+static inline int imx_dsp_ring_doorbell(struct imx_dsp_ipc *ipc,
+					unsigned int chan_idx)
+{
+	return -EIO;
+}
+
+void imx_dsp_set_data(struct imx_dsp_ipc *ipc, void *data) { }
+void *imx_dsp_get_data(struct imx_dsp_ipc *ipc) { return NULL; }
+
+#endif
+#endif /* _IMX_DSP_IPC_H */
-- 
2.17.1

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

[daniel.baluta]

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 +++++++++++++++++++
 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
+%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>;
+    };
-- 
2.17.1

Re: [PATCH 1/2] firmware: imx: Add DSP IPC protocol driver

[Oleksij Rempel]

Hi Daniel,

please, see my review inline.

On Fri, Jun 14, 2019 at 04:16:49PM +0800, daniel.baluta@nxp.com wrote:
> From: Daniel Baluta <daniel.baluta@nxp.com>
> 
> Some of i.MX8 processors (e.g i.MX8QM, i.MX8QXP) contain
> the Tensilica HiFi4 DSP for advanced pre- and post-audio
> processing.
> 
> The communication between Host CPU and DSP firmware is
> taking place using a shared memory area for message passing
> and a dedicated Messaging Unit for notifications.
> 
> DSP IPC protocol driver offers a doorbell interface using
> imx-mailbox API.
> 
> We use 4 MU channels (2 x TXDB, 2 x RXDB) to implement a
> request-reply protocol.
> 
> Connection 0 (txdb0, rxdb0):
>         - Host writes messasge to shared memory [SHMEM]
> 	- Host sends a request [MU]
> 	- DSP handles request [SHMEM]
> 	- DSP sends reply [MU]
> 
> Connection 1 (txdb1, rxdb1):
> 	- DSP writes a message to shared memory [SHMEM]
> 	- DSP sends a request [MU]
> 	- Host handles request [SHMEM]
> 	- Host sends reply [MU]
> 
> The protocol driver will be used by a Host client to
> communicate with the DSP. First client will be the i.MX8
> part from Sound Open Firmware infrastructure.
> 
> The protocol drivers offers the following interface:
> 
> On Tx:
>    - imx_dsp_ring_doorbell, will be called to notify the DSP
>    that it needs to handle a request.
> 
> On Rx:
>    - clients need to provide two callbacks:
> 	.handle_reply
> 	.handle_request
>   - the callbacks will be used by the protocol driver on
>     notification arrival from DSP.
> 
> Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
> ---
>  drivers/firmware/imx/Kconfig     |  11 ++
>  drivers/firmware/imx/Makefile    |   1 +
>  drivers/firmware/imx/imx-dsp.c   | 167 +++++++++++++++++++++++++++++++
>  include/linux/firmware/imx/dsp.h |  61 +++++++++++
>  4 files changed, 240 insertions(+)
>  create mode 100644 drivers/firmware/imx/imx-dsp.c
>  create mode 100644 include/linux/firmware/imx/dsp.h
> 
> diff --git a/drivers/firmware/imx/Kconfig b/drivers/firmware/imx/Kconfig
> index 42b566f8903f..383996b679a8 100644
> --- a/drivers/firmware/imx/Kconfig
> +++ b/drivers/firmware/imx/Kconfig
> @@ -1,4 +1,15 @@
>  # SPDX-License-Identifier: GPL-2.0-only
> +config IMX_DSP
> +	bool "IMX DSP Protocol driver"
> +	depends on IMX_MBOX
> +	help
> +	  This enables DSP IPC protocol between host CPU (Linux)
> +	  and the firmware running on DSP.
> +	  DSP exists on some i.MX8 processors (e.g i.MX8QM, i.MX8QXP).
> +
> +          It acts like a doorbell. Client might use shared memory to
> +	  exchange information with DSP side.
> +
>  config IMX_SCU
>  	bool "IMX SCU Protocol driver"
>  	depends on IMX_MBOX
> diff --git a/drivers/firmware/imx/Makefile b/drivers/firmware/imx/Makefile
> index 802c4ad8e8f9..08bc9ddfbdfb 100644
> --- a/drivers/firmware/imx/Makefile
> +++ b/drivers/firmware/imx/Makefile
> @@ -1,3 +1,4 @@
>  # SPDX-License-Identifier: GPL-2.0
> +obj-$(CONFIG_IMX_DSP)		+= imx-dsp.o
>  obj-$(CONFIG_IMX_SCU)		+= imx-scu.o misc.o imx-scu-irq.o
>  obj-$(CONFIG_IMX_SCU_PD)	+= scu-pd.o
> diff --git a/drivers/firmware/imx/imx-dsp.c b/drivers/firmware/imx/imx-dsp.c
> new file mode 100644
> index 000000000000..953fd364ad76
> --- /dev/null
> +++ b/drivers/firmware/imx/imx-dsp.c
> @@ -0,0 +1,167 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright 2018 NXP
> + *  Author: Daniel Baluta <daniel.baluta@nxp.com>
> + *
> + * Implementation of the DSP IPC interface (host side)
> + */
> +
> +#include <linux/firmware/imx/dsp.h>
> +#include <linux/kernel.h>
> +#include <linux/mailbox_client.h>
> +#include <linux/module.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +
> +static struct imx_dsp_ipc *imx_dsp_handle;
> +
> +/*
> + * Get the default handle used by DSP
> + */
> +int imx_dsp_get_handle(struct imx_dsp_ipc **ipc)
> +{
> +	if (!imx_dsp_handle)
> +		return -EPROBE_DEFER;
> +
> +	*ipc = imx_dsp_handle;
> +	return 0;
> +}
> +EXPORT_SYMBOL(imx_dsp_get_handle);

Please, extract needed device or handle form device tree. The consumer
should pars own device tree node and get the phandle to the dsp node.

> +void imx_dsp_set_data(struct imx_dsp_ipc *ipc, void *data)
> +{
> +	if (!ipc)
> +		return;
> +
> +	ipc->private_data = data;
> +}
> +EXPORT_SYMBOL(imx_dsp_set_data);
> +
> +void *imx_dsp_get_data(struct imx_dsp_ipc *ipc)
> +{
> +	if (!ipc)
> +		return NULL;
> +
> +	return ipc->private_data;
> +}
> +EXPORT_SYMBOL(imx_dsp_get_data);
> +
> +/*
> + * imx_dsp_ring_doorbell - triggers an interrupt on the other side (DSP)
> + *
> + * @dsp: DSP IPC handle
> + * @chan_idx: index of the channel where to trigger the interrupt
> + *
> + * Returns non-negative value for success, negative value for error
> + */
> +int imx_dsp_ring_doorbell(struct imx_dsp_ipc *ipc, unsigned int idx)
> +{
> +	int ret;
> +	struct imx_dsp_chan *dsp_chan;
> +
> +	if (idx > DSP_MU_CHAN_NUM)
> +		return -EINVAL;

On this test idx may overflow. DSP_MU_CHAN_NUM is 4, means idx can be:
0, 1, 2, 3. In you case idx == 4 is allowed, so the caller will be able
to corrupt the rest of imx_dsp_ipc struct.

> +	dsp_chan = &ipc->chans[idx];
> +	ret = mbox_send_message(dsp_chan->ch, NULL);
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(imx_dsp_ring_doorbell);
> +
> +/*
> + * imx_dsp_handle_rx - rx callback used by imx mailbox
> + *
> + * @c: mbox client
> + * @msg: message received
> + *
> + * Users of DSP IPC will need to privde handle_reply and handle_request
> + * callbacks.
> + */
> +static void imx_dsp_handle_rx(struct mbox_client *c, void *msg)
> +{
> +	struct imx_dsp_chan *chan = container_of(c, struct imx_dsp_chan, cl);
> +
> +	if (chan->idx == 0) {
> +		chan->ipc->ops->handle_reply(chan->ipc);
> +	} else {
> +		chan->ipc->ops->handle_request(chan->ipc);
> +		imx_dsp_ring_doorbell(chan->ipc, 1);
> +	}
> +}
> +
> +static int imx_dsp_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct imx_dsp_ipc *dsp_ipc;
> +	struct imx_dsp_chan *dsp_chan;
> +	struct mbox_client *cl;
> +	char *chan_name;
> +	int ret;
> +	int i;
> +
> +	dsp_ipc = devm_kzalloc(dev, sizeof(*dsp_ipc), GFP_KERNEL);
> +	if (!dsp_ipc)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < DSP_MU_CHAN_NUM; i++) {
> +		if (i < 2)
> +			chan_name = kasprintf(GFP_KERNEL, "txdb%d", i);
> +		else
> +			chan_name = kasprintf(GFP_KERNEL, "rxdb%d", i - 2);
> +
> +		if (!chan_name)
> +			return -ENOMEM;
> +
> +		dsp_chan = &dsp_ipc->chans[i];
> +		cl = &dsp_chan->cl;
> +		cl->dev = dev;
> +		cl->tx_block = false;
> +		cl->knows_txdone = true;
> +		cl->rx_callback = imx_dsp_handle_rx;
> +
> +		dsp_chan->ipc = dsp_ipc;
> +		dsp_chan->idx = i % 2;
> +		dsp_chan->ch = mbox_request_channel_byname(cl, chan_name);
> +		if (IS_ERR(dsp_chan->ch)) {
> +			ret = PTR_ERR(dsp_chan->ch);
> +			if (ret != -EPROBE_DEFER)
> +				dev_err(dev, "Failed to request mbox chan %s ret %d\n",
> +					chan_name, ret);
> +			return ret;

On the error you will leak the memory previously allocated chan_name.
And you should call mbox_free_channel() for each previously registered
channel in this loop. 

> +		}
> +
> +		dev_dbg(dev, "request mbox chan %s\n", chan_name);
> +		/* chan_name is not used anymore by framework */
> +		kfree(chan_name);
> +	}
> +
> +	dsp_ipc->dev = dev;
> +
> +	imx_dsp_handle = dsp_ipc;

bad idea. What happens if multiple dsp nodes are registered in the
device tree?

> +	dev_info(dev, "NXP i.MX DSP IPC initialized\n");
> +
> +	return devm_of_platform_populate(dev);
> +}
> +
> +static const struct of_device_id imx_dsp_match[] = {
> +	{ .compatible = "fsl,imx-dsp", },

i would prefer to have chip name in the compatible. For example
fsl,imx8qm-dsp. Soon or later we will need to define some quirks
for on or another chip.

> +	{ /* Sentinel */ }
> +};
> +
> +static struct platform_driver imx_dsp_driver = {
> +	.driver = {
> +		.name = "imx-dsp",
> +		.of_match_table = imx_dsp_match,
> +	},
> +	.probe = imx_dsp_probe,
> +};
> +builtin_platform_driver(imx_dsp_driver);
> +
> +MODULE_AUTHOR("Daniel Baluta <daniel.baluta@nxp.com>");
> +MODULE_DESCRIPTION("IMX DSP IPC protocol driver");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/firmware/imx/dsp.h b/include/linux/firmware/imx/dsp.h
> new file mode 100644
> index 000000000000..75637d8fab34
> --- /dev/null
> +++ b/include/linux/firmware/imx/dsp.h
> @@ -0,0 +1,61 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * Copyright 2018 NXP
> + *
> + * Header file for the DSP IPC implementation
> + */
> +
> +#ifndef _IMX_DSP_IPC_H
> +#define _IMX_DSP_IPC_H
> +
> +#include <linux/device.h>
> +#include <linux/types.h>
> +#include <linux/mailbox_client.h>
> +
> +#define DSP_MU_CHAN_NUM		4
> +
> +struct imx_dsp_chan {
> +	struct imx_dsp_ipc *ipc;
> +	struct mbox_client cl;
> +	struct mbox_chan *ch;
> +	int idx;
> +};
> +
> +struct imx_dsp_ops {
> +	void (*handle_reply)(struct imx_dsp_ipc *ipc);
> +	void (*handle_request)(struct imx_dsp_ipc *ipc);
> +};
> +
> +struct imx_dsp_ipc {
> +	/* Host <-> DSP communication uses 2 txdb and 2 rxdb channels */
> +	struct imx_dsp_chan chans[DSP_MU_CHAN_NUM];
> +	struct device *dev;
> +	struct imx_dsp_ops *ops;
> +	void *private_data;
> +};
> +
> +#if IS_ENABLED(CONFIG_IMX_DSP)
> +
> +int imx_dsp_ring_doorbell(struct imx_dsp_ipc *dsp, unsigned int chan_idx);
> +int imx_dsp_get_handle(struct imx_dsp_ipc **ipc);
> +void imx_dsp_set_data(struct imx_dsp_ipc *ipc, void *data);
> +void *imx_dsp_get_data(struct imx_dsp_ipc *ipc);
> +
> +#else
> +
> +static inline int imx_dsp_get_handle(struct imx_dsp_ipc **ipc)
> +{
> +	return -EIO;

please, use -ENOTSUPP instead.

> +}
> +
> +static inline int imx_dsp_ring_doorbell(struct imx_dsp_ipc *ipc,
> +					unsigned int chan_idx)
> +{
> +	return -EIO;
> +}
> +
> +void imx_dsp_set_data(struct imx_dsp_ipc *ipc, void *data) { }
> +void *imx_dsp_get_data(struct imx_dsp_ipc *ipc) { return NULL; }
> +
> +#endif
> +#endif /* _IMX_DSP_IPC_H */
> -- 
> 2.17.1
> 
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 1/2] firmware: imx: Add DSP IPC protocol driver

[Daniel Baluta]

On Fri, Jun 14, 2019 at 12:08 PM Oleksij Rempel <o.rempel@pengutronix.de> wrote:
>
> Hi Daniel,
>
> please, see my review inline.

Thanks Oleksij for review. See my answers inline.

>
> On Fri, Jun 14, 2019 at 04:16:49PM +0800, daniel.baluta@nxp.com wrote:
> > From: Daniel Baluta <daniel.baluta@nxp.com>
> >
> > Some of i.MX8 processors (e.g i.MX8QM, i.MX8QXP) contain
> > the Tensilica HiFi4 DSP for advanced pre- and post-audio
> > processing.
> >
> > The communication between Host CPU and DSP firmware is
> > taking place using a shared memory area for message passing
> > and a dedicated Messaging Unit for notifications.
> >
> > DSP IPC protocol driver offers a doorbell interface using
> > imx-mailbox API.
> >
> > We use 4 MU channels (2 x TXDB, 2 x RXDB) to implement a
> > request-reply protocol.
> >
> > Connection 0 (txdb0, rxdb0):
> >         - Host writes messasge to shared memory [SHMEM]
> >       - Host sends a request [MU]
> >       - DSP handles request [SHMEM]
> >       - DSP sends reply [MU]
> >
> > Connection 1 (txdb1, rxdb1):
> >       - DSP writes a message to shared memory [SHMEM]
> >       - DSP sends a request [MU]
> >       - Host handles request [SHMEM]
> >       - Host sends reply [MU]
> >
> > The protocol driver will be used by a Host client to
> > communicate with the DSP. First client will be the i.MX8
> > part from Sound Open Firmware infrastructure.
> >
> > The protocol drivers offers the following interface:
> >
> > On Tx:
> >    - imx_dsp_ring_doorbell, will be called to notify the DSP
> >    that it needs to handle a request.
> >
> > On Rx:
> >    - clients need to provide two callbacks:
> >       .handle_reply
> >       .handle_request
> >   - the callbacks will be used by the protocol driver on
> >     notification arrival from DSP.
> >
> > Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
> > ---
> >  drivers/firmware/imx/Kconfig     |  11 ++
> >  drivers/firmware/imx/Makefile    |   1 +
> >  drivers/firmware/imx/imx-dsp.c   | 167 +++++++++++++++++++++++++++++++
> >  include/linux/firmware/imx/dsp.h |  61 +++++++++++
> >  4 files changed, 240 insertions(+)
> >  create mode 100644 drivers/firmware/imx/imx-dsp.c
> >  create mode 100644 include/linux/firmware/imx/dsp.h
> >
> > diff --git a/drivers/firmware/imx/Kconfig b/drivers/firmware/imx/Kconfig
> > index 42b566f8903f..383996b679a8 100644
> > --- a/drivers/firmware/imx/Kconfig
> > +++ b/drivers/firmware/imx/Kconfig
> > @@ -1,4 +1,15 @@
> >  # SPDX-License-Identifier: GPL-2.0-only
> > +config IMX_DSP
> > +     bool "IMX DSP Protocol driver"
> > +     depends on IMX_MBOX
> > +     help
> > +       This enables DSP IPC protocol between host CPU (Linux)
> > +       and the firmware running on DSP.
> > +       DSP exists on some i.MX8 processors (e.g i.MX8QM, i.MX8QXP).
> > +
> > +          It acts like a doorbell. Client might use shared memory to
> > +       exchange information with DSP side.
> > +
> >  config IMX_SCU
> >       bool "IMX SCU Protocol driver"
> >       depends on IMX_MBOX
> > diff --git a/drivers/firmware/imx/Makefile b/drivers/firmware/imx/Makefile
> > index 802c4ad8e8f9..08bc9ddfbdfb 100644
> > --- a/drivers/firmware/imx/Makefile
> > +++ b/drivers/firmware/imx/Makefile
> > @@ -1,3 +1,4 @@
> >  # SPDX-License-Identifier: GPL-2.0
> > +obj-$(CONFIG_IMX_DSP)                += imx-dsp.o
> >  obj-$(CONFIG_IMX_SCU)                += imx-scu.o misc.o imx-scu-irq.o
> >  obj-$(CONFIG_IMX_SCU_PD)     += scu-pd.o
> > diff --git a/drivers/firmware/imx/imx-dsp.c b/drivers/firmware/imx/imx-dsp.c
> > new file mode 100644
> > index 000000000000..953fd364ad76
> > --- /dev/null
> > +++ b/drivers/firmware/imx/imx-dsp.c
> > @@ -0,0 +1,167 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright 2018 NXP
> > + *  Author: Daniel Baluta <daniel.baluta@nxp.com>
> > + *
> > + * Implementation of the DSP IPC interface (host side)
> > + */
> > +
> > +#include <linux/firmware/imx/dsp.h>
> > +#include <linux/kernel.h>
> > +#include <linux/mailbox_client.h>
> > +#include <linux/module.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/slab.h>
> > +
> > +static struct imx_dsp_ipc *imx_dsp_handle;
> > +
> > +/*
> > + * Get the default handle used by DSP
> > + */
> > +int imx_dsp_get_handle(struct imx_dsp_ipc **ipc)
> > +{
> > +     if (!imx_dsp_handle)
> > +             return -EPROBE_DEFER;
> > +
> > +     *ipc = imx_dsp_handle;
> > +     return 0;
> > +}
> > +EXPORT_SYMBOL(imx_dsp_get_handle);
>
> Please, extract needed device or handle form device tree. The consumer
> should pars own device tree node and get the phandle to the dsp node.

I understand the idea, but I'm not sure how this can be done.

imx_dsp_handle is just a pointer to the memory allocated in imx_dsp_probe.

I assume that consumer might have access to this driver's
platform_device dev and
we can hive the imx_dsp_ipc handle inside some sort of private date. I will
investigate this. So far I have followed the approach taken in
drivers/firmware/imx/imx-scu.c

>
> > +void imx_dsp_set_data(struct imx_dsp_ipc *ipc, void *data)
> > +{
> > +     if (!ipc)
> > +             return;
> > +
> > +     ipc->private_data = data;
> > +}
> > +EXPORT_SYMBOL(imx_dsp_set_data);
> > +
> > +void *imx_dsp_get_data(struct imx_dsp_ipc *ipc)
> > +{
> > +     if (!ipc)
> > +             return NULL;
> > +
> > +     return ipc->private_data;
> > +}
> > +EXPORT_SYMBOL(imx_dsp_get_data);
> > +
> > +/*
> > + * imx_dsp_ring_doorbell - triggers an interrupt on the other side (DSP)
> > + *
> > + * @dsp: DSP IPC handle
> > + * @chan_idx: index of the channel where to trigger the interrupt
> > + *
> > + * Returns non-negative value for success, negative value for error
> > + */
> > +int imx_dsp_ring_doorbell(struct imx_dsp_ipc *ipc, unsigned int idx)
> > +{
> > +     int ret;
> > +     struct imx_dsp_chan *dsp_chan;
> > +
> > +     if (idx > DSP_MU_CHAN_NUM)
> > +             return -EINVAL;
>
> On this test idx may overflow. DSP_MU_CHAN_NUM is 4, means idx can be:
> 0, 1, 2, 3. In you case idx == 4 is allowed, so the caller will be able
> to corrupt the rest of imx_dsp_ipc struct.

Indeed, you are right! Will fix in v2.

>
> > +     dsp_chan = &ipc->chans[idx];
> > +     ret = mbox_send_message(dsp_chan->ch, NULL);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     return 0;
> > +}
> > +EXPORT_SYMBOL(imx_dsp_ring_doorbell);
> > +
> > +/*
> > + * imx_dsp_handle_rx - rx callback used by imx mailbox
> > + *
> > + * @c: mbox client
> > + * @msg: message received
> > + *
> > + * Users of DSP IPC will need to privde handle_reply and handle_request
> > + * callbacks.
> > + */
> > +static void imx_dsp_handle_rx(struct mbox_client *c, void *msg)
> > +{
> > +     struct imx_dsp_chan *chan = container_of(c, struct imx_dsp_chan, cl);
> > +
> > +     if (chan->idx == 0) {
> > +             chan->ipc->ops->handle_reply(chan->ipc);
> > +     } else {
> > +             chan->ipc->ops->handle_request(chan->ipc);
> > +             imx_dsp_ring_doorbell(chan->ipc, 1);
> > +     }
> > +}
> > +
> > +static int imx_dsp_probe(struct platform_device *pdev)
> > +{
> > +     struct device *dev = &pdev->dev;
> > +     struct imx_dsp_ipc *dsp_ipc;
> > +     struct imx_dsp_chan *dsp_chan;
> > +     struct mbox_client *cl;
> > +     char *chan_name;
> > +     int ret;
> > +     int i;
> > +
> > +     dsp_ipc = devm_kzalloc(dev, sizeof(*dsp_ipc), GFP_KERNEL);
> > +     if (!dsp_ipc)
> > +             return -ENOMEM;
> > +
> > +     for (i = 0; i < DSP_MU_CHAN_NUM; i++) {
> > +             if (i < 2)
> > +                     chan_name = kasprintf(GFP_KERNEL, "txdb%d", i);
> > +             else
> > +                     chan_name = kasprintf(GFP_KERNEL, "rxdb%d", i - 2);
> > +
> > +             if (!chan_name)
> > +                     return -ENOMEM;
> > +
> > +             dsp_chan = &dsp_ipc->chans[i];
> > +             cl = &dsp_chan->cl;
> > +             cl->dev = dev;
> > +             cl->tx_block = false;
> > +             cl->knows_txdone = true;
> > +             cl->rx_callback = imx_dsp_handle_rx;
> > +
> > +             dsp_chan->ipc = dsp_ipc;
> > +             dsp_chan->idx = i % 2;
> > +             dsp_chan->ch = mbox_request_channel_byname(cl, chan_name);
> > +             if (IS_ERR(dsp_chan->ch)) {
> > +                     ret = PTR_ERR(dsp_chan->ch);
> > +                     if (ret != -EPROBE_DEFER)
> > +                             dev_err(dev, "Failed to request mbox chan %s ret %d\n",
> > +                                     chan_name, ret);
> > +                     return ret;
>
> On the error you will leak the memory previously allocated chan_name.
> And you should call mbox_free_channel() for each previously registered
> channel in this loop.

Will fix in v2.

>
> > +             }
> > +
> > +             dev_dbg(dev, "request mbox chan %s\n", chan_name);
> > +             /* chan_name is not used anymore by framework */
> > +             kfree(chan_name);
> > +     }
> > +
> > +     dsp_ipc->dev = dev;
> > +
> > +     imx_dsp_handle = dsp_ipc;
>
> bad idea. What happens if multiple dsp nodes are registered in the
> device tree?

True. Altough we have only one DSP. Having that global variable seemed
a very idea from the start. Will need to fix that also in imx-scu.c

>
> > +     dev_info(dev, "NXP i.MX DSP IPC initialized\n");
> > +
> > +     return devm_of_platform_populate(dev);
> > +}
> > +
> > +static const struct of_device_id imx_dsp_match[] = {
> > +     { .compatible = "fsl,imx-dsp", },
>
> i would prefer to have chip name in the compatible. For example
> fsl,imx8qm-dsp. Soon or later we will need to define some quirks
> for on or another chip.

I see. Will fix in v2.

>
> > +     { /* Sentinel */ }
> > +};
> > +
> > +static struct platform_driver imx_dsp_driver = {
> > +     .driver = {
> > +             .name = "imx-dsp",
> > +             .of_match_table = imx_dsp_match,
> > +     },
> > +     .probe = imx_dsp_probe,
> > +};
> > +builtin_platform_driver(imx_dsp_driver);
> > +
> > +MODULE_AUTHOR("Daniel Baluta <daniel.baluta@nxp.com>");
> > +MODULE_DESCRIPTION("IMX DSP IPC protocol driver");
> > +MODULE_LICENSE("GPL v2");
> > diff --git a/include/linux/firmware/imx/dsp.h b/include/linux/firmware/imx/dsp.h
> > new file mode 100644
> > index 000000000000..75637d8fab34
> > --- /dev/null
> > +++ b/include/linux/firmware/imx/dsp.h
> > @@ -0,0 +1,61 @@
> > +/* SPDX-License-Identifier: GPL-2.0+ */
> > +/*
> > + * Copyright 2018 NXP
> > + *
> > + * Header file for the DSP IPC implementation
> > + */
> > +
> > +#ifndef _IMX_DSP_IPC_H
> > +#define _IMX_DSP_IPC_H
> > +
> > +#include <linux/device.h>
> > +#include <linux/types.h>
> > +#include <linux/mailbox_client.h>
> > +
> > +#define DSP_MU_CHAN_NUM              4
> > +
> > +struct imx_dsp_chan {
> > +     struct imx_dsp_ipc *ipc;
> > +     struct mbox_client cl;
> > +     struct mbox_chan *ch;
> > +     int idx;
> > +};
> > +
> > +struct imx_dsp_ops {
> > +     void (*handle_reply)(struct imx_dsp_ipc *ipc);
> > +     void (*handle_request)(struct imx_dsp_ipc *ipc);
> > +};
> > +
> > +struct imx_dsp_ipc {
> > +     /* Host <-> DSP communication uses 2 txdb and 2 rxdb channels */
> > +     struct imx_dsp_chan chans[DSP_MU_CHAN_NUM];
> > +     struct device *dev;
> > +     struct imx_dsp_ops *ops;
> > +     void *private_data;
> > +};
> > +
> > +#if IS_ENABLED(CONFIG_IMX_DSP)
> > +
> > +int imx_dsp_ring_doorbell(struct imx_dsp_ipc *dsp, unsigned int chan_idx);
> > +int imx_dsp_get_handle(struct imx_dsp_ipc **ipc);
> > +void imx_dsp_set_data(struct imx_dsp_ipc *ipc, void *data);
> > +void *imx_dsp_get_data(struct imx_dsp_ipc *ipc);
> > +
> > +#else
> > +
> > +static inline int imx_dsp_get_handle(struct imx_dsp_ipc **ipc)
> > +{
> > +     return -EIO;
>
> please, use -ENOTSUPP instead.

Makes sense. Will fix in v2.

thanks,
Daniel.

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

[Rob Herring]

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
>

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

[Daniel Baluta]

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

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

[Rob Herring]

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

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.

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

[Rob Herring]

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

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

[Daniel Baluta]

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.