2022-09-23 01:51:00

by Asutosh Das

[permalink] [raw]
Subject: [PATCH v1 04/16] ufs: core: mcq: Introduce Multi Circular Queue

Introduce multi-circular queue (MCQ) which has been added
in UFSHC v4.0 standard in addition to the Single Doorbell mode.
The MCQ mode supports multiple submission and completion queues.
Add support to configure the number of queues.

Co-developed-by: Can Guo <[email protected]>
Signed-off-by: Can Guo <[email protected]>
Signed-off-by: Asutosh Das <[email protected]>
---
drivers/ufs/core/Makefile | 2 +-
drivers/ufs/core/ufs-mcq.c | 132 +++++++++++++++++++++++++++++++++++++++++
drivers/ufs/core/ufshcd-priv.h | 1 +
drivers/ufs/core/ufshcd.c | 12 ++++
include/ufs/ufshcd.h | 4 ++
5 files changed, 150 insertions(+), 1 deletion(-)
create mode 100644 drivers/ufs/core/ufs-mcq.c

diff --git a/drivers/ufs/core/Makefile b/drivers/ufs/core/Makefile
index 62f38c5..4d02e0f 100644
--- a/drivers/ufs/core/Makefile
+++ b/drivers/ufs/core/Makefile
@@ -1,7 +1,7 @@
# SPDX-License-Identifier: GPL-2.0

obj-$(CONFIG_SCSI_UFSHCD) += ufshcd-core.o
-ufshcd-core-y += ufshcd.o ufs-sysfs.o
+ufshcd-core-y += ufshcd.o ufs-sysfs.o ufs-mcq.o
ufshcd-core-$(CONFIG_DEBUG_FS) += ufs-debugfs.o
ufshcd-core-$(CONFIG_SCSI_UFS_BSG) += ufs_bsg.o
ufshcd-core-$(CONFIG_SCSI_UFS_CRYPTO) += ufshcd-crypto.o
diff --git a/drivers/ufs/core/ufs-mcq.c b/drivers/ufs/core/ufs-mcq.c
new file mode 100644
index 0000000..934556f
--- /dev/null
+++ b/drivers/ufs/core/ufs-mcq.c
@@ -0,0 +1,132 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2022 Qualcomm Innovation Center. All rights reserved.
+ *
+ * Authors:
+ * Asutosh Das <[email protected]>
+ * Can Guo <[email protected]>
+ */
+
+#include <asm/unaligned.h>
+#include <linux/dma-mapping.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include "ufshcd-priv.h"
+
+#define UFS_MCQ_MIN_RW_QUEUES 2
+#define UFS_MCQ_MIN_READ_QUEUES 0
+#define UFS_MCQ_MIN_POLL_QUEUES 0
+
+static unsigned int dev_cmd_queue = 1;
+
+static int rw_queue_count_set(const char *val, const struct kernel_param *kp)
+{
+ unsigned int n;
+ int ret;
+
+ ret = kstrtouint(val, 10, &n);
+ if (ret)
+ return ret;
+ return param_set_uint_minmax(val, kp, UFS_MCQ_MIN_RW_QUEUES,
+ num_possible_cpus());
+}
+
+static const struct kernel_param_ops rw_queue_count_ops = {
+ .set = rw_queue_count_set,
+ .get = param_get_uint,
+};
+
+static unsigned int rw_queues;
+module_param_cb(rw_queues, &rw_queue_count_ops, &rw_queues, 0644);
+MODULE_PARM_DESC(rw_queues,
+ "Number of interrupt driven I/O queues used for rw. Default value is nr_cpus");
+
+static int read_queue_count_set(const char *val, const struct kernel_param *kp)
+{
+ unsigned int n;
+ int ret;
+
+ ret = kstrtouint(val, 10, &n);
+ if (ret)
+ return ret;
+ return param_set_uint_minmax(val, kp, UFS_MCQ_MIN_READ_QUEUES,
+ num_possible_cpus());
+}
+
+static const struct kernel_param_ops read_queue_count_ops = {
+ .set = read_queue_count_set,
+ .get = param_get_uint,
+};
+
+static unsigned int read_queues;
+module_param_cb(read_queues, &read_queue_count_ops, &read_queues, 0644);
+MODULE_PARM_DESC(read_queues,
+ "Number of interrupt driven read queues used for read. Default value is 0");
+
+static int poll_queue_count_set(const char *val, const struct kernel_param *kp)
+{
+ unsigned int n;
+ int ret;
+
+ ret = kstrtouint(val, 10, &n);
+ if (ret)
+ return ret;
+ return param_set_uint_minmax(val, kp, UFS_MCQ_MIN_POLL_QUEUES,
+ num_possible_cpus());
+}
+
+static const struct kernel_param_ops poll_queue_count_ops = {
+ .set = poll_queue_count_set,
+ .get = param_get_uint,
+};
+
+static unsigned int poll_queues = 1;
+module_param_cb(poll_queues, &poll_queue_count_ops, &poll_queues, 0644);
+MODULE_PARM_DESC(poll_queues,
+ "Number of poll queues used for r/w. Default value is 1");
+
+static int ufshcd_mcq_config_nr_queues(struct ufs_hba *hba)
+{
+ int i, rem;
+ u32 hbaq_cap, cmp, tot_queues;
+ struct Scsi_Host *host = hba->host;
+
+ hbaq_cap = hba->mcq_capabilities & 0xff;
+
+ if (!rw_queues)
+ rw_queues = num_possible_cpus();
+
+ tot_queues = dev_cmd_queue + read_queues + poll_queues + rw_queues;
+ if (hbaq_cap < tot_queues) {
+ dev_err(hba->dev, "Total queues (%d) exceeds HC capacity (%d)\n",
+ tot_queues, hbaq_cap);
+ return -EOPNOTSUPP;
+ }
+
+ rem = hbaq_cap - dev_cmd_queue;
+ cmp = rem;
+ hba->nr_queues[HCTX_TYPE_DEFAULT] = min3(cmp, rw_queues,
+ num_possible_cpus());
+ rem -= hba->nr_queues[HCTX_TYPE_DEFAULT];
+ cmp = rem;
+ hba->nr_queues[HCTX_TYPE_POLL] = min(cmp, poll_queues);
+ rem -= hba->nr_queues[HCTX_TYPE_POLL];
+ cmp = rem;
+ hba->nr_queues[HCTX_TYPE_READ] = min(cmp, read_queues);
+
+ for (i = 0; i < HCTX_MAX_TYPES; i++)
+ host->nr_hw_queues += hba->nr_queues[i];
+
+ hba->nr_hw_queues = host->nr_hw_queues + dev_cmd_queue;
+ return 0;
+}
+
+int ufshcd_mcq_init(struct ufs_hba *hba)
+{
+ int ret;
+
+ ret = ufshcd_mcq_config_nr_queues(hba);
+
+ return ret;
+}
+
diff --git a/drivers/ufs/core/ufshcd-priv.h b/drivers/ufs/core/ufshcd-priv.h
index 8f67db2..cf6bdd8e 100644
--- a/drivers/ufs/core/ufshcd-priv.h
+++ b/drivers/ufs/core/ufshcd-priv.h
@@ -50,6 +50,7 @@ int ufshcd_query_attr(struct ufs_hba *hba, enum query_opcode opcode,
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);

#define SD_ASCII_STD true
#define SD_RAW false
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 426867b..f4bb402 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -8172,6 +8172,15 @@ static int ufshcd_add_lus(struct ufs_hba *hba)
return ret;
}

