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;