Linux Power Management development
 help / color / mirror / Atom feed
* [GIT PULL] OPP changes for 5.3
From: Viresh Kumar @ 2019-06-26  7:20 UTC (permalink / raw)
  To: Rafael J. Wysocki; +Cc: Linux PM

Hi Rafael,

This pull request contains:

- OPP core changes to support a wider range of devices, like IO
  devices (Rajendra Nayak and Stehpen Boyd).
- Fixes around genpd_virt_devs (Viresh Kumar).
- Fix for platform with set_opp() callback (Dmitry Osipenko).

--
viresh

-------------------------8<-------------------------

The following changes since commit a188339ca5a396acc588e5851ed7e19f66b0ebd9:

  Linux 5.2-rc1 (2019-05-19 15:47:09 -0700)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/vireshk/pm.git opp/linux-next

for you to fetch changes up to 560d1bcad715c215e7ffe5d7cffe045974b623d0:

  opp: Don't use IS_ERR on invalid supplies (2019-06-24 12:48:29 +0530)

----------------------------------------------------------------
Dmitry Osipenko (1):
      opp: Don't use IS_ERR on invalid supplies

Rajendra Nayak (1):
      opp: Make dev_pm_opp_set_rate() handle freq = 0 to drop performance votes

Stephen Boyd (1):
      opp: Don't overwrite rounded clk rate

Viresh Kumar (2):
      opp: Attach genpds to devices from within OPP core
      opp: Allocate genpd_virt_devs from dev_pm_opp_attach_genpd()

 drivers/opp/core.c     | 176 ++++++++++++++++++++++++++++++++-----------------
 drivers/opp/of.c       |  30 +--------
 include/linux/pm_opp.h |   8 +--
 3 files changed, 122 insertions(+), 92 deletions(-)


^ permalink raw reply

* [GIT PULL] cpufreq/arm changes for 5.3
From: Viresh Kumar @ 2019-06-26  7:16 UTC (permalink / raw)
  To: Rafael J. Wysocki; +Cc: Linux PM

Hi Rafael,

This pull request contains:

- Minor fixes for brcmstb driver (Florian).
- New imx-cpufreq driver, its bindings and code around it (Leonard).
- New Raspberry Pi driver (Nicolas).
- Minor fix for s5pv210 driver (Pawel).
- Minor cleanup for armada driver (YueHaibing).

--
viresh

-------------------------8<-------------------------
The following changes since commit a188339ca5a396acc588e5851ed7e19f66b0ebd9:

  Linux 5.2-rc1 (2019-05-19 15:47:09 -0700)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/vireshk/pm.git cpufreq/arm/linux-next

for you to fetch changes up to f9020441dbc39133591ff72b420f21f51896afc5:

  cpufreq: s5pv210: Don't flood kernel log after cpufreq change (2019-06-24 10:27:02 +0530)

----------------------------------------------------------------
Florian Fainelli (2):
      cpufreq: brcmstb-avs-cpufreq: Fix initial command check
      cpufreq: brcmstb-avs-cpufreq: Fix types for voltage/frequency

Leonard Crestez (5):
      cpufreq: Add imx-cpufreq-dt driver
      dt-bindings: imx-cpufreq-dt: Document opp-supported-hw usage
      cpufreq: imx-cpufreq-dt: Fix no OPPs available on unfused parts
      cpufreq: imx-cpufreq-dt: Remove global platform match list
      cpufreq: Switch imx7d to imx-cpufreq-dt for speed grading

Nicolas Saenz Julienne (1):
      cpufreq: add driver for Raspberry Pi

Paweł Chmiel (1):
      cpufreq: s5pv210: Don't flood kernel log after cpufreq change

YueHaibing (1):
      cpufreq: armada-37xx: Remove set but not used variable 'freq'

 .../devicetree/bindings/cpufreq/imx-cpufreq-dt.txt | 37 +++++++++
 drivers/cpufreq/Kconfig.arm                        | 17 ++++
 drivers/cpufreq/Makefile                           |  2 +
 drivers/cpufreq/armada-37xx-cpufreq.c              |  4 +-
 drivers/cpufreq/brcmstb-avs-cpufreq.c              | 12 +--
 drivers/cpufreq/cpufreq-dt-platdev.c               |  5 +-
 drivers/cpufreq/imx-cpufreq-dt.c                   | 97 ++++++++++++++++++++++
 drivers/cpufreq/raspberrypi-cpufreq.c              | 97 ++++++++++++++++++++++
 drivers/cpufreq/s5pv210-cpufreq.c                  |  2 +-
 drivers/soc/imx/soc-imx8.c                         |  3 +
 10 files changed, 265 insertions(+), 11 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/cpufreq/imx-cpufreq-dt.txt
 create mode 100644 drivers/cpufreq/imx-cpufreq-dt.c
 create mode 100644 drivers/cpufreq/raspberrypi-cpufreq.c


^ permalink raw reply

* Re: [PATCH V3 2/3] thermal/drivers/cpu_cooling: Unregister with the policy
From: Viresh Kumar @ 2019-06-26  6:37 UTC (permalink / raw)
  To: Daniel Lezcano
  Cc: rjw, edubezval, linux-kernel, Sudeep Holla, Amit Daniel Kachhap,
	Javi Merino, Zhang Rui, Shawn Guo, Sascha Hauer,
	Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team, Keerthy,
	open list:CPU FREQUENCY DRIVERS - ARM BIG LITTLE,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
	open list:TI BANDGAP AND THERMAL DRIVER
In-Reply-To: <da1d2603-e30a-d877-54c3-1fad218f9d57@linaro.org>

On 26-06-19, 08:02, Daniel Lezcano wrote:
> On 26/06/2019 04:58, Viresh Kumar wrote:
> > On 25-06-19, 13:32, Daniel Lezcano wrote:
> >> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> >> index aee024e42618..f07454249fbc 100644
> >> --- a/drivers/cpufreq/cpufreq.c
> >> +++ b/drivers/cpufreq/cpufreq.c
> >> @@ -1379,8 +1379,8 @@ static int cpufreq_online(unsigned int cpu)
> >>  		cpufreq_driver->ready(policy);
> >>  
> >>  	if (cpufreq_thermal_control_enabled(cpufreq_driver))
> >> -		policy->cdev = of_cpufreq_cooling_register(policy);
> >> -
> >> +		of_cpufreq_cooling_register(policy);
> >> +	
> > 
> > We don't need any error checking here anymore ?
> 
> There was no error checking initially. This comment and the others below
> are for an additional patch IMO, not a change in this one.

right, but ...

