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

[Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>]

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.