Re: [edk2-devel] [PATCH] [PATCH v1] MdeModulePkg: Add bRefClkFreq card attribute programming support

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

A general level question:
Is it possible that for a system with multiple UFS Host Controllers (HCs), each HC will have different bRefClkFreq values?
If so, my thought is that a new protocol service might be needed to get the bRefClkFreq value from each UFS HC:

typedef
EFI_STATUS
(EFIAPI *EDKII_UFS_HC_PLATFORM_GET_REF_CLK_FREQ)(
  IN     EFI_HANDLE                             ControllerHandle,
     OUT EDKII_UFS_CARD_REF_CLK_FREQ_ATTRIBUTE  *RefClkFreq
  );


Also, some more inline comments below:


> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Bandaru,
> Purna Chandra Rao
> Sent: Thursday, March 3, 2022 5:40 PM
> To: devel@edk2.groups.io
> Cc: Bandaru, Purna Chandra Rao <purna.chandra.rao.bandaru@intel.com>
> Subject: [edk2-devel] [PATCH] [PATCH v1] MdeModulePkg: Add bRefClkFreq
> card attribute programming support
> 
> When the UFS card comes out of Manufacturer, bRefClkFreq attribute is set
> to 1h on the UFS card as per the "MDV” (Manufacturer Default Value)
> specified by the spec JESD220*. However, depends on the UFS host system
> environment, it need to be set to correct value.
> 
> Reference Clock Frequency value
> 0h:19.2 MHz
> 1h: 26 MHz
> 2h: 38.4 MHz
> 3h: Obsolete
> Others: Reserved
> 
> Hsd-es-id: https://bugzilla.tianocore.org/show_bug.cgi?id=3851


REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3851


> 
> Change-Id: If09fce724f61773ce1b8771d7bf65075496d9044


Please help to remove the above 'Change-Id' information.


