[PATCH 0/4] qcom: Model RPMH power domains as thermal cooling devices

[PATCH 0/4] qcom: Model RPMH power domains as thermal cooling devices

[Thara Gopinath]

Certain RPMH power domains can be used to warm up the SoC (mx on sdm845)
if the temperature falls below certain threshold. These power domains
can be considered as thermal warming devices
(opposite of thermal cooling devices).

In kernel, these warming devices can be modeled as a 
thermal cooling device. To use these power domains as warming devices
require further tweaks in the thermal framework which are out of scope
of this patch series.

The first patch in this series extends the genpd framework to export out
the performance states of a power domain so that when the RPMH power
domain is modeled as a cooling device, the number of possible states and
current state of the cooling device can be retrieved from the genpd
framework.

The second patch implements the newly added genpd callback for RPMH power
domain driver.

The third patch implements the modeling of a RPMH power domain as
a cooling device and the final patch adds the device node entry for sdm845
to consider RPMHPD MX a cooling device.

Thara Gopinath (4):
  PM/Domains: Add support for retrieving genpd performance states
    information
  soc: qcom: rpmhpd: Introduce function to retrieve power domain
    performance state count
  thermal: qcom: Add RPMHPD cooling device driver.
  arm64: dts: qcom: Extend AOSS RPMHPD node

 arch/arm64/boot/dts/qcom/sdm845.dtsi    |   7 ++
 drivers/base/power/domain.c             |  38 +++++++++
 drivers/soc/qcom/rpmhpd.c               |  11 +++
 drivers/thermal/qcom/Kconfig            |   7 ++
 drivers/thermal/qcom/Makefile           |   1 +
 drivers/thermal/qcom/qcom-rpmhpd-cdev.c | 141 ++++++++++++++++++++++++++++++++
 include/linux/pm_domain.h               |  18 ++++
 7 files changed, 223 insertions(+)
 create mode 100644 drivers/thermal/qcom/qcom-rpmhpd-cdev.c

-- 
2.1.4

[PATCH 1/4] PM/Domains: Add support for retrieving genpd performance states information

[Thara Gopinath]

Add two new APIs in the genpd framework,
dev_pm_genpd_get_performance_state to return the current performance
state of a power domain and dev_pm_genpd_performance_state_count to
return the total number of performance states supported by a
power domain. Since the genpd framework does not maintain
a count of number of performance states supported by a power domain,
introduce a new callback(.get_performance_state_count) that can be used
to retrieve this information from power domain drivers.

Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org>
---
 drivers/base/power/domain.c | 38 ++++++++++++++++++++++++++++++++++++++
 include/linux/pm_domain.h   | 18 ++++++++++++++++++
 2 files changed, 56 insertions(+)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index b063bc4..17e0375 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -413,6 +413,44 @@ int dev_pm_genpd_set_performance_state(struct device *dev, unsigned int state)
 }
 EXPORT_SYMBOL_GPL(dev_pm_genpd_set_performance_state);
 
+int dev_pm_genpd_get_performance_state(struct device *dev,
+				       unsigned int *state)
+{
+	struct generic_pm_domain *genpd;
+
+	genpd = dev_to_genpd(dev);
+	if (IS_ERR(genpd))
+		return -ENODEV;
+
+	genpd_lock(genpd);
+	*state = genpd->performance_state;
+	genpd_unlock(genpd);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(dev_pm_genpd_get_performance_state);
+
+int dev_pm_genpd_performance_state_count(struct device *dev,
+					 unsigned int *count)
+{
+	struct generic_pm_domain *genpd;
+	int ret;
+
+	genpd = dev_to_genpd(dev);
+	if (IS_ERR(genpd))
+		return -ENODEV;
+
+	if (unlikely(!genpd->get_performance_state_count))
+		return -EINVAL;
+
+	genpd_lock(genpd);
+	ret = genpd->get_performance_state_count(genpd, count);
+	genpd_unlock(genpd);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(dev_pm_genpd_performance_state_count);
+
 static int _genpd_power_on(struct generic_pm_domain *genpd, bool timed)
 {
 	unsigned int state_idx = genpd->state_idx;
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index 91d9bf4..0e5f502 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -117,6 +117,8 @@ struct generic_pm_domain {
 						 struct dev_pm_opp *opp);
 	int (*set_performance_state)(struct generic_pm_domain *genpd,
 				     unsigned int state);
+	int (*get_performance_state_count)(struct generic_pm_domain *genpd,
+					   unsigned int *count);
 	struct gpd_dev_ops dev_ops;
 	s64 max_off_time_ns;	/* Maximum allowed "suspended" time. */
 	bool max_off_time_changed;
@@ -204,6 +206,10 @@ int pm_genpd_init(struct generic_pm_domain *genpd,
 		  struct dev_power_governor *gov, bool is_off);
 int pm_genpd_remove(struct generic_pm_domain *genpd);
 int dev_pm_genpd_set_performance_state(struct device *dev, unsigned int state);
+int dev_pm_genpd_get_performance_state(struct device *dev,
+				       unsigned int *state);
+int dev_pm_genpd_performance_state_count(struct device *dev,
+					 unsigned int *count);
 
 extern struct dev_power_governor simple_qos_governor;
 extern struct dev_power_governor pm_domain_always_on_gov;
@@ -251,6 +257,18 @@ static inline int dev_pm_genpd_set_performance_state(struct device *dev,
 	return -ENOTSUPP;
 }
 
+static inline int dev_pm_genpd_get_performance_state(struct device *dev,
+						     unsigned int *state);
+{
+	return -ENOTSUPP;
+}
+
+static inline int dev_pm_genpd_performance_state_count(struct device *dev,
+						       unsigned int *count);
+{
+	return -ENOTSUPP;
+}
+
 #define simple_qos_governor		(*(struct dev_power_governor *)(NULL))
 #define pm_domain_always_on_gov		(*(struct dev_power_governor *)(NULL))
 #endif
-- 
2.1.4

Re: [PATCH 1/4] PM/Domains: Add support for retrieving genpd performance states information

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2051 bytes --]

Hi Thara,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linus/master]
[cannot apply to v5.3-rc3 next-20190809]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Thara-Gopinath/qcom-Model-RPMH-power-domains-as-thermal-cooling-devices/20190811-103009
config: parisc-allmodconfig (attached as .config)
compiler: hppa-linux-gcc (GCC) 7.4.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.4.0 make.cross ARCH=parisc 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from drivers/soc/bcm/bcm2835-power.c:15:0:
>> include/linux/pm_domain.h:262:1: error: expected identifier or '(' before '{' token
    {
    ^
   include/linux/pm_domain.h:268:1: error: expected identifier or '(' before '{' token
    {
    ^
   include/linux/pm_domain.h:260:19: warning: 'dev_pm_genpd_get_performance_state' declared 'static' but never defined [-Wunused-function]
    static inline int dev_pm_genpd_get_performance_state(struct device *dev,
                      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/pm_domain.h:266:19: warning: 'dev_pm_genpd_performance_state_count' declared 'static' but never defined [-Wunused-function]
    static inline int dev_pm_genpd_performance_state_count(struct device *dev,
                      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

vim +262 include/linux/pm_domain.h

   259	
   260	static inline int dev_pm_genpd_get_performance_state(struct device *dev,
   261							     unsigned int *state);
 > 262	{
   263		return -ENOTSUPP;
   264	}
   265	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 57951 bytes --]

Re: [PATCH 1/4] PM/Domains: Add support for retrieving genpd performance states information

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2209 bytes --]

Hi Thara,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linus/master]
[cannot apply to v5.3-rc3 next-20190809]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Thara-Gopinath/qcom-Model-RPMH-power-domains-as-thermal-cooling-devices/20190811-103009
config: x86_64-randconfig-d001-201932 (attached as .config)
compiler: gcc-7 (Debian 7.4.0-10) 7.4.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from drivers/gpu/drm/i915/selftests/mock_gem_device.c:25:0,
                    from drivers/gpu/drm/i915/i915_gem.c:1906:
   include/linux/pm_domain.h:262:1: error: expected identifier or '(' before '{' token
    {
    ^
   include/linux/pm_domain.h:268:1: error: expected identifier or '(' before '{' token
    {
    ^
>> include/linux/pm_domain.h:260:19: error: 'dev_pm_genpd_get_performance_state' declared 'static' but never defined [-Werror=unused-function]
    static inline int dev_pm_genpd_get_performance_state(struct device *dev,
                      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> include/linux/pm_domain.h:266:19: error: 'dev_pm_genpd_performance_state_count' declared 'static' but never defined [-Werror=unused-function]
    static inline int dev_pm_genpd_performance_state_count(struct device *dev,
                      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   cc1: all warnings being treated as errors

vim +260 include/linux/pm_domain.h

   259	
 > 260	static inline int dev_pm_genpd_get_performance_state(struct device *dev,
   261							     unsigned int *state);
 > 262	{
   263		return -ENOTSUPP;
   264	}
   265	
 > 266	static inline int dev_pm_genpd_performance_state_count(struct device *dev,
   267							       unsigned int *count);
   268	{
   269		return -ENOTSUPP;
   270	}
   271	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 39407 bytes --]

Re: [PATCH 1/4] PM/Domains: Add support for retrieving genpd performance states information

[Ulf Hansson]

On Sat, 10 Aug 2019 at 02:58, Thara Gopinath <thara.gopinath@linaro.org> wrote:
>
> Add two new APIs in the genpd framework,
> dev_pm_genpd_get_performance_state to return the current performance
> state of a power domain and dev_pm_genpd_performance_state_count to
> return the total number of performance states supported by a
> power domain. Since the genpd framework does not maintain
> a count of number of performance states supported by a power domain,
> introduce a new callback(.get_performance_state_count) that can be used
> to retrieve this information from power domain drivers.

I think some brief background to *why* this is useful needs to be
squeezed into the changelog. Or at least state that following changes
makes use of it, somehow.

>
> Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org>
> ---
>  drivers/base/power/domain.c | 38 ++++++++++++++++++++++++++++++++++++++
>  include/linux/pm_domain.h   | 18 ++++++++++++++++++
>  2 files changed, 56 insertions(+)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index b063bc4..17e0375 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -413,6 +413,44 @@ int dev_pm_genpd_set_performance_state(struct device *dev, unsigned int state)
>  }
>  EXPORT_SYMBOL_GPL(dev_pm_genpd_set_performance_state);
>
> +int dev_pm_genpd_get_performance_state(struct device *dev,
> +                                      unsigned int *state)
> +{
> +       struct generic_pm_domain *genpd;
> +
> +       genpd = dev_to_genpd(dev);

We need to verify that the there is a genpd attached before doing this
cast. Let me post a patch in a day or so, it will give you a helper
function that covers this.

> +       if (IS_ERR(genpd))
> +               return -ENODEV;
> +
> +       genpd_lock(genpd);
> +       *state = genpd->performance_state;

Why not return the state, rather than assigning an out-parameter?

> +       genpd_unlock(genpd);
> +
> +       return 0;
> +}
> +EXPORT_SYMBOL_GPL(dev_pm_genpd_get_performance_state);
> +
> +int dev_pm_genpd_performance_state_count(struct device *dev,
> +                                        unsigned int *count)
> +{
> +       struct generic_pm_domain *genpd;
> +       int ret;
> +
> +       genpd = dev_to_genpd(dev);
> +       if (IS_ERR(genpd))
> +               return -ENODEV;
> +
> +       if (unlikely(!genpd->get_performance_state_count))
> +               return -EINVAL;
> +
> +       genpd_lock(genpd);
> +       ret = genpd->get_performance_state_count(genpd, count);

Why not having the callback to return the state, rather than using an
out-parameter?

> +       genpd_unlock(genpd);
> +
> +       return ret;
> +}
> +EXPORT_SYMBOL_GPL(dev_pm_genpd_performance_state_count);
> +
>  static int _genpd_power_on(struct generic_pm_domain *genpd, bool timed)
>  {
>         unsigned int state_idx = genpd->state_idx;
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index 91d9bf4..0e5f502 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -117,6 +117,8 @@ struct generic_pm_domain {
>                                                  struct dev_pm_opp *opp);
>         int (*set_performance_state)(struct generic_pm_domain *genpd,
>                                      unsigned int state);
> +       int (*get_performance_state_count)(struct generic_pm_domain *genpd,
> +                                          unsigned int *count);
>         struct gpd_dev_ops dev_ops;
>         s64 max_off_time_ns;    /* Maximum allowed "suspended" time. */
>         bool max_off_time_changed;
> @@ -204,6 +206,10 @@ int pm_genpd_init(struct generic_pm_domain *genpd,
>                   struct dev_power_governor *gov, bool is_off);
>  int pm_genpd_remove(struct generic_pm_domain *genpd);
>  int dev_pm_genpd_set_performance_state(struct device *dev, unsigned int state);
> +int dev_pm_genpd_get_performance_state(struct device *dev,
> +                                      unsigned int *state);
> +int dev_pm_genpd_performance_state_count(struct device *dev,
> +                                        unsigned int *count);
>
>  extern struct dev_power_governor simple_qos_governor;
>  extern struct dev_power_governor pm_domain_always_on_gov;
> @@ -251,6 +257,18 @@ static inline int dev_pm_genpd_set_performance_state(struct device *dev,
>         return -ENOTSUPP;
>  }
>
> +static inline int dev_pm_genpd_get_performance_state(struct device *dev,
> +                                                    unsigned int *state);
> +{
> +       return -ENOTSUPP;
> +}
> +
> +static inline int dev_pm_genpd_performance_state_count(struct device *dev,
> +                                                      unsigned int *count);
> +{
> +       return -ENOTSUPP;
> +}
> +
>  #define simple_qos_governor            (*(struct dev_power_governor *)(NULL))
>  #define pm_domain_always_on_gov                (*(struct dev_power_governor *)(NULL))
>  #endif
> --
> 2.1.4
>

Kind regards
Uffe

Re: [PATCH 1/4] PM/Domains: Add support for retrieving genpd performance states information

[Thara Gopinath]

Hi Ulf,

Thanks for the review.

On 08/22/2019 11:03 AM, Ulf Hansson wrote:
> On Sat, 10 Aug 2019 at 02:58, Thara Gopinath <thara.gopinath@linaro.org> wrote:
>>
>> Add two new APIs in the genpd framework,
>> dev_pm_genpd_get_performance_state to return the current performance
>> state of a power domain and dev_pm_genpd_performance_state_count to
>> return the total number of performance states supported by a
>> power domain. Since the genpd framework does not maintain
>> a count of number of performance states supported by a power domain,
>> introduce a new callback(.get_performance_state_count) that can be used
>> to retrieve this information from power domain drivers.
> 
> I think some brief background to *why* this is useful needs to be
> squeezed into the changelog. Or at least state that following changes
> makes use of it, somehow.

Will do.
> 
>>
>> Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org>
>> ---
>>  drivers/base/power/domain.c | 38 ++++++++++++++++++++++++++++++++++++++
>>  include/linux/pm_domain.h   | 18 ++++++++++++++++++
>>  2 files changed, 56 insertions(+)
>>
>> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
>> index b063bc4..17e0375 100644
>> --- a/drivers/base/power/domain.c
>> +++ b/drivers/base/power/domain.c
>> @@ -413,6 +413,44 @@ int dev_pm_genpd_set_performance_state(struct device *dev, unsigned int state)
>>  }
>>  EXPORT_SYMBOL_GPL(dev_pm_genpd_set_performance_state);
>>
>> +int dev_pm_genpd_get_performance_state(struct device *dev,
>> +                                      unsigned int *state)
>> +{
>> +       struct generic_pm_domain *genpd;
>> +
>> +       genpd = dev_to_genpd(dev);
> 
> We need to verify that the there is a genpd attached before doing this
> cast. Let me post a patch in a day or so, it will give you a helper
> function that covers this.

Sounds good.. Thanks.. I will wait for it.

> 
>> +       if (IS_ERR(genpd))
>> +               return -ENODEV;
>> +
>> +       genpd_lock(genpd);
>> +       *state = genpd->performance_state;
> 
> Why not return the state, rather than assigning an out-parameter?
We can. I will change it for this and dev_pm_genpd_performance_state_count.

Regards
Thara

> 
>> +       genpd_unlock(genpd);
>> +
>> +       return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(dev_pm_genpd_get_performance_state);
>> +
>> +int dev_pm_genpd_performance_state_count(struct device *dev,
>> +                                        unsigned int *count)
>> +{
>> +       struct generic_pm_domain *genpd;
>> +       int ret;
>> +
>> +       genpd = dev_to_genpd(dev);
>> +       if (IS_ERR(genpd))
>> +               return -ENODEV;
>> +
>> +       if (unlikely(!genpd->get_performance_state_count))
>> +               return -EINVAL;
>> +
>> +       genpd_lock(genpd);
>> +       ret = genpd->get_performance_state_count(genpd, count);
> 
> Why not having the callback to return the state, rather than using an
> out-parameter?
> 
>> +       genpd_unlock(genpd);
>> +
>> +       return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(dev_pm_genpd_performance_state_count);
>> +
>>  static int _genpd_power_on(struct generic_pm_domain *genpd, bool timed)
>>  {
>>         unsigned int state_idx = genpd->state_idx;
>> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
>> index 91d9bf4..0e5f502 100644
>> --- a/include/linux/pm_domain.h
>> +++ b/include/linux/pm_domain.h
>> @@ -117,6 +117,8 @@ struct generic_pm_domain {
>>                                                  struct dev_pm_opp *opp);
>>         int (*set_performance_state)(struct generic_pm_domain *genpd,
>>                                      unsigned int state);
>> +       int (*get_performance_state_count)(struct generic_pm_domain *genpd,
>> +                                          unsigned int *count);
>>         struct gpd_dev_ops dev_ops;
>>         s64 max_off_time_ns;    /* Maximum allowed "suspended" time. */
>>         bool max_off_time_changed;
>> @@ -204,6 +206,10 @@ int pm_genpd_init(struct generic_pm_domain *genpd,
>>                   struct dev_power_governor *gov, bool is_off);
>>  int pm_genpd_remove(struct generic_pm_domain *genpd);
>>  int dev_pm_genpd_set_performance_state(struct device *dev, unsigned int state);
>> +int dev_pm_genpd_get_performance_state(struct device *dev,
>> +                                      unsigned int *state);
>> +int dev_pm_genpd_performance_state_count(struct device *dev,
>> +                                        unsigned int *count);
>>
>>  extern struct dev_power_governor simple_qos_governor;
>>  extern struct dev_power_governor pm_domain_always_on_gov;
>> @@ -251,6 +257,18 @@ static inline int dev_pm_genpd_set_performance_state(struct device *dev,
>>         return -ENOTSUPP;
>>  }
>>
>> +static inline int dev_pm_genpd_get_performance_state(struct device *dev,
>> +                                                    unsigned int *state);
>> +{
>> +       return -ENOTSUPP;
>> +}
>> +
>> +static inline int dev_pm_genpd_performance_state_count(struct device *dev,
>> +                                                      unsigned int *count);
>> +{
>> +       return -ENOTSUPP;
>> +}
>> +
>>  #define simple_qos_governor            (*(struct dev_power_governor *)(NULL))
>>  #define pm_domain_always_on_gov                (*(struct dev_power_governor *)(NULL))
>>  #endif
>> --
>> 2.1.4
>>
> 
> Kind regards
> Uffe
> 


-- 
Regards
Thara

Re: [PATCH 1/4] PM/Domains: Add support for retrieving genpd performance states information

[Thara Gopinath]

On 08/22/2019 11:03 AM, Ulf Hansson wrote:
> On Sat, 10 Aug 2019 at 02:58, Thara Gopinath <thara.gopinath@linaro.org> wrote:
>>
>> Add two new APIs in the genpd framework,
>> dev_pm_genpd_get_performance_state to return the current performance
>> state of a power domain and dev_pm_genpd_performance_state_count to
>> return the total number of performance states supported by a
>> power domain. Since the genpd framework does not maintain
>> a count of number of performance states supported by a power domain,
>> introduce a new callback(.get_performance_state_count) that can be used
>> to retrieve this information from power domain drivers.
> 
> I think some brief background to *why* this is useful needs to be
> squeezed into the changelog. Or at least state that following changes
> makes use of it, somehow.
> 
>>
>> Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org>
>> ---
>>  drivers/base/power/domain.c | 38 ++++++++++++++++++++++++++++++++++++++
>>  include/linux/pm_domain.h   | 18 ++++++++++++++++++
>>  2 files changed, 56 insertions(+)
>>
>> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
>> index b063bc4..17e0375 100644
>> --- a/drivers/base/power/domain.c
>> +++ b/drivers/base/power/domain.c
>> @@ -413,6 +413,44 @@ int dev_pm_genpd_set_performance_state(struct device *dev, unsigned int state)
>>  }
>>  EXPORT_SYMBOL_GPL(dev_pm_genpd_set_performance_state);
>>
>> +int dev_pm_genpd_get_performance_state(struct device *dev,
>> +                                      unsigned int *state)
>> +{
>> +       struct generic_pm_domain *genpd;
>> +
>> +       genpd = dev_to_genpd(dev);
> 
> We need to verify that the there is a genpd attached before doing this
> cast. Let me post a patch in a day or so, it will give you a helper
> function that covers this.
> 
>> +       if (IS_ERR(genpd))
>> +               return -ENODEV;
>> +
>> +       genpd_lock(genpd);
>> +       *state = genpd->performance_state;
> 
> Why not return the state, rather than assigning an out-parameter?
> 
>> +       genpd_unlock(genpd);
>> +
>> +       return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(dev_pm_genpd_get_performance_state);
>> +
>> +int dev_pm_genpd_performance_state_count(struct device *dev,
>> +                                        unsigned int *count)
>> +{
>> +       struct generic_pm_domain *genpd;
>> +       int ret;
>> +
>> +       genpd = dev_to_genpd(dev);
>> +       if (IS_ERR(genpd))
>> +               return -ENODEV;
>> +
>> +       if (unlikely(!genpd->get_performance_state_count))
>> +               return -EINVAL;
>> +
>> +       genpd_lock(genpd);
>> +       ret = genpd->get_performance_state_count(genpd, count);
> 
> Why not having the callback to return the state, rather than using an
> out-parameter?
Hi Ulf,
I just realized that returning the state instead of using a parameter
will prevent me from access under lock. Is that okay ?

Regards
Thara
> 
>> +       genpd_unlock(genpd);
>> +
>> +       return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(dev_pm_genpd_performance_state_count);
>> +
>>  static int _genpd_power_on(struct generic_pm_domain *genpd, bool timed)
>>  {
>>         unsigned int state_idx = genpd->state_idx;
>> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
>> index 91d9bf4..0e5f502 100644
>> --- a/include/linux/pm_domain.h
>> +++ b/include/linux/pm_domain.h
>> @@ -117,6 +117,8 @@ struct generic_pm_domain {
>>                                                  struct dev_pm_opp *opp);
>>         int (*set_performance_state)(struct generic_pm_domain *genpd,
>>                                      unsigned int state);
>> +       int (*get_performance_state_count)(struct generic_pm_domain *genpd,
>> +                                          unsigned int *count);
>>         struct gpd_dev_ops dev_ops;
>>         s64 max_off_time_ns;    /* Maximum allowed "suspended" time. */
>>         bool max_off_time_changed;
>> @@ -204,6 +206,10 @@ int pm_genpd_init(struct generic_pm_domain *genpd,
>>                   struct dev_power_governor *gov, bool is_off);
>>  int pm_genpd_remove(struct generic_pm_domain *genpd);
>>  int dev_pm_genpd_set_performance_state(struct device *dev, unsigned int state);
>> +int dev_pm_genpd_get_performance_state(struct device *dev,
>> +                                      unsigned int *state);
>> +int dev_pm_genpd_performance_state_count(struct device *dev,
>> +                                        unsigned int *count);
>>
>>  extern struct dev_power_governor simple_qos_governor;
>>  extern struct dev_power_governor pm_domain_always_on_gov;
>> @@ -251,6 +257,18 @@ static inline int dev_pm_genpd_set_performance_state(struct device *dev,
>>         return -ENOTSUPP;
>>  }
>>
>> +static inline int dev_pm_genpd_get_performance_state(struct device *dev,
>> +                                                    unsigned int *state);
>> +{
>> +       return -ENOTSUPP;
>> +}
>> +
>> +static inline int dev_pm_genpd_performance_state_count(struct device *dev,
>> +                                                      unsigned int *count);
>> +{
>> +       return -ENOTSUPP;
>> +}
>> +
>>  #define simple_qos_governor            (*(struct dev_power_governor *)(NULL))
>>  #define pm_domain_always_on_gov                (*(struct dev_power_governor *)(NULL))
>>  #endif
>> --
>> 2.1.4
>>
> 
> Kind regards
> Uffe
> 


-- 
Regards
Thara

Re: [PATCH 1/4] PM/Domains: Add support for retrieving genpd performance states information

[Ulf Hansson]

On Sat, 7 Sep 2019 at 00:24, Thara Gopinath <thara.gopinath@linaro.org> wrote:
>
> On 08/22/2019 11:03 AM, Ulf Hansson wrote:
> > On Sat, 10 Aug 2019 at 02:58, Thara Gopinath <thara.gopinath@linaro.org> wrote:
> >>
> >> Add two new APIs in the genpd framework,
> >> dev_pm_genpd_get_performance_state to return the current performance
> >> state of a power domain and dev_pm_genpd_performance_state_count to
> >> return the total number of performance states supported by a
> >> power domain. Since the genpd framework does not maintain
> >> a count of number of performance states supported by a power domain,
> >> introduce a new callback(.get_performance_state_count) that can be used
> >> to retrieve this information from power domain drivers.
> >
> > I think some brief background to *why* this is useful needs to be
> > squeezed into the changelog. Or at least state that following changes
> > makes use of it, somehow.
> >
> >>
> >> Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org>
> >> ---
> >>  drivers/base/power/domain.c | 38 ++++++++++++++++++++++++++++++++++++++
> >>  include/linux/pm_domain.h   | 18 ++++++++++++++++++
> >>  2 files changed, 56 insertions(+)
> >>
> >> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> >> index b063bc4..17e0375 100644
> >> --- a/drivers/base/power/domain.c
> >> +++ b/drivers/base/power/domain.c
> >> @@ -413,6 +413,44 @@ int dev_pm_genpd_set_performance_state(struct device *dev, unsigned int state)
> >>  }
> >>  EXPORT_SYMBOL_GPL(dev_pm_genpd_set_performance_state);
> >>
> >> +int dev_pm_genpd_get_performance_state(struct device *dev,
> >> +                                      unsigned int *state)
> >> +{
> >> +       struct generic_pm_domain *genpd;
> >> +
> >> +       genpd = dev_to_genpd(dev);
> >
> > We need to verify that the there is a genpd attached before doing this
> > cast. Let me post a patch in a day or so, it will give you a helper
> > function that covers this.
> >
> >> +       if (IS_ERR(genpd))
> >> +               return -ENODEV;
> >> +
> >> +       genpd_lock(genpd);
> >> +       *state = genpd->performance_state;
> >
> > Why not return the state, rather than assigning an out-parameter?
> >
> >> +       genpd_unlock(genpd);
> >> +
> >> +       return 0;
> >> +}
> >> +EXPORT_SYMBOL_GPL(dev_pm_genpd_get_performance_state);
> >> +
> >> +int dev_pm_genpd_performance_state_count(struct device *dev,
> >> +                                        unsigned int *count)
> >> +{
> >> +       struct generic_pm_domain *genpd;
> >> +       int ret;
> >> +
> >> +       genpd = dev_to_genpd(dev);
> >> +       if (IS_ERR(genpd))
> >> +               return -ENODEV;
> >> +
> >> +       if (unlikely(!genpd->get_performance_state_count))
> >> +               return -EINVAL;
> >> +
> >> +       genpd_lock(genpd);
> >> +       ret = genpd->get_performance_state_count(genpd, count);
> >
> > Why not having the callback to return the state, rather than using an
> > out-parameter?
> Hi Ulf,
> I just realized that returning the state instead of using a parameter
> will prevent me from access under lock. Is that okay ?

Not sure I understand. Why can't you just assign a local variable and
return that?

Like this:

genpd_lock();
count = genpd->get_performance_state_count();
genpd_unlock();

return count;

[...]

Kind regards
Uffe

[PATCH 2/4] soc: qcom: rpmhpd: Introduce function to retrieve power domain performance state count

[Thara Gopinath]

Populate .get_performace_state_count in genpd ops to retrieve the count
of performance states supported by a rpmh power domain.

Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org>
---
 drivers/soc/qcom/rpmhpd.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/soc/qcom/rpmhpd.c b/drivers/soc/qcom/rpmhpd.c
index 5741ec3..c3ceffd 100644
--- a/drivers/soc/qcom/rpmhpd.c
+++ b/drivers/soc/qcom/rpmhpd.c
@@ -285,6 +285,15 @@ static unsigned int rpmhpd_get_performance_state(struct generic_pm_domain *genpd
 	return dev_pm_opp_get_level(opp);
 }
 
+static int rpmhpd_performance_states_count(struct generic_pm_domain *domain,
+					   unsigned int *count)
+{
+	struct rpmhpd *pd = domain_to_rpmhpd(domain);
+
+	*count = pd->level_count;
+	return 0;
+}
+
 static int rpmhpd_update_level_mapping(struct rpmhpd *rpmhpd)
 {
 	int i;
@@ -373,6 +382,8 @@ static int rpmhpd_probe(struct platform_device *pdev)
 		rpmhpds[i]->pd.power_on = rpmhpd_power_on;
 		rpmhpds[i]->pd.set_performance_state = rpmhpd_set_performance_state;
 		rpmhpds[i]->pd.opp_to_performance_state = rpmhpd_get_performance_state;
+		rpmhpds[i]->pd.get_performance_state_count =
+					rpmhpd_performance_states_count;
 		pm_genpd_init(&rpmhpds[i]->pd, NULL, true);
 
 		data->domains[i] = &rpmhpds[i]->pd;
-- 
2.1.4

[PATCH 3/4] thermal: qcom: Add RPMHPD cooling device driver.

[Thara Gopinath]

The MX power domain in RPMH can be used to warm the
the SoC in SDM845. To support this feature, introduce
a RPMH power domain cooling device driver that can be
plugged into the thermal framework.(The thermal framework
itself requires further modifiction to support a warming
device in place of a cooling device. Those extensions are
not introduced in this patch series).

Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org>
---
 drivers/thermal/qcom/Kconfig            |   7 ++
 drivers/thermal/qcom/Makefile           |   1 +
 drivers/thermal/qcom/qcom-rpmhpd-cdev.c | 141 ++++++++++++++++++++++++++++++++
 3 files changed, 149 insertions(+)
 create mode 100644 drivers/thermal/qcom/qcom-rpmhpd-cdev.c

diff --git a/drivers/thermal/qcom/Kconfig b/drivers/thermal/qcom/Kconfig
index aa9c1d8..a540130 100644
--- a/drivers/thermal/qcom/Kconfig
+++ b/drivers/thermal/qcom/Kconfig
@@ -20,3 +20,10 @@ config QCOM_SPMI_TEMP_ALARM
 	  trip points. The temperature reported by the thermal sensor reflects the
 	  real time die temperature if an ADC is present or an estimate of the
 	  temperature based upon the over temperature stage value.
+
+config CONFIG_QCOM_RPMHPD_CDEV
+	tristate "Qualcomm RPMHPD based cooling device"
+	depends on QCOM_RPMHPD
+	help
+	  This enables RPMHPD based cooling devices. On SDM845, this is
+	  MX power domain.
diff --git a/drivers/thermal/qcom/Makefile b/drivers/thermal/qcom/Makefile
index 7c8dc6e..e4eb520 100644
--- a/drivers/thermal/qcom/Makefile
+++ b/drivers/thermal/qcom/Makefile
@@ -4,3 +4,4 @@ obj-$(CONFIG_QCOM_TSENS)	+= qcom_tsens.o
 qcom_tsens-y			+= tsens.o tsens-common.o tsens-v0_1.o \
 				   tsens-8960.o tsens-v2.o tsens-v1.o
 obj-$(CONFIG_QCOM_SPMI_TEMP_ALARM)	+= qcom-spmi-temp-alarm.o
+obj-$(CONFIG_QCOM_RPMHPD_CDEV)		+= qcom-rpmhpd-cdev.o
diff --git a/drivers/thermal/qcom/qcom-rpmhpd-cdev.c b/drivers/thermal/qcom/qcom-rpmhpd-cdev.c
new file mode 100644
index 0000000..265523b
--- /dev/null
+++ b/drivers/thermal/qcom/qcom-rpmhpd-cdev.c
@@ -0,0 +1,141 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2019, Linaro Ltd
+ */
+#include <linux/err.h>
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/module.h>
+#include <linux/pm_domain.h>
+#include <linux/pm_runtime.h>
+#include <linux/thermal.h>
+
+struct rpmhpd_cooling_device {
+	struct thermal_cooling_device *cdev;
+	struct device *pwr_domain;
+	int max_state;
+	int cur_state;
+	bool is_pwr_domain_on;
+};
+
+static const char sdm845_rpmhpd_cdev_name[] = "mx";
+
+static const struct of_device_id rpmhpd_cdev_match_table[] = {
+	{ .compatible = "qcom,sdm845-rpmhpd-cdev", .data = &sdm845_rpmhpd_cdev_name },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, rpmhpd_cdev_match_table);
+
+static int rpmhpd_cdev_get_max_state(struct thermal_cooling_device *cdev,
+				     unsigned long *state)
+{
+	struct rpmhpd_cooling_device *rpmhpd_cdev = cdev->devdata;
+
+	*state = rpmhpd_cdev->max_state;
+	return 0;
+}
+
+static int rpmhpd_cdev_get_cur_state(struct thermal_cooling_device *cdev,
+				     unsigned long *state)
+{
+	struct rpmhpd_cooling_device *rpmhpd_cdev = cdev->devdata;
+	int perf_state;
+
+	dev_pm_genpd_get_performance_state(rpmhpd_cdev->pwr_domain,
+					   &perf_state);
+	*state = perf_state;
+	return 0;
+}
+
+static int rpmhpd_cdev_set_cur_state(struct thermal_cooling_device *cdev,
+				     unsigned long state)
+{
+	struct rpmhpd_cooling_device *rpmhpd_cdev = cdev->devdata;
+	struct device *pwr_domain = rpmhpd_cdev->pwr_domain;
+	int ret;
+
+	ret = dev_pm_genpd_set_performance_state(pwr_domain, state);
+
+	if (ret)
+		return ret;
+
+	if (state && !rpmhpd_cdev->is_pwr_domain_on) {
+		ret = pm_runtime_get_sync(pwr_domain);
+		rpmhpd_cdev->is_pwr_domain_on = true;
+	} else if (!state && rpmhpd_cdev->is_pwr_domain_on) {
+		ret = pm_runtime_put(pwr_domain);
+		rpmhpd_cdev->is_pwr_domain_on = false;
+	}
+
+	return ret;
+}
+
+static struct thermal_cooling_device_ops rpmhpd_cooling_device_ops = {
+	.get_max_state = rpmhpd_cdev_get_max_state,
+	.get_cur_state = rpmhpd_cdev_get_cur_state,
+	.set_cur_state = rpmhpd_cdev_set_cur_state,
+};
+
+static int rpmhpd_cdev_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev, *pd_dev;
+	struct rpmhpd_cooling_device *rpmhpd_cdev;
+	const char *rpmhpd_cdev_name = of_device_get_match_data(dev);
+	unsigned int count;
+	int ret;
+
+	if (!dev->pm_domain) {
+		pd_dev = dev_pm_domain_attach_by_name(dev, rpmhpd_cdev_name);
+		if (IS_ERR(pd_dev))
+			return PTR_ERR(pd_dev);
+	} else {
+		pd_dev = dev;
+	}
+
+	rpmhpd_cdev = devm_kzalloc(dev, sizeof(*rpmhpd_cdev), GFP_KERNEL);
+	if (!rpmhpd_cdev) {
+		ret = -ENOMEM;
+		goto detach_pd;
+	}
+
+	ret = dev_pm_genpd_performance_state_count(pd_dev, &count);
+	if (ret)
+		goto detach_pd;
+
+	rpmhpd_cdev->pwr_domain = pd_dev;
+	rpmhpd_cdev->max_state = count - 1;
+	rpmhpd_cdev->is_pwr_domain_on = false;
+
+	pm_runtime_enable(pd_dev);
+
+	rpmhpd_cdev->cdev = thermal_of_cooling_device_register
+					(dev->of_node, rpmhpd_cdev_name,
+					 rpmhpd_cdev,
+					 &rpmhpd_cooling_device_ops);
+	if (IS_ERR(rpmhpd_cdev->cdev)) {
+		dev_err(dev, "unable to register %s cooling device\n",
+			rpmhpd_cdev_name);
+		ret = PTR_ERR(rpmhpd_cdev->cdev);
+		goto detach_pd;
+	}
+
+	return 0;
+
+detach_pd:
+	dev_pm_domain_detach(pd_dev, false);
+	return ret;
+}
+
+static struct platform_driver rpmhpd_cdev_driver = {
+	.driver = {
+		.name = "qcom-rpmhpd-cdev",
+		.of_match_table = rpmhpd_cdev_match_table,
+	},
+	.probe = rpmhpd_cdev_probe,
+};
+module_platform_driver(rpmhpd_cdev_driver);
+
+MODULE_DESCRIPTION("Qualcomm RPMHPD cooling device driver");
+MODULE_LICENSE("GPL v2");
-- 
2.1.4

Re: [PATCH 3/4] thermal: qcom: Add RPMHPD cooling device driver.

[Ulf Hansson]

On Sat, 10 Aug 2019 at 02:58, Thara Gopinath <thara.gopinath@linaro.org> wrote:
>
> The MX power domain in RPMH can be used to warm the
> the SoC in SDM845. To support this feature, introduce
> a RPMH power domain cooling device driver that can be
> plugged into the thermal framework.(The thermal framework
> itself requires further modifiction to support a warming
> device in place of a cooling device. Those extensions are
> not introduced in this patch series).
>
> Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org>
> ---
>  drivers/thermal/qcom/Kconfig            |   7 ++
>  drivers/thermal/qcom/Makefile           |   1 +
>  drivers/thermal/qcom/qcom-rpmhpd-cdev.c | 141 ++++++++++++++++++++++++++++++++
>  3 files changed, 149 insertions(+)
>  create mode 100644 drivers/thermal/qcom/qcom-rpmhpd-cdev.c
>
> diff --git a/drivers/thermal/qcom/Kconfig b/drivers/thermal/qcom/Kconfig
> index aa9c1d8..a540130 100644
> --- a/drivers/thermal/qcom/Kconfig
> +++ b/drivers/thermal/qcom/Kconfig
> @@ -20,3 +20,10 @@ config QCOM_SPMI_TEMP_ALARM
>           trip points. The temperature reported by the thermal sensor reflects the
>           real time die temperature if an ADC is present or an estimate of the
>           temperature based upon the over temperature stage value.
> +
> +config CONFIG_QCOM_RPMHPD_CDEV
> +       tristate "Qualcomm RPMHPD based cooling device"
> +       depends on QCOM_RPMHPD
> +       help
> +         This enables RPMHPD based cooling devices. On SDM845, this is
> +         MX power domain.
> diff --git a/drivers/thermal/qcom/Makefile b/drivers/thermal/qcom/Makefile
> index 7c8dc6e..e4eb520 100644
> --- a/drivers/thermal/qcom/Makefile
> +++ b/drivers/thermal/qcom/Makefile
> @@ -4,3 +4,4 @@ obj-$(CONFIG_QCOM_TSENS)        += qcom_tsens.o
>  qcom_tsens-y                   += tsens.o tsens-common.o tsens-v0_1.o \
>                                    tsens-8960.o tsens-v2.o tsens-v1.o
>  obj-$(CONFIG_QCOM_SPMI_TEMP_ALARM)     += qcom-spmi-temp-alarm.o
> +obj-$(CONFIG_QCOM_RPMHPD_CDEV)         += qcom-rpmhpd-cdev.o
> diff --git a/drivers/thermal/qcom/qcom-rpmhpd-cdev.c b/drivers/thermal/qcom/qcom-rpmhpd-cdev.c
> new file mode 100644
> index 0000000..265523b
> --- /dev/null
> +++ b/drivers/thermal/qcom/qcom-rpmhpd-cdev.c
> @@ -0,0 +1,141 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2019, Linaro Ltd
> + */
> +#include <linux/err.h>
> +#include <linux/kernel.h>
> +#include <linux/init.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/module.h>
> +#include <linux/pm_domain.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/thermal.h>
> +
> +struct rpmhpd_cooling_device {
> +       struct thermal_cooling_device *cdev;
> +       struct device *pwr_domain;
> +       int max_state;
> +       int cur_state;
> +       bool is_pwr_domain_on;
> +};
> +
> +static const char sdm845_rpmhpd_cdev_name[] = "mx";
> +
> +static const struct of_device_id rpmhpd_cdev_match_table[] = {
> +       { .compatible = "qcom,sdm845-rpmhpd-cdev", .data = &sdm845_rpmhpd_cdev_name },
> +       { }
> +};
> +MODULE_DEVICE_TABLE(of, rpmhpd_cdev_match_table);
> +
> +static int rpmhpd_cdev_get_max_state(struct thermal_cooling_device *cdev,
> +                                    unsigned long *state)
> +{
> +       struct rpmhpd_cooling_device *rpmhpd_cdev = cdev->devdata;
> +
> +       *state = rpmhpd_cdev->max_state;
> +       return 0;
> +}
> +
> +static int rpmhpd_cdev_get_cur_state(struct thermal_cooling_device *cdev,
> +                                    unsigned long *state)
> +{
> +       struct rpmhpd_cooling_device *rpmhpd_cdev = cdev->devdata;
> +       int perf_state;
> +
> +       dev_pm_genpd_get_performance_state(rpmhpd_cdev->pwr_domain,
> +                                          &perf_state);
> +       *state = perf_state;
> +       return 0;
> +}
> +
> +static int rpmhpd_cdev_set_cur_state(struct thermal_cooling_device *cdev,
> +                                    unsigned long state)
> +{
> +       struct rpmhpd_cooling_device *rpmhpd_cdev = cdev->devdata;
> +       struct device *pwr_domain = rpmhpd_cdev->pwr_domain;

Nitpick: Using pwr_domain as the name of the variable is a bit
confusing. Why not just name it "dev"?

> +       int ret;
> +
> +       ret = dev_pm_genpd_set_performance_state(pwr_domain, state);
> +
> +       if (ret)
> +               return ret;
> +
> +       if (state && !rpmhpd_cdev->is_pwr_domain_on) {

Nitpick: To clarify, I would suggest to rename "is_pwr_domain_on" to
"runtime_resumed".

> +               ret = pm_runtime_get_sync(pwr_domain);
> +               rpmhpd_cdev->is_pwr_domain_on = true;
> +       } else if (!state && rpmhpd_cdev->is_pwr_domain_on) {
> +               ret = pm_runtime_put(pwr_domain);
> +               rpmhpd_cdev->is_pwr_domain_on = false;
> +       }
> +
> +       return ret;
> +}
> +
> +static struct thermal_cooling_device_ops rpmhpd_cooling_device_ops = {
> +       .get_max_state = rpmhpd_cdev_get_max_state,
> +       .get_cur_state = rpmhpd_cdev_get_cur_state,
> +       .set_cur_state = rpmhpd_cdev_set_cur_state,
> +};
> +
> +static int rpmhpd_cdev_probe(struct platform_device *pdev)
> +{
> +       struct device *dev = &pdev->dev, *pd_dev;
> +       struct rpmhpd_cooling_device *rpmhpd_cdev;
> +       const char *rpmhpd_cdev_name = of_device_get_match_data(dev);
> +       unsigned int count;
> +       int ret;
> +
> +       if (!dev->pm_domain) {
> +               pd_dev = dev_pm_domain_attach_by_name(dev, rpmhpd_cdev_name);

I don't think this is needed, unless you have multiple PM domains that
can be attached per device.

If there is only one PM domain the platform bus should already have
attached it at this point.

> +               if (IS_ERR(pd_dev))
> +                       return PTR_ERR(pd_dev);
> +       } else {
> +               pd_dev = dev;
> +       }
> +
> +       rpmhpd_cdev = devm_kzalloc(dev, sizeof(*rpmhpd_cdev), GFP_KERNEL);
> +       if (!rpmhpd_cdev) {
> +               ret = -ENOMEM;
> +               goto detach_pd;
> +       }
> +
> +       ret = dev_pm_genpd_performance_state_count(pd_dev, &count);
> +       if (ret)
> +               goto detach_pd;
> +
> +       rpmhpd_cdev->pwr_domain = pd_dev;
> +       rpmhpd_cdev->max_state = count - 1;
> +       rpmhpd_cdev->is_pwr_domain_on = false;
> +
> +       pm_runtime_enable(pd_dev);
> +
> +       rpmhpd_cdev->cdev = thermal_of_cooling_device_register
> +                                       (dev->of_node, rpmhpd_cdev_name,
> +                                        rpmhpd_cdev,
> +                                        &rpmhpd_cooling_device_ops);
> +       if (IS_ERR(rpmhpd_cdev->cdev)) {
> +               dev_err(dev, "unable to register %s cooling device\n",
> +                       rpmhpd_cdev_name);
> +               ret = PTR_ERR(rpmhpd_cdev->cdev);
> +               goto detach_pd;
> +       }
> +
> +       return 0;
> +
> +detach_pd:
> +       dev_pm_domain_detach(pd_dev, false);

Not needed, see above.

> +       return ret;
> +}
> +
> +static struct platform_driver rpmhpd_cdev_driver = {
> +       .driver = {
> +               .name = "qcom-rpmhpd-cdev",
> +               .of_match_table = rpmhpd_cdev_match_table,
> +       },
> +       .probe = rpmhpd_cdev_probe,

Looks like you should implement a ->remove() callback as well. Or
perhaps you think this should be a built-in-driver?

> +};
> +module_platform_driver(rpmhpd_cdev_driver);
> +
> +MODULE_DESCRIPTION("Qualcomm RPMHPD cooling device driver");
> +MODULE_LICENSE("GPL v2");
> --
> 2.1.4
>

Kind regards
Uffe

Re: [PATCH 3/4] thermal: qcom: Add RPMHPD cooling device driver.

[Thara Gopinath]

On 08/22/2019 11:19 AM, Ulf Hansson wrote:
> On Sat, 10 Aug 2019 at 02:58, Thara Gopinath <thara.gopinath@linaro.org> wrote:
>>
>> The MX power domain in RPMH can be used to warm the
>> the SoC in SDM845. To support this feature, introduce
>> a RPMH power domain cooling device driver that can be
>> plugged into the thermal framework.(The thermal framework
>> itself requires further modifiction to support a warming
>> device in place of a cooling device. Those extensions are
>> not introduced in this patch series).
>>
>> Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org>
>> ---
>>  drivers/thermal/qcom/Kconfig            |   7 ++
>>  drivers/thermal/qcom/Makefile           |   1 +
>>  drivers/thermal/qcom/qcom-rpmhpd-cdev.c | 141 ++++++++++++++++++++++++++++++++
>>  3 files changed, 149 insertions(+)
>>  create mode 100644 drivers/thermal/qcom/qcom-rpmhpd-cdev.c
>>
>> diff --git a/drivers/thermal/qcom/Kconfig b/drivers/thermal/qcom/Kconfig
>> index aa9c1d8..a540130 100644
>> --- a/drivers/thermal/qcom/Kconfig
>> +++ b/drivers/thermal/qcom/Kconfig
>> @@ -20,3 +20,10 @@ config QCOM_SPMI_TEMP_ALARM
>>           trip points. The temperature reported by the thermal sensor reflects the
>>           real time die temperature if an ADC is present or an estimate of the
>>           temperature based upon the over temperature stage value.
>> +
>> +config CONFIG_QCOM_RPMHPD_CDEV
>> +       tristate "Qualcomm RPMHPD based cooling device"
>> +       depends on QCOM_RPMHPD
>> +       help
>> +         This enables RPMHPD based cooling devices. On SDM845, this is
>> +         MX power domain.
>> diff --git a/drivers/thermal/qcom/Makefile b/drivers/thermal/qcom/Makefile
>> index 7c8dc6e..e4eb520 100644
>> --- a/drivers/thermal/qcom/Makefile
>> +++ b/drivers/thermal/qcom/Makefile
>> @@ -4,3 +4,4 @@ obj-$(CONFIG_QCOM_TSENS)        += qcom_tsens.o
>>  qcom_tsens-y                   += tsens.o tsens-common.o tsens-v0_1.o \
>>                                    tsens-8960.o tsens-v2.o tsens-v1.o
>>  obj-$(CONFIG_QCOM_SPMI_TEMP_ALARM)     += qcom-spmi-temp-alarm.o
>> +obj-$(CONFIG_QCOM_RPMHPD_CDEV)         += qcom-rpmhpd-cdev.o
>> diff --git a/drivers/thermal/qcom/qcom-rpmhpd-cdev.c b/drivers/thermal/qcom/qcom-rpmhpd-cdev.c
>> new file mode 100644
>> index 0000000..265523b
>> --- /dev/null
>> +++ b/drivers/thermal/qcom/qcom-rpmhpd-cdev.c
>> @@ -0,0 +1,141 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (c) 2019, Linaro Ltd
>> + */
>> +#include <linux/err.h>
>> +#include <linux/kernel.h>
>> +#include <linux/init.h>
>> +#include <linux/of_device.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/module.h>
>> +#include <linux/pm_domain.h>
>> +#include <linux/pm_runtime.h>
>> +#include <linux/thermal.h>
>> +
>> +struct rpmhpd_cooling_device {
>> +       struct thermal_cooling_device *cdev;
>> +       struct device *pwr_domain;
>> +       int max_state;
>> +       int cur_state;
>> +       bool is_pwr_domain_on;
>> +};
>> +
>> +static const char sdm845_rpmhpd_cdev_name[] = "mx";
>> +
>> +static const struct of_device_id rpmhpd_cdev_match_table[] = {
>> +       { .compatible = "qcom,sdm845-rpmhpd-cdev", .data = &sdm845_rpmhpd_cdev_name },
>> +       { }
>> +};
>> +MODULE_DEVICE_TABLE(of, rpmhpd_cdev_match_table);
>> +
>> +static int rpmhpd_cdev_get_max_state(struct thermal_cooling_device *cdev,
>> +                                    unsigned long *state)
>> +{
>> +       struct rpmhpd_cooling_device *rpmhpd_cdev = cdev->devdata;
>> +
>> +       *state = rpmhpd_cdev->max_state;
>> +       return 0;
>> +}
>> +
>> +static int rpmhpd_cdev_get_cur_state(struct thermal_cooling_device *cdev,
>> +                                    unsigned long *state)
>> +{
>> +       struct rpmhpd_cooling_device *rpmhpd_cdev = cdev->devdata;
>> +       int perf_state;
>> +
>> +       dev_pm_genpd_get_performance_state(rpmhpd_cdev->pwr_domain,
>> +                                          &perf_state);
>> +       *state = perf_state;
>> +       return 0;
>> +}
>> +
>> +static int rpmhpd_cdev_set_cur_state(struct thermal_cooling_device *cdev,
>> +                                    unsigned long state)
>> +{
>> +       struct rpmhpd_cooling_device *rpmhpd_cdev = cdev->devdata;
>> +       struct device *pwr_domain = rpmhpd_cdev->pwr_domain;
> 
> Nitpick: Using pwr_domain as the name of the variable is a bit
> confusing. Why not just name it "dev"?

Sure! I will change it.

> 
>> +       int ret;
>> +
>> +       ret = dev_pm_genpd_set_performance_state(pwr_domain, state);
>> +
>> +       if (ret)
>> +               return ret;
>> +
>> +       if (state && !rpmhpd_cdev->is_pwr_domain_on) {
> 
> Nitpick: To clarify, I would suggest to rename "is_pwr_domain_on" to
> "runtime_resumed".

Done!

> 
>> +               ret = pm_runtime_get_sync(pwr_domain);
>> +               rpmhpd_cdev->is_pwr_domain_on = true;
>> +       } else if (!state && rpmhpd_cdev->is_pwr_domain_on) {
>> +               ret = pm_runtime_put(pwr_domain);
>> +               rpmhpd_cdev->is_pwr_domain_on = false;
>> +       }
>> +
>> +       return ret;
>> +}
>> +
>> +static struct thermal_cooling_device_ops rpmhpd_cooling_device_ops = {
>> +       .get_max_state = rpmhpd_cdev_get_max_state,
>> +       .get_cur_state = rpmhpd_cdev_get_cur_state,
>> +       .set_cur_state = rpmhpd_cdev_set_cur_state,
>> +};
>> +
>> +static int rpmhpd_cdev_probe(struct platform_device *pdev)
>> +{
>> +       struct device *dev = &pdev->dev, *pd_dev;
>> +       struct rpmhpd_cooling_device *rpmhpd_cdev;
>> +       const char *rpmhpd_cdev_name = of_device_get_match_data(dev);
>> +       unsigned int count;
>> +       int ret;
>> +
>> +       if (!dev->pm_domain) {
>> +               pd_dev = dev_pm_domain_attach_by_name(dev, rpmhpd_cdev_name);
> 
> I don't think this is needed, unless you have multiple PM domains that
> can be attached per device.
> 
> If there is only one PM domain the platform bus should already have
> attached it at this point.

I agree. I realized after sending the patches out. I will remove this at
this point. If there is a need to have multiple power domains , we can
add it back later. There is more changes needed in the driver to support
multiple power domains anyways.

> 
>> +               if (IS_ERR(pd_dev))
>> +                       return PTR_ERR(pd_dev);
>> +       } else {
>> +               pd_dev = dev;
>> +       }
>> +
>> +       rpmhpd_cdev = devm_kzalloc(dev, sizeof(*rpmhpd_cdev), GFP_KERNEL);
>> +       if (!rpmhpd_cdev) {
>> +               ret = -ENOMEM;
>> +               goto detach_pd;
>> +       }
>> +
>> +       ret = dev_pm_genpd_performance_state_count(pd_dev, &count);
>> +       if (ret)
>> +               goto detach_pd;
>> +
>> +       rpmhpd_cdev->pwr_domain = pd_dev;
>> +       rpmhpd_cdev->max_state = count - 1;
>> +       rpmhpd_cdev->is_pwr_domain_on = false;
>> +
>> +       pm_runtime_enable(pd_dev);
>> +
>> +       rpmhpd_cdev->cdev = thermal_of_cooling_device_register
>> +                                       (dev->of_node, rpmhpd_cdev_name,
>> +                                        rpmhpd_cdev,
>> +                                        &rpmhpd_cooling_device_ops);
>> +       if (IS_ERR(rpmhpd_cdev->cdev)) {
>> +               dev_err(dev, "unable to register %s cooling device\n",
>> +                       rpmhpd_cdev_name);
>> +               ret = PTR_ERR(rpmhpd_cdev->cdev);
>> +               goto detach_pd;
>> +       }
>> +
>> +       return 0;
>> +
>> +detach_pd:
>> +       dev_pm_domain_detach(pd_dev, false);
> 
> Not needed, see above.
> 
>> +       return ret;
>> +}
>> +
>> +static struct platform_driver rpmhpd_cdev_driver = {
>> +       .driver = {
>> +               .name = "qcom-rpmhpd-cdev",
>> +               .of_match_table = rpmhpd_cdev_match_table,
>> +       },
>> +       .probe = rpmhpd_cdev_probe,
> 
> Looks like you should implement a ->remove() callback as well. Or
> perhaps you think this should be a built-in-driver?

Mmm.. I think I will add a remove.

Regards
Thara
> 
>> +};
>> +module_platform_driver(rpmhpd_cdev_driver);
>> +
>> +MODULE_DESCRIPTION("Qualcomm RPMHPD cooling device driver");
>> +MODULE_LICENSE("GPL v2");
>> --
>> 2.1.4
>>
> 
> Kind regards
> Uffe
> 


-- 
Regards
Thara

Re: [PATCH 3/4] thermal: qcom: Add RPMHPD cooling device driver.

[Bjorn Andersson]

On Fri 09 Aug 17:58 PDT 2019, Thara Gopinath wrote:

> The MX power domain in RPMH can be used to warm the
> the SoC in SDM845. To support this feature, introduce
> a RPMH power domain cooling device driver that can be
> plugged into the thermal framework.(The thermal framework
> itself requires further modifiction to support a warming
> device in place of a cooling device. Those extensions are
> not introduced in this patch series).
> 

This cooling device provides an interface to set a minimum state for a
power domain.  So while it's used for controlling the MX rail exposed by
the RPMh on some Qualcomm SoCs there's nothing Qualcomm/RPMh specific
with it.

Regards,
Bjorn

Re: [PATCH 3/4] thermal: qcom: Add RPMHPD cooling device driver.

[Thara Gopinath]

On 08/24/2019 02:10 AM, Bjorn Andersson wrote:
> On Fri 09 Aug 17:58 PDT 2019, Thara Gopinath wrote:
> 
>> The MX power domain in RPMH can be used to warm the
>> the SoC in SDM845. To support this feature, introduce
>> a RPMH power domain cooling device driver that can be
>> plugged into the thermal framework.(The thermal framework
>> itself requires further modifiction to support a warming
>> device in place of a cooling device. Those extensions are
>> not introduced in this patch series).
>>
> 
> This cooling device provides an interface to set a minimum state for a
> power domain.  So while it's used for controlling the MX rail exposed by
> the RPMh on some Qualcomm SoCs there's nothing Qualcomm/RPMh specific
> with it.
Hi Bjorn,

I agree that there is nothing Qualcomm specific. Are you suggesting a
more generic driver here ?

Regards
Thara

> 
> Regards,
> Bjorn
> 


-- 
Regards
Thara

Re: [PATCH 3/4] thermal: qcom: Add RPMHPD cooling device driver.

[Bjorn Andersson]

On Tue 27 Aug 03:42 PDT 2019, Thara Gopinath wrote:

> On 08/24/2019 02:10 AM, Bjorn Andersson wrote:
> > On Fri 09 Aug 17:58 PDT 2019, Thara Gopinath wrote:
> > 
> >> The MX power domain in RPMH can be used to warm the
> >> the SoC in SDM845. To support this feature, introduce
> >> a RPMH power domain cooling device driver that can be
> >> plugged into the thermal framework.(The thermal framework
> >> itself requires further modifiction to support a warming
> >> device in place of a cooling device. Those extensions are
> >> not introduced in this patch series).
> >>
> > 
> > This cooling device provides an interface to set a minimum state for a
> > power domain.  So while it's used for controlling the MX rail exposed by
> > the RPMh on some Qualcomm SoCs there's nothing Qualcomm/RPMh specific
> > with it.
> Hi Bjorn,
> 
> I agree that there is nothing Qualcomm specific. Are you suggesting a
> more generic driver here ?
> 

Yes, that's what I'm suggesting.

Regards,
Bjorn

Re: [PATCH 3/4] thermal: qcom: Add RPMHPD cooling device driver.

[Zhang Rui]

On Fri, 2019-08-09 at 20:58 -0400, Thara Gopinath wrote:
> The MX power domain in RPMH can be used to warm the
> the SoC in SDM845. To support this feature, introduce
> a RPMH power domain cooling device driver that can be
> plugged into the thermal framework.(The thermal framework
> itself requires further modifiction to support a warming
> device in place of a cooling device. Those extensions are
> not introduced in this patch series).

I'm wondering how this device is used, as well as the thermal extension
changes needed. To me, it's better to see this patch together with the
thermal extension changes.

thanks,
rui
> 
> Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org>
> ---
>  drivers/thermal/qcom/Kconfig            |   7 ++
>  drivers/thermal/qcom/Makefile           |   1 +
>  drivers/thermal/qcom/qcom-rpmhpd-cdev.c | 141
> ++++++++++++++++++++++++++++++++
>  3 files changed, 149 insertions(+)
>  create mode 100644 drivers/thermal/qcom/qcom-rpmhpd-cdev.c
> 
> diff --git a/drivers/thermal/qcom/Kconfig
> b/drivers/thermal/qcom/Kconfig
> index aa9c1d8..a540130 100644
> --- a/drivers/thermal/qcom/Kconfig
> +++ b/drivers/thermal/qcom/Kconfig
> @@ -20,3 +20,10 @@ config QCOM_SPMI_TEMP_ALARM
>  	  trip points. The temperature reported by the thermal sensor
> reflects the
>  	  real time die temperature if an ADC is present or an estimate
> of the
>  	  temperature based upon the over temperature stage value.
> +
> +config CONFIG_QCOM_RPMHPD_CDEV
> +	tristate "Qualcomm RPMHPD based cooling device"
> +	depends on QCOM_RPMHPD
> +	help
> +	  This enables RPMHPD based cooling devices. On SDM845, this is
> +	  MX power domain.
> diff --git a/drivers/thermal/qcom/Makefile
> b/drivers/thermal/qcom/Makefile
> index 7c8dc6e..e4eb520 100644
> --- a/drivers/thermal/qcom/Makefile
> +++ b/drivers/thermal/qcom/Makefile
> @@ -4,3 +4,4 @@ obj-$(CONFIG_QCOM_TSENS)	+= qcom_tsens.o
>  qcom_tsens-y			+= tsens.o tsens-common.o tsens-v0_1.o
> \
>  				   tsens-8960.o tsens-v2.o tsens-v1.o
>  obj-$(CONFIG_QCOM_SPMI_TEMP_ALARM)	+= qcom-spmi-temp-alarm.o
> +obj-$(CONFIG_QCOM_RPMHPD_CDEV)		+= qcom-rpmhpd-cdev.o
> diff --git a/drivers/thermal/qcom/qcom-rpmhpd-cdev.c
> b/drivers/thermal/qcom/qcom-rpmhpd-cdev.c
> new file mode 100644
> index 0000000..265523b
> --- /dev/null
> +++ b/drivers/thermal/qcom/qcom-rpmhpd-cdev.c
> @@ -0,0 +1,141 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2019, Linaro Ltd
> + */
> +#include <linux/err.h>
> +#include <linux/kernel.h>
> +#include <linux/init.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/module.h>
> +#include <linux/pm_domain.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/thermal.h>
> +
> +struct rpmhpd_cooling_device {
> +	struct thermal_cooling_device *cdev;
> +	struct device *pwr_domain;
> +	int max_state;
> +	int cur_state;
> +	bool is_pwr_domain_on;
> +};
> +
> +static const char sdm845_rpmhpd_cdev_name[] = "mx";
> +
> +static const struct of_device_id rpmhpd_cdev_match_table[] = {
> +	{ .compatible = "qcom,sdm845-rpmhpd-cdev", .data =
> &sdm845_rpmhpd_cdev_name },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, rpmhpd_cdev_match_table);
> +
> +static int rpmhpd_cdev_get_max_state(struct thermal_cooling_device
> *cdev,
> +				     unsigned long *state)
> +{
> +	struct rpmhpd_cooling_device *rpmhpd_cdev = cdev->devdata;
> +
> +	*state = rpmhpd_cdev->max_state;
> +	return 0;
> +}
> +
> +static int rpmhpd_cdev_get_cur_state(struct thermal_cooling_device
> *cdev,
> +				     unsigned long *state)
> +{
> +	struct rpmhpd_cooling_device *rpmhpd_cdev = cdev->devdata;
> +	int perf_state;
> +
> +	dev_pm_genpd_get_performance_state(rpmhpd_cdev->pwr_domain,
> +					   &perf_state);
> +	*state = perf_state;
> +	return 0;
> +}
> +
> +static int rpmhpd_cdev_set_cur_state(struct thermal_cooling_device
> *cdev,
> +				     unsigned long state)
> +{
> +	struct rpmhpd_cooling_device *rpmhpd_cdev = cdev->devdata;
> +	struct device *pwr_domain = rpmhpd_cdev->pwr_domain;
> +	int ret;
> +
> +	ret = dev_pm_genpd_set_performance_state(pwr_domain, state);
> +
> +	if (ret)
> +		return ret;
> +
> +	if (state && !rpmhpd_cdev->is_pwr_domain_on) {
> +		ret = pm_runtime_get_sync(pwr_domain);
> +		rpmhpd_cdev->is_pwr_domain_on = true;
> +	} else if (!state && rpmhpd_cdev->is_pwr_domain_on) {
> +		ret = pm_runtime_put(pwr_domain);
> +		rpmhpd_cdev->is_pwr_domain_on = false;
> +	}
> +
> +	return ret;
> +}
> +
> +static struct thermal_cooling_device_ops rpmhpd_cooling_device_ops =
> {
> +	.get_max_state = rpmhpd_cdev_get_max_state,
> +	.get_cur_state = rpmhpd_cdev_get_cur_state,
> +	.set_cur_state = rpmhpd_cdev_set_cur_state,
> +};
> +
> +static int rpmhpd_cdev_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev, *pd_dev;
> +	struct rpmhpd_cooling_device *rpmhpd_cdev;
> +	const char *rpmhpd_cdev_name = of_device_get_match_data(dev);
> +	unsigned int count;
> +	int ret;
> +
> +	if (!dev->pm_domain) {
> +		pd_dev = dev_pm_domain_attach_by_name(dev,
> rpmhpd_cdev_name);
> +		if (IS_ERR(pd_dev))
> +			return PTR_ERR(pd_dev);
> +	} else {
> +		pd_dev = dev;
> +	}
> +
> +	rpmhpd_cdev = devm_kzalloc(dev, sizeof(*rpmhpd_cdev),
> GFP_KERNEL);
> +	if (!rpmhpd_cdev) {
> +		ret = -ENOMEM;
> +		goto detach_pd;
> +	}
> +
> +	ret = dev_pm_genpd_performance_state_count(pd_dev, &count);
> +	if (ret)
> +		goto detach_pd;
> +
> +	rpmhpd_cdev->pwr_domain = pd_dev;
> +	rpmhpd_cdev->max_state = count - 1;
> +	rpmhpd_cdev->is_pwr_domain_on = false;
> +
> +	pm_runtime_enable(pd_dev);
> +
> +	rpmhpd_cdev->cdev = thermal_of_cooling_device_register
> +					(dev->of_node,
> rpmhpd_cdev_name,
> +					 rpmhpd_cdev,
> +					 &rpmhpd_cooling_device_ops);
> +	if (IS_ERR(rpmhpd_cdev->cdev)) {
> +		dev_err(dev, "unable to register %s cooling device\n",
> +			rpmhpd_cdev_name);
> +		ret = PTR_ERR(rpmhpd_cdev->cdev);
> +		goto detach_pd;
> +	}
> +
> +	return 0;
> +
> +detach_pd:
> +	dev_pm_domain_detach(pd_dev, false);
> +	return ret;
> +}
> +
> +static struct platform_driver rpmhpd_cdev_driver = {
> +	.driver = {
> +		.name = "qcom-rpmhpd-cdev",
> +		.of_match_table = rpmhpd_cdev_match_table,
> +	},
> +	.probe = rpmhpd_cdev_probe,
> +};
> +module_platform_driver(rpmhpd_cdev_driver);
> +
> +MODULE_DESCRIPTION("Qualcomm RPMHPD cooling device driver");
> +MODULE_LICENSE("GPL v2");

Re: [PATCH 3/4] thermal: qcom: Add RPMHPD cooling device driver.

[Thara Gopinath]

On 08/28/2019 08:23 AM, Zhang Rui wrote:
> On Fri, 2019-08-09 at 20:58 -0400, Thara Gopinath wrote:
>> The MX power domain in RPMH can be used to warm the
>> the SoC in SDM845. To support this feature, introduce
>> a RPMH power domain cooling device driver that can be
>> plugged into the thermal framework.(The thermal framework
>> itself requires further modifiction to support a warming
>> device in place of a cooling device. Those extensions are
>> not introduced in this patch series).
> 
> I'm wondering how this device is used, as well as the thermal extension
> changes needed. To me, it's better to see this patch together with the
> thermal extension changes.
Hello Rui,

I somehow missed this email. I am sorry about that.

IMHO, the thermal extensions, needed would be
1. Extend the dt-bindings either in the thermal-zone or in the trip
description to indicate whether a zone/trip is being monitored for
rising/falling temperature
2. Changes in thermal-core and the governors(starting with step-wise) to
monitor falling temperature as well.

There are comments for this patch series like fixing the genpd
extensions I have proposed and making the warming device more generic.
So I will post a v2 for this anyways.

I have some proto-type for the above mentioned extensions. I will post
them out asap.

Again, apologies for missing this email

Regards
Thara

> 
> thanks,
> rui
>>
>> Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org>
>> ---
>>  drivers/thermal/qcom/Kconfig            |   7 ++
>>  drivers/thermal/qcom/Makefile           |   1 +
>>  drivers/thermal/qcom/qcom-rpmhpd-cdev.c | 141
>> ++++++++++++++++++++++++++++++++
>>  3 files changed, 149 insertions(+)
>>  create mode 100644 drivers/thermal/qcom/qcom-rpmhpd-cdev.c
>>
>> diff --git a/drivers/thermal/qcom/Kconfig
>> b/drivers/thermal/qcom/Kconfig
>> index aa9c1d8..a540130 100644
>> --- a/drivers/thermal/qcom/Kconfig
>> +++ b/drivers/thermal/qcom/Kconfig
>> @@ -20,3 +20,10 @@ config QCOM_SPMI_TEMP_ALARM
>>  	  trip points. The temperature reported by the thermal sensor
>> reflects the
>>  	  real time die temperature if an ADC is present or an estimate
>> of the
>>  	  temperature based upon the over temperature stage value.
>> +
>> +config CONFIG_QCOM_RPMHPD_CDEV
>> +	tristate "Qualcomm RPMHPD based cooling device"
>> +	depends on QCOM_RPMHPD
>> +	help
>> +	  This enables RPMHPD based cooling devices. On SDM845, this is
>> +	  MX power domain.
>> diff --git a/drivers/thermal/qcom/Makefile
>> b/drivers/thermal/qcom/Makefile
>> index 7c8dc6e..e4eb520 100644
>> --- a/drivers/thermal/qcom/Makefile
>> +++ b/drivers/thermal/qcom/Makefile
>> @@ -4,3 +4,4 @@ obj-$(CONFIG_QCOM_TSENS)	+= qcom_tsens.o
>>  qcom_tsens-y			+= tsens.o tsens-common.o tsens-v0_1.o
>> \
>>  				   tsens-8960.o tsens-v2.o tsens-v1.o
>>  obj-$(CONFIG_QCOM_SPMI_TEMP_ALARM)	+= qcom-spmi-temp-alarm.o
>> +obj-$(CONFIG_QCOM_RPMHPD_CDEV)		+= qcom-rpmhpd-cdev.o
>> diff --git a/drivers/thermal/qcom/qcom-rpmhpd-cdev.c
>> b/drivers/thermal/qcom/qcom-rpmhpd-cdev.c
>> new file mode 100644
>> index 0000000..265523b
>> --- /dev/null
>> +++ b/drivers/thermal/qcom/qcom-rpmhpd-cdev.c
>> @@ -0,0 +1,141 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (c) 2019, Linaro Ltd
>> + */
>> +#include <linux/err.h>
>> +#include <linux/kernel.h>
>> +#include <linux/init.h>
>> +#include <linux/of_device.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/module.h>
>> +#include <linux/pm_domain.h>
>> +#include <linux/pm_runtime.h>
>> +#include <linux/thermal.h>
>> +
>> +struct rpmhpd_cooling_device {
>> +	struct thermal_cooling_device *cdev;
>> +	struct device *pwr_domain;
>> +	int max_state;
>> +	int cur_state;
>> +	bool is_pwr_domain_on;
>> +};
>> +
>> +static const char sdm845_rpmhpd_cdev_name[] = "mx";
>> +
>> +static const struct of_device_id rpmhpd_cdev_match_table[] = {
>> +	{ .compatible = "qcom,sdm845-rpmhpd-cdev", .data =
>> &sdm845_rpmhpd_cdev_name },
>> +	{ }
>> +};
>> +MODULE_DEVICE_TABLE(of, rpmhpd_cdev_match_table);
>> +
>> +static int rpmhpd_cdev_get_max_state(struct thermal_cooling_device
>> *cdev,
>> +				     unsigned long *state)
>> +{
>> +	struct rpmhpd_cooling_device *rpmhpd_cdev = cdev->devdata;
>> +
>> +	*state = rpmhpd_cdev->max_state;
>> +	return 0;
>> +}
>> +
>> +static int rpmhpd_cdev_get_cur_state(struct thermal_cooling_device
>> *cdev,
>> +				     unsigned long *state)
>> +{
>> +	struct rpmhpd_cooling_device *rpmhpd_cdev = cdev->devdata;
>> +	int perf_state;
>> +
>> +	dev_pm_genpd_get_performance_state(rpmhpd_cdev->pwr_domain,
>> +					   &perf_state);
>> +	*state = perf_state;
>> +	return 0;
>> +}
>> +
>> +static int rpmhpd_cdev_set_cur_state(struct thermal_cooling_device
>> *cdev,
>> +				     unsigned long state)
>> +{
>> +	struct rpmhpd_cooling_device *rpmhpd_cdev = cdev->devdata;
>> +	struct device *pwr_domain = rpmhpd_cdev->pwr_domain;
>> +	int ret;
>> +
>> +	ret = dev_pm_genpd_set_performance_state(pwr_domain, state);
>> +
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (state && !rpmhpd_cdev->is_pwr_domain_on) {
>> +		ret = pm_runtime_get_sync(pwr_domain);
>> +		rpmhpd_cdev->is_pwr_domain_on = true;
>> +	} else if (!state && rpmhpd_cdev->is_pwr_domain_on) {
>> +		ret = pm_runtime_put(pwr_domain);
>> +		rpmhpd_cdev->is_pwr_domain_on = false;
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>> +static struct thermal_cooling_device_ops rpmhpd_cooling_device_ops =
>> {
>> +	.get_max_state = rpmhpd_cdev_get_max_state,
>> +	.get_cur_state = rpmhpd_cdev_get_cur_state,
>> +	.set_cur_state = rpmhpd_cdev_set_cur_state,
>> +};
>> +
>> +static int rpmhpd_cdev_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev, *pd_dev;
>> +	struct rpmhpd_cooling_device *rpmhpd_cdev;
>> +	const char *rpmhpd_cdev_name = of_device_get_match_data(dev);
>> +	unsigned int count;
>> +	int ret;
>> +
>> +	if (!dev->pm_domain) {
>> +		pd_dev = dev_pm_domain_attach_by_name(dev,
>> rpmhpd_cdev_name);
>> +		if (IS_ERR(pd_dev))
>> +			return PTR_ERR(pd_dev);
>> +	} else {
>> +		pd_dev = dev;
>> +	}
>> +
>> +	rpmhpd_cdev = devm_kzalloc(dev, sizeof(*rpmhpd_cdev),
>> GFP_KERNEL);
>> +	if (!rpmhpd_cdev) {
>> +		ret = -ENOMEM;
>> +		goto detach_pd;
>> +	}
>> +
>> +	ret = dev_pm_genpd_performance_state_count(pd_dev, &count);
>> +	if (ret)
>> +		goto detach_pd;
>> +
>> +	rpmhpd_cdev->pwr_domain = pd_dev;
>> +	rpmhpd_cdev->max_state = count - 1;
>> +	rpmhpd_cdev->is_pwr_domain_on = false;
>> +
>> +	pm_runtime_enable(pd_dev);
>> +
>> +	rpmhpd_cdev->cdev = thermal_of_cooling_device_register
>> +					(dev->of_node,
>> rpmhpd_cdev_name,
>> +					 rpmhpd_cdev,
>> +					 &rpmhpd_cooling_device_ops);
>> +	if (IS_ERR(rpmhpd_cdev->cdev)) {
>> +		dev_err(dev, "unable to register %s cooling device\n",
>> +			rpmhpd_cdev_name);
>> +		ret = PTR_ERR(rpmhpd_cdev->cdev);
>> +		goto detach_pd;
>> +	}
>> +
>> +	return 0;
>> +
>> +detach_pd:
>> +	dev_pm_domain_detach(pd_dev, false);
>> +	return ret;
>> +}
>> +
>> +static struct platform_driver rpmhpd_cdev_driver = {
>> +	.driver = {
>> +		.name = "qcom-rpmhpd-cdev",
>> +		.of_match_table = rpmhpd_cdev_match_table,
>> +	},
>> +	.probe = rpmhpd_cdev_probe,
>> +};
>> +module_platform_driver(rpmhpd_cdev_driver);
>> +
>> +MODULE_DESCRIPTION("Qualcomm RPMHPD cooling device driver");
>> +MODULE_LICENSE("GPL v2");
> 


-- 
Regards
Thara

[PATCH 4/4] arm64: dts: qcom: Extend AOSS RPMHPD node

[Thara Gopinath]

RPMHPD hosts resources that can be used to warm up
Add nodes for these resources for sdm845 (mx power
domain).

Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org>
---
 arch/arm64/boot/dts/qcom/sdm845.dtsi | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
index d0c0d4f..c5b8459 100644
--- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
@@ -2602,6 +2602,13 @@
 				compatible = "qcom,sdm845-rsc-hlos";
 				#interconnect-cells = <1>;
 			};
+
+			rpmh_mx_cdev: mx {
+				compatible = "qcom,sdm845-rpmhpd-cdev";
+				#cooling-cells = <2>;
+				power-domains =  <&rpmhpd SDM845_MX>;
+				power-domain-names = "mx";
+			};
 		};
 
 		intc: interrupt-controller@17a00000 {
-- 
2.1.4

Re: [PATCH 0/4] qcom: Model RPMH power domains as thermal cooling devices

[Daniel Lezcano]

Hi Thara,

interesting series. Can you describe what use case this series will solve?

On 10/08/2019 02:58, Thara Gopinath wrote:
> Certain RPMH power domains can be used to warm up the SoC (mx on sdm845)
> if the temperature falls below certain threshold. 

What is the relationship between the temperature fall, the sensor(s)
location and the warming device(s) in this case?


> These power domains
> can be considered as thermal warming devices
> (opposite of thermal cooling devices).

Is it possible to elaborate how works the RPMH as a warming device and
what is the "mx on sdm845"?

> In kernel, these warming devices can be modeled as a 
> thermal cooling device. To use these power domains as warming devices
> require further tweaks in the thermal framework which are out of scope
> of this patch series.
> 
> The first patch in this series extends the genpd framework to export out
> the performance states of a power domain so that when the RPMH power
> domain is modeled as a cooling device, the number of possible states and
> current state of the cooling device can be retrieved from the genpd
> framework.
> 
> The second patch implements the newly added genpd callback for RPMH power
> domain driver.
> 
> The third patch implements the modeling of a RPMH power domain as
> a cooling device and the final patch adds the device node entry for sdm845
> to consider RPMHPD MX a cooling device.
> 
> Thara Gopinath (4):
>   PM/Domains: Add support for retrieving genpd performance states
>     information
>   soc: qcom: rpmhpd: Introduce function to retrieve power domain
>     performance state count
>   thermal: qcom: Add RPMHPD cooling device driver.
>   arm64: dts: qcom: Extend AOSS RPMHPD node
> 
>  arch/arm64/boot/dts/qcom/sdm845.dtsi    |   7 ++
>  drivers/base/power/domain.c             |  38 +++++++++
>  drivers/soc/qcom/rpmhpd.c               |  11 +++
>  drivers/thermal/qcom/Kconfig            |   7 ++
>  drivers/thermal/qcom/Makefile           |   1 +
>  drivers/thermal/qcom/qcom-rpmhpd-cdev.c | 141 ++++++++++++++++++++++++++++++++
>  include/linux/pm_domain.h               |  18 ++++
>  7 files changed, 223 insertions(+)
>  create mode 100644 drivers/thermal/qcom/qcom-rpmhpd-cdev.c
> 


-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 0/4] qcom: Model RPMH power domains as thermal cooling devices

[Thara Gopinath]

On 08/14/2019 06:52 AM, Daniel Lezcano wrote:
> 
> Hi Thara
> 
> interesting series. Can you describe what use case this series will solve?
> 
> On 10/08/2019 02:58, Thara Gopinath wrote:
>> Certain RPMH power domains can be used to warm up the SoC (mx on sdm845)
>> if the temperature falls below certain threshold. 
> 
> What is the relationship between the temperature fall, the sensor(s)
> location and the warming device(s) in this case?
Hi Daniel,

Thanks for the review!

My understanding is that there are a bunch of hot-spots. If the
temperature sensors in any of these hot-spots, detect a fall
in temperature, a bunch of resources are used to warm up the entire Soc.
In this case a power domain controlled by the resource power manager
hardened (RPMh) is one of these resources used to warm up the Soc.

> 
>> These power domains
>> can be considered as thermal warming devices
>> (opposite of thermal cooling devices).
> 
> Is it possible to elaborate how works the RPMH as a warming device and
> what is the "mx on sdm845"?
RPMh is resource power manager hardened. It takes numbers between 0-15
to set the power domains to require states (like TURBO, NOMINAL etc).
The frequency and voltage of the domains are controlled by the hardware
based on the number(the number of states supported by a power domains
are in fact read from the h/w runtime).
So MX in one such power domain controlled by RPMh on sdm845 which is
used to warm up the SoC as well. MX is modeled as a regular power domain
in the linux-kernel (registered with genpd).

Regards
Thara
> 
>> In kernel, these warming devices can be modeled as a 
>> thermal cooling device. To use these power domains as warming devices
>> require further tweaks in the thermal framework which are out of scope
>> of this patch series.
>>
>> The first patch in this series extends the genpd framework to export out
>> the performance states of a power domain so that when the RPMH power
>> domain is modeled as a cooling device, the number of possible states and
>> current state of the cooling device can be retrieved from the genpd
>> framework.
>>
>> The second patch implements the newly added genpd callback for RPMH power
>> domain driver.
>>
>> The third patch implements the modeling of a RPMH power domain as
>> a cooling device and the final patch adds the device node entry for sdm845
>> to consider RPMHPD MX a cooling device.
>>
>> Thara Gopinath (4):
>>   PM/Domains: Add support for retrieving genpd performance states
>>     information
>>   soc: qcom: rpmhpd: Introduce function to retrieve power domain
>>     performance state count
>>   thermal: qcom: Add RPMHPD cooling device driver.
>>   arm64: dts: qcom: Extend AOSS RPMHPD node
>>
>>  arch/arm64/boot/dts/qcom/sdm845.dtsi    |   7 ++
>>  drivers/base/power/domain.c             |  38 +++++++++
>>  drivers/soc/qcom/rpmhpd.c               |  11 +++
>>  drivers/thermal/qcom/Kconfig            |   7 ++
>>  drivers/thermal/qcom/Makefile           |   1 +
>>  drivers/thermal/qcom/qcom-rpmhpd-cdev.c | 141 ++++++++++++++++++++++++++++++++
>>  include/linux/pm_domain.h               |  18 ++++
>>  7 files changed, 223 insertions(+)
>>  create mode 100644 drivers/thermal/qcom/qcom-rpmhpd-cdev.c
>>
> 
> 


-- 
Regards
Thara

Re: [PATCH 0/4] qcom: Model RPMH power domains as thermal cooling devices

[Bjorn Andersson]

On Wed 14 Aug 03:52 PDT 2019, Daniel Lezcano wrote:

> 
> Hi Thara,
> 
> interesting series. Can you describe what use case this series will solve?
> 

The purpose is to ensure a minimum voltage (voltage corner) on the
memory rail (mx) at low temperature, to meet some physical requirement.

> On 10/08/2019 02:58, Thara Gopinath wrote:
> > Certain RPMH power domains can be used to warm up the SoC (mx on sdm845)
> > if the temperature falls below certain threshold. 
> 
> What is the relationship between the temperature fall, the sensor(s)
> location and the warming device(s) in this case?
> 

Presumably any on-die sensor could be used and the minimum voltage
requirement only applies below some configurable threshold.

> 
> > These power domains
> > can be considered as thermal warming devices
> > (opposite of thermal cooling devices).
> 
> Is it possible to elaborate how works the RPMH as a warming device and
> what is the "mx on sdm845"?
> 

RPMh is the interface used to configure the voltage corner (and other
shared resources, such as regulators, interconnects etc). mx is the
"memory rail" power domain.

Regards,
Bjorn