[U-Boot-Users] [PATCH] Merge code duplication in ata.h and libata.h

[U-Boot-Users] [PATCH] Merge code duplication in ata.h and libata.h

[Tor Krill]

ata.h and libata.h contains duplicate and conflicting definitions. This
patch tries to resolve this and make them both includeable in a sourcefile.

Also updated is the fsl_sata driver which uses libata.h

Signed-off-by: Tor Krill <tor@excito.com>
---
 drivers/block/fsl_sata.c |   15 ++++++++-------
 include/ata.h            |   15 +++++++++++++--
 include/libata.h         |   45 +--------------------------------------------
 3 files changed, 22 insertions(+), 53 deletions(-)

diff --git a/drivers/block/fsl_sata.c b/drivers/block/fsl_sata.c
index 7436c4d..2b63ac4 100644
--- a/drivers/block/fsl_sata.c
+++ b/drivers/block/fsl_sata.c
@@ -23,6 +23,7 @@
 #include <asm/io.h>
 #include <malloc.h>
 #include <libata.h>
+#include <ata.h>
 #include <fis.h>
 #include "fsl_sata.h"
 
@@ -609,7 +610,7 @@ static u32 fsl_sata_rw_cmd(int dev, u32 start, u32 blkcnt, u8 *buffer, int is_wr
 	cfis->lba_low = block & 0xff;
 	cfis->sector_count = (u8)(blkcnt & 0xff);
 
-	fsl_sata_exec_cmd(sata, cfis, CMD_ATA, 0, buffer, ATA_SECT_SIZE * blkcnt);
+	fsl_sata_exec_cmd(sata, cfis, CMD_ATA, 0, buffer, ATA_SECT_BYTESIZE * blkcnt);
 	return blkcnt;
 }
 
@@ -658,7 +659,7 @@ static u32 fsl_sata_rw_cmd_ext(int dev, u32 start, u32 blkcnt, u8 *buffer, int i
 	cfis->sector_count_exp = (blkcnt >> 8) & 0xff;
 	cfis->sector_count = blkcnt & 0xff;
 
-	fsl_sata_exec_cmd(sata, cfis, CMD_ATA, 0, buffer, ATA_SECT_SIZE * blkcnt);
+	fsl_sata_exec_cmd(sata, cfis, CMD_ATA, 0, buffer, ATA_SECT_BYTESIZE * blkcnt);
 	return blkcnt;
 }
 
@@ -703,7 +704,7 @@ u32 fsl_sata_rw_ncq_cmd(int dev, u32 start, u32 blkcnt, u8 *buffer, int is_write
 		ncq_channel = sata->queue_depth - 1;
 
 	/* Use the latest queue */
-	fsl_sata_exec_cmd(sata, cfis, CMD_NCQ, ncq_channel, buffer, ATA_SECT_SIZE * blkcnt);
+	fsl_sata_exec_cmd(sata, cfis, CMD_NCQ, ncq_channel, buffer, ATA_SECT_BYTESIZE * blkcnt);
 	return blkcnt;
 }
 
@@ -779,7 +780,7 @@ u32 ata_low_level_rw_lba48(int dev, u32 blknr, u32 blkcnt, void *buffer, int is_
 				fsl_sata_rw_ncq_cmd(dev, start, max_blks, addr, is_write);
 			start += max_blks;
 			blks -= max_blks;
-			addr += ATA_SECT_SIZE * max_blks;
+			addr += ATA_SECT_BYTESIZE * max_blks;
 		} else {
 			if (fsl_sata_info[dev].flags != FLAGS_FPDMA)
 				fsl_sata_rw_cmd_ext(dev, start, blks, addr, is_write);
@@ -787,7 +788,7 @@ u32 ata_low_level_rw_lba48(int dev, u32 blknr, u32 blkcnt, void *buffer, int is_
 				fsl_sata_rw_ncq_cmd(dev, start, blks, addr, is_write);
 			start += blks;
 			blks = 0;
-			addr += ATA_SECT_SIZE * blks;
+			addr += ATA_SECT_BYTESIZE * blks;
 		}
 	} while (blks != 0);
 
@@ -810,12 +811,12 @@ u32 ata_low_level_rw_lba28(int dev, u32 blknr, u32 blkcnt, void *buffer, int is_
 			fsl_sata_rw_cmd(dev, start, max_blks, addr, is_write);
 			start += max_blks;
 			blks -= max_blks;
-			addr += ATA_SECT_SIZE * max_blks;
+			addr += ATA_SECT_BYTESIZE * max_blks;
 		} else {
 			fsl_sata_rw_cmd(dev, start, blks, addr, is_write);
 			start += blks;
 			blks = 0;
-			addr += ATA_SECT_SIZE * blks;
+			addr += ATA_SECT_BYTESIZE * blks;
 		}
 	} while (blks != 0);
 
diff --git a/include/ata.h b/include/ata.h
index aa6e90d..54a1455 100644
--- a/include/ata.h
+++ b/include/ata.h
@@ -119,8 +119,10 @@ enum {
 	ATA_REG_BYTEH = ATA_REG_LBAH,
 	ATA_REG_DEVSEL = ATA_REG_DEVICE,
 	ATA_REG_IRQ = ATA_REG_NSECT,
+};
 
-	/* SETFEATURES stuff */
+/* SETFEATURES stuff */
+enum ata_set_features{
 	SETFEATURES_XFER = 0x03,
 	XFER_UDMA_7 = 0x47,
 	XFER_UDMA_6 = 0x46,
@@ -130,9 +132,13 @@ enum {
 	XFER_UDMA_2 = 0x42,
 	XFER_UDMA_1 = 0x41,
 	XFER_UDMA_0 = 0x40,
+	XFER_MW_DMA_4	= 0x24, /* CFA only */
+	XFER_MW_DMA_3	= 0x23, /* CFA only */
 	XFER_MW_DMA_2 = 0x22,
 	XFER_MW_DMA_1 = 0x21,
 	XFER_MW_DMA_0 = 0x20,
+	XFER_PIO_6		= 0x0E, /* CFA only */
+	XFER_PIO_5		= 0x0D, /* CFA only */
 	XFER_PIO_4 = 0x0C,
 	XFER_PIO_3 = 0x0B,
 	XFER_PIO_2 = 0x0A,
@@ -141,7 +147,12 @@ enum {
 	XFER_SW_DMA_2 = 0x12,
 	XFER_SW_DMA_1 = 0x11,
 	XFER_SW_DMA_0 = 0x10,
-	XFER_PIO_SLOW = 0x00
+	XFER_PIO_SLOW = 0x00,
+
+	SETFEATURES_WC_ON	= 0x02, /* Enable write cache */
+	SETFEATURES_WC_OFF	= 0x82, /* Disable write cache */
+
+	SETFEATURES_SPINUP	= 0x07, /* Spin-up drive */
 };
 /*
  * ATA Commands (only mandatory commands listed here)
diff --git a/include/libata.h b/include/libata.h
index aedba74..246dcd3 100644
--- a/include/libata.h
+++ b/include/libata.h
@@ -49,39 +49,6 @@ enum ata_cmd {
 	ATA_CMD_WRITE_FPDMA_QUEUED	= 0x61,
 };
 
-/* SETFEATURES stuff
- */
-enum ata_set_features {
-	/* 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_4		= 0x24, /* CFA only */
-	XFER_MW_DMA_3		= 0x23, /* CFA only */
-	XFER_MW_DMA_2		= 0x22,
-	XFER_MW_DMA_1		= 0x21,
-	XFER_MW_DMA_0		= 0x20,
-	XFER_PIO_6		= 0x0E, /* CFA only */
-	XFER_PIO_5		= 0x0D, /* CFA only */
-	XFER_PIO_4		= 0x0C,
-	XFER_PIO_3		= 0x0B,
-	XFER_PIO_2		= 0x0A,
-	XFER_PIO_1		= 0x09,
-	XFER_PIO_0		= 0x08,
-	XFER_PIO_SLOW		= 0x00,
-
-	SETFEATURES_WC_ON	= 0x02, /* Enable write cache */
-	SETFEATURES_WC_OFF	= 0x82, /* Disable write cache */
-
-	SETFEATURES_SPINUP	= 0x07, /* Spin-up drive */
-};
-
 enum ata_protocol {
 	ATA_PROT_UNKNOWN,	/* unknown */
 	ATA_PROT_NODATA,	/* no data */
@@ -101,26 +68,16 @@ enum ata_dev_typed {
 };
 
 enum {
-	ATA_SECT_SIZE		= 512,
+	ATA_SECT_BYTESIZE		= 512,
 	ATA_MAX_SECTORS_128	= 128,
 	ATA_MAX_SECTORS		= 256,
 	ATA_MAX_SECTORS_LBA48	= 65535,
 
 	/* 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_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_ICRC		= (1 << 7),	/* interface CRC error */
 	ATA_UNC			= (1 << 6),	/* uncorrectable media error */
 	ATA_IDNF		= (1 << 4),	/* ID not found */
-	ATA_ABORTED		= (1 << 2),	/* command aborted */
 
 	ATA_ID_WORDS		= 256,
 	ATA_ID_SERNO		= 10,
-- 
1.5.4.4

[U-Boot-Users] [PATCH] Merge code duplication in ata.h and libata.h

[Dave Liu]

> -       fsl_sata_exec_cmd(sata, cfis, CMD_ATA, 0, buffer,
> ATA_SECT_SIZE * blkcnt);
> +       fsl_sata_exec_cmd(sata, cfis, CMD_ATA, 0, buffer,
> ATA_SECT_BYTESIZE * blkcnt);
>         return blkcnt;
>  }
> 
> @@ -658,7 +659,7 @@ static u32 fsl_sata_rw_cmd_ext(int dev, u32 start,
> u32 blkcnt, u8 *buffer, int i
>         cfis->sector_count_exp = (blkcnt >> 8) & 0xff;
>         cfis->sector_count = blkcnt & 0xff;
> 
> -       fsl_sata_exec_cmd(sata, cfis, CMD_ATA, 0, buffer,
> ATA_SECT_SIZE * blkcnt);
> +       fsl_sata_exec_cmd(sata, cfis, CMD_ATA, 0, buffer,
> ATA_SECT_BYTESIZE * blkcnt);
>         return blkcnt;


Nack.

Don't do this.

I don't think the ATA_SECT_BYTESIZE is better than
the ATA_SECT_SIZE.

The libata.h is closely than the latest linux kernel.

[U-Boot-Users] [PATCH] Merge code duplication in ata.h and libata.h

[Dave Liu]

> >Don't do this.
> >
> >I don't think the ATA_SECT_BYTESIZE is better than
> >the ATA_SECT_SIZE.
> 
> The big problem here is that ata.h defines ATA_SECT_SIZE as 256 Words
> where libata defines it as 512 Bytes.
> 
> I did not want to change the old define since it would possibly break
> more than the fsl driver.

When I grep 'ATA_SECT_SIZE', I can *not* find any files including the
ATA_SECT_SIZE before the fsl_sata.c.

> But the problem still remains, it is currently impossible to use both
> libata.h and ata.h.

Why need use both them?
I believe the libata.h is enough.

If the libata.h is not enough for you, I suggest you update it from the
latest linux kernel.

I think it is correct way to handle it.
It will be more easy to to maintain the header.

[U-Boot-Users] [PATCH] Merge code duplication in ata.h and libata.h

[Tor Krill]

On 3/31/2008, "Dave Liu" <r63238@freescale.com> wrote:

>
>> -       fsl_sata_exec_cmd(sata, cfis, CMD_ATA, 0, buffer,
>> ATA_SECT_SIZE * blkcnt);
>> +       fsl_sata_exec_cmd(sata, cfis, CMD_ATA, 0, buffer,
>> ATA_SECT_BYTESIZE * blkcnt);
>>         return blkcnt;
>>  }
>>
>> @@ -658,7 +659,7 @@ static u32 fsl_sata_rw_cmd_ext(int dev, u32 start,
>> u32 blkcnt, u8 *buffer, int i
>>         cfis->sector_count_exp = (blkcnt >> 8) & 0xff;
>>         cfis->sector_count = blkcnt & 0xff;
>>
>> -       fsl_sata_exec_cmd(sata, cfis, CMD_ATA, 0, buffer,
>> ATA_SECT_SIZE * blkcnt);
>> +       fsl_sata_exec_cmd(sata, cfis, CMD_ATA, 0, buffer,
>> ATA_SECT_BYTESIZE * blkcnt);
>>         return blkcnt;
>
>
>Nack.
>
>Don't do this.
>
>I don't think the ATA_SECT_BYTESIZE is better than
>the ATA_SECT_SIZE.

The big problem here is that ata.h defines ATA_SECT_SIZE as 256 Words
where libata defines it as 512 Bytes.

I did not want to change the old define since it would possibly break
more than the fsl driver.

If its ok to change the definition to 512 Bytes and only change the
ata_piix driver ignoring other possible uses of it i could resubmit the
patch.

But the problem still remains, it is currently impossible to use both
libata.h and ata.h.

/Tor

[U-Boot-Users] [PATCH] Merge code duplication in ata.h and libata.h

[Dave Liu]

> >When I grep 'ATA_SECT_SIZE', I can *not* find any files including the
> >ATA_SECT_SIZE before the fsl_sata.c.
> 
> Well, the ata_piix driver for one uses it, but only includes ata.h. But
> that is the only one in the tree right now.
> 
> >> But the problem still remains, it is currently impossible to use both
> >> libata.h and ata.h.
> >
> >Why need use both them?
> >I believe the libata.h is enough.
> 
> Since my driver for the sil3114 uses "legacy mode" it uses a lot of the
> ata commands directly in PIO mode. These are defined in ata.h (Both in
> u-boot and Linux)

I believe the linux kernel happen at legacy mode same as you, but kernel
can handle the legacy mode, why you can not hanlde the legacy mode in u-
boot with same header file.

If you're using the I/O register descriptions and status register bits
in ata.h of u-boot. the kernel header have the same register/bit
definition. just like

ATA_REG_DATA
ATA_REG_ERR
...

ATA_BUSY
ATA_DRDY
...
ATA_ERR
ATA_SRST

[U-Boot-Users] [PATCH] Merge code duplication in ata.h and libata.h

[Tor Krill]

On 3/31/2008, "Dave Liu" <r63238@freescale.com> wrote:

>> >Don't do this.
>> >
>> >I don't think the ATA_SECT_BYTESIZE is better than
>> >the ATA_SECT_SIZE.
>>
>> The big problem here is that ata.h defines ATA_SECT_SIZE as 256 Words
>> where libata defines it as 512 Bytes.
>>
>> I did not want to change the old define since it would possibly break
>> more than the fsl driver.
>
>When I grep 'ATA_SECT_SIZE', I can *not* find any files including the
>ATA_SECT_SIZE before the fsl_sata.c.

Well, the ata_piix driver for one uses it, but only includes ata.h. But
that is the only one in the tree right now.

>> But the problem still remains, it is currently impossible to use both
>> libata.h and ata.h.
>
>Why need use both them?
>I believe the libata.h is enough.

Since my driver for the sil3114 uses "legacy mode" it uses a lot of the
ata commands directly in PIO mode. These are defined in ata.h (Both in
u-boot and Linux)

>If the libata.h is not enough for you, I suggest you update it from the
>latest linux kernel.

Still, the kernel has both a ata.h and a libata.h, where libata.h
includes ata.h. And where ATA_SECT_SIZE, and the other defines i use,
actually is defined in ata.h albeit as 512 bytes :).

>I think it is correct way to handle it.
>It will be more easy to to maintain the header.

I have no problem with either solution. If its ok to add the defines to
libata.h instead i will do that?

/Tor

[U-Boot-Users] [PATCH] Merge code duplication in ata.h and libata.h

[Wolfgang Denk]

In message <MdNGBCPY.1206951652.2688260.tor@localhost> you wrote:
> 
> >I don't think the ATA_SECT_BYTESIZE is better than
> >the ATA_SECT_SIZE.

Actually it's worse.

> The big problem here is that ata.h defines ATA_SECT_SIZE as 256 Words
> where libata defines it as 512 Bytes.

But "BYTESIZE" reads to me as "size of byte", just as if ATA sectors
used something different than 8 bits per byte.

If you need such a new definition, then please call it different.

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
Love sometimes expresses itself in sacrifice.
	-- Kirk, "Metamorphosis", stardate 3220.3