[PATCH] remoteproc: imx_rproc: iterate all notifiyids in rx callback

[PATCH] remoteproc: imx_rproc: iterate all notifiyids in rx callback

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>

The current code has an assumption that there is one tx and one rx
vring, but it is not always true. There maybe more vrings. So iterate
all notifyids to not miss any vring events.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/remoteproc/imx_rproc.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
index f9874fc5a80f..e3f40d0e9f3d 100644
--- a/drivers/remoteproc/imx_rproc.c
+++ b/drivers/remoteproc/imx_rproc.c
@@ -725,13 +725,23 @@ static int imx_rproc_addr_init(struct imx_rproc *priv,
 	return 0;
 }
 
+static int imx_rproc_notified_idr_cb(int id, void *ptr, void *data)
+{
+	struct rproc *rproc = data;
+
+	if (rproc_vq_interrupt(rproc, id) == IRQ_NONE)
+		dev_dbg(&rproc->dev, "no message in vqid: %d\n", id);
+
+	return 0;
+}
+
 static void imx_rproc_vq_work(struct work_struct *work)
 {
 	struct imx_rproc *priv = container_of(work, struct imx_rproc,
 					      rproc_work);
+	struct rproc *rproc = priv->rproc;
 
-	rproc_vq_interrupt(priv->rproc, 0);
-	rproc_vq_interrupt(priv->rproc, 1);
+	idr_for_each(&rproc->notifyids, imx_rproc_notified_idr_cb, rproc);
 }
 
 static void imx_rproc_rx_callback(struct mbox_client *cl, void *msg)
-- 
2.37.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] remoteproc: imx_rproc: iterate all notifiyids in rx callback

[Mathieu Poirier]

On Sun, Jun 25, 2023 at 08:35:14PM +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> The current code has an assumption that there is one tx and one rx
> vring, but it is not always true. There maybe more vrings. So iterate
> all notifyids to not miss any vring events.

Can you be more specific on the use case where more than 2 virqueues are
allocated?  The remoteproc core can handle more than 2 but right now the only
configuration I see doesn't support more than that.

> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  drivers/remoteproc/imx_rproc.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
> index f9874fc5a80f..e3f40d0e9f3d 100644
> --- a/drivers/remoteproc/imx_rproc.c
> +++ b/drivers/remoteproc/imx_rproc.c
> @@ -725,13 +725,23 @@ static int imx_rproc_addr_init(struct imx_rproc *priv,
>  	return 0;
>  }
>  
> +static int imx_rproc_notified_idr_cb(int id, void *ptr, void *data)
> +{
> +	struct rproc *rproc = data;
> +
> +	if (rproc_vq_interrupt(rproc, id) == IRQ_NONE)
> +		dev_dbg(&rproc->dev, "no message in vqid: %d\n", id);
> +
> +	return 0;
> +}
> +
>  static void imx_rproc_vq_work(struct work_struct *work)
>  {
>  	struct imx_rproc *priv = container_of(work, struct imx_rproc,
>  					      rproc_work);
> +	struct rproc *rproc = priv->rproc;
>  
> -	rproc_vq_interrupt(priv->rproc, 0);
> -	rproc_vq_interrupt(priv->rproc, 1);
> +	idr_for_each(&rproc->notifyids, imx_rproc_notified_idr_cb, rproc);
>  }
>  
>  static void imx_rproc_rx_callback(struct mbox_client *cl, void *msg)
> -- 
> 2.37.1
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH] remoteproc: imx_rproc: iterate all notifiyids in rx callback

[Peng Fan]

> Subject: Re: [PATCH] remoteproc: imx_rproc: iterate all notifiyids in rx
> callback
> 
> On Sun, Jun 25, 2023 at 08:35:14PM +0800, Peng Fan (OSS) wrote:
> > From: Peng Fan <peng.fan@nxp.com>
> >
> > The current code has an assumption that there is one tx and one rx
> > vring, but it is not always true. There maybe more vrings. So iterate
> > all notifyids to not miss any vring events.
> 
> Can you be more specific on the use case where more than 2 virqueues are
> allocated?  The remoteproc core can handle more than 2 but right now the
> only configuration I see doesn't support more than that.

In downstream tree, we have below remoteproc node. It use
vdev0 vring0/vring1 for vdev0, vdev1 vring0/vring1 for vdev1.
vdev0 and vdev1 are for different services, saying vdev0 for gpio rpmsg,
vdev1 for i2c rpmsg.
	cm33: imx93-cm33 {
		compatible = "fsl,imx93-cm33";
		mbox-names = "tx", "rx", "rxdb";
		mboxes = <&mu1 0 1
			  &mu1 1 1
			  &mu1 3 1>;
		memory-region = <&vdevbuffer>, <&vdev0vring0>, <&vdev0vring1>,
				<&vdev1vring0>, <&vdev1vring1>, <&rsc_table>;
		fsl,startup-delay-ms = <500>;
	};

Thanks,
Peng.

> 
> >
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > ---
> >  drivers/remoteproc/imx_rproc.c | 14 ++++++++++++--
> >  1 file changed, 12 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/remoteproc/imx_rproc.c
> > b/drivers/remoteproc/imx_rproc.c index f9874fc5a80f..e3f40d0e9f3d
> > 100644
> > --- a/drivers/remoteproc/imx_rproc.c
> > +++ b/drivers/remoteproc/imx_rproc.c
> > @@ -725,13 +725,23 @@ static int imx_rproc_addr_init(struct imx_rproc
> *priv,
> >  	return 0;
> >  }
> >
> > +static int imx_rproc_notified_idr_cb(int id, void *ptr, void *data) {
> > +	struct rproc *rproc = data;
> > +
> > +	if (rproc_vq_interrupt(rproc, id) == IRQ_NONE)
> > +		dev_dbg(&rproc->dev, "no message in vqid: %d\n", id);
> > +
> > +	return 0;
> > +}
> > +
> >  static void imx_rproc_vq_work(struct work_struct *work)  {
> >  	struct imx_rproc *priv = container_of(work, struct imx_rproc,
> >  					      rproc_work);
> > +	struct rproc *rproc = priv->rproc;
> >
> > -	rproc_vq_interrupt(priv->rproc, 0);
> > -	rproc_vq_interrupt(priv->rproc, 1);
> > +	idr_for_each(&rproc->notifyids, imx_rproc_notified_idr_cb, rproc);
> >  }
> >
> >  static void imx_rproc_rx_callback(struct mbox_client *cl, void *msg)
> > --
> > 2.37.1
> >

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] remoteproc: imx_rproc: iterate all notifiyids in rx callback

[Mathieu Poirier]

On Tue, 27 Jun 2023 at 18:55, Peng Fan <peng.fan@nxp.com> wrote:
>
> > Subject: Re: [PATCH] remoteproc: imx_rproc: iterate all notifiyids in rx
> > callback
> >
> > On Sun, Jun 25, 2023 at 08:35:14PM +0800, Peng Fan (OSS) wrote:
> > > From: Peng Fan <peng.fan@nxp.com>
> > >
> > > The current code has an assumption that there is one tx and one rx
> > > vring, but it is not always true. There maybe more vrings. So iterate
> > > all notifyids to not miss any vring events.
> >
> > Can you be more specific on the use case where more than 2 virqueues are
> > allocated?  The remoteproc core can handle more than 2 but right now the
> > only configuration I see doesn't support more than that.
>
> In downstream tree, we have below remoteproc node. It use
> vdev0 vring0/vring1 for vdev0, vdev1 vring0/vring1 for vdev1.
> vdev0 and vdev1 are for different services, saying vdev0 for gpio rpmsg,
> vdev1 for i2c rpmsg.

So you are talking about cases where more than one vdev are
instantiated and a single callback channel is available.  Please fix
your changelog description.  The way it is currently written one can
easily think you are referring to more than 2 virtqueues for each
vdev.

One more comment below.

>         cm33: imx93-cm33 {
>                 compatible = "fsl,imx93-cm33";
>                 mbox-names = "tx", "rx", "rxdb";
>                 mboxes = <&mu1 0 1
>                           &mu1 1 1
>                           &mu1 3 1>;
>                 memory-region = <&vdevbuffer>, <&vdev0vring0>, <&vdev0vring1>,
>                                 <&vdev1vring0>, <&vdev1vring1>, <&rsc_table>;
>                 fsl,startup-delay-ms = <500>;
>         };
>
> Thanks,
> Peng.
>
> >
> > >
> > > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > > ---
> > >  drivers/remoteproc/imx_rproc.c | 14 ++++++++++++--
> > >  1 file changed, 12 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/remoteproc/imx_rproc.c
> > > b/drivers/remoteproc/imx_rproc.c index f9874fc5a80f..e3f40d0e9f3d
> > > 100644
> > > --- a/drivers/remoteproc/imx_rproc.c
> > > +++ b/drivers/remoteproc/imx_rproc.c
> > > @@ -725,13 +725,23 @@ static int imx_rproc_addr_init(struct imx_rproc
> > *priv,
> > >     return 0;
> > >  }
> > >
> > > +static int imx_rproc_notified_idr_cb(int id, void *ptr, void *data) {
> > > +   struct rproc *rproc = data;
> > > +
> > > +   if (rproc_vq_interrupt(rproc, id) == IRQ_NONE)
> > > +           dev_dbg(&rproc->dev, "no message in vqid: %d\n", id);
> > > +

A debug message is already displayed by vring_interrupt(), please remove.

Thanks,
Mathieu

> > > +   return 0;
> > > +}
> > > +
> > >  static void imx_rproc_vq_work(struct work_struct *work)  {
> > >     struct imx_rproc *priv = container_of(work, struct imx_rproc,
> > >                                           rproc_work);
> > > +   struct rproc *rproc = priv->rproc;
> > >
> > > -   rproc_vq_interrupt(priv->rproc, 0);
> > > -   rproc_vq_interrupt(priv->rproc, 1);
> > > +   idr_for_each(&rproc->notifyids, imx_rproc_notified_idr_cb, rproc);
> > >  }
> > >
> > >  static void imx_rproc_rx_callback(struct mbox_client *cl, void *msg)
> > > --
> > > 2.37.1
> > >

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel