* Re: [PATCH v2 07/15] arm64: dts: imx8: switch to two cell scu clock binding
From: Fabio Estevam @ 2019-07-16 19:28 UTC (permalink / raw)
To: Dong Aisheng
Cc: open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Dong Aisheng, Catalin Marinas, Will Deacon, Rob Herring,
NXP Linux Team, Sascha Hauer, Fabio Estevam, Shawn Guo,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE
In-Reply-To: <1563290089-11085-8-git-send-email-aisheng.dong@nxp.com>
On Tue, Jul 16, 2019 at 12:37 PM Dong Aisheng <aisheng.dong@nxp.com> wrote:
>
> switch to two cell scu clock binding
Please explain why you are doing this.
^ permalink raw reply
* Re: [PATCH v2 06/15] arm64: dts: imx8: add adma lpcg clocks
From: Fabio Estevam @ 2019-07-16 19:28 UTC (permalink / raw)
To: Dong Aisheng
Cc: Mark Rutland,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Dong Aisheng, Catalin Marinas, Will Deacon, Rob Herring,
NXP Linux Team, Sascha Hauer, Fabio Estevam, Shawn Guo,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE
In-Reply-To: <1563290089-11085-7-git-send-email-aisheng.dong@nxp.com>
On Tue, Jul 16, 2019 at 12:37 PM Dong Aisheng <aisheng.dong@nxp.com> wrote:
> + uart0_lpcg: clock-controller@5a460000 {
> + reg = <0x5a460000 0x10000>;
> + #clock-cells = <1>;
> + clocks = <&clk IMX_ADMA_UART0_CLK>,
> + <&dma_ipg_clk>;
Putting the clocks into a single line helps readability, even if it is
over 80 col.
> + bit-offset = <0 16>;
> + clock-output-names = "uart0_lpcg_baud_clk",
> + "uart0_lpcg_ipg_clk";
Same here.
^ permalink raw reply
* Re: [PATCH v2 02/15] arm64: dts: imx8qxp: move scu pd node before scu clock node
From: Fabio Estevam @ 2019-07-16 19:26 UTC (permalink / raw)
To: Dong Aisheng
Cc: Mark Rutland,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Dong Aisheng, Catalin Marinas, Will Deacon, Rob Herring,
NXP Linux Team, Sascha Hauer, Fabio Estevam, Shawn Guo,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE
In-Reply-To: <1563290089-11085-3-git-send-email-aisheng.dong@nxp.com>
On Tue, Jul 16, 2019 at 12:37 PM Dong Aisheng <aisheng.dong@nxp.com> wrote:
>
> SCU clock depends on SCU Power domain. So let's move scu pd node
> before scu clock to make it probe earlier.
This looks like a fragile solution. Shouldn't this be handled via
probe deferring?
^ permalink raw reply
* Re: [PATCH V5 11/18] clk: tegra210: Add support for Tegra210 clocks
From: Sowjanya Komatineni @ 2019-07-16 19:26 UTC (permalink / raw)
To: Dmitry Osipenko, Peter De Schrijver, Joseph Lo
Cc: thierry.reding, jonathanh, tglx, jason, marc.zyngier,
linus.walleij, stefan, mark.rutland, pgaikwad, sboyd, linux-clk,
linux-gpio, jckuo, talho, linux-tegra, linux-kernel, mperttunen,
spatra, robh+dt, devicetree
In-Reply-To: <5b2945c5-fcb2-2ac0-2bf2-df869dc9c713@gmail.com>
On 7/16/19 11:43 AM, Dmitry Osipenko wrote:
> 16.07.2019 21:30, Sowjanya Komatineni пишет:
>> On 7/16/19 11:25 AM, Dmitry Osipenko wrote:
>>> 16.07.2019 21:19, Sowjanya Komatineni пишет:
>>>> On 7/16/19 9:50 AM, Sowjanya Komatineni wrote:
>>>>> On 7/16/19 8:00 AM, Dmitry Osipenko wrote:
>>>>>> 16.07.2019 11:06, Peter De Schrijver пишет:
>>>>>>> On Tue, Jul 16, 2019 at 03:24:26PM +0800, Joseph Lo wrote:
>>>>>>>>> OK, Will add to CPUFreq driver...
>>>>>>>>>> The other thing that also need attention is that T124 CPUFreq
>>>>>>>>>> driver
>>>>>>>>>> implicitly relies on DFLL driver to be probed first, which is
>>>>>>>>>> icky.
>>>>>>>>>>
>>>>>>>>> Should I add check for successful dfll clk register explicitly in
>>>>>>>>> CPUFreq driver probe and defer till dfll clk registers?
>>>>>> Probably you should use the "device links". See [1][2] for the
>>>>>> example.
>>>>>>
>>>>>> [1]
>>>>>> https://elixir.bootlin.com/linux/v5.2.1/source/drivers/gpu/drm/tegra/dc.c#L2383
>>>>>>
>>>>>>
>>>>>>
>>>>>> [2] https://www.kernel.org/doc/html/latest/driver-api/device_link.html
>>>>>>
>>>>>> Return EPROBE_DEFER instead of EINVAL if device_link_add() fails. And
>>>>>> use of_find_device_by_node() to get the DFLL's device, see [3].
>>>>>>
>>>>>> [3]
>>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/devfreq/tegra20-devfreq.c#n100
>>>>>>
>>>>>>
>>>>> Will go thru and add...
>>> Looks like I initially confused this case with getting orphaned clock.
>>> I'm now seeing that the DFLL driver registers the clock and then
>>> clk_get(dfll) should be returning EPROBE_DEFER until DFLL driver is
>>> probed, hence everything should be fine as-is and there is no real need
>>> for the 'device link'. Sorry for the confusion!
>>>
>>>>>>>> Sorry, I didn't follow the mail thread. Just regarding the DFLL
>>>>>>>> part.
>>>>>>>>
>>>>>>>> As you know it, the DFLL clock is one of the CPU clock sources and
>>>>>>>> integrated with DVFS control logic with the regulator. We will not
>>>>>>>> switch
>>>>>>>> CPU to other clock sources once we switched to DFLL. Because the
>>>>>>>> CPU has
>>>>>>>> been regulated by the DFLL HW with the DVFS table (CVB or OPP table
>>>>>>>> you see
>>>>>>>> in the driver.). We shouldn't reparent it to other sources with
>>>>>>>> unknew
>>>>>>>> freq/volt pair. That's not guaranteed to work. We allow switching to
>>>>>>>> open-loop mode but different sources.
>>>>>> Okay, then the CPUFreq driver will have to enforce DFLL freq to PLLP's
>>>>>> rate before switching to PLLP in order to have a proper CPU voltage.
>>>>> PLLP freq is safe to work for any CPU voltage. So no need to enforce
>>>>> DFLL freq to PLLP rate before changing CCLK_G source to PLLP during
>>>>> suspend
>>>>>
>>>> Sorry, please ignore my above comment. During suspend, need to change
>>>> CCLK_G source to PLLP when dfll is in closed loop mode first and then
>>>> dfll need to be set to open loop.
>>> Okay.
>>>
>>>>>>>> And I don't exactly understand why we need to switch to PLLP in CPU
>>>>>>>> idle
>>>>>>>> driver. Just keep it on CL-DVFS mode all the time.
>>>>>>>>
>>>>>>>> In SC7 entry, the dfll suspend function moves it the open-loop
>>>>>>>> mode. That's
>>>>>>>> all. The sc7-entryfirmware will handle the rest of the sequence to
>>>>>>>> turn off
>>>>>>>> the CPU power.
>>>>>>>>
>>>>>>>> In SC7 resume, the warmboot code will handle the sequence to turn on
>>>>>>>> regulator and power up the CPU cluster. And leave it on PLL_P. After
>>>>>>>> resuming to the kernel, we re-init DFLL, restore the CPU clock
>>>>>>>> policy (CPU
>>>>>>>> runs on DFLL open-loop mode) and then moving to close-loop mode.
>>>>>> The DFLL is re-inited after switching CCLK to DFLL parent during of
>>>>>> the
>>>>>> early clocks-state restoring by CaR driver. Hence instead of having
>>>>>> odd
>>>>>> hacks in the CaR driver, it is much nicer to have a proper
>>>>>> suspend-resume sequencing of the device drivers. In this case CPUFreq
>>>>>> driver is the driver that enables DFLL and switches CPU to that clock
>>>>>> source, which means that this driver is also should be responsible for
>>>>>> management of the DFLL's state during of suspend/resume process. If
>>>>>> CPUFreq driver disables DFLL during suspend and re-enables it during
>>>>>> resume, then looks like the CaR driver hacks around DFLL are not
>>>>>> needed.
>>>>>>
>>>>>>>> The DFLL part looks good to me. BTW, change the patch subject to
>>>>>>>> "Add
>>>>>>>> suspend-resume support" seems more appropriate to me.
>>>>>>>>
>>>>>>> To clarify this, the sequences for DFLL use are as follows (assuming
>>>>>>> all
>>>>>>> required DFLL hw configuration has been done)
>>>>>>>
>>>>>>> Switch to DFLL:
>>>>>>> 0) Save current parent and frequency
>>>>>>> 1) Program DFLL to open loop mode
>>>>>>> 2) Enable DFLL
>>>>>>> 3) Change cclk_g parent to DFLL
>>>>>>> For OVR regulator:
>>>>>>> 4) Change PWM output pin from tristate to output
>>>>>>> 5) Enable DFLL PWM output
>>>>>>> For I2C regulator:
>>>>>>> 4) Enable DFLL I2C output
>>>>>>> 6) Program DFLL to closed loop mode
>>>>>>>
>>>>>>> Switch away from DFLL:
>>>>>>> 0) Change cclk_g parent to PLLP so the CPU frequency is ok for any
>>>>>>> vdd_cpu voltage
>>>>>>> 1) Program DFLL to open loop mode
>>>>>>>
>>>> I see during switch away from DFLL (suspend), cclk_g parent is not
>>>> changed to PLLP before changing dfll to open loop mode.
>>>>
>>>> Will add this ...
>>> The CPUFreq driver switches parent to PLLP during the probe, similar
>>> should be done on suspend.
>>>
>>> I'm also wondering if it's always safe to switch to PLLP in the probe.
>>> If CPU is running on a lower freq than PLLP, then some other more
>>> appropriate intermediate parent should be selected.
>>>
>> CPU parents are PLL_X, PLL_P, and dfll. PLL_X always runs at higher rate
>> so switching to PLL_P during CPUFreq probe prior to dfll clock enable
>> should be safe.
> AFAIK, PLLX could run at ~200MHz. There is also a divided output of PLLP
> which CCLKG supports, the PLLP_OUT4.
>
> Probably, realistically, CPU is always running off a fast PLLX during
> boot, but I'm wondering what may happen on KEXEC. I guess ideally
> CPUFreq driver should also have a 'shutdown' callback to teardown DFLL
> on a reboot, but likely that there are other clock-related problems as
> well that may break KEXEC and thus it is not very important at the moment.
>
> [snip]
During bootup CPUG sources from PLL_X. By PLL_P source above I meant
PLL_P_OUT4.
As per clock policies, PLL_X is always used for high freq like >800Mhz
and for low frequency it will be sourced from PLLP.
^ permalink raw reply
* Re: [PATCH v2 15/15] arm64: defconfig: add imx8qm mek support
From: Fabio Estevam @ 2019-07-16 19:24 UTC (permalink / raw)
To: Dong Aisheng
Cc: open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Dong Aisheng, Catalin Marinas, Will Deacon, Rob Herring,
NXP Linux Team, Sascha Hauer, Fabio Estevam, Shawn Guo,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE
In-Reply-To: <1563290089-11085-16-git-send-email-aisheng.dong@nxp.com>
Hi Dong,
On Tue, Jul 16, 2019 at 12:38 PM Dong Aisheng <aisheng.dong@nxp.com> wrote:
>
> add imx8qm mek support
The Subject and commit log does not match with the patch content.
^ permalink raw reply
* Re: [PATCH RFC v2] PM / devfreq: Add cpu based scaling support to passive_governor
From: Saravana Kannan @ 2019-07-16 19:23 UTC (permalink / raw)
To: Sibi Sankar
Cc: MyungJoo Ham, Chanwoo Choi, Kyungmin Park, Viresh Kumar,
Nishanth Menon, Stephen Boyd, Georgi Djakov, agross, david.brown,
Rob Herring, Mark Rutland, Rafael J. Wysocki, linux-arm-msm,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, LKML,
Linux PM, hsinyi, adharmap, Saravana Kannan
In-Reply-To: <20190716133700.30024-1-sibis@codeaurora.org>
On Tue, Jul 16, 2019 at 6:37 AM Sibi Sankar <sibis@codeaurora.org> wrote:
>
> From: Saravana Kannan <skannan@codeaurora.org>
>
> Many CPU architectures have caches that can scale independent of the
> CPUs. Frequency scaling of the caches is necessary to make sure that the
> cache is not a performance bottleneck that leads to poor performance and
> power. The same idea applies for RAM/DDR.
>
> To achieve this, this patch adds support for cpu based scaling to the
> passive governor. This is accomplished by taking the current frequency
> of each CPU frequency domain and then adjust the frequency of the cache
> (or any devfreq device) based on the frequency of the CPUs. It listens
> to CPU frequency transition notifiers to keep itself up to date on the
> current CPU frequency.
>
> To decide the frequency of the device, the governor does one of the
> following:
> * Derives the optimal devfreq device opp from required-opps property of
> the parent cpu opp_table.
>
> * Scales the device frequency in proportion to the CPU frequency. So, if
> the CPUs are running at their max frequency, the device runs at its
> max frequency. If the CPUs are running at their min frequency, the
> device runs at its min frequency. It is interpolated for frequencies
> in between.
>
> Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
> [Sibi: Integrated cpu-freqmap governor into passive_governor]
> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
> ---
> v2:
> * Address Chanwoo's review comments
>
> This patch is a re-work of:
> https://patchwork.kernel.org/patch/10553171/
>
> This patch depends on Saravana's add required-opps support series:
> https://patchwork.kernel.org/cover/11016423/
>
> drivers/devfreq/Kconfig | 2 +
> drivers/devfreq/governor_passive.c | 275 +++++++++++++++++++++++++++--
> include/linux/devfreq.h | 40 ++++-
> 3 files changed, 296 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
> index f3b242987fd91..5db8633f94d83 100644
> --- a/drivers/devfreq/Kconfig
> +++ b/drivers/devfreq/Kconfig
> @@ -73,6 +73,8 @@ config DEVFREQ_GOV_PASSIVE
> device. This governor does not change the frequency by itself
> through sysfs entries. The passive governor recommends that
> devfreq device uses the OPP table to get the frequency/voltage.
> + Alternatively the governor can also be chosen to scale based on
> + the online CPUs current frequency.
>
> comment "DEVFREQ Drivers"
>
> diff --git a/drivers/devfreq/governor_passive.c b/drivers/devfreq/governor_passive.c
> index 24ce94c80f06e..743b6093cc732 100644
> --- a/drivers/devfreq/governor_passive.c
> +++ b/drivers/devfreq/governor_passive.c
> @@ -8,11 +8,86 @@
> */
>
> #include <linux/module.h>
> +#include <linux/cpu.h>
> +#include <linux/cpufreq.h>
> +#include <linux/cpumask.h>
> #include <linux/device.h>
> #include <linux/devfreq.h>
> +#include <linux/slab.h>
> #include "governor.h"
>
> -static int devfreq_passive_get_target_freq(struct devfreq *devfreq,
> +static unsigned int xlate_cpufreq_to_devfreq(struct devfreq_passive_data *data,
> + unsigned int cpu)
> +{
> + unsigned int cpu_min, cpu_max, dev_min, dev_max, cpu_percent, max_state;
> + struct devfreq_cpu_state *cpu_state = data->cpu_state[cpu];
> + struct devfreq *devfreq = (struct devfreq *)data->this;
> + unsigned long *freq_table = devfreq->profile->freq_table;
> + struct dev_pm_opp *opp = NULL, *cpu_opp = NULL;
> + unsigned long cpu_freq, freq;
> +
> + if (!cpu_state || cpu_state->first_cpu != cpu ||
> + !cpu_state->opp_table || !devfreq->opp_table)
> + return 0;
> +
> + cpu_freq = cpu_state->freq * 1000;
> + cpu_opp = devfreq_recommended_opp(cpu_state->dev, &cpu_freq, 0);
> + if (IS_ERR(cpu_opp))
> + return 0;
> +
> + opp = dev_pm_opp_xlate_opp(cpu_state->opp_table,
> + devfreq->opp_table, cpu_opp);
> + dev_pm_opp_put(cpu_opp);
> +
> + if (!IS_ERR(opp)) {
> + freq = dev_pm_opp_get_freq(opp);
> + dev_pm_opp_put(opp);
> + } else {
> + /* Use Interpolation if required opps is not available */
> + cpu_min = cpu_state->min_freq;
> + cpu_max = cpu_state->max_freq;
> + cpu_freq = cpu_state->freq;
> +
> + if (freq_table) {
> + /* Get minimum frequency according to sorting order */
> + max_state = freq_table[devfreq->profile->max_state - 1];
> + if (freq_table[0] < max_state) {
> + dev_min = freq_table[0];
> + dev_max = max_state;
> + } else {
> + dev_min = max_state;
> + dev_max = freq_table[0];
> + }
> + } else {
> + if (devfreq->max_freq <= devfreq->min_freq)
> + return 0;
> + dev_min = devfreq->min_freq;
> + dev_max = devfreq->max_freq;
> + }
> + cpu_percent = ((cpu_freq - cpu_min) * 100) / cpu_max - cpu_min;
> + freq = dev_min + mult_frac(dev_max - dev_min, cpu_percent, 100);
> + }
> +
> + return freq;
> +}
> +
> +static int get_target_freq_with_cpufreq(struct devfreq *devfreq,
> + unsigned long *freq)
> +{
> + struct devfreq_passive_data *p_data =
> + (struct devfreq_passive_data *)devfreq->data;
> + unsigned int cpu, target_freq = 0;
> +
> + for_each_online_cpu(cpu)
> + target_freq = max(target_freq,
> + xlate_cpufreq_to_devfreq(p_data, cpu));
> +
> + *freq = target_freq;
> +
> + return 0;
> +}
> +
> +static int get_target_freq_with_devfreq(struct devfreq *devfreq,
> unsigned long *freq)
> {
> struct devfreq_passive_data *p_data
> @@ -22,16 +97,6 @@ static int devfreq_passive_get_target_freq(struct devfreq *devfreq,
> struct dev_pm_opp *opp = NULL, *p_opp = NULL;
> int i, count, ret = 0;
>
> - /*
> - * If the devfreq device with passive governor has the specific method
> - * to determine the next frequency, should use the get_target_freq()
> - * of struct devfreq_passive_data.
> - */
> - if (p_data->get_target_freq) {
> - ret = p_data->get_target_freq(devfreq, freq);
> - goto out;
> - }
> -
> /*
> * If the parent and passive devfreq device uses the OPP table,
> * get the next frequency by using the OPP table.
> @@ -102,6 +167,37 @@ static int devfreq_passive_get_target_freq(struct devfreq *devfreq,
> return ret;
> }
>
> +static int devfreq_passive_get_target_freq(struct devfreq *devfreq,
> + unsigned long *freq)
> +{
> + struct devfreq_passive_data *p_data =
> + (struct devfreq_passive_data *)devfreq->data;
> + int ret;
> +
> + /*
> + * If the devfreq device with passive governor has the specific method
> + * to determine the next frequency, should use the get_target_freq()
> + * of struct devfreq_passive_data.
> + */
> + if (p_data->get_target_freq)
> + return p_data->get_target_freq(devfreq, freq);
> +
> + switch (p_data->parent_type) {
> + case DEVFREQ_PARENT_DEV:
> + ret = get_target_freq_with_devfreq(devfreq, freq);
> + break;
> + case CPUFREQ_PARENT_DEV:
> + ret = get_target_freq_with_cpufreq(devfreq, freq);
> + break;
> + default:
> + ret = -EINVAL;
> + dev_err(&devfreq->dev, "Invalid parent type\n");
> + break;
> + }
> +
> + return ret;
> +}
> +
> static int update_devfreq_passive(struct devfreq *devfreq, unsigned long freq)
> {
> int ret;
> @@ -156,6 +252,140 @@ static int devfreq_passive_notifier_call(struct notifier_block *nb,
> return NOTIFY_DONE;
> }
>
> +static int cpufreq_passive_notifier_call(struct notifier_block *nb,
> + unsigned long event, void *ptr)
> +{
> + struct devfreq_passive_data *data =
> + container_of(nb, struct devfreq_passive_data, nb);
> + struct devfreq *devfreq = (struct devfreq *)data->this;
> + struct devfreq_cpu_state *cpu_state;
> + struct cpufreq_freqs *freq = ptr;
> + unsigned int current_freq;
> + int ret;
> +
> + if (event != CPUFREQ_POSTCHANGE || !freq ||
> + !data->cpu_state[freq->policy->cpu])
> + return 0;
> +
> + cpu_state = data->cpu_state[freq->policy->cpu];
> + if (cpu_state->freq == freq->new)
> + return 0;
> +
> + /* Backup current freq and pre-update cpu state freq*/
> + current_freq = cpu_state->freq;
> + cpu_state->freq = freq->new;
> +
> + mutex_lock(&devfreq->lock);
> + ret = update_devfreq(devfreq);
> + mutex_unlock(&devfreq->lock);
> + if (ret) {
> + cpu_state->freq = current_freq;
> + dev_err(&devfreq->dev, "Couldn't update the frequency.\n");
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static int cpufreq_passive_register(struct devfreq_passive_data **p_data)
> +{
> + struct devfreq_passive_data *data = *p_data;
> + struct devfreq *devfreq = (struct devfreq *)data->this;
> + struct device *dev = devfreq->dev.parent;
> + struct opp_table *opp_table = NULL;
> + struct devfreq_cpu_state *state;
> + struct cpufreq_policy *policy;
> + struct device *cpu_dev;
> + unsigned int cpu;
> + int ret;
> +
> + get_online_cpus();
> + data->nb.notifier_call = cpufreq_passive_notifier_call;
> + ret = cpufreq_register_notifier(&data->nb,
> + CPUFREQ_TRANSITION_NOTIFIER);
> + if (ret) {
> + dev_err(dev, "Couldn't register cpufreq notifier.\n");
> + data->nb.notifier_call = NULL;
> + goto out;
> + }
> +
> + /* Populate devfreq_cpu_state */
> + for_each_online_cpu(cpu) {
> + if (data->cpu_state[cpu])
> + continue;
> +
> + policy = cpufreq_cpu_get(cpu);
> + if (policy) {
> + state = kzalloc(sizeof(*state), GFP_KERNEL);
> + if (!state) {
> + ret = -ENOMEM;
> + goto out;
> + }
> +
> + cpu_dev = get_cpu_device(cpu);
> + if (!cpu_dev) {
> + dev_err(dev, "Couldn't get cpu device.\n");
> + ret = -ENODEV;
> + goto out;
> + }
> +
> + opp_table = dev_pm_opp_get_opp_table(cpu_dev);
> + if (IS_ERR(devfreq->opp_table)) {
> + ret = PTR_ERR(opp_table);
> + goto out;
> + }
> +
> + state->dev = cpu_dev;
> + state->opp_table = opp_table;
> + state->first_cpu = cpumask_first(policy->related_cpus);
> + state->freq = policy->cur;
> + state->min_freq = policy->cpuinfo.min_freq;
> + state->max_freq = policy->cpuinfo.max_freq;
> + data->cpu_state[cpu] = state;
> + cpufreq_cpu_put(policy);
> + } else {
> + ret = -EPROBE_DEFER;
> + goto out;
> + }
> + }
> +out:
> + put_online_cpus();
> + if (ret)
> + return ret;
> +
> + /* Update devfreq */
> + mutex_lock(&devfreq->lock);
> + ret = update_devfreq(devfreq);
> + mutex_unlock(&devfreq->lock);
> + if (ret)
> + dev_err(dev, "Couldn't update the frequency.\n");
> +
> + return ret;
> +}
> +
> +static int cpufreq_passive_unregister(struct devfreq_passive_data **p_data)
> +{
> + struct devfreq_passive_data *data = *p_data;
> + struct devfreq_cpu_state *cpu_state;
> + int cpu;
> +
> + if (data->nb.notifier_call)
> + cpufreq_unregister_notifier(&data->nb,
> + CPUFREQ_TRANSITION_NOTIFIER);
> +
> + for_each_possible_cpu(cpu) {
> + cpu_state = data->cpu_state[cpu];
> + if (cpu_state) {
> + if (cpu_state->opp_table)
> + dev_pm_opp_put_opp_table(cpu_state->opp_table);
> + kfree(cpu_state);
> + cpu_state = NULL;
> + }
> + }
> +
> + return 0;
> +}
> +
> static int devfreq_passive_event_handler(struct devfreq *devfreq,
> unsigned int event, void *data)
> {
> @@ -166,7 +396,7 @@ static int devfreq_passive_event_handler(struct devfreq *devfreq,
> struct notifier_block *nb = &p_data->nb;
> int ret = 0;
>
> - if (!parent)
> + if (p_data->parent_type == DEVFREQ_PARENT_DEV && !parent)
> return -EPROBE_DEFER;
>
> switch (event) {
> @@ -174,13 +404,24 @@ static int devfreq_passive_event_handler(struct devfreq *devfreq,
> if (!p_data->this)
> p_data->this = devfreq;
>
> - nb->notifier_call = devfreq_passive_notifier_call;
> - ret = devm_devfreq_register_notifier(dev, parent, nb,
> - DEVFREQ_TRANSITION_NOTIFIER);
> + if (p_data->parent_type == DEVFREQ_PARENT_DEV) {
> + nb->notifier_call = devfreq_passive_notifier_call;
> + ret = devm_devfreq_register_notifier(dev, parent, nb,
> + DEVFREQ_TRANSITION_NOTIFIER);
> + } else if (p_data->parent_type == CPUFREQ_PARENT_DEV) {
> + ret = cpufreq_passive_register(&p_data);
> + } else {
> + ret = -EINVAL;
> + }
> break;
> case DEVFREQ_GOV_STOP:
> - devm_devfreq_unregister_notifier(dev, parent, nb,
> - DEVFREQ_TRANSITION_NOTIFIER);
> + if (p_data->parent_type == DEVFREQ_PARENT_DEV)
> + devm_devfreq_unregister_notifier(dev, parent, nb,
> + DEVFREQ_TRANSITION_NOTIFIER);
> + else if (p_data->parent_type == CPUFREQ_PARENT_DEV)
> + cpufreq_passive_unregister(&p_data);
> + else
> + ret = -EINVAL;
> break;
> default:
> break;
> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
> index 69d487329408e..f3f898dfd09b2 100644
> --- a/include/linux/devfreq.h
> +++ b/include/linux/devfreq.h
> @@ -263,6 +263,32 @@ struct devfreq_simple_ondemand_data {
> #endif
>
> #if IS_ENABLED(CONFIG_DEVFREQ_GOV_PASSIVE)
> +/**
> + * struct devfreq_cpu_state - holds the per-cpu state
> + * @freq: the current frequency of the cpu.
> + * @min_freq: the min frequency of the cpu.
> + * @max_freq: the max frequency of the cpu.
> + * @first_cpu: the cpumask of the first cpu of a policy.
> + * @dev: reference to cpu device.
> + * @opp_table: reference to cpu opp table.
> + *
> + * This structure stores the required cpu_state of a cpu.
> + * This is auto-populated by the governor.
> + */
> +struct devfreq_cpu_state {
> + unsigned int freq;
> + unsigned int min_freq;
> + unsigned int max_freq;
> + unsigned int first_cpu;
> + struct device *dev;
> + struct opp_table *opp_table;
> +};
> +
> +enum devfreq_parent_dev_type {
> + DEVFREQ_PARENT_DEV,
> + CPUFREQ_PARENT_DEV,
> +};
> +
> /**
> * struct devfreq_passive_data - void *data fed to struct devfreq
> * and devfreq_add_device
> @@ -274,13 +300,15 @@ struct devfreq_simple_ondemand_data {
> * using governors except for passive governor.
> * If the devfreq device has the specific method to decide
> * the next frequency, should use this callback.
> - * @this: the devfreq instance of own device.
> - * @nb: the notifier block for DEVFREQ_TRANSITION_NOTIFIER list
> + * @parent_type parent type of the device
> + * @this: the devfreq instance of own device.
> + * @nb: the notifier block for DEVFREQ_TRANSITION_NOTIFIER list
> + * @cpu_state: the state min/max/current frequency of all online cpu's
> *
> * The devfreq_passive_data have to set the devfreq instance of parent
> * device with governors except for the passive governor. But, don't need to
> - * initialize the 'this' and 'nb' field because the devfreq core will handle
> - * them.
> + * initialize the 'this', 'nb' and 'cpu_state' field because the devfreq core
> + * will handle them.
> */
> struct devfreq_passive_data {
> /* Should set the devfreq instance of parent device */
> @@ -289,9 +317,13 @@ struct devfreq_passive_data {
> /* Optional callback to decide the next frequency of passvice device */
> int (*get_target_freq)(struct devfreq *this, unsigned long *freq);
>
> + /* Should set the type of parent device */
> + enum devfreq_parent_dev_type parent_type;
> +
> /* For passive governor's internal use. Don't need to set them */
> struct devfreq *this;
> struct notifier_block nb;
> + struct devfreq_cpu_state *cpu_state[NR_CPUS];
> };
> #endif
Just as an FYI, I wrote this code a long time ago. I was planning on
rewriting the integration of CPU freq inputs to devfreq anyway because
notifiers are discouraged and I've had other ideas on how I could do
it. But I haven't dug into that yet.
Honestly I'd prefer waiting till the required-opps support for passive
governor patch is merged before digging too much into this. And I plan
to take a stab at this after that.
Thanks,
Saravana
^ permalink raw reply
* Re: [PATCH v2 11/11] interconnect: Add devfreq support
From: Saravana Kannan @ 2019-07-16 19:17 UTC (permalink / raw)
To: Sibi Sankar
Cc: Georgi Djakov, Rob Herring, Mark Rutland, Viresh Kumar,
Nishanth Menon, Stephen Boyd, Rafael J. Wysocki, MyungJoo Ham,
Kyungmin Park, Chanwoo Choi, Rajendra Nayak, Jordan Crouse,
Vincent Guittot, Bjorn Andersson, amit.kucheria, Sweeney, Sean,
daidavid1, Evan Green, Android Kernel Team, Linux PM
In-Reply-To: <9f2bf3fd-f7c5-40e8-6415-f334e3ef8d5d@codeaurora.org>
On Tue, Jul 16, 2019 at 11:13 AM Sibi Sankar <sibis@codeaurora.org> wrote:
>
> Hey Saravana,
>
> On 6/18/19 2:48 AM, Saravana Kannan wrote:
> > On Mon, Jun 17, 2019 at 8:44 AM Georgi Djakov <georgi.djakov@linaro.org> wrote:
> >>
> >> Hi Saravana,
> >>
> >> On 6/14/19 07:17, Saravana Kannan wrote:
> >>> Add a icc_create_devfreq() and icc_remove_devfreq() to create and remove
> >>> devfreq devices for interconnect paths. A driver can create/remove devfreq
> >>> devices for the interconnects needed for its device by calling these APIs.
> >>> This would allow various devfreq governors to work with interconnect paths
> >>> and the device driver itself doesn't have to actively manage the bandwidth
> >>> votes for the interconnects.
> >>
> >> Thanks for the patches, but creating devfreq devices for each interconnect path
> >> seems odd to me - at least for consumers that already use a governor.
> >
> > Each governor instance always handles one "frequency" (more like
> > performance) domain at a time. So if a consumer is already using a
> > governor to scale the hardware block, then using another governor to
> > scale the interconnect performance points is the right way to go about
> > it. In fact, that's exactly what devfreq passive governor's
> > documentation even says it's meant for. That's also what cpufreq does
> > for each cluster/CPU frequency domain too.
> >
> >> So for DDR
> >> scaling for example, are you suggesting that we add a devfreq device from the
> >> cpufreq driver in order to scale the interconnect between CPU<->DDR?
> >
> > Yes in general. Although, CPUs are a special case because CPUs don't
> > go through devfreq. So passive governor as it stands today won't work.
> > CPU<->DDR scaling might need a separate governor (unlikely) or some
> > changes to the passive governor that I'm happy to work on once we
> > settle this for general devices like GPU, etc. But the DT format for
> > CPUs will be identical to GPUs or any other device.
>
> using icc_create_devfreq from the cpufreq-hw driver on SDM845 SoC
> to scale CPU<->DDR would cause a circular dependency. (i.e) with
> the addition of cpufreq notifier to the passive governor as in
> https://patchwork.kernel.org/patch/11046147/ devm_devfreq_add_device
> would require the cpufreq transistion notifier register and cpu
> freq_cpu_get to go through. Please add your thought on addressing this.
This is an old series. So not going to dive into this much.
But to answer your question, I wrote the cpufreq_map governor a long
time ago. So not surprised if you are finding issues with it -- it
needs a rewrite anyway.
-Saravana
> >
> >> Also if the
> >> GPU is already using devfreq, should we add a devfreq per each interconnect
> >> path? What would be the benefit in this case - using different governors for
> >> bandwidth scaling maybe?
> >
> > When saying "separate/different governors" in this email, I mean both
> > different instance of the same governor logic with different tunables
> > AND actually different algorithms/governor logic entirely.
> >
> > The heuristics to use for each interconnect path might be (more like,
> > will be) different based on hardware characteristics (Eg: what voltage
> > domains the interconnect is sitting on) and what interconnect
> > information is available (Eg: Just busy time vs bandwidth count vs no
> > information etc) -- so having separate governors for each interconnect
> > path makes a lot of sense. It also allows userspace to control the
> > policy for scaling each of those paths based on product use cases.
> >
> > For example, when the GPU is just doing simple UI rendering, userspace
> > can use the max_freq sysfs file for the devfreq device to disallow high
> > bandwidth OPPs on the GPU<->DDR path, but those higher OPPs could be
> > allowed by userspace when the GPU is used for games. Having devfreq
> > device for each interconnect path also make it easy to debug
> > performance issues -- you can independently change the votes for each
> > path to figure out what is causing the bottleneck, etc.
> >
> > Adding a devfreq device for interconnect voting with a few lines gives
> > all these features "for free".
> >
> > This doesn't mean all users of interconnect framework NEED to use
> > devfreq for interconnect. They might do it simply based on
> > calculations based on the use case (Eg: display driver from display
> > resolution). But if they are trying to use any kind of
> > algorithm/heuristics, writing it as a devfreq governor should be
> > encouraged.
> >
> > Also want to point out that BW OPPs also work for drivers that don't
> > use devfreq at all. The interconnect-opp-table just lists the
> > meaningful OPP leveld for the path and the device driver can pick one
> > entry from the table based on the use case.
> >
> > Thanks,
> > Saravana
> >
> >
> >
>
> --
> Qualcomm Innovation Center, Inc.
> Qualcomm Innovation Center, Inc, is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project
>
> --
> To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@android.com.
>
^ permalink raw reply
* Re: [PATCH v3 2/6] OPP: Add support for bandwidth OPP tables
From: Saravana Kannan @ 2019-07-16 19:10 UTC (permalink / raw)
To: Sibi Sankar
Cc: Georgi Djakov, Rob Herring, Mark Rutland, Viresh Kumar,
Nishanth Menon, Stephen Boyd, Rafael J. Wysocki, Vincent Guittot,
Sweeney, Sean, daidavid1, Rajendra Nayak, Bjorn Andersson,
Evan Green, Android Kernel Team, Linux PM,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, LKML,
adharmap
In-Reply-To: <5dd35be3-fd03-c9cc-1eed-ce4bc1433363@codeaurora.org>
On Tue, Jul 16, 2019 at 10:33 AM Sibi Sankar <sibis@codeaurora.org> wrote:
>
> Hey Saravana,
>
> On 7/3/19 6:40 AM, Saravana Kannan wrote:
> > Not all devices quantify their performance points in terms of frequency.
> > Devices like interconnects quantify their performance points in terms of
> > bandwidth. We need a way to represent these bandwidth levels in OPP. So,
> > add support for parsing bandwidth OPPs from DT.
> >
> > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > ---
> > drivers/opp/of.c | 34 ++++++++++++++++++++++++++++++++--
> > drivers/opp/opp.h | 4 +++-
> > 2 files changed, 35 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/opp/of.c b/drivers/opp/of.c
> > index c10c782d15aa..54fa70ed2adc 100644
> > --- a/drivers/opp/of.c
> > +++ b/drivers/opp/of.c
> > @@ -552,6 +552,35 @@ void dev_pm_opp_of_remove_table(struct device *dev)
> > }
> > EXPORT_SYMBOL_GPL(dev_pm_opp_of_remove_table);
> >
> > +static int _read_opp_key(struct dev_pm_opp *new_opp, struct device_node *np)
> > +{
> > + int ret;
> > + u64 rate;
> > + u32 bw;
> > +
> > + ret = of_property_read_u64(np, "opp-hz", &rate);
> > + if (!ret) {
> > + /*
> > + * Rate is defined as an unsigned long in clk API, and so
> > + * casting explicitly to its type. Must be fixed once rate is 64
> > + * bit guaranteed in clk API.
> > + */
> > + new_opp->rate = (unsigned long)rate
> now that the rate gets set here, please remove the rate assignment in
> _opp_add_static_v2
>
> > + return 0;
> > + }
> > +
> > + ret = of_property_read_u32(np, "opp-peak-KBps", &bw);
> > + if (ret)
> > + return ret;
> > + new_opp->rate = (unsigned long) &bw;
>
> should be bw instead
Good catch. Thanks!
>
> > +
> > + ret = of_property_read_u32(np, "opp-avg-KBps", &bw);
> > + if (!ret)
> > + new_opp->avg_bw = (unsigned long) &bw;
>
> ditto
>
> > +
> > + return 0;
> > +}
> > +
> > /**
> > * _opp_add_static_v2() - Allocate static OPPs (As per 'v2' DT bindings)
> > * @opp_table: OPP table
> > @@ -589,11 +618,12 @@ static struct dev_pm_opp *_opp_add_static_v2(struct opp_table *opp_table,
> > if (!new_opp)
> > return ERR_PTR(-ENOMEM);
> >
> > - ret = of_property_read_u64(np, "opp-hz", &rate);
> > + ret = _read_opp_key(new_opp, np);
> > if (ret < 0) {
> > /* "opp-hz" is optional for devices like power domains. */
> > if (!opp_table->is_genpd) {
> > - dev_err(dev, "%s: opp-hz not found\n", __func__);
> > + dev_err(dev, "%s: opp-hz or opp-peak-bw not found\n",
> > + __func__);
>
> please remove the else part where rate value will be reset.
Ah! I flipped the meaning of the "if" check in my head. Thanks!
-Saravana
^ permalink raw reply
* Re: [PATCH V13 12/12] PCI: tegra: Add Tegra194 PCIe support
From: Bjorn Helgaas @ 2019-07-16 19:00 UTC (permalink / raw)
To: Lorenzo Pieralisi
Cc: Vidya Sagar, robh+dt, mark.rutland, thierry.reding, jonathanh,
kishon, catalin.marinas, will.deacon, jingoohan1,
gustavo.pimentel, digetx, mperttunen, linux-pci, devicetree,
linux-tegra, linux-kernel, linux-arm-kernel, kthota, mmaddireddy,
sagar.tv
In-Reply-To: <20190716112225.GA24335@e121166-lin.cambridge.arm.com>
On Tue, Jul 16, 2019 at 12:22:25PM +0100, Lorenzo Pieralisi wrote:
> On Sat, Jul 13, 2019 at 12:34:34PM +0530, Vidya Sagar wrote:
> > > > > So if the link is not up we still go ahead and make probe
> > > > > succeed. What for ?
> > > > We may need root port to be available to support hot-plugging of
> > > > endpoint devices, so, we don't fail the probe.
> > >
> > > We need it or we don't. If you do support hotplugging of endpoint
> > > devices point me at the code, otherwise link up failure means
> > > failure to probe.
> > Currently hotplugging of endpoint is not supported, but it is one of
> > the use cases that we may add support for in future.
>
> You should elaborate on this, I do not understand what you mean,
> either the root port(s) supports hotplug or it does not.
>
> > But, why should we fail probe if link up doesn't happen? As such,
> > nothing went wrong in terms of root port initialization right? I
> > checked other DWC based implementations and following are not failing
> > the probe pci-dra7xx.c, pcie-armada8k.c, pcie-artpec6.c, pcie-histb.c,
> > pcie-kirin.c, pcie-spear13xx.c, pci-exynos.c, pci-imx6.c,
> > pci-keystone.c, pci-layerscape.c
> >
> > Although following do fail the probe if link is not up. pcie-qcom.c,
> > pcie-uniphier.c, pci-meson.c
> >
> > So, to me, it looks more like a choice we can make whether to fail the
> > probe or not and in this case we are choosing not to fail.
>
> I disagree. I had an offline chat with Bjorn and whether link-up should
> fail the probe or not depends on whether the root port(s) is hotplug
> capable or not and this in turn relies on the root port "Slot
> implemented" bit in the PCI Express capabilities register.
There might be a little more we can talk about in this regard. I did
bring up the "Slot implemented" bit, but after thinking about it more,
I don't really think the host bridge driver should be looking at that.
That's a PCIe concept, and it's really *downstream* from the host
bridge itself. The host bridge is logically a device on the CPU bus,
not the PCI bus.
I'm starting to think that the host bridge driver probe should be
disconnected from question of whether the root port links are up.
Logically, the host bridge driver connects the CPU bus to a PCI root
bus, so it converts CPU-side accesses to PCI config, memory, or I/O
port transactions. Given that, the PCI core can enumerate devices on
the root bus and downstream buses.
Devices on the root bus typically include Root Ports, but might also
include endpoints, Root Complex Integrated Endpoints, Root Complex
Event Collectors, etc. I think in principle, we would want the host
bridge probe to succeed so we can use these devices even if none of
the Root Ports have a link.
If a Root Port is present, I think users will expect to see it in the
"lspci" output, even if its downstream link is not up. That will
enable things like manually poking the Root Port via "setpci" for
debug. And if it has a connector, the generic pciehp should be able
to handle hot-add events without any special help from the host bridge
driver.
On ACPI systems there is no concept of the host bridge driver probe
failing because of lack of link on a Root Port. If a Root Port
doesn't have an operational link, we still keep the pci_root.c driver,
and we'll enumerate the Root Port itself. So I tend to think DT
systems should behave the same way, i.e., the driver probe should
succeed unless it fails to allocate resources or something similar. I
think this is analogous to a NIC or USB adapter driver, where the
probe succeeds even if there's no network cable or USB device
attached.
Bjorn
^ permalink raw reply
* Re: [PATCH v3 1/6] dt-bindings: opp: Introduce opp-peak-KBps and opp-avg-KBps bindings
From: Saravana Kannan @ 2019-07-16 18:58 UTC (permalink / raw)
To: Sibi Sankar
Cc: Georgi Djakov, Rob Herring, Mark Rutland, Viresh Kumar,
Nishanth Menon, Stephen Boyd, Rafael J. Wysocki, Vincent Guittot,
Sweeney, Sean, daidavid1, Rajendra Nayak, Bjorn Andersson,
Evan Green, Android Kernel Team, Linux PM,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, LKML
In-Reply-To: <98b2e315-e8da-80ad-1ef8-e6b222c1c6fe@codeaurora.org>
On Tue, Jul 16, 2019 at 10:25 AM Sibi Sankar <sibis@codeaurora.org> wrote:
>
> Hey Saravana,
>
> https://patchwork.kernel.org/patch/10850815/
> There was already a discussion ^^ on how bandwidth bindings were to be
> named.
Yes, I'm aware of that series. That series is trying to define a BW
mapping for an existing frequency OPP table. This patch is NOT about
adding a mapping to an existing table. This patch is about adding the
notion of BW OPP tables where BW is the "key" instead of "frequency".
So let's not mixed up these two series.
-Saravana
> On 7/3/19 6:40 AM, Saravana Kannan wrote:
> > Interconnects often quantify their performance points in terms of
> > bandwidth. So, add opp-peak-KBps (required) and opp-avg-KBps (optional) to
> > allow specifying Bandwidth OPP tables in DT.
> >
> > opp-peak-KBps is a required property that replace opp-hz for Bandwidth OPP
> > tables.
> >
> > opp-avg-KBps is an optional property that can be used in Bandwidth OPP
> > tables.
> >
> > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > ---
> > Documentation/devicetree/bindings/opp/opp.txt | 15 ++++++++++++---
> > 1 file changed, 12 insertions(+), 3 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/opp/opp.txt b/Documentation/devicetree/bindings/opp/opp.txt
> > index 76b6c79604a5..c869e87caa2a 100644
> > --- a/Documentation/devicetree/bindings/opp/opp.txt
> > +++ b/Documentation/devicetree/bindings/opp/opp.txt
> > @@ -83,9 +83,14 @@ properties.
> >
> > Required properties:
> > - opp-hz: Frequency in Hz, expressed as a 64-bit big-endian integer. This is a
> > - required property for all device nodes but devices like power domains. The
> > - power domain nodes must have another (implementation dependent) property which
> > - uniquely identifies the OPP nodes.
> > + required property for all device nodes but for devices like power domains or
> > + bandwidth opp tables. The power domain nodes must have another (implementation
> > + dependent) property which uniquely identifies the OPP nodes. The interconnect
> > + opps are required to have the opp-peak-bw property.
> > +
> > +- opp-peak-KBps: Peak bandwidth in kilobytes per second, expressed as a 32-bit
> > + big-endian integer. This is a required property for all devices that don't
> > + have opp-hz. For example, bandwidth OPP tables for interconnect paths.
> >
> > Optional properties:
> > - opp-microvolt: voltage in micro Volts.
> > @@ -132,6 +137,10 @@ Optional properties:
> > - opp-level: A value representing the performance level of the device,
> > expressed as a 32-bit integer.
> >
> > +- opp-avg-KBps: Average bandwidth in kilobytes per second, expressed as a
> > + 32-bit big-endian integer. This property is only meaningful in OPP tables
> > + where opp-peak-KBps is present.
> > +
> > - clock-latency-ns: Specifies the maximum possible transition latency (in
> > nanoseconds) for switching to this OPP from any other OPP.
> >
> >
>
> --
> Qualcomm Innovation Center, Inc.
> Qualcomm Innovation Center, Inc, is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project
^ permalink raw reply
* Re: [PATCH v9 03/18] kunit: test: add string_stream a std::stream like string builder
From: Brendan Higgins @ 2019-07-16 18:55 UTC (permalink / raw)
To: Stephen Boyd
Cc: Frank Rowand, Greg KH, Josh Poimboeuf, Kees Cook, Kieran Bingham,
Luis Chamberlain, Peter Zijlstra, Rob Herring, shuah,
Theodore Ts'o, Masahiro Yamada, devicetree, dri-devel,
kunit-dev, open list:DOCUMENTATION, linux-fsdevel, linux-kbuild,
Linux Kernel Mailing List, KERNEL
In-Reply-To: <20190716153400.5CB182054F@mail.kernel.org>
On Tue, Jul 16, 2019 at 8:34 AM Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Brendan Higgins (2019-07-15 15:43:20)
> > On Mon, Jul 15, 2019 at 3:11 PM Brendan Higgins
> > <brendanhiggins@google.com> wrote:
> > >
> > > On Mon, Jul 15, 2019 at 3:04 PM Stephen Boyd <sboyd@kernel.org> wrote:
> > > >
> > > > Quoting Brendan Higgins (2019-07-15 14:11:50)
> > > > > On Mon, Jul 15, 2019 at 1:43 PM Stephen Boyd <sboyd@kernel.org> wrote:
> > > > > >
> > > > > > I also wonder if it would be better to just have a big slop buffer of a
> > > > > > 4K page or something so that we almost never have to allocate anything
> > > > > > with a string_stream and we can just rely on a reader consuming data
> > > > > > while writers are writing. That might work out better, but I don't quite
> > > > > > understand the use case for the string stream.
> > > > >
> > > > > That makes sense, but might that also waste memory since we will
> > > > > almost never need that much memory?
> > > >
> > > > Why do we care? These are unit tests.
> > >
> > > Agreed.
> > >
> > > > Having allocations in here makes
> > > > things more complicated, whereas it would be simpler to have a pointer
> > > > and a spinlock operating on a chunk of memory that gets flushed out
> > > > periodically.
> > >
> > > I am not so sure. I have to have the logic to allocate memory in some
> > > case no matter what (what if I need more memory that my preallocated
> > > chuck?). I think it is simpler to always request an allocation than to
> > > only sometimes request an allocation.
> >
> > Another even simpler alternative might be to just allocate memory
> > using kunit_kmalloc as we need it and just let the kunit_resource code
> > handle cleaning it all up when the test case finishes.
>
> Sure, sounds like a nice way to avoid duplicating similar logic to
> maintain a list of things to free later.
I think I will go that route for now.
> >
> > What do you think?
>
> If you go the allocation route then you'll need to have the flags to
> know what context you're in to allocate appropriately. Does that mean
> all the string operations will now take GFP flags?
We could set the GFP flags in the constructor, store them in a field,
and then just reuse them.
Thanks!
^ permalink raw reply
* Re: [PATCH v9 04/18] kunit: test: add kunit_stream a std::stream like logger
From: Brendan Higgins @ 2019-07-16 18:52 UTC (permalink / raw)
To: Stephen Boyd
Cc: Frank Rowand, Greg KH, Josh Poimboeuf, Kees Cook, Kieran Bingham,
Luis Chamberlain, Peter Zijlstra, Rob Herring, shuah,
Theodore Ts'o, Masahiro Yamada, devicetree, dri-devel,
kunit-dev, open list:DOCUMENTATION, linux-fsdevel, linux-kbuild,
Linux Kernel Mailing List, KERNEL
In-Reply-To: <20190716175021.9CA412173C@mail.kernel.org>
On Tue, Jul 16, 2019 at 10:50 AM Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Brendan Higgins (2019-07-16 01:37:34)
> > On Tue, Jul 16, 2019 at 12:57 AM Brendan Higgins
> > <brendanhiggins@google.com> wrote:
> > >
> > > A `struct kunit_stream` is usually associated with a message that is
> > > being built up over time like maybe an expectation; it is meant to
> > > capture the idea that we might want to send some information out to
> > > the user pertaining to some thing 'X', but we aren't sure that we
> > > actually want to send it until 'X' is complete, but do to the nature
> > > of 'X' it is easier to start constructing the message before 'X' is
> > > complete.
> > >
> > > Consider a complicated expectation, there might be multiple conditions
> > > that satisfy it and multiple conditions which could make it fail. As
> > > we start exploring the input to the expectation we gain information
> > > that we might want to share back with the user if the expectation were
> > > to fail and we might get that information before we are actually sure
> > > that the expectation does indeed fail.
> > >
> > > When we first step into the expectation we immediately know the
> > > function name, file name, and line number where we are called and
> > > would want to put that information into any message we would send to
> > > the user about this expectation. Next, we might want to check a
> > > property of the input, it may or may not be enough information on its
> > > own for the expectation to fail, but we want to share the result of
> > > the property check with the user regardless, BUT only if the
> > > expectation as a whole fails.
> > >
> > > Hence, we can have multiple `struct kunit_stream`s associated with a
> > > `struct kunit` active at any given time.
>
> I'm coming back to this now after reading the rest of the patches that
> deal with assertions and expectations. It looks like the string stream
> is there to hold a few different pieces of information:
>
> - Line Number
> - File Name
> - Function Name
>
> The above items could be stored in a structure on the stack that then
> gets printed and formatted when the expectation or assertion fails. That
> would make the whole string stream structure and code unnecessary.
Most of the expectations and assertions in this patchset are fairly
simple, and what you are describing would probably work. However, I
have some expectations I plan on adding in later patchsets that make
much more complicated checks.
> The only hypothetical case where this can't be done is a complicated
> assertion or expectation that does more than one check and can't be
> written as a function that dumps out what went wrong. Is this a real
> problem? Maybe such an assertion should just open code that logic so we
> don't have to build up a string for all the other simple cases.
I have some expectations in follow up patchsets for which I created a
set of composable matchers for matching structures and function calls
that by their nature cannot be written as a single function. The
matcher thing is a bit speculative, I know, but for any kind of
function call matching, you need to store a record of functions you
are expecting to have called and then each one needs to have a set of
expectations defined by the user; I don't think there is a way to do
that that doesn't involve having multiple separate functions each
having some information useful to constructing the message.
I know the code in question isn't in this patchset; the function
matching code was in one of the earlier versions of the RFC, but I
dropped it to make this patchset smaller and more manageable. So I get
it if you would like me to drop it and add it back in when I try to
get the function and structure matching stuff in, but I would really
prefer to keep it as is if you don't care too much.
> It seems far simpler to get rid of the string stream API and just have a
> struct for this.
>
> struct kunit_fail_msg {
> const char *line;
> const char *file;
> const char *func;
> const char *msg;
> };
>
> Then you can have the assertion macros create this on the stack (with
> another macro?).
>
> #define DEFINE_KUNIT_FAIL_MSG(name, _msg) \
> struct kunit_fail_msg name = { \
> .line = __LINE__, \
> .file = __FILE__, \
> .func = __func__, \
> .msg = _msg, \
> }
>
> Note: I don't know if the __LINE__ above will use the macro location, so
> this probably needs another wrapper to put the right line number there.
No, that should work. It picks up where the macro ends up being
finally evaluated.
> I don't want to derail this whole series on this topic, but it seems
> like a bunch of code is there to construct this same set of information
> over and over again into a buffer a little bit at a time and then throw
> it away when nothing fails just because we may want to support the case
> where we have some unstructured data to inform the user about.
Yeah, that's fair. I think there are a number of improvements to be
made with how the expectations are defined other than that, but I was
hoping I could do that after this patchset is merged. I just figured
with the kinds of things I would like to do, it would lead to a whole
new round of discussion.
In either case, I think I would still like to use the `struct
kunit_stream` as part of the interface to share the failure message
with the test case runner code in test.c, at least eventually, so that
I only have to have one way to receive data from expectations, but I
think I can do that and still do what you suggest by just constructing
the kunit_stream at the end of expectations where it is feasible.
All in all I agree with what you are saying, but I would rather do it
as a follow up possibly once we have some more code on the table. I
could just see this opening up a whole new can of worms where we
debate about exactly how expectations and assertions work for another
several months, only to rip it all out shortly there after. I know
that's how these things go, but that's my preference.
I can do what you suggest if you feel strongly about it, but I would
prefer to hold off until later. It's your call.
> Why not build in the structured part into the framework (i.e. the struct
> above) so that it's always there and then add the string building part
> later when we have to?
See above comments.
^ permalink raw reply
* Re: [PATCH V5 11/18] clk: tegra210: Add support for Tegra210 clocks
From: Dmitry Osipenko @ 2019-07-16 18:43 UTC (permalink / raw)
To: Sowjanya Komatineni, Peter De Schrijver, Joseph Lo
Cc: thierry.reding, jonathanh, tglx, jason, marc.zyngier,
linus.walleij, stefan, mark.rutland, pgaikwad, sboyd, linux-clk,
linux-gpio, jckuo, talho, linux-tegra, linux-kernel, mperttunen,
spatra, robh+dt, devicetree
In-Reply-To: <0706576a-ce61-1cf3-bed1-05f54a1e2489@nvidia.com>
16.07.2019 21:30, Sowjanya Komatineni пишет:
>
> On 7/16/19 11:25 AM, Dmitry Osipenko wrote:
>> 16.07.2019 21:19, Sowjanya Komatineni пишет:
>>> On 7/16/19 9:50 AM, Sowjanya Komatineni wrote:
>>>> On 7/16/19 8:00 AM, Dmitry Osipenko wrote:
>>>>> 16.07.2019 11:06, Peter De Schrijver пишет:
>>>>>> On Tue, Jul 16, 2019 at 03:24:26PM +0800, Joseph Lo wrote:
>>>>>>>> OK, Will add to CPUFreq driver...
>>>>>>>>> The other thing that also need attention is that T124 CPUFreq
>>>>>>>>> driver
>>>>>>>>> implicitly relies on DFLL driver to be probed first, which is
>>>>>>>>> icky.
>>>>>>>>>
>>>>>>>> Should I add check for successful dfll clk register explicitly in
>>>>>>>> CPUFreq driver probe and defer till dfll clk registers?
>>>>> Probably you should use the "device links". See [1][2] for the
>>>>> example.
>>>>>
>>>>> [1]
>>>>> https://elixir.bootlin.com/linux/v5.2.1/source/drivers/gpu/drm/tegra/dc.c#L2383
>>>>>
>>>>>
>>>>>
>>>>> [2] https://www.kernel.org/doc/html/latest/driver-api/device_link.html
>>>>>
>>>>> Return EPROBE_DEFER instead of EINVAL if device_link_add() fails. And
>>>>> use of_find_device_by_node() to get the DFLL's device, see [3].
>>>>>
>>>>> [3]
>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/devfreq/tegra20-devfreq.c#n100
>>>>>
>>>>>
>>>> Will go thru and add...
>> Looks like I initially confused this case with getting orphaned clock.
>> I'm now seeing that the DFLL driver registers the clock and then
>> clk_get(dfll) should be returning EPROBE_DEFER until DFLL driver is
>> probed, hence everything should be fine as-is and there is no real need
>> for the 'device link'. Sorry for the confusion!
>>
>>>>>>> Sorry, I didn't follow the mail thread. Just regarding the DFLL
>>>>>>> part.
>>>>>>>
>>>>>>> As you know it, the DFLL clock is one of the CPU clock sources and
>>>>>>> integrated with DVFS control logic with the regulator. We will not
>>>>>>> switch
>>>>>>> CPU to other clock sources once we switched to DFLL. Because the
>>>>>>> CPU has
>>>>>>> been regulated by the DFLL HW with the DVFS table (CVB or OPP table
>>>>>>> you see
>>>>>>> in the driver.). We shouldn't reparent it to other sources with
>>>>>>> unknew
>>>>>>> freq/volt pair. That's not guaranteed to work. We allow switching to
>>>>>>> open-loop mode but different sources.
>>>>> Okay, then the CPUFreq driver will have to enforce DFLL freq to PLLP's
>>>>> rate before switching to PLLP in order to have a proper CPU voltage.
>>>> PLLP freq is safe to work for any CPU voltage. So no need to enforce
>>>> DFLL freq to PLLP rate before changing CCLK_G source to PLLP during
>>>> suspend
>>>>
>>> Sorry, please ignore my above comment. During suspend, need to change
>>> CCLK_G source to PLLP when dfll is in closed loop mode first and then
>>> dfll need to be set to open loop.
>> Okay.
>>
>>>>>>> And I don't exactly understand why we need to switch to PLLP in CPU
>>>>>>> idle
>>>>>>> driver. Just keep it on CL-DVFS mode all the time.
>>>>>>>
>>>>>>> In SC7 entry, the dfll suspend function moves it the open-loop
>>>>>>> mode. That's
>>>>>>> all. The sc7-entryfirmware will handle the rest of the sequence to
>>>>>>> turn off
>>>>>>> the CPU power.
>>>>>>>
>>>>>>> In SC7 resume, the warmboot code will handle the sequence to turn on
>>>>>>> regulator and power up the CPU cluster. And leave it on PLL_P. After
>>>>>>> resuming to the kernel, we re-init DFLL, restore the CPU clock
>>>>>>> policy (CPU
>>>>>>> runs on DFLL open-loop mode) and then moving to close-loop mode.
>>>>> The DFLL is re-inited after switching CCLK to DFLL parent during of
>>>>> the
>>>>> early clocks-state restoring by CaR driver. Hence instead of having
>>>>> odd
>>>>> hacks in the CaR driver, it is much nicer to have a proper
>>>>> suspend-resume sequencing of the device drivers. In this case CPUFreq
>>>>> driver is the driver that enables DFLL and switches CPU to that clock
>>>>> source, which means that this driver is also should be responsible for
>>>>> management of the DFLL's state during of suspend/resume process. If
>>>>> CPUFreq driver disables DFLL during suspend and re-enables it during
>>>>> resume, then looks like the CaR driver hacks around DFLL are not
>>>>> needed.
>>>>>
>>>>>>> The DFLL part looks good to me. BTW, change the patch subject to
>>>>>>> "Add
>>>>>>> suspend-resume support" seems more appropriate to me.
>>>>>>>
>>>>>> To clarify this, the sequences for DFLL use are as follows (assuming
>>>>>> all
>>>>>> required DFLL hw configuration has been done)
>>>>>>
>>>>>> Switch to DFLL:
>>>>>> 0) Save current parent and frequency
>>>>>> 1) Program DFLL to open loop mode
>>>>>> 2) Enable DFLL
>>>>>> 3) Change cclk_g parent to DFLL
>>>>>> For OVR regulator:
>>>>>> 4) Change PWM output pin from tristate to output
>>>>>> 5) Enable DFLL PWM output
>>>>>> For I2C regulator:
>>>>>> 4) Enable DFLL I2C output
>>>>>> 6) Program DFLL to closed loop mode
>>>>>>
>>>>>> Switch away from DFLL:
>>>>>> 0) Change cclk_g parent to PLLP so the CPU frequency is ok for any
>>>>>> vdd_cpu voltage
>>>>>> 1) Program DFLL to open loop mode
>>>>>>
>>> I see during switch away from DFLL (suspend), cclk_g parent is not
>>> changed to PLLP before changing dfll to open loop mode.
>>>
>>> Will add this ...
>> The CPUFreq driver switches parent to PLLP during the probe, similar
>> should be done on suspend.
>>
>> I'm also wondering if it's always safe to switch to PLLP in the probe.
>> If CPU is running on a lower freq than PLLP, then some other more
>> appropriate intermediate parent should be selected.
>>
> CPU parents are PLL_X, PLL_P, and dfll. PLL_X always runs at higher rate
> so switching to PLL_P during CPUFreq probe prior to dfll clock enable
> should be safe.
AFAIK, PLLX could run at ~200MHz. There is also a divided output of PLLP
which CCLKG supports, the PLLP_OUT4.
Probably, realistically, CPU is always running off a fast PLLX during
boot, but I'm wondering what may happen on KEXEC. I guess ideally
CPUFreq driver should also have a 'shutdown' callback to teardown DFLL
on a reboot, but likely that there are other clock-related problems as
well that may break KEXEC and thus it is not very important at the moment.
[snip]
^ permalink raw reply
* Re: [PATCH v9 2/2] media: v4l: xilinx: Add Xilinx MIPI CSI-2 Rx Subsystem driver
From: Sakari Ailus @ 2019-07-16 18:33 UTC (permalink / raw)
To: Vishal Sagar
Cc: Vishal Sagar, Hyun Kwon, laurent.pinchart@ideasonboard.com,
mchehab@kernel.org, robh+dt@kernel.org, mark.rutland@arm.com,
Michal Simek, linux-media@vger.kernel.org,
devicetree@vger.kernel.org, hans.verkuil@cisco.com,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, Dinesh Kumar, Sandip Kothari,
Luca Ceresoli, Jacopo Mondi
In-Reply-To: <CH2PR02MB6088B5E97833233486C53E9BA7F30@CH2PR02MB6088.namprd02.prod.outlook.com>
Hi Vishal,
On Thu, Jul 11, 2019 at 09:09:37AM +0000, Vishal Sagar wrote:
...
> > > +static int xcsi2rxss_subscribe_event(struct v4l2_subdev *sd,
> > > + struct v4l2_fh *fh,
> > > + struct v4l2_event_subscription *sub)
> > > +{
> > > + struct xcsi2rxss_state *xcsi2rxss = to_xcsi2rxssstate(sd);
> > > + int ret;
> > > +
> > > + mutex_lock(&xcsi2rxss->lock);
> > > +
> > > + switch (sub->type) {
> > > + case V4L2_EVENT_XILINX_CSI2RX_SLBF:
> > > + ret = v4l2_event_subscribe(fh, sub, XCSI_MAX_EVENT, NULL);
> >
> > Please either use dev_dbg() or propose an API for conveying this
> > information downstream in the pipeline.
> >
>
> I will use a dev_alert() in the irq handler for this for now and remove this event.
dev_dbg(), please, as this is certainly not fatal. We should probably have
an API to tell this to the downstream sub-device driver in the pipeline if
the hardware has no way to detect this independently.
--
Regards,
Sakari Ailus
^ permalink raw reply
* Re: [PATCH V5 11/18] clk: tegra210: Add support for Tegra210 clocks
From: Sowjanya Komatineni @ 2019-07-16 18:30 UTC (permalink / raw)
To: Dmitry Osipenko, Peter De Schrijver, Joseph Lo
Cc: thierry.reding, jonathanh, tglx, jason, marc.zyngier,
linus.walleij, stefan, mark.rutland, pgaikwad, sboyd, linux-clk,
linux-gpio, jckuo, talho, linux-tegra, linux-kernel, mperttunen,
spatra, robh+dt, devicetree
In-Reply-To: <76e341be-6f38-2bc1-048e-1aa6883f9b88@gmail.com>
On 7/16/19 11:25 AM, Dmitry Osipenko wrote:
> 16.07.2019 21:19, Sowjanya Komatineni пишет:
>> On 7/16/19 9:50 AM, Sowjanya Komatineni wrote:
>>> On 7/16/19 8:00 AM, Dmitry Osipenko wrote:
>>>> 16.07.2019 11:06, Peter De Schrijver пишет:
>>>>> On Tue, Jul 16, 2019 at 03:24:26PM +0800, Joseph Lo wrote:
>>>>>>> OK, Will add to CPUFreq driver...
>>>>>>>> The other thing that also need attention is that T124 CPUFreq driver
>>>>>>>> implicitly relies on DFLL driver to be probed first, which is icky.
>>>>>>>>
>>>>>>> Should I add check for successful dfll clk register explicitly in
>>>>>>> CPUFreq driver probe and defer till dfll clk registers?
>>>> Probably you should use the "device links". See [1][2] for the example.
>>>>
>>>> [1]
>>>> https://elixir.bootlin.com/linux/v5.2.1/source/drivers/gpu/drm/tegra/dc.c#L2383
>>>>
>>>>
>>>> [2] https://www.kernel.org/doc/html/latest/driver-api/device_link.html
>>>>
>>>> Return EPROBE_DEFER instead of EINVAL if device_link_add() fails. And
>>>> use of_find_device_by_node() to get the DFLL's device, see [3].
>>>>
>>>> [3]
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/devfreq/tegra20-devfreq.c#n100
>>>>
>>> Will go thru and add...
> Looks like I initially confused this case with getting orphaned clock.
> I'm now seeing that the DFLL driver registers the clock and then
> clk_get(dfll) should be returning EPROBE_DEFER until DFLL driver is
> probed, hence everything should be fine as-is and there is no real need
> for the 'device link'. Sorry for the confusion!
>
>>>>>> Sorry, I didn't follow the mail thread. Just regarding the DFLL part.
>>>>>>
>>>>>> As you know it, the DFLL clock is one of the CPU clock sources and
>>>>>> integrated with DVFS control logic with the regulator. We will not
>>>>>> switch
>>>>>> CPU to other clock sources once we switched to DFLL. Because the
>>>>>> CPU has
>>>>>> been regulated by the DFLL HW with the DVFS table (CVB or OPP table
>>>>>> you see
>>>>>> in the driver.). We shouldn't reparent it to other sources with unknew
>>>>>> freq/volt pair. That's not guaranteed to work. We allow switching to
>>>>>> open-loop mode but different sources.
>>>> Okay, then the CPUFreq driver will have to enforce DFLL freq to PLLP's
>>>> rate before switching to PLLP in order to have a proper CPU voltage.
>>> PLLP freq is safe to work for any CPU voltage. So no need to enforce
>>> DFLL freq to PLLP rate before changing CCLK_G source to PLLP during
>>> suspend
>>>
>> Sorry, please ignore my above comment. During suspend, need to change
>> CCLK_G source to PLLP when dfll is in closed loop mode first and then
>> dfll need to be set to open loop.
> Okay.
>
>>>>>> And I don't exactly understand why we need to switch to PLLP in CPU
>>>>>> idle
>>>>>> driver. Just keep it on CL-DVFS mode all the time.
>>>>>>
>>>>>> In SC7 entry, the dfll suspend function moves it the open-loop
>>>>>> mode. That's
>>>>>> all. The sc7-entryfirmware will handle the rest of the sequence to
>>>>>> turn off
>>>>>> the CPU power.
>>>>>>
>>>>>> In SC7 resume, the warmboot code will handle the sequence to turn on
>>>>>> regulator and power up the CPU cluster. And leave it on PLL_P. After
>>>>>> resuming to the kernel, we re-init DFLL, restore the CPU clock
>>>>>> policy (CPU
>>>>>> runs on DFLL open-loop mode) and then moving to close-loop mode.
>>>> The DFLL is re-inited after switching CCLK to DFLL parent during of the
>>>> early clocks-state restoring by CaR driver. Hence instead of having odd
>>>> hacks in the CaR driver, it is much nicer to have a proper
>>>> suspend-resume sequencing of the device drivers. In this case CPUFreq
>>>> driver is the driver that enables DFLL and switches CPU to that clock
>>>> source, which means that this driver is also should be responsible for
>>>> management of the DFLL's state during of suspend/resume process. If
>>>> CPUFreq driver disables DFLL during suspend and re-enables it during
>>>> resume, then looks like the CaR driver hacks around DFLL are not needed.
>>>>
>>>>>> The DFLL part looks good to me. BTW, change the patch subject to "Add
>>>>>> suspend-resume support" seems more appropriate to me.
>>>>>>
>>>>> To clarify this, the sequences for DFLL use are as follows (assuming
>>>>> all
>>>>> required DFLL hw configuration has been done)
>>>>>
>>>>> Switch to DFLL:
>>>>> 0) Save current parent and frequency
>>>>> 1) Program DFLL to open loop mode
>>>>> 2) Enable DFLL
>>>>> 3) Change cclk_g parent to DFLL
>>>>> For OVR regulator:
>>>>> 4) Change PWM output pin from tristate to output
>>>>> 5) Enable DFLL PWM output
>>>>> For I2C regulator:
>>>>> 4) Enable DFLL I2C output
>>>>> 6) Program DFLL to closed loop mode
>>>>>
>>>>> Switch away from DFLL:
>>>>> 0) Change cclk_g parent to PLLP so the CPU frequency is ok for any
>>>>> vdd_cpu voltage
>>>>> 1) Program DFLL to open loop mode
>>>>>
>> I see during switch away from DFLL (suspend), cclk_g parent is not
>> changed to PLLP before changing dfll to open loop mode.
>>
>> Will add this ...
> The CPUFreq driver switches parent to PLLP during the probe, similar
> should be done on suspend.
>
> I'm also wondering if it's always safe to switch to PLLP in the probe.
> If CPU is running on a lower freq than PLLP, then some other more
> appropriate intermediate parent should be selected.
>
CPU parents are PLL_X, PLL_P, and dfll. PLL_X always runs at higher rate
so switching to PLL_P during CPUFreq probe prior to dfll clock enable
should be safe.
>>>>> For OVR regulator:
>>>>> 2) Change PWM output pin from output to tristate: vdd_cpu will go back
>>>>> to hardwired boot voltage.
>>>>> 3) Disable DFLL PWM output
>>>>>
>>>>> For I2C regulator:
>>>>> 2) Program vdd_cpu regulator voltage to the boot voltage
>>>>> 3) Disable DFLL I2C output
>>>>>
>>>>> 4) Reprogram parent saved in step 0 of 'Switch to DFLL' to the saved
>>>>> frequency
>>>>> 5) Change cclk_g parent to saved parent
>>>>> 6) Disable DFLL
>>> This is the same sequence currently implemented. But dfll
>>> suspend/resume calls are thru Tegra210 clock driver.
>>>
>>> Dmitry wants to have dfll suspend/resume along with CCLK_G restore to
>>> happen from CPUFreq driver pm_ops rather than tegra210 clock driver or
>>> tegra dfll driver.
>>>
>>> Will move it to CPUFreq driver...
>>>
>> Thanks!
^ permalink raw reply
* Re: [PATCH 1/8] tpm: block messages while suspended
From: Stephen Boyd @ 2019-07-16 18:28 UTC (permalink / raw)
To: Herbert Xu
Cc: Jason Gunthorpe, Peter Huewe, Jarkko Sakkinen, Andrey Pronin,
linux-kernel, Arnd Bergmann, Greg Kroah-Hartman, linux-integrity,
devicetree, Duncan Laurie, Guenter Roeck, Matt Mackall,
linux-crypto
In-Reply-To: <20190624142654.zprcpz42hivuyrjq@gondor.apana.org.au>
Quoting Herbert Xu (2019-06-24 07:26:54)
> On Thu, Jun 20, 2019 at 06:03:17PM -0700, Stephen Boyd wrote:
> >
> > What do you think of the attached patch? I haven't tested it, but it
> > would make sure that the kthread is frozen so that the hardware can be
> > resumed before the kthread is thawed and tries to go touch the hardware.
>
> Looks good to me.
>
Thanks for checking. I haven't been able to reproduce the problem yet to
confirm this is actually fixing anything, even after tweaking the sysctl
values for khwrng to try and force the thread to run continuously.
>From reading the bug report that caused this 'is_suspended' code to be
added to the driver I'm fairly convinced this is the right solution. To
give some more background, it looks like we use s2idle suspend (i.e.
all CPUs are idle across suspend) and then we have the kthread running
to ask the hardware for some more random numbers. The i2c transaction
fails when asking the hwrng for data, and we see these messages printed
on the resume path:
i2c_designware i2c_designware.2: i2c_dw_handle_tx_abort: lost arbitration
tpm tpm0: i2c transfer failed (attempt 3/3): -11
tpm0: tpm_transmit: tpm_send: error -11
hwrng: no data available
Userspace tasks are thawed after this failure so it looks like something
in the kernel kicks khwrng to grab more data before the i2c bus can be
resumed.
^ permalink raw reply
* Re: [PATCH V5 11/18] clk: tegra210: Add support for Tegra210 clocks
From: Dmitry Osipenko @ 2019-07-16 18:25 UTC (permalink / raw)
To: Sowjanya Komatineni, Peter De Schrijver, Joseph Lo
Cc: thierry.reding, jonathanh, tglx, jason, marc.zyngier,
linus.walleij, stefan, mark.rutland, pgaikwad, sboyd, linux-clk,
linux-gpio, jckuo, talho, linux-tegra, linux-kernel, mperttunen,
spatra, robh+dt, devicetree
In-Reply-To: <2b701832-5548-7c83-7c17-05cc2f1470c8@nvidia.com>
16.07.2019 21:19, Sowjanya Komatineni пишет:
>
> On 7/16/19 9:50 AM, Sowjanya Komatineni wrote:
>>
>> On 7/16/19 8:00 AM, Dmitry Osipenko wrote:
>>> 16.07.2019 11:06, Peter De Schrijver пишет:
>>>> On Tue, Jul 16, 2019 at 03:24:26PM +0800, Joseph Lo wrote:
>>>>>> OK, Will add to CPUFreq driver...
>>>>>>> The other thing that also need attention is that T124 CPUFreq driver
>>>>>>> implicitly relies on DFLL driver to be probed first, which is icky.
>>>>>>>
>>>>>> Should I add check for successful dfll clk register explicitly in
>>>>>> CPUFreq driver probe and defer till dfll clk registers?
>>> Probably you should use the "device links". See [1][2] for the example.
>>>
>>> [1]
>>> https://elixir.bootlin.com/linux/v5.2.1/source/drivers/gpu/drm/tegra/dc.c#L2383
>>>
>>>
>>> [2] https://www.kernel.org/doc/html/latest/driver-api/device_link.html
>>>
>>> Return EPROBE_DEFER instead of EINVAL if device_link_add() fails. And
>>> use of_find_device_by_node() to get the DFLL's device, see [3].
>>>
>>> [3]
>>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/devfreq/tegra20-devfreq.c#n100
>>>
>> Will go thru and add...
Looks like I initially confused this case with getting orphaned clock.
I'm now seeing that the DFLL driver registers the clock and then
clk_get(dfll) should be returning EPROBE_DEFER until DFLL driver is
probed, hence everything should be fine as-is and there is no real need
for the 'device link'. Sorry for the confusion!
>>>>> Sorry, I didn't follow the mail thread. Just regarding the DFLL part.
>>>>>
>>>>> As you know it, the DFLL clock is one of the CPU clock sources and
>>>>> integrated with DVFS control logic with the regulator. We will not
>>>>> switch
>>>>> CPU to other clock sources once we switched to DFLL. Because the
>>>>> CPU has
>>>>> been regulated by the DFLL HW with the DVFS table (CVB or OPP table
>>>>> you see
>>>>> in the driver.). We shouldn't reparent it to other sources with unknew
>>>>> freq/volt pair. That's not guaranteed to work. We allow switching to
>>>>> open-loop mode but different sources.
>>> Okay, then the CPUFreq driver will have to enforce DFLL freq to PLLP's
>>> rate before switching to PLLP in order to have a proper CPU voltage.
>>
>> PLLP freq is safe to work for any CPU voltage. So no need to enforce
>> DFLL freq to PLLP rate before changing CCLK_G source to PLLP during
>> suspend
>>
> Sorry, please ignore my above comment. During suspend, need to change
> CCLK_G source to PLLP when dfll is in closed loop mode first and then
> dfll need to be set to open loop.
Okay.
>>>>> And I don't exactly understand why we need to switch to PLLP in CPU
>>>>> idle
>>>>> driver. Just keep it on CL-DVFS mode all the time.
>>>>>
>>>>> In SC7 entry, the dfll suspend function moves it the open-loop
>>>>> mode. That's
>>>>> all. The sc7-entryfirmware will handle the rest of the sequence to
>>>>> turn off
>>>>> the CPU power.
>>>>>
>>>>> In SC7 resume, the warmboot code will handle the sequence to turn on
>>>>> regulator and power up the CPU cluster. And leave it on PLL_P. After
>>>>> resuming to the kernel, we re-init DFLL, restore the CPU clock
>>>>> policy (CPU
>>>>> runs on DFLL open-loop mode) and then moving to close-loop mode.
>>> The DFLL is re-inited after switching CCLK to DFLL parent during of the
>>> early clocks-state restoring by CaR driver. Hence instead of having odd
>>> hacks in the CaR driver, it is much nicer to have a proper
>>> suspend-resume sequencing of the device drivers. In this case CPUFreq
>>> driver is the driver that enables DFLL and switches CPU to that clock
>>> source, which means that this driver is also should be responsible for
>>> management of the DFLL's state during of suspend/resume process. If
>>> CPUFreq driver disables DFLL during suspend and re-enables it during
>>> resume, then looks like the CaR driver hacks around DFLL are not needed.
>>>
>>>>> The DFLL part looks good to me. BTW, change the patch subject to "Add
>>>>> suspend-resume support" seems more appropriate to me.
>>>>>
>>>> To clarify this, the sequences for DFLL use are as follows (assuming
>>>> all
>>>> required DFLL hw configuration has been done)
>>>>
>>>> Switch to DFLL:
>>>> 0) Save current parent and frequency
>>>> 1) Program DFLL to open loop mode
>>>> 2) Enable DFLL
>>>> 3) Change cclk_g parent to DFLL
>>>> For OVR regulator:
>>>> 4) Change PWM output pin from tristate to output
>>>> 5) Enable DFLL PWM output
>>>> For I2C regulator:
>>>> 4) Enable DFLL I2C output
>>>> 6) Program DFLL to closed loop mode
>>>>
>>>> Switch away from DFLL:
>>>> 0) Change cclk_g parent to PLLP so the CPU frequency is ok for any
>>>> vdd_cpu voltage
>>>> 1) Program DFLL to open loop mode
>>>>
> I see during switch away from DFLL (suspend), cclk_g parent is not
> changed to PLLP before changing dfll to open loop mode.
>
> Will add this ...
The CPUFreq driver switches parent to PLLP during the probe, similar
should be done on suspend.
I'm also wondering if it's always safe to switch to PLLP in the probe.
If CPU is running on a lower freq than PLLP, then some other more
appropriate intermediate parent should be selected.
>>>> For OVR regulator:
>>>> 2) Change PWM output pin from output to tristate: vdd_cpu will go back
>>>> to hardwired boot voltage.
>>>> 3) Disable DFLL PWM output
>>>>
>>>> For I2C regulator:
>>>> 2) Program vdd_cpu regulator voltage to the boot voltage
>>>> 3) Disable DFLL I2C output
>>>>
>>>> 4) Reprogram parent saved in step 0 of 'Switch to DFLL' to the saved
>>>> frequency
>>>> 5) Change cclk_g parent to saved parent
>>>> 6) Disable DFLL
>>
>> This is the same sequence currently implemented. But dfll
>> suspend/resume calls are thru Tegra210 clock driver.
>>
>> Dmitry wants to have dfll suspend/resume along with CCLK_G restore to
>> happen from CPUFreq driver pm_ops rather than tegra210 clock driver or
>> tegra dfll driver.
>>
>> Will move it to CPUFreq driver...
>>
> Thanks!
^ permalink raw reply
* Re: [PATCH V5 11/18] clk: tegra210: Add support for Tegra210 clocks
From: Sowjanya Komatineni @ 2019-07-16 18:19 UTC (permalink / raw)
To: Dmitry Osipenko, Peter De Schrijver, Joseph Lo
Cc: thierry.reding, jonathanh, tglx, jason, marc.zyngier,
linus.walleij, stefan, mark.rutland, pgaikwad, sboyd, linux-clk,
linux-gpio, jckuo, talho, linux-tegra, linux-kernel, mperttunen,
spatra, robh+dt, devicetree
In-Reply-To: <72b5df8c-8acb-d0d0-ebcf-b406e8404973@nvidia.com>
On 7/16/19 9:50 AM, Sowjanya Komatineni wrote:
>
> On 7/16/19 8:00 AM, Dmitry Osipenko wrote:
>> 16.07.2019 11:06, Peter De Schrijver пишет:
>>> On Tue, Jul 16, 2019 at 03:24:26PM +0800, Joseph Lo wrote:
>>>>> OK, Will add to CPUFreq driver...
>>>>>> The other thing that also need attention is that T124 CPUFreq driver
>>>>>> implicitly relies on DFLL driver to be probed first, which is icky.
>>>>>>
>>>>> Should I add check for successful dfll clk register explicitly in
>>>>> CPUFreq driver probe and defer till dfll clk registers?
>> Probably you should use the "device links". See [1][2] for the example.
>>
>> [1]
>> https://elixir.bootlin.com/linux/v5.2.1/source/drivers/gpu/drm/tegra/dc.c#L2383
>>
>>
>> [2] https://www.kernel.org/doc/html/latest/driver-api/device_link.html
>>
>> Return EPROBE_DEFER instead of EINVAL if device_link_add() fails. And
>> use of_find_device_by_node() to get the DFLL's device, see [3].
>>
>> [3]
>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/devfreq/tegra20-devfreq.c#n100
>>
> Will go thru and add...
>>>> Sorry, I didn't follow the mail thread. Just regarding the DFLL part.
>>>>
>>>> As you know it, the DFLL clock is one of the CPU clock sources and
>>>> integrated with DVFS control logic with the regulator. We will not
>>>> switch
>>>> CPU to other clock sources once we switched to DFLL. Because the
>>>> CPU has
>>>> been regulated by the DFLL HW with the DVFS table (CVB or OPP table
>>>> you see
>>>> in the driver.). We shouldn't reparent it to other sources with unknew
>>>> freq/volt pair. That's not guaranteed to work. We allow switching to
>>>> open-loop mode but different sources.
>> Okay, then the CPUFreq driver will have to enforce DFLL freq to PLLP's
>> rate before switching to PLLP in order to have a proper CPU voltage.
>
> PLLP freq is safe to work for any CPU voltage. So no need to enforce
> DFLL freq to PLLP rate before changing CCLK_G source to PLLP during
> suspend
>
Sorry, please ignore my above comment. During suspend, need to change
CCLK_G source to PLLP when dfll is in closed loop mode first and then
dfll need to be set to open loop.
>>>> And I don't exactly understand why we need to switch to PLLP in CPU
>>>> idle
>>>> driver. Just keep it on CL-DVFS mode all the time.
>>>>
>>>> In SC7 entry, the dfll suspend function moves it the open-loop
>>>> mode. That's
>>>> all. The sc7-entryfirmware will handle the rest of the sequence to
>>>> turn off
>>>> the CPU power.
>>>>
>>>> In SC7 resume, the warmboot code will handle the sequence to turn on
>>>> regulator and power up the CPU cluster. And leave it on PLL_P. After
>>>> resuming to the kernel, we re-init DFLL, restore the CPU clock
>>>> policy (CPU
>>>> runs on DFLL open-loop mode) and then moving to close-loop mode.
>> The DFLL is re-inited after switching CCLK to DFLL parent during of the
>> early clocks-state restoring by CaR driver. Hence instead of having odd
>> hacks in the CaR driver, it is much nicer to have a proper
>> suspend-resume sequencing of the device drivers. In this case CPUFreq
>> driver is the driver that enables DFLL and switches CPU to that clock
>> source, which means that this driver is also should be responsible for
>> management of the DFLL's state during of suspend/resume process. If
>> CPUFreq driver disables DFLL during suspend and re-enables it during
>> resume, then looks like the CaR driver hacks around DFLL are not needed.
>>
>>>> The DFLL part looks good to me. BTW, change the patch subject to "Add
>>>> suspend-resume support" seems more appropriate to me.
>>>>
>>> To clarify this, the sequences for DFLL use are as follows (assuming
>>> all
>>> required DFLL hw configuration has been done)
>>>
>>> Switch to DFLL:
>>> 0) Save current parent and frequency
>>> 1) Program DFLL to open loop mode
>>> 2) Enable DFLL
>>> 3) Change cclk_g parent to DFLL
>>> For OVR regulator:
>>> 4) Change PWM output pin from tristate to output
>>> 5) Enable DFLL PWM output
>>> For I2C regulator:
>>> 4) Enable DFLL I2C output
>>> 6) Program DFLL to closed loop mode
>>>
>>> Switch away from DFLL:
>>> 0) Change cclk_g parent to PLLP so the CPU frequency is ok for any
>>> vdd_cpu voltage
>>> 1) Program DFLL to open loop mode
>>>
I see during switch away from DFLL (suspend), cclk_g parent is not
changed to PLLP before changing dfll to open loop mode.
Will add this ...
>>> For OVR regulator:
>>> 2) Change PWM output pin from output to tristate: vdd_cpu will go back
>>> to hardwired boot voltage.
>>> 3) Disable DFLL PWM output
>>>
>>> For I2C regulator:
>>> 2) Program vdd_cpu regulator voltage to the boot voltage
>>> 3) Disable DFLL I2C output
>>>
>>> 4) Reprogram parent saved in step 0 of 'Switch to DFLL' to the saved
>>> frequency
>>> 5) Change cclk_g parent to saved parent
>>> 6) Disable DFLL
>
> This is the same sequence currently implemented. But dfll
> suspend/resume calls are thru Tegra210 clock driver.
>
> Dmitry wants to have dfll suspend/resume along with CCLK_G restore to
> happen from CPUFreq driver pm_ops rather than tegra210 clock driver or
> tegra dfll driver.
>
> Will move it to CPUFreq driver...
>
Thanks!
^ permalink raw reply
* Re: [PATCH v2 11/11] interconnect: Add devfreq support
From: Sibi Sankar @ 2019-07-16 18:12 UTC (permalink / raw)
To: Saravana Kannan, Georgi Djakov
Cc: Rob Herring, Mark Rutland, Viresh Kumar, Nishanth Menon,
Stephen Boyd, Rafael J. Wysocki, MyungJoo Ham, Kyungmin Park,
Chanwoo Choi, Rajendra Nayak, Jordan Crouse, Vincent Guittot,
Bjorn Andersson, amit.kucheria, seansw, daidavid1, evgreen,
Android Kernel Team, linux-pm, devicetree, linux-kernel, adharmap
In-Reply-To: <CAGETcx-xU9i1FJB5JecUoyZEfWpD8f+o9bC3SQmb-=3fLVbmQw@mail.gmail.com>
Hey Saravana,
On 6/18/19 2:48 AM, Saravana Kannan wrote:
> On Mon, Jun 17, 2019 at 8:44 AM Georgi Djakov <georgi.djakov@linaro.org> wrote:
>>
>> Hi Saravana,
>>
>> On 6/14/19 07:17, Saravana Kannan wrote:
>>> Add a icc_create_devfreq() and icc_remove_devfreq() to create and remove
>>> devfreq devices for interconnect paths. A driver can create/remove devfreq
>>> devices for the interconnects needed for its device by calling these APIs.
>>> This would allow various devfreq governors to work with interconnect paths
>>> and the device driver itself doesn't have to actively manage the bandwidth
>>> votes for the interconnects.
>>
>> Thanks for the patches, but creating devfreq devices for each interconnect path
>> seems odd to me - at least for consumers that already use a governor.
>
> Each governor instance always handles one "frequency" (more like
> performance) domain at a time. So if a consumer is already using a
> governor to scale the hardware block, then using another governor to
> scale the interconnect performance points is the right way to go about
> it. In fact, that's exactly what devfreq passive governor's
> documentation even says it's meant for. That's also what cpufreq does
> for each cluster/CPU frequency domain too.
>
>> So for DDR
>> scaling for example, are you suggesting that we add a devfreq device from the
>> cpufreq driver in order to scale the interconnect between CPU<->DDR?
>
> Yes in general. Although, CPUs are a special case because CPUs don't
> go through devfreq. So passive governor as it stands today won't work.
> CPU<->DDR scaling might need a separate governor (unlikely) or some
> changes to the passive governor that I'm happy to work on once we
> settle this for general devices like GPU, etc. But the DT format for
> CPUs will be identical to GPUs or any other device.
using icc_create_devfreq from the cpufreq-hw driver on SDM845 SoC
to scale CPU<->DDR would cause a circular dependency. (i.e) with
the addition of cpufreq notifier to the passive governor as in
https://patchwork.kernel.org/patch/11046147/ devm_devfreq_add_device
would require the cpufreq transistion notifier register and cpu
freq_cpu_get to go through. Please add your thought on addressing this.
>
>> Also if the
>> GPU is already using devfreq, should we add a devfreq per each interconnect
>> path? What would be the benefit in this case - using different governors for
>> bandwidth scaling maybe?
>
> When saying "separate/different governors" in this email, I mean both
> different instance of the same governor logic with different tunables
> AND actually different algorithms/governor logic entirely.
>
> The heuristics to use for each interconnect path might be (more like,
> will be) different based on hardware characteristics (Eg: what voltage
> domains the interconnect is sitting on) and what interconnect
> information is available (Eg: Just busy time vs bandwidth count vs no
> information etc) -- so having separate governors for each interconnect
> path makes a lot of sense. It also allows userspace to control the
> policy for scaling each of those paths based on product use cases.
>
> For example, when the GPU is just doing simple UI rendering, userspace
> can use the max_freq sysfs file for the devfreq device to disallow high
> bandwidth OPPs on the GPU<->DDR path, but those higher OPPs could be
> allowed by userspace when the GPU is used for games. Having devfreq
> device for each interconnect path also make it easy to debug
> performance issues -- you can independently change the votes for each
> path to figure out what is causing the bottleneck, etc.
>
> Adding a devfreq device for interconnect voting with a few lines gives
> all these features "for free".
>
> This doesn't mean all users of interconnect framework NEED to use
> devfreq for interconnect. They might do it simply based on
> calculations based on the use case (Eg: display driver from display
> resolution). But if they are trying to use any kind of
> algorithm/heuristics, writing it as a devfreq governor should be
> encouraged.
>
> Also want to point out that BW OPPs also work for drivers that don't
> use devfreq at all. The interconnect-opp-table just lists the
> meaningful OPP leveld for the path and the device driver can pick one
> entry from the table based on the use case.
>
> Thanks,
> Saravana
>
>
>
--
Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc, is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply
* Re: [PATCH v9 04/18] kunit: test: add kunit_stream a std::stream like logger
From: Brendan Higgins @ 2019-07-16 17:51 UTC (permalink / raw)
To: Stephen Boyd
Cc: Petr Mladek, open list:DOCUMENTATION, Peter Zijlstra,
Amir Goldstein, dri-devel, Sasha Levin, Masahiro Yamada,
Michael Ellerman, open list:KERNEL SELFTEST FRAMEWORK, shuah,
linux-nvdimm, Frank Rowand, Knut Omang, Kieran Bingham, wfg,
Joel Stanley, David Rientjes, Jeff Dike, Dan Carpenter,
devicetree, linux-kbuild,
"Bird, Timothy" <Tim.Bird>
In-Reply-To: <20190716153002.49E292054F@mail.kernel.org>
On Tue, Jul 16, 2019 at 8:30 AM Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Brendan Higgins (2019-07-16 01:37:34)
> > On Tue, Jul 16, 2019 at 12:57 AM Brendan Higgins
> > <brendanhiggins@google.com> wrote:
> > >
> > > On Mon, Jul 15, 2019 at 3:15 PM Stephen Boyd <sboyd@kernel.org> wrote:
> > > >
> > > > Quoting Brendan Higgins (2019-07-12 01:17:30)
> > > > > diff --git a/include/kunit/kunit-stream.h b/include/kunit/kunit-stream.h
> > > > > new file mode 100644
> > > > > index 0000000000000..a7b53eabf6be4
> > > > > --- /dev/null
> > > > > +++ b/include/kunit/kunit-stream.h
> > > > > +/**
> > > > > + * struct kunit_stream - a std::stream style string builder.
> > > > > + *
> > > > > + * A std::stream style string builder. Allows messages to be built up and
> > > > > + * printed all at once.
> > > > > + */
> > > > > +struct kunit_stream {
> > > > > + /* private: internal use only. */
> > > > > + struct kunit *test;
> > > > > + const char *level;
> > > >
> > > > Is the level changed? See my comment below, but I wonder if this whole
> > > > struct can go away and the wrappers can just operate on 'struct
> > > > string_stream' instead.
> > >
> > > I was inclined to agree with you when I first read your comment, but
> > > then I thought about the case that someone wants to add in a debug
> > > message (of which I currently have none). I think under most
> > > circumstances a user of kunit_stream would likely want to pick a
> > > default verbosity that maybe I should provide, but may still want
> > > different verbosity levels.
> > >
> > > The main reason I want to keep the types separate, string_stream vs.
> > > kunit_stream, is that they are intended to be used differently.
> > > string_stream is just a generic string builder. If you are using that,
> > > you are expecting to see someone building the string at some point and
> > > then doing something interesting with it. kunit_stream really tells
> > > you specifically that KUnit is putting together a message to
> > > communicate something to a user of KUnit. It is really used in a very
> > > specific way, and I wouldn't want to generalize its usage beyond how
> > > it is currently used. I think in order to preserve the author's
> > > intention it adds clarity to keep the types separate regardless of how
> > > similar they might be in reality.
>
> You may want to add some of these reasons to the commit text.
Will do.
> > > > > +
> > > > > + if (!string_stream_is_empty(stream->internal_stream)) {
> > > > > + kunit_err(stream->test,
> > > > > + "End of test case reached with uncommitted stream entries\n");
> > > > > + kunit_stream_commit(stream);
> > > > > + }
> > > > > +}
> > > > > +
> > > >
> > > > Nitpick: Drop this extra newline.
> > >
> > > Oops, nice catch.
> >
> > Not super important, but I don't want you to think that I am ignoring
> > you. I think you must have unintentionally deleted the last function
> > in this file, or maybe you are referring to something that I am just
> > not seeing, but I don't see the extra newline here.
>
> No worries. Sorry for the noise.
>
> > > property of the input, it may or may not be enough information on its
> > > own for the expectation to fail, but we want to share the result of
> > > the property check with the user regardless, BUT only if the
> > > expectation as a whole fails.
> > >
> > > Hence, we can have multiple `struct kunit_stream`s associated with a
> > > `struct kunit` active at any given time.
>
> Makes sense. I wasn't sure if there were more than one stream associated
> with a test. Sounds like there are many to one so it can't just be a
> member of the test. This could be documented somewhere so this question
> doesn't come up again.
Sounds good. Will do.
Thanks!
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply
* Re: [PATCH v9 04/18] kunit: test: add kunit_stream a std::stream like logger
From: Stephen Boyd @ 2019-07-16 17:50 UTC (permalink / raw)
To: Brendan Higgins
Cc: Frank Rowand, Greg KH, Josh Poimboeuf, Kees Cook, Kieran Bingham,
Luis Chamberlain, Peter Zijlstra, Rob Herring, shuah,
Theodore Ts'o, Masahiro Yamada, devicetree, dri-devel,
kunit-dev, open list:DOCUMENTATION, linux-fsdevel, linux-kbuild,
Linux Kernel Mailing List, KERNEL
In-Reply-To: <CAFd5g44_axVHNMBzxSURQB_-R+Rif7cZcg7PyZ_SS+5hcy5jZA@mail.gmail.com>
Quoting Brendan Higgins (2019-07-16 01:37:34)
> On Tue, Jul 16, 2019 at 12:57 AM Brendan Higgins
> <brendanhiggins@google.com> wrote:
> >
> > A `struct kunit_stream` is usually associated with a message that is
> > being built up over time like maybe an expectation; it is meant to
> > capture the idea that we might want to send some information out to
> > the user pertaining to some thing 'X', but we aren't sure that we
> > actually want to send it until 'X' is complete, but do to the nature
> > of 'X' it is easier to start constructing the message before 'X' is
> > complete.
> >
> > Consider a complicated expectation, there might be multiple conditions
> > that satisfy it and multiple conditions which could make it fail. As
> > we start exploring the input to the expectation we gain information
> > that we might want to share back with the user if the expectation were
> > to fail and we might get that information before we are actually sure
> > that the expectation does indeed fail.
> >
> > When we first step into the expectation we immediately know the
> > function name, file name, and line number where we are called and
> > would want to put that information into any message we would send to
> > the user about this expectation. Next, we might want to check a
> > property of the input, it may or may not be enough information on its
> > own for the expectation to fail, but we want to share the result of
> > the property check with the user regardless, BUT only if the
> > expectation as a whole fails.
> >
> > Hence, we can have multiple `struct kunit_stream`s associated with a
> > `struct kunit` active at any given time.
I'm coming back to this now after reading the rest of the patches that
deal with assertions and expectations. It looks like the string stream
is there to hold a few different pieces of information:
- Line Number
- File Name
- Function Name
The above items could be stored in a structure on the stack that then
gets printed and formatted when the expectation or assertion fails. That
would make the whole string stream structure and code unnecessary.
The only hypothetical case where this can't be done is a complicated
assertion or expectation that does more than one check and can't be
written as a function that dumps out what went wrong. Is this a real
problem? Maybe such an assertion should just open code that logic so we
don't have to build up a string for all the other simple cases.
It seems far simpler to get rid of the string stream API and just have a
struct for this.
struct kunit_fail_msg {
const char *line;
const char *file;
const char *func;
const char *msg;
};
Then you can have the assertion macros create this on the stack (with
another macro?).
#define DEFINE_KUNIT_FAIL_MSG(name, _msg) \
struct kunit_fail_msg name = { \
.line = __LINE__, \
.file = __FILE__, \
.func = __func__, \
.msg = _msg, \
}
Note: I don't know if the __LINE__ above will use the macro location, so
this probably needs another wrapper to put the right line number there.
I don't want to derail this whole series on this topic, but it seems
like a bunch of code is there to construct this same set of information
over and over again into a buffer a little bit at a time and then throw
it away when nothing fails just because we may want to support the case
where we have some unstructured data to inform the user about.
Why not build in the structured part into the framework (i.e. the struct
above) so that it's always there and then add the string building part
later when we have to?
^ permalink raw reply
* Re: [PATCH v3 2/6] OPP: Add support for bandwidth OPP tables
From: Sibi Sankar @ 2019-07-16 17:33 UTC (permalink / raw)
To: Saravana Kannan, Georgi Djakov, Rob Herring, Mark Rutland,
Viresh Kumar, Nishanth Menon, Stephen Boyd, Rafael J. Wysocki
Cc: vincent.guittot, seansw, daidavid1, Rajendra Nayak,
bjorn.andersson, evgreen, kernel-team, linux-pm, devicetree,
linux-kernel, adharmap
In-Reply-To: <20190703011020.151615-3-saravanak@google.com>
Hey Saravana,
On 7/3/19 6:40 AM, Saravana Kannan wrote:
> Not all devices quantify their performance points in terms of frequency.
> Devices like interconnects quantify their performance points in terms of
> bandwidth. We need a way to represent these bandwidth levels in OPP. So,
> add support for parsing bandwidth OPPs from DT.
>
> Signed-off-by: Saravana Kannan <saravanak@google.com>
> ---
> drivers/opp/of.c | 34 ++++++++++++++++++++++++++++++++--
> drivers/opp/opp.h | 4 +++-
> 2 files changed, 35 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/opp/of.c b/drivers/opp/of.c
> index c10c782d15aa..54fa70ed2adc 100644
> --- a/drivers/opp/of.c
> +++ b/drivers/opp/of.c
> @@ -552,6 +552,35 @@ void dev_pm_opp_of_remove_table(struct device *dev)
> }
> EXPORT_SYMBOL_GPL(dev_pm_opp_of_remove_table);
>
> +static int _read_opp_key(struct dev_pm_opp *new_opp, struct device_node *np)
> +{
> + int ret;
> + u64 rate;
> + u32 bw;
> +
> + ret = of_property_read_u64(np, "opp-hz", &rate);
> + if (!ret) {
> + /*
> + * Rate is defined as an unsigned long in clk API, and so
> + * casting explicitly to its type. Must be fixed once rate is 64
> + * bit guaranteed in clk API.
> + */
> + new_opp->rate = (unsigned long)rate
now that the rate gets set here, please remove the rate assignment in
_opp_add_static_v2
> + return 0;
> + }
> +
> + ret = of_property_read_u32(np, "opp-peak-KBps", &bw);
> + if (ret)
> + return ret;
> + new_opp->rate = (unsigned long) &bw;
should be bw instead
> +
> + ret = of_property_read_u32(np, "opp-avg-KBps", &bw);
> + if (!ret)
> + new_opp->avg_bw = (unsigned long) &bw;
ditto
> +
> + return 0;
> +}
> +
> /**
> * _opp_add_static_v2() - Allocate static OPPs (As per 'v2' DT bindings)
> * @opp_table: OPP table
> @@ -589,11 +618,12 @@ static struct dev_pm_opp *_opp_add_static_v2(struct opp_table *opp_table,
> if (!new_opp)
> return ERR_PTR(-ENOMEM);
>
> - ret = of_property_read_u64(np, "opp-hz", &rate);
> + ret = _read_opp_key(new_opp, np);
> if (ret < 0) {
> /* "opp-hz" is optional for devices like power domains. */
> if (!opp_table->is_genpd) {
> - dev_err(dev, "%s: opp-hz not found\n", __func__);
> + dev_err(dev, "%s: opp-hz or opp-peak-bw not found\n",
> + __func__);
please remove the else part where rate value will be reset.
> goto free_opp;
> }
>
> diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
> index 569b3525aa67..ead2cdafe957 100644
> --- a/drivers/opp/opp.h
> +++ b/drivers/opp/opp.h
> @@ -59,7 +59,8 @@ extern struct list_head opp_tables;
> * @turbo: true if turbo (boost) OPP
> * @suspend: true if suspend OPP
> * @pstate: Device's power domain's performance state.
> - * @rate: Frequency in hertz
> + * @rate: Frequency in hertz OR Peak bandwidth in kilobytes per second
> + * @avg_bw: Average bandwidth in kilobytes per second
> * @level: Performance level
> * @supplies: Power supplies voltage/current values
> * @clock_latency_ns: Latency (in nanoseconds) of switching to this OPP's
> @@ -81,6 +82,7 @@ struct dev_pm_opp {
> bool suspend;
> unsigned int pstate;
> unsigned long rate;
> + unsigned long avg_bw;
> unsigned int level;
>
> struct dev_pm_opp_supply *supplies;
>
--
Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc, is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply
* Re: [PATCH v3 1/6] dt-bindings: opp: Introduce opp-peak-KBps and opp-avg-KBps bindings
From: Sibi Sankar @ 2019-07-16 17:25 UTC (permalink / raw)
To: Saravana Kannan, Georgi Djakov, Rob Herring, Mark Rutland,
Viresh Kumar, Nishanth Menon, Stephen Boyd, Rafael J. Wysocki
Cc: vincent.guittot, seansw, daidavid1, Rajendra Nayak,
bjorn.andersson, evgreen, kernel-team, linux-pm, devicetree,
linux-kernel
In-Reply-To: <20190703011020.151615-2-saravanak@google.com>
Hey Saravana,
https://patchwork.kernel.org/patch/10850815/
There was already a discussion ^^ on how bandwidth bindings were to be
named.
On 7/3/19 6:40 AM, Saravana Kannan wrote:
> Interconnects often quantify their performance points in terms of
> bandwidth. So, add opp-peak-KBps (required) and opp-avg-KBps (optional) to
> allow specifying Bandwidth OPP tables in DT.
>
> opp-peak-KBps is a required property that replace opp-hz for Bandwidth OPP
> tables.
>
> opp-avg-KBps is an optional property that can be used in Bandwidth OPP
> tables.
>
> Signed-off-by: Saravana Kannan <saravanak@google.com>
> ---
> Documentation/devicetree/bindings/opp/opp.txt | 15 ++++++++++++---
> 1 file changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/opp/opp.txt b/Documentation/devicetree/bindings/opp/opp.txt
> index 76b6c79604a5..c869e87caa2a 100644
> --- a/Documentation/devicetree/bindings/opp/opp.txt
> +++ b/Documentation/devicetree/bindings/opp/opp.txt
> @@ -83,9 +83,14 @@ properties.
>
> Required properties:
> - opp-hz: Frequency in Hz, expressed as a 64-bit big-endian integer. This is a
> - required property for all device nodes but devices like power domains. The
> - power domain nodes must have another (implementation dependent) property which
> - uniquely identifies the OPP nodes.
> + required property for all device nodes but for devices like power domains or
> + bandwidth opp tables. The power domain nodes must have another (implementation
> + dependent) property which uniquely identifies the OPP nodes. The interconnect
> + opps are required to have the opp-peak-bw property.
> +
> +- opp-peak-KBps: Peak bandwidth in kilobytes per second, expressed as a 32-bit
> + big-endian integer. This is a required property for all devices that don't
> + have opp-hz. For example, bandwidth OPP tables for interconnect paths.
>
> Optional properties:
> - opp-microvolt: voltage in micro Volts.
> @@ -132,6 +137,10 @@ Optional properties:
> - opp-level: A value representing the performance level of the device,
> expressed as a 32-bit integer.
>
> +- opp-avg-KBps: Average bandwidth in kilobytes per second, expressed as a
> + 32-bit big-endian integer. This property is only meaningful in OPP tables
> + where opp-peak-KBps is present.
> +
> - clock-latency-ns: Specifies the maximum possible transition latency (in
> nanoseconds) for switching to this OPP from any other OPP.
>
>
--
Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc, is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply
* Re: [PATCH 2/6] irqchip/irq-pruss-intc: Add a PRUSS irqchip driver for PRUSS interrupts
From: Suman Anna @ 2019-07-16 17:21 UTC (permalink / raw)
To: David Lechner, Marc Zyngier, Rob Herring, Thomas Gleixner,
Jason Cooper
Cc: Tony Lindgren, Andrew F. Davis, Roger Quadros, Lokesh Vutla,
Grygorii Strashko, Sekhar Nori, Murali Karicheri, devicetree,
linux-omap, linux-arm-kernel, linux-kernel
In-Reply-To: <3d91800b-e858-8c73-5ea8-e99e5ea30e8e@lechnology.com>
Hi David,
On 7/11/19 11:45 AM, David Lechner wrote:
> On 7/7/19 10:52 PM, Suman Anna wrote:
>> From: "Andrew F. Davis" <afd@ti.com>
>>
>> The Programmable Real-Time Unit Subsystem (PRUSS) contains a local
>> interrupt controller (INTC) that can handle various system input events
>> and post interrupts back to the device-level initiators. The INTC can
>> support upto 64 input events with individual control configuration and
>> hardware prioritization. These events are mapped onto 10 output interrupt
>> lines through two levels of many-to-one mapping support. Different
>> interrupt lines are routed to the individual PRU cores or to the host
>> CPU, or to other devices on the SoC. Some of these events are sourced
>> from peripherals or other sub-modules within that PRUSS, while a few
>> others are sourced from SoC-level peripherals/devices.
>>
>> The PRUSS INTC platform driver manages this PRUSS interrupt controller
>> and implements an irqchip driver to provide a Linux standard way for
>> the PRU client users to enable/disable/ack/re-trigger a PRUSS system
>> event. The system events to interrupt channels and host interrupts
>> relies on the mapping configuration provided either through the PRU
>> firmware blob or via the PRU application's device tree node. The
>> mappings will be programmed during the boot/shutdown of a PRU core.
>>
>> The PRUSS INTC module is reference counted during the interrupt
>> setup phase through the irqchip's irq_request_resources() and
>> irq_release_resources() ops. This restricts the module from being
>> removed as long as there are active interrupt users.
>>
>> The driver currently supports and can be built for OMAP architecture
>> based AM335x, AM437x and AM57xx SoCs; Keystone2 architecture based
>> 66AK2G SoCs and Davinci architecture based OMAP-L13x/AM18x/DA850 SoCs.
>> All of these SoCs support 64 system events, 10 interrupt channels and
>> 10 output interrupt lines per PRUSS INTC with a few SoC integration
>> differences.
>>
>> NOTE:
>> Each PRU-ICSS's INTC on AM57xx SoCs is preceded by a Crossbar that
>> enables multiple external events to be routed to a specific number
>> of input interrupt events. Any non-default external interrupt event
>> directed towards PRUSS needs this crossbar to be setup properly.
>
> The all of the explanations above are very helpful. Great work.
>
>>
>> Signed-off-by: Andrew F. Davis <afd@ti.com>
>> Signed-off-by: Suman Anna <s-anna@ti.com>
>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>> ---
>> Prior version: https://patchwork.kernel.org/patch/10795761/
>>
>> drivers/irqchip/Kconfig | 10 +
>> drivers/irqchip/Makefile | 1 +
>> drivers/irqchip/irq-pruss-intc.c | 352 +++++++++++++++++++++++++++++++
>> 3 files changed, 363 insertions(+)
>> create mode 100644 drivers/irqchip/irq-pruss-intc.c
>>
>> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
>> index 659c5e0fb835..b0a9479d527c 100644
>> --- a/drivers/irqchip/Kconfig
>> +++ b/drivers/irqchip/Kconfig
>> @@ -447,6 +447,16 @@ config TI_SCI_INTA_IRQCHIP
>> If you wish to use interrupt aggregator irq resources managed
>> by the
>> TI System Controller, say Y here. Otherwise, say N.
>> +config TI_PRUSS_INTC
>> + tristate "TI PRU-ICSS Interrupt Controller"
>> + depends on ARCH_DAVINCI || SOC_AM33XX || SOC_AM437X || SOC_DRA7XX
>> || ARCH_KEYSTONE
>> + select IRQ_DOMAIN
>> + help
>> + This enables support for the PRU-ICSS Local Interrupt Controller
>> + present within a PRU-ICSS subsystem present on various TI SoCs.
>> + The PRUSS INTC enables various interrupts to be routed to
>> multiple
>> + different processors within the SoC.
>> +
>> endmenu
>> config SIFIVE_PLIC
>> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
>> index 606a003a0000..717f1d49e549 100644
>> --- a/drivers/irqchip/Makefile
>> +++ b/drivers/irqchip/Makefile
>> @@ -100,3 +100,4 @@ obj-$(CONFIG_MADERA_IRQ) += irq-madera.o
>> obj-$(CONFIG_LS1X_IRQ) += irq-ls1x.o
>> obj-$(CONFIG_TI_SCI_INTR_IRQCHIP) += irq-ti-sci-intr.o
>> obj-$(CONFIG_TI_SCI_INTA_IRQCHIP) += irq-ti-sci-inta.o
>> +obj-$(CONFIG_TI_PRUSS_INTC) += irq-pruss-intc.o
>> diff --git a/drivers/irqchip/irq-pruss-intc.c
>> b/drivers/irqchip/irq-pruss-intc.c
>> new file mode 100644
>> index 000000000000..d62186ad1be4
>> --- /dev/null
>> +++ b/drivers/irqchip/irq-pruss-intc.c
>> @@ -0,0 +1,352 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * PRU-ICSS INTC IRQChip driver for various TI SoCs
>> + *
>> + * Copyright (C) 2016-2019 Texas Instruments Incorporated -
>> http://www.ti.com/
>> + * Andrew F. Davis <afd@ti.com>
>> + * Suman Anna <s-anna@ti.com>
>> + */
>> +
>> +#include <linux/irq.h>
>> +#include <linux/irqchip/chained_irq.h>
>> +#include <linux/irqdomain.h>
>> +#include <linux/module.h>
>> +#include <linux/of_device.h>
>> +#include <linux/platform_device.h>
>> +
>> +/*
>> + * Number of host interrupts reaching the main MPU sub-system. Note
>> that this
>> + * is not the same as the total number of host interrupts supported
>> by the PRUSS
>> + * INTC instance
>> + */
>> +#define MAX_NUM_HOST_IRQS 8
>> +
>> +/* minimum starting host interrupt number for MPU */
>> +#define MIN_PRU_HOST_INT 2
>> +
>> +/* maximum number of system events */
>> +#define MAX_PRU_SYS_EVENTS 64
>> +
>> +/* PRU_ICSS_INTC registers */
>> +#define PRU_INTC_REVID 0x0000
>> +#define PRU_INTC_CR 0x0004
>> +#define PRU_INTC_GER 0x0010
>> +#define PRU_INTC_GNLR 0x001c
>> +#define PRU_INTC_SISR 0x0020
>> +#define PRU_INTC_SICR 0x0024
>> +#define PRU_INTC_EISR 0x0028
>> +#define PRU_INTC_EICR 0x002c
>> +#define PRU_INTC_HIEISR 0x0034
>> +#define PRU_INTC_HIDISR 0x0038
>> +#define PRU_INTC_GPIR 0x0080
>> +#define PRU_INTC_SRSR0 0x0200
>> +#define PRU_INTC_SRSR1 0x0204
>> +#define PRU_INTC_SECR0 0x0280
>> +#define PRU_INTC_SECR1 0x0284
>> +#define PRU_INTC_ESR0 0x0300
>> +#define PRU_INTC_ESR1 0x0304
>> +#define PRU_INTC_ECR0 0x0380
>> +#define PRU_INTC_ECR1 0x0384
>> +#define PRU_INTC_CMR(x) (0x0400 + (x) * 4)
>> +#define PRU_INTC_HMR(x) (0x0800 + (x) * 4)
>> +#define PRU_INTC_HIPIR(x) (0x0900 + (x) * 4)
>> +#define PRU_INTC_SIPR0 0x0d00
>> +#define PRU_INTC_SIPR1 0x0d04
>> +#define PRU_INTC_SITR0 0x0d80
>> +#define PRU_INTC_SITR1 0x0d84
>> +#define PRU_INTC_HINLR(x) (0x1100 + (x) * 4)
>> +#define PRU_INTC_HIER 0x1500
>> +
>> +/* HIPIR register bit-fields */
>> +#define INTC_HIPIR_NONE_HINT 0x80000000
>
> Unused macro. See below.
Will use the macro in the code.
>
>> +
>> +/**
>> + * struct pruss_intc - PRUSS interrupt controller structure
>> + * @irqs: kernel irq numbers corresponding to PRUSS host interrupts
>> + * @base: base virtual address of INTC register space
>> + * @irqchip: irq chip for this interrupt controller
>> + * @domain: irq domain for this interrupt controller
>> + * @lock: mutex to serialize access to INTC
>> + * @host_mask: indicate which HOST IRQs are enabled
>> + */
>> +struct pruss_intc {
>> + unsigned int irqs[MAX_NUM_HOST_IRQS];
>> + void __iomem *base;
>> + struct irq_chip *irqchip;
>> + struct irq_domain *domain;
>> + struct mutex lock; /* PRUSS INTC lock */
>> + u32 host_mask;
This is also unused in this patch, will move this to patch 4.
>> +};
>> +
>> +static inline u32 pruss_intc_read_reg(struct pruss_intc *intc,
>> unsigned int reg)
>> +{
>> + return readl_relaxed(intc->base + reg);
>> +}
>> +
>> +static inline void pruss_intc_write_reg(struct pruss_intc *intc,
>> + unsigned int reg, u32 val)
>> +{
>> + writel_relaxed(val, intc->base + reg);
>> +}
>> +
>> +static int pruss_intc_check_write(struct pruss_intc *intc, unsigned
>> int reg,
>> + unsigned int sysevent)
>> +{
>> + if (!intc)
>> + return -EINVAL;
>> +
>> + if (sysevent >= MAX_PRU_SYS_EVENTS)
>> + return -EINVAL;
>> +
>> + pruss_intc_write_reg(intc, reg, sysevent);
>> +
>> + return 0;
>> +}
>> +
>> +static void pruss_intc_init(struct pruss_intc *intc)
>> +{
>> + int i;
>> +
>> + /* configure polarity to active high for all system interrupts */
>> + pruss_intc_write_reg(intc, PRU_INTC_SIPR0, 0xffffffff);
>> + pruss_intc_write_reg(intc, PRU_INTC_SIPR1, 0xffffffff);
>> +
>> + /* configure type to pulse interrupt for all system interrupts */
>> + pruss_intc_write_reg(intc, PRU_INTC_SITR0, 0);
>> + pruss_intc_write_reg(intc, PRU_INTC_SITR1, 0);
>> +
>> + /* clear all 16 interrupt channel map registers */
>> + for (i = 0; i < 16; i++)
>> + pruss_intc_write_reg(intc, PRU_INTC_CMR(i), 0);
>> +
>> + /* clear all 3 host interrupt map registers */
>> + for (i = 0; i < 3; i++)
>> + pruss_intc_write_reg(intc, PRU_INTC_HMR(i), 0);
>> +}
>> +
>> +static void pruss_intc_irq_ack(struct irq_data *data)
>> +{
>> + struct pruss_intc *intc = irq_data_get_irq_chip_data(data);
>> + unsigned int hwirq = data->hwirq;
>> +
>> + pruss_intc_check_write(intc, PRU_INTC_SICR, hwirq);
>> +}
>> +
>> +static void pruss_intc_irq_mask(struct irq_data *data)
>> +{
>> + struct pruss_intc *intc = irq_data_get_irq_chip_data(data);
>> + unsigned int hwirq = data->hwirq;
>> +
>> + pruss_intc_check_write(intc, PRU_INTC_EICR, hwirq);
>> +}
>> +
>> +static void pruss_intc_irq_unmask(struct irq_data *data)
>> +{
>> + struct pruss_intc *intc = irq_data_get_irq_chip_data(data);
>> + unsigned int hwirq = data->hwirq;
>> +
>> + pruss_intc_check_write(intc, PRU_INTC_EISR, hwirq);
>> +}
>> +
>> +static int pruss_intc_irq_retrigger(struct irq_data *data)
>> +{
>> + struct pruss_intc *intc = irq_data_get_irq_chip_data(data);
>> + unsigned int hwirq = data->hwirq;
>> +
>> + return pruss_intc_check_write(intc, PRU_INTC_SISR, hwirq);
>> +}
>> +
>> +static int pruss_intc_irq_reqres(struct irq_data *data)
>> +{
>> + if (!try_module_get(THIS_MODULE))
>> + return -ENODEV;
>> +
>> + return 0;
>> +}
>> +
>> +static void pruss_intc_irq_relres(struct irq_data *data)
>> +{
>> + module_put(THIS_MODULE);
>> +}
>> +
>> +static int pruss_intc_irq_domain_map(struct irq_domain *d, unsigned
>> int virq,
>> + irq_hw_number_t hw)
>> +{
>> + struct pruss_intc *intc = d->host_data;
>> +
>> + irq_set_chip_data(virq, intc);
>> + irq_set_chip_and_handler(virq, intc->irqchip, handle_level_irq);
>> +
>> + return 0;
>> +}
>> +
>> +static void pruss_intc_irq_domain_unmap(struct irq_domain *d,
>> unsigned int virq)
>> +{
>> + irq_set_chip_and_handler(virq, NULL, NULL);
>> + irq_set_chip_data(virq, NULL);
>> +}
>> +
>> +static const struct irq_domain_ops pruss_intc_irq_domain_ops = {
>> + .xlate = irq_domain_xlate_onecell,
>> + .map = pruss_intc_irq_domain_map,
>> + .unmap = pruss_intc_irq_domain_unmap,
>> +};
>> +
>> +static void pruss_intc_irq_handler(struct irq_desc *desc)
>> +{
>> + unsigned int irq = irq_desc_get_irq(desc);
>> + struct irq_chip *chip = irq_desc_get_chip(desc);
>> + struct pruss_intc *intc = irq_get_handler_data(irq);
>> + u32 hipir;
>> + unsigned int virq;
>> + int i, hwirq;
>> +
>> + chained_irq_enter(chip, desc);
>> +
>> + /* find our host irq number */
>> + for (i = 0; i < MAX_NUM_HOST_IRQS; i++)
>> + if (intc->irqs[i] == irq)
>> + break;
>> + if (i == MAX_NUM_HOST_IRQS)
>> + goto err;
>> +
>> + i += MIN_PRU_HOST_INT;
>> +
>> + /* get highest priority pending PRUSS system event */
>> + hipir = pruss_intc_read_reg(intc, PRU_INTC_HIPIR(i));
>> + while (!(hipir & BIT(31))) {
>
> Is BIT(31) here supposed to be INTC_HIPIR_NONE_HINT?
Yes, will replace BIT(31) with the macro.
>
>> + hwirq = hipir & GENMASK(9, 0);
>> + virq = irq_linear_revmap(intc->domain, hwirq);
>> +
>> + /*
>> + * NOTE: manually ACK any system events that do not have a
>> + * handler mapped yet
>> + */
>> + if (unlikely(!virq))
>> + pruss_intc_check_write(intc, PRU_INTC_SICR, hwirq);
>> + else
>> + generic_handle_irq(virq);
>> +
>> + /* get next system event */
>> + hipir = pruss_intc_read_reg(intc, PRU_INTC_HIPIR(i));
>> + }
>> +err:
>> + chained_irq_exit(chip, desc);
>> +}
>> +
>> +static int pruss_intc_probe(struct platform_device *pdev)
>> +{
>> + static const char * const irq_names[] = {
>> + "host0", "host1", "host2", "host3",
>> + "host4", "host5", "host6", "host7", };
>> + struct device *dev = &pdev->dev;
>> + struct pruss_intc *intc;
>> + struct resource *res;
>> + struct irq_chip *irqchip;
>> + int i, irq;
>> +
>> + intc = devm_kzalloc(dev, sizeof(*intc), GFP_KERNEL);
>> + if (!intc)
>> + return -ENOMEM;
>> + platform_set_drvdata(pdev, intc);
>> +
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + intc->base = devm_ioremap_resource(dev, res);
>> + if (IS_ERR(intc->base)) {
>> + dev_err(dev, "failed to parse and map intc memory resource\n");
>> + return PTR_ERR(intc->base);
>> + }
>> +
>> + dev_dbg(dev, "intc memory: pa %pa size 0x%zx va %pK\n", &res->start,
>> + (size_t)resource_size(res), intc->base);
>> +
>> + mutex_init(&intc->lock);
>> +
>> + pruss_intc_init(intc);
>> +
>> + irqchip = devm_kzalloc(dev, sizeof(*irqchip), GFP_KERNEL);
>> + if (!irqchip)
>> + return -ENOMEM;
>> +
>> + irqchip->irq_ack = pruss_intc_irq_ack;
>> + irqchip->irq_mask = pruss_intc_irq_mask;
>> + irqchip->irq_unmask = pruss_intc_irq_unmask;
>> + irqchip->irq_retrigger = pruss_intc_irq_retrigger;
>> + irqchip->irq_request_resources = pruss_intc_irq_reqres;
>> + irqchip->irq_release_resources = pruss_intc_irq_relres;
>> + irqchip->name = dev_name(dev);
>
> Should we also set `irqchip->parent_device = dev;` here?
>
> I tried it and had to add pm runtime stuff as well, otherwise
> requesting irqs would fail.
I haven't seen any during my local testing. What sort of failure are you
seeing?
The clocking for the overall PRUSS module will be handled in either the
ti-sysc driver for OMAP SoCs or in the pruss platform driver.
>
>> + intc->irqchip = irqchip;
>> +
>> + /* always 64 events */
>> + intc->domain = irq_domain_add_linear(dev->of_node,
>> MAX_PRU_SYS_EVENTS,
>> + &pruss_intc_irq_domain_ops, intc);
>> + if (!intc->domain)
>> + return -ENOMEM;
>> +
>> + for (i = 0; i < MAX_NUM_HOST_IRQS; i++) {
>> + irq = platform_get_irq_byname(pdev, irq_names[i]);
>> + if (irq < 0) {
>> + dev_err(dev->parent, "platform_get_irq_byname failed for
>> %s : %d\n",
>
> Why dev->parent instead of just dev?
Yeah, will fix this. It is a vestige from previous code where we were
actually parsing the interrupts from the PRUSS node instead of the PRUSS
INTC node.
>
>> + irq_names[i], irq);
>> + goto fail_irq;
>> + }
>> +
>> + intc->irqs[i] = irq;
>> + irq_set_handler_data(irq, intc);
>> + irq_set_chained_handler(irq, pruss_intc_irq_handler);
>> + }
>> +
>> + return 0;
>> +
>> +fail_irq:
>> + while (--i >= 0) {
>> + if (intc->irqs[i])
>> + irq_set_chained_handler_and_data(intc->irqs[i], NULL,
>> + NULL);
>> + }
>> + irq_domain_remove(intc->domain);
>> + return irq;
>> +}
>> +
>> +static int pruss_intc_remove(struct platform_device *pdev)
>> +{
>> + struct pruss_intc *intc = platform_get_drvdata(pdev);
>> + unsigned int hwirq;
>> + int i;
>> +
>> + for (i = 0; i < MAX_NUM_HOST_IRQS; i++) {
>> + if (intc->irqs[i])
>> + irq_set_chained_handler_and_data(intc->irqs[i], NULL,
>> + NULL);
>> + }
>> +
>> + if (intc->domain) {
>
> Is it actuall possible for intc->domain to be NULL here?
Nope, will remove it.
Thank you for all the review comments.
regards
Suman
>
>> + for (hwirq = 0; hwirq < MAX_PRU_SYS_EVENTS; hwirq++)
>> + irq_dispose_mapping(irq_find_mapping(intc->domain,
>> + hwirq));
>> + irq_domain_remove(intc->domain);
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static const struct of_device_id pruss_intc_of_match[] = {
>> + { .compatible = "ti,pruss-intc", },
>> + { /* sentinel */ },
>> +};
>> +MODULE_DEVICE_TABLE(of, pruss_intc_of_match);
>> +
>> +static struct platform_driver pruss_intc_driver = {
>> + .driver = {
>> + .name = "pruss-intc",
>> + .of_match_table = pruss_intc_of_match,
>> + },
>> + .probe = pruss_intc_probe,
>> + .remove = pruss_intc_remove,
>> +};
>> +module_platform_driver(pruss_intc_driver);
>> +
>> +MODULE_AUTHOR("Andrew F. Davis <afd@ti.com>");
>> +MODULE_AUTHOR("Suman Anna <s-anna@ti.com>");
>> +MODULE_DESCRIPTION("TI PRU-ICSS INTC Driver");
>> +MODULE_LICENSE("GPL v2");
>>
>
^ permalink raw reply
* Re: [PATCH 1/6] dt-bindings: irqchip: Add PRUSS interrupt controller bindings
From: Suman Anna @ 2019-07-16 17:07 UTC (permalink / raw)
To: David Lechner, Andrew F. Davis, Marc Zyngier, Rob Herring,
Thomas Gleixner, Jason Cooper
Cc: devicetree, Grygorii Strashko, Tony Lindgren, Sekhar Nori,
linux-kernel, Lokesh Vutla, Murali Karicheri, linux-omap,
linux-arm-kernel, Roger Quadros
In-Reply-To: <c3f0061f-22b0-c281-b8ff-4a2cb7e91aba@lechnology.com>
Hi David,
On 7/10/19 12:08 PM, David Lechner wrote:
>
>>>> +- interrupts : all the interrupts generated towards the
>>>> main host
>>>> + processor in the SoC. The format depends
>>>> on the
>>>> + interrupt specifier for the particular
>>>> SoC's ARM GIC
>>>> + parent interrupt controller. A shared
>>>> interrupt can
>>>> + be skipped if the desired destination and
>>>> usage is by
>>>> + a different processor/device.
>>>> +- interrupt-names : should use one of the following names for
>>>> each valid
>>>> + interrupt connected to ARM GIC, the name
>>>> should match
>>>> + the corresponding host interrupt number,
>>>> + "host0", "host1", "host2", "host3",
>>>> "host4",
>>>> + "host5", "host6" or "host7"
>>>> +- interrupt-controller : mark this node as an interrupt controller
>>>> +- #interrupt-cells : should be 1. Client users shall use the
>>>> PRU System
>>>> + event number (the interrupt source that
>>>> the client
>>>> + is interested in) as the value of the
>>>> interrupts
>>>> + property in their node
>>>> +
>>>> +Optional Properties:
>>>> +--------------------
>>>> +The following properties are _required_ only for some SoCs. If none
>>>> of the below
>>>> +properties are defined, it implies that all the host interrupts 2
>>>> through 9 are
>>>> +connected exclusively to the ARM GIC.
>>>> +
>>>> +- ti,irqs-reserved : an array of 8-bit elements of host
>>>> interrupts between
>>>> + 0 and 7 (corresponding to PRUSS INTC
>>>> output interrupts
>>>> + 2 through 9) that are not connected to the
>>>> ARM GIC.
>>>
>>> The reason for 0-7 mapping to 2-9 is not instantly clear to someone
>>> reading this. If you respin this could you note that reason is
>>> interrupts 0 and 1 are always routed back into the PRUSS.
>>
>> Yeah, this is always going to be somewhat confusing since the driver has
>> to deal with all hosts from channel-mapping perspective, but only the 8
>> interrupts at most that reach MPU for handling interrupts. TRM has
>>
>> Anyway, I have already mentioned the first 2 interrupt routing in the
>> first paragraph above.
>>
>> Thinking more
>>> on that, the same is true for interrupt 7 ("host5") on AM437x/66AK2G yet
>>> we don't skip that in the naming.. now that we have the reserved IRQ
>>> mechanism above, why not leave the one-to-one interrupt to name mapping,
>>> but always have at least the first two marked as reserved for all the
>>> current devices:
>>>
>>> ti,irqs-reserved = /bits/ 8 <0 1>;
>>>
>>> Then any "hostx" listed as reserved need not be present in the host
>>> interrupts property array. To me that would solve the "managing
>>> interrupts not targeting the Linux running core" problem and keep the
>>> names consistent, e.g.:
>>
>> I had actually used the interrupt-names always starting from "host2"
>> through "host9" (names from PRU perspective) previously, and I have
>> changed this to start indexing from 0 in this series to address an
>> internal review comment from Grygorii and to align with TRM. All the
>> TRMs (except for AM572x) actually use the names/signals "host_intr0",
>> "host_intr1".."host_intr7" etc for the interrupts going towards MPU.
>> Maybe I should actually rename the interrupt-names to be host_intrX
>> instead of hostX to avoid confusion and be exactly aligned with the TRM
>> names. I will file a bug against AM57xx TRM to align the names with all
>> other SoC TRMs.
>>
>> I am using "output interrupt lines" to imply names w.r.t PRU vs "host
>> interrupt" to imply ARM GIC names.
>>
>> regards
>> Suman
>>
>
> FWIW, the AM1808 TRM only uses PRU_EVTOUT0 to PRU_EVTOUT7 and does not
> mention "host" in relation to these interrupts. The AM3xxx and AM4xxx
> also use similar names (PRU_ICSS_EVTOUT0, PRU_ICSS1_EVTOUT0) although
> they do mention that the source is "pr1_host[0] output/events exported
> from PRU_ICSS1". (Also, the older processors have AINTC instead of GIC).
Indeed, EVTOUT was only used in the Interrupts chapter on AM1808, AM335x
and AM437x. These TRMs are only getting very infrequent updates, so I
doubt we will have these names synchronized to the other SoCs.
The descriptions in PRUSS INTC sections themselves always use the term
host interrupts for all host events, but the output signals get
re-indexed to 0, which tends to be confusing.
>
> Maybe to help clarify here we could mention "event" in the docs:
>
>
> +- interrupt-names : should use one of the following names for each
> valid
> + host event interrupt connected to ARM interrupt
> + controller,the name should match the
> corresponding
> + host event interrupt number,
Yeah, I like your rewording. Will update for the next version.
> + "host0", "host1", "host2", "host3", "host4",
> + "host5", "host6" or "host7"
I will be updating these names as well to add either a int or intr suffix.
>
>
>
> ...
>
>>>> +
>>>> +Example:
>>>> +--------
>>>> +
>>>> +1. /* AM33xx PRU-ICSS */
>>>> + pruss: pruss@0 {
>
> I don't suppose there is a generic name that could be used here
> instead of pruss? It seems like there should be one for remote
> processors that aren't DSPs or other specialized processors.
>
Yeah, there is none. It is the overall PRU subsystem, the individual
cores are called PRU. The subsystems are usually referred to as PRUSS,
PRU-ICSS and ICSSG (on the newer K3 SoCs), so I simply chose the shorter
pruss.
regards
Suman
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ 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