dma: mxs-dma: several bugfixes for the MXS DMA driver
Subsystems (2)
cover dma: mxs-dma: several bugfixes for the MXS DMA driver
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
--- drivers/dma/mxs-dma.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
drivers/dma/mxs-dma.c +1 -1
@@ -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) */
Signature · 1 line
1.5.6.5
PATCH 2/4 dma: mxs-dma: Always leave mxs_dma_init() with the clock disabled.
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.
--- drivers/dma/mxs-dma.c | 7 ++----- 1 files changed, 2 insertions(+), 5 deletions(-)
drivers/dma/mxs-dma.c +2 -5
@@ -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; }
Signature · 1 line
1.5.6.5
PATCH 3/4 dma: mxs-dma: make mxs_dma_prep_slave_sg() multi user safe
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.
--- drivers/dma/mxs-dma.c | 5 ++++- 1 files changed, 4 insertions(+), 1 deletions(-)
drivers/dma/mxs-dma.c +4 -1
@@ -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;
Signature · 1 line
1.5.6.5
PATCH 4/4 dma: mxs-dma: Don't use CLKGATE bits in CTRL0 to disable DMA channels
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.
--- drivers/dma/mxs-dma.c | 29 +---------------------------- 1 files changed, 1 insertions(+), 28 deletions(-)
drivers/dma/mxs-dma.c +1 -28
@@ -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);
Signature · 1 line
1.5.6.5
Re: [PATCH 4/4] dma: mxs-dma: Don't use CLKGATE bits in CTRL0 to disable DMA channels
Quoted 86 lines · 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.htmlAlso reverse the order of
mxs_dma_disable_chan()andmxs_dma_reset_chan()inmxs_dma_control()becausemxs_dma_reset_chan()can only work when the DMA channel is enabled.Signed-off-by Lothar Waßmann<[email protected]>--- drivers/dma/mxs-dma.c | 29 +---------------------------- 1 files changed, 1 insertions(+), 28 deletions(-)drivers/dma/mxs-dma.c +0
-27@@ -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;
drivers/dma/mxs-dma.c static int mxs_dma_control(struct dma_chan *chan, enum dma_ctrl_cmd cmd, · line 522
Quoted 9 lines
drivers/dma/mxs-dma.c +1
-1@@ -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
In reply to Lothar WaßmannHi,
[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
Quoted 95 lines
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.htmlAlso reverse the order of
mxs_dma_disable_chan()andmxs_dma_reset_chan()inmxs_dma_control()becausemxs_dma_reset_chan()can only work when the DMA channel is enabled.Signed-off-by Lothar Waßmann<[email protected]>--- drivers/dma/mxs-dma.c | 29 +---------------------------- 1 files changed, 1 insertions(+), 28 deletions(-)drivers/dma/mxs-dma.c +1
-28@@ -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 3/4] dma: mxs-dma: make mxs_dma_prep_slave_sg() multi user safe
In reply to Lothar WaßmannQuoted 38 lines · 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<[email protected]>--- drivers/dma/mxs-dma.c | 5 ++++- 1 files changed, 4 insertions(+), 1 deletions(-)drivers/dma/mxs-dma.c +3
-1@@ -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;
drivers/dma/mxs-dma.c static struct dma_async_tx_descriptor *mxs_dma_prep_dma_cyclic( · line 531
Quoted 7 lines
drivers/dma/mxs-dma.c +1
-0@@ -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.
Quoted 8 lines · Lothar Waßmann
The following patchset fixes some bugs in the MXS DMA driver.
Lothar Waßmann (4):
fix a typo in comment
Always leavemxs_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
Signature · 1 line
~Vinod