[U-Boot] [PATCH 1/2] drivers: pci: add map_bar support for Enhanced Allocation

[Alex Marginean]

Hi Bin,

On 6/2/2019 4:15 PM, Bin Meng wrote:
> +Simon,
> 
> Hi Alex,
> 
> On Sat, Jun 1, 2019 at 12:26 AM Alex Marginean <alexm.osslist at gmail.com> wrote:
>>
>> Makes dm_pci_map_bar function available for integrated PCI devices that
>> support Enhanced Allocation instead of original PCI BAR mechanism.
>>
>> Signed-off-by: Alex Marginean <alexm.osslist at gmail.com>
>> ---
>>   drivers/pci/pci-uclass.c | 47 ++++++++++++++++++++++++++++++++++++++++
>>   include/pci.h            |  2 +-
>>   2 files changed, 48 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c
>> index cf1e7617ae..3204f156c3 100644
>> --- a/drivers/pci/pci-uclass.c
>> +++ b/drivers/pci/pci-uclass.c
>> @@ -1341,11 +1341,58 @@ pci_addr_t dm_pci_phys_to_bus(struct udevice *dev, phys_addr_t phys_addr,
>>          return bus_addr;
>>   }
>>
>> +static void *dm_pci_map_ea_bar(struct udevice *dev, int bar, int flags)
>> +{
>> +       int ea_off, ea_cnt, i, entry_size = 0;
> 
> nits: no need to initialize entry_size here.
> 
>> +       int bar_id = bar - PCI_BASE_ADDRESS_0;
> 
> This does not work for anything other than BAR0. It should be (bar -
> PCI_BASE_ADDRESS_0) >> 2;

Good find, you're right, I did use it only for BAR0 and missed this.

> 
>> +       u32 ea_entry;
>> +       u64 addr;
> 
> This should be: pci_addr_t addr

I think maybe phys_addr_t is more appropriate, EA functions are supposed
to be integrated and their BAR equivalent addresses map into system
address space.  In the end this goes to map_physmem, which takes a
phys_addr_t.

> 
>> +
>> +       /* handle PCI functions that use Enhanced Allocation */
>> +       ea_off = dm_pci_find_capability(dev, PCI_CAP_ID_EA);
>> +
>> +       if (!ea_off)
>> +               return 0;
> 
> Above codes are not necessary. EA offset is already known when calling
> dm_pci_map_ea_bar() from dm_pci_map_bar(). We can pass the offset to
> this function.
> 
>> +
>> +       /* EA capability structure header */
>> +       dm_pci_read_config32(dev, ea_off, &ea_entry);
>> +       ea_cnt = (ea_entry >> 16) & 0x3f;
> 
> Avoid using magic numbers, instead use a macro PCI_EA_NUM_ENT_MASK. In
> fact, Linux has several macros for EA capability
> (include/uapi/linux/pci_regs.h) and we can just import these macros in
> U-Boot too.

That's a good suggestion, I will do that.

> 
>> +       ea_off += 4;
>> +
>> +       for (i = 0; i < ea_cnt; i++, ea_off +=  entry_size) {
> 
> nits: two spaces before entry_size
> 
>> +               /* Entry header */
>> +               dm_pci_read_config32(dev, ea_off, &ea_entry);
>> +               entry_size = (ea_entry & 0x7) * 4;
> 
> Per the spec, entry size is number of DW following the initial DW in
> this entry. So it should really be: ((ea_entry & 0x7) + 1) * 4. Again
> like the bar_id comments above, we can use << 2 here instead of * 4.
> 
>> +
>> +               if (((ea_entry >> 4) & 0xf) != bar_id)
>> +                       continue;
>> +
>> +               /* Base address, 1st DW */
>> +               dm_pci_read_config32(dev, ea_off + 4, &ea_entry);
>> +               addr = ea_entry & ~0x3;
>> +               if (ea_entry & 0x2) {
>> +                       dm_pci_read_config32(dev, ea_off + 12, &ea_entry);
>> +                       addr |= (u64)ea_entry << 32;
>> +               }
>> +
>> +               /* size ignored for now */
>> +               return map_physmem(addr, flags, 0);
>> +       }
> 
> nits: should have one blank line here
> 
>> +       return 0;
>> +}
>> +
>>   void *dm_pci_map_bar(struct udevice *dev, int bar, int flags)
>>   {
>>          pci_addr_t pci_bus_addr;
>>          u32 bar_response;
>>
>> +       /*
>> +        * if the function supports Enhanced Allocation use that instead of
>> +        * BARs
>> +        */
>> +       if (dm_pci_find_capability(dev, PCI_CAP_ID_EA))
>> +               return dm_pci_map_ea_bar(dev, bar, flags);
>> +
>>          /* read BAR address */
>>          dm_pci_read_config32(dev, bar, &bar_response);
>>          pci_bus_addr = (pci_addr_t)(bar_response & ~0xf);
>> diff --git a/include/pci.h b/include/pci.h
>> index 508f7bca81..e1528bb257 100644
>> --- a/include/pci.h
>> +++ b/include/pci.h
>> @@ -1314,7 +1314,7 @@ pci_addr_t dm_pci_phys_to_bus(struct udevice *dev, phys_addr_t addr,
>>    * @dev:       Device to check
>>    * @bar:       Bar number to read (numbered from 0)
> 
> This one is confusing. It it not bar number (0/1/...), but bar
> register offset. Suggest a separate patch to correct it. And this
> function seems to only handle BAR0-BAR0 for header type 0. Please also
> comment that.

I suppose it works for BARs0-5 on type 0.  I'm not clear if it also
works for type 1 though, type 1 defines a couple of BARs at offset 0x10
too.

> 
>>    * @flags:     Flags for the region type (PCI_REGION_...)
>> - * @return: pointer to the virtual address to use
>> + * @return: pointer to the virtual address to use or 0 on error
> 
> This should be separate patch to correct the comments. Together with
> the bar comments above.
> 
>>    */
>>   void *dm_pci_map_bar(struct udevice *dev, int bar, int flags);
> 
> Please create test cases in test/dm/pci.c to cover the EA capability
> test. Especially since there are some bugs in the for loop in
> dm_pci_map_ea_bar(), we should create case to get something like BAR3
> instead of BAR0. I suspect why you did not see the issue was because
> you only covered the BAR0 hence only one iteration of the for loop was
> executed.

Yes, that's precisely what I did..

> 
> Regards,
> Bin
> 

Thank you!
Alex