Re: [PATCH 1/2] drivers: thermal: tsens: Add 0C (zeorC) interrupt support

From: Amit Kucheria
Date: Mon May 18 2020 - 11:33:53 EST

On Sun, May 17, 2020 at 3:58 PM <manafm@xxxxxxxxxxxxxx> wrote:
>
> On 2020-05-05 17:39, Amit Kucheria wrote:
> > Hi Manaf,
> >
> > Typo: fix zeorC in subject line.
> Done
> >
> > Please rebase this patch[1] on top of my patch merging tsens-common.c
> > and tsens.c.
> >
> > [1]
> > https://lore.kernel.org/linux-arm-msm/e30e2ba6fa5c007983afd4d7d4e0311c0b57917a.1588183879.git.amit.kucheria@xxxxxxxxxx/
> Done
> >
> > On Tue, May 5, 2020 at 4:42 PM Manaf Meethalavalappu Pallikunhi
> > <manafm@xxxxxxxxxxxxxx> wrote:
> >>
> >> TSENS IP v2.6+ adds 0C interrupt support. It triggers set
> >> interrupt when aggregated minimum temperature of all TSENS falls
> >> below 0C preset threshold and triggers reset interrupt when aggregate
> >> minimum temperature of all TSENS crosses above reset threshold.
> >> Add support for this interrupt in the driver.
> >>
> >> It adds another sensor to the of-thermal along with all individual
> >> TSENS. It enables to add any mitigation for 0C interrupt.
> >>
> >> Signed-off-by: Manaf Meethalavalappu Pallikunhi
> >> <manafm@xxxxxxxxxxxxxx>
> >> ---
> >> drivers/thermal/qcom/tsens-common.c | 72
> >> ++++++++++++++++++++++++++++-
> >> drivers/thermal/qcom/tsens-v2.c | 7 +++
> >> drivers/thermal/qcom/tsens.c | 51 ++++++++++++++++++--
> >> drivers/thermal/qcom/tsens.h | 11 +++++
> >> 4 files changed, 135 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/thermal/qcom/tsens-common.c
> >> b/drivers/thermal/qcom/tsens-common.c
> >> index 172545366636..44e7edeb9a90 100644
> >> --- a/drivers/thermal/qcom/tsens-common.c
> >> +++ b/drivers/thermal/qcom/tsens-common.c
> >> @@ -198,7 +198,8 @@ static void tsens_set_interrupt_v1(struct
> >> tsens_priv *priv, u32 hw_id,
> >> index = LOW_INT_CLEAR_0 + hw_id;
> >> break;
> >> case CRITICAL:
> >> - /* No critical interrupts before v2 */
> >> + case ZEROC:
> >> + /* No critical and 0c interrupts before v2 */
> >> return;
> >> }
> >> regmap_field_write(priv->rf[index], enable ? 0 : 1);
> >> @@ -229,6 +230,9 @@ static void tsens_set_interrupt_v2(struct
> >> tsens_priv *priv, u32 hw_id,
> >> index_mask = CRIT_INT_MASK_0 + hw_id;
> >> index_clear = CRIT_INT_CLEAR_0 + hw_id;
> >> break;
> >> + case ZEROC:
> >> + /* Nothing to handle for 0c interrupt */
> >> + return;
> >> }
> >>
> >> if (enable) {
> >> @@ -360,6 +364,34 @@ static inline u32 masked_irq(u32 hw_id, u32 mask,
> >> enum tsens_ver ver)
> >> return 0;
> >> }
> >>
> >> +/**
> >> + * tsens_0c_irq_thread - Threaded interrupt handler for 0c interrupt
> >
> > Let's use zeroc instead of 0c in the function and variable names and
> > comments everywhere. Easier to grep and better consistency too.
> Done
> >
> >> + * @irq: irq number
> >> + * @data: tsens controller private data
> >> + *
> >> + * Whenever interrupt triggers notify thermal framework using
> >> + * thermal_zone_device_update() to update cold temperature
> >> mitigation.
> >
> > How is this mitigation updated?
> Updated comment section
> >> + *
> >> + * Return: IRQ_HANDLED
> >> + */
> >> +irqreturn_t tsens_0c_irq_thread(int irq, void *data)
> >> +{
> >> + struct tsens_priv *priv = data;
> >> + struct tsens_sensor *s = &priv->sensor[priv->num_sensors];
> >> + int temp, ret;
> >> +
> >> + ret = regmap_field_read(priv->rf[TSENS_0C_STATUS], &temp);


> >> + if (ret)
> >> + return ret;
> >> +
> >> + dev_dbg(priv->dev, "[%u] %s: 0c interrupt is %s\n",
> >> + s->hw_id, __func__, temp ? "triggered" : "cleared");
> >
> > So triggered is printed for non-zero (including negative) values?
> This zeroc hardware generates each interrupt when any of the TSENS that
> it monitors goes below 5C or above 10c. These thresholds are not
> configurable. Hence we don't expect this to be changed from kernel side.
> So this sensor (status register) will read 0 or 1. 1 means soc
> temperature is in cold condition and 0 means it is in normal
> temperature.

All this information belongs in the function description and the part
about the status register returning 0 (for temp > 10) and 1 (for temp
<=5) belongs in the patch description too. Please add it to v3.

What happens at 7 degrees? Will the HW continue returning 1 due to
some hysteresis?

I'll review v2 in a bit.

> >
> >> +
> >> + thermal_zone_device_update(s->tzd, THERMAL_EVENT_UNSPECIFIED);
> >> +
> >> + return IRQ_HANDLED;
> >> +}
> >> +
> >> /**
> >> * tsens_critical_irq_thread() - Threaded handler for critical
> >> interrupts
> >> * @irq: irq number
> >> @@ -566,6 +598,20 @@ void tsens_disable_irq(struct tsens_priv *priv)
> >> regmap_field_write(priv->rf[INT_EN], 0);
> >> }
> >>
> >> +int tsens_get_0c_int_status(const struct tsens_sensor *s, int *temp)
> >> +{
> >> + struct tsens_priv *priv = s->priv;
> >> + int last_temp = 0, ret;
> >> +
> >> + ret = regmap_field_read(priv->rf[TSENS_0C_STATUS],
> >> &last_temp);
> >> + if (ret)
> >> + return ret;
> >> +
> >> + *temp = last_temp;
> >> +
> >> + return 0;
> >> +}
> >> +
> >> int get_temp_tsens_valid(const struct tsens_sensor *s, int *temp)
> >> {
> >> struct tsens_priv *priv = s->priv;
> >> @@ -833,6 +879,30 @@ int __init init_common(struct tsens_priv *priv)
> >> regmap_field_write(priv->rf[CC_MON_MASK], 1);
> >> }
> >>
> >> + if (tsens_version(priv) > VER_1_X && ver_minor > 5) {
> >> + /* 0C interrupt is present only on v2.6+ */
> >> + priv->rf[TSENS_0C_INT_EN] =
> >> devm_regmap_field_alloc(dev,
> >> + priv->srot_map,
> >> +
> >> priv->fields[TSENS_0C_INT_EN]);
> >> + if (IS_ERR(priv->rf[TSENS_0C_INT_EN])) {
> >> + ret = PTR_ERR(priv->rf[TSENS_0C_INT_EN]);
> >> + goto err_put_device;
> >> + }
> >> +
> >> + /* Check whether 0C interrupt is enabled or not */
> >> + regmap_field_read(priv->rf[TSENS_0C_INT_EN],
> >> &enabled);
> >> + if (enabled) {
> >> + priv->feat->zero_c_int = 1;
> >
> > This should be done at the beginning of the block where you check our
> > version is > 2.6 since the flag only says whether the feature is
> > present.
> Done
> >
> >> + priv->rf[TSENS_0C_STATUS] =
> >> devm_regmap_field_alloc(dev,
> >> + priv->tm_map,
> >> +
> >> priv->fields[TSENS_0C_STATUS]);
> >> + if (IS_ERR(priv->rf[TSENS_0C_STATUS])) {
> >> + ret =
> >> PTR_ERR(priv->rf[TSENS_0C_STATUS]);
> >> + goto err_put_device;
> >> + }
> >> + }
> >> + }
> >> +
> >> spin_lock_init(&priv->ul_lock);
> >> tsens_enable_irq(priv);
> >> tsens_debug_init(op);
> >> diff --git a/drivers/thermal/qcom/tsens-v2.c
> >> b/drivers/thermal/qcom/tsens-v2.c
> >> index b293ed32174b..ce80d82c7255 100644
> >> --- a/drivers/thermal/qcom/tsens-v2.c
> >> +++ b/drivers/thermal/qcom/tsens-v2.c
> >> @@ -11,6 +11,7 @@
> >> /* ----- SROT ------ */
> >> #define SROT_HW_VER_OFF 0x0000
> >> #define SROT_CTRL_OFF 0x0004
> >> +#define SROT_OC_CTRL_OFF 0x0018
> >>
> >> /* ----- TM ------ */
> >> #define TM_INT_EN_OFF 0x0004
> >> @@ -23,6 +24,7 @@
> >> #define TM_Sn_UPPER_LOWER_THRESHOLD_OFF 0x0020
> >> #define TM_Sn_CRITICAL_THRESHOLD_OFF 0x0060
> >> #define TM_Sn_STATUS_OFF 0x00a0
> >> +#define TM_0C_INT_STATUS_OFF 0x00e0
> >> #define TM_TRDY_OFF 0x00e4
> >> #define TM_WDOG_LOG_OFF 0x013c
> >>
> >> @@ -45,6 +47,7 @@ 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),
> >> + [TSENS_0C_INT_EN] = REG_FIELD(SROT_OC_CTRL_OFF, 0, 0),
> >>
> >> /* ----- TM ------ */
> >> /* INTERRUPT ENABLE */
> >> @@ -86,6 +89,9 @@ static const struct reg_field
> >> tsens_v2_regfields[MAX_REGFIELDS] = {
> >> REG_FIELD_FOR_EACH_SENSOR16(CRITICAL_STATUS, TM_Sn_STATUS_OFF,
> >> 19, 19),
> >> REG_FIELD_FOR_EACH_SENSOR16(MAX_STATUS, TM_Sn_STATUS_OFF,
> >> 20, 20),
> >>
> >> + /* 0C INETRRUPT STATUS */
> >
> > Typo: Interrupt
> >
> >> + [TSENS_0C_STATUS] = REG_FIELD(TM_0C_INT_STATUS_OFF, 0, 0),
> >> +
> >> /* TRDY: 1=ready, 0=in progress */
> >> [TRDY] = REG_FIELD(TM_TRDY_OFF, 0, 0),
> >> };
> >> @@ -93,6 +99,7 @@ static const struct reg_field
> >> tsens_v2_regfields[MAX_REGFIELDS] = {
> >> static const struct tsens_ops ops_generic_v2 = {
> >> .init = init_common,
> >> .get_temp = get_temp_tsens_valid,
> >> + .get_0c_status = tsens_get_0c_int_status,
> >> };
> >>
> >> struct tsens_plat_data data_tsens_v2 = {
> >> diff --git a/drivers/thermal/qcom/tsens.c
> >> b/drivers/thermal/qcom/tsens.c
> >> index 2f77d235cf73..e60870c53383 100644
> >> --- a/drivers/thermal/qcom/tsens.c
> >> +++ b/drivers/thermal/qcom/tsens.c
> >> @@ -14,6 +14,17 @@
> >> #include <linux/thermal.h>
> >> #include "tsens.h"
> >>
> >> +static int tsens_0c_get_temp(void *data, int *temp)
> >> +{
> >> + struct tsens_sensor *s = data;
> >> + struct tsens_priv *priv = s->priv;
> >> +
> >> + if (priv->ops->get_0c_status)
> >> + return priv->ops->get_0c_status(s, temp);
> >> +
> >> + return -ENOTSUPP;
> >> +}
> >> +
> >> static int tsens_get_temp(void *data, int *temp)
> >> {
> >> struct tsens_sensor *s = data;
> >> @@ -85,6 +96,10 @@ static const struct thermal_zone_of_device_ops
> >> tsens_of_ops = {
> >> .set_trips = tsens_set_trips,
> >> };
> >>
> >> +static const struct thermal_zone_of_device_ops tsens_0c_of_ops = {
> >> + .get_temp = tsens_0c_get_temp,
> >> +};
> >> +
> >> static int tsens_register_irq(struct tsens_priv *priv, char *irqname,
> >> irq_handler_t thread_fn)
> >> {
> >> @@ -142,6 +157,21 @@ static int tsens_register(struct tsens_priv
> >> *priv)
> >> ret = tsens_register_irq(priv, "critical",
> >> tsens_critical_irq_thread);
> >>
> >> + if (priv->feat->zero_c_int) {
> >> + priv->sensor[priv->num_sensors].priv = priv;
> >> + tzd = devm_thermal_zone_of_sensor_register(priv->dev,
> >> +
> >> priv->sensor[priv->num_sensors].hw_id,
> >> +
> >> &priv->sensor[priv->num_sensors],
> >> + &tsens_0c_of_ops);
> >> + if (IS_ERR(tzd)) {
> >> + ret = 0;
> >> + return ret;
> >> + }
> >> +
> >> + priv->sensor[priv->num_sensors].tzd = tzd;
> >
> > Why can't this happen in the previous loop, but increase the loop to
> > <= num_sensors? It is duplicated code.
> I think if i change default loop logic to <= num_sensors, it will break
> other legacy targets, right ?
> My idea is to guard all changes related to zeroc under zeroc related
> feature flag.
> Again, since we cannot configure any threshold from kernel side, there
> is no set_trip ops for this sensor, so we need to call register function
> differently in compared to regular sensor
> >
> >> + ret = tsens_register_irq(priv, "zeroc",
> >> tsens_0c_irq_thread);
> >> + }
> >> +
> >> return ret;
> >> }
> >>
> >> @@ -178,11 +208,22 @@ static int tsens_probe(struct platform_device
> >> *pdev)
> >> return -EINVAL;
> >> }
> >>
> >> - priv = devm_kzalloc(dev,
> >> - struct_size(priv, sensor, num_sensors),
> >> - GFP_KERNEL);
> >> - if (!priv)
> >> - return -ENOMEM;
> >> + /* Check for 0c interrupt is enabled or not */
> >> + if (platform_get_irq_byname(pdev, "zeroc") > 0) {
> >> + priv = devm_kzalloc(dev,
> >> + struct_size(priv, sensor, num_sensors
> >> + 1),
> >> + GFP_KERNEL);
> >
> > Instead of doing this, simply do the following,
> >
> > if (platform_get_irq_byname(pdev, "zeroc") > 0) {
> > num_sensors++;
> >
> > The kzalloc will just work then, no?
> I just changed logic in v2. Basically this zeroc feature is an
> optional. There is a chance that we don't need to enable in software
> even though hardware support is present.
> So I used another variable to check whether feature is enabled or not by
> checking DT interrupt configuration.

I've looked briefly at v2 and I don't like the way we play around with
num_sensors by special casing zeroc_en to allocate extra memory and
then revert it. It feels very convoluted. I'll address the rest of the
comments on v2.

> >
> >> + if (!priv)
> >> + return -ENOMEM;
> >> + /* Use Max sensor index as 0c sensor hw_id */
> >> + priv->sensor[num_sensors].hw_id =
> >> data->feat->max_sensors;
> >> + } else {
> >> + priv = devm_kzalloc(dev,
> >> + struct_size(priv, sensor,
> >> num_sensors),
> >> + GFP_KERNEL);
> >> + if (!priv)
> >> + return -ENOMEM;
> >> + }
> >>
> >> priv->dev = dev;
> >> priv->num_sensors = num_sensors;
> >> diff --git a/drivers/thermal/qcom/tsens.h
> >> b/drivers/thermal/qcom/tsens.h
> >> index 502acf0e6828..5b53a0352b4d 100644
> >> --- a/drivers/thermal/qcom/tsens.h
> >> +++ b/drivers/thermal/qcom/tsens.h
> >> @@ -34,6 +34,7 @@ enum tsens_irq_type {
> >> LOWER,
> >> UPPER,
> >> CRITICAL,
> >> + ZEROC,
> >> };
> >>
> >> /**
> >> @@ -64,6 +65,7 @@ struct tsens_sensor {
> >> * @suspend: Function to suspend the tsens device
> >> * @resume: Function to resume the tsens device
> >> * @get_trend: Function to get the thermal/temp trend
> >> + * @get_0c_status: Function to get the 0c interrupt status
> >> */
> >> struct tsens_ops {
> >> /* mandatory callbacks */
> >> @@ -76,6 +78,7 @@ struct tsens_ops {
> >> int (*suspend)(struct tsens_priv *priv);
> >> int (*resume)(struct tsens_priv *priv);
> >> int (*get_trend)(struct tsens_sensor *s, enum thermal_trend
> >> *trend);
> >> + int (*get_0c_status)(const struct tsens_sensor *s, int *temp);
> >> };
> >>
> >> #define REG_FIELD_FOR_EACH_SENSOR11(_name, _offset, _startbit,
> >> _stopbit) \
> >> @@ -161,6 +164,8 @@ enum regfield_ids {
> >> TSENS_SW_RST,
> >> SENSOR_EN,
> >> CODE_OR_TEMP,
> >> + /* 0C CTRL OFFSET */
> >> + TSENS_0C_INT_EN,
> >>
> >> /* ----- TM ------ */
> >> /* TRDY */
> >> @@ -485,6 +490,8 @@ enum regfield_ids {
> >> MAX_STATUS_14,
> >> MAX_STATUS_15,
> >>
> >> + TSENS_0C_STATUS, /* 0C INTERRUPT status */
> >> +
> >> /* Keep last */
> >> MAX_REGFIELDS
> >> };
> >> @@ -497,6 +504,7 @@ enum regfield_ids {
> >> * @srot_split: does the IP neatly splits the register space into
> >> SROT and TM,
> >> * with SROT only being available to secure boot
> >> firmware?
> >> * @has_watchdog: does this IP support watchdog functionality?
> >> + * @zero_c_int: does this IP support 0C interrupt ?
> >> * @max_sensors: maximum sensors supported by this version of the IP
> >> */
> >> struct tsens_features {
> >> @@ -505,6 +513,7 @@ struct tsens_features {
> >> unsigned int adc:1;
> >> unsigned int srot_split:1;
> >> unsigned int has_watchdog:1;
> >> + unsigned int zero_c_int:1;
> >
> > zeroc_interrupt
> Done
> >
> >> unsigned int max_sensors;
> >> };
> >>
> >> @@ -580,11 +589,13 @@ void compute_intercept_slope(struct tsens_priv
> >> *priv, u32 *pt1, u32 *pt2, u32 mo
> >> int init_common(struct tsens_priv *priv);
> >> int get_temp_tsens_valid(const struct tsens_sensor *s, int *temp);
> >> int get_temp_common(const struct tsens_sensor *s, int *temp);
> >> +int tsens_get_0c_int_status(const struct tsens_sensor *s, int *temp);
> >> int tsens_enable_irq(struct tsens_priv *priv);
> >> void tsens_disable_irq(struct tsens_priv *priv);
> >> int tsens_set_trips(void *_sensor, int low, int high);
> >> irqreturn_t tsens_irq_thread(int irq, void *data);
> >> irqreturn_t tsens_critical_irq_thread(int irq, void *data);
> >> +irqreturn_t tsens_0c_irq_thread(int irq, void *data);
> >>
> >> /* TSENS target */
> >> extern struct tsens_plat_data data_8960;
> >> --
> >> 2.26.2