dmaengine: rcar-dmac: clear channel register when error

dmaengine: rcar-dmac: clear channel register when error

[Kuninori Morimoto]

From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

We need to clear channel register in error case as recovery.
The channel is already stopped in such case, thus we don't need to call
rcar_dmac_chan_halt() before clearing.

rcar_dmac_chan_halt() will clear register and confirm DE bit.
But it will be failed because channel is already stopped in error case.
In other words, we shouldn't call it then.

// This patch started to use C++ style comment out
// because it is recent Linus request

Reported-by: Hiroki Negishi <hiroki.negishi.bx@renesas.com>
Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Reviewed-by: Hiroki Negishi <hiroki.negishi.bx@renesas.com>
---
 drivers/dma/sh/rcar-dmac.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/dma/sh/rcar-dmac.c b/drivers/dma/sh/rcar-dmac.c
index 279c930..35d7a16 100644
--- a/drivers/dma/sh/rcar-dmac.c
+++ b/drivers/dma/sh/rcar-dmac.c
@@ -1525,7 +1525,13 @@ static irqreturn_t rcar_dmac_isr_channel(int irq, void *dev)
 
 	chcr = rcar_dmac_chan_read(chan, RCAR_DMACHCR);
 	if (chcr & RCAR_DMACHCR_CAE) {
-		rcar_dmac_chan_halt(chan);
+		struct rcar_dmac *dmac = dev_get_drvdata(chan->chan.device->dev);
+
+		// We don't need to call rcar_dmac_chan_halt()
+		// because channel is already stopped in error case.
+		// We need to clear register and check DE bit as recovery.
+		rcar_dmac_write(dmac, RCAR_DMACHCLR, 1 << chan->index);
+		rcar_dmac_chcr_de_barrier(chan);
 		reinit = true;
 		goto spin_lock_end;
 	}

dmaengine: rcar-dmac: clear channel register when error

[Fabio Estevam]

Hi Kuninori-san,

On Sun, Jul 1, 2018 at 10:07 PM, Kuninori Morimoto
<kuninori.morimoto.gx@renesas.com> wrote:
>
> From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
>
> We need to clear channel register in error case as recovery.
> The channel is already stopped in such case, thus we don't need to call
> rcar_dmac_chan_halt() before clearing.
>
> rcar_dmac_chan_halt() will clear register and confirm DE bit.
> But it will be failed because channel is already stopped in error case.
> In other words, we shouldn't call it then.
>
> // This patch started to use C++ style comment out
> // because it is recent Linus request

This is valid for the SPDX part only.

For code you should still use the standard /* style.
---
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

dmaengine: rcar-dmac: clear channel register when error

[Kuninori Morimoto]

Hi Fabio

> > // This patch started to use C++ style comment out
> > // because it is recent Linus request
> 
> This is valid for the SPDX part only.
> 
> For code you should still use the standard /* style.

Wow !
Thanks, I will post v2 patch

Best regards
---
Kuninori Morimoto
--
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

dmaengine: rcar-dmac: clear channel register when error

[Laurent Pinchart]

Hi Morimoto-san,

Thank you for the patch.

On Monday, 2 July 2018 04:07:17 EEST Kuninori Morimoto wrote:
> From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> 
> We need to clear channel register in error case as recovery.
> The channel is already stopped in such case, thus we don't need to call
> rcar_dmac_chan_halt() before clearing.
> 
> rcar_dmac_chan_halt() will clear register and confirm DE bit.
> But it will be failed because channel is already stopped in error case.
> In other words, we shouldn't call it then.
> 
> // This patch started to use C++ style comment out
> // because it is recent Linus request

I fear this will generate lots of frustration :-/ While I strongly prefer the 
traditional C style, I'm fine leaving the choice to driver authors. However, I 
think that mixing different styles in the same file only hinders readability. 
Would you like to convert the whole file ? ;-)

> Reported-by: Hiroki Negishi <hiroki.negishi.bx@renesas.com>
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> Reviewed-by: Hiroki Negishi <hiroki.negishi.bx@renesas.com>
> ---
>  drivers/dma/sh/rcar-dmac.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/dma/sh/rcar-dmac.c b/drivers/dma/sh/rcar-dmac.c
> index 279c930..35d7a16 100644
> --- a/drivers/dma/sh/rcar-dmac.c
> +++ b/drivers/dma/sh/rcar-dmac.c
> @@ -1525,7 +1525,13 @@ static irqreturn_t rcar_dmac_isr_channel(int irq,
> void *dev)
> 
>  	chcr = rcar_dmac_chan_read(chan, RCAR_DMACHCR);
>  	if (chcr & RCAR_DMACHCR_CAE) {
> -		rcar_dmac_chan_halt(chan);
> +		struct rcar_dmac *dmac = dev_get_drvdata(chan->chan.device->dev);

This could be simplified with

		struct rcar_dmac *dmac = to_rcar_dmac(chan->chan.device);

> +
> +		// We don't need to call rcar_dmac_chan_halt()
> +		// because channel is already stopped in error case.
> +		// We need to clear register and check DE bit as recovery.

Is it also a request from Linus to wrap text much before the 80 characters 
limit ? :-)

> +		rcar_dmac_write(dmac, RCAR_DMACHCLR, 1 << chan->index);
> +		rcar_dmac_chcr_de_barrier(chan);
>  		reinit = true;
>  		goto spin_lock_end;
>  	}

dmaengine: rcar-dmac: clear channel register when error

[Kuninori Morimoto]

Hi Laurent

> > // This patch started to use C++ style comment out
> > // because it is recent Linus request
> 
> I fear this will generate lots of frustration :-/ While I strongly prefer the 
> traditional C style, I'm fine leaving the choice to driver authors. However, I 
> think that mixing different styles in the same file only hinders readability. 
> Would you like to convert the whole file ? ;-)

I already posted v2 patch ;P

> >  	chcr = rcar_dmac_chan_read(chan, RCAR_DMACHCR);
> >  	if (chcr & RCAR_DMACHCR_CAE) {
> > -		rcar_dmac_chan_halt(chan);
> > +		struct rcar_dmac *dmac = dev_get_drvdata(chan->chan.device->dev);
> 
> This could be simplified with
> 
> 		struct rcar_dmac *dmac = to_rcar_dmac(chan->chan.device);

Nice to know.
Will post v3 patch

Best regards
---
Kuninori Morimoto
--
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html