Re: [PATCH] PCI: qcom: Clear Attention Button Present in Slot Capabilities

[Bjorn Helgaas <helgaas@kernel.org>]

On Fri, Jul 10, 2026 at 06:20:57AM -0700, Qiang Yu wrote:
> Qcom PCIe Root Ports advertise the Attention Button Present (ABP) bit in
> the Slot Capabilities register, but the slots have no attention button.

I'm a little surprised that this would be hard-wired into the RP
instead of being some kind of strapping or config option, which would
allow the SoC to be used in platforms that do have Attention Buttons.

If it's actually hard-wired, this might be OK.  If ABP could be made
to work in hypothetical platforms that include it, maybe this should
be based on a DT property?

> When ABP is set, pcie_enable_notification() in the pciehp driver enables
> the Attention Button Pressed interrupt (ABPE) and leaves the Presence
> Detect Changed interrupt (PDCE) disabled. As a result, presence detect
> changes are never reported and surprise hot-plug detection that relies on
> Presence Detect Changed events does not work.
> 
> While at it, rename qcom_pcie_set_slot_nccs() to qcom_pcie_set_slot_cap()
> since it now programs more than just the NCCS field.

It'd be nice if the rename were a separate preparatory patch so the
functional part is by itself and obvious.

> Signed-off-by: Qiang Yu <qiang.yu@oss.qualcomm.com>
> ---
>  drivers/pci/controller/dwc/pcie-qcom.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index b193c989b2b8..89c2d4cb1fbe 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_aspm_l0s(struct dw_pcie *pci)
>  	dw_pcie_dbi_ro_wr_dis(pci);
>  }
>  
> -static void qcom_pcie_set_slot_nccs(struct dw_pcie *pci)
> +static void qcom_pcie_set_slot_cap(struct dw_pcie *pci)
>  {
>  	u16 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
>  	u32 val;
> @@ -372,6 +372,11 @@ static void qcom_pcie_set_slot_nccs(struct dw_pcie *pci)
>  	 */
>  	val = readl(pci->dbi_base + offset + PCI_EXP_SLTCAP);
>  	val |= PCI_EXP_SLTCAP_NCCS;

Need a blank line here.

> +	/*
> +	 * Qcom PCIe Root Ports do not support Attention Button, so clear
> +	 * Attention Button Present in Slot Capabilities.
> +	 */
> +	val &= ~PCI_EXP_SLTCAP_ABP;
>  	writel(val, pci->dbi_base + offset + PCI_EXP_SLTCAP);
>  
>  	dw_pcie_dbi_ro_wr_dis(pci);
> @@ -580,7 +585,7 @@ static int qcom_pcie_post_init_2_1_0(struct qcom_pcie *pcie)
>  	writel(CFG_BRIDGE_SB_INIT,
>  	       pci->dbi_base + AXI_MSTR_RESP_COMP_CTRL1);
>  
> -	qcom_pcie_set_slot_nccs(pcie->pci);
> +	qcom_pcie_set_slot_cap(pcie->pci);
>  
>  	return 0;
>  }
> @@ -660,7 +665,7 @@ static int qcom_pcie_post_init_1_0_0(struct qcom_pcie *pcie)
>  		writel(val, pcie->parf + PARF_AXI_MSTR_WR_ADDR_HALT);
>  	}
>  
> -	qcom_pcie_set_slot_nccs(pcie->pci);
> +	qcom_pcie_set_slot_cap(pcie->pci);
>  
>  	return 0;
>  }
> @@ -759,7 +764,7 @@ static int qcom_pcie_post_init_2_3_2(struct qcom_pcie *pcie)
>  	val |= EN;
>  	writel(val, pcie->parf + PARF_AXI_MSTR_WR_ADDR_HALT_V2);
>  
> -	qcom_pcie_set_slot_nccs(pcie->pci);
> +	qcom_pcie_set_slot_cap(pcie->pci);
>  
>  	return 0;
>  }
> @@ -1078,7 +1083,7 @@ static int qcom_pcie_post_init_2_7_0(struct qcom_pcie *pcie)
>  		writel(WR_NO_SNOOP_OVERRIDE_EN | RD_NO_SNOOP_OVERRIDE_EN,
>  				pcie->parf + PARF_NO_SNOOP_OVERRIDE);
>  
> -	qcom_pcie_set_slot_nccs(pcie->pci);
> +	qcom_pcie_set_slot_cap(pcie->pci);
>  
>  	return 0;
>  }
> 
> ---
> base-commit: 3edb3a038d423480efeb204dbc6ddc9a292f8ecb
> change-id: 20260710-clear_abp_0710-2f81e92b9da4
> 
> Best regards,
> --  
> Qiang Yu <qiang.yu@oss.qualcomm.com>
>