[U-Boot] [PATCH] pci: Configure expansion ROM during auto config process

[Matt Porter]

On Fri, Jun 19, 2015 at 04:15:04PM +0800, Bin Meng wrote:
> Currently PCI expansion ROM address is assigned by a call to
> pciauto_setup_rom() outside of the pci auto config process.
> This does not work when expansion ROM is on a device behind
> PCI bridge where bridge's memory limit register was already
> programmed to a value that does not cover the newly assigned
> expansion ROM address. To fix this, we should configure the
> ROM address during the auto config process.

Definitely the correct approach for the reason mentioned. There's
an issue though with the behavior of the existing expansion ROM
probe code that should be mentioned, see below.

Otherwise, looks good.

Reviewed-by: Matt Porter <mporter at konsulko.com>

> Signed-off-by: Bin Meng <bmeng.cn at gmail.com>
> ---
> 
>  drivers/pci/pci_auto.c | 40 ++++++++++++++--------------------------
>  drivers/pci/pci_rom.c  |  5 -----
>  include/pci.h          |  9 ---------
>  3 files changed, 14 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/pci/pci_auto.c b/drivers/pci/pci_auto.c
> index 7c10983..92b4933 100644
> --- a/drivers/pci/pci_auto.c
> +++ b/drivers/pci/pci_auto.c
> @@ -182,36 +182,24 @@ void pciauto_setup_device(struct pci_controller *hose,
>  		bar_nr++;
>  	}
>  
> -	pci_hose_write_config_word(hose, dev, PCI_COMMAND, cmdstat);
> -	pci_hose_write_config_byte(hose, dev, PCI_CACHE_LINE_SIZE,
> -		CONFIG_SYS_PCI_CACHE_LINE_SIZE);
> -	pci_hose_write_config_byte(hose, dev, PCI_LATENCY_TIMER, 0x80);
> -}
> -
> -int pciauto_setup_rom(struct pci_controller *hose, pci_dev_t dev)
> -{
> -	pci_addr_t bar_value;
> -	pci_size_t bar_size;
> -	u32 bar_response;
> -	u16 cmdstat = 0;
> -
> +	/* Configure the expansion ROM address */
>  	pci_hose_write_config_dword(hose, dev, PCI_ROM_ADDRESS, 0xfffffffe);
>  	pci_hose_read_config_dword(hose, dev, PCI_ROM_ADDRESS, &bar_response);
> -	if (!bar_response)
> -		return -ENOENT;
> -
> -	bar_size = -(bar_response & ~1);
> -	DEBUGF("PCI Autoconfig: ROM, size=%#x, ", bar_size);
> -	if (pciauto_region_allocate(hose->pci_mem, bar_size, &bar_value) == 0) {
> -		pci_hose_write_config_dword(hose, dev, PCI_ROM_ADDRESS,
> -					    bar_value);
> +	if (bar_response) {
> +		bar_size = -(bar_response & ~1);
> +		DEBUGF("PCI Autoconfig: ROM, size=%#x, ", bar_size);
> +		if (pciauto_region_allocate(mem, bar_size, &bar_value) == 0) {
> +			pci_hose_write_config_dword(hose, dev, PCI_ROM_ADDRESS,
> +						    bar_value);
> +		}
> +		cmdstat |= PCI_COMMAND_MEMORY;

> +		DEBUGF("\n");
>  	}
> -	DEBUGF("\n");
> -	pci_hose_read_config_word(hose, dev, PCI_COMMAND, &cmdstat);
> -	cmdstat |= PCI_COMMAND_IO | PCI_COMMAND_MEMORY | PCI_COMMAND_MASTER;
> -	pci_hose_write_config_word(hose, dev, PCI_COMMAND, cmdstat);
>  
> -	return 0;
> +	pci_hose_write_config_word(hose, dev, PCI_COMMAND, cmdstat);
> +	pci_hose_write_config_byte(hose, dev, PCI_CACHE_LINE_SIZE,
> +				   CONFIG_SYS_PCI_CACHE_LINE_SIZE);
> +	pci_hose_write_config_byte(hose, dev, PCI_LATENCY_TIMER, 0x80);

This is a good place to mention that there's a (IMHO) latent bug in the
existing expansion ROM support. The spec mentions that simply having
a BAR decoder active does not mean there's an expansion ROM present as
it could be depoped whether socketed (old school) or not. The
pci_rom_probe() code does properly check for the ROM header signature
after ROM address decoding is enabled but does not exhibit proper error
handling on exit. Rather than leaving the ROM expansion address active
it should disable decoding on an invalid header signature. e.g.:

	if (le16_to_cpu(rom_header->signature) != PCI_ROM_HDR) {
		printf("Incorrect expansion ROM header signature %04x, disabling\n",
	 	       le16_to_cpu(rom_header->signature));
+		/* Disable expansion ROM address decoding */
+		pci_write_config_dword(dev, PCI_ROM_ADDRESS, rom_address);
		return -EINVAL;
        }

I don't have a way to test this effectively other than by inspection but
I could send a proper patch.

-Matt