[v3,07/17] 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     | 34 ++++++++++++++++++++++++++++++++++
 drivers/ufs/core/ufshcd-priv.h |  9 +++++++++
 drivers/ufs/core/ufshcd.c      |  9 ++++++++-
 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, 64 insertions(+), 1 deletion(-)

On 10/20/22 11:03, Asutosh Das wrote:
> +u32 ufshcd_mcq_decide_queue_depth(struct ufs_hba *hba)
> +{
> +	u32 qd, val;
> +	int mac;
> +
> +	mac = ufshcd_mcq_vops_get_hba_mac(hba);
> +	if (mac < 0) {
> +		val = ufshcd_readl(hba, REG_UFS_MCQ_CFG);
> +		mac = FIELD_GET(MCQ_CFG_MAC_MASK, val);
> +	}

According to the UFSHCI 4.0 specification the MAC value is set by the 
host. Can the above code read the MAC value from the host controller 
before it has been set by the host? If so, how about leaving out the 
code that reads the MAC value from the controller and making it 
mandatory to implement the new get_hba_mac vop?

> +
> +	/*  MAC is a 0 based value. */
> +	mac += 1;
> +	/* max. value of bqueuedepth = 256, mac is host dependent */

host dependent -> defined by the host controller?

> +	qd = min_t(u32, mac, hba->dev_info.bqueuedepth);
> +	if (!qd)
> +		qd = mac;

How about using min_not_zero() instead of open-coding it?

Thanks,

Bart.

> On 10/20/22 11:03, Asutosh Das wrote:
> > +u32 ufshcd_mcq_decide_queue_depth(struct ufs_hba *hba)
> > +{
> > +     u32 qd, val;
> > +     int mac;
> > +
> > +     mac = ufshcd_mcq_vops_get_hba_mac(hba);
> > +     if (mac < 0) {
> > +             val = ufshcd_readl(hba, REG_UFS_MCQ_CFG);
> > +             mac = FIELD_GET(MCQ_CFG_MAC_MASK, val);
> > +     }
> 
> According to the UFSHCI 4.0 specification the MAC value is set by the
> host. Can the above code read the MAC value from the host controller
> before it has been set by the host? If so, how about leaving out the
> code that reads the MAC value from the controller and making it
> mandatory to implement the new get_hba_mac vop?
> 
> > +
> > +     /*  MAC is a 0 based value. */
> > +     mac += 1;
> > +     /* max. value of bqueuedepth = 256, mac is host dependent */
> 
> host dependent -> defined by the host controller?
> 
> > +     qd = min_t(u32, mac, hba->dev_info.bqueuedepth);
> > +     if (!qd)
> > +             qd = mac;
Isn't if mcq is supported bqueuedepth can't be zero?
The device must implements the shared queueing architecture.

Thanks,
Avri

> 
> How about using min_not_zero() instead of open-coding it?
> 
> Thanks,
> 
> Bart.

On Thu, Oct 27 2022 at 14:52 -0700, Bart Van Assche wrote:
>On 10/20/22 11:03, Asutosh Das wrote:
>>+u32 ufshcd_mcq_decide_queue_depth(struct ufs_hba *hba)
>>+{
>>+	u32 qd, val;
>>+	int mac;
>>+
>>+	mac = ufshcd_mcq_vops_get_hba_mac(hba);
>>+	if (mac < 0) {
>>+		val = ufshcd_readl(hba, REG_UFS_MCQ_CFG);
>>+		mac = FIELD_GET(MCQ_CFG_MAC_MASK, val);
>>+	}
>
>According to the UFSHCI 4.0 specification the MAC value is set by the 
>host. Can the above code read the MAC value from the host controller 
>before it has been set by the host? If so, how about leaving out the 
>code that reads the MAC value from the controller and making it 
>mandatory to implement the new get_hba_mac vop?
>
The reason it is not mandatory to define get_hba_mac vop is UFSHCI 4.0
specification mentions that the default value of MAC is 32. So even if a vendor
HC doesn't override the MAC, it'd be 32.
Hence, the current code first checks for an override, and if there's none uses
the default value defined in the HC.

-asd

On Sun, Oct 30 2022 at 04:43 -0700, Avri Altman wrote:
>> On 10/20/22 11:03, Asutosh Das wrote:
>> > +u32 ufshcd_mcq_decide_queue_depth(struct ufs_hba *hba)

[...]

>> > +     qd = min_t(u32, mac, hba->dev_info.bqueuedepth);
>> > +     if (!qd)
>> > +             qd = mac;
>Isn't if mcq is supported bqueuedepth can't be zero?
>The device must implements the shared queueing architecture.
>
Ah well, there's a Note mentioned in the UFS 4.0 spec (#7021) which kind of
confused me. I will remove this check if it can't be 0.

-asd
>

On 10/31/22 12:24, Asutosh Das wrote:
> On Thu, Oct 27 2022 at 14:52 -0700, Bart Van Assche wrote:
>> On 10/20/22 11:03, Asutosh Das wrote:
>>> +u32 ufshcd_mcq_decide_queue_depth(struct ufs_hba *hba)
>>> +{
>>> +    u32 qd, val;
>>> +    int mac;
>>> +
>>> +    mac = ufshcd_mcq_vops_get_hba_mac(hba);
>>> +    if (mac < 0) {
>>> +        val = ufshcd_readl(hba, REG_UFS_MCQ_CFG);
>>> +        mac = FIELD_GET(MCQ_CFG_MAC_MASK, val);
>>> +    }
>>
>> According to the UFSHCI 4.0 specification the MAC value is set by the 
>> host. Can the above code read the MAC value from the host controller 
>> before it has been set by the host? If so, how about leaving out the 
>> code that reads the MAC value from the controller and making it 
>> mandatory to implement the new get_hba_mac vop?
>>
> The reason it is not mandatory to define get_hba_mac vop is UFSHCI 4.0
> specification mentions that the default value of MAC is 32. So even if a 
> vendor
> HC doesn't override the MAC, it'd be 32.
> Hence, the current code first checks for an override, and if there's 
> none uses
> the default value defined in the HC.

Hi Asutosh,

Please ignore the value reported by the controller in the MAC field of 
the MCQConfig register and overwrite the MAC field without reading it 
first. It doesn't seem useful to me to read this field. I think the host 
should decide about the queue depth no matter what the current value of 
the MAC field is.

Thanks,

Bart.