[PATCH V9 0/3] Add loadable kernel module and power management support

[PATCH V9 0/3] Add loadable kernel module and power management support

[Manikanta Maddireddy]

This series of patches adds loadable kernel module and power management
support to Tegra PCIe host controller driver. irq_set_msi_desc() and
tegra_cpuidle_pcie_irqs_in_use() symbols will be taken care in next
set of patches.

These patches are tested on Jetson TK1, TX1 and TX2 platforms, following
are the verification details.
	- Multiple module insert & remove
	- PCIe device functionality after module insert
	- Free clock, resets, regulators, powergate, iomem and interrupt
	resources after module remove
	- PCIe device functionality after resume from RAM

V2: PM QoS fix is dropped in V2 from this series because the fix is
incorporated in latest 'commit 0759e80b84e3 ("PM / QoS: Fix device resume
latency framework")'. Update commit message of few patches in V2.

V3: Patches to export irq_set_msi_desc() and tegra_cpuidle_pcie_irqs_in_use()
are dropped based on review comments. These symbols will be addressed in next
series. Took care of few other review comments.

V4: Dropped pci_find_host_bridge() export patch and added new patch to use
bus->sysdata for private data.

V5: Decouple from https://patchwork.ozlabs.org/patch/832053/ and rebase
on top of linux-next

V6: Rebased on lpieralisi/pci/tegra branch

V7: Addressed comments on patch: 6 and 7 in V6
Limiting this series to linux-tegra & linux-pci since patches for other 
subsystem are merged or dropped

V8: Since LKM is not completely implemented changing PCI_TEGRA config
back to bool

V9: Squash patch-3 (PME_Turn_Off patch) on top of patch-2 (LKM patch)
in V8 to have single working LKM patch.

Manikanta Maddireddy (3):
  PCI: tegra: Free resources on probe failure
  PCI: tegra: Add loadable kernel module support
  PCI: tegra: Add power management support

 drivers/pci/host/pci-tegra.c | 354 +++++++++++++++++++++++++++++++++----------
 1 file changed, 278 insertions(+), 76 deletions(-)

-- 
2.1.4

[PATCH V9 1/3] PCI: tegra: Free resources on probe failure

[Manikanta Maddireddy]

tegra_pcie_probe() can fail in multiple instances, this patch takes care
of freeing the resources which are allocated before probe fail.

Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
Acked-by: Thierry Reding <treding@nvidia.com>
Tested-by: Thierry Reding <treding@nvidia.com>
---
V2:
* no change in this patch
V3:
* change 'if check' to 'legacy_phy is true' for tegra_pcie_phys_put_legacy()
* commit log correction
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
V7:
* no change in this patch
V8:
* no change in this patch
V9:
* no change in this patch

 drivers/pci/host/pci-tegra.c | 99 +++++++++++++++++++++++++++++++++++---------
 1 file changed, 79 insertions(+), 20 deletions(-)

diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
index e4b47ebc91fe..e68507f658d8 100644
--- a/drivers/pci/host/pci-tegra.c
+++ b/drivers/pci/host/pci-tegra.c
@@ -562,14 +562,25 @@ static int tegra_pcie_request_resources(struct tegra_pcie *pcie)
 	pci_add_resource(windows, &pcie->busn);
 
 	err = devm_request_pci_bus_resources(dev, windows);
-	if (err < 0)
+	if (err < 0) {
+		pci_free_resource_list(windows);
 		return err;
+	}
 
 	pci_remap_iospace(&pcie->pio, pcie->io.start);
 
 	return 0;
 }
 
+static void tegra_pcie_free_resources(struct tegra_pcie *pcie)
+{
+	struct pci_host_bridge *host = pci_host_bridge_from_priv(pcie);
+	struct list_head *windows = &host->windows;
+
+	pci_unmap_iospace(&pcie->pio);
+	pci_free_resource_list(windows);
+}
+
 static int tegra_pcie_map_irq(const struct pci_dev *pdev, u8 slot, u8 pin)
 {
 	struct tegra_pcie *pcie = pdev->bus->sysdata;
@@ -979,24 +990,35 @@ static int tegra_pcie_enable_controller(struct tegra_pcie *pcie)
 	return 0;
 }
 
-static void tegra_pcie_power_off(struct tegra_pcie *pcie)
+static void tegra_pcie_disable_controller(struct tegra_pcie *pcie)
 {
-	struct device *dev = pcie->dev;
-	const struct tegra_pcie_soc *soc = pcie->soc;
 	int err;
 
-	/* TODO: disable and unprepare clocks? */
+	reset_control_assert(pcie->pcie_xrst);
 
-	if (soc->program_uphy) {
+	if (pcie->soc->program_uphy) {
 		err = tegra_pcie_phy_power_off(pcie);
 		if (err < 0)
-			dev_err(dev, "failed to power off PHY(s): %d\n", err);
+			dev_err(pcie->dev, "failed to power off PHY(s): %d\n",
+				err);
 	}
+}
+
+static void tegra_pcie_power_off(struct tegra_pcie *pcie)
+{
+	struct device *dev = pcie->dev;
+	const struct tegra_pcie_soc *soc = pcie->soc;
+	int err;
 
-	reset_control_assert(pcie->pcie_xrst);
 	reset_control_assert(pcie->afi_rst);
 	reset_control_assert(pcie->pex_rst);
 
+	clk_disable_unprepare(pcie->pll_e);
+	if (soc->has_cml_clk)
+		clk_disable_unprepare(pcie->cml_clk);
+	clk_disable_unprepare(pcie->afi_clk);
+	clk_disable_unprepare(pcie->pex_clk);
+
 	if (!dev->pm_domain)
 		tegra_powergate_power_off(TEGRA_POWERGATE_PCIE);
 
@@ -1205,6 +1227,30 @@ static int tegra_pcie_phys_get(struct tegra_pcie *pcie)
 	return 0;
 }
 
+static void tegra_pcie_phys_put(struct tegra_pcie *pcie)
+{
+	struct tegra_pcie_port *port;
+	struct device *dev = pcie->dev;
+	int err, i;
+
+	if (pcie->legacy_phy) {
+		err = phy_exit(pcie->phy);
+		if (err < 0)
+			dev_err(dev, "failed to teardown PHY: %d\n", err);
+		return;
+	}
+
+	list_for_each_entry(port, &pcie->ports, list) {
+		for (i = 0; i < port->lanes; i++) {
+			err = phy_exit(port->phys[i]);
+			if (err < 0)
+				dev_err(dev, "failed to teardown PHY#%u: %d\n",
+					i, err);
+		}
+	}
+}
+
+
 static int tegra_pcie_get_resources(struct tegra_pcie *pcie)
 {
 	struct device *dev = pcie->dev;
@@ -1236,7 +1282,7 @@ static int tegra_pcie_get_resources(struct tegra_pcie *pcie)
 	err = tegra_pcie_power_on(pcie);
 	if (err) {
 		dev_err(dev, "failed to power up: %d\n", err);
-		return err;
+		goto phys_put;
 	}
 
 	pads = platform_get_resource_byname(pdev, IORESOURCE_MEM, "pads");
@@ -1288,6 +1334,9 @@ static int tegra_pcie_get_resources(struct tegra_pcie *pcie)
 
 	return 0;
 
+phys_put:
+	if (soc->program_uphy)
+		tegra_pcie_phys_put(pcie);
 poweroff:
 	tegra_pcie_power_off(pcie);
 	return err;
@@ -1295,20 +1344,15 @@ static int tegra_pcie_get_resources(struct tegra_pcie *pcie)
 
 static int tegra_pcie_put_resources(struct tegra_pcie *pcie)
 {
-	struct device *dev = pcie->dev;
 	const struct tegra_pcie_soc *soc = pcie->soc;
-	int err;
 
 	if (pcie->irq > 0)
 		free_irq(pcie->irq, pcie);
 
 	tegra_pcie_power_off(pcie);
 
-	if (soc->program_uphy) {
-		err = phy_exit(pcie->phy);
-		if (err < 0)
-			dev_err(dev, "failed to teardown PHY: %d\n", err);
-	}
+	if (soc->program_uphy)
+		tegra_pcie_phys_put(pcie);
 
 	return 0;
 }
@@ -2048,6 +2092,16 @@ static void tegra_pcie_enable_ports(struct tegra_pcie *pcie)
 	}
 }
 
+static void tegra_pcie_disable_ports(struct tegra_pcie *pcie)
+{
+	struct tegra_pcie_port *port, *tmp;
+
+	list_for_each_entry_safe(port, tmp, &pcie->ports, list) {
+		tegra_pcie_port_disable(port);
+		tegra_pcie_port_free(port);
+	}
+}
+
 static const struct tegra_pcie_soc tegra20_pcie = {
 	.num_ports = 2,
 	.msi_base_shift = 0,
@@ -2278,7 +2332,7 @@ static int tegra_pcie_probe(struct platform_device *pdev)
 
 	err = tegra_pcie_request_resources(pcie);
 	if (err)
-		goto put_resources;
+		goto disable_controller;
 
 	/* setup the AFI address translations */
 	tegra_pcie_setup_translations(pcie);
@@ -2287,7 +2341,7 @@ static int tegra_pcie_probe(struct platform_device *pdev)
 		err = tegra_pcie_enable_msi(pcie);
 		if (err < 0) {
 			dev_err(dev, "failed to enable MSI support: %d\n", err);
-			goto put_resources;
+			goto free_resources;
 		}
 	}
 
@@ -2302,7 +2356,7 @@ static int tegra_pcie_probe(struct platform_device *pdev)
 	err = pci_scan_root_bus_bridge(host);
 	if (err < 0) {
 		dev_err(dev, "failed to register host: %d\n", err);
-		goto disable_msi;
+		goto disable_ports;
 	}
 
 	pci_bus_size_bridges(host->bus);
@@ -2321,9 +2375,14 @@ static int tegra_pcie_probe(struct platform_device *pdev)
 
 	return 0;
 
-disable_msi:
+disable_ports:
+	tegra_pcie_disable_ports(pcie);
 	if (IS_ENABLED(CONFIG_PCI_MSI))
 		tegra_pcie_disable_msi(pcie);
+free_resources:
+	tegra_pcie_free_resources(pcie);
+disable_controller:
+	tegra_pcie_disable_controller(pcie);
 put_resources:
 	tegra_pcie_put_resources(pcie);
 	return err;
-- 
2.1.4

[PATCH V9 2/3] PCI: tegra: Add loadable kernel module support

[Manikanta Maddireddy]

Implement remove callback function for Tegra PCIe driver to add
loadable kernel module support.

Per PCIe r3.0, sec 5.3.3.2.1, PCIe root port should broadcast PME_Turn_Off
message before PCIe link goes to L2. PME_Turn_Off broadcast mechanism is
implemented in AFI module. Each Tegra PCIe root port has its own
PME_Turn_Off and PME_TO_Ack bitmap in AFI_PME register, program this
register to broadcast PME_Turn_Off message.

