Hi Bin, On 6 November 2014 19:12, Bin Meng <bmeng...@gmail.com> wrote: > 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.
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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot