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 <[email protected]> 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 <[email protected]> > > > > Thanks for the review! > > >> Signed-off-by: Bin Meng <[email protected]> > >> --- > >> > >> 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
signature.asc
Description: Digital signature
_______________________________________________ U-Boot mailing list [email protected] http://lists.denx.de/mailman/listinfo/u-boot