> Signed-off-by: Purna Chandra Rao Bandaru
> <purna.chandra.rao.bandaru@intel.com>
> ---
>  .../Bus/Ufs/UfsPassThruDxe/UfsPassThru.c      | 32 ++++++++++++++++++-
>  .../Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c   | 10 +-----
>  .../Protocol/UfsHostControllerPlatform.h      | 15 +++++++--
>  3 files changed, 45 insertions(+), 12 deletions(-)
> 
> diff --git a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.c
> b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.c
> index 4c2d6ae27f..ba4f661b1c 100644
> --- a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.c
> +++ b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.c
> @@ -1,6 +1,6 @@
>  /** @file
> 
> -  Copyright (c) 2014 - 2021, Intel Corporation. All rights reserved.<BR>
> +  Copyright (c) 2014 - 2022, Intel Corporation. All rights reserved.<BR>
>    Copyright (c) Microsoft Corporation.<BR>
>    SPDX-License-Identifier: BSD-2-Clause-Patent
> 
> @@ -843,6 +843,8 @@ UfsPassThruDriverBindingStart (
>    UFS_DEV_DESC                        DeviceDescriptor;
>    UINT32                              UnitDescriptorSize;
>    UINT32                              DeviceDescriptorSize;
> +  EDKII_UFS_CARD_REF_CLK_FREQ_ATTRIBUTE Attributes;
> +  UINT8                               RefClkAttr;
> 
>    Status    = EFI_SUCCESS;
>    UfsHc     = NULL;
> @@ -916,6 +918,34 @@ UfsPassThruDriverBindingStart (
>      DEBUG ((DEBUG_ERROR, "Ufs Host Controller Initialization Error, Status
> = %r\n", Status));
>      goto Error;
>    }
> +  if ((mUfsHcPlatform != NULL) &&
> +      ((mUfsHcPlatform->RefClkFreq == EdkiiUfsCardRefClkFreq19p2Mhz) ||
> +       (mUfsHcPlatform->RefClkFreq == EdkiiUfsCardRefClkFreq26Mhz) ||
> +       (mUfsHcPlatform->RefClkFreq == EdkiiUfsCardRefClkFreq38p4Mhz))) {
> +    RefClkAttr = UfsAttrRefClkFreq;
> +    Attributes = EdkiiUfsCardRefClkFreqObsolete;
> +    Status = UfsRwAttributes (Private, TRUE, RefClkAttr, 0, 0, (UINT32 *)
> &Attributes);
> +    if (!EFI_ERROR (Status)) {
> +      if (Attributes != mUfsHcPlatform->RefClkFreq) {
> +        Attributes = mUfsHcPlatform->RefClkFreq;
> +        DEBUG ((DEBUG_INFO, "Setting bRefClkFreq attribute(%x) to %x\n  0 ->
> 19.2 Mhz\n  1 -> 26 Mhz\n  2 -> 38.4 Mhz\n  3 -> Obsolete\n", RefClkAttr,
> Attributes));


Could you help to re-format the above DEBUG() into multiline style? So that the line won't be very long.


> +        Status = UfsRwAttributes (Private, FALSE, RefClkAttr, 0, 0, (UINT32 *)
> &Attributes);
> +        if (EFI_ERROR (Status)) {
> +          DEBUG ((DEBUG_ERROR, "Failed to Change Reference Clock Attribute
> to %d, Status = %r \n", mUfsHcPlatform->RefClkFreq, Status));


Could you help to re-format the above DEBUG() into multiline style too?


> +        }
> +      }
> +    } else {
> +      DEBUG ((DEBUG_ERROR, "Failed to Read Reference Clock Attribute,
> Status = %r \n", Status));
> +    }
> +  }
> +
> +  if ((mUfsHcPlatform != NULL) && (mUfsHcPlatform->Callback != NULL)) {
> +    Status = mUfsHcPlatform->Callback (Private->Handle,
> EdkiiUfsHcPostLinkStartup, &Private->UfsHcDriverInterface);
> +    if (EFI_ERROR (Status)) {
> +      DEBUG ((DEBUG_ERROR, "Failure from platform driver during
> EdkiiUfsHcPostLinkStartup, Status = %r\n", Status));
> +      return Status;
> +    }
> +  }
> 
>    //
>    // UFS 2.0 spec Section 13.1.3.3:
> diff --git a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c
> b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c
> index eba35cc669..4a9fa01e7d 100644
> --- a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c
> +++ b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c
> @@ -2,7 +2,7 @@
>    UfsPassThruDxe driver is used to produce EFI_EXT_SCSI_PASS_THRU
> protocol interface
>    for upper layer application to execute UFS-supported SCSI cmds.
> 
> -  Copyright (c) 2014 - 2019, Intel Corporation. All rights reserved.<BR>
> +  Copyright (c) 2014 - 2022, Intel Corporation. All rights reserved.<BR>
>    Copyright (c) Microsoft Corporation.<BR>
>    SPDX-License-Identifier: BSD-2-Clause-Patent
> 
> @@ -1970,14 +1970,6 @@ UfsDeviceDetection (
>          return EFI_DEVICE_ERROR;
>        }
>      } else {
> -      if ((mUfsHcPlatform != NULL) && (mUfsHcPlatform->Callback != NULL)) {
> -        Status = mUfsHcPlatform->Callback (Private->Handle,
> EdkiiUfsHcPostLinkStartup, &Private->UfsHcDriverInterface);
> -        if (EFI_ERROR (Status)) {
> -          DEBUG ((DEBUG_ERROR, "Failure from platform driver during
> EdkiiUfsHcPostLinkStartup, Status = %r\n", Status));
> -          return Status;
> -        }
> -      }
> -


Is it possible to put the "Change Reference Clock Attribute" codes here in UfsDeviceDetection()?
Moving the location of the 'EdkiiUfsHcPostLinkStartup' callback seems like a behavior change from the origin driver.

Best Regards,
Hao Wu


>        return EFI_SUCCESS;
>      }
>    }
> diff --git a/MdeModulePkg/Include/Protocol/UfsHostControllerPlatform.h
> b/MdeModulePkg/Include/Protocol/UfsHostControllerPlatform.h
> index faa82d0c4e..32e9f6488c 100644
> --- a/MdeModulePkg/Include/Protocol/UfsHostControllerPlatform.h
> +++ b/MdeModulePkg/Include/Protocol/UfsHostControllerPlatform.h
> @@ -1,7 +1,7 @@
>  /** @file
>    EDKII_UFS_HC_PLATFORM_PROTOCOL definition.
> 
> -Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) 2019 - 2022, Intel Corporation. All rights reserved.<BR>
>  SPDX-License-Identifier: BSD-2-Clause-Patent
> 
>  **/
> @@ -11,7 +11,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> 
>  #include <Protocol/UfsHostController.h>
> 
> -#define EDKII_UFS_HC_PLATFORM_PROTOCOL_VERSION  1
> +#define EDKII_UFS_HC_PLATFORM_PROTOCOL_VERSION  2
> 
>  extern EFI_GUID  gEdkiiUfsHcPlatformProtocolGuid;
> 
> @@ -83,6 +83,13 @@ typedef enum {
>    EdkiiUfsHcPostLinkStartup
>  } EDKII_UFS_HC_PLATFORM_CALLBACK_PHASE;
> 
> +typedef enum {
> +  EdkiiUfsCardRefClkFreq19p2Mhz,
> +  EdkiiUfsCardRefClkFreq26Mhz,
> +  EdkiiUfsCardRefClkFreq38p4Mhz,
> +  EdkiiUfsCardRefClkFreqObsolete
> +} EDKII_UFS_CARD_REF_CLK_FREQ_ATTRIBUTE;
> +
>  /**
>    Callback function for platform driver.
> 
> @@ -118,6 +125,10 @@ struct _EDKII_UFS_HC_PLATFORM_PROTOCOL {
>    /// for host controller.
>    ///
>    EDKII_UFS_HC_PLATFORM_CALLBACK            Callback;
> +  ///
> +  /// Reference Clock Frequency Ufs Card Attribute that need to be set in
> this Ufs Host Environment.
> +  ///
> +  EDKII_UFS_CARD_REF_CLK_FREQ_ATTRIBUTE     RefClkFreq;
>  };
> 
>  #endif
> --
> 2.26.2.windows.1
> 
> 
> 
> 
>