[PATCH 1/6] dma/imx-sdma: let sdma_run_channel call sdma_enable_channel

[PATCH 1/6] dma/imx-sdma: let sdma_run_channel call sdma_enable_channel

[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

[PATCH 2/6] dma/imx-sdma: use readl_relaxed/writel_relaxed and use writel when necessary

[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

[PATCH 3/6] dma/imx-sdma: call sdma_set_channel_priority after sdma_request_channel

[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

[PATCH 4/6] dma/imx-sdma: move clk_enable out of sdma_request_channel

[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

[PATCH 5/6] dma/imx-sdma: use num_events to validate event_id0

[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

[PATCH 6/6] dma/imx-sdma: check whether event_id0 < 32 when set event_mask

[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

Re: [PATCH 5/6] dma/imx-sdma: use num_events to validate event_id0

[Dirk Behme]

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

Re: [PATCH 5/6] dma/imx-sdma: use num_events to validate event_id0

[Shawn Guo]

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
> 
> 

Re: [PATCH 6/6] dma/imx-sdma: check whether event_id0 < 32 when set event_mask

[Shawn Guo]

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
> 
> 

Re: [PATCH 6/6] dma/imx-sdma: check whether event_id0 < 32 when set event_mask

[Richard Zhao]

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
> > 
> > 

Re: [PATCH 6/6] dma/imx-sdma: check whether event_id0 < 32 when set event_mask

[Shawn Guo]

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

Re: [PATCH 6/6] dma/imx-sdma: check whether event_id0 < 32 when set event_mask

[Richard Zhao]

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
> 

Re: [PATCH 6/6] dma/imx-sdma: check whether event_id0 < 32 when set event_mask

[Richard Zhao]

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
> 

Re: [PATCH 6/6] dma/imx-sdma: check whether event_id0 < 32 when set event_mask

[Eric Miao]

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).

Re: [PATCH 6/6] dma/imx-sdma: check whether event_id0 < 32 when set event_mask

[Richard Zhao]

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

Re: [PATCH 6/6] dma/imx-sdma: check whether event_id0 < 32 when set event_mask

[Richard Zhao]

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
> 

Re: [PATCH 6/6] dma/imx-sdma: check whether event_id0 < 32 when set event_mask

[Richard Zhao]

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
> 

Re: [PATCH 6/6] dma/imx-sdma: check whether event_id0 < 32 when set event_mask

[Shawn Guo]

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

Re: [PATCH 6/6] dma/imx-sdma: check whether event_id0 < 32 when set event_mask

[Shawn Guo]

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

Re: [PATCH 6/6] dma/imx-sdma: check whether event_id0 < 32 when set event_mask

[Shawn Guo]

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

Re: [PATCH 6/6] dma/imx-sdma: check whether event_id0 < 32 when set event_mask

[Eric Miao]

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.

Re: [PATCH 6/6] dma/imx-sdma: check whether event_id0 < 32 when set event_mask

[Richard Zhao]

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
>