[U-Boot] [PATCH] cmd: pci: add option to parse and display BAR information

[Stefan Roese]

Hi Yehuda,

On 27.10.2016 10:42, 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>

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);
>  	default:
>  		ret = CMD_RET_USAGE;
>  		break;
> @@ -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