Re: [PATCH 2/2] MdeModulePkg/SdMmcPciHcDxe: Allow additional SDHCI versions

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

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Jeff
> Brasen
> Sent: Saturday, October 06, 2018 1:13 AM
> To: edk2-devel@lists.01.org
> Cc: Jeff Brasen
> Subject: [edk2] [PATCH 2/2] MdeModulePkg/SdMmcPciHcDxe: Allow additional
> SDHCI versions
> 
> Clock control is similar in SDHCI versions greater then version 3. Add
> support for more recent versions of the controller specification.
> 
> SD card support for 1.8V is also present in controller versions 3 and
> greater.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jeff Brasen <jbrasen@nvidia.com>

Hi,

Some minor comments:
A. I suggest to list a supported controller spec. versions range like:

   if (((ControllerVer & 0xFF) >= SD_MMC_HC_CTRL_VER_300) &&
       ((ControllerVer & 0xFF) <= SD_MMC_HC_CTRL_VER_420)) {

   to restrict the supported spec. versions for the driver.

B. I have submitted a Bugzilla tracker for your proposed series:

   https://bugzilla.tianocore.org/show_bug.cgi?id=1233
   
   Could you help to include this information in the commit messages of
   your proposed patches?

Best Regards,
Hao Wu

> ---
>  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c    | 2 +-
>  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c
> b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c
> index d3f77ed..12b4659 100644
> --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c
> +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c
> @@ -978,7 +978,7 @@ SdCardIdentification (
>      return Status;
>    }
> 
> -  if ((ControllerVer & 0xFF) == SD_MMC_HC_CTRL_VER_300) {
> +  if ((ControllerVer & 0xFF) >= SD_MMC_HC_CTRL_VER_300) {
>      S18r = TRUE;
>    } else if (((ControllerVer & 0xFF) == SD_MMC_HC_CTRL_VER_100) ||
> ((ControllerVer & 0xFF) == SD_MMC_HC_CTRL_VER_200)) {
>      S18r = FALSE;
> diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> index 3b9a184..de282bf 100644
> --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> @@ -782,7 +782,7 @@ SdMmcHcClockSupply (
>    //
>    // Set SDCLK Frequency Select and Internal Clock Enable fields in Clock
> Control register.
>    //
> -  if ((ControllerVer & 0xFF) == SD_MMC_HC_CTRL_VER_300) {
> +  if ((ControllerVer & 0xFF) >= SD_MMC_HC_CTRL_VER_300) {
>      ASSERT (Divisor <= 0x3FF);
>      ClockCtrl = ((Divisor & 0xFF) << 8) | ((Divisor & 0x300) >> 2);
>    } else if (((ControllerVer & 0xFF) == 0) || ((ControllerVer & 0xFF) == 1)) {
> --
> 2.7.4
> 
> 
> -----------------------------------------------------------------------------------
> This email message is for the sole use of the intended recipient(s) and may
> contain
> confidential information.  Any unauthorized review, use, disclosure or
> distribution
> is prohibited.  If you are not the intended recipient, please contact the sender
> by
> reply email and destroy all copies of the original message.
> -----------------------------------------------------------------------------------
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel