[PATCH][RFC] ARM: mach-shmobile: setup-sh7372: fixup chclr_offset base

[PATCH][RFC] ARM: mach-shmobile: setup-sh7372: fixup chclr_offset base

[Kuninori Morimoto]

In shdma.c, chclr_write() will use (chan_reg + chclr_offset).
In sh7372 case, for example DMA0 case,
DMA1CHCLR_0 is located on 0xfe008220,
and chan_reg is started from 0xfe008020 (= sh7372_dmae0_resources).
Thus, chclr_offset should be (0x220 - 0x20) instead of 0x220.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
>> Guennadi

I guess this is bugfix patch for sh7372 dmaengine chclr_offset.
But I'm not sure detail of it.
Please check this patch, and please give this patch your acked-by
if you agree this.

 arch/arm/mach-shmobile/setup-sh7372.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/arm/mach-shmobile/setup-sh7372.c b/arch/arm/mach-shmobile/setup-sh7372.c
index 35a3a9c..dfddad8 100644
--- a/arch/arm/mach-shmobile/setup-sh7372.c
+++ b/arch/arm/mach-shmobile/setup-sh7372.c
@@ -460,7 +460,7 @@ static const struct sh_dmae_slave_config sh7372_dmae_slaves[] = {
 	},
 };
 
-#define SH7372_CHCLR 0x220
+#define SH7372_CHCLR (0x220 - 0x20)
 
 static const struct sh_dmae_channel sh7372_dmae_channels[] = {
 	{
-- 
1.7.5.4

Re: [PATCH][RFC] ARM: mach-shmobile: setup-sh7372: fixup chclr_offset base

[Guennadi Liakhovetski]

Hi Morimoto-san

On Tue, 29 May 2012, Kuninori Morimoto wrote:

> In shdma.c, chclr_write() will use (chan_reg + chclr_offset).
> In sh7372 case, for example DMA0 case,
> DMA1CHCLR_0 is located on 0xfe008220,
> and chan_reg is started from 0xfe008020 (= sh7372_dmae0_resources).
> Thus, chclr_offset should be (0x220 - 0x20) instead of 0x220.
> 
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

Yes, your patch looks correct to me. One question: have you seen a runtime 
problem, that this patch fixes? CHCLR registers are written by the driver 
only upon controller reset in a loop over all channels under a 
spin_lock_irqsave(). With this bug CHCLR registers of the channels 0, 1, 
and 5 would be skipped, i.e., would not be properly resetted. Did you 
somehow observe this problem in your tests? Which tests have you done? If 
you couldn't do many, maybe I should run some more.

Reviewed-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>

Thanks
Guennadi

> ---
> >> Guennadi
> 
> I guess this is bugfix patch for sh7372 dmaengine chclr_offset.
> But I'm not sure detail of it.
> Please check this patch, and please give this patch your acked-by
> if you agree this.
> 
>  arch/arm/mach-shmobile/setup-sh7372.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/arm/mach-shmobile/setup-sh7372.c b/arch/arm/mach-shmobile/setup-sh7372.c
> index 35a3a9c..dfddad8 100644
> --- a/arch/arm/mach-shmobile/setup-sh7372.c
> +++ b/arch/arm/mach-shmobile/setup-sh7372.c
> @@ -460,7 +460,7 @@ static const struct sh_dmae_slave_config sh7372_dmae_slaves[] = {
>  	},
>  };
>  
> -#define SH7372_CHCLR 0x220
> +#define SH7372_CHCLR (0x220 - 0x20)
>  
>  static const struct sh_dmae_channel sh7372_dmae_channels[] = {
>  	{
> -- 
> 1.7.5.4
> 

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH][RFC] ARM: mach-shmobile: setup-sh7372: fixup chclr_offset base

[Kuninori Morimoto]

Hi Guennadi

Thank you for your reply

> > In shdma.c, chclr_write() will use (chan_reg + chclr_offset).
> > In sh7372 case, for example DMA0 case,
> > DMA1CHCLR_0 is located on 0xfe008220,
> > and chan_reg is started from 0xfe008020 (= sh7372_dmae0_resources).
> > Thus, chclr_offset should be (0x220 - 0x20) instead of 0x220.
> > 
> > Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> 
> Yes, your patch looks correct to me. One question: have you seen a runtime 
> problem, that this patch fixes? CHCLR registers are written by the driver 
> only upon controller reset in a loop over all channels under a 
> spin_lock_irqsave(). With this bug CHCLR registers of the channels 0, 1, 
> and 5 would be skipped, i.e., would not be properly resetted. Did you 
> somehow observe this problem in your tests? Which tests have you done? If 
> you couldn't do many, maybe I should run some more.
> 
> Reviewed-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>

Thanks.
But sorry. I didn't have any problem.
I noticed it from source-code only, not board behavior.

Best regards
---
Kuninori Morimoto

Re: [PATCH][RFC] ARM: mach-shmobile: setup-sh7372: fixup chclr_offset base

[Guennadi Liakhovetski]

On Wed, 30 May 2012, Kuninori Morimoto wrote:

> 
> Hi Guennadi
> 
> Thank you for your reply
> 
> > > In shdma.c, chclr_write() will use (chan_reg + chclr_offset).
> > > In sh7372 case, for example DMA0 case,
> > > DMA1CHCLR_0 is located on 0xfe008220,
> > > and chan_reg is started from 0xfe008020 (= sh7372_dmae0_resources).
> > > Thus, chclr_offset should be (0x220 - 0x20) instead of 0x220.
> > > 
> > > Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> > 
> > Yes, your patch looks correct to me. One question: have you seen a runtime 
> > problem, that this patch fixes? CHCLR registers are written by the driver 
> > only upon controller reset in a loop over all channels under a 
> > spin_lock_irqsave(). With this bug CHCLR registers of the channels 0, 1, 
> > and 5 would be skipped, i.e., would not be properly resetted. Did you 
> > somehow observe this problem in your tests? Which tests have you done? If 
> > you couldn't do many, maybe I should run some more.
> > 
> > Reviewed-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> 
> Thanks.
> But sorry. I didn't have any problem.
> I noticed it from source-code only, not board behavior.

Ok, but did you at least run some tests, where this code-path would be 
entered, including (runtime-) suspend and resume? If not yet, I think, it 
would be good to at least run a couple of short tests to make sure we're 
not causing any obvious regressions. I'll try to find some time later 
today or tomorrow for this.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH][RFC] ARM: mach-shmobile: setup-sh7372: fixup chclr_offset base

[Kuninori Morimoto]

Hi Guennadi

> > Thanks.
> > But sorry. I didn't have any problem.
> > I noticed it from source-code only, not board behavior.
> 
> Ok, but did you at least run some tests, where this code-path would be 
> entered, including (runtime-) suspend and resume? If not yet, I think, it 
> would be good to at least run a couple of short tests to make sure we're 
> not causing any obvious regressions. I'll try to find some time later 
> today or tomorrow for this.

Yes. of course I tested my patch on FSI DMAEngine.
In my test, I didn't get any issue with/without this patch.

I double-checked this issue, but sh7372 has offset bug.
This is the reason why I added [RFC] on this patch.

Best regards
---
Kuninori Morimoto

Re: [PATCH][RFC] ARM: mach-shmobile: setup-sh7372: fixup chclr_offset base

[Guennadi Liakhovetski]

On Wed, 30 May 2012, Kuninori Morimoto wrote:

> Hi Guennadi
> 
> > > Thanks.
> > > But sorry. I didn't have any problem.
> > > I noticed it from source-code only, not board behavior.
> > 
> > Ok, but did you at least run some tests, where this code-path would be 
> > entered, including (runtime-) suspend and resume? If not yet, I think, it 
> > would be good to at least run a couple of short tests to make sure we're 
> > not causing any obvious regressions. I'll try to find some time later 
> > today or tomorrow for this.
> 
> Yes. of course I tested my patch on FSI DMAEngine.
> In my test, I didn't get any issue with/without this patch.
> 
> I double-checked this issue, but sh7372 has offset bug.
> This is the reason why I added [RFC] on this patch.

I tested a 9-day old next tree with and without your patch with 
runtime-suspend - both when the A3SP domain stays on and when it gets 
switched off - both versions seem to work, so, at least I don't recognise 
any regressions. Unfortunately, I couldn't test system-wide suspend 
because of some NFS problem. So, you can also add a (partially-)

Tested-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH][RFC] ARM: mach-shmobile: setup-sh7372: fixup chclr_offset base

[Rafael J. Wysocki]

On Wednesday, May 30, 2012, Guennadi Liakhovetski wrote:
> On Wed, 30 May 2012, Kuninori Morimoto wrote:
> 
> > Hi Guennadi
> > 
> > > > Thanks.
> > > > But sorry. I didn't have any problem.
> > > > I noticed it from source-code only, not board behavior.
> > > 
> > > Ok, but did you at least run some tests, where this code-path would be 
> > > entered, including (runtime-) suspend and resume? If not yet, I think, it 
> > > would be good to at least run a couple of short tests to make sure we're 
> > > not causing any obvious regressions. I'll try to find some time later 
> > > today or tomorrow for this.
> > 
> > Yes. of course I tested my patch on FSI DMAEngine.
> > In my test, I didn't get any issue with/without this patch.
> > 
> > I double-checked this issue, but sh7372 has offset bug.
> > This is the reason why I added [RFC] on this patch.
> 
> I tested a 9-day old next tree with and without your patch with 
> runtime-suspend - both when the A3SP domain stays on and when it gets 
> switched off - both versions seem to work, so, at least I don't recognise 
> any regressions. Unfortunately, I couldn't test system-wide suspend 
> because of some NFS problem. So, you can also add a (partially-)
> 
> Tested-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>

I suppose this should go into v3.5, if possible, right?

Rafael

Re: [PATCH][RFC] ARM: mach-shmobile: setup-sh7372: fixup chclr_offset base

[Guennadi Liakhovetski]

Hi Rafael

On Wed, 30 May 2012, Rafael J. Wysocki wrote:

> On Wednesday, May 30, 2012, Guennadi Liakhovetski wrote:
> > On Wed, 30 May 2012, Kuninori Morimoto wrote:
> > 
> > > Hi Guennadi
> > > 
> > > > > Thanks.
> > > > > But sorry. I didn't have any problem.
> > > > > I noticed it from source-code only, not board behavior.
> > > > 
> > > > Ok, but did you at least run some tests, where this code-path would be 
> > > > entered, including (runtime-) suspend and resume? If not yet, I think, it 
> > > > would be good to at least run a couple of short tests to make sure we're 
> > > > not causing any obvious regressions. I'll try to find some time later 
> > > > today or tomorrow for this.
> > > 
> > > Yes. of course I tested my patch on FSI DMAEngine.
> > > In my test, I didn't get any issue with/without this patch.
> > > 
> > > I double-checked this issue, but sh7372 has offset bug.
> > > This is the reason why I added [RFC] on this patch.
> > 
> > I tested a 9-day old next tree with and without your patch with 
> > runtime-suspend - both when the A3SP domain stays on and when it gets 
> > switched off - both versions seem to work, so, at least I don't recognise 
> > any regressions. Unfortunately, I couldn't test system-wide suspend 
> > because of some NFS problem. So, you can also add a (partially-)
> > 
> > Tested-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> 
> I suppose this should go into v3.5, if possible, right?

I'm not sure in fact. On the one hand the fix looks correct. On the other 
hand we haven't got any reports of this bug causing problems, that this 
patch would fix. IIUC, sh7372 is not a very widely used platform these 
days, so, pushing for 3.5 should be safe enough. Paul and Magnus certainly 
know the situation better.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH][RFC] ARM: mach-shmobile: setup-sh7372: fixup chclr_offset base

[Paul Mundt]

On Thu, May 31, 2012 at 06:10:52PM +0200, Guennadi Liakhovetski wrote:
> On Wed, 30 May 2012, Rafael J. Wysocki wrote:
> > I suppose this should go into v3.5, if possible, right?
> 
> I'm not sure in fact. On the one hand the fix looks correct. On the other 
> hand we haven't got any reports of this bug causing problems, that this 
> patch would fix. IIUC, sh7372 is not a very widely used platform these 
> days, so, pushing for 3.5 should be safe enough. Paul and Magnus certainly 
> know the situation better.
> 
Given that this is a correctness fix it should go in to 3.5 and possibly
marked for stable as well. The fact no one has reported bugs so far is
immaterial, given that few customers are running on upstream kernels
directly. The fact it's not in wide deployment helps, too.