+Simon, Hi Alex,
On Sat, Jun 1, 2019 at 12:26 AM Alex Marginean <[email protected]> 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 <[email protected]> > --- > 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 _______________________________________________ U-Boot mailing list [email protected] https://lists.denx.de/listinfo/u-boot
