[V3,12/12] PCI: tegra: Update flow control threshold in Tegra210

Message ID 1509371843-22931-13-git-send-email-mmaddireddy@nvidia.com
State Needs Review / ACK
Headers show
Series
  • Enable Tegra root port features and apply SW fixups
Related show

Commit Message

Manikanta Maddireddy Oct. 30, 2017, 1:57 p.m.
Recommended update FC threshold in Tegra210 is 0x60 for best performance
of x1 link. Setting this to 0x60 provides the best balance between number
of UpdateFC and read data sent over the link.

Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
---
V3:
* changed soc parameter name
V2:
* no change in this patch

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

Comments

Lorenzo Pieralisi Dec. 12, 2017, 5:43 p.m. | #1
On Mon, Oct 30, 2017 at 07:27:23PM +0530, Manikanta Maddireddy wrote:
> Recommended update FC threshold in Tegra210 is 0x60 for best performance
> of x1 link. Setting this to 0x60 provides the best balance between number
> of UpdateFC and read data sent over the link.
> 
> Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
> ---
> V3:
> * changed soc parameter name
> V2:
> * no change in this patch
> 
>  drivers/pci/host/pci-tegra.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
> index b29329226e3d..812d32cfdd0e 100644
> --- a/drivers/pci/host/pci-tegra.c
> +++ b/drivers/pci/host/pci-tegra.c
> @@ -223,6 +223,7 @@
>  #define  RP_VEND_XP_OPPORTUNISTIC_ACK		(1 << 27)
>  #define  RP_VEND_XP_OPPORTUNISTIC_UPDATEFC	(1 << 28)
>  #define  RP_VEND_XP_UPDATE_FC_THRESHOLD_MASK	(0xff << 18)
> +#define  RP_VEND_XP_UPDATE_FC_THRESHOLD_T210	(0x60 << 18)

You define a SOC specific threshold and a update_fc_threshold bool
variable to update it ? And what are you going to do if that's needed
on something that it is not a T210 ? Should not this be a(nother)
struct tegra_pcie_soc parameter instead than a macro ?

Not that I am happy about it but this deviates from the current
approach.

>  #define RP_VEND_CTL0	0xf44
>  #define  RP_VEND_CTL0_DSK_RST_PULSE_WIDTH_MASK	(0xf << 12)
> @@ -323,6 +324,7 @@ struct tegra_pcie_soc {
>  	bool update_clamp_threshold;
>  	bool raw_violation_fixup;
>  	bool program_deskew_time;
> +	bool update_fc_threshold;
>  };
>  
>  static inline struct tegra_msi *to_tegra_msi(struct msi_controller *chip)
> @@ -2231,6 +2233,13 @@ static void tegra_pcie_apply_sw_fixup(struct tegra_pcie_port *port)
>  		value |= RP_VEND_CTL0_DSK_RST_PULSE_WIDTH;
>  		writel(value, port->base + RP_VEND_CTL0);
>  	}
> +
> +	if (soc->update_fc_threshold) {
> +		value = readl(port->base + RP_VEND_XP);
> +		value &= ~RP_VEND_XP_UPDATE_FC_THRESHOLD_MASK;
> +		value |= RP_VEND_XP_UPDATE_FC_THRESHOLD_T210;
> +		writel(value, port->base + RP_VEND_XP);
> +	}

If, say, a platform requires update_fc_threshold and raw_violation_fixup
what takes precedence (ie they required programming the _same_
registers) ? update_fc_threshold takes precedence, since it is applied
last - but I would like you to think about this and realize that this
per-SoC mechanism does not scale anymore.

You should a) enforce some firmware initialization - most of the
parameters in struct tegra_pcie_soc could have been pre-programmed
by FW and b) think about adding some DT properties to handle the PCI
host bridge set-up.

Lorenzo

>  }
>  /*
>   * FIXME: If there are no PCIe cards attached, then calling this function
> @@ -2371,6 +2380,7 @@ static const struct tegra_pcie_soc tegra20_pcie = {
>  	.update_clamp_threshold = false,
>  	.raw_violation_fixup = false,
>  	.program_deskew_time = false,
> +	.update_fc_threshold = false,
>  };
>  
>  static const struct tegra_pcie_soc tegra30_pcie = {
> @@ -2391,6 +2401,7 @@ static const struct tegra_pcie_soc tegra30_pcie = {
>  	.update_clamp_threshold = false,
>  	.raw_violation_fixup = false,
>  	.program_deskew_time = false,
> +	.update_fc_threshold = false,
>  };
>  
>  static const struct tegra_pcie_soc tegra124_pcie = {
> @@ -2410,6 +2421,7 @@ static const struct tegra_pcie_soc tegra124_pcie = {
>  	.update_clamp_threshold = true,
>  	.raw_violation_fixup = true,
>  	.program_deskew_time = false,
> +	.update_fc_threshold = false,
>  };
>  
>  static const struct tegra_pcie_soc tegra210_pcie = {
> @@ -2437,6 +2449,7 @@ static const struct tegra_pcie_soc tegra210_pcie = {
>  	.update_clamp_threshold = true,
>  	.raw_violation_fixup = false,
>  	.program_deskew_time = true,
> +	.update_fc_threshold = true,
>  };
>  
>  static const struct tegra_pcie_soc tegra186_pcie = {
> @@ -2457,6 +2470,7 @@ static const struct tegra_pcie_soc tegra186_pcie = {
>  	.update_clamp_threshold = false,
>  	.raw_violation_fixup = false,
>  	.program_deskew_time = false,
> +	.update_fc_threshold = false,
>  };
>  
>  static const struct of_device_id tegra_pcie_of_match[] = {
> -- 
> 2.1.4
>
Thierry Reding Dec. 14, 2017, 4:13 p.m. | #2
On Tue, Dec 12, 2017 at 05:43:29PM +0000, Lorenzo Pieralisi wrote:
> On Mon, Oct 30, 2017 at 07:27:23PM +0530, Manikanta Maddireddy wrote:
> > Recommended update FC threshold in Tegra210 is 0x60 for best performance
> > of x1 link. Setting this to 0x60 provides the best balance between number
> > of UpdateFC and read data sent over the link.
> > 
> > Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
> > ---
> > V3:
> > * changed soc parameter name
> > V2:
> > * no change in this patch
> > 
> >  drivers/pci/host/pci-tegra.c | 14 ++++++++++++++
> >  1 file changed, 14 insertions(+)
> > 
> > diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
> > index b29329226e3d..812d32cfdd0e 100644
> > --- a/drivers/pci/host/pci-tegra.c
> > +++ b/drivers/pci/host/pci-tegra.c
> > @@ -223,6 +223,7 @@
> >  #define  RP_VEND_XP_OPPORTUNISTIC_ACK		(1 << 27)
> >  #define  RP_VEND_XP_OPPORTUNISTIC_UPDATEFC	(1 << 28)
> >  #define  RP_VEND_XP_UPDATE_FC_THRESHOLD_MASK	(0xff << 18)
> > +#define  RP_VEND_XP_UPDATE_FC_THRESHOLD_T210	(0x60 << 18)
> 
> You define a SOC specific threshold and a update_fc_threshold bool
> variable to update it ? And what are you going to do if that's needed
> on something that it is not a T210 ? Should not this be a(nother)
> struct tegra_pcie_soc parameter instead than a macro ?
> 
> Not that I am happy about it but this deviates from the current
> approach.
> 
> >  #define RP_VEND_CTL0	0xf44
> >  #define  RP_VEND_CTL0_DSK_RST_PULSE_WIDTH_MASK	(0xf << 12)
> > @@ -323,6 +324,7 @@ struct tegra_pcie_soc {
> >  	bool update_clamp_threshold;
> >  	bool raw_violation_fixup;
> >  	bool program_deskew_time;
> > +	bool update_fc_threshold;
> >  };
> >  
> >  static inline struct tegra_msi *to_tegra_msi(struct msi_controller *chip)
> > @@ -2231,6 +2233,13 @@ static void tegra_pcie_apply_sw_fixup(struct tegra_pcie_port *port)
> >  		value |= RP_VEND_CTL0_DSK_RST_PULSE_WIDTH;
> >  		writel(value, port->base + RP_VEND_CTL0);
> >  	}
> > +
> > +	if (soc->update_fc_threshold) {
> > +		value = readl(port->base + RP_VEND_XP);
> > +		value &= ~RP_VEND_XP_UPDATE_FC_THRESHOLD_MASK;
> > +		value |= RP_VEND_XP_UPDATE_FC_THRESHOLD_T210;
> > +		writel(value, port->base + RP_VEND_XP);
> > +	}
> 
> If, say, a platform requires update_fc_threshold and raw_violation_fixup
> what takes precedence (ie they required programming the _same_
> registers) ? update_fc_threshold takes precedence, since it is applied
> last - but I would like you to think about this and realize that this
> per-SoC mechanism does not scale anymore.
> 
> You should a) enforce some firmware initialization - most of the
> parameters in struct tegra_pcie_soc could have been pre-programmed
> by FW and

I don't think that's going to work. We reset the hardware during probe
(and a subsequent patchset will make it possible for this driver to be
a loadable module, which will power down the unit on driver removal or
even runtime PM suspend), so any firmware initialization will be lost
after that point and the driver has to reprogram all the registers.

> b) think about adding some DT properties to handle the PCI host bridge
> set-up.

That isn't going to solve the problem of precedence that you bring up
above. Where the data is coming from doesn't change that the order of
execution needs to be defined in the code.

Also, all of the parameters that we have in the struct tegra_pcie_soc
are SoC specific and therefore implied by the compatible string, which
is also why the current approach was chosen back at the time. The
reasoning was that if the property defines the programming model, then
it will be implied by the compatible string. If we moved that data out
into the device tree, we essentially wind up with the same problem, in
a different location. We'd still need to specify the data on a per-SoC
basis in device tree.

Thierry
Thierry Reding Dec. 14, 2017, 4:14 p.m. | #3
On Mon, Oct 30, 2017 at 07:27:23PM +0530, Manikanta Maddireddy wrote:
> Recommended update FC threshold in Tegra210 is 0x60 for best performance
> of x1 link. Setting this to 0x60 provides the best balance between number
> of UpdateFC and read data sent over the link.
> 
> Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
> ---
> V3:
> * changed soc parameter name
> V2:
> * no change in this patch
> 
>  drivers/pci/host/pci-tegra.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
> index b29329226e3d..812d32cfdd0e 100644
> --- a/drivers/pci/host/pci-tegra.c
> +++ b/drivers/pci/host/pci-tegra.c
> @@ -223,6 +223,7 @@
>  #define  RP_VEND_XP_OPPORTUNISTIC_ACK		(1 << 27)
>  #define  RP_VEND_XP_OPPORTUNISTIC_UPDATEFC	(1 << 28)
>  #define  RP_VEND_XP_UPDATE_FC_THRESHOLD_MASK	(0xff << 18)
> +#define  RP_VEND_XP_UPDATE_FC_THRESHOLD_T210	(0x60 << 18)
>  
>  #define RP_VEND_CTL0	0xf44
>  #define  RP_VEND_CTL0_DSK_RST_PULSE_WIDTH_MASK	(0xf << 12)
> @@ -323,6 +324,7 @@ struct tegra_pcie_soc {
>  	bool update_clamp_threshold;
>  	bool raw_violation_fixup;
>  	bool program_deskew_time;
> +	bool update_fc_threshold;
>  };

I agree with Lorenzo here that the threshold value should be a separate
field in struct tegra_pcie_soc.

Thierry

Patch

diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
index b29329226e3d..812d32cfdd0e 100644
--- a/drivers/pci/host/pci-tegra.c
+++ b/drivers/pci/host/pci-tegra.c
@@ -223,6 +223,7 @@ 
 #define  RP_VEND_XP_OPPORTUNISTIC_ACK		(1 << 27)
 #define  RP_VEND_XP_OPPORTUNISTIC_UPDATEFC	(1 << 28)
 #define  RP_VEND_XP_UPDATE_FC_THRESHOLD_MASK	(0xff << 18)
+#define  RP_VEND_XP_UPDATE_FC_THRESHOLD_T210	(0x60 << 18)
 
 #define RP_VEND_CTL0	0xf44
 #define  RP_VEND_CTL0_DSK_RST_PULSE_WIDTH_MASK	(0xf << 12)
@@ -323,6 +324,7 @@  struct tegra_pcie_soc {
 	bool update_clamp_threshold;
 	bool raw_violation_fixup;
 	bool program_deskew_time;
+	bool update_fc_threshold;
 };
 
 static inline struct tegra_msi *to_tegra_msi(struct msi_controller *chip)
@@ -2231,6 +2233,13 @@  static void tegra_pcie_apply_sw_fixup(struct tegra_pcie_port *port)
 		value |= RP_VEND_CTL0_DSK_RST_PULSE_WIDTH;
 		writel(value, port->base + RP_VEND_CTL0);
 	}
+
+	if (soc->update_fc_threshold) {
+		value = readl(port->base + RP_VEND_XP);
+		value &= ~RP_VEND_XP_UPDATE_FC_THRESHOLD_MASK;
+		value |= RP_VEND_XP_UPDATE_FC_THRESHOLD_T210;
+		writel(value, port->base + RP_VEND_XP);
+	}
 }
 /*
  * FIXME: If there are no PCIe cards attached, then calling this function
@@ -2371,6 +2380,7 @@  static const struct tegra_pcie_soc tegra20_pcie = {
 	.update_clamp_threshold = false,
 	.raw_violation_fixup = false,
 	.program_deskew_time = false,
+	.update_fc_threshold = false,
 };
 
 static const struct tegra_pcie_soc tegra30_pcie = {
@@ -2391,6 +2401,7 @@  static const struct tegra_pcie_soc tegra30_pcie = {
 	.update_clamp_threshold = false,
 	.raw_violation_fixup = false,
 	.program_deskew_time = false,
+	.update_fc_threshold = false,
 };
 
 static const struct tegra_pcie_soc tegra124_pcie = {
@@ -2410,6 +2421,7 @@  static const struct tegra_pcie_soc tegra124_pcie = {
 	.update_clamp_threshold = true,
 	.raw_violation_fixup = true,
 	.program_deskew_time = false,
+	.update_fc_threshold = false,
 };
 
 static const struct tegra_pcie_soc tegra210_pcie = {
@@ -2437,6 +2449,7 @@  static const struct tegra_pcie_soc tegra210_pcie = {
 	.update_clamp_threshold = true,
 	.raw_violation_fixup = false,
 	.program_deskew_time = true,
+	.update_fc_threshold = true,
 };
 
 static const struct tegra_pcie_soc tegra186_pcie = {
@@ -2457,6 +2470,7 @@  static const struct tegra_pcie_soc tegra186_pcie = {
 	.update_clamp_threshold = false,
 	.raw_violation_fixup = false,
 	.program_deskew_time = false,
+	.update_fc_threshold = false,
 };
 
 static const struct of_device_id tegra_pcie_of_match[] = {