[PATCH 6/6] PCI: designware: Fix DT resource retrieval

[Jingoo Han]

On Wednesday, October 16, 2013 9:15 AM, Tim Harvey wrote:
> On Tue, Oct 15, 2013 at 9:06 AM, Marek Vasut <marex at denx.de> wrote:
> >
> > The resource retrieval from the DT was not done right. This resulted
> > in bogus values in the IO/MEM window configuration. I re-used the code
> > implementation from pci-tegra.c to do the resource retrieval correctly.
> >
> > This in turn resulted in very strange behavior on two different kinds
> > of setup:
> >
> > Setup #1:                             ,-------> Intel i210 (igb driver)
> >            i.MX6 root port -----> PCIe switch
> >                                       `-------> Empty slot
> >
> > Setup #2:                             ,-------> Marvell Yukon (sky2 driver)
> >            i.MX6 root port -----> PCIe switch
> >                                       `-------> Empty slot
> >
> > Both setups expressed the same behavior. The system booted and the
> > ethernet controllers were properly detected. After the controllers
> > were configured and just after they reported "Ethernet Link Up", the
> > entire system froze. It is now clear this happened because the resources
> > were incorrectly configured, which in turn means the iATU was also not
> > configured correctly.
> >
> > Signed-off-by: Marek Vasut <marex at denx.de>
> > Cc: Bjorn Helgaas <bhelgaas at google.com>
> > Cc: Frank Li <lznuaa at gmail.com>
> > Cc: Jingoo Han <jg1.han at samsung.com>
> > Cc: Mohit KUMAR <Mohit.KUMAR at st.com>
> > Cc: Pratyush Anand <pratyush.anand at st.com>
> > Cc: Richard Zhu <r65037 at freescale.com>
> > Cc: Sascha Hauer <s.hauer at pengutronix.de>
> > Cc: Sean Cross <xobs at kosagi.com>
> > Cc: Shawn Guo <shawn.guo at linaro.org>
> > Cc: Siva Reddy Kallam <siva.kallam at samsung.com>
> > Cc: Srikanth T Shivanand <ts.srikanth at samsung.com>
> > Cc: Tim Harvey <tharvey at gateworks.com>
> > Cc: Troy Kisky <troy.kisky at boundarydevices.com>
> > Cc: Yinghai Lu <yinghai at kernel.org>
> > ---
> >  drivers/pci/host/pcie-designware.c | 17 ++++++-----------
> >  1 file changed, 6 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
> > index 5d183ae..97c5951 100644
> > --- a/drivers/pci/host/pcie-designware.c
> > +++ b/drivers/pci/host/pcie-designware.c
> > @@ -370,6 +370,7 @@ int __init dw_pcie_host_init(struct pcie_port *pp)
> >         struct device_node *np = pp->dev->of_node;
> >         struct of_pci_range range;
> >         struct of_pci_range_parser parser;
> > +       struct resource res;
> >         u32 val;
> >
> >         struct irq_domain *irq_domain;
> > @@ -381,28 +382,22 @@ int __init dw_pcie_host_init(struct pcie_port *pp)
> >
> >         /* Get the I/O and memory ranges from DT */
> >         for_each_of_pci_range(&parser, &range) {
> > -               unsigned long restype = range.flags & IORESOURCE_TYPE_BITS;
> > +               of_pci_range_to_resource(&range, np, &res);
> > +               unsigned long restype = res.flags & IORESOURCE_TYPE_BITS;

This makes the following build warning.
warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]

Please modify it as below:

		unsigned long restype;

		of_pci_range_to_resource(&range, np, &res);
		restype = res.flags & IORESOURCE_TYPE_BITS;

> >                 if (restype == IORESOURCE_IO) {
> > -                       of_pci_range_to_resource(&range, np, &pp->io);
> > +                       memcpy(&pp->io, &res, sizeof(res));
> >                         pp->io.name = "I/O";
> > -                       pp->io.start = max_t(resource_size_t,
> > -                                            PCIBIOS_MIN_IO,
> > -                                            range.pci_addr + global_io_offset);
> > -                       pp->io.end = min_t(resource_size_t,
> > -                                          IO_SPACE_LIMIT,
> > -                                          range.pci_addr + range.size
> > -                                          + global_io_offset);
> 
> Nak - removing the adjustment to io.start/io.end breaks the io space
> mapping as the entire io-space allocation gets taken by the root
> complex.  This causes any device that requests io space to fail to
> obtain it (your igb driver from Setup1 apparently doesn't use io space
> where my sky2 device from Setup2 above does).
> 
> If I put back the adjustment to io.start/io.end above my sky2 device
> gets its io resource properly but still hangs the bus after a few
> interrupts.  Are you sure you don't have other changes that you
> haven't posted yet that could have addressed the hang you get when you
> bring up your igb device?
> 
> I agree that the the 'hang' we are seeing is a bus-hang caused by an
> improper iATU mapping, however I'm not convinced its a dts parsing
> issue.
> 
> Tim

+cc Kishon Vijay Abraham I, Arnd Bergmann

I agree with Tim's opinion.

Please, don't remove the adjustment to io.start/io.end mapping.
It was already confirmed by Arnd Bergmann (ARM-SoC Maintainer).

Best regards,
Jingoo Han

> 
> >                         pp->config.io_size = resource_size(&pp->io);
> >                         pp->config.io_bus_addr = range.pci_addr;
> >                 }
> >                 if (restype == IORESOURCE_MEM) {
> > -                       of_pci_range_to_resource(&range, np, &pp->mem);
> > +                       memcpy(&pp->mem, &res, sizeof(res));
> >                         pp->mem.name = "MEM";
> >                         pp->config.mem_size = resource_size(&pp->mem);
> >                         pp->config.mem_bus_addr = range.pci_addr;
> >                 }
> >                 if (restype == 0) {
> > -                       of_pci_range_to_resource(&range, np, &pp->cfg);
> > +                       memcpy(&pp->cfg, &res, sizeof(res));
> >                         pp->config.cfg0_size = resource_size(&pp->cfg)/2;
> >                         pp->config.cfg1_size = resource_size(&pp->cfg)/2;
> >                 }
> > --
> > 1.8.4.rc3
> >