[v4,4/7] thermal: Add generic power domain warming device driver.

Commit Message

Thara Gopinath Nov. 20, 2019, 12:56 p.m. UTC

Resources modeled as power domains in linux kenrel can  be used to warm the
SoC(eg. mx power domain on sdm845).  To support this feature, introduce a
generic power domain warming 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 <[e-mail]>
---
v3->v4:
	- Removed late_init hook pd_warming_device_ops.
	- Use of_genpd_add_device instead of pm_genpd_add_device to attach
	  device to the generic power domain.
	- Use thermal_of_cooling_device_parent_register to register the
	  cooling device so that the device with genpd attached can be
	  made parent of the cooling device.
	- With above changes, remove reference to generic_pm_domain in
	  pd_warming_device.

 drivers/thermal/Kconfig              |  10 +++
 drivers/thermal/Makefile             |   2 +
 drivers/thermal/pwr_domain_warming.c | 138 +++++++++++++++++++++++++++++++++++
 include/linux/pwr_domain_warming.h   |  29 ++++++++
 4 files changed, 179 insertions(+)
 create mode 100644 drivers/thermal/pwr_domain_warming.c
 create mode 100644 include/linux/pwr_domain_warming.h

Comments

Ulf Hansson Feb. 4, 2020, 4:54 p.m. UTC | #1

On Wed, 20 Nov 2019 at 13:56, Thara Gopinath <[e-mail]> wrote:
>
> Resources modeled as power domains in linux kenrel can  be used to warm the
> SoC(eg. mx power domain on sdm845).  To support this feature, introduce a
> generic power domain warming 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 <[e-mail]>
> ---
> v3->v4:
>         - Removed late_init hook pd_warming_device_ops.
>         - Use of_genpd_add_device instead of pm_genpd_add_device to attach
>           device to the generic power domain.
>         - Use thermal_of_cooling_device_parent_register to register the
>           cooling device so that the device with genpd attached can be
>           made parent of the cooling device.
>         - With above changes, remove reference to generic_pm_domain in
>           pd_warming_device.
>
>  drivers/thermal/Kconfig              |  10 +++
>  drivers/thermal/Makefile             |   2 +
>  drivers/thermal/pwr_domain_warming.c | 138 +++++++++++++++++++++++++++++++++++
>  include/linux/pwr_domain_warming.h   |  29 ++++++++

Not sure about what the thermal maintainers think about the naming
here. In the end, it's their call.

However, normally we use "pm_domain_*", rather than "pwr_domain_*",
but maybe just "pd_*" is sufficient here.

>  4 files changed, 179 insertions(+)
>  create mode 100644 drivers/thermal/pwr_domain_warming.c
>  create mode 100644 include/linux/pwr_domain_warming.h
>
> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
> index 001a21a..0c5c93e 100644
> --- a/drivers/thermal/Kconfig
> +++ b/drivers/thermal/Kconfig
> @@ -187,6 +187,16 @@ config DEVFREQ_THERMAL
>
>           If you want this support, you should say Y here.
>
> +config PWR_DOMAIN_WARMING_THERMAL
> +       bool "Power Domain based warming device"
> +       depends on PM_GENERIC_DOMAINS_OF
> +       help
> +         This implements the generic power domain based warming
> +         mechanism through increasing the performance state of
> +         a power domain.
> +
> +         If you want this support, you should say Y here.
> +
>  config THERMAL_EMULATION
>         bool "Thermal emulation mode support"
>         help
> diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
> index 74a37c7..382c64a 100644
> --- a/drivers/thermal/Makefile
> +++ b/drivers/thermal/Makefile
> @@ -27,6 +27,8 @@ thermal_sys-$(CONFIG_CLOCK_THERMAL)   += clock_cooling.o
>  # devfreq cooling
>  thermal_sys-$(CONFIG_DEVFREQ_THERMAL) += devfreq_cooling.o
>
> +thermal_sys-$(CONFIG_PWR_DOMAIN_WARMING_THERMAL)       += pwr_domain_warming.o
> +
>  # platform thermal drivers
>  obj-y                          += broadcom/
>  obj-$(CONFIG_THERMAL_MMIO)             += thermal_mmio.o
> diff --git a/drivers/thermal/pwr_domain_warming.c b/drivers/thermal/pwr_domain_warming.c
> new file mode 100644
> index 0000000..40162b9
> --- /dev/null
> +++ b/drivers/thermal/pwr_domain_warming.c
> @@ -0,0 +1,138 @@
> +// 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_runtime.h>
> +#include <linux/slab.h>
> +#include <linux/pwr_domain_warming.h>
> +
> +struct pd_warming_device {
> +       struct thermal_cooling_device *cdev;
> +       struct device dev;
> +       int max_state;
> +       int cur_state;
> +       bool runtime_resumed;
> +};
> +
> +static int pd_wdev_get_max_state(struct thermal_cooling_device *cdev,
> +                                unsigned long *state)
> +{
> +       struct pd_warming_device *pd_wdev = cdev->devdata;
> +
> +       *state = pd_wdev->max_state;
> +       return 0;
> +}
> +
> +static int pd_wdev_get_cur_state(struct thermal_cooling_device *cdev,
> +                                unsigned long *state)
> +{
> +       struct pd_warming_device *pd_wdev = cdev->devdata;
> +
> +       *state = dev_pm_genpd_get_performance_state(&pd_wdev->dev);
> +
> +       return 0;
> +}
> +
> +static int pd_wdev_set_cur_state(struct thermal_cooling_device *cdev,
> +                                unsigned long state)
> +{
> +       struct pd_warming_device *pd_wdev = cdev->devdata;
> +       struct device *dev = &pd_wdev->dev;
> +       int ret;
> +
> +       ret = dev_pm_genpd_set_performance_state(dev, state);
> +
> +       if (ret)
> +               return ret;
> +
> +       if (state && !pd_wdev->runtime_resumed) {
> +               ret = pm_runtime_get_sync(dev);
> +               pd_wdev->runtime_resumed = true;
> +       } else if (!state && pd_wdev->runtime_resumed) {
> +               ret = pm_runtime_put(dev);
> +               pd_wdev->runtime_resumed = false;
> +       }
> +
> +       return ret;
> +}
> +
> +static struct thermal_cooling_device_ops pd_warming_device_ops = {
> +       .get_max_state  = pd_wdev_get_max_state,
> +       .get_cur_state  = pd_wdev_get_cur_state,
> +       .set_cur_state  = pd_wdev_set_cur_state,
> +};
> +
> +struct thermal_cooling_device *
> +pwr_domain_warming_register(struct device *parent, char *pd_name, int pd_id)

