[FOR DISCUSSION 0/8] Dove PMU support

[FOR DISCUSSION 0/8] Dove PMU support

[Russell King - ARM Linux]

This is a re-posting of the patch set which I posted almost 10 months
ago to support the Dove PMU, with a few additional changes.  This set
is based upon 3.19.

In this set are:

* two patches which Rafael originally acked, but there was indecision
  last time around how to handle them due to potential conflicts with
  work that Ulf was doing.  These patches have been updated to apply
  cleanly to 3.19.  I don't know if people want to take these as
  fixes to the PM domain code or not (hence why I'm posting this
  series during the merge window - if it weren't for this, I'd hold
  it off.)

* what I regard as a fix to the PM domain code; as a result of a
  previous commit, the PM domain code mismatches the runtime PM state,
  which leads to the PM domain being unexpectedly left on.  This patch
  works around that.  (It's been sent recently as well but in an old
  thread.)

* the addition of the core Dove PMU driver, which consists of a reset,
  IRQ controller, and power domains.  The reset and power domain code
  has to be closely related due to the power up/down requirements of
  the GPU/VPU subsystems needing to be performed atomically.  (This
  requirement prevents it using the MFD infrastructure, because we
  would need to hold spinlocks while calling several different
  sub-drivers.)

* addition of the RTC interrupt, so we can now receive and act on
  alarms generated by the Dove RTC.

* addition of the DT descriptions for the GPU and VPU power domains.
  These patches do not themselves add the DT descriptions for these
  units, so these patches serve as illustrations how these should be
  described.

There are a few things which are missing from this driver, such sa the
DT binding documentation.  This will follow once people are happy with
the first four patches, in particular where to locate the PMU driver
in the kernel tree.  Currently, I have it in arch/arm/mach-dove, which
is where the legacy Dove code lives, so it's not ideal.

There are some gotchas with it - PM domains need to be created prior
to any device which uses them being probed, so it is best if the driver
is initialised really early in the kernel boot.  We may be able to get
away with a core_initcall() or a postcore_initcall().

I'd ideally like to get these queued for merging in the _next_ merge
window if at all possible, if only to reduce the number of patches I've
been carrying for the last few years.

 arch/arm/Kconfig                     |   1 +
 arch/arm/boot/dts/dove.dtsi          |  25 +++
 arch/arm/mach-dove/Makefile          |   1 +
 arch/arm/mach-dove/common.c          |   2 +
 arch/arm/mach-dove/common.h          |   1 +
 arch/arm/mach-dove/include/mach/pm.h |  17 --
 arch/arm/mach-dove/irq.c             |  87 --------
 arch/arm/mach-dove/pmu.c             | 410 +++++++++++++++++++++++++++++++++++
 drivers/base/power/domain.c          |  26 ++-
 drivers/base/power/runtime.c         |   5 +
 include/linux/pm.h                   |   1 +
 11 files changed, 466 insertions(+), 110 deletions(-)

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

[FOR DISCUSSION 0/8] Dove PMU support

[Russell King - ARM Linux]

This is a re-posting of the patch set which I posted almost 10 months
ago to support the Dove PMU, with a few additional changes.  This set
is based upon 3.19.

In this set are:

* two patches which Rafael originally acked, but there was indecision
  last time around how to handle them due to potential conflicts with
  work that Ulf was doing.  These patches have been updated to apply
  cleanly to 3.19.  I don't know if people want to take these as
  fixes to the PM domain code or not (hence why I'm posting this
  series during the merge window - if it weren't for this, I'd hold
  it off.)

* what I regard as a fix to the PM domain code; as a result of a
  previous commit, the PM domain code mismatches the runtime PM state,
  which leads to the PM domain being unexpectedly left on.  This patch
  works around that.  (It's been sent recently as well but in an old
  thread.)

* the addition of the core Dove PMU driver, which consists of a reset,
  IRQ controller, and power domains.  The reset and power domain code
  has to be closely related due to the power up/down requirements of
  the GPU/VPU subsystems needing to be performed atomically.  (This
  requirement prevents it using the MFD infrastructure, because we
  would need to hold spinlocks while calling several different
  sub-drivers.)

* addition of the RTC interrupt, so we can now receive and act on
  alarms generated by the Dove RTC.

* addition of the DT descriptions for the GPU and VPU power domains.
  These patches do not themselves add the DT descriptions for these
  units, so these patches serve as illustrations how these should be
  described.

There are a few things which are missing from this driver, such sa the
DT binding documentation.  This will follow once people are happy with
the first four patches, in particular where to locate the PMU driver
in the kernel tree.  Currently, I have it in arch/arm/mach-dove, which
is where the legacy Dove code lives, so it's not ideal.

There are some gotchas with it - PM domains need to be created prior
to any device which uses them being probed, so it is best if the driver
is initialised really early in the kernel boot.  We may be able to get
away with a core_initcall() or a postcore_initcall().

I'd ideally like to get these queued for merging in the _next_ merge
window if at all possible, if only to reduce the number of patches I've
been carrying for the last few years.

 arch/arm/Kconfig                     |   1 +
 arch/arm/boot/dts/dove.dtsi          |  25 +++
 arch/arm/mach-dove/Makefile          |   1 +
 arch/arm/mach-dove/common.c          |   2 +
 arch/arm/mach-dove/common.h          |   1 +
 arch/arm/mach-dove/include/mach/pm.h |  17 --
 arch/arm/mach-dove/irq.c             |  87 --------
 arch/arm/mach-dove/pmu.c             | 410 +++++++++++++++++++++++++++++++++++
 drivers/base/power/domain.c          |  26 ++-
 drivers/base/power/runtime.c         |   5 +
 include/linux/pm.h                   |   1 +
 11 files changed, 466 insertions(+), 110 deletions(-)

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

[PATCH 1/8] pm: domains: quieten down generic pm domains

[Russell King]

PM domains are rather noisy; scheduling behaviour can cause callbacks
to take longer, which causes them to spit out a warning-level message
each time a callback takes a little longer than the previous time.
There really isn't a need for this, except when debugging.

Acked-by: Rafael J. Wysocki <rjw@rjwysocki.net>
Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/base/power/domain.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 0d8780c04a5e..b3fbc21da2dc 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -173,8 +173,8 @@ static int genpd_power_on(struct generic_pm_domain *genpd)
 	genpd->power_on_latency_ns = elapsed_ns;
 	genpd->max_off_time_changed = true;
 	genpd_recalc_cpu_exit_latency(genpd);
-	pr_warn("%s: Power-%s latency exceeded, new value %lld ns\n",
-		genpd->name, "on", elapsed_ns);
+	pr_debug("%s: Power-%s latency exceeded, new value %lld ns\n",
+		 genpd->name, "on", elapsed_ns);
 
 	return ret;
 }
@@ -199,8 +199,8 @@ static int genpd_power_off(struct generic_pm_domain *genpd)
 
 	genpd->power_off_latency_ns = elapsed_ns;
 	genpd->max_off_time_changed = true;
-	pr_warn("%s: Power-%s latency exceeded, new value %lld ns\n",
-		genpd->name, "off", elapsed_ns);
+	pr_debug("%s: Power-%s latency exceeded, new value %lld ns\n",
+		 genpd->name, "off", elapsed_ns);
 
 	return ret;
 }
-- 
1.8.3.1

[PATCH 2/8] pm: domains: avoid potential oops in pm_genpd_remove_device()

[Russell King]

pm_genpd_remove_device() should only be called with valid and present
pm domain.  There are circumstances where we may end up with something
that isn't a generic PM domain in dev->pm_domain (eg, vga_switcheroo
stuff.)

Acked-by: Rafael J. Wysocki <rjw@rjwysocki.net>
Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/base/power/domain.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index b3fbc21da2dc..11a1023fa64a 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -1509,7 +1509,7 @@ int pm_genpd_remove_device(struct generic_pm_domain *genpd,
 
 	dev_dbg(dev, "%s()\n", __func__);
 
-	if (IS_ERR_OR_NULL(genpd) || IS_ERR_OR_NULL(dev)
+	if (!pm_genpd_present(genpd) || IS_ERR_OR_NULL(dev)
 	    ||  IS_ERR_OR_NULL(dev->pm_domain)
 	    ||  pd_to_genpd(dev->pm_domain) != genpd)
 		return -EINVAL;
-- 
1.8.3.1

[PATCH 3/8] pm: domains: sync runtime PM status with PM domains

[Russell King]

Synchronise the PM domain status with runtime PM's status.  This
provides a better solution to the problem than commit 2ed127697eb1
("PM / Domains: Power on the PM domain right after attach completes").

The above commit added a call to power up the PM domain when a device
attaches to the domain.  The assumption is that the device driver
will cause a runtime PM transition, which will synchronise the PM
domain state with the runtime PM state.

However, runtime PM will, by default, assume that the device is
initially suspended.  So we have two subsystems which have differing
initial state expectations.  This is silly.

Runtime PM requires that pm_runtime_set_active() is called before
pm_runtime_enable() if the device is already powered up.  However, PM
domains are not informed of this, and this is where the problem really
lies.  We need to keep PM domains properly and fully informed of their
associated device status.

We fix this by adding a new callback - runtime_set_status() which is
triggered whenever a successful call to __pm_runtime_set_status().
PM domain code hooks into this, and updates the PM domain appropriately.

This means a driver with the following sequence:

	pm_runtime_set_active(dev);
	pm_runtime_enable(dev);

will trigger the PM domain to be powered up at this point, which keeps
runtime PM and PM domains properly in sync with each other.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/base/power/domain.c  | 16 +++++++++++++++-
 drivers/base/power/runtime.c |  5 +++++
 include/linux/pm.h           |  1 +
 3 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 11a1023fa64a..2a700cea41fc 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -741,6 +741,20 @@ static int pm_genpd_runtime_resume(struct device *dev)
 	return 0;
 }
 
+static int pm_genpd_runtime_set_status(struct device *dev)
+{
+	int ret;
+
+	dev_dbg(dev, "%s()\n", __func__);
+
+	if (pm_runtime_suspended(dev))
+		ret = pm_genpd_runtime_suspend(dev);
+	else
+		ret = pm_genpd_runtime_resume(dev);
+
+	return ret;
+}
+
 static bool pd_ignore_unused;
 static int __init pd_ignore_unused_setup(char *__unused)
 {
@@ -1907,6 +1921,7 @@ void pm_genpd_init(struct generic_pm_domain *genpd,
 	genpd->max_off_time_changed = true;
 	genpd->domain.ops.runtime_suspend = pm_genpd_runtime_suspend;
 	genpd->domain.ops.runtime_resume = pm_genpd_runtime_resume;
+	genpd->domain.ops.runtime_set_status = pm_genpd_runtime_set_status;
 	genpd->domain.ops.prepare = pm_genpd_prepare;
 	genpd->domain.ops.suspend = pm_genpd_suspend;
 	genpd->domain.ops.suspend_late = pm_genpd_suspend_late;
@@ -2223,7 +2238,6 @@ int genpd_dev_pm_attach(struct device *dev)
 	}
 
 	dev->pm_domain->detach = genpd_dev_pm_detach;
-	pm_genpd_poweron(pd);
 
 	return 0;
 }
diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
index 5070c4fe8542..a958cae67a37 100644
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -981,6 +981,7 @@ int __pm_runtime_set_status(struct device *dev, unsigned int status)
 	struct device *parent = dev->parent;
 	unsigned long flags;
 	bool notify_parent = false;
+	pm_callback_t callback;
 	int error = 0;
 
 	if (status != RPM_ACTIVE && status != RPM_SUSPENDED)
@@ -1029,6 +1030,10 @@ int __pm_runtime_set_status(struct device *dev, unsigned int status)
  out_set:
 	__update_runtime_status(dev, status);
 	dev->power.runtime_error = 0;
+	if (dev->power.no_callbacks)
+		goto out;
+	callback = RPM_GET_CALLBACK(dev, runtime_set_status);
+	rpm_callback(callback, dev);
  out:
 	spin_unlock_irqrestore(&dev->power.lock, flags);
 
diff --git a/include/linux/pm.h b/include/linux/pm.h
index 8b5976364619..ee098585d577 100644
--- a/include/linux/pm.h
+++ b/include/linux/pm.h
@@ -316,6 +316,7 @@ struct dev_pm_ops {
 	int (*runtime_suspend)(struct device *dev);
 	int (*runtime_resume)(struct device *dev);
 	int (*runtime_idle)(struct device *dev);
+	int (*runtime_set_status)(struct device *dev);
 };
 
 #ifdef CONFIG_PM_SLEEP
-- 
1.8.3.1

[PATCH 4/8] ARM: dove: create a proper PMU driver for power domains, PMU IRQs and resets

[Russell King]

The PMU device contains an interrupt controller, power control and
resets.  The interrupt controller is a little sub-standard in that
there is no race free way to clear down pending interrupts, so we try
to avoid problems by reducing the window as much as possible, and
clearing as infrequently as possible.

The interrupt support is implemented using an IRQ domain, and the
parent interrupt referenced in the standard DT way.

The power domains and reset support is closely related - there is a
defined sequence for powering down a domain which is tightly coupled
with asserting the reset.  Hence, it makes sense to group these two
together.

This patch adds the core PMU driver: power domains must be defined in
the DT file in order to make use of them.  The reset controller can
be referenced in the standard way for reset controllers.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 arch/arm/Kconfig                     |   1 +
 arch/arm/mach-dove/Makefile          |   1 +
 arch/arm/mach-dove/common.c          |   2 +
 arch/arm/mach-dove/common.h          |   1 +
 arch/arm/mach-dove/include/mach/pm.h |  17 --
 arch/arm/mach-dove/irq.c             |  87 --------
 arch/arm/mach-dove/pmu.c             | 410 +++++++++++++++++++++++++++++++++++
 7 files changed, 415 insertions(+), 104 deletions(-)
 create mode 100644 arch/arm/mach-dove/pmu.c

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 97d07ed60a0b..08e7608d1c52 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -519,6 +519,7 @@ config ARCH_DOVE
 	select PINCTRL
 	select PINCTRL_DOVE
 	select PLAT_ORION_LEGACY
+	select PM_GENERIC_DOMAINS if PM
 	help
 	  Support for the Marvell Dove SoC 88AP510
 
diff --git a/arch/arm/mach-dove/Makefile b/arch/arm/mach-dove/Makefile
index b608a21919fb..b039b91c97b9 100644
--- a/arch/arm/mach-dove/Makefile
+++ b/arch/arm/mach-dove/Makefile
@@ -1,5 +1,6 @@
 obj-y				+= common.o
 obj-$(CONFIG_DOVE_LEGACY)	+= irq.o mpp.o
 obj-$(CONFIG_PCI)		+= pcie.o
+obj-$(CONFIG_PM_GENERIC_DOMAINS)+= pmu.o
 obj-$(CONFIG_MACH_DOVE_DB)	+= dove-db-setup.o
 obj-$(CONFIG_MACH_CM_A510)	+= cm-a510.o
diff --git a/arch/arm/mach-dove/common.c b/arch/arm/mach-dove/common.c
index 0d1a89298ece..195871c87819 100644
--- a/arch/arm/mach-dove/common.c
+++ b/arch/arm/mach-dove/common.c
@@ -377,6 +377,8 @@ void __init dove_setup_cpu_wins(void)
 
 void __init dove_init(void)
 {
+	dove_init_pmu();
+
 	pr_info("Dove 88AP510 SoC, TCLK = %d MHz.\n",
 		(dove_tclk + 499999) / 1000000);
 
diff --git a/arch/arm/mach-dove/common.h b/arch/arm/mach-dove/common.h
index 1d725224d146..3e4a4178efa6 100644
--- a/arch/arm/mach-dove/common.h
+++ b/arch/arm/mach-dove/common.h
@@ -25,6 +25,7 @@ void dove_map_io(void);
 void dove_init(void);
 void dove_init_early(void);
 void dove_init_irq(void);
+int dove_init_pmu(void);
 void dove_setup_cpu_wins(void);
 void dove_ge00_init(struct mv643xx_eth_platform_data *eth_data);
 void dove_sata_init(struct mv_sata_platform_data *sata_data);
diff --git a/arch/arm/mach-dove/include/mach/pm.h b/arch/arm/mach-dove/include/mach/pm.h
index b47f75038686..625a89c15c1f 100644
--- a/arch/arm/mach-dove/include/mach/pm.h
+++ b/arch/arm/mach-dove/include/mach/pm.h
@@ -51,22 +51,5 @@
 #define  CLOCK_GATING_GIGA_PHY_MASK	(1 << CLOCK_GATING_BIT_GIGA_PHY)
 
 #define PMU_INTERRUPT_CAUSE	(DOVE_PMU_VIRT_BASE + 0x50)
-#define PMU_INTERRUPT_MASK	(DOVE_PMU_VIRT_BASE + 0x54)
-
-static inline int pmu_to_irq(int pin)
-{
-	if (pin < NR_PMU_IRQS)
-		return pin + IRQ_DOVE_PMU_START;
-
-	return -EINVAL;
-}
-
-static inline int irq_to_pmu(int irq)
-{
-	if (IRQ_DOVE_PMU_START <= irq && irq < NR_IRQS)
-		return irq - IRQ_DOVE_PMU_START;
-
-	return -EINVAL;
-}
 
 #endif
diff --git a/arch/arm/mach-dove/irq.c b/arch/arm/mach-dove/irq.c
index 4a5a7aedcb76..924d8afe4597 100644
--- a/arch/arm/mach-dove/irq.c
+++ b/arch/arm/mach-dove/irq.c
@@ -7,86 +7,14 @@
  * License version 2.  This program is licensed "as is" without any
  * warranty of any kind, whether express or implied.
  */
-
-#include <linux/kernel.h>
 #include <linux/init.h>
 #include <linux/irq.h>
-#include <linux/gpio.h>
 #include <linux/io.h>
-#include <asm/mach/arch.h>
 #include <plat/irq.h>
-#include <asm/mach/irq.h>
-#include <mach/pm.h>
 #include <mach/bridge-regs.h>
 #include <plat/orion-gpio.h>
 #include "common.h"
 
-static void pmu_irq_mask(struct irq_data *d)
-{
-	int pin = irq_to_pmu(d->irq);
-	u32 u;
-
-	u = readl(PMU_INTERRUPT_MASK);
-	u &= ~(1 << (pin & 31));
-	writel(u, PMU_INTERRUPT_MASK);
-}
-
-static void pmu_irq_unmask(struct irq_data *d)
-{
-	int pin = irq_to_pmu(d->irq);
-	u32 u;
-
-	u = readl(PMU_INTERRUPT_MASK);
-	u |= 1 << (pin & 31);
-	writel(u, PMU_INTERRUPT_MASK);
-}
-
-static void pmu_irq_ack(struct irq_data *d)
-{
-	int pin = irq_to_pmu(d->irq);
-	u32 u;
-
-	/*
-	 * The PMU mask register is not RW0C: it is RW.  This means that
-	 * the bits take whatever value is written to them; if you write
-	 * a '1', you will set the interrupt.
-	 *
-	 * Unfortunately this means there is NO race free way to clear
-	 * these interrupts.
-	 *
-	 * So, let's structure the code so that the window is as small as
-	 * possible.
-	 */
-	u = ~(1 << (pin & 31));
-	u &= readl_relaxed(PMU_INTERRUPT_CAUSE);
-	writel_relaxed(u, PMU_INTERRUPT_CAUSE);
-}
-
-static struct irq_chip pmu_irq_chip = {
-	.name		= "pmu_irq",
-	.irq_mask	= pmu_irq_mask,
-	.irq_unmask	= pmu_irq_unmask,
-	.irq_ack	= pmu_irq_ack,
-};
-
-static void pmu_irq_handler(unsigned int irq, struct irq_desc *desc)
-{
-	unsigned long cause = readl(PMU_INTERRUPT_CAUSE);
-
-	cause &= readl(PMU_INTERRUPT_MASK);
-	if (cause == 0) {
-		do_bad_IRQ(irq, desc);
-		return;
-	}
-
-	for (irq = 0; irq < NR_PMU_IRQS; irq++) {
-		if (!(cause & (1 << irq)))
-			continue;
-		irq = pmu_to_irq(irq);
-		generic_handle_irq(irq);
-	}
-}
-
 static int __initdata gpio0_irqs[4] = {
 	IRQ_DOVE_GPIO_0_7,
 	IRQ_DOVE_GPIO_8_15,
@@ -142,8 +70,6 @@ __exception_irq_entry dove_legacy_handle_irq(struct pt_regs *regs)
 
 void __init dove_init_irq(void)
 {
-	int i;
-
 	orion_irq_init(0, IRQ_VIRT_BASE + IRQ_MASK_LOW_OFF);
 	orion_irq_init(32, IRQ_VIRT_BASE + IRQ_MASK_HIGH_OFF);
 
@@ -162,17 +88,4 @@ void __init dove_init_irq(void)
 
 	orion_gpio_init(NULL, 64, 8, DOVE_GPIO2_VIRT_BASE, 0,
 			IRQ_DOVE_GPIO_START + 64, gpio2_irqs);
-
-	/*
-	 * Mask and clear PMU interrupts
-	 */
-	writel(0, PMU_INTERRUPT_MASK);
-	writel(0, PMU_INTERRUPT_CAUSE);
-
-	for (i = IRQ_DOVE_PMU_START; i < NR_IRQS; i++) {
-		irq_set_chip_and_handler(i, &pmu_irq_chip, handle_level_irq);
-		irq_set_status_flags(i, IRQ_LEVEL);
-		set_irq_flags(i, IRQF_VALID);
-	}
-	irq_set_chained_handler(IRQ_DOVE_PMU, pmu_irq_handler);
 }
diff --git a/arch/arm/mach-dove/pmu.c b/arch/arm/mach-dove/pmu.c
new file mode 100644
index 000000000000..2d1325995a68
--- /dev/null
+++ b/arch/arm/mach-dove/pmu.c
@@ -0,0 +1,410 @@
+/*
+ * Marvell Dove PMU support
+ */
+#include <linux/io.h>
+#include <linux/irq.h>
+#include <linux/irqdomain.h>
+#include <linux/of.h>
+#include <linux/of_irq.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+#include <linux/pm_domain.h>
+#include <linux/reset.h>
+#include <linux/reset-controller.h>
+#include <linux/sched.h>
+#include <linux/slab.h>
+#include <linux/spinlock.h>
+
+#include <asm/mach/irq.h>
+
+#include <mach/hardware.h>
+#include <mach/pm.h>
+
+#define PMC_SW_RST		0x30
+#define PMC_IRQ_CAUSE		0x50
+#define PMC_IRQ_MASK		0x54
+
+#define PMU_PWR			0x10
+#define  PMU_PWR_DOWN_GPU	BIT(2)
+#define  PMU_PWR_DOWN_VPU	BIT(3)
+#define PMU_ISO			0x58
+#define  PMU_ISO_VPU		BIT(0)
+#define  PMU_ISO_GPU		BIT(1)
+#define  PMU_ISO_CPU		BIT(2)
+#define  PMU_ISO_CORE		BIT(3)
+
+struct pmu_data {
+	spinlock_t lock;
+	struct device_node *of_node;
+	void __iomem *pmc_base;
+	void __iomem *pmu_base;
+	struct irq_chip_generic *irq_gc;
+	struct irq_domain *irq_domain;
+#ifdef CONFIG_RESET_CONTROLLER
+	struct reset_controller_dev reset;
+#endif
+};
+
+/*
+ * The PMU contains a register to reset various subsystems within the
+ * SoC.  Export this as a reset controller.
+ */
+#ifdef CONFIG_RESET_CONTROLLER
+#define rcdev_to_pmu(rcdev) container_of(rcdev, struct pmu_data, reset)
+
+static int pmu_reset_reset(struct reset_controller_dev *rc, unsigned long id)
+{
+	struct pmu_data *pmu = rcdev_to_pmu(rc);
+	unsigned long flags;
+	u32 val;
+
+	spin_lock_irqsave(&pmu->lock, flags);
+	val = readl_relaxed(pmu->pmc_base + PMC_SW_RST);
+	writel_relaxed(val & ~BIT(id), pmu->pmc_base + PMC_SW_RST);
+	writel_relaxed(val | BIT(id), pmu->pmc_base + PMC_SW_RST);
+	spin_unlock_irqrestore(&pmu->lock, flags);
+
+	return 0;
+}
+
+static int pmu_reset_assert(struct reset_controller_dev *rc, unsigned long id)
+{
+	struct pmu_data *pmu = rcdev_to_pmu(rc);
+	unsigned long flags;
+	u32 val = ~BIT(id);
+
+	spin_lock_irqsave(&pmu->lock, flags);
+	val &= readl_relaxed(pmu->pmc_base + PMC_SW_RST);
+	writel_relaxed(val, pmu->pmc_base + PMC_SW_RST);
+	spin_unlock_irqrestore(&pmu->lock, flags);
+
+	return 0;
+}
+
+static int pmu_reset_deassert(struct reset_controller_dev *rc, unsigned long id)
+{
+	struct pmu_data *pmu = rcdev_to_pmu(rc);
+	unsigned long flags;
+	u32 val = BIT(id);
+
+	spin_lock_irqsave(&pmu->lock, flags);
+	val |= readl_relaxed(pmu->pmc_base + PMC_SW_RST);
+	writel_relaxed(val, pmu->pmc_base + PMC_SW_RST);
+	spin_unlock_irqrestore(&pmu->lock, flags);
+
+	return 0;
+}
+
+static struct reset_control_ops pmu_reset_ops = {
+	.reset = pmu_reset_reset,
+	.assert = pmu_reset_assert,
+	.deassert = pmu_reset_deassert,
+};
+
+static struct reset_controller_dev pmu_reset __initdata = {
+	.ops = &pmu_reset_ops,
+	.owner = THIS_MODULE,
+	.nr_resets = 32,
+};
+
+static void __init pmu_reset_init(struct pmu_data *pmu)
+{
+	int ret;
+
+	pmu->reset = pmu_reset;
+	pmu->reset.of_node = pmu->of_node;
+
+	ret = reset_controller_register(&pmu->reset);
+	if (ret)
+		pr_err("pmu: %s failed: %d\n", "reset_controller_register", ret);
+}
+#else
+static void __init pmu_reset_init(struct pmu_data *pmu)
+{
+}
+#endif
+
+struct pmu_domain {
+	struct pmu_data *pmu;
+	u32 pwr_mask;
+	u32 rst_mask;
+	u32 iso_mask;
+	struct generic_pm_domain base;
+};
+
+#define to_pmu_domain(dom) container_of(dom, struct pmu_domain, base)
+
+/*
+ * This deals with the "old" Marvell sequence of bringing a power domain
+ * down/up, which is: apply power, release reset, disable isolators.
+ *
+ * Later devices apparantly use a different sequence: power up, disable
+ * isolators, assert repair signal, enable SRMA clock, enable AXI clock,
+ * enable module clock, deassert reset.
+ *
+ * Note: reading the assembly, it seems that the IO accessors have an
+ * unfortunate side-effect - they cause memory already read into registers
+ * for the if () to be re-read for the bit-set or bit-clear operation.
+ * The code is written to avoid this.
+ */
+static int pmu_domain_power_off(struct generic_pm_domain *domain)
+{
+	struct pmu_domain *pmu_dom = to_pmu_domain(domain);
+	struct pmu_data *pmu = pmu_dom->pmu;
+	unsigned long flags;
+	unsigned int val;
+	void __iomem *pmu_base = pmu->pmu_base;
+	void __iomem *pmc_base = pmu->pmc_base;
+
+	spin_lock_irqsave(&pmu->lock, flags);
+
+	/* Enable isolators */
+	if (pmu_dom->iso_mask) {
+		val = ~pmu_dom->iso_mask;
+		val &= readl_relaxed(pmu_base + PMU_ISO);
+		writel_relaxed(val, pmu_base + PMU_ISO);
+	}
+
+	/* Reset unit */
+	if (pmu_dom->rst_mask) {
+		val = ~pmu_dom->rst_mask;
+		val &= readl_relaxed(pmc_base + PMC_SW_RST);
+		writel_relaxed(val, pmc_base + PMC_SW_RST);
+	}
+
+	/* Power down */
+	val = readl_relaxed(pmu_base + PMU_PWR) | pmu_dom->pwr_mask;
+	writel_relaxed(val, pmu_base + PMU_PWR);
+
+	spin_unlock_irqrestore(&pmu->lock, flags);
+
+	return 0;
+}
+
+static int pmu_domain_power_on(struct generic_pm_domain *domain)
+{
+	struct pmu_domain *pmu_dom = to_pmu_domain(domain);
+	struct pmu_data *pmu = pmu_dom->pmu;
+	unsigned long flags;
+	unsigned int val;
+	void __iomem *pmu_base = pmu->pmu_base;
+	void __iomem *pmc_base = pmu->pmc_base;
+
+	spin_lock_irqsave(&pmu->lock, flags);
+
+	/* Power on */
+	val = ~pmu_dom->pwr_mask & readl_relaxed(pmu_base + PMU_PWR);
+	writel_relaxed(val, pmu_base + PMU_PWR);
+
+	/* Release reset */
+	if (pmu_dom->rst_mask) {
+		val = pmu_dom->rst_mask;
+		val |= readl_relaxed(pmc_base + PMC_SW_RST);
+		writel_relaxed(val, pmc_base + PMC_SW_RST);
+	}
+
+	/* Disable isolators */
+	if (pmu_dom->iso_mask) {
+		val = pmu_dom->iso_mask;
+		val |= readl_relaxed(pmu_base + PMU_ISO);
+		writel_relaxed(val, pmu_base + PMU_ISO);
+	}
+
+	spin_unlock_irqrestore(&pmu->lock, flags);
+
+	return 0;
+}
+
+static void __pmu_domain_register(struct pmu_domain *domain,
+	struct device_node *np)
+{
+	unsigned int val = readl_relaxed(domain->pmu->pmu_base + PMU_PWR);
+
+	domain->base.power_off = pmu_domain_power_off;
+	domain->base.power_on = pmu_domain_power_on;
+
+	pm_genpd_init(&domain->base, NULL, !(val & domain->pwr_mask));
+
+	if (np)
+		of_genpd_add_provider_simple(np, &domain->base);
+}
+
+/* PMU IRQ controller */
+static void pmu_irq_handler(unsigned int irq, struct irq_desc *desc)
+{
+	struct pmu_data *pmu = irq_get_handler_data(irq);
+	struct irq_chip_generic *gc = pmu->irq_gc;
+	struct irq_domain *domain = pmu->irq_domain;
+	void __iomem *base = gc->reg_base;
+	u32 stat = readl_relaxed(base + PMC_IRQ_CAUSE) & gc->mask_cache;
+	u32 done = ~0;
+
+	if (stat == 0) {
+		do_bad_IRQ(irq, desc);
+		return;
+	}
+
+	while (stat) {
+		u32 hwirq = fls(stat) - 1;
+
+		stat &= ~(1 << hwirq);
+		done &= ~(1 << hwirq);
+
+		generic_handle_irq(irq_find_mapping(domain, hwirq));
+	}
+
+	/*
+	 * The PMU mask register is not RW0C: it is RW.  This means that
+	 * the bits take whatever value is written to them; if you write
+	 * a '1', you will set the interrupt.
+	 *
+	 * Unfortunately this means there is NO race free way to clear
+	 * these interrupts.
+	 *
+	 * So, let's structure the code so that the window is as small as
+	 * possible.
+	 */
+	irq_gc_lock(gc);
+	done &= readl_relaxed(base + PMC_IRQ_CAUSE);
+	writel_relaxed(done, base + PMC_IRQ_CAUSE);
+	irq_gc_unlock(gc);
+}
+
+static int __init dove_init_pmu_irq(struct pmu_data *pmu, int irq)
+{
+	const char *name = "pmu_irq";
+	struct irq_chip_generic *gc;
+	struct irq_domain *domain;
+	int ret;
+
+	/* mask and clear all interrupts */
+	writel(0, pmu->pmc_base + PMC_IRQ_MASK);
+	writel(0, pmu->pmc_base + PMC_IRQ_CAUSE);
+
+	domain = irq_domain_add_linear(pmu->of_node, NR_PMU_IRQS,
+				       &irq_generic_chip_ops, NULL);
+	if (!domain) {
+		pr_err("%s: unable to add irq domain\n", name);
+		return -ENOMEM;
+	}
+
+	ret = irq_alloc_domain_generic_chips(domain, NR_PMU_IRQS, 1, name,
+					     handle_level_irq,
+					     IRQ_NOREQUEST | IRQ_NOPROBE, 0,
+					     IRQ_GC_INIT_MASK_CACHE);
+	if (ret) {
+		pr_err("%s: unable to alloc irq domain gc: %d\n", name, ret);
+		irq_domain_remove(domain);
+		return ret;
+	}
+
+	gc = irq_get_domain_generic_chip(domain, 0);
+	gc->reg_base = pmu->pmc_base;
+	gc->chip_types[0].regs.mask = PMC_IRQ_MASK;
+	gc->chip_types[0].chip.irq_mask = irq_gc_mask_clr_bit;
+	gc->chip_types[0].chip.irq_unmask = irq_gc_mask_set_bit;
+
+	pmu->irq_domain = domain;
+	pmu->irq_gc = gc;
+
+	/* If no of_node, populate the domain */
+	if (!pmu->of_node)
+		irq_domain_associate_many(pmu->irq_domain, IRQ_DOVE_PMU_START,
+					  0, NR_PMU_IRQS);
+
+	irq_set_handler_data(irq, pmu);
+	irq_set_chained_handler(irq, pmu_irq_handler);
+
+	return 0;
+}
+
+/*
+ * pmu {
+ *	compatible = "marvell,pmu";
+ *	reg = <0xd0000 0x8000> <0xd8000 0x8000>;
+ *	interrupts = <33>;
+ *	#reset-cells = 1;
+ *	#power-domain-cells = <0>;
+ *	vpu_domain: vpu-domain {
+ *		marvell,pmu_pwr_mask = <0x00000008>;
+ *		marvell,pmu_iso_mask = <0x00000001>;
+ *		resets = <&pmu 16>;
+ *	};
+ *	gpu_domain: gpu-domain {
+ *		marvell,pmu_pwr_mask = <0x00000004>;
+ *		marvell,pmu_iso_mask = <0x00000002>;
+ *		resets = <&pmu 18>;
+ *	};
+ * };
+ */
+int __init dove_init_pmu(void)
+{
+	struct device_node *np_pmu, *np;
+	struct pmu_data *pmu;
+	int ret, parent_irq;
+
+	/* Lookup the PMU node */
+	np_pmu = of_find_compatible_node(NULL, NULL, "marvell,pmu");
+	if (!np_pmu)
+		return 0;
+
+	pmu = kzalloc(sizeof(*pmu), GFP_KERNEL);
+	if (!pmu)
+		return -ENOMEM;
+
+	spin_lock_init(&pmu->lock);
+	pmu->of_node = np_pmu;
+	pmu->pmc_base = of_iomap(pmu->of_node, 0);
+	pmu->pmu_base = of_iomap(pmu->of_node, 1);
+	if (!pmu->pmc_base || !pmu->pmu_base) {
+		pr_err("%s: failed to map PMU\n", np_pmu->name);
+		iounmap(pmu->pmu_base);
+		iounmap(pmu->pmc_base);
+		kfree(pmu);
+		return -ENOMEM;
+	}
+
+	parent_irq = irq_of_parse_and_map(pmu->of_node, 0);
+	if (!parent_irq)
+		pr_err("%s: no interrupt specified\n", np_pmu->name);
+
+	pmu_reset_init(pmu);
+
+	for_each_available_child_of_node(pmu->of_node, np) {
+		struct of_phandle_args args;
+		struct pmu_domain *domain;
+
+		domain = kzalloc(sizeof(*domain), GFP_KERNEL);
+		if (!domain)
+			break;
+
+		domain->pmu = pmu;
+		domain->base.name = kstrdup(np->name, GFP_KERNEL);
+		if (!domain->base.name) {
+			kfree(domain);
+			break;
+		}
+
+		of_property_read_u32(np, "marvell,pmu_pwr_mask",
+				     &domain->pwr_mask);
+		of_property_read_u32(np, "marvell,pmu_iso_mask",
+				     &domain->iso_mask);
+
+		ret = of_parse_phandle_with_args(np, "resets", "#reset-cells",
+						 0, &args);
+		if (ret == 0) {
+			if (args.np == pmu->of_node)
+				domain->rst_mask = BIT(args.args[0]);
+			of_node_put(args.np);
+		}
+
+		__pmu_domain_register(domain, np);
+	}
+	pm_genpd_poweroff_unused();
+
+	ret = dove_init_pmu_irq(pmu, parent_irq);
+	if (ret)
+		pr_err("dove_init_pmu_irq() failed: %d\n", ret);
+
+	return 0;
+}
-- 
1.8.3.1

[PATCH 5/8] ARM: dt: dove: add Dove PMU DT entry to dove.dtsi

[Russell King]

Add the PMU description to the Dove DT file.  The PMU provides multiple
features, including an interrupt, reset, power and isolation controller.

Signed-off-by: Russell King <rmk+kernel-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org>
---
 arch/arm/boot/dts/dove.dtsi | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/arm/boot/dts/dove.dtsi b/arch/arm/boot/dts/dove.dtsi
index a5441d5482a6..1e664eb8bdcb 100644
--- a/arch/arm/boot/dts/dove.dtsi
+++ b/arch/arm/boot/dts/dove.dtsi
@@ -380,6 +380,15 @@
 				status = "disabled";
 			};
 
+			pmu: power-management@d0000 {
+				compatible = "marvell,pmu";
+				reg = <0xd0000 0x8000>, <0xd8000 0x8000>;
+				interrupts = <33>;
+				interrupt-controller;
+				#interrupt-cells = <1>;
+				#reset-cells = <1>;
+			};
+
 			thermal: thermal-diode@d001c {
 				compatible = "marvell,dove-thermal";
 				reg = <0xd001c 0x0c>, <0xd005c 0x08>;
-- 
1.8.3.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 5/8] ARM: dt: dove: add Dove PMU DT entry to dove.dtsi

[Russell King]

Add the PMU description to the Dove DT file.  The PMU provides multiple
features, including an interrupt, reset, power and isolation controller.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 arch/arm/boot/dts/dove.dtsi | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/arm/boot/dts/dove.dtsi b/arch/arm/boot/dts/dove.dtsi
index a5441d5482a6..1e664eb8bdcb 100644
--- a/arch/arm/boot/dts/dove.dtsi
+++ b/arch/arm/boot/dts/dove.dtsi
@@ -380,6 +380,15 @@
 				status = "disabled";
 			};
 
+			pmu: power-management at d0000 {
+				compatible = "marvell,pmu";
+				reg = <0xd0000 0x8000>, <0xd8000 0x8000>;
+				interrupts = <33>;
+				interrupt-controller;
+				#interrupt-cells = <1>;
+				#reset-cells = <1>;
+			};
+
 			thermal: thermal-diode at d001c {
 				compatible = "marvell,dove-thermal";
 				reg = <0xd001c 0x0c>, <0xd005c 0x08>;
-- 
1.8.3.1

[PATCH 6/8] ARM: dt: dove: wire up RTC interrupt

[Russell King]

Now that we have a PMU driver, we can wire up the RTC interrupt in the
DT description for Dove.

Signed-off-by: Russell King <rmk+kernel-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org>
---
 arch/arm/boot/dts/dove.dtsi | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/boot/dts/dove.dtsi b/arch/arm/boot/dts/dove.dtsi
index 1e664eb8bdcb..00b61d2be66b 100644
--- a/arch/arm/boot/dts/dove.dtsi
+++ b/arch/arm/boot/dts/dove.dtsi
@@ -624,6 +624,8 @@
 			rtc: real-time-clock@d8500 {
 				compatible = "marvell,orion-rtc";
 				reg = <0xd8500 0x20>;
+				interrupt-parent = <&pmu>;
+				interrupts = <5>;
 			};
 
 			gconf: global-config@e802c {
-- 
1.8.3.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 6/8] ARM: dt: dove: wire up RTC interrupt

[Russell King]

Now that we have a PMU driver, we can wire up the RTC interrupt in the
DT description for Dove.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 arch/arm/boot/dts/dove.dtsi | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/boot/dts/dove.dtsi b/arch/arm/boot/dts/dove.dtsi
index 1e664eb8bdcb..00b61d2be66b 100644
--- a/arch/arm/boot/dts/dove.dtsi
+++ b/arch/arm/boot/dts/dove.dtsi
@@ -624,6 +624,8 @@
 			rtc: real-time-clock at d8500 {
 				compatible = "marvell,orion-rtc";
 				reg = <0xd8500 0x20>;
+				interrupt-parent = <&pmu>;
+				interrupts = <5>;
 			};
 
 			gconf: global-config at e802c {
-- 
1.8.3.1

[PATCH 7/8] ARM: dt: dove: add video decoder power domain description

[Russell King]

Add the description of the video decoder power domain to the PMU DT
entry.

Signed-off-by: Russell King <rmk+kernel-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org>
---
 arch/arm/boot/dts/dove.dtsi | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/arm/boot/dts/dove.dtsi b/arch/arm/boot/dts/dove.dtsi
index 00b61d2be66b..a47c7cef380f 100644
--- a/arch/arm/boot/dts/dove.dtsi
+++ b/arch/arm/boot/dts/dove.dtsi
@@ -387,6 +387,13 @@
 				interrupt-controller;
 				#interrupt-cells = <1>;
 				#reset-cells = <1>;
+
+				vpu_domain: vpu-domain {
+					#power-domain-cells = <0>;
+					marvell,pmu_pwr_mask = <0x00000008>;
+					marvell,pmu_iso_mask = <0x00000001>;
+					resets = <&pmu 16>;
+				};
 			};
 
 			thermal: thermal-diode@d001c {
-- 
1.8.3.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 7/8] ARM: dt: dove: add video decoder power domain description

[Russell King]

Add the description of the video decoder power domain to the PMU DT
entry.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 arch/arm/boot/dts/dove.dtsi | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/arm/boot/dts/dove.dtsi b/arch/arm/boot/dts/dove.dtsi
index 00b61d2be66b..a47c7cef380f 100644
--- a/arch/arm/boot/dts/dove.dtsi
+++ b/arch/arm/boot/dts/dove.dtsi
@@ -387,6 +387,13 @@
 				interrupt-controller;
 				#interrupt-cells = <1>;
 				#reset-cells = <1>;
+
+				vpu_domain: vpu-domain {
+					#power-domain-cells = <0>;
+					marvell,pmu_pwr_mask = <0x00000008>;
+					marvell,pmu_iso_mask = <0x00000001>;
+					resets = <&pmu 16>;
+				};
 			};
 
 			thermal: thermal-diode at d001c {
-- 
1.8.3.1

[PATCH 8/8] ARM: dt: dove: add GPU power domain description

[Russell King]

Add the description of the GPU power domain to the PMU DT entry.

Signed-off-by: Russell King <rmk+kernel-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org>
---
 arch/arm/boot/dts/dove.dtsi | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/arm/boot/dts/dove.dtsi b/arch/arm/boot/dts/dove.dtsi
index a47c7cef380f..081f968c521a 100644
--- a/arch/arm/boot/dts/dove.dtsi
+++ b/arch/arm/boot/dts/dove.dtsi
@@ -394,6 +394,13 @@
 					marvell,pmu_iso_mask = <0x00000001>;
 					resets = <&pmu 16>;
 				};
+
+				gpu_domain: gpu-domain {
+					#power-domain-cells = <0>;
+					marvell,pmu_pwr_mask = <0x00000004>;
+					marvell,pmu_iso_mask = <0x00000002>;
+					resets = <&pmu 18>;
+				};
 			};
 
 			thermal: thermal-diode@d001c {
-- 
1.8.3.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 8/8] ARM: dt: dove: add GPU power domain description

