[PATCH V2 0/9] Add loadable kernel module and power management support

[PATCH V2 0/9] Add loadable kernel module and power management support

[Manikanta Maddireddy]

This series of patches add loadable kernel module and power management
support to Tegra PCIe host controller driver.

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

Tegra PCIe host controller driver is using ioremap_page_range() which is
not exported. This is switched with devm_ioremap() in the series
'https://patchwork.ozlabs.org/project/linux-tegra/list/?series=9907'.
Both these series will complete the loadable module support for Tegra
PCIe host driver.

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.

Manikanta Maddireddy (9):
  genirq: Export irq_set_msi_desc()
  of: Export of_pci_range_to_resource()
  ARM: tegra: Export tegra_cpuidle_pcie_irqs_in_use()
  PCI: Export pci_find_host_bridge()
  PCI: Export pci_flags
  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

 arch/arm/mach-tegra/cpuidle.c |   1 +
 drivers/of/address.c          |   1 +
 drivers/pci/host-bridge.c     |   1 +
 drivers/pci/host/Kconfig      |   2 +-
 drivers/pci/host/pci-tegra.c  | 359 +++++++++++++++++++++++++++++++++---------
 drivers/pci/setup-bus.c       |   1 +
 kernel/irq/chip.c             |   1 +
 7 files changed, 292 insertions(+), 74 deletions(-)

-- 
2.1.4

[PATCH V2 1/9] genirq: Export irq_set_msi_desc()

[Manikanta Maddireddy]

PCI MSI driver allocates MSI entry for each pci dev which creates msi_desc,
host driver maps each msi domain with hwirq and then sets msi_desc for
that irq number. Need to export irq_set_msi_desc() to allow Tegra PCIe
driver to be compiled as a loadable kernel module.

Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
---
V2:
* commit message update

 kernel/irq/chip.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
index 043bfc35b353..0e74b1051267 100644
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -136,6 +136,7 @@ int irq_set_msi_desc(unsigned int irq, struct msi_desc *entry)
 {
 	return irq_set_msi_desc_off(irq, 0, entry);
 }
