public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [PATCH 0/3] Implement UFS info protocol to pass additional UIC programming data from platform/silicon specific driver
@ 2019-03-28 13:48 Albecki, Mateusz
  2019-03-28 13:48 ` [PATCH 1/3] MdeModulePkg/UfsPassThruDxe: Fix unaligned data transfer handling Albecki, Mateusz
                   ` (3 more replies)
  0 siblings, 4 replies; 11+ messages in thread
From: Albecki, Mateusz @ 2019-03-28 13:48 UTC (permalink / raw)
  To: edk2-devel; +Cc: Albecki, Mateusz, Wu, Hao A

UFS specification allows for additional, implementation specific, programming to be done during host controller initialization. Since some hosts
might require it to allow for device detection we add a way for platform/silicon specific code to pass required attributes and their values to
generic UFS driver. Please see UFS 2.0 specification for details of the host controller initialization sequence(JESD223B section 7.1.1 step 4).
Patchset consists of 3 patches to achieve this. The first one is a bugfix for data transfers that are not aligned to DWORD boundary such as SCSI SENSE
command. The second one is code refactoring for device presence detection that cleans up the function that sends UIC commands. Final patch defines
UFS info protocol and implements driver logic for servicing it.

Tests performed:
-Test UFS LU enumeration with UFS 2.0 card with 3 enabled LUs
-Test that no unaligned data transfers are performed during enumeration
-Test that UIC is programmed according to platform table
-Test that driver operation is not impacted when no UFS info protocol is installed

CC: Wu, Hao A <hao.a.wu@intel.com>

Albecki, Mateusz (3):
  MdeModulePkg/UfsPassThruDxe: Fix unaligned data transfer handling
  MdeModulePkg/UfsPassThruDxe: Refactor UFS device presence detection
  MdeModulePkg/UfsPassThruDxe: Add UFS info protocol

 MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.c  |  62 +++++
 MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h  |   3 +
 .../Bus/Ufs/UfsPassThruDxe/UfsPassThruDxe.inf      |   1 +
 .../Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c        | 299 +++++++++++++++------
 .../Include/Protocol/UfsHostControllerInfo.h       |  75 ++++++
 MdeModulePkg/MdeModulePkg.dec                      |   3 +
 6 files changed, 356 insertions(+), 87 deletions(-)
 create mode 100644 MdeModulePkg/Include/Protocol/UfsHostControllerInfo.h

-- 
2.14.1.windows.1

--------------------------------------------------------------------

Intel Technology Poland sp. z o.o.
ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | Kapital zakladowy 200.000 PLN.

Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakiekolwiek
przegladanie lub rozpowszechnianie jest zabronione.
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by
others is strictly prohibited.



^ permalink raw reply	[flat|nested] 11+ messages in thread
* Re: [PATCH 1/3] MdeModulePkg/UfsPassThruDxe: Fix unaligned data transfer handling
@ 2019-04-01  6:53 Wu, Hao A
  0 siblings, 0 replies; 11+ messages in thread
From: Wu, Hao A @ 2019-04-01  6:53 UTC (permalink / raw)
  To: Albecki, Mateusz, edk2-devel@lists.01.org

Hello Mateusz,

There has been a Bugzilla tracker for the issue you addressed in this
patch:
https://bugzilla.tianocore.org/show_bug.cgi?id=1341

Could you help to add this information in the commit log message? Thanks.

> -----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 1/3] MdeModulePkg/UfsPassThruDxe: Fix unaligned data
> transfer handling
> 
> Since UFS spcification requiers the data buffer specified

specification -> specification
requiers -> requires


> in PRDT to be DWORD aligned in size we had a code in
> UfsInitUtpPrdt that aligned the data buffer by rounding down
> the buffer size to DWORD boundary. This meant that for SCSI
> commands that wanted to perform unaligned data transfer(such as
> SENSE command) we specified to small buffer for the data to fit
> and transfer was aborted. This change introduces code that allocates
> auxilary DWORD aligned data buffer for unaligned transfer. Device

auxilary -> auxiliary

There are some 'auxilary' typos within the update in file UfsPassThruHci.c
as well, please help to update them.


> transfers data to aligned buffer and when data transfer is over driver copies

The above line seems too long. Please help to refine it under 76 chars.


> data from aligned buffer to data buffer passed by user.
> 
> 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.h  |   1 +
>  .../Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c        | 190 +++++++++++++++-----
> -
>  2 files changed, 135 insertions(+), 56 deletions(-)
> 
> diff --git a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h
> b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h
> index b8937842b8..e591e78661 100644
> --- a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h
> +++ b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h
> @@ -98,6 +98,7 @@ typedef struct {
>    UINT32                                        CmdDescSize;
>    VOID                                          *CmdDescHost;
>    VOID                                          *CmdDescMapping;
> +  VOID                                          *AlignedDataBuf;
>    VOID                                          *DataBufMapping;
> 
>    EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET    *Packet;
> diff --git a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c
> b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c
> index 0238264878..9d0793c6be 100644
> --- a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c
> +++ b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c
> @@ -400,17 +400,13 @@ UfsInitUtpPrdt (
>    UINT8      *Remaining;
>    UINTN      PrdtNumber;
> 
> -  if ((BufferSize & (BIT0 | BIT1)) != 0) {
> -    BufferSize &= ~(BIT0 | BIT1);
> -    DEBUG ((DEBUG_WARN, "UfsInitUtpPrdt: The BufferSize [%d] is not dword-
> aligned!\n", BufferSize));
> -  }
> +  ASSERT (((UINTN)Buffer & (BIT0 | BIT1)) == 0);
> +  ASSERT ((BufferSize & (BIT1 | BIT0)) == 0);
> 
>    if (BufferSize == 0) {
>      return EFI_SUCCESS;
>    }
> 
> -  ASSERT (((UINTN)Buffer & (BIT0 | BIT1)) == 0);
> -
>    RemainingLen = BufferSize;
>    Remaining    = Buffer;
>    PrdtNumber   = (UINTN)DivU64x32 ((UINT64)BufferSize +
> UFS_MAX_DATA_LEN_PER_PRD - 1, UFS_MAX_DATA_LEN_PER_PRD);
> @@ -1212,8 +1208,6 @@ UfsSetFlag (
>    return Status;
>  }
> 
> -
> -
>  /**
>    Read specified flag from a UFS device.
> 
> @@ -1323,6 +1317,129 @@ Exit:
>    return Status;
>  }
> 
> +/**
> +  Cleanup data buffers after data transfer. This function
> +  also takes care to copy all data to user memory pool for
> +  unaligned data transfers.
> +
> +  @param[in] Private   Pointer to the UFS_PASS_THRU_PRIVATE_DATA
> +  @param[in] TransReq  Pointer to the transfer request
> +**/
> +VOID
> +UfsReconcileDataTransferBuffer (
> +  IN UFS_PASS_THRU_PRIVATE_DATA  *Private,
> +  IN UFS_PASS_THRU_TRANS_REQ     *TransReq
> +  )
> +{
> +  if (TransReq->DataBufMapping != NULL) {
> +    Private->UfsHostController->Unmap (
> +                                  Private->UfsHostController,
> +                                  TransReq->DataBufMapping
> +                                  );
> +  }
> +
> +  //
> +  // Check if unaligned transfer was performed. If it was and we read
> +  // data from device copy memory to user data buffers before cleanup.
> +  // The assumption is if auxilary aligned data buffer is not NULL then
> +  // unaligned transfer has been performed.
> +  //
> +  if (TransReq->AlignedDataBuf != NULL) {
> +    if (TransReq->Packet->DataDirection ==
> EFI_EXT_SCSI_DATA_DIRECTION_READ) {
> +      CopyMem (TransReq->Packet->InDataBuffer, TransReq->AlignedDataBuf,
> TransReq->Packet->InTransferLength);
> +    }
> +    FreePool (TransReq->AlignedDataBuf);
> +    TransReq->AlignedDataBuf = NULL;
> +  }
> +}
> +
> +/**
> +  Prepare data buffer for transfer
> +
> +  @param[in] Private   Pointer to the UFS_PASS_THRU_PRIVATE_DATA
> +  @param[in] TransReq  Pointer to the transfer request

Minor comment:
@param[in, out] TransReq ...


> +
> +  @retval EFI_DEVICE_ERROR  Failed to prepare buffer for transfer
> +  @retval EFI_SUCCESS       Buffer ready for transfer
> +**/
> +EFI_STATUS
> +UfsPrepareDataTransferBuffer (
> +  IN     UFS_PASS_THRU_PRIVATE_DATA  *Private,
> +  IN OUT UFS_PASS_THRU_TRANS_REQ     *TransReq
> +  )
> +{
> +  EFI_STATUS                           Status;
> +  VOID                                 *DataBuf;
> +  VOID                                 *AlignedDataBuf;

'AlignedDataBuf' seems not used here. Please help to remove it.


> +  UINT32                               DataLen;
> +  UINTN                                MapLength;
> +  EFI_PHYSICAL_ADDRESS                 DataBufPhyAddr;
> +  EDKII_UFS_HOST_CONTROLLER_OPERATION  Flag;
> +  UTP_TR_PRD                           *PrdtBase;
> +
> +  DataBufPhyAddr = (EFI_PHYSICAL_ADDRESS) NULL;
> +  AlignedDataBuf = NULL;
> +  DataBuf        = NULL;
> +
> +  //
> +  // For unaligned data transfers we allocate auxilary DWORD aligned
> memory pool.
> +  // When command is finished auxilary memory pool is copied into actual
> user memory.
> +  // This is requiered to assure data transfer safety(DWORD alignment
> required by UFS spec.)
> +  //
> +  if (TransReq->Packet->DataDirection ==
> EFI_EXT_SCSI_DATA_DIRECTION_READ) {
> +    if (TransReq->Packet->InTransferLength % 4 != 0) {
> +      DataLen = TransReq->Packet->InTransferLength + (4 - (TransReq->Packet-
> >InTransferLength % 4));
> +      DataBuf = AllocateZeroPool (DataLen);

The above allocated buffer may not meet the alignment requirement. It is
possible to lead to the check failure within function UfsInitUtpPrdt().

Please consider using AllocateAlignedPages() and aligns to the 'IoAlign'
field within EFI_EXT_SCSI_PASS_THRU_MODE.


> +      if (DataBuf == NULL) {
> +        return EFI_DEVICE_ERROR;
> +      }
> +      TransReq->AlignedDataBuf = DataBuf;
> +    } else {
> +      DataLen       = TransReq->Packet->InTransferLength;
> +      DataBuf       = TransReq->Packet->InDataBuffer;
> +    }
> +    Flag          = EdkiiUfsHcOperationBusMasterWrite;
> +  } else {
> +    if (TransReq->Packet->OutTransferLength % 4 != 0) {
> +      DataLen = TransReq->Packet->OutTransferLength + (4 - (TransReq-
> >Packet->OutTransferLength % 4));
> +      DataBuf = AllocateCopyPool (TransReq->Packet->OutTransferLength,
> TransReq->Packet->OutDataBuffer);

It seems to me that the size of 'DataBuf' is not dword aligned.

Also, similarly, the above allocated buffer may not meet the alignment
requirement.


> +      if (DataBuf == NULL) {
> +        return EFI_DEVICE_ERROR;
> +      }
> +      TransReq->AlignedDataBuf = DataBuf;
> +    } else {
> +      DataLen       = TransReq->Packet->OutTransferLength;
> +      DataBuf       = TransReq->Packet->OutDataBuffer;
> +    }
> +    Flag          = EdkiiUfsHcOperationBusMasterRead;
> +  }
> +
> +  if (DataLen != 0) {
> +    MapLength = DataLen;
> +    Status    = Private->UfsHostController->Map (
> +                                              Private->UfsHostController,
> +                                              Flag,
> +                                              DataBuf,
> +                                              &MapLength,
> +                                              &DataBufPhyAddr,
> +                                              &TransReq->DataBufMapping
> +                                              );
> +
> +    if (EFI_ERROR (Status) || (DataLen != MapLength)) {
> +      return EFI_DEVICE_ERROR;

Need to handle the free of the auxiliary buffer for this case, since it
will not be covered by function UfsReconcileDataTransferBuffer().

Best Regards,
Hao Wu


> +    }
> +  }
> +
> +  //
> +  // Fill PRDT table of Command UPIU for executed SCSI cmd.
> +  //
> +  PrdtBase = (UTP_TR_PRD*)((UINT8*)TransReq->CmdDescHost + ROUNDUP8
> (sizeof (UTP_COMMAND_UPIU)) + ROUNDUP8 (sizeof (UTP_RESPONSE_UPIU)));
> +  ASSERT (PrdtBase != NULL);
> +  UfsInitUtpPrdt (PrdtBase, (VOID*)(UINTN)DataBufPhyAddr, DataLen);
> +
> +  return EFI_SUCCESS;
> +}
> +
>  /**
>    Sends a UFS-supported SCSI Request Packet to a UFS device that is attached to
> the UFS host controller.
> 
> @@ -1359,24 +1476,19 @@ UfsExecScsiCmds (
>    UTP_RESPONSE_UPIU                    *Response;
>    UINT16                               SenseDataLen;
>    UINT32                               ResTranCount;
> -  VOID                                 *DataBuf;
> -  EFI_PHYSICAL_ADDRESS                 DataBufPhyAddr;
> -  UINT32                               DataLen;
> -  UINTN                                MapLength;
> -  EDKII_UFS_HOST_CONTROLLER_PROTOCOL   *UfsHc;
> -  EDKII_UFS_HOST_CONTROLLER_OPERATION  Flag;
> -  UTP_TR_PRD                           *PrdtBase;
>    EFI_TPL                              OldTpl;
>    UFS_PASS_THRU_TRANS_REQ              *TransReq;
> +  EDKII_UFS_HOST_CONTROLLER_PROTOCOL   *UfsHc;
> 
> -  TransReq       = AllocateZeroPool (sizeof (UFS_PASS_THRU_TRANS_REQ));
> +  TransReq = AllocateZeroPool (sizeof (UFS_PASS_THRU_TRANS_REQ));
>    if (TransReq == NULL) {
>      return EFI_OUT_OF_RESOURCES;
>    }
> 
>    TransReq->Signature     = UFS_PASS_THRU_TRANS_REQ_SIG;
>    TransReq->TimeoutRemain = Packet->Timeout;
> -  DataBufPhyAddr = 0;
> +  TransReq->Packet        = Packet;
> +
>    UfsHc          = Private->UfsHostController;
>    //
>    // Find out which slot of transfer request list is available.
> @@ -1405,44 +1517,16 @@ UfsExecScsiCmds (
> 
>    TransReq->CmdDescSize = TransReq->Trd->PrdtO * sizeof (UINT32) +
> TransReq->Trd->PrdtL * sizeof (UTP_TR_PRD);
> 
> -  if (Packet->DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_READ) {
> -    DataBuf       = Packet->InDataBuffer;
> -    DataLen       = Packet->InTransferLength;
> -    Flag          = EdkiiUfsHcOperationBusMasterWrite;
> -  } else {
> -    DataBuf       = Packet->OutDataBuffer;
> -    DataLen       = Packet->OutTransferLength;
> -    Flag          = EdkiiUfsHcOperationBusMasterRead;
> -  }
> -
> -  if (DataLen != 0) {
> -    MapLength = DataLen;
> -    Status    = UfsHc->Map (
> -                         UfsHc,
> -                         Flag,
> -                         DataBuf,
> -                         &MapLength,
> -                         &DataBufPhyAddr,
> -                         &TransReq->DataBufMapping
> -                         );
> -
> -    if (EFI_ERROR (Status) || (DataLen != MapLength)) {
> -      goto Exit1;
> -    }
> +  Status = UfsPrepareDataTransferBuffer (Private, TransReq);
> +  if (EFI_ERROR (Status)) {
> +    goto Exit1;
>    }
> -  //
> -  // Fill PRDT table of Command UPIU for executed SCSI cmd.
> -  //
> -  PrdtBase = (UTP_TR_PRD*)((UINT8*)TransReq->CmdDescHost + ROUNDUP8
> (sizeof (UTP_COMMAND_UPIU)) + ROUNDUP8 (sizeof (UTP_RESPONSE_UPIU)));
> -  ASSERT (PrdtBase != NULL);
> -  UfsInitUtpPrdt (PrdtBase, (VOID*)(UINTN)DataBufPhyAddr, DataLen);
> 
>    //
>    // Insert the async SCSI cmd to the Async I/O list
>    //
>    if (Event != NULL) {
>      OldTpl = gBS->RaiseTPL (TPL_NOTIFY);
> -    TransReq->Packet      = Packet;
>      TransReq->CallerEvent = Event;
>      InsertTailList (&Private->Queue, &TransReq->TransferList);
>      gBS->RestoreTPL (OldTpl);
> @@ -1521,9 +1605,7 @@ Exit:
> 
>    UfsStopExecCmd (Private, TransReq->Slot);
> 
> -  if (TransReq->DataBufMapping != NULL) {
> -    UfsHc->Unmap (UfsHc, TransReq->DataBufMapping);
> -  }
> +  UfsReconcileDataTransferBuffer (Private, TransReq);
> 
>  Exit1:
>    if (TransReq->CmdDescMapping != NULL) {
> @@ -1538,7 +1620,6 @@ Exit1:
>    return Status;
>  }
> 
> -
>  /**
>    Sent UIC DME_LINKSTARTUP command to start the link startup procedure.
> 
> @@ -2106,7 +2187,6 @@ UfsControllerStop (
>    return EFI_SUCCESS;
>  }
> 
> -
>  /**
>    Internal helper function which will signal the caller event and clean up
>    resources.
> @@ -2138,9 +2218,7 @@ SignalCallerEvent (
> 
>    UfsStopExecCmd (Private, TransReq->Slot);
> 
> -  if (TransReq->DataBufMapping != NULL) {
> -    UfsHc->Unmap (UfsHc, TransReq->DataBufMapping);
> -  }
> +  UfsReconcileDataTransferBuffer (Private, TransReq);
> 
>    if (TransReq->CmdDescMapping != NULL) {
>      UfsHc->Unmap (UfsHc, TransReq->CmdDescMapping);
> --
> 2.14.1.windows.1



^ permalink raw reply	[flat|nested] 11+ messages in thread
end of thread, other threads:[~2019-04-15  5:56 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
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
     [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
  -- strict thread matches above, loose matches on Subject: below --
2019-04-01  6:53 [PATCH 1/3] MdeModulePkg/UfsPassThruDxe: Fix unaligned data transfer handling Wu, Hao A

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox