| Message ID | 20171012205220.130048-2-briannorris@chromium.org |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | PCI: rockchip: assert PERST# in S3 | expand |
[+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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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(+)