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

[Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>]

On 17/05/2021 10:47, Dmitry Osipenko wrote:
> 17.05.2021 14:43, Krzysztof Kozlowski пишет:
> ...
>>> +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;
>>> +	}
>>> +
>>> +	dev_pm_opp_put(opp);
>>> +
>>> +	/* first dummy rate-setting initializes voltage vote */
>>> +	err = dev_pm_opp_set_rate(dev, rate);
>>> +	if (err) {
>>> +		dev_err(dev, "failed to initialize OPP clock: %d\n", err);
>>> +		return err;
>>> +	}
>>
>>
>> The devm_pm_opp_set_clkname will call clk_get(), so here you should drop
>> the clk reference at the end. Why having it twice?
> 
> The devm_pm_opp_set_clkname assigns clock to the OPP table.
> 
> The devm_clk_get() is needed for the clk_get_rate(). OPP core doesn't
> initialize voltage vote and we need this initialization for the Tegra
> memory drivers.

I did not get the answer to my question. Why you need to keep the clk
reference past this point? Why you cannot drop it after getting rate?

> The reference count of the clk will be dropped automatically once device
> driver is released. The resource-managed helper avoids the need to care
> about the error unwinding in the code, making it clean and easy to follow.

I am not saying there is a leak.

> 
> ...
>>> +EXPORT_SYMBOL_GPL(devm_tegra_core_dev_init_opp_table);
>>> diff --git a/include/soc/tegra/common.h b/include/soc/tegra/common.h
>>> index 98027a76ce3d..e8eab13aa199 100644
>>> --- a/include/soc/tegra/common.h
>>> +++ b/include/soc/tegra/common.h
>>> @@ -6,6 +6,36 @@
>>>  #ifndef __SOC_TEGRA_COMMON_H__
>>>  #define __SOC_TEGRA_COMMON_H__
>>>  
>>> +#include <linux/errno.h>
>>> +#include <linux/types.h>
>>> +
>>> +struct device;
>>> +
>>> +/**
>>> + * Tegra SoC core device OPP table configuration
>>> + *
>>> + * @init_state: pre-initialize OPP state of a device
>>> + */
>>> +struct tegra_core_opp_params {
>>> +	bool init_state;
>>> +};
>>> +
>>> +#ifdef CONFIG_ARCH_TEGRA
>>>  bool soc_is_tegra(void);
>>> +int devm_tegra_core_dev_init_opp_table(struct device *dev,
>>> +				       struct tegra_core_opp_params *params);
>>> +#else
>>> +static inline bool soc_is_tegra(void)
>>
>> This looks unrelated. Please make it a separate patch.
> 
> The missing stub for soc_is_tegra() popped up multiple times before.
> Hence it didn't look like a bad idea to me to add stub for it since this
> patch touches code around it.
> 
> I'll factor it out into a separate patch in v2.

Thanks!


Best regards,
Krzysztof