[v5,07/16] ufs: core: mcq: Calculate queue depth

  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(-)
  

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.