+static int ufshcd_config_mcq(struct ufs_hba *hba)
+{
+ int ret;
+
+ ret = ufshcd_mcq_init(hba);
+
+ return ret;
+}
+
/**
* ufshcd_probe_hba - probe hba to detect device and initialize it
* @hba: per-adapter instance
@@ -8221,6 +8230,9 @@ static int ufshcd_probe_hba(struct ufs_hba *hba, bool init_dev_params)
goto out;

if (is_mcq_supported(hba)) {
+ ret = ufshcd_config_mcq(hba);
+ if (ret)
+ goto out;
ret = scsi_add_host(host, hba->dev);
if (ret) {
dev_err(hba->dev, "scsi_add_host failed\n");
diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
index da7ec0c..298e103 100644
--- a/include/ufs/ufshcd.h
+++ b/include/ufs/ufshcd.h
@@ -827,6 +827,8 @@ struct ufs_hba_monitor {
* ufshcd_resume_complete()
* @ext_iid_sup: is EXT_IID is supported by UFSHC
* @mcq_sup: is mcq supported by UFSHC
+ * @nr_hw_queues: number of hardware queues configured
+ * @nr_queues: number of Queues of different queue types
*/
struct ufs_hba {
void __iomem *mmio_base;
@@ -977,6 +979,8 @@ struct ufs_hba {
bool complete_put;
bool ext_iid_sup;
bool mcq_sup;
+ unsigned int nr_hw_queues;
+ unsigned int nr_queues[HCTX_MAX_TYPES];
};

static inline bool is_mcq_supported(struct ufs_hba *hba)
--
2.7.4


2022-09-26 16:41:54

by Manivannan Sadhasivam

[permalink] [raw]
Subject: Re: [PATCH v1 04/16] ufs: core: mcq: Introduce Multi Circular Queue

On Thu, Sep 22, 2022 at 06:05:11PM -0700, Asutosh Das wrote:
> Introduce multi-circular queue (MCQ) which has been added
> in UFSHC v4.0 standard in addition to the Single Doorbell mode.
> The MCQ mode supports multiple submission and completion queues.
> Add support to configure the number of queues.
>
> Co-developed-by: Can Guo <[email protected]>
> Signed-off-by: Can Guo <[email protected]>
> Signed-off-by: Asutosh Das <[email protected]>
> ---
> drivers/ufs/core/Makefile | 2 +-
> drivers/ufs/core/ufs-mcq.c | 132 +++++++++++++++++++++++++++++++++++++++++
> drivers/ufs/core/ufshcd-priv.h | 1 +
> drivers/ufs/core/ufshcd.c | 12 ++++
> include/ufs/ufshcd.h | 4 ++
> 5 files changed, 150 insertions(+), 1 deletion(-)
> create mode 100644 drivers/ufs/core/ufs-mcq.c
>
> diff --git a/drivers/ufs/core/Makefile b/drivers/ufs/core/Makefile
> index 62f38c5..4d02e0f 100644
> --- a/drivers/ufs/core/Makefile
> +++ b/drivers/ufs/core/Makefile
> @@ -1,7 +1,7 @@
> # SPDX-License-Identifier: GPL-2.0
>
> obj-$(CONFIG_SCSI_UFSHCD) += ufshcd-core.o
> -ufshcd-core-y += ufshcd.o ufs-sysfs.o
> +ufshcd-core-y += ufshcd.o ufs-sysfs.o ufs-mcq.o
> ufshcd-core-$(CONFIG_DEBUG_FS) += ufs-debugfs.o
> ufshcd-core-$(CONFIG_SCSI_UFS_BSG) += ufs_bsg.o
> ufshcd-core-$(CONFIG_SCSI_UFS_CRYPTO) += ufshcd-crypto.o
> diff --git a/drivers/ufs/core/ufs-mcq.c b/drivers/ufs/core/ufs-mcq.c
> new file mode 100644
> index 0000000..934556f
> --- /dev/null
> +++ b/drivers/ufs/core/ufs-mcq.c
> @@ -0,0 +1,132 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2022 Qualcomm Innovation Center. All rights reserved.
> + *
> + * Authors:
> + * Asutosh Das <[email protected]>
> + * Can Guo <[email protected]>
> + */
> +
> +#include <asm/unaligned.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include "ufshcd-priv.h"
> +
> +#define UFS_MCQ_MIN_RW_QUEUES 2
> +#define UFS_MCQ_MIN_READ_QUEUES 0
> +#define UFS_MCQ_MIN_POLL_QUEUES 0
> +
> +static unsigned int dev_cmd_queue = 1;

This looks like a constant, so consider switching to a macro.

> +
> +static int rw_queue_count_set(const char *val, const struct kernel_param *kp)
> +{
> + unsigned int n;
> + int ret;
> +
> + ret = kstrtouint(val, 10, &n);

n is not used? While fixing, please use a better name.

> + if (ret)
> + return ret;

Newline

> + return param_set_uint_minmax(val, kp, UFS_MCQ_MIN_RW_QUEUES,
> + num_possible_cpus());
> +}
> +
> +static const struct kernel_param_ops rw_queue_count_ops = {
> + .set = rw_queue_count_set,
> + .get = param_get_uint,
> +};
> +
> +static unsigned int rw_queues;
> +module_param_cb(rw_queues, &rw_queue_count_ops, &rw_queues, 0644);
> +MODULE_PARM_DESC(rw_queues,
> + "Number of interrupt driven I/O queues used for rw. Default value is nr_cpus");
> +
> +static int read_queue_count_set(const char *val, const struct kernel_param *kp)
> +{
> + unsigned int n;
> + int ret;
> +
> + ret = kstrtouint(val, 10, &n);
> + if (ret)
> + return ret;

Same as above

> + return param_set_uint_minmax(val, kp, UFS_MCQ_MIN_READ_QUEUES,
> + num_possible_cpus());
> +}
> +
> +static const struct kernel_param_ops read_queue_count_ops = {
> + .set = read_queue_count_set,
> + .get = param_get_uint,
> +};
> +
> +static unsigned int read_queues;
> +module_param_cb(read_queues, &read_queue_count_ops, &read_queues, 0644);
> +MODULE_PARM_DESC(read_queues,
> + "Number of interrupt driven read queues used for read. Default value is 0");
> +
> +static int poll_queue_count_set(const char *val, const struct kernel_param *kp)
> +{
> + unsigned int n;
> + int ret;
> +
> + ret = kstrtouint(val, 10, &n);
> + if (ret)
> + return ret;

Same as above

> + return param_set_uint_minmax(val, kp, UFS_MCQ_MIN_POLL_QUEUES,
> + num_possible_cpus());
> +}
> +
> +static const struct kernel_param_ops poll_queue_count_ops = {
> + .set = poll_queue_count_set,
> + .get = param_get_uint,
> +};
> +
> +static unsigned int poll_queues = 1;
> +module_param_cb(poll_queues, &poll_queue_count_ops, &poll_queues, 0644);
> +MODULE_PARM_DESC(poll_queues,
> + "Number of poll queues used for r/w. Default value is 1");
> +
> +static int ufshcd_mcq_config_nr_queues(struct ufs_hba *hba)
> +{
> + int i, rem;
> + u32 hbaq_cap, cmp, tot_queues;
> + struct Scsi_Host *host = hba->host;
> +
> + hbaq_cap = hba->mcq_capabilities & 0xff;

Define 0xff

hbaq_cap should be named after the define used for 0xff. It is not quite
understandable now.

> +
> + if (!rw_queues)
> + rw_queues = num_possible_cpus();
> +
> + tot_queues = dev_cmd_queue + read_queues + poll_queues + rw_queues;
> + if (hbaq_cap < tot_queues) {
> + dev_err(hba->dev, "Total queues (%d) exceeds HC capacity (%d)\n",
> + tot_queues, hbaq_cap);
> + return -EOPNOTSUPP;
> + }
> +
> + rem = hbaq_cap - dev_cmd_queue;
> + cmp = rem;
> + hba->nr_queues[HCTX_TYPE_DEFAULT] = min3(cmp, rw_queues,
> + num_possible_cpus());
> + rem -= hba->nr_queues[HCTX_TYPE_DEFAULT];
> + cmp = rem;
> + hba->nr_queues[HCTX_TYPE_POLL] = min(cmp, poll_queues);

Hmm, so the driver is not using the number of queues set by the user?
If the number varies, I don't think it should be configurable.

Thanks,
Mani

> + rem -= hba->nr_queues[HCTX_TYPE_POLL];
> + cmp = rem;
> + hba->nr_queues[HCTX_TYPE_READ] = min(cmp, read_queues);
> +
> + for (i = 0; i < HCTX_MAX_TYPES; i++)
> + host->nr_hw_queues += hba->nr_queues[i];
> +
> + hba->nr_hw_queues = host->nr_hw_queues + dev_cmd_queue;
> + return 0;
> +}
> +
> +int ufshcd_mcq_init(struct ufs_hba *hba)
> +{
> + int ret;
> +
> + ret = ufshcd_mcq_config_nr_queues(hba);
> +
> + return ret;
> +}
> +
> diff --git a/drivers/ufs/core/ufshcd-priv.h b/drivers/ufs/core/ufshcd-priv.h
> index 8f67db2..cf6bdd8e 100644
> --- a/drivers/ufs/core/ufshcd-priv.h
> +++ b/drivers/ufs/core/ufshcd-priv.h
> @@ -50,6 +50,7 @@ int ufshcd_query_attr(struct ufs_hba *hba, enum query_opcode opcode,
> 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);
>
> #define SD_ASCII_STD true
> #define SD_RAW false
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index 426867b..f4bb402 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -8172,6 +8172,15 @@ static int ufshcd_add_lus(struct ufs_hba *hba)
> return ret;
> }
>
> +static int ufshcd_config_mcq(struct ufs_hba *hba)
> +{
> + int ret;
> +
> + ret = ufshcd_mcq_init(hba);
> +
> + return ret;
> +}
> +
> /**
> * ufshcd_probe_hba - probe hba to detect device and initialize it
> * @hba: per-adapter instance
> @@ -8221,6 +8230,9 @@ static int ufshcd_probe_hba(struct ufs_hba *hba, bool init_dev_params)
> goto out;
>
> if (is_mcq_supported(hba)) {
> + ret = ufshcd_config_mcq(hba);
> + if (ret)
> + goto out;
> ret = scsi_add_host(host, hba->dev);
> if (ret) {
> dev_err(hba->dev, "scsi_add_host failed\n");
> diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
> index da7ec0c..298e103 100644
> --- a/include/ufs/ufshcd.h
> +++ b/include/ufs/ufshcd.h
> @@ -827,6 +827,8 @@ struct ufs_hba_monitor {
> * ufshcd_resume_complete()
> * @ext_iid_sup: is EXT_IID is supported by UFSHC
> * @mcq_sup: is mcq supported by UFSHC
> + * @nr_hw_queues: number of hardware queues configured
> + * @nr_queues: number of Queues of different queue types
> */
> struct ufs_hba {
> void __iomem *mmio_base;
> @@ -977,6 +979,8 @@ struct ufs_hba {
> bool complete_put;
> bool ext_iid_sup;
> bool mcq_sup;
> + unsigned int nr_hw_queues;
> + unsigned int nr_queues[HCTX_MAX_TYPES];
> };
>
> static inline bool is_mcq_supported(struct ufs_hba *hba)
> --
> 2.7.4
>

--
மணிவண்ணன் சதாசிவம்

2022-09-26 21:43:25

by Asutosh Das

[permalink] [raw]
Subject: Re: [PATCH v1 04/16] ufs: core: mcq: Introduce Multi Circular Queue

On Mon, Sep 26 2022 at 07:50 -0700, Manivannan Sadhasivam wrote:
>On Thu, Sep 22, 2022 at 06:05:11PM -0700, Asutosh Das wrote:

[...]

>> diff --git a/drivers/ufs/core/ufs-mcq.c b/drivers/ufs/core/ufs-mcq.c
>> new file mode 100644
>> index 0000000..934556f
>> --- /dev/null
>> +++ b/drivers/ufs/core/ufs-mcq.c
[...]
>> +static int ufshcd_mcq_config_nr_queues(struct ufs_hba *hba)
>> +{
>> + int i, rem;
>> + u32 hbaq_cap, cmp, tot_queues;
>> + struct Scsi_Host *host = hba->host;
>> +
>> + hbaq_cap = hba->mcq_capabilities & 0xff;
>
>Define 0xff
>
>hbaq_cap should be named after the define used for 0xff. It is not quite
>understandable now.
>
Ok. I think you want the name hbaq_cap to be changed to what 0xff signifies.
Is that understanding correct?

>> +
>> + if (!rw_queues)
>> + rw_queues = num_possible_cpus();
>> +
>> + tot_queues = dev_cmd_queue + read_queues + poll_queues + rw_queues;
>> + if (hbaq_cap < tot_queues) {
>> + dev_err(hba->dev, "Total queues (%d) exceeds HC capacity (%d)\n",
>> + tot_queues, hbaq_cap);
>> + return -EOPNOTSUPP;
>> + }
>> +
>> + rem = hbaq_cap - dev_cmd_queue;
>> + cmp = rem;
>> + hba->nr_queues[HCTX_TYPE_DEFAULT] = min3(cmp, rw_queues,
>> + num_possible_cpus());
>> + rem -= hba->nr_queues[HCTX_TYPE_DEFAULT];
>> + cmp = rem;
>> + hba->nr_queues[HCTX_TYPE_POLL] = min(cmp, poll_queues);
>
>Hmm, so the driver is not using the number of queues set by the user?
>If the number varies, I don't think it should be configurable.
>
I think the driver would use the number of queues set by the user.
Because there's a check(hbaq_cap < tot_queues) above that ensures
that the HBA has enough queues for the given configuration.

-asd

>

2022-09-30 18:54:20

by Bart Van Assche

[permalink] [raw]
Subject: Re: [PATCH v1 04/16] ufs: core: mcq: Introduce Multi Circular Queue

On 9/22/22 18:05, Asutosh Das wrote:
> +static int ufshcd_mcq_config_nr_queues(struct ufs_hba *hba)
> +{
> + int i, rem;
> + u32 hbaq_cap, cmp, tot_queues;
> + struct Scsi_Host *host = hba->host;
> +
> + hbaq_cap = hba->mcq_capabilities & 0xff;
> +
> + if (!rw_queues)
> + rw_queues = num_possible_cpus();
> +
> + tot_queues = dev_cmd_queue + read_queues + poll_queues + rw_queues;
> + if (hbaq_cap < tot_queues) {
> + dev_err(hba->dev, "Total queues (%d) exceeds HC capacity (%d)\n",
> + tot_queues, hbaq_cap);
> + return -EOPNOTSUPP;
> + }
> +
> + rem = hbaq_cap - dev_cmd_queue;
> + cmp = rem;
> + hba->nr_queues[HCTX_TYPE_DEFAULT] = min3(cmp, rw_queues,
> + num_possible_cpus());
> + rem -= hba->nr_queues[HCTX_TYPE_DEFAULT];
> + cmp = rem;
> + hba->nr_queues[HCTX_TYPE_POLL] = min(cmp, poll_queues);
> + rem -= hba->nr_queues[HCTX_TYPE_POLL];
> + cmp = rem;
> + hba->nr_queues[HCTX_TYPE_READ] = min(cmp, read_queues);
> +
> + for (i = 0; i < HCTX_MAX_TYPES; i++)
> + host->nr_hw_queues += hba->nr_queues[i];
> +
> + hba->nr_hw_queues = host->nr_hw_queues + dev_cmd_queue;
> + return 0;
> +}

Please consider removing the variable 'cmp' and instead use min_t() or a
type cast.

Thanks,

Bart.