Once PME_TO_Ack is recieved driver will turn OFF PCIe clock, power gate
PCIe partition and turn OFF regulators.

Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
Acked-by: Thierry Reding <treding@nvidia.com>
Tested-by: Thierry Reding <treding@nvidia.com>
---
V2:
* no change in this patch
V3:
* use tegra_pcie_debugfs_exit() helper function in tegra_pcie_debugfs_init()
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
V7:
* no change in this patch
V8:
* Added new lines around conditional blocks
V9:
* Squash PME_Turn_Off patch on top this to have single working LKM patch

 drivers/pci/host/pci-tegra.c | 103 +++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 100 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
index e68507f658d8..60b1d5e1cfa4 100644
--- a/drivers/pci/host/pci-tegra.c
+++ b/drivers/pci/host/pci-tegra.c
@@ -31,10 +31,12 @@
 #include <linux/delay.h>
 #include <linux/export.h>
 #include <linux/interrupt.h>
+#include <linux/iopoll.h>
 #include <linux/irq.h>
 #include <linux/irqdomain.h>
 #include <linux/kernel.h>
 #include <linux/init.h>
+#include <linux/module.h>
 #include <linux/msi.h>
 #include <linux/of_address.h>
 #include <linux/of_pci.h>
@@ -152,6 +154,8 @@
 #define  AFI_INTR_EN_FPCI_TIMEOUT	(1 << 7)
 #define  AFI_INTR_EN_PRSNT_SENSE	(1 << 8)
 
+#define AFI_PCIE_PME		0xf0
+
 #define AFI_PCIE_CONFIG					0x0f8
 #define  AFI_PCIE_CONFIG_PCIE_DISABLE(x)		(1 << ((x) + 1))
 #define  AFI_PCIE_CONFIG_PCIE_DISABLE_ALL		0xe
@@ -232,6 +236,8 @@
 #define PADS_REFCLK_CFG_PREDI_SHIFT		8  /* 11:8 */
 #define PADS_REFCLK_CFG_DRVI_SHIFT		12 /* 15:12 */
 
+#define PME_ACK_TIMEOUT 10000
+
 struct tegra_msi {
 	struct msi_controller chip;
 	DECLARE_BITMAP(used, INT_PCI_MSI_NR);
@@ -243,8 +249,16 @@ struct tegra_msi {
 };
 
 /* used to differentiate between Tegra SoC generations */
+struct tegra_pcie_port_soc {
+	struct {
+		u8 turnoff_bit;
+		u8 ack_bit;
+	} pme;
+};
+
 struct tegra_pcie_soc {
 	unsigned int num_ports;
+	const struct tegra_pcie_port_soc *ports;
 	unsigned int msi_base_shift;
 	u32 pads_pll_ctl;
 	u32 tx_ref_sel;
@@ -1357,6 +1371,32 @@ static int tegra_pcie_put_resources(struct tegra_pcie *pcie)
 	return 0;
 }
 
+static void tegra_pcie_pme_turnoff(struct tegra_pcie_port *port)
+{
+	struct tegra_pcie *pcie = port->pcie;
+	const struct tegra_pcie_soc *soc = pcie->soc;
+	int err;
+	u32 val;
+	u8 ack_bit;
+
+	val = afi_readl(pcie, AFI_PCIE_PME);
+	val |= (0x1 << soc->ports[port->index].pme.turnoff_bit);
+	afi_writel(pcie, val, AFI_PCIE_PME);
+
+	ack_bit = soc->ports[port->index].pme.ack_bit;
+	err = readl_poll_timeout(pcie->afi + AFI_PCIE_PME, val,
+				 val & (0x1 << ack_bit), 1, PME_ACK_TIMEOUT);
+	if (err)
+		dev_err(pcie->dev, "PME Ack is not received on port: %d\n",
+			port->index);
+
+	usleep_range(10000, 11000);
+
+	val = afi_readl(pcie, AFI_PCIE_PME);
+	val &= ~(0x1 << soc->ports[port->index].pme.turnoff_bit);
+	afi_writel(pcie, val, AFI_PCIE_PME);
+}
+
 static int tegra_msi_alloc(struct tegra_msi *chip)
 {
 	int msi;
@@ -2102,8 +2142,14 @@ static void tegra_pcie_disable_ports(struct tegra_pcie *pcie)
 	}
 }
 
