Hi,
This patchset adds LU dedicated buffer mode support for WriteBooster.
In the meanwhile, enable WriteBooster capability on MediaTek UFS platforms.
v2 -> v3:
- Introduce a device quirk to support WriteBooster in pre-3.1 UFS devices (Avri Altman)
- Fix WriteBooster related sysfs nodes. Now all WriteBooster related sysfs nodes are specifically mapped to the LUN with WriteBooster enabled in LU Dedicated buffer mode (Avri Altman)
v1 -> v2:
- Change the definition name of WriteBooster buffer mode to correspond to specification (Bean Huo)
- Add patch #5: "scsi: ufs: cleanup WriteBooster feature"
Stanley Chu (5):
scsi: ufs: enable WriteBooster on some pre-3.1 UFS devices
scsi: ufs: add "index" in parameter list of ufshcd_query_flag()
scsi: ufs: add LU Dedicated buffer mode support for WriteBooster
scsi: ufs-mediatek: enable WriteBooster capability
scsi: ufs: cleanup WriteBooster feature
drivers/scsi/ufs/ufs-mediatek.c | 3 +
drivers/scsi/ufs/ufs-sysfs.c | 14 ++-
drivers/scsi/ufs/ufs.h | 7 ++
drivers/scsi/ufs/ufs_quirks.h | 7 ++
drivers/scsi/ufs/ufshcd.c | 156 +++++++++++++++++++++-----------
drivers/scsi/ufs/ufshcd.h | 3 +-
6 files changed, 135 insertions(+), 55 deletions(-)
--
2.18.0
WriteBooster feature can be supported by some pre-3.1 UFS devices
by upgrading firmware.
To enable WriteBooster feature in such devices, introduce a device
quirk to relax the entrance condition of ufshcd_wb_probe() to allow
host driver to check those devices' WriteBooster capability.
WriteBooster feature can be available if below all conditions are
satisfied,
1. Host enables WriteBooster capability
2. UFS 3.1 device or UFS pre-3.1 device with quirk
UFS_DEVICE_QUIRK_SUPPORT_EXTENDED_FEATURES enabled
3. Device descriptor has dExtendedUFSFeaturesSupport field
4. WriteBooster support is specified in above field
Signed-off-by: Stanley Chu <[email protected]>
---
drivers/scsi/ufs/ufs_quirks.h | 7 ++++
drivers/scsi/ufs/ufshcd.c | 66 ++++++++++++++++++++++-------------
2 files changed, 48 insertions(+), 25 deletions(-)
diff --git a/drivers/scsi/ufs/ufs_quirks.h b/drivers/scsi/ufs/ufs_quirks.h
index df7a1e6805a3..e3175a63c676 100644
--- a/drivers/scsi/ufs/ufs_quirks.h
+++ b/drivers/scsi/ufs/ufs_quirks.h
@@ -101,4 +101,11 @@ struct ufs_dev_fix {
*/
#define UFS_DEVICE_QUIRK_HOST_VS_DEBUGSAVECONFIGTIME (1 << 9)
+/*
+ * Some pre-3.1 UFS devices can support extended features by upgrading
+ * the firmware. Enable this quirk to make UFS core driver probe and enable
+ * supported features on such devices.
+ */
+#define UFS_DEVICE_QUIRK_SUPPORT_EXTENDED_FEATURES (1 << 10)
+
#endif /* UFS_QUIRKS_H_ */
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 915e963398c4..c6668799d956 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -229,6 +229,8 @@ static struct ufs_dev_fix ufs_fixups[] = {
UFS_DEVICE_QUIRK_HOST_PA_SAVECONFIGTIME),
UFS_FIX(UFS_VENDOR_SKHYNIX, "hB8aL1" /*H28U62301AMR*/,
UFS_DEVICE_QUIRK_HOST_VS_DEBUGSAVECONFIGTIME),
+ UFS_FIX(UFS_VENDOR_SKHYNIX, "H9HQ21AFAMZDAR",
+ UFS_DEVICE_QUIRK_SUPPORT_EXTENDED_FEATURES),
END_FIX
};
@@ -6800,9 +6802,19 @@ static int ufshcd_scsi_add_wlus(struct ufs_hba *hba)
static void ufshcd_wb_probe(struct ufs_hba *hba, u8 *desc_buf)
{
+ if (!ufshcd_is_wb_allowed(hba))
+ return;
+
+ if (hba->desc_size.dev_desc <= DEVICE_DESC_PARAM_EXT_UFS_FEATURE_SUP)
+ goto wb_disabled;
+
hba->dev_info.d_ext_ufs_feature_sup =
get_unaligned_be32(desc_buf +
DEVICE_DESC_PARAM_EXT_UFS_FEATURE_SUP);
+
+ if (!(hba->dev_info.d_ext_ufs_feature_sup & UFS_DEV_WRITE_BOOSTER_SUP))
+ goto wb_disabled;
+
/*
* WB may be supported but not configured while provisioning.
* The spec says, in dedicated wb buffer mode,
@@ -6818,11 +6830,29 @@ static void ufshcd_wb_probe(struct ufs_hba *hba, u8 *desc_buf)
hba->dev_info.b_presrv_uspc_en =
desc_buf[DEVICE_DESC_PARAM_WB_PRESRV_USRSPC_EN];
- if (!((hba->dev_info.d_ext_ufs_feature_sup &
- UFS_DEV_WRITE_BOOSTER_SUP) &&
- hba->dev_info.b_wb_buffer_type &&
+ if (!(hba->dev_info.b_wb_buffer_type &&
hba->dev_info.d_wb_alloc_units))
- hba->caps &= ~UFSHCD_CAP_WB_EN;
+ goto wb_disabled;
+
+ return;
+
+wb_disabled:
+ hba->caps &= ~UFSHCD_CAP_WB_EN;
+}
+
+static void ufs_fixup_device_setup(struct ufs_hba *hba)
+{
+ struct ufs_dev_fix *f;
+ struct ufs_dev_info *dev_info = &hba->dev_info;
+
+ for (f = ufs_fixups; f->quirk; f++) {
+ if ((f->wmanufacturerid == dev_info->wmanufacturerid ||
+ f->wmanufacturerid == UFS_ANY_VENDOR) &&
+ ((dev_info->model &&
+ STR_PRFX_EQUAL(f->model, dev_info->model)) ||
+ !strcmp(f->model, UFS_ANY_MODEL)))
+ hba->dev_quirks |= f->quirk;
+ }
}
static int ufs_get_device_desc(struct ufs_hba *hba)
@@ -6862,10 +6892,6 @@ static int ufs_get_device_desc(struct ufs_hba *hba)
model_index = desc_buf[DEVICE_DESC_PARAM_PRDCT_NAME];
- /* Enable WB only for UFS-3.1 */
- if (dev_info->wspecversion >= 0x310)
- ufshcd_wb_probe(hba, desc_buf);
-
err = ufshcd_read_string_desc(hba, model_index,
&dev_info->model, SD_ASCII_STD);
if (err < 0) {
@@ -6874,6 +6900,13 @@ static int ufs_get_device_desc(struct ufs_hba *hba)
goto out;
}
+ ufs_fixup_device_setup(hba);
+
+ /* Enable WB only for UFS-3.1 */
+ if (dev_info->wspecversion >= 0x310 ||
+ (hba->dev_quirks & UFS_DEVICE_QUIRK_SUPPORT_EXTENDED_FEATURES))
+ ufshcd_wb_probe(hba, desc_buf);
+
/*
* ufshcd_read_string_desc returns size of the string
* reset the error value
@@ -6893,21 +6926,6 @@ static void ufs_put_device_desc(struct ufs_hba *hba)
dev_info->model = NULL;
}
-static void ufs_fixup_device_setup(struct ufs_hba *hba)
-{
- struct ufs_dev_fix *f;
- struct ufs_dev_info *dev_info = &hba->dev_info;
-
- for (f = ufs_fixups; f->quirk; f++) {
- if ((f->wmanufacturerid == dev_info->wmanufacturerid ||
- f->wmanufacturerid == UFS_ANY_VENDOR) &&
- ((dev_info->model &&
- STR_PRFX_EQUAL(f->model, dev_info->model)) ||
- !strcmp(f->model, UFS_ANY_MODEL)))
- hba->dev_quirks |= f->quirk;
- }
-}
-
/**
* ufshcd_tune_pa_tactivate - Tunes PA_TActivate of local UniPro
* @hba: per-adapter instance
@@ -7244,8 +7262,6 @@ static int ufshcd_device_params_init(struct ufs_hba *hba)
ufshcd_get_ref_clk_gating_wait(hba);
- ufs_fixup_device_setup(hba);
-
if (!ufshcd_query_flag_retry(hba, UPIU_QUERY_OPCODE_READ_FLAG,
QUERY_FLAG_IDN_PWR_ON_WPE, &flag))
hba->dev_info.f_power_on_wp_en = flag;
--
2.18.0
According to UFS specification, there are two WriteBooster mode of
operations: "LU dedicated buffer" mode and "shared buffer" mode.
In the "LU dedicated buffer" mode, the WriteBooster Buffer is
dedicated to a logical unit.
If the device supports the "LU dedicated buffer" mode, this mode is
configured by setting bWriteBoosterBufferType to 00h. The logical
unit WriteBooster Buffer size is configured by setting the
dLUNumWriteBoosterBufferAllocUnits field of the related Unit
Descriptor. Only a value greater than zero enables the WriteBooster
feature in the logical unit.
Modify ufshcd_wb_probe() as above description to support LU Dedicated
buffer mode.
Note that according to UFS 3.1 specification, the valid value of
bDeviceMaxWriteBoosterLUs parameter in Geometry Descriptor is 1,
which means at most one LUN can have WriteBooster buffer in "LU
dedicated buffer mode". Therefore this patch supports only one
LUN with WriteBooster enabled. All WriteBooster related sysfs nodes
are specifically mapped to the LUN with WriteBooster enabled in
LU Dedicated buffer mode.
Signed-off-by: Stanley Chu <[email protected]>
---
drivers/scsi/ufs/ufs-sysfs.c | 14 ++++++++-
drivers/scsi/ufs/ufs.h | 7 +++++
drivers/scsi/ufs/ufshcd.c | 60 +++++++++++++++++++++++++++++-------
drivers/scsi/ufs/ufshcd.h | 1 +
4 files changed, 70 insertions(+), 12 deletions(-)
diff --git a/drivers/scsi/ufs/ufs-sysfs.c b/drivers/scsi/ufs/ufs-sysfs.c
index b86b6a40d7e6..a162f63098e5 100644
--- a/drivers/scsi/ufs/ufs-sysfs.c
+++ b/drivers/scsi/ufs/ufs-sysfs.c
@@ -622,16 +622,28 @@ static const struct attribute_group ufs_sysfs_string_descriptors_group = {
.attrs = ufs_sysfs_string_descriptors,
};
+static bool ufshcd_is_wb_flags(enum flag_idn idn)
+{
+ if (idn >= QUERY_FLAG_IDN_WB_EN &&
+ idn <= QUERY_FLAG_IDN_WB_BUFF_FLUSH_DURING_HIBERN8)
+ return true;
+ else
+ return false;
+}
+
#define UFS_FLAG(_name, _uname) \
static ssize_t _name##_show(struct device *dev, \
struct device_attribute *attr, char *buf) \
{ \
bool flag; \
+ u8 index = 0; \
int ret; \
struct ufs_hba *hba = dev_get_drvdata(dev); \
+ if (ufshcd_is_wb_flags(QUERY_FLAG_IDN##_uname)) \
+ index = ufshcd_wb_get_flag_index(hba); \
pm_runtime_get_sync(hba->dev); \
ret = ufshcd_query_flag(hba, UPIU_QUERY_OPCODE_READ_FLAG, \
- QUERY_FLAG_IDN##_uname, 0, &flag); \
+ QUERY_FLAG_IDN##_uname, index, &flag); \
pm_runtime_put_sync(hba->dev); \
if (ret) \
return -EINVAL; \
diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
index daac5053b850..eb3d3cebc87d 100644
--- a/drivers/scsi/ufs/ufs.h
+++ b/drivers/scsi/ufs/ufs.h
@@ -330,6 +330,12 @@ enum health_desc_param {
HEALTH_DESC_PARAM_LIFE_TIME_EST_B = 0x4,
};
+/* WriteBooster buffer mode */
+enum {
+ WB_BUF_MODE_LU_DEDICATED = 0x0,
+ WB_BUF_MODE_SHARED = 0x1,
+};
+
/*
* Logical Unit Write Protect
* 00h: LU not write protected
@@ -559,6 +565,7 @@ struct ufs_dev_info {
bool is_lu_power_on_wp;
/* Maximum number of general LU supported by the UFS device */
u8 max_lu_supported;
+ u8 wb_dedicated_lu;
u16 wmanufacturerid;
/*UFS device Product Name */
u8 *model;
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index f23705379b7d..8c256f6f4a65 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -5204,9 +5204,18 @@ static bool ufshcd_wb_sup(struct ufs_hba *hba)
return ufshcd_is_wb_allowed(hba);
}
+int ufshcd_wb_get_flag_index(struct ufs_hba *hba)
+{
+ if (hba->dev_info.b_wb_buffer_type == WB_BUF_MODE_LU_DEDICATED)
+ return hba->dev_info.wb_dedicated_lu;
+ else
+ return 0;
+}
+
static int ufshcd_wb_ctrl(struct ufs_hba *hba, bool enable)
{
int ret;
+ u8 index;
enum query_opcode opcode;
if (!ufshcd_wb_sup(hba))
@@ -5219,8 +5228,9 @@ static int ufshcd_wb_ctrl(struct ufs_hba *hba, bool enable)
else
opcode = UPIU_QUERY_OPCODE_CLEAR_FLAG;
+ index = ufshcd_wb_get_flag_index(hba);
ret = ufshcd_query_flag_retry(hba, opcode,
- QUERY_FLAG_IDN_WB_EN, 0, NULL);
+ QUERY_FLAG_IDN_WB_EN, index, NULL);
if (ret) {
dev_err(hba->dev, "%s write booster %s failed %d\n",
__func__, enable ? "enable" : "disable", ret);
@@ -5237,15 +5247,17 @@ 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)
{
int val;
+ u8 index;
if (set)
val = UPIU_QUERY_OPCODE_SET_FLAG;
else
val = UPIU_QUERY_OPCODE_CLEAR_FLAG;
+ index = ufshcd_wb_get_flag_index(hba);
return ufshcd_query_flag_retry(hba, val,
- QUERY_FLAG_IDN_WB_BUFF_FLUSH_DURING_HIBERN8, 0,
- NULL);
+ QUERY_FLAG_IDN_WB_BUFF_FLUSH_DURING_HIBERN8,
+ index, NULL);
}
static inline void ufshcd_wb_toggle_flush(struct ufs_hba *hba, bool enable)
@@ -5260,13 +5272,15 @@ static inline void ufshcd_wb_toggle_flush(struct ufs_hba *hba, bool enable)
static int ufshcd_wb_buf_flush_enable(struct ufs_hba *hba)
{
int ret;
+ u8 index;
if (!ufshcd_wb_sup(hba) || hba->wb_buf_flush_enabled)
return 0;
+ index = ufshcd_wb_get_flag_index(hba);
ret = ufshcd_query_flag_retry(hba, UPIU_QUERY_OPCODE_SET_FLAG,
QUERY_FLAG_IDN_WB_BUFF_FLUSH_EN,
- 0, NULL);
+ index, NULL);
if (ret)
dev_err(hba->dev, "%s WB - buf flush enable failed %d\n",
__func__, ret);
@@ -5280,12 +5294,15 @@ static int ufshcd_wb_buf_flush_enable(struct ufs_hba *hba)
static int ufshcd_wb_buf_flush_disable(struct ufs_hba *hba)
{
int ret;
+ u8 index;
if (!ufshcd_wb_sup(hba) || !hba->wb_buf_flush_enabled)
return 0;
+ index = ufshcd_wb_get_flag_index(hba);
ret = ufshcd_query_flag_retry(hba, UPIU_QUERY_OPCODE_CLEAR_FLAG,
- QUERY_FLAG_IDN_WB_BUFF_FLUSH_EN, 0, NULL);
+ QUERY_FLAG_IDN_WB_BUFF_FLUSH_EN,
+ index, NULL);
if (ret) {
dev_warn(hba->dev, "%s: WB - buf flush disable failed %d\n",
__func__, ret);
@@ -6804,6 +6821,10 @@ static int ufshcd_scsi_add_wlus(struct ufs_hba *hba)
static void ufshcd_wb_probe(struct ufs_hba *hba, u8 *desc_buf)
{
+ int ret;
+ u8 lun;
+ u32 d_lu_wb_buf_alloc = 0;
+
if (!ufshcd_is_wb_allowed(hba))
return;
@@ -6826,16 +6847,33 @@ static void ufshcd_wb_probe(struct ufs_hba *hba, u8 *desc_buf)
hba->dev_info.b_wb_buffer_type =
desc_buf[DEVICE_DESC_PARAM_WB_TYPE];
- hba->dev_info.d_wb_alloc_units =
- get_unaligned_be32(desc_buf +
- DEVICE_DESC_PARAM_WB_SHARED_ALLOC_UNITS);
hba->dev_info.b_presrv_uspc_en =
desc_buf[DEVICE_DESC_PARAM_WB_PRESRV_USRSPC_EN];
- if (!(hba->dev_info.b_wb_buffer_type &&
- hba->dev_info.d_wb_alloc_units))
- goto wb_disabled;
+ if (hba->dev_info.b_wb_buffer_type == WB_BUF_MODE_SHARED) {
+ hba->dev_info.d_wb_alloc_units =
+ get_unaligned_be32(desc_buf +
+ DEVICE_DESC_PARAM_WB_SHARED_ALLOC_UNITS);
+ if (!hba->dev_info.d_wb_alloc_units)
+ goto wb_disabled;
+ } else {
+ for (lun = 0; lun < hba->dev_info.max_lu_supported; lun++) {
+ ret = ufshcd_read_unit_desc_param(hba,
+ lun,
+ UNIT_DESC_PARAM_WB_BUF_ALLOC_UNITS,
+ (u8 *)&d_lu_wb_buf_alloc,
+ sizeof(d_lu_wb_buf_alloc));
+ if (ret)
+ goto wb_disabled;
+ if (d_lu_wb_buf_alloc) {
+ hba->dev_info.wb_dedicated_lu = lun;
+ break;
+ }
+ }
+ if (!d_lu_wb_buf_alloc)
+ goto wb_disabled;
+ }
return;
wb_disabled:
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index e555d794d441..570961ada57c 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -823,6 +823,7 @@ void ufshcd_delay_us(unsigned long us, unsigned long tolerance);
int ufshcd_wait_for_register(struct ufs_hba *hba, u32 reg, u32 mask,
u32 val, unsigned long interval_us,
unsigned long timeout_ms, bool can_sleep);
+int ufshcd_wb_get_flag_index(struct ufs_hba *hba);
void ufshcd_parse_dev_ref_clk_freq(struct ufs_hba *hba, struct clk *refclk);
void ufshcd_update_reg_hist(struct ufs_err_reg_hist *reg_hist,
u32 reg);
--
2.18.0
Hi Stanley,
On 2020-05-01 22:38, Stanley Chu wrote:
> WriteBooster feature can be supported by some pre-3.1 UFS devices
> by upgrading firmware.
>
> To enable WriteBooster feature in such devices, introduce a device
> quirk to relax the entrance condition of ufshcd_wb_probe() to allow
> host driver to check those devices' WriteBooster capability.
>
> WriteBooster feature can be available if below all conditions are
> satisfied,
>
> 1. Host enables WriteBooster capability
> 2. UFS 3.1 device or UFS pre-3.1 device with quirk
> UFS_DEVICE_QUIRK_SUPPORT_EXTENDED_FEATURES enabled
> 3. Device descriptor has dExtendedUFSFeaturesSupport field
> 4. WriteBooster support is specified in above field
>
> Signed-off-by: Stanley Chu <[email protected]>
> ---
> drivers/scsi/ufs/ufs_quirks.h | 7 ++++
> drivers/scsi/ufs/ufshcd.c | 66 ++++++++++++++++++++++-------------
> 2 files changed, 48 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/scsi/ufs/ufs_quirks.h
> b/drivers/scsi/ufs/ufs_quirks.h
> index df7a1e6805a3..e3175a63c676 100644
> --- a/drivers/scsi/ufs/ufs_quirks.h
> +++ b/drivers/scsi/ufs/ufs_quirks.h
> @@ -101,4 +101,11 @@ struct ufs_dev_fix {
> */
> #define UFS_DEVICE_QUIRK_HOST_VS_DEBUGSAVECONFIGTIME (1 << 9)
>
> +/*
> + * Some pre-3.1 UFS devices can support extended features by upgrading
> + * the firmware. Enable this quirk to make UFS core driver probe and
> enable
> + * supported features on such devices.
> + */
> +#define UFS_DEVICE_QUIRK_SUPPORT_EXTENDED_FEATURES (1 << 10)
> +
> #endif /* UFS_QUIRKS_H_ */
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 915e963398c4..c6668799d956 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -229,6 +229,8 @@ static struct ufs_dev_fix ufs_fixups[] = {
> UFS_DEVICE_QUIRK_HOST_PA_SAVECONFIGTIME),
> UFS_FIX(UFS_VENDOR_SKHYNIX, "hB8aL1" /*H28U62301AMR*/,
> UFS_DEVICE_QUIRK_HOST_VS_DEBUGSAVECONFIGTIME),
> + UFS_FIX(UFS_VENDOR_SKHYNIX, "H9HQ21AFAMZDAR",
> + UFS_DEVICE_QUIRK_SUPPORT_EXTENDED_FEATURES),
>
> END_FIX
> };
> @@ -6800,9 +6802,19 @@ static int ufshcd_scsi_add_wlus(struct ufs_hba
> *hba)
>
> static void ufshcd_wb_probe(struct ufs_hba *hba, u8 *desc_buf)
> {
> + if (!ufshcd_is_wb_allowed(hba))
> + return;
> +
> + if (hba->desc_size.dev_desc <= DEVICE_DESC_PARAM_EXT_UFS_FEATURE_SUP)
> + goto wb_disabled;
> +
> hba->dev_info.d_ext_ufs_feature_sup =
> get_unaligned_be32(desc_buf +
> DEVICE_DESC_PARAM_EXT_UFS_FEATURE_SUP);
> +
> + if (!(hba->dev_info.d_ext_ufs_feature_sup &
> UFS_DEV_WRITE_BOOSTER_SUP))
> + goto wb_disabled;
> +
> /*
> * WB may be supported but not configured while provisioning.
> * The spec says, in dedicated wb buffer mode,
> @@ -6818,11 +6830,29 @@ static void ufshcd_wb_probe(struct ufs_hba
> *hba, u8 *desc_buf)
> hba->dev_info.b_presrv_uspc_en =
> desc_buf[DEVICE_DESC_PARAM_WB_PRESRV_USRSPC_EN];
>
> - if (!((hba->dev_info.d_ext_ufs_feature_sup &
> - UFS_DEV_WRITE_BOOSTER_SUP) &&
> - hba->dev_info.b_wb_buffer_type &&
> + if (!(hba->dev_info.b_wb_buffer_type &&
> hba->dev_info.d_wb_alloc_units))
> - hba->caps &= ~UFSHCD_CAP_WB_EN;
> + goto wb_disabled;
> +
> + return;
> +
> +wb_disabled:
> + hba->caps &= ~UFSHCD_CAP_WB_EN;
> +}
> +
> +static void ufs_fixup_device_setup(struct ufs_hba *hba)
> +{
> + struct ufs_dev_fix *f;
> + struct ufs_dev_info *dev_info = &hba->dev_info;
> +
> + for (f = ufs_fixups; f->quirk; f++) {
> + if ((f->wmanufacturerid == dev_info->wmanufacturerid ||
> + f->wmanufacturerid == UFS_ANY_VENDOR) &&
> + ((dev_info->model &&
> + STR_PRFX_EQUAL(f->model, dev_info->model)) ||
> + !strcmp(f->model, UFS_ANY_MODEL)))
> + hba->dev_quirks |= f->quirk;
> + }
> }
>
> static int ufs_get_device_desc(struct ufs_hba *hba)
> @@ -6862,10 +6892,6 @@ static int ufs_get_device_desc(struct ufs_hba
> *hba)
>
> model_index = desc_buf[DEVICE_DESC_PARAM_PRDCT_NAME];
>
> - /* Enable WB only for UFS-3.1 */
> - if (dev_info->wspecversion >= 0x310)
> - ufshcd_wb_probe(hba, desc_buf);
> -
> err = ufshcd_read_string_desc(hba, model_index,
> &dev_info->model, SD_ASCII_STD);
> if (err < 0) {
> @@ -6874,6 +6900,13 @@ static int ufs_get_device_desc(struct ufs_hba
> *hba)
> goto out;
> }
>
> + ufs_fixup_device_setup(hba);
> +
> + /* Enable WB only for UFS-3.1 */
Also update this comment to reflect your change?
> + if (dev_info->wspecversion >= 0x310 ||
> + (hba->dev_quirks & UFS_DEVICE_QUIRK_SUPPORT_EXTENDED_FEATURES))
> + ufshcd_wb_probe(hba, desc_buf);
> +
Can we somehow move this after ufshcd_tune_unipro_params() or come up
with
a better way to leverage ufshcd_vops_apply_dev_quirks()? I am asking
this
because if we only rely on adding quirks to ufs_fixups in ufshcd.c, the
table will keep growing and I am sure it will - as flash vendors are
trying
to make their UFS2.1 products to be capable of WB (different densities
and
different NAND processes from different vendors, the combos can be quite
a
few). Meanwhile, some models are specifically made for some customers to
support WB, meaning having them in the table may not help in a
generalized
way, and it is not like some hot fixes that we have to take, it is just
for
a non-standard feature. If we can leverage
ufshcd_vops_apply_dev_quirks(),
SoC vendors can freely add the quirk without touching ufs_fixups table,
which means you don't need to update ufs_fixups every time just for
adding
a new model (GKI rules), you can have your own WB white list in vendor
driver. What do you think?
Thanks,
Can Guo.
> /*
> * ufshcd_read_string_desc returns size of the string
> * reset the error value
> @@ -6893,21 +6926,6 @@ static void ufs_put_device_desc(struct ufs_hba
> *hba)
> dev_info->model = NULL;
> }
>
> -static void ufs_fixup_device_setup(struct ufs_hba *hba)
> -{
> - struct ufs_dev_fix *f;
> - struct ufs_dev_info *dev_info = &hba->dev_info;
> -
> - for (f = ufs_fixups; f->quirk; f++) {
> - if ((f->wmanufacturerid == dev_info->wmanufacturerid ||
> - f->wmanufacturerid == UFS_ANY_VENDOR) &&
> - ((dev_info->model &&
> - STR_PRFX_EQUAL(f->model, dev_info->model)) ||
> - !strcmp(f->model, UFS_ANY_MODEL)))
> - hba->dev_quirks |= f->quirk;
> - }
> -}
> -
> /**
> * ufshcd_tune_pa_tactivate - Tunes PA_TActivate of local UniPro
> * @hba: per-adapter instance
> @@ -7244,8 +7262,6 @@ static int ufshcd_device_params_init(struct
> ufs_hba *hba)
>
> ufshcd_get_ref_clk_gating_wait(hba);
>
> - ufs_fixup_device_setup(hba);
> -
> if (!ufshcd_query_flag_retry(hba, UPIU_QUERY_OPCODE_READ_FLAG,
> QUERY_FLAG_IDN_PWR_ON_WPE, &flag))
> hba->dev_info.f_power_on_wp_en = flag;
Hi Stanley,
Few more nits.
Thanks,
Avri
>
> According to UFS specification, there are two WriteBooster mode of
> operations: "LU dedicated buffer" mode and "shared buffer" mode.
> In the "LU dedicated buffer" mode, the WriteBooster Buffer is
> dedicated to a logical unit.
>
> If the device supports the "LU dedicated buffer" mode, this mode is
> configured by setting bWriteBoosterBufferType to 00h. The logical
> unit WriteBooster Buffer size is configured by setting the
> dLUNumWriteBoosterBufferAllocUnits field of the related Unit
> Descriptor. Only a value greater than zero enables the WriteBooster
> feature in the logical unit.
>
> Modify ufshcd_wb_probe() as above description to support LU Dedicated
> buffer mode.
>
> Note that according to UFS 3.1 specification, the valid value of
> bDeviceMaxWriteBoosterLUs parameter in Geometry Descriptor is 1,
> which means at most one LUN can have WriteBooster buffer in "LU
> dedicated buffer mode". Therefore this patch supports only one
> LUN with WriteBooster enabled. All WriteBooster related sysfs nodes
> are specifically mapped to the LUN with WriteBooster enabled in
> LU Dedicated buffer mode.
>
> Signed-off-by: Stanley Chu <[email protected]>
> ---
> drivers/scsi/ufs/ufs-sysfs.c | 14 ++++++++-
> drivers/scsi/ufs/ufs.h | 7 +++++
> drivers/scsi/ufs/ufshcd.c | 60 +++++++++++++++++++++++++++++-------
> drivers/scsi/ufs/ufshcd.h | 1 +
> 4 files changed, 70 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/scsi/ufs/ufs-sysfs.c b/drivers/scsi/ufs/ufs-sysfs.c
> index b86b6a40d7e6..a162f63098e5 100644
> --- a/drivers/scsi/ufs/ufs-sysfs.c
> +++ b/drivers/scsi/ufs/ufs-sysfs.c
> @@ -622,16 +622,28 @@ static const struct attribute_group
> ufs_sysfs_string_descriptors_group = {
> .attrs = ufs_sysfs_string_descriptors,
> };
>
> +static bool ufshcd_is_wb_flags(enum flag_idn idn)
Inline?
And just return (idn >= QUERY_FLAG_IDN_WB_EN && idn <= QUERY_FLAG_IDN_WB_BUFF_FLUSH_DURING_HIBERN8)
> +{
> + if (idn >= QUERY_FLAG_IDN_WB_EN &&
> + idn <= QUERY_FLAG_IDN_WB_BUFF_FLUSH_DURING_HIBERN8)
> + return true;
> + else
> + return false;
> +}
> +
>
> +int ufshcd_wb_get_flag_index(struct ufs_hba *hba)
> +{
> + if (hba->dev_info.b_wb_buffer_type ==
> WB_BUF_MODE_LU_DEDICATED)
> + return hba->dev_info.wb_dedicated_lu;
> + else
No need for else.
Maybe make this static inline in ufshcd.h?
> + return 0;
> +}
> +
Hi Avri,
On Sat, 2020-05-02 at 15:32 +0000, Avri Altman wrote:
> Hi Stanley,
> Few more nits.
> Thanks,
> Avri
All fixed in v4.
Thanks for these suggestions.
Stanley Chu
>
> >
> > According to UFS specification, there are two WriteBooster mode of
> > operations: "LU dedicated buffer" mode and "shared buffer" mode.
> > In the "LU dedicated buffer" mode, the WriteBooster Buffer is
> > dedicated to a logical unit.
> >
> > If the device supports the "LU dedicated buffer" mode, this mode is
> > configured by setting bWriteBoosterBufferType to 00h. The logical
> > unit WriteBooster Buffer size is configured by setting the
> > dLUNumWriteBoosterBufferAllocUnits field of the related Unit
> > Descriptor. Only a value greater than zero enables the WriteBooster
> > feature in the logical unit.
> >
> > Modify ufshcd_wb_probe() as above description to support LU Dedicated
> > buffer mode.
> >
> > Note that according to UFS 3.1 specification, the valid value of
> > bDeviceMaxWriteBoosterLUs parameter in Geometry Descriptor is 1,
> > which means at most one LUN can have WriteBooster buffer in "LU
> > dedicated buffer mode". Therefore this patch supports only one
> > LUN with WriteBooster enabled. All WriteBooster related sysfs nodes
> > are specifically mapped to the LUN with WriteBooster enabled in
> > LU Dedicated buffer mode.
> >
> > Signed-off-by: Stanley Chu <[email protected]>
> > ---
> > drivers/scsi/ufs/ufs-sysfs.c | 14 ++++++++-
> > drivers/scsi/ufs/ufs.h | 7 +++++
> > drivers/scsi/ufs/ufshcd.c | 60 +++++++++++++++++++++++++++++-------
> > drivers/scsi/ufs/ufshcd.h | 1 +
> > 4 files changed, 70 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/scsi/ufs/ufs-sysfs.c b/drivers/scsi/ufs/ufs-sysfs.c
> > index b86b6a40d7e6..a162f63098e5 100644
> > --- a/drivers/scsi/ufs/ufs-sysfs.c
> > +++ b/drivers/scsi/ufs/ufs-sysfs.c
> > @@ -622,16 +622,28 @@ static const struct attribute_group
> > ufs_sysfs_string_descriptors_group = {
> > .attrs = ufs_sysfs_string_descriptors,
> > };
> >
> > +static bool ufshcd_is_wb_flags(enum flag_idn idn)
> Inline?
> And just return (idn >= QUERY_FLAG_IDN_WB_EN && idn <= QUERY_FLAG_IDN_WB_BUFF_FLUSH_DURING_HIBERN8)
>
> > +{
> > + if (idn >= QUERY_FLAG_IDN_WB_EN &&
> > + idn <= QUERY_FLAG_IDN_WB_BUFF_FLUSH_DURING_HIBERN8)
> > + return true;
> > + else
> > + return false;
> > +}
> > +
>
>
> >
> > +int ufshcd_wb_get_flag_index(struct ufs_hba *hba)
> > +{
> > + if (hba->dev_info.b_wb_buffer_type ==
> > WB_BUF_MODE_LU_DEDICATED)
> > + return hba->dev_info.wb_dedicated_lu;
> > + else
> No need for else.
> Maybe make this static inline in ufshcd.h?
>
> > + return 0;
> > +}
> > +
Hi Can,
On Sat, 2020-05-02 at 15:47 +0800, Can Guo wrote:
> Hi Stanley,
> On 2020-05-01 22:38, Stanley Chu wrote:
> > WriteBooster feature can be supported by some pre-3.1 UFS devices
> > by upgrading firmware.
> >
> > To enable WriteBooster feature in such devices, introduce a device
> > quirk to relax the entrance condition of ufshcd_wb_probe() to allow
> > host driver to check those devices' WriteBooster capability.
> >
> > WriteBooster feature can be available if below all conditions are
> > satisfied,
> >
> > 1. Host enables WriteBooster capability
> > 2. UFS 3.1 device or UFS pre-3.1 device with quirk
> > UFS_DEVICE_QUIRK_SUPPORT_EXTENDED_FEATURES enabled
> > 3. Device descriptor has dExtendedUFSFeaturesSupport field
> > 4. WriteBooster support is specified in above field
> >
> > Signed-off-by: Stanley Chu <[email protected]>
> > ---
> > drivers/scsi/ufs/ufs_quirks.h | 7 ++++
> > drivers/scsi/ufs/ufshcd.c | 66 ++++++++++++++++++++++-------------
> > 2 files changed, 48 insertions(+), 25 deletions(-)
> >
> > diff --git a/drivers/scsi/ufs/ufs_quirks.h
> > b/drivers/scsi/ufs/ufs_quirks.h
> > index df7a1e6805a3..e3175a63c676 100644
> > --- a/drivers/scsi/ufs/ufs_quirks.h
> > +++ b/drivers/scsi/ufs/ufs_quirks.h
> > @@ -101,4 +101,11 @@ struct ufs_dev_fix {
> > */
> > #define UFS_DEVICE_QUIRK_HOST_VS_DEBUGSAVECONFIGTIME (1 << 9)
> >
> > +/*
> > + * Some pre-3.1 UFS devices can support extended features by upgrading
> > + * the firmware. Enable this quirk to make UFS core driver probe and
> > enable
> > + * supported features on such devices.
> > + */
> > +#define UFS_DEVICE_QUIRK_SUPPORT_EXTENDED_FEATURES (1 << 10)
> > +
> > #endif /* UFS_QUIRKS_H_ */
> > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> > index 915e963398c4..c6668799d956 100644
> > --- a/drivers/scsi/ufs/ufshcd.c
> > +++ b/drivers/scsi/ufs/ufshcd.c
> > @@ -229,6 +229,8 @@ static struct ufs_dev_fix ufs_fixups[] = {
> > UFS_DEVICE_QUIRK_HOST_PA_SAVECONFIGTIME),
> > UFS_FIX(UFS_VENDOR_SKHYNIX, "hB8aL1" /*H28U62301AMR*/,
> > UFS_DEVICE_QUIRK_HOST_VS_DEBUGSAVECONFIGTIME),
> > + UFS_FIX(UFS_VENDOR_SKHYNIX, "H9HQ21AFAMZDAR",
> > + UFS_DEVICE_QUIRK_SUPPORT_EXTENDED_FEATURES),
> >
> > END_FIX
> > };
> > @@ -6800,9 +6802,19 @@ static int ufshcd_scsi_add_wlus(struct ufs_hba
> > *hba)
> >
> > static void ufshcd_wb_probe(struct ufs_hba *hba, u8 *desc_buf)
> > {
> > + if (!ufshcd_is_wb_allowed(hba))
> > + return;
> > +
> > + if (hba->desc_size.dev_desc <= DEVICE_DESC_PARAM_EXT_UFS_FEATURE_SUP)
> > + goto wb_disabled;
> > +
> > hba->dev_info.d_ext_ufs_feature_sup =
> > get_unaligned_be32(desc_buf +
> > DEVICE_DESC_PARAM_EXT_UFS_FEATURE_SUP);
> > +
> > + if (!(hba->dev_info.d_ext_ufs_feature_sup &
> > UFS_DEV_WRITE_BOOSTER_SUP))
> > + goto wb_disabled;
> > +
> > /*
> > * WB may be supported but not configured while provisioning.
> > * The spec says, in dedicated wb buffer mode,
> > @@ -6818,11 +6830,29 @@ static void ufshcd_wb_probe(struct ufs_hba
> > *hba, u8 *desc_buf)
> > hba->dev_info.b_presrv_uspc_en =
> > desc_buf[DEVICE_DESC_PARAM_WB_PRESRV_USRSPC_EN];
> >
> > - if (!((hba->dev_info.d_ext_ufs_feature_sup &
> > - UFS_DEV_WRITE_BOOSTER_SUP) &&
> > - hba->dev_info.b_wb_buffer_type &&
> > + if (!(hba->dev_info.b_wb_buffer_type &&
> > hba->dev_info.d_wb_alloc_units))
> > - hba->caps &= ~UFSHCD_CAP_WB_EN;
> > + goto wb_disabled;
> > +
> > + return;
> > +
> > +wb_disabled:
> > + hba->caps &= ~UFSHCD_CAP_WB_EN;
> > +}
> > +
> > +static void ufs_fixup_device_setup(struct ufs_hba *hba)
> > +{
> > + struct ufs_dev_fix *f;
> > + struct ufs_dev_info *dev_info = &hba->dev_info;
> > +
> > + for (f = ufs_fixups; f->quirk; f++) {
> > + if ((f->wmanufacturerid == dev_info->wmanufacturerid ||
> > + f->wmanufacturerid == UFS_ANY_VENDOR) &&
> > + ((dev_info->model &&
> > + STR_PRFX_EQUAL(f->model, dev_info->model)) ||
> > + !strcmp(f->model, UFS_ANY_MODEL)))
> > + hba->dev_quirks |= f->quirk;
> > + }
> > }
> >
> > static int ufs_get_device_desc(struct ufs_hba *hba)
> > @@ -6862,10 +6892,6 @@ static int ufs_get_device_desc(struct ufs_hba
> > *hba)
> >
> > model_index = desc_buf[DEVICE_DESC_PARAM_PRDCT_NAME];
> >
> > - /* Enable WB only for UFS-3.1 */
> > - if (dev_info->wspecversion >= 0x310)
> > - ufshcd_wb_probe(hba, desc_buf);
> > -
> > err = ufshcd_read_string_desc(hba, model_index,
> > &dev_info->model, SD_ASCII_STD);
> > if (err < 0) {
> > @@ -6874,6 +6900,13 @@ static int ufs_get_device_desc(struct ufs_hba
> > *hba)
> > goto out;
> > }
> >
> > + ufs_fixup_device_setup(hba);
> > +
> > + /* Enable WB only for UFS-3.1 */
>
> Also update this comment to reflect your change?
>
> > + if (dev_info->wspecversion >= 0x310 ||
> > + (hba->dev_quirks & UFS_DEVICE_QUIRK_SUPPORT_EXTENDED_FEATURES))
> > + ufshcd_wb_probe(hba, desc_buf);
> > +
>
> Can we somehow move this after ufshcd_tune_unipro_params() or come up
> with
> a better way to leverage ufshcd_vops_apply_dev_quirks()? I am asking
> this
> because if we only rely on adding quirks to ufs_fixups in ufshcd.c, the
> table will keep growing and I am sure it will - as flash vendors are
> trying
> to make their UFS2.1 products to be capable of WB (different densities
> and
> different NAND processes from different vendors, the combos can be quite
> a
> few). Meanwhile, some models are specifically made for some customers to
> support WB, meaning having them in the table may not help in a
> generalized
> way, and it is not like some hot fixes that we have to take, it is just
> for
> a non-standard feature. If we can leverage
> ufshcd_vops_apply_dev_quirks(),
> SoC vendors can freely add the quirk without touching ufs_fixups table,
> which means you don't need to update ufs_fixups every time just for
> adding
> a new model (GKI rules), you can have your own WB white list in vendor
> driver. What do you think?
>
> Thanks,
>
> Can Guo.
Very appreciate your useful and constructive comments : )
Please take a look at v4. In v4, I introduce a "fixup_dev_quirks" vop to
allow vendors to "fix" device quirks which can be a general solution not
only for newly introduced UFS_DEVICE_QUIRK_SUPPORT_EXTENDED_FEATURES but
also for other quirks.
Thanks,
Stanley Chu
>
> > /*
> > * ufshcd_read_string_desc returns size of the string
> > * reset the error value
> > @@ -6893,21 +6926,6 @@ static void ufs_put_device_desc(struct ufs_hba
> > *hba)
> > dev_info->model = NULL;
> > }
> >
> > -static void ufs_fixup_device_setup(struct ufs_hba *hba)
> > -{
> > - struct ufs_dev_fix *f;
> > - struct ufs_dev_info *dev_info = &hba->dev_info;
> > -
> > - for (f = ufs_fixups; f->quirk; f++) {
> > - if ((f->wmanufacturerid == dev_info->wmanufacturerid ||
> > - f->wmanufacturerid == UFS_ANY_VENDOR) &&
> > - ((dev_info->model &&
> > - STR_PRFX_EQUAL(f->model, dev_info->model)) ||
> > - !strcmp(f->model, UFS_ANY_MODEL)))
> > - hba->dev_quirks |= f->quirk;
> > - }
> > -}
> > -
> > /**
> > * ufshcd_tune_pa_tactivate - Tunes PA_TActivate of local UniPro
> > * @hba: per-adapter instance
> > @@ -7244,8 +7262,6 @@ static int ufshcd_device_params_init(struct
> > ufs_hba *hba)
> >
> > ufshcd_get_ref_clk_gating_wait(hba);
> >
> > - ufs_fixup_device_setup(hba);
> > -
> > if (!ufshcd_query_flag_retry(hba, UPIU_QUERY_OPCODE_READ_FLAG,
> > QUERY_FLAG_IDN_PWR_ON_WPE, &flag))
> > hba->dev_info.f_power_on_wp_en = flag;