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

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

[Marek Vasut]

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 <marek.vasut@gmail.com>
---
 arch/arm/mach-pxa/cpufreq-pxa2xx.c |   35 ++++++++++++++++-------------------
 1 files changed, 16 insertions(+), 19 deletions(-)

diff --git a/arch/arm/mach-pxa/cpufreq-pxa2xx.c b/arch/arm/mach-pxa/cpufreq-pxa2xx.c
index 9e4d981..1949850 100644
--- a/arch/arm/mach-pxa/cpufreq-pxa2xx.c
+++ b/arch/arm/mach-pxa/cpufreq-pxa2xx.c
@@ -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);
 
 	/*
-- 
1.7.1

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

[Marek Vasut]

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 <marek.vasut@gmail.com>
---
 arch/arm/mach-pxa/cpufreq-pxa2xx.c |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/arch/arm/mach-pxa/cpufreq-pxa2xx.c b/arch/arm/mach-pxa/cpufreq-pxa2xx.c
index 1949850..74fc4b5 100644
--- a/arch/arm/mach-pxa/cpufreq-pxa2xx.c
+++ b/arch/arm/mach-pxa/cpufreq-pxa2xx.c
@@ -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;
 }
 
-- 
1.7.1

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

[Robert Jarzmik]

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

> 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 <marek.vasut@gmail.com>
Acked-by: Robert Jarzmik <robert.jarzmik@free.fr>

Cheers.

--
Robert

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

[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 <marek.vasut@gmail.com>
---
 drivers/pcmcia/pxa2xx_base.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/pcmcia/pxa2xx_base.c b/drivers/pcmcia/pxa2xx_base.c
index df4532e..f370476 100644
--- a/drivers/pcmcia/pxa2xx_base.c
+++ b/drivers/pcmcia/pxa2xx_base.c
@@ -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;
 	}
-- 
1.7.1

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

[Robert Jarzmik]

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

> 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 <robert.jarzmik@free.fr>

Cheers.

--
Robert

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

[Marek Vasut]

Dne St 23. ?ervna 2010 19:49:59 Robert Jarzmik napsal(a):
> Marek Vasut <marek.vasut@gmail.com> writes:
> > 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 <robert.jarzmik@free.fr>
> 
> Cheers.
> 
> --
> Robert

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

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

[Russell King - ARM Linux]

On Thu, Jun 24, 2010 at 08:51:27PM +0200, Marek Vasut wrote:
> Dne St 23. ?ervna 2010 19:49:59 Robert Jarzmik napsal(a):
> > Marek Vasut <marek.vasut@gmail.com> writes:
> > > 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 <robert.jarzmik@free.fr>
> > 
> > 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"

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

[Marek Vasut]

Dne ?t 24. ?ervna 2010 21:12:08 Russell King - ARM Linux napsal(a):
> On Thu, Jun 24, 2010 at 08:51:27PM +0200, Marek Vasut wrote:
> > Dne St 23. ?ervna 2010 19:49:59 Robert Jarzmik napsal(a):
> > > Marek Vasut <marek.vasut@gmail.com> writes:
> > > > 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 <robert.jarzmik@free.fr>
> > > 
> > > 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.
> 
> 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.

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

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

[Nicolas Pitre]

On Thu, 24 Jun 2010, Russell King - ARM Linux wrote:

> On Thu, Jun 24, 2010 at 08:51:27PM +0200, Marek Vasut wrote:
> > Dne St 23. ?ervna 2010 19:49:59 Robert Jarzmik napsal(a):
> > > Marek Vasut <marek.vasut@gmail.com> writes:
> > > > 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 <robert.jarzmik@free.fr>
> > > 
> > > 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.

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.

> 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

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

[Eric Miao]

On Fri, Jun 25, 2010 at 3:46 AM, Nicolas Pitre <nico@fluxnic.net> wrote:
> On Thu, 24 Jun 2010, Russell King - ARM Linux wrote:
>
>> On Thu, Jun 24, 2010 at 08:51:27PM +0200, Marek Vasut wrote:
>> > Dne St 23. ?ervna 2010 19:49:59 Robert Jarzmik napsal(a):
>> > > Marek Vasut <marek.vasut@gmail.com> writes:
>> > > > 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 <robert.jarzmik@free.fr>
>> > >
>> > > 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.
>
> 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.
>
>> 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'.

[PATCH 4/4] pxa2xx/pcmcia: Prepare for pxa320

[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 <marek.vasut@gmail.com>
---
 drivers/pcmcia/pxa2xx_base.c |   40 +++++++++++++++++++++++++++++++++++++---
 1 files changed, 37 insertions(+), 3 deletions(-)

diff --git a/drivers/pcmcia/pxa2xx_base.c b/drivers/pcmcia/pxa2xx_base.c
index f370476..3d63d2f 100644
--- a/drivers/pcmcia/pxa2xx_base.c
+++ b/drivers/pcmcia/pxa2xx_base.c
@@ -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);
-- 
1.7.1

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

[Robert Jarzmik]

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

> 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

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

[Marek Vasut]

Dne St 23. ?ervna 2010 21:00:19 Robert Jarzmik napsal(a):
> Marek Vasut <marek.vasut@gmail.com> writes:
> > 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.
> 
> 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
> 
> Cheers.
> 
> --
> Robert