Message-ID: <59EF3C75.2070107@rock-chips.com>
Date:   Tue, 24 Oct 2017 21:13:25 +0800
From:   jeffy <jeffy.chen@...k-chips.com>
To:     Brian Norris <briannorris@...omium.org>,
        Bjorn Helgaas <bhelgaas@...gle.com>
CC:     linux-kernel@...r.kernel.org, shawn.lin@...k-chips.com,
        dianders@...omium.org, linux-pci@...r.kernel.org,
        linux-pm@...r.kernel.org, Tony Lindgren <tony@...mide.com>,
        "Rafael J. Wysocki" <rjw@...ysocki.net>
Subject: Re: [PATCH v7 1/3] PCI: Add support for wake irq

Hi Brian,

checking the pci-acpi code, it has:
1/ pci_acpi_setup() and pci_acpi_cleanup() to setup/cleanup the 
wakeup(and other stuff) for pci devices

we may need it too(for per-device wake) to parse wake irq and init 
wakeup(false) and maybe setup dedicated wakeirq.

2/ acpi_pci_wakeup(), which would do:
find a parent or root bus or pci dev itself, which can do wakeup, and 
update it's wakeup ability(with a enable_count).

we may need to do something like that, but the can_wakeup() would be 
check if wake irq avaliable, and set_device/bridge_wakeup() would be 
setup/clear dedicated wakeirq, or just call device_set_wakeup_enable()



so maybe we can:
1/ add a setup_root_bus() platform ops callback to parse/setup root 
bus's wakeirq in pci_register_host_bridge(),and clean it in 
pci_stop_bus_device()

2/ add a device_setup() and device_cleanup() callbacks to setup/clean 
pci device's wake irq, and maybe call it in pci_device_probe() or 
pci_setup_device()?

3/ add a can_wakeup() and set_device_wakeup() and set_bridge_wakeup() 
callbacks, and move acpi_pci_wakeup() and acpi_pci_propagate_wakeup()'s 
code and the enable_count code into common platform_pci_set_wakeup().


does this make sense?


On 10/24/2017 12:06 PM, jeffy wrote:
> Hi Brian,
>
> On 10/24/2017 07:02 AM, Brian Norris wrote:
>> + PM folks
>>
>> Hi Jeffy,
>>
>> It's probably good if you send the whole thing to linux-pm@ in the
>> future, if you're really trying to implement generic PCI/PM for device
>> tree systems.
> ok
>>
>> On Thu, Oct 19, 2017 at 07:10:05PM +0800, Jeffy Chen wrote:
>>> Add support for PCIE_WAKE pin.
>>
>> This is kind of an important change, so it feels like you should
>> document it a little more thoroughly than this. Particularly, I have a
>> few questions below, and it seems like some of these questions should be
>> acknowledged up front. e.g., why does this look so different than the
>> ACPI hooks?
> sure, will do in next version.
>>
>>>
>>> Signed-off-by: Jeffy Chen <jeffy.chen@...k-chips.com>
>>> ---
>>>
>>> Changes in v7:
>>> Move PCIE_WAKE handling into pci core.
>>>
>>> Changes in v6:
>>> Fix device_init_wake error handling, and add some comments.
>>>
>>> Changes in v5:
>>> Rebase
>>>
>>> Changes in v3:
>>> Fix error handling
>>>
>>> Changes in v2:
>>> Use dev_pm_set_dedicated_wake_irq
>>>          -- Suggested by Brian Norris <briannorris@...omium.com>
>>>
>>>   drivers/pci/pci.c    | 34 ++++++++++++++++++++++++++++++++--
>>>   drivers/pci/probe.c  | 32 ++++++++++++++++++++++++++++----
>>>   drivers/pci/remove.c |  9 +++++++++
>>>   3 files changed, 69 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>>> index f0d68066c726..49080a10bdf0 100644
>>> --- a/drivers/pci/pci.c
>>> +++ b/drivers/pci/pci.c
>>> @@ -603,10 +603,40 @@ static inline pci_power_t
>>> platform_pci_choose_state(struct pci_dev *dev)
>>>               pci_platform_pm->choose_state(dev) : PCI_POWER_ERROR;
>>>   }
>>>
>>> +static int pci_dev_check_wakeup(struct pci_dev *dev, void *data)
>>> +{
>>> +    bool *wakeup = data;
>>> +
>>> +    if (device_may_wakeup(&dev->dev))
>>> +        *wakeup = true;
>>> +
>>> +    return *wakeup;
>>> +}
>>> +
>>>   static inline int platform_pci_set_wakeup(struct pci_dev *dev, bool
>>> enable)
>>>   {
>>> -    return pci_platform_pm ?
>>> -            pci_platform_pm->set_wakeup(dev, enable) : -ENODEV;
>>> +    struct pci_dev *parent = dev;
>>> +    struct pci_bus *bus;
>>> +    bool wakeup = false;
>>
>> It feels like you're implementing a set of pci_platform_pm_ops, except
>> you're not actually implementing them. It almost seems like we should
>> have a drivers/pci/pci-of.c to do this. But that brings up a few
>> questions....
> i saw the drivers might call set_wakeup() in suspend/resume/shutdown to
> configure the wakeup ability, maybe we can call
> device_set_wakeup_enable() here as a common part of set_wakeup()?
>
> static inline int platform_pci_set_wakeup(struct pci_dev *dev, bool
> enable) {
>      device_set_wakeup_enable()
> ...
>      return pci_platform_pm ? pci_platform_pm->set_wakeup(dev, enable) :
> -ENODEV;
>
>>
>>> +
>>> +    if (pci_platform_pm)
>>
>> So, if somebody already registered ops, then you won't follow the "OF"
>> route? That means this all breaks as soon as a kernel has both
>> CONFIG_ACPI and CONFIG_OF enabled. This is possible on at least ARM64,
>> which 'select's OF and may also be built/run with CONFIG_ACPI.
>>
>> And that conflict is the same if we try to register pci_platform_pm_ops
>> for OF systems -- it'll be a race over who sets them up first (or
>> rather, last).
>>
>> Also, what happens on !ACPI && !OF? Or if the device tree did not
>> contain a "wakeup" definition? You're now implementing a default path
>> that doesn't make much sense IMO; you may claim wakeup capability
>> without actually having set it up somewhere.
> maybe we can use device_set_wakeup_enable(), which will check the setup
> before setting?
>>
>> I think you could use some more comments, and (again) a real commit
>> message.
> ok, will do.
>>
>>> +        return pci_platform_pm->set_wakeup(dev, enable);
>>> +
>>> +    device_set_wakeup_capable(&dev->dev, enable);
>>
>> Why are you setting that here? This function should just be telling the
>> lower layers to enable the physical WAKE# ability. In our case, it just
>> means configuring the WAKE# interrupt for wakeup -- or, since you've
>> used dev_pm_set_dedicated_wake_irq() which handles most of this
>> automatically...do you need this at all? It seems like you should
>> *either* implement these callbacks to manually manage the wakeup IRQ or
>> else use the dedicated wakeirq infrastructure -- not both.
>>
>> And even if you need this, I don't think you need to do this many times;
>> you should only need to set up the capabilities once, when you first set
>> up the device.
>>
>> And BTW, the description for the set_wakeup() callback says:
>>
>>   * @set_wakeup: enables/disables wakeup capability for the device
>>
>> I *don't* think that means "capability" as in the device framework's
>> view of "wakeup capable"; I think it means capability as in the physical
>> ability (a la, enable_irq_wake() or similar).
> i was thinking maybe we should disable the wakeup if all children
> request set_wakeup(false)?
>
> and it seems like the dedicated wakeirq can be disabled by:
> 1/ dev_pm_clear_wake_irq(), then we may need to store the irq somewhere
> to set it up again in the future?
>
> 2/ let device_may_wakeup return false:
> void dev_pm_arm_wake_irq(struct wake_irq *wirq)
> {
>          if (!wirq)
>                  return;
>
>          if (device_may_wakeup(wirq->dev)) {
>                  if (wirq->status & WAKE_IRQ_DEDICATED_ALLOCATED)
>                          enable_irq(wirq->irq);
>
>                  enable_irq_wake(wirq->irq);
>          }
>
>>
>>> +
>>> +    while ((parent = pci_upstream_bridge(parent)))
>>> +        bus = parent->bus;
>>> +
>>> +    if (!bus || !pci_is_root_bus(bus) || !bus->bridge->parent)
>>> +        return -ENODEV;
>>> +
>>> +    pci_walk_bus(bus, pci_dev_check_wakeup, &wakeup);
>>> +    device_set_wakeup_capable(bus->bridge->parent, wakeup);
>>
>> What happens to any intermediate buses? You haven't marked them as
>> wakeup-capable. Should you?
>>
>> And the more fundamental question here is: is this a per-device
>> configuration or a per-root-port configuration? The APIs here are
>> modeled after ACPI, where I guess this is a per-device thing. The PCIe
>> spec doesn't exactly specify how many WAKE# pins you need, though it
>> seems to say
>>
>> (a) it's all-or-nothing (if one device uses it, all wakeup-capable EPs
>>      should be wired up to it)
>> (b) it *can* be done as a single input to the system controller, since
>>      it's an open drain signal
>> (c) ...but I also see now in the PCIe Card Electromechanical
>>      specification:
>>
>>      "WAKE# may be bused to multiple PCI Express add-in card connectors,
>>      forming a single input connection at the PM controller, or
>>      individual connectors can have individual connections to the PM
>>      controller."
>>
>> So I think you're kind of going along the lines of (b) (as I suggested
>> to you previously), and that matches the current hardware (we only have
>> a single WAKE#) and proposed DT binding. But should this be set up in a
>> way that suits (c) too? It's hard to tell exactly what ACPI-based
>> systems do, since they have this abstracted behind ACPI interfaces that
>> seem like they *could* support per-device or per-bridge type of hookups.
> maybe we can try to setup wake irq for each pci devices which have it in
> the dts, then in the set_wakeup(), try to find the parents(or itself)
> who has wake irq, and enable/disable them(maybe also need a refcount)?
>>
>> Bjorn, any thoughts? This seems like a halfway attempt in between two
>> different designs, and I'm not really sure which one makes more sense.
>>
>> Brian
>>
>>> +
>>> +    dev_dbg(bus->bridge->parent,
>>> +        "Wakeup %s\n", wakeup ? "enabled" : "disabled");
>>> +
>>> +    return 0;
>>>   }
>>>
>>>   static inline bool platform_pci_need_resume(struct pci_dev *dev)
>>> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
>>> index cdc2f83c11c5..fd43ca832665 100644
>>> --- a/drivers/pci/probe.c
>>> +++ b/drivers/pci/probe.c
>>> @@ -7,6 +7,7 @@
>>>   #include <linux/init.h>
>>>   #include <linux/pci.h>
>>>   #include <linux/of_device.h>
>>> +#include <linux/of_irq.h>
>>>   #include <linux/of_pci.h>
>>>   #include <linux/pci_hotplug.h>
>>>   #include <linux/slab.h>
>>> @@ -17,6 +18,7 @@
>>>   #include <linux/acpi.h>
>>>   #include <linux/irqdomain.h>
>>>   #include <linux/pm_runtime.h>
>>> +#include <linux/pm_wakeirq.h>
>>>   #include "pci.h"
>>>
>>>   #define CARDBUS_LATENCY_TIMER    176    /* secondary latency timer */
>>> @@ -756,11 +758,28 @@ static int pci_register_host_bridge(struct
>>> pci_host_bridge *bridge)
>>>       struct resource *res;
>>>       char addr[64], *fmt;
>>>       const char *name;
>>> -    int err;
>>> +    int err, irq;
>>> +
>>> +    if (IS_ENABLED(CONFIG_OF) && parent && parent->of_node) {
>>> +        irq = of_irq_get_byname(parent->of_node, "wakeup");
>>> +        if (irq == -EPROBE_DEFER)
>>> +            return irq;
>>> +        if (irq > 0) {
>>> +            device_init_wakeup(parent, true);
>>> +            err = dev_pm_set_dedicated_wake_irq(parent, irq);
>>> +            if (err) {
>>> +                dev_err(parent, "Failed to setup wakeup IRQ\n");
>>> +                goto deinit_wakeup;
>>> +            }
>>> +            dev_info(parent, "Wakeup enabled with IRQ %d\n", irq);
>>> +        }
>>> +    }
>>>
>>>       bus = pci_alloc_bus(NULL);
>>> -    if (!bus)
>>> -        return -ENOMEM;
>>> +    if (!bus) {
>>> +        err = -ENOMEM;
>>> +        goto clear_wake_irq;
>>> +    }
>>>
>>>       bridge->bus = bus;
>>>
>>> @@ -856,9 +875,14 @@ static int pci_register_host_bridge(struct
>>> pci_host_bridge *bridge)
>>>   unregister:
>>>       put_device(&bridge->dev);
>>>       device_unregister(&bridge->dev);
>>> -
>>>   free:
>>>       kfree(bus);
>>> +clear_wake_irq:
>>> +    if (parent)
>>> +        dev_pm_clear_wake_irq(parent);
>>> +deinit_wakeup:
>>> +    if (parent)
>>> +        device_init_wakeup(parent, false);
>>>       return err;
>>>   }
>>>
>>> diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
>>> index 73a03d382590..cb7a326429e1 100644
>>> --- a/drivers/pci/remove.c
>>> +++ b/drivers/pci/remove.c
>>> @@ -1,6 +1,7 @@
>>>   #include <linux/pci.h>
>>>   #include <linux/module.h>
>>>   #include <linux/pci-aspm.h>
>>> +#include <linux/pm_wakeirq.h>
>>>   #include "pci.h"
>>>
>>>   static void pci_free_resources(struct pci_dev *dev)
>>> @@ -131,17 +132,25 @@ void pci_stop_root_bus(struct pci_bus *bus)
>>>   {
>>>       struct pci_dev *child, *tmp;
>>>       struct pci_host_bridge *host_bridge;
>>> +    struct device *parent;
>>>
>>>       if (!pci_is_root_bus(bus))
>>>           return;
>>>
>>>       host_bridge = to_pci_host_bridge(bus->bridge);
>>> +    parent = host_bridge->dev.parent;
>>> +
>>>       list_for_each_entry_safe_reverse(child, tmp,
>>>                        &bus->devices, bus_list)
>>>           pci_stop_bus_device(child);
>>>
>>>       /* stop the host bridge */
>>>       device_release_driver(&host_bridge->dev);
>>> +
>>> +    if (parent) {
>>> +        dev_pm_clear_wake_irq(parent);
>>> +        device_init_wakeup(parent, false);
>>> +    }
>>>   }
>>>   EXPORT_SYMBOL_GPL(pci_stop_root_bus);
>>>
>>> --
>>> 2.11.0
>>>
>>>
>>
>>
>>
>

Powered by blists - more mailing lists