Received-SPF: pass (google.com: domain of linux-kernel+bounces-101624-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Wed, 13 Mar 2024 09:36:14 -0500
From: Bjorn Helgaas <helgaas@kernel.org>
To: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Cc: Bjorn Andersson <andersson@kernel.org>,
	Konrad Dybcio <konrad.dybcio@linaro.org>,
	Lorenzo Pieralisi <lpieralisi@kernel.org>,
	Krzysztof =?utf-8?Q?Wilczy=C5=84ski?= <kw@linux.com>,
	Rob Herring <robh@kernel.org>, Bjorn Helgaas <bhelgaas@google.com>,
	linux-pci@vger.kernel.org, linux-arm-msm@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH] PCI: qcom: Implement shutdown() callback to properly
 reset the endpoint devices
Message-ID: <20240313143614.GA916555@bhelgaas>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240313-pci-qcom-shutdown-v1-1-fb1515334bfa@linaro.org>

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@linaro.org>
> ---
>  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@linaro.org>
>