| Message ID | 20200722022514.1283916-18-robh@kernel.org (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Delegated to: | Lorenzo Pieralisi |
| Headers | show |
| Series | PCI: Another round of host clean-ups | expand |
Hi Rob, On Wed, Jul 22, 2020 at 4:26 AM Rob Herring <robh@kernel.org> wrote: > The rcar-gen2 host driver still uses the old Arm PCI setup function > pci_common_init_dev(). Let's update it to use the modern > devm_pci_alloc_host_bridge(), pci_parse_request_of_pci_ranges() and > pci_host_probe() functions. > > Cc: Marek Vasut <marek.vasut+renesas@gmail.com> > Cc: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > Cc: Bjorn Helgaas <bhelgaas@google.com> > Cc: linux-renesas-soc@vger.kernel.org > Signed-off-by: Rob Herring <robh@kernel.org> This is now commit 92d69cc6275845a7 ("PCI: rcar-gen2: Convert to use modern host bridge probe functions"), and causes a crash on r8a7791/koelsch: Unable to handle kernel NULL pointer dereference at virtual address 00000008 pgd = (ptrval) [00000008] *pgd=00000000 Internal error: Oops: 5 [#1] SMP ARM CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.8.0-rc1-shmobile-00035-g92d69cc6275845a7 #645 Hardware name: Generic R-Car Gen2 (Flattened Device Tree) PC is at rcar_pci_probe+0x154/0x340 LR is at _raw_spin_unlock_irqrestore+0x18/0x20 > --- a/drivers/pci/controller/pci-rcar-gen2.c > +++ b/drivers/pci/controller/pci-rcar-gen2.c > @@ -189,19 +170,33 @@ static inline void rcar_pci_setup_errirq(struct rcar_pci_priv *priv) { } > #endif > > /* PCI host controller setup */ > -static int rcar_pci_setup(int nr, struct pci_sys_data *sys) > +static void rcar_pci_setup(struct rcar_pci_priv *priv) > { > - struct rcar_pci_priv *priv = sys->private_data; > + struct pci_host_bridge *bridge = pci_host_bridge_from_priv(priv); > struct device *dev = priv->dev; > void __iomem *reg = priv->reg; > + struct resource_entry *entry; > + unsigned long window_size; > + unsigned long window_addr; > + unsigned long window_pci; > u32 val; > - int ret; > + > + entry = resource_list_first_type(&bridge->dma_ranges, IORESOURCE_MEM); bridge->dma_ranges is not initialized => BOOM. > static int rcar_pci_probe(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > struct resource *cfg_res, *mem_res; > struct rcar_pci_priv *priv; > + struct pci_host_bridge *bridge; > void __iomem *reg; > - struct hw_pci hw; > - void *hw_private[1]; > + int ret; > + > + bridge = devm_pci_alloc_host_bridge(dev, sizeof(*priv)); > + if (!bridge) > + return -ENOMEM; > + > + priv = pci_host_bridge_priv(bridge); This is the "new" priv instance. > + bridge->sysdata = priv; > > cfg_res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > reg = devm_ioremap_resource(dev, cfg_res); However, the "old" instance is still allocated below, and should be removed. I've send a patch to fix that, which should appear soon at https://lore.kernel.org/r/20200804120430.9253-1-geert+renesas@glider.be BTW, the conversion has the following impact on r8a7791/koelsch: -pci-rcar-gen2 ee090000.pci: PCI: bus0 revision 11 +pci-rcar-gen2 ee090000.pci: host bridge /soc/pci@ee090000 ranges: +pci-rcar-gen2 ee090000.pci: MEM 0x00ee080000..0x00ee08ffff -> 0x00ee080000 +pci-rcar-gen2 ee090000.pci: PCI: revision 11 pci-rcar-gen2 ee090000.pci: PCI host bridge to bus 0000:00 -pci_bus 0000:00: root bus resource [mem 0xee080000-0xee0810ff] ^^^^^^^^^^^^^^^^^^^^^ -pci_bus 0000:00: No busn resource found for root bus, will use [bus 00-ff] +pci_bus 0000:00: root bus resource [bus 00] +pci_bus 0000:00: root bus resource [mem 0xee080000-0xee08ffff] ^^^^^^^^^^^^^^^^^^^^^ pci0: pci@ee090000 { ... reg = <0 0xee090000 0 0xc00>, <0 0xee080000 0 0x1100>; ... ranges = <0x02000000 0 0xee080000 0 0xee080000 0 0x00010000>; ... usb@1,0 { reg = <0x800 0 0 0 0>; ... }; usb@2,0 { reg = <0x1000 0 0 0 0>; ... }; } The old root bus resource size was based on the "reg" property. The new root bus resource size is based on the "ranges" property. Given the only children are hardcoded, I guess that is OK? Gr{oetje,eeting}s, Geert
On Tue, Aug 4, 2020 at 6:12 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > Hi Rob, > > On Wed, Jul 22, 2020 at 4:26 AM Rob Herring <robh@kernel.org> wrote: > > The rcar-gen2 host driver still uses the old Arm PCI setup function > > pci_common_init_dev(). Let's update it to use the modern > > devm_pci_alloc_host_bridge(), pci_parse_request_of_pci_ranges() and > > pci_host_probe() functions. > > > > Cc: Marek Vasut <marek.vasut+renesas@gmail.com> > > Cc: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > > Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > > Cc: Bjorn Helgaas <bhelgaas@google.com> > > Cc: linux-renesas-soc@vger.kernel.org > > Signed-off-by: Rob Herring <robh@kernel.org> > > This is now commit 92d69cc6275845a7 ("PCI: rcar-gen2: Convert to use > modern host bridge probe functions"), and causes a crash on r8a7791/koelsch: Can't say I'm surprised, this was a big change. Thanks for testing. > Unable to handle kernel NULL pointer dereference at virtual address 00000008 > pgd = (ptrval) > [00000008] *pgd=00000000 > Internal error: Oops: 5 [#1] SMP ARM > CPU: 0 PID: 1 Comm: swapper/0 Not tainted > 5.8.0-rc1-shmobile-00035-g92d69cc6275845a7 #645 > Hardware name: Generic R-Car Gen2 (Flattened Device Tree) > PC is at rcar_pci_probe+0x154/0x340 > LR is at _raw_spin_unlock_irqrestore+0x18/0x20 > > > --- a/drivers/pci/controller/pci-rcar-gen2.c > > +++ b/drivers/pci/controller/pci-rcar-gen2.c > > @@ -189,19 +170,33 @@ static inline void rcar_pci_setup_errirq(struct rcar_pci_priv *priv) { } > > #endif > > > > /* PCI host controller setup */ > > -static int rcar_pci_setup(int nr, struct pci_sys_data *sys) > > +static void rcar_pci_setup(struct rcar_pci_priv *priv) > > { > > - struct rcar_pci_priv *priv = sys->private_data; > > + struct pci_host_bridge *bridge = pci_host_bridge_from_priv(priv); > > struct device *dev = priv->dev; > > void __iomem *reg = priv->reg; > > + struct resource_entry *entry; > > + unsigned long window_size; > > + unsigned long window_addr; > > + unsigned long window_pci; > > u32 val; > > - int ret; > > + > > + entry = resource_list_first_type(&bridge->dma_ranges, IORESOURCE_MEM); > > bridge->dma_ranges is not initialized => BOOM. > > > static int rcar_pci_probe(struct platform_device *pdev) > > { > > struct device *dev = &pdev->dev; > > struct resource *cfg_res, *mem_res; > > struct rcar_pci_priv *priv; > > + struct pci_host_bridge *bridge; > > void __iomem *reg; > > - struct hw_pci hw; > > - void *hw_private[1]; > > + int ret; > > + > > + bridge = devm_pci_alloc_host_bridge(dev, sizeof(*priv)); > > + if (!bridge) > > + return -ENOMEM; > > + > > + priv = pci_host_bridge_priv(bridge); > > This is the "new" priv instance. > > > + bridge->sysdata = priv; > > > > cfg_res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > reg = devm_ioremap_resource(dev, cfg_res); > > However, the "old" instance is still allocated below, and should be removed. > > I've send a patch to fix that, which should appear soon at > https://lore.kernel.org/r/20200804120430.9253-1-geert+renesas@glider.be > > BTW, the conversion has the following impact on r8a7791/koelsch: > > -pci-rcar-gen2 ee090000.pci: PCI: bus0 revision 11 > +pci-rcar-gen2 ee090000.pci: host bridge /soc/pci@ee090000 ranges: > +pci-rcar-gen2 ee090000.pci: MEM 0x00ee080000..0x00ee08ffff > -> 0x00ee080000 > +pci-rcar-gen2 ee090000.pci: PCI: revision 11 > pci-rcar-gen2 ee090000.pci: PCI host bridge to bus 0000:00 > -pci_bus 0000:00: root bus resource [mem 0xee080000-0xee0810ff] > ^^^^^^^^^^^^^^^^^^^^^ > -pci_bus 0000:00: No busn resource found for root bus, will use [bus 00-ff] > +pci_bus 0000:00: root bus resource [bus 00] > +pci_bus 0000:00: root bus resource [mem 0xee080000-0xee08ffff] > ^^^^^^^^^^^^^^^^^^^^^ > > pci0: pci@ee090000 { > ... > reg = <0 0xee090000 0 0xc00>, > <0 0xee080000 0 0x1100>; > ... > ranges = <0x02000000 0 0xee080000 0 0xee080000 0 0x00010000>; > ... > usb@1,0 { > reg = <0x800 0 0 0 0>; > ... > }; > > usb@2,0 { > reg = <0x1000 0 0 0 0>; > ... > }; > > } > > The old root bus resource size was based on the "reg" property. > The new root bus resource size is based on the "ranges" property. That was wrong to have PCI memory space in 'reg', but the driver could always adjust the size to 0x1100 if needed. BTW, the binding description seems to have the 'reg' description reversed. > Given the only children are hardcoded, I guess that is OK? In the sense that those are the only 2 devices and you know their memory fits, yes. However, the memory space itself isn't hardcoded. If you wanted to do that, then the children really need 'assigned-addresses' properties. I guess it happens to work because memory space is allocated from the start and goes in order of device addresses. But if that changed to top down allocation? Rob
diff --git a/drivers/pci/controller/pci-rcar-gen2.c b/drivers/pci/controller/pci-rcar-gen2.c index 326171cb1a97..5b9888d4c34a 100644 --- a/drivers/pci/controller/pci-rcar-gen2.c +++ b/drivers/pci/controller/pci-rcar-gen2.c @@ -98,22 +98,17 @@ struct rcar_pci_priv { void __iomem *reg; struct resource mem_res; struct resource *cfg_res; - unsigned busnr; int irq; - unsigned long window_size; - unsigned long window_addr; - unsigned long window_pci; }; /* PCI configuration space operations */ static void __iomem *rcar_pci_cfg_base(struct pci_bus *bus, unsigned int devfn, int where) { - struct pci_sys_data *sys = bus->sysdata; - struct rcar_pci_priv *priv = sys->private_data; + struct rcar_pci_priv *priv = bus->sysdata; int slot, val; - if (sys->busnr != bus->number || PCI_FUNC(devfn)) + if (!pci_is_root_bus(bus) || PCI_FUNC(devfn)) return NULL; /* Only one EHCI/OHCI device built-in */ @@ -132,20 +127,6 @@ static void __iomem *rcar_pci_cfg_base(struct pci_bus *bus, unsigned int devfn, return priv->reg + (slot >> 1) * 0x100 + where; } -/* PCI interrupt mapping */ -static int rcar_pci_map_irq(const struct pci_dev *dev, u8 slot, u8 pin) -{ - struct pci_sys_data *sys = dev->bus->sysdata; - struct rcar_pci_priv *priv = sys->private_data; - int irq; - - irq = of_irq_parse_and_map_pci(dev, slot, pin); - if (!irq) - irq = priv->irq; - - return irq; -} - #ifdef CONFIG_PCI_DEBUG /* if debug enabled, then attach an error handler irq to the bridge */ @@ -189,19 +170,33 @@ static inline void rcar_pci_setup_errirq(struct rcar_pci_priv *priv) { } #endif /* PCI host controller setup */ -static int rcar_pci_setup(int nr, struct pci_sys_data *sys) +static void rcar_pci_setup(struct rcar_pci_priv *priv) { - struct rcar_pci_priv *priv = sys->private_data; + struct pci_host_bridge *bridge = pci_host_bridge_from_priv(priv); struct device *dev = priv->dev; void __iomem *reg = priv->reg; + struct resource_entry *entry; + unsigned long window_size; + unsigned long window_addr; + unsigned long window_pci; u32 val; - int ret; + + entry = resource_list_first_type(&bridge->dma_ranges, IORESOURCE_MEM); + if (!entry) { + window_addr = 0x40000000; + window_pci = 0x40000000; + window_size = SZ_1G; + } else { + window_addr = entry->res->start; + window_pci = entry->res->start - entry->offset; + window_size = resource_size(entry->res); + } pm_runtime_enable(dev); pm_runtime_get_sync(dev); val = ioread32(reg + RCAR_PCI_UNIT_REV_REG); - dev_info(dev, "PCI: bus%u revision %x\n", sys->busnr, val); + dev_info(dev, "PCI: revision %x\n", val); /* Disable Direct Power Down State and assert reset */ val = ioread32(reg + RCAR_USBCTR_REG) & ~RCAR_USBCTR_DIRPD; @@ -214,7 +209,7 @@ static int rcar_pci_setup(int nr, struct pci_sys_data *sys) RCAR_USBCTR_USBH_RST | RCAR_USBCTR_PLL_RST); /* Setup PCIAHB window1 size */ - switch (priv->window_size) { + switch (window_size) { case SZ_2G: val |= RCAR_USBCTR_PCIAHB_WIN1_2G; break; @@ -226,8 +221,8 @@ static int rcar_pci_setup(int nr, struct pci_sys_data *sys) break; default: pr_warn("unknown window size %ld - defaulting to 256M\n", - priv->window_size); - priv->window_size = SZ_256M; + window_size); + window_size = SZ_256M; /* fall-through */ case SZ_256M: val |= RCAR_USBCTR_PCIAHB_WIN1_256M; @@ -245,7 +240,7 @@ static int rcar_pci_setup(int nr, struct pci_sys_data *sys) iowrite32(val, reg + RCAR_PCI_ARBITER_CTR_REG); /* PCI-AHB mapping */ - iowrite32(priv->window_addr | RCAR_PCIAHB_PREFETCH16, + iowrite32(window_addr | RCAR_PCIAHB_PREFETCH16, reg + RCAR_PCIAHB_WIN1_CTR_REG); /* AHB-PCI mapping: OHCI/EHCI registers */ @@ -256,7 +251,7 @@ static int rcar_pci_setup(int nr, struct pci_sys_data *sys) iowrite32(RCAR_AHBPCI_WIN1_HOST | RCAR_AHBPCI_WIN_CTR_CFG, reg + RCAR_AHBPCI_WIN1_CTR_REG); /* Set PCI-AHB Window1 address */ - iowrite32(priv->window_pci | PCI_BASE_ADDRESS_MEM_PREFETCH, + iowrite32(window_pci | PCI_BASE_ADDRESS_MEM_PREFETCH, reg + PCI_BASE_ADDRESS_1); /* Set AHB-PCI bridge PCI communication area address */ val = priv->cfg_res->start + RCAR_AHBPCI_PCICOM_OFFSET; @@ -271,18 +266,7 @@ static int rcar_pci_setup(int nr, struct pci_sys_data *sys) iowrite32(RCAR_PCI_INT_A | RCAR_PCI_INT_B | RCAR_PCI_INT_PME, reg + RCAR_PCI_INT_ENABLE_REG); - if (priv->irq > 0) - rcar_pci_setup_errirq(priv); - - /* Add PCI resources */ - pci_add_resource(&sys->resources, &priv->mem_res); - ret = devm_request_pci_bus_resources(dev, &sys->resources); - if (ret < 0) - return ret; - - /* Setup bus number based on platform device id / of bus-range */ - sys->busnr = priv->busnr; - return 1; + rcar_pci_setup_errirq(priv); } static struct pci_ops rcar_pci_ops = { @@ -291,55 +275,21 @@ static struct pci_ops rcar_pci_ops = { .write = pci_generic_config_write, }; -static int rcar_pci_parse_map_dma_ranges(struct rcar_pci_priv *pci, - struct device_node *np) -{ - struct device *dev = pci->dev; - struct of_pci_range range; - struct of_pci_range_parser parser; - int index = 0; - - /* Failure to parse is ok as we fall back to defaults */ - if (of_pci_dma_range_parser_init(&parser, np)) - return 0; - - /* Get the dma-ranges from DT */ - for_each_of_pci_range(&parser, &range) { - /* Hardware only allows one inbound 32-bit range */ - if (index) - return -EINVAL; - - pci->window_addr = (unsigned long)range.cpu_addr; - pci->window_pci = (unsigned long)range.pci_addr; - pci->window_size = (unsigned long)range.size; - - /* Catch HW limitations */ - if (!(range.flags & IORESOURCE_PREFETCH)) { - dev_err(dev, "window must be prefetchable\n"); - return -EINVAL; - } - if (pci->window_addr) { - u32 lowaddr = 1 << (ffs(pci->window_addr) - 1); - - if (lowaddr < pci->window_size) { - dev_err(dev, "invalid window size/addr\n"); - return -EINVAL; - } - } - index++; - } - - return 0; -} - static int rcar_pci_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; struct resource *cfg_res, *mem_res; struct rcar_pci_priv *priv; + struct pci_host_bridge *bridge; void __iomem *reg; - struct hw_pci hw; - void *hw_private[1]; + int ret; + + bridge = devm_pci_alloc_host_bridge(dev, sizeof(*priv)); + if (!bridge) + return -ENOMEM; + + priv = pci_host_bridge_priv(bridge); + bridge->sysdata = priv; cfg_res = platform_get_resource(pdev, IORESOURCE_MEM, 0); reg = devm_ioremap_resource(dev, cfg_res); @@ -369,44 +319,18 @@ static int rcar_pci_probe(struct platform_device *pdev) return priv->irq; } - /* default window addr and size if not specified in DT */ - priv->window_addr = 0x40000000; - priv->window_pci = 0x40000000; - priv->window_size = SZ_1G; - - if (dev->of_node) { - struct resource busnr; - int ret; - - ret = of_pci_parse_bus_range(dev->of_node, &busnr); - if (ret < 0) { - dev_err(dev, "failed to parse bus-range\n"); - return ret; - } - - priv->busnr = busnr.start; - if (busnr.end != busnr.start) - dev_warn(dev, "only one bus number supported\n"); - - ret = rcar_pci_parse_map_dma_ranges(priv, dev->of_node); - if (ret < 0) { - dev_err(dev, "failed to parse dma-range\n"); - return ret; - } - } else { - priv->busnr = pdev->id; - } + ret = pci_parse_request_of_pci_ranges(dev, &bridge->windows, + &bridge->dma_ranges, NULL); + if (ret) + return ret; + + bridge->ops = &rcar_pci_ops; + + pci_add_flags(PCI_REASSIGN_ALL_BUS); + + rcar_pci_setup(priv); - hw_private[0] = priv; - memset(&hw, 0, sizeof(hw)); - hw.nr_controllers = ARRAY_SIZE(hw_private); - hw.io_optional = 1; - hw.private_data = hw_private; - hw.map_irq = rcar_pci_map_irq; - hw.ops = &rcar_pci_ops; - hw.setup = rcar_pci_setup; - pci_common_init_dev(dev, &hw); - return 0; + return pci_host_probe(bridge); } static const struct of_device_id rcar_pci_of_match[] = {
The rcar-gen2 host driver still uses the old Arm PCI setup function pci_common_init_dev(). Let's update it to use the modern devm_pci_alloc_host_bridge(), pci_parse_request_of_pci_ranges() and pci_host_probe() functions. Cc: Marek Vasut <marek.vasut+renesas@gmail.com> Cc: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> Cc: Bjorn Helgaas <bhelgaas@google.com> Cc: linux-renesas-soc@vger.kernel.org Signed-off-by: Rob Herring <robh@kernel.org> --- drivers/pci/controller/pci-rcar-gen2.c | 168 +++++++------------------ 1 file changed, 46 insertions(+), 122 deletions(-)