[PATCH 1/2] SdMmcPciDxe: Make timeout for SD card configurable

[PATCH 1/2] SdMmcPciDxe: Make timeout for SD card configurable

[Sean Rhodes]

From: Matt DeVillier <matt.devillier@gmail.com>

The default 1s timeout can delay boot splash on some hardware with no
benefit.

Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Signed-off-by: Matt DeVillier <matt.devillier@gmail.com>
Signed-off-by: Sean Rhodes <sean@starlabs.systems>
---
 MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h | 2 +-
 MdeModulePkg/MdeModulePkg.dec                      | 3 +++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h
index 85e09cf114..c9a21e01bd 100644
--- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h
+++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h
@@ -49,7 +49,7 @@ extern EDKII_SD_MMC_OVERRIDE  *mOverride;
 //
 // Generic time out value, 1 microsecond as unit.
 //
-#define SD_MMC_HC_GENERIC_TIMEOUT  1 * 1000 * 1000
+#define SD_MMC_HC_GENERIC_TIMEOUT  ((PcdGet32 (PcdSdMmcGenericTimeoutValue)
 
 //
 // SD/MMC async transfer timer interval, set by experience.
diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec
index 463e889e9a..092660f7f0 100644
--- a/MdeModulePkg/MdeModulePkg.dec
+++ b/MdeModulePkg/MdeModulePkg.dec
@@ -1559,6 +1559,9 @@
   # @Prompt Maximum permitted FwVol section nesting depth (exclusive).
   gEfiMdeModulePkgTokenSpaceGuid.PcdFwVolDxeMaxEncapsulationDepth|0x10|UINT32|0x00000030
 
+  ## SD Card timeout
+  gEfiMdeModulePkgTokenSpaceGuid.PcdSdMmcGenericTimeoutValue|1000000|UINT32|0x00000031
+
 [PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx]
   ## This PCD defines the Console output row. The default value is 25 according to UEFI spec.
   #  This PCD could be set to 0 then console output would be at max column and max row.
-- 
2.32.0

[PATCH 2/2] UefiPayloadPkg: Hookup SD Card timeout

[Sean Rhodes]

Hook SD_CARD_TIMEOUT build option to SdMmcGenericTimeoutValue PCD.

Cc: Guo Dong <guo.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Maurice Ma <maurice.ma@intel.com>
Cc: Benjamin You <benjamin.you@intel.com>
Signed-off-by: Sean Rhodes <sean@starlabs.systems>
---
 UefiPayloadPkg/UefiPayloadPkg.dsc | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/UefiPayloadPkg/UefiPayloadPkg.dsc b/UefiPayloadPkg/UefiPayloadPkg.dsc
index 1ce96a51c1..d75fe26426 100644
--- a/UefiPayloadPkg/UefiPayloadPkg.dsc
+++ b/UefiPayloadPkg/UefiPayloadPkg.dsc
@@ -33,6 +33,7 @@
   DEFINE UNIVERSAL_PAYLOAD            = FALSE
   DEFINE SECURITY_STUB_ENABLE         = TRUE
   DEFINE SMM_SUPPORT                  = FALSE
+  DEFINE SD_CARD_TIMEOUT              = 1000000
   #
   # SBL:      UEFI payload for Slim Bootloader
   # COREBOOT: UEFI payload for coreboot
@@ -398,6 +399,7 @@
 !if $(PERFORMANCE_MEASUREMENT_ENABLE)
   gEfiMdePkgTokenSpaceGuid.PcdPerformanceLibraryPropertyMask       | 0x1
 !endif
+  gEfiMdeModulePkgTokenSpaceGuid.PcdSdMmcGenericTimeoutValue|$(SD_CARD_TIMEOUT)
 
 [PcdsPatchableInModule.X64]
   gPcAtChipsetPkgTokenSpaceGuid.PcdRtcIndexRegister|$(RTC_INDEX_REGISTER)
-- 
2.32.0

Re: [edk2-devel] [PATCH 1/2] SdMmcPciDxe: Make timeout for SD card configurable

[Wu, Hao A]

Inline comments below:



> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Sean
> Rhodes
> Sent: Friday, February 11, 2022 4:43 PM
> To: devel@edk2.groups.io
> Cc: Dong, Guo <guo.dong@intel.com>; Matt DeVillier
> <matt.devillier@gmail.com>; Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray
> <ray.ni@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Gao, Liming
> <gaoliming@byosoft.com.cn>; Rhodes, Sean <sean@starlabs.systems>
> Subject: [edk2-devel] [PATCH 1/2] SdMmcPciDxe: Make timeout for SD card
> configurable


1. Please help to update the commit subject to "MdeModulePkg/SdMmcPciHcDxe: Make timeout for SD card configurable"?


> 
> From: Matt DeVillier <matt.devillier@gmail.com>
> 
> The default 1s timeout can delay boot splash on some hardware with no
> benefit.
> 
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Signed-off-by: Matt DeVillier <matt.devillier@gmail.com>
> Signed-off-by: Sean Rhodes <sean@starlabs.systems>
> ---
>  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h | 2 +-
>  MdeModulePkg/MdeModulePkg.dec                      | 3 +++
>  2 files changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h
> b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h
> index 85e09cf114..c9a21e01bd 100644
> --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h
> +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h
> @@ -49,7 +49,7 @@ extern EDKII_SD_MMC_OVERRIDE  *mOverride;
>  //
> 
>  // Generic time out value, 1 microsecond as unit.
> 
>  //
> 
> -#define SD_MMC_HC_GENERIC_TIMEOUT  1 * 1000 * 1000
> 
> +#define SD_MMC_HC_GENERIC_TIMEOUT  ((PcdGet32
> (PcdSdMmcGenericTimeoutValue)


2. The parentheses in the macro definition are not matched.
How about "... (PcdGet32 (PcdSdMmcGenericTimeoutValue))"?
Could you help to verify the patch before sending it out? Thanks in advance.


> 
> 
> 
>  //
> 
>  // SD/MMC async transfer timer interval, set by experience.
> 
> diff --git a/MdeModulePkg/MdeModulePkg.dec
> b/MdeModulePkg/MdeModulePkg.dec
> index 463e889e9a..092660f7f0 100644
> --- a/MdeModulePkg/MdeModulePkg.dec
> +++ b/MdeModulePkg/MdeModulePkg.dec
> @@ -1559,6 +1559,9 @@
>    # @Prompt Maximum permitted FwVol section nesting depth (exclusive).
> 
> 
> gEfiMdeModulePkgTokenSpaceGuid.PcdFwVolDxeMaxEncapsulationDepth|
> 0x10|UINT32|0x00000030
> 
> 
> 
> +  ## SD Card timeout


3. Could you help to update the PCD description to:
  ## Indicates the default timeout value for SD/MMC Host Controller operations in microseconds.
  # @Prompt SD/MMC Host Controller Operations Timeout (us).

> 
> +
> gEfiMdeModulePkgTokenSpaceGuid.PcdSdMmcGenericTimeoutValue|1000
> 000|UINT32|0x00000031
> 
> +


4. I think the patch is missing the PcdLib reference in files SdMmcPciHcDxe.h and SdMmcPciHcDxe.inf.

5. The patch is also missing the PcdSdMmcGenericTimeoutValue PCD reference in SdMmcPciHcDxe.inf.
Please follow other INF files in edk2 to add the PCD reference.
Also please help to verify the patch before sending it out. Thanks in advance.

6. When adding a new PCD in DEC file, please also help to update the UNI file as well.
Could you help to add below lines in file MdeModulePkg.uni:
#string STR_gEfiMdeModulePkgTokenSpaceGuid_PcdSdMmcGenericTimeoutValue_PROMPT #language en-US "SD/MMC Host Controller Operations Timeout (us)."

#string STR_gEfiMdeModulePkgTokenSpaceGuid_PcdSdMmcGenericTimeoutValue_HELP   #language en-US "Indicates the default timeout value for SD/MMC Host Controller operations in microseconds."


Best Regards,
Hao Wu


> 
>  [PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx]
> 
>    ## This PCD defines the Console output row. The default value is 25
> according to UEFI spec.
> 
>    #  This PCD could be set to 0 then console output would be at max column
> and max row.
> 
> --
> 2.32.0
> 
> 
> 
> -=-=-=-=-=-=
> Groups.io Links: You receive all messages sent to this group.
> View/Reply Online (#86612): https://edk2.groups.io/g/devel/message/86612
> Mute This Topic: https://groups.io/mt/89066986/1768737
> Group Owner: devel+owner@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub [hao.a.wu@intel.com]
> -=-=-=-=-=-=
> 

[PATCH 1/2] MdeModulePkg/SdMmcPciHcDxe: Make timeout for SD card configurable

[Sean Rhodes]

From: Matt DeVillier <matt.devillier@gmail.com>

The default 1s timeout can delay boot splash on some hardware with no
benefit.

Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Signed-off-by: Matt DeVillier <matt.devillier@gmail.com>
Signed-off-by: Sean Rhodes <sean@starlabs.systems>
---
 MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h    | 3 ++-
 MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.inf  | 4 ++++
 MdeModulePkg/Library/BrotliCustomDecompressLib/brotli | 2 +-
 MdeModulePkg/MdeModulePkg.dec                         | 4 ++++
 MdeModulePkg/MdeModulePkg.uni                         | 4 ++++
 5 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h
index 85e09cf114..b76c7cffa2 100644
--- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h
+++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h
@@ -24,6 +24,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 #include <Library/MemoryAllocationLib.h>
 #include <Library/UefiLib.h>
 #include <Library/DevicePathLib.h>
+#include <Library/PcdLib.h>
 
 #include <Protocol/DevicePath.h>
 #include <Protocol/PciIo.h>
@@ -49,7 +50,7 @@ extern EDKII_SD_MMC_OVERRIDE  *mOverride;
 //
 // Generic time out value, 1 microsecond as unit.
 //
-#define SD_MMC_HC_GENERIC_TIMEOUT  1 * 1000 * 1000
+#define SD_MMC_HC_GENERIC_TIMEOUT  (PcdGet32 (PcdSdMmcGenericTimeoutValue))
 
 //
 // SD/MMC async transfer timer interval, set by experience.
diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.inf b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.inf
index 453ecde7fd..a9d05736d7 100644
--- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.inf
+++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.inf
@@ -56,6 +56,7 @@
   BaseLib
   UefiDriverEntryPoint
   DebugLib
+  PcdLib
 
 [Protocols]
   gEdkiiSdMmcOverrideProtocolGuid               ## SOMETIMES_CONSUMES
@@ -68,3 +69,6 @@
 
 [UserExtensions.TianoCore."ExtraFiles"]
   SdMmcPciHcDxeExtra.uni
+
+[Pcd.IA32,Pcd.X64]
+  gEfiMdeModulePkgTokenSpaceGuid.PcdSdMmcGenericTimeoutValue		## CONSUMES
diff --git a/MdeModulePkg/Library/BrotliCustomDecompressLib/brotli b/MdeModulePkg/Library/BrotliCustomDecompressLib/brotli
index f4153a09f8..666c3280cc 160000
--- a/MdeModulePkg/Library/BrotliCustomDecompressLib/brotli
+++ b/MdeModulePkg/Library/BrotliCustomDecompressLib/brotli
@@ -1 +1 @@
-Subproject commit f4153a09f87cbb9c826d8fc12c74642bb2d879ea
+Subproject commit 666c3280cc11dc433c303d79a83d4ffbdd12cc8d
diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec
index 463e889e9a..40601c9583 100644
--- a/MdeModulePkg/MdeModulePkg.dec
+++ b/MdeModulePkg/MdeModulePkg.dec
@@ -1559,6 +1559,10 @@
   # @Prompt Maximum permitted FwVol section nesting depth (exclusive).
   gEfiMdeModulePkgTokenSpaceGuid.PcdFwVolDxeMaxEncapsulationDepth|0x10|UINT32|0x00000030
 
+  ## Indicates the default timeout value for SD/MMC Host Controller operations in microseconds.
+  # @Prompt SD/MMC Host Controller Operations Timeout (us).
+  gEfiMdeModulePkgTokenSpaceGuid.PcdSdMmcGenericTimeoutValue|1000000|UINT32|0x00000031
+
 [PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx]
   ## This PCD defines the Console output row. The default value is 25 according to UEFI spec.
   #  This PCD could be set to 0 then console output would be at max column and max row.
diff --git a/MdeModulePkg/MdeModulePkg.uni b/MdeModulePkg/MdeModulePkg.uni
index 27889a7280..b070f15ff2 100644
--- a/MdeModulePkg/MdeModulePkg.uni
+++ b/MdeModulePkg/MdeModulePkg.uni
@@ -1173,6 +1173,10 @@
                                                                                           " TRUE  - Capsule In Ram is supported.<BR>"
                                                                                           " FALSE - Capsule In Ram is not supported."
 
+#string STR_gEfiMdeModulePkgTokenSpaceGuid_PcdSdMmcGenericTimeoutValue_PROMPT #language en-US "SD/MMC Host Controller Operations Timeout (us)."
+
+#string STR_gEfiMdeModulePkgTokenSpaceGuid_PcdSdMmcGenericTimeoutValue_HELP   #language en-US "Indicates the default timeout value for SD/MMC Host Controller operations in microseconds."
+
 #string STR_gEfiMdeModulePkgTokenSpaceGuid_PcdCodRelocationDevPath_PROMPT  #language en-US "Capsule On Disk relocation device path."
 
 #string STR_gEfiMdeModulePkgTokenSpaceGuid_PcdCodRelocationDevPath_HELP  #language en-US   "Full device path of platform specific device to store Capsule On Disk temp relocation file.<BR>"
-- 
2.32.0

[PATCH 2/2] UefiPayloadPkg: Hookup SD Card timeout

[Sean Rhodes]

Hook SD_CARD_TIMEOUT build option to SdMmcGenericTimeoutValue PCD.

Cc: Guo Dong <guo.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Maurice Ma <maurice.ma@intel.com>
Cc: Benjamin You <benjamin.you@intel.com>
Signed-off-by: Sean Rhodes <sean@starlabs.systems>
---
 UefiPayloadPkg/UefiPayloadPkg.dsc | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/UefiPayloadPkg/UefiPayloadPkg.dsc b/UefiPayloadPkg/UefiPayloadPkg.dsc
index 1ce96a51c1..d75fe26426 100644
--- a/UefiPayloadPkg/UefiPayloadPkg.dsc
+++ b/UefiPayloadPkg/UefiPayloadPkg.dsc
@@ -33,6 +33,7 @@
   DEFINE UNIVERSAL_PAYLOAD            = FALSE
   DEFINE SECURITY_STUB_ENABLE         = TRUE
   DEFINE SMM_SUPPORT                  = FALSE
+  DEFINE SD_CARD_TIMEOUT              = 1000000
   #
   # SBL:      UEFI payload for Slim Bootloader
   # COREBOOT: UEFI payload for coreboot
@@ -398,6 +399,7 @@
 !if $(PERFORMANCE_MEASUREMENT_ENABLE)
   gEfiMdePkgTokenSpaceGuid.PcdPerformanceLibraryPropertyMask       | 0x1
 !endif
+  gEfiMdeModulePkgTokenSpaceGuid.PcdSdMmcGenericTimeoutValue|$(SD_CARD_TIMEOUT)
 
 [PcdsPatchableInModule.X64]
   gPcAtChipsetPkgTokenSpaceGuid.PcdRtcIndexRegister|$(RTC_INDEX_REGISTER)
-- 
2.32.0

Re: [PATCH 2/2] UefiPayloadPkg: Hookup SD Card timeout

[Ma, Maurice]

This patch depends on other changes in MdeModulePkg to add a new SdMmcGenericTimeoutValue PCD.   
The changes for UefiPayloadPkg look fine to me.

One minor comment is about the macro name.  Is this new introduced timeout PCD applicable to both SD and MMC device?
If so, "SD_CARD_TIMEOUT" might not be accurate to cover the MMC device.  Maybe use "SD_MMC_TIMEOUT" ?

Thanks,
Maurice

> -----Original Message-----
> From: Sean Rhodes <sean@starlabs.systems>
> Sent: Thursday, February 17, 2022 23:24
> To: devel@edk2.groups.io
> Cc: Dong, Guo <guo.dong@intel.com>; Rhodes, Sean <sean@starlabs.systems>;
> Ni, Ray <ray.ni@intel.com>; Ma, Maurice <maurice.ma@intel.com>; You,
> Benjamin <benjamin.you@intel.com>
> Subject: [PATCH 2/2] UefiPayloadPkg: Hookup SD Card timeout
> 
> Hook SD_CARD_TIMEOUT build option to SdMmcGenericTimeoutValue PCD.
> 
> Cc: Guo Dong <guo.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Maurice Ma <maurice.ma@intel.com>
> Cc: Benjamin You <benjamin.you@intel.com>
> Signed-off-by: Sean Rhodes <sean@starlabs.systems>
> ---
>  UefiPayloadPkg/UefiPayloadPkg.dsc | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/UefiPayloadPkg/UefiPayloadPkg.dsc
> b/UefiPayloadPkg/UefiPayloadPkg.dsc
> index 1ce96a51c1..d75fe26426 100644
> --- a/UefiPayloadPkg/UefiPayloadPkg.dsc
> +++ b/UefiPayloadPkg/UefiPayloadPkg.dsc
> @@ -33,6 +33,7 @@
>    DEFINE UNIVERSAL_PAYLOAD            = FALSE
> 
>    DEFINE SECURITY_STUB_ENABLE         = TRUE
> 
>    DEFINE SMM_SUPPORT                  = FALSE
> 
> +  DEFINE SD_CARD_TIMEOUT              = 1000000
> 
>    #
> 
>    # SBL:      UEFI payload for Slim Bootloader
> 
>    # COREBOOT: UEFI payload for coreboot
> 
> @@ -398,6 +399,7 @@
>  !if $(PERFORMANCE_MEASUREMENT_ENABLE)
> 
>    gEfiMdePkgTokenSpaceGuid.PcdPerformanceLibraryPropertyMask       | 0x1
> 
>  !endif
> 
> +
> gEfiMdeModulePkgTokenSpaceGuid.PcdSdMmcGenericTimeoutValue|$(SD_CAR
> D_TIMEOUT)
> 
> 
> 
>  [PcdsPatchableInModule.X64]
> 
> 
> gPcAtChipsetPkgTokenSpaceGuid.PcdRtcIndexRegister|$(RTC_INDEX_REGISTER)
> 
> --
> 2.32.0