Re: [PATCH v2 1/2] cpufreq: qcom-cpufreq-hw: Clear dcvs interrupts

[Bjorn Andersson <bjorn.andersson@linaro.org>]

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@linaro.org>
> ---
> 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?

>  
>  	/* 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);

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
>