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 <jun....@linaro.org> > --- > 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. > + > + /* 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? / 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 > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel