On Wed, Mar 13, 2024 at 05:39:22PM +0530, Manivannan Sadhasivam wrote:
> PCIe host controller drivers are supposed to properly reset the endpoint
> devices during host shutdown/reboot. Currently, Qcom driver doesn't do
> anything during host shutdown/reboot, resulting in both PERST# and refclk
> getting disabled at the same time. This prevents the endpoint device
> firmware to properly reset the state machine. Because, if the refclk is
> cutoff immediately along with PERST#, access to device specific registers
> within the endpoint will result in a firmware crash.
>
> To address this issue, let's call qcom_pcie_host_deinit() inside the
> shutdown callback, that asserts PERST# and then cuts off the refclk with a
> delay of 1ms, thus allowing the endpoint device firmware to properly
> cleanup the state machine.
I guess this 1ms delay is the PERST_DELAY_US hidden inside
qcom_ep_reset_assert()? I assume the refclk disable is done by
clk_bulk_disable_unprepare()?
#define PERST_DELAY_US 1000
qcom_pcie_shutdown
qcom_pcie_host_deinit
qcom_ep_reset_assert
gpiod_set_value_cansleep(pcie->reset, 1);
usleep_range(PERST_DELAY_US, PERST_DELAY_US + 500); <--
phy_power_off(pcie->phy)
pcie->cfg->ops->deinit()
qcom_pcie_deinit_...
clk_bulk_disable_unprepare <--
Is there a spec citation for this delay requirement? If not, how do
we know 1ms is enough for whatever the firmware needs to do?
Do other drivers require similar changes?
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx>
> ---
> drivers/pci/controller/dwc/pcie-qcom.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 2ce2a3bd932b..41434bc4761a 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -1618,6 +1618,13 @@ static int qcom_pcie_resume_noirq(struct device *dev)
> return 0;
> }
>
> +static void qcom_pcie_shutdown(struct platform_device *pdev)
> +{
> + struct qcom_pcie *pcie = platform_get_drvdata(pdev);
> +
> + qcom_pcie_host_deinit(&pcie->pci->pp);
> +}
> +
> static const struct of_device_id qcom_pcie_match[] = {
> { .compatible = "qcom,pcie-apq8064", .data = &cfg_2_1_0 },
> { .compatible = "qcom,pcie-apq8084", .data = &cfg_1_0_0 },
> @@ -1670,5 +1677,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);
>
> ---
> base-commit: 51459eb30f88651d3688b9e95fed0f97767ececb
> change-id: 20240313-pci-qcom-shutdown-d86298186560
>
> Best regards,
> --
> Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx>
>
