[U-Boot] [RFC] setting pio modes for IDE devices

[U-Boot] [RFC] setting pio modes for IDE devices

[Steven A. Falco]

The following patch adds the ability to call-out from the ide_ident
routine to a board-specific routine to set the PIO mode of an attached
device.

This feature is controlled by the preprocessor variable CONFIG_TUNE_CFA.

cmd_ide.c is modified to use the "drive identify information" read from the
device to compute a pio mode in the range 0 to 6.  This is then passed
to a board-specific routine, ide_set_piomode, to set the mode in
hardware.

Two other files are touched: file ata.h, to include word 163, which is the
"advanced CF parameters", and file ide.h, to add a prototype for the
board-specific routine (again controlled by the CONFIG_TUNE_CFA
variable).

Please let me know if this or some variation on the idea would be
acceptible.

	Steve


diff --git a/common/cmd_ide.c b/common/cmd_ide.c
index b4d9719..00b85db 100644
--- a/common/cmd_ide.c
+++ b/common/cmd_ide.c
@@ -1054,6 +1054,10 @@ static void ide_ident (block_dev_desc_t *dev_desc)
 	int do_retry = 0;
 #endif
 
+#ifdef CONFIG_TUNE_CFA
+	int pio_mode;
+#endif
+
 #if 0
 	int mode, cycle_time;
 #endif
@@ -1169,6 +1173,40 @@ static void ide_ident (block_dev_desc_t *dev_desc)
 	else
 		dev_desc->removable = 0;
 
+#ifdef CONFIG_TUNE_CFA
+	/* Mode 0 - 2 only, are directly determined by word 51. */
+	pio_mode = iop->tPIO;
+	if(pio_mode > 2) {
+		printf("WARNING: Invalid PIO (word 51 = %d).\n", pio_mode);
+		pio_mode = 0; /* Force it to dead slow, and hope for the best... */
+	}
+
+	/* Any CompactFlash Storage Card that supports PIO mode 3 or above
+	 * shall set bit 1 of word 53 to one and support the fields contained
+	 * in words 64 through 70.
+	 */
+	if(iop->field_valid & 0x02) {
+		/* Mode 3 and above are possible.  Check in order from slow
+		 * to fast, so we wind up with the highest mode allowed.
+		 */
+		if(iop->eide_pio_modes & 0x01) {
+			pio_mode = 3;
+		}
+		if(iop->eide_pio_modes & 0x02) {
+			pio_mode = 4;
+		}
+		if((iop->cf_advanced_caps & 0x07) == 0x01) {
+			pio_mode = 5;
+		}
+		if((iop->cf_advanced_caps & 0x07) == 0x02) {
+			pio_mode = 6;
+		}
+	}
+
+	/* System-specific, depends on bus speeds, etc. */
+	ide_set_piomode(pio_mode);
+#endif /* CONFIG_TUNE_CFA */
+
 #if 0
 	/*
 	 * Drive PIO mode autoselection
diff --git a/include/ata.h b/include/ata.h
index aa6e90d..3269636 100644
--- a/include/ata.h
+++ b/include/ata.h
@@ -294,7 +294,9 @@ typedef struct hd_driveid {
 	unsigned short	words130_155[26];/* reserved vendor words 130-155 */
 	unsigned short	word156;
 	unsigned short	words157_159[3];/* reserved vendor words 157-159 */
-	unsigned short	words160_255[95];/* reserved words 160-255 */
+	unsigned short	words160_162[3];/* reserved words 160-162 */
+	unsigned short	cf_advanced_caps;
+	unsigned short	words164_255[92];/* reserved words 164-255 */
 } hd_driveid_t;
 
 
diff --git a/include/ide.h b/include/ide.h
index 222f4f8..ae4624b 100644
--- a/include/ide.h
+++ b/include/ide.h
@@ -54,4 +54,9 @@ void ide_init(void);
 ulong ide_read(int device, lbaint_t blknr, ulong blkcnt, void *buffer);
 ulong ide_write(int device, lbaint_t blknr, ulong blkcnt, void *buffer);
 
+#ifdef CONFIG_TUNE_CFA
+/* Returns 0 on success, else returns 1.  pio_mode can be 0 through 6. */
+extern int ide_set_piomode(int pio_mode);
+#endif
+
 #endif /* _IDE_H */

[U-Boot] [RFC] setting pio modes for IDE devices

[Wolfgang Denk]

Dear "Steven A. Falco",

In message <48A35533.1010901@harris.com> you wrote:
> The following patch adds the ability to call-out from the ide_ident
> routine to a board-specific routine to set the PIO mode of an attached
> device.
> 
> This feature is controlled by the preprocessor variable CONFIG_TUNE_CFA.

What does CFA mean?

> cmd_ide.c is modified to use the "drive identify information" read from the
> device to compute a pio mode in the range 0 to 6.  This is then passed
> to a board-specific routine, ide_set_piomode, to set the mode in
> hardware.
> 
> Two other files are touched: file ata.h, to include word 163, which is the
> "advanced CF parameters", and file ide.h, to add a prototype for the
> board-specific routine (again controlled by the CONFIG_TUNE_CFA
> variable).

Is there a specific reason for using #ifdef's instead of a weak
function?

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
Conceptual integrity in turn dictates that the  design  must  proceed
from  one  mind,  or  from  a  very small number of agreeing resonant
minds.               - Frederick Brooks Jr., "The Mythical Man Month"

[U-Boot] [RFC] setting pio modes for IDE devices

[Steven A. Falco]

Wolfgang Denk wrote:
> Dear "Steven A. Falco",
> 
> In message <48A35533.1010901@harris.com> you wrote:
>> The following patch adds the ability to call-out from the ide_ident
>> routine to a board-specific routine to set the PIO mode of an attached
>> device.
>>
>> This feature is controlled by the preprocessor variable CONFIG_TUNE_CFA.
> 
> What does CFA mean?
> 

