Hi Bart,
Thank you so much for a detailed code review.
On 4/25/2023 5:04 PM, Bart Van Assche wrote:
On 4/17/23 14:05, Bao D. Nguyen wrote:
+/* Max mcq register polling time in milisecond unit */
A nit: please change "millisecond unit" into "milliseconds".
Yes I will change.
+static int ufshcd_mcq_poll_register(void __iomem *reg, u32 mask,
+ u32 val, unsigned long timeout_ms)
+{
+ unsigned long timeout = jiffies + msecs_to_jiffies(timeout_ms);
+ int err = 0;
+
+ /* ignore bits that we don't intend to wait on */
+ val = val & mask;
+
+ while ((readl(reg) & mask) != val) {
& has a higher precedence than != so one pair of parentheses can be left
out.
I think it is is actually the other way. & has lower precedence than !=.
Please correct me if I am wrong.
+ udelay(20);
+ if (time_after(jiffies, timeout)) {
Please use time_is_before_jiffies() instead of time_after(jiffies, ...).
time_is_before_jiffies() seems to be defined as time_after(). Could you
please explain the benefits to choose one over the other?
+ err = -ETIMEDOUT;
+ break;
+ }
+ }
+
+ return err;
+}
Please remove the variable 'err' and return the return value directly.
Yes I will change.
+
+static int ufshcd_mcq_sq_stop(struct ufs_hba *hba, struct
ufs_hw_queue *hwq)
+{
+ void __iomem *reg;
+ u32 i = hwq->id;
Please use another variable name than 'i' for a hardware queue ID ('id'?).
Yes I will change.
+ u32 i = hwq->id;
Same comment here.
Yes I will change.
+/**
+ * ufshcd_mcq_sq_cleanup - Clean up Submission Queue resources
A nit: please use lower case text for "submission queue" and also in the
comments below ("Clean up" -> "clean up").
The UFS Host Controller specification uses upper case for the Submission
Queue and Completion Queue, so I tried to follow the the spec language.
I don't have a preference. I will make the change.
+ spin_lock(&hwq->sq_lock);
+
+ /* stop the SQ fetching before working on it */
+ err = ufshcd_mcq_sq_stop(hba, hwq);
+ if (err)
+ goto unlock;
No spin locks around delay loops please. Is there anything that prevents
to change sq_lock from a spin lock into a mutex?
This function can be called from multiple non-interrupt contexts. I
needed to prevent concurrent accesses to the sq hw, so yes mutex would
work better. I will change.
+static u64 ufshcd_mcq_get_cmd_desc_addr(struct ufs_hba *hba,
+ int task_tag)
+{
+ struct ufshcd_lrb *lrbp = &hba->lrb[task_tag];
+ __le32 hi = lrbp->utr_descriptor_ptr->command_desc_base_addr_hi;
+ __le32 lo = lrbp->utr_descriptor_ptr->command_desc_base_addr_lo;
+
+ return le64_to_cpu((__le64)hi << 32 | lo);
+}
Please add a new patch at the head of this series that modifies struct
utp_transfer_req_desc such that command_desc_base_addr_lo and
command_desc_base_addr_hi are combined into a single __le64 variable.
Yes, I will add this as a separate patch.
+/**
+ * ufshcd_mcq_nullify_cmd - Nullify utrd. Host controller does not fetch
+ * transfer with Command Type = 0xF. post the Completion Queue with
OCS=ABORTED.
+ * @hba - per adapter instance.
+ * @hwq - Hardware Queue of the nullified utrd.
+ */
+static void ufshcd_mcq_nullify_cmd(struct ufs_hba *hba, struct
ufs_hw_queue *hwq)
+{
+ struct utp_transfer_req_desc *utrd;
+ u32 dword_0;
+
+ utrd = (struct utp_transfer_req_desc *)(hwq->sqe_base_addr +
+ hwq->id * sizeof(struct utp_transfer_req_desc));
Please double check this function. It has "cmd" in the function name but
none of the arguments passed to this function allows to uniquely
identify a command. Is an argument perhaps missing from this function?
Yes, I will make the correction to this function and rename it to
ufshcd_mcq_nullify_sqe()
Additionally, hwq->sqe_base_addr points to an array of SQE entries. I do
not understand why hwq->id * sizeof(struct utp_transfer_req_desc) is
added to that base address. Please clarify. >
+ utrd = (struct utp_transfer_req_desc *)(hwq->sqe_base_addr +
+ sq_head_slot * sizeof(struct utp_transfer_req_desc));
hwq->sqe_base_addr already has type struct utp_transfer_req_desc * so
the " * sizeof(struct utp_transfer_req_desc)" part looks wrong to me.
Yes, I will correct this.
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 35a3bd9..808387c 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -56,7 +56,6 @@
#define NOP_OUT_RETRIES 10
/* Timeout after 50 msecs if NOP OUT hangs without response */
#define NOP_OUT_TIMEOUT 50 /* msecs */
-
/* Query request retries */
#define QUERY_REQ_RETRIES 3
/* Query request timeout */
Is the above change really necessary?
The blank line was removed by mistake. I will put it back.
@@ -173,7 +172,6 @@ EXPORT_SYMBOL_GPL(ufshcd_dump_regs);
enum {
UFSHCD_MAX_CHANNEL = 0,
UFSHCD_MAX_ID = 1,
- UFSHCD_NUM_RESERVED = 1,
UFSHCD_CMD_PER_LUN = 32 - UFSHCD_NUM_RESERVED,
UFSHCD_CAN_QUEUE = 32 - UFSHCD_NUM_RESERVED,
};
Same question here - is this change really necessary?
I am moving the definition of UFSHCD_NUM_RESERVED to
include/ufs/ufshci.h file so that I can access it from /core/ufs-mcq.c
Thanks,
Bart.
