[U-Boot] [PATCH 22/39] x86: Move Coreboot PCI into common cpu area

[Simon Glass]

Hi Bin,

On 6 November 2014 19:12, Bin Meng <bmeng.cn at gmail.com> wrote:
> Hi Simon,
>
> On Fri, Nov 7, 2014 at 9:53 AM, Simon Glass <sjg at chromium.org> wrote:
>> Hi Bin,
>>
>>
>> On 6 November 2014 18:39, Bin Meng <bmeng.cn at gmail.com> wrote:
>>> Hi Simon,
>>>
>>> On Fri, Nov 7, 2014 at 8:26 AM, Simon Glass <sjg at chromium.org> wrote:
>>>> Hi Bin,
>>>>
>>>>
>>>> On 6 November 2014 17:07, Bin Meng <bmeng.cn at gmail.com> wrote:
>>>>> Hi Simon,
>>>>>
>>>>>> -static struct pci_config_table pci_coreboot_config_table[] = {
>>>>>> +static struct pci_config_table pci_x86_config_table[] = {
>>>>>>         /* vendor, device, class, bus, dev, func */
>>>>>>         { PCI_ANY_ID, PCI_ANY_ID, PCI_CLASS_BRIDGE_PCI,
>>>>>>                 PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID, &config_pci_bridge },
>>>>>> @@ -33,17 +33,19 @@ static struct pci_config_table pci_coreboot_config_table[] = {
>>>>>
>>>>> Use this config_table will cause infinite loop when doing
>>>>> pci_hose_scan later. I do not use config_table on my Crown Bay port,
>>>>> instead using CONFIG_PCI_PNP which works very well.
>>>>
>>>> OK, so are you saying that we should leave this separate for each board?
>>>
>>> Not really. But we can make this code really generic. So far it is broken.
>>
>> I think the issue is that booting from Coreboot is quite a different
>> use case from everything else.
>
> We can use #ifdef CONFIG_SYS_COREBOOT in avoid re-allocating memory
> resources in U-Boot.

Hmm in that case I'd rather keep the files separate, since I'd like to
minimise the use of #ifdef.

Can we just leave off CONFIG_PCI_PNP?

>
>>>>>>  void pci_init_board(void)
>>>>>>  {
>>>>>> -       coreboot_hose.config_table = pci_coreboot_config_table;
>>>>>> -       coreboot_hose.first_busno = 0;
>>>>>> -       coreboot_hose.last_busno = 0;
>>>>>> +       struct pci_controller *hose = &x86_hose;
>>>>>>
>>>>>> -       pci_set_region(coreboot_hose.regions + 0, 0x0, 0x0, 0xffffffff,
>>>>>> -               PCI_REGION_MEM);
>>>>>> -       coreboot_hose.region_count = 1;
>>>>>> +       hose->config_table = pci_x86_config_table;
>>>>>> +       hose->first_busno = 0;
>>>>>> +       hose->last_busno = 0;
>>>>>>
>>>>>> -       pci_setup_type1(&coreboot_hose);
>>>>>> +       pci_set_region(hose->regions + 0, 0x0, 0x0, 0xffffffff,
>>>>>> +                      PCI_REGION_MEM);
>>>>>> +       hose->region_count = 1;
>>>>>
>>>>> There are 3 issues with the pci_set_region here:
>>>>> 1). The whole 4GiB PCI memory region creats conflicts with the systeam
>>>>> RAM memory map. This is a programming effor and normally causes
>>>>> undefined behaviour from chipset perspective.
>>>>> 2). There is no IO region configured, that means any device behind the
>>>>> PCI/PCIe bridge with only IO bar will fail to work.
>>>>> 3). There is no systeam RAM region configured, that means any device
>>>>> driver behind the PCI/PCIe bridge will fail to create pci addr <-> cpu
>>>>> physical addr mappings.
>>>>
>>>> Actually this is not real setup - for the coreboot case it is
>>>> scan-only, there is no memory or I/O allocation going on.
>>>
>>> OK, so if U-Boot boots from coreboot, the issue #1 and #2 disappear
>>> because coreboot has already enumerated the buses and devices and have
>>> memory/io allocation setup. That's fine. But this code is assumed to
>>> be used in U-Boot without coreboot too, thus U-Boot has to do the same
>>> thing as coreboot. And in the latter case, issue #1 and #2 do matter.
>>> About the issue #3, even when U-Boot is booting from coreboot, the
>>> system RAM region still needs to be set up otherwise you won't get any
>>> PCI device driver in U-Boot work. You can try adding CONFIG_E1000 to
>>> test an Intel Pro/1000 NIC in U-Boot and see what's happening.
>>
>> If I do that, then Coreboot will find the device and allocate space
>> for it, then U-Boot will juts use the allocated space.
>
> Yes, I understand this.
>
>> Actually I think the region is completely misleading in the Coreboot
>> case and should juts be remove.
>
> I am not sure if memory and IO can be removed completely for the
> coreboot case. I will need look into the pci.c/pci_auto.c and PCI
> device driver to confirm.
>
>> Issue 3 is not a problem either I think, since again Coreboot will allocate it.
>
> No, it does matter for coreboot. If the pci hose does not have the
> system RAM region setup, the pci_mem_to_phys will fail when PCI device
> driver wants to do DMA to RAM.

Good point, that is not supported at present.

So perhaps we should always set up suitable memory/I/O/bus-master
regions and for Coreboot they will just not be used.

Regards,
Simon