[patch] input: Fix CLOCK_TICK_RATE usage ... [8/13]

[patch] input: Fix CLOCK_TICK_RATE usage ... [8/13]

[Vojtech Pavlik]

You can pull this changeset from:
	bk://kernel.bkbits.net/vojtech/input

===================================================================

ChangeSet@1.1215.104.25, 2003-06-09 14:41:31+02:00, vojtech@suse.cz
  input: Change input/misc/pcspkr.c to use CLOCK_TICK_RATE instead of
  a fixed value of 1193182. And change CLOCK_TICK_RATE and several
  usages of a fixed value 1193180 to a slightly more correct value
  of 1193182. (True freq is 1.193181818181...).


 drivers/char/vt_ioctl.c           |    4 ++--
 drivers/input/gameport/gameport.c |    2 +-
 drivers/input/joystick/analog.c   |    2 +-
 drivers/input/misc/pcspkr.c       |    2 +-
 include/asm-i386/timex.h          |    2 +-
 include/asm-x86_64/timex.h        |    2 +-
 6 files changed, 7 insertions(+), 7 deletions(-)

===================================================================

diff -Nru a/drivers/char/vt_ioctl.c b/drivers/char/vt_ioctl.c
--- a/drivers/char/vt_ioctl.c	Sat Jun 14 22:23:32 2003
+++ b/drivers/char/vt_ioctl.c	Sat Jun 14 22:23:32 2003
@@ -395,7 +395,7 @@
 		if (!perm)
 			return -EPERM;
 		if (arg)
-			arg = 1193180 / arg;
+			arg = 1193182 / arg;
 		kd_mksound(arg, 0);
 		return 0;
 
@@ -412,7 +412,7 @@
 		ticks = HZ * ((arg >> 16) & 0xffff) / 1000;
 		count = ticks ? (arg & 0xffff) : 0;
 		if (count)
-			count = 1193180 / count;
+			count = 1193182 / count;
 		kd_mksound(count, ticks);
 		return 0;
 	}
diff -Nru a/drivers/input/gameport/gameport.c b/drivers/input/gameport/gameport.c
--- a/drivers/input/gameport/gameport.c	Sat Jun 14 22:23:32 2003
+++ b/drivers/input/gameport/gameport.c	Sat Jun 14 22:23:32 2003
@@ -37,7 +37,7 @@
 
 #ifdef __i386__
 
-#define DELTA(x,y)      ((y)-(x)+((y)<(x)?1193180/HZ:0))
+#define DELTA(x,y)      ((y)-(x)+((y)<(x)?1193182/HZ:0))
 #define GET_TIME(x)     do { x = get_time_pit(); } while (0)
 
 static unsigned int get_time_pit(void)
diff -Nru a/drivers/input/joystick/analog.c b/drivers/input/joystick/analog.c
--- a/drivers/input/joystick/analog.c	Sat Jun 14 22:23:32 2003
+++ b/drivers/input/joystick/analog.c	Sat Jun 14 22:23:32 2003
@@ -138,7 +138,7 @@
 
 #ifdef __i386__
 #define GET_TIME(x)	do { if (cpu_has_tsc) rdtscl(x); else x = get_time_pit(); } while (0)
-#define DELTA(x,y)	(cpu_has_tsc?((y)-(x)):((x)-(y)+((x)<(y)?1193180L/HZ:0)))
+#define DELTA(x,y)	(cpu_has_tsc?((y)-(x)):((x)-(y)+((x)<(y)?1193182L/HZ:0)))
 #define TIME_NAME	(cpu_has_tsc?"TSC":"PIT")
 static unsigned int get_time_pit(void)
 {
diff -Nru a/drivers/input/misc/pcspkr.c b/drivers/input/misc/pcspkr.c
--- a/drivers/input/misc/pcspkr.c	Sat Jun 14 22:23:32 2003
+++ b/drivers/input/misc/pcspkr.c	Sat Jun 14 22:23:32 2003
@@ -43,7 +43,7 @@
 	} 
 
 	if (value > 20 && value < 32767)
-		count = 1193182 / value;
+		count = CLOCK_TICK_RATE / value;
 	
 	spin_lock_irqsave(&i8253_beep_lock, flags);
 
diff -Nru a/include/asm-i386/timex.h b/include/asm-i386/timex.h
--- a/include/asm-i386/timex.h	Sat Jun 14 22:23:32 2003
+++ b/include/asm-i386/timex.h	Sat Jun 14 22:23:32 2003
@@ -15,7 +15,7 @@
 #ifdef CONFIG_MELAN
 #  define CLOCK_TICK_RATE 1189200 /* AMD Elan has different frequency! */
 #else
-#  define CLOCK_TICK_RATE 1193180 /* Underlying HZ */
+#  define CLOCK_TICK_RATE 1193182 /* Underlying HZ */
 #endif
 #endif
 
diff -Nru a/include/asm-x86_64/timex.h b/include/asm-x86_64/timex.h
--- a/include/asm-x86_64/timex.h	Sat Jun 14 22:23:32 2003
+++ b/include/asm-x86_64/timex.h	Sat Jun 14 22:23:32 2003
@@ -10,7 +10,7 @@
 #include <asm/msr.h>
 #include <asm/vsyscall.h>
 
-#define CLOCK_TICK_RATE	1193180 /* Underlying HZ */
+#define CLOCK_TICK_RATE	1193182 /* Underlying HZ */
 #define CLOCK_TICK_FACTOR	20	/* Factor of both 1000000 and CLOCK_TICK_RATE */
 #define FINETUNE ((((((int)LATCH * HZ - CLOCK_TICK_RATE) << SHIFT_HZ) * \
 	(1000000/CLOCK_TICK_FACTOR) / (CLOCK_TICK_RATE/CLOCK_TICK_FACTOR)) \

RE: [patch] input: Fix CLOCK_TICK_RATE usage ... [8/13]

[Riley Williams]

Hi.

 > ChangeSet@1.1215.104.25, 2003-06-09 14:41:31+02:00, vojtech@suse.cz
 >   input: Change input/misc/pcspkr.c to use CLOCK_TICK_RATE instead of
 >   a fixed value of 1193182. And change CLOCK_TICK_RATE and several
 >   usages of a fixed value 1193180 to a slightly more correct value
 >   of 1193182. (True freq is 1.193181818181...).

Is there any reason why you used CLOCK_TICK_RATE in some places and
1193182 in others ??? I can understand your using the number in the
definition of CLOCK_TICK_RATE but not in the other cases.

If I'm reading it correctly, the result is a collection of bugs on the
AMD ELAN system as that uses a different frequency (at least, according
to the last but one hunk in your patch)...

Best wishes from Riley.
---
 * Nothing as pretty as a smile, nothing as ugly as a frown.



 >  drivers/char/vt_ioctl.c           |    4 ++--
 >  drivers/input/gameport/gameport.c |    2 +-
 >  drivers/input/joystick/analog.c   |    2 +-
 >  drivers/input/misc/pcspkr.c       |    2 +-
 >  include/asm-i386/timex.h          |    2 +-
 >  include/asm-x86_64/timex.h        |    2 +-
 >  6 files changed, 7 insertions(+), 7 deletions(-)
 >
 > ===================================================================
 >
 > diff -Nru a/drivers/char/vt_ioctl.c b/drivers/char/vt_ioctl.c
 > --- a/drivers/char/vt_ioctl.c	Sat Jun 14 22:23:32 2003
 > +++ b/drivers/char/vt_ioctl.c	Sat Jun 14 22:23:32 2003
 > @@ -395,7 +395,7 @@
 >  		if (!perm)
 >  			return -EPERM;
 >  		if (arg)
 > -			arg = 1193180 / arg;
 > +			arg = 1193182 / arg;
 >  		kd_mksound(arg, 0);
 >  		return 0;
 >
 > @@ -412,7 +412,7 @@
 >  		ticks = HZ * ((arg >> 16) & 0xffff) / 1000;
 >  		count = ticks ? (arg & 0xffff) : 0;
 >  		if (count)
 > -			count = 1193180 / count;
 > +			count = 1193182 / count;
 >  		kd_mksound(count, ticks);
 >  		return 0;
 >  	}
 > diff -Nru a/drivers/input/gameport/gameport.c
 > b/drivers/input/gameport/gameport.c
 > --- a/drivers/input/gameport/gameport.c	Sat Jun 14 22:23:32 2003
 > +++ b/drivers/input/gameport/gameport.c	Sat Jun 14 22:23:32 2003
 > @@ -37,7 +37,7 @@
 >
 >  #ifdef __i386__
 >
 > -#define DELTA(x,y)      ((y)-(x)+((y)<(x)?1193180/HZ:0))
 > +#define DELTA(x,y)      ((y)-(x)+((y)<(x)?1193182/HZ:0))
 >  #define GET_TIME(x)     do { x = get_time_pit(); } while (0)
 >
 >  static unsigned int get_time_pit(void)
 > diff -Nru a/drivers/input/joystick/analog.c
 > b/drivers/input/joystick/analog.c
 > --- a/drivers/input/joystick/analog.c	Sat Jun 14 22:23:32 2003
 > +++ b/drivers/input/joystick/analog.c	Sat Jun 14 22:23:32 2003
 > @@ -138,7 +138,7 @@
 >
 >  #ifdef __i386__
 >  #define GET_TIME(x)	do { if (cpu_has_tsc) rdtscl(x); else x =
get_time_pit(); } while (0)
 > -#define DELTA(x,y)
(cpu_has_tsc?((y)-(x)):((x)-(y)+((x)<(y)?1193180L/HZ:0)))
 > +#define DELTA(x,y)
(cpu_has_tsc?((y)-(x)):((x)-(y)+((x)<(y)?1193182L/HZ:0)))
 >  #define TIME_NAME	(cpu_has_tsc?"TSC":"PIT")
 >  static unsigned int get_time_pit(void)
 >  {
 > diff -Nru a/drivers/input/misc/pcspkr.c b/drivers/input/misc/pcspkr.c
 > --- a/drivers/input/misc/pcspkr.c	Sat Jun 14 22:23:32 2003
 > +++ b/drivers/input/misc/pcspkr.c	Sat Jun 14 22:23:32 2003
 > @@ -43,7 +43,7 @@
 >  	}
 >
 >  	if (value > 20 && value < 32767)
 > -		count = 1193182 / value;
 > +		count = CLOCK_TICK_RATE / value;
 >
 >  	spin_lock_irqsave(&i8253_beep_lock, flags);
 >
 > diff -Nru a/include/asm-i386/timex.h b/include/asm-i386/timex.h
 > --- a/include/asm-i386/timex.h	Sat Jun 14 22:23:32 2003
 > +++ b/include/asm-i386/timex.h	Sat Jun 14 22:23:32 2003
 > @@ -15,7 +15,7 @@
 >  #ifdef CONFIG_MELAN
 >  #  define CLOCK_TICK_RATE 1189200 /* AMD Elan has different frequency!
*/
 >  #else
 > -#  define CLOCK_TICK_RATE 1193180 /* Underlying HZ */
 > +#  define CLOCK_TICK_RATE 1193182 /* Underlying HZ */
 >  #endif
 >  #endif
 >
 > diff -Nru a/include/asm-x86_64/timex.h b/include/asm-x86_64/timex.h
 > --- a/include/asm-x86_64/timex.h	Sat Jun 14 22:23:32 2003
 > +++ b/include/asm-x86_64/timex.h	Sat Jun 14 22:23:32 2003
 > @@ -10,7 +10,7 @@
 >  #include <asm/msr.h>
 >  #include <asm/vsyscall.h>
 >
 > -#define CLOCK_TICK_RATE	1193180 /* Underlying HZ */
 > +#define CLOCK_TICK_RATE	1193182 /* Underlying HZ */
 >  #define CLOCK_TICK_FACTOR	20	/* Factor of both 1000000 and
CLOCK_TICK_RATE */
 >  #define FINETUNE ((((((int)LATCH * HZ - CLOCK_TICK_RATE) << SHIFT_HZ) *
\
 >  	(1000000/CLOCK_TICK_FACTOR) / (CLOCK_TICK_RATE/CLOCK_TICK_FACTOR)) \

