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

[Tuomas Tynkkynen]

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 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:
>>
>> 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.