Re: [edk2-devel] [PATCH 3/4] MdeModulePkg/SdMmcPciHcDxe: Refactor data transfer completion

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

Just a similar question to PATCH 2/4 below:


> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> Albecki, Mateusz
> Sent: Monday, February 03, 2020 10:19 PM
> To: devel@edk2.groups.io
> Cc: Albecki, Mateusz; Wu, Hao A; Marcin Wojtas; Gao, Zhichao; Gao, Liming
> Subject: [edk2-devel] [PATCH 3/4] MdeModulePkg/SdMmcPciHcDxe:
> Refactor data transfer completion
> 
> This patch refactors the way in which the driver will check
> the data transfer completion. Data transfer related
> functionalities have been moved to separate function.
> 
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Marcin Wojtas <mw@semihalf.com>
> Cc: Zhichao Gao <zhichao.gao@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> 
> Signed-off-by: Mateusz Albecki <mateusz.albecki@intel.com>
> ---
>  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c | 181
> ++++++++++++++---------
>  1 file changed, 112 insertions(+), 69 deletions(-)
> 
> diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> index 3dfaae8542..480a1664ea 100644
> --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> @@ -2447,6 +2447,112 @@ SdMmcCheckCommandComplete (
>    return EFI_NOT_READY;
>  }
> 
> +/**
> +  Update the SDMA address on the SDMA buffer boundary interrupt.
> +
> +  @param[in] Private    A pointer to the SD_MMC_HC_PRIVATE_DATA
> instance.
> +  @param[in] Trb        The pointer to the SD_MMC_HC_TRB instance.
> +
> +  @retval EFI_SUCCESS  Updated SDMA buffer address.
> +  @retval Others       Failed to update SDMA buffer address.
> +**/
> +EFI_STATUS
> +SdMmcUpdateSdmaAddress (
> +  IN SD_MMC_HC_PRIVATE_DATA  *Private,
> +  IN SD_MMC_HC_TRB           *Trb
> +  )
> +{
> +  UINT64      SdmaAddr;
> +  EFI_STATUS  Status;
> +
> +  SdmaAddr = SD_MMC_SDMA_ROUND_UP ((UINTN)Trb->DataPhy,
> SD_MMC_SDMA_BOUNDARY);
> +
> +  if (Private->ControllerVersion[Trb->Slot] >= SD_MMC_HC_CTRL_VER_400)
> {
> +    Status = SdMmcHcRwMmio (
> +               Private->PciIo,
> +               Trb->Slot,
> +               SD_MMC_HC_ADMA_SYS_ADDR,
> +               FALSE,
> +               sizeof (UINT64),
> +               &SdmaAddr
> +               );
> +  } else {
> +    Status = SdMmcHcRwMmio (
> +               Private->PciIo,
> +               Trb->Slot,
> +               SD_MMC_HC_SDMA_ADDR,
> +               FALSE,
> +               sizeof (UINT32),
> +               &SdmaAddr
> +               );
> +  }
> +
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  Trb->DataPhy = (UINT64)(UINTN)SdmaAddr;
> +  return EFI_SUCCESS;
> +}
> +
> +/**
> +  Checks if the data transfer completed and performs any actions
> +  neccessary to continue the data transfer such as SDMA system
> +  address fixup or PIO data transfer.
> +
> +  @param[in] Private    A pointer to the SD_MMC_HC_PRIVATE_DATA
> instance.
> +  @param[in] Trb        The pointer to the SD_MMC_HC_TRB instance.
> +  @param[in] IntStatus  Snapshot of the normal interrupt status register.
> +
> +  @retval EFI_SUCCESS   Data transfer completed successfully.
> +  @retval EFI_NOT_READY Data transfer completion still pending.
> +  @retval Others        Data transfer failed to complete.
> +**/
> +EFI_STATUS
> +SdMmcCheckDataTransfer (
> +  IN SD_MMC_HC_PRIVATE_DATA  *Private,
> +  IN SD_MMC_HC_TRB           *Trb,
> +  IN UINT16                  IntStatus
> +  )
> +{
> +  UINT16      Data16;
> +  EFI_STATUS  Status;
> +
> +  if ((IntStatus & BIT1) != 0) {
> +    Data16 = BIT1;
> +    Status = SdMmcHcRwMmio (
> +               Private->PciIo,
> +               Trb->Slot,
> +               SD_MMC_HC_NOR_INT_STS,
> +               FALSE,
> +               sizeof (Data16),
> +               &Data16
> +               );


Cleaning the Transfer Complete (BIT1) right after checking it is more aligned
with the spec, right?

Best Regards,
Hao Wu


> +    return Status;
> +  }
> +
> +  if ((Trb->Mode == SdMmcSdmaMode) && ((IntStatus & BIT3) != 0)) {
> +    Data16 = BIT3;
> +    Status = SdMmcHcRwMmio (
> +               Private->PciIo,
> +               Trb->Slot,
> +               SD_MMC_HC_NOR_INT_STS,
> +               FALSE,
> +               sizeof (Data16),
> +               &Data16
> +               );
> +    if (EFI_ERROR (Status)) {
> +      return Status;
> +    }
> +    Status = SdMmcUpdateSdmaAddress (Private, Trb);
> +    if (EFI_ERROR (Status)) {
> +      return Status;
> +    }
> +  }
> +
> +  return EFI_NOT_READY;
> +}
> +
>  /**
>    Check the TRB execution result.
> 
> @@ -2467,7 +2573,6 @@ SdMmcCheckTrbResult (
>    EFI_STATUS                          Status;
>    EFI_SD_MMC_PASS_THRU_COMMAND_PACKET *Packet;
>    UINT16                              IntStatus;
> -  UINT64                              SdmaAddr;
>    UINT32                              PioLength;
> 
>    Packet  = Trb->Packet;
> @@ -2530,80 +2635,18 @@ SdMmcCheckTrbResult (
>      Status = SdMmcCheckCommandComplete (Private, Trb, IntStatus);
>      if (EFI_ERROR (Status)) {
>        goto Done;
> -    } else {
> -      //
> -      // If the command doesn't require data transfer skip the transfer
> -      // complete checking.
> -      //
> -      if ((Packet->SdMmcCmdBlk->CommandType !=
> SdMmcCommandTypeAdtc) &&
> -          (Packet->SdMmcCmdBlk->ResponseType != SdMmcResponseTypeR1b)
> &&
> -          (Packet->SdMmcCmdBlk->ResponseType != SdMmcResponseTypeR5b))
> {
> -        goto Done;
> -      }
>      }
>    }
> 
> -  //
> -  // Check Transfer Complete bit is set or not.
> -  //
> -  if ((IntStatus & BIT1) == BIT1) {
> -    goto Done;
> -  }
> -
> -  //
> -  // Check if DMA interrupt is signalled for the SDMA transfer.
> -  //
> -  if ((Trb->Mode == SdMmcSdmaMode) && ((IntStatus & BIT3) == BIT3)) {
> -    //
> -    // Clear DMA interrupt bit.
> -    //
> -    IntStatus = BIT3;
> -    Status    = SdMmcHcRwMmio (
> -                  Private->PciIo,
> -                  Trb->Slot,
> -                  SD_MMC_HC_NOR_INT_STS,
> -                  FALSE,
> -                  sizeof (IntStatus),
> -                  &IntStatus
> -                  );
> -    if (EFI_ERROR (Status)) {
> -      goto Done;
> -    }
> -    //
> -    // Update SDMA Address register.
> -    //
> -    SdmaAddr = SD_MMC_SDMA_ROUND_UP ((UINTN)Trb->DataPhy,
> SD_MMC_SDMA_BOUNDARY);
> -
> -    if (Private->ControllerVersion[Trb->Slot] >= SD_MMC_HC_CTRL_VER_400)
> {
> -      Status = SdMmcHcRwMmio (
> -                 Private->PciIo,
> -                 Trb->Slot,
> -                 SD_MMC_HC_ADMA_SYS_ADDR,
> -                 FALSE,
> -                 sizeof (UINT64),
> -                 &SdmaAddr
> -                 );
> -    } else {
> -      Status = SdMmcHcRwMmio (
> -                 Private->PciIo,
> -                 Trb->Slot,
> -                 SD_MMC_HC_SDMA_ADDR,
> -                 FALSE,
> -                 sizeof (UINT32),
> -                 &SdmaAddr
> -                 );
> -    }
> -
> -    if (EFI_ERROR (Status)) {
> -      goto Done;
> -    }
> -    Trb->DataPhy = (UINT64)(UINTN)SdmaAddr;
> +  if (Packet->SdMmcCmdBlk->CommandType == SdMmcCommandTypeAdtc
> ||
> +      Packet->SdMmcCmdBlk->ResponseType == SdMmcResponseTypeR1b ||
> +      Packet->SdMmcCmdBlk->ResponseType == SdMmcResponseTypeR5b) {
> +    Status = SdMmcCheckDataTransfer (Private, Trb, IntStatus);
> +  } else {
> +    Status = EFI_SUCCESS;
>    }
> 
> -
> -  Status = EFI_NOT_READY;
>  Done:
> -
>    if (Status != EFI_NOT_READY) {
>      SdMmcHcLedOnOff (Private->PciIo, Trb->Slot, FALSE);
>      if (EFI_ERROR (Status)) {
> --
> 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.
> 
> 
>