[PATCH] EmbeddedPkg/DwEmmcDxe: limit max clock for platform

[PATCH] EmbeddedPkg/DwEmmcDxe: limit max clock for platform

[Jun Nie]

Some boards may have max clock limitation. Add a Pcd to notify
driver.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Jun Nie <jun.nie@linaro.org>
---
 EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c   | 4 ++++
 EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.inf | 1 +
 EmbeddedPkg/EmbeddedPkg.dec                 | 1 +
 3 files changed, 6 insertions(+)

diff --git a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
index fe23d11..308f3a7 100644
--- a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
+++ b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
@@ -560,6 +560,10 @@ DwEmmcSetIos (
   EFI_STATUS Status = EFI_SUCCESS;
   UINT32    Data;
 
+  if (BusClockFreq > PcdGet32 (PcdDwEmmcDxeMaxClockFrequencyInHz)) {
+    return EFI_UNSUPPORTED;
+  }
+
   if (TimingMode != EMMCBACKWARD) {
     Data = MmioRead32 (DWEMMC_UHSREG);
     switch (TimingMode) {
diff --git a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.inf b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.inf
index e3c8313..3582997 100644
--- a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.inf
+++ b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.inf
@@ -48,6 +48,7 @@
 [Pcd]
   gEmbeddedTokenSpaceGuid.PcdDwEmmcDxeBaseAddress
   gEmbeddedTokenSpaceGuid.PcdDwEmmcDxeClockFrequencyInHz
+  gDwEmmcDxeTokenSpaceGuid.PcdDwEmmcDxeMaxClockFrequencyInHz
 
 [Depex]
   TRUE
diff --git a/EmbeddedPkg/EmbeddedPkg.dec b/EmbeddedPkg/EmbeddedPkg.dec
index 0d4a062..aec8259 100644
--- a/EmbeddedPkg/EmbeddedPkg.dec
+++ b/EmbeddedPkg/EmbeddedPkg.dec
@@ -167,6 +167,7 @@
   # DwEmmc Driver PCDs
   gEmbeddedTokenSpaceGuid.PcdDwEmmcDxeBaseAddress|0x0|UINT32|0x00000035
   gEmbeddedTokenSpaceGuid.PcdDwEmmcDxeClockFrequencyInHz|0x0|UINT32|0x00000036
+  gDwEmmcDxeTokenSpaceGuid.PcdDwEmmcDxeMaxClockFrequencyInHz|0x0|UINT32|400000000
 
   #
   # Android FastBoot
-- 
1.9.1

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

[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 <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)
+
 #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
-- 
1.9.1

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

[Leif Lindholm]

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
> 

Re: [PATCH] EmbeddedPkg/DwEmmcDxe: limit max clock for platform

[Ard Biesheuvel]

On 3 July 2017 at 05:01, Jun Nie <jun.nie@linaro.org> wrote:
> Some boards may have max clock limitation. Add a Pcd to notify
> driver.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Jun Nie <jun.nie@linaro.org>
> ---
>  EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c   | 4 ++++
>  EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.inf | 1 +
>  EmbeddedPkg/EmbeddedPkg.dec                 | 1 +
>  3 files changed, 6 insertions(+)
>
> diff --git a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
> index fe23d11..308f3a7 100644
> --- a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
> +++ b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
> @@ -560,6 +560,10 @@ DwEmmcSetIos (
>    EFI_STATUS Status = EFI_SUCCESS;
>    UINT32    Data;
>
> +  if (BusClockFreq > PcdGet32 (PcdDwEmmcDxeMaxClockFrequencyInHz)) {
> +    return EFI_UNSUPPORTED;
> +  }
> +
>    if (TimingMode != EMMCBACKWARD) {
>      Data = MmioRead32 (DWEMMC_UHSREG);
>      switch (TimingMode) {
> diff --git a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.inf b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.inf
> index e3c8313..3582997 100644
> --- a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.inf
> +++ b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.inf
> @@ -48,6 +48,7 @@
>  [Pcd]
>    gEmbeddedTokenSpaceGuid.PcdDwEmmcDxeBaseAddress
>    gEmbeddedTokenSpaceGuid.PcdDwEmmcDxeClockFrequencyInHz
> +  gDwEmmcDxeTokenSpaceGuid.PcdDwEmmcDxeMaxClockFrequencyInHz
>
>  [Depex]
>    TRUE
> diff --git a/EmbeddedPkg/EmbeddedPkg.dec b/EmbeddedPkg/EmbeddedPkg.dec
> index 0d4a062..aec8259 100644
> --- a/EmbeddedPkg/EmbeddedPkg.dec
> +++ b/EmbeddedPkg/EmbeddedPkg.dec
> @@ -167,6 +167,7 @@
>    # DwEmmc Driver PCDs
>    gEmbeddedTokenSpaceGuid.PcdDwEmmcDxeBaseAddress|0x0|UINT32|0x00000035
>    gEmbeddedTokenSpaceGuid.PcdDwEmmcDxeClockFrequencyInHz|0x0|UINT32|0x00000036
> +  gDwEmmcDxeTokenSpaceGuid.PcdDwEmmcDxeMaxClockFrequencyInHz|0x0|UINT32|400000000
>

Please use the correct token space guid, or add it to the correct .dec file.

Re: [PATCH] EmbeddedPkg/DwEmmcDxe: limit max clock for platform

[Leif Lindholm]

On Mon, Jul 03, 2017 at 12:01:56PM +0800, Jun Nie wrote:
> Some boards may have max clock limitation. Add a Pcd to notify
> driver.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Jun Nie <jun.nie@linaro.org>
> ---
>  EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c   | 4 ++++
>  EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.inf | 1 +
>  EmbeddedPkg/EmbeddedPkg.dec                 | 1 +
>  3 files changed, 6 insertions(+)
> 
> diff --git a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
> index fe23d11..308f3a7 100644
> --- a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
> +++ b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
> @@ -560,6 +560,10 @@ DwEmmcSetIos (
>    EFI_STATUS Status = EFI_SUCCESS;
>    UINT32    Data;
>  
> +  if (BusClockFreq > PcdGet32 (PcdDwEmmcDxeMaxClockFrequencyInHz)) {

This snippet means that any platform that was using this driver
successfully, but where BusClockFreq is higher than the default value
of this Pcd will stop working.

> +    return EFI_UNSUPPORTED;
> +  }
> +
>    if (TimingMode != EMMCBACKWARD) {
>      Data = MmioRead32 (DWEMMC_UHSREG);
>      switch (TimingMode) {
> diff --git a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.inf b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.inf
> index e3c8313..3582997 100644
> --- a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.inf
> +++ b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.inf
> @@ -48,6 +48,7 @@
>  [Pcd]
>    gEmbeddedTokenSpaceGuid.PcdDwEmmcDxeBaseAddress
>    gEmbeddedTokenSpaceGuid.PcdDwEmmcDxeClockFrequencyInHz
> +  gDwEmmcDxeTokenSpaceGuid.PcdDwEmmcDxeMaxClockFrequencyInHz
>  
>  [Depex]
>    TRUE
> diff --git a/EmbeddedPkg/EmbeddedPkg.dec b/EmbeddedPkg/EmbeddedPkg.dec
> index 0d4a062..aec8259 100644
> --- a/EmbeddedPkg/EmbeddedPkg.dec
> +++ b/EmbeddedPkg/EmbeddedPkg.dec
> @@ -167,6 +167,7 @@
>    # DwEmmc Driver PCDs
>    gEmbeddedTokenSpaceGuid.PcdDwEmmcDxeBaseAddress|0x0|UINT32|0x00000035
>    gEmbeddedTokenSpaceGuid.PcdDwEmmcDxeClockFrequencyInHz|0x0|UINT32|0x00000036
> +  gDwEmmcDxeTokenSpaceGuid.PcdDwEmmcDxeMaxClockFrequencyInHz|0x0|UINT32|400000000

And this definition sets the default value of this Pcd to 0. Meaning
that all existing drivers would stop working.
Also, 0x00000037 would seem like a more suitable Token than 400000000.

If introducing a new limit value like this, I would strongly recommend
making "0" a magic value meaning "no restrictions".

i.e.
  UINT32 MaxFreq;
  MaxFreq = PcdGet32 (PcdDwEmmcDxeMaxClockFrequencyInHz);
  if ((MaxFreq != 0) && (BusClockFreq > MaxFreq)) {

/
    Leif

>  
>    #
>    # Android FastBoot
> -- 
> 1.9.1
>