[Russell King]

Add the description of the GPU power domain to the PMU DT entry.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 arch/arm/boot/dts/dove.dtsi | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/arm/boot/dts/dove.dtsi b/arch/arm/boot/dts/dove.dtsi
index a47c7cef380f..081f968c521a 100644
--- a/arch/arm/boot/dts/dove.dtsi
+++ b/arch/arm/boot/dts/dove.dtsi
@@ -394,6 +394,13 @@
 					marvell,pmu_iso_mask = <0x00000001>;
 					resets = <&pmu 16>;
 				};
+
+				gpu_domain: gpu-domain {
+					#power-domain-cells = <0>;
+					marvell,pmu_pwr_mask = <0x00000004>;
+					marvell,pmu_iso_mask = <0x00000002>;
+					resets = <&pmu 18>;
+				};
 			};
 
 			thermal: thermal-diode at d001c {
-- 
1.8.3.1

Re: [FOR DISCUSSION 0/8] Dove PMU support

[Andrew Lunn]

Hi Russell

Thanks for posting these. Lets try to get them merged.

> There are a few things which are missing from this driver, such sa the
> DT binding documentation.  This will follow once people are happy with
> the first four patches, in particular where to locate the PMU driver
> in the kernel tree.  Currently, I have it in arch/arm/mach-dove, which
> is where the legacy Dove code lives, so it's not ideal.

Maybe time for a drivers/pmu? arch/arm/mach-dove is not good, we want
to be able to use this code from arch/arm/mach-mvebu.

   Andrew

[FOR DISCUSSION 0/8] Dove PMU support

[Andrew Lunn]

Hi Russell

Thanks for posting these. Lets try to get them merged.

> There are a few things which are missing from this driver, such sa the
> DT binding documentation.  This will follow once people are happy with
> the first four patches, in particular where to locate the PMU driver
> in the kernel tree.  Currently, I have it in arch/arm/mach-dove, which
> is where the legacy Dove code lives, so it's not ideal.

Maybe time for a drivers/pmu? arch/arm/mach-dove is not good, we want
to be able to use this code from arch/arm/mach-mvebu.

   Andrew

[PATCH 4/8] ARM: dove: create a proper PMU driver for power domains, PMU IRQs and resets

[Sebastian Hesselbarth]

On 14.02.2015 16:27, Russell King wrote:
> The PMU device contains an interrupt controller, power control and
> resets.  The interrupt controller is a little sub-standard in that
> there is no race free way to clear down pending interrupts, so we try
> to avoid problems by reducing the window as much as possible, and
> clearing as infrequently as possible.
>
> The interrupt support is implemented using an IRQ domain, and the
> parent interrupt referenced in the standard DT way.
>
> The power domains and reset support is closely related - there is a
> defined sequence for powering down a domain which is tightly coupled
> with asserting the reset.  Hence, it makes sense to group these two
> together.
>
> This patch adds the core PMU driver: power domains must be defined in
> the DT file in order to make use of them.  The reset controller can
> be referenced in the standard way for reset controllers.
>
> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> ---
[...]
> diff --git a/arch/arm/mach-dove/pmu.c b/arch/arm/mach-dove/pmu.c
> new file mode 100644
> index 000000000000..2d1325995a68
> --- /dev/null
> +++ b/arch/arm/mach-dove/pmu.c
> @@ -0,0 +1,410 @@
> +/*
> + * Marvell Dove PMU support
> + */
[...]
> +/*
> + * pmu {
> + *	compatible = "marvell,pmu";

Russell,

Should we be more precise and call it "marvell,dove-pmu" ?

Also, can we have an additional "syscon" compatible for that node?

That will allow us to get rid of the messy iomem stuff current
pinctrl-dove is doing to get access to the pmu pinctrl registers.

> + *	reg = <0xd0000 0x8000> <0xd8000 0x8000>;
> + *	interrupts = <33>;
> + *	#reset-cells = 1;
> + *	#power-domain-cells = <0>;
> + *	vpu_domain: vpu-domain {
> + *		marvell,pmu_pwr_mask = <0x00000008>;
> + *		marvell,pmu_iso_mask = <0x00000001>;
> + *		resets = <&pmu 16>;
> + *	};
> + *	gpu_domain: gpu-domain {
> + *		marvell,pmu_pwr_mask = <0x00000004>;
> + *		marvell,pmu_iso_mask = <0x00000002>;
> + *		resets = <&pmu 18>;
> + *	};
> + * };
> + */
> +int __init dove_init_pmu(void)
> +{

How about we copy the clk subsystem way of installing early probed
pm for DT here?

For example:

#define PM_OF_DECLARE(name, compat, fn) OF_DECLARE_1(clk, name, compat, fn)

and

static int __init dove_pmu_init(struct device_node *np) { ... }
PM_OF_DECLARE(dove_pmu, "marvell,dove-pmu", dove_init_pmu);

Sebastian

> +	struct device_node *np_pmu, *np;
> +	struct pmu_data *pmu;
> +	int ret, parent_irq;
> +
> +	/* Lookup the PMU node */
> +	np_pmu = of_find_compatible_node(NULL, NULL, "marvell,pmu");
> +	if (!np_pmu)
> +		return 0;
> +
> +	pmu = kzalloc(sizeof(*pmu), GFP_KERNEL);
> +	if (!pmu)
> +		return -ENOMEM;
> +
> +	spin_lock_init(&pmu->lock);
> +	pmu->of_node = np_pmu;
> +	pmu->pmc_base = of_iomap(pmu->of_node, 0);
> +	pmu->pmu_base = of_iomap(pmu->of_node, 1);
> +	if (!pmu->pmc_base || !pmu->pmu_base) {
> +		pr_err("%s: failed to map PMU\n", np_pmu->name);
> +		iounmap(pmu->pmu_base);
> +		iounmap(pmu->pmc_base);
> +		kfree(pmu);
> +		return -ENOMEM;
> +	}
> +
> +	parent_irq = irq_of_parse_and_map(pmu->of_node, 0);
> +	if (!parent_irq)
> +		pr_err("%s: no interrupt specified\n", np_pmu->name);
> +
> +	pmu_reset_init(pmu);
> +
> +	for_each_available_child_of_node(pmu->of_node, np) {
> +		struct of_phandle_args args;
> +		struct pmu_domain *domain;
> +
> +		domain = kzalloc(sizeof(*domain), GFP_KERNEL);
> +		if (!domain)
> +			break;
> +
> +		domain->pmu = pmu;
> +		domain->base.name = kstrdup(np->name, GFP_KERNEL);
> +		if (!domain->base.name) {
> +			kfree(domain);
> +			break;
> +		}
> +
> +		of_property_read_u32(np, "marvell,pmu_pwr_mask",
> +				     &domain->pwr_mask);
> +		of_property_read_u32(np, "marvell,pmu_iso_mask",
> +				     &domain->iso_mask);
> +
> +		ret = of_parse_phandle_with_args(np, "resets", "#reset-cells",
> +						 0, &args);
> +		if (ret == 0) {
> +			if (args.np == pmu->of_node)
> +				domain->rst_mask = BIT(args.args[0]);
> +			of_node_put(args.np);
> +		}
> +
> +		__pmu_domain_register(domain, np);
> +	}
> +	pm_genpd_poweroff_unused();
> +
> +	ret = dove_init_pmu_irq(pmu, parent_irq);
> +	if (ret)
> +		pr_err("dove_init_pmu_irq() failed: %d\n", ret);
> +
> +	return 0;
> +}
>

[PATCH 4/8] ARM: dove: create a proper PMU driver for power domains, PMU IRQs and resets

[Andrew Lunn]

Hi Russell

> +static struct reset_control_ops pmu_reset_ops = {
> +	.reset = pmu_reset_reset,
> +	.assert = pmu_reset_assert,
> +	.deassert = pmu_reset_deassert,
> +};
> +
> +static struct reset_controller_dev pmu_reset __initdata = {
> +	.ops = &pmu_reset_ops,
> +	.owner = THIS_MODULE,

Dumb question: Is this still needed? There have been a lot of patches
from Wolfram Sang removing similar THIS_MODULE statements.

> + * pmu {
> + *	compatible = "marvell,pmu";

Is this maybe too generic? I'm sure Marvell has more than one pmu.
Maybe "marvell,dove-pmu"

> +int __init dove_init_pmu(void)
> +{
> +	struct device_node *np_pmu, *np;
> +	struct pmu_data *pmu;
> +	int ret, parent_irq;
> +
> +	/* Lookup the PMU node */
> +	np_pmu = of_find_compatible_node(NULL, NULL, "marvell,pmu");
> +	if (!np_pmu)
> +		return 0;
> +
> +	pmu = kzalloc(sizeof(*pmu), GFP_KERNEL);
> +	if (!pmu)
> +		return -ENOMEM;
> +
> +	spin_lock_init(&pmu->lock);
> +	pmu->of_node = np_pmu;
> +	pmu->pmc_base = of_iomap(pmu->of_node, 0);
> +	pmu->pmu_base = of_iomap(pmu->of_node, 1);
> +	if (!pmu->pmc_base || !pmu->pmu_base) {
> +		pr_err("%s: failed to map PMU\n", np_pmu->name);
> +		iounmap(pmu->pmu_base);
> +		iounmap(pmu->pmc_base);
> +		kfree(pmu);
> +		return -ENOMEM;
> +	}
> +
> +	parent_irq = irq_of_parse_and_map(pmu->of_node, 0);
> +	if (!parent_irq)
> +		pr_err("%s: no interrupt specified\n", np_pmu->name);
> +

Is it not fatal to be missing the interrupt? Seems like return -EINVAL
would be a good idea?

      Andrew

Re: [PATCH 3/8] pm: domains: sync runtime PM status with PM domains

[Ulf Hansson]

Hi Russell,

On 14 February 2015 at 16:27, Russell King <rmk+kernel@arm.linux.org.uk> wrote:
> Synchronise the PM domain status with runtime PM's status.  This
> provides a better solution to the problem than commit 2ed127697eb1
> ("PM / Domains: Power on the PM domain right after attach completes").
>
> The above commit added a call to power up the PM domain when a device
> attaches to the domain.  The assumption is that the device driver
> will cause a runtime PM transition, which will synchronise the PM
> domain state with the runtime PM state.
>
> However, runtime PM will, by default, assume that the device is
> initially suspended.  So we have two subsystems which have differing
> initial state expectations.  This is silly.
>
> Runtime PM requires that pm_runtime_set_active() is called before
> pm_runtime_enable() if the device is already powered up.  However, PM
> domains are not informed of this, and this is where the problem really
> lies.  We need to keep PM domains properly and fully informed of their
> associated device status.
>
> We fix this by adding a new callback - runtime_set_status() which is
> triggered whenever a successful call to __pm_runtime_set_status().
> PM domain code hooks into this, and updates the PM domain appropriately.
>
> This means a driver with the following sequence:
>
>         pm_runtime_set_active(dev);
>         pm_runtime_enable(dev);
>
> will trigger the PM domain to be powered up at this point, which keeps
> runtime PM and PM domains properly in sync with each other.

The issue you are trying to solve here was raised by me during the
discussion around the below merged commit.
"PM / Domains: Power on the PM domain right after attach completes"

You may find the complete thread here:
http://marc.info/?l=linux-pm&m=141623756506128&w=2

Especially I mention a "scenario 5", which is the issue you observed
and what we decided to keep as a limitation. Sorry about that!

Moreover, the below commit is also related to the similar issues and
this fixed a regression.
67732cd34382 ( PM / Domains: Fix initial default state of the
need_restore flag).

I just wanted to make sure you have all the background to why we ended
up with current solution...

Regarding $subject patch, I think it's an interesting approach but I
need some more time to think about it. Unfortunate I am OOF the next
two weeks so will have to promise you to review this as soon as I get
back.

BTW, I would also appreciate if you could keep me on cc for any
further changes related to genpd.

Kind regards
Uffe

>
> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> ---
>  drivers/base/power/domain.c  | 16 +++++++++++++++-
>  drivers/base/power/runtime.c |  5 +++++
>  include/linux/pm.h           |  1 +
>  3 files changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 11a1023fa64a..2a700cea41fc 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -741,6 +741,20 @@ static int pm_genpd_runtime_resume(struct device *dev)
>         return 0;
>  }
>
> +static int pm_genpd_runtime_set_status(struct device *dev)
> +{
> +       int ret;
> +
> +       dev_dbg(dev, "%s()\n", __func__);
> +
> +       if (pm_runtime_suspended(dev))
> +               ret = pm_genpd_runtime_suspend(dev);
> +       else
> +               ret = pm_genpd_runtime_resume(dev);
> +
> +       return ret;
> +}
> +
>  static bool pd_ignore_unused;
>  static int __init pd_ignore_unused_setup(char *__unused)
>  {
> @@ -1907,6 +1921,7 @@ void pm_genpd_init(struct generic_pm_domain *genpd,
>         genpd->max_off_time_changed = true;
>         genpd->domain.ops.runtime_suspend = pm_genpd_runtime_suspend;
>         genpd->domain.ops.runtime_resume = pm_genpd_runtime_resume;
> +       genpd->domain.ops.runtime_set_status = pm_genpd_runtime_set_status;
>         genpd->domain.ops.prepare = pm_genpd_prepare;
>         genpd->domain.ops.suspend = pm_genpd_suspend;
>         genpd->domain.ops.suspend_late = pm_genpd_suspend_late;
> @@ -2223,7 +2238,6 @@ int genpd_dev_pm_attach(struct device *dev)
>         }
>
>         dev->pm_domain->detach = genpd_dev_pm_detach;
> -       pm_genpd_poweron(pd);
>
>         return 0;
>  }
> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> index 5070c4fe8542..a958cae67a37 100644
> --- a/drivers/base/power/runtime.c
> +++ b/drivers/base/power/runtime.c
> @@ -981,6 +981,7 @@ int __pm_runtime_set_status(struct device *dev, unsigned int status)
>         struct device *parent = dev->parent;
>         unsigned long flags;
>         bool notify_parent = false;
> +       pm_callback_t callback;
>         int error = 0;
>
>         if (status != RPM_ACTIVE && status != RPM_SUSPENDED)
> @@ -1029,6 +1030,10 @@ int __pm_runtime_set_status(struct device *dev, unsigned int status)
>   out_set:
>         __update_runtime_status(dev, status);
>         dev->power.runtime_error = 0;
> +       if (dev->power.no_callbacks)
> +               goto out;
> +       callback = RPM_GET_CALLBACK(dev, runtime_set_status);
> +       rpm_callback(callback, dev);
>   out:
>         spin_unlock_irqrestore(&dev->power.lock, flags);
>
> diff --git a/include/linux/pm.h b/include/linux/pm.h
> index 8b5976364619..ee098585d577 100644
> --- a/include/linux/pm.h
> +++ b/include/linux/pm.h
> @@ -316,6 +316,7 @@ struct dev_pm_ops {
>         int (*runtime_suspend)(struct device *dev);
>         int (*runtime_resume)(struct device *dev);
>         int (*runtime_idle)(struct device *dev);
> +       int (*runtime_set_status)(struct device *dev);
>  };
>
>  #ifdef CONFIG_PM_SLEEP
> --
> 1.8.3.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4/8] ARM: dove: create a proper PMU driver for power domains, PMU IRQs and resets

[Russell King - ARM Linux]

On Sat, Feb 14, 2015 at 06:09:39PM +0100, Andrew Lunn wrote:
> Hi Russell
> 
> > +static struct reset_control_ops pmu_reset_ops = {
> > +	.reset = pmu_reset_reset,
> > +	.assert = pmu_reset_assert,
> > +	.deassert = pmu_reset_deassert,
> > +};
> > +
> > +static struct reset_controller_dev pmu_reset __initdata = {
> > +	.ops = &pmu_reset_ops,
> > +	.owner = THIS_MODULE,
> 
> Dumb question: Is this still needed? There have been a lot of patches
> from Wolfram Sang removing similar THIS_MODULE statements.

If this is not set, then .owner remains NULL; reset_controller_register()
is not wrapped to hide the initialisation of this member.  (IMHO, that's
exactly why hiding it is bad - it has created inconsistencies because
some APIs need an explicit initialisation, others don't.)

> > + * pmu {
> > + *	compatible = "marvell,pmu";
> 
> Is this maybe too generic? I'm sure Marvell has more than one pmu.
> Maybe "marvell,dove-pmu"

Changed.

> > +int __init dove_init_pmu(void)
> > +{
> > +	struct device_node *np_pmu, *np;
> > +	struct pmu_data *pmu;
> > +	int ret, parent_irq;
> > +
> > +	/* Lookup the PMU node */
> > +	np_pmu = of_find_compatible_node(NULL, NULL, "marvell,pmu");
> > +	if (!np_pmu)
> > +		return 0;
> > +
> > +	pmu = kzalloc(sizeof(*pmu), GFP_KERNEL);
> > +	if (!pmu)
> > +		return -ENOMEM;
> > +
> > +	spin_lock_init(&pmu->lock);
> > +	pmu->of_node = np_pmu;
> > +	pmu->pmc_base = of_iomap(pmu->of_node, 0);
> > +	pmu->pmu_base = of_iomap(pmu->of_node, 1);
> > +	if (!pmu->pmc_base || !pmu->pmu_base) {
> > +		pr_err("%s: failed to map PMU\n", np_pmu->name);
> > +		iounmap(pmu->pmu_base);
> > +		iounmap(pmu->pmc_base);
> > +		kfree(pmu);
> > +		return -ENOMEM;
> > +	}
> > +
> > +	parent_irq = irq_of_parse_and_map(pmu->of_node, 0);
> > +	if (!parent_irq)
> > +		pr_err("%s: no interrupt specified\n", np_pmu->name);
> > +
> 
> Is it not fatal to be missing the interrupt? Seems like return -EINVAL
> would be a good idea?

I don't think so.  The lack of parent interrupt shouldn't stop the
rest of the PMU code initialising - least of all only because the
only user of this right now is the RTC.

It may make sense to avoid initialising the PMU's IRQ support in
that case though.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

[PATCH 4/8] ARM: dove: create a proper PMU driver for power domains, PMU IRQs and resets

[Russell King - ARM Linux]

On Sat, Feb 14, 2015 at 06:02:11PM +0100, Sebastian Hesselbarth wrote:
> Should we be more precise and call it "marvell,dove-pmu" ?

Andrew made the same comment, consider it done.

> Also, can we have an additional "syscon" compatible for that node?

I'll look into it, thanks.

> How about we copy the clk subsystem way of installing early probed
> pm for DT here?
> 
> For example:
> 
> #define PM_OF_DECLARE(name, compat, fn) OF_DECLARE_1(clk, name, compat, fn)
> 
> and
> 
> static int __init dove_pmu_init(struct device_node *np) { ... }
> PM_OF_DECLARE(dove_pmu, "marvell,dove-pmu", dove_init_pmu);

Not sure it's worth the overhead.  Firstly, sticking the PMU into the
clock OF tables doesn't seem like the right thing to do.  Secondly,
it means modifying the asm-generic vmlinux.lds.S, adding another
config symbol for PMUs to enable that table, adding a sentinel entry,
and finally adding some code somewhere to scan this new table which
right now is only going to have at most one entry.

That seems like an awful lot of overhead.

It would have been easier if there was a generic mechanism, like the
initcall mechanism, where it's possible to add these kinds of
compatible matches automatically without having to mess around with
so many different files.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

[PATCH 4/8] ARM: dove: create a proper PMU driver for power domains, PMU IRQs and resets

[Russell King - ARM Linux]

On Sat, Feb 14, 2015 at 06:02:11PM +0100, Sebastian Hesselbarth wrote:
> How about we copy the clk subsystem way of installing early probed
> pm for DT here?
> 
> For example:
> 
> #define PM_OF_DECLARE(name, compat, fn) OF_DECLARE_1(clk, name, compat, fn)
> 
> and
> 
> static int __init dove_pmu_init(struct device_node *np) { ... }
> PM_OF_DECLARE(dove_pmu, "marvell,dove-pmu", dove_init_pmu);

Well, Rob's response was basically "use the machine descriptor" so I
guess it needs to be explicitly called from
arch/arm/mach-mvebu/dove.c:dove_init().

If you disagree, please discuss with Rob (on To:) and try and find a
way forward and let me know how to deal with this.

Thanks.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

[PATCH 4/8] ARM: dove: create a proper PMU driver for power domains, PMU IRQs and resets

[Sebastian Hesselbarth]

On 16.02.2015 16:58, Russell King - ARM Linux wrote:
> On Sat, Feb 14, 2015 at 06:02:11PM +0100, Sebastian Hesselbarth wrote:
>> How about we copy the clk subsystem way of installing early probed
>> pm for DT here?
>>
>> For example:
>>
>> #define PM_OF_DECLARE(name, compat, fn) OF_DECLARE_1(clk, name, compat, fn)
>>
>> and
>>
>> static int __init dove_pmu_init(struct device_node *np) { ... }
>> PM_OF_DECLARE(dove_pmu, "marvell,dove-pmu", dove_init_pmu);
>
> Well, Rob's response was basically "use the machine descriptor" so I
> guess it needs to be explicitly called from
> arch/arm/mach-mvebu/dove.c:dove_init().

Ok, I am very fine with that, too.

Still, we'd have to find a proper place for the driver, don't we?

Sebastian

[PATCH 4/8] ARM: dove: create a proper PMU driver for power domains, PMU IRQs and resets

[Russell King - ARM Linux]

On Mon, Feb 16, 2015 at 05:30:24PM +0100, Sebastian Hesselbarth wrote:
> On 16.02.2015 16:58, Russell King - ARM Linux wrote:
> >On Sat, Feb 14, 2015 at 06:02:11PM +0100, Sebastian Hesselbarth wrote:
> >>How about we copy the clk subsystem way of installing early probed
> >>pm for DT here?
> >>
> >>For example:
> >>
> >>#define PM_OF_DECLARE(name, compat, fn) OF_DECLARE_1(clk, name, compat, fn)
> >>
> >>and
> >>
> >>static int __init dove_pmu_init(struct device_node *np) { ... }
> >>PM_OF_DECLARE(dove_pmu, "marvell,dove-pmu", dove_init_pmu);
> >
> >Well, Rob's response was basically "use the machine descriptor" so I
> >guess it needs to be explicitly called from
> >arch/arm/mach-mvebu/dove.c:dove_init().
> 
> Ok, I am very fine with that, too.
> 
> Still, we'd have to find a proper place for the driver, don't we?

Yep - I'm not sure creating drivers/pmu for (at the moment) one driver
is a particularly good idea.  Maybe something in drivers/soc/ ?

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

[PATCH 4/8] ARM: dove: create a proper PMU driver for power domains, PMU IRQs and resets

[Andrew Lunn]

On Mon, Feb 16, 2015 at 04:52:54PM +0000, Russell King - ARM Linux wrote:
> On Mon, Feb 16, 2015 at 05:30:24PM +0100, Sebastian Hesselbarth wrote:
> > On 16.02.2015 16:58, Russell King - ARM Linux wrote:
> > >On Sat, Feb 14, 2015 at 06:02:11PM +0100, Sebastian Hesselbarth wrote:
> > >>How about we copy the clk subsystem way of installing early probed
> > >>pm for DT here?
> > >>
> > >>For example:
> > >>
> > >>#define PM_OF_DECLARE(name, compat, fn) OF_DECLARE_1(clk, name, compat, fn)
> > >>
> > >>and
> > >>
> > >>static int __init dove_pmu_init(struct device_node *np) { ... }
> > >>PM_OF_DECLARE(dove_pmu, "marvell,dove-pmu", dove_init_pmu);
> > >
> > >Well, Rob's response was basically "use the machine descriptor" so I
> > >guess it needs to be explicitly called from
> > >arch/arm/mach-mvebu/dove.c:dove_init().
> > 
> > Ok, I am very fine with that, too.
> > 
> > Still, we'd have to find a proper place for the driver, don't we?
> 
> Yep - I'm not sure creating drivers/pmu for (at the moment) one driver
> is a particularly good idea.  Maybe something in drivers/soc/ ?

If you did create drivers/pmu, you could move drives/soc/pmc.c into
it.

But drivers/soc also seems like a good place for the dove PMU code.

    Andrew

Re: [PATCH 3/8] pm: domains: sync runtime PM status with PM domains

[Rafael J. Wysocki]

First off, sorry for being slow to respond lately, I'm traveling now.

Also adding Alan Stern to the CC (please always do that for discussions
regarding the runtime PM core).

On Saturday, February 14, 2015 03:27:40 PM Russell King wrote:
> Synchronise the PM domain status with runtime PM's status.  This
> provides a better solution to the problem than commit 2ed127697eb1
> ("PM / Domains: Power on the PM domain right after attach completes").
> 
> The above commit added a call to power up the PM domain when a device
> attaches to the domain.  The assumption is that the device driver
> will cause a runtime PM transition, which will synchronise the PM
> domain state with the runtime PM state.
> 
> However, runtime PM will, by default, assume that the device is
> initially suspended.  So we have two subsystems which have differing
> initial state expectations.  This is silly.
> 
> Runtime PM requires that pm_runtime_set_active() is called before
> pm_runtime_enable() if the device is already powered up.

Well, this is an oversimplification of sorts.

What really is expected is that the RPM status of the device will agree
with its actual state at the moment when pm_runtime_enable() is called.

If the actual state of the device is "not powered", then it is invalid to
call pm_runtime_set_active() before pm_runtime_enable() even.

> However, PM domains are not informed of this, and this is where the problem
> really lies.  We need to keep PM domains properly and fully informed of their
> associated device status.

This goes backwards.  Rather, PM domains should tell everyone about what they
have done to the device IMO.  That is, since PM domains power up the device,
it would be logical to change its RPM status at that point to me.

That may lead to one subtle problem, though.

Suppose that, in addition to either having or not having a power domain under
it, the device has a clock that is managed directly by the driver.  The driver
may then want to do the clock management in its runtime PM callbacks.
However, if genpd changes the device's RPM status to "active", the runtime
PM framework will not execute the resume callback for the device, so things
will break if the clock is not actually enabled to start with.

IOW, we need a way for the driver and genpd to agree on what the RPM status
of the device should be set to before pm_runtime_enable() is executed.

> We fix this by adding a new callback - runtime_set_status() which is

I'm not sure if that's the way to address that, though.

Our intention for the pm_runtime_set_active() etc. calls was that they didn't
trigger any callbacks, since were are supposed to be executed with runtime
PM disabled.

Moreover ->

> triggered whenever a successful call to __pm_runtime_set_status().
> PM domain code hooks into this, and updates the PM domain appropriately.
> 
> This means a driver with the following sequence:
> 
> 	pm_runtime_set_active(dev);
> 	pm_runtime_enable(dev);
> 
> will trigger the PM domain to be powered up at this point, which keeps
> runtime PM and PM domains properly in sync with each other.
> 
> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> ---
>  drivers/base/power/domain.c  | 16 +++++++++++++++-
>  drivers/base/power/runtime.c |  5 +++++
>  include/linux/pm.h           |  1 +
>  3 files changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 11a1023fa64a..2a700cea41fc 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -741,6 +741,20 @@ static int pm_genpd_runtime_resume(struct device *dev)
>  	return 0;
>  }
>  
> +static int pm_genpd_runtime_set_status(struct device *dev)
> +{
> +	int ret;
> +
> +	dev_dbg(dev, "%s()\n", __func__);
> +
> +	if (pm_runtime_suspended(dev))
> +		ret = pm_genpd_runtime_suspend(dev);
> +	else
> +		ret = pm_genpd_runtime_resume(dev);
> +
> +	return ret;
> +}
> +
>  static bool pd_ignore_unused;
>  static int __init pd_ignore_unused_setup(char *__unused)
>  {
> @@ -1907,6 +1921,7 @@ void pm_genpd_init(struct generic_pm_domain *genpd,
>  	genpd->max_off_time_changed = true;
>  	genpd->domain.ops.runtime_suspend = pm_genpd_runtime_suspend;
>  	genpd->domain.ops.runtime_resume = pm_genpd_runtime_resume;
> +	genpd->domain.ops.runtime_set_status = pm_genpd_runtime_set_status;
>  	genpd->domain.ops.prepare = pm_genpd_prepare;
>  	genpd->domain.ops.suspend = pm_genpd_suspend;
>  	genpd->domain.ops.suspend_late = pm_genpd_suspend_late;
> @@ -2223,7 +2238,6 @@ int genpd_dev_pm_attach(struct device *dev)
>  	}
>  
>  	dev->pm_domain->detach = genpd_dev_pm_detach;
> -	pm_genpd_poweron(pd);
>  
>  	return 0;
>  }
> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> index 5070c4fe8542..a958cae67a37 100644
> --- a/drivers/base/power/runtime.c
> +++ b/drivers/base/power/runtime.c
> @@ -981,6 +981,7 @@ int __pm_runtime_set_status(struct device *dev, unsigned int status)
>  	struct device *parent = dev->parent;
>  	unsigned long flags;
>  	bool notify_parent = false;
> +	pm_callback_t callback;
>  	int error = 0;
>  
>  	if (status != RPM_ACTIVE && status != RPM_SUSPENDED)
> @@ -1029,6 +1030,10 @@ int __pm_runtime_set_status(struct device *dev, unsigned int status)
>   out_set:
>  	__update_runtime_status(dev, status);
>  	dev->power.runtime_error = 0;
> +	if (dev->power.no_callbacks)
> +		goto out;
> +	callback = RPM_GET_CALLBACK(dev, runtime_set_status);
> +	rpm_callback(callback, dev);

-> That is specific to PM domains and arguably not needed otherwise, so I would
define it in struct dev_pm_domain outside of dev_pm_ops.

>   out:
>  	spin_unlock_irqrestore(&dev->power.lock, flags);
>  
> diff --git a/include/linux/pm.h b/include/linux/pm.h
> index 8b5976364619..ee098585d577 100644
> --- a/include/linux/pm.h
> +++ b/include/linux/pm.h
> @@ -316,6 +316,7 @@ struct dev_pm_ops {
>  	int (*runtime_suspend)(struct device *dev);
>  	int (*runtime_resume)(struct device *dev);
>  	int (*runtime_idle)(struct device *dev);
> +	int (*runtime_set_status)(struct device *dev);
>  };
>  
>  #ifdef CONFIG_PM_SLEEP
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 3/8] pm: domains: sync runtime PM status with PM domains

[Alan Stern]

On Tue, 17 Feb 2015, Rafael J. Wysocki wrote:

> On Saturday, February 14, 2015 03:27:40 PM Russell King wrote:
> > Synchronise the PM domain status with runtime PM's status.  This
> > provides a better solution to the problem than commit 2ed127697eb1
> > ("PM / Domains: Power on the PM domain right after attach completes").
> > 
> > The above commit added a call to power up the PM domain when a device
> > attaches to the domain.  The assumption is that the device driver
> > will cause a runtime PM transition, which will synchronise the PM
> > domain state with the runtime PM state.
> > 
> > However, runtime PM will, by default, assume that the device is
> > initially suspended.  So we have two subsystems which have differing
> > initial state expectations.  This is silly.

As Rafael says, that's putting it a little strongly.  The runtime PM 
status is initialized to RPM_SUSPENDED, simply because it has to be 
initialized to _something_.  But the core doesn't expect that value to 
be meaningful initially.

> > Runtime PM requires that pm_runtime_set_active() is called before
> > pm_runtime_enable() if the device is already powered up.
> 
> Well, this is an oversimplification of sorts.
> 
> What really is expected is that the RPM status of the device will agree
> with its actual state at the moment when pm_runtime_enable() is called.
> 
> If the actual state of the device is "not powered", then it is invalid to
> call pm_runtime_set_active() before pm_runtime_enable() even.

In practice it wouldn't make any difference, so long as
pm_runtime_set_suspended() is called later on but before
pm_runtime_enable().

> > However, PM domains are not informed of this, and this is where the problem
> > really lies.  We need to keep PM domains properly and fully informed of their
> > associated device status.
> 
> This goes backwards.  Rather, PM domains should tell everyone about what they
> have done to the device IMO.  That is, since PM domains power up the device,
> it would be logical to change its RPM status at that point to me.
> 
> That may lead to one subtle problem, though.
> 
> Suppose that, in addition to either having or not having a power domain under
> it, the device has a clock that is managed directly by the driver.  The driver
> may then want to do the clock management in its runtime PM callbacks.
> However, if genpd changes the device's RPM status to "active", the runtime
> PM framework will not execute the resume callback for the device, so things
> will break if the clock is not actually enabled to start with.
> 
> IOW, we need a way for the driver and genpd to agree on what the RPM status
> of the device should be set to before pm_runtime_enable() is executed.

Agreed.  For example, the PM domain may simply have a policy that all
devices must start out in the active state, and require member drivers
to enforce that policy.  Or the PM domain might directly invoke a
driver's runtime-PM callback routine if the device appears to be in the
"wrong" state initially.

> > We fix this by adding a new callback - runtime_set_status() which is
> 
> I'm not sure if that's the way to address that, though.
> 
> Our intention for the pm_runtime_set_active() etc. calls was that they didn't
> trigger any callbacks, since were are supposed to be executed with runtime
> PM disabled.
> 
> Moreover ->
...
> -> That is specific to PM domains and arguably not needed otherwise, so I would
> define it in struct dev_pm_domain outside of dev_pm_ops.

My thought exactly.

Alan Stern

Re: [PATCH 3/8] pm: domains: sync runtime PM status with PM domains

[Russell King - ARM Linux]

On Tue, Feb 17, 2015 at 07:53:47PM +0100, Rafael J. Wysocki wrote:
> First off, sorry for being slow to respond lately, I'm traveling now.
> 
> Also adding Alan Stern to the CC (please always do that for discussions
> regarding the runtime PM core).

If that is the case, how about adding them into MAINTAINERS?  Without
this information in there, Alan will probably be forgotten for future
patches.

> On Saturday, February 14, 2015 03:27:40 PM Russell King wrote:
> > Runtime PM requires that pm_runtime_set_active() is called before
> > pm_runtime_enable() if the device is already powered up.
> 
> Well, this is an oversimplification of sorts.
> 
> What really is expected is that the RPM status of the device will agree
> with its actual state at the moment when pm_runtime_enable() is called.
> 
> If the actual state of the device is "not powered", then it is invalid to
> call pm_runtime_set_active() before pm_runtime_enable() even.

We're both saying exactly the same thing, so we're in full agreement,
which is good. :)

> > However, PM domains are not informed of this, and this is where the problem
> > really lies.  We need to keep PM domains properly and fully informed of their
> > associated device status.
> 
> This goes backwards.  Rather, PM domains should tell everyone about
> what they have done to the device IMO.  That is, since PM domains
> power up the device, it would be logical to change its RPM status at
> that point to me.

Right, so it may make sense for runtime PM to query the PM domain state,
and synchronise itself with that.

> That may lead to one subtle problem, though.
> 
> Suppose that, in addition to either having or not having a power domain
> under it, the device has a clock that is managed directly by the driver.
> The driver may then want to do the clock management in its runtime PM
> callbacks.  However, if genpd changes the device's RPM status to "active",
> the runtime PM framework will not execute the resume callback for the
> device, so things will break if the clock is not actually enabled to
> start with.
> 
> IOW, we need a way for the driver and genpd to agree on what the RPM status
> of the device should be set to before pm_runtime_enable() is executed.

It's worse than that.  If, in the probe, we decide at a point to query
the PM domain status, and transfer it into the RPM status, how does the
driver know whether it needs to do a "put" to cause a transition from
active to suspended?

In the case where the PM domain was suspended, the RPM status would also
be suspended at that point, and it requires a RPM get to resume it.

If the PM domain was active, then it would require a pm_runtime_put()
operation to allow it to suspend.

This is why I decided that the methodology in the runtime PM code should
apply to PM domains: runtime PM requires the actual state to match the
runtime PM state prior to calling pm_runtime_enable().  We should also
require that the PM domain state must also match the runtime PM state
prior to runtime PM being enabled too.

> > We fix this by adding a new callback - runtime_set_status() which is
> 
> I'm not sure if that's the way to address that, though.

I've come to the conclusion that this isn't a good way to handle it,
because those drivers which may make use of PM domains without using
runtime PM will be probed with the PM domain powered down.

I think we've got a rather sticky problem here, and my proposed
solution will cause problems in that scenario.

Another possibility may be to trigger PM domains to try to power down
the domain when it sees the driver call pm_runtime_enable().  If the
driver never calls pm_runtime_enable(), the PM domain will be left
active.  If it does call this function, the effect of this will be to
synchronise the PM domain with the runtime PM state.

> Our intention for the pm_runtime_set_active() etc. calls was that they didn't
> trigger any callbacks, since were are supposed to be executed with runtime
> PM disabled.
> 
> Moreover ->
> 
> > triggered whenever a successful call to __pm_runtime_set_status().
> > PM domain code hooks into this, and updates the PM domain appropriately.
> > 
> > This means a driver with the following sequence:
> > 
> > 	pm_runtime_set_active(dev);
> > 	pm_runtime_enable(dev);
> > 
> > will trigger the PM domain to be powered up at this point, which keeps
> > runtime PM and PM domains properly in sync with each other.
> > 
> > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> > ---
> >  drivers/base/power/domain.c  | 16 +++++++++++++++-
> >  drivers/base/power/runtime.c |  5 +++++
> >  include/linux/pm.h           |  1 +
> >  3 files changed, 21 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> > index 11a1023fa64a..2a700cea41fc 100644
> > --- a/drivers/base/power/domain.c
> > +++ b/drivers/base/power/domain.c
> > @@ -741,6 +741,20 @@ static int pm_genpd_runtime_resume(struct device *dev)
> >  	return 0;
> >  }
> >  
> > +static int pm_genpd_runtime_set_status(struct device *dev)
> > +{
> > +	int ret;
> > +
> > +	dev_dbg(dev, "%s()\n", __func__);
> > +
> > +	if (pm_runtime_suspended(dev))
> > +		ret = pm_genpd_runtime_suspend(dev);
> > +	else
> > +		ret = pm_genpd_runtime_resume(dev);
> > +
> > +	return ret;
> > +}
> > +
> >  static bool pd_ignore_unused;
> >  static int __init pd_ignore_unused_setup(char *__unused)
> >  {
> > @@ -1907,6 +1921,7 @@ void pm_genpd_init(struct generic_pm_domain *genpd,
> >  	genpd->max_off_time_changed = true;
> >  	genpd->domain.ops.runtime_suspend = pm_genpd_runtime_suspend;
> >  	genpd->domain.ops.runtime_resume = pm_genpd_runtime_resume;
> > +	genpd->domain.ops.runtime_set_status = pm_genpd_runtime_set_status;
> >  	genpd->domain.ops.prepare = pm_genpd_prepare;
> >  	genpd->domain.ops.suspend = pm_genpd_suspend;
> >  	genpd->domain.ops.suspend_late = pm_genpd_suspend_late;
> > @@ -2223,7 +2238,6 @@ int genpd_dev_pm_attach(struct device *dev)
> >  	}
> >  
> >  	dev->pm_domain->detach = genpd_dev_pm_detach;
> > -	pm_genpd_poweron(pd);
> >  
> >  	return 0;
> >  }
> > diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> > index 5070c4fe8542..a958cae67a37 100644
> > --- a/drivers/base/power/runtime.c
> > +++ b/drivers/base/power/runtime.c
> > @@ -981,6 +981,7 @@ int __pm_runtime_set_status(struct device *dev, unsigned int status)
> >  	struct device *parent = dev->parent;
> >  	unsigned long flags;
> >  	bool notify_parent = false;
> > +	pm_callback_t callback;
> >  	int error = 0;
> >  
> >  	if (status != RPM_ACTIVE && status != RPM_SUSPENDED)
> > @@ -1029,6 +1030,10 @@ int __pm_runtime_set_status(struct device *dev, unsigned int status)
> >   out_set:
> >  	__update_runtime_status(dev, status);
> >  	dev->power.runtime_error = 0;
> > +	if (dev->power.no_callbacks)
> > +		goto out;
> > +	callback = RPM_GET_CALLBACK(dev, runtime_set_status);
> > +	rpm_callback(callback, dev);
> 
> -> That is specific to PM domains and arguably not needed otherwise,
> so I would define it in struct dev_pm_domain outside of dev_pm_ops.

How does runtime PM know that we're using PM domains though?  How
does runtime PM know that it can cast the dev_pm_ops pointer safely?

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH 3/8] pm: domains: sync runtime PM status with PM domains

[Rafael J. Wysocki]

On Tuesday, February 17, 2015 07:42:43 PM Russell King - ARM Linux wrote:
> On Tue, Feb 17, 2015 at 07:53:47PM +0100, Rafael J. Wysocki wrote:

[cut]

> > 
> > What really is expected is that the RPM status of the device will agree
> > with its actual state at the moment when pm_runtime_enable() is called.
> > 
> > If the actual state of the device is "not powered", then it is invalid to
> > call pm_runtime_set_active() before pm_runtime_enable() even.
> 
> We're both saying exactly the same thing, so we're in full agreement,
> which is good. :)
> 
> > > However, PM domains are not informed of this, and this is where the problem
> > > really lies.  We need to keep PM domains properly and fully informed of their
> > > associated device status.
> > 
> > This goes backwards.  Rather, PM domains should tell everyone about
> > what they have done to the device IMO.  That is, since PM domains
> > power up the device, it would be logical to change its RPM status at
> > that point to me.
> 
> Right, so it may make sense for runtime PM to query the PM domain state,
> and synchronise itself with that.
> 
> > That may lead to one subtle problem, though.
> > 
> > Suppose that, in addition to either having or not having a power domain
> > under it, the device has a clock that is managed directly by the driver.
> > The driver may then want to do the clock management in its runtime PM
> > callbacks.  However, if genpd changes the device's RPM status to "active",
> > the runtime PM framework will not execute the resume callback for the
> > device, so things will break if the clock is not actually enabled to
> > start with.
> > 
> > IOW, we need a way for the driver and genpd to agree on what the RPM status
> > of the device should be set to before pm_runtime_enable() is executed.
> 
> It's worse than that.  If, in the probe, we decide at a point to query
> the PM domain status, and transfer it into the RPM status, how does the
> driver know whether it needs to do a "put" to cause a transition from
> active to suspended?

When ->probe() runs, the cases are:
(1) There are no power domains, so the device is "active" when the clock is
    enabled and "suspended" when it is disabled.
(2) There is a power domain which is initially off.  The RPM status of the
    device has to be "suspended" or the domain needs to be powered up.
(3) There is a power domain which is initially on.  The RPM status of the
    device depends on the clock state like in (1).  Also it may not be
    possible to power down the domain.

Essentially, (1) and (3) are equivalent from the ->probe() perspective.
Moreover, if the driver does not intend to enable the clock, the device
is "suspended" regardless of the domain state.  This means that the only
really interesting case is (2) and only when ->probe() will attempt to
enable the clock.

In that case it needs to do something like
(a) Bump up the device's runtime PM usage counter.
(b) Execute "power up the device for me" (missing).
(c) Enable the clock, do whatever it wants to the device, set the RPM status
    to "active" and call pm_runtime_enable().
(d) Drop down the device's runtime PM usage counter (the core will trigger
    suspend).

> In the case where the PM domain was suspended, the RPM status would also
> be suspended at that point, and it requires a RPM get to resume it.

Yes, it does, if the driver wants to access the device.

> If the PM domain was active, then it would require a pm_runtime_put()
> operation to allow it to suspend.
>
> This is why I decided that the methodology in the runtime PM code should
> apply to PM domains: runtime PM requires the actual state to match the
> runtime PM state prior to calling pm_runtime_enable().  We should also
> require that the PM domain state must also match the runtime PM state
> prior to runtime PM being enabled too.

