Hi, On 31/05/21 7:24 pm, Luca Ceresoli wrote: > Hi Pali, > > On 31/05/21 15:32, Pali Rohár wrote: >> On Monday 31 May 2021 11:05:40 Luca Ceresoli wrote: >>> The PCIe PERSTn reset pin is active low and should be asserted, then >>> deasserted. >>> >>> The current implementation only drives the pin once in "HIGH" position, >>> thus presumably it was intended to deassert the pin. This has two problems: >>> >>> 1) it assumes the pin was asserted by other means before loading the >>> driver >>> 2) it has the wrong polarity, since "HIGH" means "active", and the pin is >>> presumably configured as active low coherently with the PCIe >>> convention, thus it is driven physically to 0, keeping the device >>> under reset unless the pin is configured as active high. >>> >>> Fix both problems by: >>> >>> 1) keeping devm_gpiod_get_optional(dev, NULL, GPIOD_OUT_HIGH) as is, but >>> assuming the pin is correctly configured as "active low" this now >>> becomes a reset assertion >>> 2) adding gpiod_set_value(reset, 0) after a delay to deassert reset >>> >>> Fixes: 78bdcad05ea1 ("PCI: dra7xx: Add support to make GPIO drive PERST# line") >>> Signed-off-by: Luca Ceresoli <luca@xxxxxxxxxxxxxxxx> >>> >>> --- >>> >>> Changes v1 -> v2: >>> - No changes to the patch >>> - Reword commit message according to suggestions from Bjorn Helgaas (from >>> another patchset) >>> - Add Fixes: tag >>> --- >>> drivers/pci/controller/dwc/pci-dra7xx.c | 2 ++ >>> 1 file changed, 2 insertions(+) >>> >>> diff --git a/drivers/pci/controller/dwc/pci-dra7xx.c b/drivers/pci/controller/dwc/pci-dra7xx.c >>> index cb5d4c245ff6..11f392b7a9a2 100644 >>> --- a/drivers/pci/controller/dwc/pci-dra7xx.c >>> +++ b/drivers/pci/controller/dwc/pci-dra7xx.c >>> @@ -801,6 +801,8 @@ static int dra7xx_pcie_probe(struct platform_device *pdev) >>> dev_err(&pdev->dev, "gpio request failed, ret %d\n", ret); >>> goto err_gpio; >>> } >>> + usleep_range(1000, 2000); >> >> Hello! Just a note that this is again a new code pattern in another >> driver for different wait value of PCIe Warm Reset timeout. I sent email >> about these issues: >> https://lore.kernel.org/linux-pci/20210310110535.zh4pnn4vpmvzwl5q@pali/ >> >> Luca, how did you choose value 1000-2000 us? Do you have some reference >> or specification which says that this value needs to be used? > > Sadly I haven't access to the PCIe specification. > > I'd be very happy to know what a correct value should be and update my > patch. I had given the timing mentioned in the specification here https://lore.kernel.org/r/023c9b59-70bb-ed8d-a4c0-76eae726b574@xxxxxx The PCI EXPRESS CARD ELECTROMECHANICAL SPECIFICATION defines the Power Sequencing and Reset Signal Timings in Table 2-4. Please also refer Figure 2-10: Power Up of the CEM. ╔═════════════╤══════════════════════════════════════╤═════╤═════╤═══════╗ ║ Symbol │ Parameter │ Min │ Max │ Units ║ ╠═════════════╪══════════════════════════════════════╪═════╪═════╪═══════╣ ║ T PVPERL │ Power stable to PERST# inactive │ 100 │ │ ms ║ ╟─────────────┼──────────────────────────────────────┼─────┼─────┼───────╢ ║ T PERST-CLK │ REFCLK stable before PERST# inactive │ 100 │ │ μs ║ ╟─────────────┼──────────────────────────────────────┼─────┼─────┼───────╢ ║ T PERST │ PERST# active time │ 100 │ │ μs ║ ╟─────────────┼──────────────────────────────────────┼─────┼─────┼───────╢ ║ T FAIL │ Power level invalid to PERST# active │ │ 500 │ ns ║ ╟─────────────┼──────────────────────────────────────┼─────┼─────┼───────╢ ║ T WKRF │ WAKE# rise – fall time │ │ 100 │ ns ║ ╚═════════════╧══════════════════════════════════════╧═════╧═════╧═══════╝ The de-assertion of #PERST is w.r.t both power stable and refclk stable. I'm yet to validate this patch, but IIRC devm_gpiod_get_optional(dev, NULL, GPIOD_OUT_HIGH) will already de-assert the PERST line. Please note the board here can have various combinations of NOT gate before the gpio line is actually connected to the connector. Thanks Kishon