CFA is "Compact Flash Association".  Since this hook could be used more
generally, a better name is probably CONFIG_TUNE_PIO, so that is what I
used in the respin of the patch shown below.

>> cmd_ide.c is modified to use the "drive identify information" read from the
>> device to compute a pio mode in the range 0 to 6.  This is then passed
>> to a board-specific routine, ide_set_piomode, to set the mode in
>> hardware.
>>
>> Two other files are touched: file ata.h, to include word 163, which is the
>> "advanced CF parameters", and file ide.h, to add a prototype for the
>> board-specific routine (again controlled by the CONFIG_TUNE_CFA
>> variable).
> 
> Is there a specific reason for using #ifdef's instead of a weak
> function?
> 

No reason.  The respin below uses weak linkage.  I still included ifdefs,
because I'm guessing that if one does not use the tuning feature, then we
wouldn't want the overhead of calculating the pio_mode, only to discard it.
But I can remove the ifdefs completely if you prefer.

I can also present some example code showing how I am using this hook in a
BSP I am working on.  But the BSP is not in good enough shape for
submission.

> Best regards,
> 
> Wolfgang Denk
> 

	Steve


Signed-off-by: Steven A. Falco <sfalco@harris.com>
---
 common/cmd_ide.c |   52 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 include/ata.h    |    4 +++-
 2 files changed, 55 insertions(+), 1 deletions(-)

diff --git a/common/cmd_ide.c b/common/cmd_ide.c
index b4d9719..0e435a7 100644
--- a/common/cmd_ide.c
+++ b/common/cmd_ide.c
@@ -167,6 +167,10 @@ static void input_data(int dev, ulong *sect_buf, int words);
 static void output_data(int dev, ulong *sect_buf, int words);
 static void ident_cpy (unsigned char *dest, unsigned char *src, unsigned int len);
 
+#ifdef CONFIG_TUNE_PIO
+int inline ide_set_piomode(int pio_mode);
+#endif
+
 #ifndef CFG_ATA_PORT_ADDR
 #define CFG_ATA_PORT_ADDR(port) (port)
 #endif
@@ -838,6 +842,16 @@ __ide_inb(int dev, int port)
 unsigned char inline ide_inb(int dev, int port)
 			__attribute__((weak, alias("__ide_inb")));
 
+#ifdef CONFIG_TUNE_PIO
+int inline
+__ide_set_piomode(int pio_mode)
+{
+	return 0;
+}
+int inline ide_set_piomode(int pio_mode)
+			__attribute__((weak, alias("__ide_set_piomode")));
+#endif
+
 #ifdef __PPC__
 # ifdef CONFIG_AMIGAONEG3SE
 static void
@@ -1054,6 +1068,10 @@ static void ide_ident (block_dev_desc_t *dev_desc)
 	int do_retry = 0;
 #endif
 
+#ifdef CONFIG_TUNE_PIO
+	int pio_mode;
+#endif
+
 #if 0
 	int mode, cycle_time;
 #endif
@@ -1169,6 +1187,40 @@ static void ide_ident (block_dev_desc_t *dev_desc)
 	else
 		dev_desc->removable = 0;
 
+#ifdef CONFIG_TUNE_PIO
+	/* Mode 0 - 2 only, are directly determined by word 51. */
+	pio_mode = iop->tPIO;
+	if(pio_mode > 2) {
+		printf("WARNING: Invalid PIO (word 51 = %d).\n", pio_mode);
+		pio_mode = 0; /* Force it to dead slow, and hope for the best... */
+	}
+
+	/* Any CompactFlash Storage Card that supports PIO mode 3 or above
+	 * shall set bit 1 of word 53 to one and support the fields contained
+	 * in words 64 through 70.
+	 */
+	if(iop->field_valid & 0x02) {
+		/* Mode 3 and above are possible.  Check in order from slow
+		 * to fast, so we wind up with the highest mode allowed.
+		 */
+		if(iop->eide_pio_modes & 0x01) {
+			pio_mode = 3;
+		}
+		if(iop->eide_pio_modes & 0x02) {
+			pio_mode = 4;
+		}
+		if((iop->cf_advanced_caps & 0x07) == 0x01) {
+			pio_mode = 5;
+		}
+		if((iop->cf_advanced_caps & 0x07) == 0x02) {
+			pio_mode = 6;
+		}
+	}
+
+	/* System-specific, depends on bus speeds, etc. */
+	ide_set_piomode(pio_mode);
+#endif /* CONFIG_TUNE_PIO */
+
 #if 0
 	/*
 	 * Drive PIO mode autoselection
diff --git a/include/ata.h b/include/ata.h
index aa6e90d..3269636 100644
--- a/include/ata.h
+++ b/include/ata.h
@@ -294,7 +294,9 @@ typedef struct hd_driveid {
 	unsigned short	words130_155[26];/* reserved vendor words 130-155 */
 	unsigned short	word156;
 	unsigned short	words157_159[3];/* reserved vendor words 157-159 */
-	unsigned short	words160_255[95];/* reserved words 160-255 */
+	unsigned short	words160_162[3];/* reserved words 160-162 */
+	unsigned short	cf_advanced_caps;
+	unsigned short	words164_255[92];/* reserved words 164-255 */
 } hd_driveid_t;
 
 
-- 
1.5.5.1

[U-Boot] [RFC] setting pio modes for IDE devices

[Jean-Christophe PLAGNIOL-VILLARD]

