| Message ID | 20201002124043.25394-1-adrian.hunter@intel.com |
|---|---|
| Headers | show |
| Series | scsi: ufs: Add DeepSleep feature | expand |
> + /* > + * 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; >
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' <adrian.hunter@intel.com>; Martin K . Petersen > <martin.petersen@oracle.com>; James E . J . Bottomley <jejb@linux.ibm.com> > Cc: linux-scsi@vger.kernel.org; linux-kernel@vger.kernel.org; Alim Akhtar > <alim.akhtar@samsung.com> > 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; > >
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
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.
> > > 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
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?
> > 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
Hi Here is a patch to add DeepSleep, and a patch to workaround an issue hit when testing. Adrian Hunter (2): scsi: ufs: Add DeepSleep feature scsi: ufs: Workaround UFS devices that object to DeepSleep IMMED drivers/scsi/ufs/ufs-sysfs.c | 7 ++++ drivers/scsi/ufs/ufs.h | 1 + drivers/scsi/ufs/ufshcd.c | 86 +++++++++++++++++++++++++++++++++++++++++--- drivers/scsi/ufs/ufshcd.h | 28 ++++++++++++++- include/trace/events/ufs.h | 3 +- 5 files changed, 119 insertions(+), 6 deletions(-) Regards Adrian