[RFT] mmc: tmio: fix CMD12 (STOP) handling

[RFT] mmc: tmio: fix CMD12 (STOP) handling

[Wolfram Sang]

I always anticipated this code to be not correct, but now I had a test
case to prove it. According to all documentation I have, setting the
TMIO_STOP_STP bit ever only worked during block transfers. This bit is
like manually enforcing an autocmd12 during a so far seamless transfer.
It does NOT work when the block transfer had errors. It also does NOT
work with any other cmd except block commands. For all those, CMD12 has
to be treated like any other command. So, basically, we could use this
bit only for mrq->data->stop cmds. But for these, we happily use the
autocmd12 feature using the TMIO_STOP_SEC bit. As a result, the above
bit is not useful for us and we need to treat CMD12 as a regular cmd
always. Just remove the special handling code. Note that the BSP
recognized this issue as well yet had a more cautious solution to the
problem [1]. Which is understandable but makes CMD12 handling even more
complicated.

Checked with a Renesas Salvator-X/M3-W which needed to send CMD12 when
retuning one of my SD cards.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/horms/renesas-bsp.git/commit/?id=2838a2ff8ca776f6d18b7fbbe75f3df8dd64183a

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---

So, this is a friendly call for further testing to make sure no regressions
happen. I also put the authors of the BSP patch to CC to check if this patch
also matches their use case. I hope this is fine for these people, please
accept my apologies if not. I just really like to have your opinion on this
patch.

Geert, Simon: any chance you can run this patch on your board farms?

In any case: as far as my understanding goes, if I am wrong with my
assumptions, the worst case is that for older SoCs CMD12 handling might become
a tad more inefficient because we now do in software what was expected to be
done in hardware. However, I am quite sure that the HW feature was always
over-estimated and CMD12 support is simply broken. For my test case and the BSP
patch above, it definately was.

Thanks for any assistance,

   Wolfram


 drivers/mmc/host/tmio_mmc_core.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/mmc/host/tmio_mmc_core.c b/drivers/mmc/host/tmio_mmc_core.c
