* [U-Boot] inappropriate PCI configuration on arm64 qemu? @ 2018-05-28 7:59 AKASHI Takahiro 2018-05-30 19:18 ` Simon Glass 0 siblings, 1 reply; 6+ messages in thread From: AKASHI Takahiro @ 2018-05-28 7:59 UTC (permalink / raw) To: u-boot 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. Any ideas? Thanks, -Takahiro AKASHI ^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] inappropriate PCI configuration on arm64 qemu? 2018-05-28 7:59 [U-Boot] inappropriate PCI configuration on arm64 qemu? AKASHI Takahiro @ 2018-05-30 19:18 ` Simon Glass 2018-05-31 5:05 ` AKASHI Takahiro 0 siblings, 1 reply; 6+ messages in thread From: Simon Glass @ 2018-05-30 19:18 UTC (permalink / raw) To: u-boot +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 Regards, Simon ^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] inappropriate PCI configuration on arm64 qemu? 2018-05-30 19:18 ` Simon Glass @ 2018-05-31 5:05 ` AKASHI Takahiro 2018-05-31 10:32 ` Tuomas Tynkkynen 0 siblings, 1 reply; 6+ messages in thread From: AKASHI Takahiro @ 2018-05-31 5:05 UTC (permalink / raw) To: u-boot 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; ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [U-Boot] inappropriate PCI configuration on arm64 qemu? 2018-05-31 5:05 ` AKASHI Takahiro @ 2018-05-31 10:32 ` Tuomas Tynkkynen 2018-06-01 7:21 ` AKASHI Takahiro 0 siblings, 1 reply; 6+ messages in thread From: Tuomas Tynkkynen @ 2018-05-31 10:32 UTC (permalink / raw) To: u-boot Hi Akashi, On 05/31/2018 08:05 AM, AKASHI Takahiro wrote: > 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: Did you try it? As of today's master all of the patches are applied and at least the e1000 NIC and the Intel AHCI card that I tested works. The effect of the commit is to indeed avoid the problem you mentioned: >>> * As a result, MMIO region (0x1000.0000-0x2eff.0000) is overwritten >>> and MMIO_HIGH is the only one available at runtime. Note that even on aarch64, CONFIG_SYS_PCI_64BIT is *not* set by default. And on ARM we would need to skip that region in U-Boot anyway because we don't have the means to access physical addresses above the 4GB boundary with the CPU using U-Boot's identity-mapped page tables. > --- > 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. Yes, the patch is very minimal - I just wanted to fix the silent truncation of 64-bit addresses to 32-bit addresses and complain instead, nothing more. (As the default config of qemu_arm* doesn't have CONFIG_SYS_PCI_64BIT, the condition won't actually trigger in practice). > 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. > Yes, in theory it could be useful for some future hardware, but for QEMU point of view the current situation of totally ignoring the 64-bit mem region is probably good enough, given that most of the useful hardware that QEMU can enable is limited to 32-bit addressing anyway. ^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] inappropriate PCI configuration on arm64 qemu? 2018-05-31 10:32 ` Tuomas Tynkkynen @ 2018-06-01 7:21 ` AKASHI Takahiro 2018-06-01 10:20 ` Tuomas Tynkkynen 0 siblings, 1 reply; 6+ messages in thread From: AKASHI Takahiro @ 2018-06-01 7:21 UTC (permalink / raw) To: u-boot Tuomas, On Thu, May 31, 2018 at 01:32:20PM +0300, Tuomas Tynkkynen wrote: > Hi Akashi, > > On 05/31/2018 08:05 AM, AKASHI Takahiro wrote: > >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: > > Did you try it? As of today's master all of the patches are applied and at > least the e1000 NIC and the Intel AHCI card that I tested works. > The effect of the commit is to indeed avoid the problem you mentioned: Yes, I ran my patch but *with* CONFIG_SYS_PCI_64BIT. > >>> * As a result, MMIO region (0x1000.0000-0x2eff.0000) is overwritten > >>> and MMIO_HIGH is the only one available at runtime. > > Note that even on aarch64, CONFIG_SYS_PCI_64BIT is *not* set by default. > And on ARM we would need to skip that region in U-Boot anyway because > we don't have the means to access physical addresses above the 4GB > boundary with the CPU using U-Boot's identity-mapped page tables. Maybe you're right regarding aarch64, but the issue is not about arm/arm64 but PCI configuration. Some arch/machines, freescale mostly?, have already enabled CONFIG_SYS_PCI_64BIT. I'm afraid that there may be a possibility that your patch breaks them. Thanks, -Takahiro AKASHI > > >--- > > 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. > > Yes, the patch is very minimal - I just wanted to fix the silent truncation > of 64-bit addresses to 32-bit addresses and complain instead, nothing more. > > (As the default config of qemu_arm* doesn't have CONFIG_SYS_PCI_64BIT, > the condition won't actually trigger in practice). > > >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. > > > > Yes, in theory it could be useful for some future hardware, but for QEMU > point of view the current situation of totally ignoring the 64-bit mem region > is probably good enough, given that most of the useful hardware that QEMU > can enable is limited to 32-bit addressing anyway. ^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] inappropriate PCI configuration on arm64 qemu? 2018-06-01 7:21 ` AKASHI Takahiro @ 2018-06-01 10:20 ` Tuomas Tynkkynen 0 siblings, 0 replies; 6+ messages in thread From: Tuomas Tynkkynen @ 2018-06-01 10:20 UTC (permalink / raw) To: u-boot On 06/01/2018 10:21 AM, AKASHI Takahiro wrote: > Tuomas, > > On Thu, May 31, 2018 at 01:32:20PM +0300, Tuomas Tynkkynen wrote: >> Hi Akashi, >> >> On 05/31/2018 08:05 AM, AKASHI Takahiro wrote: >>> 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: >> >> Did you try it? As of today's master all of the patches are applied and at >> least the e1000 NIC and the Intel AHCI card that I tested works. >> The effect of the commit is to indeed avoid the problem you mentioned: > > Yes, I ran my patch but *with* CONFIG_SYS_PCI_64BIT. > I meant: Did you try your QEMU command that enables the SDHCI on U-Boot master, without any patches? It should just work. >>>>> * As a result, MMIO region (0x1000.0000-0x2eff.0000) is overwritten >>>>> and MMIO_HIGH is the only one available at runtime. >> >> Note that even on aarch64, CONFIG_SYS_PCI_64BIT is *not* set by default. >> And on ARM we would need to skip that region in U-Boot anyway because >> we don't have the means to access physical addresses above the 4GB >> boundary with the CPU using U-Boot's identity-mapped page tables. > > Maybe you're right regarding aarch64, but the issue is not about arm/arm64 > but PCI configuration. Some arch/machines, freescale mostly?, have > already enabled CONFIG_SYS_PCI_64BIT. I'm afraid that there may be > a possibility that your patch breaks them. > Which patch? The "PCI: dm: Ignore 64-bit memory regions if CONFIG_SYS_PCI_64BIT not set" commit adds: + if (!IS_ENABLED(CONFIG_SYS_PCI_64BIT) && + type == PCI_REGION_MEM && upper_32_bits(pci_addr)) { + debug(" - beyond the 32-bit boundary, ignoring\n"); + continue; + } I.e. the patch is a no-op if CONFIG_SYS_PCI_64BIT is set. Also I don't think PowerPC uses DM PCI, so that code is not executed at all on those platforms. If you mean in "PCI: autoconfig: Don't allocate 64-bit addresses to 32-bit only resources" that someone could have been relying on the truncation of high 32 bits of bus addresses with 32-bit only devices, none of the FSL platforms have bus addresses above 32-bits, only physical addresses (seen with git grep CONFIG_SYS_PCIE._MEM_), so that shouldn't cause a problem either. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-06-01 10:20 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-05-28 7:59 [U-Boot] inappropriate PCI configuration on arm64 qemu? AKASHI Takahiro 2018-05-30 19:18 ` Simon Glass 2018-05-31 5:05 ` AKASHI Takahiro 2018-05-31 10:32 ` Tuomas Tynkkynen 2018-06-01 7:21 ` AKASHI Takahiro 2018-06-01 10:20 ` Tuomas Tynkkynen
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.