+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

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