On Tuesday 22 June 2021 23:08:07 Luca Ceresoli wrote:

On 22/06/21 22:52, Pali Rohár wrote:

quoted

On Tuesday 22 June 2021 19:27:37 Kishon Vijay Abraham I wrote:

quoted

Hi Luca, Pali,

On 22/06/21 7:01 pm, Luca Ceresoli wrote:

quoted

Hi,

On 22/06/21 14:16, Pali Rohár wrote:

quoted

On Tuesday 22 June 2021 12:56:04 Lorenzo Pieralisi wrote:

quoted

[Adding Linus for GPIO discussion, thread:
https://lore.kernel.org/linux-pci/20210531090540.2663171-1-luca@lucaceresoli.net (local)]

On Tue, Jun 22, 2021 at 01:06:27PM +0200, Pali Rohár wrote:

quoted

Hello!

On Tuesday 22 June 2021 12:57:22 Luca Ceresoli wrote:

quoted

Nothing happened after a few weeks... I understand that knowing the
correct reset timings is relevant, but unfortunately I cannot help much
in finding out the correct values.

However I'm wondering what should happen to this patch. It *does* fix a
real bug, but potentially with an incorrect or non-optimal usleep range.
Do we really want to ignore a bugfix because we are not sure about how
long this delay should be?

As there is no better solution right now, I'm fine with your patch. But
patch needs to be approved by Lorenzo, so please wait for his final
answer.

I am not a GPIO expert and I have a feeling this is platform specific
beyond what the PCI specification can actually define architecturally.

In my opinion timeout is not platform specific as I wrote in email:
https://lore.kernel.org/linux-pci/20210310110535.zh4pnn4vpmvzwl5q@pali/ (local)

My experiments already proved that some PCIe cards needs to be in reset
state for some minimal time otherwise they cannot be enumerated. And it
does not matter to which platform you connect those (endpoint) cards.

I do not think that timeout itself is platform specific. GPIO controls
PERST# pin and therefore specified sleep value directly drives how long
is card on the other end of PCIe slot in Warm Reset state. PCIe CEM spec
directly says that PERST# signal controls PCIe Warm Reset.

What is here platform specific thing is that PERST# signal is controlled
by GPIO. But value of signal (high / low) and how long is in signal in
which state for me sounds like not an platform specific thing, but as
PCIe / CEM related.

That's exactly my understanding of this matter. At least for the dra7xx
controller it works exactly like this, PERSTn# is nothing but a GPIO
output from the SoC that drives the PERSTn# input of the external chip
without affecting the controller directly.

While the patch itself is correct, this kind-of changes the behavior on
already upstreamed platforms. Previously the driver expected #PERST to
be asserted be external means (or default power-up state) and only takes
care of de-asserting the #PERST line.

There are 2 platforms that will be impacted due to this change
1) arch/arm/boot/dts/am57xx-beagle-x15-common.dtsi (has an inverter on
GPIO line)
2) arch/arm/boot/dts/am571x-idk.dts (directly connected to #PERST)

For 1), gpiod_set_value(reset, 0) will assert the PERST line due to the
inverter (and GPIO_ACTIVE_LOW)
For 2), gpiod_set_value(reset, 0) will assert the PERST line because we
have GPIO_ACTIVE_HIGH

Ou! This is a problem in DT. It needs to be defined in a way that state
is same for every DTS device which uses this driver.

Why?

I'm starting to be confused by triple or more negations (asserting,
signal inverter, active low)...

In your patch is GPIO set value to 0 and Kishon wrote that GPIO set
value to 0 for those two boards assert PERST# line. Asserting PERST#
line cause endpoint PCIe card to be in reset state. And in pci-dra7xx.c
driver there is no other code which de-asserts PERST# line.

So based on all this information I deduced that your patch will cause
putting PCIe cards into reset state (forever) and therefore they would
not work.

Or do I have here some mistake?

These are different boards and each specifies its own polarity.
They are already opposite to each other right now:

https://elixir.bootlin.com/linux/v5.13-rc7/source/arch/arm/boot/dts/am57xx-beagle-x15-common.dtsi#L602

https://elixir.bootlin.com/linux/v5.13-rc7/source/arch/arm/boot/dts/am571x-idk.dts#L196

-- 
Luca

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel