[PATCH v2 0/2] cpufreq: qcom-cpufreq-hw: Fixes to DCVS interrupt handling on EPSS

[PATCH v2 0/2] cpufreq: qcom-cpufreq-hw: Fixes to DCVS interrupt handling on EPSS

[Vladimir Zapolskiy]

The series contains of two critical fixes for QCOM EPSS cpufreq-hw
driver, the fixes correct runtime issues discovered on newer supported
QCOM platforms such as SM8250 and SM8450.

The changes are based on next-20220401 tag.

Changes from v1 to v2, thanks to Bjorn:
* added a check for pending interrupt status before its handling,
* added a comment about a replaced number in the code.

Vladimir Zapolskiy (2):
  cpufreq: qcom-cpufreq-hw: Clear dcvs interrupts
  cpufreq: qcom-cpufreq-hw: Fix throttle frequency value on EPSS platforms

 drivers/cpufreq/qcom-cpufreq-hw.c | 31 +++++++++++++++++++++++++------
 1 file changed, 25 insertions(+), 6 deletions(-)

-- 
2.33.0

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

[Vladimir Zapolskiy]

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);
+	if (!(val & GT_IRQ_STATUS))
+		return IRQ_HANDLED;
 
 	/* 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);
+
 	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

[PATCH v2 2/2] cpufreq: qcom-cpufreq-hw: Fix throttle frequency value on EPSS platforms

[Vladimir Zapolskiy]

On QCOM platforms with EPSS flavour of cpufreq IP a throttled frequency is
obtained from another register REG_DOMAIN_STATE, thus the helper function
qcom_lmh_get_throttle_freq() should be modified accordingly, as for now
it returns gibberish since .reg_current_vote is unset for EPSS hardware.

To exclude a hardcoded magic number 19200 it is replaced by "xo" clock rate
in KHz.

Fixes: 275157b367f4 ("cpufreq: qcom-cpufreq-hw: Add dcvs interrupt support")
Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
---
Changes from v1 to v2:
* added a comment about a replaced 19200 number in the code, thanks
  to Bjorn for suggestion and review.

 drivers/cpufreq/qcom-cpufreq-hw.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
index e17413a6f120..d5ede769b09a 100644
--- a/drivers/cpufreq/qcom-cpufreq-hw.c
+++ b/drivers/cpufreq/qcom-cpufreq-hw.c
@@ -30,6 +30,7 @@
 
 struct qcom_cpufreq_soc_data {
 	u32 reg_enable;
+	u32 reg_domain_state;
 	u32 reg_dcvs_ctrl;
 	u32 reg_freq_lut;
 	u32 reg_volt_lut;
@@ -284,11 +285,16 @@ static void qcom_get_related_cpus(int index, struct cpumask *m)
 	}
 }
 
-static unsigned int qcom_lmh_get_throttle_freq(struct qcom_cpufreq_data *data)
+static unsigned long qcom_lmh_get_throttle_freq(struct qcom_cpufreq_data *data)
 {
-	unsigned int val = readl_relaxed(data->base + data->soc_data->reg_current_vote);
+	unsigned int lval;
 
-	return (val & 0x3FF) * 19200;
+	if (data->soc_data->reg_current_vote)
+		lval = readl_relaxed(data->base + data->soc_data->reg_current_vote) & 0x3ff;
+	else
+		lval = readl_relaxed(data->base + data->soc_data->reg_domain_state) & 0xff;
+
+	return lval * xo_rate;
 }
 
 static void qcom_lmh_dcvs_notify(struct qcom_cpufreq_data *data)
@@ -298,14 +304,12 @@ static void qcom_lmh_dcvs_notify(struct qcom_cpufreq_data *data)
 	struct device *dev = get_cpu_device(cpu);
 	unsigned long freq_hz, throttled_freq;
 	struct dev_pm_opp *opp;
-	unsigned int freq;
 
 	/*
 	 * Get the h/w throttled frequency, normalize it using the
 	 * registered opp table and use it to calculate thermal pressure.
 	 */
-	freq = qcom_lmh_get_throttle_freq(data);
-	freq_hz = freq * HZ_PER_KHZ;
+	freq_hz = qcom_lmh_get_throttle_freq(data);
 
 	opp = dev_pm_opp_find_freq_floor(dev, &freq_hz);
 	if (IS_ERR(opp) && PTR_ERR(opp) == -ERANGE)
@@ -377,6 +381,7 @@ static const struct qcom_cpufreq_soc_data qcom_soc_data = {
 
 static const struct qcom_cpufreq_soc_data epss_soc_data = {
 	.reg_enable = 0x0,
+	.reg_domain_state = 0x20,
 	.reg_dcvs_ctrl = 0xb0,
 	.reg_freq_lut = 0x100,
 	.reg_volt_lut = 0x200,
-- 
2.33.0

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

[Bjorn Andersson]

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
> 

Re: [PATCH v2 2/2] cpufreq: qcom-cpufreq-hw: Fix throttle frequency value on EPSS platforms

[Bjorn Andersson]

On Fri 01 Apr 00:14 PDT 2022, Vladimir Zapolskiy wrote:

> On QCOM platforms with EPSS flavour of cpufreq IP a throttled frequency is
> obtained from another register REG_DOMAIN_STATE, thus the helper function
> qcom_lmh_get_throttle_freq() should be modified accordingly, as for now
> it returns gibberish since .reg_current_vote is unset for EPSS hardware.
> 
> To exclude a hardcoded magic number 19200 it is replaced by "xo" clock rate
> in KHz.
> 
> Fixes: 275157b367f4 ("cpufreq: qcom-cpufreq-hw: Add dcvs interrupt support")
> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>

This could have been picked up by the maintainer already in the previous
version, if it wasn't the second patch in the series. Please send it
separately, or as the first patch of the two, so we can ask Viresh to
pick it up (just in case we don't reach an agreement of your next
version of the other patch).

Regards,
Bjorn

> ---
> Changes from v1 to v2:
> * added a comment about a replaced 19200 number in the code, thanks
>   to Bjorn for suggestion and review.
> 
>  drivers/cpufreq/qcom-cpufreq-hw.c | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
> index e17413a6f120..d5ede769b09a 100644
> --- a/drivers/cpufreq/qcom-cpufreq-hw.c
> +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
> @@ -30,6 +30,7 @@
>  
>  struct qcom_cpufreq_soc_data {
>  	u32 reg_enable;
> +	u32 reg_domain_state;
>  	u32 reg_dcvs_ctrl;
>  	u32 reg_freq_lut;
>  	u32 reg_volt_lut;
> @@ -284,11 +285,16 @@ static void qcom_get_related_cpus(int index, struct cpumask *m)
>  	}
>  }
>  
> -static unsigned int qcom_lmh_get_throttle_freq(struct qcom_cpufreq_data *data)
> +static unsigned long qcom_lmh_get_throttle_freq(struct qcom_cpufreq_data *data)
>  {
> -	unsigned int val = readl_relaxed(data->base + data->soc_data->reg_current_vote);
> +	unsigned int lval;
>  
> -	return (val & 0x3FF) * 19200;
> +	if (data->soc_data->reg_current_vote)
> +		lval = readl_relaxed(data->base + data->soc_data->reg_current_vote) & 0x3ff;
> +	else
> +		lval = readl_relaxed(data->base + data->soc_data->reg_domain_state) & 0xff;
> +
> +	return lval * xo_rate;
>  }
>  
>  static void qcom_lmh_dcvs_notify(struct qcom_cpufreq_data *data)
> @@ -298,14 +304,12 @@ static void qcom_lmh_dcvs_notify(struct qcom_cpufreq_data *data)
>  	struct device *dev = get_cpu_device(cpu);
>  	unsigned long freq_hz, throttled_freq;
>  	struct dev_pm_opp *opp;
> -	unsigned int freq;
>  
>  	/*
>  	 * Get the h/w throttled frequency, normalize it using the
>  	 * registered opp table and use it to calculate thermal pressure.
>  	 */
> -	freq = qcom_lmh_get_throttle_freq(data);
> -	freq_hz = freq * HZ_PER_KHZ;
> +	freq_hz = qcom_lmh_get_throttle_freq(data);
>  
>  	opp = dev_pm_opp_find_freq_floor(dev, &freq_hz);
>  	if (IS_ERR(opp) && PTR_ERR(opp) == -ERANGE)
> @@ -377,6 +381,7 @@ static const struct qcom_cpufreq_soc_data qcom_soc_data = {
>  
>  static const struct qcom_cpufreq_soc_data epss_soc_data = {
>  	.reg_enable = 0x0,
> +	.reg_domain_state = 0x20,
>  	.reg_dcvs_ctrl = 0xb0,
>  	.reg_freq_lut = 0x100,
>  	.reg_volt_lut = 0x200,
> -- 
> 2.33.0
> 

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

[Vladimir Zapolskiy]

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@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?
> 

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
>>

Re: [PATCH v2 2/2] cpufreq: qcom-cpufreq-hw: Fix throttle frequency value on EPSS platforms

[Viresh Kumar]

On 01-04-22, 15:26, Bjorn Andersson wrote:
> On Fri 01 Apr 00:14 PDT 2022, Vladimir Zapolskiy wrote:
> 
> > On QCOM platforms with EPSS flavour of cpufreq IP a throttled frequency is
> > obtained from another register REG_DOMAIN_STATE, thus the helper function
> > qcom_lmh_get_throttle_freq() should be modified accordingly, as for now
> > it returns gibberish since .reg_current_vote is unset for EPSS hardware.
> > 
> > To exclude a hardcoded magic number 19200 it is replaced by "xo" clock rate
> > in KHz.
> > 
> > Fixes: 275157b367f4 ("cpufreq: qcom-cpufreq-hw: Add dcvs interrupt support")
> > Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
> 
> This could have been picked up by the maintainer already in the previous
> version, if it wasn't the second patch in the series. Please send it
> separately, or as the first patch of the two, so we can ask Viresh to
> pick it up (just in case we don't reach an agreement of your next
> version of the other patch).

I have applied 2/2 now. I hope it doesn't break anything.

-- 
viresh

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

[Dmitry Baryshkov]

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@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;
>> +

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