| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <644ae31f-3829-49bc-942e-4b5e87aed140@quicinc.com>
Date: Mon, 10 Jul 2023 18:54:12 +0530
From: Praveenkumar I <quic_ipkumar@...cinc.com>
To: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
<agross@...nel.org>, <andersson@...nel.org>,
<konrad.dybcio@...aro.org>, <amitk@...nel.org>,
<thara.gopinath@...il.com>, <rafael@...nel.org>,
<daniel.lezcano@...aro.org>, <rui.zhang@...el.com>,
<robh+dt@...nel.org>, <krzysztof.kozlowski+dt@...aro.org>,
<conor+dt@...nel.org>, <linux-arm-msm@...r.kernel.org>,
<linux-pm@...r.kernel.org>, <devicetree@...r.kernel.org>,
<linux-kernel@...r.kernel.org>
CC: <quic_varada@...cinc.com>
Subject: Re: [PATCH 2/6] thermal/drivers/tsens: Add TSENS enable and
calibration support for V2
On 7/10/2023 4:52 PM, Dmitry Baryshkov wrote:
> On 10/07/2023 14:19, 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@...cinc.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.
>>
>>> + }
>>> + }
>>> +
>>> + /* 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).
>>
>>> +
>>> + 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?
>
> After taking a look at the patch 6, can we just add a separate init()
> function which will call init_common() and then initialize these fields?
Sure, will add separate init() function in tsens-v2.c
>
>>
>>> + 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?
>>
>>> 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
Powered by blists - more mailing lists