Hi Yehuda,
On 27.10.2016 10:42, yehuday@marvell.com wrote:
From: Yehuda Yitschak yehuday@marvell.com
Currently the PCI command only allows to see the BAR register values but not the size and actual base address. This little extension parses the BAR registers and displays the base, size and type of each BAR.
Signed-off-by: Yehuda Yitschak yehuday@marvell.com
I just applied this patch on top of latest mainline (with some other patches on applied as well). And get this error:
cmd/pci.c: In function 'do_pci': cmd/pci.c:715:10: warning: implicit declaration of function 'pci_bar_show' [-Wimplicit-function-declaration] return pci_bar_show(bdf); ^ cmd/built-in.o: In function `do_pci': /home/stefan/git/u-boot/u-boot/cmd/pci.c:715: undefined reference to `pci_bar_show'
This is because I have CONFIG_DM_PCI enabled in my case. It would be great if you could add support for DM_PCI based systems as well.
Please find some further minor nitpicking comments below.
cmd/pci.c | 74 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 74 insertions(+)
diff --git a/cmd/pci.c b/cmd/pci.c index 2f4978a..2db834d 100644 --- a/cmd/pci.c +++ b/cmd/pci.c @@ -80,6 +80,75 @@ static unsigned long pci_read_config(pci_dev_t dev, int offset, return val32; } }
Add an empty line here.
+/*
- Subroutine: PCI_BAR_Show
- Description: Reads the BAR registers and decodes them
- Inputs: BusDevFunc Bus+Device+Function number
- Return: 0 on success, -1 on failure
- */
+int pci_bar_show(pci_dev_t dev) +{
- u8 header_type;
- int bar_cnt, bar_id, is_64;
- u32 base_low, base_high;
- u32 size_low, size_high;
- u64 base, size;
- u32 reg_addr;
- pci_read_config_byte(dev, PCI_HEADER_TYPE, &header_type);
- if (header_type == PCI_HEADER_TYPE_CARDBUS) {
printf("CardBus doesn't support BARs\n");return -1;- }
- bar_cnt = (header_type == PCI_HEADER_TYPE_NORMAL) ? 6 : 2;
- printf(" ID Base Size Type\n");
- printf("-------------------------------------------------\n");
- bar_id = 0;
- reg_addr = PCI_BASE_ADDRESS_0;
- while (bar_cnt) {
pci_read_config_dword(dev, reg_addr, &base_low);pci_write_config_dword(dev, reg_addr, 0xFFFFFFFF);pci_read_config_dword(dev, reg_addr, &size_low);pci_write_config_dword(dev, reg_addr, base_low);bar_cnt--;
I would prefer to move this line above to the end of the loop. This makes it clearer (IMHO).
reg_addr += 4;base = base_low & ~0xF;size = size_low & ~0xF;base_high = 0x0;size_high = 0xFFFFFFFF;is_64 = 0;if ((base_low & PCI_BASE_ADDRESS_MEM_TYPE_MASK) ==PCI_BASE_ADDRESS_MEM_TYPE_64) {pci_read_config_dword(dev, reg_addr, &base_high);pci_write_config_dword(dev, reg_addr, 0xFFFFFFFF);pci_read_config_dword(dev, reg_addr, &size_high);pci_write_config_dword(dev, reg_addr, base_high);bar_cnt--;reg_addr += 4;is_64++;
Why not simply:
is_64 = 1;
?
}base = base | ((u64)base_high << 32);size = size | ((u64)size_high << 32);size = ~size + 1;printf(" %d 0x%016llx 0x%016llx %d\n",bar_id, base, size, is_64 ? 64 : 32);
It would be good to also show if the BAR is IO or MEM space. And if its prefetachable or not.
bar_id++;- }
- return 0;
+}
static void pci_show_regs(pci_dev_t dev, struct pci_reg_info *regs) { @@ -573,6 +642,7 @@ static int do_pci(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) if (argc > 4) value = simple_strtoul(argv[4], NULL, 16); case 'h': /* header */
- case 'b': /* bars */ if (argc < 3) goto usage; if ((bdf = get_pci_dev(argv[2])) == -1)
@@ -641,6 +711,8 @@ static int do_pci(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) ret = pci_cfg_write(dev, addr, size, value); #endif break;
- case 'b': /* bars */
return pci_bar_show(bdf);@@ -663,6 +735,8 @@ static char pci_help_text[] = #endif "pci header b.d.f\n" " - show header of PCI device 'bus.device.function'\n"
- "pci bar b.d.f\n"
- " - show BARs base and size for device b.d.f'\n" "pci display[.b, .w, .l] b.d.f [address] [# of objects]\n" " - display PCI configuration space (CFG)\n" "pci next[.b, .w, .l] b.d.f address\n"
Thanks, Stefan