All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 0/4] mmc: host: tmio: properly report status from autocmd12
@ 2017-02-13 18:03 Wolfram Sang
  2017-02-13 18:03 ` [PATCH v2 1/4] mmc: host: tmio: use defines for CTL_STOP_INTERNAL_ACTION values Wolfram Sang
                   ` (3 more replies)
  0 siblings, 4 replies; 13+ messages in thread
From: Wolfram Sang @ 2017-02-13 18:03 UTC (permalink / raw)
  To: linux-mmc; +Cc: linux-renesas-soc, Yoshihiro Shimoda, Wolfram Sang

New in V2:
* Fix some more comments in patch 2.
* Add Simon's tags

SDHI automatically sends CMD12 when the desired amount of data was transferred
after multi block commands. However, the response from that CMD12 was never
reported back to the mmc core, so that errors like ECC might go unnoticed. This
series aims to fix that and clean up minor issues on the way when dealing with
the autocmd12 feature. So far, I could only test that the success case was
reported back to the MMC core [1]. An error case (like ECC) could not be
created yet, but I will keep on trying. However, the initial flaw of the CMD12
response not reported back is fixed with this series as the success case
demonstrates. If an ECC error is not properly handled, it is a seperate issue
anyway. So, I think this series could go in now. No pressure for 4.11, though.
But would be nice, of course ;)

The patches are based on mmc-next as of today. A branch can be found here:

git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/topic/sdhi-autocmd12-resp

Thanks and all the best,

   Wolfram

[1] http://elinux.org/Tests:SDHI-autocmd12-responses


Wolfram Sang (4):
  mmc: host: tmio: use defines for CTL_STOP_INTERNAL_ACTION values
  mmc: host: tmio: fix minor typos in comments
  mmc: host: tmio: don't BUG on unsupported stop commands
  mmc: host: tmio: fill in response from auto cmd12

 drivers/mmc/host/tmio_mmc.h     | 10 +++++++---
 drivers/mmc/host/tmio_mmc_pio.c | 16 ++++++++++------
 2 files changed, 17 insertions(+), 9 deletions(-)

-- 
2.11.0

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH v2 1/4] mmc: host: tmio: use defines for CTL_STOP_INTERNAL_ACTION values
  2017-02-13 18:03 [PATCH v2 0/4] mmc: host: tmio: properly report status from autocmd12 Wolfram Sang
@ 2017-02-13 18:03 ` Wolfram Sang
  2017-02-13 18:03 ` [PATCH v2 2/4] mmc: host: tmio: fix minor typos in comments Wolfram Sang
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 13+ messages in thread
From: Wolfram Sang @ 2017-02-13 18:03 UTC (permalink / raw)
  To: linux-mmc
  Cc: linux-renesas-soc, Yoshihiro Shimoda, Wolfram Sang, Simon Horman

Reviewed-by: Simon Horman <horms+renesas@verge.net.au>
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/mmc/host/tmio_mmc.h     | 4 ++++
 drivers/mmc/host/tmio_mmc_pio.c | 6 +++---
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h
index 2b349d48fb9a8a..b20b451ad90daa 100644
--- a/drivers/mmc/host/tmio_mmc.h
+++ b/drivers/mmc/host/tmio_mmc.h
@@ -50,6 +50,10 @@
 #define CTL_CLK_AND_WAIT_CTL 0x138
 #define CTL_RESET_SDIO 0x1e0
 
+/* Definitions for values the CTL_STOP_INTERNAL_ACTION register can take */
+#define TMIO_STOP_STP		BIT(0)
+#define TMIO_STOP_SEC		BIT(8)
+
 /* Definitions for values the CTRL_STATUS register can take. */
 #define TMIO_STAT_CMDRESPEND    BIT(0)
 #define TMIO_STAT_DATAEND       BIT(2)
diff --git a/drivers/mmc/host/tmio_mmc_pio.c b/drivers/mmc/host/tmio_mmc_pio.c
index 6b789a739d4dfe..ad2840e1bfae51 100644
--- a/drivers/mmc/host/tmio_mmc_pio.c
+++ b/drivers/mmc/host/tmio_mmc_pio.c
@@ -340,7 +340,7 @@ static int tmio_mmc_start_command(struct tmio_mmc_host *host, struct mmc_command
 
 	/* CMD12 is handled by hardware */
 	if (cmd->opcode == MMC_STOP_TRANSMISSION && !cmd->arg) {
-		sd_ctrl_write16(host, CTL_STOP_INTERNAL_ACTION, 0x001);
+		sd_ctrl_write16(host, CTL_STOP_INTERNAL_ACTION, TMIO_STOP_STP);
 		return 0;
 	}
 
@@ -367,7 +367,7 @@ static int tmio_mmc_start_command(struct tmio_mmc_host *host, struct mmc_command
 	if (data) {
 		c |= DATA_PRESENT;
 		if (data->blocks > 1) {
-			sd_ctrl_write16(host, CTL_STOP_INTERNAL_ACTION, 0x100);
+			sd_ctrl_write16(host, CTL_STOP_INTERNAL_ACTION, TMIO_STOP_SEC);
 			c |= TRANSFER_MULTI;
 
 			/*
@@ -554,7 +554,7 @@ void tmio_mmc_do_data_irq(struct tmio_mmc_host *host)
 
 	if (stop) {
 		if (stop->opcode == MMC_STOP_TRANSMISSION && !stop->arg)
-			sd_ctrl_write16(host, CTL_STOP_INTERNAL_ACTION, 0x000);
+			sd_ctrl_write16(host, CTL_STOP_INTERNAL_ACTION, 0);
 		else
 			BUG();
 	}
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v2 2/4] mmc: host: tmio: fix minor typos in comments
  2017-02-13 18:03 [PATCH v2 0/4] mmc: host: tmio: properly report status from autocmd12 Wolfram Sang
  2017-02-13 18:03 ` [PATCH v2 1/4] mmc: host: tmio: use defines for CTL_STOP_INTERNAL_ACTION values Wolfram Sang
@ 2017-02-13 18:03 ` Wolfram Sang
  2017-02-16 15:03   ` Simon Horman
  2017-02-13 18:03 ` [PATCH v2 3/4] mmc: host: tmio: don't BUG on unsupported stop commands Wolfram Sang
  2017-02-13 18:03 ` [PATCH v2 4/4] mmc: host: tmio: fill in response from auto cmd12 Wolfram Sang
  3 siblings, 1 reply; 13+ messages in thread
From: Wolfram Sang @ 2017-02-13 18:03 UTC (permalink / raw)
  To: linux-mmc; +Cc: linux-renesas-soc, Yoshihiro Shimoda, Wolfram Sang

Making sure we match the actual register names.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/mmc/host/tmio_mmc.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h
index b20b451ad90daa..d192b562282bb2 100644
--- a/drivers/mmc/host/tmio_mmc.h
+++ b/drivers/mmc/host/tmio_mmc.h
@@ -54,7 +54,7 @@
 #define TMIO_STOP_STP		BIT(0)
 #define TMIO_STOP_SEC		BIT(8)
 
-/* Definitions for values the CTRL_STATUS register can take. */
+/* Definitions for values the CTL_STATUS register can take */
 #define TMIO_STAT_CMDRESPEND    BIT(0)
 #define TMIO_STAT_DATAEND       BIT(2)
 #define TMIO_STAT_CARD_REMOVE   BIT(3)
@@ -65,7 +65,7 @@
 #define TMIO_STAT_CARD_INSERT_A BIT(9)
 #define TMIO_STAT_SIGSTATE_A    BIT(10)
 
-/* These belong technically to CTRL_STATUS2, but the driver merges them */
+/* These belong technically to CTL_STATUS2, but the driver merges them */
 #define TMIO_STAT_CMD_IDX_ERR   BIT(16)
 #define TMIO_STAT_CRCFAIL       BIT(17)
 #define TMIO_STAT_STOPBIT_ERR   BIT(18)
@@ -89,7 +89,7 @@
 
 #define TMIO_BBS		512		/* Boot block size */
 
-/* Definitions for values the CTRL_SDIO_STATUS register can take. */
+/* Definitions for values the CTL_SDIO_STATUS register can take */
 #define TMIO_SDIO_STAT_IOIRQ	0x0001
 #define TMIO_SDIO_STAT_EXPUB52	0x4000
 #define TMIO_SDIO_STAT_EXWT	0x8000
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v2 3/4] mmc: host: tmio: don't BUG on unsupported stop commands
  2017-02-13 18:03 [PATCH v2 0/4] mmc: host: tmio: properly report status from autocmd12 Wolfram Sang
  2017-02-13 18:03 ` [PATCH v2 1/4] mmc: host: tmio: use defines for CTL_STOP_INTERNAL_ACTION values Wolfram Sang
  2017-02-13 18:03 ` [PATCH v2 2/4] mmc: host: tmio: fix minor typos in comments Wolfram Sang
@ 2017-02-13 18:03 ` Wolfram Sang
  2017-02-13 18:03 ` [PATCH v2 4/4] mmc: host: tmio: fill in response from auto cmd12 Wolfram Sang
  3 siblings, 0 replies; 13+ messages in thread
From: Wolfram Sang @ 2017-02-13 18:03 UTC (permalink / raw)
  To: linux-mmc
  Cc: linux-renesas-soc, Yoshihiro Shimoda, Wolfram Sang, Simon Horman

Halting the kernel on an unsupported stop command seems overkill, report
the error and say what we already did (due to autocmd12) instead.

Reviewed-by: Simon Horman <horms+renesas@verge.net.au>
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/mmc/host/tmio_mmc_pio.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/mmc/host/tmio_mmc_pio.c b/drivers/mmc/host/tmio_mmc_pio.c
index ad2840e1bfae51..b47dd9195fe3fe 100644
--- a/drivers/mmc/host/tmio_mmc_pio.c
+++ b/drivers/mmc/host/tmio_mmc_pio.c
@@ -553,10 +553,11 @@ void tmio_mmc_do_data_irq(struct tmio_mmc_host *host)
 	}
 
 	if (stop) {
-		if (stop->opcode == MMC_STOP_TRANSMISSION && !stop->arg)
-			sd_ctrl_write16(host, CTL_STOP_INTERNAL_ACTION, 0);
-		else
-			BUG();
+		if (stop->opcode != MMC_STOP_TRANSMISSION || stop->arg)
+			dev_err(&host->pdev->dev, "unsupported stop: CMD%u,0x%x. We did CMD12,0\n",
+				stop->opcode, stop->arg);
+
+		sd_ctrl_write16(host, CTL_STOP_INTERNAL_ACTION, 0);
 	}
 
 	schedule_work(&host->done);
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v2 4/4] mmc: host: tmio: fill in response from auto cmd12
  2017-02-13 18:03 [PATCH v2 0/4] mmc: host: tmio: properly report status from autocmd12 Wolfram Sang
                   ` (2 preceding siblings ...)
  2017-02-13 18:03 ` [PATCH v2 3/4] mmc: host: tmio: don't BUG on unsupported stop commands Wolfram Sang
@ 2017-02-13 18:03 ` Wolfram Sang
  2017-02-14 10:06   ` Yoshihiro Shimoda
  3 siblings, 1 reply; 13+ messages in thread
From: Wolfram Sang @ 2017-02-13 18:03 UTC (permalink / raw)
  To: linux-mmc
  Cc: linux-renesas-soc, Yoshihiro Shimoda, Wolfram Sang, Simon Horman

After we received the dataend interrupt, R1 response register carries
the value from the automatically generated stop command. Report that
info back to the MMC block layer, so we will be notified in case of e.g.
ECC errors which happened during the last transfer.

Reviewed-by: Simon Horman <horms+renesas@verge.net.au>
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/mmc/host/tmio_mmc_pio.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/mmc/host/tmio_mmc_pio.c b/drivers/mmc/host/tmio_mmc_pio.c
index b47dd9195fe3fe..a08db28b0100d6 100644
--- a/drivers/mmc/host/tmio_mmc_pio.c
+++ b/drivers/mmc/host/tmio_mmc_pio.c
@@ -557,6 +557,9 @@ void tmio_mmc_do_data_irq(struct tmio_mmc_host *host)
 			dev_err(&host->pdev->dev, "unsupported stop: CMD%u,0x%x. We did CMD12,0\n",
 				stop->opcode, stop->arg);
 
+		/* fill in response from auto CMD12 */
+		stop->resp[0] = sd_ctrl_read16_and_16_as_32(host, CTL_RESPONSE);
+
 		sd_ctrl_write16(host, CTL_STOP_INTERNAL_ACTION, 0);
 	}
 
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* RE: [PATCH v2 4/4] mmc: host: tmio: fill in response from auto cmd12
  2017-02-13 18:03 ` [PATCH v2 4/4] mmc: host: tmio: fill in response from auto cmd12 Wolfram Sang
@ 2017-02-14 10:06   ` Yoshihiro Shimoda
  2017-02-14 10:52     ` Wolfram Sang
  0 siblings, 1 reply; 13+ messages in thread
From: Yoshihiro Shimoda @ 2017-02-14 10:06 UTC (permalink / raw)
  To: Wolfram Sang, linux-mmc; +Cc: linux-renesas-soc, Simon Horman

Hi,

> From: Wolfram Sang [mailto:wsa+renesas@sang-engineering.com]
> Sent: Tuesday, February 14, 2017 3:04 AM
> 
> After we received the dataend interrupt, R1 response register carries
> the value from the automatically generated stop command. Report that
> info back to the MMC block layer, so we will be notified in case of e.g.
> ECC errors which happened during the last transfer.
> 
> Reviewed-by: Simon Horman <horms+renesas@verge.net.au>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

I tested this patch with a SD tester (SGDK320).
As the commit log, this patch could pass the R1 response. So,

Tested-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>


However, I think the MMC block layer should check the brq->stop.resp[0]
because brq->stop.error should be zero in this case and mmc_blk_cmd_recovery()
is not called in mmc_blk_err_check().

Best regards,
Yoshihiro Shimoda

> ---
>  drivers/mmc/host/tmio_mmc_pio.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/mmc/host/tmio_mmc_pio.c b/drivers/mmc/host/tmio_mmc_pio.c
> index b47dd9195fe3fe..a08db28b0100d6 100644
> --- a/drivers/mmc/host/tmio_mmc_pio.c
> +++ b/drivers/mmc/host/tmio_mmc_pio.c
> @@ -557,6 +557,9 @@ void tmio_mmc_do_data_irq(struct tmio_mmc_host *host)
>  			dev_err(&host->pdev->dev, "unsupported stop: CMD%u,0x%x. We did CMD12,0\n",
>  				stop->opcode, stop->arg);
> 
> +		/* fill in response from auto CMD12 */
> +		stop->resp[0] = sd_ctrl_read16_and_16_as_32(host, CTL_RESPONSE);
> +
>  		sd_ctrl_write16(host, CTL_STOP_INTERNAL_ACTION, 0);
>  	}
> 
> --
> 2.11.0

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 4/4] mmc: host: tmio: fill in response from auto cmd12
  2017-02-14 10:06   ` Yoshihiro Shimoda
@ 2017-02-14 10:52     ` Wolfram Sang
  2017-02-15 10:19       ` Ulf Hansson
  0 siblings, 1 reply; 13+ messages in thread
From: Wolfram Sang @ 2017-02-14 10:52 UTC (permalink / raw)
  To: Yoshihiro Shimoda
  Cc: Wolfram Sang, linux-mmc, linux-renesas-soc, Simon Horman

[-- Attachment #1: Type: text/plain, Size: 1456 bytes --]

Shimoda-san, Ulf,

On Tue, Feb 14, 2017 at 10:06:47AM +0000, Yoshihiro Shimoda wrote:
> Hi,
> 
> > From: Wolfram Sang [mailto:wsa+renesas@sang-engineering.com]
> > Sent: Tuesday, February 14, 2017 3:04 AM
> > 
> > After we received the dataend interrupt, R1 response register carries
> > the value from the automatically generated stop command. Report that
> > info back to the MMC block layer, so we will be notified in case of e.g.
> > ECC errors which happened during the last transfer.
> > 
> > Reviewed-by: Simon Horman <horms+renesas@verge.net.au>
> > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> 
> I tested this patch with a SD tester (SGDK320).
> As the commit log, this patch could pass the R1 response. So,
> 
> Tested-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>

Thank you very much for testing!

> However, I think the MMC block layer should check the brq->stop.resp[0]
> because brq->stop.error should be zero in this case and mmc_blk_cmd_recovery()
> is not called in mmc_blk_err_check().

I see. Ulf, do you think it makes sense to extend the condition when to
call mmc_blk_cmd_recovery() with checking if stop.resp[0] has one of the
R1_* bits set which are marked with 'ex' (and probably 'erx', too)? I
agree with Shimoda-san, that the core is a good place to do it, since it
is about parsing the R1 and not the status bits of the host hardware.

Regards,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 4/4] mmc: host: tmio: fill in response from auto cmd12
  2017-02-14 10:52     ` Wolfram Sang
@ 2017-02-15 10:19       ` Ulf Hansson
  2017-02-15 15:02         ` Wolfram Sang
  0 siblings, 1 reply; 13+ messages in thread
From: Ulf Hansson @ 2017-02-15 10:19 UTC (permalink / raw)
  To: Wolfram Sang
  Cc: Yoshihiro Shimoda, Wolfram Sang, linux-mmc, linux-renesas-soc,
	Simon Horman

On 14 February 2017 at 11:52, Wolfram Sang <wsa@the-dreams.de> wrote:
> Shimoda-san, Ulf,
>
> On Tue, Feb 14, 2017 at 10:06:47AM +0000, Yoshihiro Shimoda wrote:
>> Hi,
>>
>> > From: Wolfram Sang [mailto:wsa+renesas@sang-engineering.com]
>> > Sent: Tuesday, February 14, 2017 3:04 AM
>> >
>> > After we received the dataend interrupt, R1 response register carries
>> > the value from the automatically generated stop command. Report that
>> > info back to the MMC block layer, so we will be notified in case of e.g.
>> > ECC errors which happened during the last transfer.
>> >
>> > Reviewed-by: Simon Horman <horms+renesas@verge.net.au>
>> > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
>>
>> I tested this patch with a SD tester (SGDK320).
>> As the commit log, this patch could pass the R1 response. So,
>>
>> Tested-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
>
> Thank you very much for testing!
>
>> However, I think the MMC block layer should check the brq->stop.resp[0]
>> because brq->stop.error should be zero in this case and mmc_blk_cmd_recovery()
>> is not called in mmc_blk_err_check().
>
> I see. Ulf, do you think it makes sense to extend the condition when to
> call mmc_blk_cmd_recovery() with checking if stop.resp[0] has one of the
> R1_* bits set which are marked with 'ex' (and probably 'erx', too)? I
> agree with Shimoda-san, that the core is a good place to do it, since it
> is about parsing the R1 and not the status bits of the host hardware.

The method we use to indicate a stop command error to the mmc core, is
to set ->stop.error in the host driver before completing the request.
Perhaps set it to -EIO or -EILSEQ.

In that way mmc_blk_err_check() sees the error and invokes the
mmc_blk_cmd_recovery() to deal with it (response parsing etc).

Does that work for you?

Kind regards
Uffe

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 4/4] mmc: host: tmio: fill in response from auto cmd12
  2017-02-15 10:19       ` Ulf Hansson
@ 2017-02-15 15:02         ` Wolfram Sang
  2017-02-16  7:57           ` Ulf Hansson
  0 siblings, 1 reply; 13+ messages in thread
From: Wolfram Sang @ 2017-02-15 15:02 UTC (permalink / raw)
  To: Ulf Hansson
  Cc: Yoshihiro Shimoda, Wolfram Sang, linux-mmc, linux-renesas-soc,
	Simon Horman

[-- Attachment #1: Type: text/plain, Size: 1271 bytes --]


> > I see. Ulf, do you think it makes sense to extend the condition when to
> > call mmc_blk_cmd_recovery() with checking if stop.resp[0] has one of the
> > R1_* bits set which are marked with 'ex' (and probably 'erx', too)? I
> > agree with Shimoda-san, that the core is a good place to do it, since it
> > is about parsing the R1 and not the status bits of the host hardware.
> 
> The method we use to indicate a stop command error to the mmc core, is
> to set ->stop.error in the host driver before completing the request.
> Perhaps set it to -EIO or -EILSEQ.
> 
> In that way mmc_blk_err_check() sees the error and invokes the
> mmc_blk_cmd_recovery() to deal with it (response parsing etc).
> 
> Does that work for you?

It would work, yes. Since R1 response format is hardware independent, I
wondered if checking for ECC errors wouldn't be better suited in the
core. We roughly need something like this:

	if (stop.resp[0] & R1_CARD_ECC_FAILED)
		stop.error = -EIO;

We can copy this into every driver, of course. Yet, I wondered if we
couldn't have a helper function mapping the R1 error bits to an
apropriate error value and call that just before the check in
mmc_blk_err_check().

Do you get what I mean?

Thanks,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 4/4] mmc: host: tmio: fill in response from auto cmd12
  2017-02-15 15:02         ` Wolfram Sang
@ 2017-02-16  7:57           ` Ulf Hansson
  2017-02-16  8:37             ` Wolfram Sang
  0 siblings, 1 reply; 13+ messages in thread
From: Ulf Hansson @ 2017-02-16  7:57 UTC (permalink / raw)
  To: Wolfram Sang
  Cc: Yoshihiro Shimoda, Wolfram Sang, linux-mmc, linux-renesas-soc,
	Simon Horman

On 15 February 2017 at 16:02, Wolfram Sang <wsa@the-dreams.de> wrote:
>
>> > I see. Ulf, do you think it makes sense to extend the condition when to
>> > call mmc_blk_cmd_recovery() with checking if stop.resp[0] has one of the
>> > R1_* bits set which are marked with 'ex' (and probably 'erx', too)? I
>> > agree with Shimoda-san, that the core is a good place to do it, since it
>> > is about parsing the R1 and not the status bits of the host hardware.
>>
>> The method we use to indicate a stop command error to the mmc core, is
>> to set ->stop.error in the host driver before completing the request.
>> Perhaps set it to -EIO or -EILSEQ.
>>
>> In that way mmc_blk_err_check() sees the error and invokes the
>> mmc_blk_cmd_recovery() to deal with it (response parsing etc).
>>
>> Does that work for you?
>
> It would work, yes. Since R1 response format is hardware independent, I
> wondered if checking for ECC errors wouldn't be better suited in the
> core. We roughly need something like this:
>
>         if (stop.resp[0] & R1_CARD_ECC_FAILED)
>                 stop.error = -EIO;
>
> We can copy this into every driver, of course. Yet, I wondered if we
> couldn't have a helper function mapping the R1 error bits to an
> apropriate error value and call that just before the check in
> mmc_blk_err_check().
>
> Do you get what I mean?

I get it - and yes you have a point.

By looking at the code in mmc_blk_err_check() and
mmc_blk_cmd_recovery(), it deserves a clean-up. That said, I don't
want to treat R1_CARD_ECC_FAILED as a special case.

So if you decide to add this check in the core (which I am open to),
we should also add checks the other potential R1 errors, to be
consistent.

Kind regards
Uffe

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 4/4] mmc: host: tmio: fill in response from auto cmd12
  2017-02-16  7:57           ` Ulf Hansson
@ 2017-02-16  8:37             ` Wolfram Sang
  2017-02-16  8:57               ` Ulf Hansson
  0 siblings, 1 reply; 13+ messages in thread
From: Wolfram Sang @ 2017-02-16  8:37 UTC (permalink / raw)
  To: Ulf Hansson
  Cc: Yoshihiro Shimoda, Wolfram Sang, linux-mmc, linux-renesas-soc,
	Simon Horman

[-- Attachment #1: Type: text/plain, Size: 2264 bytes --]

Hi Ulf,

On Thu, Feb 16, 2017 at 08:57:36AM +0100, Ulf Hansson wrote:
> On 15 February 2017 at 16:02, Wolfram Sang <wsa@the-dreams.de> wrote:
> >
> >> > I see. Ulf, do you think it makes sense to extend the condition when to
> >> > call mmc_blk_cmd_recovery() with checking if stop.resp[0] has one of the
> >> > R1_* bits set which are marked with 'ex' (and probably 'erx', too)? I
> >> > agree with Shimoda-san, that the core is a good place to do it, since it
> >> > is about parsing the R1 and not the status bits of the host hardware.
> >>
> >> The method we use to indicate a stop command error to the mmc core, is
> >> to set ->stop.error in the host driver before completing the request.
> >> Perhaps set it to -EIO or -EILSEQ.
> >>
> >> In that way mmc_blk_err_check() sees the error and invokes the
> >> mmc_blk_cmd_recovery() to deal with it (response parsing etc).
> >>
> >> Does that work for you?
> >
> > It would work, yes. Since R1 response format is hardware independent, I
> > wondered if checking for ECC errors wouldn't be better suited in the
> > core. We roughly need something like this:
> >
> >         if (stop.resp[0] & R1_CARD_ECC_FAILED)
> >                 stop.error = -EIO;
> >
> > We can copy this into every driver, of course. Yet, I wondered if we
> > couldn't have a helper function mapping the R1 error bits to an
> > apropriate error value and call that just before the check in
> > mmc_blk_err_check().
> >
> > Do you get what I mean?
> 
> I get it - and yes you have a point.

Cool.

> By looking at the code in mmc_blk_err_check() and
> mmc_blk_cmd_recovery(), it deserves a clean-up. That said, I don't

What do you mean with clean-up here? I would have just added the helper
function checking R1 error bits and setting stop.error accordingly.

> want to treat R1_CARD_ECC_FAILED as a special case.
> 
> So if you decide to add this check in the core (which I am open to),
> we should also add checks the other potential R1 errors, to be
> consistent.

I agree. That's what I meant with "checking if stop.resp[0] has one of
the R1_* bits set which are marked with 'ex' (and probably 'erx',
too)?". I think these are the candidates we care about.

Thanks,

   Wolfram

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 4/4] mmc: host: tmio: fill in response from auto cmd12
  2017-02-16  8:37             ` Wolfram Sang
@ 2017-02-16  8:57               ` Ulf Hansson
  0 siblings, 0 replies; 13+ messages in thread
