Dove PMU support

Signed-off-by Russell King <[email protected]>

---
 drivers/base/power/domain.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

drivers/base/power/domain.c +3 -3

index bfb8955c406c..ea91ea0e137b 100644
@@ -42,7 +42,7 @@
 	struct gpd_timing_data *__td = &dev_gpd_data(dev)->td;			\
 	if (!__retval && __elapsed > __td->field) {				\
 		__td->field = __elapsed;					\
-		dev_warn(dev, name " latency exceeded, new value %lld ns\n",	\
+		dev_dbg(dev, name " latency exceeded, new value %lld ns\n",	\
 			__elapsed);						\
 		genpd->max_off_time_changed = true;				\
 		__td->constraint_changed = true;				\

@@ -242,7 +242,7 @@ static int __pm_genpd_poweron(struct generic_pm_domain *genpd)
 			genpd->max_off_time_changed = true;
 			genpd_recalc_cpu_exit_latency(genpd);
 			if (genpd->name)
-				pr_warning("%s: Power-on latency exceeded, "
+				pr_debug("%s: Power-on latency exceeded, "
 					"new value %lld ns\n", genpd->name,
 					elapsed_ns);
 		}

@@ -566,7 +566,7 @@ static int pm_genpd_poweroff(struct generic_pm_domain *genpd)
 			genpd->power_off_latency_ns = elapsed_ns;
 			genpd->max_off_time_changed = true;
 			if (genpd->name)
-				pr_warning("%s: Power-off latency exceeded, "
+				pr_debug("%s: Power-off latency exceeded, "
 					"new value %lld ns\n", genpd->name,
 					elapsed_ns);
 		}

Quoted 1 line · Russell King

Signed-off-by Russell King <[email protected]>

Acked-by Rafael J. Wysocki <[email protected]>

Since you need this one and [2/5] for the rest of the patchset, please feel free to take them through your tree if that's convenient.

Quoted 29 lines

---
 drivers/base/power/domain.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

drivers/base/power/domain.c +2 -2

index bfb8955c406c..ea91ea0e137b 100644
@@ -42,7 +42,7 @@
      struct gpd_timing_data *__td = &dev_gpd_data(dev)->td;                  \
      if (!__retval && __elapsed > __td->field) {                             \
              __td->field = __elapsed;                                        \
-             dev_warn(dev, name " latency exceeded, new value %lld ns\n",    \
+             dev_dbg(dev, name " latency exceeded, new value %lld ns\n",     \
                      __elapsed);                                             \
              genpd->max_off_time_changed = true;                             \
              __td->constraint_changed = true;                                \

@@ -242,7 +242,7 @@ static int __pm_genpd_poweron(struct generic_pm_domain *genpd)
                      genpd->max_off_time_changed = true;
                      genpd_recalc_cpu_exit_latency(genpd);
                      if (genpd->name)
-                             pr_warning("%s: Power-on latency exceeded, "
+                             pr_debug("%s: Power-on latency exceeded, "
                                      "new value %lld ns\n", genpd->name,
                                      elapsed_ns);
              }

drivers/base/power/domain.c static int pm_genpd_poweroff(struct generic_pm_domain *genpd) · line 572

Quoted 8 lines

drivers/base/power/domain.c +1 -1

@@ -566,7 +566,7 @@ static int pm_genpd_poweroff(struct generic_pm_domain *genpd)
                      genpd->power_off_latency_ns = elapsed_ns;
                      genpd->max_off_time_changed = true;
                      if (genpd->name)
-                             pr_warning("%s: Power-off latency exceeded, "
+                             pr_debug("%s: Power-off latency exceeded, "
                                      "new value %lld ns\n", genpd->name,
                                      elapsed_ns);
              }

--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 5 lines · Rafael J. Wysocki

Quoted 1 line · Russell King

Signed-off-by Russell King <[email protected]>

Acked-by Rafael J. Wysocki <[email protected]>

Quoted 2 lines

Since you need this one and [2/5] for the rest of the patchset, please feel free to take them through your tree if that's convenient.

Unless it complicates things for Russell, I would prefer if this could go via Rafael's tree.

I have a quite big patchset for the PM domain, it would help me if I have a single point to base my work upon.

Additionally, you have Tomasz Figa patchset below to consider for the PM domain code.

[PATCH v3 0/3] Generic Device Tree based power domain look-up

Quoted 9 lines · Ulf Hansson

Quoted 7 lines · Rafael J. Wysocki

Quoted 1 line · Russell King

Signed-off-by Russell King <[email protected]>

Acked-by Rafael J. Wysocki <[email protected]>

Since you need this one and [2/5] for the rest of the patchset, please feel free to take them through your tree if that's convenient.

Quoted 5 lines

Unless it complicates things for Russell, I would prefer if this could go via Rafael's tree.

I have a quite big patchset for the PM domain, it would help me if I have a single point to base my work upon.

No problem for me, but I'm not sure about what Russell things.

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.)

Signed-off-by Russell King <[email protected]>

---
 drivers/base/power/domain.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/base/power/domain.c +1 -1

index ea91ea0e137b..9d8faecc060c 100644
@@ -1531,7 +1531,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;

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.

Quoted 9 lines · 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.

Quoted 8 lines

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.

Hi Russell,

This patch would be simplified if this was based upon the not yet merged patchset from Tomasz Figa, "[PATCH v3 0/3] Generic Device Tree based power domain look-up".

For example you would likely not need to add some of the marvel specific DT bindings, and you wouldn?t need the bus_notifiers to add devices to the power domain. I guess I just though it could be useful input to consider while going forward, unless you already knew.

Quoted 9 lines · 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.

Quoted 8 lines

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.

Hi Russell,

This patch would be simplified if this was based upon the not yet merged patchset from Tomasz Figa, "[PATCH v3 0/3] Generic Device Tree based power domain look-up".

For example you would likely not need to add some of the marvel specific DT bindings, and you wouldn?t need the bus_notifiers to add devices to the power domain. I guess I just though it could be useful input to consider while going forward, unless you already knew.

Quoted 21 lines · Ulf Hansson

Quoted 9 lines · 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.

Quoted 8 lines

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.

Hi Russell,

Quoted 8 lines

This patch would be simplified if this was based upon the not yet merged patchset from Tomasz Figa, "[PATCH v3 0/3] Generic Device Tree based power domain look-up".

For example you would likely not need to add some of the marvel specific DT bindings, and you wouldn?t need the bus_notifiers to add devices to the power domain. I guess I just though it could be useful input to consider while going forward, unless you already knew.

In 3.19, I notice something of an odd behaviour.

My vMeta driver has runtime PM support enabled. When I explicitly register the PM domain in the pmu driver via a bus notifier, I see:

root at cubox:~# cat /sys/kernel/debug/pm_genpd/pm_genpd_summary
domain status slaves
/device runtime status
----------------------------------------------------------------------
gpu-domain on
/devices/platform/vivante/etnaviv-gpu,2d active
vpu-domain off
/devices/platform/mbus/mbus:internal-regs/f1c00000.video-decoder suspended

But when I disable that, and let the generic code do the registration, I instead get:

root at cubox:~# cat /sys/kernel/debug/pm_genpd/pm_genpd_summary
domain status slaves
/device runtime status
----------------------------------------------------------------------
gpu-domain on
/devices/platform/vivante/etnaviv-gpu,2d active
vpu-domain on
/devices/platform/mbus/mbus:internal-regs/f1c00000.video-decoder suspended

The difference being that the vpu domain remains powered.

The only difference code-wise seems to be when genpd_dev_pm_attach() is called. In the working case, it's before the device is considered for probing. In the non-working case, it's just before the device is probed.

With debugging enabled in the PM domain code, with the former case I get:

Added domain provider from /mbus/internal-regs/power-management at d0000/vpu-domain platform f1c00000.video-decoder: adding to PM domain vpu-domain platform f1c00000.video-decoder: __pm_genpd_add_device()

With the latter non-working case:

Added domain provider from /mbus/internal-regs/power-management at d0000/vpu-domain
...
ap510-vmeta f1c00000.video-decoder: adding to PM domain vpu-domain ap510-vmeta f1c00000.video-decoder: __pm_genpd_add_device()
vpu-domain: Power-on latency exceeded, new value 1578 ns

Neither of these debug messages provide much hint as to what the difference is, or the cause of the PM domain code being de-sync'd with its devices.

Maybe the PM code needs more debugging in it, and maybe the debugfs file should always be present if debugfs support is enabled?

I think what's going on is that there's a difference in the expectations from the PM domain code vs the runtime PM code. I refer to section 5 of the runtime PM documentation:

| 5. Runtime PM Initialization, Device Probing and Removal
| 
| Initially, the runtime PM is disabled for all devices, which means that the
| majority of the runtime PM helper functions described in Section 4 will return
| -EAGAIN until pm_runtime_enable() is called for the device.
| 
| In addition to that, the initial runtime PM status of all devices is
| 'suspended', but it need not reflect the actual physical state of the device.
| Thus, if the device is initially active (i.e. it is able to process I/O), its
| runtime PM status must be changed to 'active', with the help of
| pm_runtime_set_active(), before pm_runtime_enable() is called for the device.

However, the PM domain code seems to always power up the PM domain when a device is attached to it:

int genpd_dev_pm_attach(struct device *dev)
{
...

        pm_genpd_poweron(pd);

        return 0;
}

EXPORT_SYMBOL_GPL(genpd_dev_pm_attach);

So, the PM domain code ends up disagreeing with the runtime PM code about the state of the device.

I think your commit (2ed127697eb1 "PM / Domains: Power on the PM domain right after attach completes") is fundamentally wrong. The assertion you make in there is built upon the assumption that every driver will call pm_runtime_set_active(), which is not an assumption you can make.

Instead, you should be doing is to hook into __pm_runtime_set_status() and use that to trigger the PM domain power up so that the runtime PM and PM domain state is always in step with each other.

What I'm certain of is that the current situation is just totally crazy.

The following series of patches add better PMU support for Dove. This has been developed on the Cubox, and tested in non-DT and DT modes.

This also improves the interrupt handling over the existing code: the existing code ends up calling the interrupt handlers twice for every interrupt raised, because the interrupt clear-down is done at the wrong point - we need to clear down the interrupt in the device first, then clear it down in the controller.

The problem this gives is that it can be racy (see comments in the driver) so we're careful about how we do that to minimise the window.

I've included all patches here - the initial set are targetted towards adding DT support, with the final adding the non-DT support. There is a call to the initialisation function missing for DT mode - I'd like the mvebu people to comment on how that should be handled, as it needs to be done pretty early.

Also included are two PM domain changes: the first I've discussed with Rafael who seems happy with it. The second is necessary because we have no way to know if a generic PM domain is associated with a device or whether something else making use of the PM domain is installed in the dev->pm_domain pointer, so this allows that decision to be made by core PM code.

This is more a "this is where I'm at" with this stuff than a real submission, nevertheless comments on how to get it ready for submission would be welcome. I'd like to get this off my plate ASAP.

  arch/arm/Kconfig                     |   1 +
  arch/arm/boot/dts/dove.dtsi          |   7 +
  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             | 531 +++++++++++++++++++++++++++++++++++
  drivers/base/power/domain.c          |   8 +-
  9 files changed, 547 insertions(+), 108 deletions(-)

Russell,

thanks for dropping those patches. I know you are packed with a bunch of other patch sets, so if you agree, I can pick up your Dove related patches and finish them.

One thing that comes into my mind is, that we moved Dove DT to mach-mvebu starting with v3.15-rc1 so we need to find a better place for the driver than mach-dove.

Quoted 9 lines

Russell,

thanks for dropping those patches. I know you are packed with a bunch of other patch sets, so if you agree, I can pick up your Dove related patches and finish them.

One thing that comes into my mind is, that we moved Dove DT to mach-mvebu starting with v3.15-rc1 so we need to find a better place for the driver than mach-dove.

Create drivers/pmu ?

The cpufreq driver also needs access to registers within the pmu range. Should it be part of pmu.c, or should we export a regmap which cpufreq can use?

Quoted 11 lines · Andrew Lunn

Quoted 9 lines

Russell,

thanks for dropping those patches. I know you are packed with a bunch of other patch sets, so if you agree, I can pick up your Dove related patches and finish them.

One thing that comes into my mind is, that we moved Dove DT to mach-mvebu starting with v3.15-rc1 so we need to find a better place for the driver than mach-dove.

Create drivers/pmu ?

Hmm, I see no other folder it could sit in. Maybe, yes.

Quoted 3 lines

The cpufreq driver also needs access to registers within the pmu range. Should it be part of pmu.c, or should we export a regmap which cpufreq can use?

Not only cpufreq but also pinctrl as the first 16 mpps can have PMU related functions mapped to them. Syscon should do the trick.

As for DT, I should probably state that I'm far from happy with the DT situation on Dove. When I've tried it, I've encountered problems with the HDMI output - the picture spends more time blanked than displaying. I've no idea what is causing that, I've been through the SI5351 registers, the TDA998x registers, the LCD controller registers, and I can't find any reason for it. Yet, boot the same kernel without DT and it works fine. Boot back using DT, and it's unstable again.

I can't detect any difference on the actual HDMI signals themselves either. The HDMI clock seems stable and of the correct frequency.

There is definitely some difference between booting with DT and booting with legacy stuff that seems to upset HDMI.

For me, the /only/ reliably working system is one where DT is not involved. This means I remain opposed to any solutions which can't be used in a non-DT environment - at least until the HDMI issue can be resolved.

Quoted 11 lines · Russell King - ARM Linux

As for DT, I should probably state that I'm far from happy with the DT situation on Dove. When I've tried it, I've encountered problems with the HDMI output - the picture spends more time blanked than displaying. I've no idea what is causing that, I've been through the SI5351 registers, the TDA998x registers, the LCD controller registers, and I can't find any reason for it. Yet, boot the same kernel without DT and it works fine. Boot back using DT, and it's unstable again.

I can't detect any difference on the actual HDMI signals themselves either. The HDMI clock seems stable and of the correct frequency.

Quoted 7 lines

There is definitely some difference between booting with DT and booting with legacy stuff that seems to upset HDMI.

For me, the /only/ reliably working system is one where DT is not involved. This means I remain opposed to any solutions which can't be used in a non-DT environment - at least until the HDMI issue can be resolved.

Russell,

as said on IRC, I really want this to be sorted out. I prepared a Dove DT HDMI quick-hack and pushed it to

https://github.com/shesselba/linux-dove.git dove-drm-v3.16-rc1

Using your libdrm-armada and xf86-video-armada on Ubuntu raring armhf, I can run Xfce4 on 1920x1080p60 on Dove Cubox without any blanking issues. Using xrandr -s <mode> will fail after 2-3 times but that shouldn't be related to the issues you see.

Please test above branch with kernel config at
https://dl.dropboxusercontent.com/u/59928252/config-dove-v3.16-rc1-hdmi
and report back if issues are still there.

Anyone having a Dove CuBox ready, please also test. Russell has a git branch for the required video lib and drivers.

Quoted 9 lines · Sebastian Hesselbarth

Using your libdrm-armada and xf86-video-armada on Ubuntu raring armhf, I can run Xfce4 on 1920x1080p60 on Dove Cubox without any blanking issues. Using xrandr -s <mode> will fail after 2-3 times but that shouldn't be related to the issues you see.

Please test above branch with kernel config at
https://dl.dropboxusercontent.com/u/59928252/config-dove-v3.16-rc1-hdmi
and report back if issues are still there.

Quoted 3 lines

Anyone having a Dove CuBox ready, please also test. Russell has a git branch for the required video lib and drivers.

Can you point me at those?

Quoted 10 lines · Ezequiel Garcia

Quoted 9 lines · Sebastian Hesselbarth

Using your libdrm-armada and xf86-video-armada on Ubuntu raring armhf, I can run Xfce4 on 1920x1080p60 on Dove Cubox without any blanking issues. Using xrandr -s <mode> will fail after 2-3 times but that shouldn't be related to the issues you see.

Please test above branch with kernel config at
https://dl.dropboxusercontent.com/u/59928252/config-dove-v3.16-rc1-hdmi
and report back if issues are still there.

Quoted 5 lines

Quoted 3 lines

Anyone having a Dove CuBox ready, please also test. Russell has a git branch for the required video lib and drivers.

Can you point me at those?

Beware - the machine is rather aged and slow by todays standards ((c)git is rather heavy weight):

http://ftp.arm.linux.org.uk/cgit/

The xf86-video-armada.git tree is not trivial to build as there's dependencies on the Vivante galcore libraries - and even if you have them, there's then there's non-trivial changes to the kernel-side driver to support dmabuf imports. Welcome to the world of closed source graphics libraries making open source hard... I've been wishing for etnaviv for a while now...

Even though the kernel side is supposed to be GPL, I'm really not happy distributing it or publishing changes as there are a number of files with headers which seem to be GPL-incompatible (which I've eliminated from my tree through updates) but the problem is wonderful git keeps them as history... and in my tree it's a massive 93 patches, against an old version of the code (0.8.0.1998) which I then sort-of updated to the version OLPC were carrying towards the start of 2013.

It's probably best if Sebastian walks you through getting it up and running as my experience is with my kernels and userspace, which has all the necessary hacks in.

xf86-video-armada needs some missing
#include "picturestr.h"
for raring, but whole process has been very straight forward.

Quoted 3 lines · Sebastian Hesselbarth

xf86-video-armada needs some missing
#include "picturestr.h"
for raring, but whole process has been very straight forward.

I'm only aware of this (sorry, copy'n'pasted) which I found when trying to build against ubuntu 14.04. If further patches are needed to make it compatible with more Xorg versions, please ensure that they find their way into my inbox...

Quoted 5 lines · Russell King - ARM Linux

Quoted 3 lines · Sebastian Hesselbarth

xf86-video-armada needs some missing
#include "picturestr.h"
for raring, but whole process has been very straight forward.

Quoted 4 lines

I'm only aware of this (sorry, copy'n'pasted) which I found when trying to build against ubuntu 14.04. If further patches are needed to make it compatible with more Xorg versions, please ensure that they find their way into my inbox...

Your patch also makes the xorg driver build fine on raring. When I encountered the build error, I just tried the first hit on Google
- I didn't investigate it at all.

Even though the kernel side is supposed to be GPL, I'm really not happy distributing it or publishing changes as there are a number of files with headers which seem to be GPL-incompatible (which I've eliminated from my tree through updates) but the problem is wonderful git keeps them as history... and in my tree it's a massive 93 patches, against an old version of the code (0.8.0.1998) which I then sort-of updated to the version OLPC were carrying towards the start of 2013.

as said on IRC, I really want this to be sorted out. I prepared a Dove DT HDMI quick-hack and pushed it to

https://github.com/shesselba/linux-dove.git dove-drm-v3.16-rc1