* Re: [PATCH v9 01/15] ARM: Add Krait L2 register accessor functions
From: Sricharan R @ 2018-05-25 5:40 UTC (permalink / raw)
To: Bjorn Andersson
Cc: robh, viresh.kumar, mark.rutland, mturquette, sboyd, linux,
andy.gross, david.brown, rjw, linux-arm-kernel, devicetree,
linux-kernel, linux-clk, linux-arm-msm, linux-soc, linux-pm,
linux
In-Reply-To: <20180524173926.GC14924@minitux>
Hi Bjorn,
On 5/24/2018 11:09 PM, Bjorn Andersson wrote:
> On Tue 06 Mar 06:38 PST 2018, Sricharan R wrote:
>
>> From: Stephen Boyd <sboyd@codeaurora.org>
>>
>> Krait CPUs have a handful of L2 cache controller registers that
>> live behind a cp15 based indirection register. First you program
>> the indirection register (l2cpselr) to point the L2 'window'
>> register (l2cpdr) at what you want to read/write. Then you
>> read/write the 'window' register to do what you want. The
>> l2cpselr register is not banked per-cpu so we must lock around
>> accesses to it to prevent other CPUs from re-pointing l2cpdr
>> underneath us.
>>
>> Cc: Mark Rutland <mark.rutland@arm.com>
>> Cc: Russell King <linux@arm.linux.org.uk>
>> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
>
> This should have your signed-off-by here as well.
>
ok.
> Apart from that:
>
> Acked-by: Bjorn Andersson <bjorn.andersson@linaro.org>
>
Thanks.
Regards,
Sricharan
> Regards,
> Bjorn
>
>> ---
>> arch/arm/common/Kconfig | 3 ++
>> arch/arm/common/Makefile | 1 +
>> arch/arm/common/krait-l2-accessors.c | 48 +++++++++++++++++++++++++++++++
>> arch/arm/include/asm/krait-l2-accessors.h | 10 +++++++
>> 4 files changed, 62 insertions(+)
>> create mode 100644 arch/arm/common/krait-l2-accessors.c
>> create mode 100644 arch/arm/include/asm/krait-l2-accessors.h
>>
>> diff --git a/arch/arm/common/Kconfig b/arch/arm/common/Kconfig
>> index e5ad070..c8e1986 100644
>> --- a/arch/arm/common/Kconfig
>> +++ b/arch/arm/common/Kconfig
>> @@ -7,6 +7,9 @@ config DMABOUNCE
>> bool
>> select ZONE_DMA
>>
>> +config KRAIT_L2_ACCESSORS
>> + bool
>> +
>> config SHARP_LOCOMO
>> bool
>>
>> diff --git a/arch/arm/common/Makefile b/arch/arm/common/Makefile
>> index 70b4a14..eec6cd1 100644
>> --- a/arch/arm/common/Makefile
>> +++ b/arch/arm/common/Makefile
>> @@ -7,6 +7,7 @@ obj-y += firmware.o
>>
>> obj-$(CONFIG_SA1111) += sa1111.o
>> obj-$(CONFIG_DMABOUNCE) += dmabounce.o
>> +obj-$(CONFIG_KRAIT_L2_ACCESSORS) += krait-l2-accessors.o
>> obj-$(CONFIG_SHARP_LOCOMO) += locomo.o
>> obj-$(CONFIG_SHARP_PARAM) += sharpsl_param.o
>> obj-$(CONFIG_SHARP_SCOOP) += scoop.o
>> diff --git a/arch/arm/common/krait-l2-accessors.c b/arch/arm/common/krait-l2-accessors.c
>> new file mode 100644
>> index 0000000..9a97dda
>> --- /dev/null
>> +++ b/arch/arm/common/krait-l2-accessors.c
>> @@ -0,0 +1,48 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +// Copyright (c) 2018, The Linux Foundation. All rights reserved.
>> +
>> +#include <linux/spinlock.h>
>> +#include <linux/export.h>
>> +
>> +#include <asm/barrier.h>
>> +#include <asm/krait-l2-accessors.h>
>> +
>> +static DEFINE_RAW_SPINLOCK(krait_l2_lock);
>> +
>> +void krait_set_l2_indirect_reg(u32 addr, u32 val)
>> +{
>> + unsigned long flags;
>> +
>> + raw_spin_lock_irqsave(&krait_l2_lock, flags);
>> + /*
>> + * Select the L2 window by poking l2cpselr, then write to the window
>> + * via l2cpdr.
>> + */
>> + asm volatile ("mcr p15, 3, %0, c15, c0, 6 @ l2cpselr" : : "r" (addr));
>> + isb();
>> + asm volatile ("mcr p15, 3, %0, c15, c0, 7 @ l2cpdr" : : "r" (val));
>> + isb();
>> +
>> + raw_spin_unlock_irqrestore(&krait_l2_lock, flags);
>> +}
>> +EXPORT_SYMBOL(krait_set_l2_indirect_reg);
>> +
>> +u32 krait_get_l2_indirect_reg(u32 addr)
>> +{
>> + u32 val;
>> + unsigned long flags;
>> +
>> + raw_spin_lock_irqsave(&krait_l2_lock, flags);
>> + /*
>> + * Select the L2 window by poking l2cpselr, then read from the window
>> + * via l2cpdr.
>> + */
>> + asm volatile ("mcr p15, 3, %0, c15, c0, 6 @ l2cpselr" : : "r" (addr));
>> + isb();
>> + asm volatile ("mrc p15, 3, %0, c15, c0, 7 @ l2cpdr" : "=r" (val));
>> +
>> + raw_spin_unlock_irqrestore(&krait_l2_lock, flags);
>> +
>> + return val;
>> +}
>> +EXPORT_SYMBOL(krait_get_l2_indirect_reg);
>> diff --git a/arch/arm/include/asm/krait-l2-accessors.h b/arch/arm/include/asm/krait-l2-accessors.h
>> new file mode 100644
>> index 0000000..dd7c474
>> --- /dev/null
>> +++ b/arch/arm/include/asm/krait-l2-accessors.h
>> @@ -0,0 +1,10 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +// Copyright (c) 2018, The Linux Foundation. All rights reserved.
>> +
>> +#ifndef __ASMARM_KRAIT_L2_ACCESSORS_H
>> +#define __ASMARM_KRAIT_L2_ACCESSORS_H
>> +
>> +extern void krait_set_l2_indirect_reg(u32 addr, u32 val);
>> +extern u32 krait_get_l2_indirect_reg(u32 addr);
>> +
>> +#endif
>> --
>> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
>>
--
"QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
^ permalink raw reply
* Re: [PATCH] cpufreq / CPPC: Add cpuinfo_cur_freq support for CPPC
From: George Cherian @ 2018-05-25 6:27 UTC (permalink / raw)
To: Prakash, Prashanth, George Cherian, linux-kernel, linux-pm
Cc: rjw, viresh.kumar
In-Reply-To: <ac2eb509-c1b0-521a-07e5-2bf8eaaa55c2@codeaurora.org>
Hi Prashanth,
On 05/25/2018 12:55 AM, Prakash, Prashanth wrote:
> Hi George,
>
> On 5/22/2018 5:42 AM, George Cherian wrote:
>> Per Section 8.4.7.1.3 of ACPI 6.2, The platform provides performance
>> feedback via set of performance counters. To determine the actual
>> performance level delivered over time, OSPM may read a set of
>> performance counters from the Reference Performance Counter Register
>> and the Delivered Performance Counter Register.
>>
>> OSPM calculates the delivered performance over a given time period by
>> taking a beginning and ending snapshot of both the reference and
>> delivered performance counters, and calculating:
>>
>> delivered_perf = reference_perf X (delta of delivered_perf counter / delta of reference_perf counter).
>>
>> Implement the above and hook this to the cpufreq->get method.
>>
>> Signed-off-by: George Cherian <george.cherian@cavium.com>
>> ---
>> drivers/cpufreq/cppc_cpufreq.c | 44 ++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 44 insertions(+)
>>
>> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
>> index b15115a..a046915 100644
>> --- a/drivers/cpufreq/cppc_cpufreq.c
>> +++ b/drivers/cpufreq/cppc_cpufreq.c
>> @@ -240,10 +240,54 @@ static int cppc_cpufreq_cpu_init(struct cpufreq_policy *policy)
>> return ret;
>> }
>>
>> +static int cppc_get_rate_from_fbctrs(struct cppc_perf_fb_ctrs fb_ctrs_t0,
>> + struct cppc_perf_fb_ctrs fb_ctrs_t1)
>> +{
>> + u64 delta_reference, delta_delivered;
>> + u64 reference_perf, ratio;
>> +
>> + reference_perf = fb_ctrs_t0.reference_perf;
>> + if (fb_ctrs_t1.reference > fb_ctrs_t0.reference)
>> + delta_reference = fb_ctrs_t1.reference - fb_ctrs_t0.reference;
>> + else /* Counters would have wrapped-around */
>> + delta_reference = ((u64)(~((u64)0)) - fb_ctrs_t0.reference) +
>> + fb_ctrs_t1.reference;
>> +
>> + if (fb_ctrs_t1.delivered > fb_ctrs_t0.delivered)
>> + delta_delivered = fb_ctrs_t1.delivered - fb_ctrs_t0.delivered;
>> + else /* Counters would have wrapped-around */
>> + delta_delivered = ((u64)(~((u64)0)) - fb_ctrs_t0.delivered) +
>> + fb_ctrs_t1.delivered;
> We need to check that the wraparound time is long enough to make sure that
> the counters cannot wrap around more than once. We can register a get() api
> only after checking that wraparound time value is reasonably high.
>
> I am not aware of any platforms where wraparound time is soo short, but
> wouldn't hurt to check once during init.
By design the wraparound time is a 64 bit counter, for that matter even
all the feedback counters too are 64 bit counters. I don't see any
chance in which the counters can wraparound twice in back to back reads.
The only situation is in which system itself is running at a really high
frequency. Even in that case today's spec is not sufficient to support
the same.
>> +
>> + if (delta_reference) /* Check to avoid divide-by zero */
>> + ratio = (delta_delivered * 1000) / delta_reference;
> Why not just return the computed value here instead of *1000 and later /1000?
> return (ref_per * delta_del) / delta_ref;
Yes.
>> + else
>> + return -EINVAL;
> Instead of EINVAL, i think we should return current frequency.
>
Sorry, I didn't get you, How do you calculate the current frequency?
Did you mean reference performance?
> The counters can pause if CPUs are in idle state during our sampling interval, so
> If the counters did not progress, it is reasonable to assume the delivered perf was
> equal to desired perf.
No, that is wrong. Here the check is for reference performance delta.
This counter can never pause. In case of cpuidle only the delivered
counters could pause. Delivered counters will pause only if the
particular core enters power down mode, Otherwise we would be still
clocking the core and we should be getting a delta across 2 sampling
periods. In case if the reference counter is paused which by design is
not correct then there is no point in returning reference performance
numbers. That too is wrong. In case the low level FW is not updating the
counters properly then it should be evident till Linux, instead of
returning a bogus frequency.
>
> Even if platform wanted to limit, since the CPUs were asleep(idle) we could not have
> observed lower performance, so we will not throw off any logic that could be driven
> using the returned value.
>> +
>> + return (reference_perf * ratio) / 1000;
> This should be converted to KHz as cpufreq is not aware of CPPC abstract scale
In our platform all performance registers are implemented in KHz.
Because of which we never had an issue with conversion. I am not
aware whether ACPI mandates to use any particular unit. How is that
implemented in your platform? Just to avoid any extra conversion don't
you feel it is better to always report in KHz from firmware.
>> +}
>> +
>> +static unsigned int cppc_cpufreq_get_rate(unsigned int cpunum)
>> +{
>> + struct cppc_perf_fb_ctrs fb_ctrs_t0 = {0}, fb_ctrs_t1 = {0};
>> + int ret;
>> +
>> + ret = cppc_get_perf_ctrs(cpunum, &fb_ctrs_t0);
>> + if (ret)
>> + return ret;
>> +
>> + ret = cppc_get_perf_ctrs(cpunum, &fb_ctrs_t1);
>> + if (ret)
>> + return ret;
>> +
>> + return cppc_get_rate_from_fbctrs(fb_ctrs_t0, fb_ctrs_t1);
>> +}
> We need to make sure that we get a reasonably sample so make sure the reported
> performance is accurate.
> The counters can capture short transient throttling/limiting, so by sampling a really
> short duration of time we could return quite inaccurate measure of performance.
>
I would say it as a momentary thing only when the frequency is being
ramped up/down.
> We need to include some reasonable delay between the two calls. What is reasonable
> is debatable - so this can be few(2-10) microseconds defined as default. If the same value
> doesn't work for all the platforms, we might need to add a platform specific value.
>
cppc_get_perf_ctrs itself is a slow call, we have to format the CPC
packet and ring a doorbell and then the response to be read from the
shared registers. My initial implementation had a delay but in testing,
I found that it was unnecessary to have such a delay. Can you please
let me know whether it works without delay in your platform?
Or else let me know whether udelay(10) is sufficient in between the
calls.
>> +
>> static struct cpufreq_driver cppc_cpufreq_driver = {
>> .flags = CPUFREQ_CONST_LOOPS,
>> .verify = cppc_verify_policy,
>> .target = cppc_cpufreq_set_target,
>> + .get = cppc_cpufreq_get_rate,
>> .init = cppc_cpufreq_cpu_init,
>> .stop_cpu = cppc_cpufreq_stop_cpu,
>> .name = "cppc_cpufreq",
>
^ permalink raw reply
* Re: [PATCH v1 2/2] cpufreq: tegra20: Use PLL_C as intermediate clock source
From: Peter De Schrijver @ 2018-05-25 6:32 UTC (permalink / raw)
To: Dmitry Osipenko
Cc: Rafael J. Wysocki, Viresh Kumar, Thierry Reding, Jonathan Hunter,
linux-tegra, linux-pm, linux-kernel
In-Reply-To: <e6ee8116-7f73-fa7a-3df4-b17ccd8dcb0e@gmail.com>
On Thu, May 24, 2018 at 03:49:22PM +0300, Dmitry Osipenko wrote:
> On 24.05.2018 13:04, Peter De Schrijver wrote:
> > On Wed, May 23, 2018 at 07:00:20PM +0300, Dmitry Osipenko wrote:
> >> PLL_C is running at 600MHz which is significantly higher than the 216MHz
> >> of the PLL_P and it is known that PLL_C is always-ON because AHB BUS is
> >> running on that PLL. Let's use PLL_C as intermediate clock source, making
> >> CPU snappier a tad during of the frequency transition.
> >>
> >
> > pll_c isn't necessarily 600Mhz when used as a source for the second display
> > head.
>
> Hmm, indeed.
>
> Even if PLL_C rate will be adjusted, it will be higher than the PLL_P.. won't
> it? That's likely to be good enough.
>
Yes. I think it can be always higher than pll_p, but that assumes the display
driver will always program the highest possible rate for pll_c for a given mode
and then program the display divider to divide it down to the required rate.
> Do you know if any of the available CCLK parents has a glitch-less rate
> switching? I.e. CPU won't hang on the rate switch.
>
Tegra20 doesn't have dynamic ramp PLLs no. So you always have to switch to a
backup clock source before changing the rate of pll_x.
> There is other possible 600MHz source, the PLL_M. Can we use it? This one also
> may become dynamic if we'll consider implementing the memory scaling, but the
> memory frequency probably will fit the transition role pretty well.
I think this should work, but as you mention it may very well be lower than
pll_p if Tegra20 EMC scaling is re-introduced. I think that's why historically
this was never done.
Peter.
^ permalink raw reply
* Re: [PATCH 2/2] ACPI: EC: Dispatch the EC GPE directly on s2idle wake
From: Ulf Hansson @ 2018-05-25 6:49 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: kbuild-all, Linux ACPI, Zhang Rui, Linux PM, LKML, Erik Schmauss,
Bob Moore, Wang, Wendy, kbuild test robot
In-Reply-To: <201805181421.jU5hI6Bo%fengguang.wu@intel.com>
Rafael,
On 18 May 2018 at 08:10, kbuild test robot <lkp@intel.com> wrote:
> Hi Rafael,
>
> I love your patch! Yet something to improve:
>
> [auto build test ERROR on pm/linux-next]
> [also build test ERROR on v4.17-rc5]
> [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/Rafael-J-Wysocki/ACPI-PM-Dispatch-EC-GPE-early-on-s2idle-resume-if-LPS0-_DSM-is-used/20180518-070817
> base: https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
> config: arm64-defconfig (attached as .config)
> compiler: aarch64-linux-gnu-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=arm64
>
> All errors (new ones prefixed by >>):
>
> drivers/acpi/ec.o: In function `acpi_ec_dispatch_gpe':
>>> ec.c:(.text+0x239c): undefined reference to `acpi_dispatch_gpe'
> ec.c:(.text+0x239c): relocation truncated to fit: R_AARCH64_CALL26 against undefined symbol `acpi_dispatch_gpe'
>
> ---
> 0-DAY kernel test infrastructure Open Source Technology Center
> https://lists.01.org/pipermail/kbuild-all Intel Corporation
This breaks arm64 builds, would you mind to drop the offending patch
from your linux-next branch?
In case you haven't got time to check, I think the problem is caused
by CONFIG_ACPI_REDUCED_HARDWARE_ONLY being set for arm64 builds. I can
try to fix the problem, if you are busy and want help with it?
Kind regards
Uffe
^ permalink raw reply
* Re: [PATCH 2/2] ACPI: EC: Dispatch the EC GPE directly on s2idle wake
From: Rafael J. Wysocki @ 2018-05-25 8:08 UTC (permalink / raw)
To: Ulf Hansson
Cc: Rafael J. Wysocki, kbuild-all, Linux ACPI, Zhang Rui, Linux PM,
LKML, Erik Schmauss, Bob Moore, Wang, Wendy, kbuild test robot
In-Reply-To: <CAPDyKFpnrPeG23Q+i6ASCeohGR3vfGQRwQzjwGsKE=c4TyAiQA@mail.gmail.com>
On Fri, May 25, 2018 at 8:49 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> Rafael,
>
> On 18 May 2018 at 08:10, kbuild test robot <lkp@intel.com> wrote:
>> Hi Rafael,
>>
>> I love your patch! Yet something to improve:
>>
>> [auto build test ERROR on pm/linux-next]
>> [also build test ERROR on v4.17-rc5]
>> [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/Rafael-J-Wysocki/ACPI-PM-Dispatch-EC-GPE-early-on-s2idle-resume-if-LPS0-_DSM-is-used/20180518-070817
>> base: https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
>> config: arm64-defconfig (attached as .config)
>> compiler: aarch64-linux-gnu-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=arm64
>>
>> All errors (new ones prefixed by >>):
>>
>> drivers/acpi/ec.o: In function `acpi_ec_dispatch_gpe':
>>>> ec.c:(.text+0x239c): undefined reference to `acpi_dispatch_gpe'
>> ec.c:(.text+0x239c): relocation truncated to fit: R_AARCH64_CALL26 against undefined symbol `acpi_dispatch_gpe'
>>
>> ---
>> 0-DAY kernel test infrastructure Open Source Technology Center
>> https://lists.01.org/pipermail/kbuild-all Intel Corporation
>
> This breaks arm64 builds, would you mind to drop the offending patch
> from your linux-next branch?
Not really, but I can fix it.
> In case you haven't got time to check, I think the problem is caused
> by CONFIG_ACPI_REDUCED_HARDWARE_ONLY being set for arm64 builds. I can
> try to fix the problem, if you are busy and want help with it?
I'll have a look.
^ permalink raw reply
* Re: [PATCH v1 2/2] cpufreq: tegra20: Use PLL_C as intermediate clock source
From: Rafael J. Wysocki @ 2018-05-25 8:14 UTC (permalink / raw)
To: Dmitry Osipenko
Cc: Rafael J. Wysocki, Viresh Kumar, Rafael J. Wysocki,
Thierry Reding, Jonathan Hunter, Peter De Schrijver, linux-tegra,
Linux PM, Linux Kernel Mailing List
In-Reply-To: <5f9364c0-4ef5-0883-583d-42da8f016a80@gmail.com>
On Thu, May 24, 2018 at 2:28 PM, Dmitry Osipenko <digetx@gmail.com> wrote:
> On 24.05.2018 11:01, Rafael J. Wysocki wrote:
>> On Thu, May 24, 2018 at 7:37 AM, Dmitry Osipenko <digetx@gmail.com> wrote:
>>> On 24.05.2018 07:30, Viresh Kumar wrote:
>>>> On 23-05-18, 19:00, Dmitry Osipenko wrote:
>>>>> PLL_C is running at 600MHz which is significantly higher than the 216MHz
>>>>> of the PLL_P and it is known that PLL_C is always-ON because AHB BUS is
>>>>> running on that PLL. Let's use PLL_C as intermediate clock source, making
>>>>> CPU snappier a tad during of the frequency transition.
>>>>>
>>>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>>>>> ---
>>>>> drivers/cpufreq/tegra20-cpufreq.c | 25 +++++++++++++++++++++----
>>>>> 1 file changed, 21 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/drivers/cpufreq/tegra20-cpufreq.c b/drivers/cpufreq/tegra20-cpufreq.c
>>>>> index 3ad6bded6efc..4bf5ba7da40b 100644
>>>>> --- a/drivers/cpufreq/tegra20-cpufreq.c
>>>>> +++ b/drivers/cpufreq/tegra20-cpufreq.c
>>>>> @@ -25,12 +25,13 @@
>>>>> #include <linux/types.h>
>>>>>
>>>>> #define PLL_P_FREQ 216000
>>>>> +#define PLL_C_FREQ 600000
>>>>>
>>>>> static struct cpufreq_frequency_table freq_table[] = {
>>>>> { .frequency = 216000 },
>>>>> { .frequency = 312000 },
>>>>> { .frequency = 456000 },
>>>>> - { .frequency = 608000 },
>>>>> + { .frequency = 600000 },
>>>>> { .frequency = 760000 },
>>>>> { .frequency = 816000 },
>>>>> { .frequency = 912000 },
>>>>> @@ -44,6 +45,7 @@ struct tegra20_cpufreq {
>>>>> struct clk *cpu_clk;
>>>>> struct clk *pll_x_clk;
>>>>> struct clk *pll_p_clk;
>>>>> + struct clk *pll_c_clk;
>>>>> bool pll_x_prepared;
>>>>> };
>>>>>
>>>>> @@ -58,7 +60,10 @@ static unsigned int tegra_get_intermediate(struct cpufreq_policy *policy,
>>>>> if (index == 0 || policy->cur == PLL_P_FREQ)
>>>>> return 0;
>>>>>
>>>>> - return PLL_P_FREQ;
>>>>> + if (index == 3 || policy->cur == PLL_C_FREQ)
>>>>> + return 0;
>>>>
>>>> So we can choose between two different intermediate frequencies ? And
>>>> I didn't like the way magic number 3 is used here. Its prone to errors
>>>> and we better use a macro or something else here.
>>>>
>>>> Like instead of doing index == 3, what about freq_table[index].freq ==
>>>> PLL_C_FREQ ? Same for the previous patch as well.
>>>
>>> The frequency is determined by the parent clock of CCLK (CPU clock), we can
>>> choose between different parents for the CCLK. PLL_C as PLL_P and PLL_X are
>>> among the available parents for the CCLK to choose from and there some others.
>>>
>>> I don't mind to use freq_table[index].freq, though I'd like to keep compiled
>>> assembly minimal where possible. Hence the freq_table should be made constant to
>>> tell compiler that it doesn't need to emit data fetches for the table values and
>>> could embed the constants into the code where appropriate.
>>>
>>> Could we constify the "struct cpufreq_frequency_table" within the cpufreq core?
>>> Seems nothing prevents this (I already tried to constify - there are no
>>> obstacles), unless some cpufreq driver would try to modify
>>> policy->freq_table->... within the cpufreq callback implementation.
>>
>> Some drivers generate frequency tables out of external data
>> unavailable at compile time, like ACPI tables.
>
> Instead of making the table constant itself (with its values), seems we can just
> make the policy->freq_table pointer constant. I'll try to make a patch for that,
> adjusting the pointers in cpufreq core and the drivers. This works for the
> acpi-cpufreq at least.
Honestly, messing up with the whole subsystem in order to avoid an
explicit pointer case doesn't sound right to me.
>
>> But if you know it for the fact that the core doesn't modify the
>> frequency table, you could pass a constant table from the driver to
>> it, can't you?
>>
>
> Yes, but that will require to explicitly silencing the compiler warning about
> const -> non-const pointer conversion (if you're meaning this pointer
> conversion), which generally should be avoided.
Why?
^ permalink raw reply
* Re: [PATCH 8/9] PM / Domains: Add support for multi PM domains per device to genpd
From: Jon Hunter @ 2018-05-25 8:22 UTC (permalink / raw)
To: Ulf Hansson
Cc: Rajendra Nayak, Geert Uytterhoeven, Linux PM, Greg Kroah-Hartman,
Kevin Hilman, Rafael J . Wysocki, Linux Kernel Mailing List,
Todor Tomov, Viresh Kumar, linux-tegra, Vincent Guittot,
Linux ARM
In-Reply-To: <CAPDyKFp3nMoUoM0LfxcSvJKthwrAKK7yCLAkZMGW-oapUsK7GA@mail.gmail.com>
On 24/05/18 22:21, Ulf Hansson wrote:
...
>> OK, so this bit is a to-do as that is not yet exposed AFAICT. I see that you
>> said 'although we need to extend it to cover cleanup of the earlier
>> registered device, via calling device_unregister().' So if we do this then
>> that would be fine.
>
> Let me clarify the changelog. It's not a to-do, as it's already done
> as part of $subject patch.
Yes I see it now. OK, then that's fine.
Jon
--
nvpublic
^ permalink raw reply
* Re: [PATCH v4 1/7] interconnect: Add generic on-chip interconnect API
From: Amit Kucheria @ 2018-05-25 8:26 UTC (permalink / raw)
To: Georgi Djakov
Cc: Linux PM list, gregkh, Mark Rutland, Lorenzo Pieralisi,
Saravana Kannan, seansw, khilman, Michael Turquette,
Rafael J. Wysocki, LKML, Bjorn Andersson, Rob Herring,
linux-arm-msm, davidai, Vincent Guittot, lakml
In-Reply-To: <20180309210958.16672-2-georgi.djakov@linaro.org>
On Fri, Mar 9, 2018 at 11:09 PM, Georgi Djakov <georgi.djakov@linaro.org> wrote:
> This patch introduce a new API to get requirements and configure the
> interconnect buses across the entire chipset to fit with the current
> demand.
>
> The API is using a consumer/provider-based model, where the providers are
> the interconnect buses and the consumers could be various drivers.
> The consumers request interconnect resources (path) between endpoints and
> set the desired constraints on this data flow path. The providers receive
> requests from consumers and aggregate these requests for all master-slave
> pairs on that path. Then the providers configure each participating in the
> topology node according to the requested data flow path, physical links and
> constraints. The topology could be complicated and multi-tiered and is SoC
> specific.
>
> Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org>
> ---
> Documentation/interconnect/interconnect.rst | 96 ++++++
> drivers/Kconfig | 2 +
> drivers/Makefile | 1 +
> drivers/interconnect/Kconfig | 10 +
> drivers/interconnect/Makefile | 1 +
> drivers/interconnect/core.c | 489 ++++++++++++++++++++++++++++
> include/linux/interconnect-provider.h | 109 +++++++
> include/linux/interconnect.h | 40 +++
> 8 files changed, 748 insertions(+)
> create mode 100644 Documentation/interconnect/interconnect.rst
> create mode 100644 drivers/interconnect/Kconfig
> create mode 100644 drivers/interconnect/Makefile
> create mode 100644 drivers/interconnect/core.c
> create mode 100644 include/linux/interconnect-provider.h
> create mode 100644 include/linux/interconnect.h
>
> diff --git a/Documentation/interconnect/interconnect.rst b/Documentation/interconnect/interconnect.rst
> new file mode 100644
> index 000000000000..23eba68e8424
> --- /dev/null
> +++ b/Documentation/interconnect/interconnect.rst
> @@ -0,0 +1,96 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +=====================================
> +GENERIC SYSTEM INTERCONNECT SUBSYSTEM
> +=====================================
> +
> +Introduction
> +------------
> +
> +This framework is designed to provide a standard kernel interface to control
> +the settings of the interconnects on a SoC. These settings can be throughput,
> +latency and priority between multiple interconnected devices or functional
> +blocks. This can be controlled dynamically in order to save power or provide
> +maximum performance.
> +
> +The interconnect bus is a hardware with configurable parameters, which can be
> +set on a data path according to the requests received from various drivers.
> +An example of interconnect buses are the interconnects between various
> +components or functional blocks in chipsets. There can be multiple interconnects
> +on a SoC that can be multi-tiered.
> +
> +Below is a simplified diagram of a real-world SoC interconnect bus topology.
> +
> +::
> +
> + +----------------+ +----------------+
> + | HW Accelerator |--->| M NoC |<---------------+
> + +----------------+ +----------------+ |
> + | | +------------+
> + +-----+ +-------------+ V +------+ | |
> + | DDR | | +--------+ | PCIe | | |
> + +-----+ | | Slaves | +------+ | |
> + ^ ^ | +--------+ | | C NoC |
> + | | V V | |
> + +------------------+ +------------------------+ | | +-----+
> + | |-->| |-->| |-->| CPU |
> + | |-->| |<--| | +-----+
> + | Mem NoC | | S NoC | +------------+
> + | |<--| |---------+ |
> + | |<--| |<------+ | | +--------+
> + +------------------+ +------------------------+ | | +-->| Slaves |
> + ^ ^ ^ ^ ^ | | +--------+
> + | | | | | | V
> + +------+ | +-----+ +-----+ +---------+ +----------------+ +--------+
> + | CPUs | | | GPU | | DSP | | Masters |-->| P NoC |-->| Slaves |
> + +------+ | +-----+ +-----+ +---------+ +----------------+ +--------+
> + |
> + +-------+
> + | Modem |
> + +-------+
> +
> +Terminology
> +-----------
> +
> +Interconnect provider is the software definition of the interconnect hardware.
> +The interconnect providers on the above diagram are M NoC, S NoC, C NoC and Mem
> +NoC.
> +
> +Interconnect node is the software definition of the interconnect hardware
> +port. Each interconnect provider consists of multiple interconnect nodes,
> +which are connected to other SoC components including other interconnect
> +providers. The point on the diagram where the CPUs connects to the memory is
> +called an interconnect node, which belongs to the Mem NoC interconnect provider.
> +
> +Interconnect endpoints are the first or the last element of the path. Every
> +endpoint is a node, but not every node is an endpoint.
> +
> +Interconnect path is everything between two endpoints including all the nodes
> +that have to be traversed to reach from a source to destination node. It may
> +include multiple master-slave pairs across several interconnect providers.
> +
> +Interconnect consumers are the entities which make use of the data paths exposed
> +by the providers. The consumers send requests to providers requesting various
> +throughput, latency and priority. Usually the consumers are device drivers, that
> +send request based on their needs. An example for a consumer is a video decoder
> +that supports various formats and image sizes.
> +
> +Interconnect providers
> +----------------------
> +
> +Interconnect provider is an entity that implements methods to initialize and
> +configure a interconnect bus hardware. The interconnect provider drivers should
> +be registered with the interconnect provider core.
> +
> +The interconnect framework provider API functions are documented in
> +.. kernel-doc:: include/linux/interconnect-provider.h
> +
> +Interconnect consumers
> +----------------------
> +
> +Interconnect consumers are the clients which use the interconnect APIs to
> +get paths between endpoints and set their bandwidth/latency/QoS requirements
> +for these interconnect paths.
> +
This document is missing a section on the locking semantics of the
framework. Does the core ensure that the entire path is locked for
set() to propagate?
> +The interconnect framework consumer API functions are documented in
> +.. kernel-doc:: include/linux/interconnect.h
> diff --git a/drivers/Kconfig b/drivers/Kconfig
> index 879dc0604cba..96a1db022cee 100644
> --- a/drivers/Kconfig
> +++ b/drivers/Kconfig
> @@ -219,4 +219,6 @@ source "drivers/siox/Kconfig"
>
> source "drivers/slimbus/Kconfig"
>
> +source "drivers/interconnect/Kconfig"
> +
> endmenu
> diff --git a/drivers/Makefile b/drivers/Makefile
> index 24cd47014657..0cca95740d9b 100644
> --- a/drivers/Makefile
> +++ b/drivers/Makefile
> @@ -185,3 +185,4 @@ obj-$(CONFIG_TEE) += tee/
> obj-$(CONFIG_MULTIPLEXER) += mux/
> obj-$(CONFIG_UNISYS_VISORBUS) += visorbus/
> obj-$(CONFIG_SIOX) += siox/
> +obj-$(CONFIG_INTERCONNECT) += interconnect/
> diff --git a/drivers/interconnect/Kconfig b/drivers/interconnect/Kconfig
> new file mode 100644
> index 000000000000..a261c7d41deb
> --- /dev/null
> +++ b/drivers/interconnect/Kconfig
> @@ -0,0 +1,10 @@
> +menuconfig INTERCONNECT
> + tristate "On-Chip Interconnect management support"
> + help
> + Support for management of the on-chip interconnects.
> +
> + This framework is designed to provide a generic interface for
> + managing the interconnects in a SoC.
> +
> + If unsure, say no.
> +
> diff --git a/drivers/interconnect/Makefile b/drivers/interconnect/Makefile
> new file mode 100644
> index 000000000000..5edf0ae80818
> --- /dev/null
> +++ b/drivers/interconnect/Makefile
> @@ -0,0 +1 @@
> +obj-$(CONFIG_INTERCONNECT) += core.o
> diff --git a/drivers/interconnect/core.c b/drivers/interconnect/core.c
> new file mode 100644
> index 000000000000..6306e258b9b9
> --- /dev/null
> +++ b/drivers/interconnect/core.c
> @@ -0,0 +1,489 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Interconnect framework core driver
> + *
> + * Copyright (c) 2018, Linaro Ltd.
> + * Author: Georgi Djakov <georgi.djakov@linaro.org>
> + */
> +
> +#include <linux/device.h>
> +#include <linux/idr.h>
> +#include <linux/init.h>
> +#include <linux/interconnect.h>
> +#include <linux/interconnect-provider.h>
> +#include <linux/list.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/slab.h>
> +
> +static DEFINE_IDR(icc_idr);
> +static LIST_HEAD(icc_provider_list);
> +static DEFINE_MUTEX(icc_provider_list_mutex);
> +static DEFINE_MUTEX(icc_path_mutex);
> +
> +/**
> + * struct icc_req - constraints that are attached to each node
> + *
> + * @req_node: entry in list of requests for the particular @node
> + * @node: the interconnect node to which this constraint applies
> + * @avg_bw: an integer describing the average bandwidth in kbps
> + * @peak_bw: an integer describing the peak bandwidth in kbps
> + */
> +struct icc_req {
> + struct hlist_node req_node;
> + struct icc_node *node;
> + u32 avg_bw;
> + u32 peak_bw;
> +};
> +
> +/**
> + * struct icc_path - interconnect path structure
> + * @num_nodes: number of hops (nodes)
> + * @reqs: array of the requests applicable to this path of nodes
> + */
> +struct icc_path {
> + size_t num_nodes;
> + struct icc_req reqs[0];
> +};
> +
> +static struct icc_node *node_find(const int id)
> +{
> + struct icc_node *node;
> +
> + node = idr_find(&icc_idr, id);
> +
> + return node;
> +}
> +
> +static struct icc_path *path_allocate(struct icc_node *node, ssize_t num_nodes)
> +{
> + struct icc_path *path;
> + size_t i;
> +
> + path = kzalloc(sizeof(*path) + num_nodes * sizeof(*path->reqs),
> + GFP_KERNEL);
> + if (!path)
> + return ERR_PTR(-ENOMEM);
> +
> + path->num_nodes = num_nodes;
> +
> + for (i = 0; i < num_nodes; i++) {
> + hlist_add_head(&path->reqs[i].req_node, &node->req_list);
> +
> + path->reqs[i].node = node;
> + /* reference to previous node was saved during path traversal */
> + node = node->reverse;
> + }
> +
> + return path;
> +}
> +
> +static struct icc_path *path_find(struct icc_node *src, struct icc_node *dst)
> +{
> + struct icc_node *node = NULL;
> + struct list_head traverse_list;
> + struct list_head edge_list;
> + struct list_head tmp_list;
> + size_t i, number = 0;
> + bool found = false;
> +
> + INIT_LIST_HEAD(&traverse_list);
> + INIT_LIST_HEAD(&edge_list);
> + INIT_LIST_HEAD(&tmp_list);
> +
> + list_add_tail(&src->search_list, &traverse_list);
> +
> + do {
> + list_for_each_entry(node, &traverse_list, search_list) {
> + if (node == dst) {
> + found = true;
> + list_add(&node->search_list, &tmp_list);
> + break;
> + }
> + for (i = 0; i < node->num_links; i++) {
> + struct icc_node *tmp = node->links[i];
> +
> + if (!tmp)
> + return ERR_PTR(-ENOENT);
> +
> + if (tmp->is_traversed)
> + continue;
> +
> + tmp->is_traversed = true;
> + tmp->reverse = node;
> + list_add_tail(&tmp->search_list, &edge_list);
> + }
> + }
> + if (found)
> + break;
> +
> + list_splice_init(&traverse_list, &tmp_list);
> + list_splice_init(&edge_list, &traverse_list);
> +
> + /* count the number of nodes */
> + number++;
> +
> + } while (!list_empty(&traverse_list));
> +
> + /* reset the traversed state */
> + list_for_each_entry(node, &tmp_list, search_list)
> + node->is_traversed = false;
> +
> + if (found)
> + return path_allocate(dst, number);
> +
> + return ERR_PTR(-EPROBE_DEFER);
> +}
> +
> +static int path_init(struct icc_path *path)
> +{
> + struct icc_node *node;
> + size_t i;
> +
> + for (i = 0; i < path->num_nodes; i++) {
> + node = path->reqs[i].node;
> +
> + mutex_lock(&node->provider->lock);
> + node->provider->users++;
> + mutex_unlock(&node->provider->lock);
> + }
> +
> + return 0;
> +}
> +
Consider adding some comments for node_aggregate and
provider_aggregate's aggregation algorithm
"We want the path to honor all bandwidth requests, so the average
bandwidth requirements from each consumer are aggregated at each node
and provider level. The peak bandwidth requirements will then be the
highest of all the peak bw requests"
or something to the effect that.
> +static void node_aggregate(struct icc_node *node)
> +{
> + struct icc_req *r;
> + u32 agg_avg = 0;
> + u32 agg_peak = 0;
> +
> + hlist_for_each_entry(r, &node->req_list, req_node) {
> + /* sum(averages) and max(peaks) */
> + agg_avg += r->avg_bw;
> + agg_peak = max(agg_peak, r->peak_bw);
> + }
> +
> + node->avg_bw = agg_avg;
> + node->peak_bw = agg_peak;
> +}
> +
> +static void provider_aggregate(struct icc_provider *provider, u32 *avg_bw,
> + u32 *peak_bw)
> +{
> + struct icc_node *n;
> + u32 agg_avg = 0;
> + u32 agg_peak = 0;
> +
> + /* aggregate for the interconnect provider */
You could get rid of this, the function name says as much.
> + list_for_each_entry(n, &provider->nodes, node_list) {
> + /* sum the average and max the peak */
> + agg_avg += n->avg_bw;
> + agg_peak = max(agg_peak, n->peak_bw);
> + }
> +
> + *avg_bw = agg_avg;
> + *peak_bw = agg_peak;
> +}
> +
> +static int constraints_apply(struct icc_path *path)
> +{
> + struct icc_node *next, *prev = NULL;
> + int i;
> +
> + for (i = 0; i < path->num_nodes; i++, prev = next) {
> + struct icc_provider *provider;
> + u32 avg_bw = 0;
> + u32 peak_bw = 0;
> + int ret;
> +
> + next = path->reqs[i].node;
> + /*
> + * Both endpoints should be valid master-slave pairs of the
> + * same interconnect provider that will be configured.
> + */
> + if (!next || !prev)
> + continue;
> +
> + if (next->provider != prev->provider)
> + continue;
> +
> + provider = next->provider;
> + mutex_lock(&provider->lock);
> +
> + /* aggregate requests for the provider */
Get rid of comment.
> + provider_aggregate(provider, &avg_bw, &peak_bw);
> +
> + if (provider->set) {
> + /* set the constraints */
> + ret = provider->set(prev, next, avg_bw, peak_bw);
> + }
> +
> + mutex_unlock(&provider->lock);
> +
> + if (ret)
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +/**
> + * icc_set() - set constraints on an interconnect path between two endpoints
> + * @path: reference to the path returned by icc_get()
> + * @avg_bw: average bandwidth in kbps
> + * @peak_bw: peak bandwidth in kbps
> + *
> + * This function is used by an interconnect consumer to express its own needs
> + * in term of bandwidth and QoS for a previously requested path between two
> + * endpoints. The requests are aggregated and each node is updated accordingly.
> + *
> + * Returns 0 on success, or an approproate error code otherwise.
*appropriate*
> + */
> +int icc_set(struct icc_path *path, u32 avg_bw, u32 peak_bw)
> +{
> + struct icc_node *node;
> + size_t i;
> + int ret;
> +
> + if (!path)
> + return 0;
> +
> + for (i = 0; i < path->num_nodes; i++) {
> + node = path->reqs[i].node;
> +
> + mutex_lock(&icc_path_mutex);
> +
> + /* update the consumer request for this path */
> + path->reqs[i].avg_bw = avg_bw;
> + path->reqs[i].peak_bw = peak_bw;
> +
> + /* aggregate requests for this node */
> + node_aggregate(node);
> +
> + mutex_unlock(&icc_path_mutex);
> + }
> +
> + ret = constraints_apply(path);
> + if (ret)
> + pr_err("interconnect: error applying constraints (%d)", ret);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(icc_set);
> +
> +/**
> + * icc_get() - return a handle for path between two endpoints
> + * @src_id: source device port id
> + * @dst_id: destination device port id
> + *
> + * This function will search for a path between two endpoints and return an
> + * icc_path handle on success. Use icc_put() to release
> + * constraints when the they are not needed anymore.
> + *
> + * Return: icc_path pointer on success, or ERR_PTR() on error
> + */
> +struct icc_path *icc_get(const int src_id, const int dst_id)
> +{
> + struct icc_node *src, *dst;
> + struct icc_path *path = ERR_PTR(-EPROBE_DEFER);
> +
> + src = node_find(src_id);
> + if (!src)
> + goto out;
> +
> + dst = node_find(dst_id);
> + if (!dst)
> + goto out;
> +
> + mutex_lock(&icc_path_mutex);
> + path = path_find(src, dst);
> + mutex_unlock(&icc_path_mutex);
> + if (IS_ERR(path))
> + goto out;
> +
> + path_init(path);
> +
> +out:
> + return path;
> +}
> +EXPORT_SYMBOL_GPL(icc_get);
> +
> +/**
> + * icc_put() - release the reference to the icc_path
> + * @path: interconnect path
> + *
> + * Use this function to release the constraints on a path when the path is
> + * no longer needed. The constraints will be re-aggregated.
> + */
> +void icc_put(struct icc_path *path)
> +{
> + struct icc_node *node;
> + size_t i;
> + int ret;
> +
> + if (!path || WARN_ON_ONCE(IS_ERR(path)))
> + return;
> +
> + ret = icc_set(path, 0, 0);
> + if (ret)
> + pr_err("%s: error (%d)\n", __func__, ret);
> +
> + for (i = 0; i < path->num_nodes; i++) {
> + node = path->reqs[i].node;
> + hlist_del(&path->reqs[i].req_node);
> +
> + mutex_lock(&node->provider->lock);
> + node->provider->users--;
> + mutex_unlock(&node->provider->lock);
> + }
> +
> + kfree(path);
> +}
> +EXPORT_SYMBOL_GPL(icc_put);
> +
> +/**
> + * icc_node_create() - create a node
> + * @id: node id
> + *
> + * Return: icc_node pointer on success, or ERR_PTR() on error
> + */
> +struct icc_node *icc_node_create(int id)
> +{
> + struct icc_node *node;
> +
> + /* check if node already exists */
> + node = node_find(id);
> + if (node)
> + return node;
> +
> + node = kzalloc(sizeof(*node), GFP_KERNEL);
> + if (!node)
> + return ERR_PTR(-ENOMEM);
> +
> + id = idr_alloc(&icc_idr, node, id, id + 1, GFP_KERNEL);
> + if (WARN(id < 0, "couldn't get idr"))
> + return ERR_PTR(id);
> +
> + node->id = id;
> +
> + return node;
> +}
> +EXPORT_SYMBOL_GPL(icc_node_create);
> +
> +/**
> + * icc_link_create() - create a link between two nodes
> + * @src_id: source node id
> + * @dst_id: destination node id
> + *
> + * Return: 0 on success, or an error code otherwise
> + */
> +int icc_link_create(struct icc_node *node, const int dst_id)
> +{
> + struct icc_node *dst;
> + struct icc_node **new;
> + int ret = 0;
> +
> + if (IS_ERR_OR_NULL(node))
> + return PTR_ERR(node);
> +
> + mutex_lock(&node->provider->lock);
> +
> + dst = node_find(dst_id);
> + if (!dst)
> + dst = icc_node_create(dst_id);
> +
> + new = krealloc(node->links,
> + (node->num_links + 1) * sizeof(*node->links),
> + GFP_KERNEL);
> + if (!new) {
> + ret = -ENOMEM;
> + goto out;
> + }
> +
> + node->links = new;
> + node->links[node->num_links++] = dst;
> +
> +out:
> + mutex_unlock(&node->provider->lock);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(icc_link_create);
> +
> +/**
> + * icc_add_node() - add an interconnect node to interconnect provider
> + * @node: pointer to the interconnect node
> + * @provider: pointer to the interconnect provider
> + *
> + * Return: 0 on success, or an error code otherwise
> + */
> +int icc_node_add(struct icc_node *node, struct icc_provider *provider)
> +{
> + if (WARN_ON(!node))
> + return -EINVAL;
> +
> + if (WARN_ON(!provider))
> + return -EINVAL;
> +
> + node->provider = provider;
> +
> + mutex_lock(&provider->lock);
> + list_add_tail(&node->node_list, &provider->nodes);
> + mutex_unlock(&provider->lock);
> +
> + return 0;
> +}
> +
> +/**
> + * icc_add_provider() - add a new interconnect provider
> + * @icc_provider: the interconnect provider that will be added into topology
> + *
> + * Return: 0 on success, or an error code otherwise
> + */
> +int icc_add_provider(struct icc_provider *provider)
> +{
> + if (WARN_ON(!provider))
> + return -EINVAL;
> +
> + if (WARN_ON(!provider->set))
> + return -EINVAL;
> +
> + mutex_init(&provider->lock);
> + INIT_LIST_HEAD(&provider->nodes);
> +
> + mutex_lock(&icc_provider_list_mutex);
> + list_add(&provider->provider_list, &icc_provider_list);
> + mutex_unlock(&icc_provider_list_mutex);
> +
> + dev_dbg(provider->dev, "interconnect provider added to topology\n");
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(icc_add_provider);
> +
> +/**
> + * icc_del_provider() - delete previously added interconnect provider
> + * @icc_provider: the interconnect provider that will be removed from topology
> + *
> + * Return: 0 on success, or an error code otherwise
> + */
> +int icc_del_provider(struct icc_provider *provider)
> +{
> + mutex_lock(&provider->lock);
> + if (provider->users) {
> + pr_warn("interconnect provider still has %d users\n",
> + provider->users);
> + }
> + mutex_unlock(&provider->lock);
> +
> + mutex_lock(&icc_provider_list_mutex);
> + list_del(&provider->provider_list);
> + mutex_unlock(&icc_provider_list_mutex);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(icc_del_provider);
> +
> +MODULE_AUTHOR("Georgi Djakov <georgi.djakov@linaro.org");
> +MODULE_DESCRIPTION("Interconnect Driver Core");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/interconnect-provider.h b/include/linux/interconnect-provider.h
> new file mode 100644
> index 000000000000..779b5b5b1306
> --- /dev/null
> +++ b/include/linux/interconnect-provider.h
> @@ -0,0 +1,109 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (c) 2018, Linaro Ltd.
> + * Author: Georgi Djakov <georgi.djakov@linaro.org>
> + */
> +
> +#ifndef _LINUX_INTERCONNECT_PROVIDER_H
> +#define _LINUX_INTERCONNECT_PROVIDER_H
> +
> +#include <linux/interconnect.h>
> +
> +struct icc_node;
> +
> +/**
> + * struct icc_provider - interconnect provider (controller) entity that might
> + * provide multiple interconnect controls
> + *
> + * @provider_list: list of the registered interconnect providers
> + * @nodes: internal list of the interconnect provider nodes
> + * @set: pointer to device specific set operation function
> + * @dev: the device this interconnect provider belongs to
> + * @lock: lock to provide consistency during aggregation/update of constraints
> + * @users: count of active users
> + * @data: pointer to private data
> + */
> +struct icc_provider {
> + struct list_head provider_list;
> + struct list_head nodes;
> + int (*set)(struct icc_node *src, struct icc_node *dst,
> + u32 avg_bw, u32 peak_bw);
> + struct device *dev;
> + struct mutex lock;
> + int users;
> + void *data;
> +};
> +
> +/**
> + * struct icc_node - entity that is part of the interconnect topology
> + *
> + * @id: platform specific node id
> + * @name: node name used in debugfs
> + * @links: a list of targets where we can go next when traversing
> + * @num_links: number of links to other interconnect nodes
> + * @provider: points to the interconnect provider of this node
> + * @node_list: list of interconnect nodes associated with @provider
> + * @search_list: list used when walking the nodes graph
> + * @reverse: pointer to previous node when walking the nodes graph
> + * @is_traversed: flag that is used when walking the nodes graph
> + * @req_list: a list of QoS constraint requests associated with this node
> + * @avg_bw: aggregated value of average bandwidth
> + * @peak_bw: aggregated value of peak bandwidth
Consider changing to "aggregated value of {average|peak} bandwidth
requests from all consumers"
> + * @data: pointer to private data
> + */
> +struct icc_node {
> + int id;
> + const char *name;
> + struct icc_node **links;
> + size_t num_links;
> +
> + struct icc_provider *provider;
> + struct list_head node_list;
> + struct list_head orphan_list;
> + struct list_head search_list;
> + struct icc_node *reverse;
> + bool is_traversed;
> + struct hlist_head req_list;
> + u32 avg_bw;
> + u32 peak_bw;
> + void *data;
> +};
> +
> +#if IS_ENABLED(CONFIG_INTERCONNECT)
> +
> +struct icc_node *icc_node_create(int id);
> +int icc_node_add(struct icc_node *node, struct icc_provider *provider);
> +int icc_link_create(struct icc_node *node, const int dst_id);
> +int icc_add_provider(struct icc_provider *provider);
> +int icc_del_provider(struct icc_provider *provider);
> +
> +#else
> +
> +static inline struct icc_node *icc_node_create(int id)
> +{
> + return ERR_PTR(-ENOTSUPP);
> +}
> +
> +int icc_node_add(struct icc_node *node, struct icc_provider *provider)
> +{
> + return -ENOTSUPP;
> +}
> +
> +static inline int icc_link_create(struct icc_node *node, const int dst_id)
> +{
> + return -ENOTSUPP;
> +}
> +
> +static inline int icc_add_provider(struct icc_provider *provider)
> +{
> + return -ENOTSUPP;
> +}
> +
> +static inline int icc_del_provider(struct icc_provider *provider)
> +{
> + return -ENOTSUPP;
> +}
> +
> +#endif /* CONFIG_INTERCONNECT */
> +
> +#endif /* _LINUX_INTERCONNECT_PROVIDER_H */
> diff --git a/include/linux/interconnect.h b/include/linux/interconnect.h
> new file mode 100644
> index 000000000000..5a7cf72b76a5
> --- /dev/null
> +++ b/include/linux/interconnect.h
> @@ -0,0 +1,40 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (c) 2018, Linaro Ltd.
> + * Author: Georgi Djakov <georgi.djakov@linaro.org>
> + */
> +
> +#ifndef _LINUX_INTERCONNECT_H
> +#define _LINUX_INTERCONNECT_H
> +
> +#include <linux/types.h>
> +#include <linux/mutex.h>
> +
> +struct icc_path;
> +struct device;
> +
> +#if IS_ENABLED(CONFIG_INTERCONNECT)
> +
> +struct icc_path *icc_get(const int src_id, const int dst_id);
> +void icc_put(struct icc_path *path);
> +int icc_set(struct icc_path *path, u32 avg_bw, u32 peak_bw);
> +
> +#else
> +
> +static inline struct icc_path *icc_get(const int src_id, const int dst_id)
> +{
> + return NULL;
> +}
> +
> +static inline void icc_put(struct icc_path *path)
> +{
> +}
> +
> +static inline int icc_set(struct icc_path *path, u32 avg_bw, u32 peak_bw)
> +{
> + return 0;
> +}
> +
> +#endif /* CONFIG_INTERCONNECT */
> +
> +#endif /* _LINUX_INTERCONNECT_H */
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v4 5/7] interconnect: qcom: Add msm8916 interconnect provider driver
From: Amit Kucheria @ 2018-05-25 8:27 UTC (permalink / raw)
To: Georgi Djakov
Cc: Linux PM list, gregkh, Rafael J. Wysocki, Rob Herring,
Michael Turquette, khilman, Vincent Guittot, Saravana Kannan,
Bjorn Andersson, seansw, davidai, Mark Rutland, Lorenzo Pieralisi,
LKML, lakml, linux-arm-msm
In-Reply-To: <20180309210958.16672-6-georgi.djakov@linaro.org>
On Fri, Mar 9, 2018 at 11:09 PM, Georgi Djakov <georgi.djakov@linaro.org> wrote:
> Add driver for the Qualcomm interconnect buses found in msm8916 based
> platforms.
>
> Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org>
> ---
> drivers/interconnect/Kconfig | 5 +
> drivers/interconnect/Makefile | 1 +
> drivers/interconnect/qcom/Kconfig | 11 +
> drivers/interconnect/qcom/Makefile | 2 +
> drivers/interconnect/qcom/msm8916.c | 482 ++++++++++++++++++++++++++++++++++++
> include/linux/interconnect/qcom.h | 350 ++++++++++++++++++++++++++
> 6 files changed, 851 insertions(+)
> create mode 100644 drivers/interconnect/qcom/Kconfig
> create mode 100644 drivers/interconnect/qcom/msm8916.c
> create mode 100644 include/linux/interconnect/qcom.h
>
> diff --git a/drivers/interconnect/Kconfig b/drivers/interconnect/Kconfig
> index a261c7d41deb..07a8276fa35a 100644
> --- a/drivers/interconnect/Kconfig
> +++ b/drivers/interconnect/Kconfig
> @@ -8,3 +8,8 @@ menuconfig INTERCONNECT
>
> If unsure, say no.
>
> +if INTERCONNECT
> +
> +source "drivers/interconnect/qcom/Kconfig"
> +
> +endif
> diff --git a/drivers/interconnect/Makefile b/drivers/interconnect/Makefile
> index 5edf0ae80818..5971b811c2d7 100644
> --- a/drivers/interconnect/Makefile
> +++ b/drivers/interconnect/Makefile
> @@ -1 +1,2 @@
> obj-$(CONFIG_INTERCONNECT) += core.o
> +obj-$(CONFIG_INTERCONNECT_QCOM) += qcom/
> diff --git a/drivers/interconnect/qcom/Kconfig b/drivers/interconnect/qcom/Kconfig
> new file mode 100644
> index 000000000000..86465dc37bd4
> --- /dev/null
> +++ b/drivers/interconnect/qcom/Kconfig
> @@ -0,0 +1,11 @@
> +config INTERCONNECT_QCOM
> + bool "Qualcomm Network-on-Chip interconnect drivers"
> + depends on INTERCONNECT
> + depends on ARCH_QCOM || COMPILE_TEST
> + default y
> +
> +config INTERCONNECT_QCOM_MSM8916
> + tristate "Qualcomm MSM8916 interconnect driver"
> + depends on INTERCONNECT_QCOM
> + help
> + This is a driver for the Qualcomm Network-on-Chip on msm8916-based platforms.
> diff --git a/drivers/interconnect/qcom/Makefile b/drivers/interconnect/qcom/Makefile
> index 095bdef1ee6e..a0c13a25e8db 100644
> --- a/drivers/interconnect/qcom/Makefile
> +++ b/drivers/interconnect/qcom/Makefile
> @@ -1 +1,3 @@
> obj-y += smd-rpm.o
> +
> +obj-$(CONFIG_INTERCONNECT_QCOM_MSM8916) += msm8916.o
> diff --git a/drivers/interconnect/qcom/msm8916.c b/drivers/interconnect/qcom/msm8916.c
> new file mode 100644
> index 000000000000..d5b54f8261c8
> --- /dev/null
> +++ b/drivers/interconnect/qcom/msm8916.c
> @@ -0,0 +1,482 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2018 Linaro Ltd
> + * Author: Georgi Djakov <georgi.djakov@linaro.org>
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/device.h>
> +#include <linux/io.h>
> +#include <linux/interconnect-provider.h>
> +#include <linux/interconnect/qcom.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +
> +#include "smd-rpm.h"
> +
> +#define RPM_MASTER_FIELD_BW 0x00007762
> +#define RPM_BUS_MASTER_REQ 0x73616d62
> +#define RPM_BUS_SLAVE_REQ 0x766c7362
> +
> +#define to_qcom_provider(_provider) \
> + container_of(_provider, struct qcom_icc_provider, provider)
> +
> +#define DEFINE_QNODE(_name, _id, _port, _buswidth, _ap_owned, \
> + _mas_rpm_id, _slv_rpm_id, _qos_mode, \
> + _numlinks, ...) \
> + static struct qcom_icc_node _name = { \
> + .id = _id, \
> + .name = #_name, \
> + .port = _port, \
> + .buswidth = _buswidth, \
> + .qos_mode = _qos_mode, \
> + .ap_owned = _ap_owned, \
> + .mas_rpm_id = _mas_rpm_id, \
> + .slv_rpm_id = _slv_rpm_id, \
> + .num_links = _numlinks, \
> + .links = { __VA_ARGS__ }, \
> + }
Move this macro definition just above its use below.
> +enum qcom_qos_mode {
> + QCOM_QOS_MODE_BYPASS = 0,
> + QCOM_QOS_MODE_FIXED,
> + QCOM_QOS_MODE_MAX,
> +};
> +
> +struct qcom_icc_provider {
> + struct icc_provider provider;
> + void __iomem *base;
> + struct clk *bus_clk;
> + struct clk *bus_a_clk;
> +};
> +
> +#define MSM8916_MAX_LINKS 8
> +
> +/**
> + * struct qcom_icc_node - Qualcomm specific interconnect nodes
> + * @name: the node name used in debugfs
> + * @links: an array of nodes where we can go next while traversing
> + * @id: a unique node identifier
> + * @num_links: the total number of @links
> + * @port: the offset index into the masters QoS register space
> + * @buswidth: width of the interconnect between a node and the bus
units?
> + * @ap_owned: the AP CPU does the writing to QoS registers
> + * @rpm: reference to the RPM SMD driver
> + * @qos_mode: QoS mode for ap_owned resources
> + * @mas_rpm_id: RPM id for devices that are bus masters
> + * @slv_rpm_id: RPM id for devices that are bus slaves
> + * @rate: current bus clock rate in Hz
> + */
> +struct qcom_icc_node {
> + unsigned char *name;
> + u16 links[MSM8916_MAX_LINKS];
> + u16 id;
> + u16 num_links;
> + u16 port;
> + u16 buswidth;
> + bool ap_owned;
> + struct qcom_smd_rpm *rpm;
> + enum qcom_qos_mode qos_mode;
> + int mas_rpm_id;
> + int slv_rpm_id;
> + u64 rate;
> +};
> +
> +struct qcom_icc_desc {
> + struct qcom_icc_node **nodes;
> + size_t num_nodes;
> +};
> +
> +DEFINE_QNODE(mas_video, 63, 8, 16, 1, -1, -1, QCOM_QOS_MODE_BYPASS, 2, 10000, 10002);
> +DEFINE_QNODE(mas_jpeg, 62, 6, 16, 1, -1, -1, QCOM_QOS_MODE_BYPASS, 2, 10000, 10002);
> +DEFINE_QNODE(mas_vfe, 29, 9, 16, 1, -1, -1, QCOM_QOS_MODE_BYPASS, 2, 10001, 10002);
> +DEFINE_QNODE(mas_mdp, 22, 7, 16, 1, -1, -1, QCOM_QOS_MODE_BYPASS, 2, 10000, 10002);
> +DEFINE_QNODE(mas_qdss_bam, 53, 11, 16, 1, -1, -1, QCOM_QOS_MODE_FIXED, 1, 10009);
> +DEFINE_QNODE(mas_snoc_cfg, 54, 0, 16, 0, 20, -1, QCOM_QOS_MODE_BYPASS, 1, 10009);
> +DEFINE_QNODE(mas_qdss_etr, 60, 10, 16, 1, -1, -1, QCOM_QOS_MODE_FIXED, 1, 10009);
> +DEFINE_QNODE(mm_int_0, 10000, 0, 16, 1, -1, -1, QCOM_QOS_MODE_FIXED, 1, 10003);
> +DEFINE_QNODE(mm_int_1, 10001, 0, 16, 1, -1, -1, QCOM_QOS_MODE_FIXED, 1, 10003);
> +DEFINE_QNODE(mm_int_2, 10002, 0, 16, 1, -1, -1, QCOM_QOS_MODE_FIXED, 1, 10004);
> +DEFINE_QNODE(mm_int_bimc, 10003, 0, 16, 1, -1, -1, QCOM_QOS_MODE_FIXED, 1, 10008);
> +DEFINE_QNODE(snoc_int_0, 10004, 0, 8, 0, 99, 130, QCOM_QOS_MODE_FIXED, 3, 588, 519, 10027);
> +DEFINE_QNODE(snoc_int_1, 10005, 0, 8, 0, 100, 131, QCOM_QOS_MODE_FIXED, 3, 517, 663, 664);
> +DEFINE_QNODE(snoc_int_bimc, 10006, 0, 8, 0, 101, 132, QCOM_QOS_MODE_FIXED, 1, 10007);
> +DEFINE_QNODE(snoc_bimc_0_mas, 10007, 0, 8, 0, 3, -1, QCOM_QOS_MODE_FIXED, 1, 10025);
> +DEFINE_QNODE(snoc_bimc_1_mas, 10008, 0, 16, 1, -1, -1, QCOM_QOS_MODE_FIXED, 1, 10026);
> +DEFINE_QNODE(qdss_int, 10009, 0, 8, 1, -1, -1, QCOM_QOS_MODE_FIXED, 2, 10004, 10006);
> +DEFINE_QNODE(bimc_snoc_slv, 10017, 0, 8, 1, -1, -1, QCOM_QOS_MODE_FIXED, 2, 10004, 10005);
> +DEFINE_QNODE(snoc_pnoc_mas, 10027, 0, 8, 0, -1, -1, QCOM_QOS_MODE_FIXED, 1, 10028);
> +DEFINE_QNODE(pnoc_snoc_slv, 10011, 0, 8, 0, -1, 45, QCOM_QOS_MODE_FIXED, 3, 10004, 10006, 10005);
> +DEFINE_QNODE(slv_srvc_snoc, 587, 0, 8, 0, -1, 29, QCOM_QOS_MODE_FIXED, 0, 0);
> +DEFINE_QNODE(slv_qdss_stm, 588, 0, 4, 0, -1, 30, QCOM_QOS_MODE_FIXED, 0, 0);
> +DEFINE_QNODE(slv_imem, 519, 0, 8, 0, -1, 26, QCOM_QOS_MODE_FIXED, 0, 0);
> +DEFINE_QNODE(slv_apss, 517, 0, 4, 0, -1, 20, QCOM_QOS_MODE_FIXED, 0, 0);
> +DEFINE_QNODE(slv_cats_0, 663, 0, 16, 0, -1, 106, QCOM_QOS_MODE_FIXED, 0, 0);
> +DEFINE_QNODE(slv_cats_1, 664, 0, 8, 0, -1, 107, QCOM_QOS_MODE_FIXED, 0, 0);
> +DEFINE_QNODE(mas_apss, 1, 0, 8, 1, -1, -1, QCOM_QOS_MODE_FIXED, 3, 512, 10016, 514);
> +DEFINE_QNODE(mas_tcu0, 104, 5, 8, 1, -1, -1, QCOM_QOS_MODE_FIXED, 3, 512, 10016, 514);
> +DEFINE_QNODE(mas_tcu1, 105, 6, 8, 1, -1, -1, QCOM_QOS_MODE_FIXED, 3, 512, 10016, 514);
> +DEFINE_QNODE(mas_gfx, 26, 2, 8, 1, -1, -1, QCOM_QOS_MODE_FIXED, 3, 512, 10016, 514);
> +DEFINE_QNODE(bimc_snoc_mas, 10016, 0, 8, 1, -1, -1, QCOM_QOS_MODE_FIXED, 1, 10017);
> +DEFINE_QNODE(snoc_bimc_0_slv, 10025, 0, 8, 0, -1, 24, QCOM_QOS_MODE_FIXED, 1, 512);
> +DEFINE_QNODE(snoc_bimc_1_slv, 10026, 0, 8, 1, -1, -1, QCOM_QOS_MODE_FIXED, 1, 512);
> +DEFINE_QNODE(slv_ebi_ch0, 512, 0, 8, 0, -1, 0, QCOM_QOS_MODE_FIXED, 0, 0);
> +DEFINE_QNODE(slv_apps_l2, 514, 0, 8, 0, -1, -1, QCOM_QOS_MODE_FIXED, 0, 0);
> +DEFINE_QNODE(snoc_pnoc_slv, 10028, 0, 8, 0, -1, -1, QCOM_QOS_MODE_FIXED, 1, 10012);
> +DEFINE_QNODE(pnoc_int_0, 10012, 0, 8, 0, -1, -1, QCOM_QOS_MODE_FIXED, 8, 10010, 10018, 10019, 10020, 10021, 10022, 10023, 10024);
> +DEFINE_QNODE(pnoc_int_1, 10013, 0, 8, 0, -1, -1, QCOM_QOS_MODE_FIXED, 1, 10010);
> +DEFINE_QNODE(pnoc_m_0, 10014, 0, 8, 0, -1, -1, QCOM_QOS_MODE_FIXED, 1, 10012);
> +DEFINE_QNODE(pnoc_m_1, 10015, 0, 8, 0, -1, -1, QCOM_QOS_MODE_FIXED, 1, 10010);
> +DEFINE_QNODE(pnoc_s_0, 10018, 0, 8, 0, -1, -1, QCOM_QOS_MODE_FIXED, 5, 620, 624, 579, 622, 521);
> +DEFINE_QNODE(pnoc_s_1, 10019, 0, 8, 0, -1, -1, QCOM_QOS_MODE_FIXED, 5, 627, 625, 535, 577, 618);
> +DEFINE_QNODE(pnoc_s_2, 10020, 0, 8, 0, -1, -1, QCOM_QOS_MODE_FIXED, 5, 533, 630, 629, 641, 632);
> +DEFINE_QNODE(pnoc_s_3, 10021, 0, 8, 0, -1, -1, QCOM_QOS_MODE_FIXED, 5, 536, 647, 636, 635, 634);
> +DEFINE_QNODE(pnoc_s_4, 10022, 0, 8, 0, -1, -1, QCOM_QOS_MODE_FIXED, 3, 596, 589, 590);
> +DEFINE_QNODE(pnoc_s_8, 10023, 0, 8, 0, -1, -1, QCOM_QOS_MODE_FIXED, 3, 614, 606, 613);
> +DEFINE_QNODE(pnoc_s_9, 10024, 0, 8, 0, -1, -1, QCOM_QOS_MODE_FIXED, 3, 609, 522, 598);
> +DEFINE_QNODE(slv_imem_cfg, 627, 0, 4, 0, -1, -1, QCOM_QOS_MODE_FIXED, 0, 0);
> +DEFINE_QNODE(slv_crypto_0_cfg, 625, 0, 4, 0, -1, -1, QCOM_QOS_MODE_FIXED, 0, 0);
> +DEFINE_QNODE(slv_msg_ram, 535, 0, 4, 0, -1, -1, QCOM_QOS_MODE_FIXED, 0, 0);
> +DEFINE_QNODE(slv_pdm, 577, 0, 4, 0, -1, -1, QCOM_QOS_MODE_FIXED, 0, 0);
> +DEFINE_QNODE(slv_prng, 618, 0, 4, 0, -1, -1, QCOM_QOS_MODE_FIXED, 0, 0);
> +DEFINE_QNODE(slv_clk_ctl, 620, 0, 4, 0, -1, -1, QCOM_QOS_MODE_FIXED, 0, 0);
> +DEFINE_QNODE(slv_mss, 521, 0, 4, 0, -1, -1, QCOM_QOS_MODE_FIXED, 0, 0);
> +DEFINE_QNODE(slv_tlmm, 624, 0, 4, 0, -1, -1, QCOM_QOS_MODE_FIXED, 0, 0);
> +DEFINE_QNODE(slv_tcsr, 579, 0, 4, 0, -1, -1, QCOM_QOS_MODE_FIXED, 0, 0);
> +DEFINE_QNODE(slv_security, 622, 0, 4, 0, -1, -1, QCOM_QOS_MODE_FIXED, 0, 0);
> +DEFINE_QNODE(slv_spdm, 533, 0, 4, 0, -1, -1, QCOM_QOS_MODE_FIXED, 0, 0);
> +DEFINE_QNODE(slv_pnoc_cfg, 641, 0, 4, 0, -1, -1, QCOM_QOS_MODE_FIXED, 0, 0);
> +DEFINE_QNODE(slv_pmic_arb, 632, 0, 4, 0, -1, -1, QCOM_QOS_MODE_FIXED, 0, 0);
> +DEFINE_QNODE(slv_bimc_cfg, 629, 0, 4, 0, -1, -1, QCOM_QOS_MODE_FIXED, 0, 0);
> +DEFINE_QNODE(slv_boot_rom, 630, 0, 4, 0, -1, -1, QCOM_QOS_MODE_FIXED, 0, 0);
> +DEFINE_QNODE(slv_mpm, 536, 0, 4, 0, -1, -1, QCOM_QOS_MODE_FIXED, 0, 0);
> +DEFINE_QNODE(slv_qdss_cfg, 635, 0, 4, 0, -1, -1, QCOM_QOS_MODE_FIXED, 0, 0);
> +DEFINE_QNODE(slv_rbcpr_cfg, 636, 0, 4, 0, -1, -1, QCOM_QOS_MODE_FIXED, 0, 0);
> +DEFINE_QNODE(slv_snoc_cfg, 647, 0, 4, 0, -1, -1, QCOM_QOS_MODE_FIXED, 0, 0);
> +DEFINE_QNODE(slv_dehr_cfg, 634, 0, 4, 0, -1, -1, QCOM_QOS_MODE_FIXED, 0, 0);
> +DEFINE_QNODE(slv_venus_cfg, 596, 0, 4, 0, -1, -1, QCOM_QOS_MODE_FIXED, 0, 0);
> +DEFINE_QNODE(slv_display_cfg, 590, 0, 4, 0, -1, -1, QCOM_QOS_MODE_FIXED, 0, 0);
> +DEFINE_QNODE(slv_camera_cfg, 589, 0, 4, 0, -1, -1, QCOM_QOS_MODE_FIXED, 0, 0);
> +DEFINE_QNODE(slv_usb_hs, 614, 0, 4, 0, -1, -1, QCOM_QOS_MODE_FIXED, 0, 0);
> +DEFINE_QNODE(slv_sdcc_1, 606, 0, 4, 0, -1, -1, QCOM_QOS_MODE_FIXED, 0, 0);
> +DEFINE_QNODE(slv_blsp_1, 613, 0, 4, 0, -1, -1, QCOM_QOS_MODE_FIXED, 0, 0);
> +DEFINE_QNODE(slv_sdcc_2, 609, 0, 4, 0, -1, -1, QCOM_QOS_MODE_FIXED, 0, 0);
> +DEFINE_QNODE(slv_gfx_cfg, 598, 0, 4, 0, -1, -1, QCOM_QOS_MODE_FIXED, 0, 0);
> +DEFINE_QNODE(slv_audio, 522, 0, 4, 0, -1, -1, QCOM_QOS_MODE_FIXED, 0, 0);
> +DEFINE_QNODE(mas_blsp_1, 86, 0, 4, 0, -1, -1, QCOM_QOS_MODE_FIXED, 1, 10015);
> +DEFINE_QNODE(mas_spdm, 36, 0, 4, 0, -1, -1, QCOM_QOS_MODE_FIXED, 1, 10014);
> +DEFINE_QNODE(mas_dehr, 75, 0, 4, 0, -1, -1, QCOM_QOS_MODE_FIXED, 1, 10014);
> +DEFINE_QNODE(mas_audio, 15, 0, 4, 0, -1, -1, QCOM_QOS_MODE_FIXED, 1, 10014);
> +DEFINE_QNODE(mas_usb_hs, 87, 0, 4, 0, -1, -1, QCOM_QOS_MODE_FIXED, 1, 10015);
> +DEFINE_QNODE(mas_pnoc_crypto_0, 55, 0, 8, 0, -1, -1, QCOM_QOS_MODE_FIXED, 1, 10013);
> +DEFINE_QNODE(mas_pnoc_sdcc_1, 78, 7, 8, 0, -1, -1, QCOM_QOS_MODE_FIXED, 1, 10013);
> +DEFINE_QNODE(mas_pnoc_sdcc_2, 81, 8, 8, 0, -1, -1, QCOM_QOS_MODE_FIXED, 1, 10013);
> +DEFINE_QNODE(pnoc_snoc_mas, 10010, 0, 8, 0, 29, -1, QCOM_QOS_MODE_FIXED, 1, 10011);
> +
> +static struct qcom_icc_node *msm8916_snoc_nodes[] = {
> + &mas_video,
> + &mas_jpeg,
> + &mas_vfe,
> + &mas_mdp,
> + &mas_qdss_bam,
> + &mas_snoc_cfg,
> + &mas_qdss_etr,
> + &mm_int_0,
> + &mm_int_1,
> + &mm_int_2,
> + &mm_int_bimc,
> + &snoc_int_0,
> + &snoc_int_1,
> + &snoc_int_bimc,
> + &snoc_bimc_0_mas,
> + &snoc_bimc_1_mas,
> + &qdss_int,
> + &bimc_snoc_slv,
> + &snoc_pnoc_mas,
> + &pnoc_snoc_slv,
> + &slv_srvc_snoc,
> + &slv_qdss_stm,
> + &slv_imem,
> + &slv_apss,
> + &slv_cats_0,
> + &slv_cats_1,
> +};
> +
> +static struct qcom_icc_desc msm8916_snoc = {
> + .nodes = msm8916_snoc_nodes,
> + .num_nodes = ARRAY_SIZE(msm8916_snoc_nodes),
> +};
> +
> +static struct qcom_icc_node *msm8916_bimc_nodes[] = {
> + &mas_apss,
> + &mas_tcu0,
> + &mas_tcu1,
> + &mas_gfx,
> + &bimc_snoc_mas,
> + &snoc_bimc_0_slv,
> + &snoc_bimc_1_slv,
> + &slv_ebi_ch0,
> + &slv_apps_l2,
> +};
> +
> +static struct qcom_icc_desc msm8916_bimc = {
> + .nodes = msm8916_bimc_nodes,
> + .num_nodes = ARRAY_SIZE(msm8916_bimc_nodes),
> +};
> +
> +static struct qcom_icc_node *msm8916_pnoc_nodes[] = {
> + &snoc_pnoc_slv,
> + &pnoc_int_0,
> + &pnoc_int_1,
> + &pnoc_m_0,
> + &pnoc_m_1,
> + &pnoc_s_0,
> + &pnoc_s_1,
> + &pnoc_s_2,
> + &pnoc_s_3,
> + &pnoc_s_4,
> + &pnoc_s_8,
> + &pnoc_s_9,
> + &slv_imem_cfg,
> + &slv_crypto_0_cfg,
> + &slv_msg_ram,
> + &slv_pdm,
> + &slv_prng,
> + &slv_clk_ctl,
> + &slv_mss,
> + &slv_tlmm,
> + &slv_tcsr,
> + &slv_security,
> + &slv_spdm,
> + &slv_pnoc_cfg,
> + &slv_pmic_arb,
> + &slv_bimc_cfg,
> + &slv_boot_rom,
> + &slv_mpm,
> + &slv_qdss_cfg,
> + &slv_rbcpr_cfg,
> + &slv_snoc_cfg,
> + &slv_dehr_cfg,
> + &slv_venus_cfg,
> + &slv_display_cfg,
> + &slv_camera_cfg,
> + &slv_usb_hs,
> + &slv_sdcc_1,
> + &slv_blsp_1,
> + &slv_sdcc_2,
> + &slv_gfx_cfg,
> + &slv_audio,
> + &mas_blsp_1,
> + &mas_spdm,
> + &mas_dehr,
> + &mas_audio,
> + &mas_usb_hs,
> + &mas_pnoc_crypto_0,
> + &mas_pnoc_sdcc_1,
> + &mas_pnoc_sdcc_2,
> + &pnoc_snoc_mas,
> +};
> +
> +static struct qcom_icc_desc msm8916_pnoc = {
> + .nodes = msm8916_pnoc_nodes,
> + .num_nodes = ARRAY_SIZE(msm8916_pnoc_nodes),
> +};
> +
> +static int qcom_icc_init(struct icc_node *node)
> +{
> + struct qcom_icc_provider *qp = to_qcom_provider(node->provider);
> + /* TODO: init qos and priority */
> +
No need to set_rate here before enabling the clock?
> + clk_prepare_enable(qp->bus_clk);
> + clk_prepare_enable(qp->bus_a_clk);
> +
> + return 0;
> +}
> +
> +static int qcom_icc_set(struct icc_node *src, struct icc_node *dst,
> + u32 avg, u32 peak)
> +{
> + struct qcom_icc_provider *qp;
> + struct qcom_icc_node *qn;
> + struct icc_node *node;
> + struct icc_provider *provider;
> + u64 avg_bw;
> + u64 peak_bw;
> + u64 rate = 0;
> + int ret = 0;
> +
> + if (!src)
> + node = dst;
> + else
> + node = src;
> +
> + qn = node->data;
> + provider = node->provider;
> + qp = to_qcom_provider(node->provider);
> +
> + /* convert from kbps to bps */
> + avg_bw = avg * 1000ULL;
> + peak_bw = peak * 1000ULL;
> +
Since the core uses kbps and various SoC HW might use bps (or other
units), perhaps consider providing a macro in the core header such as:
#define icc_units_to_bps(bw) (bw * 1000ULL)
and then move this conversion to the top of the function where you
define the variable
u64 avg_bw = icc_units_to_bps(avg);
u64 peak_bw = icc_units_to_bps(peak);
Since other drivers will end up copying this driver, it might help
prevent silly errors in the drivers and has a side-effect of allowing
the core to change internal units w/o any driver changes down the
line, if needed.
> + /* set bandwidth */
> + if (qn->ap_owned) {
> + /* TODO: set QoS */
> + } else {
> + /* send message to the RPM processor */
> + if (qn->mas_rpm_id != -1) {
> + ret = qcom_icc_rpm_smd_send(QCOM_SMD_RPM_ACTIVE_STATE,
> + RPM_BUS_MASTER_REQ,
> + qn->mas_rpm_id,
> + avg_bw);
> + }
> +
> + if (qn->slv_rpm_id != -1) {
> + ret = qcom_icc_rpm_smd_send(QCOM_SMD_RPM_ACTIVE_STATE,
> + RPM_BUS_SLAVE_REQ,
> + qn->slv_rpm_id,
> + avg_bw);
> + }
> + }
> +
> + rate = max(avg_bw, peak_bw);
> +
> + do_div(rate, qn->buswidth);
> +
> + if (qn->rate != rate) {
> + ret = clk_set_rate(qp->bus_clk, rate);
> + if (ret) {
> + pr_err("set clk rate %lld error %d\n", rate, ret);
> + return ret;
> + }
> +
> + ret = clk_set_rate(qp->bus_a_clk, rate);
> + if (ret) {
> + pr_err("set clk rate %lld error %d\n", rate, ret);
> + return ret;
> + }
> +
> + qn->rate = rate;
> + }
> +
> + return ret;
> +}
> +
> +static int qnoc_probe(struct platform_device *pdev)
> +{
> + const struct qcom_icc_desc *desc;
> + struct qcom_icc_node **qnodes;
> + struct qcom_icc_provider *qp;
> + struct resource *res;
> + struct icc_provider *provider;
> + size_t num_nodes, i;
> + int ret;
> +
> + /* wait for RPM */
> + if (!qcom_icc_rpm_smd_available())
> + return -EPROBE_DEFER;
> +
> + desc = of_device_get_match_data(&pdev->dev);
> + if (!desc)
> + return -EINVAL;
> +
> + qnodes = desc->nodes;
> + num_nodes = desc->num_nodes;
> +
> + qp = devm_kzalloc(&pdev->dev, sizeof(*qp), GFP_KERNEL);
> + if (!qp)
> + return -ENOMEM;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + qp->base = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(qp->base))
> + return PTR_ERR(qp->base);
> +
> + qp->bus_clk = devm_clk_get(&pdev->dev, "bus_clk");
> + if (IS_ERR(qp->bus_clk))
> + return PTR_ERR(qp->bus_clk);
> +
> + qp->bus_a_clk = devm_clk_get(&pdev->dev, "bus_a_clk");
> + if (IS_ERR(qp->bus_a_clk))
> + return PTR_ERR(qp->bus_a_clk);
> +
> + provider = &qp->provider;
> + provider->dev = &pdev->dev;
> + provider->set = &qcom_icc_set;
> + INIT_LIST_HEAD(&provider->nodes);
> + provider->data = qp;
> +
> + ret = icc_add_provider(provider);
> + if (ret) {
> + dev_err(&pdev->dev, "error adding interconnect provider\n");
> + return ret;
> + }
> +
> + for (i = 0; i < num_nodes; i++) {
> + struct icc_node *node;
> + int ret;
> + size_t j;
> +
> + node = icc_node_create(qnodes[i]->id);
> + if (IS_ERR(node)) {
> + ret = PTR_ERR(node);
> + goto err;
> + }
> +
> + node->name = qnodes[i]->name;
> + node->data = qnodes[i];
> + icc_node_add(node, provider);
> +
> + dev_dbg(&pdev->dev, "registered node %p %s %d\n", node,
> + qnodes[i]->name, node->id);
> +
> + /* populate links */
> + for (j = 0; j < qnodes[i]->num_links; j++)
> + if (qnodes[i]->links[j])
> + icc_link_create(node, qnodes[i]->links[j]);
> +
> + ret = qcom_icc_init(node);
> + if (ret)
> + dev_err(&pdev->dev, "%s init error (%d)\n", node->name,
> + ret);
> + }
> +
> + platform_set_drvdata(pdev, provider);
> +
> + return ret;
> +err:
> + icc_del_provider(provider);
> + return ret;
> +}
> +
> +static int qnoc_remove(struct platform_device *pdev)
> +{
> + struct icc_provider *provider = platform_get_drvdata(pdev);
> +
> + icc_del_provider(provider);
> +
> + return 0;
> +}
> +
> +static const struct of_device_id qnoc_of_match[] = {
> + { .compatible = "qcom,msm8916-pnoc", .data = &msm8916_pnoc },
> + { .compatible = "qcom,msm8916-snoc", .data = &msm8916_snoc },
> + { .compatible = "qcom,msm8916-bimc", .data = &msm8916_bimc },
> + { },
> +};
> +MODULE_DEVICE_TABLE(of, qnoc_of_match);
> +
> +static struct platform_driver qnoc_driver = {
> + .probe = qnoc_probe,
> + .remove = qnoc_remove,
> + .driver = {
> + .name = "qnoc-msm8916",
> + .of_match_table = qnoc_of_match,
> + },
> +};
> +module_platform_driver(qnoc_driver);
> +MODULE_AUTHOR("Georgi Djakov <georgi.djakov@linaro.org>");
> +MODULE_DESCRIPTION("Qualcomm msm8916 NoC driver");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/interconnect/qcom.h b/include/linux/interconnect/qcom.h
> new file mode 100644
> index 000000000000..2cd378d2f575
> --- /dev/null
> +++ b/include/linux/interconnect/qcom.h
> @@ -0,0 +1,350 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Qualcomm interconnect IDs
> + *
> + * Copyright (c) 2018, Linaro Ltd.
> + * Author: Georgi Djakov <georgi.djakov@linaro.org>
> + */
> +
> +#ifndef __QCOM_INTERCONNECT_IDS_H
> +#define __QCOM_INTERCONNECT_IDS_H
> +
> +#define FAB_BIMC 0
> +#define FAB_SYS_NOC 1024
> +#define FAB_MMSS_NOC 2048
> +#define FAB_OCMEM_NOC 3072
> +#define FAB_PERIPH_NOC 4096
> +#define FAB_CONFIG_NOC 5120
> +#define FAB_OCMEM_VNOC 6144
> +
> +#define MASTER_AMPSS_M0 1
> +#define MASTER_AMPSS_M1 2
> +#define APPSS_MASTER_FAB_MMSS 3
> +#define APPSS_MASTER_FAB_SYSTEM 4
> +#define SYSTEM_MASTER_FAB_APPSS 5
> +#define MASTER_SPS 6
> +#define MASTER_ADM_PORT0 7
> +#define MASTER_ADM_PORT1 8
> +#define SYSTEM_MASTER_ADM1_PORT0 9
> +#define MASTER_ADM1_PORT1 10
> +#define MASTER_LPASS_PROC 11
> +#define MASTER_MSS_PROCI 12
> +#define MASTER_MSS_PROCD 13
> +#define MASTER_MSS_MDM_PORT0 14
> +#define MASTER_LPASS 15
> +#define SYSTEM_MASTER_CPSS_FPB 16
> +#define SYSTEM_MASTER_SYSTEM_FPB 17
> +#define SYSTEM_MASTER_MMSS_FPB 18
> +#define MASTER_ADM1_CI 19
> +#define MASTER_ADM0_CI 20
> +#define MASTER_MSS_MDM_PORT1 21
> +#define MASTER_MDP_PORT0 22
> +#define MASTER_MDP_PORT1 23
> +#define MMSS_MASTER_ADM1_PORT0 24
> +#define MASTER_ROTATOR 25
> +#define MASTER_GRAPHICS_3D 26
> +#define MASTER_JPEG_DEC 27
> +#define MASTER_GRAPHICS_2D_CORE0 28
> +#define MASTER_VFE 29
> +#define MASTER_VPE 30
> +#define MASTER_JPEG_ENC 31
> +#define MASTER_GRAPHICS_2D_CORE1 32
> +#define MMSS_MASTER_APPS_FAB 33
> +#define MASTER_HD_CODEC_PORT0 34
> +#define MASTER_HD_CODEC_PORT1 35
> +#define MASTER_SPDM 36
> +#define MASTER_RPM 37
> +#define MASTER_MSS 38
> +#define MASTER_RIVA 39
> +#define SYSTEM_MASTER_UNUSED_6 40
> +#define MASTER_MSS_SW_PROC 41
> +#define MASTER_MSS_FW_PROC 42
> +#define MMSS_MASTER_UNUSED_2 43
> +#define MASTER_GSS_NAV 44
> +#define MASTER_PCIE 45
> +#define MASTER_SATA 46
> +#define MASTER_CRYPTO 47
> +#define MASTER_VIDEO_CAP 48
> +#define MASTER_GRAPHICS_3D_PORT1 49
> +#define MASTER_VIDEO_ENC 50
> +#define MASTER_VIDEO_DEC 51
> +#define MASTER_LPASS_AHB 52
> +#define MASTER_QDSS_BAM 53
> +#define MASTER_SNOC_CFG 54
> +#define MASTER_CRYPTO_CORE0 55
> +#define MASTER_CRYPTO_CORE1 56
> +#define MASTER_MSS_NAV 57
> +#define MASTER_OCMEM_DMA 58
> +#define MASTER_WCSS 59
> +#define MASTER_QDSS_ETR 60
> +#define MASTER_USB3 61
> +#define MASTER_JPEG 62
> +#define MASTER_VIDEO_P0 63
> +#define MASTER_VIDEO_P1 64
> +#define MASTER_MSS_PROC 65
> +#define MASTER_JPEG_OCMEM 66
> +#define MASTER_MDP_OCMEM 67
> +#define MASTER_VIDEO_P0_OCMEM 68
> +#define MASTER_VIDEO_P1_OCMEM 69
> +#define MASTER_VFE_OCMEM 70
> +#define MASTER_CNOC_ONOC_CFG 71
> +#define MASTER_RPM_INST 72
> +#define MASTER_RPM_DATA 73
> +#define MASTER_RPM_SYS 74
> +#define MASTER_DEHR 75
> +#define MASTER_QDSS_DAP 76
> +#define MASTER_TIC 77
> +#define MASTER_SDCC_1 78
> +#define MASTER_SDCC_3 79
> +#define MASTER_SDCC_4 80
> +#define MASTER_SDCC_2 81
> +#define MASTER_TSIF 82
> +#define MASTER_BAM_DMA 83
> +#define MASTER_BLSP_2 84
> +#define MASTER_USB_HSIC 85
> +#define MASTER_BLSP_1 86
> +#define MASTER_USB_HS 87
> +#define MASTER_PNOC_CFG 88
> +#define MASTER_V_OCMEM_GFX3D 89
> +#define MASTER_IPA 90
> +#define MASTER_QPIC 91
> +#define MASTER_MDPE 92
> +#define MASTER_USB_HS2 93
> +#define MASTER_VPU 94
> +#define MASTER_UFS 95
> +#define MASTER_BCAST 96
> +#define MASTER_CRYPTO_CORE2 97
> +#define MASTER_EMAC 98
> +#define MASTER_VPU_1 99
> +#define MASTER_PCIE_1 100
> +#define MASTER_USB3_1 101
> +#define MASTER_CNOC_MNOC_MMSS_CFG 102
> +#define MASTER_CNOC_MNOC_CFG 103
> +#define MASTER_TCU_0 104
> +#define MASTER_TCU_1 105
> +#define MASTER_CPP 106
> +#define MASTER_AUDIO 107
> +
> +#define SNOC_MM_INT_0 10000
> +#define SNOC_MM_INT_1 10001
> +#define SNOC_MM_INT_2 10002
> +#define SNOC_MM_INT_BIMC 10003
> +#define SNOC_INT_0 10004
> +#define SNOC_INT_1 10005
> +#define SNOC_INT_BIMC 10006
> +#define SNOC_BIMC_0_MAS 10007
> +#define SNOC_BIMC_1_MAS 10008
> +#define SNOC_QDSS_INT 10009
> +#define PNOC_SNOC_MAS 10010
> +#define PNOC_SNOC_SLV 10011
> +#define PNOC_INT_0 10012
> +#define PNOC_INT_1 10013
> +#define PNOC_M_0 10014
> +#define PNOC_M_1 10015
> +#define BIMC_SNOC_MAS 10016
> +#define BIMC_SNOC_SLV 10017
> +#define PNOC_SLV_0 10018
> +#define PNOC_SLV_1 10019
> +#define PNOC_SLV_2 10020
> +#define PNOC_SLV_3 10021
> +#define PNOC_SLV_4 10022
> +#define PNOC_SLV_8 10023
> +#define PNOC_SLV_9 10024
> +#define SNOC_BIMC_0_SLV 10025
> +#define SNOC_BIMC_1_SLV 10026
> +#define MNOC_BIMC_MAS 10027
> +#define MNOC_BIMC_SLV 10028
> +#define BIMC_MNOC_MAS 10029
> +#define BIMC_MNOC_SLV 10030
> +#define SNOC_BIMC_MAS 10031
> +#define SNOC_BIMC_SLV 10032
> +#define CNOC_SNOC_MAS 10033
> +#define CNOC_SNOC_SLV 10034
> +#define SNOC_CNOC_MAS 10035
> +#define SNOC_CNOC_SLV 10036
> +#define OVNOC_SNOC_MAS 10037
> +#define OVNOC_SNOC_SLV 10038
> +#define SNOC_OVNOC_MAS 10039
> +#define SNOC_OVNOC_SLV 10040
> +#define SNOC_PNOC_MAS 10041
> +#define SNOC_PNOC_SLV 10042
> +#define BIMC_INT_APPS_EBI 10043
> +#define BIMC_INT_APPS_SNOC 10044
> +#define SNOC_BIMC_2_MAS 10045
> +#define SNOC_BIMC_2_SLV 10046
> +#define PNOC_SLV_5 10047
> +#define PNOC_SLV_7 10048
> +#define PNOC_INT_2 10049
> +#define PNOC_INT_3 10050
> +#define PNOC_INT_4 10051
> +#define PNOC_INT_5 10052
> +#define PNOC_INT_6 10053
> +#define PNOC_INT_7 10054
> +
> +#define SLAVE_EBI_CH0 512
> +#define SLAVE_EBI_CH1 513
> +#define SLAVE_AMPSS_L2 514
> +#define APPSS_SLAVE_FAB_MMSS 515
> +#define APPSS_SLAVE_FAB_SYSTEM 516
> +#define SYSTEM_SLAVE_FAB_APPS 517
> +#define SLAVE_SPS 518
> +#define SLAVE_SYSTEM_IMEM 519
> +#define SLAVE_AMPSS 520
> +#define SLAVE_MSS 521
> +#define SLAVE_LPASS 522
> +#define SYSTEM_SLAVE_CPSS_FPB 523
> +#define SYSTEM_SLAVE_SYSTEM_FPB 524
> +#define SYSTEM_SLAVE_MMSS_FPB 525
> +#define SLAVE_CORESIGHT 526
> +#define SLAVE_RIVA 527
> +#define SLAVE_SMI 528
> +#define MMSS_SLAVE_FAB_APPS 529
> +#define MMSS_SLAVE_FAB_APPS_1 530
> +#define SLAVE_MM_IMEM 531
> +#define SLAVE_CRYPTO 532
> +#define SLAVE_SPDM 533
> +#define SLAVE_RPM 534
> +#define SLAVE_RPM_MSG_RAM 535
> +#define SLAVE_MPM 536
> +#define SLAVE_PMIC1_SSBI1_A 537
> +#define SLAVE_PMIC1_SSBI1_B 538
> +#define SLAVE_PMIC1_SSBI1_C 539
> +#define SLAVE_PMIC2_SSBI2_A 540
> +#define SLAVE_PMIC2_SSBI2_B 541
> +#define SLAVE_GSBI1_UART 542
> +#define SLAVE_GSBI2_UART 543
> +#define SLAVE_GSBI3_UART 544
> +#define SLAVE_GSBI4_UART 545
> +#define SLAVE_GSBI5_UART 546
> +#define SLAVE_GSBI6_UART 547
> +#define SLAVE_GSBI7_UART 548
> +#define SLAVE_GSBI8_UART 549
> +#define SLAVE_GSBI9_UART 550
> +#define SLAVE_GSBI10_UART 551
> +#define SLAVE_GSBI11_UART 552
> +#define SLAVE_GSBI12_UART 553
> +#define SLAVE_GSBI1_QUP 554
> +#define SLAVE_GSBI2_QUP 555
> +#define SLAVE_GSBI3_QUP 556
> +#define SLAVE_GSBI4_QUP 557
> +#define SLAVE_GSBI5_QUP 558
> +#define SLAVE_GSBI6_QUP 559
> +#define SLAVE_GSBI7_QUP 560
> +#define SLAVE_GSBI8_QUP 561
> +#define SLAVE_GSBI9_QUP 562
> +#define SLAVE_GSBI10_QUP 563
> +#define SLAVE_GSBI11_QUP 564
> +#define SLAVE_GSBI12_QUP 565
> +#define SLAVE_EBI2_NAND 566
> +#define SLAVE_EBI2_CS0 567
> +#define SLAVE_EBI2_CS1 568
> +#define SLAVE_EBI2_CS2 569
> +#define SLAVE_EBI2_CS3 570
> +#define SLAVE_EBI2_CS4 571
> +#define SLAVE_EBI2_CS5 572
> +#define SLAVE_USB_FS1 573
> +#define SLAVE_USB_FS2 574
> +#define SLAVE_TSIF 575
> +#define SLAVE_MSM_TSSC 576
> +#define SLAVE_MSM_PDM 577
> +#define SLAVE_MSM_DIMEM 578
> +#define SLAVE_MSM_TCSR 579
> +#define SLAVE_MSM_PRNG 580
> +#define SLAVE_GSS 581
> +#define SLAVE_SATA 582
> +#define SLAVE_USB3 583
> +#define SLAVE_WCSS 584
> +#define SLAVE_OCIMEM 585
> +#define SLAVE_SNOC_OCMEM 586
> +#define SLAVE_SERVICE_SNOC 587
> +#define SLAVE_QDSS_STM 588
> +#define SLAVE_CAMERA_CFG 589
> +#define SLAVE_DISPLAY_CFG 590
> +#define SLAVE_OCMEM_CFG 591
> +#define SLAVE_CPR_CFG 592
> +#define SLAVE_CPR_XPU_CFG 593
> +#define SLAVE_MISC_CFG 594
> +#define SLAVE_MISC_XPU_CFG 595
> +#define SLAVE_VENUS_CFG 596
> +#define SLAVE_MISC_VENUS_CFG 597
> +#define SLAVE_GRAPHICS_3D_CFG 598
> +#define SLAVE_MMSS_CLK_CFG 599
> +#define SLAVE_MMSS_CLK_XPU_CFG 600
> +#define SLAVE_MNOC_MPU_CFG 601
> +#define SLAVE_ONOC_MPU_CFG 602
> +#define SLAVE_SERVICE_MNOC 603
> +#define SLAVE_OCMEM 604
> +#define SLAVE_SERVICE_ONOC 605
> +#define SLAVE_SDCC_1 606
> +#define SLAVE_SDCC_3 607
> +#define SLAVE_SDCC_2 608
> +#define SLAVE_SDCC_4 609
> +#define SLAVE_BAM_DMA 610
> +#define SLAVE_BLSP_2 611
> +#define SLAVE_USB_HSIC 612
> +#define SLAVE_BLSP_1 613
> +#define SLAVE_USB_HS 614
> +#define SLAVE_PDM 615
> +#define SLAVE_PERIPH_APU_CFG 616
> +#define SLAVE_PNOC_MPU_CFG 617
> +#define SLAVE_PRNG 618
> +#define SLAVE_SERVICE_PNOC 619
> +#define SLAVE_CLK_CTL 620
> +#define SLAVE_CNOC_MSS 621
> +#define SLAVE_SECURITY 622
> +#define SLAVE_TCSR 623
> +#define SLAVE_TLMM 624
> +#define SLAVE_CRYPTO_0_CFG 625
> +#define SLAVE_CRYPTO_1_CFG 626
> +#define SLAVE_IMEM_CFG 627
> +#define SLAVE_MESSAGE_RAM 628
> +#define SLAVE_BIMC_CFG 629
> +#define SLAVE_BOOT_ROM 630
> +#define SLAVE_CNOC_MNOC_MMSS_CFG 631
> +#define SLAVE_PMIC_ARB 632
> +#define SLAVE_SPDM_WRAPPER 633
> +#define SLAVE_DEHR_CFG 634
> +#define SLAVE_QDSS_CFG 635
> +#define SLAVE_RBCPR_CFG 636
> +#define SLAVE_RBCPR_QDSS_APU_CFG 637
> +#define SLAVE_SNOC_MPU_CFG 638
> +#define SLAVE_CNOC_ONOC_CFG 639
> +#define SLAVE_CNOC_MNOC_CFG 640
> +#define SLAVE_PNOC_CFG 641
> +#define SLAVE_SNOC_CFG 642
> +#define SLAVE_EBI1_DLL_CFG 643
> +#define SLAVE_PHY_APU_CFG 644
> +#define SLAVE_EBI1_PHY_CFG 645
> +#define SLAVE_SERVICE_CNOC 646
> +#define SLAVE_IPS_CFG 647
> +#define SLAVE_QPIC 648
> +#define SLAVE_DSI_CFG 649
> +#define SLAVE_UFS_CFG 650
> +#define SLAVE_RBCPR_CX_CFG 651
> +#define SLAVE_RBCPR_MX_CFG 652
> +#define SLAVE_PCIE_CFG 653
> +#define SLAVE_USB_PHYS_CFG 654
> +#define SLAVE_VIDEO_CAP_CFG 655
> +#define SLAVE_AVSYNC_CFG 656
> +#define SLAVE_CRYPTO_2_CFG 657
> +#define SLAVE_VPU_CFG 658
> +#define SLAVE_BCAST_CFG 659
> +#define SLAVE_KLM_CFG 660
> +#define SLAVE_GENI_IR_CFG 661
> +#define SLAVE_OCMEM_GFX 662
> +#define SLAVE_CATS_128 663
> +#define SLAVE_OCMEM_64 664
> +#define SLAVE_PCIE_0 665
> +#define SLAVE_PCIE_1 666
> +#define SLAVE_PCIE_0_CFG 667
> +#define SLAVE_PCIE_1_CFG 668
> +#define SLAVE_SRVC_MNOC 669
> +#define SLAVE_USB_HS2 670
> +#define SLAVE_AUDIO 671
> +#define SLAVE_TCU 672
> +#define SLAVE_APPSS 673
> +#define SLAVE_PCIE_PARF 674
> +#define SLAVE_USB3_PHY_CFG 675
> +#define SLAVE_IPA_CFG 676
> +
> +#endif
^ permalink raw reply
* Re: [PATCH v1 2/2] cpufreq: tegra20: Use PLL_C as intermediate clock source
From: Dmitry Osipenko @ 2018-05-25 8:28 UTC (permalink / raw)
To: Peter De Schrijver
Cc: Rafael J. Wysocki, Viresh Kumar, Thierry Reding, Jonathan Hunter,
linux-tegra, linux-pm, linux-kernel, Peter Geis
In-Reply-To: <20180525063225.GN6835@tbergstrom-lnx.Nvidia.com>
On 25.05.2018 09:32, Peter De Schrijver wrote:
> On Thu, May 24, 2018 at 03:49:22PM +0300, Dmitry Osipenko wrote:
>> On 24.05.2018 13:04, Peter De Schrijver wrote:
>>> On Wed, May 23, 2018 at 07:00:20PM +0300, Dmitry Osipenko wrote:
>>>> PLL_C is running at 600MHz which is significantly higher than the 216MHz
>>>> of the PLL_P and it is known that PLL_C is always-ON because AHB BUS is
>>>> running on that PLL. Let's use PLL_C as intermediate clock source, making
>>>> CPU snappier a tad during of the frequency transition.
>>>>
>>>
>>> pll_c isn't necessarily 600Mhz when used as a source for the second display
>>> head.
>>
>> Hmm, indeed.
>>
>> Even if PLL_C rate will be adjusted, it will be higher than the PLL_P.. won't
>> it? That's likely to be good enough.
>>
>
> Yes. I think it can be always higher than pll_p, but that assumes the display
> driver will always program the highest possible rate for pll_c for a given mode
> and then program the display divider to divide it down to the required rate.
>
>> Do you know if any of the available CCLK parents has a glitch-less rate
>> switching? I.e. CPU won't hang on the rate switch.
>>
>
> Tegra20 doesn't have dynamic ramp PLLs no. So you always have to switch to a
> backup clock source before changing the rate of pll_x.
>
>> There is other possible 600MHz source, the PLL_M. Can we use it? This one also
>> may become dynamic if we'll consider implementing the memory scaling, but the
>> memory frequency probably will fit the transition role pretty well.
>
> I think this should work, but as you mention it may very well be lower than
> pll_p if Tegra20 EMC scaling is re-introduced. I think that's why historically
> this was never done.
>
> Peter.
>
Okay, thank you very much for the input. Let's put on hold these patches then.
Peter Geis just made a cpufreq driver for the Tegra30 and turned out it's just a
dozen lines of code that we'll have to add to the tegra20-cpufreq driver to
support Tegra30. The tegra20-cpufreq driver code suits very well for the Tegra30
integration, use of PLL_C for the intermediate scaling will make that
integration a bit messy. I'll return to considering of the different
transition-PLL variant after adding the Tegra30 support.
^ permalink raw reply
* Re: [PATCH 9/9] PM / Domains: Add dev_pm_domain_attach_by_id() to manage multi PM domains
From: Jon Hunter @ 2018-05-25 8:31 UTC (permalink / raw)
To: Ulf Hansson
Cc: Rafael J . Wysocki, Linux PM, Greg Kroah-Hartman,
Geert Uytterhoeven, Todor Tomov, Rajendra Nayak, Viresh Kumar,
Vincent Guittot, Kevin Hilman, Linux Kernel Mailing List,
Linux ARM, linux-tegra
In-Reply-To: <CAPDyKFozm88OM8cDFXXwvgdMirrt2=HhZtBdyhNjo2bgjHV2Mg@mail.gmail.com>
On 24/05/18 22:11, Ulf Hansson wrote:
> On 24 May 2018 at 17:48, Jon Hunter <jonathanh@nvidia.com> wrote:
>>
>> On 18/05/18 11:31, Ulf Hansson wrote:
>>>
>>> The existing dev_pm_domain_attach() function, allows a single PM domain to
>>> be attached per device. To be able to support devices that are partitioned
>>> across multiple PM domains, let's introduce a new interface,
>>> dev_pm_domain_attach_by_id().
>>>
>>> The dev_pm_domain_attach_by_id() returns a new allocated struct device
>>> with
>>> the corresponding attached PM domain. This enables for example a driver to
>>> operate on the new device from a power management point of view. The
>>> driver
>>> may then also benefit from using the received device, to set up so called
>>> device-links towards its original device. Depending on the situation,
>>> these
>>> links may then be dynamically changed.
>>>
>>> The new interface is typically called by drivers during their probe phase,
>>> in case they manages devices which uses multiple PM domains. If that is
>>> the
>>> case, the driver also becomes responsible of managing the detaching of the
>>> PM domains, which typically should be done at the remove phase. Detaching
>>> is done by calling the existing dev_pm_domain_detach() function and for
>>> each of the received devices from dev_pm_domain_attach_by_id().
>>>
>>> Note, currently its only genpd that supports multiple PM domains per
>>> device, but dev_pm_domain_attach_by_id() can easily by extended to cover
>>> other PM domain types, if/when needed.
>>>
>>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>>> ---
>>> drivers/base/power/common.c | 33 ++++++++++++++++++++++++++++++++-
>>> include/linux/pm_domain.h | 7 +++++++
>>> 2 files changed, 39 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/base/power/common.c b/drivers/base/power/common.c
>>> index 7ae62b6..d3db974 100644
>>> --- a/drivers/base/power/common.c
>>> +++ b/drivers/base/power/common.c
>>> @@ -117,13 +117,44 @@ int dev_pm_domain_attach(struct device *dev, bool
>>> power_on)
>>> EXPORT_SYMBOL_GPL(dev_pm_domain_attach);
>>> /**
>>> + * dev_pm_domain_attach_by_id - Attach a device to one of its PM domains.
>>
>>
>> Isn't this more of a 'get'?
>
> I don't think so, at least according to the common understanding of
> how we use get and put APIs.
>
> For example, clk_get() returns a cookie to a clk, which you then can
> do a hole bunch of different clk specific operations on.
>
> This is different, there are no specific PM domain operations the
> caller can or should do. Instead the idea is to keep drivers more or
> less transparent, still using runtime PM as before. The only care the
> caller need to take is to use device links, which BTW isn't a PM
> domain specific thing.
Yes, but a user can still call pm_runtime_get/put with the device
returned if they wish to, right?
>>
>>> + * @index: The index of the PM domain.
>>> + * @dev: Device to attach.
>>
>>
>> Isn't this just the device associated with the PM domain we are getting?
>
> Correct, but please note, as stated above, I don't think it's a good
> idea to return a special PM domain cookie, because we don't want the
> caller to run special PM domain operations.
>
>>
>>> + *
>>> + * As @dev may only be attached to a single PM domain, the backend PM
>>> domain
>>> + * provider should create a virtual device to attach instead. As
>>> attachment
>>> + * succeeds, the ->detach() callback in the struct dev_pm_domain should
>>> be
>>> + * assigned by the corresponding backend attach function.
>>> + *
>>> + * This function should typically be invoked from drivers during probe
>>> phase.
>>> + * Especially for those that manages devices which requires power
>>> management
>>> + * through more than one PM domain.
>>> + *
>>> + * Callers must ensure proper synchronization of this function with power
>>> + * management callbacks.
>>> + *
>>> + * Returns the virtual attached device in case successfully attached PM
>>> domain,
>>> + * NULL in case @dev don't need a PM domain, else a PTR_ERR().
>>
>>
>> Should this be 'NULL in the case where the @dev already has a power-domain'?
>
> Yes, I think so. The intent is to be consistent with how
> dev_pm_domain_attach() works and according to the latest changes I did
> for it. It's queued for 4.18, please have a look in Rafael's tree and
> you will see.
Ah, I see your change now.
>>
>>> + */
>>> +struct device *dev_pm_domain_attach_by_id(struct device *dev,
>>> + unsigned int index)
>>> +{
>>> + if (dev->pm_domain)
>>
>>
>> I wonder if this is worthy of a ...
>>
>> if (WARN_ON(dev->pm_domain))
>>
>>> + return NULL;
>>
>>
>> Don't we consider this an error case? I wonder why not return PTR_ERR here
>> as well? This would be consistent with dev_pm_domain_attach().
>
> Please see above comment.
Right, but this case still seems like an error. My understanding is that
only drivers will use this API directly and it will not be used by the
device driver core (unlike dev_pm_domain_attach), so if anyone calls
this attempting to attach another PM domain when one is already
attached, they are doing something wrong.
Cheers
Jon
--
nvpublic
^ permalink raw reply
* Re: [PATCH v1 2/2] cpufreq: tegra20: Use PLL_C as intermediate clock source
From: Rafael J. Wysocki @ 2018-05-25 8:36 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Dmitry Osipenko, Viresh Kumar, Rafael J. Wysocki, Thierry Reding,
Jonathan Hunter, Peter De Schrijver, linux-tegra, Linux PM,
Linux Kernel Mailing List
In-Reply-To: <CAJZ5v0j80jbqLKORZR87mftbKDbAophywnuDqTfNf-VSBYfyOQ@mail.gmail.com>
On Fri, May 25, 2018 at 10:14 AM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Thu, May 24, 2018 at 2:28 PM, Dmitry Osipenko <digetx@gmail.com> wrote:
>> On 24.05.2018 11:01, Rafael J. Wysocki wrote:
>>> On Thu, May 24, 2018 at 7:37 AM, Dmitry Osipenko <digetx@gmail.com> wrote:
>>>> On 24.05.2018 07:30, Viresh Kumar wrote:
>>>>> On 23-05-18, 19:00, Dmitry Osipenko wrote:
>>>>>> PLL_C is running at 600MHz which is significantly higher than the 216MHz
>>>>>> of the PLL_P and it is known that PLL_C is always-ON because AHB BUS is
>>>>>> running on that PLL. Let's use PLL_C as intermediate clock source, making
>>>>>> CPU snappier a tad during of the frequency transition.
>>>>>>
>>>>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>>>>>> ---
>>>>>> drivers/cpufreq/tegra20-cpufreq.c | 25 +++++++++++++++++++++----
>>>>>> 1 file changed, 21 insertions(+), 4 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/cpufreq/tegra20-cpufreq.c b/drivers/cpufreq/tegra20-cpufreq.c
>>>>>> index 3ad6bded6efc..4bf5ba7da40b 100644
>>>>>> --- a/drivers/cpufreq/tegra20-cpufreq.c
>>>>>> +++ b/drivers/cpufreq/tegra20-cpufreq.c
>>>>>> @@ -25,12 +25,13 @@
>>>>>> #include <linux/types.h>
>>>>>>
>>>>>> #define PLL_P_FREQ 216000
>>>>>> +#define PLL_C_FREQ 600000
>>>>>>
>>>>>> static struct cpufreq_frequency_table freq_table[] = {
>>>>>> { .frequency = 216000 },
>>>>>> { .frequency = 312000 },
>>>>>> { .frequency = 456000 },
>>>>>> - { .frequency = 608000 },
>>>>>> + { .frequency = 600000 },
>>>>>> { .frequency = 760000 },
>>>>>> { .frequency = 816000 },
>>>>>> { .frequency = 912000 },
>>>>>> @@ -44,6 +45,7 @@ struct tegra20_cpufreq {
>>>>>> struct clk *cpu_clk;
>>>>>> struct clk *pll_x_clk;
>>>>>> struct clk *pll_p_clk;
>>>>>> + struct clk *pll_c_clk;
>>>>>> bool pll_x_prepared;
>>>>>> };
>>>>>>
>>>>>> @@ -58,7 +60,10 @@ static unsigned int tegra_get_intermediate(struct cpufreq_policy *policy,
>>>>>> if (index == 0 || policy->cur == PLL_P_FREQ)
>>>>>> return 0;
>>>>>>
>>>>>> - return PLL_P_FREQ;
>>>>>> + if (index == 3 || policy->cur == PLL_C_FREQ)
>>>>>> + return 0;
>>>>>
>>>>> So we can choose between two different intermediate frequencies ? And
>>>>> I didn't like the way magic number 3 is used here. Its prone to errors
>>>>> and we better use a macro or something else here.
>>>>>
>>>>> Like instead of doing index == 3, what about freq_table[index].freq ==
>>>>> PLL_C_FREQ ? Same for the previous patch as well.
>>>>
>>>> The frequency is determined by the parent clock of CCLK (CPU clock), we can
>>>> choose between different parents for the CCLK. PLL_C as PLL_P and PLL_X are
>>>> among the available parents for the CCLK to choose from and there some others.
>>>>
>>>> I don't mind to use freq_table[index].freq, though I'd like to keep compiled
>>>> assembly minimal where possible. Hence the freq_table should be made constant to
>>>> tell compiler that it doesn't need to emit data fetches for the table values and
>>>> could embed the constants into the code where appropriate.
>>>>
>>>> Could we constify the "struct cpufreq_frequency_table" within the cpufreq core?
>>>> Seems nothing prevents this (I already tried to constify - there are no
>>>> obstacles), unless some cpufreq driver would try to modify
>>>> policy->freq_table->... within the cpufreq callback implementation.
>>>
>>> Some drivers generate frequency tables out of external data
>>> unavailable at compile time, like ACPI tables.
>>
>> Instead of making the table constant itself (with its values), seems we can just
>> make the policy->freq_table pointer constant. I'll try to make a patch for that,
>> adjusting the pointers in cpufreq core and the drivers. This works for the
>> acpi-cpufreq at least.
>
> Honestly, messing up with the whole subsystem in order to avoid an
> explicit pointer case doesn't sound right to me.
Actually, on a second thought I agree that it is better to do it as
you suggested: make the policy->freq_table pointer constant
everywhere.
Sorry for the noise.
^ permalink raw reply
* Re: [PATCH v1 2/2] cpufreq: tegra20: Use PLL_C as intermediate clock source
From: Dmitry Osipenko @ 2018-05-25 8:41 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Viresh Kumar, Rafael J. Wysocki, Thierry Reding, Jonathan Hunter,
Peter De Schrijver, linux-tegra, Linux PM,
Linux Kernel Mailing List
In-Reply-To: <CAJZ5v0hcvSFfWVCgVU8ApvODhyZqXduaGoBmmh345aGe5MN59g@mail.gmail.com>
On 25.05.2018 11:36, Rafael J. Wysocki wrote:
> On Fri, May 25, 2018 at 10:14 AM, Rafael J. Wysocki <rafael@kernel.org> wrote:
>> On Thu, May 24, 2018 at 2:28 PM, Dmitry Osipenko <digetx@gmail.com> wrote:
>>> On 24.05.2018 11:01, Rafael J. Wysocki wrote:
>>>> On Thu, May 24, 2018 at 7:37 AM, Dmitry Osipenko <digetx@gmail.com> wrote:
>>>>> On 24.05.2018 07:30, Viresh Kumar wrote:
>>>>>> On 23-05-18, 19:00, Dmitry Osipenko wrote:
>>>>>>> PLL_C is running at 600MHz which is significantly higher than the 216MHz
>>>>>>> of the PLL_P and it is known that PLL_C is always-ON because AHB BUS is
>>>>>>> running on that PLL. Let's use PLL_C as intermediate clock source, making
>>>>>>> CPU snappier a tad during of the frequency transition.
>>>>>>>
>>>>>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>>>>>>> ---
>>>>>>> drivers/cpufreq/tegra20-cpufreq.c | 25 +++++++++++++++++++++----
>>>>>>> 1 file changed, 21 insertions(+), 4 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/cpufreq/tegra20-cpufreq.c b/drivers/cpufreq/tegra20-cpufreq.c
>>>>>>> index 3ad6bded6efc..4bf5ba7da40b 100644
>>>>>>> --- a/drivers/cpufreq/tegra20-cpufreq.c
>>>>>>> +++ b/drivers/cpufreq/tegra20-cpufreq.c
>>>>>>> @@ -25,12 +25,13 @@
>>>>>>> #include <linux/types.h>
>>>>>>>
>>>>>>> #define PLL_P_FREQ 216000
>>>>>>> +#define PLL_C_FREQ 600000
>>>>>>>
>>>>>>> static struct cpufreq_frequency_table freq_table[] = {
>>>>>>> { .frequency = 216000 },
>>>>>>> { .frequency = 312000 },
>>>>>>> { .frequency = 456000 },
>>>>>>> - { .frequency = 608000 },
>>>>>>> + { .frequency = 600000 },
>>>>>>> { .frequency = 760000 },
>>>>>>> { .frequency = 816000 },
>>>>>>> { .frequency = 912000 },
>>>>>>> @@ -44,6 +45,7 @@ struct tegra20_cpufreq {
>>>>>>> struct clk *cpu_clk;
>>>>>>> struct clk *pll_x_clk;
>>>>>>> struct clk *pll_p_clk;
>>>>>>> + struct clk *pll_c_clk;
>>>>>>> bool pll_x_prepared;
>>>>>>> };
>>>>>>>
>>>>>>> @@ -58,7 +60,10 @@ static unsigned int tegra_get_intermediate(struct cpufreq_policy *policy,
>>>>>>> if (index == 0 || policy->cur == PLL_P_FREQ)
>>>>>>> return 0;
>>>>>>>
>>>>>>> - return PLL_P_FREQ;
>>>>>>> + if (index == 3 || policy->cur == PLL_C_FREQ)
>>>>>>> + return 0;
>>>>>>
>>>>>> So we can choose between two different intermediate frequencies ? And
>>>>>> I didn't like the way magic number 3 is used here. Its prone to errors
>>>>>> and we better use a macro or something else here.
>>>>>>
>>>>>> Like instead of doing index == 3, what about freq_table[index].freq ==
>>>>>> PLL_C_FREQ ? Same for the previous patch as well.
>>>>>
>>>>> The frequency is determined by the parent clock of CCLK (CPU clock), we can
>>>>> choose between different parents for the CCLK. PLL_C as PLL_P and PLL_X are
>>>>> among the available parents for the CCLK to choose from and there some others.
>>>>>
>>>>> I don't mind to use freq_table[index].freq, though I'd like to keep compiled
>>>>> assembly minimal where possible. Hence the freq_table should be made constant to
>>>>> tell compiler that it doesn't need to emit data fetches for the table values and
>>>>> could embed the constants into the code where appropriate.
>>>>>
>>>>> Could we constify the "struct cpufreq_frequency_table" within the cpufreq core?
>>>>> Seems nothing prevents this (I already tried to constify - there are no
>>>>> obstacles), unless some cpufreq driver would try to modify
>>>>> policy->freq_table->... within the cpufreq callback implementation.
>>>>
>>>> Some drivers generate frequency tables out of external data
>>>> unavailable at compile time, like ACPI tables.
>>>
>>> Instead of making the table constant itself (with its values), seems we can just
>>> make the policy->freq_table pointer constant. I'll try to make a patch for that,
>>> adjusting the pointers in cpufreq core and the drivers. This works for the
>>> acpi-cpufreq at least.
>>
>> Honestly, messing up with the whole subsystem in order to avoid an
>> explicit pointer case doesn't sound right to me.
>
> Actually, on a second thought I agree that it is better to do it as
> you suggested: make the policy->freq_table pointer constant
> everywhere.
>
> Sorry for the noise.
No worries.
As I wrote in the reply to the other patch, the Tegra30 support is now on the
way. These changes will collide a tad with the support integration, so I'll
return to re-considering the changes made in this patchset after Tegra30 support
will land. Thank you very much for your reviews and suggestions, I'll take them
into account in the next iteration.
^ permalink raw reply
* Re: [PATCH] cpufreq / CPPC: Add cpuinfo_cur_freq support for CPPC
From: Rafael J. Wysocki @ 2018-05-25 8:46 UTC (permalink / raw)
To: George Cherian
Cc: Prakash, Prashanth, George Cherian, Linux Kernel Mailing List,
Linux PM, Rafael J. Wysocki, Viresh Kumar
In-Reply-To: <5ff2bebc-fc39-dc82-d7e6-0483e38f7d9c@caviumnetworks.com>
On Fri, May 25, 2018 at 8:27 AM, George Cherian
<gcherian@caviumnetworks.com> wrote:
> Hi Prashanth,
>
>
> On 05/25/2018 12:55 AM, Prakash, Prashanth wrote:
>>
>> Hi George,
>>
>> On 5/22/2018 5:42 AM, George Cherian wrote:
>>>
>>> Per Section 8.4.7.1.3 of ACPI 6.2, The platform provides performance
>>> feedback via set of performance counters. To determine the actual
>>> performance level delivered over time, OSPM may read a set of
>>> performance counters from the Reference Performance Counter Register
>>> and the Delivered Performance Counter Register.
>>>
>>> OSPM calculates the delivered performance over a given time period by
>>> taking a beginning and ending snapshot of both the reference and
>>> delivered performance counters, and calculating:
>>>
>>> delivered_perf = reference_perf X (delta of delivered_perf counter /
>>> delta of reference_perf counter).
>>>
>>> Implement the above and hook this to the cpufreq->get method.
>>>
>>> Signed-off-by: George Cherian <george.cherian@cavium.com>
>>> ---
>>> drivers/cpufreq/cppc_cpufreq.c | 44
>>> ++++++++++++++++++++++++++++++++++++++++++
>>> 1 file changed, 44 insertions(+)
>>>
>>> diff --git a/drivers/cpufreq/cppc_cpufreq.c
>>> b/drivers/cpufreq/cppc_cpufreq.c
>>> index b15115a..a046915 100644
>>> --- a/drivers/cpufreq/cppc_cpufreq.c
>>> +++ b/drivers/cpufreq/cppc_cpufreq.c
>>> @@ -240,10 +240,54 @@ static int cppc_cpufreq_cpu_init(struct
>>> cpufreq_policy *policy)
>>> return ret;
>>> }
>>> +static int cppc_get_rate_from_fbctrs(struct cppc_perf_fb_ctrs
>>> fb_ctrs_t0,
>>> + struct cppc_perf_fb_ctrs fb_ctrs_t1)
>>> +{
>>> + u64 delta_reference, delta_delivered;
>>> + u64 reference_perf, ratio;
>>> +
>>> + reference_perf = fb_ctrs_t0.reference_perf;
>>> + if (fb_ctrs_t1.reference > fb_ctrs_t0.reference)
>>> + delta_reference = fb_ctrs_t1.reference -
>>> fb_ctrs_t0.reference;
>>> + else /* Counters would have wrapped-around */
>>> + delta_reference = ((u64)(~((u64)0)) -
>>> fb_ctrs_t0.reference) +
>>> + fb_ctrs_t1.reference;
>>> +
>>> + if (fb_ctrs_t1.delivered > fb_ctrs_t0.delivered)
>>> + delta_delivered = fb_ctrs_t1.delivered -
>>> fb_ctrs_t0.delivered;
>>> + else /* Counters would have wrapped-around */
>>> + delta_delivered = ((u64)(~((u64)0)) -
>>> fb_ctrs_t0.delivered) +
>>> + fb_ctrs_t1.delivered;
>>
>> We need to check that the wraparound time is long enough to make sure that
>> the counters cannot wrap around more than once. We can register a get()
>> api
>> only after checking that wraparound time value is reasonably high.
>>
>> I am not aware of any platforms where wraparound time is soo short, but
>> wouldn't hurt to check once during init.
>
> By design the wraparound time is a 64 bit counter, for that matter even
> all the feedback counters too are 64 bit counters. I don't see any
> chance in which the counters can wraparound twice in back to back reads.
> The only situation is in which system itself is running at a really high
> frequency. Even in that case today's spec is not sufficient to support the
> same.
>
>>> +
>>> + if (delta_reference) /* Check to avoid divide-by zero */
>>> + ratio = (delta_delivered * 1000) / delta_reference;
>>
>> Why not just return the computed value here instead of *1000 and later
>> /1000?
>> return (ref_per * delta_del) / delta_ref;
>
> Yes.
>>>
>>> + else
>>> + return -EINVAL;
>>
>> Instead of EINVAL, i think we should return current frequency.
>>
> Sorry, I didn't get you, How do you calculate the current frequency?
> Did you mean reference performance?
>
>> The counters can pause if CPUs are in idle state during our sampling
>> interval, so
>> If the counters did not progress, it is reasonable to assume the delivered
>> perf was
>> equal to desired perf.
>
> No, that is wrong. Here the check is for reference performance delta.
> This counter can never pause. In case of cpuidle only the delivered counters
> could pause. Delivered counters will pause only if the particular core
> enters power down mode, Otherwise we would be still clocking the core and we
> should be getting a delta across 2 sampling periods. In case if the
> reference counter is paused which by design is not correct then there is no
> point in returning reference performance numbers. That too is wrong. In case
> the low level FW is not updating the
> counters properly then it should be evident till Linux, instead of returning
> a bogus frequency.
>>
>>
>> Even if platform wanted to limit, since the CPUs were asleep(idle) we
>> could not have
>> observed lower performance, so we will not throw off any logic that could
>> be driven
>> using the returned value.
>>>
>>> +
>>> + return (reference_perf * ratio) / 1000;
>>
>> This should be converted to KHz as cpufreq is not aware of CPPC abstract
>> scale
>
> In our platform all performance registers are implemented in KHz. Because of
> which we never had an issue with conversion. I am not
> aware whether ACPI mandates to use any particular unit. How is that
> implemented in your platform? Just to avoid any extra conversion don't
> you feel it is better to always report in KHz from firmware.
>
>>> +}
>>> +
>>> +static unsigned int cppc_cpufreq_get_rate(unsigned int cpunum)
>>> +{
>>> + struct cppc_perf_fb_ctrs fb_ctrs_t0 = {0}, fb_ctrs_t1 = {0};
>>> + int ret;
>>> +
>>> + ret = cppc_get_perf_ctrs(cpunum, &fb_ctrs_t0);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + ret = cppc_get_perf_ctrs(cpunum, &fb_ctrs_t1);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + return cppc_get_rate_from_fbctrs(fb_ctrs_t0, fb_ctrs_t1);
>>> +}
>>
>> We need to make sure that we get a reasonably sample so make sure the
>> reported
>> performance is accurate.
>> The counters can capture short transient throttling/limiting, so by
>> sampling a really
>> short duration of time we could return quite inaccurate measure of
>> performance.
>>
> I would say it as a momentary thing only when the frequency is being ramped
> up/down.
>
>> We need to include some reasonable delay between the two calls. What is
>> reasonable
>> is debatable - so this can be few(2-10) microseconds defined as default.
>> If the same value
>> doesn't work for all the platforms, we might need to add a platform
>> specific value.
>>
> cppc_get_perf_ctrs itself is a slow call, we have to format the CPC packet
> and ring a doorbell and then the response to be read from the shared
> registers. My initial implementation had a delay but in testing,
> I found that it was unnecessary to have such a delay. Can you please
> let me know whether it works without delay in your platform?
>
> Or else let me know whether udelay(10) is sufficient in between the
> calls.
>
>>> +
>>> static struct cpufreq_driver cppc_cpufreq_driver = {
>>> .flags = CPUFREQ_CONST_LOOPS,
>>> .verify = cppc_verify_policy,
>>> .target = cppc_cpufreq_set_target,
>>> + .get = cppc_cpufreq_get_rate,
>>> .init = cppc_cpufreq_cpu_init,
>>> .stop_cpu = cppc_cpufreq_stop_cpu,
>>> .name = "cppc_cpufreq",
>>
I was about to apply the $subject patch, but now I would like you and
Prashanth to agree on it, so please ask Prashanth for an ACK on the
final version.
^ permalink raw reply
* Re: [PATCH 2/2] ACPI: EC: Dispatch the EC GPE directly on s2idle wake
From: Rafael J. Wysocki @ 2018-05-25 8:48 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Ulf Hansson, Rafael J. Wysocki, kbuild-all, Linux ACPI, Zhang Rui,
Linux PM, LKML, Erik Schmauss, Bob Moore, Wang, Wendy,
kbuild test robot
In-Reply-To: <CAJZ5v0jFmVd_SkvgoXmi2Psqg5ALJRDsKzV8PiFysHQt9f0uog@mail.gmail.com>
On Fri, May 25, 2018 at 10:08 AM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Fri, May 25, 2018 at 8:49 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> Rafael,
>>
>> On 18 May 2018 at 08:10, kbuild test robot <lkp@intel.com> wrote:
>>> Hi Rafael,
>>>
>>> I love your patch! Yet something to improve:
>>>
>>> [auto build test ERROR on pm/linux-next]
>>> [also build test ERROR on v4.17-rc5]
>>> [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/Rafael-J-Wysocki/ACPI-PM-Dispatch-EC-GPE-early-on-s2idle-resume-if-LPS0-_DSM-is-used/20180518-070817
>>> base: https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
>>> config: arm64-defconfig (attached as .config)
>>> compiler: aarch64-linux-gnu-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=arm64
>>>
>>> All errors (new ones prefixed by >>):
>>>
>>> drivers/acpi/ec.o: In function `acpi_ec_dispatch_gpe':
>>>>> ec.c:(.text+0x239c): undefined reference to `acpi_dispatch_gpe'
>>> ec.c:(.text+0x239c): relocation truncated to fit: R_AARCH64_CALL26 against undefined symbol `acpi_dispatch_gpe'
>>>
>>> ---
>>> 0-DAY kernel test infrastructure Open Source Technology Center
>>> https://lists.01.org/pipermail/kbuild-all Intel Corporation
>>
>> This breaks arm64 builds, would you mind to drop the offending patch
>> from your linux-next branch?
>
> Not really, but I can fix it.
Should be fixed now, sorry for the breakage.
^ permalink raw reply
* Re: [PATCH v3 11/27] x86/power/64: Adapt assembly for PIE support
From: Pavel Machek @ 2018-05-25 9:10 UTC (permalink / raw)
To: Thomas Garnier
Cc: Kate Stewart, Nicolas Pitre, the arch/x86 maintainers,
Sergey Senozhatsky, Petr Mladek, Len Brown, Peter Zijlstra,
Yonghong Song, Christopher Li, Dave Hansen, Dominik Brodowski,
LKML, Masahiro Yamada, Jan Beulich, H . Peter Anvin,
Kernel Hardening, Christoph Lameter, Alok Kataria,
Linux Doc Mailing List, linux-arch, Jonathan Corbet, Herbert Xu
In-Reply-To: <CAJcbSZEnRhFDvYCG4ORH71LrcH2bAzOU5yF-oa9KkLpKo5UuSQ@mail.gmail.com>
[-- Attachment #1.1: Type: text/plain, Size: 962 bytes --]
On Thu 2018-05-24 09:37:20, Thomas Garnier wrote:
> On Thu, May 24, 2018 at 4:04 AM Pavel Machek <pavel@ucw.cz> wrote:
>
> > On Wed 2018-05-23 12:54:05, Thomas Garnier wrote:
> > > Change the assembly code to use only relative references of symbols for
> the
> > > kernel to be PIE compatible.
> > >
> > > Position Independent Executable (PIE) support will allow to extended the
> > > KASLR randomization range below the -2G memory limit.
> > >
> > > Signed-off-by: Thomas Garnier <thgarnie@google.com>
>
> > Again, was this tested?
>
> Hibernation was tested as much as I can with qemu and my dedicated
>machine.
Ok, good.
Acked-by: Pavel Machek <pavel@ucw.cz>
> Any specific test you think I should use?
Hibernation working should be good enough test for this.
Thanks,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
[-- Attachment #2: Type: text/plain, Size: 183 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH v3 09/27] x86/acpi: Adapt assembly for PIE support
From: Pavel Machek @ 2018-05-25 9:14 UTC (permalink / raw)
To: Thomas Garnier
Cc: Herbert Xu, David S . Miller, Thomas Gleixner, Ingo Molnar,
H . Peter Anvin, Peter Zijlstra, Josh Poimboeuf, Greg KH,
Philippe Ombredanne, Kate Stewart, Arnaldo Carvalho de Melo,
Yonghong Song, Andrey Ryabinin, Kees Cook, Tom Lendacky,
Kirill A. Shutemov, Andy Lutomirski, Dominik Brodowski,
Borislav Petkov, Borislav Petkov, Rafael J. Wysocki, Len Brown,
Juergen Gross <jgro>
In-Reply-To: <CAJcbSZFJ84+VC5xDQZGHctupdqwmMBgqzLzFRqCTBpi5t-2Gvw@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1606 bytes --]
On Thu 2018-05-24 09:35:42, Thomas Garnier wrote:
> On Thu, May 24, 2018 at 4:03 AM Pavel Machek <pavel@ucw.cz> wrote:
>
> > On Wed 2018-05-23 12:54:03, Thomas Garnier wrote:
> > > Change the assembly code to use only relative references of symbols for
> the
> > > kernel to be PIE compatible.
> > >
> > > Position Independent Executable (PIE) support will allow to extended the
> > > KASLR randomization range below the -2G memory limit.
>
> > What testing did this get?
>
> Tested boot, hibernation and performance on qemu and dedicated machine.
Well, this is suspend, not hibernation code.
So "sudo pm-suspend" or "echo mem > /sys/power/state" would be good
way to test this.
Thanks,
Pavel
> > > diff --git a/arch/x86/kernel/acpi/wakeup_64.S
> b/arch/x86/kernel/acpi/wakeup_64.S
> > > index 50b8ed0317a3..472659c0f811 100644
> > > --- a/arch/x86/kernel/acpi/wakeup_64.S
> > > +++ b/arch/x86/kernel/acpi/wakeup_64.S
> > > @@ -14,7 +14,7 @@
> > > * Hooray, we are in Long 64-bit mode (but still running in low
> memory)
> > > */
> > > ENTRY(wakeup_long64)
> > > - movq saved_magic, %rax
> > > + movq saved_magic(%rip), %rax
> > > movq $0x123456789abcdef0, %rdx
> > > cmpq %rdx, %rax
> > > jne bogus_64_magic
>
> > Because, as comment says, this is rather tricky code.
>
> I agree, I think maintainers feedback is very important for this patchset.
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply
* Re: [PATCH 2/2] ACPI: EC: Dispatch the EC GPE directly on s2idle wake
From: Ulf Hansson @ 2018-05-25 9:24 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Rafael J. Wysocki, kbuild-all, Linux ACPI, Zhang Rui, Linux PM,
LKML, Erik Schmauss, Bob Moore, Wang, Wendy, kbuild test robot
In-Reply-To: <CAJZ5v0j_mLPw2VT6GbAxSDizu4Rr94muweiBs6ZhLuUJe2O7vQ@mail.gmail.com>
On 25 May 2018 at 10:48, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Fri, May 25, 2018 at 10:08 AM, Rafael J. Wysocki <rafael@kernel.org> wrote:
>> On Fri, May 25, 2018 at 8:49 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>> Rafael,
>>>
>>> On 18 May 2018 at 08:10, kbuild test robot <lkp@intel.com> wrote:
>>>> Hi Rafael,
>>>>
>>>> I love your patch! Yet something to improve:
>>>>
>>>> [auto build test ERROR on pm/linux-next]
>>>> [also build test ERROR on v4.17-rc5]
>>>> [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/Rafael-J-Wysocki/ACPI-PM-Dispatch-EC-GPE-early-on-s2idle-resume-if-LPS0-_DSM-is-used/20180518-070817
>>>> base: https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
>>>> config: arm64-defconfig (attached as .config)
>>>> compiler: aarch64-linux-gnu-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=arm64
>>>>
>>>> All errors (new ones prefixed by >>):
>>>>
>>>> drivers/acpi/ec.o: In function `acpi_ec_dispatch_gpe':
>>>>>> ec.c:(.text+0x239c): undefined reference to `acpi_dispatch_gpe'
>>>> ec.c:(.text+0x239c): relocation truncated to fit: R_AARCH64_CALL26 against undefined symbol `acpi_dispatch_gpe'
>>>>
>>>> ---
>>>> 0-DAY kernel test infrastructure Open Source Technology Center
>>>> https://lists.01.org/pipermail/kbuild-all Intel Corporation
>>>
>>> This breaks arm64 builds, would you mind to drop the offending patch
>>> from your linux-next branch?
>>
>> Not really, but I can fix it.
>
> Should be fixed now, sorry for the breakage.
Thanks, yes it builds fine now!
Kind regards
Uffe
^ permalink raw reply
* PM / suspend / s2idle: avoiding splats on RT
From: Sebastian Andrzej Siewior @ 2018-05-25 9:46 UTC (permalink / raw)
To: linux-pm
Cc: Rafael J. Wysocki, Len Brown, Pavel Machek, Thomas Gleixner,
bigeasy, linux-kernel
Patch #1 is a repost with the s2idle bits included. I tested via
echo s2idle > mem_sleep && echo mem > state
and I woke up the machine via the power on button. Patches #2+ were
additionally required with s2idle.
Sebastian
^ permalink raw reply
* [PATCH 1/4] PM / suspend: Prevent might sleep splats
From: Sebastian Andrzej Siewior @ 2018-05-25 9:46 UTC (permalink / raw)
To: linux-pm
Cc: Rafael J. Wysocki, Len Brown, Pavel Machek, Thomas Gleixner,
bigeasy, linux-kernel
In-Reply-To: <20180525094648.15464-1-bigeasy@linutronix.de>
From: Thomas Gleixner <tglx@linutronix.de>
timekeeping suspend/resume calls read_persistent_clock() which takes
rtc_lock. That results in might sleep warnings because at that point
we run with interrupts disabled.
We cannot convert rtc_lock to a raw spinlock as that would trigger
other might sleep warnings.
As a workaround we disable the might sleep warnings by setting
system_state to SYSTEM_SUSPEND before calling sysdev_suspend() and
restoring it to SYSTEM_RUNNING afer sysdev_resume().
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
[bigeasy: cover s2idle]
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
include/linux/kernel.h | 1 +
kernel/power/hibernate.c | 7 +++++++
kernel/power/suspend.c | 6 ++++++
3 files changed, 14 insertions(+)
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 6a1eb0b0aad9..7aed92624531 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -542,6 +542,7 @@ extern enum system_states {
SYSTEM_HALT,
SYSTEM_POWER_OFF,
SYSTEM_RESTART,
+ SYSTEM_SUSPEND,
} system_state;
/* This cannot be an enum because some may be used in assembly source. */
diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
index 5454cc639a8d..9c85c7822383 100644
--- a/kernel/power/hibernate.c
+++ b/kernel/power/hibernate.c
@@ -287,6 +287,8 @@ static int create_image(int platform_mode)
local_irq_disable();
+ system_state = SYSTEM_SUSPEND;
+
error = syscore_suspend();
if (error) {
pr_err("Some system devices failed to power down, aborting hibernation\n");
@@ -317,6 +319,7 @@ static int create_image(int platform_mode)
syscore_resume();
Enable_irqs:
+ system_state = SYSTEM_RUNNING;
local_irq_enable();
Enable_cpus:
@@ -445,6 +448,7 @@ static int resume_target_kernel(bool platform_mode)
goto Enable_cpus;
local_irq_disable();
+ system_state = SYSTEM_SUSPEND;
error = syscore_suspend();
if (error)
@@ -478,6 +482,7 @@ static int resume_target_kernel(bool platform_mode)
syscore_resume();
Enable_irqs:
+ system_state = SYSTEM_RUNNING;
local_irq_enable();
Enable_cpus:
@@ -563,6 +568,7 @@ int hibernation_platform_enter(void)
goto Enable_cpus;
local_irq_disable();
+ system_state = SYSTEM_SUSPEND;
syscore_suspend();
if (pm_wakeup_pending()) {
error = -EAGAIN;
@@ -575,6 +581,7 @@ int hibernation_platform_enter(void)
Power_up:
syscore_resume();
+ system_state = SYSTEM_RUNNING;
local_irq_enable();
Enable_cpus:
diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
index 4c10be0f4843..9108a99878bf 100644
--- a/kernel/power/suspend.c
+++ b/kernel/power/suspend.c
@@ -110,6 +110,7 @@ static void s2idle_loop(void)
{
pm_pr_dbg("suspend-to-idle\n");
+ system_state = SYSTEM_SUSPEND;
for (;;) {
int error;
@@ -148,6 +149,7 @@ static void s2idle_loop(void)
pm_wakeup_clear(false);
}
+ system_state = SYSTEM_RUNNING;
pm_pr_dbg("resume from suspend-to-idle\n");
}
@@ -428,6 +430,8 @@ static int suspend_enter(suspend_state_t state, bool *wakeup)
arch_suspend_disable_irqs();
BUG_ON(!irqs_disabled());
+ system_state = SYSTEM_SUSPEND;
+
error = syscore_suspend();
if (!error) {
*wakeup = pm_wakeup_pending();
@@ -443,6 +447,8 @@ static int suspend_enter(suspend_state_t state, bool *wakeup)
syscore_resume();
}
+ system_state = SYSTEM_RUNNING;
+
arch_suspend_enable_irqs();
BUG_ON(irqs_disabled());
--
2.17.0
^ permalink raw reply related
* [PATCH 2/4] PM / wakeup: Make events_lock a RAW_SPINLOCK
From: Sebastian Andrzej Siewior @ 2018-05-25 9:46 UTC (permalink / raw)
To: linux-pm
Cc: Rafael J. Wysocki, Len Brown, Pavel Machek, Thomas Gleixner,
bigeasy, linux-kernel
In-Reply-To: <20180525094648.15464-1-bigeasy@linutronix.de>
The `events_lock' is acquired during suspend while interrupts are
disabled even on RT. The lock is taken only for a very brief moment.
Make it a RAW lock which avoids "sleeping while atomic" warnings on RT.
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
drivers/base/power/wakeup.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
index ea01621ed769..2e76fbcba76e 100644
--- a/drivers/base/power/wakeup.c
+++ b/drivers/base/power/wakeup.c
@@ -57,7 +57,7 @@ static void split_counters(unsigned int *cnt, unsigned int *inpr)
/* A preserved old value of the events counter. */
static unsigned int saved_count;
-static DEFINE_SPINLOCK(events_lock);
+static DEFINE_RAW_SPINLOCK(events_lock);
static void pm_wakeup_timer_fn(struct timer_list *t);
@@ -185,9 +185,9 @@ void wakeup_source_add(struct wakeup_source *ws)
ws->active = false;
ws->last_time = ktime_get();
- spin_lock_irqsave(&events_lock, flags);
+ raw_spin_lock_irqsave(&events_lock, flags);
list_add_rcu(&ws->entry, &wakeup_sources);
- spin_unlock_irqrestore(&events_lock, flags);
+ raw_spin_unlock_irqrestore(&events_lock, flags);
}
EXPORT_SYMBOL_GPL(wakeup_source_add);
@@ -202,9 +202,9 @@ void wakeup_source_remove(struct wakeup_source *ws)
if (WARN_ON(!ws))
return;
- spin_lock_irqsave(&events_lock, flags);
+ raw_spin_lock_irqsave(&events_lock, flags);
list_del_rcu(&ws->entry);
- spin_unlock_irqrestore(&events_lock, flags);
+ raw_spin_unlock_irqrestore(&events_lock, flags);
synchronize_srcu(&wakeup_srcu);
}
EXPORT_SYMBOL_GPL(wakeup_source_remove);
@@ -843,7 +843,7 @@ bool pm_wakeup_pending(void)
unsigned long flags;
bool ret = false;
- spin_lock_irqsave(&events_lock, flags);
+ raw_spin_lock_irqsave(&events_lock, flags);
if (events_check_enabled) {
unsigned int cnt, inpr;
@@ -851,7 +851,7 @@ bool pm_wakeup_pending(void)
ret = (cnt != saved_count || inpr > 0);
events_check_enabled = !ret;
}
- spin_unlock_irqrestore(&events_lock, flags);
+ raw_spin_unlock_irqrestore(&events_lock, flags);
if (ret) {
pr_info("PM: Wakeup pending, aborting suspend\n");
@@ -940,13 +940,13 @@ bool pm_save_wakeup_count(unsigned int count)
unsigned long flags;
events_check_enabled = false;
- spin_lock_irqsave(&events_lock, flags);
+ raw_spin_lock_irqsave(&events_lock, flags);
split_counters(&cnt, &inpr);
if (cnt == count && inpr == 0) {
saved_count = count;
events_check_enabled = true;
}
- spin_unlock_irqrestore(&events_lock, flags);
+ raw_spin_unlock_irqrestore(&events_lock, flags);
return events_check_enabled;
}
--
2.17.0
^ permalink raw reply related
* [PATCH 3/4] PM / s2idle: Make s2idle_wait_head swait based
From: Sebastian Andrzej Siewior @ 2018-05-25 9:46 UTC (permalink / raw)
To: linux-pm
Cc: Rafael J. Wysocki, Len Brown, Pavel Machek, Thomas Gleixner,
bigeasy, linux-kernel
In-Reply-To: <20180525094648.15464-1-bigeasy@linutronix.de>
s2idle_wait_head is used during s2idle with interrupts disabled even on
RT. There is no "custom" wake up function so swait could be used instead
which is also lower weight compared to the wait_queue.
Make s2idle_wait_head a swait_queue_head.
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
kernel/power/suspend.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
index 9108a99878bf..f865aa934835 100644
--- a/kernel/power/suspend.c
+++ b/kernel/power/suspend.c
@@ -27,6 +27,7 @@
#include <linux/export.h>
#include <linux/suspend.h>
#include <linux/syscore_ops.h>
+#include <linux/swait.h>
#include <linux/ftrace.h>
#include <trace/events/power.h>
#include <linux/compiler.h>
@@ -57,7 +58,7 @@ EXPORT_SYMBOL_GPL(pm_suspend_global_flags);
static const struct platform_suspend_ops *suspend_ops;
static const struct platform_s2idle_ops *s2idle_ops;
-static DECLARE_WAIT_QUEUE_HEAD(s2idle_wait_head);
+static DECLARE_SWAIT_QUEUE_HEAD(s2idle_wait_head);
enum s2idle_states __read_mostly s2idle_state;
static DEFINE_SPINLOCK(s2idle_lock);
@@ -91,8 +92,8 @@ static void s2idle_enter(void)
/* Push all the CPUs into the idle loop. */
wake_up_all_idle_cpus();
/* Make the current CPU wait so it can enter the idle loop too. */
- wait_event(s2idle_wait_head,
- s2idle_state == S2IDLE_STATE_WAKE);
+ swait_event(s2idle_wait_head,
+ s2idle_state == S2IDLE_STATE_WAKE);
cpuidle_pause();
put_online_cpus();
@@ -161,7 +162,7 @@ void s2idle_wake(void)
spin_lock_irqsave(&s2idle_lock, flags);
if (s2idle_state > S2IDLE_STATE_NONE) {
s2idle_state = S2IDLE_STATE_WAKE;
- wake_up(&s2idle_wait_head);
+ swake_up(&s2idle_wait_head);
}
spin_unlock_irqrestore(&s2idle_lock, flags);
}
--
2.17.0
^ permalink raw reply related
* [PATCH 4/4] PM / wakeup: Make s2idle_lock a RAW_SPINLOCK
From: Sebastian Andrzej Siewior @ 2018-05-25 9:46 UTC (permalink / raw)
To: linux-pm
Cc: Rafael J. Wysocki, Len Brown, Pavel Machek, Thomas Gleixner,
bigeasy, linux-kernel
In-Reply-To: <20180525094648.15464-1-bigeasy@linutronix.de>
The `s2idle_lock' is acquired during suspend while interrupts are
disabled even on RT. The lock is acquired for short sections only.
Make it a RAW lock which avoids "sleeping while atomic" warnings on RT.
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
kernel/power/suspend.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
index f865aa934835..ca2abb5e5109 100644
--- a/kernel/power/suspend.c
+++ b/kernel/power/suspend.c
@@ -61,7 +61,7 @@ static const struct platform_s2idle_ops *s2idle_ops;
static DECLARE_SWAIT_QUEUE_HEAD(s2idle_wait_head);
enum s2idle_states __read_mostly s2idle_state;
-static DEFINE_SPINLOCK(s2idle_lock);
+static DEFINE_RAW_SPINLOCK(s2idle_lock);
void s2idle_set_ops(const struct platform_s2idle_ops *ops)
{
@@ -79,12 +79,12 @@ static void s2idle_enter(void)
{
trace_suspend_resume(TPS("machine_suspend"), PM_SUSPEND_TO_IDLE, true);
- spin_lock_irq(&s2idle_lock);
+ raw_spin_lock_irq(&s2idle_lock);
if (pm_wakeup_pending())
goto out;
s2idle_state = S2IDLE_STATE_ENTER;
- spin_unlock_irq(&s2idle_lock);
+ raw_spin_unlock_irq(&s2idle_lock);
get_online_cpus();
cpuidle_resume();
@@ -98,11 +98,11 @@ static void s2idle_enter(void)
cpuidle_pause();
put_online_cpus();
- spin_lock_irq(&s2idle_lock);
+ raw_spin_lock_irq(&s2idle_lock);
out:
s2idle_state = S2IDLE_STATE_NONE;
- spin_unlock_irq(&s2idle_lock);
+ raw_spin_unlock_irq(&s2idle_lock);
trace_suspend_resume(TPS("machine_suspend"), PM_SUSPEND_TO_IDLE, false);
}
@@ -159,12 +159,12 @@ void s2idle_wake(void)
{
unsigned long flags;
- spin_lock_irqsave(&s2idle_lock, flags);
+ raw_spin_lock_irqsave(&s2idle_lock, flags);
if (s2idle_state > S2IDLE_STATE_NONE) {
s2idle_state = S2IDLE_STATE_WAKE;
swake_up(&s2idle_wait_head);
}
- spin_unlock_irqrestore(&s2idle_lock, flags);
+ raw_spin_unlock_irqrestore(&s2idle_lock, flags);
}
EXPORT_SYMBOL_GPL(s2idle_wake);
--
2.17.0
^ permalink raw reply related
* [PATCH V4] powercap/drivers/idle_injection: Add an idle injection framework
From: Daniel Lezcano @ 2018-05-25 9:49 UTC (permalink / raw)
To: rjw
Cc: viresh.kumar, edubezval, kevin.wangtao, leo.yan, vincent.guittot,
linux-kernel, javi.merino, rui.zhang, linux-pm, daniel.thompson
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.
The framework relies on the smpboot kthreads which handles via its
main loop 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>
---
V4:
- Wait for completion when stopping (Viresh Kumar)
- Check init already done and rollback (Viresh Kumar)
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 | 368 ++++++++++++++++++++++++++++++++++++++
include/linux/idle_injection.h | 29 +++
4 files changed, 408 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..ecf290a
--- /dev/null
+++ b/drivers/powercap/idle_injection.c
@@ -0,0 +1,368 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * 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.
+ */
+#define pr_fmt(fmt) "ii_dev: " fmt
+
+#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;
+ struct completion stop_complete;
+ 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);
+
+ if (WARN_ON_ONCE(!ii_dev))
+ return;
+
+ iit = per_cpu_ptr(&idle_injection_thread, cpu);
+
+ /*
+ * Boolean used by the smpboot main loop and used as a
+ * flip-flop in this function
+ */
+ iit->should_run = 0;
+
+ atomic_inc(&ii_dev->count);
+
+ idle_duration_ms = atomic_read(&ii_dev->idle_duration_ms);
+ if (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);
+ if (run_duration_ms) {
+ hrtimer_start(&ii_dev->timer, ms_to_ktime(run_duration_ms),
+ HRTIMER_MODE_REL_PINNED);
+ return;
+ }
+
+ complete(&ii_dev->stop_complete);
+}
+
+/**
+ * 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);
+}
+
+/**
+ * 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 resetting the idle and
+ * running durations and wait for the threads to complete. If we are
+ * in the process of injecting an idle cycle, then this will wait the
+ * end of the cycle.
+ */
+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));
+
+ init_completion(&ii_dev->stop_complete);
+
+ idle_injection_set_duration(ii_dev, 0, 0);
+
+ wait_for_completion_interruptible(&ii_dev->stop_complete);
+}
+
+/**
+ * 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;
+}
+
+static struct idle_injection_device *ii_dev_alloc(void)
+{
+ struct idle_injection_device *ii_dev;
+
+ ii_dev = kzalloc(sizeof(*ii_dev), GFP_KERNEL);
+ if (!ii_dev)
+ return NULL;
+
+ if (!alloc_cpumask_var(&ii_dev->cpumask, GFP_KERNEL)) {
+ kfree(ii_dev);
+ return NULL;
+ }
+
+ return ii_dev;
+}
+
+static void ii_dev_free(struct idle_injection_device *ii_dev)
+{
+ free_cpumask_var(ii_dev->cpumask);
+ kfree(ii_dev);
+}
+
+/**
+ * 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 of 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, cpu2;
+
+ ii_dev = ii_dev_alloc();
+ if (!ii_dev)
+ return NULL;
+
+ 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, ii_dev->cpumask) {
+
+ if (per_cpu(idle_injection_device, cpu)) {
+ pr_err("cpu%d is already registered\n", cpu);
+ goto out_rollback_per_cpu;
+ }
+
+ per_cpu(idle_injection_device, cpu) = ii_dev;
+ }
+
+ return ii_dev;
+
+out_rollback_per_cpu:
+ for_each_cpu(cpu2, ii_dev->cpumask) {
+ if (cpu == cpu2)
+ break;
+
+ per_cpu(idle_injection_device, cpu2) = NULL;
+ }
+
+ ii_dev_free(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;
+
+ ii_dev_free(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);
diff --git a/include/linux/idle_injection.h b/include/linux/idle_injection.h
new file mode 100644
index 0000000..ffd20d7
--- /dev/null
+++ b/include/linux/idle_injection.h
@@ -0,0 +1,29 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2018 Linaro Ltd
+ *
+ * Author: Daniel Lezcano <daniel.lezcano@linaro.org>
+ *
+ */
+#ifndef __IDLE_INJECTION_H__
+#define __IDLE_INJECTION_H__
+
+/* private idle injection device structure */
+struct idle_injection_device;
+
+struct idle_injection_device *idle_injection_register(struct cpumask *cpumask);
+
+void idle_injection_unregister(struct idle_injection_device *ii_dev);
+
+int idle_injection_start(struct idle_injection_device *ii_dev);
+
+void idle_injection_stop(struct idle_injection_device *ii_dev);
+
+void idle_injection_set_duration(struct idle_injection_device *ii_dev,
+ unsigned int run_duration_ms,
+ unsigned int idle_duration_ms);
+
+void idle_injection_get_duration(struct idle_injection_device *ii_dev,
+ unsigned int *run_duration_ms,
+ unsigned int *idle_duration_ms);
+#endif /* __IDLE_INJECTION_H__ */
--
2.7.4
^ permalink raw reply related
* Re: [PATCH 1/4] PM / suspend: Prevent might sleep splats
From: Rafael J. Wysocki @ 2018-05-25 10:03 UTC (permalink / raw)
To: Sebastian Andrzej Siewior
Cc: Linux PM, Rafael J. Wysocki, Len Brown, Pavel Machek,
Thomas Gleixner, Linux Kernel Mailing List
In-Reply-To: <20180525094648.15464-2-bigeasy@linutronix.de>
On Fri, May 25, 2018 at 11:46 AM, Sebastian Andrzej Siewior
<bigeasy@linutronix.de> wrote:
> From: Thomas Gleixner <tglx@linutronix.de>
>
> timekeeping suspend/resume calls read_persistent_clock() which takes
> rtc_lock. That results in might sleep warnings because at that point
> we run with interrupts disabled.
>
> We cannot convert rtc_lock to a raw spinlock as that would trigger
> other might sleep warnings.
>
> As a workaround we disable the might sleep warnings by setting
> system_state to SYSTEM_SUSPEND before calling sysdev_suspend() and
> restoring it to SYSTEM_RUNNING afer sysdev_resume().
>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> [bigeasy: cover s2idle]
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
> include/linux/kernel.h | 1 +
> kernel/power/hibernate.c | 7 +++++++
> kernel/power/suspend.c | 6 ++++++
> 3 files changed, 14 insertions(+)
>
> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> index 6a1eb0b0aad9..7aed92624531 100644
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -542,6 +542,7 @@ extern enum system_states {
> SYSTEM_HALT,
> SYSTEM_POWER_OFF,
> SYSTEM_RESTART,
> + SYSTEM_SUSPEND,
> } system_state;
>
> /* This cannot be an enum because some may be used in assembly source. */
> diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
> index 5454cc639a8d..9c85c7822383 100644
> --- a/kernel/power/hibernate.c
> +++ b/kernel/power/hibernate.c
> @@ -287,6 +287,8 @@ static int create_image(int platform_mode)
>
> local_irq_disable();
>
> + system_state = SYSTEM_SUSPEND;
> +
> error = syscore_suspend();
> if (error) {
> pr_err("Some system devices failed to power down, aborting hibernation\n");
> @@ -317,6 +319,7 @@ static int create_image(int platform_mode)
> syscore_resume();
>
> Enable_irqs:
> + system_state = SYSTEM_RUNNING;
> local_irq_enable();
>
> Enable_cpus:
> @@ -445,6 +448,7 @@ static int resume_target_kernel(bool platform_mode)
> goto Enable_cpus;
>
> local_irq_disable();
> + system_state = SYSTEM_SUSPEND;
>
> error = syscore_suspend();
> if (error)
> @@ -478,6 +482,7 @@ static int resume_target_kernel(bool platform_mode)
> syscore_resume();
>
> Enable_irqs:
> + system_state = SYSTEM_RUNNING;
> local_irq_enable();
>
> Enable_cpus:
> @@ -563,6 +568,7 @@ int hibernation_platform_enter(void)
> goto Enable_cpus;
>
> local_irq_disable();
> + system_state = SYSTEM_SUSPEND;
> syscore_suspend();
> if (pm_wakeup_pending()) {
> error = -EAGAIN;
> @@ -575,6 +581,7 @@ int hibernation_platform_enter(void)
>
> Power_up:
> syscore_resume();
> + system_state = SYSTEM_RUNNING;
> local_irq_enable();
>
> Enable_cpus:
> diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
> index 4c10be0f4843..9108a99878bf 100644
> --- a/kernel/power/suspend.c
> +++ b/kernel/power/suspend.c
> @@ -110,6 +110,7 @@ static void s2idle_loop(void)
> {
> pm_pr_dbg("suspend-to-idle\n");
>
> + system_state = SYSTEM_SUSPEND;
And here's the ambiguity.
For the other sleep variants SYSTEM_SUSPEND means "all devices have
been suspended, interrupts are off and there's one CPU active".
Here, device suspend has not even completed entirely.
I guess you could set system_state to SYSTEM_SUSPEND right before
calling timekeeping_suspend() in tick_freeze() and then back to
SYSTEM_RUNNING after timekeeping_resume() has returned in
tick_unfreeze(), but talk to Thomas about that first. :-)
^ 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