diff mbox series

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

Message ID 20240827122342.3314173-4-avri.altman@wdc.com
State New
Headers show
Series Add SDUC Support | expand

Commit Message

Avri Altman Aug. 27, 2024, 12:23 p.m. UTC 
For open-ended read/write - just send CMD22 before issuing the command.
While at it, make sure that the rw command arg is properly casting the
lower 32 bits, as it can be larger now.

Tested-by: Ricky WU <ricky_wu@realtek.com>
Signed-off-by: Avri Altman <avri.altman@wdc.com>
---
 drivers/mmc/core/block.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Comments

Adrian Hunter Sept. 3, 2024, 4:56 a.m. UTC | #1 
On 27/08/24 15:23, Avri Altman wrote:
> For open-ended read/write - just send CMD22 before issuing the command.
> While at it, make sure that the rw command arg is properly casting the
> lower 32 bits, as it can be larger now.
> 
> Tested-by: Ricky WU <ricky_wu@realtek.com>
> Signed-off-by: Avri Altman <avri.altman@wdc.com>
> ---
>  drivers/mmc/core/block.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
> index 2c9963248fcb..8816b3f0a312 100644
> --- a/drivers/mmc/core/block.c
> +++ b/drivers/mmc/core/block.c
> @@ -180,6 +180,7 @@ static void mmc_blk_rw_rq_prep(struct mmc_queue_req *mqrq,
>  static void mmc_blk_hsq_req_done(struct mmc_request *mrq);
>  static int mmc_spi_err_check(struct mmc_card *card);
>  static int mmc_blk_busy_cb(void *cb_data, bool *busy);
> +static int mmc_blk_wait_for_idle(struct mmc_queue *mq, struct mmc_host *host);
>  
>  static struct mmc_blk_data *mmc_blk_get(struct gendisk *disk)
>  {
> @@ -1664,7 +1665,7 @@ static void mmc_blk_rw_rq_prep(struct mmc_queue_req *mqrq,
>  
>  	brq->mrq.cmd = &brq->cmd;
>  
> -	brq->cmd.arg = blk_rq_pos(req);
> +	brq->cmd.arg = blk_rq_pos(req) & 0xFFFFFFFF;
>  	if (!mmc_card_blockaddr(card))
>  		brq->cmd.arg <<= 9;
>  	brq->cmd.flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 | MMC_CMD_ADTC;
> @@ -1712,6 +1713,9 @@ static void mmc_blk_rw_rq_prep(struct mmc_queue_req *mqrq,
>  			(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);
+
 	init_completion(&mrq->cmd_completion);
 
 	mmc_retune_hold(host);

>  	}
>  }
>
Avri Altman Sept. 3, 2024, 8 a.m. UTC | #2 
> On 27/08/24 15:23, Avri Altman wrote:
> > For open-ended read/write - just send CMD22 before issuing the command.
> > While at it, make sure that the rw command arg is properly casting the
> > lower 32 bits, as it can be larger now.
> >
> > Tested-by: Ricky WU <ricky_wu@realtek.com>
> > Signed-off-by: Avri Altman <avri.altman@wdc.com>
> > ---
> >  drivers/mmc/core/block.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c index
> > 2c9963248fcb..8816b3f0a312 100644
> > --- a/drivers/mmc/core/block.c
> > +++ b/drivers/mmc/core/block.c
> > @@ -180,6 +180,7 @@ static void mmc_blk_rw_rq_prep(struct
> > mmc_queue_req *mqrq,  static void mmc_blk_hsq_req_done(struct
> > mmc_request *mrq);  static int mmc_spi_err_check(struct mmc_card
> > *card);  static int mmc_blk_busy_cb(void *cb_data, bool *busy);
> > +static int mmc_blk_wait_for_idle(struct mmc_queue *mq, struct
> > +mmc_host *host);
> >
> >  static struct mmc_blk_data *mmc_blk_get(struct gendisk *disk)  { @@
> > -1664,7 +1665,7 @@ static void mmc_blk_rw_rq_prep(struct mmc_queue_req
> > *mqrq,
> >
> >       brq->mrq.cmd = &brq->cmd;
> >
> > -     brq->cmd.arg = blk_rq_pos(req);
> > +     brq->cmd.arg = blk_rq_pos(req) & 0xFFFFFFFF;
> >       if (!mmc_card_blockaddr(card))
> >               brq->cmd.arg <<= 9;
> >       brq->cmd.flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 | MMC_CMD_ADTC;
> @@
> > -1712,6 +1713,9 @@ static void mmc_blk_rw_rq_prep(struct mmc_queue_req
> *mqrq,
> >                       (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);
> +
Will give it a try.

Thanks,
Avri

>         init_completion(&mrq->cmd_completion);
> 
>         mmc_retune_hold(host);
> 
> >       }
> >  }
> >
Avri Altman Sept. 4, 2024, 8:51 a.m. UTC | #3 
> > On 27/08/24 15:23, Avri Altman wrote:
> > > For open-ended read/write - just send CMD22 before issuing the command.
> > > While at it, make sure that the rw command arg is properly casting
> > > the lower 32 bits, as it can be larger now.
> > >
> > > Tested-by: Ricky WU <ricky_wu@realtek.com>
> > > Signed-off-by: Avri Altman <avri.altman@wdc.com>
> > > ---
> > >  drivers/mmc/core/block.c | 6 +++++-
> > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
> > > index
> > > 2c9963248fcb..8816b3f0a312 100644
> > > --- a/drivers/mmc/core/block.c
> > > +++ b/drivers/mmc/core/block.c
> > > @@ -180,6 +180,7 @@ static void mmc_blk_rw_rq_prep(struct
> > > mmc_queue_req *mqrq,  static void mmc_blk_hsq_req_done(struct
> > > mmc_request *mrq);  static int mmc_spi_err_check(struct mmc_card
> > > *card);  static int mmc_blk_busy_cb(void *cb_data, bool *busy);
> > > +static int mmc_blk_wait_for_idle(struct mmc_queue *mq, struct
> > > +mmc_host *host);
> > >
> > >  static struct mmc_blk_data *mmc_blk_get(struct gendisk *disk)  { @@
> > > -1664,7 +1665,7 @@ static void mmc_blk_rw_rq_prep(struct
> > > mmc_queue_req *mqrq,
> > >
> > >       brq->mrq.cmd = &brq->cmd;
> > >
> > > -     brq->cmd.arg = blk_rq_pos(req);
> > > +     brq->cmd.arg = blk_rq_pos(req) & 0xFFFFFFFF;
> > >       if (!mmc_card_blockaddr(card))
> > >               brq->cmd.arg <<= 9;
> > >       brq->cmd.flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 | MMC_CMD_ADTC;
> > @@
> > > -1712,6 +1713,9 @@ static void mmc_blk_rw_rq_prep(struct
> > > mmc_queue_req
> > *mqrq,
> > >                       (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);
> > +
> 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.:
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.

And there is that thing of adding dword to mmc_command.

What do you think?

Thanks,
Avri
> 
> Thanks,
> Avri
> 
> >         init_completion(&mrq->cmd_completion);
> >
> >         mmc_retune_hold(host);
> >
> > >       }
> > >  }
> > >
Avri Altman Sept. 4, 2024, 9:55 a.m. UTC | #4 
> > > >                       (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.:
> 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.
> 
> And there is that thing of adding dword to mmc_command.
> 
> What do you think?
> 
> Thanks,
> Avri
> >
> > Thanks,
> > Avri
> >
> > >         init_completion(&mrq->cmd_completion);
> > >
> > >         mmc_retune_hold(host);
> > >
> > > >       }
> > > >  }
> > > >
Adrian Hunter Sept. 4, 2024, 11:11 a.m. UTC | #5 
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 */
 	int			error;		/* command error */
 

>>
>> Thanks,
>> Avri
>>>
>>> Thanks,
>>> Avri
>>>
>>>>         init_completion(&mrq->cmd_completion);
>>>>
>>>>         mmc_retune_hold(host);
>>>>
>>>>>       }
>>>>>  }
>>>>>
>
Adrian Hunter Sept. 4, 2024, 11:17 a.m. UTC | #6 
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);
>>>>>
>>>>>>       }
>>>>>>  }
>>>>>>
>>
>
diff mbox series

Patch

diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
index 2c9963248fcb..8816b3f0a312 100644
--- a/drivers/mmc/core/block.c
+++ b/drivers/mmc/core/block.c
@@ -180,6 +180,7 @@  static void mmc_blk_rw_rq_prep(struct mmc_queue_req *mqrq,
 static void mmc_blk_hsq_req_done(struct mmc_request *mrq);
 static int mmc_spi_err_check(struct mmc_card *card);
 static int mmc_blk_busy_cb(void *cb_data, bool *busy);
+static int mmc_blk_wait_for_idle(struct mmc_queue *mq, struct mmc_host *host);
 
 static struct mmc_blk_data *mmc_blk_get(struct gendisk *disk)
 {
@@ -1664,7 +1665,7 @@  static void mmc_blk_rw_rq_prep(struct mmc_queue_req *mqrq,
 
 	brq->mrq.cmd = &brq->cmd;
 
-	brq->cmd.arg = blk_rq_pos(req);
+	brq->cmd.arg = blk_rq_pos(req) & 0xFFFFFFFF;
 	if (!mmc_card_blockaddr(card))
 		brq->cmd.arg <<= 9;
 	brq->cmd.flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 | MMC_CMD_ADTC;
@@ -1712,6 +1713,9 @@  static void mmc_blk_rw_rq_prep(struct mmc_queue_req *mqrq,
 			(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));
 	}
 }