[PATCH v1 0/3] remoteproc: st: add virtio_rpmsg support

[PATCH v1 0/3] remoteproc: st: add virtio_rpmsg support

[Loic Pallardy]

Goal of this series is:
- to add vring based communication link (virtio_rpmsg)
- to add rproc_da_to_va translation function to allow firmware loading in
  pre-reserved carveout memory region

Loic Pallardy (3):
  remoteproc: st: add virtio communication support
  remoteproc: st: add da to va support
  remoteproc: core: don't allocate carveout if pa or da are defined

 drivers/remoteproc/Kconfig           |   3 +
 drivers/remoteproc/remoteproc_core.c |   5 ++
 drivers/remoteproc/st_remoteproc.c   | 161 +++++++++++++++++++++++++++++++++--
 3 files changed, 163 insertions(+), 6 deletions(-)

-- 
1.9.1

[PATCH v1 1/3] remoteproc: st: add virtio communication support

[Loic Pallardy]

This patch provides virtio communication support based on mailbox
for ST coprocessors.

Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
---
 drivers/remoteproc/Kconfig         |   3 +
 drivers/remoteproc/st_remoteproc.c | 121 ++++++++++++++++++++++++++++++++++++-
 2 files changed, 123 insertions(+), 1 deletion(-)

diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
index 51d7ca0..fd275c0 100644
--- a/drivers/remoteproc/Kconfig
+++ b/drivers/remoteproc/Kconfig
@@ -119,6 +119,9 @@ config ST_REMOTEPROC
 	tristate "ST remoteproc support"
 	depends on ARCH_STI
 	depends on REMOTEPROC
+	select MAILBOX
+	select STI_MBOX
+	select RPMSG_VIRTIO
 	help
 	  Say y here to support ST's adjunct processors via the remote
 	  processor framework.
diff --git a/drivers/remoteproc/st_remoteproc.c b/drivers/remoteproc/st_remoteproc.c
index da4e152..113caf2 100644
--- a/drivers/remoteproc/st_remoteproc.c
+++ b/drivers/remoteproc/st_remoteproc.c
@@ -15,6 +15,7 @@
 #include <linux/err.h>
 #include <linux/interrupt.h>
 #include <linux/kernel.h>
+#include <linux/mailbox_client.h>
 #include <linux/mfd/syscon.h>
 #include <linux/module.h>
 #include <linux/of.h>
@@ -25,6 +26,17 @@
 #include <linux/remoteproc.h>
 #include <linux/reset.h>
 
+#include "remoteproc_internal.h"
+
+#define ST_RPROC_MAX_VRING	2
+
+#define MBOX_RX			0
+#define MBOX_TX			1
+#define MBOX_MAX		2
+
+static struct mbox_client mbox_client_vq0;
+static struct mbox_client mbox_client_vq1;
+
 struct st_rproc_config {
 	bool			sw_reset;
 	bool			pwr_reset;
@@ -39,8 +51,45 @@ struct st_rproc {
 	u32			clk_rate;
 	struct regmap		*boot_base;
 	u32			boot_offset;
+	struct mbox_chan	*mbox_chan[ST_RPROC_MAX_VRING][MBOX_MAX];
 };
 
+static void st_rproc_mbox_callback(struct device *dev, u32 msg)
+{
+	struct rproc *rproc = dev_get_drvdata(dev);
+
+	if (rproc_vq_interrupt(rproc, msg) == IRQ_NONE)
+		dev_dbg(dev, "no message was found in vqid %d\n", msg);
+}
+
+static void st_rproc_mbox_callback_vq0(struct mbox_client *mbox_client,
+				       void *data)
+{
+	st_rproc_mbox_callback(mbox_client->dev, 0);
+}
+
+static void st_rproc_mbox_callback_vq1(struct mbox_client *mbox_client,
+				       void *data)
+{
+	st_rproc_mbox_callback(mbox_client->dev, 1);
+}
+
+static void st_rproc_kick(struct rproc *rproc, int vqid)
+{
+	struct st_rproc *ddata = rproc->priv;
+	struct device *dev = rproc->dev.parent;
+	int ret;
+
+	/* send the index of the triggered virtqueue in the mailbox payload */
+	if (vqid < ST_RPROC_MAX_VRING) {
+		ret = mbox_send_message(ddata->mbox_chan[vqid][MBOX_TX],
+					(void *)&vqid);
+		if (ret < 0)
+			dev_err(dev,
+				"failed to send message via mbox: %d\n", ret);
+	}
+}
+
 static int st_rproc_start(struct rproc *rproc)
 {
 	struct st_rproc *ddata = rproc->priv;
@@ -108,6 +157,7 @@ static int st_rproc_stop(struct rproc *rproc)
 }
 
 static struct rproc_ops st_rproc_ops = {
+	.kick		= st_rproc_kick,
 	.start		= st_rproc_start,
 	.stop		= st_rproc_stop,
 };
@@ -221,6 +271,7 @@ static int st_rproc_probe(struct platform_device *pdev)
 	struct st_rproc *ddata;
 	struct device_node *np = dev->of_node;
 	struct rproc *rproc;
+	struct mbox_chan *chan;
 	int enabled;
 	int ret;
 
@@ -257,13 +308,76 @@ static int st_rproc_probe(struct platform_device *pdev)
 		clk_set_rate(ddata->clk, ddata->clk_rate);
 	}
 
