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

From: Dmitry Baryshkov
Date: Mon Jul 10 2023 - 11:03:18 EST

On Mon, 10 Jul 2023 at 16:22, Praveenkumar I <quic_ipkumar@xxxxxxxxxxx> 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 <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.
> 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