[PATCH] scsi: ufs: core: Handle MCQ IAG events

[PATCH] scsi: ufs: core: Handle MCQ IAG events

[vamshi gajjela]

Add support for handling aggregation-based interrupts when operating
in MCQ mode.

In legacy interrupt mode, an IE.IAGES is triggered when the counter
or timer threshold is reached. To manage this, the handler now resets
the aggregation counter and timer by writing to the MCQIACRy.CTR
register.

Since the register layout of MCQIACRy is identical to the existing
UTRIACR register, this implementation reuses the previously defined
bitfield masks to maintain consistency and reduce code duplication.

Extend ufshcd_handle_mcq_cq_events() with a boolean iag parameter.
If set, the handler resets the MCQ IAG counter and timer.

Define MCQ_IAG_EVENT_STATUS (0x200000) and include it in
UFSHCD_ENABLE_MCQ_INTRS to ensure the interrupt is unmasked during
initialization.

Signed-off-by: vamshi gajjela <vamshigajjela@google.com>
---
 drivers/ufs/core/ufs-mcq.c     | 13 ++++++++++++-
 drivers/ufs/core/ufshcd-priv.h |  2 ++
 drivers/ufs/core/ufshcd.c      | 17 ++++++++++++++---
 include/ufs/ufshci.h           |  2 ++
 4 files changed, 30 insertions(+), 4 deletions(-)

diff --git a/drivers/ufs/core/ufs-mcq.c b/drivers/ufs/core/ufs-mcq.c
index 18a95b728633..377a57ce1fec 100644
--- a/drivers/ufs/core/ufs-mcq.c
+++ b/drivers/ufs/core/ufs-mcq.c
@@ -31,7 +31,8 @@
 
 #define UFSHCD_ENABLE_MCQ_INTRS	(UTP_TASK_REQ_COMPL |\
 				 UFSHCD_ERROR_MASK |\
-				 MCQ_CQ_EVENT_STATUS)
+				 MCQ_CQ_EVENT_STATUS |\
+				 MCQ_IAG_EVENT_STATUS)
 
 /* Max mcq register polling time in microseconds */
 #define MCQ_POLL_US 500000
@@ -272,6 +273,16 @@ void ufshcd_mcq_write_cqis(struct ufs_hba *hba, u32 val, int i)
 }
 EXPORT_SYMBOL_GPL(ufshcd_mcq_write_cqis);
 
+u32 ufshcd_mcq_read_mcqiacr(struct ufs_hba *hba, int i)
+{
+	return readl(mcq_opr_base(hba, OPR_CQIS, i) + REG_MCQIACR);
+}
+
+void ufshcd_mcq_write_mcqiacr(struct ufs_hba *hba, u32 val, int i)
+{
+	writel(val, mcq_opr_base(hba, OPR_CQIS, i) + REG_MCQIACR);
+}
+
 /*
  * Current MCQ specification doesn't provide a Task Tag or its equivalent in
  * the Completion Queue Entry. Find the Task Tag using an indirect method.
diff --git a/drivers/ufs/core/ufshcd-priv.h b/drivers/ufs/core/ufshcd-priv.h
index 37c32071e754..6d3d14e883b8 100644
--- a/drivers/ufs/core/ufshcd-priv.h
+++ b/drivers/ufs/core/ufshcd-priv.h
@@ -76,6 +76,8 @@ void ufshcd_mcq_compl_all_cqes_lock(struct ufs_hba *hba,
 bool ufshcd_cmd_inflight(struct scsi_cmnd *cmd);
 int ufshcd_mcq_sq_cleanup(struct ufs_hba *hba, int task_tag);
 int ufshcd_mcq_abort(struct scsi_cmnd *cmd);
+u32 ufshcd_mcq_read_mcqiacr(struct ufs_hba *hba, int i);
+void ufshcd_mcq_write_mcqiacr(struct ufs_hba *hba, u32 val, int i);
 int ufshcd_try_to_abort_task(struct ufs_hba *hba, int tag);
 void ufshcd_release_scsi_cmd(struct ufs_hba *hba, struct scsi_cmnd *cmd);
 
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 847b55789bb8..a22e1a51cb6f 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -7084,16 +7084,17 @@ static irqreturn_t ufshcd_tmc_handler(struct ufs_hba *hba)
 /**
  * ufshcd_handle_mcq_cq_events - handle MCQ completion queue events
  * @hba: per adapter instance
+ * @iag: true, to reset MCQ IAG counter and timer of the CQ
  *
  * Return: IRQ_HANDLED if interrupt is handled.
  */
-static irqreturn_t ufshcd_handle_mcq_cq_events(struct ufs_hba *hba)
+static irqreturn_t ufshcd_handle_mcq_cq_events(struct ufs_hba *hba, bool iag)
 {
 	struct ufs_hw_queue *hwq;
 	unsigned long outstanding_cqs;
 	unsigned int nr_queues;
 	int i, ret;
-	u32 events;
+	u32 events, reg;
 
 	ret = ufshcd_vops_get_outstanding_cqs(hba, &outstanding_cqs);
 	if (ret)
@@ -7108,6 +7109,13 @@ static irqreturn_t ufshcd_handle_mcq_cq_events(struct ufs_hba *hba)
 		if (events)
 			ufshcd_mcq_write_cqis(hba, events, i);
 
+		/* Clear MCQ IAG counter and timer of the CQ */
+		if (iag) {
+			reg = ufshcd_mcq_read_mcqiacr(hba, i);
+			reg |= INT_AGGR_COUNTER_AND_TIMER_RESET;
+			ufshcd_mcq_write_mcqiacr(hba, reg, i);
+		}
+
 		if (events & UFSHCD_MCQ_CQIS_TAIL_ENT_PUSH_STS)
 			ufshcd_mcq_poll_cqe_lock(hba, hwq);
 	}
@@ -7141,7 +7149,10 @@ static irqreturn_t ufshcd_sl_intr(struct ufs_hba *hba, u32 intr_status)
 		retval |= ufshcd_transfer_req_compl(hba);
 
 	if (intr_status & MCQ_CQ_EVENT_STATUS)
-		retval |= ufshcd_handle_mcq_cq_events(hba);
+		retval |= ufshcd_handle_mcq_cq_events(hba, false);
+
+	if (intr_status & MCQ_IAG_EVENT_STATUS)
+		retval |= ufshcd_handle_mcq_cq_events(hba, true);
 
 	return retval;
 }
diff --git a/include/ufs/ufshci.h b/include/ufs/ufshci.h
index 806fdaf52bd9..43e87078538a 100644
--- a/include/ufs/ufshci.h
+++ b/include/ufs/ufshci.h
@@ -115,6 +115,7 @@ enum {
 enum {
 	REG_CQIS		= 0x0,
 	REG_CQIE		= 0x4,
+	REG_MCQIACR		= 0x8,
 };
 
 enum {
@@ -188,6 +189,7 @@ static inline u32 ufshci_version(u32 major, u32 minor)
 #define SYSTEM_BUS_FATAL_ERROR			0x20000
 #define CRYPTO_ENGINE_FATAL_ERROR		0x40000
 #define MCQ_CQ_EVENT_STATUS			0x100000
+#define MCQ_IAG_EVENT_STATUS			0x200000
 
 #define UFSHCD_UIC_HIBERN8_MASK	(UIC_HIBERNATE_ENTER |\
 				UIC_HIBERNATE_EXIT)
-- 
2.53.0.473.g4a7958ca14-goog

Re: [PATCH] scsi: ufs: core: Handle MCQ IAG events

[Peter Wang (王信友)]

On Mon, 2026-03-02 at 23:31 +0530, vamshi gajjela wrote:
> @@ -7141,7 +7149,10 @@ static irqreturn_t ufshcd_sl_intr(struct
> ufs_hba *hba, u32 intr_status)
>  		retval |= ufshcd_transfer_req_compl(hba);
>  
>  	if (intr_status & MCQ_CQ_EVENT_STATUS)
> -		retval |= ufshcd_handle_mcq_cq_events(hba);
> +		retval |= ufshcd_handle_mcq_cq_events(hba, false);
> +
> +	if (intr_status & MCQ_IAG_EVENT_STATUS)
> +		retval |= ufshcd_handle_mcq_cq_events(hba, true);

Hi Vamshi,

This is strange to me.
Why does receiving an IAG_EVENT call ufshcd_handle_mcq_cq_events?
Shouldn't it be ufshcd_handle_mcq_iag_events instead?

Thanks
Peter

Re: [PATCH] scsi: ufs: core: Handle MCQ IAG events

[Bart Van Assche]

On 3/3/26 4:25 AM, Peter Wang (王信友) wrote:
> On Mon, 2026-03-02 at 23:31 +0530, vamshi gajjela wrote:
>> @@ -7141,7 +7149,10 @@ static irqreturn_t ufshcd_sl_intr(struct
>> ufs_hba *hba, u32 intr_status)
>>  retval |= ufshcd_transfer_req_compl(hba);
>>  
>>  if (intr_status & MCQ_CQ_EVENT_STATUS)
>> -retval |= ufshcd_handle_mcq_cq_events(hba);
>> +retval |= ufshcd_handle_mcq_cq_events(hba, false);
>> +
>> +if (intr_status & MCQ_IAG_EVENT_STATUS)
>> +retval |= ufshcd_handle_mcq_cq_events(hba, true);
> 
> Hi Vamshi,
> 
> This is strange to me.
> Why does receiving an IAG_EVENT call ufshcd_handle_mcq_cq_events?
> Shouldn't it be ufshcd_handle_mcq_iag_events instead?
Doesn't this follow from the UFSHCI standard? From the UFSHCI 5.0
standard: "MCQ Interrupt Aggregation Event Status (IAGES): This bit is
transparent and becomes ‘1’ when all of the following conditions are met
• Controller is operating in MCQ mode (Config.QT = 1)
• ESI is not enabled (Config.ESIE = 0)
• At least one interrupt aggregation group has triggered, which means it
has satisfied either counter or timer condition

When in MCQ mode, and ESI is not used, SW can use traditional interrupt 
approach. When this bit is set, interrupt routine needs to scan all
interrupt aggregation groups to determine which IAG has caused this
interrupt.

Thanks,

Bart.

Re: [PATCH] scsi: ufs: core: Handle MCQ IAG events

[Bart Van Assche]

On 3/2/26 12:01 PM, vamshi gajjela wrote:
> -static irqreturn_t ufshcd_handle_mcq_cq_events(struct ufs_hba *hba)
> +static irqreturn_t ufshcd_handle_mcq_cq_events(struct ufs_hba *hba, bool iag)

Please change the name of the "iag" argument into "reset_iag" to make
the purpose of this argument more clear.

> +		/* Clear MCQ IAG counter and timer of the CQ */

Please remove the above comment since it duplicates the explanation of
the "iag" argument.

Otherwise this patch looks good to me.

Thanks,

Bart.

Re: [PATCH] scsi: ufs: core: Handle MCQ IAG events

[Peter Wang (王信友)]

On Wed, 2026-03-04 at 09:02 -0600, Bart Van Assche wrote:
> Doesn't this follow from the UFSHCI standard? From the UFSHCI 5.0
> standard: "MCQ Interrupt Aggregation Event Status (IAGES): This bit
> is
> transparent and becomes ‘1’ when all of the following conditions are
> met
> • Controller is operating in MCQ mode (Config.QT = 1)
> • ESI is not enabled (Config.ESIE = 0)
> • At least one interrupt aggregation group has triggered, which means
> it
> has satisfied either counter or timer condition
> 
> When in MCQ mode, and ESI is not used, SW can use traditional
> interrupt
> approach. When this bit is set, interrupt routine needs to scan all
> interrupt aggregation groups to determine which IAG has caused this
> interrupt.
> 
> Thanks,
> 
> Bart.

Hi Bart,

I know this is absolutely correct, but reading this code is confusing:
if (cq event)
    handle cq event
if (iag event)
    handle cq event
If we cannot change it to:
if (iag event)
    handle iag event
At least, we should change the function name from
ufshcd_handle_mcq_cq_events to ufshcd_handle_mcq_cq_iag_events.

Thanks
Peter

Re: [PATCH] scsi: ufs: core: Handle MCQ IAG events

[Bart Van Assche]

On 3/4/26 9:28 PM, Peter Wang (王信友) wrote:
> I know this is absolutely correct, but reading this code is confusing:
> if (cq event)
>      handle cq event
> if (iag event)
>      handle cq event
> If we cannot change it to:
> if (iag event)
>      handle iag event

Hi Peter,

It is not clear to me why the above code is considered confusing?

UFS controllers are the only storage controllers I know of
that generate different interrupts depending on whether or not interrupt
aggregation is enabled. All other storage controllers I know of use the
same completion interrupt whether or not interrupt aggregation is
enabled.

To me the above code means that whether or not interrupt aggregation is
enabled, ufshcd_handle_mcq_cq_events() is called to process the pending
completions.

Thanks,

Bart.

Re: [PATCH] scsi: ufs: core: Handle MCQ IAG events

[Peter Wang (王信友)]

On Thu, 2026-03-05 at 06:07 -0600, Bart Van Assche wrote
> 
> Hi Peter,
> 
> It is not clear to me why the above code is considered confusing?
> 
> UFS controllers are the only storage controllers I know of
> that generate different interrupts depending on whether or not
> interrupt
> aggregation is enabled. All other storage controllers I know of use
> the
> same completion interrupt whether or not interrupt aggregation is
> enabled.
> 
> To me the above code means that whether or not interrupt aggregation
> is
> enabled, ufshcd_handle_mcq_cq_events() is called to process the
> pending
> completions.
> 
> Thanks,
> 
> Bart.

Hi Bart,

Sorry, I may not have explained it clearly enough. Normally,
the logic is to handle A when receiving A event, and handle B 
when receiving B event. But now, the code seems to be hnadle A 
when receiving B event.
If not familiar with this hardware logic, it’s easy to
misunderstand.

Thanks
Peter

Re: [PATCH] scsi: ufs: core: Handle MCQ IAG events

[VAMSHI GAJJELA]

 or

On Fri, Mar 6, 2026 at 9:19 AM Peter Wang (王信友) <peter.wang@mediatek.com> wrote:
>
> On Thu, 2026-03-05 at 06:07 -0600, Bart Van Assche wrote
> >
> > Hi Peter,
> >
> > It is not clear to me why the above code is considered confusing?
> >
> > UFS controllers are the only storage controllers I know of
> > that generate different interrupts depending on whether or not
> > interrupt
> > aggregation is enabled. All other storage controllers I know of use
> > the
> > same completion interrupt whether or not interrupt aggregation is
> > enabled.
> >
> > To me the above code means that whether or not interrupt aggregation
> > is
> > enabled, ufshcd_handle_mcq_cq_events() is called to process the
> > pending
> > completions.
> >
> > Thanks,
> >
> > Bart.
>
> Hi Bart,
>
> Sorry, I may not have explained it clearly enough. Normally,
> the logic is to handle A when receiving A event, and handle B
> when receiving B event. But now, the code seems to be hnadle A
> when receiving B event.
> If not familiar with this hardware logic, it’s easy to
> misunderstand.
>
> Thanks
> Peter

Hi Peter and Bart, thanks for the feedback and discussion

Bart: I agree that renaming the boolean flag to reset_iag improves clarity,
and I will address this in the next version of the patch

Peter:  I understand your concern about the readability when calling
ufshcd_handle_mcq_cq_events for both the events. As said, either
MCQ_CQ_EVENT_STATUS or MCQ_IAG_EVENT_STATUS, we are
essentially handling Completion Queue events. The only extra step in
the IAG path is clearing the counters/timers.

Sticking to ufshcd_handle_mcq_cq_events and adding the agreed-upon
descriptive argument reset_iag should help the readability IMO.

Do you think a more detailed comments right before those two function
calls would be beneficial?

Regards,
Vamshi G.