| Message ID | 20200314191232.3122290-1-marek.vasut@gmail.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Delegated to: | Lorenzo Pieralisi |
| Headers | show |
| Series | [V3] PCI: rcar: Add the suspend/resume for pcie-rcar driver | expand |
On Sat, Mar 14, 2020 at 08:12:32PM +0100, marek.vasut@gmail.com wrote: > From: Kazufumi Ikeda <kaz-ikeda@xc.jp.nec.com> > > This adds the suspend/resume supports for pcie-rcar. The resume handler > reprograms the hardware based on the software state kept in specific > device structures. Also it doesn't need to save any registers. > > Signed-off-by: Kazufumi Ikeda <kaz-ikeda@xc.jp.nec.com> > Signed-off-by: Gaku Inami <gaku.inami.xw@bp.renesas.com> > Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com> > Cc: Geert Uytterhoeven <geert+renesas@glider.be> > Cc: Phil Edworthy <phil.edworthy@renesas.com> > Cc: Simon Horman <horms+renesas@verge.net.au> > Cc: Wolfram Sang <wsa@the-dreams.de> > Cc: linux-renesas-soc@vger.kernel.org > --- > V2: - Change return type of rcar_pcie_hw_enable() to void > - Drop default: case in switch statement in rcar_pcie_hw_enable() > - Sort variables in rcar_pcie_resume() > V3: - Update on top of next-20200313 > --- > drivers/pci/controller/pcie-rcar.c | 86 +++++++++++++++++++++++++----- > 1 file changed, 74 insertions(+), 12 deletions(-) > > diff --git a/drivers/pci/controller/pcie-rcar.c b/drivers/pci/controller/pcie-rcar.c > index 759c6542c5c8..f86513638b8a 100644 > --- a/drivers/pci/controller/pcie-rcar.c > +++ b/drivers/pci/controller/pcie-rcar.c > @@ -452,6 +452,32 @@ static void rcar_pcie_force_speedup(struct rcar_pcie *pcie) > (macsr & LINK_SPEED) == LINK_SPEED_5_0GTS ? "5" : "2.5"); > } > > +static void rcar_pcie_hw_enable(struct rcar_pcie *pci) > +{ > + struct resource_entry *win; > + LIST_HEAD(res); > + int i = 0; > + > + /* Try setting 5 GT/s link speed */ > + rcar_pcie_force_speedup(pci); > + > + /* Setup PCI resources */ > + resource_list_for_each_entry(win, &pci->resources) { > + struct resource *res = win->res; > + > + if (!res->flags) > + continue; > + > + switch (resource_type(res)) { > + case IORESOURCE_IO: > + case IORESOURCE_MEM: > + rcar_pcie_setup_window(i, pci, win); > + i++; > + break; > + } > + } > +} > + > static int rcar_pcie_enable(struct rcar_pcie *pcie) > { > struct device *dev = pcie->dev; > @@ -891,11 +917,25 @@ static void rcar_pcie_unmap_msi(struct rcar_pcie *pcie) > irq_domain_remove(msi->domain); > } > > +static void rcar_pcie_hw_enable_msi(struct rcar_pcie *pcie) > +{ > + struct rcar_msi *msi = &pcie->msi; > + unsigned long base; > + > + /* setup MSI data target */ > + base = virt_to_phys((void *)msi->pages); > + > + rcar_pci_write_reg(pcie, lower_32_bits(base) | MSIFE, PCIEMSIALR); > + rcar_pci_write_reg(pcie, upper_32_bits(base), PCIEMSIAUR); > + > + /* enable all MSI interrupts */ > + rcar_pci_write_reg(pcie, 0xffffffff, PCIEMSIIER); > +} > + > static int rcar_pcie_enable_msi(struct rcar_pcie *pcie) > { > struct device *dev = pcie->dev; > struct rcar_msi *msi = &pcie->msi; > - phys_addr_t base; > int err, i; > > mutex_init(&msi->lock); > @@ -934,17 +974,7 @@ static int rcar_pcie_enable_msi(struct rcar_pcie *pcie) > > /* setup MSI data target */ > msi->pages = __get_free_pages(GFP_KERNEL, 0); > - if (!msi->pages) { > - err = -ENOMEM; > - goto err; > - } > - base = virt_to_phys((void *)msi->pages); > - > - rcar_pci_write_reg(pcie, lower_32_bits(base) | MSIFE, PCIEMSIALR); > - rcar_pci_write_reg(pcie, upper_32_bits(base), PCIEMSIAUR); > - > - /* enable all MSI interrupts */ > - rcar_pci_write_reg(pcie, 0xffffffff, PCIEMSIIER); > + rcar_pcie_hw_enable_msi(pcie); > > return 0; > > @@ -1219,6 +1249,37 @@ static int rcar_pcie_probe(struct platform_device *pdev) > return err; > } > > +static int rcar_pcie_resume(struct device *dev) > +{ > + struct rcar_pcie *pcie = dev_get_drvdata(dev); > + int (*hw_init_fn)(struct rcar_pcie *); > + unsigned int data; > + int err; > + > + err = rcar_pcie_parse_map_dma_ranges(pcie); > + if (err) > + return 0; > + > + /* Failure to get a link might just be that no cards are inserted */ > + hw_init_fn = of_device_get_match_data(dev); Hi Marek, happy to apply it as is, I was wondering if it is work taking this look-up out of the resume path, it is not supposed to change anyway, you can even save the function pointer at probe. Again, happy to apply it as-is, just let me know please. Thanks, Lorenzo > + err = hw_init_fn(pcie); > + if (err) { > + dev_info(dev, "PCIe link down\n"); > + return 0; > + } > + > + data = rcar_pci_read_reg(pcie, MACSR); > + dev_info(dev, "PCIe x%d: link up\n", (data >> 20) & 0x3f); > + > + /* Enable MSI */ > + if (IS_ENABLED(CONFIG_PCI_MSI)) > + rcar_pcie_hw_enable_msi(pcie); > + > + rcar_pcie_hw_enable(pcie); > + > + return 0; > +} > + > static int rcar_pcie_resume_noirq(struct device *dev) > { > struct rcar_pcie *pcie = dev_get_drvdata(dev); > @@ -1234,6 +1295,7 @@ static int rcar_pcie_resume_noirq(struct device *dev) > } > > static const struct dev_pm_ops rcar_pcie_pm_ops = { > + SET_SYSTEM_SLEEP_PM_OPS(NULL, rcar_pcie_resume) > .resume_noirq = rcar_pcie_resume_noirq, > }; > > -- > 2.25.0 >
On Sat, Mar 14, 2020 at 08:12:32PM +0100, marek.vasut@gmail.com wrote: > From: Kazufumi Ikeda <kaz-ikeda@xc.jp.nec.com> > > This adds the suspend/resume supports for pcie-rcar. The resume handler > reprograms the hardware based on the software state kept in specific > device structures. Also it doesn't need to save any registers. > > Signed-off-by: Kazufumi Ikeda <kaz-ikeda@xc.jp.nec.com> > Signed-off-by: Gaku Inami <gaku.inami.xw@bp.renesas.com> > Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com> > Cc: Geert Uytterhoeven <geert+renesas@glider.be> > Cc: Phil Edworthy <phil.edworthy@renesas.com> > Cc: Simon Horman <horms+renesas@verge.net.au> > Cc: Wolfram Sang <wsa@the-dreams.de> > Cc: linux-renesas-soc@vger.kernel.org > --- > V2: - Change return type of rcar_pcie_hw_enable() to void > - Drop default: case in switch statement in rcar_pcie_hw_enable() > - Sort variables in rcar_pcie_resume() > V3: - Update on top of next-20200313 > --- > drivers/pci/controller/pcie-rcar.c | 86 +++++++++++++++++++++++++----- > 1 file changed, 74 insertions(+), 12 deletions(-) > > diff --git a/drivers/pci/controller/pcie-rcar.c b/drivers/pci/controller/pcie-rcar.c > index 759c6542c5c8..f86513638b8a 100644 > --- a/drivers/pci/controller/pcie-rcar.c > +++ b/drivers/pci/controller/pcie-rcar.c Applied to pci/rcar for v5.18, thanks. Lorenzo > @@ -452,6 +452,32 @@ static void rcar_pcie_force_speedup(struct rcar_pcie *pcie) > (macsr & LINK_SPEED) == LINK_SPEED_5_0GTS ? "5" : "2.5"); > } > > +static void rcar_pcie_hw_enable(struct rcar_pcie *pci) > +{ > + struct resource_entry *win; > + LIST_HEAD(res); > + int i = 0; > + > + /* Try setting 5 GT/s link speed */ > + rcar_pcie_force_speedup(pci); > + > + /* Setup PCI resources */ > + resource_list_for_each_entry(win, &pci->resources) { > + struct resource *res = win->res; > + > + if (!res->flags) > + continue; > + > + switch (resource_type(res)) { > + case IORESOURCE_IO: > + case IORESOURCE_MEM: > + rcar_pcie_setup_window(i, pci, win); > + i++; > + break; > + } > + } > +} > + > static int rcar_pcie_enable(struct rcar_pcie *pcie) > { > struct device *dev = pcie->dev; > @@ -891,11 +917,25 @@ static void rcar_pcie_unmap_msi(struct rcar_pcie *pcie) > irq_domain_remove(msi->domain); > } > > +static void rcar_pcie_hw_enable_msi(struct rcar_pcie *pcie) > +{ > + struct rcar_msi *msi = &pcie->msi; > + unsigned long base; > + > + /* setup MSI data target */ > + base = virt_to_phys((void *)msi->pages); > + > + rcar_pci_write_reg(pcie, lower_32_bits(base) | MSIFE, PCIEMSIALR); > + rcar_pci_write_reg(pcie, upper_32_bits(base), PCIEMSIAUR); > + > + /* enable all MSI interrupts */ > + rcar_pci_write_reg(pcie, 0xffffffff, PCIEMSIIER); > +} > + > static int rcar_pcie_enable_msi(struct rcar_pcie *pcie) > { > struct device *dev = pcie->dev; > struct rcar_msi *msi = &pcie->msi; > - phys_addr_t base; > int err, i; > > mutex_init(&msi->lock); > @@ -934,17 +974,7 @@ static int rcar_pcie_enable_msi(struct rcar_pcie *pcie) > > /* setup MSI data target */ > msi->pages = __get_free_pages(GFP_KERNEL, 0); > - if (!msi->pages) { > - err = -ENOMEM; > - goto err; > - } > - base = virt_to_phys((void *)msi->pages); > - > - rcar_pci_write_reg(pcie, lower_32_bits(base) | MSIFE, PCIEMSIALR); > - rcar_pci_write_reg(pcie, upper_32_bits(base), PCIEMSIAUR); > - > - /* enable all MSI interrupts */ > - rcar_pci_write_reg(pcie, 0xffffffff, PCIEMSIIER); > + rcar_pcie_hw_enable_msi(pcie); > > return 0; > > @@ -1219,6 +1249,37 @@ static int rcar_pcie_probe(struct platform_device *pdev) > return err; > } > > +static int rcar_pcie_resume(struct device *dev) > +{ > + struct rcar_pcie *pcie = dev_get_drvdata(dev); > + int (*hw_init_fn)(struct rcar_pcie *); > + unsigned int data; > + int err; > + > + err = rcar_pcie_parse_map_dma_ranges(pcie); > + if (err) > + return 0; > + > + /* Failure to get a link might just be that no cards are inserted */ > + hw_init_fn = of_device_get_match_data(dev); > + err = hw_init_fn(pcie); > + if (err) { > + dev_info(dev, "PCIe link down\n"); > + return 0; > + } > + > + data = rcar_pci_read_reg(pcie, MACSR); > + dev_info(dev, "PCIe x%d: link up\n", (data >> 20) & 0x3f); > + > + /* Enable MSI */ > + if (IS_ENABLED(CONFIG_PCI_MSI)) > + rcar_pcie_hw_enable_msi(pcie); > + > + rcar_pcie_hw_enable(pcie); > + > + return 0; > +} > + > static int rcar_pcie_resume_noirq(struct device *dev) > { > struct rcar_pcie *pcie = dev_get_drvdata(dev); > @@ -1234,6 +1295,7 @@ static int rcar_pcie_resume_noirq(struct device *dev) > } > > static const struct dev_pm_ops rcar_pcie_pm_ops = { > + SET_SYSTEM_SLEEP_PM_OPS(NULL, rcar_pcie_resume) > .resume_noirq = rcar_pcie_resume_noirq, > }; > > -- > 2.25.0 >
[+cc Vaibhav] Alternate less redundant subject: PCI: rcar: Add suspend/resume support On Sat, Mar 14, 2020 at 08:12:32PM +0100, marek.vasut@gmail.com wrote: > From: Kazufumi Ikeda <kaz-ikeda@xc.jp.nec.com> > > This adds the suspend/resume supports for pcie-rcar. The resume handler > reprograms the hardware based on the software state kept in specific > device structures. Also it doesn't need to save any registers. s/This adds the/Add/ s/supports/support/ > Signed-off-by: Kazufumi Ikeda <kaz-ikeda@xc.jp.nec.com> > Signed-off-by: Gaku Inami <gaku.inami.xw@bp.renesas.com> > Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com> > Cc: Geert Uytterhoeven <geert+renesas@glider.be> > Cc: Phil Edworthy <phil.edworthy@renesas.com> > Cc: Simon Horman <horms+renesas@verge.net.au> > Cc: Wolfram Sang <wsa@the-dreams.de> > Cc: linux-renesas-soc@vger.kernel.org > --- > V2: - Change return type of rcar_pcie_hw_enable() to void > - Drop default: case in switch statement in rcar_pcie_hw_enable() > - Sort variables in rcar_pcie_resume() > V3: - Update on top of next-20200313 > --- > drivers/pci/controller/pcie-rcar.c | 86 +++++++++++++++++++++++++----- > 1 file changed, 74 insertions(+), 12 deletions(-) > > diff --git a/drivers/pci/controller/pcie-rcar.c b/drivers/pci/controller/pcie-rcar.c > index 759c6542c5c8..f86513638b8a 100644 > --- a/drivers/pci/controller/pcie-rcar.c > +++ b/drivers/pci/controller/pcie-rcar.c > @@ -452,6 +452,32 @@ static void rcar_pcie_force_speedup(struct rcar_pcie *pcie) > (macsr & LINK_SPEED) == LINK_SPEED_5_0GTS ? "5" : "2.5"); > } > > +static void rcar_pcie_hw_enable(struct rcar_pcie *pci) > +{ > + struct resource_entry *win; > + LIST_HEAD(res); > + int i = 0; > + > + /* Try setting 5 GT/s link speed */ > + rcar_pcie_force_speedup(pci); > + > + /* Setup PCI resources */ > + resource_list_for_each_entry(win, &pci->resources) { > + struct resource *res = win->res; > + > + if (!res->flags) > + continue; > + > + switch (resource_type(res)) { > + case IORESOURCE_IO: > + case IORESOURCE_MEM: > + rcar_pcie_setup_window(i, pci, win); > + i++; > + break; > + } > + } > +} > + > static int rcar_pcie_enable(struct rcar_pcie *pcie) > { > struct device *dev = pcie->dev; > @@ -891,11 +917,25 @@ static void rcar_pcie_unmap_msi(struct rcar_pcie *pcie) > irq_domain_remove(msi->domain); > } > > +static void rcar_pcie_hw_enable_msi(struct rcar_pcie *pcie) > +{ > + struct rcar_msi *msi = &pcie->msi; > + unsigned long base; > + > + /* setup MSI data target */ > + base = virt_to_phys((void *)msi->pages); > + > + rcar_pci_write_reg(pcie, lower_32_bits(base) | MSIFE, PCIEMSIALR); > + rcar_pci_write_reg(pcie, upper_32_bits(base), PCIEMSIAUR); > + > + /* enable all MSI interrupts */ > + rcar_pci_write_reg(pcie, 0xffffffff, PCIEMSIIER); > +} > + > static int rcar_pcie_enable_msi(struct rcar_pcie *pcie) > { > struct device *dev = pcie->dev; > struct rcar_msi *msi = &pcie->msi; > - phys_addr_t base; > int err, i; > > mutex_init(&msi->lock); > @@ -934,17 +974,7 @@ static int rcar_pcie_enable_msi(struct rcar_pcie *pcie) > > /* setup MSI data target */ > msi->pages = __get_free_pages(GFP_KERNEL, 0); > - if (!msi->pages) { > - err = -ENOMEM; > - goto err; > - } > - base = virt_to_phys((void *)msi->pages); > - > - rcar_pci_write_reg(pcie, lower_32_bits(base) | MSIFE, PCIEMSIALR); > - rcar_pci_write_reg(pcie, upper_32_bits(base), PCIEMSIAUR); > - > - /* enable all MSI interrupts */ > - rcar_pci_write_reg(pcie, 0xffffffff, PCIEMSIIER); > + rcar_pcie_hw_enable_msi(pcie); > > return 0; > > @@ -1219,6 +1249,37 @@ static int rcar_pcie_probe(struct platform_device *pdev) > return err; > } > > +static int rcar_pcie_resume(struct device *dev) > +{ > + struct rcar_pcie *pcie = dev_get_drvdata(dev); > + int (*hw_init_fn)(struct rcar_pcie *); > + unsigned int data; > + int err; > + > + err = rcar_pcie_parse_map_dma_ranges(pcie); > + if (err) > + return 0; > + > + /* Failure to get a link might just be that no cards are inserted */ > + hw_init_fn = of_device_get_match_data(dev); > + err = hw_init_fn(pcie); > + if (err) { > + dev_info(dev, "PCIe link down\n"); > + return 0; > + } > + > + data = rcar_pci_read_reg(pcie, MACSR); > + dev_info(dev, "PCIe x%d: link up\n", (data >> 20) & 0x3f); > + > + /* Enable MSI */ > + if (IS_ENABLED(CONFIG_PCI_MSI)) > + rcar_pcie_hw_enable_msi(pcie); > + > + rcar_pcie_hw_enable(pcie); > + > + return 0; > +} > + > static int rcar_pcie_resume_noirq(struct device *dev) > { > struct rcar_pcie *pcie = dev_get_drvdata(dev); > @@ -1234,6 +1295,7 @@ static int rcar_pcie_resume_noirq(struct device *dev) > } > > static const struct dev_pm_ops rcar_pcie_pm_ops = { > + SET_SYSTEM_SLEEP_PM_OPS(NULL, rcar_pcie_resume) This causes the following warning when CONFIG_PM_SLEEP is not set: drivers/pci/controller/pcie-rcar.c:1253:12: warning: ‘rcar_pcie_resume’ defined but not used [-Wunused-function] 1253 | static int rcar_pcie_resume(struct device *dev) | ^~~~~~~~~~~~~~~~ Most people seem to be using __maybe_unused on the suspend/resume functions to avoid this, e.g., 226e6b866d74 ("gpio: pch: Convert to dev_pm_ops"). > .resume_noirq = rcar_pcie_resume_noirq, > }; > > -- > 2.25.0 >
Hi Bjorn, On Fri, Apr 24, 2020 at 9:57 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > [+cc Vaibhav] > > Alternate less redundant subject: > > PCI: rcar: Add suspend/resume support Note that there's both pcie-rcar.c (this driver, for R-Car Gen2 and Gen3 PCIe) and pci-rcar-gen2.c (for R-Car Gen2 PCI). People tend to use the prefix "PCI: rcar: " for both :-( Gr{oetje,eeting}s, Geert
On 4/24/20 9:57 PM, Bjorn Helgaas wrote: Hi, [...] >> @@ -1234,6 +1295,7 @@ static int rcar_pcie_resume_noirq(struct device *dev) >> } >> >> static const struct dev_pm_ops rcar_pcie_pm_ops = { >> + SET_SYSTEM_SLEEP_PM_OPS(NULL, rcar_pcie_resume) > > This causes the following warning when CONFIG_PM_SLEEP is not set: > > drivers/pci/controller/pcie-rcar.c:1253:12: warning: ‘rcar_pcie_resume’ defined but not used [-Wunused-function] > 1253 | static int rcar_pcie_resume(struct device *dev) > | ^~~~~~~~~~~~~~~~ > > Most people seem to be using __maybe_unused on the suspend/resume > functions to avoid this, e.g., 226e6b866d74 ("gpio: pch: Convert to > dev_pm_ops"). Should be fixed by: PCI: pcie-rcar: Mark rcar_pcie_resume() with __maybe_unused
On 3/20/20 11:12 AM, Lorenzo Pieralisi wrote: [...] >> +static int rcar_pcie_resume(struct device *dev) >> +{ >> + struct rcar_pcie *pcie = dev_get_drvdata(dev); >> + int (*hw_init_fn)(struct rcar_pcie *); >> + unsigned int data; >> + int err; >> + >> + err = rcar_pcie_parse_map_dma_ranges(pcie); >> + if (err) >> + return 0; >> + >> + /* Failure to get a link might just be that no cards are inserted */ >> + hw_init_fn = of_device_get_match_data(dev); > > Hi Marek, Hi, > happy to apply it as is, I was wondering if it is work taking this > look-up out of the resume path, it is not supposed to change anyway, > you can even save the function pointer at probe. > > Again, happy to apply it as-is, just let me know please. I just sent subsequent patch to address this: PCI: pcie-rcar: Cache PHY init function pointer
[+cc Lorenzo] On Sat, Apr 25, 2020 at 10:55:21AM +0200, Geert Uytterhoeven wrote: > On Fri, Apr 24, 2020 at 9:57 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > > [+cc Vaibhav] > > > > Alternate less redundant subject: > > > > PCI: rcar: Add suspend/resume support > > Note that there's both pcie-rcar.c (this driver, for R-Car Gen2 and Gen3 > PCIe) and pci-rcar-gen2.c (for R-Car Gen2 PCI). > People tend to use the prefix "PCI: rcar: " for both :-( Yeah, that's pretty broken, thanks for pointing this out! For most drivers we use a chipset name ("keystone", "imx6", "tegra", etc) as the changlog tag. That's nice because it gives space for multiple drivers from the same vendor, but I don't know anything similarly specific for the R-Car drivers. pci-rcar-gen2.c seems to be for some sort of internal Conventional PCI bus? The "gen2" is confusing because "Gen 2" is more commonly used for PCIe than for Conventional PCI. I would propose keeping "rcar" for the PCIe driver and using "rcar-pci" for the Conventional PCI one, but the Conventional PCI one (pci-rcar-gen2.c) seems pretty inactive. The most recent commits are from 2018, and they're trivial cleanups. So I'm doubtful that anybody will remember when the next change comes in. Bjorn
Hi Bjorn, On Mon, Apr 27, 2020 at 7:41 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > On Sat, Apr 25, 2020 at 10:55:21AM +0200, Geert Uytterhoeven wrote: > > On Fri, Apr 24, 2020 at 9:57 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > > > [+cc Vaibhav] > > > > > > Alternate less redundant subject: > > > > > > PCI: rcar: Add suspend/resume support > > > > Note that there's both pcie-rcar.c (this driver, for R-Car Gen2 and Gen3 > > PCIe) and pci-rcar-gen2.c (for R-Car Gen2 PCI). > > People tend to use the prefix "PCI: rcar: " for both :-( > > Yeah, that's pretty broken, thanks for pointing this out! > > For most drivers we use a chipset name ("keystone", "imx6", "tegra", > etc) as the changlog tag. That's nice because it gives space for > multiple drivers from the same vendor, but I don't know anything > similarly specific for the R-Car drivers. > > pci-rcar-gen2.c seems to be for some sort of internal Conventional PCI AFAIUI it's some internal PCI glue to the *HCI USB controller. > bus? The "gen2" is confusing because "Gen 2" is more commonly used > for PCIe than for Conventional PCI. The "Gen2" applies to "R-Car", not to "PCI". > I would propose keeping "rcar" for the PCIe driver and using > "rcar-pci" for the Conventional PCI one, but the Conventional PCI one (/me resists against bike-shedding) > (pci-rcar-gen2.c) seems pretty inactive. The most recent commits are > from 2018, and they're trivial cleanups. So I'm doubtful that anybody > will remember when the next change comes in. I guess pci-rcar-gen2.c is simpler and more mature ;-) R-Car Gen2 SoCs have both (internal) PCI and PCIe, so the two drivers can be used together on the same hardware. Gr{oetje,eeting}s, Geert
On Mon, Apr 27, 2020 at 10:08:52PM +0200, Geert Uytterhoeven wrote: > Hi Bjorn, > > On Mon, Apr 27, 2020 at 7:41 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Sat, Apr 25, 2020 at 10:55:21AM +0200, Geert Uytterhoeven wrote: > > > On Fri, Apr 24, 2020 at 9:57 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > > > > [+cc Vaibhav] > > > > > > > > Alternate less redundant subject: > > > > > > > > PCI: rcar: Add suspend/resume support > > > > > > Note that there's both pcie-rcar.c (this driver, for R-Car Gen2 and Gen3 > > > PCIe) and pci-rcar-gen2.c (for R-Car Gen2 PCI). > > > People tend to use the prefix "PCI: rcar: " for both :-( > > > > Yeah, that's pretty broken, thanks for pointing this out! > > > > For most drivers we use a chipset name ("keystone", "imx6", "tegra", > > etc) as the changlog tag. That's nice because it gives space for > > multiple drivers from the same vendor, but I don't know anything > > similarly specific for the R-Car drivers. > > > > pci-rcar-gen2.c seems to be for some sort of internal Conventional PCI > > AFAIUI it's some internal PCI glue to the *HCI USB controller. > > > bus? The "gen2" is confusing because "Gen 2" is more commonly used > > for PCIe than for Conventional PCI. > > The "Gen2" applies to "R-Car", not to "PCI". Wicked :) ! > > I would propose keeping "rcar" for the PCIe driver and using > > "rcar-pci" for the Conventional PCI one, but the Conventional PCI one > > (/me resists against bike-shedding) I'd agree with Bjorn - I don't know, internal vs external seems artificial. Certainly gen2 is misleading, it does not take much to improve it. > > (pci-rcar-gen2.c) seems pretty inactive. The most recent commits are > > from 2018, and they're trivial cleanups. So I'm doubtful that anybody > > will remember when the next change comes in. > > I guess pci-rcar-gen2.c is simpler and more mature ;-) > R-Car Gen2 SoCs have both (internal) PCI and PCIe, so the two drivers > can be used together on the same hardware. I'd remove gen2 to start with, you are better placed to know the internals to come up with something significant. Lorenzo
Hi Lorenzo, On Tue, Apr 28, 2020 at 10:26 AM Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote: > On Mon, Apr 27, 2020 at 10:08:52PM +0200, Geert Uytterhoeven wrote: > > On Mon, Apr 27, 2020 at 7:41 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > > > On Sat, Apr 25, 2020 at 10:55:21AM +0200, Geert Uytterhoeven wrote: > > > > On Fri, Apr 24, 2020 at 9:57 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > > > > > [+cc Vaibhav] > > > > > > > > > > Alternate less redundant subject: > > > > > > > > > > PCI: rcar: Add suspend/resume support > > > > > > > > Note that there's both pcie-rcar.c (this driver, for R-Car Gen2 and Gen3 > > > > PCIe) and pci-rcar-gen2.c (for R-Car Gen2 PCI). > > > > People tend to use the prefix "PCI: rcar: " for both :-( > > > > > > Yeah, that's pretty broken, thanks for pointing this out! > > > > > > For most drivers we use a chipset name ("keystone", "imx6", "tegra", > > > etc) as the changlog tag. That's nice because it gives space for > > > multiple drivers from the same vendor, but I don't know anything > > > similarly specific for the R-Car drivers. > > > > > > pci-rcar-gen2.c seems to be for some sort of internal Conventional PCI > > > > AFAIUI it's some internal PCI glue to the *HCI USB controller. > > > > > bus? The "gen2" is confusing because "Gen 2" is more commonly used > > > for PCIe than for Conventional PCI. > > > > The "Gen2" applies to "R-Car", not to "PCI". > > Wicked :) ! pcie-rcar.c supports R-Car Gen1, Gen2, and Gen3. > > > I would propose keeping "rcar" for the PCIe driver and using > > > "rcar-pci" for the Conventional PCI one, but the Conventional PCI one > > > > (/me resists against bike-shedding) > > I'd agree with Bjorn - I don't know, internal vs external seems > artificial. Certainly gen2 is misleading, it does not take much > to improve it. We have lots of drivers in other subsystems with "rcar-gen2" or "rcar-gen3" as part of their names. > > > (pci-rcar-gen2.c) seems pretty inactive. The most recent commits are > > > from 2018, and they're trivial cleanups. So I'm doubtful that anybody > > > will remember when the next change comes in. > > > > I guess pci-rcar-gen2.c is simpler and more mature ;-) > > R-Car Gen2 SoCs have both (internal) PCI and PCIe, so the two drivers > > can be used together on the same hardware. > > I'd remove gen2 to start with, you are better placed to know the > internals to come up with something significant. So we're back at "PCI: rcar: ...", for both ;-) I'd say the main difference between the two drivers is PCI vs. PCIe. Gr{oetje,eeting}s, Geert
diff --git a/drivers/pci/controller/pcie-rcar.c b/drivers/pci/controller/pcie-rcar.c index 759c6542c5c8..f86513638b8a 100644 --- a/drivers/pci/controller/pcie-rcar.c +++ b/drivers/pci/controller/pcie-rcar.c @@ -452,6 +452,32 @@ static void rcar_pcie_force_speedup(struct rcar_pcie *pcie) (macsr & LINK_SPEED) == LINK_SPEED_5_0GTS ? "5" : "2.5"); } +static void rcar_pcie_hw_enable(struct rcar_pcie *pci) +{ + struct resource_entry *win; + LIST_HEAD(res); + int i = 0; + + /* Try setting 5 GT/s link speed */ + rcar_pcie_force_speedup(pci); + + /* Setup PCI resources */ + resource_list_for_each_entry(win, &pci->resources) { + struct resource *res = win->res; + + if (!res->flags) + continue; + + switch (resource_type(res)) { + case IORESOURCE_IO: + case IORESOURCE_MEM: + rcar_pcie_setup_window(i, pci, win); + i++; + break; + } + } +} + static int rcar_pcie_enable(struct rcar_pcie *pcie) { struct device *dev = pcie->dev; @@ -891,11 +917,25 @@ static void rcar_pcie_unmap_msi(struct rcar_pcie *pcie) irq_domain_remove(msi->domain); } +static void rcar_pcie_hw_enable_msi(struct rcar_pcie *pcie) +{ + struct rcar_msi *msi = &pcie->msi; + unsigned long base; + + /* setup MSI data target */ + base = virt_to_phys((void *)msi->pages); + + rcar_pci_write_reg(pcie, lower_32_bits(base) | MSIFE, PCIEMSIALR); + rcar_pci_write_reg(pcie, upper_32_bits(base), PCIEMSIAUR); + + /* enable all MSI interrupts */ + rcar_pci_write_reg(pcie, 0xffffffff, PCIEMSIIER); +} + static int rcar_pcie_enable_msi(struct rcar_pcie *pcie) { struct device *dev = pcie->dev; struct rcar_msi *msi = &pcie->msi; - phys_addr_t base; int err, i; mutex_init(&msi->lock); @@ -934,17 +974,7 @@ static int rcar_pcie_enable_msi(struct rcar_pcie *pcie) /* setup MSI data target */ msi->pages = __get_free_pages(GFP_KERNEL, 0); - if (!msi->pages) { - err = -ENOMEM; - goto err; - } - base = virt_to_phys((void *)msi->pages); - - rcar_pci_write_reg(pcie, lower_32_bits(base) | MSIFE, PCIEMSIALR); - rcar_pci_write_reg(pcie, upper_32_bits(base), PCIEMSIAUR); - - /* enable all MSI interrupts */ - rcar_pci_write_reg(pcie, 0xffffffff, PCIEMSIIER); + rcar_pcie_hw_enable_msi(pcie); return 0; @@ -1219,6 +1249,37 @@ static int rcar_pcie_probe(struct platform_device *pdev) return err; } +static int rcar_pcie_resume(struct device *dev) +{ + struct rcar_pcie *pcie = dev_get_drvdata(dev); + int (*hw_init_fn)(struct rcar_pcie *); + unsigned int data; + int err; + + err = rcar_pcie_parse_map_dma_ranges(pcie); + if (err) + return 0; + + /* Failure to get a link might just be that no cards are inserted */ + hw_init_fn = of_device_get_match_data(dev); + err = hw_init_fn(pcie); + if (err) { + dev_info(dev, "PCIe link down\n"); + return 0; + } + + data = rcar_pci_read_reg(pcie, MACSR); + dev_info(dev, "PCIe x%d: link up\n", (data >> 20) & 0x3f); + + /* Enable MSI */ + if (IS_ENABLED(CONFIG_PCI_MSI)) + rcar_pcie_hw_enable_msi(pcie); + + rcar_pcie_hw_enable(pcie); + + return 0; +} + static int rcar_pcie_resume_noirq(struct device *dev) { struct rcar_pcie *pcie = dev_get_drvdata(dev); @@ -1234,6 +1295,7 @@ static int rcar_pcie_resume_noirq(struct device *dev) } static const struct dev_pm_ops rcar_pcie_pm_ops = { + SET_SYSTEM_SLEEP_PM_OPS(NULL, rcar_pcie_resume) .resume_noirq = rcar_pcie_resume_noirq, };