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@xxxxxxxxxx>
> >> ---
> >> 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:
signature.asc
Description: PGP signature
