* [PATCH 1/8] PCI: irq: Introduce rearm_wake_irq()
From: Rafael J. Wysocki @ 2019-07-16 16:10 UTC (permalink / raw)
To: Linux PM
Cc: Linux ACPI, LKML, Len Brown, Srinivas Pandruvada, Thomas Gleixner,
Rajneesh Bhardwaj, David E. Box
In-Reply-To: <71085220.z6FKkvYQPX@kreacher>
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Introduce a new function, rearm_wake_irq(), allowing a wakeup IRQ
to be armed for systen wakeup detection again without running any
action handlers associated with it after it has been armed for
wakeup detection and triggered.
That is useful for IRQs, like ACPI SCI, that may deliver wakeup
as well as non-wakeup interrupts when armed for systen wakeup
detection. In those cases, it may be possible to determine whether
or not the delivered interrupt is a systen wakeup one without
running the entire action handler (or handlers, if the IRQ is
shared) for the IRQ, and if the interrupt turns out to be a
non-wakeup one, the IRQ can be rearmed with the help of the
new function.
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
include/linux/interrupt.h | 1 +
kernel/irq/pm.c | 20 ++++++++++++++++++++
2 files changed, 21 insertions(+)
Index: linux-pm/kernel/irq/pm.c
===================================================================
--- linux-pm.orig/kernel/irq/pm.c
+++ linux-pm/kernel/irq/pm.c
@@ -177,6 +177,26 @@ static void resume_irqs(bool want_early)
}
/**
+ * rearm_wake_irq - rearm a wakeup interrupt line after signaling wakeup
+ * @irq: Interrupt to rearm
+ */
+void rearm_wake_irq(unsigned int irq)
+{
+ unsigned long flags;
+ struct irq_desc *desc = irq_get_desc_buslock(irq, &flags, IRQ_GET_DESC_CHECK_GLOBAL);
+
+ if (!desc || !(desc->istate & IRQS_SUSPENDED) ||
+ !irqd_is_wakeup_set(&desc->irq_data))
+ return;
+
+ desc->istate &= ~IRQS_SUSPENDED;
+ irqd_set(&desc->irq_data, IRQD_WAKEUP_ARMED);
+ __enable_irq(desc);
+
+ irq_put_desc_busunlock(desc, flags);
+}
+
+/**
* irq_pm_syscore_ops - enable interrupt lines early
*
* Enable all interrupt lines with %IRQF_EARLY_RESUME set.
Index: linux-pm/include/linux/interrupt.h
===================================================================
--- linux-pm.orig/include/linux/interrupt.h
+++ linux-pm/include/linux/interrupt.h
@@ -238,6 +238,7 @@ extern void teardown_percpu_nmi(unsigned
/* The following three functions are for the core kernel use only. */
extern void suspend_device_irqs(void);
extern void resume_device_irqs(void);
+extern void rearm_wake_irq(unsigned int irq);
/**
* struct irq_affinity_notify - context for notification of IRQ affinity changes
^ permalink raw reply
* [PATCH 2/8] ACPICA: Return u32 from acpi_dispatch_gpe()
From: Rafael J. Wysocki @ 2019-07-16 16:11 UTC (permalink / raw)
To: Linux PM
Cc: Linux ACPI, LKML, Len Brown, Srinivas Pandruvada, Thomas Gleixner,
Rajneesh Bhardwaj, David E. Box
In-Reply-To: <71085220.z6FKkvYQPX@kreacher>
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
In some cases it is useful to know whether or not the
acpi_ev_detect_gpe() called by acpi_dispatch_gpe() has found
the GPE to be active, so return the return value of it (whose
data type is u32) from latter.
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
drivers/acpi/acpica/evxfgpe.c | 6 +++---
include/acpi/acpixf.h | 8 +++++++-
2 files changed, 10 insertions(+), 4 deletions(-)
Index: linux-pm/drivers/acpi/acpica/evxfgpe.c
===================================================================
--- linux-pm.orig/drivers/acpi/acpica/evxfgpe.c
+++ linux-pm/drivers/acpi/acpica/evxfgpe.c
@@ -644,17 +644,17 @@ ACPI_EXPORT_SYMBOL(acpi_get_gpe_status)
* PARAMETERS: gpe_device - Parent GPE Device. NULL for GPE0/GPE1
* gpe_number - GPE level within the GPE block
*
- * RETURN: None
+ * RETURN: INTERRUPT_HANDLED or INTERRUPT_NOT_HANDLED
*
* DESCRIPTION: Detect and dispatch a General Purpose Event to either a function
* (e.g. EC) or method (e.g. _Lxx/_Exx) handler.
*
******************************************************************************/
-void acpi_dispatch_gpe(acpi_handle gpe_device, u32 gpe_number)
+u32 acpi_dispatch_gpe(acpi_handle gpe_device, u32 gpe_number)
{
ACPI_FUNCTION_TRACE(acpi_dispatch_gpe);
- acpi_ev_detect_gpe(gpe_device, NULL, gpe_number);
+ return acpi_ev_detect_gpe(gpe_device, NULL, gpe_number);
}
ACPI_EXPORT_SYMBOL(acpi_dispatch_gpe)
Index: linux-pm/include/acpi/acpixf.h
===================================================================
--- linux-pm.orig/include/acpi/acpixf.h
+++ linux-pm/include/acpi/acpixf.h
@@ -297,6 +297,9 @@ ACPI_GLOBAL(u8, acpi_gbl_system_awake_an
#define ACPI_HW_DEPENDENT_RETURN_OK(prototype) \
ACPI_EXTERNAL_RETURN_OK(prototype)
+#define ACPI_HW_DEPENDENT_RETURN_UINT32(prototype) \
+ ACPI_EXTERNAL_RETURN_UINT32(prototype)
+
#define ACPI_HW_DEPENDENT_RETURN_VOID(prototype) \
ACPI_EXTERNAL_RETURN_VOID(prototype)
@@ -307,6 +310,9 @@ ACPI_GLOBAL(u8, acpi_gbl_system_awake_an
#define ACPI_HW_DEPENDENT_RETURN_OK(prototype) \
static ACPI_INLINE prototype {return(AE_OK);}
+#define ACPI_HW_DEPENDENT_RETURN_UINT32(prototype) \
+ static ACPI_INLINE prototype {return(0);}
+
#define ACPI_HW_DEPENDENT_RETURN_VOID(prototype) \
static ACPI_INLINE prototype {return;}
@@ -738,7 +744,7 @@ ACPI_HW_DEPENDENT_RETURN_STATUS(acpi_sta
u32 gpe_number,
acpi_event_status
*event_status))
-ACPI_HW_DEPENDENT_RETURN_VOID(void acpi_dispatch_gpe(acpi_handle gpe_device, u32 gpe_number))
+ACPI_HW_DEPENDENT_RETURN_UINT32(u32 acpi_dispatch_gpe(acpi_handle gpe_device, u32 gpe_number))
ACPI_HW_DEPENDENT_RETURN_STATUS(acpi_status acpi_disable_all_gpes(void))
ACPI_HW_DEPENDENT_RETURN_STATUS(acpi_status acpi_enable_all_runtime_gpes(void))
ACPI_HW_DEPENDENT_RETURN_STATUS(acpi_status acpi_enable_all_wakeup_gpes(void))
^ permalink raw reply
* [PATCH 0/8] PM / ACPI: sleep: Simplify the suspend-to-idle control flow
From: Rafael J. Wysocki @ 2019-07-16 16:08 UTC (permalink / raw)
To: Linux PM
Cc: Linux ACPI, LKML, Len Brown, Srinivas Pandruvada, Thomas Gleixner,
Rajneesh Bhardwaj, David E. Box
Hi All,
The rationale for these changes is explained in the changelog of patch [6/8] as follows:
"After commit 33e4f80ee69b ("ACPI / PM: Ignore spurious SCI wakeups
from suspend-to-idle") the "noirq" phases of device suspend and
resume may run for multiple times during suspend-to-idle, if there
are spurious system wakeup events while suspended. However, this
is complicated and fragile and actually unnecessary.
The main reason for doing this is that on some systems the EC may
signal system wakeup events (power button events, for example) as
well as events that should not cause the system to resume (spurious
system wakeup events). Thus, in order to determine whether or not
a given event signaled by the EC while suspended is a proper system
wakeup one, the EC GPE needs to be dispatched and to start with that
was achieved by allowing the ACPI SCI action handler to run, which
was only possible after calling resume_device_irqs().
However, dispatching the EC GPE this way turned out to take too much
time in some cases and some EC events might be missed due to that, so
commit 68e22011856f ("ACPI: EC: Dispatch the EC GPE directly on
s2idle wake") started to dispatch the EC GPE right after a wakeup
event has been detected, so in fact the full ACPI SCI action handler
doesn't need to run any more to deal with the wakeups coming from the
EC.
Use this observation to simplify the suspend-to-idle control flow
so that the "noirq" phases of device suspend and resume are each
run only once in every suspend-to-idle cycle, which is reported to
significantly reduce power drawn by some systems when suspended to
idle (by allowing them to reach a deep platform-wide low-power state
through the suspend-to-idle flow)."
A bonus is that after the essential changes the s2idle flow can be
integrated back into the generic suspend/resume one (patch [7/8])
and some simplifications can be made in drivers/base/power/main.c
after that (patch [8/8]).
Patches [1-5/8] are pre-requisite and the changes made by the first
three of them really take effect after applying patch [6/8]. Patch
[4/8], in turn, is a fix and patch [5/8] is an extra simplification.
Please refer to the changelogs for details.
For easier testing, this series is available from the git branch at:
git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git pm-s2idle-rework
Thanks,
Rafael
^ permalink raw reply
* [PATCH 7/8] PM: sleep: Integrate suspend-to-idle with generig suspend flow
From: Rafael J. Wysocki @ 2019-07-16 16:20 UTC (permalink / raw)
To: Linux PM
Cc: Linux ACPI, LKML, Len Brown, Srinivas Pandruvada, Thomas Gleixner,
Rajneesh Bhardwaj, David E. Box
In-Reply-To: <71085220.z6FKkvYQPX@kreacher>
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
After previous changes the suspend-to-idle code flow can be
integrated more tightly with the generic system suspend code flow
by making suspend_enter() call s2idle_loop() later and removing
the direct invocations of dpm_noirq_begin(),
dpm_noirq_suspend_devices(), dpm_noirq_end(), and
dpm_noirq_resume_devices() from the latter, so do that.
This change is not expected to alter functionality.
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
kernel/power/suspend.c | 21 +++++----------------
1 file changed, 5 insertions(+), 16 deletions(-)
Index: linux-pm/kernel/power/suspend.c
===================================================================
--- linux-pm.orig/kernel/power/suspend.c
+++ linux-pm/kernel/power/suspend.c
@@ -119,13 +119,6 @@ static void s2idle_enter(void)
static void s2idle_loop(void)
{
- int error;
-
- dpm_noirq_begin();
- error = dpm_noirq_suspend_devices(PMSG_SUSPEND);
- if (error)
- goto resume;
-
pm_pr_dbg("suspend-to-idle\n");
/*
@@ -150,10 +143,6 @@ static void s2idle_loop(void)
}
pm_pr_dbg("resume from suspend-to-idle\n");
-
-resume:
- dpm_noirq_resume_devices(PMSG_RESUME);
- dpm_noirq_end();
}
void s2idle_wake(void)
@@ -408,11 +397,6 @@ static int suspend_enter(suspend_state_t
if (error)
goto Devices_early_resume;
- if (state == PM_SUSPEND_TO_IDLE && pm_test_level != TEST_PLATFORM) {
- s2idle_loop();
- goto Platform_early_resume;
- }
-
error = dpm_suspend_noirq(PMSG_SUSPEND);
if (error) {
pr_err("noirq suspend of devices failed\n");
@@ -425,6 +409,11 @@ static int suspend_enter(suspend_state_t
if (suspend_test(TEST_PLATFORM))
goto Platform_wake;
+ if (state == PM_SUSPEND_TO_IDLE) {
+ s2idle_loop();
+ goto Platform_wake;
+ }
+
error = suspend_disable_secondary_cpus();
if (error || suspend_test(TEST_CPUS))
goto Enable_cpus;
^ permalink raw reply
* Re: [PATCH v11 4/5] sched/core: uclamp: Use TG's clamps to restrict TASK's clamps
From: Michal Koutný @ 2019-07-16 15:58 UTC (permalink / raw)
To: Patrick Bellasi
Cc: Alessio Balsini, Dietmar Eggemann, Morten Rasmussen,
Quentin Perret, Joel Fernandes, Paul Turner, Steve Muckle,
Suren Baghdasaryan, Todd Kjos, Peter Zijlstra, Rafael J . Wysocki,
Tejun Heo, Vincent Guittot, Viresh Kumar, Juri Lelli, Ingo Molnar,
linux-kernel, linux-pm
In-Reply-To: <20190716143435.iwwd6fjr3udlqol4@e110439-lin>
On Tue, Jul 16, 2019 at 03:34:35PM +0100, Patrick Bellasi <patrick.bellasi@arm.com> wrote:
> Am I missing something?
No, it's rather my misinterpretation of the syscall semantics.
> Otherwise, I think the changelog sentence you quoted is just
> misleading.
It certainly mislead me to thinking about the sched_setattr calls as
requests of utilization being in the given interval (substituting 0 or 1 when
only one boundary is given, and further constrained by tg's interval).
I see your point, those are actually two (mostly) independent controls.
Makes sense now.
Thanks,
Michal
^ permalink raw reply
* Re: [PATCH 14/18] drivers: firmware: psci: Manage runtime PM in the idle path for CPUs
From: Lorenzo Pieralisi @ 2019-07-16 15:53 UTC (permalink / raw)
To: Ulf Hansson
Cc: Sudeep Holla, Mark Rutland, linux-arm-kernel, Rafael J . Wysocki,
Daniel Lezcano, Raju P . L . S . S . S . N, Amit Kucheria,
Bjorn Andersson, Stephen Boyd, Niklas Cassel, Tony Lindgren,
Kevin Hilman, Lina Iyer, Viresh Kumar, Vincent Guittot,
Geert Uytterhoeven, Souvik Chakravarty, linux-pm, linux-arm-msm,
linux-kernel
In-Reply-To: <20190513192300.653-15-ulf.hansson@linaro.org>
On Mon, May 13, 2019 at 09:22:56PM +0200, Ulf Hansson wrote:
> When the hierarchical CPU topology layout is used in DT, let's allow the
> CPU to be power managed through its PM domain, via deploying runtime PM
> support.
>
> To know for which idle states runtime PM reference counting is needed,
> let's store the index of deepest idle state for the CPU, in a per CPU
> variable. This allows psci_cpu_suspend_enter() to compare this index with
> the requested idle state index and then act accordingly.
I do not see why a system with two CPU CPUidle states, say CPU retention
and CPU shutdown, should not be calling runtime PM on CPU retention
entry.
The question then is what cluster/package/system states
are allowed for a given CPU idle state, to understand
what idle states can be actually entered at any hierarchy
level given the choice made for the CPU idle state.
In the case above, a CPU entering retention state should prevent
runtime PM selecting a cluster shutdown state; most likely firmware
would demote the request to cluster retention but still, we should
find a way to describe these dependencies.
Thanks,
Lorenzo
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>
> Changes:
> - Simplify the code by using the new per CPU struct, that stores the
> needed struct device*.
>
> ---
> drivers/firmware/psci/psci.c | 22 ++++++++++++++++++++--
> 1 file changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c
> index 54e23d4ed0ea..2c4157d3a616 100644
> --- a/drivers/firmware/psci/psci.c
> +++ b/drivers/firmware/psci/psci.c
> @@ -20,6 +20,7 @@
> #include <linux/linkage.h>
> #include <linux/of.h>
> #include <linux/pm.h>
> +#include <linux/pm_runtime.h>
> #include <linux/printk.h>
> #include <linux/psci.h>
> #include <linux/reboot.h>
> @@ -298,6 +299,7 @@ static int __init psci_features(u32 psci_func_id)
>
> struct psci_cpuidle_data {
> u32 *psci_states;
> + u32 rpm_state_id;
> struct device *dev;
> };
>
> @@ -385,6 +387,7 @@ static int psci_dt_cpu_init_idle(struct cpuidle_driver *drv,
> goto free_mem;
>
> data->dev = dev;
> + data->rpm_state_id = drv->state_count - 1;
> }
>
> /* Idle states parsed correctly, store them in the per-cpu struct. */
> @@ -481,8 +484,11 @@ static int psci_suspend_finisher(unsigned long index)
> int psci_cpu_suspend_enter(unsigned long index)
> {
> int ret;
> - u32 *state = __this_cpu_read(psci_cpuidle_data.psci_states);
> - u32 composite_state = state[index - 1] | psci_get_domain_state();
> + struct psci_cpuidle_data *data = this_cpu_ptr(&psci_cpuidle_data);
> + u32 *states = data->psci_states;
> + struct device *dev = data->dev;
> + bool runtime_pm = (dev && data->rpm_state_id == index);
> + u32 composite_state;
>
> /*
> * idle state index 0 corresponds to wfi, should never be called
> @@ -491,11 +497,23 @@ int psci_cpu_suspend_enter(unsigned long index)
> if (WARN_ON_ONCE(!index))
> return -EINVAL;
>
> + /*
> + * Do runtime PM if we are using the hierarchical CPU toplogy, but only
> + * when cpuidle have selected the deepest idle state for the CPU.
> + */
> + if (runtime_pm)
> + pm_runtime_put_sync_suspend(dev);
> +
> + composite_state = states[index - 1] | psci_get_domain_state();
> +
> if (!psci_power_state_loses_context(composite_state))
> ret = psci_ops.cpu_suspend(composite_state, 0);
> else
> ret = cpu_suspend(index, psci_suspend_finisher);
>
> + if (runtime_pm)
> + pm_runtime_get_sync(dev);
> +
> /* Clear the domain state to start fresh when back from idle. */
> psci_set_domain_state(0);
>
> --
> 2.17.1
>
^ permalink raw reply
* Re: [PATCH v11 3/5] sched/core: uclamp: Propagate system defaults to root group
From: Joel Fernandes @ 2019-07-16 15:46 UTC (permalink / raw)
To: Patrick Bellasi
Cc: Michal Koutný, LKML, Linux PM, Ingo Molnar, Peter Zijlstra,
Tejun Heo, Rafael J . Wysocki, Vincent Guittot, Viresh Kumar,
Paul Turner, Quentin Perret, Dietmar Eggemann, Morten Rasmussen,
Juri Lelli, Todd Kjos, Steve Muckle, Suren Baghdasaryan,
Alessio Balsini
In-Reply-To: <20190716143417.us3xhksrsaxsl2ok@e110439-lin>
On Tue, Jul 16, 2019 at 10:34 AM Patrick Bellasi
<patrick.bellasi@arm.com> wrote:
>
> On 15-Jul 18:42, Michal Koutný wrote:
> > On Mon, Jul 08, 2019 at 09:43:55AM +0100, Patrick Bellasi <patrick.bellasi@arm.com> wrote:
> > > +static void uclamp_update_root_tg(void)
> > > +{
> > > + struct task_group *tg = &root_task_group;
> > > +
> > > + uclamp_se_set(&tg->uclamp_req[UCLAMP_MIN],
> > > + sysctl_sched_uclamp_util_min, false);
> > > + uclamp_se_set(&tg->uclamp_req[UCLAMP_MAX],
> > > + sysctl_sched_uclamp_util_max, false);
> > > +
> > > + cpu_util_update_eff(&root_task_group.css);
> > > +}
> > cpu_util_update_eff internally calls css_for_each_descendant_pre() so
> > this should be protected with rcu_read_lock().
>
> Right, good catch! Will add in v12.
>
Hopefully these can catch it in the near future!
https://lore.kernel.org/rcu/20190715143705.117908-1-joel@joelfernandes.org/T/#t
^ permalink raw reply
* Re: [PATCH v11 3/5] sched/core: uclamp: Propagate system defaults to root group
From: Michal Koutný @ 2019-07-16 15:36 UTC (permalink / raw)
To: Patrick Bellasi
Cc: Alessio Balsini, Dietmar Eggemann, Morten Rasmussen,
Quentin Perret, Joel Fernandes, Paul Turner, Steve Muckle,
Suren Baghdasaryan, Todd Kjos, Peter Zijlstra, Rafael J . Wysocki,
Tejun Heo, Vincent Guittot, Viresh Kumar, Juri Lelli, Ingo Molnar,
linux-kernel, linux-pm
In-Reply-To: <20190716143417.us3xhksrsaxsl2ok@e110439-lin>
On Tue, Jul 16, 2019 at 03:34:17PM +0100, Patrick Bellasi <patrick.bellasi@arm.com> wrote:
> > cpu_util_update_eff internally calls css_for_each_descendant_pre() so
> > this should be protected with rcu_read_lock().
>
> Right, good catch! Will add in v12.
When I responded to your other patch, it occurred to me that since
cpu_util_update_eff goes writing down to child csses, it should take
also uclamp_mutex here to avoid race with direct cgroup attribute
writers.
^ permalink raw reply
* Re: [PATCH v11 2/5] sched/core: uclamp: Propagate parent clamps
From: Michal Koutný @ 2019-07-16 15:29 UTC (permalink / raw)
To: Patrick Bellasi
Cc: Alessio Balsini, Dietmar Eggemann, Morten Rasmussen,
Quentin Perret, Joel Fernandes, Paul Turner, Steve Muckle,
Suren Baghdasaryan, Todd Kjos, Peter Zijlstra, Rafael J . Wysocki,
Tejun Heo, Vincent Guittot, Viresh Kumar, Juri Lelli, Ingo Molnar,
linux-kernel, linux-pm
In-Reply-To: <20190716140706.vuggfigjlys44lkp@e110439-lin>
On Tue, Jul 16, 2019 at 03:07:06PM +0100, Patrick Bellasi <patrick.bellasi@arm.com> wrote:
> That note comes from the previous review cycle and it's based on a
> request from Tejun to align uclamp behaviors with the way the
> delegation model is supposed to work.
I saw and hopefully understood that reasoning -- uclamp.min has the
protection semantics and uclamp.max the limit semantics.
However, what took me some time to comprehend when the effected
uclamp.min and uclamp.max cross over, i.e. that uclamp.min is then bound
by uclamp.max (besides parent's uclamp.min). Your commit message
explains that and I think it's relevant for the kernel docs file
itself.
> You right, the synchronization is introduced by a later patch:
>
> sched/core: uclamp: Update CPU's refcount on TG's clamp changes
I saw that lock but didn't realize __setscheduler_uclamp() touches only
task's struct uclamp_se, none of task_group's/css's (which is under
uclamp_mutex). That seems correct.
^ permalink raw reply
* [PATCH] cpuidle: Always stop scheduler tick on adaptive-tick CPUs
From: Rafael J. Wysocki @ 2019-07-16 15:25 UTC (permalink / raw)
To: Linux PM; +Cc: Thomas Lindroth, LKML, Peter Zijlstra, Frederic Weisbecker
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Running the scheduler tick on idle adaptive-tick CPUs is not useful
and it may also be not expected by users (as reported by Thomas), so
add a check to cpuidle_idle_call() to always stop the tick on them
regardless of the idle duration predicted by the governor.
Fixes: 554c8aa8ecad ("sched: idle: Select idle state before stopping the tick")
Reported-by: Thomas Lindroth <thomas.lindroth@gmail.com>
Tested-by: Thomas Lindroth <thomas.lindroth@gmail.com>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
kernel/sched/idle.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
Index: linux-pm/kernel/sched/idle.c
===================================================================
--- linux-pm.orig/kernel/sched/idle.c
+++ linux-pm/kernel/sched/idle.c
@@ -191,7 +191,8 @@ static void cpuidle_idle_call(void)
*/
next_state = cpuidle_select(drv, dev, &stop_tick);
- if (stop_tick || tick_nohz_tick_stopped())
+ if (stop_tick || tick_nohz_tick_stopped() ||
+ !housekeeping_cpu(dev->cpu, HK_FLAG_TICK))
tick_nohz_idle_stop_tick();
else
tick_nohz_idle_retain_tick();
^ permalink raw reply
* [PATCH v2] driver core: Remove device link creation limitation
From: Rafael J. Wysocki @ 2019-07-16 15:21 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: LKML, Linux PM, Saravana Kannan, Lukas Wunner, Jon Hunter,
Ulf Hansson, Marek Szyprowski
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Subject: [PATCH] driver core: Remove device link creation limitation
If device_link_add() is called for a consumer/supplier pair with an
existing device link between them and the existing link's type is
not in agreement with the flags passed to that function by its
caller, NULL will be returned. That is seriously inconvenient,
because it forces the callers of device_link_add() to worry about
what others may or may not do even if that is not relevant to them
for any other reasons.
It turns out, however, that this limitation can be made go away
relatively easily.
The underlying observation is that if DL_FLAG_STATELESS has been
passed to device_link_add() in flags for the given consumer/supplier
pair at least once, calling either device_link_del() or
device_link_remove() to release the link returned by it should work,
but there are no other requirements associated with that flag. In
turn, if at least one of the callers of device_link_add() for the
given consumer/supplier pair has not passed DL_FLAG_STATELESS to it
in flags, the driver core should track the status of the link and act
on it as appropriate (ie. the link should be treated as "managed").
This means that DL_FLAG_STATELESS needs to be set for managed device
links and it should be valid to call device_link_del() or
device_link_remove() to drop references to them in certain
sutiations.
To allow that to happen, introduce a new (internal) device link flag
called DL_FLAG_MANAGED and make device_link_add() set it automatically
whenever DL_FLAG_STATELESS is not passed to it. Also make it take
additional references to existing device links that were previously
stateless (that is, with DL_FLAG_STATELESS set and DL_FLAG_MANAGED
unset) and will need to be managed going forward and initialize
their status (which has been DL_STATE_NONE so far).
Accordingly, when a managed device link is dropped automatically
by the driver core, make it clear DL_FLAG_MANAGED, reset the link's
status back to DL_STATE_NONE and drop the reference to it associated
with DL_FLAG_MANAGED instead of just deleting it right away (to
allow it to stay around in case it still needs to be released
explicitly by someone).
With that, since setting DL_FLAG_STATELESS doesn't mean that the
device link in question is not managed any more, replace all of the
status-tracking checks against DL_FLAG_STATELESS with analogous
checks against DL_FLAG_MANAGED and update the documentation to
reflect these changes.
While at it, make device_link_add() reject flags that it does not
recognize, including DL_FLAG_MANAGED.
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Reviewed-by: Saravana Kannan <saravanak@google.com>
Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
-> v2:
* Add a check to device_link_add() to return NULL if unrecognized flags are
passed to it.
* Modify kerneldoc comments around DL_FLAG_MANAGED.
I've tentatively added the Tested-by tag from Marek, because I don't expect
the changes made between the initial posting and the v2 to make any difference
for him.
---
Documentation/driver-api/device_link.rst | 4
drivers/base/core.c | 176 +++++++++++++++++--------------
drivers/base/power/runtime.c | 4
include/linux/device.h | 4
4 files changed, 106 insertions(+), 82 deletions(-)
Index: linux-pm/drivers/base/core.c
===================================================================
--- linux-pm.orig/drivers/base/core.c
+++ linux-pm/drivers/base/core.c
@@ -124,6 +124,50 @@ static int device_is_dependent(struct de
return ret;
}
+static void device_link_init_status(struct device_link *link,
+ struct device *consumer,
+ struct device *supplier)
+{
+ switch (supplier->links.status) {
+ case DL_DEV_PROBING:
+ switch (consumer->links.status) {
+ case DL_DEV_PROBING:
+ /*
+ * A consumer driver can create a link to a supplier
+ * that has not completed its probing yet as long as it
+ * knows that the supplier is already functional (for
+ * example, it has just acquired some resources from the
+ * supplier).
+ */
+ link->status = DL_STATE_CONSUMER_PROBE;
+ break;
+ default:
+ link->status = DL_STATE_DORMANT;
+ break;
+ }
+ break;
+ case DL_DEV_DRIVER_BOUND:
+ switch (consumer->links.status) {
+ case DL_DEV_PROBING:
+ link->status = DL_STATE_CONSUMER_PROBE;
+ break;
+ case DL_DEV_DRIVER_BOUND:
+ link->status = DL_STATE_ACTIVE;
+ break;
+ default:
+ link->status = DL_STATE_AVAILABLE;
+ break;
+ }
+ break;
+ case DL_DEV_UNBINDING:
+ link->status = DL_STATE_SUPPLIER_UNBIND;
+ break;
+ default:
+ link->status = DL_STATE_DORMANT;
+ break;
+ }
+}
+
static int device_reorder_to_tail(struct device *dev, void *not_used)
{
struct device_link *link;
@@ -165,6 +209,10 @@ void device_pm_move_to_tail(struct devic
device_links_read_unlock(idx);
}
+#define DL_MANAGED_LINK_FLAGS (DL_FLAG_AUTOREMOVE_CONSUMER | \
+ DL_FLAG_AUTOREMOVE_SUPPLIER | \
+ DL_FLAG_AUTOPROBE_CONSUMER)
+
/**
* device_link_add - Create a link between two devices.
* @consumer: Consumer end of the link.
@@ -179,9 +227,9 @@ void device_pm_move_to_tail(struct devic
* of the link. If DL_FLAG_PM_RUNTIME is not set, DL_FLAG_RPM_ACTIVE will be
* ignored.
*
- * If DL_FLAG_STATELESS is set in @flags, the link is not going to be managed by
- * the driver core and, in particular, the caller of this function is expected
- * to drop the reference to the link acquired by it directly.
+ * If DL_FLAG_STATELESS is set in @flags, the caller of this function is
+ * expected to release the link returned by it directly with the help of either
+ * device_link_del() or device_link_remove().
*
* If that flag is not set, however, the caller of this function is handing the
* management of the link over to the driver core entirely and its return value
@@ -201,9 +249,16 @@ void device_pm_move_to_tail(struct devic
* be used to request the driver core to automaticall probe for a consmer
* driver after successfully binding a driver to the supplier device.
*
- * The combination of DL_FLAG_STATELESS and either DL_FLAG_AUTOREMOVE_CONSUMER
- * or DL_FLAG_AUTOREMOVE_SUPPLIER set in @flags at the same time is invalid and
- * will cause NULL to be returned upfront.
+ * The combination of DL_FLAG_STATELESS and one of DL_FLAG_AUTOREMOVE_CONSUMER,
+ * DL_FLAG_AUTOREMOVE_SUPPLIER, or DL_FLAG_AUTOPROBE_CONSUMER set in @flags at
+ * the same time is invalid and will cause NULL to be returned upfront.
+ * However, if a device link between the given @consumer and @supplier pair
+ * exists already when this function is called for them, the existing link will
+ * be returned regardless of its current type and status (the link's flags may
+ * be modified then). The caller of this function is then expected to treat
+ * the link as though it has just been created, so (in particular) if
+ * DL_FLAG_STATELESS was passed in @flags, the link needs to be released
+ * explicitly when not needed any more (as stated above).
*
* A side effect of the link creation is re-ordering of dpm_list and the
* devices_kset list by moving the consumer device and all devices depending
@@ -220,10 +275,8 @@ struct device_link *device_link_add(stru
struct device_link *link;
if (!consumer || !supplier ||
- (flags & DL_FLAG_STATELESS &&
- flags & (DL_FLAG_AUTOREMOVE_CONSUMER |
- DL_FLAG_AUTOREMOVE_SUPPLIER |
- DL_FLAG_AUTOPROBE_CONSUMER)) ||
+ (flags & ~(DL_FLAG_STATELESS | DL_MANAGED_LINK_FLAGS)) ||
+ (flags & DL_FLAG_STATELESS && flags & DL_MANAGED_LINK_FLAGS) ||
(flags & DL_FLAG_AUTOPROBE_CONSUMER &&
flags & (DL_FLAG_AUTOREMOVE_CONSUMER |
DL_FLAG_AUTOREMOVE_SUPPLIER)))
@@ -236,6 +289,9 @@ struct device_link *device_link_add(stru
}
}
+ if (!(flags & DL_FLAG_STATELESS))
+ flags |= DL_FLAG_MANAGED;
+
device_links_write_lock();
device_pm_lock();
@@ -262,15 +318,6 @@ struct device_link *device_link_add(stru
if (link->consumer != consumer)
continue;
- /*
- * Don't return a stateless link if the caller wants a stateful
- * one and vice versa.
- */
- if (WARN_ON((flags & DL_FLAG_STATELESS) != (link->flags & DL_FLAG_STATELESS))) {
- link = NULL;
- goto out;
- }
-
if (flags & DL_FLAG_PM_RUNTIME) {
if (!(link->flags & DL_FLAG_PM_RUNTIME)) {
pm_runtime_new_link(consumer);
@@ -281,6 +328,7 @@ struct device_link *device_link_add(stru
}
if (flags & DL_FLAG_STATELESS) {
+ link->flags |= DL_FLAG_STATELESS;
kref_get(&link->kref);
goto out;
}
@@ -299,6 +347,11 @@ struct device_link *device_link_add(stru
link->flags &= ~(DL_FLAG_AUTOREMOVE_CONSUMER |
DL_FLAG_AUTOREMOVE_SUPPLIER);
}
+ if (!(link->flags & DL_FLAG_MANAGED)) {
+ kref_get(&link->kref);
+ link->flags |= DL_FLAG_MANAGED;
+ device_link_init_status(link, consumer, supplier);
+ }
goto out;
}
@@ -325,48 +378,10 @@ struct device_link *device_link_add(stru
kref_init(&link->kref);
/* Determine the initial link state. */
- if (flags & DL_FLAG_STATELESS) {
+ if (flags & DL_FLAG_STATELESS)
link->status = DL_STATE_NONE;
- } else {
- switch (supplier->links.status) {
- case DL_DEV_PROBING:
- switch (consumer->links.status) {
- case DL_DEV_PROBING:
- /*
- * A consumer driver can create a link to a
- * supplier that has not completed its probing
- * yet as long as it knows that the supplier is
- * already functional (for example, it has just
- * acquired some resources from the supplier).
- */
- link->status = DL_STATE_CONSUMER_PROBE;
- break;
- default:
- link->status = DL_STATE_DORMANT;
- break;
- }
- break;
- case DL_DEV_DRIVER_BOUND:
- switch (consumer->links.status) {
- case DL_DEV_PROBING:
- link->status = DL_STATE_CONSUMER_PROBE;
- break;
- case DL_DEV_DRIVER_BOUND:
- link->status = DL_STATE_ACTIVE;
- break;
- default:
- link->status = DL_STATE_AVAILABLE;
- break;
- }
- break;
- case DL_DEV_UNBINDING:
- link->status = DL_STATE_SUPPLIER_UNBIND;
- break;
- default:
- link->status = DL_STATE_DORMANT;
- break;
- }
- }
+ else
+ device_link_init_status(link, consumer, supplier);
/*
* Some callers expect the link creation during consumer driver probe to
@@ -528,7 +543,7 @@ static void device_links_missing_supplie
* mark the link as "consumer probe in progress" to make the supplier removal
* wait for us to complete (or bad things may happen).
*
- * Links with the DL_FLAG_STATELESS flag set are ignored.
+ * Links without the DL_FLAG_MANAGED flag set are ignored.
*/
int device_links_check_suppliers(struct device *dev)
{
@@ -538,7 +553,7 @@ int device_links_check_suppliers(struct
device_links_write_lock();
list_for_each_entry(link, &dev->links.suppliers, c_node) {
- if (link->flags & DL_FLAG_STATELESS)
+ if (!(link->flags & DL_FLAG_MANAGED))
continue;
if (link->status != DL_STATE_AVAILABLE) {
@@ -563,7 +578,7 @@ int device_links_check_suppliers(struct
*
* Also change the status of @dev's links to suppliers to "active".
*
- * Links with the DL_FLAG_STATELESS flag set are ignored.
+ * Links without the DL_FLAG_MANAGED flag set are ignored.
*/
void device_links_driver_bound(struct device *dev)
{
@@ -572,7 +587,7 @@ void device_links_driver_bound(struct de
device_links_write_lock();
list_for_each_entry(link, &dev->links.consumers, s_node) {
- if (link->flags & DL_FLAG_STATELESS)
+ if (!(link->flags & DL_FLAG_MANAGED))
continue;
/*
@@ -593,7 +608,7 @@ void device_links_driver_bound(struct de
}
list_for_each_entry(link, &dev->links.suppliers, c_node) {
- if (link->flags & DL_FLAG_STATELESS)
+ if (!(link->flags & DL_FLAG_MANAGED))
continue;
WARN_ON(link->status != DL_STATE_CONSUMER_PROBE);
@@ -605,6 +620,13 @@ void device_links_driver_bound(struct de
device_links_write_unlock();
}
+static void device_link_drop_managed(struct device_link *link)
+{
+ link->flags &= ~DL_FLAG_MANAGED;
+ WRITE_ONCE(link->status, DL_STATE_NONE);
+ kref_put(&link->kref, __device_link_del);
+}
+
/**
* __device_links_no_driver - Update links of a device without a driver.
* @dev: Device without a drvier.
@@ -615,18 +637,18 @@ void device_links_driver_bound(struct de
* unless they already are in the "supplier unbind in progress" state in which
* case they need not be updated.
*
- * Links with the DL_FLAG_STATELESS flag set are ignored.
+ * Links without the DL_FLAG_MANAGED flag set are ignored.
*/
static void __device_links_no_driver(struct device *dev)
{
struct device_link *link, *ln;
list_for_each_entry_safe_reverse(link, ln, &dev->links.suppliers, c_node) {
- if (link->flags & DL_FLAG_STATELESS)
+ if (!(link->flags & DL_FLAG_MANAGED))
continue;
if (link->flags & DL_FLAG_AUTOREMOVE_CONSUMER)
- __device_link_del(&link->kref);
+ device_link_drop_managed(link);
else if (link->status == DL_STATE_CONSUMER_PROBE ||
link->status == DL_STATE_ACTIVE)
WRITE_ONCE(link->status, DL_STATE_AVAILABLE);
@@ -643,7 +665,7 @@ static void __device_links_no_driver(str
* %__device_links_no_driver() to update links to suppliers for it as
* appropriate.
*
- * Links with the DL_FLAG_STATELESS flag set are ignored.
+ * Links without the DL_FLAG_MANAGED flag set are ignored.
*/
void device_links_no_driver(struct device *dev)
{
@@ -652,7 +674,7 @@ void device_links_no_driver(struct devic
device_links_write_lock();
list_for_each_entry(link, &dev->links.consumers, s_node) {
- if (link->flags & DL_FLAG_STATELESS)
+ if (!(link->flags & DL_FLAG_MANAGED))
continue;
/*
@@ -680,7 +702,7 @@ void device_links_no_driver(struct devic
* invoke %__device_links_no_driver() to update links to suppliers for it as
* appropriate.
*
- * Links with the DL_FLAG_STATELESS flag set are ignored.
+ * Links without the DL_FLAG_MANAGED flag set are ignored.
*/
void device_links_driver_cleanup(struct device *dev)
{
@@ -689,7 +711,7 @@ void device_links_driver_cleanup(struct
device_links_write_lock();
list_for_each_entry_safe(link, ln, &dev->links.consumers, s_node) {
- if (link->flags & DL_FLAG_STATELESS)
+ if (!(link->flags & DL_FLAG_MANAGED))
continue;
WARN_ON(link->flags & DL_FLAG_AUTOREMOVE_CONSUMER);
@@ -702,7 +724,7 @@ void device_links_driver_cleanup(struct
*/
if (link->status == DL_STATE_SUPPLIER_UNBIND &&
link->flags & DL_FLAG_AUTOREMOVE_SUPPLIER)
- __device_link_del(&link->kref);
+ device_link_drop_managed(link);
WRITE_ONCE(link->status, DL_STATE_DORMANT);
}
@@ -724,7 +746,7 @@ void device_links_driver_cleanup(struct
*
* Return 'false' if there are no probing or active consumers.
*
- * Links with the DL_FLAG_STATELESS flag set are ignored.
+ * Links without the DL_FLAG_MANAGED flag set are ignored.
*/
bool device_links_busy(struct device *dev)
{
@@ -734,7 +756,7 @@ bool device_links_busy(struct device *de
device_links_write_lock();
list_for_each_entry(link, &dev->links.consumers, s_node) {
- if (link->flags & DL_FLAG_STATELESS)
+ if (!(link->flags & DL_FLAG_MANAGED))
continue;
if (link->status == DL_STATE_CONSUMER_PROBE
@@ -764,7 +786,7 @@ bool device_links_busy(struct device *de
* driver to unbind and start over (the consumer will not re-probe as we have
* changed the state of the link already).
*
- * Links with the DL_FLAG_STATELESS flag set are ignored.
+ * Links without the DL_FLAG_MANAGED flag set are ignored.
*/
void device_links_unbind_consumers(struct device *dev)
{
@@ -776,7 +798,7 @@ void device_links_unbind_consumers(struc
list_for_each_entry(link, &dev->links.consumers, s_node) {
enum device_link_state status;
- if (link->flags & DL_FLAG_STATELESS)
+ if (!(link->flags & DL_FLAG_MANAGED))
continue;
status = link->status;
Index: linux-pm/drivers/base/power/runtime.c
===================================================================
--- linux-pm.orig/drivers/base/power/runtime.c
+++ linux-pm/drivers/base/power/runtime.c
@@ -1624,7 +1624,7 @@ void pm_runtime_remove(struct device *de
* runtime PM references to the device, drop the usage counter of the device
* (as many times as needed).
*
- * Links with the DL_FLAG_STATELESS flag set are ignored.
+ * Links with the DL_FLAG_MANAGED flag unset are ignored.
*
* Since the device is guaranteed to be runtime-active at the point this is
* called, nothing else needs to be done here.
@@ -1641,7 +1641,7 @@ void pm_runtime_clean_up_links(struct de
idx = device_links_read_lock();
list_for_each_entry_rcu(link, &dev->links.consumers, s_node) {
- if (link->flags & DL_FLAG_STATELESS)
+ if (!(link->flags & DL_FLAG_MANAGED))
continue;
while (refcount_dec_not_one(&link->rpm_active))
Index: linux-pm/include/linux/device.h
===================================================================
--- linux-pm.orig/include/linux/device.h
+++ linux-pm/include/linux/device.h
@@ -829,12 +829,13 @@ enum device_link_state {
/*
* Device link flags.
*
- * STATELESS: The core won't track the presence of supplier/consumer drivers.
+ * STATELESS: The core will not remove this link automatically.
* AUTOREMOVE_CONSUMER: Remove the link automatically on consumer driver unbind.
* PM_RUNTIME: If set, the runtime PM framework will use this link.
* RPM_ACTIVE: Run pm_runtime_get_sync() on the supplier during link creation.
* AUTOREMOVE_SUPPLIER: Remove the link automatically on supplier driver unbind.
* AUTOPROBE_CONSUMER: Probe consumer driver automatically after supplier binds.
+ * MANAGED: The core tracks presence of supplier/consumer drivers (internal).
*/
#define DL_FLAG_STATELESS BIT(0)
#define DL_FLAG_AUTOREMOVE_CONSUMER BIT(1)
@@ -842,6 +843,7 @@ enum device_link_state {
#define DL_FLAG_RPM_ACTIVE BIT(3)
#define DL_FLAG_AUTOREMOVE_SUPPLIER BIT(4)
#define DL_FLAG_AUTOPROBE_CONSUMER BIT(5)
+#define DL_FLAG_MANAGED BIT(6)
/**
* struct device_link - Device link representation.
Index: linux-pm/Documentation/driver-api/device_link.rst
===================================================================
--- linux-pm.orig/Documentation/driver-api/device_link.rst
+++ linux-pm/Documentation/driver-api/device_link.rst
@@ -78,8 +78,8 @@ typically deleted in its ``->remove`` ca
driver is compiled as a module, the device link is added on module load and
orderly deleted on unload. The same restrictions that apply to device link
addition (e.g. exclusion of a parallel suspend/resume transition) apply equally
-to deletion. Device links with ``DL_FLAG_STATELESS`` unset (i.e. managed
-device links) are deleted automatically by the driver core.
+to deletion. Device links managed by the driver core are deleted automatically
+by it.
Several flags may be specified on device link addition, two of which
have already been mentioned above: ``DL_FLAG_STATELESS`` to express that no
^ permalink raw reply
* Re: [PATCH v4 10/24] PM / devfreq: tegra30: Don't enable consecutive-down interrupt on startup
From: Dmitry Osipenko @ 2019-07-16 15:17 UTC (permalink / raw)
To: Chanwoo Choi, Thierry Reding, MyungJoo Ham, Kyungmin Park,
Jonathan Hunter, Tomeu Vizoso
Cc: linux-pm, linux-tegra, linux-kernel
In-Reply-To: <933b99a1-ac45-25fb-e8d5-0641ec0cab18@samsung.com>
16.07.2019 15:17, Chanwoo Choi пишет:
> Hi Dmitry,
>
> On 19. 7. 8. 오전 7:32, Dmitry Osipenko wrote:
>> The consecutive-down event tells that we should perform frequency
>> de-boosting, but boosting is in a reset state on start and hence the
>> event won't do anything useful for us and it will be just a dummy
>> interrupt request.
>>
>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>> ---
>> drivers/devfreq/tegra30-devfreq.c | 1 -
>> 1 file changed, 1 deletion(-)
>>
>> diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
>> index 32fe95458ee7..878c9396bb8c 100644
>> --- a/drivers/devfreq/tegra30-devfreq.c
>> +++ b/drivers/devfreq/tegra30-devfreq.c
>> @@ -558,7 +558,6 @@ static void tegra_actmon_configure_device(struct tegra_devfreq *tegra,
>> << ACTMON_DEV_CTRL_CONSECUTIVE_ABOVE_WMARK_NUM_SHIFT;
>> val |= ACTMON_DEV_CTRL_AVG_ABOVE_WMARK_EN;
>> val |= ACTMON_DEV_CTRL_AVG_BELOW_WMARK_EN;
>> - val |= ACTMON_DEV_CTRL_CONSECUTIVE_BELOW_WMARK_EN;
>> val |= ACTMON_DEV_CTRL_CONSECUTIVE_ABOVE_WMARK_EN;
>> val |= ACTMON_DEV_CTRL_ENB;
>>
>>
>
> Maybe, I think that better to review it by Thierry.
> I'm not sure it is right or not because it depend on h/w.
>
The CONSECUTIVE_LOWER interrupts are generated if
ACTMON_DEV_CTRL_CONSECUTIVE_BELOW_WMARK_EN is enabled and ISR enables
the BELOW_WMARK_EN bit once CONSECUTIVE_UPPER event is received. The
CONSECUTIVE events are used in for the frequency boosting logic and
there is no boosting on start, hence the CONSECUTIVE_LOWER event isn't
needed on start as well. Hope this helps.
^ permalink raw reply
* Re: [PATCH 09/18] drivers: firmware: psci: Add support for PM domains using genpd
From: Sudeep Holla @ 2019-07-16 15:05 UTC (permalink / raw)
To: Ulf Hansson
Cc: Lorenzo Pieralisi, Mark Rutland, linux-arm-kernel,
Rafael J . Wysocki, Daniel Lezcano, Raju P . L . S . S . S . N,
Amit Kucheria, Bjorn Andersson, Stephen Boyd, Niklas Cassel,
Tony Lindgren, Kevin Hilman, Lina Iyer, Viresh Kumar,
Vincent Guittot, Geert Uytterhoeven, Souvik Chakravarty,
Sudeep Holla, linux-pm, linux-arm-msm, linux-kernel, Lina Iyer
In-Reply-To: <20190513192300.653-10-ulf.hansson@linaro.org>
On Mon, May 13, 2019 at 09:22:51PM +0200, Ulf Hansson wrote:
> When the hierarchical CPU topology layout is used in DT, we need to setup
> the corresponding PM domain data structures, as to allow a CPU and a group
> of CPUs to be power managed accordingly. Let's enable this by deploying
> support through the genpd interface.
>
> Additionally, when the OS initiated mode is supported by the PSCI FW, let's
> also parse the domain idle states DT bindings as to make genpd responsible
> for the state selection, when the states are compatible with
> "domain-idle-state". Otherwise, when only Platform Coordinated mode is
> supported, we rely solely on the state selection to be managed through the
> regular cpuidle framework.
>
> If the initialization of the PM domain data structures succeeds and the OS
> initiated mode is supported, we try to switch to it. In case it fails,
> let's fall back into a degraded mode, rather than bailing out and returning
> an error code.
>
> Due to that the OS initiated mode may become enabled, we need to adjust to
> maintain backwards compatibility for a kernel started through a kexec call.
> Do this by explicitly switch to Platform Coordinated mode during boot.
>
> Finally, the actual initialization of the PM domain data structures, is
> done via calling the new shared function, psci_dt_init_pm_domains().
> However, this is implemented by subsequent changes.
>
> Co-developed-by: Lina Iyer <lina.iyer@linaro.org>
> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>
> Changes:
> - Simplify code setting domain_state at power off.
> - Use the genpd ->free_state() callback to manage freeing of states.
> - Fixup a bogus while loop.
>
> ---
> drivers/firmware/psci/Makefile | 2 +-
> drivers/firmware/psci/psci.c | 7 +-
> drivers/firmware/psci/psci.h | 5 +
> drivers/firmware/psci/psci_pm_domain.c | 268 +++++++++++++++++++++++++
> 4 files changed, 280 insertions(+), 2 deletions(-)
> create mode 100644 drivers/firmware/psci/psci_pm_domain.c
>
[...]
> #endif /* __PSCI_H */
> diff --git a/drivers/firmware/psci/psci_pm_domain.c b/drivers/firmware/psci/psci_pm_domain.c
> new file mode 100644
> index 000000000000..3c6ca846caf4
> --- /dev/null
> +++ b/drivers/firmware/psci/psci_pm_domain.c
> @@ -0,0 +1,268 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * PM domains for CPUs via genpd - managed by PSCI.
> + *
> + * Copyright (C) 2019 Linaro Ltd.
> + * Author: Ulf Hansson <ulf.hansson@linaro.org>
> + *
> + */
> +
[...]
> +static int psci_pd_power_off(struct generic_pm_domain *pd)
> +{
> + struct genpd_power_state *state = &pd->states[pd->state_idx];
> + u32 *pd_state;
> +
> + /* If we have failed to enable OSI mode, then abort power off. */
> + if (psci_has_osi_support() && !osi_mode_enabled)
> + return -EBUSY;
> +
> + if (!state->data)
> + return 0;
> +
> + /* When OSI mode is enabled, set the corresponding domain state. */
> + pd_state = state->data;
> + psci_set_domain_state(*pd_state);
I trying to understand how would this scale to level 2(cluster of
clusters or for simply system). The current code for psci_set_domain_state
just stores the value @pd_state into per-cpu domain_state. E.g.: Now if
the system level pd is getting called after cluster PD, it will set the
domain state to system level PD state. It won't work with original
format and it may work with extended format if it's carefully crafted.
In short, the point is just over-writing domain_state is asking for
troubles IMO.
--
Regards,
Sudeep
^ permalink raw reply
* Re: [PATCH 07/18] drivers: firmware: psci: Prepare to use OS initiated suspend mode
From: Sudeep Holla @ 2019-07-16 14:53 UTC (permalink / raw)
To: Ulf Hansson
Cc: Lorenzo Pieralisi, Mark Rutland, linux-arm-kernel,
Rafael J . Wysocki, Daniel Lezcano, Raju P . L . S . S . S . N,
Amit Kucheria, Bjorn Andersson, Stephen Boyd, Niklas Cassel,
Tony Lindgren, Kevin Hilman, Lina Iyer, Viresh Kumar,
Vincent Guittot, Geert Uytterhoeven, Souvik Chakravarty, linux-pm,
linux-arm-msm, linux-kernel, Lina Iyer
In-Reply-To: <20190513192300.653-8-ulf.hansson@linaro.org>
On Mon, May 13, 2019 at 09:22:49PM +0200, Ulf Hansson wrote:
> The per CPU variable psci_power_state, contains an array of fixed values,
> which reflects the corresponding arm,psci-suspend-param parsed from DT, for
> each of the available CPU idle states.
>
> This isn't sufficient when using the hierarchical CPU topology in DT in
> combination with having PSCI OS initiated (OSI) mode enabled. More
> precisely, in OSI mode, Linux is responsible of telling the PSCI FW what
> idle state the cluster (a group of CPUs) should enter, while in PSCI
> Platform Coordinated (PC) mode, each CPU independently votes for an idle
> state of the cluster.
>
> For this reason, let's introduce an additional per CPU variable called
> domain_state and implement two helper functions to read/write its values.
> Following patches, which implements PM domain support for PSCI, will use
> the domain_state variable and set it to corresponding bits that represents
> the selected idle state for the cluster.
>
> Finally, in psci_cpu_suspend_enter() and psci_suspend_finisher(), let's
> take into account the values in the domain_state, as to get the complete
> suspend parameter.
>
> Co-developed-by: Lina Iyer <lina.iyer@linaro.org>
> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>
> Changes:
> - Clarify changelog.
> - Drop changes in psci_cpu_on() as it belongs in the patch for hotplug.
> - Move some code inside "#ifdef CONFIG_CPU_IDLE".
>
> ---
> drivers/firmware/psci/psci.c | 24 ++++++++++++++++++++----
> 1 file changed, 20 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c
> index b11560f7c4b9..4aec513136e4 100644
> --- a/drivers/firmware/psci/psci.c
> +++ b/drivers/firmware/psci/psci.c
> @@ -285,6 +285,17 @@ static int __init psci_features(u32 psci_func_id)
>
> #ifdef CONFIG_CPU_IDLE
> static DEFINE_PER_CPU_READ_MOSTLY(u32 *, psci_power_state);
> +static DEFINE_PER_CPU(u32, domain_state);
> +
> +static inline u32 psci_get_domain_state(void)
> +{
> + return __this_cpu_read(domain_state);
> +}
> +
> +static inline void psci_set_domain_state(u32 state)
> +{
> + __this_cpu_write(domain_state, state);
> +}
>
> static int psci_dt_parse_state_node(struct device_node *np, u32 *state)
> {
> @@ -420,15 +431,17 @@ int psci_cpu_init_idle(struct cpuidle_driver *drv, unsigned int cpu)
> static int psci_suspend_finisher(unsigned long index)
> {
> u32 *state = __this_cpu_read(psci_power_state);
> + u32 composite_state = state[index - 1] | psci_get_domain_state();
>
The more I read this code and PSCI spec, I think it's not simple OR here
unless the specification states that. ACPI LPI explicitly stated that as
it was generic and PSCI doesn't. It can be made workable for original
format, but I think it's not that simple for extended format unless the
suspend parameters are carefully designed to achieve that, so we can't
just convert existing platforms the way it's shown on HiKey in this series.
--
Regards,
Sudeep
^ permalink raw reply
* Re: [PATCH 10/18] drivers: firmware: psci: Add hierarchical domain idle states converter
From: Lorenzo Pieralisi @ 2019-07-16 14:51 UTC (permalink / raw)
To: Ulf Hansson
Cc: Sudeep Holla, Mark Rutland, Linux ARM, Rafael J . Wysocki,
Daniel Lezcano, Raju P . L . S . S . S . N, Amit Kucheria,
Bjorn Andersson, Stephen Boyd, Niklas Cassel, Tony Lindgren,
Kevin Hilman, Lina Iyer, Viresh Kumar, Vincent Guittot,
Geert Uytterhoeven, Souvik Chakravarty, Linux PM, linux-arm-msm,
Linux Kernel Mailing List
In-Reply-To: <CAPDyKFpd5Jip+ZDENXVC115JJgCReJa4G92aKa45Lna6BeV5-g@mail.gmail.com>
On Tue, Jul 16, 2019 at 10:45:49AM +0200, Ulf Hansson wrote:
[...]
> > > +static void psci_pd_convert_states(struct cpuidle_state *idle_state,
> > > + u32 *psci_state, struct genpd_power_state *state)
> > > +{
> > > + u32 *state_data = state->data;
> > > + u64 target_residency_us = state->residency_ns;
> > > + u64 exit_latency_us = state->power_on_latency_ns +
> > > + state->power_off_latency_ns;
> > > +
> > > + *psci_state = *state_data;
> > > + do_div(target_residency_us, 1000);
> > > + idle_state->target_residency = target_residency_us;
> > > + do_div(exit_latency_us, 1000);
> > > + idle_state->exit_latency = exit_latency_us;
> > > + idle_state->enter = &psci_pd_enter_pc;
> > > + idle_state->enter_s2idle = &psci_pd_enter_s2idle_pc;
> > > + idle_state->flags |= CPUIDLE_FLAG_TIMER_STOP;
> >
> > This is arbitrary and not necessarily true.
>
> The arbitrary thing you refer to here, is that the
> CPUIDLE_FLAG_TIMER_STOP? Or are you referring to the complete function
> psci_pd_convert_states()?
I refer to CPUIDLE_FLAG_TIMER_STOP. I think that on platform coordinated
system we should not bother about the hierarchical representation of the
states (I understand I asked you to make it work but it has become too
complex, I would rather focus on making the hierarchical representation
work for all idle states combination in OSI mode).
Plus side, another level of complexity removed.
> > I think that this patch is useful to represent my reservations about the
> > current approach. As a matter of fact, idle state entry will always be a
> > CPUidle decision.
> >
> > You only need PM domain information to understand when all CPUs
> > in a power domain are actually idle but that's all genPD can do
> > in this respect.
> >
> > I think this patchset would be much simpler if both CPUidle and
> > genPD governor would work on *one* set of idle states, globally
> > indexed (and that would be true for PSCI suspend parameters too).
> >
> > To work with a unified set of idle states between CPUidle and genPD
> > (tossing some ideas around):
> >
> > - We can implement a genPD CPUidle governor that in its select method
> > takes into account genPD information (for instance by avoiding
> > selection of idle states that require multiple cpus to be in idle
> > to be effectively active)
> > - We can use genPD to enable/disable CPUidle states through runtime
> > PM information
>
> I don't understand how to make this work.
>
> The CPUidle governor works on per CPU basis. The genpd governor works
> on per PM domain basis, which typically can be a group of CPUs (and
> even other devices) via subdomains, for example.
>
> 1.
> In case of Linux being in *charge* of what idle state to pick for a
> group of CPUs, that decision is best done by the genpd governor as it
> operates on "groups" of CPUs. This is used for PSCI OSI mode. Of
> course, there are idle states also per CPU, which potentially could be
> managed by the genpd governor as well, but at this point I decided to
> re-use the cpuidle governor as it's already doing the job.
>
> 2. In case the decision of what idle state to enter for the group is
> done by the FW, we can rely solely on the cpuidle governor and let it
> select states per CPU basis. This is used for PSCI PC mode.
>
> >
> > There may be other ways. My point is that current code, with two (or
> > more if the hierarchy grows) sets of idle states across two subsystems
> > (CPUidle and genPD) is not very well defined and honestly very hard to
> > grasp and prone to errors.
>
> The complexity is there, I admit that.
>
> I guess some deeper insight about genpd+its governor+runtime PM are
> needed when reviewing this, of course. As an option, you may also have
> a look at my slides [1] from OSPM (Pisa) in May this year, which via
> flow charts try to describes things in more detail.
>
> In our offlist meeting, we discussed that perhaps moving some of the
> new PSCI code introduced in this series, into a cpuidle driver
> instead, could make things more clear. For sure, I can explore that
> option, but before I go there, I think we should agree on it publicly.
I will do it but given that the generic idle infrastructure basically
is there for PSCI and:
drivers/soc/qcom/spm.c
if we create a PSCI CPUidle driver we can write one for qcom-spm and
remove the generic idle infrastructure, there would not be much
point in keeping it in the kernel; at least on ARM64 not using
PSCI for CPUidle is not even an option.
> In principle what it means is to invent a special cpuidle driver for
> PSCI, so we would need access to some of the PSCI internal functions,
> for example.
Yes.
> One thing though, the initialization of the PSCI PM domain topology is
> a separate step, managed via the new initcall, psci_dt_topology_init()
> (introduced in patch 11). That part still seems to be belong to the
> PSCI code, don't you think?
Yes but at least we can call it from a sensible place (well, sensible,
most likely from an initcall given how idle drivers are initialized).
> > > + strncpy(idle_state->name, to_of_node(state->fwnode)->name,
> > > + CPUIDLE_NAME_LEN - 1);
> > > + strncpy(idle_state->desc, to_of_node(state->fwnode)->name,
> > > + CPUIDLE_NAME_LEN - 1);
> > > +}
> > > +
> > > +static bool psci_pd_is_provider(struct device_node *np)
> > > +{
> > > + struct psci_pd_provider *pd_prov, *it;
> > > +
> > > + list_for_each_entry_safe(pd_prov, it, &psci_pd_providers, link) {
> > > + if (pd_prov->node == np)
> > > + return true;
> > > + }
> > > +
> > > + return false;
> > > +}
> > > +
> > > static int psci_pd_init(struct device_node *np)
> > > {
> > > struct generic_pm_domain *pd;
> > > @@ -265,4 +316,71 @@ int psci_dt_init_pm_domains(struct device_node *np)
> > > pr_err("failed to create CPU PM domains ret=%d\n", ret);
> > > return ret;
> > > }
> > > +
> > > +int psci_dt_pm_domains_parse_states(struct cpuidle_driver *drv,
> > > + struct device_node *cpu_node, u32 *psci_states)
> > > +{
> > > + struct genpd_power_state *pd_states;
> > > + struct of_phandle_args args;
> > > + int ret, pd_state_count, i, state_idx, psci_idx;
> > > + u32 cpu_psci_state = psci_states[drv->state_count - 2];
> >
> > This (-2) is very dodgy and I doubt it would work on hierarchies going
> > above "cluster" level.
> >
> > As I say above, I think we should work towards a single array of
> > idle states to be selected by a CPUidle governor using genPD
> > runtime information to bias the results according to the number
> > of CPUs in a genPD that entered/exit idle.
> >
> > To be more precise, all idles states should be "domain-idle-state"
> > compatible, even the CPU ones, the distinction between what CPUidle
> > and genPD manage is a bit stretched IMO in this patchset.
> >
> > We will have a chance to talk about this but I thought I would
> > comment publically if anyone else is willing to chime in, this
> > is not a PSCI problem at all, it is a CPUidle/genPD coexistence
> > design problem which is much broader.
>
> To move this forward, I think we need to move from vague ideas to
> clear and distinct plans. Whatever that means. :-)
See above.
> I understand you are concerned about the level of changes introduced
> to the PSCI code. As I stated somewhere in earlier replies, I picked
> that approach as I thought it was better to implement things in a PSCI
> specific manner to start with, then we could move things around, when
> we realize that it make sense.
I am also concerned about how the idle states are managed in
this patchset and I am pretty certain it will break when we
move away from a simple hierarchy with one CPU state and one
cluster state, we will comment on the specifics.
Moving PSCI code into a CPUidle driver will cater for the rest.
> Anyway, as a suggestion to address your concern, how about this:
>
> 1. Move some things out to a PSCI cpuidle driver. We need to decide
> more exactly on what to move and find the right level for the
> interfaces.
I will do it and post patches asap.
> 2. Don't attach the CPU to the PM domain topology in case the PSCI PC
> mode is used. I think this makes it easier, at least as a first step,
> to understand when runtime PM needs to be used/enabled.
In the PSCI CPUidle driver we can have two distinct struct
cpuidle_state->enter functions for PC and OSI, no overhead
for PC, runtime PM for OSI, decoupling done.
We can choose one or the other depending on whether:
OSI iff:
- OSI is available
- hierarchical idle states are present in DT
otherwise PC.
That's what this patch does but we will do it in a unified file.
> 3. Would it help if I volunteer to help you guys as a maintainer for
> PSCI. At least for the part of the new code that becomes introduced?
We will do as described above if that makes sense.
Thanks,
Lorenzo
^ permalink raw reply
* Re: [PATCH 18/18] arm64: dts: hikey: Convert to the hierarchical CPU topology layout
From: Sudeep Holla @ 2019-07-16 14:47 UTC (permalink / raw)
To: Ulf Hansson
Cc: Lorenzo Pieralisi, Mark Rutland, linux-arm-kernel,
Rafael J . Wysocki, Daniel Lezcano, Raju P . L . S . S . S . N,
Amit Kucheria, Bjorn Andersson, Stephen Boyd, Niklas Cassel,
Tony Lindgren, Kevin Hilman, Lina Iyer, Viresh Kumar,
Vincent Guittot, Geert Uytterhoeven, Souvik Chakravarty,
Sudeep Holla, linux-pm, linux-arm-msm, linux-kernel, Wei Xu
In-Reply-To: <20190513192300.653-19-ulf.hansson@linaro.org>
On Mon, May 13, 2019 at 09:23:00PM +0200, Ulf Hansson wrote:
> To enable the OS to manage last-man standing activities for a CPU, while an
> idle state for a group of CPUs is selected, let's convert the Hikey
> platform into using the hierarchical CPU topology layout.
>
> Cc: Wei Xu <xuwei5@hisilicon.com>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>
> Changes:
> - None.
>
> ---
> arch/arm64/boot/dts/hisilicon/hi6220.dtsi | 87 ++++++++++++++++++++---
> 1 file changed, 76 insertions(+), 11 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
> index 108e2a4227f6..36ff460f428f 100644
> --- a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
> +++ b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
> cpus {
[...]
> @@ -70,9 +128,8 @@
> };
>
> CLUSTER_SLEEP: cluster-sleep {
> - compatible = "arm,idle-state";
> - local-timer-stop;
> - arm,psci-suspend-param = <0x1010000>;
> + compatible = "domain-idle-state";
> + arm,psci-suspend-param = <0x1000000>;
> entry-latency-us = <1000>;
> exit-latency-us = <700>;
> min-residency-us = <2700>;
Again this must be original format and as per PSCI spec, your patch
changes this cluster sleep state into cluster retention state which I
think is not what you intended.
--
Regards,
Sudeep
^ permalink raw reply
* Re: [PATCH 17/18] arm64: dts: Convert to the hierarchical CPU topology layout for MSM8916
From: Sudeep Holla @ 2019-07-16 14:47 UTC (permalink / raw)
To: Ulf Hansson
Cc: Lorenzo Pieralisi, Mark Rutland, linux-arm-kernel,
Rafael J . Wysocki, Daniel Lezcano, Raju P . L . S . S . S . N,
Amit Kucheria, Bjorn Andersson, Stephen Boyd, Niklas Cassel,
Tony Lindgren, Kevin Hilman, Lina Iyer, Viresh Kumar,
Vincent Guittot, Geert Uytterhoeven, Souvik Chakravarty, linux-pm,
linux-arm-msm, linux-kernel, Lina Iyer, Andy Gross, Sudeep Holla,
David Brown
In-Reply-To: <20190513192300.653-18-ulf.hansson@linaro.org>
On Mon, May 13, 2019 at 09:22:59PM +0200, Ulf Hansson wrote:
> From: Lina Iyer <lina.iyer@linaro.org>
>
> In the hierarchical layout, we are creating power domains around each CPU
> and describes the idle states for them inside the power domain provider
> node. Note that, the CPU's idle states still needs to be compatible with
> "arm,idle-state".
>
> Furthermore, represent the CPU cluster as a separate master power domain,
> powering the CPU's power domains. The cluster node, contains the idle
> states for the cluster and each idle state needs to be compatible with the
> "domain-idle-state".
>
> If the running platform is using a PSCI FW that supports the OS initiated
> CPU suspend mode, which likely should be the case unless the PSCI FW is
> very old, this change triggers the PSCI driver to enable it.
>
> Cc: Andy Gross <andy.gross@linaro.org>
> Cc: David Brown <david.brown@linaro.org>
> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
> Co-developed-by: Ulf Hansson <ulf.hansson@linaro.org>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
[...]
> @@ -166,12 +170,57 @@
> min-residency-us = <2000>;
> local-timer-stop;
> };
> +
> + CLUSTER_RET: cluster-retention {
> + compatible = "domain-idle-state";
> + arm,psci-suspend-param = <0x1000010>;
> + entry-latency-us = <500>;
> + exit-latency-us = <500>;
> + min-residency-us = <2000>;
> + };
> +
> + CLUSTER_PWRDN: cluster-gdhs {
> + compatible = "domain-idle-state";
> + arm,psci-suspend-param = <0x1000030>;
> + entry-latency-us = <2000>;
> + exit-latency-us = <2000>;
> + min-residency-us = <6000>;
> + };
> };
> };
I was trying to understand the composition of composite state parameters
in this series and that made me look at these DT examples.
What format does the above platform use ? I tried matching them to
both original as well as extended format and I fail to understand.
Assuming original format:
State power_state PowerLevel StateType StateID
SPC 0x40000002 0(core) 0(Retention) 0x2 (Res0 b[29]=1?)
CLUSTER_RET 0x1000010 1(clusters) 0(Retention) 0x10
CLUSTER_PWRDN 0x1000030 1(clusters) 0(Retention?) 0x30
Now extended format:
State power_state StateType StateID
SPC 0x40000002 0(Retention) 0x40000002 (Res0 b[29]=1?)
CLUSTER_RET 0x1000010 0(Retention) 0x1000010
CLUSTER_PWRDN 0x1000030 0(Retention?) 0x1000030
What am I missing ?
--
Regards,
Sudeep
^ permalink raw reply
* Re: [PATCH v11 4/5] sched/core: uclamp: Use TG's clamps to restrict TASK's clamps
From: Patrick Bellasi @ 2019-07-16 14:34 UTC (permalink / raw)
To: Michal Koutný
Cc: linux-kernel, linux-pm, Ingo Molnar, Peter Zijlstra, Tejun Heo,
Rafael J . Wysocki, Vincent Guittot, Viresh Kumar, Paul Turner,
Quentin Perret, Dietmar Eggemann, Morten Rasmussen, Juri Lelli,
Todd Kjos, Joel Fernandes, Steve Muckle, Suren Baghdasaryan,
Alessio Balsini
In-Reply-To: <20190715164248.GA21982@blackbody.suse.cz>
On 15-Jul 18:42, Michal Koutný wrote:
> On Mon, Jul 08, 2019 at 09:43:56AM +0100, Patrick Bellasi <patrick.bellasi@arm.com> wrote:
> > This mimics what already happens for a task's CPU affinity mask when the
> > task is also in a cpuset, i.e. cgroup attributes are always used to
> > restrict per-task attributes.
> If I am not mistaken when set_schedaffinity(2) call is made that results
> in an empty cpuset, the call fails with EINVAL [1].
>
> If I track the code correctly, the values passed to sched_setattr(2) are
> checked against the trivial validity (umin <= umax) and later on, they
> are adjusted to match the effective clamping of the containing
> task_group. Is that correct?
>
> If the user attempted to sched_setattr [a, b], and the effective uclamp
> was [c, d] such that [a, b] ∩ [c, d] = ∅, the set uclamp will be
> silently moved out of their intended range. Wouldn't it be better to
> return with EINVAL too when the intersection is empty (since the user
> supplied range won't be attained)?
You right for the cpuset case, but I don't think we never end up with
a "empty" set in the case of utilization clamping.
We limit clamps hierarchically in such a way that:
clamp[clamp_id] = min(task::clamp[clamp_id],
tg::clamp[clamp_id],
system::clamp[clamp_id])
and we ensure, on top of the above that:
clamp[UCLAMP_MIN] = min(clamp[UCLAMP_MIN], clamp[UCLAMP_MAX])
Since it's all and only about "capping" values, at the very extreme
case you can end up with:
clamp[UCLAMP_MIN] = clamp[UCLAMP_MAX] = 0
but that's till a valid configuration.
Am I missing something?
Otherwise, I think the changelog sentence you quoted is just
misleading. I'll remove it from v12 since it does not really clarify
anything more then the rest of the message.
Cheers,
Patrick
--
#include <best/regards.h>
Patrick Bellasi
^ permalink raw reply
* Re: [PATCH v11 3/5] sched/core: uclamp: Propagate system defaults to root group
From: Patrick Bellasi @ 2019-07-16 14:34 UTC (permalink / raw)
To: Michal Koutný
Cc: linux-kernel, linux-pm, Ingo Molnar, Peter Zijlstra, Tejun Heo,
Rafael J . Wysocki, Vincent Guittot, Viresh Kumar, Paul Turner,
Quentin Perret, Dietmar Eggemann, Morten Rasmussen, Juri Lelli,
Todd Kjos, Joel Fernandes, Steve Muckle, Suren Baghdasaryan,
Alessio Balsini
In-Reply-To: <20190715164226.GA30262@blackbody.suse.cz>
On 15-Jul 18:42, Michal Koutný wrote:
> On Mon, Jul 08, 2019 at 09:43:55AM +0100, Patrick Bellasi <patrick.bellasi@arm.com> wrote:
> > +static void uclamp_update_root_tg(void)
> > +{
> > + struct task_group *tg = &root_task_group;
> > +
> > + uclamp_se_set(&tg->uclamp_req[UCLAMP_MIN],
> > + sysctl_sched_uclamp_util_min, false);
> > + uclamp_se_set(&tg->uclamp_req[UCLAMP_MAX],
> > + sysctl_sched_uclamp_util_max, false);
> > +
> > + cpu_util_update_eff(&root_task_group.css);
> > +}
> cpu_util_update_eff internally calls css_for_each_descendant_pre() so
> this should be protected with rcu_read_lock().
Right, good catch! Will add in v12.
Cheers,
Patrick
--
#include <best/regards.h>
Patrick Bellasi
^ permalink raw reply
* Re: [PATCH v11 2/5] sched/core: uclamp: Propagate parent clamps
From: Patrick Bellasi @ 2019-07-16 14:07 UTC (permalink / raw)
To: Michal Koutný
Cc: linux-kernel, linux-pm, Ingo Molnar, Peter Zijlstra, Tejun Heo,
Rafael J . Wysocki, Vincent Guittot, Viresh Kumar, Paul Turner,
Quentin Perret, Dietmar Eggemann, Morten Rasmussen, Juri Lelli,
Todd Kjos, Joel Fernandes, Steve Muckle, Suren Baghdasaryan,
Alessio Balsini
In-Reply-To: <20190715164200.GA30862@blackbody.suse.cz>
Hi Michal,
On 15-Jul 18:42, Michal Koutný wrote:
> On Mon, Jul 08, 2019 at 09:43:54AM +0100, Patrick Bellasi <patrick.bellasi@arm.com> wrote:
> > Since it's possible for a cpu.uclamp.min value to be bigger than the
> > cpu.uclamp.max value, ensure local consistency by restricting each
> > "protection"
> > (i.e. min utilization) with the corresponding "limit" (i.e. max
> > utilization).
> I think this constraint should be mentioned in the Documentation/....
That note comes from the previous review cycle and it's based on a
request from Tejun to align uclamp behaviors with the way the
delegation model is supposed to work.
I guess this part of the documentation:
https://www.kernel.org/doc/html/latest/admin-guide/cgroup-v2.html?highlight=protections#resource-distribution-models
should already cover the expected uclamp min/max behaviors.
However, I guess "repetita iuvant" in this case. I'll call this out
explicitly in the description of cpu.uclamp.min.
> > +static void cpu_util_update_eff(struct cgroup_subsys_state *css)
> > +{
> > + struct cgroup_subsys_state *top_css = css;
> > + struct uclamp_se *uc_se = NULL;
> > + unsigned int eff[UCLAMP_CNT];
> > + unsigned int clamp_id;
> > + unsigned int clamps;
> > +
> > + css_for_each_descendant_pre(css, top_css) {
> > + uc_se = css_tg(css)->parent
> > + ? css_tg(css)->parent->uclamp : NULL;
> > +
> > + for_each_clamp_id(clamp_id) {
> > + /* Assume effective clamps matches requested clamps */
> > + eff[clamp_id] = css_tg(css)->uclamp_req[clamp_id].value;
> > + /* Cap effective clamps with parent's effective clamps */
> > + if (uc_se &&
> > + eff[clamp_id] > uc_se[clamp_id].value) {
> > + eff[clamp_id] = uc_se[clamp_id].value;
> > + }
> > + }
> > + /* Ensure protection is always capped by limit */
> > + eff[UCLAMP_MIN] = min(eff[UCLAMP_MIN], eff[UCLAMP_MAX]);
> > +
> > + /* Propagate most restrictive effective clamps */
> > + clamps = 0x0;
> > + uc_se = css_tg(css)->uclamp;
> (Nitpick only, reassigning child where was parent before decreases
> readibility. IMO)
Did not checked but I think the compiler will figure out it can still
use a single pointer for both assignments.
I'll let's the compiler to its job and add in a dedicated stack var
for the parent pointer.
> > + for_each_clamp_id(clamp_id) {
> > + if (eff[clamp_id] == uc_se[clamp_id].value)
> > + continue;
> > + uc_se[clamp_id].value = eff[clamp_id];
> > + uc_se[clamp_id].bucket_id = uclamp_bucket_id(eff[clamp_id]);
> Shouldn't these writes be synchronized with writes from
> __setscheduler_uclamp()?
You right, the synchronization is introduced by a later patch:
sched/core: uclamp: Update CPU's refcount on TG's clamp changes
Cheers,
Patrick
--
#include <best/regards.h>
Patrick Bellasi
^ permalink raw reply
* Re: [PATCH v11 0/5] Add utilization clamping support (CGroups API)
From: Patrick Bellasi @ 2019-07-16 14:03 UTC (permalink / raw)
To: Michal Koutný
Cc: linux-kernel, linux-pm, Ingo Molnar, Peter Zijlstra, Tejun Heo,
Rafael J . Wysocki, Vincent Guittot, Viresh Kumar, Paul Turner,
Quentin Perret, Dietmar Eggemann, Morten Rasmussen, Juri Lelli,
Todd Kjos, Joel Fernandes, Steve Muckle, Suren Baghdasaryan,
Alessio Balsini
In-Reply-To: <20190715165116.GB21982@blackbody.suse.cz>
On 15-Jul 18:51, Michal Koutný wrote:
> Hello Patrick.
Hi Michal,
> I took a look at your series and I've posted some notes to your patches.
thanks for your review!
> One applies more to the series overall -- I see there is enum uclamp_id
> defined but at many places (local variables, function args) int or
> unsigned int is used. Besides the inconsistency, I think it'd be nice to
> use the enum at these places.
Right, I think in some of the original versions I had few code paths
where it was not possible to use enum values. That seems no more the case.
Since this change is likely affecting also core bits already merged in
5.3, in v12 I'm going to add a bulk rename patch at the end of the
series, so that we can keep a better tracking of this change.
> (Also, I may suggest CCing ML cgroups@vger.kernel.org where more eyes
> may be available to the cgroup part of your series.)
Good point, I'll add that for the upcoming v12 posting.
Cheers,
Patrick
--
#include <best/regards.h>
Patrick Bellasi
^ permalink raw reply
* Re: [PATCH v4 14/24] PM / devfreq: tegra30: Ensure that target freq won't overflow
From: Dmitry Osipenko @ 2019-07-16 13:59 UTC (permalink / raw)
To: Chanwoo Choi, Thierry Reding, MyungJoo Ham, Kyungmin Park,
Jonathan Hunter, Tomeu Vizoso
Cc: linux-pm, linux-tegra, linux-kernel
In-Reply-To: <e6b53ba0-ac98-bda6-b087-553088a43d9f@samsung.com>
16.07.2019 15:30, Chanwoo Choi пишет:
> On 19. 7. 8. 오전 7:32, Dmitry Osipenko wrote:
>> Potentially very high boosting could cause an integer overflow for a
>> highly clocked memory after conversion to MHz.
>>
>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>> ---
>> drivers/devfreq/tegra30-devfreq.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
>> index 2f59c78930bd..0de1efdaabf4 100644
>> --- a/drivers/devfreq/tegra30-devfreq.c
>> +++ b/drivers/devfreq/tegra30-devfreq.c
>> @@ -460,6 +460,7 @@ static unsigned long actmon_update_target(struct tegra_devfreq *tegra,
>> unsigned long target_freq;
>>
>> target_freq = dev->avg_count / ACTMON_SAMPLING_PERIOD + dev->boost_freq;
>> + target_freq = min(target_freq, ULONG_MAX / KHZ);
>
> Did you meet this corner case?
I can't recall that, technically it could happen and I don't feel
comfortable by having potential integer overflows anyway.
> If have to change it, you better to use 'tegra->max_freq' as following:
> min(target_freq, tegra->max_freq);
The 'tegra->max_freq' will work here, but 'ULONG_MAX / KHZ' is a
constant value which is also correct and doesn't case any harm.
Probably will be even more expressive to write this as:
target_freq = dev->avg_freq + dev->boost_freq;
target_freq = tegra_actmon_account_cpu_freq(tegra, dev, target_freq);
return min(target_freq, tegra->max_freq);
Thank you very much for the reviews!
^ permalink raw reply
* [PATCH RFC v2] PM / devfreq: Add cpu based scaling support to passive_governor
From: Sibi Sankar @ 2019-07-16 13:36 UTC (permalink / raw)
To: saravanak, myungjoo.ham, cw00.choi, kyungmin.park
Cc: viresh.kumar, nm, sboyd, georgi.djakov, agross, david.brown,
robh+dt, mark.rutland, rjw, linux-arm-msm, devicetree,
linux-kernel, linux-pm, hsinyi, adharmap, Saravana Kannan,
Sibi Sankar
From: Saravana Kannan <skannan@codeaurora.org>
Many CPU architectures have caches that can scale independent of the
CPUs. Frequency scaling of the caches is necessary to make sure that the
cache is not a performance bottleneck that leads to poor performance and
power. The same idea applies for RAM/DDR.
To achieve this, this patch adds support for cpu based scaling to the
passive governor. This is accomplished by taking the current frequency
of each CPU frequency domain and then adjust the frequency of the cache
(or any devfreq device) based on the frequency of the CPUs. It listens
to CPU frequency transition notifiers to keep itself up to date on the
current CPU frequency.
To decide the frequency of the device, the governor does one of the
following:
* Derives the optimal devfreq device opp from required-opps property of
the parent cpu opp_table.
* Scales the device frequency in proportion to the CPU frequency. So, if
the CPUs are running at their max frequency, the device runs at its
max frequency. If the CPUs are running at their min frequency, the
device runs at its min frequency. It is interpolated for frequencies
in between.
Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
[Sibi: Integrated cpu-freqmap governor into passive_governor]
Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
---
v2:
* Address Chanwoo's review comments
This patch is a re-work of:
https://patchwork.kernel.org/patch/10553171/
This patch depends on Saravana's add required-opps support series:
https://patchwork.kernel.org/cover/11016423/
drivers/devfreq/Kconfig | 2 +
drivers/devfreq/governor_passive.c | 275 +++++++++++++++++++++++++++--
include/linux/devfreq.h | 40 ++++-
3 files changed, 296 insertions(+), 21 deletions(-)
diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
index f3b242987fd91..5db8633f94d83 100644
--- a/drivers/devfreq/Kconfig
+++ b/drivers/devfreq/Kconfig
@@ -73,6 +73,8 @@ config DEVFREQ_GOV_PASSIVE
device. This governor does not change the frequency by itself
through sysfs entries. The passive governor recommends that
devfreq device uses the OPP table to get the frequency/voltage.
+ Alternatively the governor can also be chosen to scale based on
+ the online CPUs current frequency.
comment "DEVFREQ Drivers"
diff --git a/drivers/devfreq/governor_passive.c b/drivers/devfreq/governor_passive.c
index 24ce94c80f06e..743b6093cc732 100644
--- a/drivers/devfreq/governor_passive.c
+++ b/drivers/devfreq/governor_passive.c
@@ -8,11 +8,86 @@
*/
#include <linux/module.h>
+#include <linux/cpu.h>
+#include <linux/cpufreq.h>
+#include <linux/cpumask.h>
#include <linux/device.h>
#include <linux/devfreq.h>
+#include <linux/slab.h>
#include "governor.h"
-static int devfreq_passive_get_target_freq(struct devfreq *devfreq,
+static unsigned int xlate_cpufreq_to_devfreq(struct devfreq_passive_data *data,
+ unsigned int cpu)
+{
+ unsigned int cpu_min, cpu_max, dev_min, dev_max, cpu_percent, max_state;
+ struct devfreq_cpu_state *cpu_state = data->cpu_state[cpu];
+ struct devfreq *devfreq = (struct devfreq *)data->this;
+ unsigned long *freq_table = devfreq->profile->freq_table;
+ struct dev_pm_opp *opp = NULL, *cpu_opp = NULL;
+ unsigned long cpu_freq, freq;
+
+ if (!cpu_state || cpu_state->first_cpu != cpu ||
+ !cpu_state->opp_table || !devfreq->opp_table)
+ return 0;
+
+ cpu_freq = cpu_state->freq * 1000;
+ cpu_opp = devfreq_recommended_opp(cpu_state->dev, &cpu_freq, 0);
+ if (IS_ERR(cpu_opp))
+ return 0;
+
+ opp = dev_pm_opp_xlate_opp(cpu_state->opp_table,
+ devfreq->opp_table, cpu_opp);
+ dev_pm_opp_put(cpu_opp);
+
+ if (!IS_ERR(opp)) {
+ freq = dev_pm_opp_get_freq(opp);
+ dev_pm_opp_put(opp);
+ } else {
+ /* Use Interpolation if required opps is not available */
+ cpu_min = cpu_state->min_freq;
+ cpu_max = cpu_state->max_freq;
+ cpu_freq = cpu_state->freq;
+
+ if (freq_table) {
+ /* Get minimum frequency according to sorting order */
+ max_state = freq_table[devfreq->profile->max_state - 1];
+ if (freq_table[0] < max_state) {
+ dev_min = freq_table[0];
+ dev_max = max_state;
+ } else {
+ dev_min = max_state;
+ dev_max = freq_table[0];
+ }
+ } else {
+ if (devfreq->max_freq <= devfreq->min_freq)
+ return 0;
+ dev_min = devfreq->min_freq;
+ dev_max = devfreq->max_freq;
+ }
+ cpu_percent = ((cpu_freq - cpu_min) * 100) / cpu_max - cpu_min;
+ freq = dev_min + mult_frac(dev_max - dev_min, cpu_percent, 100);
+ }
+
+ return freq;
+}
+
+static int get_target_freq_with_cpufreq(struct devfreq *devfreq,
+ unsigned long *freq)
+{
+ struct devfreq_passive_data *p_data =
+ (struct devfreq_passive_data *)devfreq->data;
+ unsigned int cpu, target_freq = 0;
+
+ for_each_online_cpu(cpu)
+ target_freq = max(target_freq,
+ xlate_cpufreq_to_devfreq(p_data, cpu));
+
+ *freq = target_freq;
+
+ return 0;
+}
+
+static int get_target_freq_with_devfreq(struct devfreq *devfreq,
unsigned long *freq)
{
struct devfreq_passive_data *p_data
@@ -22,16 +97,6 @@ static int devfreq_passive_get_target_freq(struct devfreq *devfreq,
struct dev_pm_opp *opp = NULL, *p_opp = NULL;
int i, count, ret = 0;
- /*
- * If the devfreq device with passive governor has the specific method
- * to determine the next frequency, should use the get_target_freq()
- * of struct devfreq_passive_data.
- */
- if (p_data->get_target_freq) {
- ret = p_data->get_target_freq(devfreq, freq);
- goto out;
- }
-
/*
* If the parent and passive devfreq device uses the OPP table,
* get the next frequency by using the OPP table.
@@ -102,6 +167,37 @@ static int devfreq_passive_get_target_freq(struct devfreq *devfreq,
return ret;
}
+static int devfreq_passive_get_target_freq(struct devfreq *devfreq,
+ unsigned long *freq)
+{
+ struct devfreq_passive_data *p_data =
+ (struct devfreq_passive_data *)devfreq->data;
+ int ret;
+
+ /*
+ * If the devfreq device with passive governor has the specific method
+ * to determine the next frequency, should use the get_target_freq()
+ * of struct devfreq_passive_data.
+ */
+ if (p_data->get_target_freq)
+ return p_data->get_target_freq(devfreq, freq);
+
+ switch (p_data->parent_type) {
+ case DEVFREQ_PARENT_DEV:
+ ret = get_target_freq_with_devfreq(devfreq, freq);
+ break;
+ case CPUFREQ_PARENT_DEV:
+ ret = get_target_freq_with_cpufreq(devfreq, freq);
+ break;
+ default:
+ ret = -EINVAL;
+ dev_err(&devfreq->dev, "Invalid parent type\n");
+ break;
+ }
+
+ return ret;
+}
+
static int update_devfreq_passive(struct devfreq *devfreq, unsigned long freq)
{
int ret;
@@ -156,6 +252,140 @@ static int devfreq_passive_notifier_call(struct notifier_block *nb,
return NOTIFY_DONE;
}
+static int cpufreq_passive_notifier_call(struct notifier_block *nb,
+ unsigned long event, void *ptr)
+{
+ struct devfreq_passive_data *data =
+ container_of(nb, struct devfreq_passive_data, nb);
+ struct devfreq *devfreq = (struct devfreq *)data->this;
+ struct devfreq_cpu_state *cpu_state;
+ struct cpufreq_freqs *freq = ptr;
+ unsigned int current_freq;
+ int ret;
+
+ if (event != CPUFREQ_POSTCHANGE || !freq ||
+ !data->cpu_state[freq->policy->cpu])
+ return 0;
+
+ cpu_state = data->cpu_state[freq->policy->cpu];
+ if (cpu_state->freq == freq->new)
+ return 0;
+
+ /* Backup current freq and pre-update cpu state freq*/
+ current_freq = cpu_state->freq;
+ cpu_state->freq = freq->new;
+
+ mutex_lock(&devfreq->lock);
+ ret = update_devfreq(devfreq);
+ mutex_unlock(&devfreq->lock);
+ if (ret) {
+ cpu_state->freq = current_freq;
+ dev_err(&devfreq->dev, "Couldn't update the frequency.\n");
+ return ret;
+ }
+
+ return 0;
+}
+
+static int cpufreq_passive_register(struct devfreq_passive_data **p_data)
+{
+ struct devfreq_passive_data *data = *p_data;
+ struct devfreq *devfreq = (struct devfreq *)data->this;
+ struct device *dev = devfreq->dev.parent;
+ struct opp_table *opp_table = NULL;
+ struct devfreq_cpu_state *state;
+ struct cpufreq_policy *policy;
+ struct device *cpu_dev;
+ unsigned int cpu;
+ int ret;
+
+ get_online_cpus();
+ data->nb.notifier_call = cpufreq_passive_notifier_call;
+ ret = cpufreq_register_notifier(&data->nb,
+ CPUFREQ_TRANSITION_NOTIFIER);
+ if (ret) {
+ dev_err(dev, "Couldn't register cpufreq notifier.\n");
+ data->nb.notifier_call = NULL;
+ goto out;
+ }
+
+ /* Populate devfreq_cpu_state */
+ for_each_online_cpu(cpu) {
+ if (data->cpu_state[cpu])
+ continue;
+
+ policy = cpufreq_cpu_get(cpu);
+ if (policy) {
+ state = kzalloc(sizeof(*state), GFP_KERNEL);
+ if (!state) {
+ ret = -ENOMEM;
+ goto out;
+ }
+
+ cpu_dev = get_cpu_device(cpu);
+ if (!cpu_dev) {
+ dev_err(dev, "Couldn't get cpu device.\n");
+ ret = -ENODEV;
+ goto out;
+ }
+
+ opp_table = dev_pm_opp_get_opp_table(cpu_dev);
+ if (IS_ERR(devfreq->opp_table)) {
+ ret = PTR_ERR(opp_table);
+ goto out;
+ }
+
+ state->dev = cpu_dev;
+ state->opp_table = opp_table;
+ state->first_cpu = cpumask_first(policy->related_cpus);
+ state->freq = policy->cur;
+ state->min_freq = policy->cpuinfo.min_freq;
+ state->max_freq = policy->cpuinfo.max_freq;
+ data->cpu_state[cpu] = state;
+ cpufreq_cpu_put(policy);
+ } else {
+ ret = -EPROBE_DEFER;
+ goto out;
+ }
+ }
+out:
+ put_online_cpus();
+ if (ret)
+ return ret;
+
+ /* Update devfreq */
+ mutex_lock(&devfreq->lock);
+ ret = update_devfreq(devfreq);
+ mutex_unlock(&devfreq->lock);
+ if (ret)
+ dev_err(dev, "Couldn't update the frequency.\n");
+
+ return ret;
+}
+
+static int cpufreq_passive_unregister(struct devfreq_passive_data **p_data)
+{
+ struct devfreq_passive_data *data = *p_data;
+ struct devfreq_cpu_state *cpu_state;
+ int cpu;
+
+ if (data->nb.notifier_call)
+ cpufreq_unregister_notifier(&data->nb,
+ CPUFREQ_TRANSITION_NOTIFIER);
+
+ for_each_possible_cpu(cpu) {
+ cpu_state = data->cpu_state[cpu];
+ if (cpu_state) {
+ if (cpu_state->opp_table)
+ dev_pm_opp_put_opp_table(cpu_state->opp_table);
+ kfree(cpu_state);
+ cpu_state = NULL;
+ }
+ }
+
+ return 0;
+}
+
static int devfreq_passive_event_handler(struct devfreq *devfreq,
unsigned int event, void *data)
{
@@ -166,7 +396,7 @@ static int devfreq_passive_event_handler(struct devfreq *devfreq,
struct notifier_block *nb = &p_data->nb;
int ret = 0;
- if (!parent)
+ if (p_data->parent_type == DEVFREQ_PARENT_DEV && !parent)
return -EPROBE_DEFER;
switch (event) {
@@ -174,13 +404,24 @@ static int devfreq_passive_event_handler(struct devfreq *devfreq,
if (!p_data->this)
p_data->this = devfreq;
- nb->notifier_call = devfreq_passive_notifier_call;
- ret = devm_devfreq_register_notifier(dev, parent, nb,
- DEVFREQ_TRANSITION_NOTIFIER);
+ if (p_data->parent_type == DEVFREQ_PARENT_DEV) {
+ nb->notifier_call = devfreq_passive_notifier_call;
+ ret = devm_devfreq_register_notifier(dev, parent, nb,
+ DEVFREQ_TRANSITION_NOTIFIER);
+ } else if (p_data->parent_type == CPUFREQ_PARENT_DEV) {
+ ret = cpufreq_passive_register(&p_data);
+ } else {
+ ret = -EINVAL;
+ }
break;
case DEVFREQ_GOV_STOP:
- devm_devfreq_unregister_notifier(dev, parent, nb,
- DEVFREQ_TRANSITION_NOTIFIER);
+ if (p_data->parent_type == DEVFREQ_PARENT_DEV)
+ devm_devfreq_unregister_notifier(dev, parent, nb,
+ DEVFREQ_TRANSITION_NOTIFIER);
+ else if (p_data->parent_type == CPUFREQ_PARENT_DEV)
+ cpufreq_passive_unregister(&p_data);
+ else
+ ret = -EINVAL;
break;
default:
break;
diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
index 69d487329408e..f3f898dfd09b2 100644
--- a/include/linux/devfreq.h
+++ b/include/linux/devfreq.h
@@ -263,6 +263,32 @@ struct devfreq_simple_ondemand_data {
#endif
#if IS_ENABLED(CONFIG_DEVFREQ_GOV_PASSIVE)
+/**
+ * struct devfreq_cpu_state - holds the per-cpu state
+ * @freq: the current frequency of the cpu.
+ * @min_freq: the min frequency of the cpu.
+ * @max_freq: the max frequency of the cpu.
+ * @first_cpu: the cpumask of the first cpu of a policy.
+ * @dev: reference to cpu device.
+ * @opp_table: reference to cpu opp table.
+ *
+ * This structure stores the required cpu_state of a cpu.
+ * This is auto-populated by the governor.
+ */
+struct devfreq_cpu_state {
+ unsigned int freq;
+ unsigned int min_freq;
+ unsigned int max_freq;
+ unsigned int first_cpu;
+ struct device *dev;
+ struct opp_table *opp_table;
+};
+
+enum devfreq_parent_dev_type {
+ DEVFREQ_PARENT_DEV,
+ CPUFREQ_PARENT_DEV,
+};
+
/**
* struct devfreq_passive_data - void *data fed to struct devfreq
* and devfreq_add_device
@@ -274,13 +300,15 @@ struct devfreq_simple_ondemand_data {
* using governors except for passive governor.
* If the devfreq device has the specific method to decide
* the next frequency, should use this callback.
- * @this: the devfreq instance of own device.
- * @nb: the notifier block for DEVFREQ_TRANSITION_NOTIFIER list
+ * @parent_type parent type of the device
+ * @this: the devfreq instance of own device.
+ * @nb: the notifier block for DEVFREQ_TRANSITION_NOTIFIER list
+ * @cpu_state: the state min/max/current frequency of all online cpu's
*
* The devfreq_passive_data have to set the devfreq instance of parent
* device with governors except for the passive governor. But, don't need to
- * initialize the 'this' and 'nb' field because the devfreq core will handle
- * them.
+ * initialize the 'this', 'nb' and 'cpu_state' field because the devfreq core
+ * will handle them.
*/
struct devfreq_passive_data {
/* Should set the devfreq instance of parent device */
@@ -289,9 +317,13 @@ struct devfreq_passive_data {
/* Optional callback to decide the next frequency of passvice device */
int (*get_target_freq)(struct devfreq *this, unsigned long *freq);
+ /* Should set the type of parent device */
+ enum devfreq_parent_dev_type parent_type;
+
/* For passive governor's internal use. Don't need to set them */
struct devfreq *this;
struct notifier_block nb;
+ struct devfreq_cpu_state *cpu_state[NR_CPUS];
};
#endif
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply related
* Re: [PATCH v4 12/24] PM / devfreq: tegra30: Inline all one-line functions
From: Dmitry Osipenko @ 2019-07-16 13:35 UTC (permalink / raw)
To: Chanwoo Choi, Thierry Reding, MyungJoo Ham, Kyungmin Park,
Jonathan Hunter, Tomeu Vizoso
Cc: linux-pm, linux-tegra, linux-kernel
In-Reply-To: <b5634fbe-8bc1-0f04-e13b-6345dfbb5615@samsung.com>
16.07.2019 15:26, Chanwoo Choi пишет:
> Hi Dmitry,
>
> I'm not sure that it is necessary.
> As I knew, usally, the 'inline' is used on header file
> to define the empty functions.
>
> Do we have to change it with 'inline' keyword?
The 'inline' attribute tells compiler that instead of jumping into the
function, it should take the function's code and replace the function's
invocation with that code. This is done in order to help compiler
optimize code properly, please see [1]. There is absolutely no need to
create a function call into a function that consists of a single
instruction.
[1] https://gcc.gnu.org/onlinedocs/gcc-9.1.0/gcc/Inline.html
^ permalink raw reply
* Re: [PATCH v4 11/24] PM / devfreq: tegra30: Add debug messages
From: Dmitry Osipenko @ 2019-07-16 13:26 UTC (permalink / raw)
To: Chanwoo Choi, Thierry Reding, MyungJoo Ham, Kyungmin Park,
Jonathan Hunter, Tomeu Vizoso
Cc: linux-pm, linux-tegra, linux-kernel
In-Reply-To: <c883bdbe-427f-35a1-9e63-5e4953a84286@samsung.com>
16.07.2019 15:23, Chanwoo Choi пишет:
> Hi Dmitry,
>
> Usually, the kernel log print for all users
> such as changing the frequency, fail or success.
>
> But, if the log just show the register dump,
> it is not useful for all users. It is just used
> for only specific developer.
>
> I recommend that you better to add more exception handling
> code on many points instead of just showing the register dump.
The debug messages are not users, but for developers. Yes, I primarily
made the debugging to be useful for myself and will be happy to change
the way debugging is done if there will be any other active developer
for this driver. The registers dump is more than enough in order to
understand what's going on, I don't see any real need to change anything
here for now.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox