[PATCH v2 0/3] mmc: core: Add SD Discard support

[PATCH v2 0/3] mmc: core: Add SD Discard support

[Avri Altman]

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

[PATCH v2 1/3] mmc: core: Calculate the discard arg only once

[Avri Altman]

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 <avri.altman@wdc.com>
---
 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

Re: [PATCH v2 1/3] mmc: core: Calculate the discard arg only once

[Ulf Hansson]

On Wed, 6 Feb 2019 at 12:28, Avri Altman <avri.altman@wdc.com> 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 <avri.altman@wdc.com>

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
>

[PATCH v2 2/3] mmc: core: Indicate SD specs higher than 4.0

[Avri Altman]

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 <avri.altman@wdc.com>
---
 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

Re: [PATCH v2 2/3] mmc: core: Indicate SD specs higher than 4.0

[Ulf Hansson]

On Wed, 6 Feb 2019 at 12:29, Avri Altman <avri.altman@wdc.com> 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 <avri.altman@wdc.com>

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
>

[PATCH v2 3/3] mmc: core: Add discard support to sd

[Avri Altman]

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 <avri.altman@wdc.com>
---
 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

Re: [PATCH v2 3/3] mmc: core: Add discard support to sd

[Ulf Hansson]

On Wed, 6 Feb 2019 at 12:29, Avri Altman <avri.altman@wdc.com> 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 <avri.altman@wdc.com>

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
>

RE: [PATCH v2 3/3] mmc: core: Add discard support to sd

[Avri Altman]

> 
> On Wed, 6 Feb 2019 at 12:29, Avri Altman <avri.altman@wdc.com> 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 <avri.altman@wdc.com>
> 
> Deferring this a bit, need some more time to review.
A kind reminder.
Thanks,
Avri

RE: [PATCH v2 3/3] mmc: core: Add discard support to sd

[Avri Altman]

> >
> > On Wed, 6 Feb 2019 at 12:29, Avri Altman <avri.altman@wdc.com> 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 <avri.altman@wdc.com>
> >
> > Deferring this a bit, need some more time to review.
> A kind reminder.
A gentle ping.

Thanks,
Avri

> Thanks,
> Avri

Re: [PATCH v2 3/3] mmc: core: Add discard support to sd

[Ulf Hansson]

On Wed, 6 Feb 2019 at 12:29, Avri Altman <avri.altman@wdc.com> 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 <avri.altman@wdc.com>
> ---
>  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

RE: [PATCH v2 3/3] mmc: core: Add discard support to sd

[Avri Altman]

Ulf hi,
Thanks a lot for your comments.

> 
> On Wed, 6 Feb 2019 at 12:29, Avri Altman <avri.altman@wdc.com> 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 <avri.altman@wdc.com>
> > ---
> >  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