Re: [PATCH v5 5/5] drm/panfrost: Register devfreq cooling and attempt to add Energy Model

Re: [PATCH v5 5/5] drm/panfrost: Register devfreq cooling and attempt to add Energy Model

[Alyssa Rosenzweig]

[-- Attachment #1.1: Type: text/plain, Size: 1368 bytes --]

Reviewed-by: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>

On Wed, Mar 18, 2020 at 11:45:48AM +0000, Lukasz Luba wrote:
> Register devfreq cooling device and attempt to register Energy Model. This
> will add the devfreq device to the Energy Model framework. It will create
> a dedicated and unified data structures used i.e. in thermal framework.
> The last NULL parameter indicates that the power model is simplified and
> created based on DT 'dynamic-power-coefficient', voltage and frequency.
> 
> Reviewed-by: Steven Price <steven.price@arm.com>
> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
> ---
>  drivers/gpu/drm/panfrost/panfrost_devfreq.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> index 413987038fbf..8759a73db153 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> @@ -105,7 +105,7 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev)
>  	}
>  	pfdev->devfreq.devfreq = devfreq;
>  
> -	cooling = of_devfreq_cooling_register(dev->of_node, devfreq);
> +	cooling = devfreq_cooling_em_register(devfreq, NULL);
>  	if (IS_ERR(cooling))
>  		DRM_DEV_INFO(dev, "Failed to register cooling device\n");
>  	else
> -- 
> 2.17.1
> 

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v5 5/5] drm/panfrost: Register devfreq cooling and attempt to add Energy Model

[Lukasz Luba]

On 3/18/20 1:11 PM, Alyssa Rosenzweig wrote:
> Reviewed-by: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>

Thank you Alyssa for the review.

Regards,
Lukasz

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v5 2/5] OPP: refactor dev_pm_opp_of_register_em() and update related drivers

[Lukasz Luba]

On 3/18/20 11:45 AM, Lukasz Luba wrote:
> The Energy Model framework supports both: CPUs and devfreq devices. Drop
> the CPU specific interface with cpumask and add struct device. Add also a
> return value. This new interface provides easy way to create a simple
> Energy Model, which then might be used in i.e. thermal subsystem.
> 
> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
> ---
>   drivers/cpufreq/cpufreq-dt.c           |  2 +-
>   drivers/cpufreq/imx6q-cpufreq.c        |  2 +-
>   drivers/cpufreq/mediatek-cpufreq.c     |  2 +-
>   drivers/cpufreq/omap-cpufreq.c         |  2 +-
>   drivers/cpufreq/qcom-cpufreq-hw.c      |  2 +-
>   drivers/cpufreq/scpi-cpufreq.c         |  2 +-
>   drivers/cpufreq/vexpress-spc-cpufreq.c |  2 +-
>   drivers/opp/of.c                       | 71 ++++++++++++++++----------
>   include/linux/pm_opp.h                 | 15 +++++-
>   9 files changed, 65 insertions(+), 35 deletions(-)


Gentle ping.

Viresh could you have a look at it?

Regards,
Lukasz

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v5 4/5] thermal: devfreq_cooling: Refactor code and switch to use Energy Model

[Lukasz Luba]

On 3/18/20 11:45 AM, Lukasz Luba wrote:
> The overhauled Energy Model (EM) framework support also devfreq devices.
> The unified API interface of the EM can be used in the thermal subsystem to
> not duplicate code. The power table now is taken from EM structure and
> there is no need to maintain calculation for it locally. In case when the
> EM is not provided by the device a simple interface for cooling device is
> used.
> 
> There is also an improvement in code related to enabling/disabling OPPs,
> which prevents from race condition with devfreq governors.
> 
> [lkp: Reported the build warning]
> Reported-by: kbuild test robot <lkp@intel.com>
> Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org> # for tracing code
> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
> ---
>   drivers/thermal/devfreq_cooling.c | 474 ++++++++++++++++--------------
>   include/linux/devfreq_cooling.h   |  39 +--
>   include/trace/events/thermal.h    |  19 +-
>   3 files changed, 277 insertions(+), 255 deletions(-)
> 

Gentle ping.

Daniel or Amit could you have a look at this patch?

Regards,
Lukasz

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v5 1/5] PM / EM: add devices to Energy Model

[Daniel Lezcano]

Hi Lukasz,


On 18/03/2020 12:45, Lukasz Luba wrote:
> Add support of other devices into the Energy Model framework not only the
> CPUs. Change the interface to be more unified which can handle other
> devices as well.

thanks for taking care of that. Overall I like the changes in this patch
but it hard to review in details because the patch is too big :/

Could you split this patch into smaller ones?

eg. (at your convenience)

 - One patch renaming s/cap/perf/

 - One patch adding a new function:

    em_dev_register_perf_domain(struct device *dev,
				unsigned int nr_states,
				struct em_data_callback *cb);

   (+ EXPORT_SYMBOL_GPL)

    And em_register_perf_domain() using it.

 - One converting the em_register_perf_domain() user to
	em_dev_register_perf_domain

 - One adding the different new 'em' functions

 - And finally one removing em_register_perf_domain().


> Acked-by: Quentin Perret <qperret@google.com>
> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
> ---

[ ... ]

>  2. Core APIs
> @@ -70,14 +72,16 @@ CONFIG_ENERGY_MODEL must be enabled to use the EM framework.
>  Drivers are expected to register performance domains into the EM framework by
>  calling the following API::
>  
> -  int em_register_perf_domain(cpumask_t *span, unsigned int nr_states,
> -			      struct em_data_callback *cb);
> +  int em_register_perf_domain(struct device *dev, unsigned int nr_states,
> +		struct em_data_callback *cb, cpumask_t *cpus);

Isn't possible to get rid of this cpumask by using
cpufreq_cpu_get() which returns the cpufreq's policy and from their get
the related cpus ?

[ ... ]


-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v5 2/5] OPP: refactor dev_pm_opp_of_register_em() and update related drivers

[Daniel Lezcano]

On 18/03/2020 12:45, Lukasz Luba wrote:
> The Energy Model framework supports both: CPUs and devfreq devices. Drop
> the CPU specific interface with cpumask and add struct device. Add also a
> return value. This new interface provides easy way to create a simple
> Energy Model, which then might be used in i.e. thermal subsystem.

This patch contains too many different changes.

There are fixes and traces added in addition to a function prototype change.

Please provide patches separated by logical changes.

If the cpumask is extracted in the underlying function
em_register_perf_domain() as suggested in the previous patch 1/5,
dev_pm_opp_of_register_em() can be struct device centric only.

> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
> ---
>  drivers/cpufreq/cpufreq-dt.c           |  2 +-
>  drivers/cpufreq/imx6q-cpufreq.c        |  2 +-
>  drivers/cpufreq/mediatek-cpufreq.c     |  2 +-
>  drivers/cpufreq/omap-cpufreq.c         |  2 +-
>  drivers/cpufreq/qcom-cpufreq-hw.c      |  2 +-
>  drivers/cpufreq/scpi-cpufreq.c         |  2 +-
>  drivers/cpufreq/vexpress-spc-cpufreq.c |  2 +-
>  drivers/opp/of.c                       | 71 ++++++++++++++++----------
>  include/linux/pm_opp.h                 | 15 +++++-
>  9 files changed, 65 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c
> index 26fe8dfb9ce6..f9f03fd49b83 100644
> --- a/drivers/cpufreq/cpufreq-dt.c
> +++ b/drivers/cpufreq/cpufreq-dt.c
> @@ -275,7 +275,7 @@ static int cpufreq_init(struct cpufreq_policy *policy)
>  	policy->cpuinfo.transition_latency = transition_latency;
>  	policy->dvfs_possible_from_any_cpu = true;
>  
> -	dev_pm_opp_of_register_em(policy->cpus);
> +	dev_pm_opp_of_register_em(cpu_dev, policy->cpus);
>  
>  	return 0;
>  
> diff --git a/drivers/cpufreq/imx6q-cpufreq.c b/drivers/cpufreq/imx6q-cpufreq.c
> index 285b8e9aa185..9764abf17ce3 100644
> --- a/drivers/cpufreq/imx6q-cpufreq.c
> +++ b/drivers/cpufreq/imx6q-cpufreq.c
> @@ -193,7 +193,7 @@ static int imx6q_cpufreq_init(struct cpufreq_policy *policy)
>  	policy->clk = clks[ARM].clk;
>  	cpufreq_generic_init(policy, freq_table, transition_latency);
>  	policy->suspend_freq = max_freq;
> -	dev_pm_opp_of_register_em(policy->cpus);
> +	dev_pm_opp_of_register_em(cpu_dev, policy->cpus);
>  
>  	return 0;
>  }
> diff --git a/drivers/cpufreq/mediatek-cpufreq.c b/drivers/cpufreq/mediatek-cpufreq.c
> index 0c98dd08273d..7d1212c9b7c8 100644
> --- a/drivers/cpufreq/mediatek-cpufreq.c
> +++ b/drivers/cpufreq/mediatek-cpufreq.c
> @@ -448,7 +448,7 @@ static int mtk_cpufreq_init(struct cpufreq_policy *policy)
>  	policy->driver_data = info;
>  	policy->clk = info->cpu_clk;
>  
> -	dev_pm_opp_of_register_em(policy->cpus);
> +	dev_pm_opp_of_register_em(info->cpu_dev, policy->cpus);
>  
>  	return 0;
>  }
> diff --git a/drivers/cpufreq/omap-cpufreq.c b/drivers/cpufreq/omap-cpufreq.c
> index 8d14b42a8c6f..3694bb030df3 100644
> --- a/drivers/cpufreq/omap-cpufreq.c
> +++ b/drivers/cpufreq/omap-cpufreq.c
> @@ -131,7 +131,7 @@ static int omap_cpu_init(struct cpufreq_policy *policy)
>  
>  	/* FIXME: what's the actual transition time? */
>  	cpufreq_generic_init(policy, freq_table, 300 * 1000);
> -	dev_pm_opp_of_register_em(policy->cpus);
> +	dev_pm_opp_of_register_em(mpu_dev, policy->cpus);
>  
>  	return 0;
>  }
> diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
> index fc92a8842e25..0a04b6f03b9a 100644
> --- a/drivers/cpufreq/qcom-cpufreq-hw.c
> +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
> @@ -238,7 +238,7 @@ static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
>  		goto error;
>  	}
>  
> -	dev_pm_opp_of_register_em(policy->cpus);
> +	dev_pm_opp_of_register_em(cpu_dev, policy->cpus);
>  
>  	policy->fast_switch_possible = true;
>  
> diff --git a/drivers/cpufreq/scpi-cpufreq.c b/drivers/cpufreq/scpi-cpufreq.c
> index 20d1f85d5f5a..b0f5388b8854 100644
> --- a/drivers/cpufreq/scpi-cpufreq.c
> +++ b/drivers/cpufreq/scpi-cpufreq.c
> @@ -167,7 +167,7 @@ static int scpi_cpufreq_init(struct cpufreq_policy *policy)
>  
>  	policy->fast_switch_possible = false;
>  
> -	dev_pm_opp_of_register_em(policy->cpus);
> +	dev_pm_opp_of_register_em(cpu_dev, policy->cpus);
>  
>  	return 0;
>  
> diff --git a/drivers/cpufreq/vexpress-spc-cpufreq.c b/drivers/cpufreq/vexpress-spc-cpufreq.c
> index 83c85d3d67e3..4e8b1dee7c9a 100644
> --- a/drivers/cpufreq/vexpress-spc-cpufreq.c
> +++ b/drivers/cpufreq/vexpress-spc-cpufreq.c
> @@ -450,7 +450,7 @@ static int ve_spc_cpufreq_init(struct cpufreq_policy *policy)
>  	policy->freq_table = freq_table[cur_cluster];
>  	policy->cpuinfo.transition_latency = 1000000; /* 1 ms */
>  
> -	dev_pm_opp_of_register_em(policy->cpus);
> +	dev_pm_opp_of_register_em(cpu_dev, policy->cpus);
>  
>  	if (is_bL_switching_enabled())
>  		per_cpu(cpu_last_req_freq, policy->cpu) =
> diff --git a/drivers/opp/of.c b/drivers/opp/of.c
> index 0efd6cf6d023..f94d095113e7 100644
> --- a/drivers/opp/of.c
> +++ b/drivers/opp/of.c
> @@ -1036,18 +1036,18 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_get_of_node);
>  
>  /*
>   * Callback function provided to the Energy Model framework upon registration.
> - * This computes the power estimated by @CPU at @kHz if it is the frequency
> + * This computes the power estimated by @dev at @kHz if it is the frequency
>   * of an existing OPP, or at the frequency of the first OPP above @kHz otherwise
>   * (see dev_pm_opp_find_freq_ceil()). This function updates @kHz to the ceiled
>   * frequency and @mW to the associated power. The power is estimated as
> - * P = C * V^2 * f with C being the CPU's capacitance and V and f respectively
> - * the voltage and frequency of the OPP.
> + * P = C * V^2 * f with C being the device's capacitance and V and f
> + * respectively the voltage and frequency of the OPP.
>   *
> - * Returns -ENODEV if the CPU device cannot be found, -EINVAL if the power
> - * calculation failed because of missing parameters, 0 otherwise.
> + * Returns -EINVAL if the power calculation failed because of missing
> + * parameters, 0 otherwise.
>   */
> -static int __maybe_unused _get_cpu_power(unsigned long *mW, unsigned long *kHz,
> -					 struct device *cpu_dev)
> +static int __maybe_unused _get_power(unsigned long *mW, unsigned long *kHz,
> +				     struct device *dev)
>  {
>  	struct dev_pm_opp *opp;
>  	struct device_node *np;
> @@ -1056,7 +1056,7 @@ static int __maybe_unused _get_cpu_power(unsigned long *mW, unsigned long *kHz,
>  	u64 tmp;
>  	int ret;
>  
> -	np = of_node_get(cpu_dev->of_node);
> +	np = of_node_get(dev->of_node);
>  	if (!np)
>  		return -EINVAL;
>  
> @@ -1066,7 +1066,7 @@ static int __maybe_unused _get_cpu_power(unsigned long *mW, unsigned long *kHz,
>  		return -EINVAL;
>  
>  	Hz = *kHz * 1000;
> -	opp = dev_pm_opp_find_freq_ceil(cpu_dev, &Hz);
> +	opp = dev_pm_opp_find_freq_ceil(dev, &Hz);
>  	if (IS_ERR(opp))
>  		return -EINVAL;
>  
> @@ -1086,30 +1086,38 @@ static int __maybe_unused _get_cpu_power(unsigned long *mW, unsigned long *kHz,
>  
>  /**
>   * dev_pm_opp_of_register_em() - Attempt to register an Energy Model
> - * @cpus	: CPUs for which an Energy Model has to be registered
> + * @dev		: Device for which an Energy Model has to be registered
> + * @cpus	: CPUs for which an Energy Model has to be registered. For
> + *		other type of devices it should be set to NULL.
>   *
>   * This checks whether the "dynamic-power-coefficient" devicetree property has
>   * been specified, and tries to register an Energy Model with it if it has.
> + * Having this property means the voltages are known for OPPs and the EM
> + * might be calculated.
>   */
> -void dev_pm_opp_of_register_em(struct cpumask *cpus)
> +int dev_pm_opp_of_register_em(struct device *dev, struct cpumask *cpus)
>  {
> -	struct em_data_callback em_cb = EM_DATA_CB(_get_cpu_power);
> -	int ret, nr_opp, cpu = cpumask_first(cpus);
> -	struct device *cpu_dev;
> +	struct em_data_callback em_cb = EM_DATA_CB(_get_power);
>  	struct device_node *np;
> +	int ret, nr_opp;
>  	u32 cap;
>  
> -	cpu_dev = get_cpu_device(cpu);
> -	if (!cpu_dev)
> -		return;
> +	if (IS_ERR_OR_NULL(dev)) {
> +		ret = -EINVAL;
> +		goto failed;
> +	}
>  
> -	nr_opp = dev_pm_opp_get_opp_count(cpu_dev);
> -	if (nr_opp <= 0)
> -		return;
> +	nr_opp = dev_pm_opp_get_opp_count(dev);
> +	if (nr_opp <= 0) {
> +		ret = -EINVAL;
> +		goto failed;
> +	}
>  
> -	np = of_node_get(cpu_dev->of_node);
> -	if (!np)
> -		return;
> +	np = of_node_get(dev->of_node);
> +	if (!np) {
> +		ret = -EINVAL;
> +		goto failed;
> +	}
>  
>  	/*
>  	 * Register an EM only if the 'dynamic-power-coefficient' property is
> @@ -1120,9 +1128,20 @@ void dev_pm_opp_of_register_em(struct cpumask *cpus)
>  	 */
>  	ret = of_property_read_u32(np, "dynamic-power-coefficient", &cap);
>  	of_node_put(np);
> -	if (ret || !cap)
> -		return;
> +	if (ret || !cap) {
> +		dev_dbg(dev, "Couldn't find proper 'dynamic-power-coefficient' in DT\n");
> +		ret = -EINVAL;
> +		goto failed;
> +	}
>  
> -	em_register_perf_domain(cpu_dev, nr_opp, &em_cb, cpus);
> +	ret = em_register_perf_domain(dev, nr_opp, &em_cb, cpus);
> +	if (ret)
> +		goto failed;
> +
> +	return 0;
> +
> +failed:
> +	dev_dbg(dev, "Couldn't register Energy Model %d\n", ret);
> +	return ret;
>  }
>  EXPORT_SYMBOL_GPL(dev_pm_opp_of_register_em);
> diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
> index 747861816f4f..822ff9f52bf7 100644
> --- a/include/linux/pm_opp.h
> +++ b/include/linux/pm_opp.h
> @@ -11,6 +11,7 @@
>  #ifndef __LINUX_OPP_H__
>  #define __LINUX_OPP_H__
>  
> +#include <linux/energy_model.h>
>  #include <linux/err.h>
>  #include <linux/notifier.h>
>  
> @@ -360,7 +361,11 @@ int dev_pm_opp_of_get_sharing_cpus(struct device *cpu_dev, struct cpumask *cpuma
>  struct device_node *dev_pm_opp_of_get_opp_desc_node(struct device *dev);
>  struct device_node *dev_pm_opp_get_of_node(struct dev_pm_opp *opp);
>  int of_get_required_opp_performance_state(struct device_node *np, int index);
> -void dev_pm_opp_of_register_em(struct cpumask *cpus);
> +int dev_pm_opp_of_register_em(struct device *dev, struct cpumask *cpus);
> +static inline void dev_pm_opp_of_unregister_em(struct device *dev)
> +{
> +	em_unregister_perf_domain(dev);
> +}
>  #else
>  static inline int dev_pm_opp_of_add_table(struct device *dev)
>  {
> @@ -400,7 +405,13 @@ static inline struct device_node *dev_pm_opp_get_of_node(struct dev_pm_opp *opp)
>  	return NULL;
>  }
>  
> -static inline void dev_pm_opp_of_register_em(struct cpumask *cpus)
> +static inline int dev_pm_opp_of_register_em(struct device *dev,
> +					    struct cpumask *cpus)
> +{
> +	return -ENOTSUPP;
> +}
> +
> +static inline void dev_pm_opp_of_unregister_em(struct device *dev)
>  {
>  }
>  
> 


-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v5 3/5] thermal: devfreq_cooling: Use PM QoS to set frequency limits

[Daniel Lezcano]

On 18/03/2020 12:45, Lukasz Luba wrote:
> From: Matthias Kaehlcke <mka@chromium.org>
> 
> Now that devfreq supports limiting the frequency range of a device
> through PM QoS make use of it instead of disabling OPPs that should
> not be used.
> 
> The switch from disabling OPPs to PM QoS introduces a subtle behavioral
> change in case of conflicting requests (min > max): PM QoS gives
> precedence to the MIN_FREQUENCY request, while higher OPPs disabled
> with dev_pm_opp_disable() would override MIN_FREQUENCY.
> 
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>
> Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>

This patch is standalone, right? If yes, I will apply it.



-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v5 3/5] thermal: devfreq_cooling: Use PM QoS to set frequency limits

[Matthias Kaehlcke]

Hi Daniel,

On Fri, Apr 03, 2020 at 06:43:20PM +0200, Daniel Lezcano wrote:
> On 18/03/2020 12:45, Lukasz Luba wrote:
> > From: Matthias Kaehlcke <mka@chromium.org>
> > 
> > Now that devfreq supports limiting the frequency range of a device
> > through PM QoS make use of it instead of disabling OPPs that should
> > not be used.
> > 
> > The switch from disabling OPPs to PM QoS introduces a subtle behavioral
> > change in case of conflicting requests (min > max): PM QoS gives
> > precedence to the MIN_FREQUENCY request, while higher OPPs disabled
> > with dev_pm_opp_disable() would override MIN_FREQUENCY.
> > 
> > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> > Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>
> > Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
> 
> This patch is standalone, right? If yes, I will apply it.

Yes, it is standalone, please apply

Thanks

Matthias

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v5 3/5] thermal: devfreq_cooling: Use PM QoS to set frequency limits

[Daniel Lezcano]

On 03/04/2020 19:18, Matthias Kaehlcke wrote:
> Hi Daniel,
> 
> On Fri, Apr 03, 2020 at 06:43:20PM +0200, Daniel Lezcano wrote:
>> On 18/03/2020 12:45, Lukasz Luba wrote:
>>> From: Matthias Kaehlcke <mka@chromium.org>
>>>
>>> Now that devfreq supports limiting the frequency range of a device
>>> through PM QoS make use of it instead of disabling OPPs that should
>>> not be used.
>>>
>>> The switch from disabling OPPs to PM QoS introduces a subtle behavioral
>>> change in case of conflicting requests (min > max): PM QoS gives
>>> precedence to the MIN_FREQUENCY request, while higher OPPs disabled
>>> with dev_pm_opp_disable() would override MIN_FREQUENCY.
>>>
>>> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
>>> Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>
>>> Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
>>
>> This patch is standalone, right? If yes, I will apply it.
> 
> Yes, it is standalone, please apply

Applied on 'testing', thanks


-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v5 4/5] thermal: devfreq_cooling: Refactor code and switch to use Energy Model

[Daniel Lezcano]

On 18/03/2020 12:45, Lukasz Luba wrote:
> The overhauled Energy Model (EM) framework support also devfreq devices.
> The unified API interface of the EM can be used in the thermal subsystem to
> not duplicate code. The power table now is taken from EM structure and
> there is no need to maintain calculation for it locally. In case when the
> EM is not provided by the device a simple interface for cooling device is
> used.
> 
> There is also an improvement in code related to enabling/disabling OPPs,
> which prevents from race condition with devfreq governors.
> 
> [lkp: Reported the build warning]
> Reported-by: kbuild test robot <lkp@intel.com>
> Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org> # for tracing code
> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>

The changes are too big, please split this patch into smaller chunks.

> ---
>  drivers/thermal/devfreq_cooling.c | 474 ++++++++++++++++--------------
>  include/linux/devfreq_cooling.h   |  39 +--
>  include/trace/events/thermal.h    |  19 +-
>  3 files changed, 277 insertions(+), 255 deletions(-)
> 
> diff --git a/drivers/thermal/devfreq_cooling.c b/drivers/thermal/devfreq_cooling.c

[ ... ]

>  struct devfreq_cooling_device {
>  	int id;
>  	struct thermal_cooling_device *cdev;
>  	struct devfreq *devfreq;
>  	unsigned long cooling_state;
> -	u32 *power_table;
>  	u32 *freq_table;
> -	size_t freq_table_size;
> +	size_t max_level;

Could you rename it to 'max_state' ?


[ ... ]


-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v5 1/5] PM / EM: add devices to Energy Model

[Lukasz Luba]

Hi Daniel,

Thank you for the review.

On 4/3/20 5:05 PM, Daniel Lezcano wrote:
> 
> Hi Lukasz,
> 
> 
> On 18/03/2020 12:45, Lukasz Luba wrote:
>> Add support of other devices into the Energy Model framework not only the
>> CPUs. Change the interface to be more unified which can handle other
>> devices as well.
> 
> thanks for taking care of that. Overall I like the changes in this patch
> but it hard to review in details because the patch is too big :/
> 
> Could you split this patch into smaller ones?
> 
> eg. (at your convenience)
> 
>   - One patch renaming s/cap/perf/
> 
>   - One patch adding a new function:
> 
>      em_dev_register_perf_domain(struct device *dev,
> 				unsigned int nr_states,
> 				struct em_data_callback *cb);
> 
>     (+ EXPORT_SYMBOL_GPL)
> 
>      And em_register_perf_domain() using it.
> 
>   - One converting the em_register_perf_domain() user to
> 	em_dev_register_perf_domain
> 
>   - One adding the different new 'em' functions
> 
>   - And finally one removing em_register_perf_domain().

I agree and will do the split. I could also break the dependencies
for future easier merge.

> 
> 
>> Acked-by: Quentin Perret <qperret@google.com>
>> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
>> ---
> 
> [ ... ]
> 
>>   2. Core APIs
>> @@ -70,14 +72,16 @@ CONFIG_ENERGY_MODEL must be enabled to use the EM framework.
>>   Drivers are expected to register performance domains into the EM framework by
>>   calling the following API::
>>   
>> -  int em_register_perf_domain(cpumask_t *span, unsigned int nr_states,
>> -			      struct em_data_callback *cb);
>> +  int em_register_perf_domain(struct device *dev, unsigned int nr_states,
>> +		struct em_data_callback *cb, cpumask_t *cpus);
> 
> Isn't possible to get rid of this cpumask by using
> cpufreq_cpu_get() which returns the cpufreq's policy and from their get
> the related cpus ?

We had similar thoughts with Quentin and I've checked this.
Unfortunately, if the policy is a 'new policy' [1] it gets
allocated and passed into cpufreq driver ->init(policy) [2].
Then that policy is set into per_cpu pointer for each related_cpu [3]:

for_each_cpu(j, policy->related_cpus)
	per_cpu(cpufreq_cpu_data, j) = policy;


Thus, any calls of functions (i.e. cpufreq_cpu_get()) which try to
take this ptr before [3] won't work.

We are trying to register EM from cpufreq_driver->init(policy) and the
per_cpu policy is likely to be not populated at that phase.

Regards,
Lukasz

[1] 
https://elixir.bootlin.com/linux/latest/source/drivers/cpufreq/cpufreq.c#L1328
[2] 
https://elixir.bootlin.com/linux/latest/source/drivers/cpufreq/cpufreq.c#L1350
[3] 
https://elixir.bootlin.com/linux/latest/source/drivers/cpufreq/cpufreq.c#L1374



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v5 4/5] thermal: devfreq_cooling: Refactor code and switch to use Energy Model