+static const struct tegra_pcie_port_soc tegra20_pcie_ports[] = {
+	{ .pme.turnoff_bit = 0, .pme.ack_bit =  5 },
+	{ .pme.turnoff_bit = 8, .pme.ack_bit = 10 },
+};
+
 static const struct tegra_pcie_soc tegra20_pcie = {
 	.num_ports = 2,
+	.ports = tegra20_pcie_ports,
 	.msi_base_shift = 0,
 	.pads_pll_ctl = PADS_PLL_CTL_TEGRA20,
 	.tx_ref_sel = PADS_PLL_CTL_TXCLKREF_DIV10,
@@ -2117,8 +2163,15 @@ static const struct tegra_pcie_soc tegra20_pcie = {
 	.program_uphy = true,
 };
 
+static const struct tegra_pcie_port_soc tegra30_pcie_ports[] = {
+	{ .pme.turnoff_bit =  0, .pme.ack_bit =  5 },
+	{ .pme.turnoff_bit =  8, .pme.ack_bit = 10 },
+	{ .pme.turnoff_bit = 16, .pme.ack_bit = 18 },
+};
+
 static const struct tegra_pcie_soc tegra30_pcie = {
 	.num_ports = 3,
+	.ports = tegra30_pcie_ports,
 	.msi_base_shift = 8,
 	.pads_pll_ctl = PADS_PLL_CTL_TEGRA30,
 	.tx_ref_sel = PADS_PLL_CTL_TXCLKREF_BUF_EN,
@@ -2135,6 +2188,7 @@ static const struct tegra_pcie_soc tegra30_pcie = {
 
 static const struct tegra_pcie_soc tegra124_pcie = {
 	.num_ports = 2,
+	.ports = tegra20_pcie_ports,
 	.msi_base_shift = 8,
 	.pads_pll_ctl = PADS_PLL_CTL_TEGRA30,
 	.tx_ref_sel = PADS_PLL_CTL_TXCLKREF_BUF_EN,
@@ -2150,6 +2204,7 @@ static const struct tegra_pcie_soc tegra124_pcie = {
 
 static const struct tegra_pcie_soc tegra210_pcie = {
 	.num_ports = 2,
+	.ports = tegra20_pcie_ports,
 	.msi_base_shift = 8,
 	.pads_pll_ctl = PADS_PLL_CTL_TEGRA30,
 	.tx_ref_sel = PADS_PLL_CTL_TXCLKREF_BUF_EN,
@@ -2163,8 +2218,15 @@ static const struct tegra_pcie_soc tegra210_pcie = {
 	.program_uphy = true,
 };
 
+static const struct tegra_pcie_port_soc tegra186_pcie_ports[] = {
+	{ .pme.turnoff_bit =  0, .pme.ack_bit =  5 },
+	{ .pme.turnoff_bit =  8, .pme.ack_bit = 10 },
+	{ .pme.turnoff_bit = 12, .pme.ack_bit = 14 },
+};
+
 static const struct tegra_pcie_soc tegra186_pcie = {
 	.num_ports = 3,
+	.ports = tegra186_pcie_ports,
 	.msi_base_shift = 8,
 	.pads_pll_ctl = PADS_PLL_CTL_TEGRA30,
 	.tx_ref_sel = PADS_PLL_CTL_TXCLKREF_BUF_EN,
@@ -2276,6 +2338,12 @@ static const struct file_operations tegra_pcie_ports_ops = {
 	.release = seq_release,
 };
 
+static void tegra_pcie_debugfs_exit(struct tegra_pcie *pcie)
+{
+	debugfs_remove_recursive(pcie->debugfs);
+	pcie->debugfs = NULL;
+}
+
 static int tegra_pcie_debugfs_init(struct tegra_pcie *pcie)
 {
 	struct dentry *file;
@@ -2292,8 +2360,7 @@ static int tegra_pcie_debugfs_init(struct tegra_pcie *pcie)
 	return 0;
 
 remove:
-	debugfs_remove_recursive(pcie->debugfs);
-	pcie->debugfs = NULL;
+	tegra_pcie_debugfs_exit(pcie);
 	return -ENOMEM;
 }
 
@@ -2311,6 +2378,7 @@ static int tegra_pcie_probe(struct platform_device *pdev)
 
 	pcie = pci_host_bridge_priv(host);
 	host->sysdata = pcie;
+	platform_set_drvdata(pdev, pcie);
 
 	pcie->soc = of_device_get_match_data(dev);
 	INIT_LIST_HEAD(&pcie->ports);
@@ -2388,6 +2456,33 @@ static int tegra_pcie_probe(struct platform_device *pdev)
 	return err;
 }
 
+static int tegra_pcie_remove(struct platform_device *pdev)
+{
+	struct tegra_pcie *pcie = platform_get_drvdata(pdev);
+	struct pci_host_bridge *host = pci_host_bridge_from_priv(pcie);
+	struct tegra_pcie_port *port;
+
+	if (IS_ENABLED(CONFIG_DEBUG_FS))
+		tegra_pcie_debugfs_exit(pcie);
+
+	pci_stop_root_bus(host->bus);
+	pci_remove_root_bus(host->bus);
+
+	list_for_each_entry(port, &pcie->ports, list)
+		tegra_pcie_pme_turnoff(port);
+
+	tegra_pcie_disable_ports(pcie);
+
+	if (IS_ENABLED(CONFIG_PCI_MSI))
+		tegra_pcie_disable_msi(pcie);
+
+	tegra_pcie_free_resources(pcie);
+	tegra_pcie_disable_controller(pcie);
+	tegra_pcie_put_resources(pcie);
+
+	return 0;
+}
+
 static struct platform_driver tegra_pcie_driver = {
 	.driver = {
 		.name = "tegra-pcie",
@@ -2395,5 +2490,7 @@ static struct platform_driver tegra_pcie_driver = {
 		.suppress_bind_attrs = true,
 	},
 	.probe = tegra_pcie_probe,
+	.remove = tegra_pcie_remove,
 };
-builtin_platform_driver(tegra_pcie_driver);
+module_platform_driver(tegra_pcie_driver);
+MODULE_LICENSE("GPL");
-- 
2.1.4

[PATCH V9 3/3] PCI: tegra: Add power management support

[Manikanta Maddireddy]

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@nvidia.com>
Acked-by: Thierry Reding <treding@nvidia.com>
Tested-by: Thierry Reding <treding@nvidia.com>
---
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
V7:
* memory & irq alloc and AFI programming for MSI are split in two functions
V8:
* Rebased on top of latest patch-2 & 3
V9:
* no change in this patch

 drivers/pci/host/pci-tegra.c | 180 +++++++++++++++++++++++++++----------------
 1 file changed, 113 insertions(+), 67 deletions(-)

diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
index 60b1d5e1cfa4..3813820554b2 100644
--- a/drivers/pci/host/pci-tegra.c
+++ b/drivers/pci/host/pci-tegra.c
@@ -1293,31 +1293,25 @@ static int tegra_pcie_get_resources(struct tegra_pcie *pcie)
 		}
 	}
 
-	err = tegra_pcie_power_on(pcie);
-	if (err) {
-		dev_err(dev, "failed to power up: %d\n", err);
-		goto phys_put;
-	}
-
 	pads = platform_get_resource_byname(pdev, IORESOURCE_MEM, "pads");
 	pcie->pads = devm_ioremap_resource(dev, pads);
 	if (IS_ERR(pcie->pads)) {
 		err = PTR_ERR(pcie->pads);
-		goto poweroff;
+		goto phys_put;
 	}
 
 	afi = platform_get_resource_byname(pdev, IORESOURCE_MEM, "afi");
 	pcie->afi = devm_ioremap_resource(dev, afi);
 	if (IS_ERR(pcie->afi)) {
 		err = PTR_ERR(pcie->afi);
-		goto poweroff;
+		goto phys_put;
 	}
 
 	/* request configuration space, but remap later, on demand */
 	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "cs");
 	if (!res) {
 		err = -EADDRNOTAVAIL;
-		goto poweroff;
+		goto phys_put;
 	}
 
 	pcie->cs = *res;
@@ -1328,14 +1322,14 @@ static int tegra_pcie_get_resources(struct tegra_pcie *pcie)
 	pcie->cfg = devm_ioremap_resource(dev, &pcie->cs);
 	if (IS_ERR(pcie->cfg)) {
 		err = PTR_ERR(pcie->cfg);
-		goto poweroff;
+		goto phys_put;
 	}
 
 	/* request interrupt */
 	err = platform_get_irq_byname(pdev, "intr");
 	if (err < 0) {
 		dev_err(dev, "failed to get IRQ: %d\n", err);
-		goto poweroff;
+		goto phys_put;
 	}
 
 	pcie->irq = err;
@@ -1343,7 +1337,7 @@ static int tegra_pcie_get_resources(struct tegra_pcie *pcie)
 	err = request_irq(pcie->irq, tegra_pcie_isr, IRQF_SHARED, "PCIE", pcie);
 	if (err) {
 		dev_err(dev, "failed to register IRQ: %d\n", err);
-		goto poweroff;
+		goto phys_put;
 	}
 
 	return 0;
@@ -1351,8 +1345,6 @@ static int tegra_pcie_get_resources(struct tegra_pcie *pcie)
 phys_put:
 	if (soc->program_uphy)
 		tegra_pcie_phys_put(pcie);
-poweroff:
-	tegra_pcie_power_off(pcie);
 	return err;
 }
 
@@ -1363,8 +1355,6 @@ static int tegra_pcie_put_resources(struct tegra_pcie *pcie)
 	if (pcie->irq > 0)
 		free_irq(pcie->irq, pcie);
 
-	tegra_pcie_power_off(pcie);
-
 	if (soc->program_uphy)
 		tegra_pcie_phys_put(pcie);
 
@@ -1533,15 +1523,13 @@ static const struct irq_domain_ops msi_domain_ops = {
 	.map = tegra_msi_map,
 };
 
-static int tegra_pcie_enable_msi(struct tegra_pcie *pcie)
+static int tegra_pcie_msi_setup(struct tegra_pcie *pcie)
 {
 	struct pci_host_bridge *host = pci_host_bridge_from_priv(pcie);
 	struct platform_device *pdev = to_platform_device(pcie->dev);
-	const struct tegra_pcie_soc *soc = pcie->soc;
 	struct tegra_msi *msi = &pcie->msi;
 	struct device *dev = pcie->dev;
 	int err;
-	u32 reg;
 
 	mutex_init(&msi->lock);
 
@@ -1574,6 +1562,20 @@ static int tegra_pcie_enable_msi(struct tegra_pcie *pcie)
 	/* setup AFI/FPCI range */
 	msi->pages = __get_free_pages(GFP_KERNEL, 0);
 	msi->phys = virt_to_phys((void *)msi->pages);
+	host->msi = &msi->chip;
+
+	return 0;
+
+err:
+	irq_domain_remove(msi->domain);
+	return err;
+}
+
+static void tegra_pcie_enable_msi(struct tegra_pcie *pcie)
+{
+	const struct tegra_pcie_soc *soc = pcie->soc;
+	struct tegra_msi *msi = &pcie->msi;
+	u32 reg;
 
 	afi_writel(pcie, msi->phys >> soc->msi_base_shift, AFI_MSI_FPCI_BAR_ST);
 	afi_writel(pcie, msi->phys, AFI_MSI_AXI_BAR_ST);
@@ -1594,20 +1596,29 @@ static int tegra_pcie_enable_msi(struct tegra_pcie *pcie)
 	reg = afi_readl(pcie, AFI_INTR_MASK);
 	reg |= AFI_INTR_MASK_MSI_MASK;
 	afi_writel(pcie, reg, AFI_INTR_MASK);
+}
 
-	host->msi = &msi->chip;
+static void tegra_pcie_msi_teardown(struct tegra_pcie *pcie)
+{
+	struct tegra_msi *msi = &pcie->msi;
+	unsigned int i, irq;
 
-	return 0;
+	free_pages(msi->pages, 0);
+
+	if (msi->irq > 0)
+		free_irq(msi->irq, pcie);
+
+	for (i = 0; i < INT_PCI_MSI_NR; i++) {
+		irq = irq_find_mapping(msi->domain, i);
+		if (irq > 0)
+			irq_dispose_mapping(irq);
+	}
 
-err:
 	irq_domain_remove(msi->domain);
-	return err;
 }
 
 static int tegra_pcie_disable_msi(struct tegra_pcie *pcie)
 {
-	struct tegra_msi *msi = &pcie->msi;
-	unsigned int i, irq;
 	u32 value;
 
 	/* mask the MSI interrupt */
@@ -1625,19 +1636,6 @@ static int tegra_pcie_disable_msi(struct tegra_pcie *pcie)
 	afi_writel(pcie, 0, AFI_MSI_EN_VEC6);
 	afi_writel(pcie, 0, AFI_MSI_EN_VEC7);
 
-	free_pages(msi->pages, 0);
-
-	if (msi->irq > 0)
-		free_irq(msi->irq, pcie);
-
-	for (i = 0; i < INT_PCI_MSI_NR; i++) {
-		irq = irq_find_mapping(msi->domain, i);
-		if (irq > 0)
-			irq_dispose_mapping(irq);
-	}
-
-	irq_domain_remove(msi->domain);
-
 	return 0;
 }
 
@@ -2136,10 +2134,8 @@ static void tegra_pcie_disable_ports(struct tegra_pcie *pcie)
 {
 	struct tegra_pcie_port *port, *tmp;
 
-	list_for_each_entry_safe(port, tmp, &pcie->ports, list) {
+	list_for_each_entry_safe(port, tmp, &pcie->ports, list)
 		tegra_pcie_port_disable(port);
-		tegra_pcie_port_free(port);
-	}
 }
 
 static const struct tegra_pcie_port_soc tegra20_pcie_ports[] = {
@@ -2394,26 +2390,22 @@ static int tegra_pcie_probe(struct platform_device *pdev)
 		return err;
 	}
 
-	err = tegra_pcie_enable_controller(pcie);
-	if (err)
+	err = tegra_pcie_msi_setup(pcie);
+	if (err < 0) {
+		dev_err(dev, "failed to enable MSI support: %d\n", err);
 		goto put_resources;
+	}
 
-	err = tegra_pcie_request_resources(pcie);
-	if (err)
-		goto disable_controller;
-
-	/* setup the AFI address translations */
-	tegra_pcie_setup_translations(pcie);
-
-	if (IS_ENABLED(CONFIG_PCI_MSI)) {
-		err = tegra_pcie_enable_msi(pcie);
-		if (err < 0) {
-			dev_err(dev, "failed to enable MSI support: %d\n", err);
-			goto free_resources;
-		}
+	pm_runtime_enable(pcie->dev);
+	err = pm_runtime_get_sync(pcie->dev);
+	if (err) {
+		dev_err(dev, "fail to enable pcie controller: %d\n", err);
+		goto teardown_msi;
 	}
 
-	tegra_pcie_enable_ports(pcie);
+	err = tegra_pcie_request_resources(pcie);
+	if (err)
+		goto pm_runtime_put;
 
 	host->busnr = pcie->busn.start;
 	host->dev.parent = &pdev->dev;
@@ -2424,7 +2416,7 @@ static int tegra_pcie_probe(struct platform_device *pdev)
 	err = pci_scan_root_bus_bridge(host);
 	if (err < 0) {
 		dev_err(dev, "failed to register host: %d\n", err);
-		goto disable_ports;
+		goto free_resources;
 	}
 
 	pci_bus_size_bridges(host->bus);
@@ -2443,14 +2435,13 @@ static int tegra_pcie_probe(struct platform_device *pdev)
 
 	return 0;
 
-disable_ports:
-	tegra_pcie_disable_ports(pcie);
-	if (IS_ENABLED(CONFIG_PCI_MSI))
-		tegra_pcie_disable_msi(pcie);
 free_resources:
 	tegra_pcie_free_resources(pcie);
-disable_controller:
-	tegra_pcie_disable_controller(pcie);
+pm_runtime_put:
+	pm_runtime_put_sync(pcie->dev);
+	pm_runtime_disable(pcie->dev);
+teardown_msi:
+	tegra_pcie_msi_teardown(pcie);
 put_resources:
 	tegra_pcie_put_resources(pcie);
 	return err;
@@ -2460,13 +2451,32 @@ static int tegra_pcie_remove(struct platform_device *pdev)
 {
 	struct tegra_pcie *pcie = platform_get_drvdata(pdev);
 	struct pci_host_bridge *host = pci_host_bridge_from_priv(pcie);
-	struct tegra_pcie_port *port;
+	struct tegra_pcie_port *port, *tmp;
 
 	if (IS_ENABLED(CONFIG_DEBUG_FS))
 		tegra_pcie_debugfs_exit(pcie);
 
 	pci_stop_root_bus(host->bus);
 	pci_remove_root_bus(host->bus);
+	tegra_pcie_free_resources(pcie);
+	pm_runtime_put_sync(pcie->dev);
+	pm_runtime_disable(pcie->dev);
+
+	if (IS_ENABLED(CONFIG_PCI_MSI))
+		tegra_pcie_msi_teardown(pcie);
+
+	tegra_pcie_put_resources(pcie);
+
+	list_for_each_entry_safe(port, tmp, &pcie->ports, list)
+		tegra_pcie_port_free(port);
+
+	return 0;
+}
+
+static int tegra_pcie_pm_suspend(struct device *dev)
+{
+	struct tegra_pcie *pcie = dev_get_drvdata(dev);
+	struct tegra_pcie_port *port;
 
 	list_for_each_entry(port, &pcie->ports, list)
 		tegra_pcie_pme_turnoff(port);
@@ -2476,18 +2486,54 @@ static int tegra_pcie_remove(struct platform_device *pdev)
 	if (IS_ENABLED(CONFIG_PCI_MSI))
 		tegra_pcie_disable_msi(pcie);
 
-	tegra_pcie_free_resources(pcie);
 	tegra_pcie_disable_controller(pcie);
-	tegra_pcie_put_resources(pcie);
+	tegra_pcie_power_off(pcie);
+
+	return 0;
+}
+
+static int tegra_pcie_pm_resume(struct device *dev)
+{
+	struct tegra_pcie *pcie = dev_get_drvdata(dev);
+	int err;
+
+	err = tegra_pcie_power_on(pcie);
+	if (err) {
+		dev_err(dev, "tegra pcie power on fail: %d\n", err);
+		return err;
+	}
+	err = tegra_pcie_enable_controller(pcie);
+	if (err) {
+		dev_err(dev, "tegra pcie controller enable fail: %d\n", err);
+		goto poweroff;
+	}
+	tegra_pcie_setup_translations(pcie);
+
+	if (IS_ENABLED(CONFIG_PCI_MSI))
+		tegra_pcie_enable_msi(pcie);
+
+	tegra_pcie_enable_ports(pcie);
 
 	return 0;
+
+poweroff:
+	tegra_pcie_power_off(pcie);
+
+	return err;
 }
 
+static const struct dev_pm_ops tegra_pcie_pm_ops = {
+	SET_RUNTIME_PM_OPS(tegra_pcie_pm_suspend, tegra_pcie_pm_resume, NULL)
+	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(tegra_pcie_pm_suspend,
+				      tegra_pcie_pm_resume)
+};
+
 static struct platform_driver tegra_pcie_driver = {
 	.driver = {
 		.name = "tegra-pcie",
 		.of_match_table = tegra_pcie_of_match,
 		.suppress_bind_attrs = true,
+		.pm = &tegra_pcie_pm_ops,
 	},
 	.probe = tegra_pcie_probe,
 	.remove = tegra_pcie_remove,
-- 
2.1.4

Re: [PATCH V9 3/3] PCI: tegra: Add power management support

[Lorenzo Pieralisi]

On Wed, Feb 28, 2018 at 03:30:34PM +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.

Hi Manikanta,

I think that overall the series is ready to go now but first I have a
question for you and Thierry on this specific patch.

> Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
> Acked-by: Thierry Reding <treding@nvidia.com>
> Tested-by: Thierry Reding <treding@nvidia.com>
> ---
> 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
> V7:
> * memory & irq alloc and AFI programming for MSI are split in two functions
> V8:
> * Rebased on top of latest patch-2 & 3
> V9:
> * no change in this patch
> 
>  drivers/pci/host/pci-tegra.c | 180 +++++++++++++++++++++++++++----------------
>  1 file changed, 113 insertions(+), 67 deletions(-)
> 
> diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
> index 60b1d5e1cfa4..3813820554b2 100644
> --- a/drivers/pci/host/pci-tegra.c
> +++ b/drivers/pci/host/pci-tegra.c
> @@ -1293,31 +1293,25 @@ static int tegra_pcie_get_resources(struct tegra_pcie *pcie)
>  		}
>  	}
>  
> -	err = tegra_pcie_power_on(pcie);
> -	if (err) {
> -		dev_err(dev, "failed to power up: %d\n", err);
> -		goto phys_put;
> -	}
> -
>  	pads = platform_get_resource_byname(pdev, IORESOURCE_MEM, "pads");
>  	pcie->pads = devm_ioremap_resource(dev, pads);
>  	if (IS_ERR(pcie->pads)) {
>  		err = PTR_ERR(pcie->pads);
> -		goto poweroff;
> +		goto phys_put;
>  	}
>  
>  	afi = platform_get_resource_byname(pdev, IORESOURCE_MEM, "afi");
>  	pcie->afi = devm_ioremap_resource(dev, afi);
>  	if (IS_ERR(pcie->afi)) {
>  		err = PTR_ERR(pcie->afi);
> -		goto poweroff;
> +		goto phys_put;
>  	}
>  
>  	/* request configuration space, but remap later, on demand */
>  	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "cs");
>  	if (!res) {
>  		err = -EADDRNOTAVAIL;
> -		goto poweroff;
> +		goto phys_put;
>  	}
>  
>  	pcie->cs = *res;
> @@ -1328,14 +1322,14 @@ static int tegra_pcie_get_resources(struct tegra_pcie *pcie)
>  	pcie->cfg = devm_ioremap_resource(dev, &pcie->cs);
>  	if (IS_ERR(pcie->cfg)) {
>  		err = PTR_ERR(pcie->cfg);
> -		goto poweroff;
> +		goto phys_put;
>  	}
>  
>  	/* request interrupt */
>  	err = platform_get_irq_byname(pdev, "intr");
>  	if (err < 0) {
>  		dev_err(dev, "failed to get IRQ: %d\n", err);
> -		goto poweroff;
> +		goto phys_put;
>  	}
>  
>  	pcie->irq = err;
> @@ -1343,7 +1337,7 @@ static int tegra_pcie_get_resources(struct tegra_pcie *pcie)
>  	err = request_irq(pcie->irq, tegra_pcie_isr, IRQF_SHARED, "PCIE", pcie);
>  	if (err) {
>  		dev_err(dev, "failed to register IRQ: %d\n", err);
> -		goto poweroff;
> +		goto phys_put;
>  	}
>  
>  	return 0;
> @@ -1351,8 +1345,6 @@ static int tegra_pcie_get_resources(struct tegra_pcie *pcie)
>  phys_put:
>  	if (soc->program_uphy)
>  		tegra_pcie_phys_put(pcie);
> -poweroff:
> -	tegra_pcie_power_off(pcie);
>  	return err;
>  }
>  
> @@ -1363,8 +1355,6 @@ static int tegra_pcie_put_resources(struct tegra_pcie *pcie)
>  	if (pcie->irq > 0)
>  		free_irq(pcie->irq, pcie);
>  
> -	tegra_pcie_power_off(pcie);
> -
>  	if (soc->program_uphy)
>  		tegra_pcie_phys_put(pcie);
>  
> @@ -1533,15 +1523,13 @@ static const struct irq_domain_ops msi_domain_ops = {
>  	.map = tegra_msi_map,
>  };
>  
> -static int tegra_pcie_enable_msi(struct tegra_pcie *pcie)
> +static int tegra_pcie_msi_setup(struct tegra_pcie *pcie)
>  {
>  	struct pci_host_bridge *host = pci_host_bridge_from_priv(pcie);
>  	struct platform_device *pdev = to_platform_device(pcie->dev);
> -	const struct tegra_pcie_soc *soc = pcie->soc;
>  	struct tegra_msi *msi = &pcie->msi;
>  	struct device *dev = pcie->dev;
>  	int err;
> -	u32 reg;
>  
>  	mutex_init(&msi->lock);
>  
> @@ -1574,6 +1562,20 @@ static int tegra_pcie_enable_msi(struct tegra_pcie *pcie)
>  	/* setup AFI/FPCI range */
>  	msi->pages = __get_free_pages(GFP_KERNEL, 0);
>  	msi->phys = virt_to_phys((void *)msi->pages);
> +	host->msi = &msi->chip;
> +
> +	return 0;
> +
> +err:
> +	irq_domain_remove(msi->domain);
> +	return err;
> +}
> +
> +static void tegra_pcie_enable_msi(struct tegra_pcie *pcie)
> +{
> +	const struct tegra_pcie_soc *soc = pcie->soc;
> +	struct tegra_msi *msi = &pcie->msi;
> +	u32 reg;
>  
>  	afi_writel(pcie, msi->phys >> soc->msi_base_shift, AFI_MSI_FPCI_BAR_ST);
>  	afi_writel(pcie, msi->phys, AFI_MSI_AXI_BAR_ST);
> @@ -1594,20 +1596,29 @@ static int tegra_pcie_enable_msi(struct tegra_pcie *pcie)
>  	reg = afi_readl(pcie, AFI_INTR_MASK);
>  	reg |= AFI_INTR_MASK_MSI_MASK;
>  	afi_writel(pcie, reg, AFI_INTR_MASK);
> +}
>  
> -	host->msi = &msi->chip;
> +static void tegra_pcie_msi_teardown(struct tegra_pcie *pcie)
> +{
> +	struct tegra_msi *msi = &pcie->msi;
> +	unsigned int i, irq;
>  
> -	return 0;
> +	free_pages(msi->pages, 0);
> +
> +	if (msi->irq > 0)
> +		free_irq(msi->irq, pcie);
> +
> +	for (i = 0; i < INT_PCI_MSI_NR; i++) {
> +		irq = irq_find_mapping(msi->domain, i);
> +		if (irq > 0)
> +			irq_dispose_mapping(irq);
> +	}
>  
> -err:
>  	irq_domain_remove(msi->domain);
> -	return err;
>  }
>  
>  static int tegra_pcie_disable_msi(struct tegra_pcie *pcie)
>  {
> -	struct tegra_msi *msi = &pcie->msi;
> -	unsigned int i, irq;
>  	u32 value;
>  
>  	/* mask the MSI interrupt */
> @@ -1625,19 +1636,6 @@ static int tegra_pcie_disable_msi(struct tegra_pcie *pcie)
>  	afi_writel(pcie, 0, AFI_MSI_EN_VEC6);
>  	afi_writel(pcie, 0, AFI_MSI_EN_VEC7);
>  
> -	free_pages(msi->pages, 0);
> -
> -	if (msi->irq > 0)
> -		free_irq(msi->irq, pcie);
> -
> -	for (i = 0; i < INT_PCI_MSI_NR; i++) {
> -		irq = irq_find_mapping(msi->domain, i);
> -		if (irq > 0)
> -			irq_dispose_mapping(irq);
> -	}
> -
> -	irq_domain_remove(msi->domain);
> -
>  	return 0;
>  }
>  
> @@ -2136,10 +2134,8 @@ static void tegra_pcie_disable_ports(struct tegra_pcie *pcie)
>  {
>  	struct tegra_pcie_port *port, *tmp;
>  
> -	list_for_each_entry_safe(port, tmp, &pcie->ports, list) {
> +	list_for_each_entry_safe(port, tmp, &pcie->ports, list)
>  		tegra_pcie_port_disable(port);
> -		tegra_pcie_port_free(port);
> -	}
>  }
>  
>  static const struct tegra_pcie_port_soc tegra20_pcie_ports[] = {
> @@ -2394,26 +2390,22 @@ static int tegra_pcie_probe(struct platform_device *pdev)
>  		return err;
>  	}
>  
> -	err = tegra_pcie_enable_controller(pcie);
> -	if (err)
> +	err = tegra_pcie_msi_setup(pcie);
> +	if (err < 0) {
> +		dev_err(dev, "failed to enable MSI support: %d\n", err);
>  		goto put_resources;
> +	}
>  
> -	err = tegra_pcie_request_resources(pcie);
> -	if (err)
> -		goto disable_controller;
> -
> -	/* setup the AFI address translations */
> -	tegra_pcie_setup_translations(pcie);
> -
> -	if (IS_ENABLED(CONFIG_PCI_MSI)) {
> -		err = tegra_pcie_enable_msi(pcie);
> -		if (err < 0) {
> -			dev_err(dev, "failed to enable MSI support: %d\n", err);
> -			goto free_resources;
> -		}
> +	pm_runtime_enable(pcie->dev);
> +	err = pm_runtime_get_sync(pcie->dev);
> +	if (err) {
> +		dev_err(dev, "fail to enable pcie controller: %d\n", err);
> +		goto teardown_msi;
>  	}
>  
> -	tegra_pcie_enable_ports(pcie);
> +	err = tegra_pcie_request_resources(pcie);
> +	if (err)
> +		goto pm_runtime_put;
>  
>  	host->busnr = pcie->busn.start;
>  	host->dev.parent = &pdev->dev;
> @@ -2424,7 +2416,7 @@ static int tegra_pcie_probe(struct platform_device *pdev)
>  	err = pci_scan_root_bus_bridge(host);
>  	if (err < 0) {
>  		dev_err(dev, "failed to register host: %d\n", err);
> -		goto disable_ports;
> +		goto free_resources;
>  	}
>  
>  	pci_bus_size_bridges(host->bus);
> @@ -2443,14 +2435,13 @@ static int tegra_pcie_probe(struct platform_device *pdev)
>  
>  	return 0;
>  
> -disable_ports:
> -	tegra_pcie_disable_ports(pcie);
> -	if (IS_ENABLED(CONFIG_PCI_MSI))
> -		tegra_pcie_disable_msi(pcie);
>  free_resources:
>  	tegra_pcie_free_resources(pcie);
> -disable_controller:
> -	tegra_pcie_disable_controller(pcie);
> +pm_runtime_put:
> +	pm_runtime_put_sync(pcie->dev);
> +	pm_runtime_disable(pcie->dev);
> +teardown_msi:
> +	tegra_pcie_msi_teardown(pcie);
>  put_resources:
>  	tegra_pcie_put_resources(pcie);
>  	return err;
> @@ -2460,13 +2451,32 @@ static int tegra_pcie_remove(struct platform_device *pdev)
>  {
>  	struct tegra_pcie *pcie = platform_get_drvdata(pdev);
>  	struct pci_host_bridge *host = pci_host_bridge_from_priv(pcie);
> -	struct tegra_pcie_port *port;
> +	struct tegra_pcie_port *port, *tmp;
>  
>  	if (IS_ENABLED(CONFIG_DEBUG_FS))
>  		tegra_pcie_debugfs_exit(pcie);
>  
>  	pci_stop_root_bus(host->bus);
>  	pci_remove_root_bus(host->bus);
> +	tegra_pcie_free_resources(pcie);
> +	pm_runtime_put_sync(pcie->dev);
> +	pm_runtime_disable(pcie->dev);
> +
> +	if (IS_ENABLED(CONFIG_PCI_MSI))
> +		tegra_pcie_msi_teardown(pcie);
> +
> +	tegra_pcie_put_resources(pcie);
> +
> +	list_for_each_entry_safe(port, tmp, &pcie->ports, list)
> +		tegra_pcie_port_free(port);
> +
> +	return 0;
> +}
> +
> +static int tegra_pcie_pm_suspend(struct device *dev)
> +{
> +	struct tegra_pcie *pcie = dev_get_drvdata(dev);
> +	struct tegra_pcie_port *port;
>  
>  	list_for_each_entry(port, &pcie->ports, list)
>  		tegra_pcie_pme_turnoff(port);
> @@ -2476,18 +2486,54 @@ static int tegra_pcie_remove(struct platform_device *pdev)
>  	if (IS_ENABLED(CONFIG_PCI_MSI))
>  		tegra_pcie_disable_msi(pcie);
>  
> -	tegra_pcie_free_resources(pcie);
>  	tegra_pcie_disable_controller(pcie);
> -	tegra_pcie_put_resources(pcie);
> +	tegra_pcie_power_off(pcie);
> +
> +	return 0;
> +}
> +
> +static int tegra_pcie_pm_resume(struct device *dev)
> +{
> +	struct tegra_pcie *pcie = dev_get_drvdata(dev);
> +	int err;
> +
> +	err = tegra_pcie_power_on(pcie);
> +	if (err) {
> +		dev_err(dev, "tegra pcie power on fail: %d\n", err);
> +		return err;
> +	}
> +	err = tegra_pcie_enable_controller(pcie);
> +	if (err) {
> +		dev_err(dev, "tegra pcie controller enable fail: %d\n", err);
> +		goto poweroff;
> +	}
> +	tegra_pcie_setup_translations(pcie);
> +
> +	if (IS_ENABLED(CONFIG_PCI_MSI))
> +		tegra_pcie_enable_msi(pcie);
> +
> +	tegra_pcie_enable_ports(pcie);

Is it correct to report a successfull resume if some of the ports fail
to come up (whether within runtime PM or system suspend) ? I think that,
if any of the ports fails to come up, returning a failure is more
appropriate here given that the host bridge is a single device as far as
the kernel is concerned.

Thanks,
Lorenzo

>  
>  	return 0;
> +
> +poweroff:
> +	tegra_pcie_power_off(pcie);
> +
> +	return err;
>  }
>  
> +static const struct dev_pm_ops tegra_pcie_pm_ops = {
> +	SET_RUNTIME_PM_OPS(tegra_pcie_pm_suspend, tegra_pcie_pm_resume, NULL)
> +	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(tegra_pcie_pm_suspend,
> +				      tegra_pcie_pm_resume)
> +};
> +
>  static struct platform_driver tegra_pcie_driver = {
>  	.driver = {
>  		.name = "tegra-pcie",
>  		.of_match_table = tegra_pcie_of_match,
>  		.suppress_bind_attrs = true,
> +		.pm = &tegra_pcie_pm_ops,
>  	},
>  	.probe = tegra_pcie_probe,
>  	.remove = tegra_pcie_remove,
> -- 
> 2.1.4
> 

Re: [PATCH V9 3/3] PCI: tegra: Add power management support

[Manikanta Maddireddy]

On 02-Mar-18 10:06 PM, Lorenzo Pieralisi wrote:
> On Wed, Feb 28, 2018 at 03:30:34PM +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.
> 
> Hi Manikanta,
> 
> I think that overall the series is ready to go now but first I have a
> question for you and Thierry on this specific patch.
> 
>> Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
>> Acked-by: Thierry Reding <treding@nvidia.com>
>> Tested-by: Thierry Reding <treding@nvidia.com>
>> ---
>> 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
>> V7:
>> * memory & irq alloc and AFI programming for MSI are split in two functions
>> V8:
>> * Rebased on top of latest patch-2 & 3
>> V9:
>> * no change in this patch
>>
>>  drivers/pci/host/pci-tegra.c | 180 +++++++++++++++++++++++++++----------------
>>  1 file changed, 113 insertions(+), 67 deletions(-)
>>
>> diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
>> index 60b1d5e1cfa4..3813820554b2 100644
>> --- a/drivers/pci/host/pci-tegra.c
>> +++ b/drivers/pci/host/pci-tegra.c
>> @@ -1293,31 +1293,25 @@ static int tegra_pcie_get_resources(struct tegra_pcie *pcie)
>>  		}
>>  	}
>>  
>> -	err = tegra_pcie_power_on(pcie);
>> -	if (err) {
>> -		dev_err(dev, "failed to power up: %d\n", err);
>> -		goto phys_put;
>> -	}
>> -
>>  	pads = platform_get_resource_byname(pdev, IORESOURCE_MEM, "pads");
>>  	pcie->pads = devm_ioremap_resource(dev, pads);
>>  	if (IS_ERR(pcie->pads)) {
>>  		err = PTR_ERR(pcie->pads);
>> -		goto poweroff;
>> +		goto phys_put;
>>  	}
>>  
>>  	afi = platform_get_resource_byname(pdev, IORESOURCE_MEM, "afi");
>>  	pcie->afi = devm_ioremap_resource(dev, afi);
>>  	if (IS_ERR(pcie->afi)) {
>>  		err = PTR_ERR(pcie->afi);
>> -		goto poweroff;
>> +		goto phys_put;
>>  	}
>>  
>>  	/* request configuration space, but remap later, on demand */
>>  	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "cs");
>>  	if (!res) {
>>  		err = -EADDRNOTAVAIL;
>> -		goto poweroff;
>> +		goto phys_put;
>>  	}
>>  
>>  	pcie->cs = *res;
>> @@ -1328,14 +1322,14 @@ static int tegra_pcie_get_resources(struct tegra_pcie *pcie)
>>  	pcie->cfg = devm_ioremap_resource(dev, &pcie->cs);
>>  	if (IS_ERR(pcie->cfg)) {
>>  		err = PTR_ERR(pcie->cfg);
>> -		goto poweroff;
>> +		goto phys_put;
>>  	}
>>  
>>  	/* request interrupt */
>>  	err = platform_get_irq_byname(pdev, "intr");
>>  	if (err < 0) {
>>  		dev_err(dev, "failed to get IRQ: %d\n", err);
>> -		goto poweroff;
>> +		goto phys_put;
>>  	}
>>  
>>  	pcie->irq = err;
>> @@ -1343,7 +1337,7 @@ static int tegra_pcie_get_resources(struct tegra_pcie *pcie)
>>  	err = request_irq(pcie->irq, tegra_pcie_isr, IRQF_SHARED, "PCIE", pcie);
>>  	if (err) {
>>  		dev_err(dev, "failed to register IRQ: %d\n", err);
>> -		goto poweroff;
>> +		goto phys_put;
>>  	}
>>  
>>  	return 0;
>> @@ -1351,8 +1345,6 @@ static int tegra_pcie_get_resources(struct tegra_pcie *pcie)
>>  phys_put:
>>  	if (soc->program_uphy)
>>  		tegra_pcie_phys_put(pcie);
>> -poweroff:
>> -	tegra_pcie_power_off(pcie);
>>  	return err;
>>  }
>>  
>> @@ -1363,8 +1355,6 @@ static int tegra_pcie_put_resources(struct tegra_pcie *pcie)
>>  	if (pcie->irq > 0)
>>  		free_irq(pcie->irq, pcie);
>>  
>> -	tegra_pcie_power_off(pcie);
>> -
>>  	if (soc->program_uphy)
>>  		tegra_pcie_phys_put(pcie);
>>  
>> @@ -1533,15 +1523,13 @@ static const struct irq_domain_ops msi_domain_ops = {
>>  	.map = tegra_msi_map,
>>  };
>>  
>> -static int tegra_pcie_enable_msi(struct tegra_pcie *pcie)
>> +static int tegra_pcie_msi_setup(struct tegra_pcie *pcie)
>>  {
>>  	struct pci_host_bridge *host = pci_host_bridge_from_priv(pcie);
>>  	struct platform_device *pdev = to_platform_device(pcie->dev);
>> -	const struct tegra_pcie_soc *soc = pcie->soc;
>>  	struct tegra_msi *msi = &pcie->msi;
>>  	struct device *dev = pcie->dev;
>>  	int err;
>> -	u32 reg;
>>  
>>  	mutex_init(&msi->lock);
>>  
>> @@ -1574,6 +1562,20 @@ static int tegra_pcie_enable_msi(struct tegra_pcie *pcie)
>>  	/* setup AFI/FPCI range */
>>  	msi->pages = __get_free_pages(GFP_KERNEL, 0);
>>  	msi->phys = virt_to_phys((void *)msi->pages);
>> +	host->msi = &msi->chip;
>> +
>> +	return 0;
>> +
>> +err:
>> +	irq_domain_remove(msi->domain);
>> +	return err;
>> +}
>> +
>> +static void tegra_pcie_enable_msi(struct tegra_pcie *pcie)
>> +{
>> +	const struct tegra_pcie_soc *soc = pcie->soc;
>> +	struct tegra_msi *msi = &pcie->msi;
>> +	u32 reg;
>>  
>>  	afi_writel(pcie, msi->phys >> soc->msi_base_shift, AFI_MSI_FPCI_BAR_ST);
>>  	afi_writel(pcie, msi->phys, AFI_MSI_AXI_BAR_ST);
>> @@ -1594,20 +1596,29 @@ static int tegra_pcie_enable_msi(struct tegra_pcie *pcie)
>>  	reg = afi_readl(pcie, AFI_INTR_MASK);
>>  	reg |= AFI_INTR_MASK_MSI_MASK;
>>  	afi_writel(pcie, reg, AFI_INTR_MASK);
>> +}
>>  
>> -	host->msi = &msi->chip;
>> +static void tegra_pcie_msi_teardown(struct tegra_pcie *pcie)
>> +{
>> +	struct tegra_msi *msi = &pcie->msi;
>> +	unsigned int i, irq;
>>  
>> -	return 0;
>> +	free_pages(msi->pages, 0);
>> +
>> +	if (msi->irq > 0)
>> +		free_irq(msi->irq, pcie);
>> +
>> +	for (i = 0; i < INT_PCI_MSI_NR; i++) {
>> +		irq = irq_find_mapping(msi->domain, i);
>> +		if (irq > 0)
>> +			irq_dispose_mapping(irq);
>> +	}
>>  
>> -err:
>>  	irq_domain_remove(msi->domain);
>> -	return err;
>>  }
>>  
>>  static int tegra_pcie_disable_msi(struct tegra_pcie *pcie)
>>  {
>> -	struct tegra_msi *msi = &pcie->msi;
>> -	unsigned int i, irq;
>>  	u32 value;
>>  
>>  	/* mask the MSI interrupt */
>> @@ -1625,19 +1636,6 @@ static int tegra_pcie_disable_msi(struct tegra_pcie *pcie)
>>  	afi_writel(pcie, 0, AFI_MSI_EN_VEC6);
>>  	afi_writel(pcie, 0, AFI_MSI_EN_VEC7);
>>  
>> -	free_pages(msi->pages, 0);
>> -
>> -	if (msi->irq > 0)
>> -		free_irq(msi->irq, pcie);
>> -
>> -	for (i = 0; i < INT_PCI_MSI_NR; i++) {
>> -		irq = irq_find_mapping(msi->domain, i);
>> -		if (irq > 0)
>> -			irq_dispose_mapping(irq);
>> -	}
>> -
>> -	irq_domain_remove(msi->domain);
>> -
>>  	return 0;
>>  }
>>  
>> @@ -2136,10 +2134,8 @@ static void tegra_pcie_disable_ports(struct tegra_pcie *pcie)
>>  {
>>  	struct tegra_pcie_port *port, *tmp;
>>  
>> -	list_for_each_entry_safe(port, tmp, &pcie->ports, list) {
>> +	list_for_each_entry_safe(port, tmp, &pcie->ports, list)
>>  		tegra_pcie_port_disable(port);
>> -		tegra_pcie_port_free(port);
>> -	}
>>  }
>>  
>>  static const struct tegra_pcie_port_soc tegra20_pcie_ports[] = {
>> @@ -2394,26 +2390,22 @@ static int tegra_pcie_probe(struct platform_device *pdev)
>>  		return err;
>>  	}
>>  
>> -	err = tegra_pcie_enable_controller(pcie);
>> -	if (err)
>> +	err = tegra_pcie_msi_setup(pcie);
>> +	if (err < 0) {
>> +		dev_err(dev, "failed to enable MSI support: %d\n", err);
>>  		goto put_resources;
>> +	}
>>  
>> -	err = tegra_pcie_request_resources(pcie);
>> -	if (err)
>> -		goto disable_controller;
>> -
>> -	/* setup the AFI address translations */
>> -	tegra_pcie_setup_translations(pcie);
>> -
>> -	if (IS_ENABLED(CONFIG_PCI_MSI)) {
>> -		err = tegra_pcie_enable_msi(pcie);
>> -		if (err < 0) {
>> -			dev_err(dev, "failed to enable MSI support: %d\n", err);
>> -			goto free_resources;
>> -		}
>> +	pm_runtime_enable(pcie->dev);
>> +	err = pm_runtime_get_sync(pcie->dev);
>> +	if (err) {
>> +		dev_err(dev, "fail to enable pcie controller: %d\n", err);
>> +		goto teardown_msi;
>>  	}
>>  
>> -	tegra_pcie_enable_ports(pcie);
>> +	err = tegra_pcie_request_resources(pcie);
>> +	if (err)
>> +		goto pm_runtime_put;
>>  
>>  	host->busnr = pcie->busn.start;
>>  	host->dev.parent = &pdev->dev;
>> @@ -2424,7 +2416,7 @@ static int tegra_pcie_probe(struct platform_device *pdev)
>>  	err = pci_scan_root_bus_bridge(host);
>>  	if (err < 0) {
>>  		dev_err(dev, "failed to register host: %d\n", err);
>> -		goto disable_ports;
>> +		goto free_resources;
>>  	}
>>  
>>  	pci_bus_size_bridges(host->bus);
>> @@ -2443,14 +2435,13 @@ static int tegra_pcie_probe(struct platform_device *pdev)
>>  
>>  	return 0;
>>  
>> -disable_ports:
>> -	tegra_pcie_disable_ports(pcie);
>> -	if (IS_ENABLED(CONFIG_PCI_MSI))
>> -		tegra_pcie_disable_msi(pcie);
>>  free_resources:
>>  	tegra_pcie_free_resources(pcie);
>> -disable_controller:
>> -	tegra_pcie_disable_controller(pcie);
>> +pm_runtime_put:
>> +	pm_runtime_put_sync(pcie->dev);
>> +	pm_runtime_disable(pcie->dev);
>> +teardown_msi:
>> +	tegra_pcie_msi_teardown(pcie);
>>  put_resources:
>>  	tegra_pcie_put_resources(pcie);
>>  	return err;
>> @@ -2460,13 +2451,32 @@ static int tegra_pcie_remove(struct platform_device *pdev)
>>  {
>>  	struct tegra_pcie *pcie = platform_get_drvdata(pdev);
>>  	struct pci_host_bridge *host = pci_host_bridge_from_priv(pcie);
>> -	struct tegra_pcie_port *port;
>> +	struct tegra_pcie_port *port, *tmp;
>>  
>>  	if (IS_ENABLED(CONFIG_DEBUG_FS))
>>  		tegra_pcie_debugfs_exit(pcie);
>>  
>>  	pci_stop_root_bus(host->bus);
>>  	pci_remove_root_bus(host->bus);
>> +	tegra_pcie_free_resources(pcie);
>> +	pm_runtime_put_sync(pcie->dev);
>> +	pm_runtime_disable(pcie->dev);
>> +
>> +	if (IS_ENABLED(CONFIG_PCI_MSI))
>> +		tegra_pcie_msi_teardown(pcie);
>> +
>> +	tegra_pcie_put_resources(pcie);
>> +
>> +	list_for_each_entry_safe(port, tmp, &pcie->ports, list)
>> +		tegra_pcie_port_free(port);
>> +
>> +	return 0;
>> +}
>> +
>> +static int tegra_pcie_pm_suspend(struct device *dev)
>> +{
>> +	struct tegra_pcie *pcie = dev_get_drvdata(dev);
>> +	struct tegra_pcie_port *port;
>>  
>>  	list_for_each_entry(port, &pcie->ports, list)
>>  		tegra_pcie_pme_turnoff(port);
>> @@ -2476,18 +2486,54 @@ static int tegra_pcie_remove(struct platform_device *pdev)
>>  	if (IS_ENABLED(CONFIG_PCI_MSI))
>>  		tegra_pcie_disable_msi(pcie);
>>  
>> -	tegra_pcie_free_resources(pcie);
>>  	tegra_pcie_disable_controller(pcie);
>> -	tegra_pcie_put_resources(pcie);
>> +	tegra_pcie_power_off(pcie);
>> +
>> +	return 0;
>> +}
>> +
>> +static int tegra_pcie_pm_resume(struct device *dev)
>> +{
>> +	struct tegra_pcie *pcie = dev_get_drvdata(dev);
>> +	int err;
>> +
>> +	err = tegra_pcie_power_on(pcie);
>> +	if (err) {
>> +		dev_err(dev, "tegra pcie power on fail: %d\n", err);
>> +		return err;
>> +	}
>> +	err = tegra_pcie_enable_controller(pcie);
>> +	if (err) {
>> +		dev_err(dev, "tegra pcie controller enable fail: %d\n", err);
>> +		goto poweroff;
>> +	}
>> +	tegra_pcie_setup_translations(pcie);
>> +
>> +	if (IS_ENABLED(CONFIG_PCI_MSI))
>> +		tegra_pcie_enable_msi(pcie);
>> +
>> +	tegra_pcie_enable_ports(pcie);
> 
> Is it correct to report a successfull resume if some of the ports fail
> to come up (whether within runtime PM or system suspend) ? I think that,
> if any of the ports fails to come up, returning a failure is more
> appropriate here given that the host bridge is a single device as far as
> the kernel is concerned.
> 
> Thanks,
> Lorenzo
> 

Hi Lorenzo,

We(Thierry, Vidya Sagar and myself) had a discussion on this topic
and we chose not to fail probe/resume because host controller is
initialized properly, so failing the probe/resume is not the right
thing to do.

PCIe link may fail to come up if there is no endpoint in the slot
or interoperable failure where endpoint specific quirk or work
around required. In such cases host controller driver shouldn't
fail.

Also user can connect endpoint in only one slot and expect it to be
working. Even though host bridge is a single device, driver should
allow standalone port to work.

Thanks,
Manikanta

>>  
>>  	return 0;
>> +
>> +poweroff:
>> +	tegra_pcie_power_off(pcie);
>> +
>> +	return err;
>>  }
>>  
>> +static const struct dev_pm_ops tegra_pcie_pm_ops = {
>> +	SET_RUNTIME_PM_OPS(tegra_pcie_pm_suspend, tegra_pcie_pm_resume, NULL)
>> +	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(tegra_pcie_pm_suspend,
>> +				      tegra_pcie_pm_resume)
>> +};
>> +
>>  static struct platform_driver tegra_pcie_driver = {
>>  	.driver = {
>>  		.name = "tegra-pcie",
>>  		.of_match_table = tegra_pcie_of_match,
>>  		.suppress_bind_attrs = true,
>> +		.pm = &tegra_pcie_pm_ops,
>>  	},
>>  	.probe = tegra_pcie_probe,
>>  	.remove = tegra_pcie_remove,
>> -- 
>> 2.1.4
>>

Re: [PATCH V9 3/3] PCI: tegra: Add power management support

[Lorenzo Pieralisi]

On Sat, Mar 03, 2018 at 01:47:38PM +0530, Manikanta Maddireddy wrote:

[...]

> >> +static int tegra_pcie_pm_resume(struct device *dev)
> >> +{
> >> +	struct tegra_pcie *pcie = dev_get_drvdata(dev);
> >> +	int err;
> >> +
> >> +	err = tegra_pcie_power_on(pcie);
> >> +	if (err) {
> >> +		dev_err(dev, "tegra pcie power on fail: %d\n", err);
> >> +		return err;
> >> +	}
> >> +	err = tegra_pcie_enable_controller(pcie);
> >> +	if (err) {
> >> +		dev_err(dev, "tegra pcie controller enable fail: %d\n", err);
> >> +		goto poweroff;
> >> +	}
> >> +	tegra_pcie_setup_translations(pcie);
> >> +
> >> +	if (IS_ENABLED(CONFIG_PCI_MSI))
> >> +		tegra_pcie_enable_msi(pcie);
> >> +
> >> +	tegra_pcie_enable_ports(pcie);
> > 
> > Is it correct to report a successfull resume if some of the ports fail
> > to come up (whether within runtime PM or system suspend) ? I think that,
> > if any of the ports fails to come up, returning a failure is more
> > appropriate here given that the host bridge is a single device as far as
> > the kernel is concerned.
> > 
> > Thanks,
> > Lorenzo
> > 
> 
> Hi Lorenzo,
> 
> We(Thierry, Vidya Sagar and myself) had a discussion on this topic
> and we chose not to fail probe/resume because host controller is
> initialized properly, so failing the probe/resume is not the right
> thing to do.

What happens then to endpoints downstream a rootport that failed to
resume but we nonetheless reported that it successfully resume instead ?

Thanks,
Lorenzo

> PCIe link may fail to come up if there is no endpoint in the slot
> or interoperable failure where endpoint specific quirk or work
> around required. In such cases host controller driver shouldn't
> fail.
> 
> Also user can connect endpoint in only one slot and expect it to be
> working. Even though host bridge is a single device, driver should
> allow standalone port to work.
> 
> Thanks,
> Manikanta
> 
> >>  
> >>  	return 0;
> >> +
> >> +poweroff:
> >> +	tegra_pcie_power_off(pcie);
> >> +
> >> +	return err;
> >>  }
> >>  
> >> +static const struct dev_pm_ops tegra_pcie_pm_ops = {
> >> +	SET_RUNTIME_PM_OPS(tegra_pcie_pm_suspend, tegra_pcie_pm_resume, NULL)
> >> +	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(tegra_pcie_pm_suspend,
> >> +				      tegra_pcie_pm_resume)
> >> +};
> >> +
> >>  static struct platform_driver tegra_pcie_driver = {
> >>  	.driver = {
> >>  		.name = "tegra-pcie",
> >>  		.of_match_table = tegra_pcie_of_match,
> >>  		.suppress_bind_attrs = true,
> >> +		.pm = &tegra_pcie_pm_ops,
> >>  	},
> >>  	.probe = tegra_pcie_probe,
> >>  	.remove = tegra_pcie_remove,
> >> -- 
> >> 2.1.4
> >>

Re: [PATCH V9 3/3] PCI: tegra: Add power management support

[Manikanta Maddireddy]

On 05-Mar-18 3:11 PM, Lorenzo Pieralisi wrote:
> On Sat, Mar 03, 2018 at 01:47:38PM +0530, Manikanta Maddireddy wrote:
> 
> [...]
> 
>>>> +static int tegra_pcie_pm_resume(struct device *dev)
>>>> +{
>>>> +	struct tegra_pcie *pcie = dev_get_drvdata(dev);
>>>> +	int err;
>>>> +
>>>> +	err = tegra_pcie_power_on(pcie);
>>>> +	if (err) {
>>>> +		dev_err(dev, "tegra pcie power on fail: %d\n", err);
>>>> +		return err;
>>>> +	}
>>>> +	err = tegra_pcie_enable_controller(pcie);
>>>> +	if (err) {
>>>> +		dev_err(dev, "tegra pcie controller enable fail: %d\n", err);
>>>> +		goto poweroff;
>>>> +	}
>>>> +	tegra_pcie_setup_translations(pcie);
>>>> +
>>>> +	if (IS_ENABLED(CONFIG_PCI_MSI))
>>>> +		tegra_pcie_enable_msi(pcie);
>>>> +
>>>> +	tegra_pcie_enable_ports(pcie);
>>>
>>> Is it correct to report a successfull resume if some of the ports fail
>>> to come up (whether within runtime PM or system suspend) ? I think that,
>>> if any of the ports fails to come up, returning a failure is more
>>> appropriate here given that the host bridge is a single device as far as
>>> the kernel is concerned.
>>>
>>> Thanks,
>>> Lorenzo
>>>
>>
>> Hi Lorenzo,
>>
>> We(Thierry, Vidya Sagar and myself) had a discussion on this topic
>> and we chose not to fail probe/resume because host controller is
>> initialized properly, so failing the probe/resume is not the right
>> thing to do.
> 
> What happens then to endpoints downstream a rootport that failed to
> resume but we nonetheless reported that it successfully resume instead ?
> 
> Thanks,
> Lorenzo
> 

Hi Lorenzo,

If an endpoint comes up in probe & enumeration is completed and if
the same endpoint doesn't come up during resume I am observing
PCIe errors because PCIe enumeration hierarchy is unchanged &
PCI subsystem is trying to restore PCI device. This observation
is same if host driver returns error.

To handle this case driver needs to remove the respective
PCIe port enumeration hierarchy if link doesn't come up
in resume. It will tricky to identify PCI bus structure
from host port ID in resume, may be some book keeping required
here.

However if the PCIe link comes up in probe, it should come up
during resume as well else it will be a bug. Do you see the
need for changing the PCIe enumeration hierarchy in resume
based on link up status?

Thanks,
Manikanta

>> PCIe link may fail to come up if there is no endpoint in the slot
>> or interoperable failure where endpoint specific quirk or work
>> around required. In such cases host controller driver shouldn't
>> fail.
>>
>> Also user can connect endpoint in only one slot and expect it to be
>> working. Even though host bridge is a single device, driver should
>> allow standalone port to work.
>>
>> Thanks,
>> Manikanta
>>
>>>>  
>>>>  	return 0;
>>>> +
>>>> +poweroff:
>>>> +	tegra_pcie_power_off(pcie);
>>>> +
>>>> +	return err;
>>>>  }
>>>>  
>>>> +static const struct dev_pm_ops tegra_pcie_pm_ops = {
>>>> +	SET_RUNTIME_PM_OPS(tegra_pcie_pm_suspend, tegra_pcie_pm_resume, NULL)
>>>> +	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(tegra_pcie_pm_suspend,
>>>> +				      tegra_pcie_pm_resume)
>>>> +};
>>>> +
>>>>  static struct platform_driver tegra_pcie_driver = {
>>>>  	.driver = {
>>>>  		.name = "tegra-pcie",
>>>>  		.of_match_table = tegra_pcie_of_match,
>>>>  		.suppress_bind_attrs = true,
>>>> +		.pm = &tegra_pcie_pm_ops,
>>>>  	},
>>>>  	.probe = tegra_pcie_probe,
>>>>  	.remove = tegra_pcie_remove,
>>>> -- 
>>>> 2.1.4
>>>>

Re: [PATCH V9 3/3] PCI: tegra: Add power management support

[Lorenzo Pieralisi]

On Tue, Mar 06, 2018 at 03:26:33PM +0530, Manikanta Maddireddy wrote:
> 
> 
> On 05-Mar-18 3:11 PM, Lorenzo Pieralisi wrote:
> > On Sat, Mar 03, 2018 at 01:47:38PM +0530, Manikanta Maddireddy wrote:
> > 
> > [...]
> > 
> >>>> +static int tegra_pcie_pm_resume(struct device *dev)
> >>>> +{
> >>>> +	struct tegra_pcie *pcie = dev_get_drvdata(dev);
> >>>> +	int err;
> >>>> +
> >>>> +	err = tegra_pcie_power_on(pcie);
> >>>> +	if (err) {
> >>>> +		dev_err(dev, "tegra pcie power on fail: %d\n", err);
> >>>> +		return err;
> >>>> +	}
> >>>> +	err = tegra_pcie_enable_controller(pcie);
> >>>> +	if (err) {
> >>>> +		dev_err(dev, "tegra pcie controller enable fail: %d\n", err);
> >>>> +		goto poweroff;
> >>>> +	}
> >>>> +	tegra_pcie_setup_translations(pcie);
> >>>> +
> >>>> +	if (IS_ENABLED(CONFIG_PCI_MSI))
> >>>> +		tegra_pcie_enable_msi(pcie);
> >>>> +
> >>>> +	tegra_pcie_enable_ports(pcie);
> >>>
> >>> Is it correct to report a successfull resume if some of the ports fail
> >>> to come up (whether within runtime PM or system suspend) ? I think that,
> >>> if any of the ports fails to come up, returning a failure is more
> >>> appropriate here given that the host bridge is a single device as far as
> >>> the kernel is concerned.
> >>>
> >>> Thanks,
> >>> Lorenzo
> >>>
> >>
> >> Hi Lorenzo,
> >>
> >> We(Thierry, Vidya Sagar and myself) had a discussion on this topic
> >> and we chose not to fail probe/resume because host controller is
> >> initialized properly, so failing the probe/resume is not the right
> >> thing to do.
> > 
> > What happens then to endpoints downstream a rootport that failed to
> > resume but we nonetheless reported that it successfully resume instead ?
> > 
> > Thanks,
> > Lorenzo
> > 
> 
> Hi Lorenzo,
> 
> If an endpoint comes up in probe & enumeration is completed and if
> the same endpoint doesn't come up during resume I am observing
> PCIe errors because PCIe enumeration hierarchy is unchanged &
> PCI subsystem is trying to restore PCI device. This observation
> is same if host driver returns error.
> 
> To handle this case driver needs to remove the respective
> PCIe port enumeration hierarchy if link doesn't come up
> in resume. It will tricky to identify PCI bus structure
> from host port ID in resume, may be some book keeping required
> here.
> 
> However if the PCIe link comes up in probe, it should come up
> during resume as well else it will be a bug. Do you see the
> need for changing the PCIe enumeration hierarchy in resume
> based on link up status?

I think that for the time being it is fine to merge the current
set - I asked just for clarification; I agree there is no easy answer
to the issue I raised. It should be safe to merge the series in its
current form - I will keep this issue on my radar to see if there
is something we can do to improve it.

Thanks,
Lorenzo

> 
> Thanks,
> Manikanta
> 
> >> PCIe link may fail to come up if there is no endpoint in the slot
> >> or interoperable failure where endpoint specific quirk or work
> >> around required. In such cases host controller driver shouldn't
> >> fail.
> >>
> >> Also user can connect endpoint in only one slot and expect it to be
> >> working. Even though host bridge is a single device, driver should
> >> allow standalone port to work.
> >>
> >> Thanks,
> >> Manikanta
> >>
> >>>>  
> >>>>  	return 0;
> >>>> +
> >>>> +poweroff:
> >>>> +	tegra_pcie_power_off(pcie);
> >>>> +
> >>>> +	return err;
> >>>>  }
> >>>>  
> >>>> +static const struct dev_pm_ops tegra_pcie_pm_ops = {
> >>>> +	SET_RUNTIME_PM_OPS(tegra_pcie_pm_suspend, tegra_pcie_pm_resume, NULL)
> >>>> +	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(tegra_pcie_pm_suspend,
> >>>> +				      tegra_pcie_pm_resume)
> >>>> +};
> >>>> +
> >>>>  static struct platform_driver tegra_pcie_driver = {
> >>>>  	.driver = {
> >>>>  		.name = "tegra-pcie",
> >>>>  		.of_match_table = tegra_pcie_of_match,
> >>>>  		.suppress_bind_attrs = true,
> >>>> +		.pm = &tegra_pcie_pm_ops,
> >>>>  	},
> >>>>  	.probe = tegra_pcie_probe,
> >>>>  	.remove = tegra_pcie_remove,
> >>>> -- 
> >>>> 2.1.4
> >>>>

Re: [PATCH V9 3/3] PCI: tegra: Add power management support

[Manikanta Maddireddy]

On 06-Mar-18 5:29 PM, Lorenzo Pieralisi wrote:
> On Tue, Mar 06, 2018 at 03:26:33PM +0530, Manikanta Maddireddy wrote:
>>
>>
>> On 05-Mar-18 3:11 PM, Lorenzo Pieralisi wrote:
>>> On Sat, Mar 03, 2018 at 01:47:38PM +0530, Manikanta Maddireddy wrote:
>>>
>>> [...]
>>>
>>>>>> +static int tegra_pcie_pm_resume(struct device *dev)
>>>>>> +{
>>>>>> +	struct tegra_pcie *pcie = dev_get_drvdata(dev);
>>>>>> +	int err;
>>>>>> +
>>>>>> +	err = tegra_pcie_power_on(pcie);
>>>>>> +	if (err) {
>>>>>> +		dev_err(dev, "tegra pcie power on fail: %d\n", err);
>>>>>> +		return err;
>>>>>> +	}
>>>>>> +	err = tegra_pcie_enable_controller(pcie);
>>>>>> +	if (err) {
>>>>>> +		dev_err(dev, "tegra pcie controller enable fail: %d\n", err);
>>>>>> +		goto poweroff;
>>>>>> +	}
>>>>>> +	tegra_pcie_setup_translations(pcie);
>>>>>> +
>>>>>> +	if (IS_ENABLED(CONFIG_PCI_MSI))
>>>>>> +		tegra_pcie_enable_msi(pcie);
>>>>>> +
>>>>>> +	tegra_pcie_enable_ports(pcie);
>>>>>
>>>>> Is it correct to report a successfull resume if some of the ports fail
>>>>> to come up (whether within runtime PM or system suspend) ? I think that,
>>>>> if any of the ports fails to come up, returning a failure is more
>>>>> appropriate here given that the host bridge is a single device as far as
>>>>> the kernel is concerned.
>>>>>
>>>>> Thanks,
>>>>> Lorenzo
>>>>>
>>>>
>>>> Hi Lorenzo,
>>>>
>>>> We(Thierry, Vidya Sagar and myself) had a discussion on this topic
>>>> and we chose not to fail probe/resume because host controller is
>>>> initialized properly, so failing the probe/resume is not the right
>>>> thing to do.
>>>
>>> What happens then to endpoints downstream a rootport that failed to
>>> resume but we nonetheless reported that it successfully resume instead ?
>>>
>>> Thanks,
>>> Lorenzo
>>>
>>
>> Hi Lorenzo,
>>
>> If an endpoint comes up in probe & enumeration is completed and if
>> the same endpoint doesn't come up during resume I am observing
>> PCIe errors because PCIe enumeration hierarchy is unchanged &
>> PCI subsystem is trying to restore PCI device. This observation
>> is same if host driver returns error.
>>
>> To handle this case driver needs to remove the respective
>> PCIe port enumeration hierarchy if link doesn't come up
>> in resume. It will tricky to identify PCI bus structure
>> from host port ID in resume, may be some book keeping required
>> here.
>>
>> However if the PCIe link comes up in probe, it should come up
>> during resume as well else it will be a bug. Do you see the
>> need for changing the PCIe enumeration hierarchy in resume
>> based on link up status?
> 
> I think that for the time being it is fine to merge the current
> set - I asked just for clarification; I agree there is no easy answer
> to the issue I raised. It should be safe to merge the series in its
> current form - I will keep this issue on my radar to see if there
> is something we can do to improve it.
> 
> Thanks,
> Lorenzo
> 
Thank you Lorenzo

>>
>> Thanks,
>> Manikanta
>>
>>>> PCIe link may fail to come up if there is no endpoint in the slot
>>>> or interoperable failure where endpoint specific quirk or work
>>>> around required. In such cases host controller driver shouldn't
>>>> fail.
>>>>
>>>> Also user can connect endpoint in only one slot and expect it to be
>>>> working. Even though host bridge is a single device, driver should
>>>> allow standalone port to work.
>>>>
>>>> Thanks,
>>>> Manikanta
>>>>
>>>>>>  
>>>>>>  	return 0;
>>>>>> +
>>>>>> +poweroff:
>>>>>> +	tegra_pcie_power_off(pcie);
>>>>>> +
>>>>>> +	return err;
>>>>>>  }
>>>>>>  
>>>>>> +static const struct dev_pm_ops tegra_pcie_pm_ops = {
>>>>>> +	SET_RUNTIME_PM_OPS(tegra_pcie_pm_suspend, tegra_pcie_pm_resume, NULL)
>>>>>> +	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(tegra_pcie_pm_suspend,
>>>>>> +				      tegra_pcie_pm_resume)
>>>>>> +};
>>>>>> +
>>>>>>  static struct platform_driver tegra_pcie_driver = {
>>>>>>  	.driver = {
>>>>>>  		.name = "tegra-pcie",
>>>>>>  		.of_match_table = tegra_pcie_of_match,
>>>>>>  		.suppress_bind_attrs = true,
>>>>>> +		.pm = &tegra_pcie_pm_ops,
>>>>>>  	},
>>>>>>  	.probe = tegra_pcie_probe,
>>>>>>  	.remove = tegra_pcie_remove,
>>>>>> -- 
>>>>>> 2.1.4
>>>>>>

Re: [PATCH V9 0/3] Add loadable kernel module and power management support

[Lorenzo Pieralisi]

On Wed, Feb 28, 2018 at 03:30:31PM +0530, Manikanta Maddireddy wrote:
> This series of patches adds loadable kernel module and power management
> support to Tegra PCIe host controller driver. irq_set_msi_desc() and
> tegra_cpuidle_pcie_irqs_in_use() symbols will be taken care in next
> set of patches.
> 
> These patches are tested on Jetson TK1, TX1 and TX2 platforms, following
> are the verification details.
> 	- Multiple module insert & remove
> 	- PCIe device functionality after module insert
> 	- Free clock, resets, regulators, powergate, iomem and interrupt
> 	resources after module remove
> 	- PCIe device functionality after resume from RAM
> 
> V2: PM QoS fix is dropped in V2 from this series because the fix is
> incorporated in latest 'commit 0759e80b84e3 ("PM / QoS: Fix device resume
> latency framework")'. Update commit message of few patches in V2.
> 
> V3: Patches to export irq_set_msi_desc() and tegra_cpuidle_pcie_irqs_in_use()
> are dropped based on review comments. These symbols will be addressed in next
> series. Took care of few other review comments.
> 
> V4: Dropped pci_find_host_bridge() export patch and added new patch to use
> bus->sysdata for private data.
> 
> V5: Decouple from https://patchwork.ozlabs.org/patch/832053/ and rebase
> on top of linux-next
> 
> V6: Rebased on lpieralisi/pci/tegra branch
> 
> V7: Addressed comments on patch: 6 and 7 in V6
> Limiting this series to linux-tegra & linux-pci since patches for other 
> subsystem are merged or dropped
> 
> V8: Since LKM is not completely implemented changing PCI_TEGRA config
> back to bool
> 
> V9: Squash patch-3 (PME_Turn_Off patch) on top of patch-2 (LKM patch)
> in V8 to have single working LKM patch.
> 
> Manikanta Maddireddy (3):
>   PCI: tegra: Free resources on probe failure
>   PCI: tegra: Add loadable kernel module support
>   PCI: tegra: Add power management support
> 
>  drivers/pci/host/pci-tegra.c | 354 +++++++++++++++++++++++++++++++++----------
>  1 file changed, 278 insertions(+), 76 deletions(-)

Applied to pci/tegra for v4.17, thanks !

Lorenzo