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.ossl...@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.ossl...@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.
+
+ /* 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.
* @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
Thank you!
Alex
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
