diff mbox series

[v3,3/6] thermal: qcom: tsens-v0_1: Add mdm9607 correction offsets

Message ID 20230315103950.2679317-4-stephan.gerhold@kernkonzept.com (mailing list archive)
State New, archived
Delegated to: Daniel Lezcano
Headers show
Series thermal: qcom: tsens: Fix MDM9607, add MSM8909 | expand

Commit Message

Stephan Gerhold March 15, 2023, 10:39 a.m. UTC
According to the msm-3.18 vendor kernel from Qualcomm, mdm9607 needs
"correction factors" to adjust for additional offsets observed after the
factory calibration values in the fuses [1, 2].

The fixed offsets should be applied unless there is a special
calibration mode value that indicates that no offsets are needed [3].

Note that the new calibration mode values are called differently in this
patch compared to the vendor kernel:
  - TSENS_TWO_POINT_CALIB_N_WA        -> ONE_PT_CALIB2_NO_OFFSET
  - TSENS_TWO_POINT_CALIB_N_OFFSET_WA -> TWO_PT_CALIB_NO_OFFSET
This is because close inspection of the calibration function [3] reveals
that TSENS_TWO_POINT_CALIB_N_WA is actually a "one point" calibration
because the if statements skip all "point2" related code for it.

[1]: https://git.codelinaro.org/clo/la/kernel/msm-3.18/-/commit/d9d2db1b82bf3f72f5de0803d55e6849eb5b671e
[2]: https://git.codelinaro.org/clo/la/kernel/msm-3.18/-/commit/d75aef53a760e8ff7bac54049d00c8b2ee1b193e
[3]: https://git.codelinaro.org/clo/la/kernel/msm-3.18/-/blob/LE.UM.4.3.2.r1-04200-9x07/drivers/thermal/msm-tsens.c#L2987-3136

Cc: Konrad Dybcio <konrad.dybcio@linaro.org>
Fixes: a2149ab815fc ("thermal/drivers/qcom/tsens-v0_1: Add support for MDM9607")
Signed-off-by: Stephan Gerhold <stephan.gerhold@kernkonzept.com>
---
Changes in v3: None
Changes in v2: New patch
---
 drivers/thermal/qcom/tsens-v0_1.c | 11 +++++++++++
 drivers/thermal/qcom/tsens.c      | 16 +++++++++++++++-
 drivers/thermal/qcom/tsens.h      |  4 ++++
 3 files changed, 30 insertions(+), 1 deletion(-)

Comments

Konrad Dybcio March 18, 2023, 2:02 p.m. UTC | #1
On 15.03.2023 11:39, Stephan Gerhold wrote:
> According to the msm-3.18 vendor kernel from Qualcomm, mdm9607 needs
> "correction factors" to adjust for additional offsets observed after the
> factory calibration values in the fuses [1, 2].
> 
> The fixed offsets should be applied unless there is a special
> calibration mode value that indicates that no offsets are needed [3].
> 
> Note that the new calibration mode values are called differently in this
> patch compared to the vendor kernel:
>   - TSENS_TWO_POINT_CALIB_N_WA        -> ONE_PT_CALIB2_NO_OFFSET
>   - TSENS_TWO_POINT_CALIB_N_OFFSET_WA -> TWO_PT_CALIB_NO_OFFSET
> This is because close inspection of the calibration function [3] reveals
> that TSENS_TWO_POINT_CALIB_N_WA is actually a "one point" calibration
> because the if statements skip all "point2" related code for it.
> 
> [1]: https://git.codelinaro.org/clo/la/kernel/msm-3.18/-/commit/d9d2db1b82bf3f72f5de0803d55e6849eb5b671e
> [2]: https://git.codelinaro.org/clo/la/kernel/msm-3.18/-/commit/d75aef53a760e8ff7bac54049d00c8b2ee1b193e
> [3]: https://git.codelinaro.org/clo/la/kernel/msm-3.18/-/blob/LE.UM.4.3.2.r1-04200-9x07/drivers/thermal/msm-tsens.c#L2987-3136
> 
> Cc: Konrad Dybcio <konrad.dybcio@linaro.org>
> Fixes: a2149ab815fc ("thermal/drivers/qcom/tsens-v0_1: Add support for MDM9607")
> Signed-off-by: Stephan Gerhold <stephan.gerhold@kernkonzept.com>
> ---
Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>

