[PATCH v2] PCI: qcom: Ensure that PERST is asserted for at least 100 ms

[PATCH v2] PCI: qcom: Ensure that PERST is asserted for at least 100 ms

[Niklas Cassel]

Currently, there is only a 1 ms sleep after asserting PERST.

Reading the datasheets for different endpoints, some require PERST to be
asserted for 10 ms in order for the endpoint to perform a reset, others
require it to be asserted for 50 ms.

Several SoCs using this driver uses PCIe Mini Card, where we don't know
what endpoint will be plugged in.

The PCI Express Card Electromechanical Specification specifies:
"On power up, the deassertion of PERST# is delayed 100 ms (TPVPERL) from
the power rails achieving specified operating limits."

Add a sleep of 100 ms before deasserting PERST, in order to ensure that
we are compliant with the spec.

Fixes: 82a823833f4e ("PCI: qcom: Add Qualcomm PCIe controller driver")
Signed-off-by: Niklas Cassel <niklas.cassel@linaro.org>
Acked-by: Stanimir Varbanov <svarbanov@mm-sol.com>
Cc: stable@vger.kernel.org # 4.5+
---
Changes since v1:
Move the sleep into qcom_ep_reset_deassert()

 drivers/pci/controller/dwc/pcie-qcom.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index 0ed235d560e3..5d1713069d14 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -178,6 +178,8 @@ static void qcom_ep_reset_assert(struct qcom_pcie *pcie)
 
 static void qcom_ep_reset_deassert(struct qcom_pcie *pcie)
 {
+	/* Ensure that PERST has been asserted for at least 100 ms */
+	msleep(100);
 	gpiod_set_value_cansleep(pcie->reset, 0);
 	usleep_range(PERST_DELAY_US, PERST_DELAY_US + 500);
 }
-- 
2.21.0

Re: [PATCH v2] PCI: qcom: Ensure that PERST is asserted for at least 100 ms

[Bjorn Helgaas]

On Wed, May 29, 2019 at 11:43:52AM +0200, Niklas Cassel wrote:
> Currently, there is only a 1 ms sleep after asserting PERST.
> 
> Reading the datasheets for different endpoints, some require PERST to be
> asserted for 10 ms in order for the endpoint to perform a reset, others
> require it to be asserted for 50 ms.
> 
> Several SoCs using this driver uses PCIe Mini Card, where we don't know
> what endpoint will be plugged in.

I think this patch is absolutely the right thing to do.  We don't know
what might be plugged in, so we should support arbitrary endpoints.

One could imagine some sort of DT property to identify closed
environments where there is no need to support arbitrary endpoints and
it might be desirable to reduce the delay.  But that should wait
until there's a need for it and if the need *does* arise, hopefully
the definition of such a property could be generic across all SoCs.

> The PCI Express Card Electromechanical Specification specifies:
> "On power up, the deassertion of PERST# is delayed 100 ms (TPVPERL) from
> the power rails achieving specified operating limits."

It'd be nice to include the complete citation, i.e., "PCI Express Card
Electromechanical Specification r2.0, sec 2.2"

> Add a sleep of 100 ms before deasserting PERST, in order to ensure that
> we are compliant with the spec.
> 
> Fixes: 82a823833f4e ("PCI: qcom: Add Qualcomm PCIe controller driver")
> Signed-off-by: Niklas Cassel <niklas.cassel@linaro.org>
> Acked-by: Stanimir Varbanov <svarbanov@mm-sol.com>
> Cc: stable@vger.kernel.org # 4.5+
> ---
> Changes since v1:
> Move the sleep into qcom_ep_reset_deassert()
> 
>  drivers/pci/controller/dwc/pcie-qcom.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 0ed235d560e3..5d1713069d14 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -178,6 +178,8 @@ static void qcom_ep_reset_assert(struct qcom_pcie *pcie)
>  
>  static void qcom_ep_reset_deassert(struct qcom_pcie *pcie)
>  {
> +	/* Ensure that PERST has been asserted for at least 100 ms */
> +	msleep(100);
>  	gpiod_set_value_cansleep(pcie->reset, 0);
>  	usleep_range(PERST_DELAY_US, PERST_DELAY_US + 500);
>  }
> -- 
> 2.21.0
> 

Re: [PATCH v2] PCI: qcom: Ensure that PERST is asserted for at least 100 ms

[Lorenzo Pieralisi]

On Wed, May 29, 2019 at 07:52:32AM -0500, Bjorn Helgaas wrote:
> On Wed, May 29, 2019 at 11:43:52AM +0200, Niklas Cassel wrote:
> > Currently, there is only a 1 ms sleep after asserting PERST.
> > 
> > Reading the datasheets for different endpoints, some require PERST to be
> > asserted for 10 ms in order for the endpoint to perform a reset, others
> > require it to be asserted for 50 ms.
> > 
> > Several SoCs using this driver uses PCIe Mini Card, where we don't know
> > what endpoint will be plugged in.
> 
> I think this patch is absolutely the right thing to do.  We don't know
> what might be plugged in, so we should support arbitrary endpoints.
> 
> One could imagine some sort of DT property to identify closed
> environments where there is no need to support arbitrary endpoints and
> it might be desirable to reduce the delay.  But that should wait
> until there's a need for it and if the need *does* arise, hopefully
> the definition of such a property could be generic across all SoCs.
> 
> > The PCI Express Card Electromechanical Specification specifies:
> > "On power up, the deassertion of PERST# is delayed 100 ms (TPVPERL) from
> > the power rails achieving specified operating limits."
> 
> It'd be nice to include the complete citation, i.e., "PCI Express Card
> Electromechanical Specification r2.0, sec 2.2"

I will add it and merge the patch.

Thanks,
Lorenzo

> > Add a sleep of 100 ms before deasserting PERST, in order to ensure that
> > we are compliant with the spec.
> > 
> > Fixes: 82a823833f4e ("PCI: qcom: Add Qualcomm PCIe controller driver")
> > Signed-off-by: Niklas Cassel <niklas.cassel@linaro.org>
> > Acked-by: Stanimir Varbanov <svarbanov@mm-sol.com>
> > Cc: stable@vger.kernel.org # 4.5+
> > ---
> > Changes since v1:
> > Move the sleep into qcom_ep_reset_deassert()
> > 
> >  drivers/pci/controller/dwc/pcie-qcom.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> > index 0ed235d560e3..5d1713069d14 100644
> > --- a/drivers/pci/controller/dwc/pcie-qcom.c
> > +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> > @@ -178,6 +178,8 @@ static void qcom_ep_reset_assert(struct qcom_pcie *pcie)
> >  
> >  static void qcom_ep_reset_deassert(struct qcom_pcie *pcie)
> >  {
> > +	/* Ensure that PERST has been asserted for at least 100 ms */
> > +	msleep(100);
> >  	gpiod_set_value_cansleep(pcie->reset, 0);
> >  	usleep_range(PERST_DELAY_US, PERST_DELAY_US + 500);
> >  }
> > -- 
> > 2.21.0
> > 

Re: [PATCH v2] PCI: qcom: Ensure that PERST is asserted for at least 100 ms

[Lorenzo Pieralisi]

On Wed, May 29, 2019 at 11:43:52AM +0200, Niklas Cassel wrote:
> Currently, there is only a 1 ms sleep after asserting PERST.
> 
> Reading the datasheets for different endpoints, some require PERST to be
> asserted for 10 ms in order for the endpoint to perform a reset, others
> require it to be asserted for 50 ms.
> 
> Several SoCs using this driver uses PCIe Mini Card, where we don't know
> what endpoint will be plugged in.
> 
> The PCI Express Card Electromechanical Specification specifies:
> "On power up, the deassertion of PERST# is delayed 100 ms (TPVPERL) from
> the power rails achieving specified operating limits."
> 
> Add a sleep of 100 ms before deasserting PERST, in order to ensure that
> we are compliant with the spec.
> 
> Fixes: 82a823833f4e ("PCI: qcom: Add Qualcomm PCIe controller driver")
> Signed-off-by: Niklas Cassel <niklas.cassel@linaro.org>
> Acked-by: Stanimir Varbanov <svarbanov@mm-sol.com>
> Cc: stable@vger.kernel.org # 4.5+
> ---
> Changes since v1:
> Move the sleep into qcom_ep_reset_deassert()
> 
>  drivers/pci/controller/dwc/pcie-qcom.c | 2 ++
>  1 file changed, 2 insertions(+)

Applied to pci/qcom for v5.3, thanks.

Lorenzo

> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 0ed235d560e3..5d1713069d14 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -178,6 +178,8 @@ static void qcom_ep_reset_assert(struct qcom_pcie *pcie)
>  
>  static void qcom_ep_reset_deassert(struct qcom_pcie *pcie)
>  {
> +	/* Ensure that PERST has been asserted for at least 100 ms */
> +	msleep(100);
>  	gpiod_set_value_cansleep(pcie->reset, 0);
>  	usleep_range(PERST_DELAY_US, PERST_DELAY_US + 500);
>  }
> -- 
> 2.21.0
>