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 3AE4721A00AC2 for ; Mon, 3 Jul 2017 06:20:22 -0700 (PDT) Received: by mail-wm0-x22d.google.com with SMTP id i127so108526470wma.0 for ; Mon, 03 Jul 2017 06:21:59 -0700 (PDT) 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=I8hpjWlQTE+XYCCnB4+TnXXAwQZFdr45N6cxRrjyFFM=; b=FWn0f8sfevJRfSwubXY5VtQdLPFYHl32gnjLD6Ibsqx9NR7STRB54nD3VsY8k2T+MN nw2QUuE9JYqTo0mhtEcpyE0zBwhhBRcBXoqSELLnmDoXJbp+2VELDwUIxTt8c1+7pZC3 7zWhV0LJ/EHJUs4M57uSIwC7x0+CIq/NmRHrk= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=I8hpjWlQTE+XYCCnB4+TnXXAwQZFdr45N6cxRrjyFFM=; b=aAc+7zjsCEb94qI1N8Ges/95W8BekUIPKrcX8iODLhbXwdXs6AyvPLXnQ8jmzCrwST uV5qouo7oTJo9lrK/OmUBDar/TWIJDqB7+jgEIFLGmiWT7R4oAjlhzLJgOwI9PI1rrNk XYUiOg66eA18wyRJqQckGhgXmfbGSFVKT0Kmy9dBdXhl8owsg/7e+fBNA3Ok5BuDfeOZ RmG3gePCxxv5Dgk/sAzugvIlyjgUWDWIQUkdhVFOFtySPHncqtu5JKoO/B1paI/f845z GfE6gPv+kucDfxpc73JJKesceC8t10wcI4MewZyEiF3ZtdbJpffh3UEUaAIWodDVIKUC ciXQ== X-Gm-Message-State: AKS2vOyYvOvOEiGH3PJekiTIOUlzm5besub0+Jn/+wGPMz0hW1vLu9+L 9q905olWwT7f5Cul X-Received: by 10.28.34.130 with SMTP id i124mr25326884wmi.116.1499088117903; Mon, 03 Jul 2017 06:21:57 -0700 (PDT) Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id 199sm3717866wmu.0.2017.07.03.06.21.56 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 03 Jul 2017 06:21:57 -0700 (PDT) Date: Mon, 3 Jul 2017 14:21:55 +0100 From: Leif Lindholm To: Jun Nie Cc: ard.biesheuvel@linaro.org, haojian.zhuang@linaro.org, edk2-devel@lists.01.org, shawn.guo@linaro.org, jason.liu@linaro.org Message-ID: <20170703132155.GF26676@bivouac.eciton.net> References: <1499054517-22398-1-git-send-email-jun.nie@linaro.org> <1499054517-22398-2-git-send-email-jun.nie@linaro.org> MIME-Version: 1.0 In-Reply-To: <1499054517-22398-2-git-send-email-jun.nie@linaro.org> User-Agent: Mutt/1.5.23 (2014-03-12) Subject: Re: [PATCH 2/2] EmbeddedPkg/DwEmmc: Adjust FIFO threshold X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 03 Jul 2017 13:20:22 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Mon, Jul 03, 2017 at 12:01:57PM +0800, Jun Nie wrote: > Adjust FIFO threshold according to FIFO depth. Skip > the adjustment if we do not have FIFO depth info. > > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Jun Nie > --- > EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmc.h | 6 ++++ > EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c | 54 +++++++++++++++++++++++++++++ > EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.inf | 1 + > EmbeddedPkg/EmbeddedPkg.dec | 1 + > 4 files changed, 62 insertions(+) > > diff --git a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmc.h b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmc.h > index 055f1e0..2b41539 100644 > --- a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmc.h > +++ b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmc.h > @@ -38,7 +38,10 @@ > #define DWEMMC_RINTSTS ((UINT32)PcdGet32 (PcdDwEmmcDxeBaseAddress) + 0x044) > #define DWEMMC_STATUS ((UINT32)PcdGet32 (PcdDwEmmcDxeBaseAddress) + 0x048) > #define DWEMMC_FIFOTH ((UINT32)PcdGet32 (PcdDwEmmcDxeBaseAddress) + 0x04c) > +#define DWEMMC_TCBCNT ((UINT32)PcdGet32 (PcdDwEmmcDxeBaseAddress) + 0x05c) > +#define DWEMMC_TBBCNT ((UINT32)PcdGet32 (PcdDwEmmcDxeBaseAddress) + 0x060) > #define DWEMMC_DEBNCE ((UINT32)PcdGet32 (PcdDwEmmcDxeBaseAddress) + 0x064) > +#define DWEMMC_HCON ((UINT32)PcdGet32 (PcdDwEmmcDxeBaseAddress) + 0x070) > #define DWEMMC_UHSREG ((UINT32)PcdGet32 (PcdDwEmmcDxeBaseAddress) + 0x074) > #define DWEMMC_BMOD ((UINT32)PcdGet32 (PcdDwEmmcDxeBaseAddress) + 0x080) > #define DWEMMC_DBADDR ((UINT32)PcdGet32 (PcdDwEmmcDxeBaseAddress) + 0x088) > @@ -47,6 +50,7 @@ > #define DWEMMC_DSCADDR ((UINT32)PcdGet32 (PcdDwEmmcDxeBaseAddress) + 0x094) > #define DWEMMC_BUFADDR ((UINT32)PcdGet32 (PcdDwEmmcDxeBaseAddress) + 0x098) > #define DWEMMC_CARDTHRCTL ((UINT32)PcdGet32 (PcdDwEmmcDxeBaseAddress) + 0X100) > +#define DWEMMC_DATA ((UINT32)PcdGet32 (PcdDwEmmcDxeBaseAddress) + 0X200) > > #define CMD_UPDATE_CLK 0x80202000 > #define CMD_START_BIT (1 << 31) > @@ -124,4 +128,6 @@ > #define DWEMMC_CARD_RD_THR(x) ((x & 0xfff) << 16) > #define DWEMMC_CARD_RD_THR_EN (1 << 0) > > +#define DWEMMC_GET_HDATA_WIDTH(x) (((x)>>7) & 0x7) Whitespace around ">>". > + > #endif // __DWEMMC_H__ > diff --git a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c > index c67dd0d..cb32a7c 100644 > --- a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c > +++ b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c > @@ -415,6 +415,59 @@ DwEmmcReceiveResponse ( > return EFI_SUCCESS; > } > > +VOID DwEmmcAdjustFifoth( VOID DwEmmcAdjustFifoThreshold ( > + VOID > + ) > +{ > + const UINT32 Mszs[] = {1, 4, 8, 16, 32, 64, 128, 256}; CONST. Mszs is not a very self-explanatory name. Please expand it to where someone other than yourself does not have to guess what it stands for, or figure out by analysing the code. On the whole, I'm not a fan of magic numerical values in code. Apart from the outlier "1", which could be read simply as "fifo disabled", all the others are powers of 2. > + UINT32 BlkSizeDepth, Fifoth, FifoWidth, FifoDepth; What is a BlkSizeDepth? Can a more descriptive name be used? Fifoth -> FifoThreshold. > + UINT32 BlkSize = 512, Msize = 0, RxWmark = 1, TxWmark, TxWmarkInvers; 512 - please create a suitable define somewhere is not already existing. Msize (same as Mszs). RxWmark -> RxWaterMark. TxWmark -> TxWaterMark. TxWmarkInvers -> TxWaterMarkInverse. > + UINT32 Idx = ARRAY_SIZE(Mszs) - 1; Space before "(". Initialized long before use. > + > + /* Skip FIFO adjustment if we do not have platform FIFO depth info */ > + FifoDepth = PcdGet32 (PcdDwEmmcDxeFifoDepth); > + if (!FifoDepth) > + return; Always use { and }. > + > + TxWmark = FifoDepth / 2; > + TxWmarkInvers = FifoDepth - TxWmark; > + > + FifoWidth = DWEMMC_GET_HDATA_WIDTH(MmioRead32 (DWEMMC_HCON)); Space before "(". > + if (!FifoWidth) { > + FifoWidth = 2; > + } else if (FifoWidth == 2) { > + FifoWidth = 8; > + } else { > + FifoWidth = 4; > + } > + > + BlkSizeDepth = BlkSize / FifoWidth; > + > + /* > + * MSIZE is '1', > + * if BlkSize is not a multiple of the FIFO width That comment is confusing. First cause, then effect. > + */ > + if (BlkSize % FifoWidth) { > + goto done; > + } > + > + do { > + if (!((BlkSizeDepth % Mszs[Idx]) || (TxWmarkInvers % Mszs[Idx]))) { > + Msize = Idx; > + RxWmark = Mszs[Idx] - 1; > + break; > + } > + } while (--Idx > 0); The Mszs array has been designed such that this loop index can be used as an additional result. There is no performance gain to this kind of complexity, and it makes the code unreadable. Please use a simple loop and find an alternative way of deriving Msize. > + /* > + * If Idx is '0', it won't be tried > + * Thus, initial values are uesed > + */ > +done: > + Fifoth = DWEMMC_DMA_BURST_SIZE(Msize) | DWEMMC_FIFO_TWMARK(TxWmark) > + | DWEMMC_FIFO_RWMARK(RxWmark); Spaces before "(". > + MmioWrite32 (DWEMMC_FIFOTH, Fifoth); > +} > + > EFI_STATUS > PrepareDmaData ( > IN DWEMMC_IDMAC_DESCRIPTOR* IdmacDesc, > @@ -632,6 +685,7 @@ DwEmmcDxeInitialize ( > > Handle = NULL; > > + DwEmmcAdjustFifoth(); Space before "(". > gpIdmacDesc = (DWEMMC_IDMAC_DESCRIPTOR *)AllocatePages (DWEMMC_MAX_DESC_PAGES); > if (gpIdmacDesc == NULL) { > return EFI_BUFFER_TOO_SMALL; > diff --git a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.inf b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.inf > index 3582997..a3e10fe 100644 > --- a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.inf > +++ b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.inf > @@ -49,6 +49,7 @@ > gEmbeddedTokenSpaceGuid.PcdDwEmmcDxeBaseAddress > gEmbeddedTokenSpaceGuid.PcdDwEmmcDxeClockFrequencyInHz > gDwEmmcDxeTokenSpaceGuid.PcdDwEmmcDxeMaxClockFrequencyInHz > + gDwEmmcDxeTokenSpaceGuid.PcdDwEmmcDxeFifoDepth > > [Depex] > TRUE > diff --git a/EmbeddedPkg/EmbeddedPkg.dec b/EmbeddedPkg/EmbeddedPkg.dec > index aec8259..f28a5d2 100644 > --- a/EmbeddedPkg/EmbeddedPkg.dec > +++ b/EmbeddedPkg/EmbeddedPkg.dec > @@ -168,6 +168,7 @@ > gEmbeddedTokenSpaceGuid.PcdDwEmmcDxeBaseAddress|0x0|UINT32|0x00000035 > gEmbeddedTokenSpaceGuid.PcdDwEmmcDxeClockFrequencyInHz|0x0|UINT32|0x00000036 > gDwEmmcDxeTokenSpaceGuid.PcdDwEmmcDxeMaxClockFrequencyInHz|0x0|UINT32|400000000 > + gDwEmmcDxeTokenSpaceGuid.PcdDwEmmcDxeFifoDepth|0x0|UINT32|0 > > # > # Android FastBoot > -- > 1.9.1 >