From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S964929AbbLRB1t (ORCPT ); Thu, 17 Dec 2015 20:27:49 -0500 Received: from mailout2.samsung.com ([203.254.224.25]:37485 "EHLO mailout2.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753234AbbLRB1q convert rfc822-to-8bit (ORCPT ); Thu, 17 Dec 2015 20:27:46 -0500 X-AuditID: cbfee68d-f79646d000001355-8b-56736110c783 MIME-version: 1.0 Content-type: text/plain; charset=utf-8 Content-transfer-encoding: 8BIT Message-id: <56736105.6060009@samsung.com> Date: Fri, 18 Dec 2015 10:27:33 +0900 From: Chanwoo Choi User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.6.0 To: myungjoo.ham@samsung.com, =?UTF-8?B?7YGs7Ims7Iuc7Yag7ZSE?= , "kgene@kernel.org" Cc: =?UTF-8?B?67CV6rK966+8?= , "robh+dt@kernel.org" , "pawel.moll@arm.com" , "mark.rutland@arm.com" , "ijc+devicetree@hellion.org.uk" , "galak@codeaurora.org" , "linux@arm.linux.org.uk" , "tjakobi@math.uni-bielefeld.de" , "linux.amoon@gmail.com" , "linux-kernel@vger.kernel.org" , "linux-pm@vger.kernel.org" , "linux-samsung-soc@vger.kernel.org" , "devicetree@vger.kernel.org" Subject: Re: [PATCH v4 05/20] PM / devfreq: Add new passive governor References: <1874820122.661091450083873584.JavaMail.weblogic@epmlwas07b> In-reply-to: <1874820122.661091450083873584.JavaMail.weblogic@epmlwas07b> X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFvrGIsWRmVeSWpSXmKPExsWyRsSkUFcgsTjMYMcicYv5R86xWvS/Wchq ce7VSkaL1y8MLfofv2a2ONv0ht3i8q45bBafe48wWsw4v4/JYt3GW+wWty/zWiy9fpHJ4nbj CjaLCdPXsli07j3CbtG2+gOrg4DHmnlrGD1amnvYPC739TJ57Jx1l91j5fIvbB6bVnWyefw7 xu7Rt2UVo8fnTXIBnFFcNimpOZllqUX6dglcGc+W7WYsWGpf8ePLXaYGxonGXYycHBICJhKn 7k1khrDFJC7cW8/WxcjFISSwglFi1vV3bDBFC5avZoFILGWU+D2jFayDV0BQ4sfke0AJDg5m AXWJKVNyQcLMAiISv6fsZ4ewtSWWLXwNVi4k8IBRovVzMkg5r4CWxOl/QSBhFgFViXkT7rGC 2GxA4f0vbrCBlIgKREh0n6gE2Soi0Mwo8fliDwvEyCesEqs/aIPYwgIuEtdvLGeBGO8h0fdv O9gqTgFPieXHtoGdLCGwlkPidvtpVohlAhLfJh8CO1lCQFZi0wGo3yUlDq64wTKBUXwWksdm ITw2C8ljs5A8toCRZRWjaGpBckFxUnqRoV5xYm5xaV66XnJ+7iZGYHI4/e9Z7w7G2wesDzEK cDAq8fAasBWHCbEmlhVX5h5iNAU6aCKzlGhyPjAF5ZXEGxqbGVmYmpgaG5lbmimJ8ypK/QwW EkhPLEnNTk0tSC2KLyrNSS0+xMjEwSnVwHj54nQH3oteOxPut54+F/zyrcC9GfHLYxv03tev SFv5/SB36zamowe0F/LJHncNOaL5aZ7p2nWfFu2bWstumz31K0fDzQVpwat+n5ljlfmlqibx rsIFfVWZ3bNmSmme6vil2mMrnnnQMaNWxl5YoHEX15fvpnE3RQqsJm/qyDC/eN5Qb6mx01wl luKMREMt5qLiRADqrSkFCQMAAA== X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFlrJKsWRmVeSWpSXmKPExsVy+t9jAV2BxOIwg1tf+S3mHznHatH/ZiGr xblXKxktXr8wtOh//JrZ4mzTG3aLy7vmsFl87j3CaDHj/D4mi3Ubb7Fb3L7Ma7H0+kUmi9uN K9gsJkxfy2LRuvcIu0Xb6g+sDgIea+atYfRoae5h87jc18vksXPWXXaPlcu/sHlsWtXJ5vHv GLtH35ZVjB6fN8kFcEY1MNpkpCampBYppOYl56dk5qXbKnkHxzvHm5oZGOoaWlqYKynkJeam 2iq5+AToumXmAH2hpFCWmFMKFApILC5W0rfDNCE0xE3XAqYxQtc3JAiux8gADSSsYcy4crCb uaDRvmLN7PvsDYxvjboYOTkkBEwkFixfzQJhi0lcuLeerYuRi0NIYCmjxO8ZrcwgCV4BQYkf k+8BFXFwMAvISxy5lA1hqktMmZILUiEk8IBRovVzMkiYV0BL4vS/IJAwi4CqxLwJ91hBbDag 8P4XN9hASkQFIiS6T1SCLBIRaGaU+HyxB+wCZoEnrBKrP2iD2MICLhLXbyxngRjvIdH3bzvY MZwCnhLLj21jmcAoMAvJbbMQbpuFcNsCRuZVjBKpBckFxUnpuYZ5qeV6xYm5xaV56XrJ+bmb GMHJ5JnUDsaDu9wPMQpwMCrx8N5gLg4TYk0sK67MPcQowcGsJMIbEA4U4k1JrKxKLcqPLyrN SS0+xGgK9N5EZinR5HxgossriTc0NjEzsjQyN7QwMjZXEuetvRQZJiSQnliSmp2aWpBaBNPH xMEp1cAo9UEtOFNTjKlyxqLH3a8/3gmQrXy8Z//Thdpq56q8nW0VBG/GP0vfvFK+x1HW/VLx 4iifMM25a8KWpz+8fHPHrzNX2yQcE0+VSfkeNmLpbGd5JW61eLPkY7tH9n3Gb2b8a+BhtC2W Xpnu8Ke8IiHoxCl2cT5ZvsP6Kx6erT7Lp9VQZ26mEabEUpyRaKjFXFScCAAYWakbPAMAAA== DLP-Filter: Pass X-MTR: 20000000000000000@CPGS X-CFilter-Loop: Reflected Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 >> Signed-off-by: Chanwoo Choi >> [linux.amoon: Tested on Odroid U3] >> Tested-by: Anand Moon >> --- >> 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