2020-04-08 22:00:43

by Asutosh Das (asd)

Subject: [PATCH v1 1/3] scsi: ufs: add write booster feature support

The write performance of TLC NAND is considerably
lower than SLC NAND. Using SLC NAND as a WriteBooster
Buffer enables the write request to be processed with
lower latency and improves the overall write performance.

Adds support for shared-buffer mode WriteBooster.

WriteBooster enable: SW enables it when clocks are
scaled up, thus it's enabled only in high load conditions.

WriteBooster disable: SW will disable the feature,
when clocks are scaled down. Thus writes would go as normal
writes.

To keep the endurance of the WriteBooster Buffer at a
maximum, this load based toggling is adopted.

Signed-off-by: Asutosh Das <[email protected]>
Signed-off-by: Subhash Jadavani <[email protected]>
---
drivers/scsi/ufs/ufs.h | 31 +++++-
drivers/scsi/ufs/ufshcd.c | 240 +++++++++++++++++++++++++++++++++++++++++++++-
drivers/scsi/ufs/ufshcd.h | 21 ++++
3 files changed, 288 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
index 990cb48..2c77b3e 100644
--- a/drivers/scsi/ufs/ufs.h
+++ b/drivers/scsi/ufs/ufs.h
@@ -140,6 +140,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 +171,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 +198,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 +226,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 +266,9 @@ 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_TYPE = 0x54,
+ DEVICE_DESC_PARAM_WB_SHARED_ALLOC_UNITS = 0x55,
};

/* Interconnect descriptor parameters offsets in bytes*/
@@ -333,6 +344,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 +463,15 @@ enum ufs_dev_pwr_mode {
UFS_POWERDOWN_PWR_MODE = 3,
};

+enum ufs_dev_wb_buf_avail_size {
+ UFS_WB_10_PERCENT_BUF_REMAIN = 0x1,
+ UFS_WB_40_PERCENT_BUF_REMAIN = 0x4,
+};
+
+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 +562,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;
+ u32 d_wb_alloc_units;
+ bool keep_vcc_on;
};

/**
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 64e42ef..5b3a92e 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -48,6 +48,8 @@
#include "unipro.h"
#include "ufs-sysfs.h"
#include "ufs_bsg.h"
+#include <linux/blkdev.h>
+#include <asm/unaligned.h>

#define CREATE_TRACE_POINTS
#include <trace/events/ufs.h>
@@ -251,6 +253,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_buf_flush_enable(struct ufs_hba *hba);
+static int ufshcd_wb_buf_flush_disable(struct ufs_hba *hba);
+static int ufshcd_wb_ctrl(struct ufs_hba *hba, bool enable);
+static int ufshcd_wb_toggle_flush_during_h8(struct ufs_hba *hba, bool set);
+static inline void ufshcd_wb_toggle_flush(struct ufs_hba *hba, bool enable);
+
static inline bool ufshcd_valid_tag(struct ufs_hba *hba, int tag)
{
return tag >= 0 && tag < hba->nutrs;
@@ -272,6 +281,25 @@ static inline void ufshcd_disable_irq(struct ufs_hba *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);
+ ufshcd_wb_toggle_flush(hba, true);
+}
+
static void ufshcd_scsi_unblock_requests(struct ufs_hba *hba)
{
if (atomic_dec_and_test(&hba->scsi_block_reqs_cnt))
@@ -1154,6 +1182,13 @@ static int ufshcd_devfreq_scale(struct ufs_hba *hba, bool scale_up)
ufshcd_scale_clks(hba, false);
}

+ /* Enable Write Booster if we have scaled up else disable it */
+ if (!ret) {
+ up_write(&hba->clk_scaling_lock);
+ ufshcd_wb_ctrl(hba, scale_up);
+ down_write(&hba->clk_scaling_lock);
+ }
+
out_unprepare:
ufshcd_clock_scaling_unprepare(hba);
out:
@@ -5154,6 +5189,168 @@ static void ufshcd_bkops_exception_event_handler(struct ufs_hba *hba)
__func__, err);
}

+static bool ufshcd_wb_sup(struct ufs_hba *hba)
+{
+ return (ufshcd_is_wb_allowed(hba) &&
+ (hba->dev_info.d_ext_ufs_feature_sup &
+ UFS_DEV_WRITE_BOOSTER_SUP) &&
+ hba->dev_info.b_wb_buffer_type &&
+ hba->dev_info.d_wb_alloc_units);
+}
+
+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 ^ hba->wb_enabled))
+ 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 inline void ufshcd_wb_toggle_flush(struct ufs_hba *hba, bool enable)
+{
+ if (enable)
+ ufshcd_wb_buf_flush_enable(hba);
+ else
+ ufshcd_wb_buf_flush_disable(hba);
+
+}
+
+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_keep_vcc_on(struct ufs_hba *hba)
+{
+ int ret;
+ u32 cur_buf, avail_buf;
+
+ if (!ufshcd_wb_sup(hba))
+ return false;
+ /*
+ * The ufs device needs the vcc to be ON to flush.
+ * With user-space reduction enabled, it's enough to enable flush
+ * by checking only the available buffer. The threshold
+ * defined here is > 90% full.
+ * With user-space preserved enabled, the current-buffer
+ * should be checked too because the wb buffer size can reduce
+ * when disk tends to be full. This info is provided by current
+ * buffer (dCurrentWriteBoosterBufferSize). There's no point in
+ * keeping vcc on when current buffer is empty.
+ */
+ 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;
+ }
+
+ if (ret == UFS_WB_BUFF_USER_SPACE_RED_EN) {
+ if (avail_buf <= UFS_WB_10_PERCENT_BUF_REMAIN)
+ 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;
+ }
+ /* Let it continue to flush when >60% full */
+ if (avail_buf < UFS_WB_40_PERCENT_BUF_REMAIN)
+ return true;
+ }
+ return false;
+}
+
/**
* ufshcd_exception_event_handler - handle exceptions raised by device
* @work: pointer to work data
@@ -6632,6 +6829,28 @@ 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 */
+ if (dev_info->wspecversion >= 0x310) {
+ hba->dev_info.d_ext_ufs_feature_sup =
+ get_unaligned_be32(desc_buf +
+ DEVICE_DESC_PARAM_EXT_UFS_FEATURE_SUP);
+ /*
+ * WB may be supported but not configured while provisioning.
+ * The spec says, in dedicated wb buffer mode,
+ * a max of 1 lun would have wb buffer configured.
+ * Now only shared buffer mode is supported.
+ */
+ hba->dev_info.b_wb_buffer_type =
+ desc_buf[DEVICE_DESC_PARAM_WB_TYPE];
+
+ if (!hba->dev_info.b_wb_buffer_type)
+ goto skip_alloc_unit;
+ hba->dev_info.d_wb_alloc_units =
+ get_unaligned_be32(desc_buf +
+ DEVICE_DESC_PARAM_WB_SHARED_ALLOC_UNITS);
+ }
+
+skip_alloc_unit:
err = ufshcd_read_string_desc(hba, model_index,
&dev_info->model, SD_ASCII_STD);
if (err < 0) {
@@ -7142,6 +7361,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);

@@ -7802,12 +8022,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);
@@ -7931,11 +8155,21 @@ 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);
}
+ /*
+ * With wb enabled, if the bkops is enabled or if the
+ * configured WB type is 70% full, keep vcc ON
+ * for the device to flush the wb buffer
+ */
+ if ((hba->auto_bkops_enabled && ufshcd_wb_sup(hba)) ||
+ ufshcd_wb_keep_vcc_on(hba))
+ hba->dev_info.keep_vcc_on = true;
+ } else if (!ufshcd_is_runtime_pm(pm_op)) {
+ 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) ||
+ !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);
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index 6ffc08a..59d6eb6 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -344,6 +344,7 @@ struct ufs_hba_variant_ops {
void (*config_scaling_param)(struct ufs_hba *hba,
struct devfreq_dev_profile *profile,
void *data);
+ u32 (*wb_get_user_cap_mode)(struct ufs_hba *hba);
};

/* clock gating state */
@@ -555,6 +556,13 @@ enum ufshcd_caps {
* for userspace to control the power management.
*/
UFSHCD_CAP_RPM_AUTOSUSPEND = 1 << 6,
+
+ /*
+ * This capability allows the host controller driver to turn-on
+ * WriteBooster, if the underlying device supports it and is
+ * provisioned to be used. This would increase the write performance.
+ */
+ UFSHCD_CAP_WB_EN = (1 << 7),
};

/**
@@ -727,6 +735,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 */
@@ -775,6 +785,11 @@ static inline bool ufshcd_is_auto_hibern8_enabled(struct ufs_hba *hba)
return FIELD_GET(UFSHCI_AHIBERN8_TIMER_MASK, hba->ahit) ? true : false;
}

+static inline bool ufshcd_is_wb_allowed(struct ufs_hba *hba)
+{
+ return hba->caps & UFSHCD_CAP_WB_EN;
+}
+
#define ufshcd_writel(hba, val, reg) \
writel((val), (hba)->mmio_base + (reg))
#define ufshcd_readl(hba, reg) \
@@ -1130,4 +1145,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->wb_get_user_cap_mode)
+ return hba->vops->wb_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.

2020-04-12 12:44:15

by Avri Altman

Subject: RE: [PATCH v1 1/3] scsi: ufs: add write booster feature support

Hi,

>
> 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,
Might as well update the unit descriptor size - its 0x2D now,
As you are adding its new fields

> QUERY_DESC_INTERCONNECT_DEF_SIZE = 0x06,
> @@ -219,6 +226,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 +266,9 @@ 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_USER_TYPE = 0x53,

> + DEVICE_DESC_PARAM_WB_TYPE = 0x54,
> + DEVICE_DESC_PARAM_WB_SHARED_ALLOC_UNITS = 0x55,
> };
>


> +enum ufs_dev_wb_buf_user_cap_config {
> + UFS_WB_BUFF_PRESERVE_USER_SPACE = 1,
> + UFS_WB_BUFF_USER_SPACE_RED_EN = 2,
> +};
Maybe better to follow the spec here:
Reduced - 0
Preserve - 1

> +static inline void ufshcd_wb_config(struct ufs_hba *hba)
> +{
> + int ret;
> +
> + if (!ufshcd_wb_sup(hba))
> + return;
> +
> + ret = ufshcd_wb_ctrl(hba, true);
Why are you setting WB on init?

> + 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);
> + ufshcd_wb_toggle_flush(hba, true);
Why set explicit flush on init?


> +
> +
> +static bool ufshcd_wb_keep_vcc_on(struct ufs_hba *hba)
> +{
> + int ret;
> + u32 cur_buf, avail_buf;
> +
> + if (!ufshcd_wb_sup(hba))
> + return false;
> + /*
> + * The ufs device needs the vcc to be ON to flush.
> + * With user-space reduction enabled, it's enough to enable flush
> + * by checking only the available buffer. The threshold
> + * defined here is > 90% full.
> + * With user-space preserved enabled, the current-buffer
> + * should be checked too because the wb buffer size can reduce
> + * when disk tends to be full. This info is provided by current
> + * buffer (dCurrentWriteBoosterBufferSize). There's no point in
> + * keeping vcc on when current buffer is empty.
> + */
> + 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);
The Preserve User Space mode should be read from -
bWriteBoosterBufferPreserveUserSpaceEn of the device descriptor - 0ffset 0x53.
The driver should have no judgement here.
Based on what is configured, better to attach a helper pointer that will perform the below check,
e.g. ufshcd_wb_preserve_keep_vcc_on() and ufshcd_wb_reduced_keep_vcc_on().
Which will simplify the logic here and make it much more readable.
This makes the preparations you made for ufshcd_vops_get_user_cap_mode,
and your second patch unneeded.


> /**
> * ufshcd_exception_event_handler - handle exceptions raised by device
> * @work: pointer to work data
> @@ -6632,6 +6829,28 @@ 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 */
> + if (dev_info->wspecversion >= 0x310) {
> + hba->dev_info.d_ext_ufs_feature_sup =
> + get_unaligned_be32(desc_buf +
> + DEVICE_DESC_PARAM_EXT_UFS_FEATURE_SUP);
> + /*
> + * WB may be supported but not configured while provisioning.
> + * The spec says, in dedicated wb buffer mode,
> + * a max of 1 lun would have wb buffer configured.
> + * Now only shared buffer mode is supported.
> + */
> + hba->dev_info.b_wb_buffer_type =
> + desc_buf[DEVICE_DESC_PARAM_WB_TYPE];
> +
> + if (!hba->dev_info.b_wb_buffer_type)
> + goto skip_alloc_unit;
> + hba->dev_info.d_wb_alloc_units =
> + get_unaligned_be32(desc_buf +
> + DEVICE_DESC_PARAM_WB_SHARED_ALLOC_UNITS);
> + }
Maybe pack the above code in a wb_probe() designated helper,
which will establish if WB is supported or not.
If one of your validation tests fails, maybe you can just
hba->caps &= ~UFSHCD_CAP_WB_EN;
which will further simplify your ufshcd_wb_sup()


> 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) ||
> + !ufshcd_is_runtime_pm(pm_op))) {
Redundant space removal



Thanks,
Avri

2020-04-21 20:04:47

by Asutosh Das (asd)

Subject: Re: [PATCH v1 1/3] scsi: ufs: add write booster feature support

On 4/12/2020 5:43 AM, Avri Altman wrote:
> Hi,
>
>>
>> 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,
> Might as well update the unit descriptor size - its 0x2D now,
> As you are adding its new fields
>
>> QUERY_DESC_INTERCONNECT_DEF_SIZE = 0x06,
>> @@ -219,6 +226,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 +266,9 @@ 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_USER_TYPE = 0x53,
>
>> + DEVICE_DESC_PARAM_WB_TYPE = 0x54,
>> + DEVICE_DESC_PARAM_WB_SHARED_ALLOC_UNITS = 0x55,
>> };
>>
>
>
>> +enum ufs_dev_wb_buf_user_cap_config {
>> + UFS_WB_BUFF_PRESERVE_USER_SPACE = 1,
>> + UFS_WB_BUFF_USER_SPACE_RED_EN = 2,
>> +};
> Maybe better to follow the spec here:
> Reduced - 0
> Preserve - 1
>
I don't think these enums would be needed. Let me check and remove these
otherwise.
>> +static inline void ufshcd_wb_config(struct ufs_hba *hba)
>> +{
>> + int ret;
>> +
>> + if (!ufshcd_wb_sup(hba))
>> + return;
>> +
>> + ret = ufshcd_wb_ctrl(hba, true);
> Why are you setting WB on init?
During init the clocks are scaled-up. Hence, WB is set at init.
It gets disabled when the clocks are scaled down, which is almost
immediate anyway.

>
>> + 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);
>> + ufshcd_wb_toggle_flush(hba, true);
> Why set explicit flush on init?
The design is to enable flush and let the device handle flush when DB's
are empty on its own. Hence I'm setting flush at init itself.
>
>
>> +
>> +
>> +static bool ufshcd_wb_keep_vcc_on(struct ufs_hba *hba)
>> +{
>> + int ret;
>> + u32 cur_buf, avail_buf;
>> +
>> + if (!ufshcd_wb_sup(hba))
>> + return false;
>> + /*
>> + * The ufs device needs the vcc to be ON to flush.
>> + * With user-space reduction enabled, it's enough to enable flush
>> + * by checking only the available buffer. The threshold
>> + * defined here is > 90% full.
>> + * With user-space preserved enabled, the current-buffer
>> + * should be checked too because the wb buffer size can reduce
>> + * when disk tends to be full. This info is provided by current
>> + * buffer (dCurrentWriteBoosterBufferSize). There's no point in
>> + * keeping vcc on when current buffer is empty.
>> + */
>> + 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);
> The Preserve User Space mode should be read from -
> bWriteBoosterBufferPreserveUserSpaceEn of the device descriptor - 0ffset 0x53.
> The driver should have no judgement here.
> Based on what is configured, better to attach a helper pointer that will perform the below check,
> e.g. ufshcd_wb_preserve_keep_vcc_on() and ufshcd_wb_reduced_keep_vcc_on().
> Which will simplify the logic here and make it much more readable.
> This makes the preparations you made for ufshcd_vops_get_user_cap_mode,
> and your second patch unneeded.
Thanks, that makes sense.
>
>
>> /**
>> * ufshcd_exception_event_handler - handle exceptions raised by device
>> * @work: pointer to work data
>> @@ -6632,6 +6829,28 @@ 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 */
>> + if (dev_info->wspecversion >= 0x310) {
>> + hba->dev_info.d_ext_ufs_feature_sup =
>> + get_unaligned_be32(desc_buf +
>> + DEVICE_DESC_PARAM_EXT_UFS_FEATURE_SUP);
>> + /*
>> + * WB may be supported but not configured while provisioning.
>> + * The spec says, in dedicated wb buffer mode,
>> + * a max of 1 lun would have wb buffer configured.
>> + * Now only shared buffer mode is supported.
>> + */
>> + hba->dev_info.b_wb_buffer_type =
>> + desc_buf[DEVICE_DESC_PARAM_WB_TYPE];
>> +
>> + if (!hba->dev_info.b_wb_buffer_type)
>> + goto skip_alloc_unit;
>> + hba->dev_info.d_wb_alloc_units =
>> + get_unaligned_be32(desc_buf +
>> + DEVICE_DESC_PARAM_WB_SHARED_ALLOC_UNITS);
>> + }
> Maybe pack the above code in a wb_probe() designated helper,
> which will establish if WB is supported or not.
> If one of your validation tests fails, maybe you can just
> hba->caps &= ~UFSHCD_CAP_WB_EN;
> which will further simplify your ufshcd_wb_sup()
>
Good idea.
>
>> 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) ||
>> + !ufshcd_is_runtime_pm(pm_op))) {
> Redundant space removal
>
>
>
> Thanks,
> Avri
>


--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
Linux Foundation Collaborative Project

2020-06-20 21:18:28

by Rob Clark

Subject: Re: [PATCH v1 1/3] scsi: ufs: add write booster feature support

On Wed, Apr 8, 2020 at 3:00 PM Asutosh Das <[email protected]> wrote:
>
> The write performance of TLC NAND is considerably
> lower than SLC NAND. Using SLC NAND as a WriteBooster
> Buffer enables the write request to be processed with
> lower latency and improves the overall write performance.
>
> Adds support for shared-buffer mode WriteBooster.
>
> WriteBooster enable: SW enables it when clocks are
> scaled up, thus it's enabled only in high load conditions.
>
> WriteBooster disable: SW will disable the feature,
> when clocks are scaled down. Thus writes would go as normal
> writes.

btw, in v5.8-rc1 (plus handful of remaining patches for lenovo c630
laptop (sdm850)), I'm seeing a lot of:

ufshcd-qcom 1d84000.ufshc: ufshcd_query_flag: Sending flag query for
idn 14 failed, err = 253
ufshcd-qcom 1d84000.ufshc: ufshcd_query_flag: Sending flag query for
idn 14 failed, err = 253
ufshcd-qcom 1d84000.ufshc: ufshcd_query_flag_retry: query attribute,
opcode 6, idn 14, failed with error 253 after 3 retires
ufshcd-qcom 1d84000.ufshc: ufshcd_wb_ctrl write booster enable failed 253

and at least subjectively, compiling mesa seems slower, which seems
like it might be related?

not sure if that is a known issue?

BR,
-R

>
> To keep the endurance of the WriteBooster Buffer at a
> maximum, this load based toggling is adopted.
>
> Signed-off-by: Asutosh Das <[email protected]>
> Signed-off-by: Subhash Jadavani <[email protected]>
> ---
> drivers/scsi/ufs/ufs.h | 31 +++++-
> drivers/scsi/ufs/ufshcd.c | 240 +++++++++++++++++++++++++++++++++++++++++++++-
> drivers/scsi/ufs/ufshcd.h | 21 ++++
> 3 files changed, 288 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
> index 990cb48..2c77b3e 100644
> --- a/drivers/scsi/ufs/ufs.h
> +++ b/drivers/scsi/ufs/ufs.h
> @@ -140,6 +140,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 +171,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 +198,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 +226,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 +266,9 @@ 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_TYPE = 0x54,
> + DEVICE_DESC_PARAM_WB_SHARED_ALLOC_UNITS = 0x55,
> };
>
> /* Interconnect descriptor parameters offsets in bytes*/
> @@ -333,6 +344,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 +463,15 @@ enum ufs_dev_pwr_mode {
> UFS_POWERDOWN_PWR_MODE = 3,
> };
>
> +enum ufs_dev_wb_buf_avail_size {
> + UFS_WB_10_PERCENT_BUF_REMAIN = 0x1,
> + UFS_WB_40_PERCENT_BUF_REMAIN = 0x4,
> +};
> +
> +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 +562,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;
> + u32 d_wb_alloc_units;
> + bool keep_vcc_on;
> };
>
> /**
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 64e42ef..5b3a92e 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -48,6 +48,8 @@
> #include "unipro.h"
> #include "ufs-sysfs.h"
> #include "ufs_bsg.h"
> +#include <linux/blkdev.h>
> +#include <asm/unaligned.h>
>
> #define CREATE_TRACE_POINTS
> #include <trace/events/ufs.h>
> @@ -251,6 +253,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_buf_flush_enable(struct ufs_hba *hba);
> +static int ufshcd_wb_buf_flush_disable(struct ufs_hba *hba);
> +static int ufshcd_wb_ctrl(struct ufs_hba *hba, bool enable);
> +static int ufshcd_wb_toggle_flush_during_h8(struct ufs_hba *hba, bool set);
> +static inline void ufshcd_wb_toggle_flush(struct ufs_hba *hba, bool enable);
> +
> static inline bool ufshcd_valid_tag(struct ufs_hba *hba, int tag)
> {
> return tag >= 0 && tag < hba->nutrs;
> @@ -272,6 +281,25 @@ static inline void ufshcd_disable_irq(struct ufs_hba *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);
> + ufshcd_wb_toggle_flush(hba, true);
> +}
> +
> static void ufshcd_scsi_unblock_requests(struct ufs_hba *hba)
> {
> if (atomic_dec_and_test(&hba->scsi_block_reqs_cnt))
> @@ -1154,6 +1182,13 @@ static int ufshcd_devfreq_scale(struct ufs_hba *hba, bool scale_up)
> ufshcd_scale_clks(hba, false);
> }
>
> + /* Enable Write Booster if we have scaled up else disable it */
> + if (!ret) {
> + up_write(&hba->clk_scaling_lock);
> + ufshcd_wb_ctrl(hba, scale_up);
> + down_write(&hba->clk_scaling_lock);
> + }
> +
> out_unprepare:
> ufshcd_clock_scaling_unprepare(hba);
> out:
> @@ -5154,6 +5189,168 @@ static void ufshcd_bkops_exception_event_handler(struct ufs_hba *hba)
> __func__, err);
> }
>
> +static bool ufshcd_wb_sup(struct ufs_hba *hba)
> +{
> + return (ufshcd_is_wb_allowed(hba) &&
> + (hba->dev_info.d_ext_ufs_feature_sup &
> + UFS_DEV_WRITE_BOOSTER_SUP) &&
> + hba->dev_info.b_wb_buffer_type &&
> + hba->dev_info.d_wb_alloc_units);
> +}
> +
> +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 ^ hba->wb_enabled))
> + 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 inline void ufshcd_wb_toggle_flush(struct ufs_hba *hba, bool enable)
> +{
> + if (enable)
> + ufshcd_wb_buf_flush_enable(hba);
> + else
> + ufshcd_wb_buf_flush_disable(hba);
> +
> +}
> +
> +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_keep_vcc_on(struct ufs_hba *hba)
> +{
> + int ret;
> + u32 cur_buf, avail_buf;
> +
> + if (!ufshcd_wb_sup(hba))
> + return false;
> + /*
> + * The ufs device needs the vcc to be ON to flush.
> + * With user-space reduction enabled, it's enough to enable flush
> + * by checking only the available buffer. The threshold
> + * defined here is > 90% full.
> + * With user-space preserved enabled, the current-buffer
> + * should be checked too because the wb buffer size can reduce
> + * when disk tends to be full. This info is provided by current
> + * buffer (dCurrentWriteBoosterBufferSize). There's no point in
> + * keeping vcc on when current buffer is empty.
> + */
> + 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;
> + }
> +
> + if (ret == UFS_WB_BUFF_USER_SPACE_RED_EN) {
> + if (avail_buf <= UFS_WB_10_PERCENT_BUF_REMAIN)
> + 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;
> + }
> + /* Let it continue to flush when >60% full */
> + if (avail_buf < UFS_WB_40_PERCENT_BUF_REMAIN)
> + return true;
> + }
> + return false;
> +}
> +
> /**
> * ufshcd_exception_event_handler - handle exceptions raised by device
> * @work: pointer to work data
> @@ -6632,6 +6829,28 @@ 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 */
> + if (dev_info->wspecversion >= 0x310) {
> + hba->dev_info.d_ext_ufs_feature_sup =
> + get_unaligned_be32(desc_buf +
> + DEVICE_DESC_PARAM_EXT_UFS_FEATURE_SUP);
> + /*
> + * WB may be supported but not configured while provisioning.
> + * The spec says, in dedicated wb buffer mode,
> + * a max of 1 lun would have wb buffer configured.
> + * Now only shared buffer mode is supported.
> + */
> + hba->dev_info.b_wb_buffer_type =
> + desc_buf[DEVICE_DESC_PARAM_WB_TYPE];
> +
> + if (!hba->dev_info.b_wb_buffer_type)
> + goto skip_alloc_unit;
> + hba->dev_info.d_wb_alloc_units =
> + get_unaligned_be32(desc_buf +
> + DEVICE_DESC_PARAM_WB_SHARED_ALLOC_UNITS);
> + }
> +
> +skip_alloc_unit:
> err = ufshcd_read_string_desc(hba, model_index,
> &dev_info->model, SD_ASCII_STD);
> if (err < 0) {
> @@ -7142,6 +7361,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);
>
> @@ -7802,12 +8022,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);
> @@ -7931,11 +8155,21 @@ 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);
> }
> + /*
> + * With wb enabled, if the bkops is enabled or if the
> + * configured WB type is 70% full, keep vcc ON
> + * for the device to flush the wb buffer
> + */
> + if ((hba->auto_bkops_enabled && ufshcd_wb_sup(hba)) ||
> + ufshcd_wb_keep_vcc_on(hba))
> + hba->dev_info.keep_vcc_on = true;
> + } else if (!ufshcd_is_runtime_pm(pm_op)) {
> + 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) ||
> + !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);
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> index 6ffc08a..59d6eb6 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -344,6 +344,7 @@ struct ufs_hba_variant_ops {
> void (*config_scaling_param)(struct ufs_hba *hba,
> struct devfreq_dev_profile *profile,
> void *data);
> + u32 (*wb_get_user_cap_mode)(struct ufs_hba *hba);
> };
>
> /* clock gating state */
> @@ -555,6 +556,13 @@ enum ufshcd_caps {
> * for userspace to control the power management.
> */
> UFSHCD_CAP_RPM_AUTOSUSPEND = 1 << 6,
> +
> + /*
> + * This capability allows the host controller driver to turn-on
> + * WriteBooster, if the underlying device supports it and is
> + * provisioned to be used. This would increase the write performance.
> + */
> + UFSHCD_CAP_WB_EN = (1 << 7),
> };
>
> /**
> @@ -727,6 +735,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 */
> @@ -775,6 +785,11 @@ static inline bool ufshcd_is_auto_hibern8_enabled(struct ufs_hba *hba)
> return FIELD_GET(UFSHCI_AHIBERN8_TIMER_MASK, hba->ahit) ? true : false;
> }
>
> +static inline bool ufshcd_is_wb_allowed(struct ufs_hba *hba)
> +{
> + return hba->caps & UFSHCD_CAP_WB_EN;
> +}
> +
> #define ufshcd_writel(hba, val, reg) \
> writel((val), (hba)->mmio_base + (reg))
> #define ufshcd_readl(hba, reg) \
> @@ -1130,4 +1145,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->wb_get_user_cap_mode)
> + return hba->vops->wb_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.
>

2020-06-21 07:43:08

by Avri Altman

Subject: RE: [PATCH v1 1/3] scsi: ufs: add write booster feature support

>
> On Wed, Apr 8, 2020 at 3:00 PM Asutosh Das <[email protected]>
> wrote:
> >
> > The write performance of TLC NAND is considerably
> > lower than SLC NAND. Using SLC NAND as a WriteBooster
> > Buffer enables the write request to be processed with
> > lower latency and improves the overall write performance.
> >
> > Adds support for shared-buffer mode WriteBooster.
> >
> > WriteBooster enable: SW enables it when clocks are
> > scaled up, thus it's enabled only in high load conditions.
> >
> > WriteBooster disable: SW will disable the feature,
> > when clocks are scaled down. Thus writes would go as normal
> > writes.
>
> btw, in v5.8-rc1 (plus handful of remaining patches for lenovo c630
> laptop (sdm850)), I'm seeing a lot of:
>
> ufshcd-qcom 1d84000.ufshc: ufshcd_query_flag: Sending flag query for
> idn 14 failed, err = 253
> ufshcd-qcom 1d84000.ufshc: ufshcd_query_flag: Sending flag query for
> idn 14 failed, err = 253
> ufshcd-qcom 1d84000.ufshc: ufshcd_query_flag_retry: query attribute,
> opcode 6, idn 14, failed with error 253 after 3 retires
> ufshcd-qcom 1d84000.ufshc: ufshcd_wb_ctrl write booster enable failed 253
>
> and at least subjectively, compiling mesa seems slower, which seems
> like it might be related?
This looks like a device issue to be taken with the flash vendor:
The device reports that it supports wd, but returns inalid idn for flag 0xe...

Thanks,
Avri

2020-06-21 08:02:38

by Bjorn Andersson

Subject: Re: [PATCH v1 1/3] scsi: ufs: add write booster feature support

On Sun 21 Jun 00:40 PDT 2020, Avri Altman wrote:

>
> >
> > On Wed, Apr 8, 2020 at 3:00 PM Asutosh Das <[email protected]>
> > wrote:
> > >
> > > The write performance of TLC NAND is considerably
> > > lower than SLC NAND. Using SLC NAND as a WriteBooster
> > > Buffer enables the write request to be processed with
> > > lower latency and improves the overall write performance.
> > >
> > > Adds support for shared-buffer mode WriteBooster.
> > >
> > > WriteBooster enable: SW enables it when clocks are
> > > scaled up, thus it's enabled only in high load conditions.
> > >
> > > WriteBooster disable: SW will disable the feature,
> > > when clocks are scaled down. Thus writes would go as normal
> > > writes.
> >
> > btw, in v5.8-rc1 (plus handful of remaining patches for lenovo c630
> > laptop (sdm850)), I'm seeing a lot of:
> >
> > ufshcd-qcom 1d84000.ufshc: ufshcd_query_flag: Sending flag query for
> > idn 14 failed, err = 253
> > ufshcd-qcom 1d84000.ufshc: ufshcd_query_flag: Sending flag query for
> > idn 14 failed, err = 253
> > ufshcd-qcom 1d84000.ufshc: ufshcd_query_flag_retry: query attribute,
> > opcode 6, idn 14, failed with error 253 after 3 retires
> > ufshcd-qcom 1d84000.ufshc: ufshcd_wb_ctrl write booster enable failed 253
> >
> > and at least subjectively, compiling mesa seems slower, which seems
> > like it might be related?
> This looks like a device issue to be taken with the flash vendor:

There's no way for a end-user to file a bug report with the flash vendor
on a device bought from an OEM and even if they would accept the bug
report they wouldn't re-provision the flash in an shipped device.

So you will have to work around this in the driver.

Regards,
Bjorn

> The device reports that it supports wd, but returns inalid idn for flag 0xe...
>
> Thanks,
> Avri

2020-06-21 16:53:39

by Rob Clark

Subject: Re: [PATCH v1 1/3] scsi: ufs: add write booster feature support

On Sun, Jun 21, 2020 at 12:58 AM Bjorn Andersson
<[email protected]> wrote:
>
> On Sun 21 Jun 00:40 PDT 2020, Avri Altman wrote:
>
> >
> > >
> > > On Wed, Apr 8, 2020 at 3:00 PM Asutosh Das <[email protected]>
> > > wrote:
> > > >
> > > > The write performance of TLC NAND is considerably
> > > > lower than SLC NAND. Using SLC NAND as a WriteBooster
> > > > Buffer enables the write request to be processed with
> > > > lower latency and improves the overall write performance.
> > > >
> > > > Adds support for shared-buffer mode WriteBooster.
> > > >
> > > > WriteBooster enable: SW enables it when clocks are
> > > > scaled up, thus it's enabled only in high load conditions.
> > > >
> > > > WriteBooster disable: SW will disable the feature,
> > > > when clocks are scaled down. Thus writes would go as normal
> > > > writes.
> > >
> > > btw, in v5.8-rc1 (plus handful of remaining patches for lenovo c630
> > > laptop (sdm850)), I'm seeing a lot of:
> > >
> > > ufshcd-qcom 1d84000.ufshc: ufshcd_query_flag: Sending flag query for
> > > idn 14 failed, err = 253
> > > ufshcd-qcom 1d84000.ufshc: ufshcd_query_flag: Sending flag query for
> > > idn 14 failed, err = 253
> > > ufshcd-qcom 1d84000.ufshc: ufshcd_query_flag_retry: query attribute,
> > > opcode 6, idn 14, failed with error 253 after 3 retires
> > > ufshcd-qcom 1d84000.ufshc: ufshcd_wb_ctrl write booster enable failed 253
> > >
> > > and at least subjectively, compiling mesa seems slower, which seems
> > > like it might be related?
> > This looks like a device issue to be taken with the flash vendor:
>
> There's no way for a end-user to file a bug report with the flash vendor
> on a device bought from an OEM and even if they would accept the bug
> report they wouldn't re-provision the flash in an shipped device.
>
> So you will have to work around this in the driver.

oh, ugg.. well I think these msgs from dmesg identify the part if we
end up needing to use a denylist:

scsi 0:0:0:49488: Well-known LUN SKhynix H28S8Q302CMR A102 PQ: 0 ANSI: 6
scsi 0:0:0:49476: Well-known LUN SKhynix H28S8Q302CMR A102 PQ: 0 ANSI: 6
scsi 0:0:0:49456: Well-known LUN SKhynix H28S8Q302CMR A102 PQ: 0 ANSI: 6
scsi 0:0:0:0: Direct-Access SKhynix H28S8Q302CMR A102 PQ: 0 ANSI: 6
scsi 0:0:0:1: Direct-Access SKhynix H28S8Q302CMR A102 PQ: 0 ANSI: 6
sd 0:0:0:0: [sda] 29765632 4096-byte logical blocks: (122 GB/114 GiB)
sd 0:0:0:0: [sda] Write Protect is off
sd 0:0:0:0: [sda] Mode Sense: 00 32 00 10
sd 0:0:0:0: [sda] Write cache: enabled, read cache: enabled, supports
DPO and FUA
sd 0:0:0:0: [sda] Optimal transfer size 786432 bytes
scsi 0:0:0:2: Direct-Access SKhynix H28S8Q302CMR A102 PQ: 0 ANSI: 6
scsi 0:0:0:3: Direct-Access SKhynix H28S8Q302CMR A102 PQ: 0 ANSI: 6


(otoh I guess the driver could just notice that writeboost keeps
failing and stop trying to use it)

BR,
-R


> Regards,
> Bjorn
>
> > The device reports that it supports wd, but returns inalid idn for flag 0xe...
> >
> > Thanks,
> > Avri

2020-06-21 17:52:37

by Steev Klimaszewski

Subject: Re: [PATCH v1 1/3] scsi: ufs: add write booster feature support

On 6/21/20 11:50 AM, Rob Clark wrote:
> This looks like a device issue to be taken with the flash vendor:
>> There's no way for a end-user to file a bug report with the flash vendor
>> on a device bought from an OEM and even if they would accept the bug
>> report they wouldn't re-provision the flash in an shipped device.
>>
>> So you will have to work around this in the driver.
> oh, ugg.. well I think these msgs from dmesg identify the part if we
> end up needing to use a denylist:
>
> scsi 0:0:0:49488: Well-known LUN SKhynix H28S8Q302CMR A102 PQ: 0 ANSI: 6
> scsi 0:0:0:49476: Well-known LUN SKhynix H28S8Q302CMR A102 PQ: 0 ANSI: 6
> scsi 0:0:0:49456: Well-known LUN SKhynix H28S8Q302CMR A102 PQ: 0 ANSI: 6
> scsi 0:0:0:0: Direct-Access SKhynix H28S8Q302CMR A102 PQ: 0 ANSI: 6
> scsi 0:0:0:1: Direct-Access SKhynix H28S8Q302CMR A102 PQ: 0 ANSI: 6
> sd 0:0:0:0: [sda] 29765632 4096-byte logical blocks: (122 GB/114 GiB)
> sd 0:0:0:0: [sda] Write Protect is off
> sd 0:0:0:0: [sda] Mode Sense: 00 32 00 10
> sd 0:0:0:0: [sda] Write cache: enabled, read cache: enabled, supports
> DPO and FUA
> sd 0:0:0:0: [sda] Optimal transfer size 786432 bytes
> scsi 0:0:0:2: Direct-Access SKhynix H28S8Q302CMR A102 PQ: 0 ANSI: 6
> scsi 0:0:0:3: Direct-Access SKhynix H28S8Q302CMR A102 PQ: 0 ANSI: 6
>
>
> (otoh I guess the driver could just notice that writeboost keeps
> failing and stop trying to use it)
>
> BR,
> -R


FWIW, I see this on my c630 as well, but my LUN shows up as


scsi 0:0:0:49488: Well-known LUN    SAMSUNG  KLUDG4U1EA-B0C1   0500 PQ:
0 ANSI: 6

2020-06-23 04:39:31

by Kyuho Choi

Subject: Re: [PATCH v1 1/3] scsi: ufs: add write booster feature support

Hi Rob,

On 6/22/20, Rob Clark <[email protected]> wrote:
> On Sun, Jun 21, 2020 at 12:58 AM Bjorn Andersson
> <[email protected]> wrote:
>>
>> On Sun 21 Jun 00:40 PDT 2020, Avri Altman wrote:
>>
>> >
>> > >
>> > > On Wed, Apr 8, 2020 at 3:00 PM Asutosh Das <[email protected]>
>> > > wrote:
>> > > >
>> > > > The write performance of TLC NAND is considerably
>> > > > lower than SLC NAND. Using SLC NAND as a WriteBooster
>> > > > Buffer enables the write request to be processed with
>> > > > lower latency and improves the overall write performance.
>> > > >
>> > > > Adds support for shared-buffer mode WriteBooster.
>> > > >
>> > > > WriteBooster enable: SW enables it when clocks are
>> > > > scaled up, thus it's enabled only in high load conditions.
>> > > >
>> > > > WriteBooster disable: SW will disable the feature,
>> > > > when clocks are scaled down. Thus writes would go as normal
>> > > > writes.
>> > >
>> > > btw, in v5.8-rc1 (plus handful of remaining patches for lenovo c630
>> > > laptop (sdm850)), I'm seeing a lot of:
>> > >
>> > > ufshcd-qcom 1d84000.ufshc: ufshcd_query_flag: Sending flag query
>> > > for
>> > > idn 14 failed, err = 253
>> > > ufshcd-qcom 1d84000.ufshc: ufshcd_query_flag: Sending flag query
>> > > for
>> > > idn 14 failed, err = 253
>> > > ufshcd-qcom 1d84000.ufshc: ufshcd_query_flag_retry: query
>> > > attribute,
>> > > opcode 6, idn 14, failed with error 253 after 3 retires
>> > > ufshcd-qcom 1d84000.ufshc: ufshcd_wb_ctrl write booster enable
>> > > failed 253
>> > >
>> > > and at least subjectively, compiling mesa seems slower, which seems
>> > > like it might be related?
>> > This looks like a device issue to be taken with the flash vendor:
>>
>> There's no way for a end-user to file a bug report with the flash vendor
>> on a device bought from an OEM and even if they would accept the bug
>> report they wouldn't re-provision the flash in an shipped device.
>>
>> So you will have to work around this in the driver.
>
> oh, ugg.. well I think these msgs from dmesg identify the part if we
> end up needing to use a denylist:
>
> scsi 0:0:0:49488: Well-known LUN SKhynix H28S8Q302CMR A102 PQ: 0
> ANSI: 6
> scsi 0:0:0:49476: Well-known LUN SKhynix H28S8Q302CMR A102 PQ: 0
> ANSI: 6
> scsi 0:0:0:49456: Well-known LUN SKhynix H28S8Q302CMR A102 PQ: 0
> ANSI: 6
> scsi 0:0:0:0: Direct-Access SKhynix H28S8Q302CMR A102 PQ: 0 ANSI:
> 6
> scsi 0:0:0:1: Direct-Access SKhynix H28S8Q302CMR A102 PQ: 0 ANSI:
> 6
> sd 0:0:0:0: [sda] 29765632 4096-byte logical blocks: (122 GB/114 GiB)
> sd 0:0:0:0: [sda] Write Protect is off
> sd 0:0:0:0: [sda] Mode Sense: 00 32 00 10
> sd 0:0:0:0: [sda] Write cache: enabled, read cache: enabled, supports
> DPO and FUA
> sd 0:0:0:0: [sda] Optimal transfer size 786432 bytes
> scsi 0:0:0:2: Direct-Access SKhynix H28S8Q302CMR A102 PQ: 0 ANSI:
> 6
> scsi 0:0:0:3: Direct-Access SKhynix H28S8Q302CMR A102 PQ: 0 ANSI:
> 6
>

AFAIK, this device are ufs 2.1. It's not support writebooster.

I'd check latest linux scsi branch and ufshcd_wb_config function's
called without device capability check.

ufshcd_wb_config
-> ufshcd_is_wb_allowed
-> only check about hba caps with writebooster

Asutosh's first patch already check about device's capability in here.

IMO, it would be need to fixing in ufshcd_probe_hba or ufshcd_wb_config.

>
> (otoh I guess the driver could just notice that writeboost keeps
> failing and stop trying to use it)
>
> BR,
> -R
>
>
>> Regards,
>> Bjorn
>>
>> > The device reports that it supports wd, but returns inalid idn for flag
>> > 0xe...
>> >
>> > Thanks,
>> > Avri
>

2020-06-23 06:12:01

by Avri Altman

Subject: RE: [PATCH v1 1/3] scsi: ufs: add write booster feature support

>
> AFAIK, this device are ufs 2.1. It's not support writebooster.
>
> I'd check latest linux scsi branch and ufshcd_wb_config function's
> called without device capability check.
Please grep ufshcd_wb_probe.

2020-06-23 06:54:04

by Kyuho Choi

Subject: Re: [PATCH v1 1/3] scsi: ufs: add write booster feature support

Hi Avri,

On 6/23/20, Avri Altman <[email protected]> wrote:
>>
>> AFAIK, this device are ufs 2.1. It's not support writebooster.
>>
>> I'd check latest linux scsi branch and ufshcd_wb_config function's
>> called without device capability check.
> Please grep ufshcd_wb_probe.
>
I got your point, but as I mentioned, this device not support wb, this
is old products.

I'm not sure ufshcd_wb_probe are called or not in Rob and Steev's platform.
If it's called, hba->caps are setted with wb diable and this error not occured.
But (it looks) not called, same query error will be occured in
ufshcd_wb_config/ctrl.

BR,
Kyuho Choi

2020-06-24 01:11:11

by Steev Klimaszewski

Subject: Re: [PATCH v1 1/3] scsi: ufs: add write booster feature support

On 6/23/20 1:51 AM, Kyuho Choi wrote:
> Hi Avri,
>
> On 6/23/20, Avri Altman <[email protected]> wrote:
>>> AFAIK, this device are ufs 2.1. It's not support writebooster.
>>>
>>> I'd check latest linux scsi branch and ufshcd_wb_config function's
>>> called without device capability check.
>> Please grep ufshcd_wb_probe.
>>
> I got your point, but as I mentioned, this device not support wb, this
> is old products.
>
> I'm not sure ufshcd_wb_probe are called or not in Rob and Steev's platform.
> If it's called, hba->caps are setted with wb diable and this error not occured.
> But (it looks) not called, same query error will be occured in
> ufshcd_wb_config/ctrl.
>
> BR,
> Kyuho Choi

I do show ufshcd_wb_probe in my sources - I'm based on 5.8-rc2 with a
few extra patches for the c630, and the inline encryption patches.

I this is the output that I see -

1.
[    0.702501] ufshcd-qcom 1d84000.ufshc: ufshcd_populate_vreg:
Unable to find vdd-hba-supply regulator, assuming enabled
2.
[    0.702506] ufshcd-qcom 1d84000.ufshc: ufshcd_populate_vreg:
Unable to find vccq-supply regulator, assuming enabled
3.
[    0.702508] ufshcd-qcom 1d84000.ufshc: ufshcd_populate_vreg:
Unable to find vccq2-supply regulator, assuming enabled
4.
[    0.703296] ufshcd-qcom 1d84000.ufshc: Found QC Inline Crypto
Engine (ICE) v3.1.75
5.
[    0.705121] scsi host0: ufshcd
6.
[    0.720163] ALSA device list:
7.
[    0.720171]   No soundcards found.
8.
[    0.731393] ufshcd-qcom 1d84000.ufshc: ufshcd_print_pwr_info:[RX,
TX]: gear=[1, 1], lane[1, 1], pwr[SLOWAUTO_MODE, SLOWAUTO_MODE],
rate = 0
9.
[    0.893738] ufshcd-qcom 1d84000.ufshc: ufshcd_print_pwr_info:[RX,
TX]: gear=[3, 3], lane[2, 2], pwr[FAST MODE, FAST MODE], rate = 2
10.
[    0.894703] ufshcd-qcom 1d84000.ufshc:
ufshcd_find_max_sup_active_icc_level: Regulator capability was not
set, actvIccLevel=0
11.
[    0.896032] ufshcd-qcom 1d84000.ufshc: ufshcd_query_flag: Sending
flag query for idn 14 failed, err = 253
12.
[    0.896919] ufshcd-qcom 1d84000.ufshc: ufshcd_query_flag: Sending
flag query for idn 14 failed, err = 253
13.
[    0.897798] ufshcd-qcom 1d84000.ufshc: ufshcd_query_flag: Sending
flag query for idn 14 failed, err = 253
14.
[    0.898227] ufshcd-qcom 1d84000.ufshc: ufshcd_query_flag_retry:
query attribute, opcode 6, idn 14, failed with error 253 after 3 retires
15.
[    0.898798] ufshcd-qcom 1d84000.ufshc: ufshcd_wb_ctrl write
booster enable failed 253
16.
[    0.899150] ufshcd-qcom 1d84000.ufshc: ufshcd_wb_config: Enable
WB failed: 253
17.
[    0.899918] ufshcd-qcom 1d84000.ufshc: ufshcd_query_flag: Sending
flag query for idn 16 failed, err = 253
18.
[    0.900448] ufshcd-qcom 1d84000.ufshc: ufshcd_query_flag: Sending
flag query for idn 16 failed, err = 253
19.
[    0.901290] ufshcd-qcom 1d84000.ufshc: ufshcd_query_flag: Sending
flag query for idn 16 failed, err = 253
20.
[    0.901749] ufshcd-qcom 1d84000.ufshc: ufshcd_query_flag_retry:
query attribute, opcode 6, idn 16, failed with error 253 after 3 retires
21.
[    0.902285] ufshcd-qcom 1d84000.ufshc: ufshcd_wb_config: En WB
flush during H8: failed: 253
22.
[    0.903105] ufshcd-qcom 1d84000.ufshc: ufshcd_query_flag: Sending
flag query for idn 15 failed, err = 253
23.
[    0.903988] ufshcd-qcom 1d84000.ufshc: ufshcd_query_flag: Sending
flag query for idn 15 failed, err = 253
24.
[    0.904866] ufshcd-qcom 1d84000.ufshc: ufshcd_query_flag: Sending
flag query for idn 15 failed, err = 253
25.
[    0.905294] ufshcd-qcom 1d84000.ufshc: ufshcd_query_flag_retry:
query attribute, opcode 6, idn 15, failed with error 253 after 3 retires
26.
[    0.905859] ufshcd-qcom 1d84000.ufshc: ufshcd_wb_buf_flush_enable
WB - buf flush enable failed 253
27.
[    0.907659] scsi 0:0:0:49488: Well-known LUN    SAMSUNG
 KLUDG4U1EA-B0C1  0500 PQ: 0 ANSI: 6
28.
[    0.911082] scsi 0:0:0:49476: Well-known LUN    SAMSUNG
 KLUDG4U1EA-B0C1  0500 PQ: 0 ANSI: 6
29.
[    0.913268] scsi 0:0:0:49456: Well-known LUN    SAMSUNG
 KLUDG4U1EA-B0C1  0500 PQ: 0 ANSI: 6
30.
[    0.914580] scsi 0:0:0:0: Direct-Access     SAMSUNG
 KLUDG4U1EA-B0C1  0500 PQ: 0 ANSI: 6
31.
[    0.915156] sd 0:0:0:0: Power-on or device reset occurred
32.
[    0.915311] scsi 0:0:0:1: Direct-Access     SAMSUNG
 KLUDG4U1EA-B0C1  0500 PQ: 0 ANSI: 6
33.
[    0.916104] scsi 0:0:0:2: Direct-Access     SAMSUNG
 KLUDG4U1EA-B0C1  0500 PQ: 0 ANSI: 6
34.
[    0.916318] sd 0:0:0:0: [sda] 29765632 4096-byte logical blocks:
(122 GB/114 GiB)
35.
[    0.916417] sd 0:0:0:0: [sda] Write Protect is off
36.
[    0.916424] sd 0:0:0:0: [sda] Mode Sense: 00 32 00 10
37.
[    0.916589] sd 0:0:0:0: [sda] Write cache: enabled, read cache:
enabled, supports DPO and FUA
38.
[    0.916667] sd 0:0:0:0: [sda] Optimal transfer size 8192 bytes
39.
[    0.916897] sd 0:0:0:1: Power-on or device reset occurred
40.
[    0.917131] scsi 0:0:0:3: Direct-Access     SAMSUNG
 KLUDG4U1EA-B0C1  0500 PQ: 0 ANSI: 6
41.
[    0.917498] sd 0:0:0:2: Power-on or device reset occurred
42.
[    0.917994] sd 0:0:0:3: Power-on or device reset occurred
43.
[    0.919301] scsi 0:0:0:4: Direct-Access     SAMSUNG
 KLUDG4U1EA-B0C1  0500 PQ: 0 ANSI: 6
44.
[    0.920207] sd 0:0:0:2: [sdc] 1024 4096-byte logical blocks:
(4.19 MB/4.00 MiB)
45.
[    0.920310] sd 0:0:0:3: [sdd] 32768 4096-byte logical blocks:
(134 MB/128 MiB)
46.
[    0.920312] sd 0:0:0:2: [sdc] Write Protect is off
47.
[    0.920317] sd 0:0:0:2: [sdc] Mode Sense: 00 32 00 10
48.
[    0.920405] sd 0:0:0:3: [sdd] Write Protect is off
49.
[    0.920410] sd 0:0:0:3: [sdd] Mode Sense: 00 32 00 10
50.
[    0.920642] sd 0:0:0:2: [sdc] Write cache: enabled, read cache:
enabled, supports DPO and FUA
51.
[    0.921151] sd 0:0:0:2: [sdc] Optimal transfer size 8192 bytes
52.
[    0.921212] sd 0:0:0:3: [sdd] Write cache: enabled, read cache:
enabled, supports DPO and FUA
53.
[    0.921526] sd 0:0:0:3: [sdd] Optimal transfer size 8192 bytes
54.
[    0.922585] sd 0:0:0:4: Power-on or device reset occurred
55.
[    0.922983] scsi 0:0:0:5: Direct-Access     SAMSUNG
 KLUDG4U1EA-B0C1  0500 PQ: 0 ANSI: 6
56.
[    0.923490] sd 0:0:0:1: [sdb] 1024 4096-byte logical blocks:
(4.19 MB/4.00 MiB)
57.
[    0.928867] sd 0:0:0:1: [sdb] Write Protect is off
58.
[    0.928870] sd 0:0:0:1: [sdb] Mode Sense: 00 32 00 10
59.
[    0.930887] sd 0:0:0:4: [sde] 1048576 4096-byte logical blocks:
(4.29 GB/4.00 GiB)
60.
[    0.931179] sd 0:0:0:1: [sdb] Write cache: enabled, read cache:
enabled, supports DPO and FUA
61.
[    0.932015] random: fast init done
62.
[    0.932022] sd 0:0:0:5: Power-on or device reset occurred
63.
[    0.935289] sd 0:0:0:4: [sde] Write Protect is off
64.
[    0.935293] sd 0:0:0:4: [sde] Mode Sense: 00 32 00 10
65.
[    0.935396]  sda: sda1 sda2 sda3 sda4 sda5
66.
[    0.936047] sd 0:0:0:1: [sdb] Optimal transfer size 8192 bytes
67.
[    0.936358] sd 0:0:0:4: [sde] Write cache: enabled, read cache:
enabled, supports DPO and FUA
68.
[    0.936865] sd 0:0:0:4: [sde] Optimal transfer size 8192 bytes
69.
[    0.938448]  sdc: sdc1 sdc2
70.
[    0.939470] sd 0:0:0:5: [sdf] 393216 4096-byte logical blocks:
(1.61 GB/1.50 GiB)
71.
[    0.939743] sd 0:0:0:5: [sdf] Write Protect is off
72.
[    0.939747] sd 0:0:0:5: [sdf] Mode Sense: 00 32 00 10
73.
[    0.940609] sd 0:0:0:5: [sdf] Write cache: enabled, read cache:
enabled, supports DPO and FUA
74.
[    0.940837] sd 0:0:0:5: [sdf] Optimal transfer size 8192 bytes
75.
[    0.940984] sd 0:0:0:0: [sda] Attached SCSI disk
76.
[    0.941150] sd 0:0:0:2: [sdc] Attached SCSI disk
77.
[    0.945814]  sdd: sdd2 sdd3
78.
[    0.945983]  sdf: sdf2 sdf3 sdf4 sdf5
79.
[    0.946701]  sde: sde1 sde2 sde3 sde4 sde5 sde6 sde7 sde8 sde9
sde10 sde11 sde12 sde13 sde14 sde15 sde16 sde17 sde18 sde19 sde20
sde21 sde22 sde23 sde24 sde25 sde26 sde27
80.
[    0.953610]  sdb: sdb1 sdb2
81.
[    0.954035] sd 0:0:0:5: [sdf] Attached SCSI disk
82.
[    0.954131] sd 0:0:0:4: [sde] Attached SCSI disk
83.
[    0.954177] sd 0:0:0:3: [sdd] Attached SCSI disk
84.
[    0.955316] sd 0:0:0:1: [sdb] Attached SCSI disk

Full dmesg output at https://pastebin.com/azWahunu


-- Steev

2020-06-24 01:56:19

by Stanley Chu

Subject: Re: [PATCH v1 1/3] scsi: ufs: add write booster feature support

Hi Steev,

On Tue, 2020-06-23 at 20:10 -0500, Steev Klimaszewski wrote:
> On 6/23/20 1:51 AM, Kyuho Choi wrote:
> > Hi Avri,
> >
> > On 6/23/20, Avri Altman <[email protected]> wrote:
> >>> AFAIK, this device are ufs 2.1. It's not support writebooster.
> >>>
> >>> I'd check latest linux scsi branch and ufshcd_wb_config function's
> >>> called without device capability check.
> >> Please grep ufshcd_wb_probe.
> >>
> > I got your point, but as I mentioned, this device not support wb, this
> > is old products.
> >
> > I'm not sure ufshcd_wb_probe are called or not in Rob and Steev's platform.
> > If it's called, hba->caps are setted with wb diable and this error not occured.
> > But (it looks) not called, same query error will be occured in
> > ufshcd_wb_config/ctrl.
> >
> > BR,
> > Kyuho Choi
>
> I do show ufshcd_wb_probe in my sources - I'm based on 5.8-rc2 with a
> few extra patches for the c630, and the inline encryption patches.
>
> I this is the output that I see -
>
> 1.
> [ 0.702501] ufshcd-qcom 1d84000.ufshc: ufshcd_populate_vreg:
> Unable to find vdd-hba-supply regulator, assuming enabled
> 2.
> [ 0.702506] ufshcd-qcom 1d84000.ufshc: ufshcd_populate_vreg:
> Unable to find vccq-supply regulator, assuming enabled
> 3.
> [ 0.702508] ufshcd-qcom 1d84000.ufshc: ufshcd_populate_vreg:
> Unable to find vccq2-supply regulator, assuming enabled
> 4.
> [ 0.703296] ufshcd-qcom 1d84000.ufshc: Found QC Inline Crypto
> Engine (ICE) v3.1.75
> 5.
> [ 0.705121] scsi host0: ufshcd
> 6.
> [ 0.720163] ALSA device list:
> 7.
> [ 0.720171] No soundcards found.
> 8.
> [ 0.731393] ufshcd-qcom 1d84000.ufshc: ufshcd_print_pwr_info:[RX,
> TX]: gear=[1, 1], lane[1, 1], pwr[SLOWAUTO_MODE, SLOWAUTO_MODE],
> rate = 0
> 9.
> [ 0.893738] ufshcd-qcom 1d84000.ufshc: ufshcd_print_pwr_info:[RX,
> TX]: gear=[3, 3], lane[2, 2], pwr[FAST MODE, FAST MODE], rate = 2
> 10.
> [ 0.894703] ufshcd-qcom 1d84000.ufshc:
> ufshcd_find_max_sup_active_icc_level: Regulator capability was not
> set, actvIccLevel=0
> 11.
> [ 0.896032] ufshcd-qcom 1d84000.ufshc: ufshcd_query_flag: Sending
> flag query for idn 14 failed, err = 253
> 12.
> [ 0.896919] ufshcd-qcom 1d84000.ufshc: ufshcd_query_flag: Sending
> flag query for idn 14 failed, err = 253
> 13.
> [ 0.897798] ufshcd-qcom 1d84000.ufshc: ufshcd_query_flag: Sending
> flag query for idn 14 failed, err = 253
> 14.
> [ 0.898227] ufshcd-qcom 1d84000.ufshc: ufshcd_query_flag_retry:
> query attribute, opcode 6, idn 14, failed with error 253 after 3 retires
> 15.
> [ 0.898798] ufshcd-qcom 1d84000.ufshc: ufshcd_wb_ctrl write
> booster enable failed 253
> 16.
> [ 0.899150] ufshcd-qcom 1d84000.ufshc: ufshcd_wb_config: Enable
> WB failed: 253
> 17.
> [ 0.899918] ufshcd-qcom 1d84000.ufshc: ufshcd_query_flag: Sending
> flag query for idn 16 failed, err = 253
> 18.
> [ 0.900448] ufshcd-qcom 1d84000.ufshc: ufshcd_query_flag: Sending
> flag query for idn 16 failed, err = 253
> 19.
> [ 0.901290] ufshcd-qcom 1d84000.ufshc: ufshcd_query_flag: Sending
> flag query for idn 16 failed, err = 253
> 20.
> [ 0.901749] ufshcd-qcom 1d84000.ufshc: ufshcd_query_flag_retry:
> query attribute, opcode 6, idn 16, failed with error 253 after 3 retires
> 21.
> [ 0.902285] ufshcd-qcom 1d84000.ufshc: ufshcd_wb_config: En WB
> flush during H8: failed: 253
> 22.
> [ 0.903105] ufshcd-qcom 1d84000.ufshc: ufshcd_query_flag: Sending
> flag query for idn 15 failed, err = 253
> 23.
> [ 0.903988] ufshcd-qcom 1d84000.ufshc: ufshcd_query_flag: Sending
> flag query for idn 15 failed, err = 253
> 24.
> [ 0.904866] ufshcd-qcom 1d84000.ufshc: ufshcd_query_flag: Sending
> flag query for idn 15 failed, err = 253
> 25.
> [ 0.905294] ufshcd-qcom 1d84000.ufshc: ufshcd_query_flag_retry:
> query attribute, opcode 6, idn 15, failed with error 253 after 3 retires
> 26.
> [ 0.905859] ufshcd-qcom 1d84000.ufshc: ufshcd_wb_buf_flush_enable
> WB - buf flush enable failed 253

Please help try below simple patch to see if above WriteBooster messages
can be eliminated.


---
drivers/scsi/ufs/ufshcd.c | 2 ++
1 file changed, 2 insertions(+)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index f173ad1bd79f..089c0785f0b3 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -6985,6 +6985,8 @@ static int ufs_get_device_desc(struct ufs_hba
*hba)
dev_info->wspecversion == 0x220 ||
(hba->dev_quirks & UFS_DEVICE_QUIRK_SUPPORT_EXTENDED_FEATURES))
ufshcd_wb_probe(hba, desc_buf);
+ else
+ hba->caps &= ~UFSHCD_CAP_WB_EN;

