[PATCH 2/2] Fix IDE commands issued, fix endian issues, fix non MMIO

Heinrich Schuchardt
2021-02-22T18:59:33Z

On 2/22/21 6:05 PM, Reinoud Zandijk wrote:

A patch without commit message will not be accepted.

Please, run scripts/checkpatch.pl on your patch:

ERROR: Missing Signed-off-by: line(s)

> ---
> drivers/block/ide.c | 143 +++++++++++++-------------------------------
> include/ata.h | 3 +-
> 2 files changed, 42 insertions(+), 104 deletions(-)
>
> diff --git a/drivers/block/ide.c b/drivers/block/ide.c
> index 43a0776099..fa3481e936 100644
> --- a/drivers/block/ide.c

Your patch is mal-formed:

ERROR: Avoid using diff content in the commit message - patch(1) might
not work
#126:
--- a/drivers/block/ide.c

> +++ b/drivers/block/ide.c
> @@ -130,56 +130,40 @@ OUT:
> * ATAPI Support
> */
>
> -#if defined(CONFIG_IDE_SWAP_IO)

As far as possible replace #if by if.

> /* since ATAPI may use commands with not 4 bytes alligned length
> * we have our own transfer functions, 2 bytes alligned */
> __weak void ide_output_data_shorts(int dev, ushort *sect_buf, int shorts)
> {
> ushort *dbuf;
> - volatile ushort *pbuf;
> + u64 paddr = (ATA_CURR_BASE(dev) + ATA_DATA_REG);

ERROR: DOS line endings
#139: FILE: drivers/block/ide.c:138:
+^Iu64 paddr = (ATA_CURR_BASE(dev) + ATA_DATA_REG);^M$

>
> - pbuf = (ushort *)(ATA_CURR_BASE(dev) + ATA_DATA_REG);
> dbuf = (ushort *)sect_buf;
>
> debug("in output data shorts base for read is %lx\n",
> - (unsigned long) pbuf);
> + (unsigned long) paddr);
>
> while (shorts--) {
> EIEIO;
> - *pbuf = *dbuf++;
> + outw(paddr, cpu_to_le16(*dbuf++));
> }
> }
>
> __weak void ide_input_data_shorts(int dev, ushort *sect_buf, int shorts)
> {
> ushort *dbuf;
> - volatile ushort *pbuf;
> + u64 paddr = (ATA_CURR_BASE(dev) + ATA_DATA_REG);
>
> - pbuf = (ushort *)(ATA_CURR_BASE(dev) + ATA_DATA_REG);
> dbuf = (ushort *)sect_buf;
>
> debug("in input data shorts base for read is %lx\n",
> - (unsigned long) pbuf);
> + (unsigned long) paddr);
>
> while (shorts--) {
> EIEIO;
> - *dbuf++ = *pbuf;
> + *dbuf++ = le16_to_cpu(inw(paddr));
> }
> }
>
> -#else /* ! CONFIG_IDE_SWAP_IO */
> -__weak void ide_output_data_shorts(int dev, ushort *sect_buf, int shorts)
> -{
> - outsw(ATA_CURR_BASE(dev) + ATA_DATA_REG, sect_buf, shorts);
> -}
> -
> -__weak void ide_input_data_shorts(int dev, ushort *sect_buf, int shorts)
> -{
> - insw(ATA_CURR_BASE(dev) + ATA_DATA_REG, sect_buf, shorts);
> -}
> -
> -#endif /* CONFIG_IDE_SWAP_IO */
> -
> /*
> * Wait until (Status & mask) == res, or timeout (in ms)
> * Return last status
> @@ -636,19 +620,6 @@ static void ide_ident(struct blk_desc *dev_desc)
> sizeof(dev_desc->vendor));
> ident_cpy((unsigned char *)dev_desc->product, iop.serial_no,
> sizeof(dev_desc->product));
> -#ifdef __LITTLE_ENDIAN
> - /*
> - * firmware revision, model, and serial number have Big Endian Byte
> - * order in Word. Convert all three to little endian.
> - *
> - * See CF+ and CompactFlash Specification Revision 2.0:
> - * 6.2.1.6: Identify Drive, Table 39 for more details
> - */
> -
> - strswab(dev_desc->revision);
> - strswab(dev_desc->vendor);
> - strswab(dev_desc->product);
> -#endif /* __LITTLE_ENDIAN */
>
> if ((iop.config & 0x0080) == 0x0080)
> dev_desc->removable = 1;
> @@ -662,23 +633,19 @@ static void ide_ident(struct blk_desc *dev_desc)
> }
> #endif /* CONFIG_ATAPI */
>
> -#ifdef __BIG_ENDIAN
> - /* swap shorts */
> - dev_desc->lba = (iop.lba_capacity << 16) | (iop.lba_capacity >> 16);
> -#else /* ! __BIG_ENDIAN */
> - /*
> - * do not swap shorts on little endian
> - *
> - * See CF+ and CompactFlash Specification Revision 2.0:
> - * 6.2.1.6: Identfy Drive, Table 39, Word Address 57-58 for details.
> - */
> - dev_desc->lba = iop.lba_capacity;
> -#endif /* __BIG_ENDIAN */
> + iop.lba_capacity[0] = be16_to_cpu(iop.lba_capacity[0]);
> + iop.lba_capacity[1] = be16_to_cpu(iop.lba_capacity[1]);
> + dev_desc->lba =
> + ((unsigned long) iop.lba_capacity[0]) |
> + ((unsigned long) iop.lba_capacity[1] << 16);
>
> #ifdef CONFIG_LBA48
> if (iop.command_set_2 & 0x0400) { /* LBA 48 support */
> dev_desc->lba48 = 1;
> - dev_desc->lba = (unsigned long long) iop.lba48_capacity[0] |
> + for (int i = 0; i < 4; i++)
> + iop.lba48_capacity[i] = be16_to_cpu(iop.lba48_capacity[i]);
> + dev_desc->lba =
> + ((unsigned long long) iop.lba48_capacity[0] |
> ((unsigned long long) iop.lba48_capacity[1] << 16) |
> ((unsigned long long) iop.lba48_capacity[2] << 32) |
> ((unsigned long long) iop.lba48_capacity[3] << 48);
> @@ -846,90 +813,60 @@ void ide_init(void)
> #endif
> }
>
> -/* We only need to swap data if we are running on a big endian cpu. */
> -#if defined(__LITTLE_ENDIAN)
> __weak void ide_input_swap_data(int dev, ulong *sect_buf, int words)
> {
> - ide_input_data(dev, sect_buf, words);
> -}
> -#else
> -__weak void ide_input_swap_data(int dev, ulong *sect_buf, int words)
> -{
> - volatile ushort *pbuf =
> - (ushort *)(ATA_CURR_BASE(dev) + ATA_DATA_REG);
> + u64 paddr = (ATA_CURR_BASE(dev) + ATA_DATA_REG);
> ushort *dbuf = (ushort *)sect_buf;
>
> debug("in input swap data base for read is %lx\n",
> - (unsigned long) pbuf);
> + (unsigned long) paddr);
>
> while (words--) {
> -#ifdef __MIPS__
> - *dbuf++ = swab16p((u16 *)pbuf);
> - *dbuf++ = swab16p((u16 *)pbuf);
> -#else
> - *dbuf++ = ld_le16(pbuf);
> - *dbuf++ = ld_le16(pbuf);
> -#endif /* !MIPS */
> + EIEIO;
> + *dbuf++ = be16_to_cpu(inw(paddr));
> + EIEIO;
> + *dbuf++ = be16_to_cpu(inw(paddr));
> }
> }
> -#endif /* __LITTLE_ENDIAN */
> -
>
> -#if defined(CONFIG_IDE_SWAP_IO)
> __weak void ide_output_data(int dev, const ulong *sect_buf, int words)
> {
> +#if defined(CONFIG_IDE_AHB)
> + ide_write_data(dev, sect_buf, words);
> +#else
> ushort *dbuf;
> - volatile ushort *pbuf;
> + u64 paddr = (ATA_CURR_BASE(dev) + ATA_DATA_REG);
>
> - pbuf = (ushort *)(ATA_CURR_BASE(dev) + ATA_DATA_REG);
> dbuf = (ushort *)sect_buf;
> while (words--) {
> EIEIO;
> - *pbuf = *dbuf++;
> + outw(paddr, cpu_to_le16(*dbuf++));
> EIEIO;
> - *pbuf = *dbuf++;
> + outw(paddr, cpu_to_le16(*dbuf++));
> }
> +#endif /* CONFIG_IDE_AHB */
> }
> -#else /* ! CONFIG_IDE_SWAP_IO */
> -__weak void ide_output_data(int dev, const ulong *sect_buf, int words)
> -{
> -#if defined(CONFIG_IDE_AHB)
> - ide_write_data(dev, sect_buf, words);
> -#else
> - outsw(ATA_CURR_BASE(dev) + ATA_DATA_REG, sect_buf, words << 1);
> -#endif
> -}
> -#endif /* CONFIG_IDE_SWAP_IO */
>
> -#if defined(CONFIG_IDE_SWAP_IO)
> __weak void ide_input_data(int dev, ulong *sect_buf, int words)
> {
> +#if defined(CONFIG_IDE_AHB)
> + ide_read_data(dev, sect_buf, words);
> +#else
> ushort *dbuf;
> - volatile ushort *pbuf;
> + u64 paddr = (ATA_CURR_BASE(dev) + ATA_DATA_REG);
>
> - pbuf = (ushort *)(ATA_CURR_BASE(dev) + ATA_DATA_REG);
> dbuf = (ushort *)sect_buf;
>
> - debug("in input data base for read is %lx\n", (unsigned long) pbuf);
> + debug("in input data base for read is %lx\n", (unsigned long) paddr);
>
> while (words--) {
> EIEIO;
> - *dbuf++ = *pbuf;
> + *dbuf++ = le16_to_cpu(inw(paddr));
> EIEIO;
> - *dbuf++ = *pbuf;
> + *dbuf++ = le16_to_cpu(inw(paddr));
> }
> +#endif /* CONFIG_IDE_AHB */
> }
> -#else /* ! CONFIG_IDE_SWAP_IO */
> -__weak void ide_input_data(int dev, ulong *sect_buf, int words)
> -{
> -#if defined(CONFIG_IDE_AHB)
> - ide_read_data(dev, sect_buf, words);
> -#else
> - insw(ATA_CURR_BASE(dev) + ATA_DATA_REG, sect_buf, words << 1);
> -#endif
> -}
> -
> -#endif /* CONFIG_IDE_SWAP_IO */
>
> #ifdef CONFIG_BLK
> ulong ide_read(struct udevice *dev, lbaint_t blknr, lbaint_t blkcnt,
> @@ -1019,14 +956,14 @@ ulong ide_read(struct blk_desc *block_dev, lbaint_t blknr, lbaint_t blkcnt,
> if (lba48) {
> ide_outb(device, ATA_DEV_HD,
> ATA_LBA | ATA_DEVICE(device));
> - ide_outb(device, ATA_COMMAND, ATA_CMD_READ_EXT);
> + ide_outb(device, ATA_COMMAND, ATA_CMD_PIO_READ_EXT);
>
> } else
> #endif
> {
> ide_outb(device, ATA_DEV_HD, ATA_LBA |
> ATA_DEVICE(device) | ((blknr >> 24) & 0xF));
> - ide_outb(device, ATA_COMMAND, ATA_CMD_READ);
> + ide_outb(device, ATA_COMMAND, ATA_CMD_PIO_READ);
> }
>
> udelay(50);
> @@ -1116,14 +1053,14 @@ ulong ide_write(struct blk_desc *block_dev, lbaint_t blknr, lbaint_t blkcnt,
> if (lba48) {
> ide_outb(device, ATA_DEV_HD,
> ATA_LBA | ATA_DEVICE(device));
> - ide_outb(device, ATA_COMMAND, ATA_CMD_WRITE_EXT);
> + ide_outb(device, ATA_COMMAND, ATA_CMD_PIO_WRITE_EXT);
>
> } else
> #endif
> {
> ide_outb(device, ATA_DEV_HD, ATA_LBA |
> ATA_DEVICE(device) | ((blknr >> 24) & 0xF));
> - ide_outb(device, ATA_COMMAND, ATA_CMD_WRITE);
> + ide_outb(device, ATA_COMMAND, ATA_CMD_PIO_WRITE);
> }
>
> udelay(50);
> diff --git a/include/ata.h b/include/ata.h
> index 3d870c973f..7c95cfdced 100644
> --- a/include/ata.h
> +++ b/include/ata.h
> @@ -134,7 +134,8 @@ typedef struct hd_driveid {
> unsigned short cur_capacity1; /* (2 words, misaligned int) */
> unsigned char multsect; /* current multiple sector count */
> unsigned char multsect_valid; /* when (bit0==1) multsect is ok */
> - unsigned int lba_capacity; /* total number of sectors */
> + /*unsigned int lba_capacity; /--* total number of sectors */

Please, remove replaced code completely.

> + unsigned short lba_capacity[2]; /* 2 16bit values containing lba total number of sectors */

WARNING: line length of 101 exceeds 100 columns
#400: FILE: include/ata.h:138:
+ unsigned short lba_capacity[2]; /* 2 16bit values containing
lba total number of sectors */

Best regards

Heinrich

> unsigned short dma_1word; /* single-word dma info */
> unsigned short dma_mword; /* multiple-word dma info */
> unsigned short eide_pio_modes; /* bits 0:mode3 1:mode4 */
>