* Re: [PATCH v12 1/2] cpufreq: Add Kryo CPU scaling driver
From: Sudeep Holla @ 2018-05-24 14:47 UTC (permalink / raw)
To: ilialin
Cc: Sudeep Holla, linux-pm, devicetree, linux-kernel, vireshk, nm,
sboyd, robh, mark.rutland, rjw
In-Reply-To: <000701d3f368$fd31e410$f795ac30$@codeaurora.org>
On 24/05/18 15:10, ilialin@codeaurora.org wrote:
> Thank you for the explanation. However, could you suggest, which
> condition should I check then? Device tree?
>
Yes some compatible which is applicable for all the SoCs or platforms on
which this driver can work ?
--
Regards,
Sudeep
^ permalink raw reply
* Re: [PATCH 8/9] PM / Domains: Add support for multi PM domains per device to genpd
From: Jon Hunter @ 2018-05-24 14:34 UTC (permalink / raw)
To: Ulf Hansson
Cc: Rajendra Nayak, Geert Uytterhoeven, Linux PM, Greg Kroah-Hartman,
Kevin Hilman, Rafael J . Wysocki, Linux Kernel Mailing List,
Todor Tomov, Viresh Kumar, linux-tegra, Vincent Guittot,
Linux ARM
In-Reply-To: <CAPDyKFravSx3h6UnsjNGM2ViZtkxvPMULyVQnoVT9jsEP5az+A@mail.gmail.com>
On 24/05/18 13:17, Ulf Hansson wrote:
> On 24 May 2018 at 11:36, Jon Hunter <jonathanh@nvidia.com> wrote:
>>
>> On 24/05/18 08:04, Ulf Hansson wrote:
>>
>> ...
>>
>>>> Any reason why we could not add a 'boolean' argument to the API to
>>>> indicate
>>>> whether the new device should be linked? I think that I prefer the API
>>>> handles it, but I can see there could be instances where drivers may wish
>>>> to
>>>> handle it themselves.
>>>
>>>
>>> Coming back to this question. Both Tegra XUSB and Qcom Camera use
>>> case, would benefit from doing the linking themselves, as it needs
>>> different PM domains to be powered on depending on the current use
>>> case - as to avoid wasting power.
>>>
>>> However, I can understand that you prefer some simplicity over
>>> optimizations, as you told us. Then, does it mean that you are
>>> insisting on extending the APIs with a boolean for linking, or are you
>>> fine with the driver to call device_link_add()?
>>
>>
>> I am fine with the driver calling device_link_add(), but I just wonder if we
>> will find a several drivers doing this and then we will end up doing this
>> later anyway.
>
> Okay.
>
>>
>> The current API is called ...
>>
>> * genpd_dev_pm_attach_by_id() - Attach a device to one of its PM domain.
>> * @dev: Device to attach.
>> * @index: The index of the PM domain.
>>
>> This naming and description is a bit misleading, because really it is not
>> attaching the device that is passed, but creating a new device to attach a
>> PM domain to. So we should consider renaming and changing the description
>> and indicate that users need to link the device.
>
> I picked the name to be consistent with the existing
> genpd_dev_pm_attach(). Do you have a better suggestion?
Well, it appears to get more of a 'get' function and so I don't see why
we could not have 'genpd_dev_get_by_id()' and then we could have a
genpd_dev_put() as well (which would call genpd_dev_pm_detach).
> I agree, some details is missing to the description, let me try to
> improve it. Actually, I was trying to follow existing descriptions
> from genpd_dev_pm_attach(), so perhaps that also needs a little
> update.
>
> However, do note that, neither genpd_dev_pm_attach() or
> genpd_dev_pm_attach_by_id() is supposed to be called by drivers, but
> rather only by the driver core. So description may not be so
> important.
>
> In regards to good descriptions, for sure the API added in patch9,
> dev_pm_domain_attach_by_id(), needs a good one, as this is what
> drivers should be using.
OK. Same appears to apply here to the description as I mentioned above.
Still seems to be more of a 'get' than an attach. So I wonder if it
should be dev_pm_domain_get_by_id() instead?
>> Finally, how is a PM domain attached via calling genpd_dev_pm_attach_by_id()
>> detached?
>
> Via the existing genpd_dev_pm_detach(), according to what I have
> described in the change log. I clarify the description in regards to
> this as well.
OK, so this bit is a to-do as that is not yet exposed AFAICT. I see that
you said 'although we need to extend it to cover cleanup of the earlier
registered device, via calling device_unregister().' So if we do this
then that would be fine.
Cheers!
Jon
--
nvpublic
^ permalink raw reply
* [PATCH RFC] suspend: Prevent might sleep splats
From: Sebastian Andrzej Siewior @ 2018-05-24 14:24 UTC (permalink / raw)
To: linux-kernel, linux-pm
Cc: Thomas Gleixner, Rafael J. Wysocki, Pavel Machek, Len Brown
From: Thomas Gleixner <tglx@linutronix.de>
timekeeping suspend/resume calls read_persistent_clock() which takes
rtc_lock. That results in might sleep warnings because at that point
we run with interrupts disabled.
We cannot convert rtc_lock to a raw spinlock as that would trigger
other might sleep warnings.
As a workaround we disable the might sleep warnings by setting
system_state to SYSTEM_SUSPEND before calling sysdev_suspend() and
restoring it to SYSTEM_RUNNING afer sysdev_resume().
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
include/linux/kernel.h | 1 +
kernel/power/hibernate.c | 7 +++++++
kernel/power/suspend.c | 4 ++++
3 files changed, 12 insertions(+)
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -532,6 +532,7 @@ extern enum system_states {
SYSTEM_HALT,
SYSTEM_POWER_OFF,
SYSTEM_RESTART,
+ SYSTEM_SUSPEND,
} system_state;
#define TAINT_PROPRIETARY_MODULE 0
--- a/kernel/power/hibernate.c
+++ b/kernel/power/hibernate.c
@@ -287,6 +287,8 @@ static int create_image(int platform_mod
local_irq_disable();
+ system_state = SYSTEM_SUSPEND;
+
error = syscore_suspend();
if (error) {
pr_err("Some system devices failed to power down, aborting hibernation\n");
@@ -317,6 +319,7 @@ static int create_image(int platform_mod
syscore_resume();
Enable_irqs:
+ system_state = SYSTEM_RUNNING;
local_irq_enable();
Enable_cpus:
@@ -445,6 +448,7 @@ static int resume_target_kernel(bool pla
goto Enable_cpus;
local_irq_disable();
+ system_state = SYSTEM_SUSPEND;
error = syscore_suspend();
if (error)
@@ -478,6 +482,7 @@ static int resume_target_kernel(bool pla
syscore_resume();
Enable_irqs:
+ system_state = SYSTEM_RUNNING;
local_irq_enable();
Enable_cpus:
@@ -563,6 +568,7 @@ int hibernation_platform_enter(void)
goto Enable_cpus;
local_irq_disable();
+ system_state = SYSTEM_SUSPEND;
syscore_suspend();
if (pm_wakeup_pending()) {
error = -EAGAIN;
@@ -575,6 +581,7 @@ int hibernation_platform_enter(void)
Power_up:
syscore_resume();
+ system_state = SYSTEM_RUNNING;
local_irq_enable();
Enable_cpus:
--- a/kernel/power/suspend.c
+++ b/kernel/power/suspend.c
@@ -428,6 +428,8 @@ static int suspend_enter(suspend_state_t
arch_suspend_disable_irqs();
BUG_ON(!irqs_disabled());
+ system_state = SYSTEM_SUSPEND;
+
error = syscore_suspend();
if (!error) {
*wakeup = pm_wakeup_pending();
@@ -443,6 +445,8 @@ static int suspend_enter(suspend_state_t
syscore_resume();
}
+ system_state = SYSTEM_RUNNING;
+
arch_suspend_enable_irqs();
BUG_ON(irqs_disabled());
^ permalink raw reply
* RE: [PATCH v12 1/2] cpufreq: Add Kryo CPU scaling driver
From: ilialin @ 2018-05-24 14:10 UTC (permalink / raw)
To: 'Sudeep Holla'
Cc: linux-pm, devicetree, linux-kernel, vireshk, nm, sboyd, robh,
mark.rutland, rjw
In-Reply-To: <b4e221af-57f7-fd11-942d-6bc6f6b55a73@arm.com>
Thank you for the explanation. However, could you suggest, which condition should I check then? Device tree?
> -----Original Message-----
> From: Sudeep Holla <sudeep.holla@arm.com>
> Sent: Thursday, May 24, 2018 17:01
> To: ilialin@codeaurora.org; vireshk@kernel.org; nm@ti.com;
> sboyd@kernel.org; robh@kernel.org; mark.rutland@arm.com;
> rjw@rjwysocki.net
> Cc: Sudeep Holla <sudeep.holla@arm.com>; linux-pm@vger.kernel.org;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v12 1/2] cpufreq: Add Kryo CPU scaling driver
>
>
>
> On 24/05/18 14:03, ilialin@codeaurora.org wrote:
> >
> >
>
> [...]
>
> >>> +
> >>> + ret = PTR_ERR_OR_ZERO(platform_device_register_simple(
> >>> + "qcom-cpufreq-kryo", -1, NULL, 0));
> >>
> >>
> >> You simply can't do this unconditionally here. This will blow up on
> >> platforms where this driver is not supposed to work. The probe will
> >> be called on non- QCOM or non-Kryo QCOM platforms and I reckon it
> >> will crash trying to execute something in qcom_smem_get.
> >
> > What do you mean by 'unconditionally'?
>
> Why should you even add/register a device "qcom-cpufreq-kryo" on other
> platforms. Drivers can get registered, but only devices that are present or
> required by the platform need to be registered.
>
> > The driver depends on the smem and nvmem drivers, which depend on
> ARCH_QCOM:
> > + depends on QCOM_QFPROM
> > + depends on QCOM_SMEM
> >
>
> Sure, but we have something called single image for all ARM64 platforms.
> May be QCOM still used to tweeking config to build binary for your particular
> mobile platform but the distro kernel need single binary to work on all
> platforms. We have moved far away from platform specific builds long back
> now IIRC.
>
> > And if SMEM read in the probe returns something other than Kryo, it will
> exit.
> >
>
> Check what this driver does ?
>
> msm_id = qcom_smem_get(QCOM_SMEM_HOST_ANY,
> MSM_ID_SMEM, &len);
> msm_id++;
> switch ((enum _msm_id)*msm_id)
>
> I think it *will and should* crash here ? You need to check the return value
> for sure. But since qcom_smem_get return -EPROBE_DEFER, we keep
> retrying even on non QCOM platforms which is something I would like to
> avoid.
>
> Therefore that's not the main concern. Why do I have to see "qcom-cpufreq-
> kryo" device registered on my non QCOM platform ?
>
> --
> Regards,
> Sudeep
^ permalink raw reply
* [PATCH v3 2/2] sched/fair: util_est: update before schedutil
From: Patrick Bellasi @ 2018-05-24 14:10 UTC (permalink / raw)
To: linux-kernel, linux-pm
Cc: Ingo Molnar, Peter Zijlstra, Rafael J . Wysocki, Viresh Kumar,
Vincent Guittot, Dietmar Eggemann, Morten Rasmussen, Juri Lelli,
Joel Fernandes, Steve Muckle
In-Reply-To: <20180524141023.13765-1-patrick.bellasi@arm.com>
When a task is enqueue the estimated utilization of a CPU is updated
to better support the selection of the required frequency.
However, schedutil is (implicitly) updated by update_load_avg() which
always happens before util_est_{en,de}queue(), thus potentially
introducing a latency between estimated utilization updates and
frequency selections.
Let's update util_est at the beginning of enqueue_task_fair(),
which will ensure that all schedutil updates will see the most
updated estimated utilization value for a CPU.
Reported-by: Vincent Guittot <vincent.guittot@linaro.org>
Signed-off-by: Patrick Bellasi <patrick.bellasi@arm.com>
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
Acked-by: Vincent Guittot <vincent.guittot@linaro.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Vincent Guittot <vincent.guittot@linaro.org>
Cc: linux-kernel@vger.kernel.org
Cc: linux-pm@vger.kernel.org
Fixes: 7f65ea42eb00 ("sched/fair: Add util_est on top of PELT")
---
Changes in v3:
- add "Acked-by" Vincent tags
Changes in v2:
- improve comment in enqueue_task_fair() (Peter)
- add "Fixes" tag
- add "Acked-by" Viresh tag
---
kernel/sched/fair.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 748cb054fefd..e497c05aab7f 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -5385,6 +5385,14 @@ enqueue_task_fair(struct rq *rq, struct task_struct *p, int flags)
struct cfs_rq *cfs_rq;
struct sched_entity *se = &p->se;
+ /*
+ * The code below (indirectly) updates schedutil which looks at
+ * the cfs_rq utilization to select a frequency.
+ * Let's add the task's estimated utilization to the cfs_rq's
+ * estimated utilization, before we update schedutil.
+ */
+ util_est_enqueue(&rq->cfs, p);
+
/*
* If in_iowait is set, the code below may not trigger any cpufreq
* utilization updates, so do it here explicitly with the IOWAIT flag
@@ -5426,7 +5434,6 @@ enqueue_task_fair(struct rq *rq, struct task_struct *p, int flags)
if (!se)
add_nr_running(rq, 1);
- util_est_enqueue(&rq->cfs, p);
hrtick_update(rq);
}
--
2.15.1
^ permalink raw reply related
* [PATCH v3 1/2] sched/cpufreq: always consider blocked FAIR utilization
From: Patrick Bellasi @ 2018-05-24 14:10 UTC (permalink / raw)
To: linux-kernel, linux-pm
Cc: Ingo Molnar, Peter Zijlstra, Rafael J . Wysocki, Viresh Kumar,
Vincent Guittot, Dietmar Eggemann, Morten Rasmussen, Juri Lelli,
Joel Fernandes, Steve Muckle
In-Reply-To: <20180524141023.13765-1-patrick.bellasi@arm.com>
Since the refactoring introduced by:
commit 8f111bc357aa ("cpufreq/schedutil: Rewrite CPUFREQ_RT support")
we aggregate FAIR utilization only if this class has runnable tasks.
This was mainly due to avoid the risk to stay on an high frequency just
because of the blocked utilization of a CPU not being properly decayed
while the CPU was idle.
However, since:
commit 31e77c93e432 ("sched/fair: Update blocked load when newly idle")
the FAIR blocked utilization is properly decayed also for IDLE CPUs.
This allows us to use the FAIR blocked utilization as a safe mechanism
to gracefully reduce the frequency only if no FAIR tasks show up on a
CPU for a reasonable period of time.
Moreover, we also reduce the frequency drops of CPUs running periodic
tasks which, depending on the task periodicity and the time required
for a frequency switch, was increasing the chances to introduce some
undesirable performance variations.
Reported-by: Vincent Guittot <vincent.guittot@linaro.org>
Signed-off-by: Patrick Bellasi <patrick.bellasi@arm.com>
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
Acked-by: Vincent Guittot <vincent.guittot@linaro.org>
Tested-by: Vincent Guittot <vincent.guittot@linaro.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Cc: Vincent Guittot <vincent.guittot@linaro.org>
Cc: Viresh Kumar <viresh.kumar@linaro.org>
Cc: Joel Fernandes <joelaf@google.com>
Cc: linux-kernel@vger.kernel.org
Cc: linux-pm@vger.kernel.org
---
Changes in v3:
- add "Tested-by" and "Acked-by" Vincent tags
Changes in v2:
- add "Acked-by" Viresh tag
---
kernel/sched/cpufreq_schedutil.c | 17 ++++++++---------
1 file changed, 8 insertions(+), 9 deletions(-)
diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
index e13df951aca7..28592b62b1d5 100644
--- a/kernel/sched/cpufreq_schedutil.c
+++ b/kernel/sched/cpufreq_schedutil.c
@@ -183,22 +183,21 @@ static void sugov_get_util(struct sugov_cpu *sg_cpu)
static unsigned long sugov_aggregate_util(struct sugov_cpu *sg_cpu)
{
struct rq *rq = cpu_rq(sg_cpu->cpu);
- unsigned long util;
- if (rq->rt.rt_nr_running) {
- util = sg_cpu->max;
- } else {
- util = sg_cpu->util_dl;
- if (rq->cfs.h_nr_running)
- util += sg_cpu->util_cfs;
- }
+ if (rq->rt.rt_nr_running)
+ return sg_cpu->max;
/*
+ * Utilization required by DEADLINE must always be granted while, for
+ * FAIR, we use blocked utilization of IDLE CPUs as a mechanism to
+ * gracefully reduce the frequency when no tasks show up for longer
+ * periods of time.
+ *
* Ideally we would like to set util_dl as min/guaranteed freq and
* util_cfs + util_dl as requested freq. However, cpufreq is not yet
* ready for such an interface. So, we only do the latter for now.
*/
- return min(util, sg_cpu->max);
+ return min(sg_cpu->max, (sg_cpu->util_dl + sg_cpu->util_cfs));
}
static void sugov_set_iowait_boost(struct sugov_cpu *sg_cpu, u64 time, unsigned int flags)
--
2.15.1
^ permalink raw reply related
* [PATCH v3 0/2] Improve schedutil integration for FAIR tasks
From: Patrick Bellasi @ 2018-05-24 14:10 UTC (permalink / raw)
To: linux-kernel, linux-pm
Cc: Ingo Molnar, Peter Zijlstra, Rafael J . Wysocki, Viresh Kumar,
Vincent Guittot, Dietmar Eggemann, Morten Rasmussen, Juri Lelli,
Joel Fernandes, Steve Muckle
Here is the (hopefully) final update of:
https://lkml.org/lkml/2018/5/11/319
20180511131509.16275-1-patrick.bellasi@arm.com
including only the first two patches of the original series, which have been
already reviewed and acked by Viresh and Vincent.
The last patch has been removed from this series since, during the discussion,
we agree that it's not completely fixing the problem it was addressing and we
would like to explore a better and more complete solution. Thus, I'll follow up
on a separated and dedicated series.
Cheers Patrick
Changes in v3:
- add "Tested-by" and "Acked-by" Vincent tags
Changes in v2:
- improve comment in enqueue_task_fair() (Peter)
- add "Fixes" tag
- add "Acked-by" Viresh tag
Patrick Bellasi (2):
sched/cpufreq: always consider blocked FAIR utilization
sched/fair: util_est: update before schedutil
kernel/sched/cpufreq_schedutil.c | 17 ++++++++---------
kernel/sched/fair.c | 9 ++++++++-
2 files changed, 16 insertions(+), 10 deletions(-)
--
2.15.1
^ permalink raw reply
* Re: [PATCH v12 1/2] cpufreq: Add Kryo CPU scaling driver
From: Sudeep Holla @ 2018-05-24 14:01 UTC (permalink / raw)
To: ilialin, vireshk, nm, sboyd, robh, mark.rutland, rjw
Cc: Sudeep Holla, linux-pm, devicetree, linux-kernel
In-Reply-To: <000501d3f35f$96794910$c36bdb30$@codeaurora.org>
On 24/05/18 14:03, ilialin@codeaurora.org wrote:
>
>
[...]
>>> +
>>> + ret = PTR_ERR_OR_ZERO(platform_device_register_simple(
>>> + "qcom-cpufreq-kryo", -1, NULL, 0));
>>
>>
>> You simply can't do this unconditionally here. This will blow up on platforms
>> where this driver is not supposed to work. The probe will be called on non-
>> QCOM or non-Kryo QCOM platforms and I reckon it will crash trying to
>> execute something in qcom_smem_get.
>
> What do you mean by 'unconditionally'?
Why should you even add/register a device "qcom-cpufreq-kryo" on other
platforms. Drivers can get registered, but only devices that are present
or required by the platform need to be registered.
> The driver depends on the smem and nvmem drivers, which depend on ARCH_QCOM:
> + depends on QCOM_QFPROM
> + depends on QCOM_SMEM
>
Sure, but we have something called single image for all ARM64 platforms.
May be QCOM still used to tweeking config to build binary for your
particular mobile platform but the distro kernel need single binary to
work on all platforms. We have moved far away from platform specific
builds long back now IIRC.
> And if SMEM read in the probe returns something other than Kryo, it will exit.
>
Check what this driver does ?
msm_id = qcom_smem_get(QCOM_SMEM_HOST_ANY, MSM_ID_SMEM, &len);
msm_id++;
switch ((enum _msm_id)*msm_id)
I think it *will and should* crash here ? You need to check the return
value for sure. But since qcom_smem_get return -EPROBE_DEFER, we keep
retrying even on non QCOM platforms which is something I would like to
avoid.
Therefore that's not the main concern. Why do I have to see
"qcom-cpufreq-kryo" device registered on my non QCOM platform ?
--
Regards,
Sudeep
^ permalink raw reply
* Re: [PATCH 3/3] sched/fair: schedutil: explicit update only when required
From: Patrick Bellasi @ 2018-05-24 13:42 UTC (permalink / raw)
To: Joel Fernandes
Cc: linux-kernel, linux-pm, Ingo Molnar, Peter Zijlstra,
Rafael J . Wysocki, Viresh Kumar, Vincent Guittot,
Dietmar Eggemann, Morten Rasmussen, Juri Lelli, Joel Fernandes,
Todd Kjos, kernel-team, Steve Muckle
In-Reply-To: <20180517151701.GC162290@joelaf.mtv.corp.google.com>
Hi Joel,
sorry for the late reply, this thread is a bit confusing because we
keep discussing while there was already a v2 posted on list.
However, here are few comments below...
[...]
> > > > > @@ -5456,10 +5443,12 @@ static void dequeue_task_fair(struct rq *rq, struct task_struct *p, int flags)
> > > > > update_cfs_group(se);
> > > > > }
> > > > >
> > > > > + /* The task is no more visible from the root cfs_rq */
> > > > > if (!se)
> > > > > sub_nr_running(rq, 1);
> > > > >
> > > > > util_est_dequeue(&rq->cfs, p, task_sleep);
> > > > > + cpufreq_update_util(rq, 0);
> > > >
> > > > One question about this change. In enqueue, throttle and unthrottle - you are
> > > > conditionally calling cpufreq_update_util incase the task was
> > > > visible/not-visible in the hierarchy.
> > > >
> > > > But in dequeue you're unconditionally calling it. Seems a bit inconsistent.
> > > > Is this because of util_est or something? Could you add a comment here
> > > > explaining why this is so?
> > >
> > > The big question I have is incase se != NULL, then its still visible at the
> > > root RQ level.
> >
> > My understanding it that you get !se at dequeue time when we are
> > dequeuing a task from a throttled RQ. Isn't it?
>
> I don't think so? !se means the RQ is not throttled.
Yes, I agree, I "just" forgot a "not" in the sentence above... my bad!
However, we are on the same page here.
> > Thus, this means you are dequeuing a throttled task, I guess for
> > example because of a migration.
> > However, the point is that a task dequeue from a throttled RQ _is
> > already_ not visible from the root RQ, because of the sub_nr_running()
> > done by throttle_cfs_rq().
>
> Yes that's what I was wondering, so my point was if its already not visible,
> then why call schedutil. I felt call schedutil only if its visible like you
> were doing for the other paths.
Agree, as discussed in Vincent in v2, we should likely move these
schedutil updates at attach/detach time. This is when exectly we know
that the utilization has changed for a CPU.
... and that's what I'll propose in the upcoming v3 for this patch.
[...]
> I agree with your assessments below and about not calling cpufreq
> when CPU is about to idle.
Cool ;)
--
#include <best/regards.h>
Patrick Bellasi
^ permalink raw reply
* Re: OMAP serial runtime PM and autosuspend (was: Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding))
From: Tony Lindgren @ 2018-05-24 13:32 UTC (permalink / raw)
To: Johan Hovold
Cc: Sebastian Reichel, H. Nikolaus Schaller, Andreas Kemnade,
Mark Rutland, Arnd Bergmann, Pavel Machek,
linux-kernel@vger.kernel.org,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Greg Kroah-Hartman, Rob Herring, linux-serial, linux-omap,
linux-pm, Andy Shevchenko
In-Reply-To: <20180524091742.GZ30172@localhost>
* Johan Hovold <johan@kernel.org> [180524 09:20]:
> On Mon, May 21, 2018 at 08:48:32AM -0700, Tony Lindgren wrote:
> >
> > Yes the bug for closed ports needs to be fixed for sure.
>
> I did some forensic on this and it seems this problem has "always" been
> there. Specifically, closed ports have never been runtime suspended
> unless a non-negative autosuspend delay has been set by user space since
> fcdca75728ac ("ARM: OMAP2+: UART: Add runtime pm support for omap-serial
> driver") which was merged seven years ago.
>
> So while it would certainly be nice to save some more power by default,
> this would really be a new feature rather than a bug or regression fix
> (which reduces the urgency for this issue somewhat too).
Yes it's been there since the start.
> > > > > 2. aggressive serial RPM, where the controller is allowed to
> > > > > suspend while the port is open even though this may result in
> > > > > lost characters when waking up on incoming data
> > > >
> > > > In this case it seems that the only thing needed is to just
> > > > configure the autosuspend delay for the parent port. The use of
> > > > -1 has been around since the start of runtime PM AFAIK, so maybe
> > > > we should just document it. I guess we could also introduce
> > > > pm_runtime_block_autoidle_unless_configured() :)
> > >
> > > The implications of a negative autosuspend delay are already documented
> > > (in Documentation/power/runtime_pm.txt); it's just the omap drivers that
> > > gets it wrong when trying to do things which aren't currently supported
> > > (and never have been).
> > >
> > > So I still think we need a new mechanism for this.
> >
> > Well if you have some better mechanism in mind let's try it out. Short of
> > sprinkling pm_runtime_force_suspend/resume calls all over, I'm out of ideas
> > right now.
>
> Yeah, that would be too much of a hack and likely wouldn't work either
> (and we really should do away with those _force calls altogether).
>
> I've been thinking a bit too much about this already, but it may be
> possible to use the pm QoS framework for this. A resume latency can be
> set through sysfs where "n/a" is defined to mean "no latency accepted"
> (i.e. controller remains always-on while port is open) and "0" means
> "any latency accepted" (i.e. omap aggressive serial RPM is allowed).
Oh yeah, PM QoS might work here!
> Now, implementing this may get a little tricky as we want to be able to
> change this setting on the fly (consider consoles) and we need to figure
> out the interaction with serdev (user space should probably not be
> allowed to request a resume latency for ports used by serdev).
It sounds like Andy Shevchenko has a series of patches that just might
allow us to make this all generic for Linux serial framework. So adding
Andy to Cc, I don't think he has posted all the patches yet.
Andy, see the PM QoS comment above for console idling :)
> I'd be happy to dig into this some more, but not in my spare time I'm
> afraid.
Indeed.
> > > > > For normal ttys, we need a user-space interface for selecting between
> > > > > the two, and for serdev we may want a way to select the RPM scheme from
> > > > > within the kernel.
> > > > >
> > > > > Note that with my serdev controller runtime PM patch, serdev core could
> > > > > always opt for aggressive PM (as by default serdev core holds an RPM
> > > > > reference for the controller while the port is open).
> > > >
> > > > So if your serdev controller was to set the parent autosuspend
> > > > delay on open() and set it back on close() this should work?
> > >
> > > Is it really the job of a serdev driver to set the autosuspend delay of
> > > a parent controller? Isn't this somethings which depends on the
> > > characteristics of the controller (possibly configurable by user space)
> > > such as the cost of runtime suspending and resuming?
> >
> > Only in some cases will the serdev driver know it's safe to configure
> > the parent controller. Configuring the parent controller from userspace
> > works just fine as we've seen for years now.
>
> Yes, user space may override the default settings provided by the serial
> driver, but a serdev driver, in contrast, knows nothing about the
> underlying serial hardware.
>
> > > The patch I posted works with what we have today; if a parent serial
> > > controller driver uses aggressive runtime PM by default or after having
> > > been configured through sysfs to do so.
> >
> > Yeah let's stick with configuring the parent controller from userspace
> > for now at least.
>
> Yep, status quo works for the time being (since this isn't a
> regression).
>
> > > What I'm getting at here is that the delay should be set by the serial
> > > driver implementing aggressive runtime PM. Then all we need is a
> > > mechanism to determine whether an extra RPM reference should be taken at
> > > tty open or not (configurable by user space, defaulting to yes).
> >
> > OK yeah some additional on/off switch seems to be missing here.
>
> As mentioned above, PM QoS resume latency may possibly be used, and
> otherwise me may able to define a new (generic) QoS flag for this.
Good idea.
> > > Specifically, the serial drivers themselves would always use
> > > autosuspend and not have to deal with supporting the two RPM schemes
> > > (normal vs aggressive runtime PM).
> >
> > OK. So if I understand your idea right, we could have autosuspend timeout
> > set to 3000ms in the 8250_omap.c but still default to RPM blocked?
> > Then user can enable aggressive PM via /sys as desired, right?
>
> Not RPM blocked; the ports must always be able to suspend when the port
> is closed. But user space should be able to enable the aggressive
> (active) runtime PM via sysfs independently of the autosuspend delay,
> yes.
Yup OK, I like the PM QoS approach.
Regards,
Tony
^ permalink raw reply
* RE: [PATCH v12 1/2] cpufreq: Add Kryo CPU scaling driver
From: ilialin @ 2018-05-24 13:03 UTC (permalink / raw)
To: 'Sudeep Holla', vireshk, nm, sboyd, robh, mark.rutland,
rjw
Cc: linux-pm, devicetree, linux-kernel
In-Reply-To: <860be68b-cac0-9efc-b3c7-cc75b391a4c3@arm.com>
> -----Original Message-----
> From: Sudeep Holla <sudeep.holla@arm.com>
> Sent: Thursday, May 24, 2018 15:52
> To: Ilia Lin <ilialin@codeaurora.org>; vireshk@kernel.org; nm@ti.com;
> sboyd@kernel.org; robh@kernel.org; mark.rutland@arm.com;
> rjw@rjwysocki.net
> Cc: Sudeep Holla <sudeep.holla@arm.com>; linux-pm@vger.kernel.org;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v12 1/2] cpufreq: Add Kryo CPU scaling driver
>
> Hi Ilia,
>
>
> On 24/05/18 09:57, Ilia Lin wrote:
> > In Certain QCOM SoCs like apq8096 and msm8996 that have KRYO
> > processors, the CPU frequency subset and voltage value of each OPP
> > varies based on the silicon variant in use. Qualcomm Process Voltage
> > Scaling Tables defines the voltage and frequency value based on the
> > msm-id in SMEM and speedbin blown in the efuse combination.
> > The qcom-cpufreq-kryo driver reads the msm-id and efuse value from the
> > SoC to provide the OPP framework with required information.
> > This is used to determine the voltage and frequency value for each OPP
> > of
> > operating-points-v2 table when it is parsed by the OPP framework.
> >
> > Signed-off-by: Ilia Lin <ilialin@codeaurora.org>
> > ---
> > drivers/cpufreq/Kconfig.arm | 10 ++
> > drivers/cpufreq/Makefile | 1 +
> > drivers/cpufreq/cpufreq-dt-platdev.c | 3 +
> > drivers/cpufreq/qcom-cpufreq-kryo.c | 194
> > +++++++++++++++++++++++++++++++++++
> > 4 files changed, 208 insertions(+)
> > create mode 100644 drivers/cpufreq/qcom-cpufreq-kryo.c
> >
> > diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
> > index de55c7d..0bfd40e 100644
> > --- a/drivers/cpufreq/Kconfig.arm
> > +++ b/drivers/cpufreq/Kconfig.arm
> > @@ -124,6 +124,16 @@ config ARM_OMAP2PLUS_CPUFREQ
> > depends on ARCH_OMAP2PLUS
> > default ARCH_OMAP2PLUS
> >
> > +config ARM_QCOM_CPUFREQ_KRYO
> > + bool "Qualcomm Kryo based CPUFreq"
> > + depends on QCOM_QFPROM
> > + depends on QCOM_SMEM
> > + select PM_OPP
> > + help
> > + This adds the CPUFreq driver for Qualcomm Kryo SoC based boards.
> > +
> > + If in doubt, say N.
> > +
> > config ARM_S3C_CPUFREQ
> > bool
> > help
> > diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile index
> > 8d24ade..fb4a2ec 100644
> > --- a/drivers/cpufreq/Makefile
> > +++ b/drivers/cpufreq/Makefile
> > @@ -65,6 +65,7 @@ obj-$(CONFIG_MACH_MVEBU_V7) +=
> mvebu-cpufreq.o
> > obj-$(CONFIG_ARM_OMAP2PLUS_CPUFREQ) += omap-cpufreq.o
> > obj-$(CONFIG_ARM_PXA2xx_CPUFREQ) += pxa2xx-cpufreq.o
> > obj-$(CONFIG_PXA3xx) += pxa3xx-cpufreq.o
> > +obj-$(CONFIG_ARM_QCOM_CPUFREQ_KRYO) += qcom-cpufreq-
> kryo.o
> > obj-$(CONFIG_ARM_S3C2410_CPUFREQ) += s3c2410-cpufreq.o
> > obj-$(CONFIG_ARM_S3C2412_CPUFREQ) += s3c2412-cpufreq.o
> > obj-$(CONFIG_ARM_S3C2416_CPUFREQ) += s3c2416-cpufreq.o
> > diff --git a/drivers/cpufreq/cpufreq-dt-platdev.c
> > b/drivers/cpufreq/cpufreq-dt-platdev.c
> > index 3b585e4..77d6ab8 100644
> > --- a/drivers/cpufreq/cpufreq-dt-platdev.c
> > +++ b/drivers/cpufreq/cpufreq-dt-platdev.c
> > @@ -118,6 +118,9 @@
> >
> > { .compatible = "nvidia,tegra124", },
> >
> > + { .compatible = "qcom,apq8096", },
> > + { .compatible = "qcom,msm8996", },
> > +
> > { .compatible = "st,stih407", },
> > { .compatible = "st,stih410", },
> >
> > diff --git a/drivers/cpufreq/qcom-cpufreq-kryo.c
> > b/drivers/cpufreq/qcom-cpufreq-kryo.c
> > new file mode 100644
> > index 0000000..9fe379c
> > --- /dev/null
> > +++ b/drivers/cpufreq/qcom-cpufreq-kryo.c
> > @@ -0,0 +1,194 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
> > + */
> > +
> > +/*
> > + * In Certain QCOM SoCs like apq8096 and msm8996 that have KRYO
> > +processors,
> > + * the CPU frequency subset and voltage value of each OPP varies
> > + * based on the silicon variant in use. Qualcomm Process Voltage
> > +Scaling Tables
> > + * defines the voltage and frequency value based on the msm-id in
> > +SMEM
> > + * and speedbin blown in the efuse combination.
> > + * The qcom-cpufreq-kryo driver reads the msm-id and efuse value from
> > +the SoC
> > + * to provide the OPP framework with required information.
> > + * This is used to determine the voltage and frequency value for each
> > +OPP of
> > + * operating-points-v2 table when it is parsed by the OPP framework.
> > + */
> > +
> > +#include <linux/cpu.h>
> > +#include <linux/err.h>
> > +#include <linux/init.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/nvmem-consumer.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pm_opp.h>
> > +#include <linux/slab.h>
> > +#include <linux/soc/qcom/smem.h>
> > +
> > +#define MSM_ID_SMEM 137
> > +
> > +enum _msm_id {
> > + MSM8996V3 = 0xF6ul,
> > + APQ8096V3 = 0x123ul,
> > + MSM8996SG = 0x131ul,
> > + APQ8096SG = 0x138ul,
> > +};
> > +
> > +enum _msm8996_version {
> > + MSM8996_V3,
> > + MSM8996_SG,
> > + NUM_OF_MSM8996_VERSIONS,
> > +};
> > +
> > +static enum _msm8996_version __init
> > +qcom_cpufreq_kryo_get_msm_id(void)
> > +{
> > + size_t len;
> > + u32 *msm_id;
> > + enum _msm8996_version version;
> > +
> > + msm_id = qcom_smem_get(QCOM_SMEM_HOST_ANY,
> MSM_ID_SMEM, &len);
> > + /* The first 4 bytes are format, next to them is the actual msm-id */
> > + msm_id++;
> > +
> > + switch ((enum _msm_id)*msm_id) {
> > + case MSM8996V3:
> > + case APQ8096V3:
> > + version = MSM8996_V3;
> > + break;
> > + case MSM8996SG:
> > + case APQ8096SG:
> > + version = MSM8996_SG;
> > + break;
> > + default:
> > + version = NUM_OF_MSM8996_VERSIONS;
> > + }
> > +
> > + return version;
> > +}
> > +
> > +static int qcom_cpufreq_kryo_probe(struct platform_device *pdev) {
> > + struct opp_table *opp_tables[NR_CPUS] = {0};
> > + struct platform_device *cpufreq_dt_pdev;
> > + enum _msm8996_version msm8996_version;
> > + struct nvmem_cell *speedbin_nvmem;
> > + struct device_node *np;
> > + struct device *cpu_dev;
> > + unsigned cpu;
> > + u8 *speedbin;
> > + u32 versions;
> > + size_t len;
> > + int ret;
> > +
> > + cpu_dev = get_cpu_device(0);
> > + if (NULL == cpu_dev)
> > + return -ENODEV;
> > +
> > + msm8996_version = qcom_cpufreq_kryo_get_msm_id();
> > + if (NUM_OF_MSM8996_VERSIONS == msm8996_version) {
> > + dev_err(cpu_dev, "Not Snapdragon 820/821!");
> > + return -ENODEV;
> > + }
> > +
> > + np = dev_pm_opp_of_get_opp_desc_node(cpu_dev);
> > + if (IS_ERR(np))
> > + return PTR_ERR(np);
> > +
> > + if (!of_device_is_compatible(np, "operating-points-v2-kryo-cpu")) {
> > + of_node_put(np);
> > + return -ENOENT;
> > + }
> > +
> > + speedbin_nvmem = of_nvmem_cell_get(np, NULL);
> > + of_node_put(np);
> > + if (IS_ERR(speedbin_nvmem)) {
> > + dev_err(cpu_dev, "Could not get nvmem cell: %ld\n",
> > + PTR_ERR(speedbin_nvmem));
> > + return PTR_ERR(speedbin_nvmem);
> > + }
> > +
> > + speedbin = nvmem_cell_read(speedbin_nvmem, &len);
> > + nvmem_cell_put(speedbin_nvmem);
> > +
> > + switch (msm8996_version) {
> > + case MSM8996_V3:
> > + versions = 1 << (unsigned int)(*speedbin);
> > + break;
> > + case MSM8996_SG:
> > + versions = 1 << ((unsigned int)(*speedbin) + 4);
> > + break;
> > + default:
> > + BUG();
> > + break;
> > + }
> > +
> > + for_each_possible_cpu(cpu) {
> > + cpu_dev = get_cpu_device(cpu);
> > + if (NULL == cpu_dev) {
> > + ret = -ENODEV;
> > + goto free_opp;
> > + }
> > +
> > + opp_tables[cpu] =
> dev_pm_opp_set_supported_hw(cpu_dev,
> > + &versions, 1);
> > + if (IS_ERR(opp_tables[cpu])) {
> > + ret = PTR_ERR(opp_tables[cpu]);
> > + dev_err(cpu_dev, "Failed to set supported
> hardware\n");
> > + goto free_opp;
> > + }
> > + }
> > +
> > + cpufreq_dt_pdev = platform_device_register_simple("cpufreq-dt", -
> 1,
> > + NULL, 0);
> > + if (!IS_ERR(cpufreq_dt_pdev))
> > + return 0;
> > +
> > + ret = PTR_ERR(cpufreq_dt_pdev);
> > + dev_err(cpu_dev, "Failed to register platform device\n");
> > +
> > +free_opp:
> > + for_each_possible_cpu(cpu) {
> > + if (IS_ERR_OR_NULL(opp_tables[cpu]))
> > + break;
> > + dev_pm_opp_put_supported_hw(opp_tables[cpu]);
> > + }
> > +
> > + return ret;
> > +}
> > +
> > +static struct platform_driver qcom_cpufreq_kryo_driver = {
> > + .probe = qcom_cpufreq_kryo_probe,
> > + .driver = {
> > + .name = "qcom-cpufreq-kryo",
> > + },
> > +};
> > +
> > +/*
> > + * Since the driver depends on smem and nvmem drivers, which may
> > + * return EPROBE_DEFER, all the real activity is done in the probe,
> > + * which may be defered as well. The init here is only registering
> > + * the driver and the platform device.
> > + */
> > +static int __init qcom_cpufreq_kryo_init(void) {
> > + int ret;
> > +
> > + ret = platform_driver_register(&qcom_cpufreq_kryo_driver);
> > + if (unlikely(ret < 0))
> > + return ret;
> > +
> > + ret = PTR_ERR_OR_ZERO(platform_device_register_simple(
> > + "qcom-cpufreq-kryo", -1, NULL, 0));
>
>
> You simply can't do this unconditionally here. This will blow up on platforms
> where this driver is not supposed to work. The probe will be called on non-
> QCOM or non-Kryo QCOM platforms and I reckon it will crash trying to
> execute something in qcom_smem_get.
What do you mean by 'unconditionally'?
The driver depends on the smem and nvmem drivers, which depend on ARCH_QCOM:
+ depends on QCOM_QFPROM
+ depends on QCOM_SMEM
And if SMEM read in the probe returns something other than Kryo, it will exit.
>
> --
> Regards,
> Sudeep
^ permalink raw reply
* Re: [PATCH v12 1/2] cpufreq: Add Kryo CPU scaling driver
From: Sudeep Holla @ 2018-05-24 12:51 UTC (permalink / raw)
To: Ilia Lin, vireshk, nm, sboyd, robh, mark.rutland, rjw
Cc: Sudeep Holla, linux-pm, devicetree, linux-kernel
In-Reply-To: <1527152242-31281-2-git-send-email-ilialin@codeaurora.org>
Hi Ilia,
On 24/05/18 09:57, Ilia Lin wrote:
> In Certain QCOM SoCs like apq8096 and msm8996 that have KRYO processors,
> the CPU frequency subset and voltage value of each OPP varies
> based on the silicon variant in use. Qualcomm Process Voltage Scaling Tables
> defines the voltage and frequency value based on the msm-id in SMEM
> and speedbin blown in the efuse combination.
> The qcom-cpufreq-kryo driver reads the msm-id and efuse value from the SoC
> to provide the OPP framework with required information.
> This is used to determine the voltage and frequency value for each OPP of
> operating-points-v2 table when it is parsed by the OPP framework.
>
> Signed-off-by: Ilia Lin <ilialin@codeaurora.org>
> ---
> drivers/cpufreq/Kconfig.arm | 10 ++
> drivers/cpufreq/Makefile | 1 +
> drivers/cpufreq/cpufreq-dt-platdev.c | 3 +
> drivers/cpufreq/qcom-cpufreq-kryo.c | 194 +++++++++++++++++++++++++++++++++++
> 4 files changed, 208 insertions(+)
> create mode 100644 drivers/cpufreq/qcom-cpufreq-kryo.c
>
> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
> index de55c7d..0bfd40e 100644
> --- a/drivers/cpufreq/Kconfig.arm
> +++ b/drivers/cpufreq/Kconfig.arm
> @@ -124,6 +124,16 @@ config ARM_OMAP2PLUS_CPUFREQ
> depends on ARCH_OMAP2PLUS
> default ARCH_OMAP2PLUS
>
> +config ARM_QCOM_CPUFREQ_KRYO
> + bool "Qualcomm Kryo based CPUFreq"
> + depends on QCOM_QFPROM
> + depends on QCOM_SMEM
> + select PM_OPP
> + help
> + This adds the CPUFreq driver for Qualcomm Kryo SoC based boards.
> +
> + If in doubt, say N.
> +
> config ARM_S3C_CPUFREQ
> bool
> help
> diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
> index 8d24ade..fb4a2ec 100644
> --- a/drivers/cpufreq/Makefile
> +++ b/drivers/cpufreq/Makefile
> @@ -65,6 +65,7 @@ obj-$(CONFIG_MACH_MVEBU_V7) += mvebu-cpufreq.o
> obj-$(CONFIG_ARM_OMAP2PLUS_CPUFREQ) += omap-cpufreq.o
> obj-$(CONFIG_ARM_PXA2xx_CPUFREQ) += pxa2xx-cpufreq.o
> obj-$(CONFIG_PXA3xx) += pxa3xx-cpufreq.o
> +obj-$(CONFIG_ARM_QCOM_CPUFREQ_KRYO) += qcom-cpufreq-kryo.o
> obj-$(CONFIG_ARM_S3C2410_CPUFREQ) += s3c2410-cpufreq.o
> obj-$(CONFIG_ARM_S3C2412_CPUFREQ) += s3c2412-cpufreq.o
> obj-$(CONFIG_ARM_S3C2416_CPUFREQ) += s3c2416-cpufreq.o
> diff --git a/drivers/cpufreq/cpufreq-dt-platdev.c b/drivers/cpufreq/cpufreq-dt-platdev.c
> index 3b585e4..77d6ab8 100644
> --- a/drivers/cpufreq/cpufreq-dt-platdev.c
> +++ b/drivers/cpufreq/cpufreq-dt-platdev.c
> @@ -118,6 +118,9 @@
>
> { .compatible = "nvidia,tegra124", },
>
> + { .compatible = "qcom,apq8096", },
> + { .compatible = "qcom,msm8996", },
> +
> { .compatible = "st,stih407", },
> { .compatible = "st,stih410", },
>
> diff --git a/drivers/cpufreq/qcom-cpufreq-kryo.c b/drivers/cpufreq/qcom-cpufreq-kryo.c
> new file mode 100644
> index 0000000..9fe379c
> --- /dev/null
> +++ b/drivers/cpufreq/qcom-cpufreq-kryo.c
> @@ -0,0 +1,194 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
> + */
> +
> +/*
> + * In Certain QCOM SoCs like apq8096 and msm8996 that have KRYO processors,
> + * the CPU frequency subset and voltage value of each OPP varies
> + * based on the silicon variant in use. Qualcomm Process Voltage Scaling Tables
> + * defines the voltage and frequency value based on the msm-id in SMEM
> + * and speedbin blown in the efuse combination.
> + * The qcom-cpufreq-kryo driver reads the msm-id and efuse value from the SoC
> + * to provide the OPP framework with required information.
> + * This is used to determine the voltage and frequency value for each OPP of
> + * operating-points-v2 table when it is parsed by the OPP framework.
> + */
> +
> +#include <linux/cpu.h>
> +#include <linux/err.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/nvmem-consumer.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_opp.h>
> +#include <linux/slab.h>
> +#include <linux/soc/qcom/smem.h>
> +
> +#define MSM_ID_SMEM 137
> +
> +enum _msm_id {
> + MSM8996V3 = 0xF6ul,
> + APQ8096V3 = 0x123ul,
> + MSM8996SG = 0x131ul,
> + APQ8096SG = 0x138ul,
> +};
> +
> +enum _msm8996_version {
> + MSM8996_V3,
> + MSM8996_SG,
> + NUM_OF_MSM8996_VERSIONS,
> +};
> +
> +static enum _msm8996_version __init qcom_cpufreq_kryo_get_msm_id(void)
> +{
> + size_t len;
> + u32 *msm_id;
> + enum _msm8996_version version;
> +
> + msm_id = qcom_smem_get(QCOM_SMEM_HOST_ANY, MSM_ID_SMEM, &len);
> + /* The first 4 bytes are format, next to them is the actual msm-id */
> + msm_id++;
> +
> + switch ((enum _msm_id)*msm_id) {
> + case MSM8996V3:
> + case APQ8096V3:
> + version = MSM8996_V3;
> + break;
> + case MSM8996SG:
> + case APQ8096SG:
> + version = MSM8996_SG;
> + break;
> + default:
> + version = NUM_OF_MSM8996_VERSIONS;
> + }
> +
> + return version;
> +}
> +
> +static int qcom_cpufreq_kryo_probe(struct platform_device *pdev)
> +{
> + struct opp_table *opp_tables[NR_CPUS] = {0};
> + struct platform_device *cpufreq_dt_pdev;
> + enum _msm8996_version msm8996_version;
> + struct nvmem_cell *speedbin_nvmem;
> + struct device_node *np;
> + struct device *cpu_dev;
> + unsigned cpu;
> + u8 *speedbin;
> + u32 versions;
> + size_t len;
> + int ret;
> +
> + cpu_dev = get_cpu_device(0);
> + if (NULL == cpu_dev)
> + return -ENODEV;
> +
> + msm8996_version = qcom_cpufreq_kryo_get_msm_id();
> + if (NUM_OF_MSM8996_VERSIONS == msm8996_version) {
> + dev_err(cpu_dev, "Not Snapdragon 820/821!");
> + return -ENODEV;
> + }
> +
> + np = dev_pm_opp_of_get_opp_desc_node(cpu_dev);
> + if (IS_ERR(np))
> + return PTR_ERR(np);
> +
> + if (!of_device_is_compatible(np, "operating-points-v2-kryo-cpu")) {
> + of_node_put(np);
> + return -ENOENT;
> + }
> +
> + speedbin_nvmem = of_nvmem_cell_get(np, NULL);
> + of_node_put(np);
> + if (IS_ERR(speedbin_nvmem)) {
> + dev_err(cpu_dev, "Could not get nvmem cell: %ld\n",
> + PTR_ERR(speedbin_nvmem));
> + return PTR_ERR(speedbin_nvmem);
> + }
> +
> + speedbin = nvmem_cell_read(speedbin_nvmem, &len);
> + nvmem_cell_put(speedbin_nvmem);
> +
> + switch (msm8996_version) {
> + case MSM8996_V3:
> + versions = 1 << (unsigned int)(*speedbin);
> + break;
> + case MSM8996_SG:
> + versions = 1 << ((unsigned int)(*speedbin) + 4);
> + break;
> + default:
> + BUG();
> + break;
> + }
> +
> + for_each_possible_cpu(cpu) {
> + cpu_dev = get_cpu_device(cpu);
> + if (NULL == cpu_dev) {
> + ret = -ENODEV;
> + goto free_opp;
> + }
> +
> + opp_tables[cpu] = dev_pm_opp_set_supported_hw(cpu_dev,
> + &versions, 1);
> + if (IS_ERR(opp_tables[cpu])) {
> + ret = PTR_ERR(opp_tables[cpu]);
> + dev_err(cpu_dev, "Failed to set supported hardware\n");
> + goto free_opp;
> + }
> + }
> +
> + cpufreq_dt_pdev = platform_device_register_simple("cpufreq-dt", -1,
> + NULL, 0);
> + if (!IS_ERR(cpufreq_dt_pdev))
> + return 0;
> +
> + ret = PTR_ERR(cpufreq_dt_pdev);
> + dev_err(cpu_dev, "Failed to register platform device\n");
> +
> +free_opp:
> + for_each_possible_cpu(cpu) {
> + if (IS_ERR_OR_NULL(opp_tables[cpu]))
> + break;
> + dev_pm_opp_put_supported_hw(opp_tables[cpu]);
> + }
> +
> + return ret;
> +}
> +
> +static struct platform_driver qcom_cpufreq_kryo_driver = {
> + .probe = qcom_cpufreq_kryo_probe,
> + .driver = {
> + .name = "qcom-cpufreq-kryo",
> + },
> +};
> +
> +/*
> + * Since the driver depends on smem and nvmem drivers, which may
> + * return EPROBE_DEFER, all the real activity is done in the probe,
> + * which may be defered as well. The init here is only registering
> + * the driver and the platform device.
> + */
> +static int __init qcom_cpufreq_kryo_init(void)
> +{
> + int ret;
> +
> + ret = platform_driver_register(&qcom_cpufreq_kryo_driver);
> + if (unlikely(ret < 0))
> + return ret;
> +
> + ret = PTR_ERR_OR_ZERO(platform_device_register_simple(
> + "qcom-cpufreq-kryo", -1, NULL, 0));
You simply can't do this unconditionally here. This will blow up on
platforms where this driver is not supposed to work. The probe will be
called on non-QCOM or non-Kryo QCOM platforms and I reckon it will
crash trying to execute something in qcom_smem_get.
--
Regards,
Sudeep
^ permalink raw reply
* Re: [PATCH v1 2/2] cpufreq: tegra20: Use PLL_C as intermediate clock source
From: Dmitry Osipenko @ 2018-05-24 12:49 UTC (permalink / raw)
To: Peter De Schrijver
Cc: Rafael J. Wysocki, Viresh Kumar, Thierry Reding, Jonathan Hunter,
linux-tegra, linux-pm, linux-kernel
In-Reply-To: <20180524100413.GL6835@tbergstrom-lnx.Nvidia.com>
On 24.05.2018 13:04, Peter De Schrijver wrote:
> On Wed, May 23, 2018 at 07:00:20PM +0300, Dmitry Osipenko wrote:
>> PLL_C is running at 600MHz which is significantly higher than the 216MHz
>> of the PLL_P and it is known that PLL_C is always-ON because AHB BUS is
>> running on that PLL. Let's use PLL_C as intermediate clock source, making
>> CPU snappier a tad during of the frequency transition.
>>
>
> pll_c isn't necessarily 600Mhz when used as a source for the second display
> head.
Hmm, indeed.
Even if PLL_C rate will be adjusted, it will be higher than the PLL_P.. won't
it? That's likely to be good enough.
Do you know if any of the available CCLK parents has a glitch-less rate
switching? I.e. CPU won't hang on the rate switch.
There is other possible 600MHz source, the PLL_M. Can we use it? This one also
may become dynamic if we'll consider implementing the memory scaling, but the
memory frequency probably will fit the transition role pretty well.
^ permalink raw reply
* Re: [PATCH v1 2/2] cpufreq: tegra20: Use PLL_C as intermediate clock source
From: Dmitry Osipenko @ 2018-05-24 12:28 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Viresh Kumar, Rafael J. Wysocki, Thierry Reding, Jonathan Hunter,
Peter De Schrijver, linux-tegra, Linux PM,
Linux Kernel Mailing List
In-Reply-To: <CAJZ5v0iXibwwnAyjBXc+zO5YQ0=WwaoUqq1K3OWqojh8xX+Wjw@mail.gmail.com>
On 24.05.2018 11:01, Rafael J. Wysocki wrote:
> On Thu, May 24, 2018 at 7:37 AM, Dmitry Osipenko <digetx@gmail.com> wrote:
>> On 24.05.2018 07:30, Viresh Kumar wrote:
>>> On 23-05-18, 19:00, Dmitry Osipenko wrote:
>>>> PLL_C is running at 600MHz which is significantly higher than the 216MHz
>>>> of the PLL_P and it is known that PLL_C is always-ON because AHB BUS is
>>>> running on that PLL. Let's use PLL_C as intermediate clock source, making
>>>> CPU snappier a tad during of the frequency transition.
>>>>
>>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>>>> ---
>>>> drivers/cpufreq/tegra20-cpufreq.c | 25 +++++++++++++++++++++----
>>>> 1 file changed, 21 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/cpufreq/tegra20-cpufreq.c b/drivers/cpufreq/tegra20-cpufreq.c
>>>> index 3ad6bded6efc..4bf5ba7da40b 100644
>>>> --- a/drivers/cpufreq/tegra20-cpufreq.c
>>>> +++ b/drivers/cpufreq/tegra20-cpufreq.c
>>>> @@ -25,12 +25,13 @@
>>>> #include <linux/types.h>
>>>>
>>>> #define PLL_P_FREQ 216000
>>>> +#define PLL_C_FREQ 600000
>>>>
>>>> static struct cpufreq_frequency_table freq_table[] = {
>>>> { .frequency = 216000 },
>>>> { .frequency = 312000 },
>>>> { .frequency = 456000 },
>>>> - { .frequency = 608000 },
>>>> + { .frequency = 600000 },
>>>> { .frequency = 760000 },
>>>> { .frequency = 816000 },
>>>> { .frequency = 912000 },
>>>> @@ -44,6 +45,7 @@ struct tegra20_cpufreq {
>>>> struct clk *cpu_clk;
>>>> struct clk *pll_x_clk;
>>>> struct clk *pll_p_clk;
>>>> + struct clk *pll_c_clk;
>>>> bool pll_x_prepared;
>>>> };
>>>>
>>>> @@ -58,7 +60,10 @@ static unsigned int tegra_get_intermediate(struct cpufreq_policy *policy,
>>>> if (index == 0 || policy->cur == PLL_P_FREQ)
>>>> return 0;
>>>>
>>>> - return PLL_P_FREQ;
>>>> + if (index == 3 || policy->cur == PLL_C_FREQ)
>>>> + return 0;
>>>
>>> So we can choose between two different intermediate frequencies ? And
>>> I didn't like the way magic number 3 is used here. Its prone to errors
>>> and we better use a macro or something else here.
>>>
>>> Like instead of doing index == 3, what about freq_table[index].freq ==
>>> PLL_C_FREQ ? Same for the previous patch as well.
>>
>> The frequency is determined by the parent clock of CCLK (CPU clock), we can
>> choose between different parents for the CCLK. PLL_C as PLL_P and PLL_X are
>> among the available parents for the CCLK to choose from and there some others.
>>
>> I don't mind to use freq_table[index].freq, though I'd like to keep compiled
>> assembly minimal where possible. Hence the freq_table should be made constant to
>> tell compiler that it doesn't need to emit data fetches for the table values and
>> could embed the constants into the code where appropriate.
>>
>> Could we constify the "struct cpufreq_frequency_table" within the cpufreq core?
>> Seems nothing prevents this (I already tried to constify - there are no
>> obstacles), unless some cpufreq driver would try to modify
>> policy->freq_table->... within the cpufreq callback implementation.
>
> Some drivers generate frequency tables out of external data
> unavailable at compile time, like ACPI tables.
Instead of making the table constant itself (with its values), seems we can just
make the policy->freq_table pointer constant. I'll try to make a patch for that,
adjusting the pointers in cpufreq core and the drivers. This works for the
acpi-cpufreq at least.
> But if you know it for the fact that the core doesn't modify the
> frequency table, you could pass a constant table from the driver to
> it, can't you?
>
Yes, but that will require to explicitly silencing the compiler warning about
const -> non-const pointer conversion (if you're meaning this pointer
conversion), which generally should be avoided.
^ permalink raw reply
* Re: [PATCH v1] cpufreq: tegra20: Fix imbalanced clock enable count
From: Dmitry Osipenko @ 2018-05-24 12:18 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Viresh Kumar, Thierry Reding, Jonathan Hunter, linux-tegra,
linux-pm, linux-kernel
In-Reply-To: <2304436.qRMBXhMGsN@aspire.rjw.lan>
On 24.05.2018 12:36, Rafael J. Wysocki wrote:
> On Wednesday, May 23, 2018 11:30:39 AM CEST Dmitry Osipenko wrote:
>> On 23.05.2018 08:58, Viresh Kumar wrote:
>>> On 23-05-18, 00:14, Dmitry Osipenko wrote:
>>>> Tegra20-cpufreq driver missed enabling the CPU clocks. This results in a
>>>> clock-enable refcount disbalance on PLL_P <-> PLL_X reparent, causing
>>>> PLL_X to get disabled while it shouldn't. Fix this by enabling the clocks
>>>> on the driver probe.
>>>>
>>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>>>> ---
>>>>
>>>> CPUFreq maintainers,
>>>>
>>>> Please take into account that this patch is made on top of my recent
>>>> series of patches [0] "Clean up Tegra20 cpufreq driver" that was fully
>>>> reviewed, but seems not applied yet. Let me know if you prefer to re-spin
>>>> the [0], including this patch into the series.
>>>>
>>>> [0] https://patchwork.ozlabs.org/project/linux-tegra/list/?series=45321
>>>
>>> This is already picked by Rafael and is sitting in pm/bleeding-edge
>>> branch. Should get merged into linux-next in a day or two.
>>
>> Neat, thank you for letting me know.
>
> It actually is there in my linux-next branch, but linux-next proper is not
> taking new material this week AFAICS.
I've found it in yours git repo, thank you.
> You'll see this in linux-next on Monday, most probably.
>
Yes, I noticed that linux-next stopped updating for awhile.
^ permalink raw reply
* Re: [PATCH 8/9] PM / Domains: Add support for multi PM domains per device to genpd
From: Ulf Hansson @ 2018-05-24 12:17 UTC (permalink / raw)
To: Jon Hunter
Cc: Rajendra Nayak, Geert Uytterhoeven, Linux PM, Greg Kroah-Hartman,
Kevin Hilman, Rafael J . Wysocki, Linux Kernel Mailing List,
Todor Tomov, Viresh Kumar, linux-tegra, Vincent Guittot,
Linux ARM
In-Reply-To: <1687797a-93f1-7e5c-6060-01c12c070964@nvidia.com>
On 24 May 2018 at 11:36, Jon Hunter <jonathanh@nvidia.com> wrote:
>
> On 24/05/18 08:04, Ulf Hansson wrote:
>
> ...
>
>>> Any reason why we could not add a 'boolean' argument to the API to
>>> indicate
>>> whether the new device should be linked? I think that I prefer the API
>>> handles it, but I can see there could be instances where drivers may wish
>>> to
>>> handle it themselves.
>>
>>
>> Coming back to this question. Both Tegra XUSB and Qcom Camera use
>> case, would benefit from doing the linking themselves, as it needs
>> different PM domains to be powered on depending on the current use
>> case - as to avoid wasting power.
>>
>> However, I can understand that you prefer some simplicity over
>> optimizations, as you told us. Then, does it mean that you are
>> insisting on extending the APIs with a boolean for linking, or are you
>> fine with the driver to call device_link_add()?
>
>
> I am fine with the driver calling device_link_add(), but I just wonder if we
> will find a several drivers doing this and then we will end up doing this
> later anyway.
Okay.
>
> The current API is called ...
>
> * genpd_dev_pm_attach_by_id() - Attach a device to one of its PM domain.
> * @dev: Device to attach.
> * @index: The index of the PM domain.
>
> This naming and description is a bit misleading, because really it is not
> attaching the device that is passed, but creating a new device to attach a
> PM domain to. So we should consider renaming and changing the description
> and indicate that users need to link the device.
I picked the name to be consistent with the existing
genpd_dev_pm_attach(). Do you have a better suggestion?
I agree, some details is missing to the description, let me try to
improve it. Actually, I was trying to follow existing descriptions
from genpd_dev_pm_attach(), so perhaps that also needs a little
update.
However, do note that, neither genpd_dev_pm_attach() or
genpd_dev_pm_attach_by_id() is supposed to be called by drivers, but
rather only by the driver core. So description may not be so
important.
In regards to good descriptions, for sure the API added in patch9,
dev_pm_domain_attach_by_id(), needs a good one, as this is what
drivers should be using.
>
> Finally, how is a PM domain attached via calling genpd_dev_pm_attach_by_id()
> detached?
Via the existing genpd_dev_pm_detach(), according to what I have
described in the change log. I clarify the description in regards to
this as well.
Kind regards
Uffe
^ permalink raw reply
* Re: [PATCH v3 21/27] x86/ftrace: Adapt function tracing for PIE support
From: Petr Mladek @ 2018-05-24 11:40 UTC (permalink / raw)
To: Thomas Garnier, Steven Rostedt
Cc: Kate Stewart, Nicolas Pitre, x86, Sergey Senozhatsky, kvm,
Len Brown, Peter Zijlstra, Yonghong Song, Christopher Li,
Dave Hansen, Dominik Brodowski, linux-kernel, Masahiro Yamada,
Jan Beulich, Pavel Machek, H . Peter Anvin, kernel-hardening,
Christoph Lameter, Alok Kataria, linux-doc, linux-arch,
Jonathan Corbet, Herbert Xu, Baoquan He, David Woodhouse
In-Reply-To: <20180523195421.180248-22-thgarnie@google.com>
On Wed 2018-05-23 12:54:15, Thomas Garnier wrote:
> When using -fPIE/PIC with function tracing, the compiler generates a
> call through the GOT (call *__fentry__@GOTPCREL). This instruction
> takes 6 bytes instead of 5 on the usual relative call.
>
> If PIE is enabled, replace the 6th byte of the GOT call by a 1-byte nop
> so ftrace can handle the previous 5-bytes as before.
>
> Position Independent Executable (PIE) support will allow to extended the
> KASLR randomization range below the -2G memory limit.
>
> Signed-off-by: Thomas Garnier <thgarnie@google.com>
> ---
> arch/x86/include/asm/ftrace.h | 6 +++--
> arch/x86/include/asm/sections.h | 4 ++++
> arch/x86/kernel/ftrace.c | 42 +++++++++++++++++++++++++++++++--
> 3 files changed, 48 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/include/asm/ftrace.h b/arch/x86/include/asm/ftrace.h
> index c18ed65287d5..8f2decce38d8 100644
> --- a/arch/x86/include/asm/ftrace.h
> +++ b/arch/x86/include/asm/ftrace.h
> @@ -25,9 +25,11 @@ extern void __fentry__(void);
> static inline unsigned long ftrace_call_adjust(unsigned long addr)
> {
> /*
> - * addr is the address of the mcount call instruction.
> - * recordmcount does the necessary offset calculation.
> + * addr is the address of the mcount call instruction. PIE has always a
> + * byte added to the start of the function.
> */
> + if (IS_ENABLED(CONFIG_X86_PIE))
> + addr -= 1;
This seems to modify the address even for modules that are _not_ compiled with
PIE, see below.
> return addr;
> }
>
> diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
> index 01ebcb6f263e..73b3c30cb7a3 100644
> --- a/arch/x86/kernel/ftrace.c
> +++ b/arch/x86/kernel/ftrace.c
> @@ -135,6 +135,44 @@ ftrace_modify_code_direct(unsigned long ip, unsigned const char *old_code,
> return 0;
> }
>
> +/* Bytes before call GOT offset */
> +const unsigned char got_call_preinsn[] = { 0xff, 0x15 };
> +
> +static int
> +ftrace_modify_initial_code(unsigned long ip, unsigned const char *old_code,
> + unsigned const char *new_code)
> +{
> + unsigned char replaced[MCOUNT_INSN_SIZE + 1];
> +
> + ftrace_expected = old_code;
> +
> + /*
> + * If PIE is not enabled or no GOT call was found, default to the
> + * original approach to code modification.
> + */
> + if (!IS_ENABLED(CONFIG_X86_PIE) ||
> + probe_kernel_read(replaced, (void *)ip, sizeof(replaced)) ||
> + memcmp(replaced, got_call_preinsn, sizeof(got_call_preinsn)))
> + return ftrace_modify_code_direct(ip, old_code, new_code);
And this looks like an attempt to handle modules compiled without
PIE. Does it works with the right ip in that case?
I wonder if a better solution would be to update
scripts/recordmcount.c to store the incremented location into the module.
IMPORTANT: I have only vague picture about how this all works. It is
possible that I am completely wrong. The code might be correct,
especially if you tested this situation.
Best Regards,
Petr
^ permalink raw reply
* Re: [PATCH 2/3] thinkpad_acpi: add support for force_discharge
From: Henrique de Moraes Holschuh @ 2018-05-24 11:35 UTC (permalink / raw)
To: Kevin Locke
Cc: Platform Driver, Rafael J. Wysocki, Henrique de Moraes Holschuh,
Linux PM, Rafael J. Wysocki, Robert Moore, Ognjen Galic,
ACPI Devel Maling List, Andy Shevchenko,
Christoph Böhmwalder, Sebastian Reichel, Darren Hart,
devel-E0kO6a4B6psdnm+yROfE0A,
ibm-acpi-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f, Andy Shevchenko,
Len Brown
In-Reply-To: <20180521133112.GA4578@kevinolos>
On Mon, 21 May 2018, Kevin Locke wrote:
> as-expected. The only oddity I noticed is that force_discharge has a
> delay taking effect (<1 sec) transitioning from 0 to 1 (but not 1 to
> 0).
The EC can take its sweet time to obey any such requests ;-)
--
Henrique Holschuh
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
^ permalink raw reply
* Re: [PATCH v12 0/2] Kryo CPU scaling driver
From: Amit Kucheria @ 2018-05-24 11:20 UTC (permalink / raw)
To: Ilia Lin
Cc: vireshk, nm, sboyd, Rob Herring, Mark Rutland, Rafael J. Wysocki,
Linux PM list, devicetree, LKML
In-Reply-To: <1527152242-31281-1-git-send-email-ilialin@codeaurora.org>
On Thu, May 24, 2018 at 11:57 AM, Ilia Lin <ilialin@codeaurora.org> wrote:
> [v12]
> * Addressed comments from Sudeep and Viresh about the single init
>
> [v11]
> * Addressed comment from Russel about device_node reference
> * Addressed comment from Sudeep about the late_initcall
> * Transformed init into probe to take care of deferals
>
> [v10]
> * Split the series into domains
> * Addressed comments from Viresh and Sudeep about logical CPU numbering.
>
> The qcom-cpufreq-kryo driver is aimed to support different SOC versions.
> The driver reads eFuse information and chooses the required OPP subset
> by passing the OPP supported-hw parameter.
>
> The series depends on the series from Viresh:
> https://patchwork.kernel.org/patch/10418139/
>
> The previous spin was here:
> https://patchwork.kernel.org/patch/10421143/
>
> Ilia Lin (2):
> cpufreq: Add Kryo CPU scaling driver
> dt-bindings: cpufreq: Document operating-points-v2-kryo-cpu
>
> .../devicetree/bindings/opp/kryo-cpufreq.txt | 680 +++++++++++++++++++++
> drivers/cpufreq/Kconfig.arm | 10 +
> drivers/cpufreq/Makefile | 1 +
> drivers/cpufreq/cpufreq-dt-platdev.c | 3 +
> drivers/cpufreq/qcom-cpufreq-kryo.c | 194 ++++++
> 5 files changed, 888 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/opp/kryo-cpufreq.txt
> create mode 100644 drivers/cpufreq/qcom-cpufreq-kryo.c
>
> --
> 1.9.1
>
For this series,
Reviewed-by: Amit Kucheria <amit.kucheria@linaro.org>
Tested-by: Amit Kucheria <amit.kucheria@linaro.org>
^ permalink raw reply
* Re: [PATCH v3 11/27] x86/power/64: Adapt assembly for PIE support
From: Pavel Machek @ 2018-05-24 11:03 UTC (permalink / raw)
To: Thomas Garnier
Cc: Herbert Xu, David S . Miller, Thomas Gleixner, Ingo Molnar,
H . Peter Anvin, Peter Zijlstra, Josh Poimboeuf,
Greg Kroah-Hartman, Philippe Ombredanne, Kate Stewart,
Arnaldo Carvalho de Melo, Yonghong Song, Andrey Ryabinin,
Kees Cook, Tom Lendacky, Kirill A . Shutemov, Andy Lutomirski,
Dominik Brodowski, Borislav Petkov, Borislav Petkov,
Rafael J . Wysocki, Len Brown, Juerge
In-Reply-To: <20180523195421.180248-12-thgarnie@google.com>
[-- Attachment #1: Type: text/plain, Size: 1308 bytes --]
On Wed 2018-05-23 12:54:05, Thomas Garnier wrote:
> Change the assembly code to use only relative references of symbols for the
> kernel to be PIE compatible.
>
> Position Independent Executable (PIE) support will allow to extended the
> KASLR randomization range below the -2G memory limit.
>
> Signed-off-by: Thomas Garnier <thgarnie@google.com>
Again, was this tested?
> diff --git a/arch/x86/power/hibernate_asm_64.S b/arch/x86/power/hibernate_asm_64.S
> index ce8da3a0412c..6fdd7bbc3c33 100644
> --- a/arch/x86/power/hibernate_asm_64.S
> +++ b/arch/x86/power/hibernate_asm_64.S
> @@ -24,7 +24,7 @@
> #include <asm/frame.h>
>
> ENTRY(swsusp_arch_suspend)
> - movq $saved_context, %rax
> + leaq saved_context(%rip), %rax
> movq %rsp, pt_regs_sp(%rax)
> movq %rbp, pt_regs_bp(%rax)
> movq %rsi, pt_regs_si(%rax)
> @@ -115,7 +115,7 @@ ENTRY(restore_registers)
> movq %rax, %cr4; # turn PGE back on
>
> /* We don't restore %rax, it must be 0 anyway */
> - movq $saved_context, %rax
> + leaq saved_context(%rip), %rax
> movq pt_regs_sp(%rax), %rsp
> movq pt_regs_bp(%rax), %rbp
> movq pt_regs_si(%rax), %rsi
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply
* Re: [PATCH v3 09/27] x86/acpi: Adapt assembly for PIE support
From: Pavel Machek @ 2018-05-24 11:03 UTC (permalink / raw)
To: Thomas Garnier
Cc: Herbert Xu, David S . Miller, Thomas Gleixner, Ingo Molnar,
H . Peter Anvin, Peter Zijlstra, Josh Poimboeuf,
Greg Kroah-Hartman, Philippe Ombredanne, Kate Stewart,
Arnaldo Carvalho de Melo, Yonghong Song, Andrey Ryabinin,
Kees Cook, Tom Lendacky, Kirill A . Shutemov, Andy Lutomirski,
Dominik Brodowski, Borislav Petkov, Borislav Petkov,
Rafael J . Wysocki, Len Brown, Juerge
In-Reply-To: <20180523195421.180248-10-thgarnie@google.com>
[-- Attachment #1: Type: text/plain, Size: 1031 bytes --]
On Wed 2018-05-23 12:54:03, Thomas Garnier wrote:
> Change the assembly code to use only relative references of symbols for the
> kernel to be PIE compatible.
>
> Position Independent Executable (PIE) support will allow to extended the
> KASLR randomization range below the -2G memory limit.
What testing did this get?
> diff --git a/arch/x86/kernel/acpi/wakeup_64.S b/arch/x86/kernel/acpi/wakeup_64.S
> index 50b8ed0317a3..472659c0f811 100644
> --- a/arch/x86/kernel/acpi/wakeup_64.S
> +++ b/arch/x86/kernel/acpi/wakeup_64.S
> @@ -14,7 +14,7 @@
> * Hooray, we are in Long 64-bit mode (but still running in low memory)
> */
> ENTRY(wakeup_long64)
> - movq saved_magic, %rax
> + movq saved_magic(%rip), %rax
> movq $0x123456789abcdef0, %rdx
> cmpq %rdx, %rax
> jne bogus_64_magic
Because, as comment says, this is rather tricky code.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply
* Re: [PATCH] PM / core: Fix direct_complete handling for devices with no callbacks
From: Rafael J. Wysocki @ 2018-05-24 10:13 UTC (permalink / raw)
To: Ulf Hansson
Cc: Linux PM, LKML, Thomas Martitz, Lukas Wunner, Greg Kroah-Hartman
In-Reply-To: <CAPDyKFonGtpv+oEW5bm1XAO7VNuqRR7ZQ7NT5ow3OuoQJQPwmg@mail.gmail.com>
On Tuesday, May 22, 2018 1:41:06 PM CEST Ulf Hansson wrote:
> On 22 May 2018 at 13:02, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > Commit 08810a4119aa (PM / core: Add NEVER_SKIP and SMART_PREPARE
> > driver flags) inadvertently prevented the power.direct_complete flag
> > from being set for devices without PM callbacks and with disabled
> > runtime PM which also prevents power.direct_complete from being set
> > for their parents. That led to problems including a resume crash on
> > HP ZBook 14u.
> >
> > Restore the previous behavior by causing power.direct_complete to be
> > set for those devices again, but do that in a more direct way to
> > avoid overlooking that case in the future.
> >
> > Link: https://bugzilla.kernel.org/show_bug.cgi?id=199693
> > Fixes: 08810a4119aa (PM / core: Add NEVER_SKIP and SMART_PREPARE driver flags)
> > Reported-by: Thomas Martitz <kugel@rockbox.org>
> > Tested-by: Thomas Martitz <kugel@rockbox.org>
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> It seems like the resume path of HP ZBook 14u is kind of fragile,
Yes, it is.
> in case it *requires* dev->power.direct_complete to be set for devices
> like these. And that makes me wonder, that perhaps we should try to
> address that issue as well, no?
Yes, in principle.
But since the direct_complete handling needs to be fixed anyway, it doesn't
matter a lot in practice, because the resume issue on HP ZBook 14u will not
be reproducible anyway then. And since the dependency clearly is on a device
with no callbacks, I'm not worried too much about that.
^ permalink raw reply
* [GIT PULL] Power management fix for v4.17-rc7
From: Rafael J. Wysocki @ 2018-05-24 10:09 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Linux PM, Linux Kernel Mailing List
Hi Linus,
Please pull from the tag
git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git \
pm-4.17-rc7
with top-most commit c62ec4610c40bcc44f2d3d5ed1c312737279e2f3
PM / core: Fix direct_complete handling for devices with no callbacks
on top of commit 771c577c23bac90597c685971d7297ea00f99d11
Linux 4.17-rc6
to receive a power management fix for 4.17-rc7.
This fixes a regression from the 4.15 cycle that caused the system
suspend and resume overhead to increase on many systems and triggered
more serious problems on some of them (Rafael Wysocki).
Thanks!
---------------
Rafael J. Wysocki (1):
PM / core: Fix direct_complete handling for devices with no callbacks
---------------
drivers/base/power/main.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
^ permalink raw reply
* Re: [PATCH v1 2/2] cpufreq: tegra20: Use PLL_C as intermediate clock source
From: Peter De Schrijver @ 2018-05-24 10:04 UTC (permalink / raw)
To: Dmitry Osipenko
Cc: Rafael J. Wysocki, Viresh Kumar, Thierry Reding, Jonathan Hunter,
linux-tegra, linux-pm, linux-kernel
In-Reply-To: <20180523160020.15291-2-digetx@gmail.com>
On Wed, May 23, 2018 at 07:00:20PM +0300, Dmitry Osipenko wrote:
> PLL_C is running at 600MHz which is significantly higher than the 216MHz
> of the PLL_P and it is known that PLL_C is always-ON because AHB BUS is
> running on that PLL. Let's use PLL_C as intermediate clock source, making
> CPU snappier a tad during of the frequency transition.
>
pll_c isn't necessarily 600Mhz when used as a source for the second display
head.
Peter.
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
> drivers/cpufreq/tegra20-cpufreq.c | 25 +++++++++++++++++++++----
> 1 file changed, 21 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/cpufreq/tegra20-cpufreq.c b/drivers/cpufreq/tegra20-cpufreq.c
> index 3ad6bded6efc..4bf5ba7da40b 100644
> --- a/drivers/cpufreq/tegra20-cpufreq.c
> +++ b/drivers/cpufreq/tegra20-cpufreq.c
> @@ -25,12 +25,13 @@
> #include <linux/types.h>
>
> #define PLL_P_FREQ 216000
> +#define PLL_C_FREQ 600000
>
> static struct cpufreq_frequency_table freq_table[] = {
> { .frequency = 216000 },
> { .frequency = 312000 },
> { .frequency = 456000 },
> - { .frequency = 608000 },
> + { .frequency = 600000 },
> { .frequency = 760000 },
> { .frequency = 816000 },
> { .frequency = 912000 },
> @@ -44,6 +45,7 @@ struct tegra20_cpufreq {
> struct clk *cpu_clk;
> struct clk *pll_x_clk;
> struct clk *pll_p_clk;
> + struct clk *pll_c_clk;
> bool pll_x_prepared;
> };
>
> @@ -58,7 +60,10 @@ static unsigned int tegra_get_intermediate(struct cpufreq_policy *policy,
> if (index == 0 || policy->cur == PLL_P_FREQ)
> return 0;
>
> - return PLL_P_FREQ;
> + if (index == 3 || policy->cur == PLL_C_FREQ)
> + return 0;
> +
> + return PLL_C_FREQ;
> }
>
> static int tegra_target_intermediate(struct cpufreq_policy *policy,
> @@ -79,7 +84,7 @@ static int tegra_target_intermediate(struct cpufreq_policy *policy,
> */
> clk_prepare_enable(cpufreq->pll_x_clk);
>
> - ret = clk_set_parent(cpufreq->cpu_clk, cpufreq->pll_p_clk);
> + ret = clk_set_parent(cpufreq->cpu_clk, cpufreq->pll_c_clk);
> if (ret)
> clk_disable_unprepare(cpufreq->pll_x_clk);
> else
> @@ -101,6 +106,9 @@ static int tegra_target(struct cpufreq_policy *policy, unsigned int index)
> if (index == 0)
> return clk_set_parent(cpufreq->cpu_clk, cpufreq->pll_p_clk);
>
> + if (index == 3)
> + return clk_set_parent(cpufreq->cpu_clk, cpufreq->pll_c_clk);
> +
> ret = clk_set_rate(cpufreq->pll_x_clk, rate * 1000);
> /* Restore to earlier frequency on error, i.e. pll_x */
> if (ret)
> @@ -174,6 +182,12 @@ static int tegra20_cpufreq_probe(struct platform_device *pdev)
> goto put_pll_x;
> }
>
> + cpufreq->pll_c_clk = clk_get_sys(NULL, "pll_c");
> + if (IS_ERR(cpufreq->pll_c_clk)) {
> + err = PTR_ERR(cpufreq->pll_c_clk);
> + goto put_pll_p;
> + }
> +
> cpufreq->dev = &pdev->dev;
> cpufreq->driver.get = cpufreq_generic_get;
> cpufreq->driver.attr = cpufreq_generic_attr;
> @@ -190,12 +204,14 @@ static int tegra20_cpufreq_probe(struct platform_device *pdev)
>
> err = cpufreq_register_driver(&cpufreq->driver);
> if (err)
> - goto put_pll_p;
> + goto put_pll_c;
>
> platform_set_drvdata(pdev, cpufreq);
>
> return 0;
>
> +put_pll_c:
> + clk_put(cpufreq->pll_c_clk);
> put_pll_p:
> clk_put(cpufreq->pll_p_clk);
> put_pll_x:
> @@ -212,6 +228,7 @@ static int tegra20_cpufreq_remove(struct platform_device *pdev)
>
> cpufreq_unregister_driver(&cpufreq->driver);
>
> + clk_put(cpufreq->pll_c_clk);
> clk_put(cpufreq->pll_p_clk);
> clk_put(cpufreq->pll_x_clk);
> clk_put(cpufreq->cpu_clk);
> --
> 2.17.0
>
^ permalink raw reply
* Re: [RFD] Add debugfs for cpuidle ?
From: Rafael J. Wysocki @ 2018-05-24 9:39 UTC (permalink / raw)
To: Daniel Lezcano; +Cc: Linux Kernel Mailing List, Linux PM mailing list
In-Reply-To: <bcdbf067-4213-18ed-6d30-dc0dd2376c02@linaro.org>
On Thursday, May 24, 2018 10:47:54 AM CEST Daniel Lezcano wrote:
> On 24/05/2018 10:28, Rafael J. Wysocki wrote:
> > On Thursday, May 24, 2018 10:14:31 AM CEST Daniel Lezcano wrote:
> >>
> >> Hi all,
> >
> > Hi,
> >
> >> would it be interesting to add debugfs metrics for cpuidle in order
> >> to read some statistics from there and investigate the behavior ?
> >
> > What exactly would you like to see in there?
>
> It could be interesting to have statistics on a per cpu and global basis
> for:
>
> - prediction accuracy:
>
> /sys/kernel/debug/cpuidle/prediction/[good|over|under]
> /sys/kernel/debug/cpuidle/cpu*/state*/prediction/[good|over|under]
>
> - exit latency
>
> In addition, it is often the case the idle states values are dropped
> randomly, it is interesting to see the average exit latency for the
> states. That could give also an indication the cluster idle state success.
>
> /sys/kernel/debug/cpuidle/state*/latency/[min|max|avg|stddev]
> /sys/kernel/debug/cpuidle/cpu*/state*/prediction/[min|max|avg|stddev]
>
> And perhaps more information like the tick stopped stat ?
Fair enough, but that must be minimum overhead.
^ 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