[PATCH v1 0/1] Add devm_tegra_core_dev_init_opp_table() helper

[PATCH v1 0/1] Add devm_tegra_core_dev_init_opp_table() helper

[Dmitry Osipenko]

Hello,

This patch adds a new devm_tegra_core_dev_init_opp_table() helper
which is specific to NVIDIA Tegra SoCs. The goal of this helper is to
remove code duplication from Tegra device drivers.

Previously this helper was a part of patchsets that added core power
domain support. Krzysztof Kozlowski suggested to separate patches into
smaller sets with explicit dependencies.

This helper is a mandatory prerequisite by a multiple patchsets that
will add more advanced power management to GPU/media/clk and etc drivers,
fixing overheating troubles of Tegra devices. It provides OPP support
for newer device-trees and preserves compatibility with the older DTBs.

Dmitry Osipenko (1):
  soc/tegra: Add devm_tegra_core_dev_init_opp_table()

 drivers/soc/tegra/common.c | 112 +++++++++++++++++++++++++++++++++++++
 include/soc/tegra/common.h |  30 ++++++++++
 2 files changed, 142 insertions(+)

-- 
2.30.2

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

[Dmitry Osipenko]

Add common helper which initializes OPP table for Tegra SoC core devices.

Tested-by: Peter Geis <pgwipeout@gmail.com> # Ouya T30
Tested-by: Paul Fertser <fercerpav@gmail.com> # PAZ00 T20
Tested-by: Nicolas Chauvet <kwizart@gmail.com> # PAZ00 T20 and TK1 T124
Tested-by: Matt Merhar <mattmerhar@protonmail.com> # Ouya T30
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 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;
+	}
+
+	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;
+	}
+
+	return 0;
+}
+
+/**
+ * devm_tegra_core_dev_init_opp_table() - initialize OPP table
+ * @dev: device for which OPP table is initialized
+ * @params: pointer to the OPP table configuration
+ *
+ * This function will initialize OPP table and sync OPP state of a Tegra SoC
+ * core device.
+ *
+ * Return: 0 on success or errorno.
+ */
+int devm_tegra_core_dev_init_opp_table(struct device *dev,
+				       struct tegra_core_opp_params *params)
+{
+	u32 hw_version;
+	int err;
+
+	err = devm_pm_opp_set_clkname(dev, NULL);
+	if (err) {
+		dev_err(dev, "failed to set OPP clk: %d\n", err);
+		return err;
+	}
+
+	/* Tegra114+ doesn't support OPP yet */
+	if (!of_machine_is_compatible("nvidia,tegra20") &&
+	    !of_machine_is_compatible("nvidia,tegra30"))
+		return -ENODEV;
+
+	if (of_machine_is_compatible("nvidia,tegra20"))
+		hw_version = BIT(tegra_sku_info.soc_process_id);
+	else
+		hw_version = BIT(tegra_sku_info.soc_speedo_id);
+
+	err = devm_pm_opp_set_supported_hw(dev, &hw_version, 1);
+	if (err) {
+		dev_err(dev, "failed to set OPP supported HW: %d\n", err);
+		return err;
+	}
+
+	/*
+	 * Older device-trees have an empty OPP table, we will get
+	 * -ENODEV from devm_pm_opp_of_add_table() in this case.
+	 */
+	err = devm_pm_opp_of_add_table(dev);
+	if (err) {
+		if (err == -ENODEV)
+			dev_err_once(dev, "OPP table not found, please update device-tree\n");
+		else
+			dev_err(dev, "failed to add OPP table: %d\n", err);
+
+		return err;
+	}
+
+	if (params->init_state) {
+		err = tegra_core_dev_init_opp_state(dev);
+		if (err)
+			return err;
+	}
+
+	return 0;
+}
+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)
+{
+	return false;
+}
+
+static inline int
+devm_tegra_core_dev_init_opp_table(struct device *dev,
+				   struct tegra_core_opp_params *params)
+{
+	return -ENODEV;
+}
+#endif
 
 #endif /* __SOC_TEGRA_COMMON_H__ */
-- 
2.30.2

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

[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.

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@gmail.com> # Ouya T30
> Tested-by: Paul Fertser <fercerpav@gmail.com> # PAZ00 T20
> Tested-by: Nicolas Chauvet <kwizart@gmail.com> # PAZ00 T20 and TK1 T124
> Tested-by: Matt Merhar <mattmerhar@protonmail.com> # Ouya T30
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  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 ? 

> +
> +	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;
> +	}
> +
> +	return 0;
> +}

-- 
viresh

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

[Krzysztof Kozlowski]

On 16/05/2021 16:51, Dmitry Osipenko wrote:
> Add common helper which initializes OPP table for Tegra SoC core devices.
> 
> Tested-by: Peter Geis <pgwipeout@gmail.com> # Ouya T30
> Tested-by: Paul Fertser <fercerpav@gmail.com> # PAZ00 T20
> Tested-by: Nicolas Chauvet <kwizart@gmail.com> # PAZ00 T20 and TK1 T124
> Tested-by: Matt Merhar <mattmerhar@protonmail.com> # Ouya T30
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  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;
> +	}
> +
> +	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?

> +
> +	return 0;
> +}
> +
> +/**
> + * devm_tegra_core_dev_init_opp_table() - initialize OPP table
> + * @dev: device for which OPP table is initialized
> + * @params: pointer to the OPP table configuration
> + *
> + * This function will initialize OPP table and sync OPP state of a Tegra SoC
> + * core device.
> + *
> + * Return: 0 on success or errorno.
> + */
> +int devm_tegra_core_dev_init_opp_table(struct device *dev,
> +				       struct tegra_core_opp_params *params)
> +{
> +	u32 hw_version;
> +	int err;
> +
> +	err = devm_pm_opp_set_clkname(dev, NULL);
> +	if (err) {
> +		dev_err(dev, "failed to set OPP clk: %d\n", err);
> +		return err;
> +	}
> +
> +	/* Tegra114+ doesn't support OPP yet */
> +	if (!of_machine_is_compatible("nvidia,tegra20") &&
> +	    !of_machine_is_compatible("nvidia,tegra30"))
> +		return -ENODEV;
> +
> +	if (of_machine_is_compatible("nvidia,tegra20"))
> +		hw_version = BIT(tegra_sku_info.soc_process_id);
> +	else
> +		hw_version = BIT(tegra_sku_info.soc_speedo_id);
> +
> +	err = devm_pm_opp_set_supported_hw(dev, &hw_version, 1);
> +	if (err) {
> +		dev_err(dev, "failed to set OPP supported HW: %d\n", err);
> +		return err;
> +	}
> +
> +	/*
> +	 * Older device-trees have an empty OPP table, we will get
> +	 * -ENODEV from devm_pm_opp_of_add_table() in this case.
> +	 */
> +	err = devm_pm_opp_of_add_table(dev);
> +	if (err) {
> +		if (err == -ENODEV)
> +			dev_err_once(dev, "OPP table not found, please update device-tree\n");
> +		else
> +			dev_err(dev, "failed to add OPP table: %d\n", err);
> +
> +		return err;
> +	}
> +
> +	if (params->init_state) {
> +		err = tegra_core_dev_init_opp_state(dev);
> +		if (err)
> +			return err;
> +	}
> +
> +	return 0;
> +}
> +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.

> +{
> +	return false;
> +}
> +
> +static inline int
> +devm_tegra_core_dev_init_opp_table(struct device *dev,
> +				   struct tegra_core_opp_params *params)
> +{
> +	return -ENODEV;
> +}
> +#endif
>  
>  #endif /* __SOC_TEGRA_COMMON_H__ */
> 


Best regards,
Krzysztof

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@gmail.com> # Ouya T30
>> Tested-by: Paul Fertser <fercerpav@gmail.com> # PAZ00 T20
>> Tested-by: Nicolas Chauvet <kwizart@gmail.com> # PAZ00 T20 and TK1 T124
>> Tested-by: Matt Merhar <mattmerhar@protonmail.com> # Ouya T30
>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>> ---
>>  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.

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

[Dmitry Osipenko]

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.

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.

...
>> +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.

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

[Krzysztof Kozlowski]

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

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

[Dmitry Osipenko]

17.05.2021 17:52, 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.
> 

The clk reference is not needed past this point. It doesn't hurt to have
additional reference since this allows to make code cleaner.

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

[Viresh Kumar]

On 17-05-21, 17:09, Dmitry Osipenko wrote:
> 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. 

Diving the series is fine, but an API and its users should always be
in the same series. You can still apply them differently anyway.

> 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.

It can never be obvious to anyone without looking at the API in
detail. So if it is indeed required, please keep the comment as is.

-- 
viresh