index 1851c883bfc82a..fbcd56c58bce24 100644
--- a/drivers/mmc/host/tmio_mmc_core.c
+++ b/drivers/mmc/host/tmio_mmc_core.c
@@ -355,12 +355,6 @@ static int tmio_mmc_start_command(struct tmio_mmc_host *host,
 	int c = cmd->opcode;
 	u32 irq_mask = TMIO_MASK_CMD;
 
-	/* CMD12 is handled by hardware */
-	if (cmd->opcode == MMC_STOP_TRANSMISSION && !cmd->arg) {
-		sd_ctrl_write16(host, CTL_STOP_INTERNAL_ACTION, TMIO_STOP_STP);
-		return 0;
-	}
-
 	switch (mmc_resp_type(cmd)) {
 	case MMC_RSP_NONE: c |= RESP_NONE; break;
 	case MMC_RSP_R1:
-- 
2.11.0

Re: [RFT] mmc: tmio: fix CMD12 (STOP) handling

[Jan.Kloetzke]

On Mon, Jul 03, 2017 at 09:28:23PM +0200, Wolfram Sang wrote:
> I always anticipated this code to be not correct, but now I had a test
> case to prove it. According to all documentation I have, setting the
> TMIO_STOP_STP bit ever only worked during block transfers. This bit is
> like manually enforcing an autocmd12 during a so far seamless transfer.
> It does NOT work when the block transfer had errors. It also does NOT
> work with any other cmd except block commands. For all those, CMD12 has
> to be treated like any other command. So, basically, we could use this
> bit only for mrq->data->stop cmds. But for these, we happily use the
> autocmd12 feature using the TMIO_STOP_SEC bit. As a result, the above
> bit is not useful for us and we need to treat CMD12 as a regular cmd
> always. Just remove the special handling code. Note that the BSP
> recognized this issue as well yet had a more cautious solution to the
> problem [1]. Which is understandable but makes CMD12 handling even more
> complicated.
> 
> Checked with a Renesas Salvator-X/M3-W which needed to send CMD12 when
> retuning one of my SD cards.
> 
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/horms/renesas-bsp.git/commit/?id=2838a2ff8ca776f6d18b7fbbe75f3df8dd64183a
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
> 
> So, this is a friendly call for further testing to make sure no regressions
> happen. I also put the authors of the BSP patch to CC to check if this patch
> also matches their use case. I hope this is fine for these people, please
> accept my apologies if not. I just really like to have your opinion on this
> patch.

We've stumbled upon this issue some months ago and the patch below was
exactly the one that I came up with too. We've provided the feedback to
Renesas and they fixed it as [1]. As I'm laking proper data sheets for
the controller I was not able to judge it in depth, though.

Unfortunately I was not able to reproduce the original issue to test the
Renesas version [1] with enough confidence. What I can tell with
confidence is that the patch below does fix the issue that the driver
gets stuck on CMD12 and it did not have any further negative side
effects. So from my side (tested on RCar-M3):

Tested-by: Jan Klötzke <jan.kloetzke@preh.de>

Maybe someone from Reseas can comment on the rationale of their version
of the patch.

> 
> Geert, Simon: any chance you can run this patch on your board farms?
> 
> In any case: as far as my understanding goes, if I am wrong with my
> assumptions, the worst case is that for older SoCs CMD12 handling might become
> a tad more inefficient because we now do in software what was expected to be
> done in hardware. However, I am quite sure that the HW feature was always
> over-estimated and CMD12 support is simply broken. For my test case and the BSP
> patch above, it definately was.
> 
> Thanks for any assistance,
> 
>    Wolfram
> 
> 
>  drivers/mmc/host/tmio_mmc_core.c | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/drivers/mmc/host/tmio_mmc_core.c b/drivers/mmc/host/tmio_mmc_core.c
> index 1851c883bfc82a..fbcd56c58bce24 100644
> --- a/drivers/mmc/host/tmio_mmc_core.c
> +++ b/drivers/mmc/host/tmio_mmc_core.c
> @@ -355,12 +355,6 @@ static int tmio_mmc_start_command(struct tmio_mmc_host *host,
>  	int c = cmd->opcode;
>  	u32 irq_mask = TMIO_MASK_CMD;
>  
> -	/* CMD12 is handled by hardware */
> -	if (cmd->opcode == MMC_STOP_TRANSMISSION && !cmd->arg) {
> -		sd_ctrl_write16(host, CTL_STOP_INTERNAL_ACTION, TMIO_STOP_STP);
> -		return 0;
> -	}
> -
>  	switch (mmc_resp_type(cmd)) {
>  	case MMC_RSP_NONE: c |= RESP_NONE; break;
>  	case MMC_RSP_R1:
> -- 
> 2.11.0
> 

Re: [RFT] mmc: tmio: fix CMD12 (STOP) handling

[Geert Uytterhoeven]

Hi Wolfram,

On Mon, Jul 3, 2017 at 9:28 PM, Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> I always anticipated this code to be not correct, but now I had a test
> case to prove it. According to all documentation I have, setting the
> TMIO_STOP_STP bit ever only worked during block transfers. This bit is
> like manually enforcing an autocmd12 during a so far seamless transfer.
> It does NOT work when the block transfer had errors. It also does NOT
> work with any other cmd except block commands. For all those, CMD12 has
> to be treated like any other command. So, basically, we could use this
> bit only for mrq->data->stop cmds. But for these, we happily use the
> autocmd12 feature using the TMIO_STOP_SEC bit. As a result, the above
> bit is not useful for us and we need to treat CMD12 as a regular cmd
> always. Just remove the special handling code. Note that the BSP
> recognized this issue as well yet had a more cautious solution to the
> problem [1]. Which is understandable but makes CMD12 handling even more
> complicated.
>
> Checked with a Renesas Salvator-X/M3-W which needed to send CMD12 when
> retuning one of my SD cards.
>
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/horms/renesas-bsp.git/commit/?id=2838a2ff8ca776f6d18b7fbbe75f3df8dd64183a
>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>
> So, this is a friendly call for further testing to make sure no regressions
> happen. I also put the authors of the BSP patch to CC to check if this patch
> also matches their use case. I hope this is fine for these people, please
> accept my apologies if not. I just really like to have your opinion on this
> patch.
>
> Geert, Simon: any chance you can run this patch on your board farms?

I've booted all my boards with your patch applied, no visible differences.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

RE: [RFT] mmc: tmio: fix CMD12 (STOP) handling

[Yoshihiro Shimoda]

Hi Wolfram-san,

> -----Original Message-----
> From: Wolfram Sang
> Sent: Tuesday, July 4, 2017 4:28 AM
> 
> I always anticipated this code to be not correct, but now I had a test
> case to prove it. According to all documentation I have, setting the
> TMIO_STOP_STP bit ever only worked during block transfers. This bit is
> like manually enforcing an autocmd12 during a so far seamless transfer.
> It does NOT work when the block transfer had errors. It also does NOT
> work with any other cmd except block commands. For all those, CMD12 has
> to be treated like any other command. So, basically, we could use this
> bit only for mrq->data->stop cmds. But for these, we happily use the
> autocmd12 feature using the TMIO_STOP_SEC bit. As a result, the above
> bit is not useful for us and we need to treat CMD12 as a regular cmd
> always. Just remove the special handling code. Note that the BSP
> recognized this issue as well yet had a more cautious solution to the
> problem [1]. Which is understandable but makes CMD12 handling even more
> complicated.
> 
> Checked with a Renesas Salvator-X/M3-W which needed to send CMD12 when
> retuning one of my SD cards.
> 
> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/horms/renesas-bsp.git/commit/?id=2838a2ff8ca776f6d18b7fbbe75f3df8dd6
> 4183a
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
> 
> So, this is a friendly call for further testing to make sure no regressions
> happen. I also put the authors of the BSP patch to CC to check if this patch
> also matches their use case. I hope this is fine for these people, please
> accept my apologies if not. I just really like to have your opinion on this
> patch.
> 
> Geert, Simon: any chance you can run this patch on your board farms?
> 
> In any case: as far as my understanding goes, if I am wrong with my
> assumptions, the worst case is that for older SoCs CMD12 handling might become
> a tad more inefficient because we now do in software what was expected to be
> done in hardware. However, I am quite sure that the HW feature was always
> over-estimated and CMD12 support is simply broken. For my test case and the BSP
> patch above, it definately was.
> 
> Thanks for any assistance,
> 
>    Wolfram
> 
> 
>  drivers/mmc/host/tmio_mmc_core.c | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/drivers/mmc/host/tmio_mmc_core.c b/drivers/mmc/host/tmio_mmc_core.c
> index 1851c883bfc82a..fbcd56c58bce24 100644
> --- a/drivers/mmc/host/tmio_mmc_core.c
> +++ b/drivers/mmc/host/tmio_mmc_core.c
> @@ -355,12 +355,6 @@ static int tmio_mmc_start_command(struct tmio_mmc_host *host,
>  	int c = cmd->opcode;
>  	u32 irq_mask = TMIO_MASK_CMD;
> 
> -	/* CMD12 is handled by hardware */
> -	if (cmd->opcode == MMC_STOP_TRANSMISSION && !cmd->arg) {
> -		sd_ctrl_write16(host, CTL_STOP_INTERNAL_ACTION, TMIO_STOP_STP);
> -		return 0;
> -	}
> -

I and Dung-san are testing this patch.
However, this original code doesn't run by our test cases.
Do you know how this original code runs?
Anyway, we will try to make a test case somehow now.

Best regards,
Yoshihiro Shimoda

>  	switch (mmc_resp_type(cmd)) {
>  	case MMC_RSP_NONE: c |= RESP_NONE; break;
>  	case MMC_RSP_R1:
> --
> 2.11.0

Re: [RFT] mmc: tmio: fix CMD12 (STOP) handling

[Wolfram Sang]

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


> I and Dung-san are testing this patch.
> However, this original code doesn't run by our test cases.
> Do you know how this original code runs?
> Anyway, we will try to make a test case somehow now.

Thank you very much, Shimoda-san. I got it tested because a card needed
re-tuning during a transfer.

It might work if you manually induce an error bit in the driver
somewhere. I'll try to find a good sport later, too. However, I am away
from my boards today and tomorrow.


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

RE: [RFT] mmc: tmio: fix CMD12 (STOP) handling

[Yoshihiro Shimoda]

Hi Wolfram-san,

> -----Original Message-----
> From: Wolfram Sang
> Sent: Thursday, July 6, 2017 3:39 PM
> 
> > I and Dung-san are testing this patch.
> > However, this original code doesn't run by our test cases.
> > Do you know how this original code runs?
> > Anyway, we will try to make a test case somehow now.
> 
> Thank you very much, Shimoda-san. I got it tested because a card needed
> re-tuning during a transfer.
> 
> It might work if you manually induce an error bit in the driver
> somewhere. I'll try to find a good sport later, too. However, I am away
> from my boards today and tomorrow.

We tested this patch on H3 Salvator-X.
If we add debug code in this driver (e.g. 400th mmc request assumes an error),
this original code ran and the following message output:
===============
[  258.245349] mmcblk1: mmc1:e624 SU08G 7.40 GiB
[  258.259245]  mmcblk1: p1
[  268.513997] renesas_sdhi_internal_dmac ee100000.sd: timeout waiting 
for hardware interrupt (CMD12)
[  273.633998] renesas_sdhi_internal_dmac ee100000.sd: timeout waiting 
for hardware interrupt (CMD12)
[  278.754000] renesas_sdhi_internal_dmac ee100000.sd: timeout waiting 
for hardware interrupt (CMD12)
[  283.873995] renesas_sdhi_internal_dmac ee100000.sd: timeout waiting 
for hardware interrupt (CMD12)
[  288.994000] renesas_sdhi_internal_dmac ee100000.sd: timeout waiting 
for hardware interrupt (CMD12)
[  289.042012] mmcblk1: error -110 sending stop command
===============

And then, after we applied this patch, no regression happens and the following
message output:
===============
 [   66.433902] mmcblk1: mmc1:e624 SU08G 7.40 GiB
[   66.449161]  mmcblk1: p1
[   66.505322] mmcblk1: error -110 sending stop command
===============

So,

Tested-by: Nguyen Viet Dung <dung.nguyen.aj@renesas.com>

Best regards,
Yoshihiro Shimoda

Re: [RFT] mmc: tmio: fix CMD12 (STOP) handling

[Wolfram Sang]

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


> Tested-by: Nguyen Viet Dung <dung.nguyen.aj@renesas.com>

Great! Thank you very much!


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

Re: [RFT] mmc: tmio: fix CMD12 (STOP) handling

[Ulf Hansson]

On 3 July 2017 at 21:28, Wolfram Sang <wsa+renesas@sang-engineering.com> wrote:
> I always anticipated this code to be not correct, but now I had a test
> case to prove it. According to all documentation I have, setting the
> TMIO_STOP_STP bit ever only worked during block transfers. This bit is
> like manually enforcing an autocmd12 during a so far seamless transfer.
> It does NOT work when the block transfer had errors. It also does NOT
> work with any other cmd except block commands. For all those, CMD12 has
> to be treated like any other command. So, basically, we could use this
> bit only for mrq->data->stop cmds. But for these, we happily use the
> autocmd12 feature using the TMIO_STOP_SEC bit. As a result, the above
> bit is not useful for us and we need to treat CMD12 as a regular cmd
> always. Just remove the special handling code. Note that the BSP
> recognized this issue as well yet had a more cautious solution to the
> problem [1]. Which is understandable but makes CMD12 handling even more
> complicated.
>
> Checked with a Renesas Salvator-X/M3-W which needed to send CMD12 when
> retuning one of my SD cards.
>
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/horms/renesas-bsp.git/commit/?id=2838a2ff8ca776f6d18b7fbbe75f3df8dd64183a
>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Thanks, applied for next!

Should we add a stable tag?

Kind regards
Uffe

> ---
>
> So, this is a friendly call for further testing to make sure no regressions
> happen. I also put the authors of the BSP patch to CC to check if this patch
> also matches their use case. I hope this is fine for these people, please
> accept my apologies if not. I just really like to have your opinion on this
> patch.
>
> Geert, Simon: any chance you can run this patch on your board farms?
>
> In any case: as far as my understanding goes, if I am wrong with my
> assumptions, the worst case is that for older SoCs CMD12 handling might become
> a tad more inefficient because we now do in software what was expected to be
> done in hardware. However, I am quite sure that the HW feature was always
> over-estimated and CMD12 support is simply broken. For my test case and the BSP
> patch above, it definately was.
>
> Thanks for any assistance,
>
>    Wolfram
>
>
>  drivers/mmc/host/tmio_mmc_core.c | 6 ------
>  1 file changed, 6 deletions(-)
>
> diff --git a/drivers/mmc/host/tmio_mmc_core.c b/drivers/mmc/host/tmio_mmc_core.c
> index 1851c883bfc82a..fbcd56c58bce24 100644
> --- a/drivers/mmc/host/tmio_mmc_core.c
> +++ b/drivers/mmc/host/tmio_mmc_core.c
> @@ -355,12 +355,6 @@ static int tmio_mmc_start_command(struct tmio_mmc_host *host,
>         int c = cmd->opcode;
>         u32 irq_mask = TMIO_MASK_CMD;
>
> -       /* CMD12 is handled by hardware */
> -       if (cmd->opcode == MMC_STOP_TRANSMISSION && !cmd->arg) {
> -               sd_ctrl_write16(host, CTL_STOP_INTERNAL_ACTION, TMIO_STOP_STP);
> -               return 0;
> -       }
> -
>         switch (mmc_resp_type(cmd)) {
>         case MMC_RSP_NONE: c |= RESP_NONE; break;
>         case MMC_RSP_R1:
> --
> 2.11.0
>
> --
> 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: [RFT] mmc: tmio: fix CMD12 (STOP) handling

[Wolfram Sang]

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

On Tue, Jul 11, 2017 at 04:43:28PM +0200, Ulf Hansson wrote:
> On 3 July 2017 at 21:28, Wolfram Sang <wsa+renesas@sang-engineering.com> wrote:
> > I always anticipated this code to be not correct, but now I had a test
> > case to prove it. According to all documentation I have, setting the
> > TMIO_STOP_STP bit ever only worked during block transfers. This bit is
> > like manually enforcing an autocmd12 during a so far seamless transfer.
> > It does NOT work when the block transfer had errors. It also does NOT
> > work with any other cmd except block commands. For all those, CMD12 has
> > to be treated like any other command. So, basically, we could use this
> > bit only for mrq->data->stop cmds. But for these, we happily use the
> > autocmd12 feature using the TMIO_STOP_SEC bit. As a result, the above
> > bit is not useful for us and we need to treat CMD12 as a regular cmd
> > always. Just remove the special handling code. Note that the BSP
> > recognized this issue as well yet had a more cautious solution to the
> > problem [1]. Which is understandable but makes CMD12 handling even more
> > complicated.
> >
> > Checked with a Renesas Salvator-X/M3-W which needed to send CMD12 when
> > retuning one of my SD cards.
> >
> > [1] https://git.kernel.org/pub/scm/linux/kernel/git/horms/renesas-bsp.git/commit/?id=2838a2ff8ca776f6d18b7fbbe75f3df8dd64183a
> >
> > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> 
> Thanks, applied for next!
> 
> Should we add a stable tag?

Thanks! I'd like to wait until it gets further testing when it becomes
upstream.


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