2020-02-20 01:17:17

by Anson Huang

Subject: [PATCH V15 RESEND 1/5] dt-bindings: fsl: scu: add thermal binding

NXP i.MX8QXP is an ARMv8 SoC with a Cortex-M4 core inside as
system controller, the system controller is in charge of system
power, clock and thermal sensors etc. management, Linux kernel
has to communicate with system controller via MU (message unit)
IPC to get temperature from thermal sensors, this patch adds
binding doc for i.MX system controller thermal driver.

Signed-off-by: Anson Huang <[email protected]>
Reviewed-by: Rob Herring <[email protected]>
Reviewed-by: Dong Aisheng <[email protected]>
---
No change.
---
.../devicetree/bindings/arm/freescale/fsl,scu.txt | 16 ++++++++++++++++
1 file changed, 16 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
index e07735a8..7f42cc3 100644
--- a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
+++ b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
@@ -166,6 +166,17 @@ Required properties:
followed by "fsl,imx-sc-key";
- linux,keycodes: See Documentation/devicetree/bindings/input/keys.txt

+Thermal bindings based on SCU Message Protocol
+------------------------------------------------------------
+
+Required properties:
+- compatible: Should be :
+ "fsl,imx8qxp-sc-thermal"
+ followed by "fsl,imx-sc-thermal";
+
+- #thermal-sensor-cells: See Documentation/devicetree/bindings/thermal/thermal.txt
+ for a description.
+
Example (imx8qxp):
-------------
aliases {
@@ -238,6 +249,11 @@ firmware {
compatible = "fsl,imx8qxp-sc-wdt", "fsl,imx-sc-wdt";
timeout-sec = <60>;
};
+
+ tsens: thermal-sensor {
+ compatible = "fsl,imx8qxp-sc-thermal", "fsl,imx-sc-thermal";
+ #thermal-sensor-cells = <1>;
+ };
};
};

--
2.7.4

2020-02-20 01:17:25

by Anson Huang

Subject: [PATCH V15 RESEND 4/5] defconfig: arm64: add i.MX system controller thermal support

This patch enables CONFIG_IMX_SC_THERMAL as module.

Signed-off-by: Anson Huang <[email protected]>
---
No change.
---
arch/arm64/configs/defconfig | 1 +
1 file changed, 1 insertion(+)

diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index 1c74437..1ff479c 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -453,6 +453,7 @@ CONFIG_THERMAL_GOV_POWER_ALLOCATOR=y
CONFIG_CPU_THERMAL=y
CONFIG_THERMAL_EMULATION=y
CONFIG_QORIQ_THERMAL=m
+CONFIG_IMX_SC_THERMAL=m
CONFIG_ROCKCHIP_THERMAL=m
CONFIG_RCAR_THERMAL=y
CONFIG_RCAR_GEN3_THERMAL=y
--
2.7.4

2020-02-20 01:17:45

by Anson Huang

Subject: [PATCH V15 RESEND 2/5] thermal: of-thermal: add API for getting sensor ID from DT

This patch adds new API thermal_zone_of_get_sensor_id() to
provide the feature of getting sensor ID from DT thermal
zone's node. It's useful for thermal driver to register the
specific thermal zone devices from DT in a common way.

Signed-off-by: Anson Huang <[email protected]>
Reviewed-by: Dong Aisheng <[email protected]>
---
Changes since V14:
- improve the commit message and comment, no code change.
---
drivers/thermal/of-thermal.c | 65 +++++++++++++++++++++++++++++++++-----------
include/linux/thermal.h | 10 +++++++
2 files changed, 59 insertions(+), 16 deletions(-)

diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
index ef0baa9..0f57108 100644
--- a/drivers/thermal/of-thermal.c
+++ b/drivers/thermal/of-thermal.c
@@ -449,6 +449,53 @@ thermal_zone_of_add_sensor(struct device_node *zone,
}

/**
+ * thermal_zone_of_get_sensor_id - get sensor ID from a DT thermal zone
+ * @tz_np: a valid thermal zone device node.
+ * @sensor_np: a sensor node of a valid sensor device.
+ * @id: the sensor ID returned if success.
+ *
+ * This function will get sensor ID from a given thermal zone node and
+ * the sensor node must match the temperature provider @sensor_np.
+ *
+ * Return: 0 on success, proper error code otherwise.
+ */
+
+int thermal_zone_of_get_sensor_id(struct device_node *tz_np,
+ struct device_node *sensor_np,
+ u32 *id)
+{
+ struct of_phandle_args sensor_specs;
+ int ret;
+
+ ret = of_parse_phandle_with_args(tz_np,
+ "thermal-sensors",
+ "#thermal-sensor-cells",
+ 0,
+ &sensor_specs);
+ if (ret)
+ return ret;
+
+ if (sensor_specs.np != sensor_np) {
+ of_node_put(sensor_specs.np);
+ return -ENODEV;
+ }
+
+ if (sensor_specs.args_count >= 1) {
+ *id = sensor_specs.args[0];
+ WARN(sensor_specs.args_count > 1,
+ "%pOFn: too many cells in sensor specifier %d\n",
+ sensor_specs.np, sensor_specs.args_count);
+ } else {
+ *id = 0;
+ }
+
+ of_node_put(sensor_specs.np);
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(thermal_zone_of_get_sensor_id);
+
+/**
* thermal_zone_of_sensor_register - registers a sensor to a DT thermal zone
* @dev: a valid struct device pointer of a sensor device. Must contain
* a valid .of_node, for the sensor node.
@@ -499,36 +546,22 @@ thermal_zone_of_sensor_register(struct device *dev, int sensor_id, void *data,
sensor_np = of_node_get(dev->of_node);

for_each_available_child_of_node(np, child) {
- struct of_phandle_args sensor_specs;
int ret, id;

/* For now, thermal framework supports only 1 sensor per zone */
- ret = of_parse_phandle_with_args(child, "thermal-sensors",
- "#thermal-sensor-cells",
- 0, &sensor_specs);
+ ret = thermal_zone_of_get_sensor_id(child, sensor_np, &id);
if (ret)
continue;

- if (sensor_specs.args_count >= 1) {
- id = sensor_specs.args[0];
- WARN(sensor_specs.args_count > 1,
- "%pOFn: too many cells in sensor specifier %d\n",
- sensor_specs.np, sensor_specs.args_count);
- } else {
- id = 0;
- }
-
- if (sensor_specs.np == sensor_np && id == sensor_id) {
+ if (id == sensor_id) {
tzd = thermal_zone_of_add_sensor(child, sensor_np,
data, ops);
if (!IS_ERR(tzd))
tzd->ops->set_mode(tzd, THERMAL_DEVICE_ENABLED);

- of_node_put(sensor_specs.np);
of_node_put(child);
goto exit;
}
- of_node_put(sensor_specs.np);
}
exit:
of_node_put(sensor_np);
diff --git a/include/linux/thermal.h b/include/linux/thermal.h
index 126913c6..53e6f67 100644
--- a/include/linux/thermal.h
+++ b/include/linux/thermal.h
@@ -364,6 +364,9 @@ struct thermal_trip {

/* Function declarations */
#ifdef CONFIG_THERMAL_OF
+int thermal_zone_of_get_sensor_id(struct device_node *tz_np,
+ struct device_node *sensor_np,
+ u32 *id);
struct thermal_zone_device *
thermal_zone_of_sensor_register(struct device *dev, int id, void *data,
const struct thermal_zone_of_device_ops *ops);
@@ -375,6 +378,13 @@ struct thermal_zone_device *devm_thermal_zone_of_sensor_register(
void devm_thermal_zone_of_sensor_unregister(struct device *dev,
struct thermal_zone_device *tz);
#else
+
+static int thermal_zone_of_get_sensor_id(struct device_node *tz_np,
+ struct device_node *sensor_np,
+ u32 *id)
+{
+ return -ENOENT;
+}
static inline struct thermal_zone_device *
thermal_zone_of_sensor_register(struct device *dev, int id, void *data,
const struct thermal_zone_of_device_ops *ops)
--
2.7.4

2020-02-20 01:18:31

by Anson Huang

Subject: [PATCH V15 RESEND 5/5] arm64: dts: imx: add i.MX8QXP thermal support

Add i.MX8QXP CPU thermal zone support.

Signed-off-by: Anson Huang <[email protected]>
---
No change.
---
arch/arm64/boot/dts/freescale/imx8qxp.dtsi | 36 ++++++++++++++++++++++++++++++
1 file changed, 36 insertions(+)

diff --git a/arch/arm64/boot/dts/freescale/imx8qxp.dtsi b/arch/arm64/boot/dts/freescale/imx8qxp.dtsi
index fb5f752..0a14fe4 100644
--- a/arch/arm64/boot/dts/freescale/imx8qxp.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8qxp.dtsi
@@ -11,6 +11,7 @@
#include <dt-bindings/input/input.h>
#include <dt-bindings/interrupt-controller/arm-gic.h>
#include <dt-bindings/pinctrl/pads-imx8qxp.h>
+#include <dt-bindings/thermal/thermal.h>

/ {
interrupt-parent = <&gic>;
@@ -189,6 +190,11 @@
compatible = "fsl,imx8qxp-sc-wdt", "fsl,imx-sc-wdt";
timeout-sec = <60>;
};
+
+ tsens: thermal-sensor {
+ compatible = "fsl,imx8qxp-sc-thermal", "fsl,imx-sc-thermal";
+ #thermal-sensor-cells = <1>;
+ };
};

timer {
@@ -586,4 +592,34 @@
#clock-cells = <1>;
};
};
+
+ thermal_zones: thermal-zones {
+ cpu-thermal0 {
+ polling-delay-passive = <250>;
+ polling-delay = <2000>;
+ thermal-sensors = <&tsens IMX_SC_R_SYSTEM>;
+ trips {
+ cpu_alert0: trip0 {
+ temperature = <107000>;
+ hysteresis = <2000>;
+ type = "passive";
+ };
+ cpu_crit0: trip1 {
+ temperature = <127000>;
+ hysteresis = <2000>;
+ type = "critical";
+ };
+ };
+ cooling-maps {
+ map0 {
+ trip = <&cpu_alert0>;
+ cooling-device =
+ <&A35_0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
+ <&A35_1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
+ <&A35_2 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
+ <&A35_3 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
+ };
+ };
+ };
+ };
};
--
2.7.4

2020-02-20 01:18:55

by Anson Huang

Subject: [PATCH V15 RESEND 3/5] thermal: imx_sc: add i.MX system controller thermal support

i.MX8QXP is an ARMv8 SoC which has a Cortex-M4 system controller
inside, the system controller is in charge of controlling power,
clock and thermal sensors etc..

This patch adds i.MX system controller thermal driver support,
Linux kernel has to communicate with system controller via MU
(message unit) IPC to get each thermal sensor's temperature,
it supports multiple sensors which are passed from device tree,
please see the binding doc for details.

Signed-off-by: Anson Huang <[email protected]>
---
No change.
---
drivers/thermal/Kconfig | 11 +++
drivers/thermal/Makefile | 1 +
drivers/thermal/imx_sc_thermal.c | 142 +++++++++++++++++++++++++++++++++++++++
3 files changed, 154 insertions(+)
create mode 100644 drivers/thermal/imx_sc_thermal.c

diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
index 5a05db5..d1cb8dc 100644
--- a/drivers/thermal/Kconfig
+++ b/drivers/thermal/Kconfig
@@ -251,6 +251,17 @@ config IMX_THERMAL
cpufreq is used as the cooling device to throttle CPUs when the
passive trip is crossed.

+config IMX_SC_THERMAL
+ tristate "Temperature sensor driver for NXP i.MX SoCs with System Controller"
+ depends on ARCH_MXC && IMX_SCU
+ depends on OF
+ help
+ Support for Temperature Monitor (TEMPMON) found on NXP i.MX SoCs with
+ system controller inside, Linux kernel has to communicate with system
+ controller via MU (message unit) IPC to get temperature from thermal
+ sensor. It supports one critical trip point and one
+ passive trip point for each thermal sensor.
+
config MAX77620_THERMAL
tristate "Temperature sensor driver for Maxim MAX77620 PMIC"
depends on MFD_MAX77620
diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
index 9fb88e2..a11a6d8 100644
--- a/drivers/thermal/Makefile
+++ b/drivers/thermal/Makefile
@@ -43,6 +43,7 @@ obj-$(CONFIG_DB8500_THERMAL) += db8500_thermal.o
obj-$(CONFIG_ARMADA_THERMAL) += armada_thermal.o
obj-$(CONFIG_TANGO_THERMAL) += tango_thermal.o
obj-$(CONFIG_IMX_THERMAL) += imx_thermal.o
+obj-$(CONFIG_IMX_SC_THERMAL) += imx_sc_thermal.o
obj-$(CONFIG_MAX77620_THERMAL) += max77620_thermal.o
obj-$(CONFIG_QORIQ_THERMAL) += qoriq_thermal.o
obj-$(CONFIG_DA9062_THERMAL) += da9062-thermal.o
diff --git a/drivers/thermal/imx_sc_thermal.c b/drivers/thermal/imx_sc_thermal.c
new file mode 100644
index 0000000..d406ecb
--- /dev/null
+++ b/drivers/thermal/imx_sc_thermal.c
@@ -0,0 +1,142 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright 2018-2019 NXP.
+ */
+
+#include <linux/err.h>
+#include <linux/firmware/imx/sci.h>
+#include <linux/firmware/imx/types.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/thermal.h>
+
+#include "thermal_core.h"
+
+#define IMX_SC_MISC_FUNC_GET_TEMP 13
+
+static struct imx_sc_ipc *thermal_ipc_handle;
+
+struct imx_sc_sensor {
+ struct thermal_zone_device *tzd;
+ u32 resource_id;
+};
+
+struct req_get_temp {
+ u16 resource_id;
+ u8 type;
+} __packed;
+
+struct resp_get_temp {
+ u16 celsius;
+ u8 tenths;
+} __packed;
+
+struct imx_sc_msg_misc_get_temp {
+ struct imx_sc_rpc_msg hdr;
+ union {
+ struct req_get_temp req;
+ struct resp_get_temp resp;
+ } data;
+};
+
+static int imx_sc_thermal_get_temp(void *data, int *temp)
+{
+ struct imx_sc_msg_misc_get_temp msg;
+ struct imx_sc_rpc_msg *hdr = &msg.hdr;
+ struct imx_sc_sensor *sensor = data;
+ int ret;
+
+ msg.data.req.resource_id = sensor->resource_id;
+ msg.data.req.type = IMX_SC_C_TEMP;
+
+ hdr->ver = IMX_SC_RPC_VERSION;
+ hdr->svc = IMX_SC_RPC_SVC_MISC;
+ hdr->func = IMX_SC_MISC_FUNC_GET_TEMP;
+ hdr->size = 2;
+
+ ret = imx_scu_call_rpc(thermal_ipc_handle, &msg, true);
+ if (ret) {
+ dev_err(&sensor->tzd->device, "read temp sensor %d failed, ret %d\n",
+ sensor->resource_id, ret);
+ return ret;
+ }
+
+ *temp = msg.data.resp.celsius * 1000 + msg.data.resp.tenths * 100;
+
+ return 0;
+}
+
+static const struct thermal_zone_of_device_ops imx_sc_thermal_ops = {
+ .get_temp = imx_sc_thermal_get_temp,
+};
+
+static int imx_sc_thermal_probe(struct platform_device *pdev)
+{
+ struct device_node *np, *child, *sensor_np;
+ struct imx_sc_sensor *sensor;
+ int ret;
+
+ ret = imx_scu_get_handle(&thermal_ipc_handle);
+ if (ret)
+ return ret;
+
+ np = of_find_node_by_name(NULL, "thermal-zones");
+ if (!np)
+ return -ENODEV;
+
+ sensor_np = of_node_get(pdev->dev.of_node);
+
+ for_each_available_child_of_node(np, child) {
+ sensor = devm_kzalloc(&pdev->dev, sizeof(*sensor), GFP_KERNEL);
+ if (!sensor) {
+ of_node_put(sensor_np);
+ return -ENOMEM;
+ }
+
+ ret = thermal_zone_of_get_sensor_id(child,
+ sensor_np,
+ &sensor->resource_id);
+ if (ret < 0) {
+ dev_err(&pdev->dev,
+ "failed to get valid sensor resource id: %d\n",
+ ret);
+ break;
+ }
+
+ sensor->tzd = devm_thermal_zone_of_sensor_register(&pdev->dev,
+ sensor->resource_id,
+ sensor,
+ &imx_sc_thermal_ops);
+ if (IS_ERR(sensor->tzd)) {
+ dev_err(&pdev->dev, "failed to register thermal zone\n");
+ ret = PTR_ERR(sensor->tzd);
+ break;
+ }
+ }
+
+ of_node_put(sensor_np);
+
+ return ret;
+}
+
+static const struct of_device_id imx_sc_thermal_table[] = {
+ { .compatible = "fsl,imx-sc-thermal", },
+ {}
+};
+MODULE_DEVICE_TABLE(of, imx_sc_thermal_table);
+
+static struct platform_driver imx_sc_thermal_driver = {
+ .probe = imx_sc_thermal_probe,
+ .driver = {
+ .name = "imx-sc-thermal",
+ .of_match_table = imx_sc_thermal_table,
+ },
+};
+module_platform_driver(imx_sc_thermal_driver);
+
+MODULE_AUTHOR("Anson Huang <[email protected]>");
+MODULE_DESCRIPTION("Thermal driver for NXP i.MX SoCs with system controller");
+MODULE_LICENSE("GPL v2");
--
2.7.4

2020-02-21 09:14:57

by Daniel Lezcano

Subject: Re: [PATCH V15 RESEND 2/5] thermal: of-thermal: add API for getting sensor ID from DT

Hi,

On Thu, Feb 20, 2020 at 09:10:25AM +0800, Anson Huang wrote:
> This patch adds new API thermal_zone_of_get_sensor_id() to
> provide the feature of getting sensor ID from DT thermal
> zone's node. It's useful for thermal driver to register the
> specific thermal zone devices from DT in a common way.
>
> Signed-off-by: Anson Huang <[email protected]>
> Reviewed-by: Dong Aisheng <[email protected]>
> ---
> Changes since V14:
> - improve the commit message and comment, no code change.
> ---
> drivers/thermal/of-thermal.c | 65 +++++++++++++++++++++++++++++++++-----------
> include/linux/thermal.h | 10 +++++++
> 2 files changed, 59 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
> index ef0baa9..0f57108 100644
> --- a/drivers/thermal/of-thermal.c
> +++ b/drivers/thermal/of-thermal.c
> @@ -449,6 +449,53 @@ thermal_zone_of_add_sensor(struct device_node *zone,
> }
>
> /**
> + * thermal_zone_of_get_sensor_id - get sensor ID from a DT thermal zone
> + * @tz_np: a valid thermal zone device node.
> + * @sensor_np: a sensor node of a valid sensor device.
> + * @id: the sensor ID returned if success.
> + *
> + * This function will get sensor ID from a given thermal zone node and
> + * the sensor node must match the temperature provider @sensor_np.
> + *
> + * Return: 0 on success, proper error code otherwise.
> + */
> +
> +int thermal_zone_of_get_sensor_id(struct device_node *tz_np,
> + struct device_node *sensor_np,
> + u32 *id)
> +{
> + struct of_phandle_args sensor_specs;
> + int ret;
> +
> + ret = of_parse_phandle_with_args(tz_np,
> + "thermal-sensors",
> + "#thermal-sensor-cells",
> + 0,
> + &sensor_specs);
> + if (ret)
> + return ret;
> +
> + if (sensor_specs.np != sensor_np) {
> + of_node_put(sensor_specs.np);
> + return -ENODEV;
> + }
> +
> + if (sensor_specs.args_count >= 1) {

For the sake of clarity, move the sanity tests before:

if (sensor_specs.args_count > 1)
pr_warn("...");

*id = sensor_specs.args_count ? sensor_specs.args[0] : 0;

> + *id = sensor_specs.args[0];
> + WARN(sensor_specs.args_count > 1,
> + "%pOFn: too many cells in sensor specifier %d\n",
> + sensor_specs.np, sensor_specs.args_count);
> + } else {
> + *id = 0;
> + }
> +
> + of_node_put(sensor_specs.np);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(thermal_zone_of_get_sensor_id);
> +
> +/**
> * thermal_zone_of_sensor_register - registers a sensor to a DT thermal zone
> * @dev: a valid struct device pointer of a sensor device. Must contain
> * a valid .of_node, for the sensor node.
> @@ -499,36 +546,22 @@ thermal_zone_of_sensor_register(struct device *dev, int sensor_id, void *data,
> sensor_np = of_node_get(dev->of_node);
>
> for_each_available_child_of_node(np, child) {
> - struct of_phandle_args sensor_specs;
> int ret, id;
>
> /* For now, thermal framework supports only 1 sensor per zone */
> - ret = of_parse_phandle_with_args(child, "thermal-sensors",
> - "#thermal-sensor-cells",
> - 0, &sensor_specs);
> + ret = thermal_zone_of_get_sensor_id(child, sensor_np, &id);
> if (ret)
> continue;
>
> - if (sensor_specs.args_count >= 1) {
> - id = sensor_specs.args[0];
> - WARN(sensor_specs.args_count > 1,
> - "%pOFn: too many cells in sensor specifier %d\n",
> - sensor_specs.np, sensor_specs.args_count);
> - } else {
> - id = 0;
> - }

Please take also the opportunity to factor out the function
thermal_zone_of_sensor_register().

> - if (sensor_specs.np == sensor_np && id == sensor_id) {
> + if (id == sensor_id) {
> tzd = thermal_zone_of_add_sensor(child, sensor_np,
> data, ops);
> if (!IS_ERR(tzd))
> tzd->ops->set_mode(tzd, THERMAL_DEVICE_ENABLED);
>
> - of_node_put(sensor_specs.np);
> of_node_put(child);
> goto exit;
> }
> - of_node_put(sensor_specs.np);
> }
> exit:
> of_node_put(sensor_np);
> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> index 126913c6..53e6f67 100644
> --- a/include/linux/thermal.h
> +++ b/include/linux/thermal.h
> @@ -364,6 +364,9 @@ struct thermal_trip {
>
> /* Function declarations */
> #ifdef CONFIG_THERMAL_OF
> +int thermal_zone_of_get_sensor_id(struct device_node *tz_np,
> + struct device_node *sensor_np,
> + u32 *id);
> struct thermal_zone_device *
> thermal_zone_of_sensor_register(struct device *dev, int id, void *data,
> const struct thermal_zone_of_device_ops *ops);
> @@ -375,6 +378,13 @@ struct thermal_zone_device *devm_thermal_zone_of_sensor_register(
> void devm_thermal_zone_of_sensor_unregister(struct device *dev,
> struct thermal_zone_device *tz);
> #else
> +
> +static int thermal_zone_of_get_sensor_id(struct device_node *tz_np,
> + struct device_node *sensor_np,
> + u32 *id)
> +{
> + return -ENOENT;
> +}
> static inline struct thermal_zone_device *
> thermal_zone_of_sensor_register(struct device *dev, int id, void *data,
> const struct thermal_zone_of_device_ops *ops)
> --
> 2.7.4
>

--

<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

2020-02-21 09:28:27

by Anson Huang

Subject: RE: [PATCH V15 RESEND 2/5] thermal: of-thermal: add API for getting sensor ID from DT

Hi, Daniel

> Subject: Re: [PATCH V15 RESEND 2/5] thermal: of-thermal: add API for getting
> sensor ID from DT
>
> Hi,
>
> On Thu, Feb 20, 2020 at 09:10:25AM +0800, Anson Huang wrote:
> > This patch adds new API thermal_zone_of_get_sensor_id() to provide the
> > feature of getting sensor ID from DT thermal zone's node. It's useful
> > for thermal driver to register the specific thermal zone devices from
> > DT in a common way.
> >
> > Signed-off-by: Anson Huang <[email protected]>
> > Reviewed-by: Dong Aisheng <[email protected]>
> > ---
> > Changes since V14:
> > - improve the commit message and comment, no code change.
> > ---
> > drivers/thermal/of-thermal.c | 65 +++++++++++++++++++++++++++++++++-
> ----------
> > include/linux/thermal.h | 10 +++++++
> > 2 files changed, 59 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/thermal/of-thermal.c
> > b/drivers/thermal/of-thermal.c index ef0baa9..0f57108 100644
> > --- a/drivers/thermal/of-thermal.c
> > +++ b/drivers/thermal/of-thermal.c
> > @@ -449,6 +449,53 @@ thermal_zone_of_add_sensor(struct device_node
> > *zone, }
> >
> > /**
> > + * thermal_zone_of_get_sensor_id - get sensor ID from a DT thermal
> > + zone
> > + * @tz_np: a valid thermal zone device node.
> > + * @sensor_np: a sensor node of a valid sensor device.
> > + * @id: the sensor ID returned if success.
> > + *
> > + * This function will get sensor ID from a given thermal zone node
> > + and
> > + * the sensor node must match the temperature provider @sensor_np.
> > + *
> > + * Return: 0 on success, proper error code otherwise.
> > + */
> > +
> > +int thermal_zone_of_get_sensor_id(struct device_node *tz_np,
> > + struct device_node *sensor_np,
> > + u32 *id)
> > +{
> > + struct of_phandle_args sensor_specs;
> > + int ret;
> > +
> > + ret = of_parse_phandle_with_args(tz_np,
> > + "thermal-sensors",
> > + "#thermal-sensor-cells",
> > + 0,
> > + &sensor_specs);
> > + if (ret)
> > + return ret;
> > +
> > + if (sensor_specs.np != sensor_np) {
> > + of_node_put(sensor_specs.np);
> > + return -ENODEV;
> > + }
> > +
> > + if (sensor_specs.args_count >= 1) {
>
> For the sake of clarity, move the sanity tests before:
>
> if (sensor_specs.args_count > 1)
> pr_warn("...");
>
> *id = sensor_specs.args_count ? sensor_specs.args[0] : 0;
>

Make sense, will improve it in V16.

> > + *id = sensor_specs.args[0];
> > + WARN(sensor_specs.args_count > 1,
> > + "%pOFn: too many cells in sensor specifier %d\n",
> > + sensor_specs.np, sensor_specs.args_count);
> > + } else {
> > + *id = 0;
> > + }
> > +
> > + of_node_put(sensor_specs.np);
> > +
> > + return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(thermal_zone_of_get_sensor_id);
> > +
> > +/**
> > * thermal_zone_of_sensor_register - registers a sensor to a DT thermal
> zone
> > * @dev: a valid struct device pointer of a sensor device. Must contain
> > * a valid .of_node, for the sensor node.
> > @@ -499,36 +546,22 @@ thermal_zone_of_sensor_register(struct device
> *dev, int sensor_id, void *data,
> > sensor_np = of_node_get(dev->of_node);
> >
> > for_each_available_child_of_node(np, child) {
> > - struct of_phandle_args sensor_specs;
> > int ret, id;
> >
> > /* For now, thermal framework supports only 1 sensor per
> zone */
> > - ret = of_parse_phandle_with_args(child, "thermal-sensors",
> > - "#thermal-sensor-cells",
> > - 0, &sensor_specs);
> > + ret = thermal_zone_of_get_sensor_id(child, sensor_np, &id);
> > if (ret)
> > continue;
> >
> > - if (sensor_specs.args_count >= 1) {
> > - id = sensor_specs.args[0];
> > - WARN(sensor_specs.args_count > 1,
> > - "%pOFn: too many cells in sensor specifier %d\n",
> > - sensor_specs.np, sensor_specs.args_count);
> > - } else {
> > - id = 0;
> > - }
>
> Please take also the opportunity to factor out the function
> thermal_zone_of_sensor_register().

Sorry, I do NOT quite understand terms "factor out the function ...", could you please advise more detail?

Thanks,
Anson

2020-02-21 12:53:37

by Daniel Lezcano

Subject: Re: [PATCH V15 RESEND 3/5] thermal: imx_sc: add i.MX system controller thermal support

Hi Anson,

sorry for the delay with this review, hopefully the upstreaming will be now a
bit more smooth.

Apart the comments below, the driver looks good to me.

On Thu, Feb 20, 2020 at 09:10:26AM +0800, Anson Huang wrote:
> i.MX8QXP is an ARMv8 SoC which has a Cortex-M4 system controller
> inside, the system controller is in charge of controlling power,
> clock and thermal sensors etc..
>
> This patch adds i.MX system controller thermal driver support,
> Linux kernel has to communicate with system controller via MU
> (message unit) IPC to get each thermal sensor's temperature,
> it supports multiple sensors which are passed from device tree,
> please see the binding doc for details.
>
> Signed-off-by: Anson Huang <[email protected]>
> ---
> No change.
> ---
> drivers/thermal/Kconfig | 11 +++
> drivers/thermal/Makefile | 1 +
> drivers/thermal/imx_sc_thermal.c | 142 +++++++++++++++++++++++++++++++++++++++
> 3 files changed, 154 insertions(+)
> create mode 100644 drivers/thermal/imx_sc_thermal.c
>
> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
> index 5a05db5..d1cb8dc 100644
> --- a/drivers/thermal/Kconfig
> +++ b/drivers/thermal/Kconfig
> @@ -251,6 +251,17 @@ config IMX_THERMAL
> cpufreq is used as the cooling device to throttle CPUs when the
> passive trip is crossed.
>
> +config IMX_SC_THERMAL
> + tristate "Temperature sensor driver for NXP i.MX SoCs with System Controller"
> + depends on ARCH_MXC && IMX_SCU

IMX_SCU depends on IMX_MBOX which depends on ARCH_MXC. This dependency could be
simplified.

Also add the COMPILE_TEST option to improve compilation test coverage.

> + depends on OF
> + help
> + Support for Temperature Monitor (TEMPMON) found on NXP i.MX SoCs with
> + system controller inside, Linux kernel has to communicate with system
> + controller via MU (message unit) IPC to get temperature from thermal
> + sensor. It supports one critical trip point and one
> + passive trip point for each thermal sensor.
> +
> config MAX77620_THERMAL
> tristate "Temperature sensor driver for Maxim MAX77620 PMIC"
> depends on MFD_MAX77620
> diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
> index 9fb88e2..a11a6d8 100644
> --- a/drivers/thermal/Makefile
> +++ b/drivers/thermal/Makefile
> @@ -43,6 +43,7 @@ obj-$(CONFIG_DB8500_THERMAL) += db8500_thermal.o
> obj-$(CONFIG_ARMADA_THERMAL) += armada_thermal.o
> obj-$(CONFIG_TANGO_THERMAL) += tango_thermal.o
> obj-$(CONFIG_IMX_THERMAL) += imx_thermal.o
> +obj-$(CONFIG_IMX_SC_THERMAL) += imx_sc_thermal.o
> obj-$(CONFIG_MAX77620_THERMAL) += max77620_thermal.o
> obj-$(CONFIG_QORIQ_THERMAL) += qoriq_thermal.o
> obj-$(CONFIG_DA9062_THERMAL) += da9062-thermal.o
> diff --git a/drivers/thermal/imx_sc_thermal.c b/drivers/thermal/imx_sc_thermal.c
> new file mode 100644
> index 0000000..d406ecb
> --- /dev/null
> +++ b/drivers/thermal/imx_sc_thermal.c
> @@ -0,0 +1,142 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright 2018-2019 NXP.

*sigh* 2020 now ...

[ ... ]

> +static int imx_sc_thermal_get_temp(void *data, int *temp)
> +{
> + struct imx_sc_msg_misc_get_temp msg;
> + struct imx_sc_rpc_msg *hdr = &msg.hdr;
> + struct imx_sc_sensor *sensor = data;
> + int ret;
> +
> + msg.data.req.resource_id = sensor->resource_id;
> + msg.data.req.type = IMX_SC_C_TEMP;
> +
> + hdr->ver = IMX_SC_RPC_VERSION;
> + hdr->svc = IMX_SC_RPC_SVC_MISC;
> + hdr->func = IMX_SC_MISC_FUNC_GET_TEMP;
> + hdr->size = 2;

Can you explain this 'size' value?

[ ... ]

> +MODULE_DEVICE_TABLE(of, imx_sc_thermal_table);
> +
> +static struct platform_driver imx_sc_thermal_driver = {
> + .probe = imx_sc_thermal_probe,

The driver can be compiled as module but there is no 'remove' callback

> + .driver = {
> + .name = "imx-sc-thermal",
> + .of_match_table = imx_sc_thermal_table,
> + },
> +};
> +module_platform_driver(imx_sc_thermal_driver);
> +
> +MODULE_AUTHOR("Anson Huang <[email protected]>");
> +MODULE_DESCRIPTION("Thermal driver for NXP i.MX SoCs with system controller");
> +MODULE_LICENSE("GPL v2");



--

<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

2020-02-21 13:05:55

by Daniel Lezcano

Subject: Re: [PATCH V15 RESEND 5/5] arm64: dts: imx: add i.MX8QXP thermal support

On Thu, Feb 20, 2020 at 09:10:28AM +0800, Anson Huang wrote:
> Add i.MX8QXP CPU thermal zone support.
>
> Signed-off-by: Anson Huang <[email protected]>
> ---
> No change.
> ---
> arch/arm64/boot/dts/freescale/imx8qxp.dtsi | 36 ++++++++++++++++++++++++++++++
> 1 file changed, 36 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/freescale/imx8qxp.dtsi b/arch/arm64/boot/dts/freescale/imx8qxp.dtsi
> index fb5f752..0a14fe4 100644
> --- a/arch/arm64/boot/dts/freescale/imx8qxp.dtsi
> +++ b/arch/arm64/boot/dts/freescale/imx8qxp.dtsi
> @@ -11,6 +11,7 @@
> #include <dt-bindings/input/input.h>
> #include <dt-bindings/interrupt-controller/arm-gic.h>
> #include <dt-bindings/pinctrl/pads-imx8qxp.h>
> +#include <dt-bindings/thermal/thermal.h>
>
> / {
> interrupt-parent = <&gic>;
> @@ -189,6 +190,11 @@
> compatible = "fsl,imx8qxp-sc-wdt", "fsl,imx-sc-wdt";
> timeout-sec = <60>;
> };
> +
> + tsens: thermal-sensor {
> + compatible = "fsl,imx8qxp-sc-thermal", "fsl,imx-sc-thermal";
> + #thermal-sensor-cells = <1>;
> + };
> };
>
> timer {
> @@ -586,4 +592,34 @@
> #clock-cells = <1>;
> };
> };
> +
> + thermal_zones: thermal-zones {
> + cpu-thermal0 {
> + polling-delay-passive = <250>;
> + polling-delay = <2000>;
> + thermal-sensors = <&tsens IMX_SC_R_SYSTEM>;
> + trips {
> + cpu_alert0: trip0 {
> + temperature = <107000>;
> + hysteresis = <2000>;
> + type = "passive";
> + };

May be you can add a 'hot' trip point before 'critical' for future use before
reaching the emergency shutdown.

> + cpu_crit0: trip1 {
> + temperature = <127000>;
> + hysteresis = <2000>;
> + type = "critical";
> + };
> + };
> + cooling-maps {
> + map0 {
> + trip = <&cpu_alert0>;
> + cooling-device =
> + <&A35_0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> + <&A35_1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> + <&A35_2 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> + <&A35_3 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
> + };
> + };
> + };
> + };
> };
> --
> 2.7.4
>

--

<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

2020-02-21 13:07:22

by Daniel Lezcano

Subject: Re: [PATCH V15 RESEND 5/5] arm64: dts: imx: add i.MX8QXP thermal support

On Thu, Feb 20, 2020 at 09:10:28AM +0800, Anson Huang wrote:
> Add i.MX8QXP CPU thermal zone support.
>
> Signed-off-by: Anson Huang <[email protected]>
> ---
> No change.
> ---
> arch/arm64/boot/dts/freescale/imx8qxp.dtsi | 36 ++++++++++++++++++++++++++++++
> 1 file changed, 36 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/freescale/imx8qxp.dtsi b/arch/arm64/boot/dts/freescale/imx8qxp.dtsi
> index fb5f752..0a14fe4 100644
> --- a/arch/arm64/boot/dts/freescale/imx8qxp.dtsi
> +++ b/arch/arm64/boot/dts/freescale/imx8qxp.dtsi
> @@ -11,6 +11,7 @@
> #include <dt-bindings/input/input.h>
> #include <dt-bindings/interrupt-controller/arm-gic.h>
> #include <dt-bindings/pinctrl/pads-imx8qxp.h>
> +#include <dt-bindings/thermal/thermal.h>
>
> / {
> interrupt-parent = <&gic>;
> @@ -189,6 +190,11 @@
> compatible = "fsl,imx8qxp-sc-wdt", "fsl,imx-sc-wdt";
> timeout-sec = <60>;
> };
> +
> + tsens: thermal-sensor {
> + compatible = "fsl,imx8qxp-sc-thermal", "fsl,imx-sc-thermal";
> + #thermal-sensor-cells = <1>;
> + };
> };
>
> timer {
> @@ -586,4 +592,34 @@
> #clock-cells = <1>;
> };
> };
> +
> + thermal_zones: thermal-zones {
> + cpu-thermal0 {
> + polling-delay-passive = <250>;
> + polling-delay = <2000>;
> + thermal-sensors = <&tsens IMX_SC_R_SYSTEM>;
> + trips {
> + cpu_alert0: trip0 {
> + temperature = <107000>;
> + hysteresis = <2000>;
> + type = "passive";
> + };

Same comment as previous patch.

> + cpu_crit0: trip1 {
> + temperature = <127000>;
> + hysteresis = <2000>;
> + type = "critical";
> + };
> + };
> + cooling-maps {
> + map0 {
> + trip = <&cpu_alert0>;
> + cooling-device =
> + <&A35_0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> + <&A35_1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> + <&A35_2 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> + <&A35_3 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
> + };
> + };
> + };
> + };
> };
> --
> 2.7.4
>

--

<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

2020-02-21 13:13:03

by Daniel Lezcano

Subject: Re: [PATCH V15 RESEND 2/5] thermal: of-thermal: add API for getting sensor ID from DT

On Fri, Feb 21, 2020 at 09:26:29AM +0000, Anson Huang wrote:
> Hi, Daniel
>
> > > * a valid .of_node, for the sensor node.
> > > @@ -499,36 +546,22 @@ thermal_zone_of_sensor_register(struct device
> > *dev, int sensor_id, void *data,
> > > sensor_np = of_node_get(dev->of_node);
> > >
> > > for_each_available_child_of_node(np, child) {
> > > - struct of_phandle_args sensor_specs;
> > > int ret, id;
> > >
> > > /* For now, thermal framework supports only 1 sensor per
> > zone */
> > > - ret = of_parse_phandle_with_args(child, "thermal-sensors",
> > > - "#thermal-sensor-cells",
> > > - 0, &sensor_specs);
> > > + ret = thermal_zone_of_get_sensor_id(child, sensor_np, &id);
> > > if (ret)
> > > continue;
> > >
> > > - if (sensor_specs.args_count >= 1) {
> > > - id = sensor_specs.args[0];
> > > - WARN(sensor_specs.args_count > 1,
> > > - "%pOFn: too many cells in sensor specifier %d\n",
> > > - sensor_specs.np, sensor_specs.args_count);
> > > - } else {
> > > - id = 0;
> > > - }
> >
> > Please take also the opportunity to factor out the function
> > thermal_zone_of_sensor_register().
>
> Sorry, I do NOT quite understand terms "factor out the function ...", could you please advise more detail?

Never mind, I realized I puzzled myself with the changes in the series :)

Thanks

-- Daniel

--

<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

2020-02-21 23:46:59

by Anson Huang

Subject: RE: [PATCH V15 RESEND 3/5] thermal: imx_sc: add i.MX system controller thermal support

Hi, Daniel

> Subject: Re: [PATCH V15 RESEND 3/5] thermal: imx_sc: add i.MX system
> controller thermal support
>
> Hi Anson,
>
> sorry for the delay with this review, hopefully the upstreaming will be now a
> bit more smooth.

Thanks very much for review????

>
> Apart the comments below, the driver looks good to me.
>
> On Thu, Feb 20, 2020 at 09:10:26AM +0800, Anson Huang wrote:
> > i.MX8QXP is an ARMv8 SoC which has a Cortex-M4 system controller
> > inside, the system controller is in charge of controlling power, clock
> > and thermal sensors etc..
> >
> > This patch adds i.MX system controller thermal driver support, Linux
> > kernel has to communicate with system controller via MU (message unit)
> > IPC to get each thermal sensor's temperature, it supports multiple
> > sensors which are passed from device tree, please see the binding doc
> > for details.
> >
> > Signed-off-by: Anson Huang <[email protected]>
> > ---
> > No change.
> > ---
> > drivers/thermal/Kconfig | 11 +++
> > drivers/thermal/Makefile | 1 +
> > drivers/thermal/imx_sc_thermal.c | 142
> > +++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 154 insertions(+)
> > create mode 100644 drivers/thermal/imx_sc_thermal.c
> >
> > diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig index
> > 5a05db5..d1cb8dc 100644
> > --- a/drivers/thermal/Kconfig
> > +++ b/drivers/thermal/Kconfig
> > @@ -251,6 +251,17 @@ config IMX_THERMAL
> > cpufreq is used as the cooling device to throttle CPUs when the
> > passive trip is crossed.
> >
> > +config IMX_SC_THERMAL
> > + tristate "Temperature sensor driver for NXP i.MX SoCs with System
> Controller"
> > + depends on ARCH_MXC && IMX_SCU
>
> IMX_SCU depends on IMX_MBOX which depends on ARCH_MXC. This
> dependency could be simplified.
>
> Also add the COMPILE_TEST option to improve compilation test coverage.

Will make it depends on IMX_SCU and COMPILE_TEST

>
> > + depends on OF
> > + help
> > + Support for Temperature Monitor (TEMPMON) found on NXP i.MX
> SoCs with
> > + system controller inside, Linux kernel has to communicate with
> system
> > + controller via MU (message unit) IPC to get temperature from
> thermal
> > + sensor. It supports one critical trip point and one
> > + passive trip point for each thermal sensor.
> > +
> > config MAX77620_THERMAL
> > tristate "Temperature sensor driver for Maxim MAX77620 PMIC"
> > depends on MFD_MAX77620
> > diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile index
> > 9fb88e2..a11a6d8 100644
> > --- a/drivers/thermal/Makefile
> > +++ b/drivers/thermal/Makefile
> > @@ -43,6 +43,7 @@ obj-$(CONFIG_DB8500_THERMAL) +=
> db8500_thermal.o
> > obj-$(CONFIG_ARMADA_THERMAL) += armada_thermal.o
> > obj-$(CONFIG_TANGO_THERMAL) += tango_thermal.o
> > obj-$(CONFIG_IMX_THERMAL) += imx_thermal.o
> > +obj-$(CONFIG_IMX_SC_THERMAL) += imx_sc_thermal.o
> > obj-$(CONFIG_MAX77620_THERMAL) += max77620_thermal.o
> > obj-$(CONFIG_QORIQ_THERMAL) += qoriq_thermal.o
> > obj-$(CONFIG_DA9062_THERMAL) += da9062-thermal.o
> > diff --git a/drivers/thermal/imx_sc_thermal.c
> > b/drivers/thermal/imx_sc_thermal.c
> > new file mode 100644
> > index 0000000..d406ecb
> > --- /dev/null
> > +++ b/drivers/thermal/imx_sc_thermal.c
> > @@ -0,0 +1,142 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright 2018-2019 NXP.
>
> *sigh* 2020 now ...

Yes, should be 2018-2020

>
> [ ... ]
>
> > +static int imx_sc_thermal_get_temp(void *data, int *temp) {
> > + struct imx_sc_msg_misc_get_temp msg;
> > + struct imx_sc_rpc_msg *hdr = &msg.hdr;
> > + struct imx_sc_sensor *sensor = data;
> > + int ret;
> > +
> > + msg.data.req.resource_id = sensor->resource_id;
> > + msg.data.req.type = IMX_SC_C_TEMP;
> > +
> > + hdr->ver = IMX_SC_RPC_VERSION;
> > + hdr->svc = IMX_SC_RPC_SVC_MISC;
> > + hdr->func = IMX_SC_MISC_FUNC_GET_TEMP;
> > + hdr->size = 2;
>
> Can you explain this 'size' value?

The size means the SCU message size, including the header and the data, its unit
is word(4 bytes), in thermal get temperature message, the header takes 1 word and
the data takes another 1, so it is 2, we all pass the size in this way to SCU in i.MX8
SoCs, the SCU know how long message it will need to receive from AP.

>
> [ ... ]
>
> > +MODULE_DEVICE_TABLE(of, imx_sc_thermal_table);
> > +
> > +static struct platform_driver imx_sc_thermal_driver = {
> > + .probe = imx_sc_thermal_probe,
>
> The driver can be compiled as module but there is no 'remove' callback

As there is nothing needs to be done in .remove callback, so I skip it. But
I think I can add a blank .remove callback to make it more complete.

Thanks,
Anson

>
> > + .driver = {
> > + .name = "imx-sc-thermal",
> > + .of_match_table = imx_sc_thermal_table,
> > + },
> > +};
> > +module_platform_driver(imx_sc_thermal_driver);
> > +
> > +MODULE_AUTHOR("Anson Huang <[email protected]>");
> > +MODULE_DESCRIPTION("Thermal driver for NXP i.MX SoCs with system
> > +controller"); MODULE_LICENSE("GPL v2");
>
>
>
> --
>
>
> <https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.
> linaro.org%2F&amp;data=02%7C01%7Canson.huang%40nxp.com%7Cd5ce55
> aaab4141f5f6e308d7b6ccfe72%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0
> %7C0%7C637178863864826002&amp;sdata=1XOKNrrIkU9zXkljOAcSxPcKAF4g
> jShBav%2FyGO612FM%3D&amp;reserved=0> Linaro.org │ Open source
> software for ARM SoCs
>
> Follow Linaro:
> <https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.
> facebook.com%2Fpages%2FLinaro&amp;data=02%7C01%7Canson.huang%4
> 0nxp.com%7Cd5ce55aaab4141f5f6e308d7b6ccfe72%7C686ea1d3bc2b4c6fa9
> 2cd99c5c301635%7C0%7C0%7C637178863864826002&amp;sdata=gpzxYFcEV
> c4LsenqNFbMVS5Yvx8GLeqitoNB66bn8v4%3D&amp;reserved=0> Facebook |
> <https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Ftwitte
> r.com%2F%23!%2Flinaroorg&amp;data=02%7C01%7Canson.huang%40nxp.c
> om%7Cd5ce55aaab4141f5f6e308d7b6ccfe72%7C686ea1d3bc2b4c6fa92cd99c
> 5c301635%7C0%7C0%7C637178863864835997&amp;sdata=TXyn%2F1%2B2b
> sCKhCibdSjdlyMe7RqjC8LzVUh%2FrYOnNPs%3D&amp;reserved=0> Twitter |
> <https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.
> linaro.org%2Flinaro-
> blog%2F&amp;data=02%7C01%7Canson.huang%40nxp.com%7Cd5ce55aaab
> 4141f5f6e308d7b6ccfe72%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C
> 0%7C637178863864835997&amp;sdata=LCNFfsfYGB1T1IFNdh4dmjZJ8S4NIAv
> iPpihR%2FXaAYA%3D&amp;reserved=0> Blog

2020-02-21 23:53:51

by Anson Huang

Subject: RE: [PATCH V15 RESEND 5/5] arm64: dts: imx: add i.MX8QXP thermal support

Hi, Daniel

> Subject: Re: [PATCH V15 RESEND 5/5] arm64: dts: imx: add i.MX8QXP thermal
> support
>
> On Thu, Feb 20, 2020 at 09:10:28AM +0800, Anson Huang wrote:
> > Add i.MX8QXP CPU thermal zone support.
> >
> > Signed-off-by: Anson Huang <[email protected]>
> > ---
> > No change.
> > ---
> > arch/arm64/boot/dts/freescale/imx8qxp.dtsi | 36
> > ++++++++++++++++++++++++++++++
> > 1 file changed, 36 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/freescale/imx8qxp.dtsi
> > b/arch/arm64/boot/dts/freescale/imx8qxp.dtsi
> > index fb5f752..0a14fe4 100644
> > --- a/arch/arm64/boot/dts/freescale/imx8qxp.dtsi
> > +++ b/arch/arm64/boot/dts/freescale/imx8qxp.dtsi
> > @@ -11,6 +11,7 @@
> > #include <dt-bindings/input/input.h>
> > #include <dt-bindings/interrupt-controller/arm-gic.h>
> > #include <dt-bindings/pinctrl/pads-imx8qxp.h>
> > +#include <dt-bindings/thermal/thermal.h>
> >
> > / {
> > interrupt-parent = <&gic>;
> > @@ -189,6 +190,11 @@
> > compatible = "fsl,imx8qxp-sc-wdt", "fsl,imx-sc-wdt";
> > timeout-sec = <60>;
> > };
> > +
> > + tsens: thermal-sensor {
> > + compatible = "fsl,imx8qxp-sc-thermal", "fsl,imx-sc-
> thermal";
> > + #thermal-sensor-cells = <1>;
> > + };
> > };
> >
> > timer {
> > @@ -586,4 +592,34 @@
> > #clock-cells = <1>;
> > };
> > };
> > +
> > + thermal_zones: thermal-zones {
> > + cpu-thermal0 {
> > + polling-delay-passive = <250>;
> > + polling-delay = <2000>;
> > + thermal-sensors = <&tsens IMX_SC_R_SYSTEM>;
> > + trips {
> > + cpu_alert0: trip0 {
> > + temperature = <107000>;
> > + hysteresis = <2000>;
> > + type = "passive";
> > + };
>
> May be you can add a 'hot' trip point before 'critical' for future use before
> reaching the emergency shutdown.

The 'passive' trip is actually the 'hot' trip point you mentioned, and I have combined it to
below cooling map which will throttle cpu-freq when passive (hot) point is reached.
We all use 'passive' as 'hot' alarm and trigger cpu-freq throttle on i.MX platforms.

Thanks,
Anson

>
> > + cpu_crit0: trip1 {
> > + temperature = <127000>;
> > + hysteresis = <2000>;
> > + type = "critical";
> > + };
> > + };
> > + cooling-maps {
> > + map0 {
> > + trip = <&cpu_alert0>;
> > + cooling-device =
> > + <&A35_0
> THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> > + <&A35_1
> THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> > + <&A35_2
> THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> > + <&A35_3
> THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
> > + };
> > + };
> > + };
> > + };
> > };
> > --
> > 2.7.4
> >
>
> --
>
>
> <https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.
> linaro.org%2F&amp;data=02%7C01%7CAnson.Huang%40nxp.com%7Cd6c0d
> 0ce57a14985a39008d7b6cea4ff%7C686ea1d3bc2b4c6fa92cd99c5c301635%7
> C0%7C0%7C637178870951977751&amp;sdata=a7BXeoGKPsYlXmy0sCmA6IM
> SbexdiXwZEXAe8o%2BI3j0%3D&amp;reserved=0> Linaro.org │ Open source
> software for ARM SoCs
>
> Follow Linaro:
> <https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.
> facebook.com%2Fpages%2FLinaro&amp;data=02%7C01%7CAnson.Huang%4
> 0nxp.com%7Cd6c0d0ce57a14985a39008d7b6cea4ff%7C686ea1d3bc2b4c6fa9
> 2cd99c5c301635%7C0%7C0%7C637178870951977751&amp;sdata=MVpiED%
> 2Blp4h%2FV4EQsMS%2FWT4QvLJYjOZ%2FIpugeO401Vc%3D&amp;reserved=
> 0> Facebook |
> <https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Ftwitte
> r.com%2F%23!%2Flinaroorg&amp;data=02%7C01%7CAnson.Huang%40nxp.c
> om%7Cd6c0d0ce57a14985a39008d7b6cea4ff%7C686ea1d3bc2b4c6fa92cd99
> c5c301635%7C0%7C0%7C637178870951977751&amp;sdata=8sLeSLpoO9TtIsf
> jG8hco5a%2FJv5h%2BbDjM4YB43uy%2FF0%3D&amp;reserved=0> Twitter |
> <https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.
> linaro.org%2Flinaro-
> blog%2F&amp;data=02%7C01%7CAnson.Huang%40nxp.com%7Cd6c0d0ce57
> a14985a39008d7b6cea4ff%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C
> 0%7C637178870951977751&amp;sdata=XL58fpKLCFqOlQWpi7midoItJJsBamij
> XFyA2qT%2BwjM%3D&amp;reserved=0> Blog

2020-02-22 07:39:51

by Daniel Lezcano

Subject: Re: [PATCH V15 RESEND 3/5] thermal: imx_sc: add i.MX system controller thermal support

On 22/02/2020 00:46, Anson Huang wrote:
> Hi, Daniel
>
>> Subject: Re: [PATCH V15 RESEND 3/5] thermal: imx_sc: add i.MX system
>> controller thermal support
>>
>> Hi Anson,
>>
>> sorry for the delay with this review, hopefully the upstreaming will be now a
>> bit more smooth.
>
> Thanks very much for review????
>
>>
>> Apart the comments below, the driver looks good to me.
>>
>> On Thu, Feb 20, 2020 at 09:10:26AM +0800, Anson Huang wrote:
>>> i.MX8QXP is an ARMv8 SoC which has a Cortex-M4 system controller
>>> inside, the system controller is in charge of controlling power, clock
>>> and thermal sensors etc..
>>>
>>> This patch adds i.MX system controller thermal driver support, Linux
>>> kernel has to communicate with system controller via MU (message unit)
>>> IPC to get each thermal sensor's temperature, it supports multiple
>>> sensors which are passed from device tree, please see the binding doc
>>> for details.
>>>
>>> Signed-off-by: Anson Huang <[email protected]>
>>> ---
>>> No change.
>>> ---
>>> drivers/thermal/Kconfig | 11 +++
>>> drivers/thermal/Makefile | 1 +
>>> drivers/thermal/imx_sc_thermal.c | 142
>>> +++++++++++++++++++++++++++++++++++++++
>>> 3 files changed, 154 insertions(+)
>>> create mode 100644 drivers/thermal/imx_sc_thermal.c
>>>
>>> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig index
>>> 5a05db5..d1cb8dc 100644
>>> --- a/drivers/thermal/Kconfig
>>> +++ b/drivers/thermal/Kconfig
>>> @@ -251,6 +251,17 @@ config IMX_THERMAL
>>> cpufreq is used as the cooling device to throttle CPUs when the
>>> passive trip is crossed.
>>>
>>> +config IMX_SC_THERMAL
>>> + tristate "Temperature sensor driver for NXP i.MX SoCs with System
>> Controller"
>>> + depends on ARCH_MXC && IMX_SCU
>>
>> IMX_SCU depends on IMX_MBOX which depends on ARCH_MXC. This
>> dependency could be simplified.
>>
>> Also add the COMPILE_TEST option to improve compilation test coverage.
>
> Will make it depends on IMX_SCU and COMPILE_TEST
>
>>
>>> + depends on OF
>>> + help
>>> + Support for Temperature Monitor (TEMPMON) found on NXP i.MX
>> SoCs with
>>> + system controller inside, Linux kernel has to communicate with
>> system
>>> + controller via MU (message unit) IPC to get temperature from
>> thermal
>>> + sensor. It supports one critical trip point and one
>>> + passive trip point for each thermal sensor.
>>> +
>>> config MAX77620_THERMAL
>>> tristate "Temperature sensor driver for Maxim MAX77620 PMIC"
>>> depends on MFD_MAX77620
>>> diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile index
>>> 9fb88e2..a11a6d8 100644
>>> --- a/drivers/thermal/Makefile
>>> +++ b/drivers/thermal/Makefile
>>> @@ -43,6 +43,7 @@ obj-$(CONFIG_DB8500_THERMAL) +=
>> db8500_thermal.o
>>> obj-$(CONFIG_ARMADA_THERMAL) += armada_thermal.o
>>> obj-$(CONFIG_TANGO_THERMAL) += tango_thermal.o
>>> obj-$(CONFIG_IMX_THERMAL) += imx_thermal.o
>>> +obj-$(CONFIG_IMX_SC_THERMAL) += imx_sc_thermal.o
>>> obj-$(CONFIG_MAX77620_THERMAL) += max77620_thermal.o
>>> obj-$(CONFIG_QORIQ_THERMAL) += qoriq_thermal.o
>>> obj-$(CONFIG_DA9062_THERMAL) += da9062-thermal.o
>>> diff --git a/drivers/thermal/imx_sc_thermal.c
>>> b/drivers/thermal/imx_sc_thermal.c
>>> new file mode 100644
>>> index 0000000..d406ecb
>>> --- /dev/null
>>> +++ b/drivers/thermal/imx_sc_thermal.c
>>> @@ -0,0 +1,142 @@
>>> +// SPDX-License-Identifier: GPL-2.0+
>>> +/*
>>> + * Copyright 2018-2019 NXP.
>>
>> *sigh* 2020 now ...
>
> Yes, should be 2018-2020
>
>>
>> [ ... ]
>>
>>> +static int imx_sc_thermal_get_temp(void *data, int *temp) {
>>> + struct imx_sc_msg_misc_get_temp msg;
>>> + struct imx_sc_rpc_msg *hdr = &msg.hdr;
>>> + struct imx_sc_sensor *sensor = data;
>>> + int ret;
>>> +
>>> + msg.data.req.resource_id = sensor->resource_id;
>>> + msg.data.req.type = IMX_SC_C_TEMP;
>>> +
>>> + hdr->ver = IMX_SC_RPC_VERSION;
>>> + hdr->svc = IMX_SC_RPC_SVC_MISC;
>>> + hdr->func = IMX_SC_MISC_FUNC_GET_TEMP;
>>> + hdr->size = 2;
>>
>> Can you explain this 'size' value?
>
> The size means the SCU message size, including the header and the data, its unit
> is word(4 bytes), in thermal get temperature message, the header takes 1 word and
> the data takes another 1, so it is 2, we all pass the size in this way to SCU in i.MX8
> SoCs, the SCU know how long message it will need to receive from AP.

Thanks for the clarification.

>> [ ... ]
>>
>>> +MODULE_DEVICE_TABLE(of, imx_sc_thermal_table);
>>> +
>>> +static struct platform_driver imx_sc_thermal_driver = {
>>> + .probe = imx_sc_thermal_probe,
>>
>> The driver can be compiled as module but there is no 'remove' callback
>
> As there is nothing needs to be done in .remove callback, so I skip it. But
> I think I can add a blank .remove callback to make it more complete.

Ah, right. Everything is handled by "devm_"


>>> + .driver = {
>>> + .name = "imx-sc-thermal",
>>> + .of_match_table = imx_sc_thermal_table,
>>> + },
>>> +};
>>> +module_platform_driver(imx_sc_thermal_driver);
>>> +
>>> +MODULE_AUTHOR("Anson Huang <[email protected]>");
>>> +MODULE_DESCRIPTION("Thermal driver for NXP i.MX SoCs with system
>>> +controller"); MODULE_LICENSE("GPL v2");
>

--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

2020-02-22 07:56:45

by Daniel Lezcano

Subject: Re: [PATCH V15 RESEND 5/5] arm64: dts: imx: add i.MX8QXP thermal support

On 22/02/2020 00:53, Anson Huang wrote:
> Hi, Daniel
>

[ ... ]

>>> +
>>> + thermal_zones: thermal-zones {
>>> + cpu-thermal0 {
>>> + polling-delay-passive = <250>;
>>> + polling-delay = <2000>;
>>> + thermal-sensors = <&tsens IMX_SC_R_SYSTEM>;
>>> + trips {
>>> + cpu_alert0: trip0 {
>>> + temperature = <107000>;
>>> + hysteresis = <2000>;
>>> + type = "passive";
>>> + };
>>
>> May be you can add a 'hot' trip point before 'critical' for future use before
>> reaching the emergency shutdown.
>
> The 'passive' trip is actually the 'hot' trip point you mentioned, and I have combined it to
> below cooling map which will throttle cpu-freq when passive (hot) point is reached.
> We all use 'passive' as 'hot' alarm and trigger cpu-freq throttle on i.MX platforms.

Sorry, I'm not sure to get the point. A 'hot' trip point is not a
'passive' trip point. The 'hot' trip point is a critical temperature and
a notification is raised [1][2].

It is the last chance for the system to do something before the next
trip point 'critical' is reached and where an emergency shutdown is done.

The 'passive' trip point is the target temperature for mitigation in a
normal situation when the system is loaded.

[1]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/thermal/thermal_core.c#n288
[2]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/thermal/thermal_core.c#n420




--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

2020-02-22 12:19:46

by Anson Huang

Subject: RE: [PATCH V15 RESEND 5/5] arm64: dts: imx: add i.MX8QXP thermal support

Hi, Daniel

> Subject: Re: [PATCH V15 RESEND 5/5] arm64: dts: imx: add i.MX8QXP thermal
> support
>
> On 22/02/2020 00:53, Anson Huang wrote:
> > Hi, Daniel
> >
>
> [ ... ]
>
> >>> +
> >>> + thermal_zones: thermal-zones {
> >>> + cpu-thermal0 {
> >>> + polling-delay-passive = <250>;
> >>> + polling-delay = <2000>;
> >>> + thermal-sensors = <&tsens IMX_SC_R_SYSTEM>;
> >>> + trips {
> >>> + cpu_alert0: trip0 {
> >>> + temperature = <107000>;
> >>> + hysteresis = <2000>;
> >>> + type = "passive";
> >>> + };
> >>
> >> May be you can add a 'hot' trip point before 'critical' for future
> >> use before reaching the emergency shutdown.
> >
> > The 'passive' trip is actually the 'hot' trip point you mentioned, and
> > I have combined it to below cooling map which will throttle cpu-freq when
> passive (hot) point is reached.
> > We all use 'passive' as 'hot' alarm and trigger cpu-freq throttle on i.MX
> platforms.
>
> Sorry, I'm not sure to get the point. A 'hot' trip point is not a 'passive' trip
> point. The 'hot' trip point is a critical temperature and a notification is raised
> [1][2].
>
> It is the last chance for the system to do something before the next trip point
> 'critical' is reached and where an emergency shutdown is done.
>
> The 'passive' trip point is the target temperature for mitigation in a normal
> situation when the system is loaded.

Ah, now I understand your point, as all i.MX SoCs ONLY support passive and critical
point, and never support the notification of hot trip point, so I think it should be OK
to leave i.MX8QXP same as other i.MX SoCs, if we plan to support hot trip point notification,
I will cut another patch for all i.MX SoCs, does it make sense to you? If yes, please help
review the V16 patch set I sent out.

thanks again for review,
Anson


>
> [1]
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.ker
> nel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftorvalds%2Flinux.git%2F
> tree%2Fdrivers%2Fthermal%2Fthermal_core.c%23n288&amp;data=02%7C01
> %7Canson.huang%40nxp.com%7C2a1422c3cb0f46c3b13308d7b76c198c%7C
> 686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637179547223360051&a
> mp;sdata=fRmsFbCVFdv3VwFEqp1sSHxt2rucQ33yYnnaPE5gs1g%3D&amp;res
> erved=0
> [2]
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.ker
> nel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftorvalds%2Flinux.git%2F
> tree%2Fdrivers%2Fthermal%2Fthermal_core.c%23n420&amp;data=02%7C01
> %7Canson.huang%40nxp.com%7C2a1422c3cb0f46c3b13308d7b76c198c%7C
> 686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637179547223360051&a
> mp;sdata=QEiwQcL8FZKg6Rs7y9h9XKvFf%2FlVeo20MwKTiqUbMwA%3D&am
> p;reserved=0
>
>
>
>
> --
>
> <https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.
> linaro.org%2F&amp;data=02%7C01%7Canson.huang%40nxp.com%7C2a1422
> c3cb0f46c3b13308d7b76c198c%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C
> 0%7C0%7C637179547223360051&amp;sdata=xT2UtNHHA826aziV1aUmPXpa
> Bnq7dBm%2FJPFG4S2%2F5qQ%3D&amp;reserved=0> Linaro.org │ Open
> source software for ARM SoCs
>
> Follow Linaro:
> <https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.
> facebook.com%2Fpages%2FLinaro&amp;data=02%7C01%7Canson.huang%4
> 0nxp.com%7C2a1422c3cb0f46c3b13308d7b76c198c%7C686ea1d3bc2b4c6fa9
> 2cd99c5c301635%7C0%7C0%7C637179547223360051&amp;sdata=eSr89jm2
> Qxzpi2bX%2BN%2FchizrkF372ypyLgv5%2BuI82Ic%3D&amp;reserved=0>
> Facebook |
> <https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Ftwitte
> r.com%2F%23!%2Flinaroorg&amp;data=02%7C01%7Canson.huang%40nxp.c
> om%7C2a1422c3cb0f46c3b13308d7b76c198c%7C686ea1d3bc2b4c6fa92cd99
> c5c301635%7C0%7C0%7C637179547223360051&amp;sdata=cRydP%2FJpfuD
> uZFXUivk6F4At1lc%2FsOHdSzDDnDtDOug%3D&amp;reserved=0> Twitter |
> <https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.
> linaro.org%2Flinaro-
> blog%2F&amp;data=02%7C01%7Canson.huang%40nxp.com%7C2a1422c3cb0
> f46c3b13308d7b76c198c%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0
> %7C637179547223360051&amp;sdata=wlwlbrLGXt%2Fdpuvw2xpCToiAIweud
> QDvLc8x0kgJOoQ%3D&amp;reserved=0> Blog