Re: [PATCH v3 7/7] ufs: use PM OPP when scaling gears

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 13/05/2022 20:25, Manivannan Sadhasivam wrote:
> On Fri, May 13, 2022 at 08:13:47AM +0200, Krzysztof Kozlowski wrote:
>> Scaling gears requires not only scaling clocks, but also voltage levels,
>> e.g. via performance states.
>>
>> Use the provided OPP table, to set proper OPP frequency which through
>> required-opps will trigger performance state change.  This deprecates
>> the old freq-table-hz Devicetree property and old clock scaling method
>> in favor of PM core code.
>>
> 
> To be clear, you are not changing the voltages (UFS supplies) through OPP. But
> rather handle only clks and leave the power domain handling to parent OPP
> device.

Correct, the patchset itself does not introduce itself regulator
control. For Qualcomm (and maybe others) these will be scaled via OPP
performance states.

> 
>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
>>
>> ---
>>
>> Cc: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
>> ---
>>  drivers/scsi/ufs/ufshcd-pltfrm.c |  73 +++++++++++++++
>>  drivers/scsi/ufs/ufshcd.c        | 150 ++++++++++++++++++++++++-------
>>  drivers/scsi/ufs/ufshcd.h        |   6 ++
>>  3 files changed, 195 insertions(+), 34 deletions(-)
>>
>> diff --git a/drivers/scsi/ufs/ufshcd-pltfrm.c b/drivers/scsi/ufs/ufshcd-pltfrm.c
>> index 3ab555f6e66e..a603ca8e383b 100644
>> --- a/drivers/scsi/ufs/ufshcd-pltfrm.c
>> +++ b/drivers/scsi/ufs/ufshcd-pltfrm.c
>> @@ -10,6 +10,7 @@
>>  
>>  #include <linux/module.h>
>>  #include <linux/platform_device.h>
>> +#include <linux/pm_opp.h>
>>  #include <linux/pm_runtime.h>
>>  #include <linux/of.h>
>>  
>> @@ -108,6 +109,72 @@ static int ufshcd_parse_clock_info(struct ufs_hba *hba)
>>  	return ret;
>>  }
>>  
>> +static int ufshcd_parse_operating_points(struct ufs_hba *hba)
>> +{
>> +	struct device *dev = hba->dev;
>> +	struct device_node *np = dev->of_node;
>> +	struct ufs_clk_info *clki;
>> +	const char *names[16];
>> +	int cnt, i, ret;
>> +
>> +	if (!of_find_property(dev->of_node, "operating-points-v2", NULL))
>> +		return 0;
>> +
>> +	cnt = of_property_count_strings(np, "clock-names");
>> +	if (cnt <= 0) {
>> +		dev_warn(dev, "%s: Missing clock-names\n",
>> +			 __func__);
> 
> This is a hard error, right? So why not dev_err()?

Good point, but actually this (and following cases) should be return 0,
because clocks/freq-table/opp-points are not required properties. The
original code (parsing it for freq-table-hz) also does not treat it as
error.

> 
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (cnt > ARRAY_SIZE(names)) {
>> +		dev_info(dev, "%s: Too many clock-names\n",  __func__);
> 
> dev_err()?
> 
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (of_find_property(np, "freq-table-hz", NULL)) {
>> +		dev_info(dev, "%s: operating-points and freq-table-hz are incompatible\n",
>> +			 __func__);
> 
> dev_err()?
> 
>> +		return -EINVAL;
>> +	}
>> +
>> +	for (i = 0; i < cnt; i++) {
>> +		ret = of_property_read_string_index(np, "clock-names", i,
>> +						    &names[i]);
>> +		if (ret)
>> +			return ret;
>> +
>> +		clki = devm_kzalloc(dev, sizeof(*clki), GFP_KERNEL);
>> +		if (!clki)
>> +			return -ENOMEM;
>> +
>> +		clki->name = devm_kstrdup(dev, names[i], GFP_KERNEL);
>> +		if (!clki->name)
>> +			return -ENOMEM;
>> +
>> +		if (!strcmp(names[i], "ref_clk"))
>> +			clki->keep_link_active = true;
>> +
>> +		list_add_tail(&clki->list, &hba->clk_list_head);
>> +	}
>> +
>> +	ret = devm_pm_opp_set_clknames(dev, names, i);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = devm_pm_opp_register_set_opp_helper(dev, ufshcd_set_opp);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = devm_pm_opp_of_add_table(dev);
>> +	if (ret)
>> +		return ret;
>> +
>> +	hba->use_pm_opp = true;
>> +
> 
> Since you are only handling the clks in UFS driver's OPP implementation, it
> warrants atleast a comment. Otherwise, someone will add voltage to the OPP
> table and complain that it is not getting changed. Eventhough the UFS driver
> won't allow doing it, it is safer to mention it explicitly.

Sure.

> 
> Also I'm worried about the implementation specific to Qcom platforms. Like we
> rely on RPMHPD to handle the power domains, but that may not be true for other
> platforms. I know that we cannot support all possible implementations but
> atleast we should document this limitation.
> 
> Rest looks fine to me. I'll take one more look after testing this series on
> SM8450.

Using OPPs is quite generic, so other platform could implement also
regulator scaling. The changes are indeed targetting Qcom platforms, but
they are not restricting any other usage.

Best regards,
Krzysztof