Re: [PATCH v8 4/4] PCI: qcom: Implement shutdown() callback

[Konrad Dybcio]

On 11/7/26 2:45 AM, Krishna Chaitanya Chundru wrote:
> From: Manivannan Sadhasivam <mani@xxxxxxxxxx>
> 
> PCIe host controllers should bring the link down cleanly before system
> shutdown/reboot proceeds to remove power/clocks from the controller.
> Without this, the link may still be up and endpoints still have
> transactions in flight when power/clocks are cut, which can trip SMMU
> translation faults or NoC protocol errors.
> 
> Reuse dw_pcie_suspend_noirq() in the shutdown path to force the link
> into L2, putting it into D3cold.
> 
> device_shutdown() runs with interrupts enabled, unlike suspend_noirq().
> Mask the chained MSI IRQ(s) and the Global IRQ before tearing down the
> link and clocks/PHY, since a late/spurious interrupt could otherwise
> reach a handler that touches now-unclocked PARF/DBI registers. Use
> dw_pcie_mask_msi() rather than dw_pcie_free_msi() so the MSI IRQ domain
> and the MSI mappings held by downstream devices (which device_shutdown()
> does not remove) are left intact.
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx>
> Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@xxxxxxxxxxxxxxxx>
> ---
>  drivers/pci/controller/dwc/pcie-qcom.c | 41 ++++++++++++++++++++++++++++++++++
>  1 file changed, 41 insertions(+)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index d39753f0e724..c5b06db3b92a 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -2046,6 +2046,9 @@ static int qcom_pcie_ecam_host_init(struct pci_config_window *cfg)
>  
>  	dw_pcie_msi_init(pp);
>  
> +	/* Stash pci so qcom_pcie_shutdown() can mask the MSI IRQ(s) later */
> +	platform_set_drvdata(to_platform_device(dev), pci);
> +
>  	return devm_add_action_or_reset(dev, qcom_pci_free_msi, pp);
>  }
>  
> @@ -2466,6 +2469,43 @@ static int qcom_pcie_probe(struct platform_device *pdev)
>  	return ret;
>  }
>  
> +static void qcom_pcie_shutdown(struct platform_device *pdev)
> +{
> +	const struct qcom_pcie_cfg *pcie_cfg = of_device_get_match_data(&pdev->dev);
> +	struct qcom_pcie *pcie = NULL;
> +	struct dw_pcie *pci;
> +
> +	if (pcie_cfg && pcie_cfg->firmware_managed) {
> +		/*
> +		 * Firmware owns the link teardown and clock/PHY shutdown in
> +		 * this mode; Linux only owns the chained MSI IRQ(s), which
> +		 * still need to be masked off before shutdown proceeds.
> +		 */
> +		pci = platform_get_drvdata(pdev);

Revisiting my older comment, we can still stash `pci` in drvdata regardless
of the flavor and do `struct qcom_pcie *pcie = to_qcom_pcie(pci);` in
places where we need it

Konrad

> +	} else {
> +		pcie = platform_get_drvdata(pdev);
> +		pci = pcie->pci;
> +
> +		if (pcie->global_irq)
> +			disable_irq(pcie->global_irq);
> +	}
> +
> +	if (pci->pp.use_imsi_rx)
> +		dw_pcie_mask_msi(&pci->pp);
> +
> +	/*
> +	 * In firmware-managed mode 'pcie' stays NULL and firmware owns the
> +	 * link/clock teardown. Otherwise, skip if the controller is already
> +	 * suspended (e.g. a prior resume failed), since dw_pcie_suspend_noirq()
> +	 * would touch DBI registers with clocks/interconnects already off.
> +	 */
> +	if (pcie && !pcie->pci->suspended)
> +		dw_pcie_suspend_noirq(pcie->pci, true);
> +
> +	pm_runtime_put_sync(&pdev->dev);
> +	pm_runtime_disable(&pdev->dev);
> +}
> +
>  static int qcom_pcie_suspend_noirq(struct device *dev)
>  {
>  	struct qcom_pcie *pcie;
> @@ -2657,5 +2697,6 @@ static struct platform_driver qcom_pcie_driver = {
>  		.pm = &qcom_pcie_pm_ops,
>  		.probe_type = PROBE_PREFER_ASYNCHRONOUS,
>  	},
> +	.shutdown = qcom_pcie_shutdown,
>  };
>  builtin_platform_driver(qcom_pcie_driver);
>