diff mbox

[v2,1/3] mmc: tmio: correct treatment of errors during tuning

Message ID 20180119133906.11280-2-horms+renesas@verge.net.au (mailing list archive)
State Superseded
Delegated to: Geert Uytterhoeven
Headers show

Commit Message

Simon Horman Jan. 19, 2018, 1:39 p.m. UTC 
From: Masaharu Hayakawa <masaharu.hayakawa.ry@renesas.com>

If the return value of mmc_send_tuning() is error other than -EILSEQ, the
tuning fails and process goes out of for_loop.  But the correct processing
is to judge their TAP as bad.

Signed-off-by: Masaharu Hayakawa <masaharu.hayakawa.ry@renesas.com>
Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
---
v2 [Simon Horman]
* Added to patchset targeted at upstream
* Minor revision of changelog

v0 [Masaharu Hayakawa]
---
 drivers/mmc/host/tmio_mmc_core.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

Comments

Wolfram Sang Feb. 7, 2018, 9:52 p.m. UTC | #1 
On Fri, Jan 19, 2018 at 02:39:04PM +0100, Simon Horman wrote:
> From: Masaharu Hayakawa <masaharu.hayakawa.ry@renesas.com>
> 
> If the return value of mmc_send_tuning() is error other than -EILSEQ, the
> tuning fails and process goes out of for_loop.  But the correct processing
> is to judge their TAP as bad.

Ideally, we would have more specific reasons why this is correct processing.

What other codes could happen here?

> Signed-off-by: Masaharu Hayakawa <masaharu.hayakawa.ry@renesas.com>
> Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
> ---
> v2 [Simon Horman]
> * Added to patchset targeted at upstream
> * Minor revision of changelog
> 
> v0 [Masaharu Hayakawa]
> ---
>  drivers/mmc/host/tmio_mmc_core.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/mmc/host/tmio_mmc_core.c b/drivers/mmc/host/tmio_mmc_core.c
> index 6d8719be75a8..41767d33ef97 100644
> --- a/drivers/mmc/host/tmio_mmc_core.c
> +++ b/drivers/mmc/host/tmio_mmc_core.c
> @@ -800,10 +800,7 @@ static int tmio_mmc_execute_tuning(struct mmc_host *mmc, u32 opcode)
>  		if (host->prepare_tuning)
>  			host->prepare_tuning(host, i % host->tap_num);
>  
> -		ret = mmc_send_tuning(mmc, opcode, NULL);
> -		if (ret && ret != -EILSEQ)
> -			goto out;
> -		if (ret == 0)
> +		if (!mmc_send_tuning(mmc, opcode, NULL))

I'd prefer (mmc_send_tuning() == 0) here instead of '!mmc_send_tuning()'.
This reads as 'is ok' while the other reads more 'if not ok'.

>  			set_bit(i, host->taps);
>  
>  		usleep_range(1000, 1200);
> -- 
> 2.11.0
>
Simon Horman Feb. 13, 2018, 11:31 a.m. UTC | #2 
On Wed, Feb 07, 2018 at 10:52:52PM +0100, Wolfram Sang wrote:
> On Fri, Jan 19, 2018 at 02:39:04PM +0100, Simon Horman wrote:
> > From: Masaharu Hayakawa <masaharu.hayakawa.ry@renesas.com>
> > 
> > If the return value of mmc_send_tuning() is error other than -EILSEQ, the
> > tuning fails and process goes out of for_loop.  But the correct processing
> > is to judge their TAP as bad.
> 
> Ideally, we would have more specific reasons why this is correct processing.
> 
> What other codes could happen here?

I mistakenly attached log below following to patch 2/3 rather than 1/3 (this
patch) which explains my reason for including this patch in the series.

However, I am no longer able to reproduce this problem - I tried to do so
in order to work out which errors were being handled differently with
and without the patch.

I think we can drop this patch for now.



* M3-W ES1.0 / Salvator-X

[    1.812758] renesas_sdhi_internal_dmac ee100000.sd: Got CD GPIO
[    1.818778] renesas_sdhi_internal_dmac ee100000.sd: Got WP GPIO
[    1.874951] renesas_sdhi_internal_dmac ee140000.sd: mmc0 base at 0xee140000
+max clock rate 200 MHz
[    1.884950] renesas_sdhi_internal_dmac ee160000.sd: Got CD GPIO
[    1.891088] renesas_sdhi_internal_dmac ee160000.sd: Got WP GPIO
[    2.083508] mmc0: new HS400 MMC card at address 0001
[    2.084827] mmcblk0: mmc0:0001 eMMC   28.8 GiB
[    2.085234] mmcblk0boot0: mmc0:0001 eMMC   partition 1 4.00 MiB
[    2.085727] mmcblk0boot1: mmc0:0001 eMMC   partition 2 4.00 MiB
[    2.086398] mmcblk0rpmb: mmc0:0001 eMMC   partition 3 4.00 MiB, chardev
+(243:0)
[    2.097926]  mmcblk0: p1
[    2.360533] renesas_sdhi_internal_dmac ee100000.sd: Got CD GPIO
[    2.367633] renesas_sdhi_internal_dmac ee100000.sd: Got WP GPIO
[    2.424700] renesas_sdhi_internal_dmac ee100000.sd: mmc1 base at 0xee100000
+max clock rate 200 MHz
[    2.436021] renesas_sdhi_internal_dmac ee160000.sd: Got CD GPIO
[    2.443100] renesas_sdhi_internal_dmac ee160000.sd: Got WP GPIO

