* Re: [PATCH v10 1/2] cpufreq: Add Kryo CPU scaling driver
From: Viresh Kumar @ 2018-05-23 10:03 UTC (permalink / raw)
To: Ilia Lin
Cc: vireshk, nm, sboyd, robh, mark.rutland, rjw, linux-pm, devicetree,
linux-kernel
In-Reply-To: <1527068454-28921-2-git-send-email-ilialin@codeaurora.org>
On 23-05-18, 12:40, 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>
Well I gave you an Ack and you should have kept it here :(
> +static int __init qcom_cpufreq_kryo_driver_init(void)
> +{
> + struct opp_table *opp_tables[NR_CPUS] = {0};
> + enum _msm8996_version msm8996_version;
> + struct nvmem_cell *speedbin_nvmem;
> + struct platform_device *pdev;
> + 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")) {
> + ret = -ENOENT;
> + goto free_np;
As Russell pointed out, drop this goto here and write:
of_node_put(np);
return -ENOENT;
}
> + }
> +
> + speedbin_nvmem = of_nvmem_cell_get(np, NULL);
And do the same here unconditionally as you don't need to use np
anymore, i.e.
of_node_put(np);
> + if (IS_ERR(speedbin_nvmem)) {
> + ret = PTR_ERR(speedbin_nvmem);
> + dev_err(cpu_dev, "Could not get nvmem cell: %d\n", ret);
> + goto free_np;
> + }
> +
> + 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;
> + }
> + }
> +
> + pdev = platform_device_register_simple("cpufreq-dt", -1, NULL, 0);
> + if (!IS_ERR(pdev))
> + return 0;
> +
> + ret = PTR_ERR(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]);
> + }
> +free_np:
> + of_node_put(np);
And then you can remove the label and above statement.
> +
> + return ret;
> +}
> +late_initcall(qcom_cpufreq_kryo_driver_init);
> +
> +MODULE_DESCRIPTION("Qualcomm Technologies, Inc. Kryo CPUfreq driver");
> +MODULE_LICENSE("GPL v2");
> --
> 1.9.1
--
viresh
^ permalink raw reply
* Re: [PATCH v3 00/13] add power domain support for Rockchip Socs
From: Ulf Hansson @ 2018-05-23 10:02 UTC (permalink / raw)
To: Elaine Zhang
Cc: Heiko Stuebner, Rob Herring, Mark Rutland, devicetree,
Rafael J. Wysocki, Kevin Hilman, Linux PM, Linux ARM,
open list:ARM/Rockchip SoC..., Linux Kernel Mailing List,
Caesar Wang, xxx, Feng Xiao, Tao Huang
In-Reply-To: <1527058129-10260-1-git-send-email-zhangqing@rock-chips.com>
On 23 May 2018 at 08:48, Elaine Zhang <zhangqing@rock-chips.com> wrote:
> add power domain support for RK3036/RK3128/RK3228/PX30 Soc.
> fix up the wrong value when set power domain up.
>
> Change in V2:
> Fix up the commit message description and Assign author.
>
> Change in V3:
> [PATCH 01/13]: The Copyright description use SPDX tag instead.
> [PATCH 05/13]: The Copyright description use SPDX tag instead.
> [PATCH 08/13]: The Copyright description use SPDX tag instead.
> [PATCH 11/13]: The Copyright description use SPDX tag instead.
>
> Caesar Wang (3):
> dt-bindings: power: add RK3036 SoCs header for power-domain
> dt-bindings: add binding for rk3036 power domains
> Soc: rockchip: power-domain: add power domain support for rk3036
>
> Elaine Zhang (6):
> dt-bindings: power: add RK3128 SoCs header for power-domain
> dt-bindings: add binding for rk3128 power domains
> soc: rockchip: power-domain: add power domain support for rk3128
> dt-bindings: power: add RK3228 SoCs header for power-domain
> dt-bindings: add binding for rk3228 power domains
> soc: rockchip: power-domain: add power domain support for rk3228
>
> Finley Xiao (4):
> soc: rockchip: power-domain: Fix wrong value when power up pd
> dt-bindings: power: add PX30 SoCs header for power-domain
> dt-bindings: add binding for px30 power domains
> soc: rockchip: power-domain: add power domain support for px30
>
> .../bindings/soc/rockchip/power_domain.txt | 12 +++
> drivers/soc/rockchip/pm_domains.c | 116 ++++++++++++++++++++-
> include/dt-bindings/power/px30-power.h | 27 +++++
> include/dt-bindings/power/rk3036-power.h | 13 +++
> include/dt-bindings/power/rk3128-power.h | 14 +++
> include/dt-bindings/power/rk3228-power.h | 21 ++++
> 6 files changed, 202 insertions(+), 1 deletion(-)
> create mode 100644 include/dt-bindings/power/px30-power.h
> create mode 100644 include/dt-bindings/power/rk3036-power.h
> create mode 100644 include/dt-bindings/power/rk3128-power.h
> create mode 100644 include/dt-bindings/power/rk3228-power.h
>
> --
> 1.9.1
>
>
Seems like the changes in v3 is very small, so feel free to add, for the series:
Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
Kind regards
Uffe
^ permalink raw reply
* Re: [PATCH] cpufreq: Add Kryo CPU scaling driver
From: Viresh Kumar @ 2018-05-23 9:59 UTC (permalink / raw)
To: Russell King - ARM Linux
Cc: Ilia Lin, rjw, sudeep.holla, linux-clk, devicetree, linux-kernel,
linux-pm, linux-arm-msm, linux-soc, linux-arm-kernel
In-Reply-To: <20180523094033.GW17671@n2100.armlinux.org.uk>
On 23-05-18, 10:40, Russell King - ARM Linux wrote:
> On Wed, May 23, 2018 at 12:05:24PM +0300, Ilia Lin wrote:
> > + np = dev_pm_opp_of_get_opp_desc_node(cpu_dev);
> > + if (IS_ERR(np))
> > + return PTR_ERR(np);
> ...
> > +
> > + pdev = platform_device_register_simple("cpufreq-dt", -1, NULL, 0);
> > + if (!IS_ERR(pdev))
>
> Do you need to hold a reference to `np' here?
I am starting to feel bad for Ilia now. The problem is that there was
a lot of stuff wrong with the patch and even with so many reviewers it
wasn't easy to notice all the problems it had.
But you are right, this reference needs to be dropped.
--
viresh
^ permalink raw reply
* Re: [PATCH V3] powercap/drivers/idle_injection: Add an idle injection framework
From: Viresh Kumar @ 2018-05-23 9:55 UTC (permalink / raw)
To: Daniel Lezcano
Cc: rjw, edubezval, kevin.wangtao, leo.yan, vincent.guittot,
linux-kernel, javi.merino, rui.zhang, linux-pm, daniel.thompson
In-Reply-To: <61c2b628-6c38-463a-c6b5-5e5b7eeee1ab@linaro.org>
On 23-05-18, 10:00, Daniel Lezcano wrote:
> On 23/05/2018 07:41, Viresh Kumar wrote:
> > Right and that's why I said "Which is fine" in my comment above. My
> > question was more on why we error out in idle_injection_start() if
> > run_duration_ms is 0.
> >
> > Just for my understanding, is it a valid usecase where we want to run
> > the idle loop only once ? i.e. set idle_duration_ms to a non-zero
> > value but run_duration_ms to 0 ? In that case we shouldn't check for
> > zero run_duration_ms in idle_injection_start().
>
> Yes, that could be a valid use case if we want to synchronously inject
> idle cycles without period.
>
> IOW, call play_idle() on a set of cpus at the same time. And the caller
> of start is the one with the control of the period.
>
> If you want this usecase, we need to implement more things:
Well I was trying to understand the use case you have in mind. Nothing
else. So this stuff isn't required anymore.
> >>>> +void idle_injection_set_duration(struct idle_injection_device *ii_dev,
> >>>> + unsigned int run_duration_ms,
> >>>> + unsigned int idle_duration_ms)
> >>>> +{
> >>>> + atomic_set(&ii_dev->run_duration_ms, run_duration_ms);
> >>>> + atomic_set(&ii_dev->idle_duration_ms, idle_duration_ms);
> >>>
> >>> You check for valid values of these in idle_injection_start() but not
> >>> here, why ?
> >>
> >> By checking against a zero values in the start function is a way to make
> >> sure we are not starting the idle injection with uninitialized values
> >> and by setting the duration to zero is a way to stop the idle injection.
> >
> > Why do we need two ways of stopping the idle injection thread ? Why
> > isn't just calling idle_injection_stop() the right thing to do in that
> > case ?
>
> How do we prevent the last kthread in the idle_injection_fn to set the
> timer ?
Okay, I get the problem now. But this doesn't stop the kthread in a
guaranteed way and we probably need a different solution. More later..
> >>>> +void idle_injection_stop(struct idle_injection_device *ii_dev)
> >>>> +{
> >>>> + pr_debug("Stopping injecting idle cycles on CPUs '%*pbl'\n",
> >>>> + cpumask_pr_args(ii_dev->cpumask));
> >>>> +
> >>>> + hrtimer_cancel(&ii_dev->timer);
> >>>
> >>> How are we sure that idle_injection_fn() isn't running at this point
> >>> and it would start the timer cancelled here again ?
> >>
> >> Nothing will ensure that. We will have an extra idle injection in this
> >> case. We can invert the set_duration(0,0) and the timer cancellation to
> >> reduce to reduce the window.
> >
> > That's what I thought and so its racy. If someone calls
> > idle_injection_unregister(), then we call this routine and then free
> > the data structures while they are still getting used by the thread :(
>
> Yes, we need to make the framework single-user, a refcount should be
> enough. However, register() returns a pointer and the caller of
> unregister must have this pointer. If it is the case, then register and
> unregister code collaborate, if the one calling unregister cuts the
> branch of the user of the idle_injection then we have braindead code.
>
> We can handle this case by adding locks or we can have a single-user of
> the framework without lock. We don't expect a lot of idle injection
> users (I see only two right now and they are mutually exclusive), so
> having lockless code is ok for me.
Maybe I wasn't able to explain the problem I see, but lemme retry
that. Assume that there is only one use and that id cpu-idle-cooling.
We are currently running the idle loop with idle duration X and run
duration Y.
Now lets say the cooling device gets unregistered itself (maybe module
removal, etc). And it calls idle_injection_unregister() with a valid
pointer. Not sure if the thermal framework will call set_cur_state
anymore. But the problem will remain even if it does that.
We call idle_injection_stop() from unregister, which will cancel
hrtimer, set durations as 0 and return. Then we free the iidev. It is
certainly possible at this point of time that the kthread is still
running the idle loop which it may have started before unregister was
called. And so after the idle loop is finished it will try to access
ii_dev, which is already freed.
So, idle_injection_stop() needs to guarantee that the kthread and the
hrtimer are all stopped now and no one is using the ii_dev structure
anymore.
Perhaps you need some completion stuff here to give confirmation here,
etc.
--
viresh
^ permalink raw reply
* [PATCH] cpufreq: schedutil: Avoid missing updates for one-CPU policies
From: Rafael J. Wysocki @ 2018-05-23 9:47 UTC (permalink / raw)
To: Linux PM
Cc: LKML, Peter Zijlstra, Viresh Kumar, Juri Lelli, Joel Fernandes,
Patrick Bellasi, claudio, Todd Kjos
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Commit 152db033d775 (schedutil: Allow cpufreq requests to be made
even when kthread kicked) made changes to prevent utilization updates
from being discarded during processing a previous request, but it
left a small window in which that still can happen in the one-CPU
policy case. Namely, updates coming in after setting work_in_progress
in sugov_update_commit() and clearing it in sugov_work() will still
be dropped due to the work_in_progress check in sugov_update_single().
To close that window, rearrange the code so as to acquire the update
lock around the deferred update branch in sugov_update_single()
and drop the work_in_progress check from it.
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
kernel/sched/cpufreq_schedutil.c | 70 ++++++++++++++++++++++++++-------------
1 file changed, 47 insertions(+), 23 deletions(-)
Index: linux-pm/kernel/sched/cpufreq_schedutil.c
===================================================================
--- linux-pm.orig/kernel/sched/cpufreq_schedutil.c
+++ linux-pm/kernel/sched/cpufreq_schedutil.c
@@ -100,25 +100,41 @@ static bool sugov_should_update_freq(str
return delta_ns >= sg_policy->freq_update_delay_ns;
}
-static void sugov_update_commit(struct sugov_policy *sg_policy, u64 time,
- unsigned int next_freq)
+static bool sugov_update_next_freq(struct sugov_policy *sg_policy, u64 time,
+ unsigned int next_freq)
{
- struct cpufreq_policy *policy = sg_policy->policy;
-
if (sg_policy->next_freq == next_freq)
- return;
+ return false;
sg_policy->next_freq = next_freq;
sg_policy->last_freq_update_time = time;
- if (policy->fast_switch_enabled) {
- next_freq = cpufreq_driver_fast_switch(policy, next_freq);
- if (!next_freq)
- return;
+ return true;
+}
+
+static void sugov_fast_switch(struct sugov_policy *sg_policy, u64 time,
+ unsigned int next_freq)
+{
+ struct cpufreq_policy *policy = sg_policy->policy;
+
+ if (!sugov_update_next_freq(sg_policy, time, next_freq))
+ return;
+
+ next_freq = cpufreq_driver_fast_switch(policy, next_freq);
+ if (!next_freq)
+ return;
- policy->cur = next_freq;
- trace_cpu_frequency(next_freq, smp_processor_id());
- } else if (!sg_policy->work_in_progress) {
+ policy->cur = next_freq;
+ trace_cpu_frequency(next_freq, smp_processor_id());
+}
+
+static void sugov_deferred_update(struct sugov_policy *sg_policy, u64 time,
+ unsigned int next_freq)
+{
+ if (!sugov_update_next_freq(sg_policy, time, next_freq))
+ return;
+
+ if (!sg_policy->work_in_progress) {
sg_policy->work_in_progress = true;
irq_work_queue(&sg_policy->irq_work);
}
@@ -363,13 +379,6 @@ static void sugov_update_single(struct u
ignore_dl_rate_limit(sg_cpu, sg_policy);
- /*
- * For slow-switch systems, single policy requests can't run at the
- * moment if update is in progress, unless we acquire update_lock.
- */
- if (sg_policy->work_in_progress)
- return;
-
if (!sugov_should_update_freq(sg_policy, time))
return;
@@ -391,7 +400,18 @@ static void sugov_update_single(struct u
sg_policy->cached_raw_freq = 0;
}
- sugov_update_commit(sg_policy, time, next_f);
+ /*
+ * This code runs under rq->lock for the target CPU, so it won't run
+ * concurrently on two different CPUs for the same target and it is not
+ * necessary to acquire the lock in the fast switch case.
+ */
+ if (sg_policy->policy->fast_switch_enabled) {
+ sugov_fast_switch(sg_policy, time, next_f);
+ } else {
+ raw_spin_lock(&sg_policy->update_lock);
+ sugov_deferred_update(sg_policy, time, next_f);
+ raw_spin_unlock(&sg_policy->update_lock);
+ }
}
static unsigned int sugov_next_freq_shared(struct sugov_cpu *sg_cpu, u64 time)
@@ -435,7 +455,11 @@ sugov_update_shared(struct update_util_d
if (sugov_should_update_freq(sg_policy, time)) {
next_f = sugov_next_freq_shared(sg_cpu, time);
- sugov_update_commit(sg_policy, time, next_f);
+
+ if (sg_policy->policy->fast_switch_enabled)
+ sugov_fast_switch(sg_policy, time, next_f);
+ else
+ sugov_deferred_update(sg_policy, time, next_f);
}
raw_spin_unlock(&sg_policy->update_lock);
@@ -450,11 +474,11 @@ static void sugov_work(struct kthread_wo
/*
* Hold sg_policy->update_lock shortly to handle the case where:
* incase sg_policy->next_freq is read here, and then updated by
- * sugov_update_shared just before work_in_progress is set to false
+ * sugov_deferred_update() just before work_in_progress is set to false
* here, we may miss queueing the new update.
*
* Note: If a work was queued after the update_lock is released,
- * sugov_work will just be called again by kthread_work code; and the
+ * sugov_work() will just be called again by kthread_work code; and the
* request will be proceed before the sugov thread sleeps.
*/
raw_spin_lock_irqsave(&sg_policy->update_lock, flags);
^ permalink raw reply
* Re: [PATCH 8/9] PM / Domains: Add support for multi PM domains per device to genpd
From: Ulf Hansson @ 2018-05-23 9:47 UTC (permalink / raw)
To: Jon Hunter
Cc: Rajendra Nayak, Geert Uytterhoeven, Kevin Hilman,
Greg Kroah-Hartman, Linux PM, Rafael J . Wysocki,
Linux Kernel Mailing List, Todor Tomov, Viresh Kumar, linux-tegra,
Vincent Guittot, Linux ARM
In-Reply-To: <22068fb1-f6e8-f2ec-d7f4-ab9e93469d7f@nvidia.com>
On 23 May 2018 at 11:45, Jon Hunter <jonathanh@nvidia.com> wrote:
>
> On 23/05/18 10:33, Ulf Hansson wrote:
>>
>> On 23 May 2018 at 11:27, Rajendra Nayak <rnayak@codeaurora.org> wrote:
>>>
>>>
>>>
>>> On 05/23/2018 02:37 PM, Jon Hunter wrote:
>>>>
>>>>
>>>> On 23/05/18 07:12, Ulf Hansson wrote:
>>>>
>>>> ...
>>>>
>>>>>>>>> Thanks for sending this. Believe it or not this has still been on
>>>>>>>>> my to-do list
>>>>>>>>> and so we definitely need a solution for Tegra.
>>>>>>>>>
>>>>>>>>> Looking at the above it appears that additional power-domains
>>>>>>>>> exposed as devices
>>>>>>>>> to the client device. So I assume that this means that the drivers
>>>>>>>>> for devices
>>>>>>>>> with multiple power-domains will need to call RPM APIs for each of
>>>>>>>>> these
>>>>>>>>> additional power-domains. Is that correct?
>>>>>>>>
>>>>>>>>
>>>>>>>> They can, but should not!
>>>>>>>>
>>>>>>>> Instead, the driver shall use device_link_add() and
>>>>>>>> device_link_del(),
>>>>>>>> dynamically, depending on what PM domain that their original device
>>>>>>>> needs for the current running use case.
>>>>>>>>
>>>>>>>> In that way, they keep existing runtime PM deployment, operating on
>>>>>>>> its original device.
>>>>>>>
>>>>>>>
>>>>>>> OK, sounds good. Any reason why the linking cannot be handled by the
>>>>>>> above API? Is there a use-case where you would not want it linked?
>>>>>>
>>>>>>
>>>>>> I am guessing the linking is what would give the driver the ability to
>>>>>> decide which subset of powerdomains it actually wants to control
>>>>>> at any point using runtime PM. If we have cases wherein the driver
>>>>>> would want to turn on/off _all_ its associated powerdomains _always_
>>>>>> then a default linking of all would help.
>>>>>
>>>>>
>>>>> First, I think we need to decide on *where* the linking should be
>>>>> done, not at both places, as that would just mess up synchronization
>>>>> of who is responsible for calling the device_link_del() at detach.
>>>>>
>>>>> Second, It would in principle be fine to call device_link_add() and
>>>>> device_link_del() as a part of the attach/detach APIs. However, there
>>>>> is a downside to such solution, which would be that the driver then
>>>>> needs call the detach API, just to do device_link_del(). Of course
>>>>> then it would also needs to call the attach API later if/when needed.
>>>>> Doing this adds unnecessary overhead - comparing to just let the
>>>>> driver call device_link_add|del() when needed. On the upside, yes, it
>>>>> would put less burden on the drivers as it then only needs to care
>>>>> about using one set of functions.
>>>>>
>>>>> Which solution do you prefer?
>>>>
>>>>
>>>> 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.
>>>>
>>>> Rajendra, do you have a use-case right now where the driver would want
>>>> to handle the linking?
>>>
>>>
>>> So if I understand this right, any driver which does want to control
>>> individual powerdomain state would
>>> need to do the linking itself right?
>>>
>>> What I am saying is, if I have device A, with powerdomains X and Y, and
>>> if I want to turn on only X,
>>> then I would want only X to be linked with A, and at a later point if I
>>> want both X and Y to be turned on,
>>> I would then go ahead and link both X and Y to A? Is that correct or did
>>> I get it all wrong?
>>
>>
>> Correct!
>>
>>>
>>> I know atleast Camera on msm8996 would need to do this since it has 2 vfe
>>> powerdoamins, which can be
>>> turned on one at a time (depending on what resolution needs to be
>>> supported) or both together if we
>>> really need very high resolution using both vfe modules.
>>
>>
>> I think this is also the case for the Tegra XUSB subsystem.
>>
>> The usb device is always attached to one PM domain, but depending on
>> if super-speed mode is used, another PM domain for that logic needs to
>> be powered on as well.
>>
>> Jon, please correct me if I am wrong!
>
>
> Yes this is technically correct, however, in reality I think we are always
> going to enable the superspeed domain if either the host or device domain is
> enabled. So we would probably always link the superspeed with the host and
> device devices.
Why? Wouldn't that waste power if the superspeed mode isn't used?
Kind regards
Uffe
^ permalink raw reply
* Re: [PATCH 8/9] PM / Domains: Add support for multi PM domains per device to genpd
From: Jon Hunter @ 2018-05-23 9:45 UTC (permalink / raw)
To: Ulf Hansson, Rajendra Nayak
Cc: 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: <CAPDyKFoqLPRwEJ9b+XBpB4Zy79SGEC-a8V8shNhMkYOCQa0=oA@mail.gmail.com>
On 23/05/18 10:33, Ulf Hansson wrote:
> On 23 May 2018 at 11:27, Rajendra Nayak <rnayak@codeaurora.org> wrote:
>>
>>
>> On 05/23/2018 02:37 PM, Jon Hunter wrote:
>>>
>>> On 23/05/18 07:12, Ulf Hansson wrote:
>>>
>>> ...
>>>
>>>>>>>> Thanks for sending this. Believe it or not this has still been on my to-do list
>>>>>>>> and so we definitely need a solution for Tegra.
>>>>>>>>
>>>>>>>> Looking at the above it appears that additional power-domains exposed as devices
>>>>>>>> to the client device. So I assume that this means that the drivers for devices
>>>>>>>> with multiple power-domains will need to call RPM APIs for each of these
>>>>>>>> additional power-domains. Is that correct?
>>>>>>>
>>>>>>> They can, but should not!
>>>>>>>
>>>>>>> Instead, the driver shall use device_link_add() and device_link_del(),
>>>>>>> dynamically, depending on what PM domain that their original device
>>>>>>> needs for the current running use case.
>>>>>>>
>>>>>>> In that way, they keep existing runtime PM deployment, operating on
>>>>>>> its original device.
>>>>>>
>>>>>> OK, sounds good. Any reason why the linking cannot be handled by the above API? Is there a use-case where you would not want it linked?
>>>>>
>>>>> I am guessing the linking is what would give the driver the ability to decide which subset of powerdomains it actually wants to control
>>>>> at any point using runtime PM. If we have cases wherein the driver would want to turn on/off _all_ its associated powerdomains _always_
>>>>> then a default linking of all would help.
>>>>
>>>> First, I think we need to decide on *where* the linking should be
>>>> done, not at both places, as that would just mess up synchronization
>>>> of who is responsible for calling the device_link_del() at detach.
>>>>
>>>> Second, It would in principle be fine to call device_link_add() and
>>>> device_link_del() as a part of the attach/detach APIs. However, there
>>>> is a downside to such solution, which would be that the driver then
>>>> needs call the detach API, just to do device_link_del(). Of course
>>>> then it would also needs to call the attach API later if/when needed.
>>>> Doing this adds unnecessary overhead - comparing to just let the
>>>> driver call device_link_add|del() when needed. On the upside, yes, it
>>>> would put less burden on the drivers as it then only needs to care
>>>> about using one set of functions.
>>>>
>>>> Which solution do you prefer?
>>>
>>> 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.
>>>
>>> Rajendra, do you have a use-case right now where the driver would want to handle the linking?
>>
>> So if I understand this right, any driver which does want to control individual powerdomain state would
>> need to do the linking itself right?
>>
>> What I am saying is, if I have device A, with powerdomains X and Y, and if I want to turn on only X,
>> then I would want only X to be linked with A, and at a later point if I want both X and Y to be turned on,
>> I would then go ahead and link both X and Y to A? Is that correct or did I get it all wrong?
>
> Correct!
>
>>
>> I know atleast Camera on msm8996 would need to do this since it has 2 vfe powerdoamins, which can be
>> turned on one at a time (depending on what resolution needs to be supported) or both together if we
>> really need very high resolution using both vfe modules.
>
> I think this is also the case for the Tegra XUSB subsystem.
>
> The usb device is always attached to one PM domain, but depending on
> if super-speed mode is used, another PM domain for that logic needs to
> be powered on as well.
>
> Jon, please correct me if I am wrong!
Yes this is technically correct, however, in reality I think we are
always going to enable the superspeed domain if either the host or
device domain is enabled. So we would probably always link the
superspeed with the host and device devices.
Cheers
Jon
--
nvpublic
^ permalink raw reply
* Re: [PATCH v2] schedutil: Allow cpufreq requests to be made even when kthread kicked
From: Joel Fernandes @ 2018-05-23 9:42 UTC (permalink / raw)
To: Viresh Kumar
Cc: Joel Fernandes (Google.), linux-kernel, Rafael J . Wysocki,
Peter Zijlstra, Ingo Molnar, Patrick Bellasi, Juri Lelli,
Luca Abeni, Todd Kjos, claudio, kernel-team, linux-pm
In-Reply-To: <20180523090101.6xifikvjpirqafox@vireshk-i7>
On May 23, 2018 2:01:01 AM PDT, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>On 22-05-18, 15:09, Joel Fernandes wrote:
>> I agree with the race you describe for single policy slow-switch.
>Good find :)
>>
>> The mainline sugov_work could also do such reordering in sugov_work,
>I think. Even
>> with the mutex_unlock in mainline's sugov_work, that work_in_progress
>write could
>> be reordered by the CPU to happen before the read of next_freq. AIUI,
>> mutex_unlock is expected to be only a release-barrier.
>>
>> Although to be safe, I could just put an smp_mb() there. I believe
>with that,
>> no locking would be needed for such case.
>>
>> I'll send out a v3 with Acks for the original patch, and the send out
>the
>> smp_mb() as a separate patch if that's Ok.
>
>Maybe it would be better to get the fix (with smp_mb) first and then
>this optimization patch on the top? That would mean that the fix can
>get applied to stable kernels easily.
Probably. But then Rafael is changing single policy to use the lock so then barrier wouldn't be needed at all. In that case, both mine and Rafael new patch can go into stable which handles your race ( optimization == fix in this case :P )
thanks,
- Joel
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
^ permalink raw reply
* Re: [PATCH v3] schedutil: Allow cpufreq requests to be made even when kthread kicked
From: Rafael J. Wysocki @ 2018-05-23 9:40 UTC (permalink / raw)
To: Joel Fernandes
Cc: linux-kernel, Joel Fernandes (Google), Viresh Kumar,
Rafael J . Wysocki, Peter Zijlstra, Ingo Molnar, Patrick Bellasi,
Juri Lelli, Luca Abeni, Todd Kjos, claudio, kernel-team, linux-pm
In-Reply-To: <20180522225553.69483-1-joel@joelfernandes.org>
On Wednesday, May 23, 2018 12:55:53 AM CEST Joel Fernandes wrote:
> From: "Joel Fernandes (Google)" <joel@joelfernandes.org>
>
> Currently there is a chance of a schedutil cpufreq update request to be
> dropped if there is a pending update request. This pending request can
> be delayed if there is a scheduling delay of the irq_work and the wake
> up of the schedutil governor kthread.
>
> A very bad scenario is when a schedutil request was already just made,
> such as to reduce the CPU frequency, then a newer request to increase
> CPU frequency (even sched deadline urgent frequency increase requests)
> can be dropped, even though the rate limits suggest that its Ok to
> process a request. This is because of the way the work_in_progress flag
> is used.
>
> This patch improves the situation by allowing new requests to happen
> even though the old one is still being processed. Note that in this
> approach, if an irq_work was already issued, we just update next_freq
> and don't bother to queue another request so there's no extra work being
> done to make this happen.
>
> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
> Acked-by: Juri Lelli <juri.lelli@redhat.com>
> CC: Viresh Kumar <viresh.kumar@linaro.org>
> CC: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> CC: Peter Zijlstra <peterz@infradead.org>
> CC: Ingo Molnar <mingo@redhat.com>
> CC: Patrick Bellasi <patrick.bellasi@arm.com>
> CC: Juri Lelli <juri.lelli@redhat.com>
> Cc: Luca Abeni <luca.abeni@santannapisa.it>
> CC: Todd Kjos <tkjos@google.com>
> CC: claudio@evidence.eu.com
> CC: kernel-team@android.com
> CC: linux-pm@vger.kernel.org
> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> ---
> Only commit log update, no code change in v2->v3
>
> kernel/sched/cpufreq_schedutil.c | 34 ++++++++++++++++++++++++--------
> 1 file changed, 26 insertions(+), 8 deletions(-)
>
> diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
> index e13df951aca7..5c482ec38610 100644
> --- a/kernel/sched/cpufreq_schedutil.c
> +++ b/kernel/sched/cpufreq_schedutil.c
> @@ -92,9 +92,6 @@ static bool sugov_should_update_freq(struct sugov_policy *sg_policy, u64 time)
> !cpufreq_can_do_remote_dvfs(sg_policy->policy))
> return false;
>
> - if (sg_policy->work_in_progress)
> - return false;
> -
> if (unlikely(sg_policy->need_freq_update)) {
> sg_policy->need_freq_update = false;
> /*
> @@ -128,7 +125,7 @@ static void sugov_update_commit(struct sugov_policy *sg_policy, u64 time,
>
> policy->cur = next_freq;
> trace_cpu_frequency(next_freq, smp_processor_id());
> - } else {
> + } else if (!sg_policy->work_in_progress) {
> sg_policy->work_in_progress = true;
> irq_work_queue(&sg_policy->irq_work);
> }
> @@ -291,6 +288,13 @@ static void sugov_update_single(struct update_util_data *hook, u64 time,
>
> ignore_dl_rate_limit(sg_cpu, sg_policy);
>
> + /*
> + * For slow-switch systems, single policy requests can't run at the
> + * moment if update is in progress, unless we acquire update_lock.
> + */
> + if (sg_policy->work_in_progress)
> + return;
> +
> if (!sugov_should_update_freq(sg_policy, time))
> return;
>
> @@ -382,13 +386,27 @@ sugov_update_shared(struct update_util_data *hook, u64 time, unsigned int flags)
> static void sugov_work(struct kthread_work *work)
> {
> struct sugov_policy *sg_policy = container_of(work, struct sugov_policy, work);
> + unsigned int freq;
> + unsigned long flags;
> +
> + /*
> + * Hold sg_policy->update_lock shortly to handle the case where:
> + * incase sg_policy->next_freq is read here, and then updated by
> + * sugov_update_shared just before work_in_progress is set to false
> + * here, we may miss queueing the new update.
> + *
> + * Note: If a work was queued after the update_lock is released,
> + * sugov_work will just be called again by kthread_work code; and the
> + * request will be proceed before the sugov thread sleeps.
> + */
> + raw_spin_lock_irqsave(&sg_policy->update_lock, flags);
> + freq = sg_policy->next_freq;
> + sg_policy->work_in_progress = false;
> + raw_spin_unlock_irqrestore(&sg_policy->update_lock, flags);
>
> mutex_lock(&sg_policy->work_lock);
> - __cpufreq_driver_target(sg_policy->policy, sg_policy->next_freq,
> - CPUFREQ_RELATION_L);
> + __cpufreq_driver_target(sg_policy->policy, freq, CPUFREQ_RELATION_L);
> mutex_unlock(&sg_policy->work_lock);
> -
> - sg_policy->work_in_progress = false;
> }
>
> static void sugov_irq_work(struct irq_work *irq_work)
>
Applied, thanks!
^ permalink raw reply
* [PATCH v10 2/2] dt-bindings: cpufreq: Document operating-points-v2-kryo-cpu
From: Ilia Lin @ 2018-05-23 9:40 UTC (permalink / raw)
To: vireshk, nm, sboyd, robh, mark.rutland, rjw, linux-pm, devicetree,
linux-kernel
In-Reply-To: <1527068454-28921-1-git-send-email-ilialin@codeaurora.org>
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.
This change adds documentation for the DT bindings.
The "operating-points-v2-kryo-cpu" DT extends the "operating-points-v2"
with following parameters:
- nvmem-cells (NVMEM area containig the speedbin information)
- opp-supported-hw: A single 32 bit bitmap value,
representing compatible HW:
0: MSM8996 V3, speedbin 0
1: MSM8996 V3, speedbin 1
2: MSM8996 V3, speedbin 2
3: unused
4: MSM8996 SG, speedbin 0
5: MSM8996 SG, speedbin 1
6: MSM8996 SG, speedbin 2
7-31: unused
Signed-off-by: Ilia Lin <ilialin@codeaurora.org>
Reviewed-by: Rob Herring <robh@kernel.org>
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
---
.../devicetree/bindings/opp/kryo-cpufreq.txt | 680 +++++++++++++++++++++
1 file changed, 680 insertions(+)
create mode 100644 Documentation/devicetree/bindings/opp/kryo-cpufreq.txt
diff --git a/Documentation/devicetree/bindings/opp/kryo-cpufreq.txt b/Documentation/devicetree/bindings/opp/kryo-cpufreq.txt
new file mode 100644
index 0000000..c2127b9
--- /dev/null
+++ b/Documentation/devicetree/bindings/opp/kryo-cpufreq.txt
@@ -0,0 +1,680 @@
+Qualcomm Technologies, Inc. KRYO CPUFreq and OPP bindings
+===================================
+
+In Certain Qualcomm Technologies, Inc. SoCs like apq8096 and msm8996
+that have KRYO processors, the CPU ferequencies subset and voltage value
+of each OPP varies based on the silicon variant in use.
+Qualcomm Technologies, Inc. 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 (existing HW bitmap).
+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.
+
+Required properties:
+--------------------
+In 'cpus' nodes:
+- operating-points-v2: Phandle to the operating-points-v2 table to use.
+
+In 'operating-points-v2' table:
+- compatible: Should be
+ - 'operating-points-v2-kryo-cpu' for apq8096 and msm8996.
+- nvmem-cells: A phandle pointing to a nvmem-cells node representing the
+ efuse registers that has information about the
+ speedbin that is used to select the right frequency/voltage
+ value pair.
+ Please refer the for nvmem-cells
+ bindings Documentation/devicetree/bindings/nvmem/nvmem.txt
+ and also examples below.
+
+In every OPP node:
+- opp-supported-hw: A single 32 bit bitmap value, representing compatible HW.
+ Bitmap:
+ 0: MSM8996 V3, speedbin 0
+ 1: MSM8996 V3, speedbin 1
+ 2: MSM8996 V3, speedbin 2
+ 3: unused
+ 4: MSM8996 SG, speedbin 0
+ 5: MSM8996 SG, speedbin 1
+ 6: MSM8996 SG, speedbin 2
+ 7-31: unused
+
+Example 1:
+---------
+
+ cpus {
+ #address-cells = <2>;
+ #size-cells = <0>;
+
+ CPU0: cpu@0 {
+ device_type = "cpu";
+ compatible = "qcom,kryo";
+ reg = <0x0 0x0>;
+ enable-method = "psci";
+ clocks = <&kryocc 0>;
+ cpu-supply = <&pm8994_s11_saw>;
+ operating-points-v2 = <&cluster0_opp>;
+ #cooling-cells = <2>;
+ next-level-cache = <&L2_0>;
+ L2_0: l2-cache {
+ compatible = "cache";
+ cache-level = <2>;
+ };
+ };
+
+ CPU1: cpu@1 {
+ device_type = "cpu";
+ compatible = "qcom,kryo";
+ reg = <0x0 0x1>;
+ enable-method = "psci";
+ clocks = <&kryocc 0>;
+ cpu-supply = <&pm8994_s11_saw>;
+ operating-points-v2 = <&cluster0_opp>;
+ #cooling-cells = <2>;
+ next-level-cache = <&L2_0>;
+ };
+
+ CPU2: cpu@100 {
+ device_type = "cpu";
+ compatible = "qcom,kryo";
+ reg = <0x0 0x100>;
+ enable-method = "psci";
+ clocks = <&kryocc 1>;
+ cpu-supply = <&pm8994_s11_saw>;
+ operating-points-v2 = <&cluster1_opp>;
+ #cooling-cells = <2>;
+ next-level-cache = <&L2_1>;
+ L2_1: l2-cache {
+ compatible = "cache";
+ cache-level = <2>;
+ };
+ };
+
+ CPU3: cpu@101 {
+ device_type = "cpu";
+ compatible = "qcom,kryo";
+ reg = <0x0 0x101>;
+ enable-method = "psci";
+ clocks = <&kryocc 1>;
+ cpu-supply = <&pm8994_s11_saw>;
+ operating-points-v2 = <&cluster1_opp>;
+ #cooling-cells = <2>;
+ next-level-cache = <&L2_1>;
+ };
+
+ cpu-map {
+ cluster0 {
+ core0 {
+ cpu = <&CPU0>;
+ };
+
+ core1 {
+ cpu = <&CPU1>;
+ };
+ };
+
+ cluster1 {
+ core0 {
+ cpu = <&CPU2>;
+ };
+
+ core1 {
+ cpu = <&CPU3>;
+ };
+ };
+ };
+ };
+
+ cluster0_opp: opp_table0 {
+ compatible = "operating-points-v2-kryo-cpu";
+ nvmem-cells = <&speedbin_efuse>;
+ opp-shared;
+
+ opp-307200000 {
+ opp-hz = /bits/ 64 <307200000>;
+ opp-microvolt = <905000 905000 1140000>;
+ opp-supported-hw = <0x77>;
+ clock-latency-ns = <200000>;
+ };
+ opp-384000000 {
+ opp-hz = /bits/ 64 <384000000>;
+ opp-microvolt = <905000 905000 1140000>;
+ opp-supported-hw = <0x70>;
+ clock-latency-ns = <200000>;
+ };
+ opp-422400000 {
+ opp-hz = /bits/ 64 <422400000>;
+ opp-microvolt = <905000 905000 1140000>;
+ opp-supported-hw = <0x7>;
+ clock-latency-ns = <200000>;
+ };
+ opp-460800000 {
+ opp-hz = /bits/ 64 <460800000>;
+ opp-microvolt = <905000 905000 1140000>;
+ opp-supported-hw = <0x70>;
+ clock-latency-ns = <200000>;
+ };
+ opp-480000000 {
+ opp-hz = /bits/ 64 <480000000>;
+ opp-microvolt = <905000 905000 1140000>;
+ opp-supported-hw = <0x7>;
+ clock-latency-ns = <200000>;
+ };
+ opp-537600000 {
+ opp-hz = /bits/ 64 <537600000>;
+ opp-microvolt = <905000 905000 1140000>;
+ opp-supported-hw = <0x70>;
+ clock-latency-ns = <200000>;
+ };
+ opp-556800000 {
+ opp-hz = /bits/ 64 <556800000>;
+ opp-microvolt = <905000 905000 1140000>;
+ opp-supported-hw = <0x7>;
+ clock-latency-ns = <200000>;
+ };
+ opp-614400000 {
+ opp-hz = /bits/ 64 <614400000>;
+ opp-microvolt = <905000 905000 1140000>;
+ opp-supported-hw = <0x70>;
+ clock-latency-ns = <200000>;
+ };
+ opp-652800000 {
+ opp-hz = /bits/ 64 <652800000>;
+ opp-microvolt = <905000 905000 1140000>;
+ opp-supported-hw = <0x7>;
+ clock-latency-ns = <200000>;
+ };
+ opp-691200000 {
+ opp-hz = /bits/ 64 <691200000>;
+ opp-microvolt = <905000 905000 1140000>;
+ opp-supported-hw = <0x70>;
+ clock-latency-ns = <200000>;
+ };
+ opp-729600000 {
+ opp-hz = /bits/ 64 <729600000>;
+ opp-microvolt = <905000 905000 1140000>;
+ opp-supported-hw = <0x7>;
+ clock-latency-ns = <200000>;
+ };
+ opp-768000000 {
+ opp-hz = /bits/ 64 <768000000>;
+ opp-microvolt = <905000 905000 1140000>;
+ opp-supported-hw = <0x70>;
+ clock-latency-ns = <200000>;
+ };
+ opp-844800000 {
+ opp-hz = /bits/ 64 <844800000>;
+ opp-microvolt = <905000 905000 1140000>;
+ opp-supported-hw = <0x77>;
+ clock-latency-ns = <200000>;
+ };
+ opp-902400000 {
+ opp-hz = /bits/ 64 <902400000>;
+ opp-microvolt = <905000 905000 1140000>;
+ opp-supported-hw = <0x70>;
+ clock-latency-ns = <200000>;
+ };
+ opp-960000000 {
+ opp-hz = /bits/ 64 <960000000>;
+ opp-microvolt = <905000 905000 1140000>;
+ opp-supported-hw = <0x7>;
+ clock-latency-ns = <200000>;
+ };
+ opp-979200000 {
+ opp-hz = /bits/ 64 <979200000>;
+ opp-microvolt = <905000 905000 1140000>;
+ opp-supported-hw = <0x70>;
+ clock-latency-ns = <200000>;
+ };
+ opp-1036800000 {
+ opp-hz = /bits/ 64 <1036800000>;
+ opp-microvolt = <905000 905000 1140000>;
+ opp-supported-hw = <0x7>;
+ clock-latency-ns = <200000>;
+ };
+ opp-1056000000 {
+ opp-hz = /bits/ 64 <1056000000>;
+ opp-microvolt = <905000 905000 1140000>;
+ opp-supported-hw = <0x70>;
+ clock-latency-ns = <200000>;
+ };
+ opp-1113600000 {
+ opp-hz = /bits/ 64 <1113600000>;
+ opp-microvolt = <905000 905000 1140000>;
+ opp-supported-hw = <0x7>;
+ clock-latency-ns = <200000>;
+ };
+ opp-1132800000 {
+ opp-hz = /bits/ 64 <1132800000>;
+ opp-microvolt = <905000 905000 1140000>;
+ opp-supported-hw = <0x70>;
+ clock-latency-ns = <200000>;
+ };
+ opp-1190400000 {
+ opp-hz = /bits/ 64 <1190400000>;
+ opp-microvolt = <905000 905000 1140000>;
+ opp-supported-hw = <0x7>;
+ clock-latency-ns = <200000>;
+ };
+ opp-1209600000 {
+ opp-hz = /bits/ 64 <1209600000>;
+ opp-microvolt = <905000 905000 1140000>;
+ opp-supported-hw = <0x70>;
+ clock-latency-ns = <200000>;
+ };
+ opp-1228800000 {
+ opp-hz = /bits/ 64 <1228800000>;
+ opp-microvolt = <905000 905000 1140000>;
+ opp-supported-hw = <0x7>;
+ clock-latency-ns = <200000>;
+ };
+ opp-1286400000 {
+ opp-hz = /bits/ 64 <1286400000>;
+ opp-microvolt = <1140000 905000 1140000>;
+ opp-supported-hw = <0x70>;
+ clock-latency-ns = <200000>;
+ };
+ opp-1324800000 {
+ opp-hz = /bits/ 64 <1324800000>;
+ opp-microvolt = <1140000 905000 1140000>;
+ opp-supported-hw = <0x5>;
+ clock-latency-ns = <200000>;
+ };
+ opp-1363200000 {
+ opp-hz = /bits/ 64 <1363200000>;
+ opp-microvolt = <1140000 905000 1140000>;
+ opp-supported-hw = <0x72>;
+ clock-latency-ns = <200000>;
+ };
+ opp-1401600000 {
+ opp-hz = /bits/ 64 <1401600000>;
+ opp-microvolt = <1140000 905000 1140000>;
+ opp-supported-hw = <0x5>;
+ clock-latency-ns = <200000>;
+ };
+ opp-1440000000 {
+ opp-hz = /bits/ 64 <1440000000>;
+ opp-microvolt = <1140000 905000 1140000>;
+ opp-supported-hw = <0x70>;
+ clock-latency-ns = <200000>;
+ };
+ opp-1478400000 {
+ opp-hz = /bits/ 64 <1478400000>;
+ opp-microvolt = <1140000 905000 1140000>;
+ opp-supported-hw = <0x1>;
+ clock-latency-ns = <200000>;
+ };
+ opp-1497600000 {
+ opp-hz = /bits/ 64 <1497600000>;
+ opp-microvolt = <1140000 905000 1140000>;
+ opp-supported-hw = <0x4>;
+ clock-latency-ns = <200000>;
+ };
+ opp-1516800000 {
+ opp-hz = /bits/ 64 <1516800000>;
+ opp-microvolt = <1140000 905000 1140000>;
+ opp-supported-hw = <0x70>;
+ clock-latency-ns = <200000>;
+ };
+ opp-1593600000 {
+ opp-hz = /bits/ 64 <1593600000>;
+ opp-microvolt = <1140000 905000 1140000>;
+ opp-supported-hw = <0x71>;
+ clock-latency-ns = <200000>;
+ };
+ opp-1996800000 {
+ opp-hz = /bits/ 64 <1996800000>;
+ opp-microvolt = <1140000 905000 1140000>;
+ opp-supported-hw = <0x20>;
+ clock-latency-ns = <200000>;
+ };
+ opp-2188800000 {
+ opp-hz = /bits/ 64 <2188800000>;
+ opp-microvolt = <1140000 905000 1140000>;
+ opp-supported-hw = <0x10>;
+ clock-latency-ns = <200000>;
+ };
+ };
+
+ cluster1_opp: opp_table1 {
+ compatible = "operating-points-v2-kryo-cpu";
+ nvmem-cells = <&speedbin_efuse>;
+ opp-shared;
+
+ opp-307200000 {
+ opp-hz = /bits/ 64 <307200000>;
+ opp-microvolt = <905000 905000 1140000>;
+ opp-supported-hw = <0x77>;
+ clock-latency-ns = <200000>;
+ };
+ opp-384000000 {
+ opp-hz = /bits/ 64 <384000000>;
+ opp-microvolt = <905000 905000 1140000>;
+ opp-supported-hw = <0x70>;
+ clock-latency-ns = <200000>;
+ };
+ opp-403200000 {
+ opp-hz = /bits/ 64 <403200000>;
+ opp-microvolt = <905000 905000 1140000>;
+ opp-supported-hw = <0x7>;
+ clock-latency-ns = <200000>;
+ };
+ opp-460800000 {
+ opp-hz = /bits/ 64 <460800000>;
+ opp-microvolt = <905000 905000 1140000>;
+ opp-supported-hw = <0x70>;
+ clock-latency-ns = <200000>;
+ };
+ opp-480000000 {
+ opp-hz = /bits/ 64 <480000000>;
+ opp-microvolt = <905000 905000 1140000>;
+ opp-supported-hw = <0x7>;
+ clock-latency-ns = <200000>;
+ };
+ opp-537600000 {
+ opp-hz = /bits/ 64 <537600000>;
+ opp-microvolt = <905000 905000 1140000>;
+ opp-supported-hw = <0x70>;
+ clock-latency-ns = <200000>;
+ };
+ opp-556800000 {
+ opp-hz = /bits/ 64 <556800000>;
+ opp-microvolt = <905000 905000 1140000>;
+ opp-supported-hw = <0x7>;
+ clock-latency-ns = <200000>;
+ };
+ opp-614400000 {
+ opp-hz = /bits/ 64 <614400000>;
+ opp-microvolt = <905000 905000 1140000>;
+ opp-supported-hw = <0x70>;
+ clock-latency-ns = <200000>;
+ };
+ opp-652800000 {
+ opp-hz = /bits/ 64 <652800000>;
+ opp-microvolt = <905000 905000 1140000>;
+ opp-supported-hw = <0x7>;
+ clock-latency-ns = <200000>;
+ };
+ opp-691200000 {
+ opp-hz = /bits/ 64 <691200000>;
+ opp-microvolt = <905000 905000 1140000>;
+ opp-supported-hw = <0x70>;
+ clock-latency-ns = <200000>;
+ };
+ opp-729600000 {
+ opp-hz = /bits/ 64 <729600000>;
+ opp-microvolt = <905000 905000 1140000>;
+ opp-supported-hw = <0x7>;
+ clock-latency-ns = <200000>;
+ };
+ opp-748800000 {
+ opp-hz = /bits/ 64 <748800000>;
+ opp-microvolt = <905000 905000 1140000>;
+ opp-supported-hw = <0x70>;
+ clock-latency-ns = <200000>;
+ };
+ opp-806400000 {
+ opp-hz = /bits/ 64 <806400000>;
+ opp-microvolt = <905000 905000 1140000>;
+ opp-supported-hw = <0x7>;
+ clock-latency-ns = <200000>;
+ };
+ opp-825600000 {
+ opp-hz = /bits/ 64 <825600000>;
+ opp-microvolt = <905000 905000 1140000>;
+ opp-supported-hw = <0x70>;
+ clock-latency-ns = <200000>;
+ };
+ opp-883200000 {
+ opp-hz = /bits/ 64 <883200000>;
+ opp-microvolt = <905000 905000 1140000>;
+ opp-supported-hw = <0x7>;
+ clock-latency-ns = <200000>;
+ };
+ opp-902400000 {
+ opp-hz = /bits/ 64 <902400000>;
+ opp-microvolt = <905000 905000 1140000>;
+ opp-supported-hw = <0x70>;
+ clock-latency-ns = <200000>;
+ };
+ opp-940800000 {
+ opp-hz = /bits/ 64 <940800000>;
+ opp-microvolt = <905000 905000 1140000>;
+ opp-supported-hw = <0x7>;
+ clock-latency-ns = <200000>;
+ };
+ opp-979200000 {
+ opp-hz = /bits/ 64 <979200000>;
+ opp-microvolt = <905000 905000 1140000>;
+ opp-supported-hw = <0x70>;
+ clock-latency-ns = <200000>;
+ };
+ opp-1036800000 {
+ opp-hz = /bits/ 64 <1036800000>;
+ opp-microvolt = <905000 905000 1140000>;
+ opp-supported-hw = <0x7>;
+ clock-latency-ns = <200000>;
+ };
+ opp-1056000000 {
+ opp-hz = /bits/ 64 <1056000000>;
+ opp-microvolt = <905000 905000 1140000>;
+ opp-supported-hw = <0x70>;
+ clock-latency-ns = <200000>;
+ };
+ opp-1113600000 {
+ opp-hz = /bits/ 64 <1113600000>;
+ opp-microvolt = <905000 905000 1140000>;
+ opp-supported-hw = <0x7>;
+ clock-latency-ns = <200000>;
+ };
+ opp-1132800000 {
+ opp-hz = /bits/ 64 <1132800000>;
+ opp-microvolt = <905000 905000 1140000>;
+ opp-supported-hw = <0x70>;
+ clock-latency-ns = <200000>;
+ };
+ opp-1190400000 {
+ opp-hz = /bits/ 64 <1190400000>;
+ opp-microvolt = <905000 905000 1140000>;
+ opp-supported-hw = <0x7>;
+ clock-latency-ns = <200000>;
+ };
+ opp-1209600000 {
+ opp-hz = /bits/ 64 <1209600000>;
+ opp-microvolt = <905000 905000 1140000>;
+ opp-supported-hw = <0x70>;
+ clock-latency-ns = <200000>;
+ };
+ opp-1248000000 {
+ opp-hz = /bits/ 64 <1248000000>;
+ opp-microvolt = <905000 905000 1140000>;
+ opp-supported-hw = <0x7>;
+ clock-latency-ns = <200000>;
+ };
+ opp-1286400000 {
+ opp-hz = /bits/ 64 <1286400000>;
+ opp-microvolt = <905000 905000 1140000>;
+ opp-supported-hw = <0x70>;
+ clock-latency-ns = <200000>;
+ };
+ opp-1324800000 {
+ opp-hz = /bits/ 64 <1324800000>;
+ opp-microvolt = <1140000 905000 1140000>;
+ opp-supported-hw = <0x7>;
+ clock-latency-ns = <200000>;
+ };
+ opp-1363200000 {
+ opp-hz = /bits/ 64 <1363200000>;
+ opp-microvolt = <1140000 905000 1140000>;
+ opp-supported-hw = <0x70>;
+ clock-latency-ns = <200000>;
+ };
+ opp-1401600000 {
+ opp-hz = /bits/ 64 <1401600000>;
+ opp-microvolt = <1140000 905000 1140000>;
+ opp-supported-hw = <0x7>;
+ clock-latency-ns = <200000>;
+ };
+ opp-1440000000 {
+ opp-hz = /bits/ 64 <1440000000>;
+ opp-microvolt = <1140000 905000 1140000>;
+ opp-supported-hw = <0x70>;
+ clock-latency-ns = <200000>;
+ };
+ opp-1478400000 {
+ opp-hz = /bits/ 64 <1478400000>;
+ opp-microvolt = <1140000 905000 1140000>;
+ opp-supported-hw = <0x7>;
+ clock-latency-ns = <200000>;
+ };
+ opp-1516800000 {
+ opp-hz = /bits/ 64 <1516800000>;
+ opp-microvolt = <1140000 905000 1140000>;
+ opp-supported-hw = <0x70>;
+ clock-latency-ns = <200000>;
+ };
+ opp-1555200000 {
+ opp-hz = /bits/ 64 <1555200000>;
+ opp-microvolt = <1140000 905000 1140000>;
+ opp-supported-hw = <0x7>;
+ clock-latency-ns = <200000>;
+ };
+ opp-1593600000 {
+ opp-hz = /bits/ 64 <1593600000>;
+ opp-microvolt = <1140000 905000 1140000>;
+ opp-supported-hw = <0x70>;
+ clock-latency-ns = <200000>;
+ };
+ opp-1632000000 {
+ opp-hz = /bits/ 64 <1632000000>;
+ opp-microvolt = <1140000 905000 1140000>;
+ opp-supported-hw = <0x7>;
+ clock-latency-ns = <200000>;
+ };
+ opp-1670400000 {
+ opp-hz = /bits/ 64 <1670400000>;
+ opp-microvolt = <1140000 905000 1140000>;
+ opp-supported-hw = <0x70>;
+ clock-latency-ns = <200000>;
+ };
+ opp-1708800000 {
+ opp-hz = /bits/ 64 <1708800000>;
+ opp-microvolt = <1140000 905000 1140000>;
+ opp-supported-hw = <0x7>;
+ clock-latency-ns = <200000>;
+ };
+ opp-1747200000 {
+ opp-hz = /bits/ 64 <1747200000>;
+ opp-microvolt = <1140000 905000 1140000>;
+ opp-supported-hw = <0x70>;
+ clock-latency-ns = <200000>;
+ };
+ opp-1785600000 {
+ opp-hz = /bits/ 64 <1785600000>;
+ opp-microvolt = <1140000 905000 1140000>;
+ opp-supported-hw = <0x7>;
+ clock-latency-ns = <200000>;
+ };
+ opp-1804800000 {
+ opp-hz = /bits/ 64 <1804800000>;
+ opp-microvolt = <1140000 905000 1140000>;
+ opp-supported-hw = <0x6>;
+ clock-latency-ns = <200000>;
+ };
+ opp-1824000000 {
+ opp-hz = /bits/ 64 <1824000000>;
+ opp-microvolt = <1140000 905000 1140000>;
+ opp-supported-hw = <0x71>;
+ clock-latency-ns = <200000>;
+ };
+ opp-1900800000 {
+ opp-hz = /bits/ 64 <1900800000>;
+ opp-microvolt = <1140000 905000 1140000>;
+ opp-supported-hw = <0x74>;
+ clock-latency-ns = <200000>;
+ };
+ opp-1920000000 {
+ opp-hz = /bits/ 64 <1920000000>;
+ opp-microvolt = <1140000 905000 1140000>;
+ opp-supported-hw = <0x1>;
+ clock-latency-ns = <200000>;
+ };
+ opp-1977600000 {
+ opp-hz = /bits/ 64 <1977600000>;
+ opp-microvolt = <1140000 905000 1140000>;
+ opp-supported-hw = <0x30>;
+ clock-latency-ns = <200000>;
+ };
+ opp-1996800000 {
+ opp-hz = /bits/ 64 <1996800000>;
+ opp-microvolt = <1140000 905000 1140000>;
+ opp-supported-hw = <0x1>;
+ clock-latency-ns = <200000>;
+ };
+ opp-2054400000 {
+ opp-hz = /bits/ 64 <2054400000>;
+ opp-microvolt = <1140000 905000 1140000>;
+ opp-supported-hw = <0x30>;
+ clock-latency-ns = <200000>;
+ };
+ opp-2073600000 {
+ opp-hz = /bits/ 64 <2073600000>;
+ opp-microvolt = <1140000 905000 1140000>;
+ opp-supported-hw = <0x1>;
+ clock-latency-ns = <200000>;
+ };
+ opp-2150400000 {
+ opp-hz = /bits/ 64 <2150400000>;
+ opp-microvolt = <1140000 905000 1140000>;
+ opp-supported-hw = <0x31>;
+ clock-latency-ns = <200000>;
+ };
+ opp-2246400000 {
+ opp-hz = /bits/ 64 <2246400000>;
+ opp-microvolt = <1140000 905000 1140000>;
+ opp-supported-hw = <0x10>;
+ clock-latency-ns = <200000>;
+ };
+ opp-2342400000 {
+ opp-hz = /bits/ 64 <2342400000>;
+ opp-microvolt = <1140000 905000 1140000>;
+ opp-supported-hw = <0x10>;
+ clock-latency-ns = <200000>;
+ };
+ };
+
+....
+
+reserved-memory {
+ #address-cells = <2>;
+ #size-cells = <2>;
+ ranges;
+....
+ smem_mem: smem-mem@86000000 {
+ reg = <0x0 0x86000000 0x0 0x200000>;
+ no-map;
+ };
+....
+};
+
+smem {
+ compatible = "qcom,smem";
+ memory-region = <&smem_mem>;
+ hwlocks = <&tcsr_mutex 3>;
+};
+
+soc {
+....
+ qfprom: qfprom@74000 {
+ compatible = "qcom,qfprom";
+ reg = <0x00074000 0x8ff>;
+ #address-cells = <1>;
+ #size-cells = <1>;
+ ....
+ speedbin_efuse: speedbin@133 {
+ reg = <0x133 0x1>;
+ bits = <5 3>;
+ };
+ };
+};
--
1.9.1
^ permalink raw reply related
* [PATCH v10 1/2] cpufreq: Add Kryo CPU scaling driver
From: Ilia Lin @ 2018-05-23 9:40 UTC (permalink / raw)
To: vireshk, nm, sboyd, robh, mark.rutland, rjw, linux-pm, devicetree,
linux-kernel
In-Reply-To: <1527068454-28921-1-git-send-email-ilialin@codeaurora.org>
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 | 163 +++++++++++++++++++++++++++++++++++
4 files changed, 177 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..885051e
--- /dev/null
+++ b/drivers/cpufreq/qcom-cpufreq-kryo.c
@@ -0,0 +1,163 @@
+// 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 __init qcom_cpufreq_kryo_driver_init(void)
+{
+ struct opp_table *opp_tables[NR_CPUS] = {0};
+ enum _msm8996_version msm8996_version;
+ struct nvmem_cell *speedbin_nvmem;
+ struct platform_device *pdev;
+ 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")) {
+ ret = -ENOENT;
+ goto free_np;
+ }
+
+ speedbin_nvmem = of_nvmem_cell_get(np, NULL);
+ if (IS_ERR(speedbin_nvmem)) {
+ ret = PTR_ERR(speedbin_nvmem);
+ dev_err(cpu_dev, "Could not get nvmem cell: %d\n", ret);
+ goto free_np;
+ }
+
+ 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;
+ }
+ }
+
+ pdev = platform_device_register_simple("cpufreq-dt", -1, NULL, 0);
+ if (!IS_ERR(pdev))
+ return 0;
+
+ ret = PTR_ERR(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]);
+ }
+free_np:
+ of_node_put(np);
+
+ return ret;
+}
+late_initcall(qcom_cpufreq_kryo_driver_init);
+
+MODULE_DESCRIPTION("Qualcomm Technologies, Inc. Kryo CPUfreq driver");
+MODULE_LICENSE("GPL v2");
--
1.9.1
^ permalink raw reply related
* [PATCH v10 0/2] Kryo CPU scaling driver
From: Ilia Lin @ 2018-05-23 9:40 UTC (permalink / raw)
To: vireshk, nm, sboyd, robh, mark.rutland, rjw, linux-pm, devicetree,
linux-kernel
[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/10420751/
https://patchwork.kernel.org/patch/10414761/
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 | 163 +++++
5 files changed, 857 insertions(+)
create mode 100644 Documentation/devicetree/bindings/opp/kryo-cpufreq.txt
create mode 100644 drivers/cpufreq/qcom-cpufreq-kryo.c
--
1.9.1
^ permalink raw reply
* Re: [PATCH] cpufreq: Add Kryo CPU scaling driver
From: Russell King - ARM Linux @ 2018-05-23 9:40 UTC (permalink / raw)
To: Ilia Lin
Cc: viresh.kumar, rjw, sudeep.holla, linux-clk, devicetree,
linux-kernel, linux-pm, linux-arm-msm, linux-soc,
linux-arm-kernel
In-Reply-To: <1527066324-24726-1-git-send-email-ilialin@codeaurora.org>
On Wed, May 23, 2018 at 12:05:24PM +0300, Ilia Lin wrote:
> + np = dev_pm_opp_of_get_opp_desc_node(cpu_dev);
> + if (IS_ERR(np))
> + return PTR_ERR(np);
...
> +
> + pdev = platform_device_register_simple("cpufreq-dt", -1, NULL, 0);
> + if (!IS_ERR(pdev))
Do you need to hold a reference to `np' here?
> + return 0;
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up
^ permalink raw reply
* Re: [PATCH 8/9] PM / Domains: Add support for multi PM domains per device to genpd
From: Ulf Hansson @ 2018-05-23 9:33 UTC (permalink / raw)
To: Rajendra Nayak, Jon Hunter
Cc: 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: <00ba829d-faf4-168c-db00-531621b9280f@codeaurora.org>
On 23 May 2018 at 11:27, Rajendra Nayak <rnayak@codeaurora.org> wrote:
>
>
> On 05/23/2018 02:37 PM, Jon Hunter wrote:
>>
>> On 23/05/18 07:12, Ulf Hansson wrote:
>>
>> ...
>>
>>>>>>> Thanks for sending this. Believe it or not this has still been on my to-do list
>>>>>>> and so we definitely need a solution for Tegra.
>>>>>>>
>>>>>>> Looking at the above it appears that additional power-domains exposed as devices
>>>>>>> to the client device. So I assume that this means that the drivers for devices
>>>>>>> with multiple power-domains will need to call RPM APIs for each of these
>>>>>>> additional power-domains. Is that correct?
>>>>>>
>>>>>> They can, but should not!
>>>>>>
>>>>>> Instead, the driver shall use device_link_add() and device_link_del(),
>>>>>> dynamically, depending on what PM domain that their original device
>>>>>> needs for the current running use case.
>>>>>>
>>>>>> In that way, they keep existing runtime PM deployment, operating on
>>>>>> its original device.
>>>>>
>>>>> OK, sounds good. Any reason why the linking cannot be handled by the above API? Is there a use-case where you would not want it linked?
>>>>
>>>> I am guessing the linking is what would give the driver the ability to decide which subset of powerdomains it actually wants to control
>>>> at any point using runtime PM. If we have cases wherein the driver would want to turn on/off _all_ its associated powerdomains _always_
>>>> then a default linking of all would help.
>>>
>>> First, I think we need to decide on *where* the linking should be
>>> done, not at both places, as that would just mess up synchronization
>>> of who is responsible for calling the device_link_del() at detach.
>>>
>>> Second, It would in principle be fine to call device_link_add() and
>>> device_link_del() as a part of the attach/detach APIs. However, there
>>> is a downside to such solution, which would be that the driver then
>>> needs call the detach API, just to do device_link_del(). Of course
>>> then it would also needs to call the attach API later if/when needed.
>>> Doing this adds unnecessary overhead - comparing to just let the
>>> driver call device_link_add|del() when needed. On the upside, yes, it
>>> would put less burden on the drivers as it then only needs to care
>>> about using one set of functions.
>>>
>>> Which solution do you prefer?
>>
>> 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.
>>
>> Rajendra, do you have a use-case right now where the driver would want to handle the linking?
>
> So if I understand this right, any driver which does want to control individual powerdomain state would
> need to do the linking itself right?
>
> What I am saying is, if I have device A, with powerdomains X and Y, and if I want to turn on only X,
> then I would want only X to be linked with A, and at a later point if I want both X and Y to be turned on,
> I would then go ahead and link both X and Y to A? Is that correct or did I get it all wrong?
Correct!
>
> I know atleast Camera on msm8996 would need to do this since it has 2 vfe powerdoamins, which can be
> turned on one at a time (depending on what resolution needs to be supported) or both together if we
> really need very high resolution using both vfe modules.
I think this is also the case for the Tegra XUSB subsystem.
The usb device is always attached to one PM domain, but depending on
if super-speed mode is used, another PM domain for that logic needs to
be powered on as well.
Jon, please correct me if I am wrong!
Kind regards
Uffe
^ permalink raw reply
* Re: [PATCH] cpufreq: Add Kryo CPU scaling driver
From: Viresh Kumar @ 2018-05-23 9:32 UTC (permalink / raw)
To: Ilia Lin
Cc: rjw, sudeep.holla, linux, linux-clk, devicetree, linux-kernel,
linux-pm, linux-arm-msm, linux-soc, linux-arm-kernel
In-Reply-To: <1527066324-24726-1-git-send-email-ilialin@codeaurora.org>
On 23-05-18, 12:05, 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 | 163 +++++++++++++++++++++++++++++++++++
> 4 files changed, 177 insertions(+)
> create mode 100644 drivers/cpufreq/qcom-cpufreq-kryo.c
Hi Ilia,
So the patch looks good now. But I don't see how this series is going
to get merged. You are touching all parts of the kernel and no single
maintainer would be able to get these merged easily.
What I would suggest is that you divided the series in at least 3
parts.
- Patch 10 and 11, just the cpufreq stuff. So that Rafael can apply
those independently. Also mention the dependency on my OPP patches
in the cover letter for those two patches.
- All clk patches in another series, so that Stephen can apply them.
- And finally everything else to go via ARM-Soc.
Maybe things wouldn't be that simple, but that's what you would need
to do to get things merged. And please add all Acked by tags you got
to the patches.
For this patch:
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
--
viresh
^ permalink raw reply
* Re: [PATCH v1] cpufreq: tegra20: Fix imbalanced clock enable count
From: Dmitry Osipenko @ 2018-05-23 9:30 UTC (permalink / raw)
To: Viresh Kumar
Cc: Rafael J. Wysocki, Thierry Reding, Jonathan Hunter, linux-tegra,
linux-pm, linux-kernel
In-Reply-To: <20180523055817.subrv6oeckfecpcp@vireshk-i7>
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.
>> drivers/cpufreq/tegra20-cpufreq.c | 16 +++++++++++++++-
>> 1 file changed, 15 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/cpufreq/tegra20-cpufreq.c b/drivers/cpufreq/tegra20-cpufreq.c
>> index 05f57dcd5215..ca5229265b60 100644
>> --- a/drivers/cpufreq/tegra20-cpufreq.c
>> +++ b/drivers/cpufreq/tegra20-cpufreq.c
>> @@ -176,6 +176,14 @@ static int tegra20_cpufreq_probe(struct platform_device *pdev)
>> goto put_pll_x;
>> }
>>
>> + err = clk_prepare_enable(cpufreq->pll_x_clk);
>> + if (err)
>> + goto put_pll_p;
>> +
>> + err = clk_prepare_enable(cpufreq->pll_p_clk);
>> + if (err)
>> + goto disable_pll_x;
>> +
>> cpufreq->dev = &pdev->dev;
>> cpufreq->driver.get = cpufreq_generic_get;
>> cpufreq->driver.attr = cpufreq_generic_attr;
>> @@ -192,12 +200,16 @@ static int tegra20_cpufreq_probe(struct platform_device *pdev)
>>
>> err = cpufreq_register_driver(&cpufreq->driver);
>> if (err)
>> - goto put_pll_p;
>> + goto disable_pll_p;
>>
>> platform_set_drvdata(pdev, cpufreq);
>>
>> return 0;
>>
>> +disable_pll_p:
>> + clk_disable_unprepare(cpufreq->pll_p_clk);
>> +disable_pll_x:
>> + clk_disable_unprepare(cpufreq->pll_x_clk);
>> put_pll_p:
>> clk_put(cpufreq->pll_p_clk);
>> put_pll_x:
>> @@ -214,6 +226,8 @@ static int tegra20_cpufreq_remove(struct platform_device *pdev)
>>
>> cpufreq_unregister_driver(&cpufreq->driver);
>>
>> + clk_disable_unprepare(cpufreq->pll_p_clk);
>> + clk_disable_unprepare(cpufreq->pll_x_clk);
>> clk_put(cpufreq->pll_p_clk);
>> clk_put(cpufreq->pll_x_clk);
>> clk_put(cpufreq->cpu_clk);
>
> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
>
^ permalink raw reply
* Re: [PATCH 8/9] PM / Domains: Add support for multi PM domains per device to genpd
From: Rajendra Nayak @ 2018-05-23 9:27 UTC (permalink / raw)
To: Jon Hunter, Ulf Hansson
Cc: 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: <3838f17a-2ac8-bf3f-f0b1-f69bbe17629c@nvidia.com>
On 05/23/2018 02:37 PM, Jon Hunter wrote:
>
> On 23/05/18 07:12, Ulf Hansson wrote:
>
> ...
>
>>>>>> Thanks for sending this. Believe it or not this has still been on my to-do list
>>>>>> and so we definitely need a solution for Tegra.
>>>>>>
>>>>>> Looking at the above it appears that additional power-domains exposed as devices
>>>>>> to the client device. So I assume that this means that the drivers for devices
>>>>>> with multiple power-domains will need to call RPM APIs for each of these
>>>>>> additional power-domains. Is that correct?
>>>>>
>>>>> They can, but should not!
>>>>>
>>>>> Instead, the driver shall use device_link_add() and device_link_del(),
>>>>> dynamically, depending on what PM domain that their original device
>>>>> needs for the current running use case.
>>>>>
>>>>> In that way, they keep existing runtime PM deployment, operating on
>>>>> its original device.
>>>>
>>>> OK, sounds good. Any reason why the linking cannot be handled by the above API? Is there a use-case where you would not want it linked?
>>>
>>> I am guessing the linking is what would give the driver the ability to decide which subset of powerdomains it actually wants to control
>>> at any point using runtime PM. If we have cases wherein the driver would want to turn on/off _all_ its associated powerdomains _always_
>>> then a default linking of all would help.
>>
>> First, I think we need to decide on *where* the linking should be
>> done, not at both places, as that would just mess up synchronization
>> of who is responsible for calling the device_link_del() at detach.
>>
>> Second, It would in principle be fine to call device_link_add() and
>> device_link_del() as a part of the attach/detach APIs. However, there
>> is a downside to such solution, which would be that the driver then
>> needs call the detach API, just to do device_link_del(). Of course
>> then it would also needs to call the attach API later if/when needed.
>> Doing this adds unnecessary overhead - comparing to just let the
>> driver call device_link_add|del() when needed. On the upside, yes, it
>> would put less burden on the drivers as it then only needs to care
>> about using one set of functions.
>>
>> Which solution do you prefer?
>
> 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.
>
> Rajendra, do you have a use-case right now where the driver would want to handle the linking?
So if I understand this right, any driver which does want to control individual powerdomain state would
need to do the linking itself right?
What I am saying is, if I have device A, with powerdomains X and Y, and if I want to turn on only X,
then I would want only X to be linked with A, and at a later point if I want both X and Y to be turned on,
I would then go ahead and link both X and Y to A? Is that correct or did I get it all wrong?
I know atleast Camera on msm8996 would need to do this since it has 2 vfe powerdoamins, which can be
turned on one at a time (depending on what resolution needs to be supported) or both together if we
really need very high resolution using both vfe modules.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
^ permalink raw reply
* Re: [PATCH 8/9] PM / Domains: Add support for multi PM domains per device to genpd
From: Jon Hunter @ 2018-05-23 9:07 UTC (permalink / raw)
To: Ulf Hansson, Rajendra Nayak
Cc: 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: <CAPDyKFo_69kSS6UzdsJeyeiiED=S=chsdkP+R43qMHDEbZ8n_A@mail.gmail.com>
On 23/05/18 07:12, Ulf Hansson wrote:
...
>>>>> Thanks for sending this. Believe it or not this has still been on my to-do list
>>>>> and so we definitely need a solution for Tegra.
>>>>>
>>>>> Looking at the above it appears that additional power-domains exposed as devices
>>>>> to the client device. So I assume that this means that the drivers for devices
>>>>> with multiple power-domains will need to call RPM APIs for each of these
>>>>> additional power-domains. Is that correct?
>>>>
>>>> They can, but should not!
>>>>
>>>> Instead, the driver shall use device_link_add() and device_link_del(),
>>>> dynamically, depending on what PM domain that their original device
>>>> needs for the current running use case.
>>>>
>>>> In that way, they keep existing runtime PM deployment, operating on
>>>> its original device.
>>>
>>> OK, sounds good. Any reason why the linking cannot be handled by the above API? Is there a use-case where you would not want it linked?
>>
>> I am guessing the linking is what would give the driver the ability to decide which subset of powerdomains it actually wants to control
>> at any point using runtime PM. If we have cases wherein the driver would want to turn on/off _all_ its associated powerdomains _always_
>> then a default linking of all would help.
>
> First, I think we need to decide on *where* the linking should be
> done, not at both places, as that would just mess up synchronization
> of who is responsible for calling the device_link_del() at detach.
>
> Second, It would in principle be fine to call device_link_add() and
> device_link_del() as a part of the attach/detach APIs. However, there
> is a downside to such solution, which would be that the driver then
> needs call the detach API, just to do device_link_del(). Of course
> then it would also needs to call the attach API later if/when needed.
> Doing this adds unnecessary overhead - comparing to just let the
> driver call device_link_add|del() when needed. On the upside, yes, it
> would put less burden on the drivers as it then only needs to care
> about using one set of functions.
>
> Which solution do you prefer?
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.
Rajendra, do you have a use-case right now where the driver would want
to handle the linking?
Cheers
Jon
--
nvpublic
^ permalink raw reply
* Re: [PATCH v3 3/3] ARM: dts: imx6ull-colibri-wifi: remove operating points
From: Stefan Agner @ 2018-05-23 9:07 UTC (permalink / raw)
To: Sébastien Szymanski, Viresh Kumar
Cc: linux-arm-kernel, Rafael J . Wysocki, linux-pm, linux-kernel,
Shawn Guo, Sascha Hauer, Fabio Estevam, Rob Herring, Mark Rutland,
devicetree
In-Reply-To: <20180523043032.2htohlypynnvpiye@vireshk-i7>
On 23.05.2018 06:30, Viresh Kumar wrote:
> On 22-05-18, 08:28, Sébastien Szymanski wrote:
>> Operating points are now defined in the imx6ull.dtsi file so remove
>> them from board device trees.
>>
>> Signed-off-by: Sébastien Szymanski <sebastien.szymanski@armadeus.com>
>> ---
>> arch/arm/boot/dts/imx6ull-colibri-wifi.dtsi | 14 --------------
>> 1 file changed, 14 deletions(-)
>>
>> diff --git a/arch/arm/boot/dts/imx6ull-colibri-wifi.dtsi b/arch/arm/boot/dts/imx6ull-colibri-wifi.dtsi
>> index 3dffbcd50bf6..183193e8580d 100644
>> --- a/arch/arm/boot/dts/imx6ull-colibri-wifi.dtsi
>> +++ b/arch/arm/boot/dts/imx6ull-colibri-wifi.dtsi
>> @@ -20,20 +20,6 @@
>>
>> &cpu0 {
>> clock-frequency = <792000000>;
>> - operating-points = <
>> - /* kHz uV */
>> - 792000 1225000
>> - 528000 1175000
>> - 396000 1025000
>> - 198000 950000
>> - >;
>> - fsl,soc-operating-points = <
>> - /* KHz uV */
>> - 792000 1175000
>> - 528000 1175000
>> - 396000 1175000
>> - 198000 1175000
>> - >;
>> };
>>
>> &iomuxc {
>
> Maybe you should merge this with the previous patch itself.
I am with Viresh here, I rather prefer this in a single commit so it is
clear that frequencies moved to the base device tree.
Also, add a comment that frequency selection is now handled in code,
e.g.:
"The valid frequencies for a particular SKU are now selected by the
cpufreq driver according to ratings stored in OTP fuses."
But the two device tree changes with the driver do what they should do
here, so:
Tested-by: Stefan Agner <stefan@agner.ch>
Reviewed-by: Stefan Agner <stefan@agner.ch>
--
Stefan
^ permalink raw reply
* [PATCH] cpufreq: Add Kryo CPU scaling driver
From: Ilia Lin @ 2018-05-23 9:05 UTC (permalink / raw)
To: viresh.kumar, rjw, sudeep.holla, linux
Cc: linux-clk, devicetree, linux-kernel, linux-pm, linux-arm-msm,
linux-soc, linux-arm-kernel
In-Reply-To: <1526729701-8589-1-git-send-email-ilialin@codeaurora.org>
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 | 163 +++++++++++++++++++++++++++++++++++
4 files changed, 177 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..885051e
--- /dev/null
+++ b/drivers/cpufreq/qcom-cpufreq-kryo.c
@@ -0,0 +1,163 @@
+// 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 __init qcom_cpufreq_kryo_driver_init(void)
+{
+ struct opp_table *opp_tables[NR_CPUS] = {0};
+ enum _msm8996_version msm8996_version;
+ struct nvmem_cell *speedbin_nvmem;
+ struct platform_device *pdev;
+ 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")) {
+ ret = -ENOENT;
+ goto free_np;
+ }
+
+ speedbin_nvmem = of_nvmem_cell_get(np, NULL);
+ if (IS_ERR(speedbin_nvmem)) {
+ ret = PTR_ERR(speedbin_nvmem);
+ dev_err(cpu_dev, "Could not get nvmem cell: %d\n", ret);
+ goto free_np;
+ }
+
+ 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;
+ }
+ }
+
+ pdev = platform_device_register_simple("cpufreq-dt", -1, NULL, 0);
+ if (!IS_ERR(pdev))
+ return 0;
+
+ ret = PTR_ERR(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]);
+ }
+free_np:
+ of_node_put(np);
+
+ return ret;
+}
+late_initcall(qcom_cpufreq_kryo_driver_init);
+
+MODULE_DESCRIPTION("Qualcomm Technologies, Inc. Kryo CPUfreq driver");
+MODULE_LICENSE("GPL v2");
--
1.9.1
^ permalink raw reply related
* Re: [PATCH v3 1/3] cpufreq: imx6q: check speed grades for i.MX6ULL
From: Stefan Agner @ 2018-05-23 9:02 UTC (permalink / raw)
To: Sébastien Szymanski
Cc: linux-arm-kernel, Rafael J . Wysocki, Viresh Kumar, linux-pm,
linux-kernel, Shawn Guo, Sascha Hauer, Fabio Estevam, Rob Herring,
Mark Rutland, devicetree
In-Reply-To: <20180522062853.24799-1-sebastien.szymanski@armadeus.com>
On 22.05.2018 08:28, Sébastien Szymanski wrote:
> Check the max speed supported from the fuses for i.MX6ULL and update the
> operating points table accordingly.
>
> Signed-off-by: Sébastien Szymanski <sebastien.szymanski@armadeus.com>
Tested with a 528MHz and 792MHz rated i.MX 6ULL, looks good!
Tested-by: Stefan Agner <stefan@agner.ch>
Reviewed-by: Stefan Agner <stefan@agner.ch>
--
Stefan
> ---
>
> Changes for v3:
> - none
>
> Changes for v2:
> - none
>
> drivers/cpufreq/imx6q-cpufreq.c | 29 +++++++++++++++++++++++------
> 1 file changed, 23 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/cpufreq/imx6q-cpufreq.c b/drivers/cpufreq/imx6q-cpufreq.c
> index 83cf631fc9bc..f094687cae52 100644
> --- a/drivers/cpufreq/imx6q-cpufreq.c
> +++ b/drivers/cpufreq/imx6q-cpufreq.c
> @@ -266,6 +266,8 @@ static void imx6q_opp_check_speed_grading(struct
> device *dev)
> }
>
> #define OCOTP_CFG3_6UL_SPEED_696MHZ 0x2
> +#define OCOTP_CFG3_6ULL_SPEED_792MHZ 0x2
> +#define OCOTP_CFG3_6ULL_SPEED_900MHZ 0x3
>
> static void imx6ul_opp_check_speed_grading(struct device *dev)
> {
> @@ -287,16 +289,30 @@ static void
> imx6ul_opp_check_speed_grading(struct device *dev)
> * Speed GRADING[1:0] defines the max speed of ARM:
> * 2b'00: Reserved;
> * 2b'01: 528000000Hz;
> - * 2b'10: 696000000Hz;
> - * 2b'11: Reserved;
> + * 2b'10: 696000000Hz on i.MX6UL, 792000000Hz on i.MX6ULL;
> + * 2b'11: 900000000Hz on i.MX6ULL only;
> * We need to set the max speed of ARM according to fuse map.
> */
> val = readl_relaxed(base + OCOTP_CFG3);
> val >>= OCOTP_CFG3_SPEED_SHIFT;
> val &= 0x3;
> - if (val != OCOTP_CFG3_6UL_SPEED_696MHZ)
> - if (dev_pm_opp_disable(dev, 696000000))
> - dev_warn(dev, "failed to disable 696MHz OPP\n");
> +
> + if (of_machine_is_compatible("fsl,imx6ul")) {
> + if (val != OCOTP_CFG3_6UL_SPEED_696MHZ)
> + if (dev_pm_opp_disable(dev, 696000000))
> + dev_warn(dev, "failed to disable 696MHz OPP\n");
> + }
> +
> + if (of_machine_is_compatible("fsl,imx6ull")) {
> + if (val != OCOTP_CFG3_6ULL_SPEED_792MHZ)
> + if (dev_pm_opp_disable(dev, 792000000))
> + dev_warn(dev, "failed to disable 792MHz OPP\n");
> +
> + if (val != OCOTP_CFG3_6ULL_SPEED_900MHZ)
> + if (dev_pm_opp_disable(dev, 900000000))
> + dev_warn(dev, "failed to disable 900MHz OPP\n");
> + }
> +
> iounmap(base);
> put_node:
> of_node_put(np);
> @@ -356,7 +372,8 @@ static int imx6q_cpufreq_probe(struct platform_device *pdev)
> goto put_reg;
> }
>
> - if (of_machine_is_compatible("fsl,imx6ul"))
> + if (of_machine_is_compatible("fsl,imx6ul") ||
> + of_machine_is_compatible("fsl,imx6ull"))
> imx6ul_opp_check_speed_grading(cpu_dev);
> else
> imx6q_opp_check_speed_grading(cpu_dev);
^ permalink raw reply
* Re: [PATCH v2] schedutil: Allow cpufreq requests to be made even when kthread kicked
From: Viresh Kumar @ 2018-05-23 9:01 UTC (permalink / raw)
To: Joel Fernandes
Cc: Joel Fernandes (Google.), linux-kernel, Rafael J . Wysocki,
Peter Zijlstra, Ingo Molnar, Patrick Bellasi, Juri Lelli,
Luca Abeni, Todd Kjos, claudio, kernel-team, linux-pm
In-Reply-To: <20180522220953.GB40506@joelaf.mtv.corp.google.com>
On 22-05-18, 15:09, Joel Fernandes wrote:
> I agree with the race you describe for single policy slow-switch. Good find :)
>
> The mainline sugov_work could also do such reordering in sugov_work, I think. Even
> with the mutex_unlock in mainline's sugov_work, that work_in_progress write could
> be reordered by the CPU to happen before the read of next_freq. AIUI,
> mutex_unlock is expected to be only a release-barrier.
>
> Although to be safe, I could just put an smp_mb() there. I believe with that,
> no locking would be needed for such case.
>
> I'll send out a v3 with Acks for the original patch, and the send out the
> smp_mb() as a separate patch if that's Ok.
Maybe it would be better to get the fix (with smp_mb) first and then
this optimization patch on the top? That would mean that the fix can
get applied to stable kernels easily.
--
viresh
^ permalink raw reply
* Re: [PATCH RFC] schedutil: Address the r/w ordering race in kthread
From: Rafael J. Wysocki @ 2018-05-23 8:23 UTC (permalink / raw)
To: Joel Fernandes (Google)
Cc: Linux Kernel Mailing List, Joel Fernandes (Google),
Rafael J . Wysocki, Peter Zijlstra, Ingo Molnar, Patrick Bellasi,
Juri Lelli, Luca Abeni, Todd Kjos, Claudio Scordino, kernel-team,
Linux PM
In-Reply-To: <20180522235028.80564-1-joel@joelfernandes.org>
On Wed, May 23, 2018 at 1:50 AM, Joel Fernandes (Google)
<joelaf@google.com> wrote:
> Currently there is a race in schedutil code for slow-switch single-CPU
> systems. Fix it by enforcing ordering the write to work_in_progress to
> happen before the read of next_freq.
>
> Kthread Sched update
>
> sugov_work() sugov_update_single()
>
> lock();
> // The CPU is free to rearrange below
> // two in any order, so it may clear
> // the flag first and then read next
> // freq. Lets assume it does.
> work_in_progress = false
>
> if (work_in_progress)
> return;
>
> sg_policy->next_freq = 0;
> freq = sg_policy->next_freq;
> sg_policy->next_freq = real-freq;
> unlock();
>
> Reported-by: Viresh Kumar <viresh.kumar@linaro.org>
> CC: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> CC: Peter Zijlstra <peterz@infradead.org>
> CC: Ingo Molnar <mingo@redhat.com>
> CC: Patrick Bellasi <patrick.bellasi@arm.com>
> CC: Juri Lelli <juri.lelli@redhat.com>
> Cc: Luca Abeni <luca.abeni@santannapisa.it>
> CC: Todd Kjos <tkjos@google.com>
> CC: claudio@evidence.eu.com
> CC: kernel-team@android.com
> CC: linux-pm@vger.kernel.org
> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> ---
> I split this into separate patch, because this race can also happen in
> mainline.
>
> kernel/sched/cpufreq_schedutil.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
> index 5c482ec38610..ce7749da7a44 100644
> --- a/kernel/sched/cpufreq_schedutil.c
> +++ b/kernel/sched/cpufreq_schedutil.c
> @@ -401,6 +401,13 @@ static void sugov_work(struct kthread_work *work)
> */
> raw_spin_lock_irqsave(&sg_policy->update_lock, flags);
> freq = sg_policy->next_freq;
> +
> + /*
> + * sugov_update_single can access work_in_progress without update_lock,
> + * make sure next_freq is read before work_in_progress is set.
> + */
> + smp_mb();
> +
This requires a corresponding barrier somewhere else.
> sg_policy->work_in_progress = false;
> raw_spin_unlock_irqrestore(&sg_policy->update_lock, flags);
>
> --
Also, as I said I actually would prefer to use the spinlock in the
one-CPU case when the kthread is used.
I'll have a patch for that shortly.
^ permalink raw reply
* Re: [PATCH v2] schedutil: Allow cpufreq requests to be made even when kthread kicked
From: Rafael J. Wysocki @ 2018-05-23 8:18 UTC (permalink / raw)
To: Joel Fernandes
Cc: Viresh Kumar, Joel Fernandes (Google.), Linux Kernel Mailing List,
Rafael J . Wysocki, Peter Zijlstra, Ingo Molnar, Patrick Bellasi,
Juri Lelli, Luca Abeni, Todd Kjos, Claudio Scordino, kernel-team,
Linux PM
In-Reply-To: <20180522220953.GB40506@joelaf.mtv.corp.google.com>
On Wed, May 23, 2018 at 12:09 AM, Joel Fernandes <joel@joelfernandes.org> wrote:
> On Tue, May 22, 2018 at 04:04:15PM +0530, Viresh Kumar wrote:
>> Okay, me and Rafael were discussing this patch, locking and races around this.
>>
>> On 18-05-18, 11:55, Joel Fernandes (Google.) wrote:
>> > diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
>> > index e13df951aca7..5c482ec38610 100644
>> > --- a/kernel/sched/cpufreq_schedutil.c
>> > +++ b/kernel/sched/cpufreq_schedutil.c
>> > @@ -92,9 +92,6 @@ static bool sugov_should_update_freq(struct sugov_policy *sg_policy, u64 time)
>> > !cpufreq_can_do_remote_dvfs(sg_policy->policy))
>> > return false;
>> >
>> > - if (sg_policy->work_in_progress)
>> > - return false;
>> > -
>> > if (unlikely(sg_policy->need_freq_update)) {
>> > sg_policy->need_freq_update = false;
>> > /*
>> > @@ -128,7 +125,7 @@ static void sugov_update_commit(struct sugov_policy *sg_policy, u64 time,
>> >
>> > policy->cur = next_freq;
>> > trace_cpu_frequency(next_freq, smp_processor_id());
>> > - } else {
>> > + } else if (!sg_policy->work_in_progress) {
>> > sg_policy->work_in_progress = true;
>> > irq_work_queue(&sg_policy->irq_work);
>> > }
>> > @@ -291,6 +288,13 @@ static void sugov_update_single(struct update_util_data *hook, u64 time,
>> >
>> > ignore_dl_rate_limit(sg_cpu, sg_policy);
>> >
>> > + /*
>> > + * For slow-switch systems, single policy requests can't run at the
>> > + * moment if update is in progress, unless we acquire update_lock.
>> > + */
>> > + if (sg_policy->work_in_progress)
>> > + return;
>> > +
>> > if (!sugov_should_update_freq(sg_policy, time))
>> > return;
>> >
>> > @@ -382,13 +386,27 @@ sugov_update_shared(struct update_util_data *hook, u64 time, unsigned int flags)
>> > static void sugov_work(struct kthread_work *work)
>> > {
>> > struct sugov_policy *sg_policy = container_of(work, struct sugov_policy, work);
>> > + unsigned int freq;
>> > + unsigned long flags;
>> > +
>> > + /*
>> > + * Hold sg_policy->update_lock shortly to handle the case where:
>> > + * incase sg_policy->next_freq is read here, and then updated by
>> > + * sugov_update_shared just before work_in_progress is set to false
>> > + * here, we may miss queueing the new update.
>> > + *
>> > + * Note: If a work was queued after the update_lock is released,
>> > + * sugov_work will just be called again by kthread_work code; and the
>> > + * request will be proceed before the sugov thread sleeps.
>> > + */
>> > + raw_spin_lock_irqsave(&sg_policy->update_lock, flags);
>> > + freq = sg_policy->next_freq;
>> > + sg_policy->work_in_progress = false;
>> > + raw_spin_unlock_irqrestore(&sg_policy->update_lock, flags);
>> >
>> > mutex_lock(&sg_policy->work_lock);
>> > - __cpufreq_driver_target(sg_policy->policy, sg_policy->next_freq,
>> > - CPUFREQ_RELATION_L);
>> > + __cpufreq_driver_target(sg_policy->policy, freq, CPUFREQ_RELATION_L);
>> > mutex_unlock(&sg_policy->work_lock);
>> > -
>> > - sg_policy->work_in_progress = false;
>> > }
>>
>> And I do see a race here for single policy systems doing slow switching.
>>
>> Kthread Sched update
>>
>> sugov_work() sugov_update_single()
>>
>> lock();
>> // The CPU is free to rearrange below
>> // two in any order, so it may clear
>> // the flag first and then read next
>> // freq. Lets assume it does.
>> work_in_progress = false
>>
>> if (work_in_progress)
>> return;
>>
>> sg_policy->next_freq = 0;
>> freq = sg_policy->next_freq;
>> sg_policy->next_freq = real-next-freq;
>> unlock();
>>
>
> I agree with the race you describe for single policy slow-switch. Good find :)
>
> The mainline sugov_work could also do such reordering in sugov_work, I think. Even
> with the mutex_unlock in mainline's sugov_work, that work_in_progress write could
> be reordered by the CPU to happen before the read of next_freq. AIUI,
> mutex_unlock is expected to be only a release-barrier.
>
> Although to be safe, I could just put an smp_mb() there. I believe with that,
> no locking would be needed for such case.
Yes, but leaving the work_in_progress check in sugov_update_single()
means that the original problem is still there in the one-CPU policy
case. Namely, utilization updates coming in between setting
work_in_progress in sugov_update_commit() and clearing it in
sugov_work() will be discarded in the one-CPU policy case, but not in
the shared policy case.
> I'll send out a v3 with Acks for the original patch,
OK
> and the send out the smp_mb() as a separate patch if that's Ok.
I would prefer to use a spinlock in the one-CPU policy non-fast-switch
case and remove the work_in_progress check from sugov_update_single().
I can do a patch on top of yours for that. In fact, I've done that already. :-)
Thanks,
Rafael
^ permalink raw reply
* Re: [PATCH] PCI / PM: Do not clear state_saved for devices that remain suspended
From: Rafael J. Wysocki @ 2018-05-23 8:11 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Rafael J. Wysocki, Linux PCI, Linux PM, LKML, Bjorn Helgaas,
Mika Westerberg
In-Reply-To: <20180522220149.GB22385@bhelgaas-glaptop.roam.corp.google.com>
On Wed, May 23, 2018 at 12:01 AM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> On Fri, May 18, 2018 at 10:17:42AM +0200, Rafael J. Wysocki wrote:
>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>
>> The state_saved flag should not be cleared in pci_pm_suspend() if the
>> given device is going to remain suspended, or the device's config
>> space will not be restored properly during the subsequent resume.
>>
>> Namely, if the device is going to stay in suspend, both the late
>> and noirq callbacks return early for it, so if its state_saved flag
>> is cleared in pci_pm_suspend(), it will remain unset throughout the
>> remaining part of suspend and resume and pci_restore_state() called
>> for the device going forward will return without doing anything.
>>
>> For this reason, change pci_pm_suspend() to only clear state_saved
>> if the given device is not going to remain suspended. [This is
>> analogous to what commit ae860a19f37c (PCI / PM: Do not clear
>> state_saved in pci_pm_freeze() when smart suspend is set) did for
>> hibernation.]
>>
>> Fixes: c4b65157aeef (PCI / PM: Take SMART_SUSPEND driver flag into account)
>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Acked-by: Bjorn Helgaas <bhelgaas@google.com>
>
> I assume you'll take this one, too.
Yes, I will, thank you!
^ 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