2022-09-02 23:09:57

by Asutosh Das

[permalink] [raw]
Subject: [RFC PATCH v3 1/4] ufs: core: prepare ufs for multi circular queue support

Preparatory changes for upcoming multi circular queue.

Co-developed-by: Can Guo <[email protected]>
Signed-off-by: Can Guo <[email protected]>
Signed-off-by: Asutosh Das <[email protected]>
---
drivers/ufs/core/ufshcd.c | 99 +++++++++++++++++++++++++----------------------
1 file changed, 53 insertions(+), 46 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index f4f8ded..b119f45 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -249,7 +249,7 @@ static int ufshcd_reset_and_restore(struct ufs_hba *hba);
static int ufshcd_eh_host_reset_handler(struct scsi_cmnd *cmd);
static int ufshcd_clear_tm_cmd(struct ufs_hba *hba, int tag);
static void ufshcd_hba_exit(struct ufs_hba *hba);
-static int ufshcd_probe_hba(struct ufs_hba *hba, bool init_dev_params);
+static int ufshcd_probe_hba(struct ufs_hba *hba, bool initial_boot);
static int ufshcd_setup_clocks(struct ufs_hba *hba, bool on);
static inline void ufshcd_add_delay_before_dme_cmd(struct ufs_hba *hba);
static int ufshcd_host_reset_and_restore(struct ufs_hba *hba);
@@ -310,10 +310,11 @@ static void ufshcd_scsi_block_requests(struct ufs_hba *hba)
scsi_block_requests(hba->host);
}

-static void ufshcd_add_cmd_upiu_trace(struct ufs_hba *hba, unsigned int tag,
+static void ufshcd_add_cmd_upiu_trace(struct ufs_hba *hba,
+ struct ufshcd_lrb *lrbp,
enum ufs_trace_str_t str_t)
{
- struct utp_upiu_req *rq = hba->lrb[tag].ucd_req_ptr;
+ struct utp_upiu_req *rq = lrbp->ucd_req_ptr;
struct utp_upiu_header *header;

if (!trace_ufshcd_upiu_enabled())
@@ -322,7 +323,7 @@ static void ufshcd_add_cmd_upiu_trace(struct ufs_hba *hba, unsigned int tag,
if (str_t == UFS_CMD_SEND)
header = &rq->header;
else
- header = &hba->lrb[tag].ucd_rsp_ptr->header;
+ header = &lrbp->ucd_rsp_ptr->header;

trace_ufshcd_upiu(dev_name(hba->dev), str_t, header, &rq->sc.cdb,
UFS_TSF_CDB);
@@ -379,13 +380,13 @@ static void ufshcd_add_uic_command_trace(struct ufs_hba *hba,
ufshcd_readl(hba, REG_UIC_COMMAND_ARG_3));
}

-static void ufshcd_add_command_trace(struct ufs_hba *hba, unsigned int tag,
+static void ufshcd_add_command_trace(struct ufs_hba *hba,
+ struct ufshcd_lrb *lrbp,
enum ufs_trace_str_t str_t)
{
u64 lba = 0;
u8 opcode = 0, group_id = 0;
u32 intr, doorbell;
- struct ufshcd_lrb *lrbp = &hba->lrb[tag];
struct scsi_cmnd *cmd = lrbp->cmd;
struct request *rq = scsi_cmd_to_rq(cmd);
int transfer_len = -1;
@@ -394,7 +395,7 @@ static void ufshcd_add_command_trace(struct ufs_hba *hba, unsigned int tag,
return;

/* trace UPIU also */
- ufshcd_add_cmd_upiu_trace(hba, tag, str_t);
+ ufshcd_add_cmd_upiu_trace(hba, lrbp, str_t);
if (!trace_ufshcd_command_enabled())
return;

@@ -419,7 +420,7 @@ static void ufshcd_add_command_trace(struct ufs_hba *hba, unsigned int tag,

intr = ufshcd_readl(hba, REG_INTERRUPT_STATUS);
doorbell = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL);
- trace_ufshcd_command(dev_name(hba->dev), str_t, tag,
+ trace_ufshcd_command(dev_name(hba->dev), str_t, lrbp->task_tag,
doorbell, transfer_len, intr, lba, opcode, group_id);
}

@@ -2134,14 +2135,14 @@ static void ufshcd_update_monitor(struct ufs_hba *hba, const struct ufshcd_lrb *
* @task_tag: Task tag of the command
*/
static inline
-void ufshcd_send_command(struct ufs_hba *hba, unsigned int task_tag)
+void ufshcd_send_command(struct ufs_hba *hba, struct ufshcd_lrb *lrbp)
{
- struct ufshcd_lrb *lrbp = &hba->lrb[task_tag];
+ int task_tag = lrbp->task_tag;
unsigned long flags;

lrbp->issue_time_stamp = ktime_get();
lrbp->compl_time_stamp = ktime_set(0, 0);
- ufshcd_add_command_trace(hba, task_tag, UFS_CMD_SEND);
+ ufshcd_add_command_trace(hba, lrbp, UFS_CMD_SEND);
ufshcd_clk_scaling_start_busy(hba);
if (unlikely(ufshcd_should_inform_monitor(hba, lrbp)))
ufshcd_start_monitor(hba, lrbp);
@@ -2553,9 +2554,10 @@ void ufshcd_prepare_utp_scsi_cmd_upiu(struct ufshcd_lrb *lrbp, u8 upiu_flags)
/* command descriptor fields */
ucd_req_ptr->header.dword_0 = UPIU_HEADER_DWORD(
UPIU_TRANSACTION_COMMAND, upiu_flags,
- lrbp->lun, lrbp->task_tag);
+ lrbp->lun, lrbp->task_tag & 0xff);
ucd_req_ptr->header.dword_1 = UPIU_HEADER_DWORD(
- UPIU_COMMAND_SET_TYPE_SCSI, 0, 0, 0);
+ UPIU_COMMAND_SET_TYPE_SCSI, 0, 0,
+ (lrbp->task_tag & 0xf00) << 4);

/* Total EHS length and Data segment length will be zero */
ucd_req_ptr->header.dword_2 = 0;
@@ -2845,7 +2847,7 @@ static int ufshcd_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd)
goto out;
}

- ufshcd_send_command(hba, tag);
+ ufshcd_send_command(hba, lrbp);

out:
rcu_read_unlock();
@@ -2971,7 +2973,7 @@ static int ufshcd_wait_for_dev_cmd(struct ufs_hba *hba,

if (!time_left) {
err = -ETIMEDOUT;
- dev_dbg(hba->dev, "%s: dev_cmd request timedout, tag %d\n",
+ dev_err(hba->dev, "%s: dev_cmd request timedout, tag %d\n",
__func__, lrbp->task_tag);
if (!ufshcd_clear_cmds(hba, 1U << lrbp->task_tag))
/* successfully cleared the command, retry if needed */
@@ -3021,7 +3023,7 @@ static int ufshcd_exec_dev_cmd(struct ufs_hba *hba,

ufshcd_add_query_upiu_trace(hba, UFS_QUERY_SEND, lrbp->ucd_req_ptr);

- ufshcd_send_command(hba, tag);
+ ufshcd_send_command(hba, lrbp);
err = ufshcd_wait_for_dev_cmd(hba, lrbp, timeout);
ufshcd_add_query_upiu_trace(hba, err ? UFS_QUERY_ERR : UFS_QUERY_COMP,
(struct utp_upiu_req *)lrbp->ucd_rsp_ptr);
@@ -4513,6 +4515,7 @@ int ufshcd_make_hba_operational(struct ufs_hba *hba)
REG_UTP_TRANSFER_REQ_LIST_BASE_L);
ufshcd_writel(hba, upper_32_bits(hba->utrdl_dma_addr),
REG_UTP_TRANSFER_REQ_LIST_BASE_H);
+
ufshcd_writel(hba, lower_32_bits(hba->utmrdl_dma_addr),
REG_UTP_TASK_REQ_LIST_BASE_L);
ufshcd_writel(hba, upper_32_bits(hba->utmrdl_dma_addr),
@@ -5320,6 +5323,32 @@ static void ufshcd_release_scsi_cmd(struct ufs_hba *hba,
ufshcd_clk_scaling_update_busy(hba);
}

+void ufshcd_compl_one_cqe(struct ufs_hba *hba, int task_tag)
+{
+ struct ufshcd_lrb *lrbp;
+ struct scsi_cmnd *cmd;
+
+ lrbp = &hba->lrb[task_tag];
+ lrbp->compl_time_stamp = ktime_get();
+ cmd = lrbp->cmd;
+ if (cmd) {
+ if (unlikely(ufshcd_should_inform_monitor(hba, lrbp)))
+ ufshcd_update_monitor(hba, lrbp);
+ ufshcd_add_command_trace(hba, lrbp, UFS_CMD_COMP);
+ cmd->result = ufshcd_transfer_rsp_status(hba, lrbp);
+ ufshcd_release_scsi_cmd(hba, lrbp);
+ /* Do not touch lrbp after scsi done */
+ scsi_done(cmd);
+ } else if (lrbp->command_type == UTP_CMD_TYPE_DEV_MANAGE ||
+ lrbp->command_type == UTP_CMD_TYPE_UFS_STORAGE) {
+ if (hba->dev_cmd.complete) {
+ ufshcd_add_command_trace(hba, lrbp, UFS_DEV_COMP);
+ complete(hba->dev_cmd.complete);
+ ufshcd_clk_scaling_update_busy(hba);
+ }
+ }
+}
+
/**
* __ufshcd_transfer_req_compl - handle SCSI and query command completion
* @hba: per adapter instance
@@ -5328,32 +5357,10 @@ static void ufshcd_release_scsi_cmd(struct ufs_hba *hba,
static void __ufshcd_transfer_req_compl(struct ufs_hba *hba,
unsigned long completed_reqs)
{
- struct ufshcd_lrb *lrbp;
- struct scsi_cmnd *cmd;
- int index;
-
- for_each_set_bit(index, &completed_reqs, hba->nutrs) {
- lrbp = &hba->lrb[index];
- lrbp->compl_time_stamp = ktime_get();
- cmd = lrbp->cmd;
- if (cmd) {
- if (unlikely(ufshcd_should_inform_monitor(hba, lrbp)))
- ufshcd_update_monitor(hba, lrbp);
- ufshcd_add_command_trace(hba, index, UFS_CMD_COMP);
- cmd->result = ufshcd_transfer_rsp_status(hba, lrbp);
- ufshcd_release_scsi_cmd(hba, lrbp);
- /* Do not touch lrbp after scsi done */
- scsi_done(cmd);
- } else if (lrbp->command_type == UTP_CMD_TYPE_DEV_MANAGE ||
- lrbp->command_type == UTP_CMD_TYPE_UFS_STORAGE) {
- if (hba->dev_cmd.complete) {
- ufshcd_add_command_trace(hba, index,
- UFS_DEV_COMP);
- complete(hba->dev_cmd.complete);
- ufshcd_clk_scaling_update_busy(hba);
- }
- }
- }
+ int tag;
+
+ for_each_set_bit(tag, &completed_reqs, hba->nutrs)
+ ufshcd_compl_one_cqe(hba, tag);
}

/*
@@ -6869,7 +6876,7 @@ static int ufshcd_issue_devman_upiu_cmd(struct ufs_hba *hba,

ufshcd_add_query_upiu_trace(hba, UFS_QUERY_SEND, lrbp->ucd_req_ptr);

- ufshcd_send_command(hba, tag);
+ ufshcd_send_command(hba, lrbp);
/*
* ignore the returning value here - ufshcd_check_query_response is
* bound to fail since dev_cmd.query and dev_cmd.type were left empty.
@@ -8138,11 +8145,11 @@ static int ufshcd_add_lus(struct ufs_hba *hba)
/**
* ufshcd_probe_hba - probe hba to detect device and initialize it
* @hba: per-adapter instance
- * @init_dev_params: whether or not to call ufshcd_device_params_init().
+ * @initial_boot: Whether or not from initial bootup
*
* Execute link-startup and verify device initialization
*/
-static int ufshcd_probe_hba(struct ufs_hba *hba, bool init_dev_params)
+static int ufshcd_probe_hba(struct ufs_hba *hba, bool initial_boot)
{
int ret;
unsigned long flags;
@@ -8177,7 +8184,7 @@ static int ufshcd_probe_hba(struct ufs_hba *hba, bool init_dev_params)
* Initialize UFS device parameters used by driver, these
* parameters are associated with UFS descriptors.
*/
- if (init_dev_params) {
+ if (initial_boot) {
ret = ufshcd_device_params_init(hba);
if (ret)
goto out;
--
2.7.4


2022-09-05 12:45:46

by Bean Huo

[permalink] [raw]
Subject: Re: [RFC PATCH v3 1/4] ufs: core: prepare ufs for multi circular queue support

Asutosh,


On Fri, 2022-09-02 at 15:41 -0700, Asutosh Das wrote:
> Preparatory changes for upcoming multi circular queue.
>
> Co-developed-by: Can Guo <[email protected]>
> Signed-off-by: Can Guo <[email protected]>
> Signed-off-by: Asutosh Das <[email protected]>
> ---
>  drivers/ufs/core/ufshcd.c | 99 +++++++++++++++++++++++++------------
> ----------
>  1 file changed, 53 insertions(+), 46 deletions(-)
>
>
>  
> @@ -2134,14 +2135,14 @@ static void ufshcd_update_monitor(struct
> ufs_hba *hba, const struct ufshcd_lrb *
>   * @task_tag: Task tag of the command
>   */

You didn't change parameter name in the function description.

>  static inline
> -void ufshcd_send_command(struct ufs_hba *hba, unsigned int task_tag)
> +void ufshcd_send_command(struct ufs_hba *hba, struct ufshcd_lrb
> *lrbp)
>  {
> -       struct ufshcd_lrb *lrbp = &hba->lrb[task_tag];
> +       int task_tag = lrbp->task_tag;
>         unsigned long flags;
>  
>         lrbp->issue_time_stamp = ktime_get();
>         lrbp->compl_time_stamp = ktime_set(0, 0);
> -       ufshcd_add_command_trace(hba, task_tag, UFS_CMD_SEND);
> +       ufshcd_add_command_trace(hba, lrbp, UFS_CMD_SEND);
>         ufshcd_clk_scaling_start_busy(hba);
>         if (unlikely(ufshcd_should_inform_monitor(hba, lrbp)))
>                 ufshcd_start_monitor(hba, lrbp);
> @@ -2553,9 +2554,10 @@ void ufshcd_prepare_utp_scsi_cmd_upiu(struct
> ufshcd_lrb *lrbp, u8 upiu_flags)
>         /* command descriptor fields */
>         ucd_req_ptr->header.dword_0 = UPIU_HEADER_DWORD(
>                                 UPIU_TRANSACTION_COMMAND, upiu_flags,
> -                               lrbp->lun, lrbp->task_tag);
> +                               lrbp->lun, lrbp->task_tag & 0xff);
>         ucd_req_ptr->header.dword_1 = UPIU_HEADER_DWORD(
> -                               UPIU_COMMAND_SET_TYPE_SCSI, 0, 0, 0);
> +                               UPIU_COMMAND_SET_TYPE_SCSI, 0, 0,
> +                               (lrbp->task_tag & 0xf00) << 4);
>  

Are you sure here "(lrbp->task_tag & 0xf00) << 4" is correct?


this will overwrite other fields, see UPIU_HEADER_DWORD:

#define UPIU_HEADER_DWORD(byte3, byte2, byte1, byte0)\
cpu_to_be32((byte3 << 24) | (byte2 << 16) |\
(byte1 << 8) | (byte0))



>
>  
> -       ufshcd_send_command(hba, tag);
> +       ufshcd_send_command(hba, lrbp);
>         err = ufshcd_wait_for_dev_cmd(hba, lrbp, timeout);
>         ufshcd_add_query_upiu_trace(hba, err ? UFS_QUERY_ERR :
> UFS_QUERY_COMP,
>                                     (struct utp_upiu_req *)lrbp-
> >ucd_rsp_ptr);
> @@ -4513,6 +4515,7 @@ int ufshcd_make_hba_operational(struct ufs_hba
> *hba)
>                         REG_UTP_TRANSFER_REQ_LIST_BASE_L);
>         ufshcd_writel(hba, upper_32_bits(hba->utrdl_dma_addr),
>                         REG_UTP_TRANSFER_REQ_LIST_BASE_H);
> +
>         ufshcd_writel(hba, lower_32_bits(hba->utmrdl_dma_addr),
>                         REG_UTP_TASK_REQ_LIST_BASE_L);
>         ufshcd_writel(hba, upper_32_bits(hba->utmrdl_dma_addr),
> @@ -5320,6 +5323,32 @@ static void ufshcd_release_scsi_cmd(struct
> ufs_hba *hba,
>         ufshcd_clk_scaling_update_busy(hba);
>  }
>  
> +void ufshcd_compl_one_cqe(struct ufs_hba *hba, int task_tag)

This function does only complete one task. What does cqe stand for?

> +{
> +       struct ufshcd_lrb *lrbp;
> +       struct scsi_cmnd *cmd;
> +
> +       lrbp = &hba->lrb[task_tag];
> +       lrbp->compl_time_stamp = ktime_get();
> +       cmd = lrbp->cmd;
> +       if (cmd) {
> +               if (unlikely(ufshcd_should_inform_monitor(hba,
> lrbp)))
> +                       ufshcd_update_monitor(hba, lrbp);
> +               ufshcd_add_command_trace(hba, lrbp, UFS_CMD_COMP);
> +               cmd->result = ufshcd_transfer_rsp_status(hba, lrbp);
> +               ufshcd_release_scsi_cmd(hba, lrbp);
> +               /* Do not touch lrbp after scsi done */
> +               scsi_done(cmd);
> +       } else if (lrbp->command_type == UTP_CMD_TYPE_DEV_MANAGE ||
> +                  lrbp->command_type == UTP_CMD_TYPE_UFS_STORAGE) {
> +               if (hba->dev_cmd.complete) {
> +                       ufshcd_add_command_trace(hba, lrbp,
> UFS_DEV_COMP);
> +                       complete(hba->dev_cmd.complete);
> +                       ufshcd_clk_scaling_update_busy(hba);
> +               }
> +       }
> +}
> +


Kind regards,
Bean

2022-09-08 03:06:00

by Asutosh Das

[permalink] [raw]
Subject: Re: [RFC PATCH v3 1/4] ufs: core: prepare ufs for multi circular queue support

Hello Bean
Thanks for your comments. Responses inline.

On 9/5/2022 5:26 AM, Bean Huo wrote:
> Asutosh,
>
>
[...]
>
>>  static inline
>> -void ufshcd_send_command(struct ufs_hba *hba, unsigned int task_tag)
>> +void ufshcd_send_command(struct ufs_hba *hba, struct ufshcd_lrb
>> *lrbp)
>>  {
>> -       struct ufshcd_lrb *lrbp = &hba->lrb[task_tag];
>> +       int task_tag = lrbp->task_tag;
>>         unsigned long flags;
>>
>>         lrbp->issue_time_stamp = ktime_get();
>>         lrbp->compl_time_stamp = ktime_set(0, 0);
>> -       ufshcd_add_command_trace(hba, task_tag, UFS_CMD_SEND);
>> +       ufshcd_add_command_trace(hba, lrbp, UFS_CMD_SEND);
>>         ufshcd_clk_scaling_start_busy(hba);
>>         if (unlikely(ufshcd_should_inform_monitor(hba, lrbp)))
>>                 ufshcd_start_monitor(hba, lrbp);
>> @@ -2553,9 +2554,10 @@ void ufshcd_prepare_utp_scsi_cmd_upiu(struct
>> ufshcd_lrb *lrbp, u8 upiu_flags)
>>         /* command descriptor fields */
>>         ucd_req_ptr->header.dword_0 = UPIU_HEADER_DWORD(
>>                                 UPIU_TRANSACTION_COMMAND, upiu_flags,
>> -                               lrbp->lun, lrbp->task_tag);
>> +                               lrbp->lun, lrbp->task_tag & 0xff);
>>         ucd_req_ptr->header.dword_1 = UPIU_HEADER_DWORD(
>> -                               UPIU_COMMAND_SET_TYPE_SCSI, 0, 0, 0);
>> +                               UPIU_COMMAND_SET_TYPE_SCSI, 0, 0,
>> +                               (lrbp->task_tag & 0xf00) << 4);
>>
>
> Are you sure here "(lrbp->task_tag & 0xf00) << 4" is correct?
>
EXT_IID is the higher nibble in DWORD1. So this looks correct to me.

COMMAND UPIU
0 1 2 3
xx000001b Flags LUN Task Tag
4 5 6 7
IID Command Set Type Reserved Reserved EXT_IID | Reserved
>
> this will overwrite other fields, see UPIU_HEADER_DWORD:
>
> #define UPIU_HEADER_DWORD(byte3, byte2, byte1, byte0)\
> cpu_to_be32((byte3 << 24) | (byte2 << 16) |\
> (byte1 << 8) | (byte0))
>
>
>
>>
>>
>> -       ufshcd_send_command(hba, tag);
>> +       ufshcd_send_command(hba, lrbp);
>>         err = ufshcd_wait_for_dev_cmd(hba, lrbp, timeout);
>>         ufshcd_add_query_upiu_trace(hba, err ? UFS_QUERY_ERR :
>> UFS_QUERY_COMP,
>>                                     (struct utp_upiu_req *)lrbp-
>>> ucd_rsp_ptr);
>> @@ -4513,6 +4515,7 @@ int ufshcd_make_hba_operational(struct ufs_hba
>> *hba)
>>                         REG_UTP_TRANSFER_REQ_LIST_BASE_L);
>>         ufshcd_writel(hba, upper_32_bits(hba->utrdl_dma_addr),
>>                         REG_UTP_TRANSFER_REQ_LIST_BASE_H);
>> +
>>         ufshcd_writel(hba, lower_32_bits(hba->utmrdl_dma_addr),
>>                         REG_UTP_TASK_REQ_LIST_BASE_L);
>>         ufshcd_writel(hba, upper_32_bits(hba->utmrdl_dma_addr),
>> @@ -5320,6 +5323,32 @@ static void ufshcd_release_scsi_cmd(struct
>> ufs_hba *hba,
>>         ufshcd_clk_scaling_update_busy(hba);
>>  }
>>
>> +void ufshcd_compl_one_cqe(struct ufs_hba *hba, int task_tag)
>
> This function does only complete one task. What does cqe stand for?
>
CQE - Completion Queue Entry. The term 'task' is used for TM commands in
scsi, hence there was a comment to change it to CQE.

>> +{
>> +       struct ufshcd_lrb *lrbp;
>> +       struct scsi_cmnd *cmd;
>> +
>> +       lrbp = &hba->lrb[task_tag];
>> +       lrbp->compl_time_stamp = ktime_get();
>> +       cmd = lrbp->cmd;
>> +       if (cmd) {
>> +               if (unlikely(ufshcd_should_inform_monitor(hba,
>> lrbp)))
>> +                       ufshcd_update_monitor(hba, lrbp);
>> +               ufshcd_add_command_trace(hba, lrbp, UFS_CMD_COMP);
>> +               cmd->result = ufshcd_transfer_rsp_status(hba, lrbp);
>> +               ufshcd_release_scsi_cmd(hba, lrbp);
>> +               /* Do not touch lrbp after scsi done */
>> +               scsi_done(cmd);
>> +       } else if (lrbp->command_type == UTP_CMD_TYPE_DEV_MANAGE ||
>> +                  lrbp->command_type == UTP_CMD_TYPE_UFS_STORAGE) {
>> +               if (hba->dev_cmd.complete) {
>> +                       ufshcd_add_command_trace(hba, lrbp,
>> UFS_DEV_COMP);
>> +                       complete(hba->dev_cmd.complete);
>> +                       ufshcd_clk_scaling_update_busy(hba);
>> +               }
>> +       }
>> +}
>> +
>
>
> Kind regards,
> Bean
>

2022-09-08 09:37:22

by Bean Huo

[permalink] [raw]
Subject: Re: [RFC PATCH v3 1/4] ufs: core: prepare ufs for multi circular queue support

On Wed, 2022-09-07 at 20:00 -0700, Asutosh Das (asd) wrote:
> > > ufshcd_lrb *lrbp, u8 upiu_flags)
> > >           /* command descriptor fields */
> > >           ucd_req_ptr->header.dword_0 = UPIU_HEADER_DWORD(
> > >                                   UPIU_TRANSACTION_COMMAND,
> > > upiu_flags,
> > > -                               lrbp->lun, lrbp->task_tag);
> > > +                               lrbp->lun, lrbp->task_tag &
> > > 0xff);
> > >           ucd_req_ptr->header.dword_1 = UPIU_HEADER_DWORD(
> > > -                               UPIU_COMMAND_SET_TYPE_SCSI, 0, 0,
> > > 0);
> > > +                               UPIU_COMMAND_SET_TYPE_SCSI, 0, 0,
> > > +                               (lrbp->task_tag & 0xf00) << 4);
> > >   
> >
> > Are you sure here "(lrbp->task_tag & 0xf00) << 4" is correct?
> >
> EXT_IID is the higher nibble in DWORD1. So this looks correct to me.
>
> COMMAND UPIU
> 0           1     2     3
> xx000001b Flags LUN Task Tag
> 4                       5       6       7
> IID Command Set Type Reserved Reserved EXT_IID | Reserved

Hi Asutosh,

yes, [7:4] of byte 7 in UPIU header for host to device UPIUs EXT_IID,
this is correct.

but I think byte7 should be (lrbp->task_tag & 0xf00) >> 4); rather
than "<< 4"

or what I missundersood.


Kind regards,
Bean


2022-09-08 22:32:10

by Bart Van Assche

[permalink] [raw]
Subject: Re: [RFC PATCH v3 1/4] ufs: core: prepare ufs for multi circular queue support

On 9/2/22 15:41, Asutosh Das wrote:
> Preparatory changes for upcoming multi circular queue.

One patch per change please and also describe each individual change.
From
https://www.kernel.org/doc/html/latest/process/submitting-patches.html#split-changes:
"Separate each logical change into a separate patch".

Thanks,

Bart.

2022-09-09 22:58:00

by Asutosh Das

[permalink] [raw]
Subject: Re: [RFC PATCH v3 1/4] ufs: core: prepare ufs for multi circular queue support

Hello Bart,
Thanks for the comments.

On 9/8/2022 2:58 PM, Bart Van Assche wrote:
> On 9/2/22 15:41, Asutosh Das wrote:
>> Preparatory changes for upcoming multi circular queue.
>
> One patch per change please and also describe each individual change.
> From
> https://www.kernel.org/doc/html/latest/process/submitting-patches.html#split-changes: "Separate each logical change into a separate patch".
>
The intent of this change was to have all the non-mcq related changes to
the ufshcd as a separate patch.
I would add more details to the commit message of this change.
If there's anything specific in this patch that may need changes, please
let me know.

> Thanks,
>
> Bart.

-asd

2022-09-09 23:46:12

by Bart Van Assche

[permalink] [raw]
Subject: Re: [RFC PATCH v3 1/4] ufs: core: prepare ufs for multi circular queue support

On 9/9/22 15:48, Asutosh Das (asd) wrote:
> Hello Bart,
> Thanks for the comments.
>
> On 9/8/2022 2:58 PM, Bart Van Assche wrote:
>> On 9/2/22 15:41, Asutosh Das wrote:
>>> Preparatory changes for upcoming multi circular queue.
>>
>> One patch per change please and also describe each individual change.
>>  From
>> https://www.kernel.org/doc/html/latest/process/submitting-patches.html#split-changes:
>> "Separate each logical change into a separate patch".
>>
> The intent of this change was to have all the non-mcq related changes to
> the ufshcd as a separate patch.
> I would add more details to the commit message of this change.
> If there's anything specific in this patch that may need changes, please
> let me know.

Please follow the "one change per patch" rule. This is a widely followed
rule in the Linux kernel community. This rule exists because it is the
responsibility of the developer(s) who post a patch series to make it
easy for reviewers to review their work.

Thanks,

Bart.

2022-09-12 18:54:47

by Asutosh Das

[permalink] [raw]
Subject: Re: [RFC PATCH v3 1/4] ufs: core: prepare ufs for multi circular queue support

On 9/9/2022 4:38 PM, Bart Van Assche wrote:
> On 9/9/22 15:48, Asutosh Das (asd) wrote:
>> Hello Bart,
>> Thanks for the comments.
>>
>> On 9/8/2022 2:58 PM, Bart Van Assche wrote:
>>> On 9/2/22 15:41, Asutosh Das wrote:
>>>> Preparatory changes for upcoming multi circular queue.
>>>
>>> One patch per change please and also describe each individual change.
>>>  From
>>> https://www.kernel.org/doc/html/latest/process/submitting-patches.html#split-changes: "Separate each logical change into a separate patch".
>>>
>> The intent of this change was to have all the non-mcq related changes
>> to the ufshcd as a separate patch.
>> I would add more details to the commit message of this change.
>> If there's anything specific in this patch that may need changes,
>> please let me know.
>
> Please follow the "one change per patch" rule. This is a widely followed
> rule in the Linux kernel community. This rule exists because it is the
> responsibility of the developer(s) who post a patch series to make it
> easy for reviewers to review their work.
>
Let me review and restructure the code and push it again.
I would like to remove the RFC tag in the next version.
Please let me know if that's _not_ OK.

> Thanks,
>
> Bart.