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

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

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.