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

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

[Avri Altman]

D 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.

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  |  6 +++++-
 drivers/mmc/core/mmc.c   |  8 ++++++++
 drivers/mmc/core/sd.c    | 15 +++++++++++++++
 include/linux/mmc/card.h |  4 ++++
 include/linux/mmc/sd.h   |  6 ++++++
 6 files changed, 41 insertions(+), 10 deletions(-)

-- 
1.9.1

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

[Avri Altman]

The discard arg is a read-only ext_csd parameter -
set it once on card init.

Signed-off-by: Avri Altman <avri.altman@wdc.com>
---
 drivers/mmc/core/block.c | 12 +++---------
 drivers/mmc/core/mmc.c   |  8 ++++++++
 include/linux/mmc/card.h |  2 ++
 3 files changed, 13 insertions(+), 9 deletions(-)

diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
index b08fb91..3502f2c 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->discard_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->discard_arg);
 	} while (err == -EIO && !mmc_blk_reset(md, card->host, type));
 	if (err)
 		status = BLK_STS_IOERR;
diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index da892a5..120739c 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 discard_arg */
+	if (mmc_can_discard(card))
+		card->discard_arg = MMC_DISCARD_ARG;
+	else if (mmc_can_trim(card))
+		card->discard_arg = MMC_TRIM_ARG;
+	else
+		card->discard_arg = MMC_ERASE_ARG;
+
 	/*
 	 * Select timing interface
 	 */
diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
index de73778..447648b 100644
--- a/include/linux/mmc/card.h
+++ b/include/linux/mmc/card.h
@@ -308,6 +308,8 @@ struct mmc_card {
 	unsigned int    nr_parts;
 
 	unsigned int		bouncesz;	/* Bounce buffer size */
+
+	unsigned int		discard_arg;	/* discard args */
 };
 
 static inline bool mmc_large_sector(struct mmc_card *card)
-- 
1.9.1

[PATCH 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 d0d9f90..44f4cbc 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 447648b..919e644 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 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  |  6 ++++++
 3 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 5bd58b9..c495935 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -2181,7 +2181,10 @@ 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_DISCARD_ARG)
+		goto skip_arg_testing;
+
+	if (mmc_card_sd(card) && arg != SD_ERASE_ARG)
 		return -EOPNOTSUPP;
 
 	if ((arg & MMC_SECURE_ARGS) &&
@@ -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 44f4cbc..b554a17 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,6 +278,14 @@ static int mmc_read_ssr(struct mmc_card *card)
 		}
 	}
 
+	/*
+	 * 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->discard_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 1ebcf9b..ec94a5a 100644
--- a/include/linux/mmc/sd.h
+++ b/include/linux/mmc/sd.h
@@ -91,4 +91,10 @@
 #define SD_SWITCH_ACCESS_DEF	0
 #define SD_SWITCH_ACCESS_HS	1
 
+/*
+ * Erase/discard
+ */
+#define SD_ERASE_ARG			0x00000000
+#define SD_DISCARD_ARG			0x00000001
+
 #endif /* LINUX_MMC_SD_H */
-- 
1.9.1

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

[Ulf Hansson]

On Sun, 3 Feb 2019 at 09:51, Avri Altman <avri.altman@wdc.com> wrote:
>
> The discard arg is a read-only ext_csd parameter -
> set it once on card init.

I like the idea here. There is really no point checking this for every
corresponding request, nice!

However, the "discard arg" isn't specific to eMMC, as it's also used
for SD cards. So, the change log is a bit miss-leading, I think. Can
you please clarify this.

>
> Signed-off-by: Avri Altman <avri.altman@wdc.com>
> ---
>  drivers/mmc/core/block.c | 12 +++---------
>  drivers/mmc/core/mmc.c   |  8 ++++++++
>  include/linux/mmc/card.h |  2 ++
>  3 files changed, 13 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
> index b08fb91..3502f2c 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->discard_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->discard_arg);
>         } while (err == -EIO && !mmc_blk_reset(md, card->host, type));
>         if (err)
>                 status = BLK_STS_IOERR;
> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> index da892a5..120739c 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 discard_arg */
> +       if (mmc_can_discard(card))
> +               card->discard_arg = MMC_DISCARD_ARG;
> +       else if (mmc_can_trim(card))
> +               card->discard_arg = MMC_TRIM_ARG;
> +       else
> +               card->discard_arg = MMC_ERASE_ARG;

You are assigning card->discard_arg only for (e)MMC, while I think you
should do that also for SD.

I guess it practice it doesn't matter, because MMC_ERASE_ARG is zero.
Although, I would prefer to keep the code consistent, so I think you
should assign card->discard_arg for for SD cards as well.

> +
>         /*
>          * Select timing interface
>          */
> diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
> index de73778..447648b 100644
> --- a/include/linux/mmc/card.h
> +++ b/include/linux/mmc/card.h
> @@ -308,6 +308,8 @@ struct mmc_card {
>         unsigned int    nr_parts;
>
>         unsigned int            bouncesz;       /* Bounce buffer size */
> +
> +       unsigned int            discard_arg;    /* discard args */

Please move this a couple of lines above, close to the other "erase"
related variables.

You may even consider to rename the arg to "erase_arg", but I have no
strong opinion changing to that.

>  };
>
>  static inline bool mmc_large_sector(struct mmc_card *card)
> --
> 1.9.1
>

Kind regards
Uffe

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

[Avri Altman]

> On Sun, 3 Feb 2019 at 09:51, Avri Altman <avri.altman@wdc.com> wrote:
> >
> > The discard arg is a read-only ext_csd parameter -
> > set it once on card init.
> 
> I like the idea here. There is really no point checking this for every
> corresponding request, nice!
> 
> However, the "discard arg" isn't specific to eMMC, as it's also used
> for SD cards. So, the change log is a bit miss-leading, I think. Can
> you please clarify this.
Done.

> >
> > +       /* set discard_arg */
> > +       if (mmc_can_discard(card))
> > +               card->discard_arg = MMC_DISCARD_ARG;
> > +       else if (mmc_can_trim(card))
> > +               card->discard_arg = MMC_TRIM_ARG;
> > +       else
> > +               card->discard_arg = MMC_ERASE_ARG;
> 
> You are assigning card->discard_arg only for (e)MMC, while I think you
> should do that also for SD.
> 
> I guess it practice it doesn't matter, because MMC_ERASE_ARG is zero.
> Although, I would prefer to keep the code consistent, so I think you
> should assign card->discard_arg for for SD cards as well.
Done.

> > diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
> > index de73778..447648b 100644
> > --- a/include/linux/mmc/card.h
> > +++ b/include/linux/mmc/card.h
> > @@ -308,6 +308,8 @@ struct mmc_card {
> >         unsigned int    nr_parts;
> >
> >         unsigned int            bouncesz;       /* Bounce buffer size */
> > +
> > +       unsigned int            discard_arg;    /* discard args */
> 
> Please move this a couple of lines above, close to the other "erase"
> related variables.
Done.

> 
> You may even consider to rename the arg to "erase_arg", but I have no
> strong opinion changing to that.
Done.

> 
> >  };
> >
> >  static inline bool mmc_large_sector(struct mmc_card *card)
> > --
> > 1.9.1
> >
> 
> Kind regards
> Uffe