Re: [PATCH v3 5/6] scsi: ufs: add operation for the uic power mode change

[Subhash Jadavani <subhashj@codeaurora.org>]

On 8/26/2013 8:10 PM, Seungwon Jeon wrote:
> Setting PA_PWRMode using DME_SET triggers the power mode
> change. And then the result will be given by the HCS.UPMCRS.
> This operation should be done atomically.
>
> Signed-off-by: Seungwon Jeon <tgih.jun@samsung.com>
> ---
>   drivers/scsi/ufs/ufshcd.c |   84 ++++++++++++++++++++++++++++++++++++++++++--
>   drivers/scsi/ufs/ufshcd.h |    3 ++
>   drivers/scsi/ufs/ufshci.h |   12 ++++++
>   3 files changed, 95 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 00bfc1a..c67118d 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -36,9 +36,11 @@
>   #include <linux/async.h>
>   
>   #include "ufshcd.h"
> +#include "unipro.h"
>   
>   #define UFSHCD_ENABLE_INTRS	(UTP_TRANSFER_REQ_COMPL |\
>   				 UTP_TASK_REQ_COMPL |\
> +				 UIC_POWER_MODE |\
>   				 UFSHCD_ERROR_MASK)
>   /* UIC command timeout, unit: ms */
>   #define UIC_CMD_TIMEOUT	500
> @@ -520,6 +522,18 @@ static inline bool ufshcd_ready_for_uic_cmd(struct ufs_hba *hba)
>   }
>   
>   /**
> + * ufshcd_get_upmcrs - Get the power mode change request status
> + * @hba: Pointer to adapter instance
> + *
> + * This function gets the UPMCRS field of HCS register
> + * Returns value of UPMCRS field
> + */
> +static inline u8 ufshcd_get_upmcrs(struct ufs_hba *hba)
> +{
> +	return (ufshcd_readl(hba, REG_CONTROLLER_STATUS) >> 8) & 0x7;
> +}
> +
> +/**
>    * ufshcd_dispatch_uic_cmd - Dispatch UIC commands to unipro layers
>    * @hba: per adapter instance
>    * @uic_cmd: UIC command
> @@ -1526,6 +1540,64 @@ out:
>   EXPORT_SYMBOL_GPL(ufshcd_dme_get_attr);
>   
>   /**
> + * ufshcd_uic_change_pwr_mode - Perform the UIC power mode chage
> + *				using DME_SET primitives.
> + * @hba: per adapter instance
> + * @mode: powr mode value
> + *
> + * Returns 0 on success, non-zero value on failure
> + */
> +int ufshcd_uic_change_pwr_mode(struct ufs_hba *hba, u8 mode)
> +{
> +	struct uic_command uic_cmd = {0};
> +	struct completion pwr_done;
> +	unsigned long flags;
> +	u8 status;
> +	int ret;
> +
> +	uic_cmd.command = UIC_CMD_DME_SET;
> +	uic_cmd.argument1 = UIC_ARG_MIB(PA_PWRMODE);
> +	uic_cmd.argument3 = mode;
> +	init_completion(&pwr_done);
> +
> +	mutex_lock(&hba->uic_cmd_mutex);
> +
> +	spin_lock_irqsave(hba->host->host_lock, flags);
> +	hba->pwr_done = &pwr_done;
> +	spin_unlock_irqrestore(hba->host->host_lock, flags);
> +	ret = __ufshcd_send_uic_cmd(hba, &uic_cmd);
> +	if (ret) {
> +		dev_err(hba->dev,
> +			"pwr mode change with mode 0x%x uic error %d\n",
> +			mode, ret);
> +		goto out;
> +	}
> +
> +	if (!wait_for_completion_timeout(hba->pwr_done,
> +					 msecs_to_jiffies(UIC_CMD_TIMEOUT))) {
> +		dev_err(hba->dev,
> +			"pwr mode change with mode 0x%x completion timeout\n",
> +			mode);
> +		ret = -ETIMEDOUT;
> +		goto out;
> +	}
> +
> +	status = ufshcd_get_upmcrs(hba);

I have a doubt on the relation between UIC Power Mode Status (UPMS) bit 
& UIC Power Mode Change Request Status (UPMCRS) field.

Once we send the DME_SET(PA_PWRMODE) command, we will get the UIC 
command completion interrupt (with IS.UCCS bit set) which just indicate 
that the attribute is written but power mode change procedure will 
complete only after some delay which is indicated via IS.UPMS status bit 
and then we would read the HCS.UPMCRS field to know the status of power 
mode change.

This is my exact doubt: Currently UFSHCD driver first clears the IS.UPMS 
bit (ufshcd_intr() first clears the current interrupts in 
REG_INTERRUPT_STATUS and then calls the ufshcd_sl_intr() handler, and in 
your current patch we are anyway not reading the UPMCRS field from 
interrupt contex ) and then read the HCS.UPMCRS field. Will HCS.UPMCRS 
field retain its status value even after IS.UPMS bit is cleared? UFSHCI 
specification doesn't clearly mention this so either we have to get the 
clarity from JEDEC or from your ufs host controller vendor on the 
behaviour. I am checking with our ufs host controller vendor as well and 
will let you know what is the expected behaviour. BTW, i guess you might 
have already tested this patch to be working on your UFS host controller?

> +	if (status != PWR_LOCAL) {
> +		dev_err(hba->dev,
> +			"pwr mode change failed, host umpcrs:0x%x\n",
> +			status);
> +		ret = (status != PWR_OK) ? status : -1;
> +	}
> +out:
> +	spin_lock_irqsave(hba->host->host_lock, flags);
> +	hba->pwr_done = NULL;
> +	spin_unlock_irqrestore(hba->host->host_lock, flags);
> +	mutex_unlock(&hba->uic_cmd_mutex);
> +	return ret;
> +}
> +
> +/**
>    * ufshcd_complete_dev_init() - checks device readiness
>    * hba: per-adapter instance
>    *
> @@ -1992,16 +2064,20 @@ ufshcd_transfer_rsp_status(struct ufs_hba *hba, struct ufshcd_lrb *lrbp)
>   /**
>    * ufshcd_uic_cmd_compl - handle completion of uic command
>    * @hba: per adapter instance
> + * @intr_status: interrupt status generated by the controller
>    */
> -static void ufshcd_uic_cmd_compl(struct ufs_hba *hba)
> +static void ufshcd_uic_cmd_compl(struct ufs_hba *hba, u32 intr_status)
>   {
> -	if (hba->active_uic_cmd) {
> +	if ((intr_status & UIC_COMMAND_COMPL) && hba->active_uic_cmd) {
>   		hba->active_uic_cmd->argument2 |=
>   			ufshcd_get_uic_cmd_result(hba);
>   		hba->active_uic_cmd->argument3 =
>   			ufshcd_get_dme_attr_val(hba);
>   		complete(&hba->active_uic_cmd->done);
>   	}
> +
> +	if ((intr_status & UIC_POWER_MODE) && hba->pwr_done)
> +		complete(hba->pwr_done);
>   }
>   
>   /**
> @@ -2451,8 +2527,8 @@ static void ufshcd_sl_intr(struct ufs_hba *hba, u32 intr_status)
>   	if (hba->errors)
>   		ufshcd_check_errors(hba);
>   
> -	if (intr_status & UIC_COMMAND_COMPL)
> -		ufshcd_uic_cmd_compl(hba);
> +	if (intr_status & UFSHCD_UIC_MASK)
> +		ufshcd_uic_cmd_compl(hba, intr_status);
>   
>   	if (intr_status & UTP_TASK_REQ_COMPL)
>   		ufshcd_tmc_handler(hba);
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> index ab1518d..ecff83d 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -178,6 +178,7 @@ struct ufs_dev_cmd {
>    * @tm_tag_wq: wait queue for free task management slots
>    * @tm_condition: condition variable for task management
>    * @tm_slots_in_use: bit map of task management request slots in use
> + * @pwr_done: completion for power mode change
>    * @ufshcd_state: UFSHCD states
>    * @eh_flags: Error handling flags
>    * @intr_mask: Interrupt Mask Bits
> @@ -227,6 +228,8 @@ struct ufs_hba {
>   	unsigned long tm_condition;
>   	unsigned long tm_slots_in_use;
>   
> +	struct completion *pwr_done;
> +
>   	u32 ufshcd_state;
>   	u32 eh_flags;
>   	u32 intr_mask;
> diff --git a/drivers/scsi/ufs/ufshci.h b/drivers/scsi/ufs/ufshci.h
> index 1e1fe26..0475c66 100644
> --- a/drivers/scsi/ufs/ufshci.h
> +++ b/drivers/scsi/ufs/ufshci.h
> @@ -124,6 +124,9 @@ enum {
>   #define CONTROLLER_FATAL_ERROR			UFS_BIT(16)
>   #define SYSTEM_BUS_FATAL_ERROR			UFS_BIT(17)
>   
> +#define UFSHCD_UIC_MASK		(UIC_COMMAND_COMPL |\
> +				 UIC_POWER_MODE)
> +
>   #define UFSHCD_ERROR_MASK	(UIC_ERROR |\
>   				DEVICE_FATAL_ERROR |\
>   				CONTROLLER_FATAL_ERROR |\
> @@ -142,6 +145,15 @@ enum {
>   #define DEVICE_ERROR_INDICATOR			UFS_BIT(5)
>   #define UIC_POWER_MODE_CHANGE_REQ_STATUS_MASK	UFS_MASK(0x7, 8)
>   
> +enum {
> +	PWR_OK		= 0x0,
> +	PWR_LOCAL	= 0x01,
> +	PWR_REMOTE	= 0x02,
> +	PWR_BUSY	= 0x03,
> +	PWR_ERROR_CAP	= 0x04,
> +	PWR_FATAL_ERROR	= 0x05,
> +};
> +
>   /* HCE - Host Controller Enable 34h */
>   #define CONTROLLER_ENABLE	UFS_BIT(0)
>   #define CONTROLLER_DISABLE	0x0