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

[Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

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

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-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > ---
> > 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]