---
Outgoing mail is certified Virus Free.
Checked by AVG anti-virus system (http://www.grisoft.com).
Version: 6.0.489 / Virus Database: 288 - Release Date: 10-Jun-2003

Re: [patch] input: Fix CLOCK_TICK_RATE usage ... [8/13]

[Vojtech Pavlik]

On Sat, Jun 14, 2003 at 10:05:24PM +0100, Riley Williams wrote:
> Hi.
> 
>  > ChangeSet@1.1215.104.25, 2003-06-09 14:41:31+02:00, vojtech@suse.cz
>  >   input: Change input/misc/pcspkr.c to use CLOCK_TICK_RATE instead of
>  >   a fixed value of 1193182. And change CLOCK_TICK_RATE and several
>  >   usages of a fixed value 1193180 to a slightly more correct value
>  >   of 1193182. (True freq is 1.193181818181...).
> 
> Is there any reason why you used CLOCK_TICK_RATE in some places and
> 1193182 in others ??? I can understand your using the number in the
> definition of CLOCK_TICK_RATE but not in the other cases.

I only changed the numbers from 1193180 to 1193182 in the patch.
The presence of the number instead of CLOCK_TICK_RATE in many drivers
is most likely a bug by itself, but that'll need to be addressed in a
different patch.

The only one place where I fixed it for now is the pcspkr.c driver,
since that is the one that actually started the whole thing.

> If I'm reading it correctly, the result is a collection of bugs on the
> AMD ELAN system as that uses a different frequency (at least, according
> to the last but one hunk in your patch)...

Care to send me a patch to fix this all completely and for once?

Anyone disagrees with changing all the instances of 1193180/1193182 to
CLOCK_TICK_RATE?

> Best wishes from Riley.
> ---
>  * Nothing as pretty as a smile, nothing as ugly as a frown.
> 
> 
> 
>  >  drivers/char/vt_ioctl.c           |    4 ++--
>  >  drivers/input/gameport/gameport.c |    2 +-
>  >  drivers/input/joystick/analog.c   |    2 +-
>  >  drivers/input/misc/pcspkr.c       |    2 +-
>  >  include/asm-i386/timex.h          |    2 +-
>  >  include/asm-x86_64/timex.h        |    2 +-
>  >  6 files changed, 7 insertions(+), 7 deletions(-)
>  >
>  > ===================================================================
>  >
>  > diff -Nru a/drivers/char/vt_ioctl.c b/drivers/char/vt_ioctl.c
>  > --- a/drivers/char/vt_ioctl.c	Sat Jun 14 22:23:32 2003
>  > +++ b/drivers/char/vt_ioctl.c	Sat Jun 14 22:23:32 2003
>  > @@ -395,7 +395,7 @@
>  >  		if (!perm)
>  >  			return -EPERM;
>  >  		if (arg)
>  > -			arg = 1193180 / arg;
>  > +			arg = 1193182 / arg;
>  >  		kd_mksound(arg, 0);
>  >  		return 0;
>  >
>  > @@ -412,7 +412,7 @@
>  >  		ticks = HZ * ((arg >> 16) & 0xffff) / 1000;
>  >  		count = ticks ? (arg & 0xffff) : 0;
>  >  		if (count)
>  > -			count = 1193180 / count;
>  > +			count = 1193182 / count;
>  >  		kd_mksound(count, ticks);
>  >  		return 0;
>  >  	}
>  > diff -Nru a/drivers/input/gameport/gameport.c
>  > b/drivers/input/gameport/gameport.c
>  > --- a/drivers/input/gameport/gameport.c	Sat Jun 14 22:23:32 2003
>  > +++ b/drivers/input/gameport/gameport.c	Sat Jun 14 22:23:32 2003
>  > @@ -37,7 +37,7 @@
>  >
>  >  #ifdef __i386__
>  >
>  > -#define DELTA(x,y)      ((y)-(x)+((y)<(x)?1193180/HZ:0))
>  > +#define DELTA(x,y)      ((y)-(x)+((y)<(x)?1193182/HZ:0))
>  >  #define GET_TIME(x)     do { x = get_time_pit(); } while (0)
>  >
>  >  static unsigned int get_time_pit(void)
>  > diff -Nru a/drivers/input/joystick/analog.c
>  > b/drivers/input/joystick/analog.c
>  > --- a/drivers/input/joystick/analog.c	Sat Jun 14 22:23:32 2003
>  > +++ b/drivers/input/joystick/analog.c	Sat Jun 14 22:23:32 2003
>  > @@ -138,7 +138,7 @@
>  >
>  >  #ifdef __i386__
>  >  #define GET_TIME(x)	do { if (cpu_has_tsc) rdtscl(x); else x =
> get_time_pit(); } while (0)
>  > -#define DELTA(x,y)
> (cpu_has_tsc?((y)-(x)):((x)-(y)+((x)<(y)?1193180L/HZ:0)))
>  > +#define DELTA(x,y)
> (cpu_has_tsc?((y)-(x)):((x)-(y)+((x)<(y)?1193182L/HZ:0)))
>  >  #define TIME_NAME	(cpu_has_tsc?"TSC":"PIT")
>  >  static unsigned int get_time_pit(void)
>  >  {
>  > diff -Nru a/drivers/input/misc/pcspkr.c b/drivers/input/misc/pcspkr.c
>  > --- a/drivers/input/misc/pcspkr.c	Sat Jun 14 22:23:32 2003
>  > +++ b/drivers/input/misc/pcspkr.c	Sat Jun 14 22:23:32 2003
>  > @@ -43,7 +43,7 @@
>  >  	}
>  >
>  >  	if (value > 20 && value < 32767)
>  > -		count = 1193182 / value;
>  > +		count = CLOCK_TICK_RATE / value;
>  >
>  >  	spin_lock_irqsave(&i8253_beep_lock, flags);
>  >
>  > diff -Nru a/include/asm-i386/timex.h b/include/asm-i386/timex.h
>  > --- a/include/asm-i386/timex.h	Sat Jun 14 22:23:32 2003
>  > +++ b/include/asm-i386/timex.h	Sat Jun 14 22:23:32 2003
>  > @@ -15,7 +15,7 @@
>  >  #ifdef CONFIG_MELAN
>  >  #  define CLOCK_TICK_RATE 1189200 /* AMD Elan has different frequency!
> */
>  >  #else
>  > -#  define CLOCK_TICK_RATE 1193180 /* Underlying HZ */
>  > +#  define CLOCK_TICK_RATE 1193182 /* Underlying HZ */
>  >  #endif
>  >  #endif
>  >
>  > diff -Nru a/include/asm-x86_64/timex.h b/include/asm-x86_64/timex.h
>  > --- a/include/asm-x86_64/timex.h	Sat Jun 14 22:23:32 2003
>  > +++ b/include/asm-x86_64/timex.h	Sat Jun 14 22:23:32 2003
>  > @@ -10,7 +10,7 @@
>  >  #include <asm/msr.h>
>  >  #include <asm/vsyscall.h>
>  >
>  > -#define CLOCK_TICK_RATE	1193180 /* Underlying HZ */
>  > +#define CLOCK_TICK_RATE	1193182 /* Underlying HZ */
>  >  #define CLOCK_TICK_FACTOR	20	/* Factor of both 1000000 and
> CLOCK_TICK_RATE */
>  >  #define FINETUNE ((((((int)LATCH * HZ - CLOCK_TICK_RATE) << SHIFT_HZ) *
> \
>  >  	(1000000/CLOCK_TICK_FACTOR) / (CLOCK_TICK_RATE/CLOCK_TICK_FACTOR)) \
> 
> ---
> Outgoing mail is certified Virus Free.
> Checked by AVG anti-virus system (http://www.grisoft.com).
> Version: 6.0.489 / Virus Database: 288 - Release Date: 10-Jun-2003
> 

-- 
Vojtech Pavlik
SuSE Labs, SuSE CR

RE: [patch] input: Fix CLOCK_TICK_RATE usage ... [8/13]

[Riley Williams]

[-- Attachment #1: Type: text/plain, Size: 3206 bytes --]

Hi.

I've taken Linus out of the CC list as he'll not want to see this until
it's all sorted out...

 >>> ChangeSet@1.1215.104.25, 2003-06-09 14:41:31+02:00, vojtech@suse.cz
 >>>   input: Change input/misc/pcspkr.c to use CLOCK_TICK_RATE instead of
 >>>   a fixed value of 1193182. And change CLOCK_TICK_RATE and several
 >>>   usages of a fixed value 1193180 to a slightly more correct value
 >>>   of 1193182. (True freq is 1.193181818181...).

 >> Is there any reason why you used CLOCK_TICK_RATE in some places and
 >> 1193182 in others ??? I can understand your using the number in the
 >> definition of CLOCK_TICK_RATE but not in the other cases.

 > I only changed the numbers from 1193180 to 1193182 in the patch.
 > The presence of the number instead of CLOCK_TICK_RATE in many drivers
 > is most likely a bug by itself, but that'll need to be addressed in a
 > different patch.
 >
 > The only one place where I fixed it for now is the pcspkr.c driver,
 > since that is the one that actually started the whole thing.

 >> If I'm reading it correctly, the result is a collection of bugs on the
 >> AMD ELAN system as that uses a different frequency (at least, according
 >> to the last but one hunk in your patch)...

 > Care to send me a patch to fix this all completely and for once?

I'm not sure whether your patch was for the 2.4 or 2.5 kernels. Linus has
just released the 2.5.71 kernel which I haven't yet downloaded, but when
UI have, I'll produce a patch for that as well. Enclosed is the relevant
patch against the 2.4.21 raw kernel tree with comments here:

 1. The asm-arm version of timex.h includes an arm-subarch header that
    is presumably supposed to define the relevant CLOCK_TICK_RATE for
    each sub-arch. However, some don't. I've included a catch-all in
    timex.h that defines CLOCK_TICK_RATE as being the standard value
    you've used if it isn't defined otherwise.

    Note that with the exception of the catch-all I've introduced, the
    various arm sub-arches all use values other than 1193182 here, so
    this architecture may need further work.

 2. The IA64 arch didn't define CLOCK_TICK_RATE at all, but then used the
    1193182 value as a magic value in several files. I've inserted that
    as the definition thereof in timex.h for that arch.

 3. The PARISC version of timex.h didn't define CLOCK_TICK_RATE at all.
    Other than the magic values in several generic files, it apparently
    didn't use it either. I've defined it with the 1193182 value here.

This patch defines CLOCK_TICK_RATE for all architectures as far as I can
tell, so the result should compile fine across them all. I can only test
it for the ix86 arch though as that's all I have.

 > Anyone disagrees with changing all the instances of 1193180/1193182 to
 > CLOCK_TICK_RATE?

Other than the ARM architecture, that appears to be the value used for
all of the currently supported architectures in the 2.4 kernel series...

Best wishes from Riley.
---
 * Nothing as pretty as a smile, nothing as ugly as a frown.

---
Outgoing mail is certified Virus Free.
Checked by AVG anti-virus system (http://www.grisoft.com).
Version: 6.0.489 / Virus Database: 288 - Release Date: 10-Jun-2003

[-- Attachment #2: CLOCK_TICK_RATE.diff.bz2 --]
[-- Type: application/octet-stream, Size: 4304 bytes --]

RE: [patch] input: Fix CLOCK_TICK_RATE usage ... [8/13]

[David Mosberger]

>>>>> On Sun, 15 Jun 2003 11:51:00 +0100, "Riley Williams" <Riley@Williams.Name> said:

  Riley> 2. The IA64 arch didn't define CLOCK_TICK_RATE at all, but
  Riley> then used the 1193182 value as a magic value in several
  Riley> files. I've inserted that as the definition thereof in
  Riley> timex.h for that arch.

AFAIK, on ia64, it makes absolutely no sense at all to define this
magic CLOCK_TICK_RATE in timex.h.  There simply is nothing in the ia64
architecture that requires any timer to operate at 1193182 Hz.

If there are drivers that rely on the frequency, those drivers should
be fixed instead.

Please do not add CLOCK_TICK_RATE to the ia64 timex.h header file.

	--david

RE: [patch] input: Fix CLOCK_TICK_RATE usage ... [8/13]

[Riley Williams]

Hi David.

 > Riley> 2. The IA64 arch didn't define CLOCK_TICK_RATE at all, but
 > Riley>    then used the 1193182 value as a magic value in several
 > Riley>    files. I've inserted that as the definition thereof in
 > Riley>    timex.h for that arch.

 > AFAIK, on ia64, it makes absolutely no sense at all to define this
 > magic CLOCK_TICK_RATE in timex.h. There simply is nothing in the
 > ia64 architecture that requires any timer to operate at 1193182 Hz.

I think we're talking at cross-purposes here. The kernel includes a
timer that, amongst other things, measures how long it's been up, and
on most architectures, this is based on a hardware timer that runs at
a particular frequency. This define states what frequency that timer
runs at, nothing more nor less than that.

On most architectures, the said timer runs at 1,193,181.818181818 Hz.
However, there is absolutely nothing that states that it has to run at
that frequency. Indeed, some of the other architectures run at wildly
different frequencies from that one - varying from 25,000 Hz right up
to 40,000,000 Hz.

 > If there are drivers that rely on the frequency, those drivers
 > should be fixed instead.

There are generic drivers that rely on knowing the frequency of the
kernel timer, and those are almost certainly currently bug-ridden in
any architecture where the kernel timer doesn't run at the above
frequency simply because they currently assume it runs at that
frequency. However, ANY bugfix involves each architecture declaring
the frequency its particular kernel timer runs at, and thus requires
the CLOCK_TICK_RATE macro to be defined.

 > Please do not add CLOCK_TICK_RATE to the ia64 timex.h header file.

It needs to be declared there. The only question is regarding the
value it is defined to, and it would have to be somebody with better
knowledge of the ia64 than me who decides that. All I can do is to
post a reasonable default until such decision is made.

Best wishes from Riley.
---
 * Nothing as pretty as a smile, nothing as ugly as a frown.

---
Outgoing mail is certified Virus Free.
Checked by AVG anti-virus system (http://www.grisoft.com).
Version: 6.0.490 / Virus Database: 289 - Release Date: 16-Jun-2003

RE: [patch] input: Fix CLOCK_TICK_RATE usage ... [8/13]

[David Mosberger]

>>>>> On Tue, 17 Jun 2003 23:11:46 +0100, "Riley Williams" <Riley@Williams.Name> said:

  Riley> [CLOCK_TICK_RATE] needs to be declared there. The only
  Riley> question is regarding the value it is defined to, and it
  Riley> would have to be somebody with better knowledge of the ia64
  Riley> than me who decides that. All I can do is to post a
  Riley> reasonable default until such decision is made.

The ia64 platform architecture does not provide for such a timer and
hence there is no reasonable value that the macro could be set to.

Thanks,

	--david

Re: [patch] input: Fix CLOCK_TICK_RATE usage ... [8/13]

[Russell King]

On Tue, Jun 17, 2003 at 11:11:46PM +0100, Riley Williams wrote:
> On most architectures, the said timer runs at 1,193,181.818181818 Hz.

Wow.  That's more accurate than a highly expensive Caesium standard.
And there's one inside most architectures?  Wow, we're got a great
deal there, haven't we? 8)

>  > Please do not add CLOCK_TICK_RATE to the ia64 timex.h header file.
> 
> It needs to be declared there. The only question is regarding the
> value it is defined to, and it would have to be somebody with better
> knowledge of the ia64 than me who decides that. All I can do is to
> post a reasonable default until such decision is made.

If this is the case, we have a dilema on ARM.  CLOCK_TICK_RATE has
been, and currently remains (at Georges distaste) a variable on
some platforms.  I shudder to think what this is doing to some of
the maths in Georges new time keeping and timer code.

-- 
Russell King (rmk@arm.linux.org.uk)                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

Re: [patch] input: Fix CLOCK_TICK_RATE usage ... [8/13]

[Vojtech Pavlik]

On Tue, Jun 17, 2003 at 03:19:57PM -0700, David Mosberger wrote:

> >>>>> On Tue, 17 Jun 2003 23:11:46 +0100, "Riley Williams" <Riley@Williams.Name> said:
> 
>   Riley> [CLOCK_TICK_RATE] needs to be declared there. The only
>   Riley> question is regarding the value it is defined to, and it
>   Riley> would have to be somebody with better knowledge of the ia64
>   Riley> than me who decides that. All I can do is to post a
>   Riley> reasonable default until such decision is made.
> 
> The ia64 platform architecture does not provide for such a timer and
> hence there is no reasonable value that the macro could be set to.

It seems to be used for making beeps, even on that architecture.

-- 
Vojtech Pavlik
SuSE Labs, SuSE CR

Re: [patch] input: Fix CLOCK_TICK_RATE usage ... [8/13]

[David Mosberger]

>>>>> On Wed, 18 Jun 2003 00:21:46 +0200, Vojtech Pavlik <vojtech@suse.cz> said:

  Vojtech> It seems to be used for making beeps, even on that
  Vojtech> architecture.

Don't confuse architecture and implementation.  There are _some_
machines (implementations) which have legacy support.  But the
architecture is explicitly designed to allow for legacy-free machines,
as is the case for the hp zx1-based machines, for example.

It seems to me that input/misc/pcspkr.c is doing the Right Thing:

	count = 1193182 / value;

        spin_lock_irqsave(&i8253_beep_lock, flags);

        if (count) {
                /* enable counter 2 */
                outb_p(inb_p(0x61) | 3, 0x61);
                /* set command for counter 2, 2 byte write */
                outb_p(0xB6, 0x43);
                /* select desired HZ */
                outb_p(count & 0xff, 0x42);
                outb((count >> 8) & 0xff, 0x42);

so if a legacy speaker is present, it assumes a particular frequency.
In other words: it's a driver issue.  On ia64, this frequency
certainly has nothing to do with time-keeping and therefore doesn't
belong in timex.h.

	--david

Re: [patch] input: Fix CLOCK_TICK_RATE usage ... [8/13]

[Vojtech Pavlik]

On Tue, Jun 17, 2003 at 11:21:13PM +0100, Russell King wrote:

> On Tue, Jun 17, 2003 at 11:11:46PM +0100, Riley Williams wrote:
> > On most architectures, the said timer runs at 1,193,181.818181818 Hz.
> 
> Wow.  That's more accurate than a highly expensive Caesium standard.
> And there's one inside most architectures?  Wow, we're got a great
> deal there, haven't we? 8)

Well, it's unfortunately up to 400ppm off on most systems. Nevertheless
this is the 'official' frequency, actually it's a NTSC dotclock (14.3181818)
divided by 12.

> >  > Please do not add CLOCK_TICK_RATE to the ia64 timex.h header file.
> > 
> > It needs to be declared there. The only question is regarding the
> > value it is defined to, and it would have to be somebody with better
> > knowledge of the ia64 than me who decides that. All I can do is to
> > post a reasonable default until such decision is made.
> 
> If this is the case, we have a dilema on ARM.  CLOCK_TICK_RATE has
> been, and currently remains (at Georges distaste) a variable on
> some platforms.  I shudder to think what this is doing to some of
> the maths in Georges new time keeping and timer code.

-- 
Vojtech Pavlik
SuSE Labs, SuSE CR

Re: [patch] input: Fix CLOCK_TICK_RATE usage ... [8/13]

[Vojtech Pavlik]

On Tue, Jun 17, 2003 at 03:34:24PM -0700, David Mosberger wrote:
> >>>>> On Wed, 18 Jun 2003 00:21:46 +0200, Vojtech Pavlik <vojtech@suse.cz> said:
> 
>   Vojtech> It seems to be used for making beeps, even on that
>   Vojtech> architecture.
> 
> Don't confuse architecture and implementation.  There are _some_
> machines (implementations) which have legacy support.  But the
> architecture is explicitly designed to allow for legacy-free machines,
> as is the case for the hp zx1-based machines, for example.
> 
> It seems to me that input/misc/pcspkr.c is doing the Right Thing:
> 
> 	count = 1193182 / value;
> 
>         spin_lock_irqsave(&i8253_beep_lock, flags);
> 
>         if (count) {
>                 /* enable counter 2 */
>                 outb_p(inb_p(0x61) | 3, 0x61);
>                 /* set command for counter 2, 2 byte write */
>                 outb_p(0xB6, 0x43);
>                 /* select desired HZ */
>                 outb_p(count & 0xff, 0x42);
>                 outb((count >> 8) & 0xff, 0x42);
> 
> so if a legacy speaker is present, it assumes a particular frequency.
> In other words: it's a driver issue.  On ia64, this frequency
> certainly has nothing to do with time-keeping and therefore doesn't
> belong in timex.h.

I'm quite fine with that. However, different (sub)archs, for example the
AMD Elan CPUs have a slightly different base frequency. So it looks like
an arch-dependent #define is needed. I don't care about the location
(timex.h indeed seems inappropriate, maybe the right location is
pcspkr.c ...), or the name, but something needs to be done so that the
beeps have the same sound the same on all archs.

-- 
Vojtech Pavlik
SuSE Labs, SuSE CR

Re: [patch] input: Fix CLOCK_TICK_RATE usage ... [8/13]

[Russell King]

On Wed, Jun 18, 2003 at 12:42:33AM +0200, Vojtech Pavlik wrote:
> an arch-dependent #define is needed. I don't care about the location
> (timex.h indeed seems inappropriate, maybe the right location is
> pcspkr.c ...), or the name, but something needs to be done so that the
> beeps have the same sound the same on all archs.

This may be something to aspire to, but I don't think its achievable
given the nature of PC hardware.  Some "PC speakers" are actually
buzzers in some cases rather than real loudspeakers which give a
squark rather than a beep.

-- 
Russell King (rmk@arm.linux.org.uk)                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

Re: [patch] input: Fix CLOCK_TICK_RATE usage ... [8/13]

[Vojtech Pavlik]

On Tue, Jun 17, 2003 at 11:48:27PM +0100, Russell King wrote:
> On Wed, Jun 18, 2003 at 12:42:33AM +0200, Vojtech Pavlik wrote:
> > an arch-dependent #define is needed. I don't care about the location
> > (timex.h indeed seems inappropriate, maybe the right location is
> > pcspkr.c ...), or the name, but something needs to be done so that the
> > beeps have the same sound the same on all archs.
> 
> This may be something to aspire to, but I don't think its achievable
> given the nature of PC hardware.  Some "PC speakers" are actually
> buzzers in some cases rather than real loudspeakers which give a
> squark rather than a beep.

Ok, you're right. But at least we should try to program them to the same
beep frequency.

-- 
Vojtech Pavlik
SuSE Labs, SuSE CR

Re: [patch] input: Fix CLOCK_TICK_RATE usage ... [8/13]

[David Mosberger]

>>>>> On Wed, 18 Jun 2003 00:42:33 +0200, Vojtech Pavlik <vojtech@suse.cz> said:

  >> so if a legacy speaker is present, it assumes a particular
  >> frequency.  In other words: it's a driver issue.  On ia64, this
  >> frequency certainly has nothing to do with time-keeping and
  >> therefore doesn't belong in timex.h.

  Vojtech> I'm quite fine with that. However, different (sub)archs,
  Vojtech> for example the AMD Elan CPUs have a slightly different
  Vojtech> base frequency. So it looks like an arch-dependent #define
  Vojtech> is needed. I don't care about the location (timex.h indeed
  Vojtech> seems inappropriate, maybe the right location is pcspkr.c
  Vojtech> ...), or the name, but something needs to be done so that
  Vojtech> the beeps have the same sound the same on all archs.

Sounds much better to me.  Wouldn't something along the lines of this
make the most sense:

  #ifdef __ARCH_PIT_FREQ
  # define PIT_FREQ	__ARCH_PIT_FREQ
  #else
  # define PIT_FREQ	1193182
  #endif

After all, it seems like the vast majority of legacy-compatible
hardware _do_ use the standard frequency.

	--david

Re: [patch] input: Fix CLOCK_TICK_RATE usage ... [8/13]

[Vojtech Pavlik]

On Tue, Jun 17, 2003 at 04:08:32PM -0700, David Mosberger wrote:

> >>>>> On Wed, 18 Jun 2003 00:42:33 +0200, Vojtech Pavlik <vojtech@suse.cz> said:
> 
>   >> so if a legacy speaker is present, it assumes a particular
>   >> frequency.  In other words: it's a driver issue.  On ia64, this
>   >> frequency certainly has nothing to do with time-keeping and
>   >> therefore doesn't belong in timex.h.
> 
>   Vojtech> I'm quite fine with that. However, different (sub)archs,
>   Vojtech> for example the AMD Elan CPUs have a slightly different
>   Vojtech> base frequency. So it looks like an arch-dependent #define
>   Vojtech> is needed. I don't care about the location (timex.h indeed
>   Vojtech> seems inappropriate, maybe the right location is pcspkr.c
>   Vojtech> ...), or the name, but something needs to be done so that
>   Vojtech> the beeps have the same sound the same on all archs.
> 
> Sounds much better to me.  Wouldn't something along the lines of this
> make the most sense:
> 
>   #ifdef __ARCH_PIT_FREQ
>   # define PIT_FREQ	__ARCH_PIT_FREQ
>   #else
>   # define PIT_FREQ	1193182
>   #endif
> 
> After all, it seems like the vast majority of legacy-compatible
> hardware _do_ use the standard frequency.

Now, if this was in some nice include file, along with the definition of
the i8253 PIT spinlock, that'd be great. Because not just the beeper
driver uses the PIT, also some joystick code uses it if it is available.

-- 
Vojtech Pavlik
SuSE Labs, SuSE CR

Re: [patch] input: Fix CLOCK_TICK_RATE usage ... [8/13]

[David Mosberger]

>>>>> On Wed, 18 Jun 2003 01:14:11 +0200, Vojtech Pavlik <vojtech@suse.cz> said:

  >>  Sounds much better to me.  Wouldn't something along the lines of
  >> this make the most sense:

  >> #ifdef __ARCH_PIT_FREQ # define PIT_FREQ __ARCH_PIT_FREQ #else #
  >> define PIT_FREQ 1193182 #endif

  >> After all, it seems like the vast majority of legacy-compatible
  >> hardware _do_ use the standard frequency.

  Vojtech> Now, if this was in some nice include file, along with the
  Vojtech> definition of the i8253 PIT spinlock, that'd be
  Vojtech> great. Because not just the beeper driver uses the PIT,
  Vojtech> also some joystick code uses it if it is available.

ftape, too.  The LATCH() macro should also be moved to such a header
file, I think.  How about just creating asm/pit.h?  Only platforms
that need to (potentially) support legacy hardware would need to
define it.  E.g., on ia64, we could do:

 #ifndef _ASM_IA64_PIT_H
 #define _ASM_IA64_PIT_H

 #include <linux/config.h>

 #ifdef CONFIG_LEGACY_HW
 # define PIT_FREQ	1193182
 # define LATCH		((CLOCK_TICK_RATE + HZ/2) / HZ)
 #endif

 #endif /* _ASM_IA64_PIT_H */

This way, machines that support legacy hardware can define
CONFIG_LEGACY_HW and on others, the macro can be left undefined, so
that any attempt to compile drivers requiring legacy hw would fail to
compile upfront (much better than accessing random ports!).

	--david

Re: [patch] input: Fix CLOCK_TICK_RATE usage ... [8/13]

[Vojtech Pavlik]

On Tue, Jun 17, 2003 at 04:24:04PM -0700, David Mosberger wrote:
> >>>>> On Wed, 18 Jun 2003 01:14:11 +0200, Vojtech Pavlik <vojtech@suse.cz> said:
> 
>   >>  Sounds much better to me.  Wouldn't something along the lines of
>   >> this make the most sense:
> 
>   >> #ifdef __ARCH_PIT_FREQ # define PIT_FREQ __ARCH_PIT_FREQ #else #
>   >> define PIT_FREQ 1193182 #endif
> 
>   >> After all, it seems like the vast majority of legacy-compatible
>   >> hardware _do_ use the standard frequency.
> 
>   Vojtech> Now, if this was in some nice include file, along with the
>   Vojtech> definition of the i8253 PIT spinlock, that'd be
>   Vojtech> great. Because not just the beeper driver uses the PIT,
>   Vojtech> also some joystick code uses it if it is available.
> 
> ftape, too.  The LATCH() macro should also be moved to such a header
> file, I think.  How about just creating asm/pit.h?  Only platforms
> that need to (potentially) support legacy hardware would need to
> define it.  E.g., on ia64, we could do:
> 
>  #ifndef _ASM_IA64_PIT_H
>  #define _ASM_IA64_PIT_H
> 
>  #include <linux/config.h>
> 
>  #ifdef CONFIG_LEGACY_HW
>  # define PIT_FREQ	1193182
>  # define LATCH		((CLOCK_TICK_RATE + HZ/2) / HZ)
>  #endif
> 
>  #endif /* _ASM_IA64_PIT_H */
> 
> This way, machines that support legacy hardware can define
> CONFIG_LEGACY_HW and on others, the macro can be left undefined, so
> that any attempt to compile drivers requiring legacy hw would fail to
> compile upfront (much better than accessing random ports!).

Yes, that looks very good indeed. Riley?

-- 
Vojtech Pavlik
SuSE Labs, SuSE CR

Re: [patch] input: Fix CLOCK_TICK_RATE usage ... [8/13]

[george anzinger]

Vojtech Pavlik wrote:
> On Tue, Jun 17, 2003 at 11:21:13PM +0100, Russell King wrote:
> 
> 
>>On Tue, Jun 17, 2003 at 11:11:46PM +0100, Riley Williams wrote:
>>
>>>On most architectures, the said timer runs at 1,193,181.818181818 Hz.
>>
>>Wow.  That's more accurate than a highly expensive Caesium standard.
>>And there's one inside most architectures?  Wow, we're got a great
>>deal there, haven't we? 8)
> 
> 
> Well, it's unfortunately up to 400ppm off on most systems. Nevertheless
> this is the 'official' frequency, actually it's a NTSC dotclock (14.3181818)
> divided by 12.
> 
> 
>>> > Please do not add CLOCK_TICK_RATE to the ia64 timex.h header file.
>>>
>>>It needs to be declared there. The only question is regarding the
>>>value it is defined to, and it would have to be somebody with better
>>>knowledge of the ia64 than me who decides that. All I can do is to
>>>post a reasonable default until such decision is made.
>>
>>If this is the case, we have a dilema on ARM.  CLOCK_TICK_RATE has
>>been, and currently remains (at Georges distaste) a variable on
>>some platforms.  I shudder to think what this is doing to some of
>>the maths in Georges new time keeping and timer code.

So do I :)
> 
> 

-- 
George Anzinger   george@mvista.com
High-res-timers:  http://sourceforge.net/projects/high-res-timers/
Preemption patch: http://www.kernel.org/pub/linux/kernel/people/rml

Re: [patch] input: Fix CLOCK_TICK_RATE usage ... [8/13]

[george anzinger]

Vojtech Pavlik wrote:
> On Tue, Jun 17, 2003 at 04:24:04PM -0700, David Mosberger wrote:
> 
>>>>>>>On Wed, 18 Jun 2003 01:14:11 +0200, Vojtech Pavlik <vojtech@suse.cz> said:
>>
>>  >>  Sounds much better to me.  Wouldn't something along the lines of
>>  >> this make the most sense:
>>
>>  >> #ifdef __ARCH_PIT_FREQ # define PIT_FREQ __ARCH_PIT_FREQ #else #
>>  >> define PIT_FREQ 1193182 #endif
>>
>>  >> After all, it seems like the vast majority of legacy-compatible
>>  >> hardware _do_ use the standard frequency.
>>
>>  Vojtech> Now, if this was in some nice include file, along with the
>>  Vojtech> definition of the i8253 PIT spinlock, that'd be
>>  Vojtech> great. Because not just the beeper driver uses the PIT,
>>  Vojtech> also some joystick code uses it if it is available.
>>
>>ftape, too.  The LATCH() macro should also be moved to such a header
>>file, I think.  How about just creating asm/pit.h?  Only platforms
>>that need to (potentially) support legacy hardware would need to
>>define it.  E.g., on ia64, we could do:
>>
>> #ifndef _ASM_IA64_PIT_H
>> #define _ASM_IA64_PIT_H
>>
>> #include <linux/config.h>
>>
>> #ifdef CONFIG_LEGACY_HW
>> # define PIT_FREQ	1193182
>> # define LATCH		((CLOCK_TICK_RATE + HZ/2) / HZ)
----------------------------------^^^^^^^^^^^^^^^
should be PIT_FREQ me thinks :)

-g
>> #endif
>>
>> #endif /* _ASM_IA64_PIT_H */
>>
>>This way, machines that support legacy hardware can define
>>CONFIG_LEGACY_HW and on others, the macro can be left undefined, so
>>that any attempt to compile drivers requiring legacy hw would fail to
>>compile upfront (much better than accessing random ports!).
> 
> 
> Yes, that looks very good indeed. Riley?
> 

-- 
George Anzinger   george@mvista.com
High-res-timers:  http://sourceforge.net/projects/high-res-timers/
Preemption patch: http://www.kernel.org/pub/linux/kernel/people/rml

Re: [patch] input: Fix CLOCK_TICK_RATE usage ... [8/13]

[george anzinger]

Russell King wrote:
> On Tue, Jun 17, 2003 at 11:11:46PM +0100, Riley Williams wrote:
> 
>>On most architectures, the said timer runs at 1,193,181.818181818 Hz.
> 
> 
> Wow.  That's more accurate than a highly expensive Caesium standard.
> And there's one inside most architectures?  Wow, we're got a great
> deal there, haven't we? 8)
> 
> 
>> > Please do not add CLOCK_TICK_RATE to the ia64 timex.h header file.
>>
>>It needs to be declared there. The only question is regarding the
>>value it is defined to, and it would have to be somebody with better
>>knowledge of the ia64 than me who decides that. All I can do is to
>>post a reasonable default until such decision is made.
> 
> 
> If this is the case, we have a dilema on ARM.  CLOCK_TICK_RATE has
> been, and currently remains (at Georges distaste) a variable on
> some platforms.  I shudder to think what this is doing to some of
> the maths in Georges new time keeping and timer code.
> 
So just what is it used for?  On the x86, the math on it is used 
mostly (aside from LATCH) to figure out the actual value of 1/HZ. 
This is then used to compute a more correct TICK_NSEC which is added 
to xtime each tick.  This usage of CLOCK_TICK_RATE just "beats it up" 
to see how close the hardware can get to the requested rate of 1/HZ. 
Since this code is in time.h and timex.h, it is shared with all the archs.

I submit that if it is not used to actually compute a LATCH value for 
the 1/HZ tick, it should just be some rather large value that more or 
less represents the granularity of the hardwares ability to generate 
1/HZ ticks.  Once it gets above about 100MHZ, I think it actually 
drops out of the calculations.  The last thing we want to do at this 
point is make it a variable.  (Nor do you want to put a -E in your cc 
command and take a look at what is produced for the conversion constants.)

If it is not possible to make it a constant, I think we need to 
revisit the timer conversion code as we would not only have a LOT of 
code bloat, but it would add far too much time to the conversions.

-- 
George Anzinger   george@mvista.com
High-res-timers:  http://sourceforge.net/projects/high-res-timers/
Preemption patch: http://www.kernel.org/pub/linux/kernel/people/rml

Re: [patch] input: Fix CLOCK_TICK_RATE usage ... [8/13]

[Hollis Blanchard]

On Tuesday, Jun 17, 2003, at 18:24 US/Central, David Mosberger wrote:
>  #ifdef CONFIG_LEGACY_HW
>  # define PIT_FREQ	1193182
>  # define LATCH		((CLOCK_TICK_RATE + HZ/2) / HZ)
>  #endif
>
> This way, machines that support legacy hardware can define
> CONFIG_LEGACY_HW and on others, the macro can be left undefined, so
> that any attempt to compile drivers requiring legacy hw would fail to
> compile upfront (much better than accessing random ports!).

Is "having legacy hardware" an all-or-nothing condition for you? If 
not, a CONFIG_LEGACY_PIT might be more appropriate...

-- 
Hollis Blanchard
IBM Linux Technology Center

Re: [patch] input: Fix CLOCK_TICK_RATE usage ... [8/13]

[David Mosberger]

>>>>> On Wed, 18 Jun 2003 09:47:44 -0500, Hollis Blanchard <hollisb@us.ibm.com> said:

  Hollis> On Tuesday, Jun 17, 2003, at 18:24 US/Central, David Mosberger wrote:
  >> #ifdef CONFIG_LEGACY_HW
  >> # define PIT_FREQ	1193182
  >> # define LATCH		((CLOCK_TICK_RATE + HZ/2) / HZ)
  >> #endif

  >> This way, machines that support legacy hardware can define
  >> CONFIG_LEGACY_HW and on others, the macro can be left undefined, so
  >> that any attempt to compile drivers requiring legacy hw would fail to
  >> compile upfront (much better than accessing random ports!).

  Hollis> Is "having legacy hardware" an all-or-nothing condition for you? If
  Hollis> not, a CONFIG_LEGACY_PIT might be more appropriate...

I believe it is, though I'm not entirely certain about Intel's Tiger
platform.  If more fine-grained distinction really is needed, I
suspect we'd rather want something called CONFIG_8259_PIT.  Might be a
good idea to do this for all legacy devices.

	--david

Re: [patch] input: Fix CLOCK_TICK_RATE usage ... [8/13]

[David Mosberger]

>>>>> On Wed, 18 Jun 2003 09:47:44 -0500, Hollis Blanchard <hollisb@us.ibm.com> said:

  Hollis> On Tuesday, Jun 17, 2003, at 18:24 US/Central, David Mosberger wrote:
  >> #ifdef CONFIG_LEGACY_HW
  >> # define PIT_FREQ	1193182
  >> # define LATCH		((CLOCK_TICK_RATE + HZ/2) / HZ)
  >> #endif

  >> This way, machines that support legacy hardware can define
  >> CONFIG_LEGACY_HW and on others, the macro can be left undefined, so
  >> that any attempt to compile drivers requiring legacy hw would fail to
  >> compile upfront (much better than accessing random ports!).

  Hollis> Is "having legacy hardware" an all-or-nothing condition for you? If
  Hollis> not, a CONFIG_LEGACY_PIT might be more appropriate...

I believe it is, though I'm not entirely certain about Intel's Tiger
platform.  If more fine-grained distinction really is needed, I
suspect we'd rather want something called CONFIG_8259_PIT.  Might be a
good idea to do this for all legacy devices.

	--david

Re: [patch] input: Fix CLOCK_TICK_RATE usage ... [8/13]

[David Woodhouse]

On Tue, 2003-06-17 at 23:48, Russell King wrote:
> On Wed, Jun 18, 2003 at 12:42:33AM +0200, Vojtech Pavlik wrote:
> > an arch-dependent #define is needed. I don't care about the location
> > (timex.h indeed seems inappropriate, maybe the right location is
> > pcspkr.c ...), or the name, but something needs to be done so that the
> > beeps have the same sound the same on all archs.
> 
> This may be something to aspire to, but I don't think its achievable
> given the nature of PC hardware.  Some "PC speakers" are actually
> buzzers in some cases rather than real loudspeakers which give a
> squark rather than a beep.

They're not _that_ bad. Even on most recent hardware, mp3s played
through the PC speaker are relatively recognisable :)

-- 
dwmw2

Re: [patch] input: Fix CLOCK_TICK_RATE usage ... [8/13]

[Russell King]

On Thu, Jun 19, 2003 at 01:13:28PM +0100, David Woodhouse wrote:
> They're not _that_ bad. Even on most recent hardware, mp3s played
> through the PC speaker are relatively recognisable :)

Maybe you've just been fortunate not to come across any.

-- 
Russell King (rmk@arm.linux.org.uk)                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

RE: [patch] input: Fix CLOCK_TICK_RATE usage ... [8/13]

[Riley Williams]

Hi all.

I have no objection to anything along these lines. The basic scenario
is simply this:

 1. On ALL architectures except for IA64 and ARM there is a SINGLE
    value for CLOCK_TICK_RATE that is used by several GENERIC drivers.
    Currently, that value is used as a MAGIC NUMBER that corresponds
    to the value in the Ix86 case, which is clearly wrong.

 2. According to the IA64 people, those GENERIC drivers are apparently
    irrelevant for that architecture, so making the CORRECT change of
    replacing those magic numbers in the GENERIC drivers with the
    CLOCK_TICK_RATE value will make no difference to IA64.

 3. The ARM architecture has lots of sub-architectures, as was stated
    here. The ARM version of timex.h includes a sub-arch-specific
    header file which, for MOST of the sub-arch's, already defines
    CLOCK_TICK_RATE to what appears to be an appropriate value. The
    only change I made was to apply a catch-all to cover all of the
    sub-arch's that didn't.

The CLOCK_TICK_RATE value should be defined as a result of doing...

	#include <asm/timex.h>

...but I see absolutely nothing wrong with having that file do...

	#include <asm/arch/timex.h>

...and having that in turn include various other include files that
are specific to that architecture, as per your proposal.

Best wishes from Riley.
---
 * Nothing as pretty as a smile, nothing as ugly as a frown.


 > -----Original Message-----
 > From: linux-kernel-owner@vger.kernel.org
 > [mailto:linux-kernel-owner@vger.kernel.org]On Behalf Of Vojtech Pavlik
 > Sent: Wednesday, June 18, 2003 12:31 AM
 > To: davidm@hpl.hp.com
 > Cc: Vojtech Pavlik; Riley Williams; linux-kernel@vger.kernel.org
 > Subject: Re: [patch] input: Fix CLOCK_TICK_RATE usage ... [8/13]
 >
 >>>>  Sounds much better to me.  Wouldn't something along the lines of
 >>>> this make the most sense:
 >>
 >>>> #ifdef __ARCH_PIT_FREQ # define PIT_FREQ __ARCH_PIT_FREQ #else #
 >>>> define PIT_FREQ 1193182 #endif
 >>
 >>>> After all, it seems like the vast majority of legacy-compatible
 >>>> hardware _do_ use the standard frequency.
 >>
 >>> Now, if this was in some nice include file, along with the
 >>> definition of the i8253 PIT spinlock, that'd be
 >>> great. Because not just the beeper driver uses the PIT,
 >>> also some joystick code uses it if it is available.
 >>
 >> ftape, too.  The LATCH() macro should also be moved to such a header
 >> file, I think.  How about just creating asm/pit.h?  Only platforms
 >> that need to (potentially) support legacy hardware would need to
 >> define it.  E.g., on ia64, we could do:
 >>
 >>	#ifndef _ASM_IA64_PIT_H
 >>	#define _ASM_IA64_PIT_H
 >>
 >>	#include <linux/config.h>
 >>
 >>	#ifdef CONFIG_LEGACY_HW
 >>	# define PIT_FREQ	1193182
 >>	# define LATCH		((CLOCK_TICK_RATE + HZ/2) / HZ)
 >>	#endif
 >>
 >>	#endif /* _ASM_IA64_PIT_H */
 >>
 >> This way, machines that support legacy hardware can define
 >> CONFIG_LEGACY_HW and on others, the macro can be left undefined, so
 >> that any attempt to compile drivers requiring legacy hw would fail to
 >> compile upfront (much better than accessing random ports!).
 >
 > Yes, that looks very good indeed. Riley?

---
Outgoing mail is certified Virus Free.
Checked by AVG anti-virus system (http://www.grisoft.com).
Version: 6.0.491 / Virus Database: 290 - Release Date: 18-Jun-2003

RE: [patch] input: Fix CLOCK_TICK_RATE usage ... [8/13]

[David Mosberger]

>>>>> On Wed, 25 Jun 2003 09:03:34 +0100, "Riley Williams" <Riley@Williams.Name> said:

  Riley> Hi all.
  Riley> I have no objection to anything along these lines. The basic scenario
  Riley> is simply this:

  Riley> 1. On ALL architectures except for IA64 and ARM there is a SINGLE
  Riley> value for CLOCK_TICK_RATE that is used by several GENERIC drivers.
  Riley> Currently, that value is used as a MAGIC NUMBER that corresponds
  Riley> to the value in the Ix86 case, which is clearly wrong.

What do you mean be "generic"?  AFAIK, the drivers you're talking
about all depend on there being an 8259-style PIT.  As such, they
depend on the 8259 and are not generic.  This dependency should be
made explicit.

BTW: I didn't think Alpha derived its clock-tick from the PIT either,
but I could be misremembering.  Could someone more familiar with Alpha
confirm or deny?

  Riley> 2. According to the IA64 people, those GENERIC drivers are apparently
  Riley> irrelevant for that architecture, so making the CORRECT change of
  Riley> replacing those magic numbers in the GENERIC drivers with the
  Riley> CLOCK_TICK_RATE value will make no difference to IA64.

That's not precise: _some_ ia64 machies do have legacy hardware and
those should be able to use 8259-dependent drivers if they choose to
do so.

Moreover, the current drivers would compile just fine on ia64, even
though they could not possibly work correctly with the current use of
CLOCK_TICK_RATE.  With a separate header file (and a config option),
these dependencies would be made explicit and that would improve
overall cleanliness.

In other words, I still think the right way to go about this is to
have <asm/pit.h>.  On x86, this could be:

--
#include <asm/timex.h>

#define PIT_FREQ	CLOCK_TICK_RATE
#define PIT_LATCH	((PIT_FREQ + HZ/2) / HZ)
--

If you insist, you could even put this in asm-generic, though
personally I don't think that's terribly elegant.

On ia64, <asm/pit.h> could be:

#ifdef CONFIG_PIT
# define PIT_FREQ	1193182
# define PIT_LATCH	((PIT_FREQ + HZ/2) / HZ)
#endif

	--david

RE: [patch] input: Fix CLOCK_TICK_RATE usage ... [8/13]

[Riley Williams]

Hi David.

 >> I have no objection to anything along these lines. The 
 >> basic scenario is simply this:
 >>
 >> 1. On ALL architectures except for IA64 and ARM there is
 >>    a SINGLE value for CLOCK_TICK_RATE that is used by
 >>    several GENERIC drivers. Currently, that value is used
 >>    as a MAGIC NUMBER that corresponds to the value in the
 >>    Ix86 case, which is clearly wrong.

 > What do you mean be "generic"?

Not specific to any particular architecture as far as the
location of the file containing the driver code is concerned.
More simply, not located under linux/arch in the kernel tree.

 > AFAIK, the drivers you're talking about all depend on there
 > being an 8259-style PIT. As such, they depend on the 8259
 > and are not generic. This dependency should be made explicit.

In that case, ALL of the drivers in question need to be moved
under the linux/arch tree. Very few are currently there.

 > BTW: I didn't think Alpha derived its clock-tick from the
 > PIT either, but I could be misremembering. Could someone
 > more familiar with Alpha confirm or deny?

I know ZILCH about ALPHA as I've never had or seen one.

 >> 2. According to the IA64 people, those GENERIC drivers
 >>    are apparently irrelevant for that architecture, so
 >>    making the CORRECT change of replacing those magic
 >>    numbers in the GENERIC drivers with the CLOCK_TICK_RATE
 >>    value will make no difference to IA64.

 > That's not precise: _some_ ia64 machines do have legacy hardware
 > and those should be able to use 8259-dependent drivers if they
 > choose to do so.

My comment as quoted above is a summary of the comments made by
the IA64 developers in this thread. I know ZILCH about the IA64
architecture because, as with the ALPHA architecture, I've never
even seen one. I thus have to assume that comments made by the
IA64 maintainers are accurate.

 > Moreover, the current drivers would compile just fine on ia64,
 > even though they could not possibly work correctly with the
 > current use of CLOCK_TICK_RATE. With a separate header file
 > (and a config option), these dependencies would be made
 > explicit and that would improve overall cleanliness.

I agree that the dependencies need to be made explicit. However,
I'm not convinced that a separate header file is justified, much
less needed.

 > In other words, I still think the right way to go about this
 > is to have <asm/pit.h>. On x86, this could be:
 >
 > --
 > #include <asm/timex.h>
 >
 > #define PIT_FREQ	CLOCK_TICK_RATE
 > #define PIT_LATCH	((PIT_FREQ + HZ/2) / HZ)
 > --
 >
 > If you insist, you could even put this in asm-generic, though
 > personally I don't think that's terribly elegant.

The important details are...

 1. The relevant values are in a known header file.

 2. That header file is referenced and the values therein
    are used rather than just using magic numbers.

Personally, I have no problem with which header files are used.
What matters is that inclusion of a specified header file always
defines the relevant values.

 > On ia64, <asm/pit.h> could be:
 > 
 > #ifdef CONFIG_PIT
 > # define PIT_FREQ	1193182
 > # define PIT_LATCH	((PIT_FREQ + HZ/2) / HZ)
 > #endif

You would then need to ensure that if CONFIG_PIT was not defined,
no reference was ever made to either PIT_FREQ or PIT_LATCH which
can easily become ugly code that the maintainers won't touch.

Best wishes from Riley.
---
 * Nothing as pretty as a smile, nothing as ugly as a frown.

---
Outgoing mail is certified Virus Free.
Checked by AVG anti-virus system (http://www.grisoft.com).
Version: 6.0.491 / Virus Database: 290 - Release Date: 18-Jun-2003

RE: [patch] input: Fix CLOCK_TICK_RATE usage ... [8/13]

[David Mosberger]

>>>>> On Wed, 25 Jun 2003 18:56:43 +0100, "Riley Williams" <Riley@Williams.Name> said:

  >> AFAIK, the drivers you're talking about all depend on there
  >> being an 8259-style PIT. As such, they depend on the 8259
  >> and are not generic. This dependency should be made explicit.

  Riley> In that case, ALL of the drivers in question need to be moved
  Riley> under the linux/arch tree. Very few are currently there.

Not at all.  It's completely standard to have drivers in
linux/drivers/ which may never be enabled for certain platforms.  Or
what's the last time an x86 PC used an Sun SBUS driver?

  >>> 2. According to the IA64 people, those GENERIC drivers
  >>> are apparently irrelevant for that architecture, so
  >>> making the CORRECT change of replacing those magic
  >>> numbers in the GENERIC drivers with the CLOCK_TICK_RATE
  >>> value will make no difference to IA64.

  >> That's not precise: _some_ ia64 machines do have legacy hardware
  >> and those should be able to use 8259-dependent drivers if they
  >> choose to do so.

  Riley> My comment as quoted above is a summary of the comments made by
  Riley> the IA64 developers in this thread. I know ZILCH about the IA64
  Riley> architecture because, as with the ALPHA architecture, I've never
  Riley> even seen one. I thus have to assume that comments made by the
  Riley> IA64 maintainers are accurate.

You seem to fail to realize that I _am_ the ia64 linux tree maintainer.
What does it take for you to believe me?

  >> Moreover, the current drivers would compile just fine on ia64,
  >> even though they could not possibly work correctly with the
  >> current use of CLOCK_TICK_RATE. With a separate header file
  >> (and a config option), these dependencies would be made
  >> explicit and that would improve overall cleanliness.

  Riley> I agree that the dependencies need to be made explicit.

Good.

  Riley> However, I'm not convinced that a separate header file is
  Riley> justified, much less needed.

timex.h definitely is the wrong place.  If you know of a better place
(other than <asm/pit.h>), I'm all ears.

  >> In other words, I still think the right way to go about this
  >> is to have <asm/pit.h>. On x86, this could be:
  >>
  >> --
  >> #include <asm/timex.h>
  >>
  >> #define PIT_FREQ	CLOCK_TICK_RATE
  >> #define PIT_LATCH	((PIT_FREQ + HZ/2) / HZ)
  >> --
  >>
  >> If you insist, you could even put this in asm-generic, though
  >> personally I don't think that's terribly elegant.

  Riley> The important details are...

  Riley> 1. The relevant values are in a known header file.

  Riley> 2. That header file is referenced and the values therein
  Riley> are used rather than just using magic numbers.

I have no problem with that.

  Riley> Personally, I have no problem with which header files are used.
  Riley> What matters is that inclusion of a specified header file always
  Riley> defines the relevant values.

Can we then agree to just create <asm/pit.h> and be done with it?

  >> On ia64, <asm/pit.h> could be:

  >> #ifdef CONFIG_PIT
  >> # define PIT_FREQ	1193182
  >> # define PIT_LATCH	((PIT_FREQ + HZ/2) / HZ)
  >> #endif

  Riley> You would then need to ensure that if CONFIG_PIT was not defined,
  Riley> no reference was ever made to either PIT_FREQ or PIT_LATCH which
  Riley> can easily become ugly code that the maintainers won't touch.

Thanks to the new Kbuild, it's very easy: just add a "depends on PIT"
for drivers that depend on the PIT (I think that's primarily ftape and
the drivers/input/misc/{pc,98}spkr.c.

	--david

Re: [patch] input: Fix CLOCK_TICK_RATE usage ... [8/13]

[Vojtech Pavlik]

On Wed, Jun 25, 2003 at 10:20:59AM -0700, David Mosberger wrote:

> Moreover, the current drivers would compile just fine on ia64, even
> though they could not possibly work correctly with the current use of
> CLOCK_TICK_RATE.  With a separate header file (and a config option),
> these dependencies would be made explicit and that would improve
> overall cleanliness.
> 
> In other words, I still think the right way to go about this is to
> have <asm/pit.h>.  On x86, this could be:
> 
> --
> #include <asm/timex.h>
> 
> #define PIT_FREQ	CLOCK_TICK_RATE
> #define PIT_LATCH	((PIT_FREQ + HZ/2) / HZ)
> --

Actually, I think it should be the other way around:

asm-i386/pit.h:

#define PIT_FREQ	1193182
#define PIT_LATCH	((PIT_FREQ + HZ/2) / HZ)

asm-i386/timex.h:

#include <asm/pit.h>
#define CLOCK_TICK_RATE	PIT_FREQ

> If you insist, you could even put this in asm-generic, though
> personally I don't think that's terribly elegant.
> 
> On ia64, <asm/pit.h> could be:
> 
> #ifdef CONFIG_PIT
> # define PIT_FREQ	1193182
> # define PIT_LATCH	((PIT_FREQ + HZ/2) / HZ)
> #endif
> 
> 	--david

-- 
Vojtech Pavlik
SuSE Labs, SuSE CR

Re: [patch] input: Fix CLOCK_TICK_RATE usage ... [8/13]

[David Mosberger]

>>>>> On Wed, 25 Jun 2003 21:58:47 +0200, Vojtech Pavlik <vojtech@suse.cz> said:

  Vojtech> Actually, I think it should be the other way around:

Ah, yes, that looks better for x86.

	--david