RE: [PATCH 1/2] scsi: ufs: Add DeepSleep feature
HI, > Drivers that wish to support DeepSleep need to set a new capability flag > UFSHCD_CAP_DEEPSLEEP and provide a hardware reset via the existing > ->device_reset() callback. I would expect that this capability controls sending SSU 4, but it only controls the sysfs entry? > > It is assumed that UFS devices with wspecversion >= 0x310 support > DeepSleep. > > The UFS specification says to set the IMMED (immediate) flag for the > Start/Stop Unit command when entering DeepSleep. However some UFS > devices object to that, which is addressed in a subsequent patch. After failing to understand what the proper behavior should be with respect of the IMMED bit, Although I read the applicable section few time, I gave up and consult our system guy, Which is our jedec representative. This is his answer: "... In order to avoid uncertainty - the host need to set IMMED bit to '0' (this is explicitly specified by the standard). The device responds only after it switches to Pre-DeepSleep state. The host then switch to H8 and this would trigger the device to transition to DeepSleep state. ..." So maybe the 2nd patch isn't really needed. Thanks, Avri
Re: [PATCH 1/2] scsi: ufs: Add DeepSleep feature
On 5/10/20 11:02 am, Avri Altman wrote: > HI, > >> Drivers that wish to support DeepSleep need to set a new capability flag >> UFSHCD_CAP_DEEPSLEEP and provide a hardware reset via the existing >> ->device_reset() callback. > I would expect that this capability controls sending SSU 4, but it only > controls the sysfs entry? The sysfs entry is the only way to request DeepSleep. > >> >> It is assumed that UFS devices with wspecversion >= 0x310 support >> DeepSleep. >> >> The UFS specification says to set the IMMED (immediate) flag for the >> Start/Stop Unit command when entering DeepSleep. However some UFS >> devices object to that, which is addressed in a subsequent patch. > After failing to understand what the proper behavior should be with respect > of the IMMED bit, > Although I read the applicable section few time, I gave up and consult our > system guy, > Which is our jedec representative. This is his answer: > "... > In order to avoid uncertainty - the host need to set IMMED bit to '0' (this > is explicitly specified by the standard). > The device responds only after it switches to Pre-DeepSleep state. The host > then switch to H8 and this would trigger the device to transition to > DeepSleep state. > ..." > > So maybe the 2nd patch isn't really needed. Yes I managed to get it the wrong way around! I will drop patch 2 and send V2 of patch 1 in due course.
RE: [PATCH 1/2] scsi: ufs: Add DeepSleep feature
> > > On 5/10/20 11:02 am, Avri Altman wrote: > > HI, > > > >> Drivers that wish to support DeepSleep need to set a new capability flag > >> UFSHCD_CAP_DEEPSLEEP and provide a hardware reset via the existing > >> ->device_reset() callback. > > I would expect that this capability controls sending SSU 4, but it only > > controls > the sysfs entry? > > The sysfs entry is the only way to request DeepSleep. Some chipset vendors use their own modules, or even the device tree to set their default rpm_lvl / spm_lvl. Thanks, Avri
Re: [PATCH 1/2] scsi: ufs: Add DeepSleep feature
On 5/10/20 12:51 pm, Avri Altman wrote: >> >> >> On 5/10/20 11:02 am, Avri Altman wrote: >>> HI, >>> Drivers that wish to support DeepSleep need to set a new capability flag UFSHCD_CAP_DEEPSLEEP and provide a hardware reset via the existing ->device_reset() callback. >>> I would expect that this capability controls sending SSU 4, but it only >>> controls >> the sysfs entry? >> >> The sysfs entry is the only way to request DeepSleep. > Some chipset vendors use their own modules, or even the device tree > to set their default rpm_lvl / spm_lvl. I would not expect them to set it wrongly but what are you suggesting?
RE: [PATCH 1/2] scsi: ufs: Add DeepSleep feature
> > On 5/10/20 12:51 pm, Avri Altman wrote: > >> > >> > >> On 5/10/20 11:02 am, Avri Altman wrote: > >>> HI, > >>> > Drivers that wish to support DeepSleep need to set a new capability flag > UFSHCD_CAP_DEEPSLEEP and provide a hardware reset via the existing > ->device_reset() callback. > >>> I would expect that this capability controls sending SSU 4, but it only > controls > >> the sysfs entry? > >> > >> The sysfs entry is the only way to request DeepSleep. > > Some chipset vendors use their own modules, or even the device tree > > to set their default rpm_lvl / spm_lvl. > > I would not expect them to set it wrongly but what are you suggesting? Right. Let's leave it as it is. Thanks, Avri
RE: [PATCH 1/2] scsi: ufs: Add DeepSleep feature
> + /* > +* DeepSleep requires the Immediate flag. DeepSleep state is actually > +* entered when the link state goes to Hibern8. > +*/ > + if (pwr_mode == UFS_DEEPSLEEP_PWR_MODE) > + cmd[1] = 1; Shouldn't it be bit1, i.e. cmd[1] = 2 ? > cmd[4] = pwr_mode << 4; >
RE: [PATCH 1/2] scsi: ufs: Add DeepSleep feature
Please ignore - I was confused with pre-fetch. Sorry, Avri > -Original Message- > From: Avri Altman > Sent: Sunday, October 4, 2020 10:21 AM > To: 'Adrian Hunter' ; Martin K . Petersen > ; James E . J . Bottomley > Cc: linux-s...@vger.kernel.org; linux-kernel@vger.kernel.org; Alim Akhtar > > Subject: RE: [PATCH 1/2] scsi: ufs: Add DeepSleep feature > > > + /* > > +* DeepSleep requires the Immediate flag. DeepSleep state is > > actually > > +* entered when the link state goes to Hibern8. > > +*/ > > + if (pwr_mode == UFS_DEEPSLEEP_PWR_MODE) > > + cmd[1] = 1; > Shouldn't it be bit1, i.e. cmd[1] = 2 ? > > > cmd[4] = pwr_mode << 4; > >
