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

[Simon Glass]

Hi Bin,


On 6 November 2014 17:07, Bin Meng <bmeng.cn at gmail.com> wrote:
> Hi Simon,
>
> On Fri, Nov 7, 2014 at 4:20 AM, Simon Glass <sjg at chromium.org> wrote:
>> This code is not really Coreboot-specific, so move it into the common area
>> and rename it.
>
> OK, but current coreboot pci codes are broken, thus need to be fixed
> before making it common.
>
>> Signed-off-by: Simon Glass <sjg at chromium.org>
>> ---
>>
>>  arch/x86/cpu/Makefile             |  1 +
>>  arch/x86/cpu/coreboot/Makefile    |  1 -
>>  arch/x86/cpu/{coreboot => }/pci.c | 24 +++++++++++++-----------
>>  3 files changed, 14 insertions(+), 12 deletions(-)
>>  rename arch/x86/cpu/{coreboot => }/pci.c (63%)
>>
>> diff --git a/arch/x86/cpu/Makefile b/arch/x86/cpu/Makefile
>> index 9d38ef7..97f36d5 100644
>> --- a/arch/x86/cpu/Makefile
>> +++ b/arch/x86/cpu/Makefile
>> @@ -11,3 +11,4 @@
>>  extra-y        = start.o
>>  obj-$(CONFIG_X86_RESET_VECTOR) += resetvec.o start16.o
>>  obj-y  += interrupts.o cpu.o call64.o
>> +obj-$(CONFIG_PCI) += pci.o
>> diff --git a/arch/x86/cpu/coreboot/Makefile b/arch/x86/cpu/coreboot/Makefile
>> index cd0bf4e..65cf2cb 100644
>> --- a/arch/x86/cpu/coreboot/Makefile
>> +++ b/arch/x86/cpu/coreboot/Makefile
>> @@ -19,4 +19,3 @@ obj-$(CONFIG_SYS_COREBOOT) += tables.o
>>  obj-$(CONFIG_SYS_COREBOOT) += ipchecksum.o
>>  obj-$(CONFIG_SYS_COREBOOT) += sdram.o
>>  obj-$(CONFIG_SYS_COREBOOT) += timestamp.o
>> -obj-$(CONFIG_PCI) += pci.o
>> diff --git a/arch/x86/cpu/coreboot/pci.c b/arch/x86/cpu/pci.c
>> similarity index 63%
>> rename from arch/x86/cpu/coreboot/pci.c
>> rename to arch/x86/cpu/pci.c
>> index 33f16a3..f35c8b3 100644
>> --- a/arch/x86/cpu/coreboot/pci.c
>> +++ b/arch/x86/cpu/pci.c
>> @@ -13,7 +13,7 @@
>>  #include <pci.h>
>>  #include <asm/pci.h>
>>
>> -static struct pci_controller coreboot_hose;
>> +static struct pci_controller x86_hose;
>>
>>  static void config_pci_bridge(struct pci_controller *hose, pci_dev_t dev,
>>                               struct pci_config_table *table)
>> @@ -24,7 +24,7 @@ static void config_pci_bridge(struct pci_controller *hose, pci_dev_t dev,
>>         pci_hose_scan_bus(hose, secondary);
>>  }
>>
>> -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?

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

>
>> -       pci_register_hose(&coreboot_hose);
>> +       pci_setup_type1(hose);
>>
>> -       pci_hose_scan(&coreboot_hose);
>> +       pci_register_hose(hose);
>> +
>> +       pci_hose_scan(hose);
>>  }
>
> Regards,
> Bin

Regards,
Simon