Re: [PATCH v3 12/15] scsi: ufs: reduce the interrupts for power mode change requests

From: Akinobu Mita
Date: Wed Oct 21 2015 - 10:57:12 EST

2015-09-02 19:13 GMT+09:00 Yaniv Gardi <ygardi@xxxxxxxxxxxxxx>:
> DME commands such as Hibern8 enter/exit and gear switch generate 2
> completion interrupts, one for confirmation that command is received
> by local UniPro and 2nd one is the final confirmation after communication
> with remote UniPro. Currently both of these completions are registered
> as interrupt events which is not quite necessary and instead we can
> just wait for the interrupt of 2nd completion, this should reduce
> the number of interrupts and could reduce the unnecessary CPU wakeups
> to handle extra interrupts.
>
> Signed-off-by: Subhash Jadavani <subhashj@xxxxxxxxxxxxxx>
> Signed-off-by: Yaniv Gardi <ygardi@xxxxxxxxxxxxxx>
>
> ---
> drivers/scsi/ufs/ufshcd.c | 41 +++++++++++++++++++++++++++--------------
> 1 file changed, 27 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index f2d4301..fc2a52d 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -986,13 +986,15 @@ ufshcd_wait_for_uic_cmd(struct ufs_hba *hba, struct uic_command *uic_cmd)
> * __ufshcd_send_uic_cmd - Send UIC commands and retrieve the result
> * @hba: per adapter instance
> * @uic_cmd: UIC command
> + * @completion: initialize the completion only if this is set to true
> *
> * Identical to ufshcd_send_uic_cmd() expect mutex. Must be called
> * with mutex held and host_lock locked.
> * Returns 0 only if success.
> */
> static int
> -__ufshcd_send_uic_cmd(struct ufs_hba *hba, struct uic_command *uic_cmd)
> +__ufshcd_send_uic_cmd(struct ufs_hba *hba, struct uic_command *uic_cmd,
> + bool completion)
> {
> if (!ufshcd_ready_for_uic_cmd(hba)) {
> dev_err(hba->dev,
> @@ -1000,7 +1002,8 @@ __ufshcd_send_uic_cmd(struct ufs_hba *hba, struct uic_command *uic_cmd)
> return -EIO;
> }
>
> - init_completion(&uic_cmd->done);
> + if (completion)
> + init_completion(&uic_cmd->done);
>
> ufshcd_dispatch_uic_cmd(hba, uic_cmd);
>
> @@ -1025,7 +1028,7 @@ ufshcd_send_uic_cmd(struct ufs_hba *hba, struct uic_command *uic_cmd)
> ufshcd_add_delay_before_dme_cmd(hba);
>
> spin_lock_irqsave(hba->host->host_lock, flags);
> - ret = __ufshcd_send_uic_cmd(hba, uic_cmd);
> + ret = __ufshcd_send_uic_cmd(hba, uic_cmd, true);
> spin_unlock_irqrestore(hba->host->host_lock, flags);
> if (!ret)
> ret = ufshcd_wait_for_uic_cmd(hba, uic_cmd);
> @@ -2346,6 +2349,7 @@ static int ufshcd_uic_pwr_ctrl(struct ufs_hba *hba, struct uic_command *cmd)
> unsigned long flags;
> u8 status;
> int ret;
> + bool reenable_intr = false;
>
> mutex_lock(&hba->uic_cmd_mutex);
> init_completion(&uic_async_done);
> @@ -2353,15 +2357,17 @@ static int ufshcd_uic_pwr_ctrl(struct ufs_hba *hba, struct uic_command *cmd)
>
> spin_lock_irqsave(hba->host->host_lock, flags);
> hba->uic_async_done = &uic_async_done;
> - ret = __ufshcd_send_uic_cmd(hba, cmd);
> - spin_unlock_irqrestore(hba->host->host_lock, flags);
> - if (ret) {
> - dev_err(hba->dev,
> - "pwr ctrl cmd 0x%x with mode 0x%x uic error %d\n",
> - cmd->command, cmd->argument3, ret);
> - goto out;
> + if (ufshcd_readl(hba, REG_INTERRUPT_ENABLE) & UIC_COMMAND_COMPL) {
> + ufshcd_disable_intr(hba, UIC_COMMAND_COMPL);
> + /*
> + * Make sure UIC command completion interrupt is disabled before
> + * issuing UIC command.
> + */
> + wmb();
> + reenable_intr = true;
> }
> - ret = ufshcd_wait_for_uic_cmd(hba, cmd);
> + ret = __ufshcd_send_uic_cmd(hba, cmd, false);
> + spin_unlock_irqrestore(hba->host->host_lock, flags);
> if (ret) {
> dev_err(hba->dev,
> "pwr ctrl cmd 0x%x with mode 0x%x uic error %d\n",
> @@ -2387,7 +2393,10 @@ static int ufshcd_uic_pwr_ctrl(struct ufs_hba *hba, struct uic_command *cmd)
> }
> out:
> spin_lock_irqsave(hba->host->host_lock, flags);
> + hba->active_uic_cmd = NULL;
> hba->uic_async_done = NULL;
> + if (reenable_intr)
> + ufshcd_enable_intr(hba, UIC_COMMAND_COMPL);
> spin_unlock_irqrestore(hba->host->host_lock, flags);
> mutex_unlock(&hba->uic_cmd_mutex);
>
> @@ -3812,16 +3821,20 @@ static void ufshcd_sl_intr(struct ufs_hba *hba, u32 intr_status)
> */
> static irqreturn_t ufshcd_intr(int irq, void *__hba)
> {
> - u32 intr_status;
> + u32 intr_status, enabled_intr_status;
> irqreturn_t retval = IRQ_NONE;
> struct ufs_hba *hba = __hba;
>
> spin_lock(hba->host->host_lock);
> intr_status = ufshcd_readl(hba, REG_INTERRUPT_STATUS);
> + enabled_intr_status =
> + intr_status & ufshcd_readl(hba, REG_INTERRUPT_ENABLE);

Is it better to store interrupt mask to new member field in ufs_hba
when ufshcd_{enable,disable}_intr() is called and avoid register
read every interrupt? Because register read is much slower than
normal memory read and we don't want to slow high IOPS workload.

>
> - if (intr_status) {
> + if (intr_status)
> ufshcd_writel(hba, intr_status, REG_INTERRUPT_STATUS);
> - ufshcd_sl_intr(hba, intr_status);
> +
> + if (enabled_intr_status) {
> + ufshcd_sl_intr(hba, enabled_intr_status);
> retval = IRQ_HANDLED;
> }
> spin_unlock(hba->host->host_lock);
> --
> 1.8.5.2
>
> --
> QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/