Re: [PATCH v1 1/1] soc/tegra: Add devm_tegra_core_dev_init_opp_table()

[Dmitry Osipenko]

17.05.2021 06:37, Viresh Kumar пишет:
> I am not sure why you divided this into three different patchsets,
> while it should really have been one. Like this one just adds the API
> but doesn't use it.

Previously Krzysztof Kozlowski asked to split the large series into smaller sets which could be reviewed and applied separately by maintainers. He suggested that the immutable branch is a better option, I decided to implement this suggestion. So far I only sent out the memory patches which make use of the new helper, there will be more patches. The memory patches are intended to show that this helper can be utilized right now. My plan was to finalize this patch first, then Thierry will apply it and I will be able to sent the rest of the patches telling that they depend on the immutable branch.

I'll merge this helper patch and the memory patches into a single series in v2. 

> On 16-05-21, 23:51, Dmitry Osipenko wrote:
>> Add common helper which initializes OPP table for Tegra SoC core devices.
>>
>> Tested-by: Peter Geis <pgwipeout@xxxxxxxxx> # Ouya T30
>> Tested-by: Paul Fertser <fercerpav@xxxxxxxxx> # PAZ00 T20
>> Tested-by: Nicolas Chauvet <kwizart@xxxxxxxxx> # PAZ00 T20 and TK1 T124
>> Tested-by: Matt Merhar <mattmerhar@xxxxxxxxxxxxxx> # Ouya T30
>> Signed-off-by: Dmitry Osipenko <digetx@xxxxxxxxx>
>> ---
>>  drivers/soc/tegra/common.c | 112 +++++++++++++++++++++++++++++++++++++
>>  include/soc/tegra/common.h |  30 ++++++++++
>>  2 files changed, 142 insertions(+)
>>
>> diff --git a/drivers/soc/tegra/common.c b/drivers/soc/tegra/common.c
>> index 3dc54f59cafe..c3fd2facfc2d 100644
>> --- a/drivers/soc/tegra/common.c
>> +++ b/drivers/soc/tegra/common.c
>> @@ -3,9 +3,16 @@
>>   * Copyright (C) 2014 NVIDIA CORPORATION.  All rights reserved.
>>   */
>>  
>> +#define dev_fmt(fmt)	"tegra-soc: " fmt
>> +
>> +#include <linux/clk.h>
>> +#include <linux/device.h>
>> +#include <linux/export.h>
>>  #include <linux/of.h>
>> +#include <linux/pm_opp.h>
>>  
>>  #include <soc/tegra/common.h>
>> +#include <soc/tegra/fuse.h>
>>  
>>  static const struct of_device_id tegra_machine_match[] = {
>>  	{ .compatible = "nvidia,tegra20", },
>> @@ -31,3 +38,108 @@ bool soc_is_tegra(void)
>>  
>>  	return match != NULL;
>>  }
>> +
>> +static int tegra_core_dev_init_opp_state(struct device *dev)
>> +{
>> +	struct dev_pm_opp *opp;
>> +	unsigned long rate;
>> +	struct clk *clk;
>> +	int err;
>> +
>> +	clk = devm_clk_get(dev, NULL);
>> +	if (IS_ERR(clk)) {
>> +		dev_err(dev, "failed to get clk: %pe\n", clk);
>> +		return PTR_ERR(clk);
>> +	}
>> +
>> +	rate = clk_get_rate(clk);
>> +	if (!rate) {
>> +		dev_err(dev, "failed to get clk rate\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	opp = dev_pm_opp_find_freq_ceil(dev, &rate);
>> +
>> +	if (opp == ERR_PTR(-ERANGE))
>> +		opp = dev_pm_opp_find_freq_floor(dev, &rate);
>> +
>> +	err = PTR_ERR_OR_ZERO(opp);
>> +	if (err) {
>> +		dev_err(dev, "failed to get OPP for %ld Hz: %d\n",
>> +			rate, err);
>> +		return err;
>> +	}
> 
> Why do you need to do this floor/ceil thing? Why can't you simply do
> set-rate ? 

The previous versions of this patch had this comment:

/*
 * dev_pm_opp_set_rate() doesn't search for a floor clock rate and it
 * will error out if default clock rate is too high, i.e. unsupported
 * by a SoC hardware version.  Hence find floor rate by ourselves.
 */

I removed it because it appeared to me that it should be obvious why this is needed. The reason why it was added in the first place is that the tegra-clk driver initializes some clock rates to values that aren't supported by all hardware versions in accordance to the OPP tables, although technically those higher rates work okay in practice, this made dev_pm_opp_set_rate() to fail without fixing up the clock rate.

You might be right that this is not necessary anymore, the code changed since the last time it was needed. I'll re-check it for the v2. Thank you for the review.