* Re: [PATCH] cpuidle/powernv : Add Description for cpuidle state
From: Stewart Smith @ 2018-05-29 1:39 UTC (permalink / raw)
To: rjw, daniel.lezcano, benh, paulus, mpe, linux-pm, linuxppc-dev,
linux-kernel
Cc: Abhishek Goel
In-Reply-To: <20180528173442.100642-1-huntbag@linux.vnet.ibm.com>
Abhishek Goel <huntbag@linux.vnet.ibm.com> writes:
> @@ -215,7 +216,7 @@ static inline void add_powernv_state(int index, const char *name,
> u64 psscr_val, u64 psscr_mask)
> {
> strlcpy(powernv_states[index].name, name, CPUIDLE_NAME_LEN);
> - strlcpy(powernv_states[index].desc, name, CPUIDLE_NAME_LEN);
> + strlcpy(powernv_states[index].desc, desc, CPUIDLE_DESC_LEN);
We should still fall back to using name in the event of desc being null,
as not all firmware will expose the descriptions.
> @@ -311,6 +313,11 @@ static int powernv_add_idle_states(void)
> pr_warn("cpuidle-powernv: missing ibm,cpu-idle-state-names in DT\n");
> goto out;
> }
> + if (of_property_read_string_array(power_mgt,
> + "ibm,cpu-idle-state-descs", descs, dt_idle_states) < 0) {
> + pr_warn("cpuidle-powernv: missing ibm,cpu-idle-state-descs in DT\n");
> + goto out;
> + }
I don't think pr_warn is appropriate here, as for all current released
firmware we don't have that property. I think perhaps just silently
continuing on is okay, as we have to keep compatibility with that firmware.
> --- a/include/linux/cpuidle.h
> +++ b/include/linux/cpuidle.h
> @@ -17,7 +17,7 @@
>
> #define CPUIDLE_STATE_MAX 10
> #define CPUIDLE_NAME_LEN 16
> -#define CPUIDLE_DESC_LEN 32
> +#define CPUIDLE_DESC_LEN 60
Do we really get that long?
--
Stewart Smith
OPAL Architect, IBM.
^ permalink raw reply
* Re: [PATCH v3] vfio: platform: Fix using devices in PM Domains
From: Geert Uytterhoeven @ 2018-05-28 22:14 UTC (permalink / raw)
To: Auger Eric
Cc: Geert Uytterhoeven, Alex Williamson, KVM list, Linux-Renesas,
Linux PM list, Linux Kernel Mailing List
In-Reply-To: <ff3992c7-1d59-1fec-1393-47e2d278d8c9@redhat.com>
Hi Eric,
On Mon, May 28, 2018 at 10:27 PM, Auger Eric <eric.auger@redhat.com> wrote:
> On 05/28/2018 05:26 PM, Geert Uytterhoeven wrote:
>> If a device is part of a PM Domain (e.g. power and/or clock domain), its
>> power state is managed using Runtime PM. Without Runtime PM, the device
>> may not be powered up or clocked, causing subtle failures, crashes, or
>> system lock-ups when the device is accessed by the guest.
>>
>> Fix this by adding Runtime PM support, powering the device when the VFIO
>> device is opened by the guest.
>>
>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>> Reviewed-by: Simon Horman <horms+renesas@verge.net.au>
>> ---
>> v3:
>> - Drop controversial note about unsafeness of exporting fine-grained
>> power management from host to guest,
> thanks ;-)
>
>>
>> v2:
>> - Improve wording,
>> - Add Reviewed-by.
>> ---
>> drivers/vfio/platform/vfio_platform_common.c | 9 +++++++++
>> 1 file changed, 9 insertions(+)
>>
>> diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
>> index b60bb5326668498c..41f862f055054543 100644
>> --- a/drivers/vfio/platform/vfio_platform_common.c
>> +++ b/drivers/vfio/platform/vfio_platform_common.c
>> @@ -17,6 +17,7 @@
>> #include <linux/iommu.h>
>> #include <linux/module.h>
>> #include <linux/mutex.h>
>> +#include <linux/pm_runtime.h>
>> #include <linux/slab.h>
>> #include <linux/types.h>
>> #include <linux/uaccess.h>
>> @@ -233,6 +234,8 @@ static void vfio_platform_release(void *device_data)
>> const char *extra_dbg = NULL;
>> int ret;
>>
>> + pm_runtime_put(vdev->device);
>
>> +
>> ret = vfio_platform_call_reset(vdev, &extra_dbg);
> While reading the code again, the reset is using the HW. Don't you want
> to call the get() before.
Thanks, you're right, it's safer to do the reset here first...
>> if (ret && vdev->reset_required) {
>> dev_warn(vdev->device, "reset driver is required and reset call failed in release (%d) %s\n",
>> @@ -275,6 +278,10 @@ static int vfio_platform_open(void *device_data)
>> ret, extra_dbg ? extra_dbg : "");
>> goto err_rst;
>> }
>> +
>> + ret = pm_runtime_get_sync(vdev->device);
... and to do the reset here last.
Will fix and send v4.
>> + if (ret < 0)
>> + goto err_rst;
>> }
>>
>> vdev->refcnt++;
>> @@ -690,6 +697,7 @@ int vfio_platform_probe_common(struct vfio_platform_device *vdev,
>>
>> mutex_init(&vdev->igate);
>>
>> + pm_runtime_enable(vdev->device);
>> return 0;
>>
>> put_iommu:
>> @@ -707,6 +715,7 @@ struct vfio_platform_device *vfio_platform_remove_common(struct device *dev)
>> vdev = vfio_del_group_dev(dev);
>>
>> if (vdev) {
>> + pm_runtime_disable(vdev->device);
>> vfio_platform_put_reset(vdev);
>> vfio_iommu_group_put(dev->iommu_group, dev);
>> }
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* Re: [PATCH v3] vfio: platform: Fix using devices in PM Domains
From: Auger Eric @ 2018-05-28 20:27 UTC (permalink / raw)
To: Geert Uytterhoeven, Alex Williamson
Cc: kvm, linux-renesas-soc, linux-pm, linux-kernel
In-Reply-To: <1527521165-29345-1-git-send-email-geert+renesas@glider.be>
Hi Geert,
On 05/28/2018 05:26 PM, Geert Uytterhoeven wrote:
> If a device is part of a PM Domain (e.g. power and/or clock domain), its
> power state is managed using Runtime PM. Without Runtime PM, the device
> may not be powered up or clocked, causing subtle failures, crashes, or
> system lock-ups when the device is accessed by the guest.
>
> Fix this by adding Runtime PM support, powering the device when the VFIO
> device is opened by the guest.
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Reviewed-by: Simon Horman <horms+renesas@verge.net.au>
> ---
> v3:
> - Drop controversial note about unsafeness of exporting fine-grained
> power management from host to guest,
thanks ;-)
>
> v2:
> - Improve wording,
> - Add Reviewed-by.
> ---
> drivers/vfio/platform/vfio_platform_common.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
> index b60bb5326668498c..41f862f055054543 100644
> --- a/drivers/vfio/platform/vfio_platform_common.c
> +++ b/drivers/vfio/platform/vfio_platform_common.c
> @@ -17,6 +17,7 @@
> #include <linux/iommu.h>
> #include <linux/module.h>
> #include <linux/mutex.h>
> +#include <linux/pm_runtime.h>
> #include <linux/slab.h>
> #include <linux/types.h>
> #include <linux/uaccess.h>
> @@ -233,6 +234,8 @@ static void vfio_platform_release(void *device_data)
> const char *extra_dbg = NULL;
> int ret;
>
> + pm_runtime_put(vdev->device);
> +
> ret = vfio_platform_call_reset(vdev, &extra_dbg);
While reading the code again, the reset is using the HW. Don't you want
to call the get() before.
Thanks
Eric
> if (ret && vdev->reset_required) {
> dev_warn(vdev->device, "reset driver is required and reset call failed in release (%d) %s\n",
> @@ -275,6 +278,10 @@ static int vfio_platform_open(void *device_data)
> ret, extra_dbg ? extra_dbg : "");
> goto err_rst;
> }
> +
> + ret = pm_runtime_get_sync(vdev->device);
> + if (ret < 0)
> + goto err_rst;
> }
>
> vdev->refcnt++;
> @@ -690,6 +697,7 @@ int vfio_platform_probe_common(struct vfio_platform_device *vdev,
>
> mutex_init(&vdev->igate);
>
> + pm_runtime_enable(vdev->device);
> return 0;
>
> put_iommu:
> @@ -707,6 +715,7 @@ struct vfio_platform_device *vfio_platform_remove_common(struct device *dev)
> vdev = vfio_del_group_dev(dev);
>
> if (vdev) {
> + pm_runtime_disable(vdev->device);
> vfio_platform_put_reset(vdev);
> vfio_iommu_group_put(dev->iommu_group, dev);
> }
>
^ permalink raw reply
* [PATCH] cpuidle/powernv : Add Description for cpuidle state
From: Abhishek Goel @ 2018-05-28 17:34 UTC (permalink / raw)
To: rjw, daniel.lezcano, benh, paulus, mpe, linux-pm, linuxppc-dev,
linux-kernel
Cc: Abhishek Goel
Names of cpuidle states were being used for description of states
in POWER as no descriptions were added in device tree. This patch
reads description for idle states which have been added in device
tree.
The description for idle states in case of POWER can be printed
using "cpupower monitor -l" or "cpupower idle-info".
Signed-off-by: Abhishek Goel <huntbag@linux.vnet.ibm.com>
---
The skiboot patch which adds description for idle states in device tree
can be found here: https://patchwork.ozlabs.org/patch/921637/
drivers/cpuidle/cpuidle-powernv.c | 17 +++++++++++++----
include/linux/cpuidle.h | 2 +-
2 files changed, 14 insertions(+), 5 deletions(-)
diff --git a/drivers/cpuidle/cpuidle-powernv.c b/drivers/cpuidle/cpuidle-powernv.c
index 1a8234e706bc..d3915a965128 100644
--- a/drivers/cpuidle/cpuidle-powernv.c
+++ b/drivers/cpuidle/cpuidle-powernv.c
@@ -133,7 +133,7 @@ static int stop_loop(struct cpuidle_device *dev,
static struct cpuidle_state powernv_states[CPUIDLE_STATE_MAX] = {
{ /* Snooze */
.name = "snooze",
- .desc = "snooze",
+ .desc = "idle polling state",
.exit_latency = 0,
.target_residency = 0,
.enter = snooze_loop },
@@ -206,6 +206,7 @@ static int powernv_cpuidle_driver_init(void)
}
static inline void add_powernv_state(int index, const char *name,
+ const char *desc,
unsigned int flags,
int (*idle_fn)(struct cpuidle_device *,
struct cpuidle_driver *,
@@ -215,7 +216,7 @@ static inline void add_powernv_state(int index, const char *name,
u64 psscr_val, u64 psscr_mask)
{
strlcpy(powernv_states[index].name, name, CPUIDLE_NAME_LEN);
- strlcpy(powernv_states[index].desc, name, CPUIDLE_NAME_LEN);
+ strlcpy(powernv_states[index].desc, desc, CPUIDLE_DESC_LEN);
powernv_states[index].flags = flags;
powernv_states[index].target_residency = target_residency;
powernv_states[index].exit_latency = exit_latency;
@@ -250,6 +251,7 @@ static int powernv_add_idle_states(void)
u64 psscr_val[CPUIDLE_STATE_MAX];
u64 psscr_mask[CPUIDLE_STATE_MAX];
const char *names[CPUIDLE_STATE_MAX];
+ const char *descs[CPUIDLE_STATE_MAX];
u32 has_stop_states = 0;
int i, rc;
u32 supported_flags = pnv_get_supported_cpuidle_states();
@@ -311,6 +313,11 @@ static int powernv_add_idle_states(void)
pr_warn("cpuidle-powernv: missing ibm,cpu-idle-state-names in DT\n");
goto out;
}
+ if (of_property_read_string_array(power_mgt,
+ "ibm,cpu-idle-state-descs", descs, dt_idle_states) < 0) {
+ pr_warn("cpuidle-powernv: missing ibm,cpu-idle-state-descs in DT\n");
+ goto out;
+ }
/*
* If the idle states use stop instruction, probe for psscr values
@@ -414,10 +421,11 @@ static int powernv_add_idle_states(void)
target_residency = 100;
/* Add NAP state */
add_powernv_state(nr_idle_states, "Nap",
+ "stop processor execution",
CPUIDLE_FLAG_NONE, nap_loop,
target_residency, exit_latency, 0, 0);
} else if (has_stop_states && !stops_timebase) {
- add_powernv_state(nr_idle_states, names[i],
+ add_powernv_state(nr_idle_states, names[i], descs[i],
CPUIDLE_FLAG_NONE, stop_loop,
target_residency, exit_latency,
psscr_val[i], psscr_mask[i]);
@@ -434,11 +442,12 @@ static int powernv_add_idle_states(void)
target_residency = 300000;
/* Add FASTSLEEP state */
add_powernv_state(nr_idle_states, "FastSleep",
+ "Core and L2 clock gating",
CPUIDLE_FLAG_TIMER_STOP,
fastsleep_loop,
target_residency, exit_latency, 0, 0);
} else if (has_stop_states && stops_timebase) {
- add_powernv_state(nr_idle_states, names[i],
+ add_powernv_state(nr_idle_states, names[i], descs[i],
CPUIDLE_FLAG_TIMER_STOP, stop_loop,
target_residency, exit_latency,
psscr_val[i], psscr_mask[i]);
diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
index 1eefabf1621f..5094755cb132 100644
--- a/include/linux/cpuidle.h
+++ b/include/linux/cpuidle.h
@@ -17,7 +17,7 @@
#define CPUIDLE_STATE_MAX 10
#define CPUIDLE_NAME_LEN 16
-#define CPUIDLE_DESC_LEN 32
+#define CPUIDLE_DESC_LEN 60
struct module;
--
2.14.1
^ permalink raw reply related
* [PATCH v3] vfio: platform: Fix using devices in PM Domains
From: Geert Uytterhoeven @ 2018-05-28 15:26 UTC (permalink / raw)
To: Eric Auger, Alex Williamson
Cc: kvm, linux-renesas-soc, linux-pm, linux-kernel,
Geert Uytterhoeven
If a device is part of a PM Domain (e.g. power and/or clock domain), its
power state is managed using Runtime PM. Without Runtime PM, the device
may not be powered up or clocked, causing subtle failures, crashes, or
system lock-ups when the device is accessed by the guest.
Fix this by adding Runtime PM support, powering the device when the VFIO
device is opened by the guest.
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
Reviewed-by: Simon Horman <horms+renesas@verge.net.au>
---
v3:
- Drop controversial note about unsafeness of exporting fine-grained
power management from host to guest,
v2:
- Improve wording,
- Add Reviewed-by.
---
drivers/vfio/platform/vfio_platform_common.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
index b60bb5326668498c..41f862f055054543 100644
--- a/drivers/vfio/platform/vfio_platform_common.c
+++ b/drivers/vfio/platform/vfio_platform_common.c
@@ -17,6 +17,7 @@
#include <linux/iommu.h>
#include <linux/module.h>
#include <linux/mutex.h>
+#include <linux/pm_runtime.h>
#include <linux/slab.h>
#include <linux/types.h>
#include <linux/uaccess.h>
@@ -233,6 +234,8 @@ static void vfio_platform_release(void *device_data)
const char *extra_dbg = NULL;
int ret;
+ pm_runtime_put(vdev->device);
+
ret = vfio_platform_call_reset(vdev, &extra_dbg);
if (ret && vdev->reset_required) {
dev_warn(vdev->device, "reset driver is required and reset call failed in release (%d) %s\n",
@@ -275,6 +278,10 @@ static int vfio_platform_open(void *device_data)
ret, extra_dbg ? extra_dbg : "");
goto err_rst;
}
+
+ ret = pm_runtime_get_sync(vdev->device);
+ if (ret < 0)
+ goto err_rst;
}
vdev->refcnt++;
@@ -690,6 +697,7 @@ int vfio_platform_probe_common(struct vfio_platform_device *vdev,
mutex_init(&vdev->igate);
+ pm_runtime_enable(vdev->device);
return 0;
put_iommu:
@@ -707,6 +715,7 @@ struct vfio_platform_device *vfio_platform_remove_common(struct device *dev)
vdev = vfio_del_group_dev(dev);
if (vdev) {
+ pm_runtime_disable(vdev->device);
vfio_platform_put_reset(vdev);
vfio_iommu_group_put(dev->iommu_group, dev);
}
--
2.7.4
^ permalink raw reply related
* Re: [PATCH v2] vfio: platform: Fix using devices in PM Domains
From: Geert Uytterhoeven @ 2018-05-28 15:02 UTC (permalink / raw)
To: Auger Eric
Cc: Geert Uytterhoeven, Alex Williamson, KVM list, Linux-Renesas,
Linux PM list, Linux Kernel Mailing List
In-Reply-To: <10a503da-d431-f2c9-3ddc-91b065563a0d@redhat.com>
Hi Eric,
On Mon, May 28, 2018 at 4:54 PM, Auger Eric <eric.auger@redhat.com> wrote:
> On 05/18/2018 12:55 PM, Geert Uytterhoeven wrote:
>> If a device is part of a PM Domain (e.g. power and/or clock domain), its
>> power state is managed using Runtime PM. Without Runtime PM, the device
>> may not be powered up or clocked, causing subtle failures, crashes, or
>> system lock-ups when the device is accessed by the guest.
>>
>> Fix this by adding Runtime PM support, powering the device when the VFIO
>> device is opened by the guest.
>
> I fail to apply this on master branch?
It's against git://github.com/awilliam/linux-vfio.git#next.
>> Note that while more fine-grained power management could be implemented
>> on the guest side, if exported by the host, this would be inherently
>> unsafe, as abusing it may take down the whole system.
> I think I would prefer we get rid of the above paragraph.
OK, I will drop it. But you've been warned ;-)
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* Re: [PATCH v2] vfio: platform: Fix using devices in PM Domains
From: Auger Eric @ 2018-05-28 14:54 UTC (permalink / raw)
To: Geert Uytterhoeven, Alex Williamson
Cc: kvm, linux-renesas-soc, linux-pm, linux-kernel
In-Reply-To: <1526640912-894-1-git-send-email-geert+renesas@glider.be>
Hi Geert,
On 05/18/2018 12:55 PM, Geert Uytterhoeven wrote:
> If a device is part of a PM Domain (e.g. power and/or clock domain), its
> power state is managed using Runtime PM. Without Runtime PM, the device
> may not be powered up or clocked, causing subtle failures, crashes, or
> system lock-ups when the device is accessed by the guest.
>
> Fix this by adding Runtime PM support, powering the device when the VFIO
> device is opened by the guest.
I fail to apply this on master branch?
>
> Note that while more fine-grained power management could be implemented
> on the guest side, if exported by the host, this would be inherently
> unsafe, as abusing it may take down the whole system.
I think I would prefer we get rid of the above paragraph.
Thanks
Eric
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Reviewed-by: Simon Horman <horms+renesas@verge.net.au>
> ---
> v2:
> - Improve wording,
> - Add Reviewed-by.
> ---
> drivers/vfio/platform/vfio_platform_common.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
> index b60bb5326668498c..41f862f055054543 100644
> --- a/drivers/vfio/platform/vfio_platform_common.c
> +++ b/drivers/vfio/platform/vfio_platform_common.c
> @@ -17,6 +17,7 @@
> #include <linux/iommu.h>
> #include <linux/module.h>
> #include <linux/mutex.h>
> +#include <linux/pm_runtime.h>
> #include <linux/slab.h>
> #include <linux/types.h>
> #include <linux/uaccess.h>
> @@ -233,6 +234,8 @@ static void vfio_platform_release(void *device_data)
> const char *extra_dbg = NULL;
> int ret;
>
> + pm_runtime_put(vdev->device);
> +
> ret = vfio_platform_call_reset(vdev, &extra_dbg);
> if (ret && vdev->reset_required) {
> dev_warn(vdev->device, "reset driver is required and reset call failed in release (%d) %s\n",
> @@ -275,6 +278,10 @@ static int vfio_platform_open(void *device_data)
> ret, extra_dbg ? extra_dbg : "");
> goto err_rst;
> }
> +
> + ret = pm_runtime_get_sync(vdev->device);
> + if (ret < 0)
> + goto err_rst;
> }
>
> vdev->refcnt++;
> @@ -690,6 +697,7 @@ int vfio_platform_probe_common(struct vfio_platform_device *vdev,
>
> mutex_init(&vdev->igate);
>
> + pm_runtime_enable(vdev->device);
> return 0;
>
> put_iommu:
> @@ -707,6 +715,7 @@ struct vfio_platform_device *vfio_platform_remove_common(struct device *dev)
> vdev = vfio_del_group_dev(dev);
>
> if (vdev) {
> + pm_runtime_disable(vdev->device);
> vfio_platform_put_reset(vdev);
> vfio_iommu_group_put(dev->iommu_group, dev);
> }
>
^ permalink raw reply
* [PATCH] cpupower : Fix header name to read idle state name
From: Abhishek Goel @ 2018-05-28 11:03 UTC (permalink / raw)
To: shuah, trenn, linux-pm, linux-kernel; +Cc: s1seetee, Abhishek Goel
The names of the idle states in the output of cpupower monitor command are
truncated to 4 characters. On POWER9, this creates ambiguity as the states
are named "stop0", "stop1", etc.
root:~# cpupower monitor
|Idle_Stats
PKG |CORE|CPU | snoo | stop | stop | stop | stop | stop | stop
0| 0| 0| 0.00| 0.00| 0.00| 0.00| 0.00| 0.00| 1.90
0| 0| 1| 0.00| 0.00| 0.00| 0.00| 0.00| 0.00| 0.00
0| 0| 2| 0.00| 0.00| 0.00| 0.00| 0.00| 0.00| 0.00
0| 0| 3| 0.00| 0.00| 0.00| 0.00| 0.00| 0.00| 0.00
This patch modifies the output to print the state name that results in a
legible output. The names will be printed with atmost 1 padding in left.
root:~# cpupower monitor
| Idle_Stats
PKG|CORE| CPU|snooze|stop0L| stop0|stop1L| stop1|stop2L| stop2
0| 0| 0| 0.00| 0.00| 0.00| 0.00| 0.00| 0.00| 0.72
0| 0| 1| 0.00| 0.00| 0.00| 0.00| 0.00| 0.00| 0.00
0| 0| 2| 0.00| 0.00| 0.00| 0.00| 0.00| 0.00| 0.00
0| 0| 3| 0.00| 0.00| 0.00| 0.00| 0.00| 0.00| 0.00
This patch does not affect the output for intel.
Output for intel before applying the patch:
root:~# cpupower monitor
|Idle_Stats
CPU | POLL | C1-S | C1E- | C3-S | C6-S | C7s- | C8-S | C9-S | C10-
0| 0.00| 0.14| 0.39| 0.35| 7.41| 0.00| 17.67| 1.01| 70.03
2| 0.00| 0.19| 0.47| 0.10| 6.50| 0.00| 29.66| 2.17| 58.07
1| 0.00| 0.11| 0.50| 1.50| 9.11| 0.18| 18.19| 0.40| 66.63
3| 0.00| 0.67| 0.42| 0.03| 5.84| 0.00| 12.58| 0.77| 77.14
Output for intel after applying the patch:
root:~# cpupower monitor
| Idle_Stats
CPU| POLL | C1-S | C1E- | C3-S | C6-S | C7s- | C8-S | C9-S | C10-
0| 0.03| 0.33| 1.01| 0.27| 3.03| 0.00| 19.18| 0.00| 71.24
2| 0.00| 1.58| 0.58| 0.42| 8.55| 0.09| 21.11| 0.99| 63.32
1| 0.00| 1.26| 0.88| 0.43| 9.00| 0.02| 7.78| 4.65| 71.91
3| 0.00| 0.30| 0.42| 0.06| 13.62| 0.21| 30.29| 0.00| 52.45
Signed-off-by: Abhishek Goel <huntbag@linux.vnet.ibm.com>
---
Previous attempt was made by Seeteena Thoufeek. This is the patch list
link: https://patchwork.kernel.org/project/linux-pm/list/?submitter=
172041&state=*&archive=both
.../utils/idle_monitor/cpuidle_sysfs.c | 15 ++++++++
.../utils/idle_monitor/cpupower-monitor.c | 35 +++++++++++--------
.../utils/idle_monitor/cpupower-monitor.h | 9 +++++
3 files changed, 44 insertions(+), 15 deletions(-)
diff --git a/tools/power/cpupower/utils/idle_monitor/cpuidle_sysfs.c b/tools/power/cpupower/utils/idle_monitor/cpuidle_sysfs.c
index 5b3205f16217..5b8c4956ff9a 100644
--- a/tools/power/cpupower/utils/idle_monitor/cpuidle_sysfs.c
+++ b/tools/power/cpupower/utils/idle_monitor/cpuidle_sysfs.c
@@ -126,6 +126,20 @@ void fix_up_intel_idle_driver_name(char *tmp, int num)
}
}
+#ifdef __powerpc__
+void map_power_idle_state_name(char *tmp)
+{
+ if (!strncmp(tmp, "stop0_lite", CSTATE_NAME_LEN))
+ strcpy(tmp, "stop0L");
+ else if (!strncmp(tmp, "stop1_lite", CSTATE_NAME_LEN))
+ strcpy(tmp, "stop1L");
+ else if (!strncmp(tmp, "stop2_lite", CSTATE_NAME_LEN))
+ strcpy(tmp, "stop2L");
+}
+#else
+void map_power_idle_state_name(char *tmp) { }
+#endif
+
static struct cpuidle_monitor *cpuidle_register(void)
{
int num;
@@ -145,6 +159,7 @@ static struct cpuidle_monitor *cpuidle_register(void)
if (tmp == NULL)
continue;
+ map_power_idle_state_name(tmp);
fix_up_intel_idle_driver_name(tmp, num);
strncpy(cpuidle_cstates[num].name, tmp, CSTATE_NAME_LEN - 1);
free(tmp);
diff --git a/tools/power/cpupower/utils/idle_monitor/cpupower-monitor.c b/tools/power/cpupower/utils/idle_monitor/cpupower-monitor.c
index 05f953f0f0a0..051da0a7c454 100644
--- a/tools/power/cpupower/utils/idle_monitor/cpupower-monitor.c
+++ b/tools/power/cpupower/utils/idle_monitor/cpupower-monitor.c
@@ -70,36 +70,43 @@ void print_n_spaces(int n)
printf(" ");
}
-/* size of s must be at least n + 1 */
+/*s is filled with left and right spaces
+ *to make its length atleast n+1
+ */
int fill_string_with_spaces(char *s, int n)
{
+ char *temp;
int len = strlen(s);
- if (len > n)
+
+ if (len >= n)
return -1;
+
+ temp = malloc(sizeof(char) * (n+1));
for (; len < n; len++)
s[len] = ' ';
s[len] = '\0';
+ snprintf(temp, n+1, " %s", s);
+ strcpy(s, temp);
+ free(temp);
return 0;
}
+#define MAX_COL_WIDTH 6
void print_header(int topology_depth)
{
int unsigned mon;
int state, need_len;
cstate_t s;
char buf[128] = "";
- int percent_width = 4;
fill_string_with_spaces(buf, topology_depth * 5 - 1);
printf("%s|", buf);
for (mon = 0; mon < avail_monitors; mon++) {
- need_len = monitors[mon]->hw_states_num * (percent_width + 3)
+ need_len = monitors[mon]->hw_states_num * (MAX_COL_WIDTH + 1)
- 1;
- if (mon != 0) {
- printf("|| ");
- need_len--;
- }
+ if (mon != 0)
+ printf("||");
sprintf(buf, "%s", monitors[mon]->name);
fill_string_with_spaces(buf, need_len);
printf("%s", buf);
@@ -107,23 +114,21 @@ void print_header(int topology_depth)
printf("\n");
if (topology_depth > 2)
- printf("PKG |");
+ printf(" PKG|");
if (topology_depth > 1)
printf("CORE|");
if (topology_depth > 0)
- printf("CPU |");
+ printf(" CPU|");
for (mon = 0; mon < avail_monitors; mon++) {
if (mon != 0)
- printf("|| ");
- else
- printf(" ");
+ printf("||");
for (state = 0; state < monitors[mon]->hw_states_num; state++) {
if (state != 0)
- printf(" | ");
+ printf("|");
s = monitors[mon]->hw_states[state];
sprintf(buf, "%s", s.name);
- fill_string_with_spaces(buf, percent_width);
+ fill_string_with_spaces(buf, MAX_COL_WIDTH);
printf("%s", buf);
}
printf(" ");
diff --git a/tools/power/cpupower/utils/idle_monitor/cpupower-monitor.h b/tools/power/cpupower/utils/idle_monitor/cpupower-monitor.h
index 9e43f3371fbc..2ae50b499e0a 100644
--- a/tools/power/cpupower/utils/idle_monitor/cpupower-monitor.h
+++ b/tools/power/cpupower/utils/idle_monitor/cpupower-monitor.h
@@ -15,7 +15,16 @@
#define MONITORS_MAX 20
#define MONITOR_NAME_LEN 20
+
+/* CSTATE_NAME_LEN is limited by header field width defined
+ * in cpupower-monitor.c. Header field width is defined to be
+ * sum of percent width and two spaces for padding.
+ */
+#ifdef __powerpc__
+#define CSTATE_NAME_LEN 7
+#else
#define CSTATE_NAME_LEN 5
+#endif
#define CSTATE_DESC_LEN 60
int cpu_count;
--
2.17.0
^ permalink raw reply related
* [PATCH 3.18 034/185] x86/power: Fix swsusp_arch_resume prototype
From: Greg Kroah-Hartman @ 2018-05-28 10:01 UTC (permalink / raw)
To: linux-kernel
Cc: Greg Kroah-Hartman, stable, Arnd Bergmann, Thomas Gleixner,
Len Brown, Andi Kleen, Nicolas Pitre, linux-pm, Rafael J. Wysocki,
Pavel Machek, Bart Van Assche, Sasha Levin
In-Reply-To: <20180528100050.700971285@linuxfoundation.org>
3.18-stable review patch. If anyone has any objections, please let me know.
------------------
From: Arnd Bergmann <arnd@arndb.de>
[ Upstream commit 328008a72d38b5bde6491e463405c34a81a65d3e ]
The declaration for swsusp_arch_resume marks it as 'asmlinkage', but the
definition in x86-32 does not, and it fails to include the header with the
declaration. This leads to a warning when building with
link-time-optimizations:
kernel/power/power.h:108:23: error: type of 'swsusp_arch_resume' does not match original declaration [-Werror=lto-type-mismatch]
extern asmlinkage int swsusp_arch_resume(void);
^
arch/x86/power/hibernate_32.c:148:0: note: 'swsusp_arch_resume' was previously declared here
int swsusp_arch_resume(void)
This moves the declaration into a globally visible header file and fixes up
both x86 definitions to match it.
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Len Brown <len.brown@intel.com>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: Nicolas Pitre <nico@linaro.org>
Cc: linux-pm@vger.kernel.org
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: Bart Van Assche <bart.vanassche@wdc.com>
Link: https://lkml.kernel.org/r/20180202145634.200291-2-arnd@arndb.de
Signed-off-by: Sasha Levin <alexander.levin@microsoft.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
arch/x86/power/hibernate_32.c | 2 +-
arch/x86/power/hibernate_64.c | 2 +-
include/linux/suspend.h | 2 ++
kernel/power/power.h | 3 ---
4 files changed, 4 insertions(+), 5 deletions(-)
--- a/arch/x86/power/hibernate_32.c
+++ b/arch/x86/power/hibernate_32.c
@@ -142,7 +142,7 @@ static inline void resume_init_first_lev
#endif
}
-int swsusp_arch_resume(void)
+asmlinkage int swsusp_arch_resume(void)
{
int error;
--- a/arch/x86/power/hibernate_64.c
+++ b/arch/x86/power/hibernate_64.c
@@ -78,7 +78,7 @@ static int set_up_temporary_mappings(voi
return 0;
}
-int swsusp_arch_resume(void)
+asmlinkage int swsusp_arch_resume(void)
{
int error;
--- a/include/linux/suspend.h
+++ b/include/linux/suspend.h
@@ -325,6 +325,8 @@ extern int swsusp_page_is_forbidden(stru
extern void swsusp_set_page_free(struct page *);
extern void swsusp_unset_page_free(struct page *);
extern unsigned long get_safe_page(gfp_t gfp_mask);
+extern asmlinkage int swsusp_arch_suspend(void);
+extern asmlinkage int swsusp_arch_resume(void);
extern void hibernation_set_ops(const struct platform_hibernation_ops *ops);
extern int hibernate(void);
--- a/kernel/power/power.h
+++ b/kernel/power/power.h
@@ -85,9 +85,6 @@ extern int in_suspend;
extern dev_t swsusp_resume_device;
extern sector_t swsusp_resume_block;
-extern asmlinkage int swsusp_arch_suspend(void);
-extern asmlinkage int swsusp_arch_resume(void);
-
extern int create_basic_memory_bitmaps(void);
extern void free_basic_memory_bitmaps(void);
extern int hibernate_preallocate_memory(void);
^ permalink raw reply
* [PATCH 4.4 054/268] x86/power: Fix swsusp_arch_resume prototype
From: Greg Kroah-Hartman @ 2018-05-28 10:00 UTC (permalink / raw)
To: linux-kernel
Cc: Greg Kroah-Hartman, stable, Arnd Bergmann, Thomas Gleixner,
Len Brown, Andi Kleen, Nicolas Pitre, linux-pm, Rafael J. Wysocki,
Pavel Machek, Bart Van Assche, Sasha Levin
In-Reply-To: <20180528100202.045206534@linuxfoundation.org>
4.4-stable review patch. If anyone has any objections, please let me know.
------------------
From: Arnd Bergmann <arnd@arndb.de>
[ Upstream commit 328008a72d38b5bde6491e463405c34a81a65d3e ]
The declaration for swsusp_arch_resume marks it as 'asmlinkage', but the
definition in x86-32 does not, and it fails to include the header with the
declaration. This leads to a warning when building with
link-time-optimizations:
kernel/power/power.h:108:23: error: type of 'swsusp_arch_resume' does not match original declaration [-Werror=lto-type-mismatch]
extern asmlinkage int swsusp_arch_resume(void);
^
arch/x86/power/hibernate_32.c:148:0: note: 'swsusp_arch_resume' was previously declared here
int swsusp_arch_resume(void)
This moves the declaration into a globally visible header file and fixes up
both x86 definitions to match it.
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Len Brown <len.brown@intel.com>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: Nicolas Pitre <nico@linaro.org>
Cc: linux-pm@vger.kernel.org
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: Bart Van Assche <bart.vanassche@wdc.com>
Link: https://lkml.kernel.org/r/20180202145634.200291-2-arnd@arndb.de
Signed-off-by: Sasha Levin <alexander.levin@microsoft.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
arch/x86/power/hibernate_32.c | 2 +-
arch/x86/power/hibernate_64.c | 2 +-
include/linux/suspend.h | 2 ++
kernel/power/power.h | 3 ---
4 files changed, 4 insertions(+), 5 deletions(-)
--- a/arch/x86/power/hibernate_32.c
+++ b/arch/x86/power/hibernate_32.c
@@ -142,7 +142,7 @@ static inline void resume_init_first_lev
#endif
}
-int swsusp_arch_resume(void)
+asmlinkage int swsusp_arch_resume(void)
{
int error;
--- a/arch/x86/power/hibernate_64.c
+++ b/arch/x86/power/hibernate_64.c
@@ -78,7 +78,7 @@ static int set_up_temporary_mappings(voi
return 0;
}
-int swsusp_arch_resume(void)
+asmlinkage int swsusp_arch_resume(void)
{
int error;
--- a/include/linux/suspend.h
+++ b/include/linux/suspend.h
@@ -377,6 +377,8 @@ extern int swsusp_page_is_forbidden(stru
extern void swsusp_set_page_free(struct page *);
extern void swsusp_unset_page_free(struct page *);
extern unsigned long get_safe_page(gfp_t gfp_mask);
+extern asmlinkage int swsusp_arch_suspend(void);
+extern asmlinkage int swsusp_arch_resume(void);
extern void hibernation_set_ops(const struct platform_hibernation_ops *ops);
extern int hibernate(void);
--- a/kernel/power/power.h
+++ b/kernel/power/power.h
@@ -85,9 +85,6 @@ extern int in_suspend;
extern dev_t swsusp_resume_device;
extern sector_t swsusp_resume_block;
-extern asmlinkage int swsusp_arch_suspend(void);
-extern asmlinkage int swsusp_arch_resume(void);
-
extern int create_basic_memory_bitmaps(void);
extern void free_basic_memory_bitmaps(void);
extern int hibernate_preallocate_memory(void);
^ permalink raw reply
* [PATCH 4.9 079/329] x86/power: Fix swsusp_arch_resume prototype
From: Greg Kroah-Hartman @ 2018-05-28 10:00 UTC (permalink / raw)
To: linux-kernel
Cc: Greg Kroah-Hartman, stable, Arnd Bergmann, Thomas Gleixner,
Len Brown, Andi Kleen, Nicolas Pitre, linux-pm, Rafael J. Wysocki,
Pavel Machek, Bart Van Assche, Sasha Levin
In-Reply-To: <20180528100241.796630982@linuxfoundation.org>
4.9-stable review patch. If anyone has any objections, please let me know.
------------------
From: Arnd Bergmann <arnd@arndb.de>
[ Upstream commit 328008a72d38b5bde6491e463405c34a81a65d3e ]
The declaration for swsusp_arch_resume marks it as 'asmlinkage', but the
definition in x86-32 does not, and it fails to include the header with the
declaration. This leads to a warning when building with
link-time-optimizations:
kernel/power/power.h:108:23: error: type of 'swsusp_arch_resume' does not match original declaration [-Werror=lto-type-mismatch]
extern asmlinkage int swsusp_arch_resume(void);
^
arch/x86/power/hibernate_32.c:148:0: note: 'swsusp_arch_resume' was previously declared here
int swsusp_arch_resume(void)
This moves the declaration into a globally visible header file and fixes up
both x86 definitions to match it.
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Len Brown <len.brown@intel.com>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: Nicolas Pitre <nico@linaro.org>
Cc: linux-pm@vger.kernel.org
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: Bart Van Assche <bart.vanassche@wdc.com>
Link: https://lkml.kernel.org/r/20180202145634.200291-2-arnd@arndb.de
Signed-off-by: Sasha Levin <alexander.levin@microsoft.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
arch/x86/power/hibernate_32.c | 2 +-
arch/x86/power/hibernate_64.c | 2 +-
include/linux/suspend.h | 2 ++
kernel/power/power.h | 3 ---
4 files changed, 4 insertions(+), 5 deletions(-)
--- a/arch/x86/power/hibernate_32.c
+++ b/arch/x86/power/hibernate_32.c
@@ -142,7 +142,7 @@ static inline void resume_init_first_lev
#endif
}
-int swsusp_arch_resume(void)
+asmlinkage int swsusp_arch_resume(void)
{
int error;
--- a/arch/x86/power/hibernate_64.c
+++ b/arch/x86/power/hibernate_64.c
@@ -149,7 +149,7 @@ static int relocate_restore_code(void)
return 0;
}
-int swsusp_arch_resume(void)
+asmlinkage int swsusp_arch_resume(void)
{
int error;
--- a/include/linux/suspend.h
+++ b/include/linux/suspend.h
@@ -378,6 +378,8 @@ extern int swsusp_page_is_forbidden(stru
extern void swsusp_set_page_free(struct page *);
extern void swsusp_unset_page_free(struct page *);
extern unsigned long get_safe_page(gfp_t gfp_mask);
+extern asmlinkage int swsusp_arch_suspend(void);
+extern asmlinkage int swsusp_arch_resume(void);
extern void hibernation_set_ops(const struct platform_hibernation_ops *ops);
extern int hibernate(void);
--- a/kernel/power/power.h
+++ b/kernel/power/power.h
@@ -103,9 +103,6 @@ extern int in_suspend;
extern dev_t swsusp_resume_device;
extern sector_t swsusp_resume_block;
-extern asmlinkage int swsusp_arch_suspend(void);
-extern asmlinkage int swsusp_arch_resume(void);
-
extern int create_basic_memory_bitmaps(void);
extern void free_basic_memory_bitmaps(void);
extern int hibernate_preallocate_memory(void);
^ permalink raw reply
* Re: [RFC] platform: detach from PM domains on shutdown
From: Rafael J. Wysocki @ 2018-05-28 8:31 UTC (permalink / raw)
To: Peng Fan
Cc: Rafael J. Wysocki, Ulf Hansson, Rafael J. Wysocki, Fabio Estevam,
Greg Kroah-Hartman, Linux Kernel Mailing List, Linux PM,
dl-linux-imx
In-Reply-To: <HE1PR04MB300481B481F96F875DCBFB2B886E0@HE1PR04MB3004.eurprd04.prod.outlook.com>
On Monday, May 28, 2018 10:01:09 AM CEST Peng Fan wrote:
> Hi, Rafael & Uffe
>
> > -----Original Message-----
> > From: Peng Fan
> > Sent: 2018年5月18日 16:53
> > To: Rafael J. Wysocki <rjw@rjwysocki.net>
> > Cc: Rafael J. Wysocki <rafael@kernel.org>; Ulf Hansson
> > <ulf.hansson@linaro.org>; Rafael J. Wysocki <rafael.j.wysocki@intel.com>;
> > Fabio Estevam <fabio.estevam@nxp.com>; Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org>; Linux Kernel Mailing List
> > <linux-kernel@vger.kernel.org>; Linux PM <linux-pm@vger.kernel.org>;
> > dl-linux-imx <linux-imx@nxp.com>
> > Subject: RE: [RFC] platform: detach from PM domains on shutdown
> >
> >
> >
> > > -----Original Message-----
> > > From: Rafael J. Wysocki [mailto:rjw@rjwysocki.net]
> > > Sent: 2018年5月18日 15:55
> > > To: Peng Fan <peng.fan@nxp.com>
> > > Cc: Rafael J. Wysocki <rafael@kernel.org>; Ulf Hansson
> > > <ulf.hansson@linaro.org>; Rafael J. Wysocki
> > > <rafael.j.wysocki@intel.com>; Fabio Estevam <fabio.estevam@nxp.com>;
> > > Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Linux Kernel Mailing
> > > List <linux-kernel@vger.kernel.org>; Linux PM
> > > <linux-pm@vger.kernel.org>; dl-linux-imx <linux-imx@nxp.com>
> > > Subject: Re: [RFC] platform: detach from PM domains on shutdown
> > >
> > > On Thursday, May 17, 2018 2:37:31 PM CEST Peng Fan wrote:
> > > >
> > > > > -----Original Message-----
> > > > > From: rjwysocki@gmail.com [mailto:rjwysocki@gmail.com] On Behalf
> > > > > Of Rafael J. Wysocki
> > > > > Sent: 2018年5月17日 16:01
> > > > > To: Peng Fan <peng.fan@nxp.com>
> > > > > Cc: Rafael J. Wysocki <rafael@kernel.org>; Ulf Hansson
> > > > > <ulf.hansson@linaro.org>; Rafael J. Wysocki
> > > > > <rafael.j.wysocki@intel.com>; Fabio Estevam
> > > > > <fabio.estevam@nxp.com>; Greg Kroah-Hartman
> > > > > <gregkh@linuxfoundation.org>; Linux Kernel Mailing List
> > > > > <linux-kernel@vger.kernel.org>; Linux PM
> > > > > <linux-pm@vger.kernel.org>; dl-linux-imx <linux-imx@nxp.com>
> > > > > Subject: Re: [RFC] platform: detach from PM domains on shutdown
> > > > >
> > > > > On Thu, May 17, 2018 at 4:33 AM, Peng Fan <peng.fan@nxp.com> wrote:
> > > > > >
> > > > > >
> > > > > >> -----Original Message-----
> > > > > >> From: rjwysocki@gmail.com [mailto:rjwysocki@gmail.com] On
> > > > > >> Behalf Of Rafael J. Wysocki
> > > > > >> Sent: 2018年5月17日 5:35
> > > > > >> To: Ulf Hansson <ulf.hansson@linaro.org>
> > > > > >> Cc: Peng Fan <peng.fan@nxp.com>; Rafael J. Wysocki
> > > > > >> <rafael.j.wysocki@intel.com>; Fabio Estevam
> > > > > >> <fabio.estevam@nxp.com>; Greg Kroah-Hartman
> > > > > >> <gregkh@linuxfoundation.org>; Linux Kernel Mailing List
> > > > > >> <linux-kernel@vger.kernel.org>; Linux PM
> > > > > >> <linux-pm@vger.kernel.org>; dl-linux-imx <linux-imx@nxp.com>
> > > > > >> Subject: Re: [RFC] platform: detach from PM domains on shutdown
> > > > > >>
> > > > > >> On Wed, May 16, 2018 at 11:52 AM, Ulf Hansson
> > > > > >> <ulf.hansson@linaro.org>
> > > > > >> wrote:
> > > > > >> > On 15 May 2018 at 11:01, Peng Fan <peng.fan@nxp.com> wrote:
> > > > > >> >> When reboot Linux, the PM domains attached to a device are
> > > > > >> >> not shutdown. To SoCs which relys on reset the whole SoC,
> > > > > >> >> there is no need to shutdown PM domains, but to Linux
> > > > > >> >> running in a virtual machine with devices pass-through, we
> > > > > >> >> could not reset the
> > > whole SoC.
> > > > > >> >> Currently we need Linux to shutdown its PM domains when reboot.
> > > > > >> >
> > > > > >> > I am not sure I understand exactly why the PM domain needs to
> > > > > >> > be shutdown for these cases, could you please elaborate a bit on that.
> > > > > >> >
> > > > > >> > BTW, what platform are you running on and also what PM
> > > > > >> > domains are being
> > > > > >> used?
> > > > > >> >
> > > > > >> > Anyway, it seems like there may be need for certain cases,
> > > > > >> > but certainly not all - especially since it may slow down the
> > > > > >> > shutdown process, when not needed.
> > > > > >> >
> > > > > >> > Can we make this runtime configurable, via sysfs or whatever
> > > > > >> > that makes
> > > > > >> sense!?
> > > > > >> >
> > > > > >> >>
> > > > > >> >> commit 2d30bb0b3889 ("platform: Do not detach from PM
> > > > > >> >> domains on shutdown"), removes what this patch tries to add,
> > > > > >> >> because of a
> > > warning.
> > > > > >> >> commit e79aee49bcf9 ("PM: Avoid false-positive warnings in
> > > > > >> >> dev_pm_domain_set()") already fixes the false alarm warning.
> > > > > >> >> So let's detach the power domain to shutdown PM domains
> > > > > >> >> after driver
> > > > > shutdown.
> > > > > >> >>
> > > > > >> >> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > > > > >> >> ---
> > > > > >> >>
> > > > > >> >> I do not find a better place to shutdown power domain when
> > > > > >> >> reboot Linux, so add back the line that commit 2d30bb0b3889
> > > > > >> >> removes, because it is a false alarm warning as commit
> > > > > >> >> e79aee49bcf9
> > > describes.
> > > > > >> >>
> > > > > >> >> drivers/base/platform.c | 1 +
> > > > > >> >> 1 file changed, 1 insertion(+)
> > > > > >> >>
> > > > > >> >> diff --git a/drivers/base/platform.c
> > > > > >> >> b/drivers/base/platform.c index 8075ddc70a17..a5929f24dc3c
> > > > > >> >> 100644
> > > > > >> >> --- a/drivers/base/platform.c
> > > > > >> >> +++ b/drivers/base/platform.c
> > > > > >> >> @@ -616,6 +616,7 @@ static void platform_drv_shutdown(struct
> > > > > >> >> device
> > > > > >> >> *_dev)
> > > > > >> >>
> > > > > >> >> if (drv->shutdown)
> > > > > >> >> drv->shutdown(dev);
> > > > > >> >> + dev_pm_domain_detach(_dev, true);
> > > > > >> >
> > > > > >> > This would somewhat work, but only for platform devices. To
> > > > > >> > make this fully work, we need to call dev_pm_domain_detach()
> > > > > >> > from amba, spi, etc as well.
> > > > > >> >
> > > > > >> > Perhaps another option to manage this more generally, an
> > > > > >> > without having detach devices, could be to extend the struct
> > > > > >> > dev_pm_domain with a new callback, "->shutdown()" and then
> > > > > >> > make the driver core call it from device_shutdown().
> > > > > >>
> > > > > >> I'm sensing a possible ordering slippery slope with this (it
> > > > > >> will only work if all of the drivers/bus types etc do the right
> > > > > >> thing in their
> > > > > >> ->shutdown callbacks so nothing depends on the domain going
> > forward).
> > > > > >>
> > > > > >> > Typically, for genpd, I would probably count the number of
> > > > > >> > calls being made to ->shutdown() per PM domain, then when it
> > > > > >> > reaches the number of attached devices to it, allow to power off it.
> > > > > >> >
> > > > > >> > Let's see what Rafael thinks about it.
> > > > > >>
> > > > > >> I'm not sure about the use case. The hypervisor should be able
> > > > > >> to take care of turning power domains off on the client OS
> > > > > >> reboot in theory. If the client OS leaving the hypervisor
> > > > > >> needs to worry about what state it leaves behind, the design of
> > > > > >> the hypervisor is sort of
> > > > > questionable IMO.
> > > > > >
> > > > > > This is valid concern. But moving the power domain logic into
> > > > > > hypervisor mostly micro-kernel design will introduce more
> > > > > > complexity and
> > > > > make certification harder.
> > > > > > Currently, Let Linux shutdown it's power domain is the easiest
> > > > > > way to me and make things work well after reboot.
> > > > >
> > > > > Well, to put it bluntly, if your hypervisor depends on the guest
> > > > > to do the right thing on exit, it doesn't do its job. I wouldn't
> > > > > have certified it for you if that was my decision.
> > > >
> > > > It is guest os not work well after guest os reboot. The hypervisor
> > > > is not
> > > affected.
> > > >
> > > > Thinking another case without hypervisor, M4 core run RTOS, A35 Core
> > > > run Linux, when Linux rebooting, RTOS should not be affected. After
> > > > Linux reboot itself, because its power domain is not paired with
> > > open/shutdown, some devices not function well.
> > >
> > > The question boils down to whether or not devices should be detached
> > > from PM domains on shutdown IMO.
> > >
> > > They are detached from PM domains on driver removal, so I guess one
> > > answer is "yes, in analogy with that". However, the point about
> > > performace brought up by Ulf seems to be valid too.
> > >
> > > In any case, the change should be made for all of the bus types using
> > > PM domains, not just one.
> >
> > Understand, it will increase shutdown time. How about shutdown the power
> > domain in platform_driver->shutdown, let the driver handle it's power domain
> > sthudown by itself?
> > Then no need common framework change.
>
> Do you have more suggestions on how to handle this the power domain shutdown?
I think you could add a platform syscore_shutdown hook to turn all power
domains off.
^ permalink raw reply
* Re: [PATCH 09/11] misc: throttler: Add core support for non-thermal throttling
From: Greg Kroah-Hartman @ 2018-05-28 8:08 UTC (permalink / raw)
To: Matthias Kaehlcke
Cc: MyungJoo Ham, Kyungmin Park, Chanwoo Choi, Arnd Bergmann,
Rob Herring, Mark Rutland, linux-pm, devicetree, linux-kernel,
Brian Norris, Douglas Anderson
In-Reply-To: <20180525203043.249193-10-mka@chromium.org>
On Fri, May 25, 2018 at 01:30:41PM -0700, Matthias Kaehlcke wrote:
> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> index 20be70c3f118..01a1714dd2ad 100644
> --- a/drivers/misc/Makefile
> +++ b/drivers/misc/Makefile
> @@ -57,3 +57,4 @@ obj-$(CONFIG_ASPEED_LPC_SNOOP) += aspeed-lpc-snoop.o
> obj-$(CONFIG_PCI_ENDPOINT_TEST) += pci_endpoint_test.o
> obj-$(CONFIG_OCXL) += ocxl/
> obj-$(CONFIG_MISC_RTSX) += cardreader/
> +obj-y += throttler/
Shouldn't you depend on a Kconfig option to traverse into this
directory?
> diff --git a/drivers/misc/throttler/Kconfig b/drivers/misc/throttler/Kconfig
> new file mode 100644
> index 000000000000..ef8388f6bc0a
> --- /dev/null
> +++ b/drivers/misc/throttler/Kconfig
> @@ -0,0 +1,13 @@
> +menuconfig THROTTLER
> + bool "Throttler support"
> + default n
the default is always 'n' no need to say it again here :)
> + depends on OF
> + select CPU_FREQ
> + select PM_DEVFREQ
> + help
> + This option enables core support for non-thermal throttling of CPUs
> + and devfreq devices.
> +
> + Note that you also need a event monitor module usually called
> + *_throttler.
> +
> diff --git a/drivers/misc/throttler/Makefile b/drivers/misc/throttler/Makefile
> new file mode 100644
> index 000000000000..c8d920cee315
> --- /dev/null
> +++ b/drivers/misc/throttler/Makefile
> @@ -0,0 +1 @@
> +obj-$(CONFIG_THROTTLER) += core.o
> diff --git a/drivers/misc/throttler/core.c b/drivers/misc/throttler/core.c
> new file mode 100644
> index 000000000000..c058d03212b8
> --- /dev/null
> +++ b/drivers/misc/throttler/core.c
> @@ -0,0 +1,373 @@
> +/*
> + * Core code for non-thermal throttling
> + *
> + * Copyright (C) 2018 Google, Inc.
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + */
Please just use a single SPDX line, like checkpatch.pl would have warned
you about. No need for the full verbous license boiler-plate text here
at all.
thanks,
greg k-h
^ permalink raw reply
* RE: [RFC] platform: detach from PM domains on shutdown
From: Peng Fan @ 2018-05-28 8:01 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Rafael J. Wysocki, Ulf Hansson, Rafael J. Wysocki, Fabio Estevam,
Greg Kroah-Hartman, Linux Kernel Mailing List, Linux PM,
dl-linux-imx
In-Reply-To: <HE1PR04MB30041C742230314926032D4D88900@HE1PR04MB3004.eurprd04.prod.outlook.com>
Hi, Rafael & Uffe
> -----Original Message-----
> From: Peng Fan
> Sent: 2018年5月18日 16:53
> To: Rafael J. Wysocki <rjw@rjwysocki.net>
> Cc: Rafael J. Wysocki <rafael@kernel.org>; Ulf Hansson
> <ulf.hansson@linaro.org>; Rafael J. Wysocki <rafael.j.wysocki@intel.com>;
> Fabio Estevam <fabio.estevam@nxp.com>; Greg Kroah-Hartman
> <gregkh@linuxfoundation.org>; Linux Kernel Mailing List
> <linux-kernel@vger.kernel.org>; Linux PM <linux-pm@vger.kernel.org>;
> dl-linux-imx <linux-imx@nxp.com>
> Subject: RE: [RFC] platform: detach from PM domains on shutdown
>
>
>
> > -----Original Message-----
> > From: Rafael J. Wysocki [mailto:rjw@rjwysocki.net]
> > Sent: 2018年5月18日 15:55
> > To: Peng Fan <peng.fan@nxp.com>
> > Cc: Rafael J. Wysocki <rafael@kernel.org>; Ulf Hansson
> > <ulf.hansson@linaro.org>; Rafael J. Wysocki
> > <rafael.j.wysocki@intel.com>; Fabio Estevam <fabio.estevam@nxp.com>;
> > Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Linux Kernel Mailing
> > List <linux-kernel@vger.kernel.org>; Linux PM
> > <linux-pm@vger.kernel.org>; dl-linux-imx <linux-imx@nxp.com>
> > Subject: Re: [RFC] platform: detach from PM domains on shutdown
> >
> > On Thursday, May 17, 2018 2:37:31 PM CEST Peng Fan wrote:
> > >
> > > > -----Original Message-----
> > > > From: rjwysocki@gmail.com [mailto:rjwysocki@gmail.com] On Behalf
> > > > Of Rafael J. Wysocki
> > > > Sent: 2018年5月17日 16:01
> > > > To: Peng Fan <peng.fan@nxp.com>
> > > > Cc: Rafael J. Wysocki <rafael@kernel.org>; Ulf Hansson
> > > > <ulf.hansson@linaro.org>; Rafael J. Wysocki
> > > > <rafael.j.wysocki@intel.com>; Fabio Estevam
> > > > <fabio.estevam@nxp.com>; Greg Kroah-Hartman
> > > > <gregkh@linuxfoundation.org>; Linux Kernel Mailing List
> > > > <linux-kernel@vger.kernel.org>; Linux PM
> > > > <linux-pm@vger.kernel.org>; dl-linux-imx <linux-imx@nxp.com>
> > > > Subject: Re: [RFC] platform: detach from PM domains on shutdown
> > > >
> > > > On Thu, May 17, 2018 at 4:33 AM, Peng Fan <peng.fan@nxp.com> wrote:
> > > > >
> > > > >
> > > > >> -----Original Message-----
> > > > >> From: rjwysocki@gmail.com [mailto:rjwysocki@gmail.com] On
> > > > >> Behalf Of Rafael J. Wysocki
> > > > >> Sent: 2018年5月17日 5:35
> > > > >> To: Ulf Hansson <ulf.hansson@linaro.org>
> > > > >> Cc: Peng Fan <peng.fan@nxp.com>; Rafael J. Wysocki
> > > > >> <rafael.j.wysocki@intel.com>; Fabio Estevam
> > > > >> <fabio.estevam@nxp.com>; Greg Kroah-Hartman
> > > > >> <gregkh@linuxfoundation.org>; Linux Kernel Mailing List
> > > > >> <linux-kernel@vger.kernel.org>; Linux PM
> > > > >> <linux-pm@vger.kernel.org>; dl-linux-imx <linux-imx@nxp.com>
> > > > >> Subject: Re: [RFC] platform: detach from PM domains on shutdown
> > > > >>
> > > > >> On Wed, May 16, 2018 at 11:52 AM, Ulf Hansson
> > > > >> <ulf.hansson@linaro.org>
> > > > >> wrote:
> > > > >> > On 15 May 2018 at 11:01, Peng Fan <peng.fan@nxp.com> wrote:
> > > > >> >> When reboot Linux, the PM domains attached to a device are
> > > > >> >> not shutdown. To SoCs which relys on reset the whole SoC,
> > > > >> >> there is no need to shutdown PM domains, but to Linux
> > > > >> >> running in a virtual machine with devices pass-through, we
> > > > >> >> could not reset the
> > whole SoC.
> > > > >> >> Currently we need Linux to shutdown its PM domains when reboot.
> > > > >> >
> > > > >> > I am not sure I understand exactly why the PM domain needs to
> > > > >> > be shutdown for these cases, could you please elaborate a bit on that.
> > > > >> >
> > > > >> > BTW, what platform are you running on and also what PM
> > > > >> > domains are being
> > > > >> used?
> > > > >> >
> > > > >> > Anyway, it seems like there may be need for certain cases,
> > > > >> > but certainly not all - especially since it may slow down the
> > > > >> > shutdown process, when not needed.
> > > > >> >
> > > > >> > Can we make this runtime configurable, via sysfs or whatever
> > > > >> > that makes
> > > > >> sense!?
> > > > >> >
> > > > >> >>
> > > > >> >> commit 2d30bb0b3889 ("platform: Do not detach from PM
> > > > >> >> domains on shutdown"), removes what this patch tries to add,
> > > > >> >> because of a
> > warning.
> > > > >> >> commit e79aee49bcf9 ("PM: Avoid false-positive warnings in
> > > > >> >> dev_pm_domain_set()") already fixes the false alarm warning.
> > > > >> >> So let's detach the power domain to shutdown PM domains
> > > > >> >> after driver
> > > > shutdown.
> > > > >> >>
> > > > >> >> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > > > >> >> ---
> > > > >> >>
> > > > >> >> I do not find a better place to shutdown power domain when
> > > > >> >> reboot Linux, so add back the line that commit 2d30bb0b3889
> > > > >> >> removes, because it is a false alarm warning as commit
> > > > >> >> e79aee49bcf9
> > describes.
> > > > >> >>
> > > > >> >> drivers/base/platform.c | 1 +
> > > > >> >> 1 file changed, 1 insertion(+)
> > > > >> >>
> > > > >> >> diff --git a/drivers/base/platform.c
> > > > >> >> b/drivers/base/platform.c index 8075ddc70a17..a5929f24dc3c
> > > > >> >> 100644
> > > > >> >> --- a/drivers/base/platform.c
> > > > >> >> +++ b/drivers/base/platform.c
> > > > >> >> @@ -616,6 +616,7 @@ static void platform_drv_shutdown(struct
> > > > >> >> device
> > > > >> >> *_dev)
> > > > >> >>
> > > > >> >> if (drv->shutdown)
> > > > >> >> drv->shutdown(dev);
> > > > >> >> + dev_pm_domain_detach(_dev, true);
> > > > >> >
> > > > >> > This would somewhat work, but only for platform devices. To
> > > > >> > make this fully work, we need to call dev_pm_domain_detach()
> > > > >> > from amba, spi, etc as well.
> > > > >> >
> > > > >> > Perhaps another option to manage this more generally, an
> > > > >> > without having detach devices, could be to extend the struct
> > > > >> > dev_pm_domain with a new callback, "->shutdown()" and then
> > > > >> > make the driver core call it from device_shutdown().
> > > > >>
> > > > >> I'm sensing a possible ordering slippery slope with this (it
> > > > >> will only work if all of the drivers/bus types etc do the right
> > > > >> thing in their
> > > > >> ->shutdown callbacks so nothing depends on the domain going
> forward).
> > > > >>
> > > > >> > Typically, for genpd, I would probably count the number of
> > > > >> > calls being made to ->shutdown() per PM domain, then when it
> > > > >> > reaches the number of attached devices to it, allow to power off it.
> > > > >> >
> > > > >> > Let's see what Rafael thinks about it.
> > > > >>
> > > > >> I'm not sure about the use case. The hypervisor should be able
> > > > >> to take care of turning power domains off on the client OS
> > > > >> reboot in theory. If the client OS leaving the hypervisor
> > > > >> needs to worry about what state it leaves behind, the design of
> > > > >> the hypervisor is sort of
> > > > questionable IMO.
> > > > >
> > > > > This is valid concern. But moving the power domain logic into
> > > > > hypervisor mostly micro-kernel design will introduce more
> > > > > complexity and
> > > > make certification harder.
> > > > > Currently, Let Linux shutdown it's power domain is the easiest
> > > > > way to me and make things work well after reboot.
> > > >
> > > > Well, to put it bluntly, if your hypervisor depends on the guest
> > > > to do the right thing on exit, it doesn't do its job. I wouldn't
> > > > have certified it for you if that was my decision.
> > >
> > > It is guest os not work well after guest os reboot. The hypervisor
> > > is not
> > affected.
> > >
> > > Thinking another case without hypervisor, M4 core run RTOS, A35 Core
> > > run Linux, when Linux rebooting, RTOS should not be affected. After
> > > Linux reboot itself, because its power domain is not paired with
> > open/shutdown, some devices not function well.
> >
> > The question boils down to whether or not devices should be detached
> > from PM domains on shutdown IMO.
> >
> > They are detached from PM domains on driver removal, so I guess one
> > answer is "yes, in analogy with that". However, the point about
> > performace brought up by Ulf seems to be valid too.
> >
> > In any case, the change should be made for all of the bus types using
> > PM domains, not just one.
>
> Understand, it will increase shutdown time. How about shutdown the power
> domain in platform_driver->shutdown, let the driver handle it's power domain
> sthudown by itself?
> Then no need common framework change.
Do you have more suggestions on how to handle this the power domain shutdown?
Thanks,
Peng.
>
> Thanks,
> Peng
^ permalink raw reply
* Re: [PATCH 09/11] misc: throttler: Add core support for non-thermal throttling
From: Chanwoo Choi @ 2018-05-28 7:32 UTC (permalink / raw)
To: Matthias Kaehlcke, MyungJoo Ham
Cc: Kyungmin Park, Arnd Bergmann, Greg Kroah-Hartman, Rob Herring,
Mark Rutland, linux-pm, devicetree, linux-kernel, Brian Norris,
Douglas Anderson
In-Reply-To: <20180525203043.249193-10-mka@chromium.org>
Hi Matthias,
IMHO, you better to split out the devfreq patches from
'throttler' patch set. Because I'm not sure throttler is either
necessary or not.
After finishing the review of 'throttler' patches without devfreq handling,
it would be better for you to send devfreq patches separately.
Regards,
Chanwoo Choi
On 2018년 05월 26일 05:30, Matthias Kaehlcke wrote:
> The purpose of the throttler is to provide support for non-thermal
> throttling. Throttling is triggered by external event, e.g. the
> detection of a high battery discharge current, close to the OCP limit
> of the battery. The throttler is only in charge of the throttling, not
> the monitoring, which is done by another (possibly platform specific)
> driver.
>
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> ---
> drivers/misc/Kconfig | 1 +
> drivers/misc/Makefile | 1 +
> drivers/misc/throttler/Kconfig | 13 ++
> drivers/misc/throttler/Makefile | 1 +
> drivers/misc/throttler/core.c | 373 ++++++++++++++++++++++++++++++++
> include/linux/throttler.h | 10 +
> 6 files changed, 399 insertions(+)
> create mode 100644 drivers/misc/throttler/Kconfig
> create mode 100644 drivers/misc/throttler/Makefile
> create mode 100644 drivers/misc/throttler/core.c
> create mode 100644 include/linux/throttler.h
>
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index 5d713008749b..691d9625d83c 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -513,4 +513,5 @@ source "drivers/misc/echo/Kconfig"
> source "drivers/misc/cxl/Kconfig"
> source "drivers/misc/ocxl/Kconfig"
> source "drivers/misc/cardreader/Kconfig"
> +source "drivers/misc/throttler/Kconfig"
> endmenu
> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> index 20be70c3f118..01a1714dd2ad 100644
> --- a/drivers/misc/Makefile
> +++ b/drivers/misc/Makefile
> @@ -57,3 +57,4 @@ obj-$(CONFIG_ASPEED_LPC_SNOOP) += aspeed-lpc-snoop.o
> obj-$(CONFIG_PCI_ENDPOINT_TEST) += pci_endpoint_test.o
> obj-$(CONFIG_OCXL) += ocxl/
> obj-$(CONFIG_MISC_RTSX) += cardreader/
> +obj-y += throttler/
> diff --git a/drivers/misc/throttler/Kconfig b/drivers/misc/throttler/Kconfig
> new file mode 100644
> index 000000000000..ef8388f6bc0a
> --- /dev/null
> +++ b/drivers/misc/throttler/Kconfig
> @@ -0,0 +1,13 @@
> +menuconfig THROTTLER
> + bool "Throttler support"
> + default n
> + depends on OF
> + select CPU_FREQ
> + select PM_DEVFREQ
> + help
> + This option enables core support for non-thermal throttling of CPUs
> + and devfreq devices.
> +
> + Note that you also need a event monitor module usually called
> + *_throttler.
> +
> diff --git a/drivers/misc/throttler/Makefile b/drivers/misc/throttler/Makefile
> new file mode 100644
> index 000000000000..c8d920cee315
> --- /dev/null
> +++ b/drivers/misc/throttler/Makefile
> @@ -0,0 +1 @@
> +obj-$(CONFIG_THROTTLER) += core.o
> diff --git a/drivers/misc/throttler/core.c b/drivers/misc/throttler/core.c
> new file mode 100644
> index 000000000000..c058d03212b8
> --- /dev/null
> +++ b/drivers/misc/throttler/core.c
> @@ -0,0 +1,373 @@
> +/*
> + * Core code for non-thermal throttling
> + *
> + * Copyright (C) 2018 Google, Inc.
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +#include <linux/cpufreq.h>
> +#include <linux/devfreq.h>
> +#include <linux/kernel.h>
> +#include <linux/notifier.h>
> +#include <linux/of.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +
> +/*
> + * Non-thermal throttling: throttling of system components in response to
> + * external events (e.g. high battery discharge current).
> + *
> + * The throttler supports throttling through cpufreq and devfreq. Multiple
> + * levels of throttling can be configured. At level 0 no throttling is
> + * active on behalf of the throttler, for values > 0 throttling is typically
> + * configured to be increasingly aggressive with each level.
> + * The number of throttling levels is not limited by the throttler (though
> + * it is likely limited by the throttling devices). It is not necessary to
> + * configure the same number of levels for all throttling devices. If the
> + * requested throttling level for a device is higher than the maximum level
> + * of the device the throttler will sleect the maximum throttling level of
> + * the device.
> + *
> + * Non-thermal throttling is split in two parts:
> + *
> + * - throttler core
> + * - parses the thermal policy
> + * - applies throttling settings for a requested level of throttling
> + *
> + * - event monitor driver
> + * - monitors the events that trigger throttling
> + * - determines the throttling level (often limited to on/off)
> + * - requests throttler core to apply throttling settings
> + *
> + * It is possible for a system to have more than one throttler and the
> + * throttlers may make use of the same throttling devices, in case of
> + * conflicting settings for a device the more aggressive values will be
> + * applied.
> + *
> + */
> +
> +struct thrcfg {
> + uint32_t *freqs;
> + int num_levels;
> +};
> +
> +struct cpufreq_thrdev {
> + uint32_t cpu;
> + struct thrcfg cfg;
> +};
> +
> +struct devfreq_thrdev {
> + struct devfreq *devfreq;
> + struct thrcfg cfg;
> + struct throttler *thr;
> + struct notifier_block nb;
> +};
> +
> +struct __thr_cpufreq {
> + struct cpufreq_thrdev *devs;
> + int ndevs;
> + struct notifier_block nb;
> +};
> +
> +struct __thr_devfreq {
> + struct devfreq_thrdev *devs;
> + int ndevs;
> +};
> +
> +struct throttler {
> + struct device *dev;
> + int level;
> + struct __thr_cpufreq cpufreq;
> + struct __thr_devfreq devfreq;
> +};
> +
> +static unsigned long thr_get_throttling_freq(struct thrcfg *cfg, int level)
> +{
> + if (level == 0 ) {
> + WARN(true, "level == 0");
> + return 0;
> + }
> +
> + if (level <= cfg->num_levels)
> + return cfg->freqs[level - 1];
> + else
> + return cfg->freqs[cfg->num_levels - 1];
> +}
> +
> +static int thr_cpufreq_event(struct notifier_block *nb,
> + unsigned long event, void *data)
> +{
> + struct throttler *thr =
> + container_of(nb, struct throttler, cpufreq.nb);
> + struct cpufreq_policy *policy = data;
> + struct cpufreq_thrdev *ctd;
> + int i;
> +
> + if ((event != CPUFREQ_ADJUST) || (thr->level == 0))
> + return NOTIFY_DONE;
> +
> + for (i = 0; i < thr->cpufreq.ndevs; i++) {
> + ctd = &thr->cpufreq.devs[i];
> +
> + if (ctd->cpu == policy->cpu) {
> + unsigned long clamp_freq =
> + thr_get_throttling_freq(&ctd->cfg, thr->level);
> + if (clamp_freq < policy->max) {
> + cpufreq_verify_within_limits(policy, 0, clamp_freq);
> + }
> + }
> + }
> +
> + return NOTIFY_DONE;
> +}
> +
> +static int thr_devfreq_event(struct notifier_block *nb,
> + unsigned long event, void *data)
> +{
> + struct devfreq_thrdev *dtd =
> + container_of(nb, struct devfreq_thrdev, nb);
> + struct throttler *thr = dtd->thr;
> + struct devfreq_policy *policy = data;
> + unsigned long clamp_freq;
> +
> + if ((event != DEVFREQ_ADJUST) || (thr->level == 0))
> + return NOTIFY_DONE;
> +
> + clamp_freq = thr_get_throttling_freq(&dtd->cfg, thr->level);
> + if (clamp_freq < policy->max)
> + devfreq_verify_within_limits(policy, 0, clamp_freq);
> +
> + return NOTIFY_DONE;
> +}
> +
> +static void thr_cpufreq_update_policy(struct throttler *thr)
> +{
> + int i;
> +
> + for (i = 0; i < thr->cpufreq.ndevs; i++) {
> + struct cpufreq_thrdev *ctd = &thr->cpufreq.devs[i];
> + struct cpufreq_policy *policy = cpufreq_cpu_get(ctd->cpu);
> +
> + if (!policy) {
> + dev_warn(thr->dev, "CPU%d does have no cpufreq policy!\n", ctd->cpu);
> + continue;
> + }
> +
> + cpufreq_update_policy(ctd->cpu);
> + cpufreq_cpu_put(policy);
> + }
> +}
> +
> +static int thr_parse_thrcfg(struct throttler *thr,
> + struct device_node *np, struct thrcfg *cfg) {
> + int err;
> +
> + cfg->num_levels =
> + of_property_count_u32_elems(np, "throttling-frequencies");
> + if (cfg->num_levels < 0) {
> + pr_err("%s: failed to determine number of throttling frequencies\n",
> + np->full_name);
> + return cfg->num_levels;
> + }
> +
> + cfg->freqs = devm_kzalloc(thr->dev,
> + cfg->num_levels * sizeof(u32), GFP_KERNEL);
> + if (!cfg->freqs)
> + return -ENOMEM;
> +
> + err = of_property_read_u32_array(np, "throttling-frequencies",
> + cfg->freqs, cfg->num_levels);
> + if (err) {
> + pr_err("%s: failed to read throttling frequencies\n", np->full_name);
> + return err;
> + }
> +
> + return 0;
> +}
> +
> +static struct devfreq *thr_find_devfreq_dev(struct throttler *thr,
> + struct device_node *np_df) {
> + struct device_node *node;
> + struct platform_device *pdev;
> +
> + node = of_parse_phandle(np_df, "device", 0);
> + if (!node) {
> + pr_err("%s: failed to get devfreq parent device\n",
> + np_df->full_name);
> + return ERR_PTR(-EINVAL);
> + }
> +
> + pdev = of_find_device_by_node(node);
> + if (!pdev) {
> + pr_err("%s: could not find devfreq parent device\n",
> + node->full_name);
> + return ERR_PTR(-EINVAL);
> + }
> +
> + return dev_to_devfreq(&pdev->dev);
> +}
> +
> +static int thr_parse_dt(struct throttler *thr, struct device_node *np)
> +{
> + struct device_node *node, *child;
> + int err, i;
> +
> + node = of_get_child_by_name(np, "cpufreq");
> + if (node) {
> + thr->cpufreq.ndevs = of_get_child_count(node);
> + thr->cpufreq.devs = devm_kzalloc(thr->dev,
> + sizeof(*thr->cpufreq.devs) * thr->cpufreq.ndevs,
> + GFP_KERNEL);
> +
> + i = 0;
> + for_each_child_of_node(node, child) {
> + struct cpufreq_thrdev *ctd = &thr->cpufreq.devs[i];
> +
> + err = of_property_read_u32(child, "cpu", &ctd->cpu);
> + if (err) {
> + pr_err("%s: failed to read CPU id\n", child->full_name);
> + return err;
> + }
> +
> + err = thr_parse_thrcfg(thr, child, &ctd->cfg);
> + if (err)
> + return err;
> +
> + i++;
> + }
> + }
> +
> + node = of_get_child_by_name(np, "devfreq");
> + if (node) {
> + thr->devfreq.ndevs = of_get_child_count(node);
> + thr->devfreq.devs = devm_kzalloc(thr->dev,
> + sizeof(*thr->devfreq.devs) * thr->devfreq.ndevs,
> + GFP_KERNEL);
> +
> + i = 0;
> + for_each_child_of_node(node, child) {
> + struct devfreq_thrdev *dtd = &thr->devfreq.devs[i];
> +
> + dtd->thr = thr;
> +
> + dtd->devfreq = thr_find_devfreq_dev(thr, child);
> + if (IS_ERR(dtd->devfreq))
> + return PTR_ERR(dtd->devfreq);
> +
> + err = thr_parse_thrcfg(thr, child, &dtd->cfg);
> + if (err)
> + return err;
> +
> + i++;
> + }
> + }
> +
> + return 0;
> +}
> +
> +static void thr_update_devfreq(struct devfreq *devfreq)
> +{
> + mutex_lock(&devfreq->lock);
> + update_devfreq(devfreq);
> + mutex_unlock(&devfreq->lock);
> +}
> +
> +void throttler_set_level(struct throttler *thr, int level)
> +{
> + int i;
> +
> + if (level == thr->level)
> + return;
> +
> + dev_dbg(thr->dev, "throttling level: %d\n", level);
> + thr->level = level;
> +
> + if (thr->cpufreq.ndevs > 0)
> + thr_cpufreq_update_policy(thr);
> +
> + if (thr->devfreq.ndevs > 0)
> + for (i = 0; i < thr->devfreq.ndevs; i++)
> + thr_update_devfreq(thr->devfreq.devs[i].devfreq);
> +}
> +EXPORT_SYMBOL_GPL(throttler_set_level);
> +
> +struct throttler *throttler_setup(struct device *dev)
> +{
> + struct throttler *thr;
> + struct device_node *np = dev->of_node;
> + int err, i;
> +
> + if (!np)
> + /* should never happen */
> + return ERR_PTR(-EINVAL);
> +
> + thr = devm_kzalloc(dev, sizeof(*thr), GFP_KERNEL);
> + if (!thr)
> + return ERR_PTR(-ENOMEM);
> +
> + thr->dev = dev;
> +
> + err = thr_parse_dt(thr, np);
> + if (err)
> + return ERR_PTR(err);
> +
> + if (thr->cpufreq.ndevs > 0) {
> + thr->cpufreq.nb.notifier_call = thr_cpufreq_event;
> + err = cpufreq_register_notifier(&thr->cpufreq.nb,
> + CPUFREQ_POLICY_NOTIFIER);
> + if (err < 0) {
> + dev_err(dev, "failed to register cpufreq notifier\n");
> + return ERR_PTR(err);
> + }
> + }
> +
> + for (i = 0; i < thr->devfreq.ndevs; i++) {
> + struct devfreq_thrdev *dtd = &thr->devfreq.devs[i];
> +
> + dtd->nb.notifier_call = thr_devfreq_event;
> + err = devm_devfreq_register_notifier(dev, dtd->devfreq,
> + &dtd->nb, DEVFREQ_POLICY_NOTIFIER);
> + if (err < 0) {
> + dev_err(dev, "failed to register devfreq notifier\n");
> + goto err_cpufreq_unregister;
> + }
> + }
> +
> + return thr;
> +
> +err_cpufreq_unregister:
> + if (thr->cpufreq.ndevs > 0)
> + cpufreq_unregister_notifier(&thr->cpufreq.nb,
> + CPUFREQ_POLICY_NOTIFIER);
> +
> + return ERR_PTR(err);
> +}
> +EXPORT_SYMBOL_GPL(throttler_setup);
> +
> +void throttler_teardown(struct throttler *thr)
> +{
> + int i;
> +
> + thr->level = 0;
> +
> + if (thr->cpufreq.ndevs > 0) {
> + thr_cpufreq_update_policy(thr);
> +
> + cpufreq_unregister_notifier(&thr->cpufreq.nb,
> + CPUFREQ_POLICY_NOTIFIER);
> + }
> +
> + if (thr->devfreq.ndevs > 0)
> + for (i = 0; i < thr->devfreq.ndevs; i++)
> + thr_update_devfreq(thr->devfreq.devs[i].devfreq);
> +}
> +EXPORT_SYMBOL_GPL(throttler_teardown);
> diff --git a/include/linux/throttler.h b/include/linux/throttler.h
> new file mode 100644
> index 000000000000..cab8c466da4b
> --- /dev/null
> +++ b/include/linux/throttler.h
> @@ -0,0 +1,10 @@
> +#ifndef __LINUX_THROTTLER_H__
> +#define __LINUX_THROTTLER_H__
> +
> +struct throttler;
> +
> +extern struct throttler *throttler_setup(struct device *dev);
> +extern void throttler_teardown(struct throttler *thr);
> +extern void throttler_set_level(struct throttler *thr, int level);
> +
> +#endif /* __LINUX_THROTTLER_H__ */
>
^ permalink raw reply
* Re: [PATCH v2 1/2] dt-bindings: cpufreq: Introduce QCOM CPUFREQ FW bindings
From: Taniya Das @ 2018-05-28 7:29 UTC (permalink / raw)
To: Viresh Kumar, Rob Herring
Cc: Rafael J. Wysocki, linux-kernel@vger.kernel.org, linux-pm,
Stephen Boyd, Rajendra Nayak, Amit Nischal, devicetree,
Saravana Kannan, Amit Kucheria
In-Reply-To: <20180524054333.56vga66xrkfuqtkr@vireshk-i7>
Hello Rob,
Could you please suggest if the below looks okay to be implemented?
On 5/24/2018 11:13 AM, Viresh Kumar wrote:
> On 24-05-18, 10:48, Taniya Das wrote:
>> Hello Rob, Viresh,
>>
>> Thank you for the comments. If I understand correctly, the device tree nodes
>> should look something like the below.
>>
>> Though I am not sure if any vendor name could be associated in the cpu
>> nodes.
>
> Well Rob said Yes to that I think.
>
>> Please suggest if my understanding is wrong.
>>
>> cpu@0 {
>> qcom,freq-domain = <&freq_domain_table0>;
>> …
>> };
>>
>> same follows for cpu 1/2/3
>>
>> cpu@400 {
>> qcom,freq-domain = <&freq_domain_table1>;
>> …
>> };
>> same follows for cpu 5/6/7
>>
>> freq_domain_table0 : freq_table {
>> reg = < >, < >, < > ;
>> reg-names = "perf_base", "lut_base", "en_base";
>> };
>>
>> freq_domain_table1 : freq_table {
>> reg = < >, < >, < > ;
>> reg-names = "perf_base", "lut_base", "en_base";
>> };
>
> Mostly yes.
>
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation.
--
^ permalink raw reply
* Re: [PATCH] cpufreq / CPPC: Add cpuinfo_cur_freq support for CPPC
From: George Cherian @ 2018-05-28 7:09 UTC (permalink / raw)
To: Prakash, Prashanth, George Cherian, linux-kernel, linux-pm
Cc: rjw, viresh.kumar
In-Reply-To: <37517652-9a74-83f8-1315-07fe79a78d73@codeaurora.org>
Hi Prashanth,
On 05/26/2018 02:30 AM, Prakash, Prashanth wrote:
>
> On 5/25/2018 12:27 AM, George Cherian wrote:
>> Hi Prashanth,
>>
>> On 05/25/2018 12:55 AM, Prakash, Prashanth wrote:
>>> Hi George,
>>>
>>> On 5/22/2018 5:42 AM, George Cherian wrote:
>>>> Per Section 8.4.7.1.3 of ACPI 6.2, The platform provides performance
>>>> feedback via set of performance counters. To determine the actual
>>>> performance level delivered over time, OSPM may read a set of
>>>> performance counters from the Reference Performance Counter Register
>>>> and the Delivered Performance Counter Register.
>>>>
>>>> OSPM calculates the delivered performance over a given time period by
>>>> taking a beginning and ending snapshot of both the reference and
>>>> delivered performance counters, and calculating:
>>>>
>>>> delivered_perf = reference_perf X (delta of delivered_perf counter / delta of reference_perf counter).
>>>>
>>>> Implement the above and hook this to the cpufreq->get method.
>>>>
>>>> Signed-off-by: George Cherian <george.cherian@cavium.com>
>>>> ---
>>>> drivers/cpufreq/cppc_cpufreq.c | 44 ++++++++++++++++++++++++++++++++++++++++++
>>>> 1 file changed, 44 insertions(+)
>>>>
>>>> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
>>>> index b15115a..a046915 100644
>>>> --- a/drivers/cpufreq/cppc_cpufreq.c
>>>> +++ b/drivers/cpufreq/cppc_cpufreq.c
>>>> @@ -240,10 +240,54 @@ static int cppc_cpufreq_cpu_init(struct cpufreq_policy *policy)
>>>> return ret;
>>>> }
>>>> +static int cppc_get_rate_from_fbctrs(struct cppc_perf_fb_ctrs fb_ctrs_t0,
>>>> + struct cppc_perf_fb_ctrs fb_ctrs_t1)
>>>> +{
>>>> + u64 delta_reference, delta_delivered;
>>>> + u64 reference_perf, ratio;
>>>> +
>>>> + reference_perf = fb_ctrs_t0.reference_perf;
>>>> + if (fb_ctrs_t1.reference > fb_ctrs_t0.reference)
>>>> + delta_reference = fb_ctrs_t1.reference - fb_ctrs_t0.reference;
>>>> + else /* Counters would have wrapped-around */
>>>> + delta_reference = ((u64)(~((u64)0)) - fb_ctrs_t0.reference) +
>>>> + fb_ctrs_t1.reference;
>>>> +
>>>> + if (fb_ctrs_t1.delivered > fb_ctrs_t0.delivered)
>>>> + delta_delivered = fb_ctrs_t1.delivered - fb_ctrs_t0.delivered;
>>>> + else /* Counters would have wrapped-around */
>>>> + delta_delivered = ((u64)(~((u64)0)) - fb_ctrs_t0.delivered) +
>>>> + fb_ctrs_t1.delivered;
>>> We need to check that the wraparound time is long enough to make sure that
>>> the counters cannot wrap around more than once. We can register a get() api
>>> only after checking that wraparound time value is reasonably high.
>>>
>>> I am not aware of any platforms where wraparound time is soo short, but
>>> wouldn't hurt to check once during init.
>> By design the wraparound time is a 64 bit counter, for that matter even
>> all the feedback counters too are 64 bit counters. I don't see any
>> chance in which the counters can wraparound twice in back to back reads.
>> The only situation is in which system itself is running at a really high
>> frequency. Even in that case today's spec is not sufficient to support the same.
>
> The spec doesn't say these have to be 64bit registers. The wraparound
> counter register is in spec to communicate the worst case(shortest)
> counter rollover time.
Spec says these are 32 or 64 bit registers. Spec also defines counter
wraparound time in seconds. The minimum value possible is 1 as zero
means the counters are never assumed to wrap around. Even in platforms
with value set as 1 (1 sec) I dont really see a situation in which
the counter can wraparound twice if we are putting a delay of 2usec
between sampling.
>
> As as mentioned before this is just a defensive check to make sure that
> the platform has not set it to some very low number (which is allowed
> by the spec).
It might be unnecessary to have a check like this.
>
>>
>>>> +
>>>> + if (delta_reference) /* Check to avoid divide-by zero */
>>>> + ratio = (delta_delivered * 1000) / delta_reference;
>>> Why not just return the computed value here instead of *1000 and later /1000?
>>> return (ref_per * delta_del) / delta_ref;
>> Yes.
>>>> + else
>>>> + return -EINVAL;
>>> Instead of EINVAL, i think we should return current frequency.
>>>
>> Sorry, I didn't get you, How do you calculate the current frequency?
>> Did you mean reference performance?
> I mean the performance that OSPM/Linux had requested earlier.
> i.e the desired_perf
Okay, I will make necessary changes for this in v2.
>>
>>> The counters can pause if CPUs are in idle state during our sampling interval, so
>>> If the counters did not progress, it is reasonable to assume the delivered perf was
>>> equal to desired perf.
>> No, that is wrong. Here the check is for reference performance delta.
>> This counter can never pause. In case of cpuidle only the delivered counters could pause. Delivered counters will pause only if the particular core enters power down mode, Otherwise we would be still clocking the core and we should be getting a delta across 2 sampling periods. In case if the reference counter is paused which by design is not correct then there is no point in returning reference performance numbers. That too is wrong. In case the low level FW is not updating the
>> counters properly then it should be evident till Linux, instead of returning a bogus frequency.
>
> Again you are describing how it works on a specific platform and not
> how it is described in spec. Section 8.4.7.1.3.1.1 of ACPI 6.2 states
> "The Reference Performance Counter Register counts at a fixed rate
> any time the processor is active."
> > Implies the counters *may* pause in idle state -I can imagine an
> implementation where you can keep this counter running and
> account for it via delivered counter, but we cannot make any
> assumptions outside of what the spec describes.
>
>>>
>>> Even if platform wanted to limit, since the CPUs were asleep(idle) we could not have
>>> observed lower performance, so we will not throw off any logic that could be driven
>>> using the returned value.
>>>> +
>>>> + return (reference_perf * ratio) / 1000;
>>> This should be converted to KHz as cpufreq is not aware of CPPC abstract scale
>> In our platform all performance registers are implemented in KHz. Because of which we never had an issue with conversion. I am not
>> aware whether ACPI mandates to use any particular unit. How is that
>> implemented in your platform? Just to avoid any extra conversion don't
>> you feel it is better to always report in KHz from firmware.
> Again think of spec not a specific platform :)
> - The CPPC spec works on abstract scale and cpufreq works in KHz.
> - The above computed value is in abstract scale
> - The abstarct scale may be in KHz on your platform, but we cannot assume the
> same about all the platforms
For now can I assume it to be in KHz only?
I am not sure how to convert the abstract scale to Khz.
Can you please give me some pointers on the same?
In spec there is currently no interface which tells what is the abstract
scale!!
>>
>>>> +}
>>>> +
>>>> +static unsigned int cppc_cpufreq_get_rate(unsigned int cpunum)
>>>> +{
>>>> + struct cppc_perf_fb_ctrs fb_ctrs_t0 = {0}, fb_ctrs_t1 = {0};
>>>> + int ret;
>>>> +
>>>> + ret = cppc_get_perf_ctrs(cpunum, &fb_ctrs_t0);
>>>> + if (ret)
>>>> + return ret;
>>>> +
>>>> + ret = cppc_get_perf_ctrs(cpunum, &fb_ctrs_t1);
>>>> + if (ret)
>>>> + return ret;
>>>> +
>>>> + return cppc_get_rate_from_fbctrs(fb_ctrs_t0, fb_ctrs_t1);
>>>> +}
>>> We need to make sure that we get a reasonably sample so make sure the reported
>>> performance is accurate.
>>> The counters can capture short transient throttling/limiting, so by sampling a really
>>> short duration of time we could return quite inaccurate measure of performance.
>>>
>> I would say it as a momentary thing only when the frequency is being ramped up/down.
> This exact behavior would depend on how different limiting functions are implemented.
> So this would vary from one platform to another.
>>
>>> We need to include some reasonable delay between the two calls. What is reasonable
>>> is debatable - so this can be few(2-10) microseconds defined as default. If the same value
>>> doesn't work for all the platforms, we might need to add a platform specific value.
>>>
>> cppc_get_perf_ctrs itself is a slow call, we have to format the CPC packet and ring a doorbell and then the response to be read from the shared registers. My initial implementation had a delay but in testing,
>> I found that it was unnecessary to have such a delay. Can you please
>> let me know whether it works without delay in your platform?
>>
>> Or else let me know whether udelay(10) is sufficient in between the
>> calls.
> Feedback counters need not be in PCC .
> 2us should be sufficient.
Yes I will add this to v2.
>>>> +
>>>> static struct cpufreq_driver cppc_cpufreq_driver = {
>>>> .flags = CPUFREQ_CONST_LOOPS,
>>>> .verify = cppc_verify_policy,
>>>> .target = cppc_cpufreq_set_target,
>>>> + .get = cppc_cpufreq_get_rate,
>>>> .init = cppc_cpufreq_cpu_init,
>>>> .stop_cpu = cppc_cpufreq_stop_cpu,
>>>> .name = "cppc_cpufreq",
>>>
>
^ permalink raw reply
* Re: [PATCH 05/11] PM / devfreq: governors: Return device frequency limits instead of user limits
From: Chanwoo Choi @ 2018-05-28 6:56 UTC (permalink / raw)
To: Matthias Kaehlcke, MyungJoo Ham
Cc: Kyungmin Park, Arnd Bergmann, Greg Kroah-Hartman, Rob Herring,
Mark Rutland, linux-pm, devicetree, linux-kernel, Brian Norris,
Douglas Anderson
In-Reply-To: <20180525203043.249193-6-mka@chromium.org>
Hi,
On 2018년 05월 26일 05:30, Matthias Kaehlcke wrote:
> The performance, powersave and simpleondemand governors can return
> df->min/max_freq, which are the user defined frequency limits.
> update_devfreq() already takes care of adjusting the target frequency
> with the user limits if necessary, therefore we can return
> df->scaling_min/max_freq instead, which is the min/max frequency
> supported by the device at a given time (depending on the
> enabled/disabled OPPs)
As you mentioned on the description, update_devfreq() adjusts
the final target frequency. So, actually, there are no any
benefits when changing from max_freq/min_freq to scaling_max/min_freq.
I think that it is not necessary.
>
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> ---
> drivers/devfreq/governor_performance.c | 2 +-
> drivers/devfreq/governor_powersave.c | 2 +-
> drivers/devfreq/governor_simpleondemand.c | 6 +++---
> 3 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/devfreq/governor_performance.c b/drivers/devfreq/governor_performance.c
> index 1c990cb45098..a8e3478b3c43 100644
> --- a/drivers/devfreq/governor_performance.c
> +++ b/drivers/devfreq/governor_performance.c
> @@ -20,7 +20,7 @@ static int devfreq_performance_func(struct devfreq *df,
> * target callback should be able to get floor value as
> * said in devfreq.h
> */
> - *freq = df->max_freq;
> + *freq = df->scaling_max_freq;
> return 0;
> }
>
> diff --git a/drivers/devfreq/governor_powersave.c b/drivers/devfreq/governor_powersave.c
> index 0c42f23249ef..8696efd32e5a 100644
> --- a/drivers/devfreq/governor_powersave.c
> +++ b/drivers/devfreq/governor_powersave.c
> @@ -20,7 +20,7 @@ static int devfreq_powersave_func(struct devfreq *df,
> * target callback should be able to get ceiling value as
> * said in devfreq.h
> */
> - *freq = df->min_freq;
> + *freq = df->scaling_min_freq;
> return 0;
> }
>
> diff --git a/drivers/devfreq/governor_simpleondemand.c b/drivers/devfreq/governor_simpleondemand.c
> index 3da7554b4837..805fee09c754 100644
> --- a/drivers/devfreq/governor_simpleondemand.c
> +++ b/drivers/devfreq/governor_simpleondemand.c
> @@ -46,7 +46,7 @@ static int devfreq_simple_ondemand_func(struct devfreq *df,
>
> /* Assume MAX if it is going to be divided by zero */
> if (stat->total_time == 0) {
> - *freq = df->max_freq;
> + *freq = df->scaling_max_freq;
> return 0;
> }
>
> @@ -59,13 +59,13 @@ static int devfreq_simple_ondemand_func(struct devfreq *df,
> /* Set MAX if it's busy enough */
> if (stat->busy_time * 100 >
> stat->total_time * dfso_upthreshold) {
> - *freq = df->max_freq;
> + *freq = df->scaling_max_freq;
> return 0;
> }
>
> /* Set MAX if we do not know the initial frequency */
> if (stat->current_frequency == 0) {
> - *freq = df->max_freq;
> + *freq = df->scaling_max_freq;
> return 0;
> }
>
>
--
Best Regards,
Chanwoo Choi
Samsung Electronics
^ permalink raw reply
* Re: [PATCH 02/11] PM / devfreq: Fix handling of min/max_freq == 0
From: Chanwoo Choi @ 2018-05-28 6:37 UTC (permalink / raw)
To: Matthias Kaehlcke, MyungJoo Ham
Cc: Kyungmin Park, Arnd Bergmann, Greg Kroah-Hartman, Rob Herring,
Mark Rutland, linux-pm, devicetree, linux-kernel, Brian Norris,
Douglas Anderson
In-Reply-To: <20180525203043.249193-3-mka@chromium.org>
Hi,
On 2018년 05월 26일 05:30, Matthias Kaehlcke wrote:
> Commit ab8f58ad72c4 ("PM / devfreq: Set min/max_freq when adding the
> devfreq device") initializes df->min/max_freq with the min/max OPP when
> the device is added. Later commit f1d981eaecf8 ("PM / devfreq: Use the
> available min/max frequency") adds df->scaling_min/max_freq and the
> following to the frequency adjustment code:
>
> max_freq = MIN(devfreq->scaling_max_freq, devfreq->max_freq);
>
> With the current handling of min/max_freq this is incorrect:
>
> Even though df->max_freq is now initialized to a value != 0 user space
> can still set it to 0, in this case max_freq would be 0 instead of
> df->scaling_max_freq as intended. In consequence the frequency adjustment
> is not performed:
>
> if (max_freq && freq > max_freq) {
> freq = max_freq;
>
> To fix this set df->min/max freq to the min/max OPP in max/max_freq_store,
> when the user passes a value of 0. This also prevents df->max_freq from
> being set below the min OPP when df->min_freq is 0, and similar for
> min_freq. Since it is now guaranteed that df->min/max_freq can't be 0 the
> checks for this case can be removed.
>
> Fixes: f1d981eaecf8 ("PM / devfreq: Use the available min/max frequency")
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> ---
> drivers/devfreq/devfreq.c | 30 ++++++++++++++++++------------
> 1 file changed, 18 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 0057ef5b0a98..67da4e7b486b 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -283,11 +283,11 @@ int update_devfreq(struct devfreq *devfreq)
> max_freq = MIN(devfreq->scaling_max_freq, devfreq->max_freq);
> min_freq = MAX(devfreq->scaling_min_freq, devfreq->min_freq);
>
> - if (min_freq && freq < min_freq) {
> + if (freq < min_freq) {
> freq = min_freq;
> flags &= ~DEVFREQ_FLAG_LEAST_UPPER_BOUND; /* Use GLB */
> }
> - if (max_freq && freq > max_freq) {
> + if (freq > max_freq) {
> freq = max_freq;
> flags |= DEVFREQ_FLAG_LEAST_UPPER_BOUND; /* Use LUB */
> }
> @@ -1123,17 +1123,20 @@ static ssize_t min_freq_store(struct device *dev, struct device_attribute *attr,
> struct devfreq *df = to_devfreq(dev);
> unsigned long value;
> int ret;
> - unsigned long max;
>
> ret = sscanf(buf, "%lu", &value);
> if (ret != 1)
> return -EINVAL;
>
> mutex_lock(&df->lock);
> - max = df->max_freq;
> - if (value && max && value > max) {
> - ret = -EINVAL;
> - goto unlock;
> +
> + if (value) {
> + if (value > df->max_freq) {
> + ret = -EINVAL;
> + goto unlock;
> + }
> + } else {
> + value = df->profile->freq_table[df->profile->max_state - 1];
> }
If you want to prevent that df->min_freq is zero,
you should reinitialize 'value' as following.
Because freq_table must be in ascending order.
value = df->profile->freq_table[0];
>
> df->min_freq = value;
> @@ -1158,17 +1161,20 @@ static ssize_t max_freq_store(struct device *dev, struct device_attribute *attr,
> struct devfreq *df = to_devfreq(dev);
> unsigned long value;
> int ret;
> - unsigned long min;
>
> ret = sscanf(buf, "%lu", &value);
> if (ret != 1)
> return -EINVAL;
>
> mutex_lock(&df->lock);
> - min = df->min_freq;
> - if (value && min && value < min) {
> - ret = -EINVAL;
> - goto unlock;
> +
> + if (!value) {
> + value = df->profile->freq_table[0];
ditto.
value = df->profile->freq_table[df->profile->max_state - 1];
> + } else {
> + if (value < df->min_freq) {
> + ret = -EINVAL;
> + goto unlock;
> + }
> }
>
> df->max_freq = value;
>
Actually, min_freq_store() and max_freq_store() are very similar.
But, this patch changed the order of conditional statement as following:
If there is no special reason, you better to keep the same format
for the readability.
min_freq_store()
if (value) {
...
} else {
value = df->profile->freq_table[df->profile->max_state - 1];
}
max_freq_store()
if (!value) {
value = df->profile->freq_table[0];
} else {
...
--
Best Regards,
Chanwoo Choi
Samsung Electronics
^ permalink raw reply
* RE: [PATCH 2/2] PM / devfreq: Generic cpufreq governor
From: MyungJoo Ham @ 2018-05-28 6:00 UTC (permalink / raw)
To: Kyungmin Park, Chanwoo Choi, Rob Herring, Mark Rutland
Cc: Saravana Kannan, Rajendra Nayak, Amit Kucheria,
linux-pm@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <1526631889-5084-3-git-send-email-skannan@codeaurora.org>
>Many CPU architectures have caches that can scale independent of the CPUs.
>Frequency scaling of the caches is necessary to make sure the cache is not
>a performance bottleneck that leads to poor performance and power. The same
>idea applies for RAM/DDR.
>
>To achieve this, this patch series adds a generic devfreq governor that can
>listen to the frequency transitions of each CPU frequency domain and then
>adjusts the frequency of the cache (or any devfreq device) based on the
>frequency of the CPUs.
I agree that we have some hardware pieces that want to configure
frequencies based on the CPUfreq.
Creating a devfreq governor that configures devfreq-freq
based on incoming events (CPUFreq-transition-event in this case)
is indeed a good idea.
However, I would like to ask the followings:
The overall code appears to be overly complex compared what you need.
- Do you really need to revive "CPUFREQ POLICY" events for this?
especially when you are going to look at "first CPU" only?
Cheers,
MyungJoo
^ permalink raw reply
* Re: [PATCH 04/11] PM / devfreq: Remove redundant frequency adjustment from governors
From: Chanwoo Choi @ 2018-05-28 5:36 UTC (permalink / raw)
To: Matthias Kaehlcke, MyungJoo Ham
Cc: Kyungmin Park, Arnd Bergmann, Greg Kroah-Hartman, Rob Herring,
Mark Rutland, linux-pm, devicetree, linux-kernel, Brian Norris,
Douglas Anderson
In-Reply-To: <20180525203043.249193-5-mka@chromium.org>
Hi,
On 2018년 05월 26일 05:30, Matthias Kaehlcke wrote:
> The userspace and simpleondemand governor determine a target frequency and
> then adjust it according to the df->min/max_freq limits that might have
> been set by user space. This adjustment is redundant, it is done in
> update_devfreq() for any governor, right after returning from
> governor->get_target_freq().
>
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> ---
> drivers/devfreq/governor_simpleondemand.c | 5 -----
> drivers/devfreq/governor_userspace.c | 16 ++++------------
> 2 files changed, 4 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/devfreq/governor_simpleondemand.c b/drivers/devfreq/governor_simpleondemand.c
> index 278964783fa6..3da7554b4837 100644
> --- a/drivers/devfreq/governor_simpleondemand.c
> +++ b/drivers/devfreq/governor_simpleondemand.c
> @@ -84,11 +84,6 @@ static int devfreq_simple_ondemand_func(struct devfreq *df,
> b = div_u64(b, (dfso_upthreshold - dfso_downdifferential / 2));
> *freq = (unsigned long) b;
>
> - if (df->min_freq && *freq < df->min_freq)
> - *freq = df->min_freq;
> - if (df->max_freq && *freq > df->max_freq)
> - *freq = df->max_freq;
> -
> return 0;
> }
>
> diff --git a/drivers/devfreq/governor_userspace.c b/drivers/devfreq/governor_userspace.c
> index 080607c3f34d..378d84c011df 100644
> --- a/drivers/devfreq/governor_userspace.c
> +++ b/drivers/devfreq/governor_userspace.c
> @@ -26,19 +26,11 @@ static int devfreq_userspace_func(struct devfreq *df, unsigned long *freq)
> {
> struct userspace_data *data = df->data;
>
> - if (data->valid) {
> - unsigned long adjusted_freq = data->user_frequency;
> -
> - if (df->max_freq && adjusted_freq > df->max_freq)
> - adjusted_freq = df->max_freq;
> -
> - if (df->min_freq && adjusted_freq < df->min_freq)
> - adjusted_freq = df->min_freq;
> -
> - *freq = adjusted_freq;
> - } else {
> + if (data->valid)
> + *freq = data->user_frequency;
> + else
> *freq = df->previous_freq; /* No user freq specified yet */
> - }
> +
> return 0;
> }
>
>
Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
--
Best Regards,
Chanwoo Choi
Samsung Electronics
^ permalink raw reply
* Re: [PATCH 03/11] PM / devfreq: Remove check for df->max_freq == 0 from governors
From: Chanwoo Choi @ 2018-05-28 5:27 UTC (permalink / raw)
To: Matthias Kaehlcke, MyungJoo Ham
Cc: Kyungmin Park, Arnd Bergmann, Greg Kroah-Hartman, Rob Herring,
Mark Rutland, linux-pm, devicetree, linux-kernel, Brian Norris,
Douglas Anderson
In-Reply-To: <20180525203043.249193-4-mka@chromium.org>
Hi,
On 2018년 05월 26일 05:30, Matthias Kaehlcke wrote:
> Commit "PM / devfreq: Fix handling of min/max_freq == 0" ensures that
> df->max_freq is not 0, remove unnecessary checks.
>
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> ---
> drivers/devfreq/governor_performance.c | 5 +----
> drivers/devfreq/governor_simpleondemand.c | 7 +++----
> 2 files changed, 4 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/devfreq/governor_performance.c b/drivers/devfreq/governor_performance.c
> index 4d23ecfbd948..1c990cb45098 100644
> --- a/drivers/devfreq/governor_performance.c
> +++ b/drivers/devfreq/governor_performance.c
> @@ -20,10 +20,7 @@ static int devfreq_performance_func(struct devfreq *df,
> * target callback should be able to get floor value as
> * said in devfreq.h
> */
> - if (!df->max_freq)
> - *freq = UINT_MAX;
> - else
> - *freq = df->max_freq;
> + *freq = df->max_freq;
> return 0;
> }
>
> diff --git a/drivers/devfreq/governor_simpleondemand.c b/drivers/devfreq/governor_simpleondemand.c
> index 28e0f2de7100..278964783fa6 100644
> --- a/drivers/devfreq/governor_simpleondemand.c
> +++ b/drivers/devfreq/governor_simpleondemand.c
> @@ -27,7 +27,6 @@ static int devfreq_simple_ondemand_func(struct devfreq *df,
> unsigned int dfso_upthreshold = DFSO_UPTHRESHOLD;
> unsigned int dfso_downdifferential = DFSO_DOWNDIFFERENCTIAL;
> struct devfreq_simple_ondemand_data *data = df->data;
> - unsigned long max = (df->max_freq) ? df->max_freq : UINT_MAX;
>
> err = devfreq_update_stats(df);
> if (err)
> @@ -47,7 +46,7 @@ static int devfreq_simple_ondemand_func(struct devfreq *df,
>
> /* Assume MAX if it is going to be divided by zero */
> if (stat->total_time == 0) {
> - *freq = max;
> + *freq = df->max_freq;
> return 0;
> }
>
> @@ -60,13 +59,13 @@ static int devfreq_simple_ondemand_func(struct devfreq *df,
> /* Set MAX if it's busy enough */
> if (stat->busy_time * 100 >
> stat->total_time * dfso_upthreshold) {
> - *freq = max;
> + *freq = df->max_freq;
> return 0;
> }
>
> /* Set MAX if we do not know the initial frequency */
> if (stat->current_frequency == 0) {
> - *freq = max;
> + *freq = df->max_freq;
> return 0;
> }
>
>
Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
--
Best Regards,
Chanwoo Choi
Samsung Electronics
^ permalink raw reply
* Re: [PATCH 01/11] PM / devfreq: Init user limits from OPP limits, not viceversa
From: Chanwoo Choi @ 2018-05-28 5:26 UTC (permalink / raw)
To: Matthias Kaehlcke, MyungJoo Ham
Cc: Kyungmin Park, Arnd Bergmann, Greg Kroah-Hartman, Rob Herring,
Mark Rutland, linux-pm, devicetree, linux-kernel, Brian Norris,
Douglas Anderson
In-Reply-To: <20180525203043.249193-2-mka@chromium.org>
Hi,
On 2018년 05월 26일 05:30, Matthias Kaehlcke wrote:
> Commit ab8f58ad72c4 ("PM / devfreq: Set min/max_freq when adding
> the devfreq device") introduced the initialization of the user
> limits min/max_freq from the lowest/highest available OPPs. Later
> commit f1d981eaecf8 ("PM / devfreq: Use the available min/max
> frequency") added scaling_min/max_freq, which actually represent
> the frequencies of the lowest/highest available OPP. scaling_min/
> max_freq are initialized with the values from min/max_freq, which
> is totally correct in the context, but a bit awkward to read.
>
> Swap the initialization and assign scaling_min/max_freq with the
> OPP freqs and then the user limts min/max_freq with scaling_min/
> max_freq.
>
> Needless to say that this change is a NOP, intended to improve
> readability.
>
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> ---
> drivers/devfreq/devfreq.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index fe2af6aa88fc..0057ef5b0a98 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -604,21 +604,21 @@ struct devfreq *devfreq_add_device(struct device *dev,
> mutex_lock(&devfreq->lock);
> }
>
> - devfreq->min_freq = find_available_min_freq(devfreq);
> - if (!devfreq->min_freq) {
> + devfreq->scaling_min_freq = find_available_min_freq(devfreq);
> + if (!devfreq->scaling_min_freq) {
> mutex_unlock(&devfreq->lock);
> err = -EINVAL;
> goto err_dev;
> }
> - devfreq->scaling_min_freq = devfreq->min_freq;
> + devfreq->min_freq = devfreq->scaling_min_freq;
>
> - devfreq->max_freq = find_available_max_freq(devfreq);
> - if (!devfreq->max_freq) {
> + devfreq->scaling_max_freq = find_available_max_freq(devfreq);
> + if (!devfreq->scaling_max_freq) {
> mutex_unlock(&devfreq->lock);
> err = -EINVAL;
> goto err_dev;
> }
> - devfreq->scaling_max_freq = devfreq->max_freq;
> + devfreq->max_freq = devfreq->scaling_max_freq;
>
> dev_set_name(&devfreq->dev, "devfreq%d",
> atomic_inc_return(&devfreq_no));
>
I already replied with my Reviewed-by tag. You are missing my tag.
Again,
Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
--
Best Regards,
Chanwoo Choi
Samsung Electronics
^ permalink raw reply
* RE: [PATCH 08/11] PM / devfreq: Make update_devfreq() public
From: MyungJoo Ham @ 2018-05-28 5:24 UTC (permalink / raw)
Cc: Kyungmin Park, Chanwoo Choi, Arnd Bergmann, Greg Kroah-Hartman,
Rob Herring, Mark Rutland, linux-pm@vger.kernel.org,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
Brian Norris, Douglas Anderson, Matthias Kaehlcke
In-Reply-To: <20180525203043.249193-9-mka@chromium.org>
>Currently update_devfreq() is only visible to devfreq governors outside
>of devfreq.c. Make it public to allow drivers that adjust devfreq policies
>to cause a re-evaluation of the frequency after a policy change.
>
>Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
>---
> drivers/devfreq/governor.h | 3 ---
> include/linux/devfreq.h | 8 ++++++++
> 2 files changed, 8 insertions(+), 3 deletions(-)
With the requirement from patch 9/11, this commit is reasonable enough.
Acked-by: MyungJoo Ham <myungjoo.ham@samsung.com>
^ permalink raw reply
* RE: [PATCH 07/11] PM / devfreg: Add support policy notifiers
From: MyungJoo Ham @ 2018-05-28 5:19 UTC (permalink / raw)
Cc: Kyungmin Park, Chanwoo Choi, Arnd Bergmann, Greg Kroah-Hartman,
Rob Herring, Mark Rutland, linux-pm@vger.kernel.org,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
Brian Norris, Douglas Anderson, Matthias Kaehlcke
In-Reply-To: <20180525203043.249193-8-mka@chromium.org>
>Policy notifiers are called before a frequency change and may narrow
>the min/max frequency range in devfreq_policy, which is used to adjust
>the target frequency if it is beyond this range.
>
>Also add a few helpers:
> - devfreq_verify_within_[dev_]limits()
> - should be used by the notifiers for policy adjustments.
> - dev_to_devfreq()
> - lookup a devfreq strict from a device pointer
>
>Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
>---
> drivers/devfreq/devfreq.c | 47 +++++++++++++++++++++-------
> include/linux/devfreq.h | 66 +++++++++++++++++++++++++++++++++++++++
> 2 files changed, 102 insertions(+), 11 deletions(-)
Hello Matthias,
Why should we have yet another notifier from an instance of devfreq?
Wouldn't it better to let the current notifier (transition notifier)
handle new events as well by adding possible event states to it?
Anyway, is this the reason why you've separated some data of devfreq
into "policy" struct? (I was wondering why while reading commit 6/11).
Cheers
MyungJoo
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox