[PATCH] pxa2xx/cpufreq: Fix DRI computation

[PATCH] pxa2xx/cpufreq: Fix DRI computation

[Marek Vasut]

The DRI field was incorrectly computed, causing various hangs and weird
behaviour on PXA2xx machines. This patch introduces the DRI computation
according to the PXA255 (January 2004) and PXA270 (MV-S301039-00 Rev. A)
datasheets.

NOTE: The CPU type is checked only once, so the code is a little bit faster now.

Signed-off-by: Marek Vasut <marek.vasut@gmail.com>
---
 arch/arm/mach-pxa/cpufreq-pxa2xx.c |   33 +++++++++++++++++++++++++++++----
 1 files changed, 29 insertions(+), 4 deletions(-)

diff --git a/arch/arm/mach-pxa/cpufreq-pxa2xx.c b/arch/arm/mach-pxa/cpufreq-pxa2xx.c
index 9e4d981..5373db0 100644
--- a/arch/arm/mach-pxa/cpufreq-pxa2xx.c
+++ b/arch/arm/mach-pxa/cpufreq-pxa2xx.c
@@ -251,17 +251,42 @@ static void init_sdram_rows(void)
 	if (mdcnfg & (MDCNFG_DE0 | MDCNFG_DE1))
 		drac0 = MDCNFG_DRAC0(mdcnfg);
 
-	sdram_rows = 1 << (11 + max(drac0, drac2));
+	sdram_rows = 11 + max(drac0, drac2);
 }
 
 static u32 mdrefr_dri(unsigned int freq)
 {
 	u32 dri = 0;
 
-	if (cpu_is_pxa25x())
-		dri = ((freq * SDRAM_TREF) / (sdram_rows * 32));
+	/* 
+	 * PXA255 (6.5.3):
+	 * 	DRI = (Refresh time / rows * memory clock frequency) / 32
+	 * PXA270 (Table 91):
+	 * 	DRI = (Refresh time / rows * memory clock frequency - 31) / 32
+	 * 
+	 * Memory clock frequency is in MHz here! (Refresh time is in ms).
+	 */
+
+	/*
+	 * SDRAM_TREF	Refresh time
+	 * freq		Memory clock frequency (in kHz)
+	 * sdram_rows	Rows
+	 *
+	 * Here we do the common part for both CPUs:
+	 * 	(Refresh time / rows * memory clock frequency)
+	 *
+	 * NOTE: We must convert freq from kHz to MHz, but we do the
+	 * multiplication prior to division to retain percision.
+	 */
+	dri = (SDRAM_TREF * freq) / (sdram_rows * 1000);
+
+	/* On PXA27x, substitute 31 from the value first (Table 91). */
 	if (cpu_is_pxa27x())
-		dri = ((freq * SDRAM_TREF) / (sdram_rows - 31)) / 32;
+		dri -= 31;
+
+	/* Finaly, divide the result by 32. */
+	dri /= 32;
+
 	return dri;
 }
 
-- 
1.7.1

[PATCH] pxa2xx/cpufreq: Fix DRI computation

[Robert Jarzmik]

Marek Vasut <marek.vasut@gmail.com> writes:

> The DRI field was incorrectly computed, causing various hangs and weird
> behaviour on PXA2xx machines. This patch introduces the DRI computation
> according to the PXA255 (January 2004) and PXA270 (MV-S301039-00 Rev. A)
> datasheets.

Hi Marek,

I'll help a bit in the review here, but I need a few days.

Meanwhile, I began the review and I have a question : did you check
MDCNFG_DRAC0() and MDCNFG_DRAC2() definitions ? As I check my TRM, it should be,
according to my manual :
+ #define MDCNFG_DRAC2(mdcnfg) (((mdcnfg) >> 19) & 0x3)
+ #define MDCNFG_DRAC0(mdcnfg) (((mdcnfg) >> 3) & 0x3)
- #define MDCNFG_DRAC2(mdcnfg) (((mdcnfg) >> 21) & 0x3)
- #define MDCNFG_DRAC0(mdcnfg) (((mdcnfg) >> 5) & 0x3)

While I was checking the calculation, that rows calculation doesn't fit. Hence I
wonder if init_sdram_rows() will be correct.
I'll send a full review soon, don't be too quick with V2 please.

Cheers.

--
Robert

[PATCH] pxa2xx/cpufreq: Fix DRI computation

[Marek Vasut]

