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