Hi Simon, On Fri, Nov 7, 2014 at 8:26 AM, Simon Glass <s...@chromium.org> wrote: > Hi Bin, > > > On 6 November 2014 17:07, Bin Meng <bmeng...@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot