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

[Bin Meng]

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.

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

Regards,
Bin