[V3,3/3] PCI: tegra: Enable ASPM-L1 capability advertisement

Message ID 1510492674-12786-4-git-send-email-vidyas@nvidia.com
State Changes Requested
Headers show
Series
  • Add ASPM-L1 Substates support for Tegra
Related show

Commit Message

Vidya Sagar Nov. 12, 2017, 1:17 p.m.
Enables advertisement of ASPM-L1 support in capability
registers of applicable Tegra chips

Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
---
V2:
* no change in this patch

V3:
* no change in this patch

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

Comments

Bjorn Helgaas Nov. 20, 2017, 9:37 p.m. | #1
s/Enable ASPM-L1 capability advertisement/
 /Advertise ASPM L1 PM Substates support/

On Sun, Nov 12, 2017 at 06:47:54PM +0530, Vidya Sagar wrote:
> Enables advertisement of ASPM-L1 support in capability
> registers of applicable Tegra chips

Actually, I'm a little confused about whether this has to do with ASPM
L1 support (which would be advertised in the Link Capabilities
register) or the ASPM L1 Substates support (which would be advertised
in the L1 PM Substates Capabilities register)?

> Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
> ---
> V2:
> * no change in this patch
> 
> V3:
> * no change in this patch
> 
>  drivers/pci/host/pci-tegra.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
> index 29ee4bb0b7c6..fb61202ee60f 100644
> --- a/drivers/pci/host/pci-tegra.c
> +++ b/drivers/pci/host/pci-tegra.c
> @@ -250,6 +250,9 @@
>  #define  RP_VEND_XP_UPDATE_FC_THRESHOLD_MASK	(0xff << 18)
>  #define  RP_VEND_XP_UPDATE_FC_THRESHOLD_T210	(0x60 << 18)
>  
> +#define RP_VEND_XP1	0xf04
> +#define RP_VEND_XP1_LINK_PVT_CTL_L1_ASPM_SUPPORT	BIT(21)
> +
>  #define RP_VEND_CTL0	0xf44
>  #define  RP_VEND_CTL0_DSK_RST_PULSE_WIDTH_MASK	(0xf << 12)
>  #define  RP_VEND_CTL0_DSK_RST_PULSE_WIDTH	(0x9 << 12)
> @@ -351,6 +354,7 @@ struct tegra_pcie_soc {
>  	bool RAW_violation_fixup;
>  	bool program_deskew_time;
>  	bool updateFC_threshold;
> +	bool has_aspm_l1;
>  	bool has_aspm_l1ss;
>  	bool l1ss_rp_wake_fixup;
>  };
> @@ -2214,6 +2218,13 @@ static void tegra_pcie_enable_rp_features(struct tegra_pcie_port *port)
>  	value = readl(port->base + RP_VEND_CTL1);
>  	value |= RP_VEND_CTL1_ERPT;
>  	writel(value, port->base + RP_VEND_CTL1);
> +
> +	if (port->pcie->soc->has_aspm_l1) {
> +		/* Advertise ASPM-L1 state capability*/
> +		value = readl(port->base + RP_VEND_XP1);
> +		value |= RP_VEND_XP1_LINK_PVT_CTL_L1_ASPM_SUPPORT;
> +		writel(value, port->base + RP_VEND_XP1);
> +	}
>  }
>  
>  static void tegra_pcie_apply_sw_fixup(struct tegra_pcie_port *port)
> @@ -2458,6 +2469,7 @@ static const struct tegra_pcie_soc tegra20_pcie = {
>  	.RAW_violation_fixup = false,
>  	.program_deskew_time = false,
>  	.updateFC_threshold = false,
> +	.has_aspm_l1 = false,
>  	.has_aspm_l1ss = false,
>  	.l1ss_rp_wake_fixup = false,
>  };
> @@ -2481,6 +2493,7 @@ static const struct tegra_pcie_soc tegra30_pcie = {
>  	.RAW_violation_fixup = false,
>  	.program_deskew_time = false,
>  	.updateFC_threshold = false,
> +	.has_aspm_l1 = true,
>  	.has_aspm_l1ss = false,
>  	.l1ss_rp_wake_fixup = false,
>  };
> @@ -2503,6 +2516,7 @@ static const struct tegra_pcie_soc tegra124_pcie = {
>  	.RAW_violation_fixup = true,
>  	.program_deskew_time = false,
>  	.updateFC_threshold = false,
> +	.has_aspm_l1 = true,
>  	.has_aspm_l1ss = false,
>  	.l1ss_rp_wake_fixup = false,
>  };
> @@ -2533,6 +2547,7 @@ static const struct tegra_pcie_soc tegra210_pcie = {
>  	.RAW_violation_fixup = false,
>  	.program_deskew_time = true,
>  	.updateFC_threshold = true,
> +	.has_aspm_l1 = true,
>  	.has_aspm_l1ss = true,
>  	.l1ss_rp_wake_fixup = true,
>  };
> @@ -2556,6 +2571,7 @@ static const struct tegra_pcie_soc tegra186_pcie = {
>  	.RAW_violation_fixup = false,
>  	.program_deskew_time = false,
>  	.updateFC_threshold = false,
> +	.has_aspm_l1 = true,
>  	.has_aspm_l1ss = true,
>  	.l1ss_rp_wake_fixup = false,
>  };
> -- 
> 2.7.4
>
Vidya Sagar Dec. 14, 2017, 4:31 p.m. | #2
On Tuesday 21 November 2017 03:07 AM, Bjorn Helgaas wrote:
> s/Enable ASPM-L1 capability advertisement/
>   /Advertise ASPM L1 PM Substates support/
This patch controls advertisement of ASPM-L1 capability (not ASPM-L1 
Substates)
>
> On Sun, Nov 12, 2017 at 06:47:54PM +0530, Vidya Sagar wrote:
>> Enables advertisement of ASPM-L1 support in capability
>> registers of applicable Tegra chips
> Actually, I'm a little confused about whether this has to do with ASPM
> L1 support (which would be advertised in the Link Capabilities
> register) or the ASPM L1 Substates support (which would be advertised
> in the L1 PM Substates Capabilities register)?
This is to do with ASPM L1 support (and not ASPM L1 Substates)
But, since ASPM-L1SS needs ASPM-L1 anyway, I think it is ok to enable
advertisement of ASPM-L1 in the last patch of the series.
>
>> Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
>> ---
>> V2:
>> * no change in this patch
>>
>> V3:
>> * no change in this patch
>>
>>   drivers/pci/host/pci-tegra.c | 16 ++++++++++++++++
>>   1 file changed, 16 insertions(+)
>>
>> diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
>> index 29ee4bb0b7c6..fb61202ee60f 100644
>> --- a/drivers/pci/host/pci-tegra.c
>> +++ b/drivers/pci/host/pci-tegra.c
>> @@ -250,6 +250,9 @@
>>   #define  RP_VEND_XP_UPDATE_FC_THRESHOLD_MASK	(0xff << 18)
>>   #define  RP_VEND_XP_UPDATE_FC_THRESHOLD_T210	(0x60 << 18)
>>   
>> +#define RP_VEND_XP1	0xf04
>> +#define RP_VEND_XP1_LINK_PVT_CTL_L1_ASPM_SUPPORT	BIT(21)
>> +
>>   #define RP_VEND_CTL0	0xf44
>>   #define  RP_VEND_CTL0_DSK_RST_PULSE_WIDTH_MASK	(0xf << 12)
>>   #define  RP_VEND_CTL0_DSK_RST_PULSE_WIDTH	(0x9 << 12)
>> @@ -351,6 +354,7 @@ struct tegra_pcie_soc {
>>   	bool RAW_violation_fixup;
>>   	bool program_deskew_time;
>>   	bool updateFC_threshold;
>> +	bool has_aspm_l1;
>>   	bool has_aspm_l1ss;
>>   	bool l1ss_rp_wake_fixup;
>>   };
>> @@ -2214,6 +2218,13 @@ static void tegra_pcie_enable_rp_features(struct tegra_pcie_port *port)
>>   	value = readl(port->base + RP_VEND_CTL1);
>>   	value |= RP_VEND_CTL1_ERPT;
>>   	writel(value, port->base + RP_VEND_CTL1);
>> +
>> +	if (port->pcie->soc->has_aspm_l1) {
>> +		/* Advertise ASPM-L1 state capability*/
>> +		value = readl(port->base + RP_VEND_XP1);
>> +		value |= RP_VEND_XP1_LINK_PVT_CTL_L1_ASPM_SUPPORT;
>> +		writel(value, port->base + RP_VEND_XP1);
>> +	}
>>   }
>>   
>>   static void tegra_pcie_apply_sw_fixup(struct tegra_pcie_port *port)
>> @@ -2458,6 +2469,7 @@ static const struct tegra_pcie_soc tegra20_pcie = {
>>   	.RAW_violation_fixup = false,
>>   	.program_deskew_time = false,
>>   	.updateFC_threshold = false,
>> +	.has_aspm_l1 = false,
>>   	.has_aspm_l1ss = false,
>>   	.l1ss_rp_wake_fixup = false,
>>   };
>> @@ -2481,6 +2493,7 @@ static const struct tegra_pcie_soc tegra30_pcie = {
>>   	.RAW_violation_fixup = false,
>>   	.program_deskew_time = false,
>>   	.updateFC_threshold = false,
>> +	.has_aspm_l1 = true,
>>   	.has_aspm_l1ss = false,
>>   	.l1ss_rp_wake_fixup = false,
>>   };
>> @@ -2503,6 +2516,7 @@ static const struct tegra_pcie_soc tegra124_pcie = {
>>   	.RAW_violation_fixup = true,
>>   	.program_deskew_time = false,
>>   	.updateFC_threshold = false,
>> +	.has_aspm_l1 = true,
>>   	.has_aspm_l1ss = false,
>>   	.l1ss_rp_wake_fixup = false,
>>   };
>> @@ -2533,6 +2547,7 @@ static const struct tegra_pcie_soc tegra210_pcie = {
>>   	.RAW_violation_fixup = false,
>>   	.program_deskew_time = true,
>>   	.updateFC_threshold = true,
>> +	.has_aspm_l1 = true,
>>   	.has_aspm_l1ss = true,
>>   	.l1ss_rp_wake_fixup = true,
>>   };
>> @@ -2556,6 +2571,7 @@ static const struct tegra_pcie_soc tegra186_pcie = {
>>   	.RAW_violation_fixup = false,
>>   	.program_deskew_time = false,
>>   	.updateFC_threshold = false,
>> +	.has_aspm_l1 = true,
>>   	.has_aspm_l1ss = true,
>>   	.l1ss_rp_wake_fixup = false,
>>   };
>> -- 
>> 2.7.4
>>
Bjorn Helgaas Dec. 14, 2017, 11:46 p.m. | #3
On Thu, Dec 14, 2017 at 10:01:57PM +0530, Vidya Sagar wrote:
> On Tuesday 21 November 2017 03:07 AM, Bjorn Helgaas wrote:
> >s/Enable ASPM-L1 capability advertisement/
> >  /Advertise ASPM L1 PM Substates support/
> This patch controls advertisement of ASPM-L1 capability (not ASPM-L1
> Substates)
> >
> >On Sun, Nov 12, 2017 at 06:47:54PM +0530, Vidya Sagar wrote:
> >>Enables advertisement of ASPM-L1 support in capability
> >>registers of applicable Tegra chips
> >Actually, I'm a little confused about whether this has to do with ASPM
> >L1 support (which would be advertised in the Link Capabilities
> >register) or the ASPM L1 Substates support (which would be advertised
> >in the L1 PM Substates Capabilities register)?
> This is to do with ASPM L1 support (and not ASPM L1 Substates)
> But, since ASPM-L1SS needs ASPM-L1 anyway, I think it is ok to enable
> advertisement of ASPM-L1 in the last patch of the series.

