[PATCH 0/3] ufs: core: Optimize the UIC command implementation

[PATCH 0/3] ufs: core: Optimize the UIC command implementation

[Bart Van Assche]

Hi Martin,

This patch series reduces the number of readl() calls while processing UIC
commands. Please consider this patch series for the next merge window.

Thanks,

Bart.

Bart Van Assche (3):
  ufs: core: Inline two functions related to UIC commands
  ufs: core: Complain if UIC argument 2 is invalid
  ufs: core: Optimize ufshcd_add_uic_command_trace()

 drivers/ufs/core/ufshcd.c | 49 +++++++--------------------------------
 1 file changed, 9 insertions(+), 40 deletions(-)

[PATCH 1/3] ufs: core: Inline two functions related to UIC commands

[Bart Van Assche]

The implementation of the two functions ufshcd_get_uic_cmd_result() and
ufshcd_get_dme_attr_val() is very short. Additionally, both functions
only have one caller. Hence, inline both functions.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/ufs/core/ufshcd.c | 37 ++++++++-----------------------------
 1 file changed, 8 insertions(+), 29 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 4805e40ed4d7..7fb3921bceb2 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -912,33 +912,6 @@ static inline int ufshcd_get_lists_status(u32 reg)
 	return !((reg & UFSHCD_STATUS_READY) == UFSHCD_STATUS_READY);
 }
 
-/**
- * ufshcd_get_uic_cmd_result - Get the UIC command result
- * @hba: Pointer to adapter instance
- *
- * This function gets the result of UIC command completion
- *
- * Return: 0 on success; non-zero value on error.
- */
-static inline int ufshcd_get_uic_cmd_result(struct ufs_hba *hba)
-{
-	return ufshcd_readl(hba, REG_UIC_COMMAND_ARG_2) &
-	       MASK_UIC_COMMAND_RESULT;
-}
-
-/**
- * ufshcd_get_dme_attr_val - Get the value of attribute returned by UIC command
- * @hba: Pointer to adapter instance
- *
- * This function gets UIC command argument3
- *
- * Return: 0 on success; non-zero value on error.
- */
-static inline u32 ufshcd_get_dme_attr_val(struct ufs_hba *hba)
-{
-	return ufshcd_readl(hba, REG_UIC_COMMAND_ARG_3);
-}
-
 /**
  * ufshcd_get_req_rsp - returns the TR response transaction type
  * @ucd_rsp_ptr: pointer to response UPIU
@@ -5716,8 +5689,14 @@ static irqreturn_t ufshcd_uic_cmd_compl(struct ufs_hba *hba, u32 intr_status)
 		hba->errors |= (UFSHCD_UIC_HIBERN8_MASK & intr_status);
 
 	if (intr_status & UIC_COMMAND_COMPL) {
-		cmd->argument2 |= ufshcd_get_uic_cmd_result(hba);
-		cmd->argument3 = ufshcd_get_dme_attr_val(hba);
+		/*
+		 * Store the UIC command result in the lowest byte of
+		 * cmd->argument2.
+		 */
+		cmd->argument2 |= ufshcd_readl(hba, REG_UIC_COMMAND_ARG_2) &
+				  MASK_UIC_COMMAND_RESULT;
+		/* Store the DME attribute value in cmd->argument3. */
+		cmd->argument3 = ufshcd_readl(hba, REG_UIC_COMMAND_ARG_3);
 		if (!hba->uic_async_done)
 			cmd->cmd_active = false;
 		complete(&cmd->done);

[PATCH 2/3] ufs: core: Complain if UIC argument 2 is invalid

[Bart Van Assche]

According to the UFSHCI standard, the lowest byte of UIC argument 2 is
an output value. Additionally, ufshcd_uic_cmd_compl() is based on the
assumption that the lowest byte of UIC argument 2 is zero. Hence, complain
if the result byte is set when a UIC command is submitted.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/ufs/core/ufshcd.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 7fb3921bceb2..0ff9d7c2a7ac 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -2571,6 +2571,7 @@ ufshcd_dispatch_uic_cmd(struct ufs_hba *hba, struct uic_command *uic_cmd)
 	lockdep_assert_held(&hba->uic_cmd_mutex);
 
 	WARN_ON(hba->active_uic_cmd);
+	WARN_ON_ONCE(uic_cmd->argument2 & MASK_UIC_COMMAND_RESULT);
 
 	hba->active_uic_cmd = uic_cmd;
 

[PATCH 3/3] ufs: core: Optimize ufshcd_add_uic_command_trace()

[Bart Van Assche]

Use cached values in ufshcd_add_uic_command_trace() instead of calling
readl(). In ufshcd_uic_cmd_compl(), also read the result byte for power
commands since it is also set if a power command completes.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/ufs/core/ufshcd.c | 23 ++++++-----------------
 1 file changed, 6 insertions(+), 17 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 0ff9d7c2a7ac..12445e012cad 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -460,20 +460,11 @@ static void ufshcd_add_uic_command_trace(struct ufs_hba *hba,
 					 const struct uic_command *ucmd,
 					 enum ufs_trace_str_t str_t)
 {
-	u32 cmd;
-
 	if (!trace_ufshcd_uic_command_enabled())
 		return;
 
-	if (str_t == UFS_CMD_SEND)
-		cmd = ucmd->command;
-	else
-		cmd = ufshcd_readl(hba, REG_UIC_COMMAND);
-
-	trace_ufshcd_uic_command(hba, str_t, cmd,
-				 ufshcd_readl(hba, REG_UIC_COMMAND_ARG_1),
-				 ufshcd_readl(hba, REG_UIC_COMMAND_ARG_2),
-				 ufshcd_readl(hba, REG_UIC_COMMAND_ARG_3));
+	trace_ufshcd_uic_command(hba, str_t, ucmd->command, ucmd->argument1,
+				 ucmd->argument2, ucmd->argument3);
 }
 
 static void ufshcd_add_command_trace(struct ufs_hba *hba, struct scsi_cmnd *cmd,
@@ -5689,13 +5680,11 @@ static irqreturn_t ufshcd_uic_cmd_compl(struct ufs_hba *hba, u32 intr_status)
 	if (ufshcd_is_auto_hibern8_error(hba, intr_status))
 		hba->errors |= (UFSHCD_UIC_HIBERN8_MASK & intr_status);
 
+	/* Store the UIC command result in the lowest byte of cmd->argument2. */
+	cmd->argument2 |= ufshcd_readl(hba, REG_UIC_COMMAND_ARG_2) &
+		MASK_UIC_COMMAND_RESULT;
+
 	if (intr_status & UIC_COMMAND_COMPL) {
-		/*
-		 * Store the UIC command result in the lowest byte of
-		 * cmd->argument2.
-		 */
-		cmd->argument2 |= ufshcd_readl(hba, REG_UIC_COMMAND_ARG_2) &
-				  MASK_UIC_COMMAND_RESULT;
 		/* Store the DME attribute value in cmd->argument3. */
 		cmd->argument3 = ufshcd_readl(hba, REG_UIC_COMMAND_ARG_3);
 		if (!hba->uic_async_done)

Re: [PATCH 1/3] ufs: core: Inline two functions related to UIC commands

[Peter Wang (王信友)]

On Fri, 2026-04-17 at 14:30 -0700, Bart Van Assche wrote:
> The implementation of the two functions ufshcd_get_uic_cmd_result()
> and
> ufshcd_get_dme_attr_val() is very short. Additionally, both functions
> only have one caller. Hence, inline both functions.
> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---

Reviewed-by: Peter Wang <peter.wang@mediatek.com>

Re: [PATCH 2/3] ufs: core: Complain if UIC argument 2 is invalid

[Peter Wang (王信友)]

On Fri, 2026-04-17 at 14:30 -0700, Bart Van Assche wrote:
> According to the UFSHCI standard, the lowest byte of UIC argument 2
> is
> an output value. Additionally, ufshcd_uic_cmd_compl() is based on the
> assumption that the lowest byte of UIC argument 2 is zero. Hence,
> complain
> if the result byte is set when a UIC command is submitted.
> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---

Reviewed-by: Peter Wang <peter.wang@mediatek.com>

Re: [PATCH 3/3] ufs: core: Optimize ufshcd_add_uic_command_trace()

[Peter Wang (王信友)]

On Fri, 2026-04-17 at 14:30 -0700, Bart Van Assche wrote:
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index 0ff9d7c2a7ac..12445e012cad 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -460,20 +460,11 @@ static void ufshcd_add_uic_command_trace(struct
> ufs_hba *hba,
>                                          const struct uic_command
> *ucmd,
>                                          enum ufs_trace_str_t str_t)
>  {
> -       u32 cmd;
> -
>         if (!trace_ufshcd_uic_command_enabled())
>                 return;
> 
> -       if (str_t == UFS_CMD_SEND)
> -               cmd = ucmd->command;
> -       else
> -               cmd = ufshcd_readl(hba, REG_UIC_COMMAND);
> -
> -       trace_ufshcd_uic_command(hba, str_t, cmd,
> -                                ufshcd_readl(hba,
> REG_UIC_COMMAND_ARG_1),
> -                                ufshcd_readl(hba,
> REG_UIC_COMMAND_ARG_2),
> -                                ufshcd_readl(hba,
> REG_UIC_COMMAND_ARG_3));
> +       trace_ufshcd_uic_command(hba, str_t, ucmd->command, ucmd-
> >argument1,
> +                                ucmd->argument2, ucmd->argument3);
>  }

Hi Bart,

In the complete case, all these commands and arguments 1 to 3 
are filled by hardware. If we do not read them from the hardware, 
how can we be sure of the actual values written by the hardware?

Thanks
Peter

Re: [PATCH 3/3] ufs: core: Optimize ufshcd_add_uic_command_trace()

[Bart Van Assche]

On 4/21/26 1:45 AM, Peter Wang (王信友) wrote:
> In the complete case, all these commands and arguments 1 to 3
> are filled by hardware. If we do not read them from the hardware,
> how can we be sure of the actual values written by the hardware?

Hi Peter,

Are we perhaps each interpreting the UFSHCI standard in a different way?
My understanding is that the UFSHCI command flow is as follows:
* First, UICCMDARG1, UICCMDARG2 and UICCMDARG3 are written by the host.
* Next, UICCMD is written by the host. This causes the host controller
   to execute the UIC command.
* Upon completion of the command, the host controller updates the lowest
   byte of UICCMDARG2. UICCMDARG3 is only updated after execution of the
   following commands has finished: DME_GET, DME_SET, DME_PEER_GET and
   DME_PEER_SET.

Thanks,

Bart.

Re: [PATCH 3/3] ufs: core: Optimize ufshcd_add_uic_command_trace()

[Peter Wang (王信友)]

On Tue, 2026-04-21 at 12:37 -0700, Bart Van Assche wrote:
> Hi Peter,
> 
> Are we perhaps each interpreting the UFSHCI standard in a different
> way?
> My understanding is that the UFSHCI command flow is as follows:
> * First, UICCMDARG1, UICCMDARG2 and UICCMDARG3 are written by the
> host.
> * Next, UICCMD is written by the host. This causes the host
> controller
>    to execute the UIC command.
> * Upon completion of the command, the host controller updates the
> lowest
>    byte of UICCMDARG2. UICCMDARG3 is only updated after execution of
> the
>    following commands has finished: DME_GET, DME_SET, DME_PEER_GET
> and
>    DME_PEER_SET.
> 
> Thanks,
> 
> Bart.

Hi Bart,

Yes, you are right! In general cases, this is true.
But for some error cases, we need this trace debug log to check
if the hardware is working as the software expects.
Maybe the hardware is stuck or something like that.
Therefore, we still need to read the register values from 
the hardware.

Thanks
Peter

Re: [PATCH 3/3] ufs: core: Optimize ufshcd_add_uic_command_trace()

[Bart Van Assche]

On 4/22/26 12:52 AM, Peter Wang (王信友) wrote:
> Yes, you are right! In general cases, this is true.
> But for some error cases, we need this trace debug log to check
> if the hardware is working as the software expects.
> Maybe the hardware is stuck or something like that.
> Therefore, we still need to read the register values from
> the hardware.

Hi Peter,

I have started testing the patch below. This patch is intended as a
replacement for patch 3/3 in this series:

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index a44ef7e97125..76416ee88b25 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -460,20 +460,19 @@ static void ufshcd_add_uic_command_trace(struct 
ufs_hba *hba,
  					 const struct uic_command *ucmd,
  					 enum ufs_trace_str_t str_t)
  {
-	u32 cmd;
-
  	if (!trace_ufshcd_uic_command_enabled())
  		return;

  	if (str_t == UFS_CMD_SEND)
-		cmd = ucmd->command;
+		trace_ufshcd_uic_command(hba, str_t, ucmd->command,
+					 ucmd->argument1, ucmd->argument2,
+					 ucmd->argument3);
  	else
-		cmd = ufshcd_readl(hba, REG_UIC_COMMAND);
-
-	trace_ufshcd_uic_command(hba, str_t, cmd,
-				 ufshcd_readl(hba, REG_UIC_COMMAND_ARG_1),
-				 ufshcd_readl(hba, REG_UIC_COMMAND_ARG_2),
-				 ufshcd_readl(hba, REG_UIC_COMMAND_ARG_3));
+		trace_ufshcd_uic_command(
+			hba, str_t, ufshcd_readl(hba, REG_UIC_COMMAND),
+			ufshcd_readl(hba, REG_UIC_COMMAND_ARG_1),
+			ufshcd_readl(hba, REG_UIC_COMMAND_ARG_2),
+			ufshcd_readl(hba, REG_UIC_COMMAND_ARG_3));
  }

  static void ufshcd_add_command_trace(struct ufs_hba *hba, struct 
scsi_cmnd *cmd,

Re: [PATCH 3/3] ufs: core: Optimize ufshcd_add_uic_command_trace()

[Peter Wang (王信友)]

On Wed, 2026-04-22 at 09:30 -0700, Bart Van Assche wrote:
> Hi Peter,
> 
> I have started testing the patch below. This patch is intended as a
> replacement for patch 3/3 in this series:
> 
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index a44ef7e97125..76416ee88b25 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -460,20 +460,19 @@ static void ufshcd_add_uic_command_trace(struct
> ufs_hba *hba,
>                                          const struct uic_command
> *ucmd,
>                                          enum ufs_trace_str_t str_t)
>   {
> -       u32 cmd;
> -
>         if (!trace_ufshcd_uic_command_enabled())
>                 return;
> 
>         if (str_t == UFS_CMD_SEND)
> -               cmd = ucmd->command;
> +               trace_ufshcd_uic_command(hba, str_t, ucmd->command,
> +                                        ucmd->argument1, ucmd-
> >argument2,
> +                                        ucmd->argument3);
>         else
> -               cmd = ufshcd_readl(hba, REG_UIC_COMMAND);
> -
> -       trace_ufshcd_uic_command(hba, str_t, cmd,
> -                                ufshcd_readl(hba,
> REG_UIC_COMMAND_ARG_1),
> -                                ufshcd_readl(hba,
> REG_UIC_COMMAND_ARG_2),
> -                                ufshcd_readl(hba,
> REG_UIC_COMMAND_ARG_3));
> +               trace_ufshcd_uic_command(
> +                       hba, str_t, ufshcd_readl(hba,
> REG_UIC_COMMAND),
> +                       ufshcd_readl(hba, REG_UIC_COMMAND_ARG_1),
> +                       ufshcd_readl(hba, REG_UIC_COMMAND_ARG_2),
> +                       ufshcd_readl(hba, REG_UIC_COMMAND_ARG_3));
>   }
> 
>   static void ufshcd_add_command_trace(struct ufs_hba *hba, struct
> scsi_cmnd *cmd,
> 

Hi Bart,

This patch looks good to me.

Thanks
Peter