Dne Po 7. ?ervna 2010 21:41:37 Robert Jarzmik napsal(a):
> Marek Vasut <marek.vasut@gmail.com> writes:
> > The DRI field was incorrectly computed, causing various hangs and weird
> > behaviour on PXA2xx machines. This patch introduces the DRI computation
> > according to the PXA255 (January 2004) and PXA270 (MV-S301039-00 Rev. A)
> > datasheets.
> 
> Hi Marek,
> 
> I'll help a bit in the review here, but I need a few days.
> 
> Meanwhile, I began the review and I have a question : did you check
> MDCNFG_DRAC0() and MDCNFG_DRAC2() definitions ? As I check my TRM, it
> should be, according to my manual :
> + #define MDCNFG_DRAC2(mdcnfg) (((mdcnfg) >> 19) & 0x3)
> + #define MDCNFG_DRAC0(mdcnfg) (((mdcnfg) >> 3) & 0x3)
> - #define MDCNFG_DRAC2(mdcnfg) (((mdcnfg) >> 21) & 0x3)
> - #define MDCNFG_DRAC0(mdcnfg) (((mdcnfg) >> 5) & 0x3)
> 
> While I was checking the calculation, that rows calculation doesn't fit.
> Hence I wonder if init_sdram_rows() will be correct.
> I'll send a full review soon, don't be too quick with V2 please.

Robert, this is actually a good catch, I'll try with this change, maybe it'll 
stop hanging finally.

Thanks!
> 
> Cheers.
> 
> --
> Robert

[PATCH] pxa2xx/cpufreq: Fix DRI computation

[Marek Vasut]

Dne Po 7. ?ervna 2010 21:41:37 Robert Jarzmik napsal(a):
> Marek Vasut <marek.vasut@gmail.com> writes:
> > The DRI field was incorrectly computed, causing various hangs and weird
> > behaviour on PXA2xx machines. This patch introduces the DRI computation
> > according to the PXA255 (January 2004) and PXA270 (MV-S301039-00 Rev. A)
> > datasheets.
> 
> Hi Marek,
> 
> I'll help a bit in the review here, but I need a few days.
> 
> Meanwhile, I began the review and I have a question : did you check
> MDCNFG_DRAC0() and MDCNFG_DRAC2() definitions ? As I check my TRM, it
> should be, according to my manual :
> + #define MDCNFG_DRAC2(mdcnfg) (((mdcnfg) >> 19) & 0x3)
> + #define MDCNFG_DRAC0(mdcnfg) (((mdcnfg) >> 3) & 0x3)
> - #define MDCNFG_DRAC2(mdcnfg) (((mdcnfg) >> 21) & 0x3)
> - #define MDCNFG_DRAC0(mdcnfg) (((mdcnfg) >> 5) & 0x3)
> 
> While I was checking the calculation, that rows calculation doesn't fit.
> Hence I wonder if init_sdram_rows() will be correct.
> I'll send a full review soon, don't be too quick with V2 please.
> 

No actually, the definitions are correct. I just looked and what you are looking 
at is DCAC, not DRAC. Cheers
> Cheers.
> 
> --
> Robert

[PATCH] pxa2xx/cpufreq: Fix DRI computation

[Robert Jarzmik]

Marek Vasut <marek.vasut@gmail.com> writes:

> The DRI field was incorrectly computed, causing various hangs and weird
> behaviour on PXA2xx machines. This patch introduces the DRI computation
> according to the PXA255 (January 2004) and PXA270 (MV-S301039-00 Rev. A)
> datasheets.
>
> NOTE: The CPU type is checked only once, so the code is a little bit faster now.
>
> Signed-off-by: Marek Vasut <marek.vasut@gmail.com>
> ---
>  arch/arm/mach-pxa/cpufreq-pxa2xx.c |   33 +++++++++++++++++++++++++++++----
>  1 files changed, 29 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm/mach-pxa/cpufreq-pxa2xx.c b/arch/arm/mach-pxa/cpufreq-pxa2xx.c
> index 9e4d981..5373db0 100644
> --- a/arch/arm/mach-pxa/cpufreq-pxa2xx.c
> +++ b/arch/arm/mach-pxa/cpufreq-pxa2xx.c
> @@ -251,17 +251,42 @@ static void init_sdram_rows(void)
>  	if (mdcnfg & (MDCNFG_DE0 | MDCNFG_DE1))
>  		drac0 = MDCNFG_DRAC0(mdcnfg);
>  
> -	sdram_rows = 1 << (11 + max(drac0, drac2));
> +	sdram_rows = 11 + max(drac0, drac2);
I don't think that's correct.