Maybe rename this to: thermal_of_pd_warming_register()

Moreover, I think you could replace the "struct device *parent", with
a "struct device_node *node" as in-parameter. That's all you need,
right?

> +{
> +       struct pd_warming_device *pd_wdev;
> +       struct of_phandle_args pd_args;
> +       int ret;
> +
> +       pd_wdev = kzalloc(sizeof(*pd_wdev), GFP_KERNEL);
> +       if (!pd_wdev)
> +               return ERR_PTR(-ENOMEM);
> +
> +       dev_set_name(&pd_wdev->dev, "%s_warming_dev", pd_name);

Perhaps skip the in-param *pd_name and make use of the suggested
"struct device_node *node", the index and something with "warming...",
when setting the name.

Just an idea, as to simplify for the caller.

> +       pd_wdev->dev.parent = parent;

This isn't needed, I think.

> +
> +       ret = device_register(&pd_wdev->dev);
> +       if (ret)
> +               goto error;
> +
> +       pd_args.np = parent->of_node;
> +       pd_args.args[0] = pd_id;
> +       pd_args.args_count = 1;
> +
> +       ret = of_genpd_add_device(&pd_args, &pd_wdev->dev);
> +

White space.

> +       if (ret)
> +               goto error;
> +
> +       ret = dev_pm_genpd_performance_state_count(&pd_wdev->dev);
> +       if (ret < 0)
> +               goto error;
> +
> +       pd_wdev->max_state = ret - 1;
> +       pm_runtime_enable(&pd_wdev->dev);
> +       pd_wdev->runtime_resumed = false;
> +
> +       pd_wdev->cdev = thermal_of_cooling_device_parent_register
> +                                       (NULL, parent, pd_name, pd_wdev,
> +                                        &pd_warming_device_ops);

As stated in patch3, I don't get it why you need to use this new API
for "parents".

> +       if (IS_ERR(pd_wdev->cdev)) {
> +               pr_err("unable to register %s cooling device\n", pd_name);
> +               pm_runtime_disable(&pd_wdev->dev);
> +               ret = PTR_ERR(pd_wdev->cdev);
> +               goto error;
> +       }
> +
> +       return pd_wdev->cdev;
> +error:
> +       put_device(&pd_wdev->dev);

If device_register() succeeds you need to call device_unregister(),
rather than put_device() as a part of the error handling.

> +       kfree(pd_wdev);

You need a ->release() callback to manage kfree(), after you called
device_register().

> +       return ERR_PTR(ret);

Another thing is missing in the error path, which is to remove the
device for the genpd. I think calling pm_genpd_remove_device() should
work fine here.

> +}
> +EXPORT_SYMBOL_GPL(pwr_domain_warming_register);
> +
> +void pwr_domain_warming_unregister(struct thermal_cooling_device *cdev)
> +{
> +       struct pd_warming_device *pd_wdev = cdev->devdata;
> +       struct device *dev = &pd_wdev->dev;
> +
> +       if (pd_wdev->runtime_resumed) {
> +               dev_pm_genpd_set_performance_state(dev, 0);
> +               pm_runtime_put(dev);
> +               pd_wdev->runtime_resumed = false;
> +       }
> +       pm_runtime_disable(dev);
> +       thermal_cooling_device_unregister(cdev);
> +       kfree(pd_wdev);
> +}
> +EXPORT_SYMBOL_GPL(pwr_domain_warming_unregister);
> diff --git a/include/linux/pwr_domain_warming.h b/include/linux/pwr_domain_warming.h
> new file mode 100644
> index 0000000..cb6550d
> --- /dev/null
> +++ b/include/linux/pwr_domain_warming.h
> @@ -0,0 +1,29 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2019, Linaro Ltd.
> + */
> +#ifndef __PWR_DOMAIN_WARMING_H__
> +#define __PWR_DOMAIN_WARMING_H__
> +
> +#include <linux/pm_domain.h>
> +#include <linux/thermal.h>
> +
> +#ifdef CONFIG_PWR_DOMAIN_WARMING_THERMAL
> +struct thermal_cooling_device *
> +pwr_domain_warming_register(struct device *parent, char *pd_name, int pd_id);
> +
> +void pwr_domain_warming_unregister(struct thermal_cooling_device *cdev);
> +
> +#else
> +static inline struct thermal_cooling_device *
> +pwr_domain_warming_register(struct device *parent, char *pd_name, int pd_id)
> +{
> +       return ERR_PTR(-ENOSYS);
> +}
> +
> +static inline void
> +pwr_domain_warming_unregister(struct thermal_cooling_device *cdev)
> +{
> +}
> +#endif /* CONFIG_PWR_DOMAIN_WARMING_THERMAL */
> +#endif /* __PWR_DOMAIN_WARMING_H__ */
> --
> 2.1.4
>

Kind regards
Uffe

Thara Gopinath March 1, 2020, 11 p.m. UTC | #2

Hi Ulf,

Thanks for the reviews. Sorry for the delay in response.
I have started working on this again. So this should pick
up pace now.

On 2/4/20 11:54 AM, Ulf Hansson wrote:
> On Wed, 20 Nov 2019 at 13:56, Thara Gopinath <[e-mail]> wrote:
>>
>> Resources modeled as power domains in linux kenrel can  be used to warm the
>> SoC(eg. mx power domain on sdm845).  To support this feature, introduce a
>> generic power domain warming 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 <[e-mail]>
>> ---
>> v3->v4:
>>          - Removed late_init hook pd_warming_device_ops.
>>          - Use of_genpd_add_device instead of pm_genpd_add_device to attach
>>            device to the generic power domain.
>>          - Use thermal_of_cooling_device_parent_register to register the
>>            cooling device so that the device with genpd attached can be
>>            made parent of the cooling device.
>>          - With above changes, remove reference to generic_pm_domain in
>>            pd_warming_device.
>>
>>   drivers/thermal/Kconfig              |  10 +++
>>   drivers/thermal/Makefile             |   2 +
>>   drivers/thermal/pwr_domain_warming.c | 138 +++++++++++++++++++++++++++++++++++
>>   include/linux/pwr_domain_warming.h   |  29 ++++++++
> 
> Not sure about what the thermal maintainers think about the naming
> here. In the end, it's their call.
> 
> However, normally we use "pm_domain_*", rather than "pwr_domain_*",
> but maybe just "pd_*" is sufficient here.

I will rename this to pd_ for now.

> 
>>   4 files changed, 179 insertions(+)
>>   create mode 100644 drivers/thermal/pwr_domain_warming.c
>>   create mode 100644 include/linux/pwr_domain_warming.h
>>
>> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
>> index 001a21a..0c5c93e 100644
>> --- a/drivers/thermal/Kconfig
>> +++ b/drivers/thermal/Kconfig
>> @@ -187,6 +187,16 @@ config DEVFREQ_THERMAL
>>
>>            If you want this support, you should say Y here.
>>
>> +config PWR_DOMAIN_WARMING_THERMAL
>> +       bool "Power Domain based warming device"
>> +       depends on PM_GENERIC_DOMAINS_OF
>> +       help
>> +         This implements the generic power domain based warming
>> +         mechanism through increasing the performance state of
>> +         a power domain.
>> +
>> +         If you want this support, you should say Y here.
>> +
>>   config THERMAL_EMULATION
>>          bool "Thermal emulation mode support"
>>          help
>> diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
>> index 74a37c7..382c64a 100644
>> --- a/drivers/thermal/Makefile
>> +++ b/drivers/thermal/Makefile
>> @@ -27,6 +27,8 @@ thermal_sys-$(CONFIG_CLOCK_THERMAL)   += clock_cooling.o
>>   # devfreq cooling
>>   thermal_sys-$(CONFIG_DEVFREQ_THERMAL) += devfreq_cooling.o
>>
>> +thermal_sys-$(CONFIG_PWR_DOMAIN_WARMING_THERMAL)       += pwr_domain_warming.o
>> +
>>   # platform thermal drivers
>>   obj-y                          += broadcom/
>>   obj-$(CONFIG_THERMAL_MMIO)             += thermal_mmio.o
>> diff --git a/drivers/thermal/pwr_domain_warming.c b/drivers/thermal/pwr_domain_warming.c
>> new file mode 100644
>> index 0000000..40162b9
>> --- /dev/null
>> +++ b/drivers/thermal/pwr_domain_warming.c
>> @@ -0,0 +1,138 @@
>> +// 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_runtime.h>
>> +#include <linux/slab.h>
>> +#include <linux/pwr_domain_warming.h>
>> +
>> +struct pd_warming_device {
>> +       struct thermal_cooling_device *cdev;
>> +       struct device dev;
>> +       int max_state;
>> +       int cur_state;
>> +       bool runtime_resumed;
>> +};
>> +
>> +static int pd_wdev_get_max_state(struct thermal_cooling_device *cdev,
>> +                                unsigned long *state)
>> +{
>> +       struct pd_warming_device *pd_wdev = cdev->devdata;
>> +
>> +       *state = pd_wdev->max_state;
>> +       return 0;
>> +}
>> +
>> +static int pd_wdev_get_cur_state(struct thermal_cooling_device *cdev,
>> +                                unsigned long *state)
>> +{
>> +       struct pd_warming_device *pd_wdev = cdev->devdata;
>> +
>> +       *state = dev_pm_genpd_get_performance_state(&pd_wdev->dev);
>> +
>> +       return 0;
>> +}
>> +
>> +static int pd_wdev_set_cur_state(struct thermal_cooling_device *cdev,
>> +                                unsigned long state)
>> +{
>> +       struct pd_warming_device *pd_wdev = cdev->devdata;
>> +       struct device *dev = &pd_wdev->dev;
>> +       int ret;
>> +
>> +       ret = dev_pm_genpd_set_performance_state(dev, state);
>> +
>> +       if (ret)
>> +               return ret;
>> +
>> +       if (state && !pd_wdev->runtime_resumed) {
>> +               ret = pm_runtime_get_sync(dev);
>> +               pd_wdev->runtime_resumed = true;
>> +       } else if (!state && pd_wdev->runtime_resumed) {
>> +               ret = pm_runtime_put(dev);
>> +               pd_wdev->runtime_resumed = false;
>> +       }
>> +
>> +       return ret;
>> +}
>> +
>> +static struct thermal_cooling_device_ops pd_warming_device_ops = {
>> +       .get_max_state  = ::pd_wdev_get_max_state,
>> +       .get_cur_state  = pd_wdev_get_cur_state,
>> +       .set_cur_state  = pd_wdev_set_cur_state,
>> +};
>> +
>> +struct thermal_cooling_device *
>> +pwr_domain_warming_register(struct device *parent, char *pd_name, int pd_id)
> 
> Maybe rename this to: thermal_of_pd_warming_register()

How about pd_of_warming_register? It is consistent with other cooling 
device register like cpuidle_of_cooling_register and 
cpufreq_of_cooling_register.

> Moreover, I think you could replace the "struct device *parent", with
> a "struct device_node *node" as in-parameter. That's all you need,
> right?

You mean pd_wdev->dev.parent need not be populated ? The device
in this case will be created under /sys/devices which I do not think
is the correct.
With a parent device specified, the power controller that resides the 
power domain that can act as the warming dev, becomes the parent of the 
warming dev. In case of this patch series, for the mx warming dev, 
179c0000.rsc/179c0000.rsc\:power-controller/ becomes the parent.(The 
device will be created under 
/sys/devices/platform/soc\@0/179c0000.rsc/179c0000.rsc\:power-controller/)

Other way might be to register the warming device under virtual devices 
as a new category of devices.

I prefer to keep it as a child of the power controller device, but I am 
open to explore other options and to re-do this bit. What do you think?

> 
>> +{
>> +       struct pd_warming_device *pd_wdev;
>> +       struct of_phandle_args pd_args;
>> +       int ret;
>> +
>> +       pd_wdev = kzalloc(sizeof(*pd_wdev), GFP_KERNEL);
>> +       if (!pd_wdev)
>> +               return ERR_PTR(-ENOMEM);
>> +
>> +       dev_set_name(&pd_wdev->dev, "%s_warming_dev", pd_name);
> 
> Perhaps skip the in-param *pd_name and make use of the suggested
> "struct device_node *node", the index and something with "warming...",
> when setting the name.

Won't the index have to be in-param in this case ?

> 
> Just an idea, as to simplify for the caller.
> 
>> +       pd_wdev->dev.parent = parent;
> 
> This isn't needed, I think.
> 
>> +
>> +       ret = device_register(&pd_wdev->dev);
>> +       if (ret)
>> +               goto error;
>> +
>> +       pd_args.np = parent->of_node;
>> +       pd_args.args[0] = pd_id;
>> +       pd_args.args_count = 1;
>> +
>> +       ret = of_genpd_add_device(&pd_args, &pd_wdev->dev);
>> +
> 
> White space.

Will fix it.

> 
>> +       if (ret)
>> +               goto error;
>> +
>> +       ret = dev_pm_genpd_performance_state_count(&pd_wdev->dev);
>> +       if (ret < 0)
>> +               goto error;
>> +
>> +       pd_wdev->max_state = ret - 1;
>> +       pm_runtime_enable(&pd_wdev->dev);
>> +       pd_wdev->runtime_resumed = false;
>> +
>> +       pd_wdev->cdev = thermal_of_cooling_device_parent_register
>> +                                       (NULL, parent, pd_name, pd_wdev,
>> +                                        &pd_warming_device_ops);
> 
> As stated in patch3, I don't get it why you need to use this new API
> for "parents".

I agree with you. I cannot re-collect my thought process for this API.
I compiled and tested using the regular API and everything works fine.
I will drop patch 3 and use the thermal_of_cooling_device_register here.

> 
>> +       if (IS_ERR(pd_wdev->cdev)) {
>> +               pr_err("unable to register %s cooling device\n", pd_name);
>> +               pm_runtime_disable(&pd_wdev->dev);
>> +               ret = PTR_ERR(pd_wdev->cdev);
>> +               goto error;
>> +       }
>> +
>> +       return pd_wdev->cdev;
>> +error:
>> +       put_device(&pd_wdev->dev);
> 
> If device_register() succeeds you need to call device_unregister(),
> rather than put_device() as a part of the error handling.

Will fix this.

> 
>> +       kfree(pd_wdev);
> 
> You need a ->release() callback to manage kfree(), after you called
> device_register().

mm?? I did not get this. What release callback? You mean for power 
controller driver to call ?

> 
>> +       return ERR_PTR(ret);
> 
> Another thing is missing in the error path, which is to remove the
> device for the genpd. I think calling pm_genpd_remove_device() should
> work fine here.

I will fix this. I am thinking this will be be needed in 
pwr_domain_warming_unregister as well.

Ulf Hansson March 13, 2020, 1:13 p.m. UTC | #3

[...]

> >> +static struct thermal_cooling_device_ops pd_warming_device_ops = {
> >> +       .get_max_state  = ::pd_wdev_get_max_state,
> >> +       .get_cur_state  = pd_wdev_get_cur_state,
> >> +       .set_cur_state  = pd_wdev_set_cur_state,
> >> +};
> >> +
> >> +struct thermal_cooling_device *
> >> +pwr_domain_warming_register(struct device *parent, char *pd_name, int pd_id)
> >
> > Maybe rename this to: thermal_of_pd_warming_register()
>
> How about pd_of_warming_register? It is consistent with other cooling
> device register like cpuidle_of_cooling_register and
> cpufreq_of_cooling_register.

Well, we actually have the following:
of_devfreq_cooling_register()
of_cpufreq_cooling_register()
cpuidle_of_cooling_register()

So maybe this is the most consistent. :-)
of_pd_warming_register()

