Re: [PATCH 0/3] PCI: rockchip: assert PERST# in S3

[Brian Norris]

Hi Bjorn,

On Thu, Oct 12, 2017 at 10:15:23PM -0500, Bjorn Helgaas wrote:
> On Thu, Oct 12, 2017 at 01:52:17PM -0700, Brian Norris wrote:
> > Hi,
> > 
> > This patch series should mostly be self-descriptive, but it's motivated by the
> > fact that I've found differing requirements from PCIe endpoint makers regarding
> > the state of PERST# when in system suspend (S3). Additionally, some existing
> > boards are not especially well suited for holding PERST# low in S3 (e.g., the
> > pin is driven by a non-PMU GPIO, so it's hard or impossible to keep it
> > asserted). So the solution is...give it a device tree property!
> 
> How do ACPI systems solve this problem?

I don't really know, because by design ACPI is rather opaque. And I
expect people like it that way.

But from the very little experience I've had looking at ACPI
configuration, this is done on a case-by-case basis. I've heard of cases
where an Intel-based S0ix suspend-to-idle can't reinitialize PCIe
devices reliably if PERST# was asserted and so...the firmware designers
just hacked around it by not asserting PERST# (there was no difference
in power consumption for this endpoint).

For the case I'm particularly looking at (the Wifi card vendor that
suggests not to assert PERST#)...I haven't tested them on an ACPI-based
system. But they claimed that their Windows/ACPI systems don't enter L2
or assert PERST# at all -- they stay in L1.x. But then, I'm not sure
this vendor has been very successful in the desktop/laptop market, where
ACPI is common.

But I'll elaborate a little on my problems on this non-ACPI system,
where apparently I have to defend things in the open, since they're not
hidden behind an opaque BIOS :)

I'm looking at 2 different Wifi card vendors, both in M.2-compliant
packages, and using them on Rockchip RK3399. We want to support
Wake-on-WLAN, and these cards do not have an "auxiliary" power, so we
leave them fully powered in system suspend (S3) and expect them to enter
low power states when prompted. Rockchip supports L0, L0s, L1, and L2/3
link states, but no L1.1 or L1.2. Its PCIe reference clock is driven off
a clock which powers off in S3, and so by my understanding, the only
legal transition is to enter L2/L3 link state before suspend.

Now, one of these 2 Wifi vendors claims higher power consumption with
PERST# asserted, because they exit L2 and go into a detect state. They
also seem to claim that on some Windows/ACPI systems leave their link in
L1.x states in system suspend, and PERST# is not involved there either.

The other vendor is the opposite: when PERST# is not asserted, power
consumption in L2 link state is higher, and (because device is not in D3
cold?) PCIE WAKE# will not activate. After a closer reading of the PCIe
specs, this seems to be a more correct reading of the specs.

> Can you point to any relevant sections in the PCI specs?

Sure, I can try.

Per the above discussion, we expect to enter L2 link state when
suspending. The PCIe Base Specification 3.1 section 5.3.2.3 (Entry into
the L2/L3 Ready State) describes the L2/L3 transition sequence,
involving the PME_Turn_Off and PME_TO_Ack messages. It doesn't describe
what to do with PERST#.

Separately, the PCI Express Card Electromechanical Specification
Revision 3.0 talks about PERST#. Its main description is in section 2:

"PERST# (required): indicates when the applied main power is within the
specified tolerance and stable."

Considering we always keep main power on for the endpoint, this doesn't
appear to suggest anything.

There is more language in 2.2, but it's less clear:

"PERST# is asserted in advance of the power being switched off in a
power-managed state like S3."

Again, no power is switched off. And what is a "state like S3"? Rather
vague. The "S3" I refer to for RK3399 is not determined by an ACPI spec.

The PCIe M.2 specification is even lighter on details. It describes
PERST# but only in the context of powering off. Although it does refer
to the above PCIe Card EM Spec at times.

> Is this a hole in those specs?  Is this something that needs to be
> clarified by the PCI-SIG to improve interoperability?

After re-re-reading the specifications, I'm more convinced that the
first Wifi vendor got it wrong. I also don't trust them to get many
things right in general, so this is pretty much par for the course.

The only things I can suggest:
* "main power" is never defined, as far as I can tell. So "main" and
  "auxiliary" power don't have much meaning for many M.2 cards, and so
  I end up reading much of the spec with a grain of salt
* S3 is never defined in the PCIe Card EM spec, but it's thrown around a
  few times
* if possible, the PCIe base spec should mention something about the
  fundamental reset which is expected with an L2 transition. It's not
  clear what to do if you don't want to switch off power completely (and
  so enter L3), but you also don't have "auxiliary" power

> Why is this problem only showing up now?

Because initial work on RK3399 (and most upstreaming efforts) was done
on boards using the first Wifi vendor as the sole PCIe device on the
bus, and now we're introducing a second Wifi vendor, which is more
closely aligned with the PCIe Card EM spec.

Also, even if the first vendors' chips could handle PERST# OK, the first
boards may not be able to drive PERST# low in S3, as they'll get pulled
up when the GPIO bank shuts off. (It's unclear if just toggling PERST#
is enough to satisfy the spec.)

As to why no one else ran into this, I can only speculate. Maybe our 1st
Wifi vendor is the only vendor that got this wrong. Or maybe anyone who
has handled this stuff just handles it in custom BIOS code that nobody
ever reviewed.

Brian

> > Brian Norris (3):
> >   Documentation/devicetree: Add pcie-reset-suspend property
> >   of/pci: Add of_pci_get_pcie_reset_suspend() to parse
> >     pcie-reset-suspend
> >   PCI: rockchip: Support configuring PERST# state via DT
> > 
> >  Documentation/devicetree/bindings/pci/pci.txt | 11 +++++++++++
> >  drivers/of/of_pci.c                           | 25 +++++++++++++++++++++++++
> >  drivers/pci/host/pcie-rockchip.c              |  7 +++++++
> >  include/linux/of_pci.h                        |  7 +++++++
> >  4 files changed, 50 insertions(+)
> > 
> > -- 
> > 2.15.0.rc0.271.g36b669edcc-goog
> > 
> > 
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel