Messages in this thread | | | Date | Mon, 10 Jul 2023 18:54:12 +0530 | Subject | Re: [PATCH 2/6] thermal/drivers/tsens: Add TSENS enable and calibration support for V2 | From | Praveenkumar I <> |
| |
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@quicinc.com> >>> --- >>> 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
| |