>
> > Moreover, I think you could replace the "struct device *parent", with
> > a "struct device_node *node" as in-parameter. That's all you need,
> > right?
>
> You mean pd_wdev->dev.parent need not be populated ? The device
> in this case will be created under /sys/devices which I do not think
> is the correct.

Good point!

> With a parent device specified, the power controller that resides the
> power domain that can act as the warming dev, becomes the parent of the
> warming dev. In case of this patch series, for the mx warming dev,
> 179c0000.rsc/179c0000.rsc\:power-controller/ becomes the parent.(The
> device will be created under
> /sys/devices/platform/soc\@0/179c0000.rsc/179c0000.rsc\:power-controller/)
>
> Other way might be to register the warming device under virtual devices
> as a new category of devices.

No, that sounds wrong.

Another option is to create a specific bus type for these new
pd_warming devices. But I admit that sounds a bit too much, let's
assign a parent.

>
> I prefer to keep it as a child of the power controller device, but I am
> open to explore other options and to re-do this bit. What do you think?

Sure, sorry for the noise.

>
> >
> >> +{
> >> +       struct pd_warming_device *pd_wdev;
> >> +       struct of_phandle_args pd_args;
> >> +       int ret;
> >> +
> >> +       pd_wdev = kzalloc(sizeof(*pd_wdev), GFP_KERNEL);
> >> +       if (!pd_wdev)
> >> +               return ERR_PTR(-ENOMEM);
> >> +
> >> +       dev_set_name(&pd_wdev->dev, "%s_warming_dev", pd_name);
> >
> > Perhaps skip the in-param *pd_name and make use of the suggested
> > "struct device_node *node", the index and something with "warming...",
> > when setting the name.
>
> Won't the index have to be in-param in this case ?

Isn't that already the case?

Huh, long time since I reviewed this.

>
> >
> > Just an idea, as to simplify for the caller.
> >
> >> +       pd_wdev->dev.parent = parent;
> >
> > This isn't needed, I think.

So ignore this comment, as discussed above.

> >
> >> +
> >> +       ret = device_register(&pd_wdev->dev);
> >> +       if (ret)
> >> +               goto error;
> >> +
> >> +       pd_args.np = parent->of_node;
> >> +       pd_args.args[0] = pd_id;
> >> +       pd_args.args_count = 1;
> >> +
> >> +       ret = of_genpd_add_device(&pd_args, &pd_wdev->dev);
> >> +
> >
> > White space.
>
> Will fix it.
>
> >
> >> +       if (ret)
> >> +               goto error;
> >> +
> >> +       ret = dev_pm_genpd_performance_state_count(&pd_wdev->dev);
> >> +       if (ret < 0)
> >> +               goto error;
> >> +
> >> +       pd_wdev->max_state = ret - 1;
> >> +       pm_runtime_enable(&pd_wdev->dev);
> >> +       pd_wdev->runtime_resumed = false;
> >> +
> >> +       pd_wdev->cdev = thermal_of_cooling_device_parent_register
> >> +                                       (NULL, parent, pd_name, pd_wdev,
> >> +                                        &pd_warming_device_ops);
> >
> > As stated in patch3, I don't get it why you need to use this new API
> > for "parents".
>
> I agree with you. I cannot re-collect my thought process for this API.
> I compiled and tested using the regular API and everything works fine.
> I will drop patch 3 and use the thermal_of_cooling_device_register here.

Great, one confusing piece seems to go away then. :-)

>
> >
> >> +       if (IS_ERR(pd_wdev->cdev)) {
> >> +               pr_err("unable to register %s cooling device\n", pd_name);
> >> +               pm_runtime_disable(&pd_wdev->dev);
> >> +               ret = PTR_ERR(pd_wdev->cdev);
> >> +               goto error;
> >> +       }
> >> +
> >> +       return pd_wdev->cdev;
> >> +error:
> >> +       put_device(&pd_wdev->dev);
> >
> > If device_register() succeeds you need to call device_unregister(),
> > rather than put_device() as a part of the error handling.
>
> Will fix this.
>
> >
> >> +       kfree(pd_wdev);
> >
> > You need a ->release() callback to manage kfree(), after you called
> > device_register().
>
> mm?? I did not get this. What release callback? You mean for power
> controller driver to call ?

No, this how life cycle management of devices should be implemented.

Have a look at genpd_release_dev() - and see how that is being used
for genpd's virtual devices, that should explain more.

>
> >
> >> +       return ERR_PTR(ret);
> >
> > Another thing is missing in the error path, which is to remove the
> > device for the genpd. I think calling pm_genpd_remove_device() should
> > work fine here.
>
> I will fix this. I am thinking this will be be needed in
> pwr_domain_warming_unregister as well.

Yep.

Kind regards
Uffe

Thara Gopinath March 13, 2020, 3:02 p.m. UTC | #4

Hi Ulf,

Thanks for the reviews. Will send out v5 soon.

On 3/13/20 9:13 AM, Ulf Hansson wrote:
> [...]
> 
>>>> +static struct thermal_cooling_device_ops pd_warming_device_ops = {
>>>> +       .get_max_state  = ::pd_wdev_get_max_state,
>>>> +       .get_cur_state  = pd_wdev_get_cur_state,
>>>> +       .set_cur_state  = pd_wdev_set_cur_state,
>>>> +};
>>>> +
>>>> +struct thermal_cooling_device *
>>>> +pwr_domain_warming_register(struct device *parent, char *pd_name, int pd_id)
>>>
>>> Maybe rename this to: thermal_of_pd_warming_register()
>>
>> How about pd_of_warming_register? It is consistent with other cooling
>> device register like cpuidle_of_cooling_register and
>> cpufreq_of_cooling_register.
> 
> Well, we actually have the following:
> of_devfreq_cooling_register()
> of_cpufreq_cooling_register()
> cpuidle_of_cooling_register()
> 
> So maybe this is the most consistent. :-)
> of_pd_warming_register()

Sure!

> 
>>
>>> Moreover, I think you could replace the "struct device *parent", with
>>> a "struct device_node *node" as in-parameter. That's all you need,
>>> right?
>>
>> You mean pd_wdev->dev.parent need not be populated ? The device
>> in this case will be created under /sys/devices which I do not think
>> is the correct.
> 
> Good point!
> 
>> With a parent device specified, the power controller that resides the
>> power domain that can act as the warming dev, becomes the parent of the
>> warming dev. In case of this patch series, for the mx warming dev,
>> 179c0000.rsc/179c0000.rsc\:power-controller/ becomes the parent.(The
>> device will be created under
>> /sys/devices/platform/soc\@0/179c0000.rsc/179c0000.rsc\:power-controller/)
>>
>> Other way might be to register the warming device under virtual devices
>> as a new category of devices.
> 
> No, that sounds wrong.
> 
> Another option is to create a specific bus type for these new
> pd_warming devices. But I admit that sounds a bit too much, let's
> assign a parent.
> 
>>
>> I prefer to keep it as a child of the power controller device, but I am
>> open to explore other options and to re-do this bit. What do you think?
> 
> Sure, sorry for the noise.

No issues!

> 
>>
>>>
>>>> +{
>>>> +       struct pd_warming_device *pd_wdev;
>>>> +       struct of_phandle_args pd_args;
>>>> +       int ret;
>>>> +
>>>> +       pd_wdev = kzalloc(sizeof(*pd_wdev), GFP_KERNEL);
>>>> +       if (!pd_wdev)
>>>> +               return ERR_PTR(-ENOMEM);
>>>> +
>>>> +       dev_set_name(&pd_wdev->dev, "%s_warming_dev", pd_name);
>>>
>>> Perhaps skip the in-param *pd_name and make use of the suggested
>>> "struct device_node *node", the index and something with "warming...",
>>> when setting the name.
>>
>> Won't the index have to be in-param in this case ?
> 
> Isn't that already the case?
> 
> Huh, long time since I reviewed this.
> 
>>
>>>
>>> Just an idea, as to simplify for the caller.
>>>
>>>> +       pd_wdev->dev.parent = parent;
>>>
>>> This isn't needed, I think.
> 
> So ignore this comment, as discussed above.
> 
>>>
>>>> +
>>>> +       ret = device_register(&pd_wdev->dev);
>>>> +       if (ret)
>>>> +               goto error;
>>>> +
>>>> +       pd_args.np = parent->of_node;
>>>> +       pd_args.args[0] = pd_id;
>>>> +       pd_args.args_count = 1;
>>>> +
>>>> +       ret = of_genpd_add_device(&pd_args, &pd_wdev->dev);
>>>> +
>>>
>>> White space.
>>
>> Will fix it.
>>
>>>
>>>> +       if (ret)
>>>> +               goto error;
>>>> +
>>>> +       ret = dev_pm_genpd_performance_state_count(&pd_wdev->dev);
>>>> +       if (ret < 0)
>>>> +               goto error;
>>>> +
>>>> +       pd_wdev->max_state = ret - 1;
>>>> +       pm_runtime_enable(&pd_wdev->dev);
>>>> +       pd_wdev->runtime_resumed = false;
>>>> +
>>>> +       pd_wdev->cdev = thermal_of_cooling_device_parent_register
>>>> +                                       (NULL, parent, pd_name, pd_wdev,
>>>> +                                        &pd_warming_device_ops);
>>>
>>> As stated in patch3, I don't get it why you need to use this new API
>>> for "parents".
>>
>> I agree with you. I cannot re-collect my thought process for this API.
>> I compiled and tested using the regular API and everything works fine.
>> I will drop patch 3 and use the thermal_of_cooling_device_register here.
> 
> Great, one confusing piece seems to go away then. :-)
> 
>>
>>>
>>>> +       if (IS_ERR(pd_wdev->cdev)) {
>>>> +               pr_err("unable to register %s cooling device\n", pd_name);
>>>> +               pm_runtime_disable(&pd_wdev->dev);
>>>> +               ret = PTR_ERR(pd_wdev->cdev);
>>>> +               goto error;
>>>> +       }
>>>> +
>>>> +       return pd_wdev->cdev;
>>>> +error:
>>>> +       put_device(&pd_wdev->dev);
>>>
>>> If device_register() succeeds you need to call device_unregister(),
>>> rather than put_device() as a part of the error handling.
>>
>> Will fix this.
>>
>>>
>>>> +       kfree(pd_wdev);
>>>
>>> You need a ->release() callback to manage kfree(), after you called
>>> device_register().
>>
>> mm?? I did not get this. What release callback? You mean for power
>> controller driver to call ?
> 
> No, this how life cycle management of devices should be implemented.
> 
> Have a look at genpd_release_dev() - and see how that is being used
> for genpd's virtual devices, that should explain more.

Ah yes. I get it now. Will fix this.

Patch

diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
index 001a21a..0c5c93e 100644
--- a/drivers/thermal/Kconfig
+++ b/drivers/thermal/Kconfig
@@ -187,6 +187,16 @@  config DEVFREQ_THERMAL
 
 	  If you want this support, you should say Y here.
 
+config PWR_DOMAIN_WARMING_THERMAL
+	bool "Power Domain based warming device"
+	depends on PM_GENERIC_DOMAINS_OF
+	help
+	  This implements the generic power domain based warming
+	  mechanism through increasing the performance state of
+	  a power domain.
+
+	  If you want this support, you should say Y here.
+
 config THERMAL_EMULATION
 	bool "Thermal emulation mode support"
 	help
diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
index 74a37c7..382c64a 100644
--- a/drivers/thermal/Makefile
+++ b/drivers/thermal/Makefile
@@ -27,6 +27,8 @@  thermal_sys-$(CONFIG_CLOCK_THERMAL)	+= clock_cooling.o
 # devfreq cooling
 thermal_sys-$(CONFIG_DEVFREQ_THERMAL) += devfreq_cooling.o
 
+thermal_sys-$(CONFIG_PWR_DOMAIN_WARMING_THERMAL)	+= pwr_domain_warming.o
+
 # platform thermal drivers
 obj-y				+= broadcom/
 obj-$(CONFIG_THERMAL_MMIO)		+= thermal_mmio.o