+	if (of_get_property(np, "mbox-names", NULL)) {
+		mbox_client_vq0.dev		= dev;
+		mbox_client_vq0.tx_done		= NULL;
+		mbox_client_vq0.tx_block	= false;
+		mbox_client_vq0.knows_txdone	= false;
+		mbox_client_vq0.rx_callback	= st_rproc_mbox_callback_vq0;
+
+		mbox_client_vq1.dev		= dev;
+		mbox_client_vq1.tx_done		= NULL;
+		mbox_client_vq1.tx_block	= false;
+		mbox_client_vq1.knows_txdone	= false;
+		mbox_client_vq1.rx_callback	= st_rproc_mbox_callback_vq1;
+
+		/*
+		 * To control a co-processor without IPC mechanism.
+		 * This driver can be used without mbox and rpmsg.
+		 */
+		chan = mbox_request_channel_byname(&mbox_client_vq0, "vq0_rx");
+		if (IS_ERR(chan)) {
+			dev_err(&rproc->dev, "failed to request mbox chan 0\n");
+			ret = PTR_ERR(chan);
+			goto free_rproc;
+		} else {
+			ddata->mbox_chan[0][MBOX_RX] = chan;
+		}
+
+		chan = mbox_request_channel_byname(&mbox_client_vq0, "vq0_tx");
+		if (IS_ERR(chan)) {
+			dev_err(&rproc->dev, "failed to request mbox chan 0\n");
+			ret = PTR_ERR(chan);
+			goto free_one;
+		} else {
+			ddata->mbox_chan[0][MBOX_TX] = chan;
+		}
+
+		chan = mbox_request_channel_byname(&mbox_client_vq1, "vq1_rx");
+		if (IS_ERR(chan)) {
+			dev_err(&rproc->dev, "failed to request mbox chan 1\n");
+			ret = PTR_ERR(chan);
+			goto free_two;
+		} else {
+			ddata->mbox_chan[1][MBOX_RX] = chan;
+		}
+
+		chan = mbox_request_channel_byname(&mbox_client_vq1, "vq1_tx");
+		if (IS_ERR(chan)) {
+			dev_err(&rproc->dev, "failed to request mbox chan 1\n");
+			ret = PTR_ERR(chan);
+			goto free_three;
+		} else {
+			ddata->mbox_chan[1][MBOX_TX] = chan;
+		}
+	}
+
 	ret = rproc_add(rproc);
 	if (ret)
-		goto free_rproc;
+		goto free_for;
 
 	return 0;
 
+free_for:
+	mbox_free_channel(ddata->mbox_chan[1][MBOX_TX]);
+free_three:
+	mbox_free_channel(ddata->mbox_chan[1][MBOX_RX]);
+free_two:
+	mbox_free_channel(ddata->mbox_chan[0][MBOX_TX]);
+free_one:
+	mbox_free_channel(ddata->mbox_chan[0][MBOX_RX]);
 free_rproc:
+	clk_unprepare(ddata->clk);
 	rproc_free(rproc);
 	return ret;
 }
@@ -279,6 +393,11 @@ static int st_rproc_remove(struct platform_device *pdev)
 
 	of_reserved_mem_device_release(&pdev->dev);
 
+	mbox_free_channel(ddata->mbox_chan[0][MBOX_RX]);
+	mbox_free_channel(ddata->mbox_chan[1][MBOX_RX]);
+	mbox_free_channel(ddata->mbox_chan[0][MBOX_TX]);
+	mbox_free_channel(ddata->mbox_chan[1][MBOX_TX]);
+
 	rproc_free(rproc);
 
 	return 0;
-- 
1.9.1

[PATCH v1 2/3] remoteproc: st: add da to va support

[Loic Pallardy]

ST remoteproc driver needs to provide information about
carveout memory region to allow remoteproc core to load
firmware and access trace buffer.

Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
---
 drivers/remoteproc/st_remoteproc.c | 42 ++++++++++++++++++++++++++++++++------
 1 file changed, 36 insertions(+), 6 deletions(-)

diff --git a/drivers/remoteproc/st_remoteproc.c b/drivers/remoteproc/st_remoteproc.c
index 113caf2..51199a8 100644
--- a/drivers/remoteproc/st_remoteproc.c
+++ b/drivers/remoteproc/st_remoteproc.c
@@ -21,6 +21,7 @@
 #include <linux/of.h>
 #include <linux/of_device.h>
 #include <linux/of_reserved_mem.h>
+#include <linux/of_address.h>
 #include <linux/platform_device.h>
 #include <linux/regmap.h>
 #include <linux/remoteproc.h>
@@ -52,6 +53,10 @@ struct st_rproc {
 	struct regmap		*boot_base;
 	u32			boot_offset;
 	struct mbox_chan	*mbox_chan[ST_RPROC_MAX_VRING][MBOX_MAX];
+	phys_addr_t mem_phys;
+	phys_addr_t mem_reloc;
+	void *mem_region;
+	size_t mem_size;
 };
 
 static void st_rproc_mbox_callback(struct device *dev, u32 msg)
@@ -156,10 +161,23 @@ static int st_rproc_stop(struct rproc *rproc)
 	return sw_err ?: pwr_err;
 }
 
+static void *st_proc_da_to_va(struct rproc *rproc, u64 da, int len)
+{
+	struct st_rproc *ddata = rproc->priv;
+	int offset;
+
+	offset = da - ddata->mem_reloc;
+	if (offset < 0 || offset + len > ddata->mem_size)
+		return NULL;
+
+	return ddata->mem_region + offset;
+}
+
 static struct rproc_ops st_rproc_ops = {
 	.kick		= st_rproc_kick,
 	.start		= st_rproc_start,
 	.stop		= st_rproc_stop,
+	.da_to_va	= st_proc_da_to_va,
 };
 
 /*
@@ -207,7 +225,8 @@ static int st_rproc_parse_dt(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct rproc *rproc = platform_get_drvdata(pdev);
 	struct st_rproc *ddata = rproc->priv;
-	struct device_node *np = dev->of_node;
+	struct device_node *np = dev->of_node, *node;
+	struct resource res;
 	int err;
 
 	if (ddata->config->sw_reset) {
@@ -251,10 +270,23 @@ static int st_rproc_parse_dt(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
-	err = of_reserved_mem_device_init(dev);
-	if (err) {
-		dev_err(dev, "Failed to obtain shared memory\n");
+	node = of_parse_phandle(np, "memory-region", 0);
+	if (!node) {
+		dev_err(dev, "No memory-region specified\n");
+		return -EINVAL;
+	}
+
+	err = of_address_to_resource(node, 0, &res);
+	if (err)
 		return err;
+
+	ddata->mem_phys = ddata->mem_reloc = res.start;
+	ddata->mem_size = resource_size(&res);
+	ddata->mem_region = devm_ioremap_wc(dev, ddata->mem_phys, ddata->mem_size);
+	if (!ddata->mem_region) {
+		dev_err(dev, "Unable to map memory region: %pa+%zx\n",
+			&res.start, ddata->mem_size);
+		return -EBUSY;
 	}
 
 	err = clk_prepare(ddata->clk);
@@ -391,8 +423,6 @@ static int st_rproc_remove(struct platform_device *pdev)
 
 	clk_disable_unprepare(ddata->clk);
 
-	of_reserved_mem_device_release(&pdev->dev);
-
 	mbox_free_channel(ddata->mbox_chan[0][MBOX_RX]);
 	mbox_free_channel(ddata->mbox_chan[1][MBOX_RX]);
 	mbox_free_channel(ddata->mbox_chan[0][MBOX_TX]);
-- 
1.9.1

[PATCH v1 3/3] remoteproc: core: don't allocate carveout if pa or da are defined

[Loic Pallardy]

Remoteproc doesn't check if firmware requests fixed
addresses for carveout regions.
Current assumption is that platform specific driver is in
charge of coprocessor specific memory region allocation and
remoteproc core doesn't have to handle them.
If a da or a pa is specified in firmware resource table, remoteproc
core doesn't have to perform any allocation.
Access to carveout will be done thanks to rproc_da_to_pa function,
which will provide virtual address on carveout region allocated
by platform specific driver.

Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
---
 drivers/remoteproc/remoteproc_core.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index f0f6ec1..022f9a6 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -625,6 +625,11 @@ static int rproc_handle_carveout(struct rproc *rproc,
 	dev_dbg(dev, "carveout rsc: name: %s, da 0x%x, pa 0x%x, len 0x%x, flags 0x%x\n",
 		rsc->name, rsc->da, rsc->pa, rsc->len, rsc->flags);
 
+	if (rsc->pa != FW_RSC_ADDR_ANY || rsc->da != FW_RSC_ADDR_ANY) {
+		dev_dbg(dev, "carveout already allocated by low level driver\n");
+		return 0;
+	}
+
 	carveout = kzalloc(sizeof(*carveout), GFP_KERNEL);
 	if (!carveout)
 		return -ENOMEM;
-- 
1.9.1

Re: [PATCH v1 0/3] remoteproc: st: add virtio_rpmsg support

[Patrice Chotard]

On 12/07/2016 09:33 PM, Loic Pallardy wrote:
> Goal of this series is:
> - to add vring based communication link (virtio_rpmsg)
> - to add rproc_da_to_va translation function to allow firmware loading in
>   pre-reserved carveout memory region
> 
> Loic Pallardy (3):
>   remoteproc: st: add virtio communication support
>   remoteproc: st: add da to va support
>   remoteproc: core: don't allocate carveout if pa or da are defined
> 
>  drivers/remoteproc/Kconfig           |   3 +
>  drivers/remoteproc/remoteproc_core.c |   5 ++
>  drivers/remoteproc/st_remoteproc.c   | 161 +++++++++++++++++++++++++++++++++--
>  3 files changed, 163 insertions(+), 6 deletions(-)
> 

Hi Loic

For the entire series

Acked-by: Patrice Chotard <patrice.chotard@st.com>

Patrice

Re: [STLinux Kernel] [PATCH v1 0/3] remoteproc: st: add virtio_rpmsg support

[Peter Griffin]

On Wed, 07 Dec 2016, Loic Pallardy wrote:

> Goal of this series is:
> - to add vring based communication link (virtio_rpmsg)
> - to add rproc_da_to_va translation function to allow firmware loading in
>   pre-reserved carveout memory region
> 
> Loic Pallardy (3):
>   remoteproc: st: add virtio communication support
>   remoteproc: st: add da to va support
>   remoteproc: core: don't allocate carveout if pa or da are defined

For the series: -

Acked-by: Peter Griffin <peter.griffin@linaro.org>

Re: [PATCH v1 1/3] remoteproc: st: add virtio communication support

[Bjorn Andersson]

On Wed 07 Dec 12:33 PST 2016, Loic Pallardy wrote:

> This patch provides virtio communication support based on mailbox
> for ST coprocessors.
> 
> Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
> ---
>  drivers/remoteproc/Kconfig         |   3 +
>  drivers/remoteproc/st_remoteproc.c | 121 ++++++++++++++++++++++++++++++++++++-
>  2 files changed, 123 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
> index 51d7ca0..fd275c0 100644
> --- a/drivers/remoteproc/Kconfig
> +++ b/drivers/remoteproc/Kconfig
> @@ -119,6 +119,9 @@ config ST_REMOTEPROC
>  	tristate "ST remoteproc support"
>  	depends on ARCH_STI
>  	depends on REMOTEPROC
> +	select MAILBOX
> +	select STI_MBOX
> +	select RPMSG_VIRTIO
>  	help
>  	  Say y here to support ST's adjunct processors via the remote
>  	  processor framework.
> diff --git a/drivers/remoteproc/st_remoteproc.c b/drivers/remoteproc/st_remoteproc.c
> index da4e152..113caf2 100644
> --- a/drivers/remoteproc/st_remoteproc.c
> +++ b/drivers/remoteproc/st_remoteproc.c
> @@ -15,6 +15,7 @@
>  #include <linux/err.h>
>  #include <linux/interrupt.h>
>  #include <linux/kernel.h>
> +#include <linux/mailbox_client.h>
>  #include <linux/mfd/syscon.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
> @@ -25,6 +26,17 @@
>  #include <linux/remoteproc.h>
>  #include <linux/reset.h>
>  
> +#include "remoteproc_internal.h"
> +
> +#define ST_RPROC_MAX_VRING	2
> +
> +#define MBOX_RX			0
> +#define MBOX_TX			1
> +#define MBOX_MAX		2
> +
> +static struct mbox_client mbox_client_vq0;
> +static struct mbox_client mbox_client_vq1;
> +
>  struct st_rproc_config {
>  	bool			sw_reset;
>  	bool			pwr_reset;
> @@ -39,8 +51,45 @@ struct st_rproc {
>  	u32			clk_rate;
>  	struct regmap		*boot_base;
>  	u32			boot_offset;
> +	struct mbox_chan	*mbox_chan[ST_RPROC_MAX_VRING][MBOX_MAX];
>  };
>  
> +static void st_rproc_mbox_callback(struct device *dev, u32 msg)
> +{
> +	struct rproc *rproc = dev_get_drvdata(dev);
> +
> +	if (rproc_vq_interrupt(rproc, msg) == IRQ_NONE)
> +		dev_dbg(dev, "no message was found in vqid %d\n", msg);
> +}
> +
> +static void st_rproc_mbox_callback_vq0(struct mbox_client *mbox_client,
> +				       void *data)
> +{
> +	st_rproc_mbox_callback(mbox_client->dev, 0);
> +}
> +
> +static void st_rproc_mbox_callback_vq1(struct mbox_client *mbox_client,
> +				       void *data)
> +{
> +	st_rproc_mbox_callback(mbox_client->dev, 1);
> +}
> +
> +static void st_rproc_kick(struct rproc *rproc, int vqid)
> +{
> +	struct st_rproc *ddata = rproc->priv;
> +	struct device *dev = rproc->dev.parent;
> +	int ret;
> +
> +	/* send the index of the triggered virtqueue in the mailbox payload */
> +	if (vqid < ST_RPROC_MAX_VRING) {

If you flip this condition around and return if vqid >=
ST_RPROC_MAX_VRING (probably with a WARN_ON() around it) you will save
an indentation level and don't have to wrap the dev_err below.

> +		ret = mbox_send_message(ddata->mbox_chan[vqid][MBOX_TX],
> +					(void *)&vqid);
> +		if (ret < 0)
> +			dev_err(dev,
> +				"failed to send message via mbox: %d\n", ret);
> +	}
> +}
> +
>  static int st_rproc_start(struct rproc *rproc)
>  {
>  	struct st_rproc *ddata = rproc->priv;
> @@ -108,6 +157,7 @@ static int st_rproc_stop(struct rproc *rproc)
>  }
>  
>  static struct rproc_ops st_rproc_ops = {
> +	.kick		= st_rproc_kick,
>  	.start		= st_rproc_start,
>  	.stop		= st_rproc_stop,
>  };
> @@ -221,6 +271,7 @@ static int st_rproc_probe(struct platform_device *pdev)
>  	struct st_rproc *ddata;
>  	struct device_node *np = dev->of_node;
>  	struct rproc *rproc;
> +	struct mbox_chan *chan;
>  	int enabled;
>  	int ret;
>  
> @@ -257,13 +308,76 @@ static int st_rproc_probe(struct platform_device *pdev)
>  		clk_set_rate(ddata->clk, ddata->clk_rate);
>  	}
>  
> +	if (of_get_property(np, "mbox-names", NULL)) {
> +		mbox_client_vq0.dev		= dev;
> +		mbox_client_vq0.tx_done		= NULL;
> +		mbox_client_vq0.tx_block	= false;
> +		mbox_client_vq0.knows_txdone	= false;
> +		mbox_client_vq0.rx_callback	= st_rproc_mbox_callback_vq0;
> +
> +		mbox_client_vq1.dev		= dev;
> +		mbox_client_vq1.tx_done		= NULL;
> +		mbox_client_vq1.tx_block	= false;
> +		mbox_client_vq1.knows_txdone	= false;
> +		mbox_client_vq1.rx_callback	= st_rproc_mbox_callback_vq1;
> +

As far as I can see these are const (although the mbox api doesn't
declare the parameter as such), please fill them out as you declare the
variables outside the function scope.

> +		/*
> +		 * To control a co-processor without IPC mechanism.
> +		 * This driver can be used without mbox and rpmsg.
> +		 */
> +		chan = mbox_request_channel_byname(&mbox_client_vq0, "vq0_rx");
> +		if (IS_ERR(chan)) {
> +			dev_err(&rproc->dev, "failed to request mbox chan 0\n");
> +			ret = PTR_ERR(chan);
> +			goto free_rproc;
> +		} else {
> +			ddata->mbox_chan[0][MBOX_RX] = chan;

No need to have these in else blocks, as the other code paths ends with
a goto.

> +		}
> +
> +		chan = mbox_request_channel_byname(&mbox_client_vq0, "vq0_tx");
> +		if (IS_ERR(chan)) {
> +			dev_err(&rproc->dev, "failed to request mbox chan 0\n");
> +			ret = PTR_ERR(chan);
> +			goto free_one;
> +		} else {
> +			ddata->mbox_chan[0][MBOX_TX] = chan;
> +		}
> +
> +		chan = mbox_request_channel_byname(&mbox_client_vq1, "vq1_rx");
> +		if (IS_ERR(chan)) {
> +			dev_err(&rproc->dev, "failed to request mbox chan 1\n");
> +			ret = PTR_ERR(chan);
> +			goto free_two;
> +		} else {
> +			ddata->mbox_chan[1][MBOX_RX] = chan;
> +		}
> +
> +		chan = mbox_request_channel_byname(&mbox_client_vq1, "vq1_tx");
> +		if (IS_ERR(chan)) {
> +			dev_err(&rproc->dev, "failed to request mbox chan 1\n");
> +			ret = PTR_ERR(chan);
> +			goto free_three;
> +		} else {
> +			ddata->mbox_chan[1][MBOX_TX] = chan;
> +		}
> +	}
> +
>  	ret = rproc_add(rproc);
>  	if (ret)
> -		goto free_rproc;
> +		goto free_for;
>  
>  	return 0;
>  
> +free_for:
> +	mbox_free_channel(ddata->mbox_chan[1][MBOX_TX]);

If you flatten mbox_chan to a single-dimensional array you can loop over
it like:

for (i = 0; i < 4; i++)
	mbox_free_channel(ddata->mbox_chan[i]);

And you don't need to check if they are still NULL because
mbox_free_channel() handles that for you.

> +free_three:
> +	mbox_free_channel(ddata->mbox_chan[1][MBOX_RX]);
> +free_two:
> +	mbox_free_channel(ddata->mbox_chan[0][MBOX_TX]);
> +free_one:
> +	mbox_free_channel(ddata->mbox_chan[0][MBOX_RX]);
>  free_rproc:
> +	clk_unprepare(ddata->clk);

Nice catch, but I would like to have it in a separate patch - and it
should not be done when st_rproc_parse_dt() returns an error.

>  	rproc_free(rproc);
>  	return ret;
>  }
> @@ -279,6 +393,11 @@ static int st_rproc_remove(struct platform_device *pdev)
>  
>  	of_reserved_mem_device_release(&pdev->dev);
>  
> +	mbox_free_channel(ddata->mbox_chan[0][MBOX_RX]);
> +	mbox_free_channel(ddata->mbox_chan[1][MBOX_RX]);
> +	mbox_free_channel(ddata->mbox_chan[0][MBOX_TX]);
> +	mbox_free_channel(ddata->mbox_chan[1][MBOX_TX]);
> +

