* [<RFC PATCH v1> 1/2] scsi: ufs: add write booster feature support [not found] <cover.1582330473.git.asutoshd@codeaurora.org> @ 2020-02-22 0:47 ` Asutosh Das 2020-02-23 9:03 ` Avri Altman ` (2 more replies) 2020-02-22 0:47 ` [<RFC PATCH v1> 2/2] ufs-qcom: scsi: configure write booster type Asutosh Das 1 sibling, 3 replies; 9+ messages in thread From: Asutosh Das @ 2020-02-22 0:47 UTC (permalink / raw) To: subhashj, cang, rnayak, vinholikatti, jejb, martin.petersen, linux-scsi Cc: Asutosh Das, linux-arm-msm, Alim Akhtar, Avri Altman, James E.J. Bottomley, Stanley Chu, Bean Huo, Tomas Winkler, Colin Ian King, Bart Van Assche, Venkat Gopalakrishnan, Bjorn Andersson, open list Write Booster enable: SW will enable the feature when clocks are scaled up. Once clocks and gear are scaled up, SW will set “fWriteBoosterEn” flag before exiting from ufshcd_devfreq_scale() function. Write Booster disable: SW will disable the feature, when clocks are scaled down. Once clocks and gear are scaled down, SW will clear “fWriteBoosterEn” flag before exiting from ufshcd_devfreq_scale() function. Write Booster Buffer flush will be enabled in following cases: 1. During runtime suspend, driver would query bAvailableWriteBoosterBufferSize attribute and enable the Write Booster Buffer Flush if less than 30% Write Booster Buffer is available. Write Booster Buffer flush will be disabled in following case: During runtime suspend, if available buffer size is >30%, then Write Booster Flush will be stopped by clearing fWriteBoosterBufferFlushEn flag. In non-reduction case, during runtime-suspend, dCurrentWriteBoosterBufferSize attribute would determine if flush is possible. If this value is > 0 and availBuf is less than 30% then flush is needed and vcc needs to be ON. Signed-off-by: Asutosh Das <asutoshd@codeaurora.org> Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org> --- drivers/scsi/ufs/ufs-sysfs.c | 41 ++++++- drivers/scsi/ufs/ufs.h | 61 ++++++++- drivers/scsi/ufs/ufshcd.c | 286 ++++++++++++++++++++++++++++++++++++++++++- drivers/scsi/ufs/ufshcd.h | 9 ++ 4 files changed, 390 insertions(+), 7 deletions(-) diff --git a/drivers/scsi/ufs/ufs-sysfs.c b/drivers/scsi/ufs/ufs-sysfs.c index dbdf8b0..4505e27 100644 --- a/drivers/scsi/ufs/ufs-sysfs.c +++ b/drivers/scsi/ufs/ufs-sysfs.c @@ -274,6 +274,10 @@ UFS_DEVICE_DESC_PARAM(device_version, _DEV_VER, 2); UFS_DEVICE_DESC_PARAM(number_of_secure_wpa, _NUM_SEC_WPA, 1); UFS_DEVICE_DESC_PARAM(psa_max_data_size, _PSA_MAX_DATA, 4); UFS_DEVICE_DESC_PARAM(psa_state_timeout, _PSA_TMT, 1); +UFS_DEVICE_DESC_PARAM(ext_feature_sup, _EXT_UFS_FEATURE_SUP, 4); +UFS_DEVICE_DESC_PARAM(wb_presv_us_en, _WB_US_RED_EN, 1); +UFS_DEVICE_DESC_PARAM(wb_type, _WB_TYPE, 1); +UFS_DEVICE_DESC_PARAM(wb_shared_alloc_units, _WB_SHARED_ALLOC_UNITS, 4); static struct attribute *ufs_sysfs_device_descriptor[] = { &dev_attr_device_type.attr, @@ -302,6 +306,10 @@ static struct attribute *ufs_sysfs_device_descriptor[] = { &dev_attr_number_of_secure_wpa.attr, &dev_attr_psa_max_data_size.attr, &dev_attr_psa_state_timeout.attr, + &dev_attr_ext_feature_sup.attr, + &dev_attr_wb_presv_us_en.attr, + &dev_attr_wb_type.attr, + &dev_attr_wb_shared_alloc_units.attr, NULL, }; @@ -371,6 +379,12 @@ UFS_GEOMETRY_DESC_PARAM(enh4_memory_max_alloc_units, _ENM4_MAX_NUM_UNITS, 4); UFS_GEOMETRY_DESC_PARAM(enh4_memory_capacity_adjustment_factor, _ENM4_CAP_ADJ_FCTR, 2); +UFS_GEOMETRY_DESC_PARAM(wb_max_alloc_units, _WB_MAX_ALLOC_UNITS, 4); +UFS_GEOMETRY_DESC_PARAM(wb_max_wb_luns, _WB_MAX_WB_LUNS, 1); +UFS_GEOMETRY_DESC_PARAM(wb_buff_cap_adj, _WB_BUFF_CAP_ADJ, 1); +UFS_GEOMETRY_DESC_PARAM(wb_sup_red_type, _WB_SUP_RED_TYPE, 1); +UFS_GEOMETRY_DESC_PARAM(wb_sup_wb_type, _WB_SUP_WB_TYPE, 1); + static struct attribute *ufs_sysfs_geometry_descriptor[] = { &dev_attr_raw_device_capacity.attr, @@ -402,6 +416,11 @@ static struct attribute *ufs_sysfs_geometry_descriptor[] = { &dev_attr_enh3_memory_capacity_adjustment_factor.attr, &dev_attr_enh4_memory_max_alloc_units.attr, &dev_attr_enh4_memory_capacity_adjustment_factor.attr, + &dev_attr_wb_max_alloc_units.attr, + &dev_attr_wb_max_wb_luns.attr, + &dev_attr_wb_buff_cap_adj.attr, + &dev_attr_wb_sup_red_type.attr, + &dev_attr_wb_sup_wb_type.attr, NULL, }; @@ -608,7 +627,7 @@ static ssize_t _name##_show(struct device *dev, \ if (ufshcd_query_flag(hba, UPIU_QUERY_OPCODE_READ_FLAG, \ QUERY_FLAG_IDN##_uname, &flag)) \ return -EINVAL; \ - return sprintf(buf, "%s\n", flag ? "true" : "false"); \ + return snprintf(buf, PAGE_SIZE, "%s\n", flag ? "true" : "false"); \ } \ static DEVICE_ATTR_RO(_name) @@ -620,6 +639,9 @@ UFS_FLAG(life_span_mode_enable, _LIFE_SPAN_MODE_ENABLE); UFS_FLAG(phy_resource_removal, _FPHYRESOURCEREMOVAL); UFS_FLAG(busy_rtc, _BUSY_RTC); UFS_FLAG(disable_fw_update, _PERMANENTLY_DISABLE_FW_UPDATE); +UFS_FLAG(wb_enable, _WB_EN); +UFS_FLAG(wb_flush_en, _WB_BUFF_FLUSH_EN); +UFS_FLAG(wb_flush_during_h8, _WB_BUFF_FLUSH_DURING_HIBERN8); static struct attribute *ufs_sysfs_device_flags[] = { &dev_attr_device_init.attr, @@ -630,6 +652,9 @@ static struct attribute *ufs_sysfs_device_flags[] = { &dev_attr_phy_resource_removal.attr, &dev_attr_busy_rtc.attr, &dev_attr_disable_fw_update.attr, + &dev_attr_wb_enable.attr, + &dev_attr_wb_flush_en.attr, + &dev_attr_wb_flush_during_h8.attr, NULL, }; @@ -647,7 +672,7 @@ static ssize_t _name##_show(struct device *dev, \ if (ufshcd_query_attr(hba, UPIU_QUERY_OPCODE_READ_ATTR, \ QUERY_ATTR_IDN##_uname, 0, 0, &value)) \ return -EINVAL; \ - return sprintf(buf, "0x%08X\n", value); \ + return snprintf(buf, PAGE_SIZE, "0x%08X\n", value); \ } \ static DEVICE_ATTR_RO(_name) @@ -667,6 +692,11 @@ UFS_ATTRIBUTE(exception_event_status, _EE_STATUS); UFS_ATTRIBUTE(ffu_status, _FFU_STATUS); UFS_ATTRIBUTE(psa_state, _PSA_STATE); UFS_ATTRIBUTE(psa_data_size, _PSA_DATA_SIZE); +UFS_ATTRIBUTE(wb_flush_status, _WB_FLUSH_STATUS); +UFS_ATTRIBUTE(wb_avail_buf, _AVAIL_WB_BUFF_SIZE); +UFS_ATTRIBUTE(wb_life_time_est, _WB_BUFF_LIFE_TIME_EST); +UFS_ATTRIBUTE(wb_cur_buf, _CURR_WB_BUFF_SIZE); + static struct attribute *ufs_sysfs_attributes[] = { &dev_attr_boot_lun_enabled.attr, @@ -685,6 +715,10 @@ static struct attribute *ufs_sysfs_attributes[] = { &dev_attr_ffu_status.attr, &dev_attr_psa_state.attr, &dev_attr_psa_data_size.attr, + &dev_attr_wb_flush_status.attr, + &dev_attr_wb_avail_buf.attr, + &dev_attr_wb_life_time_est.attr, + &dev_attr_wb_cur_buf.attr, NULL, }; @@ -736,6 +770,8 @@ UFS_UNIT_DESC_PARAM(provisioning_type, _PROVISIONING_TYPE, 1); UFS_UNIT_DESC_PARAM(physical_memory_resourse_count, _PHY_MEM_RSRC_CNT, 8); UFS_UNIT_DESC_PARAM(context_capabilities, _CTX_CAPABILITIES, 2); UFS_UNIT_DESC_PARAM(large_unit_granularity, _LARGE_UNIT_SIZE_M1, 1); +UFS_UNIT_DESC_PARAM(wb_buf_alloc_units, _WB_BUF_ALLOC_UNITS, 4); + static struct attribute *ufs_sysfs_unit_descriptor[] = { &dev_attr_boot_lun_id.attr, @@ -751,6 +787,7 @@ static struct attribute *ufs_sysfs_unit_descriptor[] = { &dev_attr_physical_memory_resourse_count.attr, &dev_attr_context_capabilities.attr, &dev_attr_large_unit_granularity.attr, + &dev_attr_wb_buf_alloc_units.attr, NULL, }; diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h index 990cb48..ffc4e18 100644 --- a/drivers/scsi/ufs/ufs.h +++ b/drivers/scsi/ufs/ufs.h @@ -63,6 +63,7 @@ #define UFS_UPIU_MAX_UNIT_NUM_ID 0x7F #define UFS_MAX_LUNS (SCSI_W_LUN_BASE + UFS_UPIU_MAX_UNIT_NUM_ID) #define UFS_UPIU_WLUN_ID (1 << 7) +#define UFS_UPIU_MAX_GENERAL_LUN 8 /* Well known logical unit id in LUN field of UPIU */ enum { @@ -140,6 +141,9 @@ enum flag_idn { QUERY_FLAG_IDN_BUSY_RTC = 0x09, QUERY_FLAG_IDN_RESERVED3 = 0x0A, QUERY_FLAG_IDN_PERMANENTLY_DISABLE_FW_UPDATE = 0x0B, + QUERY_FLAG_IDN_WB_EN = 0x0E, + QUERY_FLAG_IDN_WB_BUFF_FLUSH_EN = 0x0F, + QUERY_FLAG_IDN_WB_BUFF_FLUSH_DURING_HIBERN8 = 0x10, }; /* Attribute idn for Query requests */ @@ -168,6 +172,10 @@ enum attr_idn { QUERY_ATTR_IDN_PSA_STATE = 0x15, QUERY_ATTR_IDN_PSA_DATA_SIZE = 0x16, QUERY_ATTR_IDN_REF_CLK_GATING_WAIT_TIME = 0x17, + QUERY_ATTR_IDN_WB_FLUSH_STATUS = 0x1C, + QUERY_ATTR_IDN_AVAIL_WB_BUFF_SIZE = 0x1D, + QUERY_ATTR_IDN_WB_BUFF_LIFE_TIME_EST = 0x1E, + QUERY_ATTR_IDN_CURR_WB_BUFF_SIZE = 0x1F, }; /* Descriptor idn for Query requests */ @@ -191,7 +199,7 @@ enum desc_header_offset { }; enum ufs_desc_def_size { - QUERY_DESC_DEVICE_DEF_SIZE = 0x40, + QUERY_DESC_DEVICE_DEF_SIZE = 0x59, QUERY_DESC_CONFIGURATION_DEF_SIZE = 0x90, QUERY_DESC_UNIT_DEF_SIZE = 0x23, QUERY_DESC_INTERCONNECT_DEF_SIZE = 0x06, @@ -219,6 +227,7 @@ enum unit_desc_param { UNIT_DESC_PARAM_PHY_MEM_RSRC_CNT = 0x18, UNIT_DESC_PARAM_CTX_CAPABILITIES = 0x20, UNIT_DESC_PARAM_LARGE_UNIT_SIZE_M1 = 0x22, + UNIT_DESC_PARAM_WB_BUF_ALLOC_UNITS = 0x29, }; /* Device descriptor parameters offsets in bytes*/ @@ -258,6 +267,10 @@ enum device_desc_param { DEVICE_DESC_PARAM_PSA_MAX_DATA = 0x25, DEVICE_DESC_PARAM_PSA_TMT = 0x29, DEVICE_DESC_PARAM_PRDCT_REV = 0x2A, + DEVICE_DESC_PARAM_EXT_UFS_FEATURE_SUP = 0x4F, + DEVICE_DESC_PARAM_WB_US_RED_EN = 0x53, + DEVICE_DESC_PARAM_WB_TYPE = 0x54, + DEVICE_DESC_PARAM_WB_SHARED_ALLOC_UNITS = 0x55, }; /* Interconnect descriptor parameters offsets in bytes*/ @@ -302,6 +315,11 @@ enum geometry_desc_param { GEOMETRY_DESC_PARAM_ENM4_MAX_NUM_UNITS = 0x3E, GEOMETRY_DESC_PARAM_ENM4_CAP_ADJ_FCTR = 0x42, GEOMETRY_DESC_PARAM_OPT_LOG_BLK_SIZE = 0x44, + GEOMETRY_DESC_PARAM_WB_MAX_ALLOC_UNITS = 0x4F, + GEOMETRY_DESC_PARAM_WB_MAX_WB_LUNS = 0x53, + GEOMETRY_DESC_PARAM_WB_BUFF_CAP_ADJ = 0x54, + GEOMETRY_DESC_PARAM_WB_SUP_RED_TYPE = 0x55, + GEOMETRY_DESC_PARAM_WB_SUP_WB_TYPE = 0x56, }; /* Health descriptor parameters offsets in bytes*/ @@ -333,6 +351,11 @@ enum { UFSHCD_AMP = 3, }; +/* Possible values for dExtendedUFSFeaturesSupport */ +enum { + UFS_DEV_WRITE_BOOSTER_SUP = BIT(8), +}; + #define POWER_DESC_MAX_SIZE 0x62 #define POWER_DESC_MAX_ACTV_ICC_LVLS 16 @@ -447,6 +470,38 @@ enum ufs_dev_pwr_mode { UFS_POWERDOWN_PWR_MODE = 3, }; +enum ufs_dev_wb_buf_avail_size { + UFS_WB_0_PERCENT_BUF_REMAIN = 0x0, + UFS_WB_10_PERCENT_BUF_REMAIN = 0x1, + UFS_WB_20_PERCENT_BUF_REMAIN = 0x2, + UFS_WB_30_PERCENT_BUF_REMAIN = 0x3, + UFS_WB_40_PERCENT_BUF_REMAIN = 0x4, + UFS_WB_50_PERCENT_BUF_REMAIN = 0x5, + UFS_WB_60_PERCENT_BUF_REMAIN = 0x6, + UFS_WB_70_PERCENT_BUF_REMAIN = 0x7, + UFS_WB_80_PERCENT_BUF_REMAIN = 0x8, + UFS_WB_90_PERCENT_BUF_REMAIN = 0x9, + UFS_WB_100_PERCENT_BUF_REMAIN = 0xA, +}; + +enum ufs_dev_wb_buf_life_time_est { + UFS_WB_0_10_PERCENT_USED = 0x1, + UFS_WB_10_20_PERCENT_USED = 0x2, + UFS_WB_20_30_PERCENT_USED = 0x3, + UFS_WB_30_40_PERCENT_USED = 0x4, + UFS_WB_40_50_PERCENT_USED = 0x5, + UFS_WB_50_60_PERCENT_USED = 0x6, + UFS_WB_60_70_PERCENT_USED = 0x7, + UFS_WB_70_80_PERCENT_USED = 0x8, + UFS_WB_80_90_PERCENT_USED = 0x9, + UFS_WB_90_100_PERCENT_USED = 0xA, + UFS_WB_MAX_USED = 0xB, +}; + +enum ufs_dev_wb_buf_user_cap_config { + UFS_WB_BUFF_PRESERVE_USER_SPACE = 1, + UFS_WB_BUFF_USER_SPACE_RED_EN = 2, +}; /** * struct utp_cmd_rsp - Response UPIU structure * @residual_transfer_count: Residual transfer count DW-3 @@ -537,6 +592,10 @@ struct ufs_dev_info { u8 *model; u16 wspecversion; u32 clk_gating_wait_us; + u32 d_ext_ufs_feature_sup; + u8 b_wb_buffer_type; + bool wb_config_lun; + bool keep_vcc_on; }; /** diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index f4aa10f..4cc1fb4 100644 --- a/drivers/scsi/ufs/ufshcd.c +++ b/drivers/scsi/ufs/ufshcd.c @@ -47,6 +47,7 @@ #include "unipro.h" #include "ufs-sysfs.h" #include "ufs_bsg.h" +#include <linux/blkdev.h> #define CREATE_TRACE_POINTS #include <trace/events/ufs.h> @@ -263,6 +264,13 @@ static int ufshcd_scale_clks(struct ufs_hba *hba, bool scale_up); static irqreturn_t ufshcd_intr(int irq, void *__hba); static int ufshcd_change_power_mode(struct ufs_hba *hba, struct ufs_pa_layer_attr *pwr_mode); +static bool ufshcd_wb_sup(struct ufs_hba *hba); +static int ufshcd_wb_ctrl(struct ufs_hba *hba, bool enable); +static int ufshcd_wb_buf_flush_enable(struct ufs_hba *hba); +static int ufshcd_wb_buf_flush_disable(struct ufs_hba *hba); +static bool ufshcd_wb_is_buf_flush_needed(struct ufs_hba *hba); +static int ufshcd_wb_toggle_flush_during_h8(struct ufs_hba *hba, bool set); + static inline bool ufshcd_valid_tag(struct ufs_hba *hba, int tag) { return tag >= 0 && tag < hba->nutrs; @@ -284,6 +292,38 @@ static inline void ufshcd_disable_irq(struct ufs_hba *hba) } } +static inline void ufshcd_wb_toggle_flush(struct ufs_hba *hba) +{ + /* + * Query dAvailableWriteBoosterBufferSize attribute and enable + * the Write BoosterBuffer Flush if only 30% Write Booster + * Buffer is available. + * In reduction case, flush only if 10% is available + */ + if (ufshcd_wb_is_buf_flush_needed(hba)) + ufshcd_wb_buf_flush_enable(hba); + else + ufshcd_wb_buf_flush_disable(hba); +} + +static inline void ufshcd_wb_config(struct ufs_hba *hba) +{ + int ret; + + if (!ufshcd_wb_sup(hba)) + return; + + ret = ufshcd_wb_ctrl(hba, true); + if (ret) + dev_err(hba->dev, "%s: Enable WB failed: %d\n", __func__, ret); + else + dev_info(hba->dev, "%s: Write Booster Configured\n", __func__); + ret = ufshcd_wb_toggle_flush_during_h8(hba, true); + if (ret) + dev_err(hba->dev, "%s: En WB flush during H8: failed: %d\n", + __func__, ret); +} + static void ufshcd_scsi_unblock_requests(struct ufs_hba *hba) { if (atomic_dec_and_test(&hba->scsi_block_reqs_cnt)) @@ -1139,6 +1179,8 @@ static int ufshcd_devfreq_scale(struct ufs_hba *hba, bool scale_up) } ret = ufshcd_vops_clk_scale_notify(hba, scale_up, POST_CHANGE); + /* Enable Write Booster if we have scaled up else disable it */ + ufshcd_wb_ctrl(hba, scale_up); out: ufshcd_clock_scaling_unprepare(hba); @@ -4572,6 +4614,37 @@ static int ufshcd_get_lu_wp(struct ufs_hba *hba, return ret; } +/* + * ufshcd_get_wb_alloc_units - returns "dLUNumWriteBoosterBufferAllocUnits" + * @hba: per-adapter instance + * @lun: UFS device lun id + * @d_lun_wbb_au: pointer to buffer to hold the LU's alloc units info + * + * Returns 0 in case of success and d_lun_wbb_au would be returned + * Returns -ENOTSUPP if reading d_lun_wbb_au is not supported. + * Returns -EINVAL in case of invalid parameters passed to this function. + */ +static int ufshcd_get_wb_alloc_units(struct ufs_hba *hba, + u8 lun, + u8 *d_lun_wbb_au) +{ + int ret; + + if (!d_lun_wbb_au) + ret = -EINVAL; + + /* WB can be supported only from LU0..LU7 */ + else if (lun >= UFS_UPIU_MAX_GENERAL_LUN) + ret = -ENOTSUPP; + else + ret = ufshcd_read_unit_desc_param(hba, + lun, + UNIT_DESC_PARAM_WB_BUF_ALLOC_UNITS, + d_lun_wbb_au, + sizeof(*d_lun_wbb_au)); + return ret; +} + /** * ufshcd_get_lu_power_on_wp_status - get LU's power on write protect * status @@ -5194,6 +5267,165 @@ static void ufshcd_bkops_exception_event_handler(struct ufs_hba *hba) __func__, err); } +static bool ufshcd_wb_sup(struct ufs_hba *hba) +{ + return ((hba->dev_info.d_ext_ufs_feature_sup & + UFS_DEV_WRITE_BOOSTER_SUP) && + (hba->dev_info.b_wb_buffer_type + || hba->dev_info.wb_config_lun)); +} + +static int ufshcd_wb_ctrl(struct ufs_hba *hba, bool enable) +{ + int ret; + enum query_opcode opcode; + + if (!ufshcd_wb_sup(hba)) + return 0; + + if (enable) + opcode = UPIU_QUERY_OPCODE_SET_FLAG; + else + opcode = UPIU_QUERY_OPCODE_CLEAR_FLAG; + + ret = ufshcd_query_flag_retry(hba, opcode, + QUERY_FLAG_IDN_WB_EN, NULL); + if (ret) { + dev_err(hba->dev, "%s write booster %s failed %d\n", + __func__, enable ? "enable" : "disable", ret); + return ret; + } + + hba->wb_enabled = enable; + dev_dbg(hba->dev, "%s write booster %s %d\n", + __func__, enable ? "enable" : "disable", ret); + + return ret; +} + +static int ufshcd_wb_toggle_flush_during_h8(struct ufs_hba *hba, bool set) +{ + int val; + + if (set) + val = UPIU_QUERY_OPCODE_SET_FLAG; + else + val = UPIU_QUERY_OPCODE_CLEAR_FLAG; + + return ufshcd_query_flag_retry(hba, val, + QUERY_FLAG_IDN_WB_BUFF_FLUSH_DURING_HIBERN8, + NULL); +} + +static int ufshcd_wb_buf_flush_enable(struct ufs_hba *hba) +{ + int ret; + + if (!ufshcd_wb_sup(hba) || hba->wb_buf_flush_enabled) + return 0; + + ret = ufshcd_query_flag_retry(hba, UPIU_QUERY_OPCODE_SET_FLAG, + QUERY_FLAG_IDN_WB_BUFF_FLUSH_EN, NULL); + if (ret) + dev_err(hba->dev, "%s WB - buf flush enable failed %d\n", + __func__, ret); + else + hba->wb_buf_flush_enabled = true; + + dev_dbg(hba->dev, "WB - Flush enabled: %d\n", ret); + return ret; +} + +static int ufshcd_wb_buf_flush_disable(struct ufs_hba *hba) +{ + int ret; + + if (!ufshcd_wb_sup(hba) || !hba->wb_buf_flush_enabled) + return 0; + + ret = ufshcd_query_flag_retry(hba, UPIU_QUERY_OPCODE_CLEAR_FLAG, + QUERY_FLAG_IDN_WB_BUFF_FLUSH_EN, NULL); + if (ret) { + dev_warn(hba->dev, "%s: WB - buf flush disable failed %d\n", + __func__, ret); + } else { + hba->wb_buf_flush_enabled = false; + dev_dbg(hba->dev, "WB - Flush disabled: %d\n", ret); + } + + return ret; +} + +static bool ufshcd_wb_is_buf_flush_needed(struct ufs_hba *hba) +{ + int ret; + u32 cur_buf, status, avail_buf; + + if (!ufshcd_wb_sup(hba)) + return false; + + ret = ufshcd_query_attr_retry(hba, UPIU_QUERY_OPCODE_READ_ATTR, + QUERY_ATTR_IDN_AVAIL_WB_BUFF_SIZE, + 0, 0, &avail_buf); + if (ret) { + dev_warn(hba->dev, "%s dAvailableWriteBoosterBufferSize read failed %d\n", + __func__, ret); + return false; + } + + ret = ufshcd_vops_get_user_cap_mode(hba); + if (ret <= 0) { + dev_dbg(hba->dev, "Get user-cap reduction mode: failed: %d\n", + ret); + /* Most commonly used */ + ret = UFS_WB_BUFF_PRESERVE_USER_SPACE; + } + + hba->dev_info.keep_vcc_on = false; + if (ret == UFS_WB_BUFF_USER_SPACE_RED_EN) { + if (avail_buf <= UFS_WB_10_PERCENT_BUF_REMAIN) { + hba->dev_info.keep_vcc_on = true; + return true; + } + return false; + } else if (ret == UFS_WB_BUFF_PRESERVE_USER_SPACE) { + ret = ufshcd_query_attr_retry(hba, UPIU_QUERY_OPCODE_READ_ATTR, + QUERY_ATTR_IDN_CURR_WB_BUFF_SIZE, + 0, 0, &cur_buf); + if (ret) { + dev_err(hba->dev, "%s dCurWriteBoosterBufferSize read failed %d\n", + __func__, ret); + return false; + } + + if (!cur_buf) { + dev_info(hba->dev, "dCurWBBuf: %d WB disabled until free-space is available\n", + cur_buf); + return false; + } + + ret = ufshcd_get_ee_status(hba, &status); + if (ret) { + dev_err(hba->dev, "%s: failed to get exception status %d\n", + __func__, ret); + if (avail_buf < UFS_WB_40_PERCENT_BUF_REMAIN) { + hba->dev_info.keep_vcc_on = true; + return true; + } + return false; + } + + status &= hba->ee_ctrl_mask; + + if ((status & MASK_EE_URGENT_BKOPS) || + (avail_buf < UFS_WB_40_PERCENT_BUF_REMAIN)) { + hba->dev_info.keep_vcc_on = true; + return true; + } + } + return false; +} + /** * ufshcd_exception_event_handler - handle exceptions raised by device * @work: pointer to work data @@ -6629,7 +6861,8 @@ static int ufs_get_device_desc(struct ufs_hba *hba) int err; size_t buff_len; u8 model_index; - u8 *desc_buf; + u8 *desc_buf, wb_buf[4]; + u32 lun, res; struct ufs_dev_info *dev_info = &hba->dev_info; buff_len = max_t(size_t, hba->desc_size.dev_desc, @@ -6660,6 +6893,40 @@ static int ufs_get_device_desc(struct ufs_hba *hba) desc_buf[DEVICE_DESC_PARAM_SPEC_VER + 1]; model_index = desc_buf[DEVICE_DESC_PARAM_PRDCT_NAME]; + /* Enable WB only for UFS-3.1 OR if desc len >= 0x59 */ + if (dev_info->wspecversion >= 0x310) { + hba->dev_info.d_ext_ufs_feature_sup = + desc_buf[DEVICE_DESC_PARAM_EXT_UFS_FEATURE_SUP] + << 24 | + desc_buf[DEVICE_DESC_PARAM_EXT_UFS_FEATURE_SUP + 1] + << 16 | + desc_buf[DEVICE_DESC_PARAM_EXT_UFS_FEATURE_SUP + 2] + << 8 | + desc_buf[DEVICE_DESC_PARAM_EXT_UFS_FEATURE_SUP + 3]; + + hba->dev_info.b_wb_buffer_type = + desc_buf[DEVICE_DESC_PARAM_WB_TYPE]; + + if (hba->dev_info.b_wb_buffer_type) + goto skip_unit_desc; + + hba->dev_info.wb_config_lun = false; + for (lun = 0; lun < UFS_UPIU_MAX_GENERAL_LUN; lun++) { + memset(wb_buf, 0, sizeof(wb_buf)); + err = ufshcd_get_wb_alloc_units(hba, lun, wb_buf); + if (err) + break; + + res = wb_buf[0] << 24 | wb_buf[1] << 16 | + wb_buf[2] << 8 | wb_buf[3]; + if (res) { + hba->dev_info.wb_config_lun = true; + break; + } + } + } + +skip_unit_desc: err = ufshcd_read_string_desc(hba, model_index, &dev_info->model, SD_ASCII_STD); if (err < 0) { @@ -7162,6 +7429,7 @@ static int ufshcd_probe_hba(struct ufs_hba *hba, bool async) /* set the state as operational after switching to desired gear */ hba->ufshcd_state = UFSHCD_STATE_OPERATIONAL; + ufshcd_wb_config(hba); /* Enable Auto-Hibernate if configured */ ufshcd_auto_hibern8_enable(hba); @@ -7823,12 +8091,16 @@ static void ufshcd_vreg_set_lpm(struct ufs_hba *hba) * * Ignore the error returned by ufshcd_toggle_vreg() as device is anyway * in low power state which would save some power. + * + * If Write Booster is enabled and the device needs to flush the WB + * buffer OR if bkops status is urgent for WB, keep Vcc on. */ if (ufshcd_is_ufs_dev_poweroff(hba) && ufshcd_is_link_off(hba) && !hba->dev_info.is_lu_power_on_wp) { ufshcd_setup_vreg(hba, false); } else if (!ufshcd_is_ufs_dev_active(hba)) { - ufshcd_toggle_vreg(hba->dev, hba->vreg_info.vcc, false); + if (!hba->dev_info.keep_vcc_on) + ufshcd_toggle_vreg(hba->dev, hba->vreg_info.vcc, false); if (!ufshcd_is_link_active(hba)) { ufshcd_config_vreg_lpm(hba, hba->vreg_info.vccq); ufshcd_config_vreg_lpm(hba, hba->vreg_info.vccq2); @@ -7952,11 +8224,16 @@ static int ufshcd_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op) /* make sure that auto bkops is disabled */ ufshcd_disable_auto_bkops(hba); } + ufshcd_wb_toggle_flush(hba); + } else if (!ufshcd_is_runtime_pm(pm_op)) { + ufshcd_wb_buf_flush_disable(hba); + hba->dev_info.keep_vcc_on = false; } if ((req_dev_pwr_mode != hba->curr_dev_pwr_mode) && - ((ufshcd_is_runtime_pm(pm_op) && !hba->auto_bkops_enabled) || - !ufshcd_is_runtime_pm(pm_op))) { + ((ufshcd_is_runtime_pm(pm_op) && (!hba->auto_bkops_enabled) + && !hba->wb_buf_flush_enabled) || + !ufshcd_is_runtime_pm(pm_op))) { /* ensure that bkops is disabled */ ufshcd_disable_auto_bkops(hba); ret = ufshcd_set_dev_pwr_mode(hba, req_dev_pwr_mode); @@ -8078,6 +8355,7 @@ static int ufshcd_resume(struct ufs_hba *hba, enum ufs_pm_op pm_op) goto vendor_suspend; } + ufshcd_wb_buf_flush_disable(hba); if (!ufshcd_is_ufs_dev_active(hba)) { ret = ufshcd_set_dev_pwr_mode(hba, UFS_ACTIVE_PWR_MODE); if (ret) diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h index 8f516b2..6f36ff3 100644 --- a/drivers/scsi/ufs/ufshcd.h +++ b/drivers/scsi/ufs/ufshcd.h @@ -327,6 +327,7 @@ struct ufs_hba_variant_ops { void (*dbg_register_dump)(struct ufs_hba *hba); int (*phy_initialization)(struct ufs_hba *); void (*device_reset)(struct ufs_hba *hba); + u32 (*get_user_cap_mode)(struct ufs_hba *hba); }; /* clock gating state */ @@ -731,6 +732,8 @@ struct ufs_hba { struct device bsg_dev; struct request_queue *bsg_queue; + bool wb_buf_flush_enabled; + bool wb_enabled; }; /* Returns true if clocks can be gated. Otherwise false */ @@ -1123,4 +1126,10 @@ static inline u8 ufshcd_scsi_to_upiu_lun(unsigned int scsi_lun) int ufshcd_dump_regs(struct ufs_hba *hba, size_t offset, size_t len, const char *prefix); +static inline unsigned int ufshcd_vops_get_user_cap_mode(struct ufs_hba *hba) +{ + if (hba->vops && hba->vops->get_user_cap_mode) + return hba->vops->get_user_cap_mode(hba); + return 0; +} #endif /* End of Header */ -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project. ^ permalink raw reply related [flat|nested] 9+ messages in thread
* RE: [<RFC PATCH v1> 1/2] scsi: ufs: add write booster feature support 2020-02-22 0:47 ` [<RFC PATCH v1> 1/2] scsi: ufs: add write booster feature support Asutosh Das @ 2020-02-23 9:03 ` Avri Altman 2020-03-16 23:45 ` Asutosh Das (asd) 2020-02-25 10:58 ` Avri Altman 2020-02-25 12:50 ` Avri Altman 2 siblings, 1 reply; 9+ messages in thread From: Avri Altman @ 2020-02-23 9:03 UTC (permalink / raw) To: Asutosh Das, subhashj, cang, rnayak, vinholikatti, jejb, martin.petersen, linux-scsi Cc: linux-arm-msm, Alim Akhtar, James E.J. Bottomley, Stanley Chu, Bean Huo, Tomas Winkler, Colin Ian King, Bart Van Assche, Venkat Gopalakrishnan, Bjorn Andersson, open list Hi, > > Maybe add few words explaining what this feature is all about. > Write Booster enable: SW will enable the feature > when clocks are scaled up. > Once clocks and gear are scaled up, > SW will set “fWriteBoosterEn” flag > before exiting from ufshcd_devfreq_scale() function. > > Write Booster disable: SW will disable the feature, > when clocks are scaled down. Once clocks and gear > are scaled down, SW will clear “fWriteBoosterEn” flag > before exiting from ufshcd_devfreq_scale() function. At this point I am not convinced that all readers are interesting which flag does what. Maybe just saying that you are intending to use the clock scaling mechanism to set WB on/off, Which is, IMHO, an excellent design choice. Also here is a good place to explain that although it is not lun-specific and will turn WB on for reads as well, It is a second order miss and has negligible impact of the overall performance. Before the next paragraph you need to explain about the various WB buffer flush options - Not all readers are familiar with the latest spec to its details. > > Write Booster Buffer flush will be enabled in following cases: > 1. During runtime suspend, driver would query > bAvailableWriteBoosterBufferSize attribute and enable the > Write Booster Buffer Flush if less than 30% Write Booster > Buffer is available. Again, just explain why make your flush part of the power management flow, Without the gory details of the exact attribute, etc. > > Write Booster Buffer flush will be disabled in following case: > During runtime suspend, if available buffer size is >30%, > then Write Booster Flush will be stopped by > clearing fWriteBoosterBufferFlushEn flag. > And here a word of the various user space configuration option, Although I would look for ways to minimize the exposure of the WB driver, To the way the platform vendor provision its device. > In non-reduction case, during runtime-suspend, > dCurrentWriteBoosterBufferSize attribute would determine > if flush is possible. > If this value is > 0 and availBuf is less > than 30% then flush is needed and vcc needs to be ON. > > Signed-off-by: Asutosh Das <asutoshd@codeaurora.org> > Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org> For RFC its fine, but when you'll send it for review, This patch should be divided into 4-5 patches, minimum. > --- > drivers/scsi/ufs/ufs-sysfs.c | 41 ++++++- > drivers/scsi/ufs/ufs.h | 61 ++++++++- > drivers/scsi/ufs/ufshcd.c | 286 > ++++++++++++++++++++++++++++++++++++++++++- > drivers/scsi/ufs/ufshcd.h | 9 ++ > 4 files changed, 390 insertions(+), 7 deletions(-) > > diff --git a/drivers/scsi/ufs/ufs-sysfs.c b/drivers/scsi/ufs/ufs-sysfs.c > index dbdf8b0..4505e27 100644 > --- a/drivers/scsi/ufs/ufs-sysfs.c > +++ b/drivers/scsi/ufs/ufs-sysfs.c > @@ -274,6 +274,10 @@ UFS_DEVICE_DESC_PARAM(device_version, > _DEV_VER, 2); > UFS_DEVICE_DESC_PARAM(number_of_secure_wpa, _NUM_SEC_WPA, 1); > UFS_DEVICE_DESC_PARAM(psa_max_data_size, _PSA_MAX_DATA, 4); > UFS_DEVICE_DESC_PARAM(psa_state_timeout, _PSA_TMT, 1); > +UFS_DEVICE_DESC_PARAM(ext_feature_sup, _EXT_UFS_FEATURE_SUP, 4); > +UFS_DEVICE_DESC_PARAM(wb_presv_us_en, _WB_US_RED_EN, 1); > +UFS_DEVICE_DESC_PARAM(wb_type, _WB_TYPE, 1); > +UFS_DEVICE_DESC_PARAM(wb_shared_alloc_units, > _WB_SHARED_ALLOC_UNITS, 4); > > static struct attribute *ufs_sysfs_device_descriptor[] = { > &dev_attr_device_type.attr, > @@ -302,6 +306,10 @@ static struct attribute > *ufs_sysfs_device_descriptor[] = { > &dev_attr_number_of_secure_wpa.attr, > &dev_attr_psa_max_data_size.attr, > &dev_attr_psa_state_timeout.attr, > + &dev_attr_ext_feature_sup.attr, > + &dev_attr_wb_presv_us_en.attr, > + &dev_attr_wb_type.attr, > + &dev_attr_wb_shared_alloc_units.attr, > NULL, > }; > > @@ -371,6 +379,12 @@ > UFS_GEOMETRY_DESC_PARAM(enh4_memory_max_alloc_units, > _ENM4_MAX_NUM_UNITS, 4); > > UFS_GEOMETRY_DESC_PARAM(enh4_memory_capacity_adjustment_factor, > _ENM4_CAP_ADJ_FCTR, 2); > +UFS_GEOMETRY_DESC_PARAM(wb_max_alloc_units, > _WB_MAX_ALLOC_UNITS, 4); > +UFS_GEOMETRY_DESC_PARAM(wb_max_wb_luns, _WB_MAX_WB_LUNS, > 1); > +UFS_GEOMETRY_DESC_PARAM(wb_buff_cap_adj, _WB_BUFF_CAP_ADJ, > 1); > +UFS_GEOMETRY_DESC_PARAM(wb_sup_red_type, _WB_SUP_RED_TYPE, > 1); > +UFS_GEOMETRY_DESC_PARAM(wb_sup_wb_type, _WB_SUP_WB_TYPE, 1); > + > > static struct attribute *ufs_sysfs_geometry_descriptor[] = { > &dev_attr_raw_device_capacity.attr, > @@ -402,6 +416,11 @@ static struct attribute > *ufs_sysfs_geometry_descriptor[] = { > &dev_attr_enh3_memory_capacity_adjustment_factor.attr, > &dev_attr_enh4_memory_max_alloc_units.attr, > &dev_attr_enh4_memory_capacity_adjustment_factor.attr, > + &dev_attr_wb_max_alloc_units.attr, > + &dev_attr_wb_max_wb_luns.attr, > + &dev_attr_wb_buff_cap_adj.attr, > + &dev_attr_wb_sup_red_type.attr, > + &dev_attr_wb_sup_wb_type.attr, > NULL, > }; > > @@ -608,7 +627,7 @@ static ssize_t _name##_show(struct device *dev, > \ > if (ufshcd_query_flag(hba, UPIU_QUERY_OPCODE_READ_FLAG, \ > QUERY_FLAG_IDN##_uname, &flag)) \ > return -EINVAL; \ > - return sprintf(buf, "%s\n", flag ? "true" : "false"); \ > + return snprintf(buf, PAGE_SIZE, "%s\n", flag ? "true" : "false"); \ This should be sent as a separate bug fix - not as part of the WB series > } \ > static DEVICE_ATTR_RO(_name) > > @@ -620,6 +639,9 @@ UFS_FLAG(life_span_mode_enable, > _LIFE_SPAN_MODE_ENABLE); > UFS_FLAG(phy_resource_removal, _FPHYRESOURCEREMOVAL); > UFS_FLAG(busy_rtc, _BUSY_RTC); > UFS_FLAG(disable_fw_update, _PERMANENTLY_DISABLE_FW_UPDATE); > +UFS_FLAG(wb_enable, _WB_EN); > +UFS_FLAG(wb_flush_en, _WB_BUFF_FLUSH_EN); > +UFS_FLAG(wb_flush_during_h8, _WB_BUFF_FLUSH_DURING_HIBERN8); > > static struct attribute *ufs_sysfs_device_flags[] = { > &dev_attr_device_init.attr, > @@ -630,6 +652,9 @@ static struct attribute *ufs_sysfs_device_flags[] = { > &dev_attr_phy_resource_removal.attr, > &dev_attr_busy_rtc.attr, > &dev_attr_disable_fw_update.attr, > + &dev_attr_wb_enable.attr, > + &dev_attr_wb_flush_en.attr, > + &dev_attr_wb_flush_during_h8.attr, > NULL, > }; > > @@ -647,7 +672,7 @@ static ssize_t _name##_show(struct device *dev, > \ > if (ufshcd_query_attr(hba, UPIU_QUERY_OPCODE_READ_ATTR, \ > QUERY_ATTR_IDN##_uname, 0, 0, &value)) \ > return -EINVAL; \ > - return sprintf(buf, "0x%08X\n", value); \ > + return snprintf(buf, PAGE_SIZE, "0x%08X\n", value); \ ditto > } \ > static DEVICE_ATTR_RO(_name) > > @@ -667,6 +692,11 @@ UFS_ATTRIBUTE(exception_event_status, > _EE_STATUS); > UFS_ATTRIBUTE(ffu_status, _FFU_STATUS); > UFS_ATTRIBUTE(psa_state, _PSA_STATE); > UFS_ATTRIBUTE(psa_data_size, _PSA_DATA_SIZE); > +UFS_ATTRIBUTE(wb_flush_status, _WB_FLUSH_STATUS); > +UFS_ATTRIBUTE(wb_avail_buf, _AVAIL_WB_BUFF_SIZE); > +UFS_ATTRIBUTE(wb_life_time_est, _WB_BUFF_LIFE_TIME_EST); > +UFS_ATTRIBUTE(wb_cur_buf, _CURR_WB_BUFF_SIZE); > + > > static struct attribute *ufs_sysfs_attributes[] = { > &dev_attr_boot_lun_enabled.attr, > @@ -685,6 +715,10 @@ static struct attribute *ufs_sysfs_attributes[] = { > &dev_attr_ffu_status.attr, > &dev_attr_psa_state.attr, > &dev_attr_psa_data_size.attr, > + &dev_attr_wb_flush_status.attr, > + &dev_attr_wb_avail_buf.attr, > + &dev_attr_wb_life_time_est.attr, > + &dev_attr_wb_cur_buf.attr, > NULL, > }; > > @@ -736,6 +770,8 @@ UFS_UNIT_DESC_PARAM(provisioning_type, > _PROVISIONING_TYPE, 1); > UFS_UNIT_DESC_PARAM(physical_memory_resourse_count, > _PHY_MEM_RSRC_CNT, 8); > UFS_UNIT_DESC_PARAM(context_capabilities, _CTX_CAPABILITIES, 2); > UFS_UNIT_DESC_PARAM(large_unit_granularity, _LARGE_UNIT_SIZE_M1, > 1); > +UFS_UNIT_DESC_PARAM(wb_buf_alloc_units, _WB_BUF_ALLOC_UNITS, > 4); > + > > static struct attribute *ufs_sysfs_unit_descriptor[] = { > &dev_attr_boot_lun_id.attr, > @@ -751,6 +787,7 @@ static struct attribute *ufs_sysfs_unit_descriptor[] = > { > &dev_attr_physical_memory_resourse_count.attr, > &dev_attr_context_capabilities.attr, > &dev_attr_large_unit_granularity.attr, > + &dev_attr_wb_buf_alloc_units.attr, > NULL, > }; You might want to consider exposing the clock scaling governor attribute here as well, Because a) we should do that already as it control an essential part of the ufs driver, and b) it control WB now as well > > diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h > index 990cb48..ffc4e18 100644 > --- a/drivers/scsi/ufs/ufs.h > +++ b/drivers/scsi/ufs/ufs.h > @@ -63,6 +63,7 @@ > #define UFS_UPIU_MAX_UNIT_NUM_ID 0x7F > #define UFS_MAX_LUNS (SCSI_W_LUN_BASE + > UFS_UPIU_MAX_UNIT_NUM_ID) > #define UFS_UPIU_WLUN_ID (1 << 7) > +#define UFS_UPIU_MAX_GENERAL_LUN 8 Are you using this for “LU dedicated buffer" case? In that case maybe MAX_DEDICATED_BUFFERS? > > /* Well known logical unit id in LUN field of UPIU */ > enum { > @@ -140,6 +141,9 @@ enum flag_idn { > QUERY_FLAG_IDN_BUSY_RTC = 0x09, > QUERY_FLAG_IDN_RESERVED3 = 0x0A, > QUERY_FLAG_IDN_PERMANENTLY_DISABLE_FW_UPDATE = 0x0B, > + QUERY_FLAG_IDN_WB_EN = 0x0E, > + QUERY_FLAG_IDN_WB_BUFF_FLUSH_EN = 0x0F, > + QUERY_FLAG_IDN_WB_BUFF_FLUSH_DURING_HIBERN8 = 0x10, > }; > > /* Attribute idn for Query requests */ > @@ -168,6 +172,10 @@ enum attr_idn { > QUERY_ATTR_IDN_PSA_STATE = 0x15, > QUERY_ATTR_IDN_PSA_DATA_SIZE = 0x16, > QUERY_ATTR_IDN_REF_CLK_GATING_WAIT_TIME = 0x17, > + QUERY_ATTR_IDN_WB_FLUSH_STATUS = 0x1C, > + QUERY_ATTR_IDN_AVAIL_WB_BUFF_SIZE = 0x1D, > + QUERY_ATTR_IDN_WB_BUFF_LIFE_TIME_EST = 0x1E, > + QUERY_ATTR_IDN_CURR_WB_BUFF_SIZE = 0x1F, > }; > > /* Descriptor idn for Query requests */ > @@ -191,7 +199,7 @@ enum desc_header_offset { > }; > > enum ufs_desc_def_size { > - QUERY_DESC_DEVICE_DEF_SIZE = 0x40, > + QUERY_DESC_DEVICE_DEF_SIZE = 0x59, > QUERY_DESC_CONFIGURATION_DEF_SIZE = 0x90, > QUERY_DESC_UNIT_DEF_SIZE = 0x23, > QUERY_DESC_INTERCONNECT_DEF_SIZE = 0x06, > @@ -219,6 +227,7 @@ enum unit_desc_param { > UNIT_DESC_PARAM_PHY_MEM_RSRC_CNT = 0x18, > UNIT_DESC_PARAM_CTX_CAPABILITIES = 0x20, > UNIT_DESC_PARAM_LARGE_UNIT_SIZE_M1 = 0x22, > + UNIT_DESC_PARAM_WB_BUF_ALLOC_UNITS = 0x29, > }; We should really come up with a way to avoid this annoying habit of jedec to keep changing the descriptor sizes. But this is out of scope of this patchset. > > /* Device descriptor parameters offsets in bytes*/ > @@ -258,6 +267,10 @@ enum device_desc_param { > DEVICE_DESC_PARAM_PSA_MAX_DATA = 0x25, > DEVICE_DESC_PARAM_PSA_TMT = 0x29, > DEVICE_DESC_PARAM_PRDCT_REV = 0x2A, > + DEVICE_DESC_PARAM_EXT_UFS_FEATURE_SUP = 0x4F, > + DEVICE_DESC_PARAM_WB_US_RED_EN = 0x53, > + DEVICE_DESC_PARAM_WB_TYPE = 0x54, > + DEVICE_DESC_PARAM_WB_SHARED_ALLOC_UNITS = 0x55, > }; > > /* Interconnect descriptor parameters offsets in bytes*/ > @@ -302,6 +315,11 @@ enum geometry_desc_param { > GEOMETRY_DESC_PARAM_ENM4_MAX_NUM_UNITS = 0x3E, > GEOMETRY_DESC_PARAM_ENM4_CAP_ADJ_FCTR = 0x42, > GEOMETRY_DESC_PARAM_OPT_LOG_BLK_SIZE = 0x44, > + GEOMETRY_DESC_PARAM_WB_MAX_ALLOC_UNITS = 0x4F, > + GEOMETRY_DESC_PARAM_WB_MAX_WB_LUNS = 0x53, > + GEOMETRY_DESC_PARAM_WB_BUFF_CAP_ADJ = 0x54, > + GEOMETRY_DESC_PARAM_WB_SUP_RED_TYPE = 0x55, > + GEOMETRY_DESC_PARAM_WB_SUP_WB_TYPE = 0x56, > }; > > /* Health descriptor parameters offsets in bytes*/ > @@ -333,6 +351,11 @@ enum { > UFSHCD_AMP = 3, > }; > > +/* Possible values for dExtendedUFSFeaturesSupport */ > +enum { > + UFS_DEV_WRITE_BOOSTER_SUP = BIT(8), > +}; > + > #define POWER_DESC_MAX_SIZE 0x62 > #define POWER_DESC_MAX_ACTV_ICC_LVLS 16 > > @@ -447,6 +470,38 @@ enum ufs_dev_pwr_mode { > UFS_POWERDOWN_PWR_MODE = 3, > }; > > +enum ufs_dev_wb_buf_avail_size { > + UFS_WB_0_PERCENT_BUF_REMAIN = 0x0, > + UFS_WB_10_PERCENT_BUF_REMAIN = 0x1, > + UFS_WB_20_PERCENT_BUF_REMAIN = 0x2, > + UFS_WB_30_PERCENT_BUF_REMAIN = 0x3, > + UFS_WB_40_PERCENT_BUF_REMAIN = 0x4, > + UFS_WB_50_PERCENT_BUF_REMAIN = 0x5, > + UFS_WB_60_PERCENT_BUF_REMAIN = 0x6, > + UFS_WB_70_PERCENT_BUF_REMAIN = 0x7, > + UFS_WB_80_PERCENT_BUF_REMAIN = 0x8, > + UFS_WB_90_PERCENT_BUF_REMAIN = 0x9, > + UFS_WB_100_PERCENT_BUF_REMAIN = 0xA, > +}; Why do need all this? Just define the threshold you need. > + > +enum ufs_dev_wb_buf_life_time_est { > + UFS_WB_0_10_PERCENT_USED = 0x1, > + UFS_WB_10_20_PERCENT_USED = 0x2, > + UFS_WB_20_30_PERCENT_USED = 0x3, > + UFS_WB_30_40_PERCENT_USED = 0x4, > + UFS_WB_40_50_PERCENT_USED = 0x5, > + UFS_WB_50_60_PERCENT_USED = 0x6, > + UFS_WB_60_70_PERCENT_USED = 0x7, > + UFS_WB_70_80_PERCENT_USED = 0x8, > + UFS_WB_80_90_PERCENT_USED = 0x9, > + UFS_WB_90_100_PERCENT_USED = 0xA, > + UFS_WB_MAX_USED = 0xB, > +}; Here also. > + > +enum ufs_dev_wb_buf_user_cap_config { > + UFS_WB_BUFF_PRESERVE_USER_SPACE = 1, > + UFS_WB_BUFF_USER_SPACE_RED_EN = 2, > +}; > /** > * struct utp_cmd_rsp - Response UPIU structure > * @residual_transfer_count: Residual transfer count DW-3 > @@ -537,6 +592,10 @@ struct ufs_dev_info { > u8 *model; > u16 wspecversion; > u32 clk_gating_wait_us; > + u32 d_ext_ufs_feature_sup; > + u8 b_wb_buffer_type; > + bool wb_config_lun; > + bool keep_vcc_on; doc please - proper struct doc this time > }; > > /** > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c > index f4aa10f..4cc1fb4 100644 > --- a/drivers/scsi/ufs/ufshcd.c > +++ b/drivers/scsi/ufs/ufshcd.c > @@ -47,6 +47,7 @@ > #include "unipro.h" > #include "ufs-sysfs.h" > #include "ufs_bsg.h" > +#include <linux/blkdev.h> > > #define CREATE_TRACE_POINTS > #include <trace/events/ufs.h> > @@ -263,6 +264,13 @@ static int ufshcd_scale_clks(struct ufs_hba *hba, > bool scale_up); > static irqreturn_t ufshcd_intr(int irq, void *__hba); > static int ufshcd_change_power_mode(struct ufs_hba *hba, > struct ufs_pa_layer_attr *pwr_mode); > +static bool ufshcd_wb_sup(struct ufs_hba *hba); > +static int ufshcd_wb_ctrl(struct ufs_hba *hba, bool enable); > +static int ufshcd_wb_buf_flush_enable(struct ufs_hba *hba); > +static int ufshcd_wb_buf_flush_disable(struct ufs_hba *hba); > +static bool ufshcd_wb_is_buf_flush_needed(struct ufs_hba *hba); > +static int ufshcd_wb_toggle_flush_during_h8(struct ufs_hba *hba, bool > set); > + > static inline bool ufshcd_valid_tag(struct ufs_hba *hba, int tag) > { > return tag >= 0 && tag < hba->nutrs; > @@ -284,6 +292,38 @@ static inline void ufshcd_disable_irq(struct ufs_hba > *hba) > } > } > > +static inline void ufshcd_wb_toggle_flush(struct ufs_hba *hba) > +{ > + /* > + * Query dAvailableWriteBoosterBufferSize attribute and enable > + * the Write BoosterBuffer Flush if only 30% Write Booster > + * Buffer is available. > + * In reduction case, flush only if 10% is available > + */ > + if (ufshcd_wb_is_buf_flush_needed(hba)) > + ufshcd_wb_buf_flush_enable(hba); > + else > + ufshcd_wb_buf_flush_disable(hba); > +} There are voices calling to set flush on hibernate on init, and never touch it again. I think you need, both in your commit log, and here as well, add a comment explaining why toggling is even needed. > + > +static inline void ufshcd_wb_config(struct ufs_hba *hba) > +{ > + int ret; > + > + if (!ufshcd_wb_sup(hba)) > + return; > + > + ret = ufshcd_wb_ctrl(hba, true); > + if (ret) > + dev_err(hba->dev, "%s: Enable WB failed: %d\n", __func__, ret); > + else > + dev_info(hba->dev, "%s: Write Booster Configured\n", __func__); > + ret = ufshcd_wb_toggle_flush_during_h8(hba, true); > + if (ret) > + dev_err(hba->dev, "%s: En WB flush during H8: failed: %d\n", > + __func__, ret); > +} > + > static void ufshcd_scsi_unblock_requests(struct ufs_hba *hba) > { > if (atomic_dec_and_test(&hba->scsi_block_reqs_cnt)) > @@ -1139,6 +1179,8 @@ static int ufshcd_devfreq_scale(struct ufs_hba > *hba, bool scale_up) > } > > ret = ufshcd_vops_clk_scale_notify(hba, scale_up, POST_CHANGE); > + /* Enable Write Booster if we have scaled up else disable it */ > + ufshcd_wb_ctrl(hba, scale_up); > > out: > ufshcd_clock_scaling_unprepare(hba); > @@ -4572,6 +4614,37 @@ static int ufshcd_get_lu_wp(struct ufs_hba *hba, > return ret; > } > > +/* > + * ufshcd_get_wb_alloc_units - returns > "dLUNumWriteBoosterBufferAllocUnits" > + * @hba: per-adapter instance > + * @lun: UFS device lun id > + * @d_lun_wbb_au: pointer to buffer to hold the LU's alloc units info > + * > + * Returns 0 in case of success and d_lun_wbb_au would be returned > + * Returns -ENOTSUPP if reading d_lun_wbb_au is not supported. > + * Returns -EINVAL in case of invalid parameters passed to this function. > + */ > +static int ufshcd_get_wb_alloc_units(struct ufs_hba *hba, > + u8 lun, > + u8 *d_lun_wbb_au) > +{ > + int ret; > + > + if (!d_lun_wbb_au) > + ret = -EINVAL; > + > + /* WB can be supported only from LU0..LU7 */ > + else if (lun >= UFS_UPIU_MAX_GENERAL_LUN) > + ret = -ENOTSUPP; > + else > + ret = ufshcd_read_unit_desc_param(hba, > + lun, > + UNIT_DESC_PARAM_WB_BUF_ALLOC_UNITS, > + d_lun_wbb_au, > + sizeof(*d_lun_wbb_au)); > + return ret; > +} > + > /** > * ufshcd_get_lu_power_on_wp_status - get LU's power on write protect > * status > @@ -5194,6 +5267,165 @@ static void > ufshcd_bkops_exception_event_handler(struct ufs_hba *hba) > __func__, err); > } > > +static bool ufshcd_wb_sup(struct ufs_hba *hba) > +{ > + return ((hba->dev_info.d_ext_ufs_feature_sup & > + UFS_DEV_WRITE_BOOSTER_SUP) && > + (hba->dev_info.b_wb_buffer_type > + || hba->dev_info.wb_config_lun)); > +} > + > +static int ufshcd_wb_ctrl(struct ufs_hba *hba, bool enable) > +{ > + int ret; > + enum query_opcode opcode; > + > + if (!ufshcd_wb_sup(hba)) > + return 0; > + > + if (enable) > + opcode = UPIU_QUERY_OPCODE_SET_FLAG; > + else > + opcode = UPIU_QUERY_OPCODE_CLEAR_FLAG; If already wb_enabled is enabled/disabled no point to send the query request > + > + ret = ufshcd_query_flag_retry(hba, opcode, > + QUERY_FLAG_IDN_WB_EN, NULL); If we are in a non-shared-buffer mode - you need to send the flag to the applicable lun. WB introduces the first not-device-level flags. Need to adjust ufshcd_query_flag for it - in a preparatory patch... Same goes to the rest of your code that query flags and attributes > + if (ret) { > + dev_err(hba->dev, "%s write booster %s failed %d\n", > + __func__, enable ? "enable" : "disable", ret); > + return ret; > + } > + > + hba->wb_enabled = enable; > + dev_dbg(hba->dev, "%s write booster %s %d\n", > + __func__, enable ? "enable" : "disable", ret); > + > + return ret; > +} > + > +static int ufshcd_wb_toggle_flush_during_h8(struct ufs_hba *hba, bool set) > +{ > + int val; > + > + if (set) > + val = UPIU_QUERY_OPCODE_SET_FLAG; > + else > + val = UPIU_QUERY_OPCODE_CLEAR_FLAG; > + > + return ufshcd_query_flag_retry(hba, val, > + QUERY_FLAG_IDN_WB_BUFF_FLUSH_DURING_HIBERN8, > + NULL); > +} > + > +static int ufshcd_wb_buf_flush_enable(struct ufs_hba *hba) > +{ > + int ret; > + > + if (!ufshcd_wb_sup(hba) || hba->wb_buf_flush_enabled) > + return 0; > + > + ret = ufshcd_query_flag_retry(hba, UPIU_QUERY_OPCODE_SET_FLAG, > + QUERY_FLAG_IDN_WB_BUFF_FLUSH_EN, NULL); > + if (ret) > + dev_err(hba->dev, "%s WB - buf flush enable failed %d\n", > + __func__, ret); > + else > + hba->wb_buf_flush_enabled = true; > + > + dev_dbg(hba->dev, "WB - Flush enabled: %d\n", ret); > + return ret; > +} > + > +static int ufshcd_wb_buf_flush_disable(struct ufs_hba *hba) > +{ > + int ret; > + > + if (!ufshcd_wb_sup(hba) || !hba->wb_buf_flush_enabled) > + return 0; > + > + ret = ufshcd_query_flag_retry(hba, > UPIU_QUERY_OPCODE_CLEAR_FLAG, > + QUERY_FLAG_IDN_WB_BUFF_FLUSH_EN, NULL); > + if (ret) { > + dev_warn(hba->dev, "%s: WB - buf flush disable failed %d\n", > + __func__, ret); > + } else { > + hba->wb_buf_flush_enabled = false; > + dev_dbg(hba->dev, "WB - Flush disabled: %d\n", ret); > + } > + > + return ret; > +} > + > +static bool ufshcd_wb_is_buf_flush_needed(struct ufs_hba *hba) > +{ > + int ret; > + u32 cur_buf, status, avail_buf; > + > + if (!ufshcd_wb_sup(hba)) > + return false; > + > + ret = ufshcd_query_attr_retry(hba, > UPIU_QUERY_OPCODE_READ_ATTR, > + QUERY_ATTR_IDN_AVAIL_WB_BUFF_SIZE, > + 0, 0, &avail_buf); > + if (ret) { > + dev_warn(hba->dev, "%s dAvailableWriteBoosterBufferSize read > failed %d\n", > + __func__, ret); > + return false; > + } > + > + ret = ufshcd_vops_get_user_cap_mode(hba); > + if (ret <= 0) { > + dev_dbg(hba->dev, "Get user-cap reduction mode: failed: %d\n", > + ret); > + /* Most commonly used */ > + ret = UFS_WB_BUFF_PRESERVE_USER_SPACE; > + } Not sure I understand how flush is related to the reduced/non-reduced buffer config? > + > + hba->dev_info.keep_vcc_on = false; > + if (ret == UFS_WB_BUFF_USER_SPACE_RED_EN) { > + if (avail_buf <= UFS_WB_10_PERCENT_BUF_REMAIN) { > + hba->dev_info.keep_vcc_on = true; > + return true; > + } > + return false; > + } else if (ret == UFS_WB_BUFF_PRESERVE_USER_SPACE) { > + ret = ufshcd_query_attr_retry(hba, > UPIU_QUERY_OPCODE_READ_ATTR, > + QUERY_ATTR_IDN_CURR_WB_BUFF_SIZE, > + 0, 0, &cur_buf); > + if (ret) { > + dev_err(hba->dev, "%s dCurWriteBoosterBufferSize read failed > %d\n", > + __func__, ret); > + return false; > + } > + > + if (!cur_buf) { > + dev_info(hba->dev, "dCurWBBuf: %d WB disabled until free- > space is available\n", > + cur_buf); > + return false; > + } > + > + ret = ufshcd_get_ee_status(hba, &status); Responding to urgent flush exception event, should you decide to add it, Should be done as part of ufshcd_exception_event_handler, and not here. > + if (ret) { > + dev_err(hba->dev, "%s: failed to get exception status %d\n", > + __func__, ret); > + if (avail_buf < UFS_WB_40_PERCENT_BUF_REMAIN) { > + hba->dev_info.keep_vcc_on = true; > + return true; > + } > + return false; > + } > + > + status &= hba->ee_ctrl_mask; > + > + if ((status & MASK_EE_URGENT_BKOPS) || > + (avail_buf < UFS_WB_40_PERCENT_BUF_REMAIN)) { > + hba->dev_info.keep_vcc_on = true; > + return true; > + } > + } > + return false; This function follow some wired logic, that needs to be further explained > +} > + > /** > * ufshcd_exception_event_handler - handle exceptions raised by device > * @work: pointer to work data > @@ -6629,7 +6861,8 @@ static int ufs_get_device_desc(struct ufs_hba > *hba) > int err; > size_t buff_len; > u8 model_index; > - u8 *desc_buf; > + u8 *desc_buf, wb_buf[4]; > + u32 lun, res; > struct ufs_dev_info *dev_info = &hba->dev_info; > > buff_len = max_t(size_t, hba->desc_size.dev_desc, > @@ -6660,6 +6893,40 @@ static int ufs_get_device_desc(struct ufs_hba > *hba) > desc_buf[DEVICE_DESC_PARAM_SPEC_VER + 1]; > > model_index = desc_buf[DEVICE_DESC_PARAM_PRDCT_NAME]; > + /* Enable WB only for UFS-3.1 OR if desc len >= 0x59 */ > + if (dev_info->wspecversion >= 0x310) { > + hba->dev_info.d_ext_ufs_feature_sup = > + desc_buf[DEVICE_DESC_PARAM_EXT_UFS_FEATURE_SUP] > + << 24 | > + desc_buf[DEVICE_DESC_PARAM_EXT_UFS_FEATURE_SUP + 1] > + << 16 | > + desc_buf[DEVICE_DESC_PARAM_EXT_UFS_FEATURE_SUP + 2] > + << 8 | > + desc_buf[DEVICE_DESC_PARAM_EXT_UFS_FEATURE_SUP + 3]; > + > + hba->dev_info.b_wb_buffer_type = > + desc_buf[DEVICE_DESC_PARAM_WB_TYPE]; > + > + if (hba->dev_info.b_wb_buffer_type) > + goto skip_unit_desc; Comment please, e.g.: /* bWriteBoosterBufferType indicates that the WB buffer is configured as a shared buffer mode */ And instead the goto pack iterating the unit descriptors in a helper? > + > + hba->dev_info.wb_config_lun = false; > + for (lun = 0; lun < UFS_UPIU_MAX_GENERAL_LUN; lun++) { > + memset(wb_buf, 0, sizeof(wb_buf)); > + err = ufshcd_get_wb_alloc_units(hba, lun, wb_buf); > + if (err) > + break; > + > + res = wb_buf[0] << 24 | wb_buf[1] << 16 | > + wb_buf[2] << 8 | wb_buf[3]; > + if (res) { > + hba->dev_info.wb_config_lun = true; > + break; Maybe you want to be aware which luns has a WB buffer, and of its WB buffer size. And as the clock scaling mechanism is unware of which writes goes to which lun, Explain that as well. > + } > + } > + } > + > +skip_unit_desc: > err = ufshcd_read_string_desc(hba, model_index, > &dev_info->model, SD_ASCII_STD); > if (err < 0) { > @@ -7162,6 +7429,7 @@ static int ufshcd_probe_hba(struct ufs_hba *hba, > bool async) > > /* set the state as operational after switching to desired gear */ > hba->ufshcd_state = UFSHCD_STATE_OPERATIONAL; > + ufshcd_wb_config(hba); Enabling WB regardless of clock scaling on init? Why not just ufshcd_wb_toggle_flush_during_h8() > > /* Enable Auto-Hibernate if configured */ > ufshcd_auto_hibern8_enable(hba); > @@ -7823,12 +8091,16 @@ static void ufshcd_vreg_set_lpm(struct ufs_hba > *hba) > * > * Ignore the error returned by ufshcd_toggle_vreg() as device is anyway > * in low power state which would save some power. > + * > + * If Write Booster is enabled and the device needs to flush the WB > + * buffer OR if bkops status is urgent for WB, keep Vcc on. I am not sure that you want to entail WB and bkops together in the same patch. If you are changing the logic of this function with respect of bkops - Do that In a separate patch, not related to WB. > */ > if (ufshcd_is_ufs_dev_poweroff(hba) && ufshcd_is_link_off(hba) && > !hba->dev_info.is_lu_power_on_wp) { > ufshcd_setup_vreg(hba, false); > } else if (!ufshcd_is_ufs_dev_active(hba)) { > - ufshcd_toggle_vreg(hba->dev, hba->vreg_info.vcc, false); > + if (!hba->dev_info.keep_vcc_on) > + ufshcd_toggle_vreg(hba->dev, hba->vreg_info.vcc, false); > if (!ufshcd_is_link_active(hba)) { > ufshcd_config_vreg_lpm(hba, hba->vreg_info.vccq); > ufshcd_config_vreg_lpm(hba, hba->vreg_info.vccq2); > @@ -7952,11 +8224,16 @@ static int ufshcd_suspend(struct ufs_hba *hba, > enum ufs_pm_op pm_op) > /* make sure that auto bkops is disabled */ > ufshcd_disable_auto_bkops(hba); > } > + ufshcd_wb_toggle_flush(hba); > + } else if (!ufshcd_is_runtime_pm(pm_op)) { > + ufshcd_wb_buf_flush_disable(hba); > + hba->dev_info.keep_vcc_on = false; > } > > if ((req_dev_pwr_mode != hba->curr_dev_pwr_mode) && > - ((ufshcd_is_runtime_pm(pm_op) && !hba->auto_bkops_enabled) || > - !ufshcd_is_runtime_pm(pm_op))) { > + ((ufshcd_is_runtime_pm(pm_op) && (!hba->auto_bkops_enabled) > + && !hba->wb_buf_flush_enabled) || > + !ufshcd_is_runtime_pm(pm_op))) { > /* ensure that bkops is disabled */ > ufshcd_disable_auto_bkops(hba); > ret = ufshcd_set_dev_pwr_mode(hba, req_dev_pwr_mode); > @@ -8078,6 +8355,7 @@ static int ufshcd_resume(struct ufs_hba *hba, > enum ufs_pm_op pm_op) > goto vendor_suspend; > } > > + ufshcd_wb_buf_flush_disable(hba); > if (!ufshcd_is_ufs_dev_active(hba)) { > ret = ufshcd_set_dev_pwr_mode(hba, UFS_ACTIVE_PWR_MODE); > if (ret) > diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h > index 8f516b2..6f36ff3 100644 > --- a/drivers/scsi/ufs/ufshcd.h > +++ b/drivers/scsi/ufs/ufshcd.h > @@ -327,6 +327,7 @@ struct ufs_hba_variant_ops { > void (*dbg_register_dump)(struct ufs_hba *hba); > int (*phy_initialization)(struct ufs_hba *); > void (*device_reset)(struct ufs_hba *hba); > + u32 (*get_user_cap_mode)(struct ufs_hba *hba); Make the name indicate that this is WB related? > }; > > /* clock gating state */ > @@ -731,6 +732,8 @@ struct ufs_hba { > > struct device bsg_dev; > struct request_queue *bsg_queue; > + bool wb_buf_flush_enabled; > + bool wb_enabled; > }; > > /* Returns true if clocks can be gated. Otherwise false */ > @@ -1123,4 +1126,10 @@ static inline u8 ufshcd_scsi_to_upiu_lun(unsigned > int scsi_lun) > int ufshcd_dump_regs(struct ufs_hba *hba, size_t offset, size_t len, > const char *prefix); > > +static inline unsigned int ufshcd_vops_get_user_cap_mode(struct ufs_hba > *hba) > +{ > + if (hba->vops && hba->vops->get_user_cap_mode) > + return hba->vops->get_user_cap_mode(hba); > + return 0; > +} > #endif /* End of Header */ > -- > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a > Linux Foundation Collaborative Project. Thanks, Avri ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [<RFC PATCH v1> 1/2] scsi: ufs: add write booster feature support 2020-02-23 9:03 ` Avri Altman @ 2020-03-16 23:45 ` Asutosh Das (asd) 0 siblings, 0 replies; 9+ messages in thread From: Asutosh Das (asd) @ 2020-03-16 23:45 UTC (permalink / raw) To: Avri Altman, subhashj, cang, rnayak, vinholikatti, jejb, martin.petersen, linux-scsi Cc: linux-arm-msm, Alim Akhtar, James E.J. Bottomley, Stanley Chu, Bean Huo, Tomas Winkler, Colin Ian King, Bart Van Assche, Venkat Gopalakrishnan, Bjorn Andersson, open list On 2/23/2020 1:03 AM, Avri Altman wrote: > Hi, >> >> > Maybe add few words explaining what this feature is all about. > >> Write Booster enable: SW will enable the feature >> when clocks are scaled up. >> Once clocks and gear are scaled up, >> SW will set “fWriteBoosterEn” flag >> before exiting from ufshcd_devfreq_scale() function. >> >> Write Booster disable: SW will disable the feature, >> when clocks are scaled down. Once clocks and gear >> are scaled down, SW will clear “fWriteBoosterEn” flag >> before exiting from ufshcd_devfreq_scale() function. > At this point I am not convinced that all readers are interesting which flag does what. > Maybe just saying that you are intending to use the clock scaling mechanism to set WB on/off, > Which is, IMHO, an excellent design choice. > Also here is a good place to explain that although it is not lun-specific and will turn WB on for reads as well, > It is a second order miss and has negligible impact of the overall performance. > > Before the next paragraph you need to explain about the various WB buffer flush options - > Not all readers are familiar with the latest spec to its details. > >> >> Write Booster Buffer flush will be enabled in following cases: >> 1. During runtime suspend, driver would query >> bAvailableWriteBoosterBufferSize attribute and enable the >> Write Booster Buffer Flush if less than 30% Write Booster >> Buffer is available. > Again, just explain why make your flush part of the power management flow, > Without the gory details of the exact attribute, etc. > >> >> Write Booster Buffer flush will be disabled in following case: >> During runtime suspend, if available buffer size is >30%, >> then Write Booster Flush will be stopped by >> clearing fWriteBoosterBufferFlushEn flag. >> > > And here a word of the various user space configuration option, > Although I would look for ways to minimize the exposure of the WB driver, > To the way the platform vendor provision its device. > >> In non-reduction case, during runtime-suspend, >> dCurrentWriteBoosterBufferSize attribute would determine >> if flush is possible. >> If this value is > 0 and availBuf is less >> than 30% then flush is needed and vcc needs to be ON. >> >> Signed-off-by: Asutosh Das <asutoshd@codeaurora.org> >> Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org> > > For RFC its fine, but when you'll send it for review, > This patch should be divided into 4-5 patches, minimum. > Looking at the number of comments, I'll send out another v2 RFC before sending out a patchset. >> --- >> drivers/scsi/ufs/ufs-sysfs.c | 41 ++++++- >> drivers/scsi/ufs/ufs.h | 61 ++++++++- >> drivers/scsi/ufs/ufshcd.c | 286 >> ++++++++++++++++++++++++++++++++++++++++++- >> drivers/scsi/ufs/ufshcd.h | 9 ++ >> 4 files changed, 390 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/scsi/ufs/ufs-sysfs.c b/drivers/scsi/ufs/ufs-sysfs.c >> index dbdf8b0..4505e27 100644 >> --- a/drivers/scsi/ufs/ufs-sysfs.c >> +++ b/drivers/scsi/ufs/ufs-sysfs.c >> @@ -274,6 +274,10 @@ UFS_DEVICE_DESC_PARAM(device_version, >> _DEV_VER, 2); >> UFS_DEVICE_DESC_PARAM(number_of_secure_wpa, _NUM_SEC_WPA, 1); >> UFS_DEVICE_DESC_PARAM(psa_max_data_size, _PSA_MAX_DATA, 4); >> UFS_DEVICE_DESC_PARAM(psa_state_timeout, _PSA_TMT, 1); >> +UFS_DEVICE_DESC_PARAM(ext_feature_sup, _EXT_UFS_FEATURE_SUP, 4); >> +UFS_DEVICE_DESC_PARAM(wb_presv_us_en, _WB_US_RED_EN, 1); >> +UFS_DEVICE_DESC_PARAM(wb_type, _WB_TYPE, 1); >> +UFS_DEVICE_DESC_PARAM(wb_shared_alloc_units, >> _WB_SHARED_ALLOC_UNITS, 4); >> >> static struct attribute *ufs_sysfs_device_descriptor[] = { >> &dev_attr_device_type.attr, >> @@ -302,6 +306,10 @@ static struct attribute >> *ufs_sysfs_device_descriptor[] = { >> &dev_attr_number_of_secure_wpa.attr, >> &dev_attr_psa_max_data_size.attr, >> &dev_attr_psa_state_timeout.attr, >> + &dev_attr_ext_feature_sup.attr, >> + &dev_attr_wb_presv_us_en.attr, >> + &dev_attr_wb_type.attr, >> + &dev_attr_wb_shared_alloc_units.attr, >> NULL, >> }; >> >> @@ -371,6 +379,12 @@ >> UFS_GEOMETRY_DESC_PARAM(enh4_memory_max_alloc_units, >> _ENM4_MAX_NUM_UNITS, 4); >> >> UFS_GEOMETRY_DESC_PARAM(enh4_memory_capacity_adjustment_factor, >> _ENM4_CAP_ADJ_FCTR, 2); >> +UFS_GEOMETRY_DESC_PARAM(wb_max_alloc_units, >> _WB_MAX_ALLOC_UNITS, 4); >> +UFS_GEOMETRY_DESC_PARAM(wb_max_wb_luns, _WB_MAX_WB_LUNS, >> 1); >> +UFS_GEOMETRY_DESC_PARAM(wb_buff_cap_adj, _WB_BUFF_CAP_ADJ, >> 1); >> +UFS_GEOMETRY_DESC_PARAM(wb_sup_red_type, _WB_SUP_RED_TYPE, >> 1); >> +UFS_GEOMETRY_DESC_PARAM(wb_sup_wb_type, _WB_SUP_WB_TYPE, 1); >> + >> >> static struct attribute *ufs_sysfs_geometry_descriptor[] = { >> &dev_attr_raw_device_capacity.attr, >> @@ -402,6 +416,11 @@ static struct attribute >> *ufs_sysfs_geometry_descriptor[] = { >> &dev_attr_enh3_memory_capacity_adjustment_factor.attr, >> &dev_attr_enh4_memory_max_alloc_units.attr, >> &dev_attr_enh4_memory_capacity_adjustment_factor.attr, >> + &dev_attr_wb_max_alloc_units.attr, >> + &dev_attr_wb_max_wb_luns.attr, >> + &dev_attr_wb_buff_cap_adj.attr, >> + &dev_attr_wb_sup_red_type.attr, >> + &dev_attr_wb_sup_wb_type.attr, >> NULL, >> }; >> >> @@ -608,7 +627,7 @@ static ssize_t _name##_show(struct device *dev, >> \ >> if (ufshcd_query_flag(hba, UPIU_QUERY_OPCODE_READ_FLAG, \ >> QUERY_FLAG_IDN##_uname, &flag)) \ >> return -EINVAL; \ >> - return sprintf(buf, "%s\n", flag ? "true" : "false"); \ >> + return snprintf(buf, PAGE_SIZE, "%s\n", flag ? "true" : "false"); \ > This should be sent as a separate bug fix - not as part of the WB series > >> } \ >> static DEVICE_ATTR_RO(_name) >> >> @@ -620,6 +639,9 @@ UFS_FLAG(life_span_mode_enable, >> _LIFE_SPAN_MODE_ENABLE); >> UFS_FLAG(phy_resource_removal, _FPHYRESOURCEREMOVAL); >> UFS_FLAG(busy_rtc, _BUSY_RTC); >> UFS_FLAG(disable_fw_update, _PERMANENTLY_DISABLE_FW_UPDATE); >> +UFS_FLAG(wb_enable, _WB_EN); >> +UFS_FLAG(wb_flush_en, _WB_BUFF_FLUSH_EN); >> +UFS_FLAG(wb_flush_during_h8, _WB_BUFF_FLUSH_DURING_HIBERN8); >> >> static struct attribute *ufs_sysfs_device_flags[] = { >> &dev_attr_device_init.attr, >> @@ -630,6 +652,9 @@ static struct attribute *ufs_sysfs_device_flags[] = { >> &dev_attr_phy_resource_removal.attr, >> &dev_attr_busy_rtc.attr, >> &dev_attr_disable_fw_update.attr, >> + &dev_attr_wb_enable.attr, >> + &dev_attr_wb_flush_en.attr, >> + &dev_attr_wb_flush_during_h8.attr, >> NULL, >> }; >> >> @@ -647,7 +672,7 @@ static ssize_t _name##_show(struct device *dev, >> \ >> if (ufshcd_query_attr(hba, UPIU_QUERY_OPCODE_READ_ATTR, \ >> QUERY_ATTR_IDN##_uname, 0, 0, &value)) \ >> return -EINVAL; \ >> - return sprintf(buf, "0x%08X\n", value); \ >> + return snprintf(buf, PAGE_SIZE, "0x%08X\n", value); \ > ditto > >> } \ >> static DEVICE_ATTR_RO(_name) >> >> @@ -667,6 +692,11 @@ UFS_ATTRIBUTE(exception_event_status, >> _EE_STATUS); >> UFS_ATTRIBUTE(ffu_status, _FFU_STATUS); >> UFS_ATTRIBUTE(psa_state, _PSA_STATE); >> UFS_ATTRIBUTE(psa_data_size, _PSA_DATA_SIZE); >> +UFS_ATTRIBUTE(wb_flush_status, _WB_FLUSH_STATUS); >> +UFS_ATTRIBUTE(wb_avail_buf, _AVAIL_WB_BUFF_SIZE); >> +UFS_ATTRIBUTE(wb_life_time_est, _WB_BUFF_LIFE_TIME_EST); >> +UFS_ATTRIBUTE(wb_cur_buf, _CURR_WB_BUFF_SIZE); >> + >> >> static struct attribute *ufs_sysfs_attributes[] = { >> &dev_attr_boot_lun_enabled.attr, >> @@ -685,6 +715,10 @@ static struct attribute *ufs_sysfs_attributes[] = { >> &dev_attr_ffu_status.attr, >> &dev_attr_psa_state.attr, >> &dev_attr_psa_data_size.attr, >> + &dev_attr_wb_flush_status.attr, >> + &dev_attr_wb_avail_buf.attr, >> + &dev_attr_wb_life_time_est.attr, >> + &dev_attr_wb_cur_buf.attr, >> NULL, >> }; >> >> @@ -736,6 +770,8 @@ UFS_UNIT_DESC_PARAM(provisioning_type, >> _PROVISIONING_TYPE, 1); >> UFS_UNIT_DESC_PARAM(physical_memory_resourse_count, >> _PHY_MEM_RSRC_CNT, 8); >> UFS_UNIT_DESC_PARAM(context_capabilities, _CTX_CAPABILITIES, 2); >> UFS_UNIT_DESC_PARAM(large_unit_granularity, _LARGE_UNIT_SIZE_M1, >> 1); >> +UFS_UNIT_DESC_PARAM(wb_buf_alloc_units, _WB_BUF_ALLOC_UNITS, >> 4); >> + >> >> static struct attribute *ufs_sysfs_unit_descriptor[] = { >> &dev_attr_boot_lun_id.attr, >> @@ -751,6 +787,7 @@ static struct attribute *ufs_sysfs_unit_descriptor[] = >> { >> &dev_attr_physical_memory_resourse_count.attr, >> &dev_attr_context_capabilities.attr, >> &dev_attr_large_unit_granularity.attr, >> + &dev_attr_wb_buf_alloc_units.attr, >> NULL, >> }; > > You might want to consider exposing the clock scaling governor attribute here as well, > Because > a) we should do that already as it control an essential part of the ufs driver, and > b) it control WB now as well > Which clock scaling governor attributes are you alluding to so that we're on the same page. >> >> diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h >> index 990cb48..ffc4e18 100644 >> --- a/drivers/scsi/ufs/ufs.h >> +++ b/drivers/scsi/ufs/ufs.h >> @@ -63,6 +63,7 @@ >> #define UFS_UPIU_MAX_UNIT_NUM_ID 0x7F >> #define UFS_MAX_LUNS (SCSI_W_LUN_BASE + >> UFS_UPIU_MAX_UNIT_NUM_ID) >> #define UFS_UPIU_WLUN_ID (1 << 7) >> +#define UFS_UPIU_MAX_GENERAL_LUN 8 > Are you using this for “LU dedicated buffer" case? > In that case maybe MAX_DEDICATED_BUFFERS? > Currently, in LU dedicated buffer case, only 1 WB buffer can be supported. In the current code, I use it to check if WB is configured in at least 1 Lun. >> >> /* Well known logical unit id in LUN field of UPIU */ >> enum { >> @@ -140,6 +141,9 @@ enum flag_idn { >> QUERY_FLAG_IDN_BUSY_RTC = 0x09, >> QUERY_FLAG_IDN_RESERVED3 = 0x0A, >> QUERY_FLAG_IDN_PERMANENTLY_DISABLE_FW_UPDATE = 0x0B, >> + QUERY_FLAG_IDN_WB_EN = 0x0E, >> + QUERY_FLAG_IDN_WB_BUFF_FLUSH_EN = 0x0F, >> + QUERY_FLAG_IDN_WB_BUFF_FLUSH_DURING_HIBERN8 = 0x10, >> }; >> >> /* Attribute idn for Query requests */ >> @@ -168,6 +172,10 @@ enum attr_idn { >> QUERY_ATTR_IDN_PSA_STATE = 0x15, >> QUERY_ATTR_IDN_PSA_DATA_SIZE = 0x16, >> QUERY_ATTR_IDN_REF_CLK_GATING_WAIT_TIME = 0x17, >> + QUERY_ATTR_IDN_WB_FLUSH_STATUS = 0x1C, >> + QUERY_ATTR_IDN_AVAIL_WB_BUFF_SIZE = 0x1D, >> + QUERY_ATTR_IDN_WB_BUFF_LIFE_TIME_EST = 0x1E, >> + QUERY_ATTR_IDN_CURR_WB_BUFF_SIZE = 0x1F, >> }; >> >> /* Descriptor idn for Query requests */ >> @@ -191,7 +199,7 @@ enum desc_header_offset { >> }; >> >> enum ufs_desc_def_size { >> - QUERY_DESC_DEVICE_DEF_SIZE = 0x40, >> + QUERY_DESC_DEVICE_DEF_SIZE = 0x59, >> QUERY_DESC_CONFIGURATION_DEF_SIZE = 0x90, >> QUERY_DESC_UNIT_DEF_SIZE = 0x23, >> QUERY_DESC_INTERCONNECT_DEF_SIZE = 0x06, >> @@ -219,6 +227,7 @@ enum unit_desc_param { >> UNIT_DESC_PARAM_PHY_MEM_RSRC_CNT = 0x18, >> UNIT_DESC_PARAM_CTX_CAPABILITIES = 0x20, >> UNIT_DESC_PARAM_LARGE_UNIT_SIZE_M1 = 0x22, >> + UNIT_DESC_PARAM_WB_BUF_ALLOC_UNITS = 0x29, >> }; > We should really come up with a way to avoid this annoying habit of jedec to keep changing the descriptor sizes. > But this is out of scope of this patchset. > Yeah - now I use the 0x59 as one of the checks for WB support. I guess, I can just check the version to be 3.1 and if so, check for WB configuration. >> >> /* Device descriptor parameters offsets in bytes*/ >> @@ -258,6 +267,10 @@ enum device_desc_param { >> DEVICE_DESC_PARAM_PSA_MAX_DATA = 0x25, >> DEVICE_DESC_PARAM_PSA_TMT = 0x29, >> DEVICE_DESC_PARAM_PRDCT_REV = 0x2A, >> + DEVICE_DESC_PARAM_EXT_UFS_FEATURE_SUP = 0x4F, >> + DEVICE_DESC_PARAM_WB_US_RED_EN = 0x53, >> + DEVICE_DESC_PARAM_WB_TYPE = 0x54, >> + DEVICE_DESC_PARAM_WB_SHARED_ALLOC_UNITS = 0x55, >> }; >> >> /* Interconnect descriptor parameters offsets in bytes*/ >> @@ -302,6 +315,11 @@ enum geometry_desc_param { >> GEOMETRY_DESC_PARAM_ENM4_MAX_NUM_UNITS = 0x3E, >> GEOMETRY_DESC_PARAM_ENM4_CAP_ADJ_FCTR = 0x42, >> GEOMETRY_DESC_PARAM_OPT_LOG_BLK_SIZE = 0x44, >> + GEOMETRY_DESC_PARAM_WB_MAX_ALLOC_UNITS = 0x4F, >> + GEOMETRY_DESC_PARAM_WB_MAX_WB_LUNS = 0x53, >> + GEOMETRY_DESC_PARAM_WB_BUFF_CAP_ADJ = 0x54, >> + GEOMETRY_DESC_PARAM_WB_SUP_RED_TYPE = 0x55, >> + GEOMETRY_DESC_PARAM_WB_SUP_WB_TYPE = 0x56, >> }; >> >> /* Health descriptor parameters offsets in bytes*/ >> @@ -333,6 +351,11 @@ enum { >> UFSHCD_AMP = 3, >> }; >> >> +/* Possible values for dExtendedUFSFeaturesSupport */ >> +enum { >> + UFS_DEV_WRITE_BOOSTER_SUP = BIT(8), >> +}; >> + >> #define POWER_DESC_MAX_SIZE 0x62 >> #define POWER_DESC_MAX_ACTV_ICC_LVLS 16 >> >> @@ -447,6 +470,38 @@ enum ufs_dev_pwr_mode { >> UFS_POWERDOWN_PWR_MODE = 3, >> }; >> >> +enum ufs_dev_wb_buf_avail_size { >> + UFS_WB_0_PERCENT_BUF_REMAIN = 0x0, >> + UFS_WB_10_PERCENT_BUF_REMAIN = 0x1, >> + UFS_WB_20_PERCENT_BUF_REMAIN = 0x2, >> + UFS_WB_30_PERCENT_BUF_REMAIN = 0x3, >> + UFS_WB_40_PERCENT_BUF_REMAIN = 0x4, >> + UFS_WB_50_PERCENT_BUF_REMAIN = 0x5, >> + UFS_WB_60_PERCENT_BUF_REMAIN = 0x6, >> + UFS_WB_70_PERCENT_BUF_REMAIN = 0x7, >> + UFS_WB_80_PERCENT_BUF_REMAIN = 0x8, >> + UFS_WB_90_PERCENT_BUF_REMAIN = 0x9, >> + UFS_WB_100_PERCENT_BUF_REMAIN = 0xA, >> +}; > Why do need all this? > Just define the threshold you need. Ok. > >> + >> +enum ufs_dev_wb_buf_life_time_est { >> + UFS_WB_0_10_PERCENT_USED = 0x1, >> + UFS_WB_10_20_PERCENT_USED = 0x2, >> + UFS_WB_20_30_PERCENT_USED = 0x3, >> + UFS_WB_30_40_PERCENT_USED = 0x4, >> + UFS_WB_40_50_PERCENT_USED = 0x5, >> + UFS_WB_50_60_PERCENT_USED = 0x6, >> + UFS_WB_60_70_PERCENT_USED = 0x7, >> + UFS_WB_70_80_PERCENT_USED = 0x8, >> + UFS_WB_80_90_PERCENT_USED = 0x9, >> + UFS_WB_90_100_PERCENT_USED = 0xA, >> + UFS_WB_MAX_USED = 0xB, >> +}; > Here also. Ok. > > >> + >> +enum ufs_dev_wb_buf_user_cap_config { >> + UFS_WB_BUFF_PRESERVE_USER_SPACE = 1, >> + UFS_WB_BUFF_USER_SPACE_RED_EN = 2, >> +}; >> /** >> * struct utp_cmd_rsp - Response UPIU structure >> * @residual_transfer_count: Residual transfer count DW-3 >> @@ -537,6 +592,10 @@ struct ufs_dev_info { >> u8 *model; >> u16 wspecversion; >> u32 clk_gating_wait_us; >> + u32 d_ext_ufs_feature_sup; >> + u8 b_wb_buffer_type; >> + bool wb_config_lun; >> + bool keep_vcc_on; > doc please - proper struct doc this time I didn't understand this comment. What exactly should I change/add here? > >> }; >> >> /** >> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c >> index f4aa10f..4cc1fb4 100644 >> --- a/drivers/scsi/ufs/ufshcd.c >> +++ b/drivers/scsi/ufs/ufshcd.c >> @@ -47,6 +47,7 @@ >> #include "unipro.h" >> #include "ufs-sysfs.h" >> #include "ufs_bsg.h" >> +#include <linux/blkdev.h> >> >> #define CREATE_TRACE_POINTS >> #include <trace/events/ufs.h> >> @@ -263,6 +264,13 @@ static int ufshcd_scale_clks(struct ufs_hba *hba, >> bool scale_up); >> static irqreturn_t ufshcd_intr(int irq, void *__hba); >> static int ufshcd_change_power_mode(struct ufs_hba *hba, >> struct ufs_pa_layer_attr *pwr_mode); >> +static bool ufshcd_wb_sup(struct ufs_hba *hba); >> +static int ufshcd_wb_ctrl(struct ufs_hba *hba, bool enable); >> +static int ufshcd_wb_buf_flush_enable(struct ufs_hba *hba); >> +static int ufshcd_wb_buf_flush_disable(struct ufs_hba *hba); >> +static bool ufshcd_wb_is_buf_flush_needed(struct ufs_hba *hba); >> +static int ufshcd_wb_toggle_flush_during_h8(struct ufs_hba *hba, bool >> set); >> + >> static inline bool ufshcd_valid_tag(struct ufs_hba *hba, int tag) >> { >> return tag >= 0 && tag < hba->nutrs; >> @@ -284,6 +292,38 @@ static inline void ufshcd_disable_irq(struct ufs_hba >> *hba) >> } >> } >> >> +static inline void ufshcd_wb_toggle_flush(struct ufs_hba *hba) >> +{ >> + /* >> + * Query dAvailableWriteBoosterBufferSize attribute and enable >> + * the Write BoosterBuffer Flush if only 30% Write Booster >> + * Buffer is available. >> + * In reduction case, flush only if 10% is available >> + */ >> + if (ufshcd_wb_is_buf_flush_needed(hba)) >> + ufshcd_wb_buf_flush_enable(hba); >> + else >> + ufshcd_wb_buf_flush_disable(hba); >> +} > There are voices calling to set flush on hibernate on init, and never touch it again. > I think you need, both in your commit log, and here as well, > add a comment explaining why toggling is even needed. Yes - I heard those voices too :) I'll change this. > >> + >> +static inline void ufshcd_wb_config(struct ufs_hba *hba) >> +{ >> + int ret; >> + >> + if (!ufshcd_wb_sup(hba)) >> + return; >> + >> + ret = ufshcd_wb_ctrl(hba, true); >> + if (ret) >> + dev_err(hba->dev, "%s: Enable WB failed: %d\n", __func__, ret); >> + else >> + dev_info(hba->dev, "%s: Write Booster Configured\n", __func__); >> + ret = ufshcd_wb_toggle_flush_during_h8(hba, true); >> + if (ret) >> + dev_err(hba->dev, "%s: En WB flush during H8: failed: %d\n", >> + __func__, ret); >> +} >> + >> static void ufshcd_scsi_unblock_requests(struct ufs_hba *hba) >> { >> if (atomic_dec_and_test(&hba->scsi_block_reqs_cnt)) >> @@ -1139,6 +1179,8 @@ static int ufshcd_devfreq_scale(struct ufs_hba >> *hba, bool scale_up) >> } >> >> ret = ufshcd_vops_clk_scale_notify(hba, scale_up, POST_CHANGE); >> + /* Enable Write Booster if we have scaled up else disable it */ >> + ufshcd_wb_ctrl(hba, scale_up); >> >> out: >> ufshcd_clock_scaling_unprepare(hba); >> @@ -4572,6 +4614,37 @@ static int ufshcd_get_lu_wp(struct ufs_hba *hba, >> return ret; >> } >> >> +/* >> + * ufshcd_get_wb_alloc_units - returns >> "dLUNumWriteBoosterBufferAllocUnits" >> + * @hba: per-adapter instance >> + * @lun: UFS device lun id >> + * @d_lun_wbb_au: pointer to buffer to hold the LU's alloc units info >> + * >> + * Returns 0 in case of success and d_lun_wbb_au would be returned >> + * Returns -ENOTSUPP if reading d_lun_wbb_au is not supported. >> + * Returns -EINVAL in case of invalid parameters passed to this function. >> + */ >> +static int ufshcd_get_wb_alloc_units(struct ufs_hba *hba, >> + u8 lun, >> + u8 *d_lun_wbb_au) >> +{ >> + int ret; >> + >> + if (!d_lun_wbb_au) >> + ret = -EINVAL; >> + >> + /* WB can be supported only from LU0..LU7 */ >> + else if (lun >= UFS_UPIU_MAX_GENERAL_LUN) >> + ret = -ENOTSUPP; >> + else >> + ret = ufshcd_read_unit_desc_param(hba, >> + lun, >> + UNIT_DESC_PARAM_WB_BUF_ALLOC_UNITS, >> + d_lun_wbb_au, >> + sizeof(*d_lun_wbb_au)); >> + return ret; >> +} >> + >> /** >> * ufshcd_get_lu_power_on_wp_status - get LU's power on write protect >> * status >> @@ -5194,6 +5267,165 @@ static void >> ufshcd_bkops_exception_event_handler(struct ufs_hba *hba) >> __func__, err); >> } >> >> +static bool ufshcd_wb_sup(struct ufs_hba *hba) >> +{ >> + return ((hba->dev_info.d_ext_ufs_feature_sup & >> + UFS_DEV_WRITE_BOOSTER_SUP) && >> + (hba->dev_info.b_wb_buffer_type >> + || hba->dev_info.wb_config_lun)); >> +} >> + >> +static int ufshcd_wb_ctrl(struct ufs_hba *hba, bool enable) >> +{ >> + int ret; >> + enum query_opcode opcode; >> + >> + if (!ufshcd_wb_sup(hba)) >> + return 0; >> + >> + if (enable) >> + opcode = UPIU_QUERY_OPCODE_SET_FLAG; >> + else >> + opcode = UPIU_QUERY_OPCODE_CLEAR_FLAG; > If already wb_enabled is enabled/disabled no point to send the query request True. I'll add a check before sending the query request. > >> + >> + ret = ufshcd_query_flag_retry(hba, opcode, >> + QUERY_FLAG_IDN_WB_EN, NULL); > If we are in a non-shared-buffer mode - you need to send the flag to the applicable lun. > WB introduces the first not-device-level flags. > Need to adjust ufshcd_query_flag for it - in a preparatory patch... Ok. This current patch didn't support dedicated buffer mode. It supported only shared-buffer mode. I'll add a comment clarifying that. I'll add the dedicated buffer mode support at a later stage, not with this series. > > Same goes to the rest of your code that query flags and attributes > Explained in the comment above. >> + if (ret) { >> + dev_err(hba->dev, "%s write booster %s failed %d\n", >> + __func__, enable ? "enable" : "disable", ret); >> + return ret; >> + } >> + >> + hba->wb_enabled = enable; >> + dev_dbg(hba->dev, "%s write booster %s %d\n", >> + __func__, enable ? "enable" : "disable", ret); >> + >> + return ret; >> +} >> + >> +static int ufshcd_wb_toggle_flush_during_h8(struct ufs_hba *hba, bool set) >> +{ >> + int val; >> + >> + if (set) >> + val = UPIU_QUERY_OPCODE_SET_FLAG; >> + else >> + val = UPIU_QUERY_OPCODE_CLEAR_FLAG; >> + >> + return ufshcd_query_flag_retry(hba, val, >> + QUERY_FLAG_IDN_WB_BUFF_FLUSH_DURING_HIBERN8, >> + NULL); >> +} >> + >> +static int ufshcd_wb_buf_flush_enable(struct ufs_hba *hba) >> +{ >> + int ret; >> + >> + if (!ufshcd_wb_sup(hba) || hba->wb_buf_flush_enabled) >> + return 0; >> + >> + ret = ufshcd_query_flag_retry(hba, UPIU_QUERY_OPCODE_SET_FLAG, >> + QUERY_FLAG_IDN_WB_BUFF_FLUSH_EN, NULL); >> + if (ret) >> + dev_err(hba->dev, "%s WB - buf flush enable failed %d\n", >> + __func__, ret); >> + else >> + hba->wb_buf_flush_enabled = true; >> + >> + dev_dbg(hba->dev, "WB - Flush enabled: %d\n", ret); >> + return ret; >> +} >> + >> +static int ufshcd_wb_buf_flush_disable(struct ufs_hba *hba) >> +{ >> + int ret; >> + >> + if (!ufshcd_wb_sup(hba) || !hba->wb_buf_flush_enabled) >> + return 0; >> + >> + ret = ufshcd_query_flag_retry(hba, >> UPIU_QUERY_OPCODE_CLEAR_FLAG, >> + QUERY_FLAG_IDN_WB_BUFF_FLUSH_EN, NULL); >> + if (ret) { >> + dev_warn(hba->dev, "%s: WB - buf flush disable failed %d\n", >> + __func__, ret); >> + } else { >> + hba->wb_buf_flush_enabled = false; >> + dev_dbg(hba->dev, "WB - Flush disabled: %d\n", ret); >> + } >> + >> + return ret; >> +} >> + >> +static bool ufshcd_wb_is_buf_flush_needed(struct ufs_hba *hba) >> +{ >> + int ret; >> + u32 cur_buf, status, avail_buf; >> + >> + if (!ufshcd_wb_sup(hba)) >> + return false; >> + >> + ret = ufshcd_query_attr_retry(hba, >> UPIU_QUERY_OPCODE_READ_ATTR, >> + QUERY_ATTR_IDN_AVAIL_WB_BUFF_SIZE, >> + 0, 0, &avail_buf); >> + if (ret) { >> + dev_warn(hba->dev, "%s dAvailableWriteBoosterBufferSize read >> failed %d\n", >> + __func__, ret); >> + return false; >> + } >> + >> + ret = ufshcd_vops_get_user_cap_mode(hba); >> + if (ret <= 0) { >> + dev_dbg(hba->dev, "Get user-cap reduction mode: failed: %d\n", >> + ret); >> + /* Most commonly used */ >> + ret = UFS_WB_BUFF_PRESERVE_USER_SPACE; >> + } > Not sure I understand how flush is related to the reduced/non-reduced buffer config? Its not. Current code distinguishes both the buffer configs to check for current_buffer capacity (dCurrentWriteBoosterBufferSize). In case of user-space reduction config, there's no need to check the current buffer capacity. > >> + >> + hba->dev_info.keep_vcc_on = false; >> + if (ret == UFS_WB_BUFF_USER_SPACE_RED_EN) { >> + if (avail_buf <= UFS_WB_10_PERCENT_BUF_REMAIN) { >> + hba->dev_info.keep_vcc_on = true; >> + return true; >> + } >> + return false; >> + } else if (ret == UFS_WB_BUFF_PRESERVE_USER_SPACE) { >> + ret = ufshcd_query_attr_retry(hba, >> UPIU_QUERY_OPCODE_READ_ATTR, >> + QUERY_ATTR_IDN_CURR_WB_BUFF_SIZE, >> + 0, 0, &cur_buf); >> + if (ret) { >> + dev_err(hba->dev, "%s dCurWriteBoosterBufferSize read failed >> %d\n", >> + __func__, ret); >> + return false; >> + } >> + >> + if (!cur_buf) { >> + dev_info(hba->dev, "dCurWBBuf: %d WB disabled until free- >> space is available\n", >> + cur_buf); >> + return false; >> + } >> + >> + ret = ufshcd_get_ee_status(hba, &status); > Responding to urgent flush exception event, should you decide to add it, > Should be done as part of ufshcd_exception_event_handler, and not here. > This code checks for the exception status because its being invoked from suspend. I think its a good practice to proactively check for the ee status here and keep the vcc on if the device needs it. >> + if (ret) { >> + dev_err(hba->dev, "%s: failed to get exception status %d\n", >> + __func__, ret); >> + if (avail_buf < UFS_WB_40_PERCENT_BUF_REMAIN) { >> + hba->dev_info.keep_vcc_on = true; >> + return true; >> + } >> + return false; >> + } >> + >> + status &= hba->ee_ctrl_mask; >> + >> + if ((status & MASK_EE_URGENT_BKOPS) || >> + (avail_buf < UFS_WB_40_PERCENT_BUF_REMAIN)) { >> + hba->dev_info.keep_vcc_on = true; >> + return true; >> + } >> + } >> + return false; > This function follow some wired logic, that needs to be further explained > This is what it tries to achieve: Invoked from suspend: Check the avail_buf status: If preserve_user_space enabled: Check for current_buf status: If cur_buf == 0: Return - No point enabling flush, since it can't flush Check if there's a bkops exception || if the avail_buf < 40%: keep_vcc_on = true This is to achieve the delicate balance between power and performance. >> +} >> + >> /** >> * ufshcd_exception_event_handler - handle exceptions raised by device >> * @work: pointer to work data >> @@ -6629,7 +6861,8 @@ static int ufs_get_device_desc(struct ufs_hba >> *hba) >> int err; >> size_t buff_len; >> u8 model_index; >> - u8 *desc_buf; >> + u8 *desc_buf, wb_buf[4]; >> + u32 lun, res; >> struct ufs_dev_info *dev_info = &hba->dev_info; >> >> buff_len = max_t(size_t, hba->desc_size.dev_desc, >> @@ -6660,6 +6893,40 @@ static int ufs_get_device_desc(struct ufs_hba >> *hba) >> desc_buf[DEVICE_DESC_PARAM_SPEC_VER + 1]; >> >> model_index = desc_buf[DEVICE_DESC_PARAM_PRDCT_NAME]; >> + /* Enable WB only for UFS-3.1 OR if desc len >= 0x59 */ >> + if (dev_info->wspecversion >= 0x310) { >> + hba->dev_info.d_ext_ufs_feature_sup = >> + desc_buf[DEVICE_DESC_PARAM_EXT_UFS_FEATURE_SUP] >> + << 24 | >> + desc_buf[DEVICE_DESC_PARAM_EXT_UFS_FEATURE_SUP + 1] >> + << 16 | >> + desc_buf[DEVICE_DESC_PARAM_EXT_UFS_FEATURE_SUP + 2] >> + << 8 | >> + desc_buf[DEVICE_DESC_PARAM_EXT_UFS_FEATURE_SUP + 3]; >> + >> + hba->dev_info.b_wb_buffer_type = >> + desc_buf[DEVICE_DESC_PARAM_WB_TYPE]; >> + >> + if (hba->dev_info.b_wb_buffer_type) >> + goto skip_unit_desc; > Comment please, e.g.: > /* bWriteBoosterBufferType indicates that the WB buffer is configured as a shared buffer mode */ > And instead the goto pack iterating the unit descriptors in a helper?Sure. I'll try and rework and reword this. > >> + >> + hba->dev_info.wb_config_lun = false; >> + for (lun = 0; lun < UFS_UPIU_MAX_GENERAL_LUN; lun++) { >> + memset(wb_buf, 0, sizeof(wb_buf)); >> + err = ufshcd_get_wb_alloc_units(hba, lun, wb_buf); >> + if (err) >> + break; >> + >> + res = wb_buf[0] << 24 | wb_buf[1] << 16 | >> + wb_buf[2] << 8 | wb_buf[3]; >> + if (res) { >> + hba->dev_info.wb_config_lun = true; >> + break; > Maybe you want to be aware which luns has a WB buffer, and of its WB buffer size. > And as the clock scaling mechanism is unware of which writes goes to which lun, > Explain that as well. > This series doesn't support per lun WB buffer configuration. But yes, I can explain that here. >> + } >> + } >> + } >> + >> +skip_unit_desc: >> err = ufshcd_read_string_desc(hba, model_index, >> &dev_info->model, SD_ASCII_STD); >> if (err < 0) { >> @@ -7162,6 +7429,7 @@ static int ufshcd_probe_hba(struct ufs_hba *hba, >> bool async) >> >> /* set the state as operational after switching to desired gear */ >> hba->ufshcd_state = UFSHCD_STATE_OPERATIONAL; >> + ufshcd_wb_config(hba); > Enabling WB regardless of clock scaling on init? > Why not just ufshcd_wb_toggle_flush_during_h8() I'll look into this. Thanks! > >> >> /* Enable Auto-Hibernate if configured */ >> ufshcd_auto_hibern8_enable(hba); >> @@ -7823,12 +8091,16 @@ static void ufshcd_vreg_set_lpm(struct ufs_hba >> *hba) >> * >> * Ignore the error returned by ufshcd_toggle_vreg() as device is anyway >> * in low power state which would save some power. >> + * >> + * If Write Booster is enabled and the device needs to flush the WB >> + * buffer OR if bkops status is urgent for WB, keep Vcc on. > I am not sure that you want to entail WB and bkops together in the same patch. > If you are changing the logic of this function with respect of bkops - > Do that In a separate patch, not related to WB. > I think it's kinda related, with WB some of the device's bkops would be related to WB as well. Can you please confirm to me if that's not the case. I was told that's the case though. >> */ >> if (ufshcd_is_ufs_dev_poweroff(hba) && ufshcd_is_link_off(hba) && >> !hba->dev_info.is_lu_power_on_wp) { >> ufshcd_setup_vreg(hba, false); >> } else if (!ufshcd_is_ufs_dev_active(hba)) { >> - ufshcd_toggle_vreg(hba->dev, hba->vreg_info.vcc, false); >> + if (!hba->dev_info.keep_vcc_on) >> + ufshcd_toggle_vreg(hba->dev, hba->vreg_info.vcc, false); >> if (!ufshcd_is_link_active(hba)) { >> ufshcd_config_vreg_lpm(hba, hba->vreg_info.vccq); >> ufshcd_config_vreg_lpm(hba, hba->vreg_info.vccq2); >> @@ -7952,11 +8224,16 @@ static int ufshcd_suspend(struct ufs_hba *hba, >> enum ufs_pm_op pm_op) >> /* make sure that auto bkops is disabled */ >> ufshcd_disable_auto_bkops(hba); >> } >> + ufshcd_wb_toggle_flush(hba); >> + } else if (!ufshcd_is_runtime_pm(pm_op)) { >> + ufshcd_wb_buf_flush_disable(hba); >> + hba->dev_info.keep_vcc_on = false; >> } >> >> if ((req_dev_pwr_mode != hba->curr_dev_pwr_mode) && >> - ((ufshcd_is_runtime_pm(pm_op) && !hba->auto_bkops_enabled) || >> - !ufshcd_is_runtime_pm(pm_op))) { >> + ((ufshcd_is_runtime_pm(pm_op) && (!hba->auto_bkops_enabled) >> + && !hba->wb_buf_flush_enabled) || >> + !ufshcd_is_runtime_pm(pm_op))) { >> /* ensure that bkops is disabled */ >> ufshcd_disable_auto_bkops(hba); >> ret = ufshcd_set_dev_pwr_mode(hba, req_dev_pwr_mode); >> @@ -8078,6 +8355,7 @@ static int ufshcd_resume(struct ufs_hba *hba, >> enum ufs_pm_op pm_op) >> goto vendor_suspend; >> } >> >> + ufshcd_wb_buf_flush_disable(hba); >> if (!ufshcd_is_ufs_dev_active(hba)) { >> ret = ufshcd_set_dev_pwr_mode(hba, UFS_ACTIVE_PWR_MODE); >> if (ret) >> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h >> index 8f516b2..6f36ff3 100644 >> --- a/drivers/scsi/ufs/ufshcd.h >> +++ b/drivers/scsi/ufs/ufshcd.h >> @@ -327,6 +327,7 @@ struct ufs_hba_variant_ops { >> void (*dbg_register_dump)(struct ufs_hba *hba); >> int (*phy_initialization)(struct ufs_hba *); >> void (*device_reset)(struct ufs_hba *hba); >> + u32 (*get_user_cap_mode)(struct ufs_hba *hba); > Make the name indicate that this is WB related? > Ok. >> }; >> >> /* clock gating state */ >> @@ -731,6 +732,8 @@ struct ufs_hba { >> >> struct device bsg_dev; >> struct request_queue *bsg_queue; >> + bool wb_buf_flush_enabled; >> + bool wb_enabled; >> }; >> >> /* Returns true if clocks can be gated. Otherwise false */ >> @@ -1123,4 +1126,10 @@ static inline u8 ufshcd_scsi_to_upiu_lun(unsigned >> int scsi_lun) >> int ufshcd_dump_regs(struct ufs_hba *hba, size_t offset, size_t len, >> const char *prefix); >> >> +static inline unsigned int ufshcd_vops_get_user_cap_mode(struct ufs_hba >> *hba) >> +{ >> + if (hba->vops && hba->vops->get_user_cap_mode) >> + return hba->vops->get_user_cap_mode(hba); >> + return 0; >> +} >> #endif /* End of Header */ >> -- >> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a >> Linux Foundation Collaborative Project. > > > Thanks, > Avri > -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, Linux Foundation Collaborative Project ^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [<RFC PATCH v1> 1/2] scsi: ufs: add write booster feature support 2020-02-22 0:47 ` [<RFC PATCH v1> 1/2] scsi: ufs: add write booster feature support Asutosh Das 2020-02-23 9:03 ` Avri Altman @ 2020-02-25 10:58 ` Avri Altman 2020-02-25 11:21 ` Avri Altman 2020-02-25 12:50 ` Avri Altman 2 siblings, 1 reply; 9+ messages in thread From: Avri Altman @ 2020-02-25 10:58 UTC (permalink / raw) To: Asutosh Das, subhashj, cang, rnayak, vinholikatti, jejb, martin.petersen, linux-scsi Cc: linux-arm-msm, Alim Akhtar, James E.J. Bottomley, Stanley Chu, Bean Huo, Tomas Winkler, Colin Ian King, Bart Van Assche, Venkat Gopalakrishnan, Bjorn Andersson, open list > + /* Enable WB only for UFS-3.1 OR if desc len >= 0x59 */ You don't really check that the descriptor size is large enough > + if (dev_info->wspecversion >= 0x310) { > + hba->dev_info.d_ext_ufs_feature_sup = > + desc_buf[DEVICE_DESC_PARAM_EXT_UFS_FEATURE_SUP] > + << 24 | > + desc_buf[DEVICE_DESC_PARAM_EXT_UFS_FEATURE_SUP + 1] > + << 16 | > + desc_buf[DEVICE_DESC_PARAM_EXT_UFS_FEATURE_SUP + 2] > + << 8 | > + desc_buf[DEVICE_DESC_PARAM_EXT_UFS_FEATURE_SUP + 3]; > + > + hba->dev_info.b_wb_buffer_type = > + desc_buf[DEVICE_DESC_PARAM_WB_TYPE]; > + > + if (hba->dev_info.b_wb_buffer_type) > + goto skip_unit_desc; > + > + hba->dev_info.wb_config_lun = false; > + for (lun = 0; lun < UFS_UPIU_MAX_GENERAL_LUN; lun++) { You forgot to set u8 wb_buf[4] = {}; > + memset(wb_buf, 0, sizeof(wb_buf)); Not needed > + err = ufshcd_get_wb_alloc_units(hba, lun, wb_buf); > + if (err) > + break; > + > + res = wb_buf[0] << 24 | wb_buf[1] << 16 | > + wb_buf[2] << 8 | wb_buf[3]; > + if (res) { > + hba->dev_info.wb_config_lun = true; > + break; > + } > + } > + } > + Thanks, Avri ^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [<RFC PATCH v1> 1/2] scsi: ufs: add write booster feature support 2020-02-25 10:58 ` Avri Altman @ 2020-02-25 11:21 ` Avri Altman 0 siblings, 0 replies; 9+ messages in thread From: Avri Altman @ 2020-02-25 11:21 UTC (permalink / raw) To: Asutosh Das, subhashj, cang, rnayak, vinholikatti, jejb, martin.petersen, linux-scsi Cc: linux-arm-msm, Alim Akhtar, James E.J. Bottomley, Stanley Chu, Bean Huo, Tomas Winkler, Colin Ian King, Bart Van Assche, Venkat Gopalakrishnan, Bjorn Andersson, open list > > + /* Enable WB only for UFS-3.1 OR if desc len >= 0x59 */ > You don't really check that the descriptor size is large enough > > > + if (dev_info->wspecversion >= 0x310) { > > + hba->dev_info.d_ext_ufs_feature_sup = > > + desc_buf[DEVICE_DESC_PARAM_EXT_UFS_FEATURE_SUP] > > + << 24 | > > + desc_buf[DEVICE_DESC_PARAM_EXT_UFS_FEATURE_SUP + > 1] > > + << 16 | > > + desc_buf[DEVICE_DESC_PARAM_EXT_UFS_FEATURE_SUP + > 2] > > + << 8 | > > + desc_buf[DEVICE_DESC_PARAM_EXT_UFS_FEATURE_SUP + > 3]; > > + > > + hba->dev_info.b_wb_buffer_type = > > + desc_buf[DEVICE_DESC_PARAM_WB_TYPE]; > > + > > + if (hba->dev_info.b_wb_buffer_type) > > + goto skip_unit_desc; > > + > > + hba->dev_info.wb_config_lun = false; > > + for (lun = 0; lun < UFS_UPIU_MAX_GENERAL_LUN; lun++) { Also lun not defined > You forgot to set > u8 wb_buf[4] = {}; > > > + memset(wb_buf, 0, sizeof(wb_buf)); > Not needed > > > + err = ufshcd_get_wb_alloc_units(hba, lun, wb_buf); > > + if (err) > > + break; > > + > > + res = wb_buf[0] << 24 | wb_buf[1] << 16 | > > + wb_buf[2] << 8 | wb_buf[3]; And res not defined as well. > > + if (res) { > > + hba->dev_info.wb_config_lun = true; > > + break; > > + } > > + } > > + } > > + > > Thanks, > Avri ^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [<RFC PATCH v1> 1/2] scsi: ufs: add write booster feature support 2020-02-22 0:47 ` [<RFC PATCH v1> 1/2] scsi: ufs: add write booster feature support Asutosh Das 2020-02-23 9:03 ` Avri Altman 2020-02-25 10:58 ` Avri Altman @ 2020-02-25 12:50 ` Avri Altman 2020-02-26 17:57 ` Asutosh Das (asd) 2020-03-16 23:52 ` Asutosh Das (asd) 2 siblings, 2 replies; 9+ messages in thread From: Avri Altman @ 2020-02-25 12:50 UTC (permalink / raw) To: Asutosh Das, subhashj, cang, rnayak, vinholikatti, jejb, martin.petersen, linux-scsi Cc: linux-arm-msm, Alim Akhtar, James E.J. Bottomley, Stanley Chu, Bean Huo, Tomas Winkler, Colin Ian King, Bart Van Assche, Venkat Gopalakrishnan, Bjorn Andersson, open list > +/* > + * ufshcd_get_wb_alloc_units - returns > "dLUNumWriteBoosterBufferAllocUnits" > + * @hba: per-adapter instance > + * @lun: UFS device lun id > + * @d_lun_wbb_au: pointer to buffer to hold the LU's alloc units info > + * > + * Returns 0 in case of success and d_lun_wbb_au would be returned > + * Returns -ENOTSUPP if reading d_lun_wbb_au is not supported. > + * Returns -EINVAL in case of invalid parameters passed to this function. > + */ > +static int ufshcd_get_wb_alloc_units(struct ufs_hba *hba, > + u8 lun, > + u8 *d_lun_wbb_au) > +{ > + int ret; > + > + if (!d_lun_wbb_au) > + ret = -EINVAL; > + > + /* WB can be supported only from LU0..LU7 */ > + else if (lun >= UFS_UPIU_MAX_GENERAL_LUN) > + ret = -ENOTSUPP; > + else > + ret = ufshcd_read_unit_desc_param(hba, > + lun, > + UNIT_DESC_PARAM_WB_BUF_ALLOC_UNITS, > + d_lun_wbb_au, > + sizeof(*d_lun_wbb_au)); You are reading here a single byte, instead of 4 > + return ret; > +} > + > /** > * ufshcd_get_lu_power_on_wp_status - get LU's power on write protect > * status > @@ -5194,6 +5267,165 @@ static void > ufshcd_bkops_exception_event_handler(struct ufs_hba *hba) > __func__, err); > } > > +static bool ufshcd_wb_sup(struct ufs_hba *hba) > +{ > + return ((hba->dev_info.d_ext_ufs_feature_sup & > + UFS_DEV_WRITE_BOOSTER_SUP) && Don't you want to have a vendor cap as well, to allow the platform vendor to control this feature? > + (hba->dev_info.b_wb_buffer_type > + || hba->dev_info.wb_config_lun)); > +} > + > + > +static bool ufshcd_wb_is_buf_flush_needed(struct ufs_hba *hba) > +{ > + int ret; > + u32 cur_buf, status, avail_buf; > + > + if (!ufshcd_wb_sup(hba)) > + return false; > + > + ret = ufshcd_query_attr_retry(hba, > UPIU_QUERY_OPCODE_READ_ATTR, > + QUERY_ATTR_IDN_AVAIL_WB_BUFF_SIZE, > + 0, 0, &avail_buf); > + if (ret) { > + dev_warn(hba->dev, "%s dAvailableWriteBoosterBufferSize read > failed %d\n", > + __func__, ret); > + return false; > + } > + > + ret = ufshcd_vops_get_user_cap_mode(hba); > + if (ret <= 0) { > + dev_dbg(hba->dev, "Get user-cap reduction mode: failed: %d\n", > + ret); > + /* Most commonly used */ > + ret = UFS_WB_BUFF_PRESERVE_USER_SPACE; > + } > + > + hba->dev_info.keep_vcc_on = false; > + if (ret == UFS_WB_BUFF_USER_SPACE_RED_EN) { > + if (avail_buf <= UFS_WB_10_PERCENT_BUF_REMAIN) { > + hba->dev_info.keep_vcc_on = true; > + return true; > + } > + return false; > + } else if (ret == UFS_WB_BUFF_PRESERVE_USER_SPACE) { > + ret = ufshcd_query_attr_retry(hba, > UPIU_QUERY_OPCODE_READ_ATTR, > + QUERY_ATTR_IDN_CURR_WB_BUFF_SIZE, > + 0, 0, &cur_buf); > + if (ret) { > + dev_err(hba->dev, "%s dCurWriteBoosterBufferSize read failed > %d\n", > + __func__, ret); > + return false; > + } > + > + if (!cur_buf) { > + dev_info(hba->dev, "dCurWBBuf: %d WB disabled until free- > space is available\n", > + cur_buf); > + return false; > + } > + > + ret = ufshcd_get_ee_status(hba, &status); > + if (ret) { > + dev_err(hba->dev, "%s: failed to get exception status %d\n", > + __func__, ret); > + if (avail_buf < UFS_WB_40_PERCENT_BUF_REMAIN) { > + hba->dev_info.keep_vcc_on = true; > + return true; > + } > + return false; > + } > + > + status &= hba->ee_ctrl_mask; > + > + if ((status & MASK_EE_URGENT_BKOPS) || So you are getting the status, but not the bkops level. And what about WRITEBOOSTER_EVENT_EN? After all it was invented specifically for WB... > + (avail_buf < UFS_WB_40_PERCENT_BUF_REMAIN)) { > + hba->dev_info.keep_vcc_on = true; > + return true; > + } > + } > + return false; > +} Thanks, Avri ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [<RFC PATCH v1> 1/2] scsi: ufs: add write booster feature support 2020-02-25 12:50 ` Avri Altman @ 2020-02-26 17:57 ` Asutosh Das (asd) 2020-03-16 23:52 ` Asutosh Das (asd) 1 sibling, 0 replies; 9+ messages in thread From: Asutosh Das (asd) @ 2020-02-26 17:57 UTC (permalink / raw) To: Avri Altman, subhashj, cang, rnayak, vinholikatti, jejb, martin.petersen, linux-scsi Cc: linux-arm-msm, Alim Akhtar, James E.J. Bottomley, Stanley Chu, Bean Huo, Tomas Winkler, Colin Ian King, Bart Van Assche, Venkat Gopalakrishnan, Bjorn Andersson, open list On 2/25/2020 4:50 AM, Avri Altman wrote: >> +/* >> + * ufshcd_get_wb_alloc_units - returns >> "dLUNumWriteBoosterBufferAllocUnits" >> + * @hba: per-adapter instance >> + * @lun: UFS device lun id >> + * @d_lun_wbb_au: pointer to buffer to hold the LU's alloc units info >> + * >> + * Returns 0 in case of success and d_lun_wbb_au would be returned >> + * Returns -ENOTSUPP if reading d_lun_wbb_au is not supported. >> + * Returns -EINVAL in case of invalid parameters passed to this function. >> + */ >> +static int ufshcd_get_wb_alloc_units(struct ufs_hba *hba, >> + u8 lun, >> + u8 *d_lun_wbb_au) >> +{ >> + int ret; >> + >> + if (!d_lun_wbb_au) >> + ret = -EINVAL; >> + >> + /* WB can be supported only from LU0..LU7 */ >> + else if (lun >= UFS_UPIU_MAX_GENERAL_LUN) >> + ret = -ENOTSUPP; >> + else >> + ret = ufshcd_read_unit_desc_param(hba, >> + lun, >> + UNIT_DESC_PARAM_WB_BUF_ALLOC_UNITS, >> + d_lun_wbb_au, >> + sizeof(*d_lun_wbb_au)); > You are reading here a single byte, instead of 4 > >> + return ret; >> +} >> + >> /** >> * ufshcd_get_lu_power_on_wp_status - get LU's power on write protect >> * status >> @@ -5194,6 +5267,165 @@ static void >> ufshcd_bkops_exception_event_handler(struct ufs_hba *hba) >> __func__, err); >> } >> >> +static bool ufshcd_wb_sup(struct ufs_hba *hba) >> +{ >> + return ((hba->dev_info.d_ext_ufs_feature_sup & >> + UFS_DEV_WRITE_BOOSTER_SUP) && > Don't you want to have a vendor cap as well, > to allow the platform vendor to control this feature? > >> + (hba->dev_info.b_wb_buffer_type >> + || hba->dev_info.wb_config_lun)); >> +} >> + >> + > > > >> +static bool ufshcd_wb_is_buf_flush_needed(struct ufs_hba *hba) >> +{ >> + int ret; >> + u32 cur_buf, status, avail_buf; >> + >> + if (!ufshcd_wb_sup(hba)) >> + return false; >> + >> + ret = ufshcd_query_attr_retry(hba, >> UPIU_QUERY_OPCODE_READ_ATTR, >> + QUERY_ATTR_IDN_AVAIL_WB_BUFF_SIZE, >> + 0, 0, &avail_buf); >> + if (ret) { >> + dev_warn(hba->dev, "%s dAvailableWriteBoosterBufferSize read >> failed %d\n", >> + __func__, ret); >> + return false; >> + } >> + >> + ret = ufshcd_vops_get_user_cap_mode(hba); >> + if (ret <= 0) { >> + dev_dbg(hba->dev, "Get user-cap reduction mode: failed: %d\n", >> + ret); >> + /* Most commonly used */ >> + ret = UFS_WB_BUFF_PRESERVE_USER_SPACE; >> + } >> + >> + hba->dev_info.keep_vcc_on = false; >> + if (ret == UFS_WB_BUFF_USER_SPACE_RED_EN) { >> + if (avail_buf <= UFS_WB_10_PERCENT_BUF_REMAIN) { >> + hba->dev_info.keep_vcc_on = true; >> + return true; >> + } >> + return false; >> + } else if (ret == UFS_WB_BUFF_PRESERVE_USER_SPACE) { >> + ret = ufshcd_query_attr_retry(hba, >> UPIU_QUERY_OPCODE_READ_ATTR, >> + QUERY_ATTR_IDN_CURR_WB_BUFF_SIZE, >> + 0, 0, &cur_buf); >> + if (ret) { >> + dev_err(hba->dev, "%s dCurWriteBoosterBufferSize read failed >> %d\n", >> + __func__, ret); >> + return false; >> + } >> + >> + if (!cur_buf) { >> + dev_info(hba->dev, "dCurWBBuf: %d WB disabled until free- >> space is available\n", >> + cur_buf); >> + return false; >> + } >> + >> + ret = ufshcd_get_ee_status(hba, &status); >> + if (ret) { >> + dev_err(hba->dev, "%s: failed to get exception status %d\n", >> + __func__, ret); >> + if (avail_buf < UFS_WB_40_PERCENT_BUF_REMAIN) { >> + hba->dev_info.keep_vcc_on = true; >> + return true; >> + } >> + return false; >> + } >> + >> + status &= hba->ee_ctrl_mask; >> + >> + if ((status & MASK_EE_URGENT_BKOPS) || > So you are getting the status, but not the bkops level. > And what about WRITEBOOSTER_EVENT_EN? After all it was invented specifically for WB... > >> + (avail_buf < UFS_WB_40_PERCENT_BUF_REMAIN)) { >> + hba->dev_info.keep_vcc_on = true; >> + return true; >> + } >> + } >> + return false; >> +} > > Thanks, > Avri > Thanks Avri/Bart for the comments. I'll wait for a couple of more days, if anyone else has any comments. I'd respond/address the comments thereafter. Thanks, asd -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, Linux Foundation Collaborative Project ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [<RFC PATCH v1> 1/2] scsi: ufs: add write booster feature support 2020-02-25 12:50 ` Avri Altman 2020-02-26 17:57 ` Asutosh Das (asd) @ 2020-03-16 23:52 ` Asutosh Das (asd) 1 sibling, 0 replies; 9+ messages in thread From: Asutosh Das (asd) @ 2020-03-16 23:52 UTC (permalink / raw) To: Avri Altman, subhashj, cang, rnayak, vinholikatti, jejb, martin.petersen, linux-scsi Cc: linux-arm-msm, Alim Akhtar, James E.J. Bottomley, Stanley Chu, Bean Huo, Tomas Winkler, Colin Ian King, Bart Van Assche, Venkat Gopalakrishnan, Bjorn Andersson, open list On 2/25/2020 4:50 AM, Avri Altman wrote: >> +/* >> + * ufshcd_get_wb_alloc_units - returns >> "dLUNumWriteBoosterBufferAllocUnits" >> + * @hba: per-adapter instance >> + * @lun: UFS device lun id >> + * @d_lun_wbb_au: pointer to buffer to hold the LU's alloc units info >> + * >> + * Returns 0 in case of success and d_lun_wbb_au would be returned >> + * Returns -ENOTSUPP if reading d_lun_wbb_au is not supported. >> + * Returns -EINVAL in case of invalid parameters passed to this function. >> + */ >> +static int ufshcd_get_wb_alloc_units(struct ufs_hba *hba, >> + u8 lun, >> + u8 *d_lun_wbb_au) >> +{ >> + int ret; >> + >> + if (!d_lun_wbb_au) >> + ret = -EINVAL; >> + >> + /* WB can be supported only from LU0..LU7 */ >> + else if (lun >= UFS_UPIU_MAX_GENERAL_LUN) >> + ret = -ENOTSUPP; >> + else >> + ret = ufshcd_read_unit_desc_param(hba, >> + lun, >> + UNIT_DESC_PARAM_WB_BUF_ALLOC_UNITS, >> + d_lun_wbb_au, >> + sizeof(*d_lun_wbb_au)); > You are reading here a single byte, instead of 4 > >> + return ret; >> +} >> + >> /** >> * ufshcd_get_lu_power_on_wp_status - get LU's power on write protect >> * status >> @@ -5194,6 +5267,165 @@ static void >> ufshcd_bkops_exception_event_handler(struct ufs_hba *hba) >> __func__, err); >> } >> >> +static bool ufshcd_wb_sup(struct ufs_hba *hba) >> +{ >> + return ((hba->dev_info.d_ext_ufs_feature_sup & >> + UFS_DEV_WRITE_BOOSTER_SUP) && > Don't you want to have a vendor cap as well, > to allow the platform vendor to control this feature? I presume each platform vendor would provide a provisioning script/method which would configure the WB properties. It can be controlled through that. > >> + (hba->dev_info.b_wb_buffer_type >> + || hba->dev_info.wb_config_lun)); >> +} >> + >> + > > > >> +static bool ufshcd_wb_is_buf_flush_needed(struct ufs_hba *hba) >> +{ >> + int ret; >> + u32 cur_buf, status, avail_buf; >> + >> + if (!ufshcd_wb_sup(hba)) >> + return false; >> + >> + ret = ufshcd_query_attr_retry(hba, >> UPIU_QUERY_OPCODE_READ_ATTR, >> + QUERY_ATTR_IDN_AVAIL_WB_BUFF_SIZE, >> + 0, 0, &avail_buf); >> + if (ret) { >> + dev_warn(hba->dev, "%s dAvailableWriteBoosterBufferSize read >> failed %d\n", >> + __func__, ret); >> + return false; >> + } >> + >> + ret = ufshcd_vops_get_user_cap_mode(hba); >> + if (ret <= 0) { >> + dev_dbg(hba->dev, "Get user-cap reduction mode: failed: %d\n", >> + ret); >> + /* Most commonly used */ >> + ret = UFS_WB_BUFF_PRESERVE_USER_SPACE; >> + } >> + >> + hba->dev_info.keep_vcc_on = false; >> + if (ret == UFS_WB_BUFF_USER_SPACE_RED_EN) { >> + if (avail_buf <= UFS_WB_10_PERCENT_BUF_REMAIN) { >> + hba->dev_info.keep_vcc_on = true; >> + return true; >> + } >> + return false; >> + } else if (ret == UFS_WB_BUFF_PRESERVE_USER_SPACE) { >> + ret = ufshcd_query_attr_retry(hba, >> UPIU_QUERY_OPCODE_READ_ATTR, >> + QUERY_ATTR_IDN_CURR_WB_BUFF_SIZE, >> + 0, 0, &cur_buf); >> + if (ret) { >> + dev_err(hba->dev, "%s dCurWriteBoosterBufferSize read failed >> %d\n", >> + __func__, ret); >> + return false; >> + } >> + >> + if (!cur_buf) { >> + dev_info(hba->dev, "dCurWBBuf: %d WB disabled until free- >> space is available\n", >> + cur_buf); >> + return false; >> + } >> + >> + ret = ufshcd_get_ee_status(hba, &status); >> + if (ret) { >> + dev_err(hba->dev, "%s: failed to get exception status %d\n", >> + __func__, ret); >> + if (avail_buf < UFS_WB_40_PERCENT_BUF_REMAIN) { >> + hba->dev_info.keep_vcc_on = true; >> + return true; >> + } >> + return false; >> + } >> + >> + status &= hba->ee_ctrl_mask; >> + >> + if ((status & MASK_EE_URGENT_BKOPS) || > So you are getting the status, but not the bkops level. > And what about WRITEBOOSTER_EVENT_EN? After all it was invented specifically for WB... Yeah - WB exception event is not supported in this series. I will push another change to support that. With WB enabled, any BKOPS exception level warrants the vcc to be on. This is to minimize performance impact. Please correct this understanding from device perspective. > >> + (avail_buf < UFS_WB_40_PERCENT_BUF_REMAIN)) { >> + hba->dev_info.keep_vcc_on = true; >> + return true; >> + } >> + } >> + return false; >> +} > > Thanks, > Avri > -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, Linux Foundation Collaborative Project ^ permalink raw reply [flat|nested] 9+ messages in thread
* [<RFC PATCH v1> 2/2] ufs-qcom: scsi: configure write booster type [not found] <cover.1582330473.git.asutoshd@codeaurora.org> 2020-02-22 0:47 ` [<RFC PATCH v1> 1/2] scsi: ufs: add write booster feature support Asutosh Das @ 2020-02-22 0:47 ` Asutosh Das 1 sibling, 0 replies; 9+ messages in thread From: Asutosh Das @ 2020-02-22 0:47 UTC (permalink / raw) To: subhashj, cang, rnayak, vinholikatti, jejb, martin.petersen, linux-scsi Cc: Asutosh Das, linux-arm-msm, Andy Gross, Bjorn Andersson, Alim Akhtar, Avri Altman, James E.J. Bottomley, open list Configure the WriteBooster type to preserve user-space mode. This would ensure that no user-space capacity is reduced when write booster is enabled. Signed-off-by: Asutosh Das <asutoshd@codeaurora.org> --- drivers/scsi/ufs/ufs-qcom.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/drivers/scsi/ufs/ufs-qcom.c b/drivers/scsi/ufs/ufs-qcom.c index 8339050..313b4a2 100644 --- a/drivers/scsi/ufs/ufs-qcom.c +++ b/drivers/scsi/ufs/ufs-qcom.c @@ -1686,6 +1686,12 @@ static void ufs_qcom_device_reset(struct ufs_hba *hba) usleep_range(10, 15); } +static u32 ufs_qcom_get_user_cap_mode(struct ufs_hba *hba) +{ + /* QCom prefers no user-space reduction mode */ + return UFS_WB_BUFF_PRESERVE_USER_SPACE; +} + /** * struct ufs_hba_qcom_vops - UFS QCOM specific variant operations * @@ -1707,6 +1713,7 @@ static const struct ufs_hba_variant_ops ufs_hba_qcom_vops = { .resume = ufs_qcom_resume, .dbg_register_dump = ufs_qcom_dump_dbg_regs, .device_reset = ufs_qcom_device_reset, + .get_user_cap_mode = ufs_qcom_get_user_cap_mode, }; /** -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project. ^ permalink raw reply related [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-03-16 23:52 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <cover.1582330473.git.asutoshd@codeaurora.org>
2020-02-22 0:47 ` [<RFC PATCH v1> 1/2] scsi: ufs: add write booster feature support Asutosh Das
2020-02-23 9:03 ` Avri Altman
2020-03-16 23:45 ` Asutosh Das (asd)
2020-02-25 10:58 ` Avri Altman
2020-02-25 11:21 ` Avri Altman
2020-02-25 12:50 ` Avri Altman
2020-02-26 17:57 ` Asutosh Das (asd)
2020-03-16 23:52 ` Asutosh Das (asd)
2020-02-22 0:47 ` [<RFC PATCH v1> 2/2] ufs-qcom: scsi: configure write booster type Asutosh Das
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).