diff mbox series

[v4,1/3] scsi: ufs: Allow RTT negotiation

Message ID 20240516055124.24490-2-avri.altman@wdc.com (mailing list archive)
State Superseded
Headers show
Series scsi: ufs: Allow RTT negotiation | expand

Commit Message

Avri Altman May 16, 2024, 5:51 a.m. UTC 
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 | 36 ++++++++++++++++++++++++++++++++++++
 include/ufs/ufshcd.h      |  2 ++
 include/ufs/ufshci.h      |  1 +
 3 files changed, 39 insertions(+)

Comments

Bart Van Assche May 16, 2024, 2:28 p.m. UTC | #1 
On 5/15/24 23:51, Avri Altman wrote:
>   void ufshcd_fixup_dev_quirks(struct ufs_hba *hba,
>   			     const struct ufs_dev_quirk *fixups)
>   {
> @@ -8278,6 +8312,8 @@ static int ufs_get_device_desc(struct ufs_hba *hba)
>   	if (hba->ext_iid_sup)
>   		ufshcd_ext_iid_probe(hba, desc_buf);
>   
> +	ufshcd_rtt_set(hba, desc_buf);
> +

Why does this call occur in ufs_get_device_desc()? ufshcd_rtt_set() sets
a device parameter. Shouldn't this call be moved one level up into
ufshcd_device_params_init()?

Otherwise this patch looks good to me.

Thanks,

Bart.
Avri Altman May 16, 2024, 3:21 p.m. UTC | #2 
> On 5/15/24 23:51, Avri Altman wrote:
> >   void ufshcd_fixup_dev_quirks(struct ufs_hba *hba,
> >                            const struct ufs_dev_quirk *fixups)
> >   {
> > @@ -8278,6 +8312,8 @@ static int ufs_get_device_desc(struct ufs_hba *hba)
> >       if (hba->ext_iid_sup)
> >               ufshcd_ext_iid_probe(hba, desc_buf);
> >
> > +     ufshcd_rtt_set(hba, desc_buf);
> > +
> 
> Why does this call occur in ufs_get_device_desc()? ufshcd_rtt_set() sets
> a device parameter. Shouldn't this call be moved one level up into
> ufshcd_device_params_init()?
Because otherwise bDeviceRTTCap is not available.
Please note several device configuration calls in  ufs_get_device_desc - all requires some device descriptor fields.

Thanks,
Avri
> 
> Otherwise this patch looks good to me.
> 
> Thanks,
> 
> Bart.
Bart Van Assche May 16, 2024, 4:04 p.m. UTC | #3 
On 5/16/24 09:21, Avri Altman wrote:
>> On 5/15/24 23:51, Avri Altman wrote:
>>>    void ufshcd_fixup_dev_quirks(struct ufs_hba *hba,
>>>                             const struct ufs_dev_quirk *fixups)
>>>    {
>>> @@ -8278,6 +8312,8 @@ static int ufs_get_device_desc(struct ufs_hba *hba)
>>>        if (hba->ext_iid_sup)
>>>                ufshcd_ext_iid_probe(hba, desc_buf);
>>>
>>> +     ufshcd_rtt_set(hba, desc_buf);
>>> +
>>
>> Why does this call occur in ufs_get_device_desc()? ufshcd_rtt_set() sets
>> a device parameter. Shouldn't this call be moved one level up into
>> ufshcd_device_params_init()?
> Because otherwise bDeviceRTTCap is not available.
> Please note several device configuration calls in  ufs_get_device_desc - all requires some device descriptor fields.

Please make sure that the functionality of ufs_get_device_desc() and the 
name of that function remain in sync.

I think that bDeviceRTTCap is available in ufshcd_device_params_init()
after ufs_get_device_desc() has returned?

Thanks,

Bart.
Avri Altman May 16, 2024, 11:56 p.m. UTC | #4 
> On 5/16/24 09:21, Avri Altman wrote:
> >> On 5/15/24 23:51, Avri Altman wrote:
> >>>    void ufshcd_fixup_dev_quirks(struct ufs_hba *hba,
> >>>                             const struct ufs_dev_quirk *fixups)
> >>>    {
> >>> @@ -8278,6 +8312,8 @@ static int ufs_get_device_desc(struct ufs_hba
> *hba)
> >>>        if (hba->ext_iid_sup)
> >>>                ufshcd_ext_iid_probe(hba, desc_buf);
> >>>
> >>> +     ufshcd_rtt_set(hba, desc_buf);
> >>> +
> >>
> >> Why does this call occur in ufs_get_device_desc()? ufshcd_rtt_set()
> >> sets a device parameter. Shouldn't this call be moved one level up
> >> into ufshcd_device_params_init()?
> > Because otherwise bDeviceRTTCap is not available.
> > Please note several device configuration calls in  ufs_get_device_desc - all
> requires some device descriptor fields.
> 
> Please make sure that the functionality of ufs_get_device_desc() and the name of
> that function remain in sync.
> 
> I think that bDeviceRTTCap is available in ufshcd_device_params_init() after
> ufs_get_device_desc() has returned?
Will make the required change.

Thanks,
Avri

> 
> Thanks,
> 
> Bart.
diff mbox series

Patch

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 0819ddafe7a6..c472bfdf071e 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,8 @@  static int ufs_get_device_desc(struct ufs_hba *hba)
 	if (hba->ext_iid_sup)
 		ufshcd_ext_iid_probe(hba, desc_buf);
 
+	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..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,