> ---
>  common/cmd_ide.c |   52 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/ata.h    |    4 +++-
>  2 files changed, 55 insertions(+), 1 deletions(-)
> 
> diff --git a/common/cmd_ide.c b/common/cmd_ide.c
> index b4d9719..0e435a7 100644
> --- a/common/cmd_ide.c
> +++ b/common/cmd_ide.c
> @@ -167,6 +167,10 @@ static void input_data(int dev, ulong *sect_buf, int words);
>  static void output_data(int dev, ulong *sect_buf, int words);
>  static void ident_cpy (unsigned char *dest, unsigned char *src, unsigned int len);
>  
> +#ifdef CONFIG_TUNE_PIO
> +int inline ide_set_piomode(int pio_mode);
> +#endif
> +
>  #ifndef CFG_ATA_PORT_ADDR
>  #define CFG_ATA_PORT_ADDR(port) (port)
>  #endif
> @@ -838,6 +842,16 @@ __ide_inb(int dev, int port)
>  unsigned char inline ide_inb(int dev, int port)
>  			__attribute__((weak, alias("__ide_inb")));
>  
why not do this?

int inline ide_set_piomode(int pio_mode) __attribute__((weak));
>  #ifdef __PPC__
>  # ifdef CONFIG_AMIGAONEG3SE
>  static void
> @@ -1054,6 +1068,10 @@ static void ide_ident (block_dev_desc_t *dev_desc)
>  	int do_retry = 0;
>  #endif
>  
> +#ifdef CONFIG_TUNE_PIO
> +	int pio_mode;
> +#endif
> +
>  #if 0
>  	int mode, cycle_time;
>  #endif
> @@ -1169,6 +1187,40 @@ static void ide_ident (block_dev_desc_t *dev_desc)
>  	else
>  		dev_desc->removable = 0;
>  
> +#ifdef CONFIG_TUNE_PIO
> +	/* Mode 0 - 2 only, are directly determined by word 51. */
> +	pio_mode = iop->tPIO;
> +	if(pio_mode > 2) {
> +		printf("WARNING: Invalid PIO (word 51 = %d).\n", pio_mode);
> +		pio_mode = 0; /* Force it to dead slow, and hope for the best... */
> +	}
> +
> +	/* Any CompactFlash Storage Card that supports PIO mode 3 or above
> +	 * shall set bit 1 of word 53 to one and support the fields contained
> +	 * in words 64 through 70.
> +	 */
> +	if(iop->field_valid & 0x02) {
> +		/* Mode 3 and above are possible.  Check in order from slow
> +		 * to fast, so we wind up with the highest mode allowed.
> +		 */
> +		if(iop->eide_pio_modes & 0x01) {
> +			pio_mode = 3;
> +		}
> +		if(iop->eide_pio_modes & 0x02) {
> +			pio_mode = 4;
> +		}
> +		if((iop->cf_advanced_caps & 0x07) == 0x01) {
> +			pio_mode = 5;
> +		}
> +		if((iop->cf_advanced_caps & 0x07) == 0x02) {
> +			pio_mode = 6;
> +		}
> +	}
> +
> +	/* System-specific, depends on bus speeds, etc. */

	if(ide_set_piomode)
		ide_set_piomode(pio_mode);

if no ide_set_piomode implementation is present gcc will drop it at
compile time IIRC.

Best Regards,
J.

[U-Boot] [RFC] setting pio modes for IDE devices

[Steven A. Falco]

Jean-Christophe PLAGNIOL-VILLARD wrote:
>> ---
>>  common/cmd_ide.c |   52 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  include/ata.h    |    4 +++-
>>  2 files changed, 55 insertions(+), 1 deletions(-)
>>
>> diff --git a/common/cmd_ide.c b/common/cmd_ide.c
>> index b4d9719..0e435a7 100644
>> --- a/common/cmd_ide.c
>> +++ b/common/cmd_ide.c
>> @@ -167,6 +167,10 @@ static void input_data(int dev, ulong *sect_buf, int words);
>>  static void output_data(int dev, ulong *sect_buf, int words);
>>  static void ident_cpy (unsigned char *dest, unsigned char *src, unsigned int len);
>>  
>> +#ifdef CONFIG_TUNE_PIO
>> +int inline ide_set_piomode(int pio_mode);
>> +#endif
>> +
>>  #ifndef CFG_ATA_PORT_ADDR
>>  #define CFG_ATA_PORT_ADDR(port) (port)
>>  #endif
>> @@ -838,6 +842,16 @@ __ide_inb(int dev, int port)
>>  unsigned char inline ide_inb(int dev, int port)
>>  			__attribute__((weak, alias("__ide_inb")));
>>  
> why not do this?
> 
> int inline ide_set_piomode(int pio_mode) __attribute__((weak));
>>  #ifdef __PPC__
>>  # ifdef CONFIG_AMIGAONEG3SE
>>  static void
>> @@ -1054,6 +1068,10 @@ static void ide_ident (block_dev_desc_t *dev_desc)
>>  	int do_retry = 0;
>>  #endif
>>  
>> +#ifdef CONFIG_TUNE_PIO
>> +	int pio_mode;
>> +#endif
>> +
>>  #if 0
>>  	int mode, cycle_time;
>>  #endif
>> @@ -1169,6 +1187,40 @@ static void ide_ident (block_dev_desc_t *dev_desc)
>>  	else
>>  		dev_desc->removable = 0;
>>  
>> +#ifdef CONFIG_TUNE_PIO
>> +	/* Mode 0 - 2 only, are directly determined by word 51. */
>> +	pio_mode = iop->tPIO;
>> +	if(pio_mode > 2) {
>> +		printf("WARNING: Invalid PIO (word 51 = %d).\n", pio_mode);
>> +		pio_mode = 0; /* Force it to dead slow, and hope for the best... */
>> +	}
>> +
>> +	/* Any CompactFlash Storage Card that supports PIO mode 3 or above
>> +	 * shall set bit 1 of word 53 to one and support the fields contained
>> +	 * in words 64 through 70.
>> +	 */
>> +	if(iop->field_valid & 0x02) {
>> +		/* Mode 3 and above are possible.  Check in order from slow
>> +		 * to fast, so we wind up with the highest mode allowed.
>> +		 */
>> +		if(iop->eide_pio_modes & 0x01) {
>> +			pio_mode = 3;
>> +		}
>> +		if(iop->eide_pio_modes & 0x02) {
>> +			pio_mode = 4;
>> +		}
>> +		if((iop->cf_advanced_caps & 0x07) == 0x01) {
>> +			pio_mode = 5;
>> +		}
>> +		if((iop->cf_advanced_caps & 0x07) == 0x02) {
>> +			pio_mode = 6;
>> +		}
>> +	}
>> +
>> +	/* System-specific, depends on bus speeds, etc. */
> 
> 	if(ide_set_piomode)
> 		ide_set_piomode(pio_mode);
> 
> if no ide_set_piomode implementation is present gcc will drop it at
> compile time IIRC.
> 
> Best Regards,
> J.
> 

At least on the x86, this generates an explicit test, even with -O2.  The generated code looks something like this:

 80483cd:	b8 00 00 00 00       	mov    $0x0,%eax
 80483d2:	85 c0                	test   %eax,%eax
 80483d4:	74 0c                	je     80483e2 <main+0x32>
 80483d6:	c7 04 24 0a 00 00 00 	movl   $0xa,(%esp)
 80483dd:	e8 1e 7c fb f7       	call   0 <_init-0x8048250>

Is there some explicit optimization flag that causes gcc to drop it at compile time?

I basically was following the style of the existing code regarding weak linkage.  See for example the ide_outb() implementation.  Also, I don't think your approach eliminates any ifdefs, because we still would not want to perform the pio_mode calculation unless we intended to use the result.

	Steve

[U-Boot] [RFC] setting pio modes for IDE devices

[Steven A. Falco]

I realized that I should be checking to see if word 163 is applicable to
the ATA device in question.  To do that, I need to call ata_id_is_cfa() from
libata.h.  However, libata.h conflicts with ata.h because of duplicate
enum values.

Therefore, this respin of the proposed patch deletes the duplicate enums
from ata.h and instead includes libata.h to supply the enums.  Then, I
can call ata_id_is_cfa() and more accurately detect PIO 5 and 6.

I believe cleaning up ata.h is a good thing, because duplicating the enums in
both places invites them to get out of sync.

Signed-off-by: Steven A. Falco <sfalco@harris.com>
---
 common/cmd_ide.c |   50 ++++++++++++++++++++++++++++++++++++++++
 include/ata.h    |   66 ++++-------------------------------------------------
 2 files changed, 55 insertions(+), 61 deletions(-)

diff --git a/common/cmd_ide.c b/common/cmd_ide.c
index b4d9719..19dd2c2 100644
--- a/common/cmd_ide.c
+++ b/common/cmd_ide.c
@@ -167,6 +167,10 @@ static void input_data(int dev, ulong *sect_buf, int words);
 static void output_data(int dev, ulong *sect_buf, int words);
 static void ident_cpy (unsigned char *dest, unsigned char *src, unsigned int len);

+#ifdef CONFIG_TUNE_PIO
+int inline ide_set_piomode(int pio_mode);
+#endif
+
 #ifndef CFG_ATA_PORT_ADDR
 #define CFG_ATA_PORT_ADDR(port) (port)
 #endif
@@ -838,6 +842,16 @@ __ide_inb(int dev, int port)
 unsigned char inline ide_inb(int dev, int port)
 			__attribute__((weak, alias("__ide_inb")));

+#ifdef CONFIG_TUNE_PIO
+int inline
+__ide_set_piomode(int pio_mode)
+{
+	return 0;
+}
+int inline ide_set_piomode(int pio_mode)
+			__attribute__((weak, alias("__ide_set_piomode")));
+#endif
+
 #ifdef __PPC__
 # ifdef CONFIG_AMIGAONEG3SE
 static void
@@ -1054,6 +1068,10 @@ static void ide_ident (block_dev_desc_t *dev_desc)
 	int do_retry = 0;
 #endif

+#ifdef CONFIG_TUNE_PIO
+	int pio_mode;
+#endif
+
 #if 0
 	int mode, cycle_time;
 #endif
@@ -1169,6 +1187,38 @@ static void ide_ident (block_dev_desc_t *dev_desc)
 	else
 		dev_desc->removable = 0;

