Messages in this thread | | | Date | Wed, 9 Nov 2022 13:28:44 -0800 | Subject | Re: [PATCH v4 07/16] ufs: core: mcq: Calculate queue depth | From | Bart Van Assche <> |
| |
On 11/9/22 11:41, 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; > + } > + > + /* MAC is a 0 based value. */ > + mac += 1;
Please make ufshcd_mcq_vops_get_hba_mac() return a 1-based value. The 0-based convention is useful for bit-constrained device registers but is confusing when not reading from a hardware register.
> + /* > + * 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);
According to the UFS specification bQueueDepth is zero if there is one queue per logical unit inside the device. Should a warning statement be added that reports a complaint if bQueueDepth == 0 since the above code does not support bQueueDepth == 0? I'm not sure whether any UFS devices exist that use per-LU queuing.
> +static int ufs_qcom_get_hba_mac(struct ufs_hba *hba) > +{ > + /* Default is 32, but Qualcomm HC supports upto 64 */
I think that "default is 32" should be left out since the code that reads the MAC register has been removed.
Additionally, please change "upto" into "up to".
Thanks,
Bart.
| |