Re: [RFC PATCH v2 4/6] PM: opp: allow control of multiple clocks

[Viresh Kumar]

On 11-04-22, 17:43, Krzysztof Kozlowski wrote:
> Devices might need to control several clocks when scaling the frequency
> and voltage.  Example is the Universal Flash Storage (UFS) which scales
> several independent clocks with change of performance levels.
> 
> Add parsing of multiple clocks and clock names

This part is fine, the OPP core should be able to do this.

> and scale all of them,

This is tricky as the OPP core can't really assume the order in which the clocks
needs to be programmed. We had the same problem with multiple regulators and the
same is left for drivers to do via the custom-api.

Either we can take the same route here, and let platforms add their own OPP
drivers which can handle this, Or hide this all behind a basic device clock's
driver, which you get with clk_get(dev, NULL).

> diff --git a/drivers/opp/core.c b/drivers/opp/core.c

> +static int _generic_set_opp_clks_only(struct device *dev,
> +				      struct opp_table *opp_table,
> +				      struct dev_pm_opp *opp)
> +{
> +	int i, ret;
> +
> +	if (!opp_table->clks)
> +		return 0;
> +
> +	for (i = 0; i < opp_table->clk_count; i++) {
> +		if (opp->rates[i]) {

This should mean that we can disable that clock and it isn't required.

> +			ret = _generic_set_opp_clk_only(dev, opp_table->clks[i],
> +							opp->rates[i]);
> +			if (ret) {
> +				dev_err(dev, "%s: failed to set clock %pC rate: %d\n",
> +					__func__, opp_table->clks[i], ret);
> +				return ret;
> +			}
> +		}
> +	}
> +
> +	return 0;
> +}

As said earlier, this won't work in the core.

> +
>  static int _generic_set_opp_regulator(struct opp_table *opp_table,
>  				      struct device *dev,
>  				      struct dev_pm_opp *opp,
> @@ -796,7 +835,7 @@ static int _generic_set_opp_regulator(struct opp_table *opp_table,
>  	}
>  
>  	/* Change frequency */
> -	ret = _generic_set_opp_clk_only(dev, opp_table->clk, freq);
> +	ret = _generic_set_opp_clks_only(dev, opp_table, opp);
>  	if (ret)
>  		goto restore_voltage;
>  
> @@ -820,7 +859,7 @@ static int _generic_set_opp_regulator(struct opp_table *opp_table,
>  	return 0;
>  
>  restore_freq:
> -	if (_generic_set_opp_clk_only(dev, opp_table->clk, old_opp->rate))
> +	if (_generic_set_opp_clks_only(dev, opp_table, old_opp))
>  		dev_err(dev, "%s: failed to restore old-freq (%lu Hz)\n",
>  			__func__, old_opp->rate);
>  restore_voltage:
> @@ -880,7 +919,7 @@ static int _set_opp_custom(const struct opp_table *opp_table,

This is where we can handle it in your case, if you don't want to hide it behind
a clk driver.

>  	}
>  
>  	data->regulators = opp_table->regulators;
> -	data->clk = opp_table->clk;
> +	data->clk = (opp_table->clks ? opp_table->clks[0] : NULL);
>  	data->dev = dev;
>  	data->old_opp.rate = old_opp->rate;
>  	data->new_opp.rate = freq;
> @@ -969,8 +1008,8 @@ static void _find_current_opp(struct device *dev, struct opp_table *opp_table)

I think this routine breaks as soon as we add support for multiple clocks.
clks[0]'s frequency can be same for multiple OPPs and this won't get you the
right OPP then.

>  	struct dev_pm_opp *opp = ERR_PTR(-ENODEV);
>  	unsigned long freq;
>  
> -	if (!IS_ERR(opp_table->clk)) {
> -		freq = clk_get_rate(opp_table->clk);
> +	if (opp_table->clks && !IS_ERR(opp_table->clks[0])) {
> +		freq = clk_get_rate(opp_table->clks[0]);
>  		opp = _find_freq_ceil(opp_table, &freq);
>  	}
>  
> @@ -1070,7 +1109,7 @@ static int _set_opp(struct device *dev, struct opp_table *opp_table,
>  						 scaling_down);
>  	} else {
>  		/* Only frequency scaling */
> -		ret = _generic_set_opp_clk_only(dev, opp_table->clk, freq);
> +		ret = _generic_set_opp_clks_only(dev, opp_table, opp);
>  	}
>  
>  	if (ret)
> @@ -1135,11 +1174,15 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)

This should have a BUG or WARN _ON() now if clock count is more than one. This
routine can't be called unless custom handler is available.

I skipped rest of the code as we need to work/decide on the design first.

Thanks.

-- 
viresh