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

[Asutosh Das <quic_asutoshd@quicinc.com>]

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
>
>
>-- 
>மணிவண்ணன் சதாசிவம்