Message-ID: <554df533-df52-ee69-6c6f-effba88b55e1@nvidia.com>
Date:   Mon, 26 Sep 2022 20:32:07 +0530
From:   Vidya Sagar <vidyas@...dia.com>
To:     Bjorn Helgaas <helgaas@...nel.org>
Cc:     jingoohan1@...il.com, gustavo.pimentel@...opsys.com,
        lpieralisi@...nel.org, robh@...nel.org, kw@...ux.com,
        bhelgaas@...gle.com, mani@...nel.org,
        Sergey.Semin@...kalelectronics.ru, dmitry.baryshkov@...aro.org,
        linmq006@...il.com, ffclaire1224@...il.com,
        thierry.reding@...il.com, jonathanh@...dia.com,
        linux-pci@...r.kernel.org, linux-arm-msm@...r.kernel.org,
        linux-kernel@...r.kernel.org, kthota@...dia.com,
        mmaddireddy@...dia.com, sagar.tv@...il.com
Subject: Re: [PATCH V4 0/3] PCI: designware-ep: Fix DBI access before core
 init



On 9/20/2022 4:10 AM, Bjorn Helgaas wrote:
> External email: Use caution opening links or attachments
> 
> 
> On Tue, Sep 20, 2022 at 12:03:39AM +0530, Vidya Sagar wrote:
>> This series attempts to fix the issue with core register (Ex:- DBI) accesses
>> causing system hang issues in platforms where there is a dependency on the
>> availability of PCIe Reference clock from the host for their core
>> initialization.
>> This series is verified on Tegra194 & Tegra234 platforms.
> 
> I think this design is just kind of weird, specifically, the fact that
> setting .core_init_notifier makes dw_pcie_ep_init() bail out early.
> The usual pattern is more like "if the specific driver sets this
> function pointer, the generic code calls it."

Thanks for the review Bjorn.

Typically the PCIe endpoints run using the reference clock from the 
hosts that they are connected to. Our hardware designers followed the 
same approach here as well, but the main difference here being that the 
controllers operating in the endpoint mode are not standalone 
controllers but part of a bigger Tegra (/Qcom) systems.
So, the complete controller initialization sequence just can't happen 
during the boot stage itself, hence the boot initialization sequence 
needs to be split into two parts viz a) early initialization - that just 
parses DT, does the programming that doesn't depend on the reference 
clock from host and b) does the programming that can only be performed 
after reference clock is available from the host
We are working with our hardware designers to avoid this dependency on 
the reference clock from the host so that all the programming can happen 
during boot itself and hardware is smart enough to switch to using the 
reference clock from the host when it is available. But, this is for 
future designs and Tegra194 & Tegra234 continue to have this limitation.

> 
> The name "dw_pcie_ep_init_complete()" is not as helpful as it could
> be: it tells us something about what has happened before this point,
> but it doesn't tell us anything about what dw_pcie_ep_init_complete()
> *does*.

To be inline with new ops ep_init_late that I added in this series, 
would it be fine to name this as dw_pcie_ep_init_late()?

> 
> Same thing with dw_pcie_ep_init_notify() -- it doesn't tell us
> anything about what the function *does*.

Would it make more sense to rename it as dw_pcie_ep_linkup_notify()?

   I see that it calls
> pci_epc_init_notify(), which calls a notifier call chain (currently
> always empty except for a test case).  I think pci_epc_linkup() is a
> better name because it says something about what's happening: the link
> is now up and we're telling somebody about it.  "pci_epc_init_notify()"
> doesn't convey that.  "pci_epc_core_initialized()" might.

Ok. I'll rename it to pci_epc_core_initialized().

> 
> It looks like both qcom and tegra wait for an interrupt before calling
> dw_pcie_ep_init_notify(), but I'm a little concerned because I can't
> figure out what specifically they do to start the process that
> ultimately generates the interrupt.

As part of 'start'ing the endpoint as mentioned in 
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/Documentation/PCI/endpoint/pci-test-howto.rst#n101
we execute the following
echo 1 > controllers/141a0000.pcie-ep/start
that enables the interrupt generation for toggles on the PERST# line.

   Presumably they request the IRQ
> *before* starting the process, but there's not much between the
> devm_request_threaded_irq() and the interrupt handler, which makes me
> wonder if both are racy.

I don't think there is any race between these two as the 'start' is 
initiated from the user space. Not sure if I'm missing something here 
though.

> 
>> Manivannan, could you please verify on qcom platforms?
>>
>> V4:
>> * Addressed review comments from Bjorn and Manivannan
>> * Added .ep_init_late() ops
>> * Added patches to refactor code in qcom and tegra platforms
>>
>> Vidya Sagar (3):
>>    PCI: designware-ep: Fix DBI access before core init
>>    PCI: qcom-ep: Refactor EP initialization completion
>>    PCI: tegra194: Refactor EP initialization completion
>>
>>   .../pci/controller/dwc/pcie-designware-ep.c   | 112 ++++++++++--------
>>   drivers/pci/controller/dwc/pcie-designware.h  |  10 +-
>>   drivers/pci/controller/dwc/pcie-qcom-ep.c     |  27 +++--
>>   drivers/pci/controller/dwc/pcie-tegra194.c    |   4 +-
>>   4 files changed, 85 insertions(+), 68 deletions(-)
>>
>> --
>> 2.17.1
>>

Powered by blists - more mailing lists