Agreed.

> > > We fix this by adding a new callback - runtime_set_status() which is
> > 
> > I'm not sure if that's the way to address that, though.
> 
> I've come to the conclusion that this isn't a good way to handle it,
> because those drivers which may make use of PM domains without using
> runtime PM will be probed with the PM domain powered down.

What would they use PM domains for, then?

PM_RUNTIME is gone now and PM is implied by PM_SLEEP, so you can't have system
suspend support without runtime PM (which presumably might be the case in
question).

> I think we've got a rather sticky problem here, and my proposed
> solution will cause problems in that scenario.
> 
> Another possibility may be to trigger PM domains to try to power down
> the domain when it sees the driver call pm_runtime_enable().  If the
> driver never calls pm_runtime_enable(), the PM domain will be left
> active.  If it does call this function, the effect of this will be to
> synchronise the PM domain with the runtime PM state.

That's more along the lines of what I'm thinking about.  I don't have a clean
idea how to implement that, though.

> > Our intention for the pm_runtime_set_active() etc. calls was that they didn't
> > trigger any callbacks, since were are supposed to be executed with runtime
> > PM disabled.
> > 
> > Moreover ->

[cut]

> > > @@ -1029,6 +1030,10 @@ int __pm_runtime_set_status(struct device *dev, unsigned int status)
> > >   out_set:
> > >  	__update_runtime_status(dev, status);
> > >  	dev->power.runtime_error = 0;
> > > +	if (dev->power.no_callbacks)
> > > +		goto out;
> > > +	callback = RPM_GET_CALLBACK(dev, runtime_set_status);
> > > +	rpm_callback(callback, dev);
> > 
> > -> That is specific to PM domains and arguably not needed otherwise,
> > so I would define it in struct dev_pm_domain outside of dev_pm_ops.
> 
> How does runtime PM know that we're using PM domains though?  How
> does runtime PM know that it can cast the dev_pm_ops pointer safely?

