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

[Bin Meng]

Hi Tom,

On Mon, Jun 22, 2015 at 2:16 AM, Tom Rini <trini at 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 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.
>
> 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