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

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

[Siddharth Vadapalli]

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.

Fixes: 4740b969aaf5 ("PCI: cadence: Retrain Link to work around Gen2 training defect")
Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
Reviewed-by: Vignesh Raghavendra <vigneshr@ti.com>
---

Hello,

This patch is based on linux-next tagged next-20230606.

v2:
https://lore.kernel.org/r/20230315070800.1615527-1-s-vadapalli@ti.com/
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@ti.com/
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");
+	} else {
+		dev_err(dev, "Fatal! Link training incomplete\n");
+		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

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

[Lorenzo Pieralisi]

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.
> 
> Fixes: 4740b969aaf5 ("PCI: cadence: Retrain Link to work around Gen2 training defect")
> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
> Reviewed-by: Vignesh Raghavendra <vigneshr@ti.com>
> ---
> 
> Hello,
> 
> This patch is based on linux-next tagged next-20230606.
> 
> v2:
> https://lore.kernel.org/r/20230315070800.1615527-1-s-vadapalli@ti.com/
> 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@ti.com/
> 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;

You can use a bool variable eg link_trained and use that below.

> +		usleep_range(0, 1000);
> +	} while (time_before(jiffies, end_jiffies));
> +
> +	if (!(link_status & PCI_EXP_LNKSTA_LT)) {
> +		dev_info(dev, "Link training complete\n");
> +	} else {
> +		dev_err(dev, "Fatal! Link training incomplete\n");
> +		return -ETIMEDOUT;
> +	}

I don't necessarily see the reason why you are adding additional
logging, more so given that this now does not affect just the
workaround but all cadence controllers.

Actually, is that something you have tested and considered ?

Thanks,
Lorenzo

> +
>  	/* 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
> 

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

[Siddharth Vadapalli]

Hello Lorenzo,

Thank you for reviewing this patch.

On 07/06/23 15:53, Lorenzo Pieralisi 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.
>>
>> Fixes: 4740b969aaf5 ("PCI: cadence: Retrain Link to work around Gen2 training defect")
>> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
>> Reviewed-by: Vignesh Raghavendra <vigneshr@ti.com>
>> ---
>>
>> Hello,
>>
>> This patch is based on linux-next tagged next-20230606.
>>
>> v2:
>> https://lore.kernel.org/r/20230315070800.1615527-1-s-vadapalli@ti.com/
>> 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@ti.com/
>> 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;
> 
> You can use a bool variable eg link_trained and use that below.

Sure, I will do that. link_trained = !(link_status & PCI_EXP_LNKSTA_LT); within
the do-while loop and checking for it to be true in the loop as well as below.

> 
>> +		usleep_range(0, 1000);
>> +	} while (time_before(jiffies, end_jiffies));
>> +
>> +	if (!(link_status & PCI_EXP_LNKSTA_LT)) {
>> +		dev_info(dev, "Link training complete\n");
>> +	} else {
>> +		dev_err(dev, "Fatal! Link training incomplete\n");
>> +		return -ETIMEDOUT;
>> +	}
> 
> I don't necessarily see the reason why you are adding additional
> logging, more so given that this now does not affect just the
> workaround but all cadence controllers.
> 
> Actually, is that something you have tested and considered ?

While I agree that I could have performed the entire Link Training check only if
the Gen2 Link Retraining Quirk is set for the RC, considering that the
completion of the Link Training is a necessity irrespective of whether or not
the Quirk exists, I preferred to add the check unconditionally. I would like to
point out that the race condition responsible for the crash is the following:
Without the completion of the Physical Layer link training, the call to the
cdns_pci_map_bus() function in order to access the End Point's registers (if an
EP device is connected) results in the crash. This is primarily observed only on
RT Linux where the software call to cdns_pci_map_bus() by PCI subsystem occurs
quite fast, before the Physical Layer link training is complete. For this
reason, irrespective of whether the Physical Layer link training occurs only
once because of the default flow or occurs a second time due to the Gen2 Link
Retraining Quirk, it appears to me that the crash could potentially occur in
both cases if we don't wait for the Physical Layer link training to complete.

Please let me know if this sounds acceptable. If not, I will check if the quirk
is set before proceeding to verify link training completion and implement this
in the v4 patch.

> 
> Thanks,
> Lorenzo
> 
>> +
>>  	/* 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.

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

[Lorenzo Pieralisi]

On Thu, Jun 08, 2023 at 09:31:59AM +0530, Siddharth Vadapalli wrote:
> Hello Lorenzo,
> 
> Thank you for reviewing this patch.
> 
> On 07/06/23 15:53, Lorenzo Pieralisi 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.
> >>
> >> Fixes: 4740b969aaf5 ("PCI: cadence: Retrain Link to work around Gen2 training defect")
> >> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
> >> Reviewed-by: Vignesh Raghavendra <vigneshr@ti.com>
> >> ---
> >>
> >> Hello,
> >>
> >> This patch is based on linux-next tagged next-20230606.
> >>
> >> v2:
> >> https://lore.kernel.org/r/20230315070800.1615527-1-s-vadapalli@ti.com/
> >> 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@ti.com/
> >> 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;
> > 
> > You can use a bool variable eg link_trained and use that below.
> 
> Sure, I will do that. link_trained = !(link_status & PCI_EXP_LNKSTA_LT); within
> the do-while loop and checking for it to be true in the loop as well as below.
> 
> > 
> >> +		usleep_range(0, 1000);
> >> +	} while (time_before(jiffies, end_jiffies));
> >> +
> >> +	if (!(link_status & PCI_EXP_LNKSTA_LT)) {
> >> +		dev_info(dev, "Link training complete\n");
> >> +	} else {
> >> +		dev_err(dev, "Fatal! Link training incomplete\n");
> >> +		return -ETIMEDOUT;
> >> +	}
> > 
> > I don't necessarily see the reason why you are adding additional
> > logging, more so given that this now does not affect just the
> > workaround but all cadence controllers.
> > 
> > Actually, is that something you have tested and considered ?
> 
> While I agree that I could have performed the entire Link Training check only if
> the Gen2 Link Retraining Quirk is set for the RC, considering that the
> completion of the Link Training is a necessity irrespective of whether or not
> the Quirk exists, I preferred to add the check unconditionally. I would like to
> point out that the race condition responsible for the crash is the following:
> Without the completion of the Physical Layer link training, the call to the
> cdns_pci_map_bus() function in order to access the End Point's registers (if an
> EP device is connected) results in the crash. This is primarily observed only on
> RT Linux where the software call to cdns_pci_map_bus() by PCI subsystem occurs
> quite fast, before the Physical Layer link training is complete. For this
> reason, irrespective of whether the Physical Layer link training occurs only
> once because of the default flow or occurs a second time due to the Gen2 Link
> Retraining Quirk, it appears to me that the crash could potentially occur in
> both cases if we don't wait for the Physical Layer link training to complete.

This means that you are fixing the controller code for all Cadence platforms
and current code is broken (I am not really sure what you mean wrt RT Linux
- please elaborate, this does not sound good to me at all), so the
commit log must be rewritten.

> Please let me know if this sounds acceptable. If not, I will check if the quirk
> is set before proceeding to verify link training completion and implement this
> in the v4 patch.

Not only this is acceptable but that's what you should do. Current code
relies on assumptions (that you will have to explain to me please) wrt
link training and that's not OK.

Lorenzo

> 
> > 
> > Thanks,
> > Lorenzo
> > 
> >> +
> >>  	/* 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.

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

[Siddharth Vadapalli]

On 08/06/23 15:14, Lorenzo Pieralisi wrote:
> On Thu, Jun 08, 2023 at 09:31:59AM +0530, Siddharth Vadapalli wrote:
>> Hello Lorenzo,
>>
>> Thank you for reviewing this patch.
>>
>> On 07/06/23 15:53, Lorenzo Pieralisi 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.
>>>>
>>>> Fixes: 4740b969aaf5 ("PCI: cadence: Retrain Link to work around Gen2 training defect")
>>>> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
>>>> Reviewed-by: Vignesh Raghavendra <vigneshr@ti.com>
>>>> ---
>>>>
>>>> Hello,
>>>>
>>>> This patch is based on linux-next tagged next-20230606.
>>>>
>>>> v2:
>>>> https://lore.kernel.org/r/20230315070800.1615527-1-s-vadapalli@ti.com/
>>>> 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@ti.com/
>>>> 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;
>>>
>>> You can use a bool variable eg link_trained and use that below.
>>
>> Sure, I will do that. link_trained = !(link_status & PCI_EXP_LNKSTA_LT); within
>> the do-while loop and checking for it to be true in the loop as well as below.
>>
>>>
>>>> +		usleep_range(0, 1000);
>>>> +	} while (time_before(jiffies, end_jiffies));
>>>> +
>>>> +	if (!(link_status & PCI_EXP_LNKSTA_LT)) {
>>>> +		dev_info(dev, "Link training complete\n");
>>>> +	} else {
>>>> +		dev_err(dev, "Fatal! Link training incomplete\n");
>>>> +		return -ETIMEDOUT;
>>>> +	}
>>>
>>> I don't necessarily see the reason why you are adding additional
>>> logging, more so given that this now does not affect just the
>>> workaround but all cadence controllers.
>>>
>>> Actually, is that something you have tested and considered ?
>>
>> While I agree that I could have performed the entire Link Training check only if
>> the Gen2 Link Retraining Quirk is set for the RC, considering that the
>> completion of the Link Training is a necessity irrespective of whether or not
>> the Quirk exists, I preferred to add the check unconditionally. I would like to
>> point out that the race condition responsible for the crash is the following:
>> Without the completion of the Physical Layer link training, the call to the
>> cdns_pci_map_bus() function in order to access the End Point's registers (if an
>> EP device is connected) results in the crash. This is primarily observed only on
>> RT Linux where the software call to cdns_pci_map_bus() by PCI subsystem occurs
>> quite fast, before the Physical Layer link training is complete. For this
>> reason, irrespective of whether the Physical Layer link training occurs only
>> once because of the default flow or occurs a second time due to the Gen2 Link
>> Retraining Quirk, it appears to me that the crash could potentially occur in
>> both cases if we don't wait for the Physical Layer link training to complete.
> 
> This means that you are fixing the controller code for all Cadence platforms
> and current code is broken (I am not really sure what you mean wrt RT Linux
> - please elaborate, this does not sound good to me at all), so the
> commit log must be rewritten.

By RT Linux, I am referring to Linux with Real Time configs enabled such as
PREEMPT_RT, which results in faster code execution and lower latency.
All along, it appears to be the case that with the Link Training initiated in
PCIe controller hardware, the driver proceeds to finish the probe and therefore
allow the PCI core subsystem to perform enumeration. However, as it turns out,
the driver appears to have been depending on the latency of other processes
running in the background, due to which the device enumeration process by the
PCI core subsystem got delayed, thereby ensuring that the Link Training process
in the PCIe controller hardware had successfully finished by that point in time.
With RT configs, that latency has been reduced, resulting in the bus enumeration
process starting off much earlier, and occurring while the PCIe controller is
still performing Link Training.

> 
>> Please let me know if this sounds acceptable. If not, I will check if the quirk
>> is set before proceeding to verify link training completion and implement this
>> in the v4 patch.
> 
> Not only this is acceptable but that's what you should do. Current code
> relies on assumptions (that you will have to explain to me please) wrt
> link training and that's not OK.

Please let me know if further clarification is required in addition to my
comment above.

> 
> Lorenzo
> 
>>
>>>
>>> Thanks,
>>> Lorenzo
>>>
>>>> +
>>>>  	/* 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.

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

[Manivannan Sadhasivam]

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().


> Fixes: 4740b969aaf5 ("PCI: cadence: Retrain Link to work around Gen2 training defect")
> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
> Reviewed-by: Vignesh Raghavendra <vigneshr@ti.com>
> ---
> 
> Hello,
> 
> This patch is based on linux-next tagged next-20230606.
> 
> v2:
> https://lore.kernel.org/r/20230315070800.1615527-1-s-vadapalli@ti.com/
> 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@ti.com/
> 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.

> +	} else {
> +		dev_err(dev, "Fatal! Link training incomplete\n");

This could be, "Link retraining incomplete".

- 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
> 

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

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

[Siddharth Vadapalli]

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@ti.com/
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.

> 
> 
>> Fixes: 4740b969aaf5 ("PCI: cadence: Retrain Link to work around Gen2 training defect")
>> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
>> Reviewed-by: Vignesh Raghavendra <vigneshr@ti.com>
>> ---
>>
>> Hello,
>>
>> This patch is based on linux-next tagged next-20230606.
>>
>> v2:
>> https://lore.kernel.org/r/20230315070800.1615527-1-s-vadapalli@ti.com/
>> 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@ti.com/
>> 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.

> 
> - 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.

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

[Manivannan Sadhasivam]

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@ti.com/
> 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?

> > 
> > 
> >> Fixes: 4740b969aaf5 ("PCI: cadence: Retrain Link to work around Gen2 training defect")
> >> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
> >> Reviewed-by: Vignesh Raghavendra <vigneshr@ti.com>
> >> ---
> >>
> >> Hello,
> >>
> >> This patch is based on linux-next tagged next-20230606.
> >>
> >> v2:
> >> https://lore.kernel.org/r/20230315070800.1615527-1-s-vadapalli@ti.com/
> >> 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@ti.com/
> >> 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.

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

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

[Siddharth Vadapalli]

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@ti.com/
>> 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.

> 
>>>
>>>
>>>> Fixes: 4740b969aaf5 ("PCI: cadence: Retrain Link to work around Gen2 training defect")
>>>> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
>>>> Reviewed-by: Vignesh Raghavendra <vigneshr@ti.com>
>>>> ---
>>>>
>>>> Hello,
>>>>
>>>> This patch is based on linux-next tagged next-20230606.
>>>>
>>>> v2:
>>>> https://lore.kernel.org/r/20230315070800.1615527-1-s-vadapalli@ti.com/
>>>> 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@ti.com/
>>>> 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.

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@ti.com/
> >> 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@ti.com>
> >>>> Reviewed-by: Vignesh Raghavendra <vigneshr@ti.com>
> >>>> ---
> >>>>
> >>>> Hello,
> >>>>
> >>>> This patch is based on linux-next tagged next-20230606.
> >>>>
> >>>> v2:
> >>>> https://lore.kernel.org/r/20230315070800.1615527-1-s-vadapalli@ti.com/
> >>>> 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@ti.com/
> >>>> 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.

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

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

[Bjorn Helgaas]

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.
> 
> Fixes: 4740b969aaf5 ("PCI: cadence: Retrain Link to work around Gen2 training defect")
> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
> Reviewed-by: Vignesh Raghavendra <vigneshr@ti.com>
> ---
> 
> Hello,
> 
> This patch is based on linux-next tagged next-20230606.
> 
> v2:
> https://lore.kernel.org/r/20230315070800.1615527-1-s-vadapalli@ti.com/
> 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@ti.com/
> 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");
> +	} else {
> +		dev_err(dev, "Fatal! Link training incomplete\n");
> +		return -ETIMEDOUT;
> +	}

Can I have a brown paper bag, please?  I totally blew it here, and I'm
sorry.

You took my advice by combining this with the existing
cdns_pcie_host_wait_for_link(), but I think my advice was poor because
(a) now this additional wait is not clearly connected with the
erratum, and (b) it affects devices that don't have the erratum.

IIUC, this is all part of a workaround for the i2085 erratum.  The
original workaround, 4740b969aaf5 ("PCI: cadence: Retrain Link to work
around Gen2 training defect"), added this:

  if (!ret && rc->quirk_retrain_flag)
    ret = cdns_pcie_retrain(pcie);

I think the wait for link train to complete should also be in
cdns_pcie_retrain() so it's clearly connected with the quirk, which
also means we'd only do the wait for devices with the erratum.

Which is EXACTLY what your first patch did, and I missed it.  I am
very sorry.  I guess maybe I thought cdns_pcie_retrain() was a
general-purpose thing, but in fact it's only used for this quirk.

Bjorn

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

[Siddharth Vadapalli]

On 09/06/23 23:09, Bjorn Helgaas 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.
>>
>> Fixes: 4740b969aaf5 ("PCI: cadence: Retrain Link to work around Gen2 training defect")
>> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
>> Reviewed-by: Vignesh Raghavendra <vigneshr@ti.com>
>> ---
>>
>> Hello,
>>
>> This patch is based on linux-next tagged next-20230606.
>>
>> v2:
>> https://lore.kernel.org/r/20230315070800.1615527-1-s-vadapalli@ti.com/
>> 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@ti.com/
>> 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");
>> +	} else {
>> +		dev_err(dev, "Fatal! Link training incomplete\n");
>> +		return -ETIMEDOUT;
>> +	}
> 
> Can I have a brown paper bag, please?  I totally blew it here, and I'm
> sorry.
> 
> You took my advice by combining this with the existing
> cdns_pcie_host_wait_for_link(), but I think my advice was poor because
> (a) now this additional wait is not clearly connected with the
> erratum, and (b) it affects devices that don't have the erratum.
> 
> IIUC, this is all part of a workaround for the i2085 erratum.  The
> original workaround, 4740b969aaf5 ("PCI: cadence: Retrain Link to work
> around Gen2 training defect"), added this:
> 
>   if (!ret && rc->quirk_retrain_flag)
>     ret = cdns_pcie_retrain(pcie);
> 
> I think the wait for link train to complete should also be in
> cdns_pcie_retrain() so it's clearly connected with the quirk, which
> also means we'd only do the wait for devices with the erratum.
> 
> Which is EXACTLY what your first patch did, and I missed it.  I am
> very sorry.  I guess maybe I thought cdns_pcie_retrain() was a
> general-purpose thing, but in fact it's only used for this quirk.

With the current approach implemented in this patch, I could do the following:
In the cdns_pcie_host_wait_for_link() function, I obtain the reference to the
struct cdns_pcie_rc *rc, using:
struct cdns_pcie_rc *rc = container_of(pcie, struct cdns_pcie_rc, pcie);
followed by checking if the quirk "quirk_retrain_flag" is set, before proceeding
with the Link Training check added by this patch. With this, only the
controllers with the quirk will check for the Link Training completion before
proceeding. However, the difference with this new approach compared to the
approach in the v2 patch is that in this new approach, even in the Link Training
Phase, the Link Training check is performed for the controllers with the quirk,
unlike the v2 patch where the Link Training check was performed only during the
Link Retraining Phase through the cdns_pcie_retrain() function.

Also, based on Mani's suggestion, I have measured the latency introduced by the
Link Training check for both quirky and non-quirky controllers at:
https://lore.kernel.org/r/a63fc8b0-581b-897f-cac6-cb0a0e82c63e@ti.com/
If the latency is acceptable, then the current implementation in this v3 patch
could be fine too.

Kindly let me know which approach among the following seems to be the best one:
1. The approach implemented in v2 patch (I will make minor changes to the patch
to print out the "Fatal" error, so that users will be informed of the cause of
the crash, followed by posting a v4 patch with this change).
2. The current implementation in the v3 patch with a check added to see if the
controller has the quirk_retrain_flag set, before proceeding with the Link
Training check.
3. The current implementation in the v3 patch as is, without any modification,
if the latency introduced is not a concern and the sanity check for Link
Training completion for non-quirky controllers appears acceptable.

> 
> Bjorn

-- 
Regards,
Siddharth.

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

[Lorenzo Pieralisi]

On Mon, Jun 12, 2023 at 09:56:27AM +0530, Siddharth Vadapalli wrote:
> 
> 
> On 09/06/23 23:09, Bjorn Helgaas 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.
> >>
> >> Fixes: 4740b969aaf5 ("PCI: cadence: Retrain Link to work around Gen2 training defect")
> >> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
> >> Reviewed-by: Vignesh Raghavendra <vigneshr@ti.com>
> >> ---
> >>
> >> Hello,
> >>
> >> This patch is based on linux-next tagged next-20230606.
> >>
> >> v2:
> >> https://lore.kernel.org/r/20230315070800.1615527-1-s-vadapalli@ti.com/
> >> 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@ti.com/
> >> 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");
> >> +	} else {
> >> +		dev_err(dev, "Fatal! Link training incomplete\n");
> >> +		return -ETIMEDOUT;
> >> +	}
> > 
> > Can I have a brown paper bag, please?  I totally blew it here, and I'm
> > sorry.
> > 
> > You took my advice by combining this with the existing
> > cdns_pcie_host_wait_for_link(), but I think my advice was poor because
> > (a) now this additional wait is not clearly connected with the
> > erratum, and (b) it affects devices that don't have the erratum.
> > 
> > IIUC, this is all part of a workaround for the i2085 erratum.  The
> > original workaround, 4740b969aaf5 ("PCI: cadence: Retrain Link to work
> > around Gen2 training defect"), added this:
> > 
> >   if (!ret && rc->quirk_retrain_flag)
> >     ret = cdns_pcie_retrain(pcie);
> > 
> > I think the wait for link train to complete should also be in
> > cdns_pcie_retrain() so it's clearly connected with the quirk, which
> > also means we'd only do the wait for devices with the erratum.
> > 
> > Which is EXACTLY what your first patch did, and I missed it.  I am
> > very sorry.  I guess maybe I thought cdns_pcie_retrain() was a
> > general-purpose thing, but in fact it's only used for this quirk.
> 
> With the current approach implemented in this patch, I could do the following:
> In the cdns_pcie_host_wait_for_link() function, I obtain the reference to the
> struct cdns_pcie_rc *rc, using:
> struct cdns_pcie_rc *rc = container_of(pcie, struct cdns_pcie_rc, pcie);
> followed by checking if the quirk "quirk_retrain_flag" is set, before proceeding
> with the Link Training check added by this patch. With this, only the
> controllers with the quirk will check for the Link Training completion before
> proceeding. However, the difference with this new approach compared to the
> approach in the v2 patch is that in this new approach, even in the Link Training
> Phase, the Link Training check is performed for the controllers with the quirk,
> unlike the v2 patch where the Link Training check was performed only during the
> Link Retraining Phase through the cdns_pcie_retrain() function.
> 
> Also, based on Mani's suggestion, I have measured the latency introduced by the
> Link Training check for both quirky and non-quirky controllers at:
> https://lore.kernel.org/r/a63fc8b0-581b-897f-cac6-cb0a0e82c63e@ti.com/
> If the latency is acceptable, then the current implementation in this v3 patch
> could be fine too.
> 
> Kindly let me know which approach among the following seems to be the best one:
> 1. The approach implemented in v2 patch (I will make minor changes to the patch
> to print out the "Fatal" error, so that users will be informed of the cause of
> the crash, followed by posting a v4 patch with this change).
> 2. The current implementation in the v3 patch with a check added to see if the
> controller has the quirk_retrain_flag set, before proceeding with the Link
> Training check.
> 3. The current implementation in the v3 patch as is, without any modification,
> if the latency introduced is not a concern and the sanity check for Link
> Training completion for non-quirky controllers appears acceptable.

The point is, you stated it yourself that the non-quirky path is broken
too in its *current* form, I don't think there is any other option on
the table other than (3) (unless we want to rely on probe time timing
to hide the issue; that to me it is not even considerable as an option).

Lorenzo

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

[Siddharth Vadapalli]

On 12/06/23 13:59, Lorenzo Pieralisi wrote:
> On Mon, Jun 12, 2023 at 09:56:27AM +0530, Siddharth Vadapalli wrote:
>>
>>
>> On 09/06/23 23:09, Bjorn Helgaas 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.
>>>>
>>>> Fixes: 4740b969aaf5 ("PCI: cadence: Retrain Link to work around Gen2 training defect")
>>>> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
>>>> Reviewed-by: Vignesh Raghavendra <vigneshr@ti.com>
>>>> ---
>>>>
>>>> Hello,
>>>>
>>>> This patch is based on linux-next tagged next-20230606.
>>>>
>>>> v2:
>>>> https://lore.kernel.org/r/20230315070800.1615527-1-s-vadapalli@ti.com/
>>>> 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@ti.com/
>>>> 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");
>>>> +	} else {
>>>> +		dev_err(dev, "Fatal! Link training incomplete\n");
>>>> +		return -ETIMEDOUT;
>>>> +	}
>>>
>>> Can I have a brown paper bag, please?  I totally blew it here, and I'm
>>> sorry.
>>>
>>> You took my advice by combining this with the existing
>>> cdns_pcie_host_wait_for_link(), but I think my advice was poor because
>>> (a) now this additional wait is not clearly connected with the
>>> erratum, and (b) it affects devices that don't have the erratum.
>>>
>>> IIUC, this is all part of a workaround for the i2085 erratum.  The
>>> original workaround, 4740b969aaf5 ("PCI: cadence: Retrain Link to work
>>> around Gen2 training defect"), added this:
>>>
>>>   if (!ret && rc->quirk_retrain_flag)
>>>     ret = cdns_pcie_retrain(pcie);
>>>
>>> I think the wait for link train to complete should also be in
>>> cdns_pcie_retrain() so it's clearly connected with the quirk, which
>>> also means we'd only do the wait for devices with the erratum.
>>>
>>> Which is EXACTLY what your first patch did, and I missed it.  I am
>>> very sorry.  I guess maybe I thought cdns_pcie_retrain() was a
>>> general-purpose thing, but in fact it's only used for this quirk.
>>
>> With the current approach implemented in this patch, I could do the following:
>> In the cdns_pcie_host_wait_for_link() function, I obtain the reference to the
>> struct cdns_pcie_rc *rc, using:
>> struct cdns_pcie_rc *rc = container_of(pcie, struct cdns_pcie_rc, pcie);
>> followed by checking if the quirk "quirk_retrain_flag" is set, before proceeding
>> with the Link Training check added by this patch. With this, only the
>> controllers with the quirk will check for the Link Training completion before
>> proceeding. However, the difference with this new approach compared to the
>> approach in the v2 patch is that in this new approach, even in the Link Training
>> Phase, the Link Training check is performed for the controllers with the quirk,
>> unlike the v2 patch where the Link Training check was performed only during the
>> Link Retraining Phase through the cdns_pcie_retrain() function.
>>
>> Also, based on Mani's suggestion, I have measured the latency introduced by the
>> Link Training check for both quirky and non-quirky controllers at:
>> https://lore.kernel.org/r/a63fc8b0-581b-897f-cac6-cb0a0e82c63e@ti.com/
>> If the latency is acceptable, then the current implementation in this v3 patch
>> could be fine too.
>>
>> Kindly let me know which approach among the following seems to be the best one:
>> 1. The approach implemented in v2 patch (I will make minor changes to the patch
>> to print out the "Fatal" error, so that users will be informed of the cause of
>> the crash, followed by posting a v4 patch with this change).
>> 2. The current implementation in the v3 patch with a check added to see if the
>> controller has the quirk_retrain_flag set, before proceeding with the Link
>> Training check.
>> 3. The current implementation in the v3 patch as is, without any modification,
>> if the latency introduced is not a concern and the sanity check for Link
>> Training completion for non-quirky controllers appears acceptable.
> 
> The point is, you stated it yourself that the non-quirky path is broken
> too in its *current* form, I don't think there is any other option on
> the table other than (3) (unless we want to rely on probe time timing
> to hide the issue; that to me it is not even considerable as an option).

If option 3 is the best one, should I post another patch implementing your
suggestion of using the boolean variable (link_trained) for storing the link
training status as suggested by you at:
https://lore.kernel.org/r/ZIBanRGGPeFw90NZ@lpieralisi/

Also, I can drop the:
dev_info(dev, "Link training complete\n");
message as suggested by Mani at:
https://lore.kernel.org/r/20230608154206.GI5672@thinkpad/

Please let me know.

> 
> Lorenzo

-- 
Regards,
Siddharth.