On 02/04/2022 01:24, Bjorn Andersson wrote:
On Fri 01 Apr 00:14 PDT 2022, Vladimir Zapolskiy wrote:
It's noted that dcvs interrupts are not self-clearing, thus an interrupt
handler runs constantly, which leads to a severe regression in runtime.
To fix the problem an explicit write to clear interrupt register is
required.
Fixes: 275157b367f4 ("cpufreq: qcom-cpufreq-hw: Add dcvs interrupt support")
Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@xxxxxxxxxx>
---
Changes from v1 to v2:
* added a check for pending interrupt status before its handling,
thanks to Bjorn for review
drivers/cpufreq/qcom-cpufreq-hw.c | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
index f9d593ff4718..e17413a6f120 100644
--- a/drivers/cpufreq/qcom-cpufreq-hw.c
+++ b/drivers/cpufreq/qcom-cpufreq-hw.c
@@ -24,6 +24,8 @@
#define CLK_HW_DIV 2
#define LUT_TURBO_IND 1
+#define GT_IRQ_STATUS BIT(2)
+
#define HZ_PER_KHZ 1000
struct qcom_cpufreq_soc_data {
@@ -31,6 +33,8 @@ struct qcom_cpufreq_soc_data {
u32 reg_dcvs_ctrl;
u32 reg_freq_lut;
u32 reg_volt_lut;
+ u32 reg_intr_clr;
+ u32 reg_intr_status;
u32 reg_current_vote;
u32 reg_perf_state;
u8 lut_row_size;
@@ -345,11 +349,19 @@ static void qcom_lmh_dcvs_poll(struct work_struct *work)
static irqreturn_t qcom_lmh_dcvs_handle_irq(int irq, void *data)
{
struct qcom_cpufreq_data *c_data = data;
+ u32 val;
+
Following the discussion below (regarding reg_int_clr), the driver
should also check that soc_data->reg_intr_status is not 0.
+ val = readl_relaxed(c_data->base + c_data->soc_data->reg_intr_status);
Seems reasonable to read the INTR_STATUS register and bail early if
there's no interrupt.
+ if (!(val & GT_IRQ_STATUS))
+ return IRQ_HANDLED;
But if we in the interrupt handler realize that there's no interrupt
pending for us, shouldn't we return IRQ_NONE?
Initially I wanted to agree with Vladimir. However after giving a
thought, returning IRQ_HANDLED here can hide other status bits being set
(e.g. on the other platforms using EPSS). If we return IRQ_NONE here,
we'll get the "IRQ: nobody cared" message and will know that some bits
from status are unhandled.
However, a separate thing. We discussed this with Vladimir. I agree with
him that this chunk is not directly related to the fix for the issue.
I'd suggest to split this patch into two patches:
- writel_relaxed to clear the interrupt (which can be picked up into the
-rc branch and into stable kernels)
- a check for the GT_IRQ_STATUS which is not strictly necessary, so it
can come through the plain -next process.
/* Disable interrupt and enable polling */
disable_irq_nosync(c_data->throttle_irq);
schedule_delayed_work(&c_data->throttle_work, 0);
+ writel_relaxed(GT_IRQ_STATUS,
+ c_data->base + c_data->soc_data->reg_intr_clr);
And in OSM (i.e. not epss_soc_data), both reg_intr_status and
reg_intr_clr will be 0, so we end up reading and writing the wrong
register.
You need to do:
if (c_data->soc_data->reg_intr_clr)
writel_relaxed(..., reg_intr_clr);
I'd second this. Despite this chunk being called from the path in which
reg_int_clr is always set, I'd still prefer to have a check. I do not
like the idea of writing to an optional register without an explicit
check (or without a comment that this function should be used only when
reg_int_clr/reg_intr_status are defined).
But according to the downstream driver, this is supposed to be done in
the polling function, right before you do enable_irq().
Regards,
Bjorn
+
return IRQ_HANDLED;
}
@@ -368,6 +380,8 @@ static const struct qcom_cpufreq_soc_data epss_soc_data = {
.reg_dcvs_ctrl = 0xb0,
.reg_freq_lut = 0x100,
.reg_volt_lut = 0x200,
+ .reg_intr_clr = 0x308,
+ .reg_intr_status = 0x30c,
.reg_perf_state = 0x320,
.lut_row_size = 4,
};
--
2.33.0
--
With best wishes
Dmitry
