[PATCH 0/2] Fix MIPS/Malta target and its IDE work

[PATCH 0/2] Fix MIPS/Malta target and its IDE work

[Reinoud Zandijk]

Patch 0001 re-enables FDT inclusion into the u-boot binary to make them boot
again. The code might not have adjusted well enough in the past to handle the
separate one.

Patch 0002 fixes IDE issues found on the Malta board:

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.

With the fixes, malta_defconfig and maltael_defconfig work again in Qemu.

Reinoud Zandijk (2):
  Re-embed the FDTs for the Malta targets.
  Fix IDE commands issued, fix endian issues, fix non MMIO

 configs/malta64_defconfig   |   1 +
 configs/malta64el_defconfig |   1 +
 configs/malta_defconfig     |   1 +
 configs/maltael_defconfig   |   1 +
 drivers/block/ide.c         | 143 ++++++++++--------------------------
 include/ata.h               |   3 +-
 6 files changed, 46 insertions(+), 104 deletions(-)

Signed-off-by: Reinoud Zandijk <reinoud@NetBSD.org>

-- 
2.29.2

[PATCH 1/2] Re-embed the FDTs for the Malta targets.

[Reinoud Zandijk]

---
 configs/malta64_defconfig   | 1 +
 configs/malta64el_defconfig | 1 +
 configs/malta_defconfig     | 1 +
 configs/maltael_defconfig   | 1 +
 4 files changed, 4 insertions(+)

diff --git a/configs/malta64_defconfig b/configs/malta64_defconfig
index 878dc6ee05..106c890faa 100644
--- a/configs/malta64_defconfig
+++ b/configs/malta64_defconfig
@@ -21,6 +21,7 @@ CONFIG_CMD_DHCP=y
 CONFIG_CMD_PING=y
 CONFIG_CMD_DATE=y
 # CONFIG_ISO_PARTITION is not set
+CONFIG_OF_EMBED=y
 CONFIG_ENV_IS_IN_FLASH=y
 CONFIG_ENV_ADDR=0xFFFFFFFFBE3E0000
 CONFIG_MTD_NOR_FLASH=y
diff --git a/configs/malta64el_defconfig b/configs/malta64el_defconfig
index 7dfe67355f..e61d61b26b 100644
--- a/configs/malta64el_defconfig
+++ b/configs/malta64el_defconfig
@@ -23,6 +23,7 @@ CONFIG_CMD_DHCP=y
 CONFIG_CMD_PING=y
 CONFIG_CMD_DATE=y
 # CONFIG_ISO_PARTITION is not set
+CONFIG_OF_EMBED=y
 CONFIG_ENV_IS_IN_FLASH=y
 CONFIG_ENV_ADDR=0xFFFFFFFFBE3E0000
 CONFIG_MTD_NOR_FLASH=y
diff --git a/configs/malta_defconfig b/configs/malta_defconfig
index 304f2198ba..c1dfa357d2 100644
--- a/configs/malta_defconfig
+++ b/configs/malta_defconfig
@@ -20,6 +20,7 @@ CONFIG_CMD_DHCP=y
 CONFIG_CMD_PING=y
 CONFIG_CMD_DATE=y
 # CONFIG_ISO_PARTITION is not set
+CONFIG_OF_EMBED=y
 CONFIG_ENV_IS_IN_FLASH=y
 CONFIG_ENV_ADDR=0xBE3E0000
 CONFIG_MTD_NOR_FLASH=y
diff --git a/configs/maltael_defconfig b/configs/maltael_defconfig
index 7436e4e943..0a8badcb84 100644
--- a/configs/maltael_defconfig
+++ b/configs/maltael_defconfig
@@ -22,6 +22,7 @@ CONFIG_CMD_DHCP=y
 CONFIG_CMD_PING=y
 CONFIG_CMD_DATE=y
 # CONFIG_ISO_PARTITION is not set
+CONFIG_OF_EMBED=y
 CONFIG_ENV_IS_IN_FLASH=y
 CONFIG_ENV_ADDR=0xBE3E0000
 CONFIG_MTD_NOR_FLASH=y
-- 
2.29.2

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

[Reinoud Zandijk]

---
 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
+++ b/drivers/block/ide.c
@@ -130,56 +130,40 @@ 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)
 {
 	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 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 */
+	unsigned short  lba_capacity[2]; /* 2 16bit values containing lba 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 */
-- 
2.29.2

[PATCH 0/2] Fix MIPS/Malta target and its IDE work

[Daniel Schwierzeck]

Am Montag, den 22.02.2021, 18:05 +0100 schrieb Reinoud Zandijk:
> Patch 0001 re-enables FDT inclusion into the u-boot binary to make
> them boot
> again. The code might not have adjusted well enough in the past to
> handle the
> separate one.

what exactly is the issue? Do you see it just on real hardware?

Booting all Malta variants in Qemu is contained in official U-Boot CI
and showed no boot failures until now. Unfortuneately I don't have
Malta hardware myself for testing.

Also CONFIG_OF_EMBED is just a debug option and should be avoided for
production builds. So if there is a real problem, I would prefer to
rather fix that instead of enabling CONFIG_OF_EMBED.

> 
> Patch 0002 fixes IDE issues found on the Malta board:
> 
> 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.
> 
> With the fixes, malta_defconfig and maltael_defconfig work again in
> Qemu.
> 
> Reinoud Zandijk (2):
>   Re-embed the FDTs for the Malta targets.
>   Fix IDE commands issued, fix endian issues, fix non MMIO
> 
>  configs/malta64_defconfig   |   1 +
>  configs/malta64el_defconfig |   1 +
>  configs/malta_defconfig     |   1 +
>  configs/maltael_defconfig   |   1 +
>  drivers/block/ide.c         | 143 ++++++++++----------------------
> ----
>  include/ata.h               |   3 +-
>  6 files changed, 46 insertions(+), 104 deletions(-)
> 
> Signed-off-by: Reinoud Zandijk <reinoud@NetBSD.org>
> 
-- 
- Daniel

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

[Heinrich Schuchardt]

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

[PATCH 1/2] Re-embed the FDTs for the Malta targets.

[Heinrich Schuchardt]

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

Please, provide a commit message and a Signed-by line.

> ---
>   configs/malta64_defconfig   | 1 +
>   configs/malta64el_defconfig | 1 +
>   configs/malta_defconfig     | 1 +
>   configs/maltael_defconfig   | 1 +
>   4 files changed, 4 insertions(+)
>
> diff --git a/configs/malta64_defconfig b/configs/malta64_defconfig
> index 878dc6ee05..106c890faa 100644
> --- a/configs/malta64_defconfig
> +++ b/configs/malta64_defconfig
> @@ -21,6 +21,7 @@ CONFIG_CMD_DHCP=y
>   CONFIG_CMD_PING=y
>   CONFIG_CMD_DATE=y
>   # CONFIG_ISO_PARTITION is not set
> +CONFIG_OF_EMBED=y
>   CONFIG_ENV_IS_IN_FLASH=y
>   CONFIG_ENV_ADDR=0xFFFFFFFFBE3E0000
>   CONFIG_MTD_NOR_FLASH=y
> diff --git a/configs/malta64el_defconfig b/configs/malta64el_defconfig
> index 7dfe67355f..e61d61b26b 100644
> --- a/configs/malta64el_defconfig
> +++ b/configs/malta64el_defconfig
> @@ -23,6 +23,7 @@ CONFIG_CMD_DHCP=y
>   CONFIG_CMD_PING=y
>   CONFIG_CMD_DATE=y
>   # CONFIG_ISO_PARTITION is not set
> +CONFIG_OF_EMBED=y
>   CONFIG_ENV_IS_IN_FLASH=y
>   CONFIG_ENV_ADDR=0xFFFFFFFFBE3E0000
>   CONFIG_MTD_NOR_FLASH=y
> diff --git a/configs/malta_defconfig b/configs/malta_defconfig
> index 304f2198ba..c1dfa357d2 100644
> --- a/configs/malta_defconfig
> +++ b/configs/malta_defconfig
> @@ -20,6 +20,7 @@ CONFIG_CMD_DHCP=y
>   CONFIG_CMD_PING=y
>   CONFIG_CMD_DATE=y
>   # CONFIG_ISO_PARTITION is not set
> +CONFIG_OF_EMBED=y

This should not be used in a defconfig.

dts/Kconfig:83:

"Boards in the mainline U-Boot tree should not use it."

Best regards

Heinrich

>   CONFIG_ENV_IS_IN_FLASH=y
>   CONFIG_ENV_ADDR=0xBE3E0000
>   CONFIG_MTD_NOR_FLASH=y
> diff --git a/configs/maltael_defconfig b/configs/maltael_defconfig
> index 7436e4e943..0a8badcb84 100644
> --- a/configs/maltael_defconfig
> +++ b/configs/maltael_defconfig
> @@ -22,6 +22,7 @@ CONFIG_CMD_DHCP=y
>   CONFIG_CMD_PING=y
>   CONFIG_CMD_DATE=y
>   # CONFIG_ISO_PARTITION is not set
> +CONFIG_OF_EMBED=y
>   CONFIG_ENV_IS_IN_FLASH=y
>   CONFIG_ENV_ADDR=0xBE3E0000
>   CONFIG_MTD_NOR_FLASH=y
>

[PATCH 0/2] Fix MIPS/Malta target and its IDE work

[Reinoud Zandijk]

Hi Daniel,

On Mon, Feb 22, 2021 at 07:23:26PM +0100, Daniel Schwierzeck wrote:
> Am Montag, den 22.02.2021, 18:05 +0100 schrieb Reinoud Zandijk:
> > Patch 0001 re-enables FDT inclusion into the u-boot binary to make
> > them boot
> > again. The code might not have adjusted well enough in the past to
> > handle the
> > separate one.
> 
> what exactly is the issue? Do you see it just on real hardware?
> 
> Booting all Malta variants in Qemu is contained in official U-Boot CI
> and showed no boot failures until now. Unfortuneately I don't have
> Malta hardware myself for testing.

If I remove it, the machine just spins in Qemu, no output, nothing. If I add
it, it works fine again. I found out by bisecting. I have no idea why the
tests aren't picking this up. Could it be a qemu/gcc/binutils combination
issue? A symbol not set as expected?

qemu 5.1.0
gcc 8.3.0
binutils 2.32

> Also CONFIG_OF_EMBED is just a debug option and should be avoided for
> production builds. So if there is a real problem, I would prefer to
> rather fix that instead of enabling CONFIG_OF_EMBED.

Thats what I already stated; Its a provisionary fix until the real issue is
tacked. Its now at least known.

With regards,
Reinoud

[PATCH 0/2] Fix MIPS/Malta target and its IDE work

[Daniel Schwierzeck]

Am Montag, den 22.02.2021, 20:56 +0100 schrieb Reinoud Zandijk:
> Hi Daniel,
> 
> On Mon, Feb 22, 2021 at 07:23:26PM +0100, Daniel Schwierzeck wrote:
> > Am Montag, den 22.02.2021, 18:05 +0100 schrieb Reinoud Zandijk:
> > > Patch 0001 re-enables FDT inclusion into the u-boot binary to
> > > make
> > > them boot
> > > again. The code might not have adjusted well enough in the past
> > > to
> > > handle the
> > > separate one.
> > 
> > what exactly is the issue? Do you see it just on real hardware?
> > 
> > Booting all Malta variants in Qemu is contained in official U-Boot
> > CI
> > and showed no boot failures until now. Unfortuneately I don't have
> > Malta hardware myself for testing.
> 
> If I remove it, the machine just spins in Qemu, no output, nothing.
> If I add
> it, it works fine again. I found out by bisecting. I have no idea why
> the
> tests aren't picking this up. Could it be a qemu/gcc/binutils
> combination
> issue? A symbol not set as expected?
> 
> qemu 5.1.0
> gcc 8.3.0
> binutils 2.32
> 

which board config did you try exactly? malta or maltael?

For maltael and malta64el an extra U-Boot binary named u-boot-swap.bin
is generated which is booted with Qemu. See also [1] respectively all
conf.malta*_qemu configs for working Qemu configurations for all Malta
variants.

The reason for that extra image is that the original MIPS YAMON loader
builds a combined EB/EL image and links it to EB. With some assembly
magic the start code determines immediately after the reset vector
which endianess the CPU is running and branches to the according EB or
LE image. This is replicated in the Qemu Malta machine implementation.
The Malta EL machine code excepts a bootloader in EB format and swaps
the code back to EL before executing it. To workaround this, we need to
feed this u-boot-swap.bin to Qemu.

[1] 
https://gitlab.denx.de/u-boot/u-boot-test-hooks/-blob/master/bin/travis-ci/conf.maltael_qemu

-- 
- Daniel

[PATCH 0/2] Fix MIPS/Malta target and its IDE work

[Reinoud Zandijk]

Hi Daniel,

On Tue, Feb 23, 2021 at 01:03:05AM +0100, Daniel Schwierzeck wrote:
> Am Montag, den 22.02.2021, 20:56 +0100 schrieb Reinoud Zandijk:
> > If I remove it, the machine just spins in Qemu, no output, nothing.
> > If I add
> > it, it works fine again. I found out by bisecting. I have no idea why
> > the
> > tests aren't picking this up. Could it be a qemu/gcc/binutils
> > combination
> > issue? A symbol not set as expected?
> > 
> > qemu 5.1.0
> > gcc 8.3.0
> > binutils 2.32
> > 
> 
> which board config did you try exactly? malta or maltael?

Both malta and maltael have the same behaviour. And yeah, for maltael i needed
the u-boot-swap.bin indeed! That was not that obvious at first but trial and
error showed it.

How are the tests performed? Are the actual u-boot images passed as compiled
with `-bios' to qemu and that alone? Or are the tests also sneaking in the FDT
to qemu? By f.e. appending them? Could the CONFIG_SYS_MALLOC_CLEAR_ON_INIT
warning/error prevented a good build? I also get that with edminib2_defconfig
so I assumed some work is still in progress on that.

> [1] 
> https://gitlab.denx.de/u-boot/u-boot-test-hooks/-blob/master/bin/travis-ci/conf.maltael_qemu

This is behind a login?

Reinoud

[PATCH 0/2] Fix MIPS/Malta target and its IDE work

[Tom Rini]

On Tue, Feb 23, 2021 at 03:19:06PM +0100, Reinoud Zandijk wrote:
> Hi Daniel,
> 
> On Tue, Feb 23, 2021 at 01:03:05AM +0100, Daniel Schwierzeck wrote:
> > Am Montag, den 22.02.2021, 20:56 +0100 schrieb Reinoud Zandijk:
> > > If I remove it, the machine just spins in Qemu, no output, nothing.
> > > If I add
> > > it, it works fine again. I found out by bisecting. I have no idea why
> > > the
> > > tests aren't picking this up. Could it be a qemu/gcc/binutils
> > > combination
> > > issue? A symbol not set as expected?
> > > 
> > > qemu 5.1.0
> > > gcc 8.3.0
> > > binutils 2.32
> > > 
> > 
> > which board config did you try exactly? malta or maltael?
> 
> Both malta and maltael have the same behaviour. And yeah, for maltael i needed
> the u-boot-swap.bin indeed! That was not that obvious at first but trial and
> error showed it.
> 
> How are the tests performed? Are the actual u-boot images passed as compiled
> with `-bios' to qemu and that alone? Or are the tests also sneaking in the FDT
> to qemu? By f.e. appending them? Could the CONFIG_SYS_MALLOC_CLEAR_ON_INIT
> warning/error prevented a good build? I also get that with edminib2_defconfig
> so I assumed some work is still in progress on that.
> 
> > [1] 
> > https://gitlab.denx.de/u-boot/u-boot-test-hooks/-blob/master/bin/travis-ci/conf.maltael_qemu
> 
> This is behind a login?

Unintentionally so, yes.  Fixing this now.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210223/9d11ca26/attachment.sig>

[PATCH 0/2] Fix MIPS/Malta target and its IDE work

[Tom Rini]

On Tue, Feb 23, 2021 at 09:26:56AM -0500, Tom Rini wrote:
> On Tue, Feb 23, 2021 at 03:19:06PM +0100, Reinoud Zandijk wrote:
> > Hi Daniel,
> > 
> > On Tue, Feb 23, 2021 at 01:03:05AM +0100, Daniel Schwierzeck wrote:
> > > Am Montag, den 22.02.2021, 20:56 +0100 schrieb Reinoud Zandijk:
> > > > If I remove it, the machine just spins in Qemu, no output, nothing.
> > > > If I add
> > > > it, it works fine again. I found out by bisecting. I have no idea why
> > > > the
> > > > tests aren't picking this up. Could it be a qemu/gcc/binutils
> > > > combination
> > > > issue? A symbol not set as expected?
> > > > 
> > > > qemu 5.1.0
> > > > gcc 8.3.0
> > > > binutils 2.32
> > > > 
> > > 
> > > which board config did you try exactly? malta or maltael?
> > 
> > Both malta and maltael have the same behaviour. And yeah, for maltael i needed
> > the u-boot-swap.bin indeed! That was not that obvious at first but trial and
> > error showed it.
> > 
> > How are the tests performed? Are the actual u-boot images passed as compiled
> > with `-bios' to qemu and that alone? Or are the tests also sneaking in the FDT
> > to qemu? By f.e. appending them? Could the CONFIG_SYS_MALLOC_CLEAR_ON_INIT
> > warning/error prevented a good build? I also get that with edminib2_defconfig
> > so I assumed some work is still in progress on that.
> > 
> > > [1] 
> > > https://gitlab.denx.de/u-boot/u-boot-test-hooks/-blob/master/bin/travis-ci/conf.maltael_qemu
> > 
> > This is behind a login?
> 
> Unintentionally so, yes.  Fixing this now.

Ah!  A typo that gets fixed up automatically if logged in, but not
otherwise:
https://gitlab.denx.de/u-boot/u-boot-test-hooks/-/blob/master/bin/travis-ci/conf.maltael_qemu
is right (note the '/' after '-' before 'blob').

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210223/c5510d2e/attachment.sig>

[PATCH 0/2] Fix MIPS/Malta target and its IDE work

[Daniel Schwierzeck]

Am Dienstag, den 23.02.2021, 15:19 +0100 schrieb Reinoud Zandijk:
> Hi Daniel,
> 
> On Tue, Feb 23, 2021 at 01:03:05AM +0100, Daniel Schwierzeck wrote:
> > Am Montag, den 22.02.2021, 20:56 +0100 schrieb Reinoud Zandijk:
> > > If I remove it, the machine just spins in Qemu, no output,
> > > nothing.
> > > If I add
> > > it, it works fine again. I found out by bisecting. I have no idea
> > > why
> > > the
> > > tests aren't picking this up. Could it be a qemu/gcc/binutils
> > > combination
> > > issue? A symbol not set as expected?
> > > 
> > > qemu 5.1.0
> > > gcc 8.3.0
> > > binutils 2.32
> > > 
> > 
> > which board config did you try exactly? malta or maltael?
> 
> Both malta and maltael have the same behaviour. And yeah, for maltael
> i needed
> the u-boot-swap.bin indeed! That was not that obvious at first but
> trial and
> error showed it.
> 
> How are the tests performed? Are the actual u-boot images passed as
> compiled
> with `-bios' to qemu and that alone? Or are the tests also sneaking
> in the FDT
> to qemu? By f.e. appending them? 

we use the images as built by the default configs. We don't use "-bios" 
but "-drive if=pflash,file=${U_BOOT_BUILD_DIR}/flash.img,format=raw" to
emulate NOR flash and to be closer to hardware.

flash.img is created with that script:
https://gitlab.denx.de/u-boot/u-boot-test-hooks/-/blob/master/bin/flash.qemu_gen_padded_image

-- 
- Daniel

[PATCH 0/2] Fix MIPS/Malta target and its IDE work

[Heinrich Schuchardt]

On 2/23/21 7:06 PM, Daniel Schwierzeck wrote:
> Am Dienstag, den 23.02.2021, 15:19 +0100 schrieb Reinoud Zandijk:
>> Hi Daniel,
>>
>> On Tue, Feb 23, 2021 at 01:03:05AM +0100, Daniel Schwierzeck wrote:
>>> Am Montag, den 22.02.2021, 20:56 +0100 schrieb Reinoud Zandijk:
>>>> If I remove it, the machine just spins in Qemu, no output,
>>>> nothing.
>>>> If I add
>>>> it, it works fine again. I found out by bisecting. I have no idea
>>>> why
>>>> the
>>>> tests aren't picking this up. Could it be a qemu/gcc/binutils
>>>> combination
>>>> issue? A symbol not set as expected?
>>>>
>>>> qemu 5.1.0
>>>> gcc 8.3.0
>>>> binutils 2.32
>>>>
>>>
>>> which board config did you try exactly? malta or maltael?
>>
>> Both malta and maltael have the same behaviour. And yeah, for maltael
>> i needed
>> the u-boot-swap.bin indeed! That was not that obvious at first but
>> trial and
>> error showed it.
>>
>> How are the tests performed? Are the actual u-boot images passed as
>> compiled
>> with `-bios' to qemu and that alone? Or are the tests also sneaking
>> in the FDT
>> to qemu? By f.e. appending them?
>
> we use the images as built by the default configs. We don't use "-bios"
> but "-drive if=pflash,file=${U_BOOT_BUILD_DIR}/flash.img,format=raw" to
> emulate NOR flash and to be closer to hardware.
>
> flash.img is created with that script:
> https://gitlab.denx.de/u-boot/u-boot-test-hooks/-/blob/master/bin/flash.qemu_gen_padded_image
>

Hello Daniel,

thanks for pointing out how the Gitlab CI test is run.

This looks a bit different to doc/board/emulation/qemu-mips.rst where
'-pflash flash' is used as argument.

Do you want to update the man-page to use -drive instead of -pflash?

Best regards

Heinrich