* 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
* Re: [PATCH v2 08/13] dt-bindings: power: add RK3228 SoCs header for power-domain
From: Rob Herring @ 2018-05-22 17:37 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: <1526268668-20006-1-git-send-email-zhangqing@rock-chips.com>
On Mon, May 14, 2018 at 11:31:08AM +0800, Elaine Zhang wrote:
> According to a description from TRM, add all the power domains.
>
> Signed-off-by: Elaine Zhang <zhangqing@rock-chips.com>
> ---
> include/dt-bindings/power/rk3228-power.h | 26 ++++++++++++++++++++++++++
> 1 file changed, 26 insertions(+)
> create mode 100644 include/dt-bindings/power/rk3228-power.h
>
> diff --git a/include/dt-bindings/power/rk3228-power.h b/include/dt-bindings/power/rk3228-power.h
> new file mode 100644
> index 000000000000..fa1264d5a995
> --- /dev/null
> +++ b/include/dt-bindings/power/rk3228-power.h
> @@ -0,0 +1,26 @@
> +/*
> + * Copyright (c) 2018 Fuzhou Rockchip Electronics Co., Ltd
> + *
> + * SPDX-License-Identifier: (GPL-2.0+ OR MIT)
This goes on the first line. checkpatch.pl will tell you this.
The license here is different than all the others, was that the intent?
> + */
> +
> +#ifndef __DT_BINDINGS_POWER_RK3228_POWER_H__
> +#define __DT_BINDINGS_POWER_RK3228_POWER_H__
> +
> +/**
> + * RK3228 idle id Summary.
> + */
> +
> +#define RK3228_PD_CORE 0
> +#define RK3228_PD_MSCH 1
> +#define RK3228_PD_BUS 2
> +#define RK3228_PD_SYS 3
> +#define RK3228_PD_VIO 4
> +#define RK3228_PD_VOP 5
> +#define RK3228_PD_VPU 6
> +#define RK3228_PD_RKVDEC 7
> +#define RK3228_PD_GPU 8
> +#define RK3228_PD_PERI 9
> +#define RK3228_PD_GMAC 10
> +
> +#endif
> --
> 1.9.1
>
>
^ permalink raw reply
* Re: [PATCH v2 06/13] dt-bindings: add binding for rk3128 power domains
From: Rob Herring @ 2018-05-22 17:36 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: <1526268626-11855-1-git-send-email-zhangqing@rock-chips.com>
On Mon, May 14, 2018 at 11:30:26AM +0800, Elaine Zhang wrote:
> Add binding documentation for the power domains
> found on Rockchip RK3128 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
* Re: [PATCH v2 05/13] dt-bindings: power: add RK3128 SoCs header for power-domain
From: Rob Herring @ 2018-05-22 17:35 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: <1526268602-9760-1-git-send-email-zhangqing@rock-chips.com>
On Mon, May 14, 2018 at 11:30:02AM +0800, Elaine Zhang wrote:
> According to a description from TRM, add all the power domains.
>
> Signed-off-by: Elaine Zhang <zhangqing@rock-chips.com>
> ---
> include/dt-bindings/power/rk3128-power.h | 28 ++++++++++++++++++++++++++++
> 1 file changed, 28 insertions(+)
> create mode 100644 include/dt-bindings/power/rk3128-power.h
>
> diff --git a/include/dt-bindings/power/rk3128-power.h b/include/dt-bindings/power/rk3128-power.h
> new file mode 100644
> index 000000000000..26aef519cd94
> --- /dev/null
> +++ b/include/dt-bindings/power/rk3128-power.h
> @@ -0,0 +1,28 @@
> +/*
> + * Copyright (c) 2017 Rockchip Electronics Co. Ltd.
> + * Author: Elaine Zhang <zhangqing@rock-chips.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
Use SPDX tag instead, otherwise:
Reviewed-by: Rob Herring <robh@kernel.org>
^ permalink raw reply
* Re: [PATCH v2 02/13] dt-bindings: add binding for rk3036 power domains
From: Rob Herring @ 2018-05-22 17:34 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: <1526268528-9247-1-git-send-email-zhangqing@rock-chips.com>
On Mon, May 14, 2018 at 11:28:48AM +0800, Elaine Zhang wrote:
> From: Caesar Wang <wxt@rock-chips.com>
>
> Add binding documentation for the power domains
> found on Rockchip RK3036 SoCs.
>
> Signed-off-by: Caesar Wang <wxt@rock-chips.com>
> 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
* Re: [PATCH v2 01/13] dt-bindings: power: add RK3036 SoCs header for power-domain
From: Rob Herring @ 2018-05-22 17:34 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: <1526268501-9200-1-git-send-email-zhangqing@rock-chips.com>
On Mon, May 14, 2018 at 11:28:21AM +0800, Elaine Zhang wrote:
> From: Caesar Wang <wxt@rock-chips.com>
>
> According to a description from TRM, add all the power domains.
>
> Signed-off-by: Caesar Wang <wxt@rock-chips.com>
> Signed-off-by: Elaine Zhang <zhangqing@rock-chips.com>
> ---
> include/dt-bindings/power/rk3036-power.h | 27 +++++++++++++++++++++++++++
> 1 file changed, 27 insertions(+)
> create mode 100644 include/dt-bindings/power/rk3036-power.h
>
> diff --git a/include/dt-bindings/power/rk3036-power.h b/include/dt-bindings/power/rk3036-power.h
> new file mode 100644
> index 000000000000..59e09f1c5af7
> --- /dev/null
> +++ b/include/dt-bindings/power/rk3036-power.h
> @@ -0,0 +1,27 @@
> +/*
> + * Copyright (c) 2017 Rockchip Electronics Co. Ltd.
> + * Author: Caesar Wang <wxt@rock-chips.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
Use SPDX tag instead.
Otherwise,
Reviewed-by: Rob Herring <robh@kernel.org>
> + */
> +
> +#ifndef __DT_BINDINGS_POWER_RK3036_POWER_H__
> +#define __DT_BINDINGS_POWER_RK3036_POWER_H__
> +
> +#define RK3036_PD_MSCH 0
> +#define RK3036_PD_CORE 1
> +#define RK3036_PD_PERI 2
> +#define RK3036_PD_VIO 3
> +#define RK3036_PD_VPU 4
> +#define RK3036_PD_GPU 5
> +#define RK3036_PD_SYS 6
> +
> +#endif
> --
> 1.9.1
>
>
^ permalink raw reply
* Re: [PATCH v2] schedutil: Allow cpufreq requests to be made even when kthread kicked
From: Rafael J. Wysocki @ 2018-05-22 17:07 UTC (permalink / raw)
To: Joel Fernandes, Viresh Kumar
Cc: 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: <CAJZ5v0j99Lwj0hnVoCX3Unz8UvP-cP1rmggjdonTVSwsrZmasg@mail.gmail.com>
On Tue, May 22, 2018 at 5:30 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Tue, May 22, 2018 at 12:02 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
>> On Mon, May 21, 2018 at 6:13 PM, Joel Fernandes <joel@joelfernandes.org> wrote:
>>> On Mon, May 21, 2018 at 10:29:52AM +0200, Rafael J. Wysocki wrote:
>>>> On Mon, May 21, 2018 at 7:14 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>>>> > On 18-05-18, 11:55, Joel Fernandes (Google.) 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.
>>>> >
>>>> > Now that this isn't an RFC anymore, you shouldn't have added below
>>>> > paragraph here. It could go to the comments section though.
>>>> >
>>>> >> I had brought up this issue at the OSPM conference and Claudio had a
>>>> >> discussion RFC with an alternate approach [1]. I prefer the approach as
>>>> >> done in the patch below since it doesn't need any new flags and doesn't
>>>> >> cause any other extra overhead.
>>>> >>
>>>> >> [1] https://patchwork.kernel.org/patch/10384261/
>>>> >>
>>>> >> LGTMed-by: Viresh Kumar <viresh.kumar@linaro.org>
>>>> >> LGTMed-by: Juri Lelli <juri.lelli@redhat.com>
>>>> >
>>>> > Looks like a Tag you just invented ? :)
>>>>
>>>> Yeah.
>>>>
>>>> The LGTM from Juri can be converned into an ACK silently IMO. That
>>>> said I have added Looks-good-to: tags to a couple of commits. :-)
>>>
>>> Cool, I'll covert them to Acks :-)
>>
>> So it looks like I should expect an update of this patch, right?
>>
>> Or do you prefer the current one to be applied and work on top of it?
>>
>
> [cut]
>
>>>
>>> I just realized that on a single policy switch that uses the governor thread,
>>> there will be 1 thread per-CPU. The sugov_update_single will be called on the
>>> same CPU with interrupts disabled.
>>
>> sugov_update_single() doesn't have to run on the target CPU.
>
> Which sadly is a bug IMO. :-/
My bad.
sugov_update_single() runs under rq->lock, so it need not run on a
target CPU so long as the CPU running it can update the frequency for
the target and there is the requisite check for that in
sugov_should_update_freq().
That means that sugov_update_single() will not run concurrently on two
different CPUs for the same target, but it may be running concurrently
with the kthread (as pointed out by Viresh).
>>> In sugov_work, we are doing a
>>> raw_spin_lock_irqsave which also disables interrupts. So I don't think
>>> there's any possibility of a race happening on the same CPU between the
>>> frequency update request and the sugov_work executing. In other words, I feel
>>> we can drop the above if (..) statement for single policies completely and
>>> only keep the changes for the shared policy. Viresh since you brought up the
>>> single policy issue initially which made me add this if statememnt, could you
>>> let me know if you agree with what I just said?
>>
>> Which is why you need the spinlock too.
>
> And you totally have a point.
Not really, sorry about that.
It is necessary to take the spinlock in the non-fast-switch case,
because of the possible race with the kthread, so something like my
patch at https://patchwork.kernel.org/patch/10418551/ is needed after
all.
^ permalink raw reply
* Re: [PATCH 18/33] power: supply: use match_string() helper
From: Sebastian Reichel @ 2018-05-22 16:16 UTC (permalink / raw)
To: Andy Shevchenko; +Cc: Yisheng Xie, Linux Kernel Mailing List, Linux PM
In-Reply-To: <CAHp75VfnJQBdXiVuv9OOT6w=wD5rKDCJKA7M2iyxxmQndNhoRA@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 619 bytes --]
Hi Andy,
On Tue, May 22, 2018 at 12:58:14AM +0300, Andy Shevchenko wrote:
> On Mon, May 21, 2018 at 2:57 PM, Yisheng Xie <xieyisheng1@huawei.com> wrote:
> > match_string() returns the index of an array for a matching string,
> > which can be used intead of open coded variant.
> >
>
> This doesn't make code looks better anyhow. I even think it makes it
> worse to read. That's why I dropped my version of the change (and yes,
> I missed the type conversion which looks here just ugly).
>
> Sebastian, if my opinion makes any difference here, I would say
> NAK to this one.
I agree.
-- Sebastian
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 1/2] power: supply: Add fwnode pointer to power_supply_config struct
From: Sebastian Reichel @ 2018-05-22 16:15 UTC (permalink / raw)
To: Adam Thomson
Cc: Heikki Krogerus, Guenter Roeck, Greg Kroah-Hartman, linux-usb,
linux-pm, linux-kernel, support.opensource
In-Reply-To: <c398d1fcc81017b9cc694a00d2c1f655874dfeac.1527000797.git.Adam.Thomson.Opensource@diasemi.com>
[-- Attachment #1: Type: text/plain, Size: 2377 bytes --]
Hi,
On Tue, May 22, 2018 at 04:16:23PM +0100, Adam Thomson wrote:
> To allow users of the power supply framework to be hw description
> agnostic, this commit adds the ability to pass a fwnode pointer,
> via the power_supply_config structure, to the initialisation code
> of the core, instead of explicitly specifying of_ndoe. If that
> fwnode pointer is provided then it will automatically resolve down
> to of_node on platforms which support it, otherwise it will be NULL.
>
> In the future, when ACPI support is added, this can be modified to
> accommodate ACPI without the need to change calling code which
> already provides the fwnode handle in this manner.
>
> Signed-off-by: Adam Thomson <Adam.Thomson.Opensource@diasemi.com>
> Suggested-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> ---
Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
-- Sebastian
> drivers/power/supply/power_supply_core.c | 4 +++-
> include/linux/power_supply.h | 2 ++
> 2 files changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c
> index ecd68c2..f57ab0a 100644
> --- a/drivers/power/supply/power_supply_core.c
> +++ b/drivers/power/supply/power_supply_core.c
> @@ -19,6 +19,7 @@
> #include <linux/err.h>
> #include <linux/of.h>
> #include <linux/power_supply.h>
> +#include <linux/property.h>
> #include <linux/thermal.h>
> #include "power_supply.h"
>
> @@ -874,7 +875,8 @@ static void psy_unregister_cooler(struct power_supply *psy)
> psy->desc = desc;
> if (cfg) {
> psy->drv_data = cfg->drv_data;
> - psy->of_node = cfg->of_node;
> + psy->of_node =
> + cfg->fwnode ? to_of_node(cfg->fwnode) : cfg->of_node;
> psy->supplied_to = cfg->supplied_to;
> psy->num_supplicants = cfg->num_supplicants;
> }
> diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
> index 0c9a572..b21c4bd9 100644
> --- a/include/linux/power_supply.h
> +++ b/include/linux/power_supply.h
> @@ -199,6 +199,8 @@ enum power_supply_notifier_events {
> /* Run-time specific power supply configuration */
> struct power_supply_config {
> struct device_node *of_node;
> + struct fwnode_handle *fwnode;
> +
> /* Driver private data */
> void *drv_data;
>
> --
> 1.9.1
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 2/2] typec: tcpm: Provide fwnode pointer as part of psy_cfg
From: Guenter Roeck @ 2018-05-22 16:08 UTC (permalink / raw)
To: Adam Thomson
Cc: Heikki Krogerus, Greg Kroah-Hartman, Sebastian Reichel, linux-usb,
linux-pm, linux-kernel, support.opensource
In-Reply-To: <449707040719c8845cf9fa6bc779bae25fca54a5.1527000797.git.Adam.Thomson.Opensource@diasemi.com>
On Tue, May 22, 2018 at 04:16:24PM +0100, Adam Thomson wrote:
> For supply registration, provide fwnode pointer of the port device,
> via the power_supply_config structure, to allow other psy drivers
> to add us as a supplier. At present this only applies to DT
> based platforms using the 'power-supplies' DT property, but in the
> future should also work for ACPI platforms when the relevant support
> is added to the power_supply core.
>
> Signed-off-by: Adam Thomson <Adam.Thomson.Opensource@diasemi.com>
> Suggested-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
> ---
> drivers/usb/typec/tcpm.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c
> index 72996cc..0ccd2ce 100644
> --- a/drivers/usb/typec/tcpm.c
> +++ b/drivers/usb/typec/tcpm.c
> @@ -14,6 +14,7 @@
> #include <linux/mutex.h>
> #include <linux/power_supply.h>
> #include <linux/proc_fs.h>
> +#include <linux/property.h>
> #include <linux/sched/clock.h>
> #include <linux/seq_file.h>
> #include <linux/slab.h>
> @@ -4500,6 +4501,7 @@ static int devm_tcpm_psy_register(struct tcpm_port *port)
> char *psy_name;
>
> psy_cfg.drv_data = port;
> + psy_cfg.fwnode = dev_fwnode(port->dev);
> psy_name = devm_kzalloc(port->dev, psy_name_len, GFP_KERNEL);
> if (!psy_name)
> return -ENOMEM;
> --
> 1.9.1
>
^ permalink raw reply
* Re: [PATCH v9 11/15] dt-bindings: cpufreq: Document operating-points-v2-kryo-cpu
From: Rob Herring @ 2018-05-22 15:55 UTC (permalink / raw)
To: Ilia Lin
Cc: mturquette, sboyd, mark.rutland, viresh.kumar, nm, lgirdwood,
broonie, andy.gross, david.brown, catalin.marinas, will.deacon,
rjw, linux-clk, devicetree, linux-kernel, linux-pm, linux-arm-msm,
linux-soc, linux-arm-kernel, rnayak, amit.kucheria,
nicolas.dechesne, celster, tfinkel
In-Reply-To: <1526901932-9514-12-git-send-email-ilialin@codeaurora.org>
On Mon, May 21, 2018 at 02:25:28PM +0300, Ilia Lin wrote:
> 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>
> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
Please add acks/reviewed-bys when posting new versions.
Rob
^ permalink raw reply
* Re: [PATCH v2] schedutil: Allow cpufreq requests to be made even when kthread kicked
From: Rafael J. Wysocki @ 2018-05-22 15:30 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: <CAJZ5v0hucyBEyWoz8nS=6pfwt8Yd5Wi+39=2WGFgfJobv7=qPQ@mail.gmail.com>
On Tue, May 22, 2018 at 12:02 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Mon, May 21, 2018 at 6:13 PM, Joel Fernandes <joel@joelfernandes.org> wrote:
>> On Mon, May 21, 2018 at 10:29:52AM +0200, Rafael J. Wysocki wrote:
>>> On Mon, May 21, 2018 at 7:14 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>>> > On 18-05-18, 11:55, Joel Fernandes (Google.) 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.
>>> >
>>> > Now that this isn't an RFC anymore, you shouldn't have added below
>>> > paragraph here. It could go to the comments section though.
>>> >
>>> >> I had brought up this issue at the OSPM conference and Claudio had a
>>> >> discussion RFC with an alternate approach [1]. I prefer the approach as
>>> >> done in the patch below since it doesn't need any new flags and doesn't
>>> >> cause any other extra overhead.
>>> >>
>>> >> [1] https://patchwork.kernel.org/patch/10384261/
>>> >>
>>> >> LGTMed-by: Viresh Kumar <viresh.kumar@linaro.org>
>>> >> LGTMed-by: Juri Lelli <juri.lelli@redhat.com>
>>> >
>>> > Looks like a Tag you just invented ? :)
>>>
>>> Yeah.
>>>
>>> The LGTM from Juri can be converned into an ACK silently IMO. That
>>> said I have added Looks-good-to: tags to a couple of commits. :-)
>>
>> Cool, I'll covert them to Acks :-)
>
> So it looks like I should expect an update of this patch, right?
>
> Or do you prefer the current one to be applied and work on top of it?
>
[cut]
>>
>> I just realized that on a single policy switch that uses the governor thread,
>> there will be 1 thread per-CPU. The sugov_update_single will be called on the
>> same CPU with interrupts disabled.
>
> sugov_update_single() doesn't have to run on the target CPU.
Which sadly is a bug IMO. :-/
>> In sugov_work, we are doing a
>> raw_spin_lock_irqsave which also disables interrupts. So I don't think
>> there's any possibility of a race happening on the same CPU between the
>> frequency update request and the sugov_work executing. In other words, I feel
>> we can drop the above if (..) statement for single policies completely and
>> only keep the changes for the shared policy. Viresh since you brought up the
>> single policy issue initially which made me add this if statememnt, could you
>> let me know if you agree with what I just said?
>
> Which is why you need the spinlock too.
And you totally have a point. With the above bug fixed, disabling
interrupts should be sufficient to prevent concurrent updates from
occurring in the one-CPU policy case and the work_in_progress check in
sugov_update_single() isn't necessary.
^ permalink raw reply
* Re: [PATCH v2] schedutil: Allow cpufreq requests to be made even when kthread kicked
From: Rafael J. Wysocki @ 2018-05-22 15:27 UTC (permalink / raw)
To: Viresh Kumar, Joel Fernandes (Google)
Cc: Rafael J. Wysocki, 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: <4237890.zlzv5C60QP@aspire.rjw.lan>
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.
^ permalink raw reply
* Re: [PATCH] PCI / PM: Clean up outdated comments in pci_target_state()
From: Rafael J. Wysocki @ 2018-05-22 15:21 UTC (permalink / raw)
To: Bjorn Helgaas; +Cc: Rafael J. Wysocki, Linux PCI, Bjorn Helgaas, Linux PM, LKML
In-Reply-To: <20180522130612.GB161672@bhelgaas-glaptop.roam.corp.google.com>
On Tue, May 22, 2018 at 3:06 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> On Mon, May 21, 2018 at 01:11:12PM +0200, Rafael J. Wysocki wrote:
>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>
>> Two comments in pci_target_state() are outdated, as the function
>> doesn't set the target power state for the device any more, only
>> finds one for it, so fix them accordingly.
>>
>> Reported-by: Bjorn Helgaas <bhelgaas@google.com>
>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Acked-by: Bjorn Helgaas <bhelgaas@google.com>
>
> I assume you'll merge this; let me know if you'd rather that I take it.
I will take it, thanks!
Can you have a look at https://patchwork.kernel.org/patch/10408977/
too, please, while at it?
^ permalink raw reply
* [PATCH 2/2] typec: tcpm: Provide fwnode pointer as part of psy_cfg
From: Adam Thomson @ 2018-05-22 15:16 UTC (permalink / raw)
To: Heikki Krogerus, Guenter Roeck, Greg Kroah-Hartman,
Sebastian Reichel
Cc: linux-usb, linux-pm, linux-kernel, support.opensource
In-Reply-To: <cover.1527000797.git.Adam.Thomson.Opensource@diasemi.com>
For supply registration, provide fwnode pointer of the port device,
via the power_supply_config structure, to allow other psy drivers
to add us as a supplier. At present this only applies to DT
based platforms using the 'power-supplies' DT property, but in the
future should also work for ACPI platforms when the relevant support
is added to the power_supply core.
Signed-off-by: Adam Thomson <Adam.Thomson.Opensource@diasemi.com>
Suggested-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
drivers/usb/typec/tcpm.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c
index 72996cc..0ccd2ce 100644
--- a/drivers/usb/typec/tcpm.c
+++ b/drivers/usb/typec/tcpm.c
@@ -14,6 +14,7 @@
#include <linux/mutex.h>
#include <linux/power_supply.h>
#include <linux/proc_fs.h>
+#include <linux/property.h>
#include <linux/sched/clock.h>
#include <linux/seq_file.h>
#include <linux/slab.h>
@@ -4500,6 +4501,7 @@ static int devm_tcpm_psy_register(struct tcpm_port *port)
char *psy_name;
psy_cfg.drv_data = port;
+ psy_cfg.fwnode = dev_fwnode(port->dev);
psy_name = devm_kzalloc(port->dev, psy_name_len, GFP_KERNEL);
if (!psy_name)
return -ENOMEM;
--
1.9.1
^ permalink raw reply related
* [PATCH 1/2] power: supply: Add fwnode pointer to power_supply_config struct
From: Adam Thomson @ 2018-05-22 15:16 UTC (permalink / raw)
To: Heikki Krogerus, Guenter Roeck, Greg Kroah-Hartman,
Sebastian Reichel
Cc: linux-usb, linux-pm, linux-kernel, support.opensource
In-Reply-To: <cover.1527000797.git.Adam.Thomson.Opensource@diasemi.com>
To allow users of the power supply framework to be hw description
agnostic, this commit adds the ability to pass a fwnode pointer,
via the power_supply_config structure, to the initialisation code
of the core, instead of explicitly specifying of_ndoe. If that
fwnode pointer is provided then it will automatically resolve down
to of_node on platforms which support it, otherwise it will be NULL.
In the future, when ACPI support is added, this can be modified to
accommodate ACPI without the need to change calling code which
already provides the fwnode handle in this manner.
Signed-off-by: Adam Thomson <Adam.Thomson.Opensource@diasemi.com>
Suggested-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
drivers/power/supply/power_supply_core.c | 4 +++-
include/linux/power_supply.h | 2 ++
2 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c
index ecd68c2..f57ab0a 100644
--- a/drivers/power/supply/power_supply_core.c
+++ b/drivers/power/supply/power_supply_core.c
@@ -19,6 +19,7 @@
#include <linux/err.h>
#include <linux/of.h>
#include <linux/power_supply.h>
+#include <linux/property.h>
#include <linux/thermal.h>
#include "power_supply.h"
@@ -874,7 +875,8 @@ static void psy_unregister_cooler(struct power_supply *psy)
psy->desc = desc;
if (cfg) {
psy->drv_data = cfg->drv_data;
- psy->of_node = cfg->of_node;
+ psy->of_node =
+ cfg->fwnode ? to_of_node(cfg->fwnode) : cfg->of_node;
psy->supplied_to = cfg->supplied_to;
psy->num_supplicants = cfg->num_supplicants;
}
diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
index 0c9a572..b21c4bd9 100644
--- a/include/linux/power_supply.h
+++ b/include/linux/power_supply.h
@@ -199,6 +199,8 @@ enum power_supply_notifier_events {
/* Run-time specific power supply configuration */
struct power_supply_config {
struct device_node *of_node;
+ struct fwnode_handle *fwnode;
+
/* Driver private data */
void *drv_data;
--
1.9.1
^ permalink raw reply related
* [PATCH 0/2] typec: tcpm: Populate fwnode for use in power_supply core
From: Adam Thomson @ 2018-05-22 15:16 UTC (permalink / raw)
To: Heikki Krogerus, Guenter Roeck, Greg Kroah-Hartman,
Sebastian Reichel
Cc: linux-usb, linux-pm, linux-kernel, support.opensource
This patch set adds support for passing a fwnode pointer as part of the psy
config structure, when intialising the psy core, so that calling code can be
written to be agnostic of the HW description required for the platform. The TCPM
code is updated subsequently to make use of this new field.
For now the main FW support in the psy core is still just DT based but in the
future ACPI will likely be added and can use this field.
Adam Thomson (2):
power: supply: Add fwnode pointer to power_supply_config struct
typec: tcpm: Provide fwnode pointer as part of psy_cfg
drivers/power/supply/power_supply_core.c | 4 +++-
drivers/usb/typec/tcpm.c | 2 ++
include/linux/power_supply.h | 2 ++
3 files changed, 7 insertions(+), 1 deletion(-)
--
1.9.1
^ permalink raw reply
* Re: [PATCH 8/9] PM / Domains: Add support for multi PM domains per device to genpd
From: Ulf Hansson @ 2018-05-22 14:47 UTC (permalink / raw)
To: Jon Hunter
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: <f5589b88-acf8-b491-df79-e359e596c8d0@nvidia.com>
[...]
>>
>> +/**
>> + * 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.
>
> If so, I can see that that would work, but it does not seem to fit the RPM model
> where currently for something like device clocks, the RPM callbacks can handle
> all clocks at once.
>
> I was wondering why we could not add a list of genpd domains to the
> dev_pm_domain struct for the device? For example ...
See above answer, hopefully that explains it.
FYI, that's why I also discovered the bug around using device links
with runtime PM during probe.
https://patchwork.kernel.org/patch/10408825/
>
> diff --git a/include/linux/pm.h b/include/linux/pm.h
> index e723b78d8357..a11d6db3c077 100644
> --- a/include/linux/pm.h
> +++ b/include/linux/pm.h
> @@ -659,6 +659,7 @@ extern void dev_pm_put_subsys_data(struct device *dev);
> * subsystem-level and driver-level callbacks.
> */
> struct dev_pm_domain {
> + struct list_head genpd_list;
> struct dev_pm_ops ops;
> void (*detach)(struct device *dev, bool power_off);
> int (*activate)(struct device *dev);
> @@ -666,6 +667,11 @@ struct dev_pm_domain {
> void (*dismiss)(struct device *dev);
> };
>
> +struct dev_pm_domain_link {
> + struct generic_pm_domain *genpd;
> + struct list_head node;
> +};
> +
> /*
> * The PM_EVENT_ messages are also used by drivers implementing the legacy
> * suspend framework, based on the ->suspend() and ->resume() callbacks common
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index e61b5cd79fe2..019593804670 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -51,7 +51,6 @@ struct dev_pm_opp;
>
> struct generic_pm_domain {
> struct device dev;
> - struct dev_pm_domain domain; /* PM domain operations */
> struct list_head gpd_list_node; /* Node in the global PM domains list */
> struct list_head master_links; /* Links with PM domain as a master */
> struct list_head slave_links; /* Links with PM domain as a slave */
> @@ -99,11 +98,6 @@ struct generic_pm_domain {
>
> };
>
> -static inline struct generic_pm_domain *pd_to_genpd(struct dev_pm_domain *pd)
> -{
> - return container_of(pd, struct generic_pm_domain, domain);
> -}
> -
>
> Obviously the above will not compile but the intent would be to allocate a
> dev_pm_domain_link struct per power-domain that the device needs and add
> to the genpd_list for the device. It would be a much bigger change in
> having to iterate through all the power-domains when turning devices on
> and off, however, it would simplify the client driver.
>
> Cheers
> Jon
>
> --
> nvpublic
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-22 14:31 UTC (permalink / raw)
To: Ulf Hansson, Rafael J . Wysocki, linux-pm
Cc: Greg Kroah-Hartman, Geert Uytterhoeven, Todor Tomov,
Rajendra Nayak, Viresh Kumar, Vincent Guittot, Kevin Hilman,
linux-kernel, linux-arm-kernel, linux-tegra
In-Reply-To: <1526639490-12167-9-git-send-email-ulf.hansson@linaro.org>
Hi Ulf,
On 18/05/18 11:31, Ulf Hansson wrote:
> To support devices being partitioned across multiple PM domains, let's
> start by extending genpd to cope with these configurations.
>
> More precisely, add a new exported function, genpd_dev_pm_attach_by_id(),
> similar to genpd_dev_pm_attach(), but the new function also allows the
> caller to provide an index to what PM domain it wants to attach.
>
> Furthermore, let genpd register a new virtual struct device via calling
> device_register() and attach it to the corresponding PM domain, which is
> looked up via calling the existing genpd OF functions. Note that the new
> device is needed, because only one PM domain can be attached per device. At
> successful attachment, genpd_dev_pm_attach_by_id() returns the new device,
> allowing the caller to operate on it to deal with power management.
>
> To deal with detaching of a PM domain for multiple PM domain case, we can
> still re-use the existing genpd_dev_pm_detach() function, although we need
> to extend it to cover cleanup of the earlier registered device, via calling
> device_unregister().
>
> An important note, genpd_dev_pm_attach_by_id() shall only be called by the
> driver core / PM core, similar to how genpd_dev_pm_attach() is used.
> Following changes deploys this.
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
> drivers/base/power/domain.c | 79 +++++++++++++++++++++++++++++++++++++++++++++
> include/linux/pm_domain.h | 8 +++++
> 2 files changed, 87 insertions(+)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index d538640..ffeb6ea 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -2171,6 +2171,15 @@ struct generic_pm_domain *of_genpd_remove_last(struct device_node *np)
> }
> EXPORT_SYMBOL_GPL(of_genpd_remove_last);
>
> +static void genpd_release_dev(struct device *dev)
> +{
> + kfree(dev);
> +}
> +
> +static struct bus_type genpd_bus_type = {
> + .name = "genpd",
> +};
> +
> /**
> * genpd_dev_pm_detach - Detach a device from its PM domain.
> * @dev: Device to detach.
> @@ -2208,6 +2217,10 @@ static void genpd_dev_pm_detach(struct device *dev, bool power_off)
>
> /* Check if PM domain can be powered off after removing this device. */
> genpd_queue_power_off_work(pd);
> +
> + /* Unregister the device if it was created by genpd. */
> + if (dev->bus == &genpd_bus_type)
> + device_unregister(dev);
> }
>
> static void genpd_dev_pm_sync(struct device *dev)
> @@ -2298,6 +2311,66 @@ int genpd_dev_pm_attach(struct device *dev)
> }
> EXPORT_SYMBOL_GPL(genpd_dev_pm_attach);
>
> +/**
> + * 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?
If so, I can see that that would work, but it does not seem to fit the RPM model
where currently for something like device clocks, the RPM callbacks can handle
all clocks at once.
I was wondering why we could not add a list of genpd domains to the
dev_pm_domain struct for the device? For example ...
diff --git a/include/linux/pm.h b/include/linux/pm.h
index e723b78d8357..a11d6db3c077 100644
--- a/include/linux/pm.h
+++ b/include/linux/pm.h
@@ -659,6 +659,7 @@ extern void dev_pm_put_subsys_data(struct device *dev);
* subsystem-level and driver-level callbacks.
*/
struct dev_pm_domain {
+ struct list_head genpd_list;
struct dev_pm_ops ops;
void (*detach)(struct device *dev, bool power_off);
int (*activate)(struct device *dev);
@@ -666,6 +667,11 @@ struct dev_pm_domain {
void (*dismiss)(struct device *dev);
};
+struct dev_pm_domain_link {
+ struct generic_pm_domain *genpd;
+ struct list_head node;
+};
+
/*
* The PM_EVENT_ messages are also used by drivers implementing the legacy
* suspend framework, based on the ->suspend() and ->resume() callbacks common
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index e61b5cd79fe2..019593804670 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -51,7 +51,6 @@ struct dev_pm_opp;
struct generic_pm_domain {
struct device dev;
- struct dev_pm_domain domain; /* PM domain operations */
struct list_head gpd_list_node; /* Node in the global PM domains list */
struct list_head master_links; /* Links with PM domain as a master */
struct list_head slave_links; /* Links with PM domain as a slave */
@@ -99,11 +98,6 @@ struct generic_pm_domain {
};
-static inline struct generic_pm_domain *pd_to_genpd(struct dev_pm_domain *pd)
-{
- return container_of(pd, struct generic_pm_domain, domain);
-}
-
Obviously the above will not compile but the intent would be to allocate a
dev_pm_domain_link struct per power-domain that the device needs and add
to the genpd_list for the device. It would be a much bigger change in
having to iterate through all the power-domains when turning devices on
and off, however, it would simplify the client driver.
Cheers
Jon
--
nvpublic
^ permalink raw reply related
* Re: [PATCH v8 04/15] clk: qcom: Add CPU clock driver for msm8996
From: kbuild test robot @ 2018-05-22 13:49 UTC (permalink / raw)
Cc: kbuild-all, mturquette, sboyd, robh, mark.rutland, viresh.kumar,
nm, lgirdwood, broonie, andy.gross, david.brown, catalin.marinas,
will.deacon, rjw, linux-clk, devicetree, linux-kernel, linux-pm,
linux-arm-msm, linux-soc, linux-arm-kernel, rnayak, ilialin,
amit.kucheria, nicolas.dechesne, celster, tfinkel
In-Reply-To: <1526555955-29960-5-git-send-email-ilialin@codeaurora.org>
[-- Attachment #1: Type: text/plain, Size: 1647 bytes --]
Hi Ilia,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on robh/for-next]
[also build test ERROR on v4.17-rc6]
[cannot apply to clk/clk-next next-20180517]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Ilia-Lin/CPU-scaling-support-for-msm8996/20180520-132401
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
config: arm-allyesconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=arm
All errors (new ones prefixed by >>):
>> drivers/soc/qcom/kryo-l2-accessors.c:7:10: fatal error: asm/sysreg.h: No such file or directory
#include <asm/sysreg.h>
^~~~~~~~~~~~~~
compilation terminated.
vim +7 drivers/soc/qcom/kryo-l2-accessors.c
f4e14120 Ilia Lin 2018-05-17 @7 #include <asm/sysreg.h>
f4e14120 Ilia Lin 2018-05-17 8 #include <soc/qcom/kryo-l2-accessors.h>
f4e14120 Ilia Lin 2018-05-17 9
:::::: The code at line 7 was first introduced by commit
:::::: f4e14120d663ce6e4670516a66bc0158dc692c45 soc: qcom: Separate kryo l2 accessors from PMU driver
:::::: TO: Ilia Lin <ilialin@codeaurora.org>
:::::: CC: 0day robot <lkp@intel.com>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 64542 bytes --]
^ permalink raw reply
* Re: [PATCH V3] powercap/drivers/idle_injection: Add an idle injection framework
From: Daniel Lezcano @ 2018-05-22 13:42 UTC (permalink / raw)
To: Viresh Kumar
Cc: rjw, edubezval, kevin.wangtao, leo.yan, vincent.guittot,
linux-kernel, javi.merino, rui.zhang, linux-pm, daniel.thompson
In-Reply-To: <20180521103223.asejwsaf7pnrs777@vireshk-i7>
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 framework relies on the smpboot kthreads which handles via its
>> mainloop the common code for hotplugging and [un]parking.
>>
>> This code was previously tested with the cpu cooling device and went
>> through several iterations. It results now in split code and API
>> exported in the header file. It was tested with the cpu cooling device
>> with success.
>>
>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>> ---
>> V3:
>> - Fixed typos (Viresh Kumar)
>> - Removed extra blank line (Viresh Kumar)
>> - Added full stop (Viresh Kumar)
>> - Fixed Return kerneldoc format (Viresh Kumar)
>> - Fixed multiple kthreads initialization (Viresh Kumar)
>> - Fixed rollbacking the actions in the unregister function (Viresh Kumar)
>> - Make idle injection kthreads name hardcoded
>> - Kthreads are created in the initcall process
>>
>> V2: Fixed checkpatch warnings
>> ---
>> drivers/powercap/Kconfig | 10 ++
>> drivers/powercap/Makefile | 1 +
>> drivers/powercap/idle_injection.c | 326 ++++++++++++++++++++++++++++++++++++++
>> include/linux/idle_injection.h | 29 ++++
>> 4 files changed, 366 insertions(+)
>> create mode 100644 drivers/powercap/idle_injection.c
>> create mode 100644 include/linux/idle_injection.h
>>
>> diff --git a/drivers/powercap/Kconfig b/drivers/powercap/Kconfig
>> index 85727ef..a767ef2 100644
>> --- a/drivers/powercap/Kconfig
>> +++ b/drivers/powercap/Kconfig
>> @@ -29,4 +29,14 @@ config INTEL_RAPL
>> controller, CPU core (Power Plance 0), graphics uncore (Power Plane
>> 1), etc.
>>
>> +config IDLE_INJECTION
>> + bool "Idle injection framework"
>> + depends on CPU_IDLE
>> + default n
>> + help
>> + This enables support for the idle injection framework. It
>> + provides a way to force idle periods on a set of specified
>> + CPUs for power capping. Idle period can be injected
>> + synchronously on a set of specified CPUs or alternatively
>> + on a per CPU basis.
>> endif
>> diff --git a/drivers/powercap/Makefile b/drivers/powercap/Makefile
>> index 0a21ef3..c3bbfee 100644
>> --- a/drivers/powercap/Makefile
>> +++ b/drivers/powercap/Makefile
>> @@ -1,2 +1,3 @@
>> obj-$(CONFIG_POWERCAP) += powercap_sys.o
>> obj-$(CONFIG_INTEL_RAPL) += intel_rapl.o
>> +obj-$(CONFIG_IDLE_INJECTION) += idle_injection.o
>> diff --git a/drivers/powercap/idle_injection.c b/drivers/powercap/idle_injection.c
>> new file mode 100644
>> index 0000000..a5fe205
>> --- /dev/null
>> +++ b/drivers/powercap/idle_injection.c
>> @@ -0,0 +1,326 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * drivers/powercap/idle_injection.c
>> + *
>> + * Copyright 2018 Linaro Limited
>> + *
>> + * Author: Daniel Lezcano <daniel.lezcano@linaro.org>
>> + *
>> + * The idle injection framework proposes a way to force a cpu to enter
>> + * an idle state during a specified amount of time for a specified
>> + * period.
>> + *
>> + * It relies on the smpboot kthreads which handles, via its main loop,
>> + * the common code for hotplugging and [un]parking.
>> + *
>> + * At init time, all the kthreads are created and parked.
>> + *
>> + * A cpumask is specified as parameter for the idle injection
>> + * registering function. The kthreads will be synchronized regarding
>> + * this cpumask.
>> + *
>> + * The idle + run duration is specified via the helpers and then the
>> + * idle injection can be started at this point.
>> + *
>> + * A kthread will call play_idle() with the specified idle duration
>> + * from above and then will schedule itself. The latest CPU belonging
>> + * to the group is in charge of setting the timer for the next idle
>> + * injection deadline.
>> + *
>> + * The task handling the timer interrupt will wakeup all the kthreads
>> + * belonging to the cpumask.
>> + */
>> +#include <linux/cpu.h>
>> +#include <linux/freezer.h>
>> +#include <linux/hrtimer.h>
>> +#include <linux/sched.h>
>> +#include <linux/slab.h>
>> +#include <linux/smpboot.h>
>> +
>> +#include <uapi/linux/sched/types.h>
>> +
>> +/**
>> + * struct idle_injection_thread - task on/off switch structure
>> + * @tsk: a pointer to a task_struct injecting the idle cycles
>> + * @should_run: a integer used as a boolean by the smpboot kthread API
>> + */
>> +struct idle_injection_thread {
>> + struct task_struct *tsk;
>> + int should_run;
>> +};
>> +
>> +/**
>> + * struct idle_injection_device - data for the idle injection
>> + * @cpumask: a cpumask containing the list of CPUs managed by the device
>> + * @timer: a hrtimer giving the tempo for the idle injection
>> + * @count: an atomic to keep track of the last task exiting the idle cycle
>> + * @idle_duration_ms: an atomic specifying the idle duration
>> + * @run_duration_ms: an atomic specifying the running duration
>> + */
>> +struct idle_injection_device {
>> + cpumask_var_t cpumask;
>> + struct hrtimer timer;
>> + atomic_t count;
>> + atomic_t idle_duration_ms;
>> + atomic_t run_duration_ms;
>> +};
>> +
>> +static DEFINE_PER_CPU(struct idle_injection_thread, idle_injection_thread);
>> +static DEFINE_PER_CPU(struct idle_injection_device *, idle_injection_device);
>> +
>> +/**
>> + * idle_injection_wakeup - Wake up all idle injection threads
>> + * @ii_dev: the idle injection device
>> + *
>> + * Every idle injection task belonging to the idle injection device
>> + * and running on an online CPU will be wake up by this call.
>> + */
>> +static void idle_injection_wakeup(struct idle_injection_device *ii_dev)
>> +{
>> + struct idle_injection_thread *iit;
>> + int cpu;
>> +
>> + for_each_cpu_and(cpu, ii_dev->cpumask, cpu_online_mask) {
>> + iit = per_cpu_ptr(&idle_injection_thread, cpu);
>> + iit->should_run = 1;
>> + wake_up_process(iit->tsk);
>> + }
>> +}
>> +
>> +/**
>> + * idle_injection_wakeup_fn - idle injection timer callback
>> + * @timer: a hrtimer structure
>> + *
>> + * This function is called when the idle injection timer expires which
>> + * will wake up the idle injection tasks and these ones, in turn, play
>> + * idle a specified amount of time.
>> + *
>> + * Return: HRTIMER_NORESTART.
>> + */
>> +static enum hrtimer_restart idle_injection_wakeup_fn(struct hrtimer *timer)
>> +{
>> + struct idle_injection_device *ii_dev =
>> + container_of(timer, struct idle_injection_device, timer);
>> +
>> + idle_injection_wakeup(ii_dev);
>> +
>> + return HRTIMER_NORESTART;
>> +}
>> +
>> +/**
>> + * idle_injection_fn - idle injection routine
>> + * @cpu: the CPU number the tasks belongs to
>> + *
>> + * The idle injection routine will stay idle the specified amount of
>> + * time
>> + */
>> +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
>
> You never replied to my comment in previous posting where I suggested
> s/mainloop/main loop/ . Maybe my comment is wrong, but it needs to be
> Nak'd.
I've seen both but it seems the correct spelling is "main loop".
>> + * 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.
>> + 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.
>> +}
>> +
>> +/**
>> + * 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;
>> +
>> + 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.
>> +
>> + 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() ?
>> }
>> +
>> +/**
>> + * idle_injection_setup - initialize the current task as a RT task
>> + * @cpu: the CPU number where the kthread is running on (not used)
>> + *
>> + * Called one time, this function is in charge of setting the task
>> + * scheduler parameters.
>> + */
>> +static void idle_injection_setup(unsigned int cpu)
>> +{
>> + struct sched_param param = { .sched_priority = MAX_USER_RT_PRIO / 2 };
>> +
>> + set_freezable();
>> +
>> + sched_setscheduler(current, SCHED_FIFO, ¶m);
>> +}
>> +
>> +/**
>> + * idle_injection_should_run - function helper for the smpboot API
>> + * @cpu: the CPU number where the kthread is running on
>> + *
>> + * Return: a boolean telling if the thread can run.
>> + */
>> +static int idle_injection_should_run(unsigned int cpu)
>> +{
>> + struct idle_injection_thread *iit =
>> + per_cpu_ptr(&idle_injection_thread, cpu);
>> +
>> + return iit->should_run;
>> +}
>> +
>> +/**
>> + * idle_injection_register - idle injection init routine
>> + * @cpumask: the list of CPUs managed by the idle injection device
>> + *
>> + * This is the initialization function in charge of creating the
>> + * initializing the timer and allocate the structures. It does not
>> + * starts the idle injection cycles.
>> + *
>> + * Return: NULL if an allocation fails.
>> + */
>> +struct idle_injection_device *idle_injection_register(struct cpumask *cpumask)
>> +{
>> + struct idle_injection_device *ii_dev;
>> + int cpu;
>> +
>> + ii_dev = kzalloc(sizeof(*ii_dev), GFP_KERNEL);
>> + if (!ii_dev)
>> + return NULL;
>> +
>> + if (!alloc_cpumask_var(&ii_dev->cpumask, GFP_KERNEL))
>> + goto out_free_ii_dev;
>> + cpumask_copy(ii_dev->cpumask, cpumask);
>> +
>> + hrtimer_init(&ii_dev->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
>> +
>> + ii_dev->timer.function = idle_injection_wakeup_fn;
>> +
>> + for_each_cpu(cpu, cpumask)
>> + per_cpu(idle_injection_device, cpu) = ii_dev;
>
> Not sure but do we need protection against registration done twice for
> a CPU ?
Yeah, that would be safer.
>> +
>> + return ii_dev;
>> +
>> +out_free_ii_dev:
>> + kfree(ii_dev);
>> + return NULL;
>> +}
>> +
>> +/**
>> + * idle_injection_unregister - Unregister the idle injection device
>> + * @ii_dev: a pointer to an idle injection device
>> + *
>> + * The function is in charge of stopping the idle injections,
>> + * unregister the kthreads and free the allocated memory in the
>> + * register function.
>> + */
>> +void idle_injection_unregister(struct idle_injection_device *ii_dev)
>> +{
>> + int cpu;
>> +
>> + idle_injection_stop(ii_dev);
>> +
>> + for_each_cpu(cpu, ii_dev->cpumask)
>> + per_cpu(idle_injection_device, cpu) = NULL;
>> +
>> + kfree(ii_dev);
>> +}
>> +
>> +static struct smp_hotplug_thread idle_injection_threads = {
>> + .store = &idle_injection_thread.tsk,
>> + .setup = idle_injection_setup,
>> + .thread_fn = idle_injection_fn,
>> + .thread_comm = "idle_inject/%u",
>> + .thread_should_run = idle_injection_should_run,
>> +};
>> +
>> +static int __init idle_injection_init(void)
>> +{
>> + return smpboot_register_percpu_thread(&idle_injection_threads);
>> +}
>> +early_initcall(idle_injection_init);
>
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
^ permalink raw reply
* Re: [PATCH] cpufreq: Add Kryo CPU scaling driver
From: Sudeep Holla @ 2018-05-22 13:07 UTC (permalink / raw)
To: Ilia Lin
Cc: viresh.kumar, linux-clk, devicetree, linux-kernel, linux-pm,
linux-arm-msm, linux-soc, linux-arm-kernel, Sudeep Holla
In-Reply-To: <1526988585-21678-1-git-send-email-ilialin@codeaurora.org>
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 ?
I haven't seen the patches from Viresh yet, if that prevents getting NULL
or not.
--
Regards,
Sudeep
^ permalink raw reply
* Re: [PATCH] PCI / PM: Clean up outdated comments in pci_target_state()
From: Bjorn Helgaas @ 2018-05-22 13:06 UTC (permalink / raw)
To: Rafael J. Wysocki; +Cc: Linux PCI, Bjorn Helgaas, Linux PM, LKML
In-Reply-To: <2360046.ecez94sF1Y@aspire.rjw.lan>
On Mon, May 21, 2018 at 01:11:12PM +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Two comments in pci_target_state() are outdated, as the function
> doesn't set the target power state for the device any more, only
> finds one for it, so fix them accordingly.
>
> Reported-by: Bjorn Helgaas <bhelgaas@google.com>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Acked-by: Bjorn Helgaas <bhelgaas@google.com>
I assume you'll merge this; let me know if you'd rather that I take it.
> ---
> drivers/pci/pci.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> Index: linux-pm/drivers/pci/pci.c
> ===================================================================
> --- linux-pm.orig/drivers/pci/pci.c
> +++ linux-pm/drivers/pci/pci.c
> @@ -2025,8 +2025,7 @@ static pci_power_t pci_target_state(stru
>
> if (platform_pci_power_manageable(dev)) {
> /*
> - * Call the platform to choose the target state of the device
> - * and enable wake-up from this state if supported.
> + * Call the platform to find the target state for the device.
> */
> pci_power_t state = platform_pci_choose_state(dev);
>
> @@ -2059,8 +2058,7 @@ static pci_power_t pci_target_state(stru
> if (wakeup) {
> /*
> * Find the deepest state from which the device can generate
> - * wake-up events, make it the target state and enable device
> - * to generate PME#.
> + * PME#.
> */
> if (dev->pme_support) {
> while (target_state
>
^ permalink raw reply
* Re: [PATCH] PM / core: Fix direct_complete handling for devices with no callbacks
From: Johan Hovold @ 2018-05-22 12:37 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Linux PM, LKML, Thomas Martitz, Ulf Hansson, Lukas Wunner,
Greg Kroah-Hartman
In-Reply-To: <6238546.EnOBWIOf9o@aspire.rjw.lan>
On Tue, May 22, 2018 at 01:02:17PM +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Commit 08810a4119aa (PM / core: Add NEVER_SKIP and SMART_PREPARE
> driver flags) inadvertently prevented the power.direct_complete flag
> from being set for devices without PM callbacks and with disabled
> runtime PM which also prevents power.direct_complete from being set
> for their parents. That led to problems including a resume crash on
> HP ZBook 14u.
>
> Restore the previous behavior by causing power.direct_complete to be
> set for those devices again, but do that in a more direct way to
> avoid overlooking that case in the future.
>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=199693
> Fixes: 08810a4119aa (PM / core: Add NEVER_SKIP and SMART_PREPARE driver flags)
I stumbled over this the other day as well and tracked it down to the
above commit. In my case (child devices to serdev clients), this was
mostly benign although it did prevent the direct-complete optimisation
during suspend.
Never got around to reporting or fixing it myself, but your analysis and
fix matches my initial findings.
> Reported-by: Thomas Martitz <kugel@rockbox.org>
> Tested-by: Thomas Martitz <kugel@rockbox.org>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Reviewed-by: Johan Hovold <johan@kernel.org>
As already suggested elsewhere in the thread, I think a stable is
warranted too.
Thanks,
Johan
^ permalink raw reply
* Re: [PATCH v2] schedutil: Allow cpufreq requests to be made even when kthread kicked
From: Rafael J. Wysocki @ 2018-05-22 12:22 UTC (permalink / raw)
To: Viresh Kumar, Joel Fernandes (Google.)
Cc: Rafael J. Wysocki, 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: <CAJZ5v0hP+gCiLa+6YvKHaj7u-DuRfmVq_vjFRMj6CnVktJRxVg@mail.gmail.com>
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.
> OK, so the fast_switch thing needs to be left outside of the spinlock
> in the single case only. Fair enough.
That would be something like the patch below (again, compiled-only).
---
kernel/sched/cpufreq_schedutil.c | 67 +++++++++++++++++++++++++++------------
1 file changed, 47 insertions(+), 20 deletions(-)
Index: linux-pm/kernel/sched/cpufreq_schedutil.c
===================================================================
--- linux-pm.orig/kernel/sched/cpufreq_schedutil.c
+++ linux-pm/kernel/sched/cpufreq_schedutil.c
@@ -92,9 +92,6 @@ static bool sugov_should_update_freq(str
!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))
return true;
@@ -103,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;
+}
- policy->cur = next_freq;
- trace_cpu_frequency(next_freq, smp_processor_id());
- } else {
+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());
+}
+
+static void sugov_update_commit(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);
}
@@ -307,7 +320,13 @@ static void sugov_update_single(struct u
sg_policy->cached_raw_freq = 0;
}
- sugov_update_commit(sg_policy, time, next_f);
+ if (sg_policy->policy->fast_switch_enabled) {
+ sugov_fast_switch(sg_policy, time, next_f);
+ } else {
+ raw_spin_lock(&sg_policy->update_lock);
+ sugov_update_commit(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)
@@ -367,7 +386,10 @@ 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_update_commit(sg_policy, time, next_f);
}
raw_spin_unlock(&sg_policy->update_lock);
@@ -376,13 +398,18 @@ sugov_update_shared(struct update_util_d
static void sugov_work(struct kthread_work *work)
{
struct sugov_policy *sg_policy = container_of(work, struct sugov_policy, work);
+ unsigned int next_freq;
+ unsigned long flags;
+
+ raw_spin_lock_irqsave(&sg_policy->update_lock, flags);
+ next_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_driver_target(sg_policy->policy, next_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)
^ 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