| Message ID | 20180523120024.28672-1-christian.gmeiner@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Simon Glass |
| Headers | show |
| Series | [U-Boot,1/2] dm: pci: make ranges dt property optional | expand |
Hi Christian, On Wed, May 23, 2018 at 8:00 PM, Christian Gmeiner <christian.gmeiner@gmail.com> wrote: > If we use u-boot as coreboot payload with a generic dts without > any ranges specified we fail in pci pre_probe and our pci bus > is not usable. > What do you mean by "a generic dts"? The coreboot payload needs to specify a dedicated dts for that board, for example to build a coreboot payload for minnowmax, we need specify minnowmax.dts in the Kconfig. README.x86 documents this. > So convert decode_regions(..) into a void function and do the simple > error handling there. > > Signed-off-by: Christian Gmeiner <christian.gmeiner@gmail.com> > --- > drivers/pci/pci-uclass.c | 21 +++++++++------------ > 1 file changed, 9 insertions(+), 12 deletions(-) > > diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c > index 49be1ebdd7..416444a230 100644 > --- a/drivers/pci/pci-uclass.c > +++ b/drivers/pci/pci-uclass.c > @@ -810,7 +810,7 @@ error: > return ret; > } > > -static int decode_regions(struct pci_controller *hose, ofnode parent_node, > +static void decode_regions(struct pci_controller *hose, ofnode parent_node, > ofnode node) > { > int pci_addr_cells, addr_cells, size_cells; > @@ -820,8 +820,11 @@ static int decode_regions(struct pci_controller *hose, ofnode parent_node, > int i; > > prop = ofnode_get_property(node, "ranges", &len); > - if (!prop) > - return -EINVAL; > + if (!prop) { > + debug("%s: Cannot decode regions\n", __func__); > + return; > + } > + > pci_addr_cells = ofnode_read_simple_addr_cells(node); > addr_cells = ofnode_read_simple_addr_cells(parent_node); > size_cells = ofnode_read_simple_size_cells(node); > @@ -876,7 +879,7 @@ static int decode_regions(struct pci_controller *hose, ofnode parent_node, > bd_t *bd = gd->bd; > > if (!bd) > - return 0; > + return; > > for (i = 0; i < CONFIG_NR_DRAM_BANKS; ++i) { > if (bd->bi_dram[i].size) { > @@ -901,13 +904,12 @@ static int decode_regions(struct pci_controller *hose, ofnode parent_node, > base, size, PCI_REGION_MEM | PCI_REGION_SYS_MEMORY); > #endif > > - return 0; > + return; > } > > static int pci_uclass_pre_probe(struct udevice *bus) > { > struct pci_controller *hose; > - int ret; > > debug("%s, bus=%d/%s, parent=%s\n", __func__, bus->seq, bus->name, > bus->parent->name); > @@ -916,12 +918,7 @@ static int pci_uclass_pre_probe(struct udevice *bus) > /* For bridges, use the top-level PCI controller */ > if (!device_is_on_pci_bus(bus)) { > hose->ctlr = bus; > - ret = decode_regions(hose, dev_ofnode(bus->parent), > - dev_ofnode(bus)); > - if (ret) { > - debug("%s: Cannot decode regions\n", __func__); > - return ret; > - } > + decode_regions(hose, dev_ofnode(bus->parent), dev_ofnode(bus)); > } else { > struct pci_controller *parent_hose; > > -- > Regards, Bin
Hi Bin, Am Mi., 23. Mai 2018 um 15:10 Uhr schrieb Bin Meng <bmeng.cn@gmail.com>: > Hi Christian, > On Wed, May 23, 2018 at 8:00 PM, Christian Gmeiner > <christian.gmeiner@gmail.com> wrote: > > If we use u-boot as coreboot payload with a generic dts without > > any ranges specified we fail in pci pre_probe and our pci bus > > is not usable. > > > What do you mean by "a generic dts"? > The coreboot payload needs to specify a dedicated dts for that board, > for example to build a coreboot payload for minnowmax, we need specify > minnowmax.dts in the Kconfig. README.x86 documents this. Yeah.. so in my eyes a "generic dts" contains only this part regarding pci: pci { compatible = "pci-x86"; #address-cells = <0x3>; #size-cells = <0x2>; }; The important part is that it does not contain any ranges. Coreboot is the one who does the pci bus setup (assigning memory windows for devices etc). So I do not want to know in u-boot at build time (ranges thing for pci) how the pci bus looks regarding addresses. My end goal is one generic u-boot payload that gets used for different FSP 2.0 based boards. > > So convert decode_regions(..) into a void function and do the simple > > error handling there. > > > > Signed-off-by: Christian Gmeiner <christian.gmeiner@gmail.com> > > --- > > drivers/pci/pci-uclass.c | 21 +++++++++------------ > > 1 file changed, 9 insertions(+), 12 deletions(-) > > > > diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c > > index 49be1ebdd7..416444a230 100644 > > --- a/drivers/pci/pci-uclass.c > > +++ b/drivers/pci/pci-uclass.c > > @@ -810,7 +810,7 @@ error: > > return ret; > > } > > > > -static int decode_regions(struct pci_controller *hose, ofnode parent_node, > > +static void decode_regions(struct pci_controller *hose, ofnode parent_node, > > ofnode node) > > { > > int pci_addr_cells, addr_cells, size_cells; > > @@ -820,8 +820,11 @@ static int decode_regions(struct pci_controller *hose, ofnode parent_node, > > int i; > > > > prop = ofnode_get_property(node, "ranges", &len); > > - if (!prop) > > - return -EINVAL; > > + if (!prop) { > > + debug("%s: Cannot decode regions\n", __func__); > > + return; > > + } > > + > > pci_addr_cells = ofnode_read_simple_addr_cells(node); > > addr_cells = ofnode_read_simple_addr_cells(parent_node); > > size_cells = ofnode_read_simple_size_cells(node); > > @@ -876,7 +879,7 @@ static int decode_regions(struct pci_controller *hose, ofnode parent_node, > > bd_t *bd = gd->bd; > > > > if (!bd) > > - return 0; > > + return; > > > > for (i = 0; i < CONFIG_NR_DRAM_BANKS; ++i) { > > if (bd->bi_dram[i].size) { > > @@ -901,13 +904,12 @@ static int decode_regions(struct pci_controller *hose, ofnode parent_node, > > base, size, PCI_REGION_MEM | PCI_REGION_SYS_MEMORY); > > #endif > > > > - return 0; > > + return; > > } > > > > static int pci_uclass_pre_probe(struct udevice *bus) > > { > > struct pci_controller *hose; > > - int ret; > > > > debug("%s, bus=%d/%s, parent=%s\n", __func__, bus->seq, bus->name, > > bus->parent->name); > > @@ -916,12 +918,7 @@ static int pci_uclass_pre_probe(struct udevice *bus) > > /* For bridges, use the top-level PCI controller */ > > if (!device_is_on_pci_bus(bus)) { > > hose->ctlr = bus; > > - ret = decode_regions(hose, dev_ofnode(bus->parent), > > - dev_ofnode(bus)); > > - if (ret) { > > - debug("%s: Cannot decode regions\n", __func__); > > - return ret; > > - } > > + decode_regions(hose, dev_ofnode(bus->parent), dev_ofnode(bus)); > > } else { > > struct pci_controller *parent_hose; > > > > -- > > > Regards, > Bin
Hi Christian, On Wed, May 23, 2018 at 9:39 PM, Christian Gmeiner <christian.gmeiner@gmail.com> wrote: > Hi Bin, > > Am Mi., 23. Mai 2018 um 15:10 Uhr schrieb Bin Meng <bmeng.cn@gmail.com>: > >> Hi Christian, > >> On Wed, May 23, 2018 at 8:00 PM, Christian Gmeiner >> <christian.gmeiner@gmail.com> wrote: >> > If we use u-boot as coreboot payload with a generic dts without >> > any ranges specified we fail in pci pre_probe and our pci bus >> > is not usable. >> > > >> What do you mean by "a generic dts"? > > >> The coreboot payload needs to specify a dedicated dts for that board, >> for example to build a coreboot payload for minnowmax, we need specify >> minnowmax.dts in the Kconfig. README.x86 documents this. > > > Yeah.. so in my eyes a "generic dts" contains only this part regarding pci: > > pci { > compatible = "pci-x86"; > #address-cells = <0x3>; > #size-cells = <0x2>; > }; > > The important part is that it does not contain any ranges. Coreboot is the > one > who does the pci bus setup (assigning memory windows for devices etc). So I > do not want to know in u-boot at build time (ranges thing for pci) how the > pci > bus looks regarding addresses. My end goal is one generic u-boot payload > that > gets used for different FSP 2.0 based boards. > I see your point. Then they way we support coreboot might be changed. We may create a generic U-Boot payload for coreboot targets. Can you respin the patch to address these comments, like u-boot -> U-Boot, adding detailed info to the commit message about this, and update README.x86? Regards, Bin
diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c index 49be1ebdd7..416444a230 100644 --- a/drivers/pci/pci-uclass.c +++ b/drivers/pci/pci-uclass.c @@ -810,7 +810,7 @@ error: return ret; } -static int decode_regions(struct pci_controller *hose, ofnode parent_node, +static void decode_regions(struct pci_controller *hose, ofnode parent_node, ofnode node) { int pci_addr_cells, addr_cells, size_cells; @@ -820,8 +820,11 @@ static int decode_regions(struct pci_controller *hose, ofnode parent_node, int i; prop = ofnode_get_property(node, "ranges", &len); - if (!prop) - return -EINVAL; + if (!prop) { + debug("%s: Cannot decode regions\n", __func__); + return; + } + pci_addr_cells = ofnode_read_simple_addr_cells(node); addr_cells = ofnode_read_simple_addr_cells(parent_node); size_cells = ofnode_read_simple_size_cells(node); @@ -876,7 +879,7 @@ static int decode_regions(struct pci_controller *hose, ofnode parent_node, bd_t *bd = gd->bd; if (!bd) - return 0; + return; for (i = 0; i < CONFIG_NR_DRAM_BANKS; ++i) { if (bd->bi_dram[i].size) { @@ -901,13 +904,12 @@ static int decode_regions(struct pci_controller *hose, ofnode parent_node, base, size, PCI_REGION_MEM | PCI_REGION_SYS_MEMORY); #endif - return 0; + return; } static int pci_uclass_pre_probe(struct udevice *bus) { struct pci_controller *hose; - int ret; debug("%s, bus=%d/%s, parent=%s\n", __func__, bus->seq, bus->name, bus->parent->name); @@ -916,12 +918,7 @@ static int pci_uclass_pre_probe(struct udevice *bus) /* For bridges, use the top-level PCI controller */ if (!device_is_on_pci_bus(bus)) { hose->ctlr = bus; - ret = decode_regions(hose, dev_ofnode(bus->parent), - dev_ofnode(bus)); - if (ret) { - debug("%s: Cannot decode regions\n", __func__); - return ret; - } + decode_regions(hose, dev_ofnode(bus->parent), dev_ofnode(bus)); } else { struct pci_controller *parent_hose;
If we use u-boot as coreboot payload with a generic dts without any ranges specified we fail in pci pre_probe and our pci bus is not usable. So convert decode_regions(..) into a void function and do the simple error handling there. Signed-off-by: Christian Gmeiner <christian.gmeiner@gmail.com> --- drivers/pci/pci-uclass.c | 21 +++++++++------------ 1 file changed, 9 insertions(+), 12 deletions(-)