dev->pm_domain is set then, so you basically do

	if (dev->pm_domain && dev->pm_domain->runtime_set_status)
		dev->pm_domain->runtime_set_status(dev);

(or whatever the new callback may be).

And if power domains are in use, dev->pm_domain has to be set before doing
anything with runtime PM to the device or things may break in more interesting
ways.


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 3/8] pm: domains: sync runtime PM status with PM domains

[Russell King - ARM Linux]

On Wed, Feb 18, 2015 at 08:02:30AM +0100, Rafael J. Wysocki wrote:
> On Tuesday, February 17, 2015 07:42:43 PM Russell King - ARM Linux wrote:
> > It's worse than that.  If, in the probe, we decide at a point to query
> > the PM domain status, and transfer it into the RPM status, how does the
> > driver know whether it needs to do a "put" to cause a transition from
> > active to suspended?
> 
> When ->probe() runs, the cases are:
> (1) There are no power domains, so the device is "active" when the clock is
>     enabled and "suspended" when it is disabled.
> (2) There is a power domain which is initially off.  The RPM status of the
>     device has to be "suspended" or the domain needs to be powered up.

(quick reply)

That's not entirely correct.

As a result of 2ed127697eb13 (PM / Domains: Power on the PM domain right
after attach completes) the power domain will _always_ be powered on prior
to ->probe() being called, if the device was attached to the PM domain
just before ->probe() is called, inspite of the domain being powered off
before the device was attached to the domain.

