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

["Wu, Hao A" <hao.a.wu@intel.com>]

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]
> -=-=-=-=-=-=
>