From: Ulf Hansson @ 2017-02-16  8:57 UTC (permalink / raw)
  To: Wolfram Sang
  Cc: Yoshihiro Shimoda, Wolfram Sang, linux-mmc, linux-renesas-soc,
	Simon Horman

On 16 February 2017 at 09:37, Wolfram Sang <wsa@the-dreams.de> wrote:
> Hi Ulf,
>
> On Thu, Feb 16, 2017 at 08:57:36AM +0100, Ulf Hansson wrote:
>> On 15 February 2017 at 16:02, Wolfram Sang <wsa@the-dreams.de> wrote:
>> >
>> >> > I see. Ulf, do you think it makes sense to extend the condition when to
>> >> > call mmc_blk_cmd_recovery() with checking if stop.resp[0] has one of the
>> >> > R1_* bits set which are marked with 'ex' (and probably 'erx', too)? I
>> >> > agree with Shimoda-san, that the core is a good place to do it, since it
>> >> > is about parsing the R1 and not the status bits of the host hardware.
>> >>
>> >> The method we use to indicate a stop command error to the mmc core, is
>> >> to set ->stop.error in the host driver before completing the request.
>> >> Perhaps set it to -EIO or -EILSEQ.
>> >>
>> >> In that way mmc_blk_err_check() sees the error and invokes the
>> >> mmc_blk_cmd_recovery() to deal with it (response parsing etc).
>> >>
>> >> Does that work for you?
>> >
>> > It would work, yes. Since R1 response format is hardware independent, I
>> > wondered if checking for ECC errors wouldn't be better suited in the
>> > core. We roughly need something like this:
>> >
>> >         if (stop.resp[0] & R1_CARD_ECC_FAILED)
>> >                 stop.error = -EIO;
>> >
>> > We can copy this into every driver, of course. Yet, I wondered if we
>> > couldn't have a helper function mapping the R1 error bits to an
>> > apropriate error value and call that just before the check in
>> > mmc_blk_err_check().
>> >
>> > Do you get what I mean?
>>
>> I get it - and yes you have a point.
>
> Cool.
>
>> By looking at the code in mmc_blk_err_check() and
>> mmc_blk_cmd_recovery(), it deserves a clean-up. That said, I don't
>
> What do you mean with clean-up here? I would have just added the helper

