Re: [RFC PATCH v2 5/6] ufs: use PM OPP when scaling gears

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

On 19/04/2022 19:01, Manivannan Sadhasivam wrote:
> On Mon, Apr 11, 2022 at 05:43:46PM +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.
>>
>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>> ---
>>  drivers/scsi/ufs/ufshcd-pltfrm.c |  69 +++++++++++++++++++
>>  drivers/scsi/ufs/ufshcd.c        | 115 +++++++++++++++++++++++--------
>>  drivers/scsi/ufs/ufshcd.h        |   4 ++
>>  3 files changed, 158 insertions(+), 30 deletions(-)
>>
>> diff --git a/drivers/scsi/ufs/ufshcd-pltfrm.c b/drivers/scsi/ufs/ufshcd-pltfrm.c
>> index c1d8b6f46868..edba585db0c1 100644
>> --- a/drivers/scsi/ufs/ufshcd-pltfrm.c
>> +++ b/drivers/scsi/ufs/ufshcd-pltfrm.c
>> @@ -107,6 +107,69 @@ 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];
>> +	bool clocks_done;
> 
> Maybe freq_table?

ok

> 
>> +	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__);
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (cnt > ARRAY_SIZE(names)) {
>> +		dev_info(dev, "%s: Too many clock-names\n",  __func__);
>> +		return -EINVAL;
>> +	}
> 
> How did you come up with 16 as the max clock count? Is this check necessary?

16 was an arbitrary choice, also mentioned in the bindings:
https://lore.kernel.org/all/20220411154347.491396-3-krzysztof.kozlowski@linaro.org/

The check is necessary from current code point of view - array is
locally allocated with fixed size. Since bindings do not allow more than
16, I am not sure if there is a point to make the code flexible now...
but if this is something you wish, I can change.

> 
>> +
>> +	/* clocks parsed by ufshcd_parse_clock_info() */
>> +	clocks_done = !!of_find_property(np, "freq-table-hz", NULL);
> 
> freq-table-hz and opp-table are mutually exclusive, isn't it?

You're right, this should be an exit.

(...)

>>  	ufshcd_init_lanes_per_dir(hba);
>>  
>>  	err = ufshcd_init(hba, mmio_base, irq);
>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>> index 5bfa62fa288a..aec7da18a550 100644
>> --- a/drivers/scsi/ufs/ufshcd.c
>> +++ b/drivers/scsi/ufs/ufshcd.c
>> @@ -1022,6 +1022,9 @@ static int ufshcd_scale_clks(struct ufs_hba *hba, bool scale_up)
>>  	int ret = 0;
>>  	ktime_t start = ktime_get();
>>  
>> +	if (hba->use_pm_opp)
>> +		return 0;
>> +
> 
> So you don't need pre and post clock changes below?

That's tricky. The UFS HCD core does not need it, but of course the
question is about the drivers actually using ufshcd_vops_clk_scale_notify().

Only QCOM UFS driver implements it and actually we might need it. Qcom
driver changes DME_VS_CORE_CLK_CTRL, so maybe this should be done here
as well. I don't know yet how to incorporate it into PM-opp framework,
because now changing frequencies and voltage is atomic from the UFS
driver perspective. Before it was not - for example first clock (with
these pre/post changes) and then voltage.

I will need to solve it somehow...


Best regards,
Krzysztof