From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754456AbcFTKOf (ORCPT ); Mon, 20 Jun 2016 06:14:35 -0400 Received: from mailout3.samsung.com ([203.254.224.33]:33904 "EHLO mailout3.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752131AbcFTKOM (ORCPT ); Mon, 20 Jun 2016 06:14:12 -0400 X-AuditID: cbfee68d-f79876d000001436-42-5767c1ea1a85 Message-id: <5767C1EA.10101@samsung.com> Date: Mon, 20 Jun 2016 19:14:02 +0900 From: Jaehoon Chung User-Agent: Mozilla/5.0 (X11; Linux i686; rv:31.0) Gecko/20100101 Thunderbird/31.6.0 MIME-version: 1.0 To: Enric Balletbo i Serra , Ulf Hansson , linux-mmc@vger.kernel.org, linux-kernel@vger.kernel.org Cc: Doug Anderson , Alim Akhtar Subject: Re: [RESEND PATCH v2 1/1] mmc: dw_mmc: Wait for data transfer after response errors. References: <1461657838-25484-1-git-send-email-enric.balletbo@collabora.com> <1461657838-25484-2-git-send-email-enric.balletbo@collabora.com> In-reply-to: <1461657838-25484-2-git-send-email-enric.balletbo@collabora.com> Content-type: text/plain; charset=windows-1252 Content-transfer-encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFprFIsWRmVeSWpSXmKPExsWyRsSkUPfVwfRwg8Mn+C2W3qq2OLvsIJvF mtuHGC0u75rDZnHkfz+jxfG14Q5sHrMbLrJ47Li7hNFj56y77B53ru1h8/i8SS6ANYrLJiU1 J7MstUjfLoErY/X05WwFa+QrdjU/Y2xg7JboYuTkkBAwkZh/u4sVwhaTuHBvPVsXIxeHkMAK RonOnbNZYIqW3nrPDpGYxSjx7loHE4TzgFFi/bpFjCBVvAIaEhMO72EHsVkEVCV+9K1jA7HZ BHQktn87zgRiiwqESTxYt5cVol5Q4sfke2AbRATmAA3qDgKxmQX8JFb0zwOq5+AQFoiXWDXT EWJXP6PErHWz2UHinAK+EmefioKYzAJ6EvcvakF0yktsXvOWGaRcQuAYu8TTvb0sEOcISHyb fIgFpF5CQFZi0wFmiL8kJQ6uuMEygVFsFpKDZiFMnYVk6gJG5lWMoqkFyQXFSelFhnrFibnF pXnpesn5uZsYgVF2+t+z3h2Mtw9YH2IU4GBU4uG1OJsWLsSaWFZcmXuI0RToiInMUqLJ+cBY ziuJNzQ2M7IwNTE1NjK3NFMS51WU+hksJJCeWJKanZpakFoUX1Sak1p8iJGJg1OqgdGt4ThD uamaztQHZx6tiY8PdBQ/V592RdjnyF+J9uvHdW1+65XK8P/k6VWXLT+uwbZGLtftyoXP6ucs m3wMYqt0DZ2Dq7jilrK7MEptjM+4/TSswe1U5Y2/7pbMi3n/M3JbJZmqLpBhmZ46O5pHqZ/V sjRTNZzrl9lb9vTHga/6+GW+xnoqsRRnJBpqMRcVJwIAAXFxr60CAAA= X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFnrOIsWRmVeSWpSXmKPExsVy+t9jQd1XB9PDDZrXcVksvVVtcXbZQTaL NbcPMVpc3jWHzeLI/35Gi+Nrwx3YPGY3XGTx2HF3CaPHzll32T3uXNvD5vF5k1wAa1QDo01G amJKapFCal5yfkpmXrqtkndwvHO8qZmBoa6hpYW5kkJeYm6qrZKLT4CuW2YO0AFKCmWJOaVA oYDE4mIlfTtME0JD3HQtYBojdH1DguB6jAzQQMIaxozV05ezFayRr9jV/IyxgbFboouRk0NC wERi6a337BC2mMSFe+vZuhi5OIQEZjFKvLvWwQThPGCUWL9uESNIFa+AhsSEw3vAOlgEVCV+ 9K1jA7HZBHQktn87zgRiiwqESTxYt5cVol5Q4sfkeywgtojAHKBB3UEgNrOAn8SK/nlA9Rwc wgLxEqtmOkLs6meUmLVuNjtInFPAV+LsU1EQk1lAT+L+RS2ITnmJzWveMk9gBDoSYcEshKpZ SKoWMDKvYpRILUguKE5KzzXMSy3XK07MLS7NS9dLzs/dxAiO5GdSOxgP7nI/xCjAwajEwzth Q3q4EGtiWXFl7iFGCQ5mJRHerXuBQrwpiZVVqUX58UWlOanFhxhNgSEwkVlKNDkfmGTySuIN jU3MjCyNzA0tjIzNlcR5H/9fFyYkkJ5YkpqdmlqQWgTTx8TBKdXAyBV98P45FenG7St9mcOP nLD7P9PTZusWwanv/VmyTv8U2vlryu05K5Vf5cWenXJ0xS4t33KbJ1HHFOpdk37PCVK1nBFQ c/rlk9uPmNa2cSqH9N5McTvBpll5JDz3gtGryMuThIp83+TdvShmeVGuPuCG0ay6Mw8uxXGG 6L3JfbtLY8VRqwap50osxRmJhlrMRcWJAD00Wwz6AgAA DLP-Filter: Pass X-MTR: 20000000000000000@CPGS X-CFilter-Loop: Reflected Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Enric, On 04/26/2016 05:03 PM, Enric Balletbo i Serra wrote: > From: Doug Anderson > > According to the DesignWare state machine description, after we get a > "response error" or "response CRC error" we move into data transfer > mode. That means that we don't necessarily need to special case > trying to deal with the failure right away. We can wait until we are > notified that the data transfer is complete (with or without errors) > and then we can deal with the failure. > > It may sound strange to defer dealing with a command that we know will > fail anyway, but this appears to fix a bug. During tuning (CMD19) on > a specific card on an rk3288-based system, we found that we could get > a "response CRC error". Sending the stop command after the "response > CRC error" would then throw the system into a confused state causing > all future tuning phases to report failure. I understood this patch what purpose has. Does it need to consider for other tuning cases? Best Regards, Jaehoon Chung > > When in the confused state, the controller would show these (hex codes > are interrupt status register): > CMD ERR: 0x00000046 (cmd=19) > CMD ERR: 0x0000004e (cmd=12) > DATA ERR: 0x00000208 > DATA ERR: 0x0000020c > CMD ERR: 0x00000104 (cmd=19) > CMD ERR: 0x00000104 (cmd=12) > DATA ERR: 0x00000208 > DATA ERR: 0x0000020c > ... > ... > > It is inherently difficult to deal with the complexity of trying to > correctly send a stop command while a data transfer is taking place > since you need to deal with different corner cases caused by the fact > that the data transfer could complete (with errors or without errors) > during various places in sending the stop command (dw_mci_stop_dma, > send_stop_abort, etc) > > Instead of adding a bunch of extra complexity to deal with this, it > seems much simpler to just use the more straightforward (and less > error-prone) path of letting the data transfer finish. There > shouldn't be any huge benefit to sending the stop command slightly > earlier, anyway. > > Signed-off-by: Doug Anderson > Signed-off-by: Enric Balletbo i Serra > Cc: Alim Akhtar > --- > Changelog since v1: > - Fix the issue found by Alim with exynos letting the data transfer > take place only when MMC_SEND_TUNING_BLOCK is issued. > > drivers/mmc/host/dw_mmc.c | 27 +++++++++++++++++++++++++++ > 1 file changed, 27 insertions(+) > > diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c > index 242f9a0..2ebeea8 100644 > --- a/drivers/mmc/host/dw_mmc.c > +++ b/drivers/mmc/host/dw_mmc.c > @@ -1761,6 +1761,33 @@ static void dw_mci_tasklet_func(unsigned long priv) > } > > if (cmd->data && err) { > + /* > + * During UHS tuning sequence, sending the stop > + * command after the response CRC error would > + * throw the system into a confused state > + * causing all future tuning phases to report > + * failure. > + * > + * In such case controller will move into a data > + * transfer state after a response error or > + * response CRC error. Let's let that finish > + * before trying to send a stop, so we'll go to > + * STATE_SENDING_DATA. > + * > + * Although letting the data transfer take place > + * will waste a bit of time (we already know > + * the command was bad), it can't cause any > + * errors since it's possible it would have > + * taken place anyway if this tasklet got > + * delayed. Allowing the transfer to take place > + * avoids races and keeps things simple. > + */ > + if ((err != -ETIMEDOUT) && > + (cmd->opcode == MMC_SEND_TUNING_BLOCK)) { > + state = STATE_SENDING_DATA; > + continue; > + } > + > dw_mci_stop_dma(host); > send_stop_abort(host, data); > state = STATE_SENDING_STOP; >