On MSM8939 the last sensor has calibration data that cannot be extracted in
one big read.
Rather than have a lot of MSM8939 specific code this series makes a generic
modification to allow any other calibration data that is non-contiguous to
be extracted and recovered.
For example s9-p2 takes bits 1-5 from @4b and bit 13 from @4d. The bit from
bit13 then becomes the sixth bit in the calibration data.
tsens_s9_p2: s9-p2@4b {
reg = <0x4b 0x1>;
bits = <1 5>;
};
tsens_s9_p2_msb: s9-p2-msb@4d {
reg = <0x4d 0x1>;
bits = <13 1>;
};
A register desciptor is introduced in the driver which takes the place of
the previous unsigned int hw_ids array in struct tsens_plat_data.
This new structure contains the previous hardware id and two variables
p1_shift and p2_shift.
If p1_shift or p2_shift is non-zero then this tells
tsens_read_calibration() to search for sX-pY-msb where msb means "most
significant bits".
The value at p1_shift/p2_shift is then used to right shift the value read
from sX-pY-msb and or that value into the base value from sX-pY.
The nvmem 'bits' field provides the mask.
Bryan O'Donoghue (3):
thermal/drivers/tsens: Add error/debug prints to calibration read
thermal/drivers/tsens: Describe sensor registers via a structure
thermal/drivers/tsens: Extract and shift-in optional MSB
drivers/thermal/qcom/tsens-v0_1.c | 56 +++++++++++++++++++++++++++++--
drivers/thermal/qcom/tsens.c | 50 ++++++++++++++++++++++++---
drivers/thermal/qcom/tsens.h | 16 ++++++++-
3 files changed, 115 insertions(+), 7 deletions(-)
--
2.39.2
In msm8939 some of the sensor calibration data traverses byte boundaries.
Two examples of this are thermal sensor 2 point 1 and sensor 9 point 2.
For sensor 2 point 1 we can get away with a simple read traversing byte
boundaries as the calibration most significant bits are adjacent to the
least significant across the byte boundary.
In this case a read starting at the end of the first byte for nine bits
will deliver up the data we want.
In the case of sensor 9 point 2 however, the most significant bits are not
adjacent and so therefore we need to perform two reads and or the bits
together.
If reg.p1_shift or reg.p2_shift is set then automatically search for
pX_sY_msb in the dts applying pX_shift as a right shift or into the pX_sY
value.
Signed-off-by: Bryan O'Donoghue <[email protected]>
---
drivers/thermal/qcom/tsens.c | 33 +++++++++++++++++++++++++++++++++
1 file changed, 33 insertions(+)
diff --git a/drivers/thermal/qcom/tsens.c b/drivers/thermal/qcom/tsens.c
index a260f563b4889..eff2c8671c343 100644
--- a/drivers/thermal/qcom/tsens.c
+++ b/drivers/thermal/qcom/tsens.c
@@ -74,6 +74,7 @@ int tsens_read_calibration(struct tsens_priv *priv, int shift, u32 *p1, u32 *p2,
{
u32 mode;
u32 base1, base2;
+ u32 msb;
char name[] = "sXX_pY_backup"; /* s10_p1_backup */
int i, ret;
@@ -122,6 +123,22 @@ int tsens_read_calibration(struct tsens_priv *priv, int shift, u32 *p1, u32 *p2,
dev_dbg(priv->dev, "%s 0x%x\n", name, p1[i]);
+ if (priv->reg && priv->reg[i].p1_shift) {
+ ret = snprintf(name, sizeof(name), "s%d_p1_msb",
+ priv->sensor[i].hw_id);
+ if (ret < 0)
+ return ret;
+
+ ret = nvmem_cell_read_variable_le_u32(priv->dev, name, &msb);
+ if (ret) {
+ dev_err(priv->dev, "Failed to read %s\n", name);
+ return ret;
+ }
+
+ dev_dbg(priv->dev, "%s 0x%x\n", name, msb);
+ p1[i] |= msb >> priv->reg[i].p1_shift;
+ }
+
ret = snprintf(name, sizeof(name), "s%d_p2%s", priv->sensor[i].hw_id,
backup ? "_backup" : "");
if (ret < 0)
@@ -134,6 +151,22 @@ int tsens_read_calibration(struct tsens_priv *priv, int shift, u32 *p1, u32 *p2,
}
dev_dbg(priv->dev, "%s 0x%x\n", name, p2[i]);
+
+ if (priv->reg && priv->reg[i].p2_shift) {
+ ret = snprintf(name, sizeof(name), "s%d_p2_msb",
+ priv->sensor[i].hw_id);
+ if (ret < 0)
+ return ret;
+
+ ret = nvmem_cell_read_variable_le_u32(priv->dev, name, &msb);
+ if (ret) {
+ dev_err(priv->dev, "Failed to read %s\n", name);
+ return ret;
+ }
+
+ dev_dbg(priv->dev, "%s 0x%x\n", name, msb);
+ p2[i] |= msb >> priv->reg[i].p2_shift;
+ }
}
switch (mode) {
--
2.39.2
Define sensor identifiers and optional shifts in a single data-structure.
This facilitates extraction of calibration data from non-contiguous
addresses.
Signed-off-by: Bryan O'Donoghue <[email protected]>
---
drivers/thermal/qcom/tsens-v0_1.c | 56 +++++++++++++++++++++++++++++--
drivers/thermal/qcom/tsens.c | 5 +--
drivers/thermal/qcom/tsens.h | 16 ++++++++-
3 files changed, 72 insertions(+), 5 deletions(-)
diff --git a/drivers/thermal/qcom/tsens-v0_1.c b/drivers/thermal/qcom/tsens-v0_1.c
index e89c6f39a3aea..c20c002d98650 100644
--- a/drivers/thermal/qcom/tsens-v0_1.c
+++ b/drivers/thermal/qcom/tsens-v0_1.c
@@ -319,10 +319,31 @@ static const struct tsens_ops ops_8916 = {
.get_temp = get_temp_common,
};
+struct tsens_reg_data reg_8916[] = {
+ {
+ .id = 0,
+ },
+ {
+ .id = 1,
+ },
+ {
+ .id = 2,
+ },
+ {
+ .id = 3,
+ },
+ {
+ .id = 4,
+ },
+ {
+ .id = 5,
+ },
+};
+
struct tsens_plat_data data_8916 = {
.num_sensors = 5,
.ops = &ops_8916,
- .hw_ids = (unsigned int []){0, 1, 2, 4, 5 },
+ .reg = reg_8916,
.feat = &tsens_v0_1_feat,
.fields = tsens_v0_1_regfields,
@@ -334,10 +355,41 @@ static const struct tsens_ops ops_8939 = {
.get_temp = get_temp_common,
};
+struct tsens_reg_data reg_8939[] = {
+ {
+ .id = 0,
+ },
+ {
+ .id = 1,
+ },
+ {
+ .id = 2,
+ },
+ {
+ .id = 3,
+ },
+ {
+ .id = 5,
+ },
+ {
+ .id = 6,
+ },
+ {
+ .id = 7,
+ },
+ {
+ .id = 8,
+ },
+ {
+ .id = 9,
+ .p2_shift = 8,
+ },
+};
+
struct tsens_plat_data data_8939 = {
.num_sensors = 9,
.ops = &ops_8939,
- .hw_ids = (unsigned int []){ 0, 1, 2, 3, 5, 6, 7, 8, 9, /* 10 */ },
+ .reg = reg_8939,
.feat = &tsens_v0_1_feat,
.fields = tsens_v0_1_regfields,
diff --git a/drivers/thermal/qcom/tsens.c b/drivers/thermal/qcom/tsens.c
index 7165b0bfe8b9f..a260f563b4889 100644
--- a/drivers/thermal/qcom/tsens.c
+++ b/drivers/thermal/qcom/tsens.c
@@ -1274,13 +1274,14 @@ static int tsens_probe(struct platform_device *pdev)
priv->num_sensors = num_sensors;
priv->ops = data->ops;
for (i = 0; i < priv->num_sensors; i++) {
- if (data->hw_ids)
- priv->sensor[i].hw_id = data->hw_ids[i];
+ if (data->reg)
+ priv->sensor[i].hw_id = data->reg[i].id;
else
priv->sensor[i].hw_id = i;
}
priv->feat = data->feat;
priv->fields = data->fields;
+ priv->reg = data->reg;
platform_set_drvdata(pdev, priv);
diff --git a/drivers/thermal/qcom/tsens.h b/drivers/thermal/qcom/tsens.h
index dba9cd38f637c..31f67da03bce6 100644
--- a/drivers/thermal/qcom/tsens.h
+++ b/drivers/thermal/qcom/tsens.h
@@ -517,18 +517,31 @@ struct tsens_features {
int trip_max_temp;
};
+/**
+ * struct tsens_reg_data - describes register data retrieved non-contiguously
+ * @id: thermal sensor identifier
+ * @p1_shift: When non-zero is the # of bits to right shift p1 MSB by
+ * @p2_shift: When non-zero is the # of bits to right shift p2 MSB by
+ */
+struct tsens_reg_data {
+ unsigned int id;
+ unsigned int p1_shift;
+ unsigned int p2_shift;
+};
+
/**
* struct tsens_plat_data - tsens compile-time platform data
* @num_sensors: Number of sensors supported by platform
* @ops: operations the tsens instance supports
* @hw_ids: Subset of sensors ids supported by platform, if not the first n
+ * @reg: Describe sensor id and calibration shifts
* @feat: features of the IP
* @fields: bitfield locations
*/
struct tsens_plat_data {
const u32 num_sensors;
const struct tsens_ops *ops;
- unsigned int *hw_ids;
+ struct tsens_reg_data *reg;
struct tsens_features *feat;
const struct reg_field *fields;
};
@@ -575,6 +588,7 @@ struct tsens_priv {
struct regmap_field *rf[MAX_REGFIELDS];
struct tsens_context ctx;
struct tsens_features *feat;
+ struct tsens_reg_data *reg;
const struct reg_field *fields;
const struct tsens_ops *ops;
--
2.39.2
Add in some dev_dbg() to aid in viewing of raw calibration data extracted.
Signed-off-by: Bryan O'Donoghue <[email protected]>
---
drivers/thermal/qcom/tsens.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/drivers/thermal/qcom/tsens.c b/drivers/thermal/qcom/tsens.c
index d3218127e617d..7165b0bfe8b9f 100644
--- a/drivers/thermal/qcom/tsens.c
+++ b/drivers/thermal/qcom/tsens.c
@@ -115,8 +115,12 @@ int tsens_read_calibration(struct tsens_priv *priv, int shift, u32 *p1, u32 *p2,
return ret;
ret = nvmem_cell_read_variable_le_u32(priv->dev, name, &p1[i]);
- if (ret)
+ if (ret) {
+ dev_err(priv->dev, "Failed to read %s\n", name);
return ret;
+ }
+
+ dev_dbg(priv->dev, "%s 0x%x\n", name, p1[i]);
ret = snprintf(name, sizeof(name), "s%d_p2%s", priv->sensor[i].hw_id,
backup ? "_backup" : "");
@@ -124,8 +128,12 @@ int tsens_read_calibration(struct tsens_priv *priv, int shift, u32 *p1, u32 *p2,
return ret;
ret = nvmem_cell_read_variable_le_u32(priv->dev, name, &p2[i]);
- if (ret)
+ if (ret) {
+ dev_err(priv->dev, "Failed to read %s\n", name);
return ret;
+ }
+
+ dev_dbg(priv->dev, "%s 0x%x\n", name, p2[i]);
}
switch (mode) {
--
2.39.2
On Thu, Apr 06, 2023 at 03:58:47PM +0100, Bryan O'Donoghue wrote:
> On MSM8939 the last sensor has calibration data that cannot be extracted in
> one big read.
>
> Rather than have a lot of MSM8939 specific code this series makes a generic
> modification to allow any other calibration data that is non-contiguous to
> be extracted and recovered.
>
> For example s9-p2 takes bits 1-5 from @4b and bit 13 from @4d. The bit from
> bit13 then becomes the sixth bit in the calibration data.
>
> tsens_s9_p2: s9-p2@4b {
> reg = <0x4b 0x1>;
> bits = <1 5>;
> };
>
> tsens_s9_p2_msb: s9-p2-msb@4d {
> reg = <0x4d 0x1>;
> bits = <13 1>;
> };
As far as I can tell the sensor with the non-contiguous calibration data
is the one with hwid=10, so do you mean s10-p2 instead of s9-p2 here?
It's easy to mix up the numbering: Since hwid=4 is missing for MSM8939,
the sensor 9 in the calibration code downstream (TSENS9_8939_POINT*)
actually refers to hwid=10. hwid=9 is sensor 8 in the calibration code
(TSENS8_8939_POINT*).
Sensor hwid=10 was disabled for MSM8939 in the tsens driver because it
seems unused, only exists on MSM8939 v3.0, and specifically to avoid
having to handle this non-contiguous calibration data, see commit
903238a33c11 ("thermal/drivers/tsens: limit num_sensors to 9 for msm8939"):
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=903238a33c116edf5f64f7a3fd246e6169cccfa6
> On msm8939 last (hwid=10) sensor was added in the hw revision 3.0.
> Calibration data for it was placed outside of the main calibration
> data blob, so it is not accessible by the current blob-parsing code.
>
> Moreover data for the sensor's p2 is not contiguous in the fuses. This
> makes it hard to use nvmem_cell API to parse calibration data in a
> generic way.
>
> Since the sensor doesn't seem to be actually used by the existing
> hardware, disable the sensor for now.
>
> Fixes: 332bc8ebab2c ("thermal: qcom: tsens-v0_1: Add support for MSM8939")
> Signed-off-by: Dmitry Baryshkov <[email protected]>
> Reviewed-by: Bryan O'Donoghue <[email protected]>
So with sensor hwid=10 disabled, I think this patch series is actually
not needed? :)
Thanks,
Stephan
On 06/04/2023 17:58, Bryan O'Donoghue wrote:
> Add in some dev_dbg() to aid in viewing of raw calibration data extracted.
>
> Signed-off-by: Bryan O'Donoghue <[email protected]>
> ---
> drivers/thermal/qcom/tsens.c | 12 ++++++++++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
Reviewed-by: Dmitry Baryshkov <[email protected]>
--
With best wishes
Dmitry
On 06/04/2023 17:58, Bryan O'Donoghue wrote:
> Define sensor identifiers and optional shifts in a single data-structure.
> This facilitates extraction of calibration data from non-contiguous
> addresses.
>
> Signed-off-by: Bryan O'Donoghue <[email protected]>
> ---
> drivers/thermal/qcom/tsens-v0_1.c | 56 +++++++++++++++++++++++++++++--
> drivers/thermal/qcom/tsens.c | 5 +--
> drivers/thermal/qcom/tsens.h | 16 ++++++++-
> 3 files changed, 72 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/thermal/qcom/tsens-v0_1.c b/drivers/thermal/qcom/tsens-v0_1.c
> index e89c6f39a3aea..c20c002d98650 100644
> --- a/drivers/thermal/qcom/tsens-v0_1.c
> +++ b/drivers/thermal/qcom/tsens-v0_1.c
> @@ -319,10 +319,31 @@ static const struct tsens_ops ops_8916 = {
> .get_temp = get_temp_common,
> };
>
> +struct tsens_reg_data reg_8916[] = {
> + {
> + .id = 0,
> + },
> + {
> + .id = 1,
> + },
> + {
> + .id = 2,
> + },
> + {
> + .id = 3,
> + },
> + {
> + .id = 4,
> + },
> + {
> + .id = 5,
> + },
> +};
> +
> struct tsens_plat_data data_8916 = {
> .num_sensors = 5,
> .ops = &ops_8916,
> - .hw_ids = (unsigned int []){0, 1, 2, 4, 5 },
> + .reg = reg_8916,
>
> .feat = &tsens_v0_1_feat,
> .fields = tsens_v0_1_regfields,
> @@ -334,10 +355,41 @@ static const struct tsens_ops ops_8939 = {
> .get_temp = get_temp_common,
> };
>
> +struct tsens_reg_data reg_8939[] = {
> + {
> + .id = 0,
> + },
> + {
> + .id = 1,
> + },
> + {
> + .id = 2,
> + },
> + {
> + .id = 3,
> + },
> + {
> + .id = 5,
> + },
> + {
> + .id = 6,
> + },
> + {
> + .id = 7,
> + },
> + {
> + .id = 8,
> + },
> + {
> + .id = 9,
Sensor 9 is contiguous. It's sensor with hwid=10, who requires two reads.
> + .p2_shift = 8,
> + },
> +};
> +
> struct tsens_plat_data data_8939 = {
> .num_sensors = 9,
> .ops = &ops_8939,
> - .hw_ids = (unsigned int []){ 0, 1, 2, 3, 5, 6, 7, 8, 9, /* 10 */ },
> + .reg = reg_8939,
>
> .feat = &tsens_v0_1_feat,
> .fields = tsens_v0_1_regfields,
> diff --git a/drivers/thermal/qcom/tsens.c b/drivers/thermal/qcom/tsens.c
> index 7165b0bfe8b9f..a260f563b4889 100644
> --- a/drivers/thermal/qcom/tsens.c
> +++ b/drivers/thermal/qcom/tsens.c
> @@ -1274,13 +1274,14 @@ static int tsens_probe(struct platform_device *pdev)
> priv->num_sensors = num_sensors;
> priv->ops = data->ops;
> for (i = 0; i < priv->num_sensors; i++) {
> - if (data->hw_ids)
> - priv->sensor[i].hw_id = data->hw_ids[i];
> + if (data->reg)
> + priv->sensor[i].hw_id = data->reg[i].id;
> else
> priv->sensor[i].hw_id = i;
> }
> priv->feat = data->feat;
> priv->fields = data->fields;
> + priv->reg = data->reg;
>
> platform_set_drvdata(pdev, priv);
>
> diff --git a/drivers/thermal/qcom/tsens.h b/drivers/thermal/qcom/tsens.h
> index dba9cd38f637c..31f67da03bce6 100644
> --- a/drivers/thermal/qcom/tsens.h
> +++ b/drivers/thermal/qcom/tsens.h
> @@ -517,18 +517,31 @@ struct tsens_features {
> int trip_max_temp;
> };
>
> +/**
> + * struct tsens_reg_data - describes register data retrieved non-contiguously
> + * @id: thermal sensor identifier
> + * @p1_shift: When non-zero is the # of bits to right shift p1 MSB by
> + * @p2_shift: When non-zero is the # of bits to right shift p2 MSB by
> + */
> +struct tsens_reg_data {
> + unsigned int id;
> + unsigned int p1_shift;
> + unsigned int p2_shift;
> +};
> +
> /**
> * struct tsens_plat_data - tsens compile-time platform data
> * @num_sensors: Number of sensors supported by platform
> * @ops: operations the tsens instance supports
> * @hw_ids: Subset of sensors ids supported by platform, if not the first n
> + * @reg: Describe sensor id and calibration shifts
> * @feat: features of the IP
> * @fields: bitfield locations
> */
> struct tsens_plat_data {
> const u32 num_sensors;
> const struct tsens_ops *ops;
> - unsigned int *hw_ids;
> + struct tsens_reg_data *reg;
> struct tsens_features *feat;
> const struct reg_field *fields;
> };
> @@ -575,6 +588,7 @@ struct tsens_priv {
> struct regmap_field *rf[MAX_REGFIELDS];
> struct tsens_context ctx;
> struct tsens_features *feat;
> + struct tsens_reg_data *reg;
> const struct reg_field *fields;
> const struct tsens_ops *ops;
>
--
With best wishes
Dmitry
On 06/04/2023 17:58, Bryan O'Donoghue wrote:
> In msm8939 some of the sensor calibration data traverses byte boundaries.
> Two examples of this are thermal sensor 2 point 1 and sensor 9 point 2.
>
> For sensor 2 point 1 we can get away with a simple read traversing byte
> boundaries as the calibration most significant bits are adjacent to the
> least significant across the byte boundary.
>
> In this case a read starting at the end of the first byte for nine bits
> will deliver up the data we want.
>
> In the case of sensor 9 point 2 however, the most significant bits are not
> adjacent and so therefore we need to perform two reads and or the bits
> together.
>
> If reg.p1_shift or reg.p2_shift is set then automatically search for
> pX_sY_msb in the dts applying pX_shift as a right shift or into the pX_sY
> value.
I think that having this in the common code is a bit of an overkill. No
other platform has this 'peculiarity' up to now. So, it might be better
to add 8939-specific calibration function that calls
tsens_read_calibration(), mixes in the s10_p2_msb and then calls
compute_intercept_slope().
>
> Signed-off-by: Bryan O'Donoghue <[email protected]>
> ---
> drivers/thermal/qcom/tsens.c | 33 +++++++++++++++++++++++++++++++++
> 1 file changed, 33 insertions(+)
>
> diff --git a/drivers/thermal/qcom/tsens.c b/drivers/thermal/qcom/tsens.c
> index a260f563b4889..eff2c8671c343 100644
> --- a/drivers/thermal/qcom/tsens.c
> +++ b/drivers/thermal/qcom/tsens.c
> @@ -74,6 +74,7 @@ int tsens_read_calibration(struct tsens_priv *priv, int shift, u32 *p1, u32 *p2,
> {
> u32 mode;
> u32 base1, base2;
> + u32 msb;
> char name[] = "sXX_pY_backup"; /* s10_p1_backup */
> int i, ret;
>
> @@ -122,6 +123,22 @@ int tsens_read_calibration(struct tsens_priv *priv, int shift, u32 *p1, u32 *p2,
>
> dev_dbg(priv->dev, "%s 0x%x\n", name, p1[i]);
>
> + if (priv->reg && priv->reg[i].p1_shift) {
> + ret = snprintf(name, sizeof(name), "s%d_p1_msb",
> + priv->sensor[i].hw_id);
> + if (ret < 0)
> + return ret;
> +
> + ret = nvmem_cell_read_variable_le_u32(priv->dev, name, &msb);
> + if (ret) {
> + dev_err(priv->dev, "Failed to read %s\n", name);
> + return ret;
> + }
> +
> + dev_dbg(priv->dev, "%s 0x%x\n", name, msb);
> + p1[i] |= msb >> priv->reg[i].p1_shift;
> + }
> +
> ret = snprintf(name, sizeof(name), "s%d_p2%s", priv->sensor[i].hw_id,
> backup ? "_backup" : "");
> if (ret < 0)
> @@ -134,6 +151,22 @@ int tsens_read_calibration(struct tsens_priv *priv, int shift, u32 *p1, u32 *p2,
> }
>
> dev_dbg(priv->dev, "%s 0x%x\n", name, p2[i]);
> +
> + if (priv->reg && priv->reg[i].p2_shift) {
> + ret = snprintf(name, sizeof(name), "s%d_p2_msb",
> + priv->sensor[i].hw_id);
> + if (ret < 0)
> + return ret;
> +
> + ret = nvmem_cell_read_variable_le_u32(priv->dev, name, &msb);
> + if (ret) {
> + dev_err(priv->dev, "Failed to read %s\n", name);
> + return ret;
> + }
> +
> + dev_dbg(priv->dev, "%s 0x%x\n", name, msb);
> + p2[i] |= msb >> priv->reg[i].p2_shift;
> + }
> }
>
> switch (mode) {
--
With best wishes
Dmitry
On 06/04/2023 17:20, Stephan Gerhold wrote:
>> Reviewed-by: Bryan O'Donoghue<[email protected]>
> So with sensor hwid=10 disabled, I think this patch series is actually
> not needed? ????
I can hardly be expected to remember back to January ..
dropping