| Message ID | 1574254593-16078-1-git-send-email-thara.gopinath@linaro.org |
|---|---|
| Headers | show |
| Series | Introduce Power domain based warming device driver | expand |
On Wed, 20 Nov 2019 at 13:56, Thara Gopinath <thara.gopinath@linaro.org> wrote: > > With introduction of power domain warming devices, devices that control the > power domain are registered as the parent of the cooling device so that the > device-genpd hierarchy in kernel is maintained intact. To enable this, > introduce a new API thermal_of_cooling_device_parent_register that takes a > parent device pointer as input. Also, modify > __thermal_cooling_device_register to register parent of a newly created > cooling device, if specified. I am not sure I understand the reasons why you need this, can you please elaborate? I remember we talked about using a "parent" device to deal with device attaching to PM domains (genpd). However, since the DT bindings for "warming devices" was concluded to consist by a single property ("#cooling-cells") as a part of the PM domain provider node, this seems not to be needed. By looking at patch 4/7, you are attaching devices via of_genpd_add_device() and I don't see any need for using a "parent" in there. Can $subject patch be dropped or what am I missing? Kind regards Uffe > > Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org> > --- > drivers/thermal/thermal_core.c | 22 +++++++++++++++++++--- > include/linux/thermal.h | 15 +++++++++++++++ > 2 files changed, 34 insertions(+), 3 deletions(-) > > diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c > index d4481cc..912ba75 100644 > --- a/drivers/thermal/thermal_core.c > +++ b/drivers/thermal/thermal_core.c > @@ -947,6 +947,7 @@ static void bind_cdev(struct thermal_cooling_device *cdev) > */ > static struct thermal_cooling_device * > __thermal_cooling_device_register(struct device_node *np, > + struct device *parent, > const char *type, void *devdata, > const struct thermal_cooling_device_ops *ops) > { > @@ -979,6 +980,8 @@ __thermal_cooling_device_register(struct device_node *np, > cdev->ops = ops; > cdev->updated = false; > cdev->device.class = &thermal_class; > + if (parent) > + cdev->device.parent = parent; > cdev->devdata = devdata; > thermal_cooling_device_setup_sysfs(cdev); > dev_set_name(&cdev->device, "cooling_device%d", cdev->id); > @@ -1024,7 +1027,8 @@ struct thermal_cooling_device * > thermal_cooling_device_register(const char *type, void *devdata, > const struct thermal_cooling_device_ops *ops) > { > - return __thermal_cooling_device_register(NULL, type, devdata, ops); > + return __thermal_cooling_device_register(NULL, NULL, type, > + devdata, ops); > } > EXPORT_SYMBOL_GPL(thermal_cooling_device_register); > > @@ -1048,10 +1052,22 @@ thermal_of_cooling_device_register(struct device_node *np, > const char *type, void *devdata, > const struct thermal_cooling_device_ops *ops) > { > - return __thermal_cooling_device_register(np, type, devdata, ops); > + return __thermal_cooling_device_register(np, NULL, type, devdata, ops); > } > EXPORT_SYMBOL_GPL(thermal_of_cooling_device_register); > > +struct thermal_cooling_device * > +thermal_of_cooling_device_parent_register(struct device_node *np, > + struct device *parent, > + const char *type, void *devdata, > + const struct > + thermal_cooling_device_ops *ops) > +{ > + return __thermal_cooling_device_register(np, parent, type, > + devdata, ops); > +} > +EXPORT_SYMBOL_GPL(thermal_of_cooling_device_parent_register); > + > static void thermal_cooling_device_release(struct device *dev, void *res) > { > thermal_cooling_device_unregister( > @@ -1088,7 +1104,7 @@ devm_thermal_of_cooling_device_register(struct device *dev, > if (!ptr) > return ERR_PTR(-ENOMEM); > > - tcd = __thermal_cooling_device_register(np, type, devdata, ops); > + tcd = __thermal_cooling_device_register(np, NULL, type, devdata, ops); > if (IS_ERR(tcd)) { > devres_free(ptr); > return tcd; > diff --git a/include/linux/thermal.h b/include/linux/thermal.h > index e45659c..ac5f268 100644 > --- a/include/linux/thermal.h > +++ b/include/linux/thermal.h > @@ -448,6 +448,11 @@ struct thermal_cooling_device * > thermal_of_cooling_device_register(struct device_node *np, const char *, void *, > const struct thermal_cooling_device_ops *); > struct thermal_cooling_device * > +thermal_of_cooling_device_parent_register(struct device_node *np, > + struct device *parent, > + const char *, void *, const struct > + thermal_cooling_device_ops *); > +struct thermal_cooling_device * > devm_thermal_of_cooling_device_register(struct device *dev, > struct device_node *np, > char *type, void *devdata, > @@ -508,6 +513,16 @@ static inline struct thermal_cooling_device * > thermal_of_cooling_device_register(struct device_node *np, > char *type, void *devdata, const struct thermal_cooling_device_ops *ops) > { return ERR_PTR(-ENODEV); } > + > +static inline struct thermal_cooling_device * > +thermal_of_cooling_device_parent_register(struct device_node *np, > + struct device *parent, > + const char *, void *, const struct > + thermal_cooling_device_ops *) > +{ > + return ERR_PTR(-ENODEV); > +} > + > static inline struct thermal_cooling_device * > devm_thermal_of_cooling_device_register(struct device *dev, > struct device_node *np, > -- > 2.1.4 >
On Wed, 20 Nov 2019 at 13:56, Thara Gopinath <thara.gopinath@linaro.org> 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 <thara.gopinath@linaro.org> > --- > 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
On Wed, 20 Nov 2019 at 13:56, Thara Gopinath <thara.gopinath@linaro.org> 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. > > 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 registerd after thermal > framework is initialized. > > drivers/soc/qcom/rpmhpd.c | 38 +++++++++++++++++++++++++++++++++++++- > 1 file changed, 37 insertions(+), 1 deletion(-) > > diff --git a/drivers/soc/qcom/rpmhpd.c b/drivers/soc/qcom/rpmhpd.c > index 9d37534..5666d1f 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/pwr_domain_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 = { > @@ -396,7 +401,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; I assume this works fine, for now. Although, nothing prevents this driver from being probed for two different compatibles for the same platform. Thus the global_desc could be overwritten with the last one being probed, so then how do you know which one to use? > + > + return 0; > } > > static struct platform_driver rpmhpd_driver = { > @@ -413,3 +425,27 @@ 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) > + pwr_domain_warming_register(rpmhpds[i]->dev, > + rpmhpds[i]->res_name, i); > + > + return 0; > +} > +late_initcall(rpmhpd_init_warming_device); For the record, there are limitations with this approach, for example you can't deal with -EPROBE_DEFER. On the other hand, I don't have anything better to suggest, from the top of my head. So, feel free to add: Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org> Kind regards Uffe
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 <thara.gopinath@linaro.org> 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 <thara.gopinath@linaro.org> >> --- >> 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.
Certain resources modeled as a generic power domain in linux kernel can be used to warm up the SoC (mx power domain 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. Since linux kernel today has no instance of a resource modeled as a power domain acting as a thermal warming device, a generic power domain based thermal warming device driver that can be used pan-Socs is the approach taken in this patch series. Since thermal warming devices can be thought of as the mirror opposite of thermal cooling devices, this patch series re-uses thermal cooling device framework. To use these power domains as warming devices require further tweaks in the thermal framework which are out of scope of this patch series. These tweaks have been posted as a separate series[1]. The first patch in this series extends the genpd framework to export out the performance states of a power domain so that when a 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 Qualcomm RPMH power domain driver which hosts the mx power domain. The third patch introduces a new cooling device register API that allows a parent to be specified for the cooling device. The fourth patch introduces the generic power domain warming device driver. The fifth patch extends Qualcomm RPMh power controller driver to register mx power domain as a thermal warming device in the kernel. The sixth patch describes the dt binding extensions for mx power domain to be a thermal warming device. The seventh patch introduces the DT entreis for sdm845 to register mx power domain as a thermal warming device. v1->v2: - Rename the patch series from "qcom: Model RPMH power domains as thermal cooling devices" to "Introduce Power domain based thermal warming devices" as it is more appropriate. - Introduce a new patch(patch 3) describing the dt-bindings for generic power domain warming device. - Patch specific changes mentioned in respective patches. v2->v3: - Changed power domain warming device from a virtual device node entry in DT to being a subnode of power domain controller binding following Rob's review comments. - Implemented Ulf's review comments. - The changes above introduced two new patches (patch 3 and 4) v3->v4: - Dropped late_init hook in cooling device ops. Instead introduced a new cooling device register API that allows to define a parent for the cooling device. - Patch specific changes mentioned in respective patches. 1. https://lkml.org/lkml/2019/9/18/1180 Thara Gopinath (7): PM/Domains: Add support for retrieving genpd performance states information soc: qcom: rpmhpd: Introduce function to retrieve power domain performance state count thermal: core: Allow cooling devices to register a parent. thermal: Add generic power domain warming device driver. soc: qcom: Extend RPMh power controller driver to register warming devices. dt-bindings: soc: qcom: Extend RPMh power controller binding to describe thermal warming device arm64: dts: qcom: Add mx power domain as thermal warming device. .../devicetree/bindings/power/qcom,rpmpd.txt | 5 + arch/arm64/boot/dts/qcom/sdm845.dtsi | 5 + drivers/base/power/domain.c | 37 ++++++ drivers/soc/qcom/rpmhpd.c | 47 ++++++- drivers/thermal/Kconfig | 10 ++ drivers/thermal/Makefile | 2 + drivers/thermal/pwr_domain_warming.c | 138 +++++++++++++++++++++ drivers/thermal/thermal_core.c | 22 +++- include/linux/pm_domain.h | 13 ++ include/linux/pwr_domain_warming.h | 29 +++++ include/linux/thermal.h | 15 +++ 11 files changed, 319 insertions(+), 4 deletions(-) create mode 100644 drivers/thermal/pwr_domain_warming.c create mode 100644 include/linux/pwr_domain_warming.h -- 2.1.4