From mboxrd@z Thu Jan 1 00:00:00 1970 From: Bin Meng Date: Sun, 21 Jun 2015 15:21:49 +0800 Subject: [U-Boot] [PATCH] pci: Configure expansion ROM during auto config process In-Reply-To: <20150619174040.GA21488@beef> References: <20150619174040.GA21488@beef> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de +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 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 > Thanks for the review! >> Signed-off-by: Bin Meng >> --- >> >> 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