Skip to contentSkip to conversation

duplicate clock calculation code in 3 IDE drivers

PATCH In review Andrey Panin 3 participants 5 messages May 22, 2002

lore
Share & tools
lore

Andrey Panin

author
#0 lore raw

PATCH 1/1 duplicate clock calculation code in 3 IDE drivers

Hi,

now it is more interesting patch. AMD, PIIX and VIA IDE drivers contain some duplicated code for (amd|piix|via)_clock calculation. Attached patch moves this code into one function in ata-timing.c file.

Please take a look at it.

Best regards.

Signature · 2 lines
Andrey Panin            | Embedded systems software engineer
[email protected]        | PGP key: wwwkeys.eu.pgp.net
drivers/ide/ata-timing.c +19 -0
diff -urN -X /usr/share/dontdiff linux.vanilla/drivers/ide/ata-timing.c linux/drivers/ide/ata-timing.c
@@ -256,3 +256,22 @@
 
 	return 0;
 }
+
+unsigned int ata_system_bus_clock(void)
+{
+	unsigned int clock = system_bus_speed * 1000;
+
+	switch (clock) {
+		case 33000: clock = 33333; break;
+		case 37000: clock = 37500; break;
+		case 41000: clock = 41666; break;
+	}
+
+	if (clock < 20000 || clock > 50000) {
+		printk(KERN_WARNING "ATA: User given PCI clock speed impossible (%d), using 33 MHz instead.\n", clock);
+		printk(KERN_WARNING "ATA: Use ide0=ata66 if you want to assume 80-wire cable\n");
+		clock = 33333;
+	}
+
+	return clock;
+}
drivers/ide/ata-timing.h +2 -0
diff -urN -X /usr/share/dontdiff linux.vanilla/drivers/ide/ata-timing.h linux/drivers/ide/ata-timing.h
@@ -82,4 +82,6 @@
 extern struct ata_timing* ata_timing_data(short speed);
 extern int ata_timing_compute(struct ata_device *drive,
 		short speed, struct ata_timing *t, int T, int UT);
+
+extern unsigned int ata_system_bus_clock(void);
 #endif
drivers/ide/amd74xx.c +1 -14
diff -urN -X /usr/share/dontdiff linux.vanilla/drivers/ide/amd74xx.c linux/drivers/ide/amd74xx.c
@@ -360,20 +360,7 @@
 /*
  * Determine the system bus clock.
  */
-
-	amd_clock = system_bus_speed * 1000;
-
-	switch (amd_clock) {
-		case 33000: amd_clock = 33333; break;
-		case 37000: amd_clock = 37500; break;
-		case 41000: amd_clock = 41666; break;
-	}
-
-	if (amd_clock < 20000 || amd_clock > 50000) {
-		printk(KERN_WARNING "AMD_IDE: User given PCI clock speed impossible (%d), using 33 MHz instead.\n", amd_clock);
-		printk(KERN_WARNING "AMD_IDE: Use ide0=ata66 if you want to assume 80-wire cable\n");
-		amd_clock = 33333;
-	}
+	amd_clock = ata_system_bus_clock();
 
 /*
  * Print the boot message.
drivers/ide/piix.c +1 -13
diff -urN -X /usr/share/dontdiff linux.vanilla/drivers/ide/piix.c linux/drivers/ide/piix.c
@@ -497,19 +497,7 @@
  * Determine the system bus clock.
  */
 
-	piix_clock = system_bus_speed * 1000;
-
-	switch (piix_clock) {
-		case 33000: piix_clock = 33333; break;
-		case 37000: piix_clock = 37500; break;
-		case 41000: piix_clock = 41666; break;
-	}
-
-	if (piix_clock < 20000 || piix_clock > 50000) {
-		printk(KERN_WARNING "PIIX: User given PCI clock speed impossible (%d), using 33 MHz instead.\n", piix_clock);
-		printk(KERN_WARNING "PIIX: Use ide0=ata66 if you want to assume 80-wire cable\n");
-		piix_clock = 33333;
-	}
+	piix_clock = ata_system_bus_clock();
 
 /*
  * Print the boot message.
drivers/ide/via82cxxx.c +1 -13
diff -urN -X /usr/share/dontdiff linux.vanilla/drivers/ide/via82cxxx.c linux/drivers/ide/via82cxxx.c
@@ -474,19 +474,7 @@
  * Determine system bus clock.
  */
 
-	via_clock = system_bus_speed * 1000;
-
-	switch (via_clock) {
-		case 33000: via_clock = 33333; break;
-		case 37000: via_clock = 37500; break;
-		case 41000: via_clock = 41666; break;
-	}
-
-	if (via_clock < 20000 || via_clock > 50000) {
-		printk(KERN_WARNING "VP_IDE: User given PCI clock speed impossible (%d), using 33 MHz instead.\n", via_clock);
-		printk(KERN_WARNING "VP_IDE: Use ide0=ata66 if you want to assume 80-wire cable.\n");
-		via_clock = 33333;
-	}
+	via_clock = ata_system_bus_clock();
 
 /*
  * Print the boot message.
#1 lore raw
Quoted 7 lines · Andrey Panin

Hi,

now it is more interesting patch. AMD, PIIX and VIA IDE drivers contain some duplicated code for (amd|piix|via)_clock calculation. Attached patch moves this code into one function in ata-timing.c file.

Please take a look at it.

Looks quite OK - though it'd be better if "system_bus_speed" already could be giving these reasonable values - this way we could get rid of the (amd|piix|via)_clock variables altogether.

Quoted 115 lines

Best regards.

Signature · 3 lines
Andrey Panin            | Embedded systems software engineer
[email protected]        | PGP key: wwwkeys.eu.pgp.net
diff -urN -X /usr/share/dontdiff linux.vanilla/drivers/ide/ata-timing.c linux/drivers/ide/ata-timing.c
drivers/ide/ata-timing.c +19 -0
@@ -256,3 +256,22 @@
 
 	return 0;
 }
+
+unsigned int ata_system_bus_clock(void)
+{
+	unsigned int clock = system_bus_speed * 1000;
+
+	switch (clock) {
+		case 33000: clock = 33333; break;
+		case 37000: clock = 37500; break;
+		case 41000: clock = 41666; break;
+	}
+
+	if (clock < 20000 || clock > 50000) {
+		printk(KERN_WARNING "ATA: User given PCI clock speed impossible (%d), using 33 MHz instead.\n", clock);
+		printk(KERN_WARNING "ATA: Use ide0=ata66 if you want to assume 80-wire cable\n");
+		clock = 33333;
+	}
+
+	return clock;
+}

diff -urN -X /usr/share/dontdiff linux.vanilla/drivers/ide/ata-timing.h linux/drivers/ide/ata-timing.h

drivers/ide/ata-timing.h +2 -0
@@ -82,4 +82,6 @@
 extern struct ata_timing* ata_timing_data(short speed);
 extern int ata_timing_compute(struct ata_device *drive,
 		short speed, struct ata_timing *t, int T, int UT);
+
+extern unsigned int ata_system_bus_clock(void);
 #endif

diff -urN -X /usr/share/dontdiff linux.vanilla/drivers/ide/amd74xx.c linux/drivers/ide/amd74xx.c

drivers/ide/amd74xx.c +1 -14
@@ -360,20 +360,7 @@
 /*
  * Determine the system bus clock.
  */
-
-	amd_clock = system_bus_speed * 1000;
-
-	switch (amd_clock) {
-		case 33000: amd_clock = 33333; break;
-		case 37000: amd_clock = 37500; break;
-		case 41000: amd_clock = 41666; break;
-	}
-
-	if (amd_clock < 20000 || amd_clock > 50000) {
-		printk(KERN_WARNING "AMD_IDE: User given PCI clock speed impossible (%d), using 33 MHz instead.\n", amd_clock);
-		printk(KERN_WARNING "AMD_IDE: Use ide0=ata66 if you want to assume 80-wire cable\n");
-		amd_clock = 33333;
-	}
+	amd_clock = ata_system_bus_clock();
 
 /*
  * Print the boot message.

diff -urN -X /usr/share/dontdiff linux.vanilla/drivers/ide/piix.c linux/drivers/ide/piix.c

drivers/ide/piix.c +1 -13
@@ -497,19 +497,7 @@
  * Determine the system bus clock.
  */
 
-	piix_clock = system_bus_speed * 1000;
-
-	switch (piix_clock) {
-		case 33000: piix_clock = 33333; break;
-		case 37000: piix_clock = 37500; break;
-		case 41000: piix_clock = 41666; break;
-	}
-
-	if (piix_clock < 20000 || piix_clock > 50000) {
-		printk(KERN_WARNING "PIIX: User given PCI clock speed impossible (%d), using 33 MHz instead.\n", piix_clock);
-		printk(KERN_WARNING "PIIX: Use ide0=ata66 if you want to assume 80-wire cable\n");
-		piix_clock = 33333;
-	}
+	piix_clock = ata_system_bus_clock();
 
 /*
  * Print the boot message.

diff -urN -X /usr/share/dontdiff linux.vanilla/drivers/ide/via82cxxx.c linux/drivers/ide/via82cxxx.c

drivers/ide/via82cxxx.c +1 -13
@@ -474,19 +474,7 @@
  * Determine system bus clock.
  */
 
-	via_clock = system_bus_speed * 1000;
-
-	switch (via_clock) {
-		case 33000: via_clock = 33333; break;
-		case 37000: via_clock = 37500; break;
-		case 41000: via_clock = 41666; break;
-	}
-
-	if (via_clock < 20000 || via_clock > 50000) {
-		printk(KERN_WARNING "VP_IDE: User given PCI clock speed impossible (%d), using 33 MHz instead.\n", via_clock);
-		printk(KERN_WARNING "VP_IDE: Use ide0=ata66 if you want to assume 80-wire cable.\n");
-		via_clock = 33333;
-	}
+	via_clock = ata_system_bus_clock();
 
 /*
  * Print the boot message.
Signature · 2 lines
Vojtech Pavlik
SuSE Labs
#2 lore raw
In reply to Andrey Panin

Uz.ytkownik Vojtech Pavlik napisa?:

Quoted 8 lines · Andrey Panin
Quoted 7 lines

Hi,

now it is more interesting patch. AMD, PIIX and VIA IDE drivers contain some duplicated code for (amd|piix|via)_clock calculation. Attached patch moves this code into one function in ata-timing.c file.

Quoted 6 lines
Quoted 1 line

Please take a look at it.

Looks quite OK - though it'd be better if "system_bus_speed" already could be giving these reasonable values - this way we could get rid of the (amd|piix|via)_clock variables altogether.

Yes indeed this would be the "kings road".

Any one of you who cares enough?

#3 lore raw
In reply to Andrey Panin
Quoted 12 lines · Martin Dalecki

Uz.ytkownik Vojtech Pavlik napisa?:

Quoted 11 lines · Andrey Panin
Quoted 8 lines

Hi,

now it is more interesting patch. AMD, PIIX and VIA IDE drivers contain some duplicated code for (amd|piix|via)_clock calculation. Attached patch moves this code into one function in ata-timing.c file.

Please take a look at it.

Quoted 8 lines
Quoted 3 lines

Looks quite OK - though it'd be better if "system_bus_speed" already could be giving these reasonable values - this way we could get rid of the (amd|piix|via)_clock variables altogether.

Yes indeed this would be the "kings road".

Any one of you who cares enough?

Here you go ...

Signature · 2 lines
Vojtech Pavlik
SuSE Labs

[email protected], 2002-05-22 13:05:17+02:00, [email protected]
In the last cset I removed the *_clock variables, but due to just search-replace the definitions stayed. This removes them.

[email protected], 2002-05-22 13:00:03+02:00, [email protected]
This cset changes system_bus_speed to kHz, because the additional precisioin is needed for UDMA133 timing computations. It can also move the triplicated sanity checking and MHz->kHz converting code from the drivers to the common code as a result.

 aec62xx.c   |    4 ++--
 ali14xx.c   |    4 ++--
 alim15x3.c  |    8 ++++----
 amd74xx.c   |   45 +++++++++++++--------------------------------
 cmd640.c    |    2 +-
 cmd64x.c    |    2 +-
 cy82c693.c  |    2 +-
 ht6560b.c   |    4 ++--
 ide.c       |   56 ++++++++++++++++++++++++++++++++------------------------
 opti621.c   |    2 +-
 piix.c      |   43 ++++++++++++-------------------------------
 qd65xx.c    |   14 +++++++-------
 via82cxxx.c |   53 +++++++++++++++++------------------------------------
 13 files changed, 95 insertions(+), 144 deletions(-)
drivers/ide/aec62xx.c +2 -2
diff -Nru a/drivers/ide/aec62xx.c b/drivers/ide/aec62xx.c
@@ -206,7 +206,7 @@
 {
 	for ( ; chipset_table->xfer_speed ; chipset_table++)
 		if (chipset_table->xfer_speed == speed) {
-			return ((byte) ((system_bus_speed <= 33) ? chipset_table->chipset_settings_33 : chipset_table->chipset_settings_34));
+			return ((byte) ((system_bus_speed <= 33333) ? chipset_table->chipset_settings_33 : chipset_table->chipset_settings_34));
 		}
 	return 0x00;
 }

@@ -215,7 +215,7 @@
 {
 	for ( ; chipset_table->xfer_speed ; chipset_table++)
 		if (chipset_table->xfer_speed == speed) {
-			return ((byte) ((system_bus_speed <= 33) ? chipset_table->ultra_settings_33 : chipset_table->ultra_settings_34));
+			return ((byte) ((system_bus_speed <= 33333) ? chipset_table->ultra_settings_33 : chipset_table->ultra_settings_34));
 		}
 	return 0x00;
 }
drivers/ide/ali14xx.c +2 -2
diff -Nru a/drivers/ide/ali14xx.c b/drivers/ide/ali14xx.c
@@ -124,8 +124,8 @@
 	/* calculate timing, according to PIO mode */
 	time1 = t->cycle;
 	time2 = t->active;
-	param3 = param1 = (time2 * system_bus_speed + 999) / 1000;
-	param4 = param2 = (time1 * system_bus_speed + 999) / 1000 - param1;
+	param3 = param1 = (time2 * system_bus_speed + 999999) / 1000000;
+	param4 = param2 = (time1 * system_bus_speed + 999999) / 1000000 - param1;
 	if (pio < XFER_PIO_3) {
 		param3 += 8;
 		param4 += 8;
drivers/ide/alim15x3.c +4 -4
diff -Nru a/drivers/ide/alim15x3.c b/drivers/ide/alim15x3.c
@@ -261,18 +261,18 @@
 
 	s_time = t->setup;
 	a_time = t->active;
-	if ((s_clc = (s_time * system_bus_speed + 999) / 1000) >= 8)
+	if ((s_clc = (s_time * system_bus_speed + 999999) / 1000000) >= 8)
 		s_clc = 0;
-	if ((a_clc = (a_time * system_bus_speed + 999) / 1000) >= 8)
+	if ((a_clc = (a_time * system_bus_speed + 999999) / 1000000) >= 8)
 		a_clc = 0;
 	c_time = t->cycle;
 
 #if 0
-	if ((r_clc = ((c_time - s_time - a_time) * system_bus_speed + 999) / 1000) >= 16)
+	if ((r_clc = ((c_time - s_time - a_time) * system_bus_speed + 999999) / 1000000) >= 16)
 		r_clc = 0;
 #endif
 
-	if (!(r_clc = (c_time * system_bus_speed + 999) / 1000 - a_clc - s_clc)) {
+	if (!(r_clc = (c_time * system_bus_speed + 999999) / 1000000 - a_clc - s_clc)) {
 		r_clc = 1;
 	} else {
 		if (r_clc >= 16)
drivers/ide/amd74xx.c +13 -32

94 lines not on this page · raw message

drivers/ide/cmd640.c +1 -1
diff -Nru a/drivers/ide/cmd640.c b/drivers/ide/cmd640.c
@@ -603,7 +603,7 @@
 	u8 cycle_count;
 
 	recovery_time = cycle_time - (setup_time + active_time);
-	clock_time = 1000 / system_bus_speed;
+	clock_time = 1000000 / system_bus_speed;
 	cycle_count = (cycle_time + clock_time - 1) / clock_time;
 
 	setup_count = (setup_time + clock_time - 1) / clock_time;
drivers/ide/cmd64x.c +1 -1
diff -Nru a/drivers/ide/cmd64x.c b/drivers/ide/cmd64x.c
@@ -305,7 +305,7 @@
 	 */
 
 	recovery_time = t->cycle - (t->setup + t->active);
-	clock_time = 1000 / system_bus_speed;
+	clock_time = 1000000 / system_bus_speed;
 	cycle_count = (t->cycle + clock_time - 1) / clock_time;
 	setup_count = (t->setup + clock_time - 1) / clock_time;
 	active_count = (t->active + clock_time - 1) / clock_time;
drivers/ide/cy82c693.c +1 -1
diff -Nru a/drivers/ide/cy82c693.c b/drivers/ide/cy82c693.c
@@ -121,7 +121,7 @@
 {
 	int clocks;
 
-	clocks = (time*bus_speed+999)/1000 -1;
+	clocks = (time*bus_speed+999999)/1000000 -1;
 
 	if (clocks < 0)
 		clocks = 0;
drivers/ide/ht6560b.c +2 -2
diff -Nru a/drivers/ide/ht6560b.c b/drivers/ide/ht6560b.c
@@ -219,8 +219,8 @@
 		/*
 		 *  Cycle times should be Vesa bus cycles
 		 */
-		active_cycles   = (active_time   * system_bus_speed + 999) / 1000;
-		recovery_cycles = (recovery_time * system_bus_speed + 999) / 1000;
+		active_cycles   = (active_time   * system_bus_speed + 999999) / 1000000;
+		recovery_cycles = (recovery_time * system_bus_speed + 999999) / 1000000;
 		/*
 		 *  Upper and lower limits
 		 */
drivers/ide/ide.c +32 -24

77 lines not on this page · raw message

drivers/ide/opti621.c +1 -1
diff -Nru a/drivers/ide/opti621.c b/drivers/ide/opti621.c
@@ -175,7 +175,7 @@
  * Use idebus=xx to select right frequency.
  */
 {
-	return ((time*bus_speed+999)/1000);
+	return ((time*bus_speed+999999)/1000000);
 }
 
 static void write_reg(byte value, int reg)
drivers/ide/piix.c +12 -31

89 lines not on this page · raw message

drivers/ide/qd65xx.c +7 -7
diff -Nru a/drivers/ide/qd65xx.c b/drivers/ide/qd65xx.c
@@ -133,12 +133,12 @@
 {
 	byte active_cycle,recovery_cycle;
 
-	if (system_bus_speed <= 33) {
-		active_cycle =   9  - IDE_IN(active_time   * system_bus_speed / 1000 + 1, 2, 9);
-		recovery_cycle = 15 - IDE_IN(recovery_time * system_bus_speed / 1000 + 1, 0, 15);
+	if (system_bus_speed <= 33333) {
+		active_cycle =   9  - IDE_IN(active_time   * system_bus_speed / 1000000 + 1, 2, 9);
+		recovery_cycle = 15 - IDE_IN(recovery_time * system_bus_speed / 1000000 + 1, 0, 15);
 	} else {
-		active_cycle =   8  - IDE_IN(active_time   * system_bus_speed / 1000 + 1, 1, 8);
-		recovery_cycle = 18 - IDE_IN(recovery_time * system_bus_speed / 1000 + 1, 3, 18);
+		active_cycle =   8  - IDE_IN(active_time   * system_bus_speed / 1000000 + 1, 1, 8);
+		recovery_cycle = 18 - IDE_IN(recovery_time * system_bus_speed / 1000000 + 1, 3, 18);
 	}
 
 	return((recovery_cycle<<4) | 0x08 | active_cycle);

@@ -152,8 +152,8 @@
 
 static byte qd6580_compute_timing (int active_time, int recovery_time)
 {
-	byte active_cycle   = 17-IDE_IN(active_time   * system_bus_speed / 1000 + 1, 2, 17);
-	byte recovery_cycle = 15-IDE_IN(recovery_time * system_bus_speed / 1000 + 1, 2, 15);
+	byte active_cycle   = 17-IDE_IN(active_time   * system_bus_speed / 1000000 + 1, 2, 17);
+	byte recovery_cycle = 15-IDE_IN(recovery_time * system_bus_speed / 1000000 + 1, 2, 15);
 
 	return((recovery_cycle<<4) | active_cycle);
 }
drivers/ide/via82cxxx.c +17 -36

104 lines not on this page · raw message

#4 lore raw

PATCH 1/1 Remove dead DISK_RECOVERY_TIME code.

In reply to Andrey Panin
Quoted 5 lines · Martin Dalecki
Quoted 3 lines
Quoted 1 line

Any one of you who cares enough?

Here you go ...

Thank's looks fine applied.

And here is one more for you ...

Signature · 2 lines
Vojtech Pavlik
SuSE Labs

[email protected], 2002-05-22 13:34:07+02:00, [email protected]
This removes all stuff related to DISK_RECOVERY_TIME. It seems to have been a hack to make some really broken hardware work - but broken beyond repair as well. It was reading the PIT timer without proper locking (hence interfering with system time), it was reading it regardless of the architecture (will work on PCs and similar only), etc, etc. Goodbye DISK_RECOVERY_TIME.

 drivers/ide/ide.c   |   35 -----------------------------------
 include/linux/ide.h |    7 -------
 2 files changed, 42 deletions(-)
drivers/ide/ide.c +0 -35
diff -Nru a/drivers/ide/ide.c b/drivers/ide/ide.c
@@ -139,34 +139,6 @@
  */
 struct ata_channel ide_hwifs[MAX_HWIFS];	/* master data repository */
 
-#if (DISK_RECOVERY_TIME > 0)
-/*
- * For really screwed hardware (hey, at least it *can* be used with Linux)
- * we can enforce a minimum delay time between successive operations.
- */
-static unsigned long read_timer (void)
-{
-	unsigned long t, flags;
-	int i;
-
-	__save_flags(flags);	/* local CPU only */
-	__cli();		/* local CPU only */
-	t = jiffies * 11932;
-	outb_p(0, 0x43);
-	i = inb_p(0x40);
-	i |= inb(0x40) << 8;
-	__restore_flags(flags);	/* local CPU only */
-	return (t - i);
-}
-#endif
-
-static inline void set_recovery_timer(struct ata_channel *channel)
-{
-#if (DISK_RECOVERY_TIME > 0)
-	channel->last_time = read_timer();
-#endif
-}
-
 static void init_hwif_data(struct ata_channel *ch, unsigned int index)
 {
 	static const byte ide_major[] = {

@@ -986,11 +958,6 @@
 	 */
 	if (block == 0 && drive->remap_0_to_1 == 1)
 		block = 1;  /* redirect MBR access to EZ-Drive partn table */
-
-#if (DISK_RECOVERY_TIME > 0)
-	while ((read_timer() - ch->last_time) < DISK_RECOVERY_TIME);
-#endif
-
 	{
 		ide_startstop_t res;
 

@@ -1462,7 +1429,6 @@
 				} else
 					startstop = ide_error(drive, drive->rq, "irq timeout", GET_STAT());
 			}
-			set_recovery_timer(ch);
 			enable_irq(ch->irq);
 
 			spin_lock_irq(ch->lock);

@@ -1612,7 +1578,6 @@
 	 * same irq as is currently being serviced here, and Linux
 	 * won't allow another of the same (on any CPU) until we return.
 	 */
-	set_recovery_timer(drive->channel);
 	if (startstop == ide_stopped) {
 		if (!ch->handler) {	/* paranoia */
 			clear_bit(IDE_BUSY, &ch->active);
include/linux/ide.h +0 -7
diff -Nru a/include/linux/ide.h b/include/linux/ide.h
@@ -40,9 +40,6 @@
 
 /* Right now this is only needed by a promise controlled.
  */
-#ifndef DISK_RECOVERY_TIME		/* off=0; on=access_delay_time */
-# define DISK_RECOVERY_TIME	0	/*  for hardware that needs it */
-#endif
 #ifndef OK_TO_RESET_CONTROLLER		/* 1 needed for good error recovery */
 # define OK_TO_RESET_CONTROLLER	0	/* 0 for use with AH2372A/B interface */
 #endif

@@ -547,10 +544,6 @@
 	unsigned slow		: 1;	/* flag: slow data port */
 	unsigned io_32bit	: 1;	/* 0=16-bit, 1=32-bit */
 	unsigned char bus_state;	/* power state of the IDE bus */
-
-#if (DISK_RECOVERY_TIME > 0)
-	unsigned long last_time;	/* time when previous rq was done */
-#endif
 };
 
 /*