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@xxxxxx> >>>>> Reviewed-by: Vignesh Raghavendra <vigneshr@xxxxxx> >>>>> --- >>>>> 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@xxxxxx >>>>> >>>>> .../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.
