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

Message ID 1515650888-9459-7-git-send-email-mmaddireddy@nvidia.com
State Superseded
Headers show
Series
  • Add loadable kernel module and power management support
Related show

Commit Message

Manikanta Maddireddy Jan. 11, 2018, 6:08 a.m.
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(+)

Comments

Thierry Reding Jan. 25, 2018, 2:36 p.m. | #1
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
Manikanta Maddireddy Jan. 29, 2018, 4:41 a.m. | #2
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?

>>  	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 linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Thierry Reding Jan. 29, 2018, 3:18 p.m. | #3
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

Patch

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);