diff --git a/drivers/thermal/pwr_domain_warming.c b/drivers/thermal/pwr_domain_warming.c
new file mode 100644
index 0000000..40162b9
--- /dev/null
+++ b/drivers/thermal/pwr_domain_warming.c
@@ -0,0 +1,138 @@ 
+// 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_runtime.h>
+#include <linux/slab.h>
+#include <linux/pwr_domain_warming.h>
+
+struct pd_warming_device {
+	struct thermal_cooling_device *cdev;
+	struct device dev;
+	int max_state;
+	int cur_state;
+	bool runtime_resumed;
+};
+
+static int pd_wdev_get_max_state(struct thermal_cooling_device *cdev,
+				 unsigned long *state)
+{
+	struct pd_warming_device *pd_wdev = cdev->devdata;
+
+	*state = pd_wdev->max_state;
+	return 0;
+}
+
+static int pd_wdev_get_cur_state(struct thermal_cooling_device *cdev,
+				 unsigned long *state)
+{
+	struct pd_warming_device *pd_wdev = cdev->devdata;
+
+	*state = dev_pm_genpd_get_performance_state(&pd_wdev->dev);
+
+	return 0;
+}
+
+static int pd_wdev_set_cur_state(struct thermal_cooling_device *cdev,
+				 unsigned long state)
+{
+	struct pd_warming_device *pd_wdev = cdev->devdata;
+	struct device *dev = &pd_wdev->dev;
+	int ret;
+
+	ret = dev_pm_genpd_set_performance_state(dev, state);
+
+	if (ret)
+		return ret;
+
+	if (state && !pd_wdev->runtime_resumed) {
+		ret = pm_runtime_get_sync(dev);
+		pd_wdev->runtime_resumed = true;
+	} else if (!state && pd_wdev->runtime_resumed) {
+		ret = pm_runtime_put(dev);
+		pd_wdev->runtime_resumed = false;
+	}
+
+	return ret;
+}
+
+static struct thermal_cooling_device_ops pd_warming_device_ops = {
+	.get_max_state	= pd_wdev_get_max_state,
+	.get_cur_state	= pd_wdev_get_cur_state,
+	.set_cur_state	= pd_wdev_set_cur_state,
+};
+
+struct thermal_cooling_device *
+pwr_domain_warming_register(struct device *parent, char *pd_name, int pd_id)
+{
+	struct pd_warming_device *pd_wdev;
+	struct of_phandle_args pd_args;
+	int ret;
+
+	pd_wdev = kzalloc(sizeof(*pd_wdev), GFP_KERNEL);
+	if (!pd_wdev)
+		return ERR_PTR(-ENOMEM);
+
+	dev_set_name(&pd_wdev->dev, "%s_warming_dev", pd_name);
+	pd_wdev->dev.parent = parent;
+
+	ret = device_register(&pd_wdev->dev);
+	if (ret)
+		goto error;
+
+	pd_args.np = parent->of_node;
+	pd_args.args[0] = pd_id;
+	pd_args.args_count = 1;
+
+	ret = of_genpd_add_device(&pd_args, &pd_wdev->dev);
+
+	if (ret)
+		goto error;
+
+	ret = dev_pm_genpd_performance_state_count(&pd_wdev->dev);
+	if (ret < 0)
+		goto error;
+
+	pd_wdev->max_state = ret - 1;
+	pm_runtime_enable(&pd_wdev->dev);
+	pd_wdev->runtime_resumed = false;
+
+	pd_wdev->cdev = thermal_of_cooling_device_parent_register
+					(NULL, parent, pd_name, pd_wdev,
+					 &pd_warming_device_ops);
+	if (IS_ERR(pd_wdev->cdev)) {
+		pr_err("unable to register %s cooling device\n", pd_name);
+		pm_runtime_disable(&pd_wdev->dev);
+		ret = PTR_ERR(pd_wdev->cdev);
+		goto error;
+	}
+
+	return pd_wdev->cdev;
+error:
+	put_device(&pd_wdev->dev);
+	kfree(pd_wdev);
+	return ERR_PTR(ret);
+}
+EXPORT_SYMBOL_GPL(pwr_domain_warming_register);
+
+void pwr_domain_warming_unregister(struct thermal_cooling_device *cdev)
+{
+	struct pd_warming_device *pd_wdev = cdev->devdata;
+	struct device *dev = &pd_wdev->dev;
+
+	if (pd_wdev->runtime_resumed) {
+		dev_pm_genpd_set_performance_state(dev, 0);
+		pm_runtime_put(dev);
+		pd_wdev->runtime_resumed = false;
+	}
+	pm_runtime_disable(dev);
+	thermal_cooling_device_unregister(cdev);
+	kfree(pd_wdev);
+}
+EXPORT_SYMBOL_GPL(pwr_domain_warming_unregister);
diff --git a/include/linux/pwr_domain_warming.h b/include/linux/pwr_domain_warming.h
new file mode 100644
index 0000000..cb6550d
--- /dev/null
+++ b/include/linux/pwr_domain_warming.h
@@ -0,0 +1,29 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2019, Linaro Ltd.
+ */
+#ifndef __PWR_DOMAIN_WARMING_H__
+#define __PWR_DOMAIN_WARMING_H__
+
+#include <linux/pm_domain.h>
+#include <linux/thermal.h>
+
+#ifdef CONFIG_PWR_DOMAIN_WARMING_THERMAL
+struct thermal_cooling_device *
+pwr_domain_warming_register(struct device *parent, char *pd_name, int pd_id);
+
+void pwr_domain_warming_unregister(struct thermal_cooling_device *cdev);
+
+#else
+static inline struct thermal_cooling_device *
+pwr_domain_warming_register(struct device *parent, char *pd_name, int pd_id)
+{
+	return ERR_PTR(-ENOSYS);
+}
+
+static inline void
+pwr_domain_warming_unregister(struct thermal_cooling_device *cdev)
+{
+}
+#endif /* CONFIG_PWR_DOMAIN_WARMING_THERMAL */
+#endif /* __PWR_DOMAIN_WARMING_H__ */