Re: cpufreq/linux/arch/arm/mach-sa1100 cpu-sa1100.c, 1.5,

Re: cpufreq/linux/arch/arm/mach-sa1100 cpu-sa1100.c, 1.5,

[Dominik Brodowski]

> >  static struct cpufreq_driver sa1110_driver = {
> > +	.flags		= CPUFREQ_STICKY |
> > +			  CPUFREQ_PANIC_OUTOFSYNC | 		<<<<
> > +			  CPUFREQ_PANIC_RESUME_OUTOFSYNC,	<<<<
>
> Erm, _why_ ?
>
> This makes very little sense.  If you put the system to sleep at
> 147MHz and it normally boots at 206.4MHz, you'll resume at 206.4MHz.

I added PANIC_OUTOFSYNC because of the following concern: if the CPU 
clock is different from what the kernel thinks it is, the RAM and LCD
and PCMCIA and ... timings might be horribly broken, so better bail out as
soon as this critical state is detetected.

With regard to PANIC_RESUME_OUTOFSYNC:

> It's up to the kernel to reset the clock rate itself.  This is nothing
> new - cpufreq has always done this, so why are we adding this new
> restriction?

cpufreq still does this. However not during sysdev-resume state [because
notifiers can sleep], but later. This means the problematic discrepancy (see
above) is there, just for a short period of time. If you think it isn't an
issue, Bruno's patch to remove both flags is perfectly valid, otherwise
these flags do make some sense.

> And to follow that up, yes, I missed it back in June when I added
> the CPUFREQ_STICKY here.  However, I was completely unaware that
> this change had been made back in May 2004.
>
> Can we please route changes to architecture files via the architecture
> people _before_ merging them.  Pretty please?

Not sending this patch to you for discussion was an oversight, and I
apologize for not having done so. After the first patch series [April 11th]
didn't generate negative feedback, I -- incorrectly -- assumed everything
was in order and asked Dave to merge.
 
	Dominik


NB: sorry for the out-of-thread reply. Delivery of cpufreq list e-mail to my
address stopped during a vacation because my ISP bounced too much (spam)
mail.

Re: cpufreq/linux/arch/arm/mach-sa1100 cpu-sa1100.c, 1.5,

[Bruno Ducrot]

Hi Dominik,

On Sun, Aug 29, 2004 at 02:49:36PM +0200, Dominik Brodowski wrote:
> > >  static struct cpufreq_driver sa1110_driver = {
> > > +	.flags		= CPUFREQ_STICKY |
> > > +			  CPUFREQ_PANIC_OUTOFSYNC | 		<<<<
> > > +			  CPUFREQ_PANIC_RESUME_OUTOFSYNC,	<<<<
> >
> > Erm, _why_ ?
> >
> > This makes very little sense.  If you put the system to sleep at
> > 147MHz and it normally boots at 206.4MHz, you'll resume at 206.4MHz.
> 
> I added PANIC_OUTOFSYNC because of the following concern: if the CPU 
> clock is different from what the kernel thinks it is, the RAM and LCD
> and PCMCIA and ... timings might be horribly broken, so better bail out as
> soon as this critical state is detetected.
> 
> With regard to PANIC_RESUME_OUTOFSYNC:
> 
> > It's up to the kernel to reset the clock rate itself.  This is nothing
> > new - cpufreq has always done this, so why are we adding this new
> > restriction?
> 
> cpufreq still does this. However not during sysdev-resume state [because
> notifiers can sleep], but later. This means the problematic discrepancy (see
> above) is there, just for a short period of time. If you think it isn't an
> issue, Bruno's patch to remove both flags is perfectly valid, otherwise
> these flags do make some sense.
> 
> > And to follow that up, yes, I missed it back in June when I added
> > the CPUFREQ_STICKY here.  However, I was completely unaware that
> > this change had been made back in May 2004.
> >
> > Can we please route changes to architecture files via the architecture
> > people _before_ merging them.  Pretty please?
> 
> Not sending this patch to you for discussion was an oversight, and I
> apologize for not having done so. After the first patch series [April 11th]
> didn't generate negative feedback, I -- incorrectly -- assumed everything
> was in order and asked Dave to merge.

Well, I don't see actually your points.  IIRC arm suspend/resume is only
available via apm, so if something goes wrong on this architecture, it's
much more likely a trouble of the firmware.  IIRC, the only action taken
some times ago was to set the frequency to what think the cpufreq
infractucture in the resume path, and this was clearly to make
arm folks happy.
I actually was thinking all those flags were added more for the
i386 architectures, not for arm, but I am apparently completely wrong,
I guess..

Cheers,

-- 
Bruno Ducrot

--  Which is worse:  ignorance or apathy?
--  Don't know.  Don't care.

Re: cpufreq/linux/arch/arm/mach-sa1100 cpu-sa1100.c, 1.5,

[Dominik Brodowski]

Hi Bruno,

On Mon, Aug 30, 2004 at 02:19:23PM +0200, Bruno Ducrot wrote:
> Well, I don't see actually your points.  IIRC arm suspend/resume is only
> available via apm, so if something goes wrong on this architecture, it's
> much more likely a trouble of the firmware.  IIRC, the only action taken
> some times ago was to set the frequency to what think the cpufreq
> infractucture in the resume path, and this was clearly to make
> arm folks happy.
> I actually was thinking all those flags were added more for the
> i386 architectures, not for arm, but I am apparently completely wrong,
> I guess..

i386 can adapt to "hidden" changes of CPU frequencies quite well now, as the
TSC code tries to detect "changes behind its back"... so these flags were
not added out of concern for the x86 architecture, but because they seemed
necessary for certain systems. Possibly I'm completely wrong, though...

Thanks,	
	Dominik

Re: cpufreq/linux/arch/arm/mach-sa1100 cpu-sa1100.c, 1.5,

[Russell King]

On Sun, Aug 29, 2004 at 02:49:36PM +0200, Dominik Brodowski wrote:
> With regard to PANIC_RESUME_OUTOFSYNC:
> 
> > It's up to the kernel to reset the clock rate itself.  This is nothing
> > new - cpufreq has always done this, so why are we adding this new
> > restriction?
> 
> cpufreq still does this. However not during sysdev-resume state [because
> notifiers can sleep], but later. This means the problematic discrepancy (see
> above) is there, just for a short period of time. If you think it isn't an
> issue, Bruno's patch to remove both flags is perfectly valid, otherwise
> these flags do make some sense.

Typically, the boot loader will have a fixed clock speed and RAM timing
setting.  This means that whenever we resume, we will resume not at the
speed the kernel last set, but what we normally boot at.  If we normally
boot at 206.4MHz, we will resume at 206.4MHz no matter what.

So, with PANIC_RESUME_OUTOFSYNC that seems to mean that we will _always_
panic on resume if cpufreq has been used.  Which kind'a makes cpufreq
completely useless on these platforms.

Obviously given this information, this flag needs to be removed from ARM.
If ARM is the only user, then the flag and associated code needs to be
completely removed.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 PCMCIA      - http://pcmcia.arm.linux.org.uk/
                 2.6 Serial core

Re: cpufreq/linux/arch/arm/mach-sa1100 cpu-sa1100.c, 1.5,

[Bruno Ducrot]

Hi,

On Mon, Aug 30, 2004 at 03:17:25PM +0200, Dominik Brodowski wrote:
> i386 can adapt to "hidden" changes of CPU frequencies quite well now, as the
> TSC code tries to detect "changes behind its back"... so these flags were
> not added out of concern for the x86 architecture, but because they seemed
> necessary for certain systems. Possibly I'm completely wrong, though...
> 

Well, for a strange reason, I was thinking all those stuff were made for
speedstep-ich and the (non)-merged speedstep-piix4, and maybe the
ownership stuff from ACPI..  I was mistaken I guess.

Cheers,

-- 
Bruno Ducrot

--  Which is worse:  ignorance or apathy?
--  Don't know.  Don't care.

Re: cpufreq/linux/arch/arm/mach-sa1100 cpu-sa1100.c, 1.5,

[Dominik Brodowski]

On Mon, Aug 30, 2004 at 04:49:40PM +0200, Bruno Ducrot wrote:
> Hi,
> 
> On Mon, Aug 30, 2004 at 03:17:25PM +0200, Dominik Brodowski wrote:
> > i386 can adapt to "hidden" changes of CPU frequencies quite well now, as the
> > TSC code tries to detect "changes behind its back"... so these flags were
> > not added out of concern for the x86 architecture, but because they seemed
> > necessary for certain systems. Possibly I'm completely wrong, though...
> > 
> 
> Well, for a strange reason, I was thinking all those stuff were made for
> speedstep-ich and the (non)-merged speedstep-piix4, and maybe the
> ownership stuff from ACPI..  I was mistaken I guess.

The detection of changes behind cpufreq's back yes -- but for the
speedstep-ich case, it was supposed to work around critical situations, not
to cause panics... If you see a valid point for the PANIC flag for these
drivers, please explain it to me, though.

Thanks,

	Dominik

Re: cpufreq/linux/arch/arm/mach-sa1100 cpu-sa1100.c, 1.5,

[Dominik Brodowski]

On Mon, Aug 30, 2004 at 03:29:54PM +0100, Russell King wrote:
> Obviously given this information, this flag needs to be removed from ARM.
> If ARM is the only user, then the flag and associated code needs to be
> completely removed.

Who shall "push" this patch upstreams? Dave or Russell?


The SA1100 and SA1110 platforms can handle situations well where the
CPU frequency is different to the value the cpufreq (and timing) code 
thinks it is, e.g. when resuming from sleep. So, remove the flags 
noting the opposite.

Signed-off-by: Dominik Brodowski <linux@dominikbrodowski.de>

---

 arch/arm/mach-sa1100/cpu-sa1100.c |    4 +---
 arch/arm/mach-sa1100/cpu-sa1110.c |    4 +---
 drivers/cpufreq/cpufreq.c         |    9 ---------
 include/linux/cpufreq.h           |    7 -------
 4 files changed, 2 insertions(+), 22 deletions(-)

diff -ruN linux-original/arch/arm/mach-sa1100/cpu-sa1100.c linux/arch/arm/mach-sa1100/cpu-sa1100.c
--- linux-original/arch/arm/mach-sa1100/cpu-sa1100.c	2004-08-30 16:55:43.002188392 +0200
+++ linux/arch/arm/mach-sa1100/cpu-sa1100.c	2004-08-30 16:57:09.285071400 +0200
@@ -230,9 +230,7 @@
 }
 
 static struct cpufreq_driver sa1100_driver = {
-	.flags		= CPUFREQ_STICKY |
-			  CPUFREQ_PANIC_OUTOFSYNC | 
-			  CPUFREQ_PANIC_RESUME_OUTOFSYNC,
+	.flags		= CPUFREQ_STICKY,
 	.verify		= sa11x0_verify_speed,
 	.target		= sa1100_target,
 	.get		= sa11x0_getspeed,
diff -ruN linux-original/arch/arm/mach-sa1100/cpu-sa1110.c linux/arch/arm/mach-sa1100/cpu-sa1110.c
--- linux-original/arch/arm/mach-sa1100/cpu-sa1110.c	2004-08-30 16:55:43.002188392 +0200
+++ linux/arch/arm/mach-sa1100/cpu-sa1110.c	2004-08-30 16:57:18.358692000 +0200
@@ -329,9 +329,7 @@
 }
 
 static struct cpufreq_driver sa1110_driver = {
-	.flags		= CPUFREQ_STICKY |
-			  CPUFREQ_PANIC_OUTOFSYNC | 
-			  CPUFREQ_PANIC_RESUME_OUTOFSYNC,
+	.flags		= CPUFREQ_STICKY,
 	.verify		= sa11x0_verify_speed,
 	.target		= sa1110_target,
 	.get		= sa11x0_getspeed,
diff -ruN linux-original/drivers/cpufreq/cpufreq.c linux/drivers/cpufreq/cpufreq.c
--- linux-original/drivers/cpufreq/cpufreq.c	2004-08-30 16:55:48.815304664 +0200
+++ linux/drivers/cpufreq/cpufreq.c	2004-08-30 16:58:48.196034648 +0200
@@ -157,9 +157,6 @@
 			    (likely(cpufreq_cpu_data[freqs->cpu]->cur)) &&
 			    (unlikely(freqs->old != cpufreq_cpu_data[freqs->cpu]->cur)))
 			{
-				if (cpufreq_driver->flags & CPUFREQ_PANIC_OUTOFSYNC)
-					panic("CPU Frequency is out of sync.");
-
 				printk(KERN_WARNING "Warning: CPU frequency is %u, "
 				       "cpufreq assumed %u kHz.\n", freqs->old, cpufreq_cpu_data[freqs->cpu]->cur);
 				freqs->old = cpufreq_cpu_data[freqs->cpu]->cur;
@@ -603,9 +600,6 @@
 {
 	struct cpufreq_freqs freqs;
 
-	if (cpufreq_driver->flags & CPUFREQ_PANIC_OUTOFSYNC)
-		panic("CPU Frequency is out of sync.");
-
 	printk(KERN_WARNING "Warning: CPU frequency out of sync: cpufreq and timing "
 	       "core thinks of %u, is %u kHz.\n", old_freq, new_freq);
 
@@ -696,9 +690,6 @@
 		if (unlikely(cur_freq != cpu_policy->cur)) {
 			struct cpufreq_freqs freqs;
 
-			if (cpufreq_driver->flags & CPUFREQ_PANIC_RESUME_OUTOFSYNC)
-				panic("CPU Frequency is out of sync.");
-
 			printk(KERN_WARNING "Warning: CPU frequency is %u, "
 			       "cpufreq assumed %u kHz.\n", cur_freq, cpu_policy->cur);
 
diff -ruN linux-original/include/linux/cpufreq.h linux/include/linux/cpufreq.h
--- linux-original/include/linux/cpufreq.h	2004-08-30 16:55:51.403911136 +0200
+++ linux/include/linux/cpufreq.h	2004-08-30 16:59:04.110615264 +0200
@@ -209,13 +209,6 @@
 #define CPUFREQ_CONST_LOOPS 	0x02	/* loops_per_jiffy or other kernel
 					 * "constants" aren't affected by
 					 * frequency transitions */
-#define CPUFREQ_PANIC_OUTOFSYNC	0x04	/* panic if cpufreq's opinion of
-					 * current frequency differs from
-					 * actual frequency */
-#define CPUFREQ_PANIC_RESUME_OUTOFSYNC 0x08 /* panic if cpufreq's opinion of
-					 * current frequency differs from
-					 * actual frequency on resume
-					 * from sleep. */
 
 
 int cpufreq_register_driver(struct cpufreq_driver *driver_data);

Re: cpufreq/linux/arch/arm/mach-sa1100 cpu-sa1100.c, 1.5,

[Bruno Ducrot]

> The detection of changes behind cpufreq's back yes -- but for the
> speedstep-ich case, it was supposed to work around critical situations, not
> to cause panics... If you see a valid point for the PANIC flag for these
> drivers, please explain it to me, though.
> 

My english is too bad: _all_ drivers panics but not speedstep-ich, etc.

-- 
Bruno Ducrot

--  Which is worse:  ignorance or apathy?
--  Don't know.  Don't care.

Re: cpufreq/linux/arch/arm/mach-sa1100 cpu-sa1100.c, 1.5,

[Dominik Brodowski]

On Mon, Aug 30, 2004 at 05:16:20PM +0200, Bruno Ducrot wrote:
> > The detection of changes behind cpufreq's back yes -- but for the
> > speedstep-ich case, it was supposed to work around critical situations, not
> > to cause panics... If you see a valid point for the PANIC flag for these
> > drivers, please explain it to me, though.
> > 
> 
> My english is too bad: _all_ drivers panics but not speedstep-ich, etc.

The other drivers panicing doesn't seem to be necessary... so I suggest to
remove these flags alltogether, as done in the patch sent a few minutes ago.

	Dominik

Re: cpufreq/linux/arch/arm/mach-sa1100 cpu-sa1100.c, 1.5,

[Russell King]

On Mon, Aug 30, 2004 at 05:08:31PM +0200, Dominik Brodowski wrote:
> On Mon, Aug 30, 2004 at 03:29:54PM +0100, Russell King wrote:
> > Obviously given this information, this flag needs to be removed from ARM.
> > If ARM is the only user, then the flag and associated code needs to be
> > completely removed.
> 
> Who shall "push" this patch upstreams? Dave or Russell?

The patch looks good, thanks.

I think Dave would be better - Dave has been pushing most cpufreq stuff
to Linus anyway.  Since this is a bug fix rather than a feature, it's
definite 2.6.9-rc material.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 PCMCIA      - http://pcmcia.arm.linux.org.uk/
                 2.6 Serial core

Re: cpufreq/linux/arch/arm/mach-sa1100 cpu-sa1100.c, 1.5,

[Dave Jones]

On Mon, Aug 30, 2004 at 05:16:07PM +0100, Russell King wrote:
 > On Mon, Aug 30, 2004 at 05:08:31PM +0200, Dominik Brodowski wrote:
 > > On Mon, Aug 30, 2004 at 03:29:54PM +0100, Russell King wrote:
 > > > Obviously given this information, this flag needs to be removed from ARM.
 > > > If ARM is the only user, then the flag and associated code needs to be
 > > > completely removed.
 > > 
 > > Who shall "push" this patch upstreams? Dave or Russell?
 > 
 > The patch looks good, thanks.
 > 
 > I think Dave would be better - Dave has been pushing most cpufreq stuff
 > to Linus anyway.  Since this is a bug fix rather than a feature, it's
 > definite 2.6.9-rc material.

Merged. I've a handful of other small bits I'm about to merge,
and then I'll send Linus a pull request tonight. Should make it in for -rc2
unless Linus surprises me 8-).

		Dave