On Thu, Jan 11, 2018 at 11:38:08AM +0530, Manikanta Maddireddy wrote: > Tegra186 powergate driver is implemented as power domain driver, power > partition ungate/gate are registered as power_on/power_off callback > functions. There are no direct functions to power gate/ungate host > controller in Tegra186. Host controller driver should add "power-domains" > property in device tree and implement runtime suspend and resume > callback functons. Power gate and ungate is taken care by power domain > driver when host controller driver calls pm_runtime_put_sync and > pm_runtime_get_sync respectively. > > Register suspend_noirq & resume_noirq callback functions to allow PCIe to > come up after resume from RAM. Both runtime and noirq pm ops share same > callback functions. > > Signed-off-by: Manikanta Maddireddy <mmaddireddy@xxxxxxxxxx> > --- > V2: > * no change in this patch > V3: > * no change in this patch > V4: > * no change in this patch > V5: > * Decoupled from https://patchwork.ozlabs.org/patch/832053/ and > rebased on linux-next > V6: > * no change in this patch > > drivers/pci/host/pci-tegra.c | 181 ++++++++++++++++++++++++++----------------- > 1 file changed, 110 insertions(+), 71 deletions(-) > > diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c [...] > @@ -1536,37 +1526,41 @@ static int tegra_pcie_enable_msi(struct tegra_pcie *pcie) > int err; > u32 reg; > > - mutex_init(&msi->lock); > + if (!msi->phys) { > + mutex_init(&msi->lock); > > - msi->chip.dev = dev; > - msi->chip.setup_irq = tegra_msi_setup_irq; > - msi->chip.teardown_irq = tegra_msi_teardown_irq; > + msi->chip.dev = dev; > + msi->chip.setup_irq = tegra_msi_setup_irq; > + msi->chip.teardown_irq = tegra_msi_teardown_irq; > > - msi->domain = irq_domain_add_linear(dev->of_node, INT_PCI_MSI_NR, > - &msi_domain_ops, &msi->chip); > - if (!msi->domain) { > - dev_err(dev, "failed to create IRQ domain\n"); > - return -ENOMEM; > - } > + msi->domain = irq_domain_add_linear(dev->of_node, > + INT_PCI_MSI_NR, > + &msi_domain_ops, > + &msi->chip); > + if (!msi->domain) { > + dev_err(dev, "failed to create IRQ domain\n"); > + return -ENOMEM; > + } > > - err = platform_get_irq_byname(pdev, "msi"); > - if (err < 0) { > - dev_err(dev, "failed to get IRQ: %d\n", err); > - goto err; > - } > + err = platform_get_irq_byname(pdev, "msi"); > + if (err < 0) { > + dev_err(dev, "failed to get IRQ: %d\n", err); > + goto err; > + } > > - msi->irq = err; > + msi->irq = err; > > - err = request_irq(msi->irq, tegra_pcie_msi_irq, IRQF_NO_THREAD, > - tegra_msi_irq_chip.name, pcie); > - if (err < 0) { > - dev_err(dev, "failed to request IRQ: %d\n", err); > - goto err; > - } > + err = request_irq(msi->irq, tegra_pcie_msi_irq, IRQF_NO_THREAD, > + tegra_msi_irq_chip.name, pcie); > + if (err < 0) { > + dev_err(dev, "failed to request IRQ: %d\n", err); > + goto err; > + } > > - /* setup AFI/FPCI range */ > - msi->pages = __get_free_pages(GFP_KERNEL, 0); > - msi->phys = virt_to_phys((void *)msi->pages); > + /* setup AFI/FPCI range */ > + msi->pages = __get_free_pages(GFP_KERNEL, 0); > + msi->phys = virt_to_phys((void *)msi->pages); > + } I think it'd be better to split this off into a separate function so that we can get rid of the confusing if (!msi->phys) conditional. It looks as though this would work, but it's confusing to have that code in a resume handler. It also becomes very asymmetric when you set up the MSIs in the resume handler, but then don't call tegra_pcie_msi_disable() from the suspend handler. I'd suggest just pulling out all of the allocations and such into a separate function, perhaps tegra_pcie_msi_setup(), and similarily split off the resource part of tegra_pcie_disable_msi() into a separate function, perhaps tegra_pcie_msi_teardown(). That way, suspend/resume will do only what they need (write registers) and everything will look more symmetric again. That in turn makes the code a lot easier to follow and maintain. Thierry
Attachment:
signature.asc
Description: PGP signature