...perhaps some re-factoring as the functions do lots of stuff.

> function checking R1 error bits and setting stop.error accordingly.

That's ok, I don't require you to do the clean up, but it would be nice. :-)

>
>> want to treat R1_CARD_ECC_FAILED as a special case.
>>
>> So if you decide to add this check in the core (which I am open to),
>> we should also add checks the other potential R1 errors, to be
>> consistent.
>
> I agree. That's what I meant with "checking if stop.resp[0] has one of
> the R1_* bits set which are marked with 'ex' (and probably 'erx',
> too)?". I think these are the candidates we care about.
>
> Thanks,
>
>    Wolfram

Kind regards
Uffe

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 2/4] mmc: host: tmio: fix minor typos in comments
  2017-02-13 18:03 ` [PATCH v2 2/4] mmc: host: tmio: fix minor typos in comments Wolfram Sang
@ 2017-02-16 15:03   ` Simon Horman
  0 siblings, 0 replies; 13+ messages in thread
From: Simon Horman @ 2017-02-16 15:03 UTC (permalink / raw)
  To: Wolfram Sang; +Cc: linux-mmc, linux-renesas-soc, Yoshihiro Shimoda

On Mon, Feb 13, 2017 at 07:03:40PM +0100, Wolfram Sang wrote:
> Making sure we match the actual register names.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Reviewed-by: Simon Horman <horms+renesas@verge.net.au>

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2017-02-16 15:04 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-02-13 18:03 [PATCH v2 0/4] mmc: host: tmio: properly report status from autocmd12 Wolfram Sang
2017-02-13 18:03 ` [PATCH v2 1/4] mmc: host: tmio: use defines for CTL_STOP_INTERNAL_ACTION values Wolfram Sang
2017-02-13 18:03 ` [PATCH v2 2/4] mmc: host: tmio: fix minor typos in comments Wolfram Sang
2017-02-16 15:03   ` Simon Horman
2017-02-13 18:03 ` [PATCH v2 3/4] mmc: host: tmio: don't BUG on unsupported stop commands Wolfram Sang
2017-02-13 18:03 ` [PATCH v2 4/4] mmc: host: tmio: fill in response from auto cmd12 Wolfram Sang
2017-02-14 10:06   ` Yoshihiro Shimoda
2017-02-14 10:52     ` Wolfram Sang
2017-02-15 10:19       ` Ulf Hansson
2017-02-15 15:02         ` Wolfram Sang
2017-02-16  7:57           ` Ulf Hansson
2017-02-16  8:37             ` Wolfram Sang
2017-02-16  8:57               ` Ulf Hansson

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.