>From bogus@does.not.exist.com  Sun Jun  6 12:36:48 2010
From: bogus@does.not.exist.com ()
Date: Sun, 06 Jun 2010 16:36:48 -0000
Subject: No subject
Message-ID: <mailman.0.1275996205.27306.linux-arm-kernel@lists.infradead.org>

that if drac == 2, then the SDRAM row is a number between 0 and (11+drac0) - 1.
That in turn means that the number of SDRAM rows is : 2 ^ (11 + drac0).
Example:
 - drac0 = 2 : row encoded on 13 bits => 2^13 rows (ie. 1 << (11 + 2)).

>  
>  static u32 mdrefr_dri(unsigned int freq)
>  {
>  	u32 dri = 0;
>  
> -	if (cpu_is_pxa25x())
> -		dri = ((freq * SDRAM_TREF) / (sdram_rows * 32));
> +	/* 
> +	 * PXA255 (6.5.3):
> +	 * 	DRI = (Refresh time / rows * memory clock frequency) / 32
> +	 * PXA270 (Table 91):
> +	 * 	DRI = (Refresh time / rows * memory clock frequency - 31) / 32
> +	 * 
> +	 * Memory clock frequency is in MHz here! (Refresh time is in ms).
> +	 */
I don't agree here either. The "freq" variable is in kHz. It comes from
"new_freq_mem = pxa_freq_settings[idx].membus;" in pxa_set_target(). The membus
value is in kHz, as in tables pxaXXX_freqs (see 104000 and 208000, second column
in pxa27x_freqs for example). As refresh time is in ms, there is no need for a
1000th multiplier.

And my understanding is that the legacy formula is incorrect, as you discovered
it. Yet it should be, IMO :
    if (cpu_is_pxa27x())
-       dri = ((freq * SDRAM_TREF) / (sdram_rows - 31)) / 32;
+       dri = ((freq * SDRAM_TREF) / sdram_rows - 31) / 32;

This is the only change to apply IMHO.

Can you confirm my analysis ?

-- 
Robert

[PATCH] pxa2xx/cpufreq: Fix DRI computation

[Marek Vasut]

Dne ?t 8. ?ervna 2010 13:23:06 Robert Jarzmik napsal(a):
> Marek Vasut <marek.vasut@gmail.com> writes:
> > The DRI field was incorrectly computed, causing various hangs and weird
> > behaviour on PXA2xx machines. This patch introduces the DRI computation
> > according to the PXA255 (January 2004) and PXA270 (MV-S301039-00 Rev. A)
> > datasheets.
> > 
> > NOTE: The CPU type is checked only once, so the code is a little bit
> > faster now.
> > 
> > Signed-off-by: Marek Vasut <marek.vasut@gmail.com>
> > ---
> > 
> >  arch/arm/mach-pxa/cpufreq-pxa2xx.c |   33
> >  +++++++++++++++++++++++++++++---- 1 files changed, 29 insertions(+), 4
> >  deletions(-)
> > 
> > diff --git a/arch/arm/mach-pxa/cpufreq-pxa2xx.c
> > b/arch/arm/mach-pxa/cpufreq-pxa2xx.c index 9e4d981..5373db0 100644
> > --- a/arch/arm/mach-pxa/cpufreq-pxa2xx.c
> > +++ b/arch/arm/mach-pxa/cpufreq-pxa2xx.c
> > @@ -251,17 +251,42 @@ static void init_sdram_rows(void)
> > 
> >  	if (mdcnfg & (MDCNFG_DE0 | MDCNFG_DE1))
> >  	
> >  		drac0 = MDCNFG_DRAC0(mdcnfg);
> > 
> > -	sdram_rows = 1 << (11 + max(drac0, drac2));
> > +	sdram_rows = 11 + max(drac0, drac2);
> 
> I don't think that's correct.
> From DRACn, you learn that SDRAM row is selected from 11+DRACn bits. That
> means that if drac == 2, then the SDRAM row is a number between 0 and
> (11+drac0) - 1. That in turn means that the number of SDRAM rows is : 2 ^
> (11 + drac0). Example:
>  - drac0 = 2 : row encoded on 13 bits => 2^13 rows (ie. 1 << (11 + 2)).

That I'm aware of, but are you sure the TRM means it that way ?
> 
> >  static u32 mdrefr_dri(unsigned int freq)
> >  {
> >  
> >  	u32 dri = 0;
> > 
> > -	if (cpu_is_pxa25x())
> > -		dri = ((freq * SDRAM_TREF) / (sdram_rows * 32));
> > +	/*
> > +	 * PXA255 (6.5.3):
> > +	 * 	DRI = (Refresh time / rows * memory clock frequency) / 32
> > +	 * PXA270 (Table 91):
> > +	 * 	DRI = (Refresh time / rows * memory clock frequency - 31) / 32
> > +	 *
> > +	 * Memory clock frequency is in MHz here! (Refresh time is in ms).
> > +	 */
> 
> I don't agree here either. The "freq" variable is in kHz.

That's concerning the TRM, not the freq value. In TRM, it's in MHz.

> It comes from
> "new_freq_mem = pxa_freq_settings[idx].membus;" in pxa_set_target(). The
> membus value is in kHz, as in tables pxaXXX_freqs (see 104000 and 208000,
> second column in pxa27x_freqs for example). As refresh time is in ms,
> there is no need for a 1000th multiplier.
> 
> And my understanding is that the legacy formula is incorrect, as you
> discovered it. Yet it should be, IMO :
>     if (cpu_is_pxa27x())
> -       dri = ((freq * SDRAM_TREF) / (sdram_rows - 31)) / 32;
> +       dri = ((freq * SDRAM_TREF) / sdram_rows - 31) / 32;
> 
> This is the only change to apply IMHO.

Ok, but still, even though I fixed it your way, it doesn't fix the real problem, 
I'll recheck though, I just need a few days too.

Cheers
> 
> Can you confirm my analysis ?

[PATCH] pxa2xx/cpufreq: Fix DRI computation

[Robert Jarzmik]

Marek Vasut <marek.vasut@gmail.com> writes:
>> > @@ -251,17 +251,42 @@ static void init_sdram_rows(void)
>> > 
>> >  	if (mdcnfg & (MDCNFG_DE0 | MDCNFG_DE1))
>> >  	
>> >  		drac0 = MDCNFG_DRAC0(mdcnfg);
>> > 
>> > -	sdram_rows = 1 << (11 + max(drac0, drac2));
>> > +	sdram_rows = 11 + max(drac0, drac2);
>> 
>> I don't think that's correct.
>> From DRACn, you learn that SDRAM row is selected from 11+DRACn bits. That
>> means that if drac == 2, then the SDRAM row is a number between 0 and
>> (11+drac0) - 1. That in turn means that the number of SDRAM rows is : 2 ^
>> (11 + drac0). Example:
>>  - drac0 = 2 : row encoded on 13 bits => 2^13 rows (ie. 1 << (11 + 2)).
>
> That I'm aware of, but are you sure the TRM means it that way ?
Not entirely sure, but quite. I'll check against values in WinCE on 2 different
platforms (mioa701 and hx4700) to be sure.

>> I don't agree here either. The "freq" variable is in kHz.
>
> That's concerning the TRM, not the freq value. In TRM, it's in MHz.
In mine, there is no unit, which means that frequency is in Hz, and refresh time
in seconds (equivalent to freq in kHz and refresh in ms).
Moreover, my TRM states :
 dri = (Number of CLK_MEM cycles ? 31) / 32
     = (Refresh time / rows x memory clock frequency ? 31) / 32.
This means :
  Number of CLK_MEM cycles = Refresh time / rows x memory clock frequency
You have to balance the physical units, so :
  number (no unit) = refresh_time(s) / rows (no unit) * mem_clock_freq(hz=1/s)
If you had refresh_time in ms and mem_clock_freq in MHz, the units would not be
balanced :
  no_unit = ms / no_unit * MHz
          = 1/1000s / no_unit * 1000000 * 1/s
          = 1000 / no_unit
  => 1 = 1000
  => can't be true, can it ?

>> And my understanding is that the legacy formula is incorrect, as you
>> discovered it. Yet it should be, IMO :
>>     if (cpu_is_pxa27x())
>> -       dri = ((freq * SDRAM_TREF) / (sdram_rows - 31)) / 32;
>> +       dri = ((freq * SDRAM_TREF) / sdram_rows - 31) / 32;
>> 
>> This is the only change to apply IMHO.
>
> Ok, but still, even though I fixed it your way, it doesn't fix the real problem, 
> I'll recheck though, I just need a few days too.
Okay, even if that settles the issue on my platform, there might be something
left. If your platform runs under another OS, could you post the values of
MDREFR and MDCNFG (ie. physical range from 0x48000000 to 0x48000008) ?

Cheers.

--
Robert