[PATCH 0/4] dma: mxs-dma: several bugfixes for the MXS DMA driver

[PATCH 0/4] dma: mxs-dma: several bugfixes for the MXS DMA driver

[Lothar Waßmann]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 402 bytes --]

The following patchset fixes some bugs in the MXS DMA driver.

Lothar Waßmann (4):
  fix a typo in comment
  Always leave mxs_dma_init() with the clock disabled.
  Using a static variable for counting the number of CCWs attached to
  Don't use CLKGATE to disable DMA clock.

 drivers/dma/mxs-dma.c |   43 ++++++++-----------------------------------
 1 files changed, 8 insertions(+), 35 deletions(-)

[PATCH 1/4] dma: mxs-dma: fix a typo in comment

[Lothar Waßmann]

Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
---
 drivers/dma/mxs-dma.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/dma/mxs-dma.c b/drivers/dma/mxs-dma.c
index b4588bd..f228f55 100644
--- a/drivers/dma/mxs-dma.c
+++ b/drivers/dma/mxs-dma.c
@@ -268,7 +268,7 @@ static irqreturn_t mxs_dma_int_handler(int irq, void *dev_id)
 	/*
 	 * When both completion and error of termination bits set at the
 	 * same time, we do not take it as an error.  IOW, it only becomes
-	 * an error we need to handler here in case of ether it's (1) an bus
+	 * an error we need to handle here in case of either it's (1) a bus
 	 * error or (2) a termination error with no completion.
 	 */
 	stat2 = ((stat2 >> MXS_DMA_CHANNELS) & stat2) | /* (1) */
-- 
1.5.6.5

[PATCH 2/4] dma: mxs-dma: Always leave mxs_dma_init() with the clock disabled.

[Lothar Waßmann]

There is no need to have the clock enabled all the time the driver is
loaded.
It will be enabled anyway in mxs_dma_alloc_chan_resources() when a
channel is actually going to be used.

Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
---
 drivers/dma/mxs-dma.c |    7 ++-----
 1 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/dma/mxs-dma.c b/drivers/dma/mxs-dma.c
index f228f55..c7574e8 100644
--- a/drivers/dma/mxs-dma.c
+++ b/drivers/dma/mxs-dma.c
@@ -580,7 +580,7 @@ static int __init mxs_dma_init(struct mxs_dma_engine *mxs_dma)
 
 	ret = clk_enable(mxs_dma->clk);
 	if (ret)
-		goto err_out;
+		return ret;
 
 	ret = mxs_reset_block(mxs_dma->base);
 	if (ret)
@@ -604,11 +604,8 @@ static int __init mxs_dma_init(struct mxs_dma_engine *mxs_dma)
 	writel(MXS_DMA_CHANNELS_MASK << MXS_DMA_CHANNELS,
 		mxs_dma->base + HW_APBHX_CTRL1 + MXS_SET_ADDR);
 
-	clk_disable(mxs_dma->clk);
-
-	return 0;
-
 err_out:
+	clk_disable(mxs_dma->clk);
 	return ret;
 }
 
-- 
1.5.6.5

[PATCH 3/4] dma: mxs-dma: make mxs_dma_prep_slave_sg() multi user safe

[Lothar Waßmann]

Using a static variable for counting the number of CCWs attached to
a DMA channel when appending a new descriptor is not multi user safe.

Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
---
 drivers/dma/mxs-dma.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/drivers/dma/mxs-dma.c b/drivers/dma/mxs-dma.c
