Re: [PATCH v5 3/9] mmc: core: Add open-ended Ext memory addressing

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

On 4/09/24 14:11, Adrian Hunter wrote:
> On 4/09/24 12:55, Avri Altman wrote:
>>>>>>                       (do_data_tag ? (1 << 29) : 0);
>>>>>>               brq->sbc.flags = MMC_RSP_R1 | MMC_CMD_AC;
>>>>>>               brq->mrq.sbc = &brq->sbc;
>>>>>> +     } else if (mmc_card_ult_capacity(card)) {
>>>>>> +             mmc_blk_wait_for_idle(mq, card->host);
>>>>>> +             mmc_send_ext_addr(card->host, blk_rq_pos(req));
>>>>>
>>>>> Did you consider having mmc_start_request() send CMD22?
>>>>> e.g.
>>>>>
>>>>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c index
>>>>> d6c819dd68ed..22677a01c0e3 100644
>>>>> --- a/drivers/mmc/core/core.c
>>>>> +++ b/drivers/mmc/core/core.c
>>>>> @@ -336,6 +336,9 @@ int mmc_start_request(struct mmc_host *host,
>>>>> struct mmc_request *mrq)  {
>>>>>         int err;
>>>>>
>>>>> +       if (mrq->cmd && mrq->cmd->ext_addr)
>>>>> +               mmc_send_ext_addr(card->host, mrq->cmd->ext_addr);
>> Oh , and yes - To state the obvious: async reqs are working fine now.
>> Not sure why it wasn't when I was calling it from mmc_blk_rw_rq_prep.
>>
>> Thanks,
>> Avri
>>
>>>>> +
>>>> Will give it a try.
>>> In the proposed form, this doesn't work.
>>> Mainly because mmc_send_ext_addr eventually calls mmc_start_request by
>>> itself:
>>> mmc_wait_for_cmd() -> mmc_wait_for_req() -> __mmc_start_req() ->
>>> mmc_start_request().
>>>
>>> Also, since need to call CMD22 for any address, it is ok mrq->cmd->ext_addr to be
>>> 0x0, Then need it to be a little bit bulkier,  e.g.:
> 
> Sorry about that
> 
>>> by adding a "check_sduc" argument to mmc_start_request, if (mrq->cmd &&
>>> check_sduc)) And make it true in mmc_blk_mq_issue_rw_rq and
>>> mmc_blk_read_single, false otherwise, But this seems to be an invalid option to
>>> me.
> 
> Probably better to put has_ext_addr flag into mmc_command.
> 
>>>
>>> And there is that thing of adding dword to mmc_command.
>>>
>>> What do you think?
> 
> Adding to mmc_command should be OK.  If you want to save space,
> it looks like 'flags' has many unused bits.
> 
> diff --git a/include/linux/mmc/core.h b/include/linux/mmc/core.h
> index f0ac2e469b32..ceb521e3598f 100644
> --- a/include/linux/mmc/core.h
> +++ b/include/linux/mmc/core.h
> @@ -17,7 +17,7 @@ struct mmc_command {
>  #define MMC_CMD23_ARG_REL_WR	(1 << 31)
>  #define MMC_CMD23_ARG_TAG_REQ	(1 << 29)
>  	u32			resp[4];
> -	unsigned int		flags;		/* expected response type */
> +	u16			flags;		/* expected response type */
>  #define MMC_RSP_PRESENT	(1 << 0)
>  #define MMC_RSP_136	(1 << 1)		/* 136 bit response */
>  #define MMC_RSP_CRC	(1 << 2)		/* expect valid crc */
> @@ -76,6 +76,10 @@ struct mmc_command {
>   */
>  #define mmc_cmd_type(cmd)	((cmd)->flags & MMC_CMD_MASK)
>  
> +/* For SDUC */
> +	u8			has_ext_addr;
> +	u8			ext_addr;
> +
>  	unsigned int		retries;	/* max number of retries */

Although retries is also way bigger than needed.  However, maybe start
by just making mmc_command bigger and see if anyone complains.

>  	int			error;		/* command error */
>  
> 
>>>
>>> Thanks,
>>> Avri
>>>>
>>>> Thanks,
>>>> Avri
>>>>
>>>>>         init_completion(&mrq->cmd_completion);
>>>>>
>>>>>         mmc_retune_hold(host);
>>>>>
>>>>>>       }
>>>>>>  }
>>>>>>
>>
>