If the PM domain is attached earlier, and pm_genpd_poweroff_unused() is
called (before they're attached, but in the kernel boot sequence) the
work for powering down the PM domains will be queued until a point where
it can be scheduled - which will be after devices are attached.  That can
allow the PM domain(s) to be powered down (as no driver is attached) which
then results in the driver's ->probe function being called with the PM
domain OFF.

So, right now, there's no way for a driver to know with certainty whether
the device it is about to probe is powered up or powered down.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH 3/8] pm: domains: sync runtime PM status with PM domains

[Alan Stern]

On Wed, 18 Feb 2015, Russell King - ARM Linux wrote:

> On Wed, Feb 18, 2015 at 08:02:30AM +0100, Rafael J. Wysocki wrote:
> > On Tuesday, February 17, 2015 07:42:43 PM Russell King - ARM Linux wrote:
> > > It's worse than that.  If, in the probe, we decide at a point to query
> > > the PM domain status, and transfer it into the RPM status, how does the
> > > driver know whether it needs to do a "put" to cause a transition from
> > > active to suspended?
> > 
> > When ->probe() runs, the cases are:
> > (1) There are no power domains, so the device is "active" when the clock is
> >     enabled and "suspended" when it is disabled.
> > (2) There is a power domain which is initially off.  The RPM status of the
> >     device has to be "suspended" or the domain needs to be powered up.
> 
> (quick reply)
> 
> That's not entirely correct.
> 
> As a result of 2ed127697eb13 (PM / Domains: Power on the PM domain right
> after attach completes) the power domain will _always_ be powered on prior
> to ->probe() being called, if the device was attached to the PM domain
> just before ->probe() is called, inspite of the domain being powered off
> before the device was attached to the domain.
> 
> If the PM domain is attached earlier, and pm_genpd_poweroff_unused() is
> called (before they're attached, but in the kernel boot sequence) the
> work for powering down the PM domains will be queued until a point where
> it can be scheduled - which will be after devices are attached.  That can
> allow the PM domain(s) to be powered down (as no driver is attached) which
> then results in the driver's ->probe function being called with the PM
> domain OFF.
> 
> So, right now, there's no way for a driver to know with certainty whether
> the device it is about to probe is powered up or powered down.

Russell:

I'm not totally familiar with how PM domains are intended to work.  The 
impression I get from looking through the code is that they are highly 
over-engineered -- but that could just be a result of my ignorance.

At any rate, I don't have a clear picture of the problem you are trying
to solve.  What's the general outline?  Are you talking about a device
that _belongs_ to a PM domain or one that _depends_ on a PM domain?  
If the device belongs to the domain, how does it get added (i.e., by
its driver during probe or by some other mechanism)?

Are you asking about how the driver can tell what the PM domain's 
runtime status is?  Or would you like to add a way for the driver to 
set the PM domain's runtime status?

What else am I missing?

Alan Stern

Re: [PATCH 3/8] pm: domains: sync runtime PM status with PM domains

[Russell King - ARM Linux]

On Wed, Feb 18, 2015 at 10:12:17AM -0500, Alan Stern wrote:
> Russell:
> 
> I'm not totally familiar with how PM domains are intended to work.  The 
> impression I get from looking through the code is that they are highly 
> over-engineered -- but that could just be a result of my ignorance.
> 
> At any rate, I don't have a clear picture of the problem you are trying
> to solve.  What's the general outline?  Are you talking about a device
> that _belongs_ to a PM domain or one that _depends_ on a PM domain?  
> If the device belongs to the domain, how does it get added (i.e., by
> its driver during probe or by some other mechanism)?
> 
> Are you asking about how the driver can tell what the PM domain's 
> runtime status is?  Or would you like to add a way for the driver to 
> set the PM domain's runtime status?
> 
> What else am I missing?

The problem is that the PM domain stuff doesn't work very well, and I
believe it's problems are just being hacked around rather than being
viewed from a higher level and fixed properly. :)

The issue that I'm seeing with 3.19 is that I have a device which
does not need to be powered up to probe.  It's default state when the
kernel initially starts to boot is that the power domain associated
with it is powered down.

Hence, the driver is written such that its probe function does this:

        pm_runtime_use_autosuspend(vi->dev);
        pm_runtime_set_autosuspend_delay(vi->dev, 100);
        pm_runtime_enable(vi->dev);

The driver lives as a module on the filesystem, and so it's loaded
after the kernel has mounted its rootfs and udev has started.

Now, I boot this platform in two ways: I have a DT mode (which I have
found is not that stable - in that it causes problems with HDMI), and
a legacy mode.

When I boot in legacy mode, the PM domain is created early in the
kernel boot (at arch_initcall time).  As part of the PM domain
initialisation, I read the current state of the hardware and tell
PM domains whether it was enabled or not.  In this case, it starts
off disabled.

Right after they're created, I call pm_genpd_poweroff_unused() and
then register a notifier for platform devices.  The call to
pm_genpd_poweroff_unused() causes a work to be queued, which will
only get executed when we reach a scheduling or preemption point.

The platform devices then get created, and the notifier attaches the
appropriate PM domains to the devices.

In 3.18 and previous kernels, this resulted in the PM domains being
left in their initial state.  This means that the presumption by the
driver that the device is suspended is correct and valid.

In 3.19, this causes the PM domain to be powered up immediately.
However, because we get to a preemption or scheduling point before
the driver module is inserted, this allows the PM domain to power
back down.

The module is then loaded, and everything works correctly.  The PM
domain debugfs file indicates that the PM domain is off.


Now, if I boot in DT mode, the game changes.  In this case, the
PM domains are created as above, but without the platform device
notifier - we rely on the generic code to attach the PM domain to
the device a driver probe time.

What this means is that when the pm_genpd_poweroff_unused() work
executes, it finds that the domain is already powered down.

The module is then loaded as in the legacy case, and at this point,
the PM domain is attached to the device just before the driver is
probed.  This causes the device to be powered on at this point.
However, the runtime PM state remains as its initialisation default
- suspended.

Time passes, and the PM domain debug file continues to indicate that
the runtime PM state of the device is suspended, but the PM domain
state is "on".


So, if I boot in legacy mode, things work fine, but only because of
the fluke that the pm_genpd_poweroff_unused() happens to be executed
before the device is probed, undoing the powering up of the domain
at attachment time.  If I boot in DT mode, the order changes, and I'm
left with the PM domain and RPM in an inconsistent state.

Right now, there is _no_ way for a device driver to know whether the
PM domain attached to its device is powered up or not.  As runtime PM
needs to know (from the device driver) the initial state of the device,
the device driver can not determine whether it is powered up or not.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH 3/8] pm: domains: sync runtime PM status with PM domains

[Rafael J. Wysocki]

On Wednesday, February 18, 2015 10:03:22 AM Russell King - ARM Linux wrote:
> On Wed, Feb 18, 2015 at 08:02:30AM +0100, Rafael J. Wysocki wrote:
> > On Tuesday, February 17, 2015 07:42:43 PM Russell King - ARM Linux wrote:
> > > It's worse than that.  If, in the probe, we decide at a point to query
> > > the PM domain status, and transfer it into the RPM status, how does the
> > > driver know whether it needs to do a "put" to cause a transition from
> > > active to suspended?
> > 
> > When ->probe() runs, the cases are:
> > (1) There are no power domains, so the device is "active" when the clock is
> >     enabled and "suspended" when it is disabled.
> > (2) There is a power domain which is initially off.  The RPM status of the
> >     device has to be "suspended" or the domain needs to be powered up.
> 
> (quick reply)
> 
> That's not entirely correct.
> 
> As a result of 2ed127697eb13 (PM / Domains: Power on the PM domain right
> after attach completes) the power domain will _always_ be powered on prior
> to ->probe() being called, if the device was attached to the PM domain
> just before ->probe() is called, inspite of the domain being powered off
> before the device was attached to the domain.
> 
> If the PM domain is attached earlier, and pm_genpd_poweroff_unused() is
> called (before they're attached, but in the kernel boot sequence) the
> work for powering down the PM domains will be queued until a point where
> it can be scheduled - which will be after devices are attached.  That can
> allow the PM domain(s) to be powered down (as no driver is attached) which
> then results in the driver's ->probe function being called with the PM
> domain OFF.
> 
> So, right now, there's no way for a driver to know with certainty whether
> the device it is about to probe is powered up or powered down.

And it looks that the driver is in a module which is only loaded after we've
called the pm_genpd_poweroff_unused().  So clearly commit 2ed127697eb13 doesn't
work for that driver.

We might just revert that commit, but that wouldn't make the problem go away.
Namely, the domain may be powered up or down by *another* driver whose device
belongs to it in parallel with your ->probe() anyway.

Rafael

Re: [PATCH 3/8] pm: domains: sync runtime PM status with PM domains

[Rafael J. Wysocki]

On Wednesday, February 18, 2015 10:12:17 AM Alan Stern wrote:
> On Wed, 18 Feb 2015, Russell King - ARM Linux wrote:
> 
> > On Wed, Feb 18, 2015 at 08:02:30AM +0100, Rafael J. Wysocki wrote:
> > > On Tuesday, February 17, 2015 07:42:43 PM Russell King - ARM Linux wrote:
> > > > It's worse than that.  If, in the probe, we decide at a point to query
> > > > the PM domain status, and transfer it into the RPM status, how does the
> > > > driver know whether it needs to do a "put" to cause a transition from
> > > > active to suspended?
> > > 
> > > When ->probe() runs, the cases are:
> > > (1) There are no power domains, so the device is "active" when the clock is
> > >     enabled and "suspended" when it is disabled.
> > > (2) There is a power domain which is initially off.  The RPM status of the
> > >     device has to be "suspended" or the domain needs to be powered up.
> > 
> > (quick reply)
> > 
> > That's not entirely correct.
> > 
> > As a result of 2ed127697eb13 (PM / Domains: Power on the PM domain right
> > after attach completes) the power domain will _always_ be powered on prior
> > to ->probe() being called, if the device was attached to the PM domain
> > just before ->probe() is called, inspite of the domain being powered off
> > before the device was attached to the domain.
> > 
> > If the PM domain is attached earlier, and pm_genpd_poweroff_unused() is
> > called (before they're attached, but in the kernel boot sequence) the
> > work for powering down the PM domains will be queued until a point where
> > it can be scheduled - which will be after devices are attached.  That can
> > allow the PM domain(s) to be powered down (as no driver is attached) which
> > then results in the driver's ->probe function being called with the PM
> > domain OFF.
> > 
> > So, right now, there's no way for a driver to know with certainty whether
> > the device it is about to probe is powered up or powered down.
> 
> Russell:
> 
> I'm not totally familiar with how PM domains are intended to work.  The 
> impression I get from looking through the code is that they are highly 
> over-engineered

Well, that very likely is my fault. :-)

> -- but that could just be a result of my ignorance.

You may be right anyway, though ...

> At any rate, I don't have a clear picture of the problem you are trying
> to solve.  What's the general outline?  Are you talking about a device
> that _belongs_ to a PM domain or one that _depends_ on a PM domain?  
> If the device belongs to the domain, how does it get added (i.e., by
> its driver during probe or by some other mechanism)?
> 
> Are you asking about how the driver can tell what the PM domain's 
> runtime status is?  Or would you like to add a way for the driver to 
> set the PM domain's runtime status?

The problem, as I see it, is that drivers have certain expectations about the
initial power states of devices that may or may not be met if power domains
are in use.

Those drivers might have been developed on systems without direct control of
power domains where the conditions for the driver at the ->probe() time were
always the same.  With power domains, though, they may change depending on
what's going on with the other devices in the domain, for example.

So the question is how we can address that in the cleanest way possible.

Rafael

Re: [PATCH 3/8] pm: domains: sync runtime PM status with PM domains

[Russell King - ARM Linux]

On Wed, Feb 18, 2015 at 05:52:25PM +0100, Rafael J. Wysocki wrote:
> Those drivers might have been developed on systems without direct control of
> power domains where the conditions for the driver at the ->probe() time were
> always the same.  With power domains, though, they may change depending on
> what's going on with the other devices in the domain, for example.
> 
> So the question is how we can address that in the cleanest way possible.

Okay, going back to what was said earlier, we have three possibilities
for drivers:

1) A driver which makes no use of runtime PM and no use of PM domains.
2) A driver which makes no use of runtime PM but may have a PM domain
   attached.
