[U-Boot] [PATCH 4/8] dm: pci: Support decoding ranges with duplicate entries

[Simon Glass <sjg@chromium.org>]

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