Re: [PATCH V14 07/21] mmc: core: Support UHS-II card control and access

[Adrian Hunter <adrian.hunter@intel.com>]

On 22/05/24 12:04, Victor Shih wrote:
> On Tue, Jan 30, 2024 at 5:36 PM Adrian Hunter <adrian.hunter@intel.com> wrote:
>>
>> On 27/01/24 10:28, Victor Shih wrote:
>>> On Tue, Jan 23, 2024 at 2:28 PM Victor Shih <victorshihgli@gmail.com> wrote:
>>>>
>>>> From: Victor Shih <victor.shih@genesyslogic.com.tw>
>>>>
>>>> Embed UHS-II access/control functionality into the MMC request
>>>> processing flow.
>>>>
>>>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>>>> Signed-off-by: Jason Lai <jason.lai@genesyslogic.com.tw>
>>>> Signed-off-by: Victor Shih <victor.shih@genesyslogic.com.tw>
>>>> ---
>>>>
>>>> Updates in V13:
>>>>  - Separate __mmc_go_idle() into one patch for re-factorring the code.
>>>>  - Move mmc_decode_scr declaration to sd.h.
>>>>  - Ues uhs2_sd_tran to stead MMC_UHS2_SD_TRAN.
>>>>  - Drop unnecessary comment.
>>>>
>>>> Updates in V12:
>>>>  - Use mmc_op_multi() to check DCMD which supports multi read/write
>>>>    in mmc_uhs2_prepare_cmd().
>>>>
>>>> Updates in V10:
>>>>  - Move some definitions of PatchV9[02/23] to PatchV10[06/23].
>>>>  - Move some definitions of PatchV9[05/23] to PatchV10[06/23].
>>>>  - Drop do_multi in the mmc_blk_rw_rq_prep().
>>>>  - Use tmode_half_duplex to instead of uhs2_tmode0_flag.
>>>>  - Move entire control of the tmode into mmc_uhs2_prepare_cmd().
>>>>
>>>> Updates in V8:
>>>>  - Add MMC_UHS2_SUPPORT to be cleared in sd_uhs2_detect().
>>>>  - Modify return value in sd_uhs2_attach().
>>>>
>>>> Updates in V7:
>>>>  - Add mmc_uhs2_card_prepare_cmd helper function in sd_ops.h.
>>>>  - Drop uhs2_state in favor of ios->timing.
>>>>  - Remove unnecessary functions.
>>>>
>>>> ---
>>>>
>>>>  drivers/mmc/core/core.c    |   10 +-
>>>>  drivers/mmc/core/sd.c      |   10 +-
>>>>  drivers/mmc/core/sd.h      |    5 +
>>>>  drivers/mmc/core/sd_ops.c  |    9 +
>>>>  drivers/mmc/core/sd_ops.h  |   17 +
>>>>  drivers/mmc/core/sd_uhs2.c | 1115 ++++++++++++++++++++++++++++++++++--
>>>>  include/linux/mmc/core.h   |   13 +
>>>>  include/linux/mmc/host.h   |   15 +
>>>>  8 files changed, 1155 insertions(+), 39 deletions(-)
>>>>
>>>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>>>> index 2edf31492a5d..be77cebe1fb8 100644
>>>> --- a/drivers/mmc/core/core.c
>>>> +++ b/drivers/mmc/core/core.c
>>>> @@ -334,6 +334,8 @@ static int mmc_mrq_prep(struct mmc_host *host, struct mmc_request *mrq)
>>>>
>>>>  int mmc_start_request(struct mmc_host *host, struct mmc_request *mrq)
>>>>  {
>>>> +       struct uhs2_command uhs2_cmd;
>>>> +       __be32 payload[4]; /* for maximum size */
>>>>         int err;
>>>>
>>>>         init_completion(&mrq->cmd_completion);
>>>> @@ -351,6 +353,8 @@ int mmc_start_request(struct mmc_host *host, struct mmc_request *mrq)
>>>>         if (err)
>>>>                 return err;
>>>>
>>>> +       mmc_uhs2_card_prepare_cmd(host, mrq, uhs2_cmd, payload);
>>>> +
>>>>         led_trigger_event(host->led, LED_FULL);
>>>>         __mmc_start_request(host, mrq);
>>>>
>>>
>>> Hi, Adrian
>>>
>>> I referenced your comments of the V9:
>>>
>>> Refer:
>>> https://patchwork.kernel.org/project/linux-mmc/patch/20230721101349.12387-7-victorshihgli@gmail.com/
>>>
>>> My understanding is as follows, please correct me if there are any mistakes.
>>> There is already "struct uhs2_command *uhs2_cmd" in struct mmc_command.
>>> If I also put "__be32 payload[4]" in struct mmc_command.
>>> The code will become:
>>> mmc_uhs2_card_prepare_cmd(host, mrq, *mrq->cmd->uhs2_cmd, mrq->cmd->payload);
>>> But a null pointer problem occurs when sending commands like COM0(mmc_go_idle).
>>> In this case I just can only plan for the time being as follows:
>>>
>>> if (mrq->cmd->uhs2_cmd)
>>>      mmc_uhs2_card_prepare_cmd(host, mrq, *mrq->cmd->uhs2_cmd,
>>> mrq->cmd->payload);
>>> else
>>>      mmc_uhs2_card_prepare_cmd(host, mrq, uhs2_cmd, payload);
>>>
>>> Would you give me any other advice?
>>
>> struct uhs2_command uhs2_cmd should not be on the stack local to
>> mmc_start_request().  Probably moving it to struct mmc_request
>> is as good as any other option.  So starting like below and
>> then with whatever other changes are needed to make it work.
>>
>>
>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>> index be77cebe1fb8..68496c51a521 100644
>> --- a/drivers/mmc/core/core.c
>> +++ b/drivers/mmc/core/core.c
>> @@ -334,8 +334,6 @@ static int mmc_mrq_prep(struct mmc_host *host, struct mmc_request *mrq)
>>
>>  int mmc_start_request(struct mmc_host *host, struct mmc_request *mrq)
>>  {
>> -       struct uhs2_command uhs2_cmd;
>> -       __be32 payload[4]; /* for maximum size */
>>         int err;
>>
>>         init_completion(&mrq->cmd_completion);
>> @@ -353,7 +351,7 @@ int mmc_start_request(struct mmc_host *host, struct mmc_request *mrq)
>>         if (err)
>>                 return err;
>>
>> -       mmc_uhs2_card_prepare_cmd(host, mrq, uhs2_cmd, payload);
>> +       mmc_uhs2_card_prepare_cmd(host, mrq);
>>
>>         led_trigger_event(host->led, LED_FULL);
>>         __mmc_start_request(host, mrq);
>> @@ -434,8 +432,6 @@ EXPORT_SYMBOL(mmc_wait_for_req_done);
>>   */
>>  int mmc_cqe_start_req(struct mmc_host *host, struct mmc_request *mrq)
>>  {
>> -       struct uhs2_command uhs2_cmd;
>> -       __be32 payload[4]; /* for maximum size */
>>         int err;
>>
>>         /*
>> @@ -456,7 +452,7 @@ int mmc_cqe_start_req(struct mmc_host *host, struct mmc_request *mrq)
>>         if (err)
>>                 goto out_err;
>>
>> -       mmc_uhs2_card_prepare_cmd(host, mrq, uhs2_cmd, payload);
>> +       mmc_uhs2_card_prepare_cmd(host, mrq);
>>
>>         err = host->cqe_ops->cqe_request(host, mrq);
>>         if (err)
>> diff --git a/drivers/mmc/core/sd_ops.h b/drivers/mmc/core/sd_ops.h
>> index d3a3465c7669..e3af68a52de8 100644
>> --- a/drivers/mmc/core/sd_ops.h
>> +++ b/drivers/mmc/core/sd_ops.h
>> @@ -24,14 +24,10 @@ int mmc_app_sd_status(struct mmc_card *card, void *ssr);
>>  int mmc_app_cmd(struct mmc_host *host, struct mmc_card *card);
>>  void mmc_uhs2_prepare_cmd(struct mmc_host *host, struct mmc_request *mrq);
>>
>> -static inline void mmc_uhs2_card_prepare_cmd(struct mmc_host *host, struct mmc_request *mrq,
>> -                                            struct uhs2_command uhs2_cmd, __be32 payload[4])
>> +static inline void mmc_uhs2_card_prepare_cmd(struct mmc_host *host, struct mmc_request *mrq)
>>  {
>> -       if (host->uhs2_sd_tran) {
>> -               uhs2_cmd.payload = payload;
>> -               mrq->cmd->uhs2_cmd = &uhs2_cmd;
>> +       if (host->uhs2_sd_tran)
>>                 mmc_uhs2_prepare_cmd(host, mrq);
>> -       }
>>  }
>>
>>  static inline int mmc_sd_can_poweroff_notify(struct mmc_card *card)
>> diff --git a/drivers/mmc/core/sd_uhs2.c b/drivers/mmc/core/sd_uhs2.c
>> index c46729d85644..9cabb6937dc1 100644
>> --- a/drivers/mmc/core/sd_uhs2.c
>> +++ b/drivers/mmc/core/sd_uhs2.c
>> @@ -1194,6 +1194,7 @@ void mmc_uhs2_prepare_cmd(struct mmc_host *host, struct mmc_request *mrq)
>>         u8 plen;
>>
>>         cmd = mrq->cmd;
>> +       cmd->uhs2_cmd = &mrq->uhs2_cmd;
>>         header = host->card->uhs2_config.node_id;
>>         if ((cmd->flags & MMC_CMD_MASK) == MMC_CMD_ADTC)
>>                 header |= UHS2_PACKET_TYPE_DCMD;
>> diff --git a/include/linux/mmc/core.h b/include/linux/mmc/core.h
>> index f30f6be86f66..83c901794c17 100644
>> --- a/include/linux/mmc/core.h
>> +++ b/include/linux/mmc/core.h
>> @@ -23,13 +23,18 @@ enum mmc_blk_status {
>>         MMC_BLK_NEW_REQUEST,
>>  };
>>
>> +#define UHS2_MAX_PAYLOAD_LEN 2
>> +#define UHS2_MAX_RESP_LEN 20
>> +
>>  struct uhs2_command {
>>         u16     header;
>>         u16     arg;
>> -       __be32  *payload;
>> -       u32     payload_len;
>> -       u32     packet_len;
>> +       __be32  payload[UHS2_MAX_PAYLOAD_LEN];
>> +       u8      payload_len;
>> +       u8      packet_len;     // TODO: is this really needed?
>>         u8      tmode_half_duplex;
>> +       u8      uhs2_resp_len;  /* UHS2 native cmd resp len */
>> +       u8      uhs2_resp[UHS2_MAX_RESP_LEN];   /* UHS2 native cmd resp */
>>  };
>>
>>  struct mmc_command {
>> @@ -119,8 +124,6 @@ struct mmc_command {
>>         struct mmc_request      *mrq;           /* associated request */
>>
>>         struct uhs2_command     *uhs2_cmd;      /* UHS2 command */
>> -       u8                      *uhs2_resp;     /* UHS2 native cmd resp */
>> -       u8                      uhs2_resp_len;  /* UHS2 native cmd resp len */
>>  };
>>
>>  struct mmc_data {
>> @@ -179,6 +182,7 @@ struct mmc_request {
>>         const struct bio_crypt_ctx *crypto_ctx;
>>         int                     crypto_key_slot;
>>  #endif
>> +       struct uhs2_command     uhs2_cmd;
>>  };
>>
>>  struct mmc_card;
>>
>>
>>
> 
> Hi, Adrian
> 
> I have modified the above into patch#7 and patch#16 in the patch series v15.
> But I will submit the patch series v16 later.
> Please help review the changes in patch series v16.
> 
> Thanks, Victor Shih

If you make a V16, please separate the Error Recovery mechanism added in V15
into a separate patch.