diff mbox series

[v2] PCI: dra7xx: Fix reset behaviour

Message ID 20210531090540.2663171-1-luca@lucaceresoli.net (mailing list archive)
State New
Delegated to: Lorenzo Pieralisi
Headers show
Series [v2] PCI: dra7xx: Fix reset behaviour | expand

Commit Message

Luca Ceresoli May 31, 2021, 9:05 a.m. UTC
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@lucaceresoli.net>

---

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(+)

Comments

Pali Rohár May 31, 2021, 1:32 p.m. UTC | #1
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@lucaceresoli.net>
> 
> ---
> 
> 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?

> +	gpiod_set_value(reset, 0);
>  
>  	reg = dra7xx_pcie_readl(dra7xx, PCIECTRL_DRA7XX_CONF_DEVICE_CMD);
>  	reg &= ~LTSSM_EN;
> -- 
> 2.25.1
>
Luca Ceresoli May 31, 2021, 1:54 p.m. UTC | #2
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@lucaceresoli.net>
>>
>> ---
>>
>> 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.
Kishon Vijay Abraham I May 31, 2021, 4 p.m. UTC | #3
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@lucaceresoli.net>
>>>
>>> ---
>>>
>>> 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@ti.com

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
Pali Rohár May 31, 2021, 4:22 p.m. UTC | #4
Hello Kishon!

On Monday 31 May 2021 21:30:30 Kishon Vijay Abraham I wrote:
> I had given the timing mentioned in the specification here
> https://lore.kernel.org/r/023c9b59-70bb-ed8d-a4c0-76eae726b574@ti.com
> 
> 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 think this does not fully answer this problematic question. One thing
is initial power on and second thing is warm reset (when both power and
clock is stable).

On more ARM boards, power is not SW controllable and is automatically
enabled when powering board on. So Tₚᵥₚₑᵣₗ is calculated since
bootloader and therefore not needed to take into account in kernel.

Tₚₑᵣₛₜ₋cₗₖ is only 100 µs and experiments proved that 100 µs not enough
for toggling PERST# GPIO. At least one 1 ms is needed and for some cards
at least 10 ms. Otherwise cards are not detected.

So when you have both power and clock stable and you want to reset card
via PERST# signal, above table does not say how long it is needed to
have PERST# in reset state.

> 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
Luca Ceresoli June 1, 2021, 9:03 a.m. UTC | #5
Hi Kishon,

On 31/05/21 18:00, Kishon Vijay Abraham I wrote:
> 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@lucaceresoli.net>
>>>>
>>>> ---
>>>>
>>>> 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@ti.com
> 
> 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. 

Perhaps in all the cases you faced, but GPIOD_OUT_HIGH [0] really means
"active", not "electrically high", and here we want reset to be
deasserted (=deactivated), not asserted (=activated).

I guess it works when the GPIO drives PERSTn without inversion (no NOT
gates or an even number of NOT gates) _and_ device tree does specify the
GPIO as active high (which is incorrect: PERSTn is active low).

> Please note
> the board here can have various combinations of NOT gate before the
> gpio
> line is actually connected to the connector.

Exactly for this reason a portable driver must never drive the signal
"electrically low" or "electrically high". That's why with my patch I
propose to give the proper interpretation [1] to GPIOD_OUT_HIGH, i.e.
"active", i.e. "reset asserted". Device tree will describe if active
means electrically low (no NOT gates between GPIO pin and device PERSTn
pin) or high (odd number of NOT gates).

Additionally, as per patch description, even in the cases where the
driver deasserts the reset, it does not assert it. Should the signal be
asserted before dra7xx_pcie_probe(), devm_gpiod_get_optional(dev, NULL,
GPIOD_OUT_HIGH) would not move the line and thus would not reset the device.

The only problem I can imagine with my patch is with existing code. If
you have a board with the reset GPIO described as active high in DT
while it is active low (no/even NOR gates on board), then you should
apply this patch _and_ fix the board device tree.

I hope the intent of the patch is clearer now.

[0]
https://www.kernel.org/doc/html/latest/driver-api/gpio/consumer.html#obtaining-and-disposing-gpios
[1]
https://www.kernel.org/doc/html/latest/driver-api/gpio/consumer.html#the-active-low-and-open-drain-semantics
diff mbox series

Patch

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);
+	gpiod_set_value(reset, 0);
 
 	reg = dra7xx_pcie_readl(dra7xx, PCIECTRL_DRA7XX_CONF_DEVICE_CMD);
 	reg &= ~LTSSM_EN;