/*
* ufshcd_read_string_desc returns size of the string
--



> 27.
> [ 0.907659] scsi 0:0:0:49488: Well-known LUN SAMSUNG
> KLUDG4U1EA-B0C1 0500 PQ: 0 ANSI: 6
> 28.
> [ 0.911082] scsi 0:0:0:49476: Well-known LUN SAMSUNG
> KLUDG4U1EA-B0C1 0500 PQ: 0 ANSI: 6
> 29.
> [ 0.913268] scsi 0:0:0:49456: Well-known LUN SAMSUNG
> KLUDG4U1EA-B0C1 0500 PQ: 0 ANSI: 6
> 30.
> [ 0.914580] scsi 0:0:0:0: Direct-Access SAMSUNG
> KLUDG4U1EA-B0C1 0500 PQ: 0 ANSI: 6
> 31.
> [ 0.915156] sd 0:0:0:0: Power-on or device reset occurred
> 32.
> [ 0.915311] scsi 0:0:0:1: Direct-Access SAMSUNG
> KLUDG4U1EA-B0C1 0500 PQ: 0 ANSI: 6
> 33.
> [ 0.916104] scsi 0:0:0:2: Direct-Access SAMSUNG
> KLUDG4U1EA-B0C1 0500 PQ: 0 ANSI: 6
> 34.
> [ 0.916318] sd 0:0:0:0: [sda] 29765632 4096-byte logical blocks:
> (122 GB/114 GiB)
> 35.
> [ 0.916417] sd 0:0:0:0: [sda] Write Protect is off
> 36.
> [ 0.916424] sd 0:0:0:0: [sda] Mode Sense: 00 32 00 10
> 37.
> [ 0.916589] sd 0:0:0:0: [sda] Write cache: enabled, read cache:
> enabled, supports DPO and FUA
> 38.
> [ 0.916667] sd 0:0:0:0: [sda] Optimal transfer size 8192 bytes
> 39.
> [ 0.916897] sd 0:0:0:1: Power-on or device reset occurred
> 40.
> [ 0.917131] scsi 0:0:0:3: Direct-Access SAMSUNG
> KLUDG4U1EA-B0C1 0500 PQ: 0 ANSI: 6
> 41.
> [ 0.917498] sd 0:0:0:2: Power-on or device reset occurred
> 42.
> [ 0.917994] sd 0:0:0:3: Power-on or device reset occurred
> 43.
> [ 0.919301] scsi 0:0:0:4: Direct-Access SAMSUNG
> KLUDG4U1EA-B0C1 0500 PQ: 0 ANSI: 6
> 44.
> [ 0.920207] sd 0:0:0:2: [sdc] 1024 4096-byte logical blocks:
> (4.19 MB/4.00 MiB)
> 45.
> [ 0.920310] sd 0:0:0:3: [sdd] 32768 4096-byte logical blocks:
> (134 MB/128 MiB)
> 46.
> [ 0.920312] sd 0:0:0:2: [sdc] Write Protect is off
> 47.
> [ 0.920317] sd 0:0:0:2: [sdc] Mode Sense: 00 32 00 10
> 48.
> [ 0.920405] sd 0:0:0:3: [sdd] Write Protect is off
> 49.
> [ 0.920410] sd 0:0:0:3: [sdd] Mode Sense: 00 32 00 10
> 50.
> [ 0.920642] sd 0:0:0:2: [sdc] Write cache: enabled, read cache:
> enabled, supports DPO and FUA
> 51.
> [ 0.921151] sd 0:0:0:2: [sdc] Optimal transfer size 8192 bytes
> 52.
> [ 0.921212] sd 0:0:0:3: [sdd] Write cache: enabled, read cache:
> enabled, supports DPO and FUA
> 53.
> [ 0.921526] sd 0:0:0:3: [sdd] Optimal transfer size 8192 bytes
> 54.
> [ 0.922585] sd 0:0:0:4: Power-on or device reset occurred
> 55.
> [ 0.922983] scsi 0:0:0:5: Direct-Access SAMSUNG
> KLUDG4U1EA-B0C1 0500 PQ: 0 ANSI: 6
> 56.
> [ 0.923490] sd 0:0:0:1: [sdb] 1024 4096-byte logical blocks:
> (4.19 MB/4.00 MiB)
> 57.
> [ 0.928867] sd 0:0:0:1: [sdb] Write Protect is off
> 58.
> [ 0.928870] sd 0:0:0:1: [sdb] Mode Sense: 00 32 00 10
> 59.
> [ 0.930887] sd 0:0:0:4: [sde] 1048576 4096-byte logical blocks:
> (4.29 GB/4.00 GiB)
> 60.
> [ 0.931179] sd 0:0:0:1: [sdb] Write cache: enabled, read cache:
> enabled, supports DPO and FUA
> 61.
> [ 0.932015] random: fast init done
> 62.
> [ 0.932022] sd 0:0:0:5: Power-on or device reset occurred
> 63.
> [ 0.935289] sd 0:0:0:4: [sde] Write Protect is off
> 64.
> [ 0.935293] sd 0:0:0:4: [sde] Mode Sense: 00 32 00 10
> 65.
> [ 0.935396] sda: sda1 sda2 sda3 sda4 sda5
> 66.
> [ 0.936047] sd 0:0:0:1: [sdb] Optimal transfer size 8192 bytes
> 67.
> [ 0.936358] sd 0:0:0:4: [sde] Write cache: enabled, read cache:
> enabled, supports DPO and FUA
> 68.
> [ 0.936865] sd 0:0:0:4: [sde] Optimal transfer size 8192 bytes
> 69.
> [ 0.938448] sdc: sdc1 sdc2
> 70.
> [ 0.939470] sd 0:0:0:5: [sdf] 393216 4096-byte logical blocks:
> (1.61 GB/1.50 GiB)
> 71.
> [ 0.939743] sd 0:0:0:5: [sdf] Write Protect is off
> 72.
> [ 0.939747] sd 0:0:0:5: [sdf] Mode Sense: 00 32 00 10
> 73.
> [ 0.940609] sd 0:0:0:5: [sdf] Write cache: enabled, read cache:
> enabled, supports DPO and FUA
> 74.
> [ 0.940837] sd 0:0:0:5: [sdf] Optimal transfer size 8192 bytes
> 75.
> [ 0.940984] sd 0:0:0:0: [sda] Attached SCSI disk
> 76.
> [ 0.941150] sd 0:0:0:2: [sdc] Attached SCSI disk
> 77.
> [ 0.945814] sdd: sdd2 sdd3
> 78.
> [ 0.945983] sdf: sdf2 sdf3 sdf4 sdf5
> 79.
> [ 0.946701] sde: sde1 sde2 sde3 sde4 sde5 sde6 sde7 sde8 sde9
> sde10 sde11 sde12 sde13 sde14 sde15 sde16 sde17 sde18 sde19 sde20
> sde21 sde22 sde23 sde24 sde25 sde26 sde27
> 80.
> [ 0.953610] sdb: sdb1 sdb2
> 81.
> [ 0.954035] sd 0:0:0:5: [sdf] Attached SCSI disk
> 82.
> [ 0.954131] sd 0:0:0:4: [sde] Attached SCSI disk
> 83.
> [ 0.954177] sd 0:0:0:3: [sdd] Attached SCSI disk
> 84.
> [ 0.955316] sd 0:0:0:1: [sdb] Attached SCSI disk
>
> Full dmesg output at https://pastebin.com/azWahunu
>
>
> -- Steev
>

2020-06-24 01:57:12

by Kyuho Choi

Subject: Re: [PATCH v1 1/3] scsi: ufs: add write booster feature support

Hi Steev,

Thanks for share log!.

On 6/24/20, Steev Klimaszewski <[email protected]> wrote:
>
> On 6/23/20 1:51 AM, Kyuho Choi wrote:
>> Hi Avri,
>>
>> On 6/23/20, Avri Altman <[email protected]> wrote:
>>>> AFAIK, this device are ufs 2.1. It's not support writebooster.
>>>>
>>>> I'd check latest linux scsi branch and ufshcd_wb_config function's
>>>> called without device capability check.
>>> Please grep ufshcd_wb_probe.
>>>
>> I got your point, but as I mentioned, this device not support wb, this
>> is old products.
>>
>> I'm not sure ufshcd_wb_probe are called or not in Rob and Steev's
>> platform.
>> If it's called, hba->caps are setted with wb diable and this error not
>> occured.
>> But (it looks) not called, same query error will be occured in
>> ufshcd_wb_config/ctrl.
>>
>> BR,
>> Kyuho Choi
>
> I do show ufshcd_wb_probe in my sources - I'm based on 5.8-rc2 with a
> few extra patches for the c630, and the inline encryption patches.
>
> I this is the output that I see -
>
> 1.
> [ 0.702501] ufshcd-qcom 1d84000.ufshc: ufshcd_populate_vreg:
> Unable to find vdd-hba-supply regulator, assuming enabled
> 2.
> [ 0.702506] ufshcd-qcom 1d84000.ufshc: ufshcd_populate_vreg:
> Unable to find vccq-supply regulator, assuming enabled
> 3.
> [ 0.702508] ufshcd-qcom 1d84000.ufshc: ufshcd_populate_vreg:
> Unable to find vccq2-supply regulator, assuming enabled
> 4.
> [ 0.703296] ufshcd-qcom 1d84000.ufshc: Found QC Inline Crypto
> Engine (ICE) v3.1.75
> 5.
> [ 0.705121] scsi host0: ufshcd
> 6.
> [ 0.720163] ALSA device list:
> 7.
> [ 0.720171] No soundcards found.
> 8.
> [ 0.731393] ufshcd-qcom 1d84000.ufshc: ufshcd_print_pwr_info:[RX,
> TX]: gear=[1, 1], lane[1, 1], pwr[SLOWAUTO_MODE, SLOWAUTO_MODE],
> rate = 0
> 9.
> [ 0.893738] ufshcd-qcom 1d84000.ufshc: ufshcd_print_pwr_info:[RX,
> TX]: gear=[3, 3], lane[2, 2], pwr[FAST MODE, FAST MODE], rate = 2
> 10.
> [ 0.894703] ufshcd-qcom 1d84000.ufshc:
> ufshcd_find_max_sup_active_icc_level: Regulator capability was not
> set, actvIccLevel=0
> 11.
> [ 0.896032] ufshcd-qcom 1d84000.ufshc: ufshcd_query_flag: Sending
> flag query for idn 14 failed, err = 253
> 12.
> [ 0.896919] ufshcd-qcom 1d84000.ufshc: ufshcd_query_flag: Sending
> flag query for idn 14 failed, err = 253
> 13.
> [ 0.897798] ufshcd-qcom 1d84000.ufshc: ufshcd_query_flag: Sending
> flag query for idn 14 failed, err = 253
> 14.
> [ 0.898227] ufshcd-qcom 1d84000.ufshc: ufshcd_query_flag_retry:
> query attribute, opcode 6, idn 14, failed with error 253 after 3
> retires
> 15.
> [ 0.898798] ufshcd-qcom 1d84000.ufshc: ufshcd_wb_ctrl write
> booster enable failed 253
> 16.
> [ 0.899150] ufshcd-qcom 1d84000.ufshc: ufshcd_wb_config: Enable
> WB failed: 253

Like here, as I mentioned in last. Some of UFS 2.1 device maybe got a
query fail like this.

> 17.
> [ 0.899918] ufshcd-qcom 1d84000.ufshc: ufshcd_query_flag: Sending
> flag query for idn 16 failed, err = 253
> 18.
> [ 0.900448] ufshcd-qcom 1d84000.ufshc: ufshcd_query_flag: Sending
> flag query for idn 16 failed, err = 253
> 19.
> [ 0.901290] ufshcd-qcom 1d84000.ufshc: ufshcd_query_flag: Sending
> flag query for idn 16 failed, err = 253
> 20.
> [ 0.901749] ufshcd-qcom 1d84000.ufshc: ufshcd_query_flag_retry:
> query attribute, opcode 6, idn 16, failed with error 253 after 3
> retires
> 21.
> [ 0.902285] ufshcd-qcom 1d84000.ufshc: ufshcd_wb_config: En WB
> flush during H8: failed: 253
> 22.
> [ 0.903105] ufshcd-qcom 1d84000.ufshc: ufshcd_query_flag: Sending
> flag query for idn 15 failed, err = 253
> 23.
> [ 0.903988] ufshcd-qcom 1d84000.ufshc: ufshcd_query_flag: Sending
> flag query for idn 15 failed, err = 253
> 24.
> [ 0.904866] ufshcd-qcom 1d84000.ufshc: ufshcd_query_flag: Sending
> flag query for idn 15 failed, err = 253
> 25.
> [ 0.905294] ufshcd-qcom 1d84000.ufshc: ufshcd_query_flag_retry:
> query attribute, opcode 6, idn 15, failed with error 253 after 3
> retires
> 26.
> [ 0.905859] ufshcd-qcom 1d84000.ufshc: ufshcd_wb_buf_flush_enable
> WB - buf flush enable failed 253
> 27.
> [ 0.907659] scsi 0:0:0:49488: Well-known LUN SAMSUNG
> KLUDG4U1EA-B0C1 0500 PQ: 0 ANSI: 6

I'd searching this ufs device part no and it's ufs 2.1 too.

> 28.
> [ 0.911082] scsi 0:0:0:49476: Well-known LUN SAMSUNG
> KLUDG4U1EA-B0C1 0500 PQ: 0 ANSI: 6
> 29.
> [ 0.913268] scsi 0:0:0:49456: Well-known LUN SAMSUNG
> KLUDG4U1EA-B0C1 0500 PQ: 0 ANSI: 6
> 30.
> [ 0.914580] scsi 0:0:0:0: Direct-Access SAMSUNG
> KLUDG4U1EA-B0C1 0500 PQ: 0 ANSI: 6
> 31.
> [ 0.915156] sd 0:0:0:0: Power-on or device reset occurred
> 32.
> [ 0.915311] scsi 0:0:0:1: Direct-Access SAMSUNG
> KLUDG4U1EA-B0C1 0500 PQ: 0 ANSI: 6
> 33.
> [ 0.916104] scsi 0:0:0:2: Direct-Access SAMSUNG
> KLUDG4U1EA-B0C1 0500 PQ: 0 ANSI: 6
> 34.
> [ 0.916318] sd 0:0:0:0: [sda] 29765632 4096-byte logical blocks:
> (122 GB/114 GiB)
> 35.
> [ 0.916417] sd 0:0:0:0: [sda] Write Protect is off
> 36.
> [ 0.916424] sd 0:0:0:0: [sda] Mode Sense: 00 32 00 10
> 37.
> [ 0.916589] sd 0:0:0:0: [sda] Write cache: enabled, read cache:
> enabled, supports DPO and FUA
> 38.
> [ 0.916667] sd 0:0:0:0: [sda] Optimal transfer size 8192 bytes
> 39.
> [ 0.916897] sd 0:0:0:1: Power-on or device reset occurred
> 40.
> [ 0.917131] scsi 0:0:0:3: Direct-Access SAMSUNG
> KLUDG4U1EA-B0C1 0500 PQ: 0 ANSI: 6
> 41.
> [ 0.917498] sd 0:0:0:2: Power-on or device reset occurred
> 42.
> [ 0.917994] sd 0:0:0:3: Power-on or device reset occurred
> 43.
> [ 0.919301] scsi 0:0:0:4: Direct-Access SAMSUNG
> KLUDG4U1EA-B0C1 0500 PQ: 0 ANSI: 6
> 44.
> [ 0.920207] sd 0:0:0:2: [sdc] 1024 4096-byte logical blocks:
> (4.19 MB/4.00 MiB)
> 45.
> [ 0.920310] sd 0:0:0:3: [sdd] 32768 4096-byte logical blocks:
> (134 MB/128 MiB)
> 46.
> [ 0.920312] sd 0:0:0:2: [sdc] Write Protect is off
> 47.
> [ 0.920317] sd 0:0:0:2: [sdc] Mode Sense: 00 32 00 10
> 48.
> [ 0.920405] sd 0:0:0:3: [sdd] Write Protect is off
> 49.
> [ 0.920410] sd 0:0:0:3: [sdd] Mode Sense: 00 32 00 10
> 50.
> [ 0.920642] sd 0:0:0:2: [sdc] Write cache: enabled, read cache:
> enabled, supports DPO and FUA
> 51.
> [ 0.921151] sd 0:0:0:2: [sdc] Optimal transfer size 8192 bytes
> 52.
> [ 0.921212] sd 0:0:0:3: [sdd] Write cache: enabled, read cache:
> enabled, supports DPO and FUA
> 53.
> [ 0.921526] sd 0:0:0:3: [sdd] Optimal transfer size 8192 bytes
> 54.
> [ 0.922585] sd 0:0:0:4: Power-on or device reset occurred
> 55.
> [ 0.922983] scsi 0:0:0:5: Direct-Access SAMSUNG
> KLUDG4U1EA-B0C1 0500 PQ: 0 ANSI: 6
> 56.
> [ 0.923490] sd 0:0:0:1: [sdb] 1024 4096-byte logical blocks:
> (4.19 MB/4.00 MiB)
> 57.
> [ 0.928867] sd 0:0:0:1: [sdb] Write Protect is off
> 58.
> [ 0.928870] sd 0:0:0:1: [sdb] Mode Sense: 00 32 00 10
> 59.
> [ 0.930887] sd 0:0:0:4: [sde] 1048576 4096-byte logical blocks:
> (4.29 GB/4.00 GiB)
> 60.
> [ 0.931179] sd 0:0:0:1: [sdb] Write cache: enabled, read cache:
> enabled, supports DPO and FUA
> 61.
> [ 0.932015] random: fast init done
> 62.
> [ 0.932022] sd 0:0:0:5: Power-on or device reset occurred
> 63.
> [ 0.935289] sd 0:0:0:4: [sde] Write Protect is off
> 64.
> [ 0.935293] sd 0:0:0:4: [sde] Mode Sense: 00 32 00 10
> 65.
> [ 0.935396] sda: sda1 sda2 sda3 sda4 sda5
> 66.
> [ 0.936047] sd 0:0:0:1: [sdb] Optimal transfer size 8192 bytes
> 67.
> [ 0.936358] sd 0:0:0:4: [sde] Write cache: enabled, read cache:
> enabled, supports DPO and FUA
> 68.
> [ 0.936865] sd 0:0:0:4: [sde] Optimal transfer size 8192 bytes
> 69.
> [ 0.938448] sdc: sdc1 sdc2
> 70.
> [ 0.939470] sd 0:0:0:5: [sdf] 393216 4096-byte logical blocks:
> (1.61 GB/1.50 GiB)
> 71.
> [ 0.939743] sd 0:0:0:5: [sdf] Write Protect is off
> 72.
> [ 0.939747] sd 0:0:0:5: [sdf] Mode Sense: 00 32 00 10
> 73.
> [ 0.940609] sd 0:0:0:5: [sdf] Write cache: enabled, read cache:
> enabled, supports DPO and FUA
> 74.
> [ 0.940837] sd 0:0:0:5: [sdf] Optimal transfer size 8192 bytes
> 75.
> [ 0.940984] sd 0:0:0:0: [sda] Attached SCSI disk
> 76.
> [ 0.941150] sd 0:0:0:2: [sdc] Attached SCSI disk
> 77.
> [ 0.945814] sdd: sdd2 sdd3
> 78.
> [ 0.945983] sdf: sdf2 sdf3 sdf4 sdf5
> 79.
> [ 0.946701] sde: sde1 sde2 sde3 sde4 sde5 sde6 sde7 sde8 sde9
> sde10 sde11 sde12 sde13 sde14 sde15 sde16 sde17 sde18 sde19 sde20
> sde21 sde22 sde23 sde24 sde25 sde26 sde27
> 80.
> [ 0.953610] sdb: sdb1 sdb2
> 81.
> [ 0.954035] sd 0:0:0:5: [sdf] Attached SCSI disk
> 82.
> [ 0.954131] sd 0:0:0:4: [sde] Attached SCSI disk
> 83.
> [ 0.954177] sd 0:0:0:3: [sdd] Attached SCSI disk
> 84.
> [ 0.955316] sd 0:0:0:1: [sdb] Attached SCSI disk
>
> Full dmesg output at https://pastebin.com/azWahunu
>
>
> -- Steev
>
>

BR,
Kyuho Choi

2020-06-24 02:07:17

by Kyuho Choi

Subject: Re: [PATCH v1 1/3] scsi: ufs: add write booster feature support

Hi Stanley,

On 6/24/20, Stanley Chu <[email protected]> wrote:
> Hi Steev,
>
> On Tue, 2020-06-23 at 20:10 -0500, Steev Klimaszewski wrote:
>> On 6/23/20 1:51 AM, Kyuho Choi wrote:
>> > Hi Avri,
>> >
>> > On 6/23/20, Avri Altman <[email protected]> wrote:
>> >>> AFAIK, this device are ufs 2.1. It's not support writebooster.
>> >>>
>> >>> I'd check latest linux scsi branch and ufshcd_wb_config function's
>> >>> called without device capability check.
>> >> Please grep ufshcd_wb_probe.
>> >>
>> > I got your point, but as I mentioned, this device not support wb, this
>> > is old products.
>> >
>> > I'm not sure ufshcd_wb_probe are called or not in Rob and Steev's
>> > platform.
>> > If it's called, hba->caps are setted with wb diable and this error not
>> > occured.
>> > But (it looks) not called, same query error will be occured in
>> > ufshcd_wb_config/ctrl.
>> >
>> > BR,
>> > Kyuho Choi
>>
>> I do show ufshcd_wb_probe in my sources - I'm based on 5.8-rc2 with a
>> few extra patches for the c630, and the inline encryption patches.
>>
>> I this is the output that I see -
>>
>> 1.
>> [ 0.702501] ufshcd-qcom 1d84000.ufshc: ufshcd_populate_vreg:
>> Unable to find vdd-hba-supply regulator, assuming enabled
>> 2.
>> [ 0.702506] ufshcd-qcom 1d84000.ufshc: ufshcd_populate_vreg:
>> Unable to find vccq-supply regulator, assuming enabled
>> 3.
>> [ 0.702508] ufshcd-qcom 1d84000.ufshc: ufshcd_populate_vreg:
>> Unable to find vccq2-supply regulator, assuming enabled
>> 4.
>> [ 0.703296] ufshcd-qcom 1d84000.ufshc: Found QC Inline Crypto
>> Engine (ICE) v3.1.75
>> 5.
>> [ 0.705121] scsi host0: ufshcd
>> 6.
>> [ 0.720163] ALSA device list:
>> 7.
>> [ 0.720171] No soundcards found.
>> 8.
>> [ 0.731393] ufshcd-qcom 1d84000.ufshc: ufshcd_print_pwr_info:[RX,
>> TX]: gear=[1, 1], lane[1, 1], pwr[SLOWAUTO_MODE, SLOWAUTO_MODE],
>> rate = 0
>> 9.
>> [ 0.893738] ufshcd-qcom 1d84000.ufshc: ufshcd_print_pwr_info:[RX,
>> TX]: gear=[3, 3], lane[2, 2], pwr[FAST MODE, FAST MODE], rate = 2
>> 10.
>> [ 0.894703] ufshcd-qcom 1d84000.ufshc:
>> ufshcd_find_max_sup_active_icc_level: Regulator capability was not
>> set, actvIccLevel=0
>> 11.
>> [ 0.896032] ufshcd-qcom 1d84000.ufshc: ufshcd_query_flag: Sending
>> flag query for idn 14 failed, err = 253
>> 12.
>> [ 0.896919] ufshcd-qcom 1d84000.ufshc: ufshcd_query_flag: Sending
>> flag query for idn 14 failed, err = 253
>> 13.
>> [ 0.897798] ufshcd-qcom 1d84000.ufshc: ufshcd_query_flag: Sending
>> flag query for idn 14 failed, err = 253
>> 14.
>> [ 0.898227] ufshcd-qcom 1d84000.ufshc: ufshcd_query_flag_retry:
>> query attribute, opcode 6, idn 14, failed with error 253 after 3
>> retires
>> 15.
>> [ 0.898798] ufshcd-qcom 1d84000.ufshc: ufshcd_wb_ctrl write
>> booster enable failed 253
>> 16.
>> [ 0.899150] ufshcd-qcom 1d84000.ufshc: ufshcd_wb_config: Enable
>> WB failed: 253
>> 17.
>> [ 0.899918] ufshcd-qcom 1d84000.ufshc: ufshcd_query_flag: Sending
>> flag query for idn 16 failed, err = 253
>> 18.
>> [ 0.900448] ufshcd-qcom 1d84000.ufshc: ufshcd_query_flag: Sending
>> flag query for idn 16 failed, err = 253
>> 19.
>> [ 0.901290] ufshcd-qcom 1d84000.ufshc: ufshcd_query_flag: Sending
>> flag query for idn 16 failed, err = 253
>> 20.
>> [ 0.901749] ufshcd-qcom 1d84000.ufshc: ufshcd_query_flag_retry:
>> query attribute, opcode 6, idn 16, failed with error 253 after 3
>> retires
>> 21.
>> [ 0.902285] ufshcd-qcom 1d84000.ufshc: ufshcd_wb_config: En WB
>> flush during H8: failed: 253
>> 22.
>> [ 0.903105] ufshcd-qcom 1d84000.ufshc: ufshcd_query_flag: Sending
>> flag query for idn 15 failed, err = 253
>> 23.
>> [ 0.903988] ufshcd-qcom 1d84000.ufshc: ufshcd_query_flag: Sending
>> flag query for idn 15 failed, err = 253
>> 24.
>> [ 0.904866] ufshcd-qcom 1d84000.ufshc: ufshcd_query_flag: Sending
>> flag query for idn 15 failed, err = 253
>> 25.
>> [ 0.905294] ufshcd-qcom 1d84000.ufshc: ufshcd_query_flag_retry:
>> query attribute, opcode 6, idn 15, failed with error 253 after 3
>> retires
>> 26.
>> [ 0.905859] ufshcd-qcom 1d84000.ufshc: ufshcd_wb_buf_flush_enable
>> WB - buf flush enable failed 253
>
> Please help try below simple patch to see if above WriteBooster messages
> can be eliminated.
>
>
> ---
> drivers/scsi/ufs/ufshcd.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index f173ad1bd79f..089c0785f0b3 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -6985,6 +6985,8 @@ static int ufs_get_device_desc(struct ufs_hba
> *hba)
> dev_info->wspecversion == 0x220 ||
> (hba->dev_quirks & UFS_DEVICE_QUIRK_SUPPORT_EXTENDED_FEATURES))
> ufshcd_wb_probe(hba, desc_buf);
> + else
> + hba->caps &= ~UFSHCD_CAP_WB_EN;

IMO, hba->caps about WB_EN is already set in ufs-vendor.c. So for
writebooster didn't support ufs devices, need to clear this caps.

>
> /*
> * ufshcd_read_string_desc returns size of the string
> --
>
>
>
>> 27.
>> [ 0.907659] scsi 0:0:0:49488: Well-known LUN SAMSUNG
>> KLUDG4U1EA-B0C1 0500 PQ: 0 ANSI: 6
>> 28.
>> [ 0.911082] scsi 0:0:0:49476: Well-known LUN SAMSUNG
>> KLUDG4U1EA-B0C1 0500 PQ: 0 ANSI: 6
>> 29.
>> [ 0.913268] scsi 0:0:0:49456: Well-known LUN SAMSUNG
>> KLUDG4U1EA-B0C1 0500 PQ: 0 ANSI: 6
>> 30.
>> [ 0.914580] scsi 0:0:0:0: Direct-Access SAMSUNG
>> KLUDG4U1EA-B0C1 0500 PQ: 0 ANSI: 6
>> 31.
>> [ 0.915156] sd 0:0:0:0: Power-on or device reset occurred
>> 32.
>> [ 0.915311] scsi 0:0:0:1: Direct-Access SAMSUNG
>> KLUDG4U1EA-B0C1 0500 PQ: 0 ANSI: 6
>> 33.
>> [ 0.916104] scsi 0:0:0:2: Direct-Access SAMSUNG
>> KLUDG4U1EA-B0C1 0500 PQ: 0 ANSI: 6
>> 34.
>> [ 0.916318] sd 0:0:0:0: [sda] 29765632 4096-byte logical blocks:
>> (122 GB/114 GiB)
>> 35.
>> [ 0.916417] sd 0:0:0:0: [sda] Write Protect is off
>> 36.
>> [ 0.916424] sd 0:0:0:0: [sda] Mode Sense: 00 32 00 10
>> 37.
>> [ 0.916589] sd 0:0:0:0: [sda] Write cache: enabled, read cache:
>> enabled, supports DPO and FUA
>> 38.
>> [ 0.916667] sd 0:0:0:0: [sda] Optimal transfer size 8192 bytes
>> 39.
>> [ 0.916897] sd 0:0:0:1: Power-on or device reset occurred
>> 40.
>> [ 0.917131] scsi 0:0:0:3: Direct-Access SAMSUNG
>> KLUDG4U1EA-B0C1 0500 PQ: 0 ANSI: 6
>> 41.
>> [ 0.917498] sd 0:0:0:2: Power-on or device reset occurred
>> 42.
>> [ 0.917994] sd 0:0:0:3: Power-on or device reset occurred
>> 43.
>> [ 0.919301] scsi 0:0:0:4: Direct-Access SAMSUNG
>> KLUDG4U1EA-B0C1 0500 PQ: 0 ANSI: 6
>> 44.
>> [ 0.920207] sd 0:0:0:2: [sdc] 1024 4096-byte logical blocks:
>> (4.19 MB/4.00 MiB)
>> 45.
>> [ 0.920310] sd 0:0:0:3: [sdd] 32768 4096-byte logical blocks:
>> (134 MB/128 MiB)
>> 46.
>> [ 0.920312] sd 0:0:0:2: [sdc] Write Protect is off
>> 47.
>> [ 0.920317] sd 0:0:0:2: [sdc] Mode Sense: 00 32 00 10
>> 48.
>> [ 0.920405] sd 0:0:0:3: [sdd] Write Protect is off
>> 49.
>> [ 0.920410] sd 0:0:0:3: [sdd] Mode Sense: 00 32 00 10
>> 50.
>> [ 0.920642] sd 0:0:0:2: [sdc] Write cache: enabled, read cache:
>> enabled, supports DPO and FUA
>> 51.
>> [ 0.921151] sd 0:0:0:2: [sdc] Optimal transfer size 8192 bytes
>> 52.
>> [ 0.921212] sd 0:0:0:3: [sdd] Write cache: enabled, read cache:
>> enabled, supports DPO and FUA
>> 53.
>> [ 0.921526] sd 0:0:0:3: [sdd] Optimal transfer size 8192 bytes
>> 54.
>> [ 0.922585] sd 0:0:0:4: Power-on or device reset occurred
>> 55.
>> [ 0.922983] scsi 0:0:0:5: Direct-Access SAMSUNG
>> KLUDG4U1EA-B0C1 0500 PQ: 0 ANSI: 6
>> 56.
>> [ 0.923490] sd 0:0:0:1: [sdb] 1024 4096-byte logical blocks:
>> (4.19 MB/4.00 MiB)
>> 57.
>> [ 0.928867] sd 0:0:0:1: [sdb] Write Protect is off
>> 58.
>> [ 0.928870] sd 0:0:0:1: [sdb] Mode Sense: 00 32 00 10
>> 59.
>> [ 0.930887] sd 0:0:0:4: [sde] 1048576 4096-byte logical blocks:
>> (4.29 GB/4.00 GiB)
>> 60.
>> [ 0.931179] sd 0:0:0:1: [sdb] Write cache: enabled, read cache:
>> enabled, supports DPO and FUA
>> 61.
>> [ 0.932015] random: fast init done
>> 62.
>> [ 0.932022] sd 0:0:0:5: Power-on or device reset occurred
>> 63.
>> [ 0.935289] sd 0:0:0:4: [sde] Write Protect is off
>> 64.
>> [ 0.935293] sd 0:0:0:4: [sde] Mode Sense: 00 32 00 10
>> 65.
>> [ 0.935396] sda: sda1 sda2 sda3 sda4 sda5
>> 66.
>> [ 0.936047] sd 0:0:0:1: [sdb] Optimal transfer size 8192 bytes
>> 67.
>> [ 0.936358] sd 0:0:0:4: [sde] Write cache: enabled, read cache:
>> enabled, supports DPO and FUA
>> 68.
>> [ 0.936865] sd 0:0:0:4: [sde] Optimal transfer size 8192 bytes
>> 69.
>> [ 0.938448] sdc: sdc1 sdc2
>> 70.
>> [ 0.939470] sd 0:0:0:5: [sdf] 393216 4096-byte logical blocks:
>> (1.61 GB/1.50 GiB)
>> 71.
>> [ 0.939743] sd 0:0:0:5: [sdf] Write Protect is off
>> 72.
>> [ 0.939747] sd 0:0:0:5: [sdf] Mode Sense: 00 32 00 10
>> 73.
>> [ 0.940609] sd 0:0:0:5: [sdf] Write cache: enabled, read cache:
>> enabled, supports DPO and FUA
>> 74.
>> [ 0.940837] sd 0:0:0:5: [sdf] Optimal transfer size 8192 bytes
>> 75.
>> [ 0.940984] sd 0:0:0:0: [sda] Attached SCSI disk
>> 76.
>> [ 0.941150] sd 0:0:0:2: [sdc] Attached SCSI disk
>> 77.
>> [ 0.945814] sdd: sdd2 sdd3
>> 78.
>> [ 0.945983] sdf: sdf2 sdf3 sdf4 sdf5
>> 79.
>> [ 0.946701] sde: sde1 sde2 sde3 sde4 sde5 sde6 sde7 sde8 sde9
>> sde10 sde11 sde12 sde13 sde14 sde15 sde16 sde17 sde18 sde19 sde20
>> sde21 sde22 sde23 sde24 sde25 sde26 sde27
>> 80.
>> [ 0.953610] sdb: sdb1 sdb2
>> 81.
>> [ 0.954035] sd 0:0:0:5: [sdf] Attached SCSI disk
>> 82.
>> [ 0.954131] sd 0:0:0:4: [sde] Attached SCSI disk
>> 83.
>> [ 0.954177] sd 0:0:0:3: [sdd] Attached SCSI disk
>> 84.
>> [ 0.955316] sd 0:0:0:1: [sdb] Attached SCSI disk
>>
>> Full dmesg output at https://pastebin.com/azWahunu
>>
>>
>> -- Steev
>>
>
>

2020-06-24 02:50:09

by Stanley Chu

Subject: Re: [PATCH v1 1/3] scsi: ufs: add write booster feature support

Hi Kyuho,

On Wed, 2020-06-24 at 11:06 +0900, Kyuho Choi wrote:
> Hi Stanley,
>
> On 6/24/20, Stanley Chu <[email protected]> wrote:
> > Hi Steev,
> >
> > On Tue, 2020-06-23 at 20:10 -0500, Steev Klimaszewski wrote:
> >> On 6/23/20 1:51 AM, Kyuho Choi wrote:
> >> > Hi Avri,
> >> >
> >> > On 6/23/20, Avri Altman <[email protected]> wrote:
> >> >>> AFAIK, this device are ufs 2.1. It's not support writebooster.
> >> >>>
> >> >>> I'd check latest linux scsi branch and ufshcd_wb_config function's
> >> >>> called without device capability check.
> >> >> Please grep ufshcd_wb_probe.
> >> >>
> >> > I got your point, but as I mentioned, this device not support wb, this
> >> > is old products.
> >> >
> >> > I'm not sure ufshcd_wb_probe are called or not in Rob and Steev's
> >> > platform.
> >> > If it's called, hba->caps are setted with wb diable and this error not
> >> > occured.
> >> > But (it looks) not called, same query error will be occured in
> >> > ufshcd_wb_config/ctrl.
> >> >
> >> > BR,
> >> > Kyuho Choi
> >>
> >> I do show ufshcd_wb_probe in my sources - I'm based on 5.8-rc2 with a
> >> few extra patches for the c630, and the inline encryption patches.
> >>
> >> I this is the output that I see -
> >>
> >> 1.
> >> [ 0.702501] ufshcd-qcom 1d84000.ufshc: ufshcd_populate_vreg:
> >> Unable to find vdd-hba-supply regulator, assuming enabled
> >> 2.
> >> [ 0.702506] ufshcd-qcom 1d84000.ufshc: ufshcd_populate_vreg:
> >> Unable to find vccq-supply regulator, assuming enabled
> >> 3.
> >> [ 0.702508] ufshcd-qcom 1d84000.ufshc: ufshcd_populate_vreg:
> >> Unable to find vccq2-supply regulator, assuming enabled
> >> 4.
> >> [ 0.703296] ufshcd-qcom 1d84000.ufshc: Found QC Inline Crypto
> >> Engine (ICE) v3.1.75
> >> 5.
> >> [ 0.705121] scsi host0: ufshcd
> >> 6.
> >> [ 0.720163] ALSA device list:
> >> 7.
> >> [ 0.720171] No soundcards found.
> >> 8.
> >> [ 0.731393] ufshcd-qcom 1d84000.ufshc: ufshcd_print_pwr_info:[RX,
> >> TX]: gear=[1, 1], lane[1, 1], pwr[SLOWAUTO_MODE, SLOWAUTO_MODE],
> >> rate = 0
> >> 9.
> >> [ 0.893738] ufshcd-qcom 1d84000.ufshc: ufshcd_print_pwr_info:[RX,
> >> TX]: gear=[3, 3], lane[2, 2], pwr[FAST MODE, FAST MODE], rate = 2
> >> 10.
> >> [ 0.894703] ufshcd-qcom 1d84000.ufshc:
> >> ufshcd_find_max_sup_active_icc_level: Regulator capability was not
> >> set, actvIccLevel=0
> >> 11.
> >> [ 0.896032] ufshcd-qcom 1d84000.ufshc: ufshcd_query_flag: Sending
> >> flag query for idn 14 failed, err = 253
> >> 12.
> >> [ 0.896919] ufshcd-qcom 1d84000.ufshc: ufshcd_query_flag: Sending
> >> flag query for idn 14 failed, err = 253
> >> 13.
> >> [ 0.897798] ufshcd-qcom 1d84000.ufshc: ufshcd_query_flag: Sending
> >> flag query for idn 14 failed, err = 253
> >> 14.
> >> [ 0.898227] ufshcd-qcom 1d84000.ufshc: ufshcd_query_flag_retry:
> >> query attribute, opcode 6, idn 14, failed with error 253 after 3
> >> retires
> >> 15.
> >> [ 0.898798] ufshcd-qcom 1d84000.ufshc: ufshcd_wb_ctrl write
> >> booster enable failed 253
> >> 16.
> >> [ 0.899150] ufshcd-qcom 1d84000.ufshc: ufshcd_wb_config: Enable
> >> WB failed: 253
> >> 17.
> >> [ 0.899918] ufshcd-qcom 1d84000.ufshc: ufshcd_query_flag: Sending
> >> flag query for idn 16 failed, err = 253
> >> 18.
> >> [ 0.900448] ufshcd-qcom 1d84000.ufshc: ufshcd_query_flag: Sending
> >> flag query for idn 16 failed, err = 253
> >> 19.
> >> [ 0.901290] ufshcd-qcom 1d84000.ufshc: ufshcd_query_flag: Sending
> >> flag query for idn 16 failed, err = 253
> >> 20.
> >> [ 0.901749] ufshcd-qcom 1d84000.ufshc: ufshcd_query_flag_retry:
> >> query attribute, opcode 6, idn 16, failed with error 253 after 3
> >> retires
> >> 21.
> >> [ 0.902285] ufshcd-qcom 1d84000.ufshc: ufshcd_wb_config: En WB
> >> flush during H8: failed: 253
> >> 22.
> >> [ 0.903105] ufshcd-qcom 1d84000.ufshc: ufshcd_query_flag: Sending
> >> flag query for idn 15 failed, err = 253
> >> 23.
> >> [ 0.903988] ufshcd-qcom 1d84000.ufshc: ufshcd_query_flag: Sending
> >> flag query for idn 15 failed, err = 253
> >> 24.
> >> [ 0.904866] ufshcd-qcom 1d84000.ufshc: ufshcd_query_flag: Sending
> >> flag query for idn 15 failed, err = 253
> >> 25.
> >> [ 0.905294] ufshcd-qcom 1d84000.ufshc: ufshcd_query_flag_retry:
> >> query attribute, opcode 6, idn 15, failed with error 253 after 3
> >> retires
> >> 26.
> >> [ 0.905859] ufshcd-qcom 1d84000.ufshc: ufshcd_wb_buf_flush_enable
> >> WB - buf flush enable failed 253
> >
> > Please help try below simple patch to see if above WriteBooster messages
> > can be eliminated.
> >
> >
> > ---
> > drivers/scsi/ufs/ufshcd.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> > index f173ad1bd79f..089c0785f0b3 100644
> > --- a/drivers/scsi/ufs/ufshcd.c
> > +++ b/drivers/scsi/ufs/ufshcd.c
> > @@ -6985,6 +6985,8 @@ static int ufs_get_device_desc(struct ufs_hba
> > *hba)
> > dev_info->wspecversion == 0x220 ||
> > (hba->dev_quirks & UFS_DEVICE_QUIRK_SUPPORT_EXTENDED_FEATURES))
> > ufshcd_wb_probe(hba, desc_buf);
> > + else
> > + hba->caps &= ~UFSHCD_CAP_WB_EN;
>
> IMO, hba->caps about WB_EN is already set in ufs-vendor.c. So for
> writebooster didn't support ufs devices, need to clear this caps.
>
> >

Thanks for the ack. Then I'll send it as a formal patch.

Thank you,
Stanley Chu

2020-06-24 16:18:21

by Steev Klimaszewski

Subject: Re: [PATCH v1 1/3] scsi: ufs: add write booster feature support

On 6/23/20 8:53 PM, Stanley Chu wrote:
> Hi Steev,
>
> Please help try below simple patch to see if above WriteBooster messages
> can be eliminated.
>
>
> ---
> drivers/scsi/ufs/ufshcd.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index f173ad1bd79f..089c0785f0b3 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -6985,6 +6985,8 @@ static int ufs_get_device_desc(struct ufs_hba
> *hba)
> dev_info->wspecversion == 0x220 ||
> (hba->dev_quirks & UFS_DEVICE_QUIRK_SUPPORT_EXTENDED_FEATURES))
> ufshcd_wb_probe(hba, desc_buf);
> + else
> + hba->caps &= ~UFSHCD_CAP_WB_EN;
>
> /*
> * ufshcd_read_string_desc returns size of the string

Hi Stanley,

That worked.


1.
[    0.704775] ufshcd-qcom 1d84000.ufshc: ufshcd_populate_vreg:
Unable to find vdd-hba-supply regulator, assuming enabled
2.
[    0.704781] ufshcd-qcom 1d84000.ufshc: ufshcd_populate_vreg:
Unable to find vccq-supply regulator, assuming enabled
3.
[    0.704783] ufshcd-qcom 1d84000.ufshc: ufshcd_populate_vreg:
Unable to find vccq2-supply regulator, assuming enabled
4.
[    0.705618] ufshcd-qcom 1d84000.ufshc: Found QC Inline Crypto
Engine (ICE) v3.1.75
5.
[    0.707496] scsi host0: ufshcd
6.
[    0.720415] ALSA device list:
7.
[    0.720422]   No soundcards found.
8.
[    0.734245] ufshcd-qcom 1d84000.ufshc: ufshcd_print_pwr_info:[RX,
TX]: gear=[1, 1], lane[1, 1], pwr[SLOWAUTO_MODE, SLOWAUTO_MODE],
rate = 0
9.
[    0.845159] ufshcd-qcom 1d84000.ufshc: ufshcd_print_pwr_info:[RX,
TX]: gear=[3, 3], lane[2, 2], pwr[FAST MODE, FAST MODE], rate = 2
10.
[    0.846399] ufshcd-qcom 1d84000.ufshc:
ufshcd_find_max_sup_active_icc_level: Regulator capability was not
set, actvIccLevel=0
11.
[    0.849258] scsi 0:0:0:49488: Well-known LUN    SAMSUNG
 KLUDG4U1EA-B0C1  0500 PQ: 0 ANSI: 6
12.
[    0.853372] scsi 0:0:0:49476: Well-known LUN    SAMSUNG
 KLUDG4U1EA-B0C1  0500 PQ: 0 ANSI: 6
13.
[    0.855135] scsi 0:0:0:49456: Well-known LUN    SAMSUNG
 KLUDG4U1EA-B0C1  0500 PQ: 0 ANSI: 6
14.
[    0.857050] scsi 0:0:0:0: Direct-Access     SAMSUNG
 KLUDG4U1EA-B0C1  0500 PQ: 0 ANSI: 6
15.
[    0.858297] sd 0:0:0:0: Power-on or device reset occurred
16.
[    0.859985] scsi 0:0:0:1: Direct-Access     SAMSUNG
 KLUDG4U1EA-B0C1  0500 PQ: 0 ANSI: 6
17.
[    0.860702] sd 0:0:0:0: [sda] 29765632 4096-byte logical blocks:
(122 GB/114 GiB)

(full dmesg output at https://pastebin.com/Pvfqe42P )

I guess you can throw my Tested-by on there.

Thanks

--Steev

2020-06-25 04:04:57

by Stanley Chu

Subject: Re: [PATCH v1 1/3] scsi: ufs: add write booster feature support

Hi Steev,

On Wed, 2020-06-24 at 11:15 -0500, Steev Klimaszewski wrote:
> On 6/23/20 8:53 PM, Stanley Chu wrote:
> > Hi Steev,
> >
> > Please help try below simple patch to see if above WriteBooster messages
> > can be eliminated.
> >
> >
> > ---
> > drivers/scsi/ufs/ufshcd.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> > index f173ad1bd79f..089c0785f0b3 100644
> > --- a/drivers/scsi/ufs/ufshcd.c
> > +++ b/drivers/scsi/ufs/ufshcd.c
> > @@ -6985,6 +6985,8 @@ static int ufs_get_device_desc(struct ufs_hba
> > *hba)
> > dev_info->wspecversion == 0x220 ||
> > (hba->dev_quirks & UFS_DEVICE_QUIRK_SUPPORT_EXTENDED_FEATURES))
> > ufshcd_wb_probe(hba, desc_buf);
> > + else
> > + hba->caps &= ~UFSHCD_CAP_WB_EN;
> >
> > /*
> > * ufshcd_read_string_desc returns size of the string
>
> Hi Stanley,
>
> That worked.
>
>
> 1.
> [ 0.704775] ufshcd-qcom 1d84000.ufshc: ufshcd_populate_vreg:
> Unable to find vdd-hba-supply regulator, assuming enabled
> 2.
> [ 0.704781] ufshcd-qcom 1d84000.ufshc: ufshcd_populate_vreg:
> Unable to find vccq-supply regulator, assuming enabled
> 3.
> [ 0.704783] ufshcd-qcom 1d84000.ufshc: ufshcd_populate_vreg:
> Unable to find vccq2-supply regulator, assuming enabled
> 4.
> [ 0.705618] ufshcd-qcom 1d84000.ufshc: Found QC Inline Crypto
> Engine (ICE) v3.1.75
> 5.
> [ 0.707496] scsi host0: ufshcd
> 6.
> [ 0.720415] ALSA device list:
> 7.
> [ 0.720422] No soundcards found.
> 8.
> [ 0.734245] ufshcd-qcom 1d84000.ufshc: ufshcd_print_pwr_info:[RX,
> TX]: gear=[1, 1], lane[1, 1], pwr[SLOWAUTO_MODE, SLOWAUTO_MODE],
> rate = 0
> 9.
> [ 0.845159] ufshcd-qcom 1d84000.ufshc: ufshcd_print_pwr_info:[RX,
> TX]: gear=[3, 3], lane[2, 2], pwr[FAST MODE, FAST MODE], rate = 2
> 10.
> [ 0.846399] ufshcd-qcom 1d84000.ufshc:
> ufshcd_find_max_sup_active_icc_level: Regulator capability was not
> set, actvIccLevel=0
> 11.
> [ 0.849258] scsi 0:0:0:49488: Well-known LUN SAMSUNG
> KLUDG4U1EA-B0C1 0500 PQ: 0 ANSI: 6
> 12.
> [ 0.853372] scsi 0:0:0:49476: Well-known LUN SAMSUNG
> KLUDG4U1EA-B0C1 0500 PQ: 0 ANSI: 6
> 13.
> [ 0.855135] scsi 0:0:0:49456: Well-known LUN SAMSUNG
> KLUDG4U1EA-B0C1 0500 PQ: 0 ANSI: 6
> 14.
> [ 0.857050] scsi 0:0:0:0: Direct-Access SAMSUNG
> KLUDG4U1EA-B0C1 0500 PQ: 0 ANSI: 6
> 15.
> [ 0.858297] sd 0:0:0:0: Power-on or device reset occurred
> 16.
> [ 0.859985] scsi 0:0:0:1: Direct-Access SAMSUNG
> KLUDG4U1EA-B0C1 0500 PQ: 0 ANSI: 6
> 17.
> [ 0.860702] sd 0:0:0:0: [sda] 29765632 4096-byte logical blocks:
> (122 GB/114 GiB)
>
> (full dmesg output at https://pastebin.com/Pvfqe42P )
>
> I guess you can throw my Tested-by on there.
>

Thanks so much for the test!
I have re-sent the patch with your "Tested-By" tag : )

Thanks a lot,
Stanley Chu