| Message ID | 1415305231-30180-23-git-send-email-sjg@chromium.org |
|---|---|
| State | Superseded |
| Delegated to: | Simon Glass |
| Headers | show |
Hi Simon, On Fri, Nov 7, 2014 at 4:20 AM, Simon Glass <sjg@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@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. > 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. > - pci_register_hose(&coreboot_hose); > + pci_setup_type1(hose); > > - pci_hose_scan(&coreboot_hose); > + pci_register_hose(hose); > + > + pci_hose_scan(hose); > } Regards, Bin
Hi Bin, On 6 November 2014 17:07, Bin Meng <bmeng.cn@gmail.com> wrote: > Hi Simon, > > On Fri, Nov 7, 2014 at 4:20 AM, Simon Glass <sjg@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@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
Hi Simon, On Fri, Nov 7, 2014 at 8:26 AM, Simon Glass <sjg@chromium.org> wrote: > Hi Bin, > > > On 6 November 2014 17:07, Bin Meng <bmeng.cn@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
Hi Bin, On 6 November 2014 18:39, Bin Meng <bmeng.cn@gmail.com> wrote: > Hi Simon, > > On Fri, Nov 7, 2014 at 8:26 AM, Simon Glass <sjg@chromium.org> wrote: >> Hi Bin, >> >> >> On 6 November 2014 17:07, Bin Meng <bmeng.cn@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. > >>>> 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. Actually I think the region is completely misleading in the Coreboot case and should juts be remove. Issue 3 is not a problem either I think, since again Coreboot will allocate it. Regards, Simon
Hi Simon, On Fri, Nov 7, 2014 at 9:53 AM, Simon Glass <sjg@chromium.org> wrote: > Hi Bin, > > > On 6 November 2014 18:39, Bin Meng <bmeng.cn@gmail.com> wrote: >> Hi Simon, >> >> On Fri, Nov 7, 2014 at 8:26 AM, Simon Glass <sjg@chromium.org> wrote: >>> Hi Bin, >>> >>> >>> On 6 November 2014 17:07, Bin Meng <bmeng.cn@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
Hi Bin, On 6 November 2014 19:12, Bin Meng <bmeng.cn@gmail.com> wrote: > Hi Simon, > > On Fri, Nov 7, 2014 at 9:53 AM, Simon Glass <sjg@chromium.org> wrote: >> Hi Bin, >> >> >> On 6 November 2014 18:39, Bin Meng <bmeng.cn@gmail.com> wrote: >>> Hi Simon, >>> >>> On Fri, Nov 7, 2014 at 8:26 AM, Simon Glass <sjg@chromium.org> wrote: >>>> Hi Bin, >>>> >>>> >>>> On 6 November 2014 17:07, Bin Meng <bmeng.cn@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
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[] = { 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; - pci_register_hose(&coreboot_hose); + pci_setup_type1(hose); - pci_hose_scan(&coreboot_hose); + pci_register_hose(hose); + + pci_hose_scan(hose); }
This code is not really Coreboot-specific, so move it into the common area and rename it. Signed-off-by: Simon Glass <sjg@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%)