diff mbox series

PCI: j721e: Fix delay before PERST# deassert

Message ID 20230703112914.68806-1-a-verma1@ti.com
State New
Headers
Series PCI: j721e: Fix delay before PERST# deassert |

Commit Message

Achal Verma July 3, 2023, 11:29 a.m. UTC 
  As per the PCIe Card Electromechanical specification REV. 3.0, PERST#
signal should be de-asserted after minimum 100ms from the time power-rails
become stable. Current delay of 100us is observed to be not enough on some
custom platform implemented using TI's K3 SOCs.

So, to ensure 100ms delay to give sufficient time for power-rails and
refclk to become stable, change delay from 100us to 100ms.

From PCIe Card Electromechanical specification REV. 3.0 section 2.6.2:
TPVPERL: Power stable to PERST# inactive - 100ms
T-PERST-CLK: REFCLK stable before PERST# inactive - 100 usec.

Fixes: f3e25911a430 ("PCI: j721e: Add TI J721E PCIe driver")
Signed-off-by: Achal Verma <a-verma1@ti.com>
---
 drivers/pci/controller/cadence/pci-j721e.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

Comments

Li Chen July 3, 2023, 1:49 p.m. UTC | #1 
Hi Achal,
 ---- On Mon, 03 Jul 2023 19:29:14 +0800  Achal Verma  wrote --- 
 > As per the PCIe Card Electromechanical specification REV. 3.0, PERST#
 > signal should be de-asserted after minimum 100ms from the time power-rails
 > become stable. Current delay of 100us is observed to be not enough on some
 > custom platform implemented using TI's K3 SOCs.
 > 
 > So, to ensure 100ms delay to give sufficient time for power-rails and
 > refclk to become stable, change delay from 100us to 100ms.

What problems could arise if the delay is too small? Would some endpoints not be able to detect it?

Regards,
Li
Achal Verma July 3, 2023, 2:43 p.m. UTC | #2 
On 7/3/2023 7:19 PM, Li Chen wrote:
> Hi Achal,
>   ---- On Mon, 03 Jul 2023 19:29:14 +0800  Achal Verma  wrote ---
>   > As per the PCIe Card Electromechanical specification REV. 3.0, PERST#
>   > signal should be de-asserted after minimum 100ms from the time power-rails
>   > become stable. Current delay of 100us is observed to be not enough on some
>   > custom platform implemented using TI's K3 SOCs.
>   >
>   > So, to ensure 100ms delay to give sufficient time for power-rails and
>   > refclk to become stable, change delay from 100us to 100ms.
> 
> What problems could arise if the delay is too small? Would some endpoints not be able to detect it?
If delay is small, cpu stall is reported during probe() while accessing 
PCIe registers in some cases.
> 
> Regards,
> Li
Bjorn Helgaas July 3, 2023, 4:21 p.m. UTC | #3 
In subject, "Fix" doesn't convey much information.  Does it increase?
Decrease?  How much time are we talking about?  PERST# deassert is at
one end of the delay; what event is at the other end?

Some of these useful bits of information could appear in the subject
line.

On Mon, Jul 03, 2023 at 04:59:14PM +0530, Achal Verma wrote:
> As per the PCIe Card Electromechanical specification REV. 3.0, PERST#

I think the current rev of this spec is r5.0.  Can you cite that
instead?  I think the relevant section is r5.0, sec 2.9.2.

> signal should be de-asserted after minimum 100ms from the time power-rails
> become stable. Current delay of 100us is observed to be not enough on some
> custom platform implemented using TI's K3 SOCs.

Is this delay for the benefit of the Root Port or for the attached
Endpoint?  If the latter, my guess is that some Endpoints might
tolerate the current shorter delay, while others might require more,
and it doesn't sound like "TI's K3 SoC" would be relevant here.

> So, to ensure 100ms delay to give sufficient time for power-rails and
> refclk to become stable, change delay from 100us to 100ms.
> 
> From PCIe Card Electromechanical specification REV. 3.0 section 2.6.2:
> TPVPERL: Power stable to PERST# inactive - 100ms
> T-PERST-CLK: REFCLK stable before PERST# inactive - 100 usec.

Numbers like 100ms that come from the PCIe specs should have #defines
for them.  If we don't have one already, can you add one, please?

> Fixes: f3e25911a430 ("PCI: j721e: Add TI J721E PCIe driver")
> Signed-off-by: Achal Verma <a-verma1@ti.com>
> ---
>  drivers/pci/controller/cadence/pci-j721e.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pci/controller/cadence/pci-j721e.c b/drivers/pci/controller/cadence/pci-j721e.c
> index e70213c9060a..fa2b4c11d2c4 100644
> --- a/drivers/pci/controller/cadence/pci-j721e.c
> +++ b/drivers/pci/controller/cadence/pci-j721e.c
> @@ -499,13 +499,12 @@ static int j721e_pcie_probe(struct platform_device *pdev)
>  		/*
>  		 * "Power Sequencing and Reset Signal Timings" table in
>  		 * PCI EXPRESS CARD ELECTROMECHANICAL SPECIFICATION, REV. 3.0
> -		 * indicates PERST# should be deasserted after minimum of 100us
> -		 * once REFCLK is stable. The REFCLK to the connector in RC
> -		 * mode is selected while enabling the PHY. So deassert PERST#
> -		 * after 100 us.
> +		 * indicates PERST# should be deasserted after minimum of 100ms
> +		 * after power rails achieve specified operating limits and
> +		 * within this period reference clock should also become stable.
>  		 */
>  		if (gpiod) {
> -			usleep_range(100, 200);
> +			msleep(100);
>  			gpiod_set_value_cansleep(gpiod, 1);
>  		}
>  
> -- 
> 2.25.1
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Achal Verma July 4, 2023, 4:06 p.m. UTC | #4 
On 7/3/2023 9:51 PM, Bjorn Helgaas wrote:
> In subject, "Fix" doesn't convey much information.  Does it increase?
> Decrease?  How much time are we talking about?  PERST# deassert is at
> one end of the delay; what event is at the other end?
Hi Bjorn,

How about "Increase delay to 100ms for PERST# deassert from moment 
power-rails achieve operating limits"
> 
> Some of these useful bits of information could appear in the subject
> line.
> 
> On Mon, Jul 03, 2023 at 04:59:14PM +0530, Achal Verma wrote:
>> As per the PCIe Card Electromechanical specification REV. 3.0, PERST#
> 
> I think the current rev of this spec is r5.0.  Can you cite that
> instead?  I think the relevant section is r5.0, sec 2.9.2.
REV 5.0 also quote same TPVPERL=100ms delay.
I refer REV. 3.0 as pci-j721e controller follows REV. 3.0
> 
>> signal should be de-asserted after minimum 100ms from the time power-rails
>> become stable. Current delay of 100us is observed to be not enough on some
>> custom platform implemented using TI's K3 SOCs.
> 
> Is this delay for the benefit of the Root Port or for the attached
> Endpoint?  If the latter, my guess is that some Endpoints might
> tolerate the current shorter delay, while others might require more,
> and it doesn't sound like "TI's K3 SoC" would be relevant here.
Its for the endpoints, TI's EVB doesn't exhibit any issues with 100us 
delay but some customer reported the issue with shorter delay.
I have been working to refactor this driver to build as a module, I too
observed the issue on re-probe after remove when delay is lesser.
> 
>> So, to ensure 100ms delay to give sufficient time for power-rails and
>> refclk to become stable, change delay from 100us to 100ms.
>>
>>  From PCIe Card Electromechanical specification REV. 3.0 section 2.6.2:
>> TPVPERL: Power stable to PERST# inactive - 100ms
>> T-PERST-CLK: REFCLK stable before PERST# inactive - 100 usec.
> 
> Numbers like 100ms that come from the PCIe specs should have #defines
> for them.  If we don't have one already, can you add one, please?
Sure, will do it in next revision but should this go in some generic PCI 
header file or just pci-j721e.c
> 
>> Fixes: f3e25911a430 ("PCI: j721e: Add TI J721E PCIe driver")
>> Signed-off-by: Achal Verma <a-verma1@ti.com>
>> ---
>>   drivers/pci/controller/cadence/pci-j721e.c | 9 ++++-----
>>   1 file changed, 4 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/pci/controller/cadence/pci-j721e.c b/drivers/pci/controller/cadence/pci-j721e.c
>> index e70213c9060a..fa2b4c11d2c4 100644
>> --- a/drivers/pci/controller/cadence/pci-j721e.c
>> +++ b/drivers/pci/controller/cadence/pci-j721e.c
>> @@ -499,13 +499,12 @@ static int j721e_pcie_probe(struct platform_device *pdev)
>>   		/*
>>   		 * "Power Sequencing and Reset Signal Timings" table in
>>   		 * PCI EXPRESS CARD ELECTROMECHANICAL SPECIFICATION, REV. 3.0
>> -		 * indicates PERST# should be deasserted after minimum of 100us
>> -		 * once REFCLK is stable. The REFCLK to the connector in RC
>> -		 * mode is selected while enabling the PHY. So deassert PERST#
>> -		 * after 100 us.
>> +		 * indicates PERST# should be deasserted after minimum of 100ms
>> +		 * after power rails achieve specified operating limits and
>> +		 * within this period reference clock should also become stable.
>>   		 */
>>   		if (gpiod) {
>> -			usleep_range(100, 200);
>> +			msleep(100);
>>   			gpiod_set_value_cansleep(gpiod, 1);
>>   		}
>>   
>> -- 
>> 2.25.1
>>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Bjorn Helgaas July 5, 2023, 3:49 p.m. UTC | #5 
On Tue, Jul 04, 2023 at 09:36:43PM +0530, Verma, Achal wrote:
> On 7/3/2023 9:51 PM, Bjorn Helgaas wrote:
> > In subject, "Fix" doesn't convey much information.  Does it increase?
> > Decrease?  How much time are we talking about?  PERST# deassert is at
> > one end of the delay; what event is at the other end?
> 
> How about "Increase delay to 100ms for PERST# deassert from moment
> power-rails achieve operating limits"

