Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20201029105941.63410-1-heikki.krogerus@linux.intel.com> <20201029105941.63410-2-heikki.krogerus@linux.intel.com>
In-Reply-To: <20201029105941.63410-2-heikki.krogerus@linux.intel.com>
From:   "Rafael J. Wysocki" <rafael@kernel.org>
Date:   Thu, 29 Oct 2020 18:10:59 +0100
Message-ID: <CAJZ5v0gc6ihoNuh5eR4MW+uf9CBH=qoRjGeeOHxgvjmUnorPCg@mail.gmail.com>
Subject: Re: [PATCHv2 1/3] software node: Power management operations for
 software nodes
To:     Heikki Krogerus <heikki.krogerus@linux.intel.com>
Cc:     "Rafael J. Wysocki" <rjw@rjwysocki.net>,
        Felipe Balbi <balbi@kernel.org>,
        Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
        Sakari Ailus <sakari.ailus@linux.intel.com>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        "open list:ULTRA-WIDEBAND (UWB) SUBSYSTEM:" 
        <linux-usb@vger.kernel.org>,
        ACPI Devel Maling List <linux-acpi@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, Oct 29, 2020 at 11:59 AM Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
>
> The software node specific PM operations make it possible to
> handle most PM related quirks separately in their own
> functions instead of conditionally in the device driver's
> generic PM functions (and in some cases all over the
> driver). The software node specific PM operations will also
> reduce the need to pass platform data in some cases, for
> example from a core MFD driver to the child device drivers,
> as from now on the core MFD driver will be able to implement
> the PM quirks directly for the child devices without the
> need to touch the drivers of those child devices.
>
> If a software node includes the PM operations, those PM
> operations are always executed separately on top of the
> other PM operations of the device, so the software node will
> never replace any of the "normal" PM operations of the
> device (including the PM domain's operations, class's or
> bus's PM operations, the device drivers own operations, or
> any other).
>
> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> ---
>  drivers/base/power/common.c |   8 +-
>  drivers/base/swnode.c       | 693 +++++++++++++++++++++++++++++++++++-
>  include/linux/property.h    |  10 +
>  3 files changed, 701 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/base/power/common.c b/drivers/base/power/common.c
> index bbddb267c2e69..b64cd4690ac63 100644
> --- a/drivers/base/power/common.c
> +++ b/drivers/base/power/common.c
> @@ -109,8 +109,14 @@ int dev_pm_domain_attach(struct device *dev, bool power_on)
>         ret = acpi_dev_pm_attach(dev, power_on);
>         if (!ret)
>                 ret = genpd_dev_pm_attach(dev);
> +       if (ret < 0)
> +               return ret;
>
> -       return ret < 0 ? ret : 0;
> +       ret = software_node_dev_pm_attach(dev, power_on);
> +       if (ret)
> +               dev_pm_domain_detach(dev, power_on);
> +
> +       return ret;
>  }
>  EXPORT_SYMBOL_GPL(dev_pm_domain_attach);
>
> diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
> index 010828fc785bc..595a9c240fede 100644
> --- a/drivers/base/swnode.c
> +++ b/drivers/base/swnode.c
> @@ -8,6 +8,8 @@
>
>  #include <linux/device.h>
>  #include <linux/kernel.h>
> +#include <linux/pm_domain.h>
> +#include <linux/pm_runtime.h>
>  #include <linux/property.h>
>  #include <linux/slab.h>
>
> @@ -48,6 +50,19 @@ EXPORT_SYMBOL_GPL(is_software_node);
>                                      struct swnode, fwnode) : NULL;     \
>         })
>
> +static inline struct swnode *dev_to_swnode(struct device *dev)
> +{
> +       struct fwnode_handle *fwnode = dev_fwnode(dev);
> +
> +       if (!fwnode)
> +               return NULL;
> +
> +       if (!is_software_node(fwnode))
> +               fwnode = fwnode->secondary;
> +
> +       return to_swnode(fwnode);
> +}
> +
>  static struct swnode *
>  software_node_to_swnode(const struct software_node *node)
>  {
> @@ -344,6 +359,673 @@ void property_entries_free(const struct property_entry *properties)
>  }
>  EXPORT_SYMBOL_GPL(property_entries_free);
>
> +/* -------------------------------------------------------------------------- */
> +/* Power management operations */
> +
> +/*
> + * The power management operations in software nodes are handled with a power
> + * management domain - a "wrapper" PM domain:
> + *
> + *   When PM operations are supplied as part of the software node, the primary
> + *   PM domain of the device is stored and replaced with a device specific
> + *   software node PM domain. The software node PM domain's PM operations, which
> + *   are implemented below, will then always call the matching PM operation of
> + *   the primary PM domain (which was stored) on top of the software node's own
> + *   operation.
> + *
> + * If the device does not have primary PM domain, the software node PM wrapper
> + * operations below will also call the classes, buses and device type's PM
> + * operations, and of course the device driver's own PM operations if they are
> + * implemented. The priority of those calls follows drivers/base/power/domain.c:
> + *
> + * 1) device type
> + * 2) class
> + * 3) bus
> + * 4) driver
> + *
> + * NOTE. The software node PM operation is always called before the primary
> + * PM domain with resume/on callbacks, and after the primary PM domain with
> + * suspend/off callbacks. This order is used because the software node PM
> + * operations are primarily meant to be used to implement quirks, quirks that
> + * may be needed to power on the device to a point where it is even possible to
> + * execute the primary PM domain's resume/on routines.
> + */

Well, this basically implements a wrapper PM domain that is somewhat
more generic, as a concept, then software nodes PM.

At least it is not specific to software nodes, so I'd prefer it to be
defined generically.

Moreover, IIUC, this breaks if the "primary" PM domain callbacks try
to get to the original PM domain via the dev->pm_domain pointer, which
the genpd callbacks do.

Do we want to wrap the ACPI PM domain only, by any chance?  If so, it
may be more straightforward to invoke swnode callbacks directly from
there, if any.