* [U-Boot] [PATCH] pci: Configure expansion ROM during auto config process
@ 2015-06-19 8:15 Bin Meng
2015-06-19 17:40 ` Matt Porter
0 siblings, 1 reply; 5+ messages in thread
From: Bin Meng @ 2015-06-19 8:15 UTC (permalink / raw)
To: u-boot
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(-)
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
--
1.8.2.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [U-Boot] [PATCH] pci: Configure expansion ROM during auto config process
2015-06-19 8:15 [U-Boot] [PATCH] pci: Configure expansion ROM during auto config process Bin Meng
@ 2015-06-19 17:40 ` Matt Porter
2015-06-21 7:21 ` Bin Meng
0 siblings, 1 reply; 5+ messages in thread
From: Matt Porter @ 2015-06-19 17:40 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* [U-Boot] [PATCH] pci: Configure expansion ROM during auto config process
2015-06-19 17:40 ` Matt Porter
@ 2015-06-21 7:21 ` Bin Meng
2015-06-21 18:16 ` Tom Rini
0 siblings, 1 reply; 5+ messages in thread
From: Bin Meng @ 2015-06-21 7:21 UTC (permalink / raw)
To: u-boot
+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
^ permalink raw reply [flat|nested] 5+ messages in thread
* [U-Boot] [PATCH] pci: Configure expansion ROM during auto config process
2015-06-21 7:21 ` Bin Meng
@ 2015-06-21 18:16 ` Tom Rini
2015-06-22 6:49 ` Bin Meng
0 siblings, 1 reply; 5+ messages in thread
From: Tom Rini @ 2015-06-21 18:16 UTC (permalink / raw)
To: u-boot
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!
--
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20150621/31dbd14c/attachment.sig>
^ permalink raw reply [flat|nested] 5+ messages in thread
* [U-Boot] [PATCH] pci: Configure expansion ROM during auto config process
2015-06-21 18:16 ` Tom Rini
@ 2015-06-22 6:49 ` Bin Meng
0 siblings, 0 replies; 5+ messages in thread
From: Bin Meng @ 2015-06-22 6:49 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2015-06-22 6:49 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-06-19 8:15 [U-Boot] [PATCH] pci: Configure expansion ROM during auto config process Bin Meng
2015-06-19 17:40 ` Matt Porter
2015-06-21 7:21 ` Bin Meng
2015-06-21 18:16 ` Tom Rini
2015-06-22 6:49 ` Bin Meng
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.