Hi Bjorn,
On 4/2/22 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;
+
+ 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?
To my knowledge returning IRQ_NONE assumes that right the same interrupt should
be still handled, either by another interrupt handler or by the original handler
again.
I believe here there is no difference in the sense above, since the interrupt is
not shared, it might happen that the check is always void and it should get its
justification, and definitely it's safe to omit the check/return here and just
make another poll/irq enable round, so, as the simplest working fix v1 of the
change without this check should be sufficient.
/* 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);
My understanding is that non-EPSS platforms do not specify a DCVS interrupt
under cpufreq-hw IP, if so, the interrupt handler is not registered and thus
the check for non-zero reg_intr_clr or other interrupt related registers is
not needed, please correct me.
But according to the downstream driver, this is supposed to be done in
the polling function, right before you do enable_irq().
The fact about downstream driver is true, however I believe functionally
there is no significant difference between clearing the interrupt status
after disabling the interrupt as above or right before enabling the interrupt
as in OSM.
The code above is simpler and arranged in the most relevant place, to my
understanding is slightly more correct, which is also proven by the testing.
--
Best wishes,
Vladimir
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
