| Message ID | 1448616727-29367-4-git-send-email-dawei.chien@mediatek.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Eduardo Valentin |
| Headers | show |
On 27-11-15, 17:32, Dawei Chien wrote: > MT8173 cpufreq driver use of_cpufreq_power_cooling_register registering > cooling devices with dynamic power coefficient. > > Signed-off-by: Dawei Chien <dawei.chien@mediatek.com> > --- > This patch is base on patchset: > https://lkml.org/lkml/2015/11/17/251 > --- > drivers/cpufreq/mt8173-cpufreq.c | 28 ++++++++++++++++++++-------- > 1 file changed, 20 insertions(+), 8 deletions(-) > > diff --git a/drivers/cpufreq/mt8173-cpufreq.c b/drivers/cpufreq/mt8173-cpufreq.c > index 83001dc..4d39468 100644 > --- a/drivers/cpufreq/mt8173-cpufreq.c > +++ b/drivers/cpufreq/mt8173-cpufreq.c > @@ -263,24 +263,34 @@ static int mtk_cpufreq_set_target(struct cpufreq_policy *policy, > return 0; > } > > +#define DYNAMIC_POWER "dynamic-power-coefficient" > + > static void mtk_cpufreq_ready(struct cpufreq_policy *policy) > { > struct mtk_cpu_dvfs_info *info = policy->driver_data; > struct device_node *np = of_node_get(info->cpu_dev->of_node); > + u32 capacitance; > > if (WARN_ON(!np)) > return; > > if (of_find_property(np, "#cooling-cells", NULL)) { > - info->cdev = of_cpufreq_cooling_register(np, > - policy->related_cpus); > + if (!info->cdev) { Why will info->cdev be non-NULL here ? > + of_property_read_u32(np, DYNAMIC_POWER, &capacitance); This can fail, in which case capacitance will be used uninitialized. Fix that by initializing it with 0 at the beginning of this routine. > + info->cdev = of_cpufreq_power_cooling_register(np, > + policy->related_cpus, > + capacitance, > + NULL); > > - if (IS_ERR(info->cdev)) { > - dev_err(info->cpu_dev, > - "running cpufreq without cooling device: %ld\n", > - PTR_ERR(info->cdev)); > + if (IS_ERR(info->cdev)) { > + dev_err(info->cpu_dev, > + "running cpufreq without cooling device: %ld\n", > + PTR_ERR(info->cdev)); > > - info->cdev = NULL; > + info->cdev = NULL; > + } > } > } > > @@ -460,7 +470,9 @@ static int mtk_cpufreq_exit(struct cpufreq_policy *policy) > { > struct mtk_cpu_dvfs_info *info = policy->driver_data; > > - cpufreq_cooling_unregister(info->cdev); > + if (info->cdev) > + cpufreq_cooling_unregister(info->cdev); > + Why do you need to update this? > dev_pm_opp_free_cpufreq_table(info->cpu_dev, &policy->freq_table); > mtk_cpu_dvfs_info_release(info); > kfree(info); > -- > 1.7.9.5
On Mon, 2015-11-30 at 11:08 +0530, Viresh Kumar wrote: > On 27-11-15, 17:32, Dawei Chien wrote: > > MT8173 cpufreq driver use of_cpufreq_power_cooling_register registering > > cooling devices with dynamic power coefficient. > > > > Signed-off-by: Dawei Chien <dawei.chien@mediatek.com> > > --- > > This patch is base on patchset: > > https://lkml.org/lkml/2015/11/17/251 > > --- > > drivers/cpufreq/mt8173-cpufreq.c | 28 ++++++++++++++++++++-------- > > 1 file changed, 20 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/cpufreq/mt8173-cpufreq.c b/drivers/cpufreq/mt8173-cpufreq.c > > index 83001dc..4d39468 100644 > > --- a/drivers/cpufreq/mt8173-cpufreq.c > > +++ b/drivers/cpufreq/mt8173-cpufreq.c > > @@ -263,24 +263,34 @@ static int mtk_cpufreq_set_target(struct cpufreq_policy *policy, > > return 0; > > } > > > > +#define DYNAMIC_POWER "dynamic-power-coefficient" > > + > > static void mtk_cpufreq_ready(struct cpufreq_policy *policy) > > { > > struct mtk_cpu_dvfs_info *info = policy->driver_data; > > struct device_node *np = of_node_get(info->cpu_dev->of_node); > > + u32 capacitance; > > > > if (WARN_ON(!np)) > > return; > > > > if (of_find_property(np, "#cooling-cells", NULL)) { > > - info->cdev = of_cpufreq_cooling_register(np, > > - policy->related_cpus); > > + if (!info->cdev) { > > Why will info->cdev be non-NULL here ? This is a error-checking to avoid user or any script by command line hotplug CPU more than two times, we don't need to register cooling device on this case. I will remove it if you don't agree it. > > + of_property_read_u32(np, DYNAMIC_POWER, &capacitance); > > This can fail, in which case capacitance will be used uninitialized. > Fix that by initializing it with 0 at the beginning of this routine. Thank you, I will follow your comment to fix it on next version. > > + info->cdev = of_cpufreq_power_cooling_register(np, > > + policy->related_cpus, > > + capacitance, > > + NULL); > > > > - if (IS_ERR(info->cdev)) { > > - dev_err(info->cpu_dev, > > - "running cpufreq without cooling device: %ld\n", > > - PTR_ERR(info->cdev)); > > + if (IS_ERR(info->cdev)) { > > + dev_err(info->cpu_dev, > > + "running cpufreq without cooling device: %ld\n", > > + PTR_ERR(info->cdev)); > > > > - info->cdev = NULL; > > + info->cdev = NULL; > > + } > > } > > } > > > > @@ -460,7 +470,9 @@ static int mtk_cpufreq_exit(struct cpufreq_policy *policy) > > { > > struct mtk_cpu_dvfs_info *info = policy->driver_data; > > > > - cpufreq_cooling_unregister(info->cdev); > > + if (info->cdev) > > + cpufreq_cooling_unregister(info->cdev); > > + > > Why do you need to update this? This is a error-checking to avoid user or any script by command line unplug CPU more than two times, we don't need to unregister cooling device on this case. I will remove it if you don't agree it. > > dev_pm_opp_free_cpufreq_table(info->cpu_dev, &policy->freq_table); > > mtk_cpu_dvfs_info_release(info); > > kfree(info); > > -- > > 1.7.9.5 > -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 30-11-15, 17:26, dawei chien wrote: > On Mon, 2015-11-30 at 11:08 +0530, Viresh Kumar wrote: > > On 27-11-15, 17:32, Dawei Chien wrote: > > > MT8173 cpufreq driver use of_cpufreq_power_cooling_register registering > > > cooling devices with dynamic power coefficient. > > > > > > Signed-off-by: Dawei Chien <dawei.chien@mediatek.com> > > > --- > > > This patch is base on patchset: > > > https://lkml.org/lkml/2015/11/17/251 > > > --- > > > drivers/cpufreq/mt8173-cpufreq.c | 28 ++++++++++++++++++++-------- > > > 1 file changed, 20 insertions(+), 8 deletions(-) > > > > > > diff --git a/drivers/cpufreq/mt8173-cpufreq.c b/drivers/cpufreq/mt8173-cpufreq.c > > > index 83001dc..4d39468 100644 > > > --- a/drivers/cpufreq/mt8173-cpufreq.c > > > +++ b/drivers/cpufreq/mt8173-cpufreq.c > > > @@ -263,24 +263,34 @@ static int mtk_cpufreq_set_target(struct cpufreq_policy *policy, > > > return 0; > > > } > > > > > > +#define DYNAMIC_POWER "dynamic-power-coefficient" > > > + > > > static void mtk_cpufreq_ready(struct cpufreq_policy *policy) > > > { > > > struct mtk_cpu_dvfs_info *info = policy->driver_data; > > > struct device_node *np = of_node_get(info->cpu_dev->of_node); > > > + u32 capacitance; > > > > > > if (WARN_ON(!np)) > > > return; > > > > > > if (of_find_property(np, "#cooling-cells", NULL)) { > > > - info->cdev = of_cpufreq_cooling_register(np, > > > - policy->related_cpus); > > > + if (!info->cdev) { > > > > Why will info->cdev be non-NULL here ? > > This is a error-checking to avoid user or any script by command line hotplug CPU > more than two times, we don't need to register cooling device on this case. Why? > I will remove it if you don't agree it. No, my agreeing or not doesn't matter. If what you are doing is useful (and I am not able to understand it), then you should make me understand that and don't change your code. But, I really do not get the reasoning behind the logic. Please elaborate that step by step. > > > @@ -460,7 +470,9 @@ static int mtk_cpufreq_exit(struct cpufreq_policy *policy) > > > { > > > struct mtk_cpu_dvfs_info *info = policy->driver_data; > > > > > > - cpufreq_cooling_unregister(info->cdev); > > > + if (info->cdev) > > > + cpufreq_cooling_unregister(info->cdev); > > > + > > > > Why do you need to update this? > > This is a error-checking to avoid user or any script by command line > unplug CPU more than two times, we don't need to unregister cooling > device on this case. > > I will remove it if you don't agree it. Same here.
On Mon, 2015-11-30 at 15:00 +0530, Viresh Kumar wrote: > On 30-11-15, 17:26, dawei chien wrote: > > On Mon, 2015-11-30 at 11:08 +0530, Viresh Kumar wrote: > > > On 27-11-15, 17:32, Dawei Chien wrote: > > > > MT8173 cpufreq driver use of_cpufreq_power_cooling_register registering > > > > cooling devices with dynamic power coefficient. > > > > > > > > Signed-off-by: Dawei Chien <dawei.chien@mediatek.com> > > > > --- > > > > This patch is base on patchset: > > > > https://lkml.org/lkml/2015/11/17/251 > > > > --- > > > > drivers/cpufreq/mt8173-cpufreq.c | 28 ++++++++++++++++++++-------- > > > > 1 file changed, 20 insertions(+), 8 deletions(-) > > > > > > > > diff --git a/drivers/cpufreq/mt8173-cpufreq.c b/drivers/cpufreq/mt8173-cpufreq.c > > > > index 83001dc..4d39468 100644 > > > > --- a/drivers/cpufreq/mt8173-cpufreq.c > > > > +++ b/drivers/cpufreq/mt8173-cpufreq.c > > > > @@ -263,24 +263,34 @@ static int mtk_cpufreq_set_target(struct cpufreq_policy *policy, > > > > return 0; > > > > } > > > > > > > > +#define DYNAMIC_POWER "dynamic-power-coefficient" > > > > + > > > > static void mtk_cpufreq_ready(struct cpufreq_policy *policy) > > > > { > > > > struct mtk_cpu_dvfs_info *info = policy->driver_data; > > > > struct device_node *np = of_node_get(info->cpu_dev->of_node); > > > > + u32 capacitance; > > > > > > > > if (WARN_ON(!np)) > > > > return; > > > > > > > > if (of_find_property(np, "#cooling-cells", NULL)) { > > > > - info->cdev = of_cpufreq_cooling_register(np, > > > > - policy->related_cpus); > > > > + if (!info->cdev) { > > > > > > Why will info->cdev be non-NULL here ? > > > > This is a error-checking to avoid user or any script by command line hotplug CPU > > more than two times, we don't need to register cooling device on this case. > > Why? As far as I know, user or shell script has the right for using command online/offline cpu. Either user by console or shell script could make cpu2 online even cpu2 already onlined. That could cause of_cpufreq_cooling_register execute many times for the same cooling device. "echo 1 > /sys/devices/system/cpu/cpu2/online" With above hotplug command, mtk_cpufreq_ready will register cooling device again as well, but fail since the cooling device already created, so we might need not register cooling device again on this case. > > I will remove it if you don't agree it. > > No, my agreeing or not doesn't matter. If what you are doing is useful > (and I am not able to understand it), then you should make me > understand that and don't change your code. > > But, I really do not get the reasoning behind the logic. Please > elaborate that step by step. Please refer to above explaining, thank you. > > > > @@ -460,7 +470,9 @@ static int mtk_cpufreq_exit(struct cpufreq_policy *policy) > > > > { > > > > struct mtk_cpu_dvfs_info *info = policy->driver_data; > > > > > > > > - cpufreq_cooling_unregister(info->cdev); > > > > + if (info->cdev) > > > > + cpufreq_cooling_unregister(info->cdev); > > > > + > > > > > > Why do you need to update this? > > > > This is a error-checking to avoid user or any script by command line > > unplug CPU more than two times, we don't need to unregister cooling > > device on this case. > > > > I will remove it if you don't agree it. > > Same here. Please refer to the same reason as above. thank you. -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 30-11-15, 18:21, dawei chien wrote: > As far as I know, user or shell script has the right for using command online/offline cpu. Right. > Either user by console or shell script could make cpu2 online even cpu2 already onlined. Hey, no. You can't online that is already online. You can write '1' to the file /sys/devices/system/cpu/cpuX/online, but it wouldn't have any affect. > That could cause of_cpufreq_cooling_register execute many times for the > same cooling device. No. > "echo 1 > /sys/devices/system/cpu/cpu2/online" > > With above hotplug command, mtk_cpufreq_ready will register cooling > device again as well, but fail since the cooling device already created, > so we might need not register cooling device again on this case. Have you tested this yourself ?
On Mon, 2015-11-30 at 16:04 +0530, Viresh Kumar wrote: > On 30-11-15, 18:21, dawei chien wrote: > > As far as I know, user or shell script has the right for using command online/offline cpu. > > Right. > > > Either user by console or shell script could make cpu2 online even cpu2 already onlined. > > Hey, no. You can't online that is already online. You can write '1' to > the file /sys/devices/system/cpu/cpuX/online, but it wouldn't have any > affect. > > > That could cause of_cpufreq_cooling_register execute many times for the > > same cooling device. > > No. You are right. certainly no, it's my misunderstand. > > "echo 1 > /sys/devices/system/cpu/cpu2/online" > > > > With above hotplug command, mtk_cpufreq_ready will register cooling > > device again as well, but fail since the cooling device already created, > > so we might need not register cooling device again on this case. > > Have you tested this yourself ? Truly sorry for misunderstanding, so we don't need to add error checking code there. (It seems whole of cluster offline, the cooling device unregister as well. If just one cpu online, cooling device register as well) Three months ago, Once I used following command, cooling device can't create any more. err message will show once I echo online command. However, this bug already fixed in mainline by you. echo 0 > /sys/devices/system/cpu/cpu2/online echo 0 > /sys/devices/system/cpu/cpu3/online echo 1 > /sys/devices/system/cpu/cpu2/online commit 528464eaa46ae1bd319882e4dd3495802e55b8c4 Author: Viresh Kumar <viresh.kumar@linaro.org> Date: Thu Jul 23 14:32:32 2015 +0530 thermal: remove dangling 'weight_attr' device file -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/cpufreq/mt8173-cpufreq.c b/drivers/cpufreq/mt8173-cpufreq.c index 83001dc..4d39468 100644 --- a/drivers/cpufreq/mt8173-cpufreq.c +++ b/drivers/cpufreq/mt8173-cpufreq.c @@ -263,24 +263,34 @@ static int mtk_cpufreq_set_target(struct cpufreq_policy *policy, return 0; } +#define DYNAMIC_POWER "dynamic-power-coefficient" + static void mtk_cpufreq_ready(struct cpufreq_policy *policy) { struct mtk_cpu_dvfs_info *info = policy->driver_data; struct device_node *np = of_node_get(info->cpu_dev->of_node); + u32 capacitance; if (WARN_ON(!np)) return; if (of_find_property(np, "#cooling-cells", NULL)) { - info->cdev = of_cpufreq_cooling_register(np, - policy->related_cpus); + if (!info->cdev) { + of_property_read_u32(np, DYNAMIC_POWER, &capacitance); + + info->cdev = of_cpufreq_power_cooling_register(np, + policy->related_cpus, + capacitance, + NULL); - if (IS_ERR(info->cdev)) { - dev_err(info->cpu_dev, - "running cpufreq without cooling device: %ld\n", - PTR_ERR(info->cdev)); + if (IS_ERR(info->cdev)) { + dev_err(info->cpu_dev, + "running cpufreq without cooling device: %ld\n", + PTR_ERR(info->cdev)); - info->cdev = NULL; + info->cdev = NULL; + } } } @@ -460,7 +470,9 @@ static int mtk_cpufreq_exit(struct cpufreq_policy *policy)
MT8173 cpufreq driver use of_cpufreq_power_cooling_register registering cooling devices with dynamic power coefficient. Signed-off-by: Dawei Chien <dawei.chien@mediatek.com> --- This patch is base on patchset: https://lkml.org/lkml/2015/11/17/251 --- drivers/cpufreq/mt8173-cpufreq.c | 28 ++++++++++++++++++++-------- 1 file changed, 20 insertions(+), 8 deletions(-) { struct mtk_cpu_dvfs_info *info = policy->driver_data; - cpufreq_cooling_unregister(info->cdev); + if (info->cdev) + cpufreq_cooling_unregister(info->cdev); + dev_pm_opp_free_cpufreq_table(info->cpu_dev, &policy->freq_table); mtk_cpu_dvfs_info_release(info); kfree(info);