| Message ID | 1571254641-13626-5-git-send-email-thara.gopinath@linaro.org (mailing list archive) |
|---|---|
| State | RFC, archived |
| Headers | show |
| Series | Introduce Power domain based warming device driver | expand |
On Wed, 16 Oct 2019 at 21:37, 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> > --- > drivers/thermal/Kconfig | 10 +++ > drivers/thermal/Makefile | 2 + > drivers/thermal/pwr_domain_warming.c | 136 +++++++++++++++++++++++++++++++++++ > include/linux/pwr_domain_warming.h | 31 ++++++++ > 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..60fae3e > --- /dev/null > +++ b/drivers/thermal/pwr_domain_warming.c > @@ -0,0 +1,136 @@ > +// 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 generic_pm_domain *gpd; No, this isn't a genpd provider and thus we should not need to carry the above pointer in the struct pd_warming_device. > + 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 int pd_wdev_late_init(struct thermal_cooling_device *cdev) > +{ > + struct pd_warming_device *pd_wdev = cdev->devdata; > + struct device *dev = &cdev->device; > + int state_count, ret; > + > + ret = pm_genpd_add_device(pd_wdev->gpd, dev); The pm_genpd_add_device() is a legacy interface and we are striving to remove it. I think there are two better options for you to use to deal with the attach thingy. 1. The easiest one is probably just to convert into using of_genpd_add_device() instead. I think you already have the information that you need in the ->cdev pointer to do that. However, that also means you need to add the ->late_init() callback to the struct thermal_cooling_device_ops, like what you do here. 2. Maybe the most correct solution is, rather than attaching &cdev->device to the PM domain, to register a separate new device (device_register()) and assign it the corresponding OF node as the genpd provider's subnode and then attach this one instead. If "power-domains" can be specified in the subnode, you can even use dev_pm_domain_attach() to attach the device to the PM domain, else of_genpd_add_device() should work. In the second step, when registering the cooling device, the new device above should be assigned as parent to the device that is about to be registered via thermal_of_cooling_device_register(). In other words, the thermal_of_cooling_device_register() needs to be extended to cope with receiving a parent device as an in-parameter, but that should be doable I think. In this way, you don't need to add the ->late_init() callback at all, but you can instead just use the parent device when operating on the PM domain. > + > + if (ret) > + return ret; > + > + state_count = dev_pm_genpd_performance_state_count(dev); > + if (state_count < 0) > + return state_count; > + > + pm_runtime_enable(dev); > + > + pd_wdev->dev = dev; > + pd_wdev->max_state = state_count - 1; > + > + return 0; > +} > + > +static struct thermal_cooling_device_ops pd_warming_device_ops = { > + .late_init = pd_wdev_late_init, > + .get_max_state = pd_wdev_get_max_state, > + .get_cur_state = pd_wdev_get_cur_state, > + .set_cur_state = pd_wdev_set_cur_state, > +}; [...] Kind regards Uffe
On 10/17/2019 04:47 AM, Ulf Hansson wrote: > On Wed, 16 Oct 2019 at 21:37, 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> >> --- >> drivers/thermal/Kconfig | 10 +++ >> drivers/thermal/Makefile | 2 + >> drivers/thermal/pwr_domain_warming.c | 136 +++++++++++++++++++++++++++++++++++ >> include/linux/pwr_domain_warming.h | 31 ++++++++ >> 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..60fae3e >> --- /dev/null >> +++ b/drivers/thermal/pwr_domain_warming.c >> @@ -0,0 +1,136 @@ >> +// 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 generic_pm_domain *gpd; > > No, this isn't a genpd provider and thus we should not need to carry > the above pointer in the struct pd_warming_device. I store this to attach the device in late_init. More about this approach below. > >> + 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 int pd_wdev_late_init(struct thermal_cooling_device *cdev) >> +{ >> + struct pd_warming_device *pd_wdev = cdev->devdata; >> + struct device *dev = &cdev->device; >> + int state_count, ret; >> + >> + ret = pm_genpd_add_device(pd_wdev->gpd, dev); > > The pm_genpd_add_device() is a legacy interface and we are striving to > remove it. I think there are two better options for you to use to deal > with the attach thingy. I was not aware of this. Apologies. > > 1. The easiest one is probably just to convert into using > of_genpd_add_device() instead. I think you already have the > information that you need in the ->cdev pointer to do that. However, > that also means you need to add the ->late_init() callback to the > struct thermal_cooling_device_ops, like what you do here. > > 2. Maybe the most correct solution is, rather than attaching > &cdev->device to the PM domain, to register a separate new device > (device_register()) and assign it the corresponding OF node as the > genpd provider's subnode and then attach this one instead. If > "power-domains" can be specified in the subnode, you can even use > dev_pm_domain_attach() to attach the device to the PM domain, else > of_genpd_add_device() should work. In the second step, when > registering the cooling device, the new device above should be > assigned as parent to the device that is about to be registered via > thermal_of_cooling_device_register(). In other words, the > thermal_of_cooling_device_register() needs to be extended to cope with > receiving a parent device as an in-parameter, but that should be > doable I think. In this way, you don't need to add the ->late_init() > callback at all, but you can instead just use the parent device when > operating on the PM domain. I did toy with registering a separate device vs reusing cdev device. My rational was, the power domain is actually controlled/needed by the cdev and hence should be attached to it. For me either solution is acceptable . It is a trade off between creating a new device and registering it as a parent of cooling device vs introducing a late init. With the second approach I should be able to do away with the generic_pm_domain pointer in pd_warming_device. To register a parent for a cooling device, I will have to introduce a new API in the thermal framework. Like thermal_of_cooling_device_parent_register. I am ok with this as well. I would like to hear on what some of the thermal maintainers/reviewers have to say about both the approaches and which is better. I will wait a few days for others to review and if there are no major comments, I will send across the series after updating it to the second approach.
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..60fae3e --- /dev/null +++ b/drivers/thermal/pwr_domain_warming.c @@ -0,0 +1,136 @@ +// 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 generic_pm_domain *gpd; + 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 int pd_wdev_late_init(struct thermal_cooling_device *cdev) +{ + struct pd_warming_device *pd_wdev = cdev->devdata; + struct device *dev = &cdev->device; + int state_count, ret; + + ret = pm_genpd_add_device(pd_wdev->gpd, dev); + + if (ret) + return ret; + + state_count = dev_pm_genpd_performance_state_count(dev); + if (state_count < 0) + return state_count; + + pm_runtime_enable(dev); + + pd_wdev->dev = dev; + pd_wdev->max_state = state_count - 1; + + return 0; +} + +static struct thermal_cooling_device_ops pd_warming_device_ops = { + .late_init = pd_wdev_late_init, + .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_node *node, + struct generic_pm_domain *gpd) +{ + struct pd_warming_device *pd_wdev; + + pd_wdev = kzalloc(sizeof(*pd_wdev), GFP_KERNEL); + if (!pd_wdev) + return ERR_PTR(-ENOMEM); + + pd_wdev->runtime_resumed = false; + pd_wdev->gpd = gpd; + + pd_wdev->cdev = thermal_of_cooling_device_register + (node, gpd->name, pd_wdev, + &pd_warming_device_ops); + if (IS_ERR(pd_wdev->cdev)) { + pr_err("unable to register %s cooling device\n", gpd->name); + if (pd_wdev->dev) + pm_runtime_disable(pd_wdev->dev); + } + + return pd_wdev->cdev; +} +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(pd_wdev->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..ed9942b --- /dev/null +++ b/include/linux/pwr_domain_warming.h @@ -0,0 +1,31 @@ +// 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_node *node, + struct generic_pm_domain *gpd); + +void pwr_domain_warming_unregister(struct thermal_cooling_device *cdev); + +#else +static inline struct thermal_cooling_device * +pwr_domain_warming_register(struct device_node *node, + struct generic_pm_domain *gpd) +{ + 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__ */
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> --- drivers/thermal/Kconfig | 10 +++ drivers/thermal/Makefile | 2 + drivers/thermal/pwr_domain_warming.c | 136 +++++++++++++++++++++++++++++++++++ include/linux/pwr_domain_warming.h | 31 ++++++++ 4 files changed, 179 insertions(+) create mode 100644 drivers/thermal/pwr_domain_warming.c create mode 100644 include/linux/pwr_domain_warming.h