Re: [PATCH v3] PCI: cadence: Fix Gen2 Link Retraining process

[Manivannan Sadhasivam]

On Fri, Jun 09, 2023 at 11:59:09AM +0530, Siddharth Vadapalli wrote:
> 
> 
> On 09/06/23 11:27, Manivannan Sadhasivam wrote:
> > On Fri, Jun 09, 2023 at 09:46:20AM +0530, Siddharth Vadapalli wrote:
> >> Hello Mani,
> >>
> >> Thank you for reviewing this patch.
> >>
> >> On 08/06/23 21:12, Manivannan Sadhasivam wrote:
> >>> On Wed, Jun 07, 2023 at 02:44:27PM +0530, Siddharth Vadapalli wrote:
> >>>> The Link Retraining process is initiated to account for the Gen2 defect in
> >>>> the Cadence PCIe controller in J721E SoC. The errata corresponding to this
> >>>> is i2085, documented at:
> >>>> https://www.ti.com/lit/er/sprz455c/sprz455c.pdf
> >>>>
> >>>> The existing workaround implemented for the errata waits for the Data Link
> >>>> initialization to complete and assumes that the link retraining process
> >>>> at the Physical Layer has completed. However, it is possible that the
> >>>> Physical Layer training might be ongoing as indicated by the
> >>>> PCI_EXP_LNKSTA_LT bit in the PCI_EXP_LNKSTA register.
> >>>>
> >>>> Fix the existing workaround, to ensure that the Physical Layer training
> >>>> has also completed, in addition to the Data Link initialization.
> >>>>
> >>>
> >>> cdns_pcie_host_wait_for_link() function is called even for the non-quirky cases
> >>> as well, so does this patch. But if your patch is only targeting the link
> >>> retraining case, you should move the logic to cdns_pcie_retrain().
> >>
> >> In the v2 version of this patch at:
> >> https://lore.kernel.org/r/20230315070800.1615527-1-s-vadapalli@xxxxxx/
> >> I had implemented it as suggested above by you. However, based on the discussion
> >> with Bjorn at:
> >> https://lore.kernel.org/r/20230509182416.GA1259841@bhelgaas/
> >> it was agreed upon that waiting for two things in succession doesn't seem to be
> >> the best way to implement it. Therefore, the cdns_pcie_host_training_complete()
> >> function in the v2 patch is merged into the cdns_pcie_host_wait_for_link()
> >> function in this patch.
> >>
> > 
> > I think Bjorn's point was to make the wait_for_link() behavior same across
> > drivers. While I agree with that, I'd like to know whether adding this wait for
> > all cases (not just during link retraining quirk) adds up any latency or not.
> > 
> > Can you measure that?
> 
> For J7200 SoC which doesn't have the link retraining quirk set, I added prints
> before and after the exact section of newly added code with this patch. The
> output with timestamps for the case where no EP is connected to the board is:
> [    1.350061] j721e-pcie 2910000.pcie: <1> Before link training check
> [    1.356324] j721e-pcie 2910000.pcie: Link training complete
> [    1.361883] j721e-pcie 2910000.pcie: <1> After link training check
> indicating a latency of about 12 milliseconds.
> 
> On the other hand, with an EP device connected, the output is:
> [    1.349822] j721e-pcie 2910000.pcie: <1> Before link training check
> [    1.356083] j721e-pcie 2910000.pcie: Link training complete
> [    1.361641] j721e-pcie 2910000.pcie: <1> After link training check
> again indicating a latency of about 12 milliseconds.
> 
> For the J721e SoC which has the quirk, without an EP device connected, the
> output is:
> [    2.668926] j721e-pcie 2910000.pcie: <1> Before link training check
> [    2.675178] j721e-pcie 2910000.pcie: Link training complete
> [    2.680734] j721e-pcie 2910000.pcie: <1> After link training check
> with the latency again being about 12 milliseconds.
> 
> Now, with the EP device connected to the board with J721e SoC, the output for
> the training phase is:
> [    2.685335] j721e-pcie 2910000.pcie: <1> Before link training check
> [    2.691592] j721e-pcie 2910000.pcie: Link training complete
> [    2.697150] j721e-pcie 2910000.pcie: <1> After link training check
> and for the retraining phase is:
> [    2.807581] j721e-pcie 2910000.pcie: <1> Before link training check
> [    2.831578] j721e-pcie 2910000.pcie: LINK DOWN!
> [    2.831905] j721e-pcie 2910000.pcie: Link training complete
> [    2.841653] j721e-pcie 2910000.pcie: <1> After link training check
> During the training phase, the latency again is about 12 milliseconds, while
> during the retraining phase, the latency is about 34 milliseconds.
> 
> Note, the above latency is measured for the Non-RT Linux kernel, with the
> assumption that the latency will be lower for the RT Linux kernel.
> 
> I suppose this indicates a latency of about 12 milliseconds for the PCIe
> controllers without the Gen2 Link Retraining quirk, while for the PCIe
> controller with the Gen2 Link Retraining quirk, it is a net latency of 12+34
> milliseconds = 46 milliseconds for the training and retraining phases.
> 

Thanks for the detailed measurement. So the latency of 12ms for non-quirky
devices seems fine to me, unless someone really cares about optimizing boot
time.

- Mani

> > 
> >>>
> >>>
> >>>> Fixes: 4740b969aaf5 ("PCI: cadence: Retrain Link to work around Gen2 training defect")
> >>>> Signed-off-by: Siddharth Vadapalli <s-vadapalli@xxxxxx>
> >>>> Reviewed-by: Vignesh Raghavendra <vigneshr@xxxxxx>
> >>>> ---
> >>>>
> >>>> Hello,
> >>>>
> >>>> This patch is based on linux-next tagged next-20230606.
> >>>>
> >>>> v2:
> >>>> https://lore.kernel.org/r/20230315070800.1615527-1-s-vadapalli@xxxxxx/
> >>>> Changes since v2:
> >>>> - Merge the cdns_pcie_host_training_complete() function with the
> >>>>   cdns_pcie_host_wait_for_link() function, as suggested by Bjorn
> >>>>   for the v2 patch.
> >>>> - Add dev_err() to notify when Link Training fails, since this is a
> >>>>   fatal error and proceeding from this point will almost always crash
> >>>>   the kernel.
> >>>>
> >>>> v1:
> >>>> https://lore.kernel.org/r/20230102075656.260333-1-s-vadapalli@xxxxxx/
> >>>> Changes since v1:
> >>>> - Collect Reviewed-by tag from Vignesh Raghavendra.
> >>>> - Rebase on next-20230315.
> >>>>
> >>>> Regards,
> >>>> Siddharth.
> >>>>
> >>>>  .../controller/cadence/pcie-cadence-host.c    | 20 +++++++++++++++++++
> >>>>  1 file changed, 20 insertions(+)
> >>>>
> >>>> diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c b/drivers/pci/controller/cadence/pcie-cadence-host.c
> >>>> index 940c7dd701d6..70a5f581ff4f 100644
> >>>> --- a/drivers/pci/controller/cadence/pcie-cadence-host.c
> >>>> +++ b/drivers/pci/controller/cadence/pcie-cadence-host.c
> >>>> @@ -12,6 +12,8 @@
> >>>>  
> >>>>  #include "pcie-cadence.h"
> >>>>  
> >>>> +#define LINK_RETRAIN_TIMEOUT HZ
> >>>> +
> >>>>  static u64 bar_max_size[] = {
> >>>>  	[RP_BAR0] = _ULL(128 * SZ_2G),
> >>>>  	[RP_BAR1] = SZ_2G,
> >>>> @@ -80,8 +82,26 @@ static struct pci_ops cdns_pcie_host_ops = {
> >>>>  static int cdns_pcie_host_wait_for_link(struct cdns_pcie *pcie)
> >>>>  {
> >>>>  	struct device *dev = pcie->dev;
> >>>> +	unsigned long end_jiffies;
> >>>> +	u16 link_status;
> >>>>  	int retries;
> >>>>  
> >>>> +	/* Wait for link training to complete */
> >>>> +	end_jiffies = jiffies + LINK_RETRAIN_TIMEOUT;
> >>>> +	do {
> >>>> +		link_status = cdns_pcie_rp_readw(pcie, CDNS_PCIE_RP_CAP_OFFSET + PCI_EXP_LNKSTA);
> >>>> +		if (!(link_status & PCI_EXP_LNKSTA_LT))
> >>>> +			break;
> >>>> +		usleep_range(0, 1000);
> >>>> +	} while (time_before(jiffies, end_jiffies));
> >>>> +
> >>>> +	if (!(link_status & PCI_EXP_LNKSTA_LT)) {
> >>>> +		dev_info(dev, "Link training complete\n");
> >>>
> >>> This info is not needed.
> >>
> >> Sure. I will drop it in the v4 patch.
> >>
> >>>
> >>>> +	} else {
> >>>> +		dev_err(dev, "Fatal! Link training incomplete\n");
> >>>
> >>> This could be, "Link retraining incomplete".
> >>
> >> I added the word "Fatal" since Linux is almost always guaranteed to crash if the
> >> link training doesn't complete before the PCI subsystem attempts to enumerate
> >> the EP devices. Therefore, adding the word "Fatal" will help the users identify
> >> what the cause of the crash is, which would otherwise be overlooked, unless the
> >> critical nature of this error is conveyed to the user.
> >>
> > 
> > Ok.
> > 
> > - Mani
> > 
> >>>
> >>> - Mani
> >>>
> >>>> +		return -ETIMEDOUT;
> >>>> +	}
> >>>> +
> >>>>  	/* Check if the link is up or not */
> >>>>  	for (retries = 0; retries < LINK_WAIT_MAX_RETRIES; retries++) {
> >>>>  		if (cdns_pcie_link_up(pcie)) {
> >>>> -- 
> >>>> 2.25.1
> >>>>
> >>>
> >>
> >> -- 
> >> Regards,
> >> Siddharth.
> > 
> 
> -- 
> Regards,
> Siddharth.

-- 
மணிவண்ணன் சதாசிவம்