[Lukasz Luba]

On 4/3/20 6:44 PM, Daniel Lezcano wrote:
> On 18/03/2020 12:45, Lukasz Luba wrote:
>> The overhauled Energy Model (EM) framework support also devfreq devices.
>> The unified API interface of the EM can be used in the thermal subsystem to
>> not duplicate code. The power table now is taken from EM structure and
>> there is no need to maintain calculation for it locally. In case when the
>> EM is not provided by the device a simple interface for cooling device is
>> used.
>>
>> There is also an improvement in code related to enabling/disabling OPPs,
>> which prevents from race condition with devfreq governors.
>>
>> [lkp: Reported the build warning]
>> Reported-by: kbuild test robot <lkp@intel.com>
>> Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org> # for tracing code
>> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
> 
> The changes are too big, please split this patch into smaller chunks.

OK, I will split it and re-base on top of thermal testing.

> 
>> ---
>>   drivers/thermal/devfreq_cooling.c | 474 ++++++++++++++++--------------
>>   include/linux/devfreq_cooling.h   |  39 +--
>>   include/trace/events/thermal.h    |  19 +-
>>   3 files changed, 277 insertions(+), 255 deletions(-)
>>
>> diff --git a/drivers/thermal/devfreq_cooling.c b/drivers/thermal/devfreq_cooling.c
> 
> [ ... ]
> 
>>   struct devfreq_cooling_device {
>>   	int id;
>>   	struct thermal_cooling_device *cdev;
>>   	struct devfreq *devfreq;
>>   	unsigned long cooling_state;
>> -	u32 *power_table;
>>   	u32 *freq_table;
>> -	size_t freq_table_size;
>> +	size_t max_level;
> 
> Could you rename it to 'max_state' ?

Yes.

Thank you for your comments.

Regards,
Lukasz


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v5 2/5] OPP: refactor dev_pm_opp_of_register_em() and update related drivers

[Lukasz Luba]

Hi Daniel,

Thank you for your comments.

On 4/3/20 5:21 PM, Daniel Lezcano wrote:
> On 18/03/2020 12:45, Lukasz Luba wrote:
>> The Energy Model framework supports both: CPUs and devfreq devices. Drop
>> the CPU specific interface with cpumask and add struct device. Add also a
>> return value. This new interface provides easy way to create a simple
>> Energy Model, which then might be used in i.e. thermal subsystem.
> 
> This patch contains too many different changes.

OK, I will create 4 patches:
1) change with new argument in API function:
    void dev_pm_opp_of_register_em(dev, cpumask)
   and updated drivers
2) changes with _get_cpu_power --> _get_power
3) changes adding int return in dev_pm_opp_of_register_em()
    and updating error handling path inside
4) header changes with new dev_pm_opp_of_unregister_em()

> 
> There are fixes and traces added in addition to a function prototype change. >
> Please provide patches separated by logical changes.

I will try to make this API change in a safe way, which
won't break cpufreq drivers compilation.

> 
> If the cpumask is extracted in the underlying function
> em_register_perf_domain() as suggested in the previous patch 1/5,
> dev_pm_opp_of_register_em() can be struct device centric only.

That would be ideal situation but unfortunately not possible to
implement (as responded in 1/5).

Regards,
Lukasz

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v5 1/5] PM / EM: add devices to Energy Model

[Daniel Lezcano]

Hi Lukasz,


On 06/04/2020 15:29, Lukasz Luba wrote:
> Hi Daniel,
> 
> Thank you for the review.
> 
> On 4/3/20 5:05 PM, Daniel Lezcano wrote:
>>
>> Hi Lukasz,
>>
>>
>> On 18/03/2020 12:45, Lukasz Luba wrote:
>>> Add support of other devices into the Energy Model framework not only
>>> the
>>> CPUs. Change the interface to be more unified which can handle other
>>> devices as well.
>>
>> thanks for taking care of that. Overall I like the changes in this patch
>> but it hard to review in details because the patch is too big :/
>>
>> Could you split this patch into smaller ones?
>>
>> eg. (at your convenience)
>>
>>   - One patch renaming s/cap/perf/
>>
>>   - One patch adding a new function:
>>
>>      em_dev_register_perf_domain(struct device *dev,
>>                 unsigned int nr_states,
>>                 struct em_data_callback *cb);
>>
>>     (+ EXPORT_SYMBOL_GPL)
>>
>>      And em_register_perf_domain() using it.
>>
>>   - One converting the em_register_perf_domain() user to
>>     em_dev_register_perf_domain
>>
>>   - One adding the different new 'em' functions
>>
>>   - And finally one removing em_register_perf_domain().
> 
> I agree and will do the split. I could also break the dependencies
> for future easier merge.
> 
>>
>>
>>> Acked-by: Quentin Perret <qperret@google.com>
>>> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
>>> ---
>>
>> [ ... ]
>>
>>>   2. Core APIs
>>> @@ -70,14 +72,16 @@ CONFIG_ENERGY_MODEL must be enabled to use the EM
>>> framework.
>>>   Drivers are expected to register performance domains into the EM
>>> framework by
>>>   calling the following API::
>>>   -  int em_register_perf_domain(cpumask_t *span, unsigned int
>>> nr_states,
>>> -                  struct em_data_callback *cb);
>>> +  int em_register_perf_domain(struct device *dev, unsigned int
>>> nr_states,
>>> +        struct em_data_callback *cb, cpumask_t *cpus);
>>
>> Isn't possible to get rid of this cpumask by using
>> cpufreq_cpu_get() which returns the cpufreq's policy and from their get
>> the related cpus ?
> 
> We had similar thoughts with Quentin and I've checked this.

Yeah, I suspected you already think about that :)

> Unfortunately, if the policy is a 'new policy' [1] it gets
> allocated and passed into cpufreq driver ->init(policy) [2].
> Then that policy is set into per_cpu pointer for each related_cpu [3]:
> 
> for_each_cpu(j, policy->related_cpus)
>     per_cpu(cpufreq_cpu_data, j) = policy;
> 
>  
> Thus, any calls of functions (i.e. cpufreq_cpu_get()) which try to
> take this ptr before [3] won't work.
> 
> We are trying to register EM from cpufreq_driver->init(policy) and the
> per_cpu policy is likely to be not populated at that phase.

What is the problem of registering at the end of the cpufreq_online ?


> [1]
> https://elixir.bootlin.com/linux/latest/source/drivers/cpufreq/cpufreq.c#L1328
> 
> [2]
> https://elixir.bootlin.com/linux/latest/source/drivers/cpufreq/cpufreq.c#L1350
> 
> [3]
> https://elixir.bootlin.com/linux/latest/source/drivers/cpufreq/cpufreq.c#L1374
> 
> 
> 


-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v5 1/5] PM / EM: add devices to Energy Model

[Lukasz Luba]

On 4/6/20 3:58 PM, Daniel Lezcano wrote:
> 
> Hi Lukasz,
> 
> 
> On 06/04/2020 15:29, Lukasz Luba wrote:
>> Hi Daniel,
>>
>> Thank you for the review.
>>
>> On 4/3/20 5:05 PM, Daniel Lezcano wrote:
>>>
>>> Hi Lukasz,
>>>
>>>
>>> On 18/03/2020 12:45, Lukasz Luba wrote:
>>>> Add support of other devices into the Energy Model framework not only
>>>> the
>>>> CPUs. Change the interface to be more unified which can handle other
>>>> devices as well.
>>>
>>> thanks for taking care of that. Overall I like the changes in this patch
>>> but it hard to review in details because the patch is too big :/
>>>
>>> Could you split this patch into smaller ones?
>>>
>>> eg. (at your convenience)
>>>
>>>    - One patch renaming s/cap/perf/
>>>
>>>    - One patch adding a new function:
>>>
>>>       em_dev_register_perf_domain(struct device *dev,
>>>                  unsigned int nr_states,
>>>                  struct em_data_callback *cb);
>>>
>>>      (+ EXPORT_SYMBOL_GPL)
>>>
>>>       And em_register_perf_domain() using it.
>>>
>>>    - One converting the em_register_perf_domain() user to
>>>      em_dev_register_perf_domain
>>>
>>>    - One adding the different new 'em' functions
>>>
>>>    - And finally one removing em_register_perf_domain().
>>
>> I agree and will do the split. I could also break the dependencies
>> for future easier merge.
>>
>>>
>>>
>>>> Acked-by: Quentin Perret <qperret@google.com>
>>>> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
>>>> ---
>>>
>>> [ ... ]
>>>
>>>>    2. Core APIs
>>>> @@ -70,14 +72,16 @@ CONFIG_ENERGY_MODEL must be enabled to use the EM
>>>> framework.
>>>>    Drivers are expected to register performance domains into the EM
>>>> framework by
>>>>    calling the following API::
>>>>    -  int em_register_perf_domain(cpumask_t *span, unsigned int
>>>> nr_states,
>>>> -                  struct em_data_callback *cb);
>>>> +  int em_register_perf_domain(struct device *dev, unsigned int
>>>> nr_states,
>>>> +        struct em_data_callback *cb, cpumask_t *cpus);
>>>
>>> Isn't possible to get rid of this cpumask by using
>>> cpufreq_cpu_get() which returns the cpufreq's policy and from their get
>>> the related cpus ?
>>
>> We had similar thoughts with Quentin and I've checked this.
> 
> Yeah, I suspected you already think about that :)
> 
>> Unfortunately, if the policy is a 'new policy' [1] it gets
>> allocated and passed into cpufreq driver ->init(policy) [2].
>> Then that policy is set into per_cpu pointer for each related_cpu [3]:
>>
>> for_each_cpu(j, policy->related_cpus)
>>      per_cpu(cpufreq_cpu_data, j) = policy;
>>
>>   
>> Thus, any calls of functions (i.e. cpufreq_cpu_get()) which try to
>> take this ptr before [3] won't work.
>>
>> We are trying to register EM from cpufreq_driver->init(policy) and the
>> per_cpu policy is likely to be not populated at that phase.
> 
> What is the problem of registering at the end of the cpufreq_online ?

We want to enable driver developers to choose one of two options for the
registration of Energy Model:
1. a simple one via dev_pm_opp_of_register_em(), which uses default
    callback function calculating power based on: voltage, freq
    and DT entry 'dynamic-power-coefficient' for each OPP
2. a more sophisticated, when driver provides callback function, which
   will be called from EM for each OPP to ask for related power;
   This interface could also be used by devices which relay not only
   on one source of 'voltage', i.e. manipulate body bias or have
   other controlling voltage for gates in the new 3D transistors. They
   might provide custom callback function in their cpufreq driver.
   This is used i.e. in cpufreq drivers which use firmware to get power,
   like scmi-cpufreq.c;

To meet this requirement the registration of EM is moved into cpufreq
drivers, not in the framework i.e cpufreq_online(). If we could limit
the support for only option 1. then we could move the registration
call into cpufreq framework and clean the cpufreq drivers.

Regards,
Lukasz

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v5 1/5] PM / EM: add devices to Energy Model

[Daniel Lezcano]

On 06/04/2020 18:07, Lukasz Luba wrote:
> 
> 
> On 4/6/20 3:58 PM, Daniel Lezcano wrote:
>>
>> Hi Lukasz,
>>
>>
>> On 06/04/2020 15:29, Lukasz Luba wrote:
>>> Hi Daniel,
>>>
>>> Thank you for the review.
>>>
>>> On 4/3/20 5:05 PM, Daniel Lezcano wrote:
>>>>
>>>> Hi Lukasz,
>>>>
>>>>
>>>> On 18/03/2020 12:45, Lukasz Luba wrote:
>>>>> Add support of other devices into the Energy Model framework not only
>>>>> the
>>>>> CPUs. Change the interface to be more unified which can handle other
>>>>> devices as well.
>>>>
>>>> thanks for taking care of that. Overall I like the changes in this
>>>> patch
>>>> but it hard to review in details because the patch is too big :/
>>>>
>>>> Could you split this patch into smaller ones?
>>>>
>>>> eg. (at your convenience)
>>>>
>>>>    - One patch renaming s/cap/perf/
>>>>
>>>>    - One patch adding a new function:
>>>>
>>>>       em_dev_register_perf_domain(struct device *dev,
>>>>                  unsigned int nr_states,
>>>>                  struct em_data_callback *cb);
>>>>
>>>>      (+ EXPORT_SYMBOL_GPL)
>>>>
>>>>       And em_register_perf_domain() using it.
>>>>
>>>>    - One converting the em_register_perf_domain() user to
>>>>      em_dev_register_perf_domain
>>>>
>>>>    - One adding the different new 'em' functions
>>>>
>>>>    - And finally one removing em_register_perf_domain().
>>>
>>> I agree and will do the split. I could also break the dependencies
>>> for future easier merge.
>>>
>>>>
>>>>
>>>>> Acked-by: Quentin Perret <qperret@google.com>
>>>>> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
>>>>> ---
>>>>
>>>> [ ... ]
>>>>
>>>>>    2. Core APIs
>>>>> @@ -70,14 +72,16 @@ CONFIG_ENERGY_MODEL must be enabled to use the EM
>>>>> framework.
>>>>>    Drivers are expected to register performance domains into the EM
>>>>> framework by
>>>>>    calling the following API::
>>>>>    -  int em_register_perf_domain(cpumask_t *span, unsigned int
>>>>> nr_states,
>>>>> -                  struct em_data_callback *cb);
>>>>> +  int em_register_perf_domain(struct device *dev, unsigned int
>>>>> nr_states,
>>>>> +        struct em_data_callback *cb, cpumask_t *cpus);
>>>>
>>>> Isn't possible to get rid of this cpumask by using
>>>> cpufreq_cpu_get() which returns the cpufreq's policy and from their get
>>>> the related cpus ?
>>>
>>> We had similar thoughts with Quentin and I've checked this.
>>
>> Yeah, I suspected you already think about that :)
>>
>>> Unfortunately, if the policy is a 'new policy' [1] it gets
>>> allocated and passed into cpufreq driver ->init(policy) [2].
>>> Then that policy is set into per_cpu pointer for each related_cpu [3]:
>>>
>>> for_each_cpu(j, policy->related_cpus)
>>>      per_cpu(cpufreq_cpu_data, j) = policy;
>>>
>>>   Thus, any calls of functions (i.e. cpufreq_cpu_get()) which try to
>>> take this ptr before [3] won't work.
>>>
>>> We are trying to register EM from cpufreq_driver->init(policy) and the
>>> per_cpu policy is likely to be not populated at that phase.
>>
>> What is the problem of registering at the end of the cpufreq_online ?
> 
> We want to enable driver developers to choose one of two options for the
> registration of Energy Model:
> 1. a simple one via dev_pm_opp_of_register_em(), which uses default
>    callback function calculating power based on: voltage, freq
>    and DT entry 'dynamic-power-coefficient' for each OPP
> 2. a more sophisticated, when driver provides callback function, which
>   will be called from EM for each OPP to ask for related power;
>   This interface could also be used by devices which relay not only
>   on one source of 'voltage', i.e. manipulate body bias or have
>   other controlling voltage for gates in the new 3D transistors. They
>   might provide custom callback function in their cpufreq driver.
>   This is used i.e. in cpufreq drivers which use firmware to get power,
>   like scmi-cpufreq.c;
> 
> To meet this requirement the registration of EM is moved into cpufreq
> drivers, not in the framework i.e cpufreq_online(). If we could limit
> the support for only option 1. then we could move the registration
> call into cpufreq framework and clean the cpufreq drivers.

I'm not sure to get your point but I think a series setting the scene by
moving the dev_pm_opp_of_register_em() to cpufreq_online() and remove
the cpumask may make sense.

Can you send the split version of patch 1/5 as a series without the
other changes ? So we can focus on first ?


-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v5 1/5] PM / EM: add devices to Energy Model

[Lukasz Luba]

On 4/6/20 10:17 PM, Daniel Lezcano wrote:
> On 06/04/2020 18:07, Lukasz Luba wrote:
>>
>>
>> On 4/6/20 3:58 PM, Daniel Lezcano wrote:
>>>
>>> Hi Lukasz,
>>>
>>>
>>> On 06/04/2020 15:29, Lukasz Luba wrote:
>>>> Hi Daniel,
>>>>
>>>> Thank you for the review.
>>>>
>>>> On 4/3/20 5:05 PM, Daniel Lezcano wrote:
>>>>>
>>>>> Hi Lukasz,
>>>>>
>>>>>
>>>>> On 18/03/2020 12:45, Lukasz Luba wrote:
>>>>>> Add support of other devices into the Energy Model framework not only
>>>>>> the
>>>>>> CPUs. Change the interface to be more unified which can handle other
>>>>>> devices as well.
>>>>>
>>>>> thanks for taking care of that. Overall I like the changes in this
>>>>> patch
>>>>> but it hard to review in details because the patch is too big :/
>>>>>
>>>>> Could you split this patch into smaller ones?
>>>>>
>>>>> eg. (at your convenience)
>>>>>
>>>>>     - One patch renaming s/cap/perf/
>>>>>
>>>>>     - One patch adding a new function:
>>>>>
>>>>>        em_dev_register_perf_domain(struct device *dev,
>>>>>                   unsigned int nr_states,
>>>>>                   struct em_data_callback *cb);
>>>>>
>>>>>       (+ EXPORT_SYMBOL_GPL)
>>>>>
>>>>>        And em_register_perf_domain() using it.
>>>>>
>>>>>     - One converting the em_register_perf_domain() user to
>>>>>       em_dev_register_perf_domain
>>>>>
>>>>>     - One adding the different new 'em' functions
>>>>>
>>>>>     - And finally one removing em_register_perf_domain().
>>>>
>>>> I agree and will do the split. I could also break the dependencies
>>>> for future easier merge.
>>>>
>>>>>
>>>>>
>>>>>> Acked-by: Quentin Perret <qperret@google.com>
>>>>>> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
>>>>>> ---
>>>>>
>>>>> [ ... ]
>>>>>
>>>>>>     2. Core APIs
>>>>>> @@ -70,14 +72,16 @@ CONFIG_ENERGY_MODEL must be enabled to use the EM
>>>>>> framework.
>>>>>>     Drivers are expected to register performance domains into the EM
>>>>>> framework by
>>>>>>     calling the following API::
>>>>>>     -  int em_register_perf_domain(cpumask_t *span, unsigned int
>>>>>> nr_states,
>>>>>> -                  struct em_data_callback *cb);
>>>>>> +  int em_register_perf_domain(struct device *dev, unsigned int
>>>>>> nr_states,
>>>>>> +        struct em_data_callback *cb, cpumask_t *cpus);
>>>>>
>>>>> Isn't possible to get rid of this cpumask by using
>>>>> cpufreq_cpu_get() which returns the cpufreq's policy and from their get
>>>>> the related cpus ?
>>>>
>>>> We had similar thoughts with Quentin and I've checked this.
>>>
>>> Yeah, I suspected you already think about that :)
>>>
>>>> Unfortunately, if the policy is a 'new policy' [1] it gets
>>>> allocated and passed into cpufreq driver ->init(policy) [2].
>>>> Then that policy is set into per_cpu pointer for each related_cpu [3]:
>>>>
>>>> for_each_cpu(j, policy->related_cpus)
>>>>       per_cpu(cpufreq_cpu_data, j) = policy;
>>>>
>>>>    Thus, any calls of functions (i.e. cpufreq_cpu_get()) which try to
>>>> take this ptr before [3] won't work.
>>>>
>>>> We are trying to register EM from cpufreq_driver->init(policy) and the
>>>> per_cpu policy is likely to be not populated at that phase.
>>>
>>> What is the problem of registering at the end of the cpufreq_online ?
>>
>> We want to enable driver developers to choose one of two options for the
>> registration of Energy Model:
>> 1. a simple one via dev_pm_opp_of_register_em(), which uses default
>>     callback function calculating power based on: voltage, freq
>>     and DT entry 'dynamic-power-coefficient' for each OPP
>> 2. a more sophisticated, when driver provides callback function, which
>>    will be called from EM for each OPP to ask for related power;
>>    This interface could also be used by devices which relay not only
>>    on one source of 'voltage', i.e. manipulate body bias or have
>>    other controlling voltage for gates in the new 3D transistors. They
>>    might provide custom callback function in their cpufreq driver.
>>    This is used i.e. in cpufreq drivers which use firmware to get power,
>>    like scmi-cpufreq.c;
>>
>> To meet this requirement the registration of EM is moved into cpufreq
>> drivers, not in the framework i.e cpufreq_online(). If we could limit
>> the support for only option 1. then we could move the registration
>> call into cpufreq framework and clean the cpufreq drivers.
> 
> I'm not sure to get your point but I think a series setting the scene by
> moving the dev_pm_opp_of_register_em() to cpufreq_online() and remove
> the cpumask may make sense.

Some of the cpufreq drivers don't use dev_pm_opp_of_register_em() but 
instead em_register_perf_domain() with their em_data_callback [1].
It is because of point 2. described above. The dev_pm_opp_of_register_em
won't work for them, so it's not a good candidate to cover all use cases
in the framework.

> 
> Can you send the split version of patch 1/5 as a series without the
> other changes ? So we can focus on first ?

Sure, I will only split patch 1/5 as you suggested and send v6.
Thank you for your time and help.

Regards,
Lukasz

[1] 
https://elixir.bootlin.com/linux/latest/source/drivers/cpufreq/scmi-cpufreq.c#L203

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel