* Re: [PATCH v4 11/24] PM / devfreq: tegra30: Add debug messages
From: Dmitry Osipenko @ 2019-07-16 13:26 UTC (permalink / raw)
To: Chanwoo Choi, Thierry Reding, MyungJoo Ham, Kyungmin Park,
Jonathan Hunter, Tomeu Vizoso
Cc: linux-pm, linux-tegra, linux-kernel
In-Reply-To: <c883bdbe-427f-35a1-9e63-5e4953a84286@samsung.com>
16.07.2019 15:23, Chanwoo Choi пишет:
> Hi Dmitry,
>
> Usually, the kernel log print for all users
> such as changing the frequency, fail or success.
>
> But, if the log just show the register dump,
> it is not useful for all users. It is just used
> for only specific developer.
>
> I recommend that you better to add more exception handling
> code on many points instead of just showing the register dump.
The debug messages are not users, but for developers. Yes, I primarily
made the debugging to be useful for myself and will be happy to change
the way debugging is done if there will be any other active developer
for this driver. The registers dump is more than enough in order to
understand what's going on, I don't see any real need to change anything
here for now.
^ permalink raw reply
* Re: [PATCH v4 09/24] PM / devfreq: tegra30: Reset boosting on startup
From: Dmitry Osipenko @ 2019-07-16 13:19 UTC (permalink / raw)
To: Chanwoo Choi, Thierry Reding, MyungJoo Ham, Kyungmin Park,
Jonathan Hunter, Tomeu Vizoso
Cc: linux-pm, linux-tegra, linux-kernel
In-Reply-To: <caed4a78-ad8a-5bee-adc4-4a342e2898c3@samsung.com>
16.07.2019 15:13, Chanwoo Choi пишет:
> On 19. 7. 8. 오전 7:32, Dmitry Osipenko wrote:
>> Governor could be stopped while boosting is active. We have assumption
>> that everything is reset on governor's restart, including the boosting
>> value, which was missed.
>>
>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>> ---
>> drivers/devfreq/tegra30-devfreq.c | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
>> index d5d04c25023b..32fe95458ee7 100644
>> --- a/drivers/devfreq/tegra30-devfreq.c
>> +++ b/drivers/devfreq/tegra30-devfreq.c
>> @@ -536,6 +536,9 @@ static void tegra_actmon_configure_device(struct tegra_devfreq *tegra,
>> {
>> u32 val = 0, target_freq;
>>
>> + /* we don't want boosting on restart */
>
> nitpick.
> I think that following comment is proper in my case.
> In my case, I think 'we' expression is not good
>
> /* Reset the boost frequency on restart */
Okay, I'll change the wording.
>> + dev->boost_freq = 0;
>> +
>> target_freq = clk_get_rate(tegra->emc_clock) / KHZ;
>> dev->avg_count = target_freq * ACTMON_SAMPLING_PERIOD;
>> device_writel(dev, dev->avg_count, ACTMON_DEV_INIT_AVG);
>>
>
> Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
>
>
^ permalink raw reply
* Re: [PATCH v4 07/24] PM / devfreq: tegra30: Use CPUFreq notifier
From: Dmitry Osipenko @ 2019-07-16 13:18 UTC (permalink / raw)
To: Chanwoo Choi, Thierry Reding, MyungJoo Ham, Kyungmin Park,
Jonathan Hunter, Tomeu Vizoso
Cc: linux-pm, linux-tegra, linux-kernel
In-Reply-To: <c94da786-05b4-1604-d208-f73f32ea937d@samsung.com>
16.07.2019 15:08, Chanwoo Choi пишет:
> On 19. 7. 8. 오전 7:32, Dmitry Osipenko wrote:
>> The CPU's client need to take into account that CPUFreq may change
>> while memory activity not, staying high. Thus an appropriate frequency
>> notifier should be used in addition to the clk-notifier.
>>
>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>> ---
>> drivers/devfreq/tegra30-devfreq.c | 105 +++++++++++++++++++++++++-----
>> 1 file changed, 88 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
>> index 2bf65409ddd8..48a799fa5f63 100644
>> --- a/drivers/devfreq/tegra30-devfreq.c
>> +++ b/drivers/devfreq/tegra30-devfreq.c
>> @@ -17,6 +17,7 @@
>> #include <linux/platform_device.h>
>> #include <linux/pm_opp.h>
>> #include <linux/reset.h>
>> +#include <linux/workqueue.h>
>>
>> #include "governor.h"
>>
>> @@ -154,7 +155,10 @@ struct tegra_devfreq {
>>
>> struct clk *emc_clock;
>> unsigned long max_freq;
>> - struct notifier_block rate_change_nb;
>> + struct notifier_block clk_rate_change_nb;
>> +
>> + struct work_struct update_work;
>
> nitpick.
> I think 'update_work' is not clear to indicate the work
> for changing the clock according to the cpu clock change.
> You better to change the name for more clearly.
Okay, I'll rename it to 'cpufreq_update_work' or something like that.
>> + struct notifier_block cpu_rate_change_nb;
>>
>> struct tegra_devfreq_device devices[ARRAY_SIZE(actmon_device_configs)];
>>
>> @@ -456,8 +460,8 @@ static irqreturn_t actmon_thread_isr(int irq, void *data)
>> return handled ? IRQ_HANDLED : IRQ_NONE;
>> }
>>
>> -static int tegra_actmon_rate_notify_cb(struct notifier_block *nb,
>> - unsigned long action, void *ptr)
>> +static int tegra_actmon_clk_notify_cb(struct notifier_block *nb,
>> + unsigned long action, void *ptr)
>> {
>> struct clk_notifier_data *data = ptr;
>> struct tegra_devfreq_device *dev;
>> @@ -467,7 +471,7 @@ static int tegra_actmon_rate_notify_cb(struct notifier_block *nb,
>> if (action != POST_RATE_CHANGE)
>> return NOTIFY_OK;
>>
>> - tegra = container_of(nb, struct tegra_devfreq, rate_change_nb);
>> + tegra = container_of(nb, struct tegra_devfreq, clk_rate_change_nb);
>>
>> /*
>> * EMC rate could change due to three reasons:
>> @@ -496,6 +500,37 @@ static int tegra_actmon_rate_notify_cb(struct notifier_block *nb,
>> return NOTIFY_OK;
>> }
>>
>> +static void tegra_actmon_delayed_update(struct work_struct *work)
>> +{
>> + struct tegra_devfreq *tegra = container_of(work, struct tegra_devfreq,
>> + update_work);
>> +
>> + mutex_lock(&tegra->devfreq->lock);
>> + update_devfreq(tegra->devfreq);
>> + mutex_unlock(&tegra->devfreq->lock);
>> +}
>> +
>> +static int tegra_actmon_cpu_notify_cb(struct notifier_block *nb,
>> + unsigned long action, void *ptr)
>> +{
>> + struct tegra_devfreq *tegra;
>> +
>> + if (action != CPUFREQ_POSTCHANGE)
>> + return NOTIFY_OK;
>> +
>> + tegra = container_of(nb, struct tegra_devfreq, cpu_rate_change_nb);
>
> nitpick. Better to check whether 'tegra' is NULL or not.
It can't be NULL, unless 'nb' is NULL and even in that case
container_of() will cause kernel crash. Hence it's absolutely
unnecessary to check for NULL here.
>> +
>> + /*
>> + * CPUFreq driver should support CPUFREQ_ASYNC_NOTIFICATION in order
>> + * to allow asynchronous notifications. This means we can't block
>> + * here for too long, otherwise CPUFreq's core will complain with a
>> + * warning splat.
>> + */
>> + schedule_work(&tegra->update_work);
>> +
>> + return NOTIFY_OK;
>> +}
>> +
>> static void tegra_actmon_configure_device(struct tegra_devfreq *tegra,
>> struct tegra_devfreq_device *dev)
>> {
>> @@ -527,9 +562,16 @@ static void tegra_actmon_configure_device(struct tegra_devfreq *tegra,
>> device_writel(dev, val, ACTMON_DEV_CTRL);
>> }
>>
>> -static void tegra_actmon_start(struct tegra_devfreq *tegra)
>> +static void tegra_actmon_stop_device(struct tegra_devfreq_device *dev)
>> +{
>> + device_writel(dev, 0x00000000, ACTMON_DEV_CTRL);
>
> Better to define the constant definition of 0x00000000
> in order to explain the correct meaning.
>
> For example,
> #define ACTMON_DEV_RESET 0x00000000
Okay.
^ permalink raw reply
* Re: [PATCH v4 03/24] PM / devfreq: tegra30: Handle possible round-rate error
From: Dmitry Osipenko @ 2019-07-16 13:09 UTC (permalink / raw)
To: Chanwoo Choi, Thierry Reding, MyungJoo Ham, Kyungmin Park,
Jonathan Hunter, Tomeu Vizoso
Cc: linux-pm, linux-tegra, linux-kernel
In-Reply-To: <f28470ca-93dc-cdf9-b008-54c7b50cfd83@samsung.com>
16.07.2019 14:50, Chanwoo Choi пишет:
> On 19. 7. 8. 오전 7:32, Dmitry Osipenko wrote:
>> The EMC clock rate rounding technically could fail, hence let's handle
>> the error cases properly.
>>
>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>> ---
>> drivers/devfreq/tegra30-devfreq.c | 17 +++++++++++++++--
>> 1 file changed, 15 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
>> index 5e2b133babdd..5e606ae3f238 100644
>> --- a/drivers/devfreq/tegra30-devfreq.c
>> +++ b/drivers/devfreq/tegra30-devfreq.c
>> @@ -592,8 +592,8 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
>> struct tegra_devfreq_device *dev;
>> struct tegra_devfreq *tegra;
>> struct devfreq *devfreq;
>> - unsigned long rate;
>> unsigned int i;
>> + long rate;
>> int err;
>>
>> tegra = devm_kzalloc(&pdev->dev, sizeof(*tegra), GFP_KERNEL);
>> @@ -650,8 +650,14 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
>>
>> reset_control_deassert(tegra->reset);
>>
>> - tegra->max_freq = clk_round_rate(tegra->emc_clock, ULONG_MAX) / KHZ;
>> + rate = clk_round_rate(tegra->emc_clock, ULONG_MAX);
>> + if (rate < 0) {
>> + dev_err(&pdev->dev, "Failed to round clock rate: %ld\n", rate);
>> + return rate;
>> + }
>> +
>> tegra->cur_freq = clk_get_rate(tegra->emc_clock) / KHZ;
>> + tegra->max_freq = rate / KHZ;
>>
>> for (i = 0; i < ARRAY_SIZE(actmon_device_configs); i++) {
>> dev = tegra->devices + i;
>> @@ -662,6 +668,13 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
>> for (rate = 0; rate <= tegra->max_freq * KHZ; rate++) {
>> rate = clk_round_rate(tegra->emc_clock, rate);
>>
>
> Please remove unneeded blank line.
I can remove it, but it was added specifically to ease reading of the code.
>> + if (rate < 0) {
>> + dev_err(&pdev->dev,
>> + "Failed to round clock rate: %ld\n", rate);
>> + err = rate;
>> + goto remove_opps;
>> + }
>
> Also, this patch doesn't contain code which restore the previous
> tegra->cur_freq/max_freq when error happen.
The error here results in abortion of the driver's probing, hence
nothing need to be restored in that case because nothing was changed at
this point yet.
^ permalink raw reply
* Re: [PATCH v4 02/24] PM / devfreq: tegra30: Keep interrupt disabled while governor is stopped
From: Dmitry Osipenko @ 2019-07-16 13:03 UTC (permalink / raw)
To: Chanwoo Choi, Thierry Reding, MyungJoo Ham, Kyungmin Park,
Jonathan Hunter, Tomeu Vizoso
Cc: linux-pm, linux-tegra, linux-kernel
In-Reply-To: <f691a845-18f3-a6fb-302c-a8a3fc13e5bf@samsung.com>
16.07.2019 14:47, Chanwoo Choi пишет:
> On 19. 7. 8. 오전 7:32, Dmitry Osipenko wrote:
>> There is no real need to keep interrupt always-enabled, will be nicer
>> to keep it disabled while governor is inactive.
>>
>> Suggested-by: Thierry Reding <thierry.reding@gmail.com>
>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>> ---
>> drivers/devfreq/tegra30-devfreq.c | 43 ++++++++++++++++---------------
>> 1 file changed, 22 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
>> index a27300f40b0b..5e2b133babdd 100644
>> --- a/drivers/devfreq/tegra30-devfreq.c
>> +++ b/drivers/devfreq/tegra30-devfreq.c
>> @@ -11,6 +11,7 @@
>> #include <linux/devfreq.h>
>> #include <linux/interrupt.h>
>> #include <linux/io.h>
>> +#include <linux/irq.h>
>> #include <linux/module.h>
>> #include <linux/mod_devicetable.h>
>> #include <linux/platform_device.h>
>> @@ -416,8 +417,6 @@ static void tegra_actmon_start(struct tegra_devfreq *tegra)
>> {
>> unsigned int i;
>>
>> - disable_irq(tegra->irq);
>> -
>> actmon_writel(tegra, ACTMON_SAMPLING_PERIOD - 1,
>> ACTMON_GLB_PERIOD_CTRL);
>>
>> @@ -442,8 +441,6 @@ static void tegra_actmon_stop(struct tegra_devfreq *tegra)
>> }
>>
>> actmon_write_barrier(tegra);
>> -
>> - enable_irq(tegra->irq);
>> }
>>
>> static int tegra_devfreq_target(struct device *dev, unsigned long *freq,
>> @@ -552,6 +549,12 @@ static int tegra_governor_event_handler(struct devfreq *devfreq,
>> {
>> struct tegra_devfreq *tegra = dev_get_drvdata(devfreq->dev.parent);
>>
>> + /*
>> + * Couple device with the governor early as it is needed at
>> + * the moment of governor's start (used by ISR).
>> + */
>> + tegra->devfreq = devfreq;
>
> I'm not sure it is necessary. Almost devfreq device get
> the devfreq instance on probe timing through devfreq_add_device directly.
This is necessary because this assignment is for the "governor" and not
the "device". Governor is started during of devfreq_add_device(), hence
there is no better way to assign device to the driver's governor.
>> +
>> switch (event) {
>> case DEVFREQ_GOV_START:
>> devfreq_monitor_start(devfreq);
>> @@ -586,10 +589,11 @@ static struct devfreq_governor tegra_devfreq_governor = {
>>
>> static int tegra_devfreq_probe(struct platform_device *pdev)
>> {
>> - struct tegra_devfreq *tegra;
>> struct tegra_devfreq_device *dev;
>> - unsigned int i;
>> + struct tegra_devfreq *tegra;
>> + struct devfreq *devfreq;
>> unsigned long rate;
>> + unsigned int i;
>> int err;
>>
>> tegra = devm_kzalloc(&pdev->dev, sizeof(*tegra), GFP_KERNEL);
>> @@ -625,6 +629,16 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
>> }
>> tegra->irq = err;
>>
>> + irq_set_status_flags(tegra->irq, IRQ_NOAUTOEN);
>> +
>> + err = devm_request_threaded_irq(&pdev->dev, tegra->irq, NULL,
>> + actmon_thread_isr, IRQF_ONESHOT,
>> + "tegra-devfreq", tegra);
>> + if (err) {
>> + dev_err(&pdev->dev, "Interrupt request failed: %d\n", err);
>> + return err;
>> + }
>> +
>> reset_control_assert(tegra->reset);
>>
>> err = clk_prepare_enable(tegra->clock);
>> @@ -672,28 +686,15 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
>> }
>>
>> tegra_devfreq_profile.initial_freq = clk_get_rate(tegra->emc_clock);
>> - tegra->devfreq = devfreq_add_device(&pdev->dev,
>> - &tegra_devfreq_profile,
>> - "tegra_actmon",
>> - NULL);
>> + devfreq = devfreq_add_device(&pdev->dev, &tegra_devfreq_profile,
>> + "tegra_actmon", NULL);
>> if (IS_ERR(tegra->devfreq)) {
>
> Have to check 'devfreq' instead of 'tegra->devfreq'.
> Did you test it? It might be failed because 'tegra->devfreq is NULL.
That's a good catch! Thank you very much.
>> err = PTR_ERR(tegra->devfreq);
>
> ditto.
Ok
^ permalink raw reply
* Re: [PATCH v4 15/24] PM / devfreq: tegra30: Fix integer overflow on CPU's freq max out
From: Chanwoo Choi @ 2019-07-16 12:32 UTC (permalink / raw)
To: Dmitry Osipenko, Thierry Reding, MyungJoo Ham, Kyungmin Park,
Jonathan Hunter, Tomeu Vizoso
Cc: linux-pm, linux-tegra, linux-kernel
In-Reply-To: <20190707223303.6755-16-digetx@gmail.com>
On 19. 7. 8. 오전 7:32, Dmitry Osipenko wrote:
> There is another kHz-conversion bug in the code, resulting in integer
> overflow. Although, this time the resulting value is 4294966296 and it's
> close to ULONG_MAX, which is okay in this case.
>
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
> drivers/devfreq/tegra30-devfreq.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
> index 0de1efdaabf4..60ebf9c9cd86 100644
> --- a/drivers/devfreq/tegra30-devfreq.c
> +++ b/drivers/devfreq/tegra30-devfreq.c
> @@ -172,7 +172,7 @@ struct tegra_actmon_emc_ratio {
> };
>
> static const struct tegra_actmon_emc_ratio actmon_emc_ratios[] = {
> - { 1400000, ULONG_MAX },
> + { 1400000, ULONG_MAX / KHZ },
> { 1200000, 750000 },
> { 1100000, 600000 },
> { 1000000, 500000 },
>
Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
--
Best Regards,
Chanwoo Choi
Samsung Electronics
^ permalink raw reply
* Re: [PATCH v4 14/24] PM / devfreq: tegra30: Ensure that target freq won't overflow
From: Chanwoo Choi @ 2019-07-16 12:30 UTC (permalink / raw)
To: Dmitry Osipenko, Thierry Reding, MyungJoo Ham, Kyungmin Park,
Jonathan Hunter, Tomeu Vizoso
Cc: linux-pm, linux-tegra, linux-kernel
In-Reply-To: <20190707223303.6755-15-digetx@gmail.com>
On 19. 7. 8. 오전 7:32, Dmitry Osipenko wrote:
> Potentially very high boosting could cause an integer overflow for a
> highly clocked memory after conversion to MHz.
>
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
> drivers/devfreq/tegra30-devfreq.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
> index 2f59c78930bd..0de1efdaabf4 100644
> --- a/drivers/devfreq/tegra30-devfreq.c
> +++ b/drivers/devfreq/tegra30-devfreq.c
> @@ -460,6 +460,7 @@ static unsigned long actmon_update_target(struct tegra_devfreq *tegra,
> unsigned long target_freq;
>
> target_freq = dev->avg_count / ACTMON_SAMPLING_PERIOD + dev->boost_freq;
> + target_freq = min(target_freq, ULONG_MAX / KHZ);
Did you meet this corner case?
If have to change it, you better to use 'tegra->max_freq' as following:
min(target_freq, tegra->max_freq);
> target_freq = tegra_actmon_account_cpu_freq(tegra, dev, target_freq);
>
> return target_freq;
>
--
Best Regards,
Chanwoo Choi
Samsung Electronics
^ permalink raw reply
* Re: [PATCH 03/10] video: pxafb: Remove cpufreq policy notifier
From: Bartlomiej Zolnierkiewicz @ 2019-07-16 12:26 UTC (permalink / raw)
To: Viresh Kumar
Cc: Rafael Wysocki, linux-pm, Vincent Guittot, dri-devel, linux-fbdev,
linux-kernel
In-Reply-To: <e69d47b1d497bdbd8c988754d98714e78ddc0a80.1563270828.git.viresh.kumar@linaro.org>
On 7/16/19 11:54 AM, Viresh Kumar wrote:
> The cpufreq policy notifier's CPUFREQ_ADJUST notification is going to
> get removed soon.
>
> The notifier callback pxafb_freq_policy() isn't doing anything apart
> from printing a debug message on CPUFREQ_ADJUST notification. There is
> no point in keeping an otherwise empty callback and registering the
> notifier.
>
> Remove it.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
Acked-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics
^ permalink raw reply
* Re: [PATCH 02/10] video: sa1100fb: Remove cpufreq policy notifier
From: Bartlomiej Zolnierkiewicz @ 2019-07-16 12:25 UTC (permalink / raw)
To: Viresh Kumar
Cc: Rafael Wysocki, linux-pm, Vincent Guittot, dri-devel, linux-fbdev,
linux-kernel
In-Reply-To: <7163e57cfa1780d42732fa6b5ec424c24d1d4dc8.1563270828.git.viresh.kumar@linaro.org>
Hi Viresh,
Please always Cc: me on fbdev patches.
On 7/16/19 11:54 AM, Viresh Kumar wrote:
> The cpufreq policy notifier's CPUFREQ_ADJUST notification is going to
> get removed soon.
>
> The notifier callback sa1100fb_freq_policy() isn't doing anything apart
> from printing a debug message on CPUFREQ_ADJUST notification. There is
> no point in keeping an otherwise empty callback and registering the
> notifier.
>
> Remove it.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
Acked-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics
^ permalink raw reply
* Re: [PATCH v4 13/24] PM / devfreq: tegra30: Constify structs
From: Chanwoo Choi @ 2019-07-16 12:26 UTC (permalink / raw)
To: Dmitry Osipenko, Thierry Reding, MyungJoo Ham, Kyungmin Park,
Jonathan Hunter, Tomeu Vizoso
Cc: linux-pm, linux-tegra, linux-kernel
In-Reply-To: <20190707223303.6755-14-digetx@gmail.com>
On 19. 7. 8. 오전 7:32, Dmitry Osipenko wrote:
> Constify unmodifiable structs for consistency.
>
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
> drivers/devfreq/tegra30-devfreq.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
> index 1a10df5dbbed..2f59c78930bd 100644
> --- a/drivers/devfreq/tegra30-devfreq.c
> +++ b/drivers/devfreq/tegra30-devfreq.c
> @@ -106,7 +106,7 @@ enum tegra_actmon_device {
> MCCPU,
> };
>
> -static struct tegra_devfreq_device_config actmon_device_configs[] = {
> +static const struct tegra_devfreq_device_config actmon_device_configs[] = {
> {
> /* MCALL: All memory accesses (including from the CPUs) */
> .offset = 0x1c0,
> @@ -171,7 +171,7 @@ struct tegra_actmon_emc_ratio {
> unsigned long emc_freq;
> };
>
> -static struct tegra_actmon_emc_ratio actmon_emc_ratios[] = {
> +static const struct tegra_actmon_emc_ratio actmon_emc_ratios[] = {
> { 1400000, ULONG_MAX },
> { 1200000, 750000 },
> { 1100000, 600000 },
> @@ -210,7 +210,7 @@ static inline unsigned long do_percent(unsigned long val, unsigned int pct)
>
> static unsigned long actmon_cpu_to_emc_rate(struct tegra_devfreq *tegra)
> {
> - struct tegra_actmon_emc_ratio *ratio = actmon_emc_ratios;
> + const struct tegra_actmon_emc_ratio *ratio = actmon_emc_ratios;
> unsigned int cpu_freq = cpufreq_get(0);
> unsigned int i;
>
>
Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
--
Best Regards,
Chanwoo Choi
Samsung Electronics
^ permalink raw reply
* Re: [PATCH v4 12/24] PM / devfreq: tegra30: Inline all one-line functions
From: Chanwoo Choi @ 2019-07-16 12:26 UTC (permalink / raw)
To: Dmitry Osipenko, Thierry Reding, MyungJoo Ham, Kyungmin Park,
Jonathan Hunter, Tomeu Vizoso
Cc: linux-pm, linux-tegra, linux-kernel
In-Reply-To: <20190707223303.6755-13-digetx@gmail.com>
Hi Dmitry,
I'm not sure that it is necessary.
As I knew, usally, the 'inline' is used on header file
to define the empty functions.
Do we have to change it with 'inline' keyword?
On 19. 7. 8. 오전 7:32, Dmitry Osipenko wrote:
> Depending on a kernel's configuration, a single line functions may not be
> inlined by compiler (like enabled ftracing for example). Let's inline such
> functions explicitly for consistency.
>
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
> drivers/devfreq/tegra30-devfreq.c | 13 +++++++------
> 1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
> index c6c4a07d3e07..1a10df5dbbed 100644
> --- a/drivers/devfreq/tegra30-devfreq.c
> +++ b/drivers/devfreq/tegra30-devfreq.c
> @@ -181,28 +181,29 @@ static struct tegra_actmon_emc_ratio actmon_emc_ratios[] = {
> { 250000, 100000 },
> };
>
> -static u32 actmon_readl(struct tegra_devfreq *tegra, u32 offset)
> +static inline u32 actmon_readl(struct tegra_devfreq *tegra, u32 offset)
> {
> return readl_relaxed(tegra->regs + offset);
> }
>
> -static void actmon_writel(struct tegra_devfreq *tegra, u32 val, u32 offset)
> +static inline void actmon_writel(struct tegra_devfreq *tegra,
> + u32 val, u32 offset)
> {
> writel_relaxed(val, tegra->regs + offset);
> }
>
> -static u32 device_readl(struct tegra_devfreq_device *dev, u32 offset)
> +static inline u32 device_readl(struct tegra_devfreq_device *dev, u32 offset)
> {
> return readl_relaxed(dev->regs + offset);
> }
>
> -static void device_writel(struct tegra_devfreq_device *dev, u32 val,
> - u32 offset)
> +static inline void device_writel(struct tegra_devfreq_device *dev,
> + u32 val, u32 offset)
> {
> writel_relaxed(val, dev->regs + offset);
> }
>
> -static unsigned long do_percent(unsigned long val, unsigned int pct)
> +static inline unsigned long do_percent(unsigned long val, unsigned int pct)
> {
> return val * pct / 100;
> }
>
--
Best Regards,
Chanwoo Choi
Samsung Electronics
^ permalink raw reply
* Re: [PATCH v4 11/24] PM / devfreq: tegra30: Add debug messages
From: Chanwoo Choi @ 2019-07-16 12:23 UTC (permalink / raw)
To: Dmitry Osipenko, Thierry Reding, MyungJoo Ham, Kyungmin Park,
Jonathan Hunter, Tomeu Vizoso
Cc: linux-pm, linux-tegra, linux-kernel
In-Reply-To: <20190707223303.6755-12-digetx@gmail.com>
Hi Dmitry,
Usually, the kernel log print for all users
such as changing the frequency, fail or success.
But, if the log just show the register dump,
it is not useful for all users. It is just used
for only specific developer.
I recommend that you better to add more exception handling
code on many points instead of just showing the register dump.
On 19. 7. 8. 오전 7:32, Dmitry Osipenko wrote:
> Add debug messages to know about what's happening in hardware and how
> driver reacts.
>
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
> drivers/devfreq/tegra30-devfreq.c | 35 +++++++++++++++++++++++++++++++
> 1 file changed, 35 insertions(+)
>
> diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
> index 878c9396bb8c..c6c4a07d3e07 100644
> --- a/drivers/devfreq/tegra30-devfreq.c
> +++ b/drivers/devfreq/tegra30-devfreq.c
> @@ -41,6 +41,7 @@
> #define ACTMON_DEV_AVG_UPPER_WMARK 0x10
> #define ACTMON_DEV_AVG_LOWER_WMARK 0x14
> #define ACTMON_DEV_COUNT_WEIGHT 0x18
> +#define ACTMON_DEV_COUNT 0x1c
> #define ACTMON_DEV_AVG_COUNT 0x20
> #define ACTMON_DEV_INTR_STATUS 0x24
>
> @@ -276,6 +277,9 @@ static void tegra_actmon_get_lower_upper(struct tegra_devfreq *tegra,
> unsigned long *lower,
> unsigned long *upper)
> {
> + struct device *ddev = tegra->devfreq->dev.parent;
> + u32 offset = dev->config->offset;
> +
> /*
> * Memory frequencies are guaranteed to have 1MHz granularity
> * and thus we need this rounding down to get a proper watermarks
> @@ -288,6 +292,9 @@ static void tegra_actmon_get_lower_upper(struct tegra_devfreq *tegra,
> *lower = tegra_actmon_lower_freq(tegra, target_freq);
> *upper = tegra_actmon_upper_freq(tegra, target_freq);
>
> + dev_dbg(ddev, "%03x: target_freq %lu lower freq %lu upper freq %lu\n",
> + offset, target_freq, *lower, *upper);
> +
> *lower /= KHZ;
> *upper /= KHZ;
>
> @@ -367,11 +374,31 @@ static void tegra_devfreq_update_wmark(struct tegra_devfreq *tegra,
> device_writel(dev, lower + delta, ACTMON_DEV_LOWER_WMARK);
> }
>
> +static void actmon_device_debug(struct tegra_devfreq *tegra,
> + struct tegra_devfreq_device *dev,
> + const char *prefix)
> +{
> + dev_dbg(tegra->devfreq->dev.parent,
> + "%03x: %s: 0x%08x 0x%08x a %u %u %u c %u %u %u b %lu cpu %u\n",
> + dev->config->offset, prefix,
> + device_readl(dev, ACTMON_DEV_INTR_STATUS),
> + device_readl(dev, ACTMON_DEV_CTRL),
> + device_readl(dev, ACTMON_DEV_AVG_COUNT),
> + device_readl(dev, ACTMON_DEV_AVG_LOWER_WMARK),
> + device_readl(dev, ACTMON_DEV_AVG_UPPER_WMARK),
> + device_readl(dev, ACTMON_DEV_COUNT),
> + device_readl(dev, ACTMON_DEV_LOWER_WMARK),
> + device_readl(dev, ACTMON_DEV_UPPER_WMARK),
> + dev->boost_freq, cpufreq_get(0));
> +}
> +
> static void actmon_isr_device(struct tegra_devfreq *tegra,
> struct tegra_devfreq_device *dev)
> {
> u32 intr_status, dev_ctrl, avg_intr_mask;
>
> + actmon_device_debug(tegra, dev, "isr+");
> +
> dev->avg_count = device_readl(dev, ACTMON_DEV_AVG_COUNT);
> intr_status = device_readl(dev, ACTMON_DEV_INTR_STATUS);
> dev_ctrl = device_readl(dev, ACTMON_DEV_CTRL);
> @@ -422,6 +449,8 @@ static void actmon_isr_device(struct tegra_devfreq *tegra,
>
> device_writel(dev, dev_ctrl, ACTMON_DEV_CTRL);
> device_writel(dev, ACTMON_INTR_STATUS_CLEAR, ACTMON_DEV_INTR_STATUS);
> +
> + actmon_device_debug(tegra, dev, "isr-");
> }
>
> static unsigned long actmon_update_target(struct tegra_devfreq *tegra,
> @@ -712,6 +741,7 @@ static struct devfreq_dev_profile tegra_devfreq_profile = {
> static int tegra_governor_get_target(struct devfreq *devfreq,
> unsigned long *freq)
> {
> + struct device *ddev = devfreq->dev.parent;
> struct devfreq_dev_status *stat;
> struct tegra_devfreq *tegra;
> struct tegra_devfreq_device *dev;
> @@ -734,6 +764,11 @@ static int tegra_governor_get_target(struct devfreq *devfreq,
> dev_target_freq = actmon_update_target(tegra, dev);
>
> target_freq = max(target_freq, dev_target_freq);
> +
> + dev_dbg(ddev, "%03x: upd: dev_target_freq %lu\n",
> + dev->config->offset, dev_target_freq);
> +
> + actmon_device_debug(tegra, dev, "upd");
> }
>
> *freq = target_freq * KHZ;
>
--
Best Regards,
Chanwoo Choi
Samsung Electronics
^ permalink raw reply
* Re: [PATCH v4 10/24] PM / devfreq: tegra30: Don't enable consecutive-down interrupt on startup
From: Chanwoo Choi @ 2019-07-16 12:17 UTC (permalink / raw)
To: Dmitry Osipenko, Thierry Reding, MyungJoo Ham, Kyungmin Park,
Jonathan Hunter, Tomeu Vizoso
Cc: linux-pm, linux-tegra, linux-kernel
In-Reply-To: <20190707223303.6755-11-digetx@gmail.com>
Hi Dmitry,
On 19. 7. 8. 오전 7:32, Dmitry Osipenko wrote:
> The consecutive-down event tells that we should perform frequency
> de-boosting, but boosting is in a reset state on start and hence the
> event won't do anything useful for us and it will be just a dummy
> interrupt request.
>
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
> drivers/devfreq/tegra30-devfreq.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
> index 32fe95458ee7..878c9396bb8c 100644
> --- a/drivers/devfreq/tegra30-devfreq.c
> +++ b/drivers/devfreq/tegra30-devfreq.c
> @@ -558,7 +558,6 @@ static void tegra_actmon_configure_device(struct tegra_devfreq *tegra,
> << ACTMON_DEV_CTRL_CONSECUTIVE_ABOVE_WMARK_NUM_SHIFT;
> val |= ACTMON_DEV_CTRL_AVG_ABOVE_WMARK_EN;
> val |= ACTMON_DEV_CTRL_AVG_BELOW_WMARK_EN;
> - val |= ACTMON_DEV_CTRL_CONSECUTIVE_BELOW_WMARK_EN;
> val |= ACTMON_DEV_CTRL_CONSECUTIVE_ABOVE_WMARK_EN;
> val |= ACTMON_DEV_CTRL_ENB;
>
>
Maybe, I think that better to review it by Thierry.
I'm not sure it is right or not because it depend on h/w.
--
Best Regards,
Chanwoo Choi
Samsung Electronics
^ permalink raw reply
* [PATCH 00/14] pending doc patches for 5.3-rc
From: Mauro Carvalho Chehab @ 2019-07-16 12:10 UTC (permalink / raw)
Cc: Mauro Carvalho Chehab, linux-scsi, esc.storagedev, linuxppc-dev,
Jonathan Corbet, alsa-devel, kvm, linux-i2c, rcu, linux-pm,
linux-doc, devicetree, linux-arch, linux-arm-kernel,
linux-watchdog, x86, dri-devel, netdev, linux-crypto, linux-sh,
linux-input, linux-pci
Those are the pending documentation patches after my pull request
for this branch:
git://git.kernel.org/pub/scm/linux/kernel/git/mchehab/linux-media.git tags/docs/v5.3-1
Patches 1 to 13 were already submitted, but got rebased. Patch 14
is a new fixup one.
Patches 1 and 2 weren't submitted before due to merge conflicts
that are now solved upstream;
Patch 3 fixes a series of random Documentation/* references that
are pointing to the wrong places.
Patch 4 fix a longstanding issue: every time a new book is added,
conf.py need changes, in order to allow generating a PDF file.
After the patch, conf.py will automatically recognize new books,
saving the trouble of keeping adding documents to it.
Patches 5 to 11 are due to fonts support when building translations.pdf.
The main focus is to add xeCJK support. While doing it, I discovered
some bugs at sphinx-pre-install script after running it with 7 different
distributions.
Patch 12 improves support for partial doc building. Currently, each
subdir needs to have its own conf.py, in order to support partial
doc build. After it, any Documentation subdir can be used to
roduce html/pdf docs with:
make SPHINXDIRS="foo bar" htmldocs
(or pdfdocs, latexdocs, epubdocs, ...)
Patch 13 is a cleanup patch: it simply get rid of all those extra
conf.py files that aren't needed anymore. The only extra config
file after it is this one:
Documentation/media/conf_nitpick.py
With enables some extra optional Sphinx features.
Patch 14 adds Documentation/virtual to the main index.rst file
and add a new *.rst file that was orphaned there.
-
After this series, there's just one more patch meant to be applied
for 5.3, with is still waiting for some patches to be merged from
linux-next:
https://git.linuxtv.org/mchehab/experimental.git/commit/?id=b1b5dc7d7bbfbbfdace2a248c6458301c6e34100
Mauro Carvalho Chehab (14):
docs: powerpc: convert docs to ReST and rename to *.rst
docs: power: add it to to the main documentation index
docs: fix broken doc references due to renames
docs: pdf: add all Documentation/*/index.rst to PDF output
docs: conf.py: add CJK package needed by translations
docs: conf.py: only use CJK if the font is available
scripts/sphinx-pre-install: fix script for RHEL/CentOS
scripts/sphinx-pre-install: don't use LaTeX with CentOS 7
scripts/sphinx-pre-install: fix latexmk dependencies
scripts/sphinx-pre-install: cleanup Gentoo checks
scripts/sphinx-pre-install: seek for Noto CJK fonts for pdf output
docs: load_config.py: avoid needing a conf.py just due to LaTeX docs
docs: remove extra conf.py files
docs: virtual: add it to the documentation body
Documentation/PCI/pci-error-recovery.rst | 5 +-
Documentation/RCU/rculist_nulls.txt | 2 +-
Documentation/admin-guide/conf.py | 10 --
Documentation/conf.py | 30 +++-
Documentation/core-api/conf.py | 10 --
Documentation/crypto/conf.py | 10 --
Documentation/dev-tools/conf.py | 10 --
.../devicetree/bindings/arm/idle-states.txt | 2 +-
Documentation/doc-guide/conf.py | 10 --
Documentation/driver-api/80211/conf.py | 10 --
Documentation/driver-api/conf.py | 10 --
Documentation/driver-api/pm/conf.py | 10 --
Documentation/filesystems/conf.py | 10 --
Documentation/gpu/conf.py | 10 --
Documentation/index.rst | 3 +
Documentation/input/conf.py | 10 --
Documentation/kernel-hacking/conf.py | 10 --
Documentation/locking/spinlocks.rst | 4 +-
Documentation/maintainer/conf.py | 10 --
Documentation/media/conf.py | 12 --
Documentation/memory-barriers.txt | 2 +-
Documentation/networking/conf.py | 10 --
Documentation/power/index.rst | 2 +-
.../{bootwrapper.txt => bootwrapper.rst} | 28 +++-
.../{cpu_families.txt => cpu_families.rst} | 23 +--
.../{cpu_features.txt => cpu_features.rst} | 6 +-
Documentation/powerpc/{cxl.txt => cxl.rst} | 46 ++++--
.../powerpc/{cxlflash.txt => cxlflash.rst} | 10 +-
.../{DAWR-POWER9.txt => dawr-power9.rst} | 15 +-
Documentation/powerpc/{dscr.txt => dscr.rst} | 18 +-
...ecovery.txt => eeh-pci-error-recovery.rst} | 108 ++++++------
...ed-dump.txt => firmware-assisted-dump.rst} | 117 +++++++------
Documentation/powerpc/{hvcs.txt => hvcs.rst} | 108 ++++++------
Documentation/powerpc/index.rst | 34 ++++
Documentation/powerpc/isa-versions.rst | 15 +-
.../powerpc/{mpc52xx.txt => mpc52xx.rst} | 12 +-
...nv.txt => pci_iov_resource_on_powernv.rst} | 15 +-
.../powerpc/{pmu-ebb.txt => pmu-ebb.rst} | 1 +
Documentation/powerpc/ptrace.rst | 156 ++++++++++++++++++
Documentation/powerpc/ptrace.txt | 151 -----------------
.../{qe_firmware.txt => qe_firmware.rst} | 37 +++--
.../{syscall64-abi.txt => syscall64-abi.rst} | 29 ++--
...al_memory.txt => transactional_memory.rst} | 45 ++---
Documentation/process/conf.py | 10 --
Documentation/sh/conf.py | 10 --
Documentation/sound/conf.py | 10 --
Documentation/sphinx/load_config.py | 27 ++-
.../translations/ko_KR/memory-barriers.txt | 2 +-
Documentation/userspace-api/conf.py | 10 --
Documentation/virtual/kvm/index.rst | 1 +
Documentation/vm/conf.py | 10 --
Documentation/watchdog/hpwdt.rst | 2 +-
Documentation/x86/conf.py | 10 --
MAINTAINERS | 14 +-
arch/powerpc/kernel/exceptions-64s.S | 2 +-
drivers/gpu/drm/drm_modes.c | 2 +-
drivers/i2c/busses/i2c-nvidia-gpu.c | 2 +-
drivers/scsi/hpsa.c | 4 +-
drivers/soc/fsl/qe/qe.c | 2 +-
drivers/tty/hvc/hvcs.c | 2 +-
include/soc/fsl/qe/qe.h | 2 +-
scripts/sphinx-pre-install | 118 ++++++++++---
62 files changed, 738 insertions(+), 678 deletions(-)
delete mode 100644 Documentation/admin-guide/conf.py
delete mode 100644 Documentation/core-api/conf.py
delete mode 100644 Documentation/crypto/conf.py
delete mode 100644 Documentation/dev-tools/conf.py
delete mode 100644 Documentation/doc-guide/conf.py
delete mode 100644 Documentation/driver-api/80211/conf.py
delete mode 100644 Documentation/driver-api/conf.py
delete mode 100644 Documentation/driver-api/pm/conf.py
delete mode 100644 Documentation/filesystems/conf.py
delete mode 100644 Documentation/gpu/conf.py
delete mode 100644 Documentation/input/conf.py
delete mode 100644 Documentation/kernel-hacking/conf.py
delete mode 100644 Documentation/maintainer/conf.py
delete mode 100644 Documentation/media/conf.py
delete mode 100644 Documentation/networking/conf.py
rename Documentation/powerpc/{bootwrapper.txt => bootwrapper.rst} (93%)
rename Documentation/powerpc/{cpu_families.txt => cpu_families.rst} (95%)
rename Documentation/powerpc/{cpu_features.txt => cpu_features.rst} (97%)
rename Documentation/powerpc/{cxl.txt => cxl.rst} (95%)
rename Documentation/powerpc/{cxlflash.txt => cxlflash.rst} (98%)
rename Documentation/powerpc/{DAWR-POWER9.txt => dawr-power9.rst} (95%)
rename Documentation/powerpc/{dscr.txt => dscr.rst} (91%)
rename Documentation/powerpc/{eeh-pci-error-recovery.txt => eeh-pci-error-recovery.rst} (82%)
rename Documentation/powerpc/{firmware-assisted-dump.txt => firmware-assisted-dump.rst} (80%)
rename Documentation/powerpc/{hvcs.txt => hvcs.rst} (91%)
create mode 100644 Documentation/powerpc/index.rst
rename Documentation/powerpc/{mpc52xx.txt => mpc52xx.rst} (91%)
rename Documentation/powerpc/{pci_iov_resource_on_powernv.txt => pci_iov_resource_on_powernv.rst} (97%)
rename Documentation/powerpc/{pmu-ebb.txt => pmu-ebb.rst} (99%)
create mode 100644 Documentation/powerpc/ptrace.rst
delete mode 100644 Documentation/powerpc/ptrace.txt
rename Documentation/powerpc/{qe_firmware.txt => qe_firmware.rst} (95%)
rename Documentation/powerpc/{syscall64-abi.txt => syscall64-abi.rst} (82%)
rename Documentation/powerpc/{transactional_memory.txt => transactional_memory.rst} (93%)
delete mode 100644 Documentation/process/conf.py
delete mode 100644 Documentation/sh/conf.py
delete mode 100644 Documentation/sound/conf.py
delete mode 100644 Documentation/userspace-api/conf.py
delete mode 100644 Documentation/vm/conf.py
delete mode 100644 Documentation/x86/conf.py
--
2.21.0
^ permalink raw reply
* [PATCH 02/14] docs: power: add it to to the main documentation index
From: Mauro Carvalho Chehab @ 2019-07-16 12:10 UTC (permalink / raw)
Cc: Mauro Carvalho Chehab, Jonathan Corbet, Rafael J. Wysocki,
Len Brown, Pavel Machek, linux-doc, linux-pm
In-Reply-To: <cover.1563277838.git.mchehab+samsung@kernel.org>
The power docs are orphaned at the documentation body.
While it could likely be moved to be inside some guide, I'm opting to just
adding it to the main index.rst, removing the :orphan: and adding the SPDX
header.
The reason is similar to what it was done for other driver-specific
subsystems: the docs there contain a mix of Kernelspace, uAPI and
admin-guide. So, better to keep them on its own directory,
while the docs there are not properly classified.
Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
---
Documentation/index.rst | 1 +
Documentation/power/index.rst | 2 +-
2 files changed, 2 insertions(+), 1 deletion(-)
diff --git a/Documentation/index.rst b/Documentation/index.rst
index 3fe6170aa41d..68ae2a4d689d 100644
--- a/Documentation/index.rst
+++ b/Documentation/index.rst
@@ -111,6 +111,7 @@ needed).
netlabel/index
networking/index
pcmcia/index
+ power/index
target/index
timers/index
watchdog/index
diff --git a/Documentation/power/index.rst b/Documentation/power/index.rst
index 20415f21e48a..002e42745263 100644
--- a/Documentation/power/index.rst
+++ b/Documentation/power/index.rst
@@ -1,4 +1,4 @@
-:orphan:
+.. SPDX-License-Identifier: GPL-2.0
================
Power Management
--
2.21.0
^ permalink raw reply related
* Re: [PATCH v4 09/24] PM / devfreq: tegra30: Reset boosting on startup
From: Chanwoo Choi @ 2019-07-16 12:13 UTC (permalink / raw)
To: Dmitry Osipenko, Thierry Reding, MyungJoo Ham, Kyungmin Park,
Jonathan Hunter, Tomeu Vizoso
Cc: linux-pm, linux-tegra, linux-kernel
In-Reply-To: <20190707223303.6755-10-digetx@gmail.com>
On 19. 7. 8. 오전 7:32, Dmitry Osipenko wrote:
> Governor could be stopped while boosting is active. We have assumption
> that everything is reset on governor's restart, including the boosting
> value, which was missed.
>
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
> drivers/devfreq/tegra30-devfreq.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
> index d5d04c25023b..32fe95458ee7 100644
> --- a/drivers/devfreq/tegra30-devfreq.c
> +++ b/drivers/devfreq/tegra30-devfreq.c
> @@ -536,6 +536,9 @@ static void tegra_actmon_configure_device(struct tegra_devfreq *tegra,
> {
> u32 val = 0, target_freq;
>
> + /* we don't want boosting on restart */
nitpick.
I think that following comment is proper in my case.
In my case, I think 'we' expression is not good
/* Reset the boost frequency on restart */
> + dev->boost_freq = 0;
> +
> target_freq = clk_get_rate(tegra->emc_clock) / KHZ;
> dev->avg_count = target_freq * ACTMON_SAMPLING_PERIOD;
> device_writel(dev, dev->avg_count, ACTMON_DEV_INIT_AVG);
>
Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
--
Best Regards,
Chanwoo Choi
Samsung Electronics
^ permalink raw reply
* Re: [PATCH v4 08/24] PM / devfreq: tegra30: Move clk-notifier's registration to governor's start
From: Chanwoo Choi @ 2019-07-16 12:11 UTC (permalink / raw)
To: Dmitry Osipenko, Thierry Reding, MyungJoo Ham, Kyungmin Park,
Jonathan Hunter, Tomeu Vizoso
Cc: linux-pm, linux-tegra, linux-kernel
In-Reply-To: <20190707223303.6755-9-digetx@gmail.com>
Hi Dmitiry,
On 19. 7. 8. 오전 7:32, Dmitry Osipenko wrote:
> There is no point in receiving of the notifications while governor is
> stopped, let's keep them disabled like we do for the CPU freq-change
> notifications. This also fixes a potential use-after-free bug if
> notification happens after device's removal.
>
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
> drivers/devfreq/tegra30-devfreq.c | 33 ++++++++++++++++++-------------
> 1 file changed, 19 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
> index 48a799fa5f63..d5d04c25023b 100644
> --- a/drivers/devfreq/tegra30-devfreq.c
> +++ b/drivers/devfreq/tegra30-devfreq.c
> @@ -576,6 +576,19 @@ static int tegra_actmon_start(struct tegra_devfreq *tegra)
> actmon_writel(tegra, ACTMON_SAMPLING_PERIOD - 1,
> ACTMON_GLB_PERIOD_CTRL);
>
> + /*
> + * CLK notifications are needed in order to reconfigure the upper
> + * consecutive watermark in accordance to the actual clock rate
> + * to avoid unnecessary upper interrupts.
> + */
> + err = clk_notifier_register(tegra->emc_clock,
> + &tegra->clk_rate_change_nb);
> + if (err) {
> + dev_err(tegra->devfreq->dev.parent,
> + "Failed to register rate change notifier\n");
> + return err;
> + }
> +
> for (i = 0; i < ARRAY_SIZE(tegra->devices); i++)
> tegra_actmon_configure_device(tegra, &tegra->devices[i]);
>
> @@ -602,6 +615,8 @@ static int tegra_actmon_start(struct tegra_devfreq *tegra)
> for (i = 0; i < ARRAY_SIZE(tegra->devices); i++)
> tegra_actmon_stop_device(&tegra->devices[i]);
>
> + clk_notifier_unregister(tegra->emc_clock, &tegra->clk_rate_change_nb);
> +
> return err;
> }
>
> @@ -618,6 +633,8 @@ static void tegra_actmon_stop(struct tegra_devfreq *tegra)
>
> for (i = 0; i < ARRAY_SIZE(tegra->devices); i++)
> tegra_actmon_stop_device(&tegra->devices[i]);
> +
> + clk_notifier_unregister(tegra->emc_clock, &tegra->clk_rate_change_nb);
> }
>
> static int tegra_devfreq_target(struct device *dev, unsigned long *freq,
> @@ -862,22 +879,14 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
>
> platform_set_drvdata(pdev, tegra);
>
> + tegra->clk_rate_change_nb.notifier_call = tegra_actmon_clk_notify_cb;
> tegra->cpu_rate_change_nb.notifier_call = tegra_actmon_cpu_notify_cb;
> INIT_WORK(&tegra->update_work, tegra_actmon_delayed_update);
>
> - tegra->clk_rate_change_nb.notifier_call = tegra_actmon_clk_notify_cb;
> - err = clk_notifier_register(tegra->emc_clock,
> - &tegra->clk_rate_change_nb);
> - if (err) {
> - dev_err(&pdev->dev,
> - "Failed to register rate change notifier\n");
> - goto remove_opps;
> - }
> -
> err = devfreq_add_governor(&tegra_devfreq_governor);
> if (err) {
> dev_err(&pdev->dev, "Failed to add governor: %d\n", err);
> - goto unreg_notifier;
> + goto remove_opps;
> }
>
> tegra_devfreq_profile.initial_freq = clk_get_rate(tegra->emc_clock);
> @@ -893,9 +902,6 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
> remove_governor:
> devfreq_remove_governor(&tegra_devfreq_governor);
>
> -unreg_notifier:
> - clk_notifier_unregister(tegra->emc_clock, &tegra->clk_rate_change_nb);
> -
> remove_opps:
> dev_pm_opp_remove_all_dynamic(&pdev->dev);
>
> @@ -912,7 +918,6 @@ static int tegra_devfreq_remove(struct platform_device *pdev)
> devfreq_remove_device(tegra->devfreq);
> devfreq_remove_governor(&tegra_devfreq_governor);
>
> - clk_notifier_unregister(tegra->emc_clock, &tegra->clk_rate_change_nb);
> dev_pm_opp_remove_all_dynamic(&pdev->dev);
>
> reset_control_reset(tegra->reset);
>
Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
--
Best Regards,
Chanwoo Choi
Samsung Electronics
^ permalink raw reply
* Re: [PATCH v4 07/24] PM / devfreq: tegra30: Use CPUFreq notifier
From: Chanwoo Choi @ 2019-07-16 12:08 UTC (permalink / raw)
To: Dmitry Osipenko, Thierry Reding, MyungJoo Ham, Kyungmin Park,
Jonathan Hunter, Tomeu Vizoso
Cc: linux-pm, linux-tegra, linux-kernel
In-Reply-To: <20190707223303.6755-8-digetx@gmail.com>
On 19. 7. 8. 오전 7:32, Dmitry Osipenko wrote:
> The CPU's client need to take into account that CPUFreq may change
> while memory activity not, staying high. Thus an appropriate frequency
> notifier should be used in addition to the clk-notifier.
>
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
> drivers/devfreq/tegra30-devfreq.c | 105 +++++++++++++++++++++++++-----
> 1 file changed, 88 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
> index 2bf65409ddd8..48a799fa5f63 100644
> --- a/drivers/devfreq/tegra30-devfreq.c
> +++ b/drivers/devfreq/tegra30-devfreq.c
> @@ -17,6 +17,7 @@
> #include <linux/platform_device.h>
> #include <linux/pm_opp.h>
> #include <linux/reset.h>
> +#include <linux/workqueue.h>
>
> #include "governor.h"
>
> @@ -154,7 +155,10 @@ struct tegra_devfreq {
>
> struct clk *emc_clock;
> unsigned long max_freq;
> - struct notifier_block rate_change_nb;
> + struct notifier_block clk_rate_change_nb;
> +
> + struct work_struct update_work;
nitpick.
I think 'update_work' is not clear to indicate the work
for changing the clock according to the cpu clock change.
You better to change the name for more clearly.
> + struct notifier_block cpu_rate_change_nb;
>
> struct tegra_devfreq_device devices[ARRAY_SIZE(actmon_device_configs)];
>
> @@ -456,8 +460,8 @@ static irqreturn_t actmon_thread_isr(int irq, void *data)
> return handled ? IRQ_HANDLED : IRQ_NONE;
> }
>
> -static int tegra_actmon_rate_notify_cb(struct notifier_block *nb,
> - unsigned long action, void *ptr)
> +static int tegra_actmon_clk_notify_cb(struct notifier_block *nb,
> + unsigned long action, void *ptr)
> {
> struct clk_notifier_data *data = ptr;
> struct tegra_devfreq_device *dev;
> @@ -467,7 +471,7 @@ static int tegra_actmon_rate_notify_cb(struct notifier_block *nb,
> if (action != POST_RATE_CHANGE)
> return NOTIFY_OK;
>
> - tegra = container_of(nb, struct tegra_devfreq, rate_change_nb);
> + tegra = container_of(nb, struct tegra_devfreq, clk_rate_change_nb);
>
> /*
> * EMC rate could change due to three reasons:
> @@ -496,6 +500,37 @@ static int tegra_actmon_rate_notify_cb(struct notifier_block *nb,
> return NOTIFY_OK;
> }
>
> +static void tegra_actmon_delayed_update(struct work_struct *work)
> +{
> + struct tegra_devfreq *tegra = container_of(work, struct tegra_devfreq,
> + update_work);
> +
> + mutex_lock(&tegra->devfreq->lock);
> + update_devfreq(tegra->devfreq);
> + mutex_unlock(&tegra->devfreq->lock);
> +}
> +
> +static int tegra_actmon_cpu_notify_cb(struct notifier_block *nb,
> + unsigned long action, void *ptr)
> +{
> + struct tegra_devfreq *tegra;
> +
> + if (action != CPUFREQ_POSTCHANGE)
> + return NOTIFY_OK;
> +
> + tegra = container_of(nb, struct tegra_devfreq, cpu_rate_change_nb);
nitpick. Better to check whether 'tegra' is NULL or not.
> +
> + /*
> + * CPUFreq driver should support CPUFREQ_ASYNC_NOTIFICATION in order
> + * to allow asynchronous notifications. This means we can't block
> + * here for too long, otherwise CPUFreq's core will complain with a
> + * warning splat.
> + */
> + schedule_work(&tegra->update_work);
> +
> + return NOTIFY_OK;
> +}
> +
> static void tegra_actmon_configure_device(struct tegra_devfreq *tegra,
> struct tegra_devfreq_device *dev)
> {
> @@ -527,9 +562,16 @@ static void tegra_actmon_configure_device(struct tegra_devfreq *tegra,
> device_writel(dev, val, ACTMON_DEV_CTRL);
> }
>
> -static void tegra_actmon_start(struct tegra_devfreq *tegra)
> +static void tegra_actmon_stop_device(struct tegra_devfreq_device *dev)
> +{
> + device_writel(dev, 0x00000000, ACTMON_DEV_CTRL);
Better to define the constant definition of 0x00000000
in order to explain the correct meaning.
For example,
#define ACTMON_DEV_RESET 0x00000000
> + device_writel(dev, ACTMON_INTR_STATUS_CLEAR, ACTMON_DEV_INTR_STATUS);
> +}
> +
> +static int tegra_actmon_start(struct tegra_devfreq *tegra)
> {
> unsigned int i;
> + int err;
>
> actmon_writel(tegra, ACTMON_SAMPLING_PERIOD - 1,
> ACTMON_GLB_PERIOD_CTRL);
> @@ -537,7 +579,30 @@ static void tegra_actmon_start(struct tegra_devfreq *tegra)
> for (i = 0; i < ARRAY_SIZE(tegra->devices); i++)
> tegra_actmon_configure_device(tegra, &tegra->devices[i]);
>
> + /*
> + * We are estimating CPU's memory bandwidth requirement based on
> + * amount of memory accesses and system's load, judging by CPU's
> + * frequency. We also don't want to receive events about CPU's
> + * frequency transaction when governor is stopped, hence notifier
> + * is registered dynamically.
> + */
> + err = cpufreq_register_notifier(&tegra->cpu_rate_change_nb,
> + CPUFREQ_TRANSITION_NOTIFIER);
> + if (err) {
> + dev_err(tegra->devfreq->dev.parent,
> + "Failed to register rate change notifier: %d\n", err);
> + goto err_stop;
> + }> +
> enable_irq(tegra->irq);
> +
> + return 0;
> +
> +err_stop:
> + for (i = 0; i < ARRAY_SIZE(tegra->devices); i++)
> + tegra_actmon_stop_device(&tegra->devices[i]);
> +
> + return err;
> }
>
> static void tegra_actmon_stop(struct tegra_devfreq *tegra)
> @@ -546,11 +611,13 @@ static void tegra_actmon_stop(struct tegra_devfreq *tegra)
>
> disable_irq(tegra->irq);
>
> - for (i = 0; i < ARRAY_SIZE(tegra->devices); i++) {
> - device_writel(&tegra->devices[i], 0x00000000, ACTMON_DEV_CTRL);
> - device_writel(&tegra->devices[i], ACTMON_INTR_STATUS_CLEAR,
> - ACTMON_DEV_INTR_STATUS);
> - }
> + cpufreq_unregister_notifier(&tegra->cpu_rate_change_nb,
> + CPUFREQ_TRANSITION_NOTIFIER);
> +
> + cancel_work_sync(&tegra->update_work);
> +
> + for (i = 0; i < ARRAY_SIZE(tegra->devices); i++)
> + tegra_actmon_stop_device(&tegra->devices[i]);
> }
>
> static int tegra_devfreq_target(struct device *dev, unsigned long *freq,
> @@ -659,6 +726,7 @@ static int tegra_governor_event_handler(struct devfreq *devfreq,
> unsigned int event, void *data)
> {
> struct tegra_devfreq *tegra = dev_get_drvdata(devfreq->dev.parent);
> + int ret = 0;
>
> /*
> * Couple device with the governor early as it is needed at
> @@ -669,7 +737,7 @@ static int tegra_governor_event_handler(struct devfreq *devfreq,
> switch (event) {
> case DEVFREQ_GOV_START:
> devfreq_monitor_start(devfreq);
> - tegra_actmon_start(tegra);
> + ret = tegra_actmon_start(tegra);
> break;
>
> case DEVFREQ_GOV_STOP:
> @@ -684,11 +752,11 @@ static int tegra_governor_event_handler(struct devfreq *devfreq,
>
> case DEVFREQ_GOV_RESUME:
> devfreq_monitor_resume(devfreq);
> - tegra_actmon_start(tegra);
> + ret = tegra_actmon_start(tegra);
> break;
> }
>
> - return 0;
> + return ret;
> }
>
> static struct devfreq_governor tegra_devfreq_governor = {
> @@ -794,9 +862,12 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
>
> platform_set_drvdata(pdev, tegra);
>
> - tegra-> .notifier_call = tegra_actmon_rate_notify_cb;
> + tegra->cpu_rate_change_nb.notifier_call = tegra_actmon_cpu_notify_cb;
> + INIT_WORK(&tegra->update_work, tegra_actmon_delayed_update);
> +
> + tegra->clk_rate_change_nb.notifier_call = tegra_actmon_clk_notify_cb;
> err = clk_notifier_register(tegra->emc_clock,
> - &tegra->rate_change_nb);
> + &tegra->clk_rate_change_nb);
> if (err) {
> dev_err(&pdev->dev,
> "Failed to register rate change notifier\n");
> @@ -823,7 +894,7 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
> devfreq_remove_governor(&tegra_devfreq_governor);
>
> unreg_notifier:
> - clk_notifier_unregister(tegra->emc_clock, &tegra->rate_change_nb);
> + clk_notifier_unregister(tegra->emc_clock, &tegra->clk_rate_change_nb);
>
> remove_opps:
> dev_pm_opp_remove_all_dynamic(&pdev->dev);
> @@ -841,7 +912,7 @@ static int tegra_devfreq_remove(struct platform_device *pdev)
> devfreq_remove_device(tegra->devfreq);
> devfreq_remove_governor(&tegra_devfreq_governor);
>
> - clk_notifier_unregister(tegra->emc_clock, &tegra->rate_change_nb);
> + clk_notifier_unregister(tegra->emc_clock, &tegra->clk_rate_change_nb);
> dev_pm_opp_remove_all_dynamic(&pdev->dev);
>
> reset_control_reset(tegra->reset);
>
--
Best Regards,
Chanwoo Choi
Samsung Electronics
^ permalink raw reply
* Re: [PATCH v2 2/4] devfreq: exynos-bus: convert to use dev_pm_opp_set_rate()
From: Bartlomiej Zolnierkiewicz @ 2019-07-16 11:56 UTC (permalink / raw)
To: Chanwoo Choi
Cc: Kamil Konieczny, Marek Szyprowski, Krzysztof Kozlowski,
Kukjin Kim, Kyungmin Park, Mark Rutland, MyungJoo Ham,
Nishanth Menon, Rob Herring, Stephen Boyd, Viresh Kumar,
devicetree, linux-arm-kernel, linux-kernel, linux-pm,
linux-samsung-soc
In-Reply-To: <1a9e5752-bc2b-3b08-a36b-fc02ca51764c@samsung.com>
On 7/16/19 1:39 PM, Bartlomiej Zolnierkiewicz wrote:
>
> On 7/16/19 1:26 PM, Chanwoo Choi wrote:
[...]
>>> Doesn't seem to be needed, care to explain it more?
>>
>> In order to fix the sequence problem between clock and regulator
>> with dev_pm_opp_set_regualtor() and want to keep two functions
>> (exynos_bus_parent_parse_of() and exynos_bus_parse_of()),
>> have to change the call order as following and then modify
>> the exception handling code when error happen.
>>
>> node = of_parse_phandle(dev->of_node, "devfreq", 0);
>> if (node) {
>> of_node_put(node);
>> passive = true
>> }
>>
>> if (!passive)
>> exynos_bus_parent_parse_of()
>> dev_pm_opp_set_regulator
>>
>> exynos_bus_parse_of()
>
> OK. This seems like a solution.
PS Thanks for explaining this in detail.
Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics
^ permalink raw reply
* Re: [PATCH v4 06/24] PM / devfreq: tegra30: Tune up boosting thresholds
From: Chanwoo Choi @ 2019-07-16 11:55 UTC (permalink / raw)
To: Dmitry Osipenko, Thierry Reding, MyungJoo Ham, Kyungmin Park,
Jonathan Hunter, Tomeu Vizoso
Cc: linux-pm, linux-tegra, linux-kernel
In-Reply-To: <20190707223303.6755-7-digetx@gmail.com>
On 19. 7. 8. 오전 7:32, Dmitry Osipenko wrote:
> Now that average-sustain coefficient / multiplier is gone, it won't hurt
> to re-tune the boosting thresholds to get a bit harder boosting for MCALL
> clients, resulting in a more reactive governing in a case of multimedia
> applications usage like 3d / video.
>
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
> drivers/devfreq/tegra30-devfreq.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
> index 16f7e6cf3b99..2bf65409ddd8 100644
> --- a/drivers/devfreq/tegra30-devfreq.c
> +++ b/drivers/devfreq/tegra30-devfreq.c
> @@ -111,8 +111,8 @@ static struct tegra_devfreq_device_config actmon_device_configs[] = {
> .irq_mask = 1 << 26,
> .boost_up_coeff = 200,
> .boost_down_coeff = 50,
> - .boost_up_threshold = 60,
> - .boost_down_threshold = 40,
> + .boost_up_threshold = 50,
> + .boost_down_threshold = 25,
> },
> {
> /* MCCPU: memory accesses from the CPUs */
>
It just adjusts the tunable point.
Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
--
Best Regards,
Chanwoo Choi
Samsung Electronics
^ permalink raw reply
* Re: [PATCH v4 04/24] PM / devfreq: tegra30: Drop write-barrier
From: Chanwoo Choi @ 2019-07-16 11:51 UTC (permalink / raw)
To: Dmitry Osipenko, Thierry Reding, MyungJoo Ham, Kyungmin Park,
Jonathan Hunter, Tomeu Vizoso
Cc: linux-pm, linux-tegra, linux-kernel
In-Reply-To: <20190707223303.6755-5-digetx@gmail.com>
Hi Dmitry,
On 19. 7. 8. 오전 7:32, Dmitry Osipenko wrote:
> There is no need in a write-barrier now, given that interrupt masking is
> handled by CPU's GIC now. Hence we know exactly that interrupt won't fire
> after stopping the devfreq's governor. In other cases we don't care about
> potential buffering of the writes to hardware and thus there is no need to
> stall CPU.
>
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
> drivers/devfreq/tegra30-devfreq.c | 14 --------------
> 1 file changed, 14 deletions(-)
>
> diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
> index 5e606ae3f238..4be7858c33bc 100644
> --- a/drivers/devfreq/tegra30-devfreq.c
> +++ b/drivers/devfreq/tegra30-devfreq.c
> @@ -230,12 +230,6 @@ static void tegra_devfreq_update_wmark(struct tegra_devfreq *tegra,
> ACTMON_DEV_LOWER_WMARK);
> }
>
> -static void actmon_write_barrier(struct tegra_devfreq *tegra)
> -{
> - /* ensure the update has reached the ACTMON */
> - readl(tegra->regs + ACTMON_GLB_STATUS);
> -}
> -
> static void actmon_isr_device(struct tegra_devfreq *tegra,
> struct tegra_devfreq_device *dev)
> {
> @@ -287,8 +281,6 @@ static void actmon_isr_device(struct tegra_devfreq *tegra,
> device_writel(dev, dev_ctrl, ACTMON_DEV_CTRL);
>
> device_writel(dev, ACTMON_INTR_STATUS_CLEAR, ACTMON_DEV_INTR_STATUS);
> -
> - actmon_write_barrier(tegra);
> }
>
> static unsigned long actmon_cpu_to_emc_rate(struct tegra_devfreq *tegra,
> @@ -376,8 +368,6 @@ static int tegra_actmon_rate_notify_cb(struct notifier_block *nb,
> tegra_devfreq_update_wmark(tegra, dev);
> }
>
> - actmon_write_barrier(tegra);
> -
> return NOTIFY_OK;
> }
>
> @@ -423,8 +413,6 @@ static void tegra_actmon_start(struct tegra_devfreq *tegra)
> for (i = 0; i < ARRAY_SIZE(tegra->devices); i++)
> tegra_actmon_configure_device(tegra, &tegra->devices[i]);
>
> - actmon_write_barrier(tegra);
> -
> enable_irq(tegra->irq);
> }
>
> @@ -439,8 +427,6 @@ static void tegra_actmon_stop(struct tegra_devfreq *tegra)
> device_writel(&tegra->devices[i], ACTMON_INTR_STATUS_CLEAR,
> ACTMON_DEV_INTR_STATUS);
> }
> -
> - actmon_write_barrier(tegra);
> }
>
> static int tegra_devfreq_target(struct device *dev, unsigned long *freq,
>
Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
--
Best Regards,
Chanwoo Choi
Samsung Electronics
^ permalink raw reply
* Re: [PATCH v4 03/24] PM / devfreq: tegra30: Handle possible round-rate error
From: Chanwoo Choi @ 2019-07-16 11:50 UTC (permalink / raw)
To: Dmitry Osipenko, Thierry Reding, MyungJoo Ham, Kyungmin Park,
Jonathan Hunter, Tomeu Vizoso
Cc: linux-pm, linux-tegra, linux-kernel
In-Reply-To: <20190707223303.6755-4-digetx@gmail.com>
On 19. 7. 8. 오전 7:32, Dmitry Osipenko wrote:
> The EMC clock rate rounding technically could fail, hence let's handle
> the error cases properly.
>
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
> drivers/devfreq/tegra30-devfreq.c | 17 +++++++++++++++--
> 1 file changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
> index 5e2b133babdd..5e606ae3f238 100644
> --- a/drivers/devfreq/tegra30-devfreq.c
> +++ b/drivers/devfreq/tegra30-devfreq.c
> @@ -592,8 +592,8 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
> struct tegra_devfreq_device *dev;
> struct tegra_devfreq *tegra;
> struct devfreq *devfreq;
> - unsigned long rate;
> unsigned int i;
> + long rate;
> int err;
>
> tegra = devm_kzalloc(&pdev->dev, sizeof(*tegra), GFP_KERNEL);
> @@ -650,8 +650,14 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
>
> reset_control_deassert(tegra->reset);
>
> - tegra->max_freq = clk_round_rate(tegra->emc_clock, ULONG_MAX) / KHZ;
> + rate = clk_round_rate(tegra->emc_clock, ULONG_MAX);
> + if (rate < 0) {
> + dev_err(&pdev->dev, "Failed to round clock rate: %ld\n", rate);
> + return rate;
> + }
> +
> tegra->cur_freq = clk_get_rate(tegra->emc_clock) / KHZ;
> + tegra->max_freq = rate / KHZ;
>
> for (i = 0; i < ARRAY_SIZE(actmon_device_configs); i++) {
> dev = tegra->devices + i;
> @@ -662,6 +668,13 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
> for (rate = 0; rate <= tegra->max_freq * KHZ; rate++) {
> rate = clk_round_rate(tegra->emc_clock, rate);
>
Please remove unneeded blank line.
> + if (rate < 0) {
> + dev_err(&pdev->dev,
> + "Failed to round clock rate: %ld\n", rate);
> + err = rate;
> + goto remove_opps;
> + }
Also, this patch doesn't contain code which restore the previous
tegra->cur_freq/max_freq when error happen.
> +
> err = dev_pm_opp_add(&pdev->dev, rate, 0);
> if (err) {
> dev_err(&pdev->dev, "Failed to add OPP: %d\n", err);
>
--
Best Regards,
Chanwoo Choi
Samsung Electronics
^ permalink raw reply
* Re: [PATCH v4 02/24] PM / devfreq: tegra30: Keep interrupt disabled while governor is stopped
From: Chanwoo Choi @ 2019-07-16 11:47 UTC (permalink / raw)
To: Dmitry Osipenko, Thierry Reding, MyungJoo Ham, Kyungmin Park,
Jonathan Hunter, Tomeu Vizoso
Cc: linux-pm, linux-tegra, linux-kernel
In-Reply-To: <20190707223303.6755-3-digetx@gmail.com>
On 19. 7. 8. 오전 7:32, Dmitry Osipenko wrote:
> There is no real need to keep interrupt always-enabled, will be nicer
> to keep it disabled while governor is inactive.
>
> Suggested-by: Thierry Reding <thierry.reding@gmail.com>
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
> drivers/devfreq/tegra30-devfreq.c | 43 ++++++++++++++++---------------
> 1 file changed, 22 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
> index a27300f40b0b..5e2b133babdd 100644
> --- a/drivers/devfreq/tegra30-devfreq.c
> +++ b/drivers/devfreq/tegra30-devfreq.c
> @@ -11,6 +11,7 @@
> #include <linux/devfreq.h>
> #include <linux/interrupt.h>
> #include <linux/io.h>
> +#include <linux/irq.h>
> #include <linux/module.h>
> #include <linux/mod_devicetable.h>
> #include <linux/platform_device.h>
> @@ -416,8 +417,6 @@ static void tegra_actmon_start(struct tegra_devfreq *tegra)
> {
> unsigned int i;
>
> - disable_irq(tegra->irq);
> -
> actmon_writel(tegra, ACTMON_SAMPLING_PERIOD - 1,
> ACTMON_GLB_PERIOD_CTRL);
>
> @@ -442,8 +441,6 @@ static void tegra_actmon_stop(struct tegra_devfreq *tegra)
> }
>
> actmon_write_barrier(tegra);
> -
> - enable_irq(tegra->irq);
> }
>
> static int tegra_devfreq_target(struct device *dev, unsigned long *freq,
> @@ -552,6 +549,12 @@ static int tegra_governor_event_handler(struct devfreq *devfreq,
> {
> struct tegra_devfreq *tegra = dev_get_drvdata(devfreq->dev.parent);
>
> + /*
> + * Couple device with the governor early as it is needed at
> + * the moment of governor's start (used by ISR).
> + */
> + tegra->devfreq = devfreq;
I'm not sure it is necessary. Almost devfreq device get
the devfreq instance on probe timing through devfreq_add_device directly.
> +
> switch (event) {
> case DEVFREQ_GOV_START:
> devfreq_monitor_start(devfreq);
> @@ -586,10 +589,11 @@ static struct devfreq_governor tegra_devfreq_governor = {
>
> static int tegra_devfreq_probe(struct platform_device *pdev)
> {
> - struct tegra_devfreq *tegra;
> struct tegra_devfreq_device *dev;
> - unsigned int i;
> + struct tegra_devfreq *tegra;
> + struct devfreq *devfreq;
> unsigned long rate;
> + unsigned int i;
> int err;
>
> tegra = devm_kzalloc(&pdev->dev, sizeof(*tegra), GFP_KERNEL);
> @@ -625,6 +629,16 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
> }
> tegra->irq = err;
>
> + irq_set_status_flags(tegra->irq, IRQ_NOAUTOEN);
> +
> + err = devm_request_threaded_irq(&pdev->dev, tegra->irq, NULL,
> + actmon_thread_isr, IRQF_ONESHOT,
> + "tegra-devfreq", tegra);
> + if (err) {
> + dev_err(&pdev->dev, "Interrupt request failed: %d\n", err);
> + return err;
> + }
> +
> reset_control_assert(tegra->reset);
>
> err = clk_prepare_enable(tegra->clock);
> @@ -672,28 +686,15 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
> }
>
> tegra_devfreq_profile.initial_freq = clk_get_rate(tegra->emc_clock);
> - tegra->devfreq = devfreq_add_device(&pdev->dev,
> - &tegra_devfreq_profile,
> - "tegra_actmon",
> - NULL);
> + devfreq = devfreq_add_device(&pdev->dev, &tegra_devfreq_profile,
> + "tegra_actmon", NULL);
> if (IS_ERR(tegra->devfreq)) {
Have to check 'devfreq' instead of 'tegra->devfreq'.
Did you test it? It might be failed because 'tegra->devfreq is NULL.
> err = PTR_ERR(tegra->devfreq);
ditto.
> goto remove_governor;
> }
>
> - err = devm_request_threaded_irq(&pdev->dev, tegra->irq, NULL,
> - actmon_thread_isr, IRQF_ONESHOT,
> - "tegra-devfreq", tegra);
> - if (err) {
> - dev_err(&pdev->dev, "Interrupt request failed: %d\n", err);
> - goto remove_devfreq;
> - }
> -
> return 0;
>
> -remove_devfreq:
> - devfreq_remove_device(tegra->devfreq);
> -
> remove_governor:
> devfreq_remove_governor(&tegra_devfreq_governor);
>
>
--
Best Regards,
Chanwoo Choi
Samsung Electronics
^ permalink raw reply
* Re: [PATCH v2 2/4] devfreq: exynos-bus: convert to use dev_pm_opp_set_rate()
From: Bartlomiej Zolnierkiewicz @ 2019-07-16 11:39 UTC (permalink / raw)
To: Chanwoo Choi
Cc: Kamil Konieczny, Marek Szyprowski, Krzysztof Kozlowski,
Kukjin Kim, Kyungmin Park, Mark Rutland, MyungJoo Ham,
Nishanth Menon, Rob Herring, Stephen Boyd, Viresh Kumar,
devicetree, linux-arm-kernel, linux-kernel, linux-pm,
linux-samsung-soc
In-Reply-To: <29cfafc4-ee22-6d38-4c67-776c48bfed8a@samsung.com>
On 7/16/19 1:26 PM, Chanwoo Choi wrote:
> Hi,
>
> On 19. 7. 16. 오후 7:59, Bartlomiej Zolnierkiewicz wrote:
>>
>> On 7/16/19 12:33 PM, Chanwoo Choi wrote:
>>> Hi Bartlomiej,
>>>
>>> On 19. 7. 16. 오후 7:13, Bartlomiej Zolnierkiewicz wrote:
>>>>
>>>> Hi Chanwoo,
>>>>
>>>> On 7/16/19 5:56 AM, Chanwoo Choi wrote:
>>>>> Hi Kamil,
>>>>>
>>>>> Looks good to me. But, this patch has some issue.
>>>>> I added the detailed reviews.
>>>>>
>>>>> I recommend that you make the separate patches as following
>>>>> in order to clarify the role of which apply the dev_pm_opp_* function.
>>>>>
>>>>> First patch,
>>>>> Need to consolidate the following two function into one function.
>>>>> because the original exynos-bus.c has the problem that the regulator
>>>>> of parent devfreq device have to be enabled before enabling the clock.
>>>>> This issue did not happen because bootloader enables the bus-related
>>>>> regulators before kernel booting.
>>>>> - exynos_bus_parse_of()
>>>>> - exynos_bus_parent_parse_of()
>>>>>> Second patch,
>>>>> Apply dev_pm_opp_set_regulators() and dev_pm_opp_set_rate()
>>>>>
>>>>>
>>>>> On 19. 7. 15. 오후 9:04, Kamil Konieczny wrote:
>>>>>> Reuse opp core code for setting bus clock and voltage. As a side
>>>>>> effect this allow useage of coupled regulators feature (required
>>>>>> for boards using Exynos5422/5800 SoCs) because dev_pm_opp_set_rate()
>>>>>> uses regulator_set_voltage_triplet() for setting regulator voltage
>>>>>> while the old code used regulator_set_voltage_tol() with fixed
>>>>>> tolerance. This patch also removes no longer needed parsing of DT
>>>>>> property "exynos,voltage-tolerance" (no Exynos devfreq DT node uses
>>>>>> it).
>>>>>>
>>>>>> Signed-off-by: Kamil Konieczny <k.konieczny@partner.samsung.com>
>>>>>> ---
>>>>>> drivers/devfreq/exynos-bus.c | 172 ++++++++++++++---------------------
>>>>>> 1 file changed, 66 insertions(+), 106 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
>>>>>> index 486cc5b422f1..7fc4f76bd848 100644
>>>>>> --- a/drivers/devfreq/exynos-bus.c
>>>>>> +++ b/drivers/devfreq/exynos-bus.c
>>>>>> @@ -25,7 +25,6 @@
>>>>>> #include <linux/slab.h>
>>>>>>
>>>>>> #define DEFAULT_SATURATION_RATIO 40
>>>>>> -#define DEFAULT_VOLTAGE_TOLERANCE 2
>>>>>>
>>>>>> struct exynos_bus {
>>>>>> struct device *dev;
>>>>>> @@ -37,9 +36,9 @@ struct exynos_bus {
>>>>>>
>>>>>> unsigned long curr_freq;
>>>>>>
>>>>>> - struct regulator *regulator;
>>>>>> + struct opp_table *opp_table;
>>>>>> +
>>>>>> struct clk *clk;
>>>>>> - unsigned int voltage_tolerance;
>>>>>> unsigned int ratio;
>>>>>> };
>>>>>>
>>>>>> @@ -99,56 +98,25 @@ static int exynos_bus_target(struct device *dev, unsigned long *freq, u32 flags)
>>>>>> {
>>>>>> struct exynos_bus *bus = dev_get_drvdata(dev);
>>>>>> struct dev_pm_opp *new_opp;
>>>>>> - unsigned long old_freq, new_freq, new_volt, tol;
>>>>>> int ret = 0;
>>>>>> -
>>>>>> - /* Get new opp-bus instance according to new bus clock */
>>>>>> + /*
>>>>>> + * New frequency for bus may not be exactly matched to opp, adjust
>>>>>> + * *freq to correct value.
>>>>>> + */
>>>>>
>>>>> You better to change this comment with following styles
>>>>> to keep the consistency:
>>>>>
>>>>> /* Get correct frequency for bus ... */
>>>>>
>>>>>> new_opp = devfreq_recommended_opp(dev, freq, flags);
>>>>>> if (IS_ERR(new_opp)) {
>>>>>> dev_err(dev, "failed to get recommended opp instance\n");
>>>>>> return PTR_ERR(new_opp);
>>>>>> }
>>>>>>
>>>>>> - new_freq = dev_pm_opp_get_freq(new_opp);
>>>>>> - new_volt = dev_pm_opp_get_voltage(new_opp);
>>>>>> dev_pm_opp_put(new_opp);
>>>>>>
>>>>>> - old_freq = bus->curr_freq;
>>>>>> -
>>>>>> - if (old_freq == new_freq)
>>>>>> - return 0;
>>>>>> - tol = new_volt * bus->voltage_tolerance / 100;
>>>>>> -
>>>>>> /* Change voltage and frequency according to new OPP level */
>>>>>> mutex_lock(&bus->lock);
>>>>>> + ret = dev_pm_opp_set_rate(dev, *freq);
>>>>>> + if (!ret)
>>>>>> + bus->curr_freq = *freq;
>>>>>
>>>>> Have to print the error log if ret has minus error value.
>>>>
>>>> dev_pm_opp_set_rate() should print the error message on all
>>>> errors so wouldn't printing the error log also here be superfluous?
>>>>
>>>> [ Please also note that the other user of dev_pm_opp_set_rate()
>>>> (cpufreq-dt cpufreq driver) doesn't do this. ]
>>>
>>> OK. Thanks for the explanation.
>>>
>>>>
>>>>> Modify it as following:
>>>>>
>>>>> if (ret < 0) {
>>>>> dev_err(dev, "failed to set bus rate\n");
>>>>> goto err:
>>>>> }
>>>>> bus->curr_freq = *freq;
>>>>>
>>>>> err:
>>>>> mutex_unlock(&bus->lock);
>>>>>
>>>>> return ret;
>>>>>
>>>>>>
>>>>>> - if (old_freq < new_freq) {
>>>>>> - ret = regulator_set_voltage_tol(bus->regulator, new_volt, tol);
>>>>>> - if (ret < 0) {
>>>>>> - dev_err(bus->dev, "failed to set voltage\n");
>>>>>> - goto out;
>>>>>> - }
>>>>>> - }
>>>>>> -
>>>>>> - ret = clk_set_rate(bus->clk, new_freq);
>>>>>> - if (ret < 0) {
>>>>>> - dev_err(dev, "failed to change clock of bus\n");
>>>>>> - clk_set_rate(bus->clk, old_freq);
>>>>>> - goto out;
>>>>>> - }
>>>>>> -
>>>>>> - if (old_freq > new_freq) {
>>>>>> - ret = regulator_set_voltage_tol(bus->regulator, new_volt, tol);
>>>>>> - if (ret < 0) {
>>>>>> - dev_err(bus->dev, "failed to set voltage\n");
>>>>>> - goto out;
>>>>>> - }
>>>>>> - }
>>>>>> - bus->curr_freq = new_freq;
>>>>>> -
>>>>>> - dev_dbg(dev, "Set the frequency of bus (%luHz -> %luHz, %luHz)\n",
>>>>>> - old_freq, new_freq, clk_get_rate(bus->clk));
>>>>>> -out:
>>>>>> mutex_unlock(&bus->lock);
>>>>>>
>>>>>> return ret;
>>>>>> @@ -194,10 +162,11 @@ static void exynos_bus_exit(struct device *dev)
>>>>>> if (ret < 0)
>>>>>> dev_warn(dev, "failed to disable the devfreq-event devices\n");
>>>>>>
>>>>>> - if (bus->regulator)
>>>>>> - regulator_disable(bus->regulator);
>>>>>> + if (bus->opp_table)
>>>>>> + dev_pm_opp_put_regulators(bus->opp_table);
>>>>>
>>>>> Have to disable regulator after disabling the clock
>>>>> to prevent the h/w fault.
>>>>>
>>>>> I think that you should call them with following sequence:
>>>>>
>>>>> clk_disable_unprepare(bus->clk);
>>>>> if (bus->opp_table)
>>>>> dev_pm_opp_put_regulators(bus->opp_table);
>>>>> dev_pm_opp_of_remove_table(dev);
>>>>>
>>>>>>
>>>>>> dev_pm_opp_of_remove_table(dev);
>>>>>> +
>>>>>> clk_disable_unprepare(bus->clk);
>>>>>> }
>>>>>>
>>>>>> @@ -209,39 +178,26 @@ static int exynos_bus_passive_target(struct device *dev, unsigned long *freq,
>>>>>> {
>>>>>> struct exynos_bus *bus = dev_get_drvdata(dev);
>>>>>> struct dev_pm_opp *new_opp;
>>>>>> - unsigned long old_freq, new_freq;
>>>>>> - int ret = 0;
>>>>>> + int ret;
>>>>>>
>>>>>> - /* Get new opp-bus instance according to new bus clock */
>>>>>> + /*
>>>>>> + * New frequency for bus may not be exactly matched to opp, adjust
>>>>>> + * *freq to correct value.
>>>>>> + */
>>>>>
>>>>> You better to change this comment with following styles
>>>>> to keep the consistency:
>>>>>
>>>>> /* Get correct frequency for bus ... */
>>>>>
>>>>>> new_opp = devfreq_recommended_opp(dev, freq, flags);
>>>>>> if (IS_ERR(new_opp)) {
>>>>>> dev_err(dev, "failed to get recommended opp instance\n");
>>>>>> return PTR_ERR(new_opp);
>>>>>> }
>>>>>>
>>>>>> - new_freq = dev_pm_opp_get_freq(new_opp);
>>>>>> dev_pm_opp_put(new_opp);
>>>>>>
>>>>>> - old_freq = bus->curr_freq;
>>>>>> -
>>>>>> - if (old_freq == new_freq)
>>>>>> - return 0;
>>>>>> -
>>>>>> /* Change the frequency according to new OPP level */
>>>>>> mutex_lock(&bus->lock);
>>>>>> + ret = dev_pm_opp_set_rate(dev, *freq);
>>>>>> + if (!ret)
>>>>>> + bus->curr_freq = *freq;
>>>>>
>>>>> ditto. Have to print the error log, check above comment.
>>>>>
>>>>>>
>>>>>> - ret = clk_set_rate(bus->clk, new_freq);
>>>>>> - if (ret < 0) {
>>>>>> - dev_err(dev, "failed to set the clock of bus\n");
>>>>>> - goto out;
>>>>>> - }
>>>>>> -
>>>>>> - *freq = new_freq;
>>>>>> - bus->curr_freq = new_freq;
>>>>>> -
>>>>>> - dev_dbg(dev, "Set the frequency of bus (%luHz -> %luHz, %luHz)\n",
>>>>>> - old_freq, new_freq, clk_get_rate(bus->clk));
>>>>>> -out:
>>>>>> mutex_unlock(&bus->lock);
>>>>>>
>>>>>> return ret;
>>>>>> @@ -259,20 +215,7 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
>>>>>> struct exynos_bus *bus)
>>>>>> {
>>>>>> struct device *dev = bus->dev;
>>>>>> - int i, ret, count, size;
>>>>>> -
>>>>>> - /* Get the regulator to provide each bus with the power */
>>>>>> - bus->regulator = devm_regulator_get(dev, "vdd");
>>>>>> - if (IS_ERR(bus->regulator)) {
>>>>>> - dev_err(dev, "failed to get VDD regulator\n");
>>>>>> - return PTR_ERR(bus->regulator);
>>>>>> - }
>>>>>> -
>>>>>> - ret = regulator_enable(bus->regulator);
>>>>>> - if (ret < 0) {
>>>>>> - dev_err(dev, "failed to enable VDD regulator\n");
>>>>>> - return ret;
>>>>>> - }
>>>>>> + int i, count, size;
>>>>>>
>>>>>> /*
>>>>>> * Get the devfreq-event devices to get the current utilization of
>>>>>> @@ -281,24 +224,20 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
>>>>>> count = devfreq_event_get_edev_count(dev);
>>>>>> if (count < 0) {
>>>>>> dev_err(dev, "failed to get the count of devfreq-event dev\n");
>>>>>> - ret = count;
>>>>>> - goto err_regulator;
>>>>>> + return count;
>>>>>> }
>>>>>> +
>>>>>> bus->edev_count = count;
>>>>>>
>>>>>> size = sizeof(*bus->edev) * count;
>>>>>> bus->edev = devm_kzalloc(dev, size, GFP_KERNEL);
>>>>>> - if (!bus->edev) {
>>>>>> - ret = -ENOMEM;
>>>>>> - goto err_regulator;
>>>>>> - }
>>>>>> + if (!bus->edev)
>>>>>> + return -ENOMEM;
>>>>>>
>>>>>> for (i = 0; i < count; i++) {
>>>>>> bus->edev[i] = devfreq_event_get_edev_by_phandle(dev, i);
>>>>>> - if (IS_ERR(bus->edev[i])) {
>>>>>> - ret = -EPROBE_DEFER;
>>>>>> - goto err_regulator;
>>>>>> - }
>>>>>> + if (IS_ERR(bus->edev[i]))
>>>>>> + return -EPROBE_DEFER;
>>>>>> }
>>>>>>
>>>>>> /*
>>>>>> @@ -314,22 +253,15 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
>>>>>> if (of_property_read_u32(np, "exynos,saturation-ratio", &bus->ratio))
>>>>>> bus->ratio = DEFAULT_SATURATION_RATIO;
>>>>>>
>>>>>> - if (of_property_read_u32(np, "exynos,voltage-tolerance",
>>>>>> - &bus->voltage_tolerance))
>>>>>> - bus->voltage_tolerance = DEFAULT_VOLTAGE_TOLERANCE;
>>>>>> -
>>>>>> return 0;
>>>>>> -
>>>>>> -err_regulator:
>>>>>> - regulator_disable(bus->regulator);
>>>>>> -
>>>>>> - return ret;
>>>>>> }
>>>>>>
>>>>>> static int exynos_bus_parse_of(struct device_node *np,
>>>>>> - struct exynos_bus *bus)
>>>>>> + struct exynos_bus *bus, bool passive)
>>>>>> {
>>>>>> struct device *dev = bus->dev;
>>>>>> + struct opp_table *opp_table;
>>>>>> + const char *vdd = "vdd";
>>>>>> struct dev_pm_opp *opp;
>>>>>> unsigned long rate;
>>>>>> int ret;
>>>>>> @@ -347,11 +279,22 @@ static int exynos_bus_parse_of(struct device_node *np,
>>>>>> return ret;
>>>>>> }
>>>>>>
>>>>>> + if (!passive) {
>>>>>> + opp_table = dev_pm_opp_set_regulators(dev, &vdd, 1);
>>>>>> + if (IS_ERR(opp_table)) {
>>>>>> + ret = PTR_ERR(opp_table);
>>>>>> + dev_err(dev, "failed to set regulators %d\n", ret);
>>>>>> + goto err_clk;/
>>>>>> + }
>>>>>> +
>>>>>> + bus->opp_table = opp_table;
>>>>>> + }
>>>>>
>>>>> This driver has exynos_bus_parent_parse_of() function for parent devfreq device.
>>>>> dev_pm_opp_set_regulators() have to be called in exynos_bus_parent_parse_of()
>>>>> because the regulator is only used by parent devfreq device.
>>>>
>>>> exynos_bus_parse_of() is called for all devfreq devices (including
>>>> parent) and (as you've noticed) the regulator should be enabled before
>>>> enabling clock (which is done in exynos_bus_parse_of()) so adding
>>>> extra argument to exynos_bus_parse_of() (like it is done currently in
>>>> the patch)
>>>
>>> I think that this patch has still the problem about call sequence
>>> between clock and regulator as following:
>>
>> Yes, this should be fixed (though the wrong sequence between regulator
>> and clock handling is not introduced by the patchset itself and is present
>> in the original driver code).
>>
>>> 273 ret = clk_prepare_enable(bus->clk);
>>> 274 if (ret < 0) {
>>> 275 dev_err(dev, "failed to get enable clock\n");
>>> 276 return ret;
>>> 277 }
>>> 278
>>> 279 if (!passive) {
>>> 280 opp_table = dev_pm_opp_set_regulators(dev, &vdd, 1);
>>> 281 if (IS_ERR(opp_table)) {
>>> 282 ret = PTR_ERR(opp_table);
>>> 283 dev_err(dev, "failed to set regulators %d\n", ret);
>>> 284 goto err_clk;
>>> 285 }
>>> 286
>>> 287 bus->opp_table = opp_table;
>>> 288 }
>>>
>>> makes it possible to do the setup correctly without the need
>>>> of merging both functions into one huge function (which would be more
>>>> difficult to follow than two simpler functions IMHO). Is that approach
>>>> acceptable or do you prefer one big function?
>>>
>>> Actually, I don't force to make one function for both
>>> exynos_bus_parse_of() and exynos_bus_parent_parse_of().
>>>
>>> If we just keep this code, dev_pm_opp_set_regulators()
>>> should be handled in exynos_bus_parent_parse_of()
>>> because only parent devfreq device controls the regulator.
>>
>> Could your please explain rationale for this requirement (besides
>> function name)?
>
> OK. I hope to satisfy the following requirements:
>
> 1. Fix the sequence problem between clock and regulator for enabling them.
> 2. dev_pm_opp_set_regulator() have to be handled in exynos_bus_parent_parse_of()
> instead of exynos_bus_parse_of() for only parent devfreq device.
> 3. exynos_bus_parse_of() have to handle the only common properties
> of both parent devfreq device and passive devfreq device.
>
>>
>> The patch adds 'bool passive' argument (which is set to false for
>> parent devfreq device and true for child devfreq device) to
>> exynos_bus_parse_of() (which is called for *all* devfreq devices
>
> As I menteiond, exynos_bus_parse_of have to handle the only common
> properties of both parent device and passive device.
>
> I gathered the properties for parent device into exynos_bus_parent_parse_of()
> This way using 'bool passive' argument is not proper in exynos_bus_parse_of().
>
>
>> and is called before exynos_bus_parent_parse_of()) and there is
>> no hard requirement to call dev_pm_opp_set_regulators() in
>> exynos_bus_parent_parse_of() so after only changing the ordering
>> between regulator and clock handling the setup code should be
>> correct.
>>
>> [ Please note that this patch moves parent/child detection before
>> exynos_bus_parse_of() call. ]
>>
>>> In order to keep the two functions, maybe have to change
>>> the call the sequence between exynos_bus_parse_of() and
>>> exynos_bus_parent_parse_of().
>>
>> Doesn't seem to be needed, care to explain it more?
>
> In order to fix the sequence problem between clock and regulator
> with dev_pm_opp_set_regualtor() and want to keep two functions
> (exynos_bus_parent_parse_of() and exynos_bus_parse_of()),
> have to change the call order as following and then modify
> the exception handling code when error happen.
>
> node = of_parse_phandle(dev->of_node, "devfreq", 0);
> if (node) {
> of_node_put(node);
> passive = true
> }
>
> if (!passive)
> exynos_bus_parent_parse_of()
> dev_pm_opp_set_regulator
>
> exynos_bus_parse_of()
OK. This seems like a solution.
Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics
>>
>>> Once again, I don't force any fixed method. I want to fix them
>>> with correct way.
>>>
>>>>
>>>>>> +
>>>>>> /* Get the freq and voltage from OPP table to scale the bus freq */
>>>>>> ret = dev_pm_opp_of_add_table(dev);
>>>>>> if (ret < 0) {
>>>>>> dev_err(dev, "failed to get OPP table\n");
>>>>>> - goto err_clk;
>>>>>> + goto err_regulator;
>>>>>> }
>>>>>>
>>>>>> rate = clk_get_rate(bus->clk);
>>>>>> @@ -362,6 +305,7 @@ static int exynos_bus_parse_of(struct device_node *np,
>>>>>> ret = PTR_ERR(opp);
>>>>>> goto err_opp;
>>>>>> }
>>>>>> +
>>>>>> bus->curr_freq = dev_pm_opp_get_freq(opp);
>>>>>> dev_pm_opp_put(opp);
>>>>>>
>>>>>> @@ -369,6 +313,13 @@ static int exynos_bus_parse_of(struct device_node *np,
>>>>>>
>>>>>> err_opp:
>>>>>> dev_pm_opp_of_remove_table(dev);
>>>>>> +
>>>>>> +err_regulator:
>>>>>> + if (bus->opp_table) {
>>>>>> + dev_pm_opp_put_regulators(bus->opp_table);
>>>>>> + bus->opp_table = NULL;
>>>>>> + }
>>>>>
>>>>> As I mentioned above, it it wrong to call dev_pm_opp_put_regulators()
>>>>> after removing the opp_table by dev_pm_opp_of_remove_table().
>>>>>
>>>>>> +
>>>>>> err_clk:
>>>>>> clk_disable_unprepare(bus->clk);
>>>>>>
>>>>>> @@ -386,6 +337,7 @@ static int exynos_bus_probe(struct platform_device *pdev)
>>>>>> struct exynos_bus *bus;
>>>>>> int ret, max_state;
>>>>>> unsigned long min_freq, max_freq;
>>>>>> + bool passive = false;
>>>>>>
>>>>>> if (!np) {
>>>>>> dev_err(dev, "failed to find devicetree node\n");
>>>>>> @@ -395,12 +347,18 @@ static int exynos_bus_probe(struct platform_device *pdev)
>>>>>> bus = devm_kzalloc(&pdev->dev, sizeof(*bus), GFP_KERNEL);
>>>>>> if (!bus)
>>>>>> return -ENOMEM;
>>>>>> +
>>>>>> mutex_init(&bus->lock);
>>>>>> bus->dev = &pdev->dev;
>>>>>> platform_set_drvdata(pdev, bus);
>>>>>> + node = of_parse_phandle(dev->of_node, "devfreq", 0);
>>>>>> + if (node) {
>>>>>> + of_node_put(node);
>>>>>> + passive = true;
>>>>>> + }
>>>>>>
>>>>>> /* Parse the device-tree to get the resource information */
>>>>>> - ret = exynos_bus_parse_of(np, bus);
>>>>>> + ret = exynos_bus_parse_of(np, bus, passive);
>>>>>> if (ret < 0)
>>>>>> return ret;
>>>>>>
>>>>>> @@ -410,13 +368,10 @@ static int exynos_bus_probe(struct platform_device *pdev)
>>>>>> goto err;
>>>>>> }
>>>>>>
>>>>>> - node = of_parse_phandle(dev->of_node, "devfreq", 0);
>>>>>> - if (node) {
>>>>>> - of_node_put(node);
>>>>>> + if (passive)
>>>>>> goto passive;
>>>>>> - } else {
>>>>>> - ret = exynos_bus_parent_parse_of(np, bus);
>>>>>> - }
>>>>>> +
>>>>>> + ret = exynos_bus_parent_parse_of(np, bus);
>>>>>>
>>>>>
>>>>> Remove unneeded blank line.
>>>>>
>>>>>> if (ret < 0)
>>>>>> goto err;
>>>>>> @@ -509,6 +464,11 @@ static int exynos_bus_probe(struct platform_device *pdev)
>>>>>>
>>>>>> err:
>>>>>> dev_pm_opp_of_remove_table(dev);
>>>>>> + if (bus->opp_table) {
>>>>>> + dev_pm_opp_put_regulators(bus->opp_table);
>>>>>> + bus->opp_table = NULL;
>>>>>> + }
>>>>>> +
>>>>>
>>>>> ditto.
>>>>> Have to disable regulator after disabling the clock
>>>>> to prevent the h/w fault.
>>>>>
>>>>> I think that you should call them with following sequence:
>>>>>
>>>>> clk_disable_unprepare(bus->clk);
>>>>> if (bus->opp_table)
>>>>> dev_pm_opp_put_regulators(bus->opp_table);
>>>>> dev_pm_opp_of_remove_table(dev);
>>>>>
>>>>>> clk_disable_unprepare(bus->clk);
>>>>>>
>>>>>> return ret;
>>>>
>>>> Best regards,
>>>> --
>>>> Bartlomiej Zolnierkiewicz
>>>> Samsung R&D Institute Poland
>>>> Samsung Electronics
>>
>> Best regards,
>> --
>> Bartlomiej Zolnierkiewicz
>> Samsung R&D Institute Poland
>> Samsung Electronics
^ permalink raw reply
* Re: [PATCH v4 01/24] PM / devfreq: tegra30: Change irq type to unsigned int
From: Chanwoo Choi @ 2019-07-16 11:35 UTC (permalink / raw)
To: Dmitry Osipenko, Thierry Reding, MyungJoo Ham, Kyungmin Park,
Jonathan Hunter, Tomeu Vizoso
Cc: linux-pm, linux-tegra, linux-kernel
In-Reply-To: <20190707223303.6755-2-digetx@gmail.com>
Hi Dmitry,
On 19. 7. 8. 오전 7:32, Dmitry Osipenko wrote:
> IRQ numbers are always positive, hence the corresponding variable should
> be unsigned to keep types consistent. This is a minor change that cleans
> up code a tad more.
>
> Suggested-by: Thierry Reding <thierry.reding@gmail.com>
> Acked-by: MyungJoo Ham <myungjoo.ham@samsung.com>
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
> drivers/devfreq/tegra30-devfreq.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
> index a6ba75f4106d..a27300f40b0b 100644
> --- a/drivers/devfreq/tegra30-devfreq.c
> +++ b/drivers/devfreq/tegra30-devfreq.c
> @@ -160,7 +160,7 @@ struct tegra_devfreq {
>
> struct tegra_devfreq_device devices[ARRAY_SIZE(actmon_device_configs)];
>
> - int irq;
> + unsigned int irq;
> };
>
> struct tegra_actmon_emc_ratio {
> @@ -618,12 +618,12 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
> return PTR_ERR(tegra->emc_clock);
> }
>
> - tegra->irq = platform_get_irq(pdev, 0);
> - if (tegra->irq < 0) {
> - err = tegra->irq;
> + err = platform_get_irq(pdev, 0);
> + if (err < 0) {
> dev_err(&pdev->dev, "Failed to get IRQ: %d\n", err);
> return err;
> }
> + tegra->irq = err;
>
> reset_control_assert(tegra->reset);
>
>
Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
--
Best Regards,
Chanwoo Choi
Samsung Electronics
^ 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