[PATCH v1] PCI: tegra: Enable root port specific features

[PATCH v1] PCI: tegra: Enable root port specific features

[Vidya Sagar]

Enables root port to advertise its ASPM-L1 capability
 resulting in possible link entry to L1 when an ASPM-L1 capable
 device is connected
Enables per-controller & per-TMS clock clamping by default
Enabling above features result in more power saving

It also avoids PM message truncation by waiting for DLLP to finish
before entering into L1 or L2

Also, it adds helper functions to access root port registers

Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
---
 drivers/pci/host/pci-tegra.c | 54 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 53 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
index 3d43874..b192a47 100644
--- a/drivers/pci/host/pci-tegra.c
+++ b/drivers/pci/host/pci-tegra.c
@@ -237,6 +237,18 @@
 		(0xf  << PADS_REFCLK_CFG_DRVI_SHIFT)     \
 	)
 
+#define NV_PCIE2_RP_VEND_XP1			0x00000F04
+#define NV_PCIE2_RP_VEND_XP_LINK_PVT_CTL_L1_ASPM_SUPPORT	(1 << 21)
+
+#define NV_PCIE2_RP_VEND_XP_BIST		0x00000F4C
+#define PCIE2_RP_VEND_XP_BIST_GOTO_L1_L2_AFTER_DLLP_DONE	(1 << 28)
+
+#define NV_PCIE2_RP_PRIV_MISC			0x00000FE0
+#define PCIE2_RP_PRIV_MISC_CTLR_CLK_CLAMP_THRESHOLD	(0xF << 16)
+#define PCIE2_RP_PRIV_MISC_CTLR_CLK_CLAMP_ENABLE	(1 << 23)
+#define PCIE2_RP_PRIV_MISC_TMS_CLK_CLAMP_THRESHOLD	(0xF << 24)
+#define PCIE2_RP_PRIV_MISC_TMS_CLK_CLAMP_ENABLE		(1 << 31)
+
 struct tegra_msi {
 	struct msi_chip chip;
 	DECLARE_BITMAP(used, INT_PCI_MSI_NR);
@@ -346,6 +358,18 @@ static inline u32 pads_readl(struct tegra_pcie *pcie, unsigned long offset)
 	return readl(pcie->pads + offset);
 }
 
+static inline void rp_writel(struct tegra_pcie_port *port, u32 value,
+							 unsigned long offset)
+{
+	writel(value, offset + port->base);
+}
+
+static inline unsigned int rp_readl(struct tegra_pcie_port *port,
+							unsigned long offset)
+{
+	return readl(offset + port->base);
+}
+
 /*
  * The configuration space mapping on Tegra is somewhat similar to the ECAM
  * defined by PCIe. However it deviates a bit in how the 4 bits for extended
@@ -1859,6 +1883,32 @@ retry:
 	return false;
 }
 
+/* Enable various features of root port */
+static void tegra_pcie_enable_rp_features(struct tegra_pcie_port *port)
+{
+	unsigned int data;
+
+	/* Power mangagement settings */
+	/* Enable clock clamping by default */
+	data = rp_readl(port, NV_PCIE2_RP_PRIV_MISC);
+	data |= PCIE2_RP_PRIV_MISC_CTLR_CLK_CLAMP_THRESHOLD |
+		PCIE2_RP_PRIV_MISC_CTLR_CLK_CLAMP_ENABLE |
+		PCIE2_RP_PRIV_MISC_TMS_CLK_CLAMP_THRESHOLD |
+		PCIE2_RP_PRIV_MISC_TMS_CLK_CLAMP_ENABLE;
+	rp_writel(port, data, NV_PCIE2_RP_PRIV_MISC);
+
+	/* Enable rootport to advertise its ASPM - L1 capability */
+	data = rp_readl(port, NV_PCIE2_RP_VEND_XP1);
+	data |= NV_PCIE2_RP_VEND_XP_LINK_PVT_CTL_L1_ASPM_SUPPORT;
+	rp_writel(port, data, NV_PCIE2_RP_VEND_XP1);
+
+	/* LTSSM : wait for DLLP to finish before entering L1 or L2/L3 */
+	/* to avoid truncating PM messages resulting in receiver errors */
+	data = rp_readl(port, NV_PCIE2_RP_VEND_XP_BIST);
+	data |= PCIE2_RP_VEND_XP_BIST_GOTO_L1_L2_AFTER_DLLP_DONE;
+	rp_writel(port, data, NV_PCIE2_RP_VEND_XP_BIST);
+}
+
 static int tegra_pcie_enable(struct tegra_pcie *pcie)
 {
 	struct tegra_pcie_port *port, *tmp;
@@ -1870,8 +1920,10 @@ static int tegra_pcie_enable(struct tegra_pcie *pcie)
 
 		tegra_pcie_port_enable(port);
 
-		if (tegra_pcie_port_check_link(port))
+		if (tegra_pcie_port_check_link(port)) {
+			tegra_pcie_enable_rp_features(port);
 			continue;
+		}
 
 		dev_info(pcie->dev, "link %u down, ignoring\n", port->index);
 
-- 
1.8.1.5

Re: [PATCH v1] PCI: tegra: Enable root port specific features

[Stephen Warren]

On 10/07/2014 03:27 AM, Vidya Sagar wrote:
> Enables root port to advertise its ASPM-L1 capability
>  resulting in possible link entry to L1 when an ASPM-L1 capable
>  device is connected
> Enables per-controller & per-TMS clock clamping by default
> Enabling above features result in more power saving
> 
> It also avoids PM message truncation by waiting for DLLP to finish
> before entering into L1 or L2
> 
> Also, it adds helper functions to access root port registers

> diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c

> @@ -1870,8 +1920,10 @@ static int tegra_pcie_enable(struct tegra_pcie *pcie)
>  
>  		tegra_pcie_port_enable(port);
>  
> -		if (tegra_pcie_port_check_link(port))
> +		if (tegra_pcie_port_check_link(port)) {
> +			tegra_pcie_enable_rp_features(port);
>  			continue;
> +		}
>  
>  		dev_info(pcie->dev, "link %u down, ignoring\n", port->index);

Wouldn't it be better to have the error case inside the if block; most
error-handling is that way. For example:

if (!tegra_pcie_port_check_link(port)) {
	dev_info(pcie->dev, "link %u down, ignoring\n", port->index);
	tegra_pcie_port_disable(port);
	tegra_pcie_port_free(port);
	continue;
}

tegra_pcie_enable_rp_features(port);
... any other "good case" code (of which there admittedly is none at
present)

Re: [PATCH v1] PCI: tegra: Enable root port specific features

[Thierry Reding]

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

On Tue, Oct 07, 2014 at 08:44:05AM -0700, Stephen Warren wrote:
> On 10/07/2014 03:27 AM, Vidya Sagar wrote:
> > Enables root port to advertise its ASPM-L1 capability
> >  resulting in possible link entry to L1 when an ASPM-L1 capable
> >  device is connected
> > Enables per-controller & per-TMS clock clamping by default
> > Enabling above features result in more power saving
> > 
> > It also avoids PM message truncation by waiting for DLLP to finish
> > before entering into L1 or L2
> > 
> > Also, it adds helper functions to access root port registers
> 
> > diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
> 
> > @@ -1870,8 +1920,10 @@ static int tegra_pcie_enable(struct tegra_pcie *pcie)
> >  
> >  		tegra_pcie_port_enable(port);
> >  
> > -		if (tegra_pcie_port_check_link(port))
> > +		if (tegra_pcie_port_check_link(port)) {
> > +			tegra_pcie_enable_rp_features(port);
> >  			continue;
> > +		}
> >  
> >  		dev_info(pcie->dev, "link %u down, ignoring\n", port->index);
> 
> Wouldn't it be better to have the error case inside the if block; most
> error-handling is that way. For example:
> 
> if (!tegra_pcie_port_check_link(port)) {
> 	dev_info(pcie->dev, "link %u down, ignoring\n", port->index);
> 	tegra_pcie_port_disable(port);
> 	tegra_pcie_port_free(port);
> 	continue;
> }
> 
> tegra_pcie_enable_rp_features(port);
> ... any other "good case" code (of which there admittedly is none at
> present)

Yes, that's a little more idiomatic. I think it could be a separate
cleanup patch, though.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v1] PCI: tegra: Enable root port specific features

[Thierry Reding]

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

On Tue, Oct 07, 2014 at 03:57:58PM +0530, Vidya Sagar wrote:
> Enables root port to advertise its ASPM-L1 capability
>  resulting in possible link entry to L1 when an ASPM-L1 capable
>  device is connected
> Enables per-controller & per-TMS clock clamping by default
> Enabling above features result in more power saving
> 
> It also avoids PM message truncation by waiting for DLLP to finish
> before entering into L1 or L2
> 
> Also, it adds helper functions to access root port registers
> 
> Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
> ---
>  drivers/pci/host/pci-tegra.c | 54 +++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 53 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
> index 3d43874..b192a47 100644
> --- a/drivers/pci/host/pci-tegra.c
> +++ b/drivers/pci/host/pci-tegra.c
> @@ -237,6 +237,18 @@
>  		(0xf  << PADS_REFCLK_CFG_DRVI_SHIFT)     \
>  	)
>  
> +#define NV_PCIE2_RP_VEND_XP1			0x00000F04
> +#define NV_PCIE2_RP_VEND_XP_LINK_PVT_CTL_L1_ASPM_SUPPORT	(1 << 21)
> +
> +#define NV_PCIE2_RP_VEND_XP_BIST		0x00000F4C
> +#define PCIE2_RP_VEND_XP_BIST_GOTO_L1_L2_AFTER_DLLP_DONE	(1 << 28)
> +
> +#define NV_PCIE2_RP_PRIV_MISC			0x00000FE0
> +#define PCIE2_RP_PRIV_MISC_CTLR_CLK_CLAMP_THRESHOLD	(0xF << 16)
> +#define PCIE2_RP_PRIV_MISC_CTLR_CLK_CLAMP_ENABLE	(1 << 23)
> +#define PCIE2_RP_PRIV_MISC_TMS_CLK_CLAMP_THRESHOLD	(0xF << 24)
> +#define PCIE2_RP_PRIV_MISC_TMS_CLK_CLAMP_ENABLE		(1 << 31)

These seem to be part of the same set as the RP_PRIV_* registers already
defined earlier in the file. In fact NV_PCIE2_RP_PRIV_MISC is defined as
RP_PRIV_MISC above already. Please move them there and drop the useless
NV_PCIE2_ and PCIE2_ prefixes.

>  struct tegra_msi {
>  	struct msi_chip chip;
>  	DECLARE_BITMAP(used, INT_PCI_MSI_NR);
> @@ -346,6 +358,18 @@ static inline u32 pads_readl(struct tegra_pcie *pcie, unsigned long offset)
>  	return readl(pcie->pads + offset);
>  }
>  
> +static inline void rp_writel(struct tegra_pcie_port *port, u32 value,
> +							 unsigned long offset)
> +{
> +	writel(value, offset + port->base);
> +}
> +
> +static inline unsigned int rp_readl(struct tegra_pcie_port *port,

readl() returns u32 and so should rp_readl().

> +							unsigned long offset)
> +{
> +	return readl(offset + port->base);
> +}

I don't see much use in these, but if you really want to add them,
please also replace any of the open-coded occurrences of this. Also
please fix the alignment of function arguments on subsequent lines.
They should align with the first argument on the first line.

> +/* Enable various features of root port */

This comment is useless. It's a rewording of the function name. Either
leave it out or describe what kind of features are being enabled. Since
you already do that with comments in the function body, I'd suggest to
just drop this comment out.

> +static void tegra_pcie_enable_rp_features(struct tegra_pcie_port *port)

I think a better name would be tegra_pcie_port_enable_features() so that
the name more accurately reflects what "object" the function operates
on.

> +{
> +	unsigned int data;

Should be u32 and a more common name as used in this driver is "value".

> +
> +	/* Power mangagement settings */

"management". Though I'm not sure why this comment is here. My best
guess is that you want to group features. Comments are terrible for
grouping code. Perhaps since everything in this function is related to
power management a better name would be tegra_pcie_port_enable_pm().
That gives you grouping for free.

> +	/* Enable clock clamping by default */
> +	data = rp_readl(port, NV_PCIE2_RP_PRIV_MISC);
> +	data |= PCIE2_RP_PRIV_MISC_CTLR_CLK_CLAMP_THRESHOLD |
> +		PCIE2_RP_PRIV_MISC_CTLR_CLK_CLAMP_ENABLE |
> +		PCIE2_RP_PRIV_MISC_TMS_CLK_CLAMP_THRESHOLD |
> +		PCIE2_RP_PRIV_MISC_TMS_CLK_CLAMP_ENABLE;
> +	rp_writel(port, data, NV_PCIE2_RP_PRIV_MISC);
> +
> +	/* Enable rootport to advertise its ASPM - L1 capability */

"root port"

> +	data = rp_readl(port, NV_PCIE2_RP_VEND_XP1);
> +	data |= NV_PCIE2_RP_VEND_XP_LINK_PVT_CTL_L1_ASPM_SUPPORT;
> +	rp_writel(port, data, NV_PCIE2_RP_VEND_XP1);
> +
> +	/* LTSSM : wait for DLLP to finish before entering L1 or L2/L3 */
> +	/* to avoid truncating PM messages resulting in receiver errors */

Proper style for block comments is:

	/*
	 * LTSSM: ...
	 * ... errors
	 */

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v1] PCI: tegra: Enable root port specific features

[Thierry Reding]

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

On Wed, Oct 08, 2014 at 09:44:05AM +0200, Thierry Reding wrote:
> On Tue, Oct 07, 2014 at 03:57:58PM +0530, Vidya Sagar wrote:
[...]
> > +static inline void rp_writel(struct tegra_pcie_port *port, u32 value,
> > +							 unsigned long offset)
> > +{
> > +	writel(value, offset + port->base);
> > +}
> > +
> > +static inline unsigned int rp_readl(struct tegra_pcie_port *port,
> 
> readl() returns u32 and so should rp_readl().
> 
> > +							unsigned long offset)
> > +{
> > +	return readl(offset + port->base);
> > +}
> 
> I don't see much use in these, but if you really want to add them,
> please also replace any of the open-coded occurrences of this. Also
> please fix the alignment of function arguments on subsequent lines.
> They should align with the first argument on the first line.

Oh, and perhaps make it a separate (precursory) patch so that this
patch can focus on the PM features.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]