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

[Bin Meng <bmeng.cn@gmail.com>]

Hi Alex,

On Mon, Jun 3, 2019 at 8:49 PM Alex Marginean <alexm.osslist@gmail.com> wrote:
>
> 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@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@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.
>

Makes sense.

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

Yes, I think type 1's BAR0/BAR1 are also supported.

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