It makes this part of the patch look funny:

> >>@@ -2458,6 +2469,7 @@ static const struct tegra_pcie_soc tegra20_pcie = {
> >>  	.RAW_violation_fixup = false,
> >>  	.program_deskew_time = false,
> >>  	.updateFC_threshold = false,

> >>+	.has_aspm_l1 = false,
> >>  	.has_aspm_l1ss = false,
> >>  	.l1ss_rp_wake_fixup = false,
> >>  };

because in PCIe terms, you have to have ASPM before you can have L1SS.
But the code had .has_aspm_l1ss before you add .has_aspm_l1.

It would make more sense if you could:

  1) make ASPM work, *then*
  2) incrementally add ASPM L1SS

But maybe that's not practical, e.g., maybe you can't make the
hardware hide the L1SS capability the way you can make it hide ASPM.

It's not that big a deal to me either way.

Bjorn

Patch

diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
index 29ee4bb0b7c6..fb61202ee60f 100644
--- a/drivers/pci/host/pci-tegra.c
+++ b/drivers/pci/host/pci-tegra.c
@@ -250,6 +250,9 @@ 
 #define  RP_VEND_XP_UPDATE_FC_THRESHOLD_MASK	(0xff << 18)
 #define  RP_VEND_XP_UPDATE_FC_THRESHOLD_T210	(0x60 << 18)
 
+#define RP_VEND_XP1	0xf04
+#define RP_VEND_XP1_LINK_PVT_CTL_L1_ASPM_SUPPORT	BIT(21)
+
 #define RP_VEND_CTL0	0xf44
 #define  RP_VEND_CTL0_DSK_RST_PULSE_WIDTH_MASK	(0xf << 12)
 #define  RP_VEND_CTL0_DSK_RST_PULSE_WIDTH	(0x9 << 12)
@@ -351,6 +354,7 @@  struct tegra_pcie_soc {
 	bool RAW_violation_fixup;
 	bool program_deskew_time;
 	bool updateFC_threshold;
+	bool has_aspm_l1;
 	bool has_aspm_l1ss;
 	bool l1ss_rp_wake_fixup;
 };
@@ -2214,6 +2218,13 @@  static void tegra_pcie_enable_rp_features(struct tegra_pcie_port *port)
 	value = readl(port->base + RP_VEND_CTL1);
 	value |= RP_VEND_CTL1_ERPT;
 	writel(value, port->base + RP_VEND_CTL1);
+
+	if (port->pcie->soc->has_aspm_l1) {
+		/* Advertise ASPM-L1 state capability*/
+		value = readl(port->base + RP_VEND_XP1);
+		value |= RP_VEND_XP1_LINK_PVT_CTL_L1_ASPM_SUPPORT;
+		writel(value, port->base + RP_VEND_XP1);
+	}
 }
 
 static void tegra_pcie_apply_sw_fixup(struct tegra_pcie_port *port)
@@ -2458,6 +2469,7 @@  static const struct tegra_pcie_soc tegra20_pcie = {
 	.RAW_violation_fixup = false,
 	.program_deskew_time = false,
 	.updateFC_threshold = false,
+	.has_aspm_l1 = false,
 	.has_aspm_l1ss = false,
 	.l1ss_rp_wake_fixup = false,
 };
@@ -2481,6 +2493,7 @@  static const struct tegra_pcie_soc tegra30_pcie = {
 	.RAW_violation_fixup = false,
 	.program_deskew_time = false,
 	.updateFC_threshold = false,
+	.has_aspm_l1 = true,
 	.has_aspm_l1ss = false,
 	.l1ss_rp_wake_fixup = false,
 };
@@ -2503,6 +2516,7 @@  static const struct tegra_pcie_soc tegra124_pcie = {
 	.RAW_violation_fixup = true,
 	.program_deskew_time = false,
 	.updateFC_threshold = false,
+	.has_aspm_l1 = true,
 	.has_aspm_l1ss = false,
 	.l1ss_rp_wake_fixup = false,
 };
@@ -2533,6 +2547,7 @@  static const struct tegra_pcie_soc tegra210_pcie = {
 	.RAW_violation_fixup = false,
 	.program_deskew_time = true,
 	.updateFC_threshold = true,
+	.has_aspm_l1 = true,
 	.has_aspm_l1ss = true,
 	.l1ss_rp_wake_fixup = true,
 };
@@ -2556,6 +2571,7 @@  static const struct tegra_pcie_soc tegra186_pcie = {
 	.RAW_violation_fixup = false,
 	.program_deskew_time = false,
 	.updateFC_threshold = false,
+	.has_aspm_l1 = true,
 	.has_aspm_l1ss = true,
 	.l1ss_rp_wake_fixup = false,
 };