diff mbox

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

Message ID BLU436-SMTP141ECAC2F67647F02F3C751BFA40@phx.gbl
State Superseded
Delegated to: Simon Glass
Headers show

Commit Message

Bin Meng June 19, 2015, 8:15 a.m. UTC
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.

Signed-off-by: Bin Meng <bmeng.cn@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(-)

Comments

Matt Porter June 19, 2015, 5:40 p.m. UTC | #1
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@konsulko.com>

> Signed-off-by: Bin Meng <bmeng.cn@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
Bin Meng June 21, 2015, 7:21 a.m. UTC | #2
+Tom since I see this patch was assigned to Tom in the patchwork.

Hi Matt,

On Sat, Jun 20, 2015 at 1:40 AM, Matt Porter <mporter@konsulko.com> wrote:
> 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@konsulko.com>
>

Thanks for the review!

>> Signed-off-by: Bin Meng <bmeng.cn@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;

Yep, this makes sense!

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

Regards,
Bin
Tom Rini June 21, 2015, 6:16 p.m. UTC | #3
On Sun, Jun 21, 2015 at 03:21:49PM +0800, Bin Meng wrote:
> +Tom since I see this patch was assigned to Tom in the patchwork.
> 
> Hi Matt,
> 
> On Sat, Jun 20, 2015 at 1:40 AM, Matt Porter <mporter@konsulko.com> wrote:
> > 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@konsulko.com>
> >
> 
> Thanks for the review!
> 
> >> Signed-off-by: Bin Meng <bmeng.cn@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;
> 
> Yep, this makes sense!
> 
> >         }
> >
> > I don't have a way to test this effectively other than by inspection but
> > I could send a proper patch.

Bin, would you have a way to test this particular error path out?  I
know Matt and I don't have any HW that would let us.  Otherwise it still
seems safe enough to add, either just in V2 or a separate patch (I don't
have a preference either).  Thanks!
Bin Meng June 22, 2015, 6:49 a.m. UTC | #4
Hi Tom,

On Mon, Jun 22, 2015 at 2:16 AM, Tom Rini <trini@konsulko.com> wrote:
> On Sun, Jun 21, 2015 at 03:21:49PM +0800, Bin Meng wrote:
>> +Tom since I see this patch was assigned to Tom in the patchwork.
>>
>> Hi Matt,
>>
>> On Sat, Jun 20, 2015 at 1:40 AM, Matt Porter <mporter@konsulko.com> wrote:
>> > 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@konsulko.com>
>> >
>>
>> Thanks for the review!
>>
>> >> Signed-off-by: Bin Meng <bmeng.cn@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;
>>
>> Yep, this makes sense!
>>
>> >         }
>> >
>> > I don't have a way to test this effectively other than by inspection but
>> > I could send a proper patch.
>
> Bin, would you have a way to test this particular error path out?  I
> know Matt and I don't have any HW that would let us.  Otherwise it still

I will see if I can test this on Intel Crown Bay,

> seems safe enough to add, either just in V2 or a separate patch (I don't
> have a preference either).  Thanks!
>

And send a v2 patch with a separate patch for Matt's proposed fix.

Regards,
Bin
diff mbox

Patch

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);
 }
 
 void pciauto_prescan_setup_bridge(struct pci_controller *hose,
diff --git a/drivers/pci/pci_rom.c b/drivers/pci/pci_rom.c
index 37450c8..f364799 100644
--- a/drivers/pci/pci_rom.c
+++ b/drivers/pci/pci_rom.c
@@ -83,11 +83,6 @@  static int pci_rom_probe(pci_dev_t dev, uint class,
 	rom_address = CONFIG_X86_OPTION_ROM_ADDR;
 #else
 
-	if (pciauto_setup_rom(pci_bus_to_hose(PCI_BUS(dev)), dev)) {
-		debug("Cannot find option ROM\n");
-		return -ENOENT;
-	}
-
 	pci_read_config_dword(dev, PCI_ROM_ADDRESS, &rom_address);
 	if (rom_address == 0x00000000 || rom_address == 0xffffffff) {
 		debug("%s: rom_address=%x\n", __func__, rom_address);
diff --git a/include/pci.h b/include/pci.h
index 07b1e9a..966be97 100644
--- a/include/pci.h
+++ b/include/pci.h
@@ -712,15 +712,6 @@  void pci_write_bar32(struct pci_controller *hose, pci_dev_t dev, int barnum,
 u32 pci_read_bar32(struct pci_controller *hose, pci_dev_t dev, int barnum);
 
 /**
- * pciauto_setup_rom() - Set up access to a device ROM
- *
- * @hose:	PCI hose to use
- * @dev:	PCI device to adjust
- * @return 0 if done, -ve on error
- */
-int pciauto_setup_rom(struct pci_controller *hose, pci_dev_t dev);
-
-/**
  * pci_hose_find_devices() - Find devices by vendor/device ID
  *
  * @hose:	PCI hose to search