BTW, did you notice some crazy readouts or would this have gone
unnoticed had you not dug in the code?

Konrad
> Changes in v3: None
> Changes in v2: New patch
> ---
>  drivers/thermal/qcom/tsens-v0_1.c | 11 +++++++++++
>  drivers/thermal/qcom/tsens.c      | 16 +++++++++++++++-
>  drivers/thermal/qcom/tsens.h      |  4 ++++
>  3 files changed, 30 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/thermal/qcom/tsens-v0_1.c b/drivers/thermal/qcom/tsens-v0_1.c
> index 1b454de3928d..e69889dd524a 100644
> --- a/drivers/thermal/qcom/tsens-v0_1.c
> +++ b/drivers/thermal/qcom/tsens-v0_1.c
> @@ -229,6 +229,17 @@ static int __init init_9607(struct tsens_priv *priv)
>  	for (i = 0; i < priv->num_sensors; ++i)
>  		priv->sensor[i].slope = 3000;
>  
> +	priv->sensor[0].p1_calib_offset = 1;
> +	priv->sensor[0].p2_calib_offset = 1;
> +	priv->sensor[1].p1_calib_offset = -4;
> +	priv->sensor[1].p2_calib_offset = -2;
> +	priv->sensor[2].p1_calib_offset = 4;
> +	priv->sensor[2].p2_calib_offset = 8;
> +	priv->sensor[3].p1_calib_offset = -3;
> +	priv->sensor[3].p2_calib_offset = -5;
> +	priv->sensor[4].p1_calib_offset = -4;
> +	priv->sensor[4].p2_calib_offset = -4;
> +
>  	return init_common(priv);
>  }
>  
> diff --git a/drivers/thermal/qcom/tsens.c b/drivers/thermal/qcom/tsens.c
> index 8020ead2794e..32d2907f76e5 100644
> --- a/drivers/thermal/qcom/tsens.c
> +++ b/drivers/thermal/qcom/tsens.c
> @@ -134,10 +134,12 @@ int tsens_read_calibration(struct tsens_priv *priv, int shift, u32 *p1, u32 *p2,
>  			p1[i] = p1[i] + (base1 << shift);
>  		break;
>  	case TWO_PT_CALIB:
> +	case TWO_PT_CALIB_NO_OFFSET:
>  		for (i = 0; i < priv->num_sensors; i++)
>  			p2[i] = (p2[i] + base2) << shift;
>  		fallthrough;
>  	case ONE_PT_CALIB2:
> +	case ONE_PT_CALIB2_NO_OFFSET:
>  		for (i = 0; i < priv->num_sensors; i++)
>  			p1[i] = (p1[i] + base1) << shift;
>  		break;
> @@ -149,6 +151,18 @@ int tsens_read_calibration(struct tsens_priv *priv, int shift, u32 *p1, u32 *p2,
>  		}
>  	}
>  
> +	/* Apply calibration offset workaround except for _NO_OFFSET modes */
> +	switch (mode) {
> +	case TWO_PT_CALIB:
> +		for (i = 0; i < priv->num_sensors; i++)
> +			p2[i] += priv->sensor[i].p2_calib_offset;
> +		fallthrough;
> +	case ONE_PT_CALIB2:
> +		for (i = 0; i < priv->num_sensors; i++)
> +			p1[i] += priv->sensor[i].p1_calib_offset;
> +		break;
> +	}
> +
>  	return mode;
>  }
>  
> @@ -254,7 +268,7 @@ void compute_intercept_slope(struct tsens_priv *priv, u32 *p1,
>  
>  		if (!priv->sensor[i].slope)
>  			priv->sensor[i].slope = SLOPE_DEFAULT;
> -		if (mode == TWO_PT_CALIB) {
> +		if (mode == TWO_PT_CALIB || mode == TWO_PT_CALIB_NO_OFFSET) {
>  			/*
>  			 * slope (m) = adc_code2 - adc_code1 (y2 - y1)/
>  			 *	temp_120_degc - temp_30_degc (x2 - x1)
> diff --git a/drivers/thermal/qcom/tsens.h b/drivers/thermal/qcom/tsens.h
> index dba9cd38f637..880be6be5c3f 100644
> --- a/drivers/thermal/qcom/tsens.h
> +++ b/drivers/thermal/qcom/tsens.h
> @@ -10,6 +10,8 @@
>  #define ONE_PT_CALIB		0x1
>  #define ONE_PT_CALIB2		0x2
>  #define TWO_PT_CALIB		0x3
> +#define ONE_PT_CALIB2_NO_OFFSET	0x6
> +#define TWO_PT_CALIB_NO_OFFSET	0x7
>  #define CAL_DEGC_PT1		30
>  #define CAL_DEGC_PT2		120
>  #define SLOPE_FACTOR		1000
> @@ -57,6 +59,8 @@ struct tsens_sensor {
>  	unsigned int			hw_id;
>  	int				slope;
>  	u32				status;
> +	int				p1_calib_offset;
> +	int				p2_calib_offset;
>  };
>  
>  /**
Stephan Gerhold March 18, 2023, 4:06 p.m. UTC | #2
On Sat, Mar 18, 2023 at 03:02:35PM +0100, Konrad Dybcio wrote:
> On 15.03.2023 11:39, Stephan Gerhold wrote:
> > According to the msm-3.18 vendor kernel from Qualcomm, mdm9607 needs
> > "correction factors" to adjust for additional offsets observed after the
> > factory calibration values in the fuses [1, 2].
> > 
> > The fixed offsets should be applied unless there is a special
> > calibration mode value that indicates that no offsets are needed [3].
> > 
> > Note that the new calibration mode values are called differently in this
> > patch compared to the vendor kernel:
> >   - TSENS_TWO_POINT_CALIB_N_WA        -> ONE_PT_CALIB2_NO_OFFSET
> >   - TSENS_TWO_POINT_CALIB_N_OFFSET_WA -> TWO_PT_CALIB_NO_OFFSET
> > This is because close inspection of the calibration function [3] reveals
> > that TSENS_TWO_POINT_CALIB_N_WA is actually a "one point" calibration
> > because the if statements skip all "point2" related code for it.
> > 
> > [1]: https://git.codelinaro.org/clo/la/kernel/msm-3.18/-/commit/d9d2db1b82bf3f72f5de0803d55e6849eb5b671e
> > [2]: https://git.codelinaro.org/clo/la/kernel/msm-3.18/-/commit/d75aef53a760e8ff7bac54049d00c8b2ee1b193e
> > [3]: https://git.codelinaro.org/clo/la/kernel/msm-3.18/-/blob/LE.UM.4.3.2.r1-04200-9x07/drivers/thermal/msm-tsens.c#L2987-3136
> > 
> > Cc: Konrad Dybcio <konrad.dybcio@linaro.org>
> > Fixes: a2149ab815fc ("thermal/drivers/qcom/tsens-v0_1: Add support for MDM9607")
> > Signed-off-by: Stephan Gerhold <stephan.gerhold@kernkonzept.com>
> > ---
> Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> 
> BTW, did you notice some crazy readouts or would this have gone
> unnoticed had you not dug in the code?
> 

I'm afraid it would likely have remained unnoticed. I think these
offsets only make a small difference but it's still good to have them
for slightly more accurate readings.

In v1 of this series I had the offsets for MSM8909 already (hardcoded
into the old calibration function with all the bit shifts/masks etc).
It was more coincidence that I checked MDM9607 for v2 because I had to
make the code more generic with the new per-sensor nvmem cells. ;)

Thanks,
Stephan
diff mbox series

Patch

diff --git a/drivers/thermal/qcom/tsens-v0_1.c b/drivers/thermal/qcom/tsens-v0_1.c
index 1b454de3928d..e69889dd524a 100644
--- a/drivers/thermal/qcom/tsens-v0_1.c
+++ b/drivers/thermal/qcom/tsens-v0_1.c
@@ -229,6 +229,17 @@  static int __init init_9607(struct tsens_priv *priv)
 	for (i = 0; i < priv->num_sensors; ++i)
 		priv->sensor[i].slope = 3000;
 
+	priv->sensor[0].p1_calib_offset = 1;
+	priv->sensor[0].p2_calib_offset = 1;
+	priv->sensor[1].p1_calib_offset = -4;
+	priv->sensor[1].p2_calib_offset = -2;
+	priv->sensor[2].p1_calib_offset = 4;
+	priv->sensor[2].p2_calib_offset = 8;
+	priv->sensor[3].p1_calib_offset = -3;
+	priv->sensor[3].p2_calib_offset = -5;
+	priv->sensor[4].p1_calib_offset = -4;
+	priv->sensor[4].p2_calib_offset = -4;
+
 	return init_common(priv);
 }
 
diff --git a/drivers/thermal/qcom/tsens.c b/drivers/thermal/qcom/tsens.c
index 8020ead2794e..32d2907f76e5 100644
--- a/drivers/thermal/qcom/tsens.c
+++ b/drivers/thermal/qcom/tsens.c
@@ -134,10 +134,12 @@  int tsens_read_calibration(struct tsens_priv *priv, int shift, u32 *p1, u32 *p2,
 			p1[i] = p1[i] + (base1 << shift);
 		break;
 	case TWO_PT_CALIB:
+	case TWO_PT_CALIB_NO_OFFSET:
 		for (i = 0; i < priv->num_sensors; i++)
 			p2[i] = (p2[i] + base2) << shift;
 		fallthrough;
 	case ONE_PT_CALIB2:
+	case ONE_PT_CALIB2_NO_OFFSET:
 		for (i = 0; i < priv->num_sensors; i++)
 			p1[i] = (p1[i] + base1) << shift;
 		break;
@@ -149,6 +151,18 @@  int tsens_read_calibration(struct tsens_priv *priv, int shift, u32 *p1, u32 *p2,
 		}
 	}
 
+	/* Apply calibration offset workaround except for _NO_OFFSET modes */
+	switch (mode) {
+	case TWO_PT_CALIB:
+		for (i = 0; i < priv->num_sensors; i++)
+			p2[i] += priv->sensor[i].p2_calib_offset;
+		fallthrough;
+	case ONE_PT_CALIB2:
+		for (i = 0; i < priv->num_sensors; i++)
+			p1[i] += priv->sensor[i].p1_calib_offset;
+		break;
+	}
+
 	return mode;
 }
 
@@ -254,7 +268,7 @@  void compute_intercept_slope(struct tsens_priv *priv, u32 *p1,
 
 		if (!priv->sensor[i].slope)
 			priv->sensor[i].slope = SLOPE_DEFAULT;
-		if (mode == TWO_PT_CALIB) {
+		if (mode == TWO_PT_CALIB || mode == TWO_PT_CALIB_NO_OFFSET) {
 			/*
 			 * slope (m) = adc_code2 - adc_code1 (y2 - y1)/
 			 *	temp_120_degc - temp_30_degc (x2 - x1)
diff --git a/drivers/thermal/qcom/tsens.h b/drivers/thermal/qcom/tsens.h
index dba9cd38f637..880be6be5c3f 100644
--- a/drivers/thermal/qcom/tsens.h
+++ b/drivers/thermal/qcom/tsens.h
@@ -10,6 +10,8 @@ 
 #define ONE_PT_CALIB		0x1
 #define ONE_PT_CALIB2		0x2
 #define TWO_PT_CALIB		0x3
+#define ONE_PT_CALIB2_NO_OFFSET	0x6
+#define TWO_PT_CALIB_NO_OFFSET	0x7
 #define CAL_DEGC_PT1		30
 #define CAL_DEGC_PT2		120
 #define SLOPE_FACTOR		1000
@@ -57,6 +59,8 @@  struct tsens_sensor {
 	unsigned int			hw_id;
 	int				slope;
 	u32				status;
+	int				p1_calib_offset;
+	int				p2_calib_offset;
 };
 
 /**