Re: [PATCH] PCI: qcom: Advertise hotplug with no command completion support

[Konrad Dybcio]

On 3/21/26 1:01 PM, Manivannan Sadhasivam wrote:
> On Fri, Mar 20, 2026 at 07:10:19AM +0530, Krishna Chaitanya Chundru wrote:
>>
>>
>> On 3/19/2026 4:29 PM, Konrad Dybcio wrote:
>>> On 3/16/26 1:22 PM, Krishna Chaitanya Chundru wrote:
>>>>
>>>> On 3/15/2026 3:39 PM, Manivannan Sadhasivam wrote:
>>>>> On Sat, Mar 14, 2026 at 07:26:34AM +0530, Krishna Chaitanya Chundru wrote:
>>>>>> QCOM PCIe controller advertise hotplug capability in hardware but do not
>>>>>> support hotplug command completion. As a result, the PCI core registers
>>>>>> the pciehp service and issues hotplug commands that never gets completions,
>>>>>> leading to repeated timeout warnings and multi-second delays during boot
>>>>>> and suspend/resume.
>>>>>>
>>>>>> Commit a54db86ddc153 ("PCI: qcom: Do not advertise hotplug capability for
>>>>>> IPs v2.7.0 and v1.9.0") avoided these timeouts by clearing the Hot-Plug
>>>>>> Capability bit entirely, which also disabled all hotplug functionality.
>>>>>>
>>>>> Just some background: I added commit a54db86ddc153 to disable hotplug for Qcom
>>>>> PCIe Root Ports since we were seeing completion timeouts for hotplug commands
>>>>> and also the PRSNT# signal was not exposed on any of our SoCs. After checking
>>>>> with some internal folks I learned that hotplug functionality was not exercised
>>>>> in Linux. So these facts made me believe that hotplug was not suppored at all.
>>>>>
>>>>> But it turned out that the Qcom Root Ports support "Data Link Layer State
>>>>> Changed Events" such as DL_Up/Down events.
>>>>>
>>>>>> Instead of disabling hotplug, mark these controllers as not supporting
>>>>>> command completion by setting the No Command Completed Support (NCCS) bit
>>>>>> in the Slot Capabilities register. This prevents the PCI hotplug driver
>>>>>> from waiting for commands completion while still allowing hotplug-related
>>>>>> functionality such as Data Link Layer state change events.
>>>>>>
>>>>>> Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@xxxxxxxxxxxxxxxx>
>>>>>> ---
>>>>>>    drivers/pci/controller/dwc/pcie-qcom.c | 2 +-
>>>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
>>>>>> index 67a16af69ddc75fca1b123e70715e692a91a9135..a2924610f3625f2456a491473c135840e31bafb9 100644
>>>>>> --- a/drivers/pci/controller/dwc/pcie-qcom.c
>>>>>> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
>>>>>> @@ -358,7 +358,7 @@ static void qcom_pcie_clear_hpc(struct dw_pcie *pci)
>>>>>>        dw_pcie_dbi_ro_wr_en(pci);
>>>>>>          val = readl(pci->dbi_base + offset + PCI_EXP_SLTCAP);
>>>>>> -    val &= ~PCI_EXP_SLTCAP_HPC;
>>>>>> +    val |= PCI_EXP_SLTCAP_NCCS;
>>>>> Are you sure that this is the only non-supported capability? What about
>>>>> Attention, Presence, Power Fault, MRL etc...?
>>>> Even though there no signals required for attention, presence etc in the hardware
>>>> there is a way to generate these MSI's with these bits set through parf, so technically
>>>> so other co-processor in the system can trigger interrupts.
>>> Are you saying that the RC itself will not generate them based on what
>>> happens on the bus, but they can be triggered artificially?
>> Yes there are parf registers through which msi's can be triggered
>> artificially.
>>
> 
> As Krishna said, it seems there are ways to *inject* these events through
> platform specific means (through PARF register space), even if the associated
> signals are not exposed to the slot. And we might end up using those events at
> some point.
> 
> So it is OK from my opinion to just disable NCCS. But this also warrants a
> comment in the code.

Alright, in any case, this is required for PCIe-over-TBT hotplug without
tapping into the "global" irq

Tested-by: Konrad Dybcio <konrad.dybcio@xxxxxxxxxxxxxxxx> # Hamoa CRD, tunneled link

and with the comment added:

Reviewed-by: Konrad Dybcio <konrad.dybcio@xxxxxxxxxxxxxxxx>

Konrad