SD spec v5.1 adds discard support. The flows and commands matches those
in eMMC, Which leaves to set the appropriate discard arg in CMD38 if
DISCARD_SUPPORT (b313) is set in the SD_STATUS register.
We set this arg on card init: not in mmc_init_erase as one might expect
but arbitrarily once the card indicated its discard support. This is
because unlike erase, it doesn't really involve any logic, and we want
to avoid the unnecessary complication.
V1->v2:
In the first patch, assign the discard arg for SD cards as well to keep
the code consistent. Rename "discard_arg" to "erase_arg", and elaborate
the change log.
Avri Altman (3):
mmc: core: Calculate the discard arg only once
mmc: core: Indicate SD specs higher than 4.0
mmc: core: Add discard support to sd
drivers/mmc/core/block.c | 12 +++---------
drivers/mmc/core/core.c | 8 ++++++--
drivers/mmc/core/mmc.c | 8 ++++++++
drivers/mmc/core/sd.c | 15 +++++++++++++++
include/linux/mmc/card.h | 3 +++
include/linux/mmc/sd.h | 6 ++++++
6 files changed, 41 insertions(+), 11 deletions(-)
--
1.9.1
In MMC, the discard arg is a read-only ext_csd parameter - set it once
on card init. To be consistent, do that for SD as well even though its
discard arg is always 0x0.
Signed-off-by: Avri Altman <[email protected]>
---
drivers/mmc/core/block.c | 12 +++---------
drivers/mmc/core/core.c | 4 ++--
drivers/mmc/core/mmc.c | 8 ++++++++
drivers/mmc/core/sd.c | 2 ++
include/linux/mmc/card.h | 1 +
include/linux/mmc/sd.h | 5 +++++
6 files changed, 21 insertions(+), 11 deletions(-)
diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
index b08fb91..131e80e 100644
--- a/drivers/mmc/core/block.c
+++ b/drivers/mmc/core/block.c
@@ -1124,7 +1124,7 @@ static void mmc_blk_issue_discard_rq(struct mmc_queue *mq, struct request *req)
{
struct mmc_blk_data *md = mq->blkdata;
struct mmc_card *card = md->queue.card;
- unsigned int from, nr, arg;
+ unsigned int from, nr;
int err = 0, type = MMC_BLK_DISCARD;
blk_status_t status = BLK_STS_OK;
@@ -1136,24 +1136,18 @@ static void mmc_blk_issue_discard_rq(struct mmc_queue *mq, struct request *req)
from = blk_rq_pos(req);
nr = blk_rq_sectors(req);
- if (mmc_can_discard(card))
- arg = MMC_DISCARD_ARG;
- else if (mmc_can_trim(card))
- arg = MMC_TRIM_ARG;
- else
- arg = MMC_ERASE_ARG;
do {
err = 0;
if (card->quirks & MMC_QUIRK_INAND_CMD38) {
err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
INAND_CMD38_ARG_EXT_CSD,
- arg == MMC_TRIM_ARG ?
+ card->erase_arg == MMC_TRIM_ARG ?
INAND_CMD38_ARG_TRIM :
INAND_CMD38_ARG_ERASE,
0);
}
if (!err)
- err = mmc_erase(card, from, nr, arg);
+ err = mmc_erase(card, from, nr, card->erase_arg);
} while (err == -EIO && !mmc_blk_reset(md, card->host, type));
if (err)
status = BLK_STS_IOERR;
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 5bd58b9..de0f1a1 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -2164,7 +2164,7 @@ static unsigned int mmc_align_erase_size(struct mmc_card *card,
* @card: card to erase
* @from: first sector to erase
* @nr: number of sectors to erase
- * @arg: erase command argument (SD supports only %MMC_ERASE_ARG)
+ * @arg: erase command argument (SD supports only %SD_ERASE_ARG)
*
* Caller must claim host before calling this function.
*/
@@ -2181,7 +2181,7 @@ int mmc_erase(struct mmc_card *card, unsigned int from, unsigned int nr,
if (!card->erase_size)
return -EOPNOTSUPP;
- if (mmc_card_sd(card) && arg != MMC_ERASE_ARG)
+ if (mmc_card_sd(card) && arg != SD_ERASE_ARG)
return -EOPNOTSUPP;
if ((arg & MMC_SECURE_ARGS) &&
diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index da892a5..09c688f 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -1743,6 +1743,14 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr,
card->ext_csd.power_off_notification = EXT_CSD_POWER_ON;
}
+ /* set erase_arg */
+ if (mmc_can_discard(card))
+ card->erase_arg = MMC_DISCARD_ARG;
+ else if (mmc_can_trim(card))
+ card->erase_arg = MMC_TRIM_ARG;
+ else
+ card->erase_arg = MMC_ERASE_ARG;
+
/*
* Select timing interface
*/
diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
index d0d9f90..bd48b28 100644
--- a/drivers/mmc/core/sd.c
+++ b/drivers/mmc/core/sd.c
@@ -271,6 +271,8 @@ static int mmc_read_ssr(struct mmc_card *card)
}
}
+ card->erase_arg = SD_ERASE_ARG;
+
return 0;
}
diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
index de73778..8f429b6 100644
--- a/include/linux/mmc/card.h
+++ b/include/linux/mmc/card.h
@@ -277,6 +277,7 @@ struct mmc_card {
unsigned int erase_shift; /* if erase unit is power 2 */
unsigned int pref_erase; /* in sectors */
unsigned int eg_boundary; /* don't cross erase-group boundaries */
+ unsigned int erase_arg; /* erase / trim / discard */
u8 erased_byte; /* value of erased bytes */
u32 raw_cid[4]; /* raw card CID */
diff --git a/include/linux/mmc/sd.h b/include/linux/mmc/sd.h
index 1ebcf9b..1a6d10f 100644
--- a/include/linux/mmc/sd.h
+++ b/include/linux/mmc/sd.h
@@ -91,4 +91,9 @@
#define SD_SWITCH_ACCESS_DEF 0
#define SD_SWITCH_ACCESS_HS 1
+/*
+ * Erase/discard
+ */
+#define SD_ERASE_ARG 0x00000000
+
#endif /* LINUX_MMC_SD_H */
--
1.9.1
SD specs version 4.x and 5.x have a dedicated slices in the SCR register.
Higher versions will rely on a combination of the existing fields.
Signed-off-by: Avri Altman <[email protected]>
---
drivers/mmc/core/sd.c | 5 +++++
include/linux/mmc/card.h | 2 ++
2 files changed, 7 insertions(+)
diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
index bd48b28..c2db94d 100644
--- a/drivers/mmc/core/sd.c
+++ b/drivers/mmc/core/sd.c
@@ -209,6 +209,11 @@ static int mmc_decode_scr(struct mmc_card *card)
/* Check if Physical Layer Spec v3.0 is supported */
scr->sda_spec3 = UNSTUFF_BITS(resp, 47, 1);
+ if (scr->sda_spec3) {
+ scr->sda_spec4 = UNSTUFF_BITS(resp, 42, 1);
+ scr->sda_specx = UNSTUFF_BITS(resp, 38, 4);
+ }
+
if (UNSTUFF_BITS(resp, 55, 1))
card->erased_byte = 0xFF;
else
diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
index 8f429b6..d791813f 100644
--- a/include/linux/mmc/card.h
+++ b/include/linux/mmc/card.h
@@ -133,6 +133,8 @@ struct mmc_ext_csd {
struct sd_scr {
unsigned char sda_vsn;
unsigned char sda_spec3;
+ unsigned char sda_spec4;
+ unsigned char sda_specx;
unsigned char bus_widths;
#define SD_SCR_BUS_WIDTH_1 (1<<0)
#define SD_SCR_BUS_WIDTH_4 (1<<2)
--
1.9.1
SD spec v5.1 adds discard support. The flows and commands are similar to
mmc, so just set the discard arg in CMD38.
Actually, there is no need to check for the spec version like we are
doing, as it is assured that the reserved bits in earlier versions are
null. Do that anyway to document the spec version that introduce it.
Signed-off-by: Avri Altman <[email protected]>
---
drivers/mmc/core/core.c | 6 +++++-
drivers/mmc/core/sd.c | 10 +++++++++-
include/linux/mmc/sd.h | 1 +
3 files changed, 15 insertions(+), 2 deletions(-)
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index de0f1a1..4d62f28 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -2164,7 +2164,7 @@ static unsigned int mmc_align_erase_size(struct mmc_card *card,
* @card: card to erase
* @from: first sector to erase
* @nr: number of sectors to erase
- * @arg: erase command argument (SD supports only %SD_ERASE_ARG)
+ * @arg: erase command argument
*
* Caller must claim host before calling this function.
*/
@@ -2181,6 +2181,9 @@ int mmc_erase(struct mmc_card *card, unsigned int from, unsigned int nr,
if (!card->erase_size)
return -EOPNOTSUPP;
+ if (mmc_card_sd(card) && arg == SD_DISCARD_ARG)
+ goto skip_arg_testing;
+
if (mmc_card_sd(card) && arg != SD_ERASE_ARG)
return -EOPNOTSUPP;
@@ -2200,6 +2203,7 @@ int mmc_erase(struct mmc_card *card, unsigned int from, unsigned int nr,
if (arg == MMC_ERASE_ARG)
nr = mmc_align_erase_size(card, &from, &to, nr);
+skip_arg_testing:
if (nr == 0)
return 0;
diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
index c2db94d..2b4fc22 100644
--- a/drivers/mmc/core/sd.c
+++ b/drivers/mmc/core/sd.c
@@ -231,6 +231,8 @@ static int mmc_read_ssr(struct mmc_card *card)
{
unsigned int au, es, et, eo;
__be32 *raw_ssr;
+ u32 resp[4] = {};
+ u8 discard_support;
int i;
if (!(card->csd.cmdclass & CCC_APP_SPEC)) {
@@ -276,7 +278,13 @@ static int mmc_read_ssr(struct mmc_card *card)
}
}
- card->erase_arg = SD_ERASE_ARG;
+ /*
+ * starting SD5.1 discard is supported if DISCARD_SUPPORT (b313) is set
+ */
+ resp[3] = card->raw_ssr[6];
+ discard_support = UNSTUFF_BITS(resp, 313 - 288, 1);
+ card->erase_arg = (card->scr.sda_specx && discard_support) ?
+ SD_DISCARD_ARG : SD_ERASE_ARG;
return 0;
}
diff --git a/include/linux/mmc/sd.h b/include/linux/mmc/sd.h
index 1a6d10f..ec94a5a 100644
--- a/include/linux/mmc/sd.h
+++ b/include/linux/mmc/sd.h
@@ -95,5 +95,6 @@
* Erase/discard
*/
#define SD_ERASE_ARG 0x00000000
+#define SD_DISCARD_ARG 0x00000001
#endif /* LINUX_MMC_SD_H */
--
1.9.1
On Wed, 6 Feb 2019 at 12:28, Avri Altman <[email protected]> wrote:
>
> In MMC, the discard arg is a read-only ext_csd parameter - set it once
> on card init. To be consistent, do that for SD as well even though its
> discard arg is always 0x0.
>
> Signed-off-by: Avri Altman <[email protected]>
Applied for next, thanks!
Kind regards
Uffe
> ---
> drivers/mmc/core/block.c | 12 +++---------
> drivers/mmc/core/core.c | 4 ++--
> drivers/mmc/core/mmc.c | 8 ++++++++
> drivers/mmc/core/sd.c | 2 ++
> include/linux/mmc/card.h | 1 +
> include/linux/mmc/sd.h | 5 +++++
> 6 files changed, 21 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
> index b08fb91..131e80e 100644
> --- a/drivers/mmc/core/block.c
> +++ b/drivers/mmc/core/block.c
> @@ -1124,7 +1124,7 @@ static void mmc_blk_issue_discard_rq(struct mmc_queue *mq, struct request *req)
> {
> struct mmc_blk_data *md = mq->blkdata;
> struct mmc_card *card = md->queue.card;
> - unsigned int from, nr, arg;
> + unsigned int from, nr;
> int err = 0, type = MMC_BLK_DISCARD;
> blk_status_t status = BLK_STS_OK;
>
> @@ -1136,24 +1136,18 @@ static void mmc_blk_issue_discard_rq(struct mmc_queue *mq, struct request *req)
> from = blk_rq_pos(req);
> nr = blk_rq_sectors(req);
>
> - if (mmc_can_discard(card))
> - arg = MMC_DISCARD_ARG;
> - else if (mmc_can_trim(card))
> - arg = MMC_TRIM_ARG;
> - else
> - arg = MMC_ERASE_ARG;
> do {
> err = 0;
> if (card->quirks & MMC_QUIRK_INAND_CMD38) {
> err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
> INAND_CMD38_ARG_EXT_CSD,
> - arg == MMC_TRIM_ARG ?
> + card->erase_arg == MMC_TRIM_ARG ?
> INAND_CMD38_ARG_TRIM :
> INAND_CMD38_ARG_ERASE,
> 0);
> }
> if (!err)
> - err = mmc_erase(card, from, nr, arg);
> + err = mmc_erase(card, from, nr, card->erase_arg);
> } while (err == -EIO && !mmc_blk_reset(md, card->host, type));
> if (err)
> status = BLK_STS_IOERR;
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index 5bd58b9..de0f1a1 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -2164,7 +2164,7 @@ static unsigned int mmc_align_erase_size(struct mmc_card *card,
> * @card: card to erase
> * @from: first sector to erase
> * @nr: number of sectors to erase
> - * @arg: erase command argument (SD supports only %MMC_ERASE_ARG)
> + * @arg: erase command argument (SD supports only %SD_ERASE_ARG)
> *
> * Caller must claim host before calling this function.
> */
> @@ -2181,7 +2181,7 @@ int mmc_erase(struct mmc_card *card, unsigned int from, unsigned int nr,
> if (!card->erase_size)
> return -EOPNOTSUPP;
>
> - if (mmc_card_sd(card) && arg != MMC_ERASE_ARG)
> + if (mmc_card_sd(card) && arg != SD_ERASE_ARG)
> return -EOPNOTSUPP;
>
> if ((arg & MMC_SECURE_ARGS) &&
> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> index da892a5..09c688f 100644
> --- a/drivers/mmc/core/mmc.c
> +++ b/drivers/mmc/core/mmc.c
> @@ -1743,6 +1743,14 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr,
> card->ext_csd.power_off_notification = EXT_CSD_POWER_ON;
> }
>
> + /* set erase_arg */
> + if (mmc_can_discard(card))
> + card->erase_arg = MMC_DISCARD_ARG;
> + else if (mmc_can_trim(card))
> + card->erase_arg = MMC_TRIM_ARG;
> + else
> + card->erase_arg = MMC_ERASE_ARG;
> +
> /*
> * Select timing interface
> */
> diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
> index d0d9f90..bd48b28 100644
> --- a/drivers/mmc/core/sd.c
> +++ b/drivers/mmc/core/sd.c
> @@ -271,6 +271,8 @@ static int mmc_read_ssr(struct mmc_card *card)
> }
> }
>
> + card->erase_arg = SD_ERASE_ARG;
> +
> return 0;
> }
>
> diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
> index de73778..8f429b6 100644
> --- a/include/linux/mmc/card.h
> +++ b/include/linux/mmc/card.h
> @@ -277,6 +277,7 @@ struct mmc_card {
> unsigned int erase_shift; /* if erase unit is power 2 */
> unsigned int pref_erase; /* in sectors */
> unsigned int eg_boundary; /* don't cross erase-group boundaries */
> + unsigned int erase_arg; /* erase / trim / discard */
> u8 erased_byte; /* value of erased bytes */
>
> u32 raw_cid[4]; /* raw card CID */
> diff --git a/include/linux/mmc/sd.h b/include/linux/mmc/sd.h
> index 1ebcf9b..1a6d10f 100644
> --- a/include/linux/mmc/sd.h
> +++ b/include/linux/mmc/sd.h
> @@ -91,4 +91,9 @@
> #define SD_SWITCH_ACCESS_DEF 0
> #define SD_SWITCH_ACCESS_HS 1
>
> +/*
> + * Erase/discard
> + */
> +#define SD_ERASE_ARG 0x00000000
> +
> #endif /* LINUX_MMC_SD_H */
> --
> 1.9.1
>
On Wed, 6 Feb 2019 at 12:29, Avri Altman <[email protected]> wrote:
>
> SD specs version 4.x and 5.x have a dedicated slices in the SCR register.
> Higher versions will rely on a combination of the existing fields.
>
> Signed-off-by: Avri Altman <[email protected]>
Applied for next, thanks!
Kind regards
Uffe
> ---
> drivers/mmc/core/sd.c | 5 +++++
> include/linux/mmc/card.h | 2 ++
> 2 files changed, 7 insertions(+)
>
> diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
> index bd48b28..c2db94d 100644
> --- a/drivers/mmc/core/sd.c
> +++ b/drivers/mmc/core/sd.c
> @@ -209,6 +209,11 @@ static int mmc_decode_scr(struct mmc_card *card)
> /* Check if Physical Layer Spec v3.0 is supported */
> scr->sda_spec3 = UNSTUFF_BITS(resp, 47, 1);
>
> + if (scr->sda_spec3) {
> + scr->sda_spec4 = UNSTUFF_BITS(resp, 42, 1);
> + scr->sda_specx = UNSTUFF_BITS(resp, 38, 4);
> + }
> +
> if (UNSTUFF_BITS(resp, 55, 1))
> card->erased_byte = 0xFF;
> else
> diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
> index 8f429b6..d791813f 100644
> --- a/include/linux/mmc/card.h
> +++ b/include/linux/mmc/card.h
> @@ -133,6 +133,8 @@ struct mmc_ext_csd {
> struct sd_scr {
> unsigned char sda_vsn;
> unsigned char sda_spec3;
> + unsigned char sda_spec4;
> + unsigned char sda_specx;
> unsigned char bus_widths;
> #define SD_SCR_BUS_WIDTH_1 (1<<0)
> #define SD_SCR_BUS_WIDTH_4 (1<<2)
> --
> 1.9.1
>
On Wed, 6 Feb 2019 at 12:29, Avri Altman <[email protected]> wrote:
>
> SD spec v5.1 adds discard support. The flows and commands are similar to
> mmc, so just set the discard arg in CMD38.
>
> Actually, there is no need to check for the spec version like we are
> doing, as it is assured that the reserved bits in earlier versions are
> null. Do that anyway to document the spec version that introduce it.
>
> Signed-off-by: Avri Altman <[email protected]>
Deferring this a bit, need some more time to review.
Kind regards
Uffe
> ---
> drivers/mmc/core/core.c | 6 +++++-
> drivers/mmc/core/sd.c | 10 +++++++++-
> include/linux/mmc/sd.h | 1 +
> 3 files changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index de0f1a1..4d62f28 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -2164,7 +2164,7 @@ static unsigned int mmc_align_erase_size(struct mmc_card *card,
> * @card: card to erase
> * @from: first sector to erase
> * @nr: number of sectors to erase
> - * @arg: erase command argument (SD supports only %SD_ERASE_ARG)
> + * @arg: erase command argument
> *
> * Caller must claim host before calling this function.
> */
> @@ -2181,6 +2181,9 @@ int mmc_erase(struct mmc_card *card, unsigned int from, unsigned int nr,
> if (!card->erase_size)
> return -EOPNOTSUPP;
>
> + if (mmc_card_sd(card) && arg == SD_DISCARD_ARG)
> + goto skip_arg_testing;
> +
> if (mmc_card_sd(card) && arg != SD_ERASE_ARG)
> return -EOPNOTSUPP;
>
> @@ -2200,6 +2203,7 @@ int mmc_erase(struct mmc_card *card, unsigned int from, unsigned int nr,
> if (arg == MMC_ERASE_ARG)
> nr = mmc_align_erase_size(card, &from, &to, nr);
>
> +skip_arg_testing:
> if (nr == 0)
> return 0;
>
> diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
> index c2db94d..2b4fc22 100644
> --- a/drivers/mmc/core/sd.c
> +++ b/drivers/mmc/core/sd.c
> @@ -231,6 +231,8 @@ static int mmc_read_ssr(struct mmc_card *card)
> {
> unsigned int au, es, et, eo;
> __be32 *raw_ssr;
> + u32 resp[4] = {};
> + u8 discard_support;
> int i;
>
> if (!(card->csd.cmdclass & CCC_APP_SPEC)) {
> @@ -276,7 +278,13 @@ static int mmc_read_ssr(struct mmc_card *card)
> }
> }
>
> - card->erase_arg = SD_ERASE_ARG;
> + /*
> + * starting SD5.1 discard is supported if DISCARD_SUPPORT (b313) is set
> + */
> + resp[3] = card->raw_ssr[6];
> + discard_support = UNSTUFF_BITS(resp, 313 - 288, 1);
> + card->erase_arg = (card->scr.sda_specx && discard_support) ?
> + SD_DISCARD_ARG : SD_ERASE_ARG;
>
> return 0;
> }
> diff --git a/include/linux/mmc/sd.h b/include/linux/mmc/sd.h
> index 1a6d10f..ec94a5a 100644
> --- a/include/linux/mmc/sd.h
> +++ b/include/linux/mmc/sd.h
> @@ -95,5 +95,6 @@
> * Erase/discard
> */
> #define SD_ERASE_ARG 0x00000000
> +#define SD_DISCARD_ARG 0x00000001
>
> #endif /* LINUX_MMC_SD_H */
> --
> 1.9.1
>
>
> On Wed, 6 Feb 2019 at 12:29, Avri Altman <[email protected]> wrote:
> >
> > SD spec v5.1 adds discard support. The flows and commands are similar to
> > mmc, so just set the discard arg in CMD38.
> >
> > Actually, there is no need to check for the spec version like we are
> > doing, as it is assured that the reserved bits in earlier versions are
> > null. Do that anyway to document the spec version that introduce it.
> >
> > Signed-off-by: Avri Altman <[email protected]>
>
> Deferring this a bit, need some more time to review.
A kind reminder.
Thanks,
Avri
> >
> > On Wed, 6 Feb 2019 at 12:29, Avri Altman <[email protected]> wrote:
> > >
> > > SD spec v5.1 adds discard support. The flows and commands are similar
> to
> > > mmc, so just set the discard arg in CMD38.
> > >
> > > Actually, there is no need to check for the spec version like we are
> > > doing, as it is assured that the reserved bits in earlier versions are
> > > null. Do that anyway to document the spec version that introduce it.
> > >
> > > Signed-off-by: Avri Altman <[email protected]>
> >
> > Deferring this a bit, need some more time to review.
> A kind reminder.
A gentle ping.
Thanks,
Avri
> Thanks,
> Avri
On Wed, 6 Feb 2019 at 12:29, Avri Altman <[email protected]> wrote:
>
> SD spec v5.1 adds discard support. The flows and commands are similar to
> mmc, so just set the discard arg in CMD38.
So this means that we from now on, if the SD card supports discard, we
are going to use it in favor of erase. This is consistent with how we
treat eMMC, so I think it makes sense. Could you perhaps fold in some
information about this in the changelog, to make this clear on what
this change means.
You may also want to explain a little bit what the difference is from
the SD card storage point of view. Like after an erase, it either 1 or
0s on the media, while after a discard it can be whatever.
>
> Actually, there is no need to check for the spec version like we are
> doing, as it is assured that the reserved bits in earlier versions are
> null. Do that anyway to document the spec version that introduce it.
The check is needed for other purposes, so please just drop this statement.
>
> Signed-off-by: Avri Altman <[email protected]>
> ---
> drivers/mmc/core/core.c | 6 +++++-
> drivers/mmc/core/sd.c | 10 +++++++++-
> include/linux/mmc/sd.h | 1 +
> 3 files changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index de0f1a1..4d62f28 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -2164,7 +2164,7 @@ static unsigned int mmc_align_erase_size(struct mmc_card *card,
> * @card: card to erase
> * @from: first sector to erase
> * @nr: number of sectors to erase
> - * @arg: erase command argument (SD supports only %SD_ERASE_ARG)
> + * @arg: erase command argument
> *
> * Caller must claim host before calling this function.
> */
> @@ -2181,6 +2181,9 @@ int mmc_erase(struct mmc_card *card, unsigned int from, unsigned int nr,
> if (!card->erase_size)
> return -EOPNOTSUPP;
>
> + if (mmc_card_sd(card) && arg == SD_DISCARD_ARG)
> + goto skip_arg_testing;
> +
This isn't consistent with the rest of the code path in this function,
please adopt to that.
> if (mmc_card_sd(card) && arg != SD_ERASE_ARG)
Couldn't you add a check for !SD_DISCARD_ARG here instead?
> return -EOPNOTSUPP;
>
> @@ -2200,6 +2203,7 @@ int mmc_erase(struct mmc_card *card, unsigned int from, unsigned int nr,
> if (arg == MMC_ERASE_ARG)
> nr = mmc_align_erase_size(card, &from, &to, nr);
>
> +skip_arg_testing:
> if (nr == 0)
> return 0;
>
> diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
> index c2db94d..2b4fc22 100644
> --- a/drivers/mmc/core/sd.c
> +++ b/drivers/mmc/core/sd.c
> @@ -231,6 +231,8 @@ static int mmc_read_ssr(struct mmc_card *card)
> {
> unsigned int au, es, et, eo;
> __be32 *raw_ssr;
> + u32 resp[4] = {};
> + u8 discard_support;
> int i;
>
> if (!(card->csd.cmdclass & CCC_APP_SPEC)) {
> @@ -276,7 +278,13 @@ static int mmc_read_ssr(struct mmc_card *card)
> }
> }
>
> - card->erase_arg = SD_ERASE_ARG;
> + /*
> + * starting SD5.1 discard is supported if DISCARD_SUPPORT (b313) is set
> + */
> + resp[3] = card->raw_ssr[6];
> + discard_support = UNSTUFF_BITS(resp, 313 - 288, 1);
Couldn't you just replace this with "discard_support =
UNSTUFF_BITS(card->raw_ssr, 313 - 256, 1);" ?
> + card->erase_arg = (card->scr.sda_specx && discard_support) ?
> + SD_DISCARD_ARG : SD_ERASE_ARG;
>
> return 0;
> }
> diff --git a/include/linux/mmc/sd.h b/include/linux/mmc/sd.h
> index 1a6d10f..ec94a5a 100644
> --- a/include/linux/mmc/sd.h
> +++ b/include/linux/mmc/sd.h
> @@ -95,5 +95,6 @@
> * Erase/discard
> */
> #define SD_ERASE_ARG 0x00000000
> +#define SD_DISCARD_ARG 0x00000001
>
> #endif /* LINUX_MMC_SD_H */
> --
> 1.9.1
>
Besides the above changes, there is more important thing I think you
have left out to consider. With discard the operation should be
completed by the card within 250ms, while for erase the timeout
depends on the number of blocks to be erased. I am fine if we address
that on top this change though, just want to point it out so we don't
forget it.
Kind regards
Uffe
Ulf hi,
Thanks a lot for your comments.
>
> On Wed, 6 Feb 2019 at 12:29, Avri Altman <[email protected]> wrote:
> >
> > SD spec v5.1 adds discard support. The flows and commands are similar to
> > mmc, so just set the discard arg in CMD38.
>
> So this means that we from now on, if the SD card supports discard, we
> are going to use it in favor of erase. This is consistent with how we
> treat eMMC, so I think it makes sense. Could you perhaps fold in some
> information about this in the changelog, to make this clear on what
> this change means.
Done.
>
> You may also want to explain a little bit what the difference is from
> the SD card storage point of view. Like after an erase, it either 1 or
> 0s on the media, while after a discard it can be whatever.
Done.
>
> >
> > Actually, there is no need to check for the spec version like we are
> > doing, as it is assured that the reserved bits in earlier versions are
> > null. Do that anyway to document the spec version that introduce it.
>
> The check is needed for other purposes, so please just drop this statement.
Done.
>
> >
> > Signed-off-by: Avri Altman <[email protected]>
> > ---
> > drivers/mmc/core/core.c | 6 +++++-
> > drivers/mmc/core/sd.c | 10 +++++++++-
> > include/linux/mmc/sd.h | 1 +
> > 3 files changed, 15 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> > index de0f1a1..4d62f28 100644
> > --- a/drivers/mmc/core/core.c
> > +++ b/drivers/mmc/core/core.c
> > @@ -2164,7 +2164,7 @@ static unsigned int mmc_align_erase_size(struct
> mmc_card *card,
> > * @card: card to erase
> > * @from: first sector to erase
> > * @nr: number of sectors to erase
> > - * @arg: erase command argument (SD supports only %SD_ERASE_ARG)
> > + * @arg: erase command argument
> > *
> > * Caller must claim host before calling this function.
> > */
> > @@ -2181,6 +2181,9 @@ int mmc_erase(struct mmc_card *card, unsigned
> int from, unsigned int nr,
> > if (!card->erase_size)
> > return -EOPNOTSUPP;
> >
> > + if (mmc_card_sd(card) && arg == SD_DISCARD_ARG)
> > + goto skip_arg_testing;
> > +
>
> This isn't consistent with the rest of the code path in this function,
> please adopt to that.
Done.
>
> > if (mmc_card_sd(card) && arg != SD_ERASE_ARG)
>
> Couldn't you add a check for !SD_DISCARD_ARG here instead?
Done.
>
> > return -EOPNOTSUPP;
> >
> > @@ -2200,6 +2203,7 @@ int mmc_erase(struct mmc_card *card, unsigned
> int from, unsigned int nr,
> > if (arg == MMC_ERASE_ARG)
> > nr = mmc_align_erase_size(card, &from, &to, nr);
> >
> > +skip_arg_testing:
> > if (nr == 0)
> > return 0;
> >
> > diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
> > index c2db94d..2b4fc22 100644
> > --- a/drivers/mmc/core/sd.c
> > +++ b/drivers/mmc/core/sd.c
> > @@ -231,6 +231,8 @@ static int mmc_read_ssr(struct mmc_card *card)
> > {
> > unsigned int au, es, et, eo;
> > __be32 *raw_ssr;
> > + u32 resp[4] = {};
> > + u8 discard_support;
> > int i;
> >
> > if (!(card->csd.cmdclass & CCC_APP_SPEC)) {
> > @@ -276,7 +278,13 @@ static int mmc_read_ssr(struct mmc_card *card)
> > }
> > }
> >
> > - card->erase_arg = SD_ERASE_ARG;
> > + /*
> > + * starting SD5.1 discard is supported if DISCARD_SUPPORT (b313) is
> set
> > + */
> > + resp[3] = card->raw_ssr[6];
> > + discard_support = UNSTUFF_BITS(resp, 313 - 288, 1);
>
> Couldn't you just replace this with "discard_support =
> UNSTUFF_BITS(card->raw_ssr, 313 - 256, 1);" ?
SD status register is transmitted from 512bit onwards.
The 512th bit is the one transmitted first and so on.
So the 313th bit has to be checked at byte offset 0x18 in which both discard and FULE were enabled.
Byte offset 0x18 means dword offset 6, and UNSTUFF_BITS accounts for 4 dwords only.
Using UNSTUFF_BITS(card->raw_ssr,....) you can only access bits 511..383, but not 313.
>
> > + card->erase_arg = (card->scr.sda_specx && discard_support) ?
> > + SD_DISCARD_ARG : SD_ERASE_ARG;
> >
> > return 0;
> > }
> > diff --git a/include/linux/mmc/sd.h b/include/linux/mmc/sd.h
> > index 1a6d10f..ec94a5a 100644
> > --- a/include/linux/mmc/sd.h
> > +++ b/include/linux/mmc/sd.h
> > @@ -95,5 +95,6 @@
> > * Erase/discard
> > */
> > #define SD_ERASE_ARG 0x00000000
> > +#define SD_DISCARD_ARG 0x00000001
> >
> > #endif /* LINUX_MMC_SD_H */
> > --
> > 1.9.1
> >
>
> Besides the above changes, there is more important thing I think you
> have left out to consider. With discard the operation should be
> completed by the card within 250ms, while for erase the timeout
> depends on the number of blocks to be erased. I am fine if we address
> that on top this change though, just want to point it out so we don't
> forget it.
Done.
Will add another patch to account for that.
Thanks a lot,
Avri
>
> Kind regards
> Uffe