* [PATCH 1/6] dma/imx-sdma: let sdma_run_channel call sdma_enable_channel @ 2012-01-10 7:01 Richard Zhao 2012-01-10 7:01 ` [PATCH 2/6] dma/imx-sdma: use readl_relaxed/writel_relaxed and use writel when necessary Richard Zhao ` (4 more replies) 0 siblings, 5 replies; 22+ messages in thread From: Richard Zhao @ 2012-01-10 7:01 UTC (permalink / raw) To: linux-kernel, linux-arm-kernel Cc: dan.j.williams, vinod.koul, shawn.guo, kernel, eric.miao, patches, Richard Zhao Let all enable channel code call sdma_enable_channel. Signed-off-by: Richard Zhao <richard.zhao@linaro.org> Acked-by: Shawn Guo <shawn.guo@linaro.org> Acked-by: Sascha Hauer <s.hauer@pengutronix.de> --- drivers/dma/imx-sdma.c | 12 ++++++------ 1 files changed, 6 insertions(+), 6 deletions(-) diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c index f59fd8f..c2bc4f1 100644 --- a/drivers/dma/imx-sdma.c +++ b/drivers/dma/imx-sdma.c @@ -394,6 +394,11 @@ static int sdma_config_ownership(struct sdma_channel *sdmac, return 0; } +static void sdma_enable_channel(struct sdma_engine *sdma, int channel) +{ + __raw_writel(1 << channel, sdma->regs + SDMA_H_START); +} + /* * sdma_run_channel - run a channel and wait till it's done */ @@ -405,7 +410,7 @@ static int sdma_run_channel(struct sdma_channel *sdmac) init_completion(&sdmac->done); - __raw_writel(1 << channel, sdma->regs + SDMA_H_START); + sdma_enable_channel(sdma, channel); ret = wait_for_completion_timeout(&sdmac->done, HZ); @@ -811,11 +816,6 @@ out: return ret; } -static void sdma_enable_channel(struct sdma_engine *sdma, int channel) -{ - __raw_writel(1 << channel, sdma->regs + SDMA_H_START); -} - static dma_cookie_t sdma_assign_cookie(struct sdma_channel *sdmac) { dma_cookie_t cookie = sdmac->chan.cookie; -- 1.7.5.4 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 2/6] dma/imx-sdma: use readl_relaxed/writel_relaxed and use writel when necessary 2012-01-10 7:01 [PATCH 1/6] dma/imx-sdma: let sdma_run_channel call sdma_enable_channel Richard Zhao @ 2012-01-10 7:01 ` Richard Zhao 2012-01-10 7:01 ` [PATCH 3/6] dma/imx-sdma: call sdma_set_channel_priority after sdma_request_channel Richard Zhao ` (3 subsequent siblings) 4 siblings, 0 replies; 22+ messages in thread From: Richard Zhao @ 2012-01-10 7:01 UTC (permalink / raw) To: linux-kernel, linux-arm-kernel Cc: dan.j.williams, vinod.koul, shawn.guo, kernel, eric.miao, patches, Richard Zhao use readl_relaxed/writel_relaxed in most places, and use writel when enable channel, because it needs memory barrier. Signed-off-by: Richard Zhao <richard.zhao@linaro.org> --- drivers/dma/imx-sdma.c | 44 ++++++++++++++++++++++---------------------- 1 files changed, 22 insertions(+), 22 deletions(-) diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c index c2bc4f1..63f4752 100644 --- a/drivers/dma/imx-sdma.c +++ b/drivers/dma/imx-sdma.c @@ -368,9 +368,9 @@ static int sdma_config_ownership(struct sdma_channel *sdmac, if (event_override && mcu_override && dsp_override) return -EINVAL; - evt = __raw_readl(sdma->regs + SDMA_H_EVTOVR); - mcu = __raw_readl(sdma->regs + SDMA_H_HOSTOVR); - dsp = __raw_readl(sdma->regs + SDMA_H_DSPOVR); + evt = readl_relaxed(sdma->regs + SDMA_H_EVTOVR); + mcu = readl_relaxed(sdma->regs + SDMA_H_HOSTOVR); + dsp = readl_relaxed(sdma->regs + SDMA_H_DSPOVR); if (dsp_override) dsp &= ~(1 << channel); @@ -387,16 +387,16 @@ static int sdma_config_ownership(struct sdma_channel *sdmac, else mcu |= (1 << channel); - __raw_writel(evt, sdma->regs + SDMA_H_EVTOVR); - __raw_writel(mcu, sdma->regs + SDMA_H_HOSTOVR); - __raw_writel(dsp, sdma->regs + SDMA_H_DSPOVR); + writel_relaxed(evt, sdma->regs + SDMA_H_EVTOVR); + writel_relaxed(mcu, sdma->regs + SDMA_H_HOSTOVR); + writel_relaxed(dsp, sdma->regs + SDMA_H_DSPOVR); return 0; } static void sdma_enable_channel(struct sdma_engine *sdma, int channel) { - __raw_writel(1 << channel, sdma->regs + SDMA_H_START); + writel(1 << channel, sdma->regs + SDMA_H_START); } /* @@ -460,9 +460,9 @@ static void sdma_event_enable(struct sdma_channel *sdmac, unsigned int event) u32 val; u32 chnenbl = chnenbl_ofs(sdma, event); - val = __raw_readl(sdma->regs + chnenbl); + val = readl_relaxed(sdma->regs + chnenbl); val |= (1 << channel); - __raw_writel(val, sdma->regs + chnenbl); + writel_relaxed(val, sdma->regs + chnenbl); } static void sdma_event_disable(struct sdma_channel *sdmac, unsigned int event) @@ -472,9 +472,9 @@ static void sdma_event_disable(struct sdma_channel *sdmac, unsigned int event) u32 chnenbl = chnenbl_ofs(sdma, event); u32 val; - val = __raw_readl(sdma->regs + chnenbl); + val = readl_relaxed(sdma->regs + chnenbl); val &= ~(1 << channel); - __raw_writel(val, sdma->regs + chnenbl); + writel_relaxed(val, sdma->regs + chnenbl); } static void sdma_handle_channel_loop(struct sdma_channel *sdmac) @@ -552,8 +552,8 @@ static irqreturn_t sdma_int_handler(int irq, void *dev_id) struct sdma_engine *sdma = dev_id; u32 stat; - stat = __raw_readl(sdma->regs + SDMA_H_INTR); - __raw_writel(stat, sdma->regs + SDMA_H_INTR); + stat = readl_relaxed(sdma->regs + SDMA_H_INTR); + writel_relaxed(stat, sdma->regs + SDMA_H_INTR); while (stat) { int channel = fls(stat) - 1; @@ -707,7 +707,7 @@ static void sdma_disable_channel(struct sdma_channel *sdmac) struct sdma_engine *sdma = sdmac->sdma; int channel = sdmac->channel; - __raw_writel(1 << channel, sdma->regs + SDMA_H_STATSTOP); + writel_relaxed(1 << channel, sdma->regs + SDMA_H_STATSTOP); sdmac->status = DMA_ERROR; } @@ -780,7 +780,7 @@ static int sdma_set_channel_priority(struct sdma_channel *sdmac, return -EINVAL; } - __raw_writel(priority, sdma->regs + SDMA_CHNPRI_0 + 4 * channel); + writel_relaxed(priority, sdma->regs + SDMA_CHNPRI_0 + 4 * channel); return 0; } @@ -1228,7 +1228,7 @@ static int __init sdma_init(struct sdma_engine *sdma) clk_enable(sdma->clk); /* Be sure SDMA has not started yet */ - __raw_writel(0, sdma->regs + SDMA_H_C0PTR); + writel_relaxed(0, sdma->regs + SDMA_H_C0PTR); sdma->channel_control = dma_alloc_coherent(NULL, MAX_DMA_CHANNELS * sizeof (struct sdma_channel_control) + @@ -1251,11 +1251,11 @@ static int __init sdma_init(struct sdma_engine *sdma) /* disable all channels */ for (i = 0; i < sdma->num_events; i++) - __raw_writel(0, sdma->regs + chnenbl_ofs(sdma, i)); + writel_relaxed(0, sdma->regs + chnenbl_ofs(sdma, i)); /* All channels have priority 0 */ for (i = 0; i < MAX_DMA_CHANNELS; i++) - __raw_writel(0, sdma->regs + SDMA_CHNPRI_0 + i * 4); + writel_relaxed(0, sdma->regs + SDMA_CHNPRI_0 + i * 4); ret = sdma_request_channel(&sdma->channel[0]); if (ret) @@ -1264,16 +1264,16 @@ static int __init sdma_init(struct sdma_engine *sdma) sdma_config_ownership(&sdma->channel[0], false, true, false); /* Set Command Channel (Channel Zero) */ - __raw_writel(0x4050, sdma->regs + SDMA_CHN0ADDR); + writel_relaxed(0x4050, sdma->regs + SDMA_CHN0ADDR); /* Set bits of CONFIG register but with static context switching */ /* FIXME: Check whether to set ACR bit depending on clock ratios */ - __raw_writel(0, sdma->regs + SDMA_H_CONFIG); + writel_relaxed(0, sdma->regs + SDMA_H_CONFIG); - __raw_writel(ccb_phys, sdma->regs + SDMA_H_C0PTR); + writel_relaxed(ccb_phys, sdma->regs + SDMA_H_C0PTR); /* Set bits of CONFIG register with given context switching mode */ - __raw_writel(SDMA_H_CONFIG_CSM, sdma->regs + SDMA_H_CONFIG); + writel_relaxed(SDMA_H_CONFIG_CSM, sdma->regs + SDMA_H_CONFIG); /* Initializes channel's priorities */ sdma_set_channel_priority(&sdma->channel[0], 7); -- 1.7.5.4 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 3/6] dma/imx-sdma: call sdma_set_channel_priority after sdma_request_channel 2012-01-10 7:01 [PATCH 1/6] dma/imx-sdma: let sdma_run_channel call sdma_enable_channel Richard Zhao 2012-01-10 7:01 ` [PATCH 2/6] dma/imx-sdma: use readl_relaxed/writel_relaxed and use writel when necessary Richard Zhao @ 2012-01-10 7:01 ` Richard Zhao 2012-01-10 7:01 ` [PATCH 4/6] dma/imx-sdma: move clk_enable out of sdma_request_channel Richard Zhao ` (2 subsequent siblings) 4 siblings, 0 replies; 22+ messages in thread From: Richard Zhao @ 2012-01-10 7:01 UTC (permalink / raw) To: linux-kernel, linux-arm-kernel Cc: dan.j.williams, vinod.koul, shawn.guo, kernel, eric.miao, patches, Richard Zhao sdma_request_channel sets the default priority. sdma_alloc_chan_resources should call sdma_set_channel_priority thereafter to over write it. Signed-off-by: Richard Zhao <richard.zhao@linaro.org> Acked-by: Shawn Guo <shawn.guo@linaro.org> --- drivers/dma/imx-sdma.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c index 63f4752..b40f630 100644 --- a/drivers/dma/imx-sdma.c +++ b/drivers/dma/imx-sdma.c @@ -875,11 +875,11 @@ static int sdma_alloc_chan_resources(struct dma_chan *chan) sdmac->peripheral_type = data->peripheral_type; sdmac->event_id0 = data->dma_request; - ret = sdma_set_channel_priority(sdmac, prio); + ret = sdma_request_channel(sdmac); if (ret) return ret; - ret = sdma_request_channel(sdmac); + ret = sdma_set_channel_priority(sdmac, prio); if (ret) return ret; -- 1.7.5.4 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 4/6] dma/imx-sdma: move clk_enable out of sdma_request_channel 2012-01-10 7:01 [PATCH 1/6] dma/imx-sdma: let sdma_run_channel call sdma_enable_channel Richard Zhao 2012-01-10 7:01 ` [PATCH 2/6] dma/imx-sdma: use readl_relaxed/writel_relaxed and use writel when necessary Richard Zhao 2012-01-10 7:01 ` [PATCH 3/6] dma/imx-sdma: call sdma_set_channel_priority after sdma_request_channel Richard Zhao @ 2012-01-10 7:01 ` Richard Zhao 2012-01-10 7:01 ` [PATCH 5/6] dma/imx-sdma: use num_events to validate event_id0 Richard Zhao 2012-01-10 7:01 ` [PATCH 6/6] dma/imx-sdma: check whether event_id0 < 32 when set event_mask Richard Zhao 4 siblings, 0 replies; 22+ messages in thread From: Richard Zhao @ 2012-01-10 7:01 UTC (permalink / raw) To: linux-kernel, linux-arm-kernel Cc: dan.j.williams, vinod.koul, shawn.guo, kernel, eric.miao, patches, Richard Zhao It makes clk_enable/disable pair more readable, and fix one bug: sdma_init calls sdma_request_channel, but seems don't know sdma_request_channel enabled the clok. Signed-off-by: Richard Zhao <richard.zhao@linaro.org> Acked-by: Shawn Guo <shawn.guo@linaro.org> --- drivers/dma/imx-sdma.c | 5 +++-- 1 files changed, 3 insertions(+), 2 deletions(-) diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c index b40f630..015d93f 100644 --- a/drivers/dma/imx-sdma.c +++ b/drivers/dma/imx-sdma.c @@ -802,8 +802,6 @@ static int sdma_request_channel(struct sdma_channel *sdmac) sdma->channel_control[channel].base_bd_ptr = sdmac->bd_phys; sdma->channel_control[channel].current_bd_ptr = sdmac->bd_phys; - clk_enable(sdma->clk); - sdma_set_channel_priority(sdmac, MXC_SDMA_DEFAULT_PRIORITY); init_completion(&sdmac->done); @@ -875,6 +873,9 @@ static int sdma_alloc_chan_resources(struct dma_chan *chan) sdmac->peripheral_type = data->peripheral_type; sdmac->event_id0 = data->dma_request; + + clk_enable(sdmac->sdma->clk); + ret = sdma_request_channel(sdmac); if (ret) return ret; -- 1.7.5.4 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 5/6] dma/imx-sdma: use num_events to validate event_id0 2012-01-10 7:01 [PATCH 1/6] dma/imx-sdma: let sdma_run_channel call sdma_enable_channel Richard Zhao ` (2 preceding siblings ...) 2012-01-10 7:01 ` [PATCH 4/6] dma/imx-sdma: move clk_enable out of sdma_request_channel Richard Zhao @ 2012-01-10 7:01 ` Richard Zhao 2012-01-10 14:02 ` Dirk Behme 2012-01-10 14:16 ` Shawn Guo 2012-01-10 7:01 ` [PATCH 6/6] dma/imx-sdma: check whether event_id0 < 32 when set event_mask Richard Zhao 4 siblings, 2 replies; 22+ messages in thread From: Richard Zhao @ 2012-01-10 7:01 UTC (permalink / raw) To: linux-kernel, linux-arm-kernel Cc: dan.j.williams, vinod.koul, shawn.guo, kernel, eric.miao, patches, Richard Zhao event number is not always 32. use num_events for checking instead. Signed-off-by: Richard Zhao <richard.zhao@linaro.org> --- drivers/dma/imx-sdma.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c index 015d93f..22fd561 100644 --- a/drivers/dma/imx-sdma.c +++ b/drivers/dma/imx-sdma.c @@ -723,7 +723,7 @@ static int sdma_config_channel(struct sdma_channel *sdmac) sdmac->per_addr = 0; if (sdmac->event_id0) { - if (sdmac->event_id0 > 32) + if (sdmac->event_id0 >= sdmac->sdma->num_events) return -EINVAL; sdma_event_enable(sdmac, sdmac->event_id0); } -- 1.7.5.4 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH 5/6] dma/imx-sdma: use num_events to validate event_id0 2012-01-10 7:01 ` [PATCH 5/6] dma/imx-sdma: use num_events to validate event_id0 Richard Zhao @ 2012-01-10 14:02 ` Dirk Behme 2012-01-10 14:16 ` Shawn Guo 1 sibling, 0 replies; 22+ messages in thread From: Dirk Behme @ 2012-01-10 14:02 UTC (permalink / raw) To: Richard Zhao Cc: linux-kernel, linux-arm-kernel, patches, vinod.koul, eric.miao, kernel, dan.j.williams, shawn.guo On 10.01.2012 08:01, Richard Zhao wrote: > event number is not always 32. use num_events for checking instead. > > Signed-off-by: Richard Zhao <richard.zhao@linaro.org> > --- > drivers/dma/imx-sdma.c | 2 +- > 1 files changed, 1 insertions(+), 1 deletions(-) > > diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c > index 015d93f..22fd561 100644 > --- a/drivers/dma/imx-sdma.c > +++ b/drivers/dma/imx-sdma.c > @@ -723,7 +723,7 @@ static int sdma_config_channel(struct sdma_channel *sdmac) > sdmac->per_addr = 0; > > if (sdmac->event_id0) { > - if (sdmac->event_id0 > 32) > + if (sdmac->event_id0 >= sdmac->sdma->num_events) > return -EINVAL; > sdma_event_enable(sdmac, sdmac->event_id0); > } In your git you have http://git.linaro.org/gitweb?p=people/riczhao/linux-2.6.git;a=commitdiff;h=4bd46e7f8ade05984ee89222bc0523b656d017d2 - if (sdmac->event_id0) { - if (sdmac->event_id0 > 32) - return -EINVAL; + if (sdmac->event_id0) sdma_event_enable(sdmac, sdmac->event_id0); I your git version outdated and replaced by this patch? Best regards Dirk ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 5/6] dma/imx-sdma: use num_events to validate event_id0 2012-01-10 7:01 ` [PATCH 5/6] dma/imx-sdma: use num_events to validate event_id0 Richard Zhao 2012-01-10 14:02 ` Dirk Behme @ 2012-01-10 14:16 ` Shawn Guo 1 sibling, 0 replies; 22+ messages in thread From: Shawn Guo @ 2012-01-10 14:16 UTC (permalink / raw) To: Richard Zhao Cc: linux-kernel, linux-arm-kernel, dan.j.williams, vinod.koul, kernel, eric.miao, patches On Tue, Jan 10, 2012 at 03:01:49PM +0800, Richard Zhao wrote: > event number is not always 32. use num_events for checking instead. > > Signed-off-by: Richard Zhao <richard.zhao@linaro.org> Acked-by: Shawn Guo <shawn.guo@linaro.org> Regards, Shawn > --- > drivers/dma/imx-sdma.c | 2 +- > 1 files changed, 1 insertions(+), 1 deletions(-) > > diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c > index 015d93f..22fd561 100644 > --- a/drivers/dma/imx-sdma.c > +++ b/drivers/dma/imx-sdma.c > @@ -723,7 +723,7 @@ static int sdma_config_channel(struct sdma_channel *sdmac) > sdmac->per_addr = 0; > > if (sdmac->event_id0) { > - if (sdmac->event_id0 > 32) > + if (sdmac->event_id0 >= sdmac->sdma->num_events) > return -EINVAL; > sdma_event_enable(sdmac, sdmac->event_id0); > } > -- > 1.7.5.4 > > ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 6/6] dma/imx-sdma: check whether event_id0 < 32 when set event_mask 2012-01-10 7:01 [PATCH 1/6] dma/imx-sdma: let sdma_run_channel call sdma_enable_channel Richard Zhao ` (3 preceding siblings ...) 2012-01-10 7:01 ` [PATCH 5/6] dma/imx-sdma: use num_events to validate event_id0 Richard Zhao @ 2012-01-10 7:01 ` Richard Zhao 2012-01-10 14:20 ` Shawn Guo 4 siblings, 1 reply; 22+ messages in thread From: Richard Zhao @ 2012-01-10 7:01 UTC (permalink / raw) To: linux-kernel, linux-arm-kernel Cc: dan.j.williams, vinod.koul, shawn.guo, kernel, eric.miao, patches, Richard Zhao Signed-off-by: Richard Zhao <richard.zhao@linaro.org> --- drivers/dma/imx-sdma.c | 7 +++++-- 1 files changed, 5 insertions(+), 2 deletions(-) diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c index 22fd561..1d5b6ab 100644 --- a/drivers/dma/imx-sdma.c +++ b/drivers/dma/imx-sdma.c @@ -753,8 +753,11 @@ static int sdma_config_channel(struct sdma_channel *sdmac) if (sdmac->event_id0 > 31) sdmac->watermark_level |= 1 << 30; } else { - sdmac->event_mask0 = 1 << sdmac->event_id0; - sdmac->event_mask1 = 1 << (sdmac->event_id0 - 32); + if (sdmac->event_id0 < 32) + sdmac->event_mask0 = 1 << sdmac->event_id0; + else + sdmac->event_mask1 = + 1 << (sdmac->event_id0 - 32); } /* Watermark Level */ sdmac->watermark_level |= sdmac->watermark_level; -- 1.7.5.4 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH 6/6] dma/imx-sdma: check whether event_id0 < 32 when set event_mask 2012-01-10 7:01 ` [PATCH 6/6] dma/imx-sdma: check whether event_id0 < 32 when set event_mask Richard Zhao @ 2012-01-10 14:20 ` Shawn Guo 2012-01-10 14:29 ` Richard Zhao 0 siblings, 1 reply; 22+ messages in thread From: Shawn Guo @ 2012-01-10 14:20 UTC (permalink / raw) To: Richard Zhao Cc: linux-kernel, linux-arm-kernel, dan.j.williams, vinod.koul, kernel, eric.miao, patches On Tue, Jan 10, 2012 at 03:01:50PM +0800, Richard Zhao wrote: > Signed-off-by: Richard Zhao <richard.zhao@linaro.org> > --- I think it deserves a sensible commit message explaining why the patch is needed. Regards, Shawn > drivers/dma/imx-sdma.c | 7 +++++-- > 1 files changed, 5 insertions(+), 2 deletions(-) > > diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c > index 22fd561..1d5b6ab 100644 > --- a/drivers/dma/imx-sdma.c > +++ b/drivers/dma/imx-sdma.c > @@ -753,8 +753,11 @@ static int sdma_config_channel(struct sdma_channel *sdmac) > if (sdmac->event_id0 > 31) > sdmac->watermark_level |= 1 << 30; > } else { > - sdmac->event_mask0 = 1 << sdmac->event_id0; > - sdmac->event_mask1 = 1 << (sdmac->event_id0 - 32); > + if (sdmac->event_id0 < 32) > + sdmac->event_mask0 = 1 << sdmac->event_id0; > + else > + sdmac->event_mask1 = > + 1 << (sdmac->event_id0 - 32); > } > /* Watermark Level */ > sdmac->watermark_level |= sdmac->watermark_level; > -- > 1.7.5.4 > > ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 6/6] dma/imx-sdma: check whether event_id0 < 32 when set event_mask 2012-01-10 14:20 ` Shawn Guo @ 2012-01-10 14:29 ` Richard Zhao 2012-01-10 15:38 ` Shawn Guo 0 siblings, 1 reply; 22+ messages in thread From: Richard Zhao @ 2012-01-10 14:29 UTC (permalink / raw) To: Shawn Guo Cc: linux-kernel, linux-arm-kernel, dan.j.williams, vinod.koul, kernel, eric.miao, patches On Tue, Jan 10, 2012 at 10:20:10PM +0800, Shawn Guo wrote: > On Tue, Jan 10, 2012 at 03:01:50PM +0800, Richard Zhao wrote: > > Signed-off-by: Richard Zhao <richard.zhao@linaro.org> > > --- > > I think it deserves a sensible commit message explaining why the patch > is needed. If event_id0 < 32, 1 << (sdmac->event_id0 - 32) is not zero. Thanks Richard > > Regards, > Shawn > > > drivers/dma/imx-sdma.c | 7 +++++-- > > 1 files changed, 5 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c > > index 22fd561..1d5b6ab 100644 > > --- a/drivers/dma/imx-sdma.c > > +++ b/drivers/dma/imx-sdma.c > > @@ -753,8 +753,11 @@ static int sdma_config_channel(struct sdma_channel *sdmac) > > if (sdmac->event_id0 > 31) > > sdmac->watermark_level |= 1 << 30; > > } else { > > - sdmac->event_mask0 = 1 << sdmac->event_id0; > > - sdmac->event_mask1 = 1 << (sdmac->event_id0 - 32); > > + if (sdmac->event_id0 < 32) > > + sdmac->event_mask0 = 1 << sdmac->event_id0; > > + else > > + sdmac->event_mask1 = > > + 1 << (sdmac->event_id0 - 32); > > } > > /* Watermark Level */ > > sdmac->watermark_level |= sdmac->watermark_level; > > -- > > 1.7.5.4 > > > > ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 6/6] dma/imx-sdma: check whether event_id0 < 32 when set event_mask 2012-01-10 14:29 ` Richard Zhao @ 2012-01-10 15:38 ` Shawn Guo 2012-01-11 0:53 ` Richard Zhao 0 siblings, 1 reply; 22+ messages in thread From: Shawn Guo @ 2012-01-10 15:38 UTC (permalink / raw) To: Richard Zhao Cc: linux-kernel, linux-arm-kernel, dan.j.williams, vinod.koul, kernel, eric.miao, patches On Tue, Jan 10, 2012 at 10:29:42PM +0800, Richard Zhao wrote: > On Tue, Jan 10, 2012 at 10:20:10PM +0800, Shawn Guo wrote: > > On Tue, Jan 10, 2012 at 03:01:50PM +0800, Richard Zhao wrote: > > > Signed-off-by: Richard Zhao <richard.zhao@linaro.org> > > > --- > > > > I think it deserves a sensible commit message explaining why the patch > > is needed. > If event_id0 < 32, 1 << (sdmac->event_id0 - 32) is not zero. > My point is you may explain the exact problem you are seeing without this patch and how the patch helps here. In general, doing so would win a warm feeling from reviewers much more easily than leaving the commit message empty there. -- Regards, Shawn ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 6/6] dma/imx-sdma: check whether event_id0 < 32 when set event_mask 2012-01-10 15:38 ` Shawn Guo @ 2012-01-11 0:53 ` Richard Zhao 2012-01-11 1:47 ` Richard Zhao 2012-01-11 13:11 ` Shawn Guo 0 siblings, 2 replies; 22+ messages in thread From: Richard Zhao @ 2012-01-11 0:53 UTC (permalink / raw) To: Shawn Guo Cc: Richard Zhao, patches, vinod.koul, linux-kernel, eric.miao, kernel, dan.j.williams, linux-arm-kernel On Tue, Jan 10, 2012 at 11:38:39PM +0800, Shawn Guo wrote: > On Tue, Jan 10, 2012 at 10:29:42PM +0800, Richard Zhao wrote: > > On Tue, Jan 10, 2012 at 10:20:10PM +0800, Shawn Guo wrote: > > > On Tue, Jan 10, 2012 at 03:01:50PM +0800, Richard Zhao wrote: > > > > Signed-off-by: Richard Zhao <richard.zhao@linaro.org> > > > > --- > > > > > > I think it deserves a sensible commit message explaining why the patch > > > is needed. > > If event_id0 < 32, 1 << (sdmac->event_id0 - 32) is not zero. This meant to make you clear about the patch. I'll add it in commit message. > > > My point is you may explain the exact problem you are seeing without > this patch The kernel don't have event_id < 32 case yet. I found the bug when I review the code. > and how the patch helps here. In general, doing so would > win a warm feeling from reviewers much more easily than leaving the > commit message empty there. I understand your point that comment as much as possible. Thanks Richard > > -- > Regards, > Shawn > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 6/6] dma/imx-sdma: check whether event_id0 < 32 when set event_mask 2012-01-11 0:53 ` Richard Zhao @ 2012-01-11 1:47 ` Richard Zhao 2012-01-11 6:37 ` Eric Miao 2012-01-11 13:11 ` Shawn Guo 1 sibling, 1 reply; 22+ messages in thread From: Richard Zhao @ 2012-01-11 1:47 UTC (permalink / raw) To: Shawn Guo Cc: patches, vinod.koul, linux-kernel, eric.miao, kernel, dan.j.williams, Richard Zhao, linux-arm-kernel On Wed, Jan 11, 2012 at 08:53:23AM +0800, Richard Zhao wrote: > On Tue, Jan 10, 2012 at 11:38:39PM +0800, Shawn Guo wrote: > > On Tue, Jan 10, 2012 at 10:29:42PM +0800, Richard Zhao wrote: > > > On Tue, Jan 10, 2012 at 10:20:10PM +0800, Shawn Guo wrote: > > > > On Tue, Jan 10, 2012 at 03:01:50PM +0800, Richard Zhao wrote: > > > > > Signed-off-by: Richard Zhao <richard.zhao@linaro.org> > > > > > --- > > > > > > > > I think it deserves a sensible commit message explaining why the patch > > > > is needed. > > > If event_id0 < 32, 1 << (sdmac->event_id0 - 32) is not zero. > This meant to make you clear about the patch. I'll add it in commit > message. unsigned int t = 31; printf("%d %08x\n", t, 1 << (t-32)); I test above code on both x86 and arm. They shows different results. x86: 31 80000000 arm: 31 00000000 I think we still need this patch. we shoud not let it depends on gcc's behavior. Thanks Richard > > > > > My point is you may explain the exact problem you are seeing without > > this patch > The kernel don't have event_id < 32 case yet. I found the bug when > I review the code. > > and how the patch helps here. In general, doing so would > > win a warm feeling from reviewers much more easily than leaving the > > commit message empty there. > I understand your point that comment as much as possible. > > Thanks > Richard > > > > -- > > Regards, > > Shawn > > > > _______________________________________________ > > linux-arm-kernel mailing list > > linux-arm-kernel@lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > > > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 6/6] dma/imx-sdma: check whether event_id0 < 32 when set event_mask 2012-01-11 1:47 ` Richard Zhao @ 2012-01-11 6:37 ` Eric Miao 2012-01-11 12:35 ` Richard Zhao 2012-01-11 13:16 ` Shawn Guo 0 siblings, 2 replies; 22+ messages in thread From: Eric Miao @ 2012-01-11 6:37 UTC (permalink / raw) To: Richard Zhao Cc: Shawn Guo, patches, vinod.koul, linux-kernel, kernel, dan.j.williams, Richard Zhao, linux-arm-kernel On Wed, Jan 11, 2012 at 9:47 AM, Richard Zhao <richard.zhao@freescale.com> wrote: > On Wed, Jan 11, 2012 at 08:53:23AM +0800, Richard Zhao wrote: >> On Tue, Jan 10, 2012 at 11:38:39PM +0800, Shawn Guo wrote: >> > On Tue, Jan 10, 2012 at 10:29:42PM +0800, Richard Zhao wrote: >> > > On Tue, Jan 10, 2012 at 10:20:10PM +0800, Shawn Guo wrote: >> > > > On Tue, Jan 10, 2012 at 03:01:50PM +0800, Richard Zhao wrote: >> > > > > Signed-off-by: Richard Zhao <richard.zhao@linaro.org> >> > > > > --- >> > > > >> > > > I think it deserves a sensible commit message explaining why the patch >> > > > is needed. >> > > If event_id0 < 32, 1 << (sdmac->event_id0 - 32) is not zero. >> This meant to make you clear about the patch. I'll add it in commit >> message. > unsigned int t = 31; > printf("%d %08x\n", t, 1 << (t-32)); > > I test above code on both x86 and arm. They shows different results. > x86: 31 80000000 > arm: 31 00000000 > > I think we still need this patch. we shoud not let it depends on gcc's > behavior. > > Thanks > Richard >> > > >> > My point is you may explain the exact problem you are seeing without >> > this patch >> The kernel don't have event_id < 32 case yet. I found the bug when >> I review the code. >> > and how the patch helps here. In general, doing so would >> > win a warm feeling from reviewers much more easily than leaving the >> > commit message empty there. >> I understand your point that comment as much as possible. >> Shawn, I think Richard has made the issue quite clear here, the original code does seem to have some problems even to me, who do not understand the very details of the SDMA: - sdmac->event_mask0 = 1 << sdmac->event_id0; - sdmac->event_mask1 = 1 << (sdmac->event_id0 - 32); 1. if sdmac->event_id0 >= 32, which will cause event_mask0 to be incorrect 2. if sdmac->event_id < 32, sdmac->event_mask1 will be incorrect An alternate way is to use the standard bit operations: struct sdma_channel { ... unsigned long event_mask[2]; ... }; set_bit(sdmac->event_id0, event_mask); Which avoids branch instructions and add a bit protection for the operation to be atomic enough (event_mask0/1 won't be inconsistent). ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 6/6] dma/imx-sdma: check whether event_id0 < 32 when set event_mask 2012-01-11 6:37 ` Eric Miao @ 2012-01-11 12:35 ` Richard Zhao 2012-01-11 12:53 ` Richard Zhao 2012-01-11 13:16 ` Shawn Guo 1 sibling, 1 reply; 22+ messages in thread From: Richard Zhao @ 2012-01-11 12:35 UTC (permalink / raw) To: Eric Miao Cc: Richard Zhao, Shawn Guo, patches, vinod.koul, linux-kernel, kernel, dan.j.williams, linux-arm-kernel On Wed, Jan 11, 2012 at 02:37:08PM +0800, Eric Miao wrote: > On Wed, Jan 11, 2012 at 9:47 AM, Richard Zhao > <richard.zhao@freescale.com> wrote: > > On Wed, Jan 11, 2012 at 08:53:23AM +0800, Richard Zhao wrote: > >> On Tue, Jan 10, 2012 at 11:38:39PM +0800, Shawn Guo wrote: > >> > On Tue, Jan 10, 2012 at 10:29:42PM +0800, Richard Zhao wrote: > >> > > On Tue, Jan 10, 2012 at 10:20:10PM +0800, Shawn Guo wrote: > >> > > > On Tue, Jan 10, 2012 at 03:01:50PM +0800, Richard Zhao wrote: > >> > > > > Signed-off-by: Richard Zhao <richard.zhao@linaro.org> > >> > > > > --- > >> > > > > >> > > > I think it deserves a sensible commit message explaining why the patch > >> > > > is needed. > >> > > If event_id0 < 32, 1 << (sdmac->event_id0 - 32) is not zero. > >> This meant to make you clear about the patch. I'll add it in commit > >> message. > > unsigned int t = 31; > > printf("%d %08x\n", t, 1 << (t-32)); > > > > I test above code on both x86 and arm. They shows different results. > > x86: 31 80000000 > > arm: 31 00000000 > > > > I think we still need this patch. we shoud not let it depends on gcc's > > behavior. > > > > Thanks > > Richard > >> > > > >> > My point is you may explain the exact problem you are seeing without > >> > this patch > >> The kernel don't have event_id < 32 case yet. I found the bug when > >> I review the code. > >> > and how the patch helps here. In general, doing so would > >> > win a warm feeling from reviewers much more easily than leaving the > >> > commit message empty there. > >> I understand your point that comment as much as possible. > >> > > Shawn, > > I think Richard has made the issue quite clear here, the original > code does seem to have some problems even to me, who do not > understand the very details of the SDMA: > > - sdmac->event_mask0 = 1 << sdmac->event_id0; > - sdmac->event_mask1 = 1 << (sdmac->event_id0 - 32); > > 1. if sdmac->event_id0 >= 32, which will cause event_mask0 to be incorrect > 2. if sdmac->event_id < 32, sdmac->event_mask1 will be incorrect > > An alternate way is to use the standard bit operations: > > struct sdma_channel { > > ... > > unsigned long event_mask[2]; > > ... > }; > > set_bit(sdmac->event_id0, event_mask); > > Which avoids branch instructions and add a bit protection for the operation > to be atomic enough (event_mask0/1 won't be inconsistent). It's a good idea. Thanks Richard ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 6/6] dma/imx-sdma: check whether event_id0 < 32 when set event_mask 2012-01-11 12:35 ` Richard Zhao @ 2012-01-11 12:53 ` Richard Zhao 2012-01-12 14:23 ` Richard Zhao 0 siblings, 1 reply; 22+ messages in thread From: Richard Zhao @ 2012-01-11 12:53 UTC (permalink / raw) To: Richard Zhao Cc: Eric Miao, Shawn Guo, patches, vinod.koul, linux-kernel, kernel, dan.j.williams, linux-arm-kernel On Wed, Jan 11, 2012 at 08:35:57PM +0800, Richard Zhao wrote: > On Wed, Jan 11, 2012 at 02:37:08PM +0800, Eric Miao wrote: > > On Wed, Jan 11, 2012 at 9:47 AM, Richard Zhao > > <richard.zhao@freescale.com> wrote: > > > On Wed, Jan 11, 2012 at 08:53:23AM +0800, Richard Zhao wrote: > > >> On Tue, Jan 10, 2012 at 11:38:39PM +0800, Shawn Guo wrote: > > >> > On Tue, Jan 10, 2012 at 10:29:42PM +0800, Richard Zhao wrote: > > >> > > On Tue, Jan 10, 2012 at 10:20:10PM +0800, Shawn Guo wrote: > > >> > > > On Tue, Jan 10, 2012 at 03:01:50PM +0800, Richard Zhao wrote: > > >> > > > > Signed-off-by: Richard Zhao <richard.zhao@linaro.org> > > >> > > > > --- > > >> > > > > > >> > > > I think it deserves a sensible commit message explaining why the patch > > >> > > > is needed. > > >> > > If event_id0 < 32, 1 << (sdmac->event_id0 - 32) is not zero. > > >> This meant to make you clear about the patch. I'll add it in commit > > >> message. > > > unsigned int t = 31; > > > printf("%d %08x\n", t, 1 << (t-32)); > > > > > > I test above code on both x86 and arm. They shows different results. > > > x86: 31 80000000 > > > arm: 31 00000000 > > > > > > I think we still need this patch. we shoud not let it depends on gcc's > > > behavior. > > > > > > Thanks > > > Richard > > >> > > > > >> > My point is you may explain the exact problem you are seeing without > > >> > this patch > > >> The kernel don't have event_id < 32 case yet. I found the bug when > > >> I review the code. > > >> > and how the patch helps here. In general, doing so would > > >> > win a warm feeling from reviewers much more easily than leaving the > > >> > commit message empty there. > > >> I understand your point that comment as much as possible. > > >> > > > > Shawn, > > > > I think Richard has made the issue quite clear here, the original > > code does seem to have some problems even to me, who do not > > understand the very details of the SDMA: > > > > - sdmac->event_mask0 = 1 << sdmac->event_id0; > > - sdmac->event_mask1 = 1 << (sdmac->event_id0 - 32); > > > > 1. if sdmac->event_id0 >= 32, which will cause event_mask0 to be incorrect > > 2. if sdmac->event_id < 32, sdmac->event_mask1 will be incorrect > > > > An alternate way is to use the standard bit operations: > > > > struct sdma_channel { > > > > ... > > > > unsigned long event_mask[2]; > > > > ... > > }; > > > > set_bit(sdmac->event_id0, event_mask); > > > > Which avoids branch instructions and add a bit protection for the operation > > to be atomic enough (event_mask0/1 won't be inconsistent). > It's a good idea. I'm not sure whether I can always use bitops for every bit operation case, event it don't need atomic. bitops has locks to be atomic. Thanks Richard > Thanks > Richard > ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 6/6] dma/imx-sdma: check whether event_id0 < 32 when set event_mask 2012-01-11 12:53 ` Richard Zhao @ 2012-01-12 14:23 ` Richard Zhao 0 siblings, 0 replies; 22+ messages in thread From: Richard Zhao @ 2012-01-12 14:23 UTC (permalink / raw) To: Richard Zhao Cc: patches, vinod.koul, linux-kernel, Eric Miao, kernel, dan.j.williams, Shawn Guo, linux-arm-kernel On Wed, Jan 11, 2012 at 08:53:38PM +0800, Richard Zhao wrote: > On Wed, Jan 11, 2012 at 08:35:57PM +0800, Richard Zhao wrote: > > On Wed, Jan 11, 2012 at 02:37:08PM +0800, Eric Miao wrote: > > > On Wed, Jan 11, 2012 at 9:47 AM, Richard Zhao > > > <richard.zhao@freescale.com> wrote: > > > > On Wed, Jan 11, 2012 at 08:53:23AM +0800, Richard Zhao wrote: > > > >> On Tue, Jan 10, 2012 at 11:38:39PM +0800, Shawn Guo wrote: > > > >> > On Tue, Jan 10, 2012 at 10:29:42PM +0800, Richard Zhao wrote: > > > >> > > On Tue, Jan 10, 2012 at 10:20:10PM +0800, Shawn Guo wrote: > > > >> > > > On Tue, Jan 10, 2012 at 03:01:50PM +0800, Richard Zhao wrote: > > > >> > > > > Signed-off-by: Richard Zhao <richard.zhao@linaro.org> > > > >> > > > > --- > > > >> > > > > > > >> > > > I think it deserves a sensible commit message explaining why the patch > > > >> > > > is needed. > > > >> > > If event_id0 < 32, 1 << (sdmac->event_id0 - 32) is not zero. > > > >> This meant to make you clear about the patch. I'll add it in commit > > > >> message. > > > > unsigned int t = 31; > > > > printf("%d %08x\n", t, 1 << (t-32)); > > > > > > > > I test above code on both x86 and arm. They shows different results. > > > > x86: 31 80000000 > > > > arm: 31 00000000 > > > > > > > > I think we still need this patch. we shoud not let it depends on gcc's > > > > behavior. > > > > > > > > Thanks > > > > Richard > > > >> > > > > > >> > My point is you may explain the exact problem you are seeing without > > > >> > this patch > > > >> The kernel don't have event_id < 32 case yet. I found the bug when > > > >> I review the code. > > > >> > and how the patch helps here. In general, doing so would > > > >> > win a warm feeling from reviewers much more easily than leaving the > > > >> > commit message empty there. > > > >> I understand your point that comment as much as possible. > > > >> > > > > > > Shawn, > > > > > > I think Richard has made the issue quite clear here, the original > > > code does seem to have some problems even to me, who do not > > > understand the very details of the SDMA: > > > > > > - sdmac->event_mask0 = 1 << sdmac->event_id0; > > > - sdmac->event_mask1 = 1 << (sdmac->event_id0 - 32); > > > > > > 1. if sdmac->event_id0 >= 32, which will cause event_mask0 to be incorrect > > > 2. if sdmac->event_id < 32, sdmac->event_mask1 will be incorrect > > > > > > An alternate way is to use the standard bit operations: > > > > > > struct sdma_channel { > > > > > > ... > > > > > > unsigned long event_mask[2]; > > > > > > ... > > > }; > > > > > > set_bit(sdmac->event_id0, event_mask); > > > > > > Which avoids branch instructions and add a bit protection for the operation > > > to be atomic enough (event_mask0/1 won't be inconsistent). > > It's a good idea. > I'm not sure whether I can always use bitops for every bit operation case, > event it don't need atomic. bitops has locks to be atomic. I'll use non-atomic bit ops, __set_bit etc. > > Thanks > Richard > > Thanks > > Richard > > > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 6/6] dma/imx-sdma: check whether event_id0 < 32 when set event_mask 2012-01-11 6:37 ` Eric Miao 2012-01-11 12:35 ` Richard Zhao @ 2012-01-11 13:16 ` Shawn Guo 2012-01-11 13:09 ` Richard Zhao 1 sibling, 1 reply; 22+ messages in thread From: Shawn Guo @ 2012-01-11 13:16 UTC (permalink / raw) To: Eric Miao Cc: Richard Zhao, patches, vinod.koul, linux-kernel, kernel, dan.j.williams, Richard Zhao, linux-arm-kernel On Wed, Jan 11, 2012 at 02:37:08PM +0800, Eric Miao wrote: > I think Richard has made the issue quite clear here, the original Yes, he has made it clear, but only after I asked for more comments, not with the empty commit message. > code does seem to have some problems even to me, who do not > understand the very details of the SDMA: > > - sdmac->event_mask0 = 1 << sdmac->event_id0; > - sdmac->event_mask1 = 1 << (sdmac->event_id0 - 32); > > 1. if sdmac->event_id0 >= 32, which will cause event_mask0 to be incorrect > 2. if sdmac->event_id < 32, sdmac->event_mask1 will be incorrect > My testing tells this is not the case. The event_mask0 will be 0 in case 1) and event_mask1 will be 0 in case 2), which is quite what we expect. And I do not believe you will see any functionality bug with the existing code. See, that's why we need verbose commit message to make the patch and the problem it's trying to address very clear. Regards, Shawn ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 6/6] dma/imx-sdma: check whether event_id0 < 32 when set event_mask 2012-01-11 13:16 ` Shawn Guo @ 2012-01-11 13:09 ` Richard Zhao 2012-01-11 13:35 ` Shawn Guo 0 siblings, 1 reply; 22+ messages in thread From: Richard Zhao @ 2012-01-11 13:09 UTC (permalink / raw) To: Shawn Guo Cc: Eric Miao, patches, vinod.koul, linux-kernel, kernel, dan.j.williams, Richard Zhao, linux-arm-kernel On Wed, Jan 11, 2012 at 09:16:17PM +0800, Shawn Guo wrote: > On Wed, Jan 11, 2012 at 02:37:08PM +0800, Eric Miao wrote: > > I think Richard has made the issue quite clear here, the original > > Yes, he has made it clear, but only after I asked for more comments, > not with the empty commit message. > > > code does seem to have some problems even to me, who do not > > understand the very details of the SDMA: > > > > - sdmac->event_mask0 = 1 << sdmac->event_id0; > > - sdmac->event_mask1 = 1 << (sdmac->event_id0 - 32); > > > > 1. if sdmac->event_id0 >= 32, which will cause event_mask0 to be incorrect > > 2. if sdmac->event_id < 32, sdmac->event_mask1 will be incorrect > > > My testing tells this is not the case. The event_mask0 will be 0 in > case 1) and event_mask1 will be 0 in case 2), which is quite what we > expect. And I do not believe you will see any functionality bug with > the existing code. Please see my mail mentioned "we shoud not let it depends on gcc's behavior." > > See, that's why we need verbose commit message to make the patch and > the problem it's trying to address very clear. I never doubt it. Thanks Richard > > Regards, > Shawn > ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 6/6] dma/imx-sdma: check whether event_id0 < 32 when set event_mask 2012-01-11 13:09 ` Richard Zhao @ 2012-01-11 13:35 ` Shawn Guo 2012-01-11 13:48 ` Eric Miao 0 siblings, 1 reply; 22+ messages in thread From: Shawn Guo @ 2012-01-11 13:35 UTC (permalink / raw) To: Richard Zhao Cc: Eric Miao, patches, vinod.koul, linux-kernel, kernel, dan.j.williams, Richard Zhao, linux-arm-kernel On Wed, Jan 11, 2012 at 09:09:11PM +0800, Richard Zhao wrote: > On Wed, Jan 11, 2012 at 09:16:17PM +0800, Shawn Guo wrote: > > On Wed, Jan 11, 2012 at 02:37:08PM +0800, Eric Miao wrote: > > > I think Richard has made the issue quite clear here, the original > > > > Yes, he has made it clear, but only after I asked for more comments, > > not with the empty commit message. > > > > > code does seem to have some problems even to me, who do not > > > understand the very details of the SDMA: > > > > > > - sdmac->event_mask0 = 1 << sdmac->event_id0; > > > - sdmac->event_mask1 = 1 << (sdmac->event_id0 - 32); > > > > > > 1. if sdmac->event_id0 >= 32, which will cause event_mask0 to be incorrect > > > 2. if sdmac->event_id < 32, sdmac->event_mask1 will be incorrect > > > > > My testing tells this is not the case. The event_mask0 will be 0 in > > case 1) and event_mask1 will be 0 in case 2), which is quite what we > > expect. And I do not believe you will see any functionality bug with > > the existing code. > Please see my mail mentioned "we shoud not let it depends on gcc's > behavior." In this case, I would rather believe that the author is smart enough to write the code intentionally based on his good understanding on behavior of arm-gcc. -- Regards, Shawn ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 6/6] dma/imx-sdma: check whether event_id0 < 32 when set event_mask 2012-01-11 13:35 ` Shawn Guo @ 2012-01-11 13:48 ` Eric Miao 0 siblings, 0 replies; 22+ messages in thread From: Eric Miao @ 2012-01-11 13:48 UTC (permalink / raw) To: Shawn Guo Cc: Richard Zhao, patches, vinod.koul, linux-kernel, kernel, dan.j.williams, Richard Zhao, linux-arm-kernel On Wed, Jan 11, 2012 at 9:35 PM, Shawn Guo <shawn.guo@linaro.org> wrote: > On Wed, Jan 11, 2012 at 09:09:11PM +0800, Richard Zhao wrote: >> On Wed, Jan 11, 2012 at 09:16:17PM +0800, Shawn Guo wrote: >> > On Wed, Jan 11, 2012 at 02:37:08PM +0800, Eric Miao wrote: >> > > I think Richard has made the issue quite clear here, the original >> > >> > Yes, he has made it clear, but only after I asked for more comments, >> > not with the empty commit message. >> > >> > > code does seem to have some problems even to me, who do not >> > > understand the very details of the SDMA: >> > > >> > > - sdmac->event_mask0 = 1 << sdmac->event_id0; >> > > - sdmac->event_mask1 = 1 << (sdmac->event_id0 - 32); >> > > >> > > 1. if sdmac->event_id0 >= 32, which will cause event_mask0 to be incorrect >> > > 2. if sdmac->event_id < 32, sdmac->event_mask1 will be incorrect >> > > >> > My testing tells this is not the case. The event_mask0 will be 0 in >> > case 1) and event_mask1 will be 0 in case 2), which is quite what we >> > expect. And I do not believe you will see any functionality bug with >> > the existing code. >> Please see my mail mentioned "we shoud not let it depends on gcc's >> behavior." > > In this case, I would rather believe that the author is smart enough > to write the code intentionally based on his good understanding on > behavior of arm-gcc. Either using the tricky gcc behavior which _will_ vary along version changes, or using "hard to understand code" is BAD idea. We don't write good code depending on author's "smartness", we write code so that it's understandable and maintainable. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 6/6] dma/imx-sdma: check whether event_id0 < 32 when set event_mask 2012-01-11 0:53 ` Richard Zhao 2012-01-11 1:47 ` Richard Zhao @ 2012-01-11 13:11 ` Shawn Guo 1 sibling, 0 replies; 22+ messages in thread From: Shawn Guo @ 2012-01-11 13:11 UTC (permalink / raw) To: Richard Zhao Cc: Richard Zhao, patches, vinod.koul, linux-kernel, eric.miao, kernel, dan.j.williams, linux-arm-kernel On Wed, Jan 11, 2012 at 08:53:23AM +0800, Richard Zhao wrote: > The kernel don't have event_id < 32 case yet. I found the bug when > I review the code. I tested audio on imx51, in which case SSI0 TX event number is 29. I do not see any problem without applying this patch. -- Regards, Shawn ^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2012-01-12 14:24 UTC | newest] Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2012-01-10 7:01 [PATCH 1/6] dma/imx-sdma: let sdma_run_channel call sdma_enable_channel Richard Zhao 2012-01-10 7:01 ` [PATCH 2/6] dma/imx-sdma: use readl_relaxed/writel_relaxed and use writel when necessary Richard Zhao 2012-01-10 7:01 ` [PATCH 3/6] dma/imx-sdma: call sdma_set_channel_priority after sdma_request_channel Richard Zhao 2012-01-10 7:01 ` [PATCH 4/6] dma/imx-sdma: move clk_enable out of sdma_request_channel Richard Zhao 2012-01-10 7:01 ` [PATCH 5/6] dma/imx-sdma: use num_events to validate event_id0 Richard Zhao 2012-01-10 14:02 ` Dirk Behme 2012-01-10 14:16 ` Shawn Guo 2012-01-10 7:01 ` [PATCH 6/6] dma/imx-sdma: check whether event_id0 < 32 when set event_mask Richard Zhao 2012-01-10 14:20 ` Shawn Guo 2012-01-10 14:29 ` Richard Zhao 2012-01-10 15:38 ` Shawn Guo 2012-01-11 0:53 ` Richard Zhao 2012-01-11 1:47 ` Richard Zhao 2012-01-11 6:37 ` Eric Miao 2012-01-11 12:35 ` Richard Zhao 2012-01-11 12:53 ` Richard Zhao 2012-01-12 14:23 ` Richard Zhao 2012-01-11 13:16 ` Shawn Guo 2012-01-11 13:09 ` Richard Zhao 2012-01-11 13:35 ` Shawn Guo 2012-01-11 13:48 ` Eric Miao 2012-01-11 13:11 ` Shawn Guo
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).