Re: [PATCH v5 1/3] PCI: rockchip: Add support for pcie wake irq

[Brian Norris]

Hi,

On Fri, Oct 13, 2017 at 02:19:06PM -0500, Bjorn Helgaas wrote:
> On Sat, Oct 14, 2017 at 02:33:45AM +0800, jeffy wrote:
> > Hi Rafael,
> > 
> > On 10/13/2017 09:21 PM, Rafael J. Wysocki wrote:
> > >>
> > >>I'm a little skeptical about dev_pm_set_dedicated_wake_irq(), not
> > >>because I know anything at all about it, but because there are only
> > >>five callers in the whole tree, three of which are in UART code, and
> > >>none in anything resembling PCI code.
> > >>
> > >>Is Rockchip really that special, or are we going about this the wrong
> > >>way?
> > 
> > we used to put these codes in the wifi driver, but another wifi
> > vendor suggests these should go into the pcie driver.
> > 
> > and as tony said, it could go into pcie common code :)
> 
> I guess the implication (I'm speculating here) is that in most
> existing cases, the WAKE# signal is fielded by an ACPI BIOS, which
> knows how it's connected.  I suppose that would end up being turned
> into an SCI that Linux already knows how to handle generically.

I wasn't sure how ACPI did this when I first suggested Rockchip take
this approach, but since then I believe have figured it out. We have:

pci_prepare_to_sleep() -> pci_enable_wake()

where pci_enable_wake() will configure PME wakeup and/or "platform" wake
(which presumably is the WAKE# signal). pci-acpi.c has registered hooks
for the latter via pci_set_platform_pm().

This doesn't really make it any more generic for discovering this
platform-specific detail. We'd have to set up some kind of platform ops
that could be shared for any DT-based platforms.

But that *does* answer the question I had about conditionality: should
we always enable WAKE# for platforms that have the pin hooked up to the
host? Or is this configured on a per-device basis? IIUC, the intention
is that there's only a single open-drain WAKE# pin for the whole system,
and it's just pulled high for EPs that don't implement it.

> And further, that the non-ACPI drivers are relatively new and you're
> the first attempt to use WAKE# with a non-ACPI PCI host driver?

Quite possibly. Or everyone just sidestepped this an configured the pin
elsewhere (e.g., you could stick a GPIO like this into a gpio-keys
device and it would mostly work).

> If this setup could be done somewhere in PCIe common code, that would
> be great.  We have so much copy and pasted code already, it'd be nice
> to avoid adding more.  I don't know if this would fit in
> pci_scan_root_bus_bridge(), doing something like dma_configure() does
> to get hold of a struct platform_device * or a struct device * so you
> could lookup the IRQ?

It looks like the infrastructure is in pci_set_platform_pm(), sort of.
But that still doesn't help you for the repetition; you're just lucky
you only have 2 controller drivers that call this right now :)

Side note: there's some dissonance between this statement, in
Documentation/driver-api/pm/devices.rst:

"Device drivers, however, are not expected to call
:c:func:`device_set_wakeup_enable()` directly in any case."

Yet:

$ git grep -l device_set_wakeup_enable drivers/ | wc -l
69

And particularly, I believe that was necessary for Wifi drivers like
drivers/net/wireless/ath/ath10k/wow.c.

Brian