Received-SPF: pass (google.com: domain of linux-kernel+bounces-101658-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Wed, 13 Mar 2024 20:32:42 +0530
From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
To: Bjorn Helgaas <helgaas@kernel.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: <20240313150242.GA2656@thinkpad>
References: <20240313-pci-qcom-shutdown-v1-1-fb1515334bfa@linaro.org>
 <20240313143614.GA916555@bhelgaas>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20240313143614.GA916555@bhelgaas>

On Wed, Mar 13, 2024 at 09:36:14AM -0500, Bjorn Helgaas wrote:
> 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()?
> 

Yes to both.

>   #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?
> 

Both PCIe base spec and Electromechanical spec only mentions Tperst, which is
the minimum time PERST# should remain asserted. But there is no mention about
the time, refclk should be active.

So I used the existing delay post PERST# assert in the driver. I do not know if
that is enough for all the endpoints out in the wild, but atleast satisfies the
requirement of the endpoint I'm working on (which is another Qcom SoC in EP
mode).

We can change the delay if someone reports any issue with the existing one.
Atleast, that's the best we could do in this situation.

> Do other drivers require similar changes?
> 

Most likely yes, but that also depends on when the drivers are cutting off the
refclk. Not all drivers are implementing the shutdown callback, and even few of
the ones implementing, do not assert PERST# since it is optional.

- Mani

> > 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>
> > 

-- 
மணிவண்ணன் சதாசிவம்