Re: [PATCH] PCI: qcom-ep: Move controller cleanups to qcom_pcie_perst_deassert()

[Krishna Chaitanya Chundru <quic_krichai@quicinc.com>]

On 7/29/2024 7:25 PM, Manivannan Sadhasivam wrote:
> On Mon, Jul 29, 2024 at 05:58:31PM +0530, Krishna Chaitanya Chundru wrote:
>>
>>
>> On 7/29/2024 5:52 PM, Manivannan Sadhasivam wrote:
>>> Currently, the endpoint cleanup function dw_pcie_ep_cleanup() and EPF
>>> deinit notify function pci_epc_deinit_notify() are called during the
>>> execution of qcom_pcie_perst_assert() i.e., when the host has asserted
>>> PERST#. But quickly after this step, refclk will also be disabled by the
>>> host.
>>>
>>> All of the Qcom endpoint SoCs supported as of now depend on the refclk from
>>> the host for keeping the controller operational. Due to this limitation,
>>> any access to the hardware registers in the absence of refclk will result
>>> in a whole endpoint crash. Unfortunately, most of the controller cleanups
>>> require accessing the hardware registers (like eDMA cleanup performed in
>>> dw_pcie_ep_cleanup(), powering down MHI EPF etc...). So these cleanup
>>> functions are currently causing the crash in the endpoint SoC once host
>>> asserts PERST#.
>>>
>>> One way to address this issue is by generating the refclk in the endpoint
>>> itself and not depending on the host. But that is not always possible as
>>> some of the endpoint designs do require the endpoint to consume refclk from
>>> the host (as I was told by the Qcom engineers).
>>>
>>> So let's fix this crash by moving the controller cleanups to the start of
>>> the qcom_pcie_perst_deassert() function. qcom_pcie_perst_deassert() is
>>> called whenever the host has deasserted PERST# and it is guaranteed that
>>> the refclk would be active at this point. So at the start of this function,
>>> the controller cleanup can be performed. Once finished, rest of the code
>>> execution for PERST# deassert can continue as usual.
>>>
>> How about doing the cleanup as part of pme turnoff message.
>> As host waits for L23 ready from the device side. we can use that time
>> to cleanup the host before sending L23 ready.
>>
> 
> Yes, but that's only applicable if the host properly powers down the device. But
> it won't work in the case of host crash or host abrupt poweroff.
> 
> - Mani
> 
Ack.

- Krishna Chaitanya.
>> - Krishna Chaitanya.
>>> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
>>> ---
>>>    drivers/pci/controller/dwc/pcie-qcom-ep.c | 12 ++++++++++--
>>>    1 file changed, 10 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/pci/controller/dwc/pcie-qcom-ep.c b/drivers/pci/controller/dwc/pcie-qcom-ep.c
>>> index 2319ff2ae9f6..e024b4dcd76d 100644
>>> --- a/drivers/pci/controller/dwc/pcie-qcom-ep.c
>>> +++ b/drivers/pci/controller/dwc/pcie-qcom-ep.c
>>> @@ -186,6 +186,8 @@ struct qcom_pcie_ep_cfg {
>>>     * @link_status: PCIe Link status
>>>     * @global_irq: Qualcomm PCIe specific Global IRQ
>>>     * @perst_irq: PERST# IRQ
>>> + * @cleanup_pending: Cleanup is pending for the controller (because refclk is
>>> + *                   needed for cleanup)
>>>     */
>>>    struct qcom_pcie_ep {
>>>    	struct dw_pcie pci;
>>> @@ -214,6 +216,7 @@ struct qcom_pcie_ep {
>>>    	enum qcom_pcie_ep_link_status link_status;
>>>    	int global_irq;
>>>    	int perst_irq;
>>> +	bool cleanup_pending;
>>>    };
>>>    static int qcom_pcie_ep_core_reset(struct qcom_pcie_ep *pcie_ep)
>>> @@ -389,6 +392,12 @@ static int qcom_pcie_perst_deassert(struct dw_pcie *pci)
>>>    		return ret;
>>>    	}
>>> +	if (pcie_ep->cleanup_pending) {
>>> +		pci_epc_deinit_notify(pci->ep.epc);
>>> +		dw_pcie_ep_cleanup(&pci->ep);
>>> +		pcie_ep->cleanup_pending = false;
>>> +	}
>>> +
>>>    	/* Assert WAKE# to RC to indicate device is ready */
>>>    	gpiod_set_value_cansleep(pcie_ep->wake, 1);
>>>    	usleep_range(WAKE_DELAY_US, WAKE_DELAY_US + 500);
>>> @@ -522,10 +531,9 @@ static void qcom_pcie_perst_assert(struct dw_pcie *pci)
>>>    {
>>>    	struct qcom_pcie_ep *pcie_ep = to_pcie_ep(pci);
>>> -	pci_epc_deinit_notify(pci->ep.epc);
>>> -	dw_pcie_ep_cleanup(&pci->ep);
>>>    	qcom_pcie_disable_resources(pcie_ep);
>>>    	pcie_ep->link_status = QCOM_PCIE_EP_LINK_DISABLED;
>>> +	pcie_ep->cleanup_pending = true;
>>>    }
>>>    /* Common DWC controller ops */
>