| Message ID | 20240514050823.735-1-avri.altman@wdc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3] scsi: ufs: Allow RTT negotiation | expand |
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.
> 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.
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.
> 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.
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.
> 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.
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.
> 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.
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,