Re: [PATCH 04/30] PCI: tegra: Add PCIe Gen2 link speed support

[Thierry Reding <thierry.reding@gmail.com>]

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

On Mon, Apr 15, 2019 at 08:17:02PM +0530, Manikanta Maddireddy wrote:
> 
> 
> On 15-Apr-19 4:51 PM, Thierry Reding wrote:
> > On Thu, Apr 11, 2019 at 10:33:29PM +0530, Manikanta Maddireddy wrote:
> >> Tegra124, 132, 210 and 186 support Gen2 link speed. After PCIe link is up
> >> in Gen1, set target link speed as Gen2 and retrain link. Link switches to
> >> Gen2 speed if Gen2 capable end point is connected, else link stays in Gen1.
> >>
> >> Per PCIe 4.0r0.9 sec 7.6.3.7 implementation note, driver need to wait for
> >> PCIe LTSSM to come back from recovery before retraining the link.
> >>
> >> Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
> >> ---
> >>  drivers/pci/controller/pci-tegra.c | 61 ++++++++++++++++++++++++++++++
> >>  1 file changed, 61 insertions(+)
> >>
> >> diff --git a/drivers/pci/controller/pci-tegra.c b/drivers/pci/controller/pci-tegra.c
> >> index a61ce9d475b4..6ccda82735f8 100644
> >> --- a/drivers/pci/controller/pci-tegra.c
> >> +++ b/drivers/pci/controller/pci-tegra.c
> >> @@ -191,6 +191,8 @@
> >>  #define  RP_LINK_CONTROL_STATUS_DL_LINK_ACTIVE	0x20000000
> >>  #define  RP_LINK_CONTROL_STATUS_LINKSTAT_MASK	0x3fff0000
> >>  
> >> +#define RP_LINK_CONTROL_STATUS_2		0x000000b0
> >> +
> >>  #define PADS_CTL_SEL		0x0000009c
> >>  
> >>  #define PADS_CTL		0x000000a0
> >> @@ -2096,6 +2098,62 @@ static void tegra_pcie_apply_pad_settings(struct tegra_pcie *pcie)
> >>  		pads_writel(pcie, soc->pads_refclk_cfg1, PADS_REFCLK_CFG1);
> >>  }
> >>  
> >> +#define LINK_RETRAIN_TIMEOUT 100000
> > This is oddly placed. I think this should go somewhere near the top of
> > the file. We already have PME_ACK_TIMEOUT there.
> >
> > But to be honest, I wouldn't even bother with the #define. This is used
> > exactly twice and is much longer to type than the actual number.
> I will move #define to top of the file. Macro name tells us what this timeout,
> so I will keep the macro intact.
> >
> >> +
> >> +static void tegra_pcie_change_link_speed(struct tegra_pcie *pcie)
> >> +{
> >> +	struct device *dev = pcie->dev;
> >> +	struct tegra_pcie_port *port, *tmp;
> >> +	ktime_t deadline;
> >> +	u32 val;
> > The driver uses u32 value for register values elsewhere. It'd be good to
> > stay consistent with that convention.
> Do you mean "unsigned long"? I observed this discrepancy, in few places u32 is used
> and in some places "unsigned long" is used to store register value. I am continuing
> to u32 and we need a new patch to change all "unsigned long" variables to u32
> which are used to store register values.

I meant to say that we spell out "value" everywhere else and don't use
the abbreviation.

Thierry

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