Maybe something like "Delay 100ms T_PVPERL from power stable to PERST#
inactive" to match the language in the spec?

> > Is this delay for the benefit of the Root Port or for the attached
> > Endpoint?  If the latter, my guess is that some Endpoints might
> > tolerate the current shorter delay, while others might require
> > more, and it doesn't sound like "TI's K3 SoC" would be relevant
> > here.
>
> Its for the endpoints, TI's EVB doesn't exhibit any issues with
> 100us delay but some customer reported the issue with shorter delay.

I wouldn't bother mentioning "some custom platform implemented using
TI's K3 SOCs" then, because the problem is that the driver didn't
observe T_PVPERL, so the problem will happen with some endpoints but
not others.

> > Numbers like 100ms that come from the PCIe specs should have #defines
> > for them.  If we don't have one already, can you add one, please?
>
> Sure, will do it in next revision but should this go in some generic PCI
> header file or just pci-j721e.c

I think it should be in drivers/pci/pci.h so all the controller
drivers can use the same thing.  Obviously none of them *currently*
use it, although there are a bunch of "msleep(100)" and a few comments
that mention T_PVPERL.

Bjorn
diff mbox series

Patch

diff --git a/drivers/pci/controller/cadence/pci-j721e.c b/drivers/pci/controller/cadence/pci-j721e.c
index e70213c9060a..fa2b4c11d2c4 100644
--- a/drivers/pci/controller/cadence/pci-j721e.c
+++ b/drivers/pci/controller/cadence/pci-j721e.c
@@ -499,13 +499,12 @@  static int j721e_pcie_probe(struct platform_device *pdev)
 		/*
 		 * "Power Sequencing and Reset Signal Timings" table in
 		 * PCI EXPRESS CARD ELECTROMECHANICAL SPECIFICATION, REV. 3.0
-		 * indicates PERST# should be deasserted after minimum of 100us
-		 * once REFCLK is stable. The REFCLK to the connector in RC
-		 * mode is selected while enabling the PHY. So deassert PERST#
-		 * after 100 us.
+		 * indicates PERST# should be deasserted after minimum of 100ms
+		 * after power rails achieve specified operating limits and
+		 * within this period reference clock should also become stable.
 		 */
 		if (gpiod) {
-			usleep_range(100, 200);
+			msleep(100);
 			gpiod_set_value_cansleep(gpiod, 1);
 		}