[U-Boot] [PATCH] IDE: Improving speed on reading data Part 1/1

[U-Boot] [PATCH] IDE: Improving speed on reading data Part 1/1

[Stefan Althoefer]

[PATCH] IDE: Improving speed on reading data

This patch improves the speed when reading blocks from
IDE devices by reading more than one block at a time. Up to
128 blocks are requested in one read command.

On my testplatform (Janz emPC-A400 with CompactFLASH card)
this nearly doubled speed.

Also the ide_wait() code was rewritten to have lower latency
by polling more frequently for status.


The patch is against "latest" u-boot git-repository

Please (still) be patient if style of submission or patches are
offending.

Signed-off-by: Stefan Althoefer <stefan.althoefer@web.de>
----

diff -uprN u-boot-orig//common/cmd_ide.c u-boot/common/cmd_ide.c
--- u-boot-orig//common/cmd_ide.c	2008-12-02 17:25:31.000000000 +0100
+++ u-boot/common/cmd_ide.c	2008-12-03 09:22:29.000000000 +0100
@@ -1302,6 +1361,7 @@ ulong ide_read (int device, lbaint_t blk
 	ulong n = 0;
 	unsigned char c;
 	unsigned char pwrsave=0; /* power save */
+	ulong scnt;
 #ifdef CONFIG_LBA48
 	unsigned char lba48 = 0;
 
@@ -1346,7 +1406,7 @@ ulong ide_read (int device, lbaint_t blk
 	}
 
 
-	while (blkcnt-- > 0) {
+	while (blkcnt > 0) {
 
 		c = ide_wait (device, IDE_TIME_OUT);
 
@@ -1368,7 +1428,8 @@ ulong ide_read (int device, lbaint_t blk
 #endif
 		}
 #endif
-		ide_outb (device, ATA_SECT_CNT, 1);
+		scnt = (blkcnt > 128) ? 128 : blkcnt;
+		ide_outb (device, ATA_SECT_CNT, scnt);
 		ide_outb (device, ATA_LBA_LOW,  (blknr >>  0) & 0xFF);
 		ide_outb (device, ATA_LBA_MID,  (blknr >>  8) & 0xFF);
 		ide_outb (device, ATA_LBA_HIGH, (blknr >> 16) & 0xFF);
@@ -1387,32 +1448,36 @@ ulong ide_read (int device, lbaint_t blk
 			ide_outb (device, ATA_COMMAND,  ATA_CMD_READ);
 		}
 
-		udelay (50);
+		while (scnt > 0) {
+			udelay (50);
 
-		if(pwrsave) {
-			c = ide_wait (device, IDE_SPIN_UP_TIME_OUT);	/* may take up to 4 sec */
-			pwrsave=0;
-		} else {
-			c = ide_wait (device, IDE_TIME_OUT);	/* can't take over 500 ms */
-		}
+			if(pwrsave) {
+				c = ide_wait (device, IDE_SPIN_UP_TIME_OUT);	/* may take up to 4 sec */
+				pwrsave=0;
+			} else {
+				c = ide_wait (device, IDE_TIME_OUT);	/* can't take over 500 ms */
+			}
 
-		if ((c&(ATA_STAT_DRQ|ATA_STAT_BUSY|ATA_STAT_ERR)) != ATA_STAT_DRQ) {
+			if ((c&(ATA_STAT_DRQ|ATA_STAT_BUSY|ATA_STAT_ERR)) != ATA_STAT_DRQ) {
 #if defined(CONFIG_SYS_64BIT_LBA) && defined(CONFIG_SYS_64BIT_VSPRINTF)
-			printf ("Error (no IRQ) dev %d blk %qd: status 0x%02x\n",
-				device, blknr, c);
+				printf ("Error (no IRQ) dev %d blk %qd: status 0x%02x\n",
+					device, blknr, c);
 #else
-			printf ("Error (no IRQ) dev %d blk %ld: status 0x%02x\n",
-				device, (ulong)blknr, c);
+				printf ("Error (no IRQ) dev %d blk %ld: status 0x%02x\n",
+					device, (ulong)blknr, c);
 #endif
-			break;
-		}
+				break;
+			}
 
-		input_data (device, buffer, ATA_SECTORWORDS);
-		(void) ide_inb (device, ATA_STATUS);	/* clear IRQ */
-
-		++n;
-		++blknr;
-		buffer += ATA_BLOCKSIZE;
+			input_data (device, buffer, ATA_SECTORWORDS);
+			(void) ide_inb (device, ATA_STATUS);	/* clear IRQ */
+
+			++n;
+			++blknr;
+			--blkcnt;
+			--scnt;
+			buffer += ATA_BLOCKSIZE;
+		}
 	}
 IDE_READ_E:
 	ide_led (DEVICE_LED(device), 0);	/* LED off	*/
@@ -1548,11 +1613,11 @@ OUT:
  */
 static uchar ide_wait (int dev, ulong t)
 {
-	ulong delay = 10 * t;		/* poll every 100 us */
+	ulong delay = (1000/5) * t;		/* poll every 5 us */
 	uchar c;
 
 	while ((c = ide_inb(dev, ATA_STATUS)) & ATA_STAT_BUSY) {
-		udelay (100);
+		udelay (5);
 		if (delay-- == 0) {
 			break;
 		}

[U-Boot] [PATCH] IDE: Improving speed on reading data Part 1/1

[Wolfgang Denk]

Dear Stefan Althoefer,

In message <49384704.sPrSOzo/CYqyo4zk%stefan.althoefer@web.de> you wrote:
> [PATCH] IDE: Improving speed on reading data
> 
> This patch improves the speed when reading blocks from
> IDE devices by reading more than one block at a time. Up to
> 128 blocks are requested in one read command.
> 
> On my testplatform (Janz emPC-A400 with CompactFLASH card)
> this nearly doubled speed.
> 
> Also the ide_wait() code was rewritten to have lower latency
> by polling more frequently for status.

Can you please use git-format-patch to format the patch? I don't know
how you created the diff, but it looks strange to me (and harldy
readable).

> The patch is against "latest" u-boot git-repository
> 
> Please (still) be patient if style of submission or patches are
> offending.

Such comments must go below the "---" line

> Signed-off-by: Stefan Althoefer <stefan.althoefer@web.de>
> ----

Thsi is the place to add comments that are not supposed to become part
of the commit message.

> -		ide_outb (device, ATA_SECT_CNT, 1);
> +		scnt = (blkcnt > 128) ? 128 : blkcnt;
> +		ide_outb (device, ATA_SECT_CNT, scnt);

What happens if you try to read at or beyond the end of the device?

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Brain off-line, please wait.

[U-Boot] [PATCH] IDE: Improving speed on reading data Part 1/1

[Stefan Althoefer]

IDE: Improving speed on reading data

This patch improves the speed when reading blocks from
IDE devices by reading more than one block at a time. Up to
128 blocks are requested in one read command.

The ide_wait() code was rewritten to have lower latency
by polling more frequently for status.

On my testplatform (Janz emPC-A400 with CompactFLASH card)
this nearly doubled speed.

Also, error handling has been improved, so that ide_read does not
attempt to read beyond the last sector of the device.

Signed-off-by: Stefan Althoefer <stefan.althoefer@web.de>
---
> Can you please use git-format-patch to format the patch? I don't know
> how you created the diff, but it looks strange to me (and harldy
> readable).

Sorry I wasn't fully aware of the wonderful world of git-* when
I prepared the first patches ... ok I'm still not (fully).

>> -		ide_outb (device, ATA_SECT_CNT, 1);
>> +		scnt = (blkcnt > 128) ? 128 : blkcnt;
>> +		ide_outb (device, ATA_SECT_CNT, scnt);
>
> What happens if you try to read at or beyond the end of the device?

Good point. The old code wasn't very smart with this either (it did not
check for bounds but let the device decide). My code added a deadlock
to this behaviour (break did not work in a inner loop).

I added some checks against block_dev_desc_t->lba to fix this.

 common/cmd_ide.c |   63 ++++++++++++++++++++++++++++++++++-------------------
 1 files changed, 40 insertions(+), 23 deletions(-)

diff --git a/common/cmd_ide.c b/common/cmd_ide.c
index dd62c87..ec64767 100644
--- a/common/cmd_ide.c
+++ b/common/cmd_ide.c
@@ -1361,6 +1361,8 @@ ulong ide_read (int device, lbaint_t blknr, ulong blkcnt, void *buffer)
 	ulong n = 0;
 	unsigned char c;
 	unsigned char pwrsave=0; /* power save */
+	ulong scnt;
+	lbaint_t blkrem;
 #ifdef CONFIG_LBA48
 	unsigned char lba48 = 0;

@@ -1372,6 +1374,12 @@ ulong ide_read (int device, lbaint_t blknr, ulong blkcnt, void *buffer)
 	debug ("ide_read dev %d start %qX, blocks %lX buffer at %lX\n",
 		device, blknr, blkcnt, (ulong)buffer);

+	if( blknr >= ide_get_dev(device)->lba ){
+		printf ("IDE read: device %d read beyond last block\n", device);
+		goto IDE_READ_E;
+	}
+	blkrem = ide_get_dev(device)->lba - blknr;
+
 	ide_led (DEVICE_LED(device), 1);	/* LED on	*/

 	/* Select device
@@ -1405,7 +1413,7 @@ ulong ide_read (int device, lbaint_t blknr, ulong blkcnt, void *buffer)
 	}


-	while (blkcnt-- > 0) {
+	while (blkcnt > 0) {

 		c = ide_wait (device, IDE_TIME_OUT);

@@ -1427,7 +1435,9 @@ ulong ide_read (int device, lbaint_t blknr, ulong blkcnt, void *buffer)
 #endif
 		}
 #endif
-		ide_outb (device, ATA_SECT_CNT, 1);
+		scnt = (blkcnt > 128) ? 128 : blkcnt;
+		scnt = (scnt > blkrem) ? blkrem : scnt;
+		ide_outb (device, ATA_SECT_CNT, scnt);
 		ide_outb (device, ATA_LBA_LOW,  (blknr >>  0) & 0xFF);
 		ide_outb (device, ATA_LBA_MID,  (blknr >>  8) & 0xFF);
 		ide_outb (device, ATA_LBA_HIGH, (blknr >> 16) & 0xFF);
@@ -1446,32 +1456,39 @@ ulong ide_read (int device, lbaint_t blknr, ulong blkcnt, void *buffer)
 			ide_outb (device, ATA_COMMAND,  ATA_CMD_READ);
 		}

-		udelay (50);
+		while (scnt > 0) {
+			udelay (50);

-		if(pwrsave) {
-			c = ide_wait (device, IDE_SPIN_UP_TIME_OUT);	/* may take up to 4 sec */
-			pwrsave=0;
-		} else {
-			c = ide_wait (device, IDE_TIME_OUT);	/* can't take over 500 ms */
-		}
+			if(pwrsave) {
+				c = ide_wait (device, IDE_SPIN_UP_TIME_OUT);	/* may take up to 4 sec */
+				pwrsave=0;
+			} else {
+				c = ide_wait (device, IDE_TIME_OUT);	/* can't take over 500 ms */
+			}

-		if ((c&(ATA_STAT_DRQ|ATA_STAT_BUSY|ATA_STAT_ERR)) != ATA_STAT_DRQ) {
+			if ((c&(ATA_STAT_DRQ|ATA_STAT_BUSY|ATA_STAT_ERR)) != ATA_STAT_DRQ) {
 #if defined(CONFIG_SYS_64BIT_LBA) && defined(CONFIG_SYS_64BIT_VSPRINTF)
-			printf ("Error (no IRQ) dev %d blk %qd: status 0x%02x\n",
-				device, blknr, c);
+				printf ("Error (no IRQ) dev %d blk %qd: status 0x%02x\n",
+					device, blknr, c);
 #else
-			printf ("Error (no IRQ) dev %d blk %ld: status 0x%02x\n",
-				device, (ulong)blknr, c);
+				printf ("Error (no IRQ) dev %d blk %ld: status 0x%02x\n",
+					device, (ulong)blknr, c);
 #endif
-			break;
-		}
+				goto IDE_READ_E;
+			}

-		input_data (device, buffer, ATA_SECTORWORDS);
-		(void) ide_inb (device, ATA_STATUS);	/* clear IRQ */
+			input_data (device, buffer, ATA_SECTORWORDS);
+			(void) ide_inb (device, ATA_STATUS);	/* clear IRQ */

-		++n;
-		++blknr;
-		buffer += ATA_BLOCKSIZE;
+			++n;
+			++blknr;
+			--blkcnt;
+			--blkrem;
+			--scnt;
+			buffer += ATA_BLOCKSIZE;
+		}
+		if (blkrem == 0)
+			break;
 	}
 IDE_READ_E:
 	ide_led (DEVICE_LED(device), 0);	/* LED off	*/
