Re: [PATCH v4 05/20] PM / devfreq: Add new passive governor

[Chanwoo Choi <cw00.choi@samsung.com>]

On 2015년 12월 14일 18:04, MyungJoo Ham wrote:
>>   
>>  This patch adds the new passive governor for DEVFREQ framework. The following
>> governors are already present and used for DVFS (Dynamic Voltage and Frequency
>> Scaling) drivers. The following governors are independently used for one device
>> driver which don't give the influence to other device drviers and also don't
>> receive the effect from other device drivers.
>> - ondemand / performance / powersave / userspace
>>
>> The passive governor depends on operation of parent driver with specific
>> governos extremely and is not able to decide the new frequency by oneself.
>> According to the decided new frequency of parent driver with governor,
>> the passive governor uses it to decide the appropriate frequency for own
>> device driver. The passive governor must need the following information
>> from device tree:
>> - the source clock and OPP tables
>> - the instance of parent device
>>
>> For exameple,
>> there are one more devfreq device drivers which need to change their source
>> clock according to their utilization on runtime. But, they share the same
>> power line (e.g., regulator). So, specific device driver is operated as parent
>> with ondemand governor and then the rest device driver with passive governor
>> is influenced by parent device.
>>
>> Suggested-by: Myungjoo Ham <myungjoo.ham@samsung.com>
>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
>> [linux.amoon: Tested on Odroid U3]
>> Tested-by: Anand Moon <linux.amoon@gmail.com>
>> ---
>>  drivers/devfreq/Kconfig            |   9 ++++
>>  drivers/devfreq/Makefile           |   1 +
>>  drivers/devfreq/devfreq.c          |  47 ++++++++++++++++
>>  drivers/devfreq/governor_passive.c | 108 +++++++++++++++++++++++++++++++++++++
>>  include/linux/devfreq.h            |  15 ++++++
>>  5 files changed, 180 insertions(+)
>>  create mode 100644 drivers/devfreq/governor_passive.c
>>
>> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
>> index 55ec774f794c..d03f635a93e1 100644
>> --- a/drivers/devfreq/Kconfig
>> +++ b/drivers/devfreq/Kconfig
>> @@ -64,6 +64,15 @@ config DEVFREQ_GOV_USERSPACE
>>  	  Otherwise, the governor does not change the frequnecy
>>  	  given at the initialization.
>>  
>> +config DEVFREQ_GOV_PASSIVE
>> +	tristate "Passive"
>> +	help
>> +	  Sets the frequency by other governors (simple_ondemand, performance,
>> +	  powersave, usersapce) of a parent devfreq device. This governor
>> +	  always has the dependency on the chosen frequency from paired
>> +	  governor. This governor does not change the frequency by oneself
>> +	  through sysfs entry.
> 
> 	Sets the frequency based on the frequency of its parent devfreq
> 	device. This governor does not change the frequency by itself
> 	through sysfs entries.

OK. I'll modify it.

> 
>> +
>>  comment "DEVFREQ Drivers"
>>  
>>  config ARM_EXYNOS_BUS_DEVFREQ
>> diff --git a/drivers/devfreq/Makefile b/drivers/devfreq/Makefile
>> index 375ebbb4fcfb..f81c313b4b79 100644
>> --- a/drivers/devfreq/Makefile
>> +++ b/drivers/devfreq/Makefile
> []
>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>> index 984c5e9e7bdd..15e58779e4c0 100644
>> --- a/drivers/devfreq/devfreq.c
>> +++ b/drivers/devfreq/devfreq.c
>> @@ -190,6 +190,31 @@ static struct devfreq_governor *find_devfreq_governor(const char *name)
>>  
>>  /* Load monitoring helper functions for governors use */
>>  
>> +static int update_devfreq_passive(struct devfreq *devfreq, unsigned long freq)
>> +{
>> +	struct devfreq *passive;
>> +	unsigned long rate;
>> +	int ret;
>> +
>> +	list_for_each_entry(passive, &devfreq->passive_dev_list, passive_node) {
>> +		if (!passive->governor)
>> +			continue;
>> +		rate = freq;
>> +
>> +		ret = passive->governor->get_target_freq(passive, &rate);
>> +		if (ret)
>> +			return ret;
>> +
>> +		ret = passive->profile->target(passive->dev.parent, &rate, 0);
>> +		if (ret)
>> +			return ret;
>> +
>> +		passive->previous_freq = rate;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>  /**
>>   * update_devfreq() - Reevaluate the device and configure frequency.
>>   * @devfreq:	the devfreq instance.
>> @@ -233,10 +258,18 @@ int update_devfreq(struct devfreq *devfreq)
>>  		flags |= DEVFREQ_FLAG_LEAST_UPPER_BOUND; /* Use LUB */
>>  	}
>>  
>> +	if (!list_empty(&devfreq->passive_dev_list)
>> +		&& devfreq->previous_freq > freq)
>> +		update_devfreq_passive(devfreq, freq);
>> +
> 
> Could you please comment somewhere appropriate
> that the dependent is going to be changed
> before its parent if the frequency is going down.
> (and after if going up)
> And state why as well.

I use the DEVFREQ_TRANSITION_NOTIFIER instead of this implementation.

> 
> And, is this viable universally?
> 
>>  	err = devfreq->profile->target(devfreq->dev.parent, &freq, flags);
>>  	if (err)
>>  		return err;
>>  
>> +	if (!list_empty(&devfreq->passive_dev_list)
>> +		&& devfreq->previous_freq < freq)
>> +		update_devfreq_passive(devfreq, freq);
>> +
>>  	if (devfreq->profile->freq_table)
>>  		if (devfreq_update_status(devfreq, freq))
>>  			dev_err(&devfreq->dev,
>> @@ -442,6 +475,10 @@ static void _remove_devfreq(struct devfreq *devfreq)
>>  		return;
>>  	}
>>  	list_del(&devfreq->node);
>> +	list_del(&devfreq->passive_node);
>> +	if (!list_empty(&devfreq->passive_dev_list))
>> +		list_del_init(&devfreq->passive_dev_list);
>> +
>>  	mutex_unlock(&devfreq_list_lock);
>>  
>>  	if (devfreq->governor)
>> @@ -559,6 +596,16 @@ struct devfreq *devfreq_add_device(struct device *dev,
>>  		goto err_init;
>>  	}
>>  
>> +	if (!strncmp(devfreq->governor_name, "passive", 7)) {
>> +		struct devfreq *parent_devfreq =
>> +			((struct devfreq_passive_data *)data)->parent;
>> +
>> +		list_add(&devfreq->passive_node,
>> +			&parent_devfreq->passive_dev_list);
>> +	} else {
>> +		INIT_LIST_HEAD(&devfreq->passive_dev_list);
>> +	}
>> +
>>  	return devfreq;
>>  
>>  err_init:
> 
> This code has become too much invasive to devfreq.c
> while being too special for the passive governor.

I agree. I'll implement again with notifier.

> 
> Why don't you add notifier chain to devfreq.c, which can be used
> by anyone else as well, and use that notifier for passive governor?
> You may refer to "cpufreq_register_notifier()" with 
> CPUFREQ_TRANSITION_NOTIFIER.

OK. I'll add the new notifier of DEVFREQ_TRANSITION_NOTIFIER
The list of supported notification:
	DEVFREQ_PRECHANGE
	DEVFREQ_POSTCHANGE

> 
>> diff --git a/drivers/devfreq/governor_passive.c b/drivers/devfreq/governor_passive.c
>> new file mode 100644
> 
> Then, utilizing notifier-block at governor_passive.c becomes possible.
> 
> You will also be able to write any frequency deciding code
> inside governor_passive.c as well, not in devfreq.c.

OK, I'll use DEVFREQ_TRANSITION_NOTIFIER for passive governor.

> 
>> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
>> index 6fa02a20eb63..95c54578a1b4 100644
>> --- a/include/linux/devfreq.h
>> +++ b/include/linux/devfreq.h
>> @@ -177,6 +177,9 @@ struct devfreq {
>>  	unsigned int *trans_table;
>>  	unsigned long *time_in_state;
>>  	unsigned long last_stat_updated;
>> +
>> +	struct list_head passive_dev_list;
>> +	struct list_head passive_node;
>>  };
> 
> You will need only one notifier head here.

OK.

> 
>>  
>>  #if defined(CONFIG_PM_DEVFREQ)
>> @@ -241,6 +244,18 @@ struct devfreq_simple_ondemand_data {
>>  };
>>  #endif
>>  
>> +/**
>> + * struct devfreq_passive_data - void *data fed to struct devfreq
>> + *	and devfreq_add_device
>> + * @parent:		The parent devfreq device.
>> + *
>> + * If the fed devfreq_passive_data pointer is NULL to the governor,
>> + * the governor return ERROR.
>> + */
>> +struct devfreq_passive_data {
>> +	struct devfreq *parent;
>> +};
>> +
> 
> Please enclose the above with #if

OK.

Thanks,
Chanwoo Choi