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

[PATCH] 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.

Signed-off-by: Niklas Cassel <niklas.cassel@linaro.org>
---
 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..cae24376237c 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -1110,6 +1110,8 @@ static int qcom_pcie_host_init(struct pcie_port *pp)
 	if (IS_ENABLED(CONFIG_PCI_MSI))
 		dw_pcie_msi_init(pp);
 
+	/* Ensure that PERST has been asserted for at least 100 ms */
+	msleep(100);
 	qcom_ep_reset_deassert(pcie);
 
 	ret = qcom_pcie_establish_link(pcie);
-- 
2.21.0

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

[Marc Gonzalez]

On 23/05/2019 21:44, 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.
> 
> Signed-off-by: Niklas Cassel <niklas.cassel@linaro.org>
> ---
>  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..cae24376237c 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -1110,6 +1110,8 @@ static int qcom_pcie_host_init(struct pcie_port *pp)
>  	if (IS_ENABLED(CONFIG_PCI_MSI))
>  		dw_pcie_msi_init(pp);
>  
> +	/* Ensure that PERST has been asserted for at least 100 ms */
> +	msleep(100);
>  	qcom_ep_reset_deassert(pcie);
>  
>  	ret = qcom_pcie_establish_link(pcie);

Currently, qcom_ep_reset_assert() and qcom_ep_reset_deassert() both include
a call to usleep_range() of 1.0 to 1.5 ms

Can we git rid of both if we sleep 100 ms before qcom_ep_reset_deassert?

Should the msleep() call be included in one of the two wrappers?

Regards.

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

[Bjorn Andersson]

On Thu 23 May 12:44 PDT 2019, 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.
> 
> Signed-off-by: Niklas Cassel <niklas.cassel@linaro.org>

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

> ---
>  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..cae24376237c 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -1110,6 +1110,8 @@ static int qcom_pcie_host_init(struct pcie_port *pp)
>  	if (IS_ENABLED(CONFIG_PCI_MSI))
>  		dw_pcie_msi_init(pp);
>  
> +	/* Ensure that PERST has been asserted for at least 100 ms */
> +	msleep(100);
>  	qcom_ep_reset_deassert(pcie);
>  
>  	ret = qcom_pcie_establish_link(pcie);
> -- 
> 2.21.0
> 

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

[Niklas Cassel]

On Fri, May 24, 2019 at 02:43:00PM +0200, Marc Gonzalez wrote:
> On 23/05/2019 21:44, 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.
> > 
> > Signed-off-by: Niklas Cassel <niklas.cassel@linaro.org>
> > ---
> >  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..cae24376237c 100644
> > --- a/drivers/pci/controller/dwc/pcie-qcom.c
> > +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> > @@ -1110,6 +1110,8 @@ static int qcom_pcie_host_init(struct pcie_port *pp)
> >  	if (IS_ENABLED(CONFIG_PCI_MSI))
> >  		dw_pcie_msi_init(pp);
> >  
> > +	/* Ensure that PERST has been asserted for at least 100 ms */
> > +	msleep(100);
> >  	qcom_ep_reset_deassert(pcie);
> >  
> >  	ret = qcom_pcie_establish_link(pcie);
> 
> Currently, qcom_ep_reset_assert() and qcom_ep_reset_deassert() both include
> a call to usleep_range() of 1.0 to 1.5 ms
> 
> Can we git rid of both if we sleep 100 ms before qcom_ep_reset_deassert?

These two sleeps after asserting/deasserting reset in qcom_ep_reset_assert()/
qcom_ep_reset_deassert() matches the sleeps in:
https://source.codeaurora.org/quic/la/kernel/msm-4.14/tree/drivers/pci/host/pci-msm.c?h=LA.UM.7.1.r1-14000-sm8150.0#n1942

and

https://source.codeaurora.org/quic/la/kernel/msm-4.14/tree/drivers/pci/host/pci-msm.c?h=LA.UM.7.1.r1-14000-sm8150.0#n1949

I would rather not remove these since that might affect existing devices.


This new sleep matches matches the sleep in:
https://source.codeaurora.org/quic/la/kernel/msm-4.14/tree/drivers/pci/host/pci-msm.c?h=LA.UM.7.1.r1-14000-sm8150.0#n3926

> 
> Should the msleep() call be included in one of the two wrappers?

This new sleep could be moved into qcom_ep_reset_deassert(),
added before the gpiod_set_value_cansleep(pcie->reset, 0) call,
if Stanimir prefers it to be placed there instead.


Kind regards,
Niklas

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

[Stanimir Varbanov]

Hi Niklas,


On 27.05.19 г. 20:15 ч., Niklas Cassel wrote:
> On Fri, May 24, 2019 at 02:43:00PM +0200, Marc Gonzalez wrote:
>> On 23/05/2019 21:44, 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.
>>>
>>> Signed-off-by: Niklas Cassel <niklas.cassel@linaro.org>
>>> ---
>>>   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..cae24376237c 100644
>>> --- a/drivers/pci/controller/dwc/pcie-qcom.c
>>> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
>>> @@ -1110,6 +1110,8 @@ static int qcom_pcie_host_init(struct pcie_port *pp)
>>>   	if (IS_ENABLED(CONFIG_PCI_MSI))
>>>   		dw_pcie_msi_init(pp);
>>>   
>>> +	/* Ensure that PERST has been asserted for at least 100 ms */
>>> +	msleep(100);
>>>   	qcom_ep_reset_deassert(pcie);
>>>   
>>>   	ret = qcom_pcie_establish_link(pcie);
>>
>> Currently, qcom_ep_reset_assert() and qcom_ep_reset_deassert() both include
>> a call to usleep_range() of 1.0 to 1.5 ms
>>
>> Can we git rid of both if we sleep 100 ms before qcom_ep_reset_deassert?
> 
> These two sleeps after asserting/deasserting reset in qcom_ep_reset_assert()/
> qcom_ep_reset_deassert() matches the sleeps in:
> https://source.codeaurora.org/quic/la/kernel/msm-4.14/tree/drivers/pci/host/pci-msm.c?h=LA.UM.7.1.r1-14000-sm8150.0#n1942
> 
> and
> 
> https://source.codeaurora.org/quic/la/kernel/msm-4.14/tree/drivers/pci/host/pci-msm.c?h=LA.UM.7.1.r1-14000-sm8150.0#n1949
> 
> I would rather not remove these since that might affect existing devices.
> 
> 
> This new sleep matches matches the sleep in:
> https://source.codeaurora.org/quic/la/kernel/msm-4.14/tree/drivers/pci/host/pci-msm.c?h=LA.UM.7.1.r1-14000-sm8150.0#n3926
> 
>>
>> Should the msleep() call be included in one of the two wrappers?
> 
> This new sleep could be moved into qcom_ep_reset_deassert(),
> added before the gpiod_set_value_cansleep(pcie->reset, 0) call,
> if Stanimir prefers it to be placed there instead.

yes, please move the sleep in qcom_ep_reset_deassert()

with that:

Acked-by: Stanimir Varbanov <svarbanov@mm-sol.com>

regards,
Stan