diff mbox series

[edk2,2/2] EmbeddedPkg/DwEmmc: Adjust FIFO threshold

Message ID 1499054517-22398-2-git-send-email-jun.nie@linaro.org
State New
Headers show
Series None | expand

Commit Message

Jun Nie July 3, 2017, 4:01 a.m. UTC 
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(+)

-- 
1.9.1

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Comments

Leif Lindholm July 3, 2017, 1:21 p.m. UTC | #1 
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

> 

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
diff mbox series

Patch

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)
+
 #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
+  )
+{
+  const UINT32 Mszs[] = {1, 4, 8, 16, 32, 64, 128, 256};
+  UINT32 BlkSizeDepth, Fifoth, FifoWidth, FifoDepth;
+  UINT32 BlkSize = 512, Msize = 0, RxWmark = 1, TxWmark, TxWmarkInvers;
+  UINT32 Idx = ARRAY_SIZE(Mszs) - 1;
+
+  /* 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;
+  }
+
+  BlkSizeDepth = BlkSize / FifoWidth;
+
+  /*
+   * MSIZE is '1',
+   * if BlkSize is not a multiple of the FIFO width
+   */
+  if (BlkSize % FifoWidth) {
+    goto done;
+  }
+
+  do {
+    if (!((BlkSizeDepth % Mszs[Idx]) || (TxWmarkInvers % Mszs[Idx]))) {
+      Msize = Idx;
+      RxWmark = Mszs[Idx] - 1;
+      break;
+    }
+  } while (--Idx > 0);
+  /*
+   * 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);
+  MmioWrite32 (DWEMMC_FIFOTH, Fifoth);
+}
+
 EFI_STATUS
 PrepareDmaData (
   IN DWEMMC_IDMAC_DESCRIPTOR*    IdmacDesc,
@@ -632,6 +685,7 @@  DwEmmcDxeInitialize (
 
   Handle = NULL;
 
+  DwEmmcAdjustFifoth();
   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