* [patch 4/5] cpuidle: add haltpoll governor
From: Marcelo Tosatti @ 2019-07-03 23:51 UTC (permalink / raw)
To: kvm-devel
Cc: Paolo Bonzini, Radim Krcmar, Andrea Arcangeli, Rafael J. Wysocki,
Peter Zijlstra, Wanpeng Li, Konrad Rzeszutek Wilk,
Raslan KarimAllah, Boris Ostrovsky, Ankur Arora,
Christian Borntraeger, linux-pm, Marcelo Tosatti
In-Reply-To: <20190703235124.783034907@amt.cnet>
[-- Attachment #1: 03-cpuidle-haltpoll-governor --]
[-- Type: text/plain, Size: 9929 bytes --]
The cpuidle_haltpoll governor, in conjunction with the haltpoll cpuidle
driver, allows guest vcpus to poll for a specified amount of time before
halting.
This provides the following benefits to host side polling:
1) The POLL flag is set while polling is performed, which allows
a remote vCPU to avoid sending an IPI (and the associated
cost of handling the IPI) when performing a wakeup.
2) The VM-exit cost can be avoided.
The downside of guest side polling is that polling is performed
even with other runnable tasks in the host.
Results comparing halt_poll_ns and server/client application
where a small packet is ping-ponged:
host --> 31.33
halt_poll_ns=300000 / no guest busy spin --> 33.40 (93.8%)
halt_poll_ns=0 / guest_halt_poll_ns=300000 --> 32.73 (95.7%)
For the SAP HANA benchmarks (where idle_spin is a parameter
of the previous version of the patch, results should be the
same):
hpns == halt_poll_ns
idle_spin=0/ idle_spin=800/ idle_spin=0/
hpns=200000 hpns=0 hpns=800000
DeleteC06T03 (100 thread) 1.76 1.71 (-3%) 1.78 (+1%)
InsertC16T02 (100 thread) 2.14 2.07 (-3%) 2.18 (+1.8%)
DeleteC00T01 (1 thread) 1.34 1.28 (-4.5%) 1.29 (-3.7%)
UpdateC00T03 (1 thread) 4.72 4.18 (-12%) 4.53 (-5%)
Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
---
Documentation/virtual/guest-halt-polling.txt | 79 ++++++++++++
drivers/cpuidle/Kconfig | 11 +
drivers/cpuidle/governors/Makefile | 1
drivers/cpuidle/governors/haltpoll.c | 175 +++++++++++++++++++++++++++
4 files changed, 266 insertions(+)
Index: linux-2.6-newcpuidle.git/drivers/cpuidle/Kconfig
===================================================================
--- linux-2.6-newcpuidle.git.orig/drivers/cpuidle/Kconfig
+++ linux-2.6-newcpuidle.git/drivers/cpuidle/Kconfig
@@ -33,6 +33,17 @@ config CPU_IDLE_GOV_TEO
Some workloads benefit from using it and it generally should be safe
to use. Say Y here if you are not happy with the alternatives.
+config CPU_IDLE_GOV_HALTPOLL
+ bool "Haltpoll governor (for virtualized systems)"
+ depends on KVM_GUEST
+ help
+ This governor implements haltpoll idle state selection, to be
+ used in conjunction with the haltpoll cpuidle driver, allowing
+ for polling for a certain amount of time before entering idle
+ state.
+
+ Some virtualized workloads benefit from using it.
+
config DT_IDLE_STATES
bool
Index: linux-2.6-newcpuidle.git/drivers/cpuidle/governors/Makefile
===================================================================
--- linux-2.6-newcpuidle.git.orig/drivers/cpuidle/governors/Makefile
+++ linux-2.6-newcpuidle.git/drivers/cpuidle/governors/Makefile
@@ -6,3 +6,4 @@
obj-$(CONFIG_CPU_IDLE_GOV_LADDER) += ladder.o
obj-$(CONFIG_CPU_IDLE_GOV_MENU) += menu.o
obj-$(CONFIG_CPU_IDLE_GOV_TEO) += teo.o
+obj-$(CONFIG_CPU_IDLE_GOV_HALTPOLL) += haltpoll.o
Index: linux-2.6-newcpuidle.git/drivers/cpuidle/governors/haltpoll.c
===================================================================
--- /dev/null
+++ linux-2.6-newcpuidle.git/drivers/cpuidle/governors/haltpoll.c
@@ -0,0 +1,150 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * haltpoll.c - haltpoll idle governor
+ *
+ * Copyright 2019 Red Hat, Inc. and/or its affiliates.
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2. See
+ * the COPYING file in the top-level directory.
+ *
+ * Authors: Marcelo Tosatti <mtosatti@redhat.com>
+ */
+
+#include <linux/kernel.h>
+#include <linux/cpuidle.h>
+#include <linux/time.h>
+#include <linux/ktime.h>
+#include <linux/hrtimer.h>
+#include <linux/tick.h>
+#include <linux/sched.h>
+#include <linux/module.h>
+#include <linux/kvm_para.h>
+
+static unsigned int guest_halt_poll_ns __read_mostly = 200000;
+module_param(guest_halt_poll_ns, uint, 0644);
+
+/* division factor to shrink halt_poll_ns */
+static unsigned int guest_halt_poll_shrink __read_mostly = 2;
+module_param(guest_halt_poll_shrink, uint, 0644);
+
+/* multiplication factor to grow per-cpu poll_limit_ns */
+static unsigned int guest_halt_poll_grow __read_mostly = 2;
+module_param(guest_halt_poll_grow, uint, 0644);
+
+/* value in us to start growing per-cpu halt_poll_ns */
+static unsigned int guest_halt_poll_grow_start __read_mostly = 50000;
+module_param(guest_halt_poll_grow_start, uint, 0644);
+
+/* allow shrinking guest halt poll */
+static bool guest_halt_poll_allow_shrink __read_mostly = true;
+module_param(guest_halt_poll_allow_shrink, bool, 0644);
+
+/**
+ * haltpoll_select - selects the next idle state to enter
+ * @drv: cpuidle driver containing state data
+ * @dev: the CPU
+ * @stop_tick: indication on whether or not to stop the tick
+ */
+static int haltpoll_select(struct cpuidle_driver *drv,
+ struct cpuidle_device *dev,
+ bool *stop_tick)
+{
+ int latency_req = cpuidle_governor_latency_req(dev->cpu);
+
+ if (!drv->state_count || latency_req == 0) {
+ *stop_tick = false;
+ return 0;
+ }
+
+ if (dev->poll_limit_ns == 0)
+ return 1;
+
+ /* Last state was poll? */
+ if (dev->last_state_idx == 0) {
+ /* Halt if no event occurred on poll window */
+ if (dev->poll_time_limit == true)
+ return 1;
+
+ *stop_tick = false;
+ /* Otherwise, poll again */
+ return 0;
+ }
+
+ *stop_tick = false;
+ /* Last state was halt: poll */
+ return 0;
+}
+
+static void adjust_poll_limit(struct cpuidle_device *dev, unsigned int block_us)
+{
+ unsigned int val;
+ u64 block_ns = block_us*NSEC_PER_USEC;
+
+ /* Grow cpu_halt_poll_us if
+ * cpu_halt_poll_us < block_ns < guest_halt_poll_us
+ */
+ if (block_ns > dev->poll_limit_ns && block_ns <= guest_halt_poll_ns) {
+ val = dev->poll_limit_ns * guest_halt_poll_grow;
+
+ if (val < guest_halt_poll_grow_start)
+ val = guest_halt_poll_grow_start;
+ if (val > guest_halt_poll_ns)
+ val = guest_halt_poll_ns;
+
+ dev->poll_limit_ns = val;
+ } else if (block_ns > guest_halt_poll_ns &&
+ guest_halt_poll_allow_shrink) {
+ unsigned int shrink = guest_halt_poll_shrink;
+
+ val = dev->poll_limit_ns;
+ if (shrink == 0)
+ val = 0;
+ else
+ val /= shrink;
+ dev->poll_limit_ns = val;
+ }
+}
+
+/**
+ * haltpoll_reflect - update variables and update poll time
+ * @dev: the CPU
+ * @index: the index of actual entered state
+ */
+static void haltpoll_reflect(struct cpuidle_device *dev, int index)
+{
+ dev->last_state_idx = index;
+
+ if (index != 0)
+ adjust_poll_limit(dev, dev->last_residency);
+}
+
+/**
+ * haltpoll_enable_device - scans a CPU's states and does setup
+ * @drv: cpuidle driver
+ * @dev: the CPU
+ */
+static int haltpoll_enable_device(struct cpuidle_driver *drv,
+ struct cpuidle_device *dev)
+{
+ dev->poll_limit_ns = 0;
+
+ return 0;
+}
+
+static struct cpuidle_governor haltpoll_governor = {
+ .name = "haltpoll",
+ .rating = 21,
+ .enable = haltpoll_enable_device,
+ .select = haltpoll_select,
+ .reflect = haltpoll_reflect,
+};
+
+static int __init init_haltpoll(void)
+{
+ if (kvm_para_available())
+ return cpuidle_register_governor(&haltpoll_governor);
+
+ return 0;
+}
+
+postcore_initcall(init_haltpoll);
Index: linux-2.6-newcpuidle.git/Documentation/virtual/guest-halt-polling.txt
===================================================================
--- /dev/null
+++ linux-2.6-newcpuidle.git/Documentation/virtual/guest-halt-polling.txt
@@ -0,0 +1,79 @@
+Guest halt polling
+==================
+
+The cpuidle_haltpoll driver, with the haltpoll governor, allows
+the guest vcpus to poll for a specified amount of time before
+halting.
+This provides the following benefits to host side polling:
+
+ 1) The POLL flag is set while polling is performed, which allows
+ a remote vCPU to avoid sending an IPI (and the associated
+ cost of handling the IPI) when performing a wakeup.
+
+ 2) The VM-exit cost can be avoided.
+
+The downside of guest side polling is that polling is performed
+even with other runnable tasks in the host.
+
+The basic logic as follows: A global value, guest_halt_poll_ns,
+is configured by the user, indicating the maximum amount of
+time polling is allowed. This value is fixed.
+
+Each vcpu has an adjustable guest_halt_poll_ns
+("per-cpu guest_halt_poll_ns"), which is adjusted by the algorithm
+in response to events (explained below).
+
+Module Parameters
+=================
+
+The haltpoll governor has 5 tunable module parameters:
+
+1) guest_halt_poll_ns:
+Maximum amount of time, in nanoseconds, that polling is
+performed before halting.
+
+Default: 200000
+
+2) guest_halt_poll_shrink:
+Division factor used to shrink per-cpu guest_halt_poll_ns when
+wakeup event occurs after the global guest_halt_poll_ns.
+
+Default: 2
+
+3) guest_halt_poll_grow:
+Multiplication factor used to grow per-cpu guest_halt_poll_ns
+when event occurs after per-cpu guest_halt_poll_ns
+but before global guest_halt_poll_ns.
+
+Default: 2
+
+4) guest_halt_poll_grow_start:
+The per-cpu guest_halt_poll_ns eventually reaches zero
+in case of an idle system. This value sets the initial
+per-cpu guest_halt_poll_ns when growing. This can
+be increased from 10000, to avoid misses during the initial
+growth stage:
+
+10k, 20k, 40k, ... (example assumes guest_halt_poll_grow=2).
+
+Default: 50000
+
+5) guest_halt_poll_allow_shrink:
+
+Bool parameter which allows shrinking. Set to N
+to avoid it (per-cpu guest_halt_poll_ns will remain
+high once achieves global guest_halt_poll_ns value).
+
+Default: Y
+
+The module parameters can be set from the debugfs files in:
+
+ /sys/module/haltpoll/parameters/
+
+Further Notes
+=============
+
+- Care should be taken when setting the guest_halt_poll_ns parameter as a
+large value has the potential to drive the cpu usage to 100% on a machine which
+would be almost entirely idle otherwise.
+
^ permalink raw reply
* [patch 1/5] add cpuidle-haltpoll driver
From: Marcelo Tosatti @ 2019-07-03 23:51 UTC (permalink / raw)
To: kvm-devel
Cc: Paolo Bonzini, Radim Krcmar, Andrea Arcangeli, Rafael J. Wysocki,
Peter Zijlstra, Wanpeng Li, Konrad Rzeszutek Wilk,
Raslan KarimAllah, Boris Ostrovsky, Ankur Arora,
Christian Borntraeger, linux-pm, Marcelo Tosatti
In-Reply-To: <20190703235124.783034907@amt.cnet>
[-- Attachment #1: 00-cpuidle-haltpoll --]
[-- Type: text/plain, Size: 4092 bytes --]
Add a cpuidle driver that calls the architecture default_idle routine.
To be used in conjunction with the haltpoll governor.
Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
---
arch/x86/kernel/process.c | 2 -
drivers/cpuidle/Kconfig | 9 +++++
drivers/cpuidle/Makefile | 1
drivers/cpuidle/cpuidle-haltpoll.c | 65 +++++++++++++++++++++++++++++++++++++
4 files changed, 76 insertions(+), 1 deletion(-)
Index: linux-2.6-newcpuidle.git/arch/x86/kernel/process.c
===================================================================
--- linux-2.6-newcpuidle.git.orig/arch/x86/kernel/process.c
+++ linux-2.6-newcpuidle.git/arch/x86/kernel/process.c
@@ -580,7 +580,7 @@ void __cpuidle default_idle(void)
safe_halt();
trace_cpu_idle_rcuidle(PWR_EVENT_EXIT, smp_processor_id());
}
-#ifdef CONFIG_APM_MODULE
+#if defined(CONFIG_APM_MODULE) || defined(CONFIG_HALTPOLL_CPUIDLE_MODULE)
EXPORT_SYMBOL(default_idle);
#endif
Index: linux-2.6-newcpuidle.git/drivers/cpuidle/Kconfig
===================================================================
--- linux-2.6-newcpuidle.git.orig/drivers/cpuidle/Kconfig
+++ linux-2.6-newcpuidle.git/drivers/cpuidle/Kconfig
@@ -51,6 +51,15 @@ depends on PPC
source "drivers/cpuidle/Kconfig.powerpc"
endmenu
+config HALTPOLL_CPUIDLE
+ tristate "Halt poll cpuidle driver"
+ depends on X86 && KVM_GUEST
+ default y
+ help
+ This option enables halt poll cpuidle driver, which allows to poll
+ before halting in the guest (more efficient than polling in the
+ host via halt_poll_ns for some scenarios).
+
endif
config ARCH_NEEDS_CPU_IDLE_COUPLED
Index: linux-2.6-newcpuidle.git/drivers/cpuidle/Makefile
===================================================================
--- linux-2.6-newcpuidle.git.orig/drivers/cpuidle/Makefile
+++ linux-2.6-newcpuidle.git/drivers/cpuidle/Makefile
@@ -7,6 +7,7 @@ obj-y += cpuidle.o driver.o governor.o s
obj-$(CONFIG_ARCH_NEEDS_CPU_IDLE_COUPLED) += coupled.o
obj-$(CONFIG_DT_IDLE_STATES) += dt_idle_states.o
obj-$(CONFIG_ARCH_HAS_CPU_RELAX) += poll_state.o
+obj-$(CONFIG_HALTPOLL_CPUIDLE) += cpuidle-haltpoll.o
##################################################################################
# ARM SoC drivers
Index: linux-2.6-newcpuidle.git/drivers/cpuidle/cpuidle-haltpoll.c
===================================================================
--- /dev/null
+++ linux-2.6-newcpuidle.git/drivers/cpuidle/cpuidle-haltpoll.c
@@ -0,0 +1,69 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * cpuidle driver for haltpoll governor.
+ *
+ * Copyright 2019 Red Hat, Inc. and/or its affiliates.
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2. See
+ * the COPYING file in the top-level directory.
+ *
+ * Authors: Marcelo Tosatti <mtosatti@redhat.com>
+ */
+
+#include <linux/init.h>
+#include <linux/cpuidle.h>
+#include <linux/module.h>
+#include <linux/sched/idle.h>
+#include <linux/kvm_para.h>
+
+static int default_enter_idle(struct cpuidle_device *dev,
+ struct cpuidle_driver *drv, int index)
+{
+ if (current_clr_polling_and_test()) {
+ local_irq_enable();
+ return index;
+ }
+ default_idle();
+ return index;
+}
+
+static struct cpuidle_driver haltpoll_driver = {
+ .name = "haltpoll",
+ .owner = THIS_MODULE,
+ .states = {
+ { /* entry 0 is for polling */ },
+ {
+ .enter = default_enter_idle,
+ .exit_latency = 1,
+ .target_residency = 1,
+ .power_usage = -1,
+ .name = "haltpoll idle",
+ .desc = "default architecture idle",
+ },
+ },
+ .safe_state_index = 0,
+ .state_count = 2,
+};
+
+static int __init haltpoll_init(void)
+{
+ struct cpuidle_driver *drv = &haltpoll_driver;
+
+ cpuidle_poll_state_init(drv);
+
+ if (!kvm_para_available())
+ return 0;
+
+ return cpuidle_register(&haltpoll_driver, NULL);
+}
+
+static void __exit haltpoll_exit(void)
+{
+ cpuidle_unregister(&haltpoll_driver);
+}
+
+module_init(haltpoll_init);
+module_exit(haltpoll_exit);
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Marcelo Tosatti <mtosatti@redhat.com>");
+
^ permalink raw reply
* [patch 2/5] cpuidle: add poll_limit_ns to cpuidle_device structure
From: Marcelo Tosatti @ 2019-07-03 23:51 UTC (permalink / raw)
To: kvm-devel
Cc: Paolo Bonzini, Radim Krcmar, Andrea Arcangeli, Rafael J. Wysocki,
Peter Zijlstra, Wanpeng Li, Konrad Rzeszutek Wilk,
Raslan KarimAllah, Boris Ostrovsky, Ankur Arora,
Christian Borntraeger, linux-pm, Marcelo Tosatti
In-Reply-To: <20190703235124.783034907@amt.cnet>
[-- Attachment #1: 01-poll-time --]
[-- Type: text/plain, Size: 5475 bytes --]
Add a poll_limit_ns variable to cpuidle_device structure.
Calculate and configure it in the new cpuidle_poll_time
function, in case its zero.
Individual governors are allowed to override this value.
Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
---
drivers/cpuidle/cpuidle.c | 40 ++++++++++++++++++++++++++++++++++++++++
drivers/cpuidle/poll_state.c | 11 ++---------
include/linux/cpuidle.h | 8 ++++++++
3 files changed, 50 insertions(+), 9 deletions(-)
Index: linux-2.6-newcpuidle.git/drivers/cpuidle/cpuidle.c
===================================================================
--- linux-2.6-newcpuidle.git.orig/drivers/cpuidle/cpuidle.c
+++ linux-2.6-newcpuidle.git/drivers/cpuidle/cpuidle.c
@@ -362,6 +362,36 @@ void cpuidle_reflect(struct cpuidle_devi
}
/**
+ * cpuidle_poll_time - return amount of time to poll for,
+ * governors can override dev->poll_limit_ns if necessary
+ *
+ * @drv: the cpuidle driver tied with the cpu
+ * @dev: the cpuidle device
+ *
+ */
+u64 cpuidle_poll_time(struct cpuidle_driver *drv,
+ struct cpuidle_device *dev)
+{
+ int i;
+ u64 limit_ns;
+
+ if (dev->poll_limit_ns)
+ return dev->poll_limit_ns;
+
+ limit_ns = TICK_NSEC;
+ for (i = 1; i < drv->state_count; i++) {
+ if (drv->states[i].disabled || dev->states_usage[i].disable)
+ continue;
+
+ limit_ns = (u64)drv->states[i].target_residency * NSEC_PER_USEC;
+ }
+
+ dev->poll_limit_ns = limit_ns;
+
+ return dev->poll_limit_ns;
+}
+
+/**
* cpuidle_install_idle_handler - installs the cpuidle idle loop handler
*/
void cpuidle_install_idle_handler(void)
Index: linux-2.6-newcpuidle.git/drivers/cpuidle/poll_state.c
===================================================================
--- linux-2.6-newcpuidle.git.orig/drivers/cpuidle/poll_state.c
+++ linux-2.6-newcpuidle.git/drivers/cpuidle/poll_state.c
@@ -20,16 +20,9 @@ static int __cpuidle poll_idle(struct cp
local_irq_enable();
if (!current_set_polling_and_test()) {
unsigned int loop_count = 0;
- u64 limit = TICK_NSEC;
- int i;
+ u64 limit;
- for (i = 1; i < drv->state_count; i++) {
- if (drv->states[i].disabled || dev->states_usage[i].disable)
- continue;
-
- limit = (u64)drv->states[i].target_residency * NSEC_PER_USEC;
- break;
- }
+ limit = cpuidle_poll_time(drv, dev);
while (!need_resched()) {
cpu_relax();
Index: linux-2.6-newcpuidle.git/include/linux/cpuidle.h
===================================================================
--- linux-2.6-newcpuidle.git.orig/include/linux/cpuidle.h
+++ linux-2.6-newcpuidle.git/include/linux/cpuidle.h
@@ -86,6 +86,7 @@ struct cpuidle_device {
ktime_t next_hrtimer;
int last_residency;
+ u64 poll_limit_ns;
struct cpuidle_state_usage states_usage[CPUIDLE_STATE_MAX];
struct cpuidle_state_kobj *kobjs[CPUIDLE_STATE_MAX];
struct cpuidle_driver_kobj *kobj_driver;
@@ -132,6 +133,8 @@ extern int cpuidle_select(struct cpuidle
extern int cpuidle_enter(struct cpuidle_driver *drv,
struct cpuidle_device *dev, int index);
extern void cpuidle_reflect(struct cpuidle_device *dev, int index);
+extern u64 cpuidle_poll_time(struct cpuidle_driver *drv,
+ struct cpuidle_device *dev);
extern int cpuidle_register_driver(struct cpuidle_driver *drv);
extern struct cpuidle_driver *cpuidle_get_driver(void);
@@ -166,6 +169,9 @@ static inline int cpuidle_enter(struct c
struct cpuidle_device *dev, int index)
{return -ENODEV; }
static inline void cpuidle_reflect(struct cpuidle_device *dev, int index) { }
+extern u64 cpuidle_poll_time(struct cpuidle_driver *drv,
+ struct cpuidle_device *dev)
+{return 0; }
static inline int cpuidle_register_driver(struct cpuidle_driver *drv)
{return -ENODEV; }
static inline struct cpuidle_driver *cpuidle_get_driver(void) {return NULL; }
Index: linux-2.6-newcpuidle.git/drivers/cpuidle/sysfs.c
===================================================================
--- linux-2.6-newcpuidle.git.orig/drivers/cpuidle/sysfs.c
+++ linux-2.6-newcpuidle.git/drivers/cpuidle/sysfs.c
@@ -334,6 +334,7 @@ struct cpuidle_state_kobj {
struct cpuidle_state_usage *state_usage;
struct completion kobj_unregister;
struct kobject kobj;
+ struct cpuidle_device *device;
};
#ifdef CONFIG_SUSPEND
@@ -391,6 +392,7 @@ static inline void cpuidle_remove_s2idle
#define kobj_to_state_obj(k) container_of(k, struct cpuidle_state_kobj, kobj)
#define kobj_to_state(k) (kobj_to_state_obj(k)->state)
#define kobj_to_state_usage(k) (kobj_to_state_obj(k)->state_usage)
+#define kobj_to_device(k) (kobj_to_state_obj(k)->device)
#define attr_to_stateattr(a) container_of(a, struct cpuidle_state_attr, attr)
static ssize_t cpuidle_state_show(struct kobject *kobj, struct attribute *attr,
@@ -414,10 +416,14 @@ static ssize_t cpuidle_state_store(struc
struct cpuidle_state *state = kobj_to_state(kobj);
struct cpuidle_state_usage *state_usage = kobj_to_state_usage(kobj);
struct cpuidle_state_attr *cattr = attr_to_stateattr(attr);
+ struct cpuidle_device *dev = kobj_to_device(kobj);
if (cattr->store)
ret = cattr->store(state, state_usage, buf, size);
+ /* reset poll time cache */
+ dev->poll_limit_ns = 0;
+
return ret;
}
@@ -468,6 +474,7 @@ static int cpuidle_add_state_sysfs(struc
}
kobj->state = &drv->states[i];
kobj->state_usage = &device->states_usage[i];
+ kobj->device = device;
init_completion(&kobj->kobj_unregister);
ret = kobject_init_and_add(&kobj->kobj, &ktype_state_cpuidle,
^ permalink raw reply
* [patch 0/5] cpuidle haltpoll driver and governor (v6)
From: Marcelo Tosatti @ 2019-07-03 23:51 UTC (permalink / raw)
To: kvm-devel
Cc: Paolo Bonzini, Radim Krcmar, Andrea Arcangeli, Rafael J. Wysocki,
Peter Zijlstra, Wanpeng Li, Konrad Rzeszutek Wilk,
Raslan KarimAllah, Boris Ostrovsky, Ankur Arora,
Christian Borntraeger, linux-pm
(rebased against queue branch of kvm.git tree)
The cpuidle-haltpoll driver with haltpoll governor allows the guest
vcpus to poll for a specified amount of time before halting.
This provides the following benefits to host side polling:
1) The POLL flag is set while polling is performed, which allows
a remote vCPU to avoid sending an IPI (and the associated
cost of handling the IPI) when performing a wakeup.
2) The VM-exit cost can be avoided.
The downside of guest side polling is that polling is performed
even with other runnable tasks in the host.
Results comparing halt_poll_ns and server/client application
where a small packet is ping-ponged:
host --> 31.33
halt_poll_ns=300000 / no guest busy spin --> 33.40 (93.8%)
halt_poll_ns=0 / guest_halt_poll_ns=300000 --> 32.73 (95.7%)
For the SAP HANA benchmarks (where idle_spin is a parameter
of the previous version of the patch, results should be the
same):
hpns == halt_poll_ns
idle_spin=0/ idle_spin=800/ idle_spin=0/
hpns=200000 hpns=0 hpns=800000
DeleteC06T03 (100 thread) 1.76 1.71 (-3%) 1.78 (+1%)
InsertC16T02 (100 thread) 2.14 2.07 (-3%) 2.18 (+1.8%)
DeleteC00T01 (1 thread) 1.34 1.28 (-4.5%) 1.29 (-3.7%)
UpdateC00T03 (1 thread) 4.72 4.18 (-12%) 4.53 (-5%)
V2:
- Move from x86 to generic code (Paolo/Christian)
- Add auto-tuning logic (Paolo)
- Add MSR to disable host side polling (Paolo)
V3:
- Do not be specific about HLT VM-exit in the documentation (Ankur Arora)
- Mark tuning parameters static and __read_mostly (Andrea Arcangeli)
- Add WARN_ON if host does not support poll control (Joao Martins)
- Use sched_clock and cleanup haltpoll_enter_idle (Peter Zijlstra)
- Mark certain functions in kvm.c as static (kernel test robot)
- Remove tracepoints as they use RCU from extended quiescent state (kernel
test robot)
V4:
- Use a haltpoll governor, use poll_state.c poll code (Rafael J. Wysocki)
V5:
- Take latency requirement into consideration (Rafael J. Wysocki)
- Set target_residency/exit_latency to 1 (Rafael J. Wysocki)
- Do not load cpuidle driver if not virtualized (Rafael J. Wysocki)
V6:
- Switch from callback to poll_limit_ns variable in cpuidle device structure
(Rafael J. Wysocki)
- Move last_used_idx to cpuidle device structure (Rafael J. Wysocki)
- Drop per-cpu device structure in haltpoll governor (Rafael J. Wysocki)
^ permalink raw reply
* Re: [RFCv2 0/8] Add imx8mm bus frequency switching
From: Leonard Crestez @ 2019-07-03 23:30 UTC (permalink / raw)
To: Saravana Kannan, Viresh Kumar
Cc: Alexandre Bailon, Georgi Djakov, Stephen Boyd, Michael Turquette,
MyungJoo Ham, Kyungmin Park, Shawn Guo, Aisheng Dong,
Fabio Estevam, Rafael J. Wysocki, Jacky Bai, Anson Huang,
Abel Vesa, Krzysztof Kozlowski, Ulf Hansson,
kernel@pengutronix.de, dl-linux-imx, Linux PM,
linux-clk@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
Vincent Guittot
In-Reply-To: <CAGETcx_63KnP75qySbhX_P_=o4=ox9J8AsBqKsFHeQRjCpSeJA@mail.gmail.com>
On 7/4/2019 1:20 AM, Saravana Kannan wrote:
>> The interconnect and devfreq parts do not communicate explicitly: they both
>> just call clk_set_min_rate and the clk core picks the minimum value that can
>> satisfy both. They are thus completely independent.
>
> Two different parts not talking to each other and just setting min
> rate can cause problems for some concurrency use cases. ICC framework
> is explicitly designed to handle cases like this and aggregate the
> needs correctly. You might want to look into that more closely.
The clk framework aggregates the min_rate requests from multiple
consumers under a big "prepare_lock" so I expect it will deal with
concurrent requests correctly. As for performance: frequency switching
shouldn't be a fast path.
>> The clk_set_min_rate approach does not mesh very well with the OPP framework.
>> Some of interconnect nodes on imx8m can run at different voltages: OPP can
>> handle this well but not in response to a clk_set_min_rate from an unrelated
>> subsystem. Maybe set voltage on a clk notifier?
>
> I think if you design it something like below, it might make your life
> a whole lot easier.
> Hopefully the formatting looks okay on your end. The arrow going up is
> just connecting devfreq to ICC.
>
> Proactive -> ICC -> clk/OPP API to set freq/volt
> ^
> |
> HW measure -> governor -> devfreq ----+
>
> That way, all voltage/frequency requirements are managed cleanly by
> clk/OPP frameworks. The ICC deals with aggregating all the
> requirements and devfreq lets you handle reactive scaling and policy.
If icc and devfreq are to directly communicate it makes more sense to do
it backwards: ICC should set a min_freq on nodes which have a devfreq
instance attached and devfreq should implement actual freq switching.
HW measurement is done on individual nodes while ICC deals with requests
along a path. In particular on imx we have a performance monitor
attached to the ddr controller and I doubt it can distinguish between
masters so how could this be mapped usefully to an interconnect request?
As far as I understand with devfreq the ddrc node could use "ondemand"
while the other nodes would default to the "passive" governor and run at
predefined default ratios relative to DDRC.
> If all of this makes sense, please take a look at [2] and provide your
> thoughts. I've dropped a few patches from [1] to avoid confusion (too
> much going on at once). I think BW OPP tables and having OPP tables
> for interconnect paths will be something you'll need (if not now,
> eventually) and something you can build on top of nicely.
I found it very confusing that you're assigning BW OPP tables to
devices. My initial understanding was that BW OPP would map "bandwidth"
to "frequency" so BW OPPs should be assigned to links along the
interconnect graph. But maybe what you want is to have OPPs for the BW
values requested by devices?
Looking at the sdm845 icc provider source and it seems that those
"peak/avg" values are actually parameters which go into a firmware
command!? It makes sense that you want interconnect to be "below"
devfreq since icc_provider.set maps very closely to what firmware exposes.
> Interconnects and interconnect paths quantify their performance
levels > in terms of bandwidth and not in terms of frequency.
On i.MX we just have a bunch of interconnect IPs for which frequencies
can be adjusted (in hz) so the above statement doesn't really hold. It
is up to an icc provider to convert aggregate bandwidth values to
frequencies along the path.
--
Regards,
Leonard
^ permalink raw reply
* [PATCH] PM: sleep: Drop dev_pm_skip_next_resume_phases()
From: Rafael J. Wysocki @ 2019-07-03 23:05 UTC (permalink / raw)
To: Linux PM; +Cc: LKML, Mika Westerberg
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
After recent hibernation-related changes, there are no more callers
of dev_pm_skip_next_resume_phases() except for the PM core itself
in which it is more straightforward to run the statements from
that function directly, so do that and drop it.
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
On top of the patch series at:
https://lore.kernel.org/linux-acpi/20190701162017.GB2640@lahna.fi.intel.com/T/#madf00de2d5a9b67e3c7bf51e882bd66ed7efc7ea
---
drivers/base/power/main.c | 19 +++----------------
include/linux/pm.h | 1 -
2 files changed, 3 insertions(+), 17 deletions(-)
Index: linux-pm/drivers/base/power/main.c
===================================================================
--- linux-pm.orig/drivers/base/power/main.c
+++ linux-pm/drivers/base/power/main.c
@@ -530,21 +530,6 @@ static void dpm_watchdog_clear(struct dp
/*------------------------- Resume routines -------------------------*/
/**
- * dev_pm_skip_next_resume_phases - Skip next system resume phases for device.
- * @dev: Target device.
- *
- * Make the core skip the "early resume" and "resume" phases for @dev.
- *
- * This function can be called by middle-layer code during the "noirq" phase of
- * system resume if necessary, but not by device drivers.
- */
-void dev_pm_skip_next_resume_phases(struct device *dev)
-{
- dev->power.is_late_suspended = false;
- dev->power.is_suspended = false;
-}
-
-/**
* suspend_event - Return a "suspend" message for given "resume" one.
* @resume_msg: PM message representing a system-wide resume transition.
*/
@@ -681,6 +666,9 @@ Skip:
dev->power.is_noirq_suspended = false;
if (skip_resume) {
+ /* Make the next phases of resume skip the device. */
+ dev->power.is_late_suspended = false;
+ dev->power.is_suspended = false;
/*
* The device is going to be left in suspend, but it might not
* have been in runtime suspend before the system suspended, so
@@ -689,7 +677,6 @@ Skip:
* device again.
*/
pm_runtime_set_suspended(dev);
- dev_pm_skip_next_resume_phases(dev);
}
Out:
Index: linux-pm/include/linux/pm.h
===================================================================
--- linux-pm.orig/include/linux/pm.h
+++ linux-pm/include/linux/pm.h
@@ -760,7 +760,6 @@ extern int pm_generic_poweroff_late(stru
extern int pm_generic_poweroff(struct device *dev);
extern void pm_generic_complete(struct device *dev);
-extern void dev_pm_skip_next_resume_phases(struct device *dev);
extern bool dev_pm_may_skip_resume(struct device *dev);
extern bool dev_pm_smart_suspend_and_suspended(struct device *dev);
^ permalink raw reply
* [PATCH] ACPI: PM: Unexport acpi_device_get_power()
From: Rafael J. Wysocki @ 2019-07-03 23:02 UTC (permalink / raw)
To: Linux ACPI; +Cc: Linux PM, LKML, Mika Westerberg
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Using acpi_device_get_power() outside of ACPI device initialization
and ACPI sysfs is problematic due to the way in which power resources
are handled by it, so unexport it and add a paragraph explaining the
pitfalls to its kerneldoc comment.
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
On top of the linux-next branch in the linux-pm.git tree.
---
drivers/acpi/device_pm.c | 6 +++++-
drivers/acpi/internal.h | 7 +++++++
include/acpi/acpi_bus.h | 1 -
3 files changed, 12 insertions(+), 2 deletions(-)
Index: linux-pm/drivers/acpi/device_pm.c
===================================================================
--- linux-pm.orig/drivers/acpi/device_pm.c
+++ linux-pm/drivers/acpi/device_pm.c
@@ -66,6 +66,11 @@ static int acpi_dev_pm_explicit_get(stru
* This function does not update the device's power.state field, but it may
* update its parent's power.state field (when the parent's power state is
* unknown and the device's power state turns out to be D0).
+ *
+ * Also, it does not update power resource reference counters to ensure that
+ * the power state returned by it will be persistent and it may return a power
+ * state shallower than previously set by acpi_device_set_power() for @device
+ * (if that power state depends on any power resources).
*/
int acpi_device_get_power(struct acpi_device *device, int *state)
{
@@ -130,7 +135,6 @@ int acpi_device_get_power(struct acpi_de
return 0;
}
-EXPORT_SYMBOL(acpi_device_get_power);
static int acpi_dev_pm_explicit_set(struct acpi_device *adev, int state)
{
Index: linux-pm/include/acpi/acpi_bus.h
===================================================================
--- linux-pm.orig/include/acpi/acpi_bus.h
+++ linux-pm/include/acpi/acpi_bus.h
@@ -506,7 +506,6 @@ int acpi_bus_get_status(struct acpi_devi
int acpi_bus_set_power(acpi_handle handle, int state);
const char *acpi_power_state_string(int state);
-int acpi_device_get_power(struct acpi_device *device, int *state);
int acpi_device_set_power(struct acpi_device *device, int state);
int acpi_bus_init_power(struct acpi_device *device);
int acpi_device_fix_up_power(struct acpi_device *device);
Index: linux-pm/drivers/acpi/internal.h
===================================================================
--- linux-pm.orig/drivers/acpi/internal.h
+++ linux-pm/drivers/acpi/internal.h
@@ -139,8 +139,15 @@ int acpi_power_get_inferred_state(struct
int acpi_power_on_resources(struct acpi_device *device, int state);
int acpi_power_transition(struct acpi_device *device, int state);
+/* --------------------------------------------------------------------------
+ Device Power Management
+ -------------------------------------------------------------------------- */
+int acpi_device_get_power(struct acpi_device *device, int *state);
int acpi_wakeup_device_init(void);
+/* --------------------------------------------------------------------------
+ Processor
+ -------------------------------------------------------------------------- */
#ifdef CONFIG_ARCH_MIGHT_HAVE_ACPI_PDC
void acpi_early_processor_set_pdc(void);
#else
^ permalink raw reply
* [PATCH] kernel/cpu: hotplug; fix non-SYSFS build errors in arch/x86/power/
From: Randy Dunlap @ 2019-07-03 22:27 UTC (permalink / raw)
To: LKML, Linux PM list, Thomas Gleixner, Rafael J. Wysocki
From: Randy Dunlap <rdunlap@infradead.org>
Fix link errors when building almost-allmodconfig but CONFIG_SYSFS
is not set/enabled.
The missing functions should not be inside #if CONFIG_SYSFS/#endif.
The non-SYSFS stub for __store_smt_control() is no longer needed.
This is almost all code movement and a little ifdef-fery changes.
Fixes these build errors:
ld: arch/x86/power/cpu.o: in function `hibernate_resume_nonboot_cpu_disable':
cpu.c:(.text+0x9f4): undefined reference to `cpuhp_smt_enable'
ld: arch/x86/power/hibernate.o: in function `arch_resume_nosmt':
hibernate.c:(.text+0x7f7): undefined reference to `cpuhp_smt_enable'
ld: hibernate.c:(.text+0x809): undefined reference to `cpuhp_smt_disable'
Fixes: 98f8cdce1db5 ("cpu/hotplug: Add sysfs state interface")
Signed-off-by: Randy Dunlap <rdunlap@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: linux-pm@vger.kernel.org
---
kernel/cpu.c | 242 +++++++++++++++++++++++--------------------------
1 file changed, 118 insertions(+), 124 deletions(-)
--- lnx-52-rc7.orig/kernel/cpu.c
+++ lnx-52-rc7/kernel/cpu.c
@@ -376,7 +376,7 @@ static void lockdep_release_cpus_lock(vo
{
}
-#endif /* CONFIG_HOTPLUG_CPU */
+#endif /* CONFIG_HOTPLUG_CPU */
/*
* Architectures that need SMT-specific errata handling during SMT hotplug
@@ -1892,6 +1892,123 @@ void __cpuhp_remove_state(enum cpuhp_sta
}
EXPORT_SYMBOL(__cpuhp_remove_state);
+#ifdef CONFIG_HOTPLUG_CPU
+
+static void cpuhp_offline_cpu_device(unsigned int cpu)
+{
+ struct device *dev = get_cpu_device(cpu);
+
+ dev->offline = true;
+ /* Tell user space about the state change */
+ kobject_uevent(&dev->kobj, KOBJ_OFFLINE);
+}
+
+static void cpuhp_online_cpu_device(unsigned int cpu)
+{
+ struct device *dev = get_cpu_device(cpu);
+
+ dev->offline = false;
+ /* Tell user space about the state change */
+ kobject_uevent(&dev->kobj, KOBJ_ONLINE);
+}
+
+int cpuhp_smt_disable(enum cpuhp_smt_control ctrlval)
+{
+ int cpu, ret = 0;
+
+ cpu_maps_update_begin();
+ for_each_online_cpu(cpu) {
+ if (topology_is_primary_thread(cpu))
+ continue;
+ ret = cpu_down_maps_locked(cpu, CPUHP_OFFLINE);
+ if (ret)
+ break;
+ /*
+ * As this needs to hold the cpu maps lock it's impossible
+ * to call device_offline() because that ends up calling
+ * cpu_down() which takes cpu maps lock. cpu maps lock
+ * needs to be held as this might race against in kernel
+ * abusers of the hotplug machinery (thermal management).
+ *
+ * So nothing would update device:offline state. That would
+ * leave the sysfs entry stale and prevent onlining after
+ * smt control has been changed to 'off' again. This is
+ * called under the sysfs hotplug lock, so it is properly
+ * serialized against the regular offline usage.
+ */
+ cpuhp_offline_cpu_device(cpu);
+ }
+ if (!ret)
+ cpu_smt_control = ctrlval;
+ cpu_maps_update_done();
+ return ret;
+}
+
+int cpuhp_smt_enable(void)
+{
+ int cpu, ret = 0;
+
+ cpu_maps_update_begin();
+ cpu_smt_control = CPU_SMT_ENABLED;
+ for_each_present_cpu(cpu) {
+ /* Skip online CPUs and CPUs on offline nodes */
+ if (cpu_online(cpu) || !node_online(cpu_to_node(cpu)))
+ continue;
+ ret = _cpu_up(cpu, 0, CPUHP_ONLINE);
+ if (ret)
+ break;
+ /* See comment in cpuhp_smt_disable() */
+ cpuhp_online_cpu_device(cpu);
+ }
+ cpu_maps_update_done();
+ return ret;
+}
+
+#if defined(CONFIG_HOTPLUG_SMT) && defined(CONFIG_SYSFS)
+static ssize_t
+__store_smt_control(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ int ctrlval, ret;
+
+ if (sysfs_streq(buf, "on"))
+ ctrlval = CPU_SMT_ENABLED;
+ else if (sysfs_streq(buf, "off"))
+ ctrlval = CPU_SMT_DISABLED;
+ else if (sysfs_streq(buf, "forceoff"))
+ ctrlval = CPU_SMT_FORCE_DISABLED;
+ else
+ return -EINVAL;
+
+ if (cpu_smt_control == CPU_SMT_FORCE_DISABLED)
+ return -EPERM;
+
+ if (cpu_smt_control == CPU_SMT_NOT_SUPPORTED)
+ return -ENODEV;
+
+ ret = lock_device_hotplug_sysfs();
+ if (ret)
+ return ret;
+
+ if (ctrlval != cpu_smt_control) {
+ switch (ctrlval) {
+ case CPU_SMT_ENABLED:
+ ret = cpuhp_smt_enable();
+ break;
+ case CPU_SMT_DISABLED:
+ case CPU_SMT_FORCE_DISABLED:
+ ret = cpuhp_smt_disable(ctrlval);
+ break;
+ }
+ }
+
+ unlock_device_hotplug();
+ return ret ? ret : count;
+}
+#endif /* CONFIG_HOTPLUG_SMT && CONFIG_SYSFS */
+
+#endif /* CONFIG_HOTPLUG_CPU */
+
#if defined(CONFIG_SYSFS) && defined(CONFIG_HOTPLUG_CPU)
static ssize_t show_cpuhp_state(struct device *dev,
struct device_attribute *attr, char *buf)
@@ -2044,129 +2161,6 @@ static const struct attribute_group cpuh
NULL
};
-#ifdef CONFIG_HOTPLUG_SMT
-
-static void cpuhp_offline_cpu_device(unsigned int cpu)
-{
- struct device *dev = get_cpu_device(cpu);
-
- dev->offline = true;
- /* Tell user space about the state change */
- kobject_uevent(&dev->kobj, KOBJ_OFFLINE);
-}
-
-static void cpuhp_online_cpu_device(unsigned int cpu)
-{
- struct device *dev = get_cpu_device(cpu);
-
- dev->offline = false;
- /* Tell user space about the state change */
- kobject_uevent(&dev->kobj, KOBJ_ONLINE);
-}
-
-int cpuhp_smt_disable(enum cpuhp_smt_control ctrlval)
-{
- int cpu, ret = 0;
-
- cpu_maps_update_begin();
- for_each_online_cpu(cpu) {
- if (topology_is_primary_thread(cpu))
- continue;
- ret = cpu_down_maps_locked(cpu, CPUHP_OFFLINE);
- if (ret)
- break;
- /*
- * As this needs to hold the cpu maps lock it's impossible
- * to call device_offline() because that ends up calling
- * cpu_down() which takes cpu maps lock. cpu maps lock
- * needs to be held as this might race against in kernel
- * abusers of the hotplug machinery (thermal management).
- *
- * So nothing would update device:offline state. That would
- * leave the sysfs entry stale and prevent onlining after
- * smt control has been changed to 'off' again. This is
- * called under the sysfs hotplug lock, so it is properly
- * serialized against the regular offline usage.
- */
- cpuhp_offline_cpu_device(cpu);
- }
- if (!ret)
- cpu_smt_control = ctrlval;
- cpu_maps_update_done();
- return ret;
-}
-
-int cpuhp_smt_enable(void)
-{
- int cpu, ret = 0;
-
- cpu_maps_update_begin();
- cpu_smt_control = CPU_SMT_ENABLED;
- for_each_present_cpu(cpu) {
- /* Skip online CPUs and CPUs on offline nodes */
- if (cpu_online(cpu) || !node_online(cpu_to_node(cpu)))
- continue;
- ret = _cpu_up(cpu, 0, CPUHP_ONLINE);
- if (ret)
- break;
- /* See comment in cpuhp_smt_disable() */
- cpuhp_online_cpu_device(cpu);
- }
- cpu_maps_update_done();
- return ret;
-}
-
-
-static ssize_t
-__store_smt_control(struct device *dev, struct device_attribute *attr,
- const char *buf, size_t count)
-{
- int ctrlval, ret;
-
- if (sysfs_streq(buf, "on"))
- ctrlval = CPU_SMT_ENABLED;
- else if (sysfs_streq(buf, "off"))
- ctrlval = CPU_SMT_DISABLED;
- else if (sysfs_streq(buf, "forceoff"))
- ctrlval = CPU_SMT_FORCE_DISABLED;
- else
- return -EINVAL;
-
- if (cpu_smt_control == CPU_SMT_FORCE_DISABLED)
- return -EPERM;
-
- if (cpu_smt_control == CPU_SMT_NOT_SUPPORTED)
- return -ENODEV;
-
- ret = lock_device_hotplug_sysfs();
- if (ret)
- return ret;
-
- if (ctrlval != cpu_smt_control) {
- switch (ctrlval) {
- case CPU_SMT_ENABLED:
- ret = cpuhp_smt_enable();
- break;
- case CPU_SMT_DISABLED:
- case CPU_SMT_FORCE_DISABLED:
- ret = cpuhp_smt_disable(ctrlval);
- break;
- }
- }
-
- unlock_device_hotplug();
- return ret ? ret : count;
-}
-
-#else /* !CONFIG_HOTPLUG_SMT */
-static ssize_t
-__store_smt_control(struct device *dev, struct device_attribute *attr,
- const char *buf, size_t count)
-{
- return -ENODEV;
-}
-#endif /* CONFIG_HOTPLUG_SMT */
-
static const char *smt_states[] = {
[CPU_SMT_ENABLED] = "on",
[CPU_SMT_DISABLED] = "off",
^ permalink raw reply
* Re: [RFCv2 0/8] Add imx8mm bus frequency switching
From: Saravana Kannan @ 2019-07-03 22:19 UTC (permalink / raw)
To: Leonard Crestez
Cc: Alexandre Bailon, Georgi Djakov, Stephen Boyd, Michael Turquette,
Viresh Kumar, MyungJoo Ham, Kyungmin Park, Shawn Guo,
Dong Aisheng, Fabio Estevam, Rafael J. Wysocki, Jacky Bai,
Anson Huang, Abel Vesa, Krzysztof Kozlowski, Ulf Hansson, kernel,
linux-imx, Linux PM, linux-clk, linux-arm-kernel
In-Reply-To: <cover.1561707104.git.leonard.crestez@nxp.com>
Hi Leonard,
On Fri, Jun 28, 2019 at 12:40 AM Leonard Crestez
<leonard.crestez@nxp.com> wrote:
>
> This series attempts to add upstream DVFS support for imx8mm, covering dynamic
> scaling of internal buses and dram. It uses the interconnect framework for
> proactive scaling (in response to explicit bandwidth requests from devices) and
> devfreq in order expose the buses and eventually implement reactive scaling (in
> response to measuredtraffic).
I'm assuming you took a glance at my patch series [1] adding BW OPP
tables and adding devfreq support for scaling interconnect paths.
Based on looking at your patch series, I think [1] would allow you to
use ICC framework for both proactive and reactive scaling. Proactive
scaling would use the ICC framework directly. Reactive scaling would
go through devfreq (so that you can use various governors/adjust
policy) before it goes to ICC framework.
> Actual scaling is performed through the clk framework: The NOC and main NICs
> are driven by composite clks and a new 'imx8m-dram' clk is included for
> scaling dram using firmware calls.
Seems reasonable. All hardware block in the end run using a clock.
> The interconnect and devfreq parts do not communicate explicitly: they both
> just call clk_set_min_rate and the clk core picks the minimum value that can
> satisfy both. They are thus completely independent.
Two different parts not talking to each other and just setting min
rate can cause problems for some concurrency use cases. ICC framework
is explicitly designed to handle cases like this and aggregate the
needs correctly. You might want to look into that more closely.
> This is easily extensible to more members of the imx8m family, some of which
> expose more detailed controls over interconnect fabric frequencies.
>
> TODO:
> * Clarify DT bindings
> * Clarify interconnect OPP picking logic
> * Implement devfreq_event for imx8m ddrc
> * Expose more dram frequencies
>
> The clk_set_min_rate approach does not mesh very well with the OPP framework.
> Some of interconnect nodes on imx8m can run at different voltages: OPP can
> handle this well but not in response to a clk_set_min_rate from an unrelated
> subsystem. Maybe set voltage on a clk notifier?
I think if you design it something like below, it might make your life
a whole lot easier.
Hopefully the formatting looks okay on your end. The arrow going up is
just connecting devfreq to ICC.
Proactive -> ICC -> clk/OPP API to set freq/volt
^
|
HW measure -> governor -> devfreq ----+
That way, all voltage/frequency requirements are managed cleanly by
clk/OPP frameworks. The ICC deals with aggregating all the
requirements and devfreq lets you handle reactive scaling and policy.
And in the beginning, while you get a governor going, you can use
"performance" governor for the "reactive scaling" users. That way,
your reactive paths will continue to have good performance while the
rest of the "proactive" clients change to use ICC APIs.
> Vendor tree does not support voltage switching, independent freqs for
> different parts of the fabric or any reactive scaling. I think it's important
> to pick an upstreaming approach which can support as much as possible.
>
> Feedback welcome.
If all of this makes sense, please take a look at [2] and provide your
thoughts. I've dropped a few patches from [1] to avoid confusion (too
much going on at once). I think BW OPP tables and having OPP tables
for interconnect paths will be something you'll need (if not now,
eventually) and something you can build on top of nicely.
Thanks,
Saravana
[1] - https://lore.kernel.org/lkml/20190614041733.120807-1-saravanak@google.com/
[2] - https://lore.kernel.org/lkml/20190703011020.151615-1-saravanak@google.com/
> Some objections were apparently raised to doing DRAM switch inside CLK:
> perhaps ICC should make min_freq requests to devfreq instead?
>
> Link to v1 (multiple chunks):
> * https://patchwork.kernel.org/patch/10976897/
> * https://patchwork.kernel.org/patch/10968303/
> * https://patchwork.kernel.org/project/linux-arm-kernel/list/?series=91251
>
> Also as a github branch (with few other changes):
> https://github.com/cdleonard/linux/tree/next_imx8mm_busfreq
>
> Alexandre Bailon (2):
> interconnect: Add generic driver for imx
> interconnect: imx: Add platform driver for imx8mm
>
> Leonard Crestez (6):
> clk: imx8mm: Add dram freq switch support
> clk: imx8m-composite: Switch to determine_rate
> arm64: dts: imx8mm: Add dram dvfs irqs to ccm node
> devfreq: Add imx-devfreq driver
> arm64: dts: imx8mm: Add interconnect node
> arm64: dts: imx8mm: Add devfreq-imx nodes
>
> arch/arm64/boot/dts/freescale/imx8mm.dtsi | 73 +++
> drivers/clk/imx/Makefile | 1 +
> drivers/clk/imx/clk-composite-8m.c | 34 +-
> drivers/clk/imx/clk-imx8m-dram.c | 357 ++++++++++++
> drivers/clk/imx/clk-imx8mm.c | 12 +
> drivers/clk/imx/clk.h | 13 +
> drivers/devfreq/Kconfig | 10 +
> drivers/devfreq/Makefile | 1 +
> drivers/devfreq/imx-devfreq.c | 142 +++++
> drivers/interconnect/Kconfig | 1 +
> drivers/interconnect/Makefile | 1 +
> drivers/interconnect/imx/Kconfig | 17 +
> drivers/interconnect/imx/Makefile | 2 +
> drivers/interconnect/imx/busfreq-imx8mm.c | 151 ++++++
> drivers/interconnect/imx/busfreq.c | 628 ++++++++++++++++++++++
> drivers/interconnect/imx/busfreq.h | 123 +++++
> include/dt-bindings/clock/imx8mm-clock.h | 4 +-
> include/dt-bindings/interconnect/imx8mm.h | 49 ++
> 18 files changed, 1606 insertions(+), 13 deletions(-)
> create mode 100644 drivers/clk/imx/clk-imx8m-dram.c
> create mode 100644 drivers/devfreq/imx-devfreq.c
> create mode 100644 drivers/interconnect/imx/Kconfig
> create mode 100644 drivers/interconnect/imx/Makefile
> create mode 100644 drivers/interconnect/imx/busfreq-imx8mm.c
> create mode 100644 drivers/interconnect/imx/busfreq.c
> create mode 100644 drivers/interconnect/imx/busfreq.h
> create mode 100644 include/dt-bindings/interconnect/imx8mm.h
>
> --
> 2.17.1
>
^ permalink raw reply
* Re: [PATCH v3 6/6] interconnect: Add OPP table support for interconnects
From: Saravana Kannan @ 2019-07-03 21:33 UTC (permalink / raw)
To: Vincent Guittot
Cc: Georgi Djakov, Rob Herring, Mark Rutland, Viresh Kumar,
Nishanth Menon, Stephen Boyd, Rafael J. Wysocki, Sweeney, Sean,
daidavid1, Rajendra Nayak, sibis, Bjorn Andersson, Evan Green,
Android Kernel Team, open list:THERMAL,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
linux-kernel
In-Reply-To: <CAKfTPtCJFaEfvu3Dnp9WSxQEwSfY=VS+xsoQ+4P+vg7_WL0BAQ@mail.gmail.com>
On Tue, Jul 2, 2019 at 11:45 PM Vincent Guittot
<vincent.guittot@linaro.org> wrote:
>
> On Wed, 3 Jul 2019 at 03:10, Saravana Kannan <saravanak@google.com> wrote:
> >
> > Interconnect paths can have different performance points. Now that OPP
> > framework supports bandwidth OPP tables, add OPP table support for
> > interconnects.
> >
> > Devices can use the interconnect-opp-table DT property to specify OPP
> > tables for interconnect paths. And the driver can obtain the OPP table for
> > an interconnect path by calling icc_get_opp_table().
>
> The opp table of a path must come from the aggregation of OPP tables
> of the interconnect providers.
The aggregation of OPP tables of the providers is certainly the
superset of what a path can achieve, but to say that OPPs for
interconnect path should match that superset is an oversimplification
of the reality in hardware.
There are lots of reasons an interconnect path might not want to use
all the available bandwidth options across all the interconnects in
the route.
1. That particular path might not have been validated or verified
during the HW design process for some of the frequencies/bandwidth
combinations of the providers.
2. Similarly during parts screening in the factory, some of the
combinations might not have been screened and can't be guaranteed
to work.
3. Only a certain set of bandwidth levels might make sense to use from
a power/performance balance given the device using it. For example:
- The big CPU might not want to use some of the lower bandwidths
but the little CPU might want to.
- The big CPU might not want to use some intermediate bandwidth
points if they don't save a lot of power compared to a higher
bandwidth levels, but the little CPU might want to.
- The little CPU might never want to use the higher set of
bandwidth levels since they won't be power efficient for the use
cases that might run on it.
4. It might not make sense from a system level power perspective.
Let's take an example of a path S (source) -> A -> B -> C -> D
(destination).
- A supports only 2, 5, 7 and 10 GB/s. B supports 1, 2 ... 10 GB/s.
C supports 5 and 10 GB/s
- If you combine and list the superset of bandwidth levels
supported in that path, that'd be 1, 2, 3, ... 10 GB/s.
- Which set of bandwidth levels make sense will depend on the
hardware characteristics of the interconnects.
- If B is the biggest power sink, then you might want to use all 10
levels.
- If A is the biggest power sink, then you might want to use all 2,
5 and 10 GB/s of the levels.
- If C is the biggest power sink then you might only want to use 5
and 10 GB/s
- The more hops and paths you get the more convoluted this gets.
5. The design of the interconnects themselves might have an impact on
which bandwidth levels are used.
- For example, the FIFO depth between two specific interconnects
might affect the valid bandwidth levels for a specific path.
- Say S1 -> A -> B -> D1, S2 -> C -> B -> D1 and S2 -> C -> D2 are
three paths.
- If C <-> B FIFO depth is small, then there might be a requirement
that C and B be closely performance matched to avoid system level
congestion due to back pressure.
- So S2 -> D1 path can't use all the bandwidth levels supported by
C-B combination.
- But S2 -> D2 can use all the bandwidth levels supported by C.
- And S1 -> D1 can use all the levels supported by A-B combination.
These are just some of the reasons I could recollect in a few minutes.
These are all real world cases I had to deal with in the past several
years of dealing with scaling interconnects. I'm sure vendors and SoCs
I'm not familiar with have other good reasons I'm not aware of.
Trying to figure this all out by aggregating OPP tables of
interconnect providers just isn't feasible nor is it efficient. The
OPP tables for an interconnect path is describing the valid BW levels
supported by that path and verified in hardware and makes a lot of
sense to capture it clearly in DT.
> So such kind of OPP table should be at
> provider level but not at path level.
They can also use it if they want to, but they'll probably want to use
a frequency OPP table.
-Saravana
>
> >
> > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > ---
> > drivers/interconnect/core.c | 27 ++++++++++++++++++++++++++-
> > include/linux/interconnect.h | 7 +++++++
> > 2 files changed, 33 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/interconnect/core.c b/drivers/interconnect/core.c
> > index 871eb4bc4efc..881bac80bc1e 100644
> > --- a/drivers/interconnect/core.c
> > +++ b/drivers/interconnect/core.c
> > @@ -47,6 +47,7 @@ struct icc_req {
> > */
> > struct icc_path {
> > size_t num_nodes;
> > + struct opp_table *opp_table;
> > struct icc_req reqs[];
> > };
> >
> > @@ -313,7 +314,7 @@ struct icc_path *of_icc_get(struct device *dev, const char *name)
> > {
> > struct icc_path *path = ERR_PTR(-EPROBE_DEFER);
> > struct icc_node *src_node, *dst_node;
> > - struct device_node *np = NULL;
> > + struct device_node *np = NULL, *opp_node;
> > struct of_phandle_args src_args, dst_args;
> > int idx = 0;
> > int ret;
> > @@ -381,10 +382,34 @@ struct icc_path *of_icc_get(struct device *dev, const char *name)
> > dev_err(dev, "%s: invalid path=%ld\n", __func__, PTR_ERR(path));
> > mutex_unlock(&icc_lock);
> >
> > + opp_node = of_parse_phandle(np, "interconnect-opp-table", idx);
> > + if (opp_node) {
> > + path->opp_table = dev_pm_opp_of_find_table_from_node(opp_node);
> > + of_node_put(opp_node);
> > + }
> > +
> > +
> > return path;
> > }
> > EXPORT_SYMBOL_GPL(of_icc_get);
> >
> > +/**
> > + * icc_get_opp_table() - Get the OPP table that corresponds to a path
> > + * @path: reference to the path returned by icc_get()
> > + *
> > + * This function will return the OPP table that corresponds to a path handle.
> > + * If the interconnect API is disabled, NULL is returned and the consumer
> > + * drivers will still build. Drivers are free to handle this specifically, but
> > + * they don't have to.
> > + *
> > + * Return: opp_table pointer on success. NULL is returned when the API is
> > + * disabled or the OPP table is missing.
> > + */
> > +struct opp_table *icc_get_opp_table(struct icc_path *path)
> > +{
> > + return path->opp_table;
> > +}
> > +
> > /**
> > * icc_set_bw() - set bandwidth constraints on an interconnect path
> > * @path: reference to the path returned by icc_get()
> > diff --git a/include/linux/interconnect.h b/include/linux/interconnect.h
> > index dc25864755ba..0c0bc55f0e89 100644
> > --- a/include/linux/interconnect.h
> > +++ b/include/linux/interconnect.h
> > @@ -9,6 +9,7 @@
> >
> > #include <linux/mutex.h>
> > #include <linux/types.h>
> > +#include <linux/pm_opp.h>
> >
> > /* macros for converting to icc units */
> > #define Bps_to_icc(x) ((x) / 1000)
> > @@ -28,6 +29,7 @@ struct device;
> > struct icc_path *icc_get(struct device *dev, const int src_id,
> > const int dst_id);
> > struct icc_path *of_icc_get(struct device *dev, const char *name);
> > +struct opp_table *icc_get_opp_table(struct icc_path *path);
> > void icc_put(struct icc_path *path);
> > int icc_set_bw(struct icc_path *path, u32 avg_bw, u32 peak_bw);
> >
> > @@ -49,6 +51,11 @@ static inline void icc_put(struct icc_path *path)
> > {
> > }
> >
> > +static inline struct opp_table *icc_get_opp_table(struct icc_path *path)
> > +{
> > + return NULL;
> > +}
> > +
> > static inline int icc_set_bw(struct icc_path *path, u32 avg_bw, u32 peak_bw)
> > {
> > return 0;
> > --
> > 2.22.0.410.gd8fdbe21b5-goog
> >
^ permalink raw reply
* Re: Thermal microconference proposal @ LPC
From: Pandruvada, Srinivas @ 2019-07-03 20:45 UTC (permalink / raw)
To: linux-pm@vger.kernel.org, amit.kucheria@linaro.org
Cc: Zhang, Rui, edubezval@gmail.com, daniel.lezcano@linaro.org
In-Reply-To: <CAP245DVGekWAS1O6ZbNi52K5LyQw8vqVXM6DSBvOieAT3v1fpQ@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1652 bytes --]
Hi Amit,
On Mon, 2019-06-24 at 19:22 +0530, Amit Kucheria wrote:
> Hi all,
>
> We've proposed a thermal microconf at LPC again this year. I've
> bcc'ed
> a few people who were interested last year. We already have the
> following topics listed for the proposal but could certainly use more
> topics especially from the non-mobile Linux community.
>
Thermal/Power mitigation on high powered laptops
Some of the high powered laptops released with KabyLake and later
generations of processors can reach more than 50W with some busy
workloads like kernel compilation. This results in CPU temperature to
quickly reach close to the critical temperature. To avoid this OEM’s
have used a very conservative power limits by default. But this
results in a bad performance on Linux laptops compared to other
operating systems. We have implemented a solution for Linux to mitigate
this. This quick presentation will show the approach Linux users
particularly kernel developers can use to get a very high performance
from these laptops.
This should be a quick 5-10 min talk.
Thanks,
Srinivas
> - Samsung: Thermal aware scheduling (Lukasz Luba)
> - Linaro: Combining idle and freq change for cpu cooling (Daniel
> Lezcano)
> - ARM: Discriminating thermal throttling (Volker Eckert)
> - IBM: Sustain turbo mode (Parth Shah)
> - QCOM/Linaro: Make thermal framework easier to use in products (Amit
> Kucheria)
>
> Please help bring this to the attention of those who might be
> interested in talking about thermal-related problems at the
> conference.
>
> Thanks.
>
> Regards,
> Amit Kucheria & Daniel Lezcano
[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 3290 bytes --]
^ permalink raw reply
* Re: [PATCH v3 0/6] Introduce Bandwidth OPPs for interconnect paths
From: Saravana Kannan @ 2019-07-03 20:35 UTC (permalink / raw)
To: Viresh Kumar
Cc: Georgi Djakov, Rob Herring, Mark Rutland, Viresh Kumar,
Nishanth Menon, Stephen Boyd, Rafael J. Wysocki, Vincent Guittot,
seansw, daidavid1, Rajendra Nayak, sibis, Bjorn Andersson,
evgreen, Android Kernel Team, Linux PM, devicetree, LKML
In-Reply-To: <20190703063632.hl2lipcoeehplyxq@vireshk-i7>
On Tue, Jul 2, 2019 at 11:36 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 02-07-19, 18:10, Saravana Kannan wrote:
> > Interconnects and interconnect paths quantify their performance levels in
> > terms of bandwidth and not in terms of frequency. So similar to how we have
> > frequency based OPP tables in DT and in the OPP framework, we need
> > bandwidth OPP table support in the OPP framework and in DT. Since there can
> > be more than one interconnect path used by a device, we also need a way to
> > assign a bandwidth OPP table to an interconnect path.
> >
> > This patch series:
> > - Adds opp-peak-KBps and opp-avg-KBps properties to OPP DT bindings
> > - Adds interconnect-opp-table property to interconnect DT bindings
> > - Adds OPP helper functions for bandwidth OPP tables
> > - Adds icc_get_opp_table() to get the OPP table for an interconnect path
> >
> > So with the DT bindings added in this patch series, the DT for a GPU
> > that does bandwidth voting from GPU to Cache and GPU to DDR would look
> > something like this:
>
> And what changed since V2 ?
Sorry, forgot to put that in. I just dropped a lot of patches that
weren't relevant to the idea of BW OPPs and tying them to
interconnects.
-Saravana
^ permalink raw reply
* RE: [PATCH] cpuidle/drivers/mobile: Add new governor for mobile/embedded systems
From: Doug Smythies @ 2019-07-03 19:12 UTC (permalink / raw)
To: 'Daniel Lezcano'
Cc: linux-kernel, 'Rafael J. Wysocki',
'Thomas Gleixner', 'Greg Kroah-Hartman',
'open list:CPU IDLE TIME MANAGEMENT FRAMEWORK', rafael
In-Reply-To: <6589a058-c538-fbf3-7761-d43ab8434654@linaro.org>
On 2019.07.03 08:16 Daniel Lezcano wrote:
> On 03/07/2019 16:23, Doug Smythies wrote:
>> On 2019.06.20 04:58 Daniel Lezcano wrote:
...
>> Anyway, I did a bunch of tests and such, but have deleted
>> most from this e-mail, because it's just noise. I'll
>> include just one set:
>>
>> For a work load that would normally result in a lot of use
>> of shallow idle states (single core pipe-test * 2 cores).
>
> Can you share the tests and the command lines?
Yes, give me a few days to repeat the tests and write
it up properly. I am leaving town in an hour and for a day.
It'll be similar to this:
http://www.smythies.com/~doug/linux/idle/teo8/pipe/index.html
parent page (which I will do a better version):
http://www.smythies.com/~doug/linux/idle/teo8/index.html
...
>> I got (all kernel 5.2-rc5 + this patch):
>>
>> Idle governor, teo; CPU frequency scaling: intel-cpufreq/ondemand;
>> Processor package power: 40.4 watts; 4.9 uSec/loop
>>
>> Idle governor, teo; CPU frequency scaling: intel-cpufreq/ondemand;
>> Processor package power: 34 watts; 5.2 uSec/loop
>>
>> Idle governor, mobile; CPU frequency scaling: intel-cpufreq/ondemand;
>> Processor package power: 25.9 watts; 11.1 uSec/loop
>>
>> Idle governor, menu; CPU frequency scaling: intel-cpufreq/ondemand;
>> Processor package power: 34.2 watts; 5.23 uSec/loop
>>
>> Idle governor, teo; CPU frequency scaling: intel-cpufreq/ondemand;
>> Maximum CPU frequency limited to 73% to match mobile energy.
>> Processor package power: 25.4 watts; 6.4 uSec/loop
>
> Ok that's interesting. Thanks for the values.
>
> The governor can be better by selecting the shallow states, the
> scheduler has to interact with the governor to give clues about the
> load, that is identified and will be the next step.
>
> Is it possible to check with the schedutil governor instead?
Oh, I already have some data, just didn't include it before:
Idle governor, teo; CPU frequency scaling: intel-cpufreq/schedutil;
Processor package power: 40.4 watts; 4.9 uSec/loop
Idle governor, mobile; CPU frequency scaling: intel-cpufreq/schedutil;
Processor package power: 12.7 watts; 19.7 uSec/loop
Idle governor, teo; CPU frequency scaling: intel-cpufreq/schedutil;
Idle states 0-3 disabled (note: Idle state 4 is the deepest on my system)
Processor package power: 36.9 watts; 8.3 uSec/loop
In my notes I wrote: "Huh?? I do not understand this result, as I had
expected more similar to the mobile governor". But I did not investigate.
Anyway, the schedutil test is the one I'll repeat and write up better.
... Doug
^ permalink raw reply
* [PATCH v2 7/7] arm: dts: mt6323: add keys, power-controller, rtc and codec
From: Frank Wunderlich @ 2019-07-03 16:48 UTC (permalink / raw)
To: Lee Jones, Rob Herring, Mark Rutland, Matthias Brugger, Sean Wang,
Sebastian Reichel, Alessandro Zummo, Alexandre Belloni,
devicetree, linux-arm-kernel, linux-mediatek, linux-kernel,
linux-pm, linux-rtc, Eddie Huang, Thomas Gleixner,
Richard Fontana, Allison Randal, David S . Miller,
Mauro Carvalho Chehab, Greg Kroah-Hartman, Rob Herring,
Linus Walleij, Nicolas Ferre, Paul E . McKenney
Cc: Josef Friedl, Frank Wunderlich
In-Reply-To: <20190703164822.17924-1-frank-w@public-files.de>
From: Josef Friedl <josef.friedl@speed.at>
support poweroff and power-related keys on bpi-r2
Suggested-by: Frank Wunderlich <frank-w@public-files.de>
Signed-off-by: Josef Friedl <josef.friedl@speed.at>
Signed-off-by: Frank Wunderlich <frank-w@public-files.de>
---
arch/arm/boot/dts/mt6323.dtsi | 27 +++++++++++++++++++++++++++
1 file changed, 27 insertions(+)
diff --git a/arch/arm/boot/dts/mt6323.dtsi b/arch/arm/boot/dts/mt6323.dtsi
index ba397407c1dd..7fda40ab5fe8 100644
--- a/arch/arm/boot/dts/mt6323.dtsi
+++ b/arch/arm/boot/dts/mt6323.dtsi
@@ -238,5 +238,32 @@
regulator-enable-ramp-delay = <216>;
};
};
+
+ mt6323keys: mt6323keys {
+ compatible = "mediatek,mt6323-keys";
+ mediatek,long-press-mode = <1>;
+ power-off-time-sec = <0>;
+
+ power {
+ linux,keycodes = <116>;
+ wakeup-source;
+ };
+
+ home {
+ linux,keycodes = <114>;
+ };
+ };
+
+ codec: mt6397codec {
+ compatible = "mediatek,mt6397-codec";
+ };
+
+ power-controller {
+ compatible = "mediatek,mt6323-pwrc";
+ };
+
+ rtc {
+ compatible = "mediatek,mt6323-rtc";
+ };
};
};
--
2.17.1
^ permalink raw reply related
* [PATCH v2 1/7] docs: dt-bindings: add poweroff
From: Frank Wunderlich @ 2019-07-03 16:48 UTC (permalink / raw)
To: Lee Jones, Rob Herring, Mark Rutland, Matthias Brugger, Sean Wang,
Sebastian Reichel, Alessandro Zummo, Alexandre Belloni,
devicetree, linux-arm-kernel, linux-mediatek, linux-kernel,
linux-pm, linux-rtc, Eddie Huang, Thomas Gleixner,
Richard Fontana, Allison Randal, David S . Miller,
Mauro Carvalho Chehab, Greg Kroah-Hartman, Rob Herring,
Linus Walleij, Nicolas Ferre, Paul E . McKenney
Cc: Josef Friedl, Frank Wunderlich
In-Reply-To: <20190703164822.17924-1-frank-w@public-files.de>
From: Josef Friedl <josef.friedl@speed.at>
add documentation for pmic, rtc and power/reset devicetree bindings
Suggested-by: Frank Wunderlich <frank-w@public-files.de>
Signed-off-by: Josef Friedl <josef.friedl@speed.at>
Signed-off-by: Frank Wunderlich <frank-w@public-files.de>
---
.../devicetree/bindings/mfd/mt6397.txt | 10 ++++++-
.../bindings/power/reset/mt6323-poweroff.txt | 20 +++++++++++++
.../devicetree/bindings/rtc/rtc-mt6397.txt | 29 +++++++++++++++++++
3 files changed, 58 insertions(+), 1 deletion(-)
create mode 100644 Documentation/devicetree/bindings/power/reset/mt6323-poweroff.txt
create mode 100644 Documentation/devicetree/bindings/rtc/rtc-mt6397.txt
diff --git a/Documentation/devicetree/bindings/mfd/mt6397.txt b/Documentation/devicetree/bindings/mfd/mt6397.txt
index 0ebd08af777d..44acb9827716 100644
--- a/Documentation/devicetree/bindings/mfd/mt6397.txt
+++ b/Documentation/devicetree/bindings/mfd/mt6397.txt
@@ -8,6 +8,7 @@ MT6397/MT6323 is a multifunction device with the following sub modules:
- Clock
- LED
- Keys
+- Power controller
It is interfaced to host controller using SPI interface by a proprietary hardware
called PMIC wrapper or pwrap. MT6397/MT6323 MFD is a child device of pwrap.
@@ -22,8 +23,10 @@ compatible: "mediatek,mt6397" or "mediatek,mt6323"
Optional subnodes:
- rtc
- Required properties:
+ Required properties: Should be one of follows
+ - compatible: "mediatek,mt6323-rtc"
- compatible: "mediatek,mt6397-rtc"
+ For details, see Documentation/devicetree/bindings/rtc/rtc-mt6397.txt
- regulators
Required properties:
- compatible: "mediatek,mt6397-regulator"
@@ -46,6 +49,11 @@ Optional subnodes:
- compatible: "mediatek,mt6397-keys" or "mediatek,mt6323-keys"
see Documentation/devicetree/bindings/input/mtk-pmic-keys.txt
+- power-controller
+ Required properties:
+ - compatible: "mediatek,mt6323-pwrc"
+ For details, see Documentation/devicetree/bindings/power/reset/mt6323-poweroff.txt
+
Example:
pwrap: pwrap@1000f000 {
compatible = "mediatek,mt8135-pwrap";
diff --git a/Documentation/devicetree/bindings/power/reset/mt6323-poweroff.txt b/Documentation/devicetree/bindings/power/reset/mt6323-poweroff.txt
new file mode 100644
index 000000000000..933f0c48e887
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/reset/mt6323-poweroff.txt
@@ -0,0 +1,20 @@
+Device Tree Bindings for Power Controller on MediaTek PMIC
+
+The power controller which could be found on PMIC is responsible for externally
+powering off or on the remote MediaTek SoC through the circuit BBPU.
+
+Required properties:
+- compatible: Should be one of follows
+ "mediatek,mt6323-pwrc": for MT6323 PMIC
+
+Example:
+
+ pmic {
+ compatible = "mediatek,mt6323";
+
+ ...
+
+ power-controller {
+ compatible = "mediatek,mt6323-pwrc";
+ };
+ }
diff --git a/Documentation/devicetree/bindings/rtc/rtc-mt6397.txt b/Documentation/devicetree/bindings/rtc/rtc-mt6397.txt
new file mode 100644
index 000000000000..ebd1cf80dcc8
--- /dev/null
+++ b/Documentation/devicetree/bindings/rtc/rtc-mt6397.txt
@@ -0,0 +1,29 @@
+Device-Tree bindings for MediaTek PMIC based RTC
+
+MediaTek PMIC based RTC is an independent function of MediaTek PMIC that works
+as a type of multi-function device (MFD). The RTC can be configured and set up
+with PMIC wrapper bus which is a common resource shared with the other
+functions found on the same PMIC.
+
+For MediaTek PMIC MFD bindings, see:
+Documentation/devicetree/bindings/mfd/mt6397.txt
+
+For MediaTek PMIC wrapper bus bindings, see:
+Documentation/devicetree/bindings/soc/mediatek/pwrap.txt
+
+Required properties:
+- compatible: Should be one of follows
+ "mediatek,mt6323-rtc": for MT6323 PMIC
+ "mediatek,mt6397-rtc": for MT6397 PMIC
+
+Example:
+
+ pmic {
+ compatible = "mediatek,mt6323";
+
+ ...
+
+ rtc {
+ compatible = "mediatek,mt6323-rtc";
+ };
+ };
--
2.17.1
^ permalink raw reply related
* [PATCH v2 0/7] implement poweroff for mt6323/6397
From: Frank Wunderlich @ 2019-07-03 16:48 UTC (permalink / raw)
To: Lee Jones, Rob Herring, Mark Rutland, Matthias Brugger, Sean Wang,
Sebastian Reichel, Alessandro Zummo, Alexandre Belloni,
devicetree, linux-arm-kernel, linux-mediatek, linux-kernel,
linux-pm, linux-rtc, Eddie Huang, Thomas Gleixner,
Richard Fontana, Allison Randal, David S . Miller,
Mauro Carvalho Chehab, Greg Kroah-Hartman, Rob Herring,
Linus Walleij, Nicolas Ferre, Paul E . McKenney
Cc: Frank Wunderlich
mainline-driver does not support mt6323
this series adds mt6323 to mt6397-rtc-driver and implement
power-controller on it.
with this poweroff is working on bananapi-r2
Original Patch from Josef Friedl
changes since v1:
- splitted into functional parts
- more infos about changes
Josef Friedl (7):
docs: dt-bindings: add poweroff
rtc: mt6397: move some common definitions into rtc.h
rtc: mt6397: improvements of rtc driver
mfd: mt6323: some improvements of mt6397-core
power: reset: add driver for mt6323 poweroff
MAINTAINERS: add Mediatek shutdown drivers
arm: dts: mt6323: add keys, power-controller, rtc and codec
.../devicetree/bindings/mfd/mt6397.txt | 10 +-
.../bindings/power/reset/mt6323-poweroff.txt | 20 ++++
.../devicetree/bindings/rtc/rtc-mt6397.txt | 29 +++++
MAINTAINERS | 7 ++
arch/arm/boot/dts/mt6323.dtsi | 27 +++++
drivers/mfd/mt6397-core.c | 40 +++++--
drivers/power/reset/Kconfig | 10 ++
drivers/power/reset/Makefile | 1 +
drivers/power/reset/mt6323-poweroff.c | 97 +++++++++++++++
drivers/rtc/rtc-mt6397.c | 110 ++++--------------
include/linux/mfd/mt6397/core.h | 2 +
include/linux/mfd/mt6397/rtc.h | 71 +++++++++++
12 files changed, 325 insertions(+), 99 deletions(-)
create mode 100644 Documentation/devicetree/bindings/power/reset/mt6323-poweroff.txt
create mode 100644 Documentation/devicetree/bindings/rtc/rtc-mt6397.txt
create mode 100644 drivers/power/reset/mt6323-poweroff.c
create mode 100644 include/linux/mfd/mt6397/rtc.h
--
2.17.1
^ permalink raw reply
* [PATCH v2 4/7] mfd: mt6323: some improvements of mt6397-core
From: Frank Wunderlich @ 2019-07-03 16:48 UTC (permalink / raw)
To: Lee Jones, Rob Herring, Mark Rutland, Matthias Brugger, Sean Wang,
Sebastian Reichel, Alessandro Zummo, Alexandre Belloni,
devicetree, linux-arm-kernel, linux-mediatek, linux-kernel,
linux-pm, linux-rtc, Eddie Huang, Thomas Gleixner,
Richard Fontana, Allison Randal, David S . Miller,
Mauro Carvalho Chehab, Greg Kroah-Hartman, Rob Herring,
Linus Walleij, Nicolas Ferre, Paul E . McKenney
Cc: Josef Friedl, Frank Wunderlich
In-Reply-To: <20190703164822.17924-1-frank-w@public-files.de>
From: Josef Friedl <josef.friedl@speed.at>
- simplyfications (resource definitions my DEFINE_RES_* macros)
- add mt6323 rtc+pwrc
Signed-off-by: Frank Wunderlich <frank-w@public-files.de>
---
drivers/mfd/mt6397-core.c | 40 ++++++++++++++++++++++++++++-----------
1 file changed, 29 insertions(+), 11 deletions(-)
diff --git a/drivers/mfd/mt6397-core.c b/drivers/mfd/mt6397-core.c
index 337bcccdb914..a4abce00f156 100644
--- a/drivers/mfd/mt6397-core.c
+++ b/drivers/mfd/mt6397-core.c
@@ -1,10 +1,11 @@
// SPDX-License-Identifier: GPL-2.0-only
/*
- * Copyright (c) 2014 MediaTek Inc.
+ * Copyright (c) 2014-2018 MediaTek Inc.
* Author: Flora Fu, MediaTek
*/
#include <linux/interrupt.h>
+#include <linux/ioport.h>
#include <linux/module.h>
#include <linux/of_device.h>
#include <linux/of_irq.h>
@@ -15,24 +16,27 @@
#include <linux/mfd/mt6397/registers.h>
#include <linux/mfd/mt6323/registers.h>
+#define MT6323_RTC_BASE 0x8000
+#define MT6323_RTC_SIZE 0x40
+
#define MT6397_RTC_BASE 0xe000
#define MT6397_RTC_SIZE 0x3e
+#define MT6323_PWRC_BASE 0x8000
+#define MT6323_PWRC_SIZE 0x40
+
#define MT6323_CID_CODE 0x23
#define MT6391_CID_CODE 0x91
#define MT6397_CID_CODE 0x97
+static const struct resource mt6323_rtc_resources[] = {
+ DEFINE_RES_MEM(MT6323_RTC_BASE, MT6323_RTC_SIZE),
+ DEFINE_RES_IRQ(MT6323_IRQ_STATUS_RTC),
+};
+
static const struct resource mt6397_rtc_resources[] = {
- {
- .start = MT6397_RTC_BASE,
- .end = MT6397_RTC_BASE + MT6397_RTC_SIZE,
- .flags = IORESOURCE_MEM,
- },
- {
- .start = MT6397_IRQ_RTC,
- .end = MT6397_IRQ_RTC,
- .flags = IORESOURCE_IRQ,
- },
+ DEFINE_RES_MEM(MT6397_RTC_BASE, MT6397_RTC_SIZE),
+ DEFINE_RES_IRQ(MT6397_IRQ_RTC),
};
static const struct resource mt6323_keys_resources[] = {
@@ -45,8 +49,17 @@ static const struct resource mt6397_keys_resources[] = {
DEFINE_RES_IRQ(MT6397_IRQ_HOMEKEY),
};
+static const struct resource mt6323_pwrc_resources[] = {
+ DEFINE_RES_MEM(MT6323_PWRC_BASE, MT6323_PWRC_SIZE),
+};
+
static const struct mfd_cell mt6323_devs[] = {
{
+ .name = "mt6323-rtc",
+ .num_resources = ARRAY_SIZE(mt6323_rtc_resources),
+ .resources = mt6323_rtc_resources,
+ .of_compatible = "mediatek,mt6323-rtc",
+ }, {
.name = "mt6323-regulator",
.of_compatible = "mediatek,mt6323-regulator"
}, {
@@ -57,6 +70,11 @@ static const struct mfd_cell mt6323_devs[] = {
.num_resources = ARRAY_SIZE(mt6323_keys_resources),
.resources = mt6323_keys_resources,
.of_compatible = "mediatek,mt6323-keys"
+ }, {
+ .name = "mt6323-pwrc",
+ .num_resources = ARRAY_SIZE(mt6323_pwrc_resources),
+ .resources = mt6323_pwrc_resources,
+ .of_compatible = "mediatek,mt6323-pwrc"
},
};
--
2.17.1
^ permalink raw reply related
* [PATCH v2 5/7] power: reset: add driver for mt6323 poweroff
From: Frank Wunderlich @ 2019-07-03 16:48 UTC (permalink / raw)
To: Lee Jones, Rob Herring, Mark Rutland, Matthias Brugger, Sean Wang,
Sebastian Reichel, Alessandro Zummo, Alexandre Belloni,
devicetree, linux-arm-kernel, linux-mediatek, linux-kernel,
linux-pm, linux-rtc, Eddie Huang, Thomas Gleixner,
Richard Fontana, Allison Randal, David S . Miller,
Mauro Carvalho Chehab, Greg Kroah-Hartman, Rob Herring,
Linus Walleij, Nicolas Ferre, Paul E . McKenney
Cc: Josef Friedl, Frank Wunderlich
In-Reply-To: <20190703164822.17924-1-frank-w@public-files.de>
From: Josef Friedl <josef.friedl@speed.at>
Suggested-by: Frank Wunderlich <frank-w@public-files.de>
Signed-off-by: Josef Friedl <josef.friedl@speed.at>
Signed-off-by: Frank Wunderlich <frank-w@public-files.de>
---
drivers/power/reset/Kconfig | 10 +++
drivers/power/reset/Makefile | 1 +
drivers/power/reset/mt6323-poweroff.c | 97 +++++++++++++++++++++++++++
include/linux/mfd/mt6397/core.h | 2 +
4 files changed, 110 insertions(+)
create mode 100644 drivers/power/reset/mt6323-poweroff.c
diff --git a/drivers/power/reset/Kconfig b/drivers/power/reset/Kconfig
index 980951dff834..492678e22088 100644
--- a/drivers/power/reset/Kconfig
+++ b/drivers/power/reset/Kconfig
@@ -140,6 +140,16 @@ config POWER_RESET_LTC2952
This driver supports an external powerdown trigger and board power
down via the LTC2952. Bindings are made in the device tree.
+config POWER_RESET_MT6323
+ bool "MediaTek MT6323 power-off driver"
+ depends on MFD_MT6397
+ help
+ The power-off driver is responsible for externally shutdown down
+ the power of a remote MediaTek SoC MT6323 is connected to through
+ controlling a tiny circuit BBPU inside MT6323 RTC.
+
+ Say Y if you have a board where MT6323 could be found.
+
config POWER_RESET_QNAP
bool "QNAP power-off driver"
depends on OF_GPIO && PLAT_ORION
diff --git a/drivers/power/reset/Makefile b/drivers/power/reset/Makefile
index 0aebee954ac1..94eaceb01d66 100644
--- a/drivers/power/reset/Makefile
+++ b/drivers/power/reset/Makefile
@@ -11,6 +11,7 @@ obj-$(CONFIG_POWER_RESET_GPIO) += gpio-poweroff.o
obj-$(CONFIG_POWER_RESET_GPIO_RESTART) += gpio-restart.o
obj-$(CONFIG_POWER_RESET_HISI) += hisi-reboot.o
obj-$(CONFIG_POWER_RESET_MSM) += msm-poweroff.o
+obj-$(CONFIG_POWER_RESET_MT6323) += mt6323-poweroff.o
obj-$(CONFIG_POWER_RESET_QCOM_PON) += qcom-pon.o
obj-$(CONFIG_POWER_RESET_OCELOT_RESET) += ocelot-reset.o
obj-$(CONFIG_POWER_RESET_PIIX4_POWEROFF) += piix4-poweroff.o
diff --git a/drivers/power/reset/mt6323-poweroff.c b/drivers/power/reset/mt6323-poweroff.c
new file mode 100644
index 000000000000..1caf43d9e46d
--- /dev/null
+++ b/drivers/power/reset/mt6323-poweroff.c
@@ -0,0 +1,97 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Power off through MediaTek PMIC
+ *
+ * Copyright (C) 2018 MediaTek Inc.
+ *
+ * Author: Sean Wang <sean.wang@mediatek.com>
+ *
+ */
+
+#include <linux/err.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/mfd/mt6397/core.h>
+#include <linux/mfd/mt6397/rtc.h>
+
+struct mt6323_pwrc {
+ struct device *dev;
+ struct regmap *regmap;
+ u32 base;
+};
+
+static struct mt6323_pwrc *mt_pwrc;
+
+static void mt6323_do_pwroff(void)
+{
+ struct mt6323_pwrc *pwrc = mt_pwrc;
+ unsigned int val;
+ int ret;
+
+ regmap_write(pwrc->regmap, pwrc->base + RTC_BBPU, RTC_BBPU_KEY);
+ regmap_write(pwrc->regmap, pwrc->base + RTC_WRTGR, 1);
+
+ ret = regmap_read_poll_timeout(pwrc->regmap,
+ pwrc->base + RTC_BBPU, val,
+ !(val & RTC_BBPU_CBUSY),
+ MTK_RTC_POLL_DELAY_US,
+ MTK_RTC_POLL_TIMEOUT);
+ if (ret)
+ dev_err(pwrc->dev, "failed to write BBPU: %d\n", ret);
+
+ /* Wait some time until system down, otherwise, notice with a warn */
+ mdelay(1000);
+
+ WARN_ONCE(1, "Unable to power off system\n");
+}
+
+static int mt6323_pwrc_probe(struct platform_device *pdev)
+{
+ struct mt6397_chip *mt6397_chip = dev_get_drvdata(pdev->dev.parent);
+ struct mt6323_pwrc *pwrc;
+ struct resource *res;
+
+ pwrc = devm_kzalloc(&pdev->dev, sizeof(*pwrc), GFP_KERNEL);
+ if (!pwrc)
+ return -ENOMEM;
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ pwrc->base = res->start;
+ pwrc->regmap = mt6397_chip->regmap;
+ pwrc->dev = &pdev->dev;
+ mt_pwrc = pwrc;
+
+ pm_power_off = &mt6323_do_pwroff;
+
+ return 0;
+}
+
+static int mt6323_pwrc_remove(struct platform_device *pdev)
+{
+ if (pm_power_off == &mt6323_do_pwroff)
+ pm_power_off = NULL;
+
+ return 0;
+}
+
+static const struct of_device_id mt6323_pwrc_dt_match[] = {
+ { .compatible = "mediatek,mt6323-pwrc" },
+ {},
+};
+MODULE_DEVICE_TABLE(of, mt6323_pwrc_dt_match);
+
+static struct platform_driver mt6323_pwrc_driver = {
+ .probe = mt6323_pwrc_probe,
+ .remove = mt6323_pwrc_remove,
+ .driver = {
+ .name = "mt6323-pwrc",
+ .of_match_table = mt6323_pwrc_dt_match,
+ },
+};
+
+module_platform_driver(mt6323_pwrc_driver);
+
+MODULE_DESCRIPTION("Poweroff driver for MT6323 PMIC");
+MODULE_AUTHOR("Sean Wang <sean.wang@mediatek.com>");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/mfd/mt6397/core.h b/include/linux/mfd/mt6397/core.h
index 25a95e72179b..652da61e3711 100644
--- a/include/linux/mfd/mt6397/core.h
+++ b/include/linux/mfd/mt6397/core.h
@@ -7,6 +7,8 @@
#ifndef __MFD_MT6397_CORE_H__
#define __MFD_MT6397_CORE_H__
+#include <linux/mutex.h>
+
enum mt6397_irq_numbers {
MT6397_IRQ_SPKL_AB = 0,
MT6397_IRQ_SPKR_AB,
--
2.17.1
^ permalink raw reply related
* [PATCH v2 6/7] MAINTAINERS: add Mediatek shutdown drivers
From: Frank Wunderlich @ 2019-07-03 16:48 UTC (permalink / raw)
To: Lee Jones, Rob Herring, Mark Rutland, Matthias Brugger, Sean Wang,
Sebastian Reichel, Alessandro Zummo, Alexandre Belloni,
devicetree, linux-arm-kernel, linux-mediatek, linux-kernel,
linux-pm, linux-rtc, Eddie Huang, Thomas Gleixner,
Richard Fontana, Allison Randal, David S . Miller,
Mauro Carvalho Chehab, Greg Kroah-Hartman, Rob Herring,
Linus Walleij, Nicolas Ferre, Paul E . McKenney
Cc: Josef Friedl, Frank Wunderlich
In-Reply-To: <20190703164822.17924-1-frank-w@public-files.de>
From: Josef Friedl <josef.friedl@speed.at>
Signed-off-by: Frank Wunderlich <frank-w@public-files.de>
---
MAINTAINERS | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index 01a52fc964da..31c1e882b7d2 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9920,6 +9920,13 @@ S: Maintained
F: drivers/net/dsa/mt7530.*
F: net/dsa/tag_mtk.c
+MEDIATEK BOARD LEVEL SHUTDOWN DRIVERS
+M: Sean Wang <sean.wang@mediatek.com>
+L: linux-pm@vger.kernel.org
+S: Maintained
+F: Documentation/devicetree/bindings/power/reset/mt6323-poweroff.txt
+F: drivers/power/reset/mt6323-poweroff.c
+
MEDIATEK JPEG DRIVER
M: Rick Chang <rick.chang@mediatek.com>
M: Bin Liu <bin.liu@mediatek.com>
--
2.17.1
^ permalink raw reply related
* [PATCH v2 2/7] rtc: mt6397: move some common definitions into rtc.h
From: Frank Wunderlich @ 2019-07-03 16:48 UTC (permalink / raw)
To: Lee Jones, Rob Herring, Mark Rutland, Matthias Brugger, Sean Wang,
Sebastian Reichel, Alessandro Zummo, Alexandre Belloni,
devicetree, linux-arm-kernel, linux-mediatek, linux-kernel,
linux-pm, linux-rtc, Eddie Huang, Thomas Gleixner,
Richard Fontana, Allison Randal, David S . Miller,
Mauro Carvalho Chehab, Greg Kroah-Hartman, Rob Herring,
Linus Walleij, Nicolas Ferre, Paul E . McKenney
Cc: Josef Friedl, Frank Wunderlich
In-Reply-To: <20190703164822.17924-1-frank-w@public-files.de>
From: Josef Friedl <josef.friedl@speed.at>
Suggested-by: Frank Wunderlich <frank-w@public-files.de>
Signed-off-by: Josef Friedl <josef.friedl@speed.at>
Signed-off-by: Frank Wunderlich <frank-w@public-files.de>
---
drivers/rtc/rtc-mt6397.c | 55 +-------------------------
include/linux/mfd/mt6397/rtc.h | 71 ++++++++++++++++++++++++++++++++++
2 files changed, 72 insertions(+), 54 deletions(-)
create mode 100644 include/linux/mfd/mt6397/rtc.h
diff --git a/drivers/rtc/rtc-mt6397.c b/drivers/rtc/rtc-mt6397.c
index b46ed4dc7015..c08ee5edf865 100644
--- a/drivers/rtc/rtc-mt6397.c
+++ b/drivers/rtc/rtc-mt6397.c
@@ -9,60 +9,7 @@
#include <linux/module.h>
#include <linux/regmap.h>
#include <linux/rtc.h>
-#include <linux/irqdomain.h>
-#include <linux/platform_device.h>
-#include <linux/of_address.h>
-#include <linux/of_irq.h>
-#include <linux/io.h>
-#include <linux/mfd/mt6397/core.h>
-
-#define RTC_BBPU 0x0000
-#define RTC_BBPU_CBUSY BIT(6)
-
-#define RTC_WRTGR 0x003c
-
-#define RTC_IRQ_STA 0x0002
-#define RTC_IRQ_STA_AL BIT(0)
-#define RTC_IRQ_STA_LP BIT(3)
-
-#define RTC_IRQ_EN 0x0004
-#define RTC_IRQ_EN_AL BIT(0)
-#define RTC_IRQ_EN_ONESHOT BIT(2)
-#define RTC_IRQ_EN_LP BIT(3)
-#define RTC_IRQ_EN_ONESHOT_AL (RTC_IRQ_EN_ONESHOT | RTC_IRQ_EN_AL)
-
-#define RTC_AL_MASK 0x0008
-#define RTC_AL_MASK_DOW BIT(4)
-
-#define RTC_TC_SEC 0x000a
-/* Min, Hour, Dom... register offset to RTC_TC_SEC */
-#define RTC_OFFSET_SEC 0
-#define RTC_OFFSET_MIN 1
-#define RTC_OFFSET_HOUR 2
-#define RTC_OFFSET_DOM 3
-#define RTC_OFFSET_DOW 4
-#define RTC_OFFSET_MTH 5
-#define RTC_OFFSET_YEAR 6
-#define RTC_OFFSET_COUNT 7
-
-#define RTC_AL_SEC 0x0018
-
-#define RTC_PDN2 0x002e
-#define RTC_PDN2_PWRON_ALARM BIT(4)
-
-#define RTC_MIN_YEAR 1968
-#define RTC_BASE_YEAR 1900
-#define RTC_NUM_YEARS 128
-#define RTC_MIN_YEAR_OFFSET (RTC_MIN_YEAR - RTC_BASE_YEAR)
-
-struct mt6397_rtc {
- struct device *dev;
- struct rtc_device *rtc_dev;
- struct mutex lock;
- struct regmap *regmap;
- int irq;
- u32 addr_base;
-};
+#include <linux/mfd/mt6397/rtc.h>
static int mtk_rtc_write_trigger(struct mt6397_rtc *rtc)
{
diff --git a/include/linux/mfd/mt6397/rtc.h b/include/linux/mfd/mt6397/rtc.h
new file mode 100644
index 000000000000..b702c29e8c74
--- /dev/null
+++ b/include/linux/mfd/mt6397/rtc.h
@@ -0,0 +1,71 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2014-2018 MediaTek Inc.
+ *
+ * Author: Tianping.Fang <tianping.fang@mediatek.com>
+ * Sean Wang <sean.wang@mediatek.com>
+ */
+
+#ifndef _LINUX_MFD_MT6397_RTC_H_
+#define _LINUX_MFD_MT6397_RTC_H_
+
+#include <linux/jiffies.h>
+#include <linux/mutex.h>
+#include <linux/regmap.h>
+#include <linux/rtc.h>
+
+#define RTC_BBPU 0x0000
+#define RTC_BBPU_CBUSY BIT(6)
+#define RTC_BBPU_KEY (0x43 << 8)
+
+#define RTC_WRTGR 0x003c
+
+#define RTC_IRQ_STA 0x0002
+#define RTC_IRQ_STA_AL BIT(0)
+#define RTC_IRQ_STA_LP BIT(3)
+
+#define RTC_IRQ_EN 0x0004
+#define RTC_IRQ_EN_AL BIT(0)
+#define RTC_IRQ_EN_ONESHOT BIT(2)
+#define RTC_IRQ_EN_LP BIT(3)
+#define RTC_IRQ_EN_ONESHOT_AL (RTC_IRQ_EN_ONESHOT | RTC_IRQ_EN_AL)
+
+#define RTC_AL_MASK 0x0008
+#define RTC_AL_MASK_DOW BIT(4)
+
+#define RTC_TC_SEC 0x000a
+/* Min, Hour, Dom... register offset to RTC_TC_SEC */
+#define RTC_OFFSET_SEC 0
+#define RTC_OFFSET_MIN 1
+#define RTC_OFFSET_HOUR 2
+#define RTC_OFFSET_DOM 3
+#define RTC_OFFSET_DOW 4
+#define RTC_OFFSET_MTH 5
+#define RTC_OFFSET_YEAR 6
+#define RTC_OFFSET_COUNT 7
+
+#define RTC_AL_SEC 0x0018
+
+#define RTC_PDN2 0x002e
+#define RTC_PDN2_PWRON_ALARM BIT(4)
+
+#define RTC_MIN_YEAR 1968
+#define RTC_BASE_YEAR 1900
+#define RTC_NUM_YEARS 128
+#define RTC_MIN_YEAR_OFFSET (RTC_MIN_YEAR - RTC_BASE_YEAR)
+
+#define MTK_RTC_POLL_DELAY_US 10
+#define MTK_RTC_POLL_TIMEOUT (jiffies_to_usecs(HZ))
+
+struct mt6397_rtc {
+ struct device *dev;
+ struct rtc_device *rtc_dev;
+
+ /* Protect register access from multiple tasks */
+ struct mutex lock;
+ struct regmap *regmap;
+ int irq;
+ u32 addr_base;
+};
+
+#endif /* _LINUX_MFD_MT6397_RTC_H_ */
--
2.17.1
^ permalink raw reply related
* [PATCH v2 3/7] rtc: mt6397: improvements of rtc driver
From: Frank Wunderlich @ 2019-07-03 16:48 UTC (permalink / raw)
To: Lee Jones, Rob Herring, Mark Rutland, Matthias Brugger, Sean Wang,
Sebastian Reichel, Alessandro Zummo, Alexandre Belloni,
devicetree, linux-arm-kernel, linux-mediatek, linux-kernel,
linux-pm, linux-rtc, Eddie Huang, Thomas Gleixner,
Richard Fontana, Allison Randal, David S . Miller,
Mauro Carvalho Chehab, Greg Kroah-Hartman, Rob Herring,
Linus Walleij, Nicolas Ferre, Paul E . McKenney
Cc: Josef Friedl, Frank Wunderlich
In-Reply-To: <20190703164822.17924-1-frank-w@public-files.de>
From: Josef Friedl <josef.friedl@speed.at>
- use regmap_read_poll_timeout to drop while-loop
- use devm-api to drop remove-callback
- add new compatible for mt6323
Signed-off-by: Frank Wunderlich <frank-w@public-files.de>
---
drivers/rtc/rtc-mt6397.c | 55 ++++++++++++++++------------------------
1 file changed, 22 insertions(+), 33 deletions(-)
diff --git a/drivers/rtc/rtc-mt6397.c b/drivers/rtc/rtc-mt6397.c
index c08ee5edf865..e5ddf0d0b6f1 100644
--- a/drivers/rtc/rtc-mt6397.c
+++ b/drivers/rtc/rtc-mt6397.c
@@ -4,16 +4,19 @@
* Author: Tianping.Fang <tianping.fang@mediatek.com>
*/
-#include <linux/delay.h>
-#include <linux/init.h>
+#include <linux/err.h>
+#include <linux/interrupt.h>
+#include <linux/mfd/mt6397/core.h>
#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/platform_device.h>
#include <linux/regmap.h>
#include <linux/rtc.h>
#include <linux/mfd/mt6397/rtc.h>
+#include <linux/mod_devicetable.h>
static int mtk_rtc_write_trigger(struct mt6397_rtc *rtc)
{
- unsigned long timeout = jiffies + HZ;
int ret;
u32 data;
@@ -21,19 +24,13 @@ static int mtk_rtc_write_trigger(struct mt6397_rtc *rtc)
if (ret < 0)
return ret;
- while (1) {
- ret = regmap_read(rtc->regmap, rtc->addr_base + RTC_BBPU,
- &data);
- if (ret < 0)
- break;
- if (!(data & RTC_BBPU_CBUSY))
- break;
- if (time_after(jiffies, timeout)) {
- ret = -ETIMEDOUT;
- break;
- }
- cpu_relax();
- }
+ ret = regmap_read_poll_timeout(rtc->regmap,
+ rtc->addr_base + RTC_BBPU, data,
+ !(data & RTC_BBPU_CBUSY),
+ MTK_RTC_POLL_DELAY_US,
+ MTK_RTC_POLL_TIMEOUT);
+ if (ret < 0)
+ dev_err(rtc->dev, "failed to write WRTGE: %d\n", ret);
return ret;
}
@@ -271,14 +268,11 @@ static int mtk_rtc_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, rtc);
- rtc->rtc_dev = devm_rtc_allocate_device(rtc->dev);
- if (IS_ERR(rtc->rtc_dev))
- return PTR_ERR(rtc->rtc_dev);
+ ret = devm_request_threaded_irq(&pdev->dev, rtc->irq, NULL,
+ mtk_rtc_irq_handler_thread,
+ IRQF_ONESHOT | IRQF_TRIGGER_HIGH,
+ "mt6397-rtc", rtc);
- ret = request_threaded_irq(rtc->irq, NULL,
- mtk_rtc_irq_handler_thread,
- IRQF_ONESHOT | IRQF_TRIGGER_HIGH,
- "mt6397-rtc", rtc);
if (ret) {
dev_err(&pdev->dev, "Failed to request alarm IRQ: %d: %d\n",
rtc->irq, ret);
@@ -287,6 +281,10 @@ static int mtk_rtc_probe(struct platform_device *pdev)
device_init_wakeup(&pdev->dev, 1);
+ rtc->rtc_dev = devm_rtc_allocate_device(&pdev->dev);
+ if (IS_ERR(rtc->rtc_dev))
+ return PTR_ERR(rtc->rtc_dev);
+
rtc->rtc_dev->ops = &mtk_rtc_ops;
ret = rtc_register_device(rtc->rtc_dev);
@@ -302,15 +300,6 @@ static int mtk_rtc_probe(struct platform_device *pdev)
return ret;
}
-static int mtk_rtc_remove(struct platform_device *pdev)
-{
- struct mt6397_rtc *rtc = platform_get_drvdata(pdev);
-
- free_irq(rtc->irq, rtc);
-
- return 0;
-}
-
#ifdef CONFIG_PM_SLEEP
static int mt6397_rtc_suspend(struct device *dev)
{
@@ -337,6 +326,7 @@ static SIMPLE_DEV_PM_OPS(mt6397_pm_ops, mt6397_rtc_suspend,
mt6397_rtc_resume);
static const struct of_device_id mt6397_rtc_of_match[] = {
+ { .compatible = "mediatek,mt6323-rtc", },
{ .compatible = "mediatek,mt6397-rtc", },
{ }
};
@@ -349,7 +339,6 @@ static struct platform_driver mtk_rtc_driver = {
.pm = &mt6397_pm_ops,
},
.probe = mtk_rtc_probe,
- .remove = mtk_rtc_remove,
};
module_platform_driver(mtk_rtc_driver);
--
2.17.1
^ permalink raw reply related
* Re: [RFC PATCH v2 0/5] sched/cpufreq: Make schedutil energy aware
From: Douglas Raillard @ 2019-07-03 16:36 UTC (permalink / raw)
To: Peter Zijlstra
Cc: linux-kernel, linux-pm, mingo, rjw, viresh.kumar, quentin.perret,
patrick.bellasi, dietmar.eggemann
In-Reply-To: <20190702155115.GW3436@hirez.programming.kicks-ass.net>
On 7/2/19 4:51 PM, Peter Zijlstra wrote:
> On Thu, Jun 27, 2019 at 06:15:58PM +0100, Douglas RAILLARD wrote:
>> Make schedutil cpufreq governor energy-aware.
>>
>> - patch 4 adds sugov_cpu_ramp_boost() function.
>> - patch 5 updates sugov_update_(single|shared)() to make use of
>> sugov_cpu_ramp_boost().
>>
>> The benefits of using the EM in schedutil are twofold:
>
>> 2) Driving the frequency selection with power in mind, in addition to
>> maximizing the utilization of the non-idle CPUs in the system.
>
>> Point 2) is enabled in
>> "sched/cpufreq: Boost schedutil frequency ramp up". It allows using
>> higher frequencies when it is known that the true utilization of
>> currently running tasks is exceeding their previous stable point.
>> The benefits are:
>>
>> * Boosting the frequency when the behavior of a runnable task changes,
>> leading to an increase in utilization. That shortens the frequency
>> ramp up duration, which in turns allows the utilization signal to
>> reach stable values quicker. Since the allowed frequency boost is
>> bounded in energy, it will behave consistently across platforms,
>> regardless of the OPP cost range.
>>
>> * The boost is only transient, and should not impact a lot the energy
>> consumed of workloads with very stable utilization signals.
>
[reordered original comments]
> This then obviously has relation to Patrick's patch that makes the EWMA
> asymmetric, but I'm thinking that the interaction is mostly favourable?
Making task_ue.ewma larger makes cpu_ue.enqueued larger, so Patrick's patch
helps increasing the utilisation as seen by schedutil in that transient time.
(see discussion on schedutil signals at the bottom). That goes in the same
direction as this series.
> So you're allowing a higher pick when the EWMA exceeds the enqueue
> thing.
TLDR: Schedutil ramp boost works on CPU rq signals, for which util est EWMA
is not defined, but the idea is the same (replace util est EWMA by util_avg).
The important point here is that when util_avg for the task becomes higher
than task_ue.enqueued, it means the knowledge of the actual needs of the task
is turned into a lower bound (=task_ue.enqueued) rather than an exact value.
This means that selecting a higher frequency than that is:
a) necessary, the task needs more computational power to do its job.
b) a shot in the dark, as it's impossible to predict exactly how much it will
need without a crystal ball.
When adding ramp boost, the bill is split: part of the "shot in the dark" comes from
the growing CPU's util_avg (see schedutil_u definition at the bottom), and part of it
comes from the ramp boost. We don't want to make the boost too costly either since
it's a shot in the dark. Therefore, we make the boost proportional to a battery life
cost rather than some guessed utilisation.
Now that I think about it, it may make sense to let this ramp-boost completely
handle this "future util prediction" case, as it's not better or worse than
util_avg at that (since it's based on it), but allows better control on
the cost of a (mis)prediction.
>
> I'm not immediately seeing how it is transient; that is, PELT has a
> wobble in it's steady state, is that accounted for?
>
The transient-ness of the ramp boost I'm introducing comes from the fact that for a
periodic task at steady state, task_ue.enqueued <= task_u when the task is executing.
That is because task_ue.enqueued is sampled at dequeue time, precisely at the moment
at which task_u is reaching its max for that task. Since we only take into account
positive boosts, ramp boost will only have an impact in the "increase transients".
About signals schedutil is based on
===================================
Here is the state of signals used by schedutil to my knowledge to compute
the final "chosen_freq":
# let's define short names to talk about
task_ue = se.avg.util_est
task_u = se.avg.util_avg
cpu_ue = cfs_rq->avg.util_est
cpu_u = cfs_rq->avg.util_avg
# How things are defined
task_u ~= LOW_PASS_FILTER(task_activations)
task_ue.enqueued = SAMPLE_AT_DEQUEUE_AND_HOLD(task_u)
task_ue.ewma = LOW_PASS_FILTER(task_ue.enqueued)
# Patrick's patch amends task_ue.ewma definition this way:
task_ue.ewma =
| task_ue.enqueued > task_ue.ewma: task_ue.enqueued
| otherwise : LOW_PASS_FILTER(task_ue.enqueued)
cpu_ue.enqueued = SUM[MAX(task_ue.ewma, task_ue.enqueued) forall task_ue in enqueued_tasks]
cpu_u = SUM[task_u forall task_ue in enqueued_tasks]
# What schedutil considers when taking freq decisions
non_cfs_u = util of deadline + rt + irq
schedutil_u = non_cfs_u + APPLY_UCLAMP(MAX(cpu_ue.enqueued, cpu_u)) + iowait_boost
schedutil_base_freq = MAP_UTIL_FREQ(schedutil_u)
STABLE(signal) =
| signal equal to the last time it was sampled by caller: True
| otherwise : False
# A diff between two util signals is converted to a EM_COST_MARGIN_SCALE value.
# They are different units, but the conversion factor is 1 in practice.
ramp_boost =
| cpu_ue.enqueued > cpu_u && STABLE(cpu_ue.enqueued):
(cpu_ue.enqueued - cpu_u) * (EM_COST_MARGIN_SCALE/SCHED_CAPACITY_SCALE)
| otherwise: 0
APPLY_RAMP_BOOST(boost, base_freq) = boosted_freq
with
acceptable_cost = ENERGY_MODEL_COST(base_freq) * (EM_COST_MARGIN_SCALE + boost)
boosted_freq = MAX[freq forall freqs if ENERGY_MODEL_COST(freq) < acceptable_cost]
# ramp-boost is applied on a freq instead of a util (unlike iowait_boost), since
# the function ENERGY_MODEL_COST(freq) is provided by the EM, and an equivalent
# ENERGY_MODEL_COST(util) would need extra calls to MAP_UTIL_FREQ().
schedutil_freq = APPLY_RAMP_BOOST(ramp_boost, schedutil_base_freq)
REAL_FREQ(ideal_freq) = MIN[freq forall freqs if freq >= ideal_freq]
POLICY_CLAMP(freq) =
| freq < policy_min_freq: policy_min_freq
| freq > policy_max_freq: policy_max_freq
| otherwise : freq
# Frequency finally used for the policy
chosen_freq = POLICY_CLAMP(REAL_FREQ(schedutil_freq))
Thanks,
Douglas
^ permalink raw reply
* Re: NO_HZ_IDLE causes consistently low cpu "iowait" time (and higher cpu "idle" time)
From: Alan Jenkins @ 2019-07-03 16:09 UTC (permalink / raw)
To: Doug Smythies; +Cc: linux-pm, linux-kernel
In-Reply-To: <000001d531a8$8931b2a0$9b9517e0$@net>
On 03/07/2019 15:06, Doug Smythies wrote:
> On 2019.07.01 08:34 Alan Jenkins wrote:
>
>> Hi
> Hi,
>
>> I tried running a simple test:
>>
>> dd if=testfile iflag=direct bs=1M of=/dev/null
>>
>> With my default settings, `vmstat 10` shows something like 85% idle time
>> to 15% iowait time. I have 4 CPUs, so this is much less than one CPU
>> worth of iowait time.
>>
>> If I boot with "nohz=off", I see idle time fall to 75% or below, and
>> iowait rise to about 25%, equivalent to one CPU. That is what I had
>> originally expected.
>>
>> (I can also see my expected numbers, if I disable *all* C-states and
>> force polling using `pm_qos_resume_latency_us` in sysfs).
>>
>> The numbers above are from a kernel somewhere around v5.2-rc5. I saw
>> the "wrong" results on some previous kernels as well. I just now
>> realized the link to NO_HZ_IDLE.[1]
>>
>> [1]
>> https://unix.stackexchange.com/questions/517757/my-basic-assumption-about-system-iowait-does-not-hold/527836#527836
>>
>> I did not find any information about this high level of inaccuracy. Can
>> anyone explain, is this behaviour expected?
> I'm not commenting on expected behaviour or not, just that it is
> inconsistent.
>
>> I found several patches that mentioned "iowait" and NO_HZ_IDLE. But if
>> they described this problem, it was not clear to me.
>>
>> I thought this might also be affecting the "IO pressure" values from the
>> new "pressure stall information"... but I am too confused already, so I
>> am only asking about iowait at the moment :-).
> Using your workload, I confirm inconsistent behaviour for /proc/stat
> (which vmstat uses) between kernels 4.15, 4.16, and 4.17:
> 4.15 does what you expect, no matter idle states enabled or disabled.
> 4.16 doesn't do what you expect regardless. (although a little erratic.)
>> = 4.17 does what you expect with only idle state 0 enabled, and doesn't otherwise.
> Actual test data vmstat (/proc/stat) (8 CPUs, 12.5% = 1 CPU)):
> Kernel idle/iowait % Idle states >= 1
> 4.15 88/12 enabled
> 4.15 88/12 disabled
> 4.16 99/1 enabled
> 4.16 99/1 disabled
> 4.17 98/2 enabled
> 4.17 88/12 disabled
>
> Note 1: I never booted with "nohz=off" because the tick never turns off for
> idle state 0, which is good enough for testing.
>
> Note 2: Myself, I don't use /proc/stat for idle time statistics. I use:
> /sys/devices/system/cpu/cpu*/cpuidle/state*/time
> And they seem to always be consistent at the higher idle percentage number.
>
> Unless someone has some insight, the next step is kernel bisection,
> once for between kernel 4.15 and 4.16, then again between 4.16 and 4.17.
> The second bisection might go faster with knowledge gained from the first.
> Alan: Can you do kernel bisection? I can only do it starting maybe Friday.
>
> ... Doug
Thanks for your help Doug!
I wish I had a faster CPU :-), but I'm familiar with bisection. I have
started and I'm down to about 8 minute builds, so I can probably be done
before Friday.
Alan
^ permalink raw reply
* Re: [PATCH] cpuidle/drivers/mobile: Add new governor for mobile/embedded systems
From: Daniel Lezcano @ 2019-07-03 15:16 UTC (permalink / raw)
To: Doug Smythies
Cc: linux-kernel, 'Rafael J. Wysocki',
'Thomas Gleixner', 'Greg Kroah-Hartman',
'open list:CPU IDLE TIME MANAGEMENT FRAMEWORK', rafael
In-Reply-To: <000101d531aa$e00987e0$a01c97a0$@net>
Hi Doug,
On 03/07/2019 16:23, Doug Smythies wrote:
> Hi Daniel,
>
> I tried your "mobile" governor, albeit not on a mobile device.
>
> On 2019.06.20 04:58 Daniel Lezcano wrote:
>
> ...
>
>> The mobile governor is a new governor targeting embedded systems
>> running on battery where the energy saving has a higher priority than
>> servers or desktops. This governor aims to save energy as much as
>> possible but with a performance degradation tolerance.
>>
>> In this way, we can optimize the governor for specific mobile workload
>> and more generally embedded systems without impacting other platforms.
>
> I just wanted to observe the lower energy, accepting performance
> degradation. My workloads may have been inappropriate.
Thanks for trying the governor. It is still basic but will be improved
step by step regarding clearly identified workload and with more help
from the scheduler. This is the first phase of the governor providing
the base bricks.
>> +
>> +#define EMA_ALPHA_VAL 64
>> +#define EMA_ALPHA_SHIFT 7
>> +#define MAX_RESCHED_INTERVAL_MS 100
>> +
>> +static DEFINE_PER_CPU(struct mobile_device, mobile_devices);
>> +
>> +static int mobile_ema_new(s64 value, s64 ema_old)
>> +{
>> + if (likely(ema_old))
>> + return ema_old + (((value - ema_old) * EMA_ALPHA_VAL) >>
>> + EMA_ALPHA_SHIFT);
>> + return value;
>> +}
>
> Do you have any information as to why these numbers?
>
> Without any background, the filter seems overly new value weighted to me.
> It is an infinite impulse response type filter, currently at:
>
> output = 0.5 * old + 0.5 * new.
>
> I tried, but didn't get anything conclusive:
>
> output = 0.875 * old + 0.125 * new.
>
> I did it this way:
>
> #define EMA_ALPHA_VAL 7
> #define EMA_ALPHA_SHIFT 3
> #define MAX_RESCHED_INTERVAL_MS 100
Ok, I will have a look at these values.
> static DEFINE_PER_CPU(struct mobile_device, mobile_devices);
>
> static int mobile_ema_new(s64 value, s64 ema_old)
> {
> if (likely(ema_old))
> return ((ema_old * EMA_ALPHA_VAL) + value) >>
> EMA_ALPHA_SHIFT;
> return value;
> }
>
> ...
>
>> + /*
>> + * Sum all the residencies in order to compute the total
>> + * duration of the idle task.
>> + */
>> + residency = dev->last_residency - s->exit_latency;
>
> What about when the CPU comes out of the idle state before it
> even gets fully into it? Under such conditions it seems to hold
> much too hard at idle states that are too deep, to the point
> where energy goes up while performance goes down.
I'm not sure there is something we can do here :/
> Anyway, I did a bunch of tests and such, but have deleted
> most from this e-mail, because it's just noise. I'll
> include just one set:
>
> For a work load that would normally result in a lot of use
> of shallow idle states (single core pipe-test * 2 cores).
Can you share the tests and the command lines?
> I got (all kernel 5.2-rc5 + this patch):
>
> Idle governor, teo; CPU frequency scaling: intel-cpufreq/ondemand;
> Processor package power: 40.4 watts; 4.9 uSec/loop
>
> Idle governor, teo; CPU frequency scaling: intel-cpufreq/ondemand;
> Processor package power: 34 watts; 5.2 uSec/loop
>
> Idle governor, mobile; CPU frequency scaling: intel-cpufreq/ondemand;
> Processor package power: 25.9 watts; 11.1 uSec/loop
>
> Idle governor, menu; CPU frequency scaling: intel-cpufreq/ondemand;
> Processor package power: 34.2 watts; 5.23 uSec/loop
>
> Idle governor, teo; CPU frequency scaling: intel-cpufreq/ondemand;
> Maximum CPU frequency limited to 73% to match mobile energy.
> Processor package power: 25.4 watts; 6.4 uSec/loop
Ok that's interesting. Thanks for the values.
The governor can be better by selecting the shallow states, the
scheduler has to interact with the governor to give clues about the
load, that is identified and will be the next step.
Is it possible to check with the schedutil governor instead?
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
^ permalink raw reply
* [PATCH] thermal: of-thermal: avoid NULL ops dereference when writing trip temperature
From: Akinobu Mita @ 2019-07-03 14:24 UTC (permalink / raw)
To: linux-pm; +Cc: Akinobu Mita, Zhang Rui, Eduardo Valentin, Daniel Lezcano
While no sensor is registered to a DT thermal zone, __thermal_zone->ops
(struct thermal_zone_of_device_ops) is set to NULL.
This causes a NULL pointer dereference when writing trip point temperature
(i.e. trip_point_[0-*]_temp).
Fix it by checking if ops is NULL under thermal_zone_device's lock that
protects against concurrent access by thermal_zone_of_sensor_register() and
thermal_zone_of_sensor_unregister().
Cc: Zhang Rui <rui.zhang@intel.com>
Cc: Eduardo Valentin <edubezval@gmail.com>
Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
drivers/thermal/of-thermal.c | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
index dc5093b..c207075 100644
--- a/drivers/thermal/of-thermal.c
+++ b/drivers/thermal/of-thermal.c
@@ -332,17 +332,20 @@ static int of_thermal_set_trip_temp(struct thermal_zone_device *tz, int trip,
int temp)
{
struct __thermal_zone *data = tz->devdata;
+ int ret = 0;
if (trip >= data->ntrips || trip < 0)
return -EDOM;
- if (data->ops->set_trip_temp) {
- int ret;
+ mutex_lock(&tz->lock);
+ if (data->ops && data->ops->set_trip_temp)
ret = data->ops->set_trip_temp(data->sensor_data, trip, temp);
- if (ret)
- return ret;
- }
+
+ mutex_unlock(&tz->lock);
+
+ if (ret)
+ return ret;
/* thermal framework should take care of data->mask & (1 << trip) */
data->trips[trip].temperature = temp;
--
2.7.4
^ permalink raw reply related
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