* [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.