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

[Subhash Jadavani <subhashj@codeaurora.org>]

On 8/27/2013 4:58 PM, Seungwon Jeon wrote:
> On Tue, August 27, 2013, Subhash Jadavani wrote:
>> 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?
> Hi Subhash,
>
> Yes, It's already tested and works fine.
> IS.UPMS is a indication to inform the result of 'power mode change'(UPMCRS).
> So clearing the interrupt would not affect this result.
Thanks for clarifying.

> Generally it seems not common idea.

Infact on SDHCi controller, there is a case where "auto-cmd error 
status" register fields are only valid when the "auto-cmd error" 
interrupt bit is set in interrupt status register. Clearing status bit 
also clears the "auto-cmd error status" register fields automatically. 
So i was in doubt whether similar could be the case here or not.

> Anyway, if your host has different behavior, please let me know.

Sure, checking on this. Would let you know soon. BTW, UFSHCI 
specification doesn't mention it clearly so it could be implementation 
dependent as well and if it is the case, we might have to change it 
later to accomodate the different implementation. So for now, you have 
my Reviewed-by: Subhash Jadavani <subhashj@codeaurora.org>

>
> Thanks,
> Seungwon Jeon
>
>>> +	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
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html