[PATCH 0/3] PM operations for software nodes

[PATCH 0/3] PM operations for software nodes

[Heikki Krogerus]

Hi guys,

Unfortunately the last patch depends on this:
https://lore.kernel.org/linux-usb/20200821131101.81915-1-heikki.krogerus@linux.intel.com/

Would it be easiest that Felipe took care of these (assuming they are
acceptable)?

thanks,

Heikki Krogerus (3):
  software node: Power management operations for software nodes
  software node: Introduce device_add_software_node()
  usb: dwc3: pci: Register a software node for the dwc3 platform device

 drivers/base/power/common.c |   8 +-
 drivers/base/swnode.c       | 569 +++++++++++++++++++++++++++++++++++-
 drivers/usb/dwc3/dwc3-pci.c | 175 ++++++-----
 include/linux/property.h    |  13 +
 4 files changed, 666 insertions(+), 99 deletions(-)

-- 
2.28.0

[PATCH 1/3] software node: Power management operations for software nodes

[Heikki Krogerus]

Adding separate PM operations vector for the software nodes.
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       | 524 +++++++++++++++++++++++++++++++++++-
 include/linux/property.h    |  10 +
 3 files changed, 532 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..9a13a61a2f0bb 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,504 @@ void property_entries_free(const struct property_entry *properties)
 }
 EXPORT_SYMBOL_GPL(property_entries_free);
 
+/* -------------------------------------------------------------------------- */
+/* Power management operations */
+
+#ifdef CONFIG_PM
+struct swnode_pm_domain {
+	struct dev_pm_domain pm_domain;
+	struct dev_pm_domain *primary;
+};
+
+#define to_swnode_pm_domain(d) \
+		container_of(d, struct swnode_pm_domain, pm_domain)
+
+static int software_node_runtime_suspend(struct device *dev)
+{
+	struct swnode_pm_domain *domain = to_swnode_pm_domain(dev->pm_domain);
+	struct swnode *swnode = dev_to_swnode(dev);
+	int ret;
+
+	if (domain->primary && domain->primary->ops.runtime_suspend)
+		ret = domain->primary->ops.runtime_suspend(dev);
+	else
+		ret = pm_generic_runtime_suspend(dev);
+
+	if (ret || !swnode->node->pm->runtime_suspend)
+		return ret;
+
+	return swnode->node->pm->runtime_suspend(dev);
+}
+
+static int software_node_runtime_resume(struct device *dev)
+{
+	struct swnode_pm_domain *domain = to_swnode_pm_domain(dev->pm_domain);
+	struct swnode *swnode = dev_to_swnode(dev);
+	int ret;
+
+	if (swnode->node->pm->runtime_resume) {
+		ret = swnode->node->pm->runtime_resume(dev);
+		if (ret)
+			return ret;
+	}
+
+	if (domain->primary && domain->primary->ops.runtime_resume)
+		ret = domain->primary->ops.runtime_resume(dev);
+	else
+		ret = pm_generic_runtime_resume(dev);
+
+	return ret;
+}
+
+static int software_node_runtime_idle(struct device *dev)
+{
+	struct swnode_pm_domain *domain = to_swnode_pm_domain(dev->pm_domain);
+	struct swnode *swnode = dev_to_swnode(dev);
+	int ret = 0;
+
+	if (domain->primary && domain->primary->ops.runtime_idle)
+		ret = domain->primary->ops.runtime_idle(dev);
+
+	if (ret || !swnode->node->pm->runtime_idle)
+		return ret;
+
+	return swnode->node->pm->runtime_idle(dev);
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int software_node_prepare(struct device *dev)
+{
+	struct swnode_pm_domain *domain = to_swnode_pm_domain(dev->pm_domain);
+	struct swnode *swnode = dev_to_swnode(dev);
+	int ret;
+
+	if (domain->primary && domain->primary->ops.prepare)
+		ret = domain->primary->ops.prepare(dev);
+	else
+		ret = pm_generic_prepare(dev);
+
+	if (ret || !swnode->node->pm->prepare)
+		return ret;
+
+	return swnode->node->pm->prepare(dev);
+}
+
+static void software_node_complete(struct device *dev)
+{
+	struct swnode_pm_domain *domain = to_swnode_pm_domain(dev->pm_domain);
+	struct swnode *swnode = dev_to_swnode(dev);
+
+	if (swnode->node->pm->complete)
+		swnode->node->pm->complete(dev);
+
+	if (domain->primary && domain->primary->ops.complete)
+		domain->primary->ops.complete(dev);
+	else
+		pm_generic_complete(dev);
+}
+
+static int software_node_suspend(struct device *dev)
+{
+	struct swnode_pm_domain *domain = to_swnode_pm_domain(dev->pm_domain);
+	struct swnode *swnode = dev_to_swnode(dev);
+	int ret;
+
+	if (domain->primary && domain->primary->ops.suspend)
+		ret = domain->primary->ops.suspend(dev);
+	else
+		ret = pm_generic_suspend(dev);
+
+	if (ret || !swnode->node->pm->suspend)
+		return ret;
+
+	return swnode->node->pm->suspend(dev);
+}
+
+static int software_node_resume(struct device *dev)
+{
+	struct swnode_pm_domain *domain = to_swnode_pm_domain(dev->pm_domain);
+	struct swnode *swnode = dev_to_swnode(dev);
+	int ret;
+
+	if (swnode->node->pm->resume) {
+		ret = swnode->node->pm->resume(dev);
+		if (ret)
+			return ret;
+	}
+
+	if (domain->primary && domain->primary->ops.resume)
+		ret = domain->primary->ops.resume(dev);
+	else
+		ret = pm_generic_resume(dev);
+
+	return ret;
+}
+
+static int software_node_freeze(struct device *dev)
+{
+	struct swnode_pm_domain *domain = to_swnode_pm_domain(dev->pm_domain);
+	struct swnode *swnode = dev_to_swnode(dev);
+	int ret;
+
+	if (domain->primary && domain->primary->ops.freeze)
+		ret = domain->primary->ops.freeze(dev);
+	else
+		ret = pm_generic_freeze(dev);
+
+	if (ret || !swnode->node->pm->freeze)
+		return ret;
+
+	return swnode->node->pm->freeze(dev);
+}
+
+static int software_node_thaw(struct device *dev)
+{
+	struct swnode_pm_domain *domain = to_swnode_pm_domain(dev->pm_domain);
+	struct swnode *swnode = dev_to_swnode(dev);
+	int ret;
+
+	if (swnode->node->pm->thaw) {
+		ret = swnode->node->pm->thaw(dev);
+		if (ret)
+			return ret;
+	}
+
+	if (domain->primary && domain->primary->ops.thaw)
+		ret = domain->primary->ops.thaw(dev);
+	else
+		ret = pm_generic_thaw(dev);
+
+	return ret;
+}
+
+static int software_node_poweroff(struct device *dev)
+{
+	struct swnode_pm_domain *domain = to_swnode_pm_domain(dev->pm_domain);
+	struct swnode *swnode = dev_to_swnode(dev);
+	int ret;
+
+	if (domain->primary && domain->primary->ops.poweroff)
+		ret = domain->primary->ops.poweroff(dev);
+	else
+		ret = pm_generic_poweroff(dev);
+
+	if (ret || !swnode->node->pm->poweroff)
+		return ret;
+
+	return swnode->node->pm->poweroff(dev);
+}
+
+static int software_node_restore(struct device *dev)
+{
+	struct swnode_pm_domain *domain = to_swnode_pm_domain(dev->pm_domain);
+	struct swnode *swnode = dev_to_swnode(dev);
+	int ret;
+
+	if (swnode->node->pm->restore) {
+		ret = swnode->node->pm->restore(dev);
+		if (ret)
+			return ret;
+	}
+
+	if (domain->primary && domain->primary->ops.restore)
+		ret = domain->primary->ops.restore(dev);
+	else
+		ret = pm_generic_restore(dev);
+
+	return ret;
+}
+
+static int software_node_suspend_late(struct device *dev)
+{
+	struct swnode_pm_domain *domain = to_swnode_pm_domain(dev->pm_domain);
+	struct swnode *swnode = dev_to_swnode(dev);
+	int ret;
+
+	if (domain->primary && domain->primary->ops.suspend_late)
+		ret = domain->primary->ops.suspend_late(dev);
+	else
+		ret = pm_generic_suspend_late(dev);
+
+	if (ret || !swnode->node->pm->suspend_late)
+		return ret;
+
+	return swnode->node->pm->suspend_late(dev);
+}
+
+static int software_node_resume_early(struct device *dev)
+{
+	struct swnode_pm_domain *domain = to_swnode_pm_domain(dev->pm_domain);
+	struct swnode *swnode = dev_to_swnode(dev);
+	int ret;
+
+	if (swnode->node->pm->resume_early) {
+		ret = swnode->node->pm->resume_early(dev);
+		if (ret)
+			return ret;
+	}
+
+	if (domain->primary && domain->primary->ops.resume_early)
+		ret = domain->primary->ops.resume_early(dev);
+	else
+		pm_generic_resume_early(dev);
+
+	return 0;
+}
+
+static int software_node_freeze_late(struct device *dev)
+{
+	struct swnode_pm_domain *domain = to_swnode_pm_domain(dev->pm_domain);
+	struct swnode *swnode = dev_to_swnode(dev);
+	int ret;
+
+	if (domain->primary && domain->primary->ops.freeze_late)
+		ret = domain->primary->ops.freeze_late(dev);
+	else
+		ret = pm_generic_freeze_late(dev);
+
+	if (ret || !swnode->node->pm->freeze_late)
+		return ret;
+
+	return swnode->node->pm->freeze_late(dev);
+}
+
+static int software_node_thaw_early(struct device *dev)
+{
+	struct swnode_pm_domain *domain = to_swnode_pm_domain(dev->pm_domain);
+	struct swnode *swnode = dev_to_swnode(dev);
+	int ret;
+
+	if (swnode->node->pm->thaw_early) {
+		ret = swnode->node->pm->thaw_early(dev);
+		if (ret)
+			return ret;
+	}
+
+	if (domain->primary && domain->primary->ops.thaw_early)
+		ret = domain->primary->ops.thaw_early(dev);
+	else
+		ret = pm_generic_thaw_early(dev);
+
+	return ret;
+}
+
+static int software_node_poweroff_late(struct device *dev)
+{
+	struct swnode_pm_domain *domain = to_swnode_pm_domain(dev->pm_domain);
+	struct swnode *swnode = dev_to_swnode(dev);
+	int ret;
+
+	if (domain->primary && domain->primary->ops.poweroff_late)
+		ret = domain->primary->ops.poweroff_late(dev);
+	else
+		ret = pm_generic_poweroff_late(dev);
+
+	if (ret || !swnode->node->pm->poweroff_late)
+		return ret;
+
+	return swnode->node->pm->poweroff(dev);
+}
+
+static int software_node_restore_early(struct device *dev)
+{
+	struct swnode_pm_domain *domain = to_swnode_pm_domain(dev->pm_domain);
+	struct swnode *swnode = dev_to_swnode(dev);
+	int ret;
+
+	if (swnode->node->pm->restore) {
+		ret = swnode->node->pm->restore_early(dev);
+		if (ret)
+			return ret;
+	}
+
+	if (domain->primary && domain->primary->ops.restore_early)
+		ret = domain->primary->ops.restore_early(dev);
+	else
+		ret = pm_generic_restore_early(dev);
+
+	return ret;
+}
+
+static int software_node_suspend_noirq(struct device *dev)
+{
+	struct swnode_pm_domain *domain = to_swnode_pm_domain(dev->pm_domain);
+	struct swnode *swnode = dev_to_swnode(dev);
+	int ret;
+
+	if (domain->primary && domain->primary->ops.suspend_noirq)
+		ret = domain->primary->ops.suspend_noirq(dev);
+	else
+		ret = pm_generic_suspend_noirq(dev);
+
+	if (ret || !swnode->node->pm->suspend_noirq)
+		return ret;
+
+	return swnode->node->pm->suspend_noirq(dev);
+}
+
+static int software_node_resume_noirq(struct device *dev)
+{
+	struct swnode_pm_domain *domain = to_swnode_pm_domain(dev->pm_domain);
+	struct swnode *swnode = dev_to_swnode(dev);
+	int ret;
+
+	if (swnode->node->pm->resume_noirq) {
+		ret = swnode->node->pm->resume_noirq(dev);
+		if (ret)
+			return ret;
+	}
+
+	if (domain->primary && domain->primary->ops.resume_noirq)
+		ret = domain->primary->ops.resume_noirq(dev);
+	else
+		ret = pm_generic_resume_noirq(dev);
+
+	return ret;
+}
+
+static int software_node_freeze_noirq(struct device *dev)
+{
+	struct swnode_pm_domain *domain = to_swnode_pm_domain(dev->pm_domain);
+	struct swnode *swnode = dev_to_swnode(dev);
+	int ret;
+
+	if (domain->primary && domain->primary->ops.freeze_noirq)
+		ret = domain->primary->ops.freeze_noirq(dev);
+	else
+		ret = pm_generic_freeze_noirq(dev);
+
+	if (ret || !swnode->node->pm->freeze_noirq)
+		return ret;
+
+	return swnode->node->pm->freeze_noirq(dev);
+}
+
+static int software_node_thaw_noirq(struct device *dev)
+{
+	struct swnode_pm_domain *domain = to_swnode_pm_domain(dev->pm_domain);
+	struct swnode *swnode = dev_to_swnode(dev);
+	int ret;
+
+	if (swnode->node->pm->thaw_noirq) {
+		ret = swnode->node->pm->thaw_noirq(dev);
+		if (ret)
+			return ret;
+	}
+
+	if (domain->primary && domain->primary->ops.thaw_noirq)
+		ret = domain->primary->ops.thaw_noirq(dev);
+	else
+		ret = pm_generic_thaw_noirq(dev);
+
+	return ret;
+}
+
+static int software_node_poweroff_noirq(struct device *dev)
+{
+	struct swnode_pm_domain *domain = to_swnode_pm_domain(dev->pm_domain);
+	struct swnode *swnode = dev_to_swnode(dev);
+	int ret;
+
+	if (domain->primary && domain->primary->ops.poweroff_noirq)
+		ret = domain->primary->ops.poweroff_noirq(dev);
+	else
+		ret = pm_generic_poweroff_noirq(dev);
+
+	if (ret || !swnode->node->pm->poweroff)
+		return ret;
+
+	return swnode->node->pm->poweroff_noirq(dev);
+}
+
+static int software_node_restore_noirq(struct device *dev)
+{
+	struct swnode_pm_domain *domain = to_swnode_pm_domain(dev->pm_domain);
+	struct swnode *swnode = dev_to_swnode(dev);
+	int ret;
+
+	if (swnode->node->pm->restore_noirq) {
+		ret = swnode->node->pm->restore_noirq(dev);
+		if (ret)
+			return ret;
+	}
+
+	if (domain->primary && domain->primary->ops.restore_noirq)
+		ret = domain->primary->ops.restore_noirq(dev);
+	else
+		ret = pm_generic_restore_noirq(dev);
+
+	return ret;
+}
+#endif /* CONFIG_PM_SLEEP */
+
+static const struct dev_pm_ops software_node_pm_ops = {
+	.runtime_suspend = software_node_runtime_suspend,
+	.runtime_resume = software_node_runtime_resume,
+	.runtime_idle = software_node_runtime_idle,
+#ifdef CONFIG_PM_SLEEP
+	.prepare = software_node_prepare,
+	.complete = software_node_complete,
+	.suspend = software_node_suspend,
+	.resume = software_node_resume,
+	.freeze = software_node_freeze,
+	.thaw = software_node_thaw,
+	.poweroff = software_node_poweroff,
+	.restore = software_node_restore,
+	.suspend_late = software_node_suspend_late,
+	.resume_early = software_node_resume_early,
+	.freeze_late = software_node_freeze_late,
+	.thaw_early = software_node_thaw_early,
+	.poweroff_late = software_node_poweroff_late,
+	.restore_early = software_node_restore_early,
+	.suspend_noirq = software_node_suspend_noirq,
+	.resume_noirq = software_node_resume_noirq,
+	.freeze_noirq = software_node_freeze_noirq,
+	.thaw_noirq = software_node_thaw_noirq,
+	.poweroff_noirq = software_node_poweroff_noirq,
+	.restore_noirq = software_node_restore_noirq,
+#endif /* CONFIG_PM_SLEEP */
+};
+
+static void software_node_dev_pm_detach(struct device *dev, bool power_off)
+{
+	struct swnode_pm_domain *domain = to_swnode_pm_domain(dev->pm_domain);
+
+	if (domain->primary && domain->primary->detach) {
+		dev->pm_domain = domain->primary;
+		domain->primary->detach(dev, power_off);
+	} else {
+		dev_pm_domain_set(dev, NULL);
+	}
+
+	kfree(domain);
+}
+
+int software_node_dev_pm_attach(struct device *dev, bool power_on)
+{
+	struct swnode *swnode = dev_to_swnode(dev);
+	struct swnode_pm_domain *domain;
+
+	if (!swnode || !swnode->node->pm)
+		return 0;
+
+	domain = kzalloc(sizeof(*domain), GFP_KERNEL);
+	if (!domain)
+		return -ENOMEM;
+
+	if (dev->pm_domain)
+		domain->pm_domain = *dev->pm_domain;
+
+	domain->primary = dev->pm_domain;
+	domain->pm_domain.ops = software_node_pm_ops;
+	domain->pm_domain.detach = software_node_dev_pm_detach;
+
+	dev_pm_domain_set(dev, &domain->pm_domain);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(software_node_dev_pm_attach);
+#endif /* CONFIG_PM */
+
 /* -------------------------------------------------------------------------- */
 /* fwnode operations */
 
@@ -845,20 +1358,13 @@ EXPORT_SYMBOL_GPL(fwnode_remove_software_node);
 
 int software_node_notify(struct device *dev, unsigned long action)
 {
-	struct fwnode_handle *fwnode = dev_fwnode(dev);
 	struct swnode *swnode;
 	int ret;
 
-	if (!fwnode)
-		return 0;
-
-	if (!is_software_node(fwnode))
-		fwnode = fwnode->secondary;
-	if (!is_software_node(fwnode))
+	swnode = dev_to_swnode(dev);
+	if (!swnode)
 		return 0;
 
-	swnode = to_swnode(fwnode);
-
 	switch (action) {
 	case KOBJ_ADD:
 		ret = sysfs_create_link(&dev->kobj, &swnode->kobj,
diff --git a/include/linux/property.h b/include/linux/property.h
index 9f805c4428195..4a2e0edb78275 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -426,11 +426,13 @@ int fwnode_graph_parse_endpoint(const struct fwnode_handle *fwnode,
  * @name: Name of the software node
  * @parent: Parent of the software node
  * @properties: Array of device properties
+ * @pm: Power management operations
  */
 struct software_node {
 	const char *name;
 	const struct software_node *parent;
 	const struct property_entry *properties;
+	const struct dev_pm_ops *pm;
 };
 
 bool is_software_node(const struct fwnode_handle *fwnode);
@@ -452,6 +454,14 @@ int software_node_register(const struct software_node *node);
 void software_node_unregister(const struct software_node *node);
 
 int software_node_notify(struct device *dev, unsigned long action);
+#ifdef CONFIG_PM
+int software_node_dev_pm_attach(struct device *dev, bool power_on);
+#else
+static inline int software_node_dev_pm_attach(struct device *dev, bool power_on)
+{
+	return 0;
+}
+#endif /* CONFIG_PM */
 
 struct fwnode_handle *
 fwnode_create_software_node(const struct property_entry *properties,
-- 
2.28.0

Re: [PATCH 1/3] software node: Power management operations for software nodes

[Rafael J. Wysocki]

On Tue, Aug 25, 2020 at 3:59 PM Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
>
> Adding separate PM operations vector for the software nodes.
> 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).

This isn't consistent with the code changes AFAICS.

The swnode PM operations are implemented as a PM domain ops, which
means that they will be executed instead of any other existing ops
rather than in addition to those.

For example, software_node_prepare() will skip bus type ops if they
are present and there is no "primary" PM domain which seems not
intended.

Also some comments might help to understand the design.

Cheers!

Re: [PATCH 1/3] software node: Power management operations for software nodes

[Heikki Krogerus]

On Fri, Sep 25, 2020 at 05:54:37PM +0200, Rafael J. Wysocki wrote:
> On Tue, Aug 25, 2020 at 3:59 PM Heikki Krogerus
> <heikki.krogerus@linux.intel.com> wrote:
> >
> > Adding separate PM operations vector for the software nodes.
> > 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).
> 
> This isn't consistent with the code changes AFAICS.
> 
> The swnode PM operations are implemented as a PM domain ops, which
> means that they will be executed instead of any other existing ops
> rather than in addition to those.
> 
> For example, software_node_prepare() will skip bus type ops if they
> are present and there is no "primary" PM domain which seems not
> intended.

True. I thought the pm_generic_*() functions will take care of also
the bus ops, but of course they don't do that.

> Also some comments might help to understand the design.

OK.

thanks,

-- 
heikki

[PATCH 2/3] software node: Introduce device_add_software_node()

[Heikki Krogerus]

This helper will register a software node and then assign
it to device at the same time. The function will also make
sure that the device can't have more than one software node.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/base/swnode.c    | 45 ++++++++++++++++++++++++++++++++++++++++
 include/linux/property.h |  3 +++
 2 files changed, 48 insertions(+)

diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index 9a13a61a2f0bb..ec14f02565a65 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -1356,6 +1356,51 @@ void fwnode_remove_software_node(struct fwnode_handle *fwnode)
 }
 EXPORT_SYMBOL_GPL(fwnode_remove_software_node);
 
+/**
+ * device_add_software_node - Assign software node to a device
+ * @dev: The device the software node is meant for.
+ * @swnode: The software node.
+ *
+ * This function will register @swnode and make it the secondary firmware node
+ * pointer of @dev. If @dev has no primary node, then @swnode will become the primary
+ * node.
+ */
+int device_add_software_node(struct device *dev, const struct software_node *swnode)
+{
+	int ret;
+
+	/* Only one software node per device. */
+	if (dev_to_swnode(dev))
+		return -EBUSY;
+
+	ret = software_node_register(swnode);
+	if (ret)
+		return ret;
+
+	set_secondary_fwnode(dev, software_node_fwnode(swnode));
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(device_add_software_node);
+
+/**
+ * device_remove_software_node - Remove device's software node
+ * @dev: The device with the software node.
+ *
+ * This function will unregister the software node of @dev.
+ */
+void device_remove_software_node(struct device *dev)
+{
+	struct swnode *swnode;
+
+	swnode = dev_to_swnode(dev);
+	if (!swnode)
+		return;
+
+	kobject_put(&swnode->kobj);
+}
+EXPORT_SYMBOL_GPL(device_remove_software_node);
+
 int software_node_notify(struct device *dev, unsigned long action)
 {
 	struct swnode *swnode;
diff --git a/include/linux/property.h b/include/linux/property.h
index 4a2e0edb78275..5f84286183f9a 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -468,4 +468,7 @@ fwnode_create_software_node(const struct property_entry *properties,
 			    const struct fwnode_handle *parent);
 void fwnode_remove_software_node(struct fwnode_handle *fwnode);
 
+int device_add_software_node(struct device *dev, const struct software_node *swnode);
+void device_remove_software_node(struct device *dev);
+
 #endif /* _LINUX_PROPERTY_H_ */
-- 
2.28.0

Re: [PATCH 2/3] software node: Introduce device_add_software_node()

[Andy Shevchenko]

On Tue, Aug 25, 2020 at 04:59:50PM +0300, Heikki Krogerus wrote:
> This helper will register a software node and then assign
> it to device at the same time. The function will also make
> sure that the device can't have more than one software node.

> +	/* Only one software node per device. */
> +	if (dev_to_swnode(dev))
> +		return -EBUSY;

> +	swnode = dev_to_swnode(dev);
> +	if (!swnode)
> +		return;

Hmm... I didn't find implementation of dev_to_swnode().
What did I miss?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/3] software node: Introduce device_add_software_node()

[Andy Shevchenko]

On Tue, Aug 25, 2020 at 05:32:28PM +0300, Andy Shevchenko wrote:
> On Tue, Aug 25, 2020 at 04:59:50PM +0300, Heikki Krogerus wrote:
> > This helper will register a software node and then assign
> > it to device at the same time. The function will also make
> > sure that the device can't have more than one software node.
> 
> > +	/* Only one software node per device. */
> > +	if (dev_to_swnode(dev))
> > +		return -EBUSY;
> 
> > +	swnode = dev_to_swnode(dev);
> > +	if (!swnode)
> > +		return;
> 
> Hmm... I didn't find implementation of dev_to_swnode().
> What did I miss?

Now found, I was looking into header, but it's defined in the c-file in
previous patch.

-- 
With Best Regards,
Andy Shevchenko

[PATCH 3/3] usb: dwc3: pci: Register a software node for the dwc3 platform device

[Heikki Krogerus]

By registering the software node directly instead of just
the properties in it, the driver can take advantage of also
the other features the software nodes have.

Initially using the nodes for isolating the Intel Broxton
specific power management quirk by handling it in Broxton's
very own power management operations (which are supplied as
part of the software node) instead of the drivers generic
ones.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/usb/dwc3/dwc3-pci.c | 175 ++++++++++++++++++------------------
 1 file changed, 86 insertions(+), 89 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-pci.c b/drivers/usb/dwc3/dwc3-pci.c
index 242b6210380a4..43cc0f602820d 100644
--- a/drivers/usb/dwc3/dwc3-pci.c
+++ b/drivers/usb/dwc3/dwc3-pci.c
@@ -54,17 +54,12 @@
  * struct dwc3_pci - Driver private structure
  * @dwc3: child dwc3 platform_device
  * @pci: our link to PCI bus
- * @guid: _DSM GUID
- * @has_dsm_for_pm: true for devices which need to run _DSM on runtime PM
  * @wakeup_work: work for asynchronous resume
  */
 struct dwc3_pci {
 	struct platform_device *dwc3;
 	struct pci_dev *pci;
 
-	guid_t guid;
-
-	unsigned int has_dsm_for_pm:1;
 	struct work_struct wakeup_work;
 };
 
@@ -108,6 +103,50 @@ static int dwc3_byt_enable_ulpi_refclock(struct pci_dev *pci)
 	return 0;
 }
 
+#ifdef CONFIG_PM
+static int dwc3_pci_intel_pm_dsm(struct device *dev, int param)
+{
+	union acpi_object *obj;
+	union acpi_object tmp;
+	union acpi_object argv4 = ACPI_INIT_DSM_ARGV4(1, &tmp);
+	guid_t guid;
+	int ret;
+
+	ret = guid_parse(PCI_INTEL_BXT_DSM_GUID, &guid);
+	if (ret)
+		return ret;
+
+	tmp.type = ACPI_TYPE_INTEGER;
+	tmp.integer.value = param;
+
+	obj = acpi_evaluate_dsm(ACPI_HANDLE(dev), &guid,
+				1, PCI_INTEL_BXT_FUNC_PMU_PWR, &argv4);
+	if (!obj) {
+		dev_err(dev, "failed to evaluate _DSM\n");
+		return -EIO;
+	}
+
+	ACPI_FREE(obj);
+
+	return 0;
+}
+
+static int dwc3_pci_intel_suspend(struct device *dev)
+{
+	return dwc3_pci_intel_pm_dsm(dev->parent, PCI_INTEL_BXT_STATE_D3);
+}
+
+static int dwc3_pci_intel_resume(struct device *dev)
+{
+	return dwc3_pci_intel_pm_dsm(dev->parent, PCI_INTEL_BXT_STATE_D0);
+}
+#endif /* CONFIG_PM */
+
+static const struct dev_pm_ops dwc3_pci_intel_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(dwc3_pci_intel_suspend, dwc3_pci_intel_resume)
+	SET_RUNTIME_PM_OPS(dwc3_pci_intel_suspend, dwc3_pci_intel_resume, NULL)
+};
+
 static const struct property_entry dwc3_pci_intel_properties[] = {
 	PROPERTY_ENTRY_STRING("dr_mode", "peripheral"),
 	PROPERTY_ENTRY_BOOL("linux,sysdev_is_parent"),
@@ -141,18 +180,28 @@ static const struct property_entry dwc3_pci_amd_properties[] = {
 	{}
 };
 
+static const struct software_node dwc3_pci_intel_swnode = {
+	.properties = dwc3_pci_intel_properties,
+};
+
+static const struct software_node dwc3_pci_intel_pm_swnode = {
+	.properties = dwc3_pci_intel_properties,
+	.pm = &dwc3_pci_intel_pm_ops,
+};
+
+static const struct software_node dwc3_pci_intel_mrfld_swnode = {
+	.properties = dwc3_pci_mrfld_properties,
+};
+
+static const struct software_node dwc3_pci_amd_swnode = {
+	.properties = dwc3_pci_amd_properties,
+};
+
 static int dwc3_pci_quirks(struct dwc3_pci *dwc)
 {
 	struct pci_dev			*pdev = dwc->pci;
 
 	if (pdev->vendor == PCI_VENDOR_ID_INTEL) {
-		if (pdev->device == PCI_DEVICE_ID_INTEL_BXT ||
-		    pdev->device == PCI_DEVICE_ID_INTEL_BXT_M ||
-		    pdev->device == PCI_DEVICE_ID_INTEL_EHLLP) {
-			guid_parse(PCI_INTEL_BXT_DSM_GUID, &dwc->guid);
-			dwc->has_dsm_for_pm = true;
-		}
-
 		if (pdev->device == PCI_DEVICE_ID_INTEL_BYT) {
 			struct gpio_desc *gpio;
 			int ret;
@@ -221,7 +270,6 @@ static void dwc3_pci_resume_work(struct work_struct *work)
 
 static int dwc3_pci_probe(struct pci_dev *pci, const struct pci_device_id *id)
 {
-	struct property_entry *p = (struct property_entry *)id->driver_data;
 	struct dwc3_pci		*dwc;
 	struct resource		res[2];
 	int			ret;
@@ -264,7 +312,7 @@ static int dwc3_pci_probe(struct pci_dev *pci, const struct pci_device_id *id)
 	dwc->dwc3->dev.parent = dev;
 	ACPI_COMPANION_SET(&dwc->dwc3->dev, ACPI_COMPANION(dev));
 
-	ret = platform_device_add_properties(dwc->dwc3, p);
+	ret = device_add_software_node(&dwc->dwc3->dev, (void *)id->driver_data);
 	if (ret < 0)
 		goto err;
 
@@ -287,6 +335,7 @@ static int dwc3_pci_probe(struct pci_dev *pci, const struct pci_device_id *id)
 
 	return 0;
 err:
+	device_remove_software_node(&dwc->dwc3->dev);
 	platform_device_put(dwc->dwc3);
 	return ret;
 }
@@ -303,121 +352,86 @@ static void dwc3_pci_remove(struct pci_dev *pci)
 #endif
 	device_init_wakeup(&pci->dev, false);
 	pm_runtime_get(&pci->dev);
+	device_remove_software_node(&dwc->dwc3->dev);
 	platform_device_unregister(dwc->dwc3);
 }
 
 static const struct pci_device_id dwc3_pci_id_table[] = {
 	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_BSW),
-	  (kernel_ulong_t) &dwc3_pci_intel_properties },
+	  (kernel_ulong_t) &dwc3_pci_intel_swnode, },
 
 	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_BYT),
-	  (kernel_ulong_t) &dwc3_pci_intel_properties, },
+	  (kernel_ulong_t) &dwc3_pci_intel_swnode, },
 
 	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_MRFLD),
-	  (kernel_ulong_t) &dwc3_pci_mrfld_properties, },
+	  (kernel_ulong_t) &dwc3_pci_intel_mrfld_swnode, },
 
 	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_CMLLP),
-	  (kernel_ulong_t) &dwc3_pci_intel_properties, },
+	  (kernel_ulong_t) &dwc3_pci_intel_swnode, },
 
 	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_CMLH),
-	  (kernel_ulong_t) &dwc3_pci_intel_properties, },
+	  (kernel_ulong_t) &dwc3_pci_intel_swnode, },
 
 	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_SPTLP),
-	  (kernel_ulong_t) &dwc3_pci_intel_properties, },
+	  (kernel_ulong_t) &dwc3_pci_intel_swnode, },
 
 	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_SPTH),
-	  (kernel_ulong_t) &dwc3_pci_intel_properties, },
+	  (kernel_ulong_t) &dwc3_pci_intel_swnode, },
 
 	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_BXT),
-	  (kernel_ulong_t) &dwc3_pci_intel_properties, },
+	  (kernel_ulong_t) &dwc3_pci_intel_pm_swnode, },
 
 	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_BXT_M),
-	  (kernel_ulong_t) &dwc3_pci_intel_properties, },
+	  (kernel_ulong_t) &dwc3_pci_intel_pm_swnode, },
 
 	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_APL),
-	  (kernel_ulong_t) &dwc3_pci_intel_properties, },
+	  (kernel_ulong_t) &dwc3_pci_intel_swnode, },
 
 	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_KBP),
-	  (kernel_ulong_t) &dwc3_pci_intel_properties, },
+	  (kernel_ulong_t) &dwc3_pci_intel_swnode, },
 
 	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_GLK),
-	  (kernel_ulong_t) &dwc3_pci_intel_properties, },
+	  (kernel_ulong_t) &dwc3_pci_intel_swnode, },
 
 	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_CNPLP),
-	  (kernel_ulong_t) &dwc3_pci_intel_properties, },
+	  (kernel_ulong_t) &dwc3_pci_intel_swnode, },
 
 	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_CNPH),
-	  (kernel_ulong_t) &dwc3_pci_intel_properties, },
+	  (kernel_ulong_t) &dwc3_pci_intel_swnode, },
 
 	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_CNPV),
-	  (kernel_ulong_t) &dwc3_pci_intel_properties, },
+	  (kernel_ulong_t) &dwc3_pci_intel_swnode, },
 
 	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_ICLLP),
-	  (kernel_ulong_t) &dwc3_pci_intel_properties, },
+	  (kernel_ulong_t) &dwc3_pci_intel_swnode, },
 
 	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_EHLLP),
