Re: [PATCH v2] PCI: dra7xx: Fix reset behaviour

From: "Pali Rohár" <pali@kernel.org>
To: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: linus.walleij@linaro.org, kishon@ti.com,
	Luca Ceresoli <luca@lucaceresoli.net>,
	linux-pci@vger.kernel.org, linux-omap@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, Rob Herring <robh@kernel.org>,
	Bjorn Helgaas <bhelgaas@google.com>
Subject: Re: [PATCH v2] PCI: dra7xx: Fix reset behaviour
Date: Tue, 22 Jun 2021 14:16:49 +0200	[thread overview]
Message-ID: <20210622121649.ouiaecdvwutgdyy5@pali> (raw)
In-Reply-To: <20210622115604.GA25503@lpieralisi>

On Tuesday 22 June 2021 12:56:04 Lorenzo Pieralisi wrote:
> [Adding Linus for GPIO discussion, thread:
> https://lore.kernel.org/linux-pci/20210531090540.2663171-1-luca@lucaceresoli.net]
> 
> On Tue, Jun 22, 2021 at 01:06:27PM +0200, Pali Rohár wrote:
> > Hello!
> > 
> > On Tuesday 22 June 2021 12:57:22 Luca Ceresoli wrote:
> > > 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/

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.

> There are two things I'd like to see:
> 
> 1) If Linus can have a look at the GPIO bits in this thread that would
>    definitely help clarify any pending controversy
> 2) Kishon to test on *existing* platforms and confirm there are no
>    regressions triggered
> 
> > I would suggest to add a comment for call "usleep_range(1000, 2000);"
> > that you have chosen some "random" values which worked fine on your
> > setup and that they fix mentioned bug. Comment just to mark this sleep
> > code that is suboptimal / not-so-correct and to prevent other people to
> > copy+paste this code into other (new) drivers...
> 
> Yes a comment would help but as I say above I am afraid this is
> a platform specific set-up, ie that delay is somewhat tied to
> a platform, not sure there is anything we can do.
> 
> If Linus and Kishon are happy with the approach we can merge this
> patch.
> 
> Lorenzo