Re: [PATCH 2/6] thermal/drivers/tsens: Add TSENS enable and calibration support for V2

From: Praveenkumar I
Date: Mon Jul 10 2023 - 09:24:35 EST



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 <quic_ipkumar@xxxxxxxxxxx>
---
  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