Re: [PATCH 2/2] EmbeddedPkg/DwEmmc: Adjust FIFO threshold

[Leif Lindholm <leif.lindholm@linaro.org>]

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 <jun.nie@linaro.org>
> ---
>  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
>