diff mbox series

[3/9] dmaengine: dw-edma: Add HDMA remote interrupt configuration

Message ID 20230609081654.330857-4-kory.maincent@bootlin.com
State New
Headers
Series Fix support of dw-edma HDMA NATIVE IP in remote setup |

Commit Message

Köry Maincent June 9, 2023, 8:16 a.m. UTC 
  From: Kory Maincent <kory.maincent@bootlin.com>

Only the local interruption was configured, remote interrupt was left
behind. This patch fix it by setting stop and abort remote interrupts when
the DW_EDMA_CHIP_LOCAL flag is not set.

Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
Fixes: e74c39573d35 ("dmaengine: dw-edma: Add support for native HDMA")
---

This patch is fixing a commit which is only in dmaengine tree and not
merged mainline.
---
 drivers/dma/dw-edma/dw-hdma-v0-core.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Serge Semin June 18, 2023, 9:48 p.m. UTC | #1 
On Fri, Jun 09, 2023 at 10:16:48AM +0200, Köry Maincent wrote:
> From: Kory Maincent <kory.maincent@bootlin.com>
> 
> Only the local interruption was configured, remote interrupt was left
> behind. This patch fix it by setting stop and abort remote interrupts when
> the DW_EDMA_CHIP_LOCAL flag is not set.
> 
> Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
> Fixes: e74c39573d35 ("dmaengine: dw-edma: Add support for native HDMA")
> ---
> 
> This patch is fixing a commit which is only in dmaengine tree and not
> merged mainline.
> ---
>  drivers/dma/dw-edma/dw-hdma-v0-core.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/dma/dw-edma/dw-hdma-v0-core.c b/drivers/dma/dw-edma/dw-hdma-v0-core.c
> index da8663f45fdb..7bd1a0f742be 100644
> --- a/drivers/dma/dw-edma/dw-hdma-v0-core.c
> +++ b/drivers/dma/dw-edma/dw-hdma-v0-core.c
> @@ -232,6 +232,8 @@ static void dw_hdma_v0_core_start(struct dw_edma_chunk *chunk, bool first)
>  		tmp = GET_CH_32(dw, chan->dir, chan->id, int_setup) |
>  		      HDMA_V0_STOP_INT_MASK | HDMA_V0_ABORT_INT_MASK |
>  		      HDMA_V0_LOCAL_STOP_INT_EN | HDMA_V0_LOCAL_ABORT_INT_EN;

> +		if (!(dw->chip->flags & DW_EDMA_CHIP_LOCAL))
> +			tmp |= HDMA_V0_REMOTE_STOP_INT_EN | HDMA_V0_REMOTE_ABORT_INT_EN;

Seems reasonable especially seeing there is a code with a similar
semantic in the dw_hdma_v0_core_write_chunk() method.

Reviewed-by: Serge Semin <fancer.lancer@gmail.com>

Just curious whether we really need to have the local IRQs left
enabled for the remote device setup... The only case I have in mind is
that it would be useful to signal a remote end-point host of such
event in some application-specific environment. It sounds exotic but
still possible.

-Serge(y)

>  		SET_CH_32(dw, chan->dir, chan->id, int_setup, tmp);
>  		/* Channel control */
>  		SET_CH_32(dw, chan->dir, chan->id, control1, HDMA_V0_LINKLIST_EN);
> -- 
> 2.25.1
>
Köry Maincent June 19, 2023, 6:16 p.m. UTC | #2 
On Mon, 19 Jun 2023 00:48:00 +0300
Serge Semin <fancer.lancer@gmail.com> wrote:

> Seems reasonable especially seeing there is a code with a similar
> semantic in the dw_hdma_v0_core_write_chunk() method.
> 
> Reviewed-by: Serge Semin <fancer.lancer@gmail.com>
> 
> Just curious whether we really need to have the local IRQs left
> enabled for the remote device setup... The only case I have in mind is
> that it would be useful to signal a remote end-point host of such
> event in some application-specific environment. It sounds exotic but
> still possible.

Thanks for your review.
Yes, we do need to let local IRQs enabled. I have tested to remove them and it
prevent the remote setup to function correctly on my board. Maybe it needs to be
set to know internally when the transfer is done, but it seems weird. I haven't
a full explanation for now.
Serge Semin June 20, 2023, 9:32 a.m. UTC | #3 
On Mon, Jun 19, 2023 at 08:16:47PM +0200, Köry Maincent wrote:
> On Mon, 19 Jun 2023 00:48:00 +0300
> Serge Semin <fancer.lancer@gmail.com> wrote:
> 
> > Seems reasonable especially seeing there is a code with a similar
> > semantic in the dw_hdma_v0_core_write_chunk() method.
> > 
> > Reviewed-by: Serge Semin <fancer.lancer@gmail.com>
> > 
> > Just curious whether we really need to have the local IRQs left
> > enabled for the remote device setup... The only case I have in mind is
> > that it would be useful to signal a remote end-point host of such
> > event in some application-specific environment. It sounds exotic but
> > still possible.
> 
> Thanks for your review.
> Yes, we do need to let local IRQs enabled. I have tested to remove them and it
> prevent the remote setup to function correctly on my board. Maybe it needs to be
> set to know internally when the transfer is done, but it seems weird. I haven't
> a full explanation for now.

Ok. Thanks for checking it out.

-Serge(y)
diff mbox series

Patch

diff --git a/drivers/dma/dw-edma/dw-hdma-v0-core.c b/drivers/dma/dw-edma/dw-hdma-v0-core.c
index da8663f45fdb..7bd1a0f742be 100644
--- a/drivers/dma/dw-edma/dw-hdma-v0-core.c
+++ b/drivers/dma/dw-edma/dw-hdma-v0-core.c
@@ -232,6 +232,8 @@  static void dw_hdma_v0_core_start(struct dw_edma_chunk *chunk, bool first)
 		tmp = GET_CH_32(dw, chan->dir, chan->id, int_setup) |
 		      HDMA_V0_STOP_INT_MASK | HDMA_V0_ABORT_INT_MASK |
 		      HDMA_V0_LOCAL_STOP_INT_EN | HDMA_V0_LOCAL_ABORT_INT_EN;
+		if (!(dw->chip->flags & DW_EDMA_CHIP_LOCAL))
+			tmp |= HDMA_V0_REMOTE_STOP_INT_EN | HDMA_V0_REMOTE_ABORT_INT_EN;
 		SET_CH_32(dw, chan->dir, chan->id, int_setup, tmp);
 		/* Channel control */
 		SET_CH_32(dw, chan->dir, chan->id, control1, HDMA_V0_LINKLIST_EN);