* [PATCH v4 0/6] IIO-based thermal sensor driver for Allwinner H3 SoC
@ 2017-09-14 14:52 Icenowy Zheng
2017-09-14 14:52 ` [PATCH v4 1/6] dt-bindings: update the Allwinner GPADC device tree binding for H3 Icenowy Zheng
` (5 more replies)
0 siblings, 6 replies; 28+ messages in thread
From: Icenowy Zheng @ 2017-09-14 14:52 UTC (permalink / raw)
To: Lee Jones, Rob Herring, Maxime Ripard, Chen-Yu Tsai,
Jonathan Cameron, Quentin Schulz
Cc: devicetree, linux-arm-kernel, linux-kernel, linux-iio,
linux-sunxi, Icenowy Zheng
Allwiner H3 SoC has a thermal sensor, which is a large refactored version of
the old Allwinner "GPADC" (although it have already only thermal part left
in A33).
This patch tried to add support for the sensor in H3 based on the A33 thermal
sensor driver by Quentin Schulz, which is already merged.
Icenowy Zheng (6):
dt-bindings: update the Allwinner GPADC device tree binding for H3
iio: adc: sun4i-gpadc-iio: rename A33-specified registers to contain
A33
iio: adc: sun4i-gpadc-iio: rework code for supporting newer THS
variants
iio: adc: sun4i-gpadc-iio: add support for H3 thermal sensor
ARM: sun8i: h3: add support for the thermal sensor in H3
ARM: sun8i: h3: add partial CPU thermal zone
.../devicetree/bindings/mfd/sun4i-gpadc.txt | 30 +++-
arch/arm/boot/dts/sun8i-h3.dtsi | 26 ++++
drivers/iio/adc/sun4i-gpadc-iio.c | 173 ++++++++++++++++++++-
include/linux/mfd/sun4i-gpadc.h | 33 +++-
4 files changed, 249 insertions(+), 13 deletions(-)
--
2.13.5
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v4 1/6] dt-bindings: update the Allwinner GPADC device tree binding for H3
2017-09-14 14:52 [PATCH v4 0/6] IIO-based thermal sensor driver for Allwinner H3 SoC Icenowy Zheng
@ 2017-09-14 14:52 ` Icenowy Zheng
2017-09-16 22:12 ` Jonathan Cameron
2017-09-18 7:33 ` Maxime Ripard
2017-09-14 14:52 ` [PATCH v4 2/6] iio: adc: sun4i-gpadc-iio: rename A33-specified registers to contain A33 Icenowy Zheng
` (4 subsequent siblings)
5 siblings, 2 replies; 28+ messages in thread
From: Icenowy Zheng @ 2017-09-14 14:52 UTC (permalink / raw)
To: Lee Jones, Rob Herring, Maxime Ripard, Chen-Yu Tsai,
Jonathan Cameron, Quentin Schulz
Cc: devicetree, linux-arm-kernel, linux-kernel, linux-iio,
linux-sunxi, Icenowy Zheng
Allwinner H3 features a thermal sensor like the one in A33, but has its
register re-arranged, the clock divider moved to CCU (originally the
clock divider is in ADC) and added a pair of bus clock and reset.
Update the binding document to cover H3.
Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
Reviewed-by: Chen-Yu Tsai <wens@csie.org>
---
Changes in v4:
- Add nvmem calibration data (not yet used by the driver)
Changes in v3:
- Clock name changes.
- Example node name changes.
- Add interupts (not yet used by the driver).
.../devicetree/bindings/mfd/sun4i-gpadc.txt | 30 ++++++++++++++++++++--
1 file changed, 28 insertions(+), 2 deletions(-)
diff --git a/Documentation/devicetree/bindings/mfd/sun4i-gpadc.txt b/Documentation/devicetree/bindings/mfd/sun4i-gpadc.txt
index badff3611a98..6c470d584bf9 100644
--- a/Documentation/devicetree/bindings/mfd/sun4i-gpadc.txt
+++ b/Documentation/devicetree/bindings/mfd/sun4i-gpadc.txt
@@ -4,12 +4,26 @@ The Allwinner SoCs all have an ADC that can also act as a thermal sensor
and sometimes as a touchscreen controller.
Required properties:
- - compatible: "allwinner,sun8i-a33-ths",
+ - compatible: must contain one of the following compatibles:
+ - "allwinner,sun8i-a33-ths"
+ - "allwinner,sun8i-h3-ths"
- reg: mmio address range of the chip,
- #thermal-sensor-cells: shall be 0,
- #io-channel-cells: shall be 0,
-Example:
+Optional properties:
+ - nvmem-cells: A phandle to the calibration data provided by a nvmem device.
+ If unspecified default values shall be used.
+ - nvmem-cell-names: Should be "calibration-data"
+
+Required properties for the following compatibles:
+ - "allwinner,sun8i-h3-ths"
+ - clocks: the bus clock and the input clock of the ADC,
+ - clock-names: should be "bus" and "mod",
+ - resets: the bus reset of the ADC,
+ - interrupts: the sampling interrupt of the ADC,
+
+Example for A33:
ths: ths@01c25000 {
compatible = "allwinner,sun8i-a33-ths";
reg = <0x01c25000 0x100>;
@@ -17,6 +31,18 @@ Example:
#io-channel-cells = <0>;
};
+Example for H3:
+ ths: thermal-sensor@1c25000 {
+ compatible = "allwinner,sun8i-h3-ths";
+ reg = <0x01c25000 0x400>;
+ clocks = <&ccu CLK_BUS_THS>, <&ccu CLK_THS>;
+ clock-names = "bus", "mod";
+ resets = <&ccu RST_BUS_THS>;
+ interrupts = <GIC_SPI 31 IRQ_TYPE_LEVEL_HIGH>;
+ #thermal-sensor-cells = <0>;
+ #io-channel-cells = <0>;
+ };
+
sun4i, sun5i and sun6i SoCs are also supported via the older binding:
sun4i resistive touchscreen controller
--
2.13.5
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH v4 2/6] iio: adc: sun4i-gpadc-iio: rename A33-specified registers to contain A33
2017-09-14 14:52 [PATCH v4 0/6] IIO-based thermal sensor driver for Allwinner H3 SoC Icenowy Zheng
2017-09-14 14:52 ` [PATCH v4 1/6] dt-bindings: update the Allwinner GPADC device tree binding for H3 Icenowy Zheng
@ 2017-09-14 14:52 ` Icenowy Zheng
2017-09-18 7:34 ` Maxime Ripard
2017-09-18 8:29 ` Lee Jones
2017-09-14 14:52 ` [PATCH v4 3/6] iio: adc: sun4i-gpadc-iio: rework code for supporting newer THS variants Icenowy Zheng
` (3 subsequent siblings)
5 siblings, 2 replies; 28+ messages in thread
From: Icenowy Zheng @ 2017-09-14 14:52 UTC (permalink / raw)
To: Lee Jones, Rob Herring, Maxime Ripard, Chen-Yu Tsai,
Jonathan Cameron, Quentin Schulz
Cc: devicetree, linux-arm-kernel, linux-kernel, linux-iio,
linux-sunxi, Icenowy Zheng
As the H3 SoC, which is also in sun8i line, has totally different
register map for the thermal sensor (a cut down version of GPADC), we
should rename A23/A33-specified registers to contain A33, in order to
prevent obfuscation with H3 registers. Currently these registers are
only prefixed "SUN8I", not "SUN8I_A33".
Add "_A33" after "SUN8I" on the register names.
Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
Reviewed-by: Chen-Yu Tsai <wens@csie.org>
---
Changes in v4:
- Change A23 to A33, as the driver never supports A23.
drivers/iio/adc/sun4i-gpadc-iio.c | 2 +-
include/linux/mfd/sun4i-gpadc.h | 6 +++---
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/iio/adc/sun4i-gpadc-iio.c b/drivers/iio/adc/sun4i-gpadc-iio.c
index 137f577d9432..68926b986cd0 100644
--- a/drivers/iio/adc/sun4i-gpadc-iio.c
+++ b/drivers/iio/adc/sun4i-gpadc-iio.c
@@ -88,7 +88,7 @@ static const struct gpadc_data sun6i_gpadc_data = {
static const struct gpadc_data sun8i_a33_gpadc_data = {
.temp_offset = -1662,
.temp_scale = 162,
- .tp_mode_en = SUN8I_GPADC_CTRL1_CHOP_TEMP_EN,
+ .tp_mode_en = SUN8I_A33_GPADC_CTRL1_CHOP_TEMP_EN,
};
struct sun4i_gpadc_iio {
diff --git a/include/linux/mfd/sun4i-gpadc.h b/include/linux/mfd/sun4i-gpadc.h
index 139872c2e0fe..78d31984a222 100644
--- a/include/linux/mfd/sun4i-gpadc.h
+++ b/include/linux/mfd/sun4i-gpadc.h
@@ -38,9 +38,9 @@
#define SUN6I_GPADC_CTRL1_ADC_CHAN_SELECT(x) (GENMASK(3, 0) & BIT(x))
#define SUN6I_GPADC_CTRL1_ADC_CHAN_MASK GENMASK(3, 0)
-/* TP_CTRL1 bits for sun8i SoCs */
-#define SUN8I_GPADC_CTRL1_CHOP_TEMP_EN BIT(8)
-#define SUN8I_GPADC_CTRL1_GPADC_CALI_EN BIT(7)
+/* TP_CTRL1 bits for A33 */
+#define SUN8I_A33_GPADC_CTRL1_CHOP_TEMP_EN BIT(8)
+#define SUN8I_A33_GPADC_CTRL1_GPADC_CALI_EN BIT(7)
#define SUN4I_GPADC_CTRL2 0x08
--
2.13.5
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH v4 3/6] iio: adc: sun4i-gpadc-iio: rework code for supporting newer THS variants
2017-09-14 14:52 [PATCH v4 0/6] IIO-based thermal sensor driver for Allwinner H3 SoC Icenowy Zheng
2017-09-14 14:52 ` [PATCH v4 1/6] dt-bindings: update the Allwinner GPADC device tree binding for H3 Icenowy Zheng
2017-09-14 14:52 ` [PATCH v4 2/6] iio: adc: sun4i-gpadc-iio: rename A33-specified registers to contain A33 Icenowy Zheng
@ 2017-09-14 14:52 ` Icenowy Zheng
2017-09-18 7:36 ` Maxime Ripard
2017-09-14 14:52 ` [PATCH v4 4/6] iio: adc: sun4i-gpadc-iio: add support for H3 thermal sensor Icenowy Zheng
` (2 subsequent siblings)
5 siblings, 1 reply; 28+ messages in thread
From: Icenowy Zheng @ 2017-09-14 14:52 UTC (permalink / raw)
To: Lee Jones, Rob Herring, Maxime Ripard, Chen-Yu Tsai,
Jonathan Cameron, Quentin Schulz
Cc: devicetree, linux-arm-kernel, linux-kernel, linux-iio,
linux-sunxi, Icenowy Zheng
The SoCs after H3 has newer thermal sensor ADCs, which have two clock
inputs (bus clock and sampling clock) and a reset. The registers are
also re-arranged.
This commit reworks the code, adds the process of the clocks and
resets, and allows the sampling start/end code and the position of value
readout register to be altered.
Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
---
drivers/iio/adc/sun4i-gpadc-iio.c | 123 +++++++++++++++++++++++++++++++++++---
1 file changed, 116 insertions(+), 7 deletions(-)
diff --git a/drivers/iio/adc/sun4i-gpadc-iio.c b/drivers/iio/adc/sun4i-gpadc-iio.c
index 68926b986cd0..97845982d050 100644
--- a/drivers/iio/adc/sun4i-gpadc-iio.c
+++ b/drivers/iio/adc/sun4i-gpadc-iio.c
@@ -22,6 +22,7 @@
* shutdown for not being used.
*/
+#include <linux/clk.h>
#include <linux/completion.h>
#include <linux/interrupt.h>
#include <linux/io.h>
@@ -31,6 +32,7 @@
#include <linux/platform_device.h>
#include <linux/pm_runtime.h>
#include <linux/regmap.h>
+#include <linux/reset.h>
#include <linux/thermal.h>
#include <linux/delay.h>
@@ -49,6 +51,15 @@ static unsigned int sun6i_gpadc_chan_select(unsigned int chan)
return SUN6I_GPADC_CTRL1_ADC_CHAN_SELECT(chan);
}
+struct sun4i_gpadc_iio;
+
+/*
+ * Prototypes for these functions, which enable these functions to be
+ * referenced in gpadc_data structures.
+ */
+static int sun4i_gpadc_sample_start(struct sun4i_gpadc_iio *info);
+static int sun4i_gpadc_sample_end(struct sun4i_gpadc_iio *info);
+
struct gpadc_data {
int temp_offset;
int temp_scale;
@@ -56,6 +67,12 @@ struct gpadc_data {
unsigned int tp_adc_select;
unsigned int (*adc_chan_select)(unsigned int chan);
unsigned int adc_chan_mask;
+ unsigned int temp_data;
+ int (*sample_start)(struct sun4i_gpadc_iio *info);
+ int (*sample_end)(struct sun4i_gpadc_iio *info);
+ bool has_bus_clk;
+ bool has_bus_rst;
+ bool has_mod_clk;
};
static const struct gpadc_data sun4i_gpadc_data = {
@@ -65,6 +82,9 @@ static const struct gpadc_data sun4i_gpadc_data = {
.tp_adc_select = SUN4I_GPADC_CTRL1_TP_ADC_SELECT,
.adc_chan_select = &sun4i_gpadc_chan_select,
.adc_chan_mask = SUN4I_GPADC_CTRL1_ADC_CHAN_MASK,
+ .temp_data = SUN4I_GPADC_TEMP_DATA,
+ .sample_start = sun4i_gpadc_sample_start,
+ .sample_end = sun4i_gpadc_sample_end,
};
static const struct gpadc_data sun5i_gpadc_data = {
@@ -74,6 +94,9 @@ static const struct gpadc_data sun5i_gpadc_data = {
.tp_adc_select = SUN4I_GPADC_CTRL1_TP_ADC_SELECT,
.adc_chan_select = &sun4i_gpadc_chan_select,
.adc_chan_mask = SUN4I_GPADC_CTRL1_ADC_CHAN_MASK,
+ .temp_data = SUN4I_GPADC_TEMP_DATA,
+ .sample_start = sun4i_gpadc_sample_start,
+ .sample_end = sun4i_gpadc_sample_end,
};
static const struct gpadc_data sun6i_gpadc_data = {
@@ -83,12 +106,18 @@ static const struct gpadc_data sun6i_gpadc_data = {
.tp_adc_select = SUN6I_GPADC_CTRL1_TP_ADC_SELECT,
.adc_chan_select = &sun6i_gpadc_chan_select,
.adc_chan_mask = SUN6I_GPADC_CTRL1_ADC_CHAN_MASK,
+ .temp_data = SUN4I_GPADC_TEMP_DATA,
+ .sample_start = sun4i_gpadc_sample_start,
+ .sample_end = sun4i_gpadc_sample_end,
};
static const struct gpadc_data sun8i_a33_gpadc_data = {
.temp_offset = -1662,
.temp_scale = 162,
.tp_mode_en = SUN8I_A33_GPADC_CTRL1_CHOP_TEMP_EN,
+ .temp_data = SUN4I_GPADC_TEMP_DATA,
+ .sample_start = sun4i_gpadc_sample_start,
+ .sample_end = sun4i_gpadc_sample_end,
};
struct sun4i_gpadc_iio {
@@ -103,6 +132,9 @@ struct sun4i_gpadc_iio {
atomic_t ignore_temp_data_irq;
const struct gpadc_data *data;
bool no_irq;
+ struct clk *bus_clk;
+ struct clk *mod_clk;
+ struct reset_control *reset;
/* prevents concurrent reads of temperature and ADC */
struct mutex mutex;
struct thermal_zone_device *tzd;
@@ -277,7 +309,7 @@ static int sun4i_gpadc_temp_read(struct iio_dev *indio_dev, int *val)
if (info->no_irq) {
pm_runtime_get_sync(indio_dev->dev.parent);
- regmap_read(info->regmap, SUN4I_GPADC_TEMP_DATA, val);
+ regmap_read(info->regmap, info->data->temp_data, val);
pm_runtime_mark_last_busy(indio_dev->dev.parent);
pm_runtime_put_autosuspend(indio_dev->dev.parent);
@@ -383,10 +415,8 @@ static irqreturn_t sun4i_gpadc_fifo_data_irq_handler(int irq, void *dev_id)
return IRQ_HANDLED;
}
-static int sun4i_gpadc_runtime_suspend(struct device *dev)
+static int sun4i_gpadc_sample_end(struct sun4i_gpadc_iio *info)
{
- struct sun4i_gpadc_iio *info = iio_priv(dev_get_drvdata(dev));
-
/* Disable the ADC on IP */
regmap_write(info->regmap, SUN4I_GPADC_CTRL1, 0);
/* Disable temperature sensor on IP */
@@ -395,10 +425,15 @@ static int sun4i_gpadc_runtime_suspend(struct device *dev)
return 0;
}
-static int sun4i_gpadc_runtime_resume(struct device *dev)
+static int sun4i_gpadc_runtime_suspend(struct device *dev)
{
struct sun4i_gpadc_iio *info = iio_priv(dev_get_drvdata(dev));
+ return info->data->sample_end(info);
+}
+
+static int sun4i_gpadc_sample_start(struct sun4i_gpadc_iio *info)
+{
/* clkin = 6MHz */
regmap_write(info->regmap, SUN4I_GPADC_CTRL0,
SUN4I_GPADC_CTRL0_ADC_CLK_DIVIDER(2) |
@@ -416,6 +451,13 @@ static int sun4i_gpadc_runtime_resume(struct device *dev)
return 0;
}
+static int sun4i_gpadc_runtime_resume(struct device *dev)
+{
+ struct sun4i_gpadc_iio *info = iio_priv(dev_get_drvdata(dev));
+
+ return info->data->sample_start(info);
+}
+
static int sun4i_gpadc_get_temp(void *data, int *temp)
{
struct sun4i_gpadc_iio *info = data;
@@ -529,17 +571,75 @@ static int sun4i_gpadc_probe_dt(struct platform_device *pdev,
return ret;
}
+ if (info->data->has_bus_rst) {
+ info->reset = devm_reset_control_get(&pdev->dev, NULL);
+ if (IS_ERR(info->reset)) {
+ ret = PTR_ERR(info->reset);
+ return ret;
+ }
+
+ ret = reset_control_deassert(info->reset);
+ if (ret)
+ return ret;
+ }
+
+ if (info->data->has_bus_clk) {
+ info->bus_clk = devm_clk_get(&pdev->dev, "bus");
+ if (IS_ERR(info->bus_clk)) {
+ ret = PTR_ERR(info->bus_clk);
+ goto assert_reset;
+ }
+
+ ret = clk_prepare_enable(info->bus_clk);
+ if (ret)
+ goto assert_reset;
+ }
+
+ if (info->data->has_mod_clk) {
+ info->mod_clk = devm_clk_get(&pdev->dev, "mod");
+ if (IS_ERR(info->mod_clk)) {
+ ret = PTR_ERR(info->mod_clk);
+ goto disable_bus_clk;
+ }
+
+ /* Running at 6MHz */
+ ret = clk_set_rate(info->mod_clk, 6000000);
+ if (ret)
+ goto disable_bus_clk;
+
+ ret = clk_prepare_enable(info->mod_clk);
+ if (ret)
+ goto disable_bus_clk;
+ }
+
if (!IS_ENABLED(CONFIG_THERMAL_OF))
return 0;
info->sensor_device = &pdev->dev;
info->tzd = thermal_zone_of_sensor_register(info->sensor_device, 0,
info, &sun4i_ts_tz_ops);
- if (IS_ERR(info->tzd))
+ if (IS_ERR(info->tzd)) {
dev_err(&pdev->dev, "could not register thermal sensor: %ld\n",
PTR_ERR(info->tzd));
+ ret = PTR_ERR(info->tzd);
+ goto disable_mod_clk;
+ }
+
+ return 0;
+
+disable_mod_clk:
+ if (info->data->has_mod_clk)
+ clk_disable_unprepare(info->mod_clk);
+
+disable_bus_clk:
+ if (info->data->has_bus_clk)
+ clk_disable_unprepare(info->bus_clk);
- return PTR_ERR_OR_ZERO(info->tzd);
+assert_reset:
+ if (info->data->has_bus_rst)
+ reset_control_assert(info->reset);
+
+ return ret;
}
static int sun4i_gpadc_probe_mfd(struct platform_device *pdev,
@@ -698,6 +798,15 @@ static int sun4i_gpadc_remove(struct platform_device *pdev)
if (!info->no_irq)
iio_map_array_unregister(indio_dev);
+ if (info->data->has_mod_clk)
+ clk_disable_unprepare(info->mod_clk);
+
+ if (info->data->has_bus_clk)
+ clk_disable_unprepare(info->bus_clk);
+
+ if (info->data->has_bus_rst)
+ reset_control_assert(info->reset);
+
return 0;
}
--
2.13.5
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH v4 4/6] iio: adc: sun4i-gpadc-iio: add support for H3 thermal sensor
2017-09-14 14:52 [PATCH v4 0/6] IIO-based thermal sensor driver for Allwinner H3 SoC Icenowy Zheng
` (2 preceding siblings ...)
2017-09-14 14:52 ` [PATCH v4 3/6] iio: adc: sun4i-gpadc-iio: rework code for supporting newer THS variants Icenowy Zheng
@ 2017-09-14 14:52 ` Icenowy Zheng
2017-09-16 9:45 ` Quentin Schulz
2017-09-14 14:52 ` [PATCH v4 5/6] ARM: sun8i: h3: add support for the thermal sensor in H3 Icenowy Zheng
2017-09-14 14:52 ` [PATCH v4 6/6] ARM: sun8i: h3: add partial CPU thermal zone Icenowy Zheng
5 siblings, 1 reply; 28+ messages in thread
From: Icenowy Zheng @ 2017-09-14 14:52 UTC (permalink / raw)
To: Lee Jones, Rob Herring, Maxime Ripard, Chen-Yu Tsai,
Jonathan Cameron, Quentin Schulz
Cc: devicetree, linux-arm-kernel, linux-kernel, linux-iio,
linux-sunxi, Icenowy Zheng
This adds support for the Allwinner H3 thermal sensor.
Allwinner H3 has a thermal sensor like the one in A33, but have its
registers nearly all re-arranged, sample clock moved to CCU and a pair
of bus clock and reset added. It's also the base of newer SoCs' thermal
sensors.
The thermal sensors on A64 and H5 is like the one on H3, but with of
course different formula factors.
Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
---
Changes in v4:
- Splitted out some code refactors.
- Code sequence changed back. (The gpadc_data went back to the start of
the source file)
drivers/iio/adc/sun4i-gpadc-iio.c | 48 +++++++++++++++++++++++++++++++++++++++
include/linux/mfd/sun4i-gpadc.h | 27 ++++++++++++++++++++++
2 files changed, 75 insertions(+)
diff --git a/drivers/iio/adc/sun4i-gpadc-iio.c b/drivers/iio/adc/sun4i-gpadc-iio.c
index 97845982d050..f7e4df6bd17a 100644
--- a/drivers/iio/adc/sun4i-gpadc-iio.c
+++ b/drivers/iio/adc/sun4i-gpadc-iio.c
@@ -59,6 +59,8 @@ struct sun4i_gpadc_iio;
*/
static int sun4i_gpadc_sample_start(struct sun4i_gpadc_iio *info);
static int sun4i_gpadc_sample_end(struct sun4i_gpadc_iio *info);
+static int sun8i_h3_gpadc_sample_start(struct sun4i_gpadc_iio *info);
+static int sun8i_h3_gpadc_sample_end(struct sun4i_gpadc_iio *info);
struct gpadc_data {
int temp_offset;
@@ -120,6 +122,24 @@ static const struct gpadc_data sun8i_a33_gpadc_data = {
.sample_end = sun4i_gpadc_sample_end,
};
+static const struct gpadc_data sun8i_h3_gpadc_data = {
+ /*
+ * The original formula on the datasheet seems to be wrong.
+ * These factors are calculated based on the formula in the BSP
+ * kernel, which is originally Tem = 217 - (T / 8.253), in which Tem
+ * is the temperature in Celsius degree and T is the raw value
+ * from the sensor.
+ */
+ .temp_offset = -1791,
+ .temp_scale = -121,
+ .temp_data = SUN8I_H3_GPADC_TEMP_DATA,
+ .sample_start = sun8i_h3_gpadc_sample_start,
+ .sample_end = sun8i_h3_gpadc_sample_end,
+ .has_bus_clk = true,
+ .has_bus_rst = true,
+ .has_mod_clk = true,
+};
+
struct sun4i_gpadc_iio {
struct iio_dev *indio_dev;
struct completion completion;
@@ -425,6 +445,14 @@ static int sun4i_gpadc_sample_end(struct sun4i_gpadc_iio *info)
return 0;
}
+static int sun8i_h3_gpadc_sample_end(struct sun4i_gpadc_iio *info)
+{
+ /* Disable temperature sensor */
+ regmap_write(info->regmap, SUN8I_H3_GPADC_CTRL2, 0);
+
+ return 0;
+}
+
static int sun4i_gpadc_runtime_suspend(struct device *dev)
{
struct sun4i_gpadc_iio *info = iio_priv(dev_get_drvdata(dev));
@@ -451,6 +479,22 @@ static int sun4i_gpadc_sample_start(struct sun4i_gpadc_iio *info)
return 0;
}
+static int sun8i_h3_gpadc_sample_start(struct sun4i_gpadc_iio *info)
+{
+ regmap_write(info->regmap, SUN8I_H3_GPADC_CTRL2,
+ SUN8I_H3_GPADC_CTRL2_TEMP_SENSE_EN |
+ SUN8I_H3_GPADC_CTRL2_T_ACQ1(31));
+ regmap_write(info->regmap, SUN4I_GPADC_CTRL0,
+ SUN4I_GPADC_CTRL0_T_ACQ(31));
+ regmap_write(info->regmap, SUN8I_H3_GPADC_CTRL3,
+ SUN4I_GPADC_CTRL3_FILTER_EN |
+ SUN4I_GPADC_CTRL3_FILTER_TYPE(1));
+ regmap_write(info->regmap, SUN8I_H3_GPADC_INTC,
+ SUN8I_H3_GPADC_INTC_TEMP_PERIOD(800));
+
+ return 0;
+}
+
static int sun4i_gpadc_runtime_resume(struct device *dev)
{
struct sun4i_gpadc_iio *info = iio_priv(dev_get_drvdata(dev));
@@ -537,6 +581,10 @@ static const struct of_device_id sun4i_gpadc_of_id[] = {
.compatible = "allwinner,sun8i-a33-ths",
.data = &sun8i_a33_gpadc_data,
},
+ {
+ .compatible = "allwinner,sun8i-h3-ths",
+ .data = &sun8i_h3_gpadc_data,
+ },
{ /* sentinel */ }
};
diff --git a/include/linux/mfd/sun4i-gpadc.h b/include/linux/mfd/sun4i-gpadc.h
index 78d31984a222..5c2a12101052 100644
--- a/include/linux/mfd/sun4i-gpadc.h
+++ b/include/linux/mfd/sun4i-gpadc.h
@@ -42,6 +42,9 @@
#define SUN8I_A33_GPADC_CTRL1_CHOP_TEMP_EN BIT(8)
#define SUN8I_A33_GPADC_CTRL1_GPADC_CALI_EN BIT(7)
+/* TP_CTRL1 bits for SoCs after H3 */
+#define SUN8I_H3_GPADC_CTRL1_GPADC_CALI_EN BIT(17)
+
#define SUN4I_GPADC_CTRL2 0x08
#define SUN4I_GPADC_CTRL2_TP_SENSITIVE_ADJUST(x) ((GENMASK(3, 0) & (x)) << 28)
@@ -49,7 +52,17 @@
#define SUN4I_GPADC_CTRL2_PRE_MEA_EN BIT(24)
#define SUN4I_GPADC_CTRL2_PRE_MEA_THRE_CNT(x) (GENMASK(23, 0) & (x))
+#define SUN8I_H3_GPADC_CTRL2 0x40
+
+#define SUN8I_H3_GPADC_CTRL2_TEMP_SENSE_EN BIT(0)
+#define SUN8I_H3_GPADC_CTRL2_T_ACQ1(x) ((GENMASK(15, 0) * (x)) << 16)
+
#define SUN4I_GPADC_CTRL3 0x0c
+/*
+ * This register is named "Average filter Control Register" in H3 Datasheet,
+ * but the register's definition is the same as the old CTRL3 register.
+ */
+#define SUN8I_H3_GPADC_CTRL3 0x70
#define SUN4I_GPADC_CTRL3_FILTER_EN BIT(2)
#define SUN4I_GPADC_CTRL3_FILTER_TYPE(x) (GENMASK(1, 0) & (x))
@@ -71,6 +84,13 @@
#define SUN4I_GPADC_INT_FIFOC_TP_UP_IRQ_EN BIT(1)
#define SUN4I_GPADC_INT_FIFOC_TP_DOWN_IRQ_EN BIT(0)
+#define SUN8I_H3_GPADC_INTC 0x44
+
+#define SUN8I_H3_GPADC_INTC_TEMP_PERIOD(x) ((GENMASK(19, 0) & (x)) << 12)
+#define SUN8I_H3_GPADC_INTC_TEMP_DATA BIT(8)
+#define SUN8I_H3_GPADC_INTC_TEMP_SHUT BIT(4)
+#define SUN8I_H3_GPADC_INTC_TEMP_ALARM BIT(0)
+
#define SUN4I_GPADC_INT_FIFOS 0x14
#define SUN4I_GPADC_INT_FIFOS_TEMP_DATA_PENDING BIT(18)
@@ -80,9 +100,16 @@
#define SUN4I_GPADC_INT_FIFOS_TP_UP_PENDING BIT(1)
#define SUN4I_GPADC_INT_FIFOS_TP_DOWN_PENDING BIT(0)
+#define SUN8I_H3_GPADC_INTS 0x44
+
+#define SUN8I_H3_GPADC_INTS_TEMP_DATA BIT(8)
+#define SUN8I_H3_GPADC_INTS_TEMP_SHUT BIT(4)
+#define SUN8I_H3_GPADC_INTS_TEMP_ALARM BIT(0)
+
#define SUN4I_GPADC_CDAT 0x1c
#define SUN4I_GPADC_TEMP_DATA 0x20
#define SUN4I_GPADC_DATA 0x24
+#define SUN8I_H3_GPADC_TEMP_DATA 0x80
#define SUN4I_GPADC_IRQ_FIFO_DATA 0
#define SUN4I_GPADC_IRQ_TEMP_DATA 1
--
2.13.5
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH v4 5/6] ARM: sun8i: h3: add support for the thermal sensor in H3
2017-09-14 14:52 [PATCH v4 0/6] IIO-based thermal sensor driver for Allwinner H3 SoC Icenowy Zheng
` (3 preceding siblings ...)
2017-09-14 14:52 ` [PATCH v4 4/6] iio: adc: sun4i-gpadc-iio: add support for H3 thermal sensor Icenowy Zheng
@ 2017-09-14 14:52 ` Icenowy Zheng
2017-09-18 8:25 ` Maxime Ripard
2017-09-14 14:52 ` [PATCH v4 6/6] ARM: sun8i: h3: add partial CPU thermal zone Icenowy Zheng
5 siblings, 1 reply; 28+ messages in thread
From: Icenowy Zheng @ 2017-09-14 14:52 UTC (permalink / raw)
To: Lee Jones, Rob Herring, Maxime Ripard, Chen-Yu Tsai,
Jonathan Cameron, Quentin Schulz
Cc: devicetree, linux-arm-kernel, linux-kernel, linux-iio,
linux-sunxi, Icenowy Zheng
As we have gained the support for the thermal sensor in H3, we can now
add its device nodes to the device tree.
Add them to the H3 device tree.
The calibration data of the thermal sensor is still not added, as
it's currently not used, and the SID node is not added yet.
The H5 thermal sensor has some differences, and will be added furtherly.
Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
Reviewed-by: Chen-Yu Tsai <wens@csie.org>
---
Changes in v4:
- Mention calibration data in commit message.
Changes in v3:
- Clock name changes.
- Splited out thermal zone addition.
arch/arm/boot/dts/sun8i-h3.dtsi | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
diff --git a/arch/arm/boot/dts/sun8i-h3.dtsi b/arch/arm/boot/dts/sun8i-h3.dtsi
index b36f9f423c39..3220da3ad790 100644
--- a/arch/arm/boot/dts/sun8i-h3.dtsi
+++ b/arch/arm/boot/dts/sun8i-h3.dtsi
@@ -72,6 +72,23 @@
};
};
+ iio-hwmon {
+ compatible = "iio-hwmon";
+ io-channels = <&ths>;
+ };
+
+ soc {
+ ths: thermal-sensor@1c25000 {
+ compatible = "allwinner,sun8i-h3-ths";
+ reg = <0x01c25000 0x100>;
+ clocks = <&ccu CLK_BUS_THS>, <&ccu CLK_THS>;
+ clock-names = "bus", "mod";
+ resets = <&ccu RST_BUS_THS>;
+ #thermal-sensor-cells = <0>;
+ #io-channel-cells = <0>;
+ };
+ };
+
timer {
compatible = "arm,armv7-timer";
interrupts = <GIC_PPI 13 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
--
2.13.5
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH v4 6/6] ARM: sun8i: h3: add partial CPU thermal zone
2017-09-14 14:52 [PATCH v4 0/6] IIO-based thermal sensor driver for Allwinner H3 SoC Icenowy Zheng
` (4 preceding siblings ...)
2017-09-14 14:52 ` [PATCH v4 5/6] ARM: sun8i: h3: add support for the thermal sensor in H3 Icenowy Zheng
@ 2017-09-14 14:52 ` Icenowy Zheng
2017-09-16 10:05 ` Quentin Schulz
5 siblings, 1 reply; 28+ messages in thread
From: Icenowy Zheng @ 2017-09-14 14:52 UTC (permalink / raw)
To: Lee Jones, Rob Herring, Maxime Ripard, Chen-Yu Tsai,
Jonathan Cameron, Quentin Schulz
Cc: devicetree, linux-arm-kernel, linux-kernel, linux-iio,
linux-sunxi, Icenowy Zheng
Because of the restriction of the OF thermal framework, the thermal
sensor will fail to probe if the thermal zone doesn't exist.
Add a partial thermal zone which claims the H3 THS as the thermal sensor.
The cooling device (CPU DVFS) is still not added as it's not ready, and
the trip points are also not added yet.
Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
---
arch/arm/boot/dts/sun8i-h3.dtsi | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/arch/arm/boot/dts/sun8i-h3.dtsi b/arch/arm/boot/dts/sun8i-h3.dtsi
index 3220da3ad790..687c6457d214 100644
--- a/arch/arm/boot/dts/sun8i-h3.dtsi
+++ b/arch/arm/boot/dts/sun8i-h3.dtsi
@@ -89,6 +89,15 @@
};
};
+ thermal-zones {
+ cpu-thermal {
+ /* milliseconds */
+ polling-delay-passive = <250>;
+ polling-delay = <1000>;
+ thermal-sensors = <&ths>;
+ };
+ };
+
timer {
compatible = "arm,armv7-timer";
interrupts = <GIC_PPI 13 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
--
2.13.5
^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH v4 4/6] iio: adc: sun4i-gpadc-iio: add support for H3 thermal sensor
2017-09-14 14:52 ` [PATCH v4 4/6] iio: adc: sun4i-gpadc-iio: add support for H3 thermal sensor Icenowy Zheng
@ 2017-09-16 9:45 ` Quentin Schulz
2017-09-16 10:14 ` icenowy
2017-09-16 22:16 ` Jonathan Cameron
0 siblings, 2 replies; 28+ messages in thread
From: Quentin Schulz @ 2017-09-16 9:45 UTC (permalink / raw)
To: Icenowy Zheng, Lee Jones, Rob Herring, Maxime Ripard,
Chen-Yu Tsai, Jonathan Cameron
Cc: devicetree, linux-iio, linux-kernel, linux-sunxi, linux-arm-kernel
Hi Icenowy,
On 14/09/2017 16:52, Icenowy Zheng wrote:
> This adds support for the Allwinner H3 thermal sensor.
>
> Allwinner H3 has a thermal sensor like the one in A33, but have its
> registers nearly all re-arranged, sample clock moved to CCU and a pair
> of bus clock and reset added. It's also the base of newer SoCs' thermal
> sensors.
>
> The thermal sensors on A64 and H5 is like the one on H3, but with of
> course different formula factors.
>
> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
> ---
> Changes in v4:
> - Splitted out some code refactors.
> - Code sequence changed back. (The gpadc_data went back to the start of
> the source file)
>
> drivers/iio/adc/sun4i-gpadc-iio.c | 48 +++++++++++++++++++++++++++++++++++++++
> include/linux/mfd/sun4i-gpadc.h | 27 ++++++++++++++++++++++
> 2 files changed, 75 insertions(+)
[...]
> diff --git a/include/linux/mfd/sun4i-gpadc.h b/include/linux/mfd/sun4i-gpadc.h
> index 78d31984a222..5c2a12101052 100644
> --- a/include/linux/mfd/sun4i-gpadc.h
> +++ b/include/linux/mfd/sun4i-gpadc.h
[...]
> +#define SUN8I_H3_GPADC_CTRL2_T_ACQ1(x) ((GENMASK(15, 0) * (x)) << 16)
> +
You want to replace * by &.
((GENMASK(15, 0) & (x)) << 16)
Would ((GENMASK(31, 16) & ((x) << 16)) make the bits you set even more
obvious?
> #define SUN4I_GPADC_CTRL3 0x0c
> +/*
> + * This register is named "Average filter Control Register" in H3 Datasheet,
> + * but the register's definition is the same as the old CTRL3 register.
> + */
> +#define SUN8I_H3_GPADC_CTRL3 0x70
>
I would name it as it is in the documentation:
SUN8I_H3_THS_FILTER
No need for comments then.
> #define SUN4I_GPADC_CTRL3_FILTER_EN BIT(2)
> #define SUN4I_GPADC_CTRL3_FILTER_TYPE(x) (GENMASK(1, 0) & (x))
> @@ -71,6 +84,13 @@
> #define SUN4I_GPADC_INT_FIFOC_TP_UP_IRQ_EN BIT(1)
> #define SUN4I_GPADC_INT_FIFOC_TP_DOWN_IRQ_EN BIT(0)
>
> +#define SUN8I_H3_GPADC_INTC 0x44
> +
> +#define SUN8I_H3_GPADC_INTC_TEMP_PERIOD(x) ((GENMASK(19, 0) & (x)) << 12)
> +#define SUN8I_H3_GPADC_INTC_TEMP_DATA BIT(8)
> +#define SUN8I_H3_GPADC_INTC_TEMP_SHUT BIT(4)
> +#define SUN8I_H3_GPADC_INTC_TEMP_ALARM BIT(0)
> +
Since it isn't an ADC anymore but rather just a THS, why don't you use
SUN8I_H3_THS instead of SUN8I_H3_GPADC? That way, it also matches the
datasheet.
> #define SUN4I_GPADC_INT_FIFOS 0x14
>
> #define SUN4I_GPADC_INT_FIFOS_TEMP_DATA_PENDING BIT(18)
> @@ -80,9 +100,16 @@
> #define SUN4I_GPADC_INT_FIFOS_TP_UP_PENDING BIT(1)
> #define SUN4I_GPADC_INT_FIFOS_TP_DOWN_PENDING BIT(0)
>
> +#define SUN8I_H3_GPADC_INTS 0x44
0x48
[...]
1) You're not using irqs, why would you define registers that will never
be used?
2) Why aren't you using irqs? I remember we discussed on IRC that you
had some problems with the H3 when resuming or when probing the driver.
The register would have a zero in it until you have a first sample that
arrived (i.e. after the sample rate you set with T_ACQ) that would make
the thermal framework panic since the thermal sensor would return
something way too hot and shutdown your board?
The H3 apparently supports IRQs, why do you not support them for the
temperature? They might be broken as it is on A33 but then it might be a
good idea to write it down in a comment in the driver (and not adding
the unused registers in the header file) or at least in the commit log.
3) Now that you have support for clocks, wouldn't it be a good idea to
disable them during suspend?
Thanks,
Quentin
--
Quentin Schulz, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v4 6/6] ARM: sun8i: h3: add partial CPU thermal zone
2017-09-14 14:52 ` [PATCH v4 6/6] ARM: sun8i: h3: add partial CPU thermal zone Icenowy Zheng
@ 2017-09-16 10:05 ` Quentin Schulz
2017-09-16 22:17 ` Jonathan Cameron
0 siblings, 1 reply; 28+ messages in thread
From: Quentin Schulz @ 2017-09-16 10:05 UTC (permalink / raw)
To: Icenowy Zheng, Lee Jones, Rob Herring, Maxime Ripard,
Chen-Yu Tsai, Jonathan Cameron
Cc: devicetree, linux-arm-kernel, linux-kernel, linux-iio, linux-sunxi
Hi Icenowy,
On 14/09/2017 16:52, Icenowy Zheng wrote:
> Because of the restriction of the OF thermal framework, the thermal
> sensor will fail to probe if the thermal zone doesn't exist.
>
Oh no, that's not good.
We discussed about it on IRC and I even proposed a patch for it, telling
you I would post it on the mailing list soon after. Of course, I forgot
and you definitely should have yelled at me for not doing it :)
I won't be able to test the patch soon. I can send it to you so that you
can test it and integrate it in your patch series so it won't block you.
Otherwise, we'll have to wait for a week or two for me to test it.
Thanks and sorry for forgetting to post the patch you need,
Quentin
> Add a partial thermal zone which claims the H3 THS as the thermal sensor.
>
> The cooling device (CPU DVFS) is still not added as it's not ready, and
> the trip points are also not added yet.
>
> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
> ---
> arch/arm/boot/dts/sun8i-h3.dtsi | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/arch/arm/boot/dts/sun8i-h3.dtsi b/arch/arm/boot/dts/sun8i-h3.dtsi
> index 3220da3ad790..687c6457d214 100644
> --- a/arch/arm/boot/dts/sun8i-h3.dtsi
> +++ b/arch/arm/boot/dts/sun8i-h3.dtsi
> @@ -89,6 +89,15 @@
> };
> };
>
> + thermal-zones {
> + cpu-thermal {
> + /* milliseconds */
> + polling-delay-passive = <250>;
> + polling-delay = <1000>;
> + thermal-sensors = <&ths>;
> + };
> + };
> +
> timer {
> compatible = "arm,armv7-timer";
> interrupts = <GIC_PPI 13 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
>
--
Quentin Schulz, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v4 4/6] iio: adc: sun4i-gpadc-iio: add support for H3 thermal sensor
2017-09-16 9:45 ` Quentin Schulz
@ 2017-09-16 10:14 ` icenowy
2017-09-16 10:35 ` Quentin Schulz
2017-09-18 8:24 ` Maxime Ripard
2017-09-16 22:16 ` Jonathan Cameron
1 sibling, 2 replies; 28+ messages in thread
From: icenowy @ 2017-09-16 10:14 UTC (permalink / raw)
To: Quentin Schulz
Cc: Lee Jones, Rob Herring, Maxime Ripard, Chen-Yu Tsai,
Jonathan Cameron, devicetree, linux-iio, linux-kernel,
linux-sunxi, linux-arm-kernel
在 2017-09-16 17:45,Quentin Schulz 写道:
> Hi Icenowy,
>
> On 14/09/2017 16:52, Icenowy Zheng wrote:
>> This adds support for the Allwinner H3 thermal sensor.
>>
>> Allwinner H3 has a thermal sensor like the one in A33, but have its
>> registers nearly all re-arranged, sample clock moved to CCU and a pair
>> of bus clock and reset added. It's also the base of newer SoCs'
>> thermal
>> sensors.
>>
>> The thermal sensors on A64 and H5 is like the one on H3, but with of
>> course different formula factors.
>>
>> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
>> ---
>> Changes in v4:
>> - Splitted out some code refactors.
>> - Code sequence changed back. (The gpadc_data went back to the start
>> of
>> the source file)
>>
>> drivers/iio/adc/sun4i-gpadc-iio.c | 48
>> +++++++++++++++++++++++++++++++++++++++
>> include/linux/mfd/sun4i-gpadc.h | 27 ++++++++++++++++++++++
>> 2 files changed, 75 insertions(+)
> [...]
>> diff --git a/include/linux/mfd/sun4i-gpadc.h
>> b/include/linux/mfd/sun4i-gpadc.h
>> index 78d31984a222..5c2a12101052 100644
>> --- a/include/linux/mfd/sun4i-gpadc.h
>> +++ b/include/linux/mfd/sun4i-gpadc.h
> [...]
>> +#define SUN8I_H3_GPADC_CTRL2_T_ACQ1(x) ((GENMASK(15, 0) * (x)) <<
>> 16)
>> +
>
> You want to replace * by &.
>
> ((GENMASK(15, 0) & (x)) << 16)
>
> Would ((GENMASK(31, 16) & ((x) << 16)) make the bits you set even more
> obvious?
>
>> #define SUN4I_GPADC_CTRL3 0x0c
>> +/*
>> + * This register is named "Average filter Control Register" in H3
>> Datasheet,
>> + * but the register's definition is the same as the old CTRL3
>> register.
>> + */
>> +#define SUN8I_H3_GPADC_CTRL3 0x70
>>
>
> I would name it as it is in the documentation:
> SUN8I_H3_THS_FILTER
The definition of this register is the same as the CTRL3.
Maybe this name is better, but the similarity between them still needs
to be documented, as the SUN4I_GPADC_CTRL3_XXX macros will be used to
populate this register.
>
> No need for comments then.
>
>> #define SUN4I_GPADC_CTRL3_FILTER_EN BIT(2)
>> #define SUN4I_GPADC_CTRL3_FILTER_TYPE(x) (GENMASK(1, 0) & (x))
>> @@ -71,6 +84,13 @@
>> #define SUN4I_GPADC_INT_FIFOC_TP_UP_IRQ_EN BIT(1)
>> #define SUN4I_GPADC_INT_FIFOC_TP_DOWN_IRQ_EN BIT(0)
>>
>> +#define SUN8I_H3_GPADC_INTC 0x44
>> +
>> +#define SUN8I_H3_GPADC_INTC_TEMP_PERIOD(x) ((GENMASK(19, 0) & (x))
>> << 12)
>> +#define SUN8I_H3_GPADC_INTC_TEMP_DATA BIT(8)
>> +#define SUN8I_H3_GPADC_INTC_TEMP_SHUT BIT(4)
>> +#define SUN8I_H3_GPADC_INTC_TEMP_ALARM BIT(0)
>> +
>
> Since it isn't an ADC anymore but rather just a THS, why don't you use
> SUN8I_H3_THS instead of SUN8I_H3_GPADC? That way, it also matches the
> datasheet.
>
>> #define SUN4I_GPADC_INT_FIFOS 0x14
>>
>> #define SUN4I_GPADC_INT_FIFOS_TEMP_DATA_PENDING BIT(18)
>> @@ -80,9 +100,16 @@
>> #define SUN4I_GPADC_INT_FIFOS_TP_UP_PENDING BIT(1)
>> #define SUN4I_GPADC_INT_FIFOS_TP_DOWN_PENDING BIT(0)
>>
>> +#define SUN8I_H3_GPADC_INTS 0x44
>
> 0x48
>
> [...]
>
> 1) You're not using irqs, why would you define registers that will
> never
> be used?
I will then rework it to use IRQs, but not now.
Maybe I should add it when I use them?
>
> 2) Why aren't you using irqs? I remember we discussed on IRC that you
> had some problems with the H3 when resuming or when probing the driver.
> The register would have a zero in it until you have a first sample that
> arrived (i.e. after the sample rate you set with T_ACQ) that would make
> the thermal framework panic since the thermal sensor would return
> something way too hot and shutdown your board?
Nope, it's another problem -- the runtime resume function is even not
called before the first sample, and the first sample will happen when
the THS is still suspended.
>
> The H3 apparently supports IRQs, why do you not support them for the
> temperature? They might be broken as it is on A33 but then it might be
> a
> good idea to write it down in a comment in the driver (and not adding
> the unused registers in the header file) or at least in the commit log.
>
> 3) Now that you have support for clocks, wouldn't it be a good idea to
> disable them during suspend?
Interesting... It's meaningful to disable the mod clock during suspend.
>
> Thanks,
> Quentin
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v4 4/6] iio: adc: sun4i-gpadc-iio: add support for H3 thermal sensor
2017-09-16 10:14 ` icenowy
@ 2017-09-16 10:35 ` Quentin Schulz
2017-09-18 8:24 ` Maxime Ripard
1 sibling, 0 replies; 28+ messages in thread
From: Quentin Schulz @ 2017-09-16 10:35 UTC (permalink / raw)
To: icenowy
Cc: devicetree, linux-iio, linux-sunxi, linux-kernel, Chen-Yu Tsai,
Rob Herring, linux-arm-kernel, Maxime Ripard, Lee Jones,
Jonathan Cameron
Hi Icenowy,
On 16/09/2017 12:14, icenowy@aosc.io wrote:
> 在 2017-09-16 17:45,Quentin Schulz 写道:
>> Hi Icenowy,
>>
>> On 14/09/2017 16:52, Icenowy Zheng wrote:
>>> This adds support for the Allwinner H3 thermal sensor.
>>>
>>> Allwinner H3 has a thermal sensor like the one in A33, but have its
>>> registers nearly all re-arranged, sample clock moved to CCU and a pair
>>> of bus clock and reset added. It's also the base of newer SoCs' thermal
>>> sensors.
>>>
>>> The thermal sensors on A64 and H5 is like the one on H3, but with of
>>> course different formula factors.
>>>
>>> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
>>> ---
>>> Changes in v4:
>>> - Splitted out some code refactors.
>>> - Code sequence changed back. (The gpadc_data went back to the start of
>>> the source file)
>>>
>>> drivers/iio/adc/sun4i-gpadc-iio.c | 48
>>> +++++++++++++++++++++++++++++++++++++++
>>> include/linux/mfd/sun4i-gpadc.h | 27 ++++++++++++++++++++++
>>> 2 files changed, 75 insertions(+)
[...]
>>> #define SUN4I_GPADC_CTRL3 0x0c
>>> +/*
>>> + * This register is named "Average filter Control Register" in H3
>>> Datasheet,
>>> + * but the register's definition is the same as the old CTRL3 register.
>>> + */
>>> +#define SUN8I_H3_GPADC_CTRL3 0x70
>>>
>>
>> I would name it as it is in the documentation:
>> SUN8I_H3_THS_FILTER
>
> The definition of this register is the same as the CTRL3.
>
> Maybe this name is better, but the similarity between them still needs
> to be documented, as the SUN4I_GPADC_CTRL3_XXX macros will be used to
> populate this register.
>
>>
>> No need for comments then.
>>
>>> #define SUN4I_GPADC_CTRL3_FILTER_EN BIT(2)
>>> #define SUN4I_GPADC_CTRL3_FILTER_TYPE(x) (GENMASK(1, 0) & (x))
They have _FILTER_ in their name, isn't it enough?
Just a matter of taste for me.
>>> @@ -71,6 +84,13 @@
>>> #define SUN4I_GPADC_INT_FIFOC_TP_UP_IRQ_EN BIT(1)
>>> #define SUN4I_GPADC_INT_FIFOC_TP_DOWN_IRQ_EN BIT(0)
>>>
>>> +#define SUN8I_H3_GPADC_INTC 0x44
>>> +
>>> +#define SUN8I_H3_GPADC_INTC_TEMP_PERIOD(x) ((GENMASK(19, 0) &
>>> (x)) << 12)
>>> +#define SUN8I_H3_GPADC_INTC_TEMP_DATA BIT(8)
>>> +#define SUN8I_H3_GPADC_INTC_TEMP_SHUT BIT(4)
>>> +#define SUN8I_H3_GPADC_INTC_TEMP_ALARM BIT(0)
>>> +
>>
>> Since it isn't an ADC anymore but rather just a THS, why don't you use
>> SUN8I_H3_THS instead of SUN8I_H3_GPADC? That way, it also matches the
>> datasheet.
>>
>>> #define SUN4I_GPADC_INT_FIFOS 0x14
>>>
>>> #define SUN4I_GPADC_INT_FIFOS_TEMP_DATA_PENDING BIT(18)
>>> @@ -80,9 +100,16 @@
>>> #define SUN4I_GPADC_INT_FIFOS_TP_UP_PENDING BIT(1)
>>> #define SUN4I_GPADC_INT_FIFOS_TP_DOWN_PENDING BIT(0)
>>>
>>> +#define SUN8I_H3_GPADC_INTS 0x44
>>
>> 0x48
>>
>> [...]
>>
>> 1) You're not using irqs, why would you define registers that will never
>> be used?
>
> I will then rework it to use IRQs, but not now.
>
> Maybe I should add it when I use them?
>
Why not make it work right away the way we want :)?
>>
>> 2) Why aren't you using irqs? I remember we discussed on IRC that you
>> had some problems with the H3 when resuming or when probing the driver.
>> The register would have a zero in it until you have a first sample that
>> arrived (i.e. after the sample rate you set with T_ACQ) that would make
>> the thermal framework panic since the thermal sensor would return
>> something way too hot and shutdown your board?
>
> Nope, it's another problem -- the runtime resume function is even not
> called before the first sample, and the first sample will happen when
> the THS is still suspended.
>
As discussed on IRC (a long time ago :) ), it's a combination of two
problems:
1) get_temp (used by thermal framework) uses pm_runtime function that
isn't ready yet <= I will send a patch for registering thermal framework
after pm_runtime to you, hopefully in the upcoming hour or tomorrow,
2) The A33 (and H3 in your implementation) does not wait for an
interrupt to read the TEMP_DATA register which resets to 0 when the
sensor is disabled (or until a first sample arrives) i.e. when probing
or when resuming. Using IRQs would get rid of 2). It isn't critical for
A33 as the formula for temp returns something really cold so thermal
does not care. But for the H3, it's critically hot and shuts down your
board. To make it work on the H3, we would have to use a delay in the
pm_resume function equal to the sensor sampling rate and I don't really
like that. If we could use IRQs, it'd be better IMHO (but they aren't
working on A33).
Thanks,
Quentin
>>
>> The H3 apparently supports IRQs, why do you not support them for the
>> temperature? They might be broken as it is on A33 but then it might be a
>> good idea to write it down in a comment in the driver (and not adding
>> the unused registers in the header file) or at least in the commit log.
>>
>> 3) Now that you have support for clocks, wouldn't it be a good idea to
>> disable them during suspend?
>
> Interesting... It's meaningful to disable the mod clock during suspend.
>
>>
>> Thanks,
>> Quentin
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
--
Quentin Schulz, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v4 1/6] dt-bindings: update the Allwinner GPADC device tree binding for H3
2017-09-14 14:52 ` [PATCH v4 1/6] dt-bindings: update the Allwinner GPADC device tree binding for H3 Icenowy Zheng
@ 2017-09-16 22:12 ` Jonathan Cameron
2017-09-18 7:33 ` Maxime Ripard
1 sibling, 0 replies; 28+ messages in thread
From: Jonathan Cameron @ 2017-09-16 22:12 UTC (permalink / raw)
To: Icenowy Zheng
Cc: Lee Jones, Rob Herring, Maxime Ripard, Chen-Yu Tsai,
Quentin Schulz, devicetree, linux-arm-kernel, linux-kernel,
linux-iio, linux-sunxi
On Thu, 14 Sep 2017 22:52:46 +0800
Icenowy Zheng <icenowy@aosc.io> wrote:
> Allwinner H3 features a thermal sensor like the one in A33, but has its
> register re-arranged, the clock divider moved to CCU (originally the
> clock divider is in ADC) and added a pair of bus clock and reset.
>
> Update the binding document to cover H3.
>
> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
> Reviewed-by: Chen-Yu Tsai <wens@csie.org>
> ---
> Changes in v4:
> - Add nvmem calibration data (not yet used by the driver)
> Changes in v3:
> - Clock name changes.
> - Example node name changes.
> - Add interupts (not yet used by the driver).
>
> .../devicetree/bindings/mfd/sun4i-gpadc.txt | 30 ++++++++++++++++++++--
> 1 file changed, 28 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/mfd/sun4i-gpadc.txt b/Documentation/devicetree/bindings/mfd/sun4i-gpadc.txt
> index badff3611a98..6c470d584bf9 100644
> --- a/Documentation/devicetree/bindings/mfd/sun4i-gpadc.txt
> +++ b/Documentation/devicetree/bindings/mfd/sun4i-gpadc.txt
> @@ -4,12 +4,26 @@ The Allwinner SoCs all have an ADC that can also act as a thermal sensor
> and sometimes as a touchscreen controller.
>
> Required properties:
> - - compatible: "allwinner,sun8i-a33-ths",
> + - compatible: must contain one of the following compatibles:
> + - "allwinner,sun8i-a33-ths"
> + - "allwinner,sun8i-h3-ths"
> - reg: mmio address range of the chip,
> - #thermal-sensor-cells: shall be 0,
> - #io-channel-cells: shall be 0,
>
> -Example:
> +Optional properties:
> + - nvmem-cells: A phandle to the calibration data provided by a nvmem device.
> + If unspecified default values shall be used.
> + - nvmem-cell-names: Should be "calibration-data"
I think a cross reference to the nvmem binding docs would be good here.
It wasn't something I could remember coming across before. Obviously
grep gets you there quickly enough, but a cross reference would be even
better.
Also would it make sense to have an example with these in?
Jonathan
> +
> +Required properties for the following compatibles:
> + - "allwinner,sun8i-h3-ths"
> + - clocks: the bus clock and the input clock of the ADC,
> + - clock-names: should be "bus" and "mod",
> + - resets: the bus reset of the ADC,
> + - interrupts: the sampling interrupt of the ADC,
> +
> +Example for A33:
> ths: ths@01c25000 {
> compatible = "allwinner,sun8i-a33-ths";
> reg = <0x01c25000 0x100>;
> @@ -17,6 +31,18 @@ Example:
> #io-channel-cells = <0>;
> };
>
> +Example for H3:
> + ths: thermal-sensor@1c25000 {
> + compatible = "allwinner,sun8i-h3-ths";
> + reg = <0x01c25000 0x400>;
> + clocks = <&ccu CLK_BUS_THS>, <&ccu CLK_THS>;
> + clock-names = "bus", "mod";
> + resets = <&ccu RST_BUS_THS>;
> + interrupts = <GIC_SPI 31 IRQ_TYPE_LEVEL_HIGH>;
> + #thermal-sensor-cells = <0>;
> + #io-channel-cells = <0>;
> + };
> +
> sun4i, sun5i and sun6i SoCs are also supported via the older binding:
>
> sun4i resistive touchscreen controller
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v4 4/6] iio: adc: sun4i-gpadc-iio: add support for H3 thermal sensor
2017-09-16 9:45 ` Quentin Schulz
2017-09-16 10:14 ` icenowy
@ 2017-09-16 22:16 ` Jonathan Cameron
1 sibling, 0 replies; 28+ messages in thread
From: Jonathan Cameron @ 2017-09-16 22:16 UTC (permalink / raw)
To: Quentin Schulz
Cc: Icenowy Zheng, Lee Jones, Rob Herring, Maxime Ripard,
Chen-Yu Tsai, devicetree, linux-iio, linux-kernel, linux-sunxi,
linux-arm-kernel
On Sat, 16 Sep 2017 11:45:47 +0200
Quentin Schulz <quentin.schulz@free-electrons.com> wrote:
> Hi Icenowy,
>
> On 14/09/2017 16:52, Icenowy Zheng wrote:
> > This adds support for the Allwinner H3 thermal sensor.
> >
> > Allwinner H3 has a thermal sensor like the one in A33, but have its
> > registers nearly all re-arranged, sample clock moved to CCU and a pair
> > of bus clock and reset added. It's also the base of newer SoCs' thermal
> > sensors.
> >
> > The thermal sensors on A64 and H5 is like the one on H3, but with of
> > course different formula factors.
> >
> > Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
> > ---
> > Changes in v4:
> > - Splitted out some code refactors.
> > - Code sequence changed back. (The gpadc_data went back to the start of
> > the source file)
> >
> > drivers/iio/adc/sun4i-gpadc-iio.c | 48 +++++++++++++++++++++++++++++++++++++++
> > include/linux/mfd/sun4i-gpadc.h | 27 ++++++++++++++++++++++
> > 2 files changed, 75 insertions(+)
> [...]
> > diff --git a/include/linux/mfd/sun4i-gpadc.h b/include/linux/mfd/sun4i-gpadc.h
> > index 78d31984a222..5c2a12101052 100644
> > --- a/include/linux/mfd/sun4i-gpadc.h
> > +++ b/include/linux/mfd/sun4i-gpadc.h
> [...]
> > +#define SUN8I_H3_GPADC_CTRL2_T_ACQ1(x) ((GENMASK(15, 0) * (x)) << 16)
> > +
>
> You want to replace * by &.
>
> ((GENMASK(15, 0) & (x)) << 16)
>
> Would ((GENMASK(31, 16) & ((x) << 16)) make the bits you set even more
> obvious?
Agreed. Would act as better 'documentation'.
Jonathan
>
> > #define SUN4I_GPADC_CTRL3 0x0c
> > +/*
> > + * This register is named "Average filter Control Register" in H3 Datasheet,
> > + * but the register's definition is the same as the old CTRL3 register.
> > + */
> > +#define SUN8I_H3_GPADC_CTRL3 0x70
> >
>
> I would name it as it is in the documentation:
> SUN8I_H3_THS_FILTER
>
> No need for comments then.
>
> > #define SUN4I_GPADC_CTRL3_FILTER_EN BIT(2)
> > #define SUN4I_GPADC_CTRL3_FILTER_TYPE(x) (GENMASK(1, 0) & (x))
> > @@ -71,6 +84,13 @@
> > #define SUN4I_GPADC_INT_FIFOC_TP_UP_IRQ_EN BIT(1)
> > #define SUN4I_GPADC_INT_FIFOC_TP_DOWN_IRQ_EN BIT(0)
> >
> > +#define SUN8I_H3_GPADC_INTC 0x44
> > +
> > +#define SUN8I_H3_GPADC_INTC_TEMP_PERIOD(x) ((GENMASK(19, 0) & (x)) << 12)
> > +#define SUN8I_H3_GPADC_INTC_TEMP_DATA BIT(8)
> > +#define SUN8I_H3_GPADC_INTC_TEMP_SHUT BIT(4)
> > +#define SUN8I_H3_GPADC_INTC_TEMP_ALARM BIT(0)
> > +
>
> Since it isn't an ADC anymore but rather just a THS, why don't you use
> SUN8I_H3_THS instead of SUN8I_H3_GPADC? That way, it also matches the
> datasheet.
>
> > #define SUN4I_GPADC_INT_FIFOS 0x14
> >
> > #define SUN4I_GPADC_INT_FIFOS_TEMP_DATA_PENDING BIT(18)
> > @@ -80,9 +100,16 @@
> > #define SUN4I_GPADC_INT_FIFOS_TP_UP_PENDING BIT(1)
> > #define SUN4I_GPADC_INT_FIFOS_TP_DOWN_PENDING BIT(0)
> >
> > +#define SUN8I_H3_GPADC_INTS 0x44
>
> 0x48
>
> [...]
>
> 1) You're not using irqs, why would you define registers that will never
> be used?
>
> 2) Why aren't you using irqs? I remember we discussed on IRC that you
> had some problems with the H3 when resuming or when probing the driver.
> The register would have a zero in it until you have a first sample that
> arrived (i.e. after the sample rate you set with T_ACQ) that would make
> the thermal framework panic since the thermal sensor would return
> something way too hot and shutdown your board?
>
> The H3 apparently supports IRQs, why do you not support them for the
> temperature? They might be broken as it is on A33 but then it might be a
> good idea to write it down in a comment in the driver (and not adding
> the unused registers in the header file) or at least in the commit log.
>
> 3) Now that you have support for clocks, wouldn't it be a good idea to
> disable them during suspend?
>
> Thanks,
> Quentin
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v4 6/6] ARM: sun8i: h3: add partial CPU thermal zone
2017-09-16 10:05 ` Quentin Schulz
@ 2017-09-16 22:17 ` Jonathan Cameron
2017-09-18 8:27 ` Maxime Ripard
0 siblings, 1 reply; 28+ messages in thread
From: Jonathan Cameron @ 2017-09-16 22:17 UTC (permalink / raw)
To: Quentin Schulz
Cc: Icenowy Zheng, Lee Jones, Rob Herring, Maxime Ripard,
Chen-Yu Tsai, devicetree, linux-arm-kernel, linux-kernel,
linux-iio, linux-sunxi
On Sat, 16 Sep 2017 12:05:49 +0200
Quentin Schulz <quentin.schulz@free-electrons.com> wrote:
> Hi Icenowy,
>
> On 14/09/2017 16:52, Icenowy Zheng wrote:
> > Because of the restriction of the OF thermal framework, the thermal
> > sensor will fail to probe if the thermal zone doesn't exist.
> >
>
> Oh no, that's not good.
>
> We discussed about it on IRC and I even proposed a patch for it, telling
> you I would post it on the mailing list soon after. Of course, I forgot
> and you definitely should have yelled at me for not doing it :)
>
> I won't be able to test the patch soon. I can send it to you so that you
> can test it and integrate it in your patch series so it won't block you.
> Otherwise, we'll have to wait for a week or two for me to test it.
>
> Thanks and sorry for forgetting to post the patch you need,
> Quentin
Other this outstanding issue I'm happy with the series, so hopefully
with Quentin's patch added we should be good to merge this one.
Jonathan
>
> > Add a partial thermal zone which claims the H3 THS as the thermal sensor.
> >
> > The cooling device (CPU DVFS) is still not added as it's not ready, and
> > the trip points are also not added yet.
> >
> > Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
> > ---
> > arch/arm/boot/dts/sun8i-h3.dtsi | 9 +++++++++
> > 1 file changed, 9 insertions(+)
> >
> > diff --git a/arch/arm/boot/dts/sun8i-h3.dtsi b/arch/arm/boot/dts/sun8i-h3.dtsi
> > index 3220da3ad790..687c6457d214 100644
> > --- a/arch/arm/boot/dts/sun8i-h3.dtsi
> > +++ b/arch/arm/boot/dts/sun8i-h3.dtsi
> > @@ -89,6 +89,15 @@
> > };
> > };
> >
> > + thermal-zones {
> > + cpu-thermal {
> > + /* milliseconds */
> > + polling-delay-passive = <250>;
> > + polling-delay = <1000>;
> > + thermal-sensors = <&ths>;
> > + };
> > + };
> > +
> > timer {
> > compatible = "arm,armv7-timer";
> > interrupts = <GIC_PPI 13 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
> >
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v4 1/6] dt-bindings: update the Allwinner GPADC device tree binding for H3
2017-09-14 14:52 ` [PATCH v4 1/6] dt-bindings: update the Allwinner GPADC device tree binding for H3 Icenowy Zheng
2017-09-16 22:12 ` Jonathan Cameron
@ 2017-09-18 7:33 ` Maxime Ripard
2017-09-18 7:36 ` Icenowy Zheng
1 sibling, 1 reply; 28+ messages in thread
From: Maxime Ripard @ 2017-09-18 7:33 UTC (permalink / raw)
To: Icenowy Zheng
Cc: Lee Jones, Rob Herring, Chen-Yu Tsai, Jonathan Cameron,
Quentin Schulz, devicetree, linux-arm-kernel, linux-kernel,
linux-iio, linux-sunxi
[-- Attachment #1: Type: text/plain, Size: 2632 bytes --]
On Thu, Sep 14, 2017 at 10:52:46PM +0800, Icenowy Zheng wrote:
> Allwinner H3 features a thermal sensor like the one in A33, but has its
> register re-arranged, the clock divider moved to CCU (originally the
> clock divider is in ADC) and added a pair of bus clock and reset.
>
> Update the binding document to cover H3.
>
> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
> Reviewed-by: Chen-Yu Tsai <wens@csie.org>
> ---
> Changes in v4:
> - Add nvmem calibration data (not yet used by the driver)
> Changes in v3:
> - Clock name changes.
> - Example node name changes.
> - Add interupts (not yet used by the driver).
>
> .../devicetree/bindings/mfd/sun4i-gpadc.txt | 30 ++++++++++++++++++++--
> 1 file changed, 28 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/mfd/sun4i-gpadc.txt b/Documentation/devicetree/bindings/mfd/sun4i-gpadc.txt
> index badff3611a98..6c470d584bf9 100644
> --- a/Documentation/devicetree/bindings/mfd/sun4i-gpadc.txt
> +++ b/Documentation/devicetree/bindings/mfd/sun4i-gpadc.txt
> @@ -4,12 +4,26 @@ The Allwinner SoCs all have an ADC that can also act as a thermal sensor
> and sometimes as a touchscreen controller.
>
> Required properties:
> - - compatible: "allwinner,sun8i-a33-ths",
> + - compatible: must contain one of the following compatibles:
> + - "allwinner,sun8i-a33-ths"
> + - "allwinner,sun8i-h3-ths"
> - reg: mmio address range of the chip,
> - #thermal-sensor-cells: shall be 0,
> - #io-channel-cells: shall be 0,
>
> -Example:
> +Optional properties:
> + - nvmem-cells: A phandle to the calibration data provided by a nvmem device.
> + If unspecified default values shall be used.
> + - nvmem-cell-names: Should be "calibration-data"
I'd prefer to have which sensor it applies to here. It wouldn't change
anything for the H3, but it definitely does for example for the A83t
that has two sensors, one for each cluster, and one for the GPU, each
with calibration data.
What about cluster0-calibration?
> +
> +Required properties for the following compatibles:
> + - "allwinner,sun8i-h3-ths"
> + - clocks: the bus clock and the input clock of the ADC,
> + - clock-names: should be "bus" and "mod",
> + - resets: the bus reset of the ADC,
> + - interrupts: the sampling interrupt of the ADC,
For resets and interrupts, you should list all of them. If there's
only one, then there's no point telling which one it is.
Thanks,
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v4 2/6] iio: adc: sun4i-gpadc-iio: rename A33-specified registers to contain A33
2017-09-14 14:52 ` [PATCH v4 2/6] iio: adc: sun4i-gpadc-iio: rename A33-specified registers to contain A33 Icenowy Zheng
@ 2017-09-18 7:34 ` Maxime Ripard
2017-09-18 8:29 ` Lee Jones
1 sibling, 0 replies; 28+ messages in thread
From: Maxime Ripard @ 2017-09-18 7:34 UTC (permalink / raw)
To: Icenowy Zheng
Cc: Lee Jones, Rob Herring, Chen-Yu Tsai, Jonathan Cameron,
Quentin Schulz, devicetree, linux-arm-kernel, linux-kernel,
linux-iio, linux-sunxi
[-- Attachment #1: Type: text/plain, Size: 724 bytes --]
On Thu, Sep 14, 2017 at 10:52:47PM +0800, Icenowy Zheng wrote:
> As the H3 SoC, which is also in sun8i line, has totally different
> register map for the thermal sensor (a cut down version of GPADC), we
> should rename A23/A33-specified registers to contain A33, in order to
> prevent obfuscation with H3 registers. Currently these registers are
> only prefixed "SUN8I", not "SUN8I_A33".
>
> Add "_A33" after "SUN8I" on the register names.
>
> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
> Reviewed-by: Chen-Yu Tsai <wens@csie.org>
Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com>
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v4 3/6] iio: adc: sun4i-gpadc-iio: rework code for supporting newer THS variants
2017-09-14 14:52 ` [PATCH v4 3/6] iio: adc: sun4i-gpadc-iio: rework code for supporting newer THS variants Icenowy Zheng
@ 2017-09-18 7:36 ` Maxime Ripard
0 siblings, 0 replies; 28+ messages in thread
From: Maxime Ripard @ 2017-09-18 7:36 UTC (permalink / raw)
To: Icenowy Zheng
Cc: Lee Jones, Rob Herring, Chen-Yu Tsai, Jonathan Cameron,
Quentin Schulz, devicetree, linux-arm-kernel, linux-kernel,
linux-iio, linux-sunxi
[-- Attachment #1: Type: text/plain, Size: 625 bytes --]
On Thu, Sep 14, 2017 at 10:52:48PM +0800, Icenowy Zheng wrote:
> The SoCs after H3 has newer thermal sensor ADCs, which have two clock
> inputs (bus clock and sampling clock) and a reset. The registers are
> also re-arranged.
>
> This commit reworks the code, adds the process of the clocks and
> resets, and allows the sampling start/end code and the position of value
> readout register to be altered.
>
> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
Please split that into separate commits.
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v4 1/6] dt-bindings: update the Allwinner GPADC device tree binding for H3
2017-09-18 7:33 ` Maxime Ripard
@ 2017-09-18 7:36 ` Icenowy Zheng
2017-09-18 8:30 ` Maxime Ripard
0 siblings, 1 reply; 28+ messages in thread
From: Icenowy Zheng @ 2017-09-18 7:36 UTC (permalink / raw)
To: Maxime Ripard
Cc: Lee Jones, Rob Herring, Chen-Yu Tsai, Jonathan Cameron,
Quentin Schulz, devicetree, linux-arm-kernel, linux-kernel,
linux-iio, linux-sunxi
于 2017年9月18日 GMT+08:00 下午3:33:36, Maxime Ripard <maxime.ripard@free-electrons.com> 写到:
>On Thu, Sep 14, 2017 at 10:52:46PM +0800, Icenowy Zheng wrote:
>> Allwinner H3 features a thermal sensor like the one in A33, but has
>its
>> register re-arranged, the clock divider moved to CCU (originally the
>> clock divider is in ADC) and added a pair of bus clock and reset.
>>
>> Update the binding document to cover H3.
>>
>> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
>> Reviewed-by: Chen-Yu Tsai <wens@csie.org>
>> ---
>> Changes in v4:
>> - Add nvmem calibration data (not yet used by the driver)
>> Changes in v3:
>> - Clock name changes.
>> - Example node name changes.
>> - Add interupts (not yet used by the driver).
>>
>> .../devicetree/bindings/mfd/sun4i-gpadc.txt | 30
>++++++++++++++++++++--
>> 1 file changed, 28 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/mfd/sun4i-gpadc.txt
>b/Documentation/devicetree/bindings/mfd/sun4i-gpadc.txt
>> index badff3611a98..6c470d584bf9 100644
>> --- a/Documentation/devicetree/bindings/mfd/sun4i-gpadc.txt
>> +++ b/Documentation/devicetree/bindings/mfd/sun4i-gpadc.txt
>> @@ -4,12 +4,26 @@ The Allwinner SoCs all have an ADC that can also
>act as a thermal sensor
>> and sometimes as a touchscreen controller.
>>
>> Required properties:
>> - - compatible: "allwinner,sun8i-a33-ths",
>> + - compatible: must contain one of the following compatibles:
>> + - "allwinner,sun8i-a33-ths"
>> + - "allwinner,sun8i-h3-ths"
>> - reg: mmio address range of the chip,
>> - #thermal-sensor-cells: shall be 0,
>> - #io-channel-cells: shall be 0,
>>
>> -Example:
>> +Optional properties:
>> + - nvmem-cells: A phandle to the calibration data provided by a
>nvmem device.
>> + If unspecified default values shall be used.
>> + - nvmem-cell-names: Should be "calibration-data"
>
>I'd prefer to have which sensor it applies to here. It wouldn't change
>anything for the H3, but it definitely does for example for the A83t
>that has two sensors, one for each cluster, and one for the GPU, each
>with calibration data.
>
>What about cluster0-calibration?
The calibration data is in fact a 2 word (8 bytes) zone,
which is reserved for 4 sensors on all SoCs, even on H3.
It's half word per sensor.
I prefer to just assume a 2 word cell for every SoC.
>
>> +
>> +Required properties for the following compatibles:
>> + - "allwinner,sun8i-h3-ths"
>> + - clocks: the bus clock and the input clock of the ADC,
>> + - clock-names: should be "bus" and "mod",
>> + - resets: the bus reset of the ADC,
>> + - interrupts: the sampling interrupt of the ADC,
>
>For resets and interrupts, you should list all of them. If there's
>only one, then there's no point telling which one it is.
>
>
>Thanks,
>Maxime
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v4 4/6] iio: adc: sun4i-gpadc-iio: add support for H3 thermal sensor
2017-09-16 10:14 ` icenowy
2017-09-16 10:35 ` Quentin Schulz
@ 2017-09-18 8:24 ` Maxime Ripard
1 sibling, 0 replies; 28+ messages in thread
From: Maxime Ripard @ 2017-09-18 8:24 UTC (permalink / raw)
To: icenowy
Cc: Quentin Schulz, Lee Jones, Rob Herring, Chen-Yu Tsai,
Jonathan Cameron, devicetree, linux-iio, linux-kernel,
linux-sunxi, linux-arm-kernel
[-- Attachment #1: Type: text/plain, Size: 740 bytes --]
On Sat, Sep 16, 2017 at 06:14:08PM +0800, icenowy@aosc.io wrote:
> > The H3 apparently supports IRQs, why do you not support them for the
> > temperature? They might be broken as it is on A33 but then it might be a
> > good idea to write it down in a comment in the driver (and not adding
> > the unused registers in the header file) or at least in the commit log.
> >
> > 3) Now that you have support for clocks, wouldn't it be a good idea to
> > disable them during suspend?
>
> Interesting... It's meaningful to disable the mod clock during suspend.
All clocks, actually. And put the device back into reset.
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v4 5/6] ARM: sun8i: h3: add support for the thermal sensor in H3
2017-09-14 14:52 ` [PATCH v4 5/6] ARM: sun8i: h3: add support for the thermal sensor in H3 Icenowy Zheng
@ 2017-09-18 8:25 ` Maxime Ripard
0 siblings, 0 replies; 28+ messages in thread
From: Maxime Ripard @ 2017-09-18 8:25 UTC (permalink / raw)
To: Icenowy Zheng
Cc: Lee Jones, Rob Herring, Chen-Yu Tsai, Jonathan Cameron,
Quentin Schulz, devicetree, linux-arm-kernel, linux-kernel,
linux-iio, linux-sunxi
[-- Attachment #1: Type: text/plain, Size: 1603 bytes --]
On Thu, Sep 14, 2017 at 10:52:50PM +0800, Icenowy Zheng wrote:
> As we have gained the support for the thermal sensor in H3, we can now
> add its device nodes to the device tree.
>
> Add them to the H3 device tree.
>
> The calibration data of the thermal sensor is still not added, as
> it's currently not used, and the SID node is not added yet.
>
> The H5 thermal sensor has some differences, and will be added furtherly.
>
> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
> Reviewed-by: Chen-Yu Tsai <wens@csie.org>
> ---
> Changes in v4:
> - Mention calibration data in commit message.
> Changes in v3:
> - Clock name changes.
> - Splited out thermal zone addition.
>
> arch/arm/boot/dts/sun8i-h3.dtsi | 17 +++++++++++++++++
> 1 file changed, 17 insertions(+)
>
> diff --git a/arch/arm/boot/dts/sun8i-h3.dtsi b/arch/arm/boot/dts/sun8i-h3.dtsi
> index b36f9f423c39..3220da3ad790 100644
> --- a/arch/arm/boot/dts/sun8i-h3.dtsi
> +++ b/arch/arm/boot/dts/sun8i-h3.dtsi
> @@ -72,6 +72,23 @@
> };
> };
>
> + iio-hwmon {
> + compatible = "iio-hwmon";
> + io-channels = <&ths>;
> + };
> +
> + soc {
> + ths: thermal-sensor@1c25000 {
> + compatible = "allwinner,sun8i-h3-ths";
> + reg = <0x01c25000 0x100>;
> + clocks = <&ccu CLK_BUS_THS>, <&ccu CLK_THS>;
> + clock-names = "bus", "mod";
> + resets = <&ccu RST_BUS_THS>;
> + #thermal-sensor-cells = <0>;
> + #io-channel-cells = <0>;
You're missing your interrupt.
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v4 6/6] ARM: sun8i: h3: add partial CPU thermal zone
2017-09-16 22:17 ` Jonathan Cameron
@ 2017-09-18 8:27 ` Maxime Ripard
2017-09-24 14:23 ` Jonathan Cameron
0 siblings, 1 reply; 28+ messages in thread
From: Maxime Ripard @ 2017-09-18 8:27 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Quentin Schulz, Icenowy Zheng, Lee Jones, Rob Herring,
Chen-Yu Tsai, devicetree, linux-arm-kernel, linux-kernel,
linux-iio, linux-sunxi
[-- Attachment #1: Type: text/plain, Size: 1280 bytes --]
Hi Jonathan,
On Sat, Sep 16, 2017 at 03:17:34PM -0700, Jonathan Cameron wrote:
> On Sat, 16 Sep 2017 12:05:49 +0200
> Quentin Schulz <quentin.schulz@free-electrons.com> wrote:
>
> > Hi Icenowy,
> >
> > On 14/09/2017 16:52, Icenowy Zheng wrote:
> > > Because of the restriction of the OF thermal framework, the thermal
> > > sensor will fail to probe if the thermal zone doesn't exist.
> > >
> >
> > Oh no, that's not good.
> >
> > We discussed about it on IRC and I even proposed a patch for it, telling
> > you I would post it on the mailing list soon after. Of course, I forgot
> > and you definitely should have yelled at me for not doing it :)
> >
> > I won't be able to test the patch soon. I can send it to you so that you
> > can test it and integrate it in your patch series so it won't block you.
> > Otherwise, we'll have to wait for a week or two for me to test it.
> >
> > Thanks and sorry for forgetting to post the patch you need,
> > Quentin
>
> Other this outstanding issue I'm happy with the series, so hopefully
> with Quentin's patch added we should be good to merge this one.
We will at least need a v5.
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v4 2/6] iio: adc: sun4i-gpadc-iio: rename A33-specified registers to contain A33
2017-09-14 14:52 ` [PATCH v4 2/6] iio: adc: sun4i-gpadc-iio: rename A33-specified registers to contain A33 Icenowy Zheng
2017-09-18 7:34 ` Maxime Ripard
@ 2017-09-18 8:29 ` Lee Jones
1 sibling, 0 replies; 28+ messages in thread
From: Lee Jones @ 2017-09-18 8:29 UTC (permalink / raw)
To: Icenowy Zheng
Cc: Rob Herring, Maxime Ripard, Chen-Yu Tsai, Jonathan Cameron,
Quentin Schulz, devicetree, linux-arm-kernel, linux-kernel,
linux-iio, linux-sunxi
On Thu, 14 Sep 2017, Icenowy Zheng wrote:
> As the H3 SoC, which is also in sun8i line, has totally different
> register map for the thermal sensor (a cut down version of GPADC), we
> should rename A23/A33-specified registers to contain A33, in order to
> prevent obfuscation with H3 registers. Currently these registers are
> only prefixed "SUN8I", not "SUN8I_A33".
>
> Add "_A33" after "SUN8I" on the register names.
>
> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
> Reviewed-by: Chen-Yu Tsai <wens@csie.org>
> ---
> Changes in v4:
> - Change A23 to A33, as the driver never supports A23.
>
> drivers/iio/adc/sun4i-gpadc-iio.c | 2 +-
> include/linux/mfd/sun4i-gpadc.h | 6 +++---
Acked-by: Lee Jones <lee.jones@linaro.org>
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v4 1/6] dt-bindings: update the Allwinner GPADC device tree binding for H3
2017-09-18 7:36 ` Icenowy Zheng
@ 2017-09-18 8:30 ` Maxime Ripard
2017-09-18 15:47 ` [linux-sunxi] " icenowy
0 siblings, 1 reply; 28+ messages in thread
From: Maxime Ripard @ 2017-09-18 8:30 UTC (permalink / raw)
To: Icenowy Zheng
Cc: Lee Jones, Rob Herring, Chen-Yu Tsai, Jonathan Cameron,
Quentin Schulz, devicetree, linux-arm-kernel, linux-kernel,
linux-iio, linux-sunxi
[-- Attachment #1: Type: text/plain, Size: 2948 bytes --]
On Mon, Sep 18, 2017 at 03:36:43PM +0800, Icenowy Zheng wrote:
> 于 2017年9月18日 GMT+08:00 下午3:33:36, Maxime Ripard <maxime.ripard@free-electrons.com> 写到:
> >On Thu, Sep 14, 2017 at 10:52:46PM +0800, Icenowy Zheng wrote:
> >> Allwinner H3 features a thermal sensor like the one in A33, but has
> >its
> >> register re-arranged, the clock divider moved to CCU (originally the
> >> clock divider is in ADC) and added a pair of bus clock and reset.
> >>
> >> Update the binding document to cover H3.
> >>
> >> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
> >> Reviewed-by: Chen-Yu Tsai <wens@csie.org>
> >> ---
> >> Changes in v4:
> >> - Add nvmem calibration data (not yet used by the driver)
> >> Changes in v3:
> >> - Clock name changes.
> >> - Example node name changes.
> >> - Add interupts (not yet used by the driver).
> >>
> >> .../devicetree/bindings/mfd/sun4i-gpadc.txt | 30
> >++++++++++++++++++++--
> >> 1 file changed, 28 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/Documentation/devicetree/bindings/mfd/sun4i-gpadc.txt
> >b/Documentation/devicetree/bindings/mfd/sun4i-gpadc.txt
> >> index badff3611a98..6c470d584bf9 100644
> >> --- a/Documentation/devicetree/bindings/mfd/sun4i-gpadc.txt
> >> +++ b/Documentation/devicetree/bindings/mfd/sun4i-gpadc.txt
> >> @@ -4,12 +4,26 @@ The Allwinner SoCs all have an ADC that can also
> >act as a thermal sensor
> >> and sometimes as a touchscreen controller.
> >>
> >> Required properties:
> >> - - compatible: "allwinner,sun8i-a33-ths",
> >> + - compatible: must contain one of the following compatibles:
> >> + - "allwinner,sun8i-a33-ths"
> >> + - "allwinner,sun8i-h3-ths"
> >> - reg: mmio address range of the chip,
> >> - #thermal-sensor-cells: shall be 0,
> >> - #io-channel-cells: shall be 0,
> >>
> >> -Example:
> >> +Optional properties:
> >> + - nvmem-cells: A phandle to the calibration data provided by a
> >nvmem device.
> >> + If unspecified default values shall be used.
> >> + - nvmem-cell-names: Should be "calibration-data"
> >
> >I'd prefer to have which sensor it applies to here. It wouldn't change
> >anything for the H3, but it definitely does for example for the A83t
> >that has two sensors, one for each cluster, and one for the GPU, each
> >with calibration data.
> >
> >What about cluster0-calibration?
>
> The calibration data is in fact a 2 word (8 bytes) zone,
> which is reserved for 4 sensors on all SoCs, even on H3.
> It's half word per sensor.
>
> I prefer to just assume a 2 word cell for every SoC.
You have three different data sources, it should be reprensented as
such.
Otherwise, the client has to get some knowledge of how the data are
stored in the provider, which is an abstraction violation.
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [linux-sunxi] Re: [PATCH v4 1/6] dt-bindings: update the Allwinner GPADC device tree binding for H3
2017-09-18 8:30 ` Maxime Ripard
@ 2017-09-18 15:47 ` icenowy
2017-09-20 7:52 ` Maxime Ripard
0 siblings, 1 reply; 28+ messages in thread
From: icenowy @ 2017-09-18 15:47 UTC (permalink / raw)
To: maxime.ripard
Cc: Lee Jones, Rob Herring, Chen-Yu Tsai, Jonathan Cameron,
Quentin Schulz, devicetree, linux-arm-kernel, linux-kernel,
linux-iio, linux-sunxi
在 2017-09-18 16:30,Maxime Ripard 写道:
> On Mon, Sep 18, 2017 at 03:36:43PM +0800, Icenowy Zheng wrote:
>> 于 2017年9月18日 GMT+08:00 下午3:33:36, Maxime Ripard
>> <maxime.ripard@free-electrons.com> 写到:
>> >On Thu, Sep 14, 2017 at 10:52:46PM +0800, Icenowy Zheng wrote:
>> >> Allwinner H3 features a thermal sensor like the one in A33, but has
>> >its
>> >> register re-arranged, the clock divider moved to CCU (originally the
>> >> clock divider is in ADC) and added a pair of bus clock and reset.
>> >>
>> >> Update the binding document to cover H3.
>> >>
>> >> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
>> >> Reviewed-by: Chen-Yu Tsai <wens@csie.org>
>> >> ---
>> >> Changes in v4:
>> >> - Add nvmem calibration data (not yet used by the driver)
>> >> Changes in v3:
>> >> - Clock name changes.
>> >> - Example node name changes.
>> >> - Add interupts (not yet used by the driver).
>> >>
>> >> .../devicetree/bindings/mfd/sun4i-gpadc.txt | 30
>> >++++++++++++++++++++--
>> >> 1 file changed, 28 insertions(+), 2 deletions(-)
>> >>
>> >> diff --git a/Documentation/devicetree/bindings/mfd/sun4i-gpadc.txt
>> >b/Documentation/devicetree/bindings/mfd/sun4i-gpadc.txt
>> >> index badff3611a98..6c470d584bf9 100644
>> >> --- a/Documentation/devicetree/bindings/mfd/sun4i-gpadc.txt
>> >> +++ b/Documentation/devicetree/bindings/mfd/sun4i-gpadc.txt
>> >> @@ -4,12 +4,26 @@ The Allwinner SoCs all have an ADC that can also
>> >act as a thermal sensor
>> >> and sometimes as a touchscreen controller.
>> >>
>> >> Required properties:
>> >> - - compatible: "allwinner,sun8i-a33-ths",
>> >> + - compatible: must contain one of the following compatibles:
>> >> + - "allwinner,sun8i-a33-ths"
>> >> + - "allwinner,sun8i-h3-ths"
>> >> - reg: mmio address range of the chip,
>> >> - #thermal-sensor-cells: shall be 0,
>> >> - #io-channel-cells: shall be 0,
>> >>
>> >> -Example:
>> >> +Optional properties:
>> >> + - nvmem-cells: A phandle to the calibration data provided by a
>> >nvmem device.
>> >> + If unspecified default values shall be used.
>> >> + - nvmem-cell-names: Should be "calibration-data"
>> >
>> >I'd prefer to have which sensor it applies to here. It wouldn't change
>> >anything for the H3, but it definitely does for example for the A83t
>> >that has two sensors, one for each cluster, and one for the GPU, each
>> >with calibration data.
>> >
>> >What about cluster0-calibration?
I prefer sensor0-calibration to sensor3-calibration now.
(Theortically the new generation THS can support up to 4 sensors)
>>
>> The calibration data is in fact a 2 word (8 bytes) zone,
>> which is reserved for 4 sensors on all SoCs, even on H3.
>> It's half word per sensor.
>>
>> I prefer to just assume a 2 word cell for every SoC.
>
> You have three different data sources, it should be reprensented as
> such.
>
> Otherwise, the client has to get some knowledge of how the data are
> stored in the provider, which is an abstraction violation.
>
> Maxime
>
> --
> Maxime Ripard, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [linux-sunxi] Re: [PATCH v4 1/6] dt-bindings: update the Allwinner GPADC device tree binding for H3
2017-09-18 15:47 ` [linux-sunxi] " icenowy
@ 2017-09-20 7:52 ` Maxime Ripard
2017-09-20 8:04 ` Icenowy Zheng
0 siblings, 1 reply; 28+ messages in thread
From: Maxime Ripard @ 2017-09-20 7:52 UTC (permalink / raw)
To: icenowy
Cc: Lee Jones, Rob Herring, Chen-Yu Tsai, Jonathan Cameron,
Quentin Schulz, devicetree, linux-arm-kernel, linux-kernel,
linux-iio, linux-sunxi
[-- Attachment #1: Type: text/plain, Size: 3140 bytes --]
On Mon, Sep 18, 2017 at 03:47:25PM +0000, icenowy@aosc.io wrote:
> 在 2017-09-18 16:30,Maxime Ripard 写道:
> > On Mon, Sep 18, 2017 at 03:36:43PM +0800, Icenowy Zheng wrote:
> > > 于 2017年9月18日 GMT+08:00 下午3:33:36, Maxime Ripard
> > > <maxime.ripard@free-electrons.com> 写到:
> > > >On Thu, Sep 14, 2017 at 10:52:46PM +0800, Icenowy Zheng wrote:
> > > >> Allwinner H3 features a thermal sensor like the one in A33, but has
> > > >its
> > > >> register re-arranged, the clock divider moved to CCU (originally the
> > > >> clock divider is in ADC) and added a pair of bus clock and reset.
> > > >>
> > > >> Update the binding document to cover H3.
> > > >>
> > > >> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
> > > >> Reviewed-by: Chen-Yu Tsai <wens@csie.org>
> > > >> ---
> > > >> Changes in v4:
> > > >> - Add nvmem calibration data (not yet used by the driver)
> > > >> Changes in v3:
> > > >> - Clock name changes.
> > > >> - Example node name changes.
> > > >> - Add interupts (not yet used by the driver).
> > > >>
> > > >> .../devicetree/bindings/mfd/sun4i-gpadc.txt | 30
> > > >++++++++++++++++++++--
> > > >> 1 file changed, 28 insertions(+), 2 deletions(-)
> > > >>
> > > >> diff --git a/Documentation/devicetree/bindings/mfd/sun4i-gpadc.txt
> > > >b/Documentation/devicetree/bindings/mfd/sun4i-gpadc.txt
> > > >> index badff3611a98..6c470d584bf9 100644
> > > >> --- a/Documentation/devicetree/bindings/mfd/sun4i-gpadc.txt
> > > >> +++ b/Documentation/devicetree/bindings/mfd/sun4i-gpadc.txt
> > > >> @@ -4,12 +4,26 @@ The Allwinner SoCs all have an ADC that can also
> > > >act as a thermal sensor
> > > >> and sometimes as a touchscreen controller.
> > > >>
> > > >> Required properties:
> > > >> - - compatible: "allwinner,sun8i-a33-ths",
> > > >> + - compatible: must contain one of the following compatibles:
> > > >> + - "allwinner,sun8i-a33-ths"
> > > >> + - "allwinner,sun8i-h3-ths"
> > > >> - reg: mmio address range of the chip,
> > > >> - #thermal-sensor-cells: shall be 0,
> > > >> - #io-channel-cells: shall be 0,
> > > >>
> > > >> -Example:
> > > >> +Optional properties:
> > > >> + - nvmem-cells: A phandle to the calibration data provided by a
> > > >nvmem device.
> > > >> + If unspecified default values shall be used.
> > > >> + - nvmem-cell-names: Should be "calibration-data"
> > > >
> > > >I'd prefer to have which sensor it applies to here. It wouldn't change
> > > >anything for the H3, but it definitely does for example for the A83t
> > > >that has two sensors, one for each cluster, and one for the GPU, each
> > > >with calibration data.
> > > >
> > > >What about cluster0-calibration?
>
> I prefer sensor0-calibration to sensor3-calibration now.
> (Theortically the new generation THS can support up to 4 sensors)
The mapping that explains what sensor0 means can change in the
future. It's better to be explicit here, and just say upfront what
it's about.
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [linux-sunxi] Re: [PATCH v4 1/6] dt-bindings: update the Allwinner GPADC device tree binding for H3
2017-09-20 7:52 ` Maxime Ripard
@ 2017-09-20 8:04 ` Icenowy Zheng
2017-09-21 19:32 ` Maxime Ripard
0 siblings, 1 reply; 28+ messages in thread
From: Icenowy Zheng @ 2017-09-20 8:04 UTC (permalink / raw)
To: maxime.ripard, Maxime Ripard
Cc: Lee Jones, Rob Herring, Chen-Yu Tsai, Jonathan Cameron,
Quentin Schulz, devicetree, linux-arm-kernel, linux-kernel,
linux-iio, linux-sunxi
于 2017年9月20日 GMT+08:00 下午3:52:23, Maxime Ripard <maxime.ripard@free-electrons.com> 写到:
>On Mon, Sep 18, 2017 at 03:47:25PM +0000, icenowy@aosc.io wrote:
>> 在 2017-09-18 16:30,Maxime Ripard 写道:
>> > On Mon, Sep 18, 2017 at 03:36:43PM +0800, Icenowy Zheng wrote:
>> > > 于 2017年9月18日 GMT+08:00 下午3:33:36, Maxime Ripard
>> > > <maxime.ripard@free-electrons.com> 写到:
>> > > >On Thu, Sep 14, 2017 at 10:52:46PM +0800, Icenowy Zheng wrote:
>> > > >> Allwinner H3 features a thermal sensor like the one in A33,
>but has
>> > > >its
>> > > >> register re-arranged, the clock divider moved to CCU
>(originally the
>> > > >> clock divider is in ADC) and added a pair of bus clock and
>reset.
>> > > >>
>> > > >> Update the binding document to cover H3.
>> > > >>
>> > > >> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
>> > > >> Reviewed-by: Chen-Yu Tsai <wens@csie.org>
>> > > >> ---
>> > > >> Changes in v4:
>> > > >> - Add nvmem calibration data (not yet used by the driver)
>> > > >> Changes in v3:
>> > > >> - Clock name changes.
>> > > >> - Example node name changes.
>> > > >> - Add interupts (not yet used by the driver).
>> > > >>
>> > > >> .../devicetree/bindings/mfd/sun4i-gpadc.txt | 30
>> > > >++++++++++++++++++++--
>> > > >> 1 file changed, 28 insertions(+), 2 deletions(-)
>> > > >>
>> > > >> diff --git
>a/Documentation/devicetree/bindings/mfd/sun4i-gpadc.txt
>> > > >b/Documentation/devicetree/bindings/mfd/sun4i-gpadc.txt
>> > > >> index badff3611a98..6c470d584bf9 100644
>> > > >> --- a/Documentation/devicetree/bindings/mfd/sun4i-gpadc.txt
>> > > >> +++ b/Documentation/devicetree/bindings/mfd/sun4i-gpadc.txt
>> > > >> @@ -4,12 +4,26 @@ The Allwinner SoCs all have an ADC that can
>also
>> > > >act as a thermal sensor
>> > > >> and sometimes as a touchscreen controller.
>> > > >>
>> > > >> Required properties:
>> > > >> - - compatible: "allwinner,sun8i-a33-ths",
>> > > >> + - compatible: must contain one of the following
>compatibles:
>> > > >> + - "allwinner,sun8i-a33-ths"
>> > > >> + - "allwinner,sun8i-h3-ths"
>> > > >> - reg: mmio address range of the chip,
>> > > >> - #thermal-sensor-cells: shall be 0,
>> > > >> - #io-channel-cells: shall be 0,
>> > > >>
>> > > >> -Example:
>> > > >> +Optional properties:
>> > > >> + - nvmem-cells: A phandle to the calibration data provided
>by a
>> > > >nvmem device.
>> > > >> + If unspecified default values shall be used.
>> > > >> + - nvmem-cell-names: Should be "calibration-data"
>> > > >
>> > > >I'd prefer to have which sensor it applies to here. It wouldn't
>change
>> > > >anything for the H3, but it definitely does for example for the
>A83t
>> > > >that has two sensors, one for each cluster, and one for the GPU,
>each
>> > > >with calibration data.
>> > > >
>> > > >What about cluster0-calibration?
>>
>> I prefer sensor0-calibration to sensor3-calibration now.
>> (Theortically the new generation THS can support up to 4 sensors)
>
>The mapping that explains what sensor0 means can change in the
>future. It's better to be explicit here, and just say upfront what
>it's about.
I think for some SoC (e.g. A64) there's no clear explain on
the functions of the sensors.
In addition, in the THS controller the sensors has a
explicit sequence, and when referencing it in the DT
the number is still needed (in thermal zones).
>
>Maxime
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [linux-sunxi] Re: [PATCH v4 1/6] dt-bindings: update the Allwinner GPADC device tree binding for H3
2017-09-20 8:04 ` Icenowy Zheng
@ 2017-09-21 19:32 ` Maxime Ripard
0 siblings, 0 replies; 28+ messages in thread
From: Maxime Ripard @ 2017-09-21 19:32 UTC (permalink / raw)
To: Icenowy Zheng
Cc: Lee Jones, Rob Herring, Chen-Yu Tsai, Jonathan Cameron,
Quentin Schulz, devicetree, linux-arm-kernel, linux-kernel,
linux-iio, linux-sunxi
[-- Attachment #1: Type: text/plain, Size: 3992 bytes --]
On Wed, Sep 20, 2017 at 08:04:02AM +0000, Icenowy Zheng wrote:
> 于 2017年9月20日 GMT+08:00 下午3:52:23, Maxime Ripard <maxime.ripard@free-electrons.com> 写到:
> >On Mon, Sep 18, 2017 at 03:47:25PM +0000, icenowy@aosc.io wrote:
> >> 在 2017-09-18 16:30,Maxime Ripard 写道:
> >> > On Mon, Sep 18, 2017 at 03:36:43PM +0800, Icenowy Zheng wrote:
> >> > > 于 2017年9月18日 GMT+08:00 下午3:33:36, Maxime Ripard
> >> > > <maxime.ripard@free-electrons.com> 写到:
> >> > > >On Thu, Sep 14, 2017 at 10:52:46PM +0800, Icenowy Zheng wrote:
> >> > > >> Allwinner H3 features a thermal sensor like the one in A33,
> >but has
> >> > > >its
> >> > > >> register re-arranged, the clock divider moved to CCU
> >(originally the
> >> > > >> clock divider is in ADC) and added a pair of bus clock and
> >reset.
> >> > > >>
> >> > > >> Update the binding document to cover H3.
> >> > > >>
> >> > > >> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
> >> > > >> Reviewed-by: Chen-Yu Tsai <wens@csie.org>
> >> > > >> ---
> >> > > >> Changes in v4:
> >> > > >> - Add nvmem calibration data (not yet used by the driver)
> >> > > >> Changes in v3:
> >> > > >> - Clock name changes.
> >> > > >> - Example node name changes.
> >> > > >> - Add interupts (not yet used by the driver).
> >> > > >>
> >> > > >> .../devicetree/bindings/mfd/sun4i-gpadc.txt | 30
> >> > > >++++++++++++++++++++--
> >> > > >> 1 file changed, 28 insertions(+), 2 deletions(-)
> >> > > >>
> >> > > >> diff --git
> >a/Documentation/devicetree/bindings/mfd/sun4i-gpadc.txt
> >> > > >b/Documentation/devicetree/bindings/mfd/sun4i-gpadc.txt
> >> > > >> index badff3611a98..6c470d584bf9 100644
> >> > > >> --- a/Documentation/devicetree/bindings/mfd/sun4i-gpadc.txt
> >> > > >> +++ b/Documentation/devicetree/bindings/mfd/sun4i-gpadc.txt
> >> > > >> @@ -4,12 +4,26 @@ The Allwinner SoCs all have an ADC that can
> >also
> >> > > >act as a thermal sensor
> >> > > >> and sometimes as a touchscreen controller.
> >> > > >>
> >> > > >> Required properties:
> >> > > >> - - compatible: "allwinner,sun8i-a33-ths",
> >> > > >> + - compatible: must contain one of the following
> >compatibles:
> >> > > >> + - "allwinner,sun8i-a33-ths"
> >> > > >> + - "allwinner,sun8i-h3-ths"
> >> > > >> - reg: mmio address range of the chip,
> >> > > >> - #thermal-sensor-cells: shall be 0,
> >> > > >> - #io-channel-cells: shall be 0,
> >> > > >>
> >> > > >> -Example:
> >> > > >> +Optional properties:
> >> > > >> + - nvmem-cells: A phandle to the calibration data provided
> >by a
> >> > > >nvmem device.
> >> > > >> + If unspecified default values shall be used.
> >> > > >> + - nvmem-cell-names: Should be "calibration-data"
> >> > > >
> >> > > >I'd prefer to have which sensor it applies to here. It wouldn't
> >change
> >> > > >anything for the H3, but it definitely does for example for the
> >A83t
> >> > > >that has two sensors, one for each cluster, and one for the GPU,
> >each
> >> > > >with calibration data.
> >> > > >
> >> > > >What about cluster0-calibration?
> >>
> >> I prefer sensor0-calibration to sensor3-calibration now.
> >> (Theortically the new generation THS can support up to 4 sensors)
> >
> >The mapping that explains what sensor0 means can change in the
> >future. It's better to be explicit here, and just say upfront what
> >it's about.
>
> I think for some SoC (e.g. A64) there's no clear explain on
> the functions of the sensors.
It's documented in the user manual ("sensor0 located in the CPU, sensor1 and
sensor2 located in the GPU"
> In addition, in the THS controller the sensors has a explicit
> sequence, and when referencing it in the DT the number is still
> needed (in thermal zones).
Yes, but that's something that can be made easier through defines too.
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v4 6/6] ARM: sun8i: h3: add partial CPU thermal zone
2017-09-18 8:27 ` Maxime Ripard
@ 2017-09-24 14:23 ` Jonathan Cameron
0 siblings, 0 replies; 28+ messages in thread
From: Jonathan Cameron @ 2017-09-24 14:23 UTC (permalink / raw)
To: Maxime Ripard
Cc: Quentin Schulz, Icenowy Zheng, Lee Jones, Rob Herring,
Chen-Yu Tsai, devicetree, linux-arm-kernel, linux-kernel,
linux-iio, linux-sunxi
On Mon, 18 Sep 2017 10:27:03 +0200
Maxime Ripard <maxime.ripard@free-electrons.com> wrote:
> Hi Jonathan,
>
> On Sat, Sep 16, 2017 at 03:17:34PM -0700, Jonathan Cameron wrote:
> > On Sat, 16 Sep 2017 12:05:49 +0200
> > Quentin Schulz <quentin.schulz@free-electrons.com> wrote:
> >
> > > Hi Icenowy,
> > >
> > > On 14/09/2017 16:52, Icenowy Zheng wrote:
> > > > Because of the restriction of the OF thermal framework, the thermal
> > > > sensor will fail to probe if the thermal zone doesn't exist.
> > > >
> > >
> > > Oh no, that's not good.
> > >
> > > We discussed about it on IRC and I even proposed a patch for it, telling
> > > you I would post it on the mailing list soon after. Of course, I forgot
> > > and you definitely should have yelled at me for not doing it :)
> > >
> > > I won't be able to test the patch soon. I can send it to you so that you
> > > can test it and integrate it in your patch series so it won't block you.
> > > Otherwise, we'll have to wait for a week or two for me to test it.
> > >
> > > Thanks and sorry for forgetting to post the patch you need,
> > > Quentin
> >
> > Other this outstanding issue I'm happy with the series, so hopefully
> > with Quentin's patch added we should be good to merge this one.
>
> We will at least need a v5.
>
> Maxime
>
Sure - I can see other issues are coming out of the woodwork!
Thanks,
Jonathan
^ permalink raw reply [flat|nested] 28+ messages in thread
end of thread, other threads:[~2017-09-24 14:23 UTC | newest]
Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-09-14 14:52 [PATCH v4 0/6] IIO-based thermal sensor driver for Allwinner H3 SoC Icenowy Zheng
2017-09-14 14:52 ` [PATCH v4 1/6] dt-bindings: update the Allwinner GPADC device tree binding for H3 Icenowy Zheng
2017-09-16 22:12 ` Jonathan Cameron
2017-09-18 7:33 ` Maxime Ripard
2017-09-18 7:36 ` Icenowy Zheng
2017-09-18 8:30 ` Maxime Ripard
2017-09-18 15:47 ` [linux-sunxi] " icenowy
2017-09-20 7:52 ` Maxime Ripard
2017-09-20 8:04 ` Icenowy Zheng
2017-09-21 19:32 ` Maxime Ripard
2017-09-14 14:52 ` [PATCH v4 2/6] iio: adc: sun4i-gpadc-iio: rename A33-specified registers to contain A33 Icenowy Zheng
2017-09-18 7:34 ` Maxime Ripard
2017-09-18 8:29 ` Lee Jones
2017-09-14 14:52 ` [PATCH v4 3/6] iio: adc: sun4i-gpadc-iio: rework code for supporting newer THS variants Icenowy Zheng
2017-09-18 7:36 ` Maxime Ripard
2017-09-14 14:52 ` [PATCH v4 4/6] iio: adc: sun4i-gpadc-iio: add support for H3 thermal sensor Icenowy Zheng
2017-09-16 9:45 ` Quentin Schulz
2017-09-16 10:14 ` icenowy
2017-09-16 10:35 ` Quentin Schulz
2017-09-18 8:24 ` Maxime Ripard
2017-09-16 22:16 ` Jonathan Cameron
2017-09-14 14:52 ` [PATCH v4 5/6] ARM: sun8i: h3: add support for the thermal sensor in H3 Icenowy Zheng
2017-09-18 8:25 ` Maxime Ripard
2017-09-14 14:52 ` [PATCH v4 6/6] ARM: sun8i: h3: add partial CPU thermal zone Icenowy Zheng
2017-09-16 10:05 ` Quentin Schulz
2017-09-16 22:17 ` Jonathan Cameron
2017-09-18 8:27 ` Maxime Ripard
2017-09-24 14:23 ` Jonathan Cameron
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).