As with the error path in probe, this would be cleaner with a loop.

Regards,
Bjorn

RE: [PATCH v1 1/3] remoteproc: st: add virtio communication support

[Loic PALLARDY]

> -----Original Message-----
> From: Bjorn Andersson [mailto:bjorn.andersson@linaro.org]
> Sent: Thursday, January 19, 2017 12:24 AM
> To: Loic PALLARDY <loic.pallardy@st.com>
> Cc: ohad@wizery.com; lee.jones@linaro.org; Patrice CHOTARD
> <patrice.chotard@st.com>; linux-remoteproc@vger.kernel.org;
> kernel@stlinux.com
> Subject: Re: [PATCH v1 1/3] remoteproc: st: add virtio communication
> support
> 
> On Wed 07 Dec 12:33 PST 2016, Loic Pallardy wrote:
> 
> > This patch provides virtio communication support based on mailbox
> > for ST coprocessors.
> >
> > Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
> > ---
> >  drivers/remoteproc/Kconfig         |   3 +
> >  drivers/remoteproc/st_remoteproc.c | 121
> ++++++++++++++++++++++++++++++++++++-
> >  2 files changed, 123 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
> > index 51d7ca0..fd275c0 100644
> > --- a/drivers/remoteproc/Kconfig
> > +++ b/drivers/remoteproc/Kconfig
> > @@ -119,6 +119,9 @@ config ST_REMOTEPROC
> >  	tristate "ST remoteproc support"
> >  	depends on ARCH_STI
> >  	depends on REMOTEPROC
> > +	select MAILBOX
> > +	select STI_MBOX
> > +	select RPMSG_VIRTIO
> >  	help
> >  	  Say y here to support ST's adjunct processors via the remote
> >  	  processor framework.
> > diff --git a/drivers/remoteproc/st_remoteproc.c
> b/drivers/remoteproc/st_remoteproc.c
> > index da4e152..113caf2 100644
> > --- a/drivers/remoteproc/st_remoteproc.c
> > +++ b/drivers/remoteproc/st_remoteproc.c
> > @@ -15,6 +15,7 @@
> >  #include <linux/err.h>
> >  #include <linux/interrupt.h>
> >  #include <linux/kernel.h>
> > +#include <linux/mailbox_client.h>
> >  #include <linux/mfd/syscon.h>
> >  #include <linux/module.h>
> >  #include <linux/of.h>
> > @@ -25,6 +26,17 @@
> >  #include <linux/remoteproc.h>
> >  #include <linux/reset.h>
> >
> > +#include "remoteproc_internal.h"
> > +
> > +#define ST_RPROC_MAX_VRING	2
> > +
> > +#define MBOX_RX			0
> > +#define MBOX_TX			1
> > +#define MBOX_MAX		2
> > +
> > +static struct mbox_client mbox_client_vq0;
> > +static struct mbox_client mbox_client_vq1;
> > +
> >  struct st_rproc_config {
> >  	bool			sw_reset;
> >  	bool			pwr_reset;
> > @@ -39,8 +51,45 @@ struct st_rproc {
> >  	u32			clk_rate;
> >  	struct regmap		*boot_base;
> >  	u32			boot_offset;
> > +	struct mbox_chan
> 	*mbox_chan[ST_RPROC_MAX_VRING][MBOX_MAX];
> >  };
> >
> > +static void st_rproc_mbox_callback(struct device *dev, u32 msg)
> > +{
> > +	struct rproc *rproc = dev_get_drvdata(dev);
> > +
> > +	if (rproc_vq_interrupt(rproc, msg) == IRQ_NONE)
> > +		dev_dbg(dev, "no message was found in vqid %d\n", msg);
> > +}
> > +
> > +static void st_rproc_mbox_callback_vq0(struct mbox_client
> *mbox_client,
> > +				       void *data)
> > +{
> > +	st_rproc_mbox_callback(mbox_client->dev, 0);
> > +}
> > +
> > +static void st_rproc_mbox_callback_vq1(struct mbox_client
> *mbox_client,
> > +				       void *data)
> > +{
> > +	st_rproc_mbox_callback(mbox_client->dev, 1);
> > +}
> > +
> > +static void st_rproc_kick(struct rproc *rproc, int vqid)
> > +{
> > +	struct st_rproc *ddata = rproc->priv;
> > +	struct device *dev = rproc->dev.parent;
> > +	int ret;
> > +
> > +	/* send the index of the triggered virtqueue in the mailbox payload
> */
> > +	if (vqid < ST_RPROC_MAX_VRING) {
> 
> If you flip this condition around and return if vqid >=
> ST_RPROC_MAX_VRING (probably with a WARN_ON() around it) you will
> save
> an indentation level and don't have to wrap the dev_err below.
OK I'll

> 
> > +		ret = mbox_send_message(ddata-
> >mbox_chan[vqid][MBOX_TX],
> > +					(void *)&vqid);
> > +		if (ret < 0)
> > +			dev_err(dev,
> > +				"failed to send message via mbox: %d\n",
> ret);
> > +	}
> > +}
> > +
> >  static int st_rproc_start(struct rproc *rproc)
> >  {
> >  	struct st_rproc *ddata = rproc->priv;
> > @@ -108,6 +157,7 @@ static int st_rproc_stop(struct rproc *rproc)
> >  }
> >
> >  static struct rproc_ops st_rproc_ops = {
> > +	.kick		= st_rproc_kick,
> >  	.start		= st_rproc_start,
> >  	.stop		= st_rproc_stop,
> >  };
> > @@ -221,6 +271,7 @@ static int st_rproc_probe(struct platform_device
> *pdev)
> >  	struct st_rproc *ddata;
> >  	struct device_node *np = dev->of_node;
> >  	struct rproc *rproc;
> > +	struct mbox_chan *chan;
> >  	int enabled;
> >  	int ret;
> >
> > @@ -257,13 +308,76 @@ static int st_rproc_probe(struct platform_device
> *pdev)
> >  		clk_set_rate(ddata->clk, ddata->clk_rate);
> >  	}
> >
> > +	if (of_get_property(np, "mbox-names", NULL)) {
> > +		mbox_client_vq0.dev		= dev;
> > +		mbox_client_vq0.tx_done		= NULL;
> > +		mbox_client_vq0.tx_block	= false;
> > +		mbox_client_vq0.knows_txdone	= false;
> > +		mbox_client_vq0.rx_callback	=
> st_rproc_mbox_callback_vq0;
> > +
> > +		mbox_client_vq1.dev		= dev;
> > +		mbox_client_vq1.tx_done		= NULL;
> > +		mbox_client_vq1.tx_block	= false;
> > +		mbox_client_vq1.knows_txdone	= false;
> > +		mbox_client_vq1.rx_callback	=
> st_rproc_mbox_callback_vq1;
> > +
> 
> As far as I can see these are const (although the mbox api doesn't
> declare the parameter as such), please fill them out as you declare the
> variables outside the function scope.
>
mbox_client can't be const as dev field must be set and is checked by mailbox framework.
Patch v2 puts mbox_client_vq1 and mbox_client_vq2 in struct st_rproc_config to be able to manage several coprocessors with rpmsg link.
 
> > +		/*
> > +		 * To control a co-processor without IPC mechanism.
> > +		 * This driver can be used without mbox and rpmsg.
> > +		 */
> > +		chan = mbox_request_channel_byname(&mbox_client_vq0,
> "vq0_rx");
> > +		if (IS_ERR(chan)) {
> > +			dev_err(&rproc->dev, "failed to request mbox chan
> 0\n");
> > +			ret = PTR_ERR(chan);
> > +			goto free_rproc;
> > +		} else {
> > +			ddata->mbox_chan[0][MBOX_RX] = chan;
> 
> No need to have these in else blocks, as the other code paths ends with
> a goto.
Yes true, I'll change

> 
> > +		}
> > +
> > +		chan = mbox_request_channel_byname(&mbox_client_vq0,
> "vq0_tx");
> > +		if (IS_ERR(chan)) {
> > +			dev_err(&rproc->dev, "failed to request mbox chan
> 0\n");
> > +			ret = PTR_ERR(chan);
> > +			goto free_one;
> > +		} else {
> > +			ddata->mbox_chan[0][MBOX_TX] = chan;
> > +		}
> > +
> > +		chan = mbox_request_channel_byname(&mbox_client_vq1,
> "vq1_rx");
> > +		if (IS_ERR(chan)) {
> > +			dev_err(&rproc->dev, "failed to request mbox chan
> 1\n");
> > +			ret = PTR_ERR(chan);
> > +			goto free_two;
> > +		} else {
> > +			ddata->mbox_chan[1][MBOX_RX] = chan;
> > +		}
> > +
> > +		chan = mbox_request_channel_byname(&mbox_client_vq1,
> "vq1_tx");
> > +		if (IS_ERR(chan)) {
> > +			dev_err(&rproc->dev, "failed to request mbox chan
> 1\n");
> > +			ret = PTR_ERR(chan);
> > +			goto free_three;
> > +		} else {
> > +			ddata->mbox_chan[1][MBOX_TX] = chan;
> > +		}
> > +	}
> > +
> >  	ret = rproc_add(rproc);
> >  	if (ret)
> > -		goto free_rproc;
> > +		goto free_for;
> >
> >  	return 0;
> >
> > +free_for:
> > +	mbox_free_channel(ddata->mbox_chan[1][MBOX_TX]);
> 
> If you flatten mbox_chan to a single-dimensional array you can loop over
> it like:
> 
> for (i = 0; i < 4; i++)
> 	mbox_free_channel(ddata->mbox_chan[i]);
> 
> And you don't need to check if they are still NULL because
> mbox_free_channel() handles that for you.
> 
OK good point, I'll try this

> > +free_three:
> > +	mbox_free_channel(ddata->mbox_chan[1][MBOX_RX]);
> > +free_two:
> > +	mbox_free_channel(ddata->mbox_chan[0][MBOX_TX]);
> > +free_one:
> > +	mbox_free_channel(ddata->mbox_chan[0][MBOX_RX]);
> >  free_rproc:
> > +	clk_unprepare(ddata->clk);
> 
> Nice catch, but I would like to have it in a separate patch - and it
> should not be done when st_rproc_parse_dt() returns an error.

Yes should be in a separate patch
> 
> >  	rproc_free(rproc);
> >  	return ret;
> >  }
> > @@ -279,6 +393,11 @@ static int st_rproc_remove(struct platform_device
> *pdev)
> >
> >  	of_reserved_mem_device_release(&pdev->dev);
> >
> > +	mbox_free_channel(ddata->mbox_chan[0][MBOX_RX]);
> > +	mbox_free_channel(ddata->mbox_chan[1][MBOX_RX]);
> > +	mbox_free_channel(ddata->mbox_chan[0][MBOX_TX]);
> > +	mbox_free_channel(ddata->mbox_chan[1][MBOX_TX]);
> > +
> 
> As with the error path in probe, this would be cleaner with a loop.
OK
Thanks Bjorn for the review, I'll send a v3.
> 
> Regards,
> Bjorn