* Re: [PATCH] cpufreq: Add Kryo CPU scaling driver
From: Viresh Kumar @ 2018-05-23 5:44 UTC (permalink / raw)
To: Sudeep Holla
Cc: Ilia Lin, linux-clk, devicetree, linux-kernel, linux-pm,
linux-arm-msm, linux-soc, linux-arm-kernel
In-Reply-To: <20180522130704.GA31065@e107155-lin>
On 22-05-18, 14:07, Sudeep Holla wrote:
> On Tue, May 22, 2018 at 02:29:45PM +0300, 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>
> > Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
>
> [...]
>
> > +
> > + 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);
>
> Will be not get NULL for all CPUs except 0 ?
With my patches, we will get the OPP table again with refcount
incremented. And on failures, we need to call put-supported-hw helper
only for the CPUs for which it passed previously.
--
viresh
^ permalink raw reply
* Re: [PATCH V3] powercap/drivers/idle_injection: Add an idle injection framework
From: Viresh Kumar @ 2018-05-23 5:41 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: <7ff0d7d4-be5e-2384-1201-df8616519892@linaro.org>
On 22-05-18, 15:42, Daniel Lezcano wrote:
> On 21/05/2018 12:32, Viresh Kumar wrote:
> > On 18-05-18, 16:50, Daniel Lezcano wrote:
> >> Initially, the cpu_cooling device for ARM was changed by adding a new
> >> policy inserting idle cycles. The intel_powerclamp driver does a
> >> similar action.
> >>
> >> Instead of implementing idle injections privately in the cpu_cooling
> >> device, move the idle injection code in a dedicated framework and give
> >> the opportunity to other frameworks to make use of it.
> >
> > I thought you agreed to move above in the comments section ?
>
> This is what I did. I just kept the relevant log here.
The fact that you are stating that you tried to update the cooling
device earlier looked like a bit of version history to me, not what
this patch is doing.
But its okay if you really want that to be preserved in git history :)
> >> +static void idle_injection_fn(unsigned int cpu)
> >> +{
> >> + struct idle_injection_device *ii_dev;
> >> + struct idle_injection_thread *iit;
> >> + int run_duration_ms, idle_duration_ms;
> >> +
> >> + ii_dev = per_cpu(idle_injection_device, cpu);
> >> +
> >> + iit = per_cpu_ptr(&idle_injection_thread, cpu);
> >> +
> >> + /*
> >> + * Boolean used by the smpboot mainloop and used as a flip-flop
> >> + * in this function
> >> + */
> >> + iit->should_run = 0;
> >> +
> >> + atomic_inc(&ii_dev->count);
> >> +
> >> + idle_duration_ms = atomic_read(&ii_dev->idle_duration_ms);
> >> +
> >> + play_idle(idle_duration_ms);
> >> +
> >> + /*
> >> + * The last CPU waking up is in charge of setting the timer. If
> >> + * the CPU is hotplugged, the timer will move to another CPU
> >> + * (which may not belong to the same cluster) but that is not a
> >> + * problem as the timer will be set again by another CPU
> >> + * belonging to the cluster. This mechanism is self adaptive.
> >> + */
> >> + if (!atomic_dec_and_test(&ii_dev->count))
> >> + return;
> >> +
> >> + run_duration_ms = atomic_read(&ii_dev->run_duration_ms);
> >
> > This reads as if it is okay to have run_duration_ms set as 0, so we
> > run idle loop only once. Which is fine, but why do you mandate this to
> > be non-zero in idle_injection_start() ?
>
> It does not make sense to run this function with a run duration set to
> zero because we will immediately go to idle again after exiting idle. So
> the action is exiting. In this context we can't accept to start
> injecting idle cycles.
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().
> >> + if (!run_duration_ms)
> >> + return;
> >> +
> >> + hrtimer_start(&ii_dev->timer, ms_to_ktime(run_duration_ms),
> >> + HRTIMER_MODE_REL_PINNED);
> >> +}
> >> +
> >> +/**
> >> + * idle_injection_set_duration - idle and run duration helper
> >> + * @run_duration_ms: an unsigned int giving the running time in milliseconds
> >> + * @idle_duration_ms: an unsigned int giving the idle time in milliseconds
> >> + */
> >> +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 ?
> >> +}
> >> +
> >> +/**
> >> + * idle_injection_get_duration - idle and run duration helper
> >> + * @run_duration_ms: a pointer to an unsigned int to store the running time
> >> + * @idle_duration_ms: a pointer to an unsigned int to store the idle time
> >> + */
> >> +void idle_injection_get_duration(struct idle_injection_device *ii_dev,
> >> + unsigned int *run_duration_ms,
> >> + unsigned int *idle_duration_ms)
> >> +{
> >> + *run_duration_ms = atomic_read(&ii_dev->run_duration_ms);
> >> + *idle_duration_ms = atomic_read(&ii_dev->idle_duration_ms);
> >> +}
> >> +
> >> +/**
> >> + * idle_injection_start - starts the idle injections
> >> + * @ii_dev: a pointer to an idle_injection_device structure
> >> + *
> >> + * The function starts the idle injection cycles by first waking up
> >> + * all the tasks the ii_dev is attached to and let them handle the
> >> + * idle-run periods.
> >> + *
> >> + * Return: -EINVAL if the idle or the running durations are not set.
> >> + */
> >> +int idle_injection_start(struct idle_injection_device *ii_dev)
> >> +{
> >> + if (!atomic_read(&ii_dev->idle_duration_ms))
> >> + return -EINVAL;
> >> +
> >> + if (!atomic_read(&ii_dev->run_duration_ms))
> >> + return -EINVAL;
> >> +
So according to above comments from me, I am saying that this
particular test isn't really required as we may want to run idle loop
only once.
> >> + pr_debug("Starting injecting idle cycles on CPUs '%*pbl'\n",
> >> + cpumask_pr_args(ii_dev->cpumask));
> >> +
> >> + idle_injection_wakeup(ii_dev);
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +/**
> >> + * idle_injection_stop - stops the idle injections
> >> + * @ii_dev: a pointer to an idle injection_device structure
> >> + *
> >> + * The function stops the idle injection by canceling the timer in
> >> + * charge of waking up the tasks to inject idle and unset the idle and
> >> + * running durations.
> >> + */
> >> +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 :(
> >> +
> >> + idle_injection_set_duration(ii_dev, 0, 0);
> >
> > And why exactly this this required ? Why shouldn't we allow this
> > sequence to work:
> >
> > idle_injection_set_duration()
> > idle_injection_start()
> > idle_injection_stop()
> > idle_injection_start()
> > idle_injection_stop()
> > idle_injection_start()
> > idle_injection_stop()
>
> Sorry, I don't get it.
>
> Who will decide to start() and stop() ?
Some confusion here about the usecase then. How do you see this stuff
getting used and how users (cooling-driver ?) will use it ?
--
viresh
^ permalink raw reply
* Re: [PATCH] cpufreq / CPPC: Add cpuinfo_cur_freq support for CPPC
From: Viresh Kumar @ 2018-05-23 5:02 UTC (permalink / raw)
To: George Cherian; +Cc: linux-kernel, linux-pm, rjw
In-Reply-To: <1526989324-4183-1-git-send-email-george.cherian@cavium.com>
On 22-05-18, 04:42, George Cherian wrote:
> Per Section 8.4.7.1.3 of ACPI 6.2, The platform provides performance
> feedback via set of performance counters. To determine the actual
> performance level delivered over time, OSPM may read a set of
> performance counters from the Reference Performance Counter Register
> and the Delivered Performance Counter Register.
>
> OSPM calculates the delivered performance over a given time period by
> taking a beginning and ending snapshot of both the reference and
> delivered performance counters, and calculating:
>
> delivered_perf = reference_perf X (delta of delivered_perf counter / delta of reference_perf counter).
>
> Implement the above and hook this to the cpufreq->get method.
>
> Signed-off-by: George Cherian <george.cherian@cavium.com>
> ---
> drivers/cpufreq/cppc_cpufreq.c | 44 ++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 44 insertions(+)
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
--
viresh
^ 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 4:51 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: <5a79d3a2-d090-645b-da69-524b7e7a4d90@nvidia.com>
On 05/23/2018 02:25 AM, Jon Hunter wrote:
>
> On 22/05/18 15:47, Ulf Hansson wrote:
>> [...]
>>
>>>>
>>>> +/**
>>>> + * genpd_dev_pm_attach_by_id() - Attach a device to one of its PM domain.
>>>> + * @dev: Device to attach.
>>>> + * @index: The index of the PM domain.
>>>> + *
>>>> + * Parse device's OF node to find a PM domain specifier at the provided @index.
>>>> + * If such is found, allocates a new device and attaches it to retrieved
>>>> + * pm_domain ops.
>>>> + *
>>>> + * Returns the allocated device if successfully attached PM domain, NULL when
>>>> + * the device don't need a PM domain or have a single PM domain, else PTR_ERR()
>>>> + * in case of failures. Note that if a power-domain exists for the device, but
>>>> + * cannot be found or turned on, then return PTR_ERR(-EPROBE_DEFER) to ensure
>>>> + * that the device is not probed and to re-try again later.
>>>> + */
>>>> +struct device *genpd_dev_pm_attach_by_id(struct device *dev,
>>>> + unsigned int index)
>>>> +{
>>>> + struct device *genpd_dev;
>>>> + int num_domains;
>>>> + int ret;
>>>> +
>>>> + if (!dev->of_node)
>>>> + return NULL;
>>>> +
>>>> + /* Deal only with devices using multiple PM domains. */
>>>> + num_domains = of_count_phandle_with_args(dev->of_node, "power-domains",
>>>> + "#power-domain-cells");
>>>> + if (num_domains < 2 || index >= num_domains)
>>>> + return NULL;
>>>> +
>>>> + /* Allocate and register device on the genpd bus. */
>>>> + genpd_dev = kzalloc(sizeof(*genpd_dev), GFP_KERNEL);
>>>> + if (!genpd_dev)
>>>> + return ERR_PTR(-ENOMEM);
>>>> +
>>>> + dev_set_name(genpd_dev, "genpd:%u:%s", index, dev_name(dev));
>>>> + genpd_dev->bus = &genpd_bus_type;
>>>> + genpd_dev->release = genpd_release_dev;
>>>> +
>>>> + ret = device_register(genpd_dev);
>>>> + if (ret) {
>>>> + kfree(genpd_dev);
>>>> + return ERR_PTR(ret);
>>>> + }
>>>> +
>>>> + /* Try to attach the device to the PM domain at the specified index. */
>>>> + ret = __genpd_dev_pm_attach(genpd_dev, dev->of_node, index);
>>>> + if (ret < 1) {
>>>> + device_unregister(genpd_dev);
>>>> + return ret ? ERR_PTR(ret) : NULL;
>>>> + }
>>>> +
>>>> + pm_runtime_set_active(genpd_dev);
>>>> + pm_runtime_enable(genpd_dev);
>>>> +
>>>> + return genpd_dev;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(genpd_dev_pm_attach_by_id);
>>>
>>> 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.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
^ permalink raw reply
* Re: [PATCH v3 3/3] ARM: dts: imx6ull-colibri-wifi: remove operating points
From: Viresh Kumar @ 2018-05-23 4:30 UTC (permalink / raw)
To: Sébastien Szymanski
Cc: linux-arm-kernel, Rafael J . Wysocki, linux-pm, linux-kernel,
Shawn Guo, Sascha Hauer, Fabio Estevam, Stefan Agner, Rob Herring,
Mark Rutland, devicetree
In-Reply-To: <20180522062853.24799-3-sebastien.szymanski@armadeus.com>
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.
--
viresh
^ permalink raw reply
* Re: [PATCH v3 1/3] cpufreq: imx6q: check speed grades for i.MX6ULL
From: Viresh Kumar @ 2018-05-23 4:30 UTC (permalink / raw)
To: Sébastien Szymanski
Cc: linux-arm-kernel, Rafael J . Wysocki, linux-pm, linux-kernel,
Shawn Guo, Sascha Hauer, Fabio Estevam, Stefan Agner, Rob Herring,
Mark Rutland, devicetree
In-Reply-To: <20180522062853.24799-1-sebastien.szymanski@armadeus.com>
On 22-05-18, 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>
> ---
>
> 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);
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
--
viresh
^ permalink raw reply
* Re: [PATCH v3 1/3] cpufreq: imx6q: check speed grades for i.MX6ULL
From: Viresh Kumar @ 2018-05-23 4:29 UTC (permalink / raw)
To: Sébastien Szymanski
Cc: linux-arm-kernel, Rafael J . Wysocki, linux-pm, linux-kernel,
Shawn Guo, Sascha Hauer, Fabio Estevam, Stefan Agner, Rob Herring,
Mark Rutland, devicetree
In-Reply-To: <20180522062853.24799-1-sebastien.szymanski@armadeus.com>
On 22-05-18, 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>
> ---
>
> Changes for v3:
> - none
@Sascha and Shawn: Can you guys please Ack this series if there is
nothing wrong with it ?
--
viresh
^ permalink raw reply
* Re: [PATCH RFC] schedutil: Address the r/w ordering race in kthread
From: Joel Fernandes @ 2018-05-23 0:18 UTC (permalink / raw)
To: Joel Fernandes (Google)
Cc: 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: <20180522235028.80564-1-joel@joelfernandes.org>
On Tue, May 22, 2018 at 04:50:28PM -0700, Joel Fernandes (Google) 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.
Aargh, s/before/after/.
Commit log has above issue but code is Ok. Should I resend this patch or
are there any additional comments? thanks!
- Joel
[..]
^ permalink raw reply
* [PATCH RFC] schedutil: Address the r/w ordering race in kthread
From: Joel Fernandes (Google) @ 2018-05-22 23:50 UTC (permalink / raw)
To: linux-kernel
Cc: Joel Fernandes (Google), Rafael J . Wysocki, Peter Zijlstra,
Ingo Molnar, Patrick Bellasi, Juri Lelli, Luca Abeni, Todd Kjos,
claudio, kernel-team, linux-pm
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();
+
sg_policy->work_in_progress = false;
raw_spin_unlock_irqrestore(&sg_policy->update_lock, flags);
--
2.17.0.441.gb46fe60e1d-goog
^ permalink raw reply related
* [PATCH v3] schedutil: Allow cpufreq requests to be made even when kthread kicked
From: Joel Fernandes @ 2018-05-22 22:55 UTC (permalink / raw)
To: linux-kernel
Cc: 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
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)
--
2.17.0.441.gb46fe60e1d-goog
^ permalink raw reply related
* Re: [RFC PATCH 09/10] arm64: dts: rk3399: Add dfi and dmc nodes.
From: Rob Herring @ 2018-05-22 22:51 UTC (permalink / raw)
To: Enric Balletbo i Serra
Cc: MyungJoo Ham, Kyungmin Park, Chanwoo Choi, Will Deacon,
Heiko Stuebner, Michael Turquette, Stephen Boyd, Sandy Huang,
David Airlie, linux-pm, linux-kernel, Derek Basehore, linux-clk,
linux-rockchip, dri-devel, Lin Huang, kernel, Sean Paul,
linux-arm-kernel, Nickey Yang, devicetree, Yakir Yang
In-Reply-To: <20180514211610.26618-10-enric.balletbo@collabora.com>
On Mon, May 14, 2018 at 11:16:09PM +0200, Enric Balletbo i Serra wrote:
> From: Lin Huang <hl@rock-chips.com>
>
> These are required to support DDR DVFS on rk3399 platform. The patch also
> introduces two new files (rk3399-dram.h and rk3399-dram-default-timing)
> with default DRAM settings.
>
> Signed-off-by: Lin Huang <hl@rock-chips.com>
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> ---
>
> .../rockchip/rk3399-dram-default-timing.dtsi | 38 ++++++++++
> arch/arm64/boot/dts/rockchip/rk3399-dram.h | 73 +++++++++++++++++++
> .../boot/dts/rockchip/rk3399-op1-opp.dtsi | 29 ++++++++
> arch/arm64/boot/dts/rockchip/rk3399.dtsi | 20 +++++
> 4 files changed, 160 insertions(+)
> create mode 100644 arch/arm64/boot/dts/rockchip/rk3399-dram-default-timing.dtsi
> create mode 100644 arch/arm64/boot/dts/rockchip/rk3399-dram.h
>
> diff --git a/arch/arm64/boot/dts/rockchip/rk3399-dram-default-timing.dtsi b/arch/arm64/boot/dts/rockchip/rk3399-dram-default-timing.dtsi
> new file mode 100644
> index 000000000000..4dfe3e1d8bdf
> --- /dev/null
> +++ b/arch/arm64/boot/dts/rockchip/rk3399-dram-default-timing.dtsi
> @@ -0,0 +1,38 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR X11)
> +/*
> + * Copyright (c) 2016-2018, Fuzhou Rockchip Electronics Co., Ltd
> + *
> + * Author: Lin Huang <hl@rock-chips.com>
> + */
> +
> +#include "rk3399-dram.h"
> +
> +rockchip,ddr3_speed_bin = <21>;
> +rockchip,pd_idle = <0x40>;
> +rockchip,sr_idle = <0x2>;
Don't do includes this way please. These should go under a node.
> +rockchip,sr_mc_gate_idle = <0x3>;
> +rockchip,srpd_lite_idle = <0x4>;
> +rockchip,standby_idle = <0x2000>;
> +rockchip,dram_dll_dis_freq = <300000000>;
> +rockchip,phy_dll_dis_freq = <125000000>;
> +rockchip,auto_pd_dis_freq = <666000000>;
> +rockchip,ddr3_odt_dis_freq = <333000000>;
> +rockchip,ddr3_drv = <DDR3_DS_40ohm>;
> +rockchip,ddr3_odt = <DDR3_ODT_120ohm>;
> +rockchip,phy_ddr3_ca_drv = <PHY_DRV_ODT_40>;
> +rockchip,phy_ddr3_dq_drv = <PHY_DRV_ODT_40>;
> +rockchip,phy_ddr3_odt = <PHY_DRV_ODT_240>;
> +rockchip,lpddr3_odt_dis_freq = <333000000>;
> +rockchip,lpddr3_drv = <LP3_DS_34ohm>;
> +rockchip,lpddr3_odt = <LP3_ODT_240ohm>;
> +rockchip,phy_lpddr3_ca_drv = <PHY_DRV_ODT_40>;
> +rockchip,phy_lpddr3_dq_drv = <PHY_DRV_ODT_40>;
> +rockchip,phy_lpddr3_odt = <PHY_DRV_ODT_240>;
> +rockchip,lpddr4_odt_dis_freq = <333000000>;
> +rockchip,lpddr4_drv = <LP4_PDDS_60ohm>;
> +rockchip,lpddr4_dq_odt = <LP4_DQ_ODT_40ohm>;
> +rockchip,lpddr4_ca_odt = <LP4_CA_ODT_40ohm>;
> +rockchip,phy_lpddr4_ca_drv = <PHY_DRV_ODT_40>;
> +rockchip,phy_lpddr4_ck_cs_drv = <PHY_DRV_ODT_80>;
> +rockchip,phy_lpddr4_dq_drv = <PHY_DRV_ODT_80>;
> +rockchip,phy_lpddr4_odt = <PHY_DRV_ODT_60>;
^ permalink raw reply
* Re: [RFC PATCH 02/10] dt-bindings: devfreq: rk3399_dmc: Add rockchip,pmu phandle.
From: Rob Herring @ 2018-05-22 22:45 UTC (permalink / raw)
To: Enric Balletbo i Serra
Cc: Mark Rutland, David Airlie, devicetree, linux-pm, Stephen Boyd,
Michael Turquette, Derek Basehore, Will Deacon, dri-devel,
linux-kernel, Chanwoo Choi, Kyungmin Park, MyungJoo Ham,
linux-rockchip, kernel, linux-clk, linux-arm-kernel, Lin Huang
In-Reply-To: <20180514211610.26618-3-enric.balletbo@collabora.com>
On Mon, May 14, 2018 at 11:16:02PM +0200, Enric Balletbo i Serra wrote:
> The Rockchip DMC (Dynamic Memory Interface) needs to access to the PMU
> general register files to know the DRAM type, so add a phandle to the
> syscon that manages these registers.
>
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> ---
>
> Documentation/devicetree/bindings/devfreq/rk3399_dmc.txt | 2 ++
> 1 file changed, 2 insertions(+)
Acked-by: Rob Herring <robh@kernel.org>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply
* Re: [PATCH v2] schedutil: Allow cpufreq requests to be made even when kthread kicked
From: Joel Fernandes @ 2018-05-22 22:28 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Viresh Kumar, 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,
Joel Fernandes (Google.)
In-Reply-To: <CAJZ5v0ir7Chz6jCZTKRNT12oMd4uyu+JgFLKmZ8RJkcRkyrzBg@mail.gmail.com>
On Tue, May 22, 2018 at 11:52:46PM +0200, Rafael J. Wysocki wrote:
> On Tue, May 22, 2018 at 11:41 PM, Joel Fernandes <joel@joelfernandes.org> wrote:
> > On Tue, May 22, 2018 at 05:27:11PM +0200, Rafael J. Wysocki wrote:
> >> On Tue, May 22, 2018 at 2:22 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> >> > On Tuesday, May 22, 2018 1:42:05 PM CEST Rafael J. Wysocki wrote:
> >> >> On Tue, May 22, 2018 at 1:38 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >> >> > On 22-05-18, 13:31, Rafael J. Wysocki wrote:
> >> >> >> So below is my (compiled-only) version of the $subject patch, obviously based
> >> >> >> on the Joel's work.
> >> >> >>
> >> >> >> Roughly, what it does is to move the fast_switch_enabled path entirely to
> >> >> >> sugov_update_single() and take the spinlock around sugov_update_commit()
> >> >> >> in the one-CPU case too.
> >> >
> >> > [cut]
> >> >
> >> >> >
> >> >> > Why do you assume that fast switch isn't possible in shared policy
> >> >> > cases ? It infact is already enabled for few drivers.
> >> >
> >> > I hope that fast_switch is not used with devfs_possible_from_any_cpu set in the
> >> > one-CPU policy case, as that looks racy even without any patching.
> >>
> >> Which would be the only case in which sugov_update_single() would run
> >> on a CPU that is not the target.
> >>
> >> And running sugov_update_single() concurrently on two different CPUs
> >> for the same target is a no-no, as we don't prevent concurrent updates
> >> from occurring in that path.
> >>
> >> Which means that the original patch from Joel will be sufficient as
> >> long as we ensure that sugov_update_single() can only run on one CPU
> >> at a time.
> >
> > Since target CPU's runqueue lock is held, I don't see how we can run
> > sugov_update_single concurrently with any other CPU for single policy, so
> > protecting such race shouldn't be necessary.
>
> If dvfs_possible_from_any_cpu is set, any CPU can run
> sugov_update_single(), but the kthread will only run on the target
> itself. So another CPU running sugov_update_single() for the target
> may be racing with the target's kthread.
>
Yes, I agree. I thought you meant the case of sugov_update_single running
currently with other sugov_update_single. So just to be on the same page,
I'll fix the commit log and repost this one as is.
And then I'll post the smp_rmb() patch separately to address the memory order
issue (which I believe is in mainline as well). Basically I was thinking to
address Viresh's issue, there should be an smp_mb() after the next_freq is
read, but before the write to work_in_progress.
thanks,
- Joel
^ permalink raw reply
* Re: [PATCH v2] schedutil: Allow cpufreq requests to be made even when kthread kicked
From: Joel Fernandes @ 2018-05-22 22:09 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: <20180522103415.cuutobi5kbhj4gcw@vireshk-i7>
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.
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.
thanks,
- Joel
^ permalink raw reply
* Re: [PATCH] PCI / PM: Do not clear state_saved for devices that remain suspended
From: Bjorn Helgaas @ 2018-05-22 22:01 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Linux PCI, Linux PM, LKML, Bjorn Helgaas, Mika Westerberg
In-Reply-To: <11415287.ki0m5Bqgns@aspire.rjw.lan>
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.
> ---
> drivers/pci/pci-driver.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> Index: linux-pm/drivers/pci/pci-driver.c
> ===================================================================
> --- linux-pm.orig/drivers/pci/pci-driver.c
> +++ linux-pm/drivers/pci/pci-driver.c
> @@ -753,10 +753,11 @@ static int pci_pm_suspend(struct device
> * better to resume the device from runtime suspend here.
> */
> if (!dev_pm_test_driver_flags(dev, DPM_FLAG_SMART_SUSPEND) ||
> - !pci_dev_keep_suspended(pci_dev))
> + !pci_dev_keep_suspended(pci_dev)) {
> pm_runtime_resume(dev);
> + pci_dev->state_saved = false;
> + }
>
> - pci_dev->state_saved = false;
> if (pm->suspend) {
> pci_power_t prev = pci_dev->current_state;
> int error;
>
^ permalink raw reply
* Re: [PATCH v2] schedutil: Allow cpufreq requests to be made even when kthread kicked
From: Rafael J. Wysocki @ 2018-05-22 21:52 UTC (permalink / raw)
To: Joel Fernandes
Cc: Rafael J. Wysocki, Viresh Kumar, 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, Joel Fernandes (Google.)
In-Reply-To: <20180522213113.GA40506@joelaf.mtv.corp.google.com>
On Tue, May 22, 2018 at 11:41 PM, Joel Fernandes <joel@joelfernandes.org> wrote:
> On Tue, May 22, 2018 at 05:27:11PM +0200, Rafael J. Wysocki wrote:
>> On Tue, May 22, 2018 at 2:22 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>> > On Tuesday, May 22, 2018 1:42:05 PM CEST Rafael J. Wysocki wrote:
>> >> On Tue, May 22, 2018 at 1:38 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>> >> > On 22-05-18, 13:31, Rafael J. Wysocki wrote:
>> >> >> So below is my (compiled-only) version of the $subject patch, obviously based
>> >> >> on the Joel's work.
>> >> >>
>> >> >> Roughly, what it does is to move the fast_switch_enabled path entirely to
>> >> >> sugov_update_single() and take the spinlock around sugov_update_commit()
>> >> >> in the one-CPU case too.
>> >
>> > [cut]
>> >
>> >> >
>> >> > Why do you assume that fast switch isn't possible in shared policy
>> >> > cases ? It infact is already enabled for few drivers.
>> >
>> > I hope that fast_switch is not used with devfs_possible_from_any_cpu set in the
>> > one-CPU policy case, as that looks racy even without any patching.
>>
>> Which would be the only case in which sugov_update_single() would run
>> on a CPU that is not the target.
>>
>> And running sugov_update_single() concurrently on two different CPUs
>> for the same target is a no-no, as we don't prevent concurrent updates
>> from occurring in that path.
>>
>> Which means that the original patch from Joel will be sufficient as
>> long as we ensure that sugov_update_single() can only run on one CPU
>> at a time.
>
> Since target CPU's runqueue lock is held, I don't see how we can run
> sugov_update_single concurrently with any other CPU for single policy, so
> protecting such race shouldn't be necessary.
If dvfs_possible_from_any_cpu is set, any CPU can run
sugov_update_single(), but the kthread will only run on the target
itself. So another CPU running sugov_update_single() for the target
may be racing with the target's kthread.
> Also the "if (work_in_progress)" check I added to the sugov_update_single
> doesn't change the behavior of single policy from what it is in mainline
> since we were doing the same thing in already sugov_should_update_freq.
No, it doesn't, which doesn't mean that this is all OK. :-)
^ permalink raw reply
* Re: [PATCH v2] schedutil: Allow cpufreq requests to be made even when kthread kicked
From: Joel Fernandes @ 2018-05-22 21:41 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Viresh Kumar, 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,
Joel Fernandes (Google.)
In-Reply-To: <CAJZ5v0henS+LZ64JQwrXR8LrY7xzcsnnYS7AoBuR9dNaC4mTRw@mail.gmail.com>
On Tue, May 22, 2018 at 05:27:11PM +0200, Rafael J. Wysocki wrote:
> On Tue, May 22, 2018 at 2:22 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > On Tuesday, May 22, 2018 1:42:05 PM CEST Rafael J. Wysocki wrote:
> >> On Tue, May 22, 2018 at 1:38 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >> > On 22-05-18, 13:31, Rafael J. Wysocki wrote:
> >> >> So below is my (compiled-only) version of the $subject patch, obviously based
> >> >> on the Joel's work.
> >> >>
> >> >> Roughly, what it does is to move the fast_switch_enabled path entirely to
> >> >> sugov_update_single() and take the spinlock around sugov_update_commit()
> >> >> in the one-CPU case too.
> >
> > [cut]
> >
> >> >
> >> > Why do you assume that fast switch isn't possible in shared policy
> >> > cases ? It infact is already enabled for few drivers.
> >
> > I hope that fast_switch is not used with devfs_possible_from_any_cpu set in the
> > one-CPU policy case, as that looks racy even without any patching.
>
> Which would be the only case in which sugov_update_single() would run
> on a CPU that is not the target.
>
> And running sugov_update_single() concurrently on two different CPUs
> for the same target is a no-no, as we don't prevent concurrent updates
> from occurring in that path.
>
> Which means that the original patch from Joel will be sufficient as
> long as we ensure that sugov_update_single() can only run on one CPU
> at a time.
Since target CPU's runqueue lock is held, I don't see how we can run
sugov_update_single concurrently with any other CPU for single policy, so
protecting such race shouldn't be necessary.
Also the "if (work_in_progress)" check I added to the sugov_update_single
doesn't change the behavior of single policy from what it is in mainline
since we were doing the same thing in already sugov_should_update_freq.
thanks,
- Joel
^ permalink raw reply
* [PATCH v1] cpufreq: tegra20: Fix imbalanced clock enable count
From: Dmitry Osipenko @ 2018-05-22 21:14 UTC (permalink / raw)
To: Rafael J. Wysocki, Viresh Kumar, Thierry Reding, Jonathan Hunter
Cc: linux-tegra, linux-pm, linux-kernel
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
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);
--
2.17.0
^ permalink raw reply related
* Re: [PATCH 8/9] PM / Domains: Add support for multi PM domains per device to genpd
From: Jon Hunter @ 2018-05-22 20:55 UTC (permalink / raw)
To: Ulf Hansson
Cc: Rafael J . Wysocki, Linux PM, Greg Kroah-Hartman,
Geert Uytterhoeven, Todor Tomov, Rajendra Nayak, Viresh Kumar,
Vincent Guittot, Kevin Hilman, Linux Kernel Mailing List,
Linux ARM, linux-tegra
In-Reply-To: <CAPDyKFr_7hbs-MC1jQn5uAgSnb1UGufXZdEmUqNTM63KemTe1g@mail.gmail.com>
On 22/05/18 15:47, Ulf Hansson wrote:
> [...]
>
>>>
>>> +/**
>>> + * genpd_dev_pm_attach_by_id() - Attach a device to one of its PM domain.
>>> + * @dev: Device to attach.
>>> + * @index: The index of the PM domain.
>>> + *
>>> + * Parse device's OF node to find a PM domain specifier at the provided @index.
>>> + * If such is found, allocates a new device and attaches it to retrieved
>>> + * pm_domain ops.
>>> + *
>>> + * Returns the allocated device if successfully attached PM domain, NULL when
>>> + * the device don't need a PM domain or have a single PM domain, else PTR_ERR()
>>> + * in case of failures. Note that if a power-domain exists for the device, but
>>> + * cannot be found or turned on, then return PTR_ERR(-EPROBE_DEFER) to ensure
>>> + * that the device is not probed and to re-try again later.
>>> + */
>>> +struct device *genpd_dev_pm_attach_by_id(struct device *dev,
>>> + unsigned int index)
>>> +{
>>> + struct device *genpd_dev;
>>> + int num_domains;
>>> + int ret;
>>> +
>>> + if (!dev->of_node)
>>> + return NULL;
>>> +
>>> + /* Deal only with devices using multiple PM domains. */
>>> + num_domains = of_count_phandle_with_args(dev->of_node, "power-domains",
>>> + "#power-domain-cells");
>>> + if (num_domains < 2 || index >= num_domains)
>>> + return NULL;
>>> +
>>> + /* Allocate and register device on the genpd bus. */
>>> + genpd_dev = kzalloc(sizeof(*genpd_dev), GFP_KERNEL);
>>> + if (!genpd_dev)
>>> + return ERR_PTR(-ENOMEM);
>>> +
>>> + dev_set_name(genpd_dev, "genpd:%u:%s", index, dev_name(dev));
>>> + genpd_dev->bus = &genpd_bus_type;
>>> + genpd_dev->release = genpd_release_dev;
>>> +
>>> + ret = device_register(genpd_dev);
>>> + if (ret) {
>>> + kfree(genpd_dev);
>>> + return ERR_PTR(ret);
>>> + }
>>> +
>>> + /* Try to attach the device to the PM domain at the specified index. */
>>> + ret = __genpd_dev_pm_attach(genpd_dev, dev->of_node, index);
>>> + if (ret < 1) {
>>> + device_unregister(genpd_dev);
>>> + return ret ? ERR_PTR(ret) : NULL;
>>> + }
>>> +
>>> + pm_runtime_set_active(genpd_dev);
>>> + pm_runtime_enable(genpd_dev);
>>> +
>>> + return genpd_dev;
>>> +}
>>> +EXPORT_SYMBOL_GPL(genpd_dev_pm_attach_by_id);
>>
>> 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?
Thanks
Jon
--
nvpublic
^ permalink raw reply
* Re: [PATCH] thermal: qcom: tsens: Allow number of sensors to come from DT
From: Rob Herring @ 2018-05-22 20:14 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Zhang Rui, Eduardo Valentin, Mark Rutland, Rajendra Nayak,
linux-pm, devicetree, linux-kernel, linux-arm-msm
In-Reply-To: <20180507235339.8836-1-bjorn.andersson@linaro.org>
On Mon, May 07, 2018 at 04:53:39PM -0700, Bjorn Andersson wrote:
> For platforms that has multiple copies of the TSENS hardware block it's
> necessary to be able to specify the number of sensors per block in DeviceTree.
>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
> .../devicetree/bindings/thermal/qcom-tsens.txt | 1 +
Reviewed-by: Rob Herring <robh@kernel.org>
> drivers/thermal/qcom/tsens.c | 12 +++++++++---
> 2 files changed, 10 insertions(+), 3 deletions(-)
^ permalink raw reply
* Re: [PATCH v2 1/2] dt-bindings: cpufreq: Introduce QCOM CPUFREQ FW bindings
From: Rob Herring @ 2018-05-22 19:31 UTC (permalink / raw)
To: Taniya Das
Cc: Rafael J. Wysocki, Viresh Kumar, linux-kernel, linux-pm,
Stephen Boyd, Rajendra Nayak, Amit Nischal, devicetree, skannan,
amit.kucheria
In-Reply-To: <1526751291-17873-2-git-send-email-tdas@codeaurora.org>
On Sat, May 19, 2018 at 11:04:50PM +0530, Taniya Das wrote:
> Add QCOM cpufreq firmware device bindings for Qualcomm Technology Inc's
> SoCs. This is required for managing the cpu frequency transitions which are
> controlled by firmware.
>
> Signed-off-by: Taniya Das <tdas@codeaurora.org>
> ---
> .../bindings/cpufreq/cpufreq-qcom-fw.txt | 68 ++++++++++++++++++++++
> 1 file changed, 68 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-fw.txt
>
> diff --git a/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-fw.txt b/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-fw.txt
> new file mode 100644
> index 0000000..bc912f4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-fw.txt
> @@ -0,0 +1,68 @@
> +Qualcomm Technologies, Inc. CPUFREQ Bindings
> +
> +CPUFREQ FW is a hardware engine used by some Qualcomm Technologies, Inc. (QTI)
> +SoCs to manage frequency in hardware. It is capable of controlling frequency
> +for multiple clusters.
> +
> +Properties:
> +- compatible
> + Usage: required
> + Value type: <string>
> + Definition: must be "qcom,cpufreq-fw".
Only 1 version ever?
> +
> +Note that #address-cells, #size-cells, and ranges shall be present to ensure
> +the cpufreq can address a freq-domain registers.
> +
> +A freq-domain sub-node would be defined for the cpus with the following
> +properties:
> +
> +- compatible:
> + Usage: required
> + Value type: <string>
> + Definition: must be "cpufreq".
Too generic. This is very much a QCom specific binding. Perhaps just
drop the compatible.
> +
> +- reg
> + Usage: required
> + Value type: <prop-encoded-array>
> + Definition: Addresses and sizes for the memory of the perf_base
> + , lut_base and en_base.
> +- reg-names
> + Usage: required
> + Value type: <stringlist>
> + Definition: Address names. Must be "perf_base", "lut_base",
> + "en_base".
_base is redundant.
> + Must be specified in the same order as the
> + corresponding addresses are specified in the reg
> + property.
Actually, must be specified in the order you specify here.
> +
> +- qcom,cpulist
> + Usage: required
> + Value type: <phandles of CPU>
> + Definition: List of related cpu handles which are under a cluster.
> +
> +Example:
> + qcom,cpufreq-fw {
> + compatible = "qcom,cpufreq-fw";
> +
> + #address-cells = <1>;
> + #size-cells = <1>;
> + ranges;
There's not a smaller address range you can restrict children to?
> +
> + freq-domain-0 {
> + compatible = "cpufreq";
> + reg = <0x17d43920 0x4>,
> + <0x17d43110 0x500>,
> + <0x17d41000 0x4>;
> + reg-names = "perf_base", "lut_base", "en_base";
> + qcom,cpulist = <&CPU0 &CPU1 &CPU2 &CPU3>;
> + };
> +
> + freq-domain-1 {
> + compatible = "cpufreq";
> + reg = <0x17d46120 0x4>,
> + <0x17d45910 0x500>,
> + <0x17d45800 0x4>;
> + reg-names = "perf_base", "lut_base", "en_base";
> + qcom,cpulist = <&CPU4 &CPU5 &CPU6 &CPU7>;
> + };
> + };
> --
> Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc.is a member
> of the Code Aurora Forum, hosted by the Linux Foundation.
>
^ permalink raw reply
* Re: [PATCH v2] PM / devfreq: Add support for QCOM devfreq firmware
From: Saravana Kannan @ 2018-05-22 18:30 UTC (permalink / raw)
To: Rob Herring
Cc: MyungJoo Ham, Kyungmin Park, Chanwoo Choi, Mark Rutland,
Rajendra Nayak, Amit Kucheria, linux-pm, devicetree, linux-kernel
In-Reply-To: <20180522180838.GA13423@rob-hp-laptop>
On 05/22/2018 11:08 AM, Rob Herring wrote:
> On Fri, May 18, 2018 at 12:52:40AM -0700, Saravana Kannan wrote:
>> The firmware present in some QCOM chipsets offloads the steps necessary for
>> changing the frequency of some devices (Eg: L3). This driver implements the
>> devfreq interface for this firmware so that various governors could be used
>> to scale the frequency of these devices.
>>
>> Each client (say cluster 0 and cluster 1) that wants to vote for a
>> particular device's frequency (say, L3 frequency) is represented as a
>> separate voter device (qcom,devfreq-fw-voter) that's a child of the
>> firmware device (qcom,devfreq-fw).
>>
>> Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
>> ---
>> .../bindings/devfreq/devfreq-qcom-fw.txt | 41 +++
>> drivers/devfreq/Kconfig | 14 +
>> drivers/devfreq/Makefile | 1 +
>> drivers/devfreq/devfreq_qcom_fw.c | 330 +++++++++++++++++++++
>> 4 files changed, 386 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/devfreq/devfreq-qcom-fw.txt
>> create mode 100644 drivers/devfreq/devfreq_qcom_fw.c
>>
>> diff --git a/Documentation/devicetree/bindings/devfreq/devfreq-qcom-fw.txt b/Documentation/devicetree/bindings/devfreq/devfreq-qcom-fw.txt
>> new file mode 100644
>> index 0000000..f882a0b
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/devfreq/devfreq-qcom-fw.txt
>> @@ -0,0 +1,41 @@
>> +QCOM Devfreq firmware device
>> +
>> +Some Qualcomm Technologies, Inc. (QTI) chipsets have a firmware that
>> +offloads the steps for frequency switching. It provides a table of
>> +supported frequencies and a register to request one of the supported
>> +freqencies.
>> +
>> +The qcom,devfreq-fw represents this firmware as a device. Sometimes,
>> +multiple entities want to vote on the frequency request that is sent to the
>> +firmware. The qcom,devfreq-fw-voter represents these voters as child
>> +devices of the corresponding qcom,devfreq-fw device.
>> +
>> +Required properties:
>> +- compatible: Must be "qcom,devfreq-fw" or "qcom,devfreq-fw-voter"
>
> No versions of firmware?
Sure, I can add a v1. Right now the interface has always been identical.
I thought if it changed in the future I'll add -v2.
>
>> +Only for qcom,devfreq-fw:
>> +- reg: Pairs of physical base addresses and region sizes of
>> + memory mapped registers.
>
> Registers? Is this firmware or h/w block?
It's a HW block that has its own firmware.
>
>> +- reg-names: Names of the bases for the above registers.
>> + Required register regions are:
>> + - "en-base": address of register to check if the
>> + firmware is enabled.
>> + - "ftbl-base": address region for the frequency
>> + table.
>> + - "perf-base": address of register to request a
>> + frequency.
>> +
>> +Example:
>> +
>> + qcom,devfreq-l3 {
>> + compatible = "qcom,devfreq-fw";
>> + reg-names = "en-base", "ftbl-base", "perf-base";
>> + reg = <0x18321000 0x4>, <0x18321110 0x600>, <0x18321920 0x4>;
>> +
>> + qcom,cpu0-l3 {
>> + compatible = "qcom,devfreq-fw-voter";
>
> There's no point in these nodes. They don't have any properties or
> resources.
These nodes decide how many voters this device supports. Each voter
would be a devfreq node that will have its own governor set. For
example, one of them would use this governor:
http://lkml.iu.edu/hypermail/linux/kernel/1805.2/02474.html
You can also attach different devfreq-event devices to each one of these
voter devices based on what events you want to use for scaling each
voter. So, the devices are definitely needed in the larger context.
Thanks,
Saravana
--
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply
* Re: [PATCH v2] PM / devfreq: Add support for QCOM devfreq firmware
From: Rob Herring @ 2018-05-22 18:08 UTC (permalink / raw)
To: Saravana Kannan
Cc: MyungJoo Ham, Kyungmin Park, Chanwoo Choi, Mark Rutland,
Rajendra Nayak, Amit Kucheria, linux-pm, devicetree, linux-kernel
In-Reply-To: <1526629965-28729-1-git-send-email-skannan@codeaurora.org>
On Fri, May 18, 2018 at 12:52:40AM -0700, Saravana Kannan wrote:
> The firmware present in some QCOM chipsets offloads the steps necessary for
> changing the frequency of some devices (Eg: L3). This driver implements the
> devfreq interface for this firmware so that various governors could be used
> to scale the frequency of these devices.
>
> Each client (say cluster 0 and cluster 1) that wants to vote for a
> particular device's frequency (say, L3 frequency) is represented as a
> separate voter device (qcom,devfreq-fw-voter) that's a child of the
> firmware device (qcom,devfreq-fw).
>
> Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
> ---
> .../bindings/devfreq/devfreq-qcom-fw.txt | 41 +++
> drivers/devfreq/Kconfig | 14 +
> drivers/devfreq/Makefile | 1 +
> drivers/devfreq/devfreq_qcom_fw.c | 330 +++++++++++++++++++++
> 4 files changed, 386 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/devfreq/devfreq-qcom-fw.txt
> create mode 100644 drivers/devfreq/devfreq_qcom_fw.c
>
> diff --git a/Documentation/devicetree/bindings/devfreq/devfreq-qcom-fw.txt b/Documentation/devicetree/bindings/devfreq/devfreq-qcom-fw.txt
> new file mode 100644
> index 0000000..f882a0b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/devfreq/devfreq-qcom-fw.txt
> @@ -0,0 +1,41 @@
> +QCOM Devfreq firmware device
> +
> +Some Qualcomm Technologies, Inc. (QTI) chipsets have a firmware that
> +offloads the steps for frequency switching. It provides a table of
> +supported frequencies and a register to request one of the supported
> +freqencies.
> +
> +The qcom,devfreq-fw represents this firmware as a device. Sometimes,
> +multiple entities want to vote on the frequency request that is sent to the
> +firmware. The qcom,devfreq-fw-voter represents these voters as child
> +devices of the corresponding qcom,devfreq-fw device.
> +
> +Required properties:
> +- compatible: Must be "qcom,devfreq-fw" or "qcom,devfreq-fw-voter"
No versions of firmware?
> +Only for qcom,devfreq-fw:
> +- reg: Pairs of physical base addresses and region sizes of
> + memory mapped registers.
Registers? Is this firmware or h/w block?
> +- reg-names: Names of the bases for the above registers.
> + Required register regions are:
> + - "en-base": address of register to check if the
> + firmware is enabled.
> + - "ftbl-base": address region for the frequency
> + table.
> + - "perf-base": address of register to request a
> + frequency.
> +
> +Example:
> +
> + qcom,devfreq-l3 {
> + compatible = "qcom,devfreq-fw";
> + reg-names = "en-base", "ftbl-base", "perf-base";
> + reg = <0x18321000 0x4>, <0x18321110 0x600>, <0x18321920 0x4>;
> +
> + qcom,cpu0-l3 {
> + compatible = "qcom,devfreq-fw-voter";
There's no point in these nodes. They don't have any properties or
resources.
> + };
> +
> + qcom,cpu4-l3 {
> + compatible = "qcom,devfreq-fw-voter";
> + };
> + };
^ permalink raw reply
* Re: [PATCH v2 11/13] dt-bindings: power: add PX30 SoCs header for power-domain
From: Rob Herring @ 2018-05-22 17:39 UTC (permalink / raw)
To: Elaine Zhang
Cc: heiko, mark.rutland, devicetree, rjw, khilman, ulf.hansson,
linux-pm, linux-arm-kernel, linux-rockchip, linux-kernel, wxt,
xxx, xf, huangtao, Finley Xiao
In-Reply-To: <1526268724-25288-1-git-send-email-zhangqing@rock-chips.com>
On Mon, May 14, 2018 at 11:32:04AM +0800, Elaine Zhang wrote:
> From: Finley Xiao <finley.xiao@rock-chips.com>
>
> According to a description from TRM, add all the power domains.
>
> Signed-off-by: Finley Xiao <finley.xiao@rock-chips.com>
> Signed-off-by: Elaine Zhang <zhangqing@rock-chips.com>
> ---
> include/dt-bindings/power/px30-power.h | 32 ++++++++++++++++++++++++++++++++
> 1 file changed, 32 insertions(+)
> create mode 100644 include/dt-bindings/power/px30-power.h
>
> diff --git a/include/dt-bindings/power/px30-power.h b/include/dt-bindings/power/px30-power.h
> new file mode 100644
> index 000000000000..4ed482e80950
> --- /dev/null
> +++ b/include/dt-bindings/power/px30-power.h
> @@ -0,0 +1,32 @@
> +/*
> + * Copyright (c) 2017 Fuzhou Rockchip Electronics Co., Ltd
> + *
> + * SPDX-License-Identifier: (GPL-2.0+ OR MIT)
Same comments here.
> + */
> +
> +#ifndef __DT_BINDINGS_POWER_PX30_POWER_H__
> +#define __DT_BINDINGS_POWER_PX30_POWER_H__
> +
> +/* VD_CORE */
> +#define PX30_PD_A35_0 0
> +#define PX30_PD_A35_1 1
> +#define PX30_PD_A35_2 2
> +#define PX30_PD_A35_3 3
> +#define PX30_PD_SCU 4
> +
> +/* VD_LOGIC */
> +#define PX30_PD_USB 5
> +#define PX30_PD_DDR 6
> +#define PX30_PD_SDCARD 7
> +#define PX30_PD_CRYPTO 8
> +#define PX30_PD_GMAC 9
> +#define PX30_PD_MMC_NAND 10
> +#define PX30_PD_VPU 11
> +#define PX30_PD_VO 12
> +#define PX30_PD_VI 13
> +#define PX30_PD_GPU 14
> +
> +/* VD_PMU */
> +#define PX30_PD_PMU 15
> +
> +#endif
> --
> 1.9.1
>
>
^ permalink raw reply
* Re: [PATCH v2 09/13] dt-bindings: add binding for rk3228 power domains
From: Rob Herring @ 2018-05-22 17:38 UTC (permalink / raw)
To: Elaine Zhang
Cc: heiko, mark.rutland, devicetree, rjw, khilman, ulf.hansson,
linux-pm, linux-arm-kernel, linux-rockchip, linux-kernel, wxt,
xxx, xf, huangtao
In-Reply-To: <1526268684-22709-1-git-send-email-zhangqing@rock-chips.com>
On Mon, May 14, 2018 at 11:31:24AM +0800, Elaine Zhang wrote:
> Add binding documentation for the power domains
> found on Rockchip RK3228 SoCs.
>
> Signed-off-by: Elaine Zhang <zhangqing@rock-chips.com>
> ---
> Documentation/devicetree/bindings/soc/rockchip/power_domain.txt | 3 +++
> 1 file changed, 3 insertions(+)
Acked-by: Rob Herring <robh@kernel.org>
^ 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