* On H3 ES2.0 / Salvator-XS:

[    2.452354] renesas_sdhi_internal_dmac ee100000.sd: Got CD GPIO
[    2.458344] renesas_sdhi_internal_dmac ee100000.sd: Got WP GPIO
[    2.513917] renesas_sdhi_internal_dmac ee140000.sd: mmc0 base at 0xee140000
+max clock rate 200 MHz
[    2.523564] renesas_sdhi_internal_dmac ee160000.sd: Got CD GPIO
[    2.529559] renesas_sdhi_internal_dmac ee160000.sd: Got WP GPIO
[    2.636678] renesas_sdhi_internal_dmac ee140000.sd: Tuning procedure failed
[    2.643739] mmc0: tuning execution failed: -5
[    2.648211] mmc0: error -5 whilst initialising MMC card
[    2.730078] renesas_sdhi_internal_dmac ee140000.sd: Tuning procedure failed
[    2.730085] mmc0: tuning execution failed: -5
[    2.730093] mmc0: error -5 whilst initialising MMC card
[    2.858718] renesas_sdhi_internal_dmac ee140000.sd: Tuning procedure failed
[    2.858725] mmc0: tuning execution failed: -5
[    2.858733] mmc0: error -5 whilst initialising MMC card
[    2.991258] renesas_sdhi_internal_dmac ee100000.sd: Got CD GPIO
[    2.998333] renesas_sdhi_internal_dmac ee100000.sd: Got WP GPIO
[    3.063121] renesas_sdhi_internal_dmac ee140000.sd: Tuning procedure failed
[    3.063128] mmc0: tuning execution failed: -5
[    3.063135] mmc0: error -5 whilst initialising MMC card
[    3.085170] renesas_sdhi_internal_dmac ee160000.sd: Got CD GPIO
[    3.092222] renesas_sdhi_internal_dmac ee160000.sd: Got WP GPIO

> 
> > Signed-off-by: Masaharu Hayakawa <masaharu.hayakawa.ry@renesas.com>
> > Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
> > ---
> > v2 [Simon Horman]
> > * Added to patchset targeted at upstream
> > * Minor revision of changelog
> > 
> > v0 [Masaharu Hayakawa]
> > ---
> >  drivers/mmc/host/tmio_mmc_core.c | 5 +----
> >  1 file changed, 1 insertion(+), 4 deletions(-)
> > 
> > diff --git a/drivers/mmc/host/tmio_mmc_core.c b/drivers/mmc/host/tmio_mmc_core.c
> > index 6d8719be75a8..41767d33ef97 100644
> > --- a/drivers/mmc/host/tmio_mmc_core.c
> > +++ b/drivers/mmc/host/tmio_mmc_core.c
> > @@ -800,10 +800,7 @@ static int tmio_mmc_execute_tuning(struct mmc_host *mmc, u32 opcode)
> >  		if (host->prepare_tuning)
> >  			host->prepare_tuning(host, i % host->tap_num);
> >  
> > -		ret = mmc_send_tuning(mmc, opcode, NULL);
> > -		if (ret && ret != -EILSEQ)
> > -			goto out;
> > -		if (ret == 0)
> > +		if (!mmc_send_tuning(mmc, opcode, NULL))
> 
> I'd prefer (mmc_send_tuning() == 0) here instead of '!mmc_send_tuning()'.
> This reads as 'is ok' while the other reads more 'if not ok'.
> 
> >  			set_bit(i, host->taps);
> >  
> >  		usleep_range(1000, 1200);
> > -- 
> > 2.11.0
> >
Wolfram Sang Feb. 13, 2018, 12:55 p.m. UTC | #3 
> I think we can drop this patch for now.

Nice! But we should keep it in mind and recall it, if issues pop up
later.
diff mbox

Patch

diff --git a/drivers/mmc/host/tmio_mmc_core.c b/drivers/mmc/host/tmio_mmc_core.c
index 6d8719be75a8..41767d33ef97 100644
--- a/drivers/mmc/host/tmio_mmc_core.c
+++ b/drivers/mmc/host/tmio_mmc_core.c
@@ -800,10 +800,7 @@  static int tmio_mmc_execute_tuning(struct mmc_host *mmc, u32 opcode)
 		if (host->prepare_tuning)
 			host->prepare_tuning(host, i % host->tap_num);
 
-		ret = mmc_send_tuning(mmc, opcode, NULL);
-		if (ret && ret != -EILSEQ)
-			goto out;
-		if (ret == 0)
+		if (!mmc_send_tuning(mmc, opcode, NULL))
 			set_bit(i, host->taps);
 
 		usleep_range(1000, 1200);