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

older
Re: [U-Boot] [U-boot-Users][GIT...

Steven A. Falco

13 Aug 2008 13 Aug '08

11:42 p.m.

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 */

Show replies by date

Wolfgang Denk

14 Aug 14 Aug

1:20 a.m.

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@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"

Steven A. Falco

4:33 p.m.

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

Jean-Christophe PLAGNIOL-VILLARD

6:42 p.m.

...

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.

Steven A. Falco

15 Aug 15 Aug

5:23 p.m.

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

Steven A. Falco

7:53 p.m.

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 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

Wolfgang Denk

8:12 p.m.

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

Steven A. Falco

9:29 p.m.

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

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

-- 1.5.5.1

Wolfgang Denk

21 Aug 21 Aug

1:21 a.m.

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

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@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.

Steven A. Falco

15 Aug 15 Aug

9:34 p.m.

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

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")));

@@ -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

Wolfgang Denk

21 Aug 21 Aug

1:31 a.m.

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

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

Steven A. Falco

15 Aug 15 Aug

9:37 p.m.

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

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

Wolfgang Denk

21 Aug 21 Aug

1:33 a.m.

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

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

6108

Age (days ago)

6115

Last active (days ago)

List overview

Download

12 comments

3 participants

tags (0)

participants (3)

Jean-Christophe PLAGNIOL-VILLARD
Steven A. Falco
Wolfgang Denk