Skip to contentSkip to conversation

pxa2xx/cpufreq: Simplify CPU frequency change asm

PATCH New Marek Vasut 5 participants 13 messages Jun 18, 2010 – Jun 28, 2010

Subsystems (4)
Arm (32-bit) Arm sub-architectures PXA2xx/PXA3xx support Pcmcia
lore
Share & tools
lore

Marek Vasut

author
#0 lore raw

PATCH 1/4 pxa2xx/cpufreq: Simplify CPU frequency change asm

This patch does the following changes to cpufreq-pxa2xx:
1) Simplifies the assembler code for frequency change
2) Reads back the MDREFR value to make sure data latched
3) Does as little operations in lock as possible

Signed-off-by Marek Vasut <[email protected]>
---
 arch/arm/mach-pxa/cpufreq-pxa2xx.c |   35 ++++++++++++++++-------------------
 1 files changed, 16 insertions(+), 19 deletions(-)
arch/arm/mach-pxa/cpufreq-pxa2xx.c +16 -19
index 9e4d981..1949850 100644
@@ -297,7 +297,7 @@ static int pxa_set_target(struct cpufreq_policy *policy,
 	unsigned int idx;
 	unsigned long flags;
 	unsigned int new_freq_cpu, new_freq_mem;
-	unsigned int unused, preset_mdrefr, postset_mdrefr, cclkcfg;
+	unsigned int preset_mdrefr, postset_mdrefr, cclkcfg;
 	int ret = 0;
 
 	/* Get the current policy */

@@ -355,29 +355,26 @@ static int pxa_set_target(struct cpufreq_policy *policy,
 		postset_mdrefr &= ~MDREFR_DB2_MASK;
 	}
 
+	/* Prepare CCLKCFG */
+	cclkcfg = pxa_freq_settings[idx].cclkcfg;
+
 	local_irq_save(flags);
 
-	/* Set new the CCCR and prepare CCLKCFG */
+	/* Set new the CCCR */
 	CCCR = pxa_freq_settings[idx].cccr;
-	cclkcfg = pxa_freq_settings[idx].cclkcfg;
 
 	asm volatile("							\n\
-		ldr	r4, [%1]		/* load MDREFR */	\n\
-		b	2f						\n\
-		.align	5						\n\
-1:									\n\
-		str	%3, [%1]		/* preset the MDREFR */	\n\
-		mcr	p14, 0, %2, c6, c0, 0	/* set CCLKCFG[FCS] */	\n\
-		str	%4, [%1]		/* postset the MDREFR */ \n\
-									\n\
-		b	3f						\n\
-2:		b	1b						\n\
-3:		nop							\n\
-	  "
-		     : "=&r" (unused)
-		     : "r" (&MDREFR), "r" (cclkcfg),
-		       "r" (preset_mdrefr), "r" (postset_mdrefr)
-		     : "r4", "r5");
+		.align	5		/* align to cache line */	\n\
+		str	%2, [%0]	/* preset the MDREFR */		\n\
+		ldr	r4, [%0]	/* make sure data latched */	\n\
+		mcr	p14, 0, %1, c6, c0, 0	/* set CCLKCFG[FCS] */	\n\
+		str	%3, [%0]	/* postset the MDREFR */	\n\
+		ldr	r4, [%0]	/* make sure data latched */	\n\
+	"
+		:: "r" (&MDREFR), "r" (cclkcfg),
+		   "r" (preset_mdrefr), "r" (postset_mdrefr)
+		: "r4");
+
 	local_irq_restore(flags);
 
 	/*
Signature · 1 line
1.7.1
author
#1 lore raw

PATCH 2/4 pxa2xx/cpufreq: Simplify DRI recomputation routine

This patch:
1) Simpifies the DRI recomputation routine by pulling out the common code
2) Fixes a bug in PXA27x DRI recomputation caused by incorrect parenthesis

Signed-off-by Marek Vasut <[email protected]>
---
 arch/arm/mach-pxa/cpufreq-pxa2xx.c |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)
arch/arm/mach-pxa/cpufreq-pxa2xx.c +5 -4
index 1949850..74fc4b5 100644
@@ -256,12 +256,13 @@ static void init_sdram_rows(void)
 
 static u32 mdrefr_dri(unsigned int freq)
 {
-	u32 dri = 0;
+	u32 dri = freq * SDRAM_TREF / sdram_rows;
 
-	if (cpu_is_pxa25x())
-		dri = ((freq * SDRAM_TREF) / (sdram_rows * 32));
 	if (cpu_is_pxa27x())
-		dri = ((freq * SDRAM_TREF) / (sdram_rows - 31)) / 32;
+		dri -= 31;
+
+	dri /= 32;
+
 	return dri;
 }
Signature · 1 line
1.7.1
author
#3 lore raw

PATCH 3/4 pxa2xx/cpufreq: Fix PCMCIA frequency scaling

In reply to Marek Vasut

The MCxx values must be based off memory clock, not CPU core clock.

This also fixes the bug where on some machines the LCD went crazy while using PCMCIA.

Signed-off-by Marek Vasut <[email protected]>
---
 drivers/pcmcia/pxa2xx_base.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)
drivers/pcmcia/pxa2xx_base.c +2 -3
index df4532e..f370476 100644
@@ -178,7 +178,6 @@ pxa2xx_pcmcia_frequency_change(struct soc_pcmcia_socket *skt,
 			       unsigned long val,
 			       struct cpufreq_freqs *freqs)
 {
-#warning "it's not clear if this is right since the core CPU (N) clock has no effect on the memory (L) clock"
 	switch (val) {
 	case CPUFREQ_PRECHANGE:
 		if (freqs->new > freqs->old) {

@@ -186,7 +185,7 @@ pxa2xx_pcmcia_frequency_change(struct soc_pcmcia_socket *skt,
 			       "pre-updating\n",
 			       freqs->new / 1000, (freqs->new / 100) % 10,
 			       freqs->old / 1000, (freqs->old / 100) % 10);
-			pxa2xx_pcmcia_set_mcxx(skt, freqs->new);
+			pxa2xx_pcmcia_set_timing(skt);
 		}
 		break;
 

@@ -196,7 +195,7 @@ pxa2xx_pcmcia_frequency_change(struct soc_pcmcia_socket *skt,
 			       "post-updating\n",
 			       freqs->new / 1000, (freqs->new / 100) % 10,
 			       freqs->old / 1000, (freqs->old / 100) % 10);
-			pxa2xx_pcmcia_set_mcxx(skt, freqs->new);
+			pxa2xx_pcmcia_set_timing(skt);
 		}
 		break;
 	}
Signature · 1 line
1.7.1
maintainer
#4 lore raw

PATCH 3/4 pxa2xx/cpufreq: Fix PCMCIA frequency scaling

Quoted 4 lines · Marek Vasut

The MCxx values must be based off memory clock, not CPU core clock.

This also fixes the bug where on some machines the LCD went crazy while using PCMCIA.

That looks correct to me.

Reviewed-by Robert Jarzmik <[email protected]>

Cheers.

--
Robert

author
#5 lore raw

PATCH 3/4 pxa2xx/cpufreq: Fix PCMCIA frequency scaling

Dne St 23. ?ervna 2010 19:49:59 Robert Jarzmik napsal(a):

Quoted 6 lines
Quoted 4 lines · Marek Vasut

The MCxx values must be based off memory clock, not CPU core clock.

This also fixes the bug where on some machines the LCD went crazy while using PCMCIA.

Quoted 8 lines

That looks correct to me.

Reviewed-by Robert Jarzmik <[email protected]>

Cheers.

--
Robert

Actually lemme rethink this ... it might still be incorrect.

#6 lore raw

PATCH 3/4 pxa2xx/cpufreq: Fix PCMCIA frequency scaling

Quoted 9 lines · Marek Vasut

Dne St 23. ?ervna 2010 19:49:59 Robert Jarzmik napsal(a):

Quoted 8 lines
Quoted 4 lines · Marek Vasut

The MCxx values must be based off memory clock, not CPU core clock.

This also fixes the bug where on some machines the LCD went crazy while using PCMCIA.

That looks correct to me.

Quoted 8 lines
Quoted 6 lines
Reviewed-by Robert Jarzmik <[email protected]>

Cheers.

--
Robert

Actually lemme rethink this ... it might still be incorrect.

As Nicolas was the one who originally wrote that code, while he was working for Montavista and interacting with Intel, I'd be surprised if he didn't try finding out what the right answer is to that fixme.

I suspect as the fixme remained that there was no clear answer.

Nicolas, can you shed any light on this:

static int
pxa2xx_pcmcia_frequency_change(struct soc_pcmcia_socket *skt,
unsigned long val,
struct cpufreq_freqs *freqs)
{
#warning "it's not clear if this is right since the core CPU (N) clock has no effect on the memory (L) clock"

author
#7 lore raw

PATCH 3/4 pxa2xx/cpufreq: Fix PCMCIA frequency scaling

Dne ?t 24. ?ervna 2010 21:12:08 Russell King - ARM Linux napsal(a):

Quoted 10 lines · Marek Vasut
Quoted 9 lines

Dne St 23. ?ervna 2010 19:49:59 Robert Jarzmik napsal(a):

Quoted 8 lines
Quoted 4 lines · Marek Vasut

The MCxx values must be based off memory clock, not CPU core clock.

This also fixes the bug where on some machines the LCD went crazy while using PCMCIA.

That looks correct to me.

Quoted 8 lines
Quoted 8 lines
Quoted 6 lines
Reviewed-by Robert Jarzmik <[email protected]>

Cheers.

--
Robert

Actually lemme rethink this ... it might still be incorrect.

Rethought ... well I think there's no problem with this. Anyway, thanks for CCing Nico.

This patch fixed an issue on zaurus so I believe it's ok.

Quoted 6 lines

As Nicolas was the one who originally wrote that code, while he was working for Montavista and interacting with Intel, I'd be surprised if he didn't try finding out what the right answer is to that fixme.

I suspect as the fixme remained that there was no clear answer.

Well if you look into some schematics, you see the pxa pcmcia is connected to the memory bus ... as other peripherals.

Quoted 10 lines

Nicolas, can you shed any light on this:

static int
pxa2xx_pcmcia_frequency_change(struct soc_pcmcia_socket *skt,
unsigned long val,
struct cpufreq_freqs *freqs)
{
#warning "it's not clear if this is right since the core CPU (N) clock has no effect on the memory (L) clock"

Cheers

PS. Russell ... feel like doing some politics again? ;-) (take it as a joke please)

#8 lore raw

PATCH 3/4 pxa2xx/cpufreq: Fix PCMCIA frequency scaling

In reply to Russell King - ARM Linux
Quoted 14 lines · Russell King - ARM Linux
Quoted 13 lines · Marek Vasut

Dne St 23. ?ervna 2010 19:49:59 Robert Jarzmik napsal(a):

Quoted 12 lines
Quoted 4 lines · Marek Vasut

The MCxx values must be based off memory clock, not CPU core clock.

This also fixes the bug where on some machines the LCD went crazy while using PCMCIA.

That looks correct to me.

Reviewed-by Robert Jarzmik <[email protected]>

Cheers.

Quoted 8 lines
Quoted 4 lines
Quoted 2 lines

--
Robert

Actually lemme rethink this ... it might still be incorrect.

As Nicolas was the one who originally wrote that code, while he was working for Montavista and interacting with Intel, I'd be surprised if he didn't try finding out what the right answer is to that fixme.

Well, I might have tried, or I might not have and this simply was never revisited. But I clearly don't remember any of the context anymore.

Quoted 1 line

I suspect as the fixme remained that there was no clear answer.

Whatever works in practice is probably the best answer at this point.

Nicolas

#9 lore raw

PATCH 3/4 pxa2xx/cpufreq: Fix PCMCIA frequency scaling

Quoted 25 lines · Nicolas Pitre
Quoted 14 lines · Russell King - ARM Linux
Quoted 13 lines · Marek Vasut

Dne St 23. ?ervna 2010 19:49:59 Robert Jarzmik napsal(a):

Quoted 12 lines
Quoted 4 lines · Marek Vasut

The MCxx values must be based off memory clock, not CPU core clock.

This also fixes the bug where on some machines the LCD went crazy while using PCMCIA.

That looks correct to me.

Reviewed-by Robert Jarzmik <[email protected]>

Cheers.

Quoted 8 lines
Quoted 4 lines
Quoted 2 lines

--
Robert

Actually lemme rethink this ... it might still be incorrect.

As Nicolas was the one who originally wrote that code, while he was working for Montavista and interacting with Intel, I'd be surprised if he didn't try finding out what the right answer is to that fixme.

Quoted 7 lines

Well, I might have tried, or I might not have and this simply was never revisited. But I clearly don't remember any of the context anymore.

Quoted 1 line

I suspect as the fixme remained that there was no clear answer.

Whatever works in practice is probably the best answer at this point.

Fair enough. And this looks correct to me as well. Applied to 'fix'.

author
#10 lore raw

PATCH 4/4 pxa2xx/pcmcia: Prepare for pxa320

In reply to Marek Vasut

PXA320 also supports PCMCIA/CF interface. This patch generalizes register access in pxa2xx_pcmcia so the pxa320 is now supported as well.

Signed-off-by Marek Vasut <[email protected]>
---
 drivers/pcmcia/pxa2xx_base.c |   40 +++++++++++++++++++++++++++++++++++++---
 1 files changed, 37 insertions(+), 3 deletions(-)
drivers/pcmcia/pxa2xx_base.c +37 -3
index f370476..3d63d2f 100644
@@ -85,6 +85,12 @@
 #define MCXX_ASST_SHIFT     (7)
 #define MCXX_HOLD_SHIFT     (14)
 
+#define	PXA2XX_SMC_BASE		(0x48000000)
+#define	PXA3XX_SMC_BASE		(0x4a000000)
+#define	MCMEM_OFFSET		(0x28)
+#define	MCATT_OFFSET		(0x30)
+#define	MCIO_OFFSET		(0x38)
+
 static inline u_int pxa2xx_mcxx_hold(u_int pcmcia_cycle_ns,
 				     u_int mem_clk_10khz)
 {

@@ -115,39 +121,62 @@ static inline u_int pxa2xx_pcmcia_cmd_time(u_int mem_clk_10khz,
 	return (300000 * (pcmcia_mcxx_asst + 1) / mem_clk_10khz);
 }
 
+static inline void pxa2xx_pcmcia_set_reg( uint32_t reg, uint32_t sock,
+					uint32_t val )
+{
+	if (cpu_is_pxa2xx())
+		reg = PXA2XX_SMC_BASE | (reg + (sock << 4));
+	if (cpu_is_pxa320())
+		reg |= PXA3XX_SMC_BASE;
+
+	__REG2(reg, val);
+}
+
 static int pxa2xx_pcmcia_set_mcmem( int sock, int speed, int clock )
 {
-	MCMEM(sock) = ((pxa2xx_mcxx_setup(speed, clock)
+	uint32_t val;
+
+	val = ((pxa2xx_mcxx_setup(speed, clock)
 		& MCXX_SETUP_MASK) << MCXX_SETUP_SHIFT)
 		| ((pxa2xx_mcxx_asst(speed, clock)
 		& MCXX_ASST_MASK) << MCXX_ASST_SHIFT)
 		| ((pxa2xx_mcxx_hold(speed, clock)
 		& MCXX_HOLD_MASK) << MCXX_HOLD_SHIFT);
 
+	pxa2xx_pcmcia_set_reg(MCMEM_OFFSET, sock, val);
+
 	return 0;
 }
 
 static int pxa2xx_pcmcia_set_mcio( int sock, int speed, int clock )
 {
-	MCIO(sock) = ((pxa2xx_mcxx_setup(speed, clock)
+	uint32_t val;
+
+	val = ((pxa2xx_mcxx_setup(speed, clock)
 		& MCXX_SETUP_MASK) << MCXX_SETUP_SHIFT)
 		| ((pxa2xx_mcxx_asst(speed, clock)
 		& MCXX_ASST_MASK) << MCXX_ASST_SHIFT)
 		| ((pxa2xx_mcxx_hold(speed, clock)
 		& MCXX_HOLD_MASK) << MCXX_HOLD_SHIFT);
 
+	pxa2xx_pcmcia_set_reg(MCIO_OFFSET, sock, val);
+
 	return 0;
 }
 
 static int pxa2xx_pcmcia_set_mcatt( int sock, int speed, int clock )
 {
-	MCATT(sock) = ((pxa2xx_mcxx_setup(speed, clock)
+	uint32_t val;
+
+	val = ((pxa2xx_mcxx_setup(speed, clock)
 		& MCXX_SETUP_MASK) << MCXX_SETUP_SHIFT)
 		| ((pxa2xx_mcxx_asst(speed, clock)
 		& MCXX_ASST_MASK) << MCXX_ASST_SHIFT)
 		| ((pxa2xx_mcxx_hold(speed, clock)
 		& MCXX_HOLD_MASK) << MCXX_HOLD_SHIFT);
 
+	pxa2xx_pcmcia_set_reg(MCATT_OFFSET, sock, val);
+
 	return 0;
 }
 

@@ -276,6 +305,11 @@ static int pxa2xx_drv_pcmcia_probe(struct platform_device *dev)
 	if (!ops)
 		return -ENODEV;
 
+	if (cpu_is_pxa320() && ops->nr > 1) {
+		dev_err(&dev->dev, "pxa320 supports only one pcmcia slot");
+		return -EINVAL;
+	}
+
 	pxa2xx_drv_pcmcia_ops(ops);
 
 	sinfo = kzalloc(SKT_DEV_INFO_SIZE(ops->nr), GFP_KERNEL);
Signature · 1 line
1.7.1
maintainer
#11 lore raw

PATCH 1/4 pxa2xx/cpufreq: Simplify CPU frequency change asm

In reply to Marek Vasut
Quoted 2 lines · Marek Vasut

This patch does the following changes to cpufreq-pxa2xx:
1) Simplifies the assembler code for frequency change

OK, but did you think over what the old code was doing ?
My guess is that all the jumps were there to trigger a load into the I-Cache before playing with memory timings. Did you consider that your new assembly fits fully into a cache line, and hence is protected from memory shutdown for the frequency change period ?

If you thought this over, then I would suggest to put a comment into the code to state that the assembly bit fits fully in one cache line. If not, I don't see clearly the benefit of removing the jumps ("b 2f" and "b 1b").

And I didn't find the reference for the need of reading the MDREFR register after the write. Would you give me a pointer in the pxa27x TRM please ?

Cheers.

--
Robert

author
#12 lore raw

PATCH 1/4 pxa2xx/cpufreq: Simplify CPU frequency change asm

Dne St 23. ?ervna 2010 21:00:19 Robert Jarzmik napsal(a):

Quoted 9 lines
Quoted 2 lines · Marek Vasut

This patch does the following changes to cpufreq-pxa2xx:
1) Simplifies the assembler code for frequency change

OK, but did you think over what the old code was doing ?
My guess is that all the jumps were there to trigger a load into the I-Cache before playing with memory timings. Did you consider that your new assembly fits fully into a cache line, and hence is protected from memory shutdown for the frequency change period ?

Good point, I'll rethink this. FTTB dropped from patchqueue.

Quoted 7 lines

If you thought this over, then I would suggest to put a comment into the code to state that the assembly bit fits fully in one cache line. If not, I don't see clearly the benefit of removing the jumps ("b 2f" and "b 1b").

And I didn't find the reference for the need of reading the MDREFR register after the write. Would you give me a pointer in the pxa27x TRM please ?

I believe the written data might be stuck in the Dcache, reading it back should prevent this from happening. We certainly don't want to end with locked-up machine, which I saw already. The reason is still unknown to me though.

Cheers

Quoted 5 lines

Cheers.

--
Robert