Message ID | 20230315070800.1615527-1-s-vadapalli@ti.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | [v2] PCI: cadence: Fix Gen2 Link Retraining process | expand |
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
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
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.
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
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
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
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
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
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;