From patchwork Fri May 24 05:07:51 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Peter Crosthwaite X-Patchwork-Id: 246055 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [208.118.235.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id 944232C0182 for ; Fri, 24 May 2013 15:08:23 +1000 (EST) Received: from localhost ([::1]:42111 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UfkEn-0005g2-EV for incoming@patchwork.ozlabs.org; Fri, 24 May 2013 01:08:21 -0400 Received: from eggs.gnu.org ([208.118.235.92]:42126) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UfkES-0005fh-Om for qemu-devel@nongnu.org; Fri, 24 May 2013 01:08:02 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UfkEL-000611-Fl for qemu-devel@nongnu.org; Fri, 24 May 2013 01:08:00 -0400 Received: from mail-bk0-x22e.google.com ([2a00:1450:4008:c01::22e]:37150) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UfkEL-00060M-5n for qemu-devel@nongnu.org; Fri, 24 May 2013 01:07:53 -0400 Received: by mail-bk0-f46.google.com with SMTP id my13so2271050bkb.5 for ; Thu, 23 May 2013 22:07:51 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=mime-version:sender:in-reply-to:references:date :x-google-sender-auth:message-id:subject:from:to:cc:content-type :x-gm-message-state; bh=X1R80a2LO/nwcvFWn6VkQcclvfadFGIIIr9Fy7c30XI=; b=jqnPOWjkD1tuKUCHggm/5oxpbHp09MMWEWuWb9zMj18Gh/9Cxf/aeyecl28EPbHGO6 ekAqJdQ4Ls2xtcskRdY7MQ+FQ71ATr82JKw8n2PpzGWau4q28hiV33OFIwDRbohVvECO /Fcu0KidF6rRQpTN/0ap9VpmXNgeWbWkHMsf88dR4zs7ooVKsCXg7FlOJTPc32b5vlmw jbd0CHrt9/UeqcDkKUQV+U5b2QU9HqHa7sCOZLXvWtcRmRKkgUK5ONiBDmd9bNR5mwr3 ZpCqwVoxw+M5heyUHleSrju+uOk8TYfwRj1xIBexOMLaKckzrLoxHnPdqwdC/xG6yQIE OIWA== MIME-Version: 1.0 X-Received: by 10.205.34.132 with SMTP id ss4mr8324115bkb.125.1369372071713; Thu, 23 May 2013 22:07:51 -0700 (PDT) Received: by 10.204.35.136 with HTTP; Thu, 23 May 2013 22:07:51 -0700 (PDT) In-Reply-To: References: Date: Fri, 24 May 2013 15:07:51 +1000 X-Google-Sender-Auth: 4IEFUdfiYRi3Nh4iGZXui6TvcDc Message-ID: From: Peter Crosthwaite To: Igor Mitsyanko X-Gm-Message-State: ALoCoQkZLW5gTVKpEWqJdH1qSETbM1ZDgdLuv5Q0LnliVZkL4fRwNw+WETXwtqHgVL8JplGbrS70 X-detected-operating-system: by eggs.gnu.org: Error: Malformed IPv6 address (bad octet value). X-Received-From: 2a00:1450:4008:c01::22e Cc: Peter Maydell , qemu-devel@nongnu.org, "Edgar E. Iglesias" Subject: Re: [Qemu-devel] [PATCH arm-devs v1 1/5] sd/sd.c: Fix "inquiry" ACMD41 X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Hi Igor, On Thu, May 23, 2013 at 8:31 PM, Igor Mitsyanko wrote: > On 05/23/2013 03:42 AM, Peter Crosthwaite wrote: >> Hi Igor, >> >> On Wed, May 22, 2013 at 11:37 PM, Igor Mitsyanko wrote: >>> >>> On 05/21/2013 10:50 AM, peter.crosthwaite@xilinx.com wrote: >>> >>> From: Peter Crosthwaite >>> >>> the SD command ACMD41 can be used in a read only mode to query device >>> state without doing the SD card initialisation. This is valid even >>> which the device is already initialised. Fix the command to be >>> responsive when in the ready state accordingly. >>> >>> Signed-off-by: Peter Crosthwaite >>> --- >>> >>> hw/sd/sd.c | 1 + >>> 1 file changed, 1 insertion(+) >>> >>> diff --git a/hw/sd/sd.c b/hw/sd/sd.c >>> index 2e0ef3e..89bfb7a 100644 >>> --- a/hw/sd/sd.c >>> +++ b/hw/sd/sd.c >>> @@ -1277,6 +1277,7 @@ static sd_rsp_type_t sd_app_command(SDState *sd, >>> } >>> switch (sd->state) { >>> case sd_idle_state: >>> + case sd_ready_state: >>> /* We accept any voltage. 10000 V is nothing. */ >>> if (req.arg) >>> sd->state = sd_ready_state; >>> >>> >>> I couldn't find any info in SD specification that would confirm this change >>> correctness, what about >>> table "Table 4-29: Card State Transition Table" which states that ACMD41 is >>> illegal in "ready" state? >>> >> >> By the letter of the spec I think you are right. Although this patch >> is needed to make my QEMU consistent with my real hardware. I'll dig >> deeper. >> > > Hello, Peter, after thinking some more about this, I assume that table > 4-29 might be incorrect. It depends on when idle->ready state transition > occurs, its not clear from specification. > > Controller issues first ACMD41 to start card's initialisation. Spec > states that this process could take up to 1sec, and all this time > controller should query card's busy state in a loop with ACMD41. After > response to ACMD41 has busy flag deasserted, card is considered to be > "ready". But this means that card was already in ready state when it > received last ACMD41 command, right? Unless card transitions to ready > state only after a response to last ACMD41 was sent. > This is exactly how it works. I did some experiments with a hacked up linux driver: break; @@ -175,13 +177,17 @@ int mmc_send_app_op_cond(struct mmc_host *host, u32 ocr, u32 *rocr) if (!(cmd.resp[0] & R1_SPI_IDLE)) break; } else { - if (cmd.resp[0] & MMC_CARD_BUSY) - break; + if (cmd.resp[0] & MMC_CARD_BUSY) { + busyness++; + printk(KERN_ALERT "busy returned\n"); + if (busyness > 5) { + break; + } + } } err = -ETIMEDOUT; - mmc_delay(10); } Basically the patch will cause the driver to send 5 more ACMD41s even after the (first) non-busy return. Real hardware (with a few different SD card manufacturers) borks on these extra ACMD41s: sdhci: Secure Digital Host Controller Interface driver sdhci: Copyright(c) Pierre Ossman sdhci-pltfm: SDHCI platform and OF driver helper mmc0: Invalid maximum block size, assuming 512 bytes mmc0: SDHCI controller on e0100000.ps7-sdio [e0100000.ps7-sdio] using ADMA usbcore: registered new interface driver usbhid usbhid: USB HID core driver TCP: cubic registered NET: Registered protocol family 10 sit: IPv6 over IPv4 tunneling driver NET: Registered protocol family 17 NET: Registered protocol family 40 VFP support v0.3: implementor 41 architecture 3 part 30 variant 9 rev 4 Registering SWP/SWPB emulation handler Freeing init memory: 6460K INIT: version 2.88 booting busy returned mmc0: error -110 whilst initialising SD card busy returned mmc0: error -110 whilst initialising SD card Starting Bootlog daemon: bootlogd. Creating /dev/flash/* device nodes busy returned mmc0: error -110 whilst initialising SD card busy returned mmc0: error -110 whilst initialising SD card QEMU before my patch is consistent with this behaviour (as expected). QEMU after my patch loses the errors (which is bad): sdhci: Secure Digital Host Controller Interface driver sdhci: Copyright(c) Pierre Ossman sdhci-pltfm: SDHCI platform and OF driver helper mmc0: SDHCI controller on e0100000.ps7-sdio [e0100000.ps7-sdio] using ADMA usbcore: registered new interface driver usbhid usbhid: USB HID core driver TCP: cubic registered NET: Registered protocol family 10 sit: IPv6 over IPv4 tunneling driver NET: Registered protocol family 17 NET: Registered protocol family 40 VFP support v0.3: implementor 41 architecture 3 part 30 variant 9 rev 0 Registering SWP/SWPB emulation handler busy returned busy returned busy returned busy returned busy returned busy returned mmc0: SD Status: Invalid Allocation Unit size. mmc0: new SD card at address 4567 Freeing init memory: 6460K mmcblk0: mmc0:4567 QEMU! 256 MiB Which only leaves your theory. The transition to ready state happens on the successful poll of ACMD41 and not before. That and ACMD41 is total illegal in ready state as documented. > If that's how real SD card behaves in your tests, then I think this > patch is OK, but it could benefit from a short comment explaining that > this behaviour is not covered by specification. > So it turns out my error-throwing guest was using an inquiry ACMD41 with non-zero bits 31:24 in the arg. QEMU as is, misinterprets this as a normal ("first") ACMD41 which is wrong. So my SD was getting initialised ahead of time and QEMU was incorrectly putting my SD in the ready state (rather than the read state being misbehaved as stated by this patch). So the next version of the patch is very different and fixes the ACMD41 inquiry vs first logic (but oddly the same subject line). I've dropped the R.B. tags, as its fundamentally a different patch. V2 on list. Regards, Peter > > Reviewed-by: Igor Mitsyanko > > >> Regards, >> Peter >> >>> -- >>> Best wishes, >>> Igor Mitsyanko >>> email: i.mitsyanko@gmail.com >> >> > > > -- > Best wishes, > Igor Mitsyanko > email: i.mitsyanko@gmail.com > Reviewed-by: Igor Mitsyanko --- a/drivers/mmc/core/sd_ops.c +++ b/drivers/mmc/core/sd_ops.c @@ -161,7 +161,9 @@ int mmc_send_app_op_cond(struct mmc_host *host, u32 ocr, u32 *rocr) cmd.arg = ocr; cmd.flags = MMC_RSP_SPI_R1 | MMC_RSP_R3 | MMC_CMD_BCR; - for (i = 100; i; i--) { + int busyness = 0; + for (i = 150; i; i--) { + mmc_delay(10); err = mmc_wait_for_app_cmd(host, NULL, &cmd, MMC_CMD_RETRIES); if (err)