[U-Boot] inappropriate PCI configuration on arm64 qemu?

[AKASHI Takahiro <takahiro.akashi@linaro.org>]

Simon,

On Wed, May 30, 2018 at 01:18:30PM -0600, Simon Glass wrote:
> +Tuomas
> 
> Hi Akashi,
> 
> On 28 May 2018 at 01:59, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote:
> > When I tried to add a SD card to qemu's virt machine (2.10.0) as,
> >         ------
> >         -device sdhci-pci \
> >         -device sd-card,drive=my_sd \
> >         -drive if=none,id=my_sd,format=raw,file=/path/my/sd.img
> >         ------
> > u-boot doesn't configure a SDHCI controller properly and an attached
> > device is never detected.
> >
> > Digging into the code, I found
> > * reading BAR5 in dm_pciauto_setup_device() shows BAR5 is a 32-bit address,
> > * pciauto_region_allocate() allocates a 64-bit address (0x80.ABCD.0000)
> >   to BAR5 as res->bus_lower is 0x80.0000.0000
> > * Upper 32-bit value is not written back to BAR5 because of !found_mem64
> >   (BAR5 is the last one and no succeeding BAR anyway.)
> >
> > On the other hand,
> > * Qemu defines two PCI memory regions for MMIO:
> >         (from qemu's hw/arm/virt.c)
> >         ------
> >         [VIRT_PCIE_MMIO] =          { 0x10000000, 0x2eff0000 },
> >         [VIRT_PCIE_PIO] =           { 0x3eff0000, 0x00010000 },
> >         [VIRT_PCIE_ECAM] =          { 0x3f000000, 0x01000000 },
> >         [VIRT_MEM] =                { 0x40000000, RAMLIMIT_BYTES },
> >         /* Second PCIe window, 512GB wide at the 512GB boundary */
> >         [VIRT_PCIE_MMIO_HIGH] =   { 0x8000000000ULL, 0x8000000000ULL },
> >         ------
> > * A PCI card is configured in decode_regions() so that
> >   'hose' has only one entry per each type of memory regions.
> >   This behavior was introduced by Simon's patch:
> >         ------
> >         commit 9526d83ac5a
> >         Author: Simon Glass <sjg@chromium.org>
> >         Date:   Thu Nov 19 20:26:58 2015 -0700
> >
> >             dm: pci: Support decoding ranges with duplicate entries
> >         ------
> > * As a result, MMIO region (0x1000.0000-0x2eff.0000) is overwritten
> >   and MMIO_HIGH is the only one available at runtime.
> >
> > I believe that this behavior is the root cause of my issue, and
> > by reverting the patch mentioned above, everything works fine.
> >
> > While I understand a concern mentioned in the commit message,
> > there should be a better way to manage the case.
> 
> There was a series that changed things in this area. Can you take a look?
> 
>    PCI: dm: Ignore 64-bit memory regions if CONFIG_SYS_PCI_64BIT not set

Ah, I didn't know that, but it seems to me that it is still insufficient.
This hack won't work on 32-bit PCI card. I found another patch from Tuomas:
---
        commit d71975ae6e0
        Author: Tuomas Tynkkynen <tuomas.tynkkynen@iki.fi>
        Date:   Mon May 14 19:38:13 2018 +0300

            PCI: autoconfig: Don't allocate 64-bit addresses to 32-bit only
            resources
---

This approach looks too conservative if 32-bit window is also available,
in addition to 64-bit space, as in the case of qemu-arm.

I'd like to propose supporting at least two type of PCI memory regions,
low mem (normal case) and high mem.
Attached is my experimental implementation for this although I might have
made any mistake as I'm not very much familiar with PCI specification.

(When you try to apply this, please revert the commit above.)

Thanks,
-Takahiro AKASHI

> Regards,
> Simon
---8<---
diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c
index 1cd1e409e3e..40282c616a4 100644
--- a/drivers/pci/pci-uclass.c
+++ b/drivers/pci/pci-uclass.c
@@ -867,13 +867,7 @@ static int decode_regions(struct pci_controller *hose, ofnode parent_node,
 			continue;
 		}
 
-		pos = -1;
-		for (i = 0; i < hose->region_count; i++) {
-			if (hose->regions[i].flags == type)
-				pos = i;
-		}
-		if (pos == -1)
-			pos = hose->region_count++;
+		pos = hose->region_count++;
 		debug(" - type=%d, pos=%d\n", type, pos);
 		pci_set_region(hose->regions + pos, pci_addr, addr, size, type);
 	}
diff --git a/drivers/pci/pci_auto.c b/drivers/pci/pci_auto.c
index d1feb503a0a..01423879552 100644
--- a/drivers/pci/pci_auto.c
+++ b/drivers/pci/pci_auto.c
@@ -19,6 +19,7 @@
 
 void dm_pciauto_setup_device(struct udevice *dev, int bars_num,
 			     struct pci_region *mem,
+			     struct pci_region *himem,
 			     struct pci_region *prefetch, struct pci_region *io,
 			     bool enum_only)
 {
@@ -29,7 +30,7 @@ void dm_pciauto_setup_device(struct udevice *dev, int bars_num,
 	u8 header_type;
 	int rom_addr;
 	pci_addr_t bar_value;
-	struct pci_region *bar_res = NULL;
+	struct pci_region *bar_res = NULL, *bar_res32 = NULL;
 	int found_mem64 = 0;
 	u16 class;
 
@@ -88,7 +89,12 @@ void dm_pciauto_setup_device(struct udevice *dev, int bars_num,
 					    PCI_BASE_ADDRESS_MEM_PREFETCH)) {
 					bar_res = prefetch;
 				} else {
-					bar_res = mem;
+					if (found_mem64) {
+						bar_res = himem;
+						bar_res32 = mem;
+					} else {
+						bar_res = mem;
+					}
 				}
 			}
 
@@ -97,8 +103,14 @@ void dm_pciauto_setup_device(struct udevice *dev, int bars_num,
 			      (unsigned long long)bar_size);
 		}
 
-		if (!enum_only && pciauto_region_allocate(bar_res, bar_size,
-							  &bar_value) == 0) {
+		if (!enum_only) {
+			if (pciauto_region_allocate(bar_res, bar_size,
+							&bar_value) &&
+			    bar_res32 && (bar_size < (0x1ULL << 32)) &&
+			    pciauto_region_allocate(bar_res32, bar_size,
+							  &bar_value))
+				goto skip_write;
+
 			/* Write it out and update our limit */
 			dm_pci_write_config32(dev, bar, (u32)bar_value);
 
@@ -116,6 +128,8 @@ void dm_pciauto_setup_device(struct udevice *dev, int bars_num,
 				dm_pci_write_config32(dev, bar, 0x00000000);
 #endif
 			}
+skip_write:
+			;
 		}
 
 		cmdstat |= (bar_response & PCI_BASE_ADDRESS_SPACE) ?
@@ -305,6 +319,7 @@ void dm_pciauto_postscan_setup_bridge(struct udevice *dev, int sub_bus)
 int dm_pciauto_config_device(struct udevice *dev)
 {
 	struct pci_region *pci_mem;
+	struct pci_region *pci_himem;
 	struct pci_region *pci_prefetch;
 	struct pci_region *pci_io;
 	unsigned int sub_bus = PCI_BUS(dm_pci_get_bdf(dev));
@@ -319,6 +334,7 @@ int dm_pciauto_config_device(struct udevice *dev)
 #endif
 
 	pci_mem = ctlr_hose->pci_mem;
+	pci_himem = ctlr_hose->pci_himem;
 	pci_prefetch = ctlr_hose->pci_prefetch;
 	pci_io = ctlr_hose->pci_io;
 
@@ -329,8 +345,8 @@ int dm_pciauto_config_device(struct udevice *dev)
 		debug("PCI Autoconfig: Found P2P bridge, device %d\n",
 		      PCI_DEV(dm_pci_get_bdf(dev)));
 
-		dm_pciauto_setup_device(dev, 2, pci_mem, pci_prefetch, pci_io,
-					enum_only);
+		dm_pciauto_setup_device(dev, 2, pci_mem, pci_himem,
+					pci_prefetch, pci_io, enum_only);
 
 		n = dm_pci_hose_probe_bus(dev);
 		if (n < 0)
@@ -343,8 +359,8 @@ int dm_pciauto_config_device(struct udevice *dev)
 		 * just do a minimal setup of the bridge,
 		 * let the OS take care of the rest
 		 */
-		dm_pciauto_setup_device(dev, 0, pci_mem, pci_prefetch, pci_io,
-					enum_only);
+		dm_pciauto_setup_device(dev, 0, pci_mem, pci_himem,
+					pci_prefetch, pci_io, enum_only);
 
 		debug("PCI Autoconfig: Found P2CardBus bridge, device %d\n",
 		      PCI_DEV(dm_pci_get_bdf(dev)));
@@ -366,7 +382,7 @@ int dm_pciauto_config_device(struct udevice *dev)
 		 * the PIMMR window to be allocated (BAR0 - 1MB size)
 		 */
 		debug("PCI Autoconfig: Broken bridge found, only minimal config\n");
-		dm_pciauto_setup_device(dev, 0, hose->pci_mem,
+		dm_pciauto_setup_device(dev, 0, hose->pci_mem, hose->pci_himem,
 					hose->pci_prefetch, hose->pci_io,
 					enum_only);
 		break;
@@ -377,8 +393,8 @@ int dm_pciauto_config_device(struct udevice *dev)
 		/* fall through */
 
 	default:
-		dm_pciauto_setup_device(dev, 6, pci_mem, pci_prefetch, pci_io,
-					enum_only);
+		dm_pciauto_setup_device(dev, 6, pci_mem, pci_himem,
+					pci_prefetch, pci_io, enum_only);
 		break;
 	}
 
diff --git a/drivers/pci/pci_auto_common.c b/drivers/pci/pci_auto_common.c
index d90dbcf91a2..e96b1cb4d12 100644
--- a/drivers/pci/pci_auto_common.c
+++ b/drivers/pci/pci_auto_common.c
@@ -78,6 +78,7 @@ void pciauto_config_init(struct pci_controller *hose)
 
 	hose->pci_io = NULL;
 	hose->pci_mem = NULL;
+	hose->pci_himem = NULL;
 	hose->pci_prefetch = NULL;
 
 	for (i = 0; i < hose->region_count; i++) {
@@ -88,6 +89,13 @@ void pciauto_config_init(struct pci_controller *hose)
 				hose->pci_io = hose->regions + i;
 			break;
 		case PCI_REGION_MEM:
+			if (hose->regions[i].bus_start > ((0x1ULL << 32) - 1)) {
+				if (!hose->pci_himem ||
+				    hose->pci_himem->size
+						< hose->regions[i].size)
+					hose->pci_himem = hose->regions + i;
+				break;
+			}
 			if (!hose->pci_mem ||
 			    hose->pci_mem->size < hose->regions[i].size)
 				hose->pci_mem = hose->regions + i;
@@ -103,6 +111,8 @@ void pciauto_config_init(struct pci_controller *hose)
 
 	if (hose->pci_mem)
 		pciauto_show_region("Memory", hose->pci_mem);
+	if (hose->pci_himem)
+		pciauto_show_region("High Memory", hose->pci_himem);
 	if (hose->pci_prefetch)
 		pciauto_show_region("Prefetchable Mem", hose->pci_prefetch);
 	if (hose->pci_io)
diff --git a/include/pci.h b/include/pci.h
index b9324405ee2..5fa7bad8798 100644
--- a/include/pci.h
+++ b/include/pci.h
@@ -582,7 +582,7 @@ struct pci_controller {
 #endif
 
 	/* Used by auto config */
-	struct pci_region *pci_mem, *pci_io, *pci_prefetch;
+	struct pci_region *pci_mem, *pci_himem, *pci_io, *pci_prefetch;
 
 #ifndef CONFIG_DM_PCI
 	int current_busno;