Re: [EXTERNAL] Re: [PATCH v3] PCI: j721e: Delay 100ms T_PVPERL from power stable to PERST# inactive
From: Verma, Achal
Date: Wed Aug 02 2023 - 05:08:29 EST
On 7/18/2023 9:25 PM, Bjorn Helgaas wrote:
On Fri, Jul 07, 2023 at 03:21:19PM +0530, Achal Verma wrote:
As per the PCIe Card Electromechanical specification REV. 5.0, PERST#
signal should be de-asserted after minimum 100ms from the time power-rails
become stable. 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. 5.0 section 2.9.2:
TPVPERL: Power stable to PERST# inactive - 100ms
Fixes: f3e25911a430 ("PCI: j721e: Add TI J721E PCIe driver")
Signed-off-by: Achal Verma <a-verma1@xxxxxx>
---
Changes from v2:
* Fix commit message.
Change from v1:
* Add macro for delay value.
drivers/pci/controller/cadence/pci-j721e.c | 11 +++++------
drivers/pci/pci.h | 2 ++
2 files changed, 7 insertions(+), 6 deletions(-)
diff --git a/drivers/pci/controller/cadence/pci-j721e.c b/drivers/pci/controller/cadence/pci-j721e.c
index e70213c9060a..32b6a7dc3cff 100644
--- a/drivers/pci/controller/cadence/pci-j721e.c
+++ b/drivers/pci/controller/cadence/pci-j721e.c
@@ -498,14 +498,13 @@ 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.
+ * PCI EXPRESS CARD ELECTROMECHANICAL SPECIFICATION, REV. 5.0
+ * 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.
I think the problem is not that the current code is *wrong*, because
we do need to observe T_PERST-CLK, but that it failed to *also*
account for T_PVPERL.
There are two delays before deasserting PERST#:
T_PVPERL: delay after power becomes stable
T_PERST-CLK: delay after REFCLK becomes stable
I assume power is enabled by phy_power_on(), and REFCLK is enabled by
clk_prepare_enable():
cdns_pcie_init_phy
cdns_pcie_enable_phy
phy_power_on <-- power becomes stable
clk_prepare_enable <-- REFCLK becomes stable
if (gpiod)
usleep_range
gpiod_set_value_cansleep(gpiod, 1) <-- deassert PERST#
I don't actually know if phy_power_on() guarantees that power is
stable before it returns. But I guess that's our assumption?
Similarly for clk_prepare_enable().
In any case, we have to observe both delays. They overlap, and
T_PVPERL is 1000 times longer than T_PERST-CLK, so there might be
enough slop in an msleep(100) to cover both, but I think I would do
the simple-minded:
msleep(PCIE_TPVPERL_MS);
usleep_range(PCIE_TPERST_CLK_US, 2 * PCIE_TPERST_CLK_US);
I think adding 100us more is not required since as you said and as also
mentioned in CEM spec, 100ms covers for both power rails and refclock to
get stable and 2 consecutive sleep call looks different to me.
But if still required (please let me know), will do the suggested
change, along with other fixes you asked below.
This is slightly more conservative than necessary because they
overlap, but at least it shows that we thought about both of them.
if (gpiod) {
- usleep_range(100, 200);
+ msleep(PCIE_TPVPERL_DELAY_MS);
gpiod_set_value_cansleep(gpiod, 1);
I wish this local variable were named something like "perst_gpiod"
instead of "gpiod". We already know from its use in
gpiod_set_value_cansleep() that it's a GPIO. What's NOT obvious from
the context is that this is the PERST# signal.
sure will change variable name to perst_gpiod.
Tangent: it looks like the DT "reset" property that I'm assuming
controls PERST# is optional. How do we enforce these delays if that
property is missing?
yes gpiod_get shouldn't be optional, will fix this too.
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index a4c397434057..6ab2367e5867 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -13,6 +13,8 @@
#define PCIE_LINK_RETRAIN_TIMEOUT_MS 1000
+#define PCIE_TPVPERL_DELAY_MS 100 /* see PCIe CEM r5.0, sec 2.9.2 */
+
extern const unsigned char pcie_link_speed[];
extern bool pci_early_dump;
--
2.25.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel