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

2017-07-07 Thread Jun Nie 
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

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

2017-07-06 Thread 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.

> +
> +  /* 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 (

[edk2] [PATCH v2 2/2] EmbeddedPkg/DwEmmc: Adjust FIFO threshold

2017-07-05 Thread Jun Nie 
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   | 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 (
+  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;
+  UINT32 BlkSize = DWEMMC_BLOCK_SIZE, Idx = 0, RxWmark = 1, TxWmark, 
TxWmarkInvers;
+
+  /* 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 */
+  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|0x