* [PATCH v5 0/3] scsi: ufs: Allow RTT negotiation
@ 2024-05-23 12:58 Avri Altman
2024-05-23 12:58 ` [PATCH v5 1/3] " Avri Altman
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Avri Altman @ 2024-05-23 12:58 UTC (permalink / raw
To: Martin K . Petersen
Cc: Bart Van Assche, Bean Huo, Peter Wang, linux-scsi, linux-kernel,
Avri Altman
The rtt-upiu packets precede any data-out upiu packets, thus
synchronizing the data input to the device: this mostly applies to write
operations, but there are other operations that requires rtt as well.
There are several rules binding this rtt - data-out dialog, specifically
There can be at most outstanding bMaxNumOfRTT such packets. This might
have an effect on write performance (sequential write in particular), as
each data-out upiu must wait for its rtt sibling.
UFSHCI expects bMaxNumOfRTT to be min(bDeviceRTTCap, NORTT). However,
as of today, there does not appear to be no-one who sets it: not the
host controller nor the driver. It wasn't an issue up to now:
bMaxNumOfRTT is set to 2 after manufacturing, and wasn't limiting the
write performance.
UFS4.0, and specifically gear 5 changes this, and requires the device to
be more attentive. This doesn't come free - the device has to allocate
more resources to that end, but the sequential write performance
improvement is significant. Early measurements shows 25% gain when
moving from rtt 2 to 9. Therefore, set bMaxNumOfRTT to be
min(bDeviceRTTCap, NORTT) as UFSHCI expects.
v4 -> v5:
Quiesce the queues before writing bMaxNumOfRTT (Bart)
Make bDeviceRTTCap available in ufshcd_device_params_init() (Bart)
v3 -> v4:
Allow bMaxNumOfRTT to be configured via sysfs (Bart)
v2 -> v3:
Allow platform vendors to take precedence having their own rtt
negotiation mechanism (Peter)
v1 -> v2:
bMaxNumOfRTT is a Persistent attribute - do not override if it was
written (Bean)
Avri Altman (3):
scsi: ufs: Allow RTT negotiation
scsi: ufs: Allow platform vendors to set rtt
scsi: ufs: sysfs: Make max_number_of_rtt read-write
Documentation/ABI/testing/sysfs-driver-ufs | 14 +++--
drivers/ufs/core/ufs-sysfs.c | 72 +++++++++++++++++++++-
drivers/ufs/core/ufshcd-priv.h | 12 ++++
drivers/ufs/core/ufshcd.c | 53 ++++++++++++----
include/ufs/ufs.h | 2 +
include/ufs/ufshcd.h | 4 ++
include/ufs/ufshci.h | 1 +
7 files changed, 139 insertions(+), 19 deletions(-)
--
2.34.1
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v5 1/3] scsi: ufs: Allow RTT negotiation
2024-05-23 12:58 [PATCH v5 0/3] scsi: ufs: Allow RTT negotiation Avri Altman
@ 2024-05-23 12:58 ` Avri Altman
2024-05-23 12:58 ` [PATCH v5 2/3] scsi: ufs: Allow platform vendors to set rtt Avri Altman
2024-05-23 12:58 ` [PATCH v5 3/3] scsi: ufs: sysfs: Make max_number_of_rtt read-write Avri Altman
2 siblings, 0 replies; 12+ messages in thread
From: Avri Altman @ 2024-05-23 12:58 UTC (permalink / raw
To: Martin K . Petersen
Cc: Bart Van Assche, Bean Huo, Peter Wang, linux-scsi, linux-kernel,
Avri Altman
The rtt-upiu packets precede any data-out upiu packets, thus
synchronizing the data input to the device: this mostly applies to write
operations, but there are other operations that requires rtt as well.
There are several rules binding this rtt - data-out dialog, specifically
There can be at most outstanding bMaxNumOfRTT such packets. This might
have an effect on write performance (sequential write in particular), as
each data-out upiu must wait for its rtt sibling.
UFSHCI expects bMaxNumOfRTT to be min(bDeviceRTTCap, NORTT). However,
as of today, there does not appears to be no-one who sets it: not the
host controller nor the driver. It wasn't an issue up to now:
bMaxNumOfRTT is set to 2 after manufacturing, and wasn't limiting the
write performance.
UFS4.0, and specifically gear 5 changes this, and requires the device to
be more attentive. This doesn't come free - the device has to allocate
more resources to that end, but the sequential write performance
improvement is significant. Early measurements shows 25% gain when
moving from rtt 2 to 9. Therefore, set bMaxNumOfRTT to be
min(bDeviceRTTCap, NORTT) as UFSHCI expects.
Signed-off-by: Avri Altman <avri.altman@wdc.com>
Reviewed-by: Bean Huo <beanhuo@micron.com>
---
drivers/ufs/core/ufshcd.c | 38 ++++++++++++++++++++++++++++++++++++++
include/ufs/ufs.h | 2 ++
include/ufs/ufshcd.h | 2 ++
include/ufs/ufshci.h | 1 +
4 files changed, 43 insertions(+)
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 0819ddafe7a6..7df8bcacbe7e 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -102,6 +102,9 @@
/* Default RTC update every 10 seconds */
#define UFS_RTC_UPDATE_INTERVAL_MS (10 * MSEC_PER_SEC)
+/* bMaxNumOfRTT is equal to two after device manufacturing */
+#define DEFAULT_MAX_NUM_RTT 2
+
/* UFSHC 4.0 compliant HC support this mode. */
static bool use_mcq_mode = true;
@@ -2405,6 +2408,8 @@ static inline int ufshcd_hba_capabilities(struct ufs_hba *hba)
((hba->capabilities & MASK_TASK_MANAGEMENT_REQUEST_SLOTS) >> 16) + 1;
hba->reserved_slot = hba->nutrs - 1;
+ hba->nortt = FIELD_GET(MASK_NUMBER_OUTSTANDING_RTT, hba->capabilities) + 1;
+
/* Read crypto capabilities */
err = ufshcd_hba_init_crypto_capabilities(hba);
if (err) {
@@ -8119,6 +8124,35 @@ static void ufshcd_ext_iid_probe(struct ufs_hba *hba, u8 *desc_buf)
dev_info->b_ext_iid_en = ext_iid_en;
}
+static void ufshcd_set_rtt(struct ufs_hba *hba)
+{
+ struct ufs_dev_info *dev_info = &hba->dev_info;
+ u32 rtt = 0;
+ u32 dev_rtt = 0;
+
+ /* RTT override makes sense only for UFS-4.0 and above */
+ if (dev_info->wspecversion < 0x400)
+ return;
+
+ if (ufshcd_query_attr_retry(hba, UPIU_QUERY_OPCODE_READ_ATTR,
+ QUERY_ATTR_IDN_MAX_NUM_OF_RTT, 0, 0, &dev_rtt)) {
+ dev_err(hba->dev, "failed reading bMaxNumOfRTT\n");
+ return;
+ }
+
+ /* do not override if it was already written */
+ if (dev_rtt != DEFAULT_MAX_NUM_RTT)
+ return;
+
+ rtt = min_t(int, dev_info->rtt_cap, hba->nortt);
+ if (rtt == dev_rtt)
+ return;
+
+ if (ufshcd_query_attr_retry(hba, UPIU_QUERY_OPCODE_WRITE_ATTR,
+ QUERY_ATTR_IDN_MAX_NUM_OF_RTT, 0, 0, &rtt))
+ dev_err(hba->dev, "failed writing bMaxNumOfRTT\n");
+}
+
void ufshcd_fixup_dev_quirks(struct ufs_hba *hba,
const struct ufs_dev_quirk *fixups)
{
@@ -8254,6 +8288,8 @@ static int ufs_get_device_desc(struct ufs_hba *hba)
desc_buf[DEVICE_DESC_PARAM_SPEC_VER + 1];
dev_info->bqueuedepth = desc_buf[DEVICE_DESC_PARAM_Q_DPTH];
+ dev_info->rtt_cap = desc_buf[DEVICE_DESC_PARAM_RTT_CAP];
+
model_index = desc_buf[DEVICE_DESC_PARAM_PRDCT_NAME];
err = ufshcd_read_string_desc(hba, model_index,
@@ -8506,6 +8542,8 @@ static int ufshcd_device_params_init(struct ufs_hba *hba)
goto out;
}
+ ufshcd_set_rtt(hba);
+
ufshcd_get_ref_clk_gating_wait(hba);
if (!ufshcd_query_flag_retry(hba, UPIU_QUERY_OPCODE_READ_FLAG,
diff --git a/include/ufs/ufs.h b/include/ufs/ufs.h
index b6003749bc83..853e95957c31 100644
--- a/include/ufs/ufs.h
+++ b/include/ufs/ufs.h
@@ -592,6 +592,8 @@ struct ufs_dev_info {
enum ufs_rtc_time rtc_type;
time64_t rtc_time_baseline;
u32 rtc_update_period;
+
+ u8 rtt_cap; /* bDeviceRTTCap */
};
/*
diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
index bad88bd91995..d74bd2d67b06 100644
--- a/include/ufs/ufshcd.h
+++ b/include/ufs/ufshcd.h
@@ -819,6 +819,7 @@ enum ufshcd_mcq_opr {
* @capabilities: UFS Controller Capabilities
* @mcq_capabilities: UFS Multi Circular Queue capabilities
* @nutrs: Transfer Request Queue depth supported by controller
+ * @nortt - Max outstanding RTTs supported by controller
* @nutmrs: Task Management Queue depth supported by controller
* @reserved_slot: Used to submit device commands. Protected by @dev_cmd.lock.
* @ufs_version: UFS Version to which controller complies
@@ -957,6 +958,7 @@ struct ufs_hba {
u32 capabilities;
int nutrs;
+ int nortt;
u32 mcq_capabilities;
int nutmrs;
u32 reserved_slot;
diff --git a/include/ufs/ufshci.h b/include/ufs/ufshci.h
index 385e1c6b8d60..c50f92bf2e1d 100644
--- a/include/ufs/ufshci.h
+++ b/include/ufs/ufshci.h
@@ -68,6 +68,7 @@ enum {
/* Controller capability masks */
enum {
MASK_TRANSFER_REQUESTS_SLOTS = 0x0000001F,
+ MASK_NUMBER_OUTSTANDING_RTT = 0x0000FF00,
MASK_TASK_MANAGEMENT_REQUEST_SLOTS = 0x00070000,
MASK_EHSLUTRD_SUPPORTED = 0x00400000,
MASK_AUTO_HIBERN8_SUPPORT = 0x00800000,
--
2.34.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v5 2/3] scsi: ufs: Allow platform vendors to set rtt
2024-05-23 12:58 [PATCH v5 0/3] scsi: ufs: Allow RTT negotiation Avri Altman
2024-05-23 12:58 ` [PATCH v5 1/3] " Avri Altman
@ 2024-05-23 12:58 ` Avri Altman
2024-05-23 13:03 ` Christoph Hellwig
2024-05-23 12:58 ` [PATCH v5 3/3] scsi: ufs: sysfs: Make max_number_of_rtt read-write Avri Altman
2 siblings, 1 reply; 12+ messages in thread
From: Avri Altman @ 2024-05-23 12:58 UTC (permalink / raw
To: Martin K . Petersen
Cc: Bart Van Assche, Bean Huo, Peter Wang, linux-scsi, linux-kernel,
Avri Altman
Allow platform vendors to take precedence having their own rtt
negotiation mechanism. This makes sense because the host controller's
nortt characteristic may vary among vendors.
Signed-off-by: Avri Altman <avri.altman@wdc.com>
---
drivers/ufs/core/ufshcd.c | 5 ++++-
include/ufs/ufshcd.h | 2 ++
2 files changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 7df8bcacbe7e..d8e0e80d66a5 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -8542,7 +8542,10 @@ static int ufshcd_device_params_init(struct ufs_hba *hba)
goto out;
}
- ufshcd_set_rtt(hba);
+ if (hba->vops && hba->vops->set_rtt)
+ hba->vops->set_rtt(hba);
+ else
+ ufshcd_set_rtt(hba);
ufshcd_get_ref_clk_gating_wait(hba);
diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
index d74bd2d67b06..495b50a72f9f 100644
--- a/include/ufs/ufshcd.h
+++ b/include/ufs/ufshcd.h
@@ -329,6 +329,7 @@ struct ufs_pwr_mode_info {
* @get_outstanding_cqs: called to get outstanding completion queues
* @config_esi: called to config Event Specific Interrupt
* @config_scsi_dev: called to configure SCSI device parameters
+ * @set_rtt: negotiate rtt
*/
struct ufs_hba_variant_ops {
const char *name;
@@ -374,6 +375,7 @@ struct ufs_hba_variant_ops {
int (*get_outstanding_cqs)(struct ufs_hba *hba,
unsigned long *ocqs);
int (*config_esi)(struct ufs_hba *hba);
+ void (*set_rtt)(struct ufs_hba *hba);
};
/* clock gating state */
--
2.34.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v5 3/3] scsi: ufs: sysfs: Make max_number_of_rtt read-write
2024-05-23 12:58 [PATCH v5 0/3] scsi: ufs: Allow RTT negotiation Avri Altman
2024-05-23 12:58 ` [PATCH v5 1/3] " Avri Altman
2024-05-23 12:58 ` [PATCH v5 2/3] scsi: ufs: Allow platform vendors to set rtt Avri Altman
@ 2024-05-23 12:58 ` Avri Altman
2024-05-23 23:05 ` Bart Van Assche
2 siblings, 1 reply; 12+ messages in thread
From: Avri Altman @ 2024-05-23 12:58 UTC (permalink / raw
To: Martin K . Petersen
Cc: Bart Van Assche, Bean Huo, Peter Wang, linux-scsi, linux-kernel,
Avri Altman
Given the importance of the RTT parameter, we want to be able to
configure it via sysfs. This is because UFS users should be discouraged
from change UFS device parameters without the UFSHCI driver being aware
of these changes.
Signed-off-by: Avri Altman <avri.altman@wdc.com>
---
Documentation/ABI/testing/sysfs-driver-ufs | 14 +++--
drivers/ufs/core/ufs-sysfs.c | 72 +++++++++++++++++++++-
drivers/ufs/core/ufshcd-priv.h | 12 ++++
drivers/ufs/core/ufshcd.c | 12 ----
4 files changed, 91 insertions(+), 19 deletions(-)
diff --git a/Documentation/ABI/testing/sysfs-driver-ufs b/Documentation/ABI/testing/sysfs-driver-ufs
index 5bf7073b4f75..fe943ce76c60 100644
--- a/Documentation/ABI/testing/sysfs-driver-ufs
+++ b/Documentation/ABI/testing/sysfs-driver-ufs
@@ -920,14 +920,16 @@ Description: This file shows whether the configuration descriptor is locked.
What: /sys/bus/platform/drivers/ufshcd/*/attributes/max_number_of_rtt
What: /sys/bus/platform/devices/*.ufs/attributes/max_number_of_rtt
-Date: February 2018
-Contact: Stanislav Nijnikov <stanislav.nijnikov@wdc.com>
+Date: May 2024
+Contact: Avri Altman <avri.altman@wdc.com>
Description: This file provides the maximum current number of
- outstanding RTTs in device that is allowed. The full
- information about the attribute could be found at
- UFS specifications 2.1.
+ outstanding RTTs in device that is allowed. bMaxNumOfRTT is a
+ read-write persistent attribute and is equal to two after device
+ manufacturing. It shall not be set to a value greater than
+ bDeviceRTTCap value, and it may be set only when the hw queues are
+ empty.
- The file is read only.
+ The file is read write.
What: /sys/bus/platform/drivers/ufshcd/*/attributes/exception_event_control
What: /sys/bus/platform/devices/*.ufs/attributes/exception_event_control
diff --git a/drivers/ufs/core/ufs-sysfs.c b/drivers/ufs/core/ufs-sysfs.c
index 3d049967f6bc..97fe1e9e63da 100644
--- a/drivers/ufs/core/ufs-sysfs.c
+++ b/drivers/ufs/core/ufs-sysfs.c
@@ -1340,6 +1340,77 @@ static const struct attribute_group ufs_sysfs_flags_group = {
.attrs = ufs_sysfs_device_flags,
};
+static ssize_t max_number_of_rtt_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct ufs_hba *hba = dev_get_drvdata(dev);
+ u32 rtt;
+ int ret;
+
+ down(&hba->host_sem);
+ if (!ufshcd_is_user_access_allowed(hba)) {
+ up(&hba->host_sem);
+ return -EBUSY;
+ }
+
+ ufshcd_rpm_get_sync(hba);
+ ret = ufshcd_query_attr(hba, UPIU_QUERY_OPCODE_READ_ATTR,
+ QUERY_ATTR_IDN_MAX_NUM_OF_RTT, 0, 0, &rtt);
+ ufshcd_rpm_put_sync(hba);
+
+ if (ret)
+ goto out;
+
+ ret = sysfs_emit(buf, "0x%08X\n", rtt);
+
+out:
+ up(&hba->host_sem);
+ return ret;
+}
+
+static ssize_t max_number_of_rtt_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct ufs_hba *hba = dev_get_drvdata(dev);
+ struct ufs_dev_info *dev_info = &hba->dev_info;
+ unsigned int rtt;
+ int ret;
+
+ if (kstrtouint(buf, 0, &rtt))
+ return -EINVAL;
+
+ if (rtt > dev_info->rtt_cap) {
+ dev_err(dev, "rtt can be at most bDeviceRTTCap\n");
+ return -EINVAL;
+ }
+
+ down(&hba->host_sem);
+ if (!ufshcd_is_user_access_allowed(hba)) {
+ ret = -EBUSY;
+ goto out;
+ }
+
+ ufshcd_rpm_get_sync(hba);
+
+ /* make sure that there are no outstanding requests when rtt is set */
+ ufshcd_scsi_block_requests(hba);
+ blk_mq_wait_quiesce_done(&hba->host->tag_set);
+
+ ret = ufshcd_query_attr(hba, UPIU_QUERY_OPCODE_WRITE_ATTR,
+ QUERY_ATTR_IDN_MAX_NUM_OF_RTT, 0, 0, &rtt);
+
+ ufshcd_scsi_unblock_requests(hba);
+
+ ufshcd_rpm_put_sync(hba);
+
+out:
+ up(&hba->host_sem);
+ return ret < 0 ? ret : count;
+}
+
+static DEVICE_ATTR_RW(max_number_of_rtt);
+
static inline bool ufshcd_is_wb_attrs(enum attr_idn idn)
{
return idn >= QUERY_ATTR_IDN_WB_FLUSH_STATUS &&
@@ -1387,7 +1458,6 @@ UFS_ATTRIBUTE(max_data_in_size, _MAX_DATA_IN);
UFS_ATTRIBUTE(max_data_out_size, _MAX_DATA_OUT);
UFS_ATTRIBUTE(reference_clock_frequency, _REF_CLK_FREQ);
UFS_ATTRIBUTE(configuration_descriptor_lock, _CONF_DESC_LOCK);
-UFS_ATTRIBUTE(max_number_of_rtt, _MAX_NUM_OF_RTT);
UFS_ATTRIBUTE(exception_event_control, _EE_CONTROL);
UFS_ATTRIBUTE(exception_event_status, _EE_STATUS);
UFS_ATTRIBUTE(ffu_status, _FFU_STATUS);
diff --git a/drivers/ufs/core/ufshcd-priv.h b/drivers/ufs/core/ufshcd-priv.h
index f42d99ce5bf1..691987e2e5f5 100644
--- a/drivers/ufs/core/ufshcd-priv.h
+++ b/drivers/ufs/core/ufshcd-priv.h
@@ -32,6 +32,18 @@ static inline bool ufshcd_is_wb_buf_flush_allowed(struct ufs_hba *hba)
!(hba->quirks & UFSHCI_QUIRK_SKIP_MANUAL_WB_FLUSH_CTRL);
}
+static inline void ufshcd_scsi_unblock_requests(struct ufs_hba *hba)
+{
+ if (atomic_dec_and_test(&hba->scsi_block_reqs_cnt))
+ scsi_unblock_requests(hba->host);
+}
+
+static inline void ufshcd_scsi_block_requests(struct ufs_hba *hba)
+{
+ if (atomic_inc_return(&hba->scsi_block_reqs_cnt) == 1)
+ scsi_block_requests(hba->host);
+}
+
#ifdef CONFIG_SCSI_UFS_HWMON
void ufs_hwmon_probe(struct ufs_hba *hba, u8 mask);
void ufs_hwmon_remove(struct ufs_hba *hba);
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index d8e0e80d66a5..f470180e6efb 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -332,18 +332,6 @@ static void ufshcd_configure_wb(struct ufs_hba *hba)
ufshcd_wb_toggle_buf_flush(hba, true);
}
-static void ufshcd_scsi_unblock_requests(struct ufs_hba *hba)
-{
- if (atomic_dec_and_test(&hba->scsi_block_reqs_cnt))
- scsi_unblock_requests(hba->host);
-}
-
-static void ufshcd_scsi_block_requests(struct ufs_hba *hba)
-{
- if (atomic_inc_return(&hba->scsi_block_reqs_cnt) == 1)
- scsi_block_requests(hba->host);
-}
-
static void ufshcd_add_cmd_upiu_trace(struct ufs_hba *hba, unsigned int tag,
enum ufs_trace_str_t str_t)
{
--
2.34.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v5 2/3] scsi: ufs: Allow platform vendors to set rtt
2024-05-23 12:58 ` [PATCH v5 2/3] scsi: ufs: Allow platform vendors to set rtt Avri Altman
@ 2024-05-23 13:03 ` Christoph Hellwig
2024-05-23 13:09 ` Avri Altman
0 siblings, 1 reply; 12+ messages in thread
From: Christoph Hellwig @ 2024-05-23 13:03 UTC (permalink / raw
To: Avri Altman
Cc: Martin K . Petersen, Bart Van Assche, Bean Huo, Peter Wang,
linux-scsi, linux-kernel
On Thu, May 23, 2024 at 03:58:25PM +0300, Avri Altman wrote:
> Allow platform vendors to take precedence having their own rtt
> negotiation mechanism. This makes sense because the host controller's
> nortt characteristic may vary among vendors.
Platform vendors have absolutelyt no business saying anything.
Fortunately that's not what you're actually doing, but I really don't
understand your vendor fetish.
^ permalink raw reply [flat|nested] 12+ messages in thread
* RE: [PATCH v5 2/3] scsi: ufs: Allow platform vendors to set rtt
2024-05-23 13:03 ` Christoph Hellwig
@ 2024-05-23 13:09 ` Avri Altman
2024-05-23 13:11 ` hch
0 siblings, 1 reply; 12+ messages in thread
From: Avri Altman @ 2024-05-23 13:09 UTC (permalink / raw
To: hch@infradead.org
Cc: Martin K . Petersen, Bart Van Assche, Bean Huo, Peter Wang,
linux-scsi@vger.kernel.org, linux-kernel@vger.kernel.org
> On Thu, May 23, 2024 at 03:58:25PM +0300, Avri Altman wrote:
> > Allow platform vendors to take precedence having their own rtt
> > negotiation mechanism. This makes sense because the host controller's
> > nortt characteristic may vary among vendors.
>
> Platform vendors have absolutelyt no business saying anything.
>
> Fortunately that's not what you're actually doing, but I really don't understand
> your vendor fetish.
It was a specific request from MTK to allow override their host controller capabilities.
Thanks,
Avri
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v5 2/3] scsi: ufs: Allow platform vendors to set rtt
2024-05-23 13:09 ` Avri Altman
@ 2024-05-23 13:11 ` hch
2024-05-24 6:06 ` Peter Wang (王信友)
0 siblings, 1 reply; 12+ messages in thread
From: hch @ 2024-05-23 13:11 UTC (permalink / raw
To: Avri Altman
Cc: hch@infradead.org, Martin K . Petersen, Bart Van Assche, Bean Huo,
Peter Wang, linux-scsi@vger.kernel.org,
linux-kernel@vger.kernel.org
On Thu, May 23, 2024 at 01:09:25PM +0000, Avri Altman wrote:
> > On Thu, May 23, 2024 at 03:58:25PM +0300, Avri Altman wrote:
> > > Allow platform vendors to take precedence having their own rtt
> > > negotiation mechanism. This makes sense because the host controller's
> > > nortt characteristic may vary among vendors.
> >
> > Platform vendors have absolutelyt no business saying anything.
> >
> > Fortunately that's not what you're actually doing, but I really don't understand
> > your vendor fetish.
> It was a specific request from MTK to allow override their host controller capabilities.
Then they need to submit a patch just like anyone who wants to improve
Linux. And not trick their NAND supplier into adding an unused hook…
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v5 3/3] scsi: ufs: sysfs: Make max_number_of_rtt read-write
2024-05-23 12:58 ` [PATCH v5 3/3] scsi: ufs: sysfs: Make max_number_of_rtt read-write Avri Altman
@ 2024-05-23 23:05 ` Bart Van Assche
2024-05-24 3:54 ` Avri Altman
0 siblings, 1 reply; 12+ messages in thread
From: Bart Van Assche @ 2024-05-23 23:05 UTC (permalink / raw
To: Avri Altman, Martin K . Petersen
Cc: Bean Huo, Peter Wang, linux-scsi, linux-kernel
On 5/23/24 05:58, Avri Altman wrote:
> + /* make sure that there are no outstanding requests when rtt is set */
> + ufshcd_scsi_block_requests(hba);
> + blk_mq_wait_quiesce_done(&hba->host->tag_set);
> +
> + ret = ufshcd_query_attr(hba, UPIU_QUERY_OPCODE_WRITE_ATTR,
> + QUERY_ATTR_IDN_MAX_NUM_OF_RTT, 0, 0, &rtt);
> +
> + ufshcd_scsi_unblock_requests(hba);
The above doesn't look correct to me. ufshcd_scsi_block_requests() does
not guarantee that all pending commands have finished by the time it
returns. Please blk_mq_freeze_queue() / blk_mq_unfreeze_queue() instead.
Thanks,
Bart.
^ permalink raw reply [flat|nested] 12+ messages in thread
* RE: [PATCH v5 3/3] scsi: ufs: sysfs: Make max_number_of_rtt read-write
2024-05-23 23:05 ` Bart Van Assche
@ 2024-05-24 3:54 ` Avri Altman
0 siblings, 0 replies; 12+ messages in thread
From: Avri Altman @ 2024-05-24 3:54 UTC (permalink / raw
To: Bart Van Assche, Martin K . Petersen
Cc: Bean Huo, Peter Wang, linux-scsi@vger.kernel.org,
linux-kernel@vger.kernel.org
>
> On 5/23/24 05:58, Avri Altman wrote:
> > + /* make sure that there are no outstanding requests when rtt is set */
> > + ufshcd_scsi_block_requests(hba);
> > + blk_mq_wait_quiesce_done(&hba->host->tag_set);
> > +
> > + ret = ufshcd_query_attr(hba, UPIU_QUERY_OPCODE_WRITE_ATTR,
> > + QUERY_ATTR_IDN_MAX_NUM_OF_RTT, 0, 0, &rtt);
> > +
> > + ufshcd_scsi_unblock_requests(hba);
>
> The above doesn't look correct to me. ufshcd_scsi_block_requests() does not
> guarantee that all pending commands have finished by the time it returns.
> Please blk_mq_freeze_queue() / blk_mq_unfreeze_queue() instead.
Thanks. Will use those to drain the queues.
Thanks,
Avri
>
> Thanks,
>
> Bart.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v5 2/3] scsi: ufs: Allow platform vendors to set rtt
2024-05-23 13:11 ` hch
@ 2024-05-24 6:06 ` Peter Wang (王信友)
2024-05-24 13:56 ` Bart Van Assche
0 siblings, 1 reply; 12+ messages in thread
From: Peter Wang (王信友) @ 2024-05-24 6:06 UTC (permalink / raw
To: hch@infradead.org, Avri.Altman@wdc.com
Cc: beanhuo@micron.com, bvanassche@acm.org,
linux-kernel@vger.kernel.org, linux-scsi@vger.kernel.org,
martin.petersen@oracle.com
On Thu, 2024-05-23 at 06:11 -0700, hch@infradead.org wrote:
>
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> On Thu, May 23, 2024 at 01:09:25PM +0000, Avri Altman wrote:
> > > On Thu, May 23, 2024 at 03:58:25PM +0300, Avri Altman wrote:
> > > > Allow platform vendors to take precedence having their own rtt
> > > > negotiation mechanism. This makes sense because the host
> controller's
> > > > nortt characteristic may vary among vendors.
> > >
> > > Platform vendors have absolutelyt no business saying anything.
> > >
> > > Fortunately that's not what you're actually doing, but I really
> don't understand
> > > your vendor fetish.
> > It was a specific request from MTK to allow override their host
> controller capabilities.
>
> Then they need to submit a patch just like anyone who wants to
> improve
> Linux. And not trick their NAND supplier into adding an unused hook…
Hi Arvi,
Could you help add below patch for mediatek?
With below mediatek patch,
"Allow RTT negotiation" patch series will more complete and
mediatek platform not affect by this patch series.
Thanks
Peter
diff --git a/drivers/ufs/host/ufs-mediatek.c b/drivers/ufs/host/ufs-
mediatek.c
index c4f997196c57..f8725f3374f7 100644
--- a/drivers/ufs/host/ufs-mediatek.c
+++ b/drivers/ufs/host/ufs-mediatek.c
@@ -1777,6 +1777,32 @@ static int ufs_mtk_config_esi(struct ufs_hba
*hba)
return ufs_mtk_config_mcq(hba, true);
}
+static void ufs_mtk_set_rtt(struct ufs_hba *hba)
+{
+ struct ufs_dev_info *dev_info = &hba->dev_info;
+ u32 rtt = 0;
+ u32 dev_rtt = 0;
+
+ /* RTT override makes sense only for UFS-4.0 and above */
+ if (dev_info->wspecversion < 0x400)
+ return;
+
+ if (ufshcd_query_attr_retry(hba, UPIU_QUERY_OPCODE_READ_ATTR,
+ QUERY_ATTR_IDN_MAX_NUM_OF_RTT, 0,
0, &dev_rtt)) {
+ dev_err(hba->dev, "failed reading bMaxNumOfRTT\n");
+ return;
+ }
+
+ /* override if not mediatek support */
+ if (dev_rtt == MTK_MAX_NUM_RTT)
+ return;
+
+ rtt = MTK_MAX_NUM_RTT;
+ if (ufshcd_query_attr_retry(hba, UPIU_QUERY_OPCODE_WRITE_ATTR,
+ QUERY_ATTR_IDN_MAX_NUM_OF_RTT, 0,
0, &rtt))
+ dev_err(hba->dev, "failed writing bMaxNumOfRTT\n");
+}
+
/*
* struct ufs_hba_mtk_vops - UFS MTK specific variant operations
*
@@ -1805,6 +1831,7 @@ static const struct ufs_hba_variant_ops
ufs_hba_mtk_vops = {
.op_runtime_config = ufs_mtk_op_runtime_config,
.mcq_config_resource = ufs_mtk_mcq_config_resource,
.config_esi = ufs_mtk_config_esi,
+ .set_rtt = ufs_mtk_set_rtt,
};
/**
diff --git a/drivers/ufs/host/ufs-mediatek.h b/drivers/ufs/host/ufs-
mediatek.h
index 3ff17e95afab..05d76a6bd772 100644
--- a/drivers/ufs/host/ufs-mediatek.h
+++ b/drivers/ufs/host/ufs-mediatek.h
@@ -189,4 +189,7 @@ struct ufs_mtk_host {
/* MTK delay of autosuspend: 500 ms */
#define MTK_RPM_AUTOSUSPEND_DELAY_MS 500
+/* MTK RTT support number */
+#define MTK_MAX_NUM_RTT 2
+
#endif /* !_UFS_MEDIATEK_H */
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v5 2/3] scsi: ufs: Allow platform vendors to set rtt
2024-05-24 6:06 ` Peter Wang (王信友)
@ 2024-05-24 13:56 ` Bart Van Assche
2024-05-25 14:55 ` Avri Altman
0 siblings, 1 reply; 12+ messages in thread
From: Bart Van Assche @ 2024-05-24 13:56 UTC (permalink / raw
To: Peter Wang (王信友), hch@infradead.org,
Avri.Altman@wdc.com
Cc: beanhuo@micron.com, linux-kernel@vger.kernel.org,
linux-scsi@vger.kernel.org, martin.petersen@oracle.com
On 5/23/24 23:06, Peter Wang (王信友) wrote:
> diff --git a/drivers/ufs/host/ufs-mediatek.c b/drivers/ufs/host/ufs-
> mediatek.c
> index c4f997196c57..f8725f3374f7 100644
> --- a/drivers/ufs/host/ufs-mediatek.c
> +++ b/drivers/ufs/host/ufs-mediatek.c
> @@ -1777,6 +1777,32 @@ static int ufs_mtk_config_esi(struct ufs_hba
> *hba)
> return ufs_mtk_config_mcq(hba, true);
> }
>
> +static void ufs_mtk_set_rtt(struct ufs_hba *hba)
> +{
> + struct ufs_dev_info *dev_info = &hba->dev_info;
> + u32 rtt = 0;
> + u32 dev_rtt = 0;
> +
> + /* RTT override makes sense only for UFS-4.0 and above */
> + if (dev_info->wspecversion < 0x400)
> + return;
> +
> + if (ufshcd_query_attr_retry(hba, UPIU_QUERY_OPCODE_READ_ATTR,
> + QUERY_ATTR_IDN_MAX_NUM_OF_RTT, 0,
> 0, &dev_rtt)) {
> + dev_err(hba->dev, "failed reading bMaxNumOfRTT\n");
> + return;
> + }
> +
> + /* override if not mediatek support */
> + if (dev_rtt == MTK_MAX_NUM_RTT)
> + return;
> +
> + rtt = MTK_MAX_NUM_RTT;
> + if (ufshcd_query_attr_retry(hba, UPIU_QUERY_OPCODE_WRITE_ATTR,
> + QUERY_ATTR_IDN_MAX_NUM_OF_RTT, 0,
> 0, &rtt))
> + dev_err(hba->dev, "failed writing bMaxNumOfRTT\n");
> +}
> +
> /*
> * struct ufs_hba_mtk_vops - UFS MTK specific variant operations
> *
> @@ -1805,6 +1831,7 @@ static const struct ufs_hba_variant_ops
> ufs_hba_mtk_vops = {
> .op_runtime_config = ufs_mtk_op_runtime_config,
> .mcq_config_resource = ufs_mtk_mcq_config_resource,
> .config_esi = ufs_mtk_config_esi,
> + .set_rtt = ufs_mtk_set_rtt,
> };
>
> /**
> diff --git a/drivers/ufs/host/ufs-mediatek.h b/drivers/ufs/host/ufs-
> mediatek.h
> index 3ff17e95afab..05d76a6bd772 100644
> --- a/drivers/ufs/host/ufs-mediatek.h
> +++ b/drivers/ufs/host/ufs-mediatek.h
> @@ -189,4 +189,7 @@ struct ufs_mtk_host {
> /* MTK delay of autosuspend: 500 ms */
> #define MTK_RPM_AUTOSUSPEND_DELAY_MS 500
>
> +/* MTK RTT support number */
> +#define MTK_MAX_NUM_RTT 2
> +
> #endif /* !_UFS_MEDIATEK_H */
The above patch would result in duplication of code. We should avoid
code duplication if that's reasonably possible. Instead of applying the
above patch, I propose to add a callback function pointer in struct
ufs_hba_variant_ops that returns the maximum RTT value supported by the
host driver.
Thanks,
Bart.
^ permalink raw reply [flat|nested] 12+ messages in thread
* RE: [PATCH v5 2/3] scsi: ufs: Allow platform vendors to set rtt
2024-05-24 13:56 ` Bart Van Assche
@ 2024-05-25 14:55 ` Avri Altman
0 siblings, 0 replies; 12+ messages in thread
From: Avri Altman @ 2024-05-25 14:55 UTC (permalink / raw
To: Bart Van Assche, Peter Wang (王信友),
hch@infradead.org
Cc: beanhuo@micron.com, linux-kernel@vger.kernel.org,
linux-scsi@vger.kernel.org, martin.petersen@oracle.com
>
> On 5/23/24 23:06, Peter Wang (王信友) wrote:
> > diff --git a/drivers/ufs/host/ufs-mediatek.c b/drivers/ufs/host/ufs-
> > mediatek.c
> > index c4f997196c57..f8725f3374f7 100644
> > --- a/drivers/ufs/host/ufs-mediatek.c
> > +++ b/drivers/ufs/host/ufs-mediatek.c
> > @@ -1777,6 +1777,32 @@ static int ufs_mtk_config_esi(struct ufs_hba
> > *hba)
> > return ufs_mtk_config_mcq(hba, true);
> > }
> >
> > +static void ufs_mtk_set_rtt(struct ufs_hba *hba)
> > +{
> > + struct ufs_dev_info *dev_info = &hba->dev_info;
> > + u32 rtt = 0;
> > + u32 dev_rtt = 0;
> > +
> > + /* RTT override makes sense only for UFS-4.0 and above */
> > + if (dev_info->wspecversion < 0x400)
> > + return;
> > +
> > + if (ufshcd_query_attr_retry(hba, UPIU_QUERY_OPCODE_READ_ATTR,
> > + QUERY_ATTR_IDN_MAX_NUM_OF_RTT, 0,
> > 0, &dev_rtt)) {
> > + dev_err(hba->dev, "failed reading bMaxNumOfRTT\n");
> > + return;
> > + }
> > +
> > + /* override if not mediatek support */
> > + if (dev_rtt == MTK_MAX_NUM_RTT)
> > + return;
> > +
> > + rtt = MTK_MAX_NUM_RTT;
> > + if (ufshcd_query_attr_retry(hba, UPIU_QUERY_OPCODE_WRITE_ATTR,
> > + QUERY_ATTR_IDN_MAX_NUM_OF_RTT, 0,
> > 0, &rtt))
> > + dev_err(hba->dev, "failed writing bMaxNumOfRTT\n");
> > +}
> > +
> > /*
> > * struct ufs_hba_mtk_vops - UFS MTK specific variant operations
> > *
> > @@ -1805,6 +1831,7 @@ static const struct ufs_hba_variant_ops
> > ufs_hba_mtk_vops = {
> > .op_runtime_config = ufs_mtk_op_runtime_config,
> > .mcq_config_resource = ufs_mtk_mcq_config_resource,
> > .config_esi = ufs_mtk_config_esi,
> > + .set_rtt = ufs_mtk_set_rtt,
> > };
> >
> > /**
> > diff --git a/drivers/ufs/host/ufs-mediatek.h b/drivers/ufs/host/ufs-
> > mediatek.h
> > index 3ff17e95afab..05d76a6bd772 100644
> > --- a/drivers/ufs/host/ufs-mediatek.h
> > +++ b/drivers/ufs/host/ufs-mediatek.h
> > @@ -189,4 +189,7 @@ struct ufs_mtk_host {
> > /* MTK delay of autosuspend: 500 ms */
> > #define MTK_RPM_AUTOSUSPEND_DELAY_MS 500
> >
> > +/* MTK RTT support number */
> > +#define MTK_MAX_NUM_RTT 2
> > +
> > #endif /* !_UFS_MEDIATEK_H */
>
> The above patch would result in duplication of code. We should avoid
> code duplication if that's reasonably possible. Instead of applying the
> above patch, I propose to add a callback function pointer in struct
> ufs_hba_variant_ops that returns the maximum RTT value supported by the
> host driver.
Yes. That would simplify things.
Would replace that with the nortt capability.
Thanks,
Avri
>
> Thanks,
>
> Bart.
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2024-05-25 14:55 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-05-23 12:58 [PATCH v5 0/3] scsi: ufs: Allow RTT negotiation Avri Altman
2024-05-23 12:58 ` [PATCH v5 1/3] " Avri Altman
2024-05-23 12:58 ` [PATCH v5 2/3] scsi: ufs: Allow platform vendors to set rtt Avri Altman
2024-05-23 13:03 ` Christoph Hellwig
2024-05-23 13:09 ` Avri Altman
2024-05-23 13:11 ` hch
2024-05-24 6:06 ` Peter Wang (王信友)
2024-05-24 13:56 ` Bart Van Assche
2024-05-25 14:55 ` Avri Altman
2024-05-23 12:58 ` [PATCH v5 3/3] scsi: ufs: sysfs: Make max_number_of_rtt read-write Avri Altman
2024-05-23 23:05 ` Bart Van Assche
2024-05-24 3:54 ` Avri Altman
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).