@@ -1607,11 +1624,11 @@ OUT:
  */
 static uchar ide_wait (int dev, ulong t)
 {
-	ulong delay = 10 * t;		/* poll every 100 us */
+	ulong delay = (1000/5) * t;		/* poll every 5 us */
 	uchar c;

 	while ((c = ide_inb(dev, ATA_STATUS)) & ATA_STAT_BUSY) {
-		udelay (100);
+		udelay (5);
 		if (delay-- == 0) {
 			break;
 		}
-- 
1.5.6

[U-Boot] [PATCH] IDE: Improving speed on reading data Part 1/1

[Jean-Christophe PLAGNIOL-VILLARD]

On 22:22 Tue 16 Dec     , Stefan Althoefer wrote:
> IDE: Improving speed on reading data
> 
> This patch improves the speed when reading blocks from
> IDE devices by reading more than one block at a time. Up to
> 128 blocks are requested in one read command.
> 
> The ide_wait() code was rewritten to have lower latency
> by polling more frequently for status.
> 
> On my testplatform (Janz emPC-A400 with CompactFLASH card)
> this nearly doubled speed.
> 
> Also, error handling has been improved, so that ide_read does not
> attempt to read beyond the last sector of the device.
> 
> Signed-off-by: Stefan Althoefer <stefan.althoefer@web.de>
> ---
> > Can you please use git-format-patch to format the patch? I don't know
> > how you created the diff, but it looks strange to me (and harldy
> > readable).
> 
> Sorry I wasn't fully aware of the wonderful world of git-* when
> I prepared the first patches ... ok I'm still not (fully).
> 
> >> -		ide_outb (device, ATA_SECT_CNT, 1);
> >> +		scnt = (blkcnt > 128) ? 128 : blkcnt;
> >> +		ide_outb (device, ATA_SECT_CNT, scnt);
> >
> > What happens if you try to read at or beyond the end of the device?
> 
> Good point. The old code wasn't very smart with this either (it did not
> check for bounds but let the device decide). My code added a deadlock
> to this behaviour (break did not work in a inner loop).
> 
> I added some checks against block_dev_desc_t->lba to fix this.
please send patch as a separate e-mail or put the comment which will not
appear in the commit message after the ---
> 
>  common/cmd_ide.c |   63 ++++++++++++++++++++++++++++++++++-------------------
>  1 files changed, 40 insertions(+), 23 deletions(-)
> 
> diff --git a/common/cmd_ide.c b/common/cmd_ide.c
> index dd62c87..ec64767 100644
> --- a/common/cmd_ide.c
> +++ b/common/cmd_ide.c
> @@ -1361,6 +1361,8 @@ ulong ide_read (int device, lbaint_t blknr, ulong blkcnt, void *buffer)
>  	ulong n = 0;
>  	unsigned char c;
>  	unsigned char pwrsave=0; /* power save */
> +	ulong scnt;
> +	lbaint_t blkrem;
>  #ifdef CONFIG_LBA48
>  	unsigned char lba48 = 0;
> 
> @@ -1372,6 +1374,12 @@ ulong ide_read (int device, lbaint_t blknr, ulong blkcnt, void *buffer)
>  	debug ("ide_read dev %d start %qX, blocks %lX buffer at %lX\n",
>  		device, blknr, blkcnt, (ulong)buffer);
> 
> +	if( blknr >= ide_get_dev(device)->lba ){
please replace with
	if (blknr >= ide_get_dev(device)->lba) {

> +		printf ("IDE read: device %d read beyond last block\n", device);
> +		goto IDE_READ_E;
> +	}
> +	blkrem = ide_get_dev(device)->lba - blknr;
> +
>  	ide_led (DEVICE_LED(device), 1);	/* LED on	*/
> 
>  	/* Select device
> @@ -1405,7 +1413,7 @@ ulong ide_read (int device, lbaint_t blknr, ulong blkcnt, void *buffer)
>  	}
> 
> 
> -	while (blkcnt-- > 0) {
> +	while (blkcnt > 0) {
> 
>  		c = ide_wait (device, IDE_TIME_OUT);
> 
> @@ -1427,7 +1435,9 @@ ulong ide_read (int device, lbaint_t blknr, ulong blkcnt, void *buffer)
>  #endif
>  		}
>  #endif
> -		ide_outb (device, ATA_SECT_CNT, 1);
> +		scnt = (blkcnt > 128) ? 128 : blkcnt;
> +		scnt = (scnt > blkrem) ? blkrem : scnt;
> +		ide_outb (device, ATA_SECT_CNT, scnt);
>  		ide_outb (device, ATA_LBA_LOW,  (blknr >>  0) & 0xFF);
>  		ide_outb (device, ATA_LBA_MID,  (blknr >>  8) & 0xFF);
>  		ide_outb (device, ATA_LBA_HIGH, (blknr >> 16) & 0xFF);
> @@ -1446,32 +1456,39 @@ ulong ide_read (int device, lbaint_t blknr, ulong blkcnt, void *buffer)
>  			ide_outb (device, ATA_COMMAND,  ATA_CMD_READ);
>  		}
> 
> -		udelay (50);
> +		while (scnt > 0) {
> +			udelay (50);
> 
> -		if(pwrsave) {
> -			c = ide_wait (device, IDE_SPIN_UP_TIME_OUT);	/* may take up to 4 sec */
> -			pwrsave=0;
> -		} else {
> -			c = ide_wait (device, IDE_TIME_OUT);	/* can't take over 500 ms */
> -		}
> +			if(pwrsave) {
> +				c = ide_wait (device, IDE_SPIN_UP_TIME_OUT);	/* may take up to 4 sec */
> +				pwrsave=0;
please add a space before and after the '='
add be carefull of the 80 chars limit
> +			} else {
> +				c = ide_wait (device, IDE_TIME_OUT);	/* can't take over 500 ms */
> +			}
> 
> -		if ((c&(ATA_STAT_DRQ|ATA_STAT_BUSY|ATA_STAT_ERR)) != ATA_STAT_DRQ) {
> +			if ((c&(ATA_STAT_DRQ|ATA_STAT_BUSY|ATA_STAT_ERR)) != ATA_STAT_DRQ) {
please add a space before and after the '&' and '|'
>  #if defined(CONFIG_SYS_64BIT_LBA) && defined(CONFIG_SYS_64BIT_VSPRINTF)
> -			printf ("Error (no IRQ) dev %d blk %qd: status 0x%02x\n",
> -				device, blknr, c);
> +				printf ("Error (no IRQ) dev %d blk %qd: status 0x%02x\n",
> +					device, blknr, c);
>  #else
> -			printf ("Error (no IRQ) dev %d blk %ld: status 0x%02x\n",
> -				device, (ulong)blknr, c);
> +				printf ("Error (no IRQ) dev %d blk %ld: status 0x%02x\n",
> +					device, (ulong)blknr, c);
>  #endif
> -			break;
> -		}
> +				goto IDE_READ_E;
> +			}
> 
> -		input_data (device, buffer, ATA_SECTORWORDS);
> -		(void) ide_inb (device, ATA_STATUS);	/* clear IRQ */
> +			input_data (device, buffer, ATA_SECTORWORDS);
> +			(void) ide_inb (device, ATA_STATUS);	/* clear IRQ */
> 
> -		++n;
> -		++blknr;
> -		buffer += ATA_BLOCKSIZE;
> +			++n;
> +			++blknr;
> +			--blkcnt;
> +			--blkrem;
> +			--scnt;
> +			buffer += ATA_BLOCKSIZE;
> +		}
> +		if (blkrem == 0)
> +			break;
>  	}
>  IDE_READ_E:
>  	ide_led (DEVICE_LED(device), 0);	/* LED off	*/
> @@ -1607,11 +1624,11 @@ OUT:
>   */
>  static uchar ide_wait (int dev, ulong t)
>  {
> -	ulong delay = 10 * t;		/* poll every 100 us */
> +	ulong delay = (1000/5) * t;		/* poll every 5 us */
why not 200 instead?
>  	uchar c;
> 
>  	while ((c = ide_inb(dev, ATA_STATUS)) & ATA_STAT_BUSY) {
> -		udelay (100);
> +		udelay (5);
>  		if (delay-- == 0) {
>  			break;
>  		}
Best Regards,
J.