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

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

[Brian Norris]

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!

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

[PATCH 1/3] Documentation/devicetree: Add pcie-reset-suspend property

[Brian Norris]

The patch is self-descriptive. I've found that we may need
platform-specific behavior for the PERST# signal in system suspend,
depending on the type of PCIe endpoints are attached.

Signed-off-by: Brian Norris <briannorris@chromium.org>
---
 Documentation/devicetree/bindings/pci/pci.txt | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/Documentation/devicetree/bindings/pci/pci.txt b/Documentation/devicetree/bindings/pci/pci.txt
index c77981c5dd18..91339b6d0652 100644
--- a/Documentation/devicetree/bindings/pci/pci.txt
+++ b/Documentation/devicetree/bindings/pci/pci.txt
@@ -24,3 +24,14 @@ driver implementation may support the following properties:
    unsupported link speed, for instance, trying to do training for
    unsupported link speed, etc.  Must be '4' for gen4, '3' for gen3, '2'
    for gen2, and '1' for gen1. Any other values are invalid.
+- pcie-reset-suspend:
+   If present this property defines whether the PCIe Reset signal (referred to
+   as PERST#) should be asserted when the system enters low-power suspend modes
+   (e.g., S3). Depending on the form factor, the associated PCIe
+   electromechanical specification may specify a particular behavior (e.g.,
+   "PERST# is asserted in advance of the power being switched off in a
+   power-managed state like S3") or it may be less clear. The net result is
+   that some endpoints perform better (e.g., lower power consumption) with
+   PERST# asserted, and others prefer PERST# deasserted. The value must be '0'
+   or '1', where '0' means do not assert PERST# and '1' means assert PERST#.
+   When absent, behavior may be platform-specific.
-- 
2.15.0.rc0.271.g36b669edcc-goog

[PATCH 2/3] of/pci: Add of_pci_get_pcie_reset_suspend() to parse pcie-reset-suspend

[Brian Norris]

This helper can be used by drivers to determine the expected PERST#
behavior when in low-power system suspend (e.g., S3). I've found the
expected behavior to vary across a few different endpoint
implementations.

Signed-off-by: Brian Norris <briannorris@chromium.org>
---
 drivers/of/of_pci.c    | 25 +++++++++++++++++++++++++
 include/linux/of_pci.h |  7 +++++++
 2 files changed, 32 insertions(+)

diff --git a/drivers/of/of_pci.c b/drivers/of/of_pci.c
index e9ec931f5b9a..ab2586f86094 100644
--- a/drivers/of/of_pci.c
+++ b/drivers/of/of_pci.c
@@ -137,6 +137,31 @@ int of_pci_get_max_link_speed(struct device_node *node)
 }
 EXPORT_SYMBOL_GPL(of_pci_get_max_link_speed);
 
+/**
+ * This function returns true if the PCIe controller should assert PCIe Reset
+ * (PERST#) when entering system suspend (e.g., S3).
+ *
+ * @node: device tree node with the reset property
+ *
+ * Returns 1 (meaning "assert reset") or 0 ("don't assert reset"), if the DT
+ * defined a valid behavior. Otherwise, returns a negative error code.
+ */
+int of_pci_get_pcie_reset_suspend(struct device_node *node)
+{
+	int ret;
+	u32 val;
+
+	ret = of_property_read_u32(node, "pcie-reset-suspend", &val);
+	if (ret)
+		return ret;
+
+	if (val > 1)
+		return -EINVAL;
+
+	return val;
+}
+EXPORT_SYMBOL_GPL(of_pci_get_pcie_reset_suspend);
+
 /**
  * of_pci_check_probe_only - Setup probe only mode if linux,pci-probe-only
  *                           is present and valid
diff --git a/include/linux/of_pci.h b/include/linux/of_pci.h
index 518c8d20647a..df453893080b 100644
--- a/include/linux/of_pci.h
+++ b/include/linux/of_pci.h
@@ -17,6 +17,7 @@ int of_irq_parse_and_map_pci(const struct pci_dev *dev, u8 slot, u8 pin);
 int of_pci_parse_bus_range(struct device_node *node, struct resource *res);
 int of_get_pci_domain_nr(struct device_node *node);
 int of_pci_get_max_link_speed(struct device_node *node);
+int of_pci_get_pcie_reset_suspend(struct device_node *node);
 void of_pci_check_probe_only(void);
 int of_pci_map_rid(struct device_node *np, u32 rid,
 		   const char *map_name, const char *map_mask_name,
@@ -69,6 +70,12 @@ of_pci_get_max_link_speed(struct device_node *node)
 	return -EINVAL;
 }
 
+static inline int
+of_pci_get_pcie_reset_suspend(struct device_node *node)
+{
+	return -EINVAL;
+}
+
 static inline void of_pci_check_probe_only(void) { }
 #endif
 
-- 
2.15.0.rc0.271.g36b669edcc-goog

[PATCH 3/3] PCI: rockchip: Support configuring PERST# state via DT

[Brian Norris]

I've found that different endpoints and board configurations have
required different behavior from the PCIe Reset (PERST#) signal when in
low-power system suspend (e.g., S3). Use the new of_pci helper to
request this state and assert (active low) PERST# before suspending.

Note that we reinitialize the link (including reconfiguring PERST#) at
resume time.

This requires that the board and system firmware supports driving this
signal low when the system is suspended, since PERST# may be pulled up
by the endpoint, and some GPIO banks are not active in S3.

Signed-off-by: Brian Norris <briannorris@chromium.org>
---
 drivers/pci/host/pcie-rockchip.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/pci/host/pcie-rockchip.c b/drivers/pci/host/pcie-rockchip.c
index 9051c6c8fea4..1ab58c1abb34 100644
--- a/drivers/pci/host/pcie-rockchip.c
+++ b/drivers/pci/host/pcie-rockchip.c
@@ -233,6 +233,7 @@ struct rockchip_pcie {
 	struct	regulator *vpcie1v8; /* 1.8V power supply */
 	struct	regulator *vpcie0v9; /* 0.9V power supply */
 	struct	gpio_desc *ep_gpio;
+	bool	suspend_reset;
 	u32	lanes;
 	u8      lanes_map;
 	u8	root_bus_nr;
@@ -1155,6 +1156,9 @@ static int rockchip_pcie_parse_dt(struct rockchip_pcie *rockchip)
 		dev_info(dev, "no vpcie0v9 regulator found\n");
 	}
 
+	/* Default not-asserted, to retain backward compatibility. */
+	rockchip->suspend_reset = of_pci_get_pcie_reset_suspend(node) > 0;
+
 	return 0;
 }
 
@@ -1463,6 +1467,9 @@ static int __maybe_unused rockchip_pcie_suspend_noirq(struct device *dev)
 		return ret;
 	}
 
+	if (rockchip->suspend_reset)
+		gpiod_set_value(rockchip->ep_gpio, 0);
+
 	rockchip_pcie_deinit_phys(rockchip);
 
 	rockchip_pcie_disable_clocks(rockchip);
-- 
2.15.0.rc0.271.g36b669edcc-goog

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

[Doug Anderson]

Hi,

On Thu, Oct 12, 2017 at 1:52 PM, Brian Norris <briannorris@chromium.org> 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!
>
> 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(+)

I'm nowhere near an expert on PCIe, but overall the series seems sane
to me.  ...and I'd agree that the discussions I've seen from different
vendors seem to have different opinions about what we should do with
this line in S3.  ...so FWIW:

Reviewed-by: Douglas Anderson <dianders@chromium.org>

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

[Bjorn Helgaas]

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?

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

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

Why is this problem only showing up now?

> 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 at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[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 at lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/3] Documentation/devicetree: Add pcie-reset-suspend property

[Bjorn Helgaas]

[+cc Doug]

On Thu, Oct 12, 2017 at 01:52:18PM -0700, Brian Norris wrote:
> The patch is self-descriptive. I've found that we may need
> platform-specific behavior for the PERST# signal in system suspend,
> depending on the type of PCIe endpoints are attached.
> 
> Signed-off-by: Brian Norris <briannorris@chromium.org>
> ---
>  Documentation/devicetree/bindings/pci/pci.txt | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/pci/pci.txt b/Documentation/devicetree/bindings/pci/pci.txt
> index c77981c5dd18..91339b6d0652 100644
> --- a/Documentation/devicetree/bindings/pci/pci.txt
> +++ b/Documentation/devicetree/bindings/pci/pci.txt
> @@ -24,3 +24,14 @@ driver implementation may support the following properties:
>     unsupported link speed, for instance, trying to do training for
>     unsupported link speed, etc.  Must be '4' for gen4, '3' for gen3, '2'
>     for gen2, and '1' for gen1. Any other values are invalid.
> +- pcie-reset-suspend:
> +   If present this property defines whether the PCIe Reset signal (referred to
> +   as PERST#) should be asserted when the system enters low-power suspend modes
> +   (e.g., S3). Depending on the form factor, the associated PCIe
> +   electromechanical specification may specify a particular behavior (e.g.,
> +   "PERST# is asserted in advance of the power being switched off in a
> +   power-managed state like S3") or it may be less clear. The net result is
> +   that some endpoints perform better (e.g., lower power consumption) with
> +   PERST# asserted, and others prefer PERST# deasserted. The value must be '0'
> +   or '1', where '0' means do not assert PERST# and '1' means assert PERST#.
> +   When absent, behavior may be platform-specific.

I don't understand this at all.  Are you suggesting that we need
different "pcie-reset-suspend" values based on what sort of endpoint
the user plugs in?

If so, I really don't want to get involved in that, because that's an
issue that needs to be resolved by the vendors and the PCI-SIG.  If we
put in a tweak like this, I think it would be a band-aid that only
delays getting a real solution figured out.

If you want a quirk to tune this based on specific devices, that might
make sense.  It would still sound like an interoperability issue and
an ongoing maintenance problem, but at least we would have specific
details about which devices are involved, and we'd have a chance to
make them work on more controllers than just Rockchip.

Bjorn

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

[Brian Norris]

Hi Bjorn,

One piece of information to add:

On Thu, Oct 12, 2017 at 11:27 PM, Brian Norris <briannorris@chromium.org> wrote:
> On Thu, Oct 12, 2017 at 10:15:23PM -0500, Bjorn Helgaas wrote:
> > 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

This part is where the first vendor seems to disagree. They claim that
because power is not switched off, PERST# can remain asserted or
deasserted -- whichever leads to least power consumption. (i.e., they
don't consider they need to follow the parts that describe "when
removing main power" (e.g., in S3) we must assert PERST#.

> * 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

Brian

[PATCH 1/3] Documentation/devicetree: Add pcie-reset-suspend property

[Brian Norris]

Hi Bjorn,

Sorry for a little delay. I haven't been able to get much better answers
out of the problematic vendor here (they insist they are within the
spec), so I guess I'll have to go ahead based on current knowledge.

On Fri, Oct 13, 2017 at 11:51:52AM -0500, Bjorn Helgaas wrote:
> [+cc Doug]
> 
> On Thu, Oct 12, 2017 at 01:52:18PM -0700, Brian Norris wrote:
> > The patch is self-descriptive. I've found that we may need
> > platform-specific behavior for the PERST# signal in system suspend,
> > depending on the type of PCIe endpoints are attached.
> > 
> > Signed-off-by: Brian Norris <briannorris@chromium.org>
> > ---
> >  Documentation/devicetree/bindings/pci/pci.txt | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/pci/pci.txt b/Documentation/devicetree/bindings/pci/pci.txt
> > index c77981c5dd18..91339b6d0652 100644
> > --- a/Documentation/devicetree/bindings/pci/pci.txt
> > +++ b/Documentation/devicetree/bindings/pci/pci.txt
> > @@ -24,3 +24,14 @@ driver implementation may support the following properties:
> >     unsupported link speed, for instance, trying to do training for
> >     unsupported link speed, etc.  Must be '4' for gen4, '3' for gen3, '2'
> >     for gen2, and '1' for gen1. Any other values are invalid.
> > +- pcie-reset-suspend:
> > +   If present this property defines whether the PCIe Reset signal (referred to
> > +   as PERST#) should be asserted when the system enters low-power suspend modes
> > +   (e.g., S3). Depending on the form factor, the associated PCIe
> > +   electromechanical specification may specify a particular behavior (e.g.,
> > +   "PERST# is asserted in advance of the power being switched off in a
> > +   power-managed state like S3") or it may be less clear. The net result is
> > +   that some endpoints perform better (e.g., lower power consumption) with
> > +   PERST# asserted, and others prefer PERST# deasserted. The value must be '0'
> > +   or '1', where '0' means do not assert PERST# and '1' means assert PERST#.
> > +   When absent, behavior may be platform-specific.
> 
> I don't understand this at all.  Are you suggesting that we need
> different "pcie-reset-suspend" values based on what sort of endpoint
> the user plugs in?

Partly. I guess I also failed to mention in this particular text (but I
did, in patch 3) that it can be a board-specific problem, related to the
fact that the only endpoint used on said board--soldered on, not
replaceable--never seemed to require PERST# to be asserted in suspend.
On such boards, I believe [1] it is physically impossible to drive this
pin low in S3 suspend. So even if we tried to assert PERST#, it would
pull back up when we suspend the system. I'm not sure if that's better
or worse than not asserting it at all.

So, that's why I settled on a device tree property. It describes the
physical ability of the board too, in some cases. (I could document this
better, I see.)

However, in other cases, we might have boards that are physically
capable, but where the use of this endpoint might still cause
interoperability problems, per your next comment. So... (continued
below)

> If so, I really don't want to get involved in that, because that's an
> issue that needs to be resolved by the vendors and the PCI-SIG.  If we

Judging by conversations with these vendors, I can't really imagine them
proactively dealing with the PCI-SIG on this. Is that really what you
think will work best?

I personally believe deferring (i.e., ignoring) the problem will not
cause any change; badly behaved vendors will just do whatever suits
them, and system designers will have to figure it out somehow -- ACPI
systems will have platform-specific behavior hidden in firmware; device
tree systems will do whatever they want out of tree; and the rare device
tree system that gets upstream support will either have suboptimal power
management, or have to have these sorts of conversations again. None of
that puts pressure on an endpoint vendor to talk to the PCI-SIG.

(I can try to put that pressure on them, but I only have so much power.)

> put in a tweak like this, I think it would be a band-aid that only
> delays getting a real solution figured out.
> 
> If you want a quirk to tune this based on specific devices, that might
> make sense.  It would still sound like an interoperability issue and
> an ongoing maintenance problem, but at least we would have specific
> details about which devices are involved, and we'd have a chance to
> make them work on more controllers than just Rockchip.

(continued) ...I suppose we could do this too. Like, a new entry in enum
pci_dev_flags, and code in drivers/pci/quirks.c? And then some helper so
that a PCIe root port driver like Rockchip's can walk its children and
check for the existence of this quirk? I'll see if I can write that up
reasonably.

Then I guess technically we could get away with not supporting the
aforementioned device tree property, since any board where the HW won't
support asserting PERST# in S3 should also have an endpoint with this
quirk.

Brian

[1] I say "I believe" because the GPIO power rail is not powered in S3
    (so at best we can possibly give a weak pull) and it is pulled up
    externally. I don't think there's any way around this.

[PATCH 1/3] Documentation/devicetree: Add pcie-reset-suspend property

[Brian Norris]

On Tue, Oct 17, 2017 at 4:39 PM, Brian Norris <briannorris@chromium.org> wrote:
> On Fri, Oct 13, 2017 at 11:51:52AM -0500, Bjorn Helgaas wrote:
> > If so, I really don't want to get involved in that, because that's an
> > issue that needs to be resolved by the vendors and the PCI-SIG.  If we
>
> Judging by conversations with these vendors, I can't really imagine them
> proactively dealing with the PCI-SIG on this. Is that really what you
> think will work best?
>
> I personally believe deferring (i.e., ignoring) the problem will not
> cause any change; badly behaved vendors will just do whatever suits
> them, and system designers will have to figure it out somehow -- ACPI
> systems will have platform-specific behavior hidden in firmware; device
> tree systems will do whatever they want out of tree; and the rare device
> tree system that gets upstream support will either have suboptimal power
> management, or have to have these sorts of conversations again. None of
> that puts pressure on an endpoint vendor to talk to the PCI-SIG.

I'll add a little more to my claim about ACPI systems. I chatted a
little more with another engineer on my team who has dealt with ACPI
firmware for a few generations of Intel platforms. Even among the
latest two platforms he dealt with, there have been two different
sorts of chipset bugs (at the host/root complex side, not just the
endpoint) that have yielded different decisions on how to handle
PERST#. This was opaque to Linux though, since that's how system
firmware rolls :)

I expect this will not be the last discrepancy on how to handle
PERST#. And to my knowledge, none of the above initiated any
discussion with the PCI-SIG.

Brian

[PATCH 1/3] Documentation/devicetree: Add pcie-reset-suspend property

[Bjorn Helgaas]

On Tue, Oct 17, 2017 at 04:39:05PM -0700, Brian Norris wrote:
> On Fri, Oct 13, 2017 at 11:51:52AM -0500, Bjorn Helgaas wrote:
> > On Thu, Oct 12, 2017 at 01:52:18PM -0700, Brian Norris wrote:
> > > The patch is self-descriptive. I've found that we may need
> > > platform-specific behavior for the PERST# signal in system suspend,
> > > depending on the type of PCIe endpoints are attached.
> > > 
> > > Signed-off-by: Brian Norris <briannorris@chromium.org>
> > > ---
> > >  Documentation/devicetree/bindings/pci/pci.txt | 11 +++++++++++
> > >  1 file changed, 11 insertions(+)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/pci/pci.txt b/Documentation/devicetree/bindings/pci/pci.txt
> > > index c77981c5dd18..91339b6d0652 100644
> > > --- a/Documentation/devicetree/bindings/pci/pci.txt
> > > +++ b/Documentation/devicetree/bindings/pci/pci.txt
> > > @@ -24,3 +24,14 @@ driver implementation may support the following properties:
> > >     unsupported link speed, for instance, trying to do training for
> > >     unsupported link speed, etc.  Must be '4' for gen4, '3' for gen3, '2'
> > >     for gen2, and '1' for gen1. Any other values are invalid.
> > > +- pcie-reset-suspend:
> > > +   If present this property defines whether the PCIe Reset signal (referred to
> > > +   as PERST#) should be asserted when the system enters low-power suspend modes
> > > +   (e.g., S3). Depending on the form factor, the associated PCIe
> > > +   electromechanical specification may specify a particular behavior (e.g.,
> > > +   "PERST# is asserted in advance of the power being switched off in a
> > > +   power-managed state like S3") or it may be less clear. The net result is
> > > +   that some endpoints perform better (e.g., lower power consumption) with
> > > +   PERST# asserted, and others prefer PERST# deasserted. The value must be '0'
> > > +   or '1', where '0' means do not assert PERST# and '1' means assert PERST#.
> > > +   When absent, behavior may be platform-specific.
> > 
> > I don't understand this at all.  Are you suggesting that we need
> > different "pcie-reset-suspend" values based on what sort of endpoint
> > the user plugs in?
> 
> Partly. I guess I also failed to mention in this particular text (but I
> did, in patch 3) that it can be a board-specific problem, related to the
> fact that the only endpoint used on said board--soldered on, not
> replaceable--never seemed to require PERST# to be asserted in suspend.
> On such boards, I believe [1] it is physically impossible to drive this
> pin low in S3 suspend. So even if we tried to assert PERST#, it would
> pull back up when we suspend the system. I'm not sure if that's better
> or worse than not asserting it at all.
> 
> So, that's why I settled on a device tree property. It describes the
> physical ability of the board too, in some cases. (I could document this
> better, I see.)

It would make sense to me to have a DT property in the PCIe host
controller object that describes how that controller works, including
its capabilities with respect to PERST#, assuming there's a reasonable
way to use that information.

If there's a DT way to describe a hard-wired PCIe endpoint, it might
make sense to have a second property in the endpoint object that
describes its preferences.  Obviously this couldn't apply to slots
where we don't know what might be plugged in.

> > If you want a quirk to tune this based on specific devices, that might
> > make sense.  It would still sound like an interoperability issue and
> > an ongoing maintenance problem, but at least we would have specific
> > details about which devices are involved, and we'd have a chance to
> > make them work on more controllers than just Rockchip.
> 
> (continued) ...I suppose we could do this too. Like, a new entry in enum
> pci_dev_flags, and code in drivers/pci/quirks.c? And then some helper so
> that a PCIe root port driver like Rockchip's can walk its children and
> check for the existence of this quirk? I'll see if I can write that up
> reasonably.

I'm not sure how this would work out.  I can see the quirk side (e.g.,
the quirk could set a bit in the root port), but who would consume it?
Would every host bridge driver have to look at the bit?  This doesn't
feel like a generic model that works well across vendors.

If devices rely on things not specified by the spec, they will work
better on some systems than on others.  That feels like an issue for
the vendors, not for us.

Obviously you want to tweak something for your particular
configuration, and you can do that either via out-of-tree code or via
some more generic way that I haven't seen yet.

Bjorn

[PATCH 1/3] Documentation/devicetree: Add pcie-reset-suspend property

[Rob Herring]

On Wed, Oct 18, 2017 at 11:17 AM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> On Tue, Oct 17, 2017 at 04:39:05PM -0700, Brian Norris wrote:
>> On Fri, Oct 13, 2017 at 11:51:52AM -0500, Bjorn Helgaas wrote:
>> > On Thu, Oct 12, 2017 at 01:52:18PM -0700, Brian Norris wrote:
>> > > The patch is self-descriptive. I've found that we may need
>> > > platform-specific behavior for the PERST# signal in system suspend,
>> > > depending on the type of PCIe endpoints are attached.
>> > >
>> > > Signed-off-by: Brian Norris <briannorris@chromium.org>
>> > > ---
>> > >  Documentation/devicetree/bindings/pci/pci.txt | 11 +++++++++++
>> > >  1 file changed, 11 insertions(+)
>> > >
>> > > diff --git a/Documentation/devicetree/bindings/pci/pci.txt b/Documentation/devicetree/bindings/pci/pci.txt
>> > > index c77981c5dd18..91339b6d0652 100644
>> > > --- a/Documentation/devicetree/bindings/pci/pci.txt
>> > > +++ b/Documentation/devicetree/bindings/pci/pci.txt
>> > > @@ -24,3 +24,14 @@ driver implementation may support the following properties:
>> > >     unsupported link speed, for instance, trying to do training for
>> > >     unsupported link speed, etc.  Must be '4' for gen4, '3' for gen3, '2'
>> > >     for gen2, and '1' for gen1. Any other values are invalid.
>> > > +- pcie-reset-suspend:
>> > > +   If present this property defines whether the PCIe Reset signal (referred to
>> > > +   as PERST#) should be asserted when the system enters low-power suspend modes
>> > > +   (e.g., S3). Depending on the form factor, the associated PCIe
>> > > +   electromechanical specification may specify a particular behavior (e.g.,
>> > > +   "PERST# is asserted in advance of the power being switched off in a
>> > > +   power-managed state like S3") or it may be less clear. The net result is
>> > > +   that some endpoints perform better (e.g., lower power consumption) with
>> > > +   PERST# asserted, and others prefer PERST# deasserted. The value must be '0'
>> > > +   or '1', where '0' means do not assert PERST# and '1' means assert PERST#.
>> > > +   When absent, behavior may be platform-specific.
>> >
>> > I don't understand this at all.  Are you suggesting that we need
>> > different "pcie-reset-suspend" values based on what sort of endpoint
>> > the user plugs in?
>>
>> Partly. I guess I also failed to mention in this particular text (but I
>> did, in patch 3) that it can be a board-specific problem, related to the
>> fact that the only endpoint used on said board--soldered on, not
>> replaceable--never seemed to require PERST# to be asserted in suspend.
>> On such boards, I believe [1] it is physically impossible to drive this
>> pin low in S3 suspend. So even if we tried to assert PERST#, it would
>> pull back up when we suspend the system. I'm not sure if that's better
>> or worse than not asserting it at all.
>>
>> So, that's why I settled on a device tree property. It describes the
>> physical ability of the board too, in some cases. (I could document this
>> better, I see.)
>
> It would make sense to me to have a DT property in the PCIe host
> controller object that describes how that controller works, including
> its capabilities with respect to PERST#, assuming there's a reasonable
> way to use that information.
>
> If there's a DT way to describe a hard-wired PCIe endpoint, it might
> make sense to have a second property in the endpoint object that
> describes its preferences.  Obviously this couldn't apply to slots
> where we don't know what might be plugged in.

That's implicit with having an endpoint described in DT though true OF
would have every device present.

Rob