Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <1c178228-2b83-e319-129f-4524b5a1bc8b@quicinc.com>
Date:   Tue, 11 Jul 2023 14:49:06 +0530
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.13.0
Subject: Re: [PATCH 2/6] thermal/drivers/tsens: Add TSENS enable and
 calibration support for V2
Content-Language: en-US
To:     Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
CC:     <agross@kernel.org>, <andersson@kernel.org>,
        <konrad.dybcio@linaro.org>, <amitk@kernel.org>,
        <thara.gopinath@gmail.com>, <rafael@kernel.org>,
        <daniel.lezcano@linaro.org>, <rui.zhang@intel.com>,
        <robh+dt@kernel.org>, <krzysztof.kozlowski+dt@linaro.org>,
        <conor+dt@kernel.org>, <linux-arm-msm@vger.kernel.org>,
        <linux-pm@vger.kernel.org>, <devicetree@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>, <quic_varada@quicinc.com>
References: <20230710103735.1375847-1-quic_ipkumar@quicinc.com>
 <20230710103735.1375847-3-quic_ipkumar@quicinc.com>
 <cf2cae88-e226-8df1-6c4f-543b683cbc8c@linaro.org>
 <a03d6823-2084-e4c6-1c9d-853127956f69@quicinc.com>
 <CAA8EJpo+r48uj1-X6O0r3wOodydMfMrPQHt+afXO7PNGqCJk3A@mail.gmail.com>
From:   Praveenkumar I <quic_ipkumar@quicinc.com>
In-Reply-To: <CAA8EJpo+r48uj1-X6O0r3wOodydMfMrPQHt+afXO7PNGqCJk3A@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 7/10/2023 8:32 PM, Dmitry Baryshkov wrote:
> On Mon, 10 Jul 2023 at 16:22, Praveenkumar I <quic_ipkumar@quicinc.com> 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@quicinc.com>
>>>> ---
>>>>    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.
Sure, will update in next patch.
>
>>>> +        }
>>>> +    }
>>>> +
>>>> +    /* 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
>
>