Re: [PATCH v1 1/2] scsi: ufs: set device as default active power mode during initialization only

From: Stanley Chu <stanley.chu@mediatek.com>
To: Can Guo <cang@codeaurora.org>
Cc: <linux-scsi-owner@vger.kernel.org>, <linux-scsi@vger.kernel.org>,
	<martin.petersen@oracle.com>, <subhashj@codeaurora.org>,
	<jejb@linux.ibm.com>, <chun-hung.wu@mediatek.com>,
	<kuohong.wang@mediatek.com>, <linux-kernel@vger.kernel.org>,
	<stable@vger.kernel.org>, <avri.altman@wdc.com>,
	<linux-mediatek@lists.infradead.org>, <peter.wang@mediatek.com>,
	<alim.akhtar@samsung.com>, <andy.teng@mediatek.com>,
	<matthias.bgg@gmail.com>, <beanhuo@micron.com>,
	<pedrom.sousa@synopsys.com>, <bvanassche@acm.org>,
	<linux-arm-kernel@lists.infradead.org>, <asutoshd@codeaurora.org>
Subject: Re: [PATCH v1 1/2] scsi: ufs: set device as default active power mode during initialization only
Date: Tue, 31 Dec 2019 15:44:50 +0800	[thread overview]
Message-ID: <1577778290.13164.45.camel@mtkswgap22> (raw)
In-Reply-To: <1577766179.13164.24.camel@mtkswgap22>

Hi Can,

On Tue, 2019-12-31 at 12:22 +0800, Stanley Chu wrote:
> Hi Can,
> 
> 
> > Hi Stanley,
> > 
> > I see skipping ufshcd_set_ufs_dev_active() in ufshcd_probe_hba()
> > if it is called from ufshcd_resume() path is the purpose here.
> > 
> > If so, then ufshcd_set_dev_pwr_mode() would be called, meaning
> > SSU command will be sent. Why is this SSU command needed to be
> > sent after a full host reset and restore? Is ufshcd_probe_hba()
> > not enough to make UFS device fully functional?
> 
> After resume (for both runtime resume and system resume), device power
> mode shall be back to "Active" to service incoming requests.
> 
> I see two cases need device power mode transition during resume flow
> 1. Device Power Mode = Sleep
> 2. Device Power Mode = PowerDown
> 
> For 1, ufshcd_probe_hba() is not invoked during resume flow,
> hba->curr_dev_pwr_mode = SLEEP, thus ufshcd_resume() can invoke
> ufshcd_set_dev_pwr_mode() to change device power mode.
> 
> For 2, ufshcd_probe_hba() is invoked during resume flow, before this
> fix, hba->curr_dev_pwr_mode will be set to ACTIVE (note that only this
> flag is set as ACTIVE, but device may be still in PowerDown state if
> device power is not fully shutdown or device HW reset is not executed
> before resume), in the end, ufshcd_resume() will not invoke
> ufshcd_set_dev_pwr_mode() to send SSU command to make device change back
> to Active power mode.
> 
> But if device is fully reset before resume (not by current mainstream
> driver), device can be already in "Active" power mode after power on
> again during resume flow. In this case, it is OK to set
> hba->curr_dev_pwr_mode as ACTIVE in ufshcd_probe_hba() and SSU command
> is not necessary.
> 
> Thanks,
> Stanley

I think currently the assumption in ufshcd_probe_hba() that "device
shall be already in Active power mode" makes sense because many device
commands will be sent to device in ufshcd_probe_hba() but device is not
promised to handle those commands in PowerDown power mode according to
UFS spec. 

So, maybe always ensuring device being Active power mode before leaving
ufshcd_probe_hba() is more reasonable. If so, I will drop this patch
first.

Thanks so much for your reviews.

Happy new year!

Thanks,
Stanley