Advanced
Mailing List Archive

Mailing List Archive

  1. Home >
  2. Linux>
  3. Kernel
[PATCH v2] PCI: cadence: Fix Gen2 Link Retraining process
s-vadapalli at ti
Mar 15, 2023, 12:15 AM
Post #1 of 15 (148 views)
Permalink
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>
---
Changes from v1:
1. Collect Reviewed-by tag from Vignesh Raghavendra.
2. Rebase on next-20230315.

v1:
https://lore.kernel.org/r/20230102075656.260333-1-s-vadapalli@ti.com

.../controller/cadence/pcie-cadence-host.c | 27 +++++++++++++++++++
1 file changed, 27 insertions(+)

diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c b/drivers/pci/controller/cadence/pcie-cadence-host.c
index 940c7dd701d6..5b14f7ee3c79 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,
@@ -77,6 +79,27 @@ static struct pci_ops cdns_pcie_host_ops = {
.write = pci_generic_config_write,
};

+static int cdns_pcie_host_training_complete(struct cdns_pcie *pcie)
+{
+ u32 pcie_cap_off = CDNS_PCIE_RP_CAP_OFFSET;
+ unsigned long end_jiffies;
+ u16 lnk_stat;
+
+ /* Wait for link training to complete. Exit after timeout. */
+ end_jiffies = jiffies + LINK_RETRAIN_TIMEOUT;
+ do {
+ lnk_stat = cdns_pcie_rp_readw(pcie, pcie_cap_off + PCI_EXP_LNKSTA);
+ if (!(lnk_stat & PCI_EXP_LNKSTA_LT))
+ break;
+ usleep_range(0, 1000);
+ } while (time_before(jiffies, end_jiffies));
+
+ if (!(lnk_stat & PCI_EXP_LNKSTA_LT))
+ return 0;
+
+ return -ETIMEDOUT;
+}
+
static int cdns_pcie_host_wait_for_link(struct cdns_pcie *pcie)
{
struct device *dev = pcie->dev;
@@ -118,6 +141,10 @@ static int cdns_pcie_retrain(struct cdns_pcie *pcie)
cdns_pcie_rp_writew(pcie, pcie_cap_off + PCI_EXP_LNKCTL,
lnk_ctl);

+ ret = cdns_pcie_host_training_complete(pcie);
+ if (ret)
+ return ret;
+
ret = cdns_pcie_host_wait_for_link(pcie);
}
return ret;
--
2.25.1
Re: [PATCH v2] PCI: cadence: Fix Gen2 Link Retraining process [ In reply to ]
vigneshr at ti
Mar 29, 2023, 8:15 AM
Post #2 of 15 (148 views)
Permalink
Hi Lorenzo, Bjorn,

On 3/15/2023 12:38 PM, 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>
> ---
> Changes from v1:
> 1. Collect Reviewed-by tag from Vignesh Raghavendra.
> 2. Rebase on next-20230315.
>
> v1:
> https://lore.kernel.org/r/20230102075656.260333-1-s-vadapalli@ti.com
>
> .../controller/cadence/pcie-cadence-host.c | 27 +++++++++++++++++++
> 1 file changed, 27 insertions(+)
>

Wondering do one of you be pulling this patch in? This patch was never
picked for 6.3-rc1 merge cycle... Just want to make sure
pcie-cadence*.c and pci-j721e.c patches have a path to reach pci tree.



[...]

Regards
Vignesh
Re: [PATCH v2] PCI: cadence: Fix Gen2 Link Retraining process [ In reply to ]
helgaas at kernel
Mar 29, 2023, 10:15 AM
Post #3 of 15 (148 views)
Permalink
On Wed, Mar 29, 2023 at 08:11:25PM +0530, Raghavendra, Vignesh wrote:
> Hi Lorenzo, Bjorn,
>
> On 3/15/2023 12:38 PM, 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>
> > ---
> > Changes from v1:
> > 1. Collect Reviewed-by tag from Vignesh Raghavendra.
> > 2. Rebase on next-20230315.
> >
> > v1:
> > https://lore.kernel.org/r/20230102075656.260333-1-s-vadapalli@ti.com
> >
> > .../controller/cadence/pcie-cadence-host.c | 27 +++++++++++++++++++
> > 1 file changed, 27 insertions(+)
>
> Wondering do one of you be pulling this patch in? This patch was never
> picked for 6.3-rc1 merge cycle... Just want to make sure
> pcie-cadence*.c and pci-j721e.c patches have a path to reach pci tree.

Yes, Lorenzo or Krzysztof will likely pick this up. I think Lorenzo
is out of the office this week.

Drive-by comment: the current patch doesn't seem to give any
indication to the user when cdns_pcie_host_training_complete() times
out. Is that timeout potentially of interest to a user? Should there
be a log message there?

Bjorn
Re: [PATCH v2] PCI: cadence: Fix Gen2 Link Retraining process [ In reply to ]
s-vadapalli at ti
Mar 29, 2023, 10:15 PM
Post #4 of 15 (148 views)
Permalink
Hello Bjorn,

On 29/03/23 22:38, Bjorn Helgaas wrote:
> On Wed, Mar 29, 2023 at 08:11:25PM +0530, Raghavendra, Vignesh wrote:
>> Hi Lorenzo, Bjorn,
>>
>> On 3/15/2023 12:38 PM, 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>
>>> ---
>>> Changes from v1:
>>> 1. Collect Reviewed-by tag from Vignesh Raghavendra.
>>> 2. Rebase on next-20230315.
>>>
>>> v1:
>>> https://lore.kernel.org/r/20230102075656.260333-1-s-vadapalli@ti.com
>>>
>>> .../controller/cadence/pcie-cadence-host.c | 27 +++++++++++++++++++
>>> 1 file changed, 27 insertions(+)
>>
>> Wondering do one of you be pulling this patch in? This patch was never
>> picked for 6.3-rc1 merge cycle... Just want to make sure
>> pcie-cadence*.c and pci-j721e.c patches have a path to reach pci tree.
>
> Yes, Lorenzo or Krzysztof will likely pick this up. I think Lorenzo
> is out of the office this week.
>
> Drive-by comment: the current patch doesn't seem to give any
> indication to the user when cdns_pcie_host_training_complete() times
> out. Is that timeout potentially of interest to a user? Should there
> be a log message there?

Thank you for reviewing the patch. The return value of -ETIMEDOUT from the
function cdns_pcie_host_training_complete() added by this patch will be handled
similar to the -ETIMEDOUT from the cdns_pcie_host_wait_for_link() function that
is already present.

If cdns_pcie_host_training_complete() returns -ETIMEDOUT, it is returned to
cdns_pcie_host_start_link() function which is called within
cdns_pcie_host_setup() function. In the cdns_pcie_host_setup() function, there
is already a dev_dbg() print for handling the case where
cdns_pcie_host_wait_for_link() times out. For this reason, I felt that for both
cases, the dev_dbg() print can be used to debug without the need for an extra
log message. Please let me know if that's fine.

Regards,
Siddharth.
Re: [PATCH v2] PCI: cadence: Fix Gen2 Link Retraining process [ In reply to ]
vigneshr at ti
Mar 30, 2023, 2:15 AM
Post #5 of 15 (148 views)
Permalink
Hi Bjorn,

On 29/03/23 22:38, Bjorn Helgaas wrote:
> On Wed, Mar 29, 2023 at 08:11:25PM +0530, Raghavendra, Vignesh wrote:
>> Hi Lorenzo, Bjorn,
>>
>> On 3/15/2023 12:38 PM, 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>
>>> ---
>>> Changes from v1:
>>> 1. Collect Reviewed-by tag from Vignesh Raghavendra.
>>> 2. Rebase on next-20230315.
>>>
>>> v1:
>>> https://lore.kernel.org/r/20230102075656.260333-1-s-vadapalli@ti.com
>>>
>>> .../controller/cadence/pcie-cadence-host.c | 27 +++++++++++++++++++
>>> 1 file changed, 27 insertions(+)
>>
>> Wondering do one of you be pulling this patch in? This patch was never
>> picked for 6.3-rc1 merge cycle... Just want to make sure
>> pcie-cadence*.c and pci-j721e.c patches have a path to reach pci tree.
>
> Yes, Lorenzo or Krzysztof will likely pick this up. I think Lorenzo
> is out of the office this week.
>

Thanks for clearing this up!

> Drive-by comment: the current patch doesn't seem to give any
> indication to the user when cdns_pcie_host_training_complete() times
> out. Is that timeout potentially of interest to a user? Should there
> be a log message there?
>

Siddharth will follow up on this comment as a separate response

--
Regards
Vignesh
Re: [PATCH v2] PCI: cadence: Fix Gen2 Link Retraining process [ In reply to ]
helgaas at kernel
Mar 30, 2023, 10:15 AM
Post #6 of 15 (148 views)
Permalink
On Thu, Mar 30, 2023 at 09:52:06AM +0530, Siddharth Vadapalli wrote:
> Hello Bjorn,
>
> On 29/03/23 22:38, Bjorn Helgaas wrote:
> > On Wed, Mar 29, 2023 at 08:11:25PM +0530, Raghavendra, Vignesh wrote:
> >> Hi Lorenzo, Bjorn,
> >>
> >> On 3/15/2023 12:38 PM, 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>
> >>> ---
> >>> Changes from v1:
> >>> 1. Collect Reviewed-by tag from Vignesh Raghavendra.
> >>> 2. Rebase on next-20230315.
> >>>
> >>> v1:
> >>> https://lore.kernel.org/r/20230102075656.260333-1-s-vadapalli@ti.com
> >>>
> >>> .../controller/cadence/pcie-cadence-host.c | 27 +++++++++++++++++++
> >>> 1 file changed, 27 insertions(+)
> >>
> >> Wondering do one of you be pulling this patch in? This patch was never
> >> picked for 6.3-rc1 merge cycle... Just want to make sure
> >> pcie-cadence*.c and pci-j721e.c patches have a path to reach pci tree.
> >
> > Yes, Lorenzo or Krzysztof will likely pick this up. I think Lorenzo
> > is out of the office this week.
> >
> > Drive-by comment: the current patch doesn't seem to give any
> > indication to the user when cdns_pcie_host_training_complete() times
> > out. Is that timeout potentially of interest to a user? Should there
> > be a log message there?
>
> Thank you for reviewing the patch. The return value of -ETIMEDOUT from the
> function cdns_pcie_host_training_complete() added by this patch will be handled
> similar to the -ETIMEDOUT from the cdns_pcie_host_wait_for_link() function that
> is already present.
>
> If cdns_pcie_host_training_complete() returns -ETIMEDOUT, it is returned to
> cdns_pcie_host_start_link() function which is called within
> cdns_pcie_host_setup() function. In the cdns_pcie_host_setup() function, there
> is already a dev_dbg() print for handling the case where
> cdns_pcie_host_wait_for_link() times out. For this reason, I felt that for both
> cases, the dev_dbg() print can be used to debug without the need for an extra
> log message. Please let me know if that's fine.

Sounds good.

dev_dbg() wouldn't be the right thing if we *expect* the link to come
up, but ISTR that maybe you can't detect device presence directly. If
that's the case, all you can do is try to bring the link up and assume
the slot is empty if it doesn't come up. If the usual reason for the
timeout is that the slot is empty, dev_dbg() should be fine.

Another drive-by comment, no action needed, seems slightly strange to
have two "start_link" functions called one after the other:

cdns_pcie_host_setup
cdns_pcie_start_link
cdns_pcie_host_start_link

I assume both are for the same link, so it's weird to have two
functions for it.

Bjorn
Re: [PATCH v2] PCI: cadence: Fix Gen2 Link Retraining process [ In reply to ]
s-vadapalli at ti
Apr 17, 2023, 10:15 PM
Post #7 of 15 (144 views)
Permalink
Hello,

Can this patch please be merged if there are no concerns?

On 30/03/23 22:32, Bjorn Helgaas wrote:
> On Thu, Mar 30, 2023 at 09:52:06AM +0530, Siddharth Vadapalli wrote:
>> Hello Bjorn,
>>
>> On 29/03/23 22:38, Bjorn Helgaas wrote:
>>> On Wed, Mar 29, 2023 at 08:11:25PM +0530, Raghavendra, Vignesh wrote:
>>>> Hi Lorenzo, Bjorn,
>>>>
>>>> On 3/15/2023 12:38 PM, 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>
>>>>> ---
>>>>> Changes from v1:
>>>>> 1. Collect Reviewed-by tag from Vignesh Raghavendra.
>>>>> 2. Rebase on next-20230315.
>>>>>
>>>>> v1:
>>>>> https://lore.kernel.org/r/20230102075656.260333-1-s-vadapalli@ti.com
>>>>>
>>>>> .../controller/cadence/pcie-cadence-host.c | 27 +++++++++++++++++++
>>>>> 1 file changed, 27 insertions(+)
>>>>
>>>> Wondering do one of you be pulling this patch in? This patch was never
>>>> picked for 6.3-rc1 merge cycle... Just want to make sure
>>>> pcie-cadence*.c and pci-j721e.c patches have a path to reach pci tree.
>>>
>>> Yes, Lorenzo or Krzysztof will likely pick this up. I think Lorenzo
>>> is out of the office this week.
>>>
>>> Drive-by comment: the current patch doesn't seem to give any
>>> indication to the user when cdns_pcie_host_training_complete() times
>>> out. Is that timeout potentially of interest to a user? Should there
>>> be a log message there?
>>
>> Thank you for reviewing the patch. The return value of -ETIMEDOUT from the
>> function cdns_pcie_host_training_complete() added by this patch will be handled
>> similar to the -ETIMEDOUT from the cdns_pcie_host_wait_for_link() function that
>> is already present.
>>
>> If cdns_pcie_host_training_complete() returns -ETIMEDOUT, it is returned to
>> cdns_pcie_host_start_link() function which is called within
>> cdns_pcie_host_setup() function. In the cdns_pcie_host_setup() function, there
>> is already a dev_dbg() print for handling the case where
>> cdns_pcie_host_wait_for_link() times out. For this reason, I felt that for both
>> cases, the dev_dbg() print can be used to debug without the need for an extra
>> log message. Please let me know if that's fine.
>
> Sounds good.
>
> dev_dbg() wouldn't be the right thing if we *expect* the link to come
> up, but ISTR that maybe you can't detect device presence directly. If
> that's the case, all you can do is try to bring the link up and assume
> the slot is empty if it doesn't come up. If the usual reason for the
> timeout is that the slot is empty, dev_dbg() should be fine.
>
> Another drive-by comment, no action needed, seems slightly strange to
> have two "start_link" functions called one after the other:
>
> cdns_pcie_host_setup
> cdns_pcie_start_link
> cdns_pcie_host_start_link
>
> I assume both are for the same link, so it's weird to have two
> functions for it.
>
> Bjorn

--
Regards,
Siddharth.
Re: [PATCH v2] PCI: cadence: Fix Gen2 Link Retraining process [ In reply to ]
lpieralisi at kernel
Apr 21, 2023, 2:15 AM
Post #8 of 15 (143 views)
Permalink
On Thu, Mar 30, 2023 at 12:02:18PM -0500, Bjorn Helgaas wrote:
> On Thu, Mar 30, 2023 at 09:52:06AM +0530, Siddharth Vadapalli wrote:
> > Hello Bjorn,
> >
> > On 29/03/23 22:38, Bjorn Helgaas wrote:
> > > On Wed, Mar 29, 2023 at 08:11:25PM +0530, Raghavendra, Vignesh wrote:
> > >> Hi Lorenzo, Bjorn,
> > >>
> > >> On 3/15/2023 12:38 PM, 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>
> > >>> ---
> > >>> Changes from v1:
> > >>> 1. Collect Reviewed-by tag from Vignesh Raghavendra.
> > >>> 2. Rebase on next-20230315.
> > >>>
> > >>> v1:
> > >>> https://lore.kernel.org/r/20230102075656.260333-1-s-vadapalli@ti.com
> > >>>
> > >>> .../controller/cadence/pcie-cadence-host.c | 27 +++++++++++++++++++
> > >>> 1 file changed, 27 insertions(+)
> > >>
> > >> Wondering do one of you be pulling this patch in? This patch was never
> > >> picked for 6.3-rc1 merge cycle... Just want to make sure
> > >> pcie-cadence*.c and pci-j721e.c patches have a path to reach pci tree.
> > >
> > > Yes, Lorenzo or Krzysztof will likely pick this up. I think Lorenzo
> > > is out of the office this week.
> > >
> > > Drive-by comment: the current patch doesn't seem to give any
> > > indication to the user when cdns_pcie_host_training_complete() times
> > > out. Is that timeout potentially of interest to a user? Should there
> > > be a log message there?
> >
> > Thank you for reviewing the patch. The return value of -ETIMEDOUT from the
> > function cdns_pcie_host_training_complete() added by this patch will be handled
> > similar to the -ETIMEDOUT from the cdns_pcie_host_wait_for_link() function that
> > is already present.
> >
> > If cdns_pcie_host_training_complete() returns -ETIMEDOUT, it is returned to
> > cdns_pcie_host_start_link() function which is called within
> > cdns_pcie_host_setup() function. In the cdns_pcie_host_setup() function, there
> > is already a dev_dbg() print for handling the case where
> > cdns_pcie_host_wait_for_link() times out. For this reason, I felt that for both
> > cases, the dev_dbg() print can be used to debug without the need for an extra
> > log message. Please let me know if that's fine.
>
> Sounds good.
>
> dev_dbg() wouldn't be the right thing if we *expect* the link to come
> up, but ISTR that maybe you can't detect device presence directly. If
> that's the case, all you can do is try to bring the link up and assume
> the slot is empty if it doesn't come up. If the usual reason for the
> timeout is that the slot is empty, dev_dbg() should be fine.
>
> Another drive-by comment, no action needed, seems slightly strange to
> have two "start_link" functions called one after the other:
>
> cdns_pcie_host_setup
> cdns_pcie_start_link
> cdns_pcie_host_start_link
>
> I assume both are for the same link, so it's weird to have two
> functions for it.

Side note: I would advise developers to reply to review comments
before nagging us to merge patches, it is not fine to leave
comments unanswered.

Yes, it is weird, the first call is for the platform driver specific
link HW setup and the second for the host *generic* link set-up and I
agree that's confusing, it is not related to this patch - that I am
merging - but should be addressed nonetheless.

Lorenzo
Re: [PATCH v2] PCI: cadence: Fix Gen2 Link Retraining process [ In reply to ]
lpieralisi at kernel
Apr 21, 2023, 2:15 AM
Post #9 of 15 (143 views)
Permalink
On Wed, 15 Mar 2023 12:38:00 +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.
>
> [...]

Applied to controller/cadence, thanks!

[1/1] PCI: cadence: Fix Gen2 Link Retraining process
https://git.kernel.org/pci/pci/c/eac223e85208

Thanks,
Lorenzo
Re: [PATCH v2] PCI: cadence: Fix Gen2 Link Retraining process [ In reply to ]
helgaas at kernel
May 8, 2023, 2:15 PM
Post #10 of 15 (139 views)
Permalink
On Wed, Mar 15, 2023 at 12:38:00PM +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>
> ---
> Changes from v1:
> 1. Collect Reviewed-by tag from Vignesh Raghavendra.
> 2. Rebase on next-20230315.
>
> v1:
> https://lore.kernel.org/r/20230102075656.260333-1-s-vadapalli@ti.com
>
> .../controller/cadence/pcie-cadence-host.c | 27 +++++++++++++++++++
> 1 file changed, 27 insertions(+)
>
> diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c b/drivers/pci/controller/cadence/pcie-cadence-host.c
> index 940c7dd701d6..5b14f7ee3c79 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,
> @@ -77,6 +79,27 @@ static struct pci_ops cdns_pcie_host_ops = {
> .write = pci_generic_config_write,
> };
>
> +static int cdns_pcie_host_training_complete(struct cdns_pcie *pcie)

This is kind of weird because it's named like a predicate, i.e., "this
function tells me whether link training is complete", but it returns
*zero* for success.

This is the opposite of j721e_pcie_link_up(), which returns "true"
when the link is up, so code like this reads naturally:

if (pcie->ops->link_up(pcie))
/* do something if the link is up */

> +{
> + u32 pcie_cap_off = CDNS_PCIE_RP_CAP_OFFSET;
> + unsigned long end_jiffies;
> + u16 lnk_stat;
> +
> + /* Wait for link training to complete. Exit after timeout. */
> + end_jiffies = jiffies + LINK_RETRAIN_TIMEOUT;
> + do {
> + lnk_stat = cdns_pcie_rp_readw(pcie, pcie_cap_off + PCI_EXP_LNKSTA);
> + if (!(lnk_stat & PCI_EXP_LNKSTA_LT))
> + break;
> + usleep_range(0, 1000);
> + } while (time_before(jiffies, end_jiffies));
> +
> + if (!(lnk_stat & PCI_EXP_LNKSTA_LT))
> + return 0;
> +
> + return -ETIMEDOUT;
> +}
> +
> static int cdns_pcie_host_wait_for_link(struct cdns_pcie *pcie)
> {
> struct device *dev = pcie->dev;
> @@ -118,6 +141,10 @@ static int cdns_pcie_retrain(struct cdns_pcie *pcie)
> cdns_pcie_rp_writew(pcie, pcie_cap_off + PCI_EXP_LNKCTL,
> lnk_ctl);
>
> + ret = cdns_pcie_host_training_complete(pcie);
> + if (ret)
> + return ret;
> +
> ret = cdns_pcie_host_wait_for_link(pcie);

It seems a little clumsy that we wait for two things in succession:

- cdns_pcie_host_training_complete() waits up to 1s for
PCI_EXP_LNKSTA_LT to be cleared

- cdns_pcie_host_wait_for_link() waits between .9s and 1s for
LINK_UP_DL_COMPLETED on j721e (and not at all for other platforms)

dw_pcie_wait_for_link() is basically similar but has a single wait
loop around the dw_pcie_link_up() callback. Several of those
callbacks check multiple things. Can we do the same here?

Is the "host" in the cdns_pcie_host_wait_for_link() name necessary?
Maybe it could be cdns_pcie_wait_for_link() to be similar to
dw_pcie_wait_for_link()? Or, if "host" is necessary, it could be
cdns_host_pcie_wait_for_link() so it matches the same
"pcie_wait_for_link" grep pattern as most of the others?

> }
> return ret;
Re: [PATCH v2] PCI: cadence: Fix Gen2 Link Retraining process [ In reply to ]
s-vadapalli at ti
May 9, 2023, 12:15 AM
Post #11 of 15 (138 views)
Permalink
Bjorn,

Thank you for reviewing the patch.

On 09/05/23 02:44, Bjorn Helgaas wrote:
> On Wed, Mar 15, 2023 at 12:38:00PM +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>
>> ---
>> Changes from v1:
>> 1. Collect Reviewed-by tag from Vignesh Raghavendra.
>> 2. Rebase on next-20230315.
>>
>> v1:
>> https://lore.kernel.org/r/20230102075656.260333-1-s-vadapalli@ti.com
>>
>> .../controller/cadence/pcie-cadence-host.c | 27 +++++++++++++++++++
>> 1 file changed, 27 insertions(+)
>>
>> diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c b/drivers/pci/controller/cadence/pcie-cadence-host.c
>> index 940c7dd701d6..5b14f7ee3c79 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,
>> @@ -77,6 +79,27 @@ static struct pci_ops cdns_pcie_host_ops = {
>> .write = pci_generic_config_write,
>> };
>>
>> +static int cdns_pcie_host_training_complete(struct cdns_pcie *pcie)
>
> This is kind of weird because it's named like a predicate, i.e., "this
> function tells me whether link training is complete", but it returns
> *zero* for success.
>
> This is the opposite of j721e_pcie_link_up(), which returns "true"
> when the link is up, so code like this reads naturally:
>
> if (pcie->ops->link_up(pcie))
> /* do something if the link is up */

I agree. The function name can be changed to indicate that it is waiting for
completion rather than indicating completion. If this is the only change, I will
post a patch to fix it. On the other hand, based on your comments in the next
section, I am thinking of an alternative approach of merging the current
"cdns_pcie_host_training_complete()" function's operation as well into the
"cdns_pcie_host_wait_for_link()" function. If this is acceptable, I will post a
different patch and the name change patch won't be necessary.

>
>> +{
>> + u32 pcie_cap_off = CDNS_PCIE_RP_CAP_OFFSET;
>> + unsigned long end_jiffies;
>> + u16 lnk_stat;
>> +
>> + /* Wait for link training to complete. Exit after timeout. */
>> + end_jiffies = jiffies + LINK_RETRAIN_TIMEOUT;
>> + do {
>> + lnk_stat = cdns_pcie_rp_readw(pcie, pcie_cap_off + PCI_EXP_LNKSTA);
>> + if (!(lnk_stat & PCI_EXP_LNKSTA_LT))
>> + break;
>> + usleep_range(0, 1000);
>> + } while (time_before(jiffies, end_jiffies));
>> +
>> + if (!(lnk_stat & PCI_EXP_LNKSTA_LT))
>> + return 0;
>> +
>> + return -ETIMEDOUT;
>> +}
>> +
>> static int cdns_pcie_host_wait_for_link(struct cdns_pcie *pcie)
>> {
>> struct device *dev = pcie->dev;
>> @@ -118,6 +141,10 @@ static int cdns_pcie_retrain(struct cdns_pcie *pcie)
>> cdns_pcie_rp_writew(pcie, pcie_cap_off + PCI_EXP_LNKCTL,
>> lnk_ctl);
>>
>> + ret = cdns_pcie_host_training_complete(pcie);
>> + if (ret)
>> + return ret;
>> +
>> ret = cdns_pcie_host_wait_for_link(pcie);
>
> It seems a little clumsy that we wait for two things in succession:
>
> - cdns_pcie_host_training_complete() waits up to 1s for
> PCI_EXP_LNKSTA_LT to be cleared
>
> - cdns_pcie_host_wait_for_link() waits between .9s and 1s for
> LINK_UP_DL_COMPLETED on j721e (and not at all for other platforms)

Is it acceptable to merge "cdns_pcie_host_training_complete()" into
"cdns_pcie_host_wait_for_link()"?

>
> dw_pcie_wait_for_link() is basically similar but has a single wait
> loop around the dw_pcie_link_up() callback. Several of those
> callbacks check multiple things. Can we do the same here?

I assume you are referring to merging the functions together?

>
> Is the "host" in the cdns_pcie_host_wait_for_link() name necessary?
> Maybe it could be cdns_pcie_wait_for_link() to be similar to
> dw_pcie_wait_for_link()? Or, if "host" is necessary, it could be
> cdns_host_pcie_wait_for_link() so it matches the same
> "pcie_wait_for_link" grep pattern as most of the others?

If the functions are merged, I believe that the word "host" can be dropped in
the new function which can be named "cdns_pcie_wait_for_link()" as suggested by you.

Please let me know.

>
>> }
>> return ret;

--
Regards,
Siddharth.
Re: [PATCH v2] PCI: cadence: Fix Gen2 Link Retraining process [ In reply to ]
helgaas at kernel
May 9, 2023, 12:15 PM
Post #12 of 15 (136 views)
Permalink
On Tue, May 09, 2023 at 12:37:31PM +0530, Siddharth Vadapalli wrote:
> Bjorn,
>
> Thank you for reviewing the patch.
>
> On 09/05/23 02:44, Bjorn Helgaas wrote:
> > On Wed, Mar 15, 2023 at 12:38:00PM +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>
> >> ---
> >> Changes from v1:
> >> 1. Collect Reviewed-by tag from Vignesh Raghavendra.
> >> 2. Rebase on next-20230315.
> >>
> >> v1:
> >> https://lore.kernel.org/r/20230102075656.260333-1-s-vadapalli@ti.com
> >>
> >> .../controller/cadence/pcie-cadence-host.c | 27 +++++++++++++++++++
> >> 1 file changed, 27 insertions(+)
> >>
> >> diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c b/drivers/pci/controller/cadence/pcie-cadence-host.c
> >> index 940c7dd701d6..5b14f7ee3c79 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,
> >> @@ -77,6 +79,27 @@ static struct pci_ops cdns_pcie_host_ops = {
> >> .write = pci_generic_config_write,
> >> };
> >>
> >> +static int cdns_pcie_host_training_complete(struct cdns_pcie *pcie)
> >
> > This is kind of weird because it's named like a predicate, i.e., "this
> > function tells me whether link training is complete", but it returns
> > *zero* for success.
> >
> > This is the opposite of j721e_pcie_link_up(), which returns "true"
> > when the link is up, so code like this reads naturally:
> >
> > if (pcie->ops->link_up(pcie))
> > /* do something if the link is up */
>
> I agree. The function name can be changed to indicate that it is
> waiting for completion rather than indicating completion. If this is
> the only change, I will post a patch to fix it. On the other hand,
> based on your comments in the next section, I am thinking of an
> alternative approach of merging the current
> "cdns_pcie_host_training_complete()" function's operation as well
> into the "cdns_pcie_host_wait_for_link()" function. If this is
> acceptable, I will post a different patch and the name change patch
> won't be necessary.

Yeah, sorry, I meant to delete this part of my response after I wrote
the one below.

> >> @@ -118,6 +141,10 @@ static int cdns_pcie_retrain(struct cdns_pcie *pcie)
> >> cdns_pcie_rp_writew(pcie, pcie_cap_off + PCI_EXP_LNKCTL,
> >> lnk_ctl);
> >>
> >> + ret = cdns_pcie_host_training_complete(pcie);
> >> + if (ret)
> >> + return ret;
> >> +
> >> ret = cdns_pcie_host_wait_for_link(pcie);
> >
> > It seems a little clumsy that we wait for two things in succession:
> >
> > - cdns_pcie_host_training_complete() waits up to 1s for
> > PCI_EXP_LNKSTA_LT to be cleared
> >
> > - cdns_pcie_host_wait_for_link() waits between .9s and 1s for
> > LINK_UP_DL_COMPLETED on j721e (and not at all for other platforms)
>
> Is it acceptable to merge "cdns_pcie_host_training_complete()" into
> "cdns_pcie_host_wait_for_link()"?

That's what I'm proposing. Maybe someone who is more familiar with
Cadence would have an argument against it, but I think making it
structurally the same as dw_pcie_wait_for_link() would be a good
thing.

Bjorn
Re: [PATCH v2] PCI: cadence: Fix Gen2 Link Retraining process [ In reply to ]
s-vadapalli at ti
May 10, 2023, 8:15 AM
Post #13 of 15 (128 views)
Permalink
On 09-05-2023 23:54, Bjorn Helgaas wrote:
> On Tue, May 09, 2023 at 12:37:31PM +0530, Siddharth Vadapalli wrote:
>> Bjorn,
>>
>> Thank you for reviewing the patch.
>>
>> On 09/05/23 02:44, Bjorn Helgaas wrote:
>>> On Wed, Mar 15, 2023 at 12:38:00PM +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>
>>>> ---
>>>> Changes from v1:
>>>> 1. Collect Reviewed-by tag from Vignesh Raghavendra.
>>>> 2. Rebase on next-20230315.
>>>>
>>>> v1:
>>>> https://lore.kernel.org/r/20230102075656.260333-1-s-vadapalli@ti.com
>>>>
>>>> .../controller/cadence/pcie-cadence-host.c | 27 +++++++++++++++++++
>>>> 1 file changed, 27 insertions(+)
>>>>
>>>> diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c b/drivers/pci/controller/cadence/pcie-cadence-host.c
>>>> index 940c7dd701d6..5b14f7ee3c79 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,
>>>> @@ -77,6 +79,27 @@ static struct pci_ops cdns_pcie_host_ops = {
>>>> .write = pci_generic_config_write,
>>>> };
>>>>
>>>> +static int cdns_pcie_host_training_complete(struct cdns_pcie *pcie)
>>>
>>> This is kind of weird because it's named like a predicate, i.e., "this
>>> function tells me whether link training is complete", but it returns
>>> *zero* for success.
>>>
>>> This is the opposite of j721e_pcie_link_up(), which returns "true"
>>> when the link is up, so code like this reads naturally:
>>>
>>> if (pcie->ops->link_up(pcie))
>>> /* do something if the link is up */
>>
>> I agree. The function name can be changed to indicate that it is
>> waiting for completion rather than indicating completion. If this is
>> the only change, I will post a patch to fix it. On the other hand,
>> based on your comments in the next section, I am thinking of an
>> alternative approach of merging the current
>> "cdns_pcie_host_training_complete()" function's operation as well
>> into the "cdns_pcie_host_wait_for_link()" function. If this is
>> acceptable, I will post a different patch and the name change patch
>> won't be necessary.
>
> Yeah, sorry, I meant to delete this part of my response after I wrote
> the one below.
>
>>>> @@ -118,6 +141,10 @@ static int cdns_pcie_retrain(struct cdns_pcie *pcie)
>>>> cdns_pcie_rp_writew(pcie, pcie_cap_off + PCI_EXP_LNKCTL,
>>>> lnk_ctl);
>>>>
>>>> + ret = cdns_pcie_host_training_complete(pcie);
>>>> + if (ret)
>>>> + return ret;
>>>> +
>>>> ret = cdns_pcie_host_wait_for_link(pcie);
>>>
>>> It seems a little clumsy that we wait for two things in succession:
>>>
>>> - cdns_pcie_host_training_complete() waits up to 1s for
>>> PCI_EXP_LNKSTA_LT to be cleared
>>>
>>> - cdns_pcie_host_wait_for_link() waits between .9s and 1s for
>>> LINK_UP_DL_COMPLETED on j721e (and not at all for other platforms)
>>
>> Is it acceptable to merge "cdns_pcie_host_training_complete()" into
>> "cdns_pcie_host_wait_for_link()"?
>
> That's what I'm proposing. Maybe someone who is more familiar with
> Cadence would have an argument against it, but I think making it
> structurally the same as dw_pcie_wait_for_link() would be a good
> thing.

Thank you for the confirmation. I will work on it and post a patch.

>
> Bjorn

--
Regards,
Siddharth.
Re: [PATCH v2] PCI: cadence: Fix Gen2 Link Retraining process [ In reply to ]
helgaas at kernel
Jun 6, 2023, 10:15 AM
Post #14 of 15 (126 views)
Permalink
On Wed, May 10, 2023 at 06:47:46PM +0530, Siddharth Vadapalli wrote:
> On 09-05-2023 23:54, Bjorn Helgaas wrote:
> > On Tue, May 09, 2023 at 12:37:31PM +0530, Siddharth Vadapalli wrote:
> >> On 09/05/23 02:44, Bjorn Helgaas wrote:
> >>> On Wed, Mar 15, 2023 at 12:38:00PM +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>
> >>>> ---
> >>>> Changes from v1:
> >>>> 1. Collect Reviewed-by tag from Vignesh Raghavendra.
> >>>> 2. Rebase on next-20230315.
> >>>>
> >>>> v1:
> >>>> https://lore.kernel.org/r/20230102075656.260333-1-s-vadapalli@ti.com
> >>>>
> >>>> .../controller/cadence/pcie-cadence-host.c | 27 +++++++++++++++++++
> >>>> 1 file changed, 27 insertions(+)
> >>>>
> >>>> diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c b/drivers/pci/controller/cadence/pcie-cadence-host.c
> >>>> index 940c7dd701d6..5b14f7ee3c79 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,
> >>>> @@ -77,6 +79,27 @@ static struct pci_ops cdns_pcie_host_ops = {
> >>>> .write = pci_generic_config_write,
> >>>> };
> >>>>
> >>>> +static int cdns_pcie_host_training_complete(struct cdns_pcie *pcie)
> >>>
> >>> This is kind of weird because it's named like a predicate, i.e., "this
> >>> function tells me whether link training is complete", but it returns
> >>> *zero* for success.
> >>>
> >>> This is the opposite of j721e_pcie_link_up(), which returns "true"
> >>> when the link is up, so code like this reads naturally:
> >>>
> >>> if (pcie->ops->link_up(pcie))
> >>> /* do something if the link is up */
> >>
> >> I agree. The function name can be changed to indicate that it is
> >> waiting for completion rather than indicating completion. If this is
> >> the only change, I will post a patch to fix it. On the other hand,
> >> based on your comments in the next section, I am thinking of an
> >> alternative approach of merging the current
> >> "cdns_pcie_host_training_complete()" function's operation as well
> >> into the "cdns_pcie_host_wait_for_link()" function. If this is
> >> acceptable, I will post a different patch and the name change patch
> >> won't be necessary.
> >
> > Yeah, sorry, I meant to delete this part of my response after I wrote
> > the one below.
> >
> >>>> @@ -118,6 +141,10 @@ static int cdns_pcie_retrain(struct cdns_pcie *pcie)
> >>>> cdns_pcie_rp_writew(pcie, pcie_cap_off + PCI_EXP_LNKCTL,
> >>>> lnk_ctl);
> >>>>
> >>>> + ret = cdns_pcie_host_training_complete(pcie);
> >>>> + if (ret)
> >>>> + return ret;
> >>>> +
> >>>> ret = cdns_pcie_host_wait_for_link(pcie);
> >>>
> >>> It seems a little clumsy that we wait for two things in succession:
> >>>
> >>> - cdns_pcie_host_training_complete() waits up to 1s for
> >>> PCI_EXP_LNKSTA_LT to be cleared
> >>>
> >>> - cdns_pcie_host_wait_for_link() waits between .9s and 1s for
> >>> LINK_UP_DL_COMPLETED on j721e (and not at all for other platforms)
> >>
> >> Is it acceptable to merge "cdns_pcie_host_training_complete()" into
> >> "cdns_pcie_host_wait_for_link()"?
> >
> > That's what I'm proposing. Maybe someone who is more familiar with
> > Cadence would have an argument against it, but I think making it
> > structurally the same as dw_pcie_wait_for_link() would be a good
> > thing.
>
> Thank you for the confirmation. I will work on it and post a patch.

Ping, do you still plan to do this? Lorenzo currently has the
v2 patch on his pci/controller/cadence branch [1], but I haven't
merged it into -next yet on the assumption that a new version is
coming.

Bjorn

[1] https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/commit/?h=controller/cadence&id=0e12f8302369
Re: [PATCH v2] PCI: cadence: Fix Gen2 Link Retraining process [ In reply to ]
s-vadapalli at ti
Jun 7, 2023, 4:15 AM
Post #15 of 15 (126 views)
Permalink
Hello Bjorn,

On 06/06/23 21:50, Bjorn Helgaas wrote:
> On Wed, May 10, 2023 at 06:47:46PM +0530, Siddharth Vadapalli wrote:
>> On 09-05-2023 23:54, Bjorn Helgaas wrote:
>>> On Tue, May 09, 2023 at 12:37:31PM +0530, Siddharth Vadapalli wrote:
>>>> On 09/05/23 02:44, Bjorn Helgaas wrote:
>>>>> On Wed, Mar 15, 2023 at 12:38:00PM +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>
>>>>>> ---
>>>>>> Changes from v1:
>>>>>> 1. Collect Reviewed-by tag from Vignesh Raghavendra.
>>>>>> 2. Rebase on next-20230315.
>>>>>>
>>>>>> v1:
>>>>>> https://lore.kernel.org/r/20230102075656.260333-1-s-vadapalli@ti.com
>>>>>>
>>>>>> .../controller/cadence/pcie-cadence-host.c | 27 +++++++++++++++++++
>>>>>> 1 file changed, 27 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c b/drivers/pci/controller/cadence/pcie-cadence-host.c
>>>>>> index 940c7dd701d6..5b14f7ee3c79 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,
>>>>>> @@ -77,6 +79,27 @@ static struct pci_ops cdns_pcie_host_ops = {
>>>>>> .write = pci_generic_config_write,
>>>>>> };
>>>>>>
>>>>>> +static int cdns_pcie_host_training_complete(struct cdns_pcie *pcie)
>>>>>
>>>>> This is kind of weird because it's named like a predicate, i.e., "this
>>>>> function tells me whether link training is complete", but it returns
>>>>> *zero* for success.
>>>>>
>>>>> This is the opposite of j721e_pcie_link_up(), which returns "true"
>>>>> when the link is up, so code like this reads naturally:
>>>>>
>>>>> if (pcie->ops->link_up(pcie))
>>>>> /* do something if the link is up */
>>>>
>>>> I agree. The function name can be changed to indicate that it is
>>>> waiting for completion rather than indicating completion. If this is
>>>> the only change, I will post a patch to fix it. On the other hand,
>>>> based on your comments in the next section, I am thinking of an
>>>> alternative approach of merging the current
>>>> "cdns_pcie_host_training_complete()" function's operation as well
>>>> into the "cdns_pcie_host_wait_for_link()" function. If this is
>>>> acceptable, I will post a different patch and the name change patch
>>>> won't be necessary.
>>>
>>> Yeah, sorry, I meant to delete this part of my response after I wrote
>>> the one below.
>>>
>>>>>> @@ -118,6 +141,10 @@ static int cdns_pcie_retrain(struct cdns_pcie *pcie)
>>>>>> cdns_pcie_rp_writew(pcie, pcie_cap_off + PCI_EXP_LNKCTL,
>>>>>> lnk_ctl);
>>>>>>
>>>>>> + ret = cdns_pcie_host_training_complete(pcie);
>>>>>> + if (ret)
>>>>>> + return ret;
>>>>>> +
>>>>>> ret = cdns_pcie_host_wait_for_link(pcie);
>>>>>
>>>>> It seems a little clumsy that we wait for two things in succession:
>>>>>
>>>>> - cdns_pcie_host_training_complete() waits up to 1s for
>>>>> PCI_EXP_LNKSTA_LT to be cleared
>>>>>
>>>>> - cdns_pcie_host_wait_for_link() waits between .9s and 1s for
>>>>> LINK_UP_DL_COMPLETED on j721e (and not at all for other platforms)
>>>>
>>>> Is it acceptable to merge "cdns_pcie_host_training_complete()" into
>>>> "cdns_pcie_host_wait_for_link()"?
>>>
>>> That's what I'm proposing. Maybe someone who is more familiar with
>>> Cadence would have an argument against it, but I think making it
>>> structurally the same as dw_pcie_wait_for_link() would be a good
>>> thing.
>>
>> Thank you for the confirmation. I will work on it and post a patch.
>
> Ping, do you still plan to do this? Lorenzo currently has the
> v2 patch on his pci/controller/cadence branch [1], but I haven't
> merged it into -next yet on the assumption that a new version is
> coming.

Sorry for the delay. I have posted the V3 patch at:
https://lore.kernel.org/r/20230607091427.852473-1-s-vadapalli@ti.com/

>
> Bjorn
>
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/commit/?h=controller/cadence&id=0e12f8302369

--
Regards,
Siddharth.

©2024 GT.net. A Gossamer Threads company.
5th Floor, 455 Granville St., Vancouver, BC V6C 1T1, Canada | Legal