Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <c0f5b71d-ddee-850e-e8ba-1304e8480e8e@ti.com>
Date:   Tue, 18 Apr 2023 09:19:05 +0530
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.10.0
CC:     <tjoseph@cadence.com>, <robh@kernel.org>, <bhelgaas@google.com>,
        <nadeem@cadence.com>, "Raghavendra, Vignesh" <vigneshr@ti.com>,
        <linux-pci@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <linux-arm-kernel@lists.infradead.org>, <srk@ti.com>, <nm@ti.com>,
        <s-vadapalli@ti.com>
Subject: Re: [PATCH v2] PCI: cadence: Fix Gen2 Link Retraining process
To:     Bjorn Helgaas <helgaas@kernel.org>, <kw@linux.com>,
        <lpieralisi@kernel.org>
References: <20230330170218.GA3155390@bhelgaas>
Content-Language: en-US
From:   Siddharth Vadapalli <s-vadapalli@ti.com>
In-Reply-To: <20230330170218.GA3155390@bhelgaas>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
Precedence: bulk

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.