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

[Bin Meng]

+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 at 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 at konsulko.com>
>

Thanks for the review!

>> 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;

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