[PATCH v3] scsi: ufs: Allow RTT negotiation

[PATCH v3] scsi: ufs: Allow RTT negotiation

[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.

While at it, allow platform vendors to take precedence having their own
rtt negotiation mechanism.

Signed-off-by: Avri Altman <avri.altman@wdc.com>
Reviewed-by: Bean Huo <beanhuo@micron.com>

---
Changes since v2:
 - Allow platform vendors to take precedence having their own rtt
   negotiation mechanism (Peter)

Changes since v1:
- bMaxNumOfRTT is a Persistent attribute - do not override if it was
  written (Bean)

---
 drivers/ufs/core/ufshcd.c | 39 +++++++++++++++++++++++++++++++++++++++
 include/ufs/ufshcd.h      |  4 ++++
 include/ufs/ufshci.h      |  1 +
 3 files changed, 44 insertions(+)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 0819ddafe7a6..0407d1064e74 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_rtt_set(struct ufs_hba *hba, u8 *desc_buf)
+{
+	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, desc_buf[DEVICE_DESC_PARAM_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)
 {
@@ -8278,6 +8312,11 @@ static int ufs_get_device_desc(struct ufs_hba *hba)
 	if (hba->ext_iid_sup)
 		ufshcd_ext_iid_probe(hba, desc_buf);
 
+	if (hba->vops && hba->vops->rtt_set)
+		hba->vops->rtt_set(hba, desc_buf);
+	else
+		ufshcd_rtt_set(hba, desc_buf);
+
 	/*
 	 * ufshcd_read_string_desc returns size of the string
 	 * reset the error value
diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
index bad88bd91995..9237ea65bd26 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
+ * @rtt_set: 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	(*rtt_set)(struct ufs_hba *hba, u8 *desc_buf);
 };
 
 /* clock gating state  */
@@ -819,6 +821,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 +960,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

Re: [PATCH v3] scsi: ufs: Allow RTT negotiation

[Bart Van Assche]

On 5/13/24 23:08, Avri Altman wrote:
> +/* bMaxNumOfRTT is equal to two after device manufacturing */
> +#define DEFAULT_MAX_NUM_RTT 2
> [ ... ]
> +	/* do not override if it was already written */
> +	if (dev_rtt != DEFAULT_MAX_NUM_RTT)
> +		return;

I haven't found any text in the UFSHCI 4.0 specification that says
that the default value for the number of outstanding RTT requests
should be 2. Did I perhaps overlook something? If I didn't overlook
anything, the driver should not try to check whether dev_rtt is at its
default value.

Thanks,

Bart.

RE: [PATCH v3] scsi: ufs: Allow RTT negotiation

[Avri Altman]

> On 5/13/24 23:08, Avri Altman wrote:
> > +/* bMaxNumOfRTT is equal to two after device manufacturing */
> > +#define DEFAULT_MAX_NUM_RTT 2
> > [ ... ]
> > +     /* do not override if it was already written */
> > +     if (dev_rtt != DEFAULT_MAX_NUM_RTT)
> > +             return;
> 
> I haven't found any text in the UFSHCI 4.0 specification that says
> that the default value for the number of outstanding RTT requests
> should be 2. Did I perhaps overlook something? If I didn't overlook
> anything, the driver should not try to check whether dev_rtt is at its
> default value.
JEDEC Standard No. 220F Page 150 Line 2837 says: "bMaxNumOfRTT is equal to two after device manufacturing,"

Thanks,
Avri

> 
> Thanks,
> 
> Bart.

Re: [PATCH v3] scsi: ufs: Allow RTT negotiation

[Bart Van Assche]

On 5/14/24 14:34, Avri Altman wrote:
>> On 5/13/24 23:08, Avri Altman wrote:
>>> +/* bMaxNumOfRTT is equal to two after device manufacturing */
>>> +#define DEFAULT_MAX_NUM_RTT 2
>>> [ ... ]
>>> +     /* do not override if it was already written */
>>> +     if (dev_rtt != DEFAULT_MAX_NUM_RTT)
>>> +             return;
>>
>> I haven't found any text in the UFSHCI 4.0 specification that says
>> that the default value for the number of outstanding RTT requests
>> should be 2. Did I perhaps overlook something? If I didn't overlook
>> anything, the driver should not try to check whether dev_rtt is at its
>> default value.
> JEDEC Standard No. 220F Page 150 Line 2837 says: "bMaxNumOfRTT is equal to two after device manufacturing,"

Thanks Avri for having looked this up.

My understanding is that the above check won't work as intended if
ufshcd_rtt_set() does not modify the RTT value. Wouldn't it be better
to add a boolean in struct ufs_hba that indicates whether or not
ufshcd_rtt_set() has been called before?

Thanks,

Bart.

RE: [PATCH v3] scsi: ufs: Allow RTT negotiation

[Avri Altman]

> On 5/14/24 14:34, Avri Altman wrote:
> >> On 5/13/24 23:08, Avri Altman wrote:
> >>> +/* bMaxNumOfRTT is equal to two after device manufacturing */
> >>> +#define DEFAULT_MAX_NUM_RTT 2
> >>> [ ... ]
> >>> +     /* do not override if it was already written */
> >>> +     if (dev_rtt != DEFAULT_MAX_NUM_RTT)
> >>> +             return;
> >>
> >> I haven't found any text in the UFSHCI 4.0 specification that says
> >> that the default value for the number of outstanding RTT requests
> >> should be 2. Did I perhaps overlook something? If I didn't overlook
> >> anything, the driver should not try to check whether dev_rtt is at its
> >> default value.
> > JEDEC Standard No. 220F Page 150 Line 2837 says: "bMaxNumOfRTT is equal to
> two after device manufacturing,"
> 
> Thanks Avri for having looked this up.
> 
> My understanding is that the above check won't work as intended if
> ufshcd_rtt_set() does not modify the RTT value. Wouldn't it be better
> to add a boolean in struct ufs_hba that indicates whether or not
> ufshcd_rtt_set() has been called before?
My intension was to not override RTT should it was written, e.g. from user space.
As this attribute is persistent.
See Bean's comment to v1.

Thanks,
Avri

> 
> Thanks,
> 
> Bart.

Re: [PATCH v3] scsi: ufs: Allow RTT negotiation

[Bart Van Assche]

On 5/14/24 15:07, Avri Altman wrote:
> Bart Van Assche wrote:
>> My understanding is that the above check won't work as intended if
>> ufshcd_rtt_set() does not modify the RTT value. Wouldn't it be better
>> to add a boolean in struct ufs_hba that indicates whether or not
>> ufshcd_rtt_set() has been called before?
 >
> My intension was to not override RTT should it was written, e.g. from user space.
> As this attribute is persistent.

How can RTT be written from user space? There is no sysfs attribute for
configuring the RTT value. If the above refers to a mechanism that
bypasses the UFSHCI kernel driver: I don't think that we should preserve
any configuration changes applied this way. As an example, the SCSI core
does not care about configuration changes applied via the SG interface.

Additionally, what does "persistent" mean in this context?

Thanks,

Bart.

RE: [PATCH v3] scsi: ufs: Allow RTT negotiation

[Avri Altman]

> On 5/14/24 15:07, Avri Altman wrote:
> > Bart Van Assche wrote:
> >> My understanding is that the above check won't work as intended if
> >> ufshcd_rtt_set() does not modify the RTT value. Wouldn't it be better
> >> to add a boolean in struct ufs_hba that indicates whether or not
> >> ufshcd_rtt_set() has been called before?
>  >
> > My intension was to not override RTT should it was written, e.g. from user
> space.
> > As this attribute is persistent.
> 
> How can RTT be written from user space? There is no sysfs attribute for
> configuring the RTT value. If the above refers to a mechanism that bypasses the
> UFSHCI kernel driver: I don't think that we should preserve any configuration
> changes applied this way. As an example, the SCSI core does not care about
> configuration changes applied via the SG interface.
Via ufs-utils - https://github.com/westerndigitalcorporation/ufs-utils
You may remember - this is why we needed the ufs-bsg interface we added few years ago.
Ufs-utils is the industry standard with respect of configuring and provisioning ufs devices,
And currently is being used by the vast majority of ufs stake-holders:
device manufacturers, platform manufacturers, mobile vendors, etc.

> 
> Additionally, what does "persistent" mean in this context?
See Table 14.27 JEDEC Standard No. 220F page 443 — Attributes access properties:
Persistent (Write) - The attribute can be written multiple times, the value is kept after power cycle 
or any type of reset event.

Thanks,
Avri

> 
> Thanks,
> 
> Bart.

Re: [PATCH v3] scsi: ufs: Allow RTT negotiation

[Bart Van Assche]

On 5/14/24 22:56, Avri Altman wrote:
> Via ufs-utils - https://github.com/westerndigitalcorporation/ufs-utils
> You may remember - this is why we needed the ufs-bsg interface we added few years ago.
> Ufs-utils is the industry standard with respect of configuring and provisioning ufs devices,
> And currently is being used by the vast majority of ufs stake-holders:
> device manufacturers, platform manufacturers, mobile vendors, etc.

Given the importance of the RTT parameter, please provide a sysfs 
attribute for reading and configuring this parameter. UFS users should
not be encouraged to change UFS device parameters without the UFSHCI
driver being aware of these changes.

Thanks,

Bart.

RE: [PATCH v3] scsi: ufs: Allow RTT negotiation

[Avri Altman]

> On 5/14/24 22:56, Avri Altman wrote:
> > Via ufs-utils - https://github.com/westerndigitalcorporation/ufs-utils
> > You may remember - this is why we needed the ufs-bsg interface we added few
> years ago.
> > Ufs-utils is the industry standard with respect of configuring and provisioning ufs
> devices,
> > And currently is being used by the vast majority of ufs stake-holders:
> > device manufacturers, platform manufacturers, mobile vendors, etc.
> 
> Given the importance of the RTT parameter, please provide a sysfs
> attribute for reading and configuring this parameter. UFS users should
> not be encouraged to change UFS device parameters without the UFSHCI
> driver being aware of these changes.
OK.

Btw, max_number_of_rtt is available for reading, like any other attribute.
Will allow to write it as well.

Thanks,
Avri 

> 
> Thanks,
> 
> Bart.