Message ID | 1478442662-21577-2-git-send-email-yehuday@marvell.com |
---|---|
State | Changes Requested |
Delegated to: | Simon Glass |
Headers | show |
On 06.11.2016 15:31, 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've tested this patch on a DM based PCI driver. And it looks good so far: => pci bar 0.0.0 ID Base Size Width Type ---------------------------------------------------------- 0 0x00000000f9000000 0x0000000000100000 32 MEM => pci bar 0.0.1 No such device => pci bar 1.0.0 ID Base Size Width Type ---------------------------------------------------------- 0 0x00000000f8000000 0x0000000000020000 32 MEM 1 0x00000000f8020000 0x0000000000020000 32 MEM 2 0x00000000ffffffe0 0x0000000000000020 32 I/O So: Tested-by: Stefan Roese <sr@denx.de> Thanks, Stefan
Hi, On 6 November 2016 at 07:31, <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> > --- > cmd/pci.c | 95 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 95 insertions(+) Reviewed-by: Simon Glass <sjg@chromium.org> > > diff --git a/cmd/pci.c b/cmd/pci.c > index 2f4978a..51eb536 100644 > --- a/cmd/pci.c > +++ b/cmd/pci.c > @@ -92,6 +92,96 @@ static void pci_show_regs(pci_dev_t dev, struct pci_reg_info *regs) > } > #endif > > +#ifdef CONFIG_DM_PCI > +int pci_bar_show(struct udevice *dev) > +#else > +int pci_bar_show(pci_dev_t dev) > +#endif > +{ > + u8 header_type; > + int bar_cnt, bar_id, is_64, is_io, mem_type; > + u32 base_low, base_high; > + u32 size_low, size_high; > + u64 base, size; > + u32 reg_addr; > + int prefetchable; > + > +#ifdef CONFIG_DM_PCI I think you can implement only the DM_PCI case, since we are trying to move everything to DM_PCI. > + dm_pci_read_config8(dev, PCI_HEADER_TYPE, &header_type); > +#else > + pci_read_config_byte(dev, PCI_HEADER_TYPE, &header_type); > +#endif > + > + if (header_type == PCI_HEADER_TYPE_CARDBUS) { > + printf("CardBus doesn't support BARs\n"); > + return -1; -ENOSYS perhaps. This is -EPERM which seems wrong. > + } > + > + bar_cnt = (header_type == PCI_HEADER_TYPE_NORMAL) ? 6 : 2; > + > + printf("ID Base Size Width Type\n"); > + printf("----------------------------------------------------------\n"); > + > + bar_id = 0; > + reg_addr = PCI_BASE_ADDRESS_0; > + while (bar_cnt) { > +#ifdef CONFIG_DM_PCI > + dm_pci_read_config32(dev, reg_addr, &base_low); > + dm_pci_write_config32(dev, reg_addr, 0xFFFFFFFF); Suggest lower-case hex. > + dm_pci_read_config32(dev, reg_addr, &size_low); > + dm_pci_write_config32(dev, reg_addr, base_low); > +#else > + 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); > +#endif > + reg_addr += 4; > + > + base = base_low & ~0xF; > + size = size_low & ~0xF; > + base_high = 0x0; > + size_high = 0xFFFFFFFF; > + is_64 = 0; > + prefetchable = base_low & PCI_BASE_ADDRESS_MEM_PREFETCH; > + is_io = base_low & PCI_BASE_ADDRESS_SPACE_IO; > + mem_type = base_low & PCI_BASE_ADDRESS_MEM_TYPE_MASK; > + > + if (mem_type == PCI_BASE_ADDRESS_MEM_TYPE_64) { > +#ifdef CONFIG_DM_PCI > + dm_pci_read_config32(dev, reg_addr, &base_high); > + dm_pci_write_config32(dev, reg_addr, 0xFFFFFFFF); > + dm_pci_read_config32(dev, reg_addr, &size_high); > + dm_pci_write_config32(dev, reg_addr, base_high); > +#else > + 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); > +#endif > + bar_cnt--; > + reg_addr += 4; > + is_64 = 1; > + } > + > + base = base | ((u64)base_high << 32); > + size = size | ((u64)size_high << 32); > + > + if ((!is_64 && size_low) || (is_64 && size)) { > + size = ~size + 1; > + printf(" %d 0x%016llx 0x%016llx %d %s %s\n", Can we drop the the address values? It is implied in U-Boot. If not, let's use %#x. > + bar_id, base, size, is_64 ? 64 : 32, > + is_io ? "I/O" : "MEM", > + prefetchable ? "Prefetchable" : ""); Check with sandbox, this gives a warning: cmd/pci.c: In function ‘pci_bar_show’: cmd/pci.c:175:11: warning: format ‘%llx’ expects argument of type ‘long long unsigned int’, but argument 3 has type ‘u64’ [-Wformat=] prefetchable ? "Prefetchable" : ""); ^ cmd/pci.c:175:11: warning: format ‘%llx’ expects argument of type ‘long long unsigned int’, but argument 4 has type ‘u64’ [-Wformat=] > + } > + > + bar_id++; > + bar_cnt--; > + } > + > + return 0; > +} > + > static struct pci_reg_info regs_start[] = { > { "vendor ID", PCI_SIZE_16, PCI_VENDOR_ID }, > { "device ID", PCI_SIZE_16, PCI_DEVICE_ID }, > @@ -573,6 +663,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 +732,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(dev); > default: > ret = CMD_RET_USAGE; > break; > @@ -663,6 +756,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" > -- > 1.9.1 > Regards, Simon
Hi Simon > -----Original Message----- > From: sjg@google.com [mailto:sjg@google.com] On Behalf Of Simon Glass > Sent: Friday, November 11, 2016 18:17 > To: Yehuda Yitschak > Cc: Bin Meng; Heiko Schocher; Przemyslaw Marczak; Stefan Roese; Stephen > Warren; U-Boot Mailing List > Subject: Re: [PATCH v2 1/1] cmd: pci: add option to parse and display BAR > information > > Hi, > > On 6 November 2016 at 07:31, <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> > > --- > > cmd/pci.c | 95 > > > ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > +++++ > > 1 file changed, 95 insertions(+) > > Reviewed-by: Simon Glass <sjg@chromium.org> > > > > > diff --git a/cmd/pci.c b/cmd/pci.c > > index 2f4978a..51eb536 100644 > > --- a/cmd/pci.c > > +++ b/cmd/pci.c > > @@ -92,6 +92,96 @@ static void pci_show_regs(pci_dev_t dev, struct > > pci_reg_info *regs) } #endif > > > > +#ifdef CONFIG_DM_PCI > > +int pci_bar_show(struct udevice *dev) #else int > > +pci_bar_show(pci_dev_t dev) #endif { > > + u8 header_type; > > + int bar_cnt, bar_id, is_64, is_io, mem_type; > > + u32 base_low, base_high; > > + u32 size_low, size_high; > > + u64 base, size; > > + u32 reg_addr; > > + int prefetchable; > > + > > +#ifdef CONFIG_DM_PCI > > I think you can implement only the DM_PCI case, since we are trying to move > everything to DM_PCI. Cool. Less clutter > > > + dm_pci_read_config8(dev, PCI_HEADER_TYPE, &header_type); #else > > + pci_read_config_byte(dev, PCI_HEADER_TYPE, &header_type); > > +#endif > > + > > + if (header_type == PCI_HEADER_TYPE_CARDBUS) { > > + printf("CardBus doesn't support BARs\n"); > > + return -1; > > -ENOSYS perhaps. This is -EPERM which seems wrong. > > > + } > > + > > + bar_cnt = (header_type == PCI_HEADER_TYPE_NORMAL) ? 6 : 2; > > + > > + printf("ID Base Size Width Type\n"); > > + > > + printf("----------------------------------------------------------\n > > + "); > > + > > + bar_id = 0; > > + reg_addr = PCI_BASE_ADDRESS_0; > > + while (bar_cnt) { > > +#ifdef CONFIG_DM_PCI > > + dm_pci_read_config32(dev, reg_addr, &base_low); > > + dm_pci_write_config32(dev, reg_addr, 0xFFFFFFFF); > > Suggest lower-case hex. > > > + dm_pci_read_config32(dev, reg_addr, &size_low); > > + dm_pci_write_config32(dev, reg_addr, base_low); #else > > + 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); > > +#endif > > + reg_addr += 4; > > + > > + base = base_low & ~0xF; > > + size = size_low & ~0xF; > > + base_high = 0x0; > > + size_high = 0xFFFFFFFF; > > + is_64 = 0; > > + prefetchable = base_low & PCI_BASE_ADDRESS_MEM_PREFETCH; > > + is_io = base_low & PCI_BASE_ADDRESS_SPACE_IO; > > + mem_type = base_low & > PCI_BASE_ADDRESS_MEM_TYPE_MASK; > > + > > + if (mem_type == PCI_BASE_ADDRESS_MEM_TYPE_64) { #ifdef > > +CONFIG_DM_PCI > > + dm_pci_read_config32(dev, reg_addr, &base_high); > > + dm_pci_write_config32(dev, reg_addr, 0xFFFFFFFF); > > + dm_pci_read_config32(dev, reg_addr, &size_high); > > + dm_pci_write_config32(dev, reg_addr, > > +base_high); #else > > + 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); #endif > > + bar_cnt--; > > + reg_addr += 4; > > + is_64 = 1; > > + } > > + > > + base = base | ((u64)base_high << 32); > > + size = size | ((u64)size_high << 32); > > + > > + if ((!is_64 && size_low) || (is_64 && size)) { > > + size = ~size + 1; > > + printf(" %d 0x%016llx 0x%016llx %d %s %s\n", > > Can we drop the the address values? It is implied in U-Boot. If not, let's use > %#x. I prefer to keep them. It makes debugging much simpler I will change the format to %#016llx. > > > + bar_id, base, size, is_64 ? 64 : 32, > > + is_io ? "I/O" : "MEM", > > + prefetchable ? "Prefetchable" : ""); > > Check with sandbox, this gives a warning: > > cmd/pci.c: In function ‘pci_bar_show’: > cmd/pci.c:175:11: warning: format ‘%llx’ expects argument of type ‘long long > unsigned int’, but argument 3 has type ‘u64’ [-Wformat=] > prefetchable ? "Prefetchable" : ""); > ^ Strange, I can't see that. What compiler are you using when you get the warning ? I am using gcc-4.8 for armv8, maybe that's why I don't see the warnings I might come down to the built-in definition of __UINT64_TYPE__ which the sandbox arch uses > cmd/pci.c:175:11: warning: format ‘%llx’ expects argument of type ‘long long > unsigned int’, but argument 4 has type ‘u64’ [-Wformat=] > > > + } > > + > > + bar_id++; > > + bar_cnt--; > > + } > > + > > + return 0; > > +} > > + > > static struct pci_reg_info regs_start[] = { > > { "vendor ID", PCI_SIZE_16, PCI_VENDOR_ID }, > > { "device ID", PCI_SIZE_16, PCI_DEVICE_ID }, @@ -573,6 +663,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 > > +732,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(dev); > > default: > > ret = CMD_RET_USAGE; > > break; > > @@ -663,6 +756,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" > > -- > > 1.9.1 > > > > Regards, > Simon
Hi, On 22 November 2016 at 03:49, Yehuda Yitschak <yehuday@marvell.com> wrote: > Hi Simon > >> -----Original Message----- >> From: sjg@google.com [mailto:sjg@google.com] On Behalf Of Simon Glass >> Sent: Friday, November 11, 2016 18:17 >> To: Yehuda Yitschak >> Cc: Bin Meng; Heiko Schocher; Przemyslaw Marczak; Stefan Roese; Stephen >> Warren; U-Boot Mailing List >> Subject: Re: [PATCH v2 1/1] cmd: pci: add option to parse and display BAR >> information >> >> Hi, >> >> On 6 November 2016 at 07:31, <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> >> > --- >> > cmd/pci.c | 95 >> > >> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ >> +++++ >> > 1 file changed, 95 insertions(+) >> >> Reviewed-by: Simon Glass <sjg@chromium.org> [...] >> >> > + bar_id, base, size, is_64 ? 64 : 32, >> > + is_io ? "I/O" : "MEM", >> > + prefetchable ? "Prefetchable" : ""); >> >> Check with sandbox, this gives a warning: >> >> cmd/pci.c: In function ‘pci_bar_show’: >> cmd/pci.c:175:11: warning: format ‘%llx’ expects argument of type ‘long long >> unsigned int’, but argument 3 has type ‘u64’ [-Wformat=] >> prefetchable ? "Prefetchable" : ""); >> ^ > > Strange, I can't see that. > What compiler are you using when you get the warning ? > I am using gcc-4.8 for armv8, maybe that's why I don't see the warnings > I might come down to the built-in definition of __UINT64_TYPE__ which the sandbox arch uses This is sandbox, perhaps this: gcc version 4.8.4 (Ubuntu 4.8.4-2ubuntu1~14.04.3) [...] Regards, Simon
diff --git a/cmd/pci.c b/cmd/pci.c index 2f4978a..51eb536 100644 --- a/cmd/pci.c +++ b/cmd/pci.c @@ -92,6 +92,96 @@ static void pci_show_regs(pci_dev_t dev, struct pci_reg_info *regs) } #endif +#ifdef CONFIG_DM_PCI +int pci_bar_show(struct udevice *dev) +#else +int pci_bar_show(pci_dev_t dev) +#endif +{ + u8 header_type; + int bar_cnt, bar_id, is_64, is_io, mem_type; + u32 base_low, base_high; + u32 size_low, size_high; + u64 base, size; + u32 reg_addr; + int prefetchable; + +#ifdef CONFIG_DM_PCI + dm_pci_read_config8(dev, PCI_HEADER_TYPE, &header_type); +#else + pci_read_config_byte(dev, PCI_HEADER_TYPE, &header_type); +#endif + + 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 Width Type\n"); + printf("----------------------------------------------------------\n"); + + bar_id = 0; + reg_addr = PCI_BASE_ADDRESS_0; + while (bar_cnt) { +#ifdef CONFIG_DM_PCI + dm_pci_read_config32(dev, reg_addr, &base_low); + dm_pci_write_config32(dev, reg_addr, 0xFFFFFFFF); + dm_pci_read_config32(dev, reg_addr, &size_low); + dm_pci_write_config32(dev, reg_addr, base_low); +#else + 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); +#endif + reg_addr += 4; + + base = base_low & ~0xF; + size = size_low & ~0xF; + base_high = 0x0; + size_high = 0xFFFFFFFF; + is_64 = 0; + prefetchable = base_low & PCI_BASE_ADDRESS_MEM_PREFETCH; + is_io = base_low & PCI_BASE_ADDRESS_SPACE_IO; + mem_type = base_low & PCI_BASE_ADDRESS_MEM_TYPE_MASK; + + if (mem_type == PCI_BASE_ADDRESS_MEM_TYPE_64) { +#ifdef CONFIG_DM_PCI + dm_pci_read_config32(dev, reg_addr, &base_high); + dm_pci_write_config32(dev, reg_addr, 0xFFFFFFFF); + dm_pci_read_config32(dev, reg_addr, &size_high); + dm_pci_write_config32(dev, reg_addr, base_high); +#else + 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); +#endif + bar_cnt--; + reg_addr += 4; + is_64 = 1; + } + + base = base | ((u64)base_high << 32); + size = size | ((u64)size_high << 32); + + if ((!is_64 && size_low) || (is_64 && size)) { + size = ~size + 1; + printf(" %d 0x%016llx 0x%016llx %d %s %s\n", + bar_id, base, size, is_64 ? 64 : 32, + is_io ? "I/O" : "MEM", + prefetchable ? "Prefetchable" : ""); + } + + bar_id++; + bar_cnt--; + } + + return 0; +} + static struct pci_reg_info regs_start[] = { { "vendor ID", PCI_SIZE_16, PCI_VENDOR_ID }, { "device ID", PCI_SIZE_16, PCI_DEVICE_ID }, @@ -573,6 +663,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 +732,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(dev); default: ret = CMD_RET_USAGE; break; @@ -663,6 +756,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"