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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20220425072710.v6gwo4gu3aouezg4@vireshk-i7>
Date:   Mon, 25 Apr 2022 12:57:10 +0530
From:   Viresh Kumar <viresh.kumar@...aro.org>
To:     Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
Cc:     Andy Gross <agross@...nel.org>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        Michael Turquette <mturquette@...libre.com>,
        Stephen Boyd <sboyd@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Viresh Kumar <vireshk@...nel.org>, Nishanth Menon <nm@...com>,
        Alim Akhtar <alim.akhtar@...sung.com>,
        Avri Altman <avri.altman@....com>,
        "James E.J. Bottomley" <jejb@...ux.ibm.com>,
        "Martin K. Petersen" <martin.petersen@...cle.com>,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        Taniya Das <tdas@...eaurora.org>,
        linux-arm-msm@...r.kernel.org, linux-clk@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-pm@...r.kernel.org, linux-scsi@...r.kernel.org
Subject: Re: [RFC PATCH v2 4/6] PM: opp: allow control of multiple clocks

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