From: Shawn Guo <shawn.guo@linaro.org>
To: Konrad Dybcio <konradybcio@gmail.com>
Cc: skrzynka@konradybcio.pl, Amit Kucheria <amit.kucheria@linaro.org>,
Andy Gross <agross@kernel.org>,
Bjorn Andersson <bjorn.andersson@linaro.org>,
Zhang Rui <rui.zhang@intel.com>,
Daniel Lezcano <daniel.lezcano@linaro.org>,
Rob Herring <robh+dt@kernel.org>,
linux-pm@vger.kernel.org, linux-arm-msm@vger.kernel.org,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/2] thermal: qcom: tsens-v0_1: Add support for MSM8939
Date: Sun, 28 Jun 2020 15:39:10 +0800 [thread overview]
Message-ID: <20200628073908.GA8343@dragon> (raw)
In-Reply-To: <20200501203311.143934-2-konradybcio@gmail.com>
On Fri, May 01, 2020 at 10:33:10PM +0200, Konrad Dybcio wrote:
> Signed-off-by: Konrad Dybcio <konradybcio@gmail.com>
For the record, I'm working my version of msm8939 tsens driver support,
and I would highlight the things that differ from this patch.
> ---
> drivers/thermal/qcom/tsens-v0_1.c | 142 +++++++++++++++++++++++++++++-
> drivers/thermal/qcom/tsens.c | 3 +
> drivers/thermal/qcom/tsens.h | 2 +-
> 3 files changed, 145 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/thermal/qcom/tsens-v0_1.c b/drivers/thermal/qcom/tsens-v0_1.c
> index 959a9371d205c..29b95886273b7 100644
> --- a/drivers/thermal/qcom/tsens-v0_1.c
> +++ b/drivers/thermal/qcom/tsens-v0_1.c
> @@ -48,6 +48,64 @@
> #define MSM8916_CAL_SEL_MASK 0xe0000000
> #define MSM8916_CAL_SEL_SHIFT 29
>
> +/* eeprom layout data for 8939 */
> +#define MSM8939_BASE0_MASK 0x000000ff
Tabs rather than spaces are used for indentation.
> +#define MSM8939_BASE1_MASK 0xff000000
> +#define MSM8939_BASE0_SHIFT
This shift is 0.
> +#define MSM8939_BASE1_SHIFT 24
> +
> +#define MSM8939_S0_P1_MASK 0x000001f8
> +#define MSM8939_S1_P1_MASK 0x001f8000
> +#define MSM8939_S2_P1_MASK_0_4 0xf8000000
> +#define MSM8939_S2_P1_MASK_5 0x00000001
This mask define is contradicting to MSM8939_S2_P1_SHIFT_5, and needs to
be confirmed.
> +#define MSM8939_S3_P1_MASK 0x00001f80
> +#define MSM8939_S4_P1_MASK 0x01f80000
> +#define MSM8939_S5_P1_MASK 0x00003f00
> +#define MSM8939_S6_P1_MASK 0x03f00000
> +#define MSM8939_S7_P1_MASK 0x0000003f
> +#define MSM8939_S8_P1_MASK 0x0003f000
> +#define MSM8939_S9_P1_MASK 0x07e00000
> +
> +#define MSM8939_S0_P2_MASK 0x00007e00
> +#define MSM8939_S1_P2_MASK 0x07e00000
> +#define MSM8939_S2_P2_MASK 0x0000007e
> +#define MSM8939_S3_P2_MASK 0x0007e000
> +#define MSM8939_S4_P2_MASK 0x7e000000
> +#define MSM8939_S5_P2_MASK 0x000fc000
> +#define MSM8939_S6_P2_MASK 0xfc000000
> +#define MSM8939_S7_P2_MASK 0x00000fc0
> +#define MSM8939_S8_P2_MASK 0x00fc0000
> +#define MSM8939_S9_P2_MASK_0_4 0xf8000000
> +#define MSM8939_S9_P2_MASK_5 0x00002000
It's contradicting to MSM8939_S9_P2_SHIFT_5, and needs to be confirmed.
> +
> +#define MSM8939_CAL_SEL_MASK 0xc0000000
Per downstream kernel, this mask is 0x7.
https://source.codeaurora.org/quic/la/kernel/msm-3.10/tree/drivers/thermal/msm8974-tsens.c?h=LA.BR.1.2.9.1_rb1.5#n370
> +#define MSM8939_CAL_SEL_SHIFT 0
> +
> +
> +#define MSM8939_S0_P1_SHIFT 3
> +#define MSM8939_S1_P1_SHIFT 15
> +#define MSM8939_S2_P1_SHIFT_0_4 27
> +#define MSM8939_S2_P1_SHIFT_5 5
> +#define MSM8939_S3_P1_SHIFT 7
> +#define MSM8939_S4_P1_SHIFT 19
> +#define MSM8939_S5_P1_SHIFT 8
> +#define MSM8939_S6_P1_SHIFT 20
> +//yes, 7 is missing in downstream
The shift is not missing but just 0.
> +#define MSM8939_S8_P1_SHIFT 12
> +#define MSM8939_S9_P1_SHIFT 21
> +
> +#define MSM8939_S0_P2_SHIFT 9
> +#define MSM8939_S1_P2_SHIFT 21
> +#define MSM8939_S2_P2_SHIFT 1
> +#define MSM8939_S3_P2_SHIFT 13
> +#define MSM8939_S4_P2_SHIFT 25
> +#define MSM8939_S5_P2_SHIFT 14
> +#define MSM8939_S6_P2_SHIFT 26
> +#define MSM8939_S7_P2_SHIFT 6
> +#define MSM8939_S8_P2_SHIFT 18
> +#define MSM8939_S9_P2_SHIFT_0_4 27
> +#define MSM8939_S9_P2_SHIFT_5 8
> +
> /* eeprom layout data for 8974 */
> #define BASE1_MASK 0xff
> #define S0_P1_MASK 0x3f00
> @@ -189,6 +247,73 @@ static int calibrate_8916(struct tsens_priv *priv)
> return 0;
> }
>
> +static int calibrate_8939(struct tsens_priv *priv)
> +{
> + int base0 = 0, base1 = 0, i;
> + u32 p1[11], p2[11];
MSM8939 gets 10 sensors, so the arrays should be [10].
> + int mode = 0;
> + u32 *qfprom_cdata, *qfprom_csel;
> +
> + qfprom_cdata = (u32 *)qfprom_read(priv->dev, "calib");
> + if (IS_ERR(qfprom_cdata))
> + return PTR_ERR(qfprom_cdata);
> +
> + qfprom_csel = (u32 *)qfprom_read(priv->dev, "calib_sel");
> + if (IS_ERR(qfprom_csel)) {
> + kfree(qfprom_cdata);
> + return PTR_ERR(qfprom_csel);
> + }
'calib_sel' is not separate from 'calib' bits on MSM8939. I chose to
read nvmem out as one go and map them to calibration data as below, per
downstream kernel.
/* Mapping between qfprom nvmem and calibration data */
cdata[0] = qfprom_cdata[12];
cdata[1] = qfprom_cdata[13];
cdata[2] = qfprom_cdata[0];
cdata[3] = qfprom_cdata[1];
cdata[4] = qfprom_cdata[22];
cdata[5] = qfprom_cdata[21];
mode = (cdata[0] & MSM8939_CAL_SEL_MASK) >> MSM8939_CAL_SEL_SHIFT;
https://source.codeaurora.org/quic/la/kernel/msm-3.10/tree/drivers/thermal/msm8974-tsens.c?h=LA.BR.1.2.9.1_rb1.5#n1286
We should support MSM8939 v3, as that's the mass production revision.
> +
> + mode = (qfprom_csel[0] & MSM8939_CAL_SEL_MASK) >> MSM8939_CAL_SEL_SHIFT;
> + dev_dbg(priv->dev, "calibration mode is %d\n", mode);
> + switch (mode) {
> + case TWO_PT_CALIB:
> + base1 = (qfprom_cdata[1] & MSM8939_BASE1_MASK) >> MSM8939_BASE1_SHIFT;
> + p2[0] = (qfprom_cdata[0] & MSM8939_S0_P2_MASK) >> MSM8939_S0_P2_SHIFT;
> + p2[1] = (qfprom_cdata[0] & MSM8939_S1_P2_MASK) >> MSM8939_S1_P2_SHIFT;
> + p2[2] = (qfprom_cdata[1] & MSM8939_S2_P2_MASK) >> MSM8939_S2_P2_SHIFT;
> + p2[3] = (qfprom_cdata[1] & MSM8939_S3_P2_MASK) >> MSM8939_S3_P2_SHIFT;
> + p2[4] = (qfprom_cdata[1] & MSM8939_S4_P2_MASK) >> MSM8939_S4_P2_SHIFT;
> + p2[5] = (qfprom_cdata[1] & MSM8939_S5_P2_MASK) >> MSM8939_S5_P2_SHIFT;
> + p2[6] = (qfprom_cdata[1] & MSM8939_S6_P2_MASK) >> MSM8939_S6_P2_SHIFT;
> + p2[7] = (qfprom_cdata[1] & MSM8939_S7_P2_MASK) >> MSM8939_S7_P2_SHIFT;
> + p2[8] = (qfprom_cdata[1] & MSM8939_S8_P2_MASK) >> MSM8939_S8_P2_SHIFT;
> + p2[9] = (qfprom_cdata[1] & MSM8939_S9_P2_MASK_0_4) >> MSM8939_S9_P2_SHIFT_0_4;
> + p2[10] = (qfprom_cdata[1] & MSM8939_S9_P2_MASK_5) >> MSM8939_S9_P2_SHIFT_5;
These do not really match downstream kernel. My version look like:
base1 = (cdata[3] & MSM8939_BASE1_MASK) >> MSM8939_BASE1_SHIFT;
p2[0] = (cdata[0] & MSM8939_S0_P2_MASK) >> MSM8939_S0_P2_SHIFT;
p2[1] = (cdata[0] & MSM8939_S1_P2_MASK) >> MSM8939_S1_P2_SHIFT;
p2[2] = (cdata[1] & MSM8939_S2_P2_MASK) >> MSM8939_S2_P2_SHIFT;
p2[3] = (cdata[1] & MSM8939_S3_P2_MASK) >> MSM8939_S3_P2_SHIFT;
p2[4] = (cdata[1] & MSM8939_S4_P2_MASK) >> MSM8939_S4_P2_SHIFT;
p2[5] = (cdata[2] & MSM8939_S5_P2_MASK) >> MSM8939_S5_P2_SHIFT;
p2[6] = (cdata[2] & MSM8939_S6_P2_MASK) >> MSM8939_S6_P2_SHIFT;
p2[7] = (cdata[3] & MSM8939_S7_P2_MASK) >> MSM8939_S7_P2_SHIFT;
p2[8] = (cdata[3] & MSM8939_S8_P2_MASK) >> MSM8939_S8_P2_SHIFT;
p2[9] = (cdata[4] & MSM8939_S9_P2_MASK_0_4) >> MSM8939_S9_P2_SHIFT_0_4;
p2[9] |= (cdata[5] & MSM8939_S9_P2_MASK_5) >> MSM8939_S9_P2_SHIFT_5;
> + for (i = 0; i < priv->num_sensors; i++)
> + p2[i] = ((base1 + p2[i]) << 3);
The left shift should be 2 per downstream kernel.
https://source.codeaurora.org/quic/la/kernel/msm-3.10/tree/drivers/thermal/msm8974-tsens.c?h=LA.BR.1.2.9.1_rb1.5#n1407
> + /* Fall through */
> + case ONE_PT_CALIB2:
> + base0 = (qfprom_cdata[0] & MSM8939_BASE0_MASK);
> + p1[0] = (qfprom_cdata[0] & MSM8939_S0_P1_MASK) >> MSM8939_S0_P1_SHIFT;
> + p1[1] = (qfprom_cdata[0] & MSM8939_S1_P1_MASK) >> MSM8939_S1_P1_SHIFT;
> + p1[2] = (qfprom_cdata[0] & MSM8939_S2_P1_MASK_0_4) >> MSM8939_S2_P1_SHIFT_0_4;
> + p1[3] = (qfprom_cdata[0] & MSM8939_S2_P1_MASK_5) >> MSM8939_S2_P1_SHIFT_5;
> + p1[4] = (qfprom_cdata[1] & MSM8939_S3_P1_MASK) >> MSM8939_S3_P1_SHIFT;
> + p1[5] = (qfprom_cdata[1] & MSM8939_S4_P1_MASK) >> MSM8939_S4_P1_SHIFT;
> + p1[6] = (qfprom_cdata[1] & MSM8939_S5_P1_MASK) >> MSM8939_S5_P1_SHIFT;
> + p1[7] = (qfprom_cdata[1] & MSM8939_S6_P1_MASK) >> MSM8939_S6_P1_SHIFT;
> + //yes, 7 is missing in downstream
Not really.
https://source.codeaurora.org/quic/la/kernel/msm-3.10/tree/drivers/thermal/msm8974-tsens.c?h=LA.BR.1.2.9.1_rb1.5#n1331
> + p1[8] = (qfprom_cdata[1] & MSM8939_S8_P1_MASK) >> MSM8939_S8_P1_SHIFT;
> + p1[9] = (qfprom_cdata[1] & MSM8939_S9_P1_MASK) >> MSM8939_S9_P1_SHIFT;
> + for (i = 0; i < priv->num_sensors; i++)
> + p1[i] = (((base0) + p1[i]) << 3);
> + break;
> + default:
> + for (i = 0; i < priv->num_sensors; i++) {
> + p1[i] = 500;
> + p2[i] = 780;
> + }
> + break;
> + }
> +
> + compute_intercept_slope(priv, p1, p2, mode);
> + kfree(qfprom_cdata);
> + kfree(qfprom_csel);
> +
> + return 0;
> +}
> +
> static int calibrate_8974(struct tsens_priv *priv)
> {
> int base1 = 0, base2 = 0, i;
> @@ -325,7 +450,7 @@ static int calibrate_8974(struct tsens_priv *priv)
> return 0;
> }
>
> -/* v0.1: 8916, 8974 */
> +/* v0.1: 8916, 8939, 8974 */
>
> static struct tsens_features tsens_v0_1_feat = {
> .ver_major = VER_0_1,
> @@ -386,6 +511,21 @@ struct tsens_plat_data data_8916 = {
> .fields = tsens_v0_1_regfields,
> };
>
> +static const struct tsens_ops ops_8939 = {
> + .init = init_common,
> + .calibrate = calibrate_8939,
> + .get_temp = get_temp_common,
> +};
> +
> +struct tsens_plat_data data_8939 = {
> + .num_sensors = 10,
> + .ops = &ops_8939,
> + .hw_ids = (unsigned int []){0, 1, 2, 4, 5, 6, 7, 8, 9 },
Should be { 0, 1, 2, 4, 5, 6, 7, 8, 9, 10 }.
https://source.codeaurora.org/quic/la/kernel/msm-3.10/tree/arch/arm/boot/dts/qcom/msm8939-v3.0.dtsi?h=LA.BR.1.2.9.1_rb1.5#n670
Shawn
> +
> + .feat = &tsens_v0_1_feat,
> + .fields = tsens_v0_1_regfields,
> +};
> +
> static const struct tsens_ops ops_8974 = {
> .init = init_common,
> .calibrate = calibrate_8974,
> diff --git a/drivers/thermal/qcom/tsens.c b/drivers/thermal/qcom/tsens.c
> index 2f77d235cf735..f654057e96ae1 100644
> --- a/drivers/thermal/qcom/tsens.c
> +++ b/drivers/thermal/qcom/tsens.c
> @@ -59,6 +59,9 @@ static const struct of_device_id tsens_table[] = {
> {
> .compatible = "qcom,msm8916-tsens",
> .data = &data_8916,
> + }, {
> + .compatible = "qcom,msm8939-tsens",
> + .data = &data_8939,
> }, {
> .compatible = "qcom,msm8974-tsens",
> .data = &data_8974,
> diff --git a/drivers/thermal/qcom/tsens.h b/drivers/thermal/qcom/tsens.h
> index 502acf0e68285..403b15546f648 100644
> --- a/drivers/thermal/qcom/tsens.h
> +++ b/drivers/thermal/qcom/tsens.h
> @@ -590,7 +590,7 @@ irqreturn_t tsens_critical_irq_thread(int irq, void *data);
> extern struct tsens_plat_data data_8960;
>
> /* TSENS v0.1 targets */
> -extern struct tsens_plat_data data_8916, data_8974;
> +extern struct tsens_plat_data data_8916, data_8939, data_8974;
>
> /* TSENS v1 targets */
> extern struct tsens_plat_data data_tsens_v1, data_8976;
> --
> 2.26.1
>
next prev parent reply other threads:[~2020-06-28 7:39 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-05-01 20:33 [PATCH 0/2] Add msm8939 tsensv0_1 support Konrad Dybcio
2020-05-01 20:33 ` [PATCH 1/2] thermal: qcom: tsens-v0_1: Add support for MSM8939 Konrad Dybcio
2020-05-04 6:31 ` Amit Kucheria
2020-05-04 8:04 ` Konrad Dybcio
2020-06-28 7:39 ` Shawn Guo [this message]
2020-06-28 8:02 ` Konrad Dybcio
2020-05-01 20:33 ` [PATCH 2/2] dt-bindings: tsens: qcom: Document MSM8939 compatible Konrad Dybcio
2020-05-04 6:28 ` Amit Kucheria
2020-05-04 19:51 ` Rob Herring
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200628073908.GA8343@dragon \
--to=shawn.guo@linaro.org \
--cc=agross@kernel.org \
--cc=amit.kucheria@linaro.org \
--cc=bjorn.andersson@linaro.org \
--cc=daniel.lezcano@linaro.org \
--cc=devicetree@vger.kernel.org \
--cc=konradybcio@gmail.com \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pm@vger.kernel.org \
--cc=robh+dt@kernel.org \
--cc=rui.zhang@intel.com \
--cc=skrzynka@konradybcio.pl \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.