[PATCH] mmc: mtk-sd: Add support for ignoring cmd response CRC

[PATCH] mmc: mtk-sd: Add support for ignoring cmd response CRC

[Andy-ld Lu]

The current process flow does not handle MMC requests that are indicated
to ignore the command response CRC. For instance, cmd12 and cmd48 from
mmc_cqe_recovery() are marked to ignore CRC, but they are not matched to
the appropriate response type in msdc_cmd_find_resp(). As a result, they
are defaulted to 'MMC_RSP_NONE', which means no response is expected.

This commit adds a new flag 'MMC_RSP_R1B_NO_CRC' to work alongside the
existing 'MMC_RSP_R1_NO_CRC' for the following process flow. It fixes the
response type setting in msdc_cmd_find_resp() and adds the logic to ignore
CRC in msdc_cmd_done().

Signed-off-by: Andy-ld Lu <andy-ld.lu@mediatek.com>
---
 drivers/mmc/host/mtk-sd.c | 11 +++++++++--
 include/linux/mmc/core.h  |  1 +
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/mtk-sd.c b/drivers/mmc/host/mtk-sd.c
index efb0d2d5716b..5d669f985a82 100644
--- a/drivers/mmc/host/mtk-sd.c
+++ b/drivers/mmc/host/mtk-sd.c
@@ -1097,11 +1097,13 @@ static inline u32 msdc_cmd_find_resp(struct msdc_host *host,
 	u32 resp;
 
 	switch (mmc_resp_type(cmd)) {
-		/* Actually, R1, R5, R6, R7 are the same */
+	/* Actually, R1, R5, R6, R7 are the same */
 	case MMC_RSP_R1:
+	case MMC_RSP_R1_NO_CRC:
 		resp = 0x1;
 		break;
 	case MMC_RSP_R1B:
+	case MMC_RSP_R1B_NO_CRC:
 		resp = 0x7;
 		break;
 	case MMC_RSP_R2:
@@ -1305,6 +1307,7 @@ static bool msdc_cmd_done(struct msdc_host *host, int events,
 {
 	bool done = false;
 	bool sbc_error;
+	bool ignore_crc = false;
 	unsigned long flags;
 	u32 *rsp;
 
@@ -1329,6 +1332,10 @@ static bool msdc_cmd_done(struct msdc_host *host, int events,
 		return true;
 	rsp = cmd->resp;
 
+	if (mmc_resp_type(cmd) == MMC_RSP_R1_NO_CRC ||
+	    mmc_resp_type(cmd) == MMC_RSP_R1B_NO_CRC)
+		ignore_crc = true;
+
 	sdr_clr_bits(host->base + MSDC_INTEN, cmd_ints_mask);
 
 	if (cmd->flags & MMC_RSP_PRESENT) {
@@ -1351,7 +1358,7 @@ static bool msdc_cmd_done(struct msdc_host *host, int events,
 			 * CRC error.
 			 */
 			msdc_reset_hw(host);
-		if (events & MSDC_INT_RSPCRCERR) {
+		if (events & MSDC_INT_RSPCRCERR && !ignore_crc) {
 			cmd->error = -EILSEQ;
 			host->error |= REQ_CMD_EIO;
 		} else if (events & MSDC_INT_CMDTMO) {
diff --git a/include/linux/mmc/core.h b/include/linux/mmc/core.h
index 56972bd78462..076e1c71a07e 100644
--- a/include/linux/mmc/core.h
+++ b/include/linux/mmc/core.h
@@ -66,6 +66,7 @@ struct mmc_command {
 
 /* Can be used by core to poll after switch to MMC HS mode */
 #define MMC_RSP_R1_NO_CRC	(MMC_RSP_PRESENT|MMC_RSP_OPCODE)
+#define MMC_RSP_R1B_NO_CRC	(MMC_RSP_PRESENT|MMC_RSP_OPCODE|MMC_RSP_BUSY)
 
 #define mmc_resp_type(cmd)	((cmd)->flags & (MMC_RSP_PRESENT|MMC_RSP_136|MMC_RSP_CRC|MMC_RSP_BUSY|MMC_RSP_OPCODE))
 
-- 
2.34.1

Re: [PATCH] mmc: mtk-sd: Add support for ignoring cmd response CRC

[AngeloGioacchino Del Regno]

Il 23/11/24 08:09, Andy-ld Lu ha scritto:
> The current process flow does not handle MMC requests that are indicated
> to ignore the command response CRC. For instance, cmd12 and cmd48 from
> mmc_cqe_recovery() are marked to ignore CRC, but they are not matched to
> the appropriate response type in msdc_cmd_find_resp(). As a result, they
> are defaulted to 'MMC_RSP_NONE', which means no response is expected.
> 
> This commit adds a new flag 'MMC_RSP_R1B_NO_CRC' to work alongside the
> existing 'MMC_RSP_R1_NO_CRC' for the following process flow. It fixes the
> response type setting in msdc_cmd_find_resp() and adds the logic to ignore
> CRC in msdc_cmd_done().
> 
> Signed-off-by: Andy-ld Lu <andy-ld.lu@mediatek.com>

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

Re: [PATCH] mmc: mtk-sd: Add support for ignoring cmd response CRC

[Ulf Hansson]

On Sat, 23 Nov 2024 at 08:10, Andy-ld Lu <andy-ld.lu@mediatek.com> wrote:
>
> The current process flow does not handle MMC requests that are indicated
> to ignore the command response CRC. For instance, cmd12 and cmd48 from
> mmc_cqe_recovery() are marked to ignore CRC, but they are not matched to
> the appropriate response type in msdc_cmd_find_resp(). As a result, they
> are defaulted to 'MMC_RSP_NONE', which means no response is expected.
>
> This commit adds a new flag 'MMC_RSP_R1B_NO_CRC' to work alongside the
> existing 'MMC_RSP_R1_NO_CRC' for the following process flow. It fixes the
> response type setting in msdc_cmd_find_resp() and adds the logic to ignore
> CRC in msdc_cmd_done().

In fact, MMC_RSP_R1_NO_CRC is not being used by the MMC core. So, host
drivers that check this flag shouldn't need to. In other words, we
should remove that flag entirely.

That said, introducing MMC_RSP_R1B_NO_CRC as the $subject patch
suggests, makes sense to me. However, I prefer if we can make it used
by the mmc core, so please change mmc_cqe_recovery() to use it too.

Kind regards
Uffe

>
> Signed-off-by: Andy-ld Lu <andy-ld.lu@mediatek.com>
> ---
>  drivers/mmc/host/mtk-sd.c | 11 +++++++++--
>  include/linux/mmc/core.h  |  1 +
>  2 files changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mmc/host/mtk-sd.c b/drivers/mmc/host/mtk-sd.c
> index efb0d2d5716b..5d669f985a82 100644
> --- a/drivers/mmc/host/mtk-sd.c
> +++ b/drivers/mmc/host/mtk-sd.c
> @@ -1097,11 +1097,13 @@ static inline u32 msdc_cmd_find_resp(struct msdc_host *host,
>         u32 resp;
>
>         switch (mmc_resp_type(cmd)) {
> -               /* Actually, R1, R5, R6, R7 are the same */
> +       /* Actually, R1, R5, R6, R7 are the same */
>         case MMC_RSP_R1:
> +       case MMC_RSP_R1_NO_CRC:
>                 resp = 0x1;
>                 break;
>         case MMC_RSP_R1B:
> +       case MMC_RSP_R1B_NO_CRC:
>                 resp = 0x7;
>                 break;
>         case MMC_RSP_R2:
> @@ -1305,6 +1307,7 @@ static bool msdc_cmd_done(struct msdc_host *host, int events,
>  {
>         bool done = false;
>         bool sbc_error;
> +       bool ignore_crc = false;
>         unsigned long flags;
>         u32 *rsp;
>
> @@ -1329,6 +1332,10 @@ static bool msdc_cmd_done(struct msdc_host *host, int events,
>                 return true;
>         rsp = cmd->resp;
>
> +       if (mmc_resp_type(cmd) == MMC_RSP_R1_NO_CRC ||
> +           mmc_resp_type(cmd) == MMC_RSP_R1B_NO_CRC)
> +               ignore_crc = true;
> +
>         sdr_clr_bits(host->base + MSDC_INTEN, cmd_ints_mask);
>
>         if (cmd->flags & MMC_RSP_PRESENT) {
> @@ -1351,7 +1358,7 @@ static bool msdc_cmd_done(struct msdc_host *host, int events,
>                          * CRC error.
>                          */
>                         msdc_reset_hw(host);
> -               if (events & MSDC_INT_RSPCRCERR) {
> +               if (events & MSDC_INT_RSPCRCERR && !ignore_crc) {
>                         cmd->error = -EILSEQ;
>                         host->error |= REQ_CMD_EIO;
>                 } else if (events & MSDC_INT_CMDTMO) {
> diff --git a/include/linux/mmc/core.h b/include/linux/mmc/core.h
> index 56972bd78462..076e1c71a07e 100644
> --- a/include/linux/mmc/core.h
> +++ b/include/linux/mmc/core.h
> @@ -66,6 +66,7 @@ struct mmc_command {
>
>  /* Can be used by core to poll after switch to MMC HS mode */
>  #define MMC_RSP_R1_NO_CRC      (MMC_RSP_PRESENT|MMC_RSP_OPCODE)
> +#define MMC_RSP_R1B_NO_CRC     (MMC_RSP_PRESENT|MMC_RSP_OPCODE|MMC_RSP_BUSY)
>
>  #define mmc_resp_type(cmd)     ((cmd)->flags & (MMC_RSP_PRESENT|MMC_RSP_136|MMC_RSP_CRC|MMC_RSP_BUSY|MMC_RSP_OPCODE))
>
> --
> 2.34.1
>

Re: [PATCH] mmc: mtk-sd: Add support for ignoring cmd response CRC

[Andy-ld Lu (卢东)]

On Mon, 2024-11-25 at 14:08 +0100, Ulf Hansson wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> On Sat, 23 Nov 2024 at 08:10, Andy-ld Lu <andy-ld.lu@mediatek.com>
> wrote:
> > 
> > The current process flow does not handle MMC requests that are
> > indicated
> > to ignore the command response CRC. For instance, cmd12 and cmd48
> > from
> > mmc_cqe_recovery() are marked to ignore CRC, but they are not
> > matched to
> > the appropriate response type in msdc_cmd_find_resp(). As a result,
> > they
> > are defaulted to 'MMC_RSP_NONE', which means no response is
> > expected.
> > 
> > This commit adds a new flag 'MMC_RSP_R1B_NO_CRC' to work alongside
> > the
> > existing 'MMC_RSP_R1_NO_CRC' for the following process flow. It
> > fixes the
> > response type setting in msdc_cmd_find_resp() and adds the logic to
> > ignore
> > CRC in msdc_cmd_done().
> 
> In fact, MMC_RSP_R1_NO_CRC is not being used by the MMC core. So,
> host
> drivers that check this flag shouldn't need to. In other words, we
> should remove that flag entirely.
> 
> That said, introducing MMC_RSP_R1B_NO_CRC as the $subject patch
> suggests, makes sense to me. However, I prefer if we can make it used
> by the mmc core, so please change mmc_cqe_recovery() to use it too.
> 
> Kind regards
> Uffe

Thanks for your review, I will follow your comment in next change.

Best regards
Andy-ld Lu
> 
> > 
> > Signed-off-by: Andy-ld Lu <andy-ld.lu@mediatek.com>
> > ---
> >  drivers/mmc/host/mtk-sd.c | 11 +++++++++--
> >  include/linux/mmc/core.h  |  1 +
> >  2 files changed, 10 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/mmc/host/mtk-sd.c b/drivers/mmc/host/mtk-sd.c
> > index efb0d2d5716b..5d669f985a82 100644
> > --- a/drivers/mmc/host/mtk-sd.c
> > +++ b/drivers/mmc/host/mtk-sd.c
> > @@ -1097,11 +1097,13 @@ static inline u32 msdc_cmd_find_resp(struct
> > msdc_host *host,
> >         u32 resp;
> > 
> >         switch (mmc_resp_type(cmd)) {
> > -               /* Actually, R1, R5, R6, R7 are the same */
> > +       /* Actually, R1, R5, R6, R7 are the same */
> >         case MMC_RSP_R1:
> > +       case MMC_RSP_R1_NO_CRC:
> >                 resp = 0x1;
> >                 break;
> >         case MMC_RSP_R1B:
> > +       case MMC_RSP_R1B_NO_CRC:
> >                 resp = 0x7;
> >                 break;
> >         case MMC_RSP_R2:
> > @@ -1305,6 +1307,7 @@ static bool msdc_cmd_done(struct msdc_host
> > *host, int events,
> >  {
> >         bool done = false;
> >         bool sbc_error;
> > +       bool ignore_crc = false;
> >         unsigned long flags;
> >         u32 *rsp;
> > 
> > @@ -1329,6 +1332,10 @@ static bool msdc_cmd_done(struct msdc_host
> > *host, int events,
> >                 return true;
> >         rsp = cmd->resp;
> > 
> > +       if (mmc_resp_type(cmd) == MMC_RSP_R1_NO_CRC ||
> > +           mmc_resp_type(cmd) == MMC_RSP_R1B_NO_CRC)
> > +               ignore_crc = true;
> > +
> >         sdr_clr_bits(host->base + MSDC_INTEN, cmd_ints_mask);
> > 
> >         if (cmd->flags & MMC_RSP_PRESENT) {
> > @@ -1351,7 +1358,7 @@ static bool msdc_cmd_done(struct msdc_host
> > *host, int events,
> >                          * CRC error.
> >                          */
> >                         msdc_reset_hw(host);
> > -               if (events & MSDC_INT_RSPCRCERR) {
> > +               if (events & MSDC_INT_RSPCRCERR && !ignore_crc) {
> >                         cmd->error = -EILSEQ;
> >                         host->error |= REQ_CMD_EIO;
> >                 } else if (events & MSDC_INT_CMDTMO) {
> > diff --git a/include/linux/mmc/core.h b/include/linux/mmc/core.h
> > index 56972bd78462..076e1c71a07e 100644
> > --- a/include/linux/mmc/core.h
> > +++ b/include/linux/mmc/core.h
> > @@ -66,6 +66,7 @@ struct mmc_command {
> > 
> >  /* Can be used by core to poll after switch to MMC HS mode */
> >  #define MMC_RSP_R1_NO_CRC      (MMC_RSP_PRESENT|MMC_RSP_OPCODE)
> > +#define
> > MMC_RSP_R1B_NO_CRC     (MMC_RSP_PRESENT|MMC_RSP_OPCODE|MMC_RSP_BUSY
> > )
> > 
> >  #define mmc_resp_type(cmd)     ((cmd)->flags &
> > (MMC_RSP_PRESENT|MMC_RSP_136|MMC_RSP_CRC|MMC_RSP_BUSY|MMC_RSP_OPCOD
> > E))
> > 
> > --
> > 2.34.1
> >