[PATCH v10 0/8] NVIDIA Tegra power management patches for 5.16

[PATCH v10 0/8] NVIDIA Tegra power management patches for 5.16

[Dmitry Osipenko]

This is a reduced version of the patchset which adds power management
support to NVIDIA Tegra drivers. Viresh Kumar asked to send these PD/OPP
patches separately for now to reduce the noise and finalize the review.

I implemented new dev_get_performance_state() GENPD callback as was
discussed in v8/v9. GR3D driver patch shows how it's used by consumer
drivers.

v10: - Replaced dev_pm_opp_from_clk_rate() with dev_pm_opp_get_current(),
       as was requested by Viresh Kumar.

     - Added more comments to the code and extended commit message,
       as was requested by Viresh Kumar and Ulf Hansson.

     - Renamed get_performance_state() to dev_get_performance_state(),
       as was requested by Ulf Hansson.

     - Factored out 'performance' code out of __genpd_dev_pm_attach() into
       a separate function genpd_dev_initialize_performance_state(), as was
       requested by Ulf Hansson.

     - Removed dev_suspended argument from dev_get_performance_state(),
       as was requested by Ulf Hansson. It's replaced by the usage of
       pm_runtime_status_suspended(), see genpd_dev_get_current_performance_state().

Dmitry Osipenko (8):
  opp: Add dev_pm_opp_get_current()
  opp: Allow dev_pm_opp_set_clkname() to replace released clock
  opp: Change type of dev_pm_opp_attach_genpd(names) argument
  PM: domains: Add dev_get_performance_state() callback
  soc/tegra: pmc: Implement dev_get_performance_state() callback
  soc/tegra: Add devm_tegra_core_dev_init_opp_table_simple()
  gpu: host1x: Add host1x_channel_stop()
  drm/tegra: gr3d: Support generic power domain and runtime PM

 drivers/base/power/domain.c  |  90 ++++++--
 drivers/gpu/drm/tegra/gr3d.c | 384 ++++++++++++++++++++++++++++++-----
 drivers/gpu/host1x/channel.c |   8 +
 drivers/opp/core.c           |  51 ++++-
 drivers/soc/tegra/pmc.c      | 101 +++++++++
 include/linux/host1x.h       |   1 +
 include/linux/pm_domain.h    |   2 +
 include/linux/pm_opp.h       |  14 +-
 include/soc/tegra/common.h   |  13 ++
 9 files changed, 586 insertions(+), 78 deletions(-)

-- 
2.32.0

[PATCH v10 1/8] opp: Add dev_pm_opp_get_current()

[Dmitry Osipenko]

Add dev_pm_opp_get_current() helper that returns OPP corresponding
to the current clock rate of a device.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/opp/core.c     | 43 +++++++++++++++++++++++++++++++++++++++---
 include/linux/pm_opp.h |  6 ++++++
 2 files changed, 46 insertions(+), 3 deletions(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 04b4691a8aac..dde8a5cc948c 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -939,7 +939,7 @@ static int _set_required_opps(struct device *dev,
 	return ret;
 }
 
-static void _find_current_opp(struct device *dev, struct opp_table *opp_table)
+static struct dev_pm_opp *_find_current_opp(struct opp_table *opp_table)
 {
 	struct dev_pm_opp *opp = ERR_PTR(-ENODEV);
 	unsigned long freq;
@@ -949,6 +949,18 @@ static void _find_current_opp(struct device *dev, struct opp_table *opp_table)
 		opp = _find_freq_ceil(opp_table, &freq);
 	}
 
+	return opp;
+}
+
+static void _find_and_set_current_opp(struct opp_table *opp_table)
+{
+	struct dev_pm_opp *opp;
+
+	if (opp_table->current_opp)
+		return;
+
+	opp = _find_current_opp(opp_table);
+
 	/*
 	 * Unable to find the current OPP ? Pick the first from the list since
 	 * it is in ascending order, otherwise rest of the code will need to
@@ -1002,8 +1014,7 @@ static int _set_opp(struct device *dev, struct opp_table *opp_table,
 		return _disable_opp_table(dev, opp_table);
 
 	/* Find the currently set OPP if we don't know already */
-	if (unlikely(!opp_table->current_opp))
-		_find_current_opp(dev, opp_table);
+	_find_and_set_current_opp(opp_table);
 
 	old_opp = opp_table->current_opp;
 
@@ -2931,3 +2942,29 @@ int dev_pm_opp_sync_regulators(struct device *dev)
 	return ret;
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_sync_regulators);
+
+/**
+ * dev_pm_opp_get_current() - Get current OPP
+ * @dev:	device for which we do this operation
+ *
+ * Get current OPP of a device based on current clock rate or by other means.
+ *
+ * Return: pointer to 'struct dev_pm_opp' on success and errorno otherwise.
+ */
+struct dev_pm_opp *dev_pm_opp_get_current(struct device *dev)
+{
+	struct opp_table *opp_table;
+	struct dev_pm_opp *opp;
+
+	opp_table = _find_opp_table(dev);
+	if (IS_ERR(opp_table))
+		return ERR_CAST(opp_table);
+
+	opp = _find_current_opp(opp_table);
+
+	/* Drop reference taken by _find_opp_table() */
+	dev_pm_opp_put_opp_table(opp_table);
+
+	return opp;
+}
+EXPORT_SYMBOL_GPL(dev_pm_opp_get_current);
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index 84150a22fd7c..c8091977efb8 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -168,6 +168,7 @@ int dev_pm_opp_get_sharing_cpus(struct device *cpu_dev, struct cpumask *cpumask)
 void dev_pm_opp_remove_table(struct device *dev);
 void dev_pm_opp_cpumask_remove_table(const struct cpumask *cpumask);
 int dev_pm_opp_sync_regulators(struct device *dev);
+struct dev_pm_opp *dev_pm_opp_get_current(struct device *dev);
 #else
 static inline struct opp_table *dev_pm_opp_get_opp_table(struct device *dev)
 {
@@ -434,6 +435,11 @@ static inline int dev_pm_opp_sync_regulators(struct device *dev)
 	return -EOPNOTSUPP;
 }
 
+static inline struct dev_pm_opp *dev_pm_opp_get_current(struct device *dev)
+{
+	return ERR_PTR(-EOPNOTSUPP);
+}
+
 #endif		/* CONFIG_PM_OPP */
 
 #if defined(CONFIG_PM_OPP) && defined(CONFIG_OF)
-- 
2.32.0

Re: [PATCH v10 1/8] opp: Add dev_pm_opp_get_current()

[Viresh Kumar]

On 31-08-21, 16:54, Dmitry Osipenko wrote:
> Add dev_pm_opp_get_current() helper that returns OPP corresponding
> to the current clock rate of a device.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/opp/core.c     | 43 +++++++++++++++++++++++++++++++++++++++---
>  include/linux/pm_opp.h |  6 ++++++
>  2 files changed, 46 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index 04b4691a8aac..dde8a5cc948c 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -939,7 +939,7 @@ static int _set_required_opps(struct device *dev,
>  	return ret;
>  }
>  
> -static void _find_current_opp(struct device *dev, struct opp_table *opp_table)
> +static struct dev_pm_opp *_find_current_opp(struct opp_table *opp_table)
>  {
>  	struct dev_pm_opp *opp = ERR_PTR(-ENODEV);
>  	unsigned long freq;
> @@ -949,6 +949,18 @@ static void _find_current_opp(struct device *dev, struct opp_table *opp_table)
>  		opp = _find_freq_ceil(opp_table, &freq);
>  	}
>  
> +	return opp;
> +}
> +
> +static void _find_and_set_current_opp(struct opp_table *opp_table)
> +{
> +	struct dev_pm_opp *opp;
> +
> +	if (opp_table->current_opp)
> +		return;

Why move this from caller as well ?

> +
> +	opp = _find_current_opp(opp_table);
> +
>  	/*
>  	 * Unable to find the current OPP ? Pick the first from the list since
>  	 * it is in ascending order, otherwise rest of the code will need to

If we aren't able to find current OPP based on current freq, then this
picks the first value from the list. Why shouldn't this be done in
your case as well ?

> @@ -1002,8 +1014,7 @@ static int _set_opp(struct device *dev, struct opp_table *opp_table,
>  		return _disable_opp_table(dev, opp_table);
>  
>  	/* Find the currently set OPP if we don't know already */
> -	if (unlikely(!opp_table->current_opp))
> -		_find_current_opp(dev, opp_table);
> +	_find_and_set_current_opp(opp_table);
>  
>  	old_opp = opp_table->current_opp;
>  
> @@ -2931,3 +2942,29 @@ int dev_pm_opp_sync_regulators(struct device *dev)
>  	return ret;
>  }
>  EXPORT_SYMBOL_GPL(dev_pm_opp_sync_regulators);
> +
> +/**
> + * dev_pm_opp_get_current() - Get current OPP
> + * @dev:	device for which we do this operation
> + *
> + * Get current OPP of a device based on current clock rate or by other means.
> + *
> + * Return: pointer to 'struct dev_pm_opp' on success and errorno otherwise.
> + */
> +struct dev_pm_opp *dev_pm_opp_get_current(struct device *dev)
> +{
> +	struct opp_table *opp_table;
> +	struct dev_pm_opp *opp;
> +
> +	opp_table = _find_opp_table(dev);
> +	if (IS_ERR(opp_table))
> +		return ERR_CAST(opp_table);
> +
> +	opp = _find_current_opp(opp_table);

This should not just go find the OPP based on current freq. This first
needs to check if curret_opp is set or not. If set, then just return
it, else run the _find_current_opp() function and update the
current_opp pointer as well.

-- 
viresh

Re: [PATCH v10 1/8] opp: Add dev_pm_opp_get_current()

[Dmitry Osipenko]

01.09.2021 07:39, Viresh Kumar пишет:
> On 31-08-21, 16:54, Dmitry Osipenko wrote:
>> Add dev_pm_opp_get_current() helper that returns OPP corresponding
>> to the current clock rate of a device.
>>
>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>> ---
>>  drivers/opp/core.c     | 43 +++++++++++++++++++++++++++++++++++++++---
>>  include/linux/pm_opp.h |  6 ++++++
>>  2 files changed, 46 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
>> index 04b4691a8aac..dde8a5cc948c 100644
>> --- a/drivers/opp/core.c
>> +++ b/drivers/opp/core.c
>> @@ -939,7 +939,7 @@ static int _set_required_opps(struct device *dev,
>>  	return ret;
>>  }
>>  
>> -static void _find_current_opp(struct device *dev, struct opp_table *opp_table)
>> +static struct dev_pm_opp *_find_current_opp(struct opp_table *opp_table)
>>  {
>>  	struct dev_pm_opp *opp = ERR_PTR(-ENODEV);
>>  	unsigned long freq;
>> @@ -949,6 +949,18 @@ static void _find_current_opp(struct device *dev, struct opp_table *opp_table)
>>  		opp = _find_freq_ceil(opp_table, &freq);
>>  	}
>>  
>> +	return opp;
>> +}
>> +
>> +static void _find_and_set_current_opp(struct opp_table *opp_table)
>> +{
>> +	struct dev_pm_opp *opp;
>> +
>> +	if (opp_table->current_opp)
>> +		return;
> 
> Why move this from caller as well ?

To make code cleaner.

>> +
>> +	opp = _find_current_opp(opp_table);
>> +
>>  	/*
>>  	 * Unable to find the current OPP ? Pick the first from the list since
>>  	 * it is in ascending order, otherwise rest of the code will need to
> 
> If we aren't able to find current OPP based on current freq, then this
> picks the first value from the list. Why shouldn't this be done in
> your case as well ?

You will get OPP which corresponds to the lowest freq, while h/w runs on
unsupported high freq. This may end with a tragedy.

>> @@ -1002,8 +1014,7 @@ static int _set_opp(struct device *dev, struct opp_table *opp_table,
>>  		return _disable_opp_table(dev, opp_table);
>>  
>>  	/* Find the currently set OPP if we don't know already */
>> -	if (unlikely(!opp_table->current_opp))
>> -		_find_current_opp(dev, opp_table);
>> +	_find_and_set_current_opp(opp_table);
>>  
>>  	old_opp = opp_table->current_opp;
>>  
>> @@ -2931,3 +2942,29 @@ int dev_pm_opp_sync_regulators(struct device *dev)
>>  	return ret;
>>  }
>>  EXPORT_SYMBOL_GPL(dev_pm_opp_sync_regulators);
>> +
>> +/**
>> + * dev_pm_opp_get_current() - Get current OPP
>> + * @dev:	device for which we do this operation
>> + *
>> + * Get current OPP of a device based on current clock rate or by other means.
>> + *
>> + * Return: pointer to 'struct dev_pm_opp' on success and errorno otherwise.
>> + */
>> +struct dev_pm_opp *dev_pm_opp_get_current(struct device *dev)
>> +{
>> +	struct opp_table *opp_table;
>> +	struct dev_pm_opp *opp;
>> +
>> +	opp_table = _find_opp_table(dev);
>> +	if (IS_ERR(opp_table))
>> +		return ERR_CAST(opp_table);
>> +
>> +	opp = _find_current_opp(opp_table);
> 
> This should not just go find the OPP based on current freq. This first
> needs to check if curret_opp is set or not. If set, then just return
> it, else run the _find_current_opp() function and update the
> current_opp pointer as well.
> 

Alright, I'll change it.

Re: [PATCH v10 1/8] opp: Add dev_pm_opp_get_current()

[Viresh Kumar]

On 01-09-21, 08:43, Dmitry Osipenko wrote:
> You will get OPP which corresponds to the lowest freq, while h/w runs on
> unsupported high freq. This may end with a tragedy.

Yeah, because you are setting a performance state with this, it can be
a problem.

-- 
viresh

[PATCH v10 2/8] opp: Allow dev_pm_opp_set_clkname() to replace released clock

[Dmitry Osipenko]

The opp_table->clk is set to error once clock is released by
dev_pm_opp_put_clkname(). This doesn't allow to set clock again,
until OPP table is re-created from scratch. Check opp_table->clk
for both NULL and ERR_PTR to allow the clock's replacement.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/opp/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index dde8a5cc948c..602e502d092e 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -2146,7 +2146,7 @@ struct opp_table *dev_pm_opp_set_clkname(struct device *dev, const char *name)
 	}
 
 	/* clk shouldn't be initialized at this point */
-	if (WARN_ON(opp_table->clk)) {
+	if (WARN_ON(!IS_ERR_OR_NULL(opp_table->clk))) {
 		ret = -EBUSY;
 		goto err;
 	}
-- 
2.32.0

Re: [PATCH v10 2/8] opp: Allow dev_pm_opp_set_clkname() to replace released clock

[Viresh Kumar]

On 31-08-21, 16:54, Dmitry Osipenko wrote:
> The opp_table->clk is set to error once clock is released by
> dev_pm_opp_put_clkname(). This doesn't allow to set clock again,

I am not sure why are you required to set the clk again here ? I mean,
users aren't expected to put clkname in the middle of using it. The
set-name API also checks that the OPP list should be empty in such a
case.

> until OPP table is re-created from scratch. Check opp_table->clk
> for both NULL and ERR_PTR to allow the clock's replacement.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/opp/core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index dde8a5cc948c..602e502d092e 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -2146,7 +2146,7 @@ struct opp_table *dev_pm_opp_set_clkname(struct device *dev, const char *name)
>  	}
>  
>  	/* clk shouldn't be initialized at this point */
> -	if (WARN_ON(opp_table->clk)) {
> +	if (WARN_ON(!IS_ERR_OR_NULL(opp_table->clk))) {
>  		ret = -EBUSY;
>  		goto err;
>  	}
> -- 
> 2.32.0

-- 
viresh

Re: [PATCH v10 2/8] opp: Allow dev_pm_opp_set_clkname() to replace released clock

[Dmitry Osipenko]

01.09.2021 07:42, Viresh Kumar пишет:
> On 31-08-21, 16:54, Dmitry Osipenko wrote:
>> The opp_table->clk is set to error once clock is released by
>> dev_pm_opp_put_clkname(). This doesn't allow to set clock again,
> 
> I am not sure why are you required to set the clk again here ? I mean,
> users aren't expected to put clkname in the middle of using it. The
> set-name API also checks that the OPP list should be empty in such a
> case.

I added explanatory comment to tegra_pmc_pd_dev_get_performance_state(),
isn't it enough?

Re: [PATCH v10 2/8] opp: Allow dev_pm_opp_set_clkname() to replace released clock

[Viresh Kumar]

On 01-09-21, 08:46, Dmitry Osipenko wrote:
> 01.09.2021 07:42, Viresh Kumar пишет:
> > On 31-08-21, 16:54, Dmitry Osipenko wrote:
> >> The opp_table->clk is set to error once clock is released by
> >> dev_pm_opp_put_clkname(). This doesn't allow to set clock again,
> > 
> > I am not sure why are you required to set the clk again here ? I mean,
> > users aren't expected to put clkname in the middle of using it. The
> > set-name API also checks that the OPP list should be empty in such a
> > case.
> 
> I added explanatory comment to tegra_pmc_pd_dev_get_performance_state(),
> isn't it enough?

It confused me even more. Lemme comment there.

-- 
viresh

[PATCH v10 3/8] opp: Change type of dev_pm_opp_attach_genpd(names) argument

[Dmitry Osipenko]

Elements of the 'names' array are not changed by the code, constify them
for consistency.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/opp/core.c     | 6 +++---
 include/linux/pm_opp.h | 8 ++++----
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 602e502d092e..d4e706a8b70d 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -2359,12 +2359,12 @@ static void _opp_detach_genpd(struct opp_table *opp_table)
  * "required-opps" are added in DT.
  */
 struct opp_table *dev_pm_opp_attach_genpd(struct device *dev,
-		const char **names, struct device ***virt_devs)
+		const char * const *names, struct device ***virt_devs)
 {
 	struct opp_table *opp_table;
 	struct device *virt_dev;
 	int index = 0, ret = -EINVAL;
-	const char **name = names;
+	const char * const *name = names;
 
 	opp_table = _add_opp_table(dev, false);
 	if (IS_ERR(opp_table))
@@ -2468,7 +2468,7 @@ static void devm_pm_opp_detach_genpd(void *data)
  *
  * Return: 0 on success and errorno otherwise.
  */
-int devm_pm_opp_attach_genpd(struct device *dev, const char **names,
+int devm_pm_opp_attach_genpd(struct device *dev, const char * const *names,
 			     struct device ***virt_devs)
 {
 	struct opp_table *opp_table;
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index c8091977efb8..37d5d71e0835 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -156,9 +156,9 @@ int devm_pm_opp_set_clkname(struct device *dev, const char *name);
 struct opp_table *dev_pm_opp_register_set_opp_helper(struct device *dev, int (*set_opp)(struct dev_pm_set_opp_data *data));
 void dev_pm_opp_unregister_set_opp_helper(struct opp_table *opp_table);
 int devm_pm_opp_register_set_opp_helper(struct device *dev, int (*set_opp)(struct dev_pm_set_opp_data *data));
-struct opp_table *dev_pm_opp_attach_genpd(struct device *dev, const char **names, struct device ***virt_devs);
+struct opp_table *dev_pm_opp_attach_genpd(struct device *dev, const char * const *names, struct device ***virt_devs);
 void dev_pm_opp_detach_genpd(struct opp_table *opp_table);
-int devm_pm_opp_attach_genpd(struct device *dev, const char **names, struct device ***virt_devs);
+int devm_pm_opp_attach_genpd(struct device *dev, const char * const *names, struct device ***virt_devs);
 struct dev_pm_opp *dev_pm_opp_xlate_required_opp(struct opp_table *src_table, struct opp_table *dst_table, struct dev_pm_opp *src_opp);
 int dev_pm_opp_xlate_performance_state(struct opp_table *src_table, struct opp_table *dst_table, unsigned int pstate);
 int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq);
@@ -377,7 +377,7 @@ static inline int devm_pm_opp_set_clkname(struct device *dev, const char *name)
 	return -EOPNOTSUPP;
 }
 
-static inline struct opp_table *dev_pm_opp_attach_genpd(struct device *dev, const char **names, struct device ***virt_devs)
+static inline struct opp_table *dev_pm_opp_attach_genpd(struct device *dev, const char * const *names, struct device ***virt_devs)
 {
 	return ERR_PTR(-EOPNOTSUPP);
 }
@@ -385,7 +385,7 @@ static inline struct opp_table *dev_pm_opp_attach_genpd(struct device *dev, cons
 static inline void dev_pm_opp_detach_genpd(struct opp_table *opp_table) {}
 
 static inline int devm_pm_opp_attach_genpd(struct device *dev,
-					   const char **names,
+					   const char * const *names,
 					   struct device ***virt_devs)
 {
 	return -EOPNOTSUPP;
-- 
2.32.0

Re: [PATCH v10 3/8] opp: Change type of dev_pm_opp_attach_genpd(names) argument

[Viresh Kumar]

On 31-08-21, 16:54, Dmitry Osipenko wrote:
> Elements of the 'names' array are not changed by the code, constify them
> for consistency.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/opp/core.c     | 6 +++---
>  include/linux/pm_opp.h | 8 ++++----
>  2 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index 602e502d092e..d4e706a8b70d 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -2359,12 +2359,12 @@ static void _opp_detach_genpd(struct opp_table *opp_table)
>   * "required-opps" are added in DT.
>   */
>  struct opp_table *dev_pm_opp_attach_genpd(struct device *dev,
> -		const char **names, struct device ***virt_devs)
> +		const char * const *names, struct device ***virt_devs)

I am sure there are issues around space around * here. Please run
checkpatch with --strict option for your series.

-- 
viresh

Re: [PATCH v10 3/8] opp: Change type of dev_pm_opp_attach_genpd(names) argument

[Dmitry Osipenko]

01.09.2021 07:41, Viresh Kumar пишет:
> On 31-08-21, 16:54, Dmitry Osipenko wrote:
>> Elements of the 'names' array are not changed by the code, constify them
>> for consistency.
>>
>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>> ---
>>  drivers/opp/core.c     | 6 +++---
>>  include/linux/pm_opp.h | 8 ++++----
>>  2 files changed, 7 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
>> index 602e502d092e..d4e706a8b70d 100644
>> --- a/drivers/opp/core.c
>> +++ b/drivers/opp/core.c
>> @@ -2359,12 +2359,12 @@ static void _opp_detach_genpd(struct opp_table *opp_table)
>>   * "required-opps" are added in DT.
>>   */
>>  struct opp_table *dev_pm_opp_attach_genpd(struct device *dev,
>> -		const char **names, struct device ***virt_devs)
>> +		const char * const *names, struct device ***virt_devs)
> 
> I am sure there are issues around space around * here. Please run
> checkpatch with --strict option for your series.
> 

It is the other way around. This fixes the checkpatch warning and that's
what checkpatch wants. You may also grep the kernel to find that this is
the only variant used in practice.

Re: [PATCH v10 3/8] opp: Change type of dev_pm_opp_attach_genpd(names) argument

[Viresh Kumar]

On 01-09-21, 08:44, Dmitry Osipenko wrote:
> 01.09.2021 07:41, Viresh Kumar пишет:
> > On 31-08-21, 16:54, Dmitry Osipenko wrote:
> >> Elements of the 'names' array are not changed by the code, constify them
> >> for consistency.
> >>
> >> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> >> ---
> >>  drivers/opp/core.c     | 6 +++---
> >>  include/linux/pm_opp.h | 8 ++++----
> >>  2 files changed, 7 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> >> index 602e502d092e..d4e706a8b70d 100644
> >> --- a/drivers/opp/core.c
> >> +++ b/drivers/opp/core.c
> >> @@ -2359,12 +2359,12 @@ static void _opp_detach_genpd(struct opp_table *opp_table)
> >>   * "required-opps" are added in DT.
> >>   */
> >>  struct opp_table *dev_pm_opp_attach_genpd(struct device *dev,
> >> -		const char **names, struct device ***virt_devs)
> >> +		const char * const *names, struct device ***virt_devs)
> > 
> > I am sure there are issues around space around * here. Please run
> > checkpatch with --strict option for your series.
> > 
> 
> It is the other way around. This fixes the checkpatch warning and that's
> what checkpatch wants. You may also grep the kernel to find that this is
> the only variant used in practice.

Heh, you are right. I somehow thought that * never has a space right
after.

-- 
viresh

[PATCH v10 4/8] PM: domains: Add dev_get_performance_state() callback

[Dmitry Osipenko]

Add optional dev_get_performance_state() callback that retrieves
performance state of a device attached to a power domain. The new callback
returns performance states of the given device, which should be read out
from hardware.

GENPD core assumes that initially performance state of all devices is
zero and consumer device driver is responsible for management of the
performance state. GENPD core now manages performance state across RPM
suspend/resume of consumer devices by dropping and restoring the state,
this allowed to move a part of performance management code into GENPD
core out of consumer drivers. The part that hasn't been moved to GENPD
core yet is the initialization of the performance state.

Hardware may be preprogrammed to a specific performance state which
isn't zero initially, hence the PD's performance state is inconsistent
with hardware at the init time. This requires each consumer driver to
explicitly sync the state. For some platforms this is a boilerplate code
replicated by each driver.

The new callback allows GENPD core to initialize the performance state,
allowing to remove the remaining boilerplate code from consumer drivers.
Now, when consumer device is resumed for the first time, the performance
state is either already set by GENPD core or it will be set in accordance
to the hardware state that was retrieved using the new callback when device
was attached to a power domain. Still, consumer drivers are free to override
the initial performance state configured by GENPD, if necessary.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/base/power/domain.c | 90 +++++++++++++++++++++++++++++++------
 include/linux/pm_domain.h   |  2 +
 2 files changed, 79 insertions(+), 13 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 5db704f02e71..598528abce01 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -2644,12 +2644,85 @@ static void genpd_dev_pm_sync(struct device *dev)
 	genpd_queue_power_off_work(pd);
 }
 
+static int genpd_dev_get_current_performance_state(struct device *dev,
+						   struct device *base_dev,
+						   unsigned int index,
+						   bool *state_default,
+						   bool *suspended)
+{
+	struct generic_pm_domain *pd = dev_to_genpd(dev);
+	int ret;
+
+	ret = of_get_required_opp_performance_state(dev->of_node, index);
+	if (ret < 0 && ret != -ENODEV && ret != -EOPNOTSUPP) {
+		dev_err(dev, "failed to get required performance state for power-domain %s: %d\n",
+			pd->name, ret);
+
+		return ret;
+	} else if (ret >= 0) {
+		*state_default = true;
+
+		return ret;
+	}
+
+	if (pd->dev_get_performance_state) {
+		ret = pd->dev_get_performance_state(pd, base_dev);
+		if (ret >= 0)
+			*suspended = pm_runtime_status_suspended(base_dev);
+		else
+			dev_err(dev, "failed to get performance state of %s for power-domain %s: %d\n",
+				dev_name(base_dev), pd->name, ret);
+
+		return ret;
+	}
+
+	return 0;
+}
+
+static int genpd_dev_initialize_performance_state(struct device *dev,
+						  struct device *base_dev,
+						  unsigned int index)
+{
+	struct generic_pm_domain *pd = dev_to_genpd(dev);
+	bool state_default = false, suspended = false;
+	int pstate, err;
+
+	pstate = genpd_dev_get_current_performance_state(dev, base_dev, index,
+							 &state_default,
+							 &suspended);
+	if (pstate <= 0)
+		return pstate;
+
+	/*
+	 * If device is suspended, then performance state will be applied
+	 * on RPM-resume. This prevents potential extra power consumption
+	 * if device won't be resumed soon.
+	 *
+	 * If device is known to be active at the moment, then the performance
+	 * state should be updated immediately to sync state with hardware.
+	 */
+	if (suspended) {
+		dev_gpd_data(dev)->rpm_pstate = pstate;
+	} else {
+		err = dev_pm_genpd_set_performance_state(dev, pstate);
+		if (err) {
+			dev_err(dev, "failed to set performance state for power-domain %s: %d\n",
+				pd->name, err);
+			return err;
+		}
+
+		if (state_default)
+			dev_gpd_data(dev)->default_pstate = pstate;
+	}
+
+	return 0;
+}
+
 static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev,
 				 unsigned int index, bool power_on)
 {
 	struct of_phandle_args pd_args;
 	struct generic_pm_domain *pd;
-	int pstate;
 	int ret;
 
 	ret = of_parse_phandle_with_args(dev->of_node, "power-domains",
@@ -2693,22 +2766,13 @@ static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev,
 		return -EPROBE_DEFER;
 	}
 
-	/* Set the default performance state */
-	pstate = of_get_required_opp_performance_state(dev->of_node, index);
-	if (pstate < 0 && pstate != -ENODEV && pstate != -EOPNOTSUPP) {
-		ret = pstate;
+	ret = genpd_dev_initialize_performance_state(dev, base_dev, index);
+	if (ret)
 		goto err;
-	} else if (pstate > 0) {
-		ret = dev_pm_genpd_set_performance_state(dev, pstate);
-		if (ret)
-			goto err;
-		dev_gpd_data(dev)->default_pstate = pstate;
-	}
+
 	return 1;
 
 err:
-	dev_err(dev, "failed to set required performance state for power-domain %s: %d\n",
-		pd->name, ret);
 	genpd_remove_device(pd, dev);
 	return ret;
 }
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index 67017c9390c8..0a4dd4986f34 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -133,6 +133,8 @@ struct generic_pm_domain {
 						 struct dev_pm_opp *opp);
 	int (*set_performance_state)(struct generic_pm_domain *genpd,
 				     unsigned int state);
+	int (*dev_get_performance_state)(struct generic_pm_domain *genpd,
+					 struct device *dev);
 	struct gpd_dev_ops dev_ops;
 	s64 max_off_time_ns;	/* Maximum allowed "suspended" time. */
 	ktime_t next_wakeup;	/* Maintained by the domain governor */
-- 
2.32.0

Re: [PATCH v10 4/8] PM: domains: Add dev_get_performance_state() callback

[Ulf Hansson]

On Tue, 31 Aug 2021 at 15:57, Dmitry Osipenko <digetx@gmail.com> wrote:
>
> Add optional dev_get_performance_state() callback that retrieves
> performance state of a device attached to a power domain. The new callback
> returns performance states of the given device, which should be read out
> from hardware.
>
> GENPD core assumes that initially performance state of all devices is
> zero and consumer device driver is responsible for management of the
> performance state. GENPD core now manages performance state across RPM
> suspend/resume of consumer devices by dropping and restoring the state,
> this allowed to move a part of performance management code into GENPD
> core out of consumer drivers. The part that hasn't been moved to GENPD
> core yet is the initialization of the performance state.
>
> Hardware may be preprogrammed to a specific performance state which
> isn't zero initially, hence the PD's performance state is inconsistent
> with hardware at the init time. This requires each consumer driver to
> explicitly sync the state. For some platforms this is a boilerplate code
> replicated by each driver.
>
> The new callback allows GENPD core to initialize the performance state,
> allowing to remove the remaining boilerplate code from consumer drivers.
> Now, when consumer device is resumed for the first time, the performance
> state is either already set by GENPD core or it will be set in accordance
> to the hardware state that was retrieved using the new callback when device
> was attached to a power domain. Still, consumer drivers are free to override
> the initial performance state configured by GENPD, if necessary.

Thanks for improving the commit message, it makes much better sense now!

>
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/base/power/domain.c | 90 +++++++++++++++++++++++++++++++------
>  include/linux/pm_domain.h   |  2 +
>  2 files changed, 79 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 5db704f02e71..598528abce01 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -2644,12 +2644,85 @@ static void genpd_dev_pm_sync(struct device *dev)
>         genpd_queue_power_off_work(pd);
>  }
>
> +static int genpd_dev_get_current_performance_state(struct device *dev,
> +                                                  struct device *base_dev,
> +                                                  unsigned int index,
> +                                                  bool *state_default,
> +                                                  bool *suspended)
> +{
> +       struct generic_pm_domain *pd = dev_to_genpd(dev);
> +       int ret;
> +
> +       ret = of_get_required_opp_performance_state(dev->of_node, index);
> +       if (ret < 0 && ret != -ENODEV && ret != -EOPNOTSUPP) {
> +               dev_err(dev, "failed to get required performance state for power-domain %s: %d\n",
> +                       pd->name, ret);
> +
> +               return ret;
> +       } else if (ret >= 0) {
> +               *state_default = true;
> +
> +               return ret;
> +       }
> +
> +       if (pd->dev_get_performance_state) {
> +               ret = pd->dev_get_performance_state(pd, base_dev);
> +               if (ret >= 0)
> +                       *suspended = pm_runtime_status_suspended(base_dev);
> +               else
> +                       dev_err(dev, "failed to get performance state of %s for power-domain %s: %d\n",
> +                               dev_name(base_dev), pd->name, ret);
> +
> +               return ret;
> +       }
> +
> +       return 0;
> +}
> +
> +static int genpd_dev_initialize_performance_state(struct device *dev,
> +                                                 struct device *base_dev,
> +                                                 unsigned int index)
> +{
> +       struct generic_pm_domain *pd = dev_to_genpd(dev);
> +       bool state_default = false, suspended = false;
> +       int pstate, err;
> +
> +       pstate = genpd_dev_get_current_performance_state(dev, base_dev, index,
> +                                                        &state_default,
> +                                                        &suspended);
> +       if (pstate <= 0)
> +               return pstate;
> +
> +       /*
> +        * If device is suspended, then performance state will be applied
> +        * on RPM-resume. This prevents potential extra power consumption
> +        * if device won't be resumed soon.
> +        *
> +        * If device is known to be active at the moment, then the performance
> +        * state should be updated immediately to sync state with hardware.
> +        */
> +       if (suspended) {
> +               dev_gpd_data(dev)->rpm_pstate = pstate;
> +       } else {
> +               err = dev_pm_genpd_set_performance_state(dev, pstate);
> +               if (err) {
> +                       dev_err(dev, "failed to set performance state for power-domain %s: %d\n",
> +                               pd->name, err);
> +                       return err;
> +               }
> +
> +               if (state_default)
> +                       dev_gpd_data(dev)->default_pstate = pstate;
> +       }
> +
> +       return 0;
> +}

As I kind of indicated in my previous reply on the earlier version, I
think the above code can be made a lot less complicated. Although,
perhaps I may be missing some points.

Anyway, I will post a couple patches, later this evening or tomorrow,
to show more exactly what I had in mind. Let's see if that can work
for you.

> +
>  static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev,
>                                  unsigned int index, bool power_on)
>  {
>         struct of_phandle_args pd_args;
>         struct generic_pm_domain *pd;
> -       int pstate;
>         int ret;
>
>         ret = of_parse_phandle_with_args(dev->of_node, "power-domains",
> @@ -2693,22 +2766,13 @@ static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev,
>                 return -EPROBE_DEFER;
>         }
>
> -       /* Set the default performance state */
> -       pstate = of_get_required_opp_performance_state(dev->of_node, index);
> -       if (pstate < 0 && pstate != -ENODEV && pstate != -EOPNOTSUPP) {
> -               ret = pstate;
> +       ret = genpd_dev_initialize_performance_state(dev, base_dev, index);
> +       if (ret)
>                 goto err;
> -       } else if (pstate > 0) {
> -               ret = dev_pm_genpd_set_performance_state(dev, pstate);
> -               if (ret)
> -                       goto err;
> -               dev_gpd_data(dev)->default_pstate = pstate;
> -       }
> +
>         return 1;
>
>  err:
> -       dev_err(dev, "failed to set required performance state for power-domain %s: %d\n",
> -               pd->name, ret);
>         genpd_remove_device(pd, dev);
>         return ret;
>  }
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index 67017c9390c8..0a4dd4986f34 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -133,6 +133,8 @@ struct generic_pm_domain {
>                                                  struct dev_pm_opp *opp);
>         int (*set_performance_state)(struct generic_pm_domain *genpd,
>                                      unsigned int state);
> +       int (*dev_get_performance_state)(struct generic_pm_domain *genpd,
> +                                        struct device *dev);
>         struct gpd_dev_ops dev_ops;
>         s64 max_off_time_ns;    /* Maximum allowed "suspended" time. */
>         ktime_t next_wakeup;    /* Maintained by the domain governor */
> --
> 2.32.0
>

Kind regards
Uffe

Re: [PATCH v10 4/8] PM: domains: Add dev_get_performance_state() callback

[Dmitry Osipenko]

01.09.2021 19:59, Ulf Hansson пишет:
>> +static int genpd_dev_initialize_performance_state(struct device *dev,
>> +                                                 struct device *base_dev,
>> +                                                 unsigned int index)
>> +{
>> +       struct generic_pm_domain *pd = dev_to_genpd(dev);
>> +       bool state_default = false, suspended = false;
>> +       int pstate, err;
>> +
>> +       pstate = genpd_dev_get_current_performance_state(dev, base_dev, index,
>> +                                                        &state_default,
>> +                                                        &suspended);
>> +       if (pstate <= 0)
>> +               return pstate;
>> +
>> +       /*
>> +        * If device is suspended, then performance state will be applied
>> +        * on RPM-resume. This prevents potential extra power consumption
>> +        * if device won't be resumed soon.
>> +        *
>> +        * If device is known to be active at the moment, then the performance
>> +        * state should be updated immediately to sync state with hardware.
>> +        */
>> +       if (suspended) {
>> +               dev_gpd_data(dev)->rpm_pstate = pstate;
>> +       } else {
>> +               err = dev_pm_genpd_set_performance_state(dev, pstate);
>> +               if (err) {
>> +                       dev_err(dev, "failed to set performance state for power-domain %s: %d\n",
>> +                               pd->name, err);
>> +                       return err;
>> +               }
>> +
>> +               if (state_default)
>> +                       dev_gpd_data(dev)->default_pstate = pstate;
>> +       }
>> +
>> +       return 0;
>> +}
> As I kind of indicated in my previous reply on the earlier version, I
> think the above code can be made a lot less complicated. Although,
> perhaps I may be missing some points.
> 
> Anyway, I will post a couple patches, later this evening or tomorrow,
> to show more exactly what I had in mind. Let's see if that can work
> for you.

It's not obvious what you're wanting to improve, I'm probably missing
yours point. So indeed will be good to see the code sample, thanks.

[PATCH v10 5/8] soc/tegra: pmc: Implement dev_get_performance_state() callback

[Dmitry Osipenko]

Implement dev_get_performance_state() callback of the power domains to
pre-initialize theirs performance (voltage) state in accordance to the
clock rate of attached devices.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/soc/tegra/pmc.c | 101 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 101 insertions(+)

diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
index 50091c4ec948..98015a6cdd1d 100644
--- a/drivers/soc/tegra/pmc.c
+++ b/drivers/soc/tegra/pmc.c
@@ -39,6 +39,7 @@
 #include <linux/platform_device.h>
 #include <linux/pm_domain.h>
 #include <linux/pm_opp.h>
+#include <linux/pm_runtime.h>
 #include <linux/reboot.h>
 #include <linux/regmap.h>
 #include <linux/reset.h>
@@ -505,6 +506,104 @@ static void tegra_pmc_scratch_writel(struct tegra_pmc *pmc, u32 value,
 		writel(value, pmc->scratch + offset);
 }
 
+static const char * const tegra_pd_no_perf_compats[] = {
+	"nvidia,tegra20-sclk",
+	"nvidia,tegra30-sclk",
+	"nvidia,tegra30-pllc",
+	"nvidia,tegra30-plle",
+	"nvidia,tegra30-pllm",
+	"nvidia,tegra20-dc",
+	"nvidia,tegra30-dc",
+	"nvidia,tegra20-emc",
+	"nvidia,tegra30-emc",
+	NULL,
+};
+
+static int
+tegra_pmc_pd_dev_get_performance_state(struct generic_pm_domain *genpd,
+				       struct device *dev)
+{
+	struct opp_table *hw_opp_table, *clk_opp_table;
+	struct dev_pm_opp *opp;
+	u32 hw_version;
+	int ret;
+
+	/*
+	 * The EMC devices are a special case because we have a protection
+	 * from non-EMC drivers getting clock handle before EMC driver is
+	 * fully initialized.  The goal of the protection is to prevent
+	 * devfreq driver from getting failures if it will try to change
+	 * EMC clock rate until clock is fully initialized.  The EMC drivers
+	 * will initialize the performance state by themselves.
+	 *
+	 * Display controller also is a special case because only controller
+	 * driver could get the clock rate based on configuration of internal
+	 * divider.
+	 *
+	 * Clock driver uses its own state syncing.
+	 */
+	if (of_device_compatible_match(dev->of_node, tegra_pd_no_perf_compats))
+		return 0;
+
+	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);
+
+	hw_opp_table = dev_pm_opp_set_supported_hw(dev, &hw_version, 1);
+	if (IS_ERR(hw_opp_table)) {
+		dev_err(dev, "failed to set OPP supported HW: %pe\n",
+			hw_opp_table);
+		return PTR_ERR(hw_opp_table);
+	}
+
+	/*
+	 * Older device-trees don't have OPPs, meanwhile drivers use
+	 * dev_pm_opp_set_rate() and it requires OPP table to be set
+	 * up using dev_pm_opp_set_clkname().
+	 *
+	 * The devm_tegra_core_dev_init_opp_table() is a common helper
+	 * that sets up OPP table for Tegra drivers and it sets the clk
+	 * for compatibility with older device-tress.  GR3D driver uses that
+	 * helper, it also uses devm_pm_opp_attach_genpd() to manually attach
+	 * power domains, which now holds the reference to OPP table that
+	 * already has clk set up implicitly by OPP core for a newly created
+	 * table using dev_pm_opp_of_add_table() invoked below.
+	 *
+	 * Hence we need to explicitly set/put the clk, otherwise
+	 * further dev_pm_opp_set_clkname() will fail with -EBUSY.
+	 */
+	clk_opp_table = dev_pm_opp_set_clkname(dev, NULL);
+	if (IS_ERR(clk_opp_table)) {
+		dev_err(dev, "failed to set OPP clk: %pe\n", clk_opp_table);
+		ret = PTR_ERR(clk_opp_table);
+		goto put_hw;
+	}
+
+	ret = dev_pm_opp_of_add_table(dev);
+	if (ret) {
+		dev_err(dev, "failed to add OPP table: %d\n", ret);
+		goto put_clk;
+	}
+
+	opp = dev_pm_opp_get_current(dev);
+	if (IS_ERR(opp)) {
+		dev_err(dev, "failed to get current OPP: %pe\n", opp);
+		ret = PTR_ERR(opp);
+	} else {
+		ret = dev_pm_opp_get_required_pstate(opp, 0);
+		dev_pm_opp_put(opp);
+	}
+
+	dev_pm_opp_of_remove_table(dev);
+put_clk:
+	dev_pm_opp_put_clkname(clk_opp_table);
+put_hw:
+	dev_pm_opp_put_supported_hw(hw_opp_table);
+
+	return ret;
+}
+
 /*
  * TODO Figure out a way to call this with the struct tegra_pmc * passed in.
  * This currently doesn't work because readx_poll_timeout() can only operate
@@ -1237,6 +1336,7 @@ static int tegra_powergate_add(struct tegra_pmc *pmc, struct device_node *np)
 
 	pg->id = id;
 	pg->genpd.name = np->name;
+	pg->genpd.dev_get_performance_state = tegra_pmc_pd_dev_get_performance_state;
 	pg->genpd.power_off = tegra_genpd_power_off;
 	pg->genpd.power_on = tegra_genpd_power_on;
 	pg->pmc = pmc;
@@ -1355,6 +1455,7 @@ static int tegra_pmc_core_pd_add(struct tegra_pmc *pmc, struct device_node *np)
 	genpd->name = np->name;
 	genpd->set_performance_state = tegra_pmc_core_pd_set_performance_state;
 	genpd->opp_to_performance_state = tegra_pmc_core_pd_opp_to_performance_state;
+	genpd->dev_get_performance_state = tegra_pmc_pd_dev_get_performance_state;
 
 	err = devm_pm_opp_set_regulators(pmc->dev, &rname, 1);
 	if (err)
-- 
2.32.0

Re: [PATCH v10 5/8] soc/tegra: pmc: Implement dev_get_performance_state() callback

[Viresh Kumar]

On 31-08-21, 16:54, Dmitry Osipenko wrote:
> +static int
> +tegra_pmc_pd_dev_get_performance_state(struct generic_pm_domain *genpd,
> +				       struct device *dev)
> +{
> +	struct opp_table *hw_opp_table, *clk_opp_table;
> +	struct dev_pm_opp *opp;
> +	u32 hw_version;
> +	int ret;
> +
> +	/*
> +	 * The EMC devices are a special case because we have a protection
> +	 * from non-EMC drivers getting clock handle before EMC driver is
> +	 * fully initialized.  The goal of the protection is to prevent
> +	 * devfreq driver from getting failures if it will try to change
> +	 * EMC clock rate until clock is fully initialized.  The EMC drivers
> +	 * will initialize the performance state by themselves.
> +	 *
> +	 * Display controller also is a special case because only controller
> +	 * driver could get the clock rate based on configuration of internal
> +	 * divider.
> +	 *
> +	 * Clock driver uses its own state syncing.
> +	 */
> +	if (of_device_compatible_match(dev->of_node, tegra_pd_no_perf_compats))
> +		return 0;
> +
> +	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);
> +
> +	hw_opp_table = dev_pm_opp_set_supported_hw(dev, &hw_version, 1);
> +	if (IS_ERR(hw_opp_table)) {
> +		dev_err(dev, "failed to set OPP supported HW: %pe\n",
> +			hw_opp_table);
> +		return PTR_ERR(hw_opp_table);
> +	}
> +
> +	/*
> +	 * Older device-trees don't have OPPs, meanwhile drivers use
> +	 * dev_pm_opp_set_rate() and it requires OPP table to be set
> +	 * up using dev_pm_opp_set_clkname().
> +	 *
> +	 * The devm_tegra_core_dev_init_opp_table() is a common helper
> +	 * that sets up OPP table for Tegra drivers and it sets the clk
> +	 * for compatibility with older device-tress.  GR3D driver uses that
> +	 * helper, it also uses devm_pm_opp_attach_genpd() to manually attach
> +	 * power domains, which now holds the reference to OPP table that
> +	 * already has clk set up implicitly by OPP core for a newly created
> +	 * table using dev_pm_opp_of_add_table() invoked below.
> +	 *
> +	 * Hence we need to explicitly set/put the clk, otherwise
> +	 * further dev_pm_opp_set_clkname() will fail with -EBUSY.
> +	 */
> +	clk_opp_table = dev_pm_opp_set_clkname(dev, NULL);
> +	if (IS_ERR(clk_opp_table)) {
> +		dev_err(dev, "failed to set OPP clk: %pe\n", clk_opp_table);
> +		ret = PTR_ERR(clk_opp_table);
> +		goto put_hw;
> +	}
> +
> +	ret = dev_pm_opp_of_add_table(dev);
> +	if (ret) {
> +		dev_err(dev, "failed to add OPP table: %d\n", ret);
> +		goto put_clk;
> +	}
> +
> +	opp = dev_pm_opp_get_current(dev);
> +	if (IS_ERR(opp)) {
> +		dev_err(dev, "failed to get current OPP: %pe\n", opp);
> +		ret = PTR_ERR(opp);
> +	} else {
> +		ret = dev_pm_opp_get_required_pstate(opp, 0);
> +		dev_pm_opp_put(opp);
> +	}
> +
> +	dev_pm_opp_of_remove_table(dev);
> +put_clk:
> +	dev_pm_opp_put_clkname(clk_opp_table);
> +put_hw:
> +	dev_pm_opp_put_supported_hw(hw_opp_table);

So you create the OPP table and just after that you remove it ? Just
to get the current OPP and pstate ? This doesn't look okay.

Moreover, this routine must be implemented with the expectation that
the genpd core can call it anytime it wants, not just at the
beginning. And so if the table is already setup by someone else then,
then this all will just fail.

I did have a look at the comment you added above regarding
devm_tegra_core_dev_init_opp_table(), but I am not sure of the series
of events right now. For me, the OPP table should just be added once
and for ever. You shouldn't remove and add it again. That is bound to
break somewhere later.

Can you give the sequence in which the whole thing works out with
respect to the OPP core? who calls
devm_tegra_core_dev_init_opp_table() and when, and when exactly will
this routine get called, etc ?

-- 
viresh

Re: [PATCH v10 5/8] soc/tegra: pmc: Implement dev_get_performance_state() callback

[Dmitry Osipenko]

01.09.2021 09:10, Viresh Kumar пишет:
> On 31-08-21, 16:54, Dmitry Osipenko wrote:
>> +static int
>> +tegra_pmc_pd_dev_get_performance_state(struct generic_pm_domain *genpd,
>> +				       struct device *dev)
>> +{
>> +	struct opp_table *hw_opp_table, *clk_opp_table;
>> +	struct dev_pm_opp *opp;
>> +	u32 hw_version;
>> +	int ret;
>> +
>> +	/*
>> +	 * The EMC devices are a special case because we have a protection
>> +	 * from non-EMC drivers getting clock handle before EMC driver is
>> +	 * fully initialized.  The goal of the protection is to prevent
>> +	 * devfreq driver from getting failures if it will try to change
>> +	 * EMC clock rate until clock is fully initialized.  The EMC drivers
>> +	 * will initialize the performance state by themselves.
>> +	 *
>> +	 * Display controller also is a special case because only controller
>> +	 * driver could get the clock rate based on configuration of internal
>> +	 * divider.
>> +	 *
>> +	 * Clock driver uses its own state syncing.
>> +	 */
>> +	if (of_device_compatible_match(dev->of_node, tegra_pd_no_perf_compats))
>> +		return 0;
>> +
>> +	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);
>> +
>> +	hw_opp_table = dev_pm_opp_set_supported_hw(dev, &hw_version, 1);
>> +	if (IS_ERR(hw_opp_table)) {
>> +		dev_err(dev, "failed to set OPP supported HW: %pe\n",
>> +			hw_opp_table);
>> +		return PTR_ERR(hw_opp_table);
>> +	}
>> +
>> +	/*
>> +	 * Older device-trees don't have OPPs, meanwhile drivers use
>> +	 * dev_pm_opp_set_rate() and it requires OPP table to be set
>> +	 * up using dev_pm_opp_set_clkname().
>> +	 *
>> +	 * The devm_tegra_core_dev_init_opp_table() is a common helper
>> +	 * that sets up OPP table for Tegra drivers and it sets the clk
>> +	 * for compatibility with older device-tress.  GR3D driver uses that
>> +	 * helper, it also uses devm_pm_opp_attach_genpd() to manually attach
>> +	 * power domains, which now holds the reference to OPP table that
>> +	 * already has clk set up implicitly by OPP core for a newly created
>> +	 * table using dev_pm_opp_of_add_table() invoked below.
>> +	 *
>> +	 * Hence we need to explicitly set/put the clk, otherwise
>> +	 * further dev_pm_opp_set_clkname() will fail with -EBUSY.
>> +	 */
>> +	clk_opp_table = dev_pm_opp_set_clkname(dev, NULL);
>> +	if (IS_ERR(clk_opp_table)) {
>> +		dev_err(dev, "failed to set OPP clk: %pe\n", clk_opp_table);
>> +		ret = PTR_ERR(clk_opp_table);
>> +		goto put_hw;
>> +	}
>> +
>> +	ret = dev_pm_opp_of_add_table(dev);
>> +	if (ret) {
>> +		dev_err(dev, "failed to add OPP table: %d\n", ret);
>> +		goto put_clk;
>> +	}
>> +
>> +	opp = dev_pm_opp_get_current(dev);
>> +	if (IS_ERR(opp)) {
>> +		dev_err(dev, "failed to get current OPP: %pe\n", opp);
>> +		ret = PTR_ERR(opp);
>> +	} else {
>> +		ret = dev_pm_opp_get_required_pstate(opp, 0);
>> +		dev_pm_opp_put(opp);
>> +	}
>> +
>> +	dev_pm_opp_of_remove_table(dev);
>> +put_clk:
>> +	dev_pm_opp_put_clkname(clk_opp_table);
>> +put_hw:
>> +	dev_pm_opp_put_supported_hw(hw_opp_table);
> 
> So you create the OPP table and just after that you remove it ? Just
> to get the current OPP and pstate ? This doesn't look okay.
> 
> Moreover, this routine must be implemented with the expectation that
> the genpd core can call it anytime it wants, not just at the
> beginning. And so if the table is already setup by someone else then,
> then this all will just fail.

This is not doable using the current OPP API, it doesn't allow to re-use initialized OPP table. The current limitation is okay because genpd core doesn't call routine anytime.

> I did have a look at the comment you added above regarding
> devm_tegra_core_dev_init_opp_table(), but I am not sure of the series
> of events right now. For me, the OPP table should just be added once
> and for ever. You shouldn't remove and add it again. That is bound to
> break somewhere later.

I see that the comment about devm_tegra_core_dev_init_opp_table() is outdated now, will update it. There was a refcounting bug in v9 where I accidentally used devm_pm_opp_of_add_table instead of dev_, still it fails similarly, but in a different place now. 

> Can you give the sequence in which the whole thing works out with
> respect to the OPP core? who calls
> devm_tegra_core_dev_init_opp_table() and when, and when exactly will
> this routine get called, etc ?
> 

gr3d_probe(struct platform_device *pdev)
{
	gr3d_init_power(dev)
	{
		static const char * const opp_genpd_names[] = { "3d0", "3d1", NULL };

		devm_pm_opp_attach_genpd(dev, opp_genpd_names)
		{
			dev_pm_opp_attach_genpd(dev, names, virt_devs)
			{
				// takes and holds table reference
				opp_table = _add_opp_table(dev, false);

				while (*name) {
					// first attachment succeeds, 
					// second fails with "tegra-gr3d 54180000.gr3d: failed to set OPP clk: -EBUSY"
					dev_pm_domain_attach_by_name(dev, *name)
					{
						tegra_pmc_pd_dev_get_performance_state(dev)
						{
							dev_pm_opp_set_clkname(dev, NULL);
							dev_pm_opp_of_add_table(dev);
							opp = dev_pm_opp_get_current(dev);
							dev_pm_opp_of_remove_table(dev);
							dev_pm_opp_put_clkname(opp_table);
							...
						}
						// opp_table->clk = ERR_PTR(-EINVAL) after the first attachment
					}
				}
			}
		}
	}

	devm_tegra_core_dev_init_opp_table_simple(&pdev->dev);

	return 0;
}


WARNING: CPU: 3 PID: 7 at drivers/opp/core.c:2151 dev_pm_opp_set_clkname+0x97/0xb8
Modules linked in:
CPU: 3 PID: 7 Comm: kworker/u8:0 Tainted: G        W         5.14.0-next-20210831-00177-g6850c69f8c92-dirty #9371
Hardware name: NVIDIA Tegra SoC (Flattened Device Tree)
Workqueue: events_unbound deferred_probe_work_func
[<c010cc91>] (unwind_backtrace) from [<c0108d35>] (show_stack+0x11/0x14)
[<c0108d35>] (show_stack) from [<c0a6e25d>] (dump_stack_lvl+0x2b/0x34)
[<c0a6e25d>] (dump_stack_lvl) from [<c011fc7f>] (__warn+0xbb/0x100)
[<c011fc7f>] (__warn) from [<c0a6b783>] (warn_slowpath_fmt+0x4b/0x80)
[<c0a6b783>] (warn_slowpath_fmt) from [<c0742613>] (dev_pm_opp_set_clkname+0x97/0xb8)
[<c0742613>] (dev_pm_opp_set_clkname) from [<c0509815>] (tegra_pmc_pd_dev_get_performance_state+0x85/0x120)
[<c0509815>] (tegra_pmc_pd_dev_get_performance_state) from [<c05cd3db>] (__genpd_dev_pm_attach+0xe7/0x218)
[<c05cd3db>] (__genpd_dev_pm_attach) from [<c05cd5d3>] (genpd_dev_pm_attach_by_id+0x8b/0xec)
[<c05cd5d3>] (genpd_dev_pm_attach_by_id) from [<c074287f>] (dev_pm_opp_attach_genpd+0x97/0x11c)
[<c074287f>] (dev_pm_opp_attach_genpd) from [<c0742913>] (devm_pm_opp_attach_genpd+0xf/0x6c)
[<c0742913>] (devm_pm_opp_attach_genpd) from [<c05ac7a5>] (gr3d_probe+0x245/0x348)
[<c05ac7a5>] (gr3d_probe) from [<c05bc003>] (platform_probe+0x43/0x84)
[<c05bc003>] (platform_probe) from [<c05ba569>] (really_probe.part.0+0x69/0x200)
[<c05ba569>] (really_probe.part.0) from [<c05ba773>] (__driver_probe_device+0x73/0xd4)
[<c05ba773>] (__driver_probe_device) from [<c05ba809>] (driver_probe_device+0x35/0xd0)
[<c05ba809>] (driver_probe_device) from [<c05bac11>] (__device_attach_driver+0x75/0x98)
[<c05bac11>] (__device_attach_driver) from [<c05b9005>] (bus_for_each_drv+0x51/0x7c)
[<c05b9005>] (bus_for_each_drv) from [<c05ba9f7>] (__device_attach+0x8b/0x104)
[<c05ba9f7>] (__device_attach) from [<c05b9b1b>] (bus_probe_device+0x5b/0x60)
[<c05b9b1b>] (bus_probe_device) from [<c05b7707>] (device_add+0x293/0x65c)
[<c05b7707>] (device_add) from [<c07798b7>] (of_platform_device_create_pdata+0x63/0x88)
[<c07798b7>] (of_platform_device_create_pdata) from [<c07799e5>] (of_platform_bus_create+0xfd/0x26c)
[<c07799e5>] (of_platform_bus_create) from [<c0779c2d>] (of_platform_populate+0x45/0x84)
[<c0779c2d>] (of_platform_populate) from [<c0779cc5>] (devm_of_platform_populate+0x41/0x6c)
[<c0779cc5>] (devm_of_platform_populate) from [<c054aa65>] (host1x_probe+0x1e9/0x2c8)
[<c054aa65>] (host1x_probe) from [<c05bc003>] (platform_probe+0x43/0x84)
[<c05bc003>] (platform_probe) from [<c05ba569>] (really_probe.part.0+0x69/0x200)
[<c05ba569>] (really_probe.part.0) from [<c05ba773>] (__driver_probe_device+0x73/0xd4)
[<c05ba773>] (__driver_probe_device) from [<c05ba809>] (driver_probe_device+0x35/0xd0)
[<c05ba809>] (driver_probe_device) from [<c05bac11>] (__device_attach_driver+0x75/0x98)
[<c05bac11>] (__device_attach_driver) from [<c05b9005>] (bus_for_each_drv+0x51/0x7c)
[<c05b9005>] (bus_for_each_drv) from [<c05ba9f7>] (__device_attach+0x8b/0x104)
[<c05ba9f7>] (__device_attach) from [<c05b9b1b>] (bus_probe_device+0x5b/0x60)
[<c05b9b1b>] (bus_probe_device) from [<c05b9dfb>] (deferred_probe_work_func+0x57/0x78)
[<c05b9dfb>] (deferred_probe_work_func) from [<c013701f>] (process_one_work+0x147/0x3f8)
[<c013701f>] (process_one_work) from [<c0137805>] (worker_thread+0x21d/0x3f4)
[<c0137805>] (worker_thread) from [<c013c1bb>] (kthread+0x123/0x140)
[<c013c1bb>] (kthread) from [<c0100135>] (ret_from_fork+0x11/0x1c)
Exception stack(0xc1567fb0 to 0xc1567ff8)
---[ end trace f68728a0d3053b54 ]---
tegra-gr3d 54180000.gr3d: failed to set OPP clk: -EBUSY
genpd genpd:1:54180000.gr3d: failed to get performance state of 54180000.gr3d for power-domain 3d1: -16
tegra-gr3d 54180000.gr3d: Couldn't attach to pm_domain: -16

Re: [PATCH v10 5/8] soc/tegra: pmc: Implement dev_get_performance_state() callback

[Viresh Kumar]

On 01-09-21, 09:57, Dmitry Osipenko wrote:
> 01.09.2021 09:10, Viresh Kumar пишет:
> > So you create the OPP table and just after that you remove it ? Just
> > to get the current OPP and pstate ? This doesn't look okay.
> > 
> > Moreover, this routine must be implemented with the expectation that
> > the genpd core can call it anytime it wants, not just at the
> > beginning. And so if the table is already setup by someone else then,
> > then this all will just fail.
> 
> This is not doable using the current OPP API, it doesn't allow to
> re-use initialized OPP table.

That isn't correct, you can call dev_pm_opp_of_add_table() as many
times as you want. It will just increase the refcount and return the
next time.

Yes, you can call the APIs like set-clk-name or supported-hw, since
they are supposed to be set only once.

> The current limitation is okay because genpd core doesn't call
> routine anytime.

Yeah, but is broken by design. People can make changes to genpd core
later on to call it as many times and they aren't required to have a
look at all the users to see who abused the API and who didn't.

> > Can you give the sequence in which the whole thing works out with
> > respect to the OPP core? who calls
> > devm_tegra_core_dev_init_opp_table() and when, and when exactly will
> > this routine get called, etc ?
> > 
> 
> gr3d_probe(struct platform_device *pdev)

Thanks for this.

> {
> 	gr3d_init_power(dev)
> 	{
> 		static const char * const opp_genpd_names[] = { "3d0", "3d1", NULL };
> 
> 		devm_pm_opp_attach_genpd(dev, opp_genpd_names)
> 		{
> 			dev_pm_opp_attach_genpd(dev, names, virt_devs)
> 			{
> 				// takes and holds table reference
> 				opp_table = _add_opp_table(dev, false);
> 
> 				while (*name) {
> 					// first attachment succeeds, 
> 					// second fails with "tegra-gr3d 54180000.gr3d: failed to set OPP clk: -EBUSY"
> 					dev_pm_domain_attach_by_name(dev, *name)
> 					{
> 						tegra_pmc_pd_dev_get_performance_state(dev)
> 						{
> 							dev_pm_opp_set_clkname(dev, NULL);
> 							dev_pm_opp_of_add_table(dev);

What you end up doing here is pretty much like
devm_tegra_core_dev_init_opp_table_simple(), right ?

> 							opp = dev_pm_opp_get_current(dev);
> 							dev_pm_opp_of_remove_table(dev);
> 							dev_pm_opp_put_clkname(opp_table);

You shouldn't be required to do this at all.

> 							...
> 						}
> 						// opp_table->clk = ERR_PTR(-EINVAL) after the first attachment
> 					}
> 				}
> 			}
> 		}
> 	}
> 
> 	devm_tegra_core_dev_init_opp_table_simple(&pdev->dev);

Can we make the call at the beginning ? before calling
gr3d_init_power() ? I mean you should only be required to initialize
the OPP table just once.

If not, then what about calling
devm_tegra_core_dev_init_opp_table_simple() from here and from
tegra_pmc_pd_dev_get_performance_state() as well ?

And update devm_tegra_core_dev_init_opp_table_simple() to call
dev_pm_opp_get_opp_table() first and return early if the OPP table is
already initialized ?

-- 
viresh

Re: [PATCH v10 5/8] soc/tegra: pmc: Implement dev_get_performance_state() callback

[Dmitry Osipenko]

01.09.2021 10:16, Viresh Kumar пишет:
> On 01-09-21, 09:57, Dmitry Osipenko wrote:
>> 01.09.2021 09:10, Viresh Kumar пишет:
>>> So you create the OPP table and just after that you remove it ? Just
>>> to get the current OPP and pstate ? This doesn't look okay.
>>>
>>> Moreover, this routine must be implemented with the expectation that
>>> the genpd core can call it anytime it wants, not just at the
>>> beginning. And so if the table is already setup by someone else then,
>>> then this all will just fail.
>>
>> This is not doable using the current OPP API, it doesn't allow to
>> re-use initialized OPP table.
> 
> That isn't correct, you can call dev_pm_opp_of_add_table() as many
> times as you want. It will just increase the refcount and return the
> next time.
> 
> Yes, you can call the APIs like set-clk-name or supported-hw, since
> they are supposed to be set only once.
> 
>> The current limitation is okay because genpd core doesn't call
>> routine anytime.
> 
> Yeah, but is broken by design. People can make changes to genpd core
> later on to call it as many times and they aren't required to have a
> look at all the users to see who abused the API and who didn't.
> 
>>> Can you give the sequence in which the whole thing works out with
>>> respect to the OPP core? who calls
>>> devm_tegra_core_dev_init_opp_table() and when, and when exactly will
>>> this routine get called, etc ?
>>>
>>
>> gr3d_probe(struct platform_device *pdev)
> 
> Thanks for this.
> 
>> {
>> 	gr3d_init_power(dev)
>> 	{
>> 		static const char * const opp_genpd_names[] = { "3d0", "3d1", NULL };
>>
>> 		devm_pm_opp_attach_genpd(dev, opp_genpd_names)
>> 		{
>> 			dev_pm_opp_attach_genpd(dev, names, virt_devs)
>> 			{
>> 				// takes and holds table reference
>> 				opp_table = _add_opp_table(dev, false);
>>
>> 				while (*name) {
>> 					// first attachment succeeds, 
>> 					// second fails with "tegra-gr3d 54180000.gr3d: failed to set OPP clk: -EBUSY"
>> 					dev_pm_domain_attach_by_name(dev, *name)
>> 					{
>> 						tegra_pmc_pd_dev_get_performance_state(dev)
>> 						{
>> 							dev_pm_opp_set_clkname(dev, NULL);
>> 							dev_pm_opp_of_add_table(dev);
> 
> What you end up doing here is pretty much like
> devm_tegra_core_dev_init_opp_table_simple(), right ?

Yes

>> 							opp = dev_pm_opp_get_current(dev);
>> 							dev_pm_opp_of_remove_table(dev);
>> 							dev_pm_opp_put_clkname(opp_table);
> 
> You shouldn't be required to do this at all.
> 
>> 							...
>> 						}
>> 						// opp_table->clk = ERR_PTR(-EINVAL) after the first attachment
>> 					}
>> 				}
>> 			}
>> 		}
>> 	}
>>
>> 	devm_tegra_core_dev_init_opp_table_simple(&pdev->dev);
> 
> Can we make the call at the beginning ? before calling
> gr3d_init_power() ? I mean you should only be required to initialize
> the OPP table just once.

This breaks dev_pm_opp_set_supported_hw() which isn't allowed to be
called for a populated OPP table, set/put_hw also isn't refcounted.

> If not, then what about calling
> devm_tegra_core_dev_init_opp_table_simple() from here and from
> tegra_pmc_pd_dev_get_performance_state() as well ?
> 
> And update devm_tegra_core_dev_init_opp_table_simple() to call
> dev_pm_opp_get_opp_table() first and return early if the OPP table is
> already initialized ?
> 

This doesn't work for devm_pm_opp_register_set_opp_helper() that is used
by gr3d_probe() because set_opp_helper() should be invoked only before
table is populated and it's already populated for the case of a
single-domain h/w since domain is attached before driver is probed.

But it's a good idea to use dev_pm_opp_get_opp_table(). I got it to work
by adding dev_pm_opp_get_opp_table() to
tegra_pmc_pd_dev_get_performance_state() and moving
devm_tegra_core_dev_init_opp_table_simple() before gr3d_init_power().

Thank you for the suggestion, I'll change it in v11.

[PATCH v10 6/8] soc/tegra: Add devm_tegra_core_dev_init_opp_table_simple()

[Dmitry Osipenko]

Only couple drivers need to get the -ENODEV error code and explicitly
initialize the performance state. Add new helper that allows to avoid
the extra boilerplate code in majority of drivers.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 include/soc/tegra/common.h | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/include/soc/tegra/common.h b/include/soc/tegra/common.h
index af41ad80ec21..265ad90e45a2 100644
--- a/include/soc/tegra/common.h
+++ b/include/soc/tegra/common.h
@@ -39,4 +39,17 @@ devm_tegra_core_dev_init_opp_table(struct device *dev,
 }
 #endif
 
+static inline int
+devm_tegra_core_dev_init_opp_table_simple(struct device *dev)
+{
+	struct tegra_core_opp_params params = {};
+	int err;
+
+	err = devm_tegra_core_dev_init_opp_table(dev, &params);
+	if (err != -ENODEV)
+		return err;
+
+	return 0;
+}
+
 #endif /* __SOC_TEGRA_COMMON_H__ */
-- 
2.32.0

[PATCH v10 7/8] gpu: host1x: Add host1x_channel_stop()

[Dmitry Osipenko]

Add host1x_channel_stop() which waits till channel becomes idle and then
stops the channel hardware. This is needed for supporting suspend/resume
by host1x drivers since the hardware state is lost after power-gating,
thus the channel needs to be stopped before client enters into suspend.

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/gpu/host1x/channel.c | 8 ++++++++
 include/linux/host1x.h       | 1 +
 2 files changed, 9 insertions(+)

diff --git a/drivers/gpu/host1x/channel.c b/drivers/gpu/host1x/channel.c
index 4cd212bb570d..2a9a3a8d5931 100644
--- a/drivers/gpu/host1x/channel.c
+++ b/drivers/gpu/host1x/channel.c
@@ -75,6 +75,14 @@ struct host1x_channel *host1x_channel_get_index(struct host1x *host,
 	return ch;
 }
 
+void host1x_channel_stop(struct host1x_channel *channel)
+{
+	struct host1x *host = dev_get_drvdata(channel->dev->parent);
+
+	host1x_hw_cdma_stop(host, &channel->cdma);
+}
+EXPORT_SYMBOL(host1x_channel_stop);
+
 static void release_channel(struct kref *kref)
 {
 	struct host1x_channel *channel =
diff --git a/include/linux/host1x.h b/include/linux/host1x.h
index 7bccf589aba7..66473b5be0af 100644
--- a/include/linux/host1x.h
+++ b/include/linux/host1x.h
@@ -181,6 +181,7 @@ struct host1x_job;
 
 struct host1x_channel *host1x_channel_request(struct host1x_client *client);
 struct host1x_channel *host1x_channel_get(struct host1x_channel *channel);
+void host1x_channel_stop(struct host1x_channel *channel);
 void host1x_channel_put(struct host1x_channel *channel);
 int host1x_job_submit(struct host1x_job *job);
 
-- 
2.32.0

[PATCH v10 8/8] drm/tegra: gr3d: Support generic power domain and runtime PM

[Dmitry Osipenko]

Add power management and support generic power domains.

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/gpu/drm/tegra/gr3d.c | 384 ++++++++++++++++++++++++++++++-----
 1 file changed, 330 insertions(+), 54 deletions(-)

diff --git a/drivers/gpu/drm/tegra/gr3d.c b/drivers/gpu/drm/tegra/gr3d.c
index 24442ade0da3..545eb4005a96 100644
--- a/drivers/gpu/drm/tegra/gr3d.c
+++ b/drivers/gpu/drm/tegra/gr3d.c
@@ -5,32 +5,47 @@
  */
 
 #include <linux/clk.h>
+#include <linux/delay.h>
 #include <linux/host1x.h>
 #include <linux/iommu.h>
 #include <linux/module.h>
 #include <linux/of_device.h>
 #include <linux/platform_device.h>
+#include <linux/pm_domain.h>
+#include <linux/pm_opp.h>
+#include <linux/pm_runtime.h>
 #include <linux/reset.h>
 
+#include <soc/tegra/common.h>
 #include <soc/tegra/pmc.h>
 
 #include "drm.h"
 #include "gem.h"
 #include "gr3d.h"
 
+enum {
+	RST_MC,
+	RST_GR3D,
+	RST_MC2,
+	RST_GR3D2,
+	RST_GR3D_MAX,
+};
+
 struct gr3d_soc {
 	unsigned int version;
+	unsigned int num_clocks;
+	unsigned int num_resets;
 };
 
 struct gr3d {
 	struct tegra_drm_client client;
 	struct host1x_channel *channel;
-	struct clk *clk_secondary;
-	struct clk *clk;
-	struct reset_control *rst_secondary;
-	struct reset_control *rst;
 
 	const struct gr3d_soc *soc;
+	struct clk_bulk_data *clocks;
+	unsigned int nclocks;
+	struct reset_control_bulk_data resets[RST_GR3D_MAX];
+	unsigned int nresets;
 
 	DECLARE_BITMAP(addr_regs, GR3D_NUM_REGS);
 };
@@ -109,16 +124,24 @@ static int gr3d_open_channel(struct tegra_drm_client *client,
 			     struct tegra_drm_context *context)
 {
 	struct gr3d *gr3d = to_gr3d(client);
+	int err;
 
 	context->channel = host1x_channel_get(gr3d->channel);
 	if (!context->channel)
 		return -ENOMEM;
 
+	err = pm_runtime_resume_and_get(client->base.dev);
+	if (err) {
+		host1x_channel_put(context->channel);
+		return err;
+	}
+
 	return 0;
 }
 
 static void gr3d_close_channel(struct tegra_drm_context *context)
 {
+	pm_runtime_put_sync(context->client->base.dev);
 	host1x_channel_put(context->channel);
 }
 
@@ -155,14 +178,20 @@ static const struct tegra_drm_client_ops gr3d_ops = {
 
 static const struct gr3d_soc tegra20_gr3d_soc = {
 	.version = 0x20,
+	.num_clocks = 1,
+	.num_resets = 2,
 };
 
 static const struct gr3d_soc tegra30_gr3d_soc = {
 	.version = 0x30,
+	.num_clocks = 2,
+	.num_resets = 4,
 };
 
 static const struct gr3d_soc tegra114_gr3d_soc = {
 	.version = 0x35,
+	.num_clocks = 1,
+	.num_resets = 2,
 };
 
 static const struct of_device_id tegra_gr3d_match[] = {
@@ -278,9 +307,211 @@ static const u32 gr3d_addr_regs[] = {
 	GR3D_GLOBAL_SAMP23SURFADDR(15),
 };
 
+static int gr3d_power_up_legacy_domain(struct device *dev, const char *name,
+				       unsigned int id)
+{
+	struct gr3d *gr3d = dev_get_drvdata(dev);
+	struct reset_control *reset;
+	struct clk *clk;
+	unsigned int i;
+	int err;
+
+	/*
+	 * Tegra20 device-tree doesn't specify 3d clock name and there is only
+	 * one clock for Tegra20. Tegra30+ device-trees always specified names
+	 * for the clocks.
+	 */
+	if (gr3d->nclocks == 1) {
+		if (id == TEGRA_POWERGATE_3D1)
+			return 0;
+
+		clk = gr3d->clocks[0].clk;
+	} else {
+		for (i = 0; i < gr3d->nclocks; i++) {
+			if (WARN_ON(!gr3d->clocks[i].id))
+				continue;
+
+			if (!strcmp(gr3d->clocks[i].id, name)) {
+				clk = gr3d->clocks[i].clk;
+				break;
+			}
+		}
+
+		if (WARN_ON(i == gr3d->nclocks))
+			return -EINVAL;
+	}
+
+	/*
+	 * We use array of resets, which includes MC resets, and MC
+	 * reset shouldn't be asserted while hardware is gated because
+	 * MC flushing will fail for gated hardware. Hence for legacy
+	 * PD we request the individual reset separately.
+	 */
+	reset = reset_control_get_exclusive_released(dev, name);
+	if (IS_ERR(reset))
+		return PTR_ERR(reset);
+
+	err = reset_control_acquire(reset);
+	if (err) {
+		dev_err(dev, "failed to acquire %s reset: %d\n", name, err);
+	} else {
+		err = tegra_powergate_sequence_power_up(id, clk, reset);
+		reset_control_release(reset);
+	}
+
+	reset_control_put(reset);
+	if (err)
+		return err;
+
+	/*
+	 * tegra_powergate_sequence_power_up() leaves clocks enabled
+	 * while GENPD not, hence keep clock-enable balanced.
+	 */
+	clk_disable_unprepare(clk);
+
+	return 0;
+}
+
+static void gr3d_del_link(void *link)
+{
+	device_link_del(link);
+}
+
+static int gr3d_init_power(struct device *dev, struct gr3d *gr3d)
+{
+	static const char * const opp_genpd_names[] = { "3d0", "3d1", NULL };
+	const u32 link_flags = DL_FLAG_STATELESS | DL_FLAG_PM_RUNTIME;
+	struct device **opp_virt_devs, *pd_dev;
+	struct device_link *link;
+	unsigned int i;
+	int err;
+
+	err = of_count_phandle_with_args(dev->of_node, "power-domains",
+					 "#power-domain-cells");
+	if (err < 0) {
+		if (err != -ENOENT)
+			return err;
+
+		/*
+		 * Older device-trees don't use GENPD. In this case we should
+		 * toggle power domain manually.
+		 */
+		err = gr3d_power_up_legacy_domain(dev, "3d",
+						  TEGRA_POWERGATE_3D);
+		if (err)
+			return err;
+
+		err = gr3d_power_up_legacy_domain(dev, "3d2",
+						  TEGRA_POWERGATE_3D1);
+		if (err)
+			return err;
+
+		return 0;
+	}
+
+	/*
+	 * The PM domain core automatically attaches a single power domain,
+	 * otherwise it skips attaching completely. We have a single domain
+	 * on Tegra20 and two domains on Tegra30+.
+	 */
+	if (dev->pm_domain)
+		return 0;
+
+	err = devm_pm_opp_attach_genpd(dev, opp_genpd_names, &opp_virt_devs);
+	if (err)
+		return err;
+
+	for (i = 0; opp_genpd_names[i]; i++) {
+		pd_dev = opp_virt_devs[i];
+		if (!pd_dev) {
+			dev_err(dev, "failed to get %s power domain\n",
+				opp_genpd_names[i]);
+			return -EINVAL;
+		}
+
+		link = device_link_add(dev, pd_dev, link_flags);
+		if (!link) {
+			dev_err(dev, "failed to link to %s\n", dev_name(pd_dev));
+			return -EINVAL;
+		}
+
+		err = devm_add_action_or_reset(dev, gr3d_del_link, link);
+		if (err)
+			return err;
+	}
+
+	return 0;
+}
+
+static int gr3d_set_opp(struct dev_pm_set_opp_data *data)
+{
+	struct gr3d *gr3d = dev_get_drvdata(data->dev);
+	unsigned int i;
+	int err;
+
+	for (i = 0; i < gr3d->nclocks; i++) {
+		err = clk_set_rate(gr3d->clocks[i].clk, data->new_opp.rate);
+		if (err) {
+			dev_err(data->dev, "failed to set %s rate to %lu: %d\n",
+				gr3d->clocks[i].id, data->new_opp.rate, err);
+			goto restore;
+		}
+	}
+
+	return 0;
+
+restore:
+	while (i--)
+		clk_set_rate(gr3d->clocks[i].clk, data->old_opp.rate);
+
+	return err;
+}
+
+static int gr3d_get_clocks(struct device *dev, struct gr3d *gr3d)
+{
+	int err;
+
+	err = devm_clk_bulk_get_all(dev, &gr3d->clocks);
+	if (err < 0) {
+		dev_err(dev, "failed to get clock: %d\n", err);
+		return err;
+	}
+	gr3d->nclocks = err;
+
+	if (gr3d->nclocks != gr3d->soc->num_clocks) {
+		dev_err(dev, "invalid number of clocks: %u\n", gr3d->nclocks);
+		return -ENOENT;
+	}
+
+	return 0;
+}
+
+static int gr3d_get_resets(struct device *dev, struct gr3d *gr3d)
+{
+	int err;
+
+	gr3d->resets[RST_MC].id = "mc";
+	gr3d->resets[RST_MC2].id = "mc2";
+	gr3d->resets[RST_GR3D].id = "3d";
+	gr3d->resets[RST_GR3D2].id = "3d2";
+	gr3d->nresets = gr3d->soc->num_resets;
+
+	err = devm_reset_control_bulk_get_optional_exclusive_released(
+				dev, gr3d->nresets, gr3d->resets);
+	if (err) {
+		dev_err(dev, "failed to get reset: %d\n", err);
+		return err;
+	}
+
+	if (WARN_ON(!gr3d->resets[RST_GR3D].rstc) ||
+	    WARN_ON(!gr3d->resets[RST_GR3D2].rstc && gr3d->nresets == 4))
+		return -ENOENT;
+
+	return 0;
+}
+
 static int gr3d_probe(struct platform_device *pdev)
 {
-	struct device_node *np = pdev->dev.of_node;
 	struct host1x_syncpt **syncpts;
 	struct gr3d *gr3d;
 	unsigned int i;
@@ -290,56 +521,33 @@ static int gr3d_probe(struct platform_device *pdev)
 	if (!gr3d)
 		return -ENOMEM;
 
+	platform_set_drvdata(pdev, gr3d);
+
 	gr3d->soc = of_device_get_match_data(&pdev->dev);
 
 	syncpts = devm_kzalloc(&pdev->dev, sizeof(*syncpts), GFP_KERNEL);
 	if (!syncpts)
 		return -ENOMEM;
 
-	gr3d->clk = devm_clk_get(&pdev->dev, NULL);
-	if (IS_ERR(gr3d->clk)) {
-		dev_err(&pdev->dev, "cannot get clock\n");
-		return PTR_ERR(gr3d->clk);
-	}
-
-	gr3d->rst = devm_reset_control_get(&pdev->dev, "3d");
-	if (IS_ERR(gr3d->rst)) {
-		dev_err(&pdev->dev, "cannot get reset\n");
-		return PTR_ERR(gr3d->rst);
-	}
+	err = gr3d_get_clocks(&pdev->dev, gr3d);
+	if (err)
+		return err;
 
-	if (of_device_is_compatible(np, "nvidia,tegra30-gr3d")) {
-		gr3d->clk_secondary = devm_clk_get(&pdev->dev, "3d2");
-		if (IS_ERR(gr3d->clk_secondary)) {
-			dev_err(&pdev->dev, "cannot get secondary clock\n");
-			return PTR_ERR(gr3d->clk_secondary);
-		}
+	err = gr3d_get_resets(&pdev->dev, gr3d);
+	if (err)
+		return err;
 
-		gr3d->rst_secondary = devm_reset_control_get(&pdev->dev,
-								"3d2");
-		if (IS_ERR(gr3d->rst_secondary)) {
-			dev_err(&pdev->dev, "cannot get secondary reset\n");
-			return PTR_ERR(gr3d->rst_secondary);
-		}
-	}
+	err = gr3d_init_power(&pdev->dev, gr3d);
+	if (err)
+		return err;
 
-	err = tegra_powergate_sequence_power_up(TEGRA_POWERGATE_3D, gr3d->clk,
-						gr3d->rst);
-	if (err < 0) {
-		dev_err(&pdev->dev, "failed to power up 3D unit\n");
+	err = devm_pm_opp_register_set_opp_helper(&pdev->dev, gr3d_set_opp);
+	if (err)
 		return err;
-	}
 
-	if (gr3d->clk_secondary) {
-		err = tegra_powergate_sequence_power_up(TEGRA_POWERGATE_3D1,
-							gr3d->clk_secondary,
-							gr3d->rst_secondary);
-		if (err < 0) {
-			dev_err(&pdev->dev,
-				"failed to power up secondary 3D unit\n");
-			return err;
-		}
-	}
+	err = devm_tegra_core_dev_init_opp_table_simple(&pdev->dev);
+	if (err)
+		return err;
 
 	INIT_LIST_HEAD(&gr3d->client.base.list);
 	gr3d->client.base.ops = &gr3d_client_ops;
@@ -352,20 +560,28 @@ static int gr3d_probe(struct platform_device *pdev)
 	gr3d->client.version = gr3d->soc->version;
 	gr3d->client.ops = &gr3d_ops;
 
+	pm_runtime_enable(&pdev->dev);
+	pm_runtime_use_autosuspend(&pdev->dev);
+	pm_runtime_set_autosuspend_delay(&pdev->dev, 200);
+
 	err = host1x_client_register(&gr3d->client.base);
 	if (err < 0) {
 		dev_err(&pdev->dev, "failed to register host1x client: %d\n",
 			err);
-		return err;
+		goto disable_rpm;
 	}
 
 	/* initialize address register map */
 	for (i = 0; i < ARRAY_SIZE(gr3d_addr_regs); i++)
 		set_bit(gr3d_addr_regs[i], gr3d->addr_regs);
 
-	platform_set_drvdata(pdev, gr3d);
-
 	return 0;
+
+disable_rpm:
+	pm_runtime_dont_use_autosuspend(&pdev->dev);
+	pm_runtime_disable(&pdev->dev);
+
+	return err;
 }
 
 static int gr3d_remove(struct platform_device *pdev)
@@ -380,23 +596,83 @@ static int gr3d_remove(struct platform_device *pdev)
 		return err;
 	}
 
-	if (gr3d->clk_secondary) {
-		reset_control_assert(gr3d->rst_secondary);
-		tegra_powergate_power_off(TEGRA_POWERGATE_3D1);
-		clk_disable_unprepare(gr3d->clk_secondary);
+	pm_runtime_dont_use_autosuspend(&pdev->dev);
+	pm_runtime_disable(&pdev->dev);
+
+	return 0;
+}
+
+static int __maybe_unused gr3d_runtime_suspend(struct device *dev)
+{
+	struct gr3d *gr3d = dev_get_drvdata(dev);
+	int err;
+
+	host1x_channel_stop(gr3d->channel);
+
+	err = reset_control_bulk_assert(gr3d->nresets, gr3d->resets);
+	if (err) {
+		dev_err(dev, "failed to assert reset: %d\n", err);
+		return err;
+	}
+
+	usleep_range(10, 20);
+
+	/*
+	 * Older device-trees don't specify MC resets and power-gating can't
+	 * be done safely in that case. Hence we will keep the power ungated
+	 * for older DTBs. For newer DTBs, GENPD will perform the power-gating.
+	 */
+
+	clk_bulk_disable_unprepare(gr3d->nclocks, gr3d->clocks);
+	reset_control_bulk_release(gr3d->nresets, gr3d->resets);
+
+	return 0;
+}
+
+static int __maybe_unused gr3d_runtime_resume(struct device *dev)
+{
+	struct gr3d *gr3d = dev_get_drvdata(dev);
+	int err;
+
+	err = reset_control_bulk_acquire(gr3d->nresets, gr3d->resets);
+	if (err) {
+		dev_err(dev, "failed to acquire reset: %d\n", err);
+		return err;
+	}
+
+	err = clk_bulk_prepare_enable(gr3d->nclocks, gr3d->clocks);
+	if (err) {
+		dev_err(dev, "failed to enable clock: %d\n", err);
+		goto release_reset;
 	}
 
-	reset_control_assert(gr3d->rst);
-	tegra_powergate_power_off(TEGRA_POWERGATE_3D);
-	clk_disable_unprepare(gr3d->clk);
+	err = reset_control_bulk_deassert(gr3d->nresets, gr3d->resets);
+	if (err) {
+		dev_err(dev, "failed to deassert reset: %d\n", err);
+		goto disable_clk;
+	}
 
 	return 0;
+
+disable_clk:
+	clk_bulk_disable_unprepare(gr3d->nclocks, gr3d->clocks);
+release_reset:
+	reset_control_bulk_release(gr3d->nresets, gr3d->resets);
+
+	return err;
 }
 
+static const struct dev_pm_ops tegra_gr3d_pm = {
+	SET_RUNTIME_PM_OPS(gr3d_runtime_suspend, gr3d_runtime_resume, NULL)
+	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
+				pm_runtime_force_resume)
+};
+
 struct platform_driver tegra_gr3d_driver = {
 	.driver = {
 		.name = "tegra-gr3d",
 		.of_match_table = tegra_gr3d_match,
+		.pm = &tegra_gr3d_pm,
 	},
 	.probe = gr3d_probe,
 	.remove = gr3d_remove,
-- 
2.32.0