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

[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

[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

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

[Wu, Hao A]

3 inline comments below:


> -----Original Message-----
> From: Sean Rhodes <sean@starlabs.systems>
> Sent: Friday, February 18, 2022 3:24 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: [PATCH 1/2] 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    | 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]


1. "[Pcd]" should be fine.


> 
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdSdMmcGenericTimeoutValue
> 		## CONSUMES


2. Could you please help to run the PatchCheck.py to check your patch before sending out:

py BaseTools/Scripts/PatchCheck.py -1
Checking git commit: HEAD
MdeModulePkg/SdMmcPciHcDxe: Make timeout for SD card configurable
The commit message format passed all checks.
Code format is not valid:
 * Tab character used
   File: MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.inf
   Line:   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


3. Why change in BrotliCustomDecompressLib is needed for this patch?


Best Regards,
Hao Wu


> 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|1000
> 000|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_PRO
> MPT  #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