On Thu, Oct 22, 2015 at 08:02:38PM +0800, Dawei Chien wrote:
> This power model is base on Intelligent Power Allocation (IPA)
> technical, requires that the operating-points of the CPUs are
> registered using the kernel's opp library and the
> `cpufreq_frequency_table` is assigned to the `struct device`
> of the cpu MT8173.
>
> Signed-off-by: Dawei.Chien <[email protected]>
> ---
> This patch is base on
> https://patchwork.kernel.org/patch/7034601/
> ---
> drivers/cpufreq/mt8173-cpufreq.c | 152 ++++++++++++++++++++++++++++++++++++--
Given that you are proposing this on top a DT binding, why reading a
table of static power model applicable only to mt8173 cpufreq driver?
I have not seen anything specific (formula etc) that prevents this code
to be generalized to other CPUs.
Can you please help me to understand?
BR,
> 1 file changed, 144 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/cpufreq/mt8173-cpufreq.c b/drivers/cpufreq/mt8173-cpufreq.c
> index 49caed2..23c19c5 100644
> --- a/drivers/cpufreq/mt8173-cpufreq.c
> +++ b/drivers/cpufreq/mt8173-cpufreq.c
> @@ -29,6 +29,16 @@
> #define MAX_VOLT_LIMIT (1150000)
> #define VOLT_TOL (10000)
>
> +struct mtk_cpu_static_power {
> + unsigned long voltage;
> + unsigned int power;
> +};
> +
> +static struct mtk_cpu_static_power *mtk_ca53_static_power_table;
> +static struct mtk_cpu_static_power *mtk_ca57_static_power_table;
> +static int mtk_ca53_static_table_length;
> +static int mtk_ca57_static_table_length;
> +
> /*
> * The struct mtk_cpu_dvfs_info holds necessary information for doing CPU DVFS
> * on each CPU power/clock domain of Mediatek SoCs. Each CPU cluster in
> @@ -51,6 +61,110 @@ struct mtk_cpu_dvfs_info {
> bool need_voltage_tracking;
> };
>
> +unsigned int mtk_cpufreq_lookup_power(const struct mtk_cpu_static_power *table,
> + unsigned int count, unsigned long voltage)
> +{
> + int i;
> +
> + for (i = 0; i < count; i++) {
> + if (voltage <= table[i].voltage)
> + return table[i].power;
> + }
> +
> + return table[count - 1].power;
> +}
> +
> +int mtk_cpufreq_get_static(cpumask_t *cpumask, int interval,
> + unsigned long voltage, u32 *power)
> +{
> + int nr_cpus = cpumask_weight(cpumask);
> +
> + *power = 0;
> +
> + if (nr_cpus) {
> + if (cpumask_test_cpu(0, cpumask))
> + *power += mtk_cpufreq_lookup_power(
> + mtk_ca53_static_power_table,
> + mtk_ca53_static_table_length,
> + voltage);
> +
> + if (cpumask_test_cpu(2, cpumask))
> + *power += mtk_cpufreq_lookup_power(
> + mtk_ca57_static_power_table,
> + mtk_ca57_static_table_length,
> + voltage);
> + }
> +
> + return 0;
> +}
> +
> +unsigned int mtk_get_power_table_info(struct cpufreq_policy *policy,
> + struct device_node *np, const char *node_name)
> +{
> + int mtk_static_table_length;
> + const struct property *prop;
> + struct mtk_cpu_dvfs_info *info = policy->driver_data;
> + struct device *cpu_dev = info->cpu_dev;
> + const __be32 *val;
> + int nr, i;
> +
> + prop = of_find_property(np, node_name, NULL);
> +
> + if (!prop) {
> + pr_err("failed to get static-power-points\n");
> + return -ENODEV;
> + }
> +
> + if (!prop->value) {
> + pr_err("failed to get static power array data\n");
> + return -EINVAL;
> + }
> +
> + nr = prop->length / sizeof(u32);
> +
> + if (nr % 2) {
> + pr_err("Invalid OPP list\n");
> + return -EINVAL;
> + }
> +
> + mtk_static_table_length = nr / 2;
> +
> + if (cpumask_test_cpu(0, policy->related_cpus)) {
> + mtk_ca53_static_table_length = mtk_static_table_length;
> + mtk_ca53_static_power_table = devm_kcalloc(cpu_dev,
> + mtk_static_table_length,
> + sizeof(*mtk_ca53_static_power_table),
> + GFP_KERNEL);
> +
> + val = prop->value;
> + for (i = 0; i < mtk_static_table_length; ++i) {
> + unsigned long voltage = be32_to_cpup(val++);
> + unsigned int power = be32_to_cpup(val++);
> +
> + mtk_ca53_static_power_table[i].voltage = voltage;
> + mtk_ca53_static_power_table[i].power = power;
> + pr_info("volt:%ld uv, power:%d mW\n", voltage, power);
> + }
> + } else {
> + mtk_ca57_static_table_length = mtk_static_table_length;
> + mtk_ca57_static_power_table = devm_kcalloc(cpu_dev,
> + mtk_static_table_length,
> + sizeof(*mtk_ca57_static_power_table),
> + GFP_KERNEL);
> + val = prop->value;
> + for (i = 0; i < mtk_static_table_length; ++i) {
> + unsigned long voltage = be32_to_cpup(val++);
> + unsigned int power = be32_to_cpup(val++);
> +
> + mtk_ca57_static_power_table[i].voltage = voltage;
> + mtk_ca57_static_power_table[i].power = power;
> + pr_info("volt:%ld uv, power:%d mW\n", voltage, power);
> + }
> + }
> +
> + return 0;
> +}
> +
> static int mtk_cpufreq_voltage_tracking(struct mtk_cpu_dvfs_info *info,
> int new_vproc)
> {
> @@ -267,20 +381,40 @@ 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;
> + int ret;
>
> if (WARN_ON(!np))
> return;
>
> if (of_find_property(np, "#cooling-cells", NULL)) {
> - info->cdev = of_cpufreq_cooling_register(np,
> - policy->related_cpus);
>
> - if (IS_ERR(info->cdev)) {
> - dev_err(info->cpu_dev,
> - "running cpufreq without cooling device: %ld\n",
> - PTR_ERR(info->cdev));
> + if (!info->cdev) {
> +
> + of_property_read_u32(np,
> + "dynamic-power-coefficient",
> + &capacitance);
> +
> + ret = mtk_get_power_table_info(policy, np,
> + "static-power-points");
> + if (ret) {
> + dev_err(info->cpu_dev,
> + "cpufreq without static-points: %d\n",
> + ret);
> + }
> +
> + info->cdev = of_cpufreq_power_cooling_register(np,
> + policy->related_cpus,
> + capacitance,
> + mtk_cpufreq_get_static);
> +
> + if (IS_ERR(info->cdev)) {
> + dev_err(info->cpu_dev,
> + "cpufreq without cdev: %ld\n",
> + PTR_ERR(info->cdev));
> + info->cdev = NULL;
> + }
>
> - info->cdev = NULL;
> }
> }
>
> @@ -460,7 +594,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);
> +
> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2015-11-04 at 11:40 -0800, Eduardo Valentin wrote:
> On Thu, Oct 22, 2015 at 08:02:38PM +0800, Dawei Chien wrote:
> > This power model is base on Intelligent Power Allocation (IPA)
> > technical, requires that the operating-points of the CPUs are
> > registered using the kernel's opp library and the
> > `cpufreq_frequency_table` is assigned to the `struct device`
> > of the cpu MT8173.
> >
> > Signed-off-by: Dawei.Chien <[email protected]>
> > ---
> > This patch is base on
> > https://patchwork.kernel.org/patch/7034601/
> > ---
> > drivers/cpufreq/mt8173-cpufreq.c | 152 ++++++++++++++++++++++++++++++++++++--
>
> Given that you are proposing this on top a DT binding, why reading a
> table of static power model applicable only to mt8173 cpufreq driver?
>
> I have not seen anything specific (formula etc) that prevents this code
> to be generalized to other CPUs.
>
> Can you please help me to understand?
>
> BR,
This is because our platform currently only support mt8173_cpufreq.c, so
that I only add static power model for our owner IC.
Please understanding that I wouldn't give a DT binding document since I
will remove static power table on next version, but I can try to explain
it.
As far as I know, static power is somewhat leakage of CPU clusters, so
that we hardly to find a formula, which can suitable all kinds of CPUs
since leakage is different. In ARM IPA framework, static power only need
to be defined by who register cpufreq_power_cooling_register. The
voltage/power table is just one way to present leakage power of CPUs.
Actually, static power is optional since dynamic power is much more than
static power.
> > 1 file changed, 144 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/cpufreq/mt8173-cpufreq.c b/drivers/cpufreq/mt8173-cpufreq.c
> > index 49caed2..23c19c5 100644
> > --- a/drivers/cpufreq/mt8173-cpufreq.c
> > +++ b/drivers/cpufreq/mt8173-cpufreq.c
> > @@ -29,6 +29,16 @@
> > #define MAX_VOLT_LIMIT (1150000)
> > #define VOLT_TOL (10000)
> >
> > +struct mtk_cpu_static_power {
> > + unsigned long voltage;
> > + unsigned int power;
> > +};
> > +
> > +static struct mtk_cpu_static_power *mtk_ca53_static_power_table;
> > +static struct mtk_cpu_static_power *mtk_ca57_static_power_table;
> > +static int mtk_ca53_static_table_length;
> > +static int mtk_ca57_static_table_length;
> > +
> > /*
> > * The struct mtk_cpu_dvfs_info holds necessary information for doing CPU DVFS
> > * on each CPU power/clock domain of Mediatek SoCs. Each CPU cluster in
> > @@ -51,6 +61,110 @@ struct mtk_cpu_dvfs_info {
> > bool need_voltage_tracking;
> > };
> >
> > +unsigned int mtk_cpufreq_lookup_power(const struct mtk_cpu_static_power *table,
> > + unsigned int count, unsigned long voltage)
> > +{
> > + int i;
> > +
> > + for (i = 0; i < count; i++) {
> > + if (voltage <= table[i].voltage)
> > + return table[i].power;
> > + }
> > +
> > + return table[count - 1].power;
> > +}
> > +
> > +int mtk_cpufreq_get_static(cpumask_t *cpumask, int interval,
> > + unsigned long voltage, u32 *power)
> > +{
> > + int nr_cpus = cpumask_weight(cpumask);
> > +
> > + *power = 0;
> > +
> > + if (nr_cpus) {
> > + if (cpumask_test_cpu(0, cpumask))
> > + *power += mtk_cpufreq_lookup_power(
> > + mtk_ca53_static_power_table,
> > + mtk_ca53_static_table_length,
> > + voltage);
> > +
> > + if (cpumask_test_cpu(2, cpumask))
> > + *power += mtk_cpufreq_lookup_power(
> > + mtk_ca57_static_power_table,
> > + mtk_ca57_static_table_length,
> > + voltage);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +unsigned int mtk_get_power_table_info(struct cpufreq_policy *policy,
> > + struct device_node *np, const char *node_name)
> > +{
> > + int mtk_static_table_length;
> > + const struct property *prop;
> > + struct mtk_cpu_dvfs_info *info = policy->driver_data;
> > + struct device *cpu_dev = info->cpu_dev;
> > + const __be32 *val;
> > + int nr, i;
> > +
> > + prop = of_find_property(np, node_name, NULL);
> > +
> > + if (!prop) {
> > + pr_err("failed to get static-power-points\n");
> > + return -ENODEV;
> > + }
> > +
> > + if (!prop->value) {
> > + pr_err("failed to get static power array data\n");
> > + return -EINVAL;
> > + }
> > +
> > + nr = prop->length / sizeof(u32);
> > +
> > + if (nr % 2) {
> > + pr_err("Invalid OPP list\n");
> > + return -EINVAL;
> > + }
> > +
> > + mtk_static_table_length = nr / 2;
> > +
> > + if (cpumask_test_cpu(0, policy->related_cpus)) {
> > + mtk_ca53_static_table_length = mtk_static_table_length;
> > + mtk_ca53_static_power_table = devm_kcalloc(cpu_dev,
> > + mtk_static_table_length,
> > + sizeof(*mtk_ca53_static_power_table),
> > + GFP_KERNEL);
> > +
> > + val = prop->value;
> > + for (i = 0; i < mtk_static_table_length; ++i) {
> > + unsigned long voltage = be32_to_cpup(val++);
> > + unsigned int power = be32_to_cpup(val++);
> > +
> > + mtk_ca53_static_power_table[i].voltage = voltage;
> > + mtk_ca53_static_power_table[i].power = power;
> > + pr_info("volt:%ld uv, power:%d mW\n", voltage, power);
> > + }
> > + } else {
> > + mtk_ca57_static_table_length = mtk_static_table_length;
> > + mtk_ca57_static_power_table = devm_kcalloc(cpu_dev,
> > + mtk_static_table_length,
> > + sizeof(*mtk_ca57_static_power_table),
> > + GFP_KERNEL);
> > + val = prop->value;
> > + for (i = 0; i < mtk_static_table_length; ++i) {
> > + unsigned long voltage = be32_to_cpup(val++);
> > + unsigned int power = be32_to_cpup(val++);
> > +
> > + mtk_ca57_static_power_table[i].voltage = voltage;
> > + mtk_ca57_static_power_table[i].power = power;
> > + pr_info("volt:%ld uv, power:%d mW\n", voltage, power);
> > + }
> > + }
> > +
> > + return 0;
> > +}
> > +
> > static int mtk_cpufreq_voltage_tracking(struct mtk_cpu_dvfs_info *info,
> > int new_vproc)
> > {
> > @@ -267,20 +381,40 @@ 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;
> > + int ret;
> >
> > if (WARN_ON(!np))
> > return;
> >
> > if (of_find_property(np, "#cooling-cells", NULL)) {
> > - info->cdev = of_cpufreq_cooling_register(np,
> > - policy->related_cpus);
> >
> > - if (IS_ERR(info->cdev)) {
> > - dev_err(info->cpu_dev,
> > - "running cpufreq without cooling device: %ld\n",
> > - PTR_ERR(info->cdev));
> > + if (!info->cdev) {
> > +
> > + of_property_read_u32(np,
> > + "dynamic-power-coefficient",
> > + &capacitance);
> > +
> > + ret = mtk_get_power_table_info(policy, np,
> > + "static-power-points");
> > + if (ret) {
> > + dev_err(info->cpu_dev,
> > + "cpufreq without static-points: %d\n",
> > + ret);
> > + }
> > +
> > + info->cdev = of_cpufreq_power_cooling_register(np,
> > + policy->related_cpus,
> > + capacitance,
> > + mtk_cpufreq_get_static);
> > +
> > + if (IS_ERR(info->cdev)) {
> > + dev_err(info->cpu_dev,
> > + "cpufreq without cdev: %ld\n",
> > + PTR_ERR(info->cdev));
> > + info->cdev = NULL;
> > + }
> >
> > - info->cdev = NULL;
> > }
> > }
> >
> > @@ -460,7 +594,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);
> > +
> > 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 [email protected]
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
Cc'ing Javi (which you should have as he wrote the power-thing for
cpu-cooling).
On 05-11-15, 19:10, dawei chien wrote:
> This is because our platform currently only support mt8173_cpufreq.c, so
> that I only add static power model for our owner IC.
Bindings are (normally) supposed to be general than a platform
specific.
> Please understanding that I wouldn't give a DT binding document since I
> will remove static power table on next version, but I can try to explain
> it.
Then just don't add things in the first place.
> As far as I know, static power is somewhat leakage of CPU clusters, so
> that we hardly to find a formula, which can suitable all kinds of CPUs
> since leakage is different. In ARM IPA framework, static power only need
> to be defined by who register cpufreq_power_cooling_register. The
> voltage/power table is just one way to present leakage power of CPUs.
The bindings don't fix the values for static power, but just provides
a field for platforms to use. Everyone can then send its own power
figures. Why do you thing it can't be generalized?
> Actually, static power is optional since dynamic power is much more than
> static power.
Maybe, we should still capture it.
@Javi ?
--
viresh
On Fri, Nov 06, 2015 at 08:54:33AM +0530, Viresh Kumar wrote:
> Cc'ing Javi (which you should have as he wrote the power-thing for
> cpu-cooling).
>
> On 05-11-15, 19:10, dawei chien wrote:
> > This is because our platform currently only support mt8173_cpufreq.c, so
> > that I only add static power model for our owner IC.
>
> Bindings are (normally) supposed to be general than a platform
> specific.
>
> > Please understanding that I wouldn't give a DT binding document since I
> > will remove static power table on next version, but I can try to explain
> > it.
>
> Then just don't add things in the first place.
>
> > As far as I know, static power is somewhat leakage of CPU clusters, so
> > that we hardly to find a formula, which can suitable all kinds of CPUs
> > since leakage is different. In ARM IPA framework, static power only need
> > to be defined by who register cpufreq_power_cooling_register. The
> > voltage/power table is just one way to present leakage power of CPUs.
>
> The bindings don't fix the values for static power, but just provides
> a field for platforms to use. Everyone can then send its own power
> figures. Why do you thing it can't be generalized?
The way they are described here is useful only for this platform, but
it's not generic. It only takes into account voltage as (I assume)
it's the only variable that affects it in this implementation. A
generalized version of the static power should take into account the
temperature and the idle state.
> > Actually, static power is optional since dynamic power is much more than
> > static power.
>
> Maybe, we should still capture it.
>
> @Javi ?
It really depends on the platform. If dawei says that for their
platform static power is negligible then it is ok to not capture it.
Cheers,
Javi
On Tue, Nov 10, 2015 at 11:20:18AM +0000, Javi Merino wrote:
> On Fri, Nov 06, 2015 at 08:54:33AM +0530, Viresh Kumar wrote:
> > Cc'ing Javi (which you should have as he wrote the power-thing for
> > cpu-cooling).
> >
> > On 05-11-15, 19:10, dawei chien wrote:
> > > This is because our platform currently only support mt8173_cpufreq.c, so
> > > that I only add static power model for our owner IC.
> >
> > Bindings are (normally) supposed to be general than a platform
> > specific.
> >
> > > Please understanding that I wouldn't give a DT binding document since I
> > > will remove static power table on next version, but I can try to explain
> > > it.
> >
> > Then just don't add things in the first place.
> >
> > > As far as I know, static power is somewhat leakage of CPU clusters, so
> > > that we hardly to find a formula, which can suitable all kinds of CPUs
> > > since leakage is different. In ARM IPA framework, static power only need
> > > to be defined by who register cpufreq_power_cooling_register. The
> > > voltage/power table is just one way to present leakage power of CPUs.
> >
> > The bindings don't fix the values for static power, but just provides
> > a field for platforms to use. Everyone can then send its own power
> > figures. Why do you thing it can't be generalized?
>
> The way they are described here is useful only for this platform, but
> it's not generic. It only takes into account voltage as (I assume)
> it's the only variable that affects it in this implementation. A
> generalized version of the static power should take into account the
> temperature and the idle state.
Still, why would we have one binding to describe static power per platform?
I would prefer we go towards a generalized binding description.
If temperature is not needed on all platforms, make it an optional
property.
BR,
Eduardo Valentin
On Tue, Nov 10, 2015 at 10:41:22AM -0800, Eduardo Valentin wrote:
> On Tue, Nov 10, 2015 at 11:20:18AM +0000, Javi Merino wrote:
> > On Fri, Nov 06, 2015 at 08:54:33AM +0530, Viresh Kumar wrote:
> > > Cc'ing Javi (which you should have as he wrote the power-thing for
> > > cpu-cooling).
> > >
> > > On 05-11-15, 19:10, dawei chien wrote:
> > > > This is because our platform currently only support mt8173_cpufreq.c, so
> > > > that I only add static power model for our owner IC.
> > >
> > > Bindings are (normally) supposed to be general than a platform
> > > specific.
> > >
> > > > Please understanding that I wouldn't give a DT binding document since I
> > > > will remove static power table on next version, but I can try to explain
> > > > it.
> > >
> > > Then just don't add things in the first place.
> > >
> > > > As far as I know, static power is somewhat leakage of CPU clusters, so
> > > > that we hardly to find a formula, which can suitable all kinds of CPUs
> > > > since leakage is different. In ARM IPA framework, static power only need
> > > > to be defined by who register cpufreq_power_cooling_register. The
> > > > voltage/power table is just one way to present leakage power of CPUs.
> > >
> > > The bindings don't fix the values for static power, but just provides
> > > a field for platforms to use. Everyone can then send its own power
> > > figures. Why do you thing it can't be generalized?
> >
> > The way they are described here is useful only for this platform, but
> > it's not generic. It only takes into account voltage as (I assume)
> > it's the only variable that affects it in this implementation. A
> > generalized version of the static power should take into account the
> > temperature and the idle state.
>
> Still, why would we have one binding to describe static power per platform?
>
> I would prefer we go towards a generalized binding description.
Sure, I wasn't saying that we want one binding per platform. I was
just saying that this binding can't be made generic because it's not.
Cheers,
Javi
On 10-11-15, 11:20, Javi Merino wrote:
> The way they are described here is useful only for this platform, but
> it's not generic. It only takes into account voltage as (I assume)
> it's the only variable that affects it in this implementation. A
> generalized version of the static power should take into account the
> temperature and the idle state.
yeah, but I thought we are talking about the final static power being
present in the DT. How the platform gets it, doesn't matter at all.
They may consider just the voltage or temperature and idle state as
well.
--
viresh