+EXPORT_SYMBOL(irq_set_msi_desc);
 
 /**
  *	irq_set_chip_data - set irq chip data for an irq
-- 
2.1.4

[PATCH V2 2/9] 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@nvidia.com>
---
V2:
* commit message update

 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

Re: [PATCH V2 2/9] of: Export of_pci_range_to_resource()

[Rob Herring]

On Sun, Nov 26, 2017 at 01:02:06AM +0530, Manikanta Maddireddy wrote:
> 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@nvidia.com>
> ---
> V2:
> * commit message update
> 
>  drivers/of/address.c | 1 +
>  1 file changed, 1 insertion(+)

Acked-by: Rob Herring <robh@kernel.org>

[PATCH V2 3/9] ARM: tegra: Export tegra_cpuidle_pcie_irqs_in_use()

[Manikanta Maddireddy]

Tegra20 has a HW bug where PCIe interrupts are lost when LP2 (Tegra idle
state) is enabled. tegra_cpuidle_pcie_irqs_in_use() disables LP2 when
PCIe irq is mapped. EXPORT tegra_cpuidle_pcie_irqs_in_use() to allow Tegra
PCIe driver to be compiled as loadable kernel module.

Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
---
V2:
* commit message update

 arch/arm/mach-tegra/cpuidle.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/mach-tegra/cpuidle.c b/arch/arm/mach-tegra/cpuidle.c
index 316563141add..7d7e6d3ce32d 100644
--- a/arch/arm/mach-tegra/cpuidle.c
+++ b/arch/arm/mach-tegra/cpuidle.c
@@ -57,3 +57,4 @@ void tegra_cpuidle_pcie_irqs_in_use(void)
 		break;
 	}
 }
+EXPORT_SYMBOL(tegra_cpuidle_pcie_irqs_in_use);
-- 
2.1.4

[PATCH V2 4/9] PCI: Export pci_find_host_bridge()

[Manikanta Maddireddy]

PCI subsystem pass pci_bus pointer to pci_ops callback functions, Tegra
host driver use pci_find_host_bridge() to get pci_host_bridge from pci_bus.
Export pci_find_host_bridge() to allow Tegra PCIe driver to be compiled as
loadable kernel module.

Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
---
V2:
* commit message update

 drivers/pci/host-bridge.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/pci/host-bridge.c b/drivers/pci/host-bridge.c
index add66236215c..e0942fc086ad 100644
--- a/drivers/pci/host-bridge.c
+++ b/drivers/pci/host-bridge.c
@@ -22,6 +22,7 @@ struct pci_host_bridge *pci_find_host_bridge(struct pci_bus *bus)
 
 	return to_pci_host_bridge(root_bus->bridge);
 }
+EXPORT_SYMBOL(pci_find_host_bridge);
 
 struct device *pci_get_host_bridge_device(struct pci_dev *dev)
 {
-- 
2.1.4

Re: [PATCH V2 4/9] PCI: Export pci_find_host_bridge()

[Christoph Hellwig]

On Sun, Nov 26, 2017 at 01:02:08AM +0530, Manikanta Maddireddy wrote:
> PCI subsystem pass pci_bus pointer to pci_ops callback functions, Tegra
> host driver use pci_find_host_bridge() to get pci_host_bridge from pci_bus.
> Export pci_find_host_bridge() to allow Tegra PCIe driver to be compiled as
> loadable kernel module.
> 
> Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
> ---
> V2:
> * commit message update
> 
>  drivers/pci/host-bridge.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/pci/host-bridge.c b/drivers/pci/host-bridge.c
> index add66236215c..e0942fc086ad 100644
> --- a/drivers/pci/host-bridge.c
> +++ b/drivers/pci/host-bridge.c
> @@ -22,6 +22,7 @@ struct pci_host_bridge *pci_find_host_bridge(struct pci_bus *bus)
>  
>  	return to_pci_host_bridge(root_bus->bridge);
>  }
> +EXPORT_SYMBOL(pci_find_host_bridge);

All PCI internals should be exported using EXPORT_SYMBOL_GPL.

[PATCH V2 5/9] PCI: Export pci_flags

[Manikanta Maddireddy]

pci_flags variable is used in inline functions in 'pci.h', Tegra PCIe
driver use one of these functions pci_add_flags() and includes 'pci.h'.
Export pci_flags to allow Tegra PCIe host controller driver to be
compiled as loadable kernel module.

Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
---
V2:
* commit message update

 drivers/pci/setup-bus.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index b1ad466199ad..3567e1c4e340 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -29,6 +29,7 @@
 #include "pci.h"
 
 unsigned int pci_flags;
+EXPORT_SYMBOL(pci_flags);
 
 struct pci_dev_resource {
 	struct list_head list;
-- 
2.1.4

Re: [PATCH V2 5/9] PCI: Export pci_flags

[Bjorn Helgaas]

[+cc Lorenzo]

On Sun, Nov 26, 2017 at 01:02:09AM +0530, Manikanta Maddireddy wrote:
> pci_flags variable is used in inline functions in 'pci.h', Tegra PCIe
> driver use one of these functions pci_add_flags() and includes 'pci.h'.
> Export pci_flags to allow Tegra PCIe host controller driver to be
> compiled as loadable kernel module.

Here's the usage in tegra_pcie_probe():

  pci_add_flags(PCI_REASSIGN_ALL_RSRC | PCI_REASSIGN_ALL_BUS);

We've probably had this discussion before, but  I don't know why Tegra
needs PCI_REASSIGN_ALL_RSRC and PCI_REASSIGN_ALL_BUS.

I would prefer to drop this usage of pci_add_flags() if possible.  It
seems to be just an arm/powerpc thing and I'm not convinced it's
really necessary.

> Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
> ---
> V2:
> * commit message update
> 
>  drivers/pci/setup-bus.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> index b1ad466199ad..3567e1c4e340 100644
> --- a/drivers/pci/setup-bus.c
> +++ b/drivers/pci/setup-bus.c
> @@ -29,6 +29,7 @@
>  #include "pci.h"
>  
>  unsigned int pci_flags;
> +EXPORT_SYMBOL(pci_flags);
>  
>  struct pci_dev_resource {
>  	struct list_head list;
> -- 
> 2.1.4
> 

Re: [PATCH V2 5/9] PCI: Export pci_flags

[Lorenzo Pieralisi]

On Wed, Nov 29, 2017 at 11:01:33AM -0600, Bjorn Helgaas wrote:
> [+cc Lorenzo]
> 
> On Sun, Nov 26, 2017 at 01:02:09AM +0530, Manikanta Maddireddy wrote:
> > pci_flags variable is used in inline functions in 'pci.h', Tegra PCIe
> > driver use one of these functions pci_add_flags() and includes 'pci.h'.
> > Export pci_flags to allow Tegra PCIe host controller driver to be
> > compiled as loadable kernel module.
> 
> Here's the usage in tegra_pcie_probe():
> 
>   pci_add_flags(PCI_REASSIGN_ALL_RSRC | PCI_REASSIGN_ALL_BUS);
> 
> We've probably had this discussion before, but  I don't know why Tegra
> needs PCI_REASSIGN_ALL_RSRC and PCI_REASSIGN_ALL_BUS.
> 
> I would prefer to drop this usage of pci_add_flags() if possible.  It
> seems to be just an arm/powerpc thing and I'm not convinced it's
> really necessary.

It is hard to say if it is really necessary (because it depends
on firmware configuration - ie pci_scan_bridge()), that's the
problem.

I suspect it can trigger regressions if we do not set it (since
it affects what pcibios_assign_all_busses() returns on eg arm/arm64).

There are two things we can do:

1) Set it unconditionally in arch code (in a hook to be defined)
2) We remove it on a per-host bridge basis and ask for testing

I agree this may have trickled from host bridge to host bridge through
copy'n'paste and it is not based on any firmware assumtpion but I can't
say if it is really needed.

Lorenzo

Re: [PATCH V2 5/9] PCI: Export pci_flags

[Bjorn Helgaas]

On Thu, Nov 30, 2017 at 10:24:37AM +0000, Lorenzo Pieralisi wrote:
> On Wed, Nov 29, 2017 at 11:01:33AM -0600, Bjorn Helgaas wrote:
> > [+cc Lorenzo]
> > 
> > On Sun, Nov 26, 2017 at 01:02:09AM +0530, Manikanta Maddireddy wrote:
> > > pci_flags variable is used in inline functions in 'pci.h', Tegra PCIe
> > > driver use one of these functions pci_add_flags() and includes 'pci.h'.
> > > Export pci_flags to allow Tegra PCIe host controller driver to be
> > > compiled as loadable kernel module.
> > 
> > Here's the usage in tegra_pcie_probe():
> > 
> >   pci_add_flags(PCI_REASSIGN_ALL_RSRC | PCI_REASSIGN_ALL_BUS);
> > 
> > We've probably had this discussion before, but  I don't know why Tegra
> > needs PCI_REASSIGN_ALL_RSRC and PCI_REASSIGN_ALL_BUS.
> > 
> > I would prefer to drop this usage of pci_add_flags() if possible.  It
> > seems to be just an arm/powerpc thing and I'm not convinced it's
> > really necessary.
> 
> It is hard to say if it is really necessary (because it depends
> on firmware configuration - ie pci_scan_bridge()), that's the
> problem.
> 
> I suspect it can trigger regressions if we do not set it (since
> it affects what pcibios_assign_all_busses() returns on eg arm/arm64).
> 
> There are two things we can do:
> 
> 1) Set it unconditionally in arch code (in a hook to be defined)
> 2) We remove it on a per-host bridge basis and ask for testing
> 
> I agree this may have trickled from host bridge to host bridge through
> copy'n'paste and it is not based on any firmware assumtpion but I can't
> say if it is really needed.

My basic position is that if resources are not assigned correctly, the
PCI core should automatically try to assign them, regardless of
whether PCI_REASSIGN_ALL_RSRC or PCI_REASSIGN_ALL_BUS is set.  If that
doesn't work, I think there's something wrongin the PCI core and we
should fix that.

This might be an opportunity to try removing the use of
pci_add_flags() and see what breaks.

Bjorn

Re: [PATCH V2 5/9] PCI: Export pci_flags

[Manikanta Maddireddy]

On 01-Dec-17 12:12 AM, Bjorn Helgaas wrote:
> On Thu, Nov 30, 2017 at 10:24:37AM +0000, Lorenzo Pieralisi wrote:
>> On Wed, Nov 29, 2017 at 11:01:33AM -0600, Bjorn Helgaas wrote:
>>> [+cc Lorenzo]
>>>
>>> On Sun, Nov 26, 2017 at 01:02:09AM +0530, Manikanta Maddireddy wrote:
>>>> pci_flags variable is used in inline functions in 'pci.h', Tegra PCIe
>>>> driver use one of these functions pci_add_flags() and includes 'pci.h'.
>>>> Export pci_flags to allow Tegra PCIe host controller driver to be
>>>> compiled as loadable kernel module.
>>>
>>> Here's the usage in tegra_pcie_probe():
>>>
>>>   pci_add_flags(PCI_REASSIGN_ALL_RSRC | PCI_REASSIGN_ALL_BUS);
>>>
>>> We've probably had this discussion before, but  I don't know why Tegra
>>> needs PCI_REASSIGN_ALL_RSRC and PCI_REASSIGN_ALL_BUS.
>>>
>>> I would prefer to drop this usage of pci_add_flags() if possible.  It
>>> seems to be just an arm/powerpc thing and I'm not convinced it's
>>> really necessary.
>>
>> It is hard to say if it is really necessary (because it depends
>> on firmware configuration - ie pci_scan_bridge()), that's the
>> problem.
>>
>> I suspect it can trigger regressions if we do not set it (since
>> it affects what pcibios_assign_all_busses() returns on eg arm/arm64).
>>
>> There are two things we can do:
>>
>> 1) Set it unconditionally in arch code (in a hook to be defined)
>> 2) We remove it on a per-host bridge basis and ask for testing
>>
>> I agree this may have trickled from host bridge to host bridge through
>> copy'n'paste and it is not based on any firmware assumtpion but I can't
>> say if it is really needed.
> 
> My basic position is that if resources are not assigned correctly, the
> PCI core should automatically try to assign them, regardless of
> whether PCI_REASSIGN_ALL_RSRC or PCI_REASSIGN_ALL_BUS is set.  If that
> doesn't work, I think there's something wrongin the PCI core and we
> should fix that.
> 
> This might be an opportunity to try removing the use of
> pci_add_flags() and see what breaks.
> 
> Bjorn
> 

As per the Tegra TRM primary, secondary and subordinate default
bus numbers are 0 and it is expecting SW to program these numbers.
I believe this is the reason for adding PCI_REASSIGN_ALL_RSRC and
PCI_REASSIGN_ALL_BUS flags.

Looking at the function pci_scan_bridge_extend(), if secondary and
subordinate bus numbers are 0 it is assigning bus numbers even if
these flags are not set.

In the basic testing with one endpoint PCIe link up is working
fine without these flags, however I would like test with PCIe switch
and multiple endpoints connected to Tegra.

Manikanta

[PATCH V2 6/9] 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

 drivers/pci/host/pci-tegra.c | 102 ++++++++++++++++++++++++++++++++++++-------
 1 file changed, 86 insertions(+), 16 deletions(-)

diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
index e9b3ff95e259..7f7b8c9c1e84 100644
--- a/drivers/pci/host/pci-tegra.c
+++ b/drivers/pci/host/pci-tegra.c
@@ -701,14 +701,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 pci_host_bridge *host = pci_find_host_bridge(pdev->bus);
@@ -1109,29 +1120,40 @@ 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? */
-
 	if (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);
 	}
