diff mbox series

[v2] dmaengine: imx-sdma: fix incorrect conversion to readl_relaxed_poll_timeout_atomic()

Message ID E1helB3-0005d6-7m@rmk-PC.armlinux.org.uk (mailing list archive)
State New, archived
Headers show
Series [v2] dmaengine: imx-sdma: fix incorrect conversion to readl_relaxed_poll_timeout_atomic() | expand

Commit Message

Russell King (Oracle) June 22, 2019, 6:55 p.m. UTC 
When imx-sdma was converted to use readl_relaxed_poll_timeout_atomic(),
the termination condition was inverted.  Fix this.

Fixes: 1d069bfa3c78 ("dmaengine: imx-sdma: ack channel 0 IRQ in the interrupt handler")
Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/dma/imx-sdma.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

Comments

Russell King (Oracle) June 22, 2019, 7:26 p.m. UTC | #1 
Well, this doesn't appear to completely solve the problem either -
one out of four of my platforms still spat out the error (because
the SDMA initialisation can run on a different CPU to that which
receives the interrupt.)

I've thought about using a completion, but that doesn't work either,
because in the case of a single CPU, the interrupts will be masked,
so we can't wait for completion.  I think we need to eliminate that
spinlock around this code.

On Sat, Jun 22, 2019 at 07:55:53PM +0100, Russell King wrote:
> When imx-sdma was converted to use readl_relaxed_poll_timeout_atomic(),
> the termination condition was inverted.  Fix this.
> 
> Fixes: 1d069bfa3c78 ("dmaengine: imx-sdma: ack channel 0 IRQ in the interrupt handler")
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> ---
>  drivers/dma/imx-sdma.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
> index 5f3c1378b90e..c45cbdb09714 100644
> --- a/drivers/dma/imx-sdma.c
> +++ b/drivers/dma/imx-sdma.c
> @@ -655,15 +655,21 @@ static void sdma_enable_channel(struct sdma_engine *sdma, int channel)
>  static int sdma_run_channel0(struct sdma_engine *sdma)
>  {
>  	int ret;
> -	u32 reg;
> +	u32 reg, mask;
> +
> +	// Disable channel 0 interrupt
> +	mask = readl_relaxed(sdma->regs + SDMA_H_INTRMSK);
> +	writel_relaxed(mask & ~1, sdma->regs + SDMA_H_INTRMSK);
>  
>  	sdma_enable_channel(sdma, 0);
>  
> -	ret = readl_relaxed_poll_timeout_atomic(sdma->regs + SDMA_H_STATSTOP,
> -						reg, !(reg & 1), 1, 500);
> +	ret = readl_relaxed_poll_timeout_atomic(sdma->regs + SDMA_H_INTR,
> +						reg, reg & 1, 1, 500);
>  	if (ret)
>  		dev_err(sdma->dev, "Timeout waiting for CH0 ready\n");
>  
> +	writel_relaxed(mask, sdma->regs + SDMA_H_INTRMSK);
> +
>  	/* Set bits of CONFIG register with dynamic context switching */
>  	reg = readl(sdma->regs + SDMA_H_CONFIG);
>  	if ((reg & SDMA_H_CONFIG_CSM) == 0) {
> -- 
> 2.7.4
> 
>
Russell King (Oracle) June 22, 2019, 8:26 p.m. UTC | #2 
On Sat, Jun 22, 2019 at 08:26:53PM +0100, Russell King - ARM Linux admin wrote:
> Well, this doesn't appear to completely solve the problem either -
> one out of four of my platforms still spat out the error (because
> the SDMA initialisation can run on a different CPU to that which
> receives the interrupt.)
> 
> I've thought about using a completion, but that doesn't work either,
> because in the case of a single CPU, the interrupts will be masked,
> so we can't wait for completion.  I think we need to eliminate that
> spinlock around this code.

It looks like iMX6 Dual does not initialise DMA properly using the 1.1
firmware - md5sum is:

5d4584134cc4cba62e1be2f382cd6f3a  /lib/firmware/imx/sdma/sdma-imx6q.bin

I've tried extending the timeout to 5ms, checking HI[0] (both from the
interrupt handler and from sdma_run_channel0() to cover the case of a
single-core setup).

After boot:

 60:          0          0       GPC   2 Level     sdma

So no interrupt was received.  Looking at the registers:

# /shared/bin32/devmem2 0x20ec02c
Value at address 0x020ec02c: 0x00000000  <= H_INTRMASK
# /shared/bin32/devmem2 0x20ec004
Value at address 0x020ec004: 0x00000000  <= H_INTR
# /shared/bin32/devmem2 0x20ec00c
Value at address 0x020ec00c: 0x00000000  <= H_START
# /shared/bin32/devmem2 0x20ec008
Value at address 0x020ec008: 0x00000001  <= H_STATSTOP

Any ideas?

> 
> On Sat, Jun 22, 2019 at 07:55:53PM +0100, Russell King wrote:
> > When imx-sdma was converted to use readl_relaxed_poll_timeout_atomic(),
> > the termination condition was inverted.  Fix this.
> > 
> > Fixes: 1d069bfa3c78 ("dmaengine: imx-sdma: ack channel 0 IRQ in the interrupt handler")
> > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> > ---
> >  drivers/dma/imx-sdma.c | 12 +++++++++---
> >  1 file changed, 9 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
> > index 5f3c1378b90e..c45cbdb09714 100644
> > --- a/drivers/dma/imx-sdma.c
> > +++ b/drivers/dma/imx-sdma.c
> > @@ -655,15 +655,21 @@ static void sdma_enable_channel(struct sdma_engine *sdma, int channel)
> >  static int sdma_run_channel0(struct sdma_engine *sdma)
> >  {
> >  	int ret;
> > -	u32 reg;
> > +	u32 reg, mask;
> > +
> > +	// Disable channel 0 interrupt
> > +	mask = readl_relaxed(sdma->regs + SDMA_H_INTRMSK);
> > +	writel_relaxed(mask & ~1, sdma->regs + SDMA_H_INTRMSK);
> >  
> >  	sdma_enable_channel(sdma, 0);
> >  
> > -	ret = readl_relaxed_poll_timeout_atomic(sdma->regs + SDMA_H_STATSTOP,
> > -						reg, !(reg & 1), 1, 500);
> > +	ret = readl_relaxed_poll_timeout_atomic(sdma->regs + SDMA_H_INTR,
> > +						reg, reg & 1, 1, 500);
> >  	if (ret)
> >  		dev_err(sdma->dev, "Timeout waiting for CH0 ready\n");
> >  
> > +	writel_relaxed(mask, sdma->regs + SDMA_H_INTRMSK);
> > +
> >  	/* Set bits of CONFIG register with dynamic context switching */
> >  	reg = readl(sdma->regs + SDMA_H_CONFIG);
> >  	if ((reg & SDMA_H_CONFIG_CSM) == 0) {
> > -- 
> > 2.7.4
> > 
> > 
> 
> -- 
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
> According to speedtest.net: 11.9Mbps down 500kbps up
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
Fabio Estevam June 23, 2019, 1:29 p.m. UTC | #3 
Hi Russell,

On Sat, Jun 22, 2019 at 5:27 PM Russell King - ARM Linux admin
<linux@armlinux.org.uk> wrote:
>
> On Sat, Jun 22, 2019 at 08:26:53PM +0100, Russell King - ARM Linux admin wrote:
> > Well, this doesn't appear to completely solve the problem either -
> > one out of four of my platforms still spat out the error (because
> > the SDMA initialisation can run on a different CPU to that which
> > receives the interrupt.)
> >
> > I've thought about using a completion, but that doesn't work either,
> > because in the case of a single CPU, the interrupts will be masked,
> > so we can't wait for completion.  I think we need to eliminate that
> > spinlock around this code.
>
> It looks like iMX6 Dual does not initialise DMA properly using the 1.1
> firmware - md5sum is:
>
> 5d4584134cc4cba62e1be2f382cd6f3a  /lib/firmware/imx/sdma/sdma-imx6q.bin
>
> I've tried extending the timeout to 5ms, checking HI[0] (both from the
> interrupt handler and from sdma_run_channel0() to cover the case of a
> single-core setup).
>
> After boot:
>
>  60:          0          0       GPC   2 Level     sdma
>
> So no interrupt was received.  Looking at the registers:
>
> # /shared/bin32/devmem2 0x20ec02c
> Value at address 0x020ec02c: 0x00000000  <= H_INTRMASK
> # /shared/bin32/devmem2 0x20ec004
> Value at address 0x020ec004: 0x00000000  <= H_INTR
> # /shared/bin32/devmem2 0x20ec00c
> Value at address 0x020ec00c: 0x00000000  <= H_START
> # /shared/bin32/devmem2 0x20ec008
> Value at address 0x020ec008: 0x00000001  <= H_STATSTOP
>
> Any ideas?

Could you please try this patch from Robin?
http://lists.infradead.org/pipermail/linux-arm-kernel/2019-June/661914.html

Thanks
Robin Gong June 25, 2019, 9 a.m. UTC | #4 
On Sun, Jun 23, 2019 at 21:30 Fabio Estevam <festevam@gmail.com> wrote:
> Hi Russell,
> 
> On Sat, Jun 22, 2019 at 5:27 PM Russell King - ARM Linux admin
> <linux@armlinux.org.uk> wrote:
> >
> > On Sat, Jun 22, 2019 at 08:26:53PM +0100, Russell King - ARM Linux admin
> wrote:
> > > Well, this doesn't appear to completely solve the problem either -
> > > one out of four of my platforms still spat out the error (because
> > > the SDMA initialisation can run on a different CPU to that which
> > > receives the interrupt.)
> > >
> > > I've thought about using a completion, but that doesn't work either,
> > > because in the case of a single CPU, the interrupts will be masked,
> > > so we can't wait for completion.  I think we need to eliminate that
> > > spinlock around this code.
> >
> > It looks like iMX6 Dual does not initialise DMA properly using the 1.1
> > firmware - md5sum is:
> >
> > 5d4584134cc4cba62e1be2f382cd6f3a
> > /lib/firmware/imx/sdma/sdma-imx6q.bin
> >
> > I've tried extending the timeout to 5ms, checking HI[0] (both from the
> > interrupt handler and from sdma_run_channel0() to cover the case of a
> > single-core setup).
> >
> > After boot:
> >
> >  60:          0          0       GPC   2 Level     sdma
> >
> > So no interrupt was received.  Looking at the registers:
> >
> > # /shared/bin32/devmem2 0x20ec02c
> > Value at address 0x020ec02c: 0x00000000  <= H_INTRMASK #
> > /shared/bin32/devmem2 0x20ec004 Value at address 0x020ec004:
> > 0x00000000  <= H_INTR # /shared/bin32/devmem2 0x20ec00c Value at
> > address 0x020ec00c: 0x00000000  <= H_START # /shared/bin32/devmem2
> > 0x20ec008 Value at address 0x020ec008: 0x00000001  <= H_STATSTOP
> >
> > Any ideas?
Seems sdma script not run as expected, thus no DONE instruction involved to clear
'HE' of H_STATSTOP and notify ARM by interrupt. So this timeout happened during the
first ' sdma_load_script()' phase ?


> Could you please try this patch from Robin?
> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Flists.infra
> dead.org%2Fpipermail%2Flinux-arm-kernel%2F2019-June%2F661914.html&a
> mp;data=02%7C01%7Cyibin.gong%40nxp.com%7C7faa18517626429780d908
> d6f7ded3b6%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636968
> 933747699843&amp;sdata=BIipoIgBc5sMahJkz33L5ucqeuHwyYnqg09ornpeLE
> 4%3D&amp;reserved=0
This should be the different case, since in Russell King's case, no any interrupt while my patch
fix the potential interrupt storm.
> 
> Thanks
diff mbox series

Patch

diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
index 5f3c1378b90e..c45cbdb09714 100644
--- a/drivers/dma/imx-sdma.c
+++ b/drivers/dma/imx-sdma.c
@@ -655,15 +655,21 @@  static void sdma_enable_channel(struct sdma_engine *sdma, int channel)
 static int sdma_run_channel0(struct sdma_engine *sdma)
 {
 	int ret;
-	u32 reg;
+	u32 reg, mask;
+
+	// Disable channel 0 interrupt
+	mask = readl_relaxed(sdma->regs + SDMA_H_INTRMSK);
+	writel_relaxed(mask & ~1, sdma->regs + SDMA_H_INTRMSK);
 
 	sdma_enable_channel(sdma, 0);
 
-	ret = readl_relaxed_poll_timeout_atomic(sdma->regs + SDMA_H_STATSTOP,
-						reg, !(reg & 1), 1, 500);
+	ret = readl_relaxed_poll_timeout_atomic(sdma->regs + SDMA_H_INTR,
+						reg, reg & 1, 1, 500);
 	if (ret)
 		dev_err(sdma->dev, "Timeout waiting for CH0 ready\n");
 
+	writel_relaxed(mask, sdma->regs + SDMA_H_INTRMSK);
+
 	/* Set bits of CONFIG register with dynamic context switching */
 	reg = readl(sdma->regs + SDMA_H_CONFIG);
 	if ((reg & SDMA_H_CONFIG_CSM) == 0) {