| Message ID | cover.1577976221.git.amit.kucheria@linaro.org |
|---|---|
| Headers | show |
| Series | thermal: tsens: Handle critical interrupts | expand |
On Thu 02 Jan 06:54 PST 2020, Amit Kucheria wrote: > We already dereference the sensor and save it into a variable. Use the > variable directly to make the code easier to read. > Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org> > Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org> > --- > drivers/thermal/qcom/tsens-common.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/thermal/qcom/tsens-common.c b/drivers/thermal/qcom/tsens-common.c > index c2df30a08b9e..1cbc5a6e5b4f 100644 > --- a/drivers/thermal/qcom/tsens-common.c > +++ b/drivers/thermal/qcom/tsens-common.c > @@ -368,7 +368,7 @@ irqreturn_t tsens_irq_thread(int irq, void *data) > tsens_set_interrupt(priv, hw_id, UPPER, disable); > if (d.up_thresh > temp) { > dev_dbg(priv->dev, "[%u] %s: re-arm upper\n", > - priv->sensor[i].hw_id, __func__); > + hw_id, __func__); > tsens_set_interrupt(priv, hw_id, UPPER, enable); > } else { > trigger = true; > @@ -379,7 +379,7 @@ irqreturn_t tsens_irq_thread(int irq, void *data) > tsens_set_interrupt(priv, hw_id, LOWER, disable); > if (d.low_thresh < temp) { > dev_dbg(priv->dev, "[%u] %s: re-arm low\n", > - priv->sensor[i].hw_id, __func__); > + hw_id, __func__); > tsens_set_interrupt(priv, hw_id, LOWER, enable); > } else { > trigger = true; > @@ -392,7 +392,7 @@ irqreturn_t tsens_irq_thread(int irq, void *data) > if (trigger) { > dev_dbg(priv->dev, "[%u] %s: TZ update trigger (%d mC)\n", > hw_id, __func__, temp); > - thermal_zone_device_update(priv->sensor[i].tzd, > + thermal_zone_device_update(s->tzd, > THERMAL_EVENT_UNSPECIFIED); > } else { > dev_dbg(priv->dev, "[%u] %s: no violation: %d\n", > @@ -435,7 +435,7 @@ int tsens_set_trips(void *_sensor, int low, int high) > spin_unlock_irqrestore(&priv->ul_lock, flags); > > dev_dbg(dev, "[%u] %s: (%d:%d)->(%d:%d)\n", > - s->hw_id, __func__, d.low_thresh, d.up_thresh, cl_low, cl_high); > + hw_id, __func__, d.low_thresh, d.up_thresh, cl_low, cl_high); > > return 0; > } > -- > 2.20.1 >
On Thu 02 Jan 06:54 PST 2020, Amit Kucheria wrote: [..] > @@ -189,6 +197,9 @@ static void tsens_set_interrupt_v1(struct tsens_priv *priv, u32 hw_id, > case LOWER: > index = LOW_INT_CLEAR_0 + hw_id; > break; > + case CRITICAL: > + /* No critical interrupts before v2 */ > + break; You need to break harder, right now you're just attempting to write "enable" to VER_MAJOR in this case. > } > regmap_field_write(priv->rf[index], enable ? 0 : 1); > } [..] > @@ -321,6 +357,64 @@ static inline u32 masked_irq(u32 hw_id, u32 mask, enum tsens_ver ver) > return 0; > } > > +/** > + * tsens_critical_irq_thread - Threaded interrupt handler for critical interrupts () on the function name to denote it being a function. > + * @irq: irq number > + * @data: tsens controller private data > + * > + * Check all sensors to find ones that violated their critical threshold limits. > + * Clear and then re-enable the interrupt. > + * > + * The level-triggered interrupt might deassert if the temperature returned to > + * within the threshold limits by the time the handler got scheduled. We > + * consider the irq to have been handled in that case. > + * > + * Return: IRQ_HANDLED > + */ > +irqreturn_t tsens_critical_irq_thread(int irq, void *data) > +{ > + struct tsens_priv *priv = data; > + struct tsens_irq_data d; > + unsigned long flags; > + int temp, ret, i; > + > + for (i = 0; i < priv->num_sensors; i++) { > + const struct tsens_sensor *s = &priv->sensor[i]; > + u32 hw_id = s->hw_id; > + > + if (IS_ERR(s->tzd)) > + continue; > + if (!tsens_threshold_violated(priv, hw_id, &d)) > + continue; > + ret = get_temp_tsens_valid(s, &temp); > + if (ret) { > + dev_err(priv->dev, "[%u] %s: error reading sensor\n", hw_id, __func__); > + continue; > + } > + > + spin_lock_irqsave(&priv->ul_lock, flags); You meant crit_lock here? But perhaps more importantly, why do you need a lock here? > + > + tsens_read_irq_state(priv, hw_id, s, &d); > + > + if (d.crit_viol && > + !masked_irq(hw_id, d.crit_irq_mask, tsens_version(priv))) { > + tsens_set_interrupt(priv, hw_id, CRITICAL, false); > + if (d.crit_thresh > temp) { > + dev_dbg(priv->dev, "[%u] %s: re-arm upper\n", > + hw_id, __func__); > + } else { > + dev_dbg(priv->dev, "[%u] %s: TZ update trigger (%d mC)\n", > + hw_id, __func__, temp); > + } > + tsens_set_interrupt(priv, hw_id, CRITICAL, true); > + } > + > + spin_unlock_irqrestore(&priv->crit_lock, flags); > + } > + > + return IRQ_HANDLED; > +} [..] > @@ -125,6 +125,28 @@ static int tsens_register(struct tsens_priv *priv) > goto err_put_device; > } > > + if (priv->feat->crit_int) { > + irq_crit = platform_get_irq_byname(pdev, "critical"); > + if (irq_crit < 0) { > + ret = irq_crit; > + /* For old DTs with no IRQ defined */ > + if (irq_crit == -ENXIO) > + ret = 0; > + goto err_crit_int; > + } > + ret = devm_request_threaded_irq(&pdev->dev, irq_crit, > + NULL, tsens_critical_irq_thread, > + IRQF_TRIGGER_HIGH | IRQF_ONESHOT, You should omit the IRQF_TRIGGER_HIGH here, it will be provided by the system configuration (DT). > + dev_name(&pdev->dev), priv); > + if (ret) { > + dev_err(&pdev->dev, "%s: failed to get critical irq\n", __func__); > + goto err_crit_int; > + } > + > + enable_irq_wake(irq_crit); > + } > + > +err_crit_int: > enable_irq_wake(irq); > > err_put_device: > diff --git a/drivers/thermal/qcom/tsens.h b/drivers/thermal/qcom/tsens.h [..] > @@ -460,6 +526,8 @@ struct tsens_context { > * @srot_map: pointer to SROT register address space > * @tm_offset: deal with old device trees that don't address TM and SROT > * address space separately > + * @ul_lock: lock while processing upper/lower threshold interrupts This looks like an unrelated fixup to a previous patch? Please keep it separate. > + * @crit_lock: lock while processing critical threshold interrupts > * @rf: array of regmap_fields used to store value of the field > * @ctx: registers to be saved and restored during suspend/resume > * @feat: features of the IP > @@ -479,6 +547,9 @@ struct tsens_priv { > /* lock for upper/lower threshold interrupts */ > spinlock_t ul_lock; > > + /* lock for critical threshold interrupts */ > + spinlock_t crit_lock; You're lacking a spin_lock_init() of this. > + > struct regmap_field *rf[MAX_REGFIELDS]; > struct tsens_context ctx; > struct tsens_features *feat; > @@ -500,6 +571,7 @@ 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); I think you should squash tsens.c and tsens-common.c into one file, so you don't need to keep adding these extern declarations for every function - separate of this series of course. Regards, Bjorn > > /* TSENS target */ > extern struct tsens_plat_data data_8960; > -- > 2.20.1 >
On Fri, Jan 3, 2020 at 1:15 AM Bjorn Andersson <bjorn.andersson@linaro.org> wrote: > > On Thu 02 Jan 06:54 PST 2020, Amit Kucheria wrote: > [..] > > @@ -189,6 +197,9 @@ static void tsens_set_interrupt_v1(struct tsens_priv *priv, u32 hw_id, > > case LOWER: > > index = LOW_INT_CLEAR_0 + hw_id; > > break; > > + case CRITICAL: > > + /* No critical interrupts before v2 */ > > + break; > > You need to break harder, right now you're just attempting to write > "enable" to VER_MAJOR in this case. Will fix. > > > } > > regmap_field_write(priv->rf[index], enable ? 0 : 1); > > } > [..] > > @@ -321,6 +357,64 @@ static inline u32 masked_irq(u32 hw_id, u32 mask, enum tsens_ver ver) > > return 0; > > } > > > > +/** > > + * tsens_critical_irq_thread - Threaded interrupt handler for critical interrupts > > () on the function name to denote it being a function. Will fix. > > > + * @irq: irq number > > + * @data: tsens controller private data > > + * > > + * Check all sensors to find ones that violated their critical threshold limits. > > + * Clear and then re-enable the interrupt. > > + * > > + * The level-triggered interrupt might deassert if the temperature returned to > > + * within the threshold limits by the time the handler got scheduled. We > > + * consider the irq to have been handled in that case. > > + * > > + * Return: IRQ_HANDLED > > + */ > > +irqreturn_t tsens_critical_irq_thread(int irq, void *data) > > +{ > > + struct tsens_priv *priv = data; > > + struct tsens_irq_data d; > > + unsigned long flags; > > + int temp, ret, i; > > + > > + for (i = 0; i < priv->num_sensors; i++) { > > + const struct tsens_sensor *s = &priv->sensor[i]; > > + u32 hw_id = s->hw_id; > > + > > + if (IS_ERR(s->tzd)) > > + continue; > > + if (!tsens_threshold_violated(priv, hw_id, &d)) > > + continue; > > + ret = get_temp_tsens_valid(s, &temp); > > + if (ret) { > > + dev_err(priv->dev, "[%u] %s: error reading sensor\n", hw_id, __func__); > > + continue; > > + } > > + > > + spin_lock_irqsave(&priv->ul_lock, flags); > > You meant crit_lock here? Good catch, will fix. > > But perhaps more importantly, why do you need a lock here? I'm reading and changing interrupt state registers in this section and there can be multiple interrupts occurring simultaneously. Without a lock, the interrupt threads could potentially stomp over each other's register state. Having said that, I think I found a potential problem in porting the downstream driver code. Basically, we only need critical interrupt to enable watchdog support. The critical interrupt HW line can be asserted by watchdog and by actual critical interrupts. One to one mapping of tsens critical interrupts to trip type CRITICAL in Linux leads to a HW shutdown. And we can use the trip type PASSIVE with multiple ranges of temperatures to handle several levels of trip. So I'll change the code below to mask the critical interrupts in the event it is triggered and only use the irq thread to handle watchdog interrupts. > > + > > + tsens_read_irq_state(priv, hw_id, s, &d); > > + > > + if (d.crit_viol && > > + !masked_irq(hw_id, d.crit_irq_mask, tsens_version(priv))) { > > + tsens_set_interrupt(priv, hw_id, CRITICAL, false); > > + if (d.crit_thresh > temp) { > > + dev_dbg(priv->dev, "[%u] %s: re-arm upper\n", > > + hw_id, __func__); > > + } else { > > + dev_dbg(priv->dev, "[%u] %s: TZ update trigger (%d mC)\n", > > + hw_id, __func__, temp); > > + } > > + tsens_set_interrupt(priv, hw_id, CRITICAL, true); > > + } > > + > > + spin_unlock_irqrestore(&priv->crit_lock, flags); > > + } > > + > > + return IRQ_HANDLED; > > +} > [..] > > @@ -125,6 +125,28 @@ static int tsens_register(struct tsens_priv *priv) > > goto err_put_device; > > } > > > > + if (priv->feat->crit_int) { > > + irq_crit = platform_get_irq_byname(pdev, "critical"); > > + if (irq_crit < 0) { > > + ret = irq_crit; > > + /* For old DTs with no IRQ defined */ > > + if (irq_crit == -ENXIO) > > + ret = 0; > > + goto err_crit_int; > > + } > > + ret = devm_request_threaded_irq(&pdev->dev, irq_crit, > > + NULL, tsens_critical_irq_thread, > > + IRQF_TRIGGER_HIGH | IRQF_ONESHOT, > > You should omit the IRQF_TRIGGER_HIGH here, it will be provided by the > system configuration (DT). Will fix. > > > + dev_name(&pdev->dev), priv); > > + if (ret) { > > + dev_err(&pdev->dev, "%s: failed to get critical irq\n", __func__); > > + goto err_crit_int; > > + } > > + > > + enable_irq_wake(irq_crit); > > + } > > + > > +err_crit_int: > > enable_irq_wake(irq); > > > > err_put_device: > > diff --git a/drivers/thermal/qcom/tsens.h b/drivers/thermal/qcom/tsens.h > [..] > > @@ -460,6 +526,8 @@ struct tsens_context { > > * @srot_map: pointer to SROT register address space > > * @tm_offset: deal with old device trees that don't address TM and SROT > > * address space separately > > + * @ul_lock: lock while processing upper/lower threshold interrupts > > This looks like an unrelated fixup to a previous patch? Please keep it > separate. Will remove. > > + * @crit_lock: lock while processing critical threshold interrupts > > * @rf: array of regmap_fields used to store value of the field > > * @ctx: registers to be saved and restored during suspend/resume > > * @feat: features of the IP > > @@ -479,6 +547,9 @@ struct tsens_priv { > > /* lock for upper/lower threshold interrupts */ > > spinlock_t ul_lock; > > > > + /* lock for critical threshold interrupts */ > > + spinlock_t crit_lock; > > You're lacking a spin_lock_init() of this. Will fix. > > + > > struct regmap_field *rf[MAX_REGFIELDS]; > > struct tsens_context ctx; > > struct tsens_features *feat; > > @@ -500,6 +571,7 @@ 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); > > I think you should squash tsens.c and tsens-common.c into one file, so > you don't need to keep adding these extern declarations for every > function - separate of this series of course. Agreed. The separation no longer makes sense. Thanks for the review.