[U-Boot] inappropriate PCI configuration on arm64 qemu?
AKASHI Takahiro
takahiro.akashi at linaro.org
Thu May 31 05:05:49 UTC 2018
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 at 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 at 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 at 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;
More information about the U-Boot
mailing list