Re: [PATCH v3 2/4] drivers: thermal: tsens: Add generic support for TSENS v1 IP

From: Eduardo Valentin
Date: Thu Nov 29 2018 - 11:56:14 EST

Next message: Eduardo Valentin: "Re: [PATCH v3 4/4] arm64: dts: qcom: qcs404: Add thermal zones for each sensor"
Previous message: Juergen Gross: "Re: [PATCH] Revert "xen/balloon: Mark unallocated host memory as UNUSABLE""
In reply to: Amit Kucheria: "[PATCH v3 2/4] drivers: thermal: tsens: Add generic support for TSENS v1 IP"
Next in thread: Amit Kucheria: "[PATCH v3 3/4] arm64: dts: qcom: qcs404: Add tsens controller"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Tue, Nov 27, 2018 at 09:59:05PM +0530, Amit Kucheria wrote:
> qcs404 has a single TSENS IP block with 10 sensors. It uses version 1.4
> of the TSENS IP, functionality for which is encapsulated inside
> qcom,tsens-v1 compatible.
>
> Signed-off-by: Amit Kucheria <amit.kucheria@xxxxxxxxxx>
> Reviewed-by: Vinod Koul <vkoul@xxxxxxxxxx>
> Tested-by: Vinod Koul <vkoul@xxxxxxxxxx>
> ---
> drivers/thermal/qcom/Makefile | 2 +-
> drivers/thermal/qcom/tsens-common.c | 2 +-
> drivers/thermal/qcom/tsens-v1.c | 196 ++++++++++++++++++++++++++++
> drivers/thermal/qcom/tsens.c | 3 +
> drivers/thermal/qcom/tsens.h | 3 +-
> 5 files changed, 203 insertions(+), 3 deletions(-)
> create mode 100644 drivers/thermal/qcom/tsens-v1.c
>
> diff --git a/drivers/thermal/qcom/Makefile b/drivers/thermal/qcom/Makefile
> index a821929ede0b..60269ee90c43 100644
> --- a/drivers/thermal/qcom/Makefile
> +++ b/drivers/thermal/qcom/Makefile
> @@ -1,2 +1,2 @@
> obj-$(CONFIG_QCOM_TSENS) += qcom_tsens.o
> -qcom_tsens-y += tsens.o tsens-common.o tsens-8916.o tsens-8974.o tsens-8960.o tsens-v2.o
> +qcom_tsens-y += tsens.o tsens-common.o tsens-8916.o tsens-8974.o tsens-8960.o tsens-v2.o tsens-v1.o
> diff --git a/drivers/thermal/qcom/tsens-common.c b/drivers/thermal/qcom/tsens-common.c
> index 3be4be2e0465..98f77401bc12 100644
> --- a/drivers/thermal/qcom/tsens-common.c
> +++ b/drivers/thermal/qcom/tsens-common.c
> @@ -76,7 +76,7 @@ void compute_intercept_slope(struct tsens_device *tmdev, u32 *p1,
> }
> }
>
> -static inline int code_to_degc(u32 adc_code, const struct tsens_sensor *s)
> +int code_to_degc(u32 adc_code, const struct tsens_sensor *s)
> {
> int degc, num, den;
>
> diff --git a/drivers/thermal/qcom/tsens-v1.c b/drivers/thermal/qcom/tsens-v1.c
> new file mode 100644
> index 000000000000..1dbf4fde6da8
> --- /dev/null
> +++ b/drivers/thermal/qcom/tsens-v1.c
> @@ -0,0 +1,196 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2018, Linaro Limited
> + */
> +
> +#include <linux/regmap.h>
> +#include <linux/bitops.h>
> +#include "tsens.h"
> +
> +/* eeprom layout data for qcs404 (v1) */
> +#define BASE0_MASK 0x000007f8
> +#define BASE1_MASK 0x0007f800
> +#define BASE0_SHIFT 3
> +#define BASE1_SHIFT 11
> +
> +#define S0_P1_MASK 0x0000003f
> +#define S1_P1_MASK 0x0003f000
> +#define S2_P1_MASK 0x3f000000
> +#define S3_P1_MASK 0x000003f0
> +#define S4_P1_MASK 0x003f0000
> +#define S5_P1_MASK 0x0000003f
> +#define S6_P1_MASK 0x0003f000
> +#define S7_P1_MASK 0x3f000000
> +#define S8_P1_MASK 0x000003f0
> +#define S9_P1_MASK 0x003f0000
> +
> +#define S0_P2_MASK 0x00000fc0
> +#define S1_P2_MASK 0x00fc0000
> +#define S2_P2_MASK_1_0 0xc0000000
> +#define S2_P2_MASK_5_2 0x0000000f
> +#define S3_P2_MASK 0x0000fc00
> +#define S4_P2_MASK 0x0fc00000
> +#define S5_P2_MASK 0x00000fc0
> +#define S6_P2_MASK 0x00fc0000
> +#define S7_P2_MASK_1_0 0xc0000000
> +#define S7_P2_MASK_5_2 0x0000000f
> +#define S8_P2_MASK 0x0000fc00
> +#define S9_P2_MASK 0x0fc00000
> +
> +#define S0_P1_SHIFT 0
> +#define S0_P2_SHIFT 6
> +#define S1_P1_SHIFT 12
> +#define S1_P2_SHIFT 18
> +#define S2_P1_SHIFT 24
> +#define S2_P2_SHIFT_1_0 30
> +
> +#define S2_P2_SHIFT_5_2 0
> +#define S3_P1_SHIFT 4
> +#define S3_P2_SHIFT 10
> +#define S4_P1_SHIFT 16
> +#define S4_P2_SHIFT 22
> +
> +#define S5_P1_SHIFT 0
> +#define S5_P2_SHIFT 6
> +#define S6_P1_SHIFT 12
> +#define S6_P2_SHIFT 18
> +#define S7_P1_SHIFT 24
> +#define S7_P2_SHIFT_1_0 30
> +
> +#define S7_P2_SHIFT_5_2 0
> +#define S8_P1_SHIFT 4
> +#define S8_P2_SHIFT 10
> +#define S9_P1_SHIFT 16
> +#define S9_P2_SHIFT 22
> +
> +#define CAL_SEL_MASK 7
> +#define CAL_SEL_SHIFT 0
> +
> +static int calibrate_v1(struct tsens_device *tmdev)
> +{
> + u32 base0 = 0, base1 = 0;
> + u32 p1[10], p2[10];
> + u32 mode = 0, lsb = 0, msb = 0;
> + u32 *qfprom_cdata;
> + int i;
> +
> + qfprom_cdata = (u32 *)qfprom_read(tmdev->dev, "calib");
> + if (IS_ERR(qfprom_cdata))
> + return PTR_ERR(qfprom_cdata);
> +
> + mode = (qfprom_cdata[4] & CAL_SEL_MASK) >> CAL_SEL_SHIFT;
> + dev_dbg(tmdev->dev, "calibration mode is %d\n", mode);
> +
> + switch (mode) {
> + case TWO_PT_CALIB:
> + base1 = (qfprom_cdata[4] & BASE1_MASK) >> BASE1_SHIFT;
> + p2[0] = (qfprom_cdata[0] & S0_P2_MASK) >> S0_P2_SHIFT;
> + p2[1] = (qfprom_cdata[0] & S1_P2_MASK) >> S1_P2_SHIFT;
> + /* This value is split over two registers, 2 bits and 4 bits */
> + lsb = (qfprom_cdata[0] & S2_P2_MASK_1_0) >> S2_P2_SHIFT_1_0;
> + msb = (qfprom_cdata[1] & S2_P2_MASK_5_2) >> S2_P2_SHIFT_5_2;
> + p2[2] = msb << 2 | lsb;
> + p2[3] = (qfprom_cdata[1] & S3_P2_MASK) >> S3_P2_SHIFT;
> + p2[4] = (qfprom_cdata[1] & S4_P2_MASK) >> S4_P2_SHIFT;
> + p2[5] = (qfprom_cdata[2] & S5_P2_MASK) >> S5_P2_SHIFT;
> + p2[6] = (qfprom_cdata[2] & S6_P2_MASK) >> S6_P2_SHIFT;
> + /* This value is split over two registers, 2 bits and 4 bits */
> + lsb = (qfprom_cdata[2] & S7_P2_MASK_1_0) >> S7_P2_SHIFT_1_0;
> + msb = (qfprom_cdata[3] & S7_P2_MASK_5_2) >> S7_P2_SHIFT_5_2;
> + p2[7] = msb << 2 | lsb;
> + p2[8] = (qfprom_cdata[3] & S8_P2_MASK) >> S8_P2_SHIFT;
> + p2[9] = (qfprom_cdata[3] & S9_P2_MASK) >> S9_P2_SHIFT;
> + for (i = 0; i < tmdev->num_sensors; i++)
> + p2[i] = ((base1 + p2[i]) << 2);
> + /* Fall through */
> + case ONE_PT_CALIB2:
> + base0 = (qfprom_cdata[4] & BASE0_MASK) >> BASE0_SHIFT;
> + p1[0] = (qfprom_cdata[0] & S0_P1_MASK) >> S0_P1_SHIFT;
> + p1[1] = (qfprom_cdata[0] & S1_P1_MASK) >> S1_P1_SHIFT;
> + p1[2] = (qfprom_cdata[0] & S2_P1_MASK) >> S2_P1_SHIFT;
> + p1[3] = (qfprom_cdata[1] & S3_P1_MASK) >> S3_P1_SHIFT;
> + p1[4] = (qfprom_cdata[1] & S4_P1_MASK) >> S4_P1_SHIFT;
> + p1[5] = (qfprom_cdata[2] & S5_P1_MASK) >> S5_P1_SHIFT;
> + p1[6] = (qfprom_cdata[2] & S6_P1_MASK) >> S6_P1_SHIFT;
> + p1[7] = (qfprom_cdata[2] & S7_P1_MASK) >> S7_P1_SHIFT;
> + p1[8] = (qfprom_cdata[3] & S8_P1_MASK) >> S8_P1_SHIFT;
> + p1[9] = (qfprom_cdata[3] & S9_P1_MASK) >> S9_P1_SHIFT;
> + for (i = 0; i < tmdev->num_sensors; i++)
> + p1[i] = (((base0) + p1[i]) << 2);
> + break;
> + default:
> + for (i = 0; i < tmdev->num_sensors; i++) {
> + p1[i] = 500;
> + p2[i] = 780;
... Wouldn't be nice to know what 500 or 780 stands for here?
Why these defaults? Do we expect to have further patches to keep
updating these?

> + }
> + break;
> + }
> +
> + compute_intercept_slope(tmdev, p1, p2, mode);
> +
> + return 0;
> +}
> +
> +#define STATUS_OFFSET 0x44
> +#define LAST_TEMP_MASK 0x3ff
> +#define STATUS_VALID_BIT BIT(14)
> +
> +static int get_temp_tsens_v1(struct tsens_device *tmdev, int id, int *temp)
> +{
> + struct tsens_sensor *s = &tmdev->sensor[id];
> + u32 code;
> + unsigned int status_reg;
> + u32 last_temp = 0, last_temp2 = 0, last_temp3 = 0;
> + int ret;
> +
> + status_reg = tmdev->tm_offset + STATUS_OFFSET + s->hw_id * 4;
> + ret = regmap_read(tmdev->tm_map, status_reg, &code);
> + if (ret)
> + return ret;
> + last_temp = code & LAST_TEMP_MASK;
> + if (code & STATUS_VALID_BIT)
> + goto done;
> +
> + /* Try a second time */
> + ret = regmap_read(tmdev->tm_map, status_reg, &code);
> + if (ret)
> + return ret;
> + if (code & STATUS_VALID_BIT) {
> + last_temp = code & LAST_TEMP_MASK;
> + goto done;
> + } else {
> + last_temp2 = code & LAST_TEMP_MASK;
> + }
> +
> + /* Try a third/last time */
> + ret = regmap_read(tmdev->tm_map, status_reg, &code);
> + if (ret)
> + return ret;
> + if (code & STATUS_VALID_BIT) {
> + last_temp = code & LAST_TEMP_MASK;
> + goto done;
> + } else {
> + last_temp3 = code & LAST_TEMP_MASK;
> + }
> +
> + if (last_temp == last_temp2)
> + last_temp = last_temp2;
> + else if (last_temp2 == last_temp3)
> + last_temp = last_temp3;
> +done:
> + /* Convert temperature from ADC code to milliCelsius */
> + *temp = code_to_degc(last_temp, s) * 1000;

This three strikes/read approach seams awkward. Is this a broken
sensor or are you missing the programming steps? Maybe you need to
either wait for a IRQ on temp ready, or maybe you need simply wait
some amount of cycles before the sensor is ready with new temp/ADC
conversion, no?

Also, the goto's dont really help, as we dont really have any
resource to rollback here..

> +
> + return 0;
> +}
> +
> +static const struct tsens_ops ops_generic_v1 = {
> + .init = init_common,
> + .calibrate = calibrate_v1,
> + .get_temp = get_temp_tsens_v1,
> +};
> +
> +const struct tsens_data data_tsens_v1 = {
> + .ops = &ops_generic_v1,
> + .reg_offsets = { [SROT_CTRL_OFFSET] = 0x4 },
> +};
> diff --git a/drivers/thermal/qcom/tsens.c b/drivers/thermal/qcom/tsens.c
> index f1ec9bbe4717..d0cc0c09894a 100644
> --- a/drivers/thermal/qcom/tsens.c
> +++ b/drivers/thermal/qcom/tsens.c
> @@ -63,6 +63,9 @@ static const struct of_device_id tsens_table[] = {
> }, {
> .compatible = "qcom,msm8996-tsens",
> .data = &data_8996,
> + }, {
> + .compatible = "qcom,tsens-v1",
> + .data = &data_tsens_v1,
> }, {
> .compatible = "qcom,tsens-v2",
> .data = &data_tsens_v2,
> diff --git a/drivers/thermal/qcom/tsens.h b/drivers/thermal/qcom/tsens.h
> index 7b7feee5dc46..f8dc96c42b94 100644
> --- a/drivers/thermal/qcom/tsens.h
> +++ b/drivers/thermal/qcom/tsens.h
> @@ -90,9 +90,10 @@ char *qfprom_read(struct device *, const char *);
> void compute_intercept_slope(struct tsens_device *, u32 *, u32 *, u32);
> int init_common(struct tsens_device *);
> int get_temp_common(struct tsens_device *, int, int *);
> +int code_to_degc(u32 adc_code, const struct tsens_sensor *s);
>
> /* TSENS v1 targets */
> -extern const struct tsens_data data_8916, data_8974, data_8960;
> +extern const struct tsens_data data_8916, data_8974, data_8960, data_tsens_v1;
> /* TSENS v2 targets */
> extern const struct tsens_data data_8996, data_tsens_v2;
>
> --
> 2.17.1
>

Next message: Eduardo Valentin: "Re: [PATCH v3 4/4] arm64: dts: qcom: qcs404: Add thermal zones for each sensor"
Previous message: Juergen Gross: "Re: [PATCH] Revert "xen/balloon: Mark unallocated host memory as UNUSABLE""
In reply to: Amit Kucheria: "[PATCH v3 2/4] drivers: thermal: tsens: Add generic support for TSENS v1 IP"
Next in thread: Amit Kucheria: "[PATCH v3 3/4] arm64: dts: qcom: qcs404: Add tsens controller"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]