IPQ5332 uses tsens v2.3.3 IP with combined interrupt for
upper/lower and critical. IPQ5332 does not have RPM and
kernel has to take care of TSENS enablement and calibration.
This patch series adds the sensor enablement and calibration
support. On top, adds IPQ5332 TSENS support.
Praveenkumar I (6):
dt-bindings: thermal: tsens: Add nvmem cells for calibration data
thermal/drivers/tsens: Add TSENS enable and calibration support for V2
dt-bindings: thermal: tsens: Add ipq5332 compatible
arm64: dts: qcom: ipq5332: Add tsens node
arm64: dts: qcom: ipq5332: Add thermal zone nodes
thermal/drivers/tsens: Add IPQ5332 support
.../bindings/thermal/qcom-tsens.yaml | 34 +++-
arch/arm64/boot/dts/qcom/ipq5332.dtsi | 185 ++++++++++++++++++
drivers/thermal/qcom/tsens-v2.c | 129 ++++++++++++
drivers/thermal/qcom/tsens.c | 40 +++-
drivers/thermal/qcom/tsens.h | 58 +++++-
5 files changed, 440 insertions(+), 6 deletions(-)
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
IPQ5332 uses TSENS v2.3.3 with combined interrupt. RPM is not
available in the SoC, hence adding new compatible to have the
sensor enablement and calibration function.
Signed-off-by: Praveenkumar I <[email protected]>
---
Documentation/devicetree/bindings/thermal/qcom-tsens.yaml | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml b/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml
index 8b7863c3989e..ee57713f6131 100644
--- a/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml
+++ b/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml
@@ -68,8 +68,10 @@ properties:
- const: qcom,tsens-v2
- description: v2 of TSENS with combined interrupt
- enum:
- - qcom,ipq8074-tsens
+ items:
+ - enum:
+ - qcom,ipq8074-tsens
+ - qcom,ipq5332-tsens
- description: v2 of TSENS with combined interrupt
items:
@@ -289,6 +291,7 @@ allOf:
contains:
enum:
- qcom,ipq8074-tsens
+ - qcom,ipq5332-tsens
then:
properties:
interrupts:
@@ -304,6 +307,7 @@ allOf:
contains:
enum:
- qcom,ipq8074-tsens
+ - qcom,ipq5332-tsens
- qcom,tsens-v0_1
- qcom,tsens-v1
- qcom,tsens-v2
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
SoCs without RPM have to enable sensors and calibrate from the kernel.
Though TSENS IP supports 16 sensors, not all are used. So added
sensors_to_en in tsens data help enable the relevant sensors.
Added new calibration function for V2 as the tsens.c calib function
only supports V1.
Signed-off-by: Praveenkumar I <[email protected]>
---
drivers/thermal/qcom/tsens-v2.c | 116 ++++++++++++++++++++++++++++++++
drivers/thermal/qcom/tsens.c | 37 +++++++++-
drivers/thermal/qcom/tsens.h | 56 +++++++++++++++
3 files changed, 208 insertions(+), 1 deletion(-)
diff --git a/drivers/thermal/qcom/tsens-v2.c b/drivers/thermal/qcom/tsens-v2.c
index 29a61d2d6ca3..db48b1d95348 100644
--- a/drivers/thermal/qcom/tsens-v2.c
+++ b/drivers/thermal/qcom/tsens-v2.c
@@ -6,11 +6,20 @@
#include <linux/bitops.h>
#include <linux/regmap.h>
+#include <linux/nvmem-consumer.h>
#include "tsens.h"
/* ----- SROT ------ */
#define SROT_HW_VER_OFF 0x0000
#define SROT_CTRL_OFF 0x0004
+#define SROT_MEASURE_PERIOD 0x0008
+#define SROT_Sn_CONVERSION 0x0060
+#define V2_SHIFT_DEFAULT 0x0003
+#define V2_SLOPE_DEFAULT 0x0cd0
+#define V2_CZERO_DEFAULT 0x016a
+#define ONE_PT_SLOPE 0x0cd0
+#define TWO_PT_SHIFTED_GAIN 921600
+#define ONE_PT_CZERO_CONST 94
/* ----- TM ------ */
#define TM_INT_EN_OFF 0x0004
@@ -59,6 +68,16 @@ static const struct reg_field tsens_v2_regfields[MAX_REGFIELDS] = {
/* CTRL_OFF */
[TSENS_EN] = REG_FIELD(SROT_CTRL_OFF, 0, 0),
[TSENS_SW_RST] = REG_FIELD(SROT_CTRL_OFF, 1, 1),
+ [SENSOR_EN] = REG_FIELD(SROT_CTRL_OFF, 3, 18),
+ [CODE_OR_TEMP] = REG_FIELD(SROT_CTRL_OFF, 21, 21),
+
+ /* MAIN_MEASURE_PERIOD */
+ [MAIN_MEASURE_PERIOD] = REG_FIELD(SROT_MEASURE_PERIOD, 0, 7),
+
+ /* Sn Conversion */
+ REG_FIELD_FOR_EACH_SENSOR16(SHIFT, SROT_Sn_CONVERSION, 23, 24),
+ REG_FIELD_FOR_EACH_SENSOR16(SLOPE, SROT_Sn_CONVERSION, 10, 22),
+ REG_FIELD_FOR_EACH_SENSOR16(CZERO, SROT_Sn_CONVERSION, 0, 9),
/* ----- TM ------ */
/* INTERRUPT ENABLE */
@@ -104,6 +123,103 @@ static const struct reg_field tsens_v2_regfields[MAX_REGFIELDS] = {
[TRDY] = REG_FIELD(TM_TRDY_OFF, 0, 0),
};
+static int tsens_v2_calibration(struct tsens_priv *priv)
+{
+ struct device *dev = priv->dev;
+ u32 mode, base0, base1;
+ u32 slope, czero;
+ char name[15];
+ int i, j, ret;
+
+ if (priv->num_sensors > MAX_SENSORS)
+ return -EINVAL;
+
+ ret = nvmem_cell_read_variable_le_u32(priv->dev, "mode", &mode);
+ if (ret == -ENOENT)
+ dev_warn(priv->dev, "Calibration data not present in DT\n");
+ if (ret < 0)
+ return ret;
+
+ dev_dbg(priv->dev, "calibration mode is %d\n", mode);
+
+ ret = nvmem_cell_read_variable_le_u32(priv->dev, "base0", &base0);
+ if (ret < 0)
+ return ret;
+
+ ret = nvmem_cell_read_variable_le_u32(priv->dev, "base1", &base1);
+ if (ret < 0)
+ return ret;
+
+ /* Read offset values and allocate SHIFT, SLOPE & CZERO regmap for enabled sensors */
+ for (i = 0; i < priv->num_sensors; i++) {
+ if (!(priv->sensors_to_en & (0x1 << i)))
+ continue;
+
+ ret = snprintf(name, sizeof(name), "s%d_offset", priv->sensor[i].hw_id);
+ if (ret < 0)
+ return ret;
+
+ ret = nvmem_cell_read_variable_le_u32(priv->dev, name, &priv->sensor[i].offset);
+ if (ret)
+ return ret;
+
+ for (j = SHIFT_0; j <= CZERO_0; j++) {
+ int idx = (i * 3) + j;
+
+ priv->rf[idx] = devm_regmap_field_alloc(dev, priv->srot_map,
+ priv->fields[idx]);
+ if (IS_ERR(priv->rf[idx]))
+ return PTR_ERR(priv->rf[idx]);
+ }
+ }
+
+ /* Based on calib mode, program SHIFT, SLOPE and CZERO for enabled sensors */
+ switch (mode) {
+ case TWO_PT_CALIB:
+ slope = (TWO_PT_SHIFTED_GAIN / (base1 - base0));
+
+ for (i = 0; i < priv->num_sensors; i++) {
+ if (!(priv->sensors_to_en & (0x1 << i)))
+ continue;
+
+ int idx = i * 3;
+
+ czero = (base0 + priv->sensor[i].offset - ((base1 - base0) / 3));
+ regmap_field_write(priv->rf[SHIFT_0 + idx], V2_SHIFT_DEFAULT);
+ regmap_field_write(priv->rf[SLOPE_0 + idx], slope);
+ regmap_field_write(priv->rf[CZERO_0 + idx], czero);
+ }
+ fallthrough;
+ case ONE_PT_CALIB2:
+ for (i = 0; i < priv->num_sensors; i++) {
+ if (!(priv->sensors_to_en & (0x1 << i)))
+ continue;
+
+ int idx = i * 3;
+
+ czero = base0 + priv->sensor[i].offset - ONE_PT_CZERO_CONST;
+ regmap_field_write(priv->rf[SHIFT_0 + idx], V2_SHIFT_DEFAULT);
+ regmap_field_write(priv->rf[SLOPE_0 + idx], ONE_PT_SLOPE);
+ regmap_field_write(priv->rf[CZERO_0 + idx], czero);
+ }
+ break;
+ default:
+ dev_dbg(priv->dev, "calibrationless mode\n");
+ for (i = 0; i < priv->num_sensors; i++) {
+ if (!(priv->sensors_to_en & (0x1 << i)))
+ continue;
+
+ int idx = i * 3;
+
+ regmap_field_write(priv->rf[SHIFT_0 + idx], V2_SHIFT_DEFAULT);
+ regmap_field_write(priv->rf[SLOPE_0 + idx], V2_SLOPE_DEFAULT);
+ regmap_field_write(priv->rf[CZERO_0 + idx], V2_CZERO_DEFAULT);
+ }
+ }
+
+ return 0;
+}
+
static const struct tsens_ops ops_generic_v2 = {
.init = init_common,
.get_temp = get_temp_tsens_valid,
diff --git a/drivers/thermal/qcom/tsens.c b/drivers/thermal/qcom/tsens.c
index 98c356acfe98..169690355dad 100644
--- a/drivers/thermal/qcom/tsens.c
+++ b/drivers/thermal/qcom/tsens.c
@@ -974,7 +974,7 @@ int __init init_common(struct tsens_priv *priv)
ret = regmap_field_read(priv->rf[TSENS_EN], &enabled);
if (ret)
goto err_put_device;
- if (!enabled) {
+ if (!enabled && !priv->sensors_to_en) {
dev_err(dev, "%s: device not enabled\n", __func__);
ret = -ENODEV;
goto err_put_device;
@@ -1006,6 +1006,40 @@ int __init init_common(struct tsens_priv *priv)
goto err_put_device;
}
+ /* Do TSENS initialization if required */
+ if (priv->sensors_to_en) {
+ priv->rf[CODE_OR_TEMP] = devm_regmap_field_alloc(dev, priv->srot_map,
+ priv->fields[CODE_OR_TEMP]);
+ if (IS_ERR(priv->rf[CODE_OR_TEMP])) {
+ ret = PTR_ERR(priv->rf[CODE_OR_TEMP]);
+ goto err_put_device;
+ }
+
+ priv->rf[MAIN_MEASURE_PERIOD] =
+ devm_regmap_field_alloc(dev, priv->srot_map,
+ priv->fields[MAIN_MEASURE_PERIOD]);
+ if (IS_ERR(priv->rf[MAIN_MEASURE_PERIOD])) {
+ ret = PTR_ERR(priv->rf[MAIN_MEASURE_PERIOD]);
+ goto err_put_device;
+ }
+
+ regmap_field_write(priv->rf[TSENS_SW_RST], 0x1);
+
+ /* Update measure period to 2ms */
+ regmap_field_write(priv->rf[MAIN_MEASURE_PERIOD], 0x1);
+
+ /* Enable available sensors */
+ regmap_field_write(priv->rf[SENSOR_EN], priv->sensors_to_en);
+
+ /* Real temperature format */
+ regmap_field_write(priv->rf[CODE_OR_TEMP], 0x1);
+
+ regmap_field_write(priv->rf[TSENS_SW_RST], 0x0);
+
+ /* Enable TSENS */
+ regmap_field_write(priv->rf[TSENS_EN], 0x1);
+ }
+
/* This loop might need changes if enum regfield_ids is reordered */
for (j = LAST_TEMP_0; j <= UP_THRESH_15; j += 16) {
for (i = 0; i < priv->feat->max_sensors; i++) {
@@ -1282,6 +1316,7 @@ static int tsens_probe(struct platform_device *pdev)
priv->dev = dev;
priv->num_sensors = num_sensors;
+ priv->sensors_to_en = data->sensors_to_en;
priv->ops = data->ops;
for (i = 0; i < priv->num_sensors; i++) {
if (data->hw_ids)
diff --git a/drivers/thermal/qcom/tsens.h b/drivers/thermal/qcom/tsens.h
index 2805de1c6827..f8897bc8944e 100644
--- a/drivers/thermal/qcom/tsens.h
+++ b/drivers/thermal/qcom/tsens.h
@@ -168,6 +168,58 @@ enum regfield_ids {
TSENS_SW_RST,
SENSOR_EN,
CODE_OR_TEMP,
+ /* MEASURE_PERIOD */
+ MAIN_MEASURE_PERIOD,
+
+ /* Sn_CONVERSION */
+ SHIFT_0,
+ SLOPE_0,
+ CZERO_0,
+ SHIFT_1,
+ SLOPE_1,
+ CZERO_1,
+ SHIFT_2,
+ SLOPE_2,
+ CZERO_2,
+ SHIFT_3,
+ SLOPE_3,
+ CZERO_3,
+ SHIFT_4,
+ SLOPE_4,
+ CZERO_4,
+ SHIFT_5,
+ SLOPE_5,
+ CZERO_5,
+ SHIFT_6,
+ SLOPE_6,
+ CZERO_6,
+ SHIFT_7,
+ SLOPE_7,
+ CZERO_7,
+ SHIFT_8,
+ SLOPE_8,
+ CZERO_8,
+ SHIFT_9,
+ SLOPE_9,
+ CZERO_9,
+ SHIFT_10,
+ SLOPE_10,
+ CZERO_10,
+ SHIFT_11,
+ SLOPE_11,
+ CZERO_11,
+ SHIFT_12,
+ SLOPE_12,
+ CZERO_12,
+ SHIFT_13,
+ SLOPE_13,
+ CZERO_13,
+ SHIFT_14,
+ SLOPE_14,
+ CZERO_14,
+ SHIFT_15,
+ SLOPE_15,
+ CZERO_15,
/* ----- TM ------ */
/* TRDY */
@@ -524,6 +576,7 @@ struct tsens_features {
/**
* struct tsens_plat_data - tsens compile-time platform data
* @num_sensors: Number of sensors supported by platform
+ * @sensors_to_en: Sensors to be enabled. Each bit represent a sensor
* @ops: operations the tsens instance supports
* @hw_ids: Subset of sensors ids supported by platform, if not the first n
* @feat: features of the IP
@@ -531,6 +584,7 @@ struct tsens_features {
*/
struct tsens_plat_data {
const u32 num_sensors;
+ const u16 sensors_to_en;
const struct tsens_ops *ops;
unsigned int *hw_ids;
struct tsens_features *feat;
@@ -551,6 +605,7 @@ struct tsens_context {
* struct tsens_priv - private data for each instance of the tsens IP
* @dev: pointer to struct device
* @num_sensors: number of sensors enabled on this device
+ * @sensors_to_en: sensors to be enabled. Each bit represents a sensor
* @tm_map: pointer to TM register address space
* @srot_map: pointer to SROT register address space
* @tm_offset: deal with old device trees that don't address TM and SROT
@@ -569,6 +624,7 @@ struct tsens_context {
struct tsens_priv {
struct device *dev;
u32 num_sensors;
+ u16 sensors_to_en;
struct regmap *tm_map;
struct regmap *srot_map;
u32 tm_offset;
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
Add TSENS V2 calibration nvmem cells for IPQ5332
Signed-off-by: Praveenkumar I <[email protected]>
---
.../bindings/thermal/qcom-tsens.yaml | 26 +++++++++++++++++--
1 file changed, 24 insertions(+), 2 deletions(-)
diff --git a/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml b/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml
index 27e9e16e6455..8b7863c3989e 100644
--- a/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml
+++ b/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml
@@ -91,7 +91,7 @@ properties:
maxItems: 2
nvmem-cells:
- oneOf:
+ anyOf:
- minItems: 1
maxItems: 2
description:
@@ -106,9 +106,13 @@ properties:
description: |
Reference to nvmem cells for the calibration mode, two calibration
bases and two cells per each sensor, main and backup copies, plus use_backup cell
+ - maxItems: 17
+ description: |
+ V2 of TSENS, reference to nvmem cells for the calibration mode, two calibration
+ bases and one cell per each sensor
nvmem-cell-names:
- oneOf:
+ anyOf:
- minItems: 1
items:
- const: calib
@@ -205,6 +209,24 @@ properties:
- const: s9_p2_backup
- const: s10_p1_backup
- const: s10_p2_backup
+ - items:
+ - const: mode
+ - const: base0
+ - const: base1
+ - const: s0_offset
+ - const: s3_offset
+ - const: s4_offset
+ - const: s5_offset
+ - const: s6_offset
+ - const: s7_offset
+ - const: s8_offset
+ - const: s9_offset
+ - const: s10_offset
+ - const: s11_offset
+ - const: s12_offset
+ - const: s13_offset
+ - const: s14_offset
+ - const: s15_offset
"#qcom,sensors":
description:
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
This patch adds thermal zone nodes for sensors present in
IPQ5332.
Signed-off-by: Praveenkumar I <[email protected]>
---
arch/arm64/boot/dts/qcom/ipq5332.dtsi | 72 +++++++++++++++++++++++++++
1 file changed, 72 insertions(+)
diff --git a/arch/arm64/boot/dts/qcom/ipq5332.dtsi b/arch/arm64/boot/dts/qcom/ipq5332.dtsi
index a1e3527178c0..8b276aeca53e 100644
--- a/arch/arm64/boot/dts/qcom/ipq5332.dtsi
+++ b/arch/arm64/boot/dts/qcom/ipq5332.dtsi
@@ -527,4 +527,76 @@ timer {
<GIC_PPI 4 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
<GIC_PPI 1 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>;
};
+
+ thermal-zones {
+ rfa-0-thermal{
+ polling-delay-passive = <0>;
+ polling-delay = <0>;
+ thermal-sensors = <&tsens 11>;
+
+ trips {
+ rfa-0-critical {
+ temperature = <125000>;
+ hysteresis = <1000>;
+ type = "critical";
+ };
+ };
+ };
+
+ rfa-1-thermal {
+ polling-delay-passive = <0>;
+ polling-delay = <0>;
+ thermal-sensors = <&tsens 12>;
+
+ trips {
+ rfa-1-critical {
+ temperature = <125000>;
+ hysteresis = <1000>;
+ type = "critical";
+ };
+ };
+ };
+
+ misc-thermal {
+ polling-delay-passive = <0>;
+ polling-delay = <0>;
+ thermal-sensors = <&tsens 13>;
+
+ trips {
+ misc-critical {
+ temperature = <125000>;
+ hysteresis = <1000>;
+ type = "critical";
+ };
+ };
+ };
+
+ cpu-top-thermal {
+ polling-delay-passive = <0>;
+ polling-delay = <0>;
+ thermal-sensors = <&tsens 14>;
+
+ trips {
+ cpu-top-critical {
+ temperature = <125000>;
+ hysteresis = <1000>;
+ type = "critical";
+ };
+ };
+ };
+
+ top-glue-thermal {
+ polling-delay-passive = <0>;
+ polling-delay = <0>;
+ thermal-sensors = <&tsens 15>;
+
+ trips {
+ top-glue-critical {
+ temperature = <125000>;
+ hysteresis = <1000>;
+ type = "critical";
+ };
+ };
+ };
+ };
};
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
IPQ5332 uses tsens v2.3.3 IP and it is having combined interrupt as
like IPQ8074. But as the SoCs does not have RPM, kernel needs to
take care of sensor enablement and calibration. Hence introduced
new ops and data for IPQ5332 and reused the feature_config from
IPQ8074.
Signed-off-by: Praveenkumar I <[email protected]>
---
drivers/thermal/qcom/tsens-v2.c | 13 +++++++++++++
drivers/thermal/qcom/tsens.c | 3 +++
drivers/thermal/qcom/tsens.h | 2 +-
3 files changed, 17 insertions(+), 1 deletion(-)
diff --git a/drivers/thermal/qcom/tsens-v2.c b/drivers/thermal/qcom/tsens-v2.c
index db48b1d95348..8b6e3876fd2c 100644
--- a/drivers/thermal/qcom/tsens-v2.c
+++ b/drivers/thermal/qcom/tsens-v2.c
@@ -237,6 +237,19 @@ struct tsens_plat_data data_ipq8074 = {
.fields = tsens_v2_regfields,
};
+static const struct tsens_ops ops_ipq5332_v2 = {
+ .init = init_common,
+ .get_temp = get_temp_tsens_valid,
+ .calibrate = tsens_v2_calibration,
+};
+
+struct tsens_plat_data data_ipq5332 = {
+ .sensors_to_en = 0xF800,
+ .ops = &ops_ipq5332_v2,
+ .feat = &ipq8074_feat,
+ .fields = tsens_v2_regfields,
+};
+
/* Kept around for backward compatibility with old msm8996.dtsi */
struct tsens_plat_data data_8996 = {
.num_sensors = 13,
diff --git a/drivers/thermal/qcom/tsens.c b/drivers/thermal/qcom/tsens.c
index 169690355dad..e8ba2901cda8 100644
--- a/drivers/thermal/qcom/tsens.c
+++ b/drivers/thermal/qcom/tsens.c
@@ -1140,6 +1140,9 @@ static const struct of_device_id tsens_table[] = {
}, {
.compatible = "qcom,ipq8074-tsens",
.data = &data_ipq8074,
+ }, {
+ .compatible = "qcom,ipq5332-tsens",
+ .data = &data_ipq5332,
}, {
.compatible = "qcom,mdm9607-tsens",
.data = &data_9607,
diff --git a/drivers/thermal/qcom/tsens.h b/drivers/thermal/qcom/tsens.h
index f8897bc8944e..36040f9beebc 100644
--- a/drivers/thermal/qcom/tsens.h
+++ b/drivers/thermal/qcom/tsens.h
@@ -701,6 +701,6 @@ extern struct tsens_plat_data data_8226, data_8909, data_8916, data_8939, data_8
extern struct tsens_plat_data data_tsens_v1, data_8976, data_8956;
/* TSENS v2 targets */
-extern struct tsens_plat_data data_8996, data_ipq8074, data_tsens_v2;
+extern struct tsens_plat_data data_8996, data_ipq8074, data_ipq5332, data_tsens_v2;
#endif /* __QCOM_TSENS_H__ */
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
On 10/07/2023 13:37, Praveenkumar I wrote:
> This patch adds thermal zone nodes for sensors present in
> IPQ5332.
>
> Signed-off-by: Praveenkumar I <[email protected]>
> ---
> arch/arm64/boot/dts/qcom/ipq5332.dtsi | 72 +++++++++++++++++++++++++++
> 1 file changed, 72 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/ipq5332.dtsi b/arch/arm64/boot/dts/qcom/ipq5332.dtsi
> index a1e3527178c0..8b276aeca53e 100644
> --- a/arch/arm64/boot/dts/qcom/ipq5332.dtsi
> +++ b/arch/arm64/boot/dts/qcom/ipq5332.dtsi
> @@ -527,4 +527,76 @@ timer {
> <GIC_PPI 4 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
> <GIC_PPI 1 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>;
> };
> +
> + thermal-zones {
> + rfa-0-thermal{
> + polling-delay-passive = <0>;
> + polling-delay = <0>;
> + thermal-sensors = <&tsens 11>;
> +
> + trips {
> + rfa-0-critical {
> + temperature = <125000>;
> + hysteresis = <1000>;
> + type = "critical";
> + };
> + };
> + };
> +
> + rfa-1-thermal {
> + polling-delay-passive = <0>;
> + polling-delay = <0>;
> + thermal-sensors = <&tsens 12>;
> +
> + trips {
> + rfa-1-critical {
> + temperature = <125000>;
> + hysteresis = <1000>;
> + type = "critical";
> + };
> + };
> + };
> +
> + misc-thermal {
> + polling-delay-passive = <0>;
> + polling-delay = <0>;
> + thermal-sensors = <&tsens 13>;
> +
> + trips {
> + misc-critical {
> + temperature = <125000>;
> + hysteresis = <1000>;
> + type = "critical";
> + };
> + };
> + };
> +
> + cpu-top-thermal {
> + polling-delay-passive = <0>;
> + polling-delay = <0>;
> + thermal-sensors = <&tsens 14>;
> +
> + trips {
> + cpu-top-critical {
> + temperature = <125000>;
> + hysteresis = <1000>;
> + type = "critical";
> + };
> + };
Could you please add a passive cooling devices for the CPU?
> + };
> +
> + top-glue-thermal {
> + polling-delay-passive = <0>;
> + polling-delay = <0>;
> + thermal-sensors = <&tsens 15>;
> +
> + trips {
> + top-glue-critical {
> + temperature = <125000>;
> + hysteresis = <1000>;
> + type = "critical";
> + };
> + };
> + };
> + };
> };
--
With best wishes
Dmitry
On 10/07/2023 13:37, Praveenkumar I wrote:
> IPQ5332 uses tsens v2.3.3 IP and it is having combined interrupt as
> like IPQ8074. But as the SoCs does not have RPM, kernel needs to
> take care of sensor enablement and calibration. Hence introduced
> new ops and data for IPQ5332 and reused the feature_config from
> IPQ8074.
>
> Signed-off-by: Praveenkumar I <[email protected]>
> ---
> drivers/thermal/qcom/tsens-v2.c | 13 +++++++++++++
> drivers/thermal/qcom/tsens.c | 3 +++
> drivers/thermal/qcom/tsens.h | 2 +-
> 3 files changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/thermal/qcom/tsens-v2.c b/drivers/thermal/qcom/tsens-v2.c
> index db48b1d95348..8b6e3876fd2c 100644
> --- a/drivers/thermal/qcom/tsens-v2.c
> +++ b/drivers/thermal/qcom/tsens-v2.c
> @@ -237,6 +237,19 @@ struct tsens_plat_data data_ipq8074 = {
> .fields = tsens_v2_regfields,
> };
>
> +static const struct tsens_ops ops_ipq5332_v2 = {
Please drop v2. It is unclear if it refers to tsens being v2 or being
specific to ipq5332 v2.
> + .init = init_common,
> + .get_temp = get_temp_tsens_valid,
> + .calibrate = tsens_v2_calibration,
> +};
> +
> +struct tsens_plat_data data_ipq5332 = {
> + .sensors_to_en = 0xF800,
This doesn't seem to match the offsets that you have enabled in the DTSI.
> + .ops = &ops_ipq5332_v2,
> + .feat = &ipq8074_feat,
> + .fields = tsens_v2_regfields,
> +};
> +
> /* Kept around for backward compatibility with old msm8996.dtsi */
> struct tsens_plat_data data_8996 = {
> .num_sensors = 13,
> diff --git a/drivers/thermal/qcom/tsens.c b/drivers/thermal/qcom/tsens.c
> index 169690355dad..e8ba2901cda8 100644
> --- a/drivers/thermal/qcom/tsens.c
> +++ b/drivers/thermal/qcom/tsens.c
> @@ -1140,6 +1140,9 @@ static const struct of_device_id tsens_table[] = {
> }, {
> .compatible = "qcom,ipq8074-tsens",
> .data = &data_ipq8074,
> + }, {
> + .compatible = "qcom,ipq5332-tsens",
> + .data = &data_ipq5332,
> }, {
> .compatible = "qcom,mdm9607-tsens",
> .data = &data_9607,
> diff --git a/drivers/thermal/qcom/tsens.h b/drivers/thermal/qcom/tsens.h
> index f8897bc8944e..36040f9beebc 100644
> --- a/drivers/thermal/qcom/tsens.h
> +++ b/drivers/thermal/qcom/tsens.h
> @@ -701,6 +701,6 @@ extern struct tsens_plat_data data_8226, data_8909, data_8916, data_8939, data_8
> extern struct tsens_plat_data data_tsens_v1, data_8976, data_8956;
>
> /* TSENS v2 targets */
> -extern struct tsens_plat_data data_8996, data_ipq8074, data_tsens_v2;
> +extern struct tsens_plat_data data_8996, data_ipq8074, data_ipq5332, data_tsens_v2;
>
> #endif /* __QCOM_TSENS_H__ */
--
With best wishes
Dmitry
On 10/07/2023 13:37, Praveenkumar I wrote:
> SoCs without RPM have to enable sensors and calibrate from the kernel.
> Though TSENS IP supports 16 sensors, not all are used. So added
> sensors_to_en in tsens data help enable the relevant sensors.
>
> Added new calibration function for V2 as the tsens.c calib function
> only supports V1.
>
> Signed-off-by: Praveenkumar I <[email protected]>
> ---
> drivers/thermal/qcom/tsens-v2.c | 116 ++++++++++++++++++++++++++++++++
> drivers/thermal/qcom/tsens.c | 37 +++++++++-
> drivers/thermal/qcom/tsens.h | 56 +++++++++++++++
> 3 files changed, 208 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/thermal/qcom/tsens-v2.c b/drivers/thermal/qcom/tsens-v2.c
> index 29a61d2d6ca3..db48b1d95348 100644
> --- a/drivers/thermal/qcom/tsens-v2.c
> +++ b/drivers/thermal/qcom/tsens-v2.c
> @@ -6,11 +6,20 @@
>
> #include <linux/bitops.h>
> #include <linux/regmap.h>
> +#include <linux/nvmem-consumer.h>
> #include "tsens.h"
>
> /* ----- SROT ------ */
> #define SROT_HW_VER_OFF 0x0000
> #define SROT_CTRL_OFF 0x0004
> +#define SROT_MEASURE_PERIOD 0x0008
> +#define SROT_Sn_CONVERSION 0x0060
> +#define V2_SHIFT_DEFAULT 0x0003
> +#define V2_SLOPE_DEFAULT 0x0cd0
> +#define V2_CZERO_DEFAULT 0x016a
> +#define ONE_PT_SLOPE 0x0cd0
> +#define TWO_PT_SHIFTED_GAIN 921600
> +#define ONE_PT_CZERO_CONST 94
>
> /* ----- TM ------ */
> #define TM_INT_EN_OFF 0x0004
> @@ -59,6 +68,16 @@ static const struct reg_field tsens_v2_regfields[MAX_REGFIELDS] = {
> /* CTRL_OFF */
> [TSENS_EN] = REG_FIELD(SROT_CTRL_OFF, 0, 0),
> [TSENS_SW_RST] = REG_FIELD(SROT_CTRL_OFF, 1, 1),
> + [SENSOR_EN] = REG_FIELD(SROT_CTRL_OFF, 3, 18),
> + [CODE_OR_TEMP] = REG_FIELD(SROT_CTRL_OFF, 21, 21),
> +
> + /* MAIN_MEASURE_PERIOD */
> + [MAIN_MEASURE_PERIOD] = REG_FIELD(SROT_MEASURE_PERIOD, 0, 7),
> +
> + /* Sn Conversion */
> + REG_FIELD_FOR_EACH_SENSOR16(SHIFT, SROT_Sn_CONVERSION, 23, 24),
> + REG_FIELD_FOR_EACH_SENSOR16(SLOPE, SROT_Sn_CONVERSION, 10, 22),
> + REG_FIELD_FOR_EACH_SENSOR16(CZERO, SROT_Sn_CONVERSION, 0, 9),
>
> /* ----- TM ------ */
> /* INTERRUPT ENABLE */
> @@ -104,6 +123,103 @@ static const struct reg_field tsens_v2_regfields[MAX_REGFIELDS] = {
> [TRDY] = REG_FIELD(TM_TRDY_OFF, 0, 0),
> };
>
> +static int tsens_v2_calibration(struct tsens_priv *priv)
> +{
> + struct device *dev = priv->dev;
> + u32 mode, base0, base1;
> + u32 slope, czero;
> + char name[15];
> + int i, j, ret;
> +
> + if (priv->num_sensors > MAX_SENSORS)
> + return -EINVAL;
> +
> + ret = nvmem_cell_read_variable_le_u32(priv->dev, "mode", &mode);
> + if (ret == -ENOENT)
> + dev_warn(priv->dev, "Calibration data not present in DT\n");
> + if (ret < 0)
> + return ret;
> +
> + dev_dbg(priv->dev, "calibration mode is %d\n", mode);
> +
> + ret = nvmem_cell_read_variable_le_u32(priv->dev, "base0", &base0);
> + if (ret < 0)
> + return ret;
> +
> + ret = nvmem_cell_read_variable_le_u32(priv->dev, "base1", &base1);
> + if (ret < 0)
> + return ret;
> +
> + /* Read offset values and allocate SHIFT, SLOPE & CZERO regmap for enabled sensors */
> + for (i = 0; i < priv->num_sensors; i++) {
> + if (!(priv->sensors_to_en & (0x1 << i)))
> + continue;
> +
> + ret = snprintf(name, sizeof(name), "s%d_offset", priv->sensor[i].hw_id);
> + if (ret < 0)
> + return ret;
> +
> + ret = nvmem_cell_read_variable_le_u32(priv->dev, name, &priv->sensor[i].offset);
> + if (ret)
> + return ret;
> +
> + for (j = SHIFT_0; j <= CZERO_0; j++) {
> + int idx = (i * 3) + j;
> +
> + priv->rf[idx] = devm_regmap_field_alloc(dev, priv->srot_map,
> + priv->fields[idx]);
> + if (IS_ERR(priv->rf[idx]))
> + return PTR_ERR(priv->rf[idx]);
I think, allocating data structures for 48 regfields, which are written
just once, to be an overkill.
> + }
> + }
> +
> + /* Based on calib mode, program SHIFT, SLOPE and CZERO for enabled sensors */
> + switch (mode) {
> + case TWO_PT_CALIB:
> + slope = (TWO_PT_SHIFTED_GAIN / (base1 - base0));
> +
> + for (i = 0; i < priv->num_sensors; i++) {
> + if (!(priv->sensors_to_en & (0x1 << i)))
> + continue;
> +
> + int idx = i * 3;
> +
> + czero = (base0 + priv->sensor[i].offset - ((base1 - base0) / 3));
> + regmap_field_write(priv->rf[SHIFT_0 + idx], V2_SHIFT_DEFAULT);
> + regmap_field_write(priv->rf[SLOPE_0 + idx], slope);
> + regmap_field_write(priv->rf[CZERO_0 + idx], czero);
> + }
> + fallthrough;
> + case ONE_PT_CALIB2:
> + for (i = 0; i < priv->num_sensors; i++) {
> + if (!(priv->sensors_to_en & (0x1 << i)))
> + continue;
> +
> + int idx = i * 3;
> +
> + czero = base0 + priv->sensor[i].offset - ONE_PT_CZERO_CONST;
> + regmap_field_write(priv->rf[SHIFT_0 + idx], V2_SHIFT_DEFAULT);
> + regmap_field_write(priv->rf[SLOPE_0 + idx], ONE_PT_SLOPE);
> + regmap_field_write(priv->rf[CZERO_0 + idx], czero);
> + }
> + break;
> + default:
> + dev_dbg(priv->dev, "calibrationless mode\n");
> + for (i = 0; i < priv->num_sensors; i++) {
> + if (!(priv->sensors_to_en & (0x1 << i)))
> + continue;
> +
> + int idx = i * 3;
> +
> + regmap_field_write(priv->rf[SHIFT_0 + idx], V2_SHIFT_DEFAULT);
> + regmap_field_write(priv->rf[SLOPE_0 + idx], V2_SLOPE_DEFAULT);
> + regmap_field_write(priv->rf[CZERO_0 + idx], V2_CZERO_DEFAULT);
> + }
> + }
This code iterates over the sensors field several times. Please consider
extracting a function that handles all setup for a single sensor, then
calling it in a loop (I should probably do the same for tsens-v0/v1 too).
> +
> + return 0;
> +}
> +
> static const struct tsens_ops ops_generic_v2 = {
> .init = init_common,
> .get_temp = get_temp_tsens_valid,
> diff --git a/drivers/thermal/qcom/tsens.c b/drivers/thermal/qcom/tsens.c
> index 98c356acfe98..169690355dad 100644
> --- a/drivers/thermal/qcom/tsens.c
> +++ b/drivers/thermal/qcom/tsens.c
> @@ -974,7 +974,7 @@ int __init init_common(struct tsens_priv *priv)
> ret = regmap_field_read(priv->rf[TSENS_EN], &enabled);
> if (ret)
> goto err_put_device;
> - if (!enabled) {
> + if (!enabled && !priv->sensors_to_en) {
> dev_err(dev, "%s: device not enabled\n", __func__);
> ret = -ENODEV;
> goto err_put_device;
> @@ -1006,6 +1006,40 @@ int __init init_common(struct tsens_priv *priv)
> goto err_put_device;
> }
>
> + /* Do TSENS initialization if required */
> + if (priv->sensors_to_en) {
Maybe it would be better to explicitly add VER_2_X_NO_RPM and check it here?
> + priv->rf[CODE_OR_TEMP] = devm_regmap_field_alloc(dev, priv->srot_map,
> + priv->fields[CODE_OR_TEMP]);
> + if (IS_ERR(priv->rf[CODE_OR_TEMP])) {
> + ret = PTR_ERR(priv->rf[CODE_OR_TEMP]);
> + goto err_put_device;
> + }
> +
> + priv->rf[MAIN_MEASURE_PERIOD] =
> + devm_regmap_field_alloc(dev, priv->srot_map,
> + priv->fields[MAIN_MEASURE_PERIOD]);
> + if (IS_ERR(priv->rf[MAIN_MEASURE_PERIOD])) {
> + ret = PTR_ERR(priv->rf[MAIN_MEASURE_PERIOD]);
> + goto err_put_device;
> + }
> +
> + regmap_field_write(priv->rf[TSENS_SW_RST], 0x1);
> +
> + /* Update measure period to 2ms */
> + regmap_field_write(priv->rf[MAIN_MEASURE_PERIOD], 0x1);
> +
> + /* Enable available sensors */
> + regmap_field_write(priv->rf[SENSOR_EN], priv->sensors_to_en);
> +
> + /* Real temperature format */
> + regmap_field_write(priv->rf[CODE_OR_TEMP], 0x1);
> +
> + regmap_field_write(priv->rf[TSENS_SW_RST], 0x0);
> +
> + /* Enable TSENS */
> + regmap_field_write(priv->rf[TSENS_EN], 0x1);
> + }
> +
> /* This loop might need changes if enum regfield_ids is reordered */
> for (j = LAST_TEMP_0; j <= UP_THRESH_15; j += 16) {
> for (i = 0; i < priv->feat->max_sensors; i++) {
> @@ -1282,6 +1316,7 @@ static int tsens_probe(struct platform_device *pdev)
>
> priv->dev = dev;
> priv->num_sensors = num_sensors;
> + priv->sensors_to_en = data->sensors_to_en;
> priv->ops = data->ops;
> for (i = 0; i < priv->num_sensors; i++) {
> if (data->hw_ids)
> diff --git a/drivers/thermal/qcom/tsens.h b/drivers/thermal/qcom/tsens.h
> index 2805de1c6827..f8897bc8944e 100644
> --- a/drivers/thermal/qcom/tsens.h
> +++ b/drivers/thermal/qcom/tsens.h
> @@ -168,6 +168,58 @@ enum regfield_ids {
> TSENS_SW_RST,
> SENSOR_EN,
> CODE_OR_TEMP,
> + /* MEASURE_PERIOD */
> + MAIN_MEASURE_PERIOD,
> +
> + /* Sn_CONVERSION */
> + SHIFT_0,
> + SLOPE_0,
> + CZERO_0,
> + SHIFT_1,
> + SLOPE_1,
> + CZERO_1,
> + SHIFT_2,
> + SLOPE_2,
> + CZERO_2,
> + SHIFT_3,
> + SLOPE_3,
> + CZERO_3,
> + SHIFT_4,
> + SLOPE_4,
> + CZERO_4,
> + SHIFT_5,
> + SLOPE_5,
> + CZERO_5,
> + SHIFT_6,
> + SLOPE_6,
> + CZERO_6,
> + SHIFT_7,
> + SLOPE_7,
> + CZERO_7,
> + SHIFT_8,
> + SLOPE_8,
> + CZERO_8,
> + SHIFT_9,
> + SLOPE_9,
> + CZERO_9,
> + SHIFT_10,
> + SLOPE_10,
> + CZERO_10,
> + SHIFT_11,
> + SLOPE_11,
> + CZERO_11,
> + SHIFT_12,
> + SLOPE_12,
> + CZERO_12,
> + SHIFT_13,
> + SLOPE_13,
> + CZERO_13,
> + SHIFT_14,
> + SLOPE_14,
> + CZERO_14,
> + SHIFT_15,
> + SLOPE_15,
> + CZERO_15,
>
> /* ----- TM ------ */
> /* TRDY */
> @@ -524,6 +576,7 @@ struct tsens_features {
> /**
> * struct tsens_plat_data - tsens compile-time platform data
> * @num_sensors: Number of sensors supported by platform
> + * @sensors_to_en: Sensors to be enabled. Each bit represent a sensor
> * @ops: operations the tsens instance supports
> * @hw_ids: Subset of sensors ids supported by platform, if not the first n
> * @feat: features of the IP
> @@ -531,6 +584,7 @@ struct tsens_features {
> */
> struct tsens_plat_data {
> const u32 num_sensors;
> + const u16 sensors_to_en;
There is already a similar field, hw_ids. Can it be used instead?
> const struct tsens_ops *ops;
> unsigned int *hw_ids;
> struct tsens_features *feat;
> @@ -551,6 +605,7 @@ struct tsens_context {
> * struct tsens_priv - private data for each instance of the tsens IP
> * @dev: pointer to struct device
> * @num_sensors: number of sensors enabled on this device
> + * @sensors_to_en: sensors to be enabled. Each bit represents a sensor
> * @tm_map: pointer to TM register address space
> * @srot_map: pointer to SROT register address space
> * @tm_offset: deal with old device trees that don't address TM and SROT
> @@ -569,6 +624,7 @@ struct tsens_context {
> struct tsens_priv {
> struct device *dev;
> u32 num_sensors;
> + u16 sensors_to_en;
> struct regmap *tm_map;
> struct regmap *srot_map;
> u32 tm_offset;
--
With best wishes
Dmitry
On 10/07/2023 14:19, Dmitry Baryshkov wrote:
> On 10/07/2023 13:37, Praveenkumar I wrote:
>> SoCs without RPM have to enable sensors and calibrate from the kernel.
>> Though TSENS IP supports 16 sensors, not all are used. So added
>> sensors_to_en in tsens data help enable the relevant sensors.
>>
>> Added new calibration function for V2 as the tsens.c calib function
>> only supports V1.
>>
>> Signed-off-by: Praveenkumar I <[email protected]>
>> ---
>> drivers/thermal/qcom/tsens-v2.c | 116 ++++++++++++++++++++++++++++++++
>> drivers/thermal/qcom/tsens.c | 37 +++++++++-
>> drivers/thermal/qcom/tsens.h | 56 +++++++++++++++
>> 3 files changed, 208 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/thermal/qcom/tsens-v2.c
>> b/drivers/thermal/qcom/tsens-v2.c
>> index 29a61d2d6ca3..db48b1d95348 100644
>> --- a/drivers/thermal/qcom/tsens-v2.c
>> +++ b/drivers/thermal/qcom/tsens-v2.c
>> @@ -6,11 +6,20 @@
>> #include <linux/bitops.h>
>> #include <linux/regmap.h>
>> +#include <linux/nvmem-consumer.h>
>> #include "tsens.h"
>> /* ----- SROT ------ */
>> #define SROT_HW_VER_OFF 0x0000
>> #define SROT_CTRL_OFF 0x0004
>> +#define SROT_MEASURE_PERIOD 0x0008
>> +#define SROT_Sn_CONVERSION 0x0060
>> +#define V2_SHIFT_DEFAULT 0x0003
>> +#define V2_SLOPE_DEFAULT 0x0cd0
>> +#define V2_CZERO_DEFAULT 0x016a
>> +#define ONE_PT_SLOPE 0x0cd0
>> +#define TWO_PT_SHIFTED_GAIN 921600
>> +#define ONE_PT_CZERO_CONST 94
>> /* ----- TM ------ */
>> #define TM_INT_EN_OFF 0x0004
>> @@ -59,6 +68,16 @@ static const struct reg_field
>> tsens_v2_regfields[MAX_REGFIELDS] = {
>> /* CTRL_OFF */
>> [TSENS_EN] = REG_FIELD(SROT_CTRL_OFF, 0, 0),
>> [TSENS_SW_RST] = REG_FIELD(SROT_CTRL_OFF, 1, 1),
>> + [SENSOR_EN] = REG_FIELD(SROT_CTRL_OFF, 3, 18),
>> + [CODE_OR_TEMP] = REG_FIELD(SROT_CTRL_OFF, 21, 21),
>> +
>> + /* MAIN_MEASURE_PERIOD */
>> + [MAIN_MEASURE_PERIOD] = REG_FIELD(SROT_MEASURE_PERIOD, 0, 7),
>> +
>> + /* Sn Conversion */
>> + REG_FIELD_FOR_EACH_SENSOR16(SHIFT, SROT_Sn_CONVERSION, 23, 24),
>> + REG_FIELD_FOR_EACH_SENSOR16(SLOPE, SROT_Sn_CONVERSION, 10, 22),
>> + REG_FIELD_FOR_EACH_SENSOR16(CZERO, SROT_Sn_CONVERSION, 0, 9),
>> /* ----- TM ------ */
>> /* INTERRUPT ENABLE */
>> @@ -104,6 +123,103 @@ static const struct reg_field
>> tsens_v2_regfields[MAX_REGFIELDS] = {
>> [TRDY] = REG_FIELD(TM_TRDY_OFF, 0, 0),
>> };
>> +static int tsens_v2_calibration(struct tsens_priv *priv)
>> +{
>> + struct device *dev = priv->dev;
>> + u32 mode, base0, base1;
>> + u32 slope, czero;
>> + char name[15];
>> + int i, j, ret;
>> +
>> + if (priv->num_sensors > MAX_SENSORS)
>> + return -EINVAL;
>> +
>> + ret = nvmem_cell_read_variable_le_u32(priv->dev, "mode", &mode);
>> + if (ret == -ENOENT)
>> + dev_warn(priv->dev, "Calibration data not present in DT\n");
>> + if (ret < 0)
>> + return ret;
>> +
>> + dev_dbg(priv->dev, "calibration mode is %d\n", mode);
>> +
>> + ret = nvmem_cell_read_variable_le_u32(priv->dev, "base0", &base0);
>> + if (ret < 0)
>> + return ret;
>> +
>> + ret = nvmem_cell_read_variable_le_u32(priv->dev, "base1", &base1);
>> + if (ret < 0)
>> + return ret;
>> +
>> + /* Read offset values and allocate SHIFT, SLOPE & CZERO regmap
>> for enabled sensors */
>> + for (i = 0; i < priv->num_sensors; i++) {
>> + if (!(priv->sensors_to_en & (0x1 << i)))
>> + continue;
>> +
>> + ret = snprintf(name, sizeof(name), "s%d_offset",
>> priv->sensor[i].hw_id);
>> + if (ret < 0)
>> + return ret;
>> +
>> + ret = nvmem_cell_read_variable_le_u32(priv->dev, name,
>> &priv->sensor[i].offset);
>> + if (ret)
>> + return ret;
>> +
>> + for (j = SHIFT_0; j <= CZERO_0; j++) {
>> + int idx = (i * 3) + j;
>> +
>> + priv->rf[idx] = devm_regmap_field_alloc(dev, priv->srot_map,
>> + priv->fields[idx]);
>> + if (IS_ERR(priv->rf[idx]))
>> + return PTR_ERR(priv->rf[idx]);
>
> I think, allocating data structures for 48 regfields, which are written
> just once, to be an overkill.
>
>> + }
>> + }
>> +
>> + /* Based on calib mode, program SHIFT, SLOPE and CZERO for
>> enabled sensors */
>> + switch (mode) {
>> + case TWO_PT_CALIB:
>> + slope = (TWO_PT_SHIFTED_GAIN / (base1 - base0));
>> +
>> + for (i = 0; i < priv->num_sensors; i++) {
>> + if (!(priv->sensors_to_en & (0x1 << i)))
>> + continue;
>> +
>> + int idx = i * 3;
>> +
>> + czero = (base0 + priv->sensor[i].offset - ((base1 -
>> base0) / 3));
>> + regmap_field_write(priv->rf[SHIFT_0 + idx],
>> V2_SHIFT_DEFAULT);
>> + regmap_field_write(priv->rf[SLOPE_0 + idx], slope);
>> + regmap_field_write(priv->rf[CZERO_0 + idx], czero);
>> + }
>> + fallthrough;
>> + case ONE_PT_CALIB2:
>> + for (i = 0; i < priv->num_sensors; i++) {
>> + if (!(priv->sensors_to_en & (0x1 << i)))
>> + continue;
>> +
>> + int idx = i * 3;
>> +
>> + czero = base0 + priv->sensor[i].offset - ONE_PT_CZERO_CONST;
>> + regmap_field_write(priv->rf[SHIFT_0 + idx],
>> V2_SHIFT_DEFAULT);
>> + regmap_field_write(priv->rf[SLOPE_0 + idx], ONE_PT_SLOPE);
>> + regmap_field_write(priv->rf[CZERO_0 + idx], czero);
>> + }
>> + break;
>> + default:
>> + dev_dbg(priv->dev, "calibrationless mode\n");
>> + for (i = 0; i < priv->num_sensors; i++) {
>> + if (!(priv->sensors_to_en & (0x1 << i)))
>> + continue;
>> +
>> + int idx = i * 3;
>> +
>> + regmap_field_write(priv->rf[SHIFT_0 + idx],
>> V2_SHIFT_DEFAULT);
>> + regmap_field_write(priv->rf[SLOPE_0 + idx],
>> V2_SLOPE_DEFAULT);
>> + regmap_field_write(priv->rf[CZERO_0 + idx],
>> V2_CZERO_DEFAULT);
>> + }
>> + }
>
> This code iterates over the sensors field several times. Please consider
> extracting a function that handles all setup for a single sensor, then
> calling it in a loop (I should probably do the same for tsens-v0/v1 too).
>
>> +
>> + return 0;
>> +}
>> +
>> static const struct tsens_ops ops_generic_v2 = {
>> .init = init_common,
>> .get_temp = get_temp_tsens_valid,
>> diff --git a/drivers/thermal/qcom/tsens.c b/drivers/thermal/qcom/tsens.c
>> index 98c356acfe98..169690355dad 100644
>> --- a/drivers/thermal/qcom/tsens.c
>> +++ b/drivers/thermal/qcom/tsens.c
>> @@ -974,7 +974,7 @@ int __init init_common(struct tsens_priv *priv)
>> ret = regmap_field_read(priv->rf[TSENS_EN], &enabled);
>> if (ret)
>> goto err_put_device;
>> - if (!enabled) {
>> + if (!enabled && !priv->sensors_to_en) {
>> dev_err(dev, "%s: device not enabled\n", __func__);
>> ret = -ENODEV;
>> goto err_put_device;
>> @@ -1006,6 +1006,40 @@ int __init init_common(struct tsens_priv *priv)
>> goto err_put_device;
>> }
>> + /* Do TSENS initialization if required */
>> + if (priv->sensors_to_en) {
>
> Maybe it would be better to explicitly add VER_2_X_NO_RPM and check it
> here?
After taking a look at the patch 6, can we just add a separate init()
function which will call init_common() and then initialize these fields?
>
>> + priv->rf[CODE_OR_TEMP] = devm_regmap_field_alloc(dev,
>> priv->srot_map,
>> + priv->fields[CODE_OR_TEMP]);
>> + if (IS_ERR(priv->rf[CODE_OR_TEMP])) {
>> + ret = PTR_ERR(priv->rf[CODE_OR_TEMP]);
>> + goto err_put_device;
>> + }
>> +
>> + priv->rf[MAIN_MEASURE_PERIOD] =
>> + devm_regmap_field_alloc(dev, priv->srot_map,
>> + priv->fields[MAIN_MEASURE_PERIOD]);
>> + if (IS_ERR(priv->rf[MAIN_MEASURE_PERIOD])) {
>> + ret = PTR_ERR(priv->rf[MAIN_MEASURE_PERIOD]);
>> + goto err_put_device;
>> + }
>> +
>> + regmap_field_write(priv->rf[TSENS_SW_RST], 0x1);
>> +
>> + /* Update measure period to 2ms */
>> + regmap_field_write(priv->rf[MAIN_MEASURE_PERIOD], 0x1);
>> +
>> + /* Enable available sensors */
>> + regmap_field_write(priv->rf[SENSOR_EN], priv->sensors_to_en);
>> +
>> + /* Real temperature format */
>> + regmap_field_write(priv->rf[CODE_OR_TEMP], 0x1);
>> +
>> + regmap_field_write(priv->rf[TSENS_SW_RST], 0x0);
>> +
>> + /* Enable TSENS */
>> + regmap_field_write(priv->rf[TSENS_EN], 0x1);
>> + }
>> +
>> /* This loop might need changes if enum regfield_ids is
>> reordered */
>> for (j = LAST_TEMP_0; j <= UP_THRESH_15; j += 16) {
>> for (i = 0; i < priv->feat->max_sensors; i++) {
>> @@ -1282,6 +1316,7 @@ static int tsens_probe(struct platform_device
>> *pdev)
>> priv->dev = dev;
>> priv->num_sensors = num_sensors;
>> + priv->sensors_to_en = data->sensors_to_en;
>> priv->ops = data->ops;
>> for (i = 0; i < priv->num_sensors; i++) {
>> if (data->hw_ids)
>> diff --git a/drivers/thermal/qcom/tsens.h b/drivers/thermal/qcom/tsens.h
>> index 2805de1c6827..f8897bc8944e 100644
>> --- a/drivers/thermal/qcom/tsens.h
>> +++ b/drivers/thermal/qcom/tsens.h
>> @@ -168,6 +168,58 @@ enum regfield_ids {
>> TSENS_SW_RST,
>> SENSOR_EN,
>> CODE_OR_TEMP,
>> + /* MEASURE_PERIOD */
>> + MAIN_MEASURE_PERIOD,
>> +
>> + /* Sn_CONVERSION */
>> + SHIFT_0,
>> + SLOPE_0,
>> + CZERO_0,
>> + SHIFT_1,
>> + SLOPE_1,
>> + CZERO_1,
>> + SHIFT_2,
>> + SLOPE_2,
>> + CZERO_2,
>> + SHIFT_3,
>> + SLOPE_3,
>> + CZERO_3,
>> + SHIFT_4,
>> + SLOPE_4,
>> + CZERO_4,
>> + SHIFT_5,
>> + SLOPE_5,
>> + CZERO_5,
>> + SHIFT_6,
>> + SLOPE_6,
>> + CZERO_6,
>> + SHIFT_7,
>> + SLOPE_7,
>> + CZERO_7,
>> + SHIFT_8,
>> + SLOPE_8,
>> + CZERO_8,
>> + SHIFT_9,
>> + SLOPE_9,
>> + CZERO_9,
>> + SHIFT_10,
>> + SLOPE_10,
>> + CZERO_10,
>> + SHIFT_11,
>> + SLOPE_11,
>> + CZERO_11,
>> + SHIFT_12,
>> + SLOPE_12,
>> + CZERO_12,
>> + SHIFT_13,
>> + SLOPE_13,
>> + CZERO_13,
>> + SHIFT_14,
>> + SLOPE_14,
>> + CZERO_14,
>> + SHIFT_15,
>> + SLOPE_15,
>> + CZERO_15,
>> /* ----- TM ------ */
>> /* TRDY */
>> @@ -524,6 +576,7 @@ struct tsens_features {
>> /**
>> * struct tsens_plat_data - tsens compile-time platform data
>> * @num_sensors: Number of sensors supported by platform
>> + * @sensors_to_en: Sensors to be enabled. Each bit represent a sensor
>> * @ops: operations the tsens instance supports
>> * @hw_ids: Subset of sensors ids supported by platform, if not the
>> first n
>> * @feat: features of the IP
>> @@ -531,6 +584,7 @@ struct tsens_features {
>> */
>> struct tsens_plat_data {
>> const u32 num_sensors;
>> + const u16 sensors_to_en;
>
> There is already a similar field, hw_ids. Can it be used instead?
>
>> const struct tsens_ops *ops;
>> unsigned int *hw_ids;
>> struct tsens_features *feat;
>> @@ -551,6 +605,7 @@ struct tsens_context {
>> * struct tsens_priv - private data for each instance of the tsens IP
>> * @dev: pointer to struct device
>> * @num_sensors: number of sensors enabled on this device
>> + * @sensors_to_en: sensors to be enabled. Each bit represents a sensor
>> * @tm_map: pointer to TM register address space
>> * @srot_map: pointer to SROT register address space
>> * @tm_offset: deal with old device trees that don't address TM and
>> SROT
>> @@ -569,6 +624,7 @@ struct tsens_context {
>> struct tsens_priv {
>> struct device *dev;
>> u32 num_sensors;
>> + u16 sensors_to_en;
>> struct regmap *tm_map;
>> struct regmap *srot_map;
>> u32 tm_offset;
>
--
With best wishes
Dmitry
On 10.07.2023 13:23, Dmitry Baryshkov wrote:
> On 10/07/2023 13:37, Praveenkumar I wrote:
>> This patch adds thermal zone nodes for sensors present in
>> IPQ5332.
>>
>> Signed-off-by: Praveenkumar I <[email protected]>
>> ---
>> arch/arm64/boot/dts/qcom/ipq5332.dtsi | 72 +++++++++++++++++++++++++++
>> 1 file changed, 72 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/ipq5332.dtsi b/arch/arm64/boot/dts/qcom/ipq5332.dtsi
>> index a1e3527178c0..8b276aeca53e 100644
>> --- a/arch/arm64/boot/dts/qcom/ipq5332.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/ipq5332.dtsi
>> @@ -527,4 +527,76 @@ timer {
>> <GIC_PPI 4 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
>> <GIC_PPI 1 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>;
>> };
>> +
>> + thermal-zones {
>> + rfa-0-thermal{
thermal {
>> + polling-delay-passive = <0>;
>> + polling-delay = <0>;
>> + thermal-sensors = <&tsens 11>;
>> +
>> + trips {
Indentation seems off, tab size for kernel code is 8 spaces.
Konrad
>> + rfa-0-critical {
>> + temperature = <125000>;
>> + hysteresis = <1000>;
>> + type = "critical";
>> + };
>> + };
>> + };
>> +
>> + rfa-1-thermal {
>> + polling-delay-passive = <0>;
>> + polling-delay = <0>;
>> + thermal-sensors = <&tsens 12>;
>> +
>> + trips {
>> + rfa-1-critical {
>> + temperature = <125000>;
>> + hysteresis = <1000>;
>> + type = "critical";
>> + };
>> + };
>> + };
>> +
>> + misc-thermal {
>> + polling-delay-passive = <0>;
>> + polling-delay = <0>;
>> + thermal-sensors = <&tsens 13>;
>> +
>> + trips {
>> + misc-critical {
>> + temperature = <125000>;
>> + hysteresis = <1000>;
>> + type = "critical";
>> + };
>> + };
>> + };
>> +
>> + cpu-top-thermal {
>> + polling-delay-passive = <0>;
>> + polling-delay = <0>;
>> + thermal-sensors = <&tsens 14>;
>> +
>> + trips {
>> + cpu-top-critical {
>> + temperature = <125000>;
>> + hysteresis = <1000>;
>> + type = "critical";
>> + };
>> + };
>
> Could you please add a passive cooling devices for the CPU?
>
>> + };
>> +
>> + top-glue-thermal {
>> + polling-delay-passive = <0>;
>> + polling-delay = <0>;
>> + thermal-sensors = <&tsens 15>;
>> +
>> + trips {
>> + top-glue-critical {
>> + temperature = <125000>;
>> + hysteresis = <1000>;
>> + type = "critical";
>> + };
>> + };
>> + };
>> + };
>> };
>
On 7/10/2023 5:44 PM, Konrad Dybcio wrote:
> On 10.07.2023 13:23, Dmitry Baryshkov wrote:
>> On 10/07/2023 13:37, Praveenkumar I wrote:
>>> This patch adds thermal zone nodes for sensors present in
>>> IPQ5332.
>>>
>>> Signed-off-by: Praveenkumar I <[email protected]>
>>> ---
>>> arch/arm64/boot/dts/qcom/ipq5332.dtsi | 72 +++++++++++++++++++++++++++
>>> 1 file changed, 72 insertions(+)
>>>
>>> diff --git a/arch/arm64/boot/dts/qcom/ipq5332.dtsi b/arch/arm64/boot/dts/qcom/ipq5332.dtsi
>>> index a1e3527178c0..8b276aeca53e 100644
>>> --- a/arch/arm64/boot/dts/qcom/ipq5332.dtsi
>>> +++ b/arch/arm64/boot/dts/qcom/ipq5332.dtsi
>>> @@ -527,4 +527,76 @@ timer {
>>> <GIC_PPI 4 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
>>> <GIC_PPI 1 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>;
>>> };
>>> +
>>> + thermal-zones {
>>> + rfa-0-thermal{
> thermal {
In all other DTS, name is 'thermal-zones". Hence followed the same.
>
>>> + polling-delay-passive = <0>;
>>> + polling-delay = <0>;
>>> + thermal-sensors = <&tsens 11>;
>>> +
>>> + trips {
> Indentation seems off, tab size for kernel code is 8 spaces.
Sure, will check the indent and update in next patch.
>
> Konrad
>>> + rfa-0-critical {
>>> + temperature = <125000>;
>>> + hysteresis = <1000>;
>>> + type = "critical";
>>> + };
>>> + };
>>> + };
>>> +
>>> + rfa-1-thermal {
>>> + polling-delay-passive = <0>;
>>> + polling-delay = <0>;
>>> + thermal-sensors = <&tsens 12>;
>>> +
>>> + trips {
>>> + rfa-1-critical {
>>> + temperature = <125000>;
>>> + hysteresis = <1000>;
>>> + type = "critical";
>>> + };
>>> + };
>>> + };
>>> +
>>> + misc-thermal {
>>> + polling-delay-passive = <0>;
>>> + polling-delay = <0>;
>>> + thermal-sensors = <&tsens 13>;
>>> +
>>> + trips {
>>> + misc-critical {
>>> + temperature = <125000>;
>>> + hysteresis = <1000>;
>>> + type = "critical";
>>> + };
>>> + };
>>> + };
>>> +
>>> + cpu-top-thermal {
>>> + polling-delay-passive = <0>;
>>> + polling-delay = <0>;
>>> + thermal-sensors = <&tsens 14>;
>>> +
>>> + trips {
>>> + cpu-top-critical {
>>> + temperature = <125000>;
>>> + hysteresis = <1000>;
>>> + type = "critical";
>>> + };
>>> + };
>> Could you please add a passive cooling devices for the CPU?
>>
>>> + };
>>> +
>>> + top-glue-thermal {
>>> + polling-delay-passive = <0>;
>>> + polling-delay = <0>;
>>> + thermal-sensors = <&tsens 15>;
>>> +
>>> + trips {
>>> + top-glue-critical {
>>> + temperature = <125000>;
>>> + hysteresis = <1000>;
>>> + type = "critical";
>>> + };
>>> + };
>>> + };
>>> + };
>>> };
--
Thanks,
Praveenkumar
On 7/10/2023 4:53 PM, Dmitry Baryshkov wrote:
> On 10/07/2023 13:37, Praveenkumar I wrote:
>> This patch adds thermal zone nodes for sensors present in
>> IPQ5332.
>>
>> Signed-off-by: Praveenkumar I <[email protected]>
>> ---
>> arch/arm64/boot/dts/qcom/ipq5332.dtsi | 72 +++++++++++++++++++++++++++
>> 1 file changed, 72 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/ipq5332.dtsi
>> b/arch/arm64/boot/dts/qcom/ipq5332.dtsi
>> index a1e3527178c0..8b276aeca53e 100644
>> --- a/arch/arm64/boot/dts/qcom/ipq5332.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/ipq5332.dtsi
>> @@ -527,4 +527,76 @@ timer {
>> <GIC_PPI 4 (GIC_CPU_MASK_SIMPLE(4) |
>> IRQ_TYPE_LEVEL_LOW)>,
>> <GIC_PPI 1 (GIC_CPU_MASK_SIMPLE(4) |
>> IRQ_TYPE_LEVEL_LOW)>;
>> };
>> +
>> + thermal-zones {
>> + rfa-0-thermal{
>> + polling-delay-passive = <0>;
>> + polling-delay = <0>;
>> + thermal-sensors = <&tsens 11>;
>> +
>> + trips {
>> + rfa-0-critical {
>> + temperature = <125000>;
>> + hysteresis = <1000>;
>> + type = "critical";
>> + };
>> + };
>> + };
>> +
>> + rfa-1-thermal {
>> + polling-delay-passive = <0>;
>> + polling-delay = <0>;
>> + thermal-sensors = <&tsens 12>;
>> +
>> + trips {
>> + rfa-1-critical {
>> + temperature = <125000>;
>> + hysteresis = <1000>;
>> + type = "critical";
>> + };
>> + };
>> + };
>> +
>> + misc-thermal {
>> + polling-delay-passive = <0>;
>> + polling-delay = <0>;
>> + thermal-sensors = <&tsens 13>;
>> +
>> + trips {
>> + misc-critical {
>> + temperature = <125000>;
>> + hysteresis = <1000>;
>> + type = "critical";
>> + };
>> + };
>> + };
>> +
>> + cpu-top-thermal {
>> + polling-delay-passive = <0>;
>> + polling-delay = <0>;
>> + thermal-sensors = <&tsens 14>;
>> +
>> + trips {
>> + cpu-top-critical {
>> + temperature = <125000>;
>> + hysteresis = <1000>;
>> + type = "critical";
>> + };
>> + };
>
> Could you please add a passive cooling devices for the CPU?
Sure, will add the passive trip.
>
>> + };
>> +
>> + top-glue-thermal {
>> + polling-delay-passive = <0>;
>> + polling-delay = <0>;
>> + thermal-sensors = <&tsens 15>;
>> +
>> + trips {
>> + top-glue-critical {
>> + temperature = <125000>;
>> + hysteresis = <1000>;
>> + type = "critical";
>> + };
>> + };
>> + };
>> + };
>> };
>
--
Thanks,
Praveenkumar
On 7/10/2023 4:49 PM, Dmitry Baryshkov wrote:
> On 10/07/2023 13:37, Praveenkumar I wrote:
>> SoCs without RPM have to enable sensors and calibrate from the kernel.
>> Though TSENS IP supports 16 sensors, not all are used. So added
>> sensors_to_en in tsens data help enable the relevant sensors.
>>
>> Added new calibration function for V2 as the tsens.c calib function
>> only supports V1.
>>
>> Signed-off-by: Praveenkumar I <[email protected]>
>> ---
>> drivers/thermal/qcom/tsens-v2.c | 116 ++++++++++++++++++++++++++++++++
>> drivers/thermal/qcom/tsens.c | 37 +++++++++-
>> drivers/thermal/qcom/tsens.h | 56 +++++++++++++++
>> 3 files changed, 208 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/thermal/qcom/tsens-v2.c
>> b/drivers/thermal/qcom/tsens-v2.c
>> index 29a61d2d6ca3..db48b1d95348 100644
>> --- a/drivers/thermal/qcom/tsens-v2.c
>> +++ b/drivers/thermal/qcom/tsens-v2.c
>> @@ -6,11 +6,20 @@
>> #include <linux/bitops.h>
>> #include <linux/regmap.h>
>> +#include <linux/nvmem-consumer.h>
>> #include "tsens.h"
>> /* ----- SROT ------ */
>> #define SROT_HW_VER_OFF 0x0000
>> #define SROT_CTRL_OFF 0x0004
>> +#define SROT_MEASURE_PERIOD 0x0008
>> +#define SROT_Sn_CONVERSION 0x0060
>> +#define V2_SHIFT_DEFAULT 0x0003
>> +#define V2_SLOPE_DEFAULT 0x0cd0
>> +#define V2_CZERO_DEFAULT 0x016a
>> +#define ONE_PT_SLOPE 0x0cd0
>> +#define TWO_PT_SHIFTED_GAIN 921600
>> +#define ONE_PT_CZERO_CONST 94
>> /* ----- TM ------ */
>> #define TM_INT_EN_OFF 0x0004
>> @@ -59,6 +68,16 @@ static const struct reg_field
>> tsens_v2_regfields[MAX_REGFIELDS] = {
>> /* CTRL_OFF */
>> [TSENS_EN] = REG_FIELD(SROT_CTRL_OFF, 0, 0),
>> [TSENS_SW_RST] = REG_FIELD(SROT_CTRL_OFF, 1, 1),
>> + [SENSOR_EN] = REG_FIELD(SROT_CTRL_OFF, 3, 18),
>> + [CODE_OR_TEMP] = REG_FIELD(SROT_CTRL_OFF, 21, 21),
>> +
>> + /* MAIN_MEASURE_PERIOD */
>> + [MAIN_MEASURE_PERIOD] = REG_FIELD(SROT_MEASURE_PERIOD, 0, 7),
>> +
>> + /* Sn Conversion */
>> + REG_FIELD_FOR_EACH_SENSOR16(SHIFT, SROT_Sn_CONVERSION, 23, 24),
>> + REG_FIELD_FOR_EACH_SENSOR16(SLOPE, SROT_Sn_CONVERSION, 10, 22),
>> + REG_FIELD_FOR_EACH_SENSOR16(CZERO, SROT_Sn_CONVERSION, 0, 9),
>> /* ----- TM ------ */
>> /* INTERRUPT ENABLE */
>> @@ -104,6 +123,103 @@ static const struct reg_field
>> tsens_v2_regfields[MAX_REGFIELDS] = {
>> [TRDY] = REG_FIELD(TM_TRDY_OFF, 0, 0),
>> };
>> +static int tsens_v2_calibration(struct tsens_priv *priv)
>> +{
>> + struct device *dev = priv->dev;
>> + u32 mode, base0, base1;
>> + u32 slope, czero;
>> + char name[15];
>> + int i, j, ret;
>> +
>> + if (priv->num_sensors > MAX_SENSORS)
>> + return -EINVAL;
>> +
>> + ret = nvmem_cell_read_variable_le_u32(priv->dev, "mode", &mode);
>> + if (ret == -ENOENT)
>> + dev_warn(priv->dev, "Calibration data not present in DT\n");
>> + if (ret < 0)
>> + return ret;
>> +
>> + dev_dbg(priv->dev, "calibration mode is %d\n", mode);
>> +
>> + ret = nvmem_cell_read_variable_le_u32(priv->dev, "base0", &base0);
>> + if (ret < 0)
>> + return ret;
>> +
>> + ret = nvmem_cell_read_variable_le_u32(priv->dev, "base1", &base1);
>> + if (ret < 0)
>> + return ret;
>> +
>> + /* Read offset values and allocate SHIFT, SLOPE & CZERO regmap
>> for enabled sensors */
>> + for (i = 0; i < priv->num_sensors; i++) {
>> + if (!(priv->sensors_to_en & (0x1 << i)))
>> + continue;
>> +
>> + ret = snprintf(name, sizeof(name), "s%d_offset",
>> priv->sensor[i].hw_id);
>> + if (ret < 0)
>> + return ret;
>> +
>> + ret = nvmem_cell_read_variable_le_u32(priv->dev, name,
>> &priv->sensor[i].offset);
>> + if (ret)
>> + return ret;
>> +
>> + for (j = SHIFT_0; j <= CZERO_0; j++) {
>> + int idx = (i * 3) + j;
>> +
>> + priv->rf[idx] = devm_regmap_field_alloc(dev,
>> priv->srot_map,
>> + priv->fields[idx]);
>> + if (IS_ERR(priv->rf[idx]))
>> + return PTR_ERR(priv->rf[idx]);
>
> I think, allocating data structures for 48 regfields, which are
> written just once, to be an overkill.
Can we change it to single field for each sensor. For example,
CONVERSION_0 instead of SHIFT_0, SLOPE_0 and CZERO_0? This way it will
be max 16 regfields.
>
>> + }
>> + }
>> +
>> + /* Based on calib mode, program SHIFT, SLOPE and CZERO for
>> enabled sensors */
>> + switch (mode) {
>> + case TWO_PT_CALIB:
>> + slope = (TWO_PT_SHIFTED_GAIN / (base1 - base0));
>> +
>> + for (i = 0; i < priv->num_sensors; i++) {
>> + if (!(priv->sensors_to_en & (0x1 << i)))
>> + continue;
>> +
>> + int idx = i * 3;
>> +
>> + czero = (base0 + priv->sensor[i].offset - ((base1 -
>> base0) / 3));
>> + regmap_field_write(priv->rf[SHIFT_0 + idx],
>> V2_SHIFT_DEFAULT);
>> + regmap_field_write(priv->rf[SLOPE_0 + idx], slope);
>> + regmap_field_write(priv->rf[CZERO_0 + idx], czero);
>> + }
>> + fallthrough;
>> + case ONE_PT_CALIB2:
>> + for (i = 0; i < priv->num_sensors; i++) {
>> + if (!(priv->sensors_to_en & (0x1 << i)))
>> + continue;
>> +
>> + int idx = i * 3;
>> +
>> + czero = base0 + priv->sensor[i].offset -
>> ONE_PT_CZERO_CONST;
>> + regmap_field_write(priv->rf[SHIFT_0 + idx],
>> V2_SHIFT_DEFAULT);
>> + regmap_field_write(priv->rf[SLOPE_0 + idx], ONE_PT_SLOPE);
>> + regmap_field_write(priv->rf[CZERO_0 + idx], czero);
>> + }
>> + break;
>> + default:
>> + dev_dbg(priv->dev, "calibrationless mode\n");
>> + for (i = 0; i < priv->num_sensors; i++) {
>> + if (!(priv->sensors_to_en & (0x1 << i)))
>> + continue;
>> +
>> + int idx = i * 3;
>> +
>> + regmap_field_write(priv->rf[SHIFT_0 + idx],
>> V2_SHIFT_DEFAULT);
>> + regmap_field_write(priv->rf[SLOPE_0 + idx],
>> V2_SLOPE_DEFAULT);
>> + regmap_field_write(priv->rf[CZERO_0 + idx],
>> V2_CZERO_DEFAULT);
>> + }
>> + }
>
> This code iterates over the sensors field several times. Please
> consider extracting a function that handles all setup for a single
> sensor, then calling it in a loop (I should probably do the same for
> tsens-v0/v1 too).
Sure. After reading the mode0, base0 and base1 from QFPROM, we can call
a function in a loop to setup the calibration for each sensor.
>
>> +
>> + return 0;
>> +}
>> +
>> static const struct tsens_ops ops_generic_v2 = {
>> .init = init_common,
>> .get_temp = get_temp_tsens_valid,
>> diff --git a/drivers/thermal/qcom/tsens.c b/drivers/thermal/qcom/tsens.c
>> index 98c356acfe98..169690355dad 100644
>> --- a/drivers/thermal/qcom/tsens.c
>> +++ b/drivers/thermal/qcom/tsens.c
>> @@ -974,7 +974,7 @@ int __init init_common(struct tsens_priv *priv)
>> ret = regmap_field_read(priv->rf[TSENS_EN], &enabled);
>> if (ret)
>> goto err_put_device;
>> - if (!enabled) {
>> + if (!enabled && !priv->sensors_to_en) {
>> dev_err(dev, "%s: device not enabled\n", __func__);
>> ret = -ENODEV;
>> goto err_put_device;
>> @@ -1006,6 +1006,40 @@ int __init init_common(struct tsens_priv *priv)
>> goto err_put_device;
>> }
>> + /* Do TSENS initialization if required */
>> + if (priv->sensors_to_en) {
>
> Maybe it would be better to explicitly add VER_2_X_NO_RPM and check it
> here?
Sure, will add a separate version macro.
>
>> + priv->rf[CODE_OR_TEMP] = devm_regmap_field_alloc(dev,
>> priv->srot_map,
>> + priv->fields[CODE_OR_TEMP]);
>> + if (IS_ERR(priv->rf[CODE_OR_TEMP])) {
>> + ret = PTR_ERR(priv->rf[CODE_OR_TEMP]);
>> + goto err_put_device;
>> + }
>> +
>> + priv->rf[MAIN_MEASURE_PERIOD] =
>> + devm_regmap_field_alloc(dev, priv->srot_map,
>> + priv->fields[MAIN_MEASURE_PERIOD]);
>> + if (IS_ERR(priv->rf[MAIN_MEASURE_PERIOD])) {
>> + ret = PTR_ERR(priv->rf[MAIN_MEASURE_PERIOD]);
>> + goto err_put_device;
>> + }
>> +
>> + regmap_field_write(priv->rf[TSENS_SW_RST], 0x1);
>> +
>> + /* Update measure period to 2ms */
>> + regmap_field_write(priv->rf[MAIN_MEASURE_PERIOD], 0x1);
>> +
>> + /* Enable available sensors */
>> + regmap_field_write(priv->rf[SENSOR_EN], priv->sensors_to_en);
>> +
>> + /* Real temperature format */
>> + regmap_field_write(priv->rf[CODE_OR_TEMP], 0x1);
>> +
>> + regmap_field_write(priv->rf[TSENS_SW_RST], 0x0);
>> +
>> + /* Enable TSENS */
>> + regmap_field_write(priv->rf[TSENS_EN], 0x1);
>> + }
>> +
>> /* This loop might need changes if enum regfield_ids is
>> reordered */
>> for (j = LAST_TEMP_0; j <= UP_THRESH_15; j += 16) {
>> for (i = 0; i < priv->feat->max_sensors; i++) {
>> @@ -1282,6 +1316,7 @@ static int tsens_probe(struct platform_device
>> *pdev)
>> priv->dev = dev;
>> priv->num_sensors = num_sensors;
>> + priv->sensors_to_en = data->sensors_to_en;
>> priv->ops = data->ops;
>> for (i = 0; i < priv->num_sensors; i++) {
>> if (data->hw_ids)
>> diff --git a/drivers/thermal/qcom/tsens.h b/drivers/thermal/qcom/tsens.h
>> index 2805de1c6827..f8897bc8944e 100644
>> --- a/drivers/thermal/qcom/tsens.h
>> +++ b/drivers/thermal/qcom/tsens.h
>> @@ -168,6 +168,58 @@ enum regfield_ids {
>> TSENS_SW_RST,
>> SENSOR_EN,
>> CODE_OR_TEMP,
>> + /* MEASURE_PERIOD */
>> + MAIN_MEASURE_PERIOD,
>> +
>> + /* Sn_CONVERSION */
>> + SHIFT_0,
>> + SLOPE_0,
>> + CZERO_0,
>> + SHIFT_1,
>> + SLOPE_1,
>> + CZERO_1,
>> + SHIFT_2,
>> + SLOPE_2,
>> + CZERO_2,
>> + SHIFT_3,
>> + SLOPE_3,
>> + CZERO_3,
>> + SHIFT_4,
>> + SLOPE_4,
>> + CZERO_4,
>> + SHIFT_5,
>> + SLOPE_5,
>> + CZERO_5,
>> + SHIFT_6,
>> + SLOPE_6,
>> + CZERO_6,
>> + SHIFT_7,
>> + SLOPE_7,
>> + CZERO_7,
>> + SHIFT_8,
>> + SLOPE_8,
>> + CZERO_8,
>> + SHIFT_9,
>> + SLOPE_9,
>> + CZERO_9,
>> + SHIFT_10,
>> + SLOPE_10,
>> + CZERO_10,
>> + SHIFT_11,
>> + SLOPE_11,
>> + CZERO_11,
>> + SHIFT_12,
>> + SLOPE_12,
>> + CZERO_12,
>> + SHIFT_13,
>> + SLOPE_13,
>> + CZERO_13,
>> + SHIFT_14,
>> + SLOPE_14,
>> + CZERO_14,
>> + SHIFT_15,
>> + SLOPE_15,
>> + CZERO_15,
>> /* ----- TM ------ */
>> /* TRDY */
>> @@ -524,6 +576,7 @@ struct tsens_features {
>> /**
>> * struct tsens_plat_data - tsens compile-time platform data
>> * @num_sensors: Number of sensors supported by platform
>> + * @sensors_to_en: Sensors to be enabled. Each bit represent a sensor
>> * @ops: operations the tsens instance supports
>> * @hw_ids: Subset of sensors ids supported by platform, if not the
>> first n
>> * @feat: features of the IP
>> @@ -531,6 +584,7 @@ struct tsens_features {
>> */
>> struct tsens_plat_data {
>> const u32 num_sensors;
>> + const u16 sensors_to_en;
>
> There is already a similar field, hw_ids. Can it be used instead?
Yes, it can be used. I missed to check this hw_ids. Will change the
num_sensors to 5 and use the hw_ids.
>
>> const struct tsens_ops *ops;
>> unsigned int *hw_ids;
>> struct tsens_features *feat;
>> @@ -551,6 +605,7 @@ struct tsens_context {
>> * struct tsens_priv - private data for each instance of the tsens IP
>> * @dev: pointer to struct device
>> * @num_sensors: number of sensors enabled on this device
>> + * @sensors_to_en: sensors to be enabled. Each bit represents a sensor
>> * @tm_map: pointer to TM register address space
>> * @srot_map: pointer to SROT register address space
>> * @tm_offset: deal with old device trees that don't address TM and
>> SROT
>> @@ -569,6 +624,7 @@ struct tsens_context {
>> struct tsens_priv {
>> struct device *dev;
>> u32 num_sensors;
>> + u16 sensors_to_en;
>> struct regmap *tm_map;
>> struct regmap *srot_map;
>> u32 tm_offset;
>
--
Thanks,
Praveenkumar
On 7/10/2023 4:52 PM, Dmitry Baryshkov wrote:
> On 10/07/2023 14:19, Dmitry Baryshkov wrote:
>> On 10/07/2023 13:37, Praveenkumar I wrote:
>>> SoCs without RPM have to enable sensors and calibrate from the kernel.
>>> Though TSENS IP supports 16 sensors, not all are used. So added
>>> sensors_to_en in tsens data help enable the relevant sensors.
>>>
>>> Added new calibration function for V2 as the tsens.c calib function
>>> only supports V1.
>>>
>>> Signed-off-by: Praveenkumar I <[email protected]>
>>> ---
>>> drivers/thermal/qcom/tsens-v2.c | 116
>>> ++++++++++++++++++++++++++++++++
>>> drivers/thermal/qcom/tsens.c | 37 +++++++++-
>>> drivers/thermal/qcom/tsens.h | 56 +++++++++++++++
>>> 3 files changed, 208 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/thermal/qcom/tsens-v2.c
>>> b/drivers/thermal/qcom/tsens-v2.c
>>> index 29a61d2d6ca3..db48b1d95348 100644
>>> --- a/drivers/thermal/qcom/tsens-v2.c
>>> +++ b/drivers/thermal/qcom/tsens-v2.c
>>> @@ -6,11 +6,20 @@
>>> #include <linux/bitops.h>
>>> #include <linux/regmap.h>
>>> +#include <linux/nvmem-consumer.h>
>>> #include "tsens.h"
>>> /* ----- SROT ------ */
>>> #define SROT_HW_VER_OFF 0x0000
>>> #define SROT_CTRL_OFF 0x0004
>>> +#define SROT_MEASURE_PERIOD 0x0008
>>> +#define SROT_Sn_CONVERSION 0x0060
>>> +#define V2_SHIFT_DEFAULT 0x0003
>>> +#define V2_SLOPE_DEFAULT 0x0cd0
>>> +#define V2_CZERO_DEFAULT 0x016a
>>> +#define ONE_PT_SLOPE 0x0cd0
>>> +#define TWO_PT_SHIFTED_GAIN 921600
>>> +#define ONE_PT_CZERO_CONST 94
>>> /* ----- TM ------ */
>>> #define TM_INT_EN_OFF 0x0004
>>> @@ -59,6 +68,16 @@ static const struct reg_field
>>> tsens_v2_regfields[MAX_REGFIELDS] = {
>>> /* CTRL_OFF */
>>> [TSENS_EN] = REG_FIELD(SROT_CTRL_OFF, 0, 0),
>>> [TSENS_SW_RST] = REG_FIELD(SROT_CTRL_OFF, 1, 1),
>>> + [SENSOR_EN] = REG_FIELD(SROT_CTRL_OFF, 3, 18),
>>> + [CODE_OR_TEMP] = REG_FIELD(SROT_CTRL_OFF, 21, 21),
>>> +
>>> + /* MAIN_MEASURE_PERIOD */
>>> + [MAIN_MEASURE_PERIOD] = REG_FIELD(SROT_MEASURE_PERIOD, 0, 7),
>>> +
>>> + /* Sn Conversion */
>>> + REG_FIELD_FOR_EACH_SENSOR16(SHIFT, SROT_Sn_CONVERSION, 23, 24),
>>> + REG_FIELD_FOR_EACH_SENSOR16(SLOPE, SROT_Sn_CONVERSION, 10, 22),
>>> + REG_FIELD_FOR_EACH_SENSOR16(CZERO, SROT_Sn_CONVERSION, 0, 9),
>>> /* ----- TM ------ */
>>> /* INTERRUPT ENABLE */
>>> @@ -104,6 +123,103 @@ static const struct reg_field
>>> tsens_v2_regfields[MAX_REGFIELDS] = {
>>> [TRDY] = REG_FIELD(TM_TRDY_OFF, 0, 0),
>>> };
>>> +static int tsens_v2_calibration(struct tsens_priv *priv)
>>> +{
>>> + struct device *dev = priv->dev;
>>> + u32 mode, base0, base1;
>>> + u32 slope, czero;
>>> + char name[15];
>>> + int i, j, ret;
>>> +
>>> + if (priv->num_sensors > MAX_SENSORS)
>>> + return -EINVAL;
>>> +
>>> + ret = nvmem_cell_read_variable_le_u32(priv->dev, "mode", &mode);
>>> + if (ret == -ENOENT)
>>> + dev_warn(priv->dev, "Calibration data not present in DT\n");
>>> + if (ret < 0)
>>> + return ret;
>>> +
>>> + dev_dbg(priv->dev, "calibration mode is %d\n", mode);
>>> +
>>> + ret = nvmem_cell_read_variable_le_u32(priv->dev, "base0", &base0);
>>> + if (ret < 0)
>>> + return ret;
>>> +
>>> + ret = nvmem_cell_read_variable_le_u32(priv->dev, "base1", &base1);
>>> + if (ret < 0)
>>> + return ret;
>>> +
>>> + /* Read offset values and allocate SHIFT, SLOPE & CZERO regmap
>>> for enabled sensors */
>>> + for (i = 0; i < priv->num_sensors; i++) {
>>> + if (!(priv->sensors_to_en & (0x1 << i)))
>>> + continue;
>>> +
>>> + ret = snprintf(name, sizeof(name), "s%d_offset",
>>> priv->sensor[i].hw_id);
>>> + if (ret < 0)
>>> + return ret;
>>> +
>>> + ret = nvmem_cell_read_variable_le_u32(priv->dev, name,
>>> &priv->sensor[i].offset);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + for (j = SHIFT_0; j <= CZERO_0; j++) {
>>> + int idx = (i * 3) + j;
>>> +
>>> + priv->rf[idx] = devm_regmap_field_alloc(dev,
>>> priv->srot_map,
>>> + priv->fields[idx]);
>>> + if (IS_ERR(priv->rf[idx]))
>>> + return PTR_ERR(priv->rf[idx]);
>>
>> I think, allocating data structures for 48 regfields, which are
>> written just once, to be an overkill.
>>
>>> + }
>>> + }
>>> +
>>> + /* Based on calib mode, program SHIFT, SLOPE and CZERO for
>>> enabled sensors */
>>> + switch (mode) {
>>> + case TWO_PT_CALIB:
>>> + slope = (TWO_PT_SHIFTED_GAIN / (base1 - base0));
>>> +
>>> + for (i = 0; i < priv->num_sensors; i++) {
>>> + if (!(priv->sensors_to_en & (0x1 << i)))
>>> + continue;
>>> +
>>> + int idx = i * 3;
>>> +
>>> + czero = (base0 + priv->sensor[i].offset - ((base1 -
>>> base0) / 3));
>>> + regmap_field_write(priv->rf[SHIFT_0 + idx],
>>> V2_SHIFT_DEFAULT);
>>> + regmap_field_write(priv->rf[SLOPE_0 + idx], slope);
>>> + regmap_field_write(priv->rf[CZERO_0 + idx], czero);
>>> + }
>>> + fallthrough;
>>> + case ONE_PT_CALIB2:
>>> + for (i = 0; i < priv->num_sensors; i++) {
>>> + if (!(priv->sensors_to_en & (0x1 << i)))
>>> + continue;
>>> +
>>> + int idx = i * 3;
>>> +
>>> + czero = base0 + priv->sensor[i].offset -
>>> ONE_PT_CZERO_CONST;
>>> + regmap_field_write(priv->rf[SHIFT_0 + idx],
>>> V2_SHIFT_DEFAULT);
>>> + regmap_field_write(priv->rf[SLOPE_0 + idx], ONE_PT_SLOPE);
>>> + regmap_field_write(priv->rf[CZERO_0 + idx], czero);
>>> + }
>>> + break;
>>> + default:
>>> + dev_dbg(priv->dev, "calibrationless mode\n");
>>> + for (i = 0; i < priv->num_sensors; i++) {
>>> + if (!(priv->sensors_to_en & (0x1 << i)))
>>> + continue;
>>> +
>>> + int idx = i * 3;
>>> +
>>> + regmap_field_write(priv->rf[SHIFT_0 + idx],
>>> V2_SHIFT_DEFAULT);
>>> + regmap_field_write(priv->rf[SLOPE_0 + idx],
>>> V2_SLOPE_DEFAULT);
>>> + regmap_field_write(priv->rf[CZERO_0 + idx],
>>> V2_CZERO_DEFAULT);
>>> + }
>>> + }
>>
>> This code iterates over the sensors field several times. Please
>> consider extracting a function that handles all setup for a single
>> sensor, then calling it in a loop (I should probably do the same for
>> tsens-v0/v1 too).
>>
>>> +
>>> + return 0;
>>> +}
>>> +
>>> static const struct tsens_ops ops_generic_v2 = {
>>> .init = init_common,
>>> .get_temp = get_temp_tsens_valid,
>>> diff --git a/drivers/thermal/qcom/tsens.c
>>> b/drivers/thermal/qcom/tsens.c
>>> index 98c356acfe98..169690355dad 100644
>>> --- a/drivers/thermal/qcom/tsens.c
>>> +++ b/drivers/thermal/qcom/tsens.c
>>> @@ -974,7 +974,7 @@ int __init init_common(struct tsens_priv *priv)
>>> ret = regmap_field_read(priv->rf[TSENS_EN], &enabled);
>>> if (ret)
>>> goto err_put_device;
>>> - if (!enabled) {
>>> + if (!enabled && !priv->sensors_to_en) {
>>> dev_err(dev, "%s: device not enabled\n", __func__);
>>> ret = -ENODEV;
>>> goto err_put_device;
>>> @@ -1006,6 +1006,40 @@ int __init init_common(struct tsens_priv *priv)
>>> goto err_put_device;
>>> }
>>> + /* Do TSENS initialization if required */
>>> + if (priv->sensors_to_en) {
>>
>> Maybe it would be better to explicitly add VER_2_X_NO_RPM and check
>> it here?
>
> After taking a look at the patch 6, can we just add a separate init()
> function which will call init_common() and then initialize these fields?
Sure, will add separate init() function in tsens-v2.c
>
>>
>>> + priv->rf[CODE_OR_TEMP] = devm_regmap_field_alloc(dev,
>>> priv->srot_map,
>>> + priv->fields[CODE_OR_TEMP]);
>>> + if (IS_ERR(priv->rf[CODE_OR_TEMP])) {
>>> + ret = PTR_ERR(priv->rf[CODE_OR_TEMP]);
>>> + goto err_put_device;
>>> + }
>>> +
>>> + priv->rf[MAIN_MEASURE_PERIOD] =
>>> + devm_regmap_field_alloc(dev, priv->srot_map,
>>> + priv->fields[MAIN_MEASURE_PERIOD]);
>>> + if (IS_ERR(priv->rf[MAIN_MEASURE_PERIOD])) {
>>> + ret = PTR_ERR(priv->rf[MAIN_MEASURE_PERIOD]);
>>> + goto err_put_device;
>>> + }
>>> +
>>> + regmap_field_write(priv->rf[TSENS_SW_RST], 0x1);
>>> +
>>> + /* Update measure period to 2ms */
>>> + regmap_field_write(priv->rf[MAIN_MEASURE_PERIOD], 0x1);
>>> +
>>> + /* Enable available sensors */
>>> + regmap_field_write(priv->rf[SENSOR_EN], priv->sensors_to_en);
>>> +
>>> + /* Real temperature format */
>>> + regmap_field_write(priv->rf[CODE_OR_TEMP], 0x1);
>>> +
>>> + regmap_field_write(priv->rf[TSENS_SW_RST], 0x0);
>>> +
>>> + /* Enable TSENS */
>>> + regmap_field_write(priv->rf[TSENS_EN], 0x1);
>>> + }
>>> +
>>> /* This loop might need changes if enum regfield_ids is
>>> reordered */
>>> for (j = LAST_TEMP_0; j <= UP_THRESH_15; j += 16) {
>>> for (i = 0; i < priv->feat->max_sensors; i++) {
>>> @@ -1282,6 +1316,7 @@ static int tsens_probe(struct platform_device
>>> *pdev)
>>> priv->dev = dev;
>>> priv->num_sensors = num_sensors;
>>> + priv->sensors_to_en = data->sensors_to_en;
>>> priv->ops = data->ops;
>>> for (i = 0; i < priv->num_sensors; i++) {
>>> if (data->hw_ids)
>>> diff --git a/drivers/thermal/qcom/tsens.h
>>> b/drivers/thermal/qcom/tsens.h
>>> index 2805de1c6827..f8897bc8944e 100644
>>> --- a/drivers/thermal/qcom/tsens.h
>>> +++ b/drivers/thermal/qcom/tsens.h
>>> @@ -168,6 +168,58 @@ enum regfield_ids {
>>> TSENS_SW_RST,
>>> SENSOR_EN,
>>> CODE_OR_TEMP,
>>> + /* MEASURE_PERIOD */
>>> + MAIN_MEASURE_PERIOD,
>>> +
>>> + /* Sn_CONVERSION */
>>> + SHIFT_0,
>>> + SLOPE_0,
>>> + CZERO_0,
>>> + SHIFT_1,
>>> + SLOPE_1,
>>> + CZERO_1,
>>> + SHIFT_2,
>>> + SLOPE_2,
>>> + CZERO_2,
>>> + SHIFT_3,
>>> + SLOPE_3,
>>> + CZERO_3,
>>> + SHIFT_4,
>>> + SLOPE_4,
>>> + CZERO_4,
>>> + SHIFT_5,
>>> + SLOPE_5,
>>> + CZERO_5,
>>> + SHIFT_6,
>>> + SLOPE_6,
>>> + CZERO_6,
>>> + SHIFT_7,
>>> + SLOPE_7,
>>> + CZERO_7,
>>> + SHIFT_8,
>>> + SLOPE_8,
>>> + CZERO_8,
>>> + SHIFT_9,
>>> + SLOPE_9,
>>> + CZERO_9,
>>> + SHIFT_10,
>>> + SLOPE_10,
>>> + CZERO_10,
>>> + SHIFT_11,
>>> + SLOPE_11,
>>> + CZERO_11,
>>> + SHIFT_12,
>>> + SLOPE_12,
>>> + CZERO_12,
>>> + SHIFT_13,
>>> + SLOPE_13,
>>> + CZERO_13,
>>> + SHIFT_14,
>>> + SLOPE_14,
>>> + CZERO_14,
>>> + SHIFT_15,
>>> + SLOPE_15,
>>> + CZERO_15,
>>> /* ----- TM ------ */
>>> /* TRDY */
>>> @@ -524,6 +576,7 @@ struct tsens_features {
>>> /**
>>> * struct tsens_plat_data - tsens compile-time platform data
>>> * @num_sensors: Number of sensors supported by platform
>>> + * @sensors_to_en: Sensors to be enabled. Each bit represent a sensor
>>> * @ops: operations the tsens instance supports
>>> * @hw_ids: Subset of sensors ids supported by platform, if not
>>> the first n
>>> * @feat: features of the IP
>>> @@ -531,6 +584,7 @@ struct tsens_features {
>>> */
>>> struct tsens_plat_data {
>>> const u32 num_sensors;
>>> + const u16 sensors_to_en;
>>
>> There is already a similar field, hw_ids. Can it be used instead?
>>
>>> const struct tsens_ops *ops;
>>> unsigned int *hw_ids;
>>> struct tsens_features *feat;
>>> @@ -551,6 +605,7 @@ struct tsens_context {
>>> * struct tsens_priv - private data for each instance of the tsens IP
>>> * @dev: pointer to struct device
>>> * @num_sensors: number of sensors enabled on this device
>>> + * @sensors_to_en: sensors to be enabled. Each bit represents a sensor
>>> * @tm_map: pointer to TM register address space
>>> * @srot_map: pointer to SROT register address space
>>> * @tm_offset: deal with old device trees that don't address TM
>>> and SROT
>>> @@ -569,6 +624,7 @@ struct tsens_context {
>>> struct tsens_priv {
>>> struct device *dev;
>>> u32 num_sensors;
>>> + u16 sensors_to_en;
>>> struct regmap *tm_map;
>>> struct regmap *srot_map;
>>> u32 tm_offset;
>>
>
--
Thanks,
Praveenkumar
On 7/10/2023 7:04 PM, Praveenkumar I wrote:
>
> On 7/10/2023 5:44 PM, Konrad Dybcio wrote:
>> On 10.07.2023 13:23, Dmitry Baryshkov wrote:
>>> On 10/07/2023 13:37, Praveenkumar I wrote:
>>>> This patch adds thermal zone nodes for sensors present in
>>>> IPQ5332.
>>>>
>>>> Signed-off-by: Praveenkumar I <[email protected]>
>>>> ---
>>>> arch/arm64/boot/dts/qcom/ipq5332.dtsi | 72
>>>> +++++++++++++++++++++++++++
>>>> 1 file changed, 72 insertions(+)
>>>>
>>>> diff --git a/arch/arm64/boot/dts/qcom/ipq5332.dtsi
>>>> b/arch/arm64/boot/dts/qcom/ipq5332.dtsi
>>>> index a1e3527178c0..8b276aeca53e 100644
>>>> --- a/arch/arm64/boot/dts/qcom/ipq5332.dtsi
>>>> +++ b/arch/arm64/boot/dts/qcom/ipq5332.dtsi
>>>> @@ -527,4 +527,76 @@ timer {
>>>> <GIC_PPI 4 (GIC_CPU_MASK_SIMPLE(4) |
>>>> IRQ_TYPE_LEVEL_LOW)>,
>>>> <GIC_PPI 1 (GIC_CPU_MASK_SIMPLE(4) |
>>>> IRQ_TYPE_LEVEL_LOW)>;
>>>> };
>>>> +
>>>> + thermal-zones {
>>>> + rfa-0-thermal{
>> thermal {
> In all other DTS, name is 'thermal-zones". Hence followed the same.
Sorry, understood now. Will give space after "rfa-0-thermal"
>>
>>>> + polling-delay-passive = <0>;
>>>> + polling-delay = <0>;
>>>> + thermal-sensors = <&tsens 11>;
>>>> +
>>>> + trips {
>> Indentation seems off, tab size for kernel code is 8 spaces.
> Sure, will check the indent and update in next patch.
>>
>> Konrad
>>>> + rfa-0-critical {
>>>> + temperature = <125000>;
>>>> + hysteresis = <1000>;
>>>> + type = "critical";
>>>> + };
>>>> + };
>>>> + };
>>>> +
>>>> + rfa-1-thermal {
>>>> + polling-delay-passive = <0>;
>>>> + polling-delay = <0>;
>>>> + thermal-sensors = <&tsens 12>;
>>>> +
>>>> + trips {
>>>> + rfa-1-critical {
>>>> + temperature = <125000>;
>>>> + hysteresis = <1000>;
>>>> + type = "critical";
>>>> + };
>>>> + };
>>>> + };
>>>> +
>>>> + misc-thermal {
>>>> + polling-delay-passive = <0>;
>>>> + polling-delay = <0>;
>>>> + thermal-sensors = <&tsens 13>;
>>>> +
>>>> + trips {
>>>> + misc-critical {
>>>> + temperature = <125000>;
>>>> + hysteresis = <1000>;
>>>> + type = "critical";
>>>> + };
>>>> + };
>>>> + };
>>>> +
>>>> + cpu-top-thermal {
>>>> + polling-delay-passive = <0>;
>>>> + polling-delay = <0>;
>>>> + thermal-sensors = <&tsens 14>;
>>>> +
>>>> + trips {
>>>> + cpu-top-critical {
>>>> + temperature = <125000>;
>>>> + hysteresis = <1000>;
>>>> + type = "critical";
>>>> + };
>>>> + };
>>> Could you please add a passive cooling devices for the CPU?
>>>
>>>> + };
>>>> +
>>>> + top-glue-thermal {
>>>> + polling-delay-passive = <0>;
>>>> + polling-delay = <0>;
>>>> + thermal-sensors = <&tsens 15>;
>>>> +
>>>> + trips {
>>>> + top-glue-critical {
>>>> + temperature = <125000>;
>>>> + hysteresis = <1000>;
>>>> + type = "critical";
>>>> + };
>>>> + };
>>>> + };
>>>> + };
>>>> };
> --
> Thanks,
> Praveenkumar
On 7/10/2023 4:54 PM, Dmitry Baryshkov wrote:
> On 10/07/2023 13:37, Praveenkumar I wrote:
>> IPQ5332 uses tsens v2.3.3 IP and it is having combined interrupt as
>> like IPQ8074. But as the SoCs does not have RPM, kernel needs to
>> take care of sensor enablement and calibration. Hence introduced
>> new ops and data for IPQ5332 and reused the feature_config from
>> IPQ8074.
>>
>> Signed-off-by: Praveenkumar I <[email protected]>
>> ---
>> drivers/thermal/qcom/tsens-v2.c | 13 +++++++++++++
>> drivers/thermal/qcom/tsens.c | 3 +++
>> drivers/thermal/qcom/tsens.h | 2 +-
>> 3 files changed, 17 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/thermal/qcom/tsens-v2.c
>> b/drivers/thermal/qcom/tsens-v2.c
>> index db48b1d95348..8b6e3876fd2c 100644
>> --- a/drivers/thermal/qcom/tsens-v2.c
>> +++ b/drivers/thermal/qcom/tsens-v2.c
>> @@ -237,6 +237,19 @@ struct tsens_plat_data data_ipq8074 = {
>> .fields = tsens_v2_regfields,
>> };
>> +static const struct tsens_ops ops_ipq5332_v2 = {
>
> Please drop v2. It is unclear if it refers to tsens being v2 or being
> specific to ipq5332 v2.
Sure, will drop v2.
>
>> + .init = init_common,
>> + .get_temp = get_temp_tsens_valid,
>> + .calibrate = tsens_v2_calibration,
>> +};
>> +
>> +struct tsens_plat_data data_ipq5332 = {
>> + .sensors_to_en = 0xF800,
>
> This doesn't seem to match the offsets that you have enabled in the DTSI.
In order to overcome the DT binding check failure, added all the
available QFPROM offsets in the DTSI. Else DT binding check failing on
"nvmem-cell-names".
>
>> + .ops = &ops_ipq5332_v2,
>> + .feat = &ipq8074_feat,
>> + .fields = tsens_v2_regfields,
>> +};
>> +
>> /* Kept around for backward compatibility with old msm8996.dtsi */
>> struct tsens_plat_data data_8996 = {
>> .num_sensors = 13,
>> diff --git a/drivers/thermal/qcom/tsens.c b/drivers/thermal/qcom/tsens.c
>> index 169690355dad..e8ba2901cda8 100644
>> --- a/drivers/thermal/qcom/tsens.c
>> +++ b/drivers/thermal/qcom/tsens.c
>> @@ -1140,6 +1140,9 @@ static const struct of_device_id tsens_table[] = {
>> }, {
>> .compatible = "qcom,ipq8074-tsens",
>> .data = &data_ipq8074,
>> + }, {
>> + .compatible = "qcom,ipq5332-tsens",
>> + .data = &data_ipq5332,
>> }, {
>> .compatible = "qcom,mdm9607-tsens",
>> .data = &data_9607,
>> diff --git a/drivers/thermal/qcom/tsens.h b/drivers/thermal/qcom/tsens.h
>> index f8897bc8944e..36040f9beebc 100644
>> --- a/drivers/thermal/qcom/tsens.h
>> +++ b/drivers/thermal/qcom/tsens.h
>> @@ -701,6 +701,6 @@ extern struct tsens_plat_data data_8226,
>> data_8909, data_8916, data_8939, data_8
>> extern struct tsens_plat_data data_tsens_v1, data_8976, data_8956;
>> /* TSENS v2 targets */
>> -extern struct tsens_plat_data data_8996, data_ipq8074, data_tsens_v2;
>> +extern struct tsens_plat_data data_8996, data_ipq8074, data_ipq5332,
>> data_tsens_v2;
>> #endif /* __QCOM_TSENS_H__ */
>
--
Thanks,
Praveenkumar
On Mon, 10 Jul 2023 at 16:22, Praveenkumar I <[email protected]> wrote:
>
>
> On 7/10/2023 4:49 PM, Dmitry Baryshkov wrote:
> > On 10/07/2023 13:37, Praveenkumar I wrote:
> >> SoCs without RPM have to enable sensors and calibrate from the kernel.
> >> Though TSENS IP supports 16 sensors, not all are used. So added
> >> sensors_to_en in tsens data help enable the relevant sensors.
> >>
> >> Added new calibration function for V2 as the tsens.c calib function
> >> only supports V1.
> >>
> >> Signed-off-by: Praveenkumar I <[email protected]>
> >> ---
> >> drivers/thermal/qcom/tsens-v2.c | 116 ++++++++++++++++++++++++++++++++
> >> drivers/thermal/qcom/tsens.c | 37 +++++++++-
> >> drivers/thermal/qcom/tsens.h | 56 +++++++++++++++
> >> 3 files changed, 208 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/thermal/qcom/tsens-v2.c
> >> b/drivers/thermal/qcom/tsens-v2.c
> >> index 29a61d2d6ca3..db48b1d95348 100644
> >> --- a/drivers/thermal/qcom/tsens-v2.c
> >> +++ b/drivers/thermal/qcom/tsens-v2.c
> >> @@ -6,11 +6,20 @@
> >> #include <linux/bitops.h>
> >> #include <linux/regmap.h>
> >> +#include <linux/nvmem-consumer.h>
> >> #include "tsens.h"
> >> /* ----- SROT ------ */
> >> #define SROT_HW_VER_OFF 0x0000
> >> #define SROT_CTRL_OFF 0x0004
> >> +#define SROT_MEASURE_PERIOD 0x0008
> >> +#define SROT_Sn_CONVERSION 0x0060
> >> +#define V2_SHIFT_DEFAULT 0x0003
> >> +#define V2_SLOPE_DEFAULT 0x0cd0
> >> +#define V2_CZERO_DEFAULT 0x016a
> >> +#define ONE_PT_SLOPE 0x0cd0
> >> +#define TWO_PT_SHIFTED_GAIN 921600
> >> +#define ONE_PT_CZERO_CONST 94
> >> /* ----- TM ------ */
> >> #define TM_INT_EN_OFF 0x0004
> >> @@ -59,6 +68,16 @@ static const struct reg_field
> >> tsens_v2_regfields[MAX_REGFIELDS] = {
> >> /* CTRL_OFF */
> >> [TSENS_EN] = REG_FIELD(SROT_CTRL_OFF, 0, 0),
> >> [TSENS_SW_RST] = REG_FIELD(SROT_CTRL_OFF, 1, 1),
> >> + [SENSOR_EN] = REG_FIELD(SROT_CTRL_OFF, 3, 18),
> >> + [CODE_OR_TEMP] = REG_FIELD(SROT_CTRL_OFF, 21, 21),
> >> +
> >> + /* MAIN_MEASURE_PERIOD */
> >> + [MAIN_MEASURE_PERIOD] = REG_FIELD(SROT_MEASURE_PERIOD, 0, 7),
> >> +
> >> + /* Sn Conversion */
> >> + REG_FIELD_FOR_EACH_SENSOR16(SHIFT, SROT_Sn_CONVERSION, 23, 24),
> >> + REG_FIELD_FOR_EACH_SENSOR16(SLOPE, SROT_Sn_CONVERSION, 10, 22),
> >> + REG_FIELD_FOR_EACH_SENSOR16(CZERO, SROT_Sn_CONVERSION, 0, 9),
> >> /* ----- TM ------ */
> >> /* INTERRUPT ENABLE */
> >> @@ -104,6 +123,103 @@ static const struct reg_field
> >> tsens_v2_regfields[MAX_REGFIELDS] = {
> >> [TRDY] = REG_FIELD(TM_TRDY_OFF, 0, 0),
> >> };
> >> +static int tsens_v2_calibration(struct tsens_priv *priv)
> >> +{
> >> + struct device *dev = priv->dev;
> >> + u32 mode, base0, base1;
> >> + u32 slope, czero;
> >> + char name[15];
> >> + int i, j, ret;
> >> +
> >> + if (priv->num_sensors > MAX_SENSORS)
> >> + return -EINVAL;
> >> +
> >> + ret = nvmem_cell_read_variable_le_u32(priv->dev, "mode", &mode);
> >> + if (ret == -ENOENT)
> >> + dev_warn(priv->dev, "Calibration data not present in DT\n");
> >> + if (ret < 0)
> >> + return ret;
> >> +
> >> + dev_dbg(priv->dev, "calibration mode is %d\n", mode);
> >> +
> >> + ret = nvmem_cell_read_variable_le_u32(priv->dev, "base0", &base0);
> >> + if (ret < 0)
> >> + return ret;
> >> +
> >> + ret = nvmem_cell_read_variable_le_u32(priv->dev, "base1", &base1);
> >> + if (ret < 0)
> >> + return ret;
> >> +
> >> + /* Read offset values and allocate SHIFT, SLOPE & CZERO regmap
> >> for enabled sensors */
> >> + for (i = 0; i < priv->num_sensors; i++) {
> >> + if (!(priv->sensors_to_en & (0x1 << i)))
> >> + continue;
> >> +
> >> + ret = snprintf(name, sizeof(name), "s%d_offset",
> >> priv->sensor[i].hw_id);
> >> + if (ret < 0)
> >> + return ret;
> >> +
> >> + ret = nvmem_cell_read_variable_le_u32(priv->dev, name,
> >> &priv->sensor[i].offset);
> >> + if (ret)
> >> + return ret;
> >> +
> >> + for (j = SHIFT_0; j <= CZERO_0; j++) {
> >> + int idx = (i * 3) + j;
> >> +
> >> + priv->rf[idx] = devm_regmap_field_alloc(dev,
> >> priv->srot_map,
> >> + priv->fields[idx]);
> >> + if (IS_ERR(priv->rf[idx]))
> >> + return PTR_ERR(priv->rf[idx]);
> >
> > I think, allocating data structures for 48 regfields, which are
> > written just once, to be an overkill.
> Can we change it to single field for each sensor. For example,
> CONVERSION_0 instead of SHIFT_0, SLOPE_0 and CZERO_0? This way it will
> be max 16 regfields.
If you move writing of the registers to the loop, you won't need
regfields. You can just call regmap_update_bits. The point is that you
don't have to allocate a one-time instance.
> >
> >> + }
> >> + }
> >> +
> >> + /* Based on calib mode, program SHIFT, SLOPE and CZERO for
> >> enabled sensors */
> >> + switch (mode) {
> >> + case TWO_PT_CALIB:
> >> + slope = (TWO_PT_SHIFTED_GAIN / (base1 - base0));
> >> +
> >> + for (i = 0; i < priv->num_sensors; i++) {
> >> + if (!(priv->sensors_to_en & (0x1 << i)))
> >> + continue;
> >> +
> >> + int idx = i * 3;
> >> +
> >> + czero = (base0 + priv->sensor[i].offset - ((base1 -
> >> base0) / 3));
> >> + regmap_field_write(priv->rf[SHIFT_0 + idx],
> >> V2_SHIFT_DEFAULT);
> >> + regmap_field_write(priv->rf[SLOPE_0 + idx], slope);
> >> + regmap_field_write(priv->rf[CZERO_0 + idx], czero);
> >> + }
> >> + fallthrough;
> >> + case ONE_PT_CALIB2:
> >> + for (i = 0; i < priv->num_sensors; i++) {
> >> + if (!(priv->sensors_to_en & (0x1 << i)))
> >> + continue;
> >> +
> >> + int idx = i * 3;
> >> +
> >> + czero = base0 + priv->sensor[i].offset -
> >> ONE_PT_CZERO_CONST;
> >> + regmap_field_write(priv->rf[SHIFT_0 + idx],
> >> V2_SHIFT_DEFAULT);
> >> + regmap_field_write(priv->rf[SLOPE_0 + idx], ONE_PT_SLOPE);
> >> + regmap_field_write(priv->rf[CZERO_0 + idx], czero);
> >> + }
> >> + break;
> >> + default:
> >> + dev_dbg(priv->dev, "calibrationless mode\n");
> >> + for (i = 0; i < priv->num_sensors; i++) {
> >> + if (!(priv->sensors_to_en & (0x1 << i)))
> >> + continue;
> >> +
> >> + int idx = i * 3;
> >> +
> >> + regmap_field_write(priv->rf[SHIFT_0 + idx],
> >> V2_SHIFT_DEFAULT);
> >> + regmap_field_write(priv->rf[SLOPE_0 + idx],
> >> V2_SLOPE_DEFAULT);
> >> + regmap_field_write(priv->rf[CZERO_0 + idx],
> >> V2_CZERO_DEFAULT);
> >> + }
> >> + }
> >
> > This code iterates over the sensors field several times. Please
> > consider extracting a function that handles all setup for a single
> > sensor, then calling it in a loop (I should probably do the same for
> > tsens-v0/v1 too).
> Sure. After reading the mode0, base0 and base1 from QFPROM, we can call
> a function in a loop to setup the calibration for each sensor.
> >
> >> +
> >> + return 0;
> >> +}
> >> +
> >> static const struct tsens_ops ops_generic_v2 = {
> >> .init = init_common,
> >> .get_temp = get_temp_tsens_valid,
> >> diff --git a/drivers/thermal/qcom/tsens.c b/drivers/thermal/qcom/tsens.c
> >> index 98c356acfe98..169690355dad 100644
> >> --- a/drivers/thermal/qcom/tsens.c
> >> +++ b/drivers/thermal/qcom/tsens.c
> >> @@ -974,7 +974,7 @@ int __init init_common(struct tsens_priv *priv)
> >> ret = regmap_field_read(priv->rf[TSENS_EN], &enabled);
> >> if (ret)
> >> goto err_put_device;
> >> - if (!enabled) {
> >> + if (!enabled && !priv->sensors_to_en) {
> >> dev_err(dev, "%s: device not enabled\n", __func__);
> >> ret = -ENODEV;
> >> goto err_put_device;
> >> @@ -1006,6 +1006,40 @@ int __init init_common(struct tsens_priv *priv)
> >> goto err_put_device;
> >> }
> >> + /* Do TSENS initialization if required */
> >> + if (priv->sensors_to_en) {
> >
> > Maybe it would be better to explicitly add VER_2_X_NO_RPM and check it
> > here?
> Sure, will add a separate version macro.
> >
> >> + priv->rf[CODE_OR_TEMP] = devm_regmap_field_alloc(dev,
> >> priv->srot_map,
> >> + priv->fields[CODE_OR_TEMP]);
> >> + if (IS_ERR(priv->rf[CODE_OR_TEMP])) {
> >> + ret = PTR_ERR(priv->rf[CODE_OR_TEMP]);
> >> + goto err_put_device;
> >> + }
> >> +
> >> + priv->rf[MAIN_MEASURE_PERIOD] =
> >> + devm_regmap_field_alloc(dev, priv->srot_map,
> >> + priv->fields[MAIN_MEASURE_PERIOD]);
> >> + if (IS_ERR(priv->rf[MAIN_MEASURE_PERIOD])) {
> >> + ret = PTR_ERR(priv->rf[MAIN_MEASURE_PERIOD]);
> >> + goto err_put_device;
> >> + }
> >> +
> >> + regmap_field_write(priv->rf[TSENS_SW_RST], 0x1);
> >> +
> >> + /* Update measure period to 2ms */
> >> + regmap_field_write(priv->rf[MAIN_MEASURE_PERIOD], 0x1);
> >> +
> >> + /* Enable available sensors */
> >> + regmap_field_write(priv->rf[SENSOR_EN], priv->sensors_to_en);
> >> +
> >> + /* Real temperature format */
> >> + regmap_field_write(priv->rf[CODE_OR_TEMP], 0x1);
> >> +
> >> + regmap_field_write(priv->rf[TSENS_SW_RST], 0x0);
> >> +
> >> + /* Enable TSENS */
> >> + regmap_field_write(priv->rf[TSENS_EN], 0x1);
> >> + }
> >> +
> >> /* This loop might need changes if enum regfield_ids is
> >> reordered */
> >> for (j = LAST_TEMP_0; j <= UP_THRESH_15; j += 16) {
> >> for (i = 0; i < priv->feat->max_sensors; i++) {
> >> @@ -1282,6 +1316,7 @@ static int tsens_probe(struct platform_device
> >> *pdev)
> >> priv->dev = dev;
> >> priv->num_sensors = num_sensors;
> >> + priv->sensors_to_en = data->sensors_to_en;
> >> priv->ops = data->ops;
> >> for (i = 0; i < priv->num_sensors; i++) {
> >> if (data->hw_ids)
> >> diff --git a/drivers/thermal/qcom/tsens.h b/drivers/thermal/qcom/tsens.h
> >> index 2805de1c6827..f8897bc8944e 100644
> >> --- a/drivers/thermal/qcom/tsens.h
> >> +++ b/drivers/thermal/qcom/tsens.h
> >> @@ -168,6 +168,58 @@ enum regfield_ids {
> >> TSENS_SW_RST,
> >> SENSOR_EN,
> >> CODE_OR_TEMP,
> >> + /* MEASURE_PERIOD */
> >> + MAIN_MEASURE_PERIOD,
> >> +
> >> + /* Sn_CONVERSION */
> >> + SHIFT_0,
> >> + SLOPE_0,
> >> + CZERO_0,
> >> + SHIFT_1,
> >> + SLOPE_1,
> >> + CZERO_1,
> >> + SHIFT_2,
> >> + SLOPE_2,
> >> + CZERO_2,
> >> + SHIFT_3,
> >> + SLOPE_3,
> >> + CZERO_3,
> >> + SHIFT_4,
> >> + SLOPE_4,
> >> + CZERO_4,
> >> + SHIFT_5,
> >> + SLOPE_5,
> >> + CZERO_5,
> >> + SHIFT_6,
> >> + SLOPE_6,
> >> + CZERO_6,
> >> + SHIFT_7,
> >> + SLOPE_7,
> >> + CZERO_7,
> >> + SHIFT_8,
> >> + SLOPE_8,
> >> + CZERO_8,
> >> + SHIFT_9,
> >> + SLOPE_9,
> >> + CZERO_9,
> >> + SHIFT_10,
> >> + SLOPE_10,
> >> + CZERO_10,
> >> + SHIFT_11,
> >> + SLOPE_11,
> >> + CZERO_11,
> >> + SHIFT_12,
> >> + SLOPE_12,
> >> + CZERO_12,
> >> + SHIFT_13,
> >> + SLOPE_13,
> >> + CZERO_13,
> >> + SHIFT_14,
> >> + SLOPE_14,
> >> + CZERO_14,
> >> + SHIFT_15,
> >> + SLOPE_15,
> >> + CZERO_15,
> >> /* ----- TM ------ */
> >> /* TRDY */
> >> @@ -524,6 +576,7 @@ struct tsens_features {
> >> /**
> >> * struct tsens_plat_data - tsens compile-time platform data
> >> * @num_sensors: Number of sensors supported by platform
> >> + * @sensors_to_en: Sensors to be enabled. Each bit represent a sensor
> >> * @ops: operations the tsens instance supports
> >> * @hw_ids: Subset of sensors ids supported by platform, if not the
> >> first n
> >> * @feat: features of the IP
> >> @@ -531,6 +584,7 @@ struct tsens_features {
> >> */
> >> struct tsens_plat_data {
> >> const u32 num_sensors;
> >> + const u16 sensors_to_en;
> >
> > There is already a similar field, hw_ids. Can it be used instead?
> Yes, it can be used. I missed to check this hw_ids. Will change the
> num_sensors to 5 and use the hw_ids.
> >
> >> const struct tsens_ops *ops;
> >> unsigned int *hw_ids;
> >> struct tsens_features *feat;
> >> @@ -551,6 +605,7 @@ struct tsens_context {
> >> * struct tsens_priv - private data for each instance of the tsens IP
> >> * @dev: pointer to struct device
> >> * @num_sensors: number of sensors enabled on this device
> >> + * @sensors_to_en: sensors to be enabled. Each bit represents a sensor
> >> * @tm_map: pointer to TM register address space
> >> * @srot_map: pointer to SROT register address space
> >> * @tm_offset: deal with old device trees that don't address TM and
> >> SROT
> >> @@ -569,6 +624,7 @@ struct tsens_context {
> >> struct tsens_priv {
> >> struct device *dev;
> >> u32 num_sensors;
> >> + u16 sensors_to_en;
> >> struct regmap *tm_map;
> >> struct regmap *srot_map;
> >> u32 tm_offset;
> >
> --
> Thanks,
> Praveenkumar
--
With best wishes
Dmitry
On Mon, 10 Jul 2023 at 16:47, Praveenkumar I <[email protected]> wrote:
>
>
> On 7/10/2023 4:54 PM, Dmitry Baryshkov wrote:
> > On 10/07/2023 13:37, Praveenkumar I wrote:
> >> IPQ5332 uses tsens v2.3.3 IP and it is having combined interrupt as
> >> like IPQ8074. But as the SoCs does not have RPM, kernel needs to
> >> take care of sensor enablement and calibration. Hence introduced
> >> new ops and data for IPQ5332 and reused the feature_config from
> >> IPQ8074.
> >>
> >> Signed-off-by: Praveenkumar I <[email protected]>
> >> ---
> >> drivers/thermal/qcom/tsens-v2.c | 13 +++++++++++++
> >> drivers/thermal/qcom/tsens.c | 3 +++
> >> drivers/thermal/qcom/tsens.h | 2 +-
> >> 3 files changed, 17 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/thermal/qcom/tsens-v2.c
> >> b/drivers/thermal/qcom/tsens-v2.c
> >> index db48b1d95348..8b6e3876fd2c 100644
> >> --- a/drivers/thermal/qcom/tsens-v2.c
> >> +++ b/drivers/thermal/qcom/tsens-v2.c
> >> @@ -237,6 +237,19 @@ struct tsens_plat_data data_ipq8074 = {
> >> .fields = tsens_v2_regfields,
> >> };
> >> +static const struct tsens_ops ops_ipq5332_v2 = {
> >
> > Please drop v2. It is unclear if it refers to tsens being v2 or being
> > specific to ipq5332 v2.
> Sure, will drop v2.
> >
> >> + .init = init_common,
> >> + .get_temp = get_temp_tsens_valid,
> >> + .calibrate = tsens_v2_calibration,
> >> +};
> >> +
> >> +struct tsens_plat_data data_ipq5332 = {
> >> + .sensors_to_en = 0xF800,
> >
> > This doesn't seem to match the offsets that you have enabled in the DTSI.
> In order to overcome the DT binding check failure, added all the
> available QFPROM offsets in the DTSI. Else DT binding check failing on
> "nvmem-cell-names".
This is not a way to overcome issues in DT bindings. Please fix DT
bindings instead by using alternatives, enums, etc.
> >
> >> + .ops = &ops_ipq5332_v2,
> >> + .feat = &ipq8074_feat,
> >> + .fields = tsens_v2_regfields,
> >> +};
> >> +
> >> /* Kept around for backward compatibility with old msm8996.dtsi */
> >> struct tsens_plat_data data_8996 = {
> >> .num_sensors = 13,
> >> diff --git a/drivers/thermal/qcom/tsens.c b/drivers/thermal/qcom/tsens.c
> >> index 169690355dad..e8ba2901cda8 100644
> >> --- a/drivers/thermal/qcom/tsens.c
> >> +++ b/drivers/thermal/qcom/tsens.c
> >> @@ -1140,6 +1140,9 @@ static const struct of_device_id tsens_table[] = {
> >> }, {
> >> .compatible = "qcom,ipq8074-tsens",
> >> .data = &data_ipq8074,
> >> + }, {
> >> + .compatible = "qcom,ipq5332-tsens",
> >> + .data = &data_ipq5332,
> >> }, {
> >> .compatible = "qcom,mdm9607-tsens",
> >> .data = &data_9607,
> >> diff --git a/drivers/thermal/qcom/tsens.h b/drivers/thermal/qcom/tsens.h
> >> index f8897bc8944e..36040f9beebc 100644
> >> --- a/drivers/thermal/qcom/tsens.h
> >> +++ b/drivers/thermal/qcom/tsens.h
> >> @@ -701,6 +701,6 @@ extern struct tsens_plat_data data_8226,
> >> data_8909, data_8916, data_8939, data_8
> >> extern struct tsens_plat_data data_tsens_v1, data_8976, data_8956;
> >> /* TSENS v2 targets */
> >> -extern struct tsens_plat_data data_8996, data_ipq8074, data_tsens_v2;
> >> +extern struct tsens_plat_data data_8996, data_ipq8074, data_ipq5332,
> >> data_tsens_v2;
> >> #endif /* __QCOM_TSENS_H__ */
> >
> --
> Thanks,
> Praveenkumar
--
With best wishes
Dmitry
On 10/07/2023 12:37, Praveenkumar I wrote:
> IPQ5332 uses TSENS v2.3.3 with combined interrupt. RPM is not
> available in the SoC, hence adding new compatible to have the
> sensor enablement and calibration function.>
> Signed-off-by: Praveenkumar I <[email protected]>
> ---
> Documentation/devicetree/bindings/thermal/qcom-tsens.yaml | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml b/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml
> index 8b7863c3989e..ee57713f6131 100644
> --- a/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml
> +++ b/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml
> @@ -68,8 +68,10 @@ properties:
> - const: qcom,tsens-v2
>
> - description: v2 of TSENS with combined interrupt
> - enum:
> - - qcom,ipq8074-tsens
> + items:
Drop items, you do not have multiple items.
> + - enum:
> + - qcom,ipq8074-tsens
> + - qcom,ipq5332-tsens
Keep the order.
>
> - description: v2 of TSENS with combined interrupt
> items:
> @@ -289,6 +291,7 @@ allOf:
> contains:
> enum:
> - qcom,ipq8074-tsens
> + - qcom,ipq5332-tsens
And here
> then:
> properties:
> interrupts:
> @@ -304,6 +307,7 @@ allOf:
> contains:
> enum:
> - qcom,ipq8074-tsens
> + - qcom,ipq5332-tsens
And here.
> - qcom,tsens-v0_1
> - qcom,tsens-v1
> - qcom,tsens-v2
Best regards,
Krzysztof
On 10/07/2023 12:37, Praveenkumar I wrote:
> Add TSENS V2 calibration nvmem cells for IPQ5332
>
> Signed-off-by: Praveenkumar I <[email protected]>
> ---
> .../bindings/thermal/qcom-tsens.yaml | 26 +++++++++++++++++--
> 1 file changed, 24 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml b/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml
> index 27e9e16e6455..8b7863c3989e 100644
> --- a/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml
> +++ b/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml
> @@ -91,7 +91,7 @@ properties:
> maxItems: 2
>
> nvmem-cells:
> - oneOf:
> + anyOf:
> - minItems: 1
> maxItems: 2
> description:
> @@ -106,9 +106,13 @@ properties:
> description: |
> Reference to nvmem cells for the calibration mode, two calibration
> bases and two cells per each sensor, main and backup copies, plus use_backup cell
> + - maxItems: 17
> + description: |
> + V2 of TSENS, reference to nvmem cells for the calibration mode, two calibration
> + bases and one cell per each sensor
I think this is already included in one of the previous entries.
Otherwise, are you sure that all new devices will have exactly 17 entries?
>
> nvmem-cell-names:
> - oneOf:
> + anyOf:
> - minItems: 1
> items:
> - const: calib
> @@ -205,6 +209,24 @@ properties:
> - const: s9_p2_backup
> - const: s10_p1_backup
> - const: s10_p2_backup
> + - items:
> + - const: mode
> + - const: base0
> + - const: base1
> + - const: s0_offset
> + - const: s3_offset
> + - const: s4_offset
> + - const: s5_offset
> + - const: s6_offset
> + - const: s7_offset
> + - const: s8_offset
> + - const: s9_offset
> + - const: s10_offset
> + - const: s11_offset
> + - const: s12_offset
> + - const: s13_offset
> + - const: s14_offset
> + - const: s15_offset
Don't introduce new naming style. Existing uses s[0-9]+, without offset
suffix. Why this should be different?
>
> "#qcom,sensors":
> description:
Best regards,
Krzysztof
On 7/11/2023 1:36 AM, Krzysztof Kozlowski wrote:
> On 10/07/2023 12:37, Praveenkumar I wrote:
>> IPQ5332 uses TSENS v2.3.3 with combined interrupt. RPM is not
>> available in the SoC, hence adding new compatible to have the
>> sensor enablement and calibration function.>
>> Signed-off-by: Praveenkumar I <[email protected]>
>> ---
>> Documentation/devicetree/bindings/thermal/qcom-tsens.yaml | 8 ++++++--
>> 1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml b/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml
>> index 8b7863c3989e..ee57713f6131 100644
>> --- a/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml
>> +++ b/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml
>> @@ -68,8 +68,10 @@ properties:
>> - const: qcom,tsens-v2
>>
>> - description: v2 of TSENS with combined interrupt
>> - enum:
>> - - qcom,ipq8074-tsens
>> + items:
> Drop items, you do not have multiple items.
Sure, will drop items.
>
>
>> + - enum:
>> + - qcom,ipq8074-tsens
>> + - qcom,ipq5332-tsens
> Keep the order.
>>
>> - description: v2 of TSENS with combined interrupt
>> items:
>> @@ -289,6 +291,7 @@ allOf:
>> contains:
>> enum:
>> - qcom,ipq8074-tsens
>> + - qcom,ipq5332-tsens
> And here
>
>> then:
>> properties:
>> interrupts:
>> @@ -304,6 +307,7 @@ allOf:
>> contains:
>> enum:
>> - qcom,ipq8074-tsens
>> + - qcom,ipq5332-tsens
> And here.
Sure, will keep the order.
--
Thanks,
Praveenkumar
>
>> - qcom,tsens-v0_1
>> - qcom,tsens-v1
>> - qcom,tsens-v2
> Best regards,
> Krzysztof
>
On 7/10/2023 8:32 PM, Dmitry Baryshkov wrote:
> On Mon, 10 Jul 2023 at 16:22, Praveenkumar I <[email protected]> wrote:
>>
>> On 7/10/2023 4:49 PM, Dmitry Baryshkov wrote:
>>> On 10/07/2023 13:37, Praveenkumar I wrote:
>>>> SoCs without RPM have to enable sensors and calibrate from the kernel.
>>>> Though TSENS IP supports 16 sensors, not all are used. So added
>>>> sensors_to_en in tsens data help enable the relevant sensors.
>>>>
>>>> Added new calibration function for V2 as the tsens.c calib function
>>>> only supports V1.
>>>>
>>>> Signed-off-by: Praveenkumar I <[email protected]>
>>>> ---
>>>> drivers/thermal/qcom/tsens-v2.c | 116 ++++++++++++++++++++++++++++++++
>>>> drivers/thermal/qcom/tsens.c | 37 +++++++++-
>>>> drivers/thermal/qcom/tsens.h | 56 +++++++++++++++
>>>> 3 files changed, 208 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/thermal/qcom/tsens-v2.c
>>>> b/drivers/thermal/qcom/tsens-v2.c
>>>> index 29a61d2d6ca3..db48b1d95348 100644
>>>> --- a/drivers/thermal/qcom/tsens-v2.c
>>>> +++ b/drivers/thermal/qcom/tsens-v2.c
>>>> @@ -6,11 +6,20 @@
>>>> #include <linux/bitops.h>
>>>> #include <linux/regmap.h>
>>>> +#include <linux/nvmem-consumer.h>
>>>> #include "tsens.h"
>>>> /* ----- SROT ------ */
>>>> #define SROT_HW_VER_OFF 0x0000
>>>> #define SROT_CTRL_OFF 0x0004
>>>> +#define SROT_MEASURE_PERIOD 0x0008
>>>> +#define SROT_Sn_CONVERSION 0x0060
>>>> +#define V2_SHIFT_DEFAULT 0x0003
>>>> +#define V2_SLOPE_DEFAULT 0x0cd0
>>>> +#define V2_CZERO_DEFAULT 0x016a
>>>> +#define ONE_PT_SLOPE 0x0cd0
>>>> +#define TWO_PT_SHIFTED_GAIN 921600
>>>> +#define ONE_PT_CZERO_CONST 94
>>>> /* ----- TM ------ */
>>>> #define TM_INT_EN_OFF 0x0004
>>>> @@ -59,6 +68,16 @@ static const struct reg_field
>>>> tsens_v2_regfields[MAX_REGFIELDS] = {
>>>> /* CTRL_OFF */
>>>> [TSENS_EN] = REG_FIELD(SROT_CTRL_OFF, 0, 0),
>>>> [TSENS_SW_RST] = REG_FIELD(SROT_CTRL_OFF, 1, 1),
>>>> + [SENSOR_EN] = REG_FIELD(SROT_CTRL_OFF, 3, 18),
>>>> + [CODE_OR_TEMP] = REG_FIELD(SROT_CTRL_OFF, 21, 21),
>>>> +
>>>> + /* MAIN_MEASURE_PERIOD */
>>>> + [MAIN_MEASURE_PERIOD] = REG_FIELD(SROT_MEASURE_PERIOD, 0, 7),
>>>> +
>>>> + /* Sn Conversion */
>>>> + REG_FIELD_FOR_EACH_SENSOR16(SHIFT, SROT_Sn_CONVERSION, 23, 24),
>>>> + REG_FIELD_FOR_EACH_SENSOR16(SLOPE, SROT_Sn_CONVERSION, 10, 22),
>>>> + REG_FIELD_FOR_EACH_SENSOR16(CZERO, SROT_Sn_CONVERSION, 0, 9),
>>>> /* ----- TM ------ */
>>>> /* INTERRUPT ENABLE */
>>>> @@ -104,6 +123,103 @@ static const struct reg_field
>>>> tsens_v2_regfields[MAX_REGFIELDS] = {
>>>> [TRDY] = REG_FIELD(TM_TRDY_OFF, 0, 0),
>>>> };
>>>> +static int tsens_v2_calibration(struct tsens_priv *priv)
>>>> +{
>>>> + struct device *dev = priv->dev;
>>>> + u32 mode, base0, base1;
>>>> + u32 slope, czero;
>>>> + char name[15];
>>>> + int i, j, ret;
>>>> +
>>>> + if (priv->num_sensors > MAX_SENSORS)
>>>> + return -EINVAL;
>>>> +
>>>> + ret = nvmem_cell_read_variable_le_u32(priv->dev, "mode", &mode);
>>>> + if (ret == -ENOENT)
>>>> + dev_warn(priv->dev, "Calibration data not present in DT\n");
>>>> + if (ret < 0)
>>>> + return ret;
>>>> +
>>>> + dev_dbg(priv->dev, "calibration mode is %d\n", mode);
>>>> +
>>>> + ret = nvmem_cell_read_variable_le_u32(priv->dev, "base0", &base0);
>>>> + if (ret < 0)
>>>> + return ret;
>>>> +
>>>> + ret = nvmem_cell_read_variable_le_u32(priv->dev, "base1", &base1);
>>>> + if (ret < 0)
>>>> + return ret;
>>>> +
>>>> + /* Read offset values and allocate SHIFT, SLOPE & CZERO regmap
>>>> for enabled sensors */
>>>> + for (i = 0; i < priv->num_sensors; i++) {
>>>> + if (!(priv->sensors_to_en & (0x1 << i)))
>>>> + continue;
>>>> +
>>>> + ret = snprintf(name, sizeof(name), "s%d_offset",
>>>> priv->sensor[i].hw_id);
>>>> + if (ret < 0)
>>>> + return ret;
>>>> +
>>>> + ret = nvmem_cell_read_variable_le_u32(priv->dev, name,
>>>> &priv->sensor[i].offset);
>>>> + if (ret)
>>>> + return ret;
>>>> +
>>>> + for (j = SHIFT_0; j <= CZERO_0; j++) {
>>>> + int idx = (i * 3) + j;
>>>> +
>>>> + priv->rf[idx] = devm_regmap_field_alloc(dev,
>>>> priv->srot_map,
>>>> + priv->fields[idx]);
>>>> + if (IS_ERR(priv->rf[idx]))
>>>> + return PTR_ERR(priv->rf[idx]);
>>> I think, allocating data structures for 48 regfields, which are
>>> written just once, to be an overkill.
>> Can we change it to single field for each sensor. For example,
>> CONVERSION_0 instead of SHIFT_0, SLOPE_0 and CZERO_0? This way it will
>> be max 16 regfields.
> If you move writing of the registers to the loop, you won't need
> regfields. You can just call regmap_update_bits. The point is that you
> don't have to allocate a one-time instance.
Sure, will update in next patch.
>
>>>> + }
>>>> + }
>>>> +
>>>> + /* Based on calib mode, program SHIFT, SLOPE and CZERO for
>>>> enabled sensors */
>>>> + switch (mode) {
>>>> + case TWO_PT_CALIB:
>>>> + slope = (TWO_PT_SHIFTED_GAIN / (base1 - base0));
>>>> +
>>>> + for (i = 0; i < priv->num_sensors; i++) {
>>>> + if (!(priv->sensors_to_en & (0x1 << i)))
>>>> + continue;
>>>> +
>>>> + int idx = i * 3;
>>>> +
>>>> + czero = (base0 + priv->sensor[i].offset - ((base1 -
>>>> base0) / 3));
>>>> + regmap_field_write(priv->rf[SHIFT_0 + idx],
>>>> V2_SHIFT_DEFAULT);
>>>> + regmap_field_write(priv->rf[SLOPE_0 + idx], slope);
>>>> + regmap_field_write(priv->rf[CZERO_0 + idx], czero);
>>>> + }
>>>> + fallthrough;
>>>> + case ONE_PT_CALIB2:
>>>> + for (i = 0; i < priv->num_sensors; i++) {
>>>> + if (!(priv->sensors_to_en & (0x1 << i)))
>>>> + continue;
>>>> +
>>>> + int idx = i * 3;
>>>> +
>>>> + czero = base0 + priv->sensor[i].offset -
>>>> ONE_PT_CZERO_CONST;
>>>> + regmap_field_write(priv->rf[SHIFT_0 + idx],
>>>> V2_SHIFT_DEFAULT);
>>>> + regmap_field_write(priv->rf[SLOPE_0 + idx], ONE_PT_SLOPE);
>>>> + regmap_field_write(priv->rf[CZERO_0 + idx], czero);
>>>> + }
>>>> + break;
>>>> + default:
>>>> + dev_dbg(priv->dev, "calibrationless mode\n");
>>>> + for (i = 0; i < priv->num_sensors; i++) {
>>>> + if (!(priv->sensors_to_en & (0x1 << i)))
>>>> + continue;
>>>> +
>>>> + int idx = i * 3;
>>>> +
>>>> + regmap_field_write(priv->rf[SHIFT_0 + idx],
>>>> V2_SHIFT_DEFAULT);
>>>> + regmap_field_write(priv->rf[SLOPE_0 + idx],
>>>> V2_SLOPE_DEFAULT);
>>>> + regmap_field_write(priv->rf[CZERO_0 + idx],
>>>> V2_CZERO_DEFAULT);
>>>> + }
>>>> + }
>>> This code iterates over the sensors field several times. Please
>>> consider extracting a function that handles all setup for a single
>>> sensor, then calling it in a loop (I should probably do the same for
>>> tsens-v0/v1 too).
>> Sure. After reading the mode0, base0 and base1 from QFPROM, we can call
>> a function in a loop to setup the calibration for each sensor.
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> static const struct tsens_ops ops_generic_v2 = {
>>>> .init = init_common,
>>>> .get_temp = get_temp_tsens_valid,
>>>> diff --git a/drivers/thermal/qcom/tsens.c b/drivers/thermal/qcom/tsens.c
>>>> index 98c356acfe98..169690355dad 100644
>>>> --- a/drivers/thermal/qcom/tsens.c
>>>> +++ b/drivers/thermal/qcom/tsens.c
>>>> @@ -974,7 +974,7 @@ int __init init_common(struct tsens_priv *priv)
>>>> ret = regmap_field_read(priv->rf[TSENS_EN], &enabled);
>>>> if (ret)
>>>> goto err_put_device;
>>>> - if (!enabled) {
>>>> + if (!enabled && !priv->sensors_to_en) {
>>>> dev_err(dev, "%s: device not enabled\n", __func__);
>>>> ret = -ENODEV;
>>>> goto err_put_device;
>>>> @@ -1006,6 +1006,40 @@ int __init init_common(struct tsens_priv *priv)
>>>> goto err_put_device;
>>>> }
>>>> + /* Do TSENS initialization if required */
>>>> + if (priv->sensors_to_en) {
>>> Maybe it would be better to explicitly add VER_2_X_NO_RPM and check it
>>> here?
>> Sure, will add a separate version macro.
>>>> + priv->rf[CODE_OR_TEMP] = devm_regmap_field_alloc(dev,
>>>> priv->srot_map,
>>>> + priv->fields[CODE_OR_TEMP]);
>>>> + if (IS_ERR(priv->rf[CODE_OR_TEMP])) {
>>>> + ret = PTR_ERR(priv->rf[CODE_OR_TEMP]);
>>>> + goto err_put_device;
>>>> + }
>>>> +
>>>> + priv->rf[MAIN_MEASURE_PERIOD] =
>>>> + devm_regmap_field_alloc(dev, priv->srot_map,
>>>> + priv->fields[MAIN_MEASURE_PERIOD]);
>>>> + if (IS_ERR(priv->rf[MAIN_MEASURE_PERIOD])) {
>>>> + ret = PTR_ERR(priv->rf[MAIN_MEASURE_PERIOD]);
>>>> + goto err_put_device;
>>>> + }
>>>> +
>>>> + regmap_field_write(priv->rf[TSENS_SW_RST], 0x1);
>>>> +
>>>> + /* Update measure period to 2ms */
>>>> + regmap_field_write(priv->rf[MAIN_MEASURE_PERIOD], 0x1);
>>>> +
>>>> + /* Enable available sensors */
>>>> + regmap_field_write(priv->rf[SENSOR_EN], priv->sensors_to_en);
>>>> +
>>>> + /* Real temperature format */
>>>> + regmap_field_write(priv->rf[CODE_OR_TEMP], 0x1);
>>>> +
>>>> + regmap_field_write(priv->rf[TSENS_SW_RST], 0x0);
>>>> +
>>>> + /* Enable TSENS */
>>>> + regmap_field_write(priv->rf[TSENS_EN], 0x1);
>>>> + }
>>>> +
>>>> /* This loop might need changes if enum regfield_ids is
>>>> reordered */
>>>> for (j = LAST_TEMP_0; j <= UP_THRESH_15; j += 16) {
>>>> for (i = 0; i < priv->feat->max_sensors; i++) {
>>>> @@ -1282,6 +1316,7 @@ static int tsens_probe(struct platform_device
>>>> *pdev)
>>>> priv->dev = dev;
>>>> priv->num_sensors = num_sensors;
>>>> + priv->sensors_to_en = data->sensors_to_en;
>>>> priv->ops = data->ops;
>>>> for (i = 0; i < priv->num_sensors; i++) {
>>>> if (data->hw_ids)
>>>> diff --git a/drivers/thermal/qcom/tsens.h b/drivers/thermal/qcom/tsens.h
>>>> index 2805de1c6827..f8897bc8944e 100644
>>>> --- a/drivers/thermal/qcom/tsens.h
>>>> +++ b/drivers/thermal/qcom/tsens.h
>>>> @@ -168,6 +168,58 @@ enum regfield_ids {
>>>> TSENS_SW_RST,
>>>> SENSOR_EN,
>>>> CODE_OR_TEMP,
>>>> + /* MEASURE_PERIOD */
>>>> + MAIN_MEASURE_PERIOD,
>>>> +
>>>> + /* Sn_CONVERSION */
>>>> + SHIFT_0,
>>>> + SLOPE_0,
>>>> + CZERO_0,
>>>> + SHIFT_1,
>>>> + SLOPE_1,
>>>> + CZERO_1,
>>>> + SHIFT_2,
>>>> + SLOPE_2,
>>>> + CZERO_2,
>>>> + SHIFT_3,
>>>> + SLOPE_3,
>>>> + CZERO_3,
>>>> + SHIFT_4,
>>>> + SLOPE_4,
>>>> + CZERO_4,
>>>> + SHIFT_5,
>>>> + SLOPE_5,
>>>> + CZERO_5,
>>>> + SHIFT_6,
>>>> + SLOPE_6,
>>>> + CZERO_6,
>>>> + SHIFT_7,
>>>> + SLOPE_7,
>>>> + CZERO_7,
>>>> + SHIFT_8,
>>>> + SLOPE_8,
>>>> + CZERO_8,
>>>> + SHIFT_9,
>>>> + SLOPE_9,
>>>> + CZERO_9,
>>>> + SHIFT_10,
>>>> + SLOPE_10,
>>>> + CZERO_10,
>>>> + SHIFT_11,
>>>> + SLOPE_11,
>>>> + CZERO_11,
>>>> + SHIFT_12,
>>>> + SLOPE_12,
>>>> + CZERO_12,
>>>> + SHIFT_13,
>>>> + SLOPE_13,
>>>> + CZERO_13,
>>>> + SHIFT_14,
>>>> + SLOPE_14,
>>>> + CZERO_14,
>>>> + SHIFT_15,
>>>> + SLOPE_15,
>>>> + CZERO_15,
>>>> /* ----- TM ------ */
>>>> /* TRDY */
>>>> @@ -524,6 +576,7 @@ struct tsens_features {
>>>> /**
>>>> * struct tsens_plat_data - tsens compile-time platform data
>>>> * @num_sensors: Number of sensors supported by platform
>>>> + * @sensors_to_en: Sensors to be enabled. Each bit represent a sensor
>>>> * @ops: operations the tsens instance supports
>>>> * @hw_ids: Subset of sensors ids supported by platform, if not the
>>>> first n
>>>> * @feat: features of the IP
>>>> @@ -531,6 +584,7 @@ struct tsens_features {
>>>> */
>>>> struct tsens_plat_data {
>>>> const u32 num_sensors;
>>>> + const u16 sensors_to_en;
>>> There is already a similar field, hw_ids. Can it be used instead?
>> Yes, it can be used. I missed to check this hw_ids. Will change the
>> num_sensors to 5 and use the hw_ids.
>>>> const struct tsens_ops *ops;
>>>> unsigned int *hw_ids;
>>>> struct tsens_features *feat;
>>>> @@ -551,6 +605,7 @@ struct tsens_context {
>>>> * struct tsens_priv - private data for each instance of the tsens IP
>>>> * @dev: pointer to struct device
>>>> * @num_sensors: number of sensors enabled on this device
>>>> + * @sensors_to_en: sensors to be enabled. Each bit represents a sensor
>>>> * @tm_map: pointer to TM register address space
>>>> * @srot_map: pointer to SROT register address space
>>>> * @tm_offset: deal with old device trees that don't address TM and
>>>> SROT
>>>> @@ -569,6 +624,7 @@ struct tsens_context {
>>>> struct tsens_priv {
>>>> struct device *dev;
>>>> u32 num_sensors;
>>>> + u16 sensors_to_en;
>>>> struct regmap *tm_map;
>>>> struct regmap *srot_map;
>>>> u32 tm_offset;
>> --
>> Thanks,
>> Praveenkumar
>
>
On 7/10/2023 8:33 PM, Dmitry Baryshkov wrote:
> On Mon, 10 Jul 2023 at 16:47, Praveenkumar I <[email protected]> wrote:
>>
>> On 7/10/2023 4:54 PM, Dmitry Baryshkov wrote:
>>> On 10/07/2023 13:37, Praveenkumar I wrote:
>>>> IPQ5332 uses tsens v2.3.3 IP and it is having combined interrupt as
>>>> like IPQ8074. But as the SoCs does not have RPM, kernel needs to
>>>> take care of sensor enablement and calibration. Hence introduced
>>>> new ops and data for IPQ5332 and reused the feature_config from
>>>> IPQ8074.
>>>>
>>>> Signed-off-by: Praveenkumar I <[email protected]>
>>>> ---
>>>> drivers/thermal/qcom/tsens-v2.c | 13 +++++++++++++
>>>> drivers/thermal/qcom/tsens.c | 3 +++
>>>> drivers/thermal/qcom/tsens.h | 2 +-
>>>> 3 files changed, 17 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/thermal/qcom/tsens-v2.c
>>>> b/drivers/thermal/qcom/tsens-v2.c
>>>> index db48b1d95348..8b6e3876fd2c 100644
>>>> --- a/drivers/thermal/qcom/tsens-v2.c
>>>> +++ b/drivers/thermal/qcom/tsens-v2.c
>>>> @@ -237,6 +237,19 @@ struct tsens_plat_data data_ipq8074 = {
>>>> .fields = tsens_v2_regfields,
>>>> };
>>>> +static const struct tsens_ops ops_ipq5332_v2 = {
>>> Please drop v2. It is unclear if it refers to tsens being v2 or being
>>> specific to ipq5332 v2.
>> Sure, will drop v2.
>>>> + .init = init_common,
>>>> + .get_temp = get_temp_tsens_valid,
>>>> + .calibrate = tsens_v2_calibration,
>>>> +};
>>>> +
>>>> +struct tsens_plat_data data_ipq5332 = {
>>>> + .sensors_to_en = 0xF800,
>>> This doesn't seem to match the offsets that you have enabled in the DTSI.
>> In order to overcome the DT binding check failure, added all the
>> available QFPROM offsets in the DTSI. Else DT binding check failing on
>> "nvmem-cell-names".
> This is not a way to overcome issues in DT bindings. Please fix DT
> bindings instead by using alternatives, enums, etc.
Sure, will fix the DT bindings.
>
>>>> + .ops = &ops_ipq5332_v2,
>>>> + .feat = &ipq8074_feat,
>>>> + .fields = tsens_v2_regfields,
>>>> +};
>>>> +
>>>> /* Kept around for backward compatibility with old msm8996.dtsi */
>>>> struct tsens_plat_data data_8996 = {
>>>> .num_sensors = 13,
>>>> diff --git a/drivers/thermal/qcom/tsens.c b/drivers/thermal/qcom/tsens.c
>>>> index 169690355dad..e8ba2901cda8 100644
>>>> --- a/drivers/thermal/qcom/tsens.c
>>>> +++ b/drivers/thermal/qcom/tsens.c
>>>> @@ -1140,6 +1140,9 @@ static const struct of_device_id tsens_table[] = {
>>>> }, {
>>>> .compatible = "qcom,ipq8074-tsens",
>>>> .data = &data_ipq8074,
>>>> + }, {
>>>> + .compatible = "qcom,ipq5332-tsens",
>>>> + .data = &data_ipq5332,
>>>> }, {
>>>> .compatible = "qcom,mdm9607-tsens",
>>>> .data = &data_9607,
>>>> diff --git a/drivers/thermal/qcom/tsens.h b/drivers/thermal/qcom/tsens.h
>>>> index f8897bc8944e..36040f9beebc 100644
>>>> --- a/drivers/thermal/qcom/tsens.h
>>>> +++ b/drivers/thermal/qcom/tsens.h
>>>> @@ -701,6 +701,6 @@ extern struct tsens_plat_data data_8226,
>>>> data_8909, data_8916, data_8939, data_8
>>>> extern struct tsens_plat_data data_tsens_v1, data_8976, data_8956;
>>>> /* TSENS v2 targets */
>>>> -extern struct tsens_plat_data data_8996, data_ipq8074, data_tsens_v2;
>>>> +extern struct tsens_plat_data data_8996, data_ipq8074, data_ipq5332,
>>>> data_tsens_v2;
>>>> #endif /* __QCOM_TSENS_H__ */
>> --
>> Thanks,
>> Praveenkumar
>
>
On 7/11/2023 1:40 AM, Krzysztof Kozlowski wrote:
> On 10/07/2023 12:37, Praveenkumar I wrote:
>> Add TSENS V2 calibration nvmem cells for IPQ5332
>>
>> Signed-off-by: Praveenkumar I <[email protected]>
>> ---
>> .../bindings/thermal/qcom-tsens.yaml | 26 +++++++++++++++++--
>> 1 file changed, 24 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml b/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml
>> index 27e9e16e6455..8b7863c3989e 100644
>> --- a/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml
>> +++ b/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml
>> @@ -91,7 +91,7 @@ properties:
>> maxItems: 2
>>
>> nvmem-cells:
>> - oneOf:
>> + anyOf:
>> - minItems: 1
>> maxItems: 2
>> description:
>> @@ -106,9 +106,13 @@ properties:
>> description: |
>> Reference to nvmem cells for the calibration mode, two calibration
>> bases and two cells per each sensor, main and backup copies, plus use_backup cell
>> + - maxItems: 17
>> + description: |
>> + V2 of TSENS, reference to nvmem cells for the calibration mode, two calibration
>> + bases and one cell per each sensor
> I think this is already included in one of the previous entries.
> Otherwise, are you sure that all new devices will have exactly 17 entries?
Previous entries does not support TSENS version 2.X.X QFPROM. TSENS V2
QFPROM has mode, base0, base1 and s[0-15]+_offset.
Ideally it should be like,
- minItems: 4
- maxItems: 19
But dt binding check fails in oneOf / anyOf condition. So added the
IPQ5332 properties which is exactly 17.
>
>>
>> nvmem-cell-names:
>> - oneOf:
>> + anyOf:
>> - minItems: 1
>> items:
>> - const: calib
>> @@ -205,6 +209,24 @@ properties:
>> - const: s9_p2_backup
>> - const: s10_p1_backup
>> - const: s10_p2_backup
>> + - items:
>> + - const: mode
>> + - const: base0
>> + - const: base1
>> + - const: s0_offset
>> + - const: s3_offset
>> + - const: s4_offset
>> + - const: s5_offset
>> + - const: s6_offset
>> + - const: s7_offset
>> + - const: s8_offset
>> + - const: s9_offset
>> + - const: s10_offset
>> + - const: s11_offset
>> + - const: s12_offset
>> + - const: s13_offset
>> + - const: s14_offset
>> + - const: s15_offset
> Don't introduce new naming style. Existing uses s[0-9]+, without offset
> suffix. Why this should be different?
As I mentioned above, s[0-9]+_p1 / s[0-9]+p2 is for TSENS V1. TSENS V2
QFPROM layout is different from the existing one.
I would like to add mode, base0, base1 and 16 patterns
'^s[0-15]+_offset$'. But DT binding check is failing in oneOf/ anyOf
condintion.
--
Thanks,
Praveenkumar
>
>>
>> "#qcom,sensors":
>> description:
> Best regards,
> Krzysztof
>
On 11/07/2023 11:39, Praveenkumar I wrote:
>
> On 7/11/2023 1:40 AM, Krzysztof Kozlowski wrote:
>> On 10/07/2023 12:37, Praveenkumar I wrote:
>>> Add TSENS V2 calibration nvmem cells for IPQ5332
>>>
>>> Signed-off-by: Praveenkumar I <[email protected]>
>>> ---
>>> .../bindings/thermal/qcom-tsens.yaml | 26 +++++++++++++++++--
>>> 1 file changed, 24 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml b/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml
>>> index 27e9e16e6455..8b7863c3989e 100644
>>> --- a/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml
>>> +++ b/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml
>>> @@ -91,7 +91,7 @@ properties:
>>> maxItems: 2
>>>
>>> nvmem-cells:
>>> - oneOf:
>>> + anyOf:
>>> - minItems: 1
>>> maxItems: 2
>>> description:
>>> @@ -106,9 +106,13 @@ properties:
>>> description: |
>>> Reference to nvmem cells for the calibration mode, two calibration
>>> bases and two cells per each sensor, main and backup copies, plus use_backup cell
>>> + - maxItems: 17
>>> + description: |
>>> + V2 of TSENS, reference to nvmem cells for the calibration mode, two calibration
>>> + bases and one cell per each sensor
>> I think this is already included in one of the previous entries.
>> Otherwise, are you sure that all new devices will have exactly 17 entries?
> Previous entries does not support TSENS version 2.X.X QFPROM. TSENS V2
> QFPROM has mode, base0, base1 and s[0-15]+_offset.
> Ideally it should be like,
> - minItems: 4
> - maxItems: 19
I see it covered:
minItems: 5
maxItems: 35
I think 17 is between 5 and 35.
> But dt binding check fails in oneOf / anyOf condition. So added the
> IPQ5332 properties which is exactly 17.
>>
>>>
>>> nvmem-cell-names:
>>> - oneOf:
>>> + anyOf:
>>> - minItems: 1
>>> items:
>>> - const: calib
>>> @@ -205,6 +209,24 @@ properties:
>>> - const: s9_p2_backup
>>> - const: s10_p1_backup
>>> - const: s10_p2_backup
>>> + - items:
>>> + - const: mode
>>> + - const: base0
>>> + - const: base1
>>> + - const: s0_offset
>>> + - const: s3_offset
>>> + - const: s4_offset
>>> + - const: s5_offset
>>> + - const: s6_offset
>>> + - const: s7_offset
>>> + - const: s8_offset
>>> + - const: s9_offset
>>> + - const: s10_offset
>>> + - const: s11_offset
>>> + - const: s12_offset
>>> + - const: s13_offset
>>> + - const: s14_offset
>>> + - const: s15_offset
>> Don't introduce new naming style. Existing uses s[0-9]+, without offset
>> suffix. Why this should be different?
> As I mentioned above, s[0-9]+_p1 / s[0-9]+p2 is for TSENS V1. TSENS V2
> QFPROM layout is different from the existing one.
I know, I did not write about p1/p2.
> I would like to add mode, base0, base1 and 16 patterns
> '^s[0-15]+_offset$'. But DT binding check is failing in oneOf/ anyOf
> condintion.
This does not explain why you need different style - this "offset" suffix.
Best regards,
Krzysztof
On 7/11/2023 3:22 PM, Krzysztof Kozlowski wrote:
> On 11/07/2023 11:39, Praveenkumar I wrote:
>> On 7/11/2023 1:40 AM, Krzysztof Kozlowski wrote:
>>> On 10/07/2023 12:37, Praveenkumar I wrote:
>>>> Add TSENS V2 calibration nvmem cells for IPQ5332
>>>>
>>>> Signed-off-by: Praveenkumar I <[email protected]>
>>>> ---
>>>> .../bindings/thermal/qcom-tsens.yaml | 26 +++++++++++++++++--
>>>> 1 file changed, 24 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml b/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml
>>>> index 27e9e16e6455..8b7863c3989e 100644
>>>> --- a/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml
>>>> +++ b/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml
>>>> @@ -91,7 +91,7 @@ properties:
>>>> maxItems: 2
>>>>
>>>> nvmem-cells:
>>>> - oneOf:
>>>> + anyOf:
>>>> - minItems: 1
>>>> maxItems: 2
>>>> description:
>>>> @@ -106,9 +106,13 @@ properties:
>>>> description: |
>>>> Reference to nvmem cells for the calibration mode, two calibration
>>>> bases and two cells per each sensor, main and backup copies, plus use_backup cell
>>>> + - maxItems: 17
>>>> + description: |
>>>> + V2 of TSENS, reference to nvmem cells for the calibration mode, two calibration
>>>> + bases and one cell per each sensor
>>> I think this is already included in one of the previous entries.
>>> Otherwise, are you sure that all new devices will have exactly 17 entries?
>> Previous entries does not support TSENS version 2.X.X QFPROM. TSENS V2
>> QFPROM has mode, base0, base1 and s[0-15]+_offset.
>> Ideally it should be like,
>> - minItems: 4
>> - maxItems: 19
> I see it covered:
> minItems: 5
> maxItems: 35
>
> I think 17 is between 5 and 35.
Okay, will remove the nvmem-cells entry.
>
>> But dt binding check fails in oneOf / anyOf condition. So added the
>> IPQ5332 properties which is exactly 17.
>>>>
>>>> nvmem-cell-names:
>>>> - oneOf:
>>>> + anyOf:
>>>> - minItems: 1
>>>> items:
>>>> - const: calib
>>>> @@ -205,6 +209,24 @@ properties:
>>>> - const: s9_p2_backup
>>>> - const: s10_p1_backup
>>>> - const: s10_p2_backup
>>>> + - items:
>>>> + - const: mode
>>>> + - const: base0
>>>> + - const: base1
>>>> + - const: s0_offset
>>>> + - const: s3_offset
>>>> + - const: s4_offset
>>>> + - const: s5_offset
>>>> + - const: s6_offset
>>>> + - const: s7_offset
>>>> + - const: s8_offset
>>>> + - const: s9_offset
>>>> + - const: s10_offset
>>>> + - const: s11_offset
>>>> + - const: s12_offset
>>>> + - const: s13_offset
>>>> + - const: s14_offset
>>>> + - const: s15_offset
>>> Don't introduce new naming style. Existing uses s[0-9]+, without offset
>>> suffix. Why this should be different?
>> As I mentioned above, s[0-9]+_p1 / s[0-9]+p2 is for TSENS V1. TSENS V2
>> QFPROM layout is different from the existing one.
> I know, I did not write about p1/p2.
>
>> I would like to add mode, base0, base1 and 16 patterns
>> '^s[0-15]+_offset$'. But DT binding check is failing in oneOf/ anyOf
>> condintion.
> This does not explain why you need different style - this "offset" suffix.
In QFPROM, the BIT field names are s0_offset, s3_offset to s15_offset.
s1_offset and s2_offset not present in IPQ5332.
Hence used the same "offset" suffix here. May I know in this case, which
nvmem-cells-names you are suggesting to use?
--
Thanks,
Praveenkumar
>
> Best regards,
> Krzysztof
>
On 11/07/2023 16:13, Praveenkumar I wrote:
>>>>> - const: calib
>>>>> @@ -205,6 +209,24 @@ properties:
>>>>> - const: s9_p2_backup
>>>>> - const: s10_p1_backup
>>>>> - const: s10_p2_backup
>>>>> + - items:
>>>>> + - const: mode
>>>>> + - const: base0
>>>>> + - const: base1
>>>>> + - const: s0_offset
>>>>> + - const: s3_offset
>>>>> + - const: s4_offset
>>>>> + - const: s5_offset
>>>>> + - const: s6_offset
>>>>> + - const: s7_offset
>>>>> + - const: s8_offset
>>>>> + - const: s9_offset
>>>>> + - const: s10_offset
>>>>> + - const: s11_offset
>>>>> + - const: s12_offset
>>>>> + - const: s13_offset
>>>>> + - const: s14_offset
>>>>> + - const: s15_offset
>>>> Don't introduce new naming style. Existing uses s[0-9]+, without offset
>>>> suffix. Why this should be different?
>>> As I mentioned above, s[0-9]+_p1 / s[0-9]+p2 is for TSENS V1. TSENS V2
>>> QFPROM layout is different from the existing one.
>> I know, I did not write about p1/p2.
>>
>>> I would like to add mode, base0, base1 and 16 patterns
>>> '^s[0-15]+_offset$'. But DT binding check is failing in oneOf/ anyOf
>>> condintion.
>> This does not explain why you need different style - this "offset" suffix.
> In QFPROM, the BIT field names are s0_offset, s3_offset to s15_offset.
> s1_offset and s2_offset not present in IPQ5332.
> Hence used the same "offset" suffix here. May I know in this case, which
> nvmem-cells-names you are suggesting to use?
"Existing uses s[0-9]+"
so s[0-9]+
Best regards,
Krzysztof