-	  (kernel_ulong_t) &dwc3_pci_intel_properties, },
+	  (kernel_ulong_t) &dwc3_pci_intel_pm_swnode },
 
 	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_TGPLP),
-	  (kernel_ulong_t) &dwc3_pci_intel_properties, },
+	  (kernel_ulong_t) &dwc3_pci_intel_swnode, },
 
 	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_TGPH),
-	  (kernel_ulong_t) &dwc3_pci_intel_properties, },
+	  (kernel_ulong_t) &dwc3_pci_intel_swnode, },
 
 	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_JSP),
-	  (kernel_ulong_t) &dwc3_pci_intel_properties, },
+	  (kernel_ulong_t) &dwc3_pci_intel_swnode, },
 
 	{ PCI_VDEVICE(AMD, PCI_DEVICE_ID_AMD_NL_USB),
-	  (kernel_ulong_t) &dwc3_pci_amd_properties, },
+	  (kernel_ulong_t) &dwc3_pci_amd_swnode, },
 	{  }	/* Terminating Entry */
 };
 MODULE_DEVICE_TABLE(pci, dwc3_pci_id_table);
 
-#if defined(CONFIG_PM) || defined(CONFIG_PM_SLEEP)
-static int dwc3_pci_dsm(struct dwc3_pci *dwc, int param)
-{
-	union acpi_object *obj;
-	union acpi_object tmp;
-	union acpi_object argv4 = ACPI_INIT_DSM_ARGV4(1, &tmp);
-
-	if (!dwc->has_dsm_for_pm)
-		return 0;
-
-	tmp.type = ACPI_TYPE_INTEGER;
-	tmp.integer.value = param;
-
-	obj = acpi_evaluate_dsm(ACPI_HANDLE(&dwc->pci->dev), &dwc->guid,
-			1, PCI_INTEL_BXT_FUNC_PMU_PWR, &argv4);
-	if (!obj) {
-		dev_err(&dwc->pci->dev, "failed to evaluate _DSM\n");
-		return -EIO;
-	}
-
-	ACPI_FREE(obj);
-
-	return 0;
-}
-#endif /* CONFIG_PM || CONFIG_PM_SLEEP */
-
 #ifdef CONFIG_PM
 static int dwc3_pci_runtime_suspend(struct device *dev)
 {
-	struct dwc3_pci		*dwc = dev_get_drvdata(dev);
-
-	if (device_can_wakeup(dev))
-		return dwc3_pci_dsm(dwc, PCI_INTEL_BXT_STATE_D3);
-
-	return -EBUSY;
+	return device_can_wakeup(dev) ? 0 : -EBUSY;
 }
 
 static int dwc3_pci_runtime_resume(struct device *dev)
 {
 	struct dwc3_pci		*dwc = dev_get_drvdata(dev);
-	int			ret;
-
-	ret = dwc3_pci_dsm(dwc, PCI_INTEL_BXT_STATE_D0);
-	if (ret)
-		return ret;
 
 	queue_work(pm_wq, &dwc->wakeup_work);
 
@@ -425,24 +439,7 @@ static int dwc3_pci_runtime_resume(struct device *dev)
 }
 #endif /* CONFIG_PM */
 
-#ifdef CONFIG_PM_SLEEP
-static int dwc3_pci_suspend(struct device *dev)
-{
-	struct dwc3_pci		*dwc = dev_get_drvdata(dev);
-
-	return dwc3_pci_dsm(dwc, PCI_INTEL_BXT_STATE_D3);
-}
-
-static int dwc3_pci_resume(struct device *dev)
-{
-	struct dwc3_pci		*dwc = dev_get_drvdata(dev);
-
-	return dwc3_pci_dsm(dwc, PCI_INTEL_BXT_STATE_D0);
-}
-#endif /* CONFIG_PM_SLEEP */
-
 static const struct dev_pm_ops dwc3_pci_dev_pm_ops = {
-	SET_SYSTEM_SLEEP_PM_OPS(dwc3_pci_suspend, dwc3_pci_resume)
 	SET_RUNTIME_PM_OPS(dwc3_pci_runtime_suspend, dwc3_pci_runtime_resume,
 		NULL)
 };
-- 
2.28.0

Re: [PATCH 0/3] PM operations for software nodes

[Andy Shevchenko]

On Tue, Aug 25, 2020 at 04:59:48PM +0300, Heikki Krogerus wrote:
> Hi guys,
> 
> Unfortunately the last patch depends on this:
> https://lore.kernel.org/linux-usb/20200821131101.81915-1-heikki.krogerus@linux.intel.com/
> 
> Would it be easiest that Felipe took care of these (assuming they are
> acceptable)?

LGTM, FWIW,
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> thanks,
> 
> Heikki Krogerus (3):
>   software node: Power management operations for software nodes
>   software node: Introduce device_add_software_node()
>   usb: dwc3: pci: Register a software node for the dwc3 platform device
> 
>  drivers/base/power/common.c |   8 +-
>  drivers/base/swnode.c       | 569 +++++++++++++++++++++++++++++++++++-
>  drivers/usb/dwc3/dwc3-pci.c | 175 ++++++-----
>  include/linux/property.h    |  13 +
>  4 files changed, 666 insertions(+), 99 deletions(-)
> 
> -- 
> 2.28.0
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/3] PM operations for software nodes

[Heikki Krogerus]

+Sakari

On Tue, Aug 25, 2020 at 04:59:48PM +0300, Heikki Krogerus wrote:
> Hi guys,
> 
> Unfortunately the last patch depends on this:
> https://lore.kernel.org/linux-usb/20200821131101.81915-1-heikki.krogerus@linux.intel.com/
> 
> Would it be easiest that Felipe took care of these (assuming they are
> acceptable)?
> 
> thanks,
> 
> Heikki Krogerus (3):
>   software node: Power management operations for software nodes
>   software node: Introduce device_add_software_node()
>   usb: dwc3: pci: Register a software node for the dwc3 platform device
> 
>  drivers/base/power/common.c |   8 +-
>  drivers/base/swnode.c       | 569 +++++++++++++++++++++++++++++++++++-
>  drivers/usb/dwc3/dwc3-pci.c | 175 ++++++-----
>  include/linux/property.h    |  13 +
>  4 files changed, 666 insertions(+), 99 deletions(-)
> 
> -- 
> 2.28.0

-- 
heikki