| Message ID | 1445104205-4079-5-git-send-email-sjg@chromium.org |
|---|---|
| State | Superseded |
| Delegated to: | Simon Glass |
| Headers | show |
On 10/17/2015 11:50 AM, Simon Glass wrote: > At present we add a new resource entry for every range entry. But some range > entries refer to configuration regions. To make this work, avoid adding two > regions of the same time. The later ranges will overwrite the earlier > (configuration) ones. s/time/type/ in the last-but-one line. What's wrong with having two regions of the same type? Equally, if we can "get away" with not storing some of the regions that happen to have a duplicate type, why not recast the function so that it only stores regions of specific (useful/desired) types, and simply dropping all of the other regions. That'd be a lot more consistent than only storing a somewhat arbitrary subset of the regions. > There does not seem to be a way to distinguish the configuration ranges > other than by ordering. Well, they do have different addresses too. But yes, the DT binding is written so that the entries in ranges must appear in a specific order, so order is the correct way to index the entries. > diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c > @@ -720,9 +721,15 @@ static int decode_regions(struct pci_controller *hose, const void *blob, > } else { > continue; > } > - debug(" - type=%d\n", type); > - pci_set_region(hose->regions + hose->region_count++, pci_addr, > - addr, size, type); > + pos = -1; > + for (i = 0; i < hose->region_count; i++) { > + if (hose->regions[i].flags == type) > + pos = i; and break too? > + } > + if (pos == -1) > + pos = hose->region_count++; > + debug(" - type=%d, pos=%d\n", type, pos); > + pci_set_region(hose->regions + pos, pci_addr, addr, size, type); > }
Hi Stephen, On 21 October 2015 at 14:25, Stephen Warren <swarren@wwwdotorg.org> wrote: > On 10/17/2015 11:50 AM, Simon Glass wrote: >> >> At present we add a new resource entry for every range entry. But some >> range >> entries refer to configuration regions. To make this work, avoid adding >> two >> regions of the same time. The later ranges will overwrite the earlier >> (configuration) ones. > > > s/time/type/ in the last-but-one line. > > What's wrong with having two regions of the same type? Equally, if we can > "get away" with not storing some of the regions that happen to have a > duplicate type, why not recast the function so that it only stores regions > of specific (useful/desired) types, and simply dropping all of the other > regions. That'd be a lot more consistent than only storing a somewhat > arbitrary subset of the regions. I'm not 100% sure that we want to disallow multiple regions. Although on non-x86 boards I've haven't seen hardware that supports multiple regions. > >> There does not seem to be a way to distinguish the configuration ranges >> other than by ordering. > > > Well, they do have different addresses too. But yes, the DT binding is > written so that the entries in ranges must appear in a specific order, so > order is the correct way to index the entries. > >> diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c > > >> @@ -720,9 +721,15 @@ static int decode_regions(struct pci_controller >> *hose, const void *blob, >> } else { >> continue; >> } >> - debug(" - type=%d\n", type); >> - pci_set_region(hose->regions + hose->region_count++, >> pci_addr, >> - addr, size, type); >> + pos = -1; >> + for (i = 0; i < hose->region_count; i++) { >> + if (hose->regions[i].flags == type) >> + pos = i; > > > and break too? Could do, might be clearer. > > >> + } >> + if (pos == -1) >> + pos = hose->region_count++; >> + debug(" - type=%d, pos=%d\n", type, pos); >> + pci_set_region(hose->regions + pos, pci_addr, addr, size, >> type); >> } > > Regards, Simon
diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c index 6dda056..a59e468 100644 --- a/drivers/pci/pci-uclass.c +++ b/drivers/pci/pci-uclass.c @@ -698,6 +698,7 @@ static int decode_regions(struct pci_controller *hose, const void *blob, int space_code; u32 flags; int type; + int pos; if (len < cells_per_record) break; @@ -720,9 +721,15 @@ static int decode_regions(struct pci_controller *hose, const void *blob, } else { continue; } - debug(" - type=%d\n", type); - pci_set_region(hose->regions + hose->region_count++, pci_addr, - addr, size, type); + pos = -1; + for (i = 0; i < hose->region_count; i++) { + if (hose->regions[i].flags == type) + pos = i; + } + if (pos == -1) + pos = hose->region_count++; + debug(" - type=%d, pos=%d\n", type, pos); + pci_set_region(hose->regions + pos, pci_addr, addr, size, type); } /* Add a region for our local memory */
At present we add a new resource entry for every range entry. But some range entries refer to configuration regions. To make this work, avoid adding two regions of the same time. The later ranges will overwrite the earlier (configuration) ones. There does not seem to be a way to distinguish the configuration ranges other than by ordering. Signed-off-by: Simon Glass <sjg@chromium.org> --- drivers/pci/pci-uclass.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-)