This series adds support for CPU temperature monitor modules implemented
on UniPhier LD20 and PXs2 SoCs. This driver supports temperature monitoring
and alert function on the module.
Changes in v4:
- fix warnings from sparse by replacing u32 with __be32
Changes in v3:
- remove TMOD_MASK and use TMOD_WIDTH representing the bit width of TMOD
Changes in v2:
- add nsleep after starting and stopping PVT
- replace temperature calculation with sign_extend32()
Changes in v1:
- separate dts from this patchset as another patchset
- remove 'reg' description on the dt-bindings document
- fix the order of calling initialization functions
- replace mask bits to use GENMASK
- fix calculation of temperature because of not considering a negative value
- use devm_request_threaded_irq() instead of devm_request_irq() and
separate a thread function from the interrupt handler
- add dependency to Kconfig
- set 120C to CRITICAL_TEMP_LIMIT as maximum temperature
- shrink each line of parameters to save the number of lines
- improve some comments and copyright description
Kunihiko Hayashi (2):
dt-bindings: thermal: add binding documentation for UniPhier thermal
monitor
thermal: uniphier: add UniPhier thermal driver
.../bindings/thermal/uniphier-thermal.txt | 64 ++++
drivers/thermal/Kconfig | 8 +
drivers/thermal/Makefile | 1 +
drivers/thermal/uniphier_thermal.c | 386 +++++++++++++++++++++
4 files changed, 459 insertions(+)
create mode 100644 Documentation/devicetree/bindings/thermal/uniphier-thermal.txt
create mode 100644 drivers/thermal/uniphier_thermal.c
--
2.7.4
Add devicetree binding documentation for thermal monitor implemented on
Socionext UniPhier SoCs.
Signed-off-by: Kunihiko Hayashi <[email protected]>
Acked-by: Rob Herring <[email protected]>
---
.../bindings/thermal/uniphier-thermal.txt | 64 ++++++++++++++++++++++
1 file changed, 64 insertions(+)
create mode 100644 Documentation/devicetree/bindings/thermal/uniphier-thermal.txt
diff --git a/Documentation/devicetree/bindings/thermal/uniphier-thermal.txt b/Documentation/devicetree/bindings/thermal/uniphier-thermal.txt
new file mode 100644
index 0000000..686c0b4
--- /dev/null
+++ b/Documentation/devicetree/bindings/thermal/uniphier-thermal.txt
@@ -0,0 +1,64 @@
+* UniPhier Thermal bindings
+
+This describes the devicetree bindings for thermal monitor supported by
+PVT(Process, Voltage and Temperature) monitoring unit implemented on Socionext
+UniPhier SoCs.
+
+Required properties:
+- compatible :
+ - "socionext,uniphier-pxs2-thermal" : For UniPhier PXs2 SoC
+ - "socionext,uniphier-ld20-thermal" : For UniPhier LD20 SoC
+- interrupts : IRQ for the temperature alarm
+- #thermal-sensor-cells : Should be 0. See ./thermal.txt for details.
+
+Optional properties:
+- socionext,tmod-calibration: A pair of calibrated values referred from PVT,
+ in case that the values aren't set on SoC,
+ like a reference board.
+
+Example:
+
+ sysctrl@61840000 {
+ compatible = "socionext,uniphier-ld20-sysctrl",
+ "simple-mfd", "syscon";
+ reg = <0x61840000 0x10000>;
+ ...
+ pvtctl: pvtctl {
+ compatible = "socionext,uniphier-ld20-thermal";
+ interrupts = <0 3 1>;
+ #thermal-sensor-cells = <0>;
+ };
+ ...
+ };
+
+ thermal-zones {
+ cpu_thermal {
+ polling-delay-passive = <250>; /* 250ms */
+ polling-delay = <1000>; /* 1000ms */
+ thermal-sensors = <&pvtctl>;
+
+ trips {
+ cpu_crit: cpu_crit {
+ temperature = <110000>; /* 110C */
+ hysteresis = <2000>;
+ type = "critical";
+ };
+ cpu_alert: cpu_alert {
+ temperature = <100000>; /* 100C */
+ hysteresis = <2000>;
+ type = "passive";
+ };
+ };
+
+ cooling-maps {
+ map0 {
+ trip = <&cpu_alert>;
+ cooling-device = <&cpu0 (-1) (-1)>;
+ };
+ map1 {
+ trip = <&cpu_alert>;
+ cooling-device = <&cpu2 (-1) (-1)>;
+ };
+ };
+ };
+ };
--
2.7.4
Add a thermal driver for on-chip PVT (Process, Voltage and Temperature)
monitoring unit implemented on UniPhier SoCs. This driver supports
temperature monitoring and alert function.
Signed-off-by: Kunihiko Hayashi <[email protected]>
---
drivers/thermal/Kconfig | 8 +
drivers/thermal/Makefile | 1 +
drivers/thermal/uniphier_thermal.c | 386 +++++++++++++++++++++++++++++++++++++
3 files changed, 395 insertions(+)
create mode 100644 drivers/thermal/uniphier_thermal.c
diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
index 776b343..6631826 100644
--- a/drivers/thermal/Kconfig
+++ b/drivers/thermal/Kconfig
@@ -453,4 +453,12 @@ config ZX2967_THERMAL
the primitive temperature sensor embedded in zx2967 SoCs.
This sensor generates the real time die temperature.
+config UNIPHIER_THERMAL
+ tristate "Socionext UniPhier thermal driver"
+ depends on ARCH_UNIPHIER || COMPILE_TEST
+ depends on THERMAL_OF && MFD_SYSCON
+ help
+ Enable this to plug in UniPhier on-chip PVT thermal driver into the
+ thermal framework. The driver supports CPU thermal zone temperature
+ reporting and a couple of trip points.
endif
diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
index 7adae20..05b6e7c 100644
--- a/drivers/thermal/Makefile
+++ b/drivers/thermal/Makefile
@@ -58,3 +58,4 @@ obj-$(CONFIG_HISI_THERMAL) += hisi_thermal.o
obj-$(CONFIG_MTK_THERMAL) += mtk_thermal.o
obj-$(CONFIG_GENERIC_ADC_THERMAL) += thermal-generic-adc.o
obj-$(CONFIG_ZX2967_THERMAL) += zx2967_thermal.o
+obj-$(CONFIG_UNIPHIER_THERMAL) += uniphier_thermal.o
diff --git a/drivers/thermal/uniphier_thermal.c b/drivers/thermal/uniphier_thermal.c
new file mode 100644
index 0000000..1a77c5bf
--- /dev/null
+++ b/drivers/thermal/uniphier_thermal.c
@@ -0,0 +1,386 @@
+/**
+ * uniphier_thermal.c - Socionext UniPhier thermal driver
+ *
+ * Copyright 2014 Panasonic Corporation
+ * Copyright 2016-2017 Socionext Inc.
+ * All rights reserved.
+ *
+ * Author:
+ * Kunihiko Hayashi <[email protected]>
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 of
+ * the License as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/bitops.h>
+#include <linux/interrupt.h>
+#include <linux/mfd/syscon.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/thermal.h>
+
+#include "thermal_core.h"
+
+/*
+ * block registers
+ * addresses are the offset from .block_base
+ */
+#define PVTCTLEN 0x0000
+#define PVTCTLEN_EN BIT(0)
+
+#define PVTCTLMODE 0x0004
+#define PVTCTLMODE_MASK 0xf
+#define PVTCTLMODE_TEMPMON 0x5
+
+#define EMONREPEAT 0x0040
+#define EMONREPEAT_ENDLESS BIT(24)
+#define EMONREPEAT_PERIOD GENMASK(3, 0)
+#define EMONREPEAT_PERIOD_1000000 0x9
+
+/*
+ * common registers
+ * addresses are the offset from .map_base
+ */
+#define PVTCTLSEL 0x0900
+#define PVTCTLSEL_MASK GENMASK(2, 0)
+#define PVTCTLSEL_MONITOR 0
+
+#define SETALERT0 0x0910
+#define SETALERT1 0x0914
+#define SETALERT2 0x0918
+#define SETALERT_TEMP_OVF (GENMASK(7, 0) << 16)
+#define SETALERT_TEMP_OVF_VALUE(val) (((val) & GENMASK(7, 0)) << 16)
+#define SETALERT_EN BIT(0)
+
+#define PMALERTINTCTL 0x0920
+#define PMALERTINTCTL_CLR(ch) BIT(4 * (ch) + 2)
+#define PMALERTINTCTL_SET(ch) BIT(4 * (ch) + 1)
+#define PMALERTINTCTL_EN(ch) BIT(4 * (ch) + 0)
+#define PMALERTINTCTL_MASK (GENMASK(10, 8) | GENMASK(6, 4) | \
+ GENMASK(2, 0))
+
+#define TMOD 0x0928
+#define TMOD_WIDTH 9
+
+#define TMODCOEF 0x0e5c
+
+#define TMODSETUP0_EN BIT(30)
+#define TMODSETUP0_VAL(val) (((val) & GENMASK(13, 0)) << 16)
+#define TMODSETUP1_EN BIT(15)
+#define TMODSETUP1_VAL(val) ((val) & GENMASK(14, 0))
+
+/* SoC critical temperature */
+#define CRITICAL_TEMP_LIMIT (120 * 1000)
+
+/* Max # of alert channels */
+#define ALERT_CH_NUM 3
+
+/* SoC specific thermal sensor data */
+struct uniphier_tm_soc_data {
+ u32 map_base;
+ u32 block_base;
+ u32 tmod_setup_addr;
+};
+
+struct uniphier_tm_dev {
+ struct regmap *regmap;
+ bool alert_en[ALERT_CH_NUM];
+ u32 tmod_calib0;
+ u32 tmod_calib1;
+ struct thermal_zone_device *tz_dev;
+ const struct uniphier_tm_soc_data *data;
+};
+
+static int uniphier_tm_initialize_sensor(struct uniphier_tm_dev *tdev)
+{
+ struct regmap *map = tdev->regmap;
+ u32 val;
+ int ret;
+
+ /* stop PVT */
+ regmap_write_bits(map, tdev->data->block_base + PVTCTLEN,
+ PVTCTLEN_EN, 0);
+
+ /*
+ * set default value if missing calibrated value
+ *
+ * Since SoC has a calibrated value that was set in advance,
+ * TMODCOEF shows non-zero and PVT refers the value internally.
+ *
+ * However, some boards don't have the calibrated value.
+ * In that case, TMODCOEF shows zero and the driver has to set
+ * default value manually.
+ */
+ ret = regmap_read(map, tdev->data->map_base + TMODCOEF, &val);
+ if (ret)
+ return ret;
+ if (!val)
+ regmap_write(map, tdev->data->tmod_setup_addr,
+ TMODSETUP0_EN | TMODSETUP0_VAL(tdev->tmod_calib0) |
+ TMODSETUP1_EN | TMODSETUP1_VAL(tdev->tmod_calib1));
+
+ /* select temperature mode */
+ regmap_write_bits(map, tdev->data->block_base + PVTCTLMODE,
+ PVTCTLMODE_MASK, PVTCTLMODE_TEMPMON);
+
+ /* set monitoring period */
+ regmap_write_bits(map, tdev->data->block_base + EMONREPEAT,
+ EMONREPEAT_ENDLESS | EMONREPEAT_PERIOD,
+ EMONREPEAT_ENDLESS | EMONREPEAT_PERIOD_1000000);
+
+ /* set monitor mode */
+ regmap_write_bits(map, tdev->data->map_base + PVTCTLSEL,
+ PVTCTLSEL_MASK, PVTCTLSEL_MONITOR);
+
+ return 0;
+}
+
+static void uniphier_tm_set_alert(struct uniphier_tm_dev *tdev, u32 ch,
+ u32 temp)
+{
+ struct regmap *map = tdev->regmap;
+
+ /* set alert temperature */
+ regmap_write_bits(map, tdev->data->map_base + SETALERT0 + (ch << 2),
+ SETALERT_EN | SETALERT_TEMP_OVF,
+ SETALERT_EN |
+ SETALERT_TEMP_OVF_VALUE(temp / 1000));
+}
+
+static void uniphier_tm_enable_sensor(struct uniphier_tm_dev *tdev)
+{
+ struct regmap *map = tdev->regmap;
+ int i;
+ u32 bits = 0;
+
+ for (i = 0; i < ALERT_CH_NUM; i++)
+ if (tdev->alert_en[i])
+ bits |= PMALERTINTCTL_EN(i);
+
+ /* enable alert interrupt */
+ regmap_write_bits(map, tdev->data->map_base + PMALERTINTCTL,
+ PMALERTINTCTL_MASK, bits);
+
+ /* start PVT */
+ regmap_write_bits(map, tdev->data->block_base + PVTCTLEN,
+ PVTCTLEN_EN, PVTCTLEN_EN);
+
+ usleep_range(700, 1500); /* The spec note says at least 700us */
+}
+
+static void uniphier_tm_disable_sensor(struct uniphier_tm_dev *tdev)
+{
+ struct regmap *map = tdev->regmap;
+
+ /* disable alert interrupt */
+ regmap_write_bits(map, tdev->data->map_base + PMALERTINTCTL,
+ PMALERTINTCTL_MASK, 0);
+
+ /* stop PVT */
+ regmap_write_bits(map, tdev->data->block_base + PVTCTLEN,
+ PVTCTLEN_EN, 0);
+
+ usleep_range(1000, 2000); /* The spec note says at least 1ms */
+}
+
+static int uniphier_tm_get_temp(void *data, int *out_temp)
+{
+ struct uniphier_tm_dev *tdev = data;
+ struct regmap *map = tdev->regmap;
+ int ret;
+ u32 temp;
+
+ ret = regmap_read(map, tdev->data->map_base + TMOD, &temp);
+ if (ret)
+ return ret;
+
+ /* MSB of the TMOD field is a sign bit */
+ *out_temp = sign_extend32(temp, TMOD_WIDTH - 1) * 1000;
+
+ return 0;
+}
+
+static const struct thermal_zone_of_device_ops uniphier_of_thermal_ops = {
+ .get_temp = uniphier_tm_get_temp,
+};
+
+static void uniphier_tm_irq_clear(struct uniphier_tm_dev *tdev)
+{
+ u32 mask = 0, bits = 0;
+ int i;
+
+ for (i = 0; i < ALERT_CH_NUM; i++) {
+ mask |= (PMALERTINTCTL_CLR(i) | PMALERTINTCTL_SET(i));
+ bits |= PMALERTINTCTL_CLR(i);
+ }
+
+ /* clear alert interrupt */
+ regmap_write_bits(tdev->regmap,
+ tdev->data->map_base + PMALERTINTCTL, mask, bits);
+}
+
+static irqreturn_t uniphier_tm_alarm_irq(int irq, void *_tdev)
+{
+ struct uniphier_tm_dev *tdev = _tdev;
+
+ disable_irq_nosync(irq);
+ uniphier_tm_irq_clear(tdev);
+
+ return IRQ_WAKE_THREAD;
+}
+
+static irqreturn_t uniphier_tm_alarm_irq_thread(int irq, void *_tdev)
+{
+ struct uniphier_tm_dev *tdev = _tdev;
+
+ thermal_zone_device_update(tdev->tz_dev, THERMAL_EVENT_UNSPECIFIED);
+
+ return IRQ_HANDLED;
+}
+
+static int uniphier_tm_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct regmap *regmap;
+ struct device_node *parent;
+ struct uniphier_tm_dev *tdev;
+ const struct thermal_trip *trips;
+ const __be32 *calib;
+ int i, ret, irq, ntrips, crit_temp = INT_MAX;
+
+ tdev = devm_kzalloc(dev, sizeof(*tdev), GFP_KERNEL);
+ if (!tdev)
+ return -ENOMEM;
+
+ tdev->data = of_device_get_match_data(dev);
+ if (WARN_ON(!tdev->data))
+ return -EINVAL;
+
+ irq = platform_get_irq(pdev, 0);
+ if (irq < 0)
+ return irq;
+
+ /* get tmod-calibration values */
+ calib = of_get_property(dev->of_node, "socionext,tmod-calibration",
+ NULL);
+ if (calib) {
+ tdev->tmod_calib0 = of_read_number(calib, 1);
+ tdev->tmod_calib1 = of_read_number(calib + 1, 1);
+ }
+
+ /* get regmap from syscon node */
+ parent = of_get_parent(dev->of_node); /* parent should be syscon node */
+ regmap = syscon_node_to_regmap(parent);
+ of_node_put(parent);
+ if (IS_ERR(regmap)) {
+ dev_err(dev, "failed to get regmap (error %ld)\n",
+ PTR_ERR(regmap));
+ return PTR_ERR(regmap);
+ }
+ tdev->regmap = regmap;
+
+ ret = uniphier_tm_initialize_sensor(tdev);
+ if (ret) {
+ dev_err(dev, "failed to initialize sensor\n");
+ return ret;
+ }
+
+ ret = devm_request_threaded_irq(dev, irq, uniphier_tm_alarm_irq,
+ uniphier_tm_alarm_irq_thread,
+ 0, "thermal", tdev);
+ if (ret)
+ return ret;
+
+ platform_set_drvdata(pdev, tdev);
+
+ tdev->tz_dev = devm_thermal_zone_of_sensor_register(dev, 0, tdev,
+ &uniphier_of_thermal_ops);
+ if (IS_ERR(tdev->tz_dev)) {
+ dev_err(dev, "failed to register sensor device\n");
+ return PTR_ERR(tdev->tz_dev);
+ }
+
+ /* get trip points */
+ trips = of_thermal_get_trip_points(tdev->tz_dev);
+ ntrips = of_thermal_get_ntrips(tdev->tz_dev);
+ if (ntrips > ALERT_CH_NUM) {
+ dev_err(dev, "thermal zone has too many trips\n");
+ return -E2BIG;
+ }
+
+ /* set alert temperatures */
+ for (i = 0; i < ntrips; i++) {
+ if (trips[i].type == THERMAL_TRIP_CRITICAL &&
+ trips[i].temperature < crit_temp)
+ crit_temp = trips[i].temperature;
+ uniphier_tm_set_alert(tdev, i, trips[i].temperature);
+ tdev->alert_en[i] = true;
+ }
+ if (crit_temp > CRITICAL_TEMP_LIMIT) {
+ dev_err(dev, "critical trip is over limit(>%d), or not set\n",
+ CRITICAL_TEMP_LIMIT);
+ return -EINVAL;
+ }
+
+ uniphier_tm_enable_sensor(tdev);
+
+ return 0;
+}
+
+static int uniphier_tm_remove(struct platform_device *pdev)
+{
+ struct uniphier_tm_dev *tdev = platform_get_drvdata(pdev);
+
+ /* disable sensor */
+ uniphier_tm_disable_sensor(tdev);
+
+ return 0;
+}
+
+static const struct uniphier_tm_soc_data uniphier_pxs2_tm_data = {
+ .map_base = 0xe000,
+ .block_base = 0xe000,
+ .tmod_setup_addr = 0xe904,
+};
+
+static const struct uniphier_tm_soc_data uniphier_ld20_tm_data = {
+ .map_base = 0xe000,
+ .block_base = 0xe800,
+ .tmod_setup_addr = 0xe938,
+};
+
+static const struct of_device_id uniphier_tm_dt_ids[] = {
+ {
+ .compatible = "socionext,uniphier-pxs2-thermal",
+ .data = &uniphier_pxs2_tm_data,
+ },
+ {
+ .compatible = "socionext,uniphier-ld20-thermal",
+ .data = &uniphier_ld20_tm_data,
+ },
+ { /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, uniphier_tm_dt_ids);
+
+static struct platform_driver uniphier_tm_driver = {
+ .probe = uniphier_tm_probe,
+ .remove = uniphier_tm_remove,
+ .driver = {
+ .name = "uniphier-thermal",
+ .of_match_table = uniphier_tm_dt_ids,
+ },
+};
+module_platform_driver(uniphier_tm_driver);
+
+MODULE_AUTHOR("Kunihiko Hayashi <[email protected]>");
+MODULE_DESCRIPTION("UniPhier thermal driver");
+MODULE_LICENSE("GPL v2");
--
2.7.4
2017-07-21 20:21 GMT+09:00 Kunihiko Hayashi <[email protected]>:
> +static int uniphier_tm_initialize_sensor(struct uniphier_tm_dev *tdev)
> +{
> + struct regmap *map = tdev->regmap;
> + u32 val;
> + int ret;
> +
> + /* stop PVT */
> + regmap_write_bits(map, tdev->data->block_base + PVTCTLEN,
> + PVTCTLEN_EN, 0);
> +
> + /*
> + * set default value if missing calibrated value
> + *
> + * Since SoC has a calibrated value that was set in advance,
> + * TMODCOEF shows non-zero and PVT refers the value internally.
> + *
> + * However, some boards don't have the calibrated value.
> + * In that case, TMODCOEF shows zero and the driver has to set
> + * default value manually.
> + */
> + ret = regmap_read(map, tdev->data->map_base + TMODCOEF, &val);
> + if (ret)
> + return ret;
> + if (!val)
> + regmap_write(map, tdev->data->tmod_setup_addr,
> + TMODSETUP0_EN | TMODSETUP0_VAL(tdev->tmod_calib0) |
> + TMODSETUP1_EN | TMODSETUP1_VAL(tdev->tmod_calib1));
This code is strange.
What if TMODCOEF has no calibrated value and
"socionext,tmod-calibration" is not set either?
->tmod_setupaddr will be set to zero
and the sensor would not work, right?
> + /* get tmod-calibration values */
> + calib = of_get_property(dev->of_node, "socionext,tmod-calibration",
> + NULL);
> + if (calib) {
> + tdev->tmod_calib0 = of_read_number(calib, 1);
> + tdev->tmod_calib1 = of_read_number(calib + 1, 1);
> + }
>From your DT change (https://patchwork.kernel.org/patch/9826391/),
this property seems a pair of u32 values, like follows:
socionext,tmod-calibration = <0x0f22 0x68ee>;
Why do you need to use of_read_number() to retrieve each u32 value?
See the comment and return value (u64):
------------------>8-----------------
/* Helper to read a big number; size is in cells (not bytes) */
static inline u64 of_read_number(const __be32 *cell, int size)
------------------8<-----------------
Also, you are not checking the length of the property.
of_read_number() may over-run the property.
of_property_read_u32_array() will be a better choice.
I'd propose the code like follows:
diff --git a/drivers/thermal/uniphier_thermal.c
b/drivers/thermal/uniphier_thermal.c
index 1a77c5bf6930..eea8a3053584 100644
--- a/drivers/thermal/uniphier_thermal.c
+++ b/drivers/thermal/uniphier_thermal.c
@@ -94,16 +94,16 @@ struct uniphier_tm_soc_data {
struct uniphier_tm_dev {
struct regmap *regmap;
bool alert_en[ALERT_CH_NUM];
- u32 tmod_calib0;
- u32 tmod_calib1;
struct thermal_zone_device *tz_dev;
const struct uniphier_tm_soc_data *data;
};
-static int uniphier_tm_initialize_sensor(struct uniphier_tm_dev *tdev)
+static int uniphier_tm_initialize_sensor(struct uniphier_tm_dev *tdev,
+ struct device *dev)
{
struct regmap *map = tdev->regmap;
u32 val;
+ u32 tmod_val[2];
int ret;
/* stop PVT */
@@ -123,10 +123,22 @@ static int uniphier_tm_initialize_sensor(struct
uniphier_tm_dev *tdev)
ret = regmap_read(map, tdev->data->map_base + TMODCOEF, &val);
if (ret)
return ret;
- if (!val)
+ if (!val) {
+ /*
+ * No preset value found in the register.
+ * Look for the fall-back values in DT.
+ */
+ ret = of_property_read_u32_array(dev->of_node,
+ "socionext,tmod-calibration",
+ tmod_val,
ARRAY_SIZE(tmod_val));
+ if (ret) {
+ dev_err(dev, "neither register nor DT has
calibrated values.\n");
+ return ret;
+ }
regmap_write(map, tdev->data->tmod_setup_addr,
- TMODSETUP0_EN | TMODSETUP0_VAL(tdev->tmod_calib0) |
- TMODSETUP1_EN | TMODSETUP1_VAL(tdev->tmod_calib1));
+ TMODSETUP0_EN | TMODSETUP0_VAL(tmod_val[0]) |
+ TMODSETUP1_EN | TMODSETUP1_VAL(tmod_val[1]));
+ }
/* select temperature mode */
regmap_write_bits(map, tdev->data->block_base + PVTCTLMODE,
@@ -254,7 +266,6 @@ static int uniphier_tm_probe(struct platform_device *pdev)
struct device_node *parent;
struct uniphier_tm_dev *tdev;
const struct thermal_trip *trips;
- const __be32 *calib;
int i, ret, irq, ntrips, crit_temp = INT_MAX;
tdev = devm_kzalloc(dev, sizeof(*tdev), GFP_KERNEL);
@@ -269,14 +280,6 @@ static int uniphier_tm_probe(struct platform_device *pdev)
if (irq < 0)
return irq;
- /* get tmod-calibration values */
- calib = of_get_property(dev->of_node, "socionext,tmod-calibration",
- NULL);
- if (calib) {
- tdev->tmod_calib0 = of_read_number(calib, 1);
- tdev->tmod_calib1 = of_read_number(calib + 1, 1);
- }
-
/* get regmap from syscon node */
parent = of_get_parent(dev->of_node); /* parent should be syscon node */
regmap = syscon_node_to_regmap(parent);
@@ -288,7 +291,7 @@ static int uniphier_tm_probe(struct platform_device *pdev)
}
tdev->regmap = regmap;
- ret = uniphier_tm_initialize_sensor(tdev);
+ ret = uniphier_tm_initialize_sensor(tdev, dev);
if (ret) {
dev_err(dev, "failed to initialize sensor\n");
return ret;
--
Best Regards
Masahiro Yamada
2017-07-21 20:21 GMT+09:00 Kunihiko Hayashi <[email protected]>:
> +static int uniphier_tm_initialize_sensor(struct uniphier_tm_dev *tdev)
> +{
> + struct regmap *map = tdev->regmap;
> + u32 val;
> + int ret;
> +
> + /* stop PVT */
> + regmap_write_bits(map, tdev->data->block_base + PVTCTLEN,
> + PVTCTLEN_EN, 0);
> +
> + /*
> + * set default value if missing calibrated value
> + *
> + * Since SoC has a calibrated value that was set in advance,
> + * TMODCOEF shows non-zero and PVT refers the value internally.
> + *
> + * However, some boards don't have the calibrated value.
> + * In that case, TMODCOEF shows zero and the driver has to set
> + * default value manually.
> + */
> + ret = regmap_read(map, tdev->data->map_base + TMODCOEF, &val);
> + if (ret)
> + return ret;
> + if (!val)
> + regmap_write(map, tdev->data->tmod_setup_addr,
> + TMODSETUP0_EN | TMODSETUP0_VAL(tdev->tmod_calib0) |
> + TMODSETUP1_EN | TMODSETUP1_VAL(tdev->tmod_calib1));
This code is strange.
What if TMODCOEF has no calibrated value and
"socionext,tmod-calibration" is not set either?
->tmod_setupaddr will be set to zero
and the sensor would not work, right?
> + /* get tmod-calibration values */
> + calib = of_get_property(dev->of_node, "socionext,tmod-calibration",
> + NULL);
> + if (calib) {
> + tdev->tmod_calib0 = of_read_number(calib, 1);
> + tdev->tmod_calib1 = of_read_number(calib + 1, 1);
> + }
>From your DT change (https://patchwork.kernel.org/patch/9826391/),
this property seems a pair of u32 values, like follows:
socionext,tmod-calibration = <0x0f22 0x68ee>;
Why do you need to use of_read_number() to retrieve each u32 value?
See the comment and return value (u64):
------------------>8-----------------
/* Helper to read a big number; size is in cells (not bytes) */
static inline u64 of_read_number(const __be32 *cell, int size)
------------------8<-----------------
Also, you are not checking the length of the property.
of_read_number() may over-run the property.
of_property_read_u32_array() will be a better choice.
I'd propose the code like follows:
diff --git a/drivers/thermal/uniphier_thermal.c
b/drivers/thermal/uniphier_thermal.c
index 1a77c5bf6930..eea8a3053584 100644
--- a/drivers/thermal/uniphier_thermal.c
+++ b/drivers/thermal/uniphier_thermal.c
@@ -94,16 +94,16 @@ struct uniphier_tm_soc_data {
struct uniphier_tm_dev {
struct regmap *regmap;
bool alert_en[ALERT_CH_NUM];
- u32 tmod_calib0;
- u32 tmod_calib1;
struct thermal_zone_device *tz_dev;
const struct uniphier_tm_soc_data *data;
};
-static int uniphier_tm_initialize_sensor(struct uniphier_tm_dev *tdev)
+static int uniphier_tm_initialize_sensor(struct uniphier_tm_dev *tdev,
+ struct device *dev)
{
struct regmap *map = tdev->regmap;
u32 val;
+ u32 tmod_val[2];
int ret;
/* stop PVT */
@@ -123,10 +123,22 @@ static int uniphier_tm_initialize_sensor(struct
uniphier_tm_dev *tdev)
ret = regmap_read(map, tdev->data->map_base + TMODCOEF, &val);
if (ret)
return ret;
- if (!val)
+ if (!val) {
+ /*
+ * No preset value found in the register.
+ * Look for the fall-back values in DT.
+ */
+ ret = of_property_read_u32_array(dev->of_node,
+ "socionext,tmod-calibration",
+ tmod_val,
ARRAY_SIZE(tmod_val));
+ if (ret) {
+ dev_err(dev, "neither register nor DT has
calibrated values.\n");
+ return ret;
+ }
regmap_write(map, tdev->data->tmod_setup_addr,
- TMODSETUP0_EN | TMODSETUP0_VAL(tdev->tmod_calib0) |
- TMODSETUP1_EN | TMODSETUP1_VAL(tdev->tmod_calib1));
+ TMODSETUP0_EN | TMODSETUP0_VAL(tmod_val[0]) |
+ TMODSETUP1_EN | TMODSETUP1_VAL(tmod_val[1]));
+ }
/* select temperature mode */
regmap_write_bits(map, tdev->data->block_base + PVTCTLMODE,
@@ -254,7 +266,6 @@ static int uniphier_tm_probe(struct platform_device *pdev)
struct device_node *parent;
struct uniphier_tm_dev *tdev;
const struct thermal_trip *trips;
- const __be32 *calib;
int i, ret, irq, ntrips, crit_temp = INT_MAX;
tdev = devm_kzalloc(dev, sizeof(*tdev), GFP_KERNEL);
@@ -269,14 +280,6 @@ static int uniphier_tm_probe(struct platform_device *pdev)
if (irq < 0)
return irq;
- /* get tmod-calibration values */
- calib = of_get_property(dev->of_node, "socionext,tmod-calibration",
- NULL);
- if (calib) {
- tdev->tmod_calib0 = of_read_number(calib, 1);
- tdev->tmod_calib1 = of_read_number(calib + 1, 1);
- }
-
/* get regmap from syscon node */
parent = of_get_parent(dev->of_node); /* parent should be syscon node */
regmap = syscon_node_to_regmap(parent);
@@ -288,7 +291,7 @@ static int uniphier_tm_probe(struct platform_device *pdev)
}
tdev->regmap = regmap;
- ret = uniphier_tm_initialize_sensor(tdev);
+ ret = uniphier_tm_initialize_sensor(tdev, dev);
if (ret) {
dev_err(dev, "failed to initialize sensor\n");
return ret;
--
Best Regards
Masahiro Yamada
2017-07-21 20:21 GMT+09:00 Kunihiko Hayashi <[email protected]>:
> This series adds support for CPU temperature monitor modules implemented
> on UniPhier LD20 and PXs2 SoCs. This driver supports temperature monitoring
> and alert function on the module.
>
> Changes in v4:
> - fix warnings from sparse by replacing u32 with __be32
Nit.
Your subject prefix indicates this patch is v5.
Do you mean "Changes in v5" or "Changes since v4" ?
> Changes in v3:
> - remove TMOD_MASK and use TMOD_WIDTH representing the bit width of TMOD
>
> Changes in v2:
> - add nsleep after starting and stopping PVT
> - replace temperature calculation with sign_extend32()
>
> Changes in v1:
> - separate dts from this patchset as another patchset
> - remove 'reg' description on the dt-bindings document
> - fix the order of calling initialization functions
> - replace mask bits to use GENMASK
> - fix calculation of temperature because of not considering a negative value
> - use devm_request_threaded_irq() instead of devm_request_irq() and
> separate a thread function from the interrupt handler
> - add dependency to Kconfig
> - set 120C to CRITICAL_TEMP_LIMIT as maximum temperature
> - shrink each line of parameters to save the number of lines
> - improve some comments and copyright description
>
> Kunihiko Hayashi (2):
> dt-bindings: thermal: add binding documentation for UniPhier thermal
> monitor
> thermal: uniphier: add UniPhier thermal driver
>
> .../bindings/thermal/uniphier-thermal.txt | 64 ++++
> drivers/thermal/Kconfig | 8 +
> drivers/thermal/Makefile | 1 +
> drivers/thermal/uniphier_thermal.c | 386 +++++++++++++++++++++
> 4 files changed, 459 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/thermal/uniphier-thermal.txt
> create mode 100644 drivers/thermal/uniphier_thermal.c
>
> --
> 2.7.4
>
--
Best Regards
Masahiro Yamada
Hello,
2017-07-21 22:24 GMT+09:00 Masahiro Yamada <[email protected]>:
> 2017-07-21 20:21 GMT+09:00 Kunihiko Hayashi <[email protected]>:
>> This series adds support for CPU temperature monitor modules implemented
>> on UniPhier LD20 and PXs2 SoCs. This driver supports temperature monitoring
>> and alert function on the module.
>>
>> Changes in v4:
>> - fix warnings from sparse by replacing u32 with __be32
>
> Nit.
>
> Your subject prefix indicates this patch is v5.
>
> Do you mean "Changes in v5" or "Changes since v4" ?
I've checked all previous series and it seems to mean "Changes from vN".
Thanks,
>
>
>
>
>
>
>> Changes in v3:
>> - remove TMOD_MASK and use TMOD_WIDTH representing the bit width of TMOD
>>
>> Changes in v2:
>> - add nsleep after starting and stopping PVT
>> - replace temperature calculation with sign_extend32()
>>
>> Changes in v1:
>> - separate dts from this patchset as another patchset
>> - remove 'reg' description on the dt-bindings document
>> - fix the order of calling initialization functions
>> - replace mask bits to use GENMASK
>> - fix calculation of temperature because of not considering a negative value
>> - use devm_request_threaded_irq() instead of devm_request_irq() and
>> separate a thread function from the interrupt handler
>> - add dependency to Kconfig
>> - set 120C to CRITICAL_TEMP_LIMIT as maximum temperature
>> - shrink each line of parameters to save the number of lines
>> - improve some comments and copyright description
>>
>> Kunihiko Hayashi (2):
>> dt-bindings: thermal: add binding documentation for UniPhier thermal
>> monitor
>> thermal: uniphier: add UniPhier thermal driver
>>
>> .../bindings/thermal/uniphier-thermal.txt | 64 ++++
>> drivers/thermal/Kconfig | 8 +
>> drivers/thermal/Makefile | 1 +
>> drivers/thermal/uniphier_thermal.c | 386 +++++++++++++++++++++
>> 4 files changed, 459 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/thermal/uniphier-thermal.txt
>> create mode 100644 drivers/thermal/uniphier_thermal.c
>>
>> --
>> 2.7.4
>>
>
>
>
> --
> Best Regards
> Masahiro Yamada
--
Masami Hiramatsu
Hello,
On Fri, 21 Jul 2017 22:34:49 +0900 <[email protected]> wrote:
> Hello,
>
> 2017-07-21 22:24 GMT+09:00 Masahiro Yamada <[email protected]>:
> > 2017-07-21 20:21 GMT+09:00 Kunihiko Hayashi <[email protected]>:
> >> This series adds support for CPU temperature monitor modules implemented
> >> on UniPhier LD20 and PXs2 SoCs. This driver supports temperature monitoring
> >> and alert function on the module.
> >>
> >> Changes in v4:
> >> - fix warnings from sparse by replacing u32 with __be32
> >
> > Nit.
> >
> > Your subject prefix indicates this patch is v5.
> >
> > Do you mean "Changes in v5" or "Changes since v4" ?
>
> I've checked all previous series and it seems to mean "Changes from vN".
Surely my description is misleading.
I'll fix it next.
> Thanks,
>
> >
> >
> >
> >
> >
> >
> >> Changes in v3:
> >> - remove TMOD_MASK and use TMOD_WIDTH representing the bit width of TMOD
> >>
> >> Changes in v2:
> >> - add nsleep after starting and stopping PVT
> >> - replace temperature calculation with sign_extend32()
> >>
> >> Changes in v1:
> >> - separate dts from this patchset as another patchset
> >> - remove 'reg' description on the dt-bindings document
> >> - fix the order of calling initialization functions
> >> - replace mask bits to use GENMASK
> >> - fix calculation of temperature because of not considering a negative value
> >> - use devm_request_threaded_irq() instead of devm_request_irq() and
> >> separate a thread function from the interrupt handler
> >> - add dependency to Kconfig
> >> - set 120C to CRITICAL_TEMP_LIMIT as maximum temperature
> >> - shrink each line of parameters to save the number of lines
> >> - improve some comments and copyright description
> >>
> >> Kunihiko Hayashi (2):
> >> dt-bindings: thermal: add binding documentation for UniPhier thermal
> >> monitor
> >> thermal: uniphier: add UniPhier thermal driver
> >>
> >> .../bindings/thermal/uniphier-thermal.txt | 64 ++++
> >> drivers/thermal/Kconfig | 8 +
> >> drivers/thermal/Makefile | 1 +
> >> drivers/thermal/uniphier_thermal.c | 386 +++++++++++++++++++++
> >> 4 files changed, 459 insertions(+)
> >> create mode 100644 Documentation/devicetree/bindings/thermal/uniphier-thermal.txt
> >> create mode 100644 drivers/thermal/uniphier_thermal.c
> >>
> >> --
> >> 2.7.4
> >>
> >
> >
> >
> > --
> > Best Regards
> > Masahiro Yamada
>
>
>
> --
> Masami Hiramatsu
--
Best Regards,
Kunihiko Hayashi
On Fri, 21 Jul 2017 22:21:05 +0900 <[email protected]> wrote:
> 2017-07-21 20:21 GMT+09:00 Kunihiko Hayashi <[email protected]>:
>
> > +static int uniphier_tm_initialize_sensor(struct uniphier_tm_dev *tdev)
> > +{
> > + struct regmap *map = tdev->regmap;
> > + u32 val;
> > + int ret;
> > +
> > + /* stop PVT */
> > + regmap_write_bits(map, tdev->data->block_base + PVTCTLEN,
> > + PVTCTLEN_EN, 0);
> > +
> > + /*
> > + * set default value if missing calibrated value
> > + *
> > + * Since SoC has a calibrated value that was set in advance,
> > + * TMODCOEF shows non-zero and PVT refers the value internally.
> > + *
> > + * However, some boards don't have the calibrated value.
> > + * In that case, TMODCOEF shows zero and the driver has to set
> > + * default value manually.
> > + */
> > + ret = regmap_read(map, tdev->data->map_base + TMODCOEF, &val);
> > + if (ret)
> > + return ret;
> > + if (!val)
> > + regmap_write(map, tdev->data->tmod_setup_addr,
> > + TMODSETUP0_EN | TMODSETUP0_VAL(tdev->tmod_calib0) |
> > + TMODSETUP1_EN | TMODSETUP1_VAL(tdev->tmod_calib1));
>
>
> This code is strange.
>
> What if TMODCOEF has no calibrated value and
> "socionext,tmod-calibration" is not set either?
>
>
> ->tmod_setupaddr will be set to zero
> and the sensor would not work, right?
Yes. When both are true, the sensor would not work.
> > + /* get tmod-calibration values */
> > + calib = of_get_property(dev->of_node, "socionext,tmod-calibration",
> > + NULL);
> > + if (calib) {
> > + tdev->tmod_calib0 = of_read_number(calib, 1);
> > + tdev->tmod_calib1 = of_read_number(calib + 1, 1);
> > + }
>
>
> From your DT change (https://patchwork.kernel.org/patch/9826391/),
> this property seems a pair of u32 values, like follows:
>
> socionext,tmod-calibration = <0x0f22 0x68ee>;
>
>
> Why do you need to use of_read_number() to retrieve each u32 value?
>
> See the comment and return value (u64):
>
> ------------------>8-----------------
> /* Helper to read a big number; size is in cells (not bytes) */
> static inline u64 of_read_number(const __be32 *cell, int size)
> ------------------8<-----------------
>
>
> Also, you are not checking the length of the property.
> of_read_number() may over-run the property.
>
>
>
> of_property_read_u32_array() will be a better choice.
>
>
> I'd propose the code like follows:
Thank you for your examples.
Surely the driver needs to get the value from the register,
and when failed, tries to get the value from DT, and at last
should issue an error when failed.
I'll try to apply your recommended code.
>
> diff --git a/drivers/thermal/uniphier_thermal.c
> b/drivers/thermal/uniphier_thermal.c
> index 1a77c5bf6930..eea8a3053584 100644
> --- a/drivers/thermal/uniphier_thermal.c
> +++ b/drivers/thermal/uniphier_thermal.c
> @@ -94,16 +94,16 @@ struct uniphier_tm_soc_data {
> struct uniphier_tm_dev {
> struct regmap *regmap;
> bool alert_en[ALERT_CH_NUM];
> - u32 tmod_calib0;
> - u32 tmod_calib1;
> struct thermal_zone_device *tz_dev;
> const struct uniphier_tm_soc_data *data;
> };
>
> -static int uniphier_tm_initialize_sensor(struct uniphier_tm_dev *tdev)
> +static int uniphier_tm_initialize_sensor(struct uniphier_tm_dev *tdev,
> + struct device *dev)
> {
> struct regmap *map = tdev->regmap;
> u32 val;
> + u32 tmod_val[2];
> int ret;
>
> /* stop PVT */
> @@ -123,10 +123,22 @@ static int uniphier_tm_initialize_sensor(struct
> uniphier_tm_dev *tdev)
> ret = regmap_read(map, tdev->data->map_base + TMODCOEF, &val);
> if (ret)
> return ret;
> - if (!val)
> + if (!val) {
> + /*
> + * No preset value found in the register.
> + * Look for the fall-back values in DT.
> + */
> + ret = of_property_read_u32_array(dev->of_node,
> + "socionext,tmod-calibration",
> + tmod_val,
> ARRAY_SIZE(tmod_val));
> + if (ret) {
> + dev_err(dev, "neither register nor DT has
> calibrated values.\n");
> + return ret;
> + }
> regmap_write(map, tdev->data->tmod_setup_addr,
> - TMODSETUP0_EN | TMODSETUP0_VAL(tdev->tmod_calib0) |
> - TMODSETUP1_EN | TMODSETUP1_VAL(tdev->tmod_calib1));
> + TMODSETUP0_EN | TMODSETUP0_VAL(tmod_val[0]) |
> + TMODSETUP1_EN | TMODSETUP1_VAL(tmod_val[1]));
> + }
>
> /* select temperature mode */
> regmap_write_bits(map, tdev->data->block_base + PVTCTLMODE,
> @@ -254,7 +266,6 @@ static int uniphier_tm_probe(struct platform_device *pdev)
> struct device_node *parent;
> struct uniphier_tm_dev *tdev;
> const struct thermal_trip *trips;
> - const __be32 *calib;
> int i, ret, irq, ntrips, crit_temp = INT_MAX;
>
> tdev = devm_kzalloc(dev, sizeof(*tdev), GFP_KERNEL);
> @@ -269,14 +280,6 @@ static int uniphier_tm_probe(struct platform_device *pdev)
> if (irq < 0)
> return irq;
>
> - /* get tmod-calibration values */
> - calib = of_get_property(dev->of_node, "socionext,tmod-calibration",
> - NULL);
> - if (calib) {
> - tdev->tmod_calib0 = of_read_number(calib, 1);
> - tdev->tmod_calib1 = of_read_number(calib + 1, 1);
> - }
> -
> /* get regmap from syscon node */
> parent = of_get_parent(dev->of_node); /* parent should be syscon node */
> regmap = syscon_node_to_regmap(parent);
> @@ -288,7 +291,7 @@ static int uniphier_tm_probe(struct platform_device *pdev)
> }
> tdev->regmap = regmap;
>
> - ret = uniphier_tm_initialize_sensor(tdev);
> + ret = uniphier_tm_initialize_sensor(tdev, dev);
> if (ret) {
> dev_err(dev, "failed to initialize sensor\n");
> return ret;
>
>
>
>
> --
> Best Regards
> Masahiro Yamada
--
Best Regards,
Kunihiko Hayashi