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
