* Re: [PATCH 2/6] thermal/drivers/cpu_cooling: Unregister with the policy
From: Daniel Lezcano @ 2019-06-24 7:45 UTC (permalink / raw)
To: Viresh Kumar
Cc: edubezval, linux-kernel, Sudeep Holla, Rafael J. Wysocki,
Amit Daniel Kachhap, Javi Merino, Zhang Rui, Shawn Guo,
Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
NXP Linux Team, Keerthy,
open list:CPU FREQUENCY DRIVERS - ARM BIG LITTLE,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
open list:TI BANDGAP AND THERMAL DRIVER
In-Reply-To: <20190624073747.hf7jd6ulkmebbxtm@vireshk-i7>
On 24/06/2019 09:37, Viresh Kumar wrote:
> On 24-06-19, 09:30, Daniel Lezcano wrote:
>> On 24/06/2019 08:03, Viresh Kumar wrote:
>>> On 21-06-19, 15:22, Daniel Lezcano wrote:
>>>> Currently the function cpufreq_cooling_register() returns a cooling
>>>> device pointer which is used back as a pointer to call the function
>>>> cpufreq_cooling_unregister(). Even if it is correct, it would make
>>>> sense to not leak the structure inside a cpufreq driver and keep the
>>>> code thermal code self-encapsulate. Moreover, that forces to add an
>>>> extra variable in each driver using this function.
>>>>
>>>> Instead of passing the cooling device to unregister, pass the policy.
>>>>
>>>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>>>> ---
>>>> drivers/cpufreq/arm_big_little.c | 2 +-
>>>> drivers/cpufreq/cpufreq.c | 2 +-
>>>> drivers/thermal/cpu_cooling.c | 18 ++++++++++--------
>>>> drivers/thermal/imx_thermal.c | 4 ++--
>>>> .../thermal/ti-soc-thermal/ti-thermal-common.c | 2 +-
>>>> include/linux/cpu_cooling.h | 6 +++---
>>>> 6 files changed, 18 insertions(+), 16 deletions(-)
>>>
>>> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
>>
>> Just a side note, does it make sense to have the function called from
>> imx_thermal.c and ti-thermal-common.c? Sounds like also a leakage from
>> cpufreq to thermal drivers, no?
>
> I am not sure what you are proposing here :)
Actually I'm asking your opinion :)
The structure in drivers/thermal/imx_thermal.c
struct imx_thermal_data {
struct cpufreq_policy *policy; <<<< in the thermal data ?!
[ ... ]
};
And then:
#ifdef CONFIG_CPU_FREQ
/*
* Create cooling device in case no #cooling-cells property is available in
* CPU node
*/
static int imx_thermal_register_legacy_cooling(struct imx_thermal_data
*data)
{
struct device_node *np;
int ret;
data->policy = cpufreq_cpu_get(0);
if (!data->policy) {
pr_debug("%s: CPUFreq policy not found\n", __func__);
return -EPROBE_DEFER;
}
np = of_get_cpu_node(data->policy->cpu, NULL);
if (!np || !of_find_property(np, "#cooling-cells", NULL)) {
data->cdev = cpufreq_cooling_register(data->policy);
if (IS_ERR(data->cdev)) {
ret = PTR_ERR(data->cdev);
cpufreq_cpu_put(data->policy);
return ret;
}
}
return 0;
}
[ ... ]
Shouldn't this be move in the drivers/cpufreq/<whatever driver> ?
--
<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 v2] PM / devfreq: Fix kernel oops on governor module load
From: MyungJoo Ham @ 2019-06-24 7:37 UTC (permalink / raw)
To: Kyungmin Park, Chanwoo Choi
Cc: kernel@collabora.com, linux-pm@vger.kernel.org,
Enric Balletbo i Serra, Ezequiel Garcia
In-Reply-To: <20190621213949.27018-1-ezequiel@collabora.com>
> A bit unexpectedly (but still documented), request_module may
> return a positive value, in case of a modprobe error.
> This is currently causing issues in the devfreq framework.
>
> When a request_module exits with a positive value, we currently
> return that via ERR_PTR. However, because the value is positive,
> it's not a ERR_VALUE proper, and is therefore treated as a
> valid struct devfreq_governor pointer, leading to a kernel oops.
>
> Fix this by returning -EINVAL if request_module returns a positive
> value.
>
> Fixes: b53b0128052ff ("PM / devfreq: Fix static checker warning in try_then_request_governor")
> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> ---
> Changes from v1:
> * Rework the fix as suggested by Enric and Chanwoo,
> handling the return vaue.
> ---
> drivers/devfreq/devfreq.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Acked-by: MyungJoo Ham <myungjoo.ham@samsung.com>
^ permalink raw reply
* Re: [PATCH 2/6] thermal/drivers/cpu_cooling: Unregister with the policy
From: Viresh Kumar @ 2019-06-24 7:37 UTC (permalink / raw)
To: Daniel Lezcano
Cc: edubezval, linux-kernel, Sudeep Holla, Rafael J. Wysocki,
Amit Daniel Kachhap, Javi Merino, Zhang Rui, Shawn Guo,
Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
NXP Linux Team, Keerthy,
open list:CPU FREQUENCY DRIVERS - ARM BIG LITTLE,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
open list:TI BANDGAP AND THERMAL DRIVER
In-Reply-To: <3f324189-aa1e-ae78-1d69-61e00c5d033a@linaro.org>
On 24-06-19, 09:30, Daniel Lezcano wrote:
> On 24/06/2019 08:03, Viresh Kumar wrote:
> > On 21-06-19, 15:22, Daniel Lezcano wrote:
> >> Currently the function cpufreq_cooling_register() returns a cooling
> >> device pointer which is used back as a pointer to call the function
> >> cpufreq_cooling_unregister(). Even if it is correct, it would make
> >> sense to not leak the structure inside a cpufreq driver and keep the
> >> code thermal code self-encapsulate. Moreover, that forces to add an
> >> extra variable in each driver using this function.
> >>
> >> Instead of passing the cooling device to unregister, pass the policy.
> >>
> >> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> >> ---
> >> drivers/cpufreq/arm_big_little.c | 2 +-
> >> drivers/cpufreq/cpufreq.c | 2 +-
> >> drivers/thermal/cpu_cooling.c | 18 ++++++++++--------
> >> drivers/thermal/imx_thermal.c | 4 ++--
> >> .../thermal/ti-soc-thermal/ti-thermal-common.c | 2 +-
> >> include/linux/cpu_cooling.h | 6 +++---
> >> 6 files changed, 18 insertions(+), 16 deletions(-)
> >
> > Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
>
> Just a side note, does it make sense to have the function called from
> imx_thermal.c and ti-thermal-common.c? Sounds like also a leakage from
> cpufreq to thermal drivers, no?
I am not sure what you are proposing here :)
--
viresh
^ permalink raw reply
* Re: Alternatives to /sys/kernel/debug/wakeup_sources
From: Greg Kroah-Hartman @ 2019-06-24 7:36 UTC (permalink / raw)
To: Tri Vo
Cc: Rafael J. Wysocki, Joel Fernandes, Rafael J. Wysocki,
Sandeep Patil, Viresh Kumar, Hridya Valsaraju, Linux PM,
Cc: Android Kernel, LKML
In-Reply-To: <CANA+-vCBW=P=dpJGfcKTt7SoNKzWcpP5pwZHSDMU6MkwBKoC9A@mail.gmail.com>
On Sun, Jun 23, 2019 at 06:48:43PM -0700, Tri Vo wrote:
> On Wed, Jun 19, 2019 at 1:35 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> >
> > On Wed, Jun 19, 2019 at 1:52 AM Joel Fernandes <joelaf@google.com> wrote:
> > >
> > > On Tue, Jun 18, 2019 at 7:15 PM Tri Vo <trong@android.com> wrote:
> > > [snip]
> > > > > > > >
> > > > > > > > Android userspace reading wakeup_sources is not ideal because:
> > > > > > > > - Debugfs API is not stable, i.e. Android tools built on top of it are
> > > > > > > > not guaranteed to be backward/forward compatible.
> > > > > > > > - This file requires debugfs to be mounted, which itself is
> > > > > > > > undesirable for security reasons.
> > > > > > > >
> > > > > > > > To address these problems, we want to contribute a way to expose these
> > > > > > > > statistics that doesn't depend on debugfs.
> > > > > > > >
> > > > > > > > Some initial thoughts/questions: Should we expose the stats in sysfs?
> > > > > > > > Or maybe implement eBPF-based solution? What do you think?
> > > > > >
> > > > > > We are going through Android's out-of-tree kernel dependencies along with
> > > > > > userspace APIs that are not necessarily considered "stable and forever
> > > > > > supported" upstream. The debugfs dependencies showed up on our radar as a
> > > > > > result and so we are wondering if we should worry about changes in debugfs
> > > > > > interface and hence the question(s) below.
> > > > > >
> > > > > > So, can we rely on /d/wakeup_sources to be considered a userspace API and
> > > > > > hence maintained stable as we do for other /proc and /sys entries?
> > > > > >
> > > > > > If yes, then we will go ahead and add tests for this in LTP or
> > > > > > somewhere else suitable.
> > > > >
> > > > > No, debugfs is not ABI.
> > > > >
> > > > > > If no, then we would love to hear suggestions for any changes that need to be
> > > > > > made or we simply just move the debugfs entry into somewhere like
> > > > > > /sys/power/ ?
> > > > >
> > > > > No, moving that entire file from debugfs into sysfs is not an option either.
> > > > >
> > > > > The statistics for the wakeup sources associated with devices are already there
> > > > > under /sys/devices/.../power/ , but I guess you want all wakeup sources?
> > > > >
> > > > > That would require adding a kobject to struct wakeup_source and exposing
> > > > > all of the statistics as separate attributes under it. In which case it would be
> > > > > good to replace the existing wakeup statistics under /sys/devices/.../power/
> > > > > with symbolic links to the attributes under the wakeup_source kobject.
> > > >
> > > > Thanks for your input, Rafael! Your suggestion makes sense. I'll work
> > > > on a patch for this.
> > >
> > > Does that entail making each wake up source, a new sysfs node under a
> > > particular device, and then adding stats under that new node?
> >
> > Not under a device, because there are wakeup source objects without
> > associated devices.
> >
> > It is conceivable to have a "wakeup_sources" directory under
> > /sys/power/ and sysfs nodes for all wakeup sources in there.
> >
> > Then, instead of exposing wakeup statistics directly under
> > /sys/devices/.../power/, there can be symbolic links from there to the
> > new wakeup source nodes under "wakeup_sources" (so as to avoid
> > exposing the same data in two different places in sysfs, which may be
> > confusing).
>
> This may be a dumb question. Is it appropriate to make symbolic links
> in sysfs from one attribute to another attribute? For example,
> /sys/devices/.../power/wakeup_count ->
> /sys/power/wakeup_sources/.../wakeup_count.
Why? would you want that?
> I only see kobject to kobject symlinks around. And I don't think we
> can make /sys/devices/.../power/ directory a symlink to where our new
> wakeup stats will be, since the former contains attributes other than
> wakeup ones.
No, don't link attributes, they refer to the kobject that created them.
I really doubt that this is the same kobject in both places :)
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH v4 13/16] PM / devfreq: tegra: Support Tegra30
From: MyungJoo Ham @ 2019-06-24 7:34 UTC (permalink / raw)
To: digetx@gmail.com
Cc: Chanwoo Choi, linux-pm@vger.kernel.org,
linux-tegra@vger.kernel.org, linux-kernel@vger.kernel.org,
jonathanh@nvidia.com, thierry.reding@gmail.com
In-Reply-To: <20190624065919epcms1p1a366de5f455f5138c438d1da8151c12f@epcms1p1>
>
>A question:
>
>Does this driver support Tegra20 as well?
>I'm asking this because ARCH_TEGRA includes ARCH_TEGRA_2x_SOC
>according to /drivers/soc/tegra/Kconfig.
>
For this matter, how about updating your 13/16 patch as follows?
---
drivers/devfreq/Kconfig | 4 ++--
drivers/devfreq/tegra-devfreq.c | 1 +
2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
index 7dd46d44579d..78c4b436aad8 100644
--- a/drivers/devfreq/Kconfig
+++ b/drivers/devfreq/Kconfig
@@ -93,8 +93,8 @@ config ARM_EXYNOS_BUS_DEVFREQ
This does not yet operate with optimal voltages.
config ARM_TEGRA_DEVFREQ
- tristate "Tegra DEVFREQ Driver"
- depends on ARCH_TEGRA_124_SOC
+ tristate "NVIDIA Tegra30/114/124/210 DEVFREQ Driver"
+ depends on ARCH_TEGRA_3x_SOC || ARCH_TEGRA_114_SOC || ARCH_TEGRA_124_SOC || ARCH_TEGRA_210_SOC
select PM_OPP
help
This adds the DEVFREQ driver for the Tegra family of SoCs.
diff --git a/drivers/devfreq/tegra-devfreq.c b/drivers/devfreq/tegra-devfreq.c
index 5cddf2199c4e..a6ba75f4106d 100644
--- a/drivers/devfreq/tegra-devfreq.c
+++ b/drivers/devfreq/tegra-devfreq.c
@@ -726,6 +726,7 @@ static int tegra_devfreq_remove(struct platform_device *pdev)
}
static const struct of_device_id tegra_devfreq_of_match[] = {
+ { .compatible = "nvidia,tegra30-actmon" },
{ .compatible = "nvidia,tegra124-actmon" },
{ },
};
--
2.17.1
^ permalink raw reply related
* RE: [EXT] Re: [PATCH] dt-bindings: thermal: Make cooling-maps property optional
From: Andy Tang @ 2019-06-24 7:32 UTC (permalink / raw)
To: Daniel Lezcano, rui.zhang@intel.com, edubezval@gmail.com
Cc: robh+dt@kernel.org, mark.rutland@arm.com,
linux-pm@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <f4675fc5-a7d1-1a6c-51ed-b6b6b599f5fe@linaro.org>
Hi Edubezval, Rui,
Are you going to pick up this patch?
BR,
Andy
> -----Original Message-----
> From: Daniel Lezcano <daniel.lezcano@linaro.org>
> Sent: 2019年6月4日 14:04
> To: Andy Tang <andy.tang@nxp.com>; rui.zhang@intel.com;
> edubezval@gmail.com
> Cc: robh+dt@kernel.org; mark.rutland@arm.com; linux-pm@vger.kernel.org;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: [EXT] Re: [PATCH] dt-bindings: thermal: Make cooling-maps property
> optional
>
> Caution: EXT Email
>
> On 04/06/2019 07:56, Andy Tang wrote:
> > Hi Edubezval, Rui,
> >
> > Any further comments?
>
> From my POV, this patch makes sense. We may be interested to show up the
> thermal zones in sysfs and optionally mitigate them via an userspace
> governor.
>
> Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>
> >> -----Original Message-----
> >> From: Yuantian Tang <andy.tang@nxp.com>
> >> Sent: 2019年5月15日 17:37
> >> To: rui.zhang@intel.com; edubezval@gmail.com
> >> Cc: robh+dt@kernel.org; daniel.lezcano@linaro.org;
> >> mark.rutland@arm.com; linux-pm@vger.kernel.org;
> >> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Andy Tang
> >> <andy.tang@nxp.com>
> >> Subject: [PATCH] dt-bindings: thermal: Make cooling-maps property
> >> optional
> >>
> >> There may be no cooling device on system, or there are no enough
> >> cooling devices for each thermal zone in multiple thermal zone cases
> >> since cooling devices can't be shared.
> >> So make this property optional to remove such limitations.
> >>
> >> Signed-off-by: Yuantian Tang <andy.tang@nxp.com>
> >> ---
> >> .../devicetree/bindings/thermal/thermal.txt | 4 ++--
> >> 1 files changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/Documentation/devicetree/bindings/thermal/thermal.txt
> >> b/Documentation/devicetree/bindings/thermal/thermal.txt
> >> index ca14ba9..694e834 100644
> >> --- a/Documentation/devicetree/bindings/thermal/thermal.txt
> >> +++ b/Documentation/devicetree/bindings/thermal/thermal.txt
> >> @@ -142,11 +142,11 @@ Required properties:
> >> - trips: A sub-node which is a container of only trip point
> nodes
> >> Type: sub-node required to describe the thermal zone.
> >>
> >> +
> >> +Optional property:
> >> - cooling-maps: A sub-node which is a container of only
> cooling device
> >> Type: sub-node map nodes, used to describe the relation
> between
> >> trips
> >> and cooling devices.
> >> -
> >> -Optional property:
> >> - coefficients: An array of integers (one signed cell)
> containing
> >> Type: array coefficients to compose a linear relation
> between
> >> Elem size: one cell the sensors listed in the thermal-sensors
> property.
> >> --
> >> 1.7.1
> >
>
>
> --
>
> <https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.
> linaro.org%2F&data=02%7C01%7Candy.tang%40nxp.com%7C2287a987
> 116c4240ed8408d6e8b27b81%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C
> 0%7C0%7C636952250606346540&sdata=D8XH3corlkEU91u7FguNtCF8
> hkzR8viNntDQ4NjF9OM%3D&reserved=0> Linaro.org │ Open source
> software for ARM SoCs
>
> Follow Linaro:
> <https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.
> facebook.com%2Fpages%2FLinaro&data=02%7C01%7Candy.tang%40nx
> p.com%7C2287a987116c4240ed8408d6e8b27b81%7C686ea1d3bc2b4c6fa92
> cd99c5c301635%7C0%7C0%7C636952250606356528&sdata=hRuvmW
> %2BrQ61tDEy7XIHKhTaTqxL%2FJC6gRy29YgsEazk%3D&reserved=0>
> Facebook |
> <https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Ftwitte
> r.com%2F%23!%2Flinaroorg&data=02%7C01%7Candy.tang%40nxp.com
> %7C2287a987116c4240ed8408d6e8b27b81%7C686ea1d3bc2b4c6fa92cd99c
> 5c301635%7C0%7C0%7C636952250606356528&sdata=j6yO88tkG0uQs
> ViSV%2F9SCOYLa3iJ6pXs8mqxs6qt4uA%3D&reserved=0> Twitter |
> <https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.
> linaro.org%2Flinaro-blog%2F&data=02%7C01%7Candy.tang%40nxp.co
> m%7C2287a987116c4240ed8408d6e8b27b81%7C686ea1d3bc2b4c6fa92cd99
> c5c301635%7C0%7C0%7C636952250606356528&sdata=q9tOF%2FiOL6
> GpdvpSiT%2FyFIFL33Dx0%2FuBBaOxcmdD1Ro%3D&reserved=0> Blog
^ permalink raw reply
* Re: [PATCH 1/2] thermal/drivers/core: Add init section table for self-encapsulation
From: Daniel Lezcano @ 2019-06-24 7:32 UTC (permalink / raw)
To: rui.zhang; +Cc: linux-kernel, linux-pm, amit.kucheria
In-Reply-To: <20190612201325.1150-1-daniel.lezcano@linaro.org>
Any chance this patch gets merged for v5.4?
Thanks
-- Daniel
On 12/06/2019 22:13, Daniel Lezcano wrote:
> Currently the governors are declared in their respective files but they
> export their [un]register functions which in turn call the [un]register
> governors core's functions. That implies a cyclic dependency which is
> not desirable. There is a way to self-encapsulate the governors by letting
> them to declare themselves in a __init section table.
>
> Define the table in the asm generic linker description like the other
> tables and provide the specific macros to deal with.
>
> Reviewed-by: Amit Kucheria <amit.kucheria@linaro.org>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
> drivers/thermal/thermal_core.h | 15 +++++++++++++++
> include/asm-generic/vmlinux.lds.h | 11 +++++++++++
> 2 files changed, 26 insertions(+)
>
> diff --git a/drivers/thermal/thermal_core.h b/drivers/thermal/thermal_core.h
> index 0df190ed82a7..be901e84aa65 100644
> --- a/drivers/thermal/thermal_core.h
> +++ b/drivers/thermal/thermal_core.h
> @@ -15,6 +15,21 @@
> /* Initial state of a cooling device during binding */
> #define THERMAL_NO_TARGET -1UL
>
> +/* Init section thermal table */
> +extern struct thermal_governor *__governor_thermal_table[];
> +extern struct thermal_governor *__governor_thermal_table_end[];
> +
> +#define THERMAL_TABLE_ENTRY(table, name) \
> + (static typeof(name) *__thermal_table_entry_##name \
> + __used __section(__##table##_thermal_table) = &name)
> +
> +#define THERMAL_GOVERNOR_DECLARE(name) THERMAL_TABLE_ENTRY(governor, name)
> +
> +#define for_each_governor_table(__governor) \
> + for (__governor = __governor_thermal_table; \
> + __governor < __governor_thermal_table_end; \
> + __governor++)
> +
> /*
> * This structure is used to describe the behavior of
> * a certain cooling device on a certain trip point
> diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
> index f8f6f04c4453..8312fdc2b2fa 100644
> --- a/include/asm-generic/vmlinux.lds.h
> +++ b/include/asm-generic/vmlinux.lds.h
> @@ -239,6 +239,16 @@
> #define ACPI_PROBE_TABLE(name)
> #endif
>
> +#ifdef CONFIG_THERMAL
> +#define THERMAL_TABLE(name) \
> + . = ALIGN(8); \
> + __##name##_thermal_table = .; \
> + KEEP(*(__##name##_thermal_table)) \
> + __##name##_thermal_table_end = .;
> +#else
> +#define THERMAL_TABLE(name)
> +#endif
> +
> #define KERNEL_DTB() \
> STRUCT_ALIGN(); \
> __dtb_start = .; \
> @@ -609,6 +619,7 @@
> IRQCHIP_OF_MATCH_TABLE() \
> ACPI_PROBE_TABLE(irqchip) \
> ACPI_PROBE_TABLE(timer) \
> + THERMAL_TABLE(governor) \
> EARLYCON_TABLE() \
> LSM_TABLE()
>
>
--
<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 2/6] thermal/drivers/cpu_cooling: Unregister with the policy
From: Daniel Lezcano @ 2019-06-24 7:30 UTC (permalink / raw)
To: Viresh Kumar
Cc: edubezval, linux-kernel, Sudeep Holla, Rafael J. Wysocki,
Amit Daniel Kachhap, Javi Merino, Zhang Rui, Shawn Guo,
Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
NXP Linux Team, Keerthy,
open list:CPU FREQUENCY DRIVERS - ARM BIG LITTLE,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
open list:TI BANDGAP AND THERMAL DRIVER
In-Reply-To: <20190624060334.kak2mjuou4napi4x@vireshk-i7>
On 24/06/2019 08:03, Viresh Kumar wrote:
> On 21-06-19, 15:22, Daniel Lezcano wrote:
>> Currently the function cpufreq_cooling_register() returns a cooling
>> device pointer which is used back as a pointer to call the function
>> cpufreq_cooling_unregister(). Even if it is correct, it would make
>> sense to not leak the structure inside a cpufreq driver and keep the
>> code thermal code self-encapsulate. Moreover, that forces to add an
>> extra variable in each driver using this function.
>>
>> Instead of passing the cooling device to unregister, pass the policy.
>>
>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>> ---
>> drivers/cpufreq/arm_big_little.c | 2 +-
>> drivers/cpufreq/cpufreq.c | 2 +-
>> drivers/thermal/cpu_cooling.c | 18 ++++++++++--------
>> drivers/thermal/imx_thermal.c | 4 ++--
>> .../thermal/ti-soc-thermal/ti-thermal-common.c | 2 +-
>> include/linux/cpu_cooling.h | 6 +++---
>> 6 files changed, 18 insertions(+), 16 deletions(-)
>
> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
Just a side note, does it make sense to have the function called from
imx_thermal.c and ti-thermal-common.c? Sounds like also a leakage from
cpufreq to thermal drivers, no?
--
<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 v4 16/16] PM / devfreq: Introduce driver for NVIDIA Tegra20
From: MyungJoo Ham @ 2019-06-24 7:24 UTC (permalink / raw)
To: digetx@gmail.com, thierry.reding@gmail.com, jonathanh@nvidia.com,
Chanwoo Choi, linux-pm@vger.kernel.org,
linux-tegra@vger.kernel.org
In-Reply-To: <CGME20190624072431epcms1p3bdfd41545e7daecb1f6472c1e6f9dcfc@epcms1p3>
> Add devfreq driver for NVIDIA Tegra20 SoC's. The driver periodically
> reads out Memory Controller counters and adjusts memory frequency based
> on the memory clients activity.
>
> Reviewed-by: Chanwoo Choi
> Signed-off-by: Dmitry Osipenko
Could you please send a separate commit for MAINTAINERS?
I can add Ack to it, but I don't feel comfortable with sending
a pull-request with MAINTAINERS entry updates.
Cheers,
MyungJoo
> ---
> MAINTAINERS | 8 ++
> drivers/devfreq/Kconfig | 10 ++
> drivers/devfreq/Makefile | 1 +
> drivers/devfreq/tegra20-devfreq.c | 212 ++++++++++++++++++++++++++++++
> 4 files changed, 231 insertions(+)
> create mode 100644 drivers/devfreq/tegra20-devfreq.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 98edc38bfd7b..e7e434f74038 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -10098,6 +10098,14 @@ F: include/linux/memblock.h
> F: mm/memblock.c
> F: Documentation/core-api/boot-time-mm.rst
>
> +MEMORY FREQUENCY SCALING DRIVER FOR NVIDIA TEGRA20
> +M: Dmitry Osipenko
> +L: linux-pm@vger.kernel.org
> +L: linux-tegra@vger.kernel.org
> +T: git git://git.kernel.org/pub/scm/linux/kernel/git/mzx/devfreq.git
> +S: Maintained
> +F: drivers/devfreq/tegra20-devfreq.c
> +
> MEMORY MANAGEMENT
> L: linux-mm@kvack.org
> W: http://www.linux-mm.org
^ permalink raw reply
* Re: [PATCH v1] OPP: Fix crashing when current OPP has unsupportable voltage
From: Viresh Kumar @ 2019-06-24 7:18 UTC (permalink / raw)
To: Dmitry Osipenko
Cc: Viresh Kumar, Nishanth Menon, Stephen Boyd, Marc Dietrich,
linux-pm, linux-tegra, linux-kernel
In-Reply-To: <20190623175053.26167-1-digetx@gmail.com>
On 23-06-19, 20:50, Dmitry Osipenko wrote:
> Fix NULL dereference caused by a typo in the code. In particular it
> happens when CPU is running on a frequency which has unsupportable voltage
> (by regulator) defined in the OPP table and a custom set_opp() callback is
> being used. The problem was spotted during of testing of upcoming update
> for the NVIDIA Tegra CPUFreq driver.
>
> Cc: stable <stable@vger.kernel.org>
> Fixes: 7e535993fa4f ("OPP: Separate out custom OPP handler specific code")
> Reported-by: Marc Dietrich <marvin24@gmx.de>
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
> drivers/opp/core.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index 9fda9a0ec016..89ec6aa220cf 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -685,7 +685,7 @@ static int _set_opp_custom(const struct opp_table *opp_table,
>
> data->old_opp.rate = old_freq;
> size = sizeof(*old_supply) * opp_table->regulator_count;
> - if (IS_ERR(old_supply))
> + if (!old_supply)
> memset(data->old_opp.supplies, 0, size);
> else
> memcpy(data->old_opp.supplies, old_supply, size);
While the change is fine, the commit log isn't. It isn't about
unsupportable voltage but frequency. The frequency the CPU is
currently running at, is not present in the OPP table and so there is
no corresponding OPP, hence no voltage supplies.
I have applied this patch with following change log:
commit 560d1bcad715c215e7ffe5d7cffe045974b623d0 (HEAD -> opp/linux-next)
Author: Dmitry Osipenko <digetx@gmail.com>
Date: Sun Jun 23 20:50:53 2019 +0300
opp: Don't use IS_ERR on invalid supplies
_set_opp_custom() receives a set of OPP supplies as its arguments and
the caller of it passes NULL when the supplies are not valid. But
_set_opp_custom(), by mistake, checks for error by performing
IS_ERR(old_supply) on it which will always evaluate to false.
The problem was spotted during of testing of upcoming update for the
NVIDIA Tegra CPUFreq driver.
Cc: stable <stable@vger.kernel.org>
Fixes: 7e535993fa4f ("OPP: Separate out custom OPP handler specific code")
Reported-by: Marc Dietrich <marvin24@gmx.de>
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
[ Viresh: Massaged changelog ]
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
drivers/opp/core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
--
viresh
^ permalink raw reply
* Re: [PATCH v4 13/16] PM / devfreq: tegra: Support Tegra30
From: MyungJoo Ham @ 2019-06-24 6:59 UTC (permalink / raw)
To: digetx@gmail.com, janathanh@nvidia.com, Chanwoo Choi
Cc: linux-pm@vger.kernel.org, linux-tegra@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <CGME20190624065919epcms1p1a366de5f455f5138c438d1da8151c12f@epcms1p1>
> On Thu, May 02, 2019 at 02:38:12AM +0300, Dmitry Osipenko wrote:
> > The devfreq driver can be used on Tegra30 without any code change and
> > it works perfectly fine, the default Tegra124 parameters are good enough
> > for Tegra30.
> >
> > Reviewed-by: Chanwoo Choi
> > Signed-off-by: Dmitry Osipenko
> > ---
> > drivers/devfreq/Kconfig | 4 ++--
> > drivers/devfreq/tegra-devfreq.c | 1 +
> > 2 files changed, 3 insertions(+), 2 deletions(-)
>
> Acked-by: Thierry Reding
>
> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
> index 7dd46d4..b291803 100644
> --- a/drivers/devfreq/Kconfig
> +++ b/drivers/devfreq/Kconfig
> @@ -93,8 +93,8 @@ config ARM_EXYNOS_BUS_DEVFREQ
> This does not yet operate with optimal voltages.
>
> config ARM_TEGRA_DEVFREQ
> - tristate "Tegra DEVFREQ Driver"
> - depends on ARCH_TEGRA_124_SOC
> + tristate "NVIDIA Tegra30/114/124/210 DEVFREQ Driver"
> + depends on ARCH_TEGRA
> select PM_OPP
> help
> This adds the DEVFREQ driver for the Tegra family of SoCs.
A question:
Does this driver support Tegra20 as well?
I'm asking this because ARCH_TEGRA includes ARCH_TEGRA_2x_SOC
according to /drivers/soc/tegra/Kconfig.
Cheers,
MyungJoo.
^ permalink raw reply
* Re: [PATCH 6/6] thermal/drivers/ti: Remove cooling device usage
From: Viresh Kumar @ 2019-06-24 6:06 UTC (permalink / raw)
To: Daniel Lezcano
Cc: edubezval, linux-kernel, Keerthy, Zhang Rui,
open list:TI BANDGAP AND THERMAL DRIVER,
open list:TI BANDGAP AND THERMAL DRIVER
In-Reply-To: <20190621132302.30414-6-daniel.lezcano@linaro.org>
On 21-06-19, 15:23, Daniel Lezcano wrote:
> The cpufreq_cooling_unregister() function uses now the policy to
> unregister itself. The only purpose of the cooling device pointer is
> to unregister the cpu cooling device.
>
> As there is no more need of this pointer, remove it.
>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
> drivers/thermal/ti-soc-thermal/ti-thermal-common.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/thermal/ti-soc-thermal/ti-thermal-common.c b/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
> index 217b1aae8b4f..170b70b6ec61 100644
> --- a/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
> +++ b/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
> @@ -41,7 +41,6 @@ struct ti_thermal_data {
> struct cpufreq_policy *policy;
> struct thermal_zone_device *ti_thermal;
> struct thermal_zone_device *pcb_tz;
> - struct thermal_cooling_device *cool_dev;
> struct ti_bandgap *bgp;
> enum thermal_device_mode mode;
> struct work_struct thermal_wq;
> @@ -233,6 +232,7 @@ int ti_thermal_register_cpu_cooling(struct ti_bandgap *bgp, int id)
> {
> struct ti_thermal_data *data;
> struct device_node *np = bgp->dev->of_node;
> + struct thermal_cooling_device *cdev;
>
> /*
> * We are assuming here that if one deploys the zone
> @@ -256,9 +256,9 @@ int ti_thermal_register_cpu_cooling(struct ti_bandgap *bgp, int id)
> }
>
> /* Register cooling device */
> - data->cool_dev = cpufreq_cooling_register(data->policy);
> - if (IS_ERR(data->cool_dev)) {
> - int ret = PTR_ERR(data->cool_dev);
> + cdev = cpufreq_cooling_register(data->policy);
> + if (IS_ERR(cdev)) {
> + int ret = PTR_ERR(cdev);
> dev_err(bgp->dev, "Failed to register cpu cooling device %d\n",
> ret);
> cpufreq_cpu_put(data->policy);
And this too..
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
--
viresh
^ permalink raw reply
* Re: [PATCH 5/6] thermal/drivers/imx: Remove cooling device usage
From: Viresh Kumar @ 2019-06-24 6:06 UTC (permalink / raw)
To: Daniel Lezcano
Cc: edubezval, linux-kernel, Zhang Rui, Shawn Guo, Sascha Hauer,
Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team,
open list:THERMAL,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE
In-Reply-To: <20190621132302.30414-5-daniel.lezcano@linaro.org>
On 21-06-19, 15:23, Daniel Lezcano wrote:
> The cpufreq_cooling_unregister() function uses now the policy to
> unregister itself. The only purpose of the cooling device pointer is
> to unregister the cpu cooling device.
>
> As there is no more need of this pointer, remove it.
>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
> drivers/thermal/imx_thermal.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c
> index 6746f1b73eb7..021c0948b740 100644
> --- a/drivers/thermal/imx_thermal.c
> +++ b/drivers/thermal/imx_thermal.c
> @@ -203,7 +203,6 @@ static struct thermal_soc_data thermal_imx7d_data = {
> struct imx_thermal_data {
> struct cpufreq_policy *policy;
> struct thermal_zone_device *tz;
> - struct thermal_cooling_device *cdev;
> enum thermal_device_mode mode;
> struct regmap *tempmon;
> u32 c1, c2; /* See formula in imx_init_calib() */
> @@ -656,6 +655,7 @@ MODULE_DEVICE_TABLE(of, of_imx_thermal_match);
> static int imx_thermal_register_legacy_cooling(struct imx_thermal_data *data)
> {
> struct device_node *np;
> + struct thermal_cooling_device *cdev;
> int ret;
>
> data->policy = cpufreq_cpu_get(0);
> @@ -667,9 +667,9 @@ static int imx_thermal_register_legacy_cooling(struct imx_thermal_data *data)
> np = of_get_cpu_node(data->policy->cpu, NULL);
>
> if (!np || !of_find_property(np, "#cooling-cells", NULL)) {
> - data->cdev = cpufreq_cooling_register(data->policy);
> - if (IS_ERR(data->cdev)) {
> - ret = PTR_ERR(data->cdev);
> + cdev = cpufreq_cooling_register(data->policy);
> + if (IS_ERR(cdev)) {
> + ret = PTR_ERR(cdev);
> cpufreq_cpu_put(data->policy);
> return ret;
> }
This too..
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
--
viresh
^ permalink raw reply
* Re: [PATCH 4/6] cpufreq: Remove cooling device usage
From: Viresh Kumar @ 2019-06-24 6:05 UTC (permalink / raw)
To: Daniel Lezcano
Cc: edubezval, linux-kernel, Rafael J. Wysocki,
open list:CPU FREQUENCY SCALING FRAMEWORK
In-Reply-To: <20190621132302.30414-4-daniel.lezcano@linaro.org>
On 21-06-19, 15:23, Daniel Lezcano wrote:
> The cpufreq_cooling_unregister() function uses now the policy to
> unregister itself. The only purpose of the cooling device pointer is
> to unregister the cpu cooling device.
>
> As there is no more need of this pointer, remove it.
>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
> drivers/cpufreq/cpufreq.c | 6 ++----
> include/linux/cpufreq.h | 3 ---
> 2 files changed, 2 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index dfbc9bea606c..1d8f85faeaca 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -1379,7 +1379,7 @@ static int cpufreq_online(unsigned int cpu)
> cpufreq_driver->ready(policy);
>
> if (cpufreq_driver->flags & CPUFREQ_IS_COOLING_DEV)
> - policy->cdev = of_cpufreq_cooling_register(policy);
> + of_cpufreq_cooling_register(policy);
>
> pr_debug("initialization complete\n");
>
> @@ -1468,10 +1468,8 @@ static int cpufreq_offline(unsigned int cpu)
> goto unlock;
> }
>
> - if (cpufreq_driver->flags & CPUFREQ_IS_COOLING_DEV) {
> + if (cpufreq_driver->flags & CPUFREQ_IS_COOLING_DEV)
> cpufreq_cooling_unregister(policy);
> - policy->cdev = NULL;
> - }
>
> if (cpufreq_driver->stop_cpu)
> cpufreq_driver->stop_cpu(policy);
> diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
> index d01a74fbc4db..9a42711f338b 100644
> --- a/include/linux/cpufreq.h
> +++ b/include/linux/cpufreq.h
> @@ -144,9 +144,6 @@ struct cpufreq_policy {
>
> /* For cpufreq driver's internal use */
> void *driver_data;
> -
> - /* Pointer to the cooling device if used for thermal mitigation */
> - struct thermal_cooling_device *cdev;
> };
>
> struct cpufreq_freqs {
This too. In my view this should be merged with 2/6.
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
--
viresh
^ permalink raw reply
* Re: [PATCH 3/6] cpufreq/drivers/arm_big_little: Remove cooling device usage
From: Viresh Kumar @ 2019-06-24 6:04 UTC (permalink / raw)
To: Daniel Lezcano
Cc: edubezval, linux-kernel, Sudeep Holla, Rafael J. Wysocki,
open list:CPU FREQUENCY DRIVERS - ARM BIG LITTLE
In-Reply-To: <20190621132302.30414-3-daniel.lezcano@linaro.org>
On 21-06-19, 15:22, Daniel Lezcano wrote:
> The cpufreq_cooling_unregister() function uses now the policy to
> unregister itself. The only purpose of the cooling device pointer is
> to unregister the cpu cooling device.
>
> As there is no more need of this pointer, remove it.
>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
> drivers/cpufreq/arm_big_little.c | 7 ++-----
> 1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/cpufreq/arm_big_little.c b/drivers/cpufreq/arm_big_little.c
> index 6b243202caa9..718c63231e66 100644
> --- a/drivers/cpufreq/arm_big_little.c
> +++ b/drivers/cpufreq/arm_big_little.c
> @@ -56,7 +56,6 @@ static bool bL_switching_enabled;
> #define ACTUAL_FREQ(cluster, freq) ((cluster == A7_CLUSTER) ? freq << 1 : freq)
> #define VIRT_FREQ(cluster, freq) ((cluster == A7_CLUSTER) ? freq >> 1 : freq)
>
> -static struct thermal_cooling_device *cdev[MAX_CLUSTERS];
> static const struct cpufreq_arm_bL_ops *arm_bL_ops;
> static struct clk *clk[MAX_CLUSTERS];
> static struct cpufreq_frequency_table *freq_table[MAX_CLUSTERS + 1];
> @@ -501,10 +500,8 @@ static int bL_cpufreq_exit(struct cpufreq_policy *policy)
> struct device *cpu_dev;
> int cur_cluster = cpu_to_cluster(policy->cpu);
>
> - if (cur_cluster < MAX_CLUSTERS) {
> + if (cur_cluster < MAX_CLUSTERS)
> cpufreq_cooling_unregister(policy);
> - cdev[cur_cluster] = NULL;
> - }
>
> cpu_dev = get_cpu_device(policy->cpu);
> if (!cpu_dev) {
> @@ -527,7 +524,7 @@ static void bL_cpufreq_ready(struct cpufreq_policy *policy)
> if (cur_cluster >= MAX_CLUSTERS)
> return;
>
> - cdev[cur_cluster] = of_cpufreq_cooling_register(policy);
> + of_cpufreq_cooling_register(policy);
> }
>
> static struct cpufreq_driver bL_cpufreq_driver = {
I will merge it with the previous commit if I were you.
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
--
viresh
^ permalink raw reply
* Re: [PATCH 2/6] thermal/drivers/cpu_cooling: Unregister with the policy
From: Viresh Kumar @ 2019-06-24 6:03 UTC (permalink / raw)
To: Daniel Lezcano
Cc: edubezval, linux-kernel, Sudeep Holla, Rafael J. Wysocki,
Amit Daniel Kachhap, Javi Merino, Zhang Rui, Shawn Guo,
Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
NXP Linux Team, Keerthy,
open list:CPU FREQUENCY DRIVERS - ARM BIG LITTLE,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
open list:TI BANDGAP AND THERMAL DRIVER
In-Reply-To: <20190621132302.30414-2-daniel.lezcano@linaro.org>
On 21-06-19, 15:22, Daniel Lezcano wrote:
> Currently the function cpufreq_cooling_register() returns a cooling
> device pointer which is used back as a pointer to call the function
> cpufreq_cooling_unregister(). Even if it is correct, it would make
> sense to not leak the structure inside a cpufreq driver and keep the
> code thermal code self-encapsulate. Moreover, that forces to add an
> extra variable in each driver using this function.
>
> Instead of passing the cooling device to unregister, pass the policy.
>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
> drivers/cpufreq/arm_big_little.c | 2 +-
> drivers/cpufreq/cpufreq.c | 2 +-
> drivers/thermal/cpu_cooling.c | 18 ++++++++++--------
> drivers/thermal/imx_thermal.c | 4 ++--
> .../thermal/ti-soc-thermal/ti-thermal-common.c | 2 +-
> include/linux/cpu_cooling.h | 6 +++---
> 6 files changed, 18 insertions(+), 16 deletions(-)
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
--
viresh
^ permalink raw reply
* Re: [RESEND,v2] cpufreq: s5pv210: Don't flood kernel log after cpufreq change
From: Viresh Kumar @ 2019-06-24 4:57 UTC (permalink / raw)
To: Paweł Chmiel
Cc: kgene, krzk, rjw, linux-arm-kernel, linux-samsung-soc, linux-pm,
linux-kernel
In-Reply-To: <20190621101043.10549-1-pawel.mikolaj.chmiel@gmail.com>
On 21-06-19, 12:10, Paweł Chmiel wrote:
> This commit replaces printk with pr_debug, so we don't flood kernel log.
>
> Signed-off-by: Paweł Chmiel <pawel.mikolaj.chmiel@gmail.com>
> Acked-by: Krzysztof Kozlowski <krzk@kernel.org>
> ---
> Changes from v1:
> - Added Acked-by
> ---
> drivers/cpufreq/s5pv210-cpufreq.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Applied. Thanks.
--
viresh
^ permalink raw reply
* [PATCH v1 07/11] PM / devfreq: tegra30: Reset boosting if clock rate changed
From: Dmitry Osipenko @ 2019-06-23 21:46 UTC (permalink / raw)
To: Thierry Reding, MyungJoo Ham, Kyungmin Park, Chanwoo Choi,
Jonathan Hunter, Tomeu Vizoso
Cc: linux-pm, linux-tegra, linux-kernel
In-Reply-To: <20190623214658.11680-1-digetx@gmail.com>
There is a situation when memory activity is going up, hence boosting up
starts to happen, and then governor ramps memory clock rate up. In this
case consecutive events may be stopped if new "COUNT" is within watermarks
range, meanwhile old boosting value remains, which is plainly wrong and
results in unneeded "go down" events after ramping up. In a result of this
change unnecessary interrupts activity goes even lower.
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
drivers/devfreq/tegra30-devfreq.c | 28 ++++++++++++++++++++++++++++
1 file changed, 28 insertions(+)
diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
index fc278f2f1b62..6fb3ca125438 100644
--- a/drivers/devfreq/tegra30-devfreq.c
+++ b/drivers/devfreq/tegra30-devfreq.c
@@ -631,6 +631,24 @@ static void tegra_actmon_stop(struct tegra_devfreq *tegra)
tegra_actmon_stop_device(&tegra->devices[i]);
}
+static void tegra_actmon_stop_boosting(struct tegra_devfreq *tegra)
+{
+ struct tegra_devfreq_device *dev = tegra->devices;
+ unsigned int i;
+ u32 dev_ctrl;
+
+ for (i = 0; i < ARRAY_SIZE(tegra->devices); i++, dev++) {
+ if (!dev->boost_freq)
+ continue;
+
+ dev_ctrl = device_readl(dev, ACTMON_DEV_CTRL);
+ dev_ctrl &= ~ACTMON_DEV_CTRL_CONSECUTIVE_BELOW_WMARK_EN;
+ device_writel(dev, dev_ctrl, ACTMON_DEV_CTRL);
+
+ dev->boost_freq = 0;
+ }
+}
+
static int tegra_devfreq_target(struct device *dev, unsigned long *freq,
u32 flags)
{
@@ -656,6 +674,16 @@ static int tegra_devfreq_target(struct device *dev, unsigned long *freq,
if (err)
goto restore_min_rate;
+ /*
+ * Hence boosting-up could be active at the moment of the rate-change
+ * and in this case boosting should be reset because it doesn't relate
+ * to the new state. If average won't follow shortly in a case of going
+ * UP, then clock rate will drop back on next update due to the missed
+ * boosting.
+ */
+ if (rate != devfreq->previous_freq)
+ tegra_actmon_stop_boosting(tegra);
+
return 0;
restore_min_rate:
--
2.22.0
^ permalink raw reply related
* [PATCH v1 08/11] PM / devfreq: tegra30: Stop de-boosting once it's finished
From: Dmitry Osipenko @ 2019-06-23 21:46 UTC (permalink / raw)
To: Thierry Reding, MyungJoo Ham, Kyungmin Park, Chanwoo Choi,
Jonathan Hunter, Tomeu Vizoso
Cc: linux-pm, linux-tegra, linux-kernel
In-Reply-To: <20190623214658.11680-1-digetx@gmail.com>
Once boosting value is 0, then further consecutive-down events won't do
anything useful since the only purpose of those consecutive events to
boost the freq and de-boosting is over in the case of 0. Note that upper
watermark is infinitely high in a case of frequency max out and thus the
upper events are stopping by themselves. In a result of this change all
parasite interrupts are fixed now and interrupts activity is nearly
non-existent now!
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
drivers/devfreq/tegra30-devfreq.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
index 6fb3ca125438..81449cc1392b 100644
--- a/drivers/devfreq/tegra30-devfreq.c
+++ b/drivers/devfreq/tegra30-devfreq.c
@@ -424,6 +424,8 @@ static void actmon_isr_device(struct tegra_devfreq *tegra,
dev->config->boost_up_coeff);
dev->boost_freq += ACTMON_BOOST_FREQ_STEP;
+ dev_ctrl |= ACTMON_DEV_CTRL_CONSECUTIVE_BELOW_WMARK_EN;
+
if (dev->boost_freq >= tegra->max_freq && !low_activity)
dev->boost_freq = tegra->max_freq;
@@ -442,6 +444,10 @@ static void actmon_isr_device(struct tegra_devfreq *tegra,
tegra_devfreq_update_wmark(tegra, dev);
}
+ /* no boosting => no need for consecutive-down interrupt */
+ if (dev->boost_freq == 0)
+ dev_ctrl &= ~ACTMON_DEV_CTRL_CONSECUTIVE_BELOW_WMARK_EN;
+
device_writel(dev, dev_ctrl, ACTMON_DEV_CTRL);
device_writel(dev, ACTMON_INTR_STATUS_CLEAR, ACTMON_DEV_INTR_STATUS);
--
2.22.0
^ permalink raw reply related
* Re: [PATCH v4 00/16] NVIDIA Tegra devfreq improvements and Tegra20/30 support
From: Dmitry Osipenko @ 2019-06-24 0:35 UTC (permalink / raw)
To: Chanwoo Choi, MyungJoo Ham
Cc: Thierry Reding, Jonathan Hunter, Kyungmin Park, Tomeu Vizoso,
linux-pm, linux-tegra, linux-kernel
In-Reply-To: <31fc1430-5a3b-cccb-06bb-7b46081edb68@samsung.com>
24.06.2019 2:50, Chanwoo Choi пишет:
> Hi Dmitry,
>
> On 19. 6. 24. 오전 2:17, Dmitry Osipenko wrote:
>> 05.06.2019 2:09, Dmitry Osipenko пишет:
>>> 04.06.2019 3:49, Chanwoo Choi пишет:
>>>> On 19. 6. 4. 오전 1:52, Dmitry Osipenko wrote:
>>>>> 03.05.2019 3:52, Dmitry Osipenko пишет:
>>>>>> 03.05.2019 3:31, Chanwoo Choi пишет:
>>>>>>> Hi Dmitry,
>>>>>>>
>>>>>>> On 19. 5. 2. 오전 8:37, Dmitry Osipenko wrote:
>>>>>>>> Changelog:
>>>>>>>>
>>>>>>>> v4: Addressed all review comments that were made by Chanwoo Choi to v3:
>>>>>>>>
>>>>>>>> - changed the driver removal order to match the probe exactly
>>>>>>>> - added clarifying comment for 1/8 ratio to the Tegra20 driver
>>>>>>>>
>>>>>>>> Chanwoo, please also note that the clk patch that should fix
>>>>>>>> compilation problem that was reported the kbuild-test-robot is already
>>>>>>>> applied and available in the recent linux-next.
>>>>>>>
>>>>>>> I knew that Stephen picked up your path about clock.
>>>>>>
>>>>>> Hi Chanwoo,
>>>>>>
>>>>>> Okay, good. Thank you very much for reviewing this series! I assume it's
>>>>>> too late now for v5.2, but it should be good to go for v5.3.
>>>>>>
>>>>>
>>>>> Hello Chanwoo,
>>>>>
>>>>> Will be nice to see the patches in the linux-next before they'll hit mainline. We have tested that
>>>>> everything works fine on a selective devices, but won't hurt to get some extra testing beforehand.
>>>>> AFAIK, at least NVIDIA people are regularly testing -next on theirs dev boards. Please note that
>>>>> this not very important, so don't bother if there is some hurdle with pushing to the tracking branch
>>>>> for now. Also please let me know if you're expecting to see some ACK's on the patches, I'm sure
>>>>> we'll be able to work out that with Thierry and Jon if necessary.
>>>>>
>>>>>
>>>>
>>>> Hi Dmitry,
>>>> I think that it is enough for applying to mainline branch.
>>>> The devfreq.git is maintained by Myungjoo. He will be merged or
>>>> reviewed if there are th remained review point.
>>>
>>> Thank you very much!
>>>
>>>>
>>>> Hi Myungjoo,
>>>> I reviewed the Dmitry's patches from v1 to v4 patches.
>>>> And then I tested them on my testing branch[1] for catching
>>>> the build warning and error. In result, it is clean.
>>>> [1] https://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/linux.git/log/?h=devfreq-testing
>>>>
>>>> Please review or apply these patches for v5.3.
>>>>
>>>
>>> Hello Myungjoo,
>>>
>>> I think this patchset should be completed now. Thierry has some extra
>>> comments to the patches, but seems nothing critical so far and all the
>>> concerns could be addressed in a follow-up series. Please let me know if
>>> you're fine with this, I can re-spin v5 as well if necessary.
>>>
>>
>> Hello Chanwoo,
>>
>> It looks like Myungjoo is inactive at the moment. Do you know if he'll
>> be back to the time of the merge window opening or you'll be curating
>> the pull request for 5.3 this time?
>
> Myungoo works in the same place. I'll talk with him.
>
>>
>> Secondly, I'll send a few more patches on top of this series, addressing
>> Thierry's comments and making more improvements. Please let me know if
>> this causes any problems and I should re-spin the whole series.
>
> OK. I'll review them.
Thank you!
^ permalink raw reply
* Re: [PATCH v4 00/16] NVIDIA Tegra devfreq improvements and Tegra20/30 support
From: Chanwoo Choi @ 2019-06-23 23:50 UTC (permalink / raw)
To: Dmitry Osipenko, MyungJoo Ham
Cc: Thierry Reding, Jonathan Hunter, Kyungmin Park, Tomeu Vizoso,
linux-pm, linux-tegra, linux-kernel
In-Reply-To: <389cc71d-8f0b-fa39-1325-433d27c75dc8@gmail.com>
Hi Dmitry,
On 19. 6. 24. 오전 2:17, Dmitry Osipenko wrote:
> 05.06.2019 2:09, Dmitry Osipenko пишет:
>> 04.06.2019 3:49, Chanwoo Choi пишет:
>>> On 19. 6. 4. 오전 1:52, Dmitry Osipenko wrote:
>>>> 03.05.2019 3:52, Dmitry Osipenko пишет:
>>>>> 03.05.2019 3:31, Chanwoo Choi пишет:
>>>>>> Hi Dmitry,
>>>>>>
>>>>>> On 19. 5. 2. 오전 8:37, Dmitry Osipenko wrote:
>>>>>>> Changelog:
>>>>>>>
>>>>>>> v4: Addressed all review comments that were made by Chanwoo Choi to v3:
>>>>>>>
>>>>>>> - changed the driver removal order to match the probe exactly
>>>>>>> - added clarifying comment for 1/8 ratio to the Tegra20 driver
>>>>>>>
>>>>>>> Chanwoo, please also note that the clk patch that should fix
>>>>>>> compilation problem that was reported the kbuild-test-robot is already
>>>>>>> applied and available in the recent linux-next.
>>>>>>
>>>>>> I knew that Stephen picked up your path about clock.
>>>>>
>>>>> Hi Chanwoo,
>>>>>
>>>>> Okay, good. Thank you very much for reviewing this series! I assume it's
>>>>> too late now for v5.2, but it should be good to go for v5.3.
>>>>>
>>>>
>>>> Hello Chanwoo,
>>>>
>>>> Will be nice to see the patches in the linux-next before they'll hit mainline. We have tested that
>>>> everything works fine on a selective devices, but won't hurt to get some extra testing beforehand.
>>>> AFAIK, at least NVIDIA people are regularly testing -next on theirs dev boards. Please note that
>>>> this not very important, so don't bother if there is some hurdle with pushing to the tracking branch
>>>> for now. Also please let me know if you're expecting to see some ACK's on the patches, I'm sure
>>>> we'll be able to work out that with Thierry and Jon if necessary.
>>>>
>>>>
>>>
>>> Hi Dmitry,
>>> I think that it is enough for applying to mainline branch.
>>> The devfreq.git is maintained by Myungjoo. He will be merged or
>>> reviewed if there are th remained review point.
>>
>> Thank you very much!
>>
>>>
>>> Hi Myungjoo,
>>> I reviewed the Dmitry's patches from v1 to v4 patches.
>>> And then I tested them on my testing branch[1] for catching
>>> the build warning and error. In result, it is clean.
>>> [1] https://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/linux.git/log/?h=devfreq-testing
>>>
>>> Please review or apply these patches for v5.3.
>>>
>>
>> Hello Myungjoo,
>>
>> I think this patchset should be completed now. Thierry has some extra
>> comments to the patches, but seems nothing critical so far and all the
>> concerns could be addressed in a follow-up series. Please let me know if
>> you're fine with this, I can re-spin v5 as well if necessary.
>>
>
> Hello Chanwoo,
>
> It looks like Myungjoo is inactive at the moment. Do you know if he'll
> be back to the time of the merge window opening or you'll be curating
> the pull request for 5.3 this time?
Myungoo works in the same place. I'll talk with him.
>
> Secondly, I'll send a few more patches on top of this series, addressing
> Thierry's comments and making more improvements. Please let me know if
> this causes any problems and I should re-spin the whole series.
OK. I'll review them.
--
Best Regards,
Chanwoo Choi
Samsung Electronics
^ permalink raw reply
* RE: [PATCH v1 01/11] PM / devfreq: tegra30: Change irq type to unsigned int
From: MyungJoo Ham @ 2019-06-24 2:12 UTC (permalink / raw)
To: Dmitry Osipenko, Thierry Reding, Kyungmin Park, Chanwoo Choi,
Jonathan Hunter, Tomeu Vizoso
Cc: linux-pm@vger.kernel.org, linux-tegra@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <20190623214658.11680-2-digetx@gmail.com>
>IRQ numbers are always positive, hence the corresponding variable should
>be unsigned to keep types consistent. This is a minor change that cleans
>up code a tad more.
>
>Suggested-by: Thierry Reding <thierry.reding@gmail.com>
>Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
Acked-by: MyungJoo Ham <myungjoo.ham@samsung.com>
Cheers,
MyungJoo
^ permalink raw reply
* [PATCH v1 09/11] PM / devfreq: tegra30: Don't enable consecutive-down interrupt on startup
From: Dmitry Osipenko @ 2019-06-23 21:46 UTC (permalink / raw)
To: Thierry Reding, MyungJoo Ham, Kyungmin Park, Chanwoo Choi,
Jonathan Hunter, Tomeu Vizoso
Cc: linux-pm, linux-tegra, linux-kernel
In-Reply-To: <20190623214658.11680-1-digetx@gmail.com>
The consecutive-down event tells that we should perform frequency
de-boosting, but boosting is in a reset state on start and hence the
event won't do anything useful for us and it will be just a dummy
interrupt request.
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
drivers/devfreq/tegra30-devfreq.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
index 81449cc1392b..168bfe78e525 100644
--- a/drivers/devfreq/tegra30-devfreq.c
+++ b/drivers/devfreq/tegra30-devfreq.c
@@ -572,7 +572,6 @@ static void tegra_actmon_configure_device(struct tegra_devfreq *tegra,
<< ACTMON_DEV_CTRL_CONSECUTIVE_ABOVE_WMARK_NUM_SHIFT;
val |= ACTMON_DEV_CTRL_AVG_ABOVE_WMARK_EN;
val |= ACTMON_DEV_CTRL_AVG_BELOW_WMARK_EN;
- val |= ACTMON_DEV_CTRL_CONSECUTIVE_BELOW_WMARK_EN;
val |= ACTMON_DEV_CTRL_CONSECUTIVE_ABOVE_WMARK_EN;
val |= ACTMON_DEV_CTRL_ENB;
--
2.22.0
^ permalink raw reply related
* [PATCH v1 05/11] PM / devfreq: tegra30: Rework frequency management logic
From: Dmitry Osipenko @ 2019-06-23 21:46 UTC (permalink / raw)
To: Thierry Reding, MyungJoo Ham, Kyungmin Park, Chanwoo Choi,
Jonathan Hunter, Tomeu Vizoso
Cc: linux-pm, linux-tegra, linux-kernel
In-Reply-To: <20190623214658.11680-1-digetx@gmail.com>
The current implementation is inaccurate and results in very intensive
interrupt activity, which neglects the whole idea of polling offload to
hardware. The reason of the shortcoming is that watermarks are not set
up correctly and this results in ACTMON constantly asking to change freq
and then these requests are ignored. Secondly, the CPU's client need to
take into account that CPUFreq may change while memory activity not,
thus an appropriate frequency notifier should be used instead of the
clk-notifier. The end result of this patch is that there are few hundreds
of ACTMON's interrupts instead of tens thousands after few minutes of a
working devfreq, meanwhile the transitions activity stays about the same
and governor becomes more reactive.
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
drivers/devfreq/tegra30-devfreq.c | 411 ++++++++++++++++++++++--------
1 file changed, 301 insertions(+), 110 deletions(-)
diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
index 4be7858c33bc..813b0f490b90 100644
--- a/drivers/devfreq/tegra30-devfreq.c
+++ b/drivers/devfreq/tegra30-devfreq.c
@@ -17,6 +17,7 @@
#include <linux/platform_device.h>
#include <linux/pm_opp.h>
#include <linux/reset.h>
+#include <linux/workqueue.h>
#include "governor.h"
@@ -40,6 +41,7 @@
#define ACTMON_DEV_AVG_UPPER_WMARK 0x10
#define ACTMON_DEV_AVG_LOWER_WMARK 0x14
#define ACTMON_DEV_COUNT_WEIGHT 0x18
+#define ACTMON_DEV_COUNT 0x1c
#define ACTMON_DEV_AVG_COUNT 0x20
#define ACTMON_DEV_INTR_STATUS 0x24
@@ -47,6 +49,8 @@
#define ACTMON_DEV_INTR_CONSECUTIVE_UPPER BIT(31)
#define ACTMON_DEV_INTR_CONSECUTIVE_LOWER BIT(30)
+#define ACTMON_DEV_INTR_AVG_BELOW_WMARK BIT(25)
+#define ACTMON_DEV_INTR_AVG_ABOVE_WMARK BIT(24)
#define ACTMON_ABOVE_WMARK_WINDOW 1
#define ACTMON_BELOW_WMARK_WINDOW 3
@@ -63,9 +67,8 @@
* ACTMON_AVERAGE_WINDOW_LOG2: default value for @DEV_CTRL_K_VAL, which
* translates to 2 ^ (K_VAL + 1). ex: 2 ^ (6 + 1) = 128
*/
-#define ACTMON_AVERAGE_WINDOW_LOG2 6
-#define ACTMON_SAMPLING_PERIOD 12 /* ms */
-#define ACTMON_DEFAULT_AVG_BAND 6 /* 1/10 of % */
+#define ACTMON_AVERAGE_WINDOW_LOG2 6
+#define ACTMON_SAMPLING_PERIOD 12 /* ms */
#define KHZ 1000
@@ -142,9 +145,6 @@ struct tegra_devfreq_device {
* watermark breaches.
*/
unsigned long boost_freq;
-
- /* Optimal frequency calculated from the stats for this device */
- unsigned long target_freq;
};
struct tegra_devfreq {
@@ -156,7 +156,8 @@ struct tegra_devfreq {
struct clk *emc_clock;
unsigned long max_freq;
- unsigned long cur_freq;
+
+ struct work_struct update_work;
struct notifier_block rate_change_nb;
struct tegra_devfreq_device devices[ARRAY_SIZE(actmon_device_configs)];
@@ -205,42 +206,216 @@ static unsigned long do_percent(unsigned long val, unsigned int pct)
return val * pct / 100;
}
+static unsigned long actmon_cpu_to_emc_rate(struct tegra_devfreq *tegra)
+{
+ struct tegra_actmon_emc_ratio *ratio = actmon_emc_ratios;
+ unsigned int cpu_freq = cpufreq_get(0);
+ unsigned int i;
+
+ for (i = 0; i < ARRAY_SIZE(actmon_emc_ratios); i++, ratio++) {
+ if (cpu_freq >= ratio->cpu_freq) {
+ if (ratio->emc_freq >= tegra->max_freq)
+ return tegra->max_freq;
+ else
+ return ratio->emc_freq;
+ }
+ }
+
+ return 0;
+}
+
+static unsigned long
+tegra_actmon_account_cpu_freq(struct tegra_devfreq *tegra,
+ struct tegra_devfreq_device *dev,
+ unsigned long target_freq,
+ unsigned long count)
+{
+ unsigned long static_cpu_emc_freq;
+
+ if (dev->config->avg_dependency_threshold &&
+ dev->config->avg_dependency_threshold < count) {
+ static_cpu_emc_freq = actmon_cpu_to_emc_rate(tegra);
+ target_freq = max(target_freq, static_cpu_emc_freq);
+ }
+
+ return target_freq;
+}
+
+static unsigned long tegra_actmon_lower_freq(struct tegra_devfreq *tegra,
+ unsigned long target_freq)
+{
+ unsigned long lower = target_freq;
+ struct dev_pm_opp *opp;
+
+ opp = dev_pm_opp_find_freq_floor(tegra->devfreq->dev.parent, &lower);
+ if (IS_ERR(opp))
+ lower = 0;
+ else
+ dev_pm_opp_put(opp);
+
+ return lower;
+}
+
+static unsigned long tegra_actmon_upper_freq(struct tegra_devfreq *tegra,
+ unsigned long target_freq)
+{
+ unsigned long upper = target_freq + 1;
+ struct dev_pm_opp *opp;
+
+ opp = dev_pm_opp_find_freq_ceil(tegra->devfreq->dev.parent, &upper);
+ if (IS_ERR(opp))
+ upper = ULONG_MAX;
+ else
+ dev_pm_opp_put(opp);
+
+ return upper;
+}
+
+static void tegra_actmon_get_lower_upper(struct tegra_devfreq *tegra,
+ struct tegra_devfreq_device *dev,
+ unsigned long count,
+ unsigned long *lower,
+ unsigned long *upper)
+{
+ unsigned long target_freq, min;
+
+ target_freq = count / ACTMON_SAMPLING_PERIOD * KHZ;
+
+ /*
+ * Memory frequencies are guaranteed to have 1MHz granularity
+ * and thus we need this rounding down to get a proper watermarks
+ * range in a case where target_freq falls into a range of
+ * next_possible_opp_freq - 1MHz.
+ */
+ target_freq = round_down(target_freq, 1000000);
+
+ *lower = tegra_actmon_lower_freq(tegra, target_freq);
+
+ /*
+ * When memory activity is below the minimum bandwidth that EMC
+ * driver could provide, the upper watermark should be set for
+ * the second OPP to avoid unnecessary interrupts activity when
+ * the minimum threshold is crossed towards zero bandwidth.
+ *
+ * ------ target_freq
+ * | ----------------- minimum possible freq
+ * v v
+ * |0|----------|min freq|----|second freq|----|third freq|---...
+ * ^ ^
+ * | |
+ * ---- lower watermark ---- upper watermark
+ */
+ min = tegra_actmon_upper_freq(tegra, 0);
+
+ if (target_freq < min)
+ *upper = tegra_actmon_upper_freq(tegra, min);
+ else
+ *upper = tegra_actmon_upper_freq(tegra, target_freq);
+
+ *lower /= KHZ;
+ *upper /= KHZ;
+
+ /*
+ * The upper watermark should take into account CPU's frequency
+ * because cpu_to_emc_rate() may override the target_freq with
+ * a higher value and thus upper watermark need to be set up
+ * accordingly to avoid parasitic upper-events.
+ */
+ *upper = tegra_actmon_account_cpu_freq(tegra, dev, *upper, count);
+
+ *lower *= ACTMON_SAMPLING_PERIOD;
+ *upper *= ACTMON_SAMPLING_PERIOD;
+}
+
static void tegra_devfreq_update_avg_wmark(struct tegra_devfreq *tegra,
struct tegra_devfreq_device *dev)
{
- u32 avg = dev->avg_count;
- u32 avg_band_freq = tegra->max_freq * ACTMON_DEFAULT_AVG_BAND / KHZ;
- u32 band = avg_band_freq * ACTMON_SAMPLING_PERIOD;
+ unsigned long count = dev->avg_count;
+ unsigned long lower, upper;
+
+ tegra_actmon_get_lower_upper(tegra, dev, count, &lower, &upper);
- device_writel(dev, avg + band, ACTMON_DEV_AVG_UPPER_WMARK);
+ /*
+ * We want to get interrupts when MCCPU client crosses the
+ * dependency threshold in order to into / out of account the
+ * CPU's freq.
+ */
+ if (lower == 0 && dev->config->avg_dependency_threshold) {
+ if (count < dev->config->avg_dependency_threshold)
+ upper = dev->config->avg_dependency_threshold;
+ else
+ lower = dev->config->avg_dependency_threshold;
+ }
- avg = max(dev->avg_count, band);
- device_writel(dev, avg - band, ACTMON_DEV_AVG_LOWER_WMARK);
+ device_writel(dev, lower, ACTMON_DEV_AVG_LOWER_WMARK);
+ device_writel(dev, upper, ACTMON_DEV_AVG_UPPER_WMARK);
}
static void tegra_devfreq_update_wmark(struct tegra_devfreq *tegra,
struct tegra_devfreq_device *dev)
{
- u32 val = tegra->cur_freq * ACTMON_SAMPLING_PERIOD;
+ unsigned long count = device_readl(dev, ACTMON_DEV_COUNT);
+ unsigned long lower, upper, delta;
- device_writel(dev, do_percent(val, dev->config->boost_up_threshold),
- ACTMON_DEV_UPPER_WMARK);
+ /*
+ * Boosting logic kicks-in once lower / upper watermark is hit.
+ *
+ * count -------- --- upper delta
+ * v v
+ * ...--|OPP N|--|lower|<------->-<--------->|upper|--|OPP N+1|--...
+ * ^ ^
+ * | |
+ * lower delta--- |
+ * |
+ * average count somewhere there------
+ */
+
+ tegra_actmon_get_lower_upper(tegra, dev, count, &lower, &upper);
+
+ /* don't forget to take boosting into account */
+ upper += dev->boost_freq;
+
+ /* the lesser memory activity goes, the harder is boost-down */
+ delta = do_percent(count - lower, dev->config->boost_down_threshold);
+ device_writel(dev, count - delta, ACTMON_DEV_LOWER_WMARK);
- device_writel(dev, do_percent(val, dev->config->boost_down_threshold),
- ACTMON_DEV_LOWER_WMARK);
+ /* the higher memory activity goes, the softer is boost-up */
+ delta = do_percent(upper - count, dev->config->boost_up_threshold);
+ device_writel(dev, count + delta, ACTMON_DEV_UPPER_WMARK);
}
static void actmon_isr_device(struct tegra_devfreq *tegra,
struct tegra_devfreq_device *dev)
{
- u32 intr_status, dev_ctrl;
+ u32 intr_status, dev_ctrl, avg_intr_mask;
+ bool low_activity = true;
dev->avg_count = device_readl(dev, ACTMON_DEV_AVG_COUNT);
- tegra_devfreq_update_avg_wmark(tegra, dev);
-
intr_status = device_readl(dev, ACTMON_DEV_INTR_STATUS);
dev_ctrl = device_readl(dev, ACTMON_DEV_CTRL);
+ avg_intr_mask = ACTMON_DEV_INTR_AVG_BELOW_WMARK |
+ ACTMON_DEV_INTR_AVG_ABOVE_WMARK;
+
+ if (intr_status & avg_intr_mask)
+ tegra_devfreq_update_avg_wmark(tegra, dev);
+
+ /*
+ * Instantly cutoff boosting if CPU isn't touching memory nearly
+ * at all for awhile, this result in a more interactive governing.
+ *
+ * MCALL doesn't have the minimum activity threshold.
+ *
+ * Note that it's important to disabled boosting before calling
+ * tegra_devfreq_update_wmark() because it takes boosting into
+ * account.
+ */
+ if (dev->config->avg_dependency_threshold &&
+ dev->avg_count < dev->config->avg_dependency_threshold)
+ dev->boost_freq = 0;
+ else
+ low_activity = false;
+
if (intr_status & ACTMON_DEV_INTR_CONSECUTIVE_UPPER) {
/*
* new_boost = min(old_boost * up_coef + step, max_freq)
@@ -249,12 +424,10 @@ static void actmon_isr_device(struct tegra_devfreq *tegra,
dev->config->boost_up_coeff);
dev->boost_freq += ACTMON_BOOST_FREQ_STEP;
- dev_ctrl |= ACTMON_DEV_CTRL_CONSECUTIVE_BELOW_WMARK_EN;
-
- if (dev->boost_freq >= tegra->max_freq)
+ if (dev->boost_freq >= tegra->max_freq && !low_activity)
dev->boost_freq = tegra->max_freq;
- else
- dev_ctrl |= ACTMON_DEV_CTRL_CONSECUTIVE_ABOVE_WMARK_EN;
+
+ tegra_devfreq_update_wmark(tegra, dev);
} else if (intr_status & ACTMON_DEV_INTR_CONSECUTIVE_LOWER) {
/*
* new_boost = old_boost * down_coef
@@ -263,19 +436,10 @@ static void actmon_isr_device(struct tegra_devfreq *tegra,
dev->boost_freq = do_percent(dev->boost_freq,
dev->config->boost_down_coeff);
- dev_ctrl |= ACTMON_DEV_CTRL_CONSECUTIVE_ABOVE_WMARK_EN;
-
if (dev->boost_freq < (ACTMON_BOOST_FREQ_STEP >> 1))
dev->boost_freq = 0;
- else
- dev_ctrl |= ACTMON_DEV_CTRL_CONSECUTIVE_BELOW_WMARK_EN;
- }
- if (dev->config->avg_dependency_threshold) {
- if (dev->avg_count >= dev->config->avg_dependency_threshold)
- dev_ctrl |= ACTMON_DEV_CTRL_CONSECUTIVE_BELOW_WMARK_EN;
- else if (dev->boost_freq == 0)
- dev_ctrl &= ~ACTMON_DEV_CTRL_CONSECUTIVE_BELOW_WMARK_EN;
+ tegra_devfreq_update_wmark(tegra, dev);
}
device_writel(dev, dev_ctrl, ACTMON_DEV_CTRL);
@@ -283,43 +447,20 @@ static void actmon_isr_device(struct tegra_devfreq *tegra,
device_writel(dev, ACTMON_INTR_STATUS_CLEAR, ACTMON_DEV_INTR_STATUS);
}
-static unsigned long actmon_cpu_to_emc_rate(struct tegra_devfreq *tegra,
- unsigned long cpu_freq)
-{
- unsigned int i;
- struct tegra_actmon_emc_ratio *ratio = actmon_emc_ratios;
-
- for (i = 0; i < ARRAY_SIZE(actmon_emc_ratios); i++, ratio++) {
- if (cpu_freq >= ratio->cpu_freq) {
- if (ratio->emc_freq >= tegra->max_freq)
- return tegra->max_freq;
- else
- return ratio->emc_freq;
- }
- }
-
- return 0;
-}
-
-static void actmon_update_target(struct tegra_devfreq *tegra,
- struct tegra_devfreq_device *dev)
+static unsigned long actmon_update_target(struct tegra_devfreq *tegra,
+ struct tegra_devfreq_device *dev)
{
- unsigned long cpu_freq = 0;
- unsigned long static_cpu_emc_freq = 0;
- unsigned int avg_sustain_coef;
-
- if (dev->config->avg_dependency_threshold) {
- cpu_freq = cpufreq_get(0);
- static_cpu_emc_freq = actmon_cpu_to_emc_rate(tegra, cpu_freq);
- }
+ unsigned long avg_sustain_coef, target_freq;
- dev->target_freq = dev->avg_count / ACTMON_SAMPLING_PERIOD;
avg_sustain_coef = 100 * 100 / dev->config->boost_up_threshold;
- dev->target_freq = do_percent(dev->target_freq, avg_sustain_coef);
- dev->target_freq += dev->boost_freq;
- if (dev->avg_count >= dev->config->avg_dependency_threshold)
- dev->target_freq = max(dev->target_freq, static_cpu_emc_freq);
+ target_freq = dev->avg_count / ACTMON_SAMPLING_PERIOD;
+ target_freq = do_percent(target_freq, avg_sustain_coef);
+ target_freq += dev->boost_freq;
+ target_freq = tegra_actmon_account_cpu_freq(tegra, dev, target_freq,
+ dev->avg_count);
+
+ return target_freq;
}
static irqreturn_t actmon_thread_isr(int irq, void *data)
@@ -347,26 +488,53 @@ static irqreturn_t actmon_thread_isr(int irq, void *data)
return handled ? IRQ_HANDLED : IRQ_NONE;
}
+static void tegra_actmon_update_watermarks(struct tegra_devfreq *tegra)
+{
+ struct tegra_devfreq_device *dev = tegra->devices;
+ unsigned int i;
+
+ for (i = 0; i < ARRAY_SIZE(tegra->devices); i++, dev++) {
+ tegra_devfreq_update_avg_wmark(tegra, dev);
+ tegra_devfreq_update_wmark(tegra, dev);
+ }
+}
+
+static void tegra_actmon_delayed_update(struct work_struct *work)
+{
+ struct tegra_devfreq *tegra = container_of(work, struct tegra_devfreq,
+ update_work);
+ unsigned long previous_freq;
+
+ mutex_lock(&tegra->devfreq->lock);
+
+ previous_freq = tegra->devfreq->previous_freq;
+
+ update_devfreq(tegra->devfreq);
+
+ /*
+ * CPU freq changed and this may result in a change of a target
+ * memory rate, hence the watermarks may need to be updated too
+ * because they depend on the CPU's freq and interrupt events will
+ * stop if the watermark's range is now larger than it should be.
+ */
+ if (tegra->devfreq->previous_freq != previous_freq)
+ tegra_actmon_update_watermarks(tegra);
+
+ mutex_unlock(&tegra->devfreq->lock);
+}
+
static int tegra_actmon_rate_notify_cb(struct notifier_block *nb,
unsigned long action, void *ptr)
{
- struct clk_notifier_data *data = ptr;
struct tegra_devfreq *tegra;
- struct tegra_devfreq_device *dev;
- unsigned int i;
- if (action != POST_RATE_CHANGE)
+ if (action != CPUFREQ_POSTCHANGE)
return NOTIFY_OK;
tegra = container_of(nb, struct tegra_devfreq, rate_change_nb);
- tegra->cur_freq = data->new_rate / KHZ;
-
- for (i = 0; i < ARRAY_SIZE(tegra->devices); i++) {
- dev = &tegra->devices[i];
-
- tegra_devfreq_update_wmark(tegra, dev);
- }
+ /* notifier runs with a disabled preemption */
+ schedule_work(&tegra->update_work);
return NOTIFY_OK;
}
@@ -374,11 +542,10 @@ static int tegra_actmon_rate_notify_cb(struct notifier_block *nb,
static void tegra_actmon_configure_device(struct tegra_devfreq *tegra,
struct tegra_devfreq_device *dev)
{
- u32 val = 0;
-
- dev->target_freq = tegra->cur_freq;
+ u32 val = 0, target_freq;
- dev->avg_count = tegra->cur_freq * ACTMON_SAMPLING_PERIOD;
+ target_freq = clk_get_rate(tegra->emc_clock) / KHZ;
+ dev->avg_count = target_freq * ACTMON_SAMPLING_PERIOD;
device_writel(dev, dev->avg_count, ACTMON_DEV_INIT_AVG);
tegra_devfreq_update_avg_wmark(tegra, dev);
@@ -403,9 +570,16 @@ static void tegra_actmon_configure_device(struct tegra_devfreq *tegra,
device_writel(dev, val, ACTMON_DEV_CTRL);
}
-static void tegra_actmon_start(struct tegra_devfreq *tegra)
+static void tegra_actmon_stop_device(struct tegra_devfreq_device *dev)
+{
+ device_writel(dev, 0x00000000, ACTMON_DEV_CTRL);
+ device_writel(dev, ACTMON_INTR_STATUS_CLEAR, ACTMON_DEV_INTR_STATUS);
+}
+
+static int tegra_actmon_start(struct tegra_devfreq *tegra)
{
unsigned int i;
+ int err;
actmon_writel(tegra, ACTMON_SAMPLING_PERIOD - 1,
ACTMON_GLB_PERIOD_CTRL);
@@ -413,7 +587,30 @@ static void tegra_actmon_start(struct tegra_devfreq *tegra)
for (i = 0; i < ARRAY_SIZE(tegra->devices); i++)
tegra_actmon_configure_device(tegra, &tegra->devices[i]);
+ /*
+ * We are estimating CPU's memory bandwidth requirement based on
+ * amount of memory accesses and system's load, judging by CPU's
+ * frequency. We also don't want to receive events about CPU's
+ * frequency transaction when governor is stopped, hence
+ * notifier is registered dynamically.
+ */
+ err = cpufreq_register_notifier(&tegra->rate_change_nb,
+ CPUFREQ_TRANSITION_NOTIFIER);
+ if (err) {
+ dev_err(tegra->devfreq->dev.parent,
+ "Failed to register rate change notifier: %d\n", err);
+ goto err_stop;
+ }
+
enable_irq(tegra->irq);
+
+ return 0;
+
+err_stop:
+ for (i = 0; i < ARRAY_SIZE(tegra->devices); i++)
+ tegra_actmon_stop_device(&tegra->devices[i]);
+
+ return err;
}
static void tegra_actmon_stop(struct tegra_devfreq *tegra)
@@ -422,11 +619,13 @@ static void tegra_actmon_stop(struct tegra_devfreq *tegra)
disable_irq(tegra->irq);
- for (i = 0; i < ARRAY_SIZE(tegra->devices); i++) {
- device_writel(&tegra->devices[i], 0x00000000, ACTMON_DEV_CTRL);
- device_writel(&tegra->devices[i], ACTMON_INTR_STATUS_CLEAR,
- ACTMON_DEV_INTR_STATUS);
- }
+ cpufreq_unregister_notifier(&tegra->rate_change_nb,
+ CPUFREQ_TRANSITION_NOTIFIER);
+
+ cancel_work_sync(&tegra->update_work);
+
+ for (i = 0; i < ARRAY_SIZE(tegra->devices); i++)
+ tegra_actmon_stop_device(&tegra->devices[i]);
}
static int tegra_devfreq_target(struct device *dev, unsigned long *freq,
@@ -469,13 +668,13 @@ static int tegra_devfreq_get_dev_status(struct device *dev,
struct tegra_devfreq_device *actmon_dev;
unsigned long cur_freq;
- cur_freq = READ_ONCE(tegra->cur_freq);
+ cur_freq = clk_get_rate(tegra->emc_clock);
/* To be used by the tegra governor */
stat->private_data = tegra;
/* The below are to be used by the other governors */
- stat->current_frequency = cur_freq * KHZ;
+ stat->current_frequency = cur_freq;
actmon_dev = &tegra->devices[MCALL];
@@ -486,7 +685,7 @@ static int tegra_devfreq_get_dev_status(struct device *dev,
stat->busy_time *= 100 / BUS_SATURATION_RATIO;
/* Number of cycles in a sampling period */
- stat->total_time = ACTMON_SAMPLING_PERIOD * cur_freq;
+ stat->total_time = cur_freq / KHZ * ACTMON_SAMPLING_PERIOD;
stat->busy_time = min(stat->busy_time, stat->total_time);
@@ -505,6 +704,7 @@ static int tegra_governor_get_target(struct devfreq *devfreq,
struct devfreq_dev_status *stat;
struct tegra_devfreq *tegra;
struct tegra_devfreq_device *dev;
+ unsigned long dev_target_freq;
unsigned long target_freq = 0;
unsigned int i;
int err;
@@ -520,9 +720,9 @@ static int tegra_governor_get_target(struct devfreq *devfreq,
for (i = 0; i < ARRAY_SIZE(tegra->devices); i++) {
dev = &tegra->devices[i];
- actmon_update_target(tegra, dev);
+ dev_target_freq = actmon_update_target(tegra, dev);
- target_freq = max(target_freq, dev->target_freq);
+ target_freq = max(target_freq, dev_target_freq);
}
*freq = target_freq * KHZ;
@@ -534,6 +734,7 @@ static int tegra_governor_event_handler(struct devfreq *devfreq,
unsigned int event, void *data)
{
struct tegra_devfreq *tegra = dev_get_drvdata(devfreq->dev.parent);
+ int ret = 0;
/*
* Couple device with the governor early as it is needed at
@@ -544,7 +745,7 @@ static int tegra_governor_event_handler(struct devfreq *devfreq,
switch (event) {
case DEVFREQ_GOV_START:
devfreq_monitor_start(devfreq);
- tegra_actmon_start(tegra);
+ ret = tegra_actmon_start(tegra);
break;
case DEVFREQ_GOV_STOP:
@@ -559,11 +760,11 @@ static int tegra_governor_event_handler(struct devfreq *devfreq,
case DEVFREQ_GOV_RESUME:
devfreq_monitor_resume(devfreq);
- tegra_actmon_start(tegra);
+ ret = tegra_actmon_start(tegra);
break;
}
- return 0;
+ return ret;
}
static struct devfreq_governor tegra_devfreq_governor = {
@@ -642,7 +843,6 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
return rate;
}
- tegra->cur_freq = clk_get_rate(tegra->emc_clock) / KHZ;
tegra->max_freq = rate / KHZ;
for (i = 0; i < ARRAY_SIZE(actmon_device_configs); i++) {
@@ -671,17 +871,12 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, tegra);
tegra->rate_change_nb.notifier_call = tegra_actmon_rate_notify_cb;
- err = clk_notifier_register(tegra->emc_clock, &tegra->rate_change_nb);
- if (err) {
- dev_err(&pdev->dev,
- "Failed to register rate change notifier\n");
- goto remove_opps;
- }
+ INIT_WORK(&tegra->update_work, tegra_actmon_delayed_update);
err = devfreq_add_governor(&tegra_devfreq_governor);
if (err) {
dev_err(&pdev->dev, "Failed to add governor: %d\n", err);
- goto unreg_notifier;
+ goto remove_opps;
}
tegra_devfreq_profile.initial_freq = clk_get_rate(tegra->emc_clock);
@@ -697,9 +892,6 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
remove_governor:
devfreq_remove_governor(&tegra_devfreq_governor);
-unreg_notifier:
- clk_notifier_unregister(tegra->emc_clock, &tegra->rate_change_nb);
-
remove_opps:
dev_pm_opp_remove_all_dynamic(&pdev->dev);
@@ -716,7 +908,6 @@ static int tegra_devfreq_remove(struct platform_device *pdev)
devfreq_remove_device(tegra->devfreq);
devfreq_remove_governor(&tegra_devfreq_governor);
- clk_notifier_unregister(tegra->emc_clock, &tegra->rate_change_nb);
dev_pm_opp_remove_all_dynamic(&pdev->dev);
reset_control_reset(tegra->reset);
--
2.22.0
^ permalink raw reply related
* [PATCH v1 03/11] PM / devfreq: tegra30: Handle possible round-rate error
From: Dmitry Osipenko @ 2019-06-23 21:46 UTC (permalink / raw)
To: Thierry Reding, MyungJoo Ham, Kyungmin Park, Chanwoo Choi,
Jonathan Hunter, Tomeu Vizoso
Cc: linux-pm, linux-tegra, linux-kernel
In-Reply-To: <20190623214658.11680-1-digetx@gmail.com>
The EMC clock rate rounding technically could fail, hence let's handle
the error cases properly.
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
drivers/devfreq/tegra30-devfreq.c | 17 +++++++++++++++--
1 file changed, 15 insertions(+), 2 deletions(-)
diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
index 5e2b133babdd..5e606ae3f238 100644
--- a/drivers/devfreq/tegra30-devfreq.c
+++ b/drivers/devfreq/tegra30-devfreq.c
@@ -592,8 +592,8 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
struct tegra_devfreq_device *dev;
struct tegra_devfreq *tegra;
struct devfreq *devfreq;
- unsigned long rate;
unsigned int i;
+ long rate;
int err;
tegra = devm_kzalloc(&pdev->dev, sizeof(*tegra), GFP_KERNEL);
@@ -650,8 +650,14 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
reset_control_deassert(tegra->reset);
- tegra->max_freq = clk_round_rate(tegra->emc_clock, ULONG_MAX) / KHZ;
+ rate = clk_round_rate(tegra->emc_clock, ULONG_MAX);
+ if (rate < 0) {
+ dev_err(&pdev->dev, "Failed to round clock rate: %ld\n", rate);
+ return rate;
+ }
+
tegra->cur_freq = clk_get_rate(tegra->emc_clock) / KHZ;
+ tegra->max_freq = rate / KHZ;
for (i = 0; i < ARRAY_SIZE(actmon_device_configs); i++) {
dev = tegra->devices + i;
@@ -662,6 +668,13 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
for (rate = 0; rate <= tegra->max_freq * KHZ; rate++) {
rate = clk_round_rate(tegra->emc_clock, rate);
+ if (rate < 0) {
+ dev_err(&pdev->dev,
+ "Failed to round clock rate: %ld\n", rate);
+ err = rate;
+ goto remove_opps;
+ }
+
err = dev_pm_opp_add(&pdev->dev, rate, 0);
if (err) {
dev_err(&pdev->dev, "Failed to add OPP: %d\n", err);
--
2.22.0
^ permalink raw reply related
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