Re: [PATCH v4 07/24] PM / devfreq: tegra30: Use CPUFreq notifier

[Dmitry Osipenko]

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@xxxxxxxxx>
>> ---
>>  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.