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

Heinrich Schuchardt xypron.glpk at gmx.de
Wed Feb 24 18:57:08 CET 2021


On 24.02.21 17:44, Reinoud Zandijk wrote:
>
> Fixes IDE issues found on the Malta board under Qemu:
>
> 1) DMA implied commands were sent to the controller in stead of the PIO
> variants. The rest of the code is DMA free and written for PIO operation.
>
> 2) direct pointer access was used to read and write the registers instead
> of the inb/inw/outb/outw functions/macros. Registers don't have to be
> memory mapped and ATA_CURR_BASE() does not have to return an offset from
> address zero.
>
> 3) Endian isues in ide_ident() and reading/writing data in general. Names
> were corrupted and sizes misreported.

It is preferable to have each issue fixed in a separate patch.

>
> Tested malta_defconfig and maltael_defconfig to work again in Qemu.

What about the other architectures which can use the driver?

@Simon:
Can we get rid of U_BOOT_LEGACY_BLK(ide)?

Best regards

Heinrich

>
>
> Signed-off-by: Reinoud Zandijk <reinoud at NetBSD.org>
>
> ---
>  drivers/block/ide.c | 152 +++++++++++++-------------------------------
>  include/ata.h       |   2 +-
>  2 files changed, 44 insertions(+), 110 deletions(-)
>
> diff --git a/drivers/block/ide.c b/drivers/block/ide.c
> index 43a0776099..862a85bc87 100644
> --- a/drivers/block/ide.c
> +++ b/drivers/block/ide.c
> @@ -130,56 +130,38 @@ OUT:
>   * ATAPI Support
>   */
>
> -#if defined(CONFIG_IDE_SWAP_IO)
>  /* 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)
>  {
> +	uintptr_t paddr = (ATA_CURR_BASE(dev) + ATA_DATA_REG);
>  	ushort *dbuf;
> -	volatile ushort *pbuf;
>
> -	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);
> +	debug("in output data shorts base for read is %p\n", (void *)paddr);
>
>  	while (shorts--) {
>  		EIEIO;
> -		*pbuf = *dbuf++;
> +		outw(cpu_to_le16(*dbuf++), paddr);
>  	}
>  }
>
>  __weak void ide_input_data_shorts(int dev, ushort *sect_buf, int shorts)
>  {
> +	uintptr_t paddr = (ATA_CURR_BASE(dev) + ATA_DATA_REG);
>  	ushort *dbuf;
> -	volatile ushort *pbuf;
>
> -	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);
> +	debug("in input data shorts base for read is %p\n", (void *)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 +618,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,26 +631,22 @@ 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] |
> -			((unsigned long long) iop.lba48_capacity[1] << 16) |
> -			((unsigned long long) iop.lba48_capacity[2] << 32) |
> -			((unsigned long long) iop.lba48_capacity[3] << 48);
> +		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));
>  	} else {
>  		dev_desc->lba48 = 0;
>  	}
> @@ -846,90 +811,59 @@ 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);
> +	uintptr_t 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);
> +	debug("in input swap data base for read is %p\n", (void *)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
> +	uintptr_t paddr = (ATA_CURR_BASE(dev) + ATA_DATA_REG);
>  	ushort *dbuf;
> -	volatile ushort *pbuf;
>
> -	pbuf = (ushort *)(ATA_CURR_BASE(dev) + ATA_DATA_REG);
>  	dbuf = (ushort *)sect_buf;
>  	while (words--) {
>  		EIEIO;
> -		*pbuf = *dbuf++;
> +		outw(cpu_to_le16(*dbuf++), paddr);
>  		EIEIO;
> -		*pbuf = *dbuf++;
> +		outw(cpu_to_le16(*dbuf++), paddr);
>  	}
> +#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
> +	uintptr_t paddr = (ATA_CURR_BASE(dev) + ATA_DATA_REG);
>  	ushort *dbuf;
> -	volatile ushort *pbuf;
>
> -	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 %p\n", (void *)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 +953,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 +1050,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..32ad5f6427 100644
> --- a/include/ata.h
> +++ b/include/ata.h
> @@ -134,7 +134,7 @@ 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 short  lba_capacity[2];/* two words containing total number of sectors */
>  	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 */
>



More information about the U-Boot mailing list