From mboxrd@z Thu Jan 1 00:00:00 1970 From: Bin Meng Date: Mon, 22 Jun 2015 14:49:55 +0800 Subject: [U-Boot] [PATCH] pci: Configure expansion ROM during auto config process In-Reply-To: <20150621181658.GT28577@bill-the-cat> References: <20150619174040.GA21488@beef> <20150621181658.GT28577@bill-the-cat> 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 Hi Tom, On Mon, Jun 22, 2015 at 2:16 AM, Tom Rini 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 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. > > 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