> >> -void cpufreq_cooling_unregister(struct thermal_cooling_device *cdev)
> >> +void cpufreq_cooling_unregister(struct cpufreq_policy *policy)
> >>  {
> >>  	struct cpufreq_cooling_device *cpufreq_cdev;
> >>  	bool last;
> >>  
> >> -	if (!cdev)
> >> -		return;

we used to return without any errors from here. Now we will have
problems if regsitering fails for some reason.

-- 
viresh

^ permalink raw reply

* Re: [PATCH v1 0/3] Add required-opps support to devfreq passive gov
From: Viresh Kumar @ 2019-06-26  6:32 UTC (permalink / raw)
  To: Saravana Kannan
  Cc: MyungJoo Ham, Kyungmin Park, Chanwoo Choi, Viresh Kumar,
	Nishanth Menon, Stephen Boyd, Rafael J. Wysocki,
	Android Kernel Team, Linux PM, LKML
In-Reply-To: <CAGETcx_v05PfscMi2qiYwHRMLryyA_494+h+kmJ3mD+GOjjeLA@mail.gmail.com>

On 24-06-19, 22:29, Saravana Kannan wrote:
> No, the CPUs will be the "parent" and the cache will be the "child".
> CPU is a special case when it comes to the actual software (not DT) as
> we'll need the devfreq governor to look at all the CPUfreq policies to
> decide the cache frequency (max of all their requirements).
> 
> I think "master" and "slave" would have been a better term as the
> master device determines its frequency using whatever means and the
> "slave" device just "follows" the master device.

Okay, so to confirm again this is what we will have:

- CPUs are called masters and Caches are slaves.

- The devfreq governor we are talking about takes care of changing
  frequency of caches (slaves) and chooses a target frequency for
  caches based on what the masters are running at.

- The CPUs OPP nodes will have required-opps property and will be
  pointing to the caches OPP nodes. The CPUs may already be using
  required-opps node for PM domain performance state thing.


Now the problem is "required-opp" means something really *required*
and it is not optional. Like a specific voltage level before we can
switch to a particular frequency. And this is how I have though of it
until now. And this shouldn't be handled asynchronously, i.e. required
OPPs must be set while configuring OPP of a device.

So, when a CPU changes frequency, we must change the performance state
of PM domain and change frequency/bw of the cache synchronously. And
in such a case "required-opps" can be a very good fit for this use
case.

But with what you are trying to do it is no longer required-opp but
good-to-have-opp. And that worries me.

-- 
viresh

^ permalink raw reply

* Re: [PATCH V3 2/3] thermal/drivers/cpu_cooling: Unregister with the policy
From: Daniel Lezcano @ 2019-06-26  6:02 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: rjw, edubezval, linux-kernel, Sudeep Holla, Amit Daniel Kachhap,
	Javi Merino, Zhang Rui, Shawn Guo, Sascha Hauer,
	Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team, Keerthy,
	open list:CPU FREQUENCY DRIVERS - ARM BIG LITTLE,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
	open list:TI BANDGAP AND THERMAL DRIVER
In-Reply-To: <20190626025831.jmyzyypxr6ezpbtu@vireshk-i7>


Hi Viresh,


On 26/06/2019 04:58, Viresh Kumar wrote:
> On 25-06-19, 13:32, Daniel Lezcano wrote:
>> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
>> index aee024e42618..f07454249fbc 100644
>> --- a/drivers/cpufreq/cpufreq.c
>> +++ b/drivers/cpufreq/cpufreq.c
>> @@ -1379,8 +1379,8 @@ static int cpufreq_online(unsigned int cpu)
>>  		cpufreq_driver->ready(policy);
>>  
>>  	if (cpufreq_thermal_control_enabled(cpufreq_driver))
>> -		policy->cdev = of_cpufreq_cooling_register(policy);
>> -
>> +		of_cpufreq_cooling_register(policy);
>> +	
> 
> We don't need any error checking here anymore ?

There was no error checking initially. This comment and the others below
are for an additional patch IMO, not a change in this one.

>>  	pr_debug("initialization complete\n");
>>  
>>  	return 0;
>> @@ -1468,10 +1468,8 @@ static int cpufreq_offline(unsigned int cpu)
>>  		goto unlock;
>>  	}
>>  
>> -	if (cpufreq_thermal_control_enabled(cpufreq_driver)) {
>> -		cpufreq_cooling_unregister(policy->cdev);
>> -		policy->cdev = NULL;
>> -	}
>> +	if (cpufreq_thermal_control_enabled(cpufreq_driver))
>> +		cpufreq_cooling_unregister(policy);
> 
> And we unregister unconditionally, even if we failed ? What if this
> routine prints error messages for such an case ?
>>  
>>  	if (cpufreq_driver->stop_cpu)
>>  		cpufreq_driver->stop_cpu(policy);
>> diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
>> index 83486775e593..007c7c6bf845 100644
>> --- a/drivers/thermal/cpu_cooling.c
>> +++ b/drivers/thermal/cpu_cooling.c
>> @@ -78,6 +78,7 @@ struct cpufreq_cooling_device {
>>  	struct cpufreq_policy *policy;
>>  	struct list_head node;
>>  	struct time_in_idle *idle_time;
>> +	struct thermal_cooling_device *cdev;
>>  };
>>  
>>  static DEFINE_IDA(cpufreq_ida);
>> @@ -606,6 +607,7 @@ __cpufreq_cooling_register(struct device_node *np,
>>  		goto remove_ida;
>>  
>>  	cpufreq_cdev->clipped_freq = get_state_freq(cpufreq_cdev, 0);
>> +	cpufreq_cdev->cdev = cdev;
>>  
>>  	mutex_lock(&cooling_list_lock);
>>  	/* Register the notifier for first cpufreq cooling device */
>> @@ -699,18 +701,18 @@ EXPORT_SYMBOL_GPL(of_cpufreq_cooling_register);
>>   *
>>   * This interface function unregisters the "thermal-cpufreq-%x" cooling device.
>>   */
>> -void cpufreq_cooling_unregister(struct thermal_cooling_device *cdev)
>> +void cpufreq_cooling_unregister(struct cpufreq_policy *policy)
>>  {
>>  	struct cpufreq_cooling_device *cpufreq_cdev;
>>  	bool last;
>>  
>> -	if (!cdev)
>> -		return;
>> -
>> -	cpufreq_cdev = cdev->devdata;
>> -
>>  	mutex_lock(&cooling_list_lock);
>> -	list_del(&cpufreq_cdev->node);
>> +	list_for_each_entry(cpufreq_cdev, &cpufreq_cdev_list, node) {
>> +		if (cpufreq_cdev->policy == policy) {
>> +			list_del(&cpufreq_cdev->node);
>> +			break;
>> +		}
>> +	}
> 
> What if we reach here without a match for the policy ? We shouldn't
> continue and error out, right ? Print an error message as well ?
> 


-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog


^ permalink raw reply

* Re: [PATCH V3 3/3] thermal/drivers/cpu_cooling: cpufreq_cooling_register returns an int
From: Viresh Kumar @ 2019-06-26  2:59 UTC (permalink / raw)
  To: Daniel Lezcano
  Cc: rjw, edubezval, linux-kernel, Amit Daniel Kachhap, Javi Merino,
	Zhang Rui, Shawn Guo, Sascha Hauer, Pengutronix Kernel Team,
	Fabio Estevam, NXP Linux Team, Keerthy,
	open list:THERMAL/CPU_COOLING,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
	open list:TI BANDGAP AND THERMAL DRIVER
In-Reply-To: <20190625113244.18146-3-daniel.lezcano@linaro.org>

On 25-06-19, 13:32, Daniel Lezcano wrote:
> It looks like after the changes in the patch the only reason for

which patch ? Instead of "patch" you should refer to the latest
changes in code, like "now that cpufreq_cooling_unregister() doesn't
accept cdev as an argument ....."

> returning (struct thermal_cooling_device *) from
> cpufreq_cooling_register() is error checking, but it would be much
> more straightforward to return int for this purpose.
> 
> Moreover, that would prevent the callers of it from doing incorrect
> things with the returned pointers (like using it to unregister the
> cooling device).
> 
> Replace the returned value an integer instead of a pointer to a

                             as ?

> thermal cooling device structure.
> 
> Suggested-by: Rafael J. Wysocki <rafael@kernel.org>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  drivers/thermal/cpu_cooling.c                 | 63 +++++++++----------
>  drivers/thermal/imx_thermal.c                 |  6 +-
>  .../ti-soc-thermal/ti-thermal-common.c        |  7 +--
>  include/linux/cpu_cooling.h                   | 16 ++---
>  4 files changed, 40 insertions(+), 52 deletions(-)

-- 
viresh

^ permalink raw reply

* Re: [PATCH V3 2/3] thermal/drivers/cpu_cooling: Unregister with the policy
From: Viresh Kumar @ 2019-06-26  2:58 UTC (permalink / raw)
  To: Daniel Lezcano
  Cc: rjw, edubezval, linux-kernel, Sudeep Holla, Amit Daniel Kachhap,
	Javi Merino, Zhang Rui, Shawn Guo, Sascha Hauer,
	Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team, Keerthy,
	open list:CPU FREQUENCY DRIVERS - ARM BIG LITTLE,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
	open list:TI BANDGAP AND THERMAL DRIVER
In-Reply-To: <20190625113244.18146-2-daniel.lezcano@linaro.org>

On 25-06-19, 13:32, Daniel Lezcano wrote:
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index aee024e42618..f07454249fbc 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -1379,8 +1379,8 @@ static int cpufreq_online(unsigned int cpu)
>  		cpufreq_driver->ready(policy);
>  
>  	if (cpufreq_thermal_control_enabled(cpufreq_driver))
> -		policy->cdev = of_cpufreq_cooling_register(policy);
> -
> +		of_cpufreq_cooling_register(policy);
> +	

We don't need any error checking here anymore ?

>  	pr_debug("initialization complete\n");
>  
>  	return 0;
> @@ -1468,10 +1468,8 @@ static int cpufreq_offline(unsigned int cpu)
>  		goto unlock;
>  	}
>  
> -	if (cpufreq_thermal_control_enabled(cpufreq_driver)) {
> -		cpufreq_cooling_unregister(policy->cdev);
> -		policy->cdev = NULL;
> -	}
> +	if (cpufreq_thermal_control_enabled(cpufreq_driver))
> +		cpufreq_cooling_unregister(policy);

And we unregister unconditionally, even if we failed ? What if this
routine prints error messages for such an case ?

>  
>  	if (cpufreq_driver->stop_cpu)
>  		cpufreq_driver->stop_cpu(policy);
> diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
> index 83486775e593..007c7c6bf845 100644
> --- a/drivers/thermal/cpu_cooling.c
> +++ b/drivers/thermal/cpu_cooling.c
> @@ -78,6 +78,7 @@ struct cpufreq_cooling_device {
>  	struct cpufreq_policy *policy;
>  	struct list_head node;
>  	struct time_in_idle *idle_time;
> +	struct thermal_cooling_device *cdev;
>  };
>  
>  static DEFINE_IDA(cpufreq_ida);
> @@ -606,6 +607,7 @@ __cpufreq_cooling_register(struct device_node *np,
>  		goto remove_ida;
>  
>  	cpufreq_cdev->clipped_freq = get_state_freq(cpufreq_cdev, 0);
> +	cpufreq_cdev->cdev = cdev;
>  
>  	mutex_lock(&cooling_list_lock);
>  	/* Register the notifier for first cpufreq cooling device */
> @@ -699,18 +701,18 @@ EXPORT_SYMBOL_GPL(of_cpufreq_cooling_register);
>   *
>   * This interface function unregisters the "thermal-cpufreq-%x" cooling device.
>   */
> -void cpufreq_cooling_unregister(struct thermal_cooling_device *cdev)
> +void cpufreq_cooling_unregister(struct cpufreq_policy *policy)
>  {
>  	struct cpufreq_cooling_device *cpufreq_cdev;
>  	bool last;
>  
> -	if (!cdev)
> -		return;
> -
> -	cpufreq_cdev = cdev->devdata;
> -
>  	mutex_lock(&cooling_list_lock);
> -	list_del(&cpufreq_cdev->node);
> +	list_for_each_entry(cpufreq_cdev, &cpufreq_cdev_list, node) {
> +		if (cpufreq_cdev->policy == policy) {
> +			list_del(&cpufreq_cdev->node);
> +			break;
> +		}
> +	}

What if we reach here without a match for the policy ? We shouldn't
continue and error out, right ? Print an error message as well ?

-- 
viresh

^ permalink raw reply

* Re: [PATCH] test_power: Add CURRENT and CHARGE_COUNTER properties
From: Lecopzer Chen @ 2019-06-26  2:36 UTC (permalink / raw)
  To: sebastian.reichel; +Cc: linux-pm, linux-kernel, yj.chiang
In-Reply-To: <20190625223537.6dlcooifqfs2rv2b@earth.universe>

Dear Sebastian,

Thanks for your review and helpful suggestion,
Since our platform is relative simple, I just miss the part you refer to.

I will fix them in patch v2.


Thanks,
	Lecopzer

^ permalink raw reply

* Re: [PATCH] test_power: Add CURRENT and CHARGE_COUNTER properties
From: Lecopzer Chen @ 2019-06-26  2:33 UTC (permalink / raw)
  To: sebastian.reichel; +Cc: linux-pm, linux-kernel

Dear Sebastian,

Thanks for your review and helpful suggestion,
Since our platform is relative simple, I just miss the part you refer to.

I will fix them in patch v2.


Thanks,
	Lecopzer

^ permalink raw reply

* Re: [PATCH v2 4/4] PM / devfreq: Add required OPPs support to passive governor
From: Chanwoo Choi @ 2019-06-26  1:59 UTC (permalink / raw)
  To: Saravana Kannan, MyungJoo Ham, Kyungmin Park, Viresh Kumar,
	Nishanth Menon, Stephen Boyd, Rafael J. Wysocki
  Cc: kernel-team, linux-pm, linux-kernel, cpgs (cpgs@samsung.com)
In-Reply-To: <20190625213337.157525-5-saravanak@google.com>

Hi Saravana,

On 19. 6. 26. 오전 6:33, Saravana Kannan wrote:
> Look at the required OPPs of the "parent" device to determine the OPP that
> is required from the slave device managed by the passive governor. This
> allows having mappings between a parent device and a slave device even when
> they don't have the same number of OPPs.
> 
> Signed-off-by: Saravana Kannan <saravanak@google.com>
> ---
>  drivers/devfreq/governor_passive.c | 20 +++++++++++++++-----
>  1 file changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/devfreq/governor_passive.c b/drivers/devfreq/governor_passive.c
> index 3bc29acbd54e..f6de03de7a64 100644
> --- a/drivers/devfreq/governor_passive.c
> +++ b/drivers/devfreq/governor_passive.c
> @@ -22,7 +22,7 @@ static int devfreq_passive_get_target_freq(struct devfreq *devfreq,
>  			= (struct devfreq_passive_data *)devfreq->data;
>  	struct devfreq *parent_devfreq = (struct devfreq *)p_data->parent;
>  	unsigned long child_freq = ULONG_MAX;
> -	struct dev_pm_opp *opp;
> +	struct dev_pm_opp *opp = NULL, *p_opp = NULL;
>  	int i, count, ret = 0;
>  
>  	/*
> @@ -59,13 +59,20 @@ static int devfreq_passive_get_target_freq(struct devfreq *devfreq,
>  	 * list of parent device. Because in this case, *freq is temporary
>  	 * value which is decided by ondemand governor.
>  	 */
> -	opp = devfreq_recommended_opp(parent_devfreq->dev.parent, freq, 0);
> -	if (IS_ERR(opp)) {
> -		ret = PTR_ERR(opp);
> +	p_opp = devfreq_recommended_opp(parent_devfreq->dev.parent, freq, 0);
> +	if (IS_ERR(p_opp)) {
> +		ret = PTR_ERR(p_opp);
>  		goto out;
>  	}
>  
> -	dev_pm_opp_put(opp);
> +	if (devfreq->opp_table && parent_devfreq->opp_table)
> +		opp = dev_pm_opp_xlate_opp(parent_devfreq->opp_table,
> +					   devfreq->opp_table, p_opp);
> +	if (opp) {
> +		*freq = dev_pm_opp_get_freq(opp);
> +		dev_pm_opp_put(opp);
> +		goto out;
> +	}
>  
>  	/*
>  	 * Get the OPP table's index of decided freqeuncy by governor
> @@ -92,6 +99,9 @@ static int devfreq_passive_get_target_freq(struct devfreq *devfreq,
>  	*freq = child_freq;
>  
>  out:
> +	if (!IS_ERR_OR_NULL(opp))
> +		dev_pm_opp_put(p_opp);
> +
>  	return ret;
>  }
>  
> 

I agree this approach. It is necessary.
Acked-by: Chanwoo Choi <cw00.choi@samsung.com>


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

^ permalink raw reply

* Re: [PATCH v2 3/4] PM / devfreq: Cache OPP table reference in devfreq
From: Chanwoo Choi @ 2019-06-26  1:57 UTC (permalink / raw)
  To: Saravana Kannan, MyungJoo Ham, Kyungmin Park, Viresh Kumar,
	Nishanth Menon, Stephen Boyd, Rafael J. Wysocki
  Cc: kernel-team, linux-pm, linux-kernel, cpgs (cpgs@samsung.com)
In-Reply-To: <20190625213337.157525-4-saravanak@google.com>

Hi Saravana,

On 19. 6. 26. 오전 6:33, Saravana Kannan wrote:
> The OPP table can be used often in devfreq. Trying to get it each time can
> be expensive, so cache it in the devfreq struct.
> 
> Signed-off-by: Saravana Kannan <saravanak@google.com>
> ---
>  drivers/devfreq/devfreq.c | 6 ++++++
>  include/linux/devfreq.h   | 1 +
>  2 files changed, 7 insertions(+)
> 
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 6b6991f0e873..ac62b78dc035 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -597,6 +597,8 @@ static void devfreq_dev_release(struct device *dev)
>  	if (devfreq->profile->exit)
>  		devfreq->profile->exit(devfreq->dev.parent);
>  
> +	if (devfreq->opp_table)
> +		dev_pm_opp_put_opp_table(devfreq->opp_table);
>  	mutex_destroy(&devfreq->lock);
>  	kfree(devfreq);
>  }
> @@ -677,6 +679,10 @@ struct devfreq *devfreq_add_device(struct device *dev,
>  	devfreq->max_freq = devfreq->scaling_max_freq;
>  
>  	devfreq->suspend_freq = dev_pm_opp_get_suspend_opp_freq(dev);
> +	devfreq->opp_table = dev_pm_opp_get_opp_table(dev);
> +	if (IS_ERR(devfreq->opp_table))
> +		devfreq->opp_table = NULL;
> +
>  	atomic_set(&devfreq->suspend_count, 0);
>  
>  	dev_set_name(&devfreq->dev, "devfreq%d",
> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
> index fbffa74bfc1b..0d877c9513d7 100644
> --- a/include/linux/devfreq.h
> +++ b/include/linux/devfreq.h
> @@ -156,6 +156,7 @@ struct devfreq {
>  	struct devfreq_dev_profile *profile;
>  	const struct devfreq_governor *governor;
>  	char governor_name[DEVFREQ_NAME_LEN];
> +	struct opp_table *opp_table;
>  	struct notifier_block nb;
>  	struct delayed_work work;
>  
> 

Looks good to me.
Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

^ permalink raw reply

* Re: [PATCH] PM / wakeup: show wakeup sources stats in sysfs
From: Greg KH @ 2019-06-26  1:46 UTC (permalink / raw)
  To: Tri Vo
  Cc: Rafael J. Wysocki, Viresh Kumar, Rafael J. Wysocki,
	Hridya Valsaraju, Sandeep Patil, LKML, Linux PM,
	Cc: Android Kernel
In-Reply-To: <CANA+-vBoabFTD=fMz+0d5Sbe9rPwnxcuxJxaMCT3KAwXYHSD7w@mail.gmail.com>

On Tue, Jun 25, 2019 at 06:33:08PM -0700, Tri Vo wrote:
> On Tue, Jun 25, 2019 at 6:12 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > +static ssize_t wakeup_source_count_show(struct wakeup_source *ws,
> > > +                                     struct wakeup_source_attribute *attr,
> > > +                                     char *buf)
> > > +{
> > > +     unsigned long flags;
> > > +     unsigned long var;
> > > +
> > > +     spin_lock_irqsave(&ws->lock, flags);
> > > +     if (strcmp(attr->attr.name, "active_count") == 0)
> > > +             var = ws->active_count;
> > > +     else if (strcmp(attr->attr.name, "event_count") == 0)
> > > +             var = ws->event_count;
> > > +     else if (strcmp(attr->attr.name, "wakeup_count") == 0)
> > > +             var = ws->wakeup_count;
> > > +     else
> > > +             var = ws->expire_count;
> > > +     spin_unlock_irqrestore(&ws->lock, flags);
> > > +
> > > +     return sprintf(buf, "%lu\n", var);
> > > +}
> >
> > Why is this lock always needed to be grabbed?  You are just reading a
> > value, who cares if it changes inbetween reading it and returning the
> > buffer string as it can change at that point in time anyway?
> 
> Right, we don't care if the value changes in between us reading and
> printing it. However, IIUC not grabbing this lock results in a data
> race, which is undefined behavior.

A data race where?  Writing to the value?  How can that happen?  All you
are doing is incrementing this variable elsewhere, what is the worst
that can happen?

thanks,

greg k-h

^ permalink raw reply

* [PATCH v2 00/17] More improvements for Tegra30 devfreq driver
From: Dmitry Osipenko @ 2019-06-26  1:32 UTC (permalink / raw)
  To: Thierry Reding, MyungJoo Ham, Kyungmin Park, Chanwoo Choi,
	Jonathan Hunter, Tomeu Vizoso
  Cc: linux-pm, linux-tegra, linux-kernel

Hello,

This series addresses some additional review comments that were made by
Thierry Reding to [1] and makes several important changes to the driver,
fixing excessive interrupts activity. In the end I'm proposing myself as
a maintainer for the Tegra devfreq drivers.

[1] https://lore.kernel.org/lkml/0fb50eb1-a173-1756-6889-2526a10ac707@gmail.com/T/

Changelog:

v2:  Added more patches that are cleaning driver's code further and
     squashing another kHz conversion bug.

     The patch "Rework frequency management logic" of the v1 series is now
     converted to "Set up watermarks properly" because I found some problems
     in the original patch and then realized that there is no need to change
     the logic much. So the logic mostly preserved and only got improvements.

     The series is based on the today's linux-next (25 Jun) and takes into
     account minor changes that MyungJoo Ham made to the already queued
     patches from the first batch [1].

Dmitry Osipenko (17):
  PM / devfreq: tegra30: Change irq type to unsigned int
  PM / devfreq: tegra30: Keep interrupt disabled while governor is
    stopped
  PM / devfreq: tegra30: Handle possible round-rate error
  PM / devfreq: tegra30: Drop write-barrier
  PM / devfreq: tegra30: Set up watermarks properly
  PM / devfreq: tegra30: Tuneup boosting thresholds
  PM / devfreq: tegra30: Use CPUFreq notifier
  PM / devfreq: tegra30: Move clk-notifier's registration to governor's
    start
  PM / devfreq: tegra30: Reset boosting on startup
  PM / devfreq: tegra30: Don't enable consecutive-down interrupt on
    startup
  PM / devfreq: tegra30: Add debug messages
  PM / devfreq: tegra30: Inline all one-line functions
  PM / devfreq: tegra30: Constify structs
  PM / devfreq: tegra30: Ensure that target freq won't overflow
  PM / devfreq: tegra30: Fix integer overflow on CPU's freq max out
  PM / devfreq: tegra30: Use kHz units uniformly in the code
  PM / devfreq: tegra20/30: Add Dmitry as a maintainer

 MAINTAINERS                       |   9 +
 drivers/devfreq/tegra30-devfreq.c | 562 +++++++++++++++++++++---------
 2 files changed, 413 insertions(+), 158 deletions(-)

-- 
2.22.0


^ permalink raw reply

* [PATCH v2 02/17] PM / devfreq: tegra30: Keep interrupt disabled while governor is stopped
From: Dmitry Osipenko @ 2019-06-26  1:32 UTC (permalink / raw)
  To: Thierry Reding, MyungJoo Ham, Kyungmin Park, Chanwoo Choi,
	Jonathan Hunter, Tomeu Vizoso
  Cc: linux-pm, linux-tegra, linux-kernel
In-Reply-To: <20190626013252.30470-1-digetx@gmail.com>

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;
+
 	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)) {
 		err = PTR_ERR(tegra->devfreq);
 		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);
 
-- 
2.22.0


^ permalink raw reply related

* [PATCH v2 01/17] PM / devfreq: tegra30: Change irq type to unsigned int
From: Dmitry Osipenko @ 2019-06-26  1:32 UTC (permalink / raw)
  To: Thierry Reding, MyungJoo Ham, Kyungmin Park, Chanwoo Choi,
	Jonathan Hunter, Tomeu Vizoso
  Cc: linux-pm, linux-tegra, linux-kernel
In-Reply-To: <20190626013252.30470-1-digetx@gmail.com>

IRQ numbers are always positive, hence the corresponding variable should
be unsigned to keep types consistent. This is a minor change that cleans
up code a tad more.

Suggested-by: Thierry Reding <thierry.reding@gmail.com>
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);
 
-- 
2.22.0


^ permalink raw reply related

* [PATCH v2 03/17] PM / devfreq: tegra30: Handle possible round-rate error
From: Dmitry Osipenko @ 2019-06-26  1:32 UTC (permalink / raw)
  To: Thierry Reding, MyungJoo Ham, Kyungmin Park, Chanwoo Choi,
	Jonathan Hunter, Tomeu Vizoso
  Cc: linux-pm, linux-tegra, linux-kernel
In-Reply-To: <20190626013252.30470-1-digetx@gmail.com>

The EMC clock rate rounding technically could fail, hence let's handle
the error cases properly.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/devfreq/tegra30-devfreq.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
index 5e2b133babdd..5e606ae3f238 100644
--- a/drivers/devfreq/tegra30-devfreq.c
+++ b/drivers/devfreq/tegra30-devfreq.c
@@ -592,8 +592,8 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
 	struct tegra_devfreq_device *dev;
 	struct tegra_devfreq *tegra;
 	struct devfreq *devfreq;
-	unsigned long rate;
 	unsigned int i;
+	long rate;
 	int err;
 
 	tegra = devm_kzalloc(&pdev->dev, sizeof(*tegra), GFP_KERNEL);
@@ -650,8 +650,14 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
 
 	reset_control_deassert(tegra->reset);
 
-	tegra->max_freq = clk_round_rate(tegra->emc_clock, ULONG_MAX) / KHZ;
+	rate = clk_round_rate(tegra->emc_clock, ULONG_MAX);
+	if (rate < 0) {
+		dev_err(&pdev->dev, "Failed to round clock rate: %ld\n", rate);
+		return rate;
+	}
+
 	tegra->cur_freq = clk_get_rate(tegra->emc_clock) / KHZ;
+	tegra->max_freq = rate / KHZ;
 
 	for (i = 0; i < ARRAY_SIZE(actmon_device_configs); i++) {
 		dev = tegra->devices + i;
@@ -662,6 +668,13 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
 	for (rate = 0; rate <= tegra->max_freq * KHZ; rate++) {
 		rate = clk_round_rate(tegra->emc_clock, rate);
 
+		if (rate < 0) {
+			dev_err(&pdev->dev,
+				"Failed to round clock rate: %ld\n", rate);
+			err = rate;
+			goto remove_opps;
+		}
+
 		err = dev_pm_opp_add(&pdev->dev, rate, 0);
 		if (err) {
 			dev_err(&pdev->dev, "Failed to add OPP: %d\n", err);
-- 
2.22.0


^ permalink raw reply related

* [PATCH v2 09/17] PM / devfreq: tegra30: Reset boosting on startup
From: Dmitry Osipenko @ 2019-06-26  1:32 UTC (permalink / raw)
  To: Thierry Reding, MyungJoo Ham, Kyungmin Park, Chanwoo Choi,
	Jonathan Hunter, Tomeu Vizoso
  Cc: linux-pm, linux-tegra, linux-kernel
In-Reply-To: <20190626013252.30470-1-digetx@gmail.com>

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 8e1c56790ac3..b36d5d644c6e 100644
--- a/drivers/devfreq/tegra30-devfreq.c
+++ b/drivers/devfreq/tegra30-devfreq.c
@@ -534,6 +534,9 @@ static void tegra_actmon_configure_device(struct tegra_devfreq *tegra,
 {
 	u32 val = 0, target_freq;
 
+	/* we don't want boosting 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);
-- 
2.22.0


^ permalink raw reply related

* [PATCH v2 07/17] PM / devfreq: tegra30: Use CPUFreq notifier
From: Dmitry Osipenko @ 2019-06-26  1:32 UTC (permalink / raw)
  To: Thierry Reding, MyungJoo Ham, Kyungmin Park, Chanwoo Choi,
	Jonathan Hunter, Tomeu Vizoso
  Cc: linux-pm, linux-tegra, linux-kernel
In-Reply-To: <20190626013252.30470-1-digetx@gmail.com>

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 9b172e2acd40..a2cae8348dce 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;
+	struct notifier_block	cpu_rate_change_nb;
 
 	struct tegra_devfreq_device devices[ARRAY_SIZE(actmon_device_configs)];
 
@@ -454,8 +458,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;
@@ -465,7 +469,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:
@@ -494,6 +498,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);
+
+	/*
+	 * CPUFreq driver should support CPUFREQ_ASYNC_NOTIFICATIO in order
+	 * to allow asynchronous notifications. This means we can't block
+	 * here for too long, otherwise CPUFreq 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)
 {
@@ -525,9 +560,16 @@ static void tegra_actmon_configure_device(struct tegra_devfreq *tegra,
 	device_writel(dev, val, ACTMON_DEV_CTRL);
 }
 
-static void tegra_actmon_start(struct tegra_devfreq *tegra)
+static void tegra_actmon_stop_device(struct tegra_devfreq_device *dev)
+{
+	device_writel(dev, 0x00000000, ACTMON_DEV_CTRL);
+	device_writel(dev, ACTMON_INTR_STATUS_CLEAR, ACTMON_DEV_INTR_STATUS);
+}
+
+static int tegra_actmon_start(struct tegra_devfreq *tegra)
 {
 	unsigned int i;
+	int err;
 
 	actmon_writel(tegra, ACTMON_SAMPLING_PERIOD - 1,
 		      ACTMON_GLB_PERIOD_CTRL);
@@ -535,7 +577,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)
@@ -544,11 +609,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,
@@ -657,6 +724,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
@@ -667,7 +735,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:
@@ -682,11 +750,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 = {
@@ -792,9 +860,12 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, tegra);
 
-	tegra->rate_change_nb.notifier_call = tegra_actmon_rate_notify_cb;
+	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");
@@ -821,7 +892,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);
@@ -839,7 +910,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);
-- 
2.22.0


^ permalink raw reply related

* [PATCH v2 08/17] PM / devfreq: tegra30: Move clk-notifier's registration to governor's start
From: Dmitry Osipenko @ 2019-06-26  1:32 UTC (permalink / raw)
  To: Thierry Reding, MyungJoo Ham, Kyungmin Park, Chanwoo Choi,
	Jonathan Hunter, Tomeu Vizoso
  Cc: linux-pm, linux-tegra, linux-kernel
In-Reply-To: <20190626013252.30470-1-digetx@gmail.com>

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 a2cae8348dce..8e1c56790ac3 100644
--- a/drivers/devfreq/tegra30-devfreq.c
+++ b/drivers/devfreq/tegra30-devfreq.c
@@ -574,6 +574,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]);
 
@@ -600,6 +613,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;
 }
 
@@ -616,6 +631,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,
@@ -860,22 +877,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);
@@ -891,9 +900,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);
 
@@ -910,7 +916,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);
-- 
2.22.0


^ permalink raw reply related

* [PATCH v2 10/17] PM / devfreq: tegra30: Don't enable consecutive-down interrupt on startup
From: Dmitry Osipenko @ 2019-06-26  1:32 UTC (permalink / raw)
  To: Thierry Reding, MyungJoo Ham, Kyungmin Park, Chanwoo Choi,
	Jonathan Hunter, Tomeu Vizoso
  Cc: linux-pm, linux-tegra, linux-kernel
In-Reply-To: <20190626013252.30470-1-digetx@gmail.com>

The consecutive-down event tells that we should perform frequency
de-boosting, but boosting is in a reset state on start and hence the
event won't do anything useful for us and it will be just a dummy
interrupt request.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/devfreq/tegra30-devfreq.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
index b36d5d644c6e..1e704289c129 100644
--- a/drivers/devfreq/tegra30-devfreq.c
+++ b/drivers/devfreq/tegra30-devfreq.c
@@ -556,7 +556,6 @@ static void tegra_actmon_configure_device(struct tegra_devfreq *tegra,
 		<< ACTMON_DEV_CTRL_CONSECUTIVE_ABOVE_WMARK_NUM_SHIFT;
 	val |= ACTMON_DEV_CTRL_AVG_ABOVE_WMARK_EN;
 	val |= ACTMON_DEV_CTRL_AVG_BELOW_WMARK_EN;
-	val |= ACTMON_DEV_CTRL_CONSECUTIVE_BELOW_WMARK_EN;
 	val |= ACTMON_DEV_CTRL_CONSECUTIVE_ABOVE_WMARK_EN;
 	val |= ACTMON_DEV_CTRL_ENB;
 
-- 
2.22.0


^ permalink raw reply related

* [PATCH v2 11/17] PM / devfreq: tegra30: Add debug messages
From: Dmitry Osipenko @ 2019-06-26  1:32 UTC (permalink / raw)
  To: Thierry Reding, MyungJoo Ham, Kyungmin Park, Chanwoo Choi,
	Jonathan Hunter, Tomeu Vizoso
  Cc: linux-pm, linux-tegra, linux-kernel
In-Reply-To: <20190626013252.30470-1-digetx@gmail.com>

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 1e704289c129..8ef7ed8d7e10 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);
@@ -421,6 +448,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,
@@ -710,6 +739,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;
@@ -732,6 +762,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;
-- 
2.22.0


^ permalink raw reply related

* [PATCH v2 12/17] PM / devfreq: tegra30: Inline all one-line functions
From: Dmitry Osipenko @ 2019-06-26  1:32 UTC (permalink / raw)
  To: Thierry Reding, MyungJoo Ham, Kyungmin Park, Chanwoo Choi,
	Jonathan Hunter, Tomeu Vizoso
  Cc: linux-pm, linux-tegra, linux-kernel
In-Reply-To: <20190626013252.30470-1-digetx@gmail.com>

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 8ef7ed8d7e10..8630dca99905 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;
 }
-- 
2.22.0


^ permalink raw reply related

* [PATCH v2 15/17] PM / devfreq: tegra30: Fix integer overflow on CPU's freq max out
From: Dmitry Osipenko @ 2019-06-26  1:32 UTC (permalink / raw)
  To: Thierry Reding, MyungJoo Ham, Kyungmin Park, Chanwoo Choi,
	Jonathan Hunter, Tomeu Vizoso
  Cc: linux-pm, linux-tegra, linux-kernel
In-Reply-To: <20190626013252.30470-1-digetx@gmail.com>

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 1dcc2d2fa9e0..c03135000af7 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 },
-- 
2.22.0


^ permalink raw reply related

* [PATCH v2 17/17] PM / devfreq: tegra20/30: Add Dmitry as a maintainer
From: Dmitry Osipenko @ 2019-06-26  1:32 UTC (permalink / raw)
  To: Thierry Reding, MyungJoo Ham, Kyungmin Park, Chanwoo Choi,
	Jonathan Hunter, Tomeu Vizoso
  Cc: linux-pm, linux-tegra, linux-kernel
In-Reply-To: <20190626013252.30470-1-digetx@gmail.com>

I was contributing to the NVIDIA Tegra30+ devfreq drivers recently and
want to help keep them working and evolving in the future.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 MAINTAINERS | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 792d2d927712..bfd827417a27 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10250,6 +10250,15 @@ F:	include/linux/memblock.h
 F:	mm/memblock.c
 F:	Documentation/core-api/boot-time-mm.rst
 
+MEMORY FREQUENCY SCALING DRIVERS FOR NVIDIA TEGRA
+M:	Dmitry Osipenko <digetx@gmail.com>
+L:	linux-pm@vger.kernel.org
+L:	linux-tegra@vger.kernel.org
+T:	git git://git.kernel.org/pub/scm/linux/kernel/git/mzx/devfreq.git
+S:	Maintained
+F:	drivers/devfreq/tegra20-devfreq.c
+F:	drivers/devfreq/tegra30-devfreq.c
+
 MEMORY MANAGEMENT
 L:	linux-mm@kvack.org
 W:	http://www.linux-mm.org
-- 
2.22.0


^ permalink raw reply related

* [PATCH v2 16/17] PM / devfreq: tegra30: Use kHz units uniformly in the code
From: Dmitry Osipenko @ 2019-06-26  1:32 UTC (permalink / raw)
  To: Thierry Reding, MyungJoo Ham, Kyungmin Park, Chanwoo Choi,
	Jonathan Hunter, Tomeu Vizoso
  Cc: linux-pm, linux-tegra, linux-kernel
In-Reply-To: <20190626013252.30470-1-digetx@gmail.com>

Now that all kHz-conversion related bugs are fixed, we can use the kHz
uniformly. This makes code cleaner and avoids integer divisions in the
code, which is useful in a case of Tegra30 that has Cortex A9 CPU that
doesn't support integer division instructions, hence all divisions are
actually made in software mode. Another small benefit from this change
is that now powertop utility correctly displays devfreq's stats, for
some reason it expects them to be in kHz.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/devfreq/tegra30-devfreq.c | 78 +++++++++++++++++--------------
 1 file changed, 43 insertions(+), 35 deletions(-)

diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
index c03135000af7..778c5a940256 100644
--- a/drivers/devfreq/tegra30-devfreq.c
+++ b/drivers/devfreq/tegra30-devfreq.c
@@ -72,6 +72,8 @@
 
 #define KHZ							1000
 
+#define KHZ_MAX						(ULONG_MAX / KHZ)
+
 /* Assume that the bus is saturated if the utilization is 25% */
 #define BUS_SATURATION_RATIO					25
 
@@ -124,7 +126,7 @@ static const struct tegra_devfreq_device_config actmon_device_configs[] = {
 		.boost_down_coeff = 90,
 		.boost_up_threshold = 27,
 		.boost_down_threshold = 10,
-		.avg_dependency_threshold = 50000,
+		.avg_dependency_threshold = 50000 / ACTMON_SAMPLING_PERIOD,
 	},
 };
 
@@ -137,8 +139,11 @@ struct tegra_devfreq_device {
 	const struct tegra_devfreq_device_config *config;
 	void __iomem *regs;
 
-	/* Average event count sampled in the last interrupt */
-	u32 avg_count;
+	/*
+	 * Average event count sampled in the last interrupt and converted
+	 * to frequency value.
+	 */
+	u32 avg_freq;
 
 	/*
 	 * Extra frequency to increase the target by due to consecutive
@@ -172,7 +177,7 @@ struct tegra_actmon_emc_ratio {
 };
 
 static const struct tegra_actmon_emc_ratio actmon_emc_ratios[] = {
-	{ 1400000, ULONG_MAX / KHZ },
+	{ 1400000,    KHZ_MAX },
 	{ 1200000,    750000 },
 	{ 1100000,    600000 },
 	{ 1000000,    500000 },
@@ -234,7 +239,7 @@ tegra_actmon_account_cpu_freq(struct tegra_devfreq *tegra,
 	unsigned long static_cpu_emc_freq;
 
 	if (dev->config->avg_dependency_threshold &&
-	    dev->config->avg_dependency_threshold < dev->avg_count) {
+	    dev->config->avg_dependency_threshold < dev->avg_freq) {
 		static_cpu_emc_freq = actmon_cpu_to_emc_rate(tegra);
 		target_freq = max(target_freq, static_cpu_emc_freq);
 	}
@@ -265,7 +270,7 @@ static unsigned long tegra_actmon_upper_freq(struct tegra_devfreq *tegra,
 
 	opp = dev_pm_opp_find_freq_ceil(tegra->devfreq->dev.parent, &upper);
 	if (IS_ERR(opp))
-		upper = ULONG_MAX;
+		upper = KHZ_MAX;
 	else
 		dev_pm_opp_put(opp);
 
@@ -287,7 +292,7 @@ static void tegra_actmon_get_lower_upper(struct tegra_devfreq *tegra,
 	 * range in a case where target_freq falls into a range of
 	 * next_possible_opp_freq - 1MHz.
 	 */
-	target_freq = round_down(target_freq, 1000000);
+	target_freq = round_down(target_freq, 1000);
 
 	/* watermarks are set at the borders of the corresponding OPPs */
 	*lower = tegra_actmon_lower_freq(tegra, target_freq);
@@ -296,9 +301,6 @@ static void tegra_actmon_get_lower_upper(struct tegra_devfreq *tegra,
 	dev_dbg(ddev, "%03x: target_freq %lu lower freq %lu upper freq %lu\n",
 		offset, target_freq, *lower, *upper);
 
-	*lower /= KHZ;
-	*upper /= KHZ;
-
 	/*
 	 * The upper watermark should take into account CPU's frequency
 	 * because cpu_to_emc_rate() may override the target_freq with
@@ -314,22 +316,23 @@ static void tegra_actmon_get_lower_upper(struct tegra_devfreq *tegra,
 static void tegra_devfreq_update_avg_wmark(struct tegra_devfreq *tegra,
 					   struct tegra_devfreq_device *dev)
 {
-	unsigned long lower, upper, freq;
+	unsigned long avg_threshold, lower, upper;
 
-	freq = dev->avg_count / ACTMON_SAMPLING_PERIOD * KHZ;
-	tegra_actmon_get_lower_upper(tegra, dev, freq, &lower, &upper);
+	tegra_actmon_get_lower_upper(tegra, dev, dev->avg_freq, &lower, &upper);
+
+	avg_threshold = dev->config->avg_dependency_threshold;
+	avg_threshold = avg_threshold * ACTMON_SAMPLING_PERIOD;
 
 	/*
 	 * We want to get interrupts when MCCPU client crosses the
 	 * dependency threshold in order to take into / out of account
 	 * the CPU's freq.
 	 */
-	if (lower < dev->config->avg_dependency_threshold &&
-	    upper > dev->config->avg_dependency_threshold) {
-		if (dev->avg_count < dev->config->avg_dependency_threshold)
-			upper = dev->config->avg_dependency_threshold;
+	if (lower < avg_threshold && upper > avg_threshold) {
+		if (dev->avg_freq < dev->config->avg_dependency_threshold)
+			upper = avg_threshold;
 		else
-			lower = dev->config->avg_dependency_threshold;
+			lower = avg_threshold;
 	}
 
 	device_writel(dev, lower, ACTMON_DEV_AVG_LOWER_WMARK);
@@ -368,8 +371,7 @@ static void tegra_devfreq_update_wmark(struct tegra_devfreq *tegra,
 	 * device. Once that mark is hit and boosting is stopped, the
 	 * interrupt is disabled by ISR.
 	 */
-	freq = dev->avg_count / ACTMON_SAMPLING_PERIOD * KHZ;
-	tegra_actmon_get_lower_upper(tegra, dev, freq, &lower, &upper);
+	tegra_actmon_get_lower_upper(tegra, dev, dev->avg_freq, &lower, &upper);
 
 	delta = do_percent(upper - lower, dev->config->boost_down_threshold);
 	device_writel(dev, lower + delta, ACTMON_DEV_LOWER_WMARK);
@@ -396,14 +398,16 @@ static void actmon_device_debug(struct tegra_devfreq *tegra,
 static void actmon_isr_device(struct tegra_devfreq *tegra,
 			      struct tegra_devfreq_device *dev)
 {
-	u32 intr_status, dev_ctrl, avg_intr_mask;
+	u32 intr_status, dev_ctrl, avg_intr_mask, avg_count;
 
 	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);
+	avg_count = device_readl(dev, ACTMON_DEV_AVG_COUNT);
 	dev_ctrl = device_readl(dev, ACTMON_DEV_CTRL);
 
+	dev->avg_freq = avg_count / ACTMON_SAMPLING_PERIOD;
+
 	avg_intr_mask = ACTMON_DEV_INTR_AVG_BELOW_WMARK |
 			ACTMON_DEV_INTR_AVG_ABOVE_WMARK;
 
@@ -457,8 +461,7 @@ static unsigned long actmon_update_target(struct tegra_devfreq *tegra,
 					  struct tegra_devfreq_device *dev)
 {	unsigned long target_freq;
 
-	target_freq = dev->avg_count / ACTMON_SAMPLING_PERIOD + dev->boost_freq;
-	target_freq = min(target_freq, ULONG_MAX / KHZ);
+	target_freq = min(dev->avg_freq + dev->boost_freq, KHZ_MAX);
 	target_freq = tegra_actmon_account_cpu_freq(tegra, dev, target_freq);
 
 	return target_freq;
@@ -495,6 +498,7 @@ static int tegra_actmon_clk_notify_cb(struct notifier_block *nb,
 	struct clk_notifier_data *data = ptr;
 	struct tegra_devfreq_device *dev;
 	struct tegra_devfreq *tegra;
+	unsigned long freq;
 	unsigned int i;
 
 	if (action != POST_RATE_CHANGE)
@@ -502,6 +506,8 @@ static int tegra_actmon_clk_notify_cb(struct notifier_block *nb,
 
 	tegra = container_of(nb, struct tegra_devfreq, clk_rate_change_nb);
 
+	freq = data->new_rate / KHZ;
+
 	/*
 	 * EMC rate could change due to three reasons:
 	 *
@@ -523,7 +529,7 @@ static int tegra_actmon_clk_notify_cb(struct notifier_block *nb,
 		dev = &tegra->devices[i];
 
 		tegra_devfreq_update_avg_wmark(tegra, dev);
-		tegra_devfreq_update_wmark(tegra, dev, data->new_rate);
+		tegra_devfreq_update_wmark(tegra, dev, freq);
 	}
 
 	return NOTIFY_OK;
@@ -563,17 +569,17 @@ static int tegra_actmon_cpu_notify_cb(struct notifier_block *nb,
 static void tegra_actmon_configure_device(struct tegra_devfreq *tegra,
 					  struct tegra_devfreq_device *dev)
 {
-	u32 val = 0, target_freq;
+	u32 val = 0;
 
 	/* we don't want boosting 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);
+	dev->avg_freq = clk_get_rate(tegra->emc_clock) / KHZ;
+	device_writel(dev, dev->avg_freq * ACTMON_SAMPLING_PERIOD,
+		      ACTMON_DEV_INIT_AVG);
 
 	tegra_devfreq_update_avg_wmark(tegra, dev);
-	tegra_devfreq_update_wmark(tegra, dev, target_freq);
+	tegra_devfreq_update_wmark(tegra, dev, dev->avg_freq);
 
 	device_writel(dev, ACTMON_COUNT_WEIGHT, ACTMON_DEV_COUNT_WEIGHT);
 	device_writel(dev, ACTMON_INTR_STATUS_CLEAR, ACTMON_DEV_INTR_STATUS);
@@ -685,7 +691,7 @@ static int tegra_devfreq_target(struct device *dev, unsigned long *freq,
 	rate = dev_pm_opp_get_freq(opp);
 	dev_pm_opp_put(opp);
 
-	err = clk_set_min_rate(tegra->emc_clock, rate);
+	err = clk_set_min_rate(tegra->emc_clock, rate * KHZ);
 	if (err)
 		return err;
 
@@ -708,7 +714,7 @@ static int tegra_devfreq_get_dev_status(struct device *dev,
 	struct tegra_devfreq_device *actmon_dev;
 	unsigned long cur_freq;
 
-	cur_freq = clk_get_rate(tegra->emc_clock);
+	cur_freq = clk_get_rate(tegra->emc_clock) / KHZ;
 
 	/* To be used by the tegra governor */
 	stat->private_data = tegra;
@@ -725,7 +731,7 @@ static int tegra_devfreq_get_dev_status(struct device *dev,
 	stat->busy_time *= 100 / BUS_SATURATION_RATIO;
 
 	/* Number of cycles in a sampling period */
-	stat->total_time = cur_freq / KHZ * ACTMON_SAMPLING_PERIOD;
+	stat->total_time = cur_freq * ACTMON_SAMPLING_PERIOD;
 
 	stat->busy_time = min(stat->busy_time, stat->total_time);
 
@@ -771,7 +777,7 @@ static int tegra_governor_get_target(struct devfreq *devfreq,
 		actmon_device_debug(tegra, dev, "upd");
 	}
 
-	*freq = target_freq * KHZ;
+	*freq = target_freq;
 
 	return 0;
 }
@@ -907,7 +913,7 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
 			goto remove_opps;
 		}
 
-		err = dev_pm_opp_add(&pdev->dev, rate, 0);
+		err = dev_pm_opp_add(&pdev->dev, rate / KHZ, 0);
 		if (err) {
 			dev_err(&pdev->dev, "Failed to add OPP: %d\n", err);
 			goto remove_opps;
@@ -927,6 +933,8 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
 	}
 
 	tegra_devfreq_profile.initial_freq = clk_get_rate(tegra->emc_clock);
+	tegra_devfreq_profile.initial_freq /= KHZ;
+
 	devfreq = devfreq_add_device(&pdev->dev, &tegra_devfreq_profile,
 				     "tegra_actmon", NULL);
 	if (IS_ERR(tegra->devfreq)) {
-- 
2.22.0


^ permalink raw reply related


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox