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 |
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 > >
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 >
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
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&sdata=BIipoIgBc5sMahJkz33L5ucqeuHwyYnqg09ornpeLE > 4%3D&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 --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) {
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(-)