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

[Bin Meng]

+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;

> +       u32 ea_entry;
> +       u64 addr;

This should be: pci_addr_t addr

> +
> +       /* 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.

> +       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.

>   * @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.

Regards,
Bin