[v3,07/17] ufs: core: mcq: Calculate queue depth
Commit Message
The ufs device defines the supported queuedepth by
bqueuedepth which has a max value of 256.
The HC defines MAC (Max Active Commands) that define
the max number of commands that in flight to the ufs
device.
Calculate and configure the nutrs based on both these
values.
Co-developed-by: Can Guo <quic_cang@quicinc.com>
Signed-off-by: Can Guo <quic_cang@quicinc.com>
Signed-off-by: Asutosh Das <quic_asutoshd@quicinc.com>
---
drivers/ufs/core/ufs-mcq.c | 34 ++++++++++++++++++++++++++++++++++
drivers/ufs/core/ufshcd-priv.h | 9 +++++++++
drivers/ufs/core/ufshcd.c | 9 ++++++++-
drivers/ufs/host/ufs-qcom.c | 8 ++++++++
include/ufs/ufs.h | 2 ++
include/ufs/ufshcd.h | 2 ++
include/ufs/ufshci.h | 1 +
7 files changed, 64 insertions(+), 1 deletion(-)
Comments
On 10/20/22 11:03, Asutosh Das wrote:
> +u32 ufshcd_mcq_decide_queue_depth(struct ufs_hba *hba)
> +{
> + u32 qd, val;
> + int mac;
> +
> + mac = ufshcd_mcq_vops_get_hba_mac(hba);
> + if (mac < 0) {
> + val = ufshcd_readl(hba, REG_UFS_MCQ_CFG);
> + mac = FIELD_GET(MCQ_CFG_MAC_MASK, val);
> + }
According to the UFSHCI 4.0 specification the MAC value is set by the
host. Can the above code read the MAC value from the host controller
before it has been set by the host? If so, how about leaving out the
code that reads the MAC value from the controller and making it
mandatory to implement the new get_hba_mac vop?
> +
> + /* MAC is a 0 based value. */
> + mac += 1;
> + /* max. value of bqueuedepth = 256, mac is host dependent */
host dependent -> defined by the host controller?
> + qd = min_t(u32, mac, hba->dev_info.bqueuedepth);
> + if (!qd)
> + qd = mac;
How about using min_not_zero() instead of open-coding it?
Thanks,
Bart.
> On 10/20/22 11:03, Asutosh Das wrote:
> > +u32 ufshcd_mcq_decide_queue_depth(struct ufs_hba *hba)
> > +{
> > + u32 qd, val;
> > + int mac;
> > +
> > + mac = ufshcd_mcq_vops_get_hba_mac(hba);
> > + if (mac < 0) {
> > + val = ufshcd_readl(hba, REG_UFS_MCQ_CFG);
> > + mac = FIELD_GET(MCQ_CFG_MAC_MASK, val);
> > + }
>
> According to the UFSHCI 4.0 specification the MAC value is set by the
> host. Can the above code read the MAC value from the host controller
> before it has been set by the host? If so, how about leaving out the
> code that reads the MAC value from the controller and making it
> mandatory to implement the new get_hba_mac vop?
>
> > +
> > + /* MAC is a 0 based value. */
> > + mac += 1;
> > + /* max. value of bqueuedepth = 256, mac is host dependent */
>
> host dependent -> defined by the host controller?
>
> > + qd = min_t(u32, mac, hba->dev_info.bqueuedepth);
> > + if (!qd)
> > + qd = mac;
Isn't if mcq is supported bqueuedepth can't be zero?
The device must implements the shared queueing architecture.
Thanks,
Avri
>
> How about using min_not_zero() instead of open-coding it?
>
> Thanks,
>
> Bart.
On Thu, Oct 27 2022 at 14:52 -0700, Bart Van Assche wrote:
>On 10/20/22 11:03, Asutosh Das wrote:
>>+u32 ufshcd_mcq_decide_queue_depth(struct ufs_hba *hba)
>>+{
>>+ u32 qd, val;
>>+ int mac;
>>+
>>+ mac = ufshcd_mcq_vops_get_hba_mac(hba);
>>+ if (mac < 0) {
>>+ val = ufshcd_readl(hba, REG_UFS_MCQ_CFG);
>>+ mac = FIELD_GET(MCQ_CFG_MAC_MASK, val);
>>+ }
>
>According to the UFSHCI 4.0 specification the MAC value is set by the
>host. Can the above code read the MAC value from the host controller
>before it has been set by the host? If so, how about leaving out the
>code that reads the MAC value from the controller and making it
>mandatory to implement the new get_hba_mac vop?
>
The reason it is not mandatory to define get_hba_mac vop is UFSHCI 4.0
specification mentions that the default value of MAC is 32. So even if a vendor
HC doesn't override the MAC, it'd be 32.
Hence, the current code first checks for an override, and if there's none uses
the default value defined in the HC.
-asd
On Sun, Oct 30 2022 at 04:43 -0700, Avri Altman wrote:
>> On 10/20/22 11:03, Asutosh Das wrote:
>> > +u32 ufshcd_mcq_decide_queue_depth(struct ufs_hba *hba)
[...]
>> > + qd = min_t(u32, mac, hba->dev_info.bqueuedepth);
>> > + if (!qd)
>> > + qd = mac;
>Isn't if mcq is supported bqueuedepth can't be zero?
>The device must implements the shared queueing architecture.
>
Ah well, there's a Note mentioned in the UFS 4.0 spec (#7021) which kind of
confused me. I will remove this check if it can't be 0.
-asd
>
On 10/31/22 12:24, Asutosh Das wrote:
> On Thu, Oct 27 2022 at 14:52 -0700, Bart Van Assche wrote:
>> On 10/20/22 11:03, Asutosh Das wrote:
>>> +u32 ufshcd_mcq_decide_queue_depth(struct ufs_hba *hba)
>>> +{
>>> + u32 qd, val;
>>> + int mac;
>>> +
>>> + mac = ufshcd_mcq_vops_get_hba_mac(hba);
>>> + if (mac < 0) {
>>> + val = ufshcd_readl(hba, REG_UFS_MCQ_CFG);
>>> + mac = FIELD_GET(MCQ_CFG_MAC_MASK, val);
>>> + }
>>
>> According to the UFSHCI 4.0 specification the MAC value is set by the
>> host. Can the above code read the MAC value from the host controller
>> before it has been set by the host? If so, how about leaving out the
>> code that reads the MAC value from the controller and making it
>> mandatory to implement the new get_hba_mac vop?
>>
> The reason it is not mandatory to define get_hba_mac vop is UFSHCI 4.0
> specification mentions that the default value of MAC is 32. So even if a
> vendor
> HC doesn't override the MAC, it'd be 32.
> Hence, the current code first checks for an override, and if there's
> none uses
> the default value defined in the HC.
Hi Asutosh,
Please ignore the value reported by the controller in the MAC field of
the MCQConfig register and overwrite the MAC field without reading it
first. It doesn't seem useful to me to read this field. I think the host
should decide about the queue depth no matter what the current value of
the MAC field is.
Thanks,
Bart.
@@ -18,6 +18,8 @@
#define UFS_MCQ_NUM_DEV_CMD_QUEUES 1
#define UFS_MCQ_MIN_POLL_QUEUES 0
+#define MAX_DEV_CMD_ENTRIES 2
+#define MCQ_CFG_MAC_MASK GENMASK(16, 8)
#define MCQ_QCFGPTR_MASK GENMASK(7, 0)
#define MCQ_QCFGPTR_UNIT 0x200
#define MCQ_SQATTR_OFFSET(c) \
@@ -88,6 +90,38 @@ static const struct ufshcd_res_info ufs_res_info[RES_MAX] = {
{.name = "mcq_vs",},
};
+/**
+ * ufshcd_mcq_decide_queue_depth - decide the queue depth
+ * @hba - per adapter instance
+ *
+ * MAC - Max. Active Command of the Host Controller (HC)
+ * HC wouldn't send more than this commands to the device.
+ * The default MAC is 32, but the max. value may vary with
+ * vendor implementation.
+ * Calculates and adjusts the queue depth based on the depth
+ * supported by the HC and ufs device.
+ */
+u32 ufshcd_mcq_decide_queue_depth(struct ufs_hba *hba)
+{
+ u32 qd, val;
+ int mac;
+
+ mac = ufshcd_mcq_vops_get_hba_mac(hba);
+ if (mac < 0) {
+ val = ufshcd_readl(hba, REG_UFS_MCQ_CFG);
+ mac = FIELD_GET(MCQ_CFG_MAC_MASK, val);
+ }
+
+ /* MAC is a 0 based value. */
+ mac += 1;
+ /* max. value of bqueuedepth = 256, mac is host dependent */
+ qd = min_t(u32, mac, hba->dev_info.bqueuedepth);
+ if (!qd)
+ qd = mac;
+
+ return qd;
+}
+
static int ufshcd_mcq_config_resource(struct ufs_hba *hba)
{
struct platform_device *pdev = to_platform_device(hba->dev);
@@ -51,6 +51,7 @@ int ufshcd_query_flag(struct ufs_hba *hba, enum query_opcode opcode,
enum flag_idn idn, u8 index, bool *flag_res);
void ufshcd_auto_hibern8_update(struct ufs_hba *hba, u32 ahit);
int ufshcd_mcq_init(struct ufs_hba *hba);
+u32 ufshcd_mcq_decide_queue_depth(struct ufs_hba *hba);
#define SD_ASCII_STD true
#define SD_RAW false
@@ -216,6 +217,14 @@ static inline void ufshcd_vops_config_scaling_param(struct ufs_hba *hba,
hba->vops->config_scaling_param(hba, p, data);
}
+static inline int ufshcd_mcq_vops_get_hba_mac(struct ufs_hba *hba)
+{
+ if (hba->vops && hba->vops->get_hba_mac)
+ return hba->vops->get_hba_mac(hba);
+
+ return -EOPNOTSUPP;
+}
+
extern const struct ufs_pm_lvl_states ufs_pm_lvl_states[];
/**
@@ -7762,6 +7762,7 @@ static int ufs_get_device_desc(struct ufs_hba *hba)
/* getting Specification Version in big endian format */
dev_info->wspecversion = desc_buf[DEVICE_DESC_PARAM_SPEC_VER] << 8 |
desc_buf[DEVICE_DESC_PARAM_SPEC_VER + 1];
+ dev_info->bqueuedepth = desc_buf[DEVICE_DESC_PARAM_Q_DPTH];
b_ufs_feature_sup = desc_buf[DEVICE_DESC_PARAM_UFS_FEAT];
model_index = desc_buf[DEVICE_DESC_PARAM_PRDCT_NAME];
@@ -8178,10 +8179,16 @@ static int ufshcd_add_lus(struct ufs_hba *hba)
static int ufshcd_alloc_mcq(struct ufs_hba *hba)
{
int ret;
+ int old_nutrs = hba->nutrs;
+ hba->nutrs = ufshcd_mcq_decide_queue_depth(hba);
ret = ufshcd_mcq_init(hba);
+ if (ret) {
+ hba->nutrs = old_nutrs;
+ return ret;
+ }
- return ret;
+ return 0;
}
/**
@@ -25,6 +25,7 @@
#define UFS_QCOM_DEFAULT_DBG_PRINT_EN \
(UFS_QCOM_DBG_PRINT_REGS_EN | UFS_QCOM_DBG_PRINT_TEST_BUS_EN)
+#define MAX_SUPP_MAC 63
enum {
TSTBUS_UAWM,
TSTBUS_UARM,
@@ -1424,6 +1425,12 @@ static void ufs_qcom_config_scaling_param(struct ufs_hba *hba,
}
#endif
+static int ufs_qcom_get_hba_mac(struct ufs_hba *hba)
+{
+ /* Default is 32, but Qualcomm HC supports upto 64 */
+ return MAX_SUPP_MAC;
+}
+
/*
* struct ufs_hba_qcom_vops - UFS QCOM specific variant operations
*
@@ -1447,6 +1454,7 @@ static const struct ufs_hba_variant_ops ufs_hba_qcom_vops = {
.device_reset = ufs_qcom_device_reset,
.config_scaling_param = ufs_qcom_config_scaling_param,
.program_key = ufs_qcom_ice_program_key,
+ .get_hba_mac = ufs_qcom_get_hba_mac,
};
/**
@@ -591,6 +591,8 @@ struct ufs_dev_info {
u8 *model;
u16 wspecversion;
u32 clk_gating_wait_us;
+ /* Stores the depth of queue in UFS device */
+ u8 bqueuedepth;
/* UFS HPB related flag */
bool hpb_enabled;
@@ -293,6 +293,7 @@ struct ufs_pwr_mode_info {
* @config_scaling_param: called to configure clock scaling parameters
* @program_key: program or evict an inline encryption key
* @event_notify: called to notify important events
+ * @get_hba_mac: called to get vendor specific mac value
*/
struct ufs_hba_variant_ops {
const char *name;
@@ -331,6 +332,7 @@ struct ufs_hba_variant_ops {
const union ufs_crypto_cfg_entry *cfg, int slot);
void (*event_notify)(struct ufs_hba *hba,
enum ufs_event_type evt, void *data);
+ int (*get_hba_mac)(struct ufs_hba *hba);
};
/* clock gating state */
@@ -57,6 +57,7 @@ enum {
REG_UFS_CCAP = 0x100,
REG_UFS_CRYPTOCAP = 0x104,
+ REG_UFS_MCQ_CFG = 0x380,
UFSHCI_CRYPTO_REG_SPACE_SIZE = 0x400,
};