+#ifdef CONFIG_TUNE_PIO
+	/* Mode 0 - 2 only, are directly determined by word 51. */
+	pio_mode = iop->tPIO;
+	if (pio_mode > 2) {
+		printf("WARNING: Invalid PIO (word 51 = %d).\n", pio_mode);
+		pio_mode = 0; /* Force it to dead slow, and hope for the best... */
+	}
+
+	/* Any CompactFlash Storage Card that supports PIO mode 3 or above
+	 * shall set bit 1 of word 53 to one and support the fields contained
+	 * in words 64 through 70.
+	 */
+	if (iop->field_valid & 0x02) {
+		/* Mode 3 and above are possible.  Check in order from slow
+		 * to fast, so we wind up with the highest mode allowed.
+		 */
+		if (iop->eide_pio_modes & 0x01)
+			pio_mode = 3;
+		if (iop->eide_pio_modes & 0x02)
+			pio_mode = 4;
+		if (ata_id_is_cfa((u16 *)iop)) {
+			if ((iop->cf_advanced_caps & 0x07) == 0x01)
+				pio_mode = 5;
+			if ((iop->cf_advanced_caps & 0x07) == 0x02)
+				pio_mode = 6;
+		}
+	}
+
+	/* System-specific, depends on bus speeds, etc. */
+	ide_set_piomode(pio_mode);
+#endif /* CONFIG_TUNE_PIO */
+
 #if 0
 	/*
 	 * Drive PIO mode autoselection
diff --git a/include/ata.h b/include/ata.h
index aa6e90d..2396769 100644
--- a/include/ata.h
+++ b/include/ata.h
@@ -33,6 +33,8 @@
 #ifndef	_ATA_H
 #define _ATA_H

+#include <libata.h>
+
 /* Register addressing depends on the hardware design; for instance,
  * 8-bit (register) and 16-bit (data) accesses might use different
  * address spaces. This is implemented by the following definitions.
@@ -83,66 +85,6 @@
 #define ATA_DEVICE(x)	((x & 1)<<4)
 #define ATA_LBA		0xE0

-enum {
-	ATA_MAX_DEVICES = 1,	/* per bus/port */
-	ATA_MAX_PRD = 256,	/* we could make these 256/256 */
-	ATA_SECT_SIZE = 256,	/*256 words per sector */
-
-	/* bits in ATA command block registers */
-	ATA_HOB = (1 << 7),	/* LBA48 selector */
-	ATA_NIEN = (1 << 1),	/* disable-irq flag */
-	/*ATA_LBA                 = (1 << 6), */ /* LBA28 selector */
-	ATA_DEV1 = (1 << 4),	/* Select Device 1 (slave) */
-	ATA_DEVICE_OBS = (1 << 7) | (1 << 5),	/* obs bits in dev reg */
-	ATA_DEVCTL_OBS = (1 << 3),	/* obsolete bit in devctl reg */
-	ATA_BUSY = (1 << 7),	/* BSY status bit */
-	ATA_DRDY = (1 << 6),	/* device ready */
-	ATA_DF = (1 << 5),	/* device fault */
-	ATA_DRQ = (1 << 3),	/* data request i/o */
-	ATA_ERR = (1 << 0),	/* have an error */
-	ATA_SRST = (1 << 2),	/* software reset */
-	ATA_ABORTED = (1 << 2),	/* command aborted */
-	/* ATA command block registers */
-	ATA_REG_DATA = 0x00,
-	ATA_REG_ERR = 0x01,
-	ATA_REG_NSECT = 0x02,
-	ATA_REG_LBAL = 0x03,
-	ATA_REG_LBAM = 0x04,
-	ATA_REG_LBAH = 0x05,
-	ATA_REG_DEVICE = 0x06,
-	ATA_REG_STATUS = 0x07,
-	ATA_PCI_CTL_OFS = 0x02,
-	/* and their aliases */
-	ATA_REG_FEATURE = ATA_REG_ERR,
-	ATA_REG_CMD = ATA_REG_STATUS,
-	ATA_REG_BYTEL = ATA_REG_LBAM,
-	ATA_REG_BYTEH = ATA_REG_LBAH,
-	ATA_REG_DEVSEL = ATA_REG_DEVICE,
-	ATA_REG_IRQ = ATA_REG_NSECT,
-
-	/* SETFEATURES stuff */
-	SETFEATURES_XFER = 0x03,
-	XFER_UDMA_7 = 0x47,
-	XFER_UDMA_6 = 0x46,
-	XFER_UDMA_5 = 0x45,
-	XFER_UDMA_4 = 0x44,
-	XFER_UDMA_3 = 0x43,
-	XFER_UDMA_2 = 0x42,
-	XFER_UDMA_1 = 0x41,
-	XFER_UDMA_0 = 0x40,
-	XFER_MW_DMA_2 = 0x22,
-	XFER_MW_DMA_1 = 0x21,
-	XFER_MW_DMA_0 = 0x20,
-	XFER_PIO_4 = 0x0C,
-	XFER_PIO_3 = 0x0B,
-	XFER_PIO_2 = 0x0A,
-	XFER_PIO_1 = 0x09,
-	XFER_PIO_0 = 0x08,
-	XFER_SW_DMA_2 = 0x12,
-	XFER_SW_DMA_1 = 0x11,
-	XFER_SW_DMA_0 = 0x10,
-	XFER_PIO_SLOW = 0x00
-};
 /*
  * ATA Commands (only mandatory commands listed here)
  */
@@ -294,7 +236,9 @@ typedef struct hd_driveid {
 	unsigned short	words130_155[26];/* reserved vendor words 130-155 */
 	unsigned short	word156;
 	unsigned short	words157_159[3];/* reserved vendor words 157-159 */
-	unsigned short	words160_255[95];/* reserved words 160-255 */
+	unsigned short	words160_162[3];/* reserved words 160-162 */
+	unsigned short	cf_advanced_caps;
+	unsigned short	words164_255[92];/* reserved words 164-255 */
 } hd_driveid_t;


-- 
1.5.5.1

[U-Boot] [RFC] setting pio modes for IDE devices

[Wolfgang Denk]

Dear "Steven A. Falco",

In message <48A5C296.1060801@harris.com> you wrote:
> I realized that I should be checking to see if word 163 is applicable to
> the ATA device in question.  To do that, I need to call ata_id_is_cfa() from
> libata.h.  However, libata.h conflicts with ata.h because of duplicate
> enum values.
> 
> Therefore, this respin of the proposed patch deletes the duplicate enums
> from ata.h and instead includes libata.h to supply the enums.  Then, I
> can call ata_id_is_cfa() and more accurately detect PIO 5 and 6.
> 
> I believe cleaning up ata.h is a good thing, because duplicating the enums in
> both places invites them to get out of sync.

It is, but can you please split this into two independent patches?

Thanks in advance.

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
A father doesn't destroy his children.
	-- Lt. Carolyn Palamas, "Who Mourns for Adonais?",
	   stardate 3468.1.

[U-Boot] [Patch 1/3] setting pio modes for IDE devices

[Steven A. Falco]

Wolfgang Denk wrote:
> Dear "Steven A. Falco",
> 
> In message <48A5C296.1060801@harris.com> you wrote:
>> I realized that I should be checking to see if word 163 is applicable to
>> the ATA device in question.  To do that, I need to call ata_id_is_cfa() from
>> libata.h.  However, libata.h conflicts with ata.h because of duplicate
>> enum values.
>>
>> Therefore, this respin of the proposed patch deletes the duplicate enums
>> from ata.h and instead includes libata.h to supply the enums.  Then, I
>> can call ata_id_is_cfa() and more accurately detect PIO 5 and 6.
>>
>> I believe cleaning up ata.h is a good thing, because duplicating the enums in
>> both places invites them to get out of sync.
> 
> It is, but can you please split this into two independent patches?
> 
> Thanks in advance.
> 
> Best regards,
> 
> Wolfgang Denk
> 

[PATCH 1/3] Replace enums in ata.h with an include of libata.h

This patch removes some enums from ata.h and replaces them with an
include of libata.h.  This way, we eliminate duplicated code, and
prevent errors whereby the different versions could be out of sync.

Signed-off-by: Steven A. Falco <sfalco@harris.com>
---
 include/ata.h |   62 +-------------------------------------------------------
 1 files changed, 2 insertions(+), 60 deletions(-)

diff --git a/include/ata.h b/include/ata.h
index aa6e90d..b669423 100644
--- a/include/ata.h
+++ b/include/ata.h
@@ -33,6 +33,8 @@
 #ifndef	_ATA_H
 #define _ATA_H

+#include <libata.h>
+
 /* Register addressing depends on the hardware design; for instance,
  * 8-bit (register) and 16-bit (data) accesses might use different
  * address spaces. This is implemented by the following definitions.
@@ -83,66 +85,6 @@
 #define ATA_DEVICE(x)	((x & 1)<<4)
 #define ATA_LBA		0xE0

-enum {
-	ATA_MAX_DEVICES = 1,	/* per bus/port */
-	ATA_MAX_PRD = 256,	/* we could make these 256/256 */
-	ATA_SECT_SIZE = 256,	/*256 words per sector */
-
-	/* bits in ATA command block registers */
-	ATA_HOB = (1 << 7),	/* LBA48 selector */
-	ATA_NIEN = (1 << 1),	/* disable-irq flag */
-	/*ATA_LBA                 = (1 << 6), */ /* LBA28 selector */
-	ATA_DEV1 = (1 << 4),	/* Select Device 1 (slave) */
-	ATA_DEVICE_OBS = (1 << 7) | (1 << 5),	/* obs bits in dev reg */
-	ATA_DEVCTL_OBS = (1 << 3),	/* obsolete bit in devctl reg */
-	ATA_BUSY = (1 << 7),	/* BSY status bit */
-	ATA_DRDY = (1 << 6),	/* device ready */
-	ATA_DF = (1 << 5),	/* device fault */
-	ATA_DRQ = (1 << 3),	/* data request i/o */
-	ATA_ERR = (1 << 0),	/* have an error */
-	ATA_SRST = (1 << 2),	/* software reset */
-	ATA_ABORTED = (1 << 2),	/* command aborted */
-	/* ATA command block registers */
-	ATA_REG_DATA = 0x00,
-	ATA_REG_ERR = 0x01,
-	ATA_REG_NSECT = 0x02,
-	ATA_REG_LBAL = 0x03,
-	ATA_REG_LBAM = 0x04,
-	ATA_REG_LBAH = 0x05,
-	ATA_REG_DEVICE = 0x06,
-	ATA_REG_STATUS = 0x07,
-	ATA_PCI_CTL_OFS = 0x02,
-	/* and their aliases */
-	ATA_REG_FEATURE = ATA_REG_ERR,
-	ATA_REG_CMD = ATA_REG_STATUS,
-	ATA_REG_BYTEL = ATA_REG_LBAM,
-	ATA_REG_BYTEH = ATA_REG_LBAH,
-	ATA_REG_DEVSEL = ATA_REG_DEVICE,
-	ATA_REG_IRQ = ATA_REG_NSECT,
-
-	/* SETFEATURES stuff */
-	SETFEATURES_XFER = 0x03,
-	XFER_UDMA_7 = 0x47,
-	XFER_UDMA_6 = 0x46,
-	XFER_UDMA_5 = 0x45,
-	XFER_UDMA_4 = 0x44,
-	XFER_UDMA_3 = 0x43,
-	XFER_UDMA_2 = 0x42,
-	XFER_UDMA_1 = 0x41,
-	XFER_UDMA_0 = 0x40,
-	XFER_MW_DMA_2 = 0x22,
-	XFER_MW_DMA_1 = 0x21,
-	XFER_MW_DMA_0 = 0x20,
-	XFER_PIO_4 = 0x0C,
-	XFER_PIO_3 = 0x0B,
-	XFER_PIO_2 = 0x0A,
-	XFER_PIO_1 = 0x09,
-	XFER_PIO_0 = 0x08,
-	XFER_SW_DMA_2 = 0x12,
-	XFER_SW_DMA_1 = 0x11,
-	XFER_SW_DMA_0 = 0x10,
-	XFER_PIO_SLOW = 0x00
-};
 /*
  * ATA Commands (only mandatory commands listed here)
  */
-- 
1.5.5.1

[U-Boot] [PATCH 2/3] setting pio modes for IDE devices

[Steven A. Falco]

Wolfgang Denk wrote:
> Dear "Steven A. Falco",
> 
> In message <48A5C296.1060801@harris.com> you wrote:
>> I realized that I should be checking to see if word 163 is applicable to
>> the ATA device in question.  To do that, I need to call ata_id_is_cfa() from
>> libata.h.  However, libata.h conflicts with ata.h because of duplicate
>> enum values.
>>
>> Therefore, this respin of the proposed patch deletes the duplicate enums
>> from ata.h and instead includes libata.h to supply the enums.  Then, I
>> can call ata_id_is_cfa() and more accurately detect PIO 5 and 6.
>>
>> I believe cleaning up ata.h is a good thing, because duplicating the enums in
>> both places invites them to get out of sync.
> 
> It is, but can you please split this into two independent patches?
> 
> Thanks in advance.
> 
> Best regards,
> 
> Wolfgang Denk
> 

[PATCH 2/3] Add a hook to allow board-specific PIO mode setting.

This patch adds a hook whereby a board-specific routine can be called to
configure hardware for a PIO mode.  The prototype for the board-specific
routine is:

	int inline ide_set_piomode(int pio_mode)

ide_set_piomode should be prepared to configure hardware for a pio_mode
between 0 and 6, inclusive.  It should return 0 on success or 1 on failure.

Signed-off-by: Steven A. Falco <sfalco@harris.com>
---
 common/cmd_ide.c |   46 ++++++++++++++++++++++++++++++++++++++++++++++
 include/ata.h    |    4 +++-
 2 files changed, 49 insertions(+), 1 deletions(-)

diff --git a/common/cmd_ide.c b/common/cmd_ide.c
index d6ba79f..0691007 100644
--- a/common/cmd_ide.c
+++ b/common/cmd_ide.c
@@ -543,6 +543,16 @@ __ide_inb(int dev, int port)
 unsigned char inline ide_inb(int dev, int port)
 			__attribute__((weak, alias("__ide_inb")));

+#ifdef CONFIG_TUNE_PIO
+int inline
+__ide_set_piomode(int pio_mode)
+{
+	return 0;
+}
+int inline ide_set_piomode(int pio_mode)
+			__attribute__((weak, alias("__ide_set_piomode")));
+#endif
+
 void ide_init (void)
 {

@@ -1053,6 +1063,10 @@ static void ide_ident (block_dev_desc_t *dev_desc)
 	int do_retry = 0;
 #endif

+#ifdef CONFIG_TUNE_PIO
+	int pio_mode;
+#endif
+
 #if 0
 	int mode, cycle_time;
 #endif
@@ -1168,6 +1182,38 @@ static void ide_ident (block_dev_desc_t *dev_desc)
 	else
 		dev_desc->removable = 0;

+#ifdef CONFIG_TUNE_PIO
+	/* Mode 0 - 2 only, are directly determined by word 51. */
+	pio_mode = iop->tPIO;
+	if (pio_mode > 2) {
+		printf("WARNING: Invalid PIO (word 51 = %d).\n", pio_mode);
+		pio_mode = 0; /* Force it to dead slow, and hope for the best... */
+	}
+
+	/* Any CompactFlash Storage Card that supports PIO mode 3 or above
+	 * shall set bit 1 of word 53 to one and support the fields contained
+	 * in words 64 through 70.
+	 */
+	if (iop->field_valid & 0x02) {
+		/* Mode 3 and above are possible.  Check in order from slow
+		 * to fast, so we wind up with the highest mode allowed.
+		 */
+		if (iop->eide_pio_modes & 0x01)
+			pio_mode = 3;
+		if (iop->eide_pio_modes & 0x02)
+			pio_mode = 4;
+		if (ata_id_is_cfa((u16 *)iop)) {
+			if ((iop->cf_advanced_caps & 0x07) == 0x01)
+				pio_mode = 5;
+			if ((iop->cf_advanced_caps & 0x07) == 0x02)
+				pio_mode = 6;
+		}
+	}
+
+	/* System-specific, depends on bus speeds, etc. */
+	ide_set_piomode(pio_mode);
+#endif /* CONFIG_TUNE_PIO */
+
 #if 0
 	/*
 	 * Drive PIO mode autoselection
diff --git a/include/ata.h b/include/ata.h
index b669423..2396769 100644
--- a/include/ata.h
+++ b/include/ata.h
@@ -236,7 +236,9 @@ typedef struct hd_driveid {
 	unsigned short	words130_155[26];/* reserved vendor words 130-155 */
 	unsigned short	word156;
 	unsigned short	words157_159[3];/* reserved vendor words 157-159 */
-	unsigned short	words160_255[95];/* reserved words 160-255 */
+	unsigned short	words160_162[3];/* reserved words 160-162 */
+	unsigned short	cf_advanced_caps;
+	unsigned short	words164_255[92];/* reserved words 164-255 */
 } hd_driveid_t;


-- 
1.5.5.1

[U-Boot] [PATCH 3/3] setting pio modes for IDE devices

[Steven A. Falco]

Wolfgang Denk wrote:
> Dear "Steven A. Falco",
> 
> In message <48A5C296.1060801@harris.com> you wrote:
>> I realized that I should be checking to see if word 163 is applicable to
>> the ATA device in question.  To do that, I need to call ata_id_is_cfa() from
>> libata.h.  However, libata.h conflicts with ata.h because of duplicate
>> enum values.
>>
>> Therefore, this respin of the proposed patch deletes the duplicate enums
>> from ata.h and instead includes libata.h to supply the enums.  Then, I
>> can call ata_id_is_cfa() and more accurately detect PIO 5 and 6.
>>
>> I believe cleaning up ata.h is a good thing, because duplicating the enums in
>> both places invites them to get out of sync.
> 
> It is, but can you please split this into two independent patches?
> 
> Thanks in advance.
> 
> Best regards,
> 
> Wolfgang Denk
> 

[PATCH 3/3] Typo in spelling of ATAPI.

Correct a small spelling mistake.

Signed-off-by: Steven A. Falco <sfalco@harris.com>
---
 common/cmd_ide.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/common/cmd_ide.c b/common/cmd_ide.c
index 0691007..a744b41 100644
--- a/common/cmd_ide.c
+++ b/common/cmd_ide.c
@@ -1822,7 +1822,7 @@ unsigned char atapi_issue(int device,unsigned char*
ccb,int ccblen, unsigned cha
 	c = atapi_wait_mask(device,ATAPI_TIME_OUT,mask,res);

 	if ((c & mask) != res) { /* DRQ must be 1, BSY 0 */
-		printf ("ATTAPI_ISSUE: Error (no IRQ) before sending ccb dev %d status
0x%02x\n",device,c);
+		printf ("ATAPI_ISSUE: Error (no IRQ) before sending ccb dev %d status
0x%02x\n",device,c);
 		err=0xFF;
 		goto AI_OUT;
 	}
-- 
1.5.5.1

[U-Boot] [Patch 1/3] setting pio modes for IDE devices

[Wolfgang Denk]

Dear "Steven A. Falco",

In message <48A5D908.9020308@harris.com> you wrote:
>
> [PATCH 1/3] Replace enums in ata.h with an include of libata.h
> 
> This patch removes some enums from ata.h and replaces them with an
> include of libata.h.  This way, we eliminate duplicated code, and
> prevent errors whereby the different versions could be out of sync.
> 
> Signed-off-by: Steven A. Falco <sfalco@harris.com>
> ---
>  include/ata.h |   62 +-------------------------------------------------------
>  1 files changed, 2 insertions(+), 60 deletions(-)

Applied, thanks.

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
READ THIS BEFORE OPENING PACKAGE: According to Certain Suggested Ver-
sions of the Grand Unified Theory, the Primary Particles Constituting
this Product May Decay to Nothingness Within the  Next  Four  Hundred
Million Years.

[U-Boot] [PATCH 2/3] setting pio modes for IDE devices

[Wolfgang Denk]

Dear "Steven A. Falco",

In message <48A5DA32.10706@harris.com> you wrote:
> 
> [PATCH 2/3] Add a hook to allow board-specific PIO mode setting.
> 
> This patch adds a hook whereby a board-specific routine can be called to
> configure hardware for a PIO mode.  The prototype for the board-specific
> routine is:
> 
> 	int inline ide_set_piomode(int pio_mode)
> 
> ide_set_piomode should be prepared to configure hardware for a pio_mode
> between 0 and 6, inclusive.  It should return 0 on success or 1 on failure.
> 
> Signed-off-by: Steven A. Falco <sfalco@harris.com>
> ---
>  common/cmd_ide.c |   46 ++++++++++++++++++++++++++++++++++++++++++++++
>  include/ata.h    |    4 +++-
>  2 files changed, 49 insertions(+), 1 deletions(-)

Applied, thanks.

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
No man knows what true happiness is until he gets married.  By  then,
of course, its too late.

[U-Boot] [PATCH 3/3] setting pio modes for IDE devices

[Wolfgang Denk]

Dear "Steven A. Falco",

In message <48A5DAFB.10007@harris.com> you wrote:
> Wolfgang Denk wrote:
> > Dear "Steven A. Falco",
> > 
> > In message <48A5C296.1060801@harris.com> you wrote:
> >> I realized that I should be checking to see if word 163 is applicable to
> >> the ATA device in question.  To do that, I need to call ata_id_is_cfa() from
> >> libata.h.  However, libata.h conflicts with ata.h because of duplicate
> >> enum values.
> >>
> >> Therefore, this respin of the proposed patch deletes the duplicate enums
> >> from ata.h and instead includes libata.h to supply the enums.  Then, I
> >> can call ata_id_is_cfa() and more accurately detect PIO 5 and 6.
> >>
> >> I believe cleaning up ata.h is a good thing, because duplicating the enums in
> >> both places invites them to get out of sync.
> > 
> > It is, but can you please split this into two independent patches?
> > 
> > Thanks in advance.
> > 
> > Best regards,
> > 
> > Wolfgang Denk
> > 
> 
> [PATCH 3/3] Typo in spelling of ATAPI.
> 
> Correct a small spelling mistake.
> 
> Signed-off-by: Steven A. Falco <sfalco@harris.com>
> ---
>  common/cmd_ide.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/common/cmd_ide.c b/common/cmd_ide.c
> index 0691007..a744b41 100644
> --- a/common/cmd_ide.c
> +++ b/common/cmd_ide.c
> @@ -1822,7 +1822,7 @@ unsigned char atapi_issue(int device,unsigned char*
> ccb,int ccblen, unsigned cha
>  	c = atapi_wait_mask(device,ATAPI_TIME_OUT,mask,res);
> 
>  	if ((c & mask) != res) { /* DRQ must be 1, BSY 0 */
> -		printf ("ATTAPI_ISSUE: Error (no IRQ) before sending ccb dev %d status
> 0x%02x\n",device,c);
^^^^^^^^^^^^^^^^^^^^^^^^
> +		printf ("ATAPI_ISSUE: Error (no IRQ) before sending ccb dev %d status
> 0x%02x\n",device,c);
^^^^^^^^^^^^^^^^^^^^^^^^
>  		err=0xFF;

This patch is line-wrapped. Please fix your mailer.

[Applied manually]

And *please* stick to the rules and put any  comments  that  are  not
supposed to become part of the commit message *below* the "---" line.

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
    \|/ ____ \|/                                     \|/ ____ \|/
     @~/ ,. \~@                                       @~/ ,. \~@
    /_( \__/ )_\                                     /_( \__/ )_\
       \__U_/                                           \__U_/