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

[Adrian Hunter]

On 27/01/24 10:28, Victor Shih wrote:
> On Tue, Jan 23, 2024 at 2:28 PM Victor Shih <victorshihgli@xxxxxxxxx> wrote:
>>
>> From: Victor Shih <victor.shih@xxxxxxxxxxxxxxxxxxx>
>>
>> Embed UHS-II access/control functionality into the MMC request
>> processing flow.
>>
>> Signed-off-by: Ulf Hansson <ulf.hansson@xxxxxxxxxx>
>> Signed-off-by: Jason Lai <jason.lai@xxxxxxxxxxxxxxxxxxx>
>> Signed-off-by: Victor Shih <victor.shih@xxxxxxxxxxxxxxxxxxx>
>> ---
>>
>> 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@xxxxxxxxx/
> 
> 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;