On Tuesday 22 June 2021 22:48:46 Pali Rohár wrote: > On Tuesday 22 June 2021 15:23:25 Lorenzo Pieralisi wrote: > > On Tue, Jun 22, 2021 at 02:16:49PM +0200, Pali Rohár wrote: > > > 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@xxxxxxxxxxxxxxxx] > > > > > > > > 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. > > > > Point taken but regardless this deviates from the PCI electromechanical > > specifications (ie T-PERST-CLK), does not it ? > > Well, I was not able to understand and decode PCIe base and PCIe CEM > specs to figure out which timeout value should be used. You wrote about > T-PERST-CLK but I'm really not sure if it is this one... Therefore I > cannot say if something deviates from spec or not. > > > I misused "platform" to > > define something that apparently is not contemplated by the PCI > > specifications (and I would like to understand why). > > > > I guess on ACPI systems (ie where the PERST# handling is implemented in > > FW) this is handled in BIOS/UEFI > > PCIe base spec does not define any standard interface for controlling > PCIe Warm Reset and PCIe CEM spec does not define any SW interface for > PERST# pin. So every board / computer with PCIe slot may connect PERST# > pin in different way to CPU. Some ARM boards connect all PERST# pins to > just one GPIO, and so via SW you can reset all PCIe cards at the same > time. No granularity to reset just one card. Some other connects all > PERST# pin to CPU reset output pin, so when CPU / board resets it cause > also reset of all PCIe cards. > > I read that some server machines have some dedicated device connected to > CPU via i2c/smbus, which controls PERST# pins for each PCIe slot > individually. And on these machines people use userspace i2cset > application to control PERST# and therefore can reset cards manually. > > If ACPI / BIOS / UEFI system has some kind of PCIe support && PERST# is > controller by software then for sure it needs to reset PCIe card (at > least putting it from reset state to normal) prior trying to read PCI > device/vendor ID from config space. > > > need to peruse the code to check how > > PERST# is handled and whether the delay is per host controller driver. > > Are there any open source implementations? Or we are just limited to > dump ACPI bytecode or BIOS / UEFI firmware and start reverse engineering > it? Because this would not be simple. > > And major problems with PCIe Warm Reset / PERST# signal I saw only on > boards where there is no BIOS / UEFI / ACPI; just native PCIe controller > drivers which talks directly to HW. > > I was not able to find any way how to control PERST# on any my x86 > laptop (standard setup with UEFI and ACPI). So I'm even not sure if on > x86 laptops is PERST# controllable by SW. I can imagine that this PIN > may be connected to some reset circuit from Embedded Controller which > may take full control of resetting card when it is needed at correct > time. > > So it is possible that code which controls PERST# on x86 does not have > to run on CPU and may be "burned" as part of other hardware... > > > > > > > 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 different things to agree on this patch > > 1) how GPIO drives PERST# > > I'm not sure what do you mean by this 1). GPIO is set to output > direction and can be either in low or high state. One of this states > represents RESET state on PERST# pin and which it is (low or high) is > defined by DTS (reset-gpio). > > So setting GPIO with output direction to value 1 (active) always puts > card into reset state and setting GPIO to value 0 (inactive) puts card > into normal state. Now I see what you mean. Some boards define in DTS that reset-gpio in inactive state puts card into reset state. Which contradicts my lines... > > 2) the PERST# de-assertion delay. > > This is open question. > > > I appreciate they are related and that Luca had to handle them together > > but logically they are separated "issues", it'd be great if we manage > > to nail down how they should be handled before we merge this code. > > > > Lorenzo > > > > > > > > > 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
