[U-Boot] [PATCH v2 1/1] cmd: pci: add option to parse and display BAR information
Yehuda Yitschak
yehuday at marvell.com
Tue Nov 22 11:49:10 CET 2016
Hi Simon
> -----Original Message-----
> From: sjg at google.com [mailto:sjg at 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 at marvell.com> wrote:
> > From: Yehuda Yitschak <yehuday at 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 at marvell.com>
> > ---
> > cmd/pci.c | 95
> >
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> +++++
> > 1 file changed, 95 insertions(+)
>
> Reviewed-by: Simon Glass <sjg at 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
More information about the U-Boot
mailing list