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