2023-04-24 14:13:16

by Po-Wen Kao

[permalink] [raw]
Subject: [PATCH v5 0/3] Several UFS MCQ Code Changes

v4 -> v5
- Send MTK driver MCQ support as independent patch and address Bart's
comment there
"[PATCH v1 0/2] Add MCQ support for MTK platform"

v3 -> v4
- Rebase on latest scsi-next/staging
- Drop "scsi: ufs: core: Remove redundant check" since it's already fixed
- Improve commit message as suggested by Manivannan Sadhasivam
- Fix patch check error

v2 -> v3
- Remove "scsi: ufs: core: Add hwq print for debug".
send later when error handling is ready.
- Rename patch title to match convention "scsi: ufs: ufs-mediatek:..."
- Add explanation for (nr_hw_queues = MAXQ + 1)
- Remove dummy line

v1 -> v2:
- Add 2 new patches
- Update MTK driver for mcq
- Export symbols for MTK driver
- Fix commit message in "scsi: ufs: core: Fix mcq nr_hw_queues"
- Split name change and fix into two patches


Po-Wen Kao (3):
scsi: ufs: core: Fix mcq tag calcualtion
scsi: ufs: core: Rename symbol sizeof_utp_transfer_cmd_desc()
scsi: ufs: core: Fix mcq nr_hw_queues

drivers/ufs/core/ufs-mcq.c | 5 +++--
drivers/ufs/core/ufshcd.c | 8 ++++----
include/ufs/ufshcd.h | 2 +-
3 files changed, 8 insertions(+), 7 deletions(-)

--
2.18.0


2023-04-24 14:13:41

by Po-Wen Kao

[permalink] [raw]
Subject: [PATCH v5 1/3] scsi: ufs: core: Fix mcq tag calcualtion

Transfer command descriptor is allocated in ufshcd_memory_alloc()
and referenced by transfer request descriptor with stride size
sizeof_utp_transfer_cmd_desc()
instead of
sizeof(struct utp_transfer_cmd_desc).

Consequently, computing tag by address offset should also refer to the
same stride.

Signed-off-by: Po-Wen Kao <[email protected]>
Reviewed-by: Bart Van Assche <[email protected]>
Reviewed-by: Manivannan Sadhasivam <[email protected]>
Reviewed-by: Stanley Chu <[email protected]>
Reviewed-by: Ziqi Chen <[email protected]>
---
drivers/ufs/core/ufs-mcq.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/ufs/core/ufs-mcq.c b/drivers/ufs/core/ufs-mcq.c
index 31df052fbc41..3a27fa4b0024 100644
--- a/drivers/ufs/core/ufs-mcq.c
+++ b/drivers/ufs/core/ufs-mcq.c
@@ -265,7 +265,7 @@ static int ufshcd_mcq_get_tag(struct ufs_hba *hba,
addr = (le64_to_cpu(cqe->command_desc_base_addr) & CQE_UCD_BA) -
hba->ucdl_dma_addr;

- return div_u64(addr, sizeof(struct utp_transfer_cmd_desc));
+ return div_u64(addr, sizeof_utp_transfer_cmd_desc(hba));
}

static void ufshcd_mcq_process_cqe(struct ufs_hba *hba,
--
2.18.0

2023-04-24 14:13:47

by Po-Wen Kao

[permalink] [raw]
Subject: [PATCH v5 3/3] scsi: ufs: core: Fix mcq nr_hw_queues

Since MAXQ is 0 based value, add one to obtain number of hardware queue.

Signed-off-by: Po-Wen Kao <[email protected]>
Reviewed-by: Bean Huo <[email protected]>
Reviewed-by: Manivannan Sadhasivam <[email protected]>
Reviewed-by: Bart Van Assche <[email protected]>
Reviewed-by: Stanley Chu <[email protected]>
---
drivers/ufs/core/ufs-mcq.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/ufs/core/ufs-mcq.c b/drivers/ufs/core/ufs-mcq.c
index a39746b2a8be..c7b807f58dca 100644
--- a/drivers/ufs/core/ufs-mcq.c
+++ b/drivers/ufs/core/ufs-mcq.c
@@ -150,7 +150,8 @@ static int ufshcd_mcq_config_nr_queues(struct ufs_hba *hba)
u32 hba_maxq, rem, tot_queues;
struct Scsi_Host *host = hba->host;

- hba_maxq = FIELD_GET(MAX_QUEUE_SUP, hba->mcq_capabilities);
+ /* maxq is 0 based value */
+ hba_maxq = FIELD_GET(MAX_QUEUE_SUP, hba->mcq_capabilities) + 1;

tot_queues = UFS_MCQ_NUM_DEV_CMD_QUEUES + read_queues + poll_queues +
rw_queues;
--
2.18.0

2023-04-24 14:14:49

by Po-Wen Kao

[permalink] [raw]
Subject: [PATCH v5 2/3] scsi: ufs: core: Rename symbol sizeof_utp_transfer_cmd_desc()

Naming the functions after standard operators like sizeof may cause
confusion. So let's rename it to ufshcd_get_ucd_size().

Signed-off-by: Po-Wen Kao <[email protected]>
Suggested-by: Manivannan Sadhasivam <[email protected]>
Reviewed-by: Stanley Chu <[email protected]>
Reviewed-by: Ziqi Chen <[email protected]>
Reviewed-by: Bart Van Assche <[email protected]>
---
drivers/ufs/core/ufs-mcq.c | 2 +-
drivers/ufs/core/ufshcd.c | 8 ++++----
include/ufs/ufshcd.h | 2 +-
3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/ufs/core/ufs-mcq.c b/drivers/ufs/core/ufs-mcq.c
index 3a27fa4b0024..a39746b2a8be 100644
--- a/drivers/ufs/core/ufs-mcq.c
+++ b/drivers/ufs/core/ufs-mcq.c
@@ -265,7 +265,7 @@ static int ufshcd_mcq_get_tag(struct ufs_hba *hba,
addr = (le64_to_cpu(cqe->command_desc_base_addr) & CQE_UCD_BA) -
hba->ucdl_dma_addr;

- return div_u64(addr, sizeof_utp_transfer_cmd_desc(hba));
+ return div_u64(addr, ufshcd_get_ucd_size(hba));
}

static void ufshcd_mcq_process_cqe(struct ufs_hba *hba,
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 17d7bb875fee..21d1831b5faf 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -2849,10 +2849,10 @@ static void ufshcd_map_queues(struct Scsi_Host *shost)
static void ufshcd_init_lrb(struct ufs_hba *hba, struct ufshcd_lrb *lrb, int i)
{
struct utp_transfer_cmd_desc *cmd_descp = (void *)hba->ucdl_base_addr +
- i * sizeof_utp_transfer_cmd_desc(hba);
+ i * ufshcd_get_ucd_size(hba);
struct utp_transfer_req_desc *utrdlp = hba->utrdl_base_addr;
dma_addr_t cmd_desc_element_addr = hba->ucdl_dma_addr +
- i * sizeof_utp_transfer_cmd_desc(hba);
+ i * ufshcd_get_ucd_size(hba);
u16 response_offset = offsetof(struct utp_transfer_cmd_desc,
response_upiu);
u16 prdt_offset = offsetof(struct utp_transfer_cmd_desc, prd_table);
@@ -3761,7 +3761,7 @@ static int ufshcd_memory_alloc(struct ufs_hba *hba)
size_t utmrdl_size, utrdl_size, ucdl_size;

/* Allocate memory for UTP command descriptors */
- ucdl_size = sizeof_utp_transfer_cmd_desc(hba) * hba->nutrs;
+ ucdl_size = ufshcd_get_ucd_size(hba) * hba->nutrs;
hba->ucdl_base_addr = dmam_alloc_coherent(hba->dev,
ucdl_size,
&hba->ucdl_dma_addr,
@@ -3861,7 +3861,7 @@ static void ufshcd_host_memory_configure(struct ufs_hba *hba)
prdt_offset =
offsetof(struct utp_transfer_cmd_desc, prd_table);

- cmd_desc_size = sizeof_utp_transfer_cmd_desc(hba);
+ cmd_desc_size = ufshcd_get_ucd_size(hba);
cmd_desc_dma_addr = hba->ucdl_dma_addr;

for (i = 0; i < hba->nutrs; i++) {
diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
index f7553293ba98..df1d04f7a542 100644
--- a/include/ufs/ufshcd.h
+++ b/include/ufs/ufshcd.h
@@ -1133,7 +1133,7 @@ static inline size_t ufshcd_sg_entry_size(const struct ufs_hba *hba)
({ (void)(hba); BUILD_BUG_ON(sg_entry_size != sizeof(struct ufshcd_sg_entry)); })
#endif

-static inline size_t sizeof_utp_transfer_cmd_desc(const struct ufs_hba *hba)
+static inline size_t ufshcd_get_ucd_size(const struct ufs_hba *hba)
{
return sizeof(struct utp_transfer_cmd_desc) + SG_ALL * ufshcd_sg_entry_size(hba);
}
--
2.18.0

2023-05-02 07:41:19

by Stanley Jhu

[permalink] [raw]
Subject: Re: [PATCH v5 1/3] scsi: ufs: core: Fix mcq tag calcualtion

Hi Po-Wen,

Po-Wen Kao <[email protected]> 於 2023年4月24日 週一 下午10:13寫道:
>
> Transfer command descriptor is allocated in ufshcd_memory_alloc()
> and referenced by transfer request descriptor with stride size
> sizeof_utp_transfer_cmd_desc()
> instead of
> sizeof(struct utp_transfer_cmd_desc).
>
> Consequently, computing tag by address offset should also refer to the
> same stride.
>
> Signed-off-by: Po-Wen Kao <[email protected]>
> Reviewed-by: Bart Van Assche <[email protected]>
> Reviewed-by: Manivannan Sadhasivam <[email protected]>
> Reviewed-by: Stanley Chu <[email protected]>
> Reviewed-by: Ziqi Chen <[email protected]>
> ---
> drivers/ufs/core/ufs-mcq.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/ufs/core/ufs-mcq.c b/drivers/ufs/core/ufs-mcq.c
> index 31df052fbc41..3a27fa4b0024 100644
> --- a/drivers/ufs/core/ufs-mcq.c
> +++ b/drivers/ufs/core/ufs-mcq.c
> @@ -265,7 +265,7 @@ static int ufshcd_mcq_get_tag(struct ufs_hba *hba,
> addr = (le64_to_cpu(cqe->command_desc_base_addr) & CQE_UCD_BA) -
> hba->ucdl_dma_addr;
>
> - return div_u64(addr, sizeof(struct utp_transfer_cmd_desc));
> + return div_u64(addr, sizeof_utp_transfer_cmd_desc(hba));

Would you also need to consider the ufshcd_release_sdb_queue() case?

Thanks.
Stanley Chu