[PATCH] mmc: sh_mmcif: Support MMC_SLEEP_AWAKE command

[PATCH] mmc: sh_mmcif: Support MMC_SLEEP_AWAKE command

[Laurent Pinchart]

The MMC_SLEEP_AWAKE and SD_IO_SEND_OP_COND commands share the same
opcode. SD_IO_SEND_OP_COND isn't supported by the SH MMCIF, but
MMC_SLEEP_AWAKE is. Discriminate between the two commands using the
command flags, and reject SD_IO_SEND_OP_COND only.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/mmc/host/sh_mmcif.c |   14 ++++----------
 1 files changed, 4 insertions(+), 10 deletions(-)

Not supporting the MMC_SLEEP_AWAKE command makes system suspend fail if an MMC
or eMMC device supporting sleep/wake is connected. The issue has been first
noticed on the Armadillo 800 EVA board.

diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c
index 724b35e..e32da11 100644
--- a/drivers/mmc/host/sh_mmcif.c
+++ b/drivers/mmc/host/sh_mmcif.c
@@ -892,21 +892,15 @@ static void sh_mmcif_request(struct mmc_host *mmc, struct mmc_request *mrq)
 
 	switch (mrq->cmd->opcode) {
 	/* MMCIF does not support SD/SDIO command */
-	case SD_IO_SEND_OP_COND:
+	case MMC_SLEEP_AWAKE: /* = SD_IO_SEND_OP_COND (5) */
+	case MMC_SEND_EXT_CSD: /* = SD_SEND_IF_COND (8) */
+		if ((mrq->cmd->flags & MMC_CMD_MASK) != MMC_CMD_BCR)
+			break;
 	case MMC_APP_CMD:
 		host->state = STATE_IDLE;
 		mrq->cmd->error = -ETIMEDOUT;
 		mmc_request_done(mmc, mrq);
 		return;
-	case MMC_SEND_EXT_CSD: /* = SD_SEND_IF_COND (8) */
-		if (!mrq->data) {
-			/* send_if_cond cmd (not support) */
-			host->state = STATE_IDLE;
-			mrq->cmd->error = -ETIMEDOUT;
-			mmc_request_done(mmc, mrq);
-			return;
-		}
-		break;
 	default:
 		break;
 	}
-- 
Regards,

Laurent Pinchart

Re: [PATCH] mmc: sh_mmcif: Support MMC_SLEEP_AWAKE command

[Simon Horman]

On Tue, Jun 12, 2012 at 10:56:09PM +0200, Laurent Pinchart wrote:
> The MMC_SLEEP_AWAKE and SD_IO_SEND_OP_COND commands share the same
> opcode. SD_IO_SEND_OP_COND isn't supported by the SH MMCIF, but
> MMC_SLEEP_AWAKE is. Discriminate between the two commands using the
> command flags, and reject SD_IO_SEND_OP_COND only.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/mmc/host/sh_mmcif.c |   14 ++++----------
>  1 files changed, 4 insertions(+), 10 deletions(-)
> 
> Not supporting the MMC_SLEEP_AWAKE command makes system suspend fail if an MMC
> or eMMC device supporting sleep/wake is connected. The issue has been first
> noticed on the Armadillo 800 EVA board.

Hi Laurent,

Do you have a test-case for this?
Also, did you check to make sure that the Mackerel still works?

Re: [PATCH] mmc: sh_mmcif: Support MMC_SLEEP_AWAKE command

[Laurent Pinchart]

Hi Simon,

On Wednesday 13 June 2012 10:12:01 Simon Horman wrote:
> On Tue, Jun 12, 2012 at 10:56:09PM +0200, Laurent Pinchart wrote:
> > The MMC_SLEEP_AWAKE and SD_IO_SEND_OP_COND commands share the same
> > opcode. SD_IO_SEND_OP_COND isn't supported by the SH MMCIF, but
> > MMC_SLEEP_AWAKE is. Discriminate between the two commands using the
> > command flags, and reject SD_IO_SEND_OP_COND only.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > 
> >  drivers/mmc/host/sh_mmcif.c |   14 ++++----------
> >  1 files changed, 4 insertions(+), 10 deletions(-)
> > 
> > Not supporting the MMC_SLEEP_AWAKE command makes system suspend fail if an
> > MMC or eMMC device supporting sleep/wake is connected. The issue has been
> > first noticed on the Armadillo 800 EVA board.
> 
> Hi Laurent,
> 
> Do you have a test-case for this?

echo mem > /sys/power/state on Armadillo 800 EVA was my test case. It failed 
without the patch, and succeeds with it.

> Also, did you check to make sure that the Mackerel still works?

Yes it still works. However, I have no way to test the MMC_SLEEP_AWAKE command 
on the Mackerel board, as my MMC card doesn't support it (on the Armadillo the 
eMMC chip supports the MMC_SLEEP_AWAKE command).

BTW, I wonder whether the current implementation is really the best one. If 
the hardware doesn't support SD/SDIO commands, instead of intercepting 
commands and rejecting the ones used by SD/SDIO at probe time, wouldn't it be 
better for host drivers to tell that they don't support SD and/or SDIO using 
flags in the mmc_host structure ?

-- 
Regards,

Laurent Pinchart

Re: [PATCH] mmc: sh_mmcif: Support MMC_SLEEP_AWAKE command

[Simon Horman]

On Wed, Jun 13, 2012 at 09:39:56AM +0200, Laurent Pinchart wrote:
> Hi Simon,
> 
> On Wednesday 13 June 2012 10:12:01 Simon Horman wrote:
> > On Tue, Jun 12, 2012 at 10:56:09PM +0200, Laurent Pinchart wrote:
> > > The MMC_SLEEP_AWAKE and SD_IO_SEND_OP_COND commands share the same
> > > opcode. SD_IO_SEND_OP_COND isn't supported by the SH MMCIF, but
> > > MMC_SLEEP_AWAKE is. Discriminate between the two commands using the
> > > command flags, and reject SD_IO_SEND_OP_COND only.
> > > 
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > ---
> > > 
> > >  drivers/mmc/host/sh_mmcif.c |   14 ++++----------
> > >  1 files changed, 4 insertions(+), 10 deletions(-)
> > > 
> > > Not supporting the MMC_SLEEP_AWAKE command makes system suspend fail if an
> > > MMC or eMMC device supporting sleep/wake is connected. The issue has been
> > > first noticed on the Armadillo 800 EVA board.
> > 
> > Hi Laurent,
> > 
> > Do you have a test-case for this?
> 
> echo mem > /sys/power/state on Armadillo 800 EVA was my test case. It failed 
> without the patch, and succeeds with it.

I'm not having much luck with or without your patch.

# echo mem > /sys/power/state
echo: write error: No such device

I am using 3.5-rc3. With the armadillo default config + EXT3 + SUSPEND.
Perhaps I am missing some options?

> > Also, did you check to make sure that the Mackerel still works?
> 
> Yes it still works. However, I have no way to test the MMC_SLEEP_AWAKE command 
> on the Mackerel board, as my MMC card doesn't support it (on the Armadillo the 
> eMMC chip supports the MMC_SLEEP_AWAKE command).
> 
> BTW, I wonder whether the current implementation is really the best one. If 
> the hardware doesn't support SD/SDIO commands, instead of intercepting 
> commands and rejecting the ones used by SD/SDIO at probe time, wouldn't it be 
> better for host drivers to tell that they don't support SD and/or SDIO using 
> flags in the mmc_host structure ?

I don't have any strong thoughts on this but your approach
does sound interesting.

Re: [PATCH] mmc: sh_mmcif: Support MMC_SLEEP_AWAKE command

[Laurent Pinchart]

Hi Simon,

On Wednesday 20 June 2012 15:42:38 Simon Horman wrote:
> On Wed, Jun 13, 2012 at 09:39:56AM +0200, Laurent Pinchart wrote:
> > On Wednesday 13 June 2012 10:12:01 Simon Horman wrote:
> > > On Tue, Jun 12, 2012 at 10:56:09PM +0200, Laurent Pinchart wrote:
> > > > The MMC_SLEEP_AWAKE and SD_IO_SEND_OP_COND commands share the same
> > > > opcode. SD_IO_SEND_OP_COND isn't supported by the SH MMCIF, but
> > > > MMC_SLEEP_AWAKE is. Discriminate between the two commands using the
> > > > command flags, and reject SD_IO_SEND_OP_COND only.
> > > > 
> > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > ---
> > > > 
> > > >  drivers/mmc/host/sh_mmcif.c |   14 ++++----------
> > > >  1 files changed, 4 insertions(+), 10 deletions(-)
> > > > 
> > > > Not supporting the MMC_SLEEP_AWAKE command makes system suspend fail
> > > > if an MMC or eMMC device supporting sleep/wake is connected. The issue
> > > > has been first noticed on the Armadillo 800 EVA board.
> > > 
> > > Hi Laurent,
> > > 
> > > Do you have a test-case for this?
> > 
> > echo mem > /sys/power/state on Armadillo 800 EVA was my test case. It
> > failed without the patch, and succeeds with it.
> 
> I'm not having much luck with or without your patch.
> 
> # echo mem > /sys/power/state
> echo: write error: No such device
> 
> I am using 3.5-rc3. With the armadillo default config + EXT3 + SUSPEND.
> Perhaps I am missing some options?

You're probably missing a patch. I'll send it as a reply to this e-mail.

> > > Also, did you check to make sure that the Mackerel still works?
> > 
> > Yes it still works. However, I have no way to test the MMC_SLEEP_AWAKE
> > command on the Mackerel board, as my MMC card doesn't support it (on the
> > Armadillo the eMMC chip supports the MMC_SLEEP_AWAKE command).
> > 
> > BTW, I wonder whether the current implementation is really the best one.
> > If the hardware doesn't support SD/SDIO commands, instead of intercepting
> > commands and rejecting the ones used by SD/SDIO at probe time, wouldn't it
> > be better for host drivers to tell that they don't support SD and/or SDIO
> > using flags in the mmc_host structure ?
> 
> I don't have any strong thoughts on this but your approach
> does sound interesting.

-- 
Regards,

Laurent Pinchart

Re: [PATCH] mmc: sh_mmcif: Support MMC_SLEEP_AWAKE command

[Guennadi Liakhovetski]

On Tue, 12 Jun 2012, Laurent Pinchart wrote:

> The MMC_SLEEP_AWAKE and SD_IO_SEND_OP_COND commands share the same
> opcode. SD_IO_SEND_OP_COND isn't supported by the SH MMCIF, but
> MMC_SLEEP_AWAKE is. Discriminate between the two commands using the
> command flags, and reject SD_IO_SEND_OP_COND only.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Acked-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>

Thanks
Guennadi

> ---
>  drivers/mmc/host/sh_mmcif.c |   14 ++++----------
>  1 files changed, 4 insertions(+), 10 deletions(-)
> 
> Not supporting the MMC_SLEEP_AWAKE command makes system suspend fail if an MMC
> or eMMC device supporting sleep/wake is connected. The issue has been first
> noticed on the Armadillo 800 EVA board.
> 
> diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c
> index 724b35e..e32da11 100644
> --- a/drivers/mmc/host/sh_mmcif.c
> +++ b/drivers/mmc/host/sh_mmcif.c
> @@ -892,21 +892,15 @@ static void sh_mmcif_request(struct mmc_host *mmc, struct mmc_request *mrq)
>  
>  	switch (mrq->cmd->opcode) {
>  	/* MMCIF does not support SD/SDIO command */
> -	case SD_IO_SEND_OP_COND:
> +	case MMC_SLEEP_AWAKE: /* = SD_IO_SEND_OP_COND (5) */
> +	case MMC_SEND_EXT_CSD: /* = SD_SEND_IF_COND (8) */
> +		if ((mrq->cmd->flags & MMC_CMD_MASK) != MMC_CMD_BCR)
> +			break;
>  	case MMC_APP_CMD:
>  		host->state = STATE_IDLE;
>  		mrq->cmd->error = -ETIMEDOUT;
>  		mmc_request_done(mmc, mrq);
>  		return;
> -	case MMC_SEND_EXT_CSD: /* = SD_SEND_IF_COND (8) */
> -		if (!mrq->data) {
> -			/* send_if_cond cmd (not support) */
> -			host->state = STATE_IDLE;
> -			mrq->cmd->error = -ETIMEDOUT;
> -			mmc_request_done(mmc, mrq);
> -			return;
> -		}
> -		break;
>  	default:
>  		break;
>  	}
> -- 
> Regards,
> 
> Laurent Pinchart
> 

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH] mmc: sh_mmcif: Support MMC_SLEEP_AWAKE command

[Chris Ball]

Hi,

