pxa2xx/cpufreq: Fix DRI computation

PATCH New Marek Vasut 2 participants 7 messages Jun 5, 2010 – Jun 9, 2010

Subsystems (3)

Arm (32-bit) Arm sub-architectures PXA2xx/PXA3xx support

lore

Share & tools

lore

Marek Vasut

author

2010-06-05 00:46 UTC #0 lore raw

PATCH 1/1 pxa2xx/cpufreq: Fix DRI computation

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 <[email protected]>

---
 arch/arm/mach-pxa/cpufreq-pxa2xx.c |   33 +++++++++++++++++++++++++++++----
 1 files changed, 29 insertions(+), 4 deletions(-)

arch/arm/mach-pxa/cpufreq-pxa2xx.c +29 -4

index 9e4d981..5373db0 100644
@@ -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;
 }

Signature · 1 line

1.7.1

Robert Jarzmik

maintainer

2010-06-07 19:41 UTC #1 lore raw

PATCH 1/1 pxa2xx/cpufreq: Fix DRI computation

Quoted 4 lines · 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.

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

Marek Vasut

author

2010-06-07 21:20 UTC #2 lore raw

PATCH 1/1 pxa2xx/cpufreq: Fix DRI computation

Dne Po 7. ?ervna 2010 21:41:37 Robert Jarzmik napsal(a):

Quoted 18 lines

Quoted 4 lines · 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.
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)

Quoted 3 lines

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!

Quoted 5 lines

Cheers.
--
Robert

Marek Vasut

author

2010-06-07 21:26 UTC #3 lore raw

PATCH 1/1 pxa2xx/cpufreq: Fix DRI computation

In reply to Robert Jarzmik

Dne Po 7. ?ervna 2010 21:41:37 Robert Jarzmik napsal(a):

Quoted 18 lines

Quoted 4 lines · 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.
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)

Quoted 4 lines

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

Quoted 4 lines

Cheers.
--
Robert

Robert Jarzmik

maintainer

2010-06-08 11:23 UTC #4 lore raw

PATCH 1/1 pxa2xx/cpufreq: Fix DRI computation

In reply to Marek Vasut

Quoted 17 lines · 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 <[email protected]>
---
 arch/arm/mach-pxa/cpufreq-pxa2xx.c |   33 +++++++++++++++++++++++++++++----
 1 files changed, 29 insertions(+), 4 deletions(-)
arch/arm/mach-pxa/cpufreq-pxa2xx.c +0 -0
index 9e4d981..5373db0 100644

arch/arm/mach-pxa/cpufreq-pxa2xx.c static void init_sdram_rows(void) · line 254

Quoted 5 lines

arch/arm/mach-pxa/cpufreq-pxa2xx.c +1 -1

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

Quoted 1 line

From [email protected] Sun Jun 6 12:36:48 2010

From: [email protected] ()
Date: Sun, 06 Jun 2010 16:36:48 -0000
Subject: No subject
Message-ID: <[email protected]>

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

Quoted 5 lines

static u32 mdrefr_dri(unsigned int freq)
{
arch/arm/mach-pxa/cpufreq-pxa2xx.c +0 -0
 	u32 dri = 0;
 

arch/arm/mach-pxa/cpufreq-pxa2xx.c static void init_sdram_rows(void) · line 268

Quoted 10 lines

arch/arm/mach-pxa/cpufreq-pxa2xx.c +8 -2

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

Signature · 1 line

Robert

Marek Vasut

author

2010-06-08 19:36 UTC #5 lore raw

PATCH 1/1 pxa2xx/cpufreq: Fix DRI computation

Dne ?t 8. ?ervna 2010 13:23:06 Robert Jarzmik napsal(a):

Quoted 26 lines

Quoted 25 lines · 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 <[email protected]>
---
 arch/arm/mach-pxa/cpufreq-pxa2xx.c |   33
+++++++++++++++++++++++++++++---- 1 files changed, 29 insertions(+), 4 deletions(-)
(quoted diff) +0 -0
b/arch/arm/mach-pxa/cpufreq-pxa2xx.c index 9e4d981..5373db0 100644
arch/arm/mach-pxa/cpufreq-pxa2xx.c +0 -0
@@ -251,17 +251,42 @@ static void init_sdram_rows(void)
 
 	if (mdcnfg & (MDCNFG_DE0 | MDCNFG_DE1))
 
 		drac0 = MDCNFG_DRAC0(mdcnfg);
 

Quoted 9 lines

arch/arm/mach-pxa/cpufreq-pxa2xx.c static void init_sdram_rows(void) · line 254
Quoted 2 lines
arch/arm/mach-pxa/cpufreq-pxa2xx.c +1 -1
-	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 ?

Quoted 6 lines

Quoted 6 lines
arch/arm/mach-pxa/cpufreq-pxa2xx.c +0 -0
 static u32 mdrefr_dri(unsigned int freq)
 {
 
 	u32 dri = 0;
 

Quoted 12 lines

arch/arm/mach-pxa/cpufreq-pxa2xx.c static void init_sdram_rows(void) · line 268
Quoted 10 lines
arch/arm/mach-pxa/cpufreq-pxa2xx.c +8 -2
-	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.

Quoted 12 lines

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())
(quoted diff) +1 -1
-       dri = ((freq * SDRAM_TREF) / (sdram_rows - 31)) / 32;
+       dri = ((freq * SDRAM_TREF) / sdram_rows - 31) / 32;

Quoted 1 line

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

Quoted 2 lines

Can you confirm my analysis ?

Robert Jarzmik

maintainer

2010-06-09 13:59 UTC #6 lore raw

PATCH 1/1 pxa2xx/cpufreq: Fix DRI computation

Quoted 16 lines · Marek Vasut

Quoted 6 lines
Quoted 6 lines
arch/arm/mach-pxa/cpufreq-pxa2xx.c +0 -0
@@ -251,17 +251,42 @@ static void init_sdram_rows(void)
 
 	if (mdcnfg & (MDCNFG_DE0 | MDCNFG_DE1))
 
 		drac0 = MDCNFG_DRAC0(mdcnfg);
 
Quoted 9 lines
arch/arm/mach-pxa/cpufreq-pxa2xx.c static void init_sdram_rows(void) · line 254
Quoted 2 lines
arch/arm/mach-pxa/cpufreq-pxa2xx.c +1 -1
-	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)).

Quoted 1 line

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.

Quoted 3 lines

Quoted 1 line
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 ?

Quoted 10 lines

Quoted 7 lines
And my understanding is that the legacy formula is incorrect, as you discovered it. Yet it should be, IMO :
    if (cpu_is_pxa27x())
(quoted diff) +1 -1
-       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