[PATCH v3 0/7]Introduce Power domain based warming device driver

[PATCH v3 0/7]Introduce Power domain based warming device driver

[Thara Gopinath]

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 late init ops for thermal cooling device that
is called after the cooling device is created and registered but before
binding it to a thermal zone for specific initializations.

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)

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: Add late init hook to cooling device ops
  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       |  10 ++
 arch/arm64/boot/dts/qcom/sdm845.dtsi               |   5 +
 drivers/base/power/domain.c                        |  37 ++++++
 drivers/soc/qcom/rpmhpd.c                          |  30 ++++-
 drivers/thermal/Kconfig                            |  10 ++
 drivers/thermal/Makefile                           |   2 +
 drivers/thermal/pwr_domain_warming.c               | 136 +++++++++++++++++++++
 drivers/thermal/thermal_core.c                     |  13 ++
 include/linux/pm_domain.h                          |  13 ++
 include/linux/pwr_domain_warming.h                 |  31 +++++
 include/linux/thermal.h                            |   1 +
 11 files changed, 287 insertions(+), 1 deletion(-)
 create mode 100644 drivers/thermal/pwr_domain_warming.c
 create mode 100644 include/linux/pwr_domain_warming.h

-- 
2.1.4

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

[Thara Gopinath]

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

These APIs are added to aid the implementation of a power domain as
a warming device. Linux kernel cooling device framework(into which
warming device can be plugged in) requires during initialization to be
provided with the maximum number of states that can be supported. When
a power domain acts as a warming device, the max state is the max number
of perfomrance states supported by the power domain. The cooling
device framework implements API to retrieve the current state of the
cooling device. This in turn translates to the current performance
state of the power domain.

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

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index cc85e87..507e530 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -408,6 +408,43 @@ int dev_pm_genpd_set_performance_state(struct device *dev, unsigned int state)
 }
 EXPORT_SYMBOL_GPL(dev_pm_genpd_set_performance_state);
 
+int dev_pm_genpd_get_performance_state(struct device *dev)
+{
+	struct generic_pm_domain *genpd;
+	unsigned int state;
+
+	genpd = dev_to_genpd_safe(dev);
+	if (IS_ERR(genpd))
+		return -ENODEV;
+
+	genpd_lock(genpd);
+	state = genpd->performance_state;
+	genpd_unlock(genpd);
+
+	return state;
+}
+EXPORT_SYMBOL_GPL(dev_pm_genpd_get_performance_state);
+
+int dev_pm_genpd_performance_state_count(struct device *dev)
+{
+	struct generic_pm_domain *genpd;
+	int count;
+
+	genpd = dev_to_genpd_safe(dev);
+	if (IS_ERR(genpd))
+		return -ENODEV;
+
+	if (unlikely(!genpd->get_performance_state_count))
+		return -EINVAL;
+
+	genpd_lock(genpd);
+	count = genpd->get_performance_state_count(genpd);
+	genpd_unlock(genpd);
+
+	return count;
+}
+EXPORT_SYMBOL_GPL(dev_pm_genpd_performance_state_count);
+
 static int _genpd_power_on(struct generic_pm_domain *genpd, bool timed)
 {
 	unsigned int state_idx = genpd->state_idx;
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index baf02ff..e88e57f 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -117,6 +117,7 @@ struct generic_pm_domain {
 						 struct dev_pm_opp *opp);
 	int (*set_performance_state)(struct generic_pm_domain *genpd,
 				     unsigned int state);
+	int (*get_performance_state_count)(struct generic_pm_domain *genpd);
 	struct gpd_dev_ops dev_ops;
 	s64 max_off_time_ns;	/* Maximum allowed "suspended" time. */
 	bool max_off_time_changed;
@@ -204,6 +205,8 @@ int pm_genpd_init(struct generic_pm_domain *genpd,
 		  struct dev_power_governor *gov, bool is_off);
 int pm_genpd_remove(struct generic_pm_domain *genpd);
 int dev_pm_genpd_set_performance_state(struct device *dev, unsigned int state);
+int dev_pm_genpd_get_performance_state(struct device *dev);
+int dev_pm_genpd_performance_state_count(struct device *dev);
 
 extern struct dev_power_governor simple_qos_governor;
 extern struct dev_power_governor pm_domain_always_on_gov;
@@ -251,6 +254,16 @@ static inline int dev_pm_genpd_set_performance_state(struct device *dev,
 	return -ENOTSUPP;
 }
 
+static inline int dev_pm_genpd_get_performance_state(struct device *dev)
+{
+	return -ENOTSUPP;
+}
+
+static inline int dev_pm_genpd_performance_state_count(struct device *dev)
+{
+	return -ENOTSUPP;
+}
+
 #define simple_qos_governor		(*(struct dev_power_governor *)(NULL))
 #define pm_domain_always_on_gov		(*(struct dev_power_governor *)(NULL))
 #endif
-- 
2.1.4

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

[Thara Gopinath]

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

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

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

[PATCH v3 3/7] thermal: core: Add late init hook to cooling device ops

[Thara Gopinath]

Add a hook in thermal_cooling_device_ops to be called after
the cooling device has been initialized and registered
but before binding it to a thermal zone.

In this patch series it is used to hook up a power domain
to the device pointer of cooling device.

It can be used for any other relevant late initializations
of a cooling device as well.

Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org>
---
 drivers/thermal/thermal_core.c | 13 +++++++++++++
 include/linux/thermal.h        |  1 +
 2 files changed, 14 insertions(+)

diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index 886e8fa..c2ecb73 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -994,6 +994,19 @@ __thermal_cooling_device_register(struct device_node *np,
 	list_add(&cdev->node, &thermal_cdev_list);
 	mutex_unlock(&thermal_list_lock);
 
+	/* Call into cdev late initialization if defined */
+	if (cdev->ops->late_init) {
+		result = cdev->ops->late_init(cdev);
+		if (result) {
+			ida_simple_remove(&thermal_cdev_ida, cdev->id);
+			put_device(&cdev->device);
+			mutex_lock(&thermal_list_lock);
+			list_del(&cdev->node);
+			mutex_unlock(&thermal_list_lock);
+			return ERR_PTR(result);
+		}
+	}
+
 	/* Update binding information for 'this' new cdev */
 	bind_cdev(cdev);
 
diff --git a/include/linux/thermal.h b/include/linux/thermal.h
index e45659c..e94b3de 100644
--- a/include/linux/thermal.h
+++ b/include/linux/thermal.h
@@ -125,6 +125,7 @@ struct thermal_cooling_device_ops {
 			   struct thermal_zone_device *, unsigned long, u32 *);
 	int (*power2state)(struct thermal_cooling_device *,
 			   struct thermal_zone_device *, u32, unsigned long *);
+	int (*late_init)(struct thermal_cooling_device *);
 };
 
 struct thermal_cooling_device {
-- 
2.1.4

[PATCH v3 4/7] thermal: Add generic power domain warming device driver.

[Thara Gopinath]

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;
+	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__ */
-- 
2.1.4

[PATCH v3 5/7] soc: qcom: Extend RPMh power controller driver to register warming devices.

[Thara Gopinath]

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>
---
 drivers/soc/qcom/rpmhpd.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/drivers/soc/qcom/rpmhpd.c b/drivers/soc/qcom/rpmhpd.c
index 9d37534..88ba615 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>
@@ -333,6 +334,7 @@ static int rpmhpd_probe(struct platform_device *pdev)
 	struct genpd_onecell_data *data;
 	struct rpmhpd **rpmhpds;
 	const struct rpmhpd_desc *desc;
+	struct device_node *child;
 
 	desc = of_device_get_match_data(dev);
 	if (!desc)
@@ -396,7 +398,24 @@ 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;
+
+	for_each_available_child_of_node(dev->of_node, child) {
+		if (!of_find_property(child, "#cooling-cells", NULL))
+			continue;
+
+		for (i = 0; i < num_pds; i++)
+			if (strcmp(child->name, rpmhpds[i]->pd.name) == 0)
+				break;
+		if (i == num_pds)
+			continue;
+		pwr_domain_warming_register(child, &rpmhpds[i]->pd);
+	}
+
+	return 0;
 }
 
 static struct platform_driver rpmhpd_driver = {
-- 
2.1.4

[PATCH v3 6/7] dt-bindings: soc: qcom: Extend RPMh power controller binding to describe thermal warming device

[Thara Gopinath]

RPMh power controller hosts mx domain that can be used as thermal
warming device. Add a sub-node to specify this.

Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org>
---
 Documentation/devicetree/bindings/power/qcom,rpmpd.txt | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/Documentation/devicetree/bindings/power/qcom,rpmpd.txt b/Documentation/devicetree/bindings/power/qcom,rpmpd.txt
index eb35b22..fff695d 100644
--- a/Documentation/devicetree/bindings/power/qcom,rpmpd.txt
+++ b/Documentation/devicetree/bindings/power/qcom,rpmpd.txt
@@ -18,6 +18,16 @@ Required Properties:
 Refer to <dt-bindings/power/qcom-rpmpd.h> for the level values for
 various OPPs for different platforms as well as Power domain indexes
 
+= SUBNODES
+RPMh alsp hosts power domains that can behave as thermal warming device.
+These are expressed as subnodes of the RPMh. The name of the node is used
+to identify the power domain and must therefor be "mx".
+
+- #cooling-cells:
+	Usage: optional
+	Value type: <u32>
+	Definition: must be 2
+
 Example: rpmh power domain controller and OPP table
 
 #include <dt-bindings/power/qcom-rpmhpd.h>
-- 
2.1.4

[PATCH v3 7/7] arm64: dts: qcom: Add mx power domain as thermal warming device.

[Thara Gopinath]

RPMh hosts mx power domain that can be used to warm up the SoC.
Add sub-node to rpmhpd node for mx to be recognized
as thermal warming device on sdm845.

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

diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
index 0222f48..0671c8a 100644
--- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
@@ -3788,6 +3788,11 @@
 						opp-level = <RPMH_REGULATOR_LEVEL_TURBO_L1>;
 					};
 				};
+
+				mx_cdev: mx {
+					#cooling-cells = <2>;
+					.name = "mx";
+				};
 			};
 
 			rsc_hlos: interconnect {
-- 
2.1.4

Re: [PATCH v3 4/7] thermal: Add generic power domain warming device driver.

[Ulf Hansson]

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

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

[Ulf Hansson]

On Wed, 16 Oct 2019 at 21:37, Thara Gopinath <thara.gopinath@linaro.org> wrote:
>
> Add two new APIs in the genpd framework,
> dev_pm_genpd_get_performance_state to return the current performance
> state of a power domain and dev_pm_genpd_performance_state_count to
> return the total number of performance states supported by a
> power domain. Since the genpd framework does not maintain
> a count of number of performance states supported by a power domain,
> introduce a new callback(.get_performance_state_count) that can be used
> to retrieve this information from power domain drivers.
>
> These APIs are added to aid the implementation of a power domain as
> a warming device. Linux kernel cooling device framework(into which
> warming device can be plugged in) requires during initialization to be
> provided with the maximum number of states that can be supported. When
> a power domain acts as a warming device, the max state is the max number
> of perfomrance states supported by the power domain. The cooling
> device framework implements API to retrieve the current state of the
> cooling device. This in turn translates to the current performance
> state of the power domain.
>
> Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org>

Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>

Kind regards
Uffe


> ---
>  drivers/base/power/domain.c | 37 +++++++++++++++++++++++++++++++++++++
>  include/linux/pm_domain.h   | 13 +++++++++++++
>  2 files changed, 50 insertions(+)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index cc85e87..507e530 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -408,6 +408,43 @@ int dev_pm_genpd_set_performance_state(struct device *dev, unsigned int state)
>  }
>  EXPORT_SYMBOL_GPL(dev_pm_genpd_set_performance_state);
>
> +int dev_pm_genpd_get_performance_state(struct device *dev)
> +{
> +       struct generic_pm_domain *genpd;
> +       unsigned int state;
> +
> +       genpd = dev_to_genpd_safe(dev);
> +       if (IS_ERR(genpd))
> +               return -ENODEV;
> +
> +       genpd_lock(genpd);
> +       state = genpd->performance_state;
> +       genpd_unlock(genpd);
> +
> +       return state;
> +}
> +EXPORT_SYMBOL_GPL(dev_pm_genpd_get_performance_state);
> +
> +int dev_pm_genpd_performance_state_count(struct device *dev)
> +{
> +       struct generic_pm_domain *genpd;
> +       int count;
> +
> +       genpd = dev_to_genpd_safe(dev);
> +       if (IS_ERR(genpd))
> +               return -ENODEV;
> +
> +       if (unlikely(!genpd->get_performance_state_count))
> +               return -EINVAL;
> +
> +       genpd_lock(genpd);
> +       count = genpd->get_performance_state_count(genpd);
> +       genpd_unlock(genpd);
> +
> +       return count;
> +}
> +EXPORT_SYMBOL_GPL(dev_pm_genpd_performance_state_count);
> +
>  static int _genpd_power_on(struct generic_pm_domain *genpd, bool timed)
>  {
>         unsigned int state_idx = genpd->state_idx;
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index baf02ff..e88e57f 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -117,6 +117,7 @@ struct generic_pm_domain {
>                                                  struct dev_pm_opp *opp);
>         int (*set_performance_state)(struct generic_pm_domain *genpd,
>                                      unsigned int state);
> +       int (*get_performance_state_count)(struct generic_pm_domain *genpd);
>         struct gpd_dev_ops dev_ops;
>         s64 max_off_time_ns;    /* Maximum allowed "suspended" time. */
>         bool max_off_time_changed;
> @@ -204,6 +205,8 @@ int pm_genpd_init(struct generic_pm_domain *genpd,
>                   struct dev_power_governor *gov, bool is_off);
>  int pm_genpd_remove(struct generic_pm_domain *genpd);
>  int dev_pm_genpd_set_performance_state(struct device *dev, unsigned int state);
> +int dev_pm_genpd_get_performance_state(struct device *dev);
> +int dev_pm_genpd_performance_state_count(struct device *dev);
>
>  extern struct dev_power_governor simple_qos_governor;
>  extern struct dev_power_governor pm_domain_always_on_gov;
> @@ -251,6 +254,16 @@ static inline int dev_pm_genpd_set_performance_state(struct device *dev,
>         return -ENOTSUPP;
>  }
>
> +static inline int dev_pm_genpd_get_performance_state(struct device *dev)
> +{
> +       return -ENOTSUPP;
> +}
> +
> +static inline int dev_pm_genpd_performance_state_count(struct device *dev)
> +{
> +       return -ENOTSUPP;
> +}
> +
>  #define simple_qos_governor            (*(struct dev_power_governor *)(NULL))
>  #define pm_domain_always_on_gov                (*(struct dev_power_governor *)(NULL))
>  #endif
> --
> 2.1.4
>

Re: [PATCH v3 6/7] dt-bindings: soc: qcom: Extend RPMh power controller binding to describe thermal warming device

[Ulf Hansson]

On Wed, 16 Oct 2019 at 21:37, Thara Gopinath <thara.gopinath@linaro.org> wrote:
>
> RPMh power controller hosts mx domain that can be used as thermal
> warming device. Add a sub-node to specify this.
>
> Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org>
> ---
>  Documentation/devicetree/bindings/power/qcom,rpmpd.txt | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/power/qcom,rpmpd.txt b/Documentation/devicetree/bindings/power/qcom,rpmpd.txt
> index eb35b22..fff695d 100644
> --- a/Documentation/devicetree/bindings/power/qcom,rpmpd.txt
> +++ b/Documentation/devicetree/bindings/power/qcom,rpmpd.txt
> @@ -18,6 +18,16 @@ Required Properties:
>  Refer to <dt-bindings/power/qcom-rpmpd.h> for the level values for
>  various OPPs for different platforms as well as Power domain indexes
>
> += SUBNODES
> +RPMh alsp hosts power domains that can behave as thermal warming device.
> +These are expressed as subnodes of the RPMh. The name of the node is used
> +to identify the power domain and must therefor be "mx".
> +
> +- #cooling-cells:
> +       Usage: optional
> +       Value type: <u32>
> +       Definition: must be 2
> +

Just wanted to express a minor thought about this. In general we use
subnodes of PM domain providers to represent the topology of PM
domains (subdomains), this is something different, which I guess is
fine.

I assume the #cooling-cells is here tells us this is not a PM domain
provider, but a "cooling device provider"?

Also, I wonder if it would be fine to specify "power-domains" here,
rather than using "name" as I think that is kind of awkward!?

>  Example: rpmh power domain controller and OPP table
>
>  #include <dt-bindings/power/qcom-rpmhpd.h>
> --
> 2.1.4
>

Kind regards
Uffe

Re: [PATCH v3 6/7] dt-bindings: soc: qcom: Extend RPMh power controller binding to describe thermal warming device

[Thara Gopinath]

Hello Ulf,
Thanks for the review!

On 10/17/2019 05:04 AM, Ulf Hansson wrote:
> On Wed, 16 Oct 2019 at 21:37, Thara Gopinath <thara.gopinath@linaro.org> wrote:
>>
>> RPMh power controller hosts mx domain that can be used as thermal
>> warming device. Add a sub-node to specify this.
>>
>> Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org>
>> ---
>>  Documentation/devicetree/bindings/power/qcom,rpmpd.txt | 10 ++++++++++
>>  1 file changed, 10 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/power/qcom,rpmpd.txt b/Documentation/devicetree/bindings/power/qcom,rpmpd.txt
>> index eb35b22..fff695d 100644
>> --- a/Documentation/devicetree/bindings/power/qcom,rpmpd.txt
>> +++ b/Documentation/devicetree/bindings/power/qcom,rpmpd.txt
>> @@ -18,6 +18,16 @@ Required Properties:
>>  Refer to <dt-bindings/power/qcom-rpmpd.h> for the level values for
>>  various OPPs for different platforms as well as Power domain indexes
>>
>> += SUBNODES
>> +RPMh alsp hosts power domains that can behave as thermal warming device.
>> +These are expressed as subnodes of the RPMh. The name of the node is used
>> +to identify the power domain and must therefor be "mx".
>> +
>> +- #cooling-cells:
>> +       Usage: optional
>> +       Value type: <u32>
>> +       Definition: must be 2
>> +
> 
> Just wanted to express a minor thought about this. In general we use
> subnodes of PM domain providers to represent the topology of PM
> domains (subdomains), this is something different, which I guess is
> fine.
> 
> I assume the #cooling-cells is here tells us this is not a PM domain
> provider, but a "cooling device provider"?
Yep.
> 
> Also, I wonder if it would be fine to specify "power-domains" here,
> rather than using "name" as I think that is kind of awkward!?
Do you mean "power-domain-names" ? I am using this to match against the
genpd names defined in the provider driver.

Warm Regards
Thara

Re: [PATCH v3 6/7] dt-bindings: soc: qcom: Extend RPMh power controller binding to describe thermal warming device

[Ulf Hansson]

On Thu, 17 Oct 2019 at 17:28, Thara Gopinath <thara.gopinath@linaro.org> wrote:
>
> Hello Ulf,
> Thanks for the review!
>
> On 10/17/2019 05:04 AM, Ulf Hansson wrote:
> > On Wed, 16 Oct 2019 at 21:37, Thara Gopinath <thara.gopinath@linaro.org> wrote:
> >>
> >> RPMh power controller hosts mx domain that can be used as thermal
> >> warming device. Add a sub-node to specify this.
> >>
> >> Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org>
> >> ---
> >>  Documentation/devicetree/bindings/power/qcom,rpmpd.txt | 10 ++++++++++
> >>  1 file changed, 10 insertions(+)
> >>
> >> diff --git a/Documentation/devicetree/bindings/power/qcom,rpmpd.txt b/Documentation/devicetree/bindings/power/qcom,rpmpd.txt
> >> index eb35b22..fff695d 100644
> >> --- a/Documentation/devicetree/bindings/power/qcom,rpmpd.txt
> >> +++ b/Documentation/devicetree/bindings/power/qcom,rpmpd.txt
> >> @@ -18,6 +18,16 @@ Required Properties:
> >>  Refer to <dt-bindings/power/qcom-rpmpd.h> for the level values for
> >>  various OPPs for different platforms as well as Power domain indexes
> >>
> >> += SUBNODES
> >> +RPMh alsp hosts power domains that can behave as thermal warming device.
> >> +These are expressed as subnodes of the RPMh. The name of the node is used
> >> +to identify the power domain and must therefor be "mx".
> >> +
> >> +- #cooling-cells:
> >> +       Usage: optional
> >> +       Value type: <u32>
> >> +       Definition: must be 2
> >> +
> >
> > Just wanted to express a minor thought about this. In general we use
> > subnodes of PM domain providers to represent the topology of PM
> > domains (subdomains), this is something different, which I guess is
> > fine.
> >
> > I assume the #cooling-cells is here tells us this is not a PM domain
> > provider, but a "cooling device provider"?
> Yep.
> >
> > Also, I wonder if it would be fine to specify "power-domains" here,
> > rather than using "name" as I think that is kind of awkward!?
> Do you mean "power-domain-names" ? I am using this to match against the
> genpd names defined in the provider driver.

No. If you are using "power-domains" it means that you allow to
describe the specifier for the provider.

From Linux point of view, it means you can use dev_pm_domain_attach()
to hook up the corresponding device with the PM domain.

Using "power-domain-names" is just to allow to specify a name rather
than an index, which makes sense if there is more than one index.
Perhaps you can state that the "power-domain-names" should be there
anyway, to be a little bit future proof if ever multiple index
(multiple PM domains).

Kind regards
Uffe

Re: [PATCH v3 4/7] thermal: Add generic power domain warming device driver.

[Thara Gopinath]

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.

-- 
Warm Regards
Thara

Re: [PATCH v3 6/7] dt-bindings: soc: qcom: Extend RPMh power controller binding to describe thermal warming device

[Thara Gopinath]

On 10/17/2019 11:43 AM, Ulf Hansson wrote:
> On Thu, 17 Oct 2019 at 17:28, Thara Gopinath <thara.gopinath@linaro.org> wrote:
>>
>> Hello Ulf,
>> Thanks for the review!
>>
>> On 10/17/2019 05:04 AM, Ulf Hansson wrote:
>>> On Wed, 16 Oct 2019 at 21:37, Thara Gopinath <thara.gopinath@linaro.org> wrote:
>>>>
>>>> RPMh power controller hosts mx domain that can be used as thermal
>>>> warming device. Add a sub-node to specify this.
>>>>
>>>> Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org>
>>>> ---
>>>>  Documentation/devicetree/bindings/power/qcom,rpmpd.txt | 10 ++++++++++
>>>>  1 file changed, 10 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/power/qcom,rpmpd.txt b/Documentation/devicetree/bindings/power/qcom,rpmpd.txt
>>>> index eb35b22..fff695d 100644
>>>> --- a/Documentation/devicetree/bindings/power/qcom,rpmpd.txt
>>>> +++ b/Documentation/devicetree/bindings/power/qcom,rpmpd.txt
>>>> @@ -18,6 +18,16 @@ Required Properties:
>>>>  Refer to <dt-bindings/power/qcom-rpmpd.h> for the level values for
>>>>  various OPPs for different platforms as well as Power domain indexes
>>>>
>>>> += SUBNODES
>>>> +RPMh alsp hosts power domains that can behave as thermal warming device.
>>>> +These are expressed as subnodes of the RPMh. The name of the node is used
>>>> +to identify the power domain and must therefor be "mx".
>>>> +
>>>> +- #cooling-cells:
>>>> +       Usage: optional
>>>> +       Value type: <u32>
>>>> +       Definition: must be 2
>>>> +
>>>
>>> Just wanted to express a minor thought about this. In general we use
>>> subnodes of PM domain providers to represent the topology of PM
>>> domains (subdomains), this is something different, which I guess is
>>> fine.
>>>
>>> I assume the #cooling-cells is here tells us this is not a PM domain
>>> provider, but a "cooling device provider"?
>> Yep.
>>>
>>> Also, I wonder if it would be fine to specify "power-domains" here,
>>> rather than using "name" as I think that is kind of awkward!?
>> Do you mean "power-domain-names" ? I am using this to match against the
>> genpd names defined in the provider driver.
> 
> No. If you are using "power-domains" it means that you allow to
> describe the specifier for the provider.
Yep. But won't this look funny in DT ? The provider node will have a sub
node with a power domain referencing to itself Like below: Is this ok ?

rpmhpd: power-controller {
                                compatible = "qcom,sdm845-rpmhpd";
                                #power-domain-cells = <1>;

			...
			...
				mx_cdev: mx {
                                        #cooling-cells = <2>;
                                        power-domains = <&rpmhpd	SDM845_MX>;
                                };
				
> 
> From Linux point of view, it means you can use dev_pm_domain_attach()
> to hook up the corresponding device with the PM domain.

Yes. Only the thermal framework does not populate cdev->dev->of_node.
But it should be a trivial thing to fix it. Also if I end up creating a
separate device, it should not matter.
> 
> Using "power-domain-names" is just to allow to specify a name rather
> than an index, which makes sense if there is more than one index.
> Perhaps you can state that the "power-domain-names" should be there
> anyway, to be a little bit future proof if ever multiple index
> (multiple PM domains).
> 
> Kind regards
> Uffe
> 


-- 
Warm Regards
Thara

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

[Thara Gopinath]

On 10/17/2019 04:49 AM, Ulf Hansson wrote:
> On Wed, 16 Oct 2019 at 21:37, Thara Gopinath <thara.gopinath@linaro.org> wrote:
>>
>> Add two new APIs in the genpd framework,
>> dev_pm_genpd_get_performance_state to return the current performance
>> state of a power domain and dev_pm_genpd_performance_state_count to
>> return the total number of performance states supported by a
>> power domain. Since the genpd framework does not maintain
>> a count of number of performance states supported by a power domain,
>> introduce a new callback(.get_performance_state_count) that can be used
>> to retrieve this information from power domain drivers.
>>
>> These APIs are added to aid the implementation of a power domain as
>> a warming device. Linux kernel cooling device framework(into which
>> warming device can be plugged in) requires during initialization to be
>> provided with the maximum number of states that can be supported. When
>> a power domain acts as a warming device, the max state is the max number
>> of perfomrance states supported by the power domain. The cooling
>> device framework implements API to retrieve the current state of the
>> cooling device. This in turn translates to the current performance
>> state of the power domain.
>>
>> Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org>
> 
> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
> 
Thanks Ulf! Do you think this patch be merged separate from the series.
Then I can drop it from the series.

-- 
Warm Regards
Thara

Re: [PATCH v3 7/7] arm64: dts: qcom: Add mx power domain as thermal warming device.

[Rob Herring]

On Wed, Oct 16, 2019 at 03:37:21PM -0400, Thara Gopinath wrote:
> RPMh hosts mx power domain that can be used to warm up the SoC.
> Add sub-node to rpmhpd node for mx to be recognized
> as thermal warming device on sdm845.
> 
> Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org>
> ---
>  arch/arm64/boot/dts/qcom/sdm845.dtsi | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> index 0222f48..0671c8a 100644
> --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> @@ -3788,6 +3788,11 @@
>  						opp-level = <RPMH_REGULATOR_LEVEL_TURBO_L1>;
>  					};
>  				};
> +
> +				mx_cdev: mx {
> +					#cooling-cells = <2>;
> +					.name = "mx";

Copy this from C code?

> +				};
>  			};
>  
>  			rsc_hlos: interconnect {
> -- 
> 2.1.4
> 

Re: [PATCH v3 6/7] dt-bindings: soc: qcom: Extend RPMh power controller binding to describe thermal warming device

[Rob Herring]

On Thu, Oct 17, 2019 at 12:10:15PM -0400, Thara Gopinath wrote:
> On 10/17/2019 11:43 AM, Ulf Hansson wrote:
> > On Thu, 17 Oct 2019 at 17:28, Thara Gopinath <thara.gopinath@linaro.org> wrote:
> >>
> >> Hello Ulf,
> >> Thanks for the review!
> >>
> >> On 10/17/2019 05:04 AM, Ulf Hansson wrote:
> >>> On Wed, 16 Oct 2019 at 21:37, Thara Gopinath <thara.gopinath@linaro.org> wrote:
> >>>>
> >>>> RPMh power controller hosts mx domain that can be used as thermal
> >>>> warming device. Add a sub-node to specify this.
> >>>>
> >>>> Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org>
> >>>> ---
> >>>>  Documentation/devicetree/bindings/power/qcom,rpmpd.txt | 10 ++++++++++
> >>>>  1 file changed, 10 insertions(+)
> >>>>
> >>>> diff --git a/Documentation/devicetree/bindings/power/qcom,rpmpd.txt b/Documentation/devicetree/bindings/power/qcom,rpmpd.txt
> >>>> index eb35b22..fff695d 100644
> >>>> --- a/Documentation/devicetree/bindings/power/qcom,rpmpd.txt
> >>>> +++ b/Documentation/devicetree/bindings/power/qcom,rpmpd.txt
> >>>> @@ -18,6 +18,16 @@ Required Properties:
> >>>>  Refer to <dt-bindings/power/qcom-rpmpd.h> for the level values for
> >>>>  various OPPs for different platforms as well as Power domain indexes
> >>>>
> >>>> += SUBNODES
> >>>> +RPMh alsp hosts power domains that can behave as thermal warming device.
> >>>> +These are expressed as subnodes of the RPMh. The name of the node is used
> >>>> +to identify the power domain and must therefor be "mx".
> >>>> +
> >>>> +- #cooling-cells:
> >>>> +       Usage: optional
> >>>> +       Value type: <u32>
> >>>> +       Definition: must be 2
> >>>> +
> >>>
> >>> Just wanted to express a minor thought about this. In general we use
> >>> subnodes of PM domain providers to represent the topology of PM
> >>> domains (subdomains), this is something different, which I guess is
> >>> fine.
> >>>
> >>> I assume the #cooling-cells is here tells us this is not a PM domain
> >>> provider, but a "cooling device provider"?
> >> Yep.
> >>>
> >>> Also, I wonder if it would be fine to specify "power-domains" here,
> >>> rather than using "name" as I think that is kind of awkward!?
> >> Do you mean "power-domain-names" ? I am using this to match against the
> >> genpd names defined in the provider driver.
> > 
> > No. If you are using "power-domains" it means that you allow to
> > describe the specifier for the provider.
> Yep. But won't this look funny in DT ? The provider node will have a sub
> node with a power domain referencing to itself Like below: Is this ok ?
> 
> rpmhpd: power-controller {
>                                 compatible = "qcom,sdm845-rpmhpd";
>                                 #power-domain-cells = <1>;
> 
> 			...
> 			...
> 				mx_cdev: mx {
>                                         #cooling-cells = <2>;
>                                         power-domains = <&rpmhpd	SDM845_MX>;
>                                 };
> 				

The whole concept here seems all wrong to me. Isn't it what's in the 
power domain that's the cooling device. A CPU power domain is not a 
cooling device, the CPU is. Or we wouldn't make a clock a cooling 
device, but what the clock drives.

Rob

Re: [PATCH v3 6/7] dt-bindings: soc: qcom: Extend RPMh power controller binding to describe thermal warming device

[Ulf Hansson]

On Tue, 29 Oct 2019 at 02:36, Rob Herring <robh@kernel.org> wrote:
>
> On Thu, Oct 17, 2019 at 12:10:15PM -0400, Thara Gopinath wrote:
> > On 10/17/2019 11:43 AM, Ulf Hansson wrote:
> > > On Thu, 17 Oct 2019 at 17:28, Thara Gopinath <thara.gopinath@linaro.org> wrote:
> > >>
> > >> Hello Ulf,
> > >> Thanks for the review!
> > >>
> > >> On 10/17/2019 05:04 AM, Ulf Hansson wrote:
> > >>> On Wed, 16 Oct 2019 at 21:37, Thara Gopinath <thara.gopinath@linaro.org> wrote:
> > >>>>
> > >>>> RPMh power controller hosts mx domain that can be used as thermal
> > >>>> warming device. Add a sub-node to specify this.
> > >>>>
> > >>>> Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org>
> > >>>> ---
> > >>>>  Documentation/devicetree/bindings/power/qcom,rpmpd.txt | 10 ++++++++++
> > >>>>  1 file changed, 10 insertions(+)
> > >>>>
> > >>>> diff --git a/Documentation/devicetree/bindings/power/qcom,rpmpd.txt b/Documentation/devicetree/bindings/power/qcom,rpmpd.txt
> > >>>> index eb35b22..fff695d 100644
> > >>>> --- a/Documentation/devicetree/bindings/power/qcom,rpmpd.txt
> > >>>> +++ b/Documentation/devicetree/bindings/power/qcom,rpmpd.txt
> > >>>> @@ -18,6 +18,16 @@ Required Properties:
> > >>>>  Refer to <dt-bindings/power/qcom-rpmpd.h> for the level values for
> > >>>>  various OPPs for different platforms as well as Power domain indexes
> > >>>>
> > >>>> += SUBNODES
> > >>>> +RPMh alsp hosts power domains that can behave as thermal warming device.
> > >>>> +These are expressed as subnodes of the RPMh. The name of the node is used
> > >>>> +to identify the power domain and must therefor be "mx".
> > >>>> +
> > >>>> +- #cooling-cells:
> > >>>> +       Usage: optional
> > >>>> +       Value type: <u32>
> > >>>> +       Definition: must be 2
> > >>>> +
> > >>>
> > >>> Just wanted to express a minor thought about this. In general we use
> > >>> subnodes of PM domain providers to represent the topology of PM
> > >>> domains (subdomains), this is something different, which I guess is
> > >>> fine.
> > >>>
> > >>> I assume the #cooling-cells is here tells us this is not a PM domain
> > >>> provider, but a "cooling device provider"?
> > >> Yep.
> > >>>
> > >>> Also, I wonder if it would be fine to specify "power-domains" here,
> > >>> rather than using "name" as I think that is kind of awkward!?
> > >> Do you mean "power-domain-names" ? I am using this to match against the
> > >> genpd names defined in the provider driver.
> > >
> > > No. If you are using "power-domains" it means that you allow to
> > > describe the specifier for the provider.
> > Yep. But won't this look funny in DT ? The provider node will have a sub
> > node with a power domain referencing to itself Like below: Is this ok ?
> >
> > rpmhpd: power-controller {
> >                                 compatible = "qcom,sdm845-rpmhpd";
> >                                 #power-domain-cells = <1>;
> >
> >                       ...
> >                       ...
> >                               mx_cdev: mx {
> >                                         #cooling-cells = <2>;
> >                                         power-domains = <&rpmhpd      SDM845_MX>;
> >                                 };
> >
>
> The whole concept here seems all wrong to me. Isn't it what's in the
> power domain that's the cooling device. A CPU power domain is not a
> cooling device, the CPU is. Or we wouldn't make a clock a cooling
> device, but what the clock drives.

Well, I don't think that's entirely correct description either.

As I see it, it's really the actual PM domain (that manages voltages
for a power island), that needs to stay in full power state and
increase its voltage level, as to warm up some of the silicon. It's
not a regular device, but more a characteristics of how the PM domain
can be used.

Kind regards
Uffe

Re: [PATCH v3 6/7] dt-bindings: soc: qcom: Extend RPMh power controller binding to describe thermal warming device

[Rob Herring]

On Tue, Oct 29, 2019 at 5:07 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Tue, 29 Oct 2019 at 02:36, Rob Herring <robh@kernel.org> wrote:
> >
> > On Thu, Oct 17, 2019 at 12:10:15PM -0400, Thara Gopinath wrote:
> > > On 10/17/2019 11:43 AM, Ulf Hansson wrote:
> > > > On Thu, 17 Oct 2019 at 17:28, Thara Gopinath <thara.gopinath@linaro.org> wrote:
> > > >>
> > > >> Hello Ulf,
> > > >> Thanks for the review!
> > > >>
> > > >> On 10/17/2019 05:04 AM, Ulf Hansson wrote:
> > > >>> On Wed, 16 Oct 2019 at 21:37, Thara Gopinath <thara.gopinath@linaro.org> wrote:
> > > >>>>
> > > >>>> RPMh power controller hosts mx domain that can be used as thermal
> > > >>>> warming device. Add a sub-node to specify this.
> > > >>>>
> > > >>>> Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org>
> > > >>>> ---
> > > >>>>  Documentation/devicetree/bindings/power/qcom,rpmpd.txt | 10 ++++++++++
> > > >>>>  1 file changed, 10 insertions(+)
> > > >>>>
> > > >>>> diff --git a/Documentation/devicetree/bindings/power/qcom,rpmpd.txt b/Documentation/devicetree/bindings/power/qcom,rpmpd.txt
> > > >>>> index eb35b22..fff695d 100644
> > > >>>> --- a/Documentation/devicetree/bindings/power/qcom,rpmpd.txt
> > > >>>> +++ b/Documentation/devicetree/bindings/power/qcom,rpmpd.txt
> > > >>>> @@ -18,6 +18,16 @@ Required Properties:
> > > >>>>  Refer to <dt-bindings/power/qcom-rpmpd.h> for the level values for
> > > >>>>  various OPPs for different platforms as well as Power domain indexes
> > > >>>>
> > > >>>> += SUBNODES
> > > >>>> +RPMh alsp hosts power domains that can behave as thermal warming device.
> > > >>>> +These are expressed as subnodes of the RPMh. The name of the node is used
> > > >>>> +to identify the power domain and must therefor be "mx".
> > > >>>> +
> > > >>>> +- #cooling-cells:
> > > >>>> +       Usage: optional
> > > >>>> +       Value type: <u32>
> > > >>>> +       Definition: must be 2
> > > >>>> +
> > > >>>
> > > >>> Just wanted to express a minor thought about this. In general we use
> > > >>> subnodes of PM domain providers to represent the topology of PM
> > > >>> domains (subdomains), this is something different, which I guess is
> > > >>> fine.
> > > >>>
> > > >>> I assume the #cooling-cells is here tells us this is not a PM domain
> > > >>> provider, but a "cooling device provider"?
> > > >> Yep.
> > > >>>
> > > >>> Also, I wonder if it would be fine to specify "power-domains" here,
> > > >>> rather than using "name" as I think that is kind of awkward!?
> > > >> Do you mean "power-domain-names" ? I am using this to match against the
> > > >> genpd names defined in the provider driver.
> > > >
> > > > No. If you are using "power-domains" it means that you allow to
> > > > describe the specifier for the provider.
> > > Yep. But won't this look funny in DT ? The provider node will have a sub
> > > node with a power domain referencing to itself Like below: Is this ok ?
> > >
> > > rpmhpd: power-controller {
> > >                                 compatible = "qcom,sdm845-rpmhpd";
> > >                                 #power-domain-cells = <1>;
> > >
> > >                       ...
> > >                       ...
> > >                               mx_cdev: mx {
> > >                                         #cooling-cells = <2>;
> > >                                         power-domains = <&rpmhpd      SDM845_MX>;
> > >                                 };
> > >
> >
> > The whole concept here seems all wrong to me. Isn't it what's in the
> > power domain that's the cooling device. A CPU power domain is not a
> > cooling device, the CPU is. Or we wouldn't make a clock a cooling
> > device, but what the clock drives.
>
> Well, I don't think that's entirely correct description either.
>
> As I see it, it's really the actual PM domain (that manages voltages
> for a power island), that needs to stay in full power state and
> increase its voltage level, as to warm up some of the silicon. It's
> not a regular device, but more a characteristics of how the PM domain
> can be used.

First I've heard of Si needing warming...

I think I'd just expect the power domain provider to know which
domains to power on then.

Rob

Re: [PATCH v3 6/7] dt-bindings: soc: qcom: Extend RPMh power controller binding to describe thermal warming device

[Ulf Hansson]

On Tue, 29 Oct 2019 at 21:16, Rob Herring <robh@kernel.org> wrote:
>
> On Tue, Oct 29, 2019 at 5:07 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> >
> > On Tue, 29 Oct 2019 at 02:36, Rob Herring <robh@kernel.org> wrote:
> > >
> > > On Thu, Oct 17, 2019 at 12:10:15PM -0400, Thara Gopinath wrote:
> > > > On 10/17/2019 11:43 AM, Ulf Hansson wrote:
> > > > > On Thu, 17 Oct 2019 at 17:28, Thara Gopinath <thara.gopinath@linaro.org> wrote:
> > > > >>
> > > > >> Hello Ulf,
> > > > >> Thanks for the review!
> > > > >>
> > > > >> On 10/17/2019 05:04 AM, Ulf Hansson wrote:
> > > > >>> On Wed, 16 Oct 2019 at 21:37, Thara Gopinath <thara.gopinath@linaro.org> wrote:
> > > > >>>>
> > > > >>>> RPMh power controller hosts mx domain that can be used as thermal
> > > > >>>> warming device. Add a sub-node to specify this.
> > > > >>>>
> > > > >>>> Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org>
> > > > >>>> ---
> > > > >>>>  Documentation/devicetree/bindings/power/qcom,rpmpd.txt | 10 ++++++++++
> > > > >>>>  1 file changed, 10 insertions(+)
> > > > >>>>
> > > > >>>> diff --git a/Documentation/devicetree/bindings/power/qcom,rpmpd.txt b/Documentation/devicetree/bindings/power/qcom,rpmpd.txt
> > > > >>>> index eb35b22..fff695d 100644
> > > > >>>> --- a/Documentation/devicetree/bindings/power/qcom,rpmpd.txt
> > > > >>>> +++ b/Documentation/devicetree/bindings/power/qcom,rpmpd.txt
> > > > >>>> @@ -18,6 +18,16 @@ Required Properties:
> > > > >>>>  Refer to <dt-bindings/power/qcom-rpmpd.h> for the level values for
> > > > >>>>  various OPPs for different platforms as well as Power domain indexes
> > > > >>>>
> > > > >>>> += SUBNODES
> > > > >>>> +RPMh alsp hosts power domains that can behave as thermal warming device.
> > > > >>>> +These are expressed as subnodes of the RPMh. The name of the node is used
> > > > >>>> +to identify the power domain and must therefor be "mx".
> > > > >>>> +
> > > > >>>> +- #cooling-cells:
> > > > >>>> +       Usage: optional
> > > > >>>> +       Value type: <u32>
> > > > >>>> +       Definition: must be 2
> > > > >>>> +
> > > > >>>
> > > > >>> Just wanted to express a minor thought about this. In general we use
> > > > >>> subnodes of PM domain providers to represent the topology of PM
> > > > >>> domains (subdomains), this is something different, which I guess is
> > > > >>> fine.
> > > > >>>
> > > > >>> I assume the #cooling-cells is here tells us this is not a PM domain
> > > > >>> provider, but a "cooling device provider"?
> > > > >> Yep.
> > > > >>>
> > > > >>> Also, I wonder if it would be fine to specify "power-domains" here,
> > > > >>> rather than using "name" as I think that is kind of awkward!?
> > > > >> Do you mean "power-domain-names" ? I am using this to match against the
> > > > >> genpd names defined in the provider driver.
> > > > >
> > > > > No. If you are using "power-domains" it means that you allow to
> > > > > describe the specifier for the provider.
> > > > Yep. But won't this look funny in DT ? The provider node will have a sub
> > > > node with a power domain referencing to itself Like below: Is this ok ?
> > > >
> > > > rpmhpd: power-controller {
> > > >                                 compatible = "qcom,sdm845-rpmhpd";
> > > >                                 #power-domain-cells = <1>;
> > > >
> > > >                       ...
> > > >                       ...
> > > >                               mx_cdev: mx {
> > > >                                         #cooling-cells = <2>;
> > > >                                         power-domains = <&rpmhpd      SDM845_MX>;
> > > >                                 };
> > > >
> > >
> > > The whole concept here seems all wrong to me. Isn't it what's in the
> > > power domain that's the cooling device. A CPU power domain is not a
> > > cooling device, the CPU is. Or we wouldn't make a clock a cooling
> > > device, but what the clock drives.
> >
> > Well, I don't think that's entirely correct description either.
> >
> > As I see it, it's really the actual PM domain (that manages voltages
> > for a power island), that needs to stay in full power state and
> > increase its voltage level, as to warm up some of the silicon. It's
> > not a regular device, but more a characteristics of how the PM domain
> > can be used.
>
> First I've heard of Si needing warming...

I guess people go to cooler places with their devices. :-)

>
> I think I'd just expect the power domain provider to know which
> domains to power on then.

Yeah, I agree. This seems reasonable.

Thanks!

Kind regards
Uffe

Re: [PATCH v3 7/7] arm64: dts: qcom: Add mx power domain as thermal warming device.

[Thara Gopinath]

On 10/28/2019 09:31 PM, Rob Herring wrote:
> On Wed, Oct 16, 2019 at 03:37:21PM -0400, Thara Gopinath wrote:
>> RPMh hosts mx power domain that can be used to warm up the SoC.
>> Add sub-node to rpmhpd node for mx to be recognized
>> as thermal warming device on sdm845.
>>
>> Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org>
>> ---
>>  arch/arm64/boot/dts/qcom/sdm845.dtsi | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
>> index 0222f48..0671c8a 100644
>> --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
>> @@ -3788,6 +3788,11 @@
>>  						opp-level = <RPMH_REGULATOR_LEVEL_TURBO_L1>;
>>  					};
>>  				};
>> +
>> +				mx_cdev: mx {
>> +					#cooling-cells = <2>;
>> +					.name = "mx";
> 
> Copy this from C code?
Hi Rob,

What do you mean ?

> 
>> +				};
>>  			};
>>  
>>  			rsc_hlos: interconnect {
>> -- 
>> 2.1.4
>>


-- 
Warm Regards
Thara

Re: [PATCH v3 6/7] dt-bindings: soc: qcom: Extend RPMh power controller binding to describe thermal warming device

[Thara Gopinath]

Hi Rob,

Thanks for the review.

On 10/29/2019 04:16 PM, Rob Herring wrote:
> On Tue, Oct 29, 2019 at 5:07 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>
>> On Tue, 29 Oct 2019 at 02:36, Rob Herring <robh@kernel.org> wrote:
>>>
>>> On Thu, Oct 17, 2019 at 12:10:15PM -0400, Thara Gopinath wrote:
>>>> On 10/17/2019 11:43 AM, Ulf Hansson wrote:
>>>>> On Thu, 17 Oct 2019 at 17:28, Thara Gopinath <thara.gopinath@linaro.org> wrote:
>>>>>>
>>>>>> Hello Ulf,
>>>>>> Thanks for the review!
>>>>>>
>>>>>> On 10/17/2019 05:04 AM, Ulf Hansson wrote:
>>>>>>> On Wed, 16 Oct 2019 at 21:37, Thara Gopinath <thara.gopinath@linaro.org> wrote:
>>>>>>>>
>>>>>>>> RPMh power controller hosts mx domain that can be used as thermal
>>>>>>>> warming device. Add a sub-node to specify this.
>>>>>>>>
>>>>>>>> Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org>
>>>>>>>> ---
>>>>>>>>  Documentation/devicetree/bindings/power/qcom,rpmpd.txt | 10 ++++++++++
>>>>>>>>  1 file changed, 10 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/Documentation/devicetree/bindings/power/qcom,rpmpd.txt b/Documentation/devicetree/bindings/power/qcom,rpmpd.txt
>>>>>>>> index eb35b22..fff695d 100644
>>>>>>>> --- a/Documentation/devicetree/bindings/power/qcom,rpmpd.txt
>>>>>>>> +++ b/Documentation/devicetree/bindings/power/qcom,rpmpd.txt
>>>>>>>> @@ -18,6 +18,16 @@ Required Properties:
>>>>>>>>  Refer to <dt-bindings/power/qcom-rpmpd.h> for the level values for
>>>>>>>>  various OPPs for different platforms as well as Power domain indexes
>>>>>>>>
>>>>>>>> += SUBNODES
>>>>>>>> +RPMh alsp hosts power domains that can behave as thermal warming device.
>>>>>>>> +These are expressed as subnodes of the RPMh. The name of the node is used
>>>>>>>> +to identify the power domain and must therefor be "mx".
>>>>>>>> +
>>>>>>>> +- #cooling-cells:
>>>>>>>> +       Usage: optional
>>>>>>>> +       Value type: <u32>
>>>>>>>> +       Definition: must be 2
>>>>>>>> +
>>>>>>>
>>>>>>> Just wanted to express a minor thought about this. In general we use
>>>>>>> subnodes of PM domain providers to represent the topology of PM
>>>>>>> domains (subdomains), this is something different, which I guess is
>>>>>>> fine.
>>>>>>>
>>>>>>> I assume the #cooling-cells is here tells us this is not a PM domain
>>>>>>> provider, but a "cooling device provider"?
>>>>>> Yep.
>>>>>>>
>>>>>>> Also, I wonder if it would be fine to specify "power-domains" here,
>>>>>>> rather than using "name" as I think that is kind of awkward!?
>>>>>> Do you mean "power-domain-names" ? I am using this to match against the
>>>>>> genpd names defined in the provider driver.
>>>>>
>>>>> No. If you are using "power-domains" it means that you allow to
>>>>> describe the specifier for the provider.
>>>> Yep. But won't this look funny in DT ? The provider node will have a sub
>>>> node with a power domain referencing to itself Like below: Is this ok ?
>>>>
>>>> rpmhpd: power-controller {
>>>>                                 compatible = "qcom,sdm845-rpmhpd";
>>>>                                 #power-domain-cells = <1>;
>>>>
>>>>                       ...
>>>>                       ...
>>>>                               mx_cdev: mx {
>>>>                                         #cooling-cells = <2>;
>>>>                                         power-domains = <&rpmhpd      SDM845_MX>;
>>>>                                 };
>>>>
>>>
>>> The whole concept here seems all wrong to me. Isn't it what's in the
>>> power domain that's the cooling device. A CPU power domain is not a
>>> cooling device, the CPU is. Or we wouldn't make a clock a cooling
>>> device, but what the clock drives.
>>
>> Well, I don't think that's entirely correct description either.
>>
>> As I see it, it's really the actual PM domain (that manages voltages
>> for a power island), that needs to stay in full power state and
>> increase its voltage level, as to warm up some of the silicon. It's
>> not a regular device, but more a characteristics of how the PM domain
>> can be used.
> 
> First I've heard of Si needing warming...
Cold regions and non-closing of circuits is what I am told.
> 
> I think I'd just expect the power domain provider to know which
> domains to power on then.
I will just retain #cooling-cells in the power domain provider and let
the driver identify the actual power domains.

> 
> Rob
> 


-- 
Warm Regards
Thara

Re: [PATCH v3 3/7] thermal: core: Add late init hook to cooling device ops

[Amit Kucheria]

On Thu, Oct 17, 2019 at 1:07 AM Thara Gopinath
<thara.gopinath@linaro.org> wrote:
>
> Add a hook in thermal_cooling_device_ops to be called after
> the cooling device has been initialized and registered
> but before binding it to a thermal zone.
>
> In this patch series it is used to hook up a power domain
> to the device pointer of cooling device.
>
> It can be used for any other relevant late initializations
> of a cooling device as well.

Please describe WHY this hook is needed.

> Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org>
> ---
>  drivers/thermal/thermal_core.c | 13 +++++++++++++
>  include/linux/thermal.h        |  1 +
>  2 files changed, 14 insertions(+)
>
> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
> index 886e8fa..c2ecb73 100644
> --- a/drivers/thermal/thermal_core.c
> +++ b/drivers/thermal/thermal_core.c
> @@ -994,6 +994,19 @@ __thermal_cooling_device_register(struct device_node *np,
>         list_add(&cdev->node, &thermal_cdev_list);
>         mutex_unlock(&thermal_list_lock);
>
> +       /* Call into cdev late initialization if defined */
> +       if (cdev->ops->late_init) {
> +               result = cdev->ops->late_init(cdev);
> +               if (result) {
> +                       ida_simple_remove(&thermal_cdev_ida, cdev->id);
> +                       put_device(&cdev->device);
> +                       mutex_lock(&thermal_list_lock);
> +                       list_del(&cdev->node);
> +                       mutex_unlock(&thermal_list_lock);
> +                       return ERR_PTR(result);
> +               }
> +       }
> +
>         /* Update binding information for 'this' new cdev */
>         bind_cdev(cdev);
>
> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> index e45659c..e94b3de 100644
> --- a/include/linux/thermal.h
> +++ b/include/linux/thermal.h
> @@ -125,6 +125,7 @@ struct thermal_cooling_device_ops {
>                            struct thermal_zone_device *, unsigned long, u32 *);
>         int (*power2state)(struct thermal_cooling_device *,
>                            struct thermal_zone_device *, u32, unsigned long *);
> +       int (*late_init)(struct thermal_cooling_device *);
>  };
>
>  struct thermal_cooling_device {
> --
> 2.1.4
>

Re: [PATCH v3 3/7] thermal: core: Add late init hook to cooling device ops

[Thara Gopinath]

On 12/03/2019 12:04 PM, Amit Kucheria wrote:
> On Thu, Oct 17, 2019 at 1:07 AM Thara Gopinath
> <thara.gopinath@linaro.org> wrote:
>>
>> Add a hook in thermal_cooling_device_ops to be called after
>> the cooling device has been initialized and registered
>> but before binding it to a thermal zone.
>>
>> In this patch series it is used to hook up a power domain
>> to the device pointer of cooling device.
>>
>> It can be used for any other relevant late initializations
>> of a cooling device as well.
> 
> Please describe WHY this hook is needed.
Hi Amit,

Thanks for the review. This patch is no longer relevant.
I had posted a v4 of this series based on other review comments[1]

1. https://lkml.org/lkml/2019/11/20/355

-- 
Warm Regards
Thara

Re: [PATCH v3 3/7] thermal: core: Add late init hook to cooling device ops

[Amit Kucheria]

On Tue, Dec 3, 2019 at 10:34 PM Amit Kucheria
<amit.kucheria@verdurent.com> wrote:
>
> On Thu, Oct 17, 2019 at 1:07 AM Thara Gopinath
> <thara.gopinath@linaro.org> wrote:
> >
> > Add a hook in thermal_cooling_device_ops to be called after
> > the cooling device has been initialized and registered
> > but before binding it to a thermal zone.
> >
> > In this patch series it is used to hook up a power domain
> > to the device pointer of cooling device.
> >
> > It can be used for any other relevant late initializations
> > of a cooling device as well.
>
> Please describe WHY this hook is needed.

Just noticed you dropped this for v4. Nevermind.

> > Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org>
> > ---
> >  drivers/thermal/thermal_core.c | 13 +++++++++++++
> >  include/linux/thermal.h        |  1 +
> >  2 files changed, 14 insertions(+)
> >
> > diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
> > index 886e8fa..c2ecb73 100644
> > --- a/drivers/thermal/thermal_core.c
> > +++ b/drivers/thermal/thermal_core.c
> > @@ -994,6 +994,19 @@ __thermal_cooling_device_register(struct device_node *np,
> >         list_add(&cdev->node, &thermal_cdev_list);
> >         mutex_unlock(&thermal_list_lock);
> >
> > +       /* Call into cdev late initialization if defined */
> > +       if (cdev->ops->late_init) {
> > +               result = cdev->ops->late_init(cdev);
> > +               if (result) {
> > +                       ida_simple_remove(&thermal_cdev_ida, cdev->id);
> > +                       put_device(&cdev->device);
> > +                       mutex_lock(&thermal_list_lock);
> > +                       list_del(&cdev->node);
> > +                       mutex_unlock(&thermal_list_lock);
> > +                       return ERR_PTR(result);
> > +               }
> > +       }
> > +
> >         /* Update binding information for 'this' new cdev */
> >         bind_cdev(cdev);
> >
> > diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> > index e45659c..e94b3de 100644
> > --- a/include/linux/thermal.h
> > +++ b/include/linux/thermal.h
> > @@ -125,6 +125,7 @@ struct thermal_cooling_device_ops {
> >                            struct thermal_zone_device *, unsigned long, u32 *);
> >         int (*power2state)(struct thermal_cooling_device *,
> >                            struct thermal_zone_device *, u32, unsigned long *);
> > +       int (*late_init)(struct thermal_cooling_device *);
> >  };
> >
> >  struct thermal_cooling_device {
> > --
> > 2.1.4
> >