3) A driver which uses runtime PM, and assumes that the device was
   initially suspended.  It calls pm_enable_device() optionally with
   a preceding call to pm_runtime_set_suspended().
4) A driver which uses runtime PM, and calls pm_runtime_set_active()
   followed by pm_enable_device().

What we want to end up with in the ideal situation is that drivers which
fall into class:

1 - may see their devices in any state; it is up to them to deal with
    whatever state the device is initially in.
2 - should see their devices in a powered up state as they have no way
    to inform that the device is active.
3 - should see their devices in a suspended state.
4 - should see their devices in a powered up state.

The problem here is that we have no way to know this prior to the drivers
probe function being called.  Whatever we do at this point, it is not
going to satisfy the requirements of everyone.

So, let's take what we're currently doing on DT, and make it the same
across the board.  In platform_drv_probe(), let's do this:

	/* attach the domain */
        ret = dev_pm_domain_attach(_dev, true);
	if (ret == -EPROBE_DEFER)
		goto defer;

	/* power up the domain - and hold it powered up */
	ret = dev_pm_domain_pre_probe(_dev);
	if (ret != -ENOSYS)
		goto error;

	ret = drv->probe(dev);

	/*
	 * remove the "hold" on the domain by this device, and start
	 * tracking its runtime PM status.
	 */
	dev_pm_domain_post_probe(_dev);

	if (ret)
		dev_pm_domain_detach(_dev, true);

What this means is that while the probe function is running, we guarantee
that the PM domain will always be powered up.  We also guarantee that
after the probe function has returned, we will then start tracking the
runtime PM state, and if the device driver leaves runtime PM in the
"suspended" state, PM domains will get to hear about it at that point,
and can transition the PM domain back to "off" mode.

Both these transitions only cause the PM domain to be affected; no
runtime PM callbacks are issued for either of these two changes.  (We
already have that for the initial state right now where attaching a
device to the PM domain causes the PM domain to be powered up.)

This is merely an extension of the current scheme.  Think of
dev_pm_domain_post_probe() as a method of synchronising the state of
PM domains with the state of runtime PM across all attached devices.


Aside:
I need to do some further checks; while considering this, I think I've
come across a worrying problem with "PM / Domains: Power on the PM domain
right after attach completes".  I think this will result in the driver's
runtime_resume callback being invoked _before_ the probe function.  I
can't test this right now as I'm in the middle of upgrading my dev box
and it doesn't have a functional install for building ARM kernels yet.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH 3/8] pm: domains: sync runtime PM status with PM domains

[Rafael J. Wysocki]

On Wednesday, February 18, 2015 05:26:51 PM Russell King - ARM Linux wrote:
> On Wed, Feb 18, 2015 at 05:52:25PM +0100, Rafael J. Wysocki wrote:
> > Those drivers might have been developed on systems without direct control of
> > power domains where the conditions for the driver at the ->probe() time were
> > always the same.  With power domains, though, they may change depending on
> > what's going on with the other devices in the domain, for example.
> > 
> > So the question is how we can address that in the cleanest way possible.
> 
> Okay, going back to what was said earlier, we have three possibilities
> for drivers:
> 
> 1) A driver which makes no use of runtime PM and no use of PM domains.
> 2) A driver which makes no use of runtime PM but may have a PM domain
>    attached.
> 3) A driver which uses runtime PM, and assumes that the device was
>    initially suspended.  It calls pm_enable_device() optionally with
>    a preceding call to pm_runtime_set_suspended().
> 4) A driver which uses runtime PM, and calls pm_runtime_set_active()
>    followed by pm_enable_device().
> 
> What we want to end up with in the ideal situation is that drivers which
> fall into class:
> 
> 1 - may see their devices in any state; it is up to them to deal with
>     whatever state the device is initially in.
> 2 - should see their devices in a powered up state as they have no way
>     to inform that the device is active.
> 3 - should see their devices in a suspended state.
> 4 - should see their devices in a powered up state.
> 
> The problem here is that we have no way to know this prior to the drivers
> probe function being called.  Whatever we do at this point, it is not
> going to satisfy the requirements of everyone.
> 
> So, let's take what we're currently doing on DT, and make it the same
> across the board.  In platform_drv_probe(), let's do this:
> 
> 	/* attach the domain */
>         ret = dev_pm_domain_attach(_dev, true);
> 	if (ret == -EPROBE_DEFER)
> 		goto defer;
> 
> 	/* power up the domain - and hold it powered up */
> 	ret = dev_pm_domain_pre_probe(_dev);
> 	if (ret != -ENOSYS)
> 		goto error;
> 
> 	ret = drv->probe(dev);
> 
> 	/*
> 	 * remove the "hold" on the domain by this device, and start
> 	 * tracking its runtime PM status.
> 	 */
> 	dev_pm_domain_post_probe(_dev);
> 
> 	if (ret)
> 		dev_pm_domain_detach(_dev, true);
> 
> What this means is that while the probe function is running, we guarantee
> that the PM domain will always be powered up.  We also guarantee that
> after the probe function has returned, we will then start tracking the
> runtime PM state, and if the device driver leaves runtime PM in the
> "suspended" state, PM domains will get to hear about it at that point,
> and can transition the PM domain back to "off" mode.
> 
> Both these transitions only cause the PM domain to be affected; no
> runtime PM callbacks are issued for either of these two changes.  (We
> already have that for the initial state right now where attaching a
> device to the PM domain causes the PM domain to be powered up.)
> 
> This is merely an extension of the current scheme.  Think of
> dev_pm_domain_post_probe() as a method of synchronising the state of
> PM domains with the state of runtime PM across all attached devices.

I like this.

And I would add two new "pre_probe" and "post_probe" (what about "init"
and "remove"?) to struct dev_pm_domain for that.

The "pre_probe" ("init") thing would be useful for solving one more problem
related to PM domains I have elsewhere.  In that case I need to defer probing
one of the device in the domain until one of the other devices is registered.

Rafael

Re: [PATCH 3/8] pm: domains: sync runtime PM status with PM domains

[Alan Stern]

On Wed, 18 Feb 2015, Russell King - ARM Linux wrote:

> So, let's take what we're currently doing on DT, and make it the same
> across the board.  In platform_drv_probe(), let's do this:
> 
> 	/* attach the domain */
>         ret = dev_pm_domain_attach(_dev, true);
> 	if (ret == -EPROBE_DEFER)
> 		goto defer;
> 
> 	/* power up the domain - and hold it powered up */
> 	ret = dev_pm_domain_pre_probe(_dev);
> 	if (ret != -ENOSYS)
> 		goto error;
> 
> 	ret = drv->probe(dev);
> 
> 	/*
> 	 * remove the "hold" on the domain by this device, and start
> 	 * tracking its runtime PM status.
> 	 */
> 	dev_pm_domain_post_probe(_dev);
> 
> 	if (ret)
> 		dev_pm_domain_detach(_dev, true);
> 
> What this means is that while the probe function is running, we guarantee
> that the PM domain will always be powered up.  We also guarantee that
> after the probe function has returned, we will then start tracking the
> runtime PM state, and if the device driver leaves runtime PM in the
> "suspended" state, PM domains will get to hear about it at that point,
> and can transition the PM domain back to "off" mode.
> 
> Both these transitions only cause the PM domain to be affected; no
> runtime PM callbacks are issued for either of these two changes.  (We
> already have that for the initial state right now where attaching a
> device to the PM domain causes the PM domain to be powered up.)
> 
> This is merely an extension of the current scheme.  Think of
> dev_pm_domain_post_probe() as a method of synchronising the state of
> PM domains with the state of runtime PM across all attached devices.

This seems like the right sort of approach.

The Runtime PM core has a general philosophy that it can never force
anything to be in the suspended state.  Therefore if you need something
to be in a well-defined power state for any length of time (or if you
need to know the power state), you have to force it into the active
state.

> Aside:
> I need to do some further checks; while considering this, I think I've
> come across a worrying problem with "PM / Domains: Power on the PM domain
> right after attach completes".  I think this will result in the driver's
> runtime_resume callback being invoked _before_ the probe function.  I
> can't test this right now as I'm in the middle of upgrading my dev box
> and it doesn't have a functional install for building ARM kernels yet.

Subsystems always face this problem, because the device core sets
dev->driver before calling the probe routine.  (There's a similar
problem on the other side, where the device core doesn't clear
dev->driver until after the remove routine has returned.)  To cope with
this, the subsystem-level runtime-PM callbacks have to be careful not
to invoke the driver-level callbacks before probing is finished or
after removal has started.

Alan Stern

Re: [PATCH 3/8] pm: domains: sync runtime PM status with PM domains

[Ulf Hansson]

On 18 February 2015 at 19:05, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Wednesday, February 18, 2015 05:26:51 PM Russell King - ARM Linux wrote:
>> On Wed, Feb 18, 2015 at 05:52:25PM +0100, Rafael J. Wysocki wrote:
>> > Those drivers might have been developed on systems without direct control of
>> > power domains where the conditions for the driver at the ->probe() time were
>> > always the same.  With power domains, though, they may change depending on
>> > what's going on with the other devices in the domain, for example.
>> >
>> > So the question is how we can address that in the cleanest way possible.
>>
>> Okay, going back to what was said earlier, we have three possibilities
>> for drivers:
>>
>> 1) A driver which makes no use of runtime PM and no use of PM domains.
>> 2) A driver which makes no use of runtime PM but may have a PM domain
>>    attached.
>> 3) A driver which uses runtime PM, and assumes that the device was
>>    initially suspended.  It calls pm_enable_device() optionally with
>>    a preceding call to pm_runtime_set_suspended().
>> 4) A driver which uses runtime PM, and calls pm_runtime_set_active()
>>    followed by pm_enable_device().
>>
>> What we want to end up with in the ideal situation is that drivers which
>> fall into class:
>>
>> 1 - may see their devices in any state; it is up to them to deal with
>>     whatever state the device is initially in.
>> 2 - should see their devices in a powered up state as they have no way
>>     to inform that the device is active.
>> 3 - should see their devices in a suspended state.
>> 4 - should see their devices in a powered up state.
>>
>> The problem here is that we have no way to know this prior to the drivers
>> probe function being called.  Whatever we do at this point, it is not
>> going to satisfy the requirements of everyone.
>>
>> So, let's take what we're currently doing on DT, and make it the same
>> across the board.  In platform_drv_probe(), let's do this:
>>
>>       /* attach the domain */
>>         ret = dev_pm_domain_attach(_dev, true);
>>       if (ret == -EPROBE_DEFER)
>>               goto defer;
>>
>>       /* power up the domain - and hold it powered up */
>>       ret = dev_pm_domain_pre_probe(_dev);
>>       if (ret != -ENOSYS)
>>               goto error;
>>
>>       ret = drv->probe(dev);
>>
>>       /*
>>        * remove the "hold" on the domain by this device, and start
>>        * tracking its runtime PM status.
>>        */
>>       dev_pm_domain_post_probe(_dev);
>>
>>       if (ret)
>>               dev_pm_domain_detach(_dev, true);
>>
>> What this means is that while the probe function is running, we guarantee
>> that the PM domain will always be powered up.  We also guarantee that
>> after the probe function has returned, we will then start tracking the
>> runtime PM state, and if the device driver leaves runtime PM in the
>> "suspended" state, PM domains will get to hear about it at that point,
>> and can transition the PM domain back to "off" mode.
>>
>> Both these transitions only cause the PM domain to be affected; no
>> runtime PM callbacks are issued for either of these two changes.  (We
>> already have that for the initial state right now where attaching a
>> device to the PM domain causes the PM domain to be powered up.)
>>
>> This is merely an extension of the current scheme.  Think of
>> dev_pm_domain_post_probe() as a method of synchronising the state of
>> PM domains with the state of runtime PM across all attached devices.
>
> I like this.

Me too!

>
> And I would add two new "pre_probe" and "post_probe" (what about "init"
> and "remove"?) to struct dev_pm_domain for that.

Actually what Russell propose, is almost exactly what I proposed in
the following below patchset way back. At that point we decided to
reject that approach.
The only difference is the naming of dev_pm_domain_pre|post_probe(). I
decided to name them dev_pm_domain_get|put(). :-)

http://marc.info/?l=linux-pm&m=141320895122707&w=2

>
> The "pre_probe" ("init") thing would be useful for solving one more problem
> related to PM domains I have elsewhere.  In that case I need to defer probing
> one of the device in the domain until one of the other devices is registered.
>
> Rafael
>

So I wonder if I should maybe refresh that patchset? Or maybe Russell
would like to pick them up?

No matter what, I happy to help!

Kind regards
Uffe

Re: [PATCH 3/8] pm: domains: sync runtime PM status with PM domains

[Russell King - ARM Linux]

On Wed, Mar 04, 2015 at 08:57:40PM +0100, Ulf Hansson wrote:
> So I wonder if I should maybe refresh that patchset? Or maybe Russell
> would like to pick them up?

I'm in no state to do anything at the moment - see my mail last night
"rmk's taking it easy for a while" (which everyone seems to have
ignored).

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.