[U-Boot] [PATCH 1/2] dm: pci: make ranges dt property optional

older
[U-Boot] [PATCH 1/2] fit: allow...

Christian Gmeiner

23 May 2018 23 May '18

2 p.m.

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(-)

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;

-- 2.17.0

Show replies by date

Christian Gmeiner

23 May 23 May

2 p.m.

New subject: [U-Boot] [PATCH 2/2] dm: pci: use a 1:1 mapping for bus <-> phy addresses

If u-boot gets used as coreboot payload all pci resources got assigned by coreboot. If a dts without any pci ranges gets used the dm is not able to access pci device memory. To get things working make use of a 1:1 mapping for bus <-> phy addresses.

This change makes it possible to get the e1000 u-boot driver working on a sandybridge device where u-boot is used as coreboot payload.

Signed-off-by: Christian Gmeiner christian.gmeiner@gmail.com --- drivers/pci/pci-uclass.c | 10 ++++++++++ 1 file changed, 10 insertions(+)

diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c index 416444a230..9cec619bb2 100644 --- a/drivers/pci/pci-uclass.c +++ b/drivers/pci/pci-uclass.c @@ -1175,6 +1175,11 @@ static int _dm_pci_bus_to_phys(struct udevice *ctlr, struct pci_region *res; int i;

+ if (hose->region_count == 0) { + *pa = bus_addr; + return 0; + } + for (i = 0; i < hose->region_count; i++) { res = &hose->regions[i];

@@ -1238,6 +1243,11 @@ int _dm_pci_phys_to_bus(struct udevice *dev, phys_addr_t phys_addr, ctlr = pci_get_controller(dev); hose = dev_get_uclass_priv(ctlr);

+ if (hose->region_count == 0) { + *ba = phys_addr; + return 0; + } + for (i = 0; i < hose->region_count; i++) { res = &hose->regions[i];

-- 2.17.0

Simon Glass

6:33 p.m.

New subject: [U-Boot] [PATCH 2/2] dm: pci: use a 1:1 mapping for bus <-> phy addresses

Hi Christian,

On 23 May 2018 at 06:00, Christian Gmeiner christian.gmeiner@gmail.com wrote:

...

If u-boot gets used as coreboot payload all pci resources got

U-Boot

(please use this consistently)

...

assigned by coreboot. If a dts without any pci ranges gets used the dm is not able to access pci device memory. To get things working make use of a 1:1 mapping for bus <-> phy addresses.

This change makes it possible to get the e1000 u-boot driver working on a sandybridge device where u-boot is used as coreboot payload.

Signed-off-by: Christian Gmeiner christian.gmeiner@gmail.com

drivers/pci/pci-uclass.c | 10 ++++++++++ 1 file changed, 10 insertions(+)

diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c index 416444a230..9cec619bb2 100644 --- a/drivers/pci/pci-uclass.c +++ b/drivers/pci/pci-uclass.c @@ -1175,6 +1175,11 @@ static int _dm_pci_bus_to_phys(struct udevice *ctlr, struct pci_region *res; int i;
  if (hose->region_count == 0) {

Or just if (!hose->region_count)

Please add a comment as to the case you are covering here. How come the mapping will be 1:1? Is that guaranteed?

...

```
          *pa = bus_addr;
```
```
          return 0;
```
```
  }
```

  for (i = 0; i < hose->region_count; i++) {
          res = &hose->regions[i];

@@ -1238,6 +1243,11 @@ int _dm_pci_phys_to_bus(struct udevice *dev, phys_addr_t phys_addr, ctlr = pci_get_controller(dev); hose = dev_get_uclass_priv(ctlr);

```
  if (hose->region_count == 0) {
```
```
          *ba = phys_addr;
```
```
          return 0;
```
```
  }
```

  for (i = 0; i < hose->region_count; i++) {
          res = &hose->regions[i];

-- 2.17.0

Regards, Simon

Bin Meng

3:10 p.m.

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

Christian Gmeiner

3:39 p.m.

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

-- greets -- Christian Gmeiner, MSc https://christian-gmeiner.info

Bin Meng

6 Jun 6 Jun

3:19 p.m.

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

2532

Age (days ago)

2546

Last active (days ago)

List overview

Download

5 comments

3 participants

tags (0)

participants (3)

Bin Meng
Christian Gmeiner
Simon Glass