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

[Tom Rini]

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