From: "Wu, Hao A" <hao.a.wu@intel.com>
To: "Albecki, Mateusz" <mateusz.albecki@intel.com>,
"edk2-devel@lists.01.org" <edk2-devel@lists.01.org>
Subject: Re: [PATCH 3/3] MdeModulePkg/UfsPassThruDxe: Add UFS info protocol
Date: Mon, 1 Apr 2019 06:53:20 +0000 [thread overview]
Message-ID: <B80AF82E9BFB8E4FBD8C89DA810C6A093C8BA004@SHSMSX104.ccr.corp.intel.com> (raw)
In-Reply-To: <ae4f34f8dc69443743d3279880cd0204fe8861cf.1553774396.git.mateusz.albecki@intel.com>
Hello Mateusz,
A couple of general comments:
A. I suggest to break this commit into 2 patches:
The first one will just introduce the new protocol.
The second one will update the UfsPassThruDxe driver to consume this new
protocol.
B. There has been a Bugzilla tracker for the feature you add in this
patch:
https://bugzilla.tianocore.org/show_bug.cgi?id=1343
Could you help to add this information in the commit log message? Thanks.
C. I prefer the new protocol to have platform level singleton instance:
I do not see much value for a platform to produce multiple instances of
this protocol, I think the additional UIC commands needed during the UFS
HC initialization for a specific platform is deterministic.
Also, the selection of protocol instance implemented in function
GetUfsHcInfoProtocol() is by:
1) LocateHandleBuffer() to get all protocol instances;
2) Choose the 1st instance if the 'Supported' service returns without
error.
I think this will bring some kind of confusion to the protocol producers,
since they do not know which one will be selected when there are multiple
instances of the protocol.
> -----Original Message-----
> From: Albecki, Mateusz
> Sent: Thursday, March 28, 2019 9:56 PM
> To: edk2-devel@lists.01.org
> Cc: Albecki, Mateusz; Wu, Hao A
> Subject: [PATCH 3/3] MdeModulePkg/UfsPassThruDxe: Add UFS info protocol
>
> UFS host controller specification allows for implementation specific
> UIC programming to take place just after host controller enable and before
> device detection. Since there is no way for generic driver to anticipate
> implementation specific programming we add a UFS info protocol
> which allows the implementation specific code to pass this information
> to generic driver. UFS info protocol is located by the driver at the
> BindingStart call and the supported instance is retained throught entire
throught -> through
> driver lifetime. Presence of the protocol is optional.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Cc: Hao Wu <hao.a.wu@intel.com>
> Signed-off-by: Albecki Mateusz <mateusz.albecki@intel.com>
> ---
> MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.c | 62
> ++++++++++++++++++
> MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h | 2 +
> .../Bus/Ufs/UfsPassThruDxe/UfsPassThruDxe.inf | 1 +
> .../Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c | 60 +++++++++++++++++
> .../Include/Protocol/UfsHostControllerInfo.h | 75
> ++++++++++++++++++++++
> MdeModulePkg/MdeModulePkg.dec | 3 +
> 6 files changed, 203 insertions(+)
> create mode 100644 MdeModulePkg/Include/Protocol/UfsHostControllerInfo.h
>
> diff --git a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.c
> b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.c
> index ea329618dc..a41079e311 100644
> --- a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.c
> +++ b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.c
> @@ -40,6 +40,7 @@ UFS_PASS_THRU_PRIVATE_DATA gUfsPassThruTemplate
> = {
> UfsRwUfsAttribute
> },
> 0, // UfsHostController
> + NULL, // UfsInfo
> 0, // UfsHcBase
> 0, // Capabilities
> 0, // TaskTag
> @@ -776,6 +777,66 @@ UfsFinishDeviceInitialization (
> return EFI_SUCCESS;
> }
>
> +/**
> + Locates UFS HC infor protocol suitable for this controller.
infor -> info
> +
> + @param[in] DriverBinding Pointer to driver binding protocol
> + @param[in] Private Pointer to host controller private data
> + @param[in] ControllerHandle Controller to which driver is bound
> +**/
> +VOID
> +GetUfsHcInfoProtocol (
> + IN EFI_DRIVER_BINDING_PROTOCOL *DriverBinding,
> + IN UFS_PASS_THRU_PRIVATE_DATA *Private,
> + IN EFI_HANDLE ControllerHandle
> + )
> +{
> + EFI_STATUS Status;
> + EFI_HANDLE *ProtocolHandleArray;
> + UINTN NoHandles;
> + UINTN HandleIndex;
> + UFS_HC_INFO_PROTOCOL *UfsHcInfo;
> +
> + Status = gBS->LocateHandleBuffer (
> + ByProtocol,
> + &gUfsHcInfoProtocolGuid,
> + NULL,
> + &NoHandles,
> + &ProtocolHandleArray
> + );
> + if (EFI_ERROR (Status)) {
> + return;
> + }
> +
> + for (HandleIndex = 0; HandleIndex < NoHandles; HandleIndex++) {
> + Status = gBS->OpenProtocol (
> + ProtocolHandleArray[HandleIndex],
> + &gUfsHcInfoProtocolGuid,
> + &UfsHcInfo,
> + DriverBinding->DriverBindingHandle,
> + ControllerHandle,
> + EFI_OPEN_PROTOCOL_BY_DRIVER
> + );
> + if (EFI_ERROR (Status)) {
> + continue;
> + }
> + if (!EFI_ERROR(UfsHcInfo->Supported (UfsHcInfo, ControllerHandle))) {
> + Private->UfsHcInfo = UfsHcInfo;
> + break;
> + } else {
> + Status = gBS->CloseProtocol (
> + ProtocolHandleArray[HandleIndex],
> + &gUfsHcInfoProtocolGuid,
> + DriverBinding->DriverBindingHandle,
> + ControllerHandle
> + );
> + ASSERT_EFI_ERROR (Status);
> + }
> + }
> +
> + FreePool (ProtocolHandleArray);
> +}
> +
> /**
> Starts a device controller or a bus controller.
>
> @@ -871,6 +932,7 @@ UfsPassThruDriverBindingStart (
> Private->UfsHostController = UfsHc;
> Private->UfsHcBase = UfsHcBase;
> InitializeListHead (&Private->Queue);
> + GetUfsHcInfoProtocol (This, Private, Controller);
>
> //
> // Initialize UFS Host Controller H/W.
> diff --git a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h
> b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h
> index e591e78661..55a8ed9bdf 100644
> --- a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h
> +++ b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h
> @@ -29,6 +29,7 @@
> #include <Library/UefiBootServicesTableLib.h>
> #include <Library/DevicePathLib.h>
> #include <Library/TimerLib.h>
> +#include <Protocol/UfsHostControllerInfo.h>
A minor comment here:
You can put the above '#include' together with other protocol header files
rather than appending it after the libraries includes.
>
> #include "UfsPassThruHci.h"
>
> @@ -66,6 +67,7 @@ typedef struct _UFS_PASS_THRU_PRIVATE_DATA {
> EFI_EXT_SCSI_PASS_THRU_PROTOCOL ExtScsiPassThru;
> EFI_UFS_DEVICE_CONFIG_PROTOCOL UfsDevConfig;
> EDKII_UFS_HOST_CONTROLLER_PROTOCOL *UfsHostController;
> + UFS_HC_INFO_PROTOCOL *UfsHcInfo;
> UINTN UfsHcBase;
> UINT32 Capabilities;
>
> diff --git a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruDxe.inf
> b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruDxe.inf
> index e550cd02b4..12b01771ff 100644
> --- a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruDxe.inf
> +++ b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruDxe.inf
> @@ -59,6 +59,7 @@
> gEfiExtScsiPassThruProtocolGuid ## BY_START
> gEfiUfsDeviceConfigProtocolGuid ## BY_START
> gEdkiiUfsHostControllerProtocolGuid ## TO_START
> + gUfsHcInfoProtocolGuid
gUfsHcInfoProtocolGuid ->
gUfsHcInfoProtocolGuid ## SOMETIMES_CONSUMES
>
> [UserExtensions.TianoCore."ExtraFiles"]
> UfsPassThruExtra.uni
> diff --git a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c
> b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c
> index c37161c4ae..125f2f2516 100644
> --- a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c
> +++ b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c
> @@ -2068,6 +2068,61 @@ UfsInitTransferRequestList (
> return EFI_SUCCESS;
> }
>
> +/**
> + Performs additional UIC programming if it has been specified by platform in
> UFS info protocol.
> +
> + @param[in] Private Pointer to host controller private data
> +
> + @retval EFI_SUCCESS Programming finished successfully or not requested
> + @retval others Programming failed
> +**/
> +EFI_STATUS
> +UfsProgramAdditionalUicAttributes (
> + IN UFS_PASS_THRU_PRIVATE_DATA *Private
> + )
> +{
> + EFI_STATUS Status;
> + UIC_ATTRIBUTE *UicAttrArray;
> + UINTN NoAttributes;
> + UINTN AttributeIndex;
> +
> + //
> + // No info protocol means that no additional programming should be
> performed
> + //
> + if (Private->UfsHcInfo == NULL) {
> + return EFI_SUCCESS;
> + }
> +
> + //
> + // If we failed to get the programming table we assume that platform
> doesn't want to do any additional programming
> + //
> + Status = Private->UfsHcInfo->GetUicProgrammingTable (Private->UfsHcInfo,
> &UicAttrArray, &NoAttributes);
> + if (EFI_ERROR (Status)) {
> + return EFI_SUCCESS;
> + }
Please help to free the content pointed by 'UicAttrArray'.
> +
> + for (AttributeIndex = 0; AttributeIndex < NoAttributes; AttributeIndex++) {
> + DEBUG ((DEBUG_ERROR, "Programing UIC attribute = %d, selector index
DEBUG_INFO should be enough.
> = %d, set type = %d, value = %d\n",
> + UicAttrArray[AttributeIndex].MibAttribute,
> UicAttrArray[AttributeIndex].GenSelectorIndex,
> + UicAttrArray[AttributeIndex].AttrSetType,
> UicAttrArray[AttributeIndex].AttributeValue));
> + Status = UfsExecUicCommands (
> + Private,
> + UfsUicDmeSet,
> + (UicAttrArray[AttributeIndex].MibAttribute << 16) |
> (UicAttrArray[AttributeIndex].GenSelectorIndex),
> + UicAttrArray[AttributeIndex].AttrSetType << 16,
> + UicAttrArray[AttributeIndex].AttributeValue
> + );
> + if (EFI_ERROR (Status)) {
> + DEBUG ((DEBUG_ERROR, "Failed to program UIC attribute = %d, selector
> index = %d, set type = %d, value = %d\n",
> + UicAttrArray[AttributeIndex].MibAttribute,
> UicAttrArray[AttributeIndex].GenSelectorIndex,
> + UicAttrArray[AttributeIndex].AttrSetType,
> UicAttrArray[AttributeIndex].AttributeValue));
> + return Status;
> + }
> + }
> +
> + return EFI_SUCCESS;
> +}
> +
> /**
> Initialize the UFS host controller.
>
> @@ -2090,6 +2145,11 @@ UfsControllerInit (
> return Status;
> }
>
> + Status = UfsProgramAdditionalUicAttributes (Private);
> + if (EFI_ERROR (Status)) {
> + DEBUG ((DEBUG_ERROR, "UfsControllerInit: Failed to perform additional
> UIC programming\n"));
> + }
> +
> Status = UfsDeviceDetection (Private);
> if (EFI_ERROR (Status)) {
> DEBUG ((DEBUG_ERROR, "UfsControllerInit: Device Detection Fails, Status
> = %r\n", Status));
> diff --git a/MdeModulePkg/Include/Protocol/UfsHostControllerInfo.h
> b/MdeModulePkg/Include/Protocol/UfsHostControllerInfo.h
> new file mode 100644
> index 0000000000..c730127482
> --- /dev/null
> +++ b/MdeModulePkg/Include/Protocol/UfsHostControllerInfo.h
> @@ -0,0 +1,75 @@
> +/** @file
> +
> + Universal Flash Storage Host Controller info Protocol.
> +
> +Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
> +This program and the accompanying materials are licensed and made
> available under
> +the terms and conditions of the BSD License that accompanies this distribution.
> +The full text of the license may be found at
> +http://opensource.org/licenses/bsd-license.php.
> +
> +THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS
> OR IMPLIED.
> +
> +**/
> +
> +#ifndef __UFS_HC_INFO_PROTOCOL_H__
> +#define __UFS_HC_INFO_PROTOCOL_H__
> +
> +#define UFS_HC_INFO_PROTOCOL_VERSION 1
> +
> +extern EFI_GUID gUfsHcInfoProtocolGuid;
I suggest to rename gUfsHcInfoProtocolGuid to:
gEdkiiUfsHcInfoProtocolGuid
which I think is a common naming convention for non UEFI spec protocols
within MdeModulePkg.
> +
> +typedef struct _UFS_HC_INFO_PROTOCOL UFS_HC_INFO_PROTOCOL;
> +
> +typedef struct {
> + UINT16 MibAttribute;
> + UINT16 GenSelectorIndex;
> + UINT8 AttrSetType;
> + UINT32 AttributeValue;
> +} UIC_ATTRIBUTE;
> +
> +/**
> + Checks if this instance of the info protocol supports
> + given host controller.
> +
> + @param[in] This Pointer to this instance of
> UFS_HC_INFO_PROTOCOL
> + @param[in] ControllerHandle Controller to check
> +
> + @retval EFI_SUCCESS This instance of info protocol supports given controller
> + @retval others This instance of info protocol doesn't support given
> controller
> +**/
> +typedef
> +EFI_STATUS
> +(*UFS_INFO_CONTROLLER_SUPPORTED) (
> + IN UFS_HC_INFO_PROTOCOL *This,
> + IN EFI_HANDLE ControllerHandle
> + );
> +
> +/**
> + Get the UIC programming table to be used just after host controller
> + enabling and before device detection.
> +
> + @param[in] This Pointer to this instance of
> UFS_HC_INFO_PROTOCOL
> + @param[in] UicAttributeArray Out variable for address of the table
> + @param[in] NoAttributes Out variable for number of attributes in the
> array
'@param[out]' for UicAttributeArray & NoAttributes.
Also, for 'UicAttributeArray', I think the content pointed by this pointer
is allocated by the protocol producer and should be freed by the consumer
after being used, right?
If so, please help to refine the description comments for
'UicAttributeArray'.
> +
> + @retval EFI_SUCCESS Table successfully found and returned
> + @retval others Table wasn't located successfully UIC programming
> shouldn't be performed.
> +**/
> +typedef
> +EFI_STATUS
> +(*UFS_INFO_GET_UIC_PROGRAMMING_TABLE) (
> + IN UFS_HC_INFO_PROTOCOL *This,
> + OUT UIC_ATTRIBUTE **UicAttributeArray,
> + OUT UINTN *NoAttributes
> + );
Judging from the usage of this service in function
UfsProgramAdditionalUicAttributes(), I think there is an assumption that
platforms will only require additional 'DME_SET' UIC commands.
I am not sure if we need to make this service a bit more general. So how
about changing the 'UIC_ATTRIBUTE' structure to:
typedef struct {
UINT8 UicOpcode;
UINT32 Arg1;
UINT32 Arg2;
UINT32 Arg3;
} UIC_COMMAND;
and update the service to:
typedef
EFI_STATUS
(*UFS_INFO_GET_UIC_PROGRAMMING_TABLE) (
IN UFS_HC_INFO_PROTOCOL *This,
OUT UIC_COMMAND **UicCommandArray,
OUT UINTN *NoCommand
);
> +
> +struct _UFS_HC_INFO_PROTOCOL {
> + UINT32 Version;
> + UFS_INFO_CONTROLLER_SUPPORTED Supported;
If there will be only one protocol instance within system, do you think we
can drop the above 'Supported' service?
Best Regards,
Hao Wu
> + UFS_INFO_GET_UIC_PROGRAMMING_TABLE GetUicProgrammingTable;
> +};
> +
> +#endif
> +
> diff --git a/MdeModulePkg/MdeModulePkg.dec
> b/MdeModulePkg/MdeModulePkg.dec
> index a2130bc439..c6be8f12a4 100644
> --- a/MdeModulePkg/MdeModulePkg.dec
> +++ b/MdeModulePkg/MdeModulePkg.dec
> @@ -581,6 +581,9 @@
> ## Include/Protocol/UfsHostController.h
> gEdkiiUfsHostControllerProtocolGuid = { 0xebc01af5, 0x7a9, 0x489e, { 0xb7,
> 0xce, 0xdc, 0x8, 0x9e, 0x45, 0x9b, 0x2f } }
>
> + ## Include/Protocol/UfsHostControllerInfo.h
> + gUfsHcInfoProtocolGuid = { 0x3d18ba13, 0xd9b1, 0x4dd4, {0xb9, 0x16, 0xd3,
> 0x07, 0x96, 0x53, 0x9e, 0xd8}}
> +
> ## Include/Protocol/EsrtManagement.h
> gEsrtManagementProtocolGuid = { 0xa340c064, 0x723c, 0x4a9c, { 0xa4,
> 0xdd, 0xd5, 0xb4, 0x7a, 0x26, 0xfb, 0xb0 }}
>
> --
> 2.14.1.windows.1
next prev parent reply other threads:[~2019-04-01 6:53 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-03-28 13:48 [PATCH 0/3] Implement UFS info protocol to pass additional UIC programming data from platform/silicon specific driver Albecki, Mateusz
2019-03-28 13:48 ` [PATCH 1/3] MdeModulePkg/UfsPassThruDxe: Fix unaligned data transfer handling Albecki, Mateusz
2019-03-28 13:56 ` [PATCH 2/3] MdeModulePkg/UfsPassThruDxe: Refactor UFS device presence detection Albecki, Mateusz
2019-04-01 6:53 ` Wu, Hao A
2019-03-28 13:56 ` [PATCH 3/3] MdeModulePkg/UfsPassThruDxe: Add UFS info protocol Albecki, Mateusz
2019-04-01 6:53 ` Wu, Hao A [this message]
[not found] ` <92CF190FF2351747A47C1708F0E09C0875DE7165@IRSMSX102.ger.corp.intel.com>
2019-04-12 6:35 ` Wu, Hao A
2019-04-12 23:22 ` Albecki, Mateusz
2019-04-15 5:56 ` Wu, Hao A
2019-04-01 6:53 ` [PATCH 0/3] Implement UFS info protocol to pass additional UIC programming data from platform/silicon specific driver Wu, Hao A
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=B80AF82E9BFB8E4FBD8C89DA810C6A093C8BA004@SHSMSX104.ccr.corp.intel.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox