[v5,4/6] soc: qcom: Extend RPMh power controller driver to register warming devices.

RPMh power control hosts power domains that can be used as
thermal warming devices. Register these power domains
with the generic power domain warming device thermal framework.

Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org>
---

v3->v4:
	- Introduce a boolean value is_warming_dev in rpmhpd structure to
	  indicate if a generic power domain can be used as a warming
	  device or not.With this change, device tree no longer has to
	  specify which power domain inside the rpmh power domain provider
	  is a warming device.
	- Move registering of warming devices into a late initcall to
	  ensure that warming devices are registered after thermal
	  framework is initialized.

 drivers/soc/qcom/rpmhpd.c | 37 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 36 insertions(+), 1 deletion(-)

On Thu 19 Mar 18:41 PDT 2020, Thara Gopinath wrote:

> RPMh power control hosts power domains that can be used as
> thermal warming devices. Register these power domains
> with the generic power domain warming device thermal framework.
> 
> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
> Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org>
> ---
> 
> v3->v4:
> 	- Introduce a boolean value is_warming_dev in rpmhpd structure to
> 	  indicate if a generic power domain can be used as a warming
> 	  device or not.With this change, device tree no longer has to
> 	  specify which power domain inside the rpmh power domain provider
> 	  is a warming device.
> 	- Move registering of warming devices into a late initcall to
> 	  ensure that warming devices are registered after thermal
> 	  framework is initialized.

This information is lost when we merge patches, as such I would like
such design decisions to be described in the commit message itself.
But...

> 
>  drivers/soc/qcom/rpmhpd.c | 37 ++++++++++++++++++++++++++++++++++++-
>  1 file changed, 36 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/soc/qcom/rpmhpd.c b/drivers/soc/qcom/rpmhpd.c
> index 7142409a3b77..4e9c0bbb8826 100644
> --- a/drivers/soc/qcom/rpmhpd.c
> +++ b/drivers/soc/qcom/rpmhpd.c
> @@ -11,6 +11,7 @@
>  #include <linux/of_device.h>
>  #include <linux/platform_device.h>
>  #include <linux/pm_opp.h>
> +#include <linux/pd_warming.h>
>  #include <soc/qcom/cmd-db.h>
>  #include <soc/qcom/rpmh.h>
>  #include <dt-bindings/power/qcom-rpmpd.h>
> @@ -48,6 +49,7 @@ struct rpmhpd {
>  	bool		enabled;
>  	const char	*res_name;
>  	u32		addr;
> +	bool		is_warming_dev;
>  };
>  
>  struct rpmhpd_desc {
> @@ -55,6 +57,8 @@ struct rpmhpd_desc {
>  	size_t num_pds;
>  };
>  
> +const struct rpmhpd_desc *global_desc;
> +
>  static DEFINE_MUTEX(rpmhpd_lock);
>  
>  /* SDM845 RPMH powerdomains */
> @@ -89,6 +93,7 @@ static struct rpmhpd sdm845_mx = {
>  	.pd = { .name = "mx", },
>  	.peer = &sdm845_mx_ao,
>  	.res_name = "mx.lvl",
> +	.is_warming_dev = true,
>  };
>  
>  static struct rpmhpd sdm845_mx_ao = {
> @@ -452,7 +457,14 @@ static int rpmhpd_probe(struct platform_device *pdev)
>  					       &rpmhpds[i]->pd);
>  	}
>  
> -	return of_genpd_add_provider_onecell(pdev->dev.of_node, data);
> +	ret = of_genpd_add_provider_onecell(pdev->dev.of_node, data);
> +
> +	if (ret)
> +		return ret;
> +
> +	global_desc = desc;
> +
> +	return 0;
>  }
>  
>  static struct platform_driver rpmhpd_driver = {
> @@ -469,3 +481,26 @@ static int __init rpmhpd_init(void)
>  	return platform_driver_register(&rpmhpd_driver);
>  }
>  core_initcall(rpmhpd_init);
> +
> +static int __init rpmhpd_init_warming_device(void)
> +{
> +	size_t num_pds;
> +	struct rpmhpd **rpmhpds;
> +	int i;
> +
> +	if (!global_desc)
> +		return -EINVAL;
> +
> +	rpmhpds = global_desc->rpmhpds;
> +	num_pds = global_desc->num_pds;
> +
> +	if (!of_find_property(rpmhpds[0]->dev->of_node, "#cooling-cells", NULL))
> +		return 0;
> +
> +	for (i = 0; i < num_pds; i++)
> +		if (rpmhpds[i]->is_warming_dev)
> +			of_pd_warming_register(rpmhpds[i]->dev, i);
> +
> +	return 0;
> +}
> +late_initcall(rpmhpd_init_warming_device);

...why can't this be done in rpmhpd_probe()?

In particular with the recent patches from John Stultz to allow rpmhpd
to be built as a module I don't think there's any guarantees that
rpmh_probe() will have succeeded before rpmhpd_init_warming_device()
executes.

Regards,
Bjorn

On 3/27/20 6:53 PM, Bjorn Andersson wrote:
> On Thu 19 Mar 18:41 PDT 2020, Thara Gopinath wrote:
> 
>> RPMh power control hosts power domains that can be used as
>> thermal warming devices. Register these power domains
>> with the generic power domain warming device thermal framework.
>>
>> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
>> Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org>
>> ---
>>
>> v3->v4:
>> 	- Introduce a boolean value is_warming_dev in rpmhpd structure to
>> 	  indicate if a generic power domain can be used as a warming
>> 	  device or not.With this change, device tree no longer has to
>> 	  specify which power domain inside the rpmh power domain provider
>> 	  is a warming device.
>> 	- Move registering of warming devices into a late initcall to
>> 	  ensure that warming devices are registered after thermal
>> 	  framework is initialized.
> 
> This information is lost when we merge patches, as such I would like
> such design decisions to be described in the commit message itself.
> But...
> 
>>
>>   drivers/soc/qcom/rpmhpd.c | 37 ++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 36 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/soc/qcom/rpmhpd.c b/drivers/soc/qcom/rpmhpd.c
>> index 7142409a3b77..4e9c0bbb8826 100644
>> --- a/drivers/soc/qcom/rpmhpd.c
>> +++ b/drivers/soc/qcom/rpmhpd.c
>> @@ -11,6 +11,7 @@
>>   #include <linux/of_device.h>
>>   #include <linux/platform_device.h>
>>   #include <linux/pm_opp.h>
>> +#include <linux/pd_warming.h>
>>   #include <soc/qcom/cmd-db.h>
>>   #include <soc/qcom/rpmh.h>
>>   #include <dt-bindings/power/qcom-rpmpd.h>
>> @@ -48,6 +49,7 @@ struct rpmhpd {
>>   	bool		enabled;
>>   	const char	*res_name;
>>   	u32		addr;
>> +	bool		is_warming_dev;
>>   };
>>   
>>   struct rpmhpd_desc {
>> @@ -55,6 +57,8 @@ struct rpmhpd_desc {
>>   	size_t num_pds;
>>   };
>>   
>> +const struct rpmhpd_desc *global_desc;
>> +
>>   static DEFINE_MUTEX(rpmhpd_lock);
>>   
>>   /* SDM845 RPMH powerdomains */
>> @@ -89,6 +93,7 @@ static struct rpmhpd sdm845_mx = {
>>   	.pd = { .name = "mx", },
>>   	.peer = &sdm845_mx_ao,
>>   	.res_name = "mx.lvl",
>> +	.is_warming_dev = true,
>>   };
>>   
>>   static struct rpmhpd sdm845_mx_ao = {
>> @@ -452,7 +457,14 @@ static int rpmhpd_probe(struct platform_device *pdev)
>>   					       &rpmhpds[i]->pd);
>>   	}
>>   
>> -	return of_genpd_add_provider_onecell(pdev->dev.of_node, data);
>> +	ret = of_genpd_add_provider_onecell(pdev->dev.of_node, data);
>> +
>> +	if (ret)
>> +		return ret;
>> +
>> +	global_desc = desc;
>> +
>> +	return 0;
>>   }
>>   
>>   static struct platform_driver rpmhpd_driver = {
>> @@ -469,3 +481,26 @@ static int __init rpmhpd_init(void)
>>   	return platform_driver_register(&rpmhpd_driver);
>>   }
>>   core_initcall(rpmhpd_init);
>> +
>> +static int __init rpmhpd_init_warming_device(void)
>> +{
>> +	size_t num_pds;
>> +	struct rpmhpd **rpmhpds;
>> +	int i;
>> +
>> +	if (!global_desc)
>> +		return -EINVAL;
>> +
>> +	rpmhpds = global_desc->rpmhpds;
>> +	num_pds = global_desc->num_pds;
>> +
>> +	if (!of_find_property(rpmhpds[0]->dev->of_node, "#cooling-cells", NULL))
>> +		return 0;
>> +
>> +	for (i = 0; i < num_pds; i++)
>> +		if (rpmhpds[i]->is_warming_dev)
>> +			of_pd_warming_register(rpmhpds[i]->dev, i);
>> +
>> +	return 0;
>> +}
>> +late_initcall(rpmhpd_init_warming_device);
> 
> ...why can't this be done in rpmhpd_probe()?
> 
> In particular with the recent patches from John Stultz to allow rpmhpd
> to be built as a module I don't think there's any guarantees that
> rpmh_probe() will have succeeded before rpmhpd_init_warming_device()
> executes.

It is to take care of boot order.
So this has to happen after the thermal framework is initialized. 
Thermal framework is initialized with core_initcall. Can I move the 
rpmhpd init as a postcore_initcall ? Then I can get rid of this separate 
function and keep it as part of probe.

> 
> Regards,
> Bjorn
>

On Mon 30 Mar 07:53 PDT 2020, Thara Gopinath wrote:
> On 3/27/20 6:53 PM, Bjorn Andersson wrote:
> > On Thu 19 Mar 18:41 PDT 2020, Thara Gopinath wrote:
[..]
> > > +static int __init rpmhpd_init_warming_device(void)
> > > +{
> > > +	size_t num_pds;
> > > +	struct rpmhpd **rpmhpds;
> > > +	int i;
> > > +
> > > +	if (!global_desc)
> > > +		return -EINVAL;
> > > +
> > > +	rpmhpds = global_desc->rpmhpds;
> > > +	num_pds = global_desc->num_pds;
> > > +
> > > +	if (!of_find_property(rpmhpds[0]->dev->of_node, "#cooling-cells", NULL))
> > > +		return 0;
> > > +
> > > +	for (i = 0; i < num_pds; i++)
> > > +		if (rpmhpds[i]->is_warming_dev)
> > > +			of_pd_warming_register(rpmhpds[i]->dev, i);
> > > +
> > > +	return 0;
> > > +}
> > > +late_initcall(rpmhpd_init_warming_device);
> > 
> > ...why can't this be done in rpmhpd_probe()?
> > 
> > In particular with the recent patches from John Stultz to allow rpmhpd
> > to be built as a module I don't think there's any guarantees that
> > rpmh_probe() will have succeeded before rpmhpd_init_warming_device()
> > executes.
> 
> It is to take care of boot order.

Understood.

> So this has to happen after the thermal framework is initialized. Thermal
> framework is initialized with core_initcall. Can I move the rpmhpd init as a
> postcore_initcall ? Then I can get rid of this separate function and keep it
> as part of probe.
> 

So I presume the problem is that if this is called from probe, you might
of_pd_warming_register(), which ends up in
__thermal_cooling_device_register() before thermal_init() has been
invoked? 

Which is bad because e.g. thermal_class is not yet initialized.


I don't want to rely on the order of initcalls for things to work, so
could we make this more robust by having
thermal_of_cooling_device_register() return -EPROBE_DEFER is
thermal_init() isn't done?

Regards,
Bjorn