+}
+
+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->afi_rst);
 	reset_control_assert(pcie->pex_rst);
 
-	if (!dev->pm_domain)
-		tegra_powergate_power_off(TEGRA_POWERGATE_PCIE);
+	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);
 
 	err = regulator_bulk_disable(pcie->num_supplies, pcie->supplies);
 	if (err < 0)
 		dev_warn(dev, "failed to disable regulators: %d\n", err);
+
+	if (!dev->pm_domain)
+		tegra_powergate_power_off(TEGRA_POWERGATE_PCIE);
 }
 
 static int tegra_pcie_power_on(struct tegra_pcie *pcie)
@@ -1262,6 +1284,15 @@ static int tegra_pcie_phys_get_legacy(struct tegra_pcie *pcie)
 	return 0;
 }
 
+static void tegra_pcie_phys_put_legacy(struct tegra_pcie *pcie)
+{
+	int err;
+
+	err = phy_exit(pcie->phy);
+	if (err < 0)
+		dev_err(pcie->dev, "failed to teardown PHY: %d\n", err);
+}
+
 static struct phy *devm_of_phy_optional_get_index(struct device *dev,
 						  struct device_node *np,
 						  const char *consumer,
@@ -1315,6 +1346,19 @@ static int tegra_pcie_port_get_phys(struct tegra_pcie_port *port)
 	return 0;
 }
 
+static void tegra_pcie_port_put_phys(struct tegra_pcie_port *port)
+{
+	struct device *dev = port->pcie->dev;
+	unsigned int i;
+	int err;
+
+	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_phys_get(struct tegra_pcie *pcie)
 {
 	const struct tegra_pcie_soc *soc = pcie->soc;
@@ -1334,6 +1378,19 @@ static int tegra_pcie_phys_get(struct tegra_pcie *pcie)
 	return 0;
 }
 
+static void tegra_pcie_phys_put(struct tegra_pcie *pcie)
+{
+	const struct tegra_pcie_soc *soc = pcie->soc;
+	struct device_node *np = pcie->dev->of_node;
+	struct tegra_pcie_port *port;
+
+	if (!soc->has_gen2 || of_find_property(np, "phys", NULL) != NULL)
+		tegra_pcie_phys_put_legacy(pcie);
+
+	list_for_each_entry(port, &pcie->ports, list)
+		tegra_pcie_port_put_phys(port);
+}
+
 static int tegra_pcie_get_resources(struct tegra_pcie *pcie)
 {
 	struct device *dev = pcie->dev;
@@ -1366,7 +1423,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");
@@ -1424,25 +1481,23 @@ static int tegra_pcie_get_resources(struct tegra_pcie *pcie)
 
 poweroff:
 	tegra_pcie_power_off(pcie);
+phys_put:
+	if (soc->program_uphy)
+		tegra_pcie_phys_put(pcie);
 	return err;
 }
 
 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;
 }
@@ -2371,6 +2426,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;
+
+	reset_control_assert(pcie->pcie_xrst);
+
+	list_for_each_entry_safe(port, tmp, &pcie->ports, list)
+		tegra_pcie_port_disable(port);
+}
+
 static void tegra_pcie_change_link_speed(struct tegra_pcie *pcie,
 					 struct pci_dev *pci_dev)
 {
@@ -2691,7 +2756,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);
@@ -2700,7 +2765,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;
 		}
 	}
 
@@ -2741,6 +2806,11 @@ static int tegra_pcie_probe(struct platform_device *pdev)
 disable_msi:
 	if (IS_ENABLED(CONFIG_PCI_MSI))
 		tegra_pcie_disable_msi(pcie);
+	tegra_pcie_disable_ports(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 V2 6/9] PCI: tegra: free resources on probe failure

[Mikko Perttunen]

On 25.11.2017 21:32, 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@nvidia.com>
> ---
> V2:
> * no change in this patch
>
>  drivers/pci/host/pci-tegra.c | 102 ++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 86 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
> index e9b3ff95e259..7f7b8c9c1e84 100644
> --- a/drivers/pci/host/pci-tegra.c
> +++ b/drivers/pci/host/pci-tegra.c
> @@ -701,14 +701,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 pci_host_bridge *host = pci_find_host_bridge(pdev->bus);
> @@ -1109,29 +1120,40 @@ 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? */
> -
>  	if (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);
>  	}
> +}
> +
> +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->afi_rst);
>  	reset_control_assert(pcie->pex_rst);
>
> -	if (!dev->pm_domain)
> -		tegra_powergate_power_off(TEGRA_POWERGATE_PCIE);
> +	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);
>
>  	err = regulator_bulk_disable(pcie->num_supplies, pcie->supplies);
>  	if (err < 0)
>  		dev_warn(dev, "failed to disable regulators: %d\n", err);
> +
> +	if (!dev->pm_domain)
> +		tegra_powergate_power_off(TEGRA_POWERGATE_PCIE);
>  }
>
>  static int tegra_pcie_power_on(struct tegra_pcie *pcie)
> @@ -1262,6 +1284,15 @@ static int tegra_pcie_phys_get_legacy(struct tegra_pcie *pcie)
>  	return 0;
>  }
>
> +static void tegra_pcie_phys_put_legacy(struct tegra_pcie *pcie)
> +{
> +	int err;
> +
> +	err = phy_exit(pcie->phy);
> +	if (err < 0)
> +		dev_err(pcie->dev, "failed to teardown PHY: %d\n", err);
> +}
> +
>  static struct phy *devm_of_phy_optional_get_index(struct device *dev,
>  						  struct device_node *np,
>  						  const char *consumer,
> @@ -1315,6 +1346,19 @@ static int tegra_pcie_port_get_phys(struct tegra_pcie_port *port)
>  	return 0;
>  }
>
> +static void tegra_pcie_port_put_phys(struct tegra_pcie_port *port)
> +{
> +	struct device *dev = port->pcie->dev;
> +	unsigned int i;
> +	int err;
> +
> +	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_phys_get(struct tegra_pcie *pcie)
>  {
>  	const struct tegra_pcie_soc *soc = pcie->soc;
> @@ -1334,6 +1378,19 @@ static int tegra_pcie_phys_get(struct tegra_pcie *pcie)
>  	return 0;
>  }
>
> +static void tegra_pcie_phys_put(struct tegra_pcie *pcie)
> +{
> +	const struct tegra_pcie_soc *soc = pcie->soc;
> +	struct device_node *np = pcie->dev->of_node;
> +	struct tegra_pcie_port *port;
> +
> +	if (!soc->has_gen2 || of_find_property(np, "phys", NULL) != NULL)
> +		tegra_pcie_phys_put_legacy(pcie);

I think it would be nicer to just check if legacy_phy is true, since 
tegra_pcie_phys_get_legacy sets it. That way we don't need to have the 
complicated check in two places.

Mikko

> +
> +	list_for_each_entry(port, &pcie->ports, list)
> +		tegra_pcie_port_put_phys(port);
> +}
> +
>  static int tegra_pcie_get_resources(struct tegra_pcie *pcie)
>  {
>  	struct device *dev = pcie->dev;
> @@ -1366,7 +1423,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");
> @@ -1424,25 +1481,23 @@ static int tegra_pcie_get_resources(struct tegra_pcie *pcie)
>
>  poweroff:
>  	tegra_pcie_power_off(pcie);
> +phys_put:
> +	if (soc->program_uphy)
> +		tegra_pcie_phys_put(pcie);
>  	return err;
>  }
>
>  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;
>  }
> @@ -2371,6 +2426,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;
> +
> +	reset_control_assert(pcie->pcie_xrst);
> +
> +	list_for_each_entry_safe(port, tmp, &pcie->ports, list)
> +		tegra_pcie_port_disable(port);
> +}
> +
>  static void tegra_pcie_change_link_speed(struct tegra_pcie *pcie,
>  					 struct pci_dev *pci_dev)
>  {
> @@ -2691,7 +2756,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);
> @@ -2700,7 +2765,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;
>  		}
>  	}
>
> @@ -2741,6 +2806,11 @@ static int tegra_pcie_probe(struct platform_device *pdev)
>  disable_msi:
>  	if (IS_ENABLED(CONFIG_PCI_MSI))
>  		tegra_pcie_disable_msi(pcie);
> +	tegra_pcie_disable_ports(pcie);
> +free_resources:
> +	tegra_pcie_free_resources(pcie);
> +disable_controller:
> +	tegra_pcie_disable_controller(pcie);
>  put_resources:
>  	tegra_pcie_put_resources(pcie);
>  	return err;
>

Re: [PATCH V2 6/9] PCI: tegra: free resources on probe failure

[Bjorn Helgaas]

Please capitalize the first word of the changelog summary so it
matches the rest of your series (and the rest of drivers/pci history).

On Sun, Nov 26, 2017 at 01:02:10AM +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@nvidia.com>

[PATCH V2 7/9] 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

 drivers/pci/host/Kconfig     |  2 +-
 drivers/pci/host/pci-tegra.c | 31 ++++++++++++++++++++++++++++++-
 2 files changed, 31 insertions(+), 2 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 7f7b8c9c1e84..bbc2807bcd4a 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>
@@ -2720,6 +2721,12 @@ static int tegra_pcie_debugfs_init(struct tegra_pcie *pcie)
 	return -ENOMEM;
 }
 
+static void tegra_pcie_debugfs_exit(struct tegra_pcie *pcie)
+{
+	debugfs_remove_recursive(pcie->debugfs);
+	pcie->debugfs = NULL;
+}
+
 static int tegra_pcie_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -2734,6 +2741,7 @@ static int tegra_pcie_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	pcie = pci_host_bridge_priv(host);
+	platform_set_drvdata(pdev, pcie);
 
 	pcie->soc = of_device_get_match_data(dev);
 	INIT_LIST_HEAD(&pcie->buses);
@@ -2816,6 +2824,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);
+	if (IS_ENABLED(CONFIG_PCI_MSI))
+		tegra_pcie_disable_msi(pcie);
+	tegra_pcie_disable_ports(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",
@@ -2823,5 +2850,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 V2 7/9] PCI: tegra: Add loadable kernel module support

[Mikko Perttunen]

On 25.11.2017 21:32, 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@nvidia.com>
> ---
> V2:
> * no change in this patch
>
>  drivers/pci/host/Kconfig     |  2 +-
>  drivers/pci/host/pci-tegra.c | 31 ++++++++++++++++++++++++++++++-
>  2 files changed, 31 insertions(+), 2 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 7f7b8c9c1e84..bbc2807bcd4a 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>
> @@ -2720,6 +2721,12 @@ static int tegra_pcie_debugfs_init(struct tegra_pcie *pcie)
>  	return -ENOMEM;
>  }
>
> +static void tegra_pcie_debugfs_exit(struct tegra_pcie *pcie)
> +{
> +	debugfs_remove_recursive(pcie->debugfs);
> +	pcie->debugfs = NULL;
> +}
> +

I think it's unnecessary to have a helper function for this - just 
inline it in the remove function.

>  static int tegra_pcie_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> @@ -2734,6 +2741,7 @@ static int tegra_pcie_probe(struct platform_device *pdev)
>  		return -ENOMEM;
>
>  	pcie = pci_host_bridge_priv(host);
> +	platform_set_drvdata(pdev, pcie);
>
>  	pcie->soc = of_device_get_match_data(dev);
>  	INIT_LIST_HEAD(&pcie->buses);
> @@ -2816,6 +2824,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);
> +	if (IS_ENABLED(CONFIG_PCI_MSI))
> +		tegra_pcie_disable_msi(pcie);
> +	tegra_pcie_disable_ports(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",
> @@ -2823,5 +2850,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");
>

Re: [PATCH V2 7/9] PCI: tegra: Add loadable kernel module support

[Manikanta Maddireddy]

On 29-Nov-17 5:31 PM, Mikko Perttunen wrote:
> On 25.11.2017 21:32, 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@nvidia.com>
>> ---
>> V2:
>> * no change in this patch
>>
>>  drivers/pci/host/Kconfig     |  2 +-
>>  drivers/pci/host/pci-tegra.c | 31 ++++++++++++++++++++++++++++++-
>>  2 files changed, 31 insertions(+), 2 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 7f7b8c9c1e84..bbc2807bcd4a 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>
>> @@ -2720,6 +2721,12 @@ static int tegra_pcie_debugfs_init(struct tegra_pcie *pcie)
>>      return -ENOMEM;
>>  }
>>
>> +static void tegra_pcie_debugfs_exit(struct tegra_pcie *pcie)
>> +{
>> +    debugfs_remove_recursive(pcie->debugfs);
>> +    pcie->debugfs = NULL;
>> +}
>> +
> 
> I think it's unnecessary to have a helper function for this - just inline it in the remove function.
This helper function can be used in tegra_pcie_debugfs_init(), I will publish a new patch calling this
helper function in both tegra_pcie_debugfs_exit and remove.
> 
>>  static int tegra_pcie_probe(struct platform_device *pdev)
>>  {
>>      struct device *dev = &pdev->dev;
>> @@ -2734,6 +2741,7 @@ static int tegra_pcie_probe(struct platform_device *pdev)
>>          return -ENOMEM;
>>
>>      pcie = pci_host_bridge_priv(host);
>> +    platform_set_drvdata(pdev, pcie);
>>
>>      pcie->soc = of_device_get_match_data(dev);
>>      INIT_LIST_HEAD(&pcie->buses);
>> @@ -2816,6 +2824,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);
>> +    if (IS_ENABLED(CONFIG_PCI_MSI))
>> +        tegra_pcie_disable_msi(pcie);
>> +    tegra_pcie_disable_ports(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",
>> @@ -2823,5 +2850,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");
>>

[PATCH V2 8/9] 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

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

diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
index bbc2807bcd4a..b380958a3deb 100644
--- a/drivers/pci/host/pci-tegra.c
+++ b/drivers/pci/host/pci-tegra.c
@@ -155,6 +155,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
@@ -315,6 +317,7 @@
 #define PADS_REFCLK_CFG_DRVI_SHIFT		12 /* 15:12 */
 
 #define LINK_RETRAIN_TIMEOUT 100000
+#define PME_ACK_TIMEOUT 10000
 
 struct tegra_msi {
 	struct msi_controller chip;
@@ -1503,6 +1506,76 @@ static int tegra_pcie_put_resources(struct tegra_pcie *pcie)
 	return 0;
 }
 
+static inline u32 get_pme_turnoff_bitmap(struct tegra_pcie_port *port)
+{
+	struct device *dev = port->pcie->dev;
+	struct device_node *np = dev->of_node;
+	int ret = 0;
+
+	switch (port->index) {
+	case 0:
+		ret = 0;
+	case 1:
+		ret = 8;
+	case 2:
+		if (of_device_is_compatible(np, "nvidia,tegra30-pcie"))
+			ret = 16;
+		else
+			ret = 12;
+	}
+	return ret;
+}
+
+static inline u32 get_pme_ack_bitmap(struct tegra_pcie_port *port)
+{
+	struct device *dev = port->pcie->dev;
+	struct device_node *np = dev->of_node;
+	int ret = 0;
+
+	switch (port->index) {
+	case 0:
+		ret = 5;
+	case 1:
+		ret = 10;
+	case 2:
+		if (of_device_is_compatible(np, "nvidia,tegra30-pcie"))
+			ret = 18;
+		else
+			ret = 14;
+	}
+	return ret;
+}
+
+static void tegra_pcie_pme_turnoff(struct tegra_pcie_port *port)
+{
+	struct tegra_pcie *pcie = port->pcie;
+	ktime_t deadline;
+	unsigned int data;
+
+	data = afi_readl(pcie, AFI_PCIE_PME);
+	data |= (0x1 << get_pme_turnoff_bitmap(port));
+	afi_writel(pcie, data, AFI_PCIE_PME);
+
+	deadline = ktime_add_us(ktime_get(), PME_ACK_TIMEOUT);
+	do {
+		data = afi_readl(pcie, AFI_PCIE_PME);
+		data &= (0x1 << get_pme_ack_bitmap(port));
+		udelay(1);
+		if (ktime_after(ktime_get(), deadline))
+			break;
+	} while (!data);
+
+	if (data)
+		dev_err(pcie->dev, "PME Ack is not receieved on port: %d\n",
+			port->index);
+
+	usleep_range(10000, 11000);
+
+	data = afi_readl(pcie, AFI_PCIE_PME);
+	data &= ~(0x1 << get_pme_turnoff_bitmap(port));
+	afi_writel(pcie, data, AFI_PCIE_PME);
+}
+
 static int tegra_msi_alloc(struct tegra_msi *chip)
 {
 	int msi;
@@ -2828,6 +2901,7 @@ 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);
@@ -2835,6 +2909,8 @@ static int tegra_pcie_remove(struct platform_device *pdev)
 	pci_remove_root_bus(host->bus);
 	if (IS_ENABLED(CONFIG_PCI_MSI))
 		tegra_pcie_disable_msi(pcie);
+	list_for_each_entry_safe(port, tmp, &pcie->ports, list)
+		tegra_pcie_pme_turnoff(port);
 	tegra_pcie_disable_ports(pcie);
 	tegra_pcie_free_resources(pcie);
 	tegra_pcie_disable_controller(pcie);
-- 
2.1.4

Re: [PATCH V2 8/9] PCI: tegra: Broadcast PME_turn_Off message before link goes to L2

[Mikko Perttunen]

On 25.11.2017 21:32, 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
>
>  drivers/pci/host/pci-tegra.c | 76 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 76 insertions(+)
>
> diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
> index bbc2807bcd4a..b380958a3deb 100644
> --- a/drivers/pci/host/pci-tegra.c
> +++ b/drivers/pci/host/pci-tegra.c
> @@ -155,6 +155,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
> @@ -315,6 +317,7 @@
>  #define PADS_REFCLK_CFG_DRVI_SHIFT		12 /* 15:12 */
>
>  #define LINK_RETRAIN_TIMEOUT 100000
> +#define PME_ACK_TIMEOUT 10000
>
>  struct tegra_msi {
>  	struct msi_controller chip;
> @@ -1503,6 +1506,76 @@ static int tegra_pcie_put_resources(struct tegra_pcie *pcie)
>  	return 0;
>  }
>
> +static inline u32 get_pme_turnoff_bitmap(struct tegra_pcie_port *port)
> +{
> +	struct device *dev = port->pcie->dev;
> +	struct device_node *np = dev->of_node;
> +	int ret = 0;
> +
> +	switch (port->index) {
> +	case 0:
> +		ret = 0;
> +	case 1:
> +		ret = 8;
> +	case 2:
> +		if (of_device_is_compatible(np, "nvidia,tegra30-pcie"))
> +			ret = 16;
> +		else
> +			ret = 12;
> +	}
> +	return ret;
> +}
> +
> +static inline u32 get_pme_ack_bitmap(struct tegra_pcie_port *port)
> +{
> +	struct device *dev = port->pcie->dev;
> +	struct device_node *np = dev->of_node;
> +	int ret = 0;
> +
> +	switch (port->index) {
> +	case 0:
> +		ret = 5;
> +	case 1:
> +		ret = 10;
> +	case 2:
> +		if (of_device_is_compatible(np, "nvidia,tegra30-pcie"))
> +			ret = 18;
> +		else
> +			ret = 14;
> +	}
> +	return ret;
> +}

 From what I can tell, the port 2 bit is 12/14 on everything after 
Tegra30 as well, so I don't think this actually works?

I think simpler would be to add a SoC data fields 'u8 
pme_turnoff_bit[3]' and 'u8 pme_ack_bit[3]' and then set that
to '.pme_turnoff_bit = { 0, 8, 16 }' and so on.

> +
> +static void tegra_pcie_pme_turnoff(struct tegra_pcie_port *port)
> +{
> +	struct tegra_pcie *pcie = port->pcie;
> +	ktime_t deadline;
> +	unsigned int data;
> +
> +	data = afi_readl(pcie, AFI_PCIE_PME);
> +	data |= (0x1 << get_pme_turnoff_bitmap(port));
> +	afi_writel(pcie, data, AFI_PCIE_PME);
> +
> +	deadline = ktime_add_us(ktime_get(), PME_ACK_TIMEOUT);
> +	do {
> +		data = afi_readl(pcie, AFI_PCIE_PME);
> +		data &= (0x1 << get_pme_ack_bitmap(port));
> +		udelay(1);
> +		if (ktime_after(ktime_get(), deadline))
> +			break;
> +	} while (!data);

Since this is a normal MMIO read, we could replace the whole loop with a 
call to readl_poll_timeout (or readl_relaxed_poll_timeout, or if we 
really must delay and not sleep, readl_poll_timeout_atomic etc.) from 
iopoll.h

   int err;
   u32 val;

   err = readl_poll_timeout(pcie->afi + AFI_PCIE_PME, val,
                            val & (0x1 << pcie->soc->pme_ack_bit[port]),
                            1, PME_ACK_TIMEOUT);
   if (err)
     ...

> +
> +	if (data)
> +		dev_err(pcie->dev, "PME Ack is not receieved on port: %d\n",
> +			port->index);
> +

Typo here, s/receieved/received/

Cheers,
Mikko

> +	usleep_range(10000, 11000);
> +
> +	data = afi_readl(pcie, AFI_PCIE_PME);
> +	data &= ~(0x1 << get_pme_turnoff_bitmap(port));
> +	afi_writel(pcie, data, AFI_PCIE_PME);
> +}
> +
>  static int tegra_msi_alloc(struct tegra_msi *chip)
>  {
>  	int msi;
> @@ -2828,6 +2901,7 @@ 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);
> @@ -2835,6 +2909,8 @@ static int tegra_pcie_remove(struct platform_device *pdev)
>  	pci_remove_root_bus(host->bus);
>  	if (IS_ENABLED(CONFIG_PCI_MSI))
>  		tegra_pcie_disable_msi(pcie);
> +	list_for_each_entry_safe(port, tmp, &pcie->ports, list)
> +		tegra_pcie_pme_turnoff(port);
>  	tegra_pcie_disable_ports(pcie);
>  	tegra_pcie_free_resources(pcie);
>  	tegra_pcie_disable_controller(pcie);
>

Re: [PATCH V2 8/9] PCI: tegra: Broadcast PME_turn_Off message before link goes to L2

[Mikko Perttunen]

On 29.11.2017 14:18, Mikko Perttunen wrote:
> On 25.11.2017 21:32, 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
>>
>>  drivers/pci/host/pci-tegra.c | 76
>> ++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 76 insertions(+)
>>
>> diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
>> index bbc2807bcd4a..b380958a3deb 100644
>> --- a/drivers/pci/host/pci-tegra.c
>> +++ b/drivers/pci/host/pci-tegra.c
>> @@ -155,6 +155,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
>> @@ -315,6 +317,7 @@
>>  #define PADS_REFCLK_CFG_DRVI_SHIFT        12 /* 15:12 */
>>
>>  #define LINK_RETRAIN_TIMEOUT 100000
>> +#define PME_ACK_TIMEOUT 10000
>>
>>  struct tegra_msi {
>>      struct msi_controller chip;
>> @@ -1503,6 +1506,76 @@ static int tegra_pcie_put_resources(struct
>> tegra_pcie *pcie)
>>      return 0;
>>  }
>>
>> +static inline u32 get_pme_turnoff_bitmap(struct tegra_pcie_port *port)
>> +{
>> +    struct device *dev = port->pcie->dev;
>> +    struct device_node *np = dev->of_node;
>> +    int ret = 0;
>> +
>> +    switch (port->index) {
>> +    case 0:
>> +        ret = 0;
>> +    case 1:
>> +        ret = 8;
>> +    case 2:
>> +        if (of_device_is_compatible(np, "nvidia,tegra30-pcie"))
>> +            ret = 16;
>> +        else
>> +            ret = 12;
>> +    }
>> +    return ret;
>> +}
>> +
>> +static inline u32 get_pme_ack_bitmap(struct tegra_pcie_port *port)
>> +{
>> +    struct device *dev = port->pcie->dev;
>> +    struct device_node *np = dev->of_node;
>> +    int ret = 0;
>> +
>> +    switch (port->index) {
>> +    case 0:
>> +        ret = 5;
>> +    case 1:
>> +        ret = 10;
>> +    case 2:
>> +        if (of_device_is_compatible(np, "nvidia,tegra30-pcie"))
>> +            ret = 18;
>> +        else
>> +            ret = 14;
>> +    }
>> +    return ret;
>> +}
>
> From what I can tell, the port 2 bit is 12/14 on everything after
> Tegra30 as well, so I don't think this actually works?

Looks like I misread this and it is correct, sorry :)
Still the below applies.

Thanks,
Mikko

>
> I think simpler would be to add a SoC data fields 'u8
> pme_turnoff_bit[3]' and 'u8 pme_ack_bit[3]' and then set that
> to '.pme_turnoff_bit = { 0, 8, 16 }' and so on.
>
>> +
>> +static void tegra_pcie_pme_turnoff(struct tegra_pcie_port *port)
>> +{
>> +    struct tegra_pcie *pcie = port->pcie;
>> +    ktime_t deadline;
>> +    unsigned int data;
>> +
>> +    data = afi_readl(pcie, AFI_PCIE_PME);
>> +    data |= (0x1 << get_pme_turnoff_bitmap(port));
>> +    afi_writel(pcie, data, AFI_PCIE_PME);
>> +
>> +    deadline = ktime_add_us(ktime_get(), PME_ACK_TIMEOUT);
>> +    do {
>> +        data = afi_readl(pcie, AFI_PCIE_PME);
>> +        data &= (0x1 << get_pme_ack_bitmap(port));
>> +        udelay(1);
>> +        if (ktime_after(ktime_get(), deadline))
>> +            break;
>> +    } while (!data);
>
> Since this is a normal MMIO read, we could replace the whole loop with a
> call to readl_poll_timeout (or readl_relaxed_poll_timeout, or if we
> really must delay and not sleep, readl_poll_timeout_atomic etc.) from
> iopoll.h
>
>   int err;
>   u32 val;
>
>   err = readl_poll_timeout(pcie->afi + AFI_PCIE_PME, val,
>                            val & (0x1 << pcie->soc->pme_ack_bit[port]),
>                            1, PME_ACK_TIMEOUT);
>   if (err)
>     ...
>
>> +
>> +    if (data)
>> +        dev_err(pcie->dev, "PME Ack is not receieved on port: %d\n",
>> +            port->index);
>> +
>
> Typo here, s/receieved/received/
>
> Cheers,
> Mikko
>
>> +    usleep_range(10000, 11000);
>> +
>> +    data = afi_readl(pcie, AFI_PCIE_PME);
>> +    data &= ~(0x1 << get_pme_turnoff_bitmap(port));
>> +    afi_writel(pcie, data, AFI_PCIE_PME);
>> +}
>> +
>>  static int tegra_msi_alloc(struct tegra_msi *chip)
>>  {
>>      int msi;
>> @@ -2828,6 +2901,7 @@ 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);
>> @@ -2835,6 +2909,8 @@ static int tegra_pcie_remove(struct
>> platform_device *pdev)
>>      pci_remove_root_bus(host->bus);
>>      if (IS_ENABLED(CONFIG_PCI_MSI))
>>          tegra_pcie_disable_msi(pcie);
>> +    list_for_each_entry_safe(port, tmp, &pcie->ports, list)
>> +        tegra_pcie_pme_turnoff(port);
>>      tegra_pcie_disable_ports(pcie);
>>      tegra_pcie_free_resources(pcie);
>>      tegra_pcie_disable_controller(pcie);
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V2 8/9] PCI: tegra: Broadcast PME_turn_Off message before link goes to L2

[Bjorn Helgaas]

On Sun, Nov 26, 2017 at 01:02:12AM +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.

s/PME_turn_Off/PME_Turn_Off/ above to match spec.

I thought PME_TO_Ack was also mis-capitalized, but it's not.  Guess
that "TO" stands for "Turn Off".

> Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
> ---
> V2:
> * no change in this patch
> 
>  drivers/pci/host/pci-tegra.c | 76 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 76 insertions(+)
> 
> diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
> index bbc2807bcd4a..b380958a3deb 100644
> --- a/drivers/pci/host/pci-tegra.c
> +++ b/drivers/pci/host/pci-tegra.c
> @@ -155,6 +155,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
> @@ -315,6 +317,7 @@
>  #define PADS_REFCLK_CFG_DRVI_SHIFT		12 /* 15:12 */
>  
>  #define LINK_RETRAIN_TIMEOUT 100000
> +#define PME_ACK_TIMEOUT 10000
>  
>  struct tegra_msi {
>  	struct msi_controller chip;
> @@ -1503,6 +1506,76 @@ static int tegra_pcie_put_resources(struct tegra_pcie *pcie)
>  	return 0;
>  }
>  
> +static inline u32 get_pme_turnoff_bitmap(struct tegra_pcie_port *port)
> +{
> +	struct device *dev = port->pcie->dev;
> +	struct device_node *np = dev->of_node;
> +	int ret = 0;

Unnecessary initialization.  In fact, the variable is unnecessary; you
can just return the value directly as soon as you know it.

> +
> +	switch (port->index) {
> +	case 0:
> +		ret = 0;
> +	case 1:
> +		ret = 8;
> +	case 2:
> +		if (of_device_is_compatible(np, "nvidia,tegra30-pcie"))
> +			ret = 16;
> +		else
> +			ret = 12;
> +	}
> +	return ret;
> +}
> +
> +static inline u32 get_pme_ack_bitmap(struct tegra_pcie_port *port)
> +{
> +	struct device *dev = port->pcie->dev;
> +	struct device_node *np = dev->of_node;
> +	int ret = 0;

Similar.

> +
> +	switch (port->index) {
> +	case 0:
> +		ret = 5;
> +	case 1:
> +		ret = 10;
> +	case 2:
> +		if (of_device_is_compatible(np, "nvidia,tegra30-pcie"))
> +			ret = 18;
> +		else
> +			ret = 14;
> +	}
> +	return ret;
> +}
> +
> +static void tegra_pcie_pme_turnoff(struct tegra_pcie_port *port)
> +{
> +	struct tegra_pcie *pcie = port->pcie;
> +	ktime_t deadline;
> +	unsigned int data;
> +
> +	data = afi_readl(pcie, AFI_PCIE_PME);
> +	data |= (0x1 << get_pme_turnoff_bitmap(port));
> +	afi_writel(pcie, data, AFI_PCIE_PME);
> +
> +	deadline = ktime_add_us(ktime_get(), PME_ACK_TIMEOUT);
> +	do {
> +		data = afi_readl(pcie, AFI_PCIE_PME);
> +		data &= (0x1 << get_pme_ack_bitmap(port));
> +		udelay(1);
> +		if (ktime_after(ktime_get(), deadline))
> +			break;
> +	} while (!data);
> +
> +	if (data)
> +		dev_err(pcie->dev, "PME Ack is not receieved on port: %d\n",

s/receieved/received/

> +			port->index);
> +
> +	usleep_range(10000, 11000);
> +
> +	data = afi_readl(pcie, AFI_PCIE_PME);
> +	data &= ~(0x1 << get_pme_turnoff_bitmap(port));
> +	afi_writel(pcie, data, AFI_PCIE_PME);
> +}
> +
>  static int tegra_msi_alloc(struct tegra_msi *chip)
>  {
>  	int msi;
> @@ -2828,6 +2901,7 @@ 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);
> @@ -2835,6 +2909,8 @@ static int tegra_pcie_remove(struct platform_device *pdev)
>  	pci_remove_root_bus(host->bus);
>  	if (IS_ENABLED(CONFIG_PCI_MSI))
>  		tegra_pcie_disable_msi(pcie);
> +	list_for_each_entry_safe(port, tmp, &pcie->ports, list)
> +		tegra_pcie_pme_turnoff(port);
>  	tegra_pcie_disable_ports(pcie);
>  	tegra_pcie_free_resources(pcie);
>  	tegra_pcie_disable_controller(pcie);
> -- 
> 2.1.4
> 

Re: [PATCH V2 8/9] PCI: tegra: Broadcast PME_turn_Off message before link goes to L2

[Manikanta Maddireddy]

On 29-Nov-17 10:21 PM, Bjorn Helgaas wrote:
> On Sun, Nov 26, 2017 at 01:02:12AM +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.
> 
> s/PME_turn_Off/PME_Turn_Off/ above to match spec.
> 
> I thought PME_TO_Ack was also mis-capitalized, but it's not.  Guess
> that "TO" stands for "Turn Off".
> 
>> Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
>> ---
>> V2:
>> * no change in this patch
>>
>>  drivers/pci/host/pci-tegra.c | 76 ++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 76 insertions(+)
>>
>> diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
>> index bbc2807bcd4a..b380958a3deb 100644
>> --- a/drivers/pci/host/pci-tegra.c
>> +++ b/drivers/pci/host/pci-tegra.c
>> @@ -155,6 +155,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
>> @@ -315,6 +317,7 @@
>>  #define PADS_REFCLK_CFG_DRVI_SHIFT		12 /* 15:12 */
>>  
>>  #define LINK_RETRAIN_TIMEOUT 100000
>> +#define PME_ACK_TIMEOUT 10000
>>  
>>  struct tegra_msi {
>>  	struct msi_controller chip;
>> @@ -1503,6 +1506,76 @@ static int tegra_pcie_put_resources(struct tegra_pcie *pcie)
>>  	return 0;
>>  }
>>  
>> +static inline u32 get_pme_turnoff_bitmap(struct tegra_pcie_port *port)
>> +{
>> +	struct device *dev = port->pcie->dev;
>> +	struct device_node *np = dev->of_node;
>> +	int ret = 0;
> 
> Unnecessary initialization.  In fact, the variable is unnecessary; you
> can just return the value directly as soon as you know it.
>
I can get rid of this function with Mikko's suggestion to use soc data
 
>> +
>> +	switch (port->index) {
>> +	case 0:
>> +		ret = 0;
>> +	case 1:
>> +		ret = 8;
>> +	case 2:
>> +		if (of_device_is_compatible(np, "nvidia,tegra30-pcie"))
>> +			ret = 16;
>> +		else
>> +			ret = 12;
>> +	}
>> +	return ret;
>> +}
>> +
>> +static inline u32 get_pme_ack_bitmap(struct tegra_pcie_port *port)
>> +{
>> +	struct device *dev = port->pcie->dev;
>> +	struct device_node *np = dev->of_node;
>> +	int ret = 0;
> 
> Similar.
>
Similar
 
>> +
>> +	switch (port->index) {
>> +	case 0:
>> +		ret = 5;
>> +	case 1:
>> +		ret = 10;
>> +	case 2:
>> +		if (of_device_is_compatible(np, "nvidia,tegra30-pcie"))
>> +			ret = 18;
>> +		else
>> +			ret = 14;
>> +	}
>> +	return ret;
>> +}
>> +
>> +static void tegra_pcie_pme_turnoff(struct tegra_pcie_port *port)
>> +{
>> +	struct tegra_pcie *pcie = port->pcie;
>> +	ktime_t deadline;
>> +	unsigned int data;
>> +
>> +	data = afi_readl(pcie, AFI_PCIE_PME);
>> +	data |= (0x1 << get_pme_turnoff_bitmap(port));
>> +	afi_writel(pcie, data, AFI_PCIE_PME);
>> +
>> +	deadline = ktime_add_us(ktime_get(), PME_ACK_TIMEOUT);
>> +	do {
>> +		data = afi_readl(pcie, AFI_PCIE_PME);
>> +		data &= (0x1 << get_pme_ack_bitmap(port));
>> +		udelay(1);
>> +		if (ktime_after(ktime_get(), deadline))
>> +			break;
>> +	} while (!data);
>> +
>> +	if (data)
>> +		dev_err(pcie->dev, "PME Ack is not receieved on port: %d\n",
> 
> s/receieved/received/
> 
>> +			port->index);
>> +
>> +	usleep_range(10000, 11000);
>> +
>> +	data = afi_readl(pcie, AFI_PCIE_PME);
>> +	data &= ~(0x1 << get_pme_turnoff_bitmap(port));
>> +	afi_writel(pcie, data, AFI_PCIE_PME);
>> +}
>> +
>>  static int tegra_msi_alloc(struct tegra_msi *chip)
>>  {
>>  	int msi;
>> @@ -2828,6 +2901,7 @@ 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);
>> @@ -2835,6 +2909,8 @@ static int tegra_pcie_remove(struct platform_device *pdev)
>>  	pci_remove_root_bus(host->bus);
>>  	if (IS_ENABLED(CONFIG_PCI_MSI))
>>  		tegra_pcie_disable_msi(pcie);
>> +	list_for_each_entry_safe(port, tmp, &pcie->ports, list)
>> +		tegra_pcie_pme_turnoff(port);
>>  	tegra_pcie_disable_ports(pcie);
>>  	tegra_pcie_free_resources(pcie);
>>  	tegra_pcie_disable_controller(pcie);
>> -- 
>> 2.1.4
>>

[PATCH V2 9/9] 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

 drivers/pci/host/pci-tegra.c | 174 ++++++++++++++++++++++++++-----------------
 1 file changed, 106 insertions(+), 68 deletions(-)

diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
index b380958a3deb..1bfdfcc8d2c1 100644
--- a/drivers/pci/host/pci-tegra.c
+++ b/drivers/pci/host/pci-tegra.c
@@ -1424,31 +1424,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;
 	}
 
 	axi_addr = pcie->soc->use_4k_conf_space ?
@@ -1456,21 +1450,21 @@ static int tegra_pcie_get_resources(struct tegra_pcie *pcie)
 	pcie->cs = devm_request_mem_region(dev, axi_addr, SZ_4K, res->name);
 	if (!pcie->cs) {
 		err = -EADDRNOTAVAIL;
-		goto poweroff;
+		goto phys_put;
 	}
 
 	pcie->cfg_va_base = devm_ioremap(dev, pcie->cs->start, SZ_4K);
 	if (!pcie->cfg_va_base) {
 		dev_err(pcie->dev, "failed to ioremap config space\n");
 		err = -EADDRNOTAVAIL;
-		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;
@@ -1478,13 +1472,11 @@ 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;
 
-poweroff:
-	tegra_pcie_power_off(pcie);
 phys_put:
 	if (soc->program_uphy)
 		tegra_pcie_phys_put(pcie);
@@ -1498,8 +1490,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);
 
@@ -1722,37 +1712,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);
@@ -2831,26 +2825,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;
 
 	pci_add_flags(PCI_REASSIGN_ALL_RSRC | PCI_REASSIGN_ALL_BUS);
 	host->busnr = pcie->busn.start;
@@ -2862,7 +2846,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 free_resources;
 	}
 
 	pci_bus_size_bridges(host->bus);
@@ -2884,14 +2868,13 @@ static int tegra_pcie_probe(struct platform_device *pdev)
 
 	return 0;
 
-disable_msi:
-	if (IS_ENABLED(CONFIG_PCI_MSI))
-		tegra_pcie_disable_msi(pcie);
-	tegra_pcie_disable_ports(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;
@@ -2901,7 +2884,6 @@ 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);
@@ -2909,21 +2891,77 @@ static int tegra_pcie_remove(struct platform_device *pdev)
 	pci_remove_root_bus(host->bus);
 	if (IS_ENABLED(CONFIG_PCI_MSI))
 		tegra_pcie_disable_msi(pcie);
+	tegra_pcie_free_resources(pcie);
+	pm_runtime_put_sync(pcie->dev);
+	pm_runtime_disable(pcie->dev);
+	tegra_pcie_put_resources(pcie);
+
+	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_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)) {
+		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