[PATCH V6 0/7] Add loadable kernel module and power management support

[PATCH V6 0/7] 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

Manikanta Maddireddy (7):
  of: Export of_pci_range_to_resource()
  PCI: tegra: Use bus->sysdata to store and get host private data
  PCI: tegra: Remove PCI_REASSIGN_ALL_BUS flag for Tegra PCIe
  PCI: tegra: Free resources on probe failure
  PCI: tegra: Add loadable kernel module support
  PCI: tegra: Broadcast PME_Turn_Off message before link goes to L2
  PCI: tegra: Add power management support

 drivers/of/address.c         |   1 +
 drivers/pci/host/Kconfig     |   2 +-
 drivers/pci/host/pci-tegra.c | 333 ++++++++++++++++++++++++++++++++-----------
 3 files changed, 253 insertions(+), 83 deletions(-)

-- 
2.1.4

[PATCH V6 1/7] of: Export of_pci_range_to_resource()

[Manikanta Maddireddy]

Tegra PCIe host driver parses of_pci_range from device tree and converts
to resource. Export of_pci_range_to_resource() to allow Tegra PCIe host
driver to be compiled as loadable kernel module.

Signed-off-by: Manikanta Maddireddy <mmaddireddy-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
V2:
* commit message update
V3:
* no change in this patch
V4:
* no change in this patch
V5:
* no change in this patch
V6:
* no change in this patch

 drivers/of/address.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/of/address.c b/drivers/of/address.c
index fa6cabfc3cb9..8d9b93f8701a 100644
--- a/drivers/of/address.c
+++ b/drivers/of/address.c
@@ -361,6 +361,7 @@ int of_pci_range_to_resource(struct of_pci_range *range,
 	res->end = (resource_size_t)OF_BAD_ADDR;
 	return err;
 }
+EXPORT_SYMBOL(of_pci_range_to_resource);
 #endif /* CONFIG_PCI */
 
 /*
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH V6 3/7] PCI: tegra: Remove PCI_REASSIGN_ALL_BUS flag for Tegra PCIe

[Manikanta Maddireddy]

Primary, secondary and subordinate default bus numbers are 0 in Tegra and
it is expecting SW to program these numbers in configration space.

pci_scan_bridge_extend() function programs these numbers in configuration
space if secondary & subordinate bus numbers are 0 or PCI_REASSIGN_ALL_BUS
flag is set. Since secondary & subordinate default bus numbers are 0,
PCI_REASSIGN_ALL_BUS flag can be removed for Tegra PCIe.

Signed-off-by: Manikanta Maddireddy <mmaddireddy-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
Acked-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
V3:
* new patch in V3
V4:
* no change in this patch
V5:
* no change in this patch
V6:
* Rebased on lpieralisi/pci/tegra branch

 drivers/pci/host/pci-tegra.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
index 059aa73d5f76..e4b47ebc91fe 100644
--- a/drivers/pci/host/pci-tegra.c
+++ b/drivers/pci/host/pci-tegra.c
@@ -2293,7 +2293,6 @@ static int tegra_pcie_probe(struct platform_device *pdev)
 
 	tegra_pcie_enable_ports(pcie);
 
-	pci_add_flags(PCI_REASSIGN_ALL_RSRC | PCI_REASSIGN_ALL_BUS);
 	host->busnr = pcie->busn.start;
 	host->dev.parent = &pdev->dev;
 	host->ops = &tegra_pcie_ops;
-- 
2.1.4

[PATCH V6 2/7] PCI: tegra: Use bus->sysdata to store and get host private data

[Manikanta Maddireddy]

Tegra host driver is using pci_find_host_bridge() to get private data,
however pci_find_host_bridge() is causing module build failure because
it is not exported. pci_find_host_bridge() can be avoided by using
bus->sysdata to store and get private data.

Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
Acked-by: Thierry Reding <treding@nvidia.com>
---
V4:
* new patch in V4
V5:
* Rebase on top of linux-next
V6:
* no change in this patch

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

diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
index 4c105fbda777..059aa73d5f76 100644
--- a/drivers/pci/host/pci-tegra.c
+++ b/drivers/pci/host/pci-tegra.c
@@ -379,8 +379,7 @@ static void __iomem *tegra_pcie_map_bus(struct pci_bus *bus,
 					unsigned int devfn,
 					int where)
 {
-	struct pci_host_bridge *host = pci_find_host_bridge(bus);
-	struct tegra_pcie *pcie = pci_host_bridge_priv(host);
+	struct tegra_pcie *pcie = bus->sysdata;
 	void __iomem *addr = NULL;
 
 	if (bus->number == 0) {
@@ -573,8 +572,7 @@ static int tegra_pcie_request_resources(struct tegra_pcie *pcie)
 
 static int tegra_pcie_map_irq(const struct pci_dev *pdev, u8 slot, u8 pin)
 {
-	struct pci_host_bridge *host = pci_find_host_bridge(pdev->bus);
-	struct tegra_pcie *pcie = pci_host_bridge_priv(host);
+	struct tegra_pcie *pcie = pdev->bus->sysdata;
 	int irq;
 
 	tegra_cpuidle_pcie_irqs_in_use();
@@ -2258,6 +2256,7 @@ static int tegra_pcie_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	pcie = pci_host_bridge_priv(host);
+	host->sysdata = pcie;
 
 	pcie->soc = of_device_get_match_data(dev);
 	INIT_LIST_HEAD(&pcie->ports);
-- 
2.1.4

[PATCH V6 4/7] 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>
---
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

 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

Re: [PATCH V6 4/7] PCI: tegra: Free resources on probe failure

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 858 bytes --]

On Thu, Jan 11, 2018 at 11:38:05AM +0530, Manikanta Maddireddy wrote:
> 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-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
> 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
> 
>  drivers/pci/host/pci-tegra.c | 99 +++++++++++++++++++++++++++++++++++---------
>  1 file changed, 79 insertions(+), 20 deletions(-)

Acked-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH V6 5/7] PCI: tegra: Add loadable kernel module support

[Manikanta Maddireddy]

Implement remove callback function for Tegra PCIe driver to add
loadable kernel module support. Change PCI_TEGRA config to tristate to
allow pci-tegra driver to be build as a module.

Signed-off-by: Manikanta Maddireddy <mmaddireddy@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

 drivers/pci/host/Kconfig     |  2 +-
 drivers/pci/host/pci-tegra.c | 34 +++++++++++++++++++++++++++++++---
 2 files changed, 32 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
index 38d12980db0f..6fd2a5937804 100644
--- a/drivers/pci/host/Kconfig
+++ b/drivers/pci/host/Kconfig
@@ -34,7 +34,7 @@ config PCI_FTPCI100
 	default ARCH_GEMINI
 
 config PCI_TEGRA
-	bool "NVIDIA Tegra PCIe controller"
+	tristate "NVIDIA Tegra PCIe controller"
 	depends on ARCH_TEGRA
 	help
 	  Say Y here if you want support for the PCIe host controller found
diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
index e68507f658d8..981f126b14d6 100644
--- a/drivers/pci/host/pci-tegra.c
+++ b/drivers/pci/host/pci-tegra.c
@@ -35,6 +35,7 @@
 #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>
@@ -2276,6 +2277,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 +2299,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 +2317,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 +2395,25 @@ 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);
+
+	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_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 +2421,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

Re: [PATCH V6 5/7] PCI: tegra: Add loadable kernel module support

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 911 bytes --]

On Thu, Jan 11, 2018 at 11:38:06AM +0530, Manikanta Maddireddy wrote:
> Implement remove callback function for Tegra PCIe driver to add
> loadable kernel module support. Change PCI_TEGRA config to tristate to
> allow pci-tegra driver to be build as a module.
> 
> Signed-off-by: Manikanta Maddireddy <mmaddireddy-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
> 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
> 
>  drivers/pci/host/Kconfig     |  2 +-
>  drivers/pci/host/pci-tegra.c | 34 +++++++++++++++++++++++++++++++---
>  2 files changed, 32 insertions(+), 4 deletions(-)

Acked-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH V6 6/7] PCI: tegra: Broadcast PME_Turn_Off message before link goes to L2

[Manikanta Maddireddy]

Per PCIe r3.0, sec 5.3.3.2.1, PCIe root port shoould 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.

Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
---
V2:
* no change in this patch
V3:
* add PME bitmap in soc data instead of using compatible string
* replace while loop with readl_poll_timeout() for polling
* commit log correction
V4:
* no change in this patch
V5:
* Rebased on linux-next
V6:
* no change in this patch

 drivers/pci/host/pci-tegra.c | 45 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
index 981f126b14d6..cc33fc0fb300 100644
--- a/drivers/pci/host/pci-tegra.c
+++ b/drivers/pci/host/pci-tegra.c
@@ -31,6 +31,7 @@
 #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>
@@ -153,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
@@ -233,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);
@@ -251,6 +256,8 @@ struct tegra_pcie_soc {
 	u32 tx_ref_sel;
 	u32 pads_refclk_cfg0;
 	u32 pads_refclk_cfg1;
+	u8 pme_turnoff_bit[3];
+	u8 pme_ack_bit[3];
 	bool has_pex_clkreq_en;
 	bool has_pex_bias_ctrl;
 	bool has_intr_prsnt_sense;
@@ -1358,6 +1365,31 @@ 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;
+
+	val = afi_readl(pcie, AFI_PCIE_PME);
+	val |= (0x1 << soc->pme_turnoff_bit[port->index]);
+	afi_writel(pcie, val, AFI_PCIE_PME);
+
+	err = readl_poll_timeout(pcie->afi + AFI_PCIE_PME, val,
+				 val & (0x1 << soc->pme_ack_bit[port->index]),
+				 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->pme_turnoff_bit[port->index]);
+	afi_writel(pcie, val, AFI_PCIE_PME);
+}
+
 static int tegra_msi_alloc(struct tegra_msi *chip)
 {
 	int msi;
@@ -2109,6 +2141,8 @@ static const struct tegra_pcie_soc tegra20_pcie = {
 	.pads_pll_ctl = PADS_PLL_CTL_TEGRA20,
 	.tx_ref_sel = PADS_PLL_CTL_TXCLKREF_DIV10,
 	.pads_refclk_cfg0 = 0xfa5cfa5c,
+	.pme_turnoff_bit = {0, 8},
+	.pme_ack_bit = {5, 10},
 	.has_pex_clkreq_en = false,
 	.has_pex_bias_ctrl = false,
 	.has_intr_prsnt_sense = false,
@@ -2125,6 +2159,8 @@ static const struct tegra_pcie_soc tegra30_pcie = {
 	.tx_ref_sel = PADS_PLL_CTL_TXCLKREF_BUF_EN,
 	.pads_refclk_cfg0 = 0xfa5cfa5c,
 	.pads_refclk_cfg1 = 0xfa5cfa5c,
+	.pme_turnoff_bit = {0, 8, 16},
+	.pme_ack_bit = {5, 10, 18},
 	.has_pex_clkreq_en = true,
 	.has_pex_bias_ctrl = true,
 	.has_intr_prsnt_sense = true,
@@ -2140,6 +2176,8 @@ static const struct tegra_pcie_soc tegra124_pcie = {
 	.pads_pll_ctl = PADS_PLL_CTL_TEGRA30,
 	.tx_ref_sel = PADS_PLL_CTL_TXCLKREF_BUF_EN,
 	.pads_refclk_cfg0 = 0x44ac44ac,
+	.pme_turnoff_bit = {0, 8},
+	.pme_ack_bit = {5, 10},
 	.has_pex_clkreq_en = true,
 	.has_pex_bias_ctrl = true,
 	.has_intr_prsnt_sense = true,
@@ -2155,6 +2193,8 @@ static const struct tegra_pcie_soc tegra210_pcie = {
 	.pads_pll_ctl = PADS_PLL_CTL_TEGRA30,
 	.tx_ref_sel = PADS_PLL_CTL_TXCLKREF_BUF_EN,
 	.pads_refclk_cfg0 = 0x90b890b8,
+	.pme_turnoff_bit = {0, 8},
+	.pme_ack_bit = {5, 10},
 	.has_pex_clkreq_en = true,
 	.has_pex_bias_ctrl = true,
 	.has_intr_prsnt_sense = true,
@@ -2171,6 +2211,8 @@ static const struct tegra_pcie_soc tegra186_pcie = {
 	.tx_ref_sel = PADS_PLL_CTL_TXCLKREF_BUF_EN,
 	.pads_refclk_cfg0 = 0x80b880b8,
 	.pads_refclk_cfg1 = 0x000480b8,
+	.pme_turnoff_bit = {0, 8, 12},
+	.pme_ack_bit = {5, 10, 14},
 	.has_pex_clkreq_en = true,
 	.has_pex_bias_ctrl = true,
 	.has_intr_prsnt_sense = true,
@@ -2399,11 +2441,14 @@ 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, *tmp;
 
 	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_safe(port, tmp, &pcie->ports, list)
+		tegra_pcie_pme_turnoff(port);
 	tegra_pcie_disable_ports(pcie);
 	if (IS_ENABLED(CONFIG_PCI_MSI))
 		tegra_pcie_disable_msi(pcie);
-- 
2.1.4

Re: [PATCH V6 6/7] PCI: tegra: Broadcast PME_Turn_Off message before link goes to L2

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 6216 bytes --]

On Thu, Jan 11, 2018 at 11:38:07AM +0530, Manikanta Maddireddy wrote:
> Per PCIe r3.0, sec 5.3.3.2.1, PCIe root port shoould 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.
> 
> Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
> ---
> V2:
> * no change in this patch
> V3:
> * add PME bitmap in soc data instead of using compatible string
> * replace while loop with readl_poll_timeout() for polling
> * commit log correction
> V4:
> * no change in this patch
> V5:
> * Rebased on linux-next
> V6:
> * no change in this patch
> 
>  drivers/pci/host/pci-tegra.c | 45 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 45 insertions(+)
> 
> diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
> index 981f126b14d6..cc33fc0fb300 100644
> --- a/drivers/pci/host/pci-tegra.c
> +++ b/drivers/pci/host/pci-tegra.c
> @@ -31,6 +31,7 @@
>  #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>
> @@ -153,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
> @@ -233,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);
> @@ -251,6 +256,8 @@ struct tegra_pcie_soc {
>  	u32 tx_ref_sel;
>  	u32 pads_refclk_cfg0;
>  	u32 pads_refclk_cfg1;
> +	u8 pme_turnoff_bit[3];
> +	u8 pme_ack_bit[3];

This seems suboptimal to me. Perhaps a better way would be:

	struct tegra_pcie_port_soc {
		struct {
			u8 turnoff_bit;
			u8 ack_bit;
		} pme;
	};

And since we already have num_ports which defines exactly how many ports
we have:

	struct tegra_pcie_soc {
		unsigned int num_ports;
		const struct tegra_pcie_port_soc *ports;
		...
	};

I suspect that as you're adding more features we may need more of this
data.

But I'm fine to keep it like this. We can always switch to something
different if the above becomes too much cluttered.

>  	bool has_pex_clkreq_en;
>  	bool has_pex_bias_ctrl;
>  	bool has_intr_prsnt_sense;
> @@ -1358,6 +1365,31 @@ 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;
> +
> +	val = afi_readl(pcie, AFI_PCIE_PME);
> +	val |= (0x1 << soc->pme_turnoff_bit[port->index]);
> +	afi_writel(pcie, val, AFI_PCIE_PME);
> +
> +	err = readl_poll_timeout(pcie->afi + AFI_PCIE_PME, val,
> +				 val & (0x1 << soc->pme_ack_bit[port->index]),
> +				 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->pme_turnoff_bit[port->index]);
> +	afi_writel(pcie, val, AFI_PCIE_PME);
> +}
> +
>  static int tegra_msi_alloc(struct tegra_msi *chip)
>  {
>  	int msi;
> @@ -2109,6 +2141,8 @@ static const struct tegra_pcie_soc tegra20_pcie = {
>  	.pads_pll_ctl = PADS_PLL_CTL_TEGRA20,
>  	.tx_ref_sel = PADS_PLL_CTL_TXCLKREF_DIV10,
>  	.pads_refclk_cfg0 = 0xfa5cfa5c,
> +	.pme_turnoff_bit = {0, 8},
> +	.pme_ack_bit = {5, 10},
>  	.has_pex_clkreq_en = false,
>  	.has_pex_bias_ctrl = false,
>  	.has_intr_prsnt_sense = false,
> @@ -2125,6 +2159,8 @@ static const struct tegra_pcie_soc tegra30_pcie = {
>  	.tx_ref_sel = PADS_PLL_CTL_TXCLKREF_BUF_EN,
>  	.pads_refclk_cfg0 = 0xfa5cfa5c,
>  	.pads_refclk_cfg1 = 0xfa5cfa5c,
> +	.pme_turnoff_bit = {0, 8, 16},
> +	.pme_ack_bit = {5, 10, 18},
>  	.has_pex_clkreq_en = true,
>  	.has_pex_bias_ctrl = true,
>  	.has_intr_prsnt_sense = true,
> @@ -2140,6 +2176,8 @@ static const struct tegra_pcie_soc tegra124_pcie = {
>  	.pads_pll_ctl = PADS_PLL_CTL_TEGRA30,
>  	.tx_ref_sel = PADS_PLL_CTL_TXCLKREF_BUF_EN,
>  	.pads_refclk_cfg0 = 0x44ac44ac,
> +	.pme_turnoff_bit = {0, 8},
> +	.pme_ack_bit = {5, 10},
>  	.has_pex_clkreq_en = true,
>  	.has_pex_bias_ctrl = true,
>  	.has_intr_prsnt_sense = true,
> @@ -2155,6 +2193,8 @@ static const struct tegra_pcie_soc tegra210_pcie = {
>  	.pads_pll_ctl = PADS_PLL_CTL_TEGRA30,
>  	.tx_ref_sel = PADS_PLL_CTL_TXCLKREF_BUF_EN,
>  	.pads_refclk_cfg0 = 0x90b890b8,
> +	.pme_turnoff_bit = {0, 8},
> +	.pme_ack_bit = {5, 10},
>  	.has_pex_clkreq_en = true,
>  	.has_pex_bias_ctrl = true,
>  	.has_intr_prsnt_sense = true,
> @@ -2171,6 +2211,8 @@ static const struct tegra_pcie_soc tegra186_pcie = {
>  	.tx_ref_sel = PADS_PLL_CTL_TXCLKREF_BUF_EN,
>  	.pads_refclk_cfg0 = 0x80b880b8,
>  	.pads_refclk_cfg1 = 0x000480b8,
> +	.pme_turnoff_bit = {0, 8, 12},
> +	.pme_ack_bit = {5, 10, 14},
>  	.has_pex_clkreq_en = true,
>  	.has_pex_bias_ctrl = true,
>  	.has_intr_prsnt_sense = true,
> @@ -2399,11 +2441,14 @@ 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, *tmp;
>  
>  	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_safe(port, tmp, &pcie->ports, list)
> +		tegra_pcie_pme_turnoff(port);

Why list_for_each_entry_safe()? tegra_pcie_pme_turnoff() doesn't do
anything that would make port become invalid.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH V6 6/7] PCI: tegra: Broadcast PME_Turn_Off message before link goes to L2

[Manikanta Maddireddy]

On 25-Jan-18 8:06 PM, Thierry Reding wrote:
> On Thu, Jan 11, 2018 at 11:38:07AM +0530, Manikanta Maddireddy wrote:
>> Per PCIe r3.0, sec 5.3.3.2.1, PCIe root port shoould 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.
>>
>> Signed-off-by: Manikanta Maddireddy <mmaddireddy-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>> ---
>> V2:
>> * no change in this patch
>> V3:
>> * add PME bitmap in soc data instead of using compatible string
>> * replace while loop with readl_poll_timeout() for polling
>> * commit log correction
>> V4:
>> * no change in this patch
>> V5:
>> * Rebased on linux-next
>> V6:
>> * no change in this patch
>>
>>  drivers/pci/host/pci-tegra.c | 45 ++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 45 insertions(+)
>>
>> diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
>> index 981f126b14d6..cc33fc0fb300 100644
>> --- a/drivers/pci/host/pci-tegra.c
>> +++ b/drivers/pci/host/pci-tegra.c
>> @@ -31,6 +31,7 @@
>>  #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>
>> @@ -153,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
>> @@ -233,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);
>> @@ -251,6 +256,8 @@ struct tegra_pcie_soc {
>>  	u32 tx_ref_sel;
>>  	u32 pads_refclk_cfg0;
>>  	u32 pads_refclk_cfg1;
>> +	u8 pme_turnoff_bit[3];
>> +	u8 pme_ack_bit[3];
> 
> This seems suboptimal to me. Perhaps a better way would be:
> 
> 	struct tegra_pcie_port_soc {
> 		struct {
> 			u8 turnoff_bit;
> 			u8 ack_bit;
> 		} pme;
> 	};
> 
> And since we already have num_ports which defines exactly how many ports
> we have:
> 
> 	struct tegra_pcie_soc {
> 		unsigned int num_ports;
> 		const struct tegra_pcie_port_soc *ports;
> 		...
> 	};
> 
> I suspect that as you're adding more features we may need more of this
> data.
> 
> But I'm fine to keep it like this. We can always switch to something
> different if the above becomes too much cluttered.
> 
I agree it is sub optimal. I moved pme bitmap values to soc data because
tegra30 and tegra186 have different bitmap values. To differentiate this
I allocated static memory in tegra_pcie_soc. I am a bit hesitant do dynamic
allocation based on num_ports because I need to use compatible string
to distinguish between tegra30 & tegra186 in runtime. I believe it is OK
to have trade off 16 bits to avoid these compatible check?

>>  	bool has_pex_clkreq_en;
>>  	bool has_pex_bias_ctrl;
>>  	bool has_intr_prsnt_sense;
>> @@ -1358,6 +1365,31 @@ 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;
>> +
>> +	val = afi_readl(pcie, AFI_PCIE_PME);
>> +	val |= (0x1 << soc->pme_turnoff_bit[port->index]);
>> +	afi_writel(pcie, val, AFI_PCIE_PME);
>> +
>> +	err = readl_poll_timeout(pcie->afi + AFI_PCIE_PME, val,
>> +				 val & (0x1 << soc->pme_ack_bit[port->index]),
>> +				 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->pme_turnoff_bit[port->index]);
>> +	afi_writel(pcie, val, AFI_PCIE_PME);
>> +}
>> +
>>  static int tegra_msi_alloc(struct tegra_msi *chip)
>>  {
>>  	int msi;
>> @@ -2109,6 +2141,8 @@ static const struct tegra_pcie_soc tegra20_pcie = {
>>  	.pads_pll_ctl = PADS_PLL_CTL_TEGRA20,
>>  	.tx_ref_sel = PADS_PLL_CTL_TXCLKREF_DIV10,
>>  	.pads_refclk_cfg0 = 0xfa5cfa5c,
>> +	.pme_turnoff_bit = {0, 8},
>> +	.pme_ack_bit = {5, 10},
>>  	.has_pex_clkreq_en = false,
>>  	.has_pex_bias_ctrl = false,
>>  	.has_intr_prsnt_sense = false,
>> @@ -2125,6 +2159,8 @@ static const struct tegra_pcie_soc tegra30_pcie = {
>>  	.tx_ref_sel = PADS_PLL_CTL_TXCLKREF_BUF_EN,
>>  	.pads_refclk_cfg0 = 0xfa5cfa5c,
>>  	.pads_refclk_cfg1 = 0xfa5cfa5c,
>> +	.pme_turnoff_bit = {0, 8, 16},
>> +	.pme_ack_bit = {5, 10, 18},
>>  	.has_pex_clkreq_en = true,
>>  	.has_pex_bias_ctrl = true,
>>  	.has_intr_prsnt_sense = true,
>> @@ -2140,6 +2176,8 @@ static const struct tegra_pcie_soc tegra124_pcie = {
>>  	.pads_pll_ctl = PADS_PLL_CTL_TEGRA30,
>>  	.tx_ref_sel = PADS_PLL_CTL_TXCLKREF_BUF_EN,
>>  	.pads_refclk_cfg0 = 0x44ac44ac,
>> +	.pme_turnoff_bit = {0, 8},
>> +	.pme_ack_bit = {5, 10},
>>  	.has_pex_clkreq_en = true,
>>  	.has_pex_bias_ctrl = true,
>>  	.has_intr_prsnt_sense = true,
>> @@ -2155,6 +2193,8 @@ static const struct tegra_pcie_soc tegra210_pcie = {
>>  	.pads_pll_ctl = PADS_PLL_CTL_TEGRA30,
>>  	.tx_ref_sel = PADS_PLL_CTL_TXCLKREF_BUF_EN,
>>  	.pads_refclk_cfg0 = 0x90b890b8,
>> +	.pme_turnoff_bit = {0, 8},
>> +	.pme_ack_bit = {5, 10},
>>  	.has_pex_clkreq_en = true,
>>  	.has_pex_bias_ctrl = true,
>>  	.has_intr_prsnt_sense = true,
>> @@ -2171,6 +2211,8 @@ static const struct tegra_pcie_soc tegra186_pcie = {
>>  	.tx_ref_sel = PADS_PLL_CTL_TXCLKREF_BUF_EN,
>>  	.pads_refclk_cfg0 = 0x80b880b8,
>>  	.pads_refclk_cfg1 = 0x000480b8,
>> +	.pme_turnoff_bit = {0, 8, 12},
>> +	.pme_ack_bit = {5, 10, 14},
>>  	.has_pex_clkreq_en = true,
>>  	.has_pex_bias_ctrl = true,
>>  	.has_intr_prsnt_sense = true,
>> @@ -2399,11 +2441,14 @@ 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, *tmp;
>>  
>>  	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_safe(port, tmp, &pcie->ports, list)
>> +		tegra_pcie_pme_turnoff(port);
> 
> Why list_for_each_entry_safe()? tegra_pcie_pme_turnoff() doesn't do
> anything that would make port become invalid.
Apart from this reason, I don't see a scenario where "making port invalid"
code run in parallel to this function. So it safe to use list_for_each_entry,
I'll do this change in next version.
> 
> Thierry
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V6 6/7] PCI: tegra: Broadcast PME_Turn_Off message before link goes to L2

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 5244 bytes --]

On Mon, Jan 29, 2018 at 10:11:42AM +0530, Manikanta Maddireddy wrote:
> 
> 
> On 25-Jan-18 8:06 PM, Thierry Reding wrote:
> > On Thu, Jan 11, 2018 at 11:38:07AM +0530, Manikanta Maddireddy wrote:
> >> Per PCIe r3.0, sec 5.3.3.2.1, PCIe root port shoould 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.
> >>
> >> Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
> >> ---
> >> V2:
> >> * no change in this patch
> >> V3:
> >> * add PME bitmap in soc data instead of using compatible string
> >> * replace while loop with readl_poll_timeout() for polling
> >> * commit log correction
> >> V4:
> >> * no change in this patch
> >> V5:
> >> * Rebased on linux-next
> >> V6:
> >> * no change in this patch
> >>
> >>  drivers/pci/host/pci-tegra.c | 45 ++++++++++++++++++++++++++++++++++++++++++++
> >>  1 file changed, 45 insertions(+)
> >>
> >> diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
> >> index 981f126b14d6..cc33fc0fb300 100644
> >> --- a/drivers/pci/host/pci-tegra.c
> >> +++ b/drivers/pci/host/pci-tegra.c
> >> @@ -31,6 +31,7 @@
> >>  #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>
> >> @@ -153,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
> >> @@ -233,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);
> >> @@ -251,6 +256,8 @@ struct tegra_pcie_soc {
> >>  	u32 tx_ref_sel;
> >>  	u32 pads_refclk_cfg0;
> >>  	u32 pads_refclk_cfg1;
> >> +	u8 pme_turnoff_bit[3];
> >> +	u8 pme_ack_bit[3];
> > 
> > This seems suboptimal to me. Perhaps a better way would be:
> > 
> > 	struct tegra_pcie_port_soc {
> > 		struct {
> > 			u8 turnoff_bit;
> > 			u8 ack_bit;
> > 		} pme;
> > 	};
> > 
> > And since we already have num_ports which defines exactly how many ports
> > we have:
> > 
> > 	struct tegra_pcie_soc {
> > 		unsigned int num_ports;
> > 		const struct tegra_pcie_port_soc *ports;
> > 		...
> > 	};
> > 
> > I suspect that as you're adding more features we may need more of this
> > data.
> > 
> > But I'm fine to keep it like this. We can always switch to something
> > different if the above becomes too much cluttered.
> > 
> I agree it is sub optimal. I moved pme bitmap values to soc data because
> tegra30 and tegra186 have different bitmap values. To differentiate this
> I allocated static memory in tegra_pcie_soc. I am a bit hesitant do dynamic
> allocation based on num_ports because I need to use compatible string
> to distinguish between tegra30 & tegra186 in runtime. I believe it is OK
> to have trade off 16 bits to avoid these compatible check?

Sorry if I was unclear. I didn't suggest that you'd dynamically allocate
memory, instead you'd hook it up like so:

	static const struct tegra_pcie_port_soc tegra186_pcie_ports[] = {
		{ .turnoff_bit =  0, .ack_bit =  5 },
		{ .turnoff_bit =  8, .ack_bit = 10 },
		{ .turnoff_bit = 12, .ack_bit = 14 },
	};

And then in the existing tegra186_pcie, you'd set this:

	static const struct tegra_pcie_soc tegra186_pcie = {
		.num_ports = 3,
		.ports = tegra186_pcie_ports,
		...
	};

And similar for the others. I realize that this is somewhat more verbose
than your original, but I think it's more readable.

It also allows you to reuse data, such as:

	static const struct tegra_pcie_port_soc tegra20_pcie_ports[] = {
		{ .turnoff_bit = 0, .ack_bit =  5 },
		{ .turnoff_bit = 8, .ack_bit = 10 },
	};

	...

	static const struct tegra_pcie_soc tegra20_pcie = {
		.num_ports = 2,
		.ports = tegra20_pcie_ports,
		...
	};

	...

	static const struct tegra_pcie_soc tegra124_pcie = {
		.num_ports = 2,
		.ports = tegra20_pcie_ports,
		...
	};

	static const struct tegra_pcie_soc tegra210_pcie = {
		.num_ports = 2,
		.ports = tegra20_pcie_ports,
		...
	};

Of course the sharing only works as long as the port definitions don't
contain data that's different between the chips.

You could even go and derive the .num_ports as ARRAY_SIZE() of the ports
definition.

So the above does not dynamically allocate memory, it is just a more
explicit way of specifying the data. It puts the data closer together,
and thereby makes it easier to read, in my opinion.

But as I said, feel free to leave this as-is. We can easily rework this
later on or if necessary.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH V6 7/7] 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>
---
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
index cc33fc0fb300..a106eb7ee695 100644
--- a/drivers/pci/host/pci-tegra.c
+++ b/drivers/pci/host/pci-tegra.c
@@ -1287,31 +1287,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;
@@ -1322,14 +1316,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;
@@ -1337,7 +1331,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;
@@ -1345,8 +1339,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;
 }
 
@@ -1357,8 +1349,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);
 
@@ -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);
+	}
 
 	afi_writel(pcie, msi->phys >> soc->msi_base_shift, AFI_MSI_FPCI_BAR_ST);
 	afi_writel(pcie, msi->phys, AFI_MSI_AXI_BAR_ST);
@@ -2129,10 +2123,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_soc tegra20_pcie = {
@@ -2375,26 +2367,16 @@ static int tegra_pcie_probe(struct platform_device *pdev)
 		return err;
 	}
 
-	err = tegra_pcie_enable_controller(pcie);
-	if (err)
+	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 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;
-		}
-	}
-
-	tegra_pcie_enable_ports(pcie);
+		goto pm_runtime_put;
 
 	host->busnr = pcie->busn.start;
 	host->dev.parent = &pdev->dev;
@@ -2405,7 +2387,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);
@@ -2424,14 +2406,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:
+	if (IS_ENABLED(CONFIG_PCI_MSI))
+		tegra_pcie_disable_msi(pcie);
+	pm_runtime_put_sync(pcie->dev);
+	pm_runtime_disable(pcie->dev);
 put_resources:
 	tegra_pcie_put_resources(pcie);
 	return err;
@@ -2447,23 +2428,81 @@ static int tegra_pcie_remove(struct platform_device *pdev)
 		tegra_pcie_debugfs_exit(pcie);
 	pci_stop_root_bus(host->bus);
 	pci_remove_root_bus(host->bus);
-	list_for_each_entry_safe(port, tmp, &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);
+	pm_runtime_put_sync(pcie->dev);
+	pm_runtime_disable(pcie->dev);
 	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, *tmp;
+
+	list_for_each_entry_safe(port, tmp, &pcie->ports, list)
+		tegra_pcie_pme_turnoff(port);
+	tegra_pcie_disable_ports(pcie);
+	tegra_pcie_disable_controller(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)) {
+		err = tegra_pcie_enable_msi(pcie);
+		if (err < 0) {
+			dev_err(dev, "failed to enable MSI support: %d\n", err);
+			goto disable_controller;
+		}
+	}
+
+	tegra_pcie_enable_ports(pcie);
 
 	return 0;
+
+disable_controller:
+	tegra_pcie_disable_controller(pcie);
+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 V6 7/7] PCI: tegra: Add power management support

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 4186 bytes --]

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@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
> 
>  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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH V6 7/7] PCI: tegra: Add power management support

[Manikanta Maddireddy]

On 25-Jan-18 8:18 PM, Thierry Reding wrote:
> 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-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>> ---
>> 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.
I'll do this change in next version.
> 
> Thierry
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html