Re: [PATCH 5/5] OPP: Remove custom OPP helper support

Re: [PATCH 5/5] OPP: Remove custom OPP helper support

[Dmitry Osipenko]

31.05.2022 13:10, Viresh Kumar пишет:
> The only user of the custom helper is migrated to use
> config_regulators() interface. Remove the now unused custom OPP helper
> support.
> 
> This cleans up _set_opp() and leaves a single code path to be used by
> all users.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/opp/core.c     | 126 +----------------------------------------
>  drivers/opp/opp.h      |   7 ---
>  include/linux/pm_opp.h |  35 ------------
>  3 files changed, 2 insertions(+), 166 deletions(-)
> 
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index 58ce1240e808..554a043bc225 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -972,36 +972,6 @@ static int _set_opp_bw(const struct opp_table *opp_table,
>  	return 0;
>  }
>  
> -static int _set_opp_custom(const struct opp_table *opp_table,
> -			   struct device *dev, struct dev_pm_opp *opp,
> -			   unsigned long freq)
> -{
> -	struct dev_pm_set_opp_data *data = opp_table->set_opp_data;
> -	struct dev_pm_opp *old_opp = opp_table->current_opp;
> -	int size;
> -
> -	/*
> -	 * We support this only if dev_pm_opp_set_config() was called
> -	 * earlier to set regulators.
> -	 */
> -	if (opp_table->sod_supplies) {
> -		size = sizeof(*old_opp->supplies) * opp_table->regulator_count;
> -		memcpy(data->old_opp.supplies, old_opp->supplies, size);
> -		memcpy(data->new_opp.supplies, opp->supplies, size);
> -		data->regulator_count = opp_table->regulator_count;
> -	} else {
> -		data->regulator_count = 0;
> -	}
> -
> -	data->regulators = opp_table->regulators;
> -	data->clk = opp_table->clk;
> -	data->dev = dev;
> -	data->old_opp.rate = old_opp->rate;
> -	data->new_opp.rate = freq;
> -
> -	return opp_table->set_opp(data);
> -}
> -
>  static int _set_required_opp(struct device *dev, struct device *pd_dev,
>  			     struct dev_pm_opp *opp, int i)
>  {
> @@ -1186,13 +1156,7 @@ static int _set_opp(struct device *dev, struct opp_table *opp_table,
>  		}
>  	}
>  
> -	if (opp_table->set_opp) {
> -		ret = _set_opp_custom(opp_table, dev, opp, freq);
> -	} else {
> -		/* Only frequency scaling */
> -		ret = _generic_set_opp_clk_only(dev, opp_table->clk, freq);
> -	}
> -
> +	ret = _generic_set_opp_clk_only(dev, opp_table->clk, freq);
>  	if (ret)
>  		return ret;
>  
> @@ -2054,7 +2018,6 @@ static void _opp_put_prop_name(struct opp_table *opp_table)
>  static int _opp_set_regulators(struct opp_table *opp_table, struct device *dev,
>  			       const char * const names[], unsigned int count)
>  {
> -	struct dev_pm_opp_supply *supplies;
>  	struct regulator *reg;
>  	int ret, i;
>  
> @@ -2082,20 +2045,8 @@ static int _opp_set_regulators(struct opp_table *opp_table, struct device *dev,
>  
>  	opp_table->regulator_count = count;
>  
> -	supplies = kmalloc_array(count * 2, sizeof(*supplies), GFP_KERNEL);
> -	if (!supplies) {
> -		ret = -ENOMEM;
> -		goto free_regulators;
> -	}
> -
> -	mutex_lock(&opp_table->lock);
> -	opp_table->sod_supplies = supplies;
> -	if (opp_table->set_opp_data) {
> -		opp_table->set_opp_data->old_opp.supplies = supplies;
> -		opp_table->set_opp_data->new_opp.supplies = supplies + count;
> -	}
> -
>  	/* Set generic config_regulators() for single regulators here */
> +	mutex_lock(&opp_table->lock);
>  	if (count == 1)
>  		opp_table->config_regulators = _opp_config_regulator_single;
>  	mutex_unlock(&opp_table->lock);
> @@ -2132,16 +2083,6 @@ static void _opp_put_regulators(struct opp_table *opp_table)
>  	for (i = opp_table->regulator_count - 1; i >= 0; i--)
>  		regulator_put(opp_table->regulators[i]);
>  
> -	mutex_lock(&opp_table->lock);
> -	if (opp_table->set_opp_data) {
> -		opp_table->set_opp_data->old_opp.supplies = NULL;
> -		opp_table->set_opp_data->new_opp.supplies = NULL;
> -	}
> -
> -	kfree(opp_table->sod_supplies);
> -	opp_table->sod_supplies = NULL;
> -	mutex_unlock(&opp_table->lock);
> -
>  	kfree(opp_table->regulators);
>  	opp_table->regulators = NULL;
>  	opp_table->regulator_count = -1;
> @@ -2195,61 +2136,6 @@ static void _opp_put_clkname(struct opp_table *opp_table)
>  	}
>  }
>  
> -/**
> - * _opp_register_set_opp_helper() - Register custom set OPP helper
> - * @dev: Device for which the helper is getting registered.
> - * @set_opp: Custom set OPP helper.
> - *
> - * This is useful to support complex platforms (like platforms with multiple
> - * regulators per device), instead of the generic OPP set rate helper.
> - *
> - * This must be called before any OPPs are initialized for the device.
> - */
> -static int _opp_register_set_opp_helper(struct opp_table *opp_table,
> -	struct device *dev, int (*set_opp)(struct dev_pm_set_opp_data *data))
> -{
> -	struct dev_pm_set_opp_data *data;
> -
> -	/* Another CPU that shares the OPP table has set the helper ? */
> -	if (opp_table->set_opp)
> -		return 0;
> -
> -	data = kzalloc(sizeof(*data), GFP_KERNEL);
> -	if (!data)
> -		return -ENOMEM;
> -
> -	mutex_lock(&opp_table->lock);
> -	opp_table->set_opp_data = data;
> -	if (opp_table->sod_supplies) {
> -		data->old_opp.supplies = opp_table->sod_supplies;
> -		data->new_opp.supplies = opp_table->sod_supplies +
> -					 opp_table->regulator_count;
> -	}
> -	mutex_unlock(&opp_table->lock);
> -
> -	opp_table->set_opp = set_opp;
> -
> -	return 0;
> -}
> -
> -/**
> - * _opp_unregister_set_opp_helper() - Releases resources blocked for set_opp helper
> - * @opp_table: OPP table returned from _opp_register_set_opp_helper().
> - *
> - * Release resources blocked for platform specific set_opp helper.
> - */
> -static void _opp_unregister_set_opp_helper(struct opp_table *opp_table)
> -{
> -	if (opp_table->set_opp) {
> -		opp_table->set_opp = NULL;
> -
> -		mutex_lock(&opp_table->lock);
> -		kfree(opp_table->set_opp_data);
> -		opp_table->set_opp_data = NULL;
> -		mutex_unlock(&opp_table->lock);
> -	}
> -}
> -
>  /**
>   * _opp_set_config_regulators_helper() - Register custom set regulator helper.
>   * @dev: Device for which the helper is getting registered.
> @@ -2447,13 +2333,6 @@ struct opp_table *dev_pm_opp_set_config(struct device *dev,
>  			goto err;
>  	}
>  
> -	/* Configure opp helper */
> -	if (config->set_opp) {
> -		ret = _opp_register_set_opp_helper(opp_table, dev, config->set_opp);
> -		if (ret)
> -			goto err;
> -	}
> -
>  	/* Configure config_regulators helper */
>  	if (config->config_regulators) {
>  		ret = _opp_set_config_regulators_helper(opp_table, dev, config->config_regulators);
> @@ -2516,7 +2395,6 @@ void dev_pm_opp_clear_config(struct opp_table *opp_table)
>  	_opp_put_regulators(opp_table);
>  	_opp_put_supported_hw(opp_table);
>  	_opp_put_config_regulators_helper(opp_table);
> -	_opp_unregister_set_opp_helper(opp_table);
>  	_opp_put_prop_name(opp_table);
>  	_opp_put_clkname(opp_table);
>  
> diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
> index 4695d315e7f9..407eee9f10ab 100644
> --- a/drivers/opp/opp.h
> +++ b/drivers/opp/opp.h
> @@ -161,9 +161,6 @@ enum opp_table_access {
>   * @enabled: Set to true if the device's resources are enabled/configured.
>   * @genpd_performance_state: Device's power domain support performance state.
>   * @is_genpd: Marks if the OPP table belongs to a genpd.
> - * @set_opp: Platform specific set_opp callback
> - * @sod_supplies: Set opp data supplies
> - * @set_opp_data: Data to be passed to set_opp callback
>   * @dentry:	debugfs dentry pointer of the real device directory (not links).
>   * @dentry_name: Name of the real dentry.
>   *
> @@ -213,10 +210,6 @@ struct opp_table {
>  	bool genpd_performance_state;
>  	bool is_genpd;
>  
> -	int (*set_opp)(struct dev_pm_set_opp_data *data);
> -	struct dev_pm_opp_supply *sod_supplies;
> -	struct dev_pm_set_opp_data *set_opp_data;
> -
>  #ifdef CONFIG_DEBUG_FS
>  	struct dentry *dentry;
>  	char dentry_name[NAME_MAX];
> diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
> index 185503aab64d..c6d8f01ef9fd 100644
> --- a/include/linux/pm_opp.h
> +++ b/include/linux/pm_opp.h
> @@ -57,39 +57,6 @@ struct dev_pm_opp_icc_bw {
>  	u32 peak;
>  };
>  
> -/**
> - * struct dev_pm_opp_info - OPP freq/voltage/current values
> - * @rate:	Target clk rate in hz
> - * @supplies:	Array of voltage/current values for all power supplies
> - *
> - * This structure stores the freq/voltage/current values for a single OPP.
> - */
> -struct dev_pm_opp_info {
> -	unsigned long rate;
> -	struct dev_pm_opp_supply *supplies;
> -};
> -
> -/**
> - * struct dev_pm_set_opp_data - Set OPP data
> - * @old_opp:	Old OPP info
> - * @new_opp:	New OPP info
> - * @regulators:	Array of regulator pointers
> - * @regulator_count: Number of regulators
> - * @clk:	Pointer to clk
> - * @dev:	Pointer to the struct device
> - *
> - * This structure contains all information required for setting an OPP.
> - */
> -struct dev_pm_set_opp_data {
> -	struct dev_pm_opp_info old_opp;
> -	struct dev_pm_opp_info new_opp;
> -
> -	struct regulator **regulators;
> -	unsigned int regulator_count;
> -	struct clk *clk;
> -	struct device *dev;
> -};
> -
>  typedef int (*config_regulators_t)(struct device *dev, struct dev_pm_opp *old_opp,
>  				   struct dev_pm_opp *new_opp, struct regulator **regulators,
>  				   unsigned int count);
> @@ -97,7 +64,6 @@ typedef int (*config_regulators_t)(struct device *dev, struct dev_pm_opp *old_op
>   * struct dev_pm_opp_config - Device OPP configuration values
>   * @clk_name: Clk name.
>   * @prop_name: Name to postfix to properties.
> - * @set_opp: Custom set OPP helper.
>   * @config_regulators: Custom set regulator helper.
>   * @supported_hw: Array of hierarchy of versions to match.
>   * @supported_hw_count: Number of elements in the array.
> @@ -111,7 +77,6 @@ typedef int (*config_regulators_t)(struct device *dev, struct dev_pm_opp *old_op
>  struct dev_pm_opp_config {
>  	const char *clk_name;
>  	const char *prop_name;
> -	int (*set_opp)(struct dev_pm_set_opp_data *data);
>  	config_regulators_t config_regulators;
>  	unsigned int *supported_hw;
>  	unsigned int supported_hw_count;

Hello Viresh,

Unfortunately we can't remove the set_opp_helper(). It's terrible that
this function is unused by Tegra 3d driver because it should be used.

The patch that supposed to use the devm_pm_opp_register_set_opp_helper()
[1] was merged a half year ago and just today I noticed that the merged
code doesn't have devm_pm_opp_register_set_opp_helper() [2]. I think
Thierry edited my patch before applying it, perhaps there was a merge
conflict :/ This needs to be fixed now.

[1] https://lore.kernel.org/all/20210817012754.8710-16-digetx@gmail.com/

[2]
https://elixir.bootlin.com/linux/v5.19-rc3/source/drivers/gpu/drm/tegra/gr3d.c#L535

Re: [PATCH 5/5] OPP: Remove custom OPP helper support

[Viresh Kumar]

Hi Dmitry,

On 25-06-22, 14:42, Dmitry Osipenko wrote:
> 31.05.2022 13:10, Viresh Kumar пишет:
> > The only user of the custom helper is migrated to use
> > config_regulators() interface. Remove the now unused custom OPP helper
> > support.
> > 
> > This cleans up _set_opp() and leaves a single code path to be used by
> > all users.

> Unfortunately we can't remove the set_opp_helper(). It's terrible that
> this function is unused by Tegra 3d driver because it should be used.
> 
> The patch that supposed to use the devm_pm_opp_register_set_opp_helper()
> [1] was merged a half year ago and just today I noticed that the merged
> code doesn't have devm_pm_opp_register_set_opp_helper() [2]. I think
> Thierry edited my patch before applying it, perhaps there was a merge
> conflict :/ This needs to be fixed now.

As the commit log above says, we aren't removing the feature, but just
changing the interface to cnofig_regulators(). That was the only
special handling the drivers were required to do earlier as well, for
which the helper interface was added.

-- 
viresh

Re: [PATCH 5/5] OPP: Remove custom OPP helper support

[Dmitry Osipenko]

27.06.2022 09:06, Viresh Kumar пишет:
> Hi Dmitry,
> 
> On 25-06-22, 14:42, Dmitry Osipenko wrote:
>> 31.05.2022 13:10, Viresh Kumar пишет:
>>> The only user of the custom helper is migrated to use
>>> config_regulators() interface. Remove the now unused custom OPP helper
>>> support.
>>>
>>> This cleans up _set_opp() and leaves a single code path to be used by
>>> all users.
> 
>> Unfortunately we can't remove the set_opp_helper(). It's terrible that
>> this function is unused by Tegra 3d driver because it should be used.
>>
>> The patch that supposed to use the devm_pm_opp_register_set_opp_helper()
>> [1] was merged a half year ago and just today I noticed that the merged
>> code doesn't have devm_pm_opp_register_set_opp_helper() [2]. I think
>> Thierry edited my patch before applying it, perhaps there was a merge
>> conflict :/ This needs to be fixed now.
> 
> As the commit log above says, we aren't removing the feature, but just
> changing the interface to cnofig_regulators(). That was the only
> special handling the drivers were required to do earlier as well, for
> which the helper interface was added.
> 

Okay, but Tegra 3d driver doesn't need config_regulators(), it needs
customized set_opp() to set clock rate for both 3d engines.

Re: [PATCH 5/5] OPP: Remove custom OPP helper support

[Viresh Kumar]

On 27-06-22, 09:10, Dmitry Osipenko wrote:
> Okay, but Tegra 3d driver doesn't need config_regulators(), it needs
> customized set_opp() to set clock rate for both 3d engines.

What does that mean, you need to set two clocks ? There is separate
support added for that already [1].

-- 
viresh

[1] https://lore.kernel.org/lkml/cover.1654849214.git.viresh.kumar@linaro.org/

Re: [PATCH 5/5] OPP: Remove custom OPP helper support

[Dmitry Osipenko]

27.06.2022 09:41, Viresh Kumar пишет:
> On 27-06-22, 09:10, Dmitry Osipenko wrote:
>> Okay, but Tegra 3d driver doesn't need config_regulators(), it needs
>> customized set_opp() to set clock rate for both 3d engines.
> 
> What does that mean, you need to set two clocks ? There is separate
> support added for that already [1].
> 

Yes, I missed that multi-clock OPP patch, thanks.

Seems _opp_compare_key() won't work properly for the multi-clocks since
Tegra doesn't have bandwidth nor level for the 3d OPPs. Why does it need
to check opp_table->clk_count == 1? Shouldn't it be opp_table->clk_count
> 0?

Re: [PATCH 5/5] OPP: Remove custom OPP helper support

[Viresh Kumar]

On 27-06-22, 10:09, Dmitry Osipenko wrote:
> Yes, I missed that multi-clock OPP patch, thanks.
> 
> Seems _opp_compare_key() won't work properly for the multi-clocks since
> Tegra doesn't have bandwidth nor level for the 3d OPPs. Why does it need
> to check opp_table->clk_count == 1? Shouldn't it be opp_table->clk_count
> > 0?

The problem is that when we have multiple clocks, we can't assume any
of them as primary. Its the combination of the clock frequencies that
make them unique. Otherwise, what will happen if we have same
frequency of the first clock in two OPPs, but different frequency of
the second clock.

Because of this, we won't also support multiple clocks in all freq
finder APIs, like dev_pm_opp_find_freq_exact(). We can't do that from
just one frequency.

Ideally, the drivers should now be calling dev_pm_opp_set_opp() to set
the OPP now.

For your case, I think you can just add levels (like index) in the OPP
table. So we can uniquely identify each OPP.

-- 
viresh

Re: [PATCH 5/5] OPP: Remove custom OPP helper support

[Dmitry Osipenko]

On 6/27/22 10:19, Viresh Kumar wrote:
> On 27-06-22, 10:09, Dmitry Osipenko wrote:
>> Yes, I missed that multi-clock OPP patch, thanks.
>>
>> Seems _opp_compare_key() won't work properly for the multi-clocks since
>> Tegra doesn't have bandwidth nor level for the 3d OPPs. Why does it need
>> to check opp_table->clk_count == 1? Shouldn't it be opp_table->clk_count
>>> 0?
> 
> The problem is that when we have multiple clocks, we can't assume any
> of them as primary. Its the combination of the clock frequencies that
> make them unique. Otherwise, what will happen if we have same
> frequency of the first clock in two OPPs, but different frequency of
> the second clock.
> 
> Because of this, we won't also support multiple clocks in all freq
> finder APIs, like dev_pm_opp_find_freq_exact(). We can't do that from
> just one frequency.
> 
> Ideally, the drivers should now be calling dev_pm_opp_set_opp() to set
> the OPP now.
> 
> For your case, I think you can just add levels (like index) in the OPP
> table. So we can uniquely identify each OPP.
> 

What about to bump the "by-level" sorting priority, making it above the
"by-rate" sorting and then always use the first clock for the "by-rate"
sorting? Then the multi-clock will work for Tegra without breaking dtbs
and those for whom this sorting option won't be appropriate will have to
add levels to the DT.

-- 
Best regards,
Dmitry

Re: [PATCH 5/5] OPP: Remove custom OPP helper support

[Viresh Kumar]

On 28-06-22, 13:04, Dmitry Osipenko wrote:
> What about to bump the "by-level" sorting priority, making it above the
> "by-rate" sorting and then always use the first clock for the "by-rate"
> sorting?

The order doesn't matter much really. If there are multiple clocks,
then we can't compare just one of them. If we don't want the level to
be introduced, which is fine, then we need to compare all the clocks.

> Then the multi-clock will work for Tegra without breaking dtbs
> and those for whom this sorting option won't be appropriate will have to
> add levels to the DT.

There was a recent discussion [1] around this, where using level was
considered sensible for such devices, like Qcom UFS.

-- 
viresh

[1] https://lore.kernel.org/lkml/65a4c28d-6702-3a9f-f837-1ea69a428777@linaro.org/