| Message ID | 1509371843-22931-13-git-send-email-mmaddireddy@nvidia.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Lorenzo Pieralisi |
| Headers | show |
| Series | Enable Tegra root port features and apply SW fixups | expand |
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 >
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
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
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[] = {
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(+)