[U-Boot] [PATCH 1/2] drivers: pci: add map_bar support for Enhanced Allocation
Bin Meng
bmeng.cn at gmail.com
Mon Jun 3 13:01:14 UTC 2019
Hi Alex,
On Mon, Jun 3, 2019 at 8:49 PM Alex Marginean <alexm.osslist at 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 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.
>
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
More information about the U-Boot
mailing list