On Wed, Jun 13 2012, Guennadi Liakhovetski wrote:
> On Tue, 12 Jun 2012, Laurent Pinchart wrote:
>
>> The MMC_SLEEP_AWAKE and SD_IO_SEND_OP_COND commands share the same
>> opcode. SD_IO_SEND_OP_COND isn't supported by the SH MMCIF, but
>> MMC_SLEEP_AWAKE is. Discriminate between the two commands using the
>> command flags, and reject SD_IO_SEND_OP_COND only.
>> 
>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> Acked-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>

Thanks, pushed to mmc-next for 3.6.

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

Re: [PATCH] mmc: sh_mmcif: Support MMC_SLEEP_AWAKE command

[Ulf Hansson]

Hi Laurent,

Your issue seem more related to a mmc protocol problem. Likely caused
by a bad patch for eMMC 4.5 poweroff notify. The patch I refer to is:
"mmc: core: Fix PowerOff Notify suspend/resume"

You may try to revert this patch and see if the same problem occurs for you.

We are trying to fix the issue inserted by the above commit in a patch named:
"MMC-4.5 Power OFF Notify Rework", please have a look if you have the time.

Kind regards
Ulf Hansson

On 12 June 2012 22:56, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> The MMC_SLEEP_AWAKE and SD_IO_SEND_OP_COND commands share the same
> opcode. SD_IO_SEND_OP_COND isn't supported by the SH MMCIF, but
> MMC_SLEEP_AWAKE is. Discriminate between the two commands using the
> command flags, and reject SD_IO_SEND_OP_COND only.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/mmc/host/sh_mmcif.c |   14 ++++----------
>  1 files changed, 4 insertions(+), 10 deletions(-)
>
> Not supporting the MMC_SLEEP_AWAKE command makes system suspend fail if an MMC
> or eMMC device supporting sleep/wake is connected. The issue has been first
> noticed on the Armadillo 800 EVA board.
>
> diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c
> index 724b35e..e32da11 100644
> --- a/drivers/mmc/host/sh_mmcif.c
> +++ b/drivers/mmc/host/sh_mmcif.c
> @@ -892,21 +892,15 @@ static void sh_mmcif_request(struct mmc_host *mmc, struct mmc_request *mrq)
>
>        switch (mrq->cmd->opcode) {
>        /* MMCIF does not support SD/SDIO command */
> -       case SD_IO_SEND_OP_COND:
> +       case MMC_SLEEP_AWAKE: /* = SD_IO_SEND_OP_COND (5) */
> +       case MMC_SEND_EXT_CSD: /* = SD_SEND_IF_COND (8) */
> +               if ((mrq->cmd->flags & MMC_CMD_MASK) != MMC_CMD_BCR)
> +                       break;
>        case MMC_APP_CMD:
>                host->state = STATE_IDLE;
>                mrq->cmd->error = -ETIMEDOUT;
>                mmc_request_done(mmc, mrq);
>                return;
> -       case MMC_SEND_EXT_CSD: /* = SD_SEND_IF_COND (8) */
> -               if (!mrq->data) {
> -                       /* send_if_cond cmd (not support) */
> -                       host->state = STATE_IDLE;
> -                       mrq->cmd->error = -ETIMEDOUT;
> -                       mmc_request_done(mmc, mrq);
> -                       return;
> -               }
> -               break;
>        default:
>                break;
>        }
> --
> Regards,
>
> Laurent Pinchart
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] mmc: sh_mmcif: Support MMC_SLEEP_AWAKE command

[Laurent Pinchart]

Hi Ulf,

On Wednesday 20 June 2012 13:54:33 Ulf Hansson wrote:
> Hi Laurent,
> 
> Your issue seem more related to a mmc protocol problem. Likely caused
> by a bad patch for eMMC 4.5 poweroff notify. The patch I refer to is:
> "mmc: core: Fix PowerOff Notify suspend/resume"
> 
> You may try to revert this patch and see if the same problem occurs for you.
> 
> We are trying to fix the issue inserted by the above commit in a patch
> named: "MMC-4.5 Power OFF Notify Rework", please have a look if you have
> the time.

I think the two issues are othorgonal (but please feel free to tell me if 
there's something I'm missing).

The MMCIF controller driver rejects the MMC_SLEEP_AWAKE command 
unconditionally, even though it should be accepted, because it happens to 
share the command number with an SDIO command (SD_IO_SEND_OP_COND) used to 
probe SDIO support. My patch fixes this by accepting the MMC_SLEEP_AWAKE 
command but still rejecting the SD_IO_SEND_OP_COND command.

-- 
Regards,

Laurent Pinchart