lkml.org 
[lkml]   [2022]   [Jul]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH 3/3] thermal: qcom: tsens: Implement re-initialization workaround quirk
From
Hi Bjorn,

Thanks for your review.

On 7/19/22 9:00 AM, Bjorn Andersson wrote:
> On Fri 01 Jul 09:58 CDT 2022, Bhupesh Sharma wrote:
>
>> Since for some QCoM tsens controllers, its suggested to
>> monitor the controller health periodically and in case an
>> issue is detected, to re-initialize the tsens controller
>> via trustzone, add the support for the same in the
>> qcom tsens driver.
>>
>> Note that Once the tsens controller is reset using scm call,
>> all SROT and TM region registers will enter the reset mode.
>>
>> While all the SROT registers will be re-programmed and
>> re-enabled in trustzone prior to the scm call exit, the TM
>> region registers will not re-initialized in trustzone and thus
>> need to be handled by the tsens driver.
>>
>> Cc: Amit Kucheria <amitk@kernel.org>
>> Cc: Thara Gopinath <thara.gopinath@gmail.com>
>> Cc: linux-pm@vger.kernel.org
>> Cc: linux-arm-msm@vger.kernel.org
>> Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org>
>> ---
>> drivers/thermal/qcom/tsens-v2.c | 3 +
>> drivers/thermal/qcom/tsens.c | 237 +++++++++++++++++++++++++++++++-
>> drivers/thermal/qcom/tsens.h | 6 +
>> 3 files changed, 239 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/thermal/qcom/tsens-v2.c b/drivers/thermal/qcom/tsens-v2.c
>> index 61d38a56d29a..9bb542f16482 100644
>> --- a/drivers/thermal/qcom/tsens-v2.c
>> +++ b/drivers/thermal/qcom/tsens-v2.c
>> @@ -88,6 +88,9 @@ static const struct reg_field tsens_v2_regfields[MAX_REGFIELDS] = {
>>
>> /* TRDY: 1=ready, 0=in progress */
>> [TRDY] = REG_FIELD(TM_TRDY_OFF, 0, 0),
>> +
>> + /* FIRST_ROUND_COMPLETE: 1=complete, 0=not complete */
>> + [FIRST_ROUND_COMPLETE] = REG_FIELD(TM_TRDY_OFF, 3, 3),
>> };
>>
>> static const struct tsens_ops ops_generic_v2 = {
>> diff --git a/drivers/thermal/qcom/tsens.c b/drivers/thermal/qcom/tsens.c
>> index 97f4d4454f20..28d42ae0eb47 100644
>> --- a/drivers/thermal/qcom/tsens.c
>> +++ b/drivers/thermal/qcom/tsens.c
>> @@ -7,6 +7,7 @@
>> #include <linux/debugfs.h>
>> #include <linux/err.h>
>> #include <linux/io.h>
>> +#include <linux/qcom_scm.h>
>> #include <linux/module.h>
>> #include <linux/nvmem-consumer.h>
>> #include <linux/of.h>
>> @@ -21,6 +22,8 @@
>> #include "../thermal_hwmon.h"
>> #include "tsens.h"
>>
>> +LIST_HEAD(tsens_device_list);
>> +
>> /**
>> * struct tsens_irq_data - IRQ status and temperature violations
>> * @up_viol: upper threshold violated
>> @@ -594,19 +597,159 @@ static void tsens_disable_irq(struct tsens_priv *priv)
>> regmap_field_write(priv->rf[INT_EN], 0);
>> }
>>
>> +static int tsens_reenable_hw_after_scm(struct tsens_priv *priv)
>> +{
>> + unsigned long flags;
>> +
>> + spin_lock_irqsave(&priv->ul_lock, flags);
>> +
>> + /* Re-enable watchdog, unmask the bark and
>> + * disable cycle completion monitoring.
>> + */
>> + regmap_field_write(priv->rf[WDOG_BARK_CLEAR], 1);
>> + regmap_field_write(priv->rf[WDOG_BARK_CLEAR], 0);
>> + regmap_field_write(priv->rf[WDOG_BARK_MASK], 0);
>> + regmap_field_write(priv->rf[CC_MON_MASK], 1);
>> +
>> + /* Re-enable interrupts */
>> + tsens_enable_irq(priv);
>> +
>> + spin_unlock_irqrestore(&priv->ul_lock, flags);
>> +
>> + return 0;
>> +}
>> +
>> int get_temp_tsens_valid(const struct tsens_sensor *s, int *temp)
>> {
>> - struct tsens_priv *priv = s->priv;
>> + struct tsens_priv *priv = s->priv, *priv_reinit;
>> int hw_id = s->hw_id;
>> u32 temp_idx = LAST_TEMP_0 + hw_id;
>> u32 valid_idx = VALID_0 + hw_id;
>> u32 valid;
>> - int ret;
>> + int ret, trdy, first_round, tsens_ret, sw_reg;
>> + unsigned long timeout;
>> + static atomic_t in_tsens_reinit;
>
> This is a global state, I suggest you move it to the top of the file to
> make that obvious.

Sure.

>> /* VER_0 doesn't have VALID bit */
>> if (tsens_version(priv) == VER_0)
>> goto get_temp;
>>
>> + /* For some tsens controllers, its suggested to
>> + * monitor the controller health periodically
>> + * and in case an issue is detected to reinit
>> + * tsens controller via trustzone.
>> + */
>> + if (priv->needs_reinit_wa) {
>
> I would suggest that you move all this entire block to a separate
> function, maybe something:
>
> int tsens_health_check_and_reinit()

Ok. Will fix in v2.

>> + /* First check if TRDY is SET */
>> + timeout = jiffies + usecs_to_jiffies(TIMEOUT_US);
>> + do {
>> + ret = regmap_field_read(priv->rf[TRDY], &trdy);
>> + if (ret)
>> + goto err;
>> + if (!trdy)
>> + continue;
>> + } while (time_before(jiffies, timeout));
>
> This looks like a regmap_field_read()

Not sure, I completely understand this comment. Can you please elaborate?

>> +
>> + if (!trdy) {
>> + ret = regmap_field_read(priv->rf[FIRST_ROUND_COMPLETE], &first_round);
>> + if (ret)
>> + goto err;
>> +
>> + if (!first_round) {
>> + if (atomic_read(&in_tsens_reinit)) {
>> + dev_dbg(priv->dev, "tsens re-init is in progress\n");
>> + ret = -EAGAIN;
>
> Is it preferred to return -EAGAIN here, over just serializing this whole
> thing using a mutex?

Right, using a mutex to serialize here makes sense. Will fix in v2.

>> + goto err;
>> + }
>> +
>> + /* Wait for 2 ms for tsens controller to recover */
>> + timeout = jiffies + msecs_to_jiffies(RESET_TIMEOUT_MS);
>> + do {
>> + ret = regmap_field_read(priv->rf[FIRST_ROUND_COMPLETE],
>> + &first_round);
>> + if (ret)
>> + goto err;
>> +
>> + if (first_round) {
>> + dev_dbg(priv->dev, "tsens controller recovered\n");
>> + goto sensor_read;
>> + }
>> + } while (time_before(jiffies, timeout));
>> +
>> + /*
>> + * tsens controller did not recover,
>> + * proceed with SCM call to re-init it
>> + */
>> + if (atomic_read(&in_tsens_reinit)) {
>> + dev_dbg(priv->dev, "tsens re-init is in progress\n");
>> + ret = -EAGAIN;
>> + goto err;
>> + }
>> +
>> + atomic_set(&in_tsens_reinit, 1);
>
> Afaict nothing prevents two different processes to run the remainder of
> the recovery in parallel. I think you need some locking here.

Ack.

>> +
>> + /*
>> + * Invoke scm call only if SW register write is
>> + * reflecting in controller. Try it for 2 ms.
>> + */
>> + timeout = jiffies + msecs_to_jiffies(RESET_TIMEOUT_MS);
>> + do {
>> + ret = regmap_field_write(priv->rf[INT_EN], BIT(2));
>
> Do we know what BIT(2) is and would we be allowed to give it a define?

Sure, I will add a define here.

>> + if (ret)
>> + goto err_unset;
>> +
>> + ret = regmap_field_read(priv->rf[INT_EN], &sw_reg);
>> + if (ret)
>> + goto err_unset;
>> +
>> + if (!(sw_reg & BIT(2)))
>> + continue;
>
> Why not:
>
> } while (sw_reg & BIT(2) && time_before(jiffies, timeout));

Sure.

>> + } while (time_before(jiffies, timeout));
>> +
>> + if (!(sw_reg & BIT(2))) {
>> + ret = -ENOTRECOVERABLE;
>> + goto err_unset;
>> + }
>> +
>> + ret = qcom_scm_tsens_reinit(&tsens_ret);
>> + if (ret || tsens_ret) {
>> + dev_err(priv->dev, "tsens reinit scm call failed (%d : %d)\n",
>> + ret, tsens_ret);
>> + if (tsens_ret)
>> + ret = -ENOTRECOVERABLE;
>
> If that's the api for the SCM, feel free to move the -ENOTRECOVERABLE to
> the scm function.

Ok, let me check and fix this in v2.

>> +
>> + goto err_unset;
>> + }
>> +
>> + /* After the SCM call, we need to re-enable
>> + * the interrupts and also set active threshold
>> + * for each sensor.
>> + */
>> + list_for_each_entry(priv_reinit,
>> + &tsens_device_list, list) {
>> + ret = tsens_reenable_hw_after_scm(priv_reinit);
>> + if (ret) {
>> + dev_err(priv->dev,
>> + "tsens re-enable after scm call failed (%d)\n",
>> + ret);
>> + ret = -ENOTRECOVERABLE;
>> + goto err_unset;
>> + }
>> + }
>> +
>> + atomic_set(&in_tsens_reinit, 0);
>> +
>> + /* Notify reinit wa worker */
>> + list_for_each_entry(priv_reinit,
>
> Do you need to loop twice over the tsens instances?
>
>> + &tsens_device_list, list) {
>> + queue_work(priv_reinit->reinit_wa_worker,
>> + &priv_reinit->reinit_wa_notify);
>> + }
>> + }
>> + }
>> + }
>> +
>> +sensor_read:
>> /* Valid bit is 0 for 6 AHB clock cycles.
>> * At 19.2MHz, 1 AHB clock is ~60ns.
>> * We should enter this loop very, very rarely.
>> @@ -623,6 +766,12 @@ int get_temp_tsens_valid(const struct tsens_sensor *s, int *temp)
>> *temp = tsens_hw_to_mC(s, temp_idx);
>>
>> return 0;
>> +
>> +err_unset:
>> + atomic_set(&in_tsens_reinit, 0);
>> +
>> +err:
>> + return ret;
>> }
>>
>> int get_temp_common(const struct tsens_sensor *s, int *temp)
>> @@ -860,6 +1009,14 @@ int __init init_common(struct tsens_priv *priv)
>> goto err_put_device;
>> }
>>
>> + priv->rf[FIRST_ROUND_COMPLETE] = devm_regmap_field_alloc(dev,
>> + priv->tm_map,
>> + priv->fields[FIRST_ROUND_COMPLETE]);
>> + if (IS_ERR(priv->rf[FIRST_ROUND_COMPLETE])) {
>> + ret = PTR_ERR(priv->rf[FIRST_ROUND_COMPLETE]);
>> + goto err_put_device;
>> + }
>> +
>> /* 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++) {
>> @@ -1097,6 +1254,43 @@ static int tsens_register(struct tsens_priv *priv)
>> return ret;
>> }
>>
>> +static void tsens_reinit_worker_notify(struct work_struct *work)
>> +{
>> + int i, ret, temp;
>
> priv->num_sensors is unsigned, so i could be too.

Ok.

>> + struct tsens_irq_data d;
>> + struct tsens_priv *priv = container_of(work, struct tsens_priv,
>> + reinit_wa_notify);
>> +
>> + for (i = 0; i < priv->num_sensors; i++) {
>> + const struct tsens_sensor *s = &priv->sensor[i];
>> + u32 hw_id = s->hw_id;
>> +
>> + if (!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__);
>
> Please express yourself in the message, instead of using __func__.

This was a reuse from the existing tsens irq handler code, but I agree.
Let me fix it in v2.

>> + continue;
>> + }
>> +
>> + tsens_read_irq_state(priv, hw_id, s, &d);
>> +
>> + if ((d.up_thresh < temp) || (d.low_thresh > temp)) {
>> + dev_dbg(priv->dev, "[%u] %s: TZ update trigger (%d mC)\n",
>> + hw_id, __func__, temp);
>> + thermal_zone_device_update(s->tzd,
>> + THERMAL_EVENT_UNSPECIFIED);
>
> This is just 86 chars long, no need to wrap the line.

Sure.

>> + } else {
>> + dev_dbg(priv->dev, "[%u] %s: no violation: %d\n",
>
> Double space after ':'

Again this is a reuse from the existing tsens irq handler code, but I
agree. Let me fix it in v2.

>> + hw_id, __func__, temp);
>> + }
>> + }
>> +}
>> +
>> static int tsens_probe(struct platform_device *pdev)
>> {
>> int ret, i;
>> @@ -1139,6 +1333,19 @@ static int tsens_probe(struct platform_device *pdev)
>> priv->dev = dev;
>> priv->num_sensors = num_sensors;
>> priv->needs_reinit_wa = data->needs_reinit_wa;
>> +
>> + if (priv->needs_reinit_wa && !qcom_scm_is_available())
>> + return -EPROBE_DEFER;
>> +
>> + if (priv->needs_reinit_wa) {
>> + priv->reinit_wa_worker = alloc_workqueue("tsens_reinit_work",
>> + WQ_HIGHPRI, 0);
>
> Do you really need your own work queue for this, how about just
> scheduling the work on system_highpri_wq?

Ok, let me use 'system_highpri_wq' in v2.

Regards,
Bhupesh

\
 
 \ /
  Last update: 2022-07-20 10:28    [W:0.125 / U:0.032 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site