Add supporting functions to handle ufs abort in mcq mode.
Signed-off-by: Bao D. Nguyen <[email protected]>
---
drivers/ufs/core/ufs-mcq.c | 190 +++++++++++++++++++++++++++++++++++++++++
drivers/ufs/core/ufshcd-priv.h | 10 +++
drivers/ufs/core/ufshcd.c | 2 -
include/ufs/ufshci.h | 17 ++++
4 files changed, 217 insertions(+), 2 deletions(-)
diff --git a/drivers/ufs/core/ufs-mcq.c b/drivers/ufs/core/ufs-mcq.c
index 31df052..a463674 100644
--- a/drivers/ufs/core/ufs-mcq.c
+++ b/drivers/ufs/core/ufs-mcq.c
@@ -12,6 +12,9 @@
#include <linux/module.h>
#include <linux/platform_device.h>
#include "ufshcd-priv.h"
+#include <linux/delay.h>
+#include <scsi/scsi_cmnd.h>
+#include <linux/bitfield.h>
#define MAX_QUEUE_SUP GENMASK(7, 0)
#define UFS_MCQ_MIN_RW_QUEUES 2
@@ -27,6 +30,9 @@
#define MCQ_ENTRY_SIZE_IN_DWORD 8
#define CQE_UCD_BA GENMASK_ULL(63, 7)
+/* Max mcq register polling time in milisecond unit */
+#define MCQ_POLL_MS 500
+
static int rw_queue_count_set(const char *val, const struct kernel_param *kp)
{
return param_set_uint_minmax(val, kp, UFS_MCQ_MIN_RW_QUEUES,
@@ -429,3 +435,187 @@ int ufshcd_mcq_init(struct ufs_hba *hba)
host->host_tagset = 1;
return 0;
}
+
+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) {
+ udelay(20);
+ if (time_after(jiffies, timeout)) {
+ err = -ETIMEDOUT;
+ break;
+ }
+ }
+
+ return err;
+}
+
+static int ufshcd_mcq_sq_stop(struct ufs_hba *hba, struct ufs_hw_queue *hwq)
+{
+ void __iomem *reg;
+ u32 i = hwq->id;
+ int err;
+
+ writel(SQ_STOP, mcq_opr_base(hba, OPR_SQD, i) + REG_SQRTC);
+ reg = mcq_opr_base(hba, OPR_SQD, i) + REG_SQRTS;
+ err = ufshcd_mcq_poll_register(reg, SQ_STS, SQ_STS, MCQ_POLL_MS);
+ if (err)
+ dev_err(hba->dev, "%s: failed. hwq-id=%d, err=%d\n",
+ __func__, i, err);
+ return err;
+}
+
+static int ufshcd_mcq_sq_start(struct ufs_hba *hba, struct ufs_hw_queue *hwq)
+{
+ void __iomem *reg;
+ u32 i = hwq->id;
+ int err;
+
+ writel(SQ_START, mcq_opr_base(hba, OPR_SQD, i) + REG_SQRTC);
+ reg = mcq_opr_base(hba, OPR_SQD, i) + REG_SQRTS;
+ err = ufshcd_mcq_poll_register(reg, SQ_STS, 0, MCQ_POLL_MS);
+ if (err)
+ dev_err(hba->dev, "%s: failed. hwq-id=%d, err=%d\n",
+ __func__, i, err);
+ return err;
+}
+
+/**
+ * ufshcd_mcq_sq_cleanup - Clean up Submission Queue resources
+ * associated with the pending command.
+ * @hba - per adapter instance.
+ * @task_tag - The command's task tag.
+ * @result - Result of the Clean up operation.
+ *
+ * Returns 0 and result on completion. Returns error code if
+ * the operation fails.
+ */
+int ufshcd_mcq_sq_cleanup(struct ufs_hba *hba, int task_tag, int *result)
+{
+ struct ufshcd_lrb *lrbp = &hba->lrb[task_tag];
+ struct scsi_cmnd *cmd = lrbp->cmd;
+ struct ufs_hw_queue *hwq;
+ void __iomem *reg, *opr_sqd_base;
+ u32 nexus, i;
+ int err;
+
+ if (task_tag != hba->nutrs - UFSHCD_NUM_RESERVED) {
+ if (!cmd)
+ return FAILED;
+ hwq = ufshcd_mcq_req_to_hwq(hba, scsi_cmd_to_rq(cmd));
+ } else {
+ hwq = hba->dev_cmd_queue;
+ }
+
+ i = hwq->id;
+
+ spin_lock(&hwq->sq_lock);
+
+ /* stop the SQ fetching before working on it */
+ err = ufshcd_mcq_sq_stop(hba, hwq);
+ if (err)
+ goto unlock;
+
+ /* SQCTI = EXT_IID, IID, LUN, Task Tag */
+ nexus = lrbp->lun << 8 | task_tag;
+ opr_sqd_base = mcq_opr_base(hba, OPR_SQD, i);
+ writel(nexus, opr_sqd_base + REG_SQCTI);
+
+ /* SQRTCy.ICU = 1 */
+ writel(SQ_ICU, opr_sqd_base + REG_SQRTC);
+
+ /* Poll SQRTSy.CUS = 1. Return result from SQRTSy.RTC */
+ reg = opr_sqd_base + REG_SQRTS;
+ err = ufshcd_mcq_poll_register(reg, SQ_CUS, SQ_CUS, MCQ_POLL_MS);
+ if (err)
+ dev_err(hba->dev, "%s: failed. hwq=%d, lun=0x%x, tag=%d\n",
+ __func__, i, lrbp->lun, task_tag);
+
+ *result = FIELD_GET(SQ_ICU_ERR_CODE_MASK, readl(reg));
+
+ if (ufshcd_mcq_sq_start(hba, hwq))
+ err = FAILED;
+
+unlock:
+ spin_unlock(&hwq->sq_lock);
+ return err;
+}
+
+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);
+}
+
+/**
+ * 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));
+ dword_0 = le32_to_cpu(utrd->header.dword_0);
+ dword_0 &= ~UPIU_COMMAND_TYPE_MASK;
+ dword_0 |= FIELD_PREP(UPIU_COMMAND_TYPE_MASK, 0xF);
+ utrd->header.dword_0 = cpu_to_le32(dword_0);
+}
+
+/**
+ * ufshcd_mcq_sqe_search - Search for the cmd in the Submission Queue (SQ)
+ * @hba - per adapter instance.
+ * @hwq - Hardware Queue to be searched.
+ * @task_tag - The command's task tag.
+ *
+ * Returns true if the SQE containing the command is present in the SQ
+ * (not fetched by the controller); returns false if the SQE is not in the SQ.
+ */
+static bool ufshcd_mcq_sqe_search(struct ufs_hba *hba,
+ struct ufs_hw_queue *hwq, int task_tag)
+{
+ struct utp_transfer_req_desc *utrd;
+ u32 mask = hwq->max_entries - 1;
+ bool ret = false;
+ u64 addr, match;
+ u32 sq_head_slot;
+
+ spin_lock(&hwq->sq_lock);
+
+ ufshcd_mcq_sq_stop(hba, hwq);
+ sq_head_slot = ufshcd_mcq_get_sq_head_slot(hwq);
+ if (sq_head_slot == hwq->sq_tail_slot)
+ goto out;
+
+ addr = ufshcd_mcq_get_cmd_desc_addr(hba, task_tag);
+ while (sq_head_slot != hwq->sq_tail_slot) {
+ utrd = (struct utp_transfer_req_desc *)(hwq->sqe_base_addr +
+ sq_head_slot * sizeof(struct utp_transfer_req_desc));
+ match = le64_to_cpu((__le64)utrd->command_desc_base_addr_hi << 32 |
+ utrd->command_desc_base_addr_lo) & CQE_UCD_BA;
+ if (addr == match) {
+ ret = true;
+ goto out;
+ }
+ sq_head_slot = (sq_head_slot + 1) & mask;
+ }
+
+out:
+ ufshcd_mcq_sq_start(hba, hwq);
+ spin_unlock(&hwq->sq_lock);
+ return ret;
+}
diff --git a/drivers/ufs/core/ufshcd-priv.h b/drivers/ufs/core/ufshcd-priv.h
index 529f850..1a40cf7 100644
--- a/drivers/ufs/core/ufshcd-priv.h
+++ b/drivers/ufs/core/ufshcd-priv.h
@@ -78,6 +78,8 @@ struct ufs_hw_queue *ufshcd_mcq_req_to_hwq(struct ufs_hba *hba,
unsigned long ufshcd_mcq_poll_cqe_lock(struct ufs_hba *hba,
struct ufs_hw_queue *hwq);
+int ufshcd_mcq_sq_cleanup(struct ufs_hba *hba, int task_tag, int *result);
+
#define UFSHCD_MCQ_IO_QUEUE_OFFSET 1
#define SD_ASCII_STD true
#define SD_RAW false
@@ -403,4 +405,12 @@ static inline struct cq_entry *ufshcd_mcq_cur_cqe(struct ufs_hw_queue *q)
return cqe + q->cq_head_slot;
}
+
+static inline u32 ufshcd_mcq_get_sq_head_slot(struct ufs_hw_queue *q)
+{
+ u32 val = readl(q->mcq_sq_head);
+
+ return val / sizeof(struct utp_transfer_req_desc);
+}
+
#endif /* _UFSHCD_PRIV_H_ */
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 */
@@ -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,
};
diff --git a/include/ufs/ufshci.h b/include/ufs/ufshci.h
index 11424bb..ceb8664 100644
--- a/include/ufs/ufshci.h
+++ b/include/ufs/ufshci.h
@@ -99,6 +99,9 @@ enum {
enum {
REG_SQHP = 0x0,
REG_SQTP = 0x4,
+ REG_SQRTC = 0x8,
+ REG_SQCTI = 0xC,
+ REG_SQRTS = 0x10,
};
enum {
@@ -111,12 +114,26 @@ enum {
REG_CQIE = 0x4,
};
+enum {
+ SQ_START = 0x0,
+ SQ_STOP = 0x1,
+ SQ_ICU = 0x2,
+};
+
+enum {
+ SQ_STS = 0x1,
+ SQ_CUS = 0x2,
+};
+
+#define SQ_ICU_ERR_CODE_MASK GENMASK(7, 4)
+#define UPIU_COMMAND_TYPE_MASK GENMASK(31, 28)
#define UFS_MASK(mask, offset) ((mask) << (offset))
/* UFS Version 08h */
#define MINOR_VERSION_NUM_MASK UFS_MASK(0xFFFF, 0)
#define MAJOR_VERSION_NUM_MASK UFS_MASK(0xFFFF, 16)
+#define UFSHCD_NUM_RESERVED 1
/*
* Controller UFSHCI version
* - 2.x and newer use the following scheme:
--
2.7.4
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".
> +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.
> + udelay(20);
> + if (time_after(jiffies, timeout)) {
Please use time_is_before_jiffies() instead of time_after(jiffies, ...).
> + err = -ETIMEDOUT;
> + break;
> + }
> + }
> +
> + return err;
> +}
Please remove the variable 'err' and return the return value directly.
> +
> +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'?).
> + u32 i = hwq->id;
Same comment here.
> +/**
> + * 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").
> + 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?
> +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.
> +/**
> + * 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?
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.
> 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?
> @@ -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?
Thanks,
Bart.
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.