| Message ID | e0ae0fccc579fa17b6cfcf3b8bcf963f5ebe67fa.1663894792.git.quic_asutoshd@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add Multi Circular Queue Support | expand |
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 <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/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 <quic_asutoshd@quicinc.com> > + * Can Guo <quic_cang@quicinc.com> > + */ > + > +#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 >
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 >
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.
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 <quic_asutoshd@quicinc.com> + * Can Guo <quic_cang@quicinc.com> + */ + +#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)