Certain resources modeled as a generic power domain in linux kernel can be
used to warm up the SoC (mx power domain on sdm845) if the temperature
falls below certain threshold. These power domains can be considered as
thermal warming devices. (opposite of thermal cooling devices).
In kernel, these warming devices can be modeled as a thermal cooling
device. Since linux kernel today has no instance of a resource modeled as
a power domain acting as a thermal warming device, a generic power domain
based thermal warming device driver that can be used pan-Socs is the
approach taken in this patch series. Since thermal warming devices can be
thought of as the mirror opposite of thermal cooling devices, this patch
series re-uses thermal cooling device framework. To use these power
domains as warming devices require further tweaks in the thermal framework
which are out of scope of this patch series. These tweaks have been posted
as a separate series[1].
The first patch in this series extends the genpd framework to export out
the performance states of a power domain so that when a power domain is
modeled as a cooling device, the number of possible states and current
state of the cooling device can be retrieved from the genpd framework.
The second patch implements the newly added genpd callback for Qualcomm
RPMH power domain driver which hosts the mx power domain.
The third patch introduces a new cooling device register API that allows
a parent to be specified for the cooling device.
The fourth patch introduces the generic power domain warming device
driver.
The fifth patch extends Qualcomm RPMh power controller driver to register
mx power domain as a thermal warming device in the kernel.
The sixth patch describes the dt binding extensions for mx power domain to
be a thermal warming device.
The seventh patch introduces the DT entreis for sdm845 to register mx
power domain as a thermal warming device.
v1->v2:
- Rename the patch series from "qcom: Model RPMH power domains as
thermal cooling devices" to "Introduce Power domain based
thermal warming devices" as it is more appropriate.
- Introduce a new patch(patch 3) describing the dt-bindings for
generic power domain warming device.
- Patch specific changes mentioned in respective patches.
v2->v3:
- Changed power domain warming device from a virtual device node
entry in DT to being a subnode of power domain controller
binding following Rob's review comments.
- Implemented Ulf's review comments.
- The changes above introduced two new patches (patch 3 and 4)
v3->v4:
- Dropped late_init hook in cooling device ops. Instead introduced
a new cooling device register API that allows to define a parent
for the cooling device.
- Patch specific changes mentioned in respective patches.
1. https://lkml.org/lkml/2019/9/18/1180
Thara Gopinath (7):
PM/Domains: Add support for retrieving genpd performance states
information
soc: qcom: rpmhpd: Introduce function to retrieve power domain
performance state count
thermal: core: Allow cooling devices to register a parent.
thermal: Add generic power domain warming device driver.
soc: qcom: Extend RPMh power controller driver to register warming
devices.
dt-bindings: soc: qcom: Extend RPMh power controller binding to
describe thermal warming device
arm64: dts: qcom: Add mx power domain as thermal warming device.
.../devicetree/bindings/power/qcom,rpmpd.txt | 5 +
arch/arm64/boot/dts/qcom/sdm845.dtsi | 5 +
drivers/base/power/domain.c | 37 ++++++
drivers/soc/qcom/rpmhpd.c | 47 ++++++-
drivers/thermal/Kconfig | 10 ++
drivers/thermal/Makefile | 2 +
drivers/thermal/pwr_domain_warming.c | 138 +++++++++++++++++++++
drivers/thermal/thermal_core.c | 22 +++-
include/linux/pm_domain.h | 13 ++
include/linux/pwr_domain_warming.h | 29 +++++
include/linux/thermal.h | 15 +++
11 files changed, 319 insertions(+), 4 deletions(-)
create mode 100644 drivers/thermal/pwr_domain_warming.c
create mode 100644 include/linux/pwr_domain_warming.h
--
2.1.4
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 <[email protected]>
Reviewed-by: Ulf Hansson <[email protected]>
---
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
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 <[email protected]>
---
v3->v4:
- Introduce a boolean value is_warming_dev in rpmhpd structure to
indicate if a generic power domain can be used as a warming
device or not.With this change, device tree no longer has to
specify which power domain inside the rpmh power domain provider
is a warming device.
- Move registering of warming devices into a late initcall to
ensure that warming devices are registerd after thermal
framework is initialized.
drivers/soc/qcom/rpmhpd.c | 38 +++++++++++++++++++++++++++++++++++++-
1 file changed, 37 insertions(+), 1 deletion(-)
diff --git a/drivers/soc/qcom/rpmhpd.c b/drivers/soc/qcom/rpmhpd.c
index 9d37534..5666d1f 100644
--- a/drivers/soc/qcom/rpmhpd.c
+++ b/drivers/soc/qcom/rpmhpd.c
@@ -11,6 +11,7 @@
#include <linux/of_device.h>
#include <linux/platform_device.h>
#include <linux/pm_opp.h>
+#include <linux/pwr_domain_warming.h>
#include <soc/qcom/cmd-db.h>
#include <soc/qcom/rpmh.h>
#include <dt-bindings/power/qcom-rpmpd.h>
@@ -48,6 +49,7 @@ struct rpmhpd {
bool enabled;
const char *res_name;
u32 addr;
+ bool is_warming_dev;
};
struct rpmhpd_desc {
@@ -55,6 +57,8 @@ struct rpmhpd_desc {
size_t num_pds;
};
+const struct rpmhpd_desc *global_desc;
+
static DEFINE_MUTEX(rpmhpd_lock);
/* SDM845 RPMH powerdomains */
@@ -89,6 +93,7 @@ static struct rpmhpd sdm845_mx = {
.pd = { .name = "mx", },
.peer = &sdm845_mx_ao,
.res_name = "mx.lvl",
+ .is_warming_dev = true,
};
static struct rpmhpd sdm845_mx_ao = {
@@ -396,7 +401,14 @@ static int rpmhpd_probe(struct platform_device *pdev)
&rpmhpds[i]->pd);
}
- return of_genpd_add_provider_onecell(pdev->dev.of_node, data);
+ ret = of_genpd_add_provider_onecell(pdev->dev.of_node, data);
+
+ if (ret)
+ return ret;
+
+ global_desc = desc;
+
+ return 0;
}
static struct platform_driver rpmhpd_driver = {
@@ -413,3 +425,27 @@ static int __init rpmhpd_init(void)
return platform_driver_register(&rpmhpd_driver);
}
core_initcall(rpmhpd_init);
+
+static int __init rpmhpd_init_warming_device(void)
+{
+ size_t num_pds;
+ struct rpmhpd **rpmhpds;
+ int i;
+
+ if (!global_desc)
+ return -EINVAL;
+
+ rpmhpds = global_desc->rpmhpds;
+ num_pds = global_desc->num_pds;
+
+ if (!of_find_property(rpmhpds[0]->dev->of_node, "#cooling-cells", NULL))
+ return 0;
+
+ for (i = 0; i < num_pds; i++)
+ if (rpmhpds[i]->is_warming_dev)
+ pwr_domain_warming_register(rpmhpds[i]->dev,
+ rpmhpds[i]->res_name, i);
+
+ return 0;
+}
+late_initcall(rpmhpd_init_warming_device);
--
2.1.4
RPMh power controller hosts mx domain that can be used as thermal warming
device. Add #cooling-cells property to the power domain provider node to
indicate this.
Signed-off-by: Thara Gopinath <[email protected]>
---
v3->v4:
- Removed subnode to indicate that mx power domain is a warming
device. Instead #cooling-cells is used as a power domain
provider property to indicate if the provider hosts a power
domain that can be used as a warming device.
Documentation/devicetree/bindings/power/qcom,rpmpd.txt | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/Documentation/devicetree/bindings/power/qcom,rpmpd.txt b/Documentation/devicetree/bindings/power/qcom,rpmpd.txt
index bc75bf4..a193d33 100644
--- a/Documentation/devicetree/bindings/power/qcom,rpmpd.txt
+++ b/Documentation/devicetree/bindings/power/qcom,rpmpd.txt
@@ -19,6 +19,11 @@ 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
+Optional Properties
+ - #cooling-cells: must be 2
+ RPMh also hosts power domains that can behave as thermal warming
+ device. If so, indicate this by specifying #cooling-cells.
+
Example: rpmh power domain controller and OPP table
#include <dt-bindings/power/qcom-rpmhpd.h>
--
2.1.4
RPMh hosts mx power domain that can be used to warm up the SoC. Indicate
this by using #cooling-cells property.
Signed-off-by: Thara Gopinath <[email protected]>
---
v3->v4:
- Removed subnode to indicate that mx power domain is a warming
device. Instead #cooling-cells is used as a power domain
provider property to indicate if the provider hosts a power
domain that can be used as a warming device.
arch/arm64/boot/dts/qcom/sdm845.dtsi | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
index 23260a0..71d6f91 100644
--- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
@@ -3118,6 +3118,7 @@
rpmhpd: power-controller {
compatible = "qcom,sdm845-rpmhpd";
#power-domain-cells = <1>;
+ #cooling-cells = <2>;
operating-points-v2 = <&rpmhpd_opp_table>;
rpmhpd_opp_table: opp-table {
--
2.1.4
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 <[email protected]>
---
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
With introduction of power domain warming devices, devices that control the
power domain are registered as the parent of the cooling device so that the
device-genpd hierarchy in kernel is maintained intact. To enable this,
introduce a new API thermal_of_cooling_device_parent_register that takes a
parent device pointer as input. Also, modify
__thermal_cooling_device_register to register parent of a newly created
cooling device, if specified.
Signed-off-by: Thara Gopinath <[email protected]>
---
drivers/thermal/thermal_core.c | 22 +++++++++++++++++++---
include/linux/thermal.h | 15 +++++++++++++++
2 files changed, 34 insertions(+), 3 deletions(-)
diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index d4481cc..912ba75 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -947,6 +947,7 @@ static void bind_cdev(struct thermal_cooling_device *cdev)
*/
static struct thermal_cooling_device *
__thermal_cooling_device_register(struct device_node *np,
+ struct device *parent,
const char *type, void *devdata,
const struct thermal_cooling_device_ops *ops)
{
@@ -979,6 +980,8 @@ __thermal_cooling_device_register(struct device_node *np,
cdev->ops = ops;
cdev->updated = false;
cdev->device.class = &thermal_class;
+ if (parent)
+ cdev->device.parent = parent;
cdev->devdata = devdata;
thermal_cooling_device_setup_sysfs(cdev);
dev_set_name(&cdev->device, "cooling_device%d", cdev->id);
@@ -1024,7 +1027,8 @@ struct thermal_cooling_device *
thermal_cooling_device_register(const char *type, void *devdata,
const struct thermal_cooling_device_ops *ops)
{
- return __thermal_cooling_device_register(NULL, type, devdata, ops);
+ return __thermal_cooling_device_register(NULL, NULL, type,
+ devdata, ops);
}
EXPORT_SYMBOL_GPL(thermal_cooling_device_register);
@@ -1048,10 +1052,22 @@ thermal_of_cooling_device_register(struct device_node *np,
const char *type, void *devdata,
const struct thermal_cooling_device_ops *ops)
{
- return __thermal_cooling_device_register(np, type, devdata, ops);
+ return __thermal_cooling_device_register(np, NULL, type, devdata, ops);
}
EXPORT_SYMBOL_GPL(thermal_of_cooling_device_register);
+struct thermal_cooling_device *
+thermal_of_cooling_device_parent_register(struct device_node *np,
+ struct device *parent,
+ const char *type, void *devdata,
+ const struct
+ thermal_cooling_device_ops *ops)
+{
+ return __thermal_cooling_device_register(np, parent, type,
+ devdata, ops);
+}
+EXPORT_SYMBOL_GPL(thermal_of_cooling_device_parent_register);
+
static void thermal_cooling_device_release(struct device *dev, void *res)
{
thermal_cooling_device_unregister(
@@ -1088,7 +1104,7 @@ devm_thermal_of_cooling_device_register(struct device *dev,
if (!ptr)
return ERR_PTR(-ENOMEM);
- tcd = __thermal_cooling_device_register(np, type, devdata, ops);
+ tcd = __thermal_cooling_device_register(np, NULL, type, devdata, ops);
if (IS_ERR(tcd)) {
devres_free(ptr);
return tcd;
diff --git a/include/linux/thermal.h b/include/linux/thermal.h
index e45659c..ac5f268 100644
--- a/include/linux/thermal.h
+++ b/include/linux/thermal.h
@@ -448,6 +448,11 @@ struct thermal_cooling_device *
thermal_of_cooling_device_register(struct device_node *np, const char *, void *,
const struct thermal_cooling_device_ops *);
struct thermal_cooling_device *
+thermal_of_cooling_device_parent_register(struct device_node *np,
+ struct device *parent,
+ const char *, void *, const struct
+ thermal_cooling_device_ops *);
+struct thermal_cooling_device *
devm_thermal_of_cooling_device_register(struct device *dev,
struct device_node *np,
char *type, void *devdata,
@@ -508,6 +513,16 @@ static inline struct thermal_cooling_device *
thermal_of_cooling_device_register(struct device_node *np,
char *type, void *devdata, const struct thermal_cooling_device_ops *ops)
{ return ERR_PTR(-ENODEV); }
+
+static inline struct thermal_cooling_device *
+thermal_of_cooling_device_parent_register(struct device_node *np,
+ struct device *parent,
+ const char *, void *, const struct
+ thermal_cooling_device_ops *)
+{
+ return ERR_PTR(-ENODEV);
+}
+
static inline struct thermal_cooling_device *
devm_thermal_of_cooling_device_register(struct device *dev,
struct device_node *np,
--
2.1.4
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 <[email protected]>
---
v3->v4:
- Removed late_init hook pd_warming_device_ops.
- Use of_genpd_add_device instead of pm_genpd_add_device to attach
device to the generic power domain.
- Use thermal_of_cooling_device_parent_register to register the
cooling device so that the device with genpd attached can be
made parent of the cooling device.
- With above changes, remove reference to generic_pm_domain in
pd_warming_device.
drivers/thermal/Kconfig | 10 +++
drivers/thermal/Makefile | 2 +
drivers/thermal/pwr_domain_warming.c | 138 +++++++++++++++++++++++++++++++++++
include/linux/pwr_domain_warming.h | 29 ++++++++
4 files changed, 179 insertions(+)
create mode 100644 drivers/thermal/pwr_domain_warming.c
create mode 100644 include/linux/pwr_domain_warming.h
diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
index 001a21a..0c5c93e 100644
--- a/drivers/thermal/Kconfig
+++ b/drivers/thermal/Kconfig
@@ -187,6 +187,16 @@ config DEVFREQ_THERMAL
If you want this support, you should say Y here.
+config PWR_DOMAIN_WARMING_THERMAL
+ bool "Power Domain based warming device"
+ depends on PM_GENERIC_DOMAINS_OF
+ help
+ This implements the generic power domain based warming
+ mechanism through increasing the performance state of
+ a power domain.
+
+ If you want this support, you should say Y here.
+
config THERMAL_EMULATION
bool "Thermal emulation mode support"
help
diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
index 74a37c7..382c64a 100644
--- a/drivers/thermal/Makefile
+++ b/drivers/thermal/Makefile
@@ -27,6 +27,8 @@ thermal_sys-$(CONFIG_CLOCK_THERMAL) += clock_cooling.o
# devfreq cooling
thermal_sys-$(CONFIG_DEVFREQ_THERMAL) += devfreq_cooling.o
+thermal_sys-$(CONFIG_PWR_DOMAIN_WARMING_THERMAL) += pwr_domain_warming.o
+
# platform thermal drivers
obj-y += broadcom/
obj-$(CONFIG_THERMAL_MMIO) += thermal_mmio.o
diff --git a/drivers/thermal/pwr_domain_warming.c b/drivers/thermal/pwr_domain_warming.c
new file mode 100644
index 0000000..40162b9
--- /dev/null
+++ b/drivers/thermal/pwr_domain_warming.c
@@ -0,0 +1,138 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2019, Linaro Ltd
+ */
+#include <linux/err.h>
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/module.h>
+#include <linux/pm_runtime.h>
+#include <linux/slab.h>
+#include <linux/pwr_domain_warming.h>
+
+struct pd_warming_device {
+ struct thermal_cooling_device *cdev;
+ struct device dev;
+ int max_state;
+ int cur_state;
+ bool runtime_resumed;
+};
+
+static int pd_wdev_get_max_state(struct thermal_cooling_device *cdev,
+ unsigned long *state)
+{
+ struct pd_warming_device *pd_wdev = cdev->devdata;
+
+ *state = pd_wdev->max_state;
+ return 0;
+}
+
+static int pd_wdev_get_cur_state(struct thermal_cooling_device *cdev,
+ unsigned long *state)
+{
+ struct pd_warming_device *pd_wdev = cdev->devdata;
+
+ *state = dev_pm_genpd_get_performance_state(&pd_wdev->dev);
+
+ return 0;
+}
+
+static int pd_wdev_set_cur_state(struct thermal_cooling_device *cdev,
+ unsigned long state)
+{
+ struct pd_warming_device *pd_wdev = cdev->devdata;
+ struct device *dev = &pd_wdev->dev;
+ int ret;
+
+ ret = dev_pm_genpd_set_performance_state(dev, state);
+
+ if (ret)
+ return ret;
+
+ if (state && !pd_wdev->runtime_resumed) {
+ ret = pm_runtime_get_sync(dev);
+ pd_wdev->runtime_resumed = true;
+ } else if (!state && pd_wdev->runtime_resumed) {
+ ret = pm_runtime_put(dev);
+ pd_wdev->runtime_resumed = false;
+ }
+
+ return ret;
+}
+
+static struct thermal_cooling_device_ops pd_warming_device_ops = {
+ .get_max_state = pd_wdev_get_max_state,
+ .get_cur_state = pd_wdev_get_cur_state,
+ .set_cur_state = pd_wdev_set_cur_state,
+};
+
+struct thermal_cooling_device *
+pwr_domain_warming_register(struct device *parent, char *pd_name, int pd_id)
+{
+ struct pd_warming_device *pd_wdev;
+ struct of_phandle_args pd_args;
+ int ret;
+
+ pd_wdev = kzalloc(sizeof(*pd_wdev), GFP_KERNEL);
+ if (!pd_wdev)
+ return ERR_PTR(-ENOMEM);
+
+ dev_set_name(&pd_wdev->dev, "%s_warming_dev", pd_name);
+ pd_wdev->dev.parent = parent;
+
+ ret = device_register(&pd_wdev->dev);
+ if (ret)
+ goto error;
+
+ pd_args.np = parent->of_node;
+ pd_args.args[0] = pd_id;
+ pd_args.args_count = 1;
+
+ ret = of_genpd_add_device(&pd_args, &pd_wdev->dev);
+
+ if (ret)
+ goto error;
+
+ ret = dev_pm_genpd_performance_state_count(&pd_wdev->dev);
+ if (ret < 0)
+ goto error;
+
+ pd_wdev->max_state = ret - 1;
+ pm_runtime_enable(&pd_wdev->dev);
+ pd_wdev->runtime_resumed = false;
+
+ pd_wdev->cdev = thermal_of_cooling_device_parent_register
+ (NULL, parent, pd_name, pd_wdev,
+ &pd_warming_device_ops);
+ if (IS_ERR(pd_wdev->cdev)) {
+ pr_err("unable to register %s cooling device\n", pd_name);
+ pm_runtime_disable(&pd_wdev->dev);
+ ret = PTR_ERR(pd_wdev->cdev);
+ goto error;
+ }
+
+ return pd_wdev->cdev;
+error:
+ put_device(&pd_wdev->dev);
+ kfree(pd_wdev);
+ return ERR_PTR(ret);
+}
+EXPORT_SYMBOL_GPL(pwr_domain_warming_register);
+
+void pwr_domain_warming_unregister(struct thermal_cooling_device *cdev)
+{
+ struct pd_warming_device *pd_wdev = cdev->devdata;
+ struct device *dev = &pd_wdev->dev;
+
+ if (pd_wdev->runtime_resumed) {
+ dev_pm_genpd_set_performance_state(dev, 0);
+ pm_runtime_put(dev);
+ pd_wdev->runtime_resumed = false;
+ }
+ pm_runtime_disable(dev);
+ thermal_cooling_device_unregister(cdev);
+ kfree(pd_wdev);
+}
+EXPORT_SYMBOL_GPL(pwr_domain_warming_unregister);
diff --git a/include/linux/pwr_domain_warming.h b/include/linux/pwr_domain_warming.h
new file mode 100644
index 0000000..cb6550d
--- /dev/null
+++ b/include/linux/pwr_domain_warming.h
@@ -0,0 +1,29 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2019, Linaro Ltd.
+ */
+#ifndef __PWR_DOMAIN_WARMING_H__
+#define __PWR_DOMAIN_WARMING_H__
+
+#include <linux/pm_domain.h>
+#include <linux/thermal.h>
+
+#ifdef CONFIG_PWR_DOMAIN_WARMING_THERMAL
+struct thermal_cooling_device *
+pwr_domain_warming_register(struct device *parent, char *pd_name, int pd_id);
+
+void pwr_domain_warming_unregister(struct thermal_cooling_device *cdev);
+
+#else
+static inline struct thermal_cooling_device *
+pwr_domain_warming_register(struct device *parent, char *pd_name, int pd_id)
+{
+ return ERR_PTR(-ENOSYS);
+}
+
+static inline void
+pwr_domain_warming_unregister(struct thermal_cooling_device *cdev)
+{
+}
+#endif /* CONFIG_PWR_DOMAIN_WARMING_THERMAL */
+#endif /* __PWR_DOMAIN_WARMING_H__ */
--
2.1.4
On Wed, 20 Nov 2019 07:56:32 -0500, Thara Gopinath wrote:
> RPMh power controller hosts mx domain that can be used as thermal warming
> device. Add #cooling-cells property to the power domain provider node to
> indicate this.
>
> Signed-off-by: Thara Gopinath <[email protected]>
> ---
> v3->v4:
> - Removed subnode to indicate that mx power domain is a warming
> device. Instead #cooling-cells is used as a power domain
> provider property to indicate if the provider hosts a power
> domain that can be used as a warming device.
>
> Documentation/devicetree/bindings/power/qcom,rpmpd.txt | 5 +++++
> 1 file changed, 5 insertions(+)
>
Acked-by: Rob Herring <[email protected]>
On 11/20/2019 07:56 AM, Thara Gopinath wrote:
> 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].
Hi,
Can this series be merged ? It has been acked from DT and genpd point of
view.
Warm Regards
Thara
On Wed, 20 Nov 2019 at 13:56, Thara Gopinath <[email protected]> wrote:
>
> 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 <[email protected]>
Apologize for the delays! Re-kicking reviews now, I will provide
further comments later today or tomorrow.
For this one:
Reviewed-by: Ulf Hansson <[email protected]>
Kind regards
Uffe
> ---
> 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
>
On Wed, 20 Nov 2019 at 13:56, Thara Gopinath <[email protected]> wrote:
>
> With introduction of power domain warming devices, devices that control the
> power domain are registered as the parent of the cooling device so that the
> device-genpd hierarchy in kernel is maintained intact. To enable this,
> introduce a new API thermal_of_cooling_device_parent_register that takes a
> parent device pointer as input. Also, modify
> __thermal_cooling_device_register to register parent of a newly created
> cooling device, if specified.
I am not sure I understand the reasons why you need this, can you
please elaborate?
I remember we talked about using a "parent" device to deal with device
attaching to PM domains (genpd). However, since the DT bindings for
"warming devices" was concluded to consist by a single property
("#cooling-cells") as a part of the PM domain provider node, this
seems not to be needed.
By looking at patch 4/7, you are attaching devices via
of_genpd_add_device() and I don't see any need for using a "parent" in
there.
Can $subject patch be dropped or what am I missing?
Kind regards
Uffe
>
> Signed-off-by: Thara Gopinath <[email protected]>
> ---
> drivers/thermal/thermal_core.c | 22 +++++++++++++++++++---
> include/linux/thermal.h | 15 +++++++++++++++
> 2 files changed, 34 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
> index d4481cc..912ba75 100644
> --- a/drivers/thermal/thermal_core.c
> +++ b/drivers/thermal/thermal_core.c
> @@ -947,6 +947,7 @@ static void bind_cdev(struct thermal_cooling_device *cdev)
> */
> static struct thermal_cooling_device *
> __thermal_cooling_device_register(struct device_node *np,
> + struct device *parent,
> const char *type, void *devdata,
> const struct thermal_cooling_device_ops *ops)
> {
> @@ -979,6 +980,8 @@ __thermal_cooling_device_register(struct device_node *np,
> cdev->ops = ops;
> cdev->updated = false;
> cdev->device.class = &thermal_class;
> + if (parent)
> + cdev->device.parent = parent;
> cdev->devdata = devdata;
> thermal_cooling_device_setup_sysfs(cdev);
> dev_set_name(&cdev->device, "cooling_device%d", cdev->id);
> @@ -1024,7 +1027,8 @@ struct thermal_cooling_device *
> thermal_cooling_device_register(const char *type, void *devdata,
> const struct thermal_cooling_device_ops *ops)
> {
> - return __thermal_cooling_device_register(NULL, type, devdata, ops);
> + return __thermal_cooling_device_register(NULL, NULL, type,
> + devdata, ops);
> }
> EXPORT_SYMBOL_GPL(thermal_cooling_device_register);
>
> @@ -1048,10 +1052,22 @@ thermal_of_cooling_device_register(struct device_node *np,
> const char *type, void *devdata,
> const struct thermal_cooling_device_ops *ops)
> {
> - return __thermal_cooling_device_register(np, type, devdata, ops);
> + return __thermal_cooling_device_register(np, NULL, type, devdata, ops);
> }
> EXPORT_SYMBOL_GPL(thermal_of_cooling_device_register);
>
> +struct thermal_cooling_device *
> +thermal_of_cooling_device_parent_register(struct device_node *np,
> + struct device *parent,
> + const char *type, void *devdata,
> + const struct
> + thermal_cooling_device_ops *ops)
> +{
> + return __thermal_cooling_device_register(np, parent, type,
> + devdata, ops);
> +}
> +EXPORT_SYMBOL_GPL(thermal_of_cooling_device_parent_register);
> +
> static void thermal_cooling_device_release(struct device *dev, void *res)
> {
> thermal_cooling_device_unregister(
> @@ -1088,7 +1104,7 @@ devm_thermal_of_cooling_device_register(struct device *dev,
> if (!ptr)
> return ERR_PTR(-ENOMEM);
>
> - tcd = __thermal_cooling_device_register(np, type, devdata, ops);
> + tcd = __thermal_cooling_device_register(np, NULL, type, devdata, ops);
> if (IS_ERR(tcd)) {
> devres_free(ptr);
> return tcd;
> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> index e45659c..ac5f268 100644
> --- a/include/linux/thermal.h
> +++ b/include/linux/thermal.h
> @@ -448,6 +448,11 @@ struct thermal_cooling_device *
> thermal_of_cooling_device_register(struct device_node *np, const char *, void *,
> const struct thermal_cooling_device_ops *);
> struct thermal_cooling_device *
> +thermal_of_cooling_device_parent_register(struct device_node *np,
> + struct device *parent,
> + const char *, void *, const struct
> + thermal_cooling_device_ops *);
> +struct thermal_cooling_device *
> devm_thermal_of_cooling_device_register(struct device *dev,
> struct device_node *np,
> char *type, void *devdata,
> @@ -508,6 +513,16 @@ static inline struct thermal_cooling_device *
> thermal_of_cooling_device_register(struct device_node *np,
> char *type, void *devdata, const struct thermal_cooling_device_ops *ops)
> { return ERR_PTR(-ENODEV); }
> +
> +static inline struct thermal_cooling_device *
> +thermal_of_cooling_device_parent_register(struct device_node *np,
> + struct device *parent,
> + const char *, void *, const struct
> + thermal_cooling_device_ops *)
> +{
> + return ERR_PTR(-ENODEV);
> +}
> +
> static inline struct thermal_cooling_device *
> devm_thermal_of_cooling_device_register(struct device *dev,
> struct device_node *np,
> --
> 2.1.4
>
On Wed, 20 Nov 2019 at 13:56, Thara Gopinath <[email protected]> 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 <[email protected]>
> ---
> v3->v4:
> - Removed late_init hook pd_warming_device_ops.
> - Use of_genpd_add_device instead of pm_genpd_add_device to attach
> device to the generic power domain.
> - Use thermal_of_cooling_device_parent_register to register the
> cooling device so that the device with genpd attached can be
> made parent of the cooling device.
> - With above changes, remove reference to generic_pm_domain in
> pd_warming_device.
>
> drivers/thermal/Kconfig | 10 +++
> drivers/thermal/Makefile | 2 +
> drivers/thermal/pwr_domain_warming.c | 138 +++++++++++++++++++++++++++++++++++
> include/linux/pwr_domain_warming.h | 29 ++++++++
Not sure about what the thermal maintainers think about the naming
here. In the end, it's their call.
However, normally we use "pm_domain_*", rather than "pwr_domain_*",
but maybe just "pd_*" is sufficient here.
> 4 files changed, 179 insertions(+)
> create mode 100644 drivers/thermal/pwr_domain_warming.c
> create mode 100644 include/linux/pwr_domain_warming.h
>
> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
> index 001a21a..0c5c93e 100644
> --- a/drivers/thermal/Kconfig
> +++ b/drivers/thermal/Kconfig
> @@ -187,6 +187,16 @@ config DEVFREQ_THERMAL
>
> If you want this support, you should say Y here.
>
> +config PWR_DOMAIN_WARMING_THERMAL
> + bool "Power Domain based warming device"
> + depends on PM_GENERIC_DOMAINS_OF
> + help
> + This implements the generic power domain based warming
> + mechanism through increasing the performance state of
> + a power domain.
> +
> + If you want this support, you should say Y here.
> +
> config THERMAL_EMULATION
> bool "Thermal emulation mode support"
> help
> diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
> index 74a37c7..382c64a 100644
> --- a/drivers/thermal/Makefile
> +++ b/drivers/thermal/Makefile
> @@ -27,6 +27,8 @@ thermal_sys-$(CONFIG_CLOCK_THERMAL) += clock_cooling.o
> # devfreq cooling
> thermal_sys-$(CONFIG_DEVFREQ_THERMAL) += devfreq_cooling.o
>
> +thermal_sys-$(CONFIG_PWR_DOMAIN_WARMING_THERMAL) += pwr_domain_warming.o
> +
> # platform thermal drivers
> obj-y += broadcom/
> obj-$(CONFIG_THERMAL_MMIO) += thermal_mmio.o
> diff --git a/drivers/thermal/pwr_domain_warming.c b/drivers/thermal/pwr_domain_warming.c
> new file mode 100644
> index 0000000..40162b9
> --- /dev/null
> +++ b/drivers/thermal/pwr_domain_warming.c
> @@ -0,0 +1,138 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2019, Linaro Ltd
> + */
> +#include <linux/err.h>
> +#include <linux/kernel.h>
> +#include <linux/init.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/module.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/slab.h>
> +#include <linux/pwr_domain_warming.h>
> +
> +struct pd_warming_device {
> + struct thermal_cooling_device *cdev;
> + struct device dev;
> + int max_state;
> + int cur_state;
> + bool runtime_resumed;
> +};
> +
> +static int pd_wdev_get_max_state(struct thermal_cooling_device *cdev,
> + unsigned long *state)
> +{
> + struct pd_warming_device *pd_wdev = cdev->devdata;
> +
> + *state = pd_wdev->max_state;
> + return 0;
> +}
> +
> +static int pd_wdev_get_cur_state(struct thermal_cooling_device *cdev,
> + unsigned long *state)
> +{
> + struct pd_warming_device *pd_wdev = cdev->devdata;
> +
> + *state = dev_pm_genpd_get_performance_state(&pd_wdev->dev);
> +
> + return 0;
> +}
> +
> +static int pd_wdev_set_cur_state(struct thermal_cooling_device *cdev,
> + unsigned long state)
> +{
> + struct pd_warming_device *pd_wdev = cdev->devdata;
> + struct device *dev = &pd_wdev->dev;
> + int ret;
> +
> + ret = dev_pm_genpd_set_performance_state(dev, state);
> +
> + if (ret)
> + return ret;
> +
> + if (state && !pd_wdev->runtime_resumed) {
> + ret = pm_runtime_get_sync(dev);
> + pd_wdev->runtime_resumed = true;
> + } else if (!state && pd_wdev->runtime_resumed) {
> + ret = pm_runtime_put(dev);
> + pd_wdev->runtime_resumed = false;
> + }
> +
> + return ret;
> +}
> +
> +static struct thermal_cooling_device_ops pd_warming_device_ops = {
> + .get_max_state = pd_wdev_get_max_state,
> + .get_cur_state = pd_wdev_get_cur_state,
> + .set_cur_state = pd_wdev_set_cur_state,
> +};
> +
> +struct thermal_cooling_device *
> +pwr_domain_warming_register(struct device *parent, char *pd_name, int pd_id)
Maybe rename this to: thermal_of_pd_warming_register()
Moreover, I think you could replace the "struct device *parent", with
a "struct device_node *node" as in-parameter. That's all you need,
right?
> +{
> + struct pd_warming_device *pd_wdev;
> + struct of_phandle_args pd_args;
> + int ret;
> +
> + pd_wdev = kzalloc(sizeof(*pd_wdev), GFP_KERNEL);
> + if (!pd_wdev)
> + return ERR_PTR(-ENOMEM);
> +
> + dev_set_name(&pd_wdev->dev, "%s_warming_dev", pd_name);
Perhaps skip the in-param *pd_name and make use of the suggested
"struct device_node *node", the index and something with "warming...",
when setting the name.
Just an idea, as to simplify for the caller.
> + pd_wdev->dev.parent = parent;
This isn't needed, I think.
> +
> + ret = device_register(&pd_wdev->dev);
> + if (ret)
> + goto error;
> +
> + pd_args.np = parent->of_node;
> + pd_args.args[0] = pd_id;
> + pd_args.args_count = 1;
> +
> + ret = of_genpd_add_device(&pd_args, &pd_wdev->dev);
> +
White space.
> + if (ret)
> + goto error;
> +
> + ret = dev_pm_genpd_performance_state_count(&pd_wdev->dev);
> + if (ret < 0)
> + goto error;
> +
> + pd_wdev->max_state = ret - 1;
> + pm_runtime_enable(&pd_wdev->dev);
> + pd_wdev->runtime_resumed = false;
> +
> + pd_wdev->cdev = thermal_of_cooling_device_parent_register
> + (NULL, parent, pd_name, pd_wdev,
> + &pd_warming_device_ops);
As stated in patch3, I don't get it why you need to use this new API
for "parents".
> + if (IS_ERR(pd_wdev->cdev)) {
> + pr_err("unable to register %s cooling device\n", pd_name);
> + pm_runtime_disable(&pd_wdev->dev);
> + ret = PTR_ERR(pd_wdev->cdev);
> + goto error;
> + }
> +
> + return pd_wdev->cdev;
> +error:
> + put_device(&pd_wdev->dev);
If device_register() succeeds you need to call device_unregister(),
rather than put_device() as a part of the error handling.
> + kfree(pd_wdev);
You need a ->release() callback to manage kfree(), after you called
device_register().
> + return ERR_PTR(ret);
Another thing is missing in the error path, which is to remove the
device for the genpd. I think calling pm_genpd_remove_device() should
work fine here.
> +}
> +EXPORT_SYMBOL_GPL(pwr_domain_warming_register);
> +
> +void pwr_domain_warming_unregister(struct thermal_cooling_device *cdev)
> +{
> + struct pd_warming_device *pd_wdev = cdev->devdata;
> + struct device *dev = &pd_wdev->dev;
> +
> + if (pd_wdev->runtime_resumed) {
> + dev_pm_genpd_set_performance_state(dev, 0);
> + pm_runtime_put(dev);
> + pd_wdev->runtime_resumed = false;
> + }
> + pm_runtime_disable(dev);
> + thermal_cooling_device_unregister(cdev);
> + kfree(pd_wdev);
> +}
> +EXPORT_SYMBOL_GPL(pwr_domain_warming_unregister);
> diff --git a/include/linux/pwr_domain_warming.h b/include/linux/pwr_domain_warming.h
> new file mode 100644
> index 0000000..cb6550d
> --- /dev/null
> +++ b/include/linux/pwr_domain_warming.h
> @@ -0,0 +1,29 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2019, Linaro Ltd.
> + */
> +#ifndef __PWR_DOMAIN_WARMING_H__
> +#define __PWR_DOMAIN_WARMING_H__
> +
> +#include <linux/pm_domain.h>
> +#include <linux/thermal.h>
> +
> +#ifdef CONFIG_PWR_DOMAIN_WARMING_THERMAL
> +struct thermal_cooling_device *
> +pwr_domain_warming_register(struct device *parent, char *pd_name, int pd_id);
> +
> +void pwr_domain_warming_unregister(struct thermal_cooling_device *cdev);
> +
> +#else
> +static inline struct thermal_cooling_device *
> +pwr_domain_warming_register(struct device *parent, char *pd_name, int pd_id)
> +{
> + return ERR_PTR(-ENOSYS);
> +}
> +
> +static inline void
> +pwr_domain_warming_unregister(struct thermal_cooling_device *cdev)
> +{
> +}
> +#endif /* CONFIG_PWR_DOMAIN_WARMING_THERMAL */
> +#endif /* __PWR_DOMAIN_WARMING_H__ */
> --
> 2.1.4
>
Kind regards
Uffe
On Wed, 20 Nov 2019 at 13:57, Thara Gopinath <[email protected]> wrote:
>
> RPMh hosts mx power domain that can be used to warm up the SoC. Indicate
> this by using #cooling-cells property.
>
> Signed-off-by: Thara Gopinath <[email protected]>
Reviewed-by: Ulf Hansson <[email protected]>
Kind regards
Uffe
> ---
> v3->v4:
> - Removed subnode to indicate that mx power domain is a warming
> device. Instead #cooling-cells is used as a power domain
> provider property to indicate if the provider hosts a power
> domain that can be used as a warming device.
>
> arch/arm64/boot/dts/qcom/sdm845.dtsi | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> index 23260a0..71d6f91 100644
> --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> @@ -3118,6 +3118,7 @@
> rpmhpd: power-controller {
> compatible = "qcom,sdm845-rpmhpd";
> #power-domain-cells = <1>;
> + #cooling-cells = <2>;
> operating-points-v2 = <&rpmhpd_opp_table>;
>
> rpmhpd_opp_table: opp-table {
> --
> 2.1.4
>
On Wed, 20 Nov 2019 at 13:56, Thara Gopinath <[email protected]> wrote:
>
> RPMh power control hosts power domains that can be used as
> thermal warming devices. Register these power domains
> with the generic power domain warming device thermal framework.
>
> Signed-off-by: Thara Gopinath <[email protected]>
> ---
> v3->v4:
> - Introduce a boolean value is_warming_dev in rpmhpd structure to
> indicate if a generic power domain can be used as a warming
> device or not.With this change, device tree no longer has to
> specify which power domain inside the rpmh power domain provider
> is a warming device.
> - Move registering of warming devices into a late initcall to
> ensure that warming devices are registerd after thermal
> framework is initialized.
>
> drivers/soc/qcom/rpmhpd.c | 38 +++++++++++++++++++++++++++++++++++++-
> 1 file changed, 37 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/soc/qcom/rpmhpd.c b/drivers/soc/qcom/rpmhpd.c
> index 9d37534..5666d1f 100644
> --- a/drivers/soc/qcom/rpmhpd.c
> +++ b/drivers/soc/qcom/rpmhpd.c
> @@ -11,6 +11,7 @@
> #include <linux/of_device.h>
> #include <linux/platform_device.h>
> #include <linux/pm_opp.h>
> +#include <linux/pwr_domain_warming.h>
> #include <soc/qcom/cmd-db.h>
> #include <soc/qcom/rpmh.h>
> #include <dt-bindings/power/qcom-rpmpd.h>
> @@ -48,6 +49,7 @@ struct rpmhpd {
> bool enabled;
> const char *res_name;
> u32 addr;
> + bool is_warming_dev;
> };
>
> struct rpmhpd_desc {
> @@ -55,6 +57,8 @@ struct rpmhpd_desc {
> size_t num_pds;
> };
>
> +const struct rpmhpd_desc *global_desc;
> +
> static DEFINE_MUTEX(rpmhpd_lock);
>
> /* SDM845 RPMH powerdomains */
> @@ -89,6 +93,7 @@ static struct rpmhpd sdm845_mx = {
> .pd = { .name = "mx", },
> .peer = &sdm845_mx_ao,
> .res_name = "mx.lvl",
> + .is_warming_dev = true,
> };
>
> static struct rpmhpd sdm845_mx_ao = {
> @@ -396,7 +401,14 @@ static int rpmhpd_probe(struct platform_device *pdev)
> &rpmhpds[i]->pd);
> }
>
> - return of_genpd_add_provider_onecell(pdev->dev.of_node, data);
> + ret = of_genpd_add_provider_onecell(pdev->dev.of_node, data);
> +
> + if (ret)
> + return ret;
> +
> + global_desc = desc;
I assume this works fine, for now.
Although, nothing prevents this driver from being probed for two
different compatibles for the same platform. Thus the global_desc
could be overwritten with the last one being probed, so then how do
you know which one to use?
> +
> + return 0;
> }
>
> static struct platform_driver rpmhpd_driver = {
> @@ -413,3 +425,27 @@ static int __init rpmhpd_init(void)
> return platform_driver_register(&rpmhpd_driver);
> }
> core_initcall(rpmhpd_init);
> +
> +static int __init rpmhpd_init_warming_device(void)
> +{
> + size_t num_pds;
> + struct rpmhpd **rpmhpds;
> + int i;
> +
> + if (!global_desc)
> + return -EINVAL;
> +
> + rpmhpds = global_desc->rpmhpds;
> + num_pds = global_desc->num_pds;
> +
> + if (!of_find_property(rpmhpds[0]->dev->of_node, "#cooling-cells", NULL))
> + return 0;
> +
> + for (i = 0; i < num_pds; i++)
> + if (rpmhpds[i]->is_warming_dev)
> + pwr_domain_warming_register(rpmhpds[i]->dev,
> + rpmhpds[i]->res_name, i);
> +
> + return 0;
> +}
> +late_initcall(rpmhpd_init_warming_device);
For the record, there are limitations with this approach, for example
you can't deal with -EPROBE_DEFER.
On the other hand, I don't have anything better to suggest, from the
top of my head. So, feel free to add:
Reviewed-by: Ulf Hansson <[email protected]>
Kind regards
Uffe
On Wed, 20 Nov 2019 at 13:57, Thara Gopinath <[email protected]> wrote:
>
> RPMh power controller hosts mx domain that can be used as thermal warming
> device. Add #cooling-cells property to the power domain provider node to
> indicate this.
>
> Signed-off-by: Thara Gopinath <[email protected]>
Reviewed-by: Ulf Hansson <[email protected]>
Kind regards
Uffe
> ---
> v3->v4:
> - Removed subnode to indicate that mx power domain is a warming
> device. Instead #cooling-cells is used as a power domain
> provider property to indicate if the provider hosts a power
> domain that can be used as a warming device.
>
> Documentation/devicetree/bindings/power/qcom,rpmpd.txt | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/power/qcom,rpmpd.txt b/Documentation/devicetree/bindings/power/qcom,rpmpd.txt
> index bc75bf4..a193d33 100644
> --- a/Documentation/devicetree/bindings/power/qcom,rpmpd.txt
> +++ b/Documentation/devicetree/bindings/power/qcom,rpmpd.txt
> @@ -19,6 +19,11 @@ 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
>
> +Optional Properties
> + - #cooling-cells: must be 2
> + RPMh also hosts power domains that can behave as thermal warming
> + device. If so, indicate this by specifying #cooling-cells.
> +
> Example: rpmh power domain controller and OPP table
>
> #include <dt-bindings/power/qcom-rpmhpd.h>
> --
> 2.1.4
>
Hi Ulf,
Thanks for the reviews. Sorry for the delay in response.
I have started working on this again. So this should pick
up pace now.
On 2/4/20 11:54 AM, Ulf Hansson wrote:
> On Wed, 20 Nov 2019 at 13:56, Thara Gopinath <[email protected]> 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 <[email protected]>
>> ---
>> v3->v4:
>> - Removed late_init hook pd_warming_device_ops.
>> - Use of_genpd_add_device instead of pm_genpd_add_device to attach
>> device to the generic power domain.
>> - Use thermal_of_cooling_device_parent_register to register the
>> cooling device so that the device with genpd attached can be
>> made parent of the cooling device.
>> - With above changes, remove reference to generic_pm_domain in
>> pd_warming_device.
>>
>> drivers/thermal/Kconfig | 10 +++
>> drivers/thermal/Makefile | 2 +
>> drivers/thermal/pwr_domain_warming.c | 138 +++++++++++++++++++++++++++++++++++
>> include/linux/pwr_domain_warming.h | 29 ++++++++
>
> Not sure about what the thermal maintainers think about the naming
> here. In the end, it's their call.
>
> However, normally we use "pm_domain_*", rather than "pwr_domain_*",
> but maybe just "pd_*" is sufficient here.
I will rename this to pd_ for now.
>
>> 4 files changed, 179 insertions(+)
>> create mode 100644 drivers/thermal/pwr_domain_warming.c
>> create mode 100644 include/linux/pwr_domain_warming.h
>>
>> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
>> index 001a21a..0c5c93e 100644
>> --- a/drivers/thermal/Kconfig
>> +++ b/drivers/thermal/Kconfig
>> @@ -187,6 +187,16 @@ config DEVFREQ_THERMAL
>>
>> If you want this support, you should say Y here.
>>
>> +config PWR_DOMAIN_WARMING_THERMAL
>> + bool "Power Domain based warming device"
>> + depends on PM_GENERIC_DOMAINS_OF
>> + help
>> + This implements the generic power domain based warming
>> + mechanism through increasing the performance state of
>> + a power domain.
>> +
>> + If you want this support, you should say Y here.
>> +
>> config THERMAL_EMULATION
>> bool "Thermal emulation mode support"
>> help
>> diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
>> index 74a37c7..382c64a 100644
>> --- a/drivers/thermal/Makefile
>> +++ b/drivers/thermal/Makefile
>> @@ -27,6 +27,8 @@ thermal_sys-$(CONFIG_CLOCK_THERMAL) += clock_cooling.o
>> # devfreq cooling
>> thermal_sys-$(CONFIG_DEVFREQ_THERMAL) += devfreq_cooling.o
>>
>> +thermal_sys-$(CONFIG_PWR_DOMAIN_WARMING_THERMAL) += pwr_domain_warming.o
>> +
>> # platform thermal drivers
>> obj-y += broadcom/
>> obj-$(CONFIG_THERMAL_MMIO) += thermal_mmio.o
>> diff --git a/drivers/thermal/pwr_domain_warming.c b/drivers/thermal/pwr_domain_warming.c
>> new file mode 100644
>> index 0000000..40162b9
>> --- /dev/null
>> +++ b/drivers/thermal/pwr_domain_warming.c
>> @@ -0,0 +1,138 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (c) 2019, Linaro Ltd
>> + */
>> +#include <linux/err.h>
>> +#include <linux/kernel.h>
>> +#include <linux/init.h>
>> +#include <linux/of_device.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/module.h>
>> +#include <linux/pm_runtime.h>
>> +#include <linux/slab.h>
>> +#include <linux/pwr_domain_warming.h>
>> +
>> +struct pd_warming_device {
>> + struct thermal_cooling_device *cdev;
>> + struct device dev;
>> + int max_state;
>> + int cur_state;
>> + bool runtime_resumed;
>> +};
>> +
>> +static int pd_wdev_get_max_state(struct thermal_cooling_device *cdev,
>> + unsigned long *state)
>> +{
>> + struct pd_warming_device *pd_wdev = cdev->devdata;
>> +
>> + *state = pd_wdev->max_state;
>> + return 0;
>> +}
>> +
>> +static int pd_wdev_get_cur_state(struct thermal_cooling_device *cdev,
>> + unsigned long *state)
>> +{
>> + struct pd_warming_device *pd_wdev = cdev->devdata;
>> +
>> + *state = dev_pm_genpd_get_performance_state(&pd_wdev->dev);
>> +
>> + return 0;
>> +}
>> +
>> +static int pd_wdev_set_cur_state(struct thermal_cooling_device *cdev,
>> + unsigned long state)
>> +{
>> + struct pd_warming_device *pd_wdev = cdev->devdata;
>> + struct device *dev = &pd_wdev->dev;
>> + int ret;
>> +
>> + ret = dev_pm_genpd_set_performance_state(dev, state);
>> +
>> + if (ret)
>> + return ret;
>> +
>> + if (state && !pd_wdev->runtime_resumed) {
>> + ret = pm_runtime_get_sync(dev);
>> + pd_wdev->runtime_resumed = true;
>> + } else if (!state && pd_wdev->runtime_resumed) {
>> + ret = pm_runtime_put(dev);
>> + pd_wdev->runtime_resumed = false;
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +static struct thermal_cooling_device_ops pd_warming_device_ops = {
>> + .get_max_state = ::pd_wdev_get_max_state,
>> + .get_cur_state = pd_wdev_get_cur_state,
>> + .set_cur_state = pd_wdev_set_cur_state,
>> +};
>> +
>> +struct thermal_cooling_device *
>> +pwr_domain_warming_register(struct device *parent, char *pd_name, int pd_id)
>
> Maybe rename this to: thermal_of_pd_warming_register()
How about pd_of_warming_register? It is consistent with other cooling
device register like cpuidle_of_cooling_register and
cpufreq_of_cooling_register.
> Moreover, I think you could replace the "struct device *parent", with
> a "struct device_node *node" as in-parameter. That's all you need,
> right?
You mean pd_wdev->dev.parent need not be populated ? The device
in this case will be created under /sys/devices which I do not think
is the correct.
With a parent device specified, the power controller that resides the
power domain that can act as the warming dev, becomes the parent of the
warming dev. In case of this patch series, for the mx warming dev,
179c0000.rsc/179c0000.rsc\:power-controller/ becomes the parent.(The
device will be created under
/sys/devices/platform/soc\@0/179c0000.rsc/179c0000.rsc\:power-controller/)
Other way might be to register the warming device under virtual devices
as a new category of devices.
I prefer to keep it as a child of the power controller device, but I am
open to explore other options and to re-do this bit. What do you think?
>
>> +{
>> + struct pd_warming_device *pd_wdev;
>> + struct of_phandle_args pd_args;
>> + int ret;
>> +
>> + pd_wdev = kzalloc(sizeof(*pd_wdev), GFP_KERNEL);
>> + if (!pd_wdev)
>> + return ERR_PTR(-ENOMEM);
>> +
>> + dev_set_name(&pd_wdev->dev, "%s_warming_dev", pd_name);
>
> Perhaps skip the in-param *pd_name and make use of the suggested
> "struct device_node *node", the index and something with "warming...",
> when setting the name.
Won't the index have to be in-param in this case ?
>
> Just an idea, as to simplify for the caller.
>
>> + pd_wdev->dev.parent = parent;
>
> This isn't needed, I think.
>
>> +
>> + ret = device_register(&pd_wdev->dev);
>> + if (ret)
>> + goto error;
>> +
>> + pd_args.np = parent->of_node;
>> + pd_args.args[0] = pd_id;
>> + pd_args.args_count = 1;
>> +
>> + ret = of_genpd_add_device(&pd_args, &pd_wdev->dev);
>> +
>
> White space.
Will fix it.
>
>> + if (ret)
>> + goto error;
>> +
>> + ret = dev_pm_genpd_performance_state_count(&pd_wdev->dev);
>> + if (ret < 0)
>> + goto error;
>> +
>> + pd_wdev->max_state = ret - 1;
>> + pm_runtime_enable(&pd_wdev->dev);
>> + pd_wdev->runtime_resumed = false;
>> +
>> + pd_wdev->cdev = thermal_of_cooling_device_parent_register
>> + (NULL, parent, pd_name, pd_wdev,
>> + &pd_warming_device_ops);
>
> As stated in patch3, I don't get it why you need to use this new API
> for "parents".
I agree with you. I cannot re-collect my thought process for this API.
I compiled and tested using the regular API and everything works fine.
I will drop patch 3 and use the thermal_of_cooling_device_register here.
>
>> + if (IS_ERR(pd_wdev->cdev)) {
>> + pr_err("unable to register %s cooling device\n", pd_name);
>> + pm_runtime_disable(&pd_wdev->dev);
>> + ret = PTR_ERR(pd_wdev->cdev);
>> + goto error;
>> + }
>> +
>> + return pd_wdev->cdev;
>> +error:
>> + put_device(&pd_wdev->dev);
>
> If device_register() succeeds you need to call device_unregister(),
> rather than put_device() as a part of the error handling.
Will fix this.
>
>> + kfree(pd_wdev);
>
> You need a ->release() callback to manage kfree(), after you called
> device_register().
mm?? I did not get this. What release callback? You mean for power
controller driver to call ?
>
>> + return ERR_PTR(ret);
>
> Another thing is missing in the error path, which is to remove the
> device for the genpd. I think calling pm_genpd_remove_device() should
> work fine here.
I will fix this. I am thinking this will be be needed in
pwr_domain_warming_unregister as well.
--
Warm Regards
Thara
On 2/4/20 12:40 PM, Ulf Hansson wrote:
> On Wed, 20 Nov 2019 at 13:56, Thara Gopinath <[email protected]> wrote:
>>
>> RPMh power control hosts power domains that can be used as
>> thermal warming devices. Register these power domains
>> with the generic power domain warming device thermal framework.
>>
>> Signed-off-by: Thara Gopinath <[email protected]>
>> ---
>> v3->v4:
>> - Introduce a boolean value is_warming_dev in rpmhpd structure to
>> indicate if a generic power domain can be used as a warming
>> device or not.With this change, device tree no longer has to
>> specify which power domain inside the rpmh power domain provider
>> is a warming device.
>> - Move registering of warming devices into a late initcall to
>> ensure that warming devices are registerd after thermal
>> framework is initialized.
>>
>> drivers/soc/qcom/rpmhpd.c | 38 +++++++++++++++++++++++++++++++++++++-
>> 1 file changed, 37 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/soc/qcom/rpmhpd.c b/drivers/soc/qcom/rpmhpd.c
>> index 9d37534..5666d1f 100644
>> --- a/drivers/soc/qcom/rpmhpd.c
>> +++ b/drivers/soc/qcom/rpmhpd.c
>> @@ -11,6 +11,7 @@
>> #include <linux/of_device.h>
>> #include <linux/platform_device.h>
>> #include <linux/pm_opp.h>
>> +#include <linux/pwr_domain_warming.h>
>> #include <soc/qcom/cmd-db.h>
>> #include <soc/qcom/rpmh.h>
>> #include <dt-bindings/power/qcom-rpmpd.h>
>> @@ -48,6 +49,7 @@ struct rpmhpd {
>> bool enabled;
>> const char *res_name;
>> u32 addr;
>> + bool is_warming_dev;
>> };
>>
>> struct rpmhpd_desc {
>> @@ -55,6 +57,8 @@ struct rpmhpd_desc {
>> size_t num_pds;
>> };
>>
>> +const struct rpmhpd_desc *global_desc;
>> +
>> static DEFINE_MUTEX(rpmhpd_lock);
>>
>> /* SDM845 RPMH powerdomains */
>> @@ -89,6 +93,7 @@ static struct rpmhpd sdm845_mx = {
>> .pd = { .name = "mx", },
>> .peer = &sdm845_mx_ao,
>> .res_name = "mx.lvl",
>> + .is_warming_dev = true,
>> };
>>
>> static struct rpmhpd sdm845_mx_ao = {
>> @@ -396,7 +401,14 @@ static int rpmhpd_probe(struct platform_device *pdev)
>> &rpmhpds[i]->pd);
>> }
>>
>> - return of_genpd_add_provider_onecell(pdev->dev.of_node, data);
>> + ret = of_genpd_add_provider_onecell(pdev->dev.of_node, data);
>> +
>> + if (ret)
>> + return ret;
>> +
>> + global_desc = desc;
>
> I assume this works fine, for now.
>
> Although, nothing prevents this driver from being probed for two
> different compatibles for the same platform. Thus the global_desc
> could be overwritten with the last one being probed, so then how do
> you know which one to use?
Yes. It works fine for now. There are multiple ways to fix this in
future. One is to make global_desc an array. Other would be to move
the code in rpmhpd_init_warming_device to this init and make this a
post_core init considering thermal subsytem uses core init. Like you
said I will leave this at this for now and we can fix this if a need
arises. I don't think there is a need for multiple compatibles for the
same platform now. Thanks for the reviewed by! I will add it in the next
version.
>
>> +
>> + return 0;
>> }
>>
>> static struct platform_driver rpmhpd_driver = {
>> @@ -413,3 +425,27 @@ static int __init rpmhpd_init(void)
>> return platform_driver_register(&rpmhpd_driver);
>> }
>> core_initcall(rpmhpd_init);
>> +
>> +static int __init rpmhpd_init_warming_device(void)
>> +{
>> + size_t num_pds;
>> + struct rpmhpd **rpmhpds;
>> + int i;
>> +
>> + if (!global_desc)
>> + return -EINVAL;
>> +
>> + rpmhpds = global_desc->rpmhpds;
>> + num_pds = global_desc->num_pds;
>> +
>> + if (!of_find_property(rpmhpds[0]->dev->of_node, "#cooling-cells", NULL))
>> + return 0;
>> +
>> + for (i = 0; i < num_pds; i++)
>> + if (rpmhpds[i]->is_warming_dev)
>> + pwr_domain_warming_register(rpmhpds[i]->dev,
>> + rpmhpds[i]->res_name, i);
>> +
>> + return 0;
>> +}
>> +late_initcall(rpmhpd_init_warming_device);
>
> For the record, there are limitations with this approach, for example
> you can't deal with -EPROBE_DEFER.
>
> On the other hand, I don't have anything better to suggest, from the
> top of my head. So, feel free to add:
>
> Reviewed-by: Ulf Hansson <[email protected]>
>
> Kind regards
> Uffe
>
--
Warm Regards
Thara
On 2/4/20 12:41 PM, Ulf Hansson wrote:
> On Wed, 20 Nov 2019 at 13:57, Thara Gopinath <[email protected]> wrote:
>>
>> RPMh power controller hosts mx domain that can be used as thermal warming
>> device. Add #cooling-cells property to the power domain provider node to
>> indicate this.
>>
>> Signed-off-by: Thara Gopinath <[email protected]>
>
> Reviewed-by: Ulf Hansson <[email protected]>
Thanks! This file does not exist anymore. It has been moved to yaml
format! I will resend this in the correct file.
>
> Kind regards
> Uffe
>
>> ---
>> v3->v4:
>> - Removed subnode to indicate that mx power domain is a warming
>> device. Instead #cooling-cells is used as a power domain
>> provider property to indicate if the provider hosts a power
>> domain that can be used as a warming device.
>>
>> Documentation/devicetree/bindings/power/qcom,rpmpd.txt | 5 +++++
>> 1 file changed, 5 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/power/qcom,rpmpd.txt b/Documentation/devicetree/bindings/power/qcom,rpmpd.txt
>> index bc75bf4..a193d33 100644
>> --- a/Documentation/devicetree/bindings/power/qcom,rpmpd.txt
>> +++ b/Documentation/devicetree/bindings/power/qcom,rpmpd.txt
>> @@ -19,6 +19,11 @@ 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
>>
>> +Optional Properties
>> + - #cooling-cells: must be 2
>> + RPMh also hosts power domains that can behave as thermal warming
>> + device. If so, indicate this by specifying #cooling-cells.
>> +
>> Example: rpmh power domain controller and OPP table
>>
>> #include <dt-bindings/power/qcom-rpmhpd.h>
>> --
>> 2.1.4
>>
--
Warm Regards
Thara
[...]
> >> +static struct thermal_cooling_device_ops pd_warming_device_ops = {
> >> + .get_max_state = ::pd_wdev_get_max_state,
> >> + .get_cur_state = pd_wdev_get_cur_state,
> >> + .set_cur_state = pd_wdev_set_cur_state,
> >> +};
> >> +
> >> +struct thermal_cooling_device *
> >> +pwr_domain_warming_register(struct device *parent, char *pd_name, int pd_id)
> >
> > Maybe rename this to: thermal_of_pd_warming_register()
>
> How about pd_of_warming_register? It is consistent with other cooling
> device register like cpuidle_of_cooling_register and
> cpufreq_of_cooling_register.
Well, we actually have the following:
of_devfreq_cooling_register()
of_cpufreq_cooling_register()
cpuidle_of_cooling_register()
So maybe this is the most consistent. :-)
of_pd_warming_register()
>
> > Moreover, I think you could replace the "struct device *parent", with
> > a "struct device_node *node" as in-parameter. That's all you need,
> > right?
>
> You mean pd_wdev->dev.parent need not be populated ? The device
> in this case will be created under /sys/devices which I do not think
> is the correct.
Good point!
> With a parent device specified, the power controller that resides the
> power domain that can act as the warming dev, becomes the parent of the
> warming dev. In case of this patch series, for the mx warming dev,
> 179c0000.rsc/179c0000.rsc\:power-controller/ becomes the parent.(The
> device will be created under
> /sys/devices/platform/soc\@0/179c0000.rsc/179c0000.rsc\:power-controller/)
>
> Other way might be to register the warming device under virtual devices
> as a new category of devices.
No, that sounds wrong.
Another option is to create a specific bus type for these new
pd_warming devices. But I admit that sounds a bit too much, let's
assign a parent.
>
> I prefer to keep it as a child of the power controller device, but I am
> open to explore other options and to re-do this bit. What do you think?
Sure, sorry for the noise.
>
> >
> >> +{
> >> + struct pd_warming_device *pd_wdev;
> >> + struct of_phandle_args pd_args;
> >> + int ret;
> >> +
> >> + pd_wdev = kzalloc(sizeof(*pd_wdev), GFP_KERNEL);
> >> + if (!pd_wdev)
> >> + return ERR_PTR(-ENOMEM);
> >> +
> >> + dev_set_name(&pd_wdev->dev, "%s_warming_dev", pd_name);
> >
> > Perhaps skip the in-param *pd_name and make use of the suggested
> > "struct device_node *node", the index and something with "warming...",
> > when setting the name.
>
> Won't the index have to be in-param in this case ?
Isn't that already the case?
Huh, long time since I reviewed this.
>
> >
> > Just an idea, as to simplify for the caller.
> >
> >> + pd_wdev->dev.parent = parent;
> >
> > This isn't needed, I think.
So ignore this comment, as discussed above.
> >
> >> +
> >> + ret = device_register(&pd_wdev->dev);
> >> + if (ret)
> >> + goto error;
> >> +
> >> + pd_args.np = parent->of_node;
> >> + pd_args.args[0] = pd_id;
> >> + pd_args.args_count = 1;
> >> +
> >> + ret = of_genpd_add_device(&pd_args, &pd_wdev->dev);
> >> +
> >
> > White space.
>
> Will fix it.
>
> >
> >> + if (ret)
> >> + goto error;
> >> +
> >> + ret = dev_pm_genpd_performance_state_count(&pd_wdev->dev);
> >> + if (ret < 0)
> >> + goto error;
> >> +
> >> + pd_wdev->max_state = ret - 1;
> >> + pm_runtime_enable(&pd_wdev->dev);
> >> + pd_wdev->runtime_resumed = false;
> >> +
> >> + pd_wdev->cdev = thermal_of_cooling_device_parent_register
> >> + (NULL, parent, pd_name, pd_wdev,
> >> + &pd_warming_device_ops);
> >
> > As stated in patch3, I don't get it why you need to use this new API
> > for "parents".
>
> I agree with you. I cannot re-collect my thought process for this API.
> I compiled and tested using the regular API and everything works fine.
> I will drop patch 3 and use the thermal_of_cooling_device_register here.
Great, one confusing piece seems to go away then. :-)
>
> >
> >> + if (IS_ERR(pd_wdev->cdev)) {
> >> + pr_err("unable to register %s cooling device\n", pd_name);
> >> + pm_runtime_disable(&pd_wdev->dev);
> >> + ret = PTR_ERR(pd_wdev->cdev);
> >> + goto error;
> >> + }
> >> +
> >> + return pd_wdev->cdev;
> >> +error:
> >> + put_device(&pd_wdev->dev);
> >
> > If device_register() succeeds you need to call device_unregister(),
> > rather than put_device() as a part of the error handling.
>
> Will fix this.
>
> >
> >> + kfree(pd_wdev);
> >
> > You need a ->release() callback to manage kfree(), after you called
> > device_register().
>
> mm?? I did not get this. What release callback? You mean for power
> controller driver to call ?
No, this how life cycle management of devices should be implemented.
Have a look at genpd_release_dev() - and see how that is being used
for genpd's virtual devices, that should explain more.
>
> >
> >> + return ERR_PTR(ret);
> >
> > Another thing is missing in the error path, which is to remove the
> > device for the genpd. I think calling pm_genpd_remove_device() should
> > work fine here.
>
> I will fix this. I am thinking this will be be needed in
> pwr_domain_warming_unregister as well.
Yep.
Kind regards
Uffe
Hi Ulf,
Thanks for the reviews. Will send out v5 soon.
On 3/13/20 9:13 AM, Ulf Hansson wrote:
> [...]
>
>>>> +static struct thermal_cooling_device_ops pd_warming_device_ops = {
>>>> + .get_max_state = ::pd_wdev_get_max_state,
>>>> + .get_cur_state = pd_wdev_get_cur_state,
>>>> + .set_cur_state = pd_wdev_set_cur_state,
>>>> +};
>>>> +
>>>> +struct thermal_cooling_device *
>>>> +pwr_domain_warming_register(struct device *parent, char *pd_name, int pd_id)
>>>
>>> Maybe rename this to: thermal_of_pd_warming_register()
>>
>> How about pd_of_warming_register? It is consistent with other cooling
>> device register like cpuidle_of_cooling_register and
>> cpufreq_of_cooling_register.
>
> Well, we actually have the following:
> of_devfreq_cooling_register()
> of_cpufreq_cooling_register()
> cpuidle_of_cooling_register()
>
> So maybe this is the most consistent. :-)
> of_pd_warming_register()
Sure!
>
>>
>>> Moreover, I think you could replace the "struct device *parent", with
>>> a "struct device_node *node" as in-parameter. That's all you need,
>>> right?
>>
>> You mean pd_wdev->dev.parent need not be populated ? The device
>> in this case will be created under /sys/devices which I do not think
>> is the correct.
>
> Good point!
>
>> With a parent device specified, the power controller that resides the
>> power domain that can act as the warming dev, becomes the parent of the
>> warming dev. In case of this patch series, for the mx warming dev,
>> 179c0000.rsc/179c0000.rsc\:power-controller/ becomes the parent.(The
>> device will be created under
>> /sys/devices/platform/soc\@0/179c0000.rsc/179c0000.rsc\:power-controller/)
>>
>> Other way might be to register the warming device under virtual devices
>> as a new category of devices.
>
> No, that sounds wrong.
>
> Another option is to create a specific bus type for these new
> pd_warming devices. But I admit that sounds a bit too much, let's
> assign a parent.
>
>>
>> I prefer to keep it as a child of the power controller device, but I am
>> open to explore other options and to re-do this bit. What do you think?
>
> Sure, sorry for the noise.
No issues!
>
>>
>>>
>>>> +{
>>>> + struct pd_warming_device *pd_wdev;
>>>> + struct of_phandle_args pd_args;
>>>> + int ret;
>>>> +
>>>> + pd_wdev = kzalloc(sizeof(*pd_wdev), GFP_KERNEL);
>>>> + if (!pd_wdev)
>>>> + return ERR_PTR(-ENOMEM);
>>>> +
>>>> + dev_set_name(&pd_wdev->dev, "%s_warming_dev", pd_name);
>>>
>>> Perhaps skip the in-param *pd_name and make use of the suggested
>>> "struct device_node *node", the index and something with "warming...",
>>> when setting the name.
>>
>> Won't the index have to be in-param in this case ?
>
> Isn't that already the case?
>
> Huh, long time since I reviewed this.
>
>>
>>>
>>> Just an idea, as to simplify for the caller.
>>>
>>>> + pd_wdev->dev.parent = parent;
>>>
>>> This isn't needed, I think.
>
> So ignore this comment, as discussed above.
>
>>>
>>>> +
>>>> + ret = device_register(&pd_wdev->dev);
>>>> + if (ret)
>>>> + goto error;
>>>> +
>>>> + pd_args.np = parent->of_node;
>>>> + pd_args.args[0] = pd_id;
>>>> + pd_args.args_count = 1;
>>>> +
>>>> + ret = of_genpd_add_device(&pd_args, &pd_wdev->dev);
>>>> +
>>>
>>> White space.
>>
>> Will fix it.
>>
>>>
>>>> + if (ret)
>>>> + goto error;
>>>> +
>>>> + ret = dev_pm_genpd_performance_state_count(&pd_wdev->dev);
>>>> + if (ret < 0)
>>>> + goto error;
>>>> +
>>>> + pd_wdev->max_state = ret - 1;
>>>> + pm_runtime_enable(&pd_wdev->dev);
>>>> + pd_wdev->runtime_resumed = false;
>>>> +
>>>> + pd_wdev->cdev = thermal_of_cooling_device_parent_register
>>>> + (NULL, parent, pd_name, pd_wdev,
>>>> + &pd_warming_device_ops);
>>>
>>> As stated in patch3, I don't get it why you need to use this new API
>>> for "parents".
>>
>> I agree with you. I cannot re-collect my thought process for this API.
>> I compiled and tested using the regular API and everything works fine.
>> I will drop patch 3 and use the thermal_of_cooling_device_register here.
>
> Great, one confusing piece seems to go away then. :-)
>
>>
>>>
>>>> + if (IS_ERR(pd_wdev->cdev)) {
>>>> + pr_err("unable to register %s cooling device\n", pd_name);
>>>> + pm_runtime_disable(&pd_wdev->dev);
>>>> + ret = PTR_ERR(pd_wdev->cdev);
>>>> + goto error;
>>>> + }
>>>> +
>>>> + return pd_wdev->cdev;
>>>> +error:
>>>> + put_device(&pd_wdev->dev);
>>>
>>> If device_register() succeeds you need to call device_unregister(),
>>> rather than put_device() as a part of the error handling.
>>
>> Will fix this.
>>
>>>
>>>> + kfree(pd_wdev);
>>>
>>> You need a ->release() callback to manage kfree(), after you called
>>> device_register().
>>
>> mm?? I did not get this. What release callback? You mean for power
>> controller driver to call ?
>
> No, this how life cycle management of devices should be implemented.
>
> Have a look at genpd_release_dev() - and see how that is being used
> for genpd's virtual devices, that should explain more.
Ah yes. I get it now. Will fix this.
--
Warm Regards
Thara