From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qk0-x231.google.com (mail-qk0-x231.google.com [IPv6:2607:f8b0:400d:c09::231]) (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 F241B21CE73F4 for ; Fri, 7 Jul 2017 02:11:39 -0700 (PDT) Received: by mail-qk0-x231.google.com with SMTP id p21so22452001qke.3 for ; Fri, 07 Jul 2017 02:13:21 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=bEkbNjcLbweE2S/ADWr1hhZ3OQtw4gyU7DWNjX/Y3I0=; b=O+En4Hpyq5/EfhmvMEIfDbl1ZVWv97v+tG4Q9KIO1fBeZkQLr4B7Sd+WCtR+asKe5m h5loxb6xV4VZBWBnZt4lwUvyeXLH7PpnjX8m5+gFPqcTBgS4LSMF2Zg8j6MFRKT8zP65 KtxuxNer2adZdCZXDYUmiAbTBZ6PbmpUxa3OQ= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=bEkbNjcLbweE2S/ADWr1hhZ3OQtw4gyU7DWNjX/Y3I0=; b=Z1oork/OYrrHOG4mdMhUeGNJLoy6TU5BuwobG4xyRaNSzqMezn5wd81IwbngKxP7LJ VB6ad2AIsHp5LDAIC1MG6owwdnBTf+A/ByTj1iNAncy/7mnQza0bFtpHbkVIhBLif+0E m8e9NlJA275wg1O+pFMHziAunhizaeshmzxnooB7GSnRFFS+MzDErLaaCroj3N2CYdpH KZxaMLt8zdFy/wFmKkA3N1p1+rBpfz/PMzHfjidpLva0mW40FmnOUZwG9Iwcr5bEFeV+ Ix24ffRvqHpdtnXFzVI/lFI8Pg84MteXhLtoonblY+zQU1qoqsJUWAzzI4jnffrZJ5Tu 5img== X-Gm-Message-State: AKS2vOz92qKlnxznA25bHYwC7Wea9olKkF6Bam8DsJT+zp8nP/ZYaeOa 7zMfRNMQGPog559HVQncilg8R2WwLKgo X-Received: by 10.55.3.139 with SMTP id 133mr62462543qkd.65.1499418800546; Fri, 07 Jul 2017 02:13:20 -0700 (PDT) MIME-Version: 1.0 Received: by 10.55.110.66 with HTTP; Fri, 7 Jul 2017 02:13:19 -0700 (PDT) In-Reply-To: <20170706152232.GQ26676@bivouac.eciton.net> References: <1499243228-1225-1-git-send-email-jun.nie@linaro.org> <1499243228-1225-2-git-send-email-jun.nie@linaro.org> <20170706152232.GQ26676@bivouac.eciton.net> From: Jun Nie Date: Fri, 7 Jul 2017 17:13:19 +0800 Message-ID: To: Leif Lindholm Cc: Ard Biesheuvel , Haojian Zhuang , edk2-devel@lists.01.org, Shawn Guo , Jason Liu Subject: Re: [PATCH v2 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: Fri, 07 Jul 2017 09:11:40 -0000 Content-Type: text/plain; charset="UTF-8" 2017-07-06 23:22 GMT+08:00 Leif Lindholm : > On Wed, Jul 05, 2017 at 04:27:08PM +0800, Jun Nie wrote: >> Adjust FIFO threshold according to FIFO depth. Skip >> the adjustment if we do not have FIFO depth info. >> > > So, this is a big improvement in readability - but some of my generic > style comments do not appear to have been addressed. > >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Jun Nie >> --- >> EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmc.h | 6 ++++ >> EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c | 50 +++++++++++++++++++++++++++++ >> EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.inf | 1 + >> EmbeddedPkg/EmbeddedPkg.dec | 1 + >> 4 files changed, 58 insertions(+) >> >> diff --git a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmc.h b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmc.h >> index 055f1e0..90c7676 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) >> + >> #endif // __DWEMMC_H__ >> diff --git a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c >> index bb26b69..70e064d 100644 >> --- a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c >> +++ b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c >> @@ -415,6 +415,55 @@ DwEmmcReceiveResponse ( >> return EFI_SUCCESS; >> } >> >> +VOID DwEmmcAdjustFifothreshold ( > > This still needs to be: > VOID > DwEmmcAdjustFifoThreshold ( > >> + VOID >> + ) >> +{ >> + /* DMA multiple transaction size map to reg value as array index */ >> + CONST UINT32 BurstSize[] = {1, 4, 8, 16, 32, 64, 128, 256}; >> + UINT32 BlkDepthInFifo, Fifoth, FifoWidth, FifoDepth; > > Fifoth -> FifoThreshold. > >> + UINT32 BlkSize = DWEMMC_BLOCK_SIZE, Idx = 0, RxWmark = 1, TxWmark, TxWmarkInvers; > > RxWmark -> RxWaterMark. > TxWmark -> TxWaterMark. > TxWmarkInvers -> TxWaterMarkInverse. My fault. The style is totally different with kernel and I am not fully customized yet. > >> + >> + /* Skip FIFO adjustment if we do not have platform FIFO depth info */ >> + FifoDepth = PcdGet32 (PcdDwEmmcDxeFifoDepth); >> + if (!FifoDepth) { >> + return; >> + } >> + >> + TxWmark = FifoDepth / 2; >> + TxWmarkInvers = FifoDepth - TxWmark; >> + >> + FifoWidth = DWEMMC_GET_HDATA_WIDTH (MmioRead32 (DWEMMC_HCON)); >> + if (!FifoWidth) { >> + FifoWidth = 2; >> + } else if (FifoWidth == 2) { >> + FifoWidth = 8; >> + } else { >> + FifoWidth = 4; >> + } >> + >> + BlkDepthInFifo = BlkSize / FifoWidth; >> + >> + /* if BlkSize is not a multiple of the FIFO width */ > > This comment still only says exactly what the code says. > Why are we done if BlkSize is not a multiple of FifoWidth? > Is this situation possible? What does it indicate? You find the key point. As this driver is dedicated for EMMC, not shared with MMC, this check shall never be true. We can ignore it. Jun > > / > Leif > >> + if (BlkSize % FifoWidth) { >> + goto done; >> + } >> + >> + Idx = ARRAY_SIZE (BurstSize) - 1; >> + while (Idx && ((BlkDepthInFifo % BurstSize[Idx]) || (TxWmarkInvers % BurstSize[Idx]))) { >> + Idx--; >> + } >> + RxWmark = BurstSize[Idx] - 1; >> + /* >> + * If Idx is '0', it won't be tried >> + * Thus, initial values are used >> + */ >> +done: >> + Fifoth = DWEMMC_DMA_BURST_SIZE (Idx) | DWEMMC_FIFO_TWMARK (TxWmark) >> + | DWEMMC_FIFO_RWMARK (RxWmark); >> + MmioWrite32 (DWEMMC_FIFOTH, Fifoth); >> +} >> + >> EFI_STATUS >> PrepareDmaData ( >> IN DWEMMC_IDMAC_DESCRIPTOR* IdmacDesc, >> @@ -633,6 +682,7 @@ DwEmmcDxeInitialize ( >> >> Handle = NULL; >> >> + DwEmmcAdjustFifothreshold (); >> 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 99b4f99..bc4413e 100644 >> --- a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.inf >> +++ b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.inf >> @@ -49,6 +49,7 @@ >> gEmbeddedTokenSpaceGuid.PcdDwEmmcDxeBaseAddress >> gEmbeddedTokenSpaceGuid.PcdDwEmmcDxeClockFrequencyInHz >> gEmbeddedTokenSpaceGuid.PcdDwEmmcDxeMaxClockFreqInHz >> + gEmbeddedTokenSpaceGuid.PcdDwEmmcDxeFifoDepth >> >> [Depex] >> TRUE >> diff --git a/EmbeddedPkg/EmbeddedPkg.dec b/EmbeddedPkg/EmbeddedPkg.dec >> index 2da9b2f..5f39d9d 100644 >> --- a/EmbeddedPkg/EmbeddedPkg.dec >> +++ b/EmbeddedPkg/EmbeddedPkg.dec >> @@ -170,6 +170,7 @@ >> gEmbeddedTokenSpaceGuid.PcdDwEmmcDxeBaseAddress|0x0|UINT32|0x00000035 >> gEmbeddedTokenSpaceGuid.PcdDwEmmcDxeClockFrequencyInHz|0x0|UINT32|0x00000036 >> gEmbeddedTokenSpaceGuid.PcdDwEmmcDxeMaxClockFreqInHz|0x0|UINT32|0x00000037 >> + gEmbeddedTokenSpaceGuid.PcdDwEmmcDxeFifoDepth|0x0|UINT32|0x00000038 >> >> # >> # Android FastBoot >> -- >> 1.9.1 >>