From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-x22d.google.com (mail-wm0-x22d.google.com [IPv6:2a00:1450:400c:c09::22d]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id EFA8A81DFC for ; Mon, 14 Nov 2016 09:15:24 -0800 (PST) Received: by mail-wm0-x22d.google.com with SMTP id g23so110354885wme.1 for ; Mon, 14 Nov 2016 09:15:29 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=mYaybk/spOw6Gndey3IAV7VbpvFXlNKSvk9voVzNftk=; b=YaILM210huibJ0RNLIeZyUpF/KtV/nmFOZgKyd2odPD2f7RwZZiQ3+Sld024oGYJJM tJq0XRjS0p9QAmpeVPMnHRcGlWdV6JrR/ZJjz6GGDJPerlK4mFYRVybD2sjL039Gxq/I fgphx66/ZRi+96pI4o01EGLeUGGT3CnGRD3bQ= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=mYaybk/spOw6Gndey3IAV7VbpvFXlNKSvk9voVzNftk=; b=e3Tx0OxjsSF/Ki/Tm7U/SxsDFuxev1euhy/y7HtDicNru6CLNmFs9yNHwBI2XnQ+gL cLwz1hsZNyIR+kYIU328adSk3l7um3OQBcl3UF10jyTOTGg+vcqdpoif4dibsDEWbSTi kr5LcFKDqDdWi/eT9/BzgQcqkdpRfKKPzXqs3REQq6WcY2vXEIeYIPoQyQ+zf5KNFyRM FAcaIdufATdhAfdQRJJL8CxGq+aLISRJC81teXMmLUi09mIUmZr2vGGznrIPdm8tzGEG fXIErn2ww3Vqp3DO+J/MeoWQjbBapN1aX//IA4LzZSpG+ftDZcvR/LKRS31xbFgOgKUY zzyw== X-Gm-Message-State: ABUngvcLPCgjaMhKupjDkJkj6kFsqZc1PbkDv2LAw7vJmr2PtbnOYsCgVX1UjDs/+UXHl7tl X-Received: by 10.194.31.201 with SMTP id c9mr19494392wji.176.1479143728063; Mon, 14 Nov 2016 09:15:28 -0800 (PST) Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id a13sm26843804wma.18.2016.11.14.09.15.27 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 14 Nov 2016 09:15:27 -0800 (PST) Date: Mon, 14 Nov 2016 17:15:25 +0000 From: Leif Lindholm To: Haojian Zhuang Cc: ryan.harkin@linaro.org, edk2-devel@lists.01.org, ard.biesheuvel@linaro.org Message-ID: <20161114171525.GA27644@bivouac.eciton.net> References: <1479019678-12621-1-git-send-email-haojian.zhuang@linaro.org> <1479019678-12621-9-git-send-email-haojian.zhuang@linaro.org> MIME-Version: 1.0 In-Reply-To: <1479019678-12621-9-git-send-email-haojian.zhuang@linaro.org> User-Agent: Mutt/1.5.23 (2014-03-12) Subject: Re: [PATCH v5 8/9] MmcDxe: expand to support multiple blocks X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 14 Nov 2016 17:15:25 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Sun, Nov 13, 2016 at 02:47:57PM +0800, Haojian Zhuang wrote: > Make use of DMA to transfer multiple blocks at one time. It could > improve the performance on MMC/SD driver. > > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Haojian Zhuang > Tested-by: Ryan Harkin > --- > EmbeddedPkg/Include/Protocol/MmcHost.h | 6 + > EmbeddedPkg/Universal/MmcDxe/MmcBlockIo.c | 177 ++++++++++++++++++++---------- > 2 files changed, 123 insertions(+), 60 deletions(-) > > diff --git a/EmbeddedPkg/Include/Protocol/MmcHost.h b/EmbeddedPkg/Include/Protocol/MmcHost.h > index 5028045..cd5335a 100644 > --- a/EmbeddedPkg/Include/Protocol/MmcHost.h > +++ b/EmbeddedPkg/Include/Protocol/MmcHost.h > @@ -62,6 +62,7 @@ typedef UINT32 MMC_CMD; > #define MMC_CMD20 (MMC_INDX(20) | MMC_CMD_WAIT_RESPONSE) > #define MMC_CMD23 (MMC_INDX(23) | MMC_CMD_WAIT_RESPONSE) > #define MMC_CMD24 (MMC_INDX(24) | MMC_CMD_WAIT_RESPONSE) > +#define MMC_CMD25 (MMC_INDX(25) | MMC_CMD_WAIT_RESPONSE) > #define MMC_CMD55 (MMC_INDX(55) | MMC_CMD_WAIT_RESPONSE) > #define MMC_ACMD41 (MMC_INDX(41) | MMC_CMD_WAIT_RESPONSE | MMC_CMD_NO_CRC_RESPONSE) > #define MMC_ACMD51 (MMC_INDX(51) | MMC_CMD_WAIT_RESPONSE) > @@ -152,6 +153,10 @@ typedef EFI_STATUS (EFIAPI *MMC_SETIOS) ( > IN UINT32 TimingMode > ); > > +typedef BOOLEAN (EFIAPI *MMC_ISMULTIBLOCK) ( > + IN EFI_MMC_HOST_PROTOCOL *This > + ); > + > > struct _EFI_MMC_HOST_PROTOCOL { > > @@ -169,6 +174,7 @@ struct _EFI_MMC_HOST_PROTOCOL { > MMC_WRITEBLOCKDATA WriteBlockData; > > MMC_SETIOS SetIos; > + MMC_ISMULTIBLOCK IsMultiBlock; > > }; > > diff --git a/EmbeddedPkg/Universal/MmcDxe/MmcBlockIo.c b/EmbeddedPkg/Universal/MmcDxe/MmcBlockIo.c > index 0e1ef57..1fcb3b5 100644 > --- a/EmbeddedPkg/Universal/MmcDxe/MmcBlockIo.c > +++ b/EmbeddedPkg/Universal/MmcDxe/MmcBlockIo.c > @@ -126,6 +126,95 @@ MmcStopTransmission ( > #define MMCI0_BLOCKLEN 512 > #define MMCI0_TIMEOUT 10000 > > +STATIC EFI_STATUS STATIC EFI_STATUS > +MmcTransferBlock ( > + IN EFI_BLOCK_IO_PROTOCOL *This, > + IN UINTN Cmd, > + IN UINTN Transfer, > + IN UINT32 MediaId, > + IN EFI_LBA Lba, > + IN UINTN BufferSize, > + OUT VOID *Buffer > + ) > +{ > + EFI_STATUS Status; > + UINTN CmdArg; > + INTN Timeout; > + UINT32 Response[4]; > + MMC_HOST_INSTANCE *MmcHostInstance; > + EFI_MMC_HOST_PROTOCOL *MmcHost; > + > + MmcHostInstance = MMC_HOST_INSTANCE_FROM_BLOCK_IO_THIS (This); > + MmcHost = MmcHostInstance->MmcHost; > + > + //Set command argument based on the card access mode (Byte mode or Block mode) > + if (MmcHostInstance->CardInfo.OCRData.AccessMode & BIT1) { The code would be more readable if instead of BIT1 there was a symbolic name of the flag we're testing for. (OK, I see the code has been moved from elsewhere in this form, but it would be good to make the code more clear.) > + CmdArg = Lba; > + } else { > + CmdArg = Lba * This->Media->BlockSize; > + } > + > + Status = MmcHost->SendCommand (MmcHost, Cmd, CmdArg); > + if (EFI_ERROR (Status)) { > + DEBUG ((EFI_D_ERROR, "%a(MMC_CMD%d): Error %r\n", __func__, Cmd, Status)); > + return Status; > + } > + > + if (Transfer == MMC_IOBLOCKS_READ) { > + // Read Data > + Status = MmcHost->ReadBlockData (MmcHost, Lba, BufferSize, Buffer); > + if (EFI_ERROR (Status)) { > + DEBUG ((EFI_D_BLKIO, "%a(): Error Read Block Data and Status = %r\n", __func__, Status)); > + MmcStopTransmission (MmcHost); > + return Status; > + } > + Status = MmcNotifyState (MmcHostInstance, MmcProgrammingState); > + if (EFI_ERROR (Status)) { > + DEBUG ((EFI_D_ERROR, "%a() : Error MmcProgrammingState\n", __func__)); > + return Status; > + } > + } else { > + // Write Data > + Status = MmcHost->WriteBlockData (MmcHost, Lba, BufferSize, Buffer); > + if (EFI_ERROR (Status)) { > + DEBUG ((EFI_D_BLKIO, "%a(): Error Write Block Data and Status = %r\n", __func__, Status)); > + MmcStopTransmission (MmcHost); > + return Status; > + } > + } > + > + // Command 13 - Read status and wait for programming to complete (return to tran) > + Timeout = MMCI0_TIMEOUT; > + CmdArg = MmcHostInstance->CardInfo.RCA << 16; > + Response[0] = 0; > + while(!(Response[0] & MMC_R0_READY_FOR_DATA) > + && (MMC_R0_CURRENTSTATE (Response) != MMC_R0_STATE_TRAN) > + && Timeout--) { > + Status = MmcHost->SendCommand (MmcHost, MMC_CMD13, CmdArg); > + if (!EFI_ERROR (Status)) { > + MmcHost->ReceiveResponse (MmcHost, MMC_RESPONSE_TYPE_R1, Response); > + if (Response[0] & MMC_R0_READY_FOR_DATA) { > + break; // Prevents delay once finished > + } > + } > + gBS->Stall (1); Can you provide a comment of why this stall was chosen? (Ah, again, moved code. Ignore.) > + } > + > + if (BufferSize > This->Media->BlockSize) { > + Status = MmcHost->SendCommand (MmcHost, MMC_CMD12, 0); > + if (EFI_ERROR (Status)) { > + DEBUG ((EFI_D_BLKIO, "%a(): Error and Status:%r\n", __func__, Status)); > + } > + } > + > + Status = MmcNotifyState (MmcHostInstance, MmcTransferState); > + if (EFI_ERROR (Status)) { > + DEBUG ((EFI_D_ERROR, "MmcIoBlocks() : Error MmcTransferState\n")); > + return Status; > + } > + return Status; > +} > + > EFI_STATUS > MmcIoBlocks ( > IN EFI_BLOCK_IO_PROTOCOL *This, > @@ -145,6 +234,7 @@ MmcIoBlocks ( > EFI_MMC_HOST_PROTOCOL *MmcHost; > UINTN BytesRemainingToBeTransfered; > UINTN BlockCount; > + UINTN ConsumeSize; > > BlockCount = 1; > MmcHostInstance = MMC_HOST_INSTANCE_FROM_BLOCK_IO_THIS (This); > @@ -165,6 +255,10 @@ MmcIoBlocks ( > return EFI_NO_MEDIA; > } > > + if (MmcHost->IsMultiBlock && MmcHost->IsMultiBlock(MmcHost)) { > + BlockCount = (BufferSize + This->Media->BlockSize - 1) / This->Media->BlockSize; > + } > + > // All blocks must be within the device > if ((Lba + (BufferSize / This->Media->BlockSize)) > (This->Media->LastBlock + 1)) { > return EFI_INVALID_PARAMETER; > @@ -196,7 +290,7 @@ MmcIoBlocks ( > CmdArg = MmcHostInstance->CardInfo.RCA << 16; > Response[0] = 0; > Timeout = 20; > - while( (!(Response[0] & MMC_R0_READY_FOR_DATA)) > + while(!(Response[0] & MMC_R0_READY_FOR_DATA) This is cleanup, but it is unrelated to functional changes, so please leave out. > && (MMC_R0_CURRENTSTATE (Response) != MMC_R0_STATE_TRAN) > && Timeout--) { > Status = MmcHost->SendCommand (MmcHost, MMC_CMD13, CmdArg); > @@ -210,75 +304,38 @@ MmcIoBlocks ( > return EFI_NOT_READY; > } > > - //Set command argument based on the card access mode (Byte mode or Block mode) > - if (MmcHostInstance->CardInfo.OCRData.AccessMode & BIT1) { > - CmdArg = Lba; > - } else { > - CmdArg = Lba * This->Media->BlockSize; > - } > - > if (Transfer == MMC_IOBLOCKS_READ) { > - // Read a single block > - Cmd = MMC_CMD17; > - } else { > - // Write a single block > - Cmd = MMC_CMD24; > - } > - Status = MmcHost->SendCommand (MmcHost, Cmd, CmdArg); > - if (EFI_ERROR (Status)) { > - DEBUG ((EFI_D_ERROR, "MmcIoBlocks(MMC_CMD%d): Error %r\n", Cmd, Status)); > - return Status; > - } > - > - if (Transfer == MMC_IOBLOCKS_READ) { > - // Read one block of Data > - Status = MmcHost->ReadBlockData (MmcHost, Lba, This->Media->BlockSize, Buffer); > - if (EFI_ERROR (Status)) { > - DEBUG ((EFI_D_BLKIO, "MmcIoBlocks(): Error Read Block Data and Status = %r\n", Status)); > - MmcStopTransmission (MmcHost); > - return Status; > - } > - Status = MmcNotifyState (MmcHostInstance, MmcProgrammingState); > - if (EFI_ERROR (Status)) { > - DEBUG ((EFI_D_ERROR, "MmcIoBlocks() : Error MmcProgrammingState\n")); > - return Status; > + if (BlockCount == 1) { > + // Read a single block > + Cmd = MMC_CMD17; > + } else { > + // Read multiple blocks > + Cmd = MMC_CMD18; > } > } else { > - // Write one block of Data > - Status = MmcHost->WriteBlockData (MmcHost, Lba, This->Media->BlockSize, Buffer); > - if (EFI_ERROR (Status)) { > - DEBUG ((EFI_D_BLKIO, "MmcIoBlocks(): Error Write Block Data and Status = %r\n", Status)); > - MmcStopTransmission (MmcHost); > - return Status; > + if (BlockCount == 1) { > + // Write a single block > + Cmd = MMC_CMD24; > + } else { > + // Write multiple blocks > + Cmd = MMC_CMD25; > } > } > > - // Command 13 - Read status and wait for programming to complete (return to tran) > - Timeout = MMCI0_TIMEOUT; > - CmdArg = MmcHostInstance->CardInfo.RCA << 16; > - Response[0] = 0; > - while( (!(Response[0] & MMC_R0_READY_FOR_DATA)) > - && (MMC_R0_CURRENTSTATE (Response) != MMC_R0_STATE_TRAN) > - && Timeout--) { > - Status = MmcHost->SendCommand (MmcHost, MMC_CMD13, CmdArg); > - if (!EFI_ERROR (Status)) { > - MmcHost->ReceiveResponse (MmcHost, MMC_RESPONSE_TYPE_R1, Response); > - if ((Response[0] & MMC_R0_READY_FOR_DATA)) { > - break; // Prevents delay once finished > - } > - } > - gBS->Stall (1); > + ConsumeSize = BlockCount * This->Media->BlockSize; > + if (BytesRemainingToBeTransfered < ConsumeSize) { > + ConsumeSize = BytesRemainingToBeTransfered; > } > - > - Status = MmcNotifyState (MmcHostInstance, MmcTransferState); > + Status = MmcTransferBlock (This, Cmd, Transfer, MediaId, Lba, ConsumeSize, Buffer); > if (EFI_ERROR (Status)) { > - DEBUG ((EFI_D_ERROR, "MmcIoBlocks() : Error MmcTransferState\n")); > - return Status; > + DEBUG ((EFI_D_ERROR, "%a(): Failed to transfer block and Status:%r\n", __func__, Status)); > } > > - BytesRemainingToBeTransfered -= This->Media->BlockSize; > - Lba += BlockCount; > - Buffer = (UINT8 *)Buffer + This->Media->BlockSize; > + BytesRemainingToBeTransfered -= ConsumeSize; > + if (BytesRemainingToBeTransfered > 0) { > + Lba += BlockCount; > + Buffer = (UINT8 *)Buffer + ConsumeSize; > + } > } > > return EFI_SUCCESS; > -- > 2.7.4 >