| Message ID | ae139f730dcec6462f9218856bf974eda4a29e1d.1669176158.git.quic_asutoshd@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add Multi Circular Queue Support | expand |
On 11/22/22 20:10, Asutosh Das wrote: > +int ufshcd_mcq_decide_queue_depth(struct ufs_hba *hba) > +{ > + int mac; > + > + /* Mandatory to implement get_hba_mac() */ > + mac = ufshcd_mcq_vops_get_hba_mac(hba); > + if (mac < 0) { > + dev_err(hba->dev, "Failed to get mac, err=%d\n", mac); > + return mac; > + } > + > + WARN_ON(!hba->dev_info.bqueuedepth); > + /* > + * max. value of bqueuedepth = 256, mac is host dependent. > + * It is mandatory for UFS device to define bQueueDepth if > + * shared queuing architecture is enabled. > + */ > + return min_t(int, mac, hba->dev_info.bqueuedepth); > +} Isn't WARN_ON_ONCE() recommended over WARN_ON()? Otherwise this patch looks good to me. Thanks, Bart.
On Tue, Nov 22, 2022 at 08:10:20PM -0800, Asutosh Das wrote: > 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 | 32 ++++++++++++++++++++++++++++++++ > drivers/ufs/core/ufshcd-priv.h | 9 +++++++++ > drivers/ufs/core/ufshcd.c | 17 ++++++++++++++++- > 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, 70 insertions(+), 1 deletion(-) > > diff --git a/drivers/ufs/core/ufs-mcq.c b/drivers/ufs/core/ufs-mcq.c > index 4aaa6aa..e95f748 100644 > --- a/drivers/ufs/core/ufs-mcq.c > +++ b/drivers/ufs/core/ufs-mcq.c > @@ -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,36 @@ 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 > + * Kernel doc should define the return value also. > + * MAC - Max. Active Command of the Host Controller (HC) > + * HC wouldn't send more than this commands to the device. > + * It is mandatory to implement get_hba_mac() to enable MCQ mode. > + * Calculates and adjusts the queue depth based on the depth > + * supported by the HC and ufs device. > + */ > +int ufshcd_mcq_decide_queue_depth(struct ufs_hba *hba) > +{ > + int mac; > + > + /* Mandatory to implement get_hba_mac() */ > + mac = ufshcd_mcq_vops_get_hba_mac(hba); > + if (mac < 0) { > + dev_err(hba->dev, "Failed to get mac, err=%d\n", mac); > + return mac; > + } > + > + WARN_ON(!hba->dev_info.bqueuedepth); Instead of panic, you could just print and return an error. > + /* > + * max. value of bqueuedepth = 256, mac is host dependent. > + * It is mandatory for UFS device to define bQueueDepth if > + * shared queuing architecture is enabled. > + */ > + return min_t(int, mac, hba->dev_info.bqueuedepth); > +} > + > static int ufshcd_mcq_config_resource(struct ufs_hba *hba) > { > struct platform_device *pdev = to_platform_device(hba->dev); > diff --git a/drivers/ufs/core/ufshcd-priv.h b/drivers/ufs/core/ufshcd-priv.h > index 9368ba2..9f40fa5 100644 > --- a/drivers/ufs/core/ufshcd-priv.h > +++ b/drivers/ufs/core/ufshcd-priv.h > @@ -62,6 +62,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); > +int ufshcd_mcq_decide_queue_depth(struct ufs_hba *hba); > > #define SD_ASCII_STD true > #define SD_RAW false > @@ -227,6 +228,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) Again, no inline please. > +{ > + 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[]; > > /** > diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c > index 0c4cd8f..ae065da 100644 > --- a/drivers/ufs/core/ufshcd.c > +++ b/drivers/ufs/core/ufshcd.c > @@ -7783,6 +7783,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]; > @@ -8198,7 +8199,21 @@ static int ufshcd_add_lus(struct ufs_hba *hba) > > static int ufshcd_alloc_mcq(struct ufs_hba *hba) > { > - return ufshcd_mcq_init(hba); > + int ret; > + int old_nutrs = hba->nutrs; > + > + ret = ufshcd_mcq_decide_queue_depth(hba); > + if (ret < 0) > + return ret; > + > + hba->nutrs = ret; > + ret = ufshcd_mcq_init(hba); > + if (ret) { > + hba->nutrs = old_nutrs; > + return ret; > + } > + > + return 0; > } > > /** > diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c > index 8ad1415..7bd3c37 100644 > --- a/drivers/ufs/host/ufs-qcom.c > +++ b/drivers/ufs/host/ufs-qcom.c > @@ -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 64 Similar definitions are part of ufs-qcom.h. Thanks, Mani > 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) > +{ > + /* Qualcomm HC supports up to 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, > }; > > /** > diff --git a/include/ufs/ufs.h b/include/ufs/ufs.h > index ba2a1d8..5112418 100644 > --- a/include/ufs/ufs.h > +++ b/include/ufs/ufs.h > @@ -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; > diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h > index 7bf7599..e03b310 100644 > --- a/include/ufs/ufshcd.h > +++ b/include/ufs/ufshcd.h > @@ -297,6 +297,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, mandatory for mcq mode > */ > struct ufs_hba_variant_ops { > const char *name; > @@ -335,6 +336,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 */ > diff --git a/include/ufs/ufshci.h b/include/ufs/ufshci.h > index 4d4da06..67fcebd 100644 > --- a/include/ufs/ufshci.h > +++ b/include/ufs/ufshci.h > @@ -57,6 +57,7 @@ enum { > REG_UFS_CCAP = 0x100, > REG_UFS_CRYPTOCAP = 0x104, > > + REG_UFS_MCQ_CFG = 0x380, > UFSHCI_CRYPTO_REG_SPACE_SIZE = 0x400, > }; > > -- > 2.7.4 >
On Mon, Nov 28 2022 at 07:15 -0800, Manivannan Sadhasivam wrote: >On Tue, Nov 22, 2022 at 08:10:20PM -0800, Asutosh Das wrote: >> 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 | 32 ++++++++++++++++++++++++++++++++ >> drivers/ufs/core/ufshcd-priv.h | 9 +++++++++ >> drivers/ufs/core/ufshcd.c | 17 ++++++++++++++++- >> 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, 70 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/ufs/core/ufs-mcq.c b/drivers/ufs/core/ufs-mcq.c >> index 4aaa6aa..e95f748 100644 >> --- a/drivers/ufs/core/ufs-mcq.c >> +++ b/drivers/ufs/core/ufs-mcq.c >> @@ -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,36 @@ static const struct ufshcd_res_info ufs_res_info[RES_MAX] = { >> {.name = "mcq_vs",}, >> }; [...] Hello Mani, Thanks for taking a look. >> + WARN_ON(!hba->dev_info.bqueuedepth); > >Instead of panic, you could just print and return an error. > I'd make it WARN_ON_ONCE(); >> + /* >> + * max. value of bqueuedepth = 256, mac is host dependent. >> + * It is mandatory for UFS device to define bQueueDepth if >> + * shared queuing architecture is enabled. >> + */ >> + return min_t(int, mac, hba->dev_info.bqueuedepth); >> +} >> + >> static int ufshcd_mcq_config_resource(struct ufs_hba *hba) >> { >> struct platform_device *pdev = to_platform_device(hba->dev); >> diff --git a/drivers/ufs/core/ufshcd-priv.h b/drivers/ufs/core/ufshcd-priv.h >> index 9368ba2..9f40fa5 100644 >> --- a/drivers/ufs/core/ufshcd-priv.h >> +++ b/drivers/ufs/core/ufshcd-priv.h >> @@ -62,6 +62,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); >> +int ufshcd_mcq_decide_queue_depth(struct ufs_hba *hba); >> >> #define SD_ASCII_STD true >> #define SD_RAW false >> @@ -227,6 +228,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) > >Again, no inline please. > It spits out the following warning for all files that include this header, when inline is removed: warning: 'ufshcd_mcq_vops_get_hba_mac' defined but not used [-Wunused-function] [...] >> +#define MAX_SUPP_MAC 64 > >Similar definitions are part of ufs-qcom.h. > >Thanks, >Mani > > >-- >மணிவண்ணன் சதாசிவம்
On 11/28/22 11:54, Asutosh Das wrote: > On Mon, Nov 28 2022 at 07:15 -0800, Manivannan Sadhasivam wrote: >> On Tue, Nov 22, 2022 at 08:10:20PM -0800, Asutosh Das wrote: >>> +static inline int ufshcd_mcq_vops_get_hba_mac(struct ufs_hba *hba) >> >> Again, no inline please. >> > It spits out the following warning for all files that include this > header, when > inline is removed: > warning: 'ufshcd_mcq_vops_get_hba_mac' defined but not used > [-Wunused-function] My understanding is that the "no inline" rule applies to .c files only and also that functions defined in header files should be declared "static inline". Thanks, Bart.
diff --git a/drivers/ufs/core/ufs-mcq.c b/drivers/ufs/core/ufs-mcq.c index 4aaa6aa..e95f748 100644 --- a/drivers/ufs/core/ufs-mcq.c +++ b/drivers/ufs/core/ufs-mcq.c @@ -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,36 @@ 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. + * It is mandatory to implement get_hba_mac() to enable MCQ mode. + * Calculates and adjusts the queue depth based on the depth + * supported by the HC and ufs device. + */ +int ufshcd_mcq_decide_queue_depth(struct ufs_hba *hba) +{ + int mac; + + /* Mandatory to implement get_hba_mac() */ + mac = ufshcd_mcq_vops_get_hba_mac(hba); + if (mac < 0) { + dev_err(hba->dev, "Failed to get mac, err=%d\n", mac); + return mac; + } + + WARN_ON(!hba->dev_info.bqueuedepth); + /* + * max. value of bqueuedepth = 256, mac is host dependent. + * It is mandatory for UFS device to define bQueueDepth if + * shared queuing architecture is enabled. + */ + return min_t(int, mac, hba->dev_info.bqueuedepth); +} + static int ufshcd_mcq_config_resource(struct ufs_hba *hba) { struct platform_device *pdev = to_platform_device(hba->dev); diff --git a/drivers/ufs/core/ufshcd-priv.h b/drivers/ufs/core/ufshcd-priv.h index 9368ba2..9f40fa5 100644 --- a/drivers/ufs/core/ufshcd-priv.h +++ b/drivers/ufs/core/ufshcd-priv.h @@ -62,6 +62,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); +int ufshcd_mcq_decide_queue_depth(struct ufs_hba *hba); #define SD_ASCII_STD true #define SD_RAW false @@ -227,6 +228,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[]; /** diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c index 0c4cd8f..ae065da 100644 --- a/drivers/ufs/core/ufshcd.c +++ b/drivers/ufs/core/ufshcd.c @@ -7783,6 +7783,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]; @@ -8198,7 +8199,21 @@ static int ufshcd_add_lus(struct ufs_hba *hba) static int ufshcd_alloc_mcq(struct ufs_hba *hba) { - return ufshcd_mcq_init(hba); + int ret; + int old_nutrs = hba->nutrs; + + ret = ufshcd_mcq_decide_queue_depth(hba); + if (ret < 0) + return ret; + + hba->nutrs = ret; + ret = ufshcd_mcq_init(hba); + if (ret) { + hba->nutrs = old_nutrs; + return ret; + } + + return 0; } /** diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c index 8ad1415..7bd3c37 100644 --- a/drivers/ufs/host/ufs-qcom.c +++ b/drivers/ufs/host/ufs-qcom.c @@ -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 64 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) +{ + /* Qualcomm HC supports up to 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, }; /** diff --git a/include/ufs/ufs.h b/include/ufs/ufs.h index ba2a1d8..5112418 100644 --- a/include/ufs/ufs.h +++ b/include/ufs/ufs.h @@ -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; diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h index 7bf7599..e03b310 100644 --- a/include/ufs/ufshcd.h +++ b/include/ufs/ufshcd.h @@ -297,6 +297,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, mandatory for mcq mode */ struct ufs_hba_variant_ops { const char *name; @@ -335,6 +336,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 */ diff --git a/include/ufs/ufshci.h b/include/ufs/ufshci.h index 4d4da06..67fcebd 100644 --- a/include/ufs/ufshci.h +++ b/include/ufs/ufshci.h @@ -57,6 +57,7 @@ enum { REG_UFS_CCAP = 0x100, REG_UFS_CRYPTOCAP = 0x104, + REG_UFS_MCQ_CFG = 0x380, UFSHCI_CRYPTO_REG_SPACE_SIZE = 0x400, };