lkml.org 
[lkml]   [2020]   [Nov]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH V4 1/2] scsi: ufs: Add DeepSleep feature
    On 2020-11-04 16:37, Adrian Hunter wrote:
    > On 4/11/20 10:29 am, Can Guo wrote:
    >> Hi Adrian,
    >>
    >> On 2020-11-03 22:14, Adrian Hunter wrote:
    >>> DeepSleep is a UFS v3.1 feature that achieves the lowest power
    >>> consumption
    >>> of the device, apart from power off.
    >>>
    >>> In DeepSleep mode, no commands are accepted, and the only way to exit
    >>> is
    >>> using a hardware reset or power cycle.
    >>>
    >>> This patch assumes that if a power cycle was an option, then power
    >>> off
    >>> would be preferable, so only exit via a hardware reset is supported.
    >>>
    >>> 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.
    >>>
    >>> It is assumed that UFS devices with wspecversion >= 0x310 support
    >>> DeepSleep.
    >>>
    >>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
    >>> ---
    >>>  Documentation/ABI/testing/sysfs-driver-ufs | 34 +++++++++++--------
    >>>  drivers/scsi/ufs/ufs-sysfs.c               |  7 ++++
    >>>  drivers/scsi/ufs/ufs.h                     |  1 +
    >>>  drivers/scsi/ufs/ufshcd.c                  | 39
    >>> ++++++++++++++++++++--
    >>>  drivers/scsi/ufs/ufshcd.h                  | 17 +++++++++-
    >>>  include/trace/events/ufs.h                 |  3 +-
    >>>  6 files changed, 83 insertions(+), 18 deletions(-)
    >>>
    >>> diff --git a/Documentation/ABI/testing/sysfs-driver-ufs
    >>> b/Documentation/ABI/testing/sysfs-driver-ufs
    >>> index adc0d0e91607..e77fa784d6d8 100644
    >>> --- a/Documentation/ABI/testing/sysfs-driver-ufs
    >>> +++ b/Documentation/ABI/testing/sysfs-driver-ufs
    >>> @@ -916,21 +916,24 @@ Date:        September 2014
    >>>  Contact:    Subhash Jadavani <subhashj@codeaurora.org>
    >>>  Description:    This entry could be used to set or show the UFS
    >>> device
    >>>          runtime power management level. The current driver
    >>> -        implementation supports 6 levels with next target states:
    >>> +        implementation supports 7 levels with next target states:
    >>>
    >>>          ==  ====================================================
    >>> -        0   an UFS device will stay active, an UIC link will
    >>> +        0   UFS device will stay active, UIC link will
    >>>              stay active
    >>> -        1   an UFS device will stay active, an UIC link will
    >>> +        1   UFS device will stay active, UIC link will
    >>>              hibernate
    >>> -        2   an UFS device will moved to sleep, an UIC link will
    >>> +        2   UFS device will be moved to sleep, UIC link will
    >>>              stay active
    >>> -        3   an UFS device will moved to sleep, an UIC link will
    >>> +        3   UFS device will be moved to sleep, UIC link will
    >>>              hibernate
    >>> -        4   an UFS device will be powered off, an UIC link will
    >>> +        4   UFS device will be powered off, UIC link will
    >>>              hibernate
    >>> -        5   an UFS device will be powered off, an UIC link will
    >>> +        5   UFS device will be powered off, UIC link will
    >>>              be powered off
    >>> +        6   UFS device will be moved to deep sleep, UIC link
    >>> +        will be powered off. Note, deep sleep might not be
    >>> +        supported in which case this value will not be accepted
    >>
    >> Nitpicking, usually higher spm/rpm_lvl means better power saving.
    >> Since POWERDOWN+LINKOFF achieves the lowest power consumption, can
    >> we put DEEPSLEEP_LINKOFF to 5, and POWERDOWN_LINKOFF to 6?
    >
    > That would break the API i.e. we shouldn't change the meaning of '5'
    >
    > Also, pedantically it depends on whether regulators are provided as to
    > whether Deep Sleep is higher/lower than PowerDown i.e. without
    > regulators to
    > switch off the device, it will remain in PowerDown mode which does not
    > save
    > as much power as Deep Sleep.
    >

    OK, that makes sense. The change LGTM, I gave my reivewed-by tag.

    Regards,

    Can Guo.

    >>
    >> Thanks,
    >>
    >> Can Guo.
    >>
    >>>          ==  ====================================================
    >>>
    >>>  What:        /sys/bus/platform/drivers/ufshcd/*/rpm_target_dev_state
    >>> @@ -954,21 +957,24 @@ Date:        September 2014
    >>>  Contact:    Subhash Jadavani <subhashj@codeaurora.org>
    >>>  Description:    This entry could be used to set or show the UFS
    >>> device
    >>>          system power management level. The current driver
    >>> -        implementation supports 6 levels with next target states:
    >>> +        implementation supports 7 levels with next target states:
    >>>
    >>>          ==  ====================================================
    >>> -        0   an UFS device will stay active, an UIC link will
    >>> +        0   UFS device will stay active, UIC link will
    >>>              stay active
    >>> -        1   an UFS device will stay active, an UIC link will
    >>> +        1   UFS device will stay active, UIC link will
    >>>              hibernate
    >>> -        2   an UFS device will moved to sleep, an UIC link will
    >>> +        2   UFS device will be moved to sleep, UIC link will
    >>>              stay active
    >>> -        3   an UFS device will moved to sleep, an UIC link will
    >>> +        3   UFS device will be moved to sleep, UIC link will
    >>>              hibernate
    >>> -        4   an UFS device will be powered off, an UIC link will
    >>> +        4   UFS device will be powered off, UIC link will
    >>>              hibernate
    >>> -        5   an UFS device will be powered off, an UIC link will
    >>> +        5   UFS device will be powered off, UIC link will
    >>>              be powered off
    >>> +        6   UFS device will be moved to deep sleep, UIC link
    >>> +        will be powered off. Note, deep sleep might not be
    >>> +        supported in which case this value will not be accepted
    >>>          ==  ====================================================
    >>>
    >>>  What:        /sys/bus/platform/drivers/ufshcd/*/spm_target_dev_state
    >>> diff --git a/drivers/scsi/ufs/ufs-sysfs.c
    >>> b/drivers/scsi/ufs/ufs-sysfs.c
    >>> index bdcd27faa054..08e72b7eef6a 100644
    >>> --- a/drivers/scsi/ufs/ufs-sysfs.c
    >>> +++ b/drivers/scsi/ufs/ufs-sysfs.c
    >>> @@ -28,6 +28,7 @@ static const char
    >>> *ufschd_ufs_dev_pwr_mode_to_string(
    >>>      case UFS_ACTIVE_PWR_MODE:    return "ACTIVE";
    >>>      case UFS_SLEEP_PWR_MODE:    return "SLEEP";
    >>>      case UFS_POWERDOWN_PWR_MODE:    return "POWERDOWN";
    >>> +    case UFS_DEEPSLEEP_PWR_MODE:    return "DEEPSLEEP";
    >>>      default:            return "UNKNOWN";
    >>>      }
    >>>  }
    >>> @@ -38,6 +39,7 @@ static inline ssize_t ufs_sysfs_pm_lvl_store(struct
    >>> device *dev,
    >>>                           bool rpm)
    >>>  {
    >>>      struct ufs_hba *hba = dev_get_drvdata(dev);
    >>> +    struct ufs_dev_info *dev_info = &hba->dev_info;
    >>>      unsigned long flags, value;
    >>>
    >>>      if (kstrtoul(buf, 0, &value))
    >>> @@ -46,6 +48,11 @@ static inline ssize_t
    >>> ufs_sysfs_pm_lvl_store(struct
    >>> device *dev,
    >>>      if (value >= UFS_PM_LVL_MAX)
    >>>          return -EINVAL;
    >>>
    >>> +    if (ufs_pm_lvl_states[value].dev_state == UFS_DEEPSLEEP_PWR_MODE
    >>> &&
    >>> +        (!(hba->caps & UFSHCD_CAP_DEEPSLEEP) ||
    >>> +         !(dev_info->wspecversion >= 0x310)))
    >>> +        return -EINVAL;
    >>> +
    >>>      spin_lock_irqsave(hba->host->host_lock, flags);
    >>>      if (rpm)
    >>>          hba->rpm_lvl = value;
    >>> diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
    >>> index f8ab16f30fdc..d593edb48767 100644
    >>> --- a/drivers/scsi/ufs/ufs.h
    >>> +++ b/drivers/scsi/ufs/ufs.h
    >>> @@ -442,6 +442,7 @@ enum ufs_dev_pwr_mode {
    >>>      UFS_ACTIVE_PWR_MODE    = 1,
    >>>      UFS_SLEEP_PWR_MODE    = 2,
    >>>      UFS_POWERDOWN_PWR_MODE    = 3,
    >>> +    UFS_DEEPSLEEP_PWR_MODE    = 4,
    >>>  };
    >>>
    >>>  #define UFS_WB_BUF_REMAIN_PERCENT(val) ((val) / 10)
    >>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
    >>> index 2309253d3101..ee083b96e405 100644
    >>> --- a/drivers/scsi/ufs/ufshcd.c
    >>> +++ b/drivers/scsi/ufs/ufshcd.c
    >>> @@ -163,6 +163,11 @@ struct ufs_pm_lvl_states ufs_pm_lvl_states[] = {
    >>>      {UFS_SLEEP_PWR_MODE, UIC_LINK_HIBERN8_STATE},
    >>>      {UFS_POWERDOWN_PWR_MODE, UIC_LINK_HIBERN8_STATE},
    >>>      {UFS_POWERDOWN_PWR_MODE, UIC_LINK_OFF_STATE},
    >>> +    /*
    >>> +     * For DeepSleep, the link is first put in hibern8 and then off.
    >>> +     * Leaving the link in hibern8 is not supported.
    >>> +     */
    >>> +    {UFS_DEEPSLEEP_PWR_MODE, UIC_LINK_OFF_STATE},
    >>>  };
    >>>
    >>>  static inline enum ufs_dev_pwr_mode
    >>> @@ -8297,7 +8302,8 @@ static int ufshcd_link_state_transition(struct
    >>> ufs_hba *hba,
    >>>      }
    >>>      /*
    >>>       * If autobkops is enabled, link can't be turned off because
    >>> -     * turning off the link would also turn off the device.
    >>> +     * turning off the link would also turn off the device, except
    >>> in the
    >>> +     * case of DeepSleep where the device is expected to remain
    >>> powered.
    >>>       */
    >>>      else if ((req_link_state == UIC_LINK_OFF_STATE) &&
    >>>           (!check_for_bkops || !hba->auto_bkops_enabled)) {
    >>> @@ -8307,6 +8313,9 @@ static int ufshcd_link_state_transition(struct
    >>> ufs_hba *hba,
    >>>           * put the link in low power mode is to send the DME end
    >>> point
    >>>           * to device and then send the DME reset command to local
    >>>           * unipro. But putting the link in hibern8 is much faster.
    >>> +         *
    >>> +         * Note also that putting the link in Hibern8 is a
    >>> requirement
    >>> +         * for entering DeepSleep.
    >>>           */
    >>>          ret = ufshcd_uic_hibern8_enter(hba);
    >>>          if (ret) {
    >>> @@ -8439,6 +8448,7 @@ static void ufshcd_hba_vreg_set_hpm(struct
    >>> ufs_hba
    >>> *hba)
    >>>  static int ufshcd_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op)
    >>>  {
    >>>      int ret = 0;
    >>> +    int check_for_bkops;
    >>>      enum ufs_pm_level pm_lvl;
    >>>      enum ufs_dev_pwr_mode req_dev_pwr_mode;
    >>>      enum uic_link_state req_link_state;
    >>> @@ -8524,7 +8534,13 @@ static int ufshcd_suspend(struct ufs_hba *hba,
    >>> enum ufs_pm_op pm_op)
    >>>      }
    >>>
    >>>      flush_work(&hba->eeh_work);
    >>> -    ret = ufshcd_link_state_transition(hba, req_link_state, 1);
    >>> +
    >>> +    /*
    >>> +     * In the case of DeepSleep, the device is expected to remain
    >>> powered
    >>> +     * with the link off, so do not check for bkops.
    >>> +     */
    >>> +    check_for_bkops = !ufshcd_is_ufs_dev_deepsleep(hba);
    >>> +    ret = ufshcd_link_state_transition(hba, req_link_state,
    >>> check_for_bkops);
    >>>      if (ret)
    >>>          goto set_dev_active;
    >>>
    >>> @@ -8565,11 +8581,25 @@ static int ufshcd_suspend(struct ufs_hba
    >>> *hba,
    >>> enum ufs_pm_op pm_op)
    >>>      if (hba->clk_scaling.is_allowed)
    >>>          ufshcd_resume_clkscaling(hba);
    >>>      ufshcd_vreg_set_hpm(hba);
    >>> +    /*
    >>> +     * Device hardware reset is required to exit DeepSleep. Also,
    >>> for
    >>> +     * DeepSleep, the link is off so host reset and restore will be
    >>> done
    >>> +     * further below.
    >>> +     */
    >>> +    if (ufshcd_is_ufs_dev_deepsleep(hba)) {
    >>> +        ufshcd_vops_device_reset(hba);
    >>> +        WARN_ON(!ufshcd_is_link_off(hba));
    >>> +    }
    >>>      if (ufshcd_is_link_hibern8(hba) &&
    >>> !ufshcd_uic_hibern8_exit(hba))
    >>>          ufshcd_set_link_active(hba);
    >>>      else if (ufshcd_is_link_off(hba))
    >>>          ufshcd_host_reset_and_restore(hba);
    >>>  set_dev_active:
    >>> +    /* Can also get here needing to exit DeepSleep */
    >>> +    if (ufshcd_is_ufs_dev_deepsleep(hba)) {
    >>> +        ufshcd_vops_device_reset(hba);
    >>> +        ufshcd_host_reset_and_restore(hba);
    >>> +    }
    >>>      if (!ufshcd_set_dev_pwr_mode(hba, UFS_ACTIVE_PWR_MODE))
    >>>          ufshcd_disable_auto_bkops(hba);
    >>>  enable_gating:
    >>> @@ -8631,6 +8661,9 @@ static int ufshcd_resume(struct ufs_hba *hba,
    >>> enum ufs_pm_op pm_op)
    >>>      if (ret)
    >>>          goto disable_vreg;
    >>>
    >>> +    /* For DeepSleep, the only supported option is to have the link
    >>> off */
    >>> +    WARN_ON(ufshcd_is_ufs_dev_deepsleep(hba) &&
    >>> !ufshcd_is_link_off(hba));
    >>> +
    >>>      if (ufshcd_is_link_hibern8(hba)) {
    >>>          ret = ufshcd_uic_hibern8_exit(hba);
    >>>          if (!ret) {
    >>> @@ -8644,6 +8677,8 @@ static int ufshcd_resume(struct ufs_hba *hba,
    >>> enum ufs_pm_op pm_op)
    >>>          /*
    >>>           * A full initialization of the host and the device is
    >>>           * required since the link was put to off during suspend.
    >>> +         * Note, in the case of DeepSleep, the device will exit
    >>> +         * DeepSleep due to device reset.
    >>>           */
    >>>          ret = ufshcd_reset_and_restore(hba);
    >>>          /*
    >>> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
    >>> index 0fbb735bb70c..213be0667b59 100644
    >>> --- a/drivers/scsi/ufs/ufshcd.h
    >>> +++ b/drivers/scsi/ufs/ufshcd.h
    >>> @@ -114,16 +114,22 @@ enum uic_link_state {
    >>>      ((h)->curr_dev_pwr_mode = UFS_SLEEP_PWR_MODE)
    >>>  #define ufshcd_set_ufs_dev_poweroff(h) \
    >>>      ((h)->curr_dev_pwr_mode = UFS_POWERDOWN_PWR_MODE)
    >>> +#define ufshcd_set_ufs_dev_deepsleep(h) \
    >>> +    ((h)->curr_dev_pwr_mode = UFS_DEEPSLEEP_PWR_MODE)
    >>>  #define ufshcd_is_ufs_dev_active(h) \
    >>>      ((h)->curr_dev_pwr_mode == UFS_ACTIVE_PWR_MODE)
    >>>  #define ufshcd_is_ufs_dev_sleep(h) \
    >>>      ((h)->curr_dev_pwr_mode == UFS_SLEEP_PWR_MODE)
    >>>  #define ufshcd_is_ufs_dev_poweroff(h) \
    >>>      ((h)->curr_dev_pwr_mode == UFS_POWERDOWN_PWR_MODE)
    >>> +#define ufshcd_is_ufs_dev_deepsleep(h) \
    >>> +    ((h)->curr_dev_pwr_mode == UFS_DEEPSLEEP_PWR_MODE)
    >>>
    >>>  /*
    >>>   * UFS Power management levels.
    >>> - * Each level is in increasing order of power savings.
    >>> + * Each level is in increasing order of power savings, except
    >>> DeepSleep
    >>> + * which is lower than PowerDown with power on but not PowerDown
    >>> with
    >>> + * power off.
    >>>   */
    >>>  enum ufs_pm_level {
    >>>      UFS_PM_LVL_0, /* UFS_ACTIVE_PWR_MODE, UIC_LINK_ACTIVE_STATE */
    >>> @@ -132,6 +138,7 @@ enum ufs_pm_level {
    >>>      UFS_PM_LVL_3, /* UFS_SLEEP_PWR_MODE, UIC_LINK_HIBERN8_STATE */
    >>>      UFS_PM_LVL_4, /* UFS_POWERDOWN_PWR_MODE, UIC_LINK_HIBERN8_STATE
    >>> */
    >>>      UFS_PM_LVL_5, /* UFS_POWERDOWN_PWR_MODE, UIC_LINK_OFF_STATE */
    >>> +    UFS_PM_LVL_6, /* UFS_DEEPSLEEP_PWR_MODE, UIC_LINK_OFF_STATE */
    >>>      UFS_PM_LVL_MAX
    >>>  };
    >>>
    >>> @@ -599,6 +606,14 @@ enum ufshcd_caps {
    >>>       * This would increase power savings.
    >>>       */
    >>>      UFSHCD_CAP_AGGR_POWER_COLLAPSE            = 1 << 9,
    >>> +
    >>> +    /*
    >>> +     * This capability allows the host controller driver to use
    >>> DeepSleep,
    >>> +     * if it is supported by the UFS device. The host controller
    >>> driver must
    >>> +     * support device hardware reset via the hba->device_reset()
    >>> callback,
    >>> +     * in order to exit DeepSleep state.
    >>> +     */
    >>> +    UFSHCD_CAP_DEEPSLEEP                = 1 << 10,
    >>>  };
    >>>
    >>>  struct ufs_hba_variant_params {
    >>> diff --git a/include/trace/events/ufs.h b/include/trace/events/ufs.h
    >>> index 84841b3a7ffd..2362244c2a9e 100644
    >>> --- a/include/trace/events/ufs.h
    >>> +++ b/include/trace/events/ufs.h
    >>> @@ -19,7 +19,8 @@
    >>>  #define UFS_PWR_MODES            \
    >>>      EM(UFS_ACTIVE_PWR_MODE)        \
    >>>      EM(UFS_SLEEP_PWR_MODE)        \
    >>> -    EMe(UFS_POWERDOWN_PWR_MODE)
    >>> +    EM(UFS_POWERDOWN_PWR_MODE)    \
    >>> +    EMe(UFS_DEEPSLEEP_PWR_MODE)
    >>>
    >>>  #define UFSCHD_CLK_GATING_STATES    \
    >>>      EM(CLKS_OFF)            \

    \
     
     \ /
      Last update: 2020-11-04 10:05    [W:7.015 / U:0.028 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site