index c7574e8..78336b0 100644
--- a/drivers/dma/mxs-dma.c
+++ b/drivers/dma/mxs-dma.c
@@ -111,6 +111,7 @@ struct mxs_dma_chan {
 	int				chan_irq;
 	struct mxs_dma_ccw		*ccw;
 	dma_addr_t			ccw_phys;
+	int				desc_count;
 	dma_cookie_t			last_completed;
 	enum dma_status			status;
 	unsigned int			flags;
@@ -386,7 +387,7 @@ static struct dma_async_tx_descriptor *mxs_dma_prep_slave_sg(
 	struct scatterlist *sg;
 	int i, j;
 	u32 *pio;
-	static int idx;
+	int idx = append ? mxs_chan->desc_count : 0;
 
 	if (mxs_chan->status == DMA_IN_PROGRESS && !append)
 		return NULL;
@@ -462,6 +463,7 @@ static struct dma_async_tx_descriptor *mxs_dma_prep_slave_sg(
 			}
 		}
 	}
+	mxs_chan->desc_count = idx;
 
 	return &mxs_chan->desc;
 
@@ -523,6 +525,7 @@ static struct dma_async_tx_descriptor *mxs_dma_prep_dma_cyclic(
 
 		i++;
 	}
+	mxs_chan->desc_count = i;
 
 	return &mxs_chan->desc;
 
-- 
1.5.6.5

Re: [PATCH 3/4] dma: mxs-dma: make mxs_dma_prep_slave_sg() multi user safe

[Huang Shijie]

于 2011年12月08日 16:15, Lothar Waßmann 写道:
> Using a static variable for counting the number of CCWs attached to
> a DMA channel when appending a new descriptor is not multi user safe.
>
> Signed-off-by: Lothar Waßmann<LW@KARO-electronics.de>
> ---
>   drivers/dma/mxs-dma.c |    5 ++++-
>   1 files changed, 4 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/dma/mxs-dma.c b/drivers/dma/mxs-dma.c
> index c7574e8..78336b0 100644
> --- a/drivers/dma/mxs-dma.c
> +++ b/drivers/dma/mxs-dma.c
> @@ -111,6 +111,7 @@ struct mxs_dma_chan {
>   	int				chan_irq;
>   	struct mxs_dma_ccw		*ccw;
>   	dma_addr_t			ccw_phys;
> +	int				desc_count;
>   	dma_cookie_t			last_completed;
>   	enum dma_status			status;
>   	unsigned int			flags;
> @@ -386,7 +387,7 @@ static struct dma_async_tx_descriptor *mxs_dma_prep_slave_sg(
>   	struct scatterlist *sg;
>   	int i, j;
>   	u32 *pio;
> -	static int idx;
> +	int idx = append ? mxs_chan->desc_count : 0;
>
>   	if (mxs_chan->status == DMA_IN_PROGRESS&&  !append)
>   		return NULL;
> @@ -462,6 +463,7 @@ static struct dma_async_tx_descriptor *mxs_dma_prep_slave_sg(
>   			}
>   		}
>   	}
> +	mxs_chan->desc_count = idx;
>
>   	return&mxs_chan->desc;
>
> @@ -523,6 +525,7 @@ static struct dma_async_tx_descriptor *mxs_dma_prep_dma_cyclic(
>
>   		i++;
>   	}
> +	mxs_chan->desc_count = i;
>
>   	return&mxs_chan->desc;
>
I like this patch.

[PATCH 4/4] dma: mxs-dma: Don't use CLKGATE bits in CTRL0 to disable DMA channels

[Lothar Waßmann]

This is how the original Freescale code (unintentionally) worked,
because the code path which would have asserted the CLKGATE bit was
never actually reached in their code.
This fixes the nefarious "DMA timout" bug when multiple DMA channels
(e.g. GPMI NAND and MMC) are used at the same time.
If a better fix for this problem should be found, the clkgate handling
could be reinstated.
See http://lists.infradead.org/pipermail/linux-arm-kernel/2011-September/065228.html

Also reverse the order of mxs_dma_disable_chan() and
mxs_dma_reset_chan() in mxs_dma_control() because mxs_dma_reset_chan()
can only work when the DMA channel is enabled.

Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
---
 drivers/dma/mxs-dma.c |   29 +----------------------------
 1 files changed, 1 insertions(+), 28 deletions(-)

diff --git a/drivers/dma/mxs-dma.c b/drivers/dma/mxs-dma.c
index 78336b0..71ba93a 100644
--- a/drivers/dma/mxs-dma.c
+++ b/drivers/dma/mxs-dma.c
@@ -44,7 +44,6 @@
 #define HW_APBHX_CTRL0				0x000
 #define BM_APBH_CTRL0_APB_BURST8_EN		(1 << 29)
 #define BM_APBH_CTRL0_APB_BURST_EN		(1 << 28)
-#define BP_APBH_CTRL0_CLKGATE_CHANNEL		8
 #define BP_APBH_CTRL0_RESET_CHANNEL		16
 #define HW_APBHX_CTRL1				0x010
 #define HW_APBHX_CTRL2				0x020
@@ -131,23 +130,6 @@ struct mxs_dma_engine {
 	struct mxs_dma_chan		mxs_chans[MXS_DMA_CHANNELS];
 };
 
-static inline void mxs_dma_clkgate(struct mxs_dma_chan *mxs_chan, int enable)
-{
-	struct mxs_dma_engine *mxs_dma = mxs_chan->mxs_dma;
-	int chan_id = mxs_chan->chan.chan_id;
-	int set_clr = enable ? MXS_CLR_ADDR : MXS_SET_ADDR;
-
-	/* enable apbh channel clock */
-	if (dma_is_apbh()) {
-		if (apbh_is_old())
-			writel(1 << (chan_id + BP_APBH_CTRL0_CLKGATE_CHANNEL),
-				mxs_dma->base + HW_APBHX_CTRL0 + set_clr);
-		else
-			writel(1 << chan_id,
-				mxs_dma->base + HW_APBHX_CTRL0 + set_clr);
-	}
-}
-
 static void mxs_dma_reset_chan(struct mxs_dma_chan *mxs_chan)
 {
 	struct mxs_dma_engine *mxs_dma = mxs_chan->mxs_dma;
@@ -166,9 +148,6 @@ static void mxs_dma_enable_chan(struct mxs_dma_chan *mxs_chan)
 	struct mxs_dma_engine *mxs_dma = mxs_chan->mxs_dma;
 	int chan_id = mxs_chan->chan.chan_id;
 
-	/* clkgate needs to be enabled before writing other registers */
-	mxs_dma_clkgate(mxs_chan, 1);
-
 	/* set cmd_addr up */
 	writel(mxs_chan->ccw_phys,
 		mxs_dma->base + HW_APBHX_CHn_NXTCMDAR(chan_id));
@@ -179,9 +158,6 @@ static void mxs_dma_enable_chan(struct mxs_dma_chan *mxs_chan)
 
 static void mxs_dma_disable_chan(struct mxs_dma_chan *mxs_chan)
 {
-	/* disable apbh channel clock */
-	mxs_dma_clkgate(mxs_chan, 0);
-
 	mxs_chan->status = DMA_SUCCESS;
 }
 
@@ -339,10 +315,7 @@ static int mxs_dma_alloc_chan_resources(struct dma_chan *chan)
 	if (ret)
 		goto err_clk;
 
-	/* clkgate needs to be enabled for reset to finish */
-	mxs_dma_clkgate(mxs_chan, 1);
 	mxs_dma_reset_chan(mxs_chan);
-	mxs_dma_clkgate(mxs_chan, 0);
 
 	dma_async_tx_descriptor_init(&mxs_chan->desc, chan);
 	mxs_chan->desc.tx_submit = mxs_dma_tx_submit;
@@ -542,8 +515,8 @@ static int mxs_dma_control(struct dma_chan *chan, enum dma_ctrl_cmd cmd,
 
 	switch (cmd) {
 	case DMA_TERMINATE_ALL:
-		mxs_dma_disable_chan(mxs_chan);
 		mxs_dma_reset_chan(mxs_chan);
+		mxs_dma_disable_chan(mxs_chan);
 		break;
 	case DMA_PAUSE:
 		mxs_dma_pause_chan(mxs_chan);
-- 
1.5.6.5

Re: [PATCH 4/4] dma: mxs-dma: Don't use CLKGATE bits in CTRL0 to disable DMA channels

[Huang Shijie]

于 2011年12月08日 16:15, Lothar Waßmann 写道:
> This is how the original Freescale code (unintentionally) worked,
> because the code path which would have asserted the CLKGATE bit was
> never actually reached in their code.
> This fixes the nefarious "DMA timout" bug when multiple DMA channels
> (e.g. GPMI NAND and MMC) are used at the same time.
> If a better fix for this problem should be found, the clkgate handling
> could be reinstated.
> See http://lists.infradead.org/pipermail/linux-arm-kernel/2011-September/065228.html
>
> Also reverse the order of mxs_dma_disable_chan() and
> mxs_dma_reset_chan() in mxs_dma_control() because mxs_dma_reset_chan()
> can only work when the DMA channel is enabled.
>
> Signed-off-by: Lothar Waßmann<LW@KARO-electronics.de>
> ---
>   drivers/dma/mxs-dma.c |   29 +----------------------------
>   1 files changed, 1 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/dma/mxs-dma.c b/drivers/dma/mxs-dma.c
> index 78336b0..71ba93a 100644
> --- a/drivers/dma/mxs-dma.c
> +++ b/drivers/dma/mxs-dma.c
> @@ -44,7 +44,6 @@
>   #define HW_APBHX_CTRL0				0x000
>   #define BM_APBH_CTRL0_APB_BURST8_EN		(1<<  29)
>   #define BM_APBH_CTRL0_APB_BURST_EN		(1<<  28)
> -#define BP_APBH_CTRL0_CLKGATE_CHANNEL		8
>   #define BP_APBH_CTRL0_RESET_CHANNEL		16
>   #define HW_APBHX_CTRL1				0x010
>   #define HW_APBHX_CTRL2				0x020
> @@ -131,23 +130,6 @@ struct mxs_dma_engine {
>   	struct mxs_dma_chan		mxs_chans[MXS_DMA_CHANNELS];
>   };
>
> -static inline void mxs_dma_clkgate(struct mxs_dma_chan *mxs_chan, int enable)
> -{
> -	struct mxs_dma_engine *mxs_dma = mxs_chan->mxs_dma;
> -	int chan_id = mxs_chan->chan.chan_id;
> -	int set_clr = enable ? MXS_CLR_ADDR : MXS_SET_ADDR;
> -
> -	/* enable apbh channel clock */
> -	if (dma_is_apbh()) {
> -		if (apbh_is_old())
> -			writel(1<<  (chan_id + BP_APBH_CTRL0_CLKGATE_CHANNEL),
> -				mxs_dma->base + HW_APBHX_CTRL0 + set_clr);
> -		else
> -			writel(1<<  chan_id,
> -				mxs_dma->base + HW_APBHX_CTRL0 + set_clr);
> -	}
> -}
> -
>   static void mxs_dma_reset_chan(struct mxs_dma_chan *mxs_chan)
>   {
>   	struct mxs_dma_engine *mxs_dma = mxs_chan->mxs_dma;
> @@ -166,9 +148,6 @@ static void mxs_dma_enable_chan(struct mxs_dma_chan *mxs_chan)
>   	struct mxs_dma_engine *mxs_dma = mxs_chan->mxs_dma;
>   	int chan_id = mxs_chan->chan.chan_id;
>
> -	/* clkgate needs to be enabled before writing other registers */
> -	mxs_dma_clkgate(mxs_chan, 1);
> -
>   	/* set cmd_addr up */
>   	writel(mxs_chan->ccw_phys,
>   		mxs_dma->base + HW_APBHX_CHn_NXTCMDAR(chan_id));
> @@ -179,9 +158,6 @@ static void mxs_dma_enable_chan(struct mxs_dma_chan *mxs_chan)
>
>   static void mxs_dma_disable_chan(struct mxs_dma_chan *mxs_chan)
>   {
> -	/* disable apbh channel clock */
> -	mxs_dma_clkgate(mxs_chan, 0);
> -
>   	mxs_chan->status = DMA_SUCCESS;
>   }
>
> @@ -339,10 +315,7 @@ static int mxs_dma_alloc_chan_resources(struct dma_chan *chan)
>   	if (ret)
>   		goto err_clk;
>
> -	/* clkgate needs to be enabled for reset to finish */
> -	mxs_dma_clkgate(mxs_chan, 1);
>   	mxs_dma_reset_chan(mxs_chan);
> -	mxs_dma_clkgate(mxs_chan, 0);
>
>   	dma_async_tx_descriptor_init(&mxs_chan->desc, chan);
>   	mxs_chan->desc.tx_submit = mxs_dma_tx_submit;
> @@ -542,8 +515,8 @@ static int mxs_dma_control(struct dma_chan *chan, enum dma_ctrl_cmd cmd,
>
>   	switch (cmd) {
>   	case DMA_TERMINATE_ALL:
> -		mxs_dma_disable_chan(mxs_chan);
>   		mxs_dma_reset_chan(mxs_chan);
> +		mxs_dma_disable_chan(mxs_chan);
>   		break;
>   	case DMA_PAUSE:
>   		mxs_dma_pause_chan(mxs_chan);
I can test it.

Shawn, do you have any opnion?

Re: [PATCH 4/4] dma: mxs-dma: Don't use CLKGATE bits in CTRL0 to disable DMA channels

[Huang Shijie]

Hi,

[1] i tested the cmd:
  #dd if=/dev/mtd0 > /dev/null & dd if=/dev/mmcblk0 > /dev/null

The `dd if=/dev/mtd0 > /dev/null`  finished successfully;
but `dd if=/dev/mmcblk0 > /dev/null ` will failed.

[2] So i only tested the SD card with:
   #dd if=/dev/mmcblk0 > /dev/null

   it failed too.

[3] I think you have fixed the bug.

Huang Shijie








> This is how the original Freescale code (unintentionally) worked,
> because the code path which would have asserted the CLKGATE bit was
> never actually reached in their code.
> This fixes the nefarious "DMA timout" bug when multiple DMA channels
> (e.g. GPMI NAND and MMC) are used at the same time.
> If a better fix for this problem should be found, the clkgate handling
> could be reinstated.
> See http://lists.infradead.org/pipermail/linux-arm-kernel/2011-September/065228.html
>
> Also reverse the order of mxs_dma_disable_chan() and
> mxs_dma_reset_chan() in mxs_dma_control() because mxs_dma_reset_chan()
> can only work when the DMA channel is enabled.
>
> Signed-off-by: Lothar Waßmann<LW@KARO-electronics.de>
> ---
>   drivers/dma/mxs-dma.c |   29 +----------------------------
>   1 files changed, 1 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/dma/mxs-dma.c b/drivers/dma/mxs-dma.c
> index 78336b0..71ba93a 100644
> --- a/drivers/dma/mxs-dma.c
> +++ b/drivers/dma/mxs-dma.c
> @@ -44,7 +44,6 @@
>   #define HW_APBHX_CTRL0				0x000
>   #define BM_APBH_CTRL0_APB_BURST8_EN		(1<<  29)
>   #define BM_APBH_CTRL0_APB_BURST_EN		(1<<  28)
> -#define BP_APBH_CTRL0_CLKGATE_CHANNEL		8
>   #define BP_APBH_CTRL0_RESET_CHANNEL		16
>   #define HW_APBHX_CTRL1				0x010
>   #define HW_APBHX_CTRL2				0x020
> @@ -131,23 +130,6 @@ struct mxs_dma_engine {
>   	struct mxs_dma_chan		mxs_chans[MXS_DMA_CHANNELS];
>   };
>
> -static inline void mxs_dma_clkgate(struct mxs_dma_chan *mxs_chan, int enable)
> -{
> -	struct mxs_dma_engine *mxs_dma = mxs_chan->mxs_dma;
> -	int chan_id = mxs_chan->chan.chan_id;
> -	int set_clr = enable ? MXS_CLR_ADDR : MXS_SET_ADDR;
> -
> -	/* enable apbh channel clock */
> -	if (dma_is_apbh()) {
> -		if (apbh_is_old())
> -			writel(1<<  (chan_id + BP_APBH_CTRL0_CLKGATE_CHANNEL),
> -				mxs_dma->base + HW_APBHX_CTRL0 + set_clr);
> -		else
> -			writel(1<<  chan_id,
> -				mxs_dma->base + HW_APBHX_CTRL0 + set_clr);
> -	}
> -}
> -
>   static void mxs_dma_reset_chan(struct mxs_dma_chan *mxs_chan)
>   {
>   	struct mxs_dma_engine *mxs_dma = mxs_chan->mxs_dma;
> @@ -166,9 +148,6 @@ static void mxs_dma_enable_chan(struct mxs_dma_chan *mxs_chan)
>   	struct mxs_dma_engine *mxs_dma = mxs_chan->mxs_dma;
>   	int chan_id = mxs_chan->chan.chan_id;
>
> -	/* clkgate needs to be enabled before writing other registers */
> -	mxs_dma_clkgate(mxs_chan, 1);
> -
>   	/* set cmd_addr up */
>   	writel(mxs_chan->ccw_phys,
>   		mxs_dma->base + HW_APBHX_CHn_NXTCMDAR(chan_id));
> @@ -179,9 +158,6 @@ static void mxs_dma_enable_chan(struct mxs_dma_chan *mxs_chan)
>
>   static void mxs_dma_disable_chan(struct mxs_dma_chan *mxs_chan)
>   {
> -	/* disable apbh channel clock */
> -	mxs_dma_clkgate(mxs_chan, 0);
> -
>   	mxs_chan->status = DMA_SUCCESS;
>   }
>
> @@ -339,10 +315,7 @@ static int mxs_dma_alloc_chan_resources(struct dma_chan *chan)
>   	if (ret)
>   		goto err_clk;
>
> -	/* clkgate needs to be enabled for reset to finish */
> -	mxs_dma_clkgate(mxs_chan, 1);
>   	mxs_dma_reset_chan(mxs_chan);
> -	mxs_dma_clkgate(mxs_chan, 0);
>
>   	dma_async_tx_descriptor_init(&mxs_chan->desc, chan);
>   	mxs_chan->desc.tx_submit = mxs_dma_tx_submit;
> @@ -542,8 +515,8 @@ static int mxs_dma_control(struct dma_chan *chan, enum dma_ctrl_cmd cmd,
>
>   	switch (cmd) {
>   	case DMA_TERMINATE_ALL:
> -		mxs_dma_disable_chan(mxs_chan);
>   		mxs_dma_reset_chan(mxs_chan);
> +		mxs_dma_disable_chan(mxs_chan);
>   		break;
>   	case DMA_PAUSE:
>   		mxs_dma_pause_chan(mxs_chan);

Re: [PATCH 0/4] dma: mxs-dma: several bugfixes for the MXS DMA driver

[Vinod Koul]

On Thu, 2011-12-08 at 09:15 +0100, Lothar Waßmann wrote:
> The following patchset fixes some bugs in the MXS DMA driver.
> 
> Lothar Waßmann (4):
>   fix a typo in comment
>   Always leave mxs_dma_init() with the clock disabled.
>   Using a static variable for counting the number of CCWs attached to
>   Don't use CLKGATE to